Verification refcounted key hash table in transaction pool #12398

bkase · 2022-12-19T23:19:43Z

Adds a verification key refcounted table in the transaction pool to track keys as they enter/exit the mempool.

Also adjusted the User_command.to_verifiable to be more flexible with its fallback rather than assume it's a ledger lookup.

Plan to test this as part of #12364

Fixes XXX

…o verification-key-hash-table

psteckler · 2022-12-19T23:23:16Z

src/lib/network_pool/transaction_pool.ml

+          | None ->
+              None
+          | Some (x, value) ->
+              if Int.equal x 1 then None else Some (x - 1, value) )


can use x = 1

deepthiskumar

the ref counts need to be updated in this function as well.

mina/src/lib/network_pool/transaction_pool.ml

Line 471 in 827d49a

let handle_transition_frontier_diff

deepthiskumar · 2022-12-20T08:41:56Z

src/lib/mina_base/zkapp_command.ml

+      zkapp.verification_key
+    with
+    | Some vk ->
+        Ok vk


should we check if the vk hash is as expected?

deepthiskumar · 2022-12-20T08:45:20Z

src/lib/mina_base/zkapp_command.ml

+                      (* we haven't set anything; lookup the vk in the fallback *)
+                      let vk_hash =
+                        failwith
+                          "TODO: Lookup the vk_hash inside this account update"


impl this? vk hash should be available in account updates

oops, thanks forgot about this

deepthiskumar · 2022-12-20T08:48:22Z

src/lib/network_pool/transaction_pool.ml

@@ -327,6 +327,29 @@ struct
        t
    end

+    module Vk_refcount_table = struct
+      (* what's the right incantation of "field" here that is better than State_hash but will compile *)


Zkapp_basic.F maybe?

deepthiskumar · 2022-12-20T09:06:41Z

src/lib/staged_ledger/staged_ledger.ml

@@ -1023,7 +1023,12 @@ module T = struct
      Or_error.try_with (fun () ->
          List.map cs ~f:(fun cmd ->
              let open Ledger in
-              User_command.to_verifiable ~ledger ~get ~location_of_account cmd
+              (* TODO: what are we supposed to do here? *)


I don't think we need to do anything here. Verifiable.create does what's required

deepthiskumar · 2022-12-20T09:08:01Z

src/lib/mina_base/zkapp_command.ml

@@ -1386,6 +1395,21 @@ end = struct
    end
  end]

+  let find_vk_via_ledger ~ledger ~get ~location_of_account _vk_hash account_id =


nit: make it a labelled arg ~expected_vk_hash?

… into verification-key-hash-table

bkase · 2022-12-21T23:19:57Z

I am very not confident that I covered the handle_transition_frontier_diff is properly instrumented with vk_refcount_table inc and dec (this is the scary part of using the ref counted table). Please review very carefully.

mrmr1993 · 2023-01-03T18:28:30Z

src/lib/staged_ledger/staged_ledger.ml

-              User_command.to_verifiable ~ledger ~get ~location_of_account cmd
+              User_command.to_verifiable
+                ~find_vk:
+                  (Zkapp_command.Verifiable.find_vk_via_ledger ~ledger ~get


Won't this get the wrong vk if one zkApp command sets it and a later one attempts to use it? It feels like we need to accumulate across all of the commands.

mrmr1993 · 2023-01-03T18:31:20Z

src/lib/mina_base/zkapp_command.ml

+      zkapp.verification_key
+    with
+    | Some vk ->
+        Ok vk


mrmr1993 · 2023-01-03T18:37:06Z

src/lib/mina_base/zkapp_command.ml

@@ -1655,7 +1666,8 @@ struct
  let forget (t : t) : T.t = t.zkapp_command

  let to_valid (t : T.t) ~ledger ~get ~location_of_account : t Or_error.t =
-    Verifiable.create t ~ledger ~get ~location_of_account
+    Verifiable.create t
+      ~find_vk:(Verifiable.find_vk_via_ledger ~ledger ~get ~location_of_account)


Does it make sense to pass this in as an argument? Looking in the ledger is not always the right thing to do (although it's possible this is only used in tests..)

mrmr1993 · 2023-01-03T18:41:27Z

src/lib/network_pool/transaction_pool.ml

+                                          Ok z
+                                    in
+                                    (* first we check if there's anything in the running cache within this diff so far *)
+                                    F.Map.find running_cache vk_hash


Does it make sense to factor out (some of) this logic into a function in Zkapp_command? It seems like we probably want something similar to this in (at least) the staged ledger diff application logic too.

mrmr1993 · 2023-01-03T18:43:36Z

src/lib/network_pool/transaction_pool.ml

+      include Comparable.Make (T)
+    end
+
+    let extract_vks : User_command.t -> Verification_key_wire.t List.t =


This probably shouldn't live here. In User_command / Zkapp_command / both?

bkase · 2023-01-06T04:42:41Z

!ci-build-me

…es-in-apply

…n-apply

…o verification-key-hash-table

bkase · 2023-01-13T18:30:38Z

!ci-build-me

mrmr1993

LGTM, but I suspect the zkapps_examples are broken due to the patch (and if they're not, we're not testing them properly / something is broken!)

mrmr1993 · 2023-01-17T21:20:08Z

src/lib/network_pool/transaction_pool.ml

-              User_command.Zkapp_command valid_zkapp_command
-          | Signed_command _ ->
-              failwith "Expected Zkapp_command valid user command" )
+      let sequence_or_error (xs : 'a Or_error.t list) : 'a list Or_error.t =


Nit: Or_error.combine_errors?

mrmr1993 · 2023-01-17T21:22:57Z

src/lib/zkapps_examples/zkapps_examples.ml

-        ; caller = t.caller
+        ; caller =
+            t.caller
+            (* the vk hash is a dummy, to be patched with `patch_verification_key_hashes`, below *)


Nit: shift this comment down?

mrmr1993 · 2023-01-17T21:24:54Z

src/lib/transaction_snark/test/zkapps_examples/actions/actions.ml

-        ~ledger zkapp_command =
+        ~ledger zkapp_command0 =
+      let zkapp_command =
+        Zkapps_examples.patch_verification_key_hashes ~ledger zkapp_command0


I'm not sure that it's valid to patch at this stage.. I think you need to pass the verification key hash into the circuit while it's constructing the transaction, in the same way we do for public_key, token_id, caller, etc.

Specifically, this test checked that staged ledger diffs filter out commands that fail to check against the verification key given from the ledger. However, now that verification key hashes are part of user commands, we check _in the prediff_ that the verificaiton keys are valid.

This reverts commit 9164cda.

bkase · 2023-01-19T18:11:38Z

!ci-build-me

bkase added 8 commits December 16, 2022 22:28

Scaffold for verification_key table

277ad0a

Adds libiconv dep

ec9ff1f

Adds other changes from patch

f175230

Runs nixfmt on code

47fb7eb

Scaffold for verification_key table

4797c8d

Rebases and fixes up errors on top of vk in account updates

8392da4

Merge remote-tracking branch 'origin/verification-key-hash-table' int…

c28cbdf

…o verification-key-hash-table

Reformats changes

827d49a

bkase requested a review from a team as a code owner December 19, 2022 23:19

psteckler reviewed Dec 19, 2022

View reviewed changes

deepthiskumar requested changes Dec 20, 2022

View reviewed changes

bkase added 2 commits December 21, 2022 16:44

Merge remote-tracking branch 'origin/feature/vk-hash-in-acct-updates'…

a24a51b

… into verification-key-hash-table

Makes requested changes

8c4a2d7

mrmr1993 reviewed Jan 3, 2023

View reviewed changes

bkase added 5 commits January 4, 2023 06:32

Double check vk_hash in find_vk_via_ledger

1b0f589

Makes to_valid also use find_vk

9208ff6

Moves extract_vks to relevant locations

4226ce8

Lifts vk lookup fallback logic from pool to reusable

25bdc37

Reassigns vk-hashes before verifiabling them

0991145

bkase mentioned this pull request Jan 6, 2023

Transaction pool rejects transaction which sets and immediately uses the verification key #12109

Closed

bkase and others added 8 commits January 11, 2023 17:27

WIP on fix

6504944

Check vk hashes in apply

4d1aab1

fix tests

a1ca141

uncomment test

70a59c6

Removes dead logic in staged ledger, closer to correct logic in pools

81288a1

Restore Proof auth in create tokens test

1876c6d

Also replaces setting vks

f59c1b0

use Local_state.add_check

18a4275

psteckler and others added 3 commits January 12, 2023 13:53

Merge branch 'verification-key-hash-table' into feature/check-vk-hash…

1a4ca27

…es-in-apply

Merge pull request #12498 from MinaProtocol/feature/check-vk-hashes-i…

ddafda9

…n-apply

Merge remote-tracking branch 'origin/verification-key-hash-table' int…

06e3100

…o verification-key-hash-table

mrmr1993 approved these changes Jan 17, 2023

View reviewed changes

bkase added 4 commits January 18, 2023 10:45

Requested changes

83efa90

Revert "Removed test that is no longer valid"

164b104

This reverts commit 9164cda.

Temporarily comments out staged ledger test

3a245de

bkase merged commit 3ddb7ac into feature/vk-hash-in-acct-updates Jan 21, 2023

bkase deleted the verification-key-hash-table branch January 21, 2023 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verification refcounted key hash table in transaction pool #12398

Verification refcounted key hash table in transaction pool #12398

bkase commented Dec 19, 2022

psteckler Dec 19, 2022

deepthiskumar left a comment

deepthiskumar Dec 20, 2022

mrmr1993 Jan 3, 2023

deepthiskumar Dec 20, 2022

bkase Dec 20, 2022

deepthiskumar Dec 20, 2022

deepthiskumar Dec 20, 2022

deepthiskumar Dec 20, 2022

bkase commented Dec 21, 2022

mrmr1993 Jan 3, 2023

mrmr1993 Jan 3, 2023

mrmr1993 Jan 3, 2023

mrmr1993 Jan 3, 2023

mrmr1993 Jan 3, 2023

bkase commented Jan 6, 2023

bkase commented Jan 13, 2023

mrmr1993 left a comment

mrmr1993 Jan 17, 2023

mrmr1993 Jan 17, 2023

mrmr1993 Jan 17, 2023

bkase commented Jan 19, 2023

Verification refcounted key hash table in transaction pool #12398

Verification refcounted key hash table in transaction pool #12398

Conversation

bkase commented Dec 19, 2022

Choose a reason for hiding this comment

deepthiskumar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkase commented Dec 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkase commented Jan 6, 2023

bkase commented Jan 13, 2023

mrmr1993 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkase commented Jan 19, 2023