Add Call Filter That Prevents Nested `batch_all` (#9009)

* add filter preventing nested `batch_all`

* more tests

* fix test

* cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_utility --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/utility/src/weights.rs --template=./.maintain/frame-weight-template.hbs

Co-authored-by: Parity Bot <admin@parity.io>

substrate/frame/utility/src/lib.rs

@@ -62,7 +62,7 @@ use sp_core::TypeId;
 use sp_io::hashing::blake2_256;
 use frame_support::{
 	transactional,
-	traits::{OriginTrait, UnfilteredDispatchable},
+	traits::{OriginTrait, UnfilteredDispatchable, IsSubType},
 	weights::{GetDispatchInfo, extract_actual_weight},
 	dispatch::PostDispatchInfo,
 };
@@ -91,7 +91,9 @@ pub mod pallet {
 		/// The overarching call type.
 		type Call: Parameter + Dispatchable<Origin=Self::Origin, PostInfo=PostDispatchInfo>
 			+ GetDispatchInfo + From<frame_system::Call<Self>>
-			+ UnfilteredDispatchable<Origin=Self::Origin>;
+			+ UnfilteredDispatchable<Origin=Self::Origin>
+			+ IsSubType<Call<Self>>
+			+ IsType<<Self as frame_system::Config>::Call>;
 
 		/// Weight information for extrinsics in this pallet.
 		type WeightInfo: WeightInfo;
@@ -266,7 +268,13 @@ pub mod pallet {
 				let result = if is_root {
 					call.dispatch_bypass_filter(origin.clone())
 				} else {
-					call.dispatch(origin.clone())
+					let mut filtered_origin = origin.clone();
+					// Don't allow users to nest `batch_all` calls.
+					filtered_origin.add_filter(move |c: &<T as frame_system::Config>::Call| {
+						let c = <T as Config>::Call::from_ref(c);
+						!matches!(c.is_sub_type(), Some(Call::batch_all(_)))
+					});
+					call.dispatch(filtered_origin)
 				};
 				// Add the weight of this call.
 				weight = weight.saturating_add(extract_actual_weight(&result, &info));

substrate/frame/utility/src/tests.rs

@@ -519,3 +519,44 @@ fn batch_all_handles_weight_refund() {
 		);
 	});
 }
+
+#[test]
+fn batch_all_does_not_nest() {
+	new_test_ext().execute_with(|| {
+		let batch_all = Call::Utility(
+			UtilityCall::batch_all(
+				vec![
+					Call::Balances(BalancesCall::transfer(2, 1)),
+					Call::Balances(BalancesCall::transfer(2, 1)),
+					Call::Balances(BalancesCall::transfer(2, 1)),
+				]
+			)
+		);
+
+		let info = batch_all.get_dispatch_info();
+
+		assert_eq!(Balances::free_balance(1), 10);
+		assert_eq!(Balances::free_balance(2), 10);
+		// A nested batch_all call will not pass the filter, and fail with `BadOrigin`.
+		assert_noop!(
+			Utility::batch_all(Origin::signed(1), vec![batch_all.clone()]),
+			DispatchErrorWithPostInfo {
+				post_info: PostDispatchInfo {
+					actual_weight: Some(<Test as Config>::WeightInfo::batch_all(1) + info.weight),
+					pays_fee: Pays::Yes
+				},
+				error: DispatchError::BadOrigin,
+			}
+		);
+
+		// And for those who want to get a little fancy, we check that the filter persists across
+		// other kinds of dispatch wrapping functions... in this case `batch_all(batch(batch_all(..)))`
+		let batch_nested = Call::Utility(UtilityCall::batch(vec![batch_all]));
+		// Batch will end with `Ok`, but does not actually execute as we can see from the event
+		// and balances.
+		assert_ok!(Utility::batch_all(Origin::signed(1), vec![batch_nested]));
+		System::assert_has_event(utility::Event::BatchInterrupted(0, DispatchError::BadOrigin).into());
+		assert_eq!(Balances::free_balance(1), 10);
+		assert_eq!(Balances::free_balance(2), 10);
+	});
+}

substrate/frame/utility/src/weights.rs

 // This file is part of Substrate.
 
-// Copyright (C) 2020-2021 Parity Technologies (UK) Ltd.
+// Copyright (C) 2021 Parity Technologies (UK) Ltd.
 // SPDX-License-Identifier: Apache-2.0
 
 // Licensed under the Apache License, Version 2.0 (the "License");
@@ -15,9 +15,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-//! Weights for pallet_utility
-//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0
-//! DATE: 2020-10-27, STEPS: `[50, ]`, REPEAT: 20, LOW RANGE: [], HIGH RANGE: []
+//! Autogenerated weights for pallet_utility
+//!
+//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 3.0.0
+//! DATE: 2021-06-03, STEPS: `[50, ]`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]`
 //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 128
 
 // Executed Command:
@@ -46,44 +47,39 @@ pub trait WeightInfo {
 	fn batch(c: u32, ) -> Weight;
 	fn as_derivative() -> Weight;
 	fn batch_all(c: u32, ) -> Weight;
-	
 }
 
 /// Weights for pallet_utility using the Substrate node and recommended hardware.
 pub struct SubstrateWeight<T>(PhantomData<T>);
 impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	fn batch(c: u32, ) -> Weight {
-		(20_071_000 as Weight)
-			.saturating_add((2_739_000 as Weight).saturating_mul(c as Weight))
-			
+		(19_099_000 as Weight)
+			// Standard Error: 1_000
+			.saturating_add((640_000 as Weight).saturating_mul(c as Weight))
 	}
 	fn as_derivative() -> Weight {
-		(5_721_000 as Weight)
-			
+		(3_701_000 as Weight)
 	}
 	fn batch_all(c: u32, ) -> Weight {
-		(21_440_000 as Weight)
-			.saturating_add((2_738_000 as Weight).saturating_mul(c as Weight))
-			
+		(19_199_000 as Weight)
+			// Standard Error: 0
+			.saturating_add((1_061_000 as Weight).saturating_mul(c as Weight))
 	}
-	
 }
 
 // For backwards compatibility and tests
 impl WeightInfo for () {
 	fn batch(c: u32, ) -> Weight {
-		(20_071_000 as Weight)
-			.saturating_add((2_739_000 as Weight).saturating_mul(c as Weight))
-			
+		(19_099_000 as Weight)
+			// Standard Error: 1_000
+			.saturating_add((640_000 as Weight).saturating_mul(c as Weight))
 	}
 	fn as_derivative() -> Weight {
-		(5_721_000 as Weight)
-			
+		(3_701_000 as Weight)
 	}
 	fn batch_all(c: u32, ) -> Weight {
-		(21_440_000 as Weight)
-			.saturating_add((2_738_000 as Weight).saturating_mul(c as Weight))
-			
+		(19_199_000 as Weight)
+			// Standard Error: 0
+			.saturating_add((1_061_000 as Weight).saturating_mul(c as Weight))
 	}
-	
 }