Add circuit breaker for builder #4488
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
g11tech
force-pushed
the
g11tech/builder-circuit-breaker
branch
from
7771b13
to
a82e277
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
dapplion
reviewed
Aug 29, 2022
| // an api call to the builder to check its status, so we will not await for | ||
| // its completion here as we want to trigger local execution here without | ||
| // delay | ||
| this.chain.updateBuilderStatus(clockSlot).catch((e) => { |
There was a problem hiding this comment.
Without a more in-depth review this looks problematic. Since this is async there's a racing condition on producing the block and updating the status. Could be that the builder is disabled too late and the block is already produced?
Why not check at the end of every epoch (or some point, say at 2nd slot) the status of the previous epoch and update the status of the builder with it?
- That allows to have constant metrics of status
- You can subscribe to block event instead of
getSlotsPresent()and have real-time cumulative status of blocks per epoch, which can also go into a metric
There was a problem hiding this comment.
👍 for counting the slots via event and the corresponding metric
However with regard to calling updateBuilderStatus here: for disabling, this is synchronous because the builder's check status api is only being called for enabling, which if it doesn't respond within a second should anyway be ok to have builder disabled for the next slot's block.
I think its a good idea to check for builder status here before proposing next slot.
There was a problem hiding this comment.
However with regard to calling updateBuilderStatus here: for disabling, this is synchronous because the builder's check status api is only being called for enabling
Got it, then can you find a way to make it more obvious on the code that disabling is sync?
g11tech
force-pushed
the
g11tech/builder-circuit-breaker
branch
from
1f8d9de
to
4afca90
Compare
dapplion
previously approved these changes
Aug 30, 2022
dapplion
approved these changes
Aug 30, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
In case there is a malevolent or buggy builder, it can take down the entire network by not propagating the blocks
This PR add a circuit breaker for builder in bad network conditions
Description
Closes #4483
Manual Test chronology on
ropsten-1