Create faster genesis root population method and get rid of Root.transfer_accounts_with #17659
Conversation
Instead of having a populate_root method to populate an existing, empty root ledger from genesis, the genesis ledger now has a create_root method. This method assumes the responsibility for creating the root ledger itself. The create_root method can thus checkpoint the genesis ledger directly when the genesis ledger is backed by a database.
|
!ci-nightly-me |
| Ledger.Root.transfer_accounts_with | ||
| ~stable:Ledger_transfer_stable.transfer_accounts | ||
| let transfer_snarked_root ~src ~dest = | ||
| Ledger_transfer_any.transfer_accounts |
There was a problem hiding this comment.
I kept the use of Ledger_transfer_any.transfer_accounts in this method. It's used exactly once, when populating the root snarked ledger from a recovered potential snarked ledger.
This could be removed too, if reading all the accounts in the database and regenerating the database structure is not important for this recovery mechanism.
There was a problem hiding this comment.
We trust filesystem, right? So I'd suggest removing this as well.
|
!ci-build-me |
|
CI is broken it seems |
|
!ci-build-me |
| @@ -1029,7 +1029,8 @@ module For_tests = struct | |||
| ~directory:(Filename.temp_file "snarked_ledger" "") | |||
| ~ledger_depth | |||
| in | |||
| Persistent_root.reset_to_genesis_exn persistent_root ~precomputed_values ; | |||
| Async.Thread_safe.block_on_async_exn (fun () -> | |||
There was a problem hiding this comment.
Please don't use this function in non-top level. Async.Thread_safe.block_on_async_exn is only supposed to be called once per OS process.
There was a problem hiding this comment.
I'd propagate this _ Deferred.t all the way to top level.
| @@ -77,11 +77,6 @@ struct | |||
| let as_unmasked t = | |||
| match t with Stable_db db -> Any_ledger.cast (module Stable_db) db | |||
|
|
|||
| let transfer_accounts_with ~stable ~src ~dest = | |||
There was a problem hiding this comment.
Now this is no longer needed, can we go with my old solution using existential type for backing root with converting ledger? This seems to be place you pointed out that won't work with existential wrapped converting ledgers.
Context: #17645 (comment)
| Ledger.Root.transfer_accounts_with | ||
| ~stable:Ledger_transfer_stable.transfer_accounts | ||
| let transfer_snarked_root ~src ~dest = | ||
| Ledger_transfer_any.transfer_accounts |
There was a problem hiding this comment.
We trust filesystem, right? So I'd suggest removing this as well.
| (Mina_block.Validated.state_hash transition) ; | ||
| ignore | ||
| @@ populate_root (Persistent_root.Instance.snarked_ledger instance) ) ; | ||
| Async.Thread_safe.block_on_async_exn (fun () -> |
There was a problem hiding this comment.
Can we remove these 2 Async.Thread_safe.block_on_async_exn as well?
Explain your changes:
Instead of having a populate_root method to populate an existing, empty root ledger from genesis, the genesis ledger now has a create_root method. This method assumes the responsibility for creating the root ledger itself. The resulting API is more convenient to use elsewhere in the code base. This design also allows the create_root method to checkpoint the genesis ledger directly when the genesis ledger is backed by a database, which is much faster than an account transfer.
The
transfer_accounts_withmethod has been removed - it is unclear if it is necessary.Explain how you tested your changes:
The nightly tests should cover this change. I also added some temporary log lines to measure the time taken to populate a snarked root from genesis, to compare against the measurements I did in #17570. I got this result when I synced a daemon to mainnet with an empty config directory:
So, this should resolve that issue as well.
Checklist: