Fix for #7122 Avoid attempting to serve BlobsByRange RPC requests on Fulu slots

[SunnysidedJ]

Issue Addressed

#7122 Avoid attempting to serve BlobsByRange RPC requests on Fulu slots

Proposed Changes

Added a check if Fulu slot is requested to BlobsByRange RPC handler.

Additional Info

Please see comments below

[SunnysidedJ]

A question left for penalizing peers: https://github.com/sigp/lighthouse/issues/7122#issuecomment-2811455509

[SunnysidedJ]

Test in progress as commented on the test file

[SunnysidedJ]

Current status: Issue-specific tests undergoing Implemented as per the latest spec PR but can easily changeable to other options mentioned in the PR

[SunnysidedJ]

Current status: Issue-specific tests undergoing

Having some questions atm

Sync-related questions

1. Lighthouse nodes should not request to sync Fulu slots. Where do we handle this if so?
2. Do we automatically request data columns instead?

If sync is to be implemented I'd happy to work on, but seems better to do it after https://github.com/sigp/lighthouse/pull/7352

Other

1. Do we need to implement for BlobsByRoot as well?
2. The call self.get_block_roots_for_slot_range() within the function handle_blobs_by_root_request_inner() contributes to the metric BEACON_PROCESSOR_GET_BLOCK_ROOTS_TIME. Is this something wanted?
3. The function get_block_roots_for_slot_range() isn't error-ing out when the requested range is beyond the slots the client has (I might be wrong). Is this also expected behaviour?
4. In the line in handle_blobs_by_root_request_inner() match self.chain.get_blobs(&root) {, does it actually return any blobs for Fulu slots? If this is guaranteed the change won't be needed(?)

[jimmygchen]

2. Do we automatically request data columns instead?

Yes. When making range request, we determine the batch type here: https://github.com/sigp/lighthouse/blob/a6bdc474db0147c4194123bbd71dcfb562b36ae4/beacon_node/network/src/sync/range_sync/chain.rs#L1100

1. Do we need to implement for BlobsByRoot as well?

Oh yeah, we should not try to serve BlobsByRoot for fulu slots

2. The call self.get_block_roots_for_slot_range() within the function handle_blobs_by_root_request_inner() contributes to the metric BEACON_PROCESSOR_GET_BLOCK_ROOTS_TIME. Is this something wanted?

I don't see it being used in handle_blobs_by_root_request_inner, it's only used in handing by range requests.

3. The function get_block_roots_for_slot_range() isn't error-ing out when the requested range is beyond the slots the client has (I might be wrong). Is this also expected behaviour?

Yeah it doesn't error out. It just doesn't return blocks for the slots that the node doesn't have.

4. In the line in handle_blobs_by_root_request_inner() match self.chain.get_blobs(&root) {, does it actually return any blobs for Fulu slots? If this is guaranteed the change won't be needed(?)

It doesn't return blobs, because there's no blobs in Fulu slot. However for the blobs_by_range method, we do waste resources doing lookup on the block roots before calling get_blobs. We shouldn't attempt to do this if the requested slot is in Fulu. https://github.com/sigp/lighthouse/blob/39eb8145f89e11e3fd3a6c9c3d1031e8772ac491/beacon_node/network/src/network_beacon_processor/rpc_methods.rs#L920-L921

[SunnysidedJ]

Hi @jimmygchen, thank you for detailed answers. I've made changes based on them. It'd be appreciated if LH team can have a look once available :)

[SunnysidedJ]

Status: review ready

• Added avoiding mechanism and its tests for BlobsByRoot() as well
• All tests passes

[jimmygchen]

Thanks @SunnysidedJ! Would you mind fixing the code conflicts - will take a look once it's resolved.

[mergify[bot]]

Some required checks have failed. Could you please take a look @SunnysidedJ? 🙏

[mergify[bot]]

All required checks have passed and there are no merge conflicts. This pull request may now be ready for another review.

[mergify[bot]]

This pull request has merge conflicts. Could you please resolve them @SunnysidedJ? 🙏

[mergify[bot]]

Hi @SunnysidedJ, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

[jimmygchen]

Superceded by #7756. Thanks a lot for your help and added clarification in the spec @SunnysidedJ 🙏