diff --git a/consensus/src/units/control_hash.rs b/consensus/src/units/control_hash.rs index a415e342..5ae747d1 100644 --- a/consensus/src/units/control_hash.rs +++ b/consensus/src/units/control_hash.rs @@ -1,14 +1,66 @@ use crate::{units::UnitCoord, Hasher, NodeCount, NodeIndex, NodeMap, Round}; use codec::{Decode, Encode}; +use std::fmt::{Display, Formatter, Result as FmtResult}; +use std::hash::Hash; -#[derive(Debug, PartialEq)] -pub enum Error { +#[derive(Eq, Debug, PartialEq)] +pub enum Error { + RoundZeroWithSomeParents(NodeCount), + RoundZeroBadControlHash(H::Hash, H::Hash), NotDescendantOfPreviousUnit(NodeIndex), DescendantOfPreviousUnitButWrongRound(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, + "non-zero round unit is not descendant of its creator's previous unit, creator index: {:?}", + node_index) + } + Error::DescendantOfPreviousUnitButWrongRound(round) => { + write!( + f, + "non-zero round unit is descendant of its creator's previous unit, but has wrong round {:?}", + round + ) + } + Error::NotEnoughParentsForRound(round) => { + write!( + f, + "non-zero round unit has not enough parents from the round {:?}", + round + ) + } + Error::ParentsHigherThanRound(round) => { + write!( + f, + "non-zero round 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)] @@ -42,48 +94,63 @@ impl ControlHash { .map(|(node_index, &round)| UnitCoord::new(round, node_index)) } - /// Returns number of non-empty parents - pub(crate) fn n_parents(&self) -> NodeCount { - NodeCount(self.parents().count()) - } - /// Returns number of all members in abft consensus pub(crate) fn n_members(&self) -> NodeCount { self.parents.size() } - /// Validate - pub fn validate(&self, unit_coord: UnitCoord) -> Result<(), Error> { + 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)?; - self.check_if_parents_greater_than_previous_round(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, - unit_coord: UnitCoord, - ) -> Result<(), Error> { + fn check_if_parents_greater_than_previous_round(&self, round: Round) -> Result<(), Error> { let parents_greater_than_previous_round = self .parents() - .filter(|&parent| parent.round > unit_coord.round - 1) + .filter(|&parent| parent.round > round - 1) .count(); if parents_greater_than_previous_round > 0 { - return Err(Error::ParentsHigherThanRound(unit_coord.round - 1)); + return Err(Error::ParentsHigherThanRound(round - 1)); } Ok(()) } - fn previous_round_have_enough_parents(&self, unit_coord: UnitCoord) -> Result<(), Error> { + fn previous_round_have_enough_parents(&self, round: Round) -> Result<(), Error> { let previous_round_parents = self .parents() - .filter(|&parent| parent.round == unit_coord.round - 1) + .filter(|&parent| parent.round == round - 1) .count(); if previous_round_parents < self.n_members().consensus_threshold().0 { - return Err(Error::NotEnoughParentsForRound(unit_coord.round - 1)); + return Err(Error::NotEnoughParentsForRound(round - 1)); } Ok(()) } @@ -91,7 +158,7 @@ impl ControlHash { fn unit_creator_is_descendant_of_previous_unit( &self, unit_coord: UnitCoord, - ) -> Result<(), Error> { + ) -> Result<(), Error> { match self.parents.get(unit_coord.creator) { None => return Err(Error::NotDescendantOfPreviousUnit(unit_coord.creator)), Some(&parent_round) => { @@ -110,7 +177,7 @@ pub mod tests { use crate::units::UnitCoord; use crate::{units::ControlHash, NodeCount, NodeIndex}; use aleph_bft_mock::Hasher64; - use aleph_bft_types::Round; + use aleph_bft_types::{NodeMap, Round}; use codec::{Decode, Encode}; #[test] @@ -141,7 +208,7 @@ pub mod tests { [119, 216, 234, 178, 169, 143, 117, 127] ); - assert_eq!(ch.n_parents(), NodeCount(6)); + assert_eq!(ch.parents().count(), 6); assert_eq!(ch.n_members(), NodeCount(7)); let parents: Vec<_> = ch.parents().collect(); @@ -157,7 +224,52 @@ pub mod tests { } #[test] - fn given_control_hash_when_validate_with_correct_unit_coord_then_validate_is_ok() { + 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(([5; 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 borked_ch_bytes: Vec = vec![16, 0, 0, 0, 0, 129, 100, 217, 65, 183, 158, 24, 201]; + let borked_ch = ControlHash::::decode(&mut borked_ch_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, @@ -174,7 +286,7 @@ pub mod tests { #[test] #[should_panic(expected = "Round must be greater than 0")] - fn given_control_hash_when_validate_called_for_initial_unit_then_validate_panics() { + fn given_initial_round_when_validate_called_for_initial_unit_then_function_panics() { let parent_map = vec![ Some(([0; 8], 2)), None, @@ -183,10 +295,11 @@ pub mod tests { ] .into(); let ch = ControlHash::::new(&parent_map); - let _ = ch.validate(UnitCoord::new(0, NodeIndex(1))); + let _ = ch.validate_non_initial_round(UnitCoord::new(0, NodeIndex(1))); } #[test] - fn given_control_hash_when_creator_parent_does_not_exist_then_err_is_returned_from_validate() { + 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, @@ -203,7 +316,7 @@ pub mod tests { } #[test] - fn given_control_hash_when_creator_parent_exists_but_has_wrong_round_then_err_is_returned_from_validate( + 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)), @@ -221,7 +334,7 @@ pub mod tests { } #[test] - fn given_control_hash_when_there_are_not_enough_previous_round_parents_then_err_is_returned_from_validate( + fn given_non_initial_round_when_there_are_not_enough_previous_round_parents_then_err_is_returned_from_validate( ) { let parent_map = vec![ None, @@ -239,7 +352,7 @@ pub mod tests { } #[test] - fn given_control_hash_when_there_are_parents_from_greater_rounds_then_err_is_returned_from_validate( + 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)), @@ -257,7 +370,8 @@ pub mod tests { } #[test] - fn given_correct_control_hash_when_only_round_change_then_control_hash_does_not_match() { + fn given_correct_control_hash_when_only_single_property_change_then_control_hash_does_not_match( + ) { let all_parents_from_round_three = vec![ Some(([193, 179, 113, 82, 221, 179, 199, 217], 3)), Some(([215, 1, 244, 177, 19, 155, 43, 208], 3)), diff --git a/consensus/src/units/mod.rs b/consensus/src/units/mod.rs index 802f41e7..5f4b8e85 100644 --- a/consensus/src/units/mod.rs +++ b/consensus/src/units/mod.rs @@ -14,7 +14,7 @@ mod store; mod testing; mod validator; -pub use control_hash::ControlHash; +pub use control_hash::{ControlHash, Error as ControlHashError}; pub(crate) use store::*; #[cfg(test)] pub use testing::{ @@ -70,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() } diff --git a/consensus/src/units/validator.rs b/consensus/src/units/validator.rs index 9f73823c..4784c419 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 nit: {:?}. 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,10 @@ 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::::create_control_hash(&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.get(pre_unit.creator()).is_none() { - 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) } } @@ -152,6 +112,7 @@ impl Validator { #[cfg(test)] mod tests { use super::{ValidationError::*, Validator as GenericValidator}; + use crate::units::ControlHashError; use crate::{ units::{ full_unit_to_unchecked_signed_unit, preunit_to_unchecked_signed_unit, @@ -197,7 +158,9 @@ mod tests { 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 +224,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);