-
Notifications
You must be signed in to change notification settings - Fork 583
Add an initial method to generate a full reference hard fork config for testing #17816
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
Related: #17752 |
96a8de1
to
6a7b23f
Compare
I suggest changing the folder structure to something like $ tree genesis/
genesis/
├── berkeley
│ ├── activated
│ ├── daemon.json
│ └── whatever.tar.gz
└── mesa
├── activated
├── daemon.json
└── whatever.tar.gz
3 directories, 6 files So to be forward compatible. Also this allow us to mark completeness of fork config separately. Maybe |
I think the directory structure is something that @dkijania might have opinions on as well, since he thought having this command output the precomputed fork block and the legacy format config would be useful for testing. I was also wondering if it would be useful to save the |
I hadn't intended this to match the output of the automatic config dump exactly, but maybe that's wise. In that case, the structure for the migrated (mesa) format would look like
if we wanted the output in This is slightly complicated by the fact that the current compatible daemon wants the config directory to look like this:
Maybe it would be simpler if the command accepted a few parameters to control where the config directories and precomputed block json file should go, separately. |
Or it could be this by default:
and in the automatic config dump we'd omit the |
src/app/cli/src/init/client.ml
Outdated
Cli_lib.Exceptions.handle_nicely | ||
Test_genesis_creation.time_genesis_creation ) | ||
|
||
let test_generate_hardfork_config = |
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.
Am i understand correctly that we are putting this command under test since its mocking some parameters, right?
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.
That's a good point. I don't actually think it needs to be under test. It could just be an advanced command. It's mocking some parameters only because I haven't added them yet. The genesis timestamp delta in particular could be an optional parameter to the command, and the daemon could use its configured timestamp delta if that parameter isn't set. But, the runtime config for that delta hasn't been added yet either.
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.
Any unit, component tests for this feature?
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.
It does seems this PR has some rough edges to be completed before called functional
src/app/cli/src/init/mina_run.ml
Outdated
Mina_lib.Hardfork_config.dump_reference_config | ||
~breadcrumb_spec:`Stop_slot ~directory_name mina | ||
in | ||
match result with |
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.
Nit:
- convert error to json with helpers in
Error_json
and attach them to metadata in logging. - this log level should be
error
, I think - I think it's also useful to log a info line on succeed
src/app/cli/src/init/client.ml
Outdated
Daemon_rpcs.Generate_hardfork_config.rpc directory_name port | ||
with | ||
| Error e -> | ||
eprintf "Failed to request hardfork config generation: %s\n" |
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.
Nit:
- use logger
- use
Error_json
to log error in json to logging metadata
src/app/cli/src/init/client.ml
Outdated
(Error.to_string_hum e) ; | ||
exit 17 | ||
| Ok () -> | ||
printf "Hardfork configuration successfully requested in %s\n" |
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.
Nit: use logger. and log dir name in metadata.
src/app/cli/src/init/mina_run.ml
Outdated
() | ||
in | ||
don't_wait_for (dump ()) ; | ||
return (Ok ()) ) |
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 don't understand the fire-and-forget pattern here. Why can't this be a blocking operation?
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.
Like we block for a good reason. Client side block indicates that server is working.
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.
Good catch. I'll change this. I wrote it like this because initially I was migrating the ledgers all at once (before the ledger sync was fully merged) and that was causing the daemon to become unresponsive, which made the command fail due to the timeout.
In reality we should be making sure that the daemon remains responsive throughout the hard fork config generation.
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.
Should hopefully be fixed in the latest update.
module Generate_hardfork_config = struct | ||
type query = string [@@deriving bin_io_unversioned] | ||
|
||
type response = unit Or_error.t [@@deriving bin_io_unversioned] |
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 assume this is not the final RPC version since more param to be added.
src/lib/mina_lib/mina_lib.ml
Outdated
, Account.Hardfork.of_stable acct ) ) | ||
in | ||
(* should this be a parameter? *) | ||
let chunk_size = 1 lsl 6 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.
Probably, but for now could we lift it to top level so it's easier to spot? When we want them actually to be params we can always do a refactor.
src/lib/mina_lib/mina_lib.ml
Outdated
Ledger.Hardfork_db.create ~directory_name ~depth () | ||
in | ||
`Inputs | ||
( accounts |
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.
We don't need to pass accounts along with length of accounts, I think. the later is just length of the previous.
src/lib/mina_lib/mina_lib.ml
Outdated
let genesis_next_epoch_ledger_opt = | ||
match source_ledgers.next_epoch_ledger with | ||
| `Genesis l -> | ||
get_genesis_migration_inputs ~name:"staking_ledger" l |
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.
Not something that's very important, but it seems here we can also clash if current epoch staking ledger = next epoch staking ledger
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.
Also I'm confused by the naming scheme of these ledgers.
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.
Not something that's very important, but it seems here we can also clash if current epoch staking ledger = next epoch staking ledger
True. This is actually likely to be encountered in my proposed component test for this command (single node with simple genesis ledger) so I think I'll end up wanting to fix that issue in the runtime_genesis_ledger.exe
and here.
Also I'm confused by the naming scheme of these ledgers.
The ~name
? I just wanted a temporary name for the ledger databases that were being generated. I do see that I accidentally give ~name:"staking_ledger"
to the genesis next epoch ledger. I'll fix that.
~target_dir ~ledger_name_prefix root = | ||
let open Deferred.Or_error.Let_syntax in | ||
let root_hash = get_root_hash root in | ||
let ledger_dirname = get_directory root |> Option.value_exn 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.
Nit: add an error message here.
src/lib/mina_lib/mina_lib.ml
Outdated
| `Root l -> | ||
Ledger.Root.create_stable_checkpoint l ~directory_name | ||
| `Uncommitted _l -> | ||
Ledger.Root.create_stable_checkpoint |
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 don't understand the necessity of breaking apply diffs and creating checkpoint into 2 steps. It make here fishy at a first glance.
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'm hoping the new code is a little easier to follow. I still do it in stages, but hopefully a little more cleanly:
- Collect the diffs and copy the roots
- Migrate the stable database if the migrated version isn't available
- Apply the diffs to the copied roots
I'm doing it this way because I wanted the code to work with or without ledger sync enabled, so (2) might take a while. So, I'd like to gather all the inputs necessary for generating the hard fork config in one go, including copying the roots. Then I can do whatever slow, gradual things need to be done on the copies of everything.
I'm still slightly concerned about what might happen if, say, we try generating a hard fork config at the precise moment the daemon decides to commit to a new snarked root. We might have to move the code to gather the hard fork inputs into the transition controller, or at least add some kind of "fork input copying in progress" async flag(s) somewhere, so the controller knows not to mess with the underlying databases until we're done.
Do you mind left some instructions here using these configs to boot a new post-HF node instance? That'd be helpful |
I think you can use symlink? Although I personally think it's not critical. I'm fine keeping it as it currently is, just a nit idea of improvement. |
6a7b23f
to
371f953
Compare
I've force-pushed to bring in the recent |
That is something I want to add next. I was hoping for feedback on the general design/initial implementation before I kept going. (Thanks for that, by the way). The basic component test I was thinking of was starting a single daemon with a known genesis ledger, using this command to generate a compatible hard fork config, then restarting the daemon with the output config.
I agree. |
371f953
to
2c885cc
Compare
I have updated/rewritten the PR. It's hopefully a bit cleaner. I'll update the PR description on Monday, but just to have it down:
I think it will be easier for testing purposes to create the config directories according to what the Still to do for this code (in subsequent PRs, unless you feel strongly about doing this now)
Incidentally, if |
All you should need to do is something like:
I haven't tested the migrated hard fork config again since I wrote the previous version of this PR two weeks ago, but I have tested the |
The client RPC command
mina advanced test generate-hard-fork-config --hardfork-config-dir <DIR>
can be used to request that a running daemon generate and save a complete hard fork config in the specified directory<DIR>
. This will currently contain:A
genesis/
directory containing thetar.gz
archives of the hard fork genesis ledgersdaemon.json
containing the updated genesis constants for the hard forkgenesis_legacy/
directory containing thetar.gz
archives of the hard fork genesis ledgers without any account migration applieddaemon.legacy.json
containing the updated genesis constants based on thegenesis_legacy/
ledgersactivated
file to indicate that the hard fork config was generated fully without errorsThis method is not quite complete - it needs:
slot_tx_end
(or the best tip if that's not set) at the moment