GRANDPA: optimize votes_ancestries when needed (paritytech#2262) (par…

…itytech#2264)

* GRANDPA: optimize votes_ancestries when needed

* Address review comments

bridges/primitives/header-chain/src/justification.rs

@@ -21,7 +21,7 @@
 
 use crate::ChainWithGrandpa;
 
-use bp_runtime::{BlockNumberOf, Chain, HashOf};
+use bp_runtime::{BlockNumberOf, Chain, HashOf, HeaderId};
 use codec::{Decode, Encode, MaxEncodedLen};
 use finality_grandpa::voter_set::VoterSet;
 use frame_support::{RuntimeDebug, RuntimeDebugNoBound};
@@ -84,6 +84,10 @@ impl<H: HeaderT> GrandpaJustification<H> {
  8u32.saturating_add(max_expected_signed_commit_size)
  .saturating_add(max_expected_votes_ancestries_size)
  }
+
+ pub fn commit_target_id(&self) -> HeaderId<H::Hash, H::Number> {
+ HeaderId(self.commit.target_number, self.commit.target_hash)
+ }
 }
 
 impl<H: HeaderT> crate::FinalityProof<H::Number> for GrandpaJustification<H> {
@@ -109,12 +113,12 @@ pub enum Error {
  InvalidAuthoritySignature,
  /// The justification contains precommit for header that is not a descendant of the commit
  /// header.
- PrecommitIsNotCommitDescendant,
+ UnrelatedAncestryVote,
  /// The cumulative weight of all votes in the justification is not enough to justify commit
  /// header finalization.
  TooLowCumulativeWeight,
  /// The justification contains extra (unused) headers in its `votes_ancestries` field.
- ExtraHeadersInVotesAncestries,
+ RedundantVotesAncestries,
 }
 
 /// Given GRANDPA authorities set size, return number of valid authorities votes that the
@@ -139,20 +143,25 @@ pub fn verify_and_optimize_justification<Header: HeaderT>(
  finalized_target: (Header::Hash, Header::Number),
  authorities_set_id: SetId,
  authorities_set: &VoterSet<AuthorityId>,
- justification: GrandpaJustification<Header>,
-) -> Result<GrandpaJustification<Header>, Error>
+ justification: &mut GrandpaJustification<Header>,
+) -> Result<(), Error>
 where
  Header::Number: finality_grandpa::BlockNumberOps,
 {
- let mut optimizer = OptimizationCallbacks(Vec::new());
+ let mut optimizer = OptimizationCallbacks {
+ extra_precommits: vec![],
+ redundant_votes_ancestries: Default::default(),
+ };
  verify_justification_with_callbacks(
  finalized_target,
  authorities_set_id,
  authorities_set,
- &justification,
+ justification,
  &mut optimizer,
  )?;
- Ok(optimizer.optimize(justification))
+ optimizer.optimize(justification);
+
+ Ok(())
 }
 
 /// Verify that justification, that is generated by given authority set, finalizes given header.
@@ -175,19 +184,28 @@ where
 }
 
 /// Verification callbacks.
-trait VerificationCallbacks {
+trait VerificationCallbacks<Header: HeaderT> {
  /// Called when we see a precommit from unknown authority.
  fn on_unkown_authority(&mut self, precommit_idx: usize) -> Result<(), Error>;
  /// Called when we see a precommit with duplicate vote from known authority.
  fn on_duplicate_authority_vote(&mut self, precommit_idx: usize) -> Result<(), Error>;
+ /// Called when we see a precommit with an invalid signature.
+ fn on_invalid_authority_signature(&mut self, precommit_idx: usize) -> Result<(), Error>;
  /// Called when we see a precommit after we've collected enough votes from authorities.
  fn on_redundant_authority_vote(&mut self, precommit_idx: usize) -> Result<(), Error>;
+ /// Called when we see a precommit that is not a descendant of the commit target.
+ fn on_unrelated_ancestry_vote(&mut self, precommit_idx: usize) -> Result<(), Error>;
+ /// Called when there are redundant headers in the votes ancestries.
+ fn on_redundant_votes_ancestries(
+ &mut self,
+ redundant_votes_ancestries: BTreeSet<Header::Hash>,
+ ) -> Result<(), Error>;
 }
 
 /// Verification callbacks that reject all unknown, duplicate or redundant votes.
 struct StrictVerificationCallbacks;
 
-impl VerificationCallbacks for StrictVerificationCallbacks {
+impl<Header: HeaderT> VerificationCallbacks<Header> for StrictVerificationCallbacks {
  fn on_unkown_authority(&mut self, _precommit_idx: usize) -> Result<(), Error> {
  Err(Error::UnknownAuthorityVote)
  }
@@ -196,45 +214,82 @@ impl VerificationCallbacks for StrictVerificationCallbacks {
  Err(Error::DuplicateAuthorityVote)
  }
 
+ fn on_invalid_authority_signature(&mut self, _precommit_idx: usize) -> Result<(), Error> {
+ Err(Error::InvalidAuthoritySignature)
+ }
+
  fn on_redundant_authority_vote(&mut self, _precommit_idx: usize) -> Result<(), Error> {
  Err(Error::RedundantVotesInJustification)
  }
+
+ fn on_unrelated_ancestry_vote(&mut self, _precommit_idx: usize) -> Result<(), Error> {
+ Err(Error::UnrelatedAncestryVote)
+ }
+
+ fn on_redundant_votes_ancestries(
+ &mut self,
+ _redundant_votes_ancestries: BTreeSet<Header::Hash>,
+ ) -> Result<(), Error> {
+ Err(Error::RedundantVotesAncestries)
+ }
 }
 
 /// Verification callbacks for justification optimization.
-struct OptimizationCallbacks(Vec<usize>);
-
-impl OptimizationCallbacks {
- fn optimize<Header: HeaderT>(
- self,
- mut justification: GrandpaJustification<Header>,
- ) -> GrandpaJustification<Header> {
- for invalid_precommit_idx in self.0.into_iter().rev() {
+struct OptimizationCallbacks<Header: HeaderT> {
+ extra_precommits: Vec<usize>,
+ redundant_votes_ancestries: BTreeSet<Header::Hash>,
+}
+
+impl<Header: HeaderT> OptimizationCallbacks<Header> {
+ fn optimize(self, justification: &mut GrandpaJustification<Header>) {
+ for invalid_precommit_idx in self.extra_precommits.into_iter().rev() {
  justification.commit.precommits.remove(invalid_precommit_idx);
  }
- justification
+ if !self.redundant_votes_ancestries.is_empty() {
+ justification
+ .votes_ancestries
+ .retain(|header| !self.redundant_votes_ancestries.contains(&header.hash()))
+ }
  }
 }
 
-impl VerificationCallbacks for OptimizationCallbacks {
+impl<Header: HeaderT> VerificationCallbacks<Header> for OptimizationCallbacks<Header> {
  fn on_unkown_authority(&mut self, precommit_idx: usize) -> Result<(), Error> {
- self.0.push(precommit_idx);
+ self.extra_precommits.push(precommit_idx);
  Ok(())
  }
 
  fn on_duplicate_authority_vote(&mut self, precommit_idx: usize) -> Result<(), Error> {
- self.0.push(precommit_idx);
+ self.extra_precommits.push(precommit_idx);
+ Ok(())
+ }
+
+ fn on_invalid_authority_signature(&mut self, precommit_idx: usize) -> Result<(), Error> {
+ self.extra_precommits.push(precommit_idx);
  Ok(())
  }
 
  fn on_redundant_authority_vote(&mut self, precommit_idx: usize) -> Result<(), Error> {
- self.0.push(precommit_idx);
+ self.extra_precommits.push(precommit_idx);
+ Ok(())
+ }
+
+ fn on_unrelated_ancestry_vote(&mut self, precommit_idx: usize) -> Result<(), Error> {
+ self.extra_precommits.push(precommit_idx);
+ Ok(())
+ }
+
+ fn on_redundant_votes_ancestries(
+ &mut self,
+ redundant_votes_ancestries: BTreeSet<Header::Hash>,
+ ) -> Result<(), Error> {
+ self.redundant_votes_ancestries = redundant_votes_ancestries;
  Ok(())
  }
 }
 
 /// Verify that justification, that is generated by given authority set, finalizes given header.
-fn verify_justification_with_callbacks<Header: HeaderT, C: VerificationCallbacks>(
+fn verify_justification_with_callbacks<Header: HeaderT, C: VerificationCallbacks<Header>>(
  finalized_target: (Header::Hash, Header::Number),
  authorities_set_id: SetId,
  authorities_set: &VoterSet<AuthorityId>,
@@ -249,8 +304,8 @@ where
  return Err(Error::InvalidJustificationTarget)
  }
 
- let threshold = authorities_set.threshold().0.into();
- let mut chain = AncestryChain::new(&justification.votes_ancestries);
+ let threshold = authorities_set.threshold().get();
+ let mut chain = AncestryChain::new(justification);
  let mut signature_buffer = Vec::new();
  let mut votes = BTreeSet::new();
  let mut cumulative_weight = 0u64;
@@ -277,34 +332,20 @@ where
  // there's a lot of code in `validate_commit` and `import_precommit` functions inside
  // `finality-grandpa` crate (mostly related to reporting equivocations). But the only thing
  // that we care about is that only first vote from the authority is accepted
- if !votes.insert(signed.id.clone()) {
+ if votes.contains(&signed.id) {
  callbacks.on_duplicate_authority_vote(precommit_idx)?;
  continue
  }
 
- // everything below this line can't just `continue`, because state is already altered
-
- // precommits aren't allowed for block lower than the target
- if signed.precommit.target_number < justification.commit.target_number {
- return Err(Error::PrecommitIsNotCommitDescendant)
- }
- // all precommits must be descendants of target block
- chain = chain
- .ensure_descendant(&justification.commit.target_hash, &signed.precommit.target_hash)?;
- // since we know now that the precommit target is the descendant of the justification
- // target, we may increase 'weight' of the justification target
- //
- // there's a lot of code in the `VoteGraph::insert` method inside `finality-grandpa` crate,
- // but in the end it is only used to find GHOST, which we don't care about. The only thing
- // that we care about is that the justification target has enough weight
- cumulative_weight = cumulative_weight.checked_add(authority_info.weight().0.into()).expect(
- "sum of weights of ALL authorities is expected not to overflow - this is guaranteed by\
- existence of VoterSet;\
- the order of loop conditions guarantees that we can account vote from same authority\
- multiple times;\
- thus we'll never overflow the u64::MAX;\
- qed",
- );
+ // all precommits must be descendants of the target block
+ let route =
+ match chain.ancestry(&signed.precommit.target_hash, &signed.precommit.target_number) {
+ Some(route) => route,
+ None => {
+ callbacks.on_unrelated_ancestry_vote(precommit_idx)?;
+ continue
+ },
+ };
 
  // verify authority signature
  if !sp_consensus_grandpa::check_message_signature_with_buffer(
@@ -315,76 +356,98 @@ where
  authorities_set_id,
  &mut signature_buffer,
  ) {
- return Err(Error::InvalidAuthoritySignature)
+ callbacks.on_invalid_authority_signature(precommit_idx)?;
+ continue
  }
+
+ // now we can count the vote since we know that it is valid
+ votes.insert(signed.id.clone());
+ chain.mark_route_as_visited(route);
+ cumulative_weight = cumulative_weight.saturating_add(authority_info.weight().get());
  }
 
- // check that there are no extra headers in the justification
- if !chain.unvisited.is_empty() {
- return Err(Error::ExtraHeadersInVotesAncestries)
+ // check that the cumulative weight of validators that voted for the justification target (or
+ // one of its descendents) is larger than the required threshold.
+ if cumulative_weight < threshold {
+ return Err(Error::TooLowCumulativeWeight)
  }
 
- // check that the cumulative weight of validators voted for the justification target (or one
- // of its descendents) is larger than required threshold.
- if cumulative_weight >= threshold {
- Ok(())
- } else {
- Err(Error::TooLowCumulativeWeight)
+ // check that there are no extra headers in the justification
+ if !chain.is_fully_visited() {
+ callbacks.on_redundant_votes_ancestries(chain.unvisited)?;
  }
+
+ Ok(())
 }
 
 /// Votes ancestries with useful methods.
 #[derive(RuntimeDebug)]
 pub struct AncestryChain<Header: HeaderT> {
+ /// We expect all forks in the ancestry chain to be descendants of base.
+ base: HeaderId<Header::Hash, Header::Number>,
  /// Header hash => parent header hash mapping.
  pub parents: BTreeMap<Header::Hash, Header::Hash>,
- /// Hashes of headers that were not visited by `is_ancestor` method.
+ /// Hashes of headers that were not visited by `ancestry()`.
  pub unvisited: BTreeSet<Header::Hash>,
 }
 
 impl<Header: HeaderT> AncestryChain<Header> {
  /// Create new ancestry chain.
- pub fn new(ancestry: &[Header]) -> AncestryChain<Header> {
+ pub fn new(justification: &GrandpaJustification<Header>) -> AncestryChain<Header> {
  let mut parents = BTreeMap::new();
  let mut unvisited = BTreeSet::new();
- for ancestor in ancestry {
+ for ancestor in &justification.votes_ancestries {
  let hash = ancestor.hash();
  let parent_hash = *ancestor.parent_hash();
  parents.insert(hash, parent_hash);
  unvisited.insert(hash);
  }
- AncestryChain { parents, unvisited }
+ AncestryChain { base: justification.commit_target_id(), parents, unvisited }
  }
 
- /// Returns `Ok(_)` if `precommit_target` is a descendant of the `commit_target` block and
- /// `Err(_)` otherwise.
- pub fn ensure_descendant(
- mut self,
- commit_target: &Header::Hash,
- precommit_target: &Header::Hash,
- ) -> Result<Self, Error> {
- let mut current_hash = *precommit_target;
+ /// Returns a route if the precommit target block is a descendant of the `base` block.
+ pub fn ancestry(
+ &self,
+ precommit_target_hash: &Header::Hash,
+ precommit_target_number: &Header::Number,
+ ) -> Option<Vec<Header::Hash>> {
+ if precommit_target_number < &self.base.number() {
+ return None
+ }
+
+ let mut route = vec![];
+ let mut current_hash = *precommit_target_hash;
  loop {
- if current_hash == *commit_target {
+ if current_hash == self.base.hash() {
  break
  }
 
- let is_visited_before = !self.unvisited.remove(&current_hash);
  current_hash = match self.parents.get(&current_hash) {
  Some(parent_hash) => {
+ let is_visited_before = self.unvisited.get(&current_hash).is_none();
  if is_visited_before {
- // `Some(parent_hash)` means that the `current_hash` is in the `parents`
- // container `is_visited_before` means that it has been visited before in
- // some of previous calls => since we assume that previous call has finished
- // with `true`, this also will be finished with `true`
- return Ok(self)
+ // If the current header has been visited in a previous call, it is a
+ // descendent of `base` (we assume that the previous call was successful).
+ return Some(route)
  }
+ route.push(current_hash);
 
  *parent_hash
  },
- None => return Err(Error::PrecommitIsNotCommitDescendant),
+ None => return None,
  };
  }
- Ok(self)
+
+ Some(route)
+ }
+
+ fn mark_route_as_visited(&mut self, route: Vec<Header::Hash>) {
+ for hash in route {
+ self.unvisited.remove(&hash);
+ }
+ }
+
+ fn is_fully_visited(&self) -> bool {
+ self.unvisited.is_empty()
  }
 }