Do not call repeated times FCU #12091
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
potuz
added
Bug
james-prysm
reviewed
Mar 7, 2023
| } | ||
|
|
||
| return nil | ||
| // Only need to prune attestations from pool if the head has changed. | ||
| return s.pruneAttsFromPool(headBlock) |
There was a problem hiding this comment.
would it be possible to refactor and return the headblock,isNewHead,isNewProposer instead? this way we can also use it for the postBlockOperations?
There was a problem hiding this comment.
I dont think we should alter the signature. Some options are:
- call
postBlockOperationshere - call
postBlockOperationsdownstream but return nil on non-canonical block
terencechain
reviewed
Mar 7, 2023
Comment on lines
63
to
65
| if !isNewHead { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This is the most significant change in this PR
terencechain
previously approved these changes
Mar 7, 2023
terencechain
approved these changes
Mar 7, 2023
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.
This PR, with deceptively low line count, addresses a few issues
It is important to notice that this PR effectively changes the way we deal with the engine API under some circumstances. For example, when inserting an early non-canonical block, we will not send an FCU even if proposing next slot, this is deferred until 4 seconds into the slot.
There are two main issues that are left unattended
on_tickwill race to process attestations and update head. On the one hand a second call to process attestation is almost a noop. The second call to updateHead is a noop.Accompanying doc with all edge cases analysis https://www.notion.so/arbitrum/Late-Block-Reorgs-c35737afbf4b4631b98b666872dd565c