fix(blockchain): modify location and error handling of forkchoiceUpdated engine API calls
#2394
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2394 +/- ##
==========================================
+ Coverage 27.08% 27.11% +0.02%
==========================================
Files 351 350 -1
Lines 15543 15528 -15
Branches 20 20
==========================================
Hits 4210 4210
+ Misses 11130 11115 -15
Partials 203 203
|
shotes
force-pushed
the
fix-ErrAcceptedPayloadStatus-handling
branch
from
b55b546 to
6791ec6
Compare
| @@ -89,13 +89,9 @@ func (ee *Engine) NotifyForkchoiceUpdate( | |||
| switch { | |||
| // We do not bubble the error up, since we want to handle it | |||
| // in the same way as the other cases. | |||
| case errors.IsAny( | |||
| err, | |||
| engineerrors.ErrAcceptedPayloadStatus, | |||
There was a problem hiding this comment.
ErrAcceptedPayloadStatus does not get returned by notifyForkchoiceUpdated engine API calls.
shotes
changed the title
forkchoiceUpdated engine API calls
| engineerrors.ErrAcceptedPayloadStatus, | ||
| engineerrors.ErrSyncingPayloadStatus, | ||
| ): | ||
| case errors.Is(err, engineerrors.ErrSyncingPayloadStatus): |
There was a problem hiding this comment.
If the block is syncing, we cannot verify it. This change requires more thought. How does this effect a new node syncing to the chain? Will this break that?
There was a problem hiding this comment.
After thinking about this more, I think this is right. Please tell me why this is wrong.
- If the EL is syncing, we cannot verify the block. Therefore we should not give it the OK in a proposal.
- If we are a simple node catching up to the network, replaying blocks, then
FinalizeBlockwill be executed sequentially, guaranteeing that the EL always has the previous block. Therefore it should never be possible to get the SYNCING status. - In fact, it should be theoretically impossible for this status to happen, as we now ALWAYS send the payload via
newPayloadright before theforkchoiceUpdated. So it should not be possible for the EL to not have the previous block.
There was a problem hiding this comment.
There was a problem hiding this comment.
If the block is syncing, we cannot verify it. This change requires more thought. How does this effect a new node syncing to the chain? Will this break that?
We have had situations where the EL cannot find peers, and as such is stuck in a syncing state. Previously, consensus would continue but now it will error out. If we introduce this, we need to have some sort of indefinite looping that waits until the EL is synced.
There was a problem hiding this comment.
If we are a simple node catching up to the network, replaying blocks, then FinalizeBlock will be executed sequentially, guaranteeing that the EL always has the previous block. Therefore it should never be possible to get the SYNCING status.
Theoretically this is the case but we observed that when syncing full nodes without EL peers, it was returning "SYNCING" status - something to investigate.
| // Since we have single slot finality, the previous block is already | ||
| // considered final from cometBFT perspective. The newPayload being | ||
| // submitted from this proposal should be set as the new head of the | ||
| // chain, and we can update the finalized block with the EL. We send the | ||
| // FCU now which fully verifies that the block is VALID or INVALID. |
There was a problem hiding this comment.
This bubbles up to the s.stateProcessor.Transition call which is also called in ProcessProposal.
Previously we would only set the FCU head in the EL after FinalizeBlock as we confirmed with Informal that this was never going to revert.
Now it seems possible that the "head", i.e. "latest" tag could revert given that we are setting it before FinalizeBlock. This would require that anyone waiting for finality must wait for the "finalized" tag in the EL which is friction we would have to discuss with DX
| switch err := sp.processExecutionPayload(ctx, st, blk); { | ||
| case err == nil: | ||
| // keep going with the processing | ||
| case errors.Is(err, engineerrors.ErrAcceptedPayloadStatus): |
There was a problem hiding this comment.
agree with removal here. this was useless with the comment being inconsistent
This PR is to resolve issue #2405
There are several fixes in here:
NotifyForkchoiceUpdateimmediately afterVerifyAndNotifyNewPayload. This validates that the EL is willing, able, and DOES, update the head of the chain to the proposed payload. This is the primary fix and resolves handling around theACCEPTEDstatus. This also enforces that errors in the forkchoiceUpdated propagate to consensus and aren't ignored.Bto setXas the final block andYas the head. After sending this forkchoice update (in FinalizeBlock), the round does not reach consensus for some reason. Then the optimistic builder also fails a proposal that triggers an optimistic rebuild inrebuildPayloadForRejectedBlock(). This will attempt to setW(the block beforeX) as the final block, while settingXas the head in order to rebuild a block onX.RequestPayloadSync(), which would result in the same issue.Edit: my concerns about 1 and 2 are incorrect. In the live codebase (not this PR), fork choice updates are only sent in FinalizeBlock, and those block hashes are never to be changed, even if FinalizeBlock is called multiple times.