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

Fix self-updating verification key #812

Merged
merged 9 commits into from
Mar 28, 2023
Merged

Fix self-updating verification key #812

merged 9 commits into from
Mar 28, 2023

Conversation

mitschabaude
Copy link
Member

@mitschabaude mitschabaude commented Mar 27, 2023

fixes #813 - updating of the zkapp verification key
there was a bug where the update was using the new (not old) verification key hash inside body.authorizationKind

Copy link
Contributor

@MartinMinkov MartinMinkov left a comment

Choose a reason for hiding this comment

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

I tested the script, runs excellently. LGTM :D

let isProver = proverData !== undefined;
if (!isProver && priorAccountUpdates === undefined) {
throw Error(
'invariant violation: only the prover can call `setProofAuthorizationKind()` without passing in `priorAccountUpdates`'
Copy link
Contributor

Choose a reason for hiding this comment

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

How can the user run into this error? I tried to reproduce it but I don't quite understand how to trigger it 😅

setProofAuthorizationKind seems only to happen when we analyze methods. Could we come up with a better error message that gives guidance on what the potential issues could be? The error message has some internal parameters used, which could be confusing without knowing the code of SnarkyJS.

priorAccountUpdates is confusing since that mostly seems like an internal thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

The error is not supposed to ever happen :D I just added this as an 'assert' to make sure that we notice when the assumptions here are violated

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! Do you think it would be of any value to indicate that this error should never happen and means something is really broken in a more obvious way?

Maybe decreasing unknown error fatigue would be nice, and give a message that lets users know that it's not the fault of their zkApp?

CHANGELOG.md Outdated Show resolved Hide resolved
@mitschabaude mitschabaude merged commit a16d77d into main Mar 28, 2023
@mitschabaude mitschabaude deleted the fix/update-self-vk branch March 28, 2023 10:24
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.

Wrong verification key hash when updating the verification key from within a zkApp method
3 participants