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

Performance improvement: fetch merkle paths from masks instead of disk #14570

Conversation

mrmr1993
Copy link
Member

This PR improves the performance of merkle path requests when the mask already has the full merkle path. When this case is hit, we avoid walking through all of the intermediate masks, as well as disk access to the database.

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? List them

@mrmr1993
Copy link
Member Author

!ci-build-me

@mrmr1993
Copy link
Member Author

!ci-nightly-me

@mrmr1993
Copy link
Member Author

!ci-build-me

1 similar comment
@mrmr1993
Copy link
Member Author

!ci-build-me

@mrmr1993
Copy link
Member Author

!ci-nightly-me

@mrmr1993
Copy link
Member Author

!ci-build-me

@mrmr1993
Copy link
Member Author

!ci-nightly-me

| Error _ ->
(* Caught by [try_with] above. *) assert false
in
Direction.map sibling_dir ~left:(`Right hash) ~right:(`Left hash)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of direction seems to be flipped here. Is there any reason for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems off to me too, but inverting the direction causes the tests to fail. Not sure why it's like this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this actually should not be inverted. The reason why we get the test failure is because the parent's merkle_path function is wrong. And the root cause of that is here: https://github.com/MinaProtocol/mina/pull/14513/files#r1407811510

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a fix for the above

@mrmr1993 mrmr1993 marked this pull request as ready for review November 21, 2023 18:21
@mrmr1993 mrmr1993 requested a review from a team as a code owner November 21, 2023 18:21
Copy link
Contributor

@Sventimir Sventimir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I just have 2 minor issues explained belowe.

src/lib/merkle_mask/masking_merkle_tree.ml Outdated Show resolved Hide resolved
src/lib/merkle_mask/masking_merkle_tree.ml Outdated Show resolved Hide resolved
@georgeee georgeee force-pushed the feature/batch-merkle-path-lookups branch from 15a4df4 to b9c0b92 Compare November 27, 2023 16:35
@georgeee georgeee force-pushed the feature/improved-merkle-masks-for-staged-ledger branch 2 times, most recently from 0dca053 to 5085720 Compare November 27, 2023 16:51
| Error _ ->
(* Caught by [try_with] above. *) assert false
in
Direction.map sibling_dir ~left:(`Right hash) ~right:(`Left hash)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this actually should not be inverted. The reason why we get the test failure is because the parent's merkle_path function is wrong. And the root cause of that is here: https://github.com/MinaProtocol/mina/pull/14513/files#r1407811510

@georgeee georgeee force-pushed the feature/batch-merkle-path-lookups branch from b9c0b92 to 07af60b Compare November 28, 2023 17:23
@georgeee georgeee force-pushed the feature/improved-merkle-masks-for-staged-ledger branch from 5085720 to 67b4f1d Compare November 28, 2023 17:31
@georgeee
Copy link
Member

!ci-build-me

@georgeee georgeee force-pushed the feature/improved-merkle-masks-for-staged-ledger branch from 6828275 to 5f56888 Compare November 28, 2023 17:37
@georgeee georgeee force-pushed the feature/batch-merkle-path-lookups branch from 07af60b to f644690 Compare November 28, 2023 20:06
@georgeee georgeee force-pushed the feature/improved-merkle-masks-for-staged-ledger branch from 5f56888 to d1e5ed8 Compare November 28, 2023 20:07
@georgeee georgeee force-pushed the feature/batch-merkle-path-lookups branch from f644690 to 5147f71 Compare November 28, 2023 20:12
@georgeee georgeee force-pushed the feature/improved-merkle-masks-for-staged-ledger branch from d1e5ed8 to a636e02 Compare November 28, 2023 20:13
mrmr1993 and others added 22 commits December 1, 2023 22:04
Includes a bugfix in Merkle_ledger.Database.wide_merkle_path_batch
Base automatically changed from feature/batch-merkle-path-lookups to feature/batch-account-lookups December 5, 2023 06:31
…e-merkle-paths

Use 'wide merkle paths' to optimize `Sparse_ledger.of_ledger_subset_exn`
…subset-no-mutability

Avoid ledger copy and mutation in `Sparse_ledger.of_ledger_subset_exn`
…-account-preloading

Allow merkle masks to handle empty accounts directly
…ading

Preload accounts into merkle path for staged ledger diff application
@deepthiskumar deepthiskumar merged commit 0b75a84 into feature/batch-account-lookups Dec 5, 2023
1 check passed
@deepthiskumar deepthiskumar deleted the feature/improved-merkle-masks-for-staged-ledger branch December 5, 2023 06:33
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.

6 participants