[0.1] Initial round of backports for 0.1.10#4680
Conversation
|
I've assigned @joostjager as a reviewer! |
|
I've now examined the portions of the diff that were truncated in my prior pass — the
No issues found. All substantive changes in this backport set are correct. Verified during this pass:
Cross-cutting (operational, not a code bug, unchanged from prior note): |
joostjager
left a comment
There was a problem hiding this comment.
I've assumed all conflicts were trivial aside from the first commit "Remove direct calls to handle_monitor_update_completion!". That commit, even though it is small, seems to have quite a few subtleties. Might want to ask one of the original reviewers of #3947 for a final look.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
`handle_monitor_update_completion!` (and `handle_monitor_update_completion_actions`) are a pretty annoying API as they have several preconditions. Luckily, we already have `channel_monitor_update` which does the right work, handling both the open- and closed- channel cases and correctly checking the preconditions to `handle_monitor_update_completion!`, so here we convert calls to `handle_monitor_update_completion!` (aside from `handle_new_monitor_update!`) to just calling `channelMonitor_update`. Backport of 8b9b078 Nontrivial conflicts resolved in: * lightning/src/ln/channelmanager.rs due to the tracking of in-flight monitor updates having moved from `OutPoint`s to `ChannelId`s in 0.2. Also note that 8f4a4d2 updated some code which was introduced in this commit and had already previously been backported to 0.1.
We previously assumed background events would eventually be processed prior to another `ChannelManager` write, so we would immediately remove all in-flight monitor updates that completed since the last `ChannelManager` serialization. This isn't always the case, so we now keep them all around until we're ready to handle them, i.e., when `process_background_events` is called. This was discovered while fuzzing `chanmon_consistency_target` on the main branch with some changes that allow it to connect blocks. It was triggered by reloading the `ChannelManager` after a monitor update completion for an outgoing HTLC, calling `ChannelManager::best_block_updated`, and reloading the `ChannelManager` once again. A test is included that provides a minimal reproduction of this case. Backport of 7e84268 Conflicts resolved in: * lightning/src/ln/channelmanager.rs * lightning/src/ln/reload_tests.rs
Backport of f128b85 Conflicts resolved in: * lightning/src/ln/channelmanager.rs
When we make an MPP claim we push RAA blockers for each chanel to ensure we don't allow any single channel to make too much progress until all channels have the preimage durably on disk. We don't have to store those RAA blockers on disk in the ChannelManager as there's no point - if the ChannelManager gets to disk with the RAA blockers it also brought with it the pending ChannelMonitorUpdates that contain the preimages and will now be replayed, ensuring the preimage makes it to all ChannelMonitors. However, just because those RAA blockers dissapear on reload doesn't mean the implications of them does too - if a later ChannelMonitorUpdate was blocked in the channel we don't have logic to unblock it on startup. Here we add such logic, simply attempting to unblock all blocked `ChannelMonitorUpdate`s that existed on startup. Code written by Claude. Fixes lightningdevkit#4518 Backport of b0c312d Conflicts resolved in: * lightning/src/ln/channelmanager.rs
Add a characterization test for a claimed MPP payment whose preimage monitor updates are only partially persisted before restart. The test drives both channels through a held fee-update commitment dance, claims with async monitor persistence, reloads one fresh and one stale monitor, and verifies that we don't leave a sender-side HTLC stuck after reconnect. Backport of 01d55dc Conflicts due to different channel sorting and holding cell free timing resolved in: * lightning/src/ln/reload_tests.rs
In a very specific case, forgetting to do so can lead to a debug assertion failure when we see a double-claim of an HTLC (see the included test). Found by @joostjager's work on growing the chanmon_consistency fuzzer. Backport of f14b4b2 Silent conflicts resolved in: * lightning/src/ln/functional_tests.rs
Backport of 6bf2352 Conflicts resolved in: * lightning/src/chain/chainmonitor.rs
Incomplete keysend MPPs skipped the receive timeout path, allowing partial payments to hold HTLC slots until CLTV expiry instead of failing after `MPP_TIMEOUT_TICKS`. Apply the existing `total_mpp_amount_msat` completeness check to all MPP receives and add a regression test covering the keysend case. The timeout logic was originally added only for invoice-backed MPPs in 2022, and that invoice-only guard remained when receive-side MPP keysend support landed in 2023, leaving this gap latent until now. Co-Authored-By: HAL 9000 Backport of fd8846b Conflicts resolved in: * lightning/src/ln/channelmanager.rs * lightning/src/ln/payment_tests.rs
`GetHistoryRes::height` from electrum-client is a *signed* integer. Here we first check for `<= 0` *before* casting to `u32`. Signed-off-by: Elias Rohrer <dev@tnull.de> Backport of 8b383bb
`OMDomainResolver` rate-limits in-flight DNSSEC proof builds via a `pending_query_count` counter capped at `MAX_PENDING_RESPONSES` (1024). The counter was only released when the proof build succeeded, so any failure mode -- NXDOMAIN, insecure zones, unreachable resolvers, I/O timeouts, malformed names -- permanently consumed a slot. Because the queried name is attacker-controlled (it travels in over a `DNSSECQuery` onion message from any LN peer, given DNS resolution is an opt-in network-advertised feature), an adversary could exhaust the counter with ~1025 failing queries and persistently DoS the resolver for any subsequent legitimate BIP-353 lookups, until the process is restarted. Always release the slot once the proof build completes, regardless of outcome, and add a regression test which points the resolver at a TCP-refusing local port and asserts the counter returns to zero. Co-Authored-By: HAL 9000 Backport of fb4103d Conflicts resolved in: * lightning-dns-resolver/src/lib.rs
`PrintableString` is the sanitiser LDK uses to render untrusted strings
(node aliases, BOLT-12 invoice / offer text, `UntrustedString`, LSPS
messages, `lightning-invoice` descriptions) to logs and UI. It only
replaced `char::is_control` matches (Unicode general category `Cc`)
with U+FFFD, leaving the entire `Cf` (Format) category untouched.
That is the exact category covering the bidirectional override /
isolate codepoints (U+202A..U+202E, U+2066..U+2069) and zero-width
characters (U+200B..U+200D, U+FEFF) behind the "Trojan Source" attack
family (CVE-2021-42574): a peer can set its alias / invoice description
/ offer fields to e.g. `safe\u{202E}cipsxe.exe`, which previously
passed through verbatim while a human reader sees `safeexe.cips` —
defeating the threat model `PrintableString` exists to defend against.
Replace `Cf` codepoints alongside `Cc` ones. The `Cf` ranges are
inlined as a `matches!` table sourced from Unicode 16.0 to keep the
change `no_std`-friendly with no new dependencies.
Co-Authored-By: HAL 9000
Signed-off-by: Elias Rohrer <dev@tnull.de>
Backport of 1a01b5a
1a01b5a added detection of unicode format characters in `PrintableString`, but used a hard-coded table which may eventually become out of date. Here we switch to an auto-generated table, include all `General_Category` `Other` characters, and also ban unallocated code points. Finally, CI validates that the file is kept up to date. Written by Claude Backport of 65e8cc8
`composite_custom_message_handler!` expanded `peer_connected` to call every sub-handler and remember the last error, but never undo the already-succeeded ones. The `CustomMessageHandler::peer_connected` contract is that `PeerManager` will *not* invoke `peer_disconnected` when `peer_connected` returns `Err` — so any per-peer state allocated by an earlier sub-handler that returned `Ok` was leaked permanently once a later sub-handler returned `Err`. A peer who can elicit `Err` from any sub-handler in the composite (feature-bit gate, banlist, etc.) could repeatedly reconnect to grow that leaked state without bound (slow resource DoS), and "currently connected" predicates in the leaking sub-handler would lie about peers that were actually rejected. Mirror the rollback pattern `PeerManager` already uses for the four built-in handlers (`peer_handler.rs:2149-2188`): record each sub-handler's `peer_connected` result, and if any returned `Err`, call `peer_disconnected` on the ones that succeeded before propagating the failure. Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de> Backport of 5455058 Silent conflicts resolved in: * lightning-custom-message/src/lib.rs
`EsploraSyncClient::get_confirmed_tx` parsed the SPV proof returned by
the Esplora server but threw away the security check: the merkle root
computed by `PartialMerkleTree::extract_matches` was discarded
(`let _ = …`), and only the leaf-equality check (`matches[0] == txid`)
remained. Anyone can construct a single-leaf partial tree advertising
an arbitrary txid via `PartialMerkleTree::from_txids(&[txid], &[true])`,
so this gate was vacuous.
A malicious or compromised Esplora server could therefore convince
`EsploraSyncClient` that any transaction was confirmed in any block by
returning `MerkleBlock { header: real_header, txn: forged_partial_tree }`,
causing LDK to feed a synthesized `ConfirmedTx` into `Confirm`
implementations such as `ChannelManager` / `ChainMonitor`. From there,
the channel-funding / closing / HTLC flows would treat the transaction
as confirmed at an attacker-chosen height, with consequences ranging
from premature state transitions to force-close races.
Capture the merkle root returned by `extract_matches` and require it
to equal `block_header.merkle_root`, matching the validation the
Electrum sibling already performs via `validate_merkle_proof`.
Co-Authored-By: HAL 9000
Backport of b64efcd
The exhaustive filesystem store listing treated leftover temp and trash files as namespace directories after identifying them as non-keys. Skip those artifacts before recursing so migrations can ignore crash leftovers. Backport of 9246d86 Conflicts resolved due to moved/split file in: * lightning-persister/src/fs_store.rs
lnd is preparing to ship a release with opt-in onion messages without support for forwarding onion messages from non-channel peers. This breaks the common BOLT 12 OM flow today where we direct-connect to the blinded path introduction point and send the `invoice_request` without a channel. For CLN it turns out this is fine as they never select a peer for their introduction point at all. However, for LDK this would break existing nodes as nodes might now pick an lnd peer as an introduction node but it won't forward the onion message. For now, we just drop the separate introduction point selection and just always use ourselves as an introduction point (assuming we're an announced node). This should also have the side-effect of making offers marginally more robust, which may be worth it, even if it sucks to drop any pretense of privacy. Backport of 8c08a30 Conflicts resolved in: * lightning/src/ln/offers_tests.rs * lightning/src/onion_message/messenger.rs
If an outbound payment was abandoned with htlcs in-flight and later claimed, we would previously have the PaymentSent::fee_paid_msat be set to None. This contradicted some docs on the event that stated the field would always be Some after 0.0.103. Backport of 3e9e6e9 Conflicts resolved in: * lightning/src/ln/functional_tests.rs * lightning/src/ln/outbound_payment.rs * lightning/src/ln/payment_tests.rs
9366866 to
e60acde
Compare
There will obviously be a chunk more to go, but this is the first chunk of things already merged. Includes #3947 (as a prereq for #4377), #4377, #4520, #4537, #4544, #4558, #4590, #4591, #4593, #4605, #4595, #4596, #4618, #4647, and #4651.