-
Notifications
You must be signed in to change notification settings - Fork 51
Don't process scheduled transactions during epoch transitions #560
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: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ import "FlowToken" | |||||
| import "FlowFees" | ||||||
| import "FlowStorageFees" | ||||||
| import "ViewResolver" | ||||||
| import "FlowEpoch" | ||||||
|
|
||||||
| /// FlowTransactionScheduler enables smart contracts to schedule autonomous execution in the future. | ||||||
| /// | ||||||
|
|
@@ -1284,6 +1285,14 @@ access(all) contract FlowTransactionScheduler { | |||||
| return | ||||||
| } | ||||||
|
|
||||||
| // Skip processing if the epoch is in one of the phase transitions | ||||||
| // or expensive operations | ||||||
| // We don't skip if it is running in a test environment, | ||||||
| // so we check automaticRewardsEnabled() because it is only true on testnet and mainnet. | ||||||
| if FlowEpoch.isPhaseTransition() && FlowEpoch.automaticRewardsEnabled() { | ||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to skip this check if it is on the emulator because epochs don't run on the emulator. Is checking automatic rewards enabled an okay way to do that? I assume that it is only enabled on testnet and mainnet
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure emulator even has flowEpoch
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The emulator has the FlowEpoch contract, but regular epoch functionality isn't enabled, which is why I skip this check for the emulator
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Maybe front-load the less expensive operation. Not sure about Cadence, but usually if the first condition is false, we will skip evaluating the second. |
||||||
| return | ||||||
| } | ||||||
|
|
||||||
| self.removeExecutedTransactions(currentTimestamp) | ||||||
|
|
||||||
| let pendingTransactions = self.pendingQueue() | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1296,6 +1296,41 @@ access(all) contract FlowEpoch { | |
| ?? 0.0 | ||
| } | ||
|
|
||
| /// Checks if the current epoch is in a phase transition | ||
| /// so that other system contracts can skip computation-heavy operations | ||
| /// during the phase transition. | ||
| access(all) fun isPhaseTransition(): Bool { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes me nervous that this is duplicating the state machine transition logic from I know it's hard to refactor deployed contracts, so maybe there just isn't a good way to do this, but I'm wondering if there is a way to use the same logic here and in |
||
| switch self.currentEpochPhase { | ||
| case EpochPhase.STAKINGAUCTION: | ||
| if let previousEpochMetadata = self.getEpochMetadata(FlowEpoch.currentEpochCounter.saturatingSubtract(1)) { | ||
| // Paying rewards for the previous epoch | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if auto-rewards are disabled? If we are manually paying rewards for some reason, we might pass this check for a portion of the staking auction period (for example, suppose on Mainnet we manually paid rewards Thursday morning).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (This is also not really a phase transition. The phase transition from Committed to Staking happened in the prior block and is captured by line 1327. This is just "an expensive thing the FlowEpoch contract does", which I think is a more accurate description of what this function is trying to capture.) |
||
| if self.currentEpochCounter > 0 && !previousEpochMetadata.rewardsPaid { | ||
| return true | ||
| } | ||
| } | ||
| let currentBlock = getCurrentBlock() | ||
| let currentEpochMetadata = self.getEpochMetadata(self.currentEpochCounter)! | ||
| // Staking auction is ending | ||
| if currentBlock.view >= currentEpochMetadata.stakingEndView { | ||
| return true | ||
| } | ||
| case EpochPhase.EPOCHSETUP: | ||
| // QC and DKG are completed and will be cleaned up | ||
| if FlowClusterQC.votingCompleted() && (FlowDKG.dkgCompleted() != nil) { | ||
| return true | ||
| } | ||
| case EpochPhase.EPOCHCOMMIT: | ||
| let currentBlock = getCurrentBlock() | ||
| let currentEpochMetadata = FlowEpoch.getEpochMetadata(FlowEpoch.currentEpochCounter)! | ||
| // Epoch is ending | ||
| if currentBlock.view >= currentEpochMetadata.endView { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| init (currentEpochCounter: UInt64, | ||
| numViewsInEpoch: UInt64, | ||
| numViewsInStakingAuction: UInt64, | ||
|
|
||
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import "FlowEpoch" | ||
|
|
||
| access(all) fun main(): Bool { | ||
| return FlowEpoch.isPhaseTransition() | ||
| } |
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 understand the desire to avoid adding load to the blocks with expensive epoch operations, but I'm not convinced it is necessary.
The downside of adding load to these epoch operation blocks is that blocks which already have an abnormally long execution time, get longer still. (4 blocks per week on Mainnet which currently take X seconds to compute, now take X+Y seconds to compute.)