AI-generated analysisPublished automatically and not human-verified. Validated context appears in community notes below.
← Watch feed
Moderate 60 Bitcoin

Roll back composite sub-handlers when one rejects `peer_connected`

Public commit record

What the developer wrote

Authored by Elias Rohrer

73/100 · Adequate
Roll back composite sub-handlers when one rejects `peer_connected`

`composite_custom_message_handler!` expanded `peer_connected` to call
every sub-handler and remember the last error, but never undo the
already-succeeded ones. The `CustomMessageHandler::peer_connected`
contract is that `PeerManager` will *not* invoke `peer_disconnected`
when `peer_connected` returns `Err` — so any per-peer state allocated
by an earlier sub-handler that returned `Ok` was leaked permanently
once a later sub-handler returned `Err`.

A peer who can elicit `Err` from any sub-handler in the composite
(feature-bit gate, banlist, etc.) could repeatedly reconnect to grow
that leaked state without bound (slow resource DoS), and "currently
connected" predicates in the leaking sub-handler would lie about
peers that were actually rejected.

Mirror the rollback pattern `PeerManager` already uses for the four
built-in handlers (`peer_handler.rs:2149-2188`): record each
sub-handler's `peer_connected` result, and if any returned `Err`,
call `peer_disconnected` on the ones that succeeded before
propagating the failure.

Co-Authored-By: HAL 9000
Signed-off-by: Elias Rohrer <dev@tnull.de>
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context
The short version

What changed, and why it matters

This commit fixes a bug in a Rust macro that combines multiple custom Lightning message handlers. Previously, if one sub-handler rejected a new peer connection, the handlers that had already accepted it were not told the connection failed. That left behind stale per-peer state that could accumulate over reconnects, causing a slow resource drain and making internal 'is this peer connected?' checks lie. The patch now rolls back the successful sub-handlers by calling their disconnect routine before returning the error, and adds a test proving no state leaks.

Recommended action

Apply the patch and run the new unit test. Users running composite custom message handlers should upgrade to avoid unbounded per-peer state growth and incorrect connection predicates.

Security signals we found

01

Resource leak / slow DoS from repeated reconnects when a composite sub-handler rejects `peer_connected`

02

State inconsistency: sub-handlers believe a peer is connected after the composite rejected the peer

03

Violation of `CustomMessageHandler::peer_connected` contract remedied by explicit rollback

04

New regression test `peer_connected_failure_does_not_leak_subhandler_state` added

Risk score

Why this scored 60/100

Our methodology →
Potential impact 14/30
Exploitability 13/25
Stealth signal 9/15
Affected reach 10/15
Confidence 9/10
Evidence quality 5/5
Human-validated context

Community notes

Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.

No validated notes yet.

The AI analysis stands alone for now. Submit a note if you can add evidence or important context.