Uses vks from earlier account updates if present #12248
Conversation
src/lib/mina_base/zkapp_command.ml
Outdated
| let account_id = Account_update.account_id p in | ||
| let vks_overrided' = |
There was a problem hiding this comment.
I believe it's 100% safe to always record the newly set verification key unconditionally (regardless of authorization), but would love someone to double check if I'm thinking through this properly.
There was a problem hiding this comment.
If it isn't safe, then the transaction will get rejected at that point, and we'll never end up using that vk 👍
| * If an account_update replaces the verification_key (or deletes it), | ||
| * subsequent account_updates use the replaced key instead of looking in the | ||
| * ledger for the key (ie set by a previous transaction). | ||
| *) |
There was a problem hiding this comment.
Added this here because it took me a bit to really think through all the edge cases.
There was a problem hiding this comment.
This works if there are multiple vk updates, and the most-recently-set vk is used for each update?
src/lib/mina_base/zkapp_command.ml
Outdated
| (* we haven't set anything; lookup the vk in the ledger *) | ||
| find_vk account_id | ||
| in | ||
| Option.map prioritized_vk ~f:(fun vk -> |
There was a problem hiding this comment.
Nit: let%map prioritised_vk =
src/lib/mina_base/zkapp_command.ml
Outdated
| let vks_overrided' = | ||
| match Account_update.verification_key p with | ||
| | Zkapp_basic.Set_or_keep.Set None -> | ||
| Account_id.Map.change vks_overrided account_id ~f:(fun _ -> |
There was a problem hiding this comment.
Account_id.Map.set would be clearer here
src/lib/mina_base/zkapp_command.ml
Outdated
| let account_id = Account_update.account_id p in | ||
| let vks_overrided' = | ||
| match Account_update.verification_key p with | ||
| | Zkapp_basic.Set_or_keep.Set None -> |
There was a problem hiding this comment.
This and the match branch below can be combined
src/lib/mina_base/zkapp_command.ml
Outdated
| Some (Some vk) ) | ||
| | Zkapp_basic.Set_or_keep.Keep -> | ||
| vks_overrided | ||
| in | ||
| if Control.(Tag.equal Tag.Proof (Control.tag p.authorization)) then |
There was a problem hiding this comment.
don't think you need the 2nd Control in this expression
src/lib/mina_base/account_update.ml
Outdated
| @@ -1538,6 +1538,10 @@ include T | |||
| let account_id (t : t) : Account_id.t = | |||
| Account_id.create t.body.public_key t.body.token_id | |||
|
|
|||
| let verification_key (t : t) : | |||
There was a problem hiding this comment.
the function name suggests it's pulling out the vk, but it's converting it to an option.
maybe verification_key_update_to_option ...
src/lib/mina_base/zkapp_command.ml
Outdated
| * early due to an error *) | ||
| Some | ||
| Account_id.Map.empty ) ~f:(fun acc p -> | ||
| let%bind vks_overrided = acc in |
|
!ci-build-me |
|
I'm landing as is and following up with an integration test in a separate PR |
Fixes #12122
Still needs a test