diff --git a/Cargo.lock b/Cargo.lock index f9417979..9ced235d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -28,7 +28,7 @@ dependencies = [ [[package]] name = "aleph-bft" -version = "0.40.0" +version = "0.41.0" dependencies = [ "aleph-bft-mock", "aleph-bft-rmc", diff --git a/README.md b/README.md index 0a70d547..0f978745 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.40" + aleph-bft = "^0.41" ``` - 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 609ae3c1..0ccbbaa7 100644 --- a/consensus/Cargo.toml +++ b/consensus/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "aleph-bft" -version = "0.40.0" +version = "0.41.0" edition = "2021" authors = ["Cardinal Cryptography"] categories = ["algorithms", "data-structures", "cryptography", "database"] diff --git a/consensus/src/alerts/handler.rs b/consensus/src/alerts/handler.rs index 214f462e..3cb0b61c 100644 --- a/consensus/src/alerts/handler.rs +++ b/consensus/src/alerts/handler.rs @@ -277,12 +277,13 @@ impl Handler { #[cfg(test)] mod tests { + use crate::units::ControlHash; use crate::{ alerts::{ handler::{Error, Handler, RmcResponse}, Alert, AlertMessage, ForkProof, ForkingNotification, }, - units::{ControlHash, FullUnit, PreUnit}, + units::{FullUnit, PreUnit}, PartiallyMultisigned, Recipient, Round, }; use aleph_bft_mock::{Data, Hasher64, Keychain, Signature}; diff --git a/consensus/src/backup/loader.rs b/consensus/src/backup/loader.rs index 97fefb7b..a2cf339f 100644 --- a/consensus/src/backup/loader.rs +++ b/consensus/src/backup/loader.rs @@ -111,11 +111,8 @@ impl BackupLoader { )); } - let parent_ids = &full_unit.as_pre_unit().control_hash().parents_mask; - // Sanity check: verify that all unit's parents appeared in backup before it. - for parent_id in parent_ids.elements() { - let parent = UnitCoord::new(coord.round() - 1, parent_id); + for parent in full_unit.as_pre_unit().control_hash().parents() { if !already_loaded_coords.contains(&parent) { return Err(LoaderError::InconsistentData(coord)); } diff --git a/consensus/src/creation/creator.rs b/consensus/src/creation/creator.rs index 594f028d..a8653cc8 100644 --- a/consensus/src/creation/creator.rs +++ b/consensus/src/creation/creator.rs @@ -86,13 +86,22 @@ impl Creator { pub fn create_unit(&self, round: Round) -> Result> { let parents = match round.checked_sub(1) { None => NodeMap::with_size(self.n_members), - Some(prev_round) => self - .round_collectors - .get(usize::from(prev_round)) - .ok_or(ConstraintError::NotEnoughParents)? - .prospective_parents(self.node_id)? - .clone(), + Some(prev_round) => { + let parents = self + .round_collectors + .get(usize::from(prev_round)) + .ok_or(ConstraintError::NotEnoughParents)? + .prospective_parents(self.node_id)? + .clone(); + let mut parents_with_rounds_and_hashes = NodeMap::with_size(parents.size()); + for (parent_index, hash) in parents.into_iter() { + // we cannot have here round 0 units + parents_with_rounds_and_hashes.insert(parent_index, (hash, prev_round)); + } + parents_with_rounds_and_hashes + } }; + Ok(PreUnit::new( self.node_id, round, diff --git a/consensus/src/dag/reconstruction/dag.rs b/consensus/src/dag/reconstruction/dag.rs index f3b07d8a..6c16aaa3 100644 --- a/consensus/src/dag/reconstruction/dag.rs +++ b/consensus/src/dag/reconstruction/dag.rs @@ -121,14 +121,16 @@ mod test { Hasher, NodeCount, NodeIndex, NodeMap, }; use aleph_bft_mock::Hasher64; + use aleph_bft_types::Round; use std::collections::HashSet; fn full_parents_to_map( parents: Vec<::Hash>, - ) -> NodeMap<::Hash> { + parent_round: Round, + ) -> NodeMap<(::Hash, Round)> { let mut result = NodeMap::with_size(NodeCount(parents.len())); for (id, parent) in parents.into_iter().enumerate() { - result.insert(NodeIndex(id), parent); + result.insert(NodeIndex(id), (parent, parent_round)); } result } @@ -150,8 +152,8 @@ mod test { .map(|unit| ReconstructedUnit::initial(unit.clone())) .collect(); let mut result = vec![initial_units]; - for (units, parents) in dag.iter().skip(1).zip(hashes) { - let parents = full_parents_to_map(parents); + for ((parent_round, units), parents) in dag.iter().skip(1).enumerate().zip(hashes) { + let parents = full_parents_to_map(parents, parent_round as Round); let reconstructed = units .iter() .map(|unit| { diff --git a/consensus/src/dag/reconstruction/mod.rs b/consensus/src/dag/reconstruction/mod.rs index 120853d0..20d2435d 100644 --- a/consensus/src/dag/reconstruction/mod.rs +++ b/consensus/src/dag/reconstruction/mod.rs @@ -21,22 +21,11 @@ pub struct ReconstructedUnit { impl ReconstructedUnit { /// Returns a reconstructed unit if the parents agree with the hash, errors out otherwise. - pub fn with_parents(unit: U, parents: NodeMap>) -> Result { - match unit.control_hash().combined_hash - == ControlHash::::combine_hashes(&parents) + pub fn with_parents(unit: U, parents: NodeMap<(HashFor, Round)>) -> Result { + match unit.control_hash().combined_hash() + == ControlHash::::create_control_hash(&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, - }) - } + true => Ok(ReconstructedUnit { unit, parents }), false => Err(unit), } } @@ -249,13 +238,14 @@ impl Reconstruction { #[cfg(test)] mod test { - use std::collections::HashMap; - use crate::{ dag::reconstruction::{ReconstructedUnit, Reconstruction, ReconstructionResult, Request}, units::{random_full_parent_units_up_to, Unit, UnitCoord, UnitWithParents}, NodeCount, NodeIndex, }; + use aleph_bft_types::{NodeMap, Round}; + use rand::Rng; + use std::collections::HashMap; #[test] fn reconstructs_initial_units() { @@ -415,4 +405,87 @@ mod test { unit_hash ) } + #[test] + fn given_wrong_rounds_with_matching_hashes_when_calling_with_parents_then_err_is_returned() { + const MAX_ROUND: Round = 7; + + let mut rng = rand::thread_rng(); + let node_count = NodeCount(7); + let mut reconstruction = Reconstruction::new(); + + let dag = random_full_parent_units_up_to(MAX_ROUND, node_count, 43); + for units in &dag { + for unit in units { + let round = unit.round(); + let ReconstructionResult { units, requests } = + reconstruction.add_unit(unit.clone()); + assert!(requests.is_empty()); + assert_eq!(units.len(), 1); + match round { + 0 => { + let mut parents_map: NodeMap<(_, _)> = NodeMap::with_size(node_count); + assert!( + ReconstructedUnit::with_parents(unit.clone(), parents_map.clone()) + .is_ok(), + "Initial units should not have parents!" + ); + + let random_parent_index = rng.gen::() % node_count.0 as u64; + parents_map.insert( + NodeIndex(random_parent_index as usize), + (unit.hash(), 2 as Round), + ); + assert_eq!( + ReconstructedUnit::with_parents(unit.clone(), parents_map), + Err(unit.clone()), + "Initial unit reconstructed with a non-empty parent!" + ); + } + round => { + let mut parents_map: NodeMap<(_, _)> = NodeMap::with_size(node_count); + assert_eq!( + ReconstructedUnit::with_parents(unit.clone(), parents_map.clone()), + Err(unit.clone()), + "Non-initial rounds should have parents!" + ); + + let random_parent_index = rng.gen::() % node_count.0 as u64; + parents_map.insert( + NodeIndex(random_parent_index as usize), + (unit.hash(), round as Round), + ); + assert_eq!( + ReconstructedUnit::with_parents(unit.clone(), parents_map.clone()), + Err(unit.clone()), + "Unit reconstructed with missing parents and wrong parent rounds!" + ); + + let this_unit_control_hash = unit.control_hash(); + let mut parents: NodeMap<(_, _)> = + NodeMap::with_size(this_unit_control_hash.n_members()); + for (node_index, &(hash, round)) in units[0].parents.iter() { + parents.insert(node_index, (hash, round)); + } + assert!( + ReconstructedUnit::with_parents(unit.clone(), parents.clone()).is_ok(), + "Reconstructed unit control hash does not match unit's control hash!" + ); + let random_parent_index = rng.gen::() % node_count.0 as u64; + let random_parent_index = NodeIndex(random_parent_index as usize); + let &(parent_hash, _) = parents.get(random_parent_index).unwrap(); + let wrong_round = match round { + 1 => MAX_ROUND, + _ => 0, + }; + parents_map.insert(random_parent_index, (parent_hash, wrong_round)); + assert_eq!( + ReconstructedUnit::with_parents(unit.clone(), parents_map.clone()), + Err(unit.clone()), + "Unit reconstructed with one parent having wrong round!" + ); + } + } + } + } + } } diff --git a/consensus/src/dag/reconstruction/parents.rs b/consensus/src/dag/reconstruction/parents.rs index 820c5bf8..2bbff7ef 100644 --- a/consensus/src/dag/reconstruction/parents.rs +++ b/consensus/src/dag/reconstruction/parents.rs @@ -3,13 +3,14 @@ use crate::{ units::{ControlHash, HashFor, Unit, UnitCoord}, NodeIndex, NodeMap, }; +use aleph_bft_types::Round; use std::collections::{hash_map::Entry, HashMap}; /// A unit in the process of reconstructing its parents. #[derive(Debug, PartialEq, Eq, Clone)] enum ReconstructingUnit { /// We are trying to optimistically reconstruct the unit from potential parents we get. - Reconstructing(U, NodeMap>), + Reconstructing(U, NodeMap<(HashFor, Round)>), /// We are waiting for receiving an explicit list of unit parents. WaitingForParents(U), } @@ -29,11 +30,7 @@ impl ReconstructingUnit { round != 0, "We should never try to reconstruct parents of a unit of round 0." ); - let coords = unit - .control_hash() - .parents() - .map(|parent_id| UnitCoord::new(round - 1, parent_id)) - .collect(); + let coords = unit.control_hash().parents().collect(); ( ReconstructingUnit::Reconstructing(unit, NodeMap::with_size(n_members)), coords, @@ -44,14 +41,15 @@ impl ReconstructingUnit { self, parent_id: NodeIndex, parent_hash: HashFor, + parent_round: Round, ) -> SingleParentReconstructionResult { use ReconstructingUnit::*; use SingleParentReconstructionResult::*; match self { Reconstructing(unit, mut parents) => { - parents.insert(parent_id, parent_hash); + parents.insert(parent_id, (parent_hash, parent_round)); match parents.item_count() == unit.control_hash().parents().count() { - // We have enought parents, just need to check the control hash matches. + // We have enough parents, just need to check the control hash matches. true => match ReconstructedUnit::with_parents(unit, parents) { Ok(unit) => Reconstructed(unit), // If the control hash doesn't match we want to get an explicit list of parents. @@ -85,9 +83,11 @@ impl ReconstructingUnit { return Err(self); } let mut parents_map = NodeMap::with_size(control_hash.n_members()); - for parent_id in control_hash.parents() { - match parents.get(&UnitCoord::new(self.as_unit().round() - 1, parent_id)) { - Some(parent_hash) => parents_map.insert(parent_id, *parent_hash), + for parent_coord in control_hash.parents() { + match parents.get(&parent_coord) { + Some(parent_hash) => { + parents_map.insert(parent_coord.creator(), (*parent_hash, parent_coord.round())) + } // The parents were inconsistent with the control hash. None => return Err(self), } @@ -118,10 +118,11 @@ impl Reconstruction { child_hash: HashFor, parent_id: NodeIndex, parent_hash: HashFor, + parent_round: Round, ) -> ReconstructionResult { use SingleParentReconstructionResult::*; match self.reconstructing_units.remove(&child_hash) { - Some(child) => match child.reconstruct_parent(parent_id, parent_hash) { + Some(child) => match child.reconstruct_parent(parent_id, parent_hash, parent_round) { Reconstructed(unit) => ReconstructionResult::reconstructed(unit), InProgress(unit) => { self.reconstructing_units.insert(child_hash, unit); @@ -161,6 +162,7 @@ impl Reconstruction { child_hash, unit_coord.creator(), unit_hash, + unit_coord.round(), )); } } @@ -178,6 +180,7 @@ impl Reconstruction { unit_hash, parent_coord.creator(), *parent_hash, + parent_coord.round(), )), None => { self.waiting_for_coord diff --git a/consensus/src/network/mod.rs b/consensus/src/network/mod.rs index 1093e8f4..65564456 100644 --- a/consensus/src/network/mod.rs +++ b/consensus/src/network/mod.rs @@ -46,9 +46,10 @@ mod tests { member::UnitMessage, network::NetworkDataInner::{Alert, Units}, units::{ControlHash, FullUnit, PreUnit, UncheckedSignedUnit, UnitCoord}, - Hasher, NodeIndex, NodeSubset, Round, Signed, + Hasher, NodeIndex, Round, Signed, }; use aleph_bft_mock::{Data, Hasher64, Keychain, PartialMultisignature, Signature}; + use aleph_bft_types::NodeMap; use codec::{Decode, Encode}; fn test_unchecked_unit( @@ -56,10 +57,7 @@ mod tests { round: Round, data: Data, ) -> UncheckedSignedUnit { - let control_hash = ControlHash { - parents_mask: NodeSubset::with_size(7.into()), - combined_hash: 0.using_encoded(Hasher64::hash), - }; + let control_hash = ControlHash::new(&NodeMap::with_size(7.into())); let pu = PreUnit::new(creator, round, control_hash); let signable = FullUnit::new(pu, Some(data), 0); Signed::sign(signable, &Keychain::new(0.into(), creator)).into_unchecked() diff --git a/consensus/src/testing/byzantine.rs b/consensus/src/testing/byzantine.rs index aaa77263..aa4abe74 100644 --- a/consensus/src/testing/byzantine.rs +++ b/consensus/src/testing/byzantine.rs @@ -107,7 +107,13 @@ impl<'a> MaliciousMember<'a> { fn create_if_possible(&mut self, round: Round) -> bool { if let Some(parents) = self.pick_parents(round) { debug!(target: "malicious-member", "Creating a legit unit for round {}.", round); - let control_hash = ControlHash::::new(&parents); + let mut node_with_parents = NodeMap::with_size(parents.size()); + if round > 0 { + for (node_index, &hash) in parents.iter() { + node_with_parents.insert(node_index, (hash, round - 1)); + } + } + let control_hash = ControlHash::::new(&node_with_parents); let new_preunit = PreUnit::::new(self.node_ix, round, control_hash); if round != self.forking_round { let full_unit = FullUnit::new(new_preunit, Some(0), self.session_id); diff --git a/consensus/src/testing/crash_recovery.rs b/consensus/src/testing/crash_recovery.rs index 89bf8815..740dae04 100644 --- a/consensus/src/testing/crash_recovery.rs +++ b/consensus/src/testing/crash_recovery.rs @@ -132,11 +132,10 @@ fn verify_backup(buf: &mut &[u8]) -> HashSet { let unit = >::decode(buf).unwrap(); let full_unit = unit.as_signable(); let coord = full_unit.coord(); - let parent_ids = &full_unit.as_pre_unit().control_hash().parents_mask; + let control_hash = &full_unit.as_pre_unit().control_hash(); - for parent_id in parent_ids.elements() { - let parent = UnitCoord::new(coord.round() - 1, parent_id); - assert!(already_saved.contains(&parent)); + for parent_coord in control_hash.parents() { + assert!(already_saved.contains(&parent_coord)); } already_saved.insert(coord); diff --git a/consensus/src/testing/dag.rs b/consensus/src/testing/dag.rs index 86f7b227..3c38b702 100644 --- a/consensus/src/testing/dag.rs +++ b/consensus/src/testing/dag.rs @@ -28,7 +28,7 @@ type Dag = GenericDag; #[derive(Clone)] struct UnitWithParents { unit: SignedUnit, - parent_hashes: NodeMap, + parent_hashes: NodeMap<(Hash64, Round)>, } impl UnitWithParents { @@ -36,7 +36,7 @@ impl UnitWithParents { round: Round, creator: NodeIndex, variant: Data, - parent_hashes: NodeMap, + parent_hashes: NodeMap<(Hash64, Round)>, ) -> Self { let keychain = Keychain::new(parent_hashes.size(), creator); let control_hash = ControlHash::new(&parent_hashes); @@ -53,7 +53,11 @@ impl UnitWithParents { } fn parent_hashes(&self) -> Vec { - self.parent_hashes.values().cloned().collect() + self.parent_hashes + .values() + .map(|(hash, _)| hash) + .cloned() + .collect() } } @@ -330,7 +334,7 @@ fn generate_random_dag( let parent = dag[previous_round_index][parent_ix.0] .choose(&mut rng) .unwrap(); - parents.insert(*parent_ix, parent.hash()); + parents.insert(*parent_ix, (parent.hash(), r - 1)); curr_n_parents += 1.into(); if curr_n_parents == n_parents { break; diff --git a/consensus/src/units/control_hash.rs b/consensus/src/units/control_hash.rs new file mode 100644 index 00000000..ab7db2fb --- /dev/null +++ b/consensus/src/units/control_hash.rs @@ -0,0 +1,395 @@ +use crate::{units::UnitCoord, Hasher, NodeCount, NodeIndex, NodeMap, Round}; +use codec::{Decode, Encode}; +use std::{ + fmt::{Display, Formatter, Result as FmtResult}, + hash::Hash, +}; + +#[derive(Eq, Debug, PartialEq)] +pub enum Error { + RoundZeroWithSomeParents(NodeCount), + RoundZeroBadControlHash(H::Hash, H::Hash), + NotDescendantOfPreviousUnit(NodeIndex), + DescendantOfPreviousUnitHasWrongRound(Round), + NotEnoughParentsForRound(Round), + ParentsHigherThanRound(Round), +} + +impl Display for Error { + fn fmt(&self, f: &mut Formatter<'_>) -> FmtResult { + match self { + Error::RoundZeroWithSomeParents(node_count) => { + write!( + f, + "zero round unit with non-empty parents count: {:?}", + node_count + ) + } + Error::RoundZeroBadControlHash(current_hash, expected_hash) => { + write!( + f, + "zero round unit with wrong control hash: {:?}, expected: {:?}", + current_hash, expected_hash + ) + } + Error::NotDescendantOfPreviousUnit(node_index) => { + write!( + f, + "unit is not descendant of its creator's previous unit, creator index: {:?}", + node_index + ) + } + Error::DescendantOfPreviousUnitHasWrongRound(round) => { + write!(f, "creator's previous unit has wrong round {:?}", round) + } + Error::NotEnoughParentsForRound(round) => { + write!(f, "unit has not enough parents from the round {:?}", round) + } + Error::ParentsHigherThanRound(round) => { + write!(f, "unit has parents higher than round {:?}", round) + } + } + } +} + +/// Combined hashes of the parents of a unit together with the set of indices of creators of the +/// parents. By parent here we mean a parent hash and its round. +#[derive(Clone, Eq, PartialEq, Hash, Debug, Decode, Encode)] +pub struct ControlHash { + parents: NodeMap, + combined_hash: H::Hash, +} + +impl ControlHash { + /// Creates new control hash from parents hashes and rounds + pub fn new(parents_with_rounds_and_hashes: &NodeMap<(H::Hash, Round)>) -> Self { + let mut parents_with_rounds = NodeMap::with_size(parents_with_rounds_and_hashes.size()); + for (parent_index, (_, parent_round)) in parents_with_rounds_and_hashes.iter() { + parents_with_rounds.insert(parent_index, *parent_round); + } + ControlHash { + parents: parents_with_rounds, + combined_hash: Self::create_control_hash(parents_with_rounds_and_hashes), + } + } + + /// Calculate parent control hash, which includes all parent hashes and their rounds into account. + pub fn create_control_hash(parent_map: &NodeMap<(H::Hash, Round)>) -> H::Hash { + parent_map.using_encoded(H::hash) + } + + pub fn combined_hash(&self) -> H::Hash { + self.combined_hash + } + + /// Iterator over non-empty parents - returns [`UnitCoord`]s + pub fn parents(&self) -> impl Iterator + '_ { + self.parents + .iter() + .map(|(node_index, &round)| UnitCoord::new(round, node_index)) + } + + /// Returns number of all members in abft consensus + pub fn n_members(&self) -> NodeCount { + self.parents.size() + } + + pub fn validate(&self, unit_coord: UnitCoord) -> Result<(), Error> { + match unit_coord.round { + 0 => self.validate_initial_round(), + _ => self.validate_non_initial_round(unit_coord), + } + } + + fn validate_initial_round(&self) -> Result<(), Error> { + let parents_count = self.parents().count(); + if parents_count > 0 { + return Err(Error::RoundZeroWithSomeParents(NodeCount(parents_count))); + } + let recalculated_control_hash = + ControlHash::::create_control_hash(&NodeMap::with_size(self.n_members())); + if self.combined_hash != recalculated_control_hash { + return Err(Error::RoundZeroBadControlHash( + self.combined_hash, + recalculated_control_hash, + )); + } + + Ok(()) + } + + fn validate_non_initial_round(&self, unit_coord: UnitCoord) -> Result<(), Error> { + assert!(unit_coord.round > 0, "Round must be greater than 0"); + + self.unit_creator_is_descendant_of_previous_unit(unit_coord)?; + self.previous_round_have_enough_parents(unit_coord.round)?; + self.check_if_parents_greater_than_previous_round(unit_coord.round)?; + + Ok(()) + } + + fn check_if_parents_greater_than_previous_round(&self, round: Round) -> Result<(), Error> { + let parents_greater_than_previous_round = self + .parents() + .any(|unit_coord| unit_coord.round > round - 1); + if parents_greater_than_previous_round { + return Err(Error::ParentsHigherThanRound(round - 1)); + } + Ok(()) + } + + fn previous_round_have_enough_parents(&self, round: Round) -> Result<(), Error> { + let previous_round_parents = self + .parents() + .filter(|&parent| parent.round == round - 1) + .count(); + if previous_round_parents < self.n_members().consensus_threshold().0 { + return Err(Error::NotEnoughParentsForRound(round - 1)); + } + Ok(()) + } + + fn unit_creator_is_descendant_of_previous_unit( + &self, + unit_coord: UnitCoord, + ) -> Result<(), Error> { + match self.parents.get(unit_coord.creator) { + None => return Err(Error::NotDescendantOfPreviousUnit(unit_coord.creator)), + Some(&parent_round) => { + if unit_coord.round - 1 != parent_round { + return Err(Error::DescendantOfPreviousUnitHasWrongRound(parent_round)); + } + } + } + Ok(()) + } +} + +#[cfg(test)] +pub mod tests { + use crate::units::{control_hash::Error, ControlHash, NodeCount, NodeIndex, UnitCoord}; + use aleph_bft_mock::Hasher64; + use aleph_bft_types::{NodeMap, Round}; + use codec::{Decode, Encode}; + + #[test] + fn given_control_hash_is_encoded_when_same_control_hash_is_decoded_then_results_are_the_same() { + let ch = + ControlHash::::new(&vec![Some(([0; 8], 2)), None, Some(([1; 8], 2))].into()); + let encoded = ch.encode(); + let decoded = + ControlHash::decode(&mut encoded.as_slice()).expect("should decode correctly"); + assert_eq!(decoded, ch); + } + + #[test] + fn given_control_hash_then_basic_properties_are_correct() { + let parent_map = vec![ + Some(([0; 8], 2)), + None, + Some(([2; 8], 2)), + Some(([3; 8], 2)), + Some(([4; 8], 2)), + Some(([5; 8], 2)), + Some(([6; 8], 1)), + ] + .into(); + let ch = ControlHash::::new(&parent_map); + assert_eq!( + ControlHash::::create_control_hash(&parent_map), + [249, 141, 250, 222, 107, 240, 194, 10] + ); + + assert_eq!(ch.parents().count(), 6); + assert_eq!(ch.n_members(), NodeCount(7)); + + let parents: Vec<_> = ch.parents().collect(); + let expected_parents = vec![ + UnitCoord::new(2, NodeIndex(0)), + UnitCoord::new(2, NodeIndex(2)), + UnitCoord::new(2, NodeIndex(3)), + UnitCoord::new(2, NodeIndex(4)), + UnitCoord::new(2, NodeIndex(5)), + UnitCoord::new(1, NodeIndex(6)), + ]; + assert_eq!(parents, expected_parents); + } + + #[test] + fn given_initial_round_when_validate_with_non_empty_parents_then_validate_returns_err() { + let parent_map = vec![ + Some(([0; 8], 2)), + None, + Some(([2; 8], 2)), + Some(([3; 8], 2)), + Some(([4; 8], 2)), + Some(([5; 8], 2)), + Some(([6; 8], 1)), + ] + .into(); + let ch = ControlHash::::new(&parent_map); + assert_eq!( + ch.validate(UnitCoord::new(0, NodeIndex(1))) + .expect_err("validate() should return error, returned Ok(()) instead"), + Error::RoundZeroWithSomeParents(NodeCount(parent_map.item_count())) + ); + } + + #[test] + fn given_initial_round_when_validate_with_different_node_count_then_validate_returns_err() { + // Correct control hash for initial unit with count = 4 is + // [16, 0, 0, 0, 0, 129, 99, 217, 65, 183, 158, 24, 201]; + // First 5 bytes is vec![None;4] scale encoded - 0x16 is Compact(4) , followed by 4 null bytes + // Then 8 bytes is Hasher64 representation of that NodeMap + // In this test, we change random byte to mimic situation e.g. attacker using different + // hash algorithm, just trying to send us garbage. Decode still work, as 8 random bytes is + // is valid generic Hasher64 representation, but validation should not work + + let correct_control_hash = ControlHash::::new(&NodeMap::with_size(NodeCount(4))); + let encoded_control_hash = correct_control_hash.encode(); + let mut borked_control_hash_bytes = encoded_control_hash[0..=7].to_vec(); + borked_control_hash_bytes.extend([129, 100, 217, 65, 183, 158, 24, 201]); + let borked_ch = ControlHash::::decode(&mut borked_control_hash_bytes.as_slice()) + .expect("should decode correctly"); + + assert_eq!( + borked_ch + .validate(UnitCoord::new(0, NodeIndex(4))) + .expect_err("validate() should return error, returned Ok(()) instead"), + Error::RoundZeroBadControlHash( + borked_ch.combined_hash, + ControlHash::::create_control_hash(&NodeMap::with_size(NodeCount(4))) + ) + ); + } + + #[test] + fn given_non_initial_round_when_validate_with_correct_unit_coord_then_validate_is_ok() { + let parent_map = vec![ + Some(([0; 8], 2)), + None, + Some(([2; 8], 2)), + Some(([3; 8], 2)), + Some(([4; 8], 2)), + Some(([5; 8], 2)), + Some(([6; 8], 1)), + ] + .into(); + let ch = ControlHash::::new(&parent_map); + assert!(ch.validate(UnitCoord::new(3, NodeIndex(2))).is_ok()); + } + + #[test] + fn given_non_initial_round_when_creator_parent_does_not_exist_then_err_is_returned_from_validate( + ) { + let parent_map = vec![ + Some(([0; 8], 2)), + None, + Some(([2; 8], 2)), + Some(([3; 8], 2)), + ] + .into(); + let ch = ControlHash::::new(&parent_map); + assert_eq!( + ch.validate(UnitCoord::new(3, NodeIndex(1))) + .expect_err("validate() should return error, returned Ok(()) instead"), + Error::NotDescendantOfPreviousUnit(NodeIndex(1)) + ); + } + + #[test] + fn given_non_initial_round_hash_when_creator_parent_exists_but_has_wrong_round_then_err_is_returned_from_validate( + ) { + let parent_map = vec![ + Some(([0; 8], 2)), + Some(([1; 8], 1)), + Some(([2; 8], 2)), + Some(([3; 8], 2)), + ] + .into(); + let ch = ControlHash::::new(&parent_map); + assert_eq!( + ch.validate(UnitCoord::new(3, NodeIndex(1))) + .expect_err("validate() should return error, returned Ok(()) instead"), + Error::DescendantOfPreviousUnitHasWrongRound(1) + ); + } + + #[test] + fn given_non_initial_round_when_there_are_not_enough_previous_round_parents_then_err_is_returned_from_validate( + ) { + let parent_map = vec![ + None, + Some(([1; 8], 2)), + Some(([2; 8], 2)), + Some(([3; 8], 1)), + ] + .into(); + let ch = ControlHash::::new(&parent_map); + assert_eq!( + ch.validate(UnitCoord::new(3, NodeIndex(1))) + .expect_err("validate() should return error, returned Ok(()) instead"), + Error::NotEnoughParentsForRound(2) + ); + } + + #[test] + fn given_non_initial_round_when_there_are_parents_from_greater_rounds_then_err_is_returned_from_validate( + ) { + let parent_map = vec![ + Some(([0; 8], 2)), + Some(([1; 8], 2)), + Some(([2; 8], 2)), + Some(([3; 8], 3)), + ] + .into(); + let ch = ControlHash::::new(&parent_map); + assert_eq!( + ch.validate(UnitCoord::new(3, NodeIndex(1))) + .expect_err("validate() should return error, returned Ok(()) instead"), + Error::ParentsHigherThanRound(2) + ); + } + + #[test] + fn given_correct_control_hash_when_only_single_property_change_then_control_hash_does_not_match( + ) { + let all_parents_from_round_three = [ + Some(([193, 179, 113, 82, 221, 179, 199, 217], 3)), + Some(([215, 1, 244, 177, 19, 155, 43, 208], 3)), + Some(([12, 108, 24, 87, 75, 135, 37, 3], 3)), + Some(([3, 221, 173, 235, 29, 224, 247, 233], 3)), + ]; + + let parents_from_round_three = &all_parents_from_round_three[0..3].to_vec().into(); + let control_hash_of_fourth_round_unit = + ControlHash::::new(parents_from_round_three); + + let mut parents_from_round_three_but_one_hash_replaced = parents_from_round_three.clone(); + parents_from_round_three_but_one_hash_replaced.insert( + NodeIndex(2), + ([234, 170, 183, 55, 61, 24, 31, 143], 3 as Round), + ); + let borked_hash_of_fourth_round_unit = + ControlHash::::new(&parents_from_round_three_but_one_hash_replaced); + assert_ne!( + borked_hash_of_fourth_round_unit, + control_hash_of_fourth_round_unit + ); + + let mut parents_from_round_three_but_one_unit_round_replaced = + parents_from_round_three.clone(); + parents_from_round_three_but_one_unit_round_replaced + .insert(NodeIndex(2), ([12, 108, 24, 87, 75, 135, 37, 3], 2)); + let control_hash_of_fourth_round_unit_but_one_unit_replaced = + ControlHash::::new(&parents_from_round_three_but_one_unit_round_replaced); + assert_ne!( + borked_hash_of_fourth_round_unit, + control_hash_of_fourth_round_unit_but_one_unit_replaced + ); + assert_ne!( + control_hash_of_fourth_round_unit_but_one_unit_replaced, + control_hash_of_fourth_round_unit + ); + } +} diff --git a/consensus/src/units/mod.rs b/consensus/src/units/mod.rs index ea251aac..5f4b8e85 100644 --- a/consensus/src/units/mod.rs +++ b/consensus/src/units/mod.rs @@ -1,17 +1,20 @@ use std::fmt::{Display, Formatter, Result as FmtResult}; use crate::{ - Data, Hasher, Index, MultiKeychain, NodeCount, NodeIndex, NodeMap, NodeSubset, Round, - SessionId, Signable, Signed, UncheckedSigned, + Data, Hasher, Index, MultiKeychain, NodeCount, NodeIndex, Round, SessionId, Signable, Signed, + UncheckedSigned, }; use codec::{Decode, Encode}; use derivative::Derivative; use parking_lot::RwLock; +mod control_hash; mod store; #[cfg(test)] mod testing; mod validator; + +pub use control_hash::{ControlHash, Error as ControlHashError}; pub(crate) use store::*; #[cfg(test)] pub use testing::{ @@ -52,39 +55,6 @@ impl Display for UnitCoord { } } -/// Combined hashes of the parents of a unit together with the set of indices of creators of the -/// parents -#[derive(Clone, Eq, PartialEq, Hash, Debug, Decode, Encode)] -pub struct ControlHash { - pub(crate) parents_mask: NodeSubset, - pub(crate) combined_hash: H::Hash, -} - -impl ControlHash { - pub(crate) fn new(parent_map: &NodeMap) -> Self { - ControlHash { - parents_mask: parent_map.to_subset(), - combined_hash: Self::combine_hashes(parent_map), - } - } - - pub(crate) fn combine_hashes(parent_map: &NodeMap) -> H::Hash { - parent_map.using_encoded(H::hash) - } - - pub(crate) fn parents(&self) -> impl Iterator + '_ { - self.parents_mask.elements() - } - - pub(crate) fn n_parents(&self) -> NodeCount { - NodeCount(self.parents().count()) - } - - pub(crate) fn n_members(&self) -> NodeCount { - NodeCount(self.parents_mask.size()) - } -} - /// The simplest type representing a unit, consisting of coordinates and a control hash #[derive(Clone, Eq, PartialEq, Hash, Debug, Decode, Encode)] pub struct PreUnit { @@ -100,10 +70,6 @@ impl PreUnit { } } - pub(crate) fn n_parents(&self) -> NodeCount { - self.control_hash.n_parents() - } - pub(crate) fn n_members(&self) -> NodeCount { self.control_hash.n_members() } @@ -218,6 +184,7 @@ pub trait UnitWithParents: Unit { fn parents(&self) -> impl Iterator>; fn direct_parents(&self) -> impl Iterator>; fn parent_for(&self, index: NodeIndex) -> Option<&HashFor>; + fn node_count(&self) -> NodeCount; } @@ -274,7 +241,7 @@ pub type HashFor = <::Hasher as Hasher>::Hash; #[cfg(test)] pub mod tests { use crate::{ - units::{random_full_parent_units_up_to, ControlHash, FullUnit, Unit}, + units::{random_full_parent_units_up_to, FullUnit, Unit}, Hasher, NodeCount, }; use aleph_bft_mock::{Data, Hasher64}; @@ -293,15 +260,6 @@ pub mod tests { } } - #[test] - fn test_control_hash_codec() { - let ch = ControlHash::::new(&vec![Some([0; 8]), None, Some([1; 8])].into()); - let encoded = ch.encode(); - let decoded = - ControlHash::decode(&mut encoded.as_slice()).expect("should decode correctly"); - assert_eq!(decoded, ch); - } - #[test] fn test_full_unit_codec() { for full_unit in random_full_parent_units_up_to(3, NodeCount(4), 43) diff --git a/consensus/src/units/testing.rs b/consensus/src/units/testing.rs index 14ad0076..b4774760 100644 --- a/consensus/src/units/testing.rs +++ b/consensus/src/units/testing.rs @@ -135,7 +135,7 @@ fn random_initial_reconstructed_units( .collect() } -fn parent_map>(parents: &Vec) -> NodeMap { +fn parent_map>(parents: &Vec) -> NodeMap<(Hash64, Round)> { let n_members = parents .last() .expect("there are parents") @@ -143,7 +143,7 @@ fn parent_map>(parents: &Vec) -> NodeMap { .n_members(); let mut result = NodeMap::with_size(n_members); for parent in parents { - result.insert(parent.creator(), parent.hash()); + result.insert(parent.creator(), (parent.hash(), parent.round())); } result } @@ -166,6 +166,7 @@ pub fn random_reconstructed_unit_with_parents>( keychain: &Keychain, round: Round, ) -> DagUnit { + assert!(round > 0); ReconstructedUnit::with_parents( full_unit_to_signed_unit(random_unit_with_parents(creator, parents, round), keychain), parent_map(parents), diff --git a/consensus/src/units/validator.rs b/consensus/src/units/validator.rs index 362e3d4a..b867b18d 100644 --- a/consensus/src/units/validator.rs +++ b/consensus/src/units/validator.rs @@ -1,7 +1,7 @@ +use crate::units::{ControlHashError, UnitCoord}; use crate::{ - units::{ControlHash, FullUnit, PreUnit, SignedUnit, UncheckedSignedUnit, Unit}, - Data, Hasher, Keychain, NodeCount, NodeIndex, NodeMap, Round, SessionId, Signature, - SignatureError, + units::{FullUnit, PreUnit, SignedUnit, UncheckedSignedUnit, Unit}, + Data, Hasher, Keychain, NodeCount, NodeIndex, Round, SessionId, Signature, SignatureError, }; use std::{ fmt::{Display, Formatter, Result as FmtResult}, @@ -15,10 +15,7 @@ pub enum ValidationError { WrongSession(FullUnit), RoundTooHigh(FullUnit), WrongNumberOfMembers(PreUnit), - RoundZeroWithParents(PreUnit), - RoundZeroBadControlHash(PreUnit), - NotEnoughParents(PreUnit), - NotDescendantOfPreviousUnit(PreUnit), + ParentValidationFailed(PreUnit, ControlHashError), } impl Display for ValidationError { @@ -34,20 +31,10 @@ impl Display for ValidationError { pu.n_members(), pu ), - RoundZeroWithParents(pu) => write!(f, "zero round unit with parents: {:?}", pu), - RoundZeroBadControlHash(pu) => { - write!(f, "zero round unit with wrong control hash: {:?}", pu) - } - NotEnoughParents(pu) => write!( - f, - "nonzero round unit with only {:?} parents: {:?}", - pu.n_parents(), - pu - ), - NotDescendantOfPreviousUnit(pu) => write!( + ParentValidationFailed(pu, control_hash_error) => write!( f, - "nonzero round unit is not descendant of its creator's previous unit: {:?}", - pu + "parent validation failed for unit: {:?}. Internal error: {}", + pu, control_hash_error ), } } @@ -105,10 +92,6 @@ impl Validator { self.validate_unit_parents(su) } - fn threshold(&self) -> NodeCount { - self.keychain.node_count().consensus_threshold() - } - fn validate_unit_parents( &self, su: SignedUnit, @@ -118,33 +101,11 @@ impl Validator { if n_members != self.keychain.node_count() { return Err(ValidationError::WrongNumberOfMembers(pre_unit.clone())); } - let round = pre_unit.round(); - let n_parents = pre_unit.n_parents(); - match round { - 0 => { - if n_parents > NodeCount(0) { - return Err(ValidationError::RoundZeroWithParents(pre_unit.clone())); - } - if pre_unit.control_hash().combined_hash - != ControlHash::::combine_hashes(&NodeMap::with_size(n_members)) - { - return Err(ValidationError::RoundZeroBadControlHash(pre_unit.clone())); - } - } - // NOTE: at this point we cannot validate correctness of the control hash, in principle it could be - // just a random hash, but we still would not be able to deduce that by looking at the unit only. - _ => { - if n_parents < self.threshold() { - return Err(ValidationError::NotEnoughParents(pre_unit.clone())); - } - let control_hash = &pre_unit.control_hash(); - if !control_hash.parents_mask[pre_unit.creator()] { - return Err(ValidationError::NotDescendantOfPreviousUnit( - pre_unit.clone(), - )); - } - } - } + let unit_coord = UnitCoord::new(pre_unit.round(), pre_unit.creator()); + pre_unit + .control_hash + .validate(unit_coord) + .map_err(|e| ValidationError::ParentValidationFailed(pre_unit.clone(), e))?; Ok(su) } } @@ -156,10 +117,12 @@ mod tests { units::{ full_unit_to_unchecked_signed_unit, preunit_to_unchecked_signed_unit, random_full_parent_units_up_to, random_unit_with_parents, PreUnit, + {ControlHash, ControlHashError}, }, NodeCount, NodeIndex, }; use aleph_bft_mock::Keychain; + use codec::{Decode, Encode}; type Validator = GenericValidator; @@ -191,13 +154,20 @@ mod tests { .as_pre_unit() .clone(); let mut control_hash = preunit.control_hash().clone(); - control_hash.combined_hash = [0, 1, 0, 1, 0, 1, 0, 1]; + let encoded = control_hash.encode(); + // first 8 bytes is encoded NodeMap of size 7 + let mut borked_control_hash_bytes = encoded[0..=7].to_vec(); + borked_control_hash_bytes.extend([0u8, 1u8, 0u8, 1u8, 0u8, 1u8, 0u8, 1u8]); + control_hash = ControlHash::decode(&mut borked_control_hash_bytes.as_slice()) + .expect("should decode correctly"); let preunit = PreUnit::new(preunit.creator(), preunit.round(), control_hash); let unchecked_unit = preunit_to_unchecked_signed_unit(preunit.clone(), session_id, &keychain); let other_preunit = match validator.validate_unit(unchecked_unit.clone()) { Ok(_) => panic!("Validated bad unit."), - Err(RoundZeroBadControlHash(unit)) => unit, + Err(ParentValidationFailed(unit, ControlHashError::RoundZeroBadControlHash(_, _))) => { + unit + } Err(e) => panic!("Unexpected error from validator: {:?}", e), }; assert_eq!(other_preunit, preunit); @@ -261,7 +231,10 @@ mod tests { let validator = Validator::new(session_id, keychain, max_round); let other_preunit = match validator.validate_unit(unchecked_unit) { Ok(_) => panic!("Validated bad unit."), - Err(NotEnoughParents(other_preunit)) => other_preunit, + Err(ParentValidationFailed( + other_preunit, + ControlHashError::NotEnoughParentsForRound(_), + )) => other_preunit, Err(e) => panic!("Unexpected error from validator: {:?}", e), }; assert_eq!(other_preunit, preunit);