Skip to content

Add an implementation of a Converting_merkle_tree #16853

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

Open
wants to merge 10 commits into
base: compatible
Choose a base branch
from

Conversation

mrmr1993
Copy link
Contributor

@mrmr1993 mrmr1993 commented Apr 4, 2025

This PR introduces a converting merkle tree implementation. From the doc-comment:

(** Create a merkle tree implementation that proxies through to
    [Primary_ledger] for all reads and writes, but also updates the
    [Converting_ledger] on every mutation.
    The goal of this is to make it easy to upgrade ledgers for breaking changes
    to the merkle tree. A running daemon can use this to keep track of ledgers
    as normal, but can also retrieve the [Converting_ledger] so that it may be
    used for an automated switch-over at upgrade time.
*)

This mechanism forms the core of our automated ledger upgrades for hard-forks. I have tested this manually with

  • the same Converting_ledger implementation as the Primary_ledger implementation, and confirming that the resulting hashes are identical
  • a Converting_ledger that upgrades the app-state size for zkApps to 32 field elements, and confirming that the ledger contents are as expected.

This PR is separated from the rest of the implementation because it needs its own careful review: if we are not correctly propagating all of the side-effects, then the ledgers will fall out of sync and our ledger upgrade will not be successful. The follow-up PRs will make use of this mechanism to provide the option of an upgrade mechanism.

Design note: the Converting_ledger is treated as optional. In my initial experiments, I attempted to multiplex between a non-optional Converting_ledger and the normal masks / db implementations, but this resulted in large abstraction leaks into the ledger interface. Instead, the code is least affected by replacing uses of Ledger.Db with Ledger.Converting_db and similarly replacing the default ledger masks with a converting mask. In sum, this implementation achieves:

  • the ability to turn it on or off
    • I believe that we don't want to turn this on until we are approaching the hard fork, because this has some performance implications
  • a clear logical separation between upgraded ledger management and primary ledger management
    • in particular, this is important around the database ledgers, and especially so for the epoch ledgers (where we use database snapshots extensively)
  • mostly unchanged code wherever we aren't directly reasoning about database ledgers or conversions.

@mrmr1993 mrmr1993 requested a review from a team as a code owner April 4, 2025 13:39
@mrmr1993
Copy link
Contributor Author

mrmr1993 commented Apr 4, 2025

!ci-build-me

@mrmr1993
Copy link
Contributor Author

mrmr1993 commented Apr 4, 2025

!ci-bypass-changelog

@martyall
Copy link
Member

I have been continuing this work for PR #17155 , you can find some tests in the mina context (see ledger.ml).

My recommendation after working with this in an application context would be to make the converting_ledger field non-optional and to decide to use this ledger impl or a different one depending on dynamic configuration. The way we use an existential Any_ledger type for all of our ledgers makes this more natural and is less ambiguous to configure.

After speaking with @georgeee I'm going to request this change and implement it as part of this PR. I will also add a simple test

@martyall
Copy link
Member

!ci-build-me

… from intf, functorize Base_types in prep for tests
@martyall martyall force-pushed the feature/add-migrating-ledger-functor branch from a8a6a0b to 3e4b74b Compare May 13, 2025 04:04
@martyall
Copy link
Member

!ci-build-me

@martyall
Copy link
Member

@georgeee

In addition to the work that Matthew did, I have done the following:

  • use non-optional secondary ledger in the converting ledger module, we are going to handle the optionality using configuration as discussed
  • refactored some of the existing code in merkle_ledger_tests (especially in the test_stubs and test_database modules) to look more like the implementation I found while working on Add Converting Ledger feature with example and tests #17155 . These changes are not just for fun -- they are important to be able to re-use code between the existing database tests and the new converting ledger tests i wrote
  • added the Unstable account type in this pr that I was using in Add Converting Ledger feature with example and tests #17155. This is because the merkle_ledger_tests are using the actual account types from mina, which meant i needed an alternative account type to write the test. Again, Unstable is an arbitrary name, we can change it in subsequent work.
  • Added a test_converting module with all the setup and a simple converting ledger test. More can be added over time if desired

@martyall
Copy link
Member

!ci-build-me

@martyall
Copy link
Member

!ci-build-me

Copy link
Member

@cjjdespres cjjdespres left a comment

Choose a reason for hiding this comment

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

So the idea is that the Converting_merkle_tree ledger acts as the primary
ledger for reads, but remembers to set the state of the secondary ledger
correctly for any writes/updates. In a future PR, code will be added to initialize this ledger, and the Converting_merkle_tree will be used to keep it up-to-date even as we continue to use the original ledger. Then, the actual hard fork will switch us over to the new ledger? I don't see any issues with this initial implementation, in that case.

The test is just a sample, as you mention in the changelog, but the structure of it also looks good. The PR description mentions

This PR is separated from the rest of the implementation because it needs its own careful review: if we are not correctly propagating all of the side-effects, then the ledgers will fall out of sync and our ledger upgrade will not be successful.

Is the new test case expected to encounter all of these side effects, or will more tests need to be added later, perhaps in a follow-up PR? It's probably fine to delay that until whatever PR actually enables the feature, if so. It might be difficult to test for ledger desync more thoroughly without having a concrete use of the Converting_merkle_tree in the code.

@@ -54,6 +54,7 @@ let test_db () =
Quickcheck.test ~trials:5 ~sexp_of:[%sexp_of: Balance.t list]
gen_non_zero_balances ~f:(fun balances ->
let account_ids = Account_id.gen_accounts num_accounts in
let T = Account_id.eq in
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Member

@martyall martyall May 27, 2025

Choose a reason for hiding this comment

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

So every time you see a diff of this form

let T = Some_module.eq

It was a hack in order to be able to assign module signatures to things in the file src/lib/merkle_ledger_tests/test_stubs.ml that previously had only module defs with no signature. (The reason I added signatures was because personally I was having a hard time understanding what role different modules were playing in the ledger/db creation -- imagine a giant haskell file with no type signatures).

Since some of those signature's declarations were injected via an include statement and have opaque type t, I used this hack in order to get the call sites to compile. In this particular case it was because of this module signature

I am not an ocaml expert, but I could not find another way to get this to compile. I was also trying things like include Account_id with type t = blah but I wasn't getting that to work. Maybe @glyh or @georgeee has a better idea of how to get around this.

Copy link
Member

@glyh glyh May 28, 2025

Choose a reason for hiding this comment

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

This is a type witness, you bring them into context, so the compiler knows some types under current context are equal. They are purely compile time, and in runtime they're just a wrapper for Fn.id.

I'll take a look at the code to see if we could get rid of them, as in general I hate this pattern.

Copy link
Member

@glyh glyh May 28, 2025

Choose a reason for hiding this comment

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

In this case, it's of type (Mina_base.Account_id.t, Account_id.t) Type_equal.t, so it's instructing the compiler these 2 types are equal.

I personally think it's fine if we just expose the internal of the opaque t in the signature: We already exposed it via wire type, which means this should be a public-available interface. Hence it can't be of more harm doing so.

Comment on lines +39 to +40
(* to prevent pre-image attack,
* important impossible to create an account such that (merge a b = hash_account account) *)
Copy link
Member

Choose a reason for hiding this comment

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

Is this mentioned here to justify the test_ledger_%d prefix that's included in the hash in merge?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose this comment already exists next to other test account implementations.

Copy link
Member

@martyall martyall May 27, 2025

Choose a reason for hiding this comment

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

This is actually copy/paste from the other Hash definition, I have no idea if it's justified. Unfortunately the way things were already set up Hash module depends on the Account (not ideal imo), but 99% of the module's declarations are the same. Hence the copy/paste

@martyall
Copy link
Member

So the idea is that the Converting_merkle_tree ledger acts as the primary
ledger for reads, but remembers to set the state of the secondary ledger
correctly for any writes/updates. In a future PR, code will be added to initialize this ledger, and the Converting_merkle_tree will be used to keep it up-to-date even as we continue to use the original ledger. Then, the actual hard fork will switch us over to the new ledger? I don't see any issues with this initial implementation, in that case.

@cjjdespres This is also my understanding

Is the new test case expected to encounter all of these side effects, or will more tests need to be added later, perhaps in a follow-up PR?

We can definitely add more later. I just wanted to add the initial test setup and the diff was already getting quite large (though most of the diff is coming from the still untested library code).

@glyh
Copy link
Member

glyh commented May 28, 2025

Just a general request: could you put merkle_ledger_tests under folder merkle_ledger? It would help us have this repo more organized. Just moving the folder would be enough because dune doesn't resolve module by file path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants