Rebase fork onto rust-lightning v0.2.5 - #28
Draft
amackillop wants to merge 147 commits into
Draft
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
When an HTLC is intercepted while the peer is offline, the store previously persisted to S3 before inserting into the in-memory HashMap. This created a window where peer_connected() could run, read an empty HashMap, and find 0 HTLCs to forward -- even though the HTLC was being stored at that exact moment. Two fixes: - htlc_store: Insert into HashMap before S3 persist (with rollback on failure) so the HTLC is immediately visible to other threads. - service: After storing, re-check if the peer reconnected during the persist window. If so, process HTLCs immediately rather than relying on the webhook path that already missed them.
Production logs show 16 instances over 7 days where peer_connected() fires and immediately tries to forward stored HTLCs, but the HTLC silently fails because the channel hasn't completed ChannelReestablish yet. The root cause: forward_intercepted_htlc() checks is_usable() (channel confirmed, not shutting down) which passes. But later, send_htlc() checks is_peer_disconnected() which fails because clear_peer_disconnected only happens after channel_reestablish completes - which is AFTER peer_connected fires. Timeline of the bug: 1. peer_connected fires (TCP/noise handshake complete) 2. LSPS4 calls forward_intercepted_htlc() on channel manager 3. forward_intercepted_htlc checks is_usable() - passes, returns Ok(()) 4. HTLC is queued in forward_htlcs map 5. process_pending_htlc_forwards -> send_htlc() checks is_peer_disconnected() - FAILS 6. HTLC is failed back to payer with no LSPS4-level error logged This commit adds logging at every step of this pipeline to confirm the theory and quantify the problem: LSPS4 service (lightning-liquidity): - peer_connected: log is_usable/is_channel_ready for all channels - peer_disconnected: log orphaned HTLCs left in store - channel_ready: log channel state and pending HTLCs - htlc_intercepted: log connected_peers set size and channel usability - execute_htlc_actions: log PRE-FORWARD channel state, forward result - calculate_htlc_actions: log is_usable per channel - handle_expired_htlcs: log details of expired HTLCs LDK core (lightning): - forward_intercepted_htlc: log is_usable vs is_live vs is_connected, emit WARNING when is_usable && !is_live (the race condition) - channel_reestablish: log when clear_peer_disconnected() fires - send_htlc: log when HTLC is rejected due to peer_disconnected - process_forward_htlcs: log channel liveness before queue_add_htlc Grafana queries to confirm the bug after deploying: - "forward_intercepted_htlc.*is_usable: true.*is_live: false" - "send_htlc: REJECTING HTLC.*peer_disconnected" - "peer_connected.*is_usable: false"
The 10s threshold was too tight for the webhook delivery pipeline, causing premature HTLC expiry and RetriesExhausted failures on 402 payments. 45s gives ample room for webhook round-trips while still well within CLTV delta bounds.
LDK fires peer_connected after the TCP+Init handshake but before channel_reestablish completes. During this window, channels exist but are not yet usable (is_usable=false). The previous code forwarded HTLCs unconditionally in peer_connected and htlc_intercepted, causing ~10% of payments to fail on reconnect. Distinguish two connected-peer cases in htlc_intercepted and peer_connected: - Channels exist but none usable (reestablish in progress): defer the HTLC for timer-based retry via process_pending_htlcs(). - No channels exist (first payment, JIT open needed): proceed immediately so calculate_htlc_actions_for_peer can emit the OpenChannel event. process_pending_htlcs (5s timer) only retries the reestablish case (channels exist, waiting to become usable). It must not handle the no-channel case to avoid emitting duplicate OpenChannel events while a JIT open is already in flight. Remove the re-check race in htlc_intercepted that could fire concurrently with peer_connected. The webhook + peer_connected path is the single owner of the offline-peer reconnect flow. Blocking inside peer_connected was also considered but rejected: there is no LDK event for "channel usable after reconnect" to wake on, so it would require a spin-wait with arbitrary timeout. A timer-based retry is cleaner and avoids holding the lock.
process_pending_htlcs and htlc_intercepted both call calculate_htlc_actions_for_peer independently. When both observe insufficient capacity on an existing channel, both emit OpenChannel — opening two JIT channels for one payment. The timer's role is to retry deferred forwards through channels that became usable after reestablish, not to open new channels. Restrict it to forwarding only: if the action set contains a new channel request, skip and let htlc_intercepted, peer_connected, or channel_ready handle it. A second race hid behind non-usable channels appearing in the capacity map. A still-opening channel advertised high outbound_capacity, so the HTLC was routed into it. The forward failed (channel still opening), consuming the InterceptId. channel_ready fired moments later but found an empty store — the payment hung until the payer retried. Filter the capacity map to usable channels only. A third window exists between the usability check and the actual forward_intercepted_htlc call: the peer can disconnect in between, wasting the InterceptId on a doomed forward. Add a peer liveness check immediately before forwarding so the HTLC survives for retry on the next timer tick. The residual sub-millisecond window is acceptable; plumbing connection state into the channel manager's forward path would be far more invasive for marginal gain.
…blish When an HTLC arrives for an offline peer, htlc_intercepted stores it and sends a webhook. When the peer reconnects, peer_connected deferred all processing if channels existed but weren't usable yet (reestablish in progress). Later, process_pending_htlcs found insufficient capacity but assumed a channel open was already in flight - nobody ever opened the channel. Fix: call process_htlcs_for_peer even when channels aren't usable. calculate_htlc_actions skips non-usable channels, so if existing capacity is insufficient it returns new_channel_needed_msat and execute_htlc_actions emits OpenChannel. No premature forwarding occurs since the forwards list is empty (no usable channels). The actual HTLC forwards happen via channel_ready once the new channel is established.
Instead of always opening new channels when outbound capacity is insufficient, prefer splicing into the largest existing usable channel. Falls back to new channel open when no usable channels exist. Key changes: - Add SpliceChannel event variant to LSPS4ServiceEvent - Add per-peer liquidity_cooldown to prevent 1Hz retry loops - Modify calculate_htlc_actions_for_peer to prefer splice over new channel - Add cooldown checks in execute_htlc_actions and process_pending_htlcs - Clear cooldown on channel_ready (covers both splice-lock and new channel) Use is_channel_ready instead of is_usable when selecting splice candidates. This ensures we prefer splice over new channel even during channel_reestablish (~1s window). splice_channel() will fail with "pending open/close" if the channel isn't usable yet, and the timer retries once reestablishment completes. --------- Co-authored-by: Martin Saposnic <martinsaposnic@gmail.com>
This reverts commit 6632c59.
When a splice promotes a new FundingScope, `holder_max_htlc_value_in_flight_msat` and `counterparty_max_htlc_value_in_flight_msat` stay pinned at their pre-splice values instead of scaling with the new channel capacity. Any HTLC larger than the pre-splice cap is rejected: - on the sender with HTLCMaximum inside send_htlc - on the receiver with "Remote HTLC add would put them over our max HTLC value", which force-closes the channel Both manifestations break LSPS4 JIT splice flows where the client's embedded node first opens a small JIT channel, then splices in capacity when a larger payment arrives. The receiver force-close is particularly damaging because it destroys the channel along with any pending fees. Rescale both caps proportionally to the channel-value change on splice promotion. Also tightens the test_splice_in assertions (the previous check compared msats to sats).
Channel usability (is_usable) was checked at four separate points: htlc_intercepted, peer_connected, process_pending_htlcs, and calculate_htlc_actions_for_peer. Each had its own deferral logic, and they had to coordinate. Move the usability check to execute_htlc_actions, right before forward_intercepted_htlc. If no usable channel exists, the forward is skipped and the HTLC stays in store for the timer to retry. htlc_intercepted, peer_connected, and process_pending_htlcs now all call process_htlcs_for_peer unconditionally. Change the pre-forward guard from is_peer_connected to has_usable_channel, which covers the disconnect+reconnect race where the peer is connected but the channel has not finished reestablishing.
htlc_intercepted only persisted HTLCs when a liquidity action (splice/open) was needed. This prevents calculate_htlc_actions from returning empty actions (i.e. "do nothing, let the timer handle it") because the HTLC would never make it into the store and the timer would never see it. Move the insert before calculate_htlc_actions_for_peer so every intercepted HTLC is in the store before we decide what to do with it. execute_htlc_actions already removes the HTLC on successful forward, so the store entry is short-lived on the happy path. The cost is two extra KV store round-trips (S3 write + delete) per HTLC on the forward-only path, where previously there were none. This hits every payment since LSPS4 intercepts all of them. Unfortunately unavoidable: the insert must happen before action calculation because we don't know yet whether the result will be "forward now" or "do nothing and defer to the timer." If we wait until after and the result is "do nothing," we've already lost the HTLC. Replace the unconditional unwrap on insert with explicit error handling. On persistence failure: - Forward-only: proceed. If the forward succeeds the store entry was redundant. If persistence failed and the channel becomes unusable between calculate and execute, the HTLC is orphaned (not in store for timer retry, not forwarded) until LDK's CLTV timeout cleans it up. Requires both S3 failure and a channel state change between two adjacent calls. - Liquidity action needed: fail the HTLC back to sender. The splice/open is async and the timer needs the stored HTLC to forward once the channel is ready. Without it, we'd spend on-chain fees on a splice that never results in a forward.
When the LSP intercepts an HTLC for an offline peer, the peer reconnects and peer_connected fires before channel_reestablish completes. All channels report is_usable=false at this point. calculate_htlc_actions_for_peer builds its capacity map from is_usable channels (empty set), finds zero capacity, then falls through to the splice candidate filter. That filter used is_channel_ready, which matches mid-reestablish channels, so it emits a splice. Once reestablish finishes, the HTLC gets forwarded through the existing channel. The splice was wasted, and the 30s liquidity cooldown it sets blocks all subsequent liquidity actions for that peer. Two fixes in calculate_htlc_actions_for_peer: 1. Early return when channels exist but none are usable. The capacity map is empty so any decision would be wrong. The HTLC stays in the store (persisted by the prior commit) and the 1Hz timer retries once channels become usable. 2. Splice candidate filter changed from is_channel_ready to is_usable. A mid-reestablish channel maye already have sufficient capacity but it's just not visible yet. Splicing into it adds unnecessary on-chain cost and a 30s cooldown for capacity that was never actually insufficient. splice_channel() would also fail if the channel does become usable in time.
LiquidityManager gained an 8th generic parameter (Logger) and KVStore targets now require KVStoreSync. Several test files and doctests were not updated after these changes landed. background-processor: The NO_LIQUIDITY_MANAGER const uses a dyn trait object for KVStore, but trait objects can only name one trait. Add a KVStoreFull supertrait combining KVStore and KVStoreSync so the dyn bound remains expressible. Add the Logger generic and KVStoreSync impl to the process_events_async doctest's stub Store type. lightning-liquidity tests: Add missing lsps4_service_config and lsps4_client_config fields (None) to config initializers in lsps0, lsps2, and lsps5 integration tests. Pass logger to LiquidityManagerSync::new calls and add TestLogger to type annotations. channel.rs tests: Remove duplicate import block left over from a bad merge in 736d3a8. Fix field accesses that moved from ChannelContext to FundingScope (channel_value_satoshis, holder_selected_channel_reserve_satoshis). Update TestFeeEstimator construction to use the new constructor API.
On 0-conf channels, splice_locked is sent immediately because the channel's minimum_depth is 0. This is fine when the LSP initiates the splice (it constructed the funding output), but when the counterparty initiates a splice, the LSP trusts a funding output it didn't build. The counterparty can double-spend the splice tx after the LSP has already sent splice_locked. Add ChannelHandshakeConfig::splice_minimum_depth which, when set, overrides the per-FundingScope minimum_depth for splices where is_initiator is false. The override is applied in on_tx_signatures_exchange before the funding is pushed to negotiated_candidates, so check_funding_meets_minimum_depth sees the configured depth instead of the channel's 0-conf minimum. Self-initiated splices are unaffected: the LSP constructed the funding output and the existing 0-conf behavior is correct.
Introduce a Nix dev shell and a justfile so work on this fork has a reproducible toolchain and a one-command local gate, mirroring the setup in the ldk-node repo. The toolchain is pinned to stable 1.90.0 via fenix rather than tracking latest. The version is bounded on both ends: clippy must be at least 1.87 so ci/check-lint.sh can resolve the lint names it allows (older clippy hard-errors on the unknown clippy::manual_is_multiple_of), but 1.92+ adds clippy::assertions_on_constants which fires on existing code in lightning/src/chain/channelmonitor.rs and would break -D warnings on the inherited tree. 1.90.0 sits in that window and matches what CI's stable resolves to today. fenix is used over rust-overlay because it pins an exact channel by hash, so the shell can't drift. just check only compiles and runs the workspace test suite. It deliberately skips clippy and rustfmt: this branch is not clean under either of CI's gates (pre-existing dead code in lsps2/lsps4 trips check-lint.sh's -D warnings, and the tree predates the pinned rustfmt), and reformatting or de-linting code we don't own is out of scope.
Introduce the fee_policy module: a FeePolicy/FeeTier ADT and a single
pure resolve_skim mapping a policy plus an HTLC amount to the msat to
skim. Shared foundation for upcoming work that waives the JIT-channel
skim for grant recipients; nothing wires it up yet.
When the skim would eat the whole HTLC, resolve_skim waives it rather
than clamping to amount - 1: a residual that small may fall below
htlc_minimum_msat and be rejected anyway, so clamping would bank a fee
on a payment that never settles. This only triggers for a rate >= 100%
or an outsized Custom base; the standard 2% never reaches it.
This is also where resolve_skim deliberately diverges from the code it
replaces. The service computed the proportional fee in u64, so
amount * ppm overflowed for very large HTLCs and skimmed nothing,
forwarding the whole HTLC for free. resolve_skim multiplies in u128 and
skims correctly. At the standard 2% the result is identical for any
realistic HTLC; only the previously-overflowing range changes, from
"free" to "charged". Tests pin the standard tier against the legacy
computation for non-overflow sizes and document the large-HTLC case.
The other arms are encoding-only. ZeroFee resolves to zero; Custom
{ ppm, base_msat } adds a flat base to the proportional component.
Custom is kept though v1 never constructs it: it's free in the TLV and
documents the shape of an explicit rate.
Both enums serialize via impl_writeable_tlv_based_enum! with reserved
type bytes (FeeTier: Standard=0, ZeroFee=2, Custom=4; FeePolicy: Flat=0)
so future variants are additive. Flat is a length-prefixed tuple variant
wrapping FeeTier, letting a later commit store a policy as a TLV field
that defaults cleanly on old records.
Persist a FeePolicy per intercept SCID record so a later milestone can look up a peer's policy at the skim site. ScidWithPeer::new still hands every record Flat(Standard), so nothing changes yet. The field uses a length-delimited TLV with a default_value of Flat(Standard) at type 4. Records written before this field existed carry only types 0 and 2, so they read back as Standard with no migration. A test encodes a copy of the old two-field layout and decodes it through the new struct to pin that default.
Replace the inline compute_forward_fee block in calculate_htlc_actions_for_peer with a single resolve_skim call against a literal Flat(Standard) policy. This is the one place the LSP decides what to skim; routing it through the pure function is what later milestones need to swap the literal for a per-peer policy lookup. Not a strict no-op: it inherits the u128 overflow fix from resolve_skim, so a >9223-BTC HTLC is now skimmed 2% instead of overflowing and forwarding free. Every realistic HTLC is unchanged. The peer's stored policy is still ignored; every forward resolves Flat(Standard) until the lookup lands. The two old log lines (overflow, skim-ate-the-HTLC) collapse into one: resolve_skim can't overflow, so a zero skim on a non-zero HTLC can only mean the fee would have eaten the whole amount.
MDK-980 lets a registering node carry a signed grant for a
non-standard FeePolicy that the LSP verifies locally before honouring.
This is just the pure core: the claim ADT and the verifier. Nothing
calls it yet, so behaviour is unchanged.
A claim is a versioned TLV pair. ClaimPayload binds {scheme, node_id,
policy}; SignedFeeClaim wraps the encoded payload as an opaque byte
string plus a detached BIP340 signature over SHA256 of those bytes.
Keeping the payload opaque means the verifier hashes exactly what was
signed and never reproduces the payload's TLV layout to check the
signature. TLV rather than a fixed concatenation keeps later schemes
(more policy arms, an issuer key-id) additive: a new tag, not a re-issue.
verify_claim takes a slice of issuer keys, not a single Option. An empty
slice rejects every claim, which is how the feature stays inert until a
key is configured. A non-empty slice accepts a signature from any one of
the keys. That covers a no-key-id claim today and key rotation later,
and per-key scoping can slot in without a breaking config change.
The verifier borrows a caller-supplied secp context rather than building
its own, so the verifier stays allocation free and context lifetime is
the caller's call. The handler wiring that follows will own a long-lived
verify context on the service and hand it in. The scheme byte is read
and matched before the signature check because it selects which
verification rules apply, so an unknown scheme is rejected before any
crypto runs.
The wire format is the contract MDK-981 (ldk-node) and MDK-982 (the TS
minter) must reproduce byte-for-byte, so an in-tree test vector pins a
known issuer key and claim hex. Drift in the TLV layout or the signing
input fails that test instead of silently diverging across repos. The
signing helper is test-only; the verifier does no I/O.
Adds an optional fee_claim string to RegisterNodeRequest so a node can
present a signed grant when it registers. The field is hex of the
SignedFeeClaim bytes from the previous commit. Nothing reads it yet; the
service-side verify and persist land later.
serde(default, skip_serializing_if = "Option::is_none") keeps the wire
backward compatible both ways. An old client that sends {} still decodes
to fee_claim: None through the existing params.unwrap_or(json!({})) path,
and a node with no claim emits {} rather than an explicit null, so the
request bytes match today exactly when no claim is set.
The client constructs the request with fee_claim: None; only a node that
has been granted a claim populates it, which is wired up in a later
change.
The persisted ScidWithPeer already carries a policy field, but the store only built peer<->scid lookups, so the granted policy was written to disk and then lost in memory. This adds a policy_by_peer map kept in lockstep with the existing two: populated on load from the decoded records, upserted on insert, dropped on remove. A get_policy accessor reads it back. ScidWithPeer::new now takes the policy explicitly rather than defaulting it, so there is one way to build the record and the policy is always named at the construction site. add_intercepted_scid passes Flat(Standard), the path a peer with no claim takes; the register handler will pass a verified grant. The on-disk default is unchanged: it comes from the TLV default_value, not the constructor. get_policy warns as dead code until the handler reads it. Keeping policy in its own map rather than handing out whole ScidWithPeer records keeps the skim site's read to a single lookup by node id, which is all it needs.
Wires the claim verifier into register_node. The service now holds a set of trusted issuer keys and a long-lived verification context; when a peer registers, any fee_claim it presented is verified against those keys and the granted policy is persisted onto the peer's SCID record before the response is enqueued, so the policy is in place by the time the SCID is handed out. The feature is inert by default. An empty issuer_pubkeys set short-circuits before any crypto and resolves every peer to the standard policy, byte for byte identical to today. Population of the key set is the operator's job downstream; nothing in-tree sets it. A grant is only ever upserted, never downgraded. An absent or unverifiable claim returns None and leaves an existing record untouched, so a transient miss or a malformed claim can't wipe a live grant; a brand-new record falls back to standard. This deviates from a literal "else standard" on purpose: once a node has been granted zero-fee, a dropped claim on a later re-registration must not silently restore the 2% skim. The verifier borrows the service's context rather than allocating per call, and resolve_claim_policy logs and swallows verification failures rather than failing the registration: a bad claim should cost the node its discount, not its channel. add_intercepted_scid is removed. It only built a standard-policy record, which the new persist path now does directly with the resolved policy, so the old helper had no remaining caller.
The forwarding skim now reads the peer's granted policy from the SCID store instead of hard-coding Flat(Standard). A peer with no grant resolves to Standard, so the skim is byte-for-byte the historical 2% for everyone the issuer set has not waived. This is the read side that makes the persisted grant actually take effect; before it, a verified ZeroFee was stored and then ignored. The lookup is hoisted out of the per-HTLC loop because the policy is keyed by peer, not by HTLC, so it is one store read per batch rather than one per forward. A zero skim is now two distinct cases, so the log is split. ZeroFee is the expected path and logs at info. Any other tier skimming nothing means the fee would have consumed the whole HTLC, which keeps the error-level log: a forward of the full amount that we expected to take a cut on. Previously ZeroFee was unreachable, so the single error log was correct; now that a grant can legitimately waive the fee, the error would be noise.
MDK-980 added the fee_claim field to RegisterNodeRequest and the verifier that reads it, but the client still hardcoded None, so no node could ever present a claim. This lets the caller pass one. The claim rides as a per-call argument rather than a field on LSPS4ClientConfig: that config derives Copy, which an Option<String> would break, and the value belongs to the layer above (ldk-node), which already holds it and relays it on every registration. There is a single call site, so threading it as a parameter is the smaller, honest change. The value is opaque here: a lowercase-hex signed grant the LSP alone decodes and verifies. Passing None reproduces today's behavior exactly, so nothing changes until a node is configured with a claim.
Upstream 0.2.3 added lsps5_integration_tests cases that construct LiquidityManagerSync directly. Our LSPS4 work extends the liquidity config structs and appends a logger parameter to the constructor, so the new upstream call sites no longer compile. Same role as the earlier 'Fix tests for LSPS4 and Logger generic additions' commit, applied to the v0.2.5 base.
amackillop
changed the title
The v0.2.5 rebase dropped the MDK-799 dual-advertise commits because upstream 0.2.2 renumbered SplicePrototype to bit 63, turning their clear_splicing() ACINQ carve-out into a production-bit strip. That left bit-155-only clients unable to splice: we neither advertised a bit they recognize nor recognized the bit they advertise, so LSPS4 would fall back to opening a second channel on every liquidity top-up. We have to assume such clients exist: client version telemetry (MDK-424, lightning-node#824) has not shipped, and the legacy e2e jobs cannot distinguish a splice from the channel-open fallback by design. Reintroduce the bit under a new SplicePrototypeLegacy feature (155 is unused upstream now) and accept it at the splice_channel gate. The wire protocol is unchanged between v0.2 and v0.2.5 (msgs.rs and interactivetxs.rs are byte-identical across the patch series), so a 0.2.5 LSP can splice with a v0.2-fork client; only the negotiation bit moved. The ACINQ carve-out returns unchanged: eclair rejects an Init carrying both 155 (their pre-standard splice bit) and 63. Sunset: once lightningdevkit#824 ships and shows no bit-155 clients, drop the define_feature block, the set_splicing_legacy_optional() call, the strip, and the gate's legacy arm.
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
15 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.
Review vehicle only — do not merge. Downstream consumes this branch by rev pin (
ldk-node→lightning-node). Merging intolsp-0.2.0would only add a pointless merge commit;lsp-0.2.0stays frozen as the old line.What
lsp-0.2.5= our 33 fork commits rebased from upstreamv0.2ontov0.2.5, plus two new commits (an upstream-test fix and the legacy splice-bit restore, below). We were missing all 112 upstream commits from the 0.2.1–0.2.5 patch series.Security highlights we pick up:
possiblyrandomHashDoS fix (Get real rand inpossiblyrandomon supported platforms w/o feat lightningdevkit/rust-lightning#4719); LSPS1/2/5 persistence no longer sticks after one KVStore failure (Reset LSPS5persistence_in_flightcounter on persist errors lightningdevkit/rust-lightning#4597); anchor reserve underestimates (Count zero-fee-commitments channels in anchor reserve check lightningdevkit/rust-lightning#4592, Account for UTXO base weight in anchor reserve checks lightningdevkit/rust-lightning#4670); 0FC splice-out reserve breach (Error if the calculated reserve would be greater than the channel value lightningdevkit/rust-lightning#4580); MPP-claim/async-persist restart wedge (Attempt to unblock blocked monitor updates on startup lightningdevkit/rust-lightning#4520); log sanitization for counterparty strings.splice_channelfails fast on non-splicing peers (Assert peer supports splicing before splicing channel lightningdevkit/rust-lightning#4262); async-persist restart force-closure bug (Hold in-flight monitor updates until background event processing lightningdevkit/rust-lightning#4377);SplicePrototypemoved to bit 63 (SwitchSplicePrototypefeature flag to the prod feature bit lightningdevkit/rust-lightning#4387).How to review
Don't read the file diff (it includes upstream's 112 commits). Review the rebase itself:
29/33 commits are byte-identical (
=). The interesting entries:Dropped (2):
Strip splice prototype bit when peering with ACINQ+Dual-advertise splice prototype and production feature bits(MDK-799). Upstream 0.2.2 redefinedSplicePrototypeas bit 63 (==SpliceProduction, lightningdevkit#4387). Both commits applied cleanly on the rebase but with broken semantics: the ACINQ strip'sclear_splicing()now clears the production bit, i.e. it would have disabled splicing with ACINQ outright; dual-advertise's extraset_splicing_production_optional()is a no-op. Replaced by the restore commit below.Modified (4):
Make minimum channel reserve configurable (#1): upstream changedget_holder_selected_channel_reserve_satoshisto returnResult(err when channel value < minimum) and cap the proportional input at 1M. Kept upstream's shape, threaded our configurablemin_their_channel_reserve_satoshisthrough it, including theErrbound — so the LSPmin=0case can never hit the error path, preserving the reserve-free-channel behavior. Also de-duplicated a stacked import block git auto-merge produced in the channel.rs test module, and ported ourtest_configurable_min_channel_reserveto the renamed test APIs (context→fundingaccessors,TestFeeEstimator::new).Migrate LSPS4 to async/sync persistence: auto-merge adopted upstream'sdid_persist |=accumulation pattern (from Bypass monitor sync requests when no partition key given inupdate_monitor_with_chain_datalightningdevkit/rust-lightning#4544-era logging fix) at the LSPS2/5 persist call sites. No LSPS4 logic change.Fix tests for LSPS4 and Logger generic additions (#19)/Gate counterparty-initiated splices behind conf depth (#20): context-only conflicts in the channel.rs test module and splicing_tests.rs (upstream appended its new 0FC reserve-breach tests at the same spot our min-depth test lives). Both sides kept.New commits (2):
Fix upstream lsps5 test for LSPS4 constructor and config additions— upstream 0.2.3's newlsps5_integration_testsconstructLiquidityManagerSyncdirectly and predate our extra logger param /lsps4_*_configfields. Same role as old Fix tests for LSPS4 and Logger generic additions #19, applied to the new base.Restore splice negotiation with clients on the legacy prototype bit(d6f87a44d) — correct reimplementation of the two dropped commits on the new base. NewSplicePrototypeLegacyfeature at bit 155 (unused upstream post-renumber), dual-advertised by default inprovided_init_features, accepted at thesplice_channelgate (supports_splicing() || supports_splicing_legacy()), ACINQ strip clears the legacy bit only. We must assume bit-155-only clients exist until client version telemetry (MDK-424, CumuloGlobal/lightning-node#824) ships. Safe cross-version: the splice wire protocol is byte-identical betweenv0.2andv0.2.5(msgs.rs/interactivetxs.rsuntouched across the patch series) — only the negotiation bit moved. Sunset path in the commit message. Upstream'stest_splicing_not_supported_api_erroradapted to clear both bits; new unit test covers dual-advertise + strip.Also verified: LSPS4 does not need the lightningdevkit#4597 persistence-stuck fix — its
persist()is a no-op; the SCID/HTLC stores persist immediately per operation.Test evidence
RUSTFLAGS="--cfg=lsps1_service" cargo test --workspace(the justfilecheckcommand) atd6f87a44d: 1509 passed, 0 failed across 31 test binaries.