fix(p2p): consult connection limits before any other behaviour

[MegaRedHand]

🗒️ Description / Motivation

A Platåberget follower built with release-fast (which keeps debug-assertions on) lost its P2P task to a panic inside libp2p request-response. The process, the API and the chain actor kept running, so the head froze for about five hours with nothing but this in the log:

panicked at .../rust-libp2p-da8daccbaa8a6b4a/2f14d0e/protocols/request-response/src/lib.rs:708:9:
assertion `left == right` failed
  left: false
 right: true

That line is debug_assert_eq!(connections.is_empty(), remaining_established == 0) in on_connection_closed: request-response still counted a connection to a peer the swarm said had none left.

The cause is the field order of Behaviour, where connection_limits came last:

Step	What happens
1	The swarm calls handle_established_*_connection on Behaviour; the derive calls each field in declaration order with ?
2	Every request_response::Behaviour in ReqResp records the connection in its connected map (preload_new_handler)
3	connection_limits refuses it (per-peer, total, inbound or outbound ceiling)
4	The swarm reports a ListenFailure/DialFailure, and never a ConnectionEstablished or ConnectionClosed for it. Request-response ignores both, so the entry stays
5	The peer's last real connection closes with remaining_established == 0, while request-response still holds the phantom: the assert fires

Without debug assertions the phantom is worse than a leak. try_send_request picks a connection by request_id % connections.len(), so some requests to that peer go to NotifyHandler::One(phantom). The swarm drops an event for an unknown connection silently (swarm/src/lib.rs:1214 in the pinned fork), and the request timeout lives in the handler that was never spawned, so no OutboundFailure ever comes back. ethlambda retires in-flight requests on OutboundFailure.

What Changed

• crates/net/p2p/src/lib.rs: connection_limits is now the first field of Behaviour (and of its struct literal in build_swarm), with a doc comment saying why it has to stay first. A refusal now happens before any other behaviour sees the connection.
• crates/net/p2p/src/lib.rs (tests): a regression test, below.

Correctness / Behavior Guarantees

• Which connections are refused is unchanged: same limits, same counts. What changes is that no other behaviour sees a refused connection.
• connection_limits is safe to put first. It records established connections only on FromSwarm::ConnectionEstablished, emits no events (poll is always Pending), and its handler is dummy, so the protocols offered and the event dispatch are unchanged.
• No field after it refuses connections, so no behaviour can be left holding a phantom.
• Lean runs unlimited_connections(), which never refuses, so the lean network is unaffected.

Tests Added / Run

• tests::a_connection_the_limits_refuse_leaves_no_request_response_state builds the real beacon swarm with build_swarm and drives its Behaviour the way the swarm does: two connections from one peer (MAX_CONNECTIONS_PER_PEER), a third refused and reported as a ListenFailure, then both held connections close. Before the fix it fails in both build modes:

  • with debug assertions (release-fast, as CI runs it): the production panic at request-response/src/lib.rs:708:9
  • without (CARGO_PROFILE_RELEASE_FAST_DEBUG_ASSERTIONS=false): its own assertion, since the field still reports the peer as connected

  After the fix it passes in both.

• cargo test -p ethlambda-p2p --lib --profile release-fast: 208 passed, 1 ignored.

Related Issues / PRs

• Failing debug_assert in Request-Response protocol libp2p/rust-libp2p#4773: the same assert, open. The maintainers' advice there is to put connection-management behaviours first, which is what this does.
• swarm: connection management behaviours can lead to inconsistent state libp2p/rust-libp2p#4870: the underlying design issue (the handle_* callbacks take &mut self), open.
• fix(request-response): debug_assert panic on last connection close libp2p/rust-libp2p#6601: fixes request-response itself by recording connections on ConnectionEstablished. Open and unreviewed; it could be cherry-picked into lambdaclass/rust-libp2p later, and this PR does not depend on it.

✅ Verification Checklist

• Ran make fmt — clean for this change. cargo fmt --all -- --check also flags the mod order in bin/ethlambda/src/main.rs, which is already on beacon-chain-integration (from c635524) and left out of this PR
• Ran make lint (clippy with -D warnings) — clean
• Ran make test (test-consensus plus test-node, at release-fast) — left to CI; ran the p2p unit tests above

[github-actions]

🤖 Kimi Code Review

I'll review this PR focusing on the libp2p connection ordering fix and its test. This is a well-targeted fix for a subtle libp2p interaction bug.

Overall Assessment

The fix is correct and well-explained. The PR addresses a real bug where request_response::Behaviour could hold phantom connections when connection limits refused a connection, due to libp2p's handler polling order. However, I have several concerns about the test's robustness and some code quality issues.

Issues Found

1. Test uses ConnectionId::new_unchecked with potentially conflicting IDs (Line 3577, 3594)

let held: Vec<ConnectionId> = (0..max_per_peer).map(ConnectionId::new_unchecked).collect();
// ...
let refused = ConnectionId::new_unchecked(max_per_peer);

Problem: ConnectionId::new_unchecked is unsafe in the sense that it bypasses libp2p's internal ID generation. While this works for isolated tests, if libp2p's ConnectionId implementation changes its numbering scheme or adds validation, this test could break or exhibit undefined behavior. More critically, if any other test in the same process also creates ConnectionIds this way, collisions are possible.

Suggestion: Add a comment explaining why new_unchecked is necessary here (to simulate the swarm's ID sequence without a real swarm), or wrap this in a module-level comment. Consider using a test-specific counter if feasible.

2. Test doesn't verify all ReqResp members (Line 3618)

let blocks_by_range = &behaviour.req_resp.beacon_blocks_by_range;
assert!(
    !blocks_by_range.is_connected(&peer),
    "a request/response field still holds the connection the limits refused"
);

Problem: ReqResp likely contains multiple request-response protocols (e.g., beacon_blocks_by_root, blobs_by_range, etc.). The test only checks beacon_blocks_by_range. If other fields in ReqResp also track connections independently, the bug could persist there unchecked.

Suggestion: Iterate over all request-response fields in ReqResp or add a helper method to ReqResp that checks is_connected across all protocols. If ReqResp derives or implements a common interface, use it:

// Suggested addition to ReqResp or test helper
fn assert_no_phantom_connections(req_resp: &ReqResp, peer: &PeerId) {
    assert!(!req_resp.beacon_blocks_by_range.is_connected(peer));
    assert!(!req_resp.beacon_blocks_by_root.is_connected(peer));
    // ... all other fields
}

3. Missing assertion that the fix actually catches the bug (regression test completeness)

The test verifies the absence of phantom connections after the fix, but doesn't demonstrate that the bug would occur with the old field ordering. This is a minor concern for regression test value.

Suggestion: Consider adding a brief comment or a separate test showing that with connection_limits last, blocks_by_range.is_connected(&peer) would return true after the sequence. However, this may be overkill given the clear root cause analysis in comments.

4. Documentation comment has minor typo (Line 540-541)

    /// Refuses connections past the configured ceiling. A deny from any member
    /// behaviour denies the connection, so registering this is the whole

Not a bug, but: "behaviour" vs "behavior" inconsistency (British vs American spelling). The codebase seems to use both; pick one for consistency. This appears throughout the file.

5. Test name is excessively long (Line 3536)

async fn a_connection_the_limits_refuse_leaves_no_request_response_state() {

While descriptive, this exceeds typical Rust naming conventions. Rust tests are usually snake_case but more concise.

Suggestion: connection_refusal_clears_req_resp_state or phantom_connection_not_leaked_on_refusal. Not critical.

6. Potential race condition in comment explanation (Line 543-548)

    /// First, and it has to stay first. The derive asks each field for a
    /// handler in declaration order and stops at the first refusal, and a
    /// refused connection never produces a `ConnectionClosed`.

Clarification needed: The comment correctly explains the mechanism, but should explicitly note that this is a libp2p NetworkBehaviour derive macro implementation detail, not a guaranteed stable API contract. If libp2p changes the derive macro behavior, this fix could silently break.

Suggestion: Add a warning comment:

    /// NOTE: This relies on the `NetworkBehaviour` derive macro polling fields
    /// in declaration order. If libp2p changes this, the ordering constraint
    /// may need re-evaluation. See rust-libp2p#4773, #4870.

7. build_swarm test setup is heavy (Line 3548-3565)

The test constructs a full BeaconWireConfig with Config::mainnet() just to test connection limit ordering. This creates unnecessary dependencies and slows test execution.

Suggestion: If possible, extract a lighter Behaviour construction path for unit tests, or verify that Config::mainnet() doesn't trigger heavy initialization (it likely doesn't, but this couples the test to mainnet config validity).

8. No test for outbound connection refusal

The test only covers handle_established_inbound_connection. The same bug could theoretically affect outbound connections if limits are applied there.

Suggestion: Add a symmetric test or verify that handle_established_outbound_connection follows the same code path through the derive macro. The field ordering fix should cover both, but explicit testing provides confidence.

Positive Observations

1. Excellent root cause documentation: The comments in lines 543-551 and 3521-3535 clearly explain the libp2p bug and why the ordering matters.

2. Proper use of FromSwarm events: The test correctly simulates the swarm's event sequence, including the ListenFailure that a refused connection receives instead of ConnectionClosed.

3. Debug assertion context: The comment about debug_assert killing the P2P task (line 3529) correctly identifies the production symptom that would manifest.

4. Correct fix placement: Moving connection_limits to the first position is the minimal, correct fix.

Summary

Priority	Item	Action
Medium	Test only checks one ReqResp field	Check all request-response protocols
Low	ConnectionId::new_unchecked usage	Add safety comment
Low	Missing outbound test	Consider adding
Low	libp2p derive macro dependency	Add stability warning comment
Trivial	Spelling inconsistency	Standardize "behavior/behaviour"

The core fix is correct and well-justified. The test effectively validates the specific bug scenario, but could be more comprehensive in coverage of ReqResp fields.

---

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

[github-actions]

🤖 Claude Code Review

Review of PR 640: fix(p2p): consult connection limits before any other behaviour

I read the Behaviour struct and build_swarm in crates/net/p2p/src/lib.rs. I did not run the tests or build anything. The repo snapshot is one squashed commit of about 187k lines, so I could only judge the connection-limits change itself.

The fix looks correct.

• connection_limits is now the first field of #[derive(NetworkBehaviour)] struct Behaviour (lib.rs:549-550).
• The derive asks each field for a handler in declaration order and stops at the first refusal. Putting the limiter first means a refused connection never reaches the request_response::Behaviours in ReqResp.
• That removes the phantom connections they used to record. A refused connection never produces a ConnectionClosed, so those phantoms were never cleaned up. This matches rust-libp2p#4773 and #4870.
• connection_limits::Behaviour records a connection only on ConnectionEstablished, not when it is asked for a handler. A later behaviour refusing a connection therefore can't leave stale state in the limiter. Ordering only needs fixing in this one direction.
• The struct literal in build_swarm (lib.rs:834-839) uses named fields, so the reorder there is cosmetic. The doc comment on the field explains why it has to stay first.

Suggestions (non-blocking)

1. No test pins the ordering.
   • A reorder of the struct fields would bring the panic back (a debug_assert in debug builds, silently dropped requests in release).
   • A small test would catch it: build a swarm with max_established_incoming(0), dial it, then assert that req_resp holds no connection state. If that is too heavy, a short comment pointing at the existing Exceeded downcast site (lib.rs:1763-1768) would be a reasonable fallback.
2. Lean's unlimited_connections() also relies on the ordering. With default limits it never denies, so it is harmless today. The doc comment says the ordering matters for beacon only. Say it applies to both, so nobody reads the lean path as exempt if limits are added there later.

No security or consensus concerns in this change.

---

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

[github-actions]

🤖 Codex Code Review

Looks good overall. The change is narrow, addresses a real libp2p ordering hazard, and the added regression test exercises the exact failure mode.

• crates/net/p2p/src/lib.rs:539 — Moving connection_limits to the first field in Behaviour is correct. Given NetworkBehaviour’s declaration-order delegation, this prevents ReqResp from observing a connection that the composed behaviour later rejects. That avoids stale per-peer connection state and is the right root-cause fix.
• crates/net/p2p/src/lib.rs:833 — Reordering the struct initialization to match the field order is clean and avoids confusion; no correctness concern here.
• crates/net/p2p/src/lib.rs:3525 — The new test is valuable and well targeted. It validates the subtle invariant that a refused inbound connection must not leave request_response believing the peer is still connected after all admitted sockets close.

No consensus-layer logic is touched here, so fork choice / attestation / state transition / SSZ / XMSS risks do not appear impacted by this PR.

One small maintainability note:

• crates/net/p2p/src/lib.rs:3576 and crates/net/p2p/src/lib.rs:3588 — ConnectionId::new_unchecked(...) is acceptable in a test, but it would be slightly clearer to add a short test-local helper or comment explaining why the unchecked constructor is safe in this synthetic sequence.

Validation

• I attempted to run the new test, but the environment blocks rustup from creating temp files under the default home, so I could not execute it here.

If you want, I can also do a second pass on the surrounding IncomingConnectionError / peer-forgetting logic for related edge cases.

---

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