Conversation
procdump
marked this pull request as draft
September 3, 2026 12:00
procdump
marked this pull request as ready for review
September 3, 2026 12:03
procdump
force-pushed
the
fix/panic-request-response-connection-tracking-assert_main
branch
5 times, most recently
from
September 15, 2026 06:27
e6562bb to
0eb5e30
Compare
procdump
force-pushed
the
fix/panic-request-response-connection-tracking-assert_main
branch
2 times, most recently
from
September 17, 2026 14:23
245d69f to
5f0ad36
Compare
procdump
force-pushed
the
fix/panic-request-response-connection-tracking-assert_main
branch
from
September 23, 2026 10:39
5f0ad36 to
a1b453d
Compare
Recording at handle_established_* left a phantom entry when a sibling behaviour denied the connection, tripping the on_connection_closed debug_assert and mis-routing requests to a dead connection. Record on FromSwarm::ConnectionEstablished instead, so `connected` mirrors the swarm's committed set.
procdump
force-pushed
the
fix/panic-request-response-connection-tracking-assert_main
branch
from
September 29, 2026 13:35
a1b453d to
da8c9e4
Compare
2 of 3 tasks
MegaRedHand
added a commit
to lambdaclass/ethlambda
that referenced
this pull request
Oct 1, 2026
## 🗒️ 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 - libp2p/rust-libp2p#4773: the same assert, open. The maintainers' advice there is to put connection-management behaviours first, which is what this does. - libp2p/rust-libp2p#4870: the underlying design issue (the `handle_*` callbacks take `&mut self`), open. - 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 - [x] 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 - [x] 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
This branch has not been deployed
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
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.
Description
The problem
I've been experiencing panics with
debugbuilds in some tests around therayls blockchain, where
rust-libp2pv0.56.0is used.After some investigation it turns out
request-responserecords a connection inself.connectedfrom
handle_established_inbound_connection/handle_established_outbound_connection. Those run athandler-creation time, before the swarm has committed to the connection. In a composed
NetworkBehaviourthe derive calls each field'shandle_established_*in order and?-propagatesthe result, so a sibling ordered after
request-responsereturningConnectionDeniedaborts theconnection once we have already recorded it. Neither
ConnectionEstablishednorConnectionClosedever follows for that id, and the entry is left behind as a phantom.
That has two consequences:
self.connecteddesynchronises fromremaining_established, so closing the peer's last realconnection reports
remaining_established == 0against a non-empty list and trips thedebug_assert_eq!inon_connection_closed. This is the panic I was seeing.send_requestnotifies aconnection_idthat has no handler behind it instead of dialing the peer. This one is silent andhappens in release builds too.
What's changed
A
self.connectedentry is now created onFromSwarm::ConnectionEstablished, the first point atwhich the swarm has committed to the connection, so
connectedmirrors the swarm's established setby construction.
handle_established_*become pure handler factories with no side effects.Queued requests can no longer be preloaded into the handler at construction, so they are emitted as
ToSwarm::NotifyHandler { handler: NotifyHandler::One(connection_id), .. }instead. Same queue, sameconnection — they are just dispatched on the next
pollrather than baked into the handler.AI Assistance Disclosure
Tools used (required — write
noneif no AI was used): Claude CodeClaude Code wrote the two regression tests and helped explore alternative fixes, including checking
whether the change could affect consumers negatively. I reviewed and verified the result.
Attestation (required):
Notes & open questions
Reproducing
Keep the two new tests and restore the old recording logic:
Further steps
I also have this backported to
0.56.0here where I no longer observe the panic after a few nights of testing. In this regard if this PR gets merged is it possible that0.56.1gets published with this fix in?I'd be happy to have this resolved so share your feedback.