Fixes nonce issue with feeless calls

[gupnik]

Fixes #3991

feeless_if was introduced as a mechanism to trigger feeless calls without requiring an existence of the account. However, the requirement for sufficients/providers to be non-zero in CheckNonce makes it impossible to do so.

This PR removes this check for an account with 0 nonce if the ED has been provided as a result of the execution. This is validated during post_dispatch.

NOTE: This check in post_dispatch should NEVER fail, otherwise this will lead to wasted work by the block producers.

[liamaharon]

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.

[gupnik]

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.

[liamaharon]

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

[gupnik]

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 :)

[liamaharon]

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

[gupnik]

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?

[georgepisaltu]

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.

[liamaharon]

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?..

[muharem]

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.

[gupnik]

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.

[georgepisaltu]

Commented on the original issue too but other arguments against doing this would be:

• this feature is a huge footgun; users can easily DOS their chain if they see this extension, assume it can make some calls free based on conditional code (which it does), and then attach this to some extrinsic they have (one that doesn't endow)
• it's bad to condition the behavior of a crucial extension like CheckNonce based on what an optional extension might or might not do; extensions are in a pipeline and are independent for a reason
• we introduce feeless logic into an extension that is supposed to check nonces to guard against replay attacks; this is a "separation of concerns" problem as one should have nothing to do with the other
• this will make more sense and be easier to implement in the context of General transactions and TransactionExtension (see the POCs for meta transactions here and here)

[gupnik]

Replied on the issue as well, but for these points specifically:

it's bad to condition the behavior of a crucial extension like CheckNonce based on what an optional extension might or might not do; extensions are in a pipeline and are independent for a reason

I think there's a confusion between pallet::feeless_if macro and SkipCheckIfFeeless extension. The former marks a call in your pallet as feeless while the latter uses this information to skip the transaction payment. The change here only uses the former and does not rely on the presence of the latter.

we introduce feeless logic into an extension that is supposed to check nonces to guard against replay attacks; this is a "separation of concerns" problem as one should have nothing to do with the other

Same answer as above

this feature is a huge footgun; users can easily DOS their chain if they see this extension, assume it can make some calls free based on conditional code (which it does), and then attach this to some extrinsic they have (one that doesn't endow)

Indeed, but I was hoping that this could be solved by docs. Happy to hear other suggestions as well.

this will make more sense and be easier to implement in the context of General transactions and TransactionExtension (see the POCs for meta transactions #3712 and #4122)

I would explore this further to see how a POC with the transaction extension would like. Thanks.

[georgepisaltu]

Again, the problems are separate and I'd like to focus only on the first one. If this PR is meant to target the problem of allowing nonexistent accounts to run signed transactions for free, then I am against the approach and I favor what is described in the meta transaction RFC as solution 2. Initial issue for reference #266.

I was hoping that this could be solved by docs

I don't think docs can realistically cover this corner case given the amount of hidden complexity pertaining not only to this PR, but to the entire extension logic.

Happy to hear other suggestions as well

Please refer to #4123.

I would explore this further to see how a POC with the transaction extension would like.

The linked POCs above do the first part of relaying a transaction, with the mechanism for providing for a new account before relaying described in the RFC, it's just an idea right now but it would be a provider pallet based on deposits, or the relayer can pay for the ED. Unfortunately, I'm too short on time to come up with a POC for it in the timeframe of this PR.