Provide blocks / blobs regardless of payload validation status #7198
Conversation
This file contains hidden or 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
The p2p specs state: > Non-faulty, optimistic nodes may send blocks which result in an INVALID response from an execution engine. To prevent network segregation between optimistic and non-optimistic nodes, transmission of an INVALID execution payload via the Req/Resp domain SHOULD NOT cause a node to be down-scored or disconnected. Transmission of a block which is invalid due to any consensus layer rules (i.e., not execution layer rules) MAY result in down-scoring or disconnection. They further state: > - Optimistic nodes must listen to block gossip to obtain a view of the head of the chain. > - Therefore, optimistic nodes must propagate gossip blocks. Otherwise, they'd be censoring. > - If optimistic nodes will propagate blocks via gossip, then they must respond to requests for the parent via RPC. > - Therefore, optimistic nodes must send optimistic blocks via RPC. - https://github.com/ethereum/consensus-specs/blob/v1.5.0/specs/bellatrix/p2p-interface.md#why-allow-invalid-payloads-on-the-p2p-network It is unclear where the requirement to respond to parent comes from (in theory the parent could be outside the sync range...), and it is also vague what "RPC" means. However, the intention can be inferred when looking at practical triggers for Req/Resp messages: - Status message advertising a locally unknown root - Attestation on gossip that refers to a locally unknown block - Gossip message of a descendant block with an unknown parent block - Sidecar referring to unknown block As we participate in gossip while optimistically synced, we trigger this request logic in other peers. Currently, Nimbus answers with empty resp when the primary head is optimistically synced (i.e., EL is syncing). This is actively hurting the sync performance of others, as such a resp asserts explicit non-existence of the requested data. We could improve that by answering with `3: ResourceUnavailable` error to indicate that we are actively syncing / have pruned the data, and let the peer ask someone else for the data. Further, we could also serve the known-good data up to the finalized checkpoint or the latest valid head. However, withholding data that we actually have available doesn't help the network, especially so in difficult to sync non-finality scenarios or when genesis syncing from archive node peers. Therefore, recommending to remove the checks that prevent answering Req/Resp while opt synced.
Merged
etan-status
commented
May 27, 2025
| @@ -487,10 +470,6 @@ p2pProtocol BeaconSync(version = 1, | |||
| for i in startIndex..endIndex: | |||
| for k in reqColumns: | |||
| if dag.db.getDataColumnSidecarSZ(blockIds[i].root, ColumnIndex k, bytes): | |||
| if blockIds[i].slot.epoch >= dag.cfg.DENEB_FORK_EPOCH and | |||
There was a problem hiding this comment.
Not sure why this was DENEB. DataColumns are a Fulu feature, and executionValid is bellatrix related...
There was a problem hiding this comment.
should make it FULU, might be a regression since the time we used Deneb for peerdas devnets, but overall, getting removed so won't matter anymore
There was a problem hiding this comment.
FULU makes equally no sense, same as DENEB xD But yes, proposing to remove it.
There was a problem hiding this comment.
ok yes, executionValid isn't related to Fulu in anyway, not sure why i put DENEB earlier, my bad :")
cheatfate
approved these changes
May 27, 2025
|
This is one of the reasons why |
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.
The p2p specs state:
They further state:
It is unclear where the requirement to respond to parent comes from (in theory the parent could be outside the sync range...), and it is also vague what "RPC" means. However, the intention can be inferred when looking at practical triggers for Req/Resp messages:
As we participate in gossip while optimistically synced, we trigger this request logic in other peers. Currently, Nimbus answers with empty resp when the primary head is optimistically synced (i.e., EL is syncing). This is actively hurting the sync performance of others, as such a resp asserts explicit non-existence of the requested data.
We could improve that by answering with
3: ResourceUnavailableerror to indicate that we are actively syncing / have pruned the data, and let the peer ask someone else for the data. Further, we could also serve the known-good data up to the finalized checkpoint or the latest valid head.However, withholding data that we actually have available doesn't help the network, especially so in difficult to sync non-finality scenarios or when genesis syncing from archive node peers. Therefore, recommending to remove the checks that prevent answering Req/Resp while opt synced.