Fail HTLC backwards before upstream claims on-chain #3556
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
In a coming commit we'll expire HTLCs backwards even if we haven't yet claimed them on-chain based on their inbound edge being close to causing a channel force-closure. Here we track the incoming edge's CLTV expiry in the pending-routing state so that we can include it in the `HTLCSource` in the next commit. Co-authored-by: Matt Corallo <[email protected]>
In a coming commit we'll expire HTLCs backwards even if we haven't yet claimed them on-chain based on their inbound edge being close to causing a channel force-closure. Here we track and expose the incoming edge's CLTV expiry in the `HTLCSource`, giving `ChannelMonitor` access to it. Co-authored-by: Matt Corallo <[email protected]>
This field was used to test that any HTLC failures didn't come in after an HTLC was fulfilled (indicating, somewhat dubiously, that there may be a bug causing us to fail when we shouldn't have). In the next commit, we'll be failing HTLCs based on on-chain HTLC expiry, but may ultimately receive the preimage thereafter. This would make the `historical_inbound_htlc_fulfills` checks potentially-brittle, so we just remove them as they have dubious value.
Fail inbound HTLCs if they expire within a certain number of blocks from the current height. If we haven't seen the preimage for an HTLC by the time the previous hop's timeout expires, we've lost that HTLC, so we might as well fail it back instead of having our counterparty force-close the channel. Co-authored-by: Matt Corallo <[email protected]>
If we've signed the latest holder tx (i.e. we've force-closed and broadcasted our state), there's not much reason to accept counterparty-transaction-updating `ChannelMonitorUpdate`s, we should make sure the `ChannelManager` fails the channel as soon as possible. This standardizes the failure cases to also match those added to the previous commit, which makes things a bit more readable.
wpaulino
reviewed
Jan 23, 2025
| continue; | ||
| } | ||
| if !duplicate_event { | ||
| log_error!(logger, "Failing back HTLC {} upstream to preserve the \ |
There was a problem hiding this comment.
Nit: is this really considered an error? Info/warn seems better suited.
There was a problem hiding this comment.
Its an error in that a violation of our assumptions around tx confirmation time happened, and its probably an important enough case that users should see it and think hard about what is happening.
| mine_transaction(&nodes[1], &node_1_txn[1]); // HTLC timeout | ||
| connect_blocks(&nodes[1], ANTI_REORG_DELAY); | ||
| // Expect handling another fail back event, but the HTLC is already gone | ||
| expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], |
There was a problem hiding this comment.
We could avoid having this duplicate go out if we check the set on the confirmed timeout path, but I guess it's not worth doing since a restart in between would yield it anyway.
There was a problem hiding this comment.
I also spent a while trying to move the event generation to where we do the actual failure so that we could detect the duplicate in ChannelManager, but the changeset kinda blew up on me :/. And, yea, this should be a really rare/never kinda case, so duplicate handling-failed events skewing users' forwarding statistics is probably not the end of the world.
arik-so
reviewed
Jan 23, 2025
wpaulino
approved these changes
Jan 23, 2025
arik-so
approved these changes
Jan 27, 2025
Merged
|
Backported in #3567 |
3 tasks
TheBlueMatt
added a commit
to TheBlueMatt/rust-lightning
that referenced
this pull request
Jan 29, 2025
v0.1.1 - Jan 28, 2025 - "Onchain Matters" API Updates =========== * A `ChannelManager::send_payment_with_route` was (re-)added, with semantics similar to `ChannelManager::send_payment` (rather than like the pre-0.1 `send_payent_with_route`, lightningdevkit#3534). * `RawBolt11Invoice::{to,from}_raw` were added (lightningdevkit#3549). Bug Fixes ========= * HTLCs which were forwarded where the inbound edge times out within the next three blocks will have the inbound HTLC failed backwards irrespective of the status of the outbound HTLC. This avoids the peer force-closing the channel (and claiming the inbound edge HTLC on-chain) even if we have not yet managed to claim the outbound edge on chain (lightningdevkit#3556). * On restart, replay of `Event::SpendableOutput`s could have caused `OutputSweeper` to generate double-spending transactions, making it unable to claim any delayed claims. This was resolved by retaining old claims for more than four weeks after they are claimed on-chain to detect replays (lightningdevkit#3559). * Fixed the additional feerate we will pay each time we RBF on-chain claims to match the Bitcoin Core policy (1 sat/vB) instead of 16 sats/vB (lightningdevkit#3457). * Fixed a cased where a custom `Router` which returns an invalid `Route`, provided to `ChannelManager`, can result in an outbound payment remaining pending forever despite no HTLCs being pending (lightningdevkit#3531). Security ======== 0.1.1 fixes a denial-of-service vulnerability allowing channel counterparties to cause force-closure of unrelated channels. * If a malicious channel counterparty force-closes a channel, broadcasting a revoked commitment transaction while the channel at closure time included multiple non-dust forwarded outbound HTLCs with identical payment hashes and amounts, failure to fail the HTLCs backwards could cause the channels on which we recieved the corresponding inbound HTLCs to be force-closed. Note that we'll receive, at a minimum, the malicious counterparty's reserve value when they broadcast the stale commitment (lightningdevkit#3556). Thanks to Matt Morehouse for reporting this issue.
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.
This rebases and replaces #2457. It removes the config option, as once we've lost the HTLC in question, not failing back to get three additional blocks of room to claim the inbound edge (after at least 39 blocks have already passed) doesn't seem like a reasonable tradeoff in nearly any case (and I'd like to reduce the 3 number to 2 anyway). It also cleans up
channela bit more by removinghistorical_inbound_htlc_fulfillsentirely, usesSentHTLCId(resolving an issue where two inbound channels have relayed two different HTLCs with the same HTLC ID into a channel causing confusion), and a few other cleanups.I'd like to backport this to 0.1 because its important for future feerate spikes.