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.

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

CheckNonce should not enforce the account creation requirement for feeless calls #3991

Open
gupnik opened this issue Apr 5, 2024 · 7 comments · May be fixed by #4165
Open

CheckNonce should not enforce the account creation requirement for feeless calls #3991

gupnik opened this issue Apr 5, 2024 · 7 comments · May be fixed by #4165
Assignees
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@gupnik
Copy link
Contributor

gupnik commented Apr 5, 2024

As raised in https://substrate.stackexchange.com/questions/11259/how-to-enable-free-execution-for-specific-extrinsics-using-the-skipcheckiffeeles/11262#11262, SkipCheckIfFeeless, though skips the payment extensions, doesn't work as intended due to these requirements in CheckNonce:

if account.providers.is_zero() && account.sufficients.is_zero() {
// Nonce storage not paid for
return Err(InvalidTransaction::Payment.into())
}

So, we need a mechanism for CheckNonce to skip these checks for feeless calls.

@gupnik gupnik added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Apr 5, 2024
@gupnik gupnik self-assigned this Apr 5, 2024
@bkchr
Copy link
Member

bkchr commented Apr 5, 2024

So, we need a mechanism for CheckNonce to skip these checks for feeless calls.

The fix can not just be to allow this for feeless calls, because this would mean that people can store the nonce for free on chain.

When the account doesn't has any ED, we could only accept nonce zero. However, this would mean that if they don't get some ED at some point later, you could always replay the transactions.

@gupnik
Copy link
Contributor Author

gupnik commented Apr 5, 2024

When the account doesn't has any ED, we could only accept nonce zero. However, this would mean that if they don't get some ED at some point later, you could always replay the transactions.

We can add this check in the post_dispatch to ensure that the account gets an ED?

@georgepisaltu
Copy link
Contributor

We can add this check in the post_dispatch to ensure that the account gets an ED?

Not really, since you can't know for sure ahead of time that ED will be provided. See the explanation for post_dispatch on both SignedExtension and TransactionExtension.

@gupnik
Copy link
Contributor Author

gupnik commented Apr 6, 2024

We can add this check in the post_dispatch to ensure that the account gets an ED?

Not really, since you can't know for sure ahead of time that ED will be provided. See the explanation for post_dispatch on both SignedExtension and TransactionExtension.

I understand your point. However, it should be expected that the runtime dev ensures that the ED is allocated for such feeless transactions (with nonce as zero)? So, this should never happen and this error should just be a fail-safe? I am not sure if its better to not add this check, causing the transactions to be replayable?

@georgepisaltu
Copy link
Contributor

Reading the stack exchange question and the PR mentioned #4165, I think we're conflating 2 problems:

  1. having a nice way to let users not pay transaction fees in certain conditions
  2. allowing nonexistent accounts to run signed transactions for free

Problem 1 is already nicely solved by the combination of SkipCheckIfFeeless with ChargeTransactionPayment.

Problem 2 is an entirely separate problem that should not be addressed by SkipCheckIfFeeless, but by a separate mechanism. This is not a new issue, more details on approaches can be found here as well as POCs here and here.

I believe there is no issue with CheckNonce and SkipCheckIfFeeless, both are doing what they are supposed to do. The use case to be covered here, which is free calls that endow accounts, airdrops etc., should be addressed in a separate context, like the one I mentioned above.

@gupnik
Copy link
Contributor Author

gupnik commented Apr 19, 2024

Problem 2 is an entirely separate problem that should not be addressed by SkipCheckIfFeeless, but by a separate mechanism. This is #266, more details on approaches can be found #4123 as well as POCs #3712 and #4122.

Thanks!.The proposed approaches are indeed quite interesting and might solve this generally. However, the PR linked here tries to solve this specifically for feeless calls when used in combination with CheckNonce extension.

I believe there is no issue with CheckNonce and SkipCheckIfFeeless, both are doing what they are supposed to do.

Please note that the linked PR is not causing any change in the exisiting SkipCheckIfFeeless extension. Rather, it's providing an option to avoid the nonce check if a feeless call specified by #[pallet::feeless_if] (not necessarily used in tandem with SkipCheckIfFeeless) endows the ED during the execution. I am not sure why this should not be handled by the nonce check extension.

@georgepisaltu
Copy link
Contributor

However, the PR linked here tries to solve this specifically for feeless calls when used in combination with CheckNonce extension.

Reiterating, I think problem 2 is very difficult to solve because of the attack vector and I view a CheckNonce solution as not much of a solution, just a workaround and a way to shift the burden of this extra piece of logic which we don't yet have onto a crucial extension that already does its job well.

Rather, it's providing an option to avoid the nonce check if a feeless call specified by #[pallet::feeless_if] (not necessarily used in tandem with SkipCheckIfFeeless) endows the ED during the execution.

The fact that you can't know in advance if the ED will be provided and the validation logic happens before dispatch is exactly why this problem is so difficult to solve in the context of an extension and is best handled somewhere else (detailed on the linked PR). The extension contract is just not built to handle this usecase.

I am not sure why this should not be handled by the nonce check extension.

In addition to what is mentioned on this issue, and to the reasons here (and subsequent explanations):

  • There is a clear, robust solution to this problem detailed in Vision: Meta Transaction #266 and RFC: Meta Transaction Implementation #4123
  • The solution introduces so much complexity to feeless_if that it's not worth the benefit of solving this problem, especially when we have another friendlier solution in mind
  • My understanding is feeless_if works for existing accounts, which serves its purpose nicely
  • I don't see anything wrong with CheckNonce checking the nonce and rejecting an unpaid nonce at validation (also I don't think adding this responsibility to it is right); if you want to skip the nonce check, wrap it in a SkipCheckIfFeeless?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants