-
Notifications
You must be signed in to change notification settings - Fork 586
Apply vesting params update to runtime genesis ledger and root #18106
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
…slot_reduction_update to accounts
… vesting transformation
cjjdespres
left a comment
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 think this could have a small changelog entry as well. I don't think the ledger sync part of this needs to be mentioned, but you could point out that the runtime_genesis_ledger can now optionally apply the vesting parameter update from the slot reduction MIP when it creates Mesa genesis ledgers.
Otherwise I think this is good. The full functionality of the Any_root is probably not needed at this point, I agree.
| ~doc: | ||
| "INT slot when HF should happen, this is only used for Migrate_exit \ | ||
| mode" | ||
| (optional Cli_lib.Arg_type.global_slot) |
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.
Actually, the daemon already has the hard fork genesis delta:
mina/src/lib/runtime_config/runtime_config.ml
Line 483 in f8bd813
| ; hard_fork_genesis_slot_delta : int option [@default None] |
which is the number of slots after the slot_chain_end that the hard fork should occur.
So the same calculation as this:
mina/src/lib/mina_lib/mina_lib.ml
Line 461 in f8bd813
| let scheduled_hard_fork_genesis_slot t : |
should be used. Then it has to be converted to a global slot since genesis like I did before this commit: 3a5ed3a
(I'm realizing that commit, while correct, did remove the more broadly useful global_slot_since_hard_fork_to_genesis method in favour of the fancy but less broadly useful move_hard_fork_consensus_to_scheduled_genesis)
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.
But calculating the hard fork slot once and storing it in the Mina_lib.Config is useful, I think. A follow-up PR could remove move_hard_fork_consensus_to_scheduled_genesis entirely from the hard fork config generation, and the hard fork global slot since genesis could be taken directly from the mina lib 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.
Looks like technical debt is hitting hard here. There's an cycle dependency ... I'll do more refactor
| ~doc: | ||
| "INT the hardfork slot since global genesis at which vesting \ | ||
| parameter update should happen. By default, don't update the \ | ||
| vesting parameters" |
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.
Having this parameter be a global slot since genesis might be a little confusing, because the slot_tx_end and slot_chain_end are global slots since hard fork. I don't think it comes up in the existing hard fork test because I'm pretty sure Global_slot_since_genesis = Global_slot_since_hard_fork there; there's only a difference if the consensus constants have a non-zero proof.fork.global_slot_since_genesis. (That's another potential modification/variation to the existing HF test, incidentally - doing a fork followed by another fork. That would end up being a pretty long test.)
These will be different for Berkely->Mesa, so it's probably better to have this be a global slot since hard fork too.
Or, if we wanted the same parameters here as in the daemon, this --hardfork-slot option could be replaced by two options --slot-chain-end and --hard-fork-genesis-slot-delta.
…_config_file, it's already in precomputed_values
…edger_helper, remove cli arg hardfork_slot
…ince global genesis
|
!ci-build-me |
As title.
Context: https://o1-labs.slack.com/archives/C07EYM9GR1U/p1762970442022119
I refactored ledger root to remove the functor as it's unnecessarily complicated.
Closes #18041
I'll try to add some test in follow up PRs.