Skip to content
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

[FLIP 204] Add TargetEndTime to FlowEpoch smart contract #395

Merged
merged 9 commits into from
Nov 14, 2023

Conversation

jordanschalm
Copy link
Member

@jordanschalm jordanschalm commented Nov 10, 2023

Addresses #394.

This PR adds the new field TargetEndTime to the EpochSetup event, which will be used by the Cruise Control system to moderate block rate so that epochs end at the desired time. See details in FLIP 204

I have not added targetEndTime to the EpochStart event. Here is why:

  • We can't easily persist the target end time, without creating a new ad-hoc storage path. I don't think this is worth the extra complexity.
  • The timing config can change between the EpochSetup and EpochStart events. If we don't store the value, we may communicate different values in different events for the same epoch.

@jordanschalm jordanschalm marked this pull request as ready for review November 11, 2023 18:53
Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'll push back the dates on the upgrades so we can get this in

///
pub struct EpochTimingConfig {
/// The duration of each epoch, in seconds
pub let duration: UInt64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will every epoch always have the same duration? I understand the config can change, but what if the duration changes, but we are using a reference epoch that is before the change? Would we just never do that?

Copy link
Member Author

@jordanschalm jordanschalm Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, all epochs have the same duration (1 week on Mainnet). If/when we want to change the duration, then we would change the timing config.

The reference epoch needs to contain a real reference counter that has been used in the past. But the associated duration can be anything we want (it does not need to match the duration that the reference epoch actually took, when it occurred). So there isn't anything stopping us from using a reference epoch that is before the duration change, if we want to.

@jordanschalm
Copy link
Member Author

I'll push back the dates on the upgrades so we can get this in

👍 Initially I thought we would need to push a flow-go patch before including this change in an upgrade. However, it seems that the relevant service event parsing is already backward-compatible (tested in onflow/flow-go#5013), so we won't need to do this.

We should still make sure to go through an epoch transition on Canary before upgrading Testnet/Mainnet to be safe, but otherwise there's no blockers for including this in the next upgrade.

@joshuahannan joshuahannan merged commit ec49fc8 into access-slots Nov 14, 2023
2 checks passed
@joshuahannan joshuahannan deleted the jordan/394-epoch-target-end-time branch November 14, 2023 17:21
@joshuahannan joshuahannan restored the jordan/394-epoch-target-end-time branch November 15, 2023 17:07
@joshuahannan joshuahannan deleted the jordan/394-epoch-target-end-time branch February 7, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants