-
Notifications
You must be signed in to change notification settings - Fork 548
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
Verification key hashes in account updates #12380
Merged
Merged
Changes from 2 commits
Commits
Show all changes
78 commits
Select commit
Hold shift + click to select a range
9aca861
Verification key hash in account updates
psteckler 930fc49
fix builds
psteckler 78ff40b
Address some PR comments
psteckler 88cae81
move vk hash to Authorization_kind
psteckler 16ee054
revert accidental commit
psteckler 98ec9da
fix replayer build
psteckler 632852e
fix builds
psteckler c017a06
more build fixes
psteckler 7e67961
fix schema
psteckler 708908e
fix table name
psteckler 1068e0d
revert
psteckler 277ad0a
Scaffold for verification_key table
bkase ec9ff1f
Adds libiconv dep
bkase f175230
Adds other changes from patch
bkase 47fb7eb
Runs nixfmt on code
bkase 4797c8d
Scaffold for verification_key table
bkase d3bd47f
rm unused binding
psteckler 8392da4
Rebases and fixes up errors on top of vk in account updates
bkase c28cbdf
Merge remote-tracking branch 'origin/verification-key-hash-table' int…
bkase 827d49a
Reformats changes
bkase cf0304a
fix mina-signer test
mitschabaude 6821744
fix to_input for authorization kind
mitschabaude 8f39329
bump snarkyjs
mitschabaude 0de763a
Merge branch 'develop' into feature/vk-hash-in-acct-updates
mitschabaude 5dde85a
bump snarkyjs
mitschabaude 9bb920d
bump snarkyjs
mitschabaude 5b44790
add record ellipsis
psteckler 6797692
update txn hash test
psteckler 543064c
replace vk hashes in tests
psteckler cdf3be8
Merge branch 'develop' into feature/vk-hash-in-acct-updates
psteckler 28e20b6
fix snarkyjs intg test
mitschabaude f7ea9de
bump snarkyjs
mitschabaude a24a51b
Merge remote-tracking branch 'origin/feature/vk-hash-in-acct-updates'…
bkase 8c4a2d7
Makes requested changes
bkase 1b0f589
Double check vk_hash in find_vk_via_ledger
bkase 9208ff6
Makes to_valid also use find_vk
bkase 4226ce8
Moves extract_vks to relevant locations
bkase 25bdc37
Lifts vk lookup fallback logic from pool to reusable
bkase 0991145
Reassigns vk-hashes before verifiabling them
bkase 6504944
WIP on fix
bkase 4d1aab1
Check vk hashes in apply
psteckler a1ca141
fix tests
psteckler 70a59c6
uncomment test
psteckler 81288a1
Removes dead logic in staged ledger, closer to correct logic in pools
bkase 1876c6d
Restore Proof auth in create tokens test
psteckler f59c1b0
Also replaces setting vks
bkase 18a4275
use Local_state.add_check
psteckler 1a4ca27
Merge branch 'verification-key-hash-table' into feature/check-vk-hash…
psteckler ddafda9
Merge pull request #12498 from MinaProtocol/feature/check-vk-hashes-i…
psteckler 06e3100
Merge remote-tracking branch 'origin/verification-key-hash-table' int…
bkase 83efa90
Requested changes
bkase 9164cda
Removed test that is no longer valid
bkase 164b104
Revert "Removed test that is no longer valid"
bkase 3a245de
Temporarily comments out staged ledger test
bkase 3ddb7ac
Merge pull request #12398 from MinaProtocol/verification-key-hash-table
bkase e34a1c1
Merge remote-tracking branch 'origin/develop' into feature/vk-hash-in…
bkase b2178ab
Underscores unused var
bkase 0e90770
Uses call_type not caller
bkase 3aea534
Param for proof
bkase eb36df5
Merge branch 'develop' into feature/vk-hash-in-acct-updates
mitschabaude 3111f34
fixup
mitschabaude 0226cb0
Fix test compilation errors
nholland94 b232b81
Merge branch 'develop' into feature/vk-hash-in-acct-updates
mitschabaude 2b9b51a
Merge branch 'feature/vk-hash-in-acct-updates' of github.com:MinaProt…
mitschabaude 56e035f
regenerate graphql json
mitschabaude 08e8b53
bump snarkyjs
mitschabaude 2ea13cc
Merge remote-tracking branch 'origin/develop' into feature/vk-hash-in…
nholland94 350c754
bump snarkyjs
mitschabaude 47b052c
Merge remote-tracking branch 'origin/develop' into feature/vk-hash-in…
bkase 1a57eee
Wrong submodule
bkase 3b65aad
Merge remote-tracking branch 'origin/develop' into feature/vk-hash-in…
bkase 737ea0b
Uses newer arg form
bkase 34a6fe6
Fixes error
bkase be8706f
Fixes find_vk usage
bkase 56f2283
Address review comments
nholland94 6d75d16
bump snarkyjs
mitschabaude 91e7722
bump snarkyjs
mitschabaude cba24b6
Fix unit tests
nholland94 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,15 @@ | ||
module Proof_with_vk_hash = struct | ||
module V1 = struct | ||
type ('proof, 'hash) t = { proof : 'proof; verification_key_hash : 'hash } | ||
end | ||
end | ||
|
||
module V2 = struct | ||
type t = | ||
| Proof of Pickles.Side_loaded.Proof.V2.t | ||
| Proof of | ||
( Pickles.Side_loaded.Proof.V2.t | ||
, Snark_params.Tick.Field.t ) | ||
Proof_with_vk_hash.V1.t | ||
| Signature of Mina_base_signature.V1.t | ||
| None_given | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ | |
mina_numbers | ||
one_or_two | ||
pickles | ||
random_oracle | ||
signature_lib | ||
sgn | ||
snark_params | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be actual hash, right? (like other account update fields)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal of replacing the authorization with a dummy is so we can calculate the transaction hash from the data in the archive db, which does not include proofs or signatures. Could we not use a dummy for the vk hash part of the authorization?
In the replayer, we could get the vk for the account from the ledger it maintains, unless the vk is being set in a previous account update in the containing zkApp, and calculate its hash. If it's being set, the replayer could track those changes.
Suppose we want to calculate the transaction hash, just by looking at the
zkapp_account_update
table, and tables reachable from it, and we don't have the replayer machinery available. I think it makes sense to use the dummy vk hash here.The vk is not part of the account update that's part of the zkApp being hashed. There may be a vk update, in which case, there's a previous vk being used for the proof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vk hash is part of the account update, right? It is in the authorization field which is part of an account update. We need to include the vk hash in transaction commitment and by that extension I think we should include it in this hash as well
Also, it should be in the archive database as well (I'm thinking of it as an enforced precondition). For the replayer, we don't need to verify the proof but when we apply transactions from the database, the vk hash in the account update needs to match the vk hash in the account (that check will be added to
Zkapp_command_logic
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in the PR, the vk hash is part of the authorization part of the account update.
If instead we place it as a separate field, and not in the authorization, then there's no need for the dummy hash. In that case, it makes sense to add a column to the account update body table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it is needed only for proof updates. maybe it it could part of
authorization_kind
then? In the archive DB a nullable field inzkapp_account_update_body
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can move it to the
Proof
case ofAuthorization_kind.t
(we may need to add it toControl.Tag.t
so they still match).In the db, we can add a new column for the vk hash, which is not NULL only when the kind is
Proof
, or change the authorization kind column to a string.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New column seems better to me