Make TransactionExtension tuple of tuple transparent for implication

prdoc/pr_7028.prdoc

@@ -0,0 +1,25 @@
+title: 'Fix implication order in implementation of `TransactionExtension` for tuple'
+doc:
+- audience:
+  - Runtime Dev
+  - Runtime User
+  description: |-
+    Before this PR, the implications were different in the pipeline `(A, B, C)` and `((A, B), C)`.
+    This PR fixes this behavior and make nested tuple transparant, the implication order of tuple of
+    tuple is now the same as in a single tuple.
+
+    For runtime users this mean that the implication can be breaking depending on the pipeline used
+    in the runtime.
+
+    For runtime developers this breaks usage of `TransactionExtension::validate`.
+    When calling `TransactionExtension::validate` the implication must now implement `Implication`
+    trait, you can use `TxBaseImplication` to wrap the type and use it as the base implication.
+    E.g. instead of `&(extension_version, call),` you can write `&TxBaseImplication((extension_version, call))`.
+
+crates:
+- name: sp-runtime
+  bump: major
+- name: pallet-skip-feeless-payment
+  bump: major
+- name: frame-system
+  bump: major

substrate/frame/system/src/extensions/check_non_zero_sender.rs

@@ -86,7 +86,7 @@ mod tests {
 	use crate::mock::{new_test_ext, Test, CALL};
 	use frame_support::{assert_ok, dispatch::DispatchInfo};
 	use sp_runtime::{
-		traits::{AsTransactionAuthorizedOrigin, DispatchTransaction},
+		traits::{AsTransactionAuthorizedOrigin, DispatchTransaction, TxBaseImplication},
 		transaction_validity::{TransactionSource::External, TransactionValidityError},
 	};
 
@@ -118,7 +118,7 @@ mod tests {
 			let info = DispatchInfo::default();
 			let len = 0_usize;
 			let (_, _, origin) = CheckNonZeroSender::<Test>::new()
-				.validate(None.into(), CALL, &info, len, (), CALL, External)
+				.validate(None.into(), CALL, &info, len, (), &TxBaseImplication(CALL), External)
 				.unwrap();
 			assert!(!origin.is_transaction_authorized());
 		})

substrate/frame/system/src/extensions/check_nonce.rs

@@ -186,7 +186,7 @@ mod tests {
 		assert_ok, assert_storage_noop, dispatch::GetDispatchInfo, traits::OriginTrait,
 	};
 	use sp_runtime::{
-		traits::{AsTransactionAuthorizedOrigin, DispatchTransaction},
+		traits::{AsTransactionAuthorizedOrigin, DispatchTransaction, TxBaseImplication},
 		transaction_validity::TransactionSource::External,
 	};
 
@@ -335,7 +335,7 @@ mod tests {
 			let info = DispatchInfo::default();
 			let len = 0_usize;
 			let (_, val, origin) = CheckNonce::<Test>(1u64.into())
-				.validate(None.into(), CALL, &info, len, (), CALL, External)
+				.validate(None.into(), CALL, &info, len, (), &TxBaseImplication(CALL), External)
 				.unwrap();
 			assert!(!origin.is_transaction_authorized());
 			assert_ok!(CheckNonce::<Test>(1u64.into()).prepare(val, &origin, CALL, &info, len));
@@ -359,7 +359,7 @@ mod tests {
 			let len = 0_usize;
 			// run the validation step
 			let (_, val, origin) = CheckNonce::<Test>(1u64.into())
-				.validate(Some(1).into(), CALL, &info, len, (), CALL, External)
+				.validate(Some(1).into(), CALL, &info, len, (), &TxBaseImplication(CALL), External)
 				.unwrap();
 			// mutate `AccountData` for the caller
 			crate::Account::<Test>::mutate(1, |info| {

substrate/frame/transaction-payment/skip-feeless-payment/src/lib.rs

@@ -46,7 +46,8 @@ use frame_support::{
 use scale_info::{StaticTypeInfo, TypeInfo};
 use sp_runtime::{
 	traits::{
-		DispatchInfoOf, DispatchOriginOf, PostDispatchInfoOf, TransactionExtension, ValidateResult,
+		DispatchInfoOf, DispatchOriginOf, Implication, PostDispatchInfoOf, TransactionExtension,
+		ValidateResult,
 	},
 	transaction_validity::TransactionValidityError,
 };
@@ -147,7 +148,7 @@ where
 		info: &DispatchInfoOf<T::RuntimeCall>,
 		len: usize,
 		self_implicit: S::Implicit,
-		inherited_implication: &impl Encode,
+		inherited_implication: &impl Implication,
 		source: TransactionSource,
 	) -> ValidateResult<Self::Val, T::RuntimeCall> {
 		if call.is_feeless(&origin) {

substrate/primitives/runtime/src/traits/mod.rs

@@ -55,7 +55,8 @@ use std::str::FromStr;
 
 pub mod transaction_extension;
 pub use transaction_extension::{
-	DispatchTransaction, TransactionExtension, TransactionExtensionMetadata, ValidateResult,
+	DispatchTransaction, Implication, ImplicationParts, TransactionExtension,
+	TransactionExtensionMetadata, TxBaseImplication, ValidateResult,
 };
 
 /// A lazy value.

substrate/primitives/runtime/src/traits/transaction_extension/dispatch_transaction.rs

@@ -111,7 +111,7 @@ where
 			info,
 			len,
 			self.implicit()?,
-			&(extension_version, call),
+			&TxBaseImplication((extension_version, call)),
 			source,
 		) {
 			// After validation, some origin must have been authorized.

substrate/primitives/runtime/src/traits/transaction_extension/mod.rs

@@ -43,6 +43,59 @@ mod dispatch_transaction;
 pub use as_transaction_extension::AsTransactionExtension;
 pub use dispatch_transaction::DispatchTransaction;
 
+/// The base implication in a transaction.
+///
+/// This struct is used to represent the base implication in the transaction, that is
+/// the implication not part of any transaction extensions. It usually comprises of the call and
+/// the transaction extension version.
+///
+/// The concept of implication in the transaction extension pipeline is explained in the trait
+/// documentation: [`TransactionExtension`].
+#[derive(Encode)]
+pub struct TxBaseImplication<T>(pub T);
+
+impl<T: Encode> Implication for TxBaseImplication<T> {
+	fn parts(&self) -> ImplicationParts<&impl Encode, &impl Encode, &impl Encode> {
+		ImplicationParts { base: self, explicit: &(), implicit: &() }
+	}
+}
+
+/// The implication in a transaction.
+///
+/// The concept of implication in the transaction extension pipeline is explained in the trait
+/// documentation: [`TransactionExtension`].
+#[derive(Encode)]
+pub struct ImplicationParts<Base, Explicit, Implicit> {
+	/// The base implication, that is implication not part of any transaction extension, usually
+	/// the call and the transaction extension version,
+	pub base: Base,
+	/// The explicit implication in transaction extensions,
+	pub explicit: Explicit,
+	/// The implicit implication in transaction extensions.
+	pub implicit: Implicit,
+}
+
+impl<Base: Encode, Explicit: Encode, Implicit: Encode> Implication
+	for ImplicationParts<Base, Explicit, Implicit>
+{
+	fn parts(&self) -> ImplicationParts<&impl Encode, &impl Encode, &impl Encode> {
+		ImplicationParts { base: &self.base, explicit: &self.explicit, implicit: &self.implicit }
+	}
+}
+
+/// Interface of implications in the transaction extension pipeline.
+///
+/// Implications can be encoded, this is useful for checking signature on the implications.
+/// Implications can be split into parts, this allow to destructure and restructure the
+/// implications, this is useful for nested pipeline.
+///
+/// The concept of implication in the transaction extension pipeline is explained in the trait
+/// documentation: [`TransactionExtension`].
+pub trait Implication: Encode {
+	/// Destructure the implication into its parts.
+	fn parts(&self) -> ImplicationParts<&impl Encode, &impl Encode, &impl Encode>;
+}
+
 /// Shortcut for the result value of the `validate` function.
 pub type ValidateResult<Val, Call> =
 	Result<(ValidTransaction, Val, DispatchOriginOf<Call>), TransactionValidityError>;
@@ -244,7 +297,7 @@ pub trait TransactionExtension<Call: Dispatchable>:
 		info: &DispatchInfoOf<Call>,
 		len: usize,
 		self_implicit: Self::Implicit,
-		inherited_implication: &impl Encode,
+		inherited_implication: &impl Implication,
 		source: TransactionSource,
 	) -> ValidateResult<Self::Val, Call>;
 
@@ -499,7 +552,7 @@ impl<Call: Dispatchable> TransactionExtension<Call> for Tuple {
 		info: &DispatchInfoOf<Call>,
 		len: usize,
 		self_implicit: Self::Implicit,
-		inherited_implication: &impl Encode,
+		inherited_implication: &impl Implication,
 		source: TransactionSource,
 	) -> Result<
 		(ValidTransaction, Self::Val, <Call as Dispatchable>::RuntimeOrigin),
@@ -510,23 +563,20 @@ impl<Call: Dispatchable> TransactionExtension<Call> for Tuple {
 		let following_explicit_implications = for_tuples!( ( #( &self.Tuple ),* ) );
 		let following_implicit_implications = self_implicit;
 
+		let implication_parts = inherited_implication.parts();
+
 		for_tuples!(#(
 			// Implication of this pipeline element not relevant for later items, so we pop it.
 			let (_item, following_explicit_implications) = following_explicit_implications.pop_front();
 			let (item_implicit, following_implicit_implications) = following_implicit_implications.pop_front();
 			let (item_valid, item_val, origin) = {
-				let implications = (
-					// The first is the implications born of the fact we return the mutated
-					// origin.
-					inherited_implication,
-					// This is the explicitly made implication born of the fact the new origin is
-					// passed into the next items in this pipeline-tuple.
-					&following_explicit_implications,
-					// This is the implicitly made implication born of the fact the new origin is
-					// passed into the next items in this pipeline-tuple.
-					&following_implicit_implications,
-				);
-				Tuple.validate(origin, call, info, len, item_implicit, &implications, source)?
+				Tuple.validate(origin, call, info, len, item_implicit,
+					&ImplicationParts {
+						base: implication_parts.base,
+						explicit: (&following_explicit_implications, implication_parts.explicit),
+						implicit: (&following_implicit_implications, implication_parts.implicit),
+					},
+					source)?
 			};
 			let valid = valid.combine_with(item_valid);
 			let val = val.push_back(item_val);
@@ -620,7 +670,7 @@ impl<Call: Dispatchable> TransactionExtension<Call> for () {
 		_info: &DispatchInfoOf<Call>,
 		_len: usize,
 		_self_implicit: Self::Implicit,
-		_inherited_implication: &impl Encode,
+		_inherited_implication: &impl Implication,
 		_source: TransactionSource,
 	) -> Result<
 		(ValidTransaction, (), <Call as Dispatchable>::RuntimeOrigin),
@@ -639,3 +689,168 @@ impl<Call: Dispatchable> TransactionExtension<Call> for () {
 		Ok(())
 	}
 }
+
+#[cfg(test)]
+mod test {
+	use super::*;
+
+	#[test]
+	fn test_implications_on_nested_structure() {
+		use scale_info::TypeInfo;
+		use std::cell::RefCell;
+
+		#[derive(Clone, Debug, Eq, PartialEq, Encode, Decode, TypeInfo)]
+		struct MockExtension {
+			also_implicit: u8,
+			explicit: u8,
+		}
+
+		const CALL_IMPLICIT: u8 = 23;
+
+		thread_local! {
+			static COUNTER: RefCell<u8> = RefCell::new(1);
+		}
+
+		impl TransactionExtension<()> for MockExtension {
+			const IDENTIFIER: &'static str = "MockExtension";
+			type Implicit = u8;
+			fn implicit(&self) -> Result<Self::Implicit, TransactionValidityError> {
+				Ok(self.also_implicit)
+			}
+			type Val = ();
+			type Pre = ();
+			fn weight(&self, _call: &()) -> Weight {
+				Weight::zero()
+			}
+			fn prepare(
+				self,
+				_val: Self::Val,
+				_origin: &DispatchOriginOf<()>,
+				_call: &(),
+				_info: &DispatchInfoOf<()>,
+				_len: usize,
+			) -> Result<Self::Pre, TransactionValidityError> {
+				Ok(())
+			}
+			fn validate(
+				&self,
+				origin: DispatchOriginOf<()>,
+				_call: &(),
+				_info: &DispatchInfoOf<()>,
+				_len: usize,
+				self_implicit: Self::Implicit,
+				inherited_implication: &impl Implication,
+				_source: TransactionSource,
+			) -> ValidateResult<Self::Val, ()> {
+				COUNTER.with(|c| {
+					let mut counter = c.borrow_mut();
+
+					assert_eq!(self_implicit, *counter);
+					assert_eq!(
+						self,
+						&MockExtension { also_implicit: *counter, explicit: *counter + 1 }
+					);
+
+					// Implications must be call then 1 to 22 then 1 to 22 odd.
+					let mut assert_implications = Vec::new();
+					assert_implications.push(CALL_IMPLICIT);
+					for i in *counter + 2..23 {
+						assert_implications.push(i);
+					}
+					for i in *counter + 2..23 {
+						if i % 2 == 1 {
+							assert_implications.push(i);
+						}
+					}
+					assert_eq!(inherited_implication.encode(), assert_implications,);
+
+					*counter += 2;
+				});
+				Ok((ValidTransaction::default(), (), origin))
+			}
+			fn post_dispatch_details(
+				_pre: Self::Pre,
+				_info: &DispatchInfoOf<()>,
+				_post_info: &PostDispatchInfoOf<()>,
+				_len: usize,
+				_result: &DispatchResult,
+			) -> Result<Weight, TransactionValidityError> {
+				Ok(Weight::zero())
+			}
+		}
+
+		// Test for one nested structure
+
+		let ext = (
+			MockExtension { also_implicit: 1, explicit: 2 },
+			MockExtension { also_implicit: 3, explicit: 4 },
+			(
+				MockExtension { also_implicit: 5, explicit: 6 },
+				MockExtension { also_implicit: 7, explicit: 8 },
+				(
+					MockExtension { also_implicit: 9, explicit: 10 },
+					MockExtension { also_implicit: 11, explicit: 12 },
+				),
+				MockExtension { also_implicit: 13, explicit: 14 },
+				MockExtension { also_implicit: 15, explicit: 16 },
+			),
+			MockExtension { also_implicit: 17, explicit: 18 },
+			(MockExtension { also_implicit: 19, explicit: 20 },),
+			MockExtension { also_implicit: 21, explicit: 22 },
+		);
+
+		let implicit = ext.implicit().unwrap();
+
+		let res = ext
+			.validate(
+				(),
+				&(),
+				&DispatchInfoOf::<()>::default(),
+				0,
+				implicit,
+				&TxBaseImplication(CALL_IMPLICIT),
+				TransactionSource::Local,
+			)
+			.expect("valid");
+
+		assert_eq!(res.0, ValidTransaction::default());
+
+		// Test for another nested structure
+
+		COUNTER.with(|c| {
+			*c.borrow_mut() = 1;
+		});
+
+		let ext = (
+			MockExtension { also_implicit: 1, explicit: 2 },
+			MockExtension { also_implicit: 3, explicit: 4 },
+			MockExtension { also_implicit: 5, explicit: 6 },
+			MockExtension { also_implicit: 7, explicit: 8 },
+			MockExtension { also_implicit: 9, explicit: 10 },
+			MockExtension { also_implicit: 11, explicit: 12 },
+			(
+				MockExtension { also_implicit: 13, explicit: 14 },
+				MockExtension { also_implicit: 15, explicit: 16 },
+				MockExtension { also_implicit: 17, explicit: 18 },
+				MockExtension { also_implicit: 19, explicit: 20 },
+				MockExtension { also_implicit: 21, explicit: 22 },
+			),
+		);
+
+		let implicit = ext.implicit().unwrap();
+
+		let res = ext
+			.validate(
+				(),
+				&(),
+				&DispatchInfoOf::<()>::default(),
+				0,
+				implicit,
+				&TxBaseImplication(CALL_IMPLICIT),
+				TransactionSource::Local,
+			)
+			.expect("valid");
+
+		assert_eq!(res.0, ValidTransaction::default());
+	}
+}