From b8cc9b8be12af52ad37617f64c75f5d843dfec72 Mon Sep 17 00:00:00 2001 From: Nikhil Gupta <17176722+gupnik@users.noreply.github.com> Date: Wed, 17 Apr 2024 10:25:56 +0530 Subject: [PATCH] Fixes nonce issue with feeless calls --- .../system/src/extensions/check_nonce.rs | 123 ++++++++++++++++-- substrate/frame/system/src/mock.rs | 29 +++++ 2 files changed, 138 insertions(+), 14 deletions(-) diff --git a/substrate/frame/system/src/extensions/check_nonce.rs b/substrate/frame/system/src/extensions/check_nonce.rs index 7504a814aef1..a92447056ab4 100644 --- a/substrate/frame/system/src/extensions/check_nonce.rs +++ b/substrate/frame/system/src/extensions/check_nonce.rs @@ -17,7 +17,10 @@ use crate::Config; use codec::{Decode, Encode}; -use frame_support::dispatch::DispatchInfo; +use frame_support::{ + dispatch::{CheckIfFeeless, DispatchInfo}, + traits::OriginTrait, +}; use scale_info::TypeInfo; use sp_runtime::{ traits::{DispatchInfoOf, Dispatchable, One, SignedExtension, Zero}, @@ -60,12 +63,16 @@ impl sp_std::fmt::Debug for CheckNonce { impl SignedExtension for CheckNonce where - T::RuntimeCall: Dispatchable, + T::RuntimeCall: Dispatchable + + CheckIfFeeless>, { type AccountId = T::AccountId; type Call = T::RuntimeCall; type AdditionalSigned = (); - type Pre = (); + type Pre = ( + Self::AccountId, // who + bool, // is feeless with zero nonce + ); const IDENTIFIER: &'static str = "CheckNonce"; fn additional_signed(&self) -> sp_std::result::Result<(), TransactionValidityError> { @@ -75,14 +82,19 @@ where fn pre_dispatch( self, who: &Self::AccountId, - _call: &Self::Call, + call: &Self::Call, _info: &DispatchInfoOf, _len: usize, - ) -> Result<(), TransactionValidityError> { + ) -> Result { let mut account = crate::Account::::get(who); + let mut is_feeless_with_zero_nonce = false; if account.providers.is_zero() && account.sufficients.is_zero() { - // Nonce storage not paid for - return Err(InvalidTransaction::Payment.into()) + if !(call.is_feeless(&T::RuntimeOrigin::signed(who.clone())) && account.nonce.is_zero()) + { + return Err(InvalidTransaction::Payment.into()) + } else { + is_feeless_with_zero_nonce = true; + } } if self.0 != account.nonce { return Err(if self.0 < account.nonce { @@ -92,22 +104,52 @@ where } .into()) } - account.nonce += T::Nonce::one(); - crate::Account::::insert(who, account); - Ok(()) + if !is_feeless_with_zero_nonce { + account.nonce += T::Nonce::one(); + crate::Account::::insert(who, account); + } + Ok((who.clone(), is_feeless_with_zero_nonce)) + } + + fn post_dispatch( + _pre: Option, + _info: &DispatchInfoOf, + _post_info: &sp_runtime::traits::PostDispatchInfoOf, + _len: usize, + _result: &sp_runtime::DispatchResult, + ) -> Result<(), TransactionValidityError> { + if let Some((who, is_feeless_with_zero_nonce)) = _pre { + if !is_feeless_with_zero_nonce { + Ok(()) + } else { + let mut account = crate::Account::::get(who.clone()); + if account.providers.is_zero() && account.sufficients.is_zero() { + return Err(InvalidTransaction::Payment.into()) + } else { + account.nonce += T::Nonce::one(); + crate::Account::::insert(who, account); + Ok(()) + } + } + } else { + Ok(()) + } } fn validate( &self, who: &Self::AccountId, - _call: &Self::Call, + call: &Self::Call, _info: &DispatchInfoOf, _len: usize, ) -> TransactionValidity { let account = crate::Account::::get(who); if account.providers.is_zero() && account.sufficients.is_zero() { - // Nonce storage not paid for - return InvalidTransaction::Payment.into() + if !(call.is_feeless(&T::RuntimeOrigin::signed(who.clone())) && account.nonce.is_zero()) + { + // Nonce storage not paid for + return InvalidTransaction::Payment.into() + } } if self.0 < account.nonce { return InvalidTransaction::Stale.into() @@ -133,7 +175,7 @@ where #[cfg(test)] mod tests { use super::*; - use crate::mock::{new_test_ext, Test, CALL}; + use crate::mock::{new_test_ext, Test, CALL, FEELESS_CALL}; use frame_support::{assert_noop, assert_ok}; #[test] @@ -214,4 +256,57 @@ mod tests { assert_ok!(CheckNonce::(1).pre_dispatch(&3, CALL, &info, len)); }) } + + #[test] + fn feeless_calls_should_work_without_account_existence() { + new_test_ext().execute_with(|| { + let info = DispatchInfo::default(); + let len = 0_usize; + assert_noop!( + CheckNonce::(0).validate(&1, CALL, &info, len), + InvalidTransaction::Payment + ); + assert_noop!( + CheckNonce::(0).pre_dispatch(&1, CALL, &info, len), + InvalidTransaction::Payment + ); + // feeless call should work without account existence + assert_ok!(CheckNonce::(0).validate(&1, FEELESS_CALL, &info, len)); + + let pre_dispatch_result = + CheckNonce::(0).pre_dispatch(&1, FEELESS_CALL, &info, len); + assert_ok!(pre_dispatch_result); + + let post_dispatch_result = CheckNonce::::post_dispatch( + Some(pre_dispatch_result.unwrap()), + &info, + &Default::default(), + len, + &Ok(()), + ); + assert_noop!(post_dispatch_result, InvalidTransaction::Payment); + + crate::Account::::insert( + 1, + crate::AccountInfo { + nonce: 0, + consumers: 0, + providers: 1, + sufficients: 0, + data: 0, + }, + ); + let post_dispatch_result = CheckNonce::::post_dispatch( + Some(pre_dispatch_result.unwrap()), + &info, + &Default::default(), + len, + &Ok(()), + ); + assert_ok!(post_dispatch_result); + + let account = crate::Account::::get(1); + assert_eq!(account.nonce, 1); + }) + } } diff --git a/substrate/frame/system/src/mock.rs b/substrate/frame/system/src/mock.rs index e1959e572e99..a5e4fac04d9f 100644 --- a/substrate/frame/system/src/mock.rs +++ b/substrate/frame/system/src/mock.rs @@ -21,10 +21,36 @@ use sp_runtime::{BuildStorage, Perbill}; type Block = mocking::MockBlock; +#[frame_support::pallet(dev_mode)] +pub mod pallet_dummy { + use super::frame_system; + use frame_support::pallet_prelude::*; + use frame_system::pallet_prelude::*; + + #[pallet::pallet] + pub struct Pallet(_); + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::call] + impl Pallet { + #[pallet::feeless_if(|_origin: &OriginFor, data: &u32| -> bool { + *data == 0 + })] + pub fn aux(_origin: OriginFor, #[pallet::compact] _data: u32) -> DispatchResult { + unreachable!() + } + } +} + +impl pallet_dummy::Config for Test {} + frame_support::construct_runtime!( pub enum Test { System: frame_system, + Dummy: pallet_dummy, } ); @@ -110,6 +136,9 @@ pub type SysEvent = frame_system::Event; pub const CALL: &::RuntimeCall = &RuntimeCall::System(frame_system::Call::set_heap_pages { pages: 0u64 }); +pub const FEELESS_CALL: &::RuntimeCall = + &RuntimeCall::Dummy(pallet_dummy::Call::aux { data: 0u32 }); + /// Create new externalities for `System` module tests. pub fn new_test_ext() -> sp_io::TestExternalities { let mut ext: sp_io::TestExternalities =