Merge pull request #2971 from jbesraa/review-club/2815

Fix comparison in `get_dust_buffer_feerate`

Fix `cmp::max` execution in `ChannelContext::get_dust_buffer_feerate`

  The current `cmp::max` doesnt align with the function comment, ie its
  comparing 2530 and `feerate_plus_quarter` instead of `feerate_per_kw
  + 2530` and `feerate_plus_quarter` which is fixed in this commit

Fix race between handshake_complete and timer_tick_occurred

The initial noise handshake on connection establishment must complete
within a single timer tick. This timeout is enforced via
`awaiting_pong_timer_tick_intervals` whenever a timer tick fires while
our handshake has yet to complete. Currently, on an inbound connection,
if a timer tick fires after we've sent act two of the noise handshake
along with our init message and before receiving the counterparty's init
message, we begin enforcing such timeout. Even if we immediately
continue to process the counterparty's init message to complete to
handshake, the timeout enforcement is not cleared. With the handshake
complete, `awaiting_pong_timer_tick_intervals` is now tracked to enforce
a pong timeout, except a ping was never actually sent. If a single timer
tick fires again without having received a message from the peer, or
enough timer ticks fire to trigger the
`MAX_BUFFER_DRAIN_TICK_INTERVALS_PER_PEER` logic, we'll end up
disconnecting the peer due to a timeout for a pong we'll never receive.

We fix this by always resetting `awaiting_pong_timer_tick_intervals`
upon processing our counterparty's init message.

Log pending in-flight updates when we are missing a monitor

If we are missing a `ChannelMonitor` for which the `ChannelManager`
has pending updates, it may be useful to print what the updates we
have actually are, at least useful in identifying the bug(s).

Add BlindedPath::introduction_node_id method

Blinded paths use a pubkey to identify the introduction node, but it
will soon allow using a directed short channel id instead. Add an
introduction_node_id method to BlindedPath to facilitate lookup in the
latter case.

Add IndexedMap::get_key_value

Useful for applying Option::map when needing both the key and value for
when needing a reference to the key with a longer lifetime.

Generalize onion message ForwardTlvs::next_node_id

Allow either using a node id or a short channel id when forwarding an
onion message. This allows for a more compact representation of blinded
paths, which is advantageous for reducing offer QR code size.

Follow-up commits will implement handling the short channel id case as
it requires looking up the destination node id.

Merge pull request #2910 from tnull/2024-02-rustfmt-sign-mod

`rustfmt`:  Format `sign` module

Add test coverage for cc78b77c715d6ef62693d4c1bc7190da990ec0fa

cc78b77c715d6ef62693d4c1bc7190da990ec0fa fixed an important
downgrade bug, but neglected to add test coverage. Here we recitfy
that by adding a few simple tests of common cases.

Tests heavily tweaked by Matt Corallo <git@bluematt.me>.

Remove `no-std-check/src/lib.rs` from `rustfmt` exclusion list

.. now continously checking its formatting in CI.

Remove `msrv-no-dev-deps-check/src/lib.rs` from `rustfmt` exclusion list

.. now continously checking its formatting in CI.

Run `rustfmt` on `msrv-no-dev-deps-check/src/lib.rs`

Remove `sign/type_resolver.rs` from `rustfmt` exclusion list

.. now continously checking its formatting in CI.

Run `rustfmt` on `sign/type_resolver.rs`

Remove `sign/taproot.rs` from `rustfmt` exclusion list

.. now continously checking its formatting in CI.

Run `rustfmt` on `sign/taproot.rs`

Remove `sign/mod.rs` from `rustfmt` exclusion list

.. now continously checking its formatting in CI.

Run `rustfmt` on `sign/mod.rs`

Move `input_idx` retrieval into closure

Move `htlc_basepoint` to variable

Move `hash_to_message` to variable

Move low-R comments into blocks

Pull anchor check into helper function

Remove `sign/ecdsa.rs` from `rustfmt` exclusion list

.. now continously checking its formatting in CI.

Run `rustfmt` on `sing/ecdsa.rs`

Merge pull request #2969 from TheBlueMatt/2024-03-fix-upgradable-enum

Allow `MaybeReadable` to not fully read in `upgradable_option`

Whils this is generally not supported, issues in our
`MaybeReadable` implementations may occur, and we should try to be
robust against them.

Ensure we read the full TLV stream length when maybe-reading `None`

If we are reading an object that is `MaybeReadable` in a TLV stream
using `upgradable_required`, it may return early with `Ok(None)`.
In this case, it will not read any further TLVs from the TLV
stream. This is fine, except that we generally expect
`MaybeReadable` always consume the correct number of bytes for the
full object, even if it doesn't understand it.

This could pose a problem, for example, in cases where we're
reading a TLV-stream `MaybeReadable` object inside another
TLV-stream object. In that case, the `MaybeReadable` object may
return `Ok(None)` and not consume all the available bytes, causing
the outer TLV read to fail as the TLV length does not match.

Fix unknown handling in `impl_writeable_tlv_based_enum_upgradable`

`impl_writeable_tlv_based_enum_upgradable` professed to supporting
upgrades by returning `None` from `MaybeReadable` when unknown
variants written by newer versions of LDK were read. However, it
generally didn't support this as it didn't discard bytes for
unknown types, resulting in corrupt reading.

This is fixed here for enum variants written as a TLV stream,
however we don't have a length prefix for tuple enum variants, so
the documentation on the macro is updated to mention that
downgrades are not supported for tuple variants.

Merge pull request #2704 from jkczyz/2023-11-channelmanager-docs

`ChannelManager` documentation refresh

Add BOLT 12 Refunds section to ChannelManager docs

Add BOLT 12 Offers section to ChannelManager docs

Add a payments section to ChannelManager docs

Add a channels section to ChannelManager docs

Add an operations section to ChannelManager docs

ChannelManager initialization docs with example

Merge pull request #2954 from TheBlueMatt/2024-03-test-ci-beta-fail

Fix new warnings causing CI build failures on rustc beta

Merge pull request #2967 from tnull/2024-03-refactor-drop-handle-message

Split `PeerManager::handle_message` to avoid explicit `mem::drop`

Fix unused warning for un-accessed enum variant field in net-tokio

Remove a handful of redundant imports

... that newer rustc now warns about.

Mark several test_utils which are only used in test as `cfg(test)`

Drop now-unused methods on `Time`

We no longer use `Time` during scoring, which makes several of its
methods now useless. We remove those here.

Use `crate::prelude::*` rather than specific imports

New rustc beta now warns on duplicate imports when one of the
imports is from a wildcard import or the default prelude. Thus, to
avoid this here we prefer to always use `crate::prelude::*` and let
it decide if we actually need to import anything.

Add more `std` prelude items to `crate::prelude`

New rustc beta now warns on duplicate imports when one of the
imports is from a wildcard import or the default prelude. Thus, for
simplicity, we need to make our `crate::prelude` mostly identical
to the `std` one, allowing us to always simply use the
`crate::prelude` and let it decide if we need to import anything.

Allow(unused_imports) on prelude imports

New rustc now warns on duplicate imports when one of the imports
is from a wildcard import or the default prelude. Thus, because we
often don't actually use the imports from our prelude (as they
exist to duplicate the `std` default prelude), we have to mark most
of our `crate::prelude` imports with `#[allow(unused_imports)]`,
which we do here.

Debug more information when we fail to find a lock call symbol

Fix new warnings causing CI build failures on rustc beta

Split `PeerManager::handle_message` to avoid explicit `mem::drop`

Previously, `handle_message` was a single large method consisting of two
logical parts: one modifying the peer state hence requiring us to hold
the `peer_lock` `MutexGuard`, and, after calling `mem::drop(peer_lock)`,
the remainder which does not only *not* require to hold the
`MutexGuard`, but relies on it being dropped to avoid double-locking.

However, the `mem::drop` was easily overlooked, making reasoning about
lock orders etc. a headache. Here, we therefore have
`handle_message` call two sub-methods reflecting the two logical parts,
allowing us to avoid the explicit `mem::drop`, while at the same time
making it less error-prone due to the two methods' signatures.

Merge pull request #2731 from shaavan/issue2711

Delay broadcasting Channel Updates until connected to peers

Delay broadcasting Channel Updates until connected to peers

    - We might generate channel updates to be broadcast when
      we are not connected to any peers to broadcast them to.
    - This PR ensures to cache them and broadcast them only when
      we are connected to some peers.

Other Changes:
    1. Introduce a test.
    2. Update the relevant current tests affected by this change.
    3. Fix a typo.
    4. Introduce two functions in functional_utils that optionally
       connect and disconnect a dummy node during broadcast testing.

Merge pull request #2978 from wpaulino/reserve-async-signing-channel-tlvs

Reserve async signing related channel TLV types

Reserve async signing related channel TLV types

A LDK user deployed to production a WIP version of the async signing
branch in which two new TLVs were added to channel. To prevent them from
needing to perform a migration, we can just new types for TLVs that have
yet to be included in a release. A note has been added to ensure types
45 and 47 are not used for another purpose.

Merge pull request #2975 from valentinewallace/2024-03-blinded-path-custom-tlvs

Merge pull request #2845 from wpaulino/decode-htlc-onion-when-committed

Support decoding HTLC onions once fully committed

Test sending + receiving custom TLVs to blinded paths.

Merge pull request #2974 from benthecarman/dang-value

Add DecodeError::DangerousValue for decoding invalid channel managers

Add DecodeError::DangerousValue for decoding invalid channel managers

This would help distinguish different types of errors when deserialzing
a channel manager. InvalidValue was used previously but this could be
because it is an old serialization format, whereas DangerousValue is a
lot more clear on why the deserialization failed.

Contrast ChannelManager and ChannelMonitor in docs

Re-write ChannelManager summary documentation

Merge pull request #2976 from wvanlint/simplify_trait_object_impl

Simplify implementation for KVStore trait objects

Simplify implementation for KVStore trait objects

The implementation of the Persist and Persister trait for KVStore types
can also be used for trait objects if there is no implicit bound of
`Sized`.

Break ChannelManager docs into sections

ChannelManager docs aren't very approachable as they consist of a large
wall of texts without much direction. As a first step of improvement,
add sections to help delineate the existing text and make it easier to
scan.

Decode update_add_htlc onions before forwarding HTLCs

This commit completes all of the groundwork necessary to decode incoming
`update_add_htlc` onions once they're fully committed to by both sides.
HTLCs are tracked in batches per-channel on the channel they were
received on. While this path is unreachable for now, until
`InboundHTLCResolution::Resolved` is replaced with
`InboundHTLCResolution::Pending`, it will allow us to obtain
`HTLCHandlingFailed` events for _any_ failed HTLC that comes across a
channel.

Refactor forward_htlcs to return whether to push a forward event

When decoding pending `update_add_htlc` onions, we may need to forward
HTLCs using `ChannelManager::forward_htlcs`. This may end up queueing a
`PendingHTLCsForwardable` event, but we're only decoding these pending
onions as a result of handling a `PendingHTLCsForwardable`, so we
shouldn't have to queue another one and wait for it to be handled. By
having a `forward_htlcs` variant that does not push the forward event,
we can ignore the forward event push when forwarding HTLCs which we just
decoded the onion for.

Consider pending decode_update_add_htlcs when pushing forward event

Since decoding pending `update_add_htlc` onions will go through the HTLC
forwarding path, we'll want to make sure we don't queue more events than
necessary if we have both HTLCs to forward/fail and pending
`update_add_htlc` onions to decode.

Refactor incoming HTLC accept checks out from Channel::update_add_htlc

In the future, we plan to completely remove
`decode_update_add_htlc_onion` and replace it with a batched variant.
This refactor, while improving readability in its current form, does not
feature any functional changes and allows us to reuse the incoming HTLC
acceptance checks in the batched variant.

Only include channel_update in failure if needed by error code

This simplifies the failure path by allowing us to return the general
error code for a failure, which we can then amend based on whether it
was for a phantom forward.

Refactor outgoing channel lookup out from decode_update_add_htlc_onion

In the future, we plan to complete remove `decode_update_add_htlc_onion`
and replace it with a batched variant. This refactor, while improving
readability in its current form, does not feature any functional changes
and allows us to reuse most of the logic in the batched variant.

Refactor outgoing HTLC checks out from decode_update_add_htlc_onion

In the future, we plan to complete remove `decode_update_add_htlc_onion`
and replace it with a batched variant. This refactor, while improving
readability in its current form, does not feature any functional changes
and allows us to reuse most of the logic in the batched variant.

Refactor HTLCFailureMsg generation out from decode_update_add_htlc_onion

In the future, we plan to complete remove `decode_update_add_htlc_onion`
and replace it with a batched variant. This refactor, while improving
readability in its current form, does not feature any functional changes
and allows us to reuse most of the logic in the batched variant.

Add is_intro_node_blinded_forward helper for onion_utils::Hop

Add new HTLCDestination variant for invalid onion

The existing variants do not cover such case as we previously never
surfaced `HTLCHandlingFailed` events for HTLCs that we failed back with
`UpdateFailMalformedHTLC` due to an invalid onion packet.

Track pending update_add_htlcs in ChannelManager for later processing

We plan to decode the onions of these `update_add_htlc`s as part of the
HTLC forwarding flow (i.e., `process_pending_htlc_forwards`), so we'll
need to track them per-channel at the `ChannelManager` level.

Remove unreachable handling of htlc_forwards upon channel_reestablish

`htlc_forwards` only returns a `Some` value from
`handle_channel_resumption` if we provide it a non-empty
`pending_forwards`. Since we don't, we'll never have a value to handle.

Track incoming UpdateAddHTLC until HTLC resolution

This commit serves as a stepping stone to moving towards resolving HTLCs
once the HTLC has been fully committed to by both sides.

Currently, we decode HTLC onions immediately upon receiving an
`update_add_htlc`. Doing so determines what we should do with the HTLC:
forward it, or immediately fail it back if it cannot be accepted. This
action is tracked until the HTLC is fully committed to by both sides,
and a new commitment in the latter case is proposed to fully remove the
HTLC. While this has worked so far, it has some minor privacy
implications, as forwarding/failing back do not go through the usual
`PendingHTLCsForwardable` flow. It also presents issues with the
quiescence handshake, as failures through this path do not go through
the holding cell abstraction, leading to a potential violation of the
handshake by sending an `update_fail_*` after already having sent
`stfu`.

Since `pending_inbound_htlcs` are written pre-TLVs, we introduce a new
serialization version in which we change the `PendingHTLCStatus`
serialization of
`InboundHTLC::AwaitingRemoteRevokeToRemove/AwaitingRemovedRemoteRevoke`
to be an option instead. We'll still write it as the current version
(`MIN_SERIALIZATION_VERSION`), but we'll support reading the new version
to allow users to downgrade back to this commit.

Support Vec serialization that include element length prefix

We add new macro alternatives to
impl_writeable_for_vec/impl_readable_for_vec that add a length prefix to
each element in the `Vec`. This is intended to be used over the existing
macros when attempting to serialize a `Vec` with elements of variable
lengths.

Don't consume readers with FixedLengthReader

We can't always assume that we're done reading after using a
FixedLengthReader. In some cases, we may need to read a set of
length-prefixed objects one at a time, and we'd like to do so without
incurring an additional allocation by reading a number of bytes first to
then deserialize them.

Support receiving custom TLVs to blinded paths.

Support sending custom TLVs to blinded recipients.

Merge pull request #2906 from arik-so/arik/trampoline/2024-02-trampoline-onion-construction-vectors

Trampoline onion construction vectors

Test Trampoline payload serialization.

Trampoline onion construction.

Rename TrampolineOnionPacket enum comment.

Merge pull request #2959 from tnull/2024-03-support-gossip-queries-in-ignoring-handler

Signal `GossipQuery` support when using `IgnoringMessagHandler`

Merge pull request #2963 from jkczyz/2024-03-a-channel-manager

Use `AChannelManager` in `BackgroundProcessor`

Use AChannelManager in BackgroundProcessor

Replace instance of ChannelManager in BackgroundProcessor and in
Persister with AChannelManager. This reduces the number of type
parameters need in those types, which would need to be repeated in an
async version of Persister.

Remove duplicate docs from trait implementations

Fix indentation in doc example

Merge pull request #2756 from arik-so/arik/trampoline/2023-11-outbound

Serialize Trampoline payloads in outbound onions.

Add Trampoline variant to OutboundOnionPayload.

Introduce TrampolineOnionPacket.

Merge pull request #2946 from tnull/2024-03-txsync-readd-reorged-output-spends

Tx-Sync: Track spent `WatchedOutput`s and re-add if unconfirmed

Merge pull request #2953 from dunxen/2024-03-fix2941

Fix `ChannelManager::accept_inbound_channel` error handling

Dedup `confirmed_txs` `Vec`

Previously, we would just push to the `confirmed_txs` `Vec`, leading to
redundant `Confirm::transactions_confirmed` calls, especially now that
we re-confirm previously disconnected spends.

Here, we ensure that we don't push additional `ConfirmedTx` entries if
already one with matching `Txid` is present. This not only gets rid of
the spurious `transactions_confirmed` calls (which are harmless), but
more importantly saves us from issuing unnecessary network calls, which
improves latency.

Merge pull request #2934 from TheBlueMatt/2023-03-no-num-traits

Replace the generic `parse_int_be` with a macro called twice

Signal `GossipQuery` support when using `IgnoringMessagHandler`

With its v24.02 release CLN made `GossipQueries` a required feature,
leading to a incompatibility between LDK and CLN when using
`IgnoringMessagHandler` as a `RoutingMessageHandler`, which is usually
the case when a node uses RGS.

To fix this issue, we let `IgnoringMessagHandler` signal `GossipQuery`
support, just to go ahead and ignore every gossip message the peer will
send us. While this is nonsensical and still might result in some
unnecessary bandwidth wasted, we have to do something to fix the
incompatibility.

Test ChannelManager::accept_inbound_channel errors

Fix ChannelManager::accept_inbound_channel error handling

Merge pull request #2935 from valentinewallace/2024-03-keysend-to-blinded

Support keysend to blinded paths

Merge pull request #2419 from jurvis/2023-03-interactivetxs

Interactive Transaction Construction