Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge 0.1.1 into 0.1-bindings #3571

Open
wants to merge 22 commits into
base: 0.1-bindings
Choose a base branch
from

Conversation

TheBlueMatt
Copy link
Collaborator

No description provided.

arik-so and others added 22 commits January 28, 2025 18:17
Previously, the `feerate_bump` method did not enforce the dust
threshold, which could result in us thinking we had raised the fee
rate without actually having done so. Instead,
`compute_package_output` blindly accepted the updated fee rate while
enforcing a non-dust output value, resulting in repeated broadcast
attempts of an identical transaction.

Conflicts due to removal of a preceding commit resolved in:
 * lightning/src/chain/package.rs
Bitcoin Core relay policy does not require 16s/vB, which it was
previously set to.

Trivial conflicts due to removal of a preceding commit resolved in:
 * lightning/src/chain/chaininterface.rs
Create some tests for various `feerate_bump` scenarios and ensure
among other thigns that there are no underflows.
We need to stop passing this Vec by value for the next commit so we can pass it
to a different method.
Prior to this patch, if we attempted to send a payment or probe to a buggy
route, we would error but continue storing the pending outbound payment
forever. Attempts to retry would result in a “duplicate payment” error.

In the case of ChannelManager::send_payment, we would also fail to generate a
PaymentFailed event, even if the user manually called abandon_payment.

This bug is unlikely to have ever been hit in the wild as most users use LDK’s
router. Discovered in the course of adding a new send_to_route API.

Now, we’ll properly generate events and remove the outbound from storage.
When an outbound payment fails while paying to a route, we need to remove the
session_privs for each failed path in the outbound payment.

Previously we were sometimes removing in pay_route_internal and sometimes in
handle_pay_route_err, so refactor this so we always remove in
handle_pay_route_err.
Support more ergonomically sending payments to specific routes.

We removed the original version of this API because it was hard to work with,
but the concept of sending a payment to a specific route is still useful.
Previously, users were able to do this via manually matching the payment id in
their router, but that's cumbersome when we could just handle it internally.

Trivial `use` conflicts resolved in:
 * lightning/src/ln/chanmon_update_fail_tests.rs
 * lightning/src/ln/functional_tests.rs
Silent rebase conflicts resolved in:
 * lightning/src/routing/router.rs
for remote signing of invoices
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.
.. previously we just used the 4032 magic number, here we put it in a
`pub const` that is reusable elsewhere.
Previously, we would prune tracked descriptors once we see a spend hit
`ANTI_REORG_DELAY = 6` confirmations. However, this could lead to a
scenario where lingering `ChannelMonitor`s waiting to be archived would
still regenerate and replay `Event::SpendableOutput`s, i.e., we would
re-add the same (now unspendable due to be actually being already spent)
outputs again after having intially pruned them.

Here, we therefore keep the tracked descriptors around for longer, in
particular at least `ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038`
confirmations, at which point we assume the lingering monitors to have
been likely archived, and it's 'safe' for us to also forget about the
descriptors.
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants