Include payment_nonce in PaymentSent for payer proof construction#83
Closed
vincenzopalazzo wants to merge 97 commits intomainfrom
Closed
Include payment_nonce in PaymentSent for payer proof construction#83vincenzopalazzo wants to merge 97 commits intomainfrom
vincenzopalazzo wants to merge 97 commits intomainfrom
Conversation
When we first added auto-archiving of resolved `ChannelMonitor`s, we wanted to be somewhat cautious of flipping it on by default as archiving a `ChannelMonitor` too soon would be a critical bug and, while we were confident in it, we weren't 100%. Since then its been used extensively in various LDK deployments, including `ldk-node`. Given its now seen substantial use, and performs an important anti-DoS function, here we flip to calling it by default on a new timer in `lightning-background-processor`. Fixes lightningdevkit#218
Rather than `match`ing on on several optional objects (with another one to come in a future commit), build an iterator over the futures using the fact that an `Option` is an iterator.
`P2PGossipSync` is a rather poor design. It currently basically requires two circular `Arc` references, leaving `NetworkGraph`s to leak if LDK is un-loaded: * `P2PGossipSync` owns/holds a reference to the `GossipVerifier` and `GossipVerifier` holds an `Arc` to the `P2PGossipSync` and * `PeerManager` holds a reference to the `P2PGossipSync` (as the gossip message handler) which owns/holds a reference to the `GossipVerifier`, which has a `Deref` (likely an `Arc` in practice) to the `PeerManager`. Instead, we should move towards the same design we have elsewhere - hold a `Notifier` and expose waiting on it to the background processor then poll for completion from there (in this case, as in others by checking for completion when handling `get_and_clear_pending_msg_events` calls). Here we take the first step towards this, adding a shared `Notifier` to `PendingChecks` and piping it through to `UtxoFuture`s so that they can be simply resolved and wake the background processor (once it waits on the new `Notifier`).
`P2PGossipSync` is a rather poor design. It currently basically requires two circular `Arc` references, leaving `NetworkGraph`s to leak if LDK is un-loaded: * `P2PGossipSync` owns/holds a reference to the `GossipVerifier` and `GossipVerifier` holds an `Arc` to the `P2PGossipSync` and * `PeerManager` holds a reference to the `P2PGossipSync` (as the gossip message handler) which owns/holds a reference to the `GossipVerifier`, which has a `Deref` (likely an `Arc` in practice) to the `PeerManager`. Instead, we should move towards the same design we have elsewhere - hold a `Notifier` and expose waiting on it to the background processor then poll for completion from there (in this case, as in others by checking for completion when handling `get_and_clear_pending_msg_events` calls). Here we do the bulk of this work, moving `UtxoFuture` resolution to a simple function that signals the `Notifier` and stores the result. We then poll to convert the result into forwarded messages in `P2PGossipSync::get_and_clear_pending_message_events`. Note that we still rely on manual wakeups from the gossip validator, but that will be fixed in the next commit.
`P2PGossipSync` is a rather poor design. It currently basically requires two circular `Arc` references, leaving `NetworkGraph`s to leak if LDK is un-loaded: * `P2PGossipSync` owns/holds a reference to the `GossipVerifier` and `GossipVerifier` holds an `Arc` to the `P2PGossipSync` and * `PeerManager` holds a reference to the `P2PGossipSync` (as the gossip message handler) which owns/holds a reference to the `GossipVerifier`, which has a `Deref` (likely an `Arc` in practice) to the `PeerManager`. Instead, we should move towards the same design we have elsewhere - hold a `Notifier` and expose waiting on it to the background processor then poll for completion from there (in this case, as in others by checking for completion when handling `get_and_clear_pending_msg_events` calls). After the last few commits of setup, here we finally switch to waking the background processor directly when we detect async gossip validation completion, allowing us to drop the circular references in `P2PGossipSync`/`GossipVerifier` entirely. Fixes lightningdevkit#3369
Now that we do not rely on circular references for `P2PGossipSync` validation, we no longer need the hacky `P2PGossipSync::add_utxo_lookup` method to add the gossip validator after building the `P2PGossipSync` first. Thus, we remove it here, updating some tests that relied on it.
This commit fixes the payment_hash function of Bolt11Invoice to return a PaymentHash type instead of a sha256 byte stream. Code and test files dependent on this function have also been modified to adhere to the updated changes.
`LSPS5ServiceEvent::SendWebhookNotification`'s docs say to send the `WebhookNotification` as the HTTP request body "as JSON", which is great, but it leaves the dev to figure out how to do that. Its nice to have a helper to do that, which is trivial so we provide it here.
…5-auto-serialize Add a trivial helper to LSPS5's `WebhookNotification`
Because they end up both being used to validate a `Bolt12Invoice`, we ended up with a single `OffersContext` both for inclusion in a `Refund` and an `InvoiceRequest`. However, this is ambiguous, and while it doesn't seem like an issue, it also seems like a nice property to only use a given `OffersContext` in one place. Further, in the next commit, we use `OffersContext` to figure out what we're building a blinded path for and changing behavior based on it, so its nice to be unambiguous. Thus, we split the single existing context into `OutboundPaymentInRefund` and `OutboundPaymentInInvReq`.
After much discussion in lightningdevkit#3246 we mostly decided to allow downstream developers to override whatever decisions the `DefaultMessageRouter` makes regarding blinded path selection by providing easy overrides for the selected `OnionMessageRouter`. We did not, however, actually select good defaults for `DefaultMessageRouter`. Here we add those defaults, taking advantage of the `MessageContext` we're given to detect why we're building a blinded path and selecting blinding and compaction parameters based on it. Specifically, if the blinded path is not being built for an offers context, we always use a non-compact blinded path and always pad it to four hops (including the recipient). However, if the blinded path is being built for an `Offers` context which implies it might need to fit in a QR code (or, worse, a payment onion), we reduce our padding and try to build a compact blinded path if possible. We retain the `NodeIdMessageRouter` to disable compact blinded path creation but use the same path-padding heuristic as for `DefaultMessageRouter`.
If we're building a blinded message path with extra dummy hops, we have to ensure we at least hide the length of the data in pre-final hops as otherwise the dummy hops are trivially obvious. Here we do so, taking an extra `bool` parameter to `BlindedMessagePath` constructors to decide whether to pad every hop to the existing `MESSAGE_PADDING_ROUND_OFF` or whether to only ensure that each non-final hop has an identical hop data length. In cases where the `DefaultMessageRouter` opts to use compact paths, it now also selects compact padding, whether short channel IDs are available or not.
…use-non-compact-bps Default to padding blinded paths
…prune Automatically archive resolved `ChannelMonitor`s in the BP
Using extend is slightly cleaner because it doesn't require mut on the parameters.
Extract error message strings into local variables before constructing ChannelError return tuples, reducing nesting and improving readability.
Tests previously selected a random block connect style to improve coverage. However, this randomness made test output non-deterministic and significantly cluttered diffs when comparing logs before and after changes. To address this, an LDK_TEST_CONNECT_STYLE environment variable is added to override the random selection and enable deterministic test runs. Note that broader coverage may be better achieved via targeted tests per connect style or a test matrix cycling through all styles, but this change focuses on improving reproducibility and debuggability.
…ip-validate-arc-loop Remove circular reference in `GossipVerifier`
Add a `RandomState` hasher implementation for tests that supports deterministic behavior via the `LDK_TEST_DETERMINISTIC_HASHES=1` environment variable. When set, hash maps use fixed keys ensuring consistent iteration order across test runs. By default, tests continue to use std's RandomState for random hashing, keeping test behavior close to production. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…-var Improve test determinism with configurable connect style and deterministic hash maps
Reading `ChannelMonitor`s on startup is one of the slowest parts of
LDK initialization. Now that we have an async `KVStore`, there's no
need for that, we can simply paralellize their loading, which we do
here.
Sadly, because Rust futures are pretty unergonomic, we have to add
some `unsafe {}` here, but arguing its fine is relatively
straightforward.
`tokio::spawn` can be use both to spawn a forever-running background task or to spawn a task which gets `poll`ed independently and eventually returns a result which the callsite wants. In LDK, we have only ever needed the first, and thus didn't bother defining a return type for `FutureSpawner::spawn`. However, in the next commit we'll start using `FutureSpawner` in a context where we actually do want the spawned future's result. Thus, here, we add a result output to `FutureSpawner::spawn`, mirroring the `tokio::spawn` API.
`MonitorUpdatingPersister::read_all_channel_monitors_with_updates` was made to do the IO operations in parallel in a previous commit, however in practice this doesn't provide material parallelism for large routing nodes. Because deserializing `ChannelMonitor`s is the bulk of the work (when IO operations are sufficiently fast), we end up blocked in single-threaded work nearly the entire time. Here, we add an alternative option - a new `read_all_channel_monitors_with_updates_parallel` method which uses the `FutureSpawner` to cause the deserialization operations to proceed in parallel.
When reading `ChannelMonitor`s from a `MonitorUpdatingPersister` on startup, we have to make sure to load any `ChannelMonitorUpdate`s and re-apply them as well. For users of async persistence who don't have any `ChannelMonitorUpdate`s (e.g. because they set `maximum_pending_updates` to 0 or, in the future, we avoid persisting updates for small `ChannelMonitor`s), this means two round-trips to the storage backend, one to load the `ChannelMonitor` and one to try to read the next `ChannelMonitorUpdate` only to have it fail. Instead, here, we use `KVStore::list` to fetch the list of stored `ChannelMonitorUpdate`s, which for async `KVStore` users allows us to parallelize the list of update fetching and the `ChannelMonitor` loading itself. Then we know exactly when to stop reading `ChannelMonitorUpdate`s, including reading none if there are none to read. This also avoids relying on `KVStore::read` correctly returning `NotFound` in order to correctly discover when to stop reading `ChannelMonitorUpdate`s.
When reading `ChannelMonitor`s from a `MonitorUpdatingPersister` on startup, we have to make sure to load any `ChannelMonitorUpdate`s and re-apply them as well. Now that we know which `ChannelMonitorUpdate`s to load from `list`ing the entries from the `KVStore` we can parallelize the reads themselves, which we do here. Now, loading all `ChannelMonitor`s from an async `KVStore` requires only three full RTTs - one to list the set of `ChannelMonitor`s, one to both fetch the `ChanelMonitor` and list the set of `ChannelMonitorUpdate`s, and one to fetch all the `ChannelMonitorUpdate`s (with the last one skipped when there are no `ChannelMonitorUpdate`s to read).
…ogging Add monitor_updating_paused logging
fuzz: support initial async monitor persistence in chanmon_consistency
Adds a new constructor for blinded paths that allows specifying the number of dummy hops. This enables users to insert arbitrary hops before the real destination, enhancing privacy by making it harder to infer the sender–receiver distance or identify the final destination. Lays the groundwork for future use of dummy hops in blinded path construction.
Upcoming commits will need the ability to specify whether a blinded path contains dummy hops. This change adds that support to the testing framework ahead of time, so later tests can express dummy-hop scenarios explicitly.
Routes `fn connect_outbound` through Tor. This uses a unique stream isolation parameter for each connection: the hex-encoding of 32 random bytes sourced from the `entropy_source` parameter.
Introduce Dummy Hop support for Blinded Payment Path
Some splicing use cases require to simultaneously splice in and out in the same splice transaction. Add support for such splices using the funding inputs to pay the appropriate fees just like the splice-in case, opposed to using the channel value like the splice-out case. This requires using the contributed input value when checking if the inputs are sufficient to cover fees, not the net contributed value. The latter may be negative in the net splice-out case.
…tbound net-tokio: add `fn socks5_connect_outbound`
…-and-out Mixed mode splicing
…-holding-cells Free holding cells immediately rather than in message sending
We expect callers of `handle_post_event_actions` to hold a read lock on `total_consistency_lock`, and found that we forgot it in `process_pending_events` until recently. Here we add a relevant assertion to avoid such issues in the future.
…ons-lock-assert Add `total_consistency_lock` check in `handle_post_event_actions`
Replace the separate `p_id: u8` (for payment hash generation) and `p_idx: u64` (for payment IDs) with a single `p_ctr: u64` counter. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-Authored-By: Matt Corallo <git@bluematt.me>
Change `get_payment_secret_hash` to return `(PaymentSecret, PaymentHash)` directly instead of `Option<...>`. The function calls `create_inbound_payment_for_hash` with `min_value_msat=None` and `min_final_cltv_expiry=None`, which cannot fail. Remove the retry loop and use `.expect()` since the call will always succeed with these parameters. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-Authored-By: Matt Corallo <git@bluematt.me>
Rename `middle_chan_id` to `middle_scid` and `dest_chan_id` to `dest_scid` in `send_hop_noret` and `send_hop_payment`. These parameters are short channel IDs (SCIDs), not channel IDs, so the rename improves clarity. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-Authored-By: Matt Corallo <git@bluematt.me>
Add transaction tracking to TestBroadcaster and verify no unexpected broadcasts occur during normal fuzzing operation: - Store all broadcast transactions in TestBroadcaster - Clear the broadcast set after initial channel opens - Assert in test_return! that no transactions were broadcast Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace the standalone `send_noret` and `send_hop_noret` functions with closures defined inside the main loop. This allows them to capture the `nodes` array and use simple node indices (0, 1, 2) instead of passing `&nodes[x]` references at each call site. The `send_payment` and `send_hop_payment` functions are modified to take pre-computed `PaymentSecret`, `PaymentHash`, and `PaymentId` parameters, with the closures handling the credential generation. This centralizes where `PaymentId` is constructed, which will make it easier to add payment tracking in a future change. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-Authored-By: Matt Corallo <git@bluematt.me>
Track payment lifecycle by maintaining pending_payments and resolved_payments arrays per node: - When sending a payment, add its PaymentId to pending_payments - On PaymentSent/PaymentFailed events, move the PaymentId from pending to resolved (or assert it was already resolved for duplicate events) This tracking enables verifying payment state consistency after node restarts. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-Authored-By: Matt Corallo <git@bluematt.me>
In the last few days there was incompatibility of `cargo-semver-checks` with the new stable Rust 1.93.0. While this should fixed by today's release of `cargo-semver-checks`, we take the opportunity to drop an unnecessary install step from the CI workflow, as the action will bring their own Rust version if not configured otherwise.
…-checks-on-msrv Drop unnecessary Rust install in SemVer CI
Electrum's `blockchain.scripthash.get_history` will return the *confirmed* history for any scripthash, but will then also append any matching entries from the mempool, with respective `height` fields set to 0 or -1 (depending on whether all inputs are confirmed or not). Unfortunately we previously only included a filter for confirmed `get_history` entries in the watched output case, and forgot to add such a check also when checking for watched transactions. This would have us treat the entry as confirmed, then failing on the `get_merkle` step which of course couldn't prove block inclusion. Here we simply fix this omission and skip entries that are still unconfirmed (e.g., unconfirmed funding transactions from 0conf channels). Signed-off-by: Elias Rohrer <dev@tnull.de>
…ents fuzz: chanmon_consistency payment tracking and assertions
…mprovements `ElectrumSyncClient`: Skip unconfirmed `get_history` entries
…invoice-payment_hash-return-type Change Bolt11Invoice payment_hash function return type
BOLT12 offers are designed for compactness to fit in QR codes, and can be "much longer-lived than a particular invoice". These goals are in tension when constructing blinded paths: BOLT04 allows using either `short_channel_id` or `next_node_id` in the encrypted_data_tlv for routing, but each has trade-offs. Using SCIDs produces smaller encoded offers suitable for QR codes, but if the referenced channel closes or is modified (e.g., via splicing), the blinded path breaks and the offer becomes unusable. Using full node IDs produces larger offers but paths remain valid as long as the node connection persists, regardless of specific channel lifecycle. For long-lived offers where stability matters more than size, users may prefer full node ID paths. This adds a `compact_paths` field to `DefaultMessageRouter` and a `with_compact_paths` constructor to allow downstream applications to choose their preferred trade-off. The default behavior (compact paths enabled) is preserved.
Add the `Nonce` from the BOLT 12 `OffersContext` to `Event::PaymentSent` so downstream consumers can build payer proofs without needing to set `manually_handle_bolt12_invoices` and intercept `InvoiceReceived`. The nonce is extracted from `OffersContext::OutboundPaymentForOffer` or `OffersContext::OutboundPaymentForRefund` when the invoice is received, threaded through `HTLCSource::OutboundRoute` (persisted for restart safety), stored in `PendingOutboundPayment::Retryable` (available for retries), and finally included in the `PaymentSent` event. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
ops wrong command! sorry |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
payment_nonce: Option<Nonce>field toEvent::PaymentSentso downstream consumers (e.g. ldk-node) can construct payer proofs without interceptingInvoiceReceivedOffersContext::OutboundPaymentForOffer/OutboundPaymentForRefundwhen the BOLT 12 invoice is receivedHTLCSource::OutboundRoute,PendingOutboundPayment::Retryable, andSendAlongPathArgsso it survives restarts and retriesThis removes the need for ldk-node to set
manually_handle_bolt12_invoices = trueand manually callsend_payment_for_bolt12_invoicejust to capture the nonce, as was done in lightningdevkit/ldk-node#845.Test plan
cargo checkpassescargo check --testspassesoutbound_payment::tests(12/12 pass)ln::payment_tests(51/51 pass)ln::offers_tests(37/37 pass)ln::channelmanager::tests(17/17 pass)🤖 Generated with Claude Code