-
Notifications
You must be signed in to change notification settings - Fork 584
Set hard fork config slot and timestamp properly #18028
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
Set hard fork config slot and timestamp properly #18028
Conversation
The hard fork global slot since genesis and the hard fork timestamp are now set according to the stop slots and the hard fork genesis slot delta, if those components of the daemon config have been set. If they have not, then the slot and timestamp of the hard fork block itself are used as a fallback.
|
!ci-bypass-changelog |
|
!ci-build-me |
src/lib/mina_lib/mina_lib.ml
Outdated
| let global_slot_since_hard_fork_to_genesis | ||
| ~(constraint_constants : Genesis_constants.Constraint_constants.t) | ||
| global_slot = | ||
| (* Convert the global slot to a span of slots since the current hard fork *) | ||
| let global_slot_span = | ||
| global_slot |> Mina_numbers.Global_slot_since_hard_fork.to_uint32 | ||
| |> Mina_numbers.Global_slot_span.of_uint32 | ||
| in | ||
| (* Retrieve the global slot since genesis of the genesis of the current | ||
| chain *) | ||
| let current_genesis_global_slot = | ||
| constraint_constants.fork | ||
| |> Option.value_map ~default:Mina_numbers.Global_slot_since_genesis.zero | ||
| ~f:(fun fork -> fork.global_slot_since_genesis) | ||
| in | ||
| (* Add the slot span to the current chain's genesis slot to get the desired quantity *) | ||
| Mina_numbers.Global_slot_since_genesis.add current_genesis_global_slot | ||
| global_slot_span |
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 calculation is correct, but I couldn't find an existing method to do it.
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 suprised there's no such helpers elsewhere.
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.
Was going to suggest we put this into module Global_slot_since_hardfork. But there could be some circular dependency issue(or maybe not?).
Could we put a comment in that module indicating there's a conversion implementation here?
I guess also a comment here this is noted in module Global_slot_since_hardfork.
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.
LGTM overall.
| ; block_timestamp : Block_time.t | ||
| } | ||
|
|
||
| (** The genesis state timestamp string is the timestamp of the start of the |
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.
Suggest:
Replace
the timestamp of the start of the [global_slot] of the hard fork
with
the timestamp of the start of the epoch of the [global_slot]
I don't think anything implementation-wise here is related to HF, although it's probably used in that context.
Also I'd suggest rename the input from global_slot -> global_slot_since_hard_fork just so I don't need to look at type annotations.
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 is actually the start of the slot itself, not the epoch or anything like that. I think a "slot" is technically the entire 3 minute time interval. A lot of the code only deals with the start of it, though; the block producer always schedules blocks to be produced at the start of the slot, and sets the block timestamp equal to the start of the slot too.
src/lib/mina_lib/mina_lib.ml
Outdated
| let global_slot_since_hard_fork_to_genesis | ||
| ~(constraint_constants : Genesis_constants.Constraint_constants.t) | ||
| global_slot = | ||
| (* Convert the global slot to a span of slots since the current hard fork *) | ||
| let global_slot_span = | ||
| global_slot |> Mina_numbers.Global_slot_since_hard_fork.to_uint32 | ||
| |> Mina_numbers.Global_slot_span.of_uint32 | ||
| in | ||
| (* Retrieve the global slot since genesis of the genesis of the current | ||
| chain *) | ||
| let current_genesis_global_slot = | ||
| constraint_constants.fork | ||
| |> Option.value_map ~default:Mina_numbers.Global_slot_since_genesis.zero | ||
| ~f:(fun fork -> fork.global_slot_since_genesis) | ||
| in | ||
| (* Add the slot span to the current chain's genesis slot to get the desired quantity *) | ||
| Mina_numbers.Global_slot_since_genesis.add current_genesis_global_slot | ||
| global_slot_span |
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 suprised there's no such helpers elsewhere.
src/lib/mina_lib/mina_lib.ml
Outdated
| let global_slot_since_hard_fork_to_genesis | ||
| ~(constraint_constants : Genesis_constants.Constraint_constants.t) | ||
| global_slot = | ||
| (* Convert the global slot to a span of slots since the current hard fork *) | ||
| let global_slot_span = | ||
| global_slot |> Mina_numbers.Global_slot_since_hard_fork.to_uint32 | ||
| |> Mina_numbers.Global_slot_span.of_uint32 | ||
| in | ||
| (* Retrieve the global slot since genesis of the genesis of the current | ||
| chain *) | ||
| let current_genesis_global_slot = | ||
| constraint_constants.fork | ||
| |> Option.value_map ~default:Mina_numbers.Global_slot_since_genesis.zero | ||
| ~f:(fun fork -> fork.global_slot_since_genesis) | ||
| in | ||
| (* Add the slot span to the current chain's genesis slot to get the desired quantity *) | ||
| Mina_numbers.Global_slot_since_genesis.add current_genesis_global_slot | ||
| global_slot_span |
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.
Was going to suggest we put this into module Global_slot_since_hardfork. But there could be some circular dependency issue(or maybe not?).
Could we put a comment in that module indicating there's a conversion implementation here?
I guess also a comment here this is noted in module Global_slot_since_hardfork.
src/lib/mina_lib/mina_lib.ml
Outdated
| let global_slot_since_genesis = | ||
| Mina_block.consensus_state block | ||
| |> Consensus.Data.Consensus_state.global_slot_since_genesis | ||
| global_slot_since_hard_fork_to_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.
It's a bit weird here. In consensus state we're tracking slot both since genesis and since hard fork. I wonder why.
Is this just a misdesign, or on purpose?
That would affect our attitude on the helper doing conversion of slot since hf -> slot since 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.
The Global_slot_since_genesis name is maybe a bit misleading. It's supposed to track the slot number starting from the genesis of the very first chain (the one before Berkeley), and it's always supposed to go up, never down. The Global_slot_since_hard_fork gets reset whenever there's a hard fork.
That's my understanding, anyway.
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.
So the proof.fork.global_slot_since_genesis in the daemon.json lets you know that the new chain is starting at that particular "absolute" slot.
That's why I think the equality
global_slot_since_genesis
= global_slot_since_hard_fork
+ proof.fork.global_slot_since_genesis
is justified.
| val best_chain_block_by_state_hash : | ||
| t -> State_hash.t -> Transition_frontier.Breadcrumb.t Or_error.t | ||
|
|
||
| val best_chain_block_before_stop_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.
Nice
| let hard_fork_genesis_slot_delta t = | ||
| let open Option.Let_syntax in | ||
| let%bind daemon = t.daemon in | ||
| let%map delta = daemon.hard_fork_genesis_slot_delta 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.
I was going to suggest we change t.daemon.hard_fork_genesis_slot_delta type to be Global_slot_span.t so this conversion is not needed every time we need this value.
However it seems there's some obscure json conversion there. I guess we can add a note there for follow-up if there's ever going to be refactor around there.
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 could probably add the stop slots and genesis delta to the main Mina_lib.Config as well (except as their real types). Then the daemon could just parse them once at startup and store those values.
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.
Yes, but it's nit and requires quite a bit of refactoring, so just some idea for later.
|
!ci-build-me |
|
!ci-build-me |
|
The commit 3a5ed3a I pushed calculates the hard fork global slot since genesis a little differently. It does exactly what mina/src/lib/consensus/proof_of_stake.ml Line 1851 in e8e5f9e
Basically the method calculates the right global slot since genesis by pretending that we transitioned from the hard fork block's slot to the scheduled genesis slot. I think the other way was also probably fine, but this is more obviously correct in my opinion. |
|
Oh, I think I've checked again and forgot to comment this lgtm after the follow up commits, sorry |
The hard fork global slot since genesis and the hard fork timestamp are now set according to the stop slots and the hard fork genesis slot delta, if those components of the daemon config have been set. If they have not, then the slot and timestamp of the hard fork block itself are used as a fallback.
The
forkConfiggraphql query still does not set the genesis timestamp in the config it returns, just to keep its behaviour unchanged for now. I think it may end up being more convenient to have the daemon set the timestamp itself when it can; if so, theforkConfigoutput can probably be changed to include the timestamp. I think the hard fork test(s) would have to be adjusted slightly if we were to do that.