-
Notifications
You must be signed in to change notification settings - Fork 96
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
Make Subset optimization optional #250
Make Subset optimization optional #250
Conversation
src/honey_badger/epoch_state.rs
Outdated
|
||
finalize_subset_with_step!(); | ||
} | ||
}, |
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.
Maybe this could be simplified—and the macros, Self
destructuring and static methods avoided—if SubsetHandler
had a handle
method that takes a SubsetOutput
and returns a Vec<SubsetOutput>
:
- If it's
Incremental
, it returns its argument immediately. - If it's
AllAtEnd
it stores eachContribution
input, and then returns all of them later, whenDone
arrives.
We wouldn't need an explicit distinction here, and the code could remain almost unchanged.
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.
That makes sense -- I thought about doing that and decided to accept the complexity to avoid heap allocations and copying. You definitely are more familiar with what tradeoffs the codebase needs, though, so I'll make that change once I get to a dev machine.
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 think that would be premature optimization: The bottleneck in hbbft is on way lower layers (mainly in the cryptography), so our priority should be to keep the code simple.
@afck: I've changed the static method/macro binding to be |
Rebased changes to what has been pulled in today. |
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.
Looks great!
Let's just make sure everything is documented, in particular public types and methods.
@@ -58,26 +60,42 @@ where | |||
self | |||
} | |||
|
|||
pub fn subset_handling_strategy( |
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.
Please add a short doc comment explaining subset handling. (Also in HoneyBadgerBuilder
.)
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 noticed I was missing documentation and intended to get to it today -- thank you for reminding me! :)
@afck: I've added doc comments written in terms of what I've learned about internals, but would it be better/more user-friendly if they were written in terms of how That could be saved for a future PR, of course -- just thinking out loud. |
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.
Looks good to me! The comments are fine; we can add further explanations to them in a separate PR, if needed.
@vkomenda: Feel free to merge if you're happy with it.
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.
Looks very good. I like it!
Let's wait for the CI to complete tests. drop_and_readd failed again and I restarted the CI job.
Fixes #243. Allow client code to control the behavior of
EpochState
and when it decides to send decryption shares. There are two options for the behavior for handling the output of aSubset
algorithm, which are represented by the newSubsetHandlingStrategy
(bikeshedding welcome!) enum:Incremental
, which is the current (optimized) behavior.AllAtEnd
, which is supposed to be the old behavior. Reviewers crunched for time might want to place priority on making sure I've done this right.SubsetHandlingStrategy
is exposed at the builder level. Internally, anEpochState
instance now stores aSubsetHandler
enum member that models the necessary state for each different strategy. To resolve ownership issues, I've changedEpochState::{process_decryption,send_decryption_share}
to take members of theEpochState
struct instead of the entire struct (self
). I've also created thefinalize_subset_with_step
method which also uses members and notself
.Tests compile, but I'm currently blocked on #249 for testing locally until I get home and have access to my *nix machines.Current concerns