From ab4de940175d88e25f91b5385195caf544f5887f Mon Sep 17 00:00:00 2001 From: Serban Iorga Date: Thu, 19 Oct 2023 08:16:40 +0300 Subject: [PATCH] Grandpa justifications: Avoid duplicate vote ancestries (#2634) (#2635) * Grandpa justifications: Avoid duplicate vote ancestries --- .../verification/equivocation.rs | 7 +++ .../src/justification/verification/mod.rs | 59 +++++++++++++++---- .../justification/verification/optimizer.rs | 15 +++++ .../src/justification/verification/strict.rs | 9 ++- .../tests/implementation_match.rs | 4 +- .../tests/justification/optimizer.rs | 20 +++++++ .../tests/justification/strict.rs | 16 ++++- 7 files changed, 114 insertions(+), 16 deletions(-) diff --git a/primitives/header-chain/src/justification/verification/equivocation.rs b/primitives/header-chain/src/justification/verification/equivocation.rs index e2d7a8e804c2..fbad30128199 100644 --- a/primitives/header-chain/src/justification/verification/equivocation.rs +++ b/primitives/header-chain/src/justification/verification/equivocation.rs @@ -101,6 +101,13 @@ impl<'a, Header: HeaderT> EquivocationsCollector<'a, Header> { } impl<'a, Header: HeaderT> JustificationVerifier
for EquivocationsCollector<'a, Header> { + fn process_duplicate_votes_ancestries( + &mut self, + _duplicate_votes_ancestries: Vec, + ) -> Result<(), JustificationVerificationError> { + Ok(()) + } + fn process_redundant_vote( &mut self, _precommit_idx: usize, diff --git a/primitives/header-chain/src/justification/verification/mod.rs b/primitives/header-chain/src/justification/verification/mod.rs index a66fc1e0d91d..c71149bf9c28 100644 --- a/primitives/header-chain/src/justification/verification/mod.rs +++ b/primitives/header-chain/src/justification/verification/mod.rs @@ -27,7 +27,13 @@ use finality_grandpa::voter_set::VoterSet; use sp_consensus_grandpa::{AuthorityId, AuthoritySignature, SetId}; use sp_runtime::{traits::Header as HeaderT, RuntimeDebug}; use sp_std::{ - collections::{btree_map::BTreeMap, btree_set::BTreeSet}, + collections::{ + btree_map::{ + BTreeMap, + Entry::{Occupied, Vacant}, + }, + btree_set::BTreeSet, + }, prelude::*, }; @@ -44,23 +50,40 @@ pub struct AncestryChain { /// We expect all forks in the ancestry chain to be descendants of base. base: HeaderId, /// Header hash => parent header hash mapping. - pub parents: BTreeMap, + parents: BTreeMap, /// Hashes of headers that were not visited by `ancestry()`. - pub unvisited: BTreeSet, + unvisited: BTreeSet, } impl AncestryChain
{ - /// Create new ancestry chain. - pub fn new(justification: &GrandpaJustification
) -> AncestryChain
{ + /// Creates a new instance of `AncestryChain` starting from a `GrandpaJustification`. + /// + /// Returns the `AncestryChain` and a `Vec` containing the `votes_ancestries` entries + /// that were ignored when creating it, because they are duplicates. + pub fn new( + justification: &GrandpaJustification
, + ) -> (AncestryChain
, Vec) { let mut parents = BTreeMap::new(); let mut unvisited = BTreeSet::new(); - for ancestor in &justification.votes_ancestries { + let mut ignored_idxs = Vec::new(); + for (idx, ancestor) in justification.votes_ancestries.iter().enumerate() { let hash = ancestor.hash(); - let parent_hash = *ancestor.parent_hash(); - parents.insert(hash, parent_hash); - unvisited.insert(hash); + match parents.entry(hash) { + Occupied(_) => { + ignored_idxs.push(idx); + }, + Vacant(entry) => { + entry.insert(*ancestor.parent_hash()); + unvisited.insert(hash); + }, + } } - AncestryChain { base: justification.commit_target_id(), parents, unvisited } + (AncestryChain { base: justification.commit_target_id(), parents, unvisited }, ignored_idxs) + } + + /// Returns the hash of a block's parent if the block is present in the ancestry. + pub fn parent_hash_of(&self, hash: &Header::Hash) -> Option<&Header::Hash> { + self.parents.get(hash) } /// Returns a route if the precommit target block is a descendant of the `base` block. @@ -80,7 +103,7 @@ impl AncestryChain
{ break } - current_hash = match self.parents.get(¤t_hash) { + current_hash = match self.parent_hash_of(¤t_hash) { Some(parent_hash) => { let is_visited_before = self.unvisited.get(¤t_hash).is_none(); if is_visited_before { @@ -117,6 +140,8 @@ pub enum Error { InvalidAuthorityList, /// Justification is finalizing unexpected header. InvalidJustificationTarget, + /// The justification contains duplicate headers in its `votes_ancestries` field. + DuplicateVotesAncestries, /// Error validating a precommit Precommit(PrecommitError), /// The cumulative weight of all votes in the justification is not enough to justify commit @@ -168,6 +193,12 @@ enum IterationFlow { /// Verification callbacks. trait JustificationVerifier { + /// Called when there are duplicate headers in the votes ancestries. + fn process_duplicate_votes_ancestries( + &mut self, + duplicate_votes_ancestries: Vec, + ) -> Result<(), Error>; + fn process_redundant_vote( &mut self, precommit_idx: usize, @@ -216,10 +247,14 @@ trait JustificationVerifier { } let threshold = context.voter_set.threshold().get(); - let mut chain = AncestryChain::new(justification); + let (mut chain, ignored_idxs) = AncestryChain::new(justification); let mut signature_buffer = Vec::new(); let mut cumulative_weight = 0u64; + if !ignored_idxs.is_empty() { + self.process_duplicate_votes_ancestries(ignored_idxs)?; + } + for (precommit_idx, signed) in justification.commit.precommits.iter().enumerate() { if cumulative_weight >= threshold { let action = diff --git a/primitives/header-chain/src/justification/verification/optimizer.rs b/primitives/header-chain/src/justification/verification/optimizer.rs index 6552b359170a..3f1e6ab670ca 100644 --- a/primitives/header-chain/src/justification/verification/optimizer.rs +++ b/primitives/header-chain/src/justification/verification/optimizer.rs @@ -33,6 +33,7 @@ struct JustificationOptimizer { votes: BTreeSet, extra_precommits: Vec, + duplicate_votes_ancestries_idxs: Vec, redundant_votes_ancestries: BTreeSet, } @@ -41,6 +42,11 @@ impl JustificationOptimizer
{ for invalid_precommit_idx in self.extra_precommits.into_iter().rev() { justification.commit.precommits.remove(invalid_precommit_idx); } + if !self.duplicate_votes_ancestries_idxs.is_empty() { + for idx in self.duplicate_votes_ancestries_idxs.iter().rev() { + justification.votes_ancestries.swap_remove(*idx); + } + } if !self.redundant_votes_ancestries.is_empty() { justification .votes_ancestries @@ -50,6 +56,14 @@ impl JustificationOptimizer
{ } impl JustificationVerifier
for JustificationOptimizer
{ + fn process_duplicate_votes_ancestries( + &mut self, + duplicate_votes_ancestries: Vec, + ) -> Result<(), Error> { + self.duplicate_votes_ancestries_idxs = duplicate_votes_ancestries.to_vec(); + Ok(()) + } + fn process_redundant_vote( &mut self, precommit_idx: usize, @@ -118,6 +132,7 @@ pub fn verify_and_optimize_justification( let mut optimizer = JustificationOptimizer { votes: BTreeSet::new(), extra_precommits: vec![], + duplicate_votes_ancestries_idxs: vec![], redundant_votes_ancestries: Default::default(), }; optimizer.verify_justification(finalized_target, context, justification)?; diff --git a/primitives/header-chain/src/justification/verification/strict.rs b/primitives/header-chain/src/justification/verification/strict.rs index f899c6c8efc0..858cf517a431 100644 --- a/primitives/header-chain/src/justification/verification/strict.rs +++ b/primitives/header-chain/src/justification/verification/strict.rs @@ -26,7 +26,7 @@ use crate::justification::verification::{ }; use sp_consensus_grandpa::AuthorityId; use sp_runtime::traits::Header as HeaderT; -use sp_std::collections::btree_set::BTreeSet; +use sp_std::{collections::btree_set::BTreeSet, vec::Vec}; /// Verification callbacks that reject all unknown, duplicate or redundant votes. struct StrictJustificationVerifier { @@ -34,6 +34,13 @@ struct StrictJustificationVerifier { } impl JustificationVerifier
for StrictJustificationVerifier { + fn process_duplicate_votes_ancestries( + &mut self, + _duplicate_votes_ancestries: Vec, + ) -> Result<(), Error> { + Err(Error::DuplicateVotesAncestries) + } + fn process_redundant_vote( &mut self, _precommit_idx: usize, diff --git a/primitives/header-chain/tests/implementation_match.rs b/primitives/header-chain/tests/implementation_match.rs index f664a419621f..1f61f91ff4bb 100644 --- a/primitives/header-chain/tests/implementation_match.rs +++ b/primitives/header-chain/tests/implementation_match.rs @@ -42,7 +42,7 @@ struct AncestryChain(bp_header_chain::justification::AncestryChain); impl AncestryChain { fn new(justification: &GrandpaJustification) -> Self { - Self(bp_header_chain::justification::AncestryChain::new(justification)) + Self(bp_header_chain::justification::AncestryChain::new(justification).0) } } @@ -58,7 +58,7 @@ impl finality_grandpa::Chain for AncestryChain { if current_hash == base { break } - match self.0.parents.get(¤t_hash) { + match self.0.parent_hash_of(¤t_hash) { Some(parent_hash) => { current_hash = *parent_hash; route.push(current_hash); diff --git a/primitives/header-chain/tests/justification/optimizer.rs b/primitives/header-chain/tests/justification/optimizer.rs index 21bcd7e86b51..8d7e2d650256 100644 --- a/primitives/header-chain/tests/justification/optimizer.rs +++ b/primitives/header-chain/tests/justification/optimizer.rs @@ -158,6 +158,26 @@ fn unrelated_ancestry_votes_are_removed_by_optimizer() { assert_eq!(num_precommits_before - 1, num_precommits_after); } +#[test] +fn duplicate_votes_ancestries_are_removed_by_optimizer() { + let mut justification = make_default_justification::(&test_header(1)); + let optimized_votes_ancestries = justification.votes_ancestries.clone(); + justification.votes_ancestries = justification + .votes_ancestries + .into_iter() + .flat_map(|item| std::iter::repeat(item).take(3)) + .collect(); + + verify_and_optimize_justification::( + header_id::(1), + &verification_context(TEST_GRANDPA_SET_ID), + &mut justification, + ) + .unwrap(); + + assert_eq!(justification.votes_ancestries, optimized_votes_ancestries); +} + #[test] fn redundant_votes_ancestries_are_removed_by_optimizer() { let mut justification = make_default_justification::(&test_header(1)); diff --git a/primitives/header-chain/tests/justification/strict.rs b/primitives/header-chain/tests/justification/strict.rs index 188c9f5baba2..639a669572b2 100644 --- a/primitives/header-chain/tests/justification/strict.rs +++ b/primitives/header-chain/tests/justification/strict.rs @@ -149,7 +149,21 @@ fn justification_with_invalid_authority_signature_rejected() { } #[test] -fn justification_with_invalid_precommit_ancestry() { +fn justification_with_duplicate_votes_ancestry() { + let mut justification = make_default_justification::(&test_header(1)); + justification.votes_ancestries.push(justification.votes_ancestries[0].clone()); + + assert_eq!( + verify_justification::( + header_id::(1), + &verification_context(TEST_GRANDPA_SET_ID), + &justification, + ), + Err(JustificationVerificationError::DuplicateVotesAncestries), + ); +} +#[test] +fn justification_with_redundant_votes_ancestry() { let mut justification = make_default_justification::(&test_header(1)); justification.votes_ancestries.push(test_header(10));