From b5a5ac4487890046d226bedb0238eaccb423ae42 Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Sat, 4 Jan 2025 11:03:30 +0900 Subject: [PATCH] Make `TransactionExtension` tuple of tuple transparent for implication (#7028) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently `(A, B, C)` and `((A, B), C)` change the order of implications in the transaction extension pipeline. This order is not accessible in the metadata, because the metadata is just a vector of transaction extension, the nested structure is not visible. This PR make the implementation for tuple of `TransactionExtension` better for tuple of tuple. `(A, B, C)` and `((A, B), C)` don't change the implication for the validation A. This is a breaking change but only when using the trait `TransactionExtension` the code implementing the trait is not breaking (surprising rust behavior but fine). --------- Co-authored-by: command-bot <> Co-authored-by: Bastian Köcher --- prdoc/pr_7028.prdoc | 25 ++ .../src/extensions/check_non_zero_sender.rs | 4 +- .../system/src/extensions/check_nonce.rs | 6 +- .../skip-feeless-payment/src/lib.rs | 5 +- .../primitives/runtime/src/traits/mod.rs | 3 +- .../dispatch_transaction.rs | 2 +- .../src/traits/transaction_extension/mod.rs | 258 +++++++++++++++++- 7 files changed, 279 insertions(+), 24 deletions(-) create mode 100644 prdoc/pr_7028.prdoc diff --git a/prdoc/pr_7028.prdoc b/prdoc/pr_7028.prdoc new file mode 100644 index 000000000000..ead918fc2e00 --- /dev/null +++ b/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 diff --git a/substrate/frame/system/src/extensions/check_non_zero_sender.rs b/substrate/frame/system/src/extensions/check_non_zero_sender.rs index 577e2b324fca..978eebaf3dac 100644 --- a/substrate/frame/system/src/extensions/check_non_zero_sender.rs +++ b/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::::new() - .validate(None.into(), CALL, &info, len, (), CALL, External) + .validate(None.into(), CALL, &info, len, (), &TxBaseImplication(CALL), External) .unwrap(); assert!(!origin.is_transaction_authorized()); }) diff --git a/substrate/frame/system/src/extensions/check_nonce.rs b/substrate/frame/system/src/extensions/check_nonce.rs index 004ec08a26f2..bc19a09e06a9 100644 --- a/substrate/frame/system/src/extensions/check_nonce.rs +++ b/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::(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::(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::(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::::mutate(1, |info| { diff --git a/substrate/frame/transaction-payment/skip-feeless-payment/src/lib.rs b/substrate/frame/transaction-payment/skip-feeless-payment/src/lib.rs index dd907f6fcbb7..5ba1d1297679 100644 --- a/substrate/frame/transaction-payment/skip-feeless-payment/src/lib.rs +++ b/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, len: usize, self_implicit: S::Implicit, - inherited_implication: &impl Encode, + inherited_implication: &impl Implication, source: TransactionSource, ) -> ValidateResult { if call.is_feeless(&origin) { diff --git a/substrate/primitives/runtime/src/traits/mod.rs b/substrate/primitives/runtime/src/traits/mod.rs index cfcc3e5a354d..d371152dc40a 100644 --- a/substrate/primitives/runtime/src/traits/mod.rs +++ b/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. diff --git a/substrate/primitives/runtime/src/traits/transaction_extension/dispatch_transaction.rs b/substrate/primitives/runtime/src/traits/transaction_extension/dispatch_transaction.rs index 28030d12fc9f..1fbaab0d45ac 100644 --- a/substrate/primitives/runtime/src/traits/transaction_extension/dispatch_transaction.rs +++ b/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. diff --git a/substrate/primitives/runtime/src/traits/transaction_extension/mod.rs b/substrate/primitives/runtime/src/traits/transaction_extension/mod.rs index f8c5dc6a724e..27f33acb69cc 100644 --- a/substrate/primitives/runtime/src/traits/transaction_extension/mod.rs +++ b/substrate/primitives/runtime/src/traits/transaction_extension/mod.rs @@ -43,6 +43,72 @@ mod dispatch_transaction; pub use as_transaction_extension::AsTransactionExtension; pub use dispatch_transaction::DispatchTransaction; +/// Provides `Sealed` trait. +mod private { + /// Special trait that prevents the implementation of some traits outside of this crate. + pub trait Sealed {} +} + +/// 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(pub T); + +impl Implication for TxBaseImplication { + fn parts(&self) -> ImplicationParts<&impl Encode, &impl Encode, &impl Encode> { + ImplicationParts { base: self, explicit: &(), implicit: &() } + } +} + +impl private::Sealed for TxBaseImplication {} + +/// 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 { + /// 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 Implication + for ImplicationParts +{ + fn parts(&self) -> ImplicationParts<&impl Encode, &impl Encode, &impl Encode> { + ImplicationParts { base: &self.base, explicit: &self.explicit, implicit: &self.implicit } + } +} + +impl private::Sealed for ImplicationParts {} + +/// 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. +/// +/// This trait is sealed, consider using [`TxBaseImplication`] and [`ImplicationParts`] +/// implementations. +/// +/// The concept of implication in the transaction extension pipeline is explained in the trait +/// documentation: [`TransactionExtension`]. +pub trait Implication: Encode + private::Sealed { + /// 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 = Result<(ValidTransaction, Val, DispatchOriginOf), TransactionValidityError>; @@ -244,7 +310,7 @@ pub trait TransactionExtension: info: &DispatchInfoOf, len: usize, self_implicit: Self::Implicit, - inherited_implication: &impl Encode, + inherited_implication: &impl Implication, source: TransactionSource, ) -> ValidateResult; @@ -499,7 +565,7 @@ impl TransactionExtension for Tuple { info: &DispatchInfoOf, len: usize, self_implicit: Self::Implicit, - inherited_implication: &impl Encode, + inherited_implication: &impl Implication, source: TransactionSource, ) -> Result< (ValidTransaction, Self::Val, ::RuntimeOrigin), @@ -510,23 +576,20 @@ impl TransactionExtension 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 +683,7 @@ impl TransactionExtension for () { _info: &DispatchInfoOf, _len: usize, _self_implicit: Self::Implicit, - _inherited_implication: &impl Encode, + _inherited_implication: &impl Implication, _source: TransactionSource, ) -> Result< (ValidTransaction, (), ::RuntimeOrigin), @@ -639,3 +702,168 @@ impl TransactionExtension 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 = RefCell::new(1); + } + + impl TransactionExtension<()> for MockExtension { + const IDENTIFIER: &'static str = "MockExtension"; + type Implicit = u8; + fn implicit(&self) -> Result { + 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 { + Ok(()) + } + fn validate( + &self, + origin: DispatchOriginOf<()>, + _call: &(), + _info: &DispatchInfoOf<()>, + _len: usize, + self_implicit: Self::Implicit, + inherited_implication: &impl Implication, + _source: TransactionSource, + ) -> ValidateResult { + 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 { + 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()); + } +}