-
Notifications
You must be signed in to change notification settings - Fork 548
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
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
georgeee
force-pushed
the
georgeee/extract-transaction-to_wire
branch
from
October 23, 2024 19:23
9e5122b
to
2fab8c8
Compare
Version linter is failing, as expected. The only issue raised is the following:
I.e. this is expected |
georgeee
force-pushed
the
georgeee/extract-transaction-to_wire
branch
from
October 24, 2024 09:32
3e07acf
to
17cf118
Compare
8 tasks
georgeee
force-pushed
the
georgeee/extract-transaction-to_wire
branch
from
October 24, 2024 17:32
df3eaaa
to
454b8ac
Compare
georgeee
changed the base branch from
compatible
to
georgeee/extract-transaction-to_wire-tmp-base
October 24, 2024 17:33
georgeee
force-pushed
the
georgeee/extract-transaction-to_wire
branch
from
October 24, 2024 17:49
454b8ac
to
0e4fb24
Compare
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.
|
!ci-build-me |
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
changed the base branch from
georgeee/extract-transaction-to_wire-tmp-base
to
compatible
January 24, 2025 23:15
georgeee
force-pushed
the
georgeee/extract-transaction-to_wire
branch
from
January 25, 2025 14:44
0e4fb24
to
5166b32
Compare
Closing in favor of a newer and much better organized PR: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR relies on #16280, #16285 before being retargeted against
compatible
and mergedProblem
The serialization of a zkApp is currently performed via an inline
Wire
type. To convert this type toZkapp_command.t
, hashes ofCall_forest.t
are computed.There are several drawbacks to computing hashes in the serialization code:
Solution
Avoid auto-converting
Zkapp_command.Wire.t
toZkapp_command.t
during serialization. Instead, use theWire
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
. Typet
is re-introduced without use of versioned types. A newZkapp_command.Wire
module is introduced to host the corresponding versioned type. Conversion betweenZkapp_command.t
andZkapp_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 separateWire.t
andt
types withto_wire
andof_wire
conversion functions. One notable exception to this rule isStaged_ledger_diff.t
, which usesZkapp_command.Wire.t
inside and has an explicitStaged_ledger_diff.With_hashes_computed.t
that usesZkapp_command.t
inside.Another important change is in
Mina_block.XX
module family. TypeMina_block.with_hash
is redefined: fromMina_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 fromMina_block.header
tofst
, fromMina_block.body
tosnd
or equivalent definitions likeWith_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 withZkapp_command.Wire.Stable
which is serialization-equivalent to itTransition_frontier.Diff
are updated, with backwards-compatible versioningQuality assurance
Checklist