Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes nonce issue with feeless calls #4165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixes nonce issue with feeless calls #4165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 109 additions & 14 deletions substrate/frame/system/src/extensions/check_nonce.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -60,12 +63,16 @@ impl<T: Config> sp_std::fmt::Debug for CheckNonce<T> {

impl<T: Config> SignedExtension for CheckNonce<T>
where
T::RuntimeCall: Dispatchable<Info = DispatchInfo>,
T::RuntimeCall: Dispatchable<Info = DispatchInfo>
+ CheckIfFeeless<Origin = crate::pallet_prelude::OriginFor<T>>,
{
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> {
Expand All @@ -75,14 +82,19 @@ where
fn pre_dispatch(
self,
who: &Self::AccountId,
_call: &Self::Call,
call: &Self::Call,
_info: &DispatchInfoOf<Self::Call>,
_len: usize,
) -> Result<(), TransactionValidityError> {
) -> Result<Self::Pre, TransactionValidityError> {
let mut account = crate::Account::<T>::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 {
Expand All @@ -92,22 +104,52 @@ where
}
.into())
}
account.nonce += T::Nonce::one();
crate::Account::<T>::insert(who, account);
Ok(())
if !is_feeless_with_zero_nonce {
account.nonce += T::Nonce::one();
crate::Account::<T>::insert(who, account);
}
Ok((who.clone(), is_feeless_with_zero_nonce))
}

fn post_dispatch(
_pre: Option<Self::Pre>,
_info: &DispatchInfoOf<Self::Call>,
_post_info: &sp_runtime::traits::PostDispatchInfoOf<Self::Call>,
_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::<T>::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::<T>::insert(who, account);
Comment on lines +129 to +130
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this not lead to accounts submitting feeless txns bloating the chain at no cost? There should be very clear docs explaining this trade off, and potential vulnerability here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this not lead to accounts submitting feeless txns bloating the chain at no cost?

Not if ED is being provided during the execution of the call. This is what is being validated in the post_dispatch.

There should be very clear docs explaining this trade off, and potential vulnerability here.

Indeed.

Open to suggestions if there is a better way to do this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to mention here https://paritytech.github.io/polkadot-sdk/master/frame_support/pallet_macros/attr.feeless_if.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to mention here https://paritytech.github.io/polkadot-sdk/master/frame_support/pallet_macros/attr.feeless_if.html

Thanks. I meant if there are alternate approaches that could be tried :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we destroy the account after the extrinsic is processed and it's no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, no transaction can be triggered at all when SkipCheckIfFeeless and CheckNonce are used at the same time, cause the check for nonce would cause all such transactions to be invalid. This just provides an additional mechanism to be able to at least trigger those calls where the endowment occurs during the execution.

Runtime can devs can definitely customise their CheckNonce extension to handle this specifically, but I am not sure if there's a downside to make it a bit easier?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, no transaction can be triggered at all when SkipCheckIfFeeless and CheckNonce are used at the same time, cause the check for nonce would cause all such transactions to be invalid.

Even transactions where the user already exists? If I have a funded account and I submit an extrinsic with a call that passes the feeless_if checks, does it not work? As I said here, the two problems are different and allowing nonexistent accounts to run signed transactions for free is difficult to solve and requires a robust solution because of the attack vector introduced. IMO the approach here is too big of a compromise.

Copy link
Contributor

@liamaharon liamaharon Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we destroy the account after the extrinsic is processed and it's no longer needed?

But the feeless call might have been used for an airdrop (based on a certain check) which could have paid for the ED. This is one of the major use cases. In that case, we don't want to destroy it.

This would bypass ED requirements, and the acc would get destroyed next ED check? :/

I think if we want to allow airdrops to new accounts, that airdrop asset needs to be sufficient otherwise we would be disabling and throwing away the point of EDs and sufficient assets which I doubt is something we should be working towards?..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this logic looks valid. I think it even valid without is_feeless condition (but without it it will fail later).
But I do not like the way it complicates the general implementation used by most of the runtime. The use case is quite specific (airdrop claims). May be we provide another implementation which handles this use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this logic looks valid. I think it even valid without is_feeless condition (but without it it will fail later). But I do not like the way it complicates the general implementation used by most of the runtime. The use case is quite specific (airdrop claims). May be we provide another implementation which handles this use case.

Yes, I was thinking to implement a wrapper similar to the SkipCheckIfFeeless extension itself such that the wrapped CheckNonce will only be called if the feeless conditions are met and the nonce is 0. This would remove the coupling here, while providing an extensible mechanism.

Ok(())
}
}
} else {
Ok(())
}
}

fn validate(
&self,
who: &Self::AccountId,
_call: &Self::Call,
call: &Self::Call,
_info: &DispatchInfoOf<Self::Call>,
_len: usize,
) -> TransactionValidity {
let account = crate::Account::<T>::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()
Expand All @@ -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]
Expand Down Expand Up @@ -214,4 +256,57 @@ mod tests {
assert_ok!(CheckNonce::<Test>(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::<Test>(0).validate(&1, CALL, &info, len),
InvalidTransaction::Payment
);
assert_noop!(
CheckNonce::<Test>(0).pre_dispatch(&1, CALL, &info, len),
InvalidTransaction::Payment
);
// feeless call should work without account existence
assert_ok!(CheckNonce::<Test>(0).validate(&1, FEELESS_CALL, &info, len));

let pre_dispatch_result =
CheckNonce::<Test>(0).pre_dispatch(&1, FEELESS_CALL, &info, len);
assert_ok!(pre_dispatch_result);

let post_dispatch_result = CheckNonce::<Test>::post_dispatch(
Some(pre_dispatch_result.unwrap()),
&info,
&Default::default(),
len,
&Ok(()),
);
assert_noop!(post_dispatch_result, InvalidTransaction::Payment);

crate::Account::<Test>::insert(
1,
crate::AccountInfo {
nonce: 0,
consumers: 0,
providers: 1,
sufficients: 0,
data: 0,
},
);
let post_dispatch_result = CheckNonce::<Test>::post_dispatch(
Some(pre_dispatch_result.unwrap()),
&info,
&Default::default(),
len,
&Ok(()),
);
assert_ok!(post_dispatch_result);

let account = crate::Account::<Test>::get(1);
assert_eq!(account.nonce, 1);
})
}
}
29 changes: 29 additions & 0 deletions substrate/frame/system/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,36 @@ use sp_runtime::{BuildStorage, Perbill};

type Block = mocking::MockBlock<Test>;

#[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<T>(_);

#[pallet::config]
pub trait Config: frame_system::Config {}

#[pallet::call]
impl<T: Config> Pallet<T> {
#[pallet::feeless_if(|_origin: &OriginFor<T>, data: &u32| -> bool {
*data == 0
})]
pub fn aux(_origin: OriginFor<T>, #[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,
}
);

Expand Down Expand Up @@ -110,6 +136,9 @@ pub type SysEvent = frame_system::Event<Test>;
pub const CALL: &<Test as Config>::RuntimeCall =
&RuntimeCall::System(frame_system::Call::set_heap_pages { pages: 0u64 });

pub const FEELESS_CALL: &<Test as Config>::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 =
Expand Down
Loading