Skip to content

Commit

Permalink
GRANDPA: optimize votes_ancestries when needed (paritytech#2262) (par…
Browse files Browse the repository at this point in the history 
…itytech#2264)

* GRANDPA: optimize votes_ancestries when needed

* Address review comments
  • Loading branch information
@serban300
serban300 committed Apr 8, 2024
1 parent 8519dae commit 7e3965b
Show file tree
Hide file tree
Showing 6 changed files with 255 additions and 115 deletions.
225 changes: 144 additions & 81 deletions bridges/primitives/header-chain/src/justification.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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> {
Expand All @@ -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
Expand All @@ -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.
Expand All @@ -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)
}
Expand All @@ -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>,
Expand All @@ -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;
Expand All @@ -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(
Expand All @@ -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()
}
}
Loading

0 comments on commit 7e3965b

Please sign in to comment.