Allow merkle masks to handle empty accounts directly #14585

mrmr1993 · 2023-11-20T20:35:42Z

This PR builds upon #14571, implementing a similar preloading mechanism for empty accounts to avoid falling back 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 · 2023-11-20T20:35:51Z

!ci-build-me

mrmr1993 · 2023-11-20T20:35:56Z

!ci-nightly-me

mrmr1993 · 2023-11-21T03:50:01Z

!ci-build-me

sebastiencs · 2023-11-21T10:18:46Z

src/lib/merkle_ledger_tests/test_mask.ml

@@ -188,17 +188,6 @@ module Make (Test : Test_intf) = struct
        (* verify all hashes to root are same in mask and parent *)
        compare_maskable_mask_hashes maskable attached_mask dummy_address )

-  let%test "mask delegates to parent" =


Remove invalid test: mask parents should not be changed

Is this actually an invalid test ? Masks parents are sometimes changed/modified.
It is the reason why there is the method Mask.Attached.parent_set_notify:

mina/src/lib/merkle_mask/maskable_merkle_tree.ml

Line 227 in 5528528

Mask.Attached.parent_set_notify mask account )

Removing this test means we're breaking the relation parent/child described in specs/merkle_tree.md

@georgeee Can we add this test back? If the dummy account is added before the child mask is attached, it should do as expected, right?

sebastiencs · 2023-11-21T10:25:57Z

src/lib/merkle_mask/masking_merkle_tree.ml

@@ -181,7 +181,16 @@ module Make (Inputs : Inputs_intf.S) = struct
      | Some account ->
          Some account
      | None ->
-          Base.get (get_parent t) location
+          let is_empty =
+            match t.current_location with


Suggested change

match t.current_location with

match (last_filled t) with

(last_filled t) should probably be used here ? to get updated location from all the parents.
This would allow to keep the test above to pass.

I'll defer to @nholland94, but IMO we break a bunch of invariants (e.g. merkle paths) if we allow incompatible writes to the parent ledger, and the test is misguided.

This affects masks behavior: that would break this property of masks, which is quite important IMO:

Whenever a read occurs on a mask, the mask will first attempt to retrieve the node from its internal sparse collection, and if it does not exist there, will forward the read request to its parent.

(text from specs/merkle_tree.md)

If the performance of (last_filled t) is the concern, it could be called only once in get_batch and self_merkle_path

Hmm, this is confusing. We should probably update the mask interface to not allow updating parent masks if it breaks the invariants. I'm seeing calls to parent_set_notify which is to notify child masks about updates in the parent mask?
Re the test, if the dummy account was added to the parent mask before creating the child mask, then it should pass?

We should probably update the mask interface to not allow updating parent masks if it breaks the invariants.

This was my suggestion last week, but I believe consensus was that this will be too big of a change to implement right now.

But in general I agree with this, though supposedly related functionalities are just dead code (the riskiness of a change lies in the fact that it'd require a complicated analysis of the whole codebase to prove it's actually dead, though I'd argue seeing it alive is worse and we'd better throw a runtime error when such things occur).

Ok. created #14631 to track this. We should address this before making any planned changes to this library

deepthiskumar · 2023-11-28T03:06:58Z

src/lib/merkle_mask/masking_merkle_tree.ml

-      self_find_or_batch_lookup self_find_location
-        Base.location_of_account_batch
+    let location_of_account_batch t =
+      (* TODO consider handling special case of empty addresses *)


@mrmr1993 looks like this is needed. self_find_or_batch_lookup would end up accessing disk for empty accounts otherwise, no?

P.S. this is my TODO which I left when doing rebases

I believe we need it, I will be able to implement it I think

We can have a follow-up PR for this, I think

@deepthiskumar I guess we don't have a choice in this function because unlike loading an account by location here we can't tell of an account whether it's a new one or just wasn't loaded from parent.

It might have an implication to performance of transaction processing (I do see some db lookups despite the fact that we preload a lot of stuff). But I think we'll handle it via special-casing unsafe_preload function, not right here.

P.S. I removed the TODO

Justification: we don't have a choice in this function because unlike loading an account by location here we can't tell of an account whether it's a new one or just wasn't loaded from parent. It might have an implication to performance of transaction processing (I do see some db lookups despite the fact that we preload a lot of stuff). But I think we'll handle it via special-casing unsafe_preload function, not right here.

deepthiskumar · 2023-11-30T22:01:45Z

!ci-build-me

Includes a bugfix in Merkle_ledger.Database.wide_merkle_path_batch

…k-empty-account-preloading

…ure/sparse-ledger-of-subset-no-mutability

…ture/sparse-ledger-wide-merkle-paths

…edger-mask-maps

Replace tables with maps in merkle masks

…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`

mrmr1993 mentioned this pull request Nov 20, 2023

Use the pre-computed hashes from the database when preloading into a sparse ledger mask #14586

Closed

7 tasks

sebastiencs reviewed Nov 21, 2023

View reviewed changes

mrmr1993 marked this pull request as ready for review November 21, 2023 18:21

mrmr1993 requested a review from a team as a code owner November 21, 2023 18:21

georgeee force-pushed the feature/merkle-mask-preloading branch from df57650 to 344a10d Compare November 27, 2023 16:53

georgeee force-pushed the feature/merkle-mask-empty-account-preloading branch 2 times, most recently from e26350e to 64e75d1 Compare November 27, 2023 18:00

deepthiskumar reviewed Nov 28, 2023

View reviewed changes

ghost-not-in-the-shell approved these changes Nov 28, 2023

View reviewed changes

georgeee force-pushed the feature/merkle-mask-preloading branch from a0c92df to 576bc95 Compare November 28, 2023 17:56

georgeee force-pushed the feature/merkle-mask-empty-account-preloading branch 2 times, most recently from e8ee28a to 4a9f96f Compare November 28, 2023 18:27

georgeee force-pushed the feature/merkle-mask-preloading branch from 576bc95 to d530750 Compare November 28, 2023 20:07

georgeee force-pushed the feature/merkle-mask-empty-account-preloading branch from 4a9f96f to 4a490a8 Compare November 28, 2023 20:07

georgeee force-pushed the feature/merkle-mask-preloading branch from d530750 to b3525d3 Compare November 28, 2023 20:13

georgeee force-pushed the feature/merkle-mask-empty-account-preloading branch from 4a490a8 to 94cfcd1 Compare November 28, 2023 20:14

georgeee force-pushed the feature/merkle-mask-preloading branch from b3525d3 to 5e71099 Compare November 28, 2023 20:29

mrmr1993 and others added 5 commits November 28, 2023 21:30

Allow fetching empty merkle paths from masks

499fc6f

Handle known-empty accounts in merkle masks without hitting the db

e889475

Handle known-empty accounts in get_batch for merkle masks

cc770e5

Remove invalid test: mask parents should not be changed

8c66c3e

georgeee force-pushed the feature/merkle-mask-empty-account-preloading branch from 94cfcd1 to df631b4 Compare November 28, 2023 20:30

deepthiskumar mentioned this pull request Nov 28, 2023

Disable updating masks when there are child masks #14631

Open

mrmr1993 and others added 3 commits November 28, 2023 21:31

Avoid ledger copy and mutation in Sparse_ledger.of_ledger_subset_exn

a8b5b8e

refactor the code a little bit

40060b7

further clean up the code

4ae3174

Small refactoring in of_ledger_subset_exn

717ff96

deepthiskumar and others added 24 commits December 1, 2023 21:56

fix the order in the location list

9c89cca

Add Sparse_ledger.add_wide_path

c86d1d5

Add add_wide_path_batch to Ledger_intf

69b33f2

Use wide ledger paths for Sparse_ledger.of_ledger_subset_exn

91133a2

Refactor wide merkle path functions

52003ed

Includes a bugfix in Merkle_ledger.Database.wide_merkle_path_batch

Small rewrite of path_batch_impl

eaa054c

Nit: rename variables

8e47aaf

Refactor wide_merkle_path_batch

d77b5e5

fix the order of the locations

19c2500

fixup! Use wide ledger paths for Sparse_ledger.of_ledger_subset_exn

13d8d2c

Refactor add_wide_path_unsafe

39fee93

Generalize add_path in sparse ledger

c20e6b3

Simplify add_path_impl's recursion

d6e6137

Replace tables with maps in merkle masks

758955f

Fix merkle ledger tests

175c24a

Use map refs instead of mutable map fields

8371f64

Fixup after rebase

4403440

Merge branch 'feature/merkle-mask-preloading' into feature/merkle-mas…

b2e07a7

…k-empty-account-preloading

Merge branch 'feature/merkle-mask-empty-account-preloading' into feat…

a27ed50

…ure/sparse-ledger-of-subset-no-mutability

Merge branch 'feature/sparse-ledger-of-subset-no-mutability' into fea…

5baa860

…ture/sparse-ledger-wide-merkle-paths

Merge branch 'feature/sparse-ledger-wide-merkle-paths' into feature/l…

7240af9

…edger-mask-maps

Merge pull request #14595 from MinaProtocol/feature/ledger-mask-maps

6458ec8

Replace tables with maps in merkle masks

Merge pull request #14594 from MinaProtocol/feature/sparse-ledger-wid…

5364a0f

…e-merkle-paths Use 'wide merkle paths' to optimize `Sparse_ledger.of_ledger_subset_exn`

Merge pull request #14587 from MinaProtocol/feature/sparse-ledger-of-…

dc434fa

…subset-no-mutability Avoid ledger copy and mutation in `Sparse_ledger.of_ledger_subset_exn`

deepthiskumar merged commit 4b56a35 into feature/merkle-mask-preloading Dec 5, 2023
1 check passed

deepthiskumar deleted the feature/merkle-mask-empty-account-preloading branch December 5, 2023 06:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow merkle masks to handle empty accounts directly #14585

Allow merkle masks to handle empty accounts directly #14585

mrmr1993 commented Nov 20, 2023

mrmr1993 commented Nov 20, 2023

mrmr1993 commented Nov 20, 2023

mrmr1993 commented Nov 21, 2023

sebastiencs Nov 21, 2023

deepthiskumar Nov 28, 2023

sebastiencs Nov 21, 2023

mrmr1993 Nov 21, 2023

sebastiencs Nov 22, 2023

deepthiskumar Nov 28, 2023

georgeee Nov 28, 2023

deepthiskumar Nov 28, 2023

deepthiskumar Nov 28, 2023

georgeee Nov 28, 2023 •

edited

Loading

georgeee Nov 28, 2023

georgeee Nov 28, 2023 •

edited

Loading

deepthiskumar commented Nov 30, 2023

Allow merkle masks to handle empty accounts directly #14585

Allow merkle masks to handle empty accounts directly #14585

Conversation

mrmr1993 commented Nov 20, 2023

mrmr1993 commented Nov 20, 2023

mrmr1993 commented Nov 20, 2023

mrmr1993 commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeee Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeee Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

deepthiskumar commented Nov 30, 2023

georgeee Nov 28, 2023 •

edited

Loading

georgeee Nov 28, 2023 •

edited

Loading