Ensure peer_disconnected is called after a handler refuses a connection
#3580
Conversation
If one message handler refuses a connection by returning an `Err` from `peer_connected`, other handlers which already got the `peer_connected` will not see the corresponding `peer_disconnected`, leaving them in a potentially-inconsistent state. Here we ensure we call the `peer_disconnected` handler for all handlers which received a `peer_connected` event (except the one which refused the connection).
...to make it identical to all our other message handlers.
This adds a `ConnectionTracker` test util which is used across
`TestChannelMessageHandler`, `TestRoutingMessageHandler` and
`TestCustomMessageHandler`, asserting that `peer_connected` and
`peer_disconnected` methods are well-ordered. This expands test
coverage from just `TestChannelMessageHandler` to cover all test
handlers and adds some useful features which we'll use to test
the fix in the next commit.
This also adds an additional test which tests
`peer_{dis,}connected` consistency when a handler refuses a
connection by returning an `Err` from `peer_connected`.
There was a problem hiding this comment.
Should we enforce uniformity going forward by having the respective handler interfaces inherit from a general trait PeerMessageHandler interface and move peer_{connected, disconnected} and provided_{node,init}_features to it?
Note this would also allow us to iterate over all previously-succeeded handlers rather than having to call them individually (which might get stale at some point in the future).
There was a problem hiding this comment.
Yea, we should definitely require uniformity going forward. We could definitely do a higer-level interface, do you want it in this PR or when we add the next handler?
There was a problem hiding this comment.
We could definitely do a higer-level interface, do you want it in this PR or when we add the next handler?
I'd have a slight preference to do it right away, but feel free to do in a follow-up if you'd rather land this quickly.
There was a problem hiding this comment.
I wrote the patch, but its 36 files changed, 982 insertions(+), 1018 deletions(-), so kinda would prefer to split it up just for sanity.
|
Gonna go ahead and land this as both reviewers said LGTM and don't have remaining comments. |
|
Backported the first commit in #3613 |
v0.1.2 - Apr 02, 2025 - "Foolishly Edgy Cases" API Updates =========== * `lightning-invoice` is now re-exported as `lightning::bolt11_invoice` (lightningdevkit#3671). Performance Improvements ======================== * `rapid-gossip-sync` graph parsing is substantially faster, resolving a regression in 0.1 (lightningdevkit#3581). * `NetworkGraph` loading is now substantially faster and does fewer allocations, resulting in a 20% further improvement in `rapid-gossip-sync` loading when initializing from scratch (lightningdevkit#3581). * `ChannelMonitor`s for closed channels are no longer always re-persisted immediately after startup, reducing on-startup I/O burden (lightningdevkit#3619). Bug Fixes ========= * BOLT 11 invoices longer than 1023 bytes long (and up to 7089 bytes) now properly parse (lightningdevkit#3665). * In some cases, when using synchronous persistence with higher latency than the latency to communicate with peers, when receiving an MPP payment with multiple parts received over the same channel, a channel could hang and not make progress, eventually leading to a force-closure due to timed-out HTLCs. This has now been fixed (lightningdevkit#3680). * Some rare cases with multi-hop BOLT 11 route hints or multiple redundant blinded paths could have led to the router creating invalid `Route`s were fixed (lightningdevkit#3586). * Corrected the decay logic in `ProbabilisticScorer`'s historical buckets model. Note that by default historical buckets are only decayed if no new datapoints have been added for a channel for two weeks (lightningdevkit#3562). * `{Channel,Onion}MessageHandler::peer_disconnected` will now be called if a different message handler refused connection by returning an `Err` from its `peer_connected` method (lightningdevkit#3580). * If the counterparty broadcasts a revoked state with pending HTLCs, those will now be claimed with other outputs which we consider to not be vulnerable to pinning attacks if they are not yet claimable by our counterparty, potentially reducing our exposure to pinning attacks (lightningdevkit#3564).
This is similar to #3110 but does the disconnection calls inline to avoid the races described in the discussion there.
Only the first commit should be backported.