shaavan [Tue, 21 Nov 2023 14:41:46 +0000 (20:11 +0530)]
Explicitly reject routes that double-back
- If a path within a route passes through the same channelID twice,
that shows the path is looped and will be rejected by nodes.
- Add a check to explicitly reject such payment before trying to send
them.
Matt Corallo [Mon, 13 Nov 2023 22:50:40 +0000 (22:50 +0000)]
`derive(Hash)` for P2P messages
In other languages (Java and C#, notably), overriding `Eq` without
overriding `Hash` can lead to surprising or broken behavior. Even
in Rust, its usually the case that you actually want both. Here we
add missing `Hash` derivations for P2P messages, to at least
address the first pile of warnings the C# compiler dumps.
Matt Corallo [Mon, 13 Nov 2023 22:48:48 +0000 (22:48 +0000)]
Rely on const generic big arrays for `PartialEq` in msgs
Implementation of standard traits on arrays longer than 32 elements
was shipped in rustc 1.47, which is below our MSRV of 1.48 and we
can use to remove some unnecessary manual implementation of
`PartialEq` on `OnionPacket`.
Matt Corallo [Sat, 26 Aug 2023 19:59:21 +0000 (19:59 +0000)]
Don't send init `closing_signed` too early after final HTLC removal
If we remove an HTLC (or fee update), commit, and receive our
counterparty's `revoke_and_ack`, we remove all knowledge of said
HTLC (or fee update). However, the latest local commitment
transaction that we can broadcast still contains the HTLC (or old
fee), thus we are not eligible for initiating the `closing_signed`
negotiation if we're shutting down and are generally expecting a
counterparty `commitment_signed` immediately.
Because we don't have any tracking of these updates in the `Channel`
(only the `ChannelMonitor` is aware of the HTLC being in our latest
local commitment transaction), we'd previously send a
`closing_signed` too early, causing LDK<->LDK channels with an HTLC
pending towards the channel initiator at the time of `shutdown` to
always fail to cooperatively close.
To fix this race, we add an additional unpersisted bool to
`Channel` and use that to gate sending the initial `closing_signed`.
Matt Corallo [Tue, 31 Oct 2023 18:39:04 +0000 (18:39 +0000)]
Drop non-anchor channel fee upper bound limit entirely
Quite a while ago we added checks for the total current dust
exposure on a channel to explicitly limit dust inflation attacks.
When we did this, we kept the existing upper bound on the channel's
feerate in place. However, these two things are redundant - the
point of the feerate upper bound is to prevent dust inflation, and
it does so in a crude way that can cause spurious force-closures.
Here we simply drop the upper bound entirely, relying on the dust
inflation limit to prevent dust inflation instead.
Matt Corallo [Fri, 10 Nov 2023 19:23:21 +0000 (19:23 +0000)]
Replace maze of BOLT11 payment utilities with parameter generators
`lightning-invoice` was historically responsible for actually
paying invoices, handling retries and everything. However, that
turned out to be buggy and hard to maintain, so the payment logic
was eventually moved into `ChannelManager`. However, the old
utilites remain.
Because our payment logic has a number of tunable parameters and
there are different ways to pay a BOLT11 invoice, we ended up with
six different methods to pay or probe a BOLT11 invoice, with more
requested as various options still were not exposed.
Instead, here, we replace all six methods with two simple ones
which return the arguments which need to be passed to
`ChannelManager`. Those arguments can be further tweaked before
passing them on, allowing more flexibility.
Matt Corallo [Sat, 4 Nov 2023 23:02:18 +0000 (23:02 +0000)]
Stop writing signer data as a part of channels
This breaks backwards compatibility with versions of LDK prior to
0.0.113 as they expect to always read signer data.
This also substantially reduces allocations during `ChannelManager`
serialization, as we currently don't pre-allocate the `Vec` that
the signer gets written in to. We could alternatively pre-allocate
that `Vec`, but we've been set up to skip the write entirely for a
while, and 0.0.113 was released nearly a year ago. Users
downgrading to LDK 0.0.112 and before at this point should not be
expected.
Matt Corallo [Sat, 4 Nov 2023 22:09:44 +0000 (22:09 +0000)]
Avoid allocating when checking gossip message signatures
When we check gossip message signatures, there's no reason to
serialize out the full gossip message before hashing, and it
generates a lot of allocations during the initial startup when we
fetch the full gossip from peers.
Matt Corallo [Sat, 4 Nov 2023 21:21:58 +0000 (21:21 +0000)]
Avoid a `tokio::mpsc::Sender` clone for each P2P send operation
Whenever we go to send bytes to a peer, we need to construct a
waker for tokio to call back into if we need to finish sending
later. That waker needs some reference to the peer's read task to
wake it up, hidden behind a single `*const ()`. To do this, we'd
previously simply stored a `Box<tokio::mpsc::Sender>` in that
pointer, which requires a `clone` for each waker construction. This
leads to substantial malloc traffic.
Instead, here, we replace this box with an `Arc`, leaving a single
`tokio::mpsc::Sender` floating around and simply change the
refcounts whenever we construct a new waker, which we can do
without allocations.
Matt Corallo [Sat, 4 Nov 2023 20:37:21 +0000 (20:37 +0000)]
Avoid re-allocating to encrypt gossip messages when forwarding
When we forward gossip messages, we store them in a separate buffer
before we encrypt them (and commit to the order in which they'll
appear on the wire). Rather than storing that buffer encoded with
no headroom, requiring re-allocating to add the message length and
two MAC blocks, we here add the headroom prior to pushing it into
the gossip buffer, avoiding an allocation.
Matt Corallo [Sat, 4 Nov 2023 20:20:12 +0000 (20:20 +0000)]
Use `VecDeque`, rather than `LinkedList` in peer message buffering
When buffering outbound messages for peers, `LinkedList` adds
rather substantial allocation overhead, which we avoid here by
swapping for a `VecDeque`.
Matt Corallo [Mon, 6 Nov 2023 16:57:13 +0000 (16:57 +0000)]
Avoid unnecessarily alloc'ing a new buffer when decrypting messages
When decrypting P2P messages, we already have a read buffer that we
read the message into. There's no reason to allocate a new `Vec` to
store the decrypted message when we can just overwrite the read
buffer and call it a day.
Matt Corallo [Sat, 4 Nov 2023 21:01:18 +0000 (21:01 +0000)]
Pre-allocate the full `Vec` prior to serializing as a `Vec<u8>`
We end up generating a substantial amount of allocations just
doubling `Vec`s when serializing to them, and our
`serialized_length` method is generally rather effecient, so we
just rely on it and allocate correctly up front.
Orbital [Fri, 3 Nov 2023 00:50:20 +0000 (19:50 -0500)]
refactor to remove message_digest
We change the Bolt12Invoice struct to carry a tagged hash. Because
message_digest is then only used in one place, we can inline it in
the TaggedHash constructor.
Matt Corallo [Sat, 4 Nov 2023 03:53:46 +0000 (03:53 +0000)]
Reduce on-startup heap frag due to network graph map/vec doubling
When we're reading a `NetworkGraph`, we know how many
nodes/channels we are reading, there's no reason not to
pre-allocate the `IndexedMap`'s inner `HashMap` and `Vec`, which we
do here.
This seems to reduce on-startup heap fragmentation with glibc by
something like 100MiB.
Matthew Rheaume [Tue, 31 Oct 2023 22:36:11 +0000 (15:36 -0700)]
Added `temporary_channel_id` to `create_channel`.
By default, LDK will generate the initial temporary channel ID for you.
However, in certain cases, it's desirable to have a temporary channel ID
specified by the caller in case of any pre-negotiation that needs to
happen between peers prior to the channel open message. For example, LND
has a `FundingShim` API that allows for advanced funding flows based on
the temporary channel ID of the channel.
This patch adds support for optionally specifying the temporary channel
ID of the channel through the `create_channel` API.
Chris Waterson [Wed, 6 Sep 2023 18:38:34 +0000 (11:38 -0700)]
Add basic async signer tests
Adds a `get_signer` method to the context so that a test can get ahold of the
channel signer. Adds a `set_available` method on the `TestChannelSigner` to
allow a test to enable and disable the signer: when disabled some of the
signer's methods will return `Err` which will typically activate the error
handling case. Adds a `set_channel_signer_available` function on the test
`Node` class to make it easy to enable and disable a specific signer.
Adds a new `async_signer_tests` module:
* Check for asynchronous handling of `funding_created` and `funding_signed`.
* Check that we correctly resume processing after awaiting an asynchronous
signature for a `commitment_signed` event.
* Verify correct handling during peer disconnect.
* Verify correct handling for inbound zero-conf.
If sign_counterparty_commitment fails (i.e. because the signer is
temporarily disconnected), this really indicates that we should
retry the message sending which required the signature later,
rather than force-closing the channel (which probably won't even
work if the signer is missing).
This commit adds retrying of inbound funding_created signing
failures, regenerating the `FundingSigned` message, attempting to
re-sign, and sending it to our peers if we succeed.
If sign_counterparty_commitment fails (i.e. because the signer is
temporarily disconnected), this really indicates that we should
retry the message sending which required the signature later,
rather than force-closing the channel (which probably won't even
work if the signer is missing).
This commit adds retrying of outbound funding_created signing
failures, regenerating the `FundingCreated` message, attempting to
re-sign, and sending it to our peers if we succeed.
If sign_counterparty_commitment fails (i.e. because the signer is
temporarily disconnected), this really indicates that we should
retry the message sending which required the signature later,
rather than force-closing the channel (which probably won't even
work if the signer is missing).
This commit adds initial retrying of failures, specifically
regenerating commitment updates, attempting to re-sign the
`CommitmentSigned` message, and sending it to our peers if we
succed.
Matt Corallo [Tue, 5 Sep 2023 21:13:07 +0000 (21:13 +0000)]
Handle sign_counterparty_commitment failing during inb funding
If sign_counterparty_commitment fails (i.e. because the signer is
temporarily disconnected), this really indicates that we should
retry the message sending which required the signature later,
rather than force-closing the channel (which probably won't even
work if the signer is missing).
Here we add initial handling of sign_counterparty_commitment
failing during inbound channel funding, setting a flag in
`ChannelContext` which indicates we should retry sending the
`funding_signed` later. We don't yet add any ability to do that
retry.
Matt Corallo [Tue, 5 Sep 2023 21:06:22 +0000 (21:06 +0000)]
Handle sign_counterparty_commitment failing during outb funding
If sign_counterparty_commitment fails (i.e. because the signer is
temporarily disconnected), this really indicates that we should
retry the message sending which required the signature later,
rather than force-closing the channel (which probably won't even
work if the signer is missing).
Here we add initial handling of sign_counterparty_commitment
failing during outbound channel funding, setting a new flag in
`ChannelContext` which indicates we should retry sending the
`funding_created` later. We don't yet add any ability to do that
retry.
Matt Corallo [Tue, 5 Sep 2023 20:46:28 +0000 (20:46 +0000)]
Handling for sign_counterparty_commitment failing during normal op
If sign_counterparty_commitment fails (i.e. because the signer is
temporarily disconnected), this really indicates that we should
retry the message sending later, rather than force-closing the
channel (which probably won't even work if the signer is missing).
Here we add initial handling of sign_counterparty_commitment
failing during normal channel operation, setting a new flag in
`ChannelContext` which indicates we should retry sending the
commitment update later. We don't yet add any ability to do that
retry.
Jeffrey Czyz [Wed, 18 Oct 2023 21:28:54 +0000 (16:28 -0500)]
Re-add one-hop onion message fuzzing test
Revert fuzz test removal in 6dc42235baaa22320ad78d3e05fab31edad99328.
The test originally checked that OnionMessenger would fail for one-hop
blinded paths. The commit added support for such paths, but changing the
checks was not sufficient since the node was not connected to the
introduction node of the reply path. This is required in order to work
with the trivial TestMessageRouter. Fix this by explicitly connecting
the nodes.
Jeffrey Czyz [Wed, 25 Oct 2023 21:37:26 +0000 (16:37 -0500)]
Wrap long onion_message fuzz strings
Some editors like vim slow to a crawl when scrolling over long strings
when syntax highlighting is turned on. Limit the length in fuzz strings
to avoid this.
Additional BOLT 12 tests specific to Offer were added, which will live
in the offer module. Thus, it makes sense to move the bech32 tests to
the parse module.
Matt Corallo [Mon, 23 Oct 2023 18:55:17 +0000 (18:55 +0000)]
Use a tuple, not a struct, for `PendingOnionMessage` in bindings
Bindings aren't currently able to handle a struct with a generic
which is actually exposed - we map all structs concretely to a
single type, whereas having fluctuating types on a struct requires
mapping the inner field to a trait first.
Since this isn't super practical, we make `PendingOnionMessage` a
tuple in bindings, rather than a struct.
Matt Corallo [Sat, 21 Oct 2023 02:42:48 +0000 (02:42 +0000)]
Do not compile the `Simple*` type aliases in `c_bindings` at all
Because the bindings changes now require further changes to our
type definitions, avoiding building the `Simple*` type aliases
entirely makes the patchset there simpler.
Matt Corallo [Fri, 20 Oct 2023 17:34:12 +0000 (17:34 +0000)]
Use `Default::default()` for scoring params in tests
In 26c1639ab69d6780c97a118f09e42cb42304088a we switched to using
`Default::default()` to initialize `()` for scoring parameters in
tests. A number of `()`s slipped back in recently, which we replace
here.
Wilmer Paulino [Fri, 13 Oct 2023 21:09:37 +0000 (14:09 -0700)]
Don't sign holder HTLCs along with holder commitments
`sign_holder_commitment_and_htlcs` never really made sense. Unlike
`sign_counterparty_commitment`, the signatures for holder HTLC
transactions may be required much later than the commitment
transaction's. While it was nice for us to only reach the signer once to
obtain all holder signatures, it's not really ideal anymore as we want
our signatures to be random and not reused.
We no longer return all holder HTLC signatures and instead defer to
obtaining them via `EcdsaChannelSigner::sign_holder_htlc_transaction`.