Merge pull request #2658 from wpaulino/bogus-channel-reestablish

Send bogus ChannelReestablish for unknown channels

Merge pull request #2599 from Evanfeenstra/peel-onion

public static peel_onion method on OnionMessenger

Release short_to_chan_info lock throughout forwarding_channel_not_found

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.

Disconnect peer when force closing a funded channel with an error

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.

Send bogus ChannelReestablish for unknown channels

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.

public static peel_onion method on OnionMessenger

Merge pull request #2662 from jkczyz/2023-10-chain-hash

Use `ChainHash` instead of `BlockHash` as applicable

Merge pull request #2665 from TheBlueMatt/2023-10-scanable-test-logs

Make test log lines somewhat more eye-scannable

Use ChainHash instead of BlockHash as applicable

ChainHash is more appropriate for places where an arbitrary BlockHash is
not desirable. This type was introduced in later versions of the bitcoin
crate, thus BlockHash was used instead.

Using ChainHash also makes it easier to check if ChannelManager is
compatible with an Offer.

Merge pull request #2664 from TheBlueMatt/2023-10-ci-disk-usage

Reduce disk usage in CI

Merge pull request #2625 from tnull/2023-09-moar-tests-n-fixes

Improve test coverage of #2575 router fixes

Make test log lines somewhat more eye-scannable

When running tests, our log output should be reasonably readable
by developers, but currently it repeats the module twice (via the
module and file name), and then starts the log line at a variable
location.

Instead, we only print the module and then align the start of the
log lines so that the output is much more scannable.

Reduce disk usage in CI

Recently github appears to have reduced the available free disk
space in actions runs, causing CI to fail with out of space errors.
Here we simply run `cargo clean` a few times in CI to reduce our
disk usage somewhat.

Merge pull request #2655 from TheBlueMatt/2023-10-no-test-net

Replace `lightning-block-sync` test that depended on `foo.com`

Merge pull request #2639 from vladimirfomene/fix-final-incorrect-cltv

Fix final incorrect cltv

fix: use the update_add_htlc's cltv_expiry for comparison

As noted in BOLT 4, we should be using the update_add_htlc's cltv_expiry,
not the CLTV expiry set by the sender in the onion for this comparison.
See here: https://github.com/lightning/bolts/blob/4dcc377209509b13cf89a4b91fde7d478f5b46d8/04-onion-routing.md?plain=1#L334

Replace `lightning-block-sync` test that depended on `foo.com`

Our tests should generally not rely on internet access, and should
not rely on the behavior of any given remote server. However, one
of the `endpoint_tests` in `lightning-block-sync::http` relied on
`foo.com` resolving to a single socket address, which both might
change in the future and makes our tests fail without internet.

Merge pull request #2650 from TheBlueMatt/2023-10-make-clippy-shut-up

Make clippy shut up about `PartialOrd` and `Ord` both impl'd

Merge pull request #2657 from Fedeparma74/fix-close-channel-deadlock

Fix deadlock when closing an unavailable channel

Fix deadlock when closing an unavailable channel

Merge pull request #2652 from tnull/2023-10-deref-achannelmanager

Merge pull request #2651 from G8XSU/rm-same-feerate-log

Remove log for not changing feerate when it was equal

Merge pull request #2649 from benthecarman/display-outpoint

Impl Display for Outpoint

Have methods take `AChannelManager` as `Deref::Target`

Impl Display for Outpoint

Remove log for not changing feerate when it was equal

Log is not required in this case and creates unnecessary log lines at
trace level.

Make clippy shut up about `PartialOrd` and `Ord` both impl'd

Clippy gets mad that we have an implementation of `ParialOrd` and
`Ord` separately, even though both are identical. Making
`ParitalOrd` call `Ord` makes clippy shut up.

Merge pull request #2640 from sr-gi/20231003-expose-from-be-bytes

Makes Features::from_be_bytes public

Makes Features::from_be_bytes public

Downstream projects building Feature<T> are most likely doing so with a
big-endian byte array, however only `from_le_bytes` is exposed.

Merge pull request #2645 from TheBlueMatt/2023-10-changelog-dates

Set the timestamp for and provide diff stats for 0.0.117 CHANGELOG

Set the timestamp for and provide diff stats for 0.0.117 CHANGELOG

Merge pull request #2620 from TheBlueMatt/2023-09-cut-0.0.117

Cut 0.0.117

Bump crate versions to 0.0.117/invoice 0.25

Add 0.0.117 release notes

Merge pull request #2631 from TheBlueMatt/2023-09-pm-no-refs-reqd

Fix `Simple*PeerManager` to not require refs to the `UtxoLookup`

Merge pull request #2634 from TheBlueMatt/2023-09-claimable-unwrap

Avoid unwrap'ing `channel_parameters` in to_counterparty signing

Clean up at least some lifetimes on `SimpleRefPeerManager`

Rather than simply a, b, c, d...we at least use names for a few
things, also splitting the reused 'f lifetime.

Note when we're allowed to unwrap channel parameters in docs

This further documents the parameter-fetching utilities in
`InMemorySigner` to hopefully make them more robust against future
spurious `unwrap`s.

Make `InMemorySigner` parameter-fetching utilities return `Option`s

In the previous commit we fixed a bug where we were spuriously
(indirectly) `unwrap`ing the `channel_parameters` in
`InMemorySigner`. Here we make such bugs much less likely in the
future by having the utilities which do the `unwrap`ing internally
return `Option`s instead.

This makes the `unwrap`s clear at the callsite.

Merge pull request #2595 from TheBlueMatt/2023-09-117-bindings-part1

Various cleanups to make bindings generation simpler

Merge pull request #2632 from TheBlueMatt/2023-09-writeable-rwlock

Implement Readable/Writeable for RwLock wrappers

Merge pull request #2633 from TheBlueMatt/2023-09-expose-onion-parse

Expose `parse_onion_address` publicly in `no-std`

Avoid unwrap'ing `channel_parameters` in to_counterparty signing

Previously, `StaticPaymentOutputDescriptor`s did not include
`channel_parameters` for the signer. As a result, when going to
spend old `StaticPaymentOutputDescriptor`s,
`InMemorySigner::sign_counterparty_payment_input` may be called
with `channel_parameters` set to `None`. This should be fine, but
in fa2a2efef47b5da48ac7a239f3d8a835a7f28164 we started relying on
it (indirectly via `channel_features`) for signing. This caused an
`unwrap` when spending old output descriptors.

This is fixed here by simply avoiding the unwrap and assuming old
`StaticPaymentOutputDescriptor`s represent non-anchor channels.

Remove unused `mem::drop` which drops a reference

Fix new unused variable warning in `test_utils.rs`

Use `crate::prelude::*` to load `Vec`, rather than `crate::Vec`

This is kinda dumb, but the bindings get confused when referring
to `Vec` absolutely in a `use` statement, and there's no reason not
to load our prelude everywhere.

Drop various bounds on types passed to `MonitorUpdatingPersister`

The new `MonitorUpdatingPersister` has a few redundant type bounds
(re-specified on functions after having been specified on the
struct itself), which we remove here.

Further, it requires a `Deref<FeeEstimator>` which is `Clone`able.
This is generally fine in rust, but annoying in bindings, so we
simply elide it in favor if a `&Deref<FeeEstimator>`.

Make `create_onion_message` a freestanding function

The new `create_onion_message` function in `OnionMessenger` is hard
to handle - it has various generic requirements indirectly via the
struct, but they're not bounded by any of the method parameters.
Thus, you can't simply call `OnionMessenger::create_onion_message`,
as various bounds are not specified.

Instead, we move it to a freestanding function so that it can be
called directly without explicitly setting bounds.

Mark `AChannelManager` no-export

The trait itself has no purpose for bindings, as all structs are
concretized anyway. Further, the bindings have specific handling
for generic bounding traits like this.

Mark `SpendableOutputDescriptor::to_psbt_input` as no-export

Its honestly likely not all that useful as its not materially
interoperable with other PSBT libraries. Instead, users should
simply fetch the full PSBT and use the inputs from it as they see
fit.

Drop unnecessary crate:: prefix when accessing `bitcoin` in macro

Unexported macros don't need to use the `$crate` prefix.

Avoid ref to a `Vec` when accessing custom onion TLVs

The bindings can't easily return a reference to a `Vec`, so we
instead split the implementation into vec/slice depending on the
`c_bindings` flag.

Avoid blanket impls in bindings builds

The C bindings implements `Deref` for all traits, so having a
blanket `Deref` implementation ends up conflicting with this.

Simplify score bounding with a unified type

In a few places we require a unified scorer, which implements both
`ScoreLookUp` and `ScoreUpdate`. Rather than double-bounding (which
the bindings generator can't handle directly), we use a top-level
`Score` trait which requires both and is implemented for all
implementers of both supertraits.

Remove unnecessary bounds in scoring

In our scoring logic we have a handful of unnecessary bounds,
leading to extra diff in the bindings branch when those bounds have
to be removed as well as a few cases where bindings generation
simply gets confused.

Here we remove a number of bounds across the scoring traits and
impls, cleaning things up and simplifying bindings changes.

Use `Default::default()` to construct `()` as score-updating param

In 6b0d94a3029f74de3a7542cbba0d48c2f7e5866b we switched most tests
to `Default::default()` for scoring parameters passed to
route-fetching. Here we do the same for the scoring parameters when
passed to score-updating.

Expose `parse_onion_address` publicly in `no-std`

The reason for having a separate `parse_onion_address` from
`FromStr` is to have an onion parsing function in `no-std`, but
when we added it we forgot to make it public. We do this here, as
well as fix a few compilation warnings in `no-std`.

Switch `Simple*ChannelManager` locks around `Score` to `RwLock`

This switches the locks used around `ProbabilisticScorer` in
`Simple*ChannelManager` type aliases to `RwLock`.

Fix `Simple*PeerManager` to not require refs to the `UtxoLookup`

`UtxoLookup` doesn't strictly need to be referenced from the
`PeerManager`, and in fact the new `GossipVerifier` in
`lightning-block-sync` requires itself to be owned by the
`PeerManager` (for circular type reasons).

This allows us to use `lightning-block-sync`'s `GossipVerifier`
with `SimpleArcPeerManager` in ldk-sample.

Implement `Readable`/`Writeable` for `RwLock` wrappers

We now support separate R/W locks in `LockableScore`, which allow
us to do routefinding in parallel, however in order to support
`WriteableScore` for such users we need to implement `Writeable`
for `RwLock` wrappers around `Writeable` types, which we do here.

Merge pull request #2630 from TheBlueMatt/2023-09-117-rc1

Bump crate versions to 0.0.117-rc1/invoice 0.25-rc1

Merge pull request #2610 from wpaulino/missing-htlc-claim-balance

Fix matching of second-stage HTLC claim in get_htlc_balance

Merge pull request #2621 from G8XSU/dont-persist-erroneous-update

Persist entire monitor if there is an error while applying monitor_update

Bump crate versions to 0.0.117-rc1/invoice 0.25-rc1

Fix wrong node broadcaster in test_yield_anchors_events

Add anchors coverage to test_revoked_counterparty_aggregated_claims

Add anchors coverage to test_revoked_counterparty_htlc_tx_balances

Add update_persisted_channel doc to indicate rare case for None update

Persist full monitor if there is an error while applying monitor_update

Motivation: When there is an error while applying monitor_update to a
channel_monitor,  we don't want to persist a 'monitor_update' which
results in a failure to apply later while reading 'channel_monitor' with
updates from storage. Instead, we should persist the entire 'channel_monitor'
here.

Correctly mark chain_sync updates in test_utils

We were incorrectly marking updates as chain_sync
or not in test_utils based on whether monitor_update
is None or not. Instead, use UpdateOrigin to determine it.

Add anchors coverage to test_revoked_counterparty_commitment_balances

Add anchors coverage to test_balances_on_local_commitment_htlcs

Add anchors coverage to test_claim_value_force_close

Add anchors coverage to chanmon_claim_value_coop_close

Add test util to handle bump HTLC events

Fix matching of second-stage HTLC claim in get_htlc_balance

We incorrectly assumed that the descriptor's output index from
second-stage HTLC transaction would always match the HTLC's output index
in the commitment transaction. This doesn't make any sense though, we
need to make sure we map the descriptor to it's corresponding HTLC in
the commitment. Instead, we check that the transaction from which the
descriptor originated from spends the HTLC in question.

Note that pre-anchors, second-stage HTLC transactions are always 1
input-1 output, so previously we would only match if the HTLC was the
first output in the commitment transaction. Post-anchors, they are
malleable, so  we can aggregate multiple HTLC claims into a single
transaction making this even more likely to happen. Unfortunately, we
lacked proper coverage in this area so the bug went unnoticed. To
address this, we aim to extend our existing coverage of
`get_claimable_balances` to anchor outputs channels in the following
commits.

Merge pull request #2629 from jkczyz/2023-09-invreqfailed

Config-guard `Event::InvoiceRequestFailed`

Merge pull request #2605 from wpaulino/anchors-monitor-track-to-remote-script

Use correct to_remote script in counterparty commitments

Remove unused imports

Fix build warning for unused method

Remove an unnecessary enumerate

Config-guard Event::InvoiceRequestFailed

The event cannot be generated publicly, so remove it for now to avoid
users needing to handle it.

Merge pull request #2628 from wvanlint/fix_multiple_shutdown_results

Fix handling multiple ShutdownResults

Expose witness_script for StaticPaymentOutputDescriptor

Fix incorrect anchors `counterparty_payment_script` upon deserialization

Merge pull request #2622 from wpaulino/funding-and-commitment-tx-confirm-same-block

Avoid early return upon confirmation of channel funding

Merge pull request #2626 from TheBlueMatt/2023-09-revert-2476

Revert "Remove AvailableBalances::balance_msat"

Fix off-by-one max witness estimate for P2WPKH StaticPaymentDescriptor

We were not accounting for the extra byte denoting the number of items
in the witness stack.

Support signing to_remote anchors variant for StaticPaymentOutput

`to_remote` outputs on commitment transactions with anchor outputs have
an additional `1 CSV` constraint on its spending condition,
transitioning away from the previous P2WPKH script to a P2WSH.

Since our `ChannelMonitor` was never updated to track the proper
`to_remote` script on anchor outputs channels, we also missed updating
our signer to handle the new script changes.

Use correct to_remote script in counterparty commitments

While our commitment transactions did use the correct `to_remote`
script, the `ChannelMonitor`'s was not as it is tracked separately. This
would lead to users never receiving an `Event::SpendableOutputs` with a
`StaticPaymentOutput` descriptor to claim the funds.

Luckily, any users affected which had channel closures confirmed by a
counterparty commitment just need to replay the closing transaction to
receive the event.

Merge pull request #2624 from wpaulino/2609-follow-up

Address 2609 follow-up comments

Fix handling multiple ShutdownResults

Merge pull request #2623 from wpaulino/htlc-claim-receive-preimage-after-close

Claim HTLCs with preimage from currently confirmed commitment

Avoid early return upon confirmation of channel funding

This early return is only possible if the channel requires a single
confirmation, allowing a `channel_ready` message to go out. This can be
problematic though if a commitment transaction (specifically from the
counterparty, as the channel would be immediately closed if a local
commitment is broadcast) also confirms within the same block. The
`ChannelMonitor` will detect both, but it won't inform the
`ChannelManager` at all. Luckily, while the channel still is considered
open to the `ChannelManager`, the `ChannelMonitor` will reject any
further updates to the channel state.

Revert "Remove AvailableBalances::balance_msat"

While removing the `balance_msat` field absolutely makes sense -
it is, at best, confusing - we really need a solid replacement for
it before we can do so. While one such replacement is in progress,
it is not complete and we'd like to not block our current release
on its completion.

This reverts commit ef5be580f51d6756612fea710516c0297203f4dc.

Test we consider route hints if we are the src of the first hop

Previously, we would only consider route hints if the entry point was
in our first hops or in the network graph. We fixed this by also
considering hints if our own node ID was the first src.

Here, we add test coverage for this behavior.

Test we prefer first hops over route hints

We previously added logic that would avoid adding superflous candidates
for route hints if we detect that we have a first hop for this channel.

Here we add test coverage that we actually prefer the first hop over the
route hint, but still consider the remaining hints.

Only yield DelayedPaymentOutput descriptors once their delay expires

Otherwise, we could give users a descriptor ahead of time that will
result in an invalid transaction spend/broadcast.

Merge pull request #2617 from wpaulino/no-persist-same-channel-update

Avoid persisting on same counterparty's ChannelUpdate

Merge pull request #2591 from TheBlueMatt/2023-09-2562-followups

Doc and comment followups to #2562