fix(p2p): never leave a block root pending without an outcome

[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.

Three paths ended an attempt without retiring the root.

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.

3. libp2p never reporting an outcome. libp2p-request-response deliberately drops a request whose dial the swarm rejects with DialError::DialPeerConditionFalse:

// protocols/request-response/src/lib.rs:742
if let DialError::DialPeerConditionFalse(_) = error {
    // Dial-condition fails because there is already another ongoing dial.
    return;
}

That is upstream PR 6000, shipped in 0.29.0; our pin is 0.30.0. The request stays in the behaviour's pending_outbound_requests and is drained only by preload_new_handler on the next connection to that peer, so no OutboundFailure is emitted and nothing downstream can react.

What Changed

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.
• matching_block() picks the one block that can answer a single-root request, replacing the loop. It subsumes the old empty-response special case, which is why the request_id parameter and the re-insert it existed for are gone.
• record_fetch_failure() holds the retry accounting (attempt count, failed-peer set, backoff, give-up) and returns a FetchFailure outcome. handle_fetch_failure is now the logging and scheduling shell around it.
• fetch_block_from_peer takes a Context and arms a watchdog for the attempt it just sent.

crates/net/p2p/src/lib.rs

• New BlockFetchTimeout { root, peer, request_id, attempt } protocol message and its handler. It fires into handle_fetch_failure only when the pending entry is still on the same attempt, so a settled or superseded attempt leaves it a no-op.
• REQ_RESP_TIMEOUT is now passed explicitly to request_response::Config instead of being inherited from Default::default(). Same value libp2p defaulted to, so no behavior change, but the watchdog's ordering constraint is now stated in code rather than assumed.
• ROOT_FETCH_WATCHDOG sits above it, so a request that did reach a connection still fails through libp2p's own path first.

Correctness / Behavior Guarantees

• New invariant: every entry in pending_root_requests has an outstanding timer or an in-flight request that will retire it. Previously the table's only exits were a matching response, an OutboundFailure chain, and a failed retry send.
• The watchdog cannot preempt libp2p's own failure reporting: ROOT_FETCH_WATCHDOG > REQ_RESP_TIMEOUT is asserted in a test.
• A stale watchdog is a no-op. Any outcome either clears the entry or advances attempts, and the timer carries the attempt it was armed for.
• record_fetch_failure returns NotPending for an untracked root, so a late or duplicate failure cannot resurrect a root that already succeeded.
• Give-up still clears the entry. Leaving it behind would be the same permanent lock as never failing the attempt.
• Behavior change worth knowing: 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. MAX_FETCH_RETRIES is unchanged.

Tests Added / Run

Five tests in req_resp::handlers::tests:

• matching_block_rejects_a_response_that_answers_a_different_root — the regression for hole 2.
• record_fetch_failure_ignores_an_untracked_root
• record_fetch_failure_backs_off_and_excludes_the_failing_peer — attempt counting, doubling backoff, failed-peer exclusion.
• record_fetch_failure_clears_the_root_when_it_gives_up
• the_fetch_watchdog_outlasts_the_libp2p_request_timeout

make fmt && make lint && make test

All green: 663 tests, 0 failures.

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).

Related Issues / PRs

• Related to feat(p2p): add a TCP (noise + yamux) transport alongside QUIC #600 (the TCP fallback widens the window in which concurrent dials to one peer can trip the DisconnectedAndNotDialing condition)

✅ 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