-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add FLIP 204 - Smart-Contract-Specified Epoch Switchover Timing #204
Add FLIP 204 - Smart-Contract-Specified Epoch Switchover Timing #204
Conversation
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.
Only looked at the Cadence code
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Bastian Müller <[email protected]>
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
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 pretty good to me! I am also in favor of option 2. Shouldn't be too hard to implement
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
```cadence | ||
pub struct EpochTimingConfig { | ||
duration: UInt64 // in seconds | ||
refCounter: UInt64 // the counter of a reference epoch |
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.
How is the reference epoch chosen?
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 doesn't really matter. We can pick any previous epoch transition. Initially we would just pick the most recent one.
The same reference epoch can be used for as long as we want, so long as there is no discontinuity in epoch timing (very long period of downtime or epoch fallback mode being active for an extended period). If that happens, then we would pick a new reference epoch (the first one after the discontinuity).
protocol/20231003-service-account-specified-epoch-switchover.md.md
Outdated
Show resolved
Hide resolved
### Q&A | ||
#### What happens during a `resetEpoch`? | ||
|
||
In **Option 1.1** and **2**, the timing of a particular epoch transition does not affect the target timing for other epochs. Therefore, the `TargetEndTime` computation of an epoch during a spork will not behave differently from any other epoch. |
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.
If we're using a reference epoch to calculate the time of future epochs and the reset epoch doesn't happen at the expected epoch switchover time, won't that be a problem since it wasn't the expected length?
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.
No, I don't think it would cause a problem. Using a reference epoch decouples the target end time computation from actual switchover timing. The target end time is 100% dependent on that reference epoch; the actual network can do anything in the interim and it won't affect the target end time.
That can be a downside, if we have a large, durable timing change, because it won't be reflected here. We would need to manually change the reference epoch to accommodate that. But we don't expect that to happen, so don't optimize for it.
It can be an upside, because:
- we can do the
resetEpoch
at any time - it has no impact - small fluctuations in switchover timing have no impact and therefore don't accumulate
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.
okay I think that makes sense
- clean up code samples - label options 1.1, 1.2 - add suggestions
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.
@jordanschalm Would you want to work on the implementation and make a PR based off onflow/flow-core-contracts#379 so we can try to get this into the same upgrade as the other changes? I addressed all your comments so it is good to go from my end
Yes, although I will not have bandwidth to work on this until after the Offsite next week - Oct 16 at the earliest. Is there a timeline for doing that upgrade? |
That should probably be fine. We don't have a timeline yet |
Adds FLIP 204