Storage Layer for All FRAME Extrinsics (#11431)

* add new trait

* implement DispatchableWithStorageLayer

* at least one transactional

* all dispatch is at least transactional

* storage_layer api

* add test

* storage layer tests

* deprecate transactional tag

* i guess no reason to deprecate

* remove transactional from batch_all

* update tests

* extend trait

* cargo run --quiet --profile=production --features runtime-benchmarks --manifest-path bin/node/cli/Cargo.toml -- benchmark pallet --chain=dev --steps=50 --repeat=20 --pallet=pallet_balances --extrinsic=* --execution=wasm --wasm-execution=compiled --output=./frame/balances/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* cargo run --quiet --profile=production --features runtime-benchmarks --manifest-path bin/node/cli/Cargo.toml -- benchmark pallet --chain=dev --steps=50 --repeat=20 --pallet=pallet_balances --extrinsic=* --execution=wasm --wasm-execution=compiled --output=./frame/balances/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* cargo run --quiet --profile=production --features runtime-benchmarks --manifest-path bin/node/cli/Cargo.toml -- benchmark pallet --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* fix copy paste name

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

* Create run_all_benchmarks.sh

* uncomment build

* update number of steps and repeats

* add skip build

* Update run_all_benchmarks.sh

* Update run_all_benchmarks.sh

* new benchmarks

* Update frame/support/src/traits/dispatch.rs

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

* Update frame/support/src/traits/dispatch.rs

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

* Update frame/support/test/tests/storage_layers.rs

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

* Update frame/support/test/tests/storage_layers.rs

* weights

* Update dispatch.rs

* doc link

* decl_macro support

Co-authored-by: Parity Bot <admin@parity.io>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

substrate/frame/support/src/dispatch.rs

@@ -27,7 +27,8 @@ pub use crate::{
 		result,
 	},
 	traits::{
-		CallMetadata, GetCallMetadata, GetCallName, GetStorageVersion, UnfilteredDispatchable,
+		CallMetadata, DispatchableWithStorageLayer, GetCallMetadata, GetCallName,
+		GetStorageVersion, UnfilteredDispatchable,
 	},
 	weights::{
 		ClassifyDispatch, DispatchInfo, GetDispatchInfo, PaysFee, PostDispatchInfo,
@@ -1469,7 +1470,11 @@ macro_rules! decl_module {
 		$ignore:ident
 		$mod_type:ident<$trait_instance:ident $(, $instance:ident)?> $fn_name:ident $origin:ident $system:ident [ $( $param_name:ident),* ]
 	) => {
-		<$mod_type<$trait_instance $(, $instance)?>>::$fn_name( $origin $(, $param_name )* ).map(Into::into).map_err(Into::into)
+			// We execute all dispatchable in at least one storage layer, allowing them
+			// to return an error at any point, and undoing any storage changes.
+			$crate::storage::in_storage_layer(|| {
+				<$mod_type<$trait_instance $(, $instance)?>>::$fn_name( $origin $(, $param_name )* ).map(Into::into).map_err(Into::into)
+			})
 	};
 
 	// no `deposit_event` function wanted

substrate/frame/support/src/storage/mod.rs

@@ -30,7 +30,9 @@ use sp_runtime::generic::{Digest, DigestItem};
 use sp_std::prelude::*;
 
 pub use self::{
-	transactional::{with_transaction, with_transaction_unchecked},
+	transactional::{
+		in_storage_layer, with_storage_layer, with_transaction, with_transaction_unchecked,
+	},
 	types::StorageEntryMetadataBuilder,
 };
 pub use sp_runtime::TransactionOutcome;

substrate/frame/support/src/storage/transactional.rs

@@ -158,6 +158,40 @@ pub fn with_transaction_unchecked<R>(f: impl FnOnce() -> TransactionOutcome<R>)
 	}
 }
 
+/// Execute the supplied function, adding a new storage layer.
+///
+/// This is the same as `with_transaction`, but assuming that any function returning
+/// an `Err` should rollback, and any function returning `Ok` should commit. This
+/// provides a cleaner API to the developer who wants this behavior.
+pub fn with_storage_layer<T, E>(f: impl FnOnce() -> Result<T, E>) -> Result<T, E>
+where
+	E: From<DispatchError>,
+{
+	with_transaction(|| {
+		let r = f();
+		if r.is_ok() {
+			TransactionOutcome::Commit(r)
+		} else {
+			TransactionOutcome::Rollback(r)
+		}
+	})
+}
+
+/// Execute the supplied function, ensuring we are at least in one storage layer.
+///
+/// If we are already in a storage layer, we just execute the provided closure.
+/// If we are not, we execute the closure within a [`with_storage_layer`].
+pub fn in_storage_layer<T, E>(f: impl FnOnce() -> Result<T, E>) -> Result<T, E>
+where
+	E: From<DispatchError>,
+{
+	if is_transactional() {
+		f()
+	} else {
+		with_storage_layer(f)
+	}
+}
+
 #[cfg(test)]
 mod tests {
 	use super::*;
@@ -229,4 +263,33 @@ mod tests {
 			assert_eq!(get_transaction_level(), 0);
 		});
 	}
+
+	#[test]
+	fn in_storage_layer_works() {
+		TestExternalities::default().execute_with(|| {
+			assert_eq!(get_transaction_level(), 0);
+
+			let res = in_storage_layer(|| -> DispatchResult {
+				assert_eq!(get_transaction_level(), 1);
+				in_storage_layer(|| -> DispatchResult {
+					// We are still in the same layer :)
+					assert_eq!(get_transaction_level(), 1);
+					Ok(())
+				})
+			});
+
+			assert_ok!(res);
+
+			let res = in_storage_layer(|| -> DispatchResult {
+				assert_eq!(get_transaction_level(), 1);
+				in_storage_layer(|| -> DispatchResult {
+					// We are still in the same layer :)
+					assert_eq!(get_transaction_level(), 1);
+					Err("epic fail".into())
+				})
+			});
+
+			assert_noop!(res, "epic fail");
+		});
+	}
 }

substrate/frame/support/src/traits.rs

@@ -94,8 +94,8 @@ pub use storage::{
 
 mod dispatch;
 pub use dispatch::{
-	AsEnsureOriginWithArg, EnsureOneOf, EnsureOrigin, EnsureOriginWithArg, OriginTrait,
-	UnfilteredDispatchable,
+	AsEnsureOriginWithArg, DispatchableWithStorageLayer, EnsureOneOf, EnsureOrigin,
+	EnsureOriginWithArg, OriginTrait, UnfilteredDispatchable,
 };
 
 mod voting;

substrate/frame/support/src/traits/dispatch.rs

@@ -101,6 +101,23 @@ pub trait UnfilteredDispatchable {
 	fn dispatch_bypass_filter(self, origin: Self::Origin) -> DispatchResultWithPostInfo;
 }
 
+/// Type that can be dispatched with an additional storage layer which is used to execute the call.
+pub trait DispatchableWithStorageLayer {
+	/// The origin type of the runtime, (i.e. `frame_system::Config::Origin`).
+	type Origin;
+
+	/// Same as `dispatch` from the [`frame_support::dispatch::Dispatchable`] trait, but
+	/// specifically spawns a new storage layer to execute the call inside of.
+	fn dispatch_with_storage_layer(self, origin: Self::Origin) -> DispatchResultWithPostInfo;
+
+	/// Same as `dispatch_bypass_filter` from the [`UnfilteredDispatchable`] trait, but specifically
+	/// spawns a new storage layer to execute the call inside of.
+	fn dispatch_bypass_filter_with_storage_layer(
+		self,
+		origin: Self::Origin,
+	) -> DispatchResultWithPostInfo;
+}
+
 /// Methods available on `frame_system::Config::Origin`.
 pub trait OriginTrait: Sized {
 	/// Runtime call type, as in `frame_system::Config::Call`

substrate/frame/support/src/weights/block_weights.rs

@@ -16,7 +16,7 @@
 // limitations under the License.
 
 //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev
-//! DATE: 2022-05-23 (Y/M/D)
+//! DATE: 2022-05-24 (Y/M/D)
 //!
 //! SHORT-NAME: `block`, LONG-NAME: `BlockExecution`, RUNTIME: `Development`
 //! WARMUPS: `10`, REPEAT: `100`
@@ -27,7 +27,7 @@
 //   ./target/production/substrate
 //   benchmark
 //   overhead
-//   --dev
+//   --chain=dev
 //   --execution=wasm
 //   --wasm-execution=compiled
 //   --weight-path=./frame/support/src/weights/
@@ -44,16 +44,16 @@ parameter_types! {
 	/// Calculated by multiplying the *Average* with `1` and adding `0`.
 	///
 	/// Stats nanoseconds:
-	///   Min, Max: 5_295_788, 5_473_440
-	///   Average:  5_343_308
-	///   Median:   5_323_240
-	///   Std-Dev:  38368.68
+	///   Min, Max: 5_303_128, 5_507_784
+	///   Average:  5_346_284
+	///   Median:   5_328_139
+	///   Std-Dev:  41749.5
 	///
 	/// Percentiles nanoseconds:
-	///   99th: 5_470_141
-	///   95th: 5_418_269
-	///   75th: 5_355_579
-	pub const BlockExecutionWeight: Weight = 5_343_308 * WEIGHT_PER_NANOS;
+	///   99th: 5_489_273
+	///   95th: 5_433_314
+	///   75th: 5_354_812
+	pub const BlockExecutionWeight: Weight = 5_346_284 * WEIGHT_PER_NANOS;
 }
 
 #[cfg(test)]

substrate/frame/support/src/weights/extrinsic_weights.rs

@@ -16,7 +16,7 @@
 // limitations under the License.
 
 //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev
-//! DATE: 2022-05-23 (Y/M/D)
+//! DATE: 2022-05-24 (Y/M/D)
 //!
 //! SHORT-NAME: `extrinsic`, LONG-NAME: `ExtrinsicBase`, RUNTIME: `Development`
 //! WARMUPS: `10`, REPEAT: `100`
@@ -27,7 +27,7 @@
 //   ./target/production/substrate
 //   benchmark
 //   overhead
-//   --dev
+//   --chain=dev
 //   --execution=wasm
 //   --wasm-execution=compiled
 //   --weight-path=./frame/support/src/weights/
@@ -44,16 +44,16 @@ parameter_types! {
 	/// Calculated by multiplying the *Average* with `1` and adding `0`.
 	///
 	/// Stats nanoseconds:
-	///   Min, Max: 85_423, 86_142
-	///   Average:  85_795
-	///   Median:   85_790
-	///   Std-Dev:  162.37
+	///   Min, Max: 86_060, 86_999
+	///   Average:  86_298
+	///   Median:   86_248
+	///   Std-Dev:  207.19
 	///
 	/// Percentiles nanoseconds:
-	///   99th: 86_115
-	///   95th: 86_069
-	///   75th: 85_937
-	pub const ExtrinsicBaseWeight: Weight = 85_795 * WEIGHT_PER_NANOS;
+	///   99th: 86_924
+	///   95th: 86_828
+	///   75th: 86_347
+	pub const ExtrinsicBaseWeight: Weight = 86_298 * WEIGHT_PER_NANOS;
 }
 
 #[cfg(test)]