DKG proof of concept implementation #2

maqi · 2020-03-23T11:30:24Z

This PR is trying to provide a POC implemenation of a DKG protocol, following the one as suggested at
https://github.com/dashpay/dips/blob/master/dip-0006/bls_m-of-n_threshold_scheme_and_dkg.md#distributed-key-generation-dkg-protocol

This is the attempt trying to address the routing issue of have 50% threshold in BLS set (maidsafe/sn_routing#2045)

- Fixes inconsistencies and typos. - Aligns with original paper. - Clarifies abbreviations and similar.

Yoga07

LGTM! 👍

src/key_gen/mod.rs

src/dev_utils/peer.rs

dirvine

Some small changes, mainly not use seeded rng and defo not maidsafe_utils

src/key_gen/tests.rs

Cargo.toml

dirvine · 2020-04-13T16:42:13Z

Cargo.toml

+bincode = "1.1.4"
+failure = "~0.1.5"
+lazy_static = "~1.2.0"
+rand = "~0.4.2"


Also use latest rand please.

My suggestion would be to use the same version of rand that threshold_crypto uses. Then we wouldn't need the RngAdapter wrapper leading to somehow simpler code. The last published version of threshold_crypto (0.3.2) uses rand v0.6.5.

but the latest rand is at 0.7.3, and seems threshold_crypto depends on a bit old rand_core version.
So I'd suggest keep the RngAdapter, so that the latest rand can be used within this crate (avoid a potential adapter outside this crate)

OK, I don't mind that. It's also worth keeping an eye on the threshold_crypto crate because it seems they already use rand 0.7.3 in master. So likely the next published version will have it too.

Cargo.toml

madadam

Some comments mostly about style, naming, consistency and documentation. Not much about the logic itself which I would have to spend more time on. Maybe a followup PR can expand the test suite (possibly using property testing too) to give us more confidence in the correctness of the algorithm,

Cargo.toml

madadam · 2020-04-14T08:43:02Z

src/key_gen/mod.rs

+    }
+
+    // TODO: so far this function has to be called externally to indicates a completion of the
+    //       contribution phase. May need to be further verified whether there is a better approach.


It's not clear to me when this function should be called. Maybe add more detailed explanation?

src/key_gen/mod.rs

madadam · 2020-04-14T08:45:34Z

src/key_gen/mod.rs

+///   d, call `finalize_complaining_phase` to complete the complaining phase. (This separate call may need to
+///      depend on a separate timer & checker against the key generator's current status)
+///   e, repeat step c when there is incoming `DkgMessage`.
+///   f, call `generate` to get the public-key set and secret-key share, if the procedure finalized.


if the procedure finalized.

How do I know it's finalized?

call generate

The function is called generate_keys

How do I know it's finalized?

have to wait for a fixed interval or keeps polling.
This is same as to when the finalize_complaining_phase shall be called.

src/key_gen/message.rs

src/key_gen/tests.rs

S-Coyle · 2020-04-14T09:53:40Z

@maqi A note on licensing of this repo - the license file says BSD-3, but the license blurb at the top of each of your source code files says GPL v3.

@dirvine can you confirm it's deliberate that this repo is BSD-3 only, and not MIT or BSD-3 as per the majority of our other repos.

dirvine · 2020-04-14T09:55:42Z

You are right @S-Coyle it should be dual bsd-3/mit

dirvine

Not sure why my comments don't show at time so adding new review here

S-Coyle · 2020-04-14T10:06:21Z

@maqi given the above, perhaps you can update the license files as part of this PR to match that of all our duel BSD/MIT repos, and of course update the license blurb at the top of each source code file

src/dev_utils/peer.rs

madadam · 2020-04-14T13:19:17Z

src/key_gen/mod.rs

+    /// Returns the new secret key share and the public key set.
+    pub fn generate_keys(&self) -> Result<(BTreeSet<S::PublicId>, Outcome), Error> {
+        if self.phase != Phase::Finalization {
+            return Err(Error::Unknown);


Should we maybe have more specific error for this? Something like OutcomeNotReady. Or alternatively, change the return type to Option as there seems to be only two possibilities: either the outcome is available or it is not.

right, will consider whether it will be a more explicit error or an option to be more suitable, within the next PR

madadam · 2020-04-14T13:21:15Z

src/key_gen/mod.rs

+    //       contribution phase. That is, the owner of the key_gen instance has to wait for a fixed
+    //       interval, say an expected timer of 5 minutes, to allow the messages to be exchanged.
+    //       May need to be further verified whether there is a better approach.
+    pub fn finalize_contributing_phase<R: RngCore>(


I wonder if it would be possible to reduce the API surface a bit by merging the finalize_contributing_phase and finalize_complaining_phase into one function which automatically detects what to do based on the current phase. Would that make sense?

good idea, will give a try within the next PR

madadam

Left two suggestions which don't necessarily need to be addressed in this PR. Approving already.

maqi changed the title ~~DKG proof of concept implementation~~ WIP DKG proof of concept implementation Mar 23, 2020

maqi changed the title ~~WIP DKG proof of concept implementation~~ DKG proof of concept implementation Apr 6, 2020

maqi and others added 8 commits April 6, 2020 22:55

poc first draft

30592a7

adding the Complain and Justificate phases

7c00d7e

non-contributing member shall be considered as invalid

15305dd

fix/align/clarify domain language

18a4ff0

- Fixes inconsistencies and typos. - Aligns with original paper. - Clarifies abbreviations and similar.

resovling fixme points

bf3d25b

test/key_gen: threshold_signature unit test

b614fc5

test/key_gen: unit test for group_encrypt

67ea901

test/key_gen: more unit tests

48d7bf4

maqi force-pushed the poc branch from db23a3f to dd444b7 Compare April 6, 2020 15:02

chore/ci: clippy and tests scripts

59047d7

maqi force-pushed the poc branch from dd444b7 to 59047d7 Compare April 6, 2020 16:19

maqi requested a review from Yoga07 April 7, 2020 09:37

Yoga07 previously approved these changes Apr 13, 2020

View reviewed changes

dirvine reviewed Apr 13, 2020

View reviewed changes

src/key_gen/mod.rs Outdated Show resolved Hide resolved

dirvine reviewed Apr 13, 2020

View reviewed changes

src/dev_utils/peer.rs Outdated Show resolved Hide resolved

maqi requested a review from madadam April 13, 2020 08:28

dirvine requested changes Apr 13, 2020

View reviewed changes

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

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

maqi dismissed Yoga07’s stale review via 6484e36 April 13, 2020 12:23

fix/key_gen: address review comments

1033b11

maqi force-pushed the poc branch from 6484e36 to 1033b11 Compare April 13, 2020 15:03

feat/key_gen: when DKG procedure blocked, return potential non-voters

1cd3c77

dirvine requested changes Apr 13, 2020

View reviewed changes

madadam reviewed Apr 14, 2020

View reviewed changes

chore/key_gen: dependency update

9e2ade5

dirvine requested changes Apr 14, 2020

View reviewed changes

madadam reviewed Apr 14, 2020

View reviewed changes

src/dev_utils/peer.rs Outdated Show resolved Hide resolved

madadam reviewed Apr 14, 2020

View reviewed changes

dirvine previously approved these changes Apr 14, 2020

View reviewed changes

madadam reviewed Apr 14, 2020

View reviewed changes

madadam previously approved these changes Apr 14, 2020

View reviewed changes

fix/key_gen: address review comments

f25f200

maqi dismissed stale reviews from madadam and dirvine via f25f200 April 14, 2020 13:32

maqi force-pushed the poc branch from e22ff80 to f25f200 Compare April 14, 2020 13:32

S-Coyle approved these changes Apr 14, 2020

View reviewed changes

dirvine merged commit 8900e0e into maidsafe:master Apr 14, 2020

This was referenced Apr 14, 2020

Pending API related suggestions from the PoC PR #5

Closed

Pending test related suggestions from the PoC PR #6

Closed

Yoga07 mentioned this pull request Apr 14, 2020

Improves threshold_signature test #8

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DKG proof of concept implementation #2

DKG proof of concept implementation #2

maqi commented Mar 23, 2020 •

edited

Loading

Yoga07 left a comment

dirvine left a comment

dirvine Apr 13, 2020

madadam Apr 14, 2020

maqi Apr 14, 2020

madadam Apr 14, 2020

madadam left a comment

madadam Apr 14, 2020

madadam Apr 14, 2020

madadam Apr 14, 2020

maqi Apr 14, 2020 •

edited

Loading

S-Coyle commented Apr 14, 2020

dirvine commented Apr 14, 2020

dirvine left a comment

S-Coyle commented Apr 14, 2020

madadam Apr 14, 2020

maqi Apr 14, 2020

madadam Apr 14, 2020

maqi Apr 14, 2020

madadam left a comment

DKG proof of concept implementation #2

DKG proof of concept implementation #2

Conversation

maqi commented Mar 23, 2020 • edited Loading

Yoga07 left a comment

Choose a reason for hiding this comment

dirvine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madadam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maqi Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

S-Coyle commented Apr 14, 2020

dirvine commented Apr 14, 2020

dirvine left a comment

Choose a reason for hiding this comment

S-Coyle commented Apr 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madadam left a comment

Choose a reason for hiding this comment

maqi commented Mar 23, 2020 •

edited

Loading

maqi Apr 14, 2020 •

edited

Loading