-
Notifications
You must be signed in to change notification settings - Fork 578
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
base: compatible
Are you sure you want to change the base?
Conversation
!ci-build-me |
!ci-bypass-changelog |
I have been continuing this work for PR #17155 , you can find some tests in the mina context (see My recommendation after working with this in an application context would be to make the 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 |
…converting_merkle_ledger module
!ci-build-me |
… from intf, functorize Base_types in prep for tests
a8a6a0b
to
3e4b74b
Compare
!ci-build-me |
…of a migrated account type
…se we can't have it apparently
In addition to the work that Matthew did, I have done the following:
|
!ci-build-me |
!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.
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 |
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.
What does this do?
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.
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.
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.
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.
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.
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.
(* to prevent pre-image attack, | ||
* important impossible to create an account such that (merge a b = hash_account account) *) |
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.
Is this mentioned here to justify the test_ledger_%d
prefix that's included in the hash in merge
?
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 suppose this comment already exists next to other test account implementations.
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.
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
@cjjdespres This is also my understanding
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). |
Just a general request: could you put |
This PR introduces a converting merkle tree implementation. From the doc-comment:
This mechanism forms the core of our automated ledger upgrades for hard-forks. I have tested this manually with
Converting_ledger
implementation as thePrimary_ledger
implementation, and confirming that the resulting hashes are identicalConverting_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-optionalConverting_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 ofLedger.Db
withLedger.Converting_db
and similarly replacing the default ledger masks with a converting mask. In sum, this implementation achieves: