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

@@ -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:

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.

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"]

consensus/src/dag/reconstruction/dag.rs

@@ -96,7 +96,6 @@ impl<U: UnitWithParents> Dag<U> {
         }
         let missing_parents = unit
             .parents()
-            .values()
             .filter(|parent| !self.dag_units.contains(parent))
             .cloned()
             .collect();

consensus/src/dag/reconstruction/mod.rs

@@ -1,22 +1,22 @@
-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;
 
 /// A unit with its parents represented explicitly.
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub struct ReconstructedUnit<U: Unit> {
     unit: U,
-    parents: NodeMap<HashFor<U>>,
+    parents: NodeMap<(HashFor<U>, Round)>,
 }
 
 impl<U: Unit> ReconstructedUnit<U> {
@@ -25,7 +25,18 @@ impl<U: Unit> ReconstructedUnit<U> {
         match unit.control_hash().combined_hash
             == ControlHash::<U::Hasher>::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<U: Unit> WrappedUnit<U::Hasher> for ReconstructedUnit<U> {
 }
 
 impl<U: Unit> UnitWithParents for ReconstructedUnit<U> {
-    fn parents(&self) -> &NodeMap<HashFor<Self>> {
-        &self.parents
+    fn parents(&self) -> impl Iterator<Item = &HashFor<U>> {
+        self.parents.values().map(|(hash, _)| hash)
+    }
+
+    fn direct_parents(&self) -> impl Iterator<Item = &HashFor<Self>> {
+        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>> {
+        self.parents.get(index).map(|(hash, _)| hash)
+    }
+
+    fn node_count(&self) -> NodeCount {
+        self.parents.size()
     }
 }
 
@@ -89,7 +125,7 @@ impl<D: Data, H: Hasher, K: MultiKeychain> From<ReconstructedUnit<Signed<FullUni
     for OrderedUnit<D, H>
 {
     fn from(unit: ReconstructedUnit<Signed<FullUnit<H, D>, 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);
                         }

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())
             );
         }
     }

consensus/src/dissemination/responder.rs

@@ -57,7 +57,6 @@ impl<H: Hasher, D: Data, MK: MultiKeychain> Responder<H, D, MK> {
             .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);
                 }
             }

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<U: UnitWithParents> CandidateElection<U> {
 
     fn parent_votes(
         &mut self,
-        parents: &NodeMap<HashFor<U>>,
+        parents: Vec<HashFor<U>>,
     ) -> Result<(NodeCount, NodeCount), CandidateOutcome<U::Hasher>> {
         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<U: UnitWithParents> CandidateElection<U> {
 
     fn vote_from_parents(
         &mut self,
-        parents: &NodeMap<HashFor<U>>,
+        parents: Vec<HashFor<U>>,
+        threshold: NodeCount,
         relative_round: Round,
     ) -> Result<bool, CandidateOutcome<U::Hasher>> {
         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<U: UnitWithParents> CandidateElection<U> {
         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();

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);

consensus/src/extension/units.rs

@@ -64,8 +64,8 @@ impl<U: UnitWithParents> Units<U> {
                 .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);
                 }
             }

consensus/src/testing/dag.rs

@@ -139,18 +139,16 @@ 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)
             .expect("we have the unit")
             .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);
     }

consensus/src/units/mod.rs

@@ -215,7 +215,10 @@ pub trait WrappedUnit<H: Hasher>: Unit<Hasher = H> {
 }
 
 pub trait UnitWithParents: Unit {
-    fn parents(&self) -> &NodeMap<HashFor<Self>>;
+    fn parents(&self) -> impl Iterator<Item = &HashFor<Self>>;
+    fn direct_parents(&self) -> impl Iterator<Item = &HashFor<Self>>;
+    fn parent_for(&self, index: NodeIndex) -> Option<&HashFor<Self>>;
+    fn node_count(&self) -> NodeCount;
 }
 
 impl<H: Hasher, D: Data> Unit for FullUnit<H, D> {