diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index cdd8b5a87f..1a60d16d7d 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -311,11 +311,6 @@ parameter_types! { // Note that once this is hit the pallet will essentially throttle incoming requests down to one // call per block. pub const MaxRequests: u32 = 50; - pub const WestendSessionLength: bp_westend::BlockNumber = bp_westend::SESSION_LENGTH; - pub const RialtoSessionLength: bp_rialto::BlockNumber = bp_rialto::SESSION_LENGTH; - - // TODO [#846]: Right now this will break benchmarking if it is greater than `u8::MAX` - pub const RialtoValidatorCount: u32 = 255; pub const WestendValidatorCount: u32 = 255; } @@ -323,8 +318,6 @@ pub type RialtoGrandpaInstance = (); impl pallet_bridge_grandpa::Config for Runtime { type BridgedChain = bp_rialto::Rialto; type MaxRequests = MaxRequests; - type MaxBridgedSessionLength = RialtoSessionLength; - type MaxBridgedValidatorCount = RialtoValidatorCount; // TODO [#391]: Use weights generated for the Millau runtime instead of Rialto ones. type WeightInfo = pallet_bridge_grandpa::weights::RialtoWeight; @@ -334,8 +327,6 @@ pub type WestendGrandpaInstance = pallet_bridge_grandpa::Instance1; impl pallet_bridge_grandpa::Config for Runtime { type BridgedChain = bp_westend::Westend; type MaxRequests = MaxRequests; - type MaxBridgedSessionLength = WestendSessionLength; - type MaxBridgedValidatorCount = WestendValidatorCount; // TODO [#391]: Use weights generated for the Millau runtime instead of Rialto ones. type WeightInfo = pallet_bridge_grandpa::weights::RialtoWeight; diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs index a0ccbf27e6..dae683aa82 100644 --- a/bridges/bin/rialto/runtime/src/lib.rs +++ b/bridges/bin/rialto/runtime/src/lib.rs @@ -417,17 +417,11 @@ parameter_types! { // Note that once this is hit the pallet will essentially throttle incoming requests down to one // call per block. pub const MaxRequests: u32 = 50; - pub const MillauSessionLength: bp_millau::BlockNumber = bp_millau::SESSION_LENGTH; - - // TODO [#846]: Right now this will break benchmarking if it is greater than `u8::MAX` - pub const MillauValidatorCount: u32 = 255; } impl pallet_bridge_grandpa::Config for Runtime { type BridgedChain = bp_millau::Millau; type MaxRequests = MaxRequests; - type MaxBridgedSessionLength = MillauSessionLength; - type MaxBridgedValidatorCount = MillauValidatorCount; type WeightInfo = pallet_bridge_grandpa::weights::RialtoWeight; } diff --git a/bridges/modules/grandpa/src/benchmarking.rs b/bridges/modules/grandpa/src/benchmarking.rs index 089e7f88e7..a3abe508d8 100644 --- a/bridges/modules/grandpa/src/benchmarking.rs +++ b/bridges/modules/grandpa/src/benchmarking.rs @@ -51,13 +51,23 @@ use bp_test_utils::{ TEST_GRANDPA_ROUND, TEST_GRANDPA_SET_ID, }; use frame_benchmarking::{benchmarks_instance_pallet, whitelisted_caller}; -use frame_support::traits::Get; use frame_system::RawOrigin; -use num_traits::cast::AsPrimitive; use sp_finality_grandpa::AuthorityId; use sp_runtime::traits::{One, Zero}; use sp_std::{vec, vec::Vec}; +// The maximum number of vote ancestries to include in a justification. +// +// In practice this would be limited by the session length (number of blocks a single authority set +// can produce) of a given chain. +const MAX_VOTE_ANCESTRIES: u32 = 1000; + +// The maximum number of pre-commits to include in a justification. In practice this scales with the +// number of validators. +// +// TODO [#846]: Right now this will break benchmarking if it is greater than `u8::MAX` +const MAX_VALIDATOR_SET_SIZE: u32 = 255; + benchmarks_instance_pallet! { // This is the "gold standard" benchmark for this extrinsic, and it's what should be used to // annotate the weight in the pallet. @@ -65,8 +75,8 @@ benchmarks_instance_pallet! { // The other benchmarks related to `submit_finality_proof` are looking at the effect of specific // parameters and are there mostly for seeing how specific codepaths behave. submit_finality_proof { - let s in 1..T::MaxBridgedSessionLength::get().as_() as u32; - let p in 1..T::MaxBridgedValidatorCount::get(); + let v in 1..MAX_VOTE_ANCESTRIES; + let p in 1..MAX_VALIDATOR_SET_SIZE; let caller: T::AccountId = whitelisted_caller(); @@ -90,8 +100,8 @@ benchmarks_instance_pallet! { round: TEST_GRANDPA_ROUND, set_id: TEST_GRANDPA_SET_ID, authorities: accounts(p as u8).iter().map(|k| (*k, 1)).collect::>(), - depth: s, - forks: p, + votes: v, + forks: 1, }; let justification = make_justification_for_header(params); @@ -108,7 +118,7 @@ benchmarks_instance_pallet! { // What we want to check here is the effect of vote ancestries on justification verification // do this by varying the number of headers between `finality_target` and `header_of_chain`. submit_finality_proof_on_single_fork { - let s in 1..T::MaxBridgedSessionLength::get().as_() as u32; + let v in 1..MAX_VOTE_ANCESTRIES; let caller: T::AccountId = whitelisted_caller(); @@ -127,7 +137,7 @@ benchmarks_instance_pallet! { round: TEST_GRANDPA_ROUND, set_id: TEST_GRANDPA_SET_ID, authorities: test_keyring(), - depth: s, + votes: v, forks: 1, }; @@ -146,7 +156,7 @@ benchmarks_instance_pallet! { // We do this by creating many forks, whose head will be used as a signed pre-commit in the // final justification. submit_finality_proof_on_many_forks { - let p in 1..T::MaxBridgedValidatorCount::get(); + let p in 1..MAX_VALIDATOR_SET_SIZE; let caller: T::AccountId = whitelisted_caller(); @@ -170,7 +180,7 @@ benchmarks_instance_pallet! { round: TEST_GRANDPA_ROUND, set_id: TEST_GRANDPA_SET_ID, authorities: accounts(p as u8).iter().map(|k| (*k, 1)).collect::>(), - depth: 2, + votes: p, forks: p, }; diff --git a/bridges/modules/grandpa/src/lib.rs b/bridges/modules/grandpa/src/lib.rs index 2721507d47..9f7ca8231c 100644 --- a/bridges/modules/grandpa/src/lib.rs +++ b/bridges/modules/grandpa/src/lib.rs @@ -44,7 +44,6 @@ use codec::{Decode, Encode}; use finality_grandpa::voter_set::VoterSet; use frame_support::ensure; use frame_system::{ensure_signed, RawOrigin}; -use num_traits::cast::AsPrimitive; #[cfg(feature = "std")] use serde::{Deserialize, Serialize}; use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID}; @@ -92,27 +91,6 @@ pub mod pallet { #[pallet::constant] type MaxRequests: Get; - /// The maximum length of a session on the bridged chain. - /// - /// The pallet uses this to bound justification verification since justifications contain - /// ancestry proofs whose size is capped at `MaxBridgedSessionLength`. - #[pallet::constant] - type MaxBridgedSessionLength: Get>; - - /// The number of validators on the bridged chain. - /// - /// The pallet uses this to bound justification verification since justifications may - /// contain up to `MaxBridgedValidatorCount` number of signed `pre-commit` messages which - /// need to be verified. - /// - /// Note that `MaxBridgedValidatorCount` should *not* match the exact number of validators - /// on the bridged chain. Instead it should be a number which is greater than the actual - /// number of validators in order to provide some buffer room should the validator set - /// increase in size. If this number ends up being lower than the actual number of - /// validators on the bridged chain you risk stalling the bridge. - #[pallet::constant] - type MaxBridgedValidatorCount: Get; - /// Weights gathered through benchmarking. type WeightInfo: WeightInfo; } @@ -141,8 +119,8 @@ pub mod pallet { /// If successful in verification, it will write the target header to the underlying storage /// pallet. #[pallet::weight(T::WeightInfo::submit_finality_proof( - T::MaxBridgedSessionLength::get().as_() as u32, - T::MaxBridgedValidatorCount::get(), + justification.votes_ancestries.len() as u32, + justification.commit.precommits.len() as u32, ))] pub fn submit_finality_proof( origin: OriginFor, @@ -181,15 +159,7 @@ pub mod pallet { log::info!(target: "runtime::bridge-grandpa", "Succesfully imported finalized header with hash {:?}!", hash); - // Note that the number of precommits is indicitive of the number of GRANDPA forks being - // voted on. - let precommits = justification.commit.precommits.len(); - - // This represents the average number of votes in a single fork since the weight formula - // uses that metric instead of the aggregated number of vote ancestries. - let votes = (justification.votes_ancestries.len() + precommits - 1) / precommits; - - Ok(Some(T::WeightInfo::submit_finality_proof(votes as u32, precommits as u32)).into()) + Ok(().into()) } /// Bootstrap the bridge pallet with an initial header and authority set from which to sync. diff --git a/bridges/modules/grandpa/src/mock.rs b/bridges/modules/grandpa/src/mock.rs index 1f9857eaf0..9646ebc552 100644 --- a/bridges/modules/grandpa/src/mock.rs +++ b/bridges/modules/grandpa/src/mock.rs @@ -87,8 +87,6 @@ parameter_types! { impl grandpa::Config for TestRuntime { type BridgedChain = TestBridgedChain; type MaxRequests = MaxRequests; - type MaxBridgedSessionLength = SessionLength; - type MaxBridgedValidatorCount = NumValidators; type WeightInfo = (); } diff --git a/bridges/modules/grandpa/src/weights.rs b/bridges/modules/grandpa/src/weights.rs index dec26d061d..57dba9a684 100644 --- a/bridges/modules/grandpa/src/weights.rs +++ b/bridges/modules/grandpa/src/weights.rs @@ -1,4 +1,4 @@ -// Copyright 2021 Parity Technologies (UK) Ltd. +// Copyright 2019-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 @@ -17,7 +17,7 @@ //! Autogenerated weights for pallet_bridge_grandpa //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 3.0.0 -//! DATE: 2021-03-26, STEPS: [50, ], REPEAT: 20 +//! DATE: 2021-04-02, STEPS: [50, ], REPEAT: 20 //! LOW RANGE: [], HIGH RANGE: [] //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled //! CHAIN: Some("dev"), DB CACHE: 128 @@ -57,85 +57,74 @@ use sp_std::marker::PhantomData; /// Weight functions needed for pallet_bridge_grandpa. pub trait WeightInfo { - fn submit_finality_proof(s: u32, p: u32) -> Weight; - fn submit_finality_proof_on_single_fork(s: u32) -> Weight; + fn submit_finality_proof(v: u32, p: u32) -> Weight; + fn submit_finality_proof_on_single_fork(v: u32) -> Weight; fn submit_finality_proof_on_many_forks(p: u32) -> Weight; fn find_scheduled_change(n: u32) -> Weight; fn read_write_authority_sets(n: u32) -> Weight; - fn write_authority_sets(n: u32) -> Weight; } /// Weights for pallet_bridge_grandpa using the Rialto node and recommended hardware. pub struct RialtoWeight(PhantomData); impl WeightInfo for RialtoWeight { - fn submit_finality_proof(s: u32, p: u32) -> Weight { + fn submit_finality_proof(v: u32, p: u32) -> Weight { (0 as Weight) - .saturating_add((3_248_661_000 as Weight).saturating_mul(s as Weight)) - .saturating_add((776_552_000 as Weight).saturating_mul(p as Weight)) + .saturating_add((160_060_000 as Weight).saturating_mul(v as Weight)) + .saturating_add((640_223_000 as Weight).saturating_mul(p as Weight)) .saturating_add(T::DbWeight::get().reads(5 as Weight)) .saturating_add(T::DbWeight::get().writes(3 as Weight)) } - fn submit_finality_proof_on_single_fork(s: u32) -> Weight { - (189_213_000 as Weight) - .saturating_add((12_937_000 as Weight).saturating_mul(s as Weight)) + fn submit_finality_proof_on_single_fork(v: u32) -> Weight { + (189_597_000 as Weight) + .saturating_add((11_680_000 as Weight).saturating_mul(v as Weight)) .saturating_add(T::DbWeight::get().reads(5 as Weight)) .saturating_add(T::DbWeight::get().writes(3 as Weight)) } fn submit_finality_proof_on_many_forks(p: u32) -> Weight { (0 as Weight) - .saturating_add((138_751_000 as Weight).saturating_mul(p as Weight)) + .saturating_add((130_061_000 as Weight).saturating_mul(p as Weight)) .saturating_add(T::DbWeight::get().reads(5 as Weight)) .saturating_add(T::DbWeight::get().writes(3 as Weight)) } fn find_scheduled_change(n: u32) -> Weight { - (301_000 as Weight).saturating_add((10_000 as Weight).saturating_mul(n as Weight)) + (502_000 as Weight).saturating_add((8_000 as Weight).saturating_mul(n as Weight)) } fn read_write_authority_sets(n: u32) -> Weight { - (6_787_000 as Weight) - .saturating_add((247_000 as Weight).saturating_mul(n as Weight)) + (7_677_000 as Weight) + .saturating_add((230_000 as Weight).saturating_mul(n as Weight)) .saturating_add(T::DbWeight::get().reads(1 as Weight)) .saturating_add(T::DbWeight::get().writes(1 as Weight)) } - fn write_authority_sets(n: u32) -> Weight { - (3_383_000 as Weight) - .saturating_add((99_000 as Weight).saturating_mul(n as Weight)) - .saturating_add(T::DbWeight::get().writes(1 as Weight)) - } } // For backwards compatibility and tests impl WeightInfo for () { - fn submit_finality_proof(s: u32, p: u32) -> Weight { + fn submit_finality_proof(v: u32, p: u32) -> Weight { (0 as Weight) - .saturating_add((3_248_661_000 as Weight).saturating_mul(s as Weight)) - .saturating_add((776_552_000 as Weight).saturating_mul(p as Weight)) + .saturating_add((160_060_000 as Weight).saturating_mul(v as Weight)) + .saturating_add((640_223_000 as Weight).saturating_mul(p as Weight)) .saturating_add(RocksDbWeight::get().reads(5 as Weight)) .saturating_add(RocksDbWeight::get().writes(3 as Weight)) } - fn submit_finality_proof_on_single_fork(s: u32) -> Weight { - (189_213_000 as Weight) - .saturating_add((12_937_000 as Weight).saturating_mul(s as Weight)) + fn submit_finality_proof_on_single_fork(v: u32) -> Weight { + (189_597_000 as Weight) + .saturating_add((11_680_000 as Weight).saturating_mul(v as Weight)) .saturating_add(RocksDbWeight::get().reads(5 as Weight)) .saturating_add(RocksDbWeight::get().writes(3 as Weight)) } fn submit_finality_proof_on_many_forks(p: u32) -> Weight { (0 as Weight) - .saturating_add((138_751_000 as Weight).saturating_mul(p as Weight)) + .saturating_add((130_061_000 as Weight).saturating_mul(p as Weight)) .saturating_add(RocksDbWeight::get().reads(5 as Weight)) .saturating_add(RocksDbWeight::get().writes(3 as Weight)) } fn find_scheduled_change(n: u32) -> Weight { - (301_000 as Weight).saturating_add((10_000 as Weight).saturating_mul(n as Weight)) + (502_000 as Weight).saturating_add((8_000 as Weight).saturating_mul(n as Weight)) } fn read_write_authority_sets(n: u32) -> Weight { - (6_787_000 as Weight) - .saturating_add((247_000 as Weight).saturating_mul(n as Weight)) + (7_677_000 as Weight) + .saturating_add((230_000 as Weight).saturating_mul(n as Weight)) .saturating_add(RocksDbWeight::get().reads(1 as Weight)) .saturating_add(RocksDbWeight::get().writes(1 as Weight)) } - fn write_authority_sets(n: u32) -> Weight { - (3_383_000 as Weight) - .saturating_add((99_000 as Weight).saturating_mul(n as Weight)) - .saturating_add(RocksDbWeight::get().writes(1 as Weight)) - } } diff --git a/bridges/primitives/header-chain/tests/justification.rs b/bridges/primitives/header-chain/tests/justification.rs index 96bb6d442a..e17b24ed81 100644 --- a/bridges/primitives/header-chain/tests/justification.rs +++ b/bridges/primitives/header-chain/tests/justification.rs @@ -23,24 +23,29 @@ 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 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, + authorities: authorities.clone(), + votes: 7, + forks: 3, }; + let justification = make_justification_for_header::(params.clone()); assert_eq!( verify_justification::( header_id::(1), TEST_GRANDPA_SET_ID, &voter_set(), - &make_justification_for_header::(params) + &justification, ), Ok(()), ); + + assert_eq!(justification.commit.precommits.len(), authorities.len()); + assert_eq!(justification.votes_ancestries.len(), params.votes as usize); } #[test] @@ -50,7 +55,7 @@ fn valid_justification_accepted_with_single_fork() { round: TEST_GRANDPA_ROUND, set_id: TEST_GRANDPA_SET_ID, authorities: vec![(ALICE, 1), (BOB, 1), (CHARLIE, 1), (DAVE, 1), (EVE, 1)], - depth: 5, + votes: 5, forks: 1, }; @@ -78,7 +83,7 @@ fn valid_justification_accepted_with_arbitrary_number_of_authorities() { round: TEST_GRANDPA_ROUND, set_id: TEST_GRANDPA_SET_ID, authorities: authorities.clone(), - depth: 5, + votes: n.into(), forks: n.into(), }; @@ -163,12 +168,14 @@ fn justification_with_invalid_precommit_ancestry() { #[test] 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 authorities = vec![(ALICE, 1), (BOB, 1)]; + 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, + authorities: authorities.clone(), + votes: 2 * authorities.len() as u32, forks: 2, }; diff --git a/bridges/primitives/test-utils/src/lib.rs b/bridges/primitives/test-utils/src/lib.rs index f6c2f6d094..39652fc83a 100644 --- a/bridges/primitives/test-utils/src/lib.rs +++ b/bridges/primitives/test-utils/src/lib.rs @@ -34,6 +34,7 @@ pub const TEST_GRANDPA_ROUND: u64 = 1; pub const TEST_GRANDPA_SET_ID: SetId = 1; /// Configuration parameters when generating test GRANDPA justifications. +#[derive(Clone)] pub struct JustificationGeneratorParams { /// The header which we want to finalize. pub header: H, @@ -42,10 +43,16 @@ pub struct JustificationGeneratorParams { /// The current authority set ID. pub set_id: SetId, /// The current GRANDPA authority set. + /// + /// The size of the set will determine the number of pre-commits in our justification. pub authorities: Vec<(Account, 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. + /// The total number of vote ancestries in our justification. + /// + /// These may be distributed among many different forks. + pub votes: u32, + /// The number of forks. + /// + /// Useful for creating a "worst-case" scenario in which each authority is on its own fork. pub forks: u32, } @@ -56,7 +63,7 @@ impl Default for JustificationGeneratorParams { round: TEST_GRANDPA_ROUND, set_id: TEST_GRANDPA_SET_ID, authorities: test_keyring(), - depth: 2, + votes: 2, forks: 1, } } @@ -76,8 +83,8 @@ pub fn make_default_justification(header: &H) -> GrandpaJustificatio /// 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). +/// a specific number of pre-commits (tuned with the number of "authorities") and/or a specific +/// number of vote ancestries (tuned with the "votes" parameter). /// /// Note: This needs at least three authorities or else the verifier will complain about /// being given an invalid commit. @@ -87,7 +94,7 @@ pub fn make_justification_for_header(params: JustificationGeneratorP round, set_id, authorities, - depth, + mut votes, forks, } = params; @@ -95,15 +102,27 @@ pub fn make_justification_for_header(params: JustificationGeneratorP let mut precommits = vec![]; let mut votes_ancestries = vec![]; - assert!(depth != 0, "Can't have a chain of zero length."); + assert!(forks != 0, "Need at least one fork to have a chain.."); + assert!(votes >= forks, "Need at least one header per fork."); assert!( forks as usize <= authorities.len(), "If we have more forks than authorities we can't create valid pre-commits for all the forks." ); + // Roughly, how many vote ancestries do we want per fork + let target_depth = (votes + forks - 1) / forks; + let mut unsigned_precommits = vec![]; for i in 0..forks { - let chain = generate_chain(i as u8, depth, &header); + let depth = if votes >= target_depth { + votes -= target_depth; + target_depth + } else { + votes + }; + + // Note: Adding 1 to account for the target header + let chain = generate_chain(i as u8, depth + 1, &header); // We don't include our finality target header in the vote ancestries for child in &chain[1..] {