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

Option to manually specify fee payer nonce #497

Merged
merged 5 commits into from
Nov 10, 2022
Merged

Conversation

Trivo25
Copy link
Member

@Trivo25 Trivo25 commented Oct 19, 2022

Allows users to manually specify a nonce instead of having to fetch it remotely

closes #437

CHANGELOG.md Outdated Show resolved Hide resolved
src/lib/mina.ts Outdated Show resolved Hide resolved
@garethtdavies
Copy link
Contributor

garethtdavies commented Oct 20, 2022

Thanks! Managed to sign offline with this but hit a GraphQL error in testing on Berkeley, so presumably, there was another breaking change. Field \"authorizationKind\" is not defined by type AccountUpdateBodyInput.

Edit: Rebased on releases branch and works great!

@garethtdavies
Copy link
Contributor

garethtdavies commented Oct 20, 2022

Actually, while it sent it failed due to a nonce precondition being unsatisfied. This is the transaction https://berkeley.minaexplorer.com/transaction/CkpZT6ktxvrrWcn3C9iUyR7zacwm3FgouZnng7fibtsivpqP53bi3

This is the notable part (the nonce was 71):

"preconditions": {
                "account": {
                    "balance": null,
                    "delegate": null,
                    "nonce": {
                        "lower": 1,
                        "upper": 1
                    },

I'm not fetching anything, so is this cached somewhere? Even so, this also may need to be set in the preconditions to get this to work? (just to reiterate, this is this PR rebased off releases in case something else has changed since then)

@mitschabaude
Copy link
Collaborator

To solve the problem that @garethtdavies highlights, we should populate the fetch cache with the input nonce as well, using addCachedAccount

@Trivo25
Copy link
Member Author

Trivo25 commented Oct 20, 2022

@garethtdavies I'll get this fixed this evening :)

@garethtdavies
Copy link
Contributor

garethtdavies commented Oct 20, 2022

Issue here then would be, you can only specify one nonce whereas you might want a different one for fee payer and the zkApp nonce assuming they are not the same? I was hoping you might be able to get around it by manually specifying the precondition, though not sure a way to force that outside of a smart contract e.g.

zkappaccount.account.nonce.assertEquals(UInt32.from(zkappnonce + 1));

Note, that this nonce needs nonce + 1 if using the same account.

I posted in Discord #qa-task-force as I think there are issues using the same fee payer and zkapp account when batching, and making an account update that increments the nonce, as it doesn't infer the nonce correctly when one tx has been broadcast.

Here's the code I am using: https://gist.github.com/garethtdavies/552aa3c65f9d4bf4e5f9a891f6c25fbb

@mitschabaude
Copy link
Collaborator

@garethtdavies in principle, during transaction construction, we infer nonces within the transaction intelligently, so if the fee payer account appears again among the other account updates, it would automatically get the fee payer nonce + 1

@mitschabaude
Copy link
Collaborator

Also, I assume payer.account.nonce.assertEquals(UInt32.from(i+1)); would work if you put it after sign(), the way its written currently, sign overwrites the nonce you set

@garethtdavies
Copy link
Contributor

@garethtdavies in principle, during transaction construction, we infer nonces within the transaction intelligently, so if the fee payer account appears again among the other account updates, it would automatically get the fee payer nonce + 1

OK, that part is good then, but still would hit the issue that the node doesn't infer this correctly. This isn't a snarkyJS issue though, but if you send a tx with nonce 100, and the precondition for the send is 101 then the node still has an inferred nonce of 101. I'd need to send the next tx with nonce 102 but the daemon will reject it as it doesn't see the 101...

@garethtdavies
Copy link
Contributor

garethtdavies commented Oct 20, 2022

Also, I assume payer.account.nonce.assertEquals(UInt32.from(i+1)); would work if you put it after sign(), the way its written currently, sign overwrites the nonce you set

Yes, it does! Awesome, thanks so I can hack my way around this. I'm not sure adding the fee payer nonce to the preconditions is required, not least as you might want to specify a different account nonce i.e. different fee payer and sender.

@garethtdavies
Copy link
Contributor

Here's a concrete example of the issue https://gist.github.com/garethtdavies/9883c5ae1c3a25ef9a49d85730347911. The first tx sends, but then subsequent ones fail Couldn't send zkApp command: ["Invalid_nonce"] as the nonce is correctly incremented by 2 but this is not inferred correctly by the node.

@mitschabaude
Copy link
Collaborator

You're completely right @garethtdavies. In fact, we had planned since ages to make sign() in these cases not increase the nonce, but set useFullCommitment=true instead. Seems we should finally do this

@garethtdavies
Copy link
Contributor

Got it. FWIW this workaround now works https://gist.github.com/garethtdavies/22f89a82ee9c539977af331fa681aba5, so it's possible currently, not just via the same account. But I think this PR does everything it is supposed to do!

@Trivo25 Trivo25 merged commit 5bbae3c into main Nov 10, 2022
@Trivo25 Trivo25 deleted the nonce-feepayerspec branch November 10, 2022 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Allow offline signing of zkApp transactions
4 participants