Merge pull request #3302 from TheBlueMatt/2024-09-atomic-cleanups

Simplify and fix AtomicCounter

Merge pull request #3010 from shaavan/issue2836

Introduce Retry InvoiceRequest Flow

Rename `AtomicCounter::get_increment` to simply `next`

Its a counter, `next` is super clear, `get_increment` is a bit
less so.

Simplify and fix `AtomicCounter`

`AtomicCounter` was slightly race-y on 32-bit platforms because it
increments the high `AtomicUsize` independently from the low
`AtomicUsize`, leading to a potential race where another thread
could observe the low increment but not the high increment and see
a value of 0 twice.

This isn't a big deal because (a) most platforms are 64-bit these
days, (b) 32-bit platforms aren't super likely to have their
counter overflow 32 bits anyway, and (c) the two writes are
back-to-back so having another thread read during that window is
very unlikely.

However, we can also optimize the counter somewhat by using the
`target_has_atomic = "64"` cfg flag, which we do here, allowing us
to use `AtomicU64` even on 32-bit platforms where 64-bit atomics
are available.

This changes some test behavior slightly, which requires
adaptation.

Fixes #3000

Introduce handle_message_received test

- Add a test to verify the functionality of the handle_message_received
  function.
- Ensure the test covers scenarios where InvoiceRequest messages are retried
  for PendingOutboundPayments after a simulated connection loss.

Introduce message_received in ChannelMessageHandler

- Introduce the `message_received` function to manage the
  behavior when a message is received from any peer.
- This function is used within `ChannelManager` to retry `InvoiceRequest`
  messages if we haven't received the corresponding invoice yet.
- This change makes the offer communication robust against sudden
  connection drops where the initial attempt to send the message
  might have failed.

Introduce enqueue_invoice_request Function

1. Separate the logic of forming `invoice_request` messages from
   `invoice_request` and `reply_paths` and enqueueing them into a
   separate function.
2. This logic will be reused in the following commit when reforming
   `invoice_request` messages for retrying.

Introduce RetryableInvoiceRequest in AwaitingInvoice

1. To enable the retry of the Invoice Request message, it's necessary
   to store the essential data required to recreate the message.
2. A new struct is introduced to manage this data, ensuring the
   InvoiceRequest message can be reliably recreated for retries.
3. The addition of an `awaiting_invoice` flag allows tracking of
   retryable invoice requests, preventing the need to lock the
   `pending_outbound_payment` mutex.

Merge pull request #3291 from tnull/2024-09-add-their-node-id-routing-msg-handler

Add node id to remaining `RoutingMessageHandler::handle_` methods

Merge pull request #3163 from shaavan/invoice_reply_path

Introduce Reply Paths for BOLT12 Invoice in Offers Flow.

Don't rely on route-fetching rand in `blinded_path_with_custom_tlv`

`blinded_path_with_custom_tlv` indirectly relied on route CLTV
randomization when sending because nodes were at substantially
different block heights after setup. Instead we make sure all nodes
are at the same height which makes the test more robust.

Drop `Debug` on `InMemorySigner` (and `EntropySource`)

`InMemorySigner` has various private keys in it which makes
`Debug` either useless or dangerous (because most keys won't log
anything, but if they did we'd risk logging private key material).

Refactor: Take `their_node_id` by value across all handler interfaces

In order to maintain interface consistency, we refactor all message
handler interfaces to take `PublicKey` rather than `&PublicKey`, as the
difference in efficiency should be negigible and the former is easier to
handle in binding languages.

Over time, we also want to move (no pun intended) towards all messaging
interfaces using move semantics, so dropping the reference for
`PublicKey` is the first step in this direction.

Add node id to remaining `RoutingMessageHandler::handle_` methods

Previously, some `RoutingMessageHandler::handle_` methods (in particular
the ones handling node and channel announcements, as well as channel
updates, omitted the `their_node_id` argument. This didn't allow
implementors to discern *who* sent a particular method.

Here, we add `their_node_id: Option<&PublicKey>` to have them learn who
sent a message, and set `None` if our own node it the originator of a
broadcast operation.

Update Offers Test to Verify BOLT12 Invoice Reply Paths

1. Updated the Offers Test to check the reply paths in BOLT12 Invoices.
2. Changed the `extract_invoice` return type from `Option<BlindedMessagePath>`
   to `BlindedMessagePath` since all BOLT12Invoices now have a corresponding
   reply path by default.

Add reply_path to BOLT12Invoices in Offers Flow

1. Introduced reply_path in BOLT12Invoices to address a gap in error handling.
   Previously, if a BOLT12Invoice sent in the offers flow generated an Invoice Error,
   the payer had no way to send this error back to the payee.
2. By adding a reply_path to the Invoice Message, the payer can now communicate
   any errors back to the payee, ensuring better error handling and communication
   within the offers flow.

Add HMAC, and nonce to OffersContext::InboundPayment

Introduce HMAC and nonce calculation when sending Invoice with
reply path, so that if we receive InvoiceError back for the
corresponding Invoice we can verify the payment hash before logging it.

Merge pull request #3309 from TheBlueMatt/2024-09-always-print-why

Always print why we disconnect explicitly

Merge pull request #3305 from TheBlueMatt/2024-09-no-redundant-gossip-validation

Avoid redundant `{channel,node}_announcement` signature checks

Always print why we disconnect explicitly

...and drop the old, confusing, "protocol error" message when we
get a duplicate connection from a peer.

This should slightly further disambiguate disconnection reasons,
which are a key debugging tool for assigning blame.

Merge pull request #3306 from TheBlueMatt/2024-09-chan-id-hex

Write ChannelIds out as hex in Debug output

Introduce Verification trait.

- The trait defines the public method one may define for creating and
  verifying the HMAC.
- Using a pub trait to define these method allows the flexibility for
  other `OffersMessageHandler` construct to construct the HMAC and
authenticate the message.

Merge pull request #3296 from alecchendev/2024-09-async-close-sign-followup

Finish closing channel after async closing signed

Avoid redundant `{channel,node}_announcement` signature checks

If we receive `{channel,node}_announcement` messages which we
already have, we first validate their signatures and then look in
our graph and discover that we should discard the messages. This
avoids a second lock in `node_announcement` handling but does not
impact our locking in `channel_announcement` handling. It also
avoids lock contention in cases where the signatures are invalid,
but that should be exceedingly rare.

For nodes with relatively few peers, this is a fine state to be in,
however for nodes with many peers, we may see the same messages
hundreds of times. This causes a rather substantial waste of CPU
resources validating gossip messages.

Instead, here, we change to checking our network graph first and
then validate the signatures only if we don't already have the
message.

Finish closing channel after async closing signed

In addressing a followup to test reconnection during closing negotation
with async signing, we change things to only return a `ShutdownResult`
when we actually finish shutting down the channel, i.e. we have the
signature ready to send the final closing signed. This slightly
simplifies the logic where we would shutdown our channel
prematurely before we got the final signature. This also means
that we don't push multiple `ChannelClosed` events if we receive closing
signed, reconnect, and receive closing signed again.

Clean up imports in `lightning-types`

To remove the now-redundant `hex_conservative` explicit dependency.

Write `ChannelId`s out as hex in `Debug` output

`ChannelId`s are almost always referenced as hex, so having debug
output print the raw bytes is somewhat annoying. Instead, we should
dump them as hex the same way we do for `Display`.

This uses the `hex_conservative` `impl_fmt_macros` which does all
the work for us, like we use for `lightning_types`.

impl `Borrow<[u8]>` for `ChannelId`

We do this for `Payment*` in `lightning-types` and its needed for
the `hex_conservaitve` `impl_fmt_traits` macro which we'll use in
the next commit.

Merge pull request #3304 from TheBlueMatt/2024-09-tokio-util-msrv

Fix MSRV builds of `lightning-transaction-sync` in CI

Reduce features on `lightning-transaction-sync`'s `tokio` dev-dep

This should marginally reduce the chance of MSRV or other issues in
the future, and is just good practice.

Test `lightning-transaction-sync` in our no-dev-deps MSRV test

Now that `lightning-transaction-sync` is in the workspace and
covered by our MSRV we should be testing it in our
`msrv-no-dev-deps-check` test crate.

Fix MSRV test of `lightning-transaction-sync` with `esplora-async`

`lightning-transaction-sync`'s `esplora-async` dependency
indirectly depends on `tokio-util`, which, like tokio, recently
bumped its MSRV.

Here we update `ci/ci-tests.sh` to pin `tokio-util` to make MSRV
builds pass.

HMAC Construction and Verification for PaymentHash

When a InvoiceError is received for a sent BOLT12Invoice, the
corresponding PaymentHash is to be logged. Introduce hmac construction
and verification function for PaymentHash for this purpose.

Merge pull request #3290 from tnull/2024-09-add-tx-sync-copyright

Add copyright notices to `lightning-transaction-sync` modules

Add copyright notices to `lightning-transaction-sync` modules

Merge pull request #3278 from TheBlueMatt/2024-08-124-relnotes

Add an 0.0.124 CHANGELOG entry

Update versions to 0.0.124 and invoice 0.32 types 0.1 release

Add an 0.0.124 CHANGELOG entry

Merge pull request #3285 from TheBlueMatt/2024-08-tx-too-small

Correct handling of added `OP_RETURN` outputs

Merge pull request #3284 from TheBlueMatt/2024-08-bindings-imports-suck

Correct `ANCHOR_INPUT_WITNESS_WEIGHT` constant

`ANCHOR_INPUT_WITNESS_WEIGHT` is too high by two weight units,
likely it was calculated to include the SegWit marker bytes, but
it is used to describe an `Input::satisfaction_weight`, which does
not expect the marker bytes.

This corrects that oversight, reducing the constant by two and
adding the marker bytes back in our own internal weight
calculations. It also fixes a second issue where the constant was
too low by one when `grind_signatures` is not set, as that may
result in a signature being one byte longer than we expect.

Add a test for the past few commits

This adds a single test for coin selection which exercises the
issues fixed in the past three commits.

Drop bogus debug assertion that we don't overpay on fees

We should always select at least as many coins as is required to
meet the feerate target, but its perfectly fine if we overshoot.
Specifically, we may overshoot deliberately if we choose to burn
change to fee instead.

Handle under-coin-selecting due to an added OP_RETURN output

When we do coin selection for channel close anchor spends, we may
do coin selection targeting exactly the input values we need.
However, if coin selection does not include a change output, we may
add an OP_RETURN output, which may cause us to end up with less
fee than we wanted on the resulting package.

Here we address this issue by running coin selection twice - first
without seeking the extra weight of the OP_RETURN output, and again
if we find that we under-selected.

Ensure we never try to broadcast a transaction <= 64 bytes

While these are consensus-valid, they have been nonstandard for
quite some time and will not relay nor confirm.

Specify imports manually in `types::features::sealed`

There's not a lot of reason to do this, but sadly the bindings
don't currently resolve wildcard imports at all, and I don't want
to deal with implementing it right now.

Merge pull request #3282 from TheBlueMatt/2024-08-fix-bench-logging

Dont output logs when benchmarking

Dont output logs when benchmarking

In 11ab302087622b546d116fc9679f601667d18c4d we accidentally removed
the `not(ldk_bench)` bound before outputting logs to stderr, which
we restore here. Instead of simply ignoring logs in benchmarks,
which we did previously, we instead format logs (in a way that LLVM
will not optimize out).

Remove log level filtering in tests

There's no reason why we should be filtering our logs during tests.
It seems mostly like the `TestLogger` was written more as a sample
logger than a test logger, but these days we have samples, so we
should just drop the filtering and show everything.

Merge pull request #3280 from TheBlueMatt/2024-08-124rc

Bump versions to rc1

Merge pull request #3279 from TheBlueMatt/2024-08-no-pub-usize

Use `u64` for `required_unknown_bits_from` indexes, not `usize`

Bump versions to rc1

Merge pull request #3257 from tnull/2024-08-fix-is-public

Rename instances of `is_public` to `is_announced`

Rename `announced_channel` to `is_announced_for_forwarding`

.. we rename the flag configuring whether we announce a channel or not.

Use `u64` for `required_unknown_bits_from` indexes, not `usize`

While `usize` should be fine, we're multiplying the index by 8 so
if we have a jumbo feature bit fitting in a 32-bit size type may
not quite work. More importantly, this would be the first use of a
`usize` in the public API and dealing with it in bindings is
annoying so we just replace with a `u64`.

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