Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Backport: GRANDPA: optimize votes_ancestries when needed #2264

Merged
merged 1 commit into from
Jul 13, 2023
Merged

Backport: GRANDPA: optimize votes_ancestries when needed #2264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
225 changes: 144 additions & 81 deletions primitives/header-chain/src/justification.rs
View file
Open in desktop
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