From 1c7b5d1b30a678e928e1c991bc66881ab0936a98 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 22 Mar 2021 15:32:48 -0400 Subject: [PATCH] Tunable Justification Generator (#835) * Add way to create tunable GRANDPA justifications * Use new function in tests * Allow multiple authorities on a single fork * Only store pre-commit targets instead of full ancestry chains * Rename precommit_header to be more generic * Push new digest item instead of overriding entire digest * Ensure that we generate chains with non-zero length * Extract justification creation parameters into struct * Appease Clippy --- bridges/modules/finality-verifier/src/lib.rs | 27 ++-- bridges/modules/substrate/src/fork_tests.rs | 6 +- bridges/modules/substrate/src/verifier.rs | 14 +- .../header-chain/tests/justification.rs | 71 ++++++++-- bridges/primitives/test-utils/src/lib.rs | 123 +++++++++++++++--- 5 files changed, 184 insertions(+), 57 deletions(-) diff --git a/bridges/modules/finality-verifier/src/lib.rs b/bridges/modules/finality-verifier/src/lib.rs index 8c77745025..4399d78a74 100644 --- a/bridges/modules/finality-verifier/src/lib.rs +++ b/bridges/modules/finality-verifier/src/lib.rs @@ -522,7 +522,7 @@ mod tests { use super::*; use crate::mock::{run_test, test_header, Origin, TestHash, TestHeader, TestNumber, TestRuntime}; use bp_test_utils::{ - authority_list, keyring, make_justification_for_header, + authority_list, make_default_justification, make_justification_for_header, JustificationGeneratorParams, Keyring::{Alice, Bob}, }; use codec::Encode; @@ -551,10 +551,7 @@ mod tests { fn submit_finality_proof(header: u8) -> frame_support::dispatch::DispatchResultWithPostInfo { let header = test_header(header.into()); - - let set_id = 1; - let grandpa_round = 1; - let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode(); + let justification = make_default_justification(&header).encode(); Module::::submit_finality_proof(Origin::signed(1), header, justification) } @@ -726,9 +723,11 @@ mod tests { let header = test_header(1); - let set_id = 2; - let grandpa_round = 1; - let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode(); + let params = JustificationGeneratorParams:: { + set_id: 2, + ..Default::default() + }; + let justification = make_justification_for_header(params).encode(); assert_err!( Module::::submit_finality_proof(Origin::signed(1), header, justification,), @@ -802,9 +801,7 @@ mod tests { header.digest = change_log(0); // Create a valid justification for the header - let set_id = 1; - let grandpa_round = 1; - let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode(); + let justification = make_default_justification(&header).encode(); // Let's import our test header assert_ok!(Module::::submit_finality_proof( @@ -836,9 +833,7 @@ mod tests { header.digest = change_log(1); // Create a valid justification for the header - let set_id = 1; - let grandpa_round = 1; - let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode(); + let justification = make_default_justification(&header).encode(); // Should not be allowed to import this header assert_err!( @@ -859,9 +854,7 @@ mod tests { header.digest = forced_change_log(0); // Create a valid justification for the header - let set_id = 1; - let grandpa_round = 1; - let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode(); + let justification = make_default_justification(&header).encode(); // Should not be allowed to import this header assert_err!( diff --git a/bridges/modules/substrate/src/fork_tests.rs b/bridges/modules/substrate/src/fork_tests.rs index 9dc145b641..acd147109a 100644 --- a/bridges/modules/substrate/src/fork_tests.rs +++ b/bridges/modules/substrate/src/fork_tests.rs @@ -60,7 +60,7 @@ use crate::verifier::*; use crate::{BestFinalized, BestHeight, BridgeStorage, NextScheduledChange, PalletStorage}; use bp_header_chain::AuthoritySet; use bp_test_utils::{ - authority_list, keyring, make_justification_for_header, + authority_list, make_default_justification, Keyring::{Alice, Bob}, }; use codec::Encode; @@ -457,9 +457,7 @@ where // `grandpa_round`). // // See for more: https://github.com/paritytech/parity-bridges-common/issues/430 - let grandpa_round = 1; - let set_id = 1; - let justification = make_justification_for_header(header, grandpa_round, set_id, &keyring()).encode(); + let justification = make_default_justification(header).encode(); let res = verifier .import_finality_proof(header.hash(), justification.into()) diff --git a/bridges/modules/substrate/src/verifier.rs b/bridges/modules/substrate/src/verifier.rs index 47647aafc5..4849941124 100644 --- a/bridges/modules/substrate/src/verifier.rs +++ b/bridges/modules/substrate/src/verifier.rs @@ -343,7 +343,7 @@ mod tests { use crate::mock::*; use crate::{BestFinalized, BestHeight, HeaderId, ImportedHeaders, PalletStorage}; use bp_test_utils::{ - authority_list, keyring, make_justification_for_header, + authority_list, make_default_justification, Keyring::{Alice, Bob}, }; use codec::Encode; @@ -605,8 +605,7 @@ mod tests { assert_eq!(storage.best_headers().len(), 1); // Now lets finalize our best header - let grandpa_round = 1; - let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode(); + let justification = make_default_justification(&header).encode(); assert_ok!(verifier.import_finality_proof(header.hash(), justification.into())); // Our best header should only appear once in the list of best headers @@ -729,8 +728,7 @@ mod tests { storage.update_current_authority_set(authority_set); // We'll need this justification to finalize the header - let grandpa_round = 1; - let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode(); + let justification = make_default_justification(&header).encode(); let mut verifier = Verifier { storage: storage.clone(), @@ -754,8 +752,7 @@ mod tests { let authority_set = AuthoritySet { authorities, set_id }; storage.update_current_authority_set(authority_set); - let grandpa_round = 1; - let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode(); + let justification = make_default_justification(&header).encode(); let mut verifier = Verifier { storage: storage.clone(), @@ -798,8 +795,7 @@ mod tests { // This header enacts an authority set change upon finalization let header = test_header(2); - let grandpa_round = 1; - let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode(); + let justification = make_default_justification(&header).encode(); // Schedule a change at the height of our header let set_id = 2; diff --git a/bridges/primitives/header-chain/tests/justification.rs b/bridges/primitives/header-chain/tests/justification.rs index 74f3a39f26..29ba744133 100644 --- a/bridges/primitives/header-chain/tests/justification.rs +++ b/bridges/primitives/header-chain/tests/justification.rs @@ -16,14 +16,55 @@ //! Tests for Grandpa Justification code. -use bp_header_chain::justification::{verify_justification, Error, GrandpaJustification}; +use bp_header_chain::justification::{verify_justification, Error}; +use bp_test_utils::Keyring::*; use bp_test_utils::*; use codec::Encode; type TestHeader = sp_runtime::testing::Header; -fn make_justification_for_header_1() -> GrandpaJustification { - make_justification_for_header(&test_header(1), TEST_GRANDPA_ROUND, TEST_GRANDPA_SET_ID, &keyring()) +#[test] +fn valid_justification_accepted() { + let params = JustificationGeneratorParams { + header: test_header(1), + round: TEST_GRANDPA_ROUND, + set_id: TEST_GRANDPA_SET_ID, + authorities: vec![(Alice, 1), (Bob, 1), (Charlie, 1), (Dave, 1), (Eve, 1)], + depth: 5, + forks: 5, + }; + + assert_eq!( + verify_justification::( + header_id::(1), + TEST_GRANDPA_SET_ID, + &voter_set(), + &make_justification_for_header::(params).encode() + ), + Ok(()), + ); +} + +#[test] +fn valid_justification_accepted_with_single_fork() { + let params = JustificationGeneratorParams { + header: test_header(1), + round: TEST_GRANDPA_ROUND, + set_id: TEST_GRANDPA_SET_ID, + authorities: vec![(Alice, 1), (Bob, 1), (Charlie, 1), (Dave, 1), (Eve, 1)], + depth: 5, + forks: 1, + }; + + assert_eq!( + verify_justification::( + header_id::(1), + TEST_GRANDPA_SET_ID, + &voter_set(), + &make_justification_for_header::(params).encode() + ), + Ok(()), + ); } #[test] @@ -41,7 +82,7 @@ fn justification_with_invalid_target_rejected() { header_id::(2), TEST_GRANDPA_SET_ID, &voter_set(), - &make_justification_for_header_1().encode(), + &make_default_justification::(&test_header(1)).encode(), ), Err(Error::InvalidJustificationTarget), ); @@ -49,7 +90,7 @@ fn justification_with_invalid_target_rejected() { #[test] fn justification_with_invalid_commit_rejected() { - let mut justification = make_justification_for_header_1(); + let mut justification = make_default_justification::(&test_header(1)); justification.commit.precommits.clear(); assert_eq!( @@ -65,7 +106,7 @@ fn justification_with_invalid_commit_rejected() { #[test] fn justification_with_invalid_authority_signature_rejected() { - let mut justification = make_justification_for_header_1(); + let mut justification = make_default_justification::(&test_header(1)); justification.commit.precommits[0].signature = Default::default(); assert_eq!( @@ -81,7 +122,7 @@ fn justification_with_invalid_authority_signature_rejected() { #[test] fn justification_with_invalid_precommit_ancestry() { - let mut justification = make_justification_for_header_1(); + let mut justification = make_default_justification::(&test_header(1)); justification.votes_ancestries.push(test_header(10)); assert_eq!( @@ -96,14 +137,24 @@ fn justification_with_invalid_precommit_ancestry() { } #[test] -fn valid_justification_accepted() { +fn justification_is_invalid_if_we_dont_meet_threshold() { + // Need at least three authorities to sign off or else the voter set threshold can't be reached + let params = JustificationGeneratorParams { + header: test_header(1), + round: TEST_GRANDPA_ROUND, + set_id: TEST_GRANDPA_SET_ID, + authorities: vec![(Alice, 1), (Bob, 1)], + depth: 2, + forks: 2, + }; + assert_eq!( verify_justification::( header_id::(1), TEST_GRANDPA_SET_ID, &voter_set(), - &make_justification_for_header_1().encode(), + &make_justification_for_header::(params).encode() ), - Ok(()), + Err(Error::InvalidJustificationCommit), ); } diff --git a/bridges/primitives/test-utils/src/lib.rs b/bridges/primitives/test-utils/src/lib.rs index 499d1a5a21..31dabde5ea 100644 --- a/bridges/primitives/test-utils/src/lib.rs +++ b/bridges/primitives/test-utils/src/lib.rs @@ -31,29 +31,95 @@ use sp_std::prelude::*; pub const TEST_GRANDPA_ROUND: u64 = 1; pub const TEST_GRANDPA_SET_ID: SetId = 1; -/// Get a valid Grandpa justification for a header given a Grandpa round, authority set ID, and -/// authority list. -pub fn make_justification_for_header( - header: &H, - round: u64, - set_id: SetId, - authorities: &[(Keyring, AuthorityWeight)], -) -> GrandpaJustification { +/// Configuration parameters when generating test GRANDPA justifications. +pub struct JustificationGeneratorParams { + /// The header which we want to finalize. + pub header: H, + /// The GRANDPA round number for the current authority set. + pub round: u64, + /// The current authority set ID. + pub set_id: SetId, + /// The current GRANDPA authority set. + pub authorities: Vec<(Keyring, AuthorityWeight)>, + /// The number of headers included in our justification's vote ancestries. + pub depth: u32, + /// The number of forks, and thus the number of pre-commits in our justification. + pub forks: u32, +} + +impl Default for JustificationGeneratorParams { + fn default() -> Self { + Self { + header: test_header(One::one()), + round: TEST_GRANDPA_ROUND, + set_id: TEST_GRANDPA_SET_ID, + authorities: keyring(), + depth: 2, + forks: 1, + } + } +} + +/// Make a valid GRANDPA justification with sensible defaults +pub fn make_default_justification(header: &H) -> GrandpaJustification { + let params = JustificationGeneratorParams:: { + header: header.clone(), + ..Default::default() + }; + + make_justification_for_header(params) +} + +/// Generate justifications in a way where we are able to tune the number of pre-commits +/// and vote ancestries which are included in the justification. +/// +/// This is useful for benchmarkings where we want to generate valid justifications with +/// a specific number of pre-commits (tuned with the "forks" parameter) and/or a specific +/// number of vote ancestries (tuned with the "depth" parameter). +/// +/// Note: This needs at least three authorities or else the verifier will complain about +/// being given an invalid commit. +pub fn make_justification_for_header(params: JustificationGeneratorParams) -> GrandpaJustification { + let JustificationGeneratorParams { + header, + round, + set_id, + authorities, + depth, + forks, + } = params; + let (target_hash, target_number) = (header.hash(), *header.number()); let mut precommits = vec![]; let mut votes_ancestries = vec![]; - // We want to make sure that the header included in the vote ancestries - // is actually related to our target header - let mut precommit_header = test_header::(target_number + One::one()); - precommit_header.set_parent_hash(target_hash); + assert!(depth != 0, "Can't have a chain of zero length."); + assert!( + forks as usize <= authorities.len(), + "If we have more forks than authorities we can't create valid pre-commits for all the forks." + ); + + let mut unsigned_precommits = vec![]; + for i in 0..forks { + let chain = generate_chain(i as u8, depth, &header); + + // We don't include our finality target header in the vote ancestries + for child in &chain[1..] { + votes_ancestries.push(child.clone()); + } + + // The header we need to use when pre-commiting is the one at the highest height + // on our chain. + let precommit_candidate = chain.last().map(|h| (h.hash(), *h.number())).unwrap(); + unsigned_precommits.push(precommit_candidate); + } + + for (i, (id, _weight)) in authorities.iter().enumerate() { + // Assign authorities to sign pre-commits in a round-robin fashion + let target = unsigned_precommits[i % forks as usize]; + let precommit = signed_precommit::(&id, target, round, set_id); - // I'm using the same header for all the voters since it doesn't matter as long - // as they all vote on blocks _ahead_ of the one we're interested in finalizing - for (id, _weight) in authorities.iter() { - let precommit = signed_precommit::(id, (precommit_header.hash(), *precommit_header.number()), round, set_id); precommits.push(precommit); - votes_ancestries.push(precommit_header.clone()); } GrandpaJustification { @@ -67,6 +133,29 @@ pub fn make_justification_for_header( } } +fn generate_chain(fork_id: u8, depth: u32, ancestor: &H) -> Vec { + let mut headers = vec![ancestor.clone()]; + + for i in 1..depth { + let parent = &headers[(i - 1) as usize]; + let (hash, num) = (parent.hash(), *parent.number()); + + let mut header = test_header::(num + One::one()); + header.set_parent_hash(hash); + + // Modifying the digest so headers at the same height but in different forks have different + // hashes + header + .digest_mut() + .logs + .push(sp_runtime::DigestItem::Other(vec![fork_id])); + + headers.push(header); + } + + headers +} + fn signed_precommit( signer: &Keyring, target: (H::Hash, H::Number),