Wire up blob fetching with request manager#4527
Wire up blob fetching with request manager#4527tersec merged 11 commits intostatus-im:unstablefrom henridf:eip4844-sync
Conversation
| if not(isNil(peer)): | ||
| rman.network.peerPool.release(peer) | ||
|
|
||
| proc fetchAncestorBlocksAndBlobsFromNetwork(rman: RequestManager, |
There was a problem hiding this comment.
(This has a lot of duplication with fetchAncestorBlocksFromNetwork, but there isn't a shared chunk that can be cleanly factored out.)
|
Not sure why just one build is failing, but this message makes me wonder if it's some infra issue
|
|
CI is green. |
|
|
||
| let start = SyncMoment.now(0) | ||
|
|
||
| let getBlobs = rman.isSlotWithBlobs(rman.dag.head.slot) |
There was a problem hiding this comment.
Is this the only reason RequestManager needs a reference to the DAG?
There was a problem hiding this comment.
Yes. I don't really like bringing in that reference, but the RequestManager needs to know where the head is at.
There was a problem hiding this comment.
I'm not sure I understand why though. My read of https://github.com/ethereum/consensus-specs/blob/dev/specs/eip4844/p2p-interface.md#beaconblockandblobssidecarbyroot-v1 is that /eth2/beacon_chain/req/beacon_block_and_blobs_sidecar_by_root/1/ will only work for EIP4844 blocks, so even if the head has s.epoch >= rman.dag.cfg.EIP4844_FORK_EPOCH (isSlotWithBlobs), Nimbus might request a root which is pre-EIP4844.
This could happen a few ways, e.g., a nonfinalizing network where one fork advances through EIP4844_FORK_EPOCH, and another has been a side chain, but then the network as a whole reorgs to a new chain -- but this node doesn't have those blocks or blobs, so it has to request them. If it looks like
slot 600: ancestor of current head
slot 610: ancestor of another sidechain
-------------------
EIP-4844 fork epoch
-------------------
slot 700: current head, based on 600
slot 710: future reorg target based on 610 (which this node rejected, for any of several reasons. either way, needs to request it)
But the current logic requests from slot 610 via an EIP4844 req/resp protocol endpoint which won't support it, from what I can tell, because once its head reaches EIP4844, it will become unable to request pre-EIP4844 slots in the event of such reorgs.
This isn't a major hazard on a finalizing network, but I'm not sure it's a reliable heuristic on a nonfinalizing network.
One can also construct failure scenarios the opposite direction -- this node's head is stuck just before EIP4844 fork epoch, while a side chain it's not for whatever reason following reaches/passes the EIP4844 fork epoch.
Rather, is this something that needs to be know specifically root by root, or am I missing something, and it's somehow possible to extrapolate from the DAG head to all the roots one might request?
There was a problem hiding this comment.
Hmm, I had come up with simpler scenarios, and had sort of convinced myself they would work, but I realize now that's not the case. Even in this simpler example without forks:
------------------
slot 599: current head
slot 600: EIP-4844 fork epoch
slot 601
slot 602: we received this block and need to fill in 600,601
------------------
where we would request 600 and 601 using the pre-EIP488 RPC, which would give no results. (from https://github.com/ethereum/consensus-specs/blob/dev/specs/eip4844/p2p-interface.md#beaconblocksbyroot-v2: "After EIP4844_FORK_EPOCH, BeaconBlocksByRootV2 is replaced by BeaconBlockAndBlobsSidecarByRootV1").
And so we get stuck, I think.
It seems like there's a fundamental issue here, since we have no idea of knowing based on a root whether it's pre or post a particular epoch.
The only thing I can think of is some sort of fallback where we try the other endpoint if one doesn't return results. Not great but I'm not seeing any other way.
There was a problem hiding this comment.
Yeah, what are other CLs doing, I guess? Seems ugly.
There was a problem hiding this comment.
Thinking the same thing. Will be a good discussion topic soon.
There was a problem hiding this comment.
@tersec: in 26b8cc9, I modified the "fetch blocks vs blobs" logic. Per the comment:
# As soon as EIP4844_FORK_EPOCH comes around in wall time, we
# switch to requesting blocks and blobs. In the vicinity of the
# transition, that means that we *may* request blobs for a
# pre-eip4844. In that case, we get ResourceUnavailable from the
# peer and fall back to requesting blocks only.
Co-authored-by: tersec <tersec@users.noreply.github.com>
| # transition, that means that we *may* request blobs for a | ||
| # pre-eip4844. In that case, we get ResourceUnavailable from the | ||
| # peer and fall back to requesting blocks only. | ||
| let getBlobs = rman.isBlobsTime(rman.dag.head.slot) |
There was a problem hiding this comment.
This isn't wall time -- the comment's logic I agree with, but that's not what this does. As a bonus, by following wall time rather than head, no need to pass the DAG anymore.
There was a problem hiding this comment.
Removed the slot parameter to isBlobsTime (which was looking at wall clock, the slot parameter was leftover from before).
Re the dag, we do at least need EIP4844_FORK_EPOCH. I could just pass the RuntimeConfig (dag.cfg) to the request manager.
Head branch was pushed to by a user without write access
2 participants
(Putting this up while I work on using BlobsSidecarsbyRange in the sync manager, which is proving to be a challenge).