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

Remove Zkapp_command.Wire.t #16556

Draft
wants to merge 29 commits into
base: georgeee/proof-cache-tag
Choose a base branch
from

Conversation

georgeee
Copy link
Member

@georgeee georgeee commented Feb 3, 2025

This PR removes Zkapp_command.Wire.t along with a Binable-based trick that was used in combination with Zkapp_command.Wire.t to provide serialization for Zkapp_command.t.

Motivation for the refactoring lies in the fact that in the previous version hashing code was invoked from the serialization. This is problematic in two ways:

  1. It gives us very little control over the sequence of hashing
    • in the light of recent work, it's very beneficial to batch independent hashing operations
    • having hash computation buried so deep in the code means that realizing benefits of the performance improvement is practically impossible
  2. Hashing relies on "signature kind" configuration
    • having hash computation buried so deep means that it's practically impossible to use the value supplied by the runtime config
    • using the value supplied by compile-time configuration induces an unnecessary inflexibility in use of built binaries, and stops us from achieving eventual goal of making all of the compile-time configuration overridable with the runtime config

Explain how you tested your changes:

  • Most of the changes are pure refactoring
  • Some changes modify logging format in an unsubstantial way
    • many log changes are associated with Zkapp_command.Stable.Latest.t being used instead of Zkapp_command.t in various places were an error occurs during transaction processing
    • because none of these is documented as an outside-facing feature, and is meant for supplying more fine-grained debug information via logs, this seems a safe change
  • Format accepted by mina advanced verify-commands has changed, but the old format is still supported with a new optional --legacy flag

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

In Transaction_hash.User_command_with_valid_signature
Remove Transaction_hash.User_command_with_valid_signature.Stable
Define stable and regular types separately
Define stable and regular types separately
Contains a few temporary edits to be unwinded later.
Plus: define `generate` and `unwrap` as `Fn.id` in `zkapp_command.ml`
and use these definitions in all of the types that depend on
`Zkapp_command.t`.
This interface file is an exact copy of the transaction_witness.ml,
hence it's redundant to keep it.
This commit is a no-op change, but prepares for the future switch of
types.
This is a no-op change which prepares codebase for future switch of
zkapp_command types.
This is a no-op modification that is required for future switch of
zkapp_command types.
Remainder of changes in preparation of removing hashes from
Zkapp_command.Stable.Latest.t.
Use a no-hash representation.
@georgeee georgeee requested a review from a team as a code owner February 3, 2025 13:04
@georgeee georgeee requested a review from a team as a code owner February 3, 2025 13:04
@georgeee georgeee marked this pull request as draft February 3, 2025 13:04
@georgeee
Copy link
Member Author

georgeee commented Feb 3, 2025

!ci-build-me

@georgeee georgeee changed the title Remove Zkapp_command.Wire.t Remove Zkapp_command.Wire.t 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