Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Import CPU code #292
Changes from 5 commits
4b91715
5083ee9
2465541
f44dd6d
818351d
9c5ab82
378dce4
c06cc23
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 theTestNetwork
and then refactor the network according to needs.See https://github.com/Inversed-Tech/iris-hawk/blob/main/src/aby3_store.rs for reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are ways to do that, I've done a few times using a
DashMap
https://docs.rs/dashmap/latest/dashmap/ https://github.com/tf-encrypted/moose/blob/main/moose/src/networking/local.rsThere was a problem hiding this comment.
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#L146There was a problem hiding this comment.
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 toless_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.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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