Improve call, and usage in pallet utility (paritytech#9418)

* WIP

* WIP

* WIP

* add some tests and limit

* remove wip test

* fmt

* Update bin/node/runtime/src/lib.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* fmt

* use primitives allocation limit

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

bin/node/runtime/src/lib.rs

@@ -1679,4 +1679,14 @@ mod tests {
 
 		is_submit_signed_transaction::<Runtime>();
 	}
+
+	#[test]
+	fn call_size() {
+		assert!(
+			core::mem::size_of::<Call>() <= 200,
+			"size of Call is more than 200 bytes: some calls have too big arguments, use Box to reduce the
+			size of Call.
+			If the limit is too strong, maybe consider increase the limit to 300.",
+		);
+	}
 }

frame/babe/src/equivocation.rs

@@ -155,7 +155,8 @@ where
 	) -> DispatchResult {
 		use frame_system::offchain::SubmitTransaction;
 
-		let call = Call::report_equivocation_unsigned(equivocation_proof, key_owner_proof);
+		let call =
+			Call::report_equivocation_unsigned(Box::new(equivocation_proof), key_owner_proof);
 
 		match SubmitTransaction::<T, Call<T>>::submit_unsigned_transaction(call.into()) {
 			Ok(()) => log::info!(

frame/babe/src/lib.rs

@@ -349,12 +349,12 @@ pub mod pallet {
 		))]
 		pub fn report_equivocation(
 			origin: OriginFor<T>,
-			equivocation_proof: EquivocationProof<T::Header>,
+			equivocation_proof: Box<EquivocationProof<T::Header>>,
 			key_owner_proof: T::KeyOwnerProof,
 		) -> DispatchResultWithPostInfo {
 			let reporter = ensure_signed(origin)?;
 
-			Self::do_report_equivocation(Some(reporter), equivocation_proof, key_owner_proof)
+			Self::do_report_equivocation(Some(reporter), *equivocation_proof, key_owner_proof)
 		}
 
 		/// Report authority equivocation/misbehavior. This method will verify
@@ -370,14 +370,14 @@ pub mod pallet {
 		))]
 		pub fn report_equivocation_unsigned(
 			origin: OriginFor<T>,
-			equivocation_proof: EquivocationProof<T::Header>,
+			equivocation_proof: Box<EquivocationProof<T::Header>>,
 			key_owner_proof: T::KeyOwnerProof,
 		) -> DispatchResultWithPostInfo {
 			ensure_none(origin)?;
 
 			Self::do_report_equivocation(
 				T::HandleEquivocation::block_author(),
-				equivocation_proof,
+				*equivocation_proof,
 				key_owner_proof,
 			)
 		}

frame/babe/src/tests.rs

@@ -440,8 +440,12 @@ fn report_equivocation_current_session_works() {
 		let key_owner_proof = Historical::prove(key).unwrap();
 
 		// report the equivocation
-		Babe::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof)
-			.unwrap();
+		Babe::report_equivocation_unsigned(
+			Origin::none(),
+			Box::new(equivocation_proof),
+			key_owner_proof,
+		)
+		.unwrap();
 
 		// start a new era so that the results of the offence report
 		// are applied at era end
@@ -508,8 +512,12 @@ fn report_equivocation_old_session_works() {
 		assert_eq!(Staking::slashable_balance_of(&offending_validator_id), 10_000);
 
 		// report the equivocation
-		Babe::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof)
-			.unwrap();
+		Babe::report_equivocation_unsigned(
+			Origin::none(),
+			Box::new(equivocation_proof),
+			key_owner_proof,
+		)
+		.unwrap();
 
 		// start a new era so that the results of the offence report
 		// are applied at era end
@@ -558,7 +566,7 @@ fn report_equivocation_invalid_key_owner_proof() {
 		assert_err!(
 			Babe::report_equivocation_unsigned(
 				Origin::none(),
-				equivocation_proof.clone(),
+				Box::new(equivocation_proof.clone()),
 				key_owner_proof
 			),
 			Error::<Test>::InvalidKeyOwnershipProof,
@@ -576,7 +584,11 @@ fn report_equivocation_invalid_key_owner_proof() {
 		start_era(2);
 
 		assert_err!(
-			Babe::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof),
+			Babe::report_equivocation_unsigned(
+				Origin::none(),
+				Box::new(equivocation_proof),
+				key_owner_proof,
+			),
 			Error::<Test>::InvalidKeyOwnershipProof,
 		);
 	})
@@ -608,7 +620,7 @@ fn report_equivocation_invalid_equivocation_proof() {
 			assert_err!(
 				Babe::report_equivocation_unsigned(
 					Origin::none(),
-					equivocation_proof,
+					Box::new(equivocation_proof),
 					key_owner_proof.clone(),
 				),
 				Error::<Test>::InvalidEquivocationProof,
@@ -714,8 +726,10 @@ fn report_equivocation_validate_unsigned_prevents_duplicates() {
 		let key = (sp_consensus_babe::KEY_TYPE, &offending_authority_pair.public());
 		let key_owner_proof = Historical::prove(key).unwrap();
 
-		let inner =
-			Call::report_equivocation_unsigned(equivocation_proof.clone(), key_owner_proof.clone());
+		let inner = Call::report_equivocation_unsigned(
+			Box::new(equivocation_proof.clone()),
+			key_owner_proof.clone(),
+		);
 
 		// only local/inblock reports are allowed
 		assert_eq!(
@@ -746,8 +760,12 @@ fn report_equivocation_validate_unsigned_prevents_duplicates() {
 		assert_ok!(<Babe as sp_runtime::traits::ValidateUnsigned>::pre_dispatch(&inner));
 
 		// we submit the report
-		Babe::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof)
-			.unwrap();
+		Babe::report_equivocation_unsigned(
+			Origin::none(),
+			Box::new(equivocation_proof),
+			key_owner_proof,
+		)
+		.unwrap();
 
 		// the report should now be considered stale and the transaction is invalid.
 		// the check for staleness should be done on both `validate_unsigned` and on `pre_dispatch`
@@ -805,7 +823,7 @@ fn valid_equivocation_reports_dont_pay_fees() {
 
 		// check the dispatch info for the call.
 		let info = Call::<Test>::report_equivocation_unsigned(
-			equivocation_proof.clone(),
+			Box::new(equivocation_proof.clone()),
 			key_owner_proof.clone(),
 		)
 		.get_dispatch_info();
@@ -817,7 +835,7 @@ fn valid_equivocation_reports_dont_pay_fees() {
 		// report the equivocation.
 		let post_info = Babe::report_equivocation_unsigned(
 			Origin::none(),
-			equivocation_proof.clone(),
+			Box::new(equivocation_proof.clone()),
 			key_owner_proof.clone(),
 		)
 		.unwrap();
@@ -829,11 +847,14 @@ fn valid_equivocation_reports_dont_pay_fees() {
 
 		// report the equivocation again which is invalid now since it is
 		// duplicate.
-		let post_info =
-			Babe::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof)
-				.err()
-				.unwrap()
-				.post_info;
+		let post_info = Babe::report_equivocation_unsigned(
+			Origin::none(),
+			Box::new(equivocation_proof),
+			key_owner_proof,
+		)
+		.err()
+		.unwrap()
+		.post_info;
 
 		// the fee is not waived and the original weight is kept.
 		assert!(post_info.actual_weight.is_none());

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

@@ -26,7 +26,10 @@ use rand::{prelude::SliceRandom, rngs::SmallRng, SeedableRng};
 use sp_arithmetic::{per_things::Percent, traits::One};
 use sp_npos_elections::IndexAssignment;
 use sp_runtime::InnerOf;
-use sp_std::convert::{TryFrom, TryInto};
+use sp_std::{
+	boxed::Box,
+	convert::{TryFrom, TryInto},
+};
 
 const SEED: u32 = 999;
 
@@ -317,7 +320,7 @@ frame_benchmarking::benchmarks! {
 		let caller = frame_benchmarking::whitelisted_caller();
 		T::Currency::make_free_balance_be(&caller,  T::Currency::minimum_balance() * 10u32.into());
 
-	}: _(RawOrigin::Signed(caller), solution, c)
+	}: _(RawOrigin::Signed(caller), Box::new(solution), c)
 	verify {
 		assert!(<MultiPhase<T>>::signed_submissions().len() as u32 == c + 1);
 	}
@@ -344,9 +347,15 @@ frame_benchmarking::benchmarks! {
 
 		// encode the most significant storage item that needs to be decoded in the dispatch.
 		let encoded_snapshot = <MultiPhase<T>>::snapshot().unwrap().encode();
-		let encoded_call = <Call<T>>::submit_unsigned(raw_solution.clone(), witness).encode();
+		let encoded_call = <Call<T>>::submit_unsigned(Box::new(raw_solution.clone()), witness).encode();
 	}: {
-		assert_ok!(<MultiPhase<T>>::submit_unsigned(RawOrigin::None.into(), raw_solution, witness));
+		assert_ok!(
+			<MultiPhase<T>>::submit_unsigned(
+				RawOrigin::None.into(),
+				Box::new(raw_solution),
+				witness,
+			)
+		);
 		let _decoded_snap = <RoundSnapshot<T::AccountId> as Decode>::decode(&mut &*encoded_snapshot)
 			.unwrap();
 		let _decoded_call = <Call<T> as Decode>::decode(&mut &*encoded_call).unwrap();

frame/election-provider-multi-phase/src/lib.rs

@@ -857,7 +857,7 @@ pub mod pallet {
 		))]
 		pub fn submit_unsigned(
 			origin: OriginFor<T>,
-			solution: RawSolution<CompactOf<T>>,
+			solution: Box<RawSolution<CompactOf<T>>>,
 			witness: SolutionOrSnapshotSize,
 		) -> DispatchResultWithPostInfo {
 			ensure_none(origin)?;
@@ -876,7 +876,7 @@ pub mod pallet {
 			assert!(targets as u32 == witness.targets, "{}", error_message);
 
 			let ready =
-				Self::feasibility_check(solution, ElectionCompute::Unsigned).expect(error_message);
+				Self::feasibility_check(*solution, ElectionCompute::Unsigned).expect(error_message);
 
 			// Store the newly received solution.
 			log!(info, "queued unsigned solution with score {:?}", ready.score);
@@ -947,7 +947,7 @@ pub mod pallet {
 		#[pallet::weight(T::WeightInfo::submit(*num_signed_submissions))]
 		pub fn submit(
 			origin: OriginFor<T>,
-			solution: RawSolution<CompactOf<T>>,
+			solution: Box<RawSolution<CompactOf<T>>>,
 			num_signed_submissions: u32,
 		) -> DispatchResult {
 			let who = ensure_signed(origin)?;
@@ -982,7 +982,8 @@ pub mod pallet {
 				T::SignedRewardBase::get().saturating_add(call_fee)
 			};
 
-			let submission = SignedSubmission { who: who.clone(), deposit, solution, reward };
+			let submission =
+				SignedSubmission { who: who.clone(), deposit, solution: *solution, reward };
 
 			// insert the submission if the queue has space or it's better than the weakest
 			// eject the weakest if the queue was full
@@ -1927,7 +1928,7 @@ mod tests {
 				let solution = RawSolution { score: [(5 + s).into(), 0, 0], ..Default::default() };
 				assert_ok!(MultiPhase::submit(
 					crate::mock::Origin::signed(99),
-					solution,
+					Box::new(solution),
 					MultiPhase::signed_submissions().len() as u32
 				));
 			}

frame/election-provider-multi-phase/src/signed.rs

@@ -499,7 +499,11 @@ mod tests {
 		origin: Origin,
 		solution: RawSolution<CompactOf<Runtime>>,
 	) -> DispatchResult {
-		MultiPhase::submit(origin, solution, MultiPhase::signed_submissions().len() as u32)
+		MultiPhase::submit(
+			origin,
+			Box::new(solution),
+			MultiPhase::signed_submissions().len() as u32,
+		)
 	}
 
 	#[test]
@@ -532,7 +536,7 @@ mod tests {
 
 			// now try and cheat by passing a lower queue length
 			assert_noop!(
-				MultiPhase::submit(Origin::signed(99), solution, 0),
+				MultiPhase::submit(Origin::signed(99), Box::new(solution), 0),
 				Error::<Runtime>::SignedInvalidWitness,
 			);
 		})

frame/election-provider-multi-phase/src/unsigned.rs

@@ -34,7 +34,7 @@ use sp_runtime::{
 	traits::TrailingZeroInput,
 	DispatchError, SaturatedConversion,
 };
-use sp_std::{cmp::Ordering, convert::TryFrom, vec::Vec};
+use sp_std::{boxed::Box, cmp::Ordering, convert::TryFrom, vec::Vec};
 
 /// Storage key used to store the last block number at which offchain worker ran.
 pub(crate) const OFFCHAIN_LAST_BLOCK: &[u8] = b"parity/multi-phase-unsigned-election";
@@ -208,7 +208,7 @@ impl<T: Config> Pallet<T> {
 		let (raw_solution, witness) = Self::mine_and_check(iters)?;
 
 		let score = raw_solution.score.clone();
-		let call: Call<T> = Call::submit_unsigned(raw_solution, witness).into();
+		let call: Call<T> = Call::submit_unsigned(Box::new(raw_solution), witness).into();
 
 		log!(
 			debug,
@@ -773,7 +773,7 @@ mod tests {
 	fn validate_unsigned_retracts_wrong_phase() {
 		ExtBuilder::default().desired_targets(0).build_and_execute(|| {
 			let solution = RawSolution::<TestCompact> { score: [5, 0, 0], ..Default::default() };
-			let call = Call::submit_unsigned(solution.clone(), witness());
+			let call = Call::submit_unsigned(Box::new(solution.clone()), witness());
 
 			// initial
 			assert_eq!(MultiPhase::current_phase(), Phase::Off);
@@ -842,7 +842,7 @@ mod tests {
 			assert!(MultiPhase::current_phase().is_unsigned());
 
 			let solution = RawSolution::<TestCompact> { score: [5, 0, 0], ..Default::default() };
-			let call = Call::submit_unsigned(solution.clone(), witness());
+			let call = Call::submit_unsigned(Box::new(solution.clone()), witness());
 
 			// initial
 			assert!(<MultiPhase as ValidateUnsigned>::validate_unsigned(
@@ -879,7 +879,7 @@ mod tests {
 			assert!(MultiPhase::current_phase().is_unsigned());
 
 			let solution = RawSolution::<TestCompact> { score: [5, 0, 0], ..Default::default() };
-			let call = Call::submit_unsigned(solution.clone(), witness());
+			let call = Call::submit_unsigned(Box::new(solution.clone()), witness());
 			assert_eq!(solution.compact.unique_targets().len(), 0);
 
 			// won't work anymore.
@@ -905,7 +905,7 @@ mod tests {
 
 				let solution =
 					RawSolution::<TestCompact> { score: [5, 0, 0], ..Default::default() };
-				let call = Call::submit_unsigned(solution.clone(), witness());
+				let call = Call::submit_unsigned(Box::new(solution.clone()), witness());
 
 				assert_eq!(
 					<MultiPhase as ValidateUnsigned>::validate_unsigned(
@@ -931,7 +931,7 @@ mod tests {
 
 			// This is in itself an invalid BS solution.
 			let solution = RawSolution::<TestCompact> { score: [5, 0, 0], ..Default::default() };
-			let call = Call::submit_unsigned(solution.clone(), witness());
+			let call = Call::submit_unsigned(Box::new(solution.clone()), witness());
 			let outer_call: OuterCall = call.into();
 			let _ = outer_call.dispatch(Origin::none());
 		})
@@ -951,7 +951,7 @@ mod tests {
 			let mut correct_witness = witness();
 			correct_witness.voters += 1;
 			correct_witness.targets -= 1;
-			let call = Call::submit_unsigned(solution.clone(), correct_witness);
+			let call = Call::submit_unsigned(Box::new(solution.clone()), correct_witness);
 			let outer_call: OuterCall = call.into();
 			let _ = outer_call.dispatch(Origin::none());
 		})
@@ -972,7 +972,7 @@ mod tests {
 
 			// ensure this solution is valid.
 			assert!(MultiPhase::queued_solution().is_none());
-			assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, witness));
+			assert_ok!(MultiPhase::submit_unsigned(Origin::none(), Box::new(solution), witness));
 			assert!(MultiPhase::queued_solution().is_some());
 		})
 	}
@@ -1054,7 +1054,11 @@ mod tests {
 				};
 				let (solution, witness) = MultiPhase::prepare_election_result(result).unwrap();
 				assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution));
-				assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, witness));
+				assert_ok!(MultiPhase::submit_unsigned(
+					Origin::none(),
+					Box::new(solution),
+					witness
+				));
 				assert_eq!(MultiPhase::queued_solution().unwrap().score[0], 10);
 
 				// trial 1: a solution who's score is only 2, i.e. 20% better in the first element.
@@ -1096,7 +1100,11 @@ mod tests {
 
 				// and it is fine
 				assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution));
-				assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, witness));
+				assert_ok!(MultiPhase::submit_unsigned(
+					Origin::none(),
+					Box::new(solution),
+					witness
+				));
 			})
 	}