Skip to content

Conversation

@chong-he
Copy link
Member

Issue Addressed

#7756 introduces a logging issue, where the relevant log:

debug!(
%peer_id,
block_root = ?slots_by_block_root.keys(),
returned = send_blob_count,
"BlobsByRoot outgoing response processed"
);

obtains the block_root from slots_by_block_root.keys(). If the block_root is empty (block not found in the data availability checker), then the log will not show any block root:

DEBUG BlobsByRoot outgoing response processed peer_id: 16Uiu2HAmCBxs1ZFfsbAfhSA98rUUL8Q1egLPb6WpGdKZxX6HqQYX, block_root: [], returned: 4

This PR revises to return the block_root in the request as a vector of block root

@chong-he chong-he requested a review from jxs as a code owner October 27, 2025 07:01
@chong-he chong-he added ready-for-review The code is ready for review low-hanging-fruit Easy to resolve, get it before someone else does! v8.0.0 Q4 2025 Fusaka Mainnet Release labels Oct 27, 2025
request: BlobsByRootRequest,
) -> Result<(), (RpcErrorResponse, &'static str)> {
let requested_roots: Vec<Hash256> =
request.blob_ids.iter().map(|id| id.block_root).collect();
Copy link
Member Author

@chong-he chong-he Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note that the behaviour before #7756 is to always return the first block_root

let Some(requested_root) = request.blob_ids.as_slice().first().map(|id| id.block_root)

A (possibly) desirable outcome from this is, we can deduce to know that if we are rejecting BlobsByRoot request after Fulu slots, for example, if returned = 0, then we can look for the block_root(s) and check if they are all Fulu, and if they are, then we know we don't return anything

@chong-he chong-he requested a review from jimmygchen October 27, 2025 07:06
inbound_request_id: InboundRequestId,
request: BlobsByRootRequest,
) -> Result<(), (RpcErrorResponse, &'static str)> {
let requested_roots: Vec<Hash256> =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use HashSet here to deduplicate block roots?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, change in b5d7afd

@jimmygchen 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 Oct 27, 2025
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Oct 27, 2025
@mergify mergify bot added the queued label Oct 27, 2025
mergify bot added a commit that referenced this pull request Oct 27, 2025
mergify bot added a commit that referenced this pull request Oct 27, 2025
@mergify mergify bot merged commit ba706ce into sigp:unstable Oct 27, 2025
37 checks passed
@mergify mergify bot removed the queued label Oct 27, 2025
@chong-he chong-he deleted the blobbyroot-logging branch October 27, 2025 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

low-hanging-fruit Easy to resolve, get it before someone else does! ready-for-merge This PR is ready to merge. v8.0.0 Q4 2025 Fusaka Mainnet Release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants