[MegaRedHand]

🗒️ Description / Motivation

A root sits in pending_root_requests from the moment a BlocksByRoot request is sent until something retires it, and the FetchBlock handler deduplicates every later fetch against that table. A root that is never retired is unreachable for the life of the process: the chain actor can ask for it any number of times, no request reaches the wire, and nothing is logged above debug to say so.

Two response paths ended an attempt without retiring the root. Neither needs a race; both are reachable with an ordinary peer on the other end.

1. An error response for a root request. The arm removed the id from outbound_requests, put it straight back, and returned:

Some(request @ PendingRequestKind::Root(_)) => {
    server.outbound_requests.insert(request_id, request);
}

handle_fetch_failure never reads outbound_requests, and an error response completes the exchange, so no OutboundFailure follows to pick the root up. Our own by-root responder never sends an error code, but other clients do.

2. A response whose blocks all fail the requested-root check. Every block hit continue, so pending_root_requests.remove was never reached and the function returned having retired nothing. outbound_requests had already been drained at the dispatch site, so no later event could recover it either.

Both leave the root pending with no retry scheduled, which is why the symptom is an indefinite silence rather than the error!("...giving up") you would expect from a fetch that ran out of attempts.

A leak alongside them. That re-insert appears twice on main: in the error arm above, and in the empty-response path, where a comment states the premise outright — "Re-insert so failure handling can find it". handle_fetch_failure takes root and peer as arguments and has never read outbound_requests, so it bought nothing either time. It also leaked: the dispatch site has already removed the id, the exchange is over, and no later libp2p event carries that id, so nothing removes it a second time. Every empty or error BlocksByRoot response left one dead entry in the map for the life of the process. The empty-response path did fail the root correctly, so the leak was its only symptom.

What Changed

All in crates/net/p2p/src/req_resp/handlers.rs.

• The error-response arm and the no-matching-block path both route into handle_fetch_failure, which is the single funnel that retires an attempt.
• The response path finds the one block that can answer a single-root request instead of looping. It subsumes the old empty-response special case, which is why the request_id parameter and the leaking re-insert it existed for are both gone.

Correctness / Behavior Guarantees

• The invariant: an attempt only ends by retiring the root, either by delivering the block or by charging a failure that schedules a retry or gives up. Previously two paths ended an attempt while leaving the entry behind.
• Retry policy is unchanged: MAX_FETCH_RETRIES attempts with doubling backoff, then the entry is dropped and an error! is logged. Dropping the entry means the P2P side forgets the root, not that it is blacklisted; the chain starts a fresh cycle the next time an orphan referencing that root arrives.
• A failure for a root that is no longer pending stays a no-op, so a late or duplicate event cannot resurrect a root that already succeeded.
• outbound_requests is leak-free in the ordinary case: two insert sites at send, three remove sites covering every terminal event, and a retry inserts a fresh id only after the failure that triggered it removed the old one. That bound rests on every request eventually producing a terminal event, the same assumption behind dropping the watchdog below; if it is ever wrong, this map leaks alongside the stranded root.
• Behavior change worth reviewing: a peer that answers with an error code, or with blocks we did not ask for, now costs an attempt and triggers a backoff retry against a different peer, where before it silently ended the fetch.

Tests Added / Run

No new tests. Both funnels sit behind an async handler taking &mut P2PServer and a live Context, which the crate has no harness for; earlier revisions of this branch reached the retry accounting by extracting it into a free function, and that indirection was not worth keeping for its own sake. Worth knowing that the existing suite did not catch either hole.

make fmt && make lint && make test

All green: 658 tests, 0 failures.

A third path that was considered and rejected

Earlier revisions added a watchdog for a request libp2p never reports an outcome for, because libp2p-request-response drops a request whose dial the swarm rejects with DialError::DialPeerConditionFalse (upstream PR 6000, in our 0.30.0 pin). That case is not reachable here, so it was removed in 3f67c80:

• Rejection needs a concurrent dial or an existing connection. A concurrent dial resolves either way and both outcomes drain the peer's parked requests, via preload_new_handler or on_dial_failure. That is the invariant PR 6000 relies on.
• An existing connection would strand the request, but it requires the behaviour's connection map to disagree with the pool. swarm_adapter.rs runs one task selecting between swarm events and commands, so a command never observes a half-updated swarm.

The reported symptom agrees. The request-size histogram last moved an hour before the stall, so the stranding request did reach the wire; a dial-path strand never reaches the codec and would not have moved it at all.

Not in scope

• BlocksByRange has the same shape, and a worse failure mode: a stranded range request leaves range_sync_state.in_flight = true forever, wedging sync rather than losing one block.
• The beacon branches carry the same bug in crates/net/p2p/src/sync.rs (record_root_fetch_failure, keyed by RootKey).

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test (cargo test --workspace --profile release-fast) — all passing

[github-actions]

🤖 Kimi Code Review

I'll review this PR which addresses a critical bug where BlocksByRoot requests that libp2p silently drops cause permanent deduplication of those blocks. The fix adds a watchdog timer and improves failure handling.

Overall Assessment

The PR correctly identifies and fixes a serious liveness bug. The structure is good with clear separation between testable pure logic and async effects. However, I found several issues ranging from correctness to maintainability concerns.

---

Critical Issues

1. Race Condition: Watchdog Can Fire After Successful Response

File: crates/net/p2p/src/req_resp/handlers.rs, lines 480-496

The watchdog is armed unconditionally, but if a response arrives and succeeds before the watchdog fires, the BlockFetchTimeout message still gets sent. The handler checks pending.attempts == msg.attempt (line 605 in lib.rs), but there's a subtle issue:

let attempt = server
    .pending_root_requests
    .entry(root)
    .or_insert(PendingRequest { attempts: 1, ... })
    .attempts;  // This reads attempt N

// ... later, on success ...
pending.attempts += 1;  // This increments to N+1 in record_fetch_failure

Wait—actually on success, pending_root_requests.remove(&requested_root) is called (line 339 in handlers.rs), so the entry is gone. But on retry, pending.attempts += 1 happens in record_fetch_failure. Then RetryBlockFetch fires, which calls fetch_block_from_peer again, which reads attempts (now N+1) and arms a new watchdog with attempt = N+1.

If the old watchdog (with attempt = N) fires after the retry is already in flight, it checks pending.attempts == msg.attempt → N+1 == N → false, so it's correctly ignored.

However, there's a worse case: what if the response arrives, calls handle_fetch_failure (which increments attempts and schedules retry), and then before fetch_block_from_peer runs for the retry, the old watchdog fires? At that moment pending.attempts == N+1 and msg.attempt == N, so it's ignored. Good.

But what if handle_fetch_failure removes the entry (gave up), then the old watchdog fires? pending.get(&msg.root) returns None, so is_some_and returns false. Good.

Actually, re-reading: the success path removes the entry entirely. The retry path increments attempts. The watchdog check pending.attempts == msg.attempt seems correct for both cases.

Wait—I found it. Consider: response arrives, handle_blocks_by_root_response is called, it calls matching_block, gets None, calls handle_fetch_failure. record_fetch_failure increments attempts from 1 to 2, returns FetchFailure::Retry. handle_fetch_failure then calls send_after with backoff. But the old watchdog with attempt=1 is still in flight. When it fires, pending.attempts == 2 != 1, so ignored. Correct.

But what about the new request? fetch_block_from_peer is called via RetryBlockFetch, it reads attempts == 2, inserts into outbound_requests with new request_id, arms new watchdog with attempt=2. This seems correct.

Actually, I think there's a different bug. Let me re-check...

In fetch_block_from_peer, line 475-480:

let attempt = server
    .pending_root_requests
    .entry(root)
    .or_insert(PendingRequest { attempts: 1, ... })
    .attempts;

This uses entry().or_insert() which means if the entry exists, it reads attempts. But or_insert with attempts: 1 would reset to 1 if the entry were absent—but we just incremented it to 2 in record_fetch_failure. The entry should exist because record_fetch_failure only removes on GaveUp. So this reads 2. Correct.

I think the race is actually OK due to the attempt counter check. But this relies on a subtle invariant. A comment explaining this would help, or better: cancel the watchdog explicitly.

Recommendation: Add a CancelBlockFetchTimeout mechanism or at minimum document the attempt-counter-based cancellation invariant prominently. The current code is correct but fragile.

---

2. outbound_requests Leak on Success Path

File: crates/net/p2p/src/req_resp/handlers.rs, lines 330-350

On successful block receipt:

server.pending_root_requests.remove(&requested_root);
// ... forward to blockchain ...

But request_id is not removed from server.outbound_requests! The entry in outbound_requests mapping request_id → Root(root) remains forever. This is a memory leak, and more critically, if libp2p later reuses a request_id (they're typically u64s that wrap), there could be confusion.

Wait—let me check if outbound_requests is cleaned elsewhere. Searching... In handle_req_resp_message, for Message::Response success case:

let Some(request_kind) = server.outbound_requests.remove(&request_id) else {
    continue;
};

Yes! Line 62-63 in the original, now line 60-61 in the new code. The remove happens in handle_req_resp_message before calling handle_blocks_by_root_response. So outbound_requests is cleaned on the response path.

But what about the watchdog path? In handle_block_fetch_timeout:

self.outbound_requests.remove(&msg.request_id);

Yes, line 623 in lib.rs. Good.

But what about the retry path? When RetryBlockFetch fires and calls fetch_block_from_peer again, a new request_id is generated and inserted. The old request_id... was already removed on the failure path that led to retry. Let me trace:

• Request fails → OutboundFailure event → handle_req_resp_message → outbound_requests.remove(&request_id) → gets Some(Root(root)) → calls handle_fetch_failure → schedules retry. Good, outbound_requests cleaned.

• Or watchdog fires → handle_block_fetch_timeout → outbound_requests.remove(&msg.request_id). Good.

• Or success → handle_req_resp_message → outbound_requests.remove(&request_id) → gets Some(Root(root)) → calls handle_blocks_by_root_response. Good.

So outbound_requests is properly cleaned in all paths. My mistake.

---

3. matching_block Drops Unsolicited Blocks Without Penalty

File: crates/net/p2p/src/req_resp/handlers.rs, lines 288-296

fn matching_block(blocks: Vec<SignedBlock>, requested_root: H256) -> Option<SignedBlock> {
    blocks
        .into_iter()
        .find(|block| block.message.hash_tree_root() == requested_root)
}

A malicious peer can send many blocks that don't match, and only the first matching one is returned. The peer isn't penalized for sending unsolicited data. In Ethereum p2p, this is typically a protocol violation that should lead to peer scoring/penalty.

Recommendation: At minimum, log at warn! level when unsolicited blocks are detected, and consider incrementing a peer misbehavior counter. The current debug! in handle_blocks_by_root_response is insufficient for detecting abuse.

---

4. Test the_fetch_watchdog_outlasts_the_libp2p_request_timeout Is Fragile

File: crates/net/p2p/src/req_resp/handlers.rs, lines 800-803

#[test]
fn the_fetch_watchdog_outlasts_the_libp2p_request_timeout() {
    assert!(ROOT_FETCH_WATCHDOG > crate::REQ_RESP_TIMEOUT);
}

This tests a constant ordering that should be enforced at compile time. A unit test for this is wasteful and can break if someone changes constants without running tests.

Recommendation: Use a const_assert! or static assertion instead:

const _: () = assert!(ROOT_FETCH_WATCHDOG.as_secs() > REQ_RESP_TIMEOUT.as_secs());

Or with the static_assertions crate. This fails at compile time, not test time.

---

Moderate Issues

5. record_fetch_failure Mutates Before Returning

File: crates/net/p2p/src/req_resp/handlers.rs, lines 604-628

fn record_fetch_failure(...) -> FetchFailure {
    let Some(pending) = pending_root_requests.get_mut(&root) else {
        return FetchFailure::NotPending;
    };
    pending.failed_peers.insert(peer);  // Mutation
    let attempts = pending.attempts;
    if attempts >= MAX_FETCH_RETRIES {
        pending_root_requests.remove(&root);  // Mutation
        return FetchFailure::GaveUp { attempts };
    }
    pending.attempts += 1;  // Mutation
    // ...
}

The function both mutates and returns a value describing what it did. This is testable but the side effects are hidden in the name "record". The attempts returned in Retry is the pre-increment value, which is used for logging and backoff calculation.

This is actually a bug in the backoff calculation. Let me check:

let backoff_ms = INITIAL_BACKOFF_MS * BACKOFF_MULTIPLIER.pow(attempts - 1);

For attempts = 1 (first failure): BACKOFF_MULTIPLIER.pow(0) = 1, so INITIAL_BACKOFF_MS. Correct.
For attempts = 2 (second failure): BACKOFF_MULTIPLIER.pow(1) = 2, so 2 * INITIAL_BACKOFF_MS. Correct.

But wait—the attempts field in PendingRequest starts at 1. After first record_fetch_failure, it increments to 2. The next call reads attempts = 2, returns it in Retry, calculates 2^1 = 2. Correct.

Actually, looking more carefully: the attempts returned is the current attempt count before incrementing, which represents "how many attempts have been made so far". The backoff is for the next attempt. So attempt 1 failed, we're scheduling attempt 2, backoff is 2^(1-1) = 1 * INITIAL. Then attempt 2 fails, scheduling attempt 3, backoff is 2^(2-1) = 2 * INITIAL. This seems correct.

But the naming is confusing. attempts in FetchFailure::Retry means "the attempt that just failed", not "the next attempt". The log says attempts=%attempts, "Block fetch failed, scheduling retry" which is correct for the failure that just happened.

However, there's an off-by-one in understanding: MAX_FETCH_RETRIES is checked against attempts before increment. So if MAX_FETCH_RETRIES = 3:

• Start: attempts = 1
• Fail 1: attempts = 1 < 3, increment to 2, retry
• Fail 2: attempts = 2 < 3, increment to 3, retry
• Fail 3: attempts = 3 >= 3, give up

So we retry on attempts 1 and 2, and give up on attempt 3. That's 2 retries, 3 total attempts. Is MAX_FETCH_RETRIES meant to be total attempts or number of retries? The name suggests retries, but the code implements total attempts. This is a naming/documentation issue, not necessarily a bug if documented.

Recommendation: Clarify in comments whether MAX_FETCH_RETRIES is total attempts or number of retries after the first attempt.

---

6. handle_fetch_failure Is pub(crate) But Documented as Internal

File: crates/net/p2p/src/req_resp/handlers.rs, line 641

pub(crate) async fn handle_fetch_failure(...)

The doc comment says "Every path that ends an attempt must come through here" which is good. But it's pub(crate) and re-exported in mod.rs. This is fine for the architecture.

---

7. Missing Send Bound on FetchFailure

File: crates/net/p2p/src/req_resp/handlers.rs, lines 581-590

#[derive(Debug, PartialEq, Eq)]
enum FetchFailure { ... }

Not an issue since it's only used synchronously, but if this ever crosses async boundaries, Send would be needed. Currently fine.

---

Minor Issues

8. Inconsistent Logging Levels

File: crates/net/p2p/src/req_resp/handlers.rs, line 317

debug!(
    %peer,
    received,
    expected_root = %ethlambda_types::ShortRoot(&requested_root.0),
    "BlocksByRoot response carried no matching block"
);

A peer sending no matching block is at least suspicious, possibly malicious. debug! is too low. Use warn! for protocol violations.

Similarly, line 614 in lib.rs:

warn!(..., "BlocksByRoot request produced no libp2p outcome, failing it");

This is correctly warn!.

9. fetch_block_from_peer Returns bool for Success

File: crates/net/p2p/src/req_resp/handlers.rs, line 403

The bool return is used to decide whether to remove from pending_root_requests on final failure. This is a C-style pattern. Consider an enum:

enum FetchResult {
    RequestSent,
    NoPeers,
    AlreadyPending, // if this were possible
}

But the current code is functional, just not idiomatic Rust.

---

Positive Aspects

1. Good test coverage for record_fetch_failure and matching_block logic
2. Clear separation of pure logic (record_fetch_failure) from effects (handle_fetch_failure)
3. Attempt counter for watchdog deduplication is correct and race-safe
4. Comments explain the libp2p bug being worked around
5. Constants are well-named and documented

---

Summary

Priority	Issue	Location
Medium	Replace watchdog ordering test with compile-time assert	handlers.rs:800-803
Medium	Clarify MAX_FETCH_RETRIES semantics (attempts vs retries)	handlers.rs:604-628
Low	Elevate log level for unsolicited blocks	handlers.rs:317
Low	Document race safety of attempt-counter watchdog cancellation	lib.rs:595-625 or handlers.rs:480-496

The core fix is correct and well-implemented. The watchdog approach properly handles the libp2p bug without introducing new races. The PR should be approved with minor cleanups.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

[github-actions]

🤖 Codex Code Review

No material findings in the touched code.

The new BlocksByRoot cleanup path looks correct: the watchdog in crates/net/p2p/src/lib.rs closes the silent-drop hole, and the response/error handling in crates/net/p2p/src/req_resp/handlers.rs and handlers.rs now consistently retires or retries root fetches instead of leaving them permanently deduplicated. The extracted record_fetch_failure logic and added unit tests also improve maintainability.

Residual risk: I could not execute cargo test in this sandbox because cargo/rustup attempted to write under /home/runner/.rustup and /home/runner/.cargo, which are read-only here.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have everything needed to write the review.

Code Review: PR 608 — fix(p2p): never leave a block root pending without an outcome

Overall this is a well-reasoned fix for a real bug (permanently-stuck pending_root_requests entries), with good test coverage for the three failure paths it explicitly targets. The record_fetch_failure/handle_fetch_failure split is a nice testability improvement, and matching_block replacing the loop is a clean simplification.

However, the new watchdog mechanism itself reintroduces a subtler version of the exact problem it's meant to solve.

1. Watchdog correlates on attempts, which is not unique across a root's full lifetime — false-positive timeouts (crates/net/p2p/src/lib.rs:612-619, crates/net/p2p/src/req_resp/handlers.rs:475-489)

fetch_block_from_peer arms the watchdog with the current pending.attempts value (starting at 1 for a fresh entry), and handle_block_fetch_timeout treats the attempt as "still outstanding" purely by checking pending.attempts == msg.attempt. That counter is not a lifetime-unique identifier for a root — it resets to 1 every time the entry is removed and recreated (.entry(root).or_insert(PendingRequest { attempts: 1, .. }) at handlers.rs:477-480), which happens both on success (pending_root_requests.remove in handle_blocks_by_root_response) and on give-up (record_fetch_failure's GaveUp branch).

Concretely:

• A full give-up cycle (10 attempts, backoff doubling from a few ms up to ~2.5s per INITIAL_BACKOFF_MS/BACKOFF_MULTIPLIER) completes in low single-digit seconds — far faster than the 15s ROOT_FETCH_WATCHDOG.
• When it gives up, the entry is removed, but the up-to-10 watchdog timers armed during that cycle (for attempt values 1..=10) are all still scheduled to fire over the next ~10-15s — they were never cancelled (this framework has no timer cancellation, which is fine/expected), but nothing invalidates their attempt tag either.
• If the same root is requested again within that window (very plausible: the same missing parent gets re-announced by gossip, or a re-org needs it again), the new cycle starts a fresh PendingRequest { attempts: 1, .. }. As soon as it reaches whichever attempt number a still-pending stale watchdog was tagged with (attempt 1 is essentially guaranteed to collide immediately), that stale watchdog fires, sees pending.attempts == msg.attempt, concludes the new, healthy, in-flight request "produced no libp2p outcome", and calls handle_fetch_failure with the stale peer from the old cycle.

Effects: the new cycle's attempt counter gets bumped by a phantom failure it didn't actually suffer, an unrelated peer gets marked as failed_peers for a request it was never party to, a needless duplicate RetryBlockFetch gets scheduled, and — worst case — since every leftover watchdog from the old cycle (1..=10) can collide with the corresponding attempt of the new cycle as it progresses, the new cycle can be pushed to hit MAX_FETCH_RETRIES and give up much sooner than its actual failure count would justify. This directly contradicts the PR's stated invariant ("A stale watchdog is a no-op... the timer carries the attempt it was armed for") — that only holds within one continuous attempt chain, not across a remove-and-recreate cycle for the same root.

Suggested fix: correlate on request_id instead of attempts. Every completion path (Message::Response success/error, OutboundFailure) already does server.outbound_requests.remove(&request_id) exactly once. So the watchdog can just do the same removal and match on it as the sole "is this attempt still outstanding" signal:

let Some(PendingRequestKind::Root(root)) = self.outbound_requests.remove(&msg.request_id) else {
    trace!(root = %msg.root, "Block fetch settled before the watchdog fired");
    return;
};

request_id is generated by libp2p as a monotonic counter, so unlike attempts it can't collide across separate fetch cycles for the same root. This also makes the attempt field on BlockFetchTimeout and the attempts == msg.attempt check unnecessary.

Test coverage: none of the five new tests exercise a give-up-then-refetch (or success-then-refetch) sequence within the watchdog window; the_fetch_watchdog_outlasts_the_libp2p_request_timeout only checks the constant ordering. A test that recreates a PendingRequest for a root after a prior cycle and confirms a leftover watchdog for the old cycle is a no-op would have caught this.

Minor

• handle_block_fetch_timeout's warn! message ("BlocksByRoot request produced no libp2p outcome, failing it", lib.rs:621-626) would be misleading in the false-positive scenario above — the request may well have had a perfectly normal outcome, just for a different (already-completed) cycle. Fixing the correlation per Point 1 also fixes this log's accuracy.
• Everything else — the error-response funnel into handle_fetch_failure, matching_block replacing the mismatched-root loop, record_fetch_failure returning a typed FetchFailure enum, and pinning REQ_RESP_TIMEOUT explicitly with the ordering assertion against ROOT_FETCH_WATCHDOG — is solid, idiomatic, and matches the codebase's inspect_err/metrics/logging conventions.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Kimi Code Review

I'll review this PR focusing on the request-response handling changes for block-by-root fetching. The changes appear to be a bugfix for a request tracking/deduplication issue.

Overall Assessment

This is a correctness fix for a serious bug: previously, error responses to BlocksByRoot requests would leave roots in pending_root_requests forever, causing permanent deduplication of those blocks. The fix ensures all completion paths properly retire pending requests.

---

Detailed Review

crates/net/p2p/src/req_resp/handlers.rs

Lines 100-105: Error response handling — Good fix, but logic gap remains

Some(PendingRequestKind::Root(root)) => {
    // An error response completes the exchange, so
    // no `OutboundFailure` follows to retire the
    // root. Fail it here or it stays pending
    // forever and deduplicates every later fetch.
    handle_fetch_failure(server, root, peer, ctx).await;
}

The comment and fix are correct. However, there's a remaining concern: what about OutboundFailure events that do arrive for requests already handled here? The old code re-inserted the request; the new code doesn't.

Check if OutboundFailure handling (around line 97's None branch or elsewhere) will call handle_fetch_failure on an already-removed root, and whether that's harmless. Looking at handle_fetch_failure (line 570): it does get_mut(&root) and returns early if missing — safe.

Lines 289-318: handle_blocks_by_root_response — Logic change introduces subtle issue

let answer = blocks
    .into_iter()
    .find(|block| block.message.hash_tree_root() == requested_root);

Issue: Unsolicited blocks are silently dropped without peer scoring

The old code logged debug for mismatched roots and continued. The new code uses .find() and silently ignores any extra blocks. Per the comment ("anything else the peer sent is unsolicited"), this is intentional, but:

1. No peer penalty for unsolicited blocks — A malicious peer could stuff responses with junk blocks to waste bandwidth, with no consequence. Consider incrementing a peer score or disconnecting if received > 1 or if any block has wrong root.

2. Missing validation that answer is at index 0 — The spec may require the first block to match; this accepts a matching block at any position.

Line 302: server.pending_root_requests.remove(&requested_root);

This is now called before blockchain.new_block(). If new_block fails or panics, the root is already removed. This is actually correct (idempotent failure handling), but verify handle_fetch_failure isn't also called on new_block errors — it isn't in this path. Acceptable.

Lines 556-572: handle_fetch_failure — Race condition concern

let Some(pending) = server.pending_root_requests.get_mut(&root) else {
    return;
};

This is called from multiple async paths. The get_mut borrows server mutably for the duration. Check if any .await point inside could yield and allow interleaving — the function appears to be synchronous after this point until the .await on line 586 (ctx.clock.sleep(...)), but the mutable borrow of pending is dropped before then. OK.

However, lines 580-586 (not fully shown in diff but implied):

pending.failed_peers.insert(peer);
// ... retry logic with .await ...

If pending has no more peers to try and the sleep/timeout .await yields, another task could modify pending_root_requests. The get_mut borrow doesn't extend across .await, so this is safe, but verify the retry logic re-checks existence before re-inserting or removing.

Line 80-85: handle_blocks_by_root_response signature change

Removing request_id parameter is correct since the function no longer needs to re-insert into outbound_requests. Verify no callers remain with old signature — diff shows this is the only call site. Good.

---

Security Considerations

Issue	Severity	Notes
Permanent block unavailability (fixed)	Critical	Original bug could permanently prevent fetching certain blocks
No penalty for unsolicited blocks	Low-Medium	Bandwidth waste, no DoS vector beyond existing
Single peer can fail all retries	Medium	failed_peers tracking is per-root, but retry logic not shown in diff

---

Suggested Improvements

1. Add peer scoring for malformed responses (line 291-302)

// After .find(), if received > 1 or any non-matching block exists:
if received > 1 || blocks.iter().any(|b| b.message.hash_tree_root() != requested_root) {
    // Increment peer score for protocol violation
    server.peer_scores.entry(peer).and_modify(|s| *s += 1).or_insert(1);
}

2. Verify spec compliance on response ordering

Check if the consensus spec requires the requested block to be first in the response. If so, use .next() instead of .find():

let mut blocks = blocks.into_iter();
let answer = blocks.next().filter(|b| b.message.hash_tree_root() == requested_root);
// Then verify blocks.next() is None or handle as violation

3. Defensive assertion in handle_fetch_failure

The early return on missing root is correct but consider a debug_assert! or metric to catch unexpected double-removals in testing:

let Some(pending) = server.pending_root_requests.get_mut(&root) else {
    trace!(%root, "handle_fetch_failure called for unknown/already-removed root");
    return;
};

---

Positive Acknowledgments

1. Excellent comment at lines 100-104 — clearly explains why the fix is necessary
2. Function-level docstring on handle_fetch_failure (lines 552-560) — documents the critical invariant that all paths must retire roots
3. Early return pattern with let-else — idiomatic Rust, clean control flow
4. Removal of request_id parameter — simplifies API after removing re-insertion logic

---

Summary

Category	Verdict
Bug fix correctness	✅ Correct — fixes root leak
Remaining DoS/attack surface	⚠️ Minor — no peer penalty for bad responses
Memory safety	✅ No issues
Async safety	✅ No obvious races
Spec compliance	❓ Verify response ordering requirement

Recommend merging with follow-up for peer scoring.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

[github-actions]

🤖 Claude Code Review

Review: PR 608 — fix(p2p): never leave a block root pending without an outcome

Summary of the fix: Two response paths in handle_req_resp_message/handle_blocks_by_root_response could end a BlocksByRoot fetch attempt without going through handle_fetch_failure, leaving the root stuck in pending_root_requests forever (since FetchBlock dedups against that map). This PR routes both paths — an Error response for a root request, and a Success response whose blocks don't match the requested root — through handle_fetch_failure.

Correctness

• The fix is correct and addresses a real bug. Previously the error-response arm did server.outbound_requests.insert(request_id, request) — re-inserting into a map that handle_fetch_failure (invoked only from OutboundFailure) never reads, and since an Error response terminates the exchange (no follow-up OutboundFailure), the root was orphaned permanently. Now it's routed into handle_fetch_failure directly, with the request_id already consumed via remove at the call site (handlers.rs:96) before the fail path runs, so no stale map entry survives either way.
• The handle_blocks_by_root_response rewrite (handlers.rs:288-322) replaces the continue-based loop with .find(), matching the stated invariant that a single-root request has at most one valid answer. This correctly short-circuits: the old loop's continue on mismatch meant a response with only mismatched blocks fell out of the loop having removed nothing from pending_root_requests — silently losing the root, same class of bug as the error-response case.
• handle_fetch_failure's early return (get_mut(&root) is None → no-op) correctly guards against late/duplicate failure events resurrecting an already-satisfied or already-abandoned root — this is documented inline and matches the PR's stated invariant.
• Good instinct to drop the earlier watchdog approach after showing DialPeerConditionFalse isn't reachable in this codebase's dial path (single-task swarm adapter serializing commands and events). I confirmed no dead code (const asserts, record_fetch_failure, FetchFailure enum, watchdog message types) was left behind from the abandoned revisions — the final diff is clean and minimal.

Minor observations (non-blocking)

• handle_blocks_by_root_response no longer bounds how many blocks it will scan via .find() before giving up — a peer could pad a response to a single-root request with many unsolicited blocks. This isn't new behavior (the old loop iterated over the same blocks vec), and MAX_REQUEST_BLOCKS isn't enforced on the response side for either version, so it's out of scope for this fix, but worth a follow-up if a malicious peer's oversized responses become a concern.
• The PR description explicitly calls out BlocksByRange's analogous stranding risk (range_sync_state.in_flight never reset) and the beacon-chain equivalent in sync.rs/RootKey as out of scope. Reasonable to defer, but worth an issue/tracking ticket so it doesn't get lost, since it's the same bug class.

Process notes

• The commit history shows healthy iteration (watchdog added, then proven unnecessary and reverted; helper indirection added for testability, then folded back in when the abstraction wasn't earning its keep) — the final state is simpler than earlier revisions, which is the right outcome even though tests were dropped along with the extracted record_fetch_failure function. Given the stated constraint (accounting logic is behind &mut P2PServer + live Context, no harness exists), that tradeoff is defensible, though it does mean this specific fix has no regression test — something to flag if the crate gains a P2P test harness later.

Overall: a well-scoped, correctly targeted fix with a clean diff. No blocking issues found.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

No blocking findings.

The change in handlers.rs and handlers.rs looks correct: BlocksByRoot responses now always retire the request_id, and empty or mismatched responses flow through handle_fetch_failure instead of leaving the root stuck in pending_root_requests and permanently deduplicated.

Residual risk: this behavior is not obviously covered by tests, and it is easy to regress because block-protocol error chunks are still decoded as Success(Blocks(..)) in codec.rs, so the root-request error path in handlers.rs is not the only path that matters. A focused regression test around empty, mismatched, and error BlocksByRoot responses would close that gap.

I couldn’t run cargo test here because the environment blocks cargo/rustup writes outside the workspace and dependency fetches.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt