Matt Corallo [Wed, 9 Oct 2024 19:05:18 +0000 (19:05 +0000)]
Req the counterparty node id when claiming against a closed chan
Currently we store in-flight `ChannelMonitorUpdate`s in the
per-peer structure in `ChannelManager`. This is nice and simple as
we're generally updating it when we're updating other per-peer
data, so we already have the relevant lock(s) and map entries.
Sadly, when we're claiming an HTLC against a closed channel, we
didn't have the `counterparty_node_id` available until it was
added in 0.0.124 (and now we only have it for HTLCs which were
forwarded in 0.0.124). This means we can't look up the per-peer
structure when claiming old HTLCs, making it difficult to track the
new `ChannelMonitorUpdate` as in-flight.
While we could transition the in-flight `ChannelMonitorUpdate`
tracking to a new global map indexed by `OutPoint`, doing so would
result in a major lock which would be highly contended across
channels with different peers.
Instead, as we move towards tracking in-flight
`ChannelMonitorUpdate`s for closed channels we'll keep our existing
storage, leaving only the `counterparty_node_id` issue to contend
with.
Here we simply accept the issue, requiring that
`counterparty_node_id` be available when claiming HTLCs against a
closed channel. On startup, we explicitly check for any forwarded
HTLCs which came from a closed channel where the forward happened
prior to 0.0.124, failing to deserialize, or logging an warning if
the channel is still open (implying things may work out, but panics
may occur if the channel closes prior to HTLC resolution).
While this is a somewhat dissapointing resolution, LDK nodes which
forward HTLCs are generally fairly well-upgraded, so it is not
anticipated to be an issue in practice.
Matt Corallo [Fri, 4 Oct 2024 17:54:00 +0000 (17:54 +0000)]
Build `per_peer_state` immediately in `ChannelManager` deser
Instead of first building a map from peers to a list of channels
then pulling out of that to build the `per_peer_state`, we build
`per_peer_state` immediately and store channels in it immediately.
This avoids an unnecessary map indirection but also gives us
access to the new fields in `per_peer_state` when reading
`Channel`s which we'll need in a coming commit.
Matt Corallo [Sat, 21 Sep 2024 04:23:09 +0000 (04:23 +0000)]
Avoid a `short_to_chan_info` read lock in `claim_funds_from_hop`
In 453ed11f80b40f28b6e95a74b1f7ed2cd7f012ad we started tracking the
counterparty's `node_id` in `HTLCPreviousHopData`, however we were
still trying to look it up using `prev_short_channel_id` in
`claim_funds_from_hop`.
Because we now usually have the counterparty's `node_id` directly
accessible, we should skip the `prev_short_channel_id` lookup.
This will also be more important in the next commit where we need
to look up state for our counterparty to generate
`ChannelMonitorUpdate`s whether we have a live channel or not.
Matt Corallo [Sun, 29 Sep 2024 19:30:48 +0000 (19:30 +0000)]
Add missing `update_maps_on_chan_removal` call in signer restore
When a channel is closed, we have to call
`update_maps_on_chan_removal` in the same per-peer-state lock as
the removal of the `ChannelPhase` object. We forgot to do so in
`ChannelManager::signer_unblocked` leaving dangling references to
the channel.
We also take this opportunity to include more context in the
channel-closure log in `ChannelManager::signer_unblocked` and add
documentation to `update_maps_on_chan_removal` and
`finish_close_channel` to hopefully avoid this issue in the future.
Matt Corallo [Sun, 29 Sep 2024 15:22:29 +0000 (15:22 +0000)]
Pass the `peer_state` lock through to `update_maps_on_chan_removal`
`update_maps_on_chan_removal` is used to perform `ChannelManager`
state updates when a channel is being removed, prior to dropping
the `peer_state` lock. In a future commit we'll use it to update
fields in the `per_peer_state`, but in order to do so we'll need to
have access to that state in the macro.
Here we get set up for this by passing the per-peer state to
`update_maps_on_chan_removal`, which is sadly a fairly large patch.
Matt Corallo [Wed, 2 Oct 2024 18:21:33 +0000 (18:21 +0000)]
Allow a `DNSResolverMessageHandler` to set `dns_resolver` feature
A `DNSResolverMessageHandler` which handles resolution requests
should want the `NodeFeatures` included in the node's
`node_announcement` to include `dns_resolver` to indicate to the
world that it provides that service. Here we enable this by
requesting extra feature flags from the `DNSResolverMessageHandler`
in the features `OnionMessenger`, in turn, provides to
`PeerManager` (which builds the `node_announcement`).
Matt Corallo [Wed, 2 Oct 2024 18:12:38 +0000 (18:12 +0000)]
Add support for parsing the `dns_resolver` feature bit
This feature bit is used to indicate that a node will make DNS
queries on behalf of onion message senders, returning DNSSEC TXT
proofs for the requested names.
It is used to signal support for bLIP 32 resolution and can be used
to find nodes from which we can try to resolve BIP 32 HRNs.
Duncan Dean [Fri, 6 Sep 2024 10:26:19 +0000 (12:26 +0200)]
Add an `explicit_type` TLV syntax for avoiding certain cases of type inference
This new syntax is used to fix "dependency on fallback of ! -> ()".
This avoids cases where code compiles with a fallback of the
never type leading to the unit type. The behaviour in Rust edition 2024
would make this a compile error.
Matt Corallo [Thu, 12 Sep 2024 15:17:15 +0000 (15:17 +0000)]
Call `ChannelMessageHandler::message_received` without peer lock
While `message_received` purports to be called on every message,
prior to the message, doing so on `Init` messages means we have to
call `message_received` while holding the per-peer mutex, which
can cause some lock contention.
Instead, here, we call `message_received` after processing `Init`
messages (which is probably more useful anyway - the peer isn't
really "connected" until we've processed the `Init` messages),
allowing us to call it unlocked.
Matt Corallo [Thu, 12 Sep 2024 15:13:11 +0000 (15:13 +0000)]
Check that we aren't reading a second message in BOLT 12 retry test
`creates_and_pays_for_offer_with_retry` intends to check that we
re-send a BOLT 12 `invoice_request` in response to a
`message_received` call, but doesn't actually test that there were
no messages in the outbound buffer after the initial send, which we
do here.
Matt Corallo [Thu, 12 Sep 2024 15:57:42 +0000 (15:57 +0000)]
Add the core functionality required to resolve Human Readable Names
This adds a new utility struct, `OMNameResolver`, which implements
the core functionality required to resolve Human Readable Names,
namely generating `DNSSECQuery` onion messages, tracking the state
of requests, and ultimately receiving and verifying `DNSSECProof`
onion messages.
It tracks pending requests with a `PaymentId`, allowing for easy
integration into `ChannelManager` in a coming commit - mapping
received proofs to `PaymentId`s which we can then complete by
handing them `Offer`s to pay.
It does not, directly, implement `DNSResolverMessageHandler`, but
an implementation of `DNSResolverMessageHandler` becomes trivial
with `OMNameResolver` handling the inbound messages and creating
the messages to send.
Matt Corallo [Sat, 13 Jul 2024 16:23:47 +0000 (16:23 +0000)]
Add a type to track `HumanReadableName`s
BIP 353 `HumanReadableName`s are represented as `â‚¿user@domain` and
can be resolved using DNS into a `bitcoin:` URI. In the next
commit, we will add such a resolver using onion messages to fetch
records from the DNS, which will rely on this new type to get name
information from outside LDK.
Matt Corallo [Mon, 30 Sep 2024 16:16:36 +0000 (16:16 +0000)]
Add DNS(SEC) query and proof messages and onion message handler
This creates the initial DNSSEC proof and query messages in a new
module in `onion_message`, as well as a new message handler to
handle them.
In the coming commits, a default implementation will be added which
verifies DNSSEC proofs which can be used to resolve BIP 353 URIs
without relying on anything outside of the lightning network.
Matt Corallo [Mon, 30 Sep 2024 14:45:36 +0000 (14:45 +0000)]
Add a `MessageContext::DNSResolution` to protect against probing
When we make a DNSSEC query with a reply path, we don't want to
allow the DNS resolver to attempt to respond to various nodes to
try to detect (through timining or other analysis) whether we were
the one who made the query. Thus, we need to include a nonce in the
context in our reply path, which we set up here by creating a new
context type for DNS resolutions.
Matt Corallo [Wed, 11 Sep 2024 23:36:29 +0000 (23:36 +0000)]
Validate `channel_update` signatures without holding a graph lock
We often process many gossip messages in parallel across different
peer connections, making the `NetworkGraph` mutexes fairly
contention-sensitive (not to mention the potential that we want to
send a payment and need to find a path to do so).
Because we need to look up a node's public key to validate a
signature on `channel_update` messages, we always need to take a
`NetworkGraph::channels` lock before we can validate the message.
For simplicity, and to avoid taking a lock twice, we'd always
validated the `channel_update` signature while holding the same
lock, but here we address the contention issues by doing a
`channel_update` validation in three stages.
First we take a read lock on `NetworkGraph::channels` and check if
the `channel_update` is new, then release the lock and validate the
message signature, and finally take a write lock, (re-check if the
`channel_update` is new) and update the graph.
DefaultRouter::create_blinded_payment_paths may creat a one-hop blinded
path with the recipient as the introduction node. Update the privacy
section of DefaultRouter's docs to indicate this as is done in the docs
for DefaultMessageRouter.
ChannelManager is parameterized by a Router, which must also implement
MessageRouter. Instead, add a MessageRouter parameter such that the
Router and MessageRouter traits can be de-coupled. This simplifies using
something other than DefaultMessageRouter, which DefaultRouter currently
delegates to.
Matt Corallo [Thu, 12 Sep 2024 18:36:58 +0000 (18:36 +0000)]
Add a `PaymentId` for inbound payments
We expect our users to have fully idempotent `Event` handling as we
may replay events on restart for one of a number of reasons. This
isn't a big deal as long as all our events have some kind of
identifier users can use to check if the `Event` has already been
handled.
For outbound payments, this is the `PaymentId` they provide in the
send methods, however for inbound payments we don't have a great
option.
`PaymentHash` largely suffices - users can simply always claim in
response to a `PaymentClaimable` of sufficient value and treat a
`PaymentClaimed` event as duplicate any time they see a second one
for the same `PaymentHash`. This mostly works, but may result in
accepting duplicative payments if someone (incorrectly) pays twice
for the same `PaymentHash`.
Users could also fail for duplicative `PaymentClaimable` events of
the same `PaymentHash`, but doing so may result in spuriously
failing a payment if the `PaymentClaimable` event is a replay and
they never saw a corresponding `PaymentClaimed` event.
While none of this will result in spuriously thinking they've been
paid when they have not, it does result in some pretty awkward
semantics which we'd rather avoid our users having to deal with.
Instead, here, we add a new `PaymentId` which is simply an HMAC of
the HTLCs (as Channel ID, HTLC ID pairs) which were included in the
payment.
Matt Corallo [Sun, 8 Sep 2024 16:38:22 +0000 (16:38 +0000)]
Add an `inbound_payment_id_secret` to `ChannelManager`
In the next commit we'll start generating `PaymentId`s for inbound
payments randomly by HMAC'ing the HTLC set of the payment. Here we
start by defining the HMAC secret for these HMACs.
This requires one small test adaptation and a full_stack_target
fuzz change because it changes the RNG consumption.
Matt Corallo [Sun, 8 Sep 2024 16:09:12 +0000 (16:09 +0000)]
Do not check the ordering of HTLCs in `PaymentClaim[able,ed]`
In the next commit we'll change the order of HTLCs in
`PaymentClaim[able,ed]` events. This shouldn't break anything, but
our current functional tests check that the HTLCs are provided in
the order they expect (the order they were received). Instead, here
we only validate that each claimed HTLC matches one expected path.
Matt Corallo [Wed, 18 Sep 2024 18:03:11 +0000 (18:03 +0000)]
Only attempt to `rustfmt` files checked into git
This avoids `rustfmt` failing on Rust files generated by dependent
crates in `target`, eg
```
+ rustfmt --edition 2021 --check ./target/debug/build/thiserror-8230374e07b5c05a/out/probe.rs
Diff in /home/matt/rust-lightning-3/target/debug/build/thiserror-8230374e07b5c05a/out/probe.rs at line 1:
Elias Rohrer [Wed, 4 Sep 2024 09:10:13 +0000 (11:10 +0200)]
Check workspace members with default features individually in CI
Previously, we would only check the workspace as a whole. This however
would mean that we would check/test crates with `lightning`'s default
features enabled, allowing failures-to-build under the crates own
default features to slip through, if they didn't explicitly enable
`lightning/std`, for example.
Here, we extend the CI to check the workspace as a whole but then run
checks, tests, and doc generation on the workspace members individually,
asserting that all of them build even when not built as part of the same
workspace as `lightning`.
Fix bug where we double-pay an offer due to stale manager
This fixes the following bug:
- An outbound payment is AwaitingInvoice
- We receive an invoice and lock the HTLCs into the relevant ChannelMonitors
- The monitors are successfully persisted, but the ChannelManager fails to
persist, so the outbound payment remains AwaitingInvoice
- We restart, causing the channels to close due to a stale ChannelManager
- We receive a duplicate invoice, and attempt to pay it again due to the
payment still being AwaitingInvoice in the stale ChannelManager
After the fix for this, we will notice that the payment is already locked into
the monitor on startup and transition the incorrectly-AwaitingInvoice payment
to Retryable, which prevents double-paying on duplicate invoice receipt.
Jeffrey Czyz [Thu, 1 Aug 2024 15:56:51 +0000 (10:56 -0500)]
Rename Offer::signing_pubkey to Offer::issuer_signing_pubkey
The spec was recently changed to use offer_issuer_id instead of
offer_node_id. LDK always used signing_pubkey to avoid confusion with a
node_id. Rename it to issuer_signing_pubkey now as InvoiceRequest and
Bolt12Invoice will have similarly named methods in upcoming commits.
Move the code that ensures that HTLCs locked into ChannelMonitors are
synchronized with the ChannelManager's OutboundPayments store to the
outbound_payments module.
This is useful both because ChannelManager::read is very long/confusing method,
so it's nice to encapsulate some of its functionality, and because we need to
fix an existing bug in this logic where we may risk double-paying an offer due
to outbound_payments being stale on startup. See the next commit for this
bugfix.