BPT-868: Rebase 0.2_fork onto upstream v0.2.3#2
Closed
saidalaoui-sq wants to merge 92 commits into
Closed
Conversation
Discourage people from running Knots with LDK
…ots-settings Update CHANGELOG for 0.2
`SocketDescriptor::send_data` semantics were changed in 0.2, without changing the method signature. As such the release notes really should be explicit about this.
…send-data-relnotes Note `SocketDescriptor::send_data` semantics changes in renotes
`AttributionData` is a part of the public `UpdateFulfillHTLC` and `UpdateFailHTLC` messages, but its not actually `pub`. Yet again re-exports bite us and leave us with a broken public API - we ended up accidentally sealing `AttributionData`. Instead, here, we just make `onion_utils` `pub` so that we avoid making the same mistake in the future. Note that this still leaves us with arather useless public `AttributionData` API - it can't be created, updated, or decoded, it can only be serialized and deserialized, but at least it exists. Backport of bd57823 Conflicts resolved in: * lightning/src/ln/onion_utils.rs semver-breaking `pub use` removal dropped in: * lightning/src/ln/mod.rs
Backport of 6578b88
The next backport commit requires this and it was done upstream in 173481f, which we partially backport here.
In 20877b3 we added a `debug_assert`ion to validate that if we call `maybe_free_holding_cell_htlcs` and it doesn't manage to generate a new commitment (implying `!can_generate_new_commitment()`) that we don't have any HTLCs to fail, but there was no reason for that, and its reachable. Here we simply remove the spurious debug assertion and add a test that exercises it. Backport of b524b9b
Previously, `lightning-background-processor`'s `Selector` would poll all other futures *before* finally polling the sleeper and returning the `exit` flag if it's ready. This could lead to scenarios where we infinitely keep processing background events and never respect the `exit` flag, as long as any of other futures keep being ready. Here, we instead bias the `Selector` to always *first* poll the sleeper future, and hence have us act on the `exit` flag immediately if is set. Backport of 9c0ca26
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> Backport of cc1eb16
In `ChannelMonitor` logging, we often wrap a logger with `WithChannelMonitor` to automatically include metadata in our structured logging. That's great, except having too many logger wrapping types flying around makes for less compatibility if we have methods that want to require a wrapped-logger. Here we change the `WithChannelMonitor` "constructors" to actually return a `WithContext` instead, making things more consistent. Backport of 0f253c0
In much of LDK we pass around `Logger` objects both to avoid having to `Clone` `Logger` `Deref`s (soon to only be `Logger`s) and to allow us to set context with a wrapper such that any log calls on that wrapper get additional useful metadata in them. Sadly, when we added a `Logger` type to `OutboundPayments` we broke the ability to do the second thing - payment information logged directly or indirectly via logic in the `OutboundPayments` has no context making log-searching rather challenging. Here we fix this by retunring to passing loggers explicitly to `OutboundPayments` methods that need them, specifically requiring `WithContext` wrappers to ensure the callsite sets appropriate context on the logger. Fixes lightningdevkit#4307 Backport of 5e64c40 Conflicts resolved in: * lightning/src/ln/channelmanager.rs
This is really dumb, `assert!(cfg!(fuzzing))` is a perfectly reasonable thing to write! Backport of 6ff720b
[0.2] Backports and cut 0.2.1
…1-date Correct relase date for 0.2.1
Backport of 60b5d66 Conflicts resolved in: * lightning/src/ln/chanmon_update_fail_tests.rs
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
When we shipped 0.2 we used the feature bit 155 to signal splicing, in line with what eclair was using. However, eclair was actually using that bit to signal splicing on a previous design which is incompatible with the current spec. The result of this was that eclair nodes may attempt to splice using their protocol and we'd fail to deserialize their splice message (resulting in a reconnect, which luckily would clear their splice attempt and return the connection to normal). As we really need to get off of their feature bit and there's not much reason to keep using a non-final-spec bit, we simply redefine `SplicePrototype` to bit 63 here. Backport of 98c3cff
In debug mode, using SignedAmount::abs can lead to an integer overflow when used with SignedAmount::MIN. Use SignedAmount::unsigned_abs to avoid this. Backport of 2d948fd Conflicts resolved in: * lightning/src/ln/channel.rs
Per the spec clarification in lightning/bolts#1316: - Writers MUST set offer_amount greater than zero when present - Readers MUST NOT respond to offers where offer_amount is zero Reject amount_msats(0) in the builder with InvalidAmount, and reject parsed offers with amount=0 (with or without currency) during TLV deserialization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Backport of a06c446 Conflicts resolved in: * lightning/src/offers/offer.rs Compared to the upstream commit, this instead allows downstream code to pass a 0 amount but converts it to no-amount to ensure upgraded readers of the built offer will accept it. This avoids changing the API in a backport.
…-0.2 [0.2] Reject offer_amount of 0 as invalid per BOLT 12
Previously, we logged "Persisting LiquidityManager..." on each background processor wakeup, which can be very spammy, even on TRACE level. Here, we opt to only log if something actually needed to be repersisted and we did so (in case of failure we're logging that anyways, too). Backport of 369ea98
The `chanmon_consistency` fuzz target found a reconnect ordering where `signer_pending_revoke_and_ack` and `monitor_pending_revoke_and_ack` could both describe the same owed `revoke_and_ack`. The channel first received a `commitment_signed` whose monitor update completed, but the signer could not provide the next point or secret, leaving `signer_pending_revoke_and_ack` set. Later, receiving the peer `revoke_and_ack` freed holding-cell HTLCs and produced a held monitor update. While that monitor update was still blocked, `channel_reestablish` saw the peer one state behind and recorded `monitor_pending_revoke_and_ack`, plus the corresponding monitor-pending `commitment_signed`, so the messages could be replayed once monitor updating was restored. If the signer unblocked before the held monitor update was released, `signer_maybe_unblocked` generated and sent the already monitor-safe RAA using `signer_pending_revoke_and_ack`. The monitor-pending flag was not cleared at that point, so `monitor_updating_restored` later generated the same RAA again when the held update completed. The peer had already advanced after accepting the signer-unblocked RAA, so it rejected the duplicate secret as not corresponding to its current pubkey and force-closed. Fix this by clearing `monitor_pending_revoke_and_ack` in the signer-resume path only once a signer-pending RAA is actually being returned. Backport of 27223fd Silent conflicts resolved in: * lightning/src/ln/async_signer_tests.rs
LSPS5 webhook signatures are zbase32 strings, and the verifier accepts case aliases when decoding them. The replay cache compared raw header strings, so a case-only change could bypass immediate replay detection even though it represented the same signature bytes. Canonicalize the verified signature text before cache lookup and storage. Keying the replay cache on decoded signature bytes would be the semantic ideal, but doing that locally would decode once in the validator and again inside message_signing::verify. This keeps the fix local while matching the verifier's identity semantics. Add regression coverage for the case-varied replay. Backport of 3c128ed
Replayed intercepted HTLC events should not duplicate queued payments or panic after restart. Ignore already-queued intercept IDs so persisted queues remain stable across event replay. Co-Authored-By: HAL 9000 Backport of 6997c88
Persisting LSPS2 service state can race with replayed intercepted HTLC events after restart. Cover replaying the same intercepted HTLC after restoring peer state so duplicate queueing is caught. Co-Authored-By: HAL 9000 Backport of 68e71c2 Trivial conflict resolved in: * lightning-liquidity/src/lsps2/service.rs
If we have a high (200%+) proportional fee as an intermediate blinded node combined with a low inbound amount, we previously had some code that calculated the outbound amount of the forward that would've underflowed. This would've caused a panic in debug builds and caused us to relay a payment that should've been rejected (due to being unable to cover our high fee) in release builds. Reported by Project Loupe. Backport of e560ec1
Clarify the return value so callers know it reports whether the forced peer-state write reached the store. Co-Authored-By: HAL 9000 Backport of 12815f3
When a prunable peer gains state before removal, persist() now reports that the forced peer-state write reached the store. Co-Authored-By: HAL 9000 Backport of a1cda95
When a prunable client gains state before removal, persist() now reports that the forced peer-state write reached the store. Co-Authored-By: HAL 9000 Backport of d757191
If a caller of send_payment_with_route provided a route with either no paths, or where the first path had 0 hops, the method would panic due to attempting to unwrap a dummy pubkey that was initialized with 32 bytes instead of the required 33. Reported by Project Loupe. Backport of 54cdd85
A handler built with `composite_custom_message_handler!` routes an incoming message type to the sub-handler whose pattern matches it and assumed the sub-handler would always decode it. But per the `CustomMessageReader` contract a sub-handler returns `Ok(None)` for a type it doesn't recognize, and a sub-handler's pattern -- a range in particular -- can be broader than the types it actually decodes. Since the message type comes from peer input, this let a remote peer panic the message-processing thread with a single custom message whose type falls in a sub-handler's pattern but isn't decoded by it. Report such a message as unknown instead, matching how `wire::do_read` handles an undecoded custom message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Backport of 77ac339
Drain queued intercepted HTLCs before removing pending LSPS2 JIT channel state in channel_open_abandoned. Add a real interception regression test that verifies the held HTLC is no longer pending after the abandon call. Backport of ccc8b55 Silent conflicts resolved in: * lightning-liquidity/tests/lsps2_integration_tests.rs
…backports-2 [0.2] Another round of backports + initial release notes
Payment parameters should use the canonical payee key from BOLT11 invoices. When an invoice includes an n field, using that key avoids attempting signature recovery that may legitimately be unavailable. Co-Authored-By: HAL 9000 This finding was discovered by Project Loupe Backport of 06393eb Conflicts resolved in: * lightning/src/routing/router.rs
Crafted route hints can overflow aggregate downstream proportional fees when the payer disables the routing fee cap. Treat such paths as unusable so route finding fails cleanly instead of panicking. Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de> Backport of beffe75
Counterparty-provided strings in network messages (Error, Warning, TxAbort) were logged without length limits, allowing a malicious peer to bloat log files. Some logging sites also lacked the same sanitization used for other untrusted strings. Add a `DebugMsg` struct and `log_msg!` macro that consistently truncate messages to 512 characters while preserving `PrintableString` sanitization. Replace all bare `msg.data` and ad hoc `PrintableString(&msg.data)` usages at the 7 relevant logging sites in `peer_handler.rs` and `channel.rs`. Co-Authored-By: HAL 9000 Backport of e2f611e Conflicts resolved in: * lightning/src/ln/peer_handler.rs
If an RGS server sends snapshots that are absurdly-sized, they can bloat a client's network graph, eventually leading to an OOM. While we generally consider RGS servers to be semi-trusted (at least in the sense that they can often simply not respond and leave a client unable to find paths) we should still avoid allowing them to OOM a client. Thus, here, we naively start ignoring new channels from an RGS server if they leave our graph 10x larger than we expect. This at least avoids the OOM even if we end up not being able to make payments. Reported by Jordan Mecom of Block's Security Team Backport of 7a89362 Conflicts resolved in: * lightning/src/routing/gossip.rs
Luckily this was only used in `ChannelManager` and scorer deserialization, though we anticipate occasionally fetching the second from an only semi-trusted source. Backport of 5b4626f
Rust's panicy string slicing behavior has always been a sharp edge and here it finally caught up with us. Ensure we don't slice into a string provided in an onion message until we're sure the index is a character boundary. Reported by Jordan Mecom of Block's Security Team Backport of ae852b5
It turns out that conditionally-enabling a dependency via `target` in `Cargo.toml` does not enable the corresponding dependency `feature` when compiling the code. As a result, only when building `possiblyrandom` with an explicit `getrandom` feature did we ever actually return random values. This fixes this by matching the `target` cfg in `Cargo.toml` to the cfg in `lib.rs`. Reported by Project Loupe Backport of b7c9935
Cut 0.2.3/dns-resolver 0.3.1/invoice 0.34.1/types 0.3.2
`LSPSDateTime::is_past` coerced `chrono`'s `i64` timestamp into a `u64` via `try_into().expect(...)`. Because `LSPSDateTime` is parsed from peer-controlled RFC 3339 strings (which can be pre-1970 and so yield negative timestamps), this could be triggered remotely: an attacker-supplied `valid_until` / `expires_at` field of e.g. `"1900-01-01T00:00:00Z"` would parse successfully, land in LSPS state before any HMAC / promise check, and panic the LSP thread on the next `prune_pending_requests` sweep. Concretely reachable today via LSPS2 `opening_fee_params.valid_until` (in the buy request) and the LSPS1 expiry fields. Make `LSPSDateTime::from_str` reject pre-epoch datetimes, and route serde deserialization through it: `#[serde(transparent)]` was delegating Deserialize directly to `chrono`'s impl and bypassing our parser, so peer JSON had to be guarded separately. With both paths funnelled through one parser, no `LSPSDateTime` value with a negative inner timestamp can be constructed and `is_past` is safe by construction. Co-Authored-By: HAL 9000 Backport of 837763a Conflicts resolved in: * lightning-liquidity/src/lsps0/ser.rs
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
14 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
Verification
Linear: BPT-868