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

SunnysidedJ · 2025-04-17T01:22:38Z

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 · 2025-04-17T01:26:40Z

A question left for penalizing peers: #7122 (comment)

beacon_node/network/src/network_beacon_processor/tests.rs

SunnysidedJ · 2025-04-17T01:30:32Z

Test in progress as commented on the test file

dapplion · 2025-04-23T14:44:37Z

beacon_node/network/src/network_beacon_processor/tests.rs

@@ -1,4 +1,4 @@
-#![cfg(not(debug_assertions))] // Tests are too slow in debug.
+//#![cfg(not(debug_assertions))] // Tests are too slow in debug.


Unwanted diff

dapplion · 2025-04-23T14:45:33Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

+                RpcErrorResponse::InvalidRequest,
+                "Req including Fulu slots",
+            ))
+        }


Hmm, rethinking this maybe this is too punishing? Why not allow the request to creep into Fulu and just check that the start slot is in Deneb. It's fine to return empty for slots in Fulu.

Lighthouse only does by_range requests for a single epoch, but other clients may have different logic.

Yes, I agree that it could be bit too punishing. The issue was vague in a way that we didn't set how much punishment we will give. Do you think it is okay to accept the request as long as it contains pre-Fulu slots?

Also, it seems like there's no check from Lighthouse to not send Fulu slots as left on the comment #7122 (comment). So, we could potentially be penalized by ourselves

Feels like something that should be clarified in the spec to align behaviour on all clients. Do you want to raise an issue to the specs?

Sure! Just raised a PR to the specs ethereum/consensus-specs#4286

Changed as per the spec PR to not punish and return empty for Fulu slots

SunnysidedJ · 2025-04-30T12:29:58Z

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -7146,6 +7146,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
        let end_slot = start_slot.saturating_add(count);
        let mut roots = vec![];

+        // let's explicitly check count = 0 since it's a public function for readabiilty purpose.


The original code handles edge cases.

SunnysidedJ · 2025-04-30T12:30:34Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

@@ -880,8 +880,16 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
            "Received BlobsByRange Request"
        );

+        if req.count == 0 {


Again, let's handle corner cases explicitly at some point

Will apply the same to other similar functions in this file if agreed

SunnysidedJ · 2025-04-30T12:40:03Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

+            let fulu_epoch = self.chain.spec.fulu_fork_epoch.unwrap();
+            let fulu_start_slot = fulu_epoch.start_slot(T::EthSpec::slots_per_epoch());
+            // See the justification for the formula in PR https://github.com/sigp/lighthouse/pull/7328
+            req.count = fulu_start_slot.as_u64().saturating_sub(req.start_slot);


Justification:
Let's consider every case one-by-one.

Range requested lies entirely on pre-Fulu
Case req.start_slot + req.count <= fulu_start_slot then
req.count = req.count

Range requested lies entirely on Fulu
Case req.start_slot >= fulu_start_slot then
req.count = 0

Range requested lies on both pre-Fulu and Fulu
Case req.start_slot < fulu_start_slot then
req.count = req.count - ((req.start_slot + req.count) - fulu_start_slot)
req.count = fulu_start_slot - req.start_slot

Union of all cases results in req.count = fulu_start_slot.saturating_sub(req.start_slot)

SunnysidedJ · 2025-04-30T12:42:15Z

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 · 2025-04-30T12:54:27Z

Current status: Issue-specific tests undergoing

Having some questions atm

Sync-related questions

Lighthouse nodes should not request to sync Fulu slots. Where do we handle this if so?
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 #7352

Other

Do we need to implement for BlobsByRoot as well?
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?
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?
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 · 2025-05-05T06:24:14Z

Do we automatically request data columns instead?

Yes. When making range request, we determine the batch type here:

lighthouse/beacon_node/network/src/sync/range_sync/chain.rs

Line 1100 in a6bdc47

let batch_type = network.batch_type(epoch);

Do we need to implement for BlobsByRoot as well?

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

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.

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.

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.

lighthouse/beacon_node/network/src/network_beacon_processor/rpc_methods.rs

Lines 920 to 921 in 39eb814

    
           let block_roots = 
        
               self.get_block_roots_for_slot_range(req.start_slot, req.count, "BlobsByRange")?;

init commit, test fails

2c0311e

SunnysidedJ requested a review from jxs as a code owner April 17, 2025 01:22

SunnysidedJ commented Apr 17, 2025

View reviewed changes

beacon_node/network/src/network_beacon_processor/tests.rs Outdated Show resolved Hide resolved

eserilev added das Data Availability Sampling fulu Required for the upcoming Fulu hard fork labels Apr 17, 2025

dapplion reviewed Apr 23, 2025

View reviewed changes

SunnysidedJ mentioned this pull request Apr 25, 2025

Fulu BlobSidecarsByRange/Root Deprecation Support ethereum/consensus-specs#4286

Merged

jimmygchen added the work-in-progress PR is a work-in-progress label Apr 30, 2025

implementation complete with existing test passed

f79d741

SunnysidedJ commented Apr 30, 2025

View reviewed changes

tests blocked. non-zero start slot not working

6d18d89

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SunnysidedJ commented Apr 17, 2025

SunnysidedJ commented Apr 17, 2025

SunnysidedJ commented Apr 17, 2025

dapplion Apr 23, 2025

dapplion Apr 23, 2025 •

edited

Loading

SunnysidedJ Apr 23, 2025

dapplion Apr 24, 2025

SunnysidedJ Apr 25, 2025 •

edited

Loading

SunnysidedJ Apr 30, 2025

SunnysidedJ Apr 30, 2025

SunnysidedJ Apr 30, 2025

SunnysidedJ Apr 30, 2025

SunnysidedJ Apr 30, 2025

SunnysidedJ commented Apr 30, 2025 •

edited

Loading

SunnysidedJ commented Apr 30, 2025

jimmygchen commented May 5, 2025 •

edited

Loading

		@@ -1,4 +1,4 @@
		#![cfg(not(debug_assertions))] // Tests are too slow in debug.
		//#![cfg(not(debug_assertions))] // Tests are too slow in debug.

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

Are you sure you want to change the base?

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

Conversation

SunnysidedJ commented Apr 17, 2025

Issue Addressed

Proposed Changes

Additional Info

SunnysidedJ commented Apr 17, 2025

SunnysidedJ commented Apr 17, 2025

Choose a reason for hiding this comment

dapplion Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunnysidedJ Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunnysidedJ commented Apr 30, 2025 • edited Loading

SunnysidedJ commented Apr 30, 2025

jimmygchen commented May 5, 2025 • edited Loading

dapplion Apr 23, 2025 •

edited

Loading

SunnysidedJ Apr 25, 2025 •

edited

Loading

SunnysidedJ commented Apr 30, 2025 •

edited

Loading

jimmygchen commented May 5, 2025 •

edited

Loading