Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

grandpa: fix creation of justification with equivocating precommits in commit #11302

Merged
merged 5 commits into from
Jun 14, 2022
Merged
Changes from 1 commit
Commits
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
Next Next commit
grandpa: fix creation of justification ancestry
we would reject commits that have precommits targeting blocks lower than the
commit target. when there is an equivocation (or if it the commit is not
minimal) it is possible to have such precommits and we should assume that they
are the round base.
andresilva committed Apr 26, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 750cdad259b147e7a4f643a413f9cc7b3630e491
2 changes: 1 addition & 1 deletion client/finality-grandpa/src/finality_proof.rs
Original file line number Diff line number Diff line change
@@ -237,7 +237,7 @@ where
}

#[cfg(test)]
pub(crate) mod tests {
mod tests {
use super::*;
use crate::{authorities::AuthoritySetChanges, BlockNumberOps, ClientError, SetId};
use futures::executor::block_on;
43 changes: 39 additions & 4 deletions client/finality-grandpa/src/justification.rs
Original file line number Diff line number Diff line change
@@ -66,23 +66,41 @@ impl<Block: BlockT> GrandpaJustification<Block> {
Err(Error::Client(ClientError::BadJustification(msg)))
};

// we pick the precommit for the lowest block as the base that
// should serve as the root block for populating ancestry (i.e.
// collect all headers from all precommit blocks to the base)
let (base_hash, base_number) = match commit
.precommits
.iter()
.map(|signed| &signed.precommit)
.min_by_key(|precommit| precommit.target_number)
.map(|precommit| (precommit.target_hash.clone(), precommit.target_number))
{
None => return error(),
andresilva marked this conversation as resolved.
Show resolved Hide resolved
Some(base) => base,
};

for signed in commit.precommits.iter() {
let mut current_hash = signed.precommit.target_hash;
loop {
if current_hash == commit.target_hash {
if current_hash == base_hash {
break
}

match client.header(BlockId::Hash(current_hash))? {
Some(current_header) => {
if *current_header.number() <= commit.target_number {
// NOTE: this should never happen as we pick the lowest block
// as base and only traverse backwards from the other blocks
// in the commit. but better be safe to avoid an unbound loop.
if *current_header.number() <= base_number {
return error()
}

let parent_hash = *current_header.parent_hash();
if votes_ancestries_hashes.insert(current_hash) {
votes_ancestries.push(current_header);
}

current_hash = parent_hash;
},
_ => return error(),
@@ -149,6 +167,23 @@ impl<Block: BlockT> GrandpaJustification<Block> {
},
}

// we pick the precommit for the lowest block as the base that
// should serve as the root block for populating ancestry (i.e.
// collect all headers from all precommit blocks to the base)
let base_hash = self
.commit
.precommits
.iter()
.map(|signed| &signed.precommit)
.min_by_key(|precommit| precommit.target_number)
.map(|precommit| precommit.target_hash.clone())
.expect(
"can only fail if precommits is empty; \
commit has been validated above; \
valid commits must include precommits; \
qed.",
);

let mut buf = Vec::new();
let mut visited_hashes = HashSet::new();
for signed in self.commit.precommits.iter() {
@@ -165,11 +200,11 @@ impl<Block: BlockT> GrandpaJustification<Block> {
))
}

if self.commit.target_hash == signed.precommit.target_hash {
if base_hash == signed.precommit.target_hash {
continue
}

match ancestry_chain.ancestry(self.commit.target_hash, signed.precommit.target_hash) {
match ancestry_chain.ancestry(base_hash, signed.precommit.target_hash) {
Ok(route) => {
// ancestry starts from parent hash but the precommit target hash has been
// visited