Wilmer Paulino [Wed, 24 Jan 2024 01:08:17 +0000 (17:08 -0800)]
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.
Wilmer Paulino [Wed, 24 Jan 2024 00:55:40 +0000 (16:55 -0800)]
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.
Wilmer Paulino [Tue, 23 Jan 2024 00:29:41 +0000 (16:29 -0800)]
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.
Wilmer Paulino [Fri, 8 Mar 2024 07:03:25 +0000 (23:03 -0800)]
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.
Wilmer Paulino [Mon, 22 Jan 2024 23:28:59 +0000 (15:28 -0800)]
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.
Wilmer Paulino [Mon, 22 Jan 2024 23:24:51 +0000 (15:24 -0800)]
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.
Wilmer Paulino [Mon, 11 Mar 2024 17:21:43 +0000 (10:21 -0700)]
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.
Wilmer Paulino [Thu, 7 Mar 2024 22:37:58 +0000 (14:37 -0800)]
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.
Elias Rohrer [Tue, 19 Mar 2024 14:30:16 +0000 (15:30 +0100)]
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.
benthecarman [Tue, 20 Feb 2024 18:47:50 +0000 (18:47 +0000)]
Add HTLCsTimedOut closing reason
Before a force closure from timed out HTLCs was treated the same as when
the user manually force closed the channel. This leads to various UX
issues. This adds a new `ClosureReason` called `HTLCsTimedOut` that
signifies that the closure was caused because the HTLCs timed out. To go
along with this, previously we'd always send "Channel force-closed" when
force closing the channel in the error message which was ambigous, now
we send the force closure reason so the peer can know why the channel
was closed.
Jeffrey Czyz [Tue, 19 Mar 2024 22:04:01 +0000 (17:04 -0500)]
Use OnionMessenger::send_onion_message in tests
Use OnionMessenger's public interface in tests whenever possible (i.e.,
when not using any intermediate_nodes in an OnionMessagePath. This
allows us to exercise DefaultMessageRouter, and, in particular that a
path can be found for an unannounced sender when its in the introduction
node.
Jeffrey Czyz [Tue, 19 Mar 2024 21:04:43 +0000 (16:04 -0500)]
Fix sender is the introduction node onion messages
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath
where the sender is the introduction node but only if the sender is
announced. If the sender is unannounced, then DefaultMessageRouter will
fail. While DefaultMessageRouter will only create a blinded path with an
announced introduction node, it may receive one where the introduction
node is unannounced. Don't return an error in this case, as the
OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the
offer creator) wants to use it for payments without an intermediary node
and without putting its node id in the offer.
Elias Rohrer [Tue, 19 Mar 2024 14:07:27 +0000 (15:07 +0100)]
Track spent `WatchedOutput`s and re-add if unconfirmed
Previously, we would track a spending transaction but wouldn't account
for it being reorged out of the chain, in which case we wouldn't monitor
the `WatchedOutput`s until they'd be reloaded on restart.
Here, we keep any `WatchedOutput`s around until their spends are
sufficiently confirmed and only prune them after `ANTI_REORG_DELAY`.
Elias Rohrer [Thu, 7 Mar 2024 09:37:08 +0000 (10:37 +0100)]
Expose `{prev,next}_user_channel_id` fields in `PaymentForwarded`
This is useful for users that track channels by `user_channel_id`.
For example, in `lightning-liquidity` we currently keep a full
`HashMap<ChanelId, u128>` around *just* to be able to associate
`PaymentForwarded` events with the channels otherwise tracked by
`user_channel_id`.
Matt Corallo [Thu, 14 Mar 2024 19:47:07 +0000 (19:47 +0000)]
Avoid writing `ChannelManager` when hitting lnd bug 6039
When we hit lnd bug 6039, we end up sending error messages to peers
in a loop. This should be fine, but because we used the generic
`PersistenceNotifierGuard::notify_on_drop` lock above the specific
handling, we end up writing `ChannelManager` every time we manage a
round-trip to our peer.
This can add up quite quickly, and isn't actually changing, so we
really need to avoid writing the `ChannelManager` in this case.
Duncan Dean [Tue, 21 Mar 2023 19:12:53 +0000 (21:12 +0200)]
Implement interactive tx construction protocol via `InteractiveTxConstructor`
This implements the interactive construction protocol described at
https://github.com/lightning/bolts/blob/78e5a6b066d3a8e235931dfc06aa325337874749/02-peer-protocol.md?plain=1#L92.
Our implementation includes a state machine with typed states and transitions
to ensure consumers have compile-time assurances that the protocol is upheld.
States are tracked as in the `StateMachine` enum and can take on all
possible states during the negotiation.
The states are further divided into two categories, namely by the two traits
they implement, either `ReceivedMsgState` or `SentMsgState`.
The defined `StateTransitions` enforce the transitions that `ReceivedMsgState`,
`SentMsgState`, and the `_TxComplete`s can go through.
Co-authored-by: Wilmer Paulino <9447167+wpaulino@users.noreply.github.com> Co-authored-by: Duncan Dean <git@dunxen.dev> Co-authored-by: Jurvis Tan <5944973+jurvis@users.noreply.github.com>
Matt Corallo [Wed, 13 Mar 2024 19:18:20 +0000 (19:18 +0000)]
Replace the generic `parse_int_be` with a macro called twice
`parse_int_be` is generic across integer types and also input
types, but to do so it relies on the `num-traits` crate. There's
not a lot of reason for this now that std has `from_be_bytes`, so
we drop the generic now and replace it with a macro which is called
twice to create two functions, both only supporting conversion from
`u5` arrays.
Matt Corallo [Wed, 13 Mar 2024 19:16:20 +0000 (19:16 +0000)]
Use std's `from_be_bytes` rather than our `to_int_be` for int conv
`lightning-invoice` was mostly written before std's `from_be_bytes`
was stabilized, so used its own `to_int_be` utility to do int
conversions from `u8` arrays. Now that the std option has been
stable for quite some time, we should juse use it instead.
Matt Corallo [Tue, 6 Feb 2024 23:17:50 +0000 (23:17 +0000)]
Add `PersistenceNotifierGuard` take to offer/refund payments
This resolves an issue where offer and refund payments get delayed
while we wait for the `invoice_request`/`invoice` onion messages to
get sent. It further ensures we're likely to have the
`ChannelManager` persisted with the new payment info after
initiating the send/receive.
Matt Corallo [Tue, 6 Feb 2024 23:13:08 +0000 (23:13 +0000)]
Use `{}`, not `{:?}` for `PublicKey` logging
The `Debug` serialization of `PublicKey`s includes both the X and Y
coordinate, which isn't something most of our users deal with.
Instead, logging using `Display` gives users the keys they're used
to.
Matt Corallo [Tue, 12 Mar 2024 15:19:51 +0000 (15:19 +0000)]
Avoid new "out of disk space" issues in CI
Our 1.63 build on Ubuntu has been failing for quite some time
because it runs out of disk space trying to build tests in the last
cfg-flag steps. Thus, we add a few new `cargo clean`s here to fix
it.
Jeffrey Czyz [Thu, 29 Feb 2024 21:21:03 +0000 (15:21 -0600)]
Fail request_refund_payment for unsupported chain
If a Refund has an unsupported chain, ChannelManager should not send an
invoice as it can't be paid on that chain. Instead, return an error when
calling ChannelManager::request_refund_payment for such refunds.
Use correct min/max htlc in test util for constructing blinded pay params.
In testing, we use channel updates to construct blinded paths and the
{Forward,Receive}Tlvs encoded within. Given a blinded path from node A > B > C,
we currently use channel_update_A->B to construct the payment constraints for
A’s blinded payload.
This is incorrect for setting A's PaymentConstraints::htlc_minimum_msat,
because channel_update_A->B contains the minimum value that *B* will accept,
and we want the constraints to contain the min value that *A* will accept.
This never caused test failures before because min/max htlc values were always
identical in both channel directions.
Therefore, set A’s htlc min/max values to the min/max that A will accept.
Jeffrey Czyz [Thu, 7 Mar 2024 22:59:09 +0000 (16:59 -0600)]
Drop error type parameter from SignError
SignError allows implementors of SignFunction to return a custom error
type. Drop this as an unconstrained type causes problems with bindings
and isn't useful unless the caller can take some sort of action based on
different errors.
Jeffrey Czyz [Fri, 1 Mar 2024 16:17:08 +0000 (10:17 -0600)]
Support BOLT 12 signing in c_bindings
Replace the Fn trait bound on signing methods with a dedicated trait
since Fn is not supported in bindings. Implement the trait for Fn so
that closures can still be used in Rust.
Jeffrey Czyz [Mon, 26 Feb 2024 02:42:57 +0000 (20:42 -0600)]
Add c_bindings version of InvoiceBuilder
Use the macros introduced in the previous commit to define two builders
for each type parameterization of InvoiceBuilder
- InvoiceWithExplicitSigningPubkeyBuilder
- InvoiceWithDerivedSigningPubkeyBuilder
The difference between these and InvoiceBuilder is that these have
methods that take `self` by mutable reference instead of by value and
don't return anything instead returning the modified builder. This is
required because bindings don't support move semantics nor impl blocks
specific to a certain type parameterization. Because of this, the
builder's contents must be cloned when building a Bolt12Invoice.
Keeps InvoiceBuilder defined so that it can be used internally in
ChannelManager's OffersMessageHandler even when compiled for c_bindings.
Elias Rohrer [Fri, 8 Mar 2024 09:24:50 +0000 (10:24 +0100)]
Impl `PartialEq`/`Eq` for `Offer`/`Refund`
We add custom implementations based on comparing the `bytes` for
`Offer`/`Refund` themselves as this is sufficient and should be faster
than comapring all fields in the worst case.
Jeffrey Czyz [Sun, 25 Feb 2024 20:58:07 +0000 (14:58 -0600)]
Macro-ize InvoiceBuilder
InvoiceBuilder is not exported to bindings because it has methods that
take `self` by value and are only implemented for certain type
parameterizations. Define these methods using macros such that different
builders and related methods can be defined for c_bindings.
Jeffrey Czyz [Fri, 23 Feb 2024 19:34:24 +0000 (13:34 -0600)]
Add c_bindings version of InvoiceRequestBuilder
Use the macros introduced in the previous commit to define two builders
for each type parameterization of InvoiceRequestBuilder
- InvoiceRequestWithExplicitPayerIdBuilder
- InvoiceRequestWithDerivedPayerIdBuilder
The difference between these and InvoiceRequestBuilder is that these
have methods that take `self` by mutable reference instead of by value
and don't return anything instead returning the modified builder. This
is required because bindings don't support move semantics nor impl
blocks specific to a certain type parameterization. Because of this, the
builder's contents must be cloned when building an InvoiceRequest.
Keeps InvoiceRequestBuilder defined so that it can be used internally in
ChannelManager::pay_for_offer even when compiled for c_bindings.
Jeffrey Czyz [Thu, 22 Feb 2024 22:47:38 +0000 (16:47 -0600)]
Macro-ize InvoiceRequestBuilder
InvoiceRequestBuilder is not exported to bindings because it has methods
that take `self` by value and are only implemented for certain type
parameterizations. Define these methods using macros such that different
builders and related methods can be defined for c_bindings.
Jeffrey Czyz [Sat, 24 Feb 2024 02:30:50 +0000 (20:30 -0600)]
Add c_bindings version of RefundBuilder
Use the macros introduced in the previous commit to define a builder
called RefundMaybeWithDerivedMetadataBuilder.
The difference between this and RefundBuilder is that this has methods
that take `self` by mutable reference instead of by value and don't
return anything instead returning the modified builder. This is required
because bindings don't support move semantics. Because of this, the
builder's contents must be cloned when building a Refund.
Keeps RefundBuilder defined so that it can be used internally in
ChannelManager::create_refund_builder even when compiled for c_bindings.
Jeffrey Czyz [Fri, 23 Feb 2024 22:52:49 +0000 (16:52 -0600)]
Macro-ize RefundBuilder
RefundBuilder is not exported to bindings because it has methods that
take `self` by value. Define these methods using macros such that
different builders and related methods can be defined for c_bindings.
Jeffrey Czyz [Fri, 23 Feb 2024 22:33:23 +0000 (16:33 -0600)]
Add c_bindings version of OfferBuilder
Use the macros introduced in the previous commit to define two builders
for each type parameterization of OfferBuilder
- OfferWithExplicitMetadataBuilder
- OfferWithDerivedMetadataBuilder
The difference between these and OfferBuilder is that these have methods
that take `self` by mutable reference instead of by value and don't
return anything instead returning the modified builder. This is required
because bindings don't support move semantics nor impl blocks specific
to a certain type parameterization. Because of this, the builder's
contents must be cloned when building an Offer.
Keeps OfferBuilder defined so that it can be used internally in
ChannelManager::create_offer_builder even when compiled for c_bindings.
Elias Rohrer [Fri, 1 Mar 2024 10:42:54 +0000 (11:42 +0100)]
Prefer implementing `From` over `Into`
.. as the std library docs state that implementing Into should be avoided:
"One should avoid implementing Into and implement From instead.
Implementing From automatically provides one with an implementation of
Into thanks to the blanket implementation in the standard library."