fix: ask the source of the header for the block body first#1152
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a source-peer-first block-body retrieval flow: a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Handler
participant GossipClient
participant Peer as "Peer (peer_id)"
participant PeerList as "PeerList / Network"
Handler->>GossipClient: pull_block_body(header, use_trusted?, source_peer_id)
GossipClient->>Peer: pull_block_body_from_peer(header, peer, peer_list)
Peer-->>GossipClient: Accepted(Some BlockBody) / Accepted(None) / Rejected(reason) / Error
alt Accepted(Some)
GossipClient-->>Handler: (peer_id, BlockBody)
else Accepted(None)
GossipClient->>PeerList: mark peer lacking block body / return PeerNetworkError
GossipClient-->>Handler: PeerNetworkError (MissingBody)
else Rejected(reason)
GossipClient->>PeerList: maybe trigger handshake or mark offline
GossipClient-->>Handler: PeerNetworkError (Rejected(reason))
else Error (handshake required)
GossipClient->>PeerList: start handshake
GossipClient->>Peer: retry request
Peer-->>GossipClient: result
GossipClient-->>Handler: final Result or PeerNetworkError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRsSuggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/p2p/src/gossip_data_handler.rs (1)
1251-1251: 🧹 Nitpick | 🔵 Trivial
source_peer_idinside the loop shadows the function parameterThe match binding
Ok((source_peer_id, irys_block_body))reuses the name of the function parameter, which refers to the original header source peer. Inside the loop arm the name correctly identifies the network peer that served the body, but the shadowing makes it non-obvious whether the scoring penalty at Line 1269 targets the original source or the network peer. Renaming the inner binding removes the ambiguity.♻️ Proposed rename
- Ok((source_peer_id, irys_block_body)) => { + Ok((network_peer_id, irys_block_body)) => { match irys_block_body.tx_ids_match_the_header(header) { Ok(true) => { debug!( "Fetched block body for block {} height {} from peer {:?}", - block_hash, header.height, source_peer_id + block_hash, header.height, network_peer_id ); return Ok((*irys_block_body).clone()); } Ok(false) => { // ... self.peer_list.decrease_peer_score_by_peer_id( - &source_peer_id, + &network_peer_id, // ... ); debug!( - "Penalized peer {} for serving bad block body", source_peer_id + "Penalized peer {} for serving bad block body", network_peer_id ); - failed_attempts.push((Some(source_peer_id), error)); + failed_attempts.push((Some(network_peer_id), error)); } Err(e) => { // ... - failed_attempts.push((Some(source_peer_id), error)); + failed_attempts.push((Some(network_peer_id), error)); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/p2p/src/gossip_data_handler.rs` at line 1251, The match arm currently binds Ok((source_peer_id, irys_block_body)) which shadows the function parameter source_peer_id and obscures which peer is used when applying the scoring penalty later; rename the inner binding (e.g., to served_peer_id or body_source_peer) in the Ok((..., ...)) pattern and update subsequent uses in that arm (including where the score penalty is applied) so the function parameter and the peer that served the body are distinct and unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/p2p/src/gossip_client.rs`:
- Around line 1168-1200: pull_block_body_from_peer currently collapses all
GossipResponse::Rejected cases into a generic FailedToRequestData and misses the
side-effects implemented in pull_block_header_from_peer; modify
pull_block_body_from_peer so that on GossipResponse::Rejected it mirrors
pull_block_header_from_peer: when the reason is HandshakeRequired call
initiate_handshake(peer.0).await, sleep for one second, then retry the pull
(e.g., re-call pull_data_and_update_the_score or re-run
pull_block_body_from_peer once) before failing; when the reason is
GossipDisabled call peer_list.set_is_online_by_peer_id(peer.0, false) (or the
existing set_is_online_by_peer_id method) and then return the appropriate
FailedToRequestData; for other rejection reasons preserve the existing error
return. Use the same helper names seen in the file:
pull_data_and_update_the_score, initiate_handshake, set_is_online_by_peer_id,
and pull_block_body_from_peer for the retry to keep behavior consistent with
pull_block_header_from_peer.
In `@crates/p2p/src/gossip_data_handler.rs`:
- Around line 1182-1188: The code silently skips the source-peer-first fetch
when peer_list.peer_by_id(&source_peer_id) returns None; add a debug-level log
that reports the missing source peer so evictions between header receipt and
body fetch are visible. Modify the block around
peer_list.peer_by_id(&source_peer_id) in gossip_data_handler.rs (the code that
constructs source_peer from source_peer_id and source_peer_item) to log a
message like "source peer {source_peer_id} not found in peer_list, skipping
source-first fetch" when peer_by_id returns None, referencing peer_list,
source_peer_id, and the peer_by_id call so the absence is clearly recorded.
- Line 1183: Replace the nonconforming call to peer_by_id with the established
get_peer method for consistency: locate the lookup expression using
self.peer_list.peer_by_id(&source_peer_id) in gossip_data_handler.rs (around the
handling block that sets source_peer_item) and change it to use
self.peer_list.get_peer(&source_peer_id) so it matches other lookups (e.g., the
ones at lines referencing get_peer); no behavior change required beyond the
method name swap.
---
Outside diff comments:
In `@crates/p2p/src/gossip_data_handler.rs`:
- Line 1251: The match arm currently binds Ok((source_peer_id, irys_block_body))
which shadows the function parameter source_peer_id and obscures which peer is
used when applying the scoring penalty later; rename the inner binding (e.g., to
served_peer_id or body_source_peer) in the Ok((..., ...)) pattern and update
subsequent uses in that arm (including where the score penalty is applied) so
the function parameter and the peer that served the body are distinct and
unambiguous.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/p2p/src/gossip_client.rs`:
- Around line 1168-1243: The three functions pull_block_body_from_peer,
pull_block_header_from_peer, and pull_transaction_from_peer duplicate the same
retry-with-handshake logic; extract that into a single helper (e.g.
pull_data_with_retry or pull_data_from_peer_with_retry) that takes the
GossipDataRequestV2, the peer tuple, peer_list, and a mapping closure
Fn(GossipDataV2) -> Result<T, PeerNetworkError>; move the for attempt in 0..2
loop, the call to pull_data_and_update_the_score, the GossipResponse handling
(Accepted(Some/None), Rejected with RejectionReason cases including
HandshakeRequired sleep + peer_list.initiate_handshake, GossipDisabled ->
set_is_online_by_peer_id, and error mapping) into that helper and return
Ok((peer_id, mapped_value)) or appropriate PeerNetworkError so the three
original methods simply build their request, provide a small mapper that
extracts/validates the expected variant, and call the new helper.
This was referenced Feb 19, 2026
Merged
This was referenced Mar 31, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the changes
This PR modifies block body pulling function to ask the header source first, reducing network errors
Related Issue(s)
Checklist
Additional Context
Summary by CodeRabbit
New Features
Bug Fixes