Add weight for decoding the call to benchmarks (#9781)

* First two bechmarks converted

* Add decoding weight to benchmarks.

* Update frame/democracy/src/benchmarking.rs

* Adapt to new Call-ing convention

* Resolve conflicts and change more calls

* Remove error impl for codec and use plain `expect` for error handling instead

* Compile fix

* Spaces to tabs

* Update frame/democracy/src/benchmarking.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Add origin-type specifier to benchmarks macro

* formatting

* Update frame/benchmarking/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Change manual to automatic benchmark syntax

* Formatting

* Revert "Change manual to automatic benchmark syntax"

This reverts commit ea5b5d906b318b6525c1e6d2bd05c5011595c21a.

Because tests are lost and cleanup code in the verify function is not run on failing calls.

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

substrate/frame/benchmarking/src/lib.rs

@@ -131,6 +131,13 @@ macro_rules! whitelist {
 ///     let c = 0 .. 10 => setup_c_in_some_other_way(&caller, c);
 ///   }: baz(Origin::Signed(caller))
 ///
+///   // You may optionally specify the origin type if it can't be determined automatically like
+///   // this.
+///   baz3 {
+///     let caller = account::<T>(b"caller", 0, benchmarks_seed);
+///     let l in 1 .. MAX_LENGTH => initialize_l(l);
+///   }: baz<T::Origin>(Origin::Signed(caller), vec![0u8; l])
+///
 ///   // this is benchmarking some code that is not a dispatchable.
 ///   populate_a_set {
 ///     let x in 0 .. 10_000;
@@ -305,7 +312,7 @@ macro_rules! benchmarks_iter {
 		( $( $names:tt )* ) // This contains $( $( { $instance } )? $name:ident )*
 		( $( $names_extra:tt )* )
 		( $( $names_skip_meta:tt )* )
-		$name:ident { $( $code:tt )* }: _ ( $origin:expr $( , $arg:expr )* )
+		$name:ident { $( $code:tt )* }: _ $(< $origin_type:ty>)? ( $origin:expr $( , $arg:expr )* )
 		verify $postcode:block
 		$( $rest:tt )*
 	) => {
@@ -315,7 +322,7 @@ macro_rules! benchmarks_iter {
 			( $( $names )* )
 			( $( $names_extra )* )
 			( $( $names_skip_meta )* )
-			$name { $( $code )* }: $name ( $origin $( , $arg )* )
+			$name { $( $code )* }: $name $(< $origin_type >)? ( $origin $( , $arg )* )
 			verify $postcode
 			$( $rest )*
 		}
@@ -327,7 +334,7 @@ macro_rules! benchmarks_iter {
 		( $( $names:tt )* )
 		( $( $names_extra:tt )* )
 		( $( $names_skip_meta:tt )* )
-		$name:ident { $( $code:tt )* }: $dispatch:ident ( $origin:expr $( , $arg:expr )* )
+		$name:ident { $( $code:tt )* }: $dispatch:ident $(<$origin_type:ty>)? ( $origin:expr $( , $arg:expr )* )
 		verify $postcode:block
 		$( $rest:tt )*
 	) => {
@@ -350,15 +357,14 @@ macro_rules! benchmarks_iter {
 						&__call
 					);
 				}: {
-					let call_decoded = <
+					let __call_decoded = <
 						Call<T $(, $instance )?>
 						as $crate::frame_support::codec::Decode
 					>::decode(&mut &__benchmarked_call_encoded[..])
 						.expect("call is encoded above, encoding must be correct");
-
-					<
-						Call<T $(, $instance)? > as $crate::frame_support::traits::UnfilteredDispatchable
-					>::dispatch_bypass_filter(call_decoded, $origin.into())?;
+					let __origin = $crate::to_origin!($origin $(, $origin_type)?);
+					<Call<T $(, $instance)? > as $crate::frame_support::traits::UnfilteredDispatchable
+						>::dispatch_bypass_filter(__call_decoded, __origin)?;
 				}
 				verify $postcode
 				$( $rest )*
@@ -488,6 +494,17 @@ macro_rules! benchmarks_iter {
 	};
 }
 
+#[macro_export]
+#[doc(hidden)]
+macro_rules! to_origin {
+	($origin:expr) => {
+		$origin.into()
+	};
+	($origin:expr, $origin_type:ty) => {
+		<T::Origin as From<$origin_type>>::from($origin)
+	};
+}
+
 #[macro_export]
 #[doc(hidden)]
 macro_rules! benchmark_backend {

substrate/frame/benchmarking/src/utils.rs

@@ -16,7 +16,6 @@
 // limitations under the License.
 
 //! Interfaces, types and utils for benchmarking a FRAME runtime.
-
 use codec::{Decode, Encode};
 use frame_support::{
 	dispatch::{DispatchError, DispatchErrorWithPostInfo},

substrate/frame/democracy/src/benchmarking.rs

@@ -22,6 +22,7 @@ use super::*;
 use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelist_account};
 use frame_support::{
 	assert_noop, assert_ok,
+	codec::Decode,
 	traits::{
 		schedule::DispatchTime, Currency, EnsureOrigin, Get, OnInitialize, UnfilteredDispatchable,
 	},
@@ -194,9 +195,8 @@ benchmarks! {
 	emergency_cancel {
 		let origin = T::CancellationOrigin::successful_origin();
 		let referendum_index = add_referendum::<T>(0)?;
-		let call = Call::<T>::emergency_cancel { ref_index: referendum_index };
 		assert_ok!(Democracy::<T>::referendum_status(referendum_index));
-	}: { call.dispatch_bypass_filter(origin)? }
+	}: _<T::Origin>(origin, referendum_index)
 	verify {
 		// Referendum has been canceled
 		assert_noop!(
@@ -219,14 +219,11 @@ benchmarks! {
 		assert_ok!(
 			Democracy::<T>::external_propose(T::ExternalOrigin::successful_origin(), hash.clone())
 		);
-
+		let origin = T::BlacklistOrigin::successful_origin();
 		// Add a referendum of our proposal.
 		let referendum_index = add_referendum::<T>(0)?;
 		assert_ok!(Democracy::<T>::referendum_status(referendum_index));
-
-		let call = Call::<T>::blacklist { proposal_hash: hash, maybe_ref_index: Some(referendum_index) };
-		let origin = T::BlacklistOrigin::successful_origin();
-	}: { call.dispatch_bypass_filter(origin)? }
+	}: _<T::Origin>(origin, hash, Some(referendum_index))
 	verify {
 		// Referendum has been canceled
 		assert_noop!(
@@ -246,9 +243,7 @@ benchmarks! {
 			proposal_hash,
 			(T::BlockNumber::zero(), vec![T::AccountId::default(); v as usize])
 		);
-
-		let call = Call::<T>::external_propose { proposal_hash };
-	}: { call.dispatch_bypass_filter(origin)? }
+	}: _<T::Origin>(origin, proposal_hash)
 	verify {
 		// External proposal created
 		ensure!(<NextExternal<T>>::exists(), "External proposal didn't work");
@@ -257,8 +252,7 @@ benchmarks! {
 	external_propose_majority {
 		let origin = T::ExternalMajorityOrigin::successful_origin();
 		let proposal_hash = T::Hashing::hash_of(&0);
-		let call = Call::<T>::external_propose_majority { proposal_hash };
-	}: { call.dispatch_bypass_filter(origin)? }
+	}: _<T::Origin>(origin, proposal_hash)
 	verify {
 		// External proposal created
 		ensure!(<NextExternal<T>>::exists(), "External proposal didn't work");
@@ -267,8 +261,7 @@ benchmarks! {
 	external_propose_default {
 		let origin = T::ExternalDefaultOrigin::successful_origin();
 		let proposal_hash = T::Hashing::hash_of(&0);
-		let call = Call::<T>::external_propose_default { proposal_hash };
-	}: { call.dispatch_bypass_filter(origin)? }
+	}: _<T::Origin>(origin, proposal_hash)
 	verify {
 		// External proposal created
 		ensure!(<NextExternal<T>>::exists(), "External proposal didn't work");
@@ -283,13 +276,7 @@ benchmarks! {
 		let origin_fast_track = T::FastTrackOrigin::successful_origin();
 		let voting_period = T::FastTrackVotingPeriod::get();
 		let delay = 0u32;
-		let call = Call::<T>::fast_track {
-			proposal_hash,
-			voting_period: voting_period.into(),
-			delay: delay.into()
-		};
-
-	}: { call.dispatch_bypass_filter(origin_fast_track)? }
+	}: _<T::Origin>(origin_fast_track, proposal_hash, voting_period.into(), delay.into())
 	verify {
 		assert_eq!(Democracy::<T>::referendum_count(), 1, "referendum not created")
 	}
@@ -310,10 +297,9 @@ benchmarks! {
 		vetoers.sort();
 		Blacklist::<T>::insert(proposal_hash, (T::BlockNumber::zero(), vetoers));
 
-		let call = Call::<T>::veto_external { proposal_hash };
 		let origin = T::VetoOrigin::successful_origin();
 		ensure!(NextExternal::<T>::get().is_some(), "no external proposal");
-	}: { call.dispatch_bypass_filter(origin)? }
+	}: _<T::Origin>(origin, proposal_hash)
 	verify {
 		assert!(NextExternal::<T>::get().is_none());
 		let (_, new_vetoers) = <Blacklist<T>>::get(&proposal_hash).ok_or("no blacklist")?;
@@ -774,9 +760,13 @@ benchmarks! {
 			Some(PreimageStatus::Available { .. }) => (),
 			_ => return Err("preimage not available".into())
 		}
+		let origin = RawOrigin::Root.into();
+		let call = Call::<T>::enact_proposal { proposal_hash, index: 0 }.encode();
 	}: {
 		assert_eq!(
-			Democracy::<T>::enact_proposal(RawOrigin::Root.into(), proposal_hash, 0),
+			<Call<T> as Decode>::decode(&mut &*call)
+				.expect("call is encoded above, encoding must be correct")
+				.dispatch_bypass_filter(origin),
 			Err(Error::<T>::PreimageInvalid.into())
 		);
 	}

substrate/frame/election-provider-multi-phase/src/benchmarking.rs

@@ -350,7 +350,6 @@ frame_benchmarking::benchmarks! {
 
 		assert!(<MultiPhase<T>>::queued_solution().is_none());
 		<CurrentPhase<T>>::put(Phase::Unsigned((true, 1u32.into())));
-
 	}: _(RawOrigin::None, Box::new(raw_solution), witness)
 	verify {
 		assert!(<MultiPhase<T>>::queued_solution().is_some());

substrate/frame/elections-phragmen/src/benchmarking.rs

@@ -24,7 +24,10 @@ use super::*;
 use frame_benchmarking::{
 	account, benchmarks, impl_benchmark_test_suite, whitelist, BenchmarkError, BenchmarkResult,
 };
-use frame_support::{dispatch::DispatchResultWithPostInfo, traits::OnInitialize};
+use frame_support::{
+	dispatch::{DispatchResultWithPostInfo, UnfilteredDispatchable},
+	traits::OnInitialize,
+};
 use frame_system::RawOrigin;
 
 use crate::Pallet as Elections;
@@ -401,15 +404,23 @@ benchmarks! {
 
 		let _ = fill_seats_up_to::<T>(m)?;
 		let removing = as_lookup::<T>(<Elections<T>>::members_ids()[0].clone());
+		let who = T::Lookup::lookup(removing.clone()).expect("member was added above");
+		let call = Call::<T>::remove_member { who: removing, has_replacement: false }.encode();
 	}: {
 		assert_eq!(
-			<Elections<T>>::remove_member(RawOrigin::Root.into(), removing, false).unwrap_err().error,
+			<Call<T> as Decode>::decode(&mut &*call)
+				.expect("call is encoded above, encoding must be correct")
+				.dispatch_bypass_filter(RawOrigin::Root.into())
+				.unwrap_err()
+				.error,
 			Error::<T>::InvalidReplacement.into(),
 		);
 	}
 	verify {
 		// must still have enough members.
 		assert_eq!(<Elections<T>>::members().len() as u32, T::DesiredMembers::get());
+		// on fail, `who` must still be a member
+		assert!(<Elections<T>>::members_ids().contains(&who));
 		#[cfg(test)]
 		{
 			// reset members in between benchmark tests.

substrate/frame/gilt/src/benchmarking.rs

@@ -50,17 +50,12 @@ benchmarks! {
 
 	place_bid_max {
 		let caller: T::AccountId = whitelisted_caller();
+		let origin = RawOrigin::Signed(caller.clone());
 		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
 		for i in 0..T::MaxQueueLen::get() {
-			Gilt::<T>::place_bid(RawOrigin::Signed(caller.clone()).into(), T::MinFreeze::get(), 1)?;
+			Gilt::<T>::place_bid(origin.clone().into(), T::MinFreeze::get(), 1)?;
 		}
-	}: {
-		Gilt::<T>::place_bid(
-			RawOrigin::Signed(caller.clone()).into(),
-			T::MinFreeze::get() * BalanceOf::<T>::from(2u32),
-			1,
-		)?
-	}
+	}: place_bid(origin, T::MinFreeze::get() * BalanceOf::<T>::from(2u32), 1)
 	verify {
 		assert_eq!(QueueTotals::<T>::get()[0], (
 			T::MaxQueueLen::get(),
@@ -81,9 +76,9 @@ benchmarks! {
 	}
 
 	set_target {
-		let call = Call::<T>::set_target { target: Default::default() };
 		let origin = T::AdminOrigin::successful_origin();
-	}: { call.dispatch_bypass_filter(origin)? }
+	}: _<T::Origin>(origin, Default::default())
+	verify {}
 
 	thaw {
 		let caller: T::AccountId = whitelisted_caller();

substrate/frame/im-online/src/benchmarking.rs

@@ -93,10 +93,12 @@ benchmarks! {
 		let e in 1 .. MAX_EXTERNAL_ADDRESSES;
 		let (input_heartbeat, signature) = create_heartbeat::<T>(k, e)?;
 		let call = Call::heartbeat { heartbeat: input_heartbeat, signature };
+		let call_enc = call.encode();
 	}: {
-		ImOnline::<T>::validate_unsigned(TransactionSource::InBlock, &call)
-			.map_err(<&str>::from)?;
-		call.dispatch_bypass_filter(RawOrigin::None.into())?;
+		ImOnline::<T>::validate_unsigned(TransactionSource::InBlock, &call).map_err(<&str>::from)?;
+		<Call<T> as Decode>::decode(&mut &*call_enc)
+			.expect("call is encoded above, encoding must be correct")
+			.dispatch_bypass_filter(RawOrigin::None.into())?;
 	}
 }
 

substrate/frame/lottery/src/benchmarking.rs

@@ -22,7 +22,7 @@
 use super::*;
 
 use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller};
-use frame_support::traits::{EnsureOrigin, OnInitialize, UnfilteredDispatchable};
+use frame_support::traits::{EnsureOrigin, OnInitialize};
 use frame_system::RawOrigin;
 use sp_runtime::traits::{Bounded, Zero};
 
@@ -73,11 +73,9 @@ benchmarks! {
 	set_calls {
 		let n in 0 .. T::MaxCalls::get() as u32;
 		let calls = vec![frame_system::Call::<T>::remark { remark: vec![] }.into(); n as usize];
-
-		let call = Call::<T>::set_calls { calls };
 		let origin = T::ManagerOrigin::successful_origin();
 		assert!(CallIndices::<T>::get().is_empty());
-	}: { call.dispatch_bypass_filter(origin)? }
+	}: _<T::Origin>(origin, calls)
 	verify {
 		if !n.is_zero() {
 			assert!(!CallIndices::<T>::get().is_empty());
@@ -88,10 +86,8 @@ benchmarks! {
 		let price = BalanceOf::<T>::max_value();
 		let end = 10u32.into();
 		let payout = 5u32.into();
-
-		let call = Call::<T>::start_lottery { price, length: end, delay: payout, repeat: true };
 		let origin = T::ManagerOrigin::successful_origin();
-	}: { call.dispatch_bypass_filter(origin)? }
+	}: _<T::Origin>(origin, price, end, payout, true)
 	verify {
 		assert!(crate::Lottery::<T>::get().is_some());
 	}
@@ -99,9 +95,8 @@ benchmarks! {
 	stop_repeat {
 		setup_lottery::<T>(true)?;
 		assert_eq!(crate::Lottery::<T>::get().unwrap().repeat, true);
-		let call = Call::<T>::stop_repeat {};
 		let origin = T::ManagerOrigin::successful_origin();
-	}: { call.dispatch_bypass_filter(origin)? }
+	}: _<T::Origin>(origin)
 	verify {
 		assert_eq!(crate::Lottery::<T>::get().unwrap().repeat, false);
 	}

substrate/frame/staking/src/benchmarking.rs

@@ -23,6 +23,7 @@ use testing_utils::*;
 
 use frame_election_provider_support::SortedListProvider;
 use frame_support::{
+	dispatch::UnfilteredDispatchable,
 	pallet_prelude::*,
 	traits::{Currency, CurrencyToVote, Get, Imbalance},
 };
@@ -764,9 +765,15 @@ benchmarks! {
 		<ErasValidatorReward<T>>::insert(current_era, total_payout);
 
 		let caller: T::AccountId = whitelisted_caller();
+		let origin = RawOrigin::Signed(caller);
+		let calls: Vec<_> = payout_calls_arg.iter().map(|arg|
+			Call::<T>::payout_stakers { validator_stash: arg.0.clone(), era: arg.1 }.encode()
+		).collect();
 	}: {
-		for arg in payout_calls_arg {
-			<Staking<T>>::payout_stakers(RawOrigin::Signed(caller.clone()).into(), arg.0, arg.1)?;
+		for call in calls {
+			<Call<T> as Decode>::decode(&mut &*call)
+				.expect("call is encoded above, encoding must be correct")
+				.dispatch_bypass_filter(origin.clone().into())?;
 		}
 	}