Don't process scheduled transactions during epoch transitions #560
Conversation
| // 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() { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'm not sure emulator even has flowEpoch
https://github.com/onflow/flow-emulator/blob/master/emulator/blockchain.go#L2221
There was a problem hiding this comment.
The emulator has the FlowEpoch contract, but regular epoch functionality isn't enabled, which is why I skip this check for the emulator
joshuahannan
force-pushed
the
josh/skip-during-epochs
branch
from
5e2dd4b to
ce48e6b
Compare
| switch self.currentEpochPhase { | ||
| case EpochPhase.STAKINGAUCTION: | ||
| if let previousEpochMetadata = self.getEpochMetadata(FlowEpoch.currentEpochCounter.saturatingSubtract(1)) { | ||
| // Paying rewards for the previous epoch |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
(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.)
| // 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() { |
There was a problem hiding this comment.
| if FlowEpoch.isPhaseTransition() && FlowEpoch.automaticRewardsEnabled() { | |
| if FlowEpoch.automaticRewardsEnabled() && FlowEpoch.isPhaseTransition() { |
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.
| /// 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 { |
There was a problem hiding this comment.
It makes me nervous that this is duplicating the state machine transition logic from advanceBlock. Any future changes would need to be carefully applied to both functions.
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 advanceBlock. Maybe this function could return an enum which maps to a possible state machine transition in advanceBlock, like Enum{``PayRewards``, ``StakingToSetup``, ``SetupToCommitted``, ``EndEpoch``, ``Noop``}. Then advanceBlock could switch on the returned enum, and callers interested in whether any expensive operation is occurring could check isPhaseOperation != Noop.
| // Skip processing if the epoch is in one of the phase transitions | ||
| // or expensive operations |
There was a problem hiding this comment.
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.)
- Are there any other downsides, besides adding Y seconds of execution time to these blocks?
- If we are just increasing the execution time of 4 blocks per week, it's not clear to me this is necessary. If we are concerned about this exceeding hard limits which would cause halts then this makes more sense to me.
- Do we know what X and Y are in practice?
- If we already have a few blocks that take X=5 seconds, Y is small, and so this PR is preventing them from taking like 6 seconds, that is not clearly a good trade-off to me, because we still have the same basic problem that some blocks take abnormally long to execute.
- On the other hand, if Y is large compared to X, then this will be a problem for every block (not just blocks where epoch operations are occurring).
Closes #527
Since epoch transitions already take a lot of computation, we shouldn't process scheduled transactions in the same block that epoch transitions are occurring.
Processing always happens before the epoch heartbeat, so we can make all the same checks that the epoch contract makes.
Adds a utility function to
FlowEpochto indicate if a phase transition is occurring so other system contracts can potentially use it in the future.