Cache account update action hashes in a new aux field in Account_update #17120

cjjdespres · 2025-05-01T19:31:51Z

Explain your changes:

A new aux field has been added to the Account_update types. For the stable versions of these types it holds nothing, but for the unstable versions of these types it is used to cache the hash of the actions of the body of the account update. This new field is not intended for serialization, so the yojson and sexp definitions in Account_update were modified to ensure that the serialization format for these types is unchanged.

This PR is prompted by the observation in #16968 that the --transfer-parties-get-actions-events flag in the mina ledger test apply benchmark causes the final application time to increase by roughly 7 seconds. (Benchmarking was done on my laptop). This time would also increase by a further 7 seconds for every 100 actions added to the max actions in the genesis constants. By caching the actions_hash on Account_update creation, this increase is eliminated, and the final application time is now constant with respect to the number of actions in an account update.

Explain how you tested your changes:

I ran mina ledger test apply with and without the --transfer-parties-get-actions-events flag, and with various edited values of the max actions in the genesis constants. In all cases the final application time in that benchmark was roughly 3 seconds:

With actions=events=0:

Result of application 580: true (took 3.1157171726226807s): new root jwN9rKMrRduBhpxuSGHMFShc4iY4cTuMnL1i3ghc69ygHuva6Mt

With actions=events=1000 and max_action_elements=max_event_elements=1000:

Result of application 580: true (took 3.2762835025787354s): new root jxKot9cGbGuffuVWRCYXeHKAwNuniDRRSjCiKxSp6GxBpArHRpX

Increasing the actions and events still causes the real time of the benchmark to increase, but this is independent of the number of actions and events in the other parties in the ledger - it goes from about 1m with 100 max, to 7m with 1000 max.

Checklist:

Dependency versions are unchanged
Modified the current draft of release notes with details on what is completed or incomplete within this project
Document code purpose, how to use it
Tests were added for the new behavior
- Document test purpose, significance of failures
- Test names should reflect their purpose
All tests pass (CI will check this if you didn't)
Serialized types are in stable-versioned modules

georgeee · 2025-05-01T22:23:28Z

src/lib/mina_base/user_command.ml

-    account_update_digest_r forest_digest_r )
-    (t1 : (proof_l, account_update_digest_l, forest_digest_l) with_forest)
-    (t2 : (proof_r, account_update_digest_r, forest_digest_r) with_forest) =
+    (type account_update_digest_l forest_digest_l account_update_digest_r


I'd rollback the explicit variable, as it ensured that the function is abstract in proof type too

georgeee · 2025-05-01T22:24:52Z

src/lib/mina_base/zkapp_command.ml

-    ({ fee_payer; memo; account_updates } : (_, _, _) with_forest) :
-    (unit, unit, unit) with_forest =
+    ({ fee_payer; memo; account_updates } :
+      ((_, (_, _) Control.Poly.t) Account_update.Poly.t, _, _) with_forest ) :


I think it's fine to remove type signature here

src/lib/mina_wire_types/mina_base/mina_base_account_update.ml

cjjdespres · 2025-05-02T20:09:14Z

I haven't been able to have the wire and stable types be of different shapes just yet, but I did fix the derived Binable instance for the account update types, and address the other two small comments.

cjjdespres · 2025-05-05T20:46:41Z

I've merged the branch from #17147 and added a commit to restore the old wire types version of Account_update

cjjdespres · 2025-05-20T12:54:06Z

The ledger_test_apply.sh benchmark still shows the same improvement (3s even with 100 actions and events) after these latest changes.

georgeee · 2025-05-21T12:10:14Z

!ci-build-me

georgeee · 2025-05-21T12:30:27Z

src/lib/mina_block/legacy_format.ml

-        | User_command.Poly.Signed_command tx ->
-            Helper.to_yojson ~proof_to_yojson:Proof.to_yojson (Signed_command tx)
+        | User_command.Poly.Signed_command _ as tx ->
+            Helper.to_yojson ~proof_to_yojson:Nothing.unreachable_code tx


Is the change from Proof.to_yojson necessary? Could we find a more elegant solution?
Or at least document this usage of unreachable_code.

Seeing this code makes me worry that we could have an unexpected runtime error because of it, although it feels like we shouldn't I'd consider rewriting something or leaving a comment in code to explain the "why" and "how".

This unreachable_code function takes a value of the uninhabited Never.t type. (I think that's its name). It's more like type level documentation that the proof_to_yojson argument is unused in the body of Helper.to_yojson during this call. I'll document it.

Added a comment

georgeee · 2025-05-21T12:49:01Z

src/lib/mina_base/account_update.ml


      let to_latest = Fn.id
    end
  end]

+  let with_no_aux ~body ~authorization : t = { body; authorization }


Fee payer doesn't have aux.

So I suggest to rename the method to simply make, and then we can derive it with make added to [@@deriving ...].

I have renamed that method

georgeee · 2025-05-21T14:37:24Z

src/lib/mina_base/account_update.ml

+
+      let to_latest = Fn.id
+
+      let to_yojson : _ -> Yojson.Safe.t = Nothing.unreachable_code


Do we need these?

Yes, at least with how deriving yojson and sexp currently work. When you invoke that on types like t = (A.t, B.t) Poly.t it will always generate an instance that looks something like

let to_yojson = Poly.to_yojson A.to_yojson B.to_yojson

So the yojson and sexp functions for this No_aux type must exist if that type will appear in those kinds of type definitions, to satisfy the ppx code generator.

georgeee · 2025-05-21T20:51:00Z

src/lib/mina_base/zkapp_command.ml

  let events_elements events =
    List.fold events ~init:0 ~f:(fun acc event -> acc + Array.length event)
  in
-  let all_updates, num_event_elements, num_action_elements =
+  let statement_tuple = ((), (), ()) in


I don't understand this change, please explain

The old code collected all the updates in the command into a list, then mapped over the list with const ((),(),()), basically. It never used the values in the list, so this new code should be equivalent to the old code. (The only thing that Update_group.group_by_zkapp_command_rev requires of the statement list is that it have the correct length, really.)

The reason why I had to change it is that I need this to work with a command with updates with any aux parameter, and the fee payer Account_update.t gets computed in this function here with of_fee_payer, so to keep the old code structure I would have needed to unify the fee payer update aux parameter with the aux parameter in the t.account_updates themselves. It seemed better to just skip that work than to, say, go through the trouble of mapping the aux to unit in the t.account_updates.

georgeee · 2025-05-21T20:58:18Z

src/lib/mina_wire_types/test/type_equalities.ml

@@ -247,9 +247,6 @@ module Mina_base = struct
      (O.Zkapp_command.Call_forest.Stable)
      (W.Zkapp_command.Call_forest)
  include Assert_equal0V2 (O.Control.Stable) (W.Control)
-  include Assert_equal0V1 (O.Account_update.Stable) (W.Account_update)


Changes in this file need to be reverted?

I'll double check, but I believe these need to be deleted because the wire types versions of these types are no longer equal in representation to the stable types.

I'm hoping this does not cause any issues with the o1js bindings

Yes, O.Account_update.Stable is the stable account update type, which is not equal to the wire types W.Account_update. If I were to add a type Account_update.Wire.V1.t = (Body.Stable.V1.t, Control.Stable.V2.t) Account_update.Poly.Wire.V1.t, then I'm pretty sure it would instead be true that

include Assert_equal0V1 (O.Account_update.Wire.Stable) (W.Account_update)

I'd have to add stable wire types to keep the other assertions.

This does make me worry a bit that the stable/wire split is going to mess up the o1js bindings - I'd imagine that the Account_update type is actively used in those.

georgeee · 2025-05-21T22:15:57Z

src/lib/mina_base/account_update.ml

+    end]
+  end
+
+  [%%versioned_binable


why is it versioned_binable? Seems like normal versioned to me.

The correct binable instance for this type is the one derived through the Poly.Wire type, I think, so I don't think it should be regular versioned. You can see in the Without_aux type below that a correct binable instance is derived with Wire there. I will check again to see why I didn't derive it through Poly.Wire here. I vaguely recall that it might have been to work around some quirks in the ppx-generated code? Either that or the binable instance is never used and the compiler didn't complain about the missing instance anywhere, so the omission was never caught.

I checked - I can in fact use

include Binable.Of_binable2_without_uuid (Wire.Stable.V1) (struct type nonrec ('x, 'y) t = ('x, 'y, unit) t let of_binable t = of_wire t let to_binable = to_wire end)

to give Poly.Stable.V1.t a binable instance. The only issue is that the generated instance is not compatible with the ppx deriving mechanism for binable, so things defined in terms of Poly.Stable.V1.t would still need to use an explicit Binable.Of_binable* to get their own binable instance.

The reason why the generated instance is not compatible is because of some things I'm forced to do:

I need to use Of_binable2 for Poly.Stable.V1.t, since Poly.Wire.Stable.V1.t has arity 2

('x, 'y) Poly.Wire.Stable.V1.t needs to have arity 2 because we want it to be definitionally equal to Mina_wire_types.Mina_base.Account_update.Poly.V1.t, which has arity 2

('x, 'y, 'z) Poly.V1.t needs to have arity 3 because it needs to have the aux field present

The ppx deriving binable mechanism always assumes that an arity 3 type has bin_shape, etc., functions that take 3 functions, one for each type parameter. However, the functions generated by (1) only take two functions, so the code generated by deriving binable for types defined in terms of Poly.Stable.V1.t is ill-formed as a result.

I also tried giving Mina_wire_types.Mina_base.Account_update.Poly.V1.t a phantom 'aux parameter so that Poly.Wire.Stable.V1.t could have the same phantom parameter, making everything arity 3 and allowing me to use Of_binable3 for Poly.Stable.V1.t. That still doesn't work, because Account_update.Poly.V1.of_wire must have a type equivalent to

val of_wire : ('x, 'y, 'aux) Account_update.Poly.Wire.V1.t -> ('x, 'y, unit) Account_update.Poly.V1.t

since we have to materialize a concrete value for aux out of thin air. This signature seems to be incompatible with what the Of_binable* functors expect.

georgeee · 2025-05-21T22:16:33Z

src/lib/mina_base/account_update.ml

+            (** The cached hash of the actions in an account update body *)
+      }
+
+    let to_yojson : _ -> Yojson.Safe.t = Nothing.unreachable_code


Do we need these?

georgeee · 2025-05-21T22:23:26Z

src/app/zkapps_examples/test/tokens/tokens.ml

-                   ~use_full_commitment:true ~balance_change:(int_to_amount 1)
-                   ~token_id:owned_token_id
-             }
+           @@ Account_update.with_aux


TODO: worth making a simple follow-up PR which would define a function in the file which would take parameters ~may_use_token, ~(balance_change : int) and ~token_id.

Otherwise I see a lot of code duplication.

I can do that in a follow-up PR, sure.

cjjdespres · 2025-05-28T16:07:05Z

!ci-build-me

cjjdespres · 2025-05-28T16:27:35Z

!ci-build-me

cjjdespres · 2025-05-28T16:29:54Z

I think I've gotten to all the review comments. Just to summarize the outstanding issues:

From Cache account update action hashes in a new aux field in Account_update #17120 (comment): I can give Account_update.Poly.V1.t a correct binable instance with Of_binable2, but the resulting methods are incompatible with deriving binable, so types defined in terms of Account_update.Poly.V1.t would still need to use Of_binable* themselves.
From Cache account update action hashes in a new aux field in Account_update #17120 (comment): The type assertions I deleted are in fact still failing, because the stable types and wire types have different representations. I could add Wire versions of all the affected types and then assert that those are equal to the mina_wire_types types, but I don't know if that would make sense.

I also had to change this bit of code in response to a failed test:

https://github.com/MinaProtocol/mina/pull/17120/files#diff-1edc359c0b2f3f296bb16dd374c402ecfb87b2de6d1f0d28968966c26e61fd2bL1673-R1808

I need to provide the ~skip_data if I want this field to be skipped in the graphql conversion.

This field (in Account_update.Poly.t and Account_update.Fee_payer.t) is intended to contain auxiliary data related to the Account_update. For now, the only type this field is used with is the No_aux.t type, which indicates that no auxiliary data is present. In future, the non-stable types will cache the hash of the actions in their body in this field. Since the aux field is not intended for serialization, the yojson/sexp instances for Account_update.Poly.t have been modified to preserve the existing serialization structure. In particular, during serialization this field is ignored, and during deserialization this field cannot be present, and can only take on the type No_aux.t.

This Aux_data will hold the cached value of the hash of the actions in the account update body. For now, it only takes on the value Field.zero. Various functions have been adapted to improve their polymorphism, so they can work with both the stable and unstable versions of the account update types.

The actions_hash is now properly cached on Account_update creation

cjjdespres · 2025-05-29T19:50:00Z

!ci-build-me

cjjdespres · 2025-05-29T19:50:32Z

Had to fix the conflicts from #17299

cjjdespres requested a review from a team as a code owner May 1, 2025 19:31

cjjdespres mentioned this pull request May 1, 2025

Add an aux field to Account_update #17108

Closed

georgeee requested changes May 1, 2025

View reviewed changes

cjjdespres force-pushed the add-account-update-aux branch from 6591b58 to b56c266 Compare May 5, 2025 20:45

This was referenced May 12, 2025

Simplify usages of Account_update.map_proofs #17012

Merged

Remove Staged_ledger_diff from wire types #17147

Merged

cjjdespres force-pushed the add-account-update-aux branch from 1927b90 to 4d8b7c3 Compare May 20, 2025 12:22

georgeee requested changes May 21, 2025

View reviewed changes

georgeee reviewed May 21, 2025

View reviewed changes

cjjdespres force-pushed the add-account-update-aux branch from 4d8b7c3 to db9c6c2 Compare May 28, 2025 16:05

cjjdespres force-pushed the add-account-update-aux branch from db9c6c2 to faae69d Compare May 28, 2025 16:27

cjjdespres and others added 12 commits May 29, 2025 15:16

Parametrize with_forest by account update

d48caba

Parametrize With_hashes_and_data by update

db08f9c

Improve Account_update documentation

968120f

Update and use the Account_update aux field

e78b97a

The actions_hash is now properly cached on Account_update creation

Add changelog file

6b189dc

Clean up the wire representation of Account_update

0aa9fc7

Simplify function signature

787a01f

Restore parametricity in with_forest comparison

04e1aa5

Restore old Account_update wire type

6124c7e

Update 17120.md

250f998

cjjdespres added 2 commits May 29, 2025 15:45

Rename Fee_payer.with_no_aux to make

2b5f9cb

Document use of unreachable_code

3bf7044

cjjdespres force-pushed the add-account-update-aux branch from faae69d to 3bf7044 Compare May 29, 2025 19:49


		let to_latest = Fn.id

		let to_yojson : _ -> Yojson.Safe.t = Nothing.unreachable_code

Cache account update action hashes in a new aux field in Account_update #17120

Are you sure you want to change the base?

Cache account update action hashes in a new aux field in Account_update #17120

Conversation

cjjdespres commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explain your changes:

Explain how you tested your changes:

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjjdespres commented May 2, 2025

Uh oh!

cjjdespres commented May 5, 2025

Uh oh!

cjjdespres commented May 20, 2025

Uh oh!

georgeee commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjjdespres commented May 28, 2025

Uh oh!

cjjdespres commented May 28, 2025

Uh oh!

cjjdespres commented May 28, 2025

Uh oh!

cjjdespres commented May 29, 2025

Uh oh!

cjjdespres commented May 29, 2025

Uh oh!

Uh oh!

cjjdespres commented May 1, 2025 •

edited

Loading