Verification key hashes in account updates #12380
Conversation
|
!ci-build-me |
|
!ci-build-me |
| Control.Proof Proof.transaction_dummy | ||
| Control.Proof | ||
| { proof = Proof.transaction_dummy | ||
| ; verification_key_hash = Zkapp_account.dummy_vk_hash () |
There was a problem hiding this comment.
This should be actual hash, right? (like other account update fields)
There was a problem hiding this comment.
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.
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.
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.
But it is needed only for proof updates. maybe it it could part of authorization_kind then? In the archive DB a nullable field in zkapp_account_update_body?
There was a problem hiding this comment.
Yes, we can move it to the Proof case of Authorization_kind.t (we may need to add it to Control.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.
New column seems better to me
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
1 similar comment
|
!ci-build-me |
…ocol/mina into feature/vk-hash-in-acct-updates
9b73ac2 to
2ea13cc
Compare
|
The SnarkyJS part is good to go! |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
| end) = | ||
| struct | ||
| let create_all (cmds : T.t list) | ||
| ~(find_vk : |
There was a problem hiding this comment.
Calling find_vk like this one at a time is going to be inefficient, since each invocation will fallback to the db ledger if the vk was not modified in any of the frontier masks. I fixed a number of issues on mainnet this year related to not batching db operations, and code like this could reintroduce similar performance issues if we need to lookup a lot of vks. We should consider fixing this now by just rewriting find_vk to work on batches.
There was a problem hiding this comment.
I've decided to implement this in a separate PR. The work for this is already tracked in this prior issue #11705.
|
!ci-build-me |
Add verification key hashes to account updates with a
Proofauthorization, to know which verification key is meant to verify the accompanying proof.Closes #12378.
Closes #12392
Closes #12122