Don't trigger manager persistence on unexpected release_htlc message.
If someone sends us an unexpected or duplicate release_held_htlc onion message,
we should simply ignore it and not persist the entire ChannelManager in
response.
Support abandoning pending outbound async payments.
Async payments may have very high expires because we may be waiting for days
for the recipient to come online, so it's important that users be able to
abandon these payments early if needed.
Support sending async payments as an always-online sender.
Async receive is not yet supported.
Here we process inbound release_htlc onion messages, check that they actually
correspond to one of our outbound payments, and actually forward the HTLCs.
Valid release_htlc receipt indicates that the recipient has now come online to
receive.
Because we may receive a static invoice to pay days before the recipient
actually comes back online to receive the payment, it's good to do as many
checks as we can up-front. Here we ensure that the blinded paths provided
in the invoice won't cause us to exceed the maximum onion packet size.
Support initiating an async payment to a static invoice.
Supported when the sender is an always-online node. Here we send the initial
held_htlc_available onion message upon receipt of a static invoice, next we'll
need to actually send HTLCs upon getting a response to said OM.
Store async payment data in PendingOutboundPayment.
Adds a pending outbound payment variant for async payments, which indicates
that we have received a static invoice to pay and have generated a keysend preimage
for the eventual payment. When the recipient comes back online, we'll
transition from this new state to Retryable and actually forward the HTLCs.
Support checking that a static invoice matches an outbound invreq.
Useful for ensuring that an inbound static invoice matches one of our outbound
invreqs, otherwise it is an unexpected invoice and should be ignored and not
paid.
Upcoming commits will support sending and receiving held_htlc_available and
release_held_htlc messages. These messages need to be enqueued so that they can
be released in ChannelManager's implementation of AsyncPaymentsMessageHandler
to OnionMessenger for sending.
Pass context into held_htlc_available message handling.
Useful for using the payment_id within to look up the corresponding outbound
async payment so we know we can safely release the HTLCs to the now-onlinen
recipient.
This context will be used in reply paths for outbound held_htlc_available
messages, so we can authenticate the corresponding release_held_htlc messages.
- 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.
shaavan [Mon, 10 Jun 2024 11:46:08 +0000 (17:16 +0530)]
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.
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.
shaavan [Mon, 10 Jun 2024 11:52:09 +0000 (17:22 +0530)]
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.
Elias Rohrer [Wed, 11 Sep 2024 14:06:44 +0000 (16:06 +0200)]
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.
Elias Rohrer [Wed, 4 Sep 2024 13:39:20 +0000 (15:39 +0200)]
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.
shaavan [Thu, 29 Aug 2024 11:36:18 +0000 (17:06 +0530)]
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.
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.
- 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.
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.
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.
Matt Corallo [Mon, 9 Sep 2024 15:30:54 +0000 (15:30 +0000)]
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`.
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.
Matt Corallo [Tue, 3 Sep 2024 15:09:32 +0000 (15:09 +0000)]
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.
Matt Corallo [Thu, 29 Aug 2024 22:47:32 +0000 (22:47 +0000)]
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.
Matt Corallo [Thu, 29 Aug 2024 20:43:10 +0000 (20:43 +0000)]
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.
Matt Corallo [Sun, 1 Sep 2024 01:08:38 +0000 (01:08 +0000)]
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.
Matt Corallo [Fri, 30 Aug 2024 17:38:37 +0000 (17:38 +0000)]
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).
Matt Corallo [Mon, 19 Aug 2024 21:07:46 +0000 (21:07 +0000)]
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.
Matt Corallo [Thu, 29 Aug 2024 16:45:27 +0000 (16:45 +0000)]
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`.
Arik Sosman [Wed, 28 Aug 2024 16:33:14 +0000 (09:33 -0700)]
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.
Arik Sosman [Wed, 28 Aug 2024 16:32:35 +0000 (09:32 -0700)]
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.
Matt Corallo [Wed, 28 Aug 2024 14:35:54 +0000 (14:35 +0000)]
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.
Matt Corallo [Tue, 27 Aug 2024 16:47:57 +0000 (16:47 +0000)]
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.
Matt Corallo [Tue, 27 Aug 2024 16:44:58 +0000 (16:44 +0000)]
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.
Matt Corallo [Tue, 27 Aug 2024 19:17:06 +0000 (19:17 +0000)]
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.
Vincenzo Palazzo [Tue, 27 Aug 2024 08:18:28 +0000 (10:18 +0200)]
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.
Matt Corallo [Thu, 22 Aug 2024 19:15:34 +0000 (19:15 +0000)]
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.
Matt Corallo [Thu, 22 Aug 2024 15:25:56 +0000 (15:25 +0000)]
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.
Matt Corallo [Mon, 26 Aug 2024 19:25:46 +0000 (19:25 +0000)]
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`.