Improve zkapp cmd validity check error report

changes/17805.md

@@ -0,0 +1 @@
+Improve error messages on checking zkApp command validity. Now the validity check won't short-circuit, but will check all potential errors in all account updates before reporting it.

src/lib/mina_base/zkapp_command.ml

@@ -551,82 +551,105 @@ end = struct
    * subsequent account_updates use the replaced key instead of looking in the
    * ledger for the key (ie set by a previous transaction).
    *)
-  let create ({ fee_payer; account_updates; memo } : T.t) ~failed ~find_vk =
-    With_return.with_return (fun { return } ->
-        let tbl = Account_id.Table.create () in
-        let vks_overridden =
-          (* Keep track of the verification keys that have been set so far
-             during this transaction.
-          *)
-          ref Account_id.Map.empty
-        in
-        let account_updates =
-          Call_forest.map account_updates ~f:(fun p ->
-              let account_id = Account_update.account_id p in
-              let vks_overriden' =
-                match Account_update.verification_key_update_to_option p with
-                | Zkapp_basic.Set_or_keep.Set vk_next ->
-                    Account_id.Map.set !vks_overridden ~key:account_id
-                      ~data:vk_next
-                | Zkapp_basic.Set_or_keep.Keep ->
-                    !vks_overridden
-              in
-              let () =
-                match Account_update.check_authorization p with
-                | Ok () ->
-                    ()
-                | Error _ as err ->
-                    return err
-              in
-              match (p.body.authorization_kind, failed) with
-              | Proof vk_hash, false -> (
-                  let prioritized_vk =
-                    (* only lookup _past_ vk setting, ie exclude the new one we
-                     * potentially set in this account_update (use the non-'
-                     * vks_overrided) . *)
-                    match Account_id.Map.find !vks_overridden account_id with
-                    | Some (Some vk) -> (
-                        match
-                          ok_if_vk_hash_expected ~got:vk ~expected:vk_hash
-                        with
-                        | Ok vk ->
-                            Some vk
-                        | Error err ->
-                            return (Error err) )
-                    | Some None ->
-                        (* we explicitly have erased the key *)
-                        let err =
-                          Error.create
-                            "No verification key found for proved account \
-                             update: the verification key was removed by a \
-                             previous account update"
-                            ("account_id", account_id)
-                            [%sexp_of: string * Account_id.t]
-                        in
-                        return (Error err)
-                    | None -> (
-                        (* we haven't set anything; lookup the vk in the fallback *)
-                        match find_vk vk_hash account_id with
-                        | Error e ->
-                            return (Error e)
-                        | Ok vk ->
-                            Some vk )
-                  in
-                  match prioritized_vk with
-                  | Some prioritized_vk ->
-                      Account_id.Table.update tbl account_id ~f:(fun _ ->
-                          With_hash.hash prioritized_vk ) ;
-                      (* return the updated overrides *)
-                      vks_overridden := vks_overriden' ;
-                      (p, Some prioritized_vk)
-                  | None ->
-                      (* The transaction failed, so we allow the vk to be missing. *)
-                      (p, None) )
-              | _ ->
-                  vks_overridden := vks_overriden' ;
-                  (p, None) )
-        in
-        Ok { Poly.fee_payer; account_updates; memo } )
+  let create =
+    let dummy_vk =
+      { With_hash.data = Side_loaded_verification_key.dummy
+      ; hash = Pasta_bindings.Fp.of_int 0
+      }
+    in
+    fun ({ fee_payer; account_updates; memo } : T.t) ~failed ~find_vk ->
+      let error_messages = Queue.create () in
+      let tbl = Account_id.Table.create () in
+      let vks_overridden =
+        (* Keep track of the verification keys that have been set so far
+           during this transaction.
+        *)
+        ref Account_id.Map.empty
+      in
+      let account_updates =
+        Call_forest.mapi account_updates ~f:(fun update_idx p ->
+            let account_id = Account_update.account_id p in
+            let vks_overridden' =
+              match Account_update.verification_key_update_to_option p with
+              | Zkapp_basic.Set_or_keep.Set vk_next ->
+                  Account_id.Map.set !vks_overridden ~key:account_id
+                    ~data:vk_next
+              | Keep ->
+                  !vks_overridden
+            in
+            Account_update.check_authorization p
+            |> Result.iter_error ~f:(fun err ->
+                   Queue.enqueue error_messages
+                     (`Assoc
+                       [ ("account_id", Account_id.to_yojson account_id)
+                       ; ("update_index", `Int update_idx)
+                       ; ("error", Error_json.error_to_yojson err)
+                       ] ) ) ;
+            match (p.body.authorization_kind, failed) with
+            | Proof vk_hash, false ->
+                let prioritized_vk =
+                  (* only lookup _past_ vk setting, ie exclude the new one we
+                   * potentially set in this account_update (use the non-'
+                   * vks_overrided) . *)
+                  match Account_id.Map.find !vks_overridden account_id with
+                  | Some (Some vk) -> (
+                      match
+                        ok_if_vk_hash_expected ~got:vk ~expected:vk_hash
+                      with
+                      | Ok vk ->
+                          vk
+                      | Error err ->
+                          Queue.enqueue error_messages
+                            (`Assoc
+                              [ ("account_id", Account_id.to_yojson account_id)
+                              ; ("update_index", `Int update_idx)
+                              ; ("error", Error_json.error_to_yojson err)
+                              ] ) ;
+                          dummy_vk )
+                  | Some None ->
+                      (* we explicitly have erased the key *)
+                      Queue.enqueue error_messages
+                        (`Assoc
+                          [ ("account_id", Account_id.to_yojson account_id)
+                          ; ("update_index", `Int update_idx)
+                          ; ( "error"
+                            , `String
+                                "No verification key found for proved account \
+                                 update: the verification key was removed by a \
+                                 previous account update" )
+                          ] ) ;
+                      dummy_vk
+                  | None -> (
+                      (* we haven't set anything; lookup the vk in the fallback *)
+                      match find_vk vk_hash account_id with
+                      | Error err ->
+                          Queue.enqueue error_messages
+                            (`Assoc
+                              [ ("account_id", Account_id.to_yojson account_id)
+                              ; ("update_index", `Int update_idx)
+                              ; ("error", Error_json.error_to_yojson err)
+                              ] ) ;
+                          dummy_vk
+                      | Ok vk ->
+                          vk )
+                in
+                Account_id.Table.update tbl account_id
+                  ~f:(const @@ With_hash.hash prioritized_vk) ;
+                (* return the updated overrides *)
+                vks_overridden := vks_overridden' ;
+                (p, Some prioritized_vk)
+            | _ ->
+                vks_overridden := vks_overridden' ;
+                (p, None) )
+      in
+      match Queue.to_list error_messages with
+      | [] ->
+          Ok { Poly.fee_payer; account_updates; memo }
+      | errs ->
+          let errs_strings = List.map ~f:Yojson.Safe.to_string errs in
+          Error
+            (Error.create "Failed to create zkApp command:" errs_strings
+               [%sexp_of: string list] )
 
   module type Cache_intf = sig
     type t

src/lib/mina_lib/dune

@@ -101,5 +101,12 @@
  (instrumentation
   (backend bisect_ppx))
  (preprocess
-  (pps ppx_jane ppx_mina ppx_version ppx_inline_test ppx_deriving.std))
+  (pps 
+    ppx_jane 
+    ppx_mina
+    ppx_version
+    ppx_inline_test
+    ppx_deriving.std
+    ppx_sexp_conv
+    ppx_here))
  (synopsis "Mina gut layer"))

src/lib/mina_lib/mina_lib.ml

@@ -986,28 +986,20 @@ let add_zkapp_transactions t
     |> Deferred.don't_wait_for ;
     Ivar.read result_ivar
   in
-  let well_formed_errors =
-    List.find_map zkapp_commands ~f:(fun cmd ->
-        match
-          User_command.check_well_formedness
-            ~genesis_constants:t.config.precomputed_values.genesis_constants
-            (Zkapp_command cmd)
-        with
-        | Ok () ->
-            None
-        | Error errs ->
-            Some errs )
-  in
-  match well_formed_errors with
-  | None ->
+  let all_errs = Queue.create () in
+  List.iteri zkapp_commands ~f:(fun idx cmd ->
+      User_command.check_well_formedness
+        ~genesis_constants:t.config.precomputed_values.genesis_constants
+        (Zkapp_command cmd)
+      |> Result.iter_error ~f:(fun this_errs ->
+             Queue.enqueue all_errs (idx, this_errs) ) ) ;
+  match Queue.to_list all_errs with
+  | [] ->
       add_all_txns ()
-  | Some errs ->
-      let error =
-        Error.of_string
-          ( List.map errs ~f:User_command.Well_formedness_error.to_string
-          |> String.concat ~sep:"," )
-      in
-      Deferred.Result.fail error
+  | errs ->
+      Error.create ~here:[%here] "zkapp transaction invalid" errs
+        [%sexp_of: (int * User_command.Well_formedness_error.t list) list]
+      |> Deferred.Result.fail
 
 let next_producer_timing t = t.next_producer_timing