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

Optimization: output early from Subset? #170

Closed
afck opened this issue Jul 29, 2018 · 7 comments · Fixed by #233
Closed

Optimization: output early from Subset? #170

afck opened this issue Jul 29, 2018 · 7 comments · Fixed by #233
Labels
good first issue Good for newcomers

Comments

@afck
Copy link
Collaborator

afck commented Jul 29, 2018

Subset could output multiple times: Once for each accepted contribution, and then once more when it terminates, to say that the remaining contributions have been rejected. That way, Honey Badger can start exchanging decryption shares sooner, and by the time the agreement instances for lagging nodes' contributions finally output false, the accepted ones will already be decrypted and ready to be output.

(We should measure, of course, whether it actually improves performance and whether it's worth it.)

What needs to be changed

The Subset output needs to be an enum with two variants, e.g. Contribution(Vec<u8>) and Done.

Instead of outputting all accepted contributions when all have been decided, we should output a single Contribution(_) as soon as it gets accepted, and then only output an empty Done once all have been decided.

In Honey Badger, we wouldn't set the SubsetState to Complete on a Contribution(_) output, but only on Done.

The simulation example should be run a few times with and without the change, and maybe with different parameters, to test whether it improves performance.

@afck afck added the good first issue Good for newcomers label Aug 27, 2018
@afck afck changed the title Optimization: output early from Common Subset? Optimization: output early from Subset? Aug 27, 2018
@DemiMarie
Copy link
Contributor

I will take this one.

@DemiMarie
Copy link
Contributor

DemiMarie commented Sep 13, 2018

I am slightly confused: if I return from

hbbft/src/subset.rs

Lines 255 to 257 in 422d8ef

if let Some(output) = output.into_iter().next() {
output
} else {
the compiler (rightly) warns about unreachable code. Where should the code that comes afterward be placed?

(Sorry, I know very little about the codebase)

@c0gent
Copy link
Contributor

c0gent commented Sep 13, 2018

Could you clarify your question please and give an example of exactly what you're trying to do?

@vkomenda
Copy link
Contributor

@DemiMarie, if you return from the if let Some(output) clause then value needs to become a function, not a let-binding, because the else clause also returns. Hence the code after value is unreachable. Please correct if I understood something wrong.

@afck
Copy link
Collaborator Author

afck commented Sep 16, 2018

How confident is everyone that this doesn't hurt censorship resistance?
I imagine it shouldn't make a difference, because even though the decrypted content of one accepted contribution can now influence your vote on the others, you never know the contents of any contribution while you can still influence its outcome.

But the fact that I can't imagine a problem doesn't mean there is none.
(No need to hold off with merging this, fo course: If we have doubts, we could always make this a configuration option later.)

@c0gent
Copy link
Contributor

c0gent commented Sep 16, 2018

I can't think of a problem either but since this change may not be that much of an optimization anyway I think keeping it an option or build feature is best.

@afck
Copy link
Collaborator Author

afck commented Sep 16, 2018

As discussed let's make it a runtime option. This will need to be passed through from the HoneyBadger builder down to Subset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants