Send bogus ChannelReestablish for unknown channels #2658
Merged
Conversation
This file contains 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
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2658 +/- ##
==========================================
- Coverage 89.02% 89.02% -0.01%
==========================================
Files 112 112
Lines 87168 87513 +345
Branches 87168 87513 +345
==========================================
+ Hits 77605 77907 +302
- Misses 7327 7365 +38
- Partials 2236 2241 +5
☔ View full report in Codecov by Sentry. |
TheBlueMatt
reviewed
Oct 11, 2023
|
Can you update the error message "Peer sent a garbage channel_reestablish (usually an lnd node with lost state asking us to force-close for them)"? Otherwise LGTM. |
|
Thoughts on also disconnecting on error/broadcast? This would ensure we also get the counterparty to broadcast whenever we reconnect. |
|
Yea, I think we should. Just change the |
wpaulino
force-pushed
the
bogus-channel-reestablish
branch
from
5710e1f to
9d7dc48
Compare
I think I covered all the cases where we actually have a commitment transaction to broadcast, would appreciate if you could double check. |
wpaulino
force-pushed
the
bogus-channel-reestablish
branch
from
9d7dc48 to
6cbfecb
Compare
arik-so
reviewed
Oct 17, 2023
arik-so
previously approved these changes
Oct 18, 2023
TheBlueMatt
reviewed
Oct 18, 2023
Unfortunately, lnd doesn't force close on errors (https://github.com/lightningnetwork/lnd/blob/abb1e3463f3a83bbb843d5c399869dbe930ad94f/htlcswitch/link.go#L2119). One of the few ways to get an lnd counterparty to force close is by replicating what they do when restoring static channel backups (SCBs). They send an invalid `ChannelReestablish` with `0` commitment numbers and an invalid `your_last_per_commitment_secret`. Since we received a `ChannelReestablish` for a channel that doesn't exist, we can assume it's likely the channel closed from our point of view, but it remains open on the counterparty's side. By sending this bogus `ChannelReestablish` message now as a response to theirs, we trigger them to force close broadcasting their latest state. If the closing transaction from our point of view remains unconfirmed, it'll enter a race with the counterparty's to-be-broadcast latest commitment transaction.
We do this to ensure that the counterparty will always broadcast their latest state when we broadcast ours. Usually, they'll do this with the `error` message alone, but if they don't receive it or ignore it, then we'll force them to broadcast by sending them a bogus `channel_reestablish` upon reconnecting. Note that this doesn't apply to unfunded channels as there is no commitment transaction to broadcast.
Not doing so caused a lock order inversion between the locks `ChannelManager::best_block` and `ChannelManager::short_to_chan_info` after the addition of `test_trigger_lnd_force_close`. It turns out that we were holding the `short_to_chan_info` for longer than needed when processing HTLC forwards. We only need to acquire it to quickly obtain channel info, and there aren't any other locks within `forwarding_channel_not_found` that depend on it being held.
wpaulino
force-pushed
the
bogus-channel-reestablish
branch
from
6cbfecb to
ed38eac
Compare
TheBlueMatt
approved these changes
Oct 18, 2023
arik-so
approved these changes
Oct 18, 2023
k0k0ne
pushed a commit
to bitlightlabs/rust-lightning
that referenced
this pull request
Sep 30, 2024
0.0.118 - Oct 23, 2023 - "Just the Twelve Sinks" API Updates =========== * BOLT12 sending and receiving is now supported as an alpha feature. You may run into unexpected issues and will need to have a direct connection with the offer's blinded path introduction points as messages are not yet routed. We are seeking feedback from early testers (lightningdevkit#2578, lightningdevkit#2039). * `ConfirmationTarget` has been rewritten to provide information about the specific use LDK needs the feerate estimate for, rather than the generic low-, medium-, and high-priority estimates. This allows LDK users to more accurately target their feerate estimates (lightningdevkit#2660). For those wishing to retain their existing behavior, see the table below for conversion. * `ChainHash` is now used in place of `BlockHash` where it represents the genesis block (lightningdevkit#2662). * `lightning-invoice` payment utilities now take a `Deref` to `AChannelManager` (lightningdevkit#2652). * `peel_onion` is provided to statelessly decode an `OnionMessage` (lightningdevkit#2599). * `ToSocketAddrs` + `Display` are now impl'd for `SocketAddress` (lightningdevkit#2636, lightningdevkit#2670) * `Display` is now implemented for `OutPoint` (lightningdevkit#2649). * `Features::from_be_bytes` is now provided (lightningdevkit#2640). For those moving to the new `ConfirmationTarget`, the new variants in terms of the old mempool/low/medium/high priorities are as follows: * `OnChainSweep` = `HighPriority` * `MaxAllowedNonAnchorChannelRemoteFee` = `max(25 * 250, HighPriority * 10)` * `MinAllowedAnchorChannelRemoteFee` = `MempoolMinimum` * `MinAllowedNonAnchorChannelRemoteFee` = `Background - 250` * `AnchorChannelFee` = `Background` * `NonAnchorChannelFee` = `Normal` * `ChannelCloseMinimum` = `Background` Bug Fixes ========= * Calling `ChannelManager::close_channel[_with_feerate_and_script]` on a channel which did not exist would immediately hang holding several key `ChannelManager`-internal locks (lightningdevkit#2657). * Channel information updates received from a failing HTLC are no longer applied to our `NetworkGraph`. This prevents a node which we attempted to route a payment through from being able to learn the sender of the payment. In some rare cases, this may result in marginally reduced payment success rates (lightningdevkit#2666). * Anchor outputs are now properly considered when calculating the amount available to send in HTLCs. This can prevent force-closes in anchor channels when sending payments which overflow the available balance (lightningdevkit#2674). * A peer that sends an `update_fulfill_htlc` message for a forwarded HTLC, then reconnects prior to sending a `commitment_signed` (thus retransmitting their `update_fulfill_htlc`) may result in the channel stalling and being unable to make progress (lightningdevkit#2661). * In exceedingly rare circumstances, messages intended to be sent to a peer prior to reconnection can be sent after reconnection. This could result in undefined channel state and force-closes (lightningdevkit#2663). Backwards Compatibility ======================= * Creating a blinded path to receive a payment then downgrading to LDK prior to 0.0.117 may result in failure to receive the payment (lightningdevkit#2413). * Calling `ChannelManager::pay_for_offer` or `ChannelManager::create_refund_builder` may prevent downgrading to LDK prior to 0.0.118 until the payment times out and has been removed (lightningdevkit#2039). Node Compatibility ================== * LDK now sends a bogus `channel_reestablish` message to peers when they ask to resume an unknown channel. This should cause LND nodes to force-close and broadcast the latest channel state to the chain. In order to trigger this when we wish to force-close a channel, LDK now disconnects immediately after sending a channel-closing `error` message. This should result in cooperative peers also working to confirm the latest commitment transaction when we wish to force-close (lightningdevkit#2658). Security ======== 0.0.118 expands mitigations against transaction cycling attacks to non-anchor channels, though note that no mitigations which exist today are considered robust to prevent the class of attacks. * In order to mitigate against transaction cycling attacks, non-anchor HTLC transactions are now properly re-signed before broadcasting (lightningdevkit#2667). In total, this release features 61 files changed, 3470 insertions, 1503 deletions in 85 commits from 12 authors, in alphabetical order: * Antonio Yang * Elias Rohrer * Evan Feenstra * Fedeparma74 * Gursharan Singh * Jeffrey Czyz * Matt Corallo * Sergi Delgado Segura * Vladimir Fomene * Wilmer Paulino * benthecarman * slanesuke
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.
Unfortunately, lnd doesn't force close on errors
(https://github.com/lightningnetwork/lnd/blob/abb1e3463f3a83bbb843d5c399869dbe930ad94f/htlcswitch/link.go#L2119). One of the few ways to get an lnd counterparty to force close is by replicating what they do when restoring static channel backups (SCBs). They send an invalid
ChannelReestablishwith0commitment numbers and an invalidyour_last_per_commitment_secret.Since we received a
ChannelReestablishfor a channel that doesn't exist, we can assume it's likely the channel closed from our point of view, but it remains open on the counterparty's side. By sending this bogusChannelReestablishmessage now as a response to theirs, we trigger them to force close broadcasting their latest state. If the closing transaction from our point of view remains unconfirmed, it'll enter a race with the counterparty's to-be-broadcast latest commitment transaction.Confirmed locally this will trigger a force close on both sides (LDK <-> LND) after:
ChannelReestablishfirst and LDK responding with its own