Skip to content

Roll back composite sub-handlers when one rejects peer_connected#4595

Open
tnull wants to merge 1 commit intolightningdevkit:mainfrom
tnull:2026-05-composite-handler-peer-connected-rollback
Open

Roll back composite sub-handlers when one rejects peer_connected#4595
tnull wants to merge 1 commit intolightningdevkit:mainfrom
tnull:2026-05-composite-handler-peer-connected-rollback

Conversation

@tnull
Copy link
Copy Markdown
Contributor

@tnull tnull commented May 5, 2026

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

@tnull tnull requested a review from jkczyz May 5, 2026 19:16
@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented May 5, 2026

👋 Thanks for assigning @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

ldk-claude-review-bot commented May 5, 2026

No issues found.

I reviewed the entire diff thoroughly:

  • Macro logic: The rollback correctly calls all sub-handlers, records results, and on any failure calls peer_disconnected only on handlers that returned Ok. This matches the peer_connected/peer_disconnected trait contract (line 109 of peer_handler.rs).
  • Macro hygiene: any_err (macro-definition context) and $field (call-site context) are in different hygiene scopes — no collision risk.
  • Variable shadowing: let $field = self.$field.peer_connected(...) shadows the field name locally, but self.$field still correctly accesses the struct field for the peer_disconnected rollback call.
  • Edge cases: Single handler (rollback check is a no-op since the one that failed won't pass is_ok()), zero handlers (always returns Ok(())).
  • Test coverage: Correctly verifies rollback via atomic counter, and debug_assert!(false) in ErroringHandler::peer_disconnected catches any incorrect rollback of the failing handler.

Comment thread lightning-custom-message/src/lib.rs Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

❌ Patch coverage is 37.64706% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.11%. Comparing base (1a26867) to head (5455058).
⚠️ Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
lightning-custom-message/src/lib.rs 37.64% 51 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4595      +/-   ##
==========================================
- Coverage   86.84%   86.11%   -0.73%     
==========================================
  Files         161      157       -4     
  Lines      109260   108772     -488     
  Branches   109260   108772     -488     
==========================================
- Hits        94882    93668    -1214     
- Misses      11797    12487     +690     
- Partials     2581     2617      +36     
Flag Coverage Δ
fuzzing-fake-hashes ?
fuzzing-real-hashes ?
tests 86.11% <37.64%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from needing to add the debug_assert.

`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>
@tnull tnull force-pushed the 2026-05-composite-handler-peer-connected-rollback branch from 7de6891 to 5455058 Compare May 6, 2026 09:05
@tnull tnull requested review from TheBlueMatt and jkczyz May 6, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants