Check data availability boundary in rpc request

[emhane]

Issue Addressed

Addressing this closed PR ethereum/consensus-specs#3211.

Proposed Changes

Check data availability boundary differently for ByRange and ByRoot request and serve client error message with regard to data availability boundary.

Additional Info

For blobs by root, should we do smthg here with the BeaconBlobOrphan column eventually, like responding "data was available"? @michaelsproul
As in the issues you mention here in the last two paragraphs #3852 (review)

[realbigsean]

Nice! Thanks for getting some clarity around this behavior

[realbigsean]

For blobs by root, should we do smthg here with the BeaconBlobOrphan column eventually, like responding "data was available"? @michaelsproul

This is an interesting question.. I think this would need to be spec'd out as a specific response code if we wanted to convey "we vouch that this was available at some point"

But this is sort of a "trust me bro" type of statement from a peer so I'm not sure it's actually worth much. I think BeaconBlobOrphan information would be useful when trying to reprocess blocks we've already imported

[pawanjay176]

Nice work! LGTM overall. Just a few nits

[emhane]

Ok so,

• Most error handling is equivalent for BlobsByRoot and BlobsByRange, those are now in a new method handle_blobs_request_error. The errors that are not included depend on the fact that BlobsByRange should respond with a chain, and BlobsByRoot can respond with nonconsecutive blocks&blobs.
• For BlobsByRoot, it's meaningless to check the early attesters cache since we will only be serving finalised block&blobs, for BlobsByRange on the other hand, it makes sense.
• I noticed that after BeaconChain::get_blobs or BeaconChain::get_block gets called, the returned value was always wrapped in a an atomically referenced counter, so I moved this into to those methods.
• I think it's appropriate to send sigterm for Server Error, also for BlobsByRoot which allows responses of nonconsecutive blocks&blobs.

[realbigsean]

So it looks like the latest commits combine some parts of blob ByRange and ByRoot error handling. I think we actually have to keep them distinct as before, otherwise I think this results in us over-using ResourceUnavailable. This is how this particular error is specified:

3: ResourceUnavailable -- the responder does not have requested resource. The response payload adheres to the ErrorMessage schema (described below). Note: This response code is only valid as a response where specified.

In ByRoot it's specified as a response here:

Clients MUST support requesting blocks and sidecars since minimum_request_epoch, where minimum_request_epoch = max(finalized_epoch, current_epoch - MIN_EPOCHS_FOR_BLOBS_SIDECARS_REQUESTS, EIP4844_FORK_EPOCH). If any root in the request content references a block earlier than minimum_request_epoch, peers SHOULD respond with error code 3: ResourceUnavailable.

This would map to our NoKzgCommitmentsFieldOnBlock error because this means it's a pre-4844 block. It would also map to our BlobsOlderThanDataAvailabilityBoundary. I spoke to Age and Diva and they suggested for the scenario where we don't have a block that we should, we should use a custom error rather than re-using BlobsUnavailable to avoid confusing these scenarios.

Confusingly, ResourceUnavailable is used in the ByRange request for the scenario where we don't have blobs that we should, and is not used otherwise.

Peers that are unable to reply to blobs sidecars requests within the MIN_EPOCHS_FOR_BLOBS_SIDECARS_REQUESTS epoch range SHOULD respond with error code 3: ResourceUnavailable. Such peers that are unable to successfully reply to this range of requests MAY get descored or disconnected at any time.

I tried to implement the ResourceUnavailable handling more precisely here: #3912

Building on top of this commit e145504

[realbigsean]

closed in favor of #3912