[consensus] Add consensus::aggregation
#974
Conversation
bcc3600 to
4321ffb
Compare
3e34e05 to
0bbdbea
Compare
| use commonware_utils::{max_faults, Array}; | ||
| use std::collections::{btree_map, BTreeMap, HashMap, HashSet}; | ||
|
|
||
| /// A data structure that keeps track of the reported tip for each validator. |
There was a problem hiding this comment.
This is an awesome data structure.
There was a problem hiding this comment.
Wonder if it makes sense to give it a catchier name given so (and publicly export it maybe in utils)?
There was a problem hiding this comment.
Agree it could be useful in utils but would rather not add stuff to the public API there until it gets more use and we can settle more on solidified behavior/functions (if it's used in at least one more place)
consensus/src/aggregation/safe_tip.rs
Show resolved
Hide resolved
83bc6d4 to
01039b6
Compare
|
I think this one will realllllly benefit from #1002 ❤️ (sorry I thought this would go out as a review comment lol) |
consensus/src/aggregation/mod.rs
Outdated
| @@ -0,0 +1,1093 @@ | |||
| //! Threshold signature aggregation for agreeing on externally-finalized hashes. | |||
There was a problem hiding this comment.
nit: Recover threshold signatures over an externally synchronized stream of digests.
I suppose the "aggregation" here is really what we'd call recovery (and it would be aggregation in a multi-signature case). 🤔
There was a problem hiding this comment.
...now second guessing the name aggregation lol
consensus/src/aggregation/wire.rs
Outdated
There was a problem hiding this comment.
In other consensus crates, we combine types and wire. Curious if you feel it is better to separate (I've taken the position that anything in types may be serialized somewhere and sent over the wire so delineation doesn't add anything).
There was a problem hiding this comment.
I think it's better to keep them separate. Just easier to see the direct types that you are expected to send over the wire or not
| digest: D, | ||
| sender: &mut WrappedSender<NetS, PeerAck<V, D>>, | ||
| ) -> Result<(), Error> { | ||
| // Entry must be `Pending::Unverified`, or return early |
There was a problem hiding this comment.
Should we not just store digest in Verified immediately (and skip Unverified in this case)?
There was a problem hiding this comment.
Didn't quite understand this comment. I think it's good to check that the existing value is Unverified, otherwise we can completely skip any other work here since it has already been verified before (and is still in verified state, or was completed)
| //! | ||
| //! # Design Decisions | ||
| //! | ||
| //! ## Missing Signature Resolution |
There was a problem hiding this comment.
Could add a recommendation to use resolver for this backfilling (if you are missing something).
There was a problem hiding this comment.
Makes sense but commonware_resolver is not currently imported by the commonware_consensus crate and so there is not a great way to link it in rustdoc. Will leave it out for now
77f9b4c to
29ba1c5
Compare
ecfa97e to
a14e04d
Compare
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #974 +/- ##
==========================================
- Coverage 91.39% 90.57% -0.83%
==========================================
Files 218 228 +10
Lines 59455 54654 -4801
==========================================
- Hits 54338 49502 -4836
- Misses 5117 5152 +35
... and 167 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Fixes #814
TODO:
Activityemission for conflicts #1084ordered-broadcastandaggregation#1002