DKG votes, sigs, knowledge and AE ground work #5

grumbach · 2022-07-15T15:13:25Z

This PR includes voting mechanisms for SDKG:

one round broadcasting Parts
one round broadcasting Acks over these Parts
one round broadcasting all the Acks over all the Parts, ensuring all the participants have the same sets (Acks and Parts)
termination once we've seen everyone's signature over those sets

Basic AE:

we deal with packet loss with RequestAntiEntropy responses
we have an AntiEntropy type containing all the votes we need to send to make AE requesting nodes catchup

Removes the deprecated Failure crate fixing #3

We still need:

tests
proptests

grumbach · 2022-07-15T15:29:57Z

src/state.rs

+
+pub enum VoteResponse {
+    WaitingForMoreVotes,
+    BroadcastVote(Box<DkgSignedVote>),


Boxed this one to appease clippy

davidrusu · 2022-07-18T14:36:00Z

src/knowledge.rs

+
+    pub fn got_all_acks(&self, participants_len: usize) -> bool {
+        self.acks.len() == participants_len
+            && self.acks.iter().any(|(_, a)| a.len() != participants_len)


Do you mean to say "all parts have acks from all participants?

Suggested change

&& self.acks.iter().any(|(_, a)| a.len() != participants_len)

&& self.acks.iter().all(|(_, a)| a.len() == participants_len)

davidrusu · 2022-07-18T14:37:56Z

src/knowledge.rs

+
+pub(crate) struct Knowledge {
+    pub parts: BTreeSet<IdPart>,
+    pub acks: BTreeMap<IdPart, BTreeSet<IdAck>>,


Suggested change

pub acks: BTreeMap<IdPart, BTreeSet<IdAck>>,

pub part_acks: BTreeMap<IdPart, BTreeSet<IdAck>>,

davidrusu · 2022-07-18T14:51:54Z

src/state.rs

+            pub_keys,
+            keygen: sync_key_gen,
+            all_votes: BTreeSet::new(),
+            our_part: opt_part.ok_or(Error::NotInPubKeySet)?,


Did you see any reason to have SyncKeyGen::new() return an optional Part? I don't really see any reason behind the Observer idea since messages are encrypted between participants, an observer would just be seeing ciphertext flying around.

SyncKeyGen::new() returns a None instead of Some(part) when you are an observer but still want a keygen to generate the PublicKeySet (but now private key shares). In our use here, all the nodes in DkgState are not observers, so we expect our_part to be Some.

Although we don't use them here, observers can also create a SyncKeyGen to check all the messages and ultimately generate the PublicKeySet from all the Acks and Parts. They just don't generate a starting Part as they are not participants.

messages are encrypted between participants

Messages are partly encrypted but enough data is leaked through so the observers can generate the PublicKeySet but not any secret key.

davidrusu · 2022-07-18T14:54:04Z

src/state.rs

+}
+
+impl<R: bls::rand::RngCore + Clone> DkgState<R> {
+    pub fn from(


Use new instead of from. We need to change this in sn_consensus as well.

Suggested change

pub fn from(

pub fn new(

from is implies the From trait: https://doc.rust-lang.org/std/convert/trait.From.html

davidrusu · 2022-07-18T15:51:28Z

src/state.rs

+    pub fn new(
+        our_id: NodeId,
+        secret_key: SecretKey,
+        pub_keys: BTreeMap<NodeId, PublicKey>,


Q: where do we get these pub keys from? 🤔

Adults are using ed25519 keys normally, and then they generate a BLS key only when being promoted to elders

Made an issue for that one #6

davidrusu · 2022-07-18T17:58:28Z

src/knowledge.rs

+                    if let Some(entry) = self.part_acks.get_mut(&id_part) {
+                        entry.insert((id, ack));
+                    } else {
+                        self.part_acks.insert(id_part, BTreeSet::from([(id, ack)]));
+                    }


Use the entry api:

Suggested change

if let Some(entry) = self.part_acks.get_mut(&id_part) {

entry.insert((id, ack));

} else {

self.part_acks.insert(id_part, BTreeSet::from([(id, ack)]));

}

let acks = self.part_acks.entry(id_part).or_default();

acks.insert((id, ack));

davidrusu · 2022-07-18T18:14:09Z

src/state.rs

+        let (sync_key_gen, opt_part) = SyncKeyGen::new(
+            our_id,
+            secret_key.clone(),
+            Arc::new(pub_keys.clone()),


We shouldn't need the Arc on these pub keys. If it's run in a multi-threaded context, then the caller can wrap the entire DkgState in an Arc themselves.

davidrusu · 2022-07-18T18:17:17Z

src/state.rs

+            Ok(k) => k,
+        };
+
+        let participants_len = self.pub_keys.len();


Call this num_participants or n_participants to show it's the number of participants, rather than length

davidrusu · 2022-07-18T18:20:16Z

src/state.rs

+                    return Err(Error::FaultyVote(format!(
+                        "Ack fault: {:?} by {:?} for part by {:?}",
+                        fault, sender_id, part_id
+                    )));


You can reduce these 4 lines down to 1 if you put the variables into the format string i.e. "... {fault:?} ..."

davidrusu · 2022-07-18T18:21:53Z

src/state.rs

+                    return Err(Error::FaultyVote(format!(
+                        "Part fault: {:?} by {:?}",
+                        fault, sender_id
+                    )));


same here, could reduce to 1 line

davidrusu

Looks good, we can address some of the bigger points in future PR's.

I like the DkgCurrentState enum, it may help to have something similar in sn_consensus 🤔

grumbach added 3 commits July 15, 2022 17:04

feat: dkg votes, sigs, knowledge and AE ground work

2850e32

feat: make DkgSignedVote public

6e23df2

feat: remove unecessary outcome field

c2a04b3

grumbach commented Jul 15, 2022

View reviewed changes

chore: fix typos

c72baf1

grumbach mentioned this pull request Jul 15, 2022

chore: use thiserror instead of failure #4

Closed

davidrusu reviewed Jul 18, 2022

View reviewed changes

chore: improve naming and code style

465bfe1

davidrusu reviewed Jul 18, 2022

View reviewed changes

davidrusu approved these changes Jul 18, 2022

View reviewed changes

grumbach mentioned this pull request Jul 19, 2022

Use Ed25519 keys instead of bls #6

Closed

chore: improve readability

8420b5f

grumbach merged commit b77034d into maidsafe:main Jul 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DKG votes, sigs, knowledge and AE ground work #5

DKG votes, sigs, knowledge and AE ground work #5

grumbach commented Jul 15, 2022 •

edited

Loading

grumbach Jul 15, 2022

davidrusu Jul 18, 2022

davidrusu Jul 18, 2022

davidrusu Jul 18, 2022

grumbach Jul 18, 2022

grumbach Jul 18, 2022

grumbach Jul 18, 2022 •

edited

Loading

davidrusu Jul 18, 2022 •

edited

Loading

davidrusu Jul 18, 2022

grumbach Jul 19, 2022 •

edited

Loading

davidrusu Jul 18, 2022

davidrusu Jul 18, 2022

davidrusu Jul 18, 2022

davidrusu Jul 18, 2022

davidrusu Jul 18, 2022

davidrusu left a comment

	&& self.acks.iter().any(\|(_, a)\| a.len() != participants_len)
	&& self.acks.iter().all(\|(_, a)\| a.len() == participants_len)

	pub acks: BTreeMap<IdPart, BTreeSet<IdAck>>,
	pub part_acks: BTreeMap<IdPart, BTreeSet<IdAck>>,

DKG votes, sigs, knowledge and AE ground work #5

DKG votes, sigs, knowledge and AE ground work #5

Conversation

grumbach commented Jul 15, 2022 • edited Loading

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

Choose a reason for hiding this comment

grumbach Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

davidrusu Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grumbach Jul 19, 2022 • edited Loading

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

Choose a reason for hiding this comment

davidrusu left a comment

Choose a reason for hiding this comment

grumbach commented Jul 15, 2022 •

edited

Loading

grumbach Jul 18, 2022 •

edited

Loading

davidrusu Jul 18, 2022 •

edited

Loading

grumbach Jul 19, 2022 •

edited

Loading