Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not compute hashes in serialization of zkapp #16279

Closed
wants to merge 7 commits into from

Conversation

georgeee
Copy link
Member

@georgeee georgeee commented Oct 23, 2024

This PR relies on #16280, #16285 before being retargeted against compatible and merged

Problem

The serialization of a zkApp is currently performed via an inline Wire type. To convert this type to Zkapp_command.t, hashes of Call_forest.t are computed.

There are several drawbacks to computing hashes in the serialization code:

  • It’s difficult to measure serialization and network performance separately from hashing.
  • Hashing requires access to configuration (hash prefixes are derived from the network ID), which becomes deeply embedded in the code.
  • Hashing is more efficient when done in batches, which is not possible in the current implementation.
  • Some checks for transactions and blocks could be performed before the hashes are computed.

Solution

Avoid auto-converting Zkapp_command.Wire.t to Zkapp_command.t during serialization. Instead, use the Wire type throughout the serialization process and compute the hashes at a higher level of the code when needed.

Notes for reviewer

PR's core change is in src/lib/mina_base/zkapp_command.ml. Type t is re-introduced without use of versioned types. A new Zkapp_command.Wire module is introduced to host the corresponding versioned type. Conversion between Zkapp_command.t and Zkapp_command.Wire.t is performed outside of versioned code, at the call sites that require the conversion (it's useful because conversion is a hashing-intensive operation).

Then all of the types that transitively used Zkapp_command.t are redefined to have separate Wire.t and t types with to_wire and of_wire conversion functions. One notable exception to this rule is Staged_ledger_diff.t, which uses Zkapp_command.Wire.t inside and has an explicit Staged_ledger_diff.With_hashes_computed.t that uses Zkapp_command.t inside.

Another important change is in Mina_block.XX module family. Type Mina_block.with_hash is redefined: from Mina_block.t State_hash.With_state_hashes.t to (Mina_block.Header.t * Staged_ledger_diff.With_hashes_computed.t) State_hash.With_state_hashes.t. This change causes many rewrites from Mina_block.header to fst, from Mina_block.body to snd or equivalent definitions like With_hash.map ~f:(fun (h, _) -> ...).

Once the types are modified, rest of PR is just following the long path of satisfying the compiler. Some functions are given two distinct version to work with wire and non-wire types. Many functions are rewritten in generic form to work for both type definitions.

Version type changes

Versioned type changes are at the core of this PR. Most types remained unchanged, with a few exceptions, which are fully compatible with existing mainnet/devnet networks.

  • Zkapp_command.T.Stable is removed, wherever it was used, it's replaced with Zkapp_command.Wire.Stable which is serialization-equivalent to it
  • Some types in Transition_frontier.Diff are updated, with backwards-compatible versioning
    • These types are only used for database storage, for that reason it's a soft-fork change (networking wasn't touched)

Quality assurance

  • Code compiles
  • Tests pass

Checklist

  • Dependency versions are unchanged
    • Notify Velocity team if dependencies must change in CI
  • 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
    • Mention expected invariants, implicit constraints
  • 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
  • Does this close issues? None

@georgeee georgeee requested a review from a team as a code owner October 23, 2024 19:10
@georgeee georgeee marked this pull request as draft October 23, 2024 19:10
@georgeee georgeee force-pushed the georgeee/extract-transaction-to_wire branch from 9e5122b to 2fab8c8 Compare October 23, 2024 19:23
@georgeee
Copy link
Member Author

Version linter is failing, as expected.

The only issue raised is the following:

Command type at path src/lib/mina_base/zkapp_command.ml:Mina_base__Zkapp_command.T.Stable.V1.t in release branch (origin/compatible) is missing from PR branch

I.e. this is expected

@georgeee georgeee force-pushed the georgeee/extract-transaction-to_wire branch from 3e07acf to 17cf118 Compare October 24, 2024 09:32
@georgeee georgeee force-pushed the georgeee/extract-transaction-to_wire branch from df3eaaa to 454b8ac Compare October 24, 2024 17:32
@georgeee georgeee changed the base branch from compatible to georgeee/extract-transaction-to_wire-tmp-base October 24, 2024 17:33
@georgeee georgeee force-pushed the georgeee/extract-transaction-to_wire branch from 454b8ac to 0e4fb24 Compare October 24, 2024 17:49
@georgeee
Copy link
Member Author

georgeee commented Oct 24, 2024

I left two fixups in the branch history for the purpose of demonstration of what went wrong with the initial commit. This is instructive for a reviewer to get a better sense of what else could have gone wrong.

  • First fixup fixes versioned types used for database (initial version modified their shapes unintentionally)
  • Second fixup fixes a bug of a very "standard" type: during rewrite if b then <..> else <..> was rewritten to if b' then <..> else <..> where b' was a function call equivalent to not b, fixed by swapping the conditional cases

@georgeee
Copy link
Member Author

!ci-build-me

@georgeee georgeee marked this pull request as ready for review October 24, 2024 17:56
Problem: serialization of a zkapp is performed via a inline Wire type.
To convert this type to Zkapp_command.t, hashes of Call_forest.t are
computed.

Computing hashes in serialization code has a few drawbacks:
* It's hard to measure serialization/network performance separately from
  hashes
* Hashing requires access to configuration (hash prefixes are derived
  from network id), which gets burried deep in code
* Hashing is more performant when executed in batches, which isn't
  possible now
* Some checks for transactions and blocks can be performed before these
  hashes are computed

Solution: do not auto-convert Zkapp_command.Wire.t to Zkapp_command.t in
serialization, rather use the former everywhere in serialization and
compute hashes at a higher level of code, when necessary.
This a regression test for mk_zkapp_commands_single_block: before the
fix (that will be delivered as part of PR #16285) arrived, it was
failing, and is passing after the fix.

Along with testing the generator, it also checks the invariant
`Fn.compose of_wire to_wire = ident`.
@georgeee georgeee changed the base branch from georgeee/extract-transaction-to_wire-tmp-base to compatible January 24, 2025 23:15
@georgeee georgeee force-pushed the georgeee/extract-transaction-to_wire branch from 0e4fb24 to 5166b32 Compare January 25, 2025 14:44
@georgeee
Copy link
Member Author

georgeee commented Feb 3, 2025

Closing in favor of a newer and much better organized PR:

@georgeee georgeee closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant