Matt Corallo [Sun, 5 Mar 2023 20:38:42 +0000 (20:38 +0000)]
Avoid enums containing references with lifetimes
Having struct fields with references to other structs is tough in
our bindings logic, but even worse if the fields are in an enum.
Its simplest to just take the clone penalty here.
Matt Corallo [Tue, 28 Feb 2023 21:45:14 +0000 (21:45 +0000)]
Export `outbound_payment` structs in their respective modules
Re-exports in Rust make `use` statements a little shorter, but for
otherwise don't materially change a crate's API. Sadly, the C
bindings generator currently can't figure out re-exports, but it
also exports everything into one global namespace, so it doesn't
matter much anyway.
Matt Corallo [Tue, 23 Jan 2024 19:55:24 +0000 (19:55 +0000)]
Stop relying on a `Clone`able `NetworkGraph` ref in `DefaultRouter`
While there's not really much harm in requiring a `Clone`able
reference (they almost always are), it does make our bindings
struggle a bit as they don't support multi-trait bounds (as it
would require synthesizing a new C trait, which the bindings don't
do automatically). Luckily, there's really no reason for it, and we
can just call the `DefaultMessageRouter` directly when we want to
route a message.
Matt Corallo [Thu, 18 Jan 2024 20:53:20 +0000 (20:53 +0000)]
Move RGS `GraphSyncError` into the top-level module
The top-level module is only a few hundred lines, so there's not a
lot of reason to hide the `GraphSyncError` in its own module.
Instead, we simply move it to the top-level `lib.rs`, which doesn't
change the public API as it was previously re-exported at the top
level.
Matt Corallo [Tue, 23 Jan 2024 19:49:36 +0000 (19:49 +0000)]
Store `EntropySource` in `DefaultRouter` instead of passing it
...as an arg to `Router`. Passing an `EntropySource` around all
the time is a bit strange as the `Router` may or may not actually
use it, and the `DefaultRouter` can just as easily store it.
The bindings cannot express lifetimes, so exposing any field which
is a reference (and not `clone`-able, at least for garbage
collected language bindings) is unsafe for those expecting a
high-level interface.
Thus, we simply no-export the `RouteHopCandidate` inner struct
fields which are references (there are relevant accessors for them
anyway).
Matt Corallo [Thu, 18 Jan 2024 00:42:55 +0000 (00:42 +0000)]
[bindings] Mark `WithContext` log wrapper with no-export
There's not a lot of reason for downstream users to use the
`WithContext` wrapper, it mostly exists for our own downstream
crates anyway, and dealing with lifetimes in bindings isn't super
practical, so simply no-export it.
Matt Corallo [Wed, 20 Dec 2023 05:17:43 +0000 (05:17 +0000)]
[bindings] Drop the lifetime bound on `Record` for bindings builds
Because log `Record`s are now being passed by ownership to `log`,
the bindings get quite annoyed that there's a lifetime hanging
around. We already don't use this lifetime in bindings (the
`FormatArgs` is converted to a string and stored on the heap), so
we can just drop the lifetime, even though it requires some
macro'ing of the struct definition to do so.
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.
Matt Corallo [Fri, 16 Feb 2024 19:26:22 +0000 (19:26 +0000)]
Fix `Route` serialization round-trip
When the `max_total_routing_fee_msat` parameter was added to
`RouteParameters`, the serialization used `map` to get the max fee,
accidentally writing an `Option<Option<u64>>`, but then read it as
an `Option<u64>`. Thus, any `Route`s with a `route_params` written
will fail to be read back.
Luckily, this is an incredibly rarely-used bit of code, so only one
user managed to hit it.
Matt Corallo [Fri, 16 Feb 2024 18:41:54 +0000 (18:41 +0000)]
Fix blinded path serialization in `Route`
`Route`'s blinded_path serialization logic writes a blinded path
`Option` per path hop, however on read we (correctly) only read one
blinded path `Option` per path. This causes serialization of
`Route`s with blinded paths to fail to round-trip.
Here we fix this by writing blinded paths per path.
Matt Corallo [Thu, 28 Mar 2024 22:02:09 +0000 (22:02 +0000)]
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.
Matt Corallo [Tue, 26 Mar 2024 15:08:20 +0000 (15:08 +0000)]
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.
Jeffrey Czyz [Thu, 18 Jan 2024 21:34:19 +0000 (15:34 -0600)]
Fix panic when peer is mid-handshake
Peer::their_node_id is set to Some during the handshake process.
However, df3ab2ee2753e7f9ec02ddf1c8a51db77c50e35d accesses the field
unconditionally, causing a panic. This may be triggered if a gossip
message is received mid-handshake from another peer or if the user calls
broadcast_node_announcement during this time. The latter tends to be
executed on a timer.
Ensure that Peer::their_node_id is only accessed once the handshake is
complete.
Matt Corallo [Mon, 22 Jan 2024 20:26:32 +0000 (20:26 +0000)]
Fix deadlock when handling bad calls to `batch_funding.._generated`
When handling calls to `batch_funding_transaction_generated` which
were missing outputs for one of the batch channels, we'd previously
deadlock when trying to clean up the now-closed channels. This
fixes that and adds a new test case for it.
Jeffrey Czyz [Thu, 11 Jan 2024 19:13:12 +0000 (13:13 -0600)]
Use consistent cltv_expiry_delta in ForwardTlvs
When converting from CounterpartyForwardingInfo to PaymentRelay, the
cltv_expiry_delta is copied. Then, when forming a blinded payment path,
the value is mutated so that esoteric values don't reveal information
about the path. However, the value was only used in computing
PaymentConstraints and wasn't actually updated in PaymentRelay. Move the
logic for modifying the cltv_expiry_delta to the conversion code to
avoid this inconsistency.
Fix sender double-including shadow offset in CLTV expiry height.
The excess delta is included in the final RouteHop::cltv_expiry_delta, so by
adding it explicitly to cur_cltv we were erroneously including it twice in the
total cltv expiry.
This could've add up to an extra MAX_SHADOW_CLTV_DELTA_OFFSET (432) blocks to
the total cltv expiry.
Matt Corallo [Mon, 15 Jan 2024 01:25:10 +0000 (01:25 +0000)]
Move cryptographic algorithms and utilities to a new `crypto` mod
As we'd generally like the `lightning` crate to, over time, have
more modules rather than being very monolithic, we should move the
cryptographic things into their own module, which we do here.
We also take this opportunity to move stream adapters into their
own module and make clear that the ChaChaPoly `decrypt` method is
variable time.
Previously, we were setting the final blinded hop's CLTV expiry height to
best_block_height + total_blinded_path_cltv_delta + shadow_cltv_offset. This is
incorrect, it should instead be set to best_block_height + shadow_cltv_offset
only -- it doesn't make sense to include the delta for the other blinded hops
in the final hop's expiry.
The reason this too-high final cltv value didn't cause test failures previously
is because of a 2nd bug that is fixed in an upcoming commit where the sender
adds the shadow offset twice to the total path CLTV expiry. This 2nd offset
meant that intermediate nodes had some buffer CLTV to subtract their delta from
while still (usually) have enough leftover to meet the expiry in the final hop's
onion.
Matt Corallo [Wed, 10 Jan 2024 18:27:57 +0000 (18:27 +0000)]
Expose `onion_message` items directly rather than via re-exports
When we originally added the `onion_message` module, there weren't
a lot of public items in it, and it didn't make a lot of sense to
export the whole sub-module structure publicly. So, instead, we
exported the public items via re-exports directly in the
`onion_message` top-level module. However, as time went on, more
and more things entered the module, which left the top-level module
rather cluttered.
Worse, in 0.0.119, we exposed
`onion_message::messenger::SendSuccess` via the return type of
`send_message`, but forgot to re-export the enum itself, making
it impossible to actually use from external code.
Here we address both issues and simply replace the re-export with
the underlying sub-module structure.
Matt Corallo [Wed, 10 Jan 2024 22:24:43 +0000 (22:24 +0000)]
Drop `PeerManager` type bound on `UtxoLookup` entirely
In 67659677d4fdb6bf418d66dfa37c61706425232b we relaxed the bounds
set on `UtxoLookup` to enable those using `RoutingMessageHandler`
other than `P2PGossipSync` to use `UtxoLookup`. Sadly, because this
requires having a concrete `PeerManager` type which does *not* use
`UtxoLookup` in the `RoutingMessageHandler` type, this broke users
who were directly using `P2PGossipSync`.
We could split `UtxoLookup` into two, with different bounds, for
the two use-cases, but instead here we simply switch to storing a
reference to the `PeerManager` via a `dyn Fn` which allows us to
wake the `PeerManager` when we need to.
The prior name seems to reference onion decode errors specifically, when in
fact the error contents are generic failure codes for any error that occurs
during HTLC receipt.
Matt Corallo [Mon, 27 Nov 2023 18:51:13 +0000 (18:51 +0000)]
Drop the unused `PaymentKey` type
935a716cc6c4fada075e2b740a70bb1b7b349d49 added new wrappers for the
various channel keys, including a payment_key. However, the
`payment_key` has been unused in lightning since the introduction
(and broad requiring) of the `static_remotekey` feature.
Thus, we simply remove it (and an incredibly stale TODO) here.
Matt Corallo [Tue, 9 Jan 2024 17:11:03 +0000 (17:11 +0000)]
Test individual monitor update compl in chanmon_consistency fuzzer
When users do async monitor updating, it may not be the case that
all pending monitors will complete updating at once. Thus, we
should fuzz monitor updates completing out of order, which we do
here.