Add converting ledger with test #17428

glyh · 2025-06-17T09:41:48Z

Continuation of #17379

Status: waiting for next round of review.

glyh · 2025-06-17T09:44:35Z

src/lib/mina_ledger/ledger.ml

+      if Unstable_db.num_accounts db2 = 0 then
+        Or_error.return @@ Converting_ledger.create_with_migration db1 db2
+      else if not (Db.num_accounts db1 = Unstable_db.num_accounts db2) then (
+        (* TODO: ensure 2 dbs have exact same accounts *)


Well, we have test coverage for this so it's not that fatal, though

To respond to #17379 (comment) here - yes, this is insufficient as a desync check. A converting merkle tree is two databases and we write to them separately, so it should be possible for the daemon to crash between database updates, which would cause a desync. I think you're right, that that is only detectable with a full equality check (up to account conversion) between the databases.

I think part of the slowness of migration is all the hashing that needs to be done to populate the database, not necessarily the actual account conversion. If the Unstable.of_stable conversion function is expected to be fast, then we might be able to do this integrity check right here. @georgeee do you know how heavy the real conversion function is likely to be?

I think part of the slowness of migration is all the hashing that needs to be done to populate the database, not necessarily the actual account conversion.

This is my expectation as well

do you know how heavy the real conversion function is likely to be?

We expect conversion to be trivial, i.e. not using any cryptographic operations at all.

cjjdespres · 2025-07-07T21:34:30Z

src/lib/mina_ledger/ledger.ml

+      if Unstable_db.num_accounts db2 = 0 then
+        Or_error.return @@ Converting_ledger.create_with_migration db1 db2
+      else if not (Db.num_accounts db1 = Unstable_db.num_accounts db2) then (
+        (* TODO: ensure 2 dbs have exact same accounts *)


To respond to #17379 (comment) here - yes, this is insufficient as a desync check. A converting merkle tree is two databases and we write to them separately, so it should be possible for the daemon to crash between database updates, which would cause a desync. I think you're right, that that is only detectable with a full equality check (up to account conversion) between the databases.

cjjdespres · 2025-07-07T21:46:24Z

src/lib/mina_ledger/ledger.ml

+        (* TODO: ensure 2 dbs have exact same accounts *)
+        Db.close db1 ;
+        Unstable_db.close db2 ;
+        Or_error.error_string


I think if a desync is detected then we might want to clean up the secondary database and re-migrate, and just issue a warning/error log message. I'm not sure if that cleanup should happen here or if it should be the responsibility of the caller of this method, though. I'd have to check to see how the existing databases are cleaned up.

(The issue I see is similar to the crash back-off loop with the empty PID file - if the invalid resource isn't cleaned up automatically somehow then the working directory of the daemon will become unusable).

cjjdespres · 2025-07-07T21:52:23Z

src/lib/mina_ledger/ledger.ml

+      if Unstable_db.num_accounts db2 = 0 then
+        Or_error.return @@ Converting_ledger.create_with_migration db1 db2
+      else if not (Db.num_accounts db1 = Unstable_db.num_accounts db2) then (
+        (* TODO: ensure 2 dbs have exact same accounts *)


I think part of the slowness of migration is all the hashing that needs to be done to populate the database, not necessarily the actual account conversion. If the Unstable.of_stable conversion function is expected to be fast, then we might be able to do this integrity check right here. @georgeee do you know how heavy the real conversion function is likely to be?

cjjdespres · 2025-07-07T21:54:04Z

I think my comments are showing up twice because I made them "request changes"?

glyh · 2025-07-13T07:58:09Z

src/lib/mina_ledger/ledger.ml

+      else
+        let%bind db1_accounts = Db.accounts db1 in
+        let%bind db2_accounts = Unstable_db.accounts db2 in
+        (* TODO: check if relying on identifier is enough for equality *)


You could do something similar to what's done in the merkle ledger tests for database equality:

mina/src/lib/merkle_ledger_tests/test_converting.ml

Lines 194 to 199 in 76b8999

Db.iteri primary ~f:(fun idx primary_account ->

let stored_migrated_account =

Db_migrated.get_at_index_exn migrated idx

in

[%test_eq: Migrated.Account.t] stored_migrated_account

(Db_converting.convert primary_account) ) ) ) )

Synced ledgers should have accounts at identical indices.

Because of #17428 (comment) I don't think we need to worry about the speed of account conversion. Hopefully.

src/lib/mina_ledger/ledger.ml

glyh · 2025-07-15T05:25:42Z

!ci-build-me

cjjdespres · 2025-07-15T15:30:29Z

src/lib/merkle_ledger/database.ml

@@ -43,7 +42,17 @@ module Make (Inputs : Intf.Inputs.DATABASE) = struct

  let depth t = t.depth

-  let create ?directory_name ~depth () =
+  let rec remove_dir_recursively path =


There is Mina_stdlib_unix.File_system.rmrf for this, potentially

glyh · 2025-07-16T00:50:05Z

rebased atop compatible

cjjdespres

I think that's the last thing. Could you clean up the commits as well? They could probably be condensed down to the first one.

cjjdespres · 2025-07-16T01:37:27Z

src/lib/merkle_ledger/database.ml

@@ -43,7 +44,7 @@ module Make (Inputs : Intf.Inputs.DATABASE) = struct

  let depth t = t.depth

-  let create ?directory_name ~depth () =
+  let create ?directory_name ?fresh ~depth () =


Sorry, I missed this before - could you declare the fresh with a default to avoid having to deal with the Some later on here?

The new Converting_ledger ledger type in mina_ledger is backed by a converting_merkle_tree - this will keep a secondary ledger database in an unstable format synced with a primary ledger database in a stable format.

glyh · 2025-07-16T02:02:02Z

!ci-build-me

amc-ie · 2025-07-16T06:11:30Z

!ci-bypass-changelog

glyh requested a review from a team as a code owner June 17, 2025 09:41

glyh added the ledger-migration label Jun 17, 2025

glyh commented Jun 17, 2025

View reviewed changes

cjjdespres requested changes Jul 7, 2025

View reviewed changes

glyh commented Jul 13, 2025

View reviewed changes

src/lib/mina_ledger/ledger.ml Outdated Show resolved Hide resolved

cjjdespres mentioned this pull request Jul 14, 2025

Abstract snarked ledger type and add checkpoint operations #17534

Closed

cjjdespres reviewed Jul 15, 2025

View reviewed changes

glyh force-pushed the lyh/converting-ledger-and-tests branch from 95d8adc to 6a0c5de Compare July 16, 2025 00:50

cjjdespres requested changes Jul 16, 2025

View reviewed changes

Add converting ledger with test

e806e3d

The new Converting_ledger ledger type in mina_ledger is backed by a converting_merkle_tree - this will keep a secondary ledger database in an unstable format synced with a primary ledger database in a stable format.

glyh force-pushed the lyh/converting-ledger-and-tests branch from 119cfac to e806e3d Compare July 16, 2025 02:00

cjjdespres approved these changes Jul 16, 2025

View reviewed changes

glyh merged commit 2ea4285 into compatible Jul 16, 2025
71 checks passed

glyh deleted the lyh/converting-ledger-and-tests branch July 16, 2025 06:18

glyh mentioned this pull request Jul 22, 2025

Add converting ledger with test #17379

Closed

7 tasks

	Db.iteri primary ~f:(fun idx primary_account ->
	let stored_migrated_account =
	Db_migrated.get_at_index_exn migrated idx
	in
	[%test_eq: Migrated.Account.t] stored_migrated_account
	(Db_converting.convert primary_account) ) ) ) )

Add converting ledger with test #17428

Add converting ledger with test #17428

Uh oh!

Conversation

glyh commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjjdespres commented Jul 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glyh commented Jul 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glyh commented Jul 16, 2025

Uh oh!

cjjdespres left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glyh commented Jul 16, 2025

Uh oh!

amc-ie commented Jul 16, 2025

Uh oh!

Uh oh!

Uh oh!

glyh commented Jun 17, 2025 •

edited

Loading