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

allow multiple DKGs with partial participants to support split #330

Merged
merged 2 commits into from Jul 11, 2019

Conversation

Projects
None yet
3 participants
@jeanphilippeD
Copy link
Contributor

commented Jul 5, 2019

Closes #326
Test and fix the code to allow DKG support for Routing section split.

jeanphilippeD added some commits Jul 5, 2019

feat/parsec: Handle and test DKG with some participants
Add new test to cover 2 DKGs from the subsets of member that will form
the new elders after a section split. To maintain the invariant that
all blocks are the same for all peer, ignore the secret key state:
For the participants in a DKG it will be Some, for the others it will be
None.

Add `participants` to the `Observation::DkgResult` so user and test
can identify what result is for what. This create a link with the
`Observation::StartDkg` event.

clippy and test pass.
refactor/parsec: Add DkgResultWrapper to provide the needed traits
For Block, DkgResultWrapper only consider the public key set for its
comparaison operation and serialisation. These are mostly done to
satisfy Observation requirements and allow the tests comparing blocks
from different peers to pass when DKG is run.

Clarify that these don't quite represent well DkgResult operations by
implementing them only in a wrapper.

clippy and test pass.

@jeanphilippeD jeanphilippeD requested a review from fizyk20 Jul 5, 2019

Show resolved Hide resolved src/key_gen/dkg_result.rs
Show resolved Hide resolved tests/integration_tests.rs Outdated

@jeanphilippeD jeanphilippeD force-pushed the jeanphilippeD:allow_split_dkg branch from d28e103 to b8cd8c2 Jul 8, 2019

@jeanphilippeD

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Ok, fixed the small comments with a push-force.

@jeanphilippeD jeanphilippeD requested a review from maqi Jul 8, 2019

@@ -29,9 +29,12 @@ pub struct Block<T: NetworkEvent, P: PublicId> {

impl<T: NetworkEvent, P: PublicId> Block<T, P> {
/// Create a `Block` with no signatures for a single DkgResult
pub fn new_dkg_block(result: DkgResult) -> Self {
pub fn new_dkg_block((participants, dkg_result): (BTreeSet<P>, DkgResult)) -> Self {

This comment has been minimized.

Copy link
@maqi

maqi Jul 9, 2019

Member

I thought we normally take the format of participants: BTreeSet<P>, dkg_result: DkgResult, instead of a pair?

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Jul 9, 2019

Author Contributor

I can, but it mean we have to destructure the tuple at the call site.
Is there an issue with this way of doing it?

This comment has been minimized.

Copy link
@maqi

maqi Jul 9, 2019

Member

no issue, just a preference I think.

Show resolved Hide resolved src/key_gen/dkg_result.rs
Show resolved Hide resolved src/key_gen/tests.rs
/// have one assuming less than 1/3 malicious.
DkgResult(DkgResult),
DkgResult {
/// Participants in DKG that have a secret key.

This comment has been minimized.

Copy link
@maqi

maqi Jul 9, 2019

Member

This comment indicates there could be participants has no secret key?

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Jul 9, 2019

Author Contributor

Parsec voters that will also compute a DkgResult block but are not in the participants list will not have a secret key.

This comment has been minimized.

Copy link
@maqi

maqi Jul 9, 2019

Member

Ah, I see. hmm could it be better to put this sentence as comment as well?

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Jul 10, 2019

Author Contributor

Rephrased

Show resolved Hide resolved src/observation.rs
Show resolved Hide resolved tests/integration_tests.rs
@jeanphilippeD

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Address minor comment, @maqi could you re-approve.

@maqi

maqi approved these changes Jul 11, 2019

@jeanphilippeD jeanphilippeD merged commit a8e5829 into maidsafe:master Jul 11, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jeanphilippeD jeanphilippeD deleted the jeanphilippeD:allow_split_dkg branch Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.