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

replace Root with Root_(hash, common) in persistent database #16650

Conversation

glyh
Copy link

@glyh glyh commented Feb 21, 2025

This is a follow up for #16624

This PR only contains patch to be merged into compatible. It's compatible because we try to replace Root with (Root_hash, Root_common) when:

  1. initializing a new DB;
  2. trying to moving the root;
  3. trying to query root or root_hash

That means attempting to perform transactions on old persistence database, the bottleneck will occur once and not after.

Here's the PR for the testcase.

@glyh glyh requested a review from a team as a code owner February 21, 2025 02:59
@glyh glyh assigned glyh and unassigned glyh Feb 21, 2025
Copy link
Member

@svv232 svv232 left a comment

Choose a reason for hiding this comment

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

I made some grammatical suggestions, but overall looks good! How was this tested dynamically against loading a pre-hard fork db?

In hard forks, `Root` should be replaced by `(Root_hash, Root_common)`;
For now, we try to replace `Root` with `(Root_hash, Root_common)` when:
1. initializing a new DB;
2. trying to moving the root;
Copy link
Member

Choose a reason for hiding this comment

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

trying to move the root.

(* TODO:
In hard forks, `Root` should be replaced by `(Root_hash, Root_common)`;
For now, we try to replace `Root` with `(Root_hash, Root_common)` when:
1. initializing a new DB;
Copy link
Member

Choose a reason for hiding this comment

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

initializing a new DB.

For now, we try to replace `Root` with `(Root_hash, Root_common)` when:
1. initializing a new DB;
2. trying to moving the root;
3. trying to query `root` or `root_hash`
Copy link
Member

Choose a reason for hiding this comment

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

Trying to query root or root_hash.

2. trying to moving the root;
3. trying to query `root` or `root_hash`
The reason for this is `Root_common` is too big(250MB+), and most of the time
we just need the hash. Reading the whole Root result in efficiency problems,
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize we and use a semi colon instead of a comma.

@svv232
Copy link
Member

svv232 commented Feb 21, 2025

!ci-nightly-me

@glyh
Copy link
Author

glyh commented Feb 21, 2025

Hello @svv232 The plan for testing is to have a persistent database that only has Root in it. Try to access it once, confirming the 90s time. And then access it again, confirming it takes less than a second.

George told me to implement something similar to src/app/cli/src/init/transaction_snark_profiler.ml and hand it over to @dkijania to integrate it on CI.

@svv232
Copy link
Member

svv232 commented Feb 21, 2025

Hello @svv232 The plan for testing is to have a persistent database that only has Root in it. Try to access it once, confirming the 90s time. And then access it again, confirming it takes less than a second.

George told me to implement something similar to src/app/cli/src/init/transaction_snark_profiler.ml and hand it over to @dkijania to integrate it on CI.

ok sure! I was just looking for confirmation that this is indeed backwards compatible. It seems okay to me, but having evidence from dynamic testing is pretty nice. How were you testing this by hand against your local db? I will be convinced to approve if this is indeed the case.

I will also be convinced if you show the tool output if you already have it with an old db with just root.

@svv232
Copy link
Member

svv232 commented Feb 21, 2025

!ci-nightly-me

@glyh
Copy link
Author

glyh commented Feb 21, 2025

@svv232 I'm testing this in https://github.com/glyh/mina/tree/persistent_frontier_test_destructive And Using the data from https://storage.googleapis.com/o1labs-ci-test-data/dump_2025_02-13-07-54-53.tar.gz

So What I do is:

  1. Unzip the zip file to some folder, say /abc/def, we'll have a folder /abc/def/2025_02-13-07-54-53
  2. Under branch persistent_frontier_test_destructive, in nix develop mina, run dune build on root directory for mina's repo
  3. cd into _build/default/src/lib/transition_frontier/persistent_frontier/tests/
  4. run TEST_DUMP_PERSISTENT_FRONTIER_SYNC=/abc/def ./test_persistent_frontier.exe test "Deserialization perf test". This should fail, and taking ~90s, which is our bottleneck
  5. run the above command again. This should succeed in ~1s.

EDIT:
It turns out the bottleneck is no longer 90s, but 60s in the branch persistent_frontier_test_destructive. I guess I accidentally optimized something else as well.

@svv232
Copy link
Member

svv232 commented Feb 21, 2025

!ci-build-me

@glyh
Copy link
Author

glyh commented Feb 21, 2025

fatal: couldn't find remote ref corvo/persistent-frontier-split-key-root
🚨 Error: The command exited with status 128
user command error: exit status 128

I assume I should relocated this PR in MinaProtocol/mina instead of glyh/mina?

@glyh
Copy link
Author

glyh commented Feb 21, 2025

Reopen as requested by @dkijania

@dkijania
Copy link
Member

!ci-build-me

@glyh glyh closed this Feb 21, 2025
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.

3 participants