Proper threshold support #95
Conversation
Pull Request Test Coverage Report for Build 13447280574Details
💛 - Coveralls |
| description, | ||
| evidence: EvidenceEnum::InvalidEchoPack(InvalidEchoPackEvidence { | ||
| normal_broadcast, | ||
| invalid_echo_sender: from, | ||
| invalid_echo_sender: failed_for, |
There was a problem hiding this comment.
Could this be renamed to echo_sender you think? To me "failed for" is a bit ambiguous: "failed for who? for this node?".
| .cloned() | ||
| .collect::<BTreeSet<_>>(); | ||
|
|
||
| let round_sends_echo_broadcast = !echo_broadcast.payload().is_none(); |
There was a problem hiding this comment.
Maybe add a is_some to complement is_none?
| let round_sends_echo_broadcast = !echo_broadcast.payload().is_none(); | |
| let round_sends_echo_broadcast = echo_broadcast.payload().is_some(); |
| let mut banned = self.provable_errors.keys().cloned().collect::<BTreeSet<_>>(); | ||
| banned.extend(self.unprovable_errors.keys().cloned()); | ||
| banned |
There was a problem hiding this comment.
Question for my education: Is it possible for a party to be present in both the provable and unprovable sets?
Also, this could perhaps be written as
self.provable_errors
.keys()
.chain(self.unprovable_errors.keys())
.cloned()
.collect()Not sure which is more readable tbh.
|
|
||
| pub(crate) fn is_quorum(&self, ids: &BTreeSet<Id>) -> bool { | ||
| // TODO: assuming `ids` are a subset of `self.ids`. Can we? | ||
| ids.len() >= self.threshold |
There was a problem hiding this comment.
Worth making sure I think: ids.is_subset(&self.ids) && ids.len() >= self.threshold
| let ids = self.ids.intersection(banned_ids).collect::<BTreeSet<_>>(); | ||
| self.ids.len() - ids.len() >= self.threshold |
There was a problem hiding this comment.
Would this work here?
let intersection_count = self.ids.intersection(banned_ids).count();
self.ids.len() - intersection_count >= self.threshold
| pub echo_round: EchoRoundCommunicationInfo<Id>, | ||
| } | ||
|
|
||
| /// Describes what other parties this rounds sends messages to, and what other parties it expects messages from. |
There was a problem hiding this comment.
| /// Describes what other parties this rounds sends messages to, and what other parties it expects messages from. | |
| /// Describes which other parties this rounds sends messages to, and which other parties it expects messages from. |
|
|
||
| /// The specific way the node participates in the echo round following this round. | ||
| /// | ||
| /// `None` means that, if there is an echo round, the message destinations and expected messages senders |
There was a problem hiding this comment.
Writing None here is a bit confusing given it's not an Option. Maybe reword this as "An enum describing the way the node participates in the echo round following this round. It can take one of three values, None, SameAsMainRound and Custom, where None means that, if there is an echo round, the message destinations and … …."? (Although: what does SameAsMainRound even mean then?)
| } | ||
| } | ||
|
|
||
| /// Encapsulates the communication info for the main round and the possible echo round. |
There was a problem hiding this comment.
I think it'd be better to beef up this doc comment and thin out the lower-level ones a bit, so that the doc reader can learn about the CommunicationInfo struct's purpose and usage in one spot.
Fixes #11
Done:
TODO:
IdSetthat would tell us if quorum can never be reached given the already banned nodesCommunicationInfoshould be like for thatIdSetto be a trait, allowing users to specify their own quorum conditions?protocolandsessionlevels #25?