Use Vote Ancestries Directly in Weight Calculations (#862)

* Use more accurate weight calculation in declared weight

* Remove session length and validator set size config constants

* Remove config params from mock

* Allow specifying total number of votes-ancestries per justification

* Change limits used during benchmarking

* Regenerate weights

* Use simplified weight annotation

* Remove comment

* Address leftover TODO

* Prevent possible divide by zero errors

* Use correct argument order in weight declaration

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<Runtime>;
@@ -334,8 +327,6 @@ pub type WestendGrandpaInstance = pallet_bridge_grandpa::Instance1;
 impl pallet_bridge_grandpa::Config<WestendGrandpaInstance> 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<Runtime>;

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<Runtime>;
 }
 

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::<Vec<_>>(),
-			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::<Vec<_>>(),
-			depth: 2,
+			votes: p,
 			forks: p,
 		};
 

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<u32>;
 
-		/// 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<BridgedBlockNumber<Self, I>>;
-
-		/// 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<u32>;
-
 		/// 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<T>,
@@ -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.

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 = ();
 }
 

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<T>(PhantomData<T>);
 impl<T: frame_system::Config> WeightInfo for RialtoWeight<T> {
-	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))
-	}
 }

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::<TestHeader>(params.clone());
 	assert_eq!(
 		verify_justification::<TestHeader>(
 			header_id::<TestHeader>(1),
 			TEST_GRANDPA_SET_ID,
 			&voter_set(),
-			&make_justification_for_header::<TestHeader>(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,
 	};
 

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<H> {
 	/// The header which we want to finalize.
 	pub header: H,
@@ -42,10 +43,16 @@ pub struct JustificationGeneratorParams<H> {
 	/// 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<H: HeaderT> Default for JustificationGeneratorParams<H> {
 			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<H: HeaderT>(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<H: HeaderT>(params: JustificationGeneratorP
 		round,
 		set_id,
 		authorities,
-		depth,
+		mut votes,
 		forks,
 	} = params;
 
@@ -95,15 +102,27 @@ pub fn make_justification_for_header<H: HeaderT>(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..] {