Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import CPU code #292

Merged
merged 8 commits into from
Sep 4, 2024
Merged

Import CPU code #292

merged 8 commits into from
Sep 4, 2024

Conversation

dkales
Copy link
Collaborator

@dkales dkales commented Aug 29, 2024

No description provided.

@github-actions github-actions bot added the enhancement New feature or request label Aug 29, 2024
iris-mpc-cpu/src/error.rs Outdated Show resolved Hide resolved
iris-mpc-cpu/src/error.rs Outdated Show resolved Hide resolved
};

impl<N: NetworkTrait> IrisWorker<N> {
pub async fn async_rep3_dot(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want both async and sync variants of these functions? Also, do we even want both REP3 and Shamir dot products?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for starters wanted to get the code going with rep3, then we could switch to Shamir. And regarding sync/vs async - would like to avoid blocking code, so prefer async instead of sync functions.

iris-mpc-cpu/src/protocol/iris_worker.rs Outdated Show resolved Hide resolved
Comment on lines +13 to +31
async fn send(&mut self, id: PartyID, data: Bytes) -> Result<(), IoError>;
async fn send_next_id(&mut self, data: Bytes) -> Result<(), IoError>;
async fn send_prev_id(&mut self, data: Bytes) -> Result<(), IoError>;

async fn receive(&mut self, id: PartyID) -> Result<BytesMut, IoError>;
async fn receive_prev_id(&mut self) -> Result<BytesMut, IoError>;
async fn receive_next_id(&mut self) -> Result<BytesMut, IoError>;

async fn broadcast(&mut self, data: Bytes) -> Result<Vec<BytesMut>, IoError>;
//======= sync world =========
fn blocking_send(&mut self, id: PartyID, data: Bytes) -> Result<(), IoError>;
fn blocking_send_next_id(&mut self, data: Bytes) -> Result<(), IoError>;
fn blocking_send_prev_id(&mut self, data: Bytes) -> Result<(), IoError>;

fn blocking_receive(&mut self, id: PartyID) -> Result<BytesMut, IoError>;
fn blocking_receive_prev_id(&mut self) -> Result<BytesMut, IoError>;
fn blocking_receive_next_id(&mut self) -> Result<BytesMut, IoError>;

fn blocking_broadcast(&mut self, data: Bytes) -> Result<Vec<BytesMut>, IoError>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also maybe consider reducing this down to the actual networking impl set that we want to have (async/sync)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, there is no concrete impl here other than the test-network right? What is the plan here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, there's not concrete impl other than that. We were thinking that the plan could be to get in the Aby3Store implementation which uses the TestNetwork and then refactor the network according to needs.
See https://github.com/Inversed-Tech/iris-hawk/blob/main/src/aby3_store.rs for reference

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is something for the infra team to dictate, in our impl we used QUIC using the quinn library as a concrete impl, but I guess just standard TLS over TCP is also fine. I think there was some talk of actually using NCCL here, but I don't think the ergonomics (and probably also perf) are very nice

Copy link
Collaborator

@rdragos rdragos Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think why I like using &self instead of &mut self is that it gets rid of the locks when doing operations on a global network that I'm doing: https://github.com/Inversed-Tech/iris-hawk/blob/main/src/aby3_store.rs#L146

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, mostly because the interface of hawk-pack is &self. But there would still need to be a lot of syncing internally otherwise you cannot use the networking impl for MT code anyway, as concurrent calls to less_than would use the same networking and therefore may receive messages in the wrong order. So a correct implementation would need to separate the channels/multiplex the networking correctly anyway using internal locking etc. I think it would be easier to have the lock in there, and e.g., grab a new channel from the "global" network manager that can be used for the local execution here. The "global" network manager can then internally use the locks to allow a &self bound, get a new channel for the protocol and execute it on that Channel it exclusively owns.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I agree. The global network manager should have the locking logic inside of it, rather than spilling into other parts of the code.
Is this already done somewhere, or do you reckon this would be part of a large refactoring?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no sadly this is not implemented right now

@dkales dkales marked this pull request as ready for review September 4, 2024 15:17
@dkales dkales requested a review from philsippl September 4, 2024 15:18
@dkales
Copy link
Collaborator Author

dkales commented Sep 4, 2024

@philsippl, @rdragos wants to merge this as a baseline to build the hawk-pack interface on, anything holding this back?

@philsippl
Copy link
Contributor

@dkales sounds good, don't see see a reason not to merge>

@dkales dkales merged commit 3793432 into main Sep 4, 2024
13 checks passed
@dkales dkales deleted the feat/cpu branch September 4, 2024 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants