Update submit finality proof weight formula (#981)

* updated weight formula for submit_finality_proof

* remove common prefix traces

* update docs

* single benchmark

* Re-generate weights.

* Update delivery transaction limits

Co-authored-by: Tomasz Drwięga <tomasz@parity.io>
Co-authored-by: Hernando Castano <hernando@hcastano.com>

bridges/modules/grandpa/src/benchmarking.rs

@@ -38,24 +38,18 @@
 //!
 //! Note that the worst case scenario here would be a justification where each validator has it's
 //! own fork which is `SESSION_LENGTH` blocks long.
-//!
-//! As far as benchmarking results go, the only benchmark that should be used in
-//! `pallet-bridge-grandpa` to annotate weights is the `submit_finality_proof` one. The others are
-//! looking at the effects of specific code paths and do not actually reflect the overall worst case
-//! scenario.
 
 use crate::*;
 
 use bp_test_utils::{
-	accounts, authority_list, make_justification_for_header, test_keyring, JustificationGeneratorParams, ALICE,
-	TEST_GRANDPA_ROUND, TEST_GRANDPA_SET_ID,
+	accounts, make_justification_for_header, JustificationGeneratorParams, 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 sp_finality_grandpa::AuthorityId;
 use sp_runtime::traits::Zero;
-use sp_std::{vec, vec::Vec};
+use sp_std::vec::Vec;
 
 // The maximum number of vote ancestries to include in a justification.
 //
@@ -75,81 +69,46 @@ fn header_number<T: Config<I>, I: 'static, N: From<u32>>() -> N {
 	(T::HeadersToKeep::get() + 1).into()
 }
 
+/// Prepare header and its justification to submit using `submit_finality_proof`.
+fn prepare_benchmark_data<T: Config<I>, I: 'static>(
+	precommits: u32,
+	ancestors: u32,
+) -> (BridgedHeader<T, I>, GrandpaJustification<BridgedHeader<T, I>>) {
+	let authority_list = accounts(precommits as u16)
+		.iter()
+		.map(|id| (AuthorityId::from(*id), 1))
+		.collect::<Vec<_>>();
+
+	let init_data = InitializationData {
+		header: bp_test_utils::test_header(Zero::zero()),
+		authority_list,
+		set_id: TEST_GRANDPA_SET_ID,
+		is_halted: false,
+	};
+
+	bootstrap_bridge::<T, I>(init_data);
+
+	let header: BridgedHeader<T, I> = bp_test_utils::test_header(header_number::<T, I, _>());
+	let params = JustificationGeneratorParams {
+		header: header.clone(),
+		round: TEST_GRANDPA_ROUND,
+		set_id: TEST_GRANDPA_SET_ID,
+		authorities: accounts(precommits as u16).iter().map(|k| (*k, 1)).collect::<Vec<_>>(),
+		ancestors,
+		forks: 1,
+	};
+	let justification = make_justification_for_header(params);
+	(header, justification)
+}
+
 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.
-	//
-	// 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 v in 1..MAX_VOTE_ANCESTRIES;
 		let p in 1..MAX_VALIDATOR_SET_SIZE;
-
-		let caller: T::AccountId = whitelisted_caller();
-
-		let authority_list = accounts(p as u16)
-			.iter()
-			.map(|id| (AuthorityId::from(*id), 1))
-			.collect::<Vec<_>>();
-
-		let init_data = InitializationData {
-			header: bp_test_utils::test_header(Zero::zero()),
-			authority_list,
-			set_id: TEST_GRANDPA_SET_ID,
-			is_halted: false,
-		};
-
-		bootstrap_bridge::<T, I>(init_data);
-
-		let header: BridgedHeader<T, I> = bp_test_utils::test_header(header_number::<T, I, _>());
-		let params = JustificationGeneratorParams {
-			header: header.clone(),
-			round: TEST_GRANDPA_ROUND,
-			set_id: TEST_GRANDPA_SET_ID,
-			authorities: accounts(p as u16).iter().map(|k| (*k, 1)).collect::<Vec<_>>(),
-			votes: v,
-			forks: 1,
-		};
-
-		let justification = make_justification_for_header(params);
-
-	}: _(RawOrigin::Signed(caller), header, justification)
-	verify {
-		let header: BridgedHeader<T, I> = bp_test_utils::test_header(header_number::<T, I, _>());
-		let expected_hash = header.hash();
-
-		assert_eq!(<BestFinalized<T, I>>::get(), expected_hash);
-		assert!(<ImportedHeaders<T, I>>::contains_key(expected_hash));
-	}
-
-	// 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 v in 1..MAX_VOTE_ANCESTRIES;
-
 		let caller: T::AccountId = whitelisted_caller();
-
-		let init_data = InitializationData {
-			header: bp_test_utils::test_header(Zero::zero()),
-			authority_list: authority_list(),
-			set_id: TEST_GRANDPA_SET_ID,
-			is_halted: false,
-		};
-
-		bootstrap_bridge::<T, I>(init_data);
-		let header: BridgedHeader<T, I> = bp_test_utils::test_header(header_number::<T, I, _>());
-
-		let params = JustificationGeneratorParams {
-			header: header.clone(),
-			round: TEST_GRANDPA_ROUND,
-			set_id: TEST_GRANDPA_SET_ID,
-			authorities: test_keyring(),
-			votes: v,
-			forks: 1,
-		};
-
-		let justification = make_justification_for_header(params);
-
+		let (header, justification) = prepare_benchmark_data::<T, I>(p, v);
 	}: submit_finality_proof(RawOrigin::Signed(caller), header, justification)
 	verify {
 		let header: BridgedHeader<T, I> = bp_test_utils::test_header(header_number::<T, I, _>());
@@ -158,124 +117,4 @@ benchmarks_instance_pallet! {
 		assert_eq!(<BestFinalized<T, I>>::get(), expected_hash);
 		assert!(<ImportedHeaders<T, I>>::contains_key(expected_hash));
 	}
-
-	// What we want to check here is the effect of many pre-commits on justification verification.
-	// 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..MAX_VALIDATOR_SET_SIZE;
-
-		let caller: T::AccountId = whitelisted_caller();
-
-		let authority_list = accounts(p as u16)
-			.iter()
-			.map(|id| (AuthorityId::from(*id), 1))
-			.collect::<Vec<_>>();
-
-		let init_data = InitializationData {
-			header: bp_test_utils::test_header(Zero::zero()),
-			authority_list,
-			set_id: TEST_GRANDPA_SET_ID,
-			is_halted: false,
-		};
-
-		bootstrap_bridge::<T, I>(init_data);
-		let header: BridgedHeader<T, I> = bp_test_utils::test_header(header_number::<T, I, _>());
-
-		let params = JustificationGeneratorParams {
-			header: header.clone(),
-			round: TEST_GRANDPA_ROUND,
-			set_id: TEST_GRANDPA_SET_ID,
-			authorities: accounts(p as u16).iter().map(|k| (*k, 1)).collect::<Vec<_>>(),
-			votes: p,
-			forks: p,
-		};
-
-		let justification = make_justification_for_header(params);
-
-	}: submit_finality_proof(RawOrigin::Signed(caller), header, justification)
-	verify {
-		let header: BridgedHeader<T, I> = bp_test_utils::test_header(header_number::<T, I, _>());
-		let expected_hash = header.hash();
-
-		assert_eq!(<BestFinalized<T, I>>::get(), expected_hash);
-		assert!(<ImportedHeaders<T, I>>::contains_key(expected_hash));
-	}
-
-	// Here we want to find out the overheaded of looking through consensus digests found in a
-	// header. As the number of logs in a header grows, how much more work do we require to look
-	// through them?
-	//
-	// Note that this should be the same for looking through scheduled changes and forces changes,
-	// which is why we only have one benchmark for this.
-	find_scheduled_change {
-		// Not really sure what a good bound for this is.
-		let n in 1..1000;
-
-		let mut logs = vec![];
-		for i in 0..n {
-			// We chose a non-consensus log on purpose since that way we have to look through all
-			// the logs in the header
-			logs.push(sp_runtime::DigestItem::Other(vec![]));
-		}
-
-		let mut header: BridgedHeader<T, I> = bp_test_utils::test_header(Zero::zero());
-		let digest = header.digest_mut();
-		*digest = sp_runtime::Digest {
-			logs,
-		};
-
-	}: {
-		crate::find_scheduled_change(&header)
-	}
-
-	// What we want to check here is how long it takes to read and write the authority set tracked
-	// by the pallet as the number of authorities grows.
-	read_write_authority_sets {
-		// The current max target number of validators on Polkadot/Kusama
-		let n in 1..1000;
-
-		let mut authorities = vec![];
-		for i in 0..n {
-			authorities.push((ALICE, 1));
-		}
-
-		let authority_set = bp_header_chain::AuthoritySet {
-			authorities: authorities.iter().map(|(id, w)| (AuthorityId::from(*id), *w)).collect(),
-			set_id: 0
-		};
-
-		<CurrentAuthoritySet<T, I>>::put(&authority_set);
-
-	}: {
-		let authority_set = <CurrentAuthoritySet<T, I>>::get();
-		<CurrentAuthoritySet<T, I>>::put(&authority_set);
-	}
-}
-
-#[cfg(test)]
-mod tests {
-	use super::*;
-	use frame_support::assert_ok;
-
-	#[test]
-	fn finality_proof_is_valid() {
-		mock::run_test(|| {
-			assert_ok!(test_benchmark_submit_finality_proof::<mock::TestRuntime>());
-		});
-	}
-
-	#[test]
-	fn single_fork_finality_proof_is_valid() {
-		mock::run_test(|| {
-			assert_ok!(test_benchmark_submit_finality_proof_on_single_fork::<mock::TestRuntime>());
-		});
-	}
-
-	#[test]
-	fn multi_fork_finality_proof_is_valid() {
-		mock::run_test(|| {
-			assert_ok!(test_benchmark_submit_finality_proof_on_many_forks::<mock::TestRuntime>());
-		});
-	}
 }

bridges/modules/grandpa/src/lib.rs

@@ -46,6 +46,7 @@ use frame_support::{ensure, fail};
 use frame_system::{ensure_signed, RawOrigin};
 use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID};
 use sp_runtime::traits::{BadOrigin, Header as HeaderT, Zero};
+use sp_std::convert::TryInto;
 
 #[cfg(test)]
 mod mock;
@@ -124,8 +125,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(
-			justification.votes_ancestries.len() as u32,
-			justification.commit.precommits.len() as u32,
+			justification.commit.precommits.len().try_into().unwrap_or(u32::MAX),
+			justification.votes_ancestries.len().try_into().unwrap_or(u32::MAX),
 		))]
 		pub fn submit_finality_proof(
 			origin: OriginFor<T>,

bridges/modules/grandpa/src/weights.rs

@@ -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-04-21, STEPS: [50, ], REPEAT: 20
+//! DATE: 2021-06-03, STEPS: [50, ], REPEAT: 20
 //! LOW RANGE: [], HIGH RANGE: []
 //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled
 //! CHAIN: Some("dev"), DB CACHE: 128
@@ -48,74 +48,28 @@ use sp_std::marker::PhantomData;
 
 /// Weight functions needed for pallet_bridge_grandpa.
 pub trait WeightInfo {
-	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 submit_finality_proof(p: u32, v: 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(v: u32, p: u32) -> Weight {
+	fn submit_finality_proof(p: u32, v: u32) -> Weight {
 		(0 as Weight)
-			.saturating_add((756_462_000 as Weight).saturating_mul(v as Weight))
-			.saturating_add((791_236_000 as Weight).saturating_mul(p as Weight))
+			.saturating_add((59_692_000 as Weight).saturating_mul(p as Weight))
+			.saturating_add((6_876_000 as Weight).saturating_mul(v as Weight))
 			.saturating_add(T::DbWeight::get().reads(7 as Weight))
 			.saturating_add(T::DbWeight::get().writes(6 as Weight))
 	}
-	fn submit_finality_proof_on_single_fork(v: u32) -> Weight {
-		(280_121_000 as Weight)
-			.saturating_add((14_098_000 as Weight).saturating_mul(v as Weight))
-			.saturating_add(T::DbWeight::get().reads(7 as Weight))
-			.saturating_add(T::DbWeight::get().writes(6 as Weight))
-	}
-	fn submit_finality_proof_on_many_forks(p: u32) -> Weight {
-		(10_370_940_000 as Weight)
-			.saturating_add((96_902_000 as Weight).saturating_mul(p as Weight))
-			.saturating_add(T::DbWeight::get().reads(7 as Weight))
-			.saturating_add(T::DbWeight::get().writes(6 as Weight))
-	}
-	fn find_scheduled_change(n: u32) -> Weight {
-		(479_000 as Weight).saturating_add((11_000 as Weight).saturating_mul(n as Weight))
-	}
-	fn read_write_authority_sets(n: u32) -> Weight {
-		(8_030_000 as Weight)
-			.saturating_add((232_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))
-	}
 }
 
 // For backwards compatibility and tests
 impl WeightInfo for () {
-	fn submit_finality_proof(v: u32, p: u32) -> Weight {
+	fn submit_finality_proof(p: u32, v: u32) -> Weight {
 		(0 as Weight)
-			.saturating_add((756_462_000 as Weight).saturating_mul(v as Weight))
-			.saturating_add((791_236_000 as Weight).saturating_mul(p as Weight))
+			.saturating_add((59_692_000 as Weight).saturating_mul(p as Weight))
+			.saturating_add((6_876_000 as Weight).saturating_mul(v as Weight))
 			.saturating_add(RocksDbWeight::get().reads(7 as Weight))
 			.saturating_add(RocksDbWeight::get().writes(6 as Weight))
 	}
-	fn submit_finality_proof_on_single_fork(v: u32) -> Weight {
-		(280_121_000 as Weight)
-			.saturating_add((14_098_000 as Weight).saturating_mul(v as Weight))
-			.saturating_add(RocksDbWeight::get().reads(7 as Weight))
-			.saturating_add(RocksDbWeight::get().writes(6 as Weight))
-	}
-	fn submit_finality_proof_on_many_forks(p: u32) -> Weight {
-		(10_370_940_000 as Weight)
-			.saturating_add((96_902_000 as Weight).saturating_mul(p as Weight))
-			.saturating_add(RocksDbWeight::get().reads(7 as Weight))
-			.saturating_add(RocksDbWeight::get().writes(6 as Weight))
-	}
-	fn find_scheduled_change(n: u32) -> Weight {
-		(479_000 as Weight).saturating_add((11_000 as Weight).saturating_mul(n as Weight))
-	}
-	fn read_write_authority_sets(n: u32) -> Weight {
-		(8_030_000 as Weight)
-			.saturating_add((232_000 as Weight).saturating_mul(n as Weight))
-			.saturating_add(RocksDbWeight::get().reads(1 as Weight))
-			.saturating_add(RocksDbWeight::get().writes(1 as Weight))
-	}
 }