-
Notifications
You must be signed in to change notification settings - Fork 526
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
Optimization: Avoid duplicating proofs during txn verification #14525
Optimization: Avoid duplicating proofs during txn verification #14525
Conversation
745e66a
to
62558ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly sad that we're introducing some potential future error if the command/result ordering ever gets decoupled; ideally we'd add a unit test to catch this kind of regression, but I'm not sure it's worth refactoring the code to do that and risk making this a larger change than it needs to be given where we are with the testnet.
Curious if anyone disagrees with me? cc @nholland94
One way to make it more explicit is to tag (in the caller) each command with an id that then must be matched in the response, something like this: (* Each command is tagged with a unique identifier in the caller. *)
let tag_commands_with_ids commands =
List.mapi commands ~f:(fun id command -> (id, command))
(* The verify function in the subprocess now also returns the identifier with the result. *)
let verify_commands (tagged_commands : (int * User_command.Verifiable.t With_status.t) list) :
(int * verification_result) list Deferred.t =
...
let reinject_valid_user_command_into_valid_result command result =
match result with
| #invalid as invalid ->
invalid (* Directly return invalid results *)
| `Valid_assuming x ->
`Valid_assuming x
| `Valid -> ... (* reinject command into [`Valid command] *)
(* In the parent process, use the IDs to match results to commands. *)
let reassociate_commands_with_results tagged_commands results =
let result_map = Int.Map.of_alist_exn results in
List.map tagged_commands ~f:(fun (id, command) ->
match Map.find result_map id with
| Some result -> reinject_valid_user_command_into_valid_result command result
| None -> failwith "Verification result missing for command"
) ...
|
Pushed a new commit that implements tagging in a similar way to what was described in the previous comment. If you don't think it is necessary or that there is a better solution I will revert it. |
757f9ff
to
88529de
Compare
!ci-build-me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The id tagging isn't a perfect guarantee, but it's better for nothing, and we can consider a more robust solution in the future if we end up having more complicated verifier control flow.
!ci-build-me |
!ci-nightly-me |
!approved-for-mainnet |
… the verifier responses
…match even if ordering changes
88fec9b
to
b2681e5
Compare
!ci-build-me |
@dkijania just rebased and triggered a new CI run |
We tested changes on our tailored pipeline for forks (unfortunately status is not reported here): https://buildkite.com/o-1-labs-2/open-mina-pipeline/builds/54 There are 4 failures:
TL;DR: After we clean up rampup branch this is good to be merged from CI perspective. No action needed from openmina team |
!ci-build-me |
!approved-for-mainnet |
Explain your changes:
Valid.t
back to the node, which in will decode them from a stream creating duplicates of the already existing transactions. This is an issue for zkApp transactions because those contain proofs which are quite memory-heavy.This patch removes the duplicates from the response, and converts the
Verifiable.t
values to theirValid.t
counterparts through the unsafeUser_command.to_valid_unsafe
function.Explain how you tested your changes:
From our testing, we noted this saves quite a bit of memory (both these nodes were launched at the same time):
Checklist: