Avoid attempting to serve blobs after Fulu fork #7756

chong-he · 2025-07-18T01:33:58Z

Issue Addressed

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

michaelsproul · 2025-07-25T06:13:25Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

+                if let Some(fulu_slot) = fulu_start_slot {
+                    if blob.slot() >= fulu_slot {


This can be simplified slightly using fulu_start_slot.is_some_and(|fulu_slot| blob.slot() >= fulu_slot).

The fulu_slot is used in the logging below. So after changing, the compiler is saying fulu_sot not found.

Should I remove fulu_slot from the logging?

michaelsproul · 2025-07-25T06:13:40Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

+                                if let Some(fulu_slot) = fulu_start_slot {
+                                    if blob_sidecar.slot() >= fulu_slot {


jimmygchen · 2025-07-28T05:40:50Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

            // First attempt to get the blobs from the RPC cache.
            if let Ok(Some(blob)) = self.chain.data_availability_checker.get_blob(id) {
+                // Check if the blob requested is from a Fulu slot, if so, skip the current blob id and proceed to the next


I think if the block is after fulu slot, this condition will never satisfy, as the result will always be Ok(None) as we never store blobs from Fulu. I don't think we need to handle it here as it should be unreachable.

Revised in bf20880

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

jimmygchen · 2025-07-28T06:02:47Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

@@ -306,6 +326,20 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                    Ok(blobs_sidecar_list) => {
                        'inner: for blob_sidecar in blobs_sidecar_list.iter() {
                            if blob_sidecar.index == *index {
+                                // Same logic as above to check for Fulu slot
+                                if let Some(fulu_slot) = fulu_start_slot {


This will also be unreachable.

The purpose to avoid serving post-fulu slots in the byRange method is to save us work from doing block roots lookup - it doesn't seem as relevant here, as we've already hit the database and incurred the cost.

The spec also says the following, so we can't really penalise these peers during this deprecation period

Clients SHOULD NOT penalize peers for requesting blob sidecars from FULU_FORK_EPOCH.

I think we could probably check all the caches to see if we have the block before hitting the database?

Revised in bf20880, trying to avoid touching the database

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

jimmygchen · 2025-07-28T06:28:23Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

@@ -884,6 +918,36 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
        );

        let request_start_slot = Slot::from(req.start_slot);
+        // This variable may only change when the request_start_slot + req.count spans across the Fulu fork slot
+        let mut effective_count = req.count;


You could make the if / else block return the count, and that would allow you to remove the mut

Try to revise this in 32c3ab0, not sure if it is good?

yep that's better!

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

jimmygchen

Hi @chong-he I've added some comments.
I think it might be worth adding something similar to data columns methods, to avoid block roots retrieval if the slots are before Fulu

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

Lines 1064 to 1085 in befef72

    
               pub fn handle_data_columns_by_range_request_inner( 
        
                   &self, 
        
                   peer_id: PeerId, 
        
                   inbound_request_id: InboundRequestId, 
        
                   req: DataColumnsByRangeRequest, 
        
               ) -> Result<(), (RpcErrorResponse, &'static str)> { 
        
                   debug!( 
        
                       %peer_id, 
        
                       count = req.count, 
        
                       start_slot = req.start_slot, 
        
                       "Received DataColumnsByRange Request" 
        
                   ); 
        
                   // Should not send more than max request data columns 
        
                   if req.max_requested::<T::EthSpec>() > self.chain.spec.max_request_data_column_sidecars { 
        
                       return Err(( 
        
                           RpcErrorResponse::InvalidRequest, 
        
                           "Request exceeded `MAX_REQUEST_BLOBS_SIDECARS`", 
        
                       )); 
        
                   } 
        
                   let request_start_slot = Slot::from(req.start_slot);

mergify · 2025-08-07T07:11:41Z

Some required checks have failed. Could you please take a look @chong-he? 🙏

jimmygchen · 2025-08-19T05:26:05Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

+        let request_start_epoch = request_start_slot.epoch(T::EthSpec::slots_per_epoch());
+        // Should not send more than max request blob sidecars
+        if req.max_blobs_requested(request_start_epoch, &self.chain.spec)
+            > self.chain.spec.max_request_blob_sidecars_electra


should we use the spec.max_request_blob_sidecars(fork_name) function here?
keeping those fields private means we don't leak the logic elsewhere -if we ever need to use a different config value for a different fork, we only need to update the above function.

Thanks, revised in 0cbd9fd
I didn't notice this function is available, when I use the Electra one I was feeling unsure about it

jimmygchen · 2025-08-19T05:37:34Z

beacon_node/network/src/network_beacon_processor/tests.rs

+}
+
+#[tokio::test]
+async fn test_blobs_by_range_spans_fulu_fork() {


I don't think this test would fail before your PR?
but i think it's useful that it covers blobs by range works before Fulu

Yes this test wouldn't fail even without this PR change, which is why I think the test is not robust enough.

I have already started from mid-epoch slot, and the blob request spans across fork boundary. I wonder if I am missing something in the test?

jimmygchen · 2025-08-19T05:43:04Z

beacon_node/network/src/network_beacon_processor/tests.rs

+
+#[tokio::test]
+async fn test_blobs_by_root_post_fulu_should_return_empty() {
+    // Only test for Fulu fork


i think it would be worth adding a test to confirm it still work right before fulu fork?

Added a test in 0cbd9fd

jimmygchen · 2025-08-19T05:45:40Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

@@ -268,21 +268,64 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
        inbound_request_id: InboundRequestId,
        request: BlobsByRootRequest,
    ) -> Result<(), (RpcErrorResponse, &'static str)> {
-        let Some(requested_root) = request.blob_ids.as_slice().first().map(|id| id.block_root)
+        let Some(_requested_root) = request.blob_ids.as_slice().first().map(|id| id.block_root)


This block is now redundant and can be removed

Sure. The reason I am keeping this is, if we remove this, then it wouldn't have the check that if the request is empty before proceeding. wdyt?

Anyway I removed it in 0cbd9fd

jimmygchen · 2025-08-19T05:47:49Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

        let mut blob_list_results = HashMap::new();
+        let mut retrieve_blob_slot = HashMap::new();


I think it's fine being a bit more verbose here to help readability - what about using the map naming convention values_by_key, e.g. slots_by_block_root?

Rename the variable as suggested

jimmygchen · 2025-08-19T05:52:43Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

+                    retrieve_blob_slot.insert(*root, slot);
+                    Some(slot)
+                } else {
+                    // Blobs not found in cache, return None to avoid hitting the database


This is the case where block is not found in cache, and we just need to hit the database right?

Revise the comment to be clearer, thanks!

jimmygchen · 2025-08-19T05:56:27Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

+                                        "BlobsByRoot request is at or after Fulu slot, returning empty response"
+                                    );
+                                    break 'inner;
+                                }


I think we can remove this block - if we have the blob then it means fulu is not activated and there's no need to check for fulu slot

Removed in 0cbd9fd

jimmygchen · 2025-08-19T06:00:17Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

+                debug!(
+                    %peer_id,
+                    %request_start_slot,
+                    %fulu_start_slot,


i think we can remove all the fulu start slot logging to reduce the logging noise - this doesn't give us much during debugging as there's many easy way to find out, e.g. /config/spec, network config etc

All fulu-related logging removed in 0cbd9fd

jimmygchen · 2025-08-19T06:12:46Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

+                %block_root,
+                ?blobs_indices,
+                returned = send_blob_count,
+                "BlobsByRoot outgoing response processed"


This log isn't accurate because send_blob_count is the total number of returned responses.

Maybe we could just log one entry, and the list of unique roots requested?

Only log once after the revision 0cbd9fd

chong-he · 2025-08-20T02:44:05Z

Thanks for the detailed review @jimmygchen. I have address all comments as above.

chong-he added 3 commits July 17, 2025 12:52

add fulu_start_slot empty response

e0aa917

BlobsByRange that spans Fulu fork

4f8be4f

rename

cbc4306

chong-he requested a review from jxs as a code owner July 18, 2025 01:33

chong-he added the work-in-progress PR is a work-in-progress label Jul 18, 2025

chong-he changed the title ~~Das blobs by range~~ Do not response to blobs request after Fulu fork Jul 18, 2025

chong-he changed the title ~~Do not response to blobs request after Fulu fork~~ Avoid attempting to serve Blobs after Fulu fork Jul 18, 2025

chong-he changed the title ~~Avoid attempting to serve Blobs after Fulu fork~~ Avoid attempting to serve blobs after Fulu fork Jul 18, 2025

chong-he added 3 commits July 18, 2025 21:52

Add BlobsByRange test

b9149f6

add Fuly BlobsByRoot

befef72

test

1832273

chong-he added ready-for-review The code is ready for review fulu Required for the upcoming Fulu hard fork and removed work-in-progress PR is a work-in-progress labels Jul 22, 2025

chong-he requested a review from jimmygchen July 22, 2025 01:15

jimmygchen mentioned this pull request Jul 22, 2025

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

Closed

michaelsproul reviewed Jul 25, 2025

View reviewed changes

jimmygchen reviewed Jul 28, 2025

View reviewed changes

beacon_node/network/src/network_beacon_processor/rpc_methods.rs Show resolved Hide resolved

jimmygchen reviewed Jul 28, 2025

View reviewed changes

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

jimmygchen reviewed Jul 28, 2025

View reviewed changes

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

jimmygchen reviewed Jul 28, 2025

View reviewed changes

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

jimmygchen reviewed Jul 28, 2025

View reviewed changes

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

jimmygchen requested changes Jul 28, 2025

View reviewed changes

jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jul 28, 2025

fix BlobsByRoot

bf20880

chong-he added the ready-for-review The code is ready for review label Aug 6, 2025

Merge branch 'unstable' into das-BlobsByRange

acc9a5a

mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Aug 7, 2025

lint

1fe8f82

chong-he added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 7, 2025

jimmygchen added the v8.0.0 Q4 2025 Fusaka Mainnet Release label Aug 12, 2025

chong-he and others added 2 commits August 13, 2025 11:14

Merge branch 'unstable' into das-BlobsByRange

3d394a9

if loop lint

00687f2

jimmygchen reviewed Aug 19, 2025

View reviewed changes

jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Aug 19, 2025

revise

0cbd9fd

chong-he added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 20, 2025

jimmygchen added v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky v8.0.0 Q4 2025 Fusaka Mainnet Release and removed v8.0.0 Q4 2025 Fusaka Mainnet Release v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky labels Aug 20, 2025

		if let Some(fulu_slot) = fulu_start_slot {
		if blob.slot() >= fulu_slot {

		if let Some(fulu_slot) = fulu_start_slot {
		if blob_sidecar.slot() >= fulu_slot {

	pub fn handle_data_columns_by_range_request_inner(
	&self,
	peer_id: PeerId,
	inbound_request_id: InboundRequestId,
	req: DataColumnsByRangeRequest,
	) -> Result<(), (RpcErrorResponse, &'static str)> {
	debug!(
	%peer_id,
	count = req.count,
	start_slot = req.start_slot,
	"Received DataColumnsByRange Request"
	);

	// Should not send more than max request data columns
	if req.max_requested::<T::EthSpec>() > self.chain.spec.max_request_data_column_sidecars {
	return Err((
	RpcErrorResponse::InvalidRequest,
	"Request exceeded `MAX_REQUEST_BLOBS_SIDECARS`",
	));
	}

	let request_start_slot = Slot::from(req.start_slot);

		let mut blob_list_results = HashMap::new();
		let mut retrieve_blob_slot = HashMap::new();

Avoid attempting to serve blobs after Fulu fork #7756

Are you sure you want to change the base?

Avoid attempting to serve blobs after Fulu fork #7756

Uh oh!

Conversation

chong-he commented Jul 18, 2025

Issue Addressed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmygchen Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jimmygchen left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmygchen Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chong-he commented Aug 20, 2025

Uh oh!

jimmygchen Jul 28, 2025 •

edited

Loading

jimmygchen Aug 19, 2025 •

edited

Loading