diff --git a/bridges/primitives/header-chain/Cargo.toml b/bridges/primitives/header-chain/Cargo.toml index dc58dafb97..bf2268aeb4 100644 --- a/bridges/primitives/header-chain/Cargo.toml +++ b/bridges/primitives/header-chain/Cargo.toml @@ -20,6 +20,7 @@ sp-runtime = { git = "https://github.com/paritytech/substrate", branch = "master sp-std = { git = "https://github.com/paritytech/substrate", branch = "master" , default-features = false } [dev-dependencies] +assert_matches = "1.5" bp-test-utils = { path = "../test-utils" } [features] diff --git a/bridges/primitives/header-chain/src/justification.rs b/bridges/primitives/header-chain/src/justification.rs index e3bff85e50..625d776259 100644 --- a/bridges/primitives/header-chain/src/justification.rs +++ b/bridges/primitives/header-chain/src/justification.rs @@ -20,103 +20,13 @@ //! will ever be moved to the sp_finality_grandpa, we should reuse that implementation. use codec::{Decode, Encode}; -use finality_grandpa::{voter_set::VoterSet, Chain, Error as GrandpaError}; +use finality_grandpa::voter_set::VoterSet; use frame_support::RuntimeDebug; use sp_finality_grandpa::{AuthorityId, AuthoritySignature, SetId}; use sp_runtime::traits::Header as HeaderT; use sp_std::collections::{btree_map::BTreeMap, btree_set::BTreeSet}; use sp_std::prelude::*; -/// Justification verification error. -#[derive(RuntimeDebug, PartialEq)] -pub enum Error { - /// Failed to decode justification. - JustificationDecode, - /// Justification is finalizing unexpected header. - InvalidJustificationTarget, - /// Invalid commit in justification. - InvalidJustificationCommit, - /// Justification has invalid authority singature. - InvalidAuthoritySignature, - /// The justification has precommit for the header that has no route from the target header. - InvalidPrecommitAncestryProof, - /// The justification has 'unused' headers in its precommit ancestries. - InvalidPrecommitAncestries, -} - -/// Decode justification target. -pub fn decode_justification_target( - raw_justification: &[u8], -) -> Result<(Header::Hash, Header::Number), Error> { - GrandpaJustification::
::decode(&mut &*raw_justification) - .map(|justification| (justification.commit.target_hash, justification.commit.target_number)) - .map_err(|_| Error::JustificationDecode) -} - -/// Verify that justification, that is generated by given authority set, finalizes given header. -pub fn verify_justification( - finalized_target: (Header::Hash, Header::Number), - authorities_set_id: SetId, - authorities_set: &VoterSet, - justification: &GrandpaJustification
, -) -> Result<(), Error> -where - Header::Number: finality_grandpa::BlockNumberOps, -{ - // Ensure that it is justification for the expected header - if (justification.commit.target_hash, justification.commit.target_number) != finalized_target { - return Err(Error::InvalidJustificationTarget); - } - - // Validate commit of the justification. Note that `validate_commit()` assumes that all - // signatures are valid. We'll check the validity of the signatures later since they're more - // resource intensive to verify. - let ancestry_chain = AncestryChain::new(&justification.votes_ancestries); - match finality_grandpa::validate_commit(&justification.commit, authorities_set, &ancestry_chain) { - Ok(ref result) if result.ghost().is_some() => {} - _ => return Err(Error::InvalidJustificationCommit), - } - - // Now that we know that the commit is correct, check authorities signatures - let mut buf = Vec::new(); - let mut visited_hashes = BTreeSet::new(); - for signed in &justification.commit.precommits { - if !sp_finality_grandpa::check_message_signature_with_buffer( - &finality_grandpa::Message::Precommit(signed.precommit.clone()), - &signed.id, - &signed.signature, - justification.round, - authorities_set_id, - &mut buf, - ) { - return Err(Error::InvalidAuthoritySignature); - } - - if justification.commit.target_hash == signed.precommit.target_hash { - continue; - } - - match ancestry_chain.ancestry(justification.commit.target_hash, signed.precommit.target_hash) { - Ok(route) => { - // ancestry starts from parent hash but the precommit target hash has been visited - visited_hashes.insert(signed.precommit.target_hash); - visited_hashes.extend(route); - } - _ => { - // could this happen in practice? I don't think so, but original code has this check - return Err(Error::InvalidPrecommitAncestryProof); - } - } - } - - // both iterators are `BTree*` iterators, so have the same order => safe to compare - if !visited_hashes.iter().eq(ancestry_chain.ancestry.keys()) { - return Err(Error::InvalidPrecommitAncestries); - } - - Ok(()) -} - /// A GRANDPA Justification is a proof that a given header was finalized /// at a certain height and with a certain set of authorities. /// @@ -138,44 +48,172 @@ impl crate::FinalityProof for GrandpaJustification { } } -/// A utility trait implementing `finality_grandpa::Chain` using a given set of headers. -#[derive(RuntimeDebug)] -struct AncestryChain { - ancestry: BTreeMap, +/// Justification verification error. +#[derive(RuntimeDebug, PartialEq)] +pub enum Error { + /// Failed to decode justification. + JustificationDecode, + /// Justification is finalizing unexpected header. + InvalidJustificationTarget, + /// The authority has provided an invalid signature. + InvalidAuthoritySignature, + /// The justification contains precommit for header that is not a descendant of the commit header. + PrecommitIsNotCommitDescendant, + /// 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, } -impl AncestryChain
{ - fn new(ancestry: &[Header]) -> AncestryChain
{ - AncestryChain { - ancestry: ancestry - .iter() - .map(|header| (header.hash(), *header.parent_hash())) - .collect(), - } - } +/// Decode justification target. +pub fn decode_justification_target( + raw_justification: &[u8], +) -> Result<(Header::Hash, Header::Number), Error> { + GrandpaJustification::
::decode(&mut &*raw_justification) + .map(|justification| (justification.commit.target_hash, justification.commit.target_number)) + .map_err(|_| Error::JustificationDecode) } -impl finality_grandpa::Chain for AncestryChain
+/// Verify that justification, that is generated by given authority set, finalizes given header. +pub fn verify_justification( + finalized_target: (Header::Hash, Header::Number), + authorities_set_id: SetId, + authorities_set: &VoterSet, + justification: &GrandpaJustification
, +) -> Result<(), Error> where Header::Number: finality_grandpa::BlockNumberOps, { - fn ancestry(&self, base: Header::Hash, block: Header::Hash) -> Result, GrandpaError> { - let mut route = Vec::new(); - let mut current_hash = block; - loop { - if current_hash == base { - break; - } - match self.ancestry.get(¤t_hash).cloned() { - Some(parent_hash) => { - current_hash = parent_hash; - route.push(current_hash); - } - _ => return Err(GrandpaError::NotDescendent), - } - } - route.pop(); // remove the base + // ensure that it is justification for the expected header + if (justification.commit.target_hash, justification.commit.target_number) != finalized_target { + return Err(Error::InvalidJustificationTarget); + } - Ok(route) + let mut chain = AncestryChain::new(&justification.votes_ancestries); + let mut signature_buffer = Vec::new(); + let mut votes = BTreeSet::new(); + let mut cumulative_weight = 0u64; + for signed in &justification.commit.precommits { + // authority must be in the set + let authority_info = match authorities_set.get(&signed.id) { + Some(authority_info) => authority_info, + None => { + // just ignore precommit from unknown authority as `finality_grandpa::import_precommit` does + continue; + } + }; + + // check if authority has already voted in the same round. + // + // there's a lot of code in `validate_commit` and `import_precommit` functions inside + // `finality-grandpa` crate (mostly related to reporing 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()) { + continue; + } + + // everything below this line can't just `continue`, because state is already altered + + // all precommits must be for block higher than the target + if signed.precommit.target_number < justification.commit.target_number { + return Err(Error::PrecommitIsNotCommitDescendant); + } + // all precommits must be for target block descendents + 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", + ); + // verify authority signature + if !sp_finality_grandpa::check_message_signature_with_buffer( + &finality_grandpa::Message::Precommit(signed.precommit.clone()), + &signed.id, + &signed.signature, + justification.round, + authorities_set_id, + &mut signature_buffer, + ) { + return Err(Error::InvalidAuthoritySignature); + } + } + + // 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 voted for the justification target (or one + // of its descendents) is larger than required threshold. + let threshold = authorities_set.threshold().0.into(); + if cumulative_weight >= threshold { + Ok(()) + } else { + Err(Error::TooLowCumulativeWeight) + } +} + +/// Votes ancestries with useful methods. +#[derive(RuntimeDebug)] +pub struct AncestryChain { + /// Header hash => parent header hash mapping. + pub parents: BTreeMap, + /// Hashes of headers that weren't visited by `is_ancestor` method. + pub unvisited: BTreeSet, +} + +impl AncestryChain
{ + /// Create new ancestry chain. + pub fn new(ancestry: &[Header]) -> AncestryChain
{ + let mut parents = BTreeMap::new(); + let mut unvisited = BTreeSet::new(); + for ancestor in ancestry { + let hash = ancestor.hash(); + let parent_hash = *ancestor.parent_hash(); + parents.insert(hash, parent_hash); + unvisited.insert(hash); + } + AncestryChain { parents, unvisited } + } + + /// Returns `Err(_)` if `precommit_target` is a descendant of the `commit_target` block and `Ok(_)` otherwise. + pub fn ensure_descendant( + mut self, + commit_target: &Header::Hash, + precommit_target: &Header::Hash, + ) -> Result { + let mut current_hash = *precommit_target; + loop { + if current_hash == *commit_target { + break; + } + + let is_visited_before = !self.unvisited.remove(¤t_hash); + current_hash = match self.parents.get(¤t_hash) { + Some(parent_hash) => { + 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); + } + + *parent_hash + } + None => return Err(Error::PrecommitIsNotCommitDescendant), + }; + } + Ok(self) } } diff --git a/bridges/primitives/header-chain/tests/implementation_match.rs b/bridges/primitives/header-chain/tests/implementation_match.rs new file mode 100644 index 0000000000..0b55c19035 --- /dev/null +++ b/bridges/primitives/header-chain/tests/implementation_match.rs @@ -0,0 +1,317 @@ +// Copyright 2020-2021 Parity Technologies (UK) Ltd. +// This file is part of Parity Bridges Common. + +// Parity Bridges Common is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Parity Bridges Common is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Parity Bridges Common. If not, see . + +//! Tests inside this module are made to ensure that our custom justification verification +//! implementation works exactly as `fn finality_grandpa::validate_commit`. +//! +//! Some of tests in this module may partially duplicate tests from `justification.rs`, +//! but their purpose is different. + +use assert_matches::assert_matches; +use bp_header_chain::justification::{verify_justification, Error, GrandpaJustification}; +use bp_test_utils::{ + header_id, make_justification_for_header, signed_precommit, test_header, Account, JustificationGeneratorParams, + ALICE, BOB, CHARLIE, DAVE, EVE, TEST_GRANDPA_SET_ID, +}; +use finality_grandpa::voter_set::VoterSet; +use sp_finality_grandpa::{AuthorityId, AuthorityWeight}; +use sp_runtime::traits::Header as HeaderT; + +type TestHeader = sp_runtime::testing::Header; +type TestHash = ::Hash; +type TestNumber = ::Number; + +/// Implementation of `finality_grandpa::Chain` that is used in tests. +struct AncestryChain(bp_header_chain::justification::AncestryChain); + +impl AncestryChain { + fn new(ancestry: &[TestHeader]) -> Self { + Self(bp_header_chain::justification::AncestryChain::new(ancestry)) + } +} + +impl finality_grandpa::Chain for AncestryChain { + fn ancestry(&self, base: TestHash, block: TestHash) -> Result, finality_grandpa::Error> { + let mut route = Vec::new(); + let mut current_hash = block; + loop { + if current_hash == base { + break; + } + match self.0.parents.get(¤t_hash).cloned() { + Some(parent_hash) => { + current_hash = parent_hash; + route.push(current_hash); + } + _ => return Err(finality_grandpa::Error::NotDescendent), + } + } + route.pop(); // remove the base + + Ok(route) + } +} + +/// Get a full set of accounts. +fn full_accounts_set() -> Vec<(Account, AuthorityWeight)> { + vec![(ALICE, 1), (BOB, 1), (CHARLIE, 1), (DAVE, 1), (EVE, 1)] +} + +/// Get a full set of GRANDPA authorities. +fn full_voter_set() -> VoterSet { + VoterSet::new(full_accounts_set().iter().map(|(id, w)| (AuthorityId::from(*id), *w))).unwrap() +} + +/// Get a minimal set of accounts. +fn minimal_accounts_set() -> Vec<(Account, AuthorityWeight)> { + // there are 5 accounts in the full set => we need 2/3 + 1 accounts, which results in 4 accounts + vec![(ALICE, 1), (BOB, 1), (CHARLIE, 1), (DAVE, 1)] +} + +/// Get a minimal subset of GRANDPA authorities that have enough cumulative vote weight to justify a header finality. +pub fn minimal_voter_set() -> VoterSet { + VoterSet::new( + minimal_accounts_set() + .iter() + .map(|(id, w)| (AuthorityId::from(*id), *w)), + ) + .unwrap() +} + +/// Make a valid GRANDPA justification with sensible defaults. +pub fn make_default_justification(header: &TestHeader) -> GrandpaJustification { + make_justification_for_header(JustificationGeneratorParams { + header: header.clone(), + authorities: minimal_accounts_set(), + ..Default::default() + }) +} + +// the `finality_grandpa::validate_commit` function has two ways to report an unsuccessful +// commit validation: +// +// 1) to return `Err()` (which only may happen if `finality_grandpa::Chain` implementation +// returns an error); +// 2) to return `Ok(validation_result) if validation_result.ghost().is_none()`. +// +// Our implementation would just return error in both cases. + +#[test] +fn same_result_when_precommit_target_has_lower_number_than_commit_target() { + let mut justification = make_default_justification(&test_header(1)); + // the number of header in precommit (0) is lower than number of header in commit (1) + justification.commit.precommits[0].precommit.target_number = 0; + + // our implementation returns an error + assert_eq!( + verify_justification::( + header_id::(1), + TEST_GRANDPA_SET_ID, + &full_voter_set(), + &justification, + ), + Err(Error::PrecommitIsNotCommitDescendant), + ); + // original implementation returns empty GHOST + assert_matches!( + finality_grandpa::validate_commit( + &justification.commit, + &full_voter_set(), + &AncestryChain::new(&justification.votes_ancestries), + ) + .map(|result| result.ghost().cloned()), + Ok(None) + ); +} + +#[test] +fn same_result_when_precommit_target_is_not_descendant_of_commit_target() { + let not_descendant = test_header::(10); + let mut justification = make_default_justification(&test_header(1)); + // the route from header of commit (1) to header of precommit (10) is missing from + // the votes ancestries + justification.commit.precommits[0].precommit.target_number = *not_descendant.number(); + justification.commit.precommits[0].precommit.target_hash = not_descendant.hash(); + justification.votes_ancestries.push(not_descendant); + + // our implementation returns an error + assert_eq!( + verify_justification::( + header_id::(1), + TEST_GRANDPA_SET_ID, + &full_voter_set(), + &justification, + ), + Err(Error::PrecommitIsNotCommitDescendant), + ); + // original implementation returns empty GHOST + assert_matches!( + finality_grandpa::validate_commit( + &justification.commit, + &full_voter_set(), + &AncestryChain::new(&justification.votes_ancestries), + ) + .map(|result| result.ghost().cloned()), + Ok(None) + ); +} + +#[test] +fn same_result_when_justification_contains_duplicate_vote() { + let mut justification = make_default_justification(&test_header(1)); + // the justification may contain exactly the same vote (i.e. same precommit and same signature) + // multiple times && it isn't treated as an error by original implementation + justification + .commit + .precommits + .push(justification.commit.precommits[0].clone()); + justification + .commit + .precommits + .push(justification.commit.precommits[0].clone()); + + // our implementation succeeds + assert_eq!( + verify_justification::( + header_id::(1), + TEST_GRANDPA_SET_ID, + &full_voter_set(), + &justification, + ), + Ok(()), + ); + // original implementation returns non-empty GHOST + assert_matches!( + finality_grandpa::validate_commit( + &justification.commit, + &full_voter_set(), + &AncestryChain::new(&justification.votes_ancestries), + ) + .map(|result| result.ghost().cloned()), + Ok(Some(_)) + ); +} + +#[test] +fn same_result_when_authority_equivocates_once_in_a_round() { + let mut justification = make_default_justification(&test_header(1)); + // the justification original implementation allows authority to submit two different + // votes in a single round, of which only first is 'accepted' + justification.commit.precommits.push(signed_precommit::( + &ALICE, + header_id::(1), + justification.round, + TEST_GRANDPA_SET_ID, + )); + + // our implementation succeeds + assert_eq!( + verify_justification::( + header_id::(1), + TEST_GRANDPA_SET_ID, + &full_voter_set(), + &justification, + ), + Ok(()), + ); + // original implementation returns non-empty GHOST + assert_matches!( + finality_grandpa::validate_commit( + &justification.commit, + &full_voter_set(), + &AncestryChain::new(&justification.votes_ancestries), + ) + .map(|result| result.ghost().cloned()), + Ok(Some(_)) + ); +} + +#[test] +fn same_result_when_authority_equivocates_twice_in_a_round() { + let mut justification = make_default_justification(&test_header(1)); + // there's some code in the original implementation that should return an error when + // same authority submits more than two different votes in a single round: + // https://github.com/paritytech/finality-grandpa/blob/6aeea2d1159d0f418f0b86e70739f2130629ca09/src/lib.rs#L473 + // but there's also a code that prevents this from happening: + // https://github.com/paritytech/finality-grandpa/blob/6aeea2d1159d0f418f0b86e70739f2130629ca09/src/round.rs#L287 + // => so now we are also just ignoring all votes from the same authority, except the first one + justification.commit.precommits.push(signed_precommit::( + &ALICE, + header_id::(1), + justification.round, + TEST_GRANDPA_SET_ID, + )); + justification.commit.precommits.push(signed_precommit::( + &ALICE, + header_id::(1), + justification.round, + TEST_GRANDPA_SET_ID, + )); + + // our implementation succeeds + assert_eq!( + verify_justification::( + header_id::(1), + TEST_GRANDPA_SET_ID, + &full_voter_set(), + &justification, + ), + Ok(()), + ); + // original implementation returns non-empty GHOST + assert_matches!( + finality_grandpa::validate_commit( + &justification.commit, + &full_voter_set(), + &AncestryChain::new(&justification.votes_ancestries), + ) + .map(|result| result.ghost().cloned()), + Ok(Some(_)) + ); +} + +#[test] +fn same_result_when_there_are_not_enough_cumulative_weight_to_finalize_commit_target() { + // just remove one authority from the minimal set and we shall not reach the threshold + let mut authorities_set = minimal_accounts_set(); + authorities_set.pop(); + let justification = make_justification_for_header(JustificationGeneratorParams { + header: test_header(1), + authorities: authorities_set, + ..Default::default() + }); + + // our implementation returns an error + assert_eq!( + verify_justification::( + header_id::(1), + TEST_GRANDPA_SET_ID, + &full_voter_set(), + &justification, + ), + Err(Error::TooLowCumulativeWeight), + ); + // original implementation returns empty GHOST + assert_matches!( + finality_grandpa::validate_commit( + &justification.commit, + &full_voter_set(), + &AncestryChain::new(&justification.votes_ancestries), + ) + .map(|result| result.ghost().cloned()), + Ok(None) + ); +} diff --git a/bridges/primitives/header-chain/tests/justification.rs b/bridges/primitives/header-chain/tests/justification.rs index 1ce739e453..ab8d9c4905 100644 --- a/bridges/primitives/header-chain/tests/justification.rs +++ b/bridges/primitives/header-chain/tests/justification.rs @@ -23,7 +23,7 @@ type TestHeader = sp_runtime::testing::Header; #[test] fn valid_justification_accepted() { - let authorities = vec![(ALICE, 1), (BOB, 1), (CHARLIE, 1), (DAVE, 1), (EVE, 1)]; + let authorities = vec![(ALICE, 1), (BOB, 1), (CHARLIE, 1), (DAVE, 1)]; let params = JustificationGeneratorParams { header: test_header(1), round: TEST_GRANDPA_ROUND, @@ -129,7 +129,7 @@ fn justification_with_invalid_commit_rejected() { &voter_set(), &justification, ), - Err(Error::InvalidJustificationCommit), + Err(Error::ExtraHeadersInVotesAncestries), ); } @@ -161,7 +161,7 @@ fn justification_with_invalid_precommit_ancestry() { &voter_set(), &justification, ), - Err(Error::InvalidPrecommitAncestries), + Err(Error::ExtraHeadersInVotesAncestries), ); } @@ -186,6 +186,6 @@ fn justification_is_invalid_if_we_dont_meet_threshold() { &voter_set(), &make_justification_for_header::(params) ), - Err(Error::InvalidJustificationCommit), + Err(Error::TooLowCumulativeWeight), ); } diff --git a/bridges/primitives/test-utils/src/lib.rs b/bridges/primitives/test-utils/src/lib.rs index 0fcc263763..0eff3fe585 100644 --- a/bridges/primitives/test-utils/src/lib.rs +++ b/bridges/primitives/test-utils/src/lib.rs @@ -177,7 +177,8 @@ fn generate_chain(fork_id: u8, depth: u32, ancestor: &H) -> Vec { headers } -fn signed_precommit( +/// Create signed precommit with given target. +pub fn signed_precommit( signer: &Account, target: (H::Hash, H::Number), round: u64,