From 71f03144faa5856c85f477142c013fe36659b9e3 Mon Sep 17 00:00:00 2001 From: Marcin Date: Mon, 23 Dec 2024 14:43:42 +0100 Subject: [PATCH] A0-4563: Introduce notion of non-direct parents in Extender (#506) * First working version * fmt * clippy * Bump consensus crate version * Updated readme * Revered changes to unit testing code * Review * fmt * Review * Try to fix cargo audit pipeline --- .github/workflows/cargo-audit.yml | 5 ++ Cargo.lock | 2 +- README.md | 2 +- consensus/Cargo.toml | 2 +- consensus/src/dag/reconstruction/dag.rs | 1 - consensus/src/dag/reconstruction/mod.rs | 60 ++++++++++++++++----- consensus/src/dag/reconstruction/parents.rs | 12 ++--- consensus/src/dissemination/responder.rs | 5 +- consensus/src/extension/election.rs | 22 ++++---- consensus/src/extension/extender.rs | 2 +- consensus/src/extension/units.rs | 4 +- consensus/src/testing/dag.rs | 8 ++- consensus/src/units/mod.rs | 5 +- 13 files changed, 87 insertions(+), 43 deletions(-) diff --git a/.github/workflows/cargo-audit.yml b/.github/workflows/cargo-audit.yml index c6c9a2a..ee92cc6 100644 --- a/.github/workflows/cargo-audit.yml +++ b/.github/workflows/cargo-audit.yml @@ -20,6 +20,11 @@ jobs: - name: Checkout source code uses: actions/checkout@v4 + - name: Install cargo audit + shell: bash + run: | + cargo install cargo-audit --locked + - name: Run `cargo-audit` uses: actions-rs/audit-check@v1 with: diff --git a/Cargo.lock b/Cargo.lock index 3bb9c58..f941797 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -28,7 +28,7 @@ dependencies = [ [[package]] name = "aleph-bft" -version = "0.39.1" +version = "0.40.0" dependencies = [ "aleph-bft-mock", "aleph-bft-rmc", diff --git a/README.md b/README.md index 0a78b29..0a70d54 100644 --- a/README.md +++ b/README.md @@ -60,7 +60,7 @@ More details are available [in the book][reference-link-implementation-details]. - Import AlephBFT in your crate ```toml [dependencies] - aleph-bft = "^0.39" + aleph-bft = "^0.40" ``` - The main entry point is the `run_session` function, which returns a Future that runs the consensus algorithm. diff --git a/consensus/Cargo.toml b/consensus/Cargo.toml index 300a89a..609ae3c 100644 --- a/consensus/Cargo.toml +++ b/consensus/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "aleph-bft" -version = "0.39.1" +version = "0.40.0" edition = "2021" authors = ["Cardinal Cryptography"] categories = ["algorithms", "data-structures", "cryptography", "database"] diff --git a/consensus/src/dag/reconstruction/dag.rs b/consensus/src/dag/reconstruction/dag.rs index e488b49..f3b07d8 100644 --- a/consensus/src/dag/reconstruction/dag.rs +++ b/consensus/src/dag/reconstruction/dag.rs @@ -96,7 +96,6 @@ impl Dag { } let missing_parents = unit .parents() - .values() .filter(|parent| !self.dag_units.contains(parent)) .cloned() .collect(); diff --git a/consensus/src/dag/reconstruction/mod.rs b/consensus/src/dag/reconstruction/mod.rs index ec7b8c1..120853d 100644 --- a/consensus/src/dag/reconstruction/mod.rs +++ b/consensus/src/dag/reconstruction/mod.rs @@ -1,14 +1,14 @@ -use std::collections::HashMap; - use crate::{ units::{ControlHash, FullUnit, HashFor, Unit, UnitCoord, UnitWithParents, WrappedUnit}, Hasher, NodeMap, SessionId, }; +use aleph_bft_rmc::NodeCount; +use std::collections::HashMap; mod dag; mod parents; -use aleph_bft_types::{Data, MultiKeychain, OrderedUnit, Signed}; +use aleph_bft_types::{Data, MultiKeychain, NodeIndex, OrderedUnit, Round, Signed}; use dag::Dag; use parents::Reconstruction as ParentReconstruction; @@ -16,7 +16,7 @@ use parents::Reconstruction as ParentReconstruction; #[derive(Debug, PartialEq, Eq, Clone)] pub struct ReconstructedUnit { unit: U, - parents: NodeMap>, + parents: NodeMap<(HashFor, Round)>, } impl ReconstructedUnit { @@ -25,7 +25,18 @@ impl ReconstructedUnit { match unit.control_hash().combined_hash == ControlHash::::combine_hashes(&parents) { - true => Ok(ReconstructedUnit { unit, parents }), + true => { + let unit_round = unit.round(); + let mut parents_with_rounds = NodeMap::with_size(parents.size()); + for (parent_index, hash) in parents.into_iter() { + // we cannot have here round 0 units + parents_with_rounds.insert(parent_index, (hash, unit_round.saturating_sub(1))); + } + Ok(ReconstructedUnit { + unit, + parents: parents_with_rounds, + }) + } false => Err(unit), } } @@ -72,8 +83,33 @@ impl WrappedUnit for ReconstructedUnit { } impl UnitWithParents for ReconstructedUnit { - fn parents(&self) -> &NodeMap> { - &self.parents + fn parents(&self) -> impl Iterator> { + self.parents.values().map(|(hash, _)| hash) + } + + fn direct_parents(&self) -> impl Iterator> { + self.parents + .values() + .filter_map(|(hash, parent_round)| match self.unit.coord().round() { + // round 0 units cannot have non-empty parents + 0 => None, + + unit_round => { + if unit_round - 1 == *parent_round { + Some(hash) + } else { + None + } + } + }) + } + + fn parent_for(&self, index: NodeIndex) -> Option<&HashFor> { + self.parents.get(index).map(|(hash, _)| hash) + } + + fn node_count(&self) -> NodeCount { + self.parents.size() } } @@ -89,7 +125,7 @@ impl From { fn from(unit: ReconstructedUnit, K>>) -> Self { - let parents = unit.parents().values().cloned().collect(); + let parents = unit.parents().cloned().collect(); let unit = unit.unpack(); let creator = unit.creator(); let round = unit.round(); @@ -233,7 +269,7 @@ mod test { assert_eq!(units.len(), 1); let reconstructed_unit = units.pop().expect("just checked its there"); assert_eq!(reconstructed_unit, ReconstructedUnit::initial(unit.clone())); - assert_eq!(reconstructed_unit.parents().item_count(), 0); + assert_eq!(reconstructed_unit.parents().count(), 0); } } @@ -254,15 +290,15 @@ mod test { match round { 0 => { assert_eq!(reconstructed_unit, ReconstructedUnit::initial(unit.clone())); - assert_eq!(reconstructed_unit.parents().item_count(), 0); + assert_eq!(reconstructed_unit.parents().count(), 0); } round => { - assert_eq!(reconstructed_unit.parents().item_count(), 4); + assert_eq!(reconstructed_unit.parents().count(), 4); let parents = dag .get((round - 1) as usize) .expect("the parents are there"); for (parent, reconstructed_parent) in - parents.iter().zip(reconstructed_unit.parents().values()) + parents.iter().zip(reconstructed_unit.parents()) { assert_eq!(&parent.hash(), reconstructed_parent); } diff --git a/consensus/src/dag/reconstruction/parents.rs b/consensus/src/dag/reconstruction/parents.rs index ac1ea95..820c5bf 100644 --- a/consensus/src/dag/reconstruction/parents.rs +++ b/consensus/src/dag/reconstruction/parents.rs @@ -237,7 +237,7 @@ mod test { assert_eq!(units.len(), 1); let reconstructed_unit = units.pop().expect("just checked its there"); assert_eq!(reconstructed_unit, ReconstructedUnit::initial(unit.clone())); - assert_eq!(reconstructed_unit.parents().item_count(), 0); + assert_eq!(reconstructed_unit.parents().count(), 0); } } @@ -258,15 +258,15 @@ mod test { match round { 0 => { assert_eq!(reconstructed_unit, ReconstructedUnit::initial(unit.clone())); - assert_eq!(reconstructed_unit.parents().item_count(), 0); + assert_eq!(reconstructed_unit.parents().count(), 0); } round => { - assert_eq!(reconstructed_unit.parents().item_count(), 4); + assert_eq!(reconstructed_unit.parents().count(), 4); let parents = dag .get((round - 1) as usize) .expect("the parents are there"); for (parent, reconstructed_parent) in - parents.iter().zip(reconstructed_unit.parents().values()) + parents.iter().zip(reconstructed_unit.parents()) { assert_eq!(&parent.hash(), reconstructed_parent); } @@ -371,11 +371,11 @@ mod test { assert!(requests.is_empty()); assert_eq!(units.len(), 1); let reconstructed_unit = units.pop().expect("just checked its there"); - assert_eq!(reconstructed_unit.parents().item_count(), 4); + assert_eq!(reconstructed_unit.parents().count(), 4); for (coord, parent_hash) in parent_hashes { assert_eq!( Some(&parent_hash), - reconstructed_unit.parents().get(coord.creator()) + reconstructed_unit.parent_for(coord.creator()) ); } } diff --git a/consensus/src/dissemination/responder.rs b/consensus/src/dissemination/responder.rs index ae01602..95ab5de 100644 --- a/consensus/src/dissemination/responder.rs +++ b/consensus/src/dissemination/responder.rs @@ -57,7 +57,6 @@ impl Responder { .map(|unit| { let parents = unit .parents() - .values() .map(|parent_hash| { units .unit(parent_hash) @@ -270,8 +269,8 @@ mod test { match response { Response::Parents(response_hash, parents) => { assert_eq!(response_hash, requested_unit.hash()); - assert_eq!(parents.len(), requested_unit.parents().size().0); - for (parent, parent_hash) in zip(parents, requested_unit.parents().values()) { + assert_eq!(parents.len(), requested_unit.parents().count()); + for (parent, parent_hash) in zip(parents, requested_unit.parents()) { assert_eq!(&parent.as_signable().hash(), parent_hash); } } diff --git a/consensus/src/extension/election.rs b/consensus/src/extension/election.rs index c2afc17..8ef87bc 100644 --- a/consensus/src/extension/election.rs +++ b/consensus/src/extension/election.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use crate::{ extension::units::Units, units::{HashFor, UnitWithParents}, - Hasher, NodeCount, NodeIndex, NodeMap, Round, + Hasher, NodeCount, NodeIndex, Round, }; fn common_vote(relative_round: Round) -> bool { @@ -49,11 +49,11 @@ impl CandidateElection { fn parent_votes( &mut self, - parents: &NodeMap>, + parents: Vec>, ) -> Result<(NodeCount, NodeCount), CandidateOutcome> { let (mut votes_for, mut votes_against) = (NodeCount(0), NodeCount(0)); - for parent in parents.values() { - match self.votes.get(parent).expect("units are added in order") { + for parent in parents { + match self.votes.get(&parent).expect("units are added in order") { true => votes_for += NodeCount(1), false => votes_against += NodeCount(1), } @@ -63,11 +63,11 @@ impl CandidateElection { fn vote_from_parents( &mut self, - parents: &NodeMap>, + parents: Vec>, + threshold: NodeCount, relative_round: Round, ) -> Result> { use CandidateOutcome::*; - let threshold = parents.size().consensus_threshold(); // Gather parents' votes. let (votes_for, votes_against) = self.parent_votes(parents)?; assert!(votes_for + votes_against >= threshold); @@ -105,9 +105,13 @@ impl CandidateElection { let vote = match relative_round { 0 => unreachable!("just checked that voter and election rounds are not equal"), // Direct descendands vote for, all other units of that round against. - 1 => voter.parents().get(self.candidate_creator) == Some(&self.candidate_hash), + 1 => voter.parent_for(self.candidate_creator) == Some(&self.candidate_hash), // Otherwise we compute the vote based on the parents' votes. - _ => self.vote_from_parents(voter.parents(), relative_round)?, + _ => { + let threshold = voter.node_count().consensus_threshold(); + let direct_parents = voter.direct_parents().cloned().collect(); + self.vote_from_parents(direct_parents, threshold, relative_round)? + } }; self.votes.insert(voter.hash(), vote); Ok(()) @@ -360,7 +364,7 @@ mod test { #[test] #[ignore] - // TODO(A0-4563) Uncomment after changes to parent voting code + // TODO(A0-4559) Uncomment fn given_minimal_dag_with_orphaned_node_when_electing_then_orphaned_node_is_not_head() { use ElectionResult::*; let mut units = Units::new(); diff --git a/consensus/src/extension/extender.rs b/consensus/src/extension/extender.rs index bf55602..7a8f5fb 100644 --- a/consensus/src/extension/extender.rs +++ b/consensus/src/extension/extender.rs @@ -101,7 +101,7 @@ mod test { #[test] #[ignore] - // TODO(A0-4563) Uncomment after changes to parent voting code + // TODO(A0-4559) Uncomment fn given_minimal_dag_with_orphaned_node_when_producing_batches_have_correct_length() { let mut extender = Extender::new(); let n_members = NodeCount(4); diff --git a/consensus/src/extension/units.rs b/consensus/src/extension/units.rs index ca2b42f..e9deacf 100644 --- a/consensus/src/extension/units.rs +++ b/consensus/src/extension/units.rs @@ -64,8 +64,8 @@ impl Units { .expect("head is picked among units we have"), ); while let Some(u) = queue.pop_front() { - for u_hash in u.parents().clone().into_values() { - if let Some(v) = self.units.remove(&u_hash) { + for u_hash in u.parents() { + if let Some(v) = self.units.remove(u_hash) { queue.push_back(v); } } diff --git a/consensus/src/testing/dag.rs b/consensus/src/testing/dag.rs index 30023b2..86f7b22 100644 --- a/consensus/src/testing/dag.rs +++ b/consensus/src/testing/dag.rs @@ -139,7 +139,7 @@ impl DagFeeder { fn on_reconstructed_unit(&mut self, unit: ReconstructedUnit) { let h = unit.hash(); - let parents = unit.parents(); + let parents: HashSet<_> = unit.parents().cloned().collect(); let expected_hashes: HashSet<_> = self .units_map .get(&h) @@ -147,10 +147,8 @@ impl DagFeeder { .parent_hashes() .into_iter() .collect(); - assert_eq!(parents.item_count(), expected_hashes.len()); - for (_, hash) in parents { - assert!(expected_hashes.contains(hash)); - } + + assert_eq!(parents, expected_hashes); self.result.push(unit.clone()); self.store.insert(unit); } diff --git a/consensus/src/units/mod.rs b/consensus/src/units/mod.rs index 285bfa1..ea251aa 100644 --- a/consensus/src/units/mod.rs +++ b/consensus/src/units/mod.rs @@ -215,7 +215,10 @@ pub trait WrappedUnit: Unit { } pub trait UnitWithParents: Unit { - fn parents(&self) -> &NodeMap>; + fn parents(&self) -> impl Iterator>; + fn direct_parents(&self) -> impl Iterator>; + fn parent_for(&self, index: NodeIndex) -> Option<&HashFor>; + fn node_count(&self) -> NodeCount; } impl Unit for FullUnit {