Merge pull request #3238 from Mirebella/clippy-checks

Added clippy ignore rules for all errors and warnings

Merge pull request #3275 from TheBlueMatt/2024-08-3259-followups

#3259 followups

Merge pull request #3276 from arik-so/arik/2024/08/apply_monitor_updates_on_archive

Process updates before archiving monitors.

Process updates before archiving monitors.

Previously, `MonitorUpdatingPersister` was disregarding any unapplied
monitor updates when archiving them. This commit ensures that upon
reading monitors, their corresponding updates are also read and
applied prior to archiving.

Store Broadcaster and FeeEstimator on MonitorUpdatingPersister.

`MonitorUpdatingPersister` does not currently correctly archive
monitors because it neglects any unapplied updates. In order to start
applying these updates, the archiving methods will require access to
instances of `BroadcasterInterface` and `FeeEstimator`.

This commit requires that the `MonitorUpdatingPersister` be
instantiated with those instances, obviating the need for passing
them around, and laying the foundation for the following commit.

Test manual funding transaction `Event::DiscardFunding` generation

Correct manual shutdown detection on channel closure

In 5e874c3dc9cf1606a3cbbccab3a0d25089a97280 we'd intended to not
reveal the dummy funding transaction in `Event::DiscardFunding`.
However, instead of looking at the channel that was just closed,
the logic only looks at any other channels which were funded as a
part of the same batch. Because manually-funded transactions
cannot currently be done for batch funding, this was actually dead
code, preventing the new changes from taking effect.

Add additional documentation on `Channel::unbroadcasted_funding`

Merge pull request #3268 from TheBlueMatt/2024-08-moar-feerate-categories

Split up `ConfirmationTarget` even more

Fix CI linting error

Add clippy ignore rules for all errors and warnings

Test new `ConfirmationTarget` selection based on HTLC set

This updates `test_yield_anchors_events` to test both anchor
channels with and without HTLCs, and relies on overriding only the
singular expected `ConfirmationTarget` used, testing the new
`ConfirmationTarget::UrgentOnChainSweep` use.

Don't ignore events in `test_yield_anchors_events`

Our tests should never ignore the events generated as they provide
critical context about what's happening in LDK. Here we fix
`test_yield_anchors_events` to avoid doing so.

Add a constructor and per-target override to `TestFeeEstimator`

This will allow us to test `ConfirmationTarget`s used in functional
tests by setting an override on just the target we expect to be
used.

Add missing pending changelog for downgrade on unsafe funding

Only generate an `Event::DiscardFunding` when we need to

5e874c3dc9cf1606a3cbbccab3a0d25089a97280 changed
`Event::DiscardFunding` to not include a dummy transaction when we
were funded without a full funding tx, but in doing so started
generating `DiscardFunding` events on every channel closure rather
than only when there's actually still a pending funding broadcast.

This restores the previous behavior to only generate the event when
we should actually discard the funding tx.

Merge pull request #3259 from vincenzopalazzo/macros/dummy-tx

[RFC] event: store the outpoint when is_manual_broadcast

event: store the outpoint when is_manual_broadcast

With [1], it's possible to specify `manual_broadcast` for
the channel funding transaction. When `is_manual_broadcast` is
set to true, the transaction in the `DiscardFunding` event is
replaced with a dummy empty transaction.

This commit checks if `is_manual_broadcast` is true and
stores the funding OutPoint in the DiscardFunding event instead.

[1] https://github.com/lightningdevkit/rust-lightning/pull/3024

Link: https://github.com/lightningdevkit/rust-lightning/issues/3164
Suggested-by: TheBlueMatt
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

Split `ConfirmationTarget::OnChainSweep` into urgent and non-urgent

When we force-close a channel, occasionally its due to feerate
disagreements or other non-HTLC-related issues. In those cases,
there's no reason to use a very urgent feerate estimate - we don't
have any timers expiring soon.

Instead, we should give users the information they need to be more
economical on fees in this case, which we do here by splitting
`OnChainSweep` into `UrgentOnChainSweep` and
`NonUrgentOnChainSweep` `ConfirmationTarget`s.

Add a new `ConfirmationTarget::MaximumFeeEstimate`

When we broke `ConfirmationTarget` out into task-specific names, we
left `MaxDustHTLCExposure::FeeRateMultiplier` as using the "when we
broadcast feerate" as we were mostly concerned about the dust
thresholds on outbound channels where we pick the fee and drive our
own funds to dust.

In 51bf78d604b28fe189171e1b976fe87cbb2614b7, that changed to
include transaction fees on both inbound and outbound channels in
our dust exposure amount, but we continued to use
`ConfirmationTarget::OnChainSweep` for the fee estimator threshold.

While the `MaxDustHTLCExposure::FeeRateMultiplier` value is quite
conservative and shouldn't lead to force-closures unless feerate
estimates disagree by something like 500 sat/vB (with only one HTLC
active in a channel), this happened on Aug 22 when feerates spiked
from 4 sat/vB to over 1000 sat/vB in one block.

To avoid simple feerate estimate horizons causing this in the
future, here we add a new
`ConfirmationTarget::MaximumFeeEstimate` which is used for dust
calculations. This allows users to split out the estimates they use
for checking counterparty feerates from the estimates used for
actual broadcasting.

Merge pull request #3273 from TheBlueMatt/2024-08-bindings-no-static

Return owned `String`s for onion message message types

Merge pull request #3248 from jkczyz/2024-08-blinded-path-utils-refactor

Refactor `BlindedPath` construction utils

Return owned `String`s for onion message message types

Returning a reference from a trait method is relatively difficult
to map in bindings and is currently handled by storing the object
in the trait instance, returning a reference to the local field.

This is fine when the object we're returning only needs to live as
long as the trait, but when it needs to be `'static` (as is the
case for onion message `msg_type`s), there's not really a good way
to map them at all.

Instead, here, condition on `#[cfg(c_bindings)]` we return a fully
owned `String`. This is obviously relatively less effecient, but
the extra allocation and `memcpy` isn't the end of the world,
especially given it should be released relatively quickly.

Note that this breaks doctests in with `c_bindings`.

Merge pull request #3272 from TheBlueMatt/2024-08-bindings-hidden

Mark new `types` `#[doc(hidden)]` methods as bindings no-export

Mark `io::Cursor` as no-export in bindings

Mark new `types` `#[doc(hidden)]` methods as bindings no-export

Merge pull request #3153 from alecchendev/2024-07-async-closing-signed

Allow sending closing tx signatures asynchronously

Merge pull request #2992 from G8XSU/monitor-update-seq-doc

Doc Clarity: Handling gaps in persisted ChannelMonitorUpdates.

Merge pull request #3269 from TheBlueMatt/2024-08-this-should-work-bindings

Use the actual type, not the associated type, in trait impls

Test async sign_closing_transaction

Allow sending closing tx signatures asynchronously

Use the actual type, not the associated type, in trait impls

This is equivalent and just easier for the bindings to deal with
(though, really, they should handle it).

Merge pull request #3266 from optout21/bech32-tests

Add invoice serialization tests [TestOnly][Minor]

Add invoice serialization tests

Merge pull request #3263 from TheBlueMatt/2024-08-bindings-om

Remove message type bound on `ResponseInstruction`

Add a `MessageSendInstructions::ForReply`

In order to allow onion message handlers to reply asynchronously
without introducing a circular dependency graph, the message
handlers need to be able to send replies described by
`MessageSendInstructions`. This allows them to send replies via the
normal message queuing (i.e. without making a function call to
`OnionMessenger`).

Here we enable that by adding a `MessageSendInstructions::ForReply`
variant which holds `ReplyInstruction`s.

Fixes #3178

Change `send_onion_message` to take a `MessageSendInstructions`

This lets callers include a `reply_path` without doing the
path-finding at the callsite, utilizing the built-in path-finding
logic in `OnionMessenger`

3/3 Use `MessageSendInstructions` instead of `PendingOnionMessage`

Now that the `MessageRouter` can `create_blinded_paths` forcing
callers of the `OnionMessenger` to provide it with a reply path up
front is unnecessary complexity, doubly so in message handlers.

Here we take the next step towards untangling that, moving from
`PendingOnionMessage` to `MessageSendInstructions` for the outbound
message queue in `AsyncPaymentsMessageHandler`. Better, we can also
drop the `c_bindings`-specific message queue variant, unifying the
APIs.

Here we also drop `PendingOnionMessage` entirely.

2/3 Use `MessageSendInstructions` instead of `PendingOnionMessage`

Now that the `MessageRouter` can `create_blinded_paths` forcing
callers of the `OnionMessenger` to provide it with a reply path up
front is unnecessary complexity, doubly so in message handlers.

Here we take the next step towards untangling that, moving from
`PendingOnionMessage` to `MessageSendInstructions` for the outbound
message queue in `OffersMessageHandler`. Better, we can also drop
the `c_bindings`-specific message queue variant, unifying the APIs.

Because `ChannelManager` needs to actually control the reply path
set in individual messages, however, we have to halfway this patch,
adding a new `MessageSendInstructions` variant that allows
specifying the `reply_path` explicitly. Still, because other message
handlers are moving this way, its nice to be consistent.

1/3 Use `MessageSendInstructions` instead of `PendingOnionMessage`

Now that the `MessageRouter` can `create_blinded_paths` forcing
callers of the `OnionMessenger` to provide it with a reply path up
front is unnecessary complexity, doubly so in message handlers.

Here we take the first step towards untangling that, moving from
`PendingOnionMessage` to `MessageSendInstructions` for the outbound
message queue in `CustomMessageHandler`. Better, we can also drop
the `c_bindings`-specific message queue variant, unifying the APIs.

Give `MessageSendInstructions` any `Destination`, not only Blinded

In the next commit we'll use `MessageSendInstructions` for all
messages, so it needs the ability to take any `Destination`, not
only a `Destination::Blinded`.

Make `ResponseInstruction` `Clone`able

Pull the guts of `ResponseInstruction` into a new enum

In the coming commits we'll use the `ResponseInstruction` enum's
contents for all messages, allowing message senders to rely on the
in-`OnionMessegner` reply path selection logic.

In order to do so and avoid users confusing the new
`MessageSendInstructions` for `ResponseInstruction`, we leave
`ResponseInstruction` as a now-unconstructible struct which wraps a
`MessageSendInstructions`.

Remove message type bound on `ResponseInstruction`

Our onion message handlers generally have one or more methods which
return a `ResponseInstruction` parameterized with the expected
message type (enum) of the message handler.

Sadly, that restriction is impossible to represent in our bindings -
our bindings concretize all LDK structs, enums, and traits into a
single concrete instance with generics set to our concrete trait
instances (which hold a jump table). This prevents us from having
multiple instances of `ResponseInstruction` structs for different
message types.

Our bindings do, however, support different parameterizations of
standard enums, including `Option`s and tuples.

In order to support bindings for the onion message handlers, we
are thus forced into std types bound by expected message types,
which we do here by making `ResponseInstruction` contain only the
instructions and generally using it as a two-tuple of
`(message, ResponseInstruction)`.

Merge pull request #3193 from tnull/2024-07-2995-followups

#2995 followups

Merge pull request #3265 from TheBlueMatt/2024-08-globally-unique-names

Disambiguate blinded path `ForwardNode`s between payment + message

Merge pull request #3250 from TheBlueMatt/2024-08-feature-cleanup

Correct `ConnectionNeeded` docs

Avoid allocating `Vec` for a single result

Fix permissive test

Merge pull request #3260 from TheBlueMatt/2024-08-more-deref

Bound `Persister`'s `WriteableScore` by `Deref`

Disambiguate blinded path `ForwardNode`s between payment + message

We currently have two structs with identical names in our public
API - `blinded_path::message::ForwardNode` and
`blinded_path::payment::ForwardNode`. This makes the API somewhat
awkward to use - users have to try (and fail) to import
`ForwardNode` twice only to then have to change their imports.

More importantly, however, this makes the API very hard to use in
some bindings languages where rename-imports or module imports
aren't available.

Thus, here, we rename both to give them context.

Remove uneccessary `std` bounds on many tests

We never actually build with `#![no_std]` in tests as Rust does
not support it. Thus, many tests have spurious `std` feature gates
when they run just fine without them. Here we remove those gates,
though note that tests that depend on behavior elsewhere in the
codebase which is `std`-gated obviously need to retain their
feature gates.

Reduce `feature = std` flags in `gossip.rs` somewhat

Fewer feature flags makes for more readable code, so we opt for
that over very marginally more effecient code here.

Drop `std::Error` impl for `PeerHandleError`

Not sure why we ever really had this, no one really ever bounds
anything on `std::Error` and its kinda a dead type, so there's no
need for us to `impl` it for our types.

Simplify `Instant` mocking in outbound payments

To handle `std` and `no-std` concepts of time in scoring, we'd
originally written a generic `Time` trait which we could use to
fetch the current time, observe real (not wall-clock) elapsed time,
and serialize the time.

Eventually, scoring stopped using this `Time` trait but outbound
payment retry expiry started using it instead to mock time in
tests.

Since scoring no longer uses the full features which required the
`Time` trait, we can substantially simplify by just having the
mocking option.

Remove note about `std`/`no-std` scorer serialization compat

In 81389dee306d960a8030bec5ffa304004148ce85 we removed a note about
mixing the `std` and `no-std` feature when de/serializing
`ProbabilisticScorer`s but forgot to note that there was a second
copy of that note in the module documentation.

This removes that note.

Bound `Persister`'s `WriteableScore` by `Deref`

This makes the trait marginally more flexible, but more importantly
matches our normal structure which makes the bindings generator a
bit happier.

Rename construct_keys_callback functions

Require Destination in construct_keys_callback

Use a single iterator to construct a BlindedPath

Instead of using separate iterators for pubkeys and TLVs, use an
iterator that yields a tuple of them. This allows for holding a mutable
reference to the TLVs such that they can be padded. With two iterators,
the pubkey iterator would have a reference to the ForwardNode slice when
constructing a payment path. However, this would prevent holding mutable
references in the TLVs iterator.

Generalize build_keys_helper

When constructing a blinded path, two iterators are used: one for the
pubkeys and another for Writeable TLVs. The first iterator is used in
the build_keys_helper utility function while the second is used inside
of a callback. Update this utility to work on any type that can be
borrowed as a PublicKey. This allows for using a single iterator of
tuples, which is necessary for padding the hops without additional
allocations and clones.

Add a construct_keys_callback for blinded paths

When constructing a BlindedPath, an Option<Destination> parameter to
construct_keys_callback is never passed. Make a separate utility
function that doesn't take this parameter. This allows for using the
build_keys_helper macro with a Iterator yielding items other than
PublicKey.

Use PublicKey values in construct_keys_callback

Instead of accepting iterators yielding PublicKey by reference in
utils::construct_keys_callback, take iterators yielding values since
these implement Copy and need to be copied anyway. This will help avoid
a situation when padding where both a reference and mutable reference
are needed.

Refactor helper macro from construct_keys_callback

When constructing a BlindedPath, utils::construct_blinded_hops uses two
iterators. However, this makes it difficult to pad blinded hops to equal
sizes without allocating a vector or cloning data. Refactor the
construct_keys_callback utility function so that is can be used with an
Iterator with different Item types. This allows using a single Iterator
of tuples while still supporting its use only with pubkeys.

Merge pull request #3247 from dunxen/2024-08-deprecate-balancemsat

Deprecate AvailableBalances::balance_msat

Merge pull request #3258 from TheBlueMatt/2024-08-default-for-bindings

Consistently pass `DefaultRouter::new` `Default` scoring parameters

Doc Clarity: Handling gaps in persisted ChannelMonitorUpdates

If there are any gaps in the persisted [`ChannelMonitorUpdate`]s,
implementer can safely ignore [`ChannelMonitorUpdate`]s after the gap and load without them.
Since, acc. to channel-manager, we have only made progress if all contiguos
ChannelMonitorUpdates have been persisted.

Test ChannelDetails serialization to catch mutants

Merge pull request #3254 from TheBlueMatt/2024-08-flaky-test

Correct `peer_handler::test_process_events_multithreaded`

Deprecate AvailableBalances::balance_msat

The ChannelMonitor::get_claimable_balances method provides a more
straightforward approach to the balance of a channel, which satisfies
most use cases. The computation of AvailableBalances::balance_msat is
complex and originally had a different purpose that is not applicable
anymore. We deprecate AvailableBalances::balance_msat now and will remove
it in a following release.

Co-authored-by: Willem Van Lint <noreply@wvanlint.dev>

Consistently pass `DefaultRouter::new` `Default` scoring parameters

In 26c1639ab69d6780c97a118f09e42cb42304088a (and later in
50c55dcf32466e3ccf17acea50697fc664950deb and
fb693ecbf81be0146a81cc7fd95342c403eacaa9) we switched to using
`Default::default()` to initialize `()` for scoring parameters in
tests. One `()`s slipped back in recently, which we replace here.

Merge pull request #3255 from TheBlueMatt/2024-08-124-beta

Bump version numbers to 0.0.124-beta/invoice 0.32-beta

Merge pull request #3253 from TheBlueMatt/2024-08-new-types-export

Re-export `lightning_types` in top-level `lightning` modules

Bump version numbers to 0.0.124-beta/invoice 0.32-beta

Merge pull request #3245 from valentinewallace/2024-07-blinded-pay-path-refactor

Move `BlindedPayInfo` into `BlindedPaymentPath`

Correct `peer_handler::test_process_events_multithreaded`

This test was added some time ago in
0c034e9a82e4339fb32af9da63832ac2a64abb0b, but never made any sense.
`PeerManager::process_events` will go around its loop as many times
is required to ensure we've always processed all events which were
pending prior to a `process_events` call, so having a test that
checks that we never go around more than twice is obviously broken.

And, indeed, in CI this tests fails with some regularity.

Instead, the test here is changed to ensure that we detectably go
around the loop again at least once.

Fixes #2385

Test blinded PaymentParameter round trip serialization.

Move BlindedPayInfo into blinded_path::payment module.

Move BlindedPayInfo into BlindedPaymentPath.

Also removes the implementation of Writeable for BlindedPaymentPath, to ensure
callsites are explicit about what they're writing.

Merge pull request #3204 from valentinewallace/2024-07-rb-test-vectors

Implement route blinding test vectors

Merge pull request #3188 from TheBlueMatt/2023-12-cache-scoring-points

Better Encapsulate Scoring and Cache Point Count

Merge pull request #3249 from TheBlueMatt/2024-08-feature-cleanup-1

bitcoin 0.32 upgrade followups

Consolidate sign_closing_transaction callsites to get_closing_signed_msg

Merge pull request #3222 from tnull/2024-08-rustfmt-lightning-persister

`rustfmt`: Run on `lightning-persister`

Avoid && in public API for onion peeling/decoding.

No reason to take a reference to a Deref.

Implement route blinding spec test vectors.

Fix blinded hop feature serialization.

We were writing a length redundantly...

Support next_blinding_override in blinded payment paths.

This allow us to forward blinded payments where the blinded path that we are
forwarding within was concatenated to another blinded path that starts at the
next hop.

Also allows constructing blinded paths using this override.

Remove the `std` feature implications from the `lightning` crate

Now that we don't have to have everything in our entire ecosystem
use the same `std`/`no-std` feature combinations we should start by
untangling our own features a bit.

This takes one last step, removing the implications of the `std`
feature from the `lightning` crate.

Drop the `_test_vectors` feature in favor of a `cfg` flag

This exists just for tests, so there's no reason for it to be
publicly visible.

Somewhat clean up `ci-tests.sh` and sort by `RUSTFLAGS`

Drop the `no-std` feature from `lightning-rapid-gossip-sync`

Now that we don't have to have everything in our entire ecosystem
use the same `std`/`no-std` feature combinations we should start by
untangling our own features a bit.

This takes another step by removing the `no-std` feature entirely
from the `lightning-rapid-gossip-sync` crate and removing all
feature implications on dependencies from the remaining `std`
feature.

Drop the `no-std` feature from `lightning-invoice`

Now that we don't have to have everything in our entire ecosystem
use the same `std`/`no-std` feature combinations we should start by
untangling our own features a bit.

This takes another step by removing the `no-std` feature entirely
from the `lightning-invoice` crate and removing all feature
implications on dependencies from the remaining `std` feature.

Drop the `no-std` feature from BP and drop feature implications

Now that we don't have to have everything in our entire ecosystem
use the same `std`/`no-std` feature combinations we should start by
untangling our own features a bit.

This takes the first step by removing the `no-std` feature entirely
from the `lightning-background-processor` crate and removing most
feature implications on dependencies from the remaining `std`
feature.

It also addresses a CI oversight where we were not testing
`lightning-background-processor` without the `std` feature in CI at
all.

Ensure we always pass a `path` for in-workspace dependencies

In order to ensure our crates depend on the workspace copies of
each other in test builds we need to override the crates.io
dependency with a local `path`.

We can do this in one of two ways - either specify the `path` in
the dependency listing in each crate's `Cargo.toml` or use the
workspace `Cargo.toml` to `patch` all dependencies. The first is
tedious while the second lets us have it all in one place. However,
the second option does break `cargo *` in individual crate
directories (forcing the use of `cargo -p crate *` instead) and
makes it rather difficult to depend on local versions of workspace
crates.

Thus, here we drop the `patch.crates-io` from our top-level
`Cargo.toml` entirely.

Still, we do update the `ci/ci-tests.sh` script here to use
`cargo -p crate` instead of switching to each crate's directory as
it allows `cargo` to use a shared `target` and may speed up tests.

Optimize `BufReader` somewhat for the only used case

`rust-bitcoin` doesn't ever actually *use* its `BufRead`
requirement when deserializing objects, and forcing it is somewhat
inefficient, so we optimize the only (actual) case here by passing
reads straight through to the backing stream.

Fix bug in `BufReader::fill_buf` when reaching EOF

When we reach EOF we may return a full buffer when we should return
an empty one.

Merge pull request #3240 from tnull/2024-08-add-ci-flag-for-bitcoind

Disallow skipping tx-sync tests in CI

Re-export `lightning_types` in top-level `lightning` modules

Since we now have many types in one place, it makes sense to export
them in that place. Further, doing so finally somewhat starts to
reduce our `lightning::ln` module size, which historically is the
dumping ground for everything when most things really should be
top-level modules in `lightning`.

Here we take a step in the right direction by exporting
`lightning_types` as `lightning::types` and encouraging users to
use those paths directly rather than the ones in `lightning::ln`.