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

Preload accounts into merkle path for staged ledger diff application #14571

Conversation

mrmr1993
Copy link
Member

This PR adds preloading for ledger masks, using batch operations to get merkle paths and accounts for the transactions. This lets us avoid disk IO when applying transactions.

This PR builds upon #14570.

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 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
@mrmr1993
Copy link
Member Author

!ci-build-me

@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
@georgeee georgeee force-pushed the feature/merkle-mask-preloading branch from df57650 to 344a10d Compare November 27, 2023 16:53
@@ -613,6 +624,33 @@ module Make (Inputs : Inputs_intf.S) = struct
self_find_or_batch_lookup self_find_location
Base.location_of_account_batch

let unsafe_preload_accounts_from_parent t account_ids =
Copy link
Member

Choose a reason for hiding this comment

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

@mrmr1993 A comment on when this could be used or should not be used would be helpful

Copy link
Member

Choose a reason for hiding this comment

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

Addressed

let sibling_addr = Location.Addr.sibling addr in
let addr = Location.Addr.parent_exn addr in
let hash = match path with `Left hash | `Right hash -> hash in
self_set_hash t sibling_addr 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 we should use addr instead of sibling_addr here because merkle_path is already the path of siblings. We did it like this here because the merkle_path function itself is wrong.

Copy link
Member

Choose a reason for hiding this comment

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

I cherry-picked all commits from PR #14586 (which I will close) to this branch and function set_merkle_path_unsafe stops being used and is completely removed by this change.

So I guess we can close this discussion.

@georgeee georgeee force-pushed the feature/improved-merkle-masks-for-staged-ledger branch 2 times, most recently from 6828275 to 5f56888 Compare November 28, 2023 17:37
@georgeee georgeee force-pushed the feature/merkle-mask-preloading branch from a0c92df to 576bc95 Compare November 28, 2023 17:56
@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/merkle-mask-preloading branch from 576bc95 to d530750 Compare November 28, 2023 20:07
@georgeee georgeee force-pushed the feature/improved-merkle-masks-for-staged-ledger branch from d1e5ed8 to a636e02 Compare November 28, 2023 20:13
@georgeee georgeee force-pushed the feature/merkle-mask-preloading branch from d530750 to b3525d3 Compare November 28, 2023 20:13
@georgeee georgeee force-pushed the feature/merkle-mask-preloading branch from b3525d3 to 5e71099 Compare November 28, 2023 20:29
@deepthiskumar
Copy link
Member

!ci-build-me

deepthiskumar and others added 26 commits December 1, 2023 21:56
Includes a bugfix in Merkle_ledger.Database.wide_merkle_path_batch
…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
@deepthiskumar deepthiskumar merged commit d9035e7 into feature/improved-merkle-masks-for-staged-ledger Dec 5, 2023
1 check passed
@deepthiskumar deepthiskumar deleted the feature/merkle-mask-preloading 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.

5 participants