LSP-Plugin: Add MPP support - #8948
Conversation
dc5b671 to
44f666d
Compare
44f666d to
df7c132
Compare
73b18ed to
da925c6
Compare
|
Cache cleared, and restarting the CI while I review the code 👍 |
|
Nice PR, maybe a bit on the long side, and a bit of duplication, but the architecture is nice. ACK |
| )] | ||
| InsufficientDeductibleCapacity { | ||
| opening_fee_msat: u64, | ||
| deductible_capacity_msat: u128, |
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct PaymentPart { | ||
| pub htlc_id: u64, |
There was a problem hiding this comment.
Just a quick doubt I had: this refers to which ID? As far as I remember the numbering of HTLCs was using a composite (channel_id, htlc_id) key because the protocol insists on numbering HTLCs, making either an alias necessary or the composite key to make them unique.
This is the DB HTLC ID that counts up monotonically, not the protocol ID which is per-channel, correct?
There was a problem hiding this comment.
Very good question indeed, I'll need to check what exactly the htlc_accepted hook provides.
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum SessionEvent { |
There was a problem hiding this comment.
That's quite the extensive list of potential errors and outcomes, nice listing 👍
| } | ||
| } | ||
|
|
||
| pub fn apply(&mut self, input: SessionInput) -> Result<ApplyResult> { |
There was a problem hiding this comment.
You'll probably want to break this up into a dispatch method (containing the match (field_a, field_b, input)) and several handler methods that take the involved parts and pieces of information as explicit parameters. That'll help get a quick overview, and allow diving deep on specific operations if interested.
There was a problem hiding this comment.
do you mean something like the following? This would indeed make it more readable.
pub fn apply(&mut self, input: SessionInput) -> Result<ApplyResult> {
match(&mut self.stat) {
SessionState::Collecting => apply_on_collecting(input),
SessionState::AwaitingChannelReady => apply_on_awaiting_channel_ready(input),
....
}
}
// Maybe needs to get the state passed here and reinjected into the Session later
fn apply_on_collecting(&mut self, input: SessionInput) -> Result<ApplyResult> {
// Check what needs to be "taken" here to please the ownership model
match(input) {
....
}
}There was a problem hiding this comment.
Yep, pretty much, can be a cleanup though. Also a quick diagram on allowable state changes would make this much easier to follow as well. All followup things though, not a blocker.
There was a problem hiding this comment.
That is not to say I don't like the (state, event) matching, as that automatically gives you an indication whether all states and transitions are covered, via the exhaustive enumeration rule in Rust.
| scid: ShortChannelId, | ||
| datastore: D, | ||
| ) -> ActorInboxHandle { | ||
| let (tx, inbox) = mpsc::channel(128); // Should we use max_htlcs? |
There was a problem hiding this comment.
The backlog is mostly intended for bursty behavior, and should be set to the maximum number of events in flight. If there is no more room, we will drop block the sending side, no messages should be lost. If the sender does not need to make progress, and the expectation is that we can process messages in the order they arrive (no interleaving) it should be safe to set the backlog to 1. Don't think about elements queued up, rather consider if you need to make progress on the sendign side at all, while elements are being processed.
| } | ||
| } | ||
|
|
||
| fn execute_action(&mut self, action: SessionAction) { |
There was a problem hiding this comment.
Love the match () {} matrix, less so the deep nesting, as it pulls the matrix apart and makes reasoning about its relations and transitions harder :-)
There was a problem hiding this comment.
I'll clean it up and move stuff into handler functions
| return Ok(serde_json::json!({ | ||
| "result": "continue", | ||
| "mindepth": 0, | ||
| "reserve": 0, |
There was a problem hiding this comment.
Hm, this will break megalithic LSP? They do not have a way to set no reserve.
There was a problem hiding this comment.
Whoo! GOOD catch. totally forgot about megalith here. I'll make it an option with a sane (set nothing) default
| }; | ||
|
|
||
| // Main loop: process inbox events | ||
| loop { |
There was a problem hiding this comment.
Is this not duplicating the entire FSM logic, just because we enter the system through recovery, rather than kicking off a new session? We could just call into the dispatch of events here, and everything else would be the same, or am I missing something?
There was a problem hiding this comment.
Let me see if I can clean it up a bit
| ) -> Result<(String, String)> { | ||
| (**self) | ||
| .fund_channel(peer_id, channel_capacity_msat, opening_fee_params) | ||
| .fund_channel(peer_id, channel_capacity_msat, opening_fee_params, scid) |
There was a problem hiding this comment.
Are there "let's define ALL the operations on a variant of the original behavior" useful? I am failing to see how defining the operations on Arc<T> and then just having them forward to T could be useful 🤔
|
The CI failures appear to NOT be flaky tests, as I am seeing a lot of the new tests failing on us. |
da925c6 to
d0fc010
Compare
|
@nepet can you rebase, resolve conflicts and clear CI by the end of this week so we can add this to 26.06? |
4d6bfcf to
16d662b
Compare
|
rebased on master |
| pub opening_fee_params: OpeningFeeParams, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub expected_payment_size: Option<Msat>, | ||
| pub channel_capacity_msat: Msat, |
There was a problem hiding this comment.
These are guaranteed to be new entries, right? If you try to load an existing one with non-optional fields it'll likely fail.
| } | ||
| } | ||
|
|
||
| pub fn apply(&mut self, input: SessionInput) -> Result<ApplyResult> { |
There was a problem hiding this comment.
Yep, pretty much, can be a cleanup though. Also a quick diagram on allowable state changes would make this much easier to follow as well. All followup things though, not a blocker.
| return Ok(ApplyResult { | ||
| actions: vec![ | ||
| SessionAction::FailHtlcs { | ||
| failure_code: UNKNOWN_NEXT_PEER, |
There was a problem hiding this comment.
You probably don't want to use UNKNOWN_NEXT_PEER as that is a permanent error, and the client is supposed to remember that the channel never existed, and future attempts with blocklist them. It's a common error lnd always gets wrong too.
| // We don't check for max parts here as we are in the middle of | ||
| // the channel funding. We'll check once we transitioned. | ||
|
|
||
| Ok(ApplyResult { |
There was a problem hiding this comment.
Sorry my eyes glazed over at this point, I trust the logic is sensible, as reconstructing it from code is painful.
| } | ||
| } | ||
|
|
||
| pub fn apply(&mut self, input: SessionInput) -> Result<ApplyResult> { |
There was a problem hiding this comment.
That is not to say I don't like the (state, event) matching, as that automatically gives you an indication whether all states and transitions are covered, via the exhaustive enumeration rule in Rust.
The UNKNOWN_NEXT_PEER constant carried the value 0x4010 which is not a valid BOLT4 failure code; unknown_next_peer is PERM|10 = 0x400a. Every path that was supposed to fail with unknown_next_peer actually sent the deprecated incorrect_payment_amount code. Also stop using the permanent unknown_next_peer for retryable failures (too many parts, fee allocation failure, channel funding failure) as senders are expected to blocklist the route on permanent errors. These now fail with temporary_channel_failure; LSPS2 only mandates unknown_next_peer when the payment cannot cover the opening fee or valid_until has passed, and requires temporary_channel_failure for a client disconnect during channel establishment. Addresses review feedback by @cdecker on ElementsProject#8948.
HTLC ids are a per-channel counter, so MPP parts of one payment arriving over different incoming channels can share the same id -- the common case for multi-part payments. The session actor tracked pending HTLC replies in a map keyed by the bare id, so a colliding part silently replaced the earlier part's reply channel: the earlier hook call resolved to a generic continue (failing the part upstream with WIRE_UNKNOWN_NEXT_PEER), the manager misread the dropped reply as a dead actor and removed the live session, and forward amounts could be cross-wired between parts. Introduce HtlcId as the composite of the incoming channel scid and the per-channel id, carry it in PaymentPart/ForwardPart and key the pending-reply map with it. Addresses review feedback by @cdecker on ElementsProject#8948 (HTLC id ambiguity).
Both sides unconditionally requested a zero channel reserve on JIT channels: the client's openchannel hook always replied reserve=0 and the LSP's fundchannel_start always sent reserve=0. Some LSP implementations (e.g. Megalithic) cannot handle an explicit zero reserve. Leave the reserve at lightningd's default and add opt-in flags: experimental-lsps-client-zero-reserve on the client and experimental-lsps2-zero-reserve on the service. Also add uniform #[serde(default)] on DatastoreEntry's optional fields and a regression test pinning that entries persisted without the newer fields keep deserializing. Addresses review feedback by @cdecker on ElementsProject#8948 (Megalithic reserve).
The UNKNOWN_NEXT_PEER constant carried the value 0x4010 which is not a valid BOLT4 failure code; unknown_next_peer is PERM|10 = 0x400a. Every path that was supposed to fail with unknown_next_peer actually sent the deprecated incorrect_payment_amount code. Also stop using the permanent unknown_next_peer for retryable failures (too many parts, fee allocation failure, channel funding failure) as senders are expected to blocklist the route on permanent errors. These now fail with temporary_channel_failure; LSPS2 only mandates unknown_next_peer when the payment cannot cover the opening fee or valid_until has passed, and requires temporary_channel_failure for a client disconnect during channel establishment. Addresses review feedback by @cdecker on ElementsProject#8948.
HTLC ids are a per-channel counter, so MPP parts of one payment arriving over different incoming channels can share the same id -- the common case for multi-part payments. The session actor tracked pending HTLC replies in a map keyed by the bare id, so a colliding part silently replaced the earlier part's reply channel: the earlier hook call resolved to a generic continue (failing the part upstream with WIRE_UNKNOWN_NEXT_PEER), the manager misread the dropped reply as a dead actor and removed the live session, and forward amounts could be cross-wired between parts. Introduce HtlcId as the composite of the incoming channel scid and the per-channel id, carry it in PaymentPart/ForwardPart and key the pending-reply map with it. Addresses review feedback by @cdecker on ElementsProject#8948 (HTLC id ambiguity).
Both sides unconditionally requested a zero channel reserve on JIT channels: the client's openchannel hook always replied reserve=0 and the LSP's fundchannel_start always sent reserve=0. Some LSP implementations (e.g. Megalithic) cannot handle an explicit zero reserve. Leave the reserve at lightningd's default and add opt-in flags: experimental-lsps-client-zero-reserve on the client and experimental-lsps2-zero-reserve on the service. Also add uniform #[serde(default)] on DatastoreEntry's optional fields and a regression test pinning that entries persisted without the newer fields keep deserializing. Addresses review feedback by @cdecker on ElementsProject#8948 (Megalithic reserve).
36c53dd to
4c58bff
Compare
The UNKNOWN_NEXT_PEER constant carried the value 0x4010 which is not a valid BOLT4 failure code; unknown_next_peer is PERM|10 = 0x400a. Every path that was supposed to fail with unknown_next_peer actually sent the deprecated incorrect_payment_amount code. Also stop using the permanent unknown_next_peer for retryable failures (too many parts, fee allocation failure, channel funding failure) as senders are expected to blocklist the route on permanent errors. These now fail with temporary_channel_failure; LSPS2 only mandates unknown_next_peer when the payment cannot cover the opening fee or valid_until has passed, and requires temporary_channel_failure for a client disconnect during channel establishment. Addresses review feedback by @cdecker on ElementsProject#8948.
HTLC ids are a per-channel counter, so MPP parts of one payment arriving over different incoming channels can share the same id -- the common case for multi-part payments. The session actor tracked pending HTLC replies in a map keyed by the bare id, so a colliding part silently replaced the earlier part's reply channel: the earlier hook call resolved to a generic continue (failing the part upstream with WIRE_UNKNOWN_NEXT_PEER), the manager misread the dropped reply as a dead actor and removed the live session, and forward amounts could be cross-wired between parts. Introduce HtlcId as the composite of the incoming channel scid and the per-channel id, carry it in PaymentPart/ForwardPart and key the pending-reply map with it. Addresses review feedback by @cdecker on ElementsProject#8948 (HTLC id ambiguity).
On restart CLN re-plays the htlc_accepted hook for incoming HTLCs that were held but not resolved before the crash. The recovered session actor silently dropped these AddPart inputs, which also dropped the hook's reply channel: the manager misread that as a dead actor, removed the recovered session from its map, and subsequent forward_event notifications found no session -- so the withheld funding transaction was never broadcast even though the client could still settle the payment. Route AddPart through the same convert/apply pipeline as the other recovered inputs (the FSM already forwards late parts in the AwaitingSettlement and Broadcasting states) and release any pending replies when a recovered actor terminates.
Three related races allowed a second on-chain channel to be funded for a single lsps2.buy: - create_session did not check whether the active datastore entry already carried a channel_id, so any part arriving for a hash without a live session (payer retry, late relay, replay) started a fresh Collecting session that could reach the threshold and fund again. Refuse with a new SessionAlreadyFunded error instead. - on_payment_settled/on_payment_failed removed the session handle from the map while the actor was still alive, making the FSM's late-part forwarding paths (AwaitingSettlement/Broadcasting + AddPart) unreachable and opening the retry window above. Keep the handle registered and prune dead handles lazily, as on_part and on_new_block already do. - Map-entry removal on a dead handle was not identity-checked and could tear down a newer live session created concurrently for the same hash. Only remove the entry when it still refers to the same actor. The htlc_accepted hook now fails parts hitting a terminated or already-funded session with temporary_channel_failure instead of falling through to continue (which surfaced upstream as the permanent unknown_next_peer).
The CLTV check failed held HTLCs only once the chain tip was already past the earliest cltv_expiry. At that point the upstream peer is entitled to force-close to claim the HTLC timeout on-chain, so failing off-chain is too late. Fail once fewer than CLTV_SAFETY_BUFFER (6) blocks remain before the earliest expiry.
Every ForwardHtlcs action started a new 5-second listpeerchannels poller and overwrote the previous JoinHandle without aborting it, so each late-arriving part leaked a poll task that keeps running for the lifetime of the channel on successful sessions. Start the poller only once and abort it when the actor terminates.
Recovery aborted on the first entry that failed to recover (RPC error, malformed entry), leaving every other persisted session unrecovered while CLN replays their HTLCs. Log and continue per entry instead. An overflowing opening-fee computation surfaced as an FSM error that the actor only logged, leaving the HTLCs hanging until the collect timeout failed them with the wrong code. Fail the session immediately with unknown_next_peer as LSPS2 mandates.
… range The buy response advertised client_trusts_lsp: false while the funding flow withholds the funding transaction until the preimage is revealed -- which is exactly the client-trusts-LSP model. A spec-following client is entitled to wait for the funding tx in its mempool before settling when the flag is false, which would deadlock against the withhold. Advertise true until the broadcast-on-channel_ready mode is implemented. Also add the LSPS2-mandated check that a requested payment_size_msat lies within min/max_payment_size_msat, returning error 202/203. The existing tests for this passed for the wrong reasons (the mock had no blockheight, and the below-min case tripped the fee check instead); they now pin the exact error codes with a fully configured mock.
Compute the opening fee from the negotiated payment_size_msat instead of the sum of collected parts. BOLT4 permits the payer to overpay, and the client verifies the deducted extra_fees against the fee promised for payment_size_msat -- charging on parts_sum makes a compliant client reject overpaid payments. Var-invoice mode keeps using the first HTLC's value as mandated. Only attach the extra_fee TLV (type 65537) to forwarded parts that actually have a fee deducted; LSPS2 forbids including it otherwise, and we attached a zero-value TLV to every part.
Both sides unconditionally requested a zero channel reserve on JIT channels: the client's openchannel hook always replied reserve=0 and the LSP's fundchannel_start always sent reserve=0. Some LSP implementations (e.g. Megalithic) cannot handle an explicit zero reserve. Leave the reserve at lightningd's default and add opt-in flags: experimental-lsps-client-zero-reserve on the client and experimental-lsps2-zero-reserve on the service. Also add uniform #[serde(default)] on DatastoreEntry's optional fields and a regression test pinning that entries persisted without the newer fields keep deserializing. Addresses review feedback by @cdecker on ElementsProject#8948 (Megalithic reserve).
A single failed forward (e.g. a part exceeding the channel's max_accepted_htlcs) abandoned the whole session and closed the channel even though the client could still settle the other offered parts. Thread the failed part's (channel, htlc id) through the forward_event path and drop just that part from the session; abandon only when no forwarded parts are left (or the failed part is unknown, as for recovered sessions, preserving the previous behavior). Resolves the TODO about #HTLCs > max_accepted_htlcs.
- Recovered session actors now carry the peer id from the datastore entry so the Disconnect action works after recovery, instead of warning on an empty public key. - Move tokio's test-util feature to dev-dependencies. - Resolve the inbox-capacity TODO with a rationale comment: senders use backpressure, the buffer only needs to cover in-flight inputs. - Document the Msat/u64/u128 amount conventions in the session module. - Drop the unused LspsBuyJitChannelResponse struct and unused test helper, fix needless returns, a clone on a Copy type, and a while-let loop in code this branch introduced.
finalize_session wrote the finalized entry with MUST_CREATE before deleting the active one. A crash between the write and the delete leaves the active entry in place; the next startup recovers it, finalizes it again, and the MUST_CREATE write fails - so the delete never runs and the entry is re-recovered on every restart, forever. Write with CREATE_OR_REPLACE and delete the active key regardless of the write result. The scid is unique per buy request, so replacing a finalized record for the same scid loses nothing.
The htlc_accepted hook finalized the datastore entry for any HTLC on a known jit scid once opening_fee_params.valid_until had passed, with no check for an in-flight session. A late MPP part arriving after the offer expired while the session was in AwaitingChannelReady or AwaitingSettlement moved the active entry - the one holding channel_id and funding_psbt - into finalized and deleted it. A crash before the actor finished then left the withheld channel and its reserved inputs unrecoverable, since list_active_sessions no longer saw the session. Move the check into SessionManager::create_session, behind the existing channel_id guard, so it can only reject a session that does not exist yet. Late parts now reach the AwaitingSettlement and Broadcasting AddPart arms that were written for them. The wire behaviour for a genuinely stale offer is unchanged: unknown_next_peer.
abandon_session skipped both close and unreserveinputs whenever the channel was not in CHANNELD_NORMAL. The main trigger for AbandonSession is the 5s poll reporting that the channel is no longer normal, so on exactly that path the call became a no-op: the funding PSBT's inputs stayed reserved until the reservation expired, and a channel stuck in e.g. CHANNELD_AWAITING_LOCKIN was never closed. close_and_unreserve had the same guard, and there it is worse - recovery clears channel_id and funding_psbt from the entry afterwards, losing the PSBT handle entirely. Split the two questions the NORMAL check was conflating. is_channel_alive keeps its meaning for the health poller; cleanup now asks channel_state and closes any channel that is not already shutting down, closing, or onchain. unreserveinputs runs unconditionally: it skips inputs that are not reserved and still succeeds, so repeating it is harmless.
finalize_session writes the finalized entry and then deletes the active one, so a crash between the two leaves both keys in place. Pre-seed the finalized key to reproduce that state, drive a session to a collect timeout, and assert the active entry is still cleaned up and the placeholder replaced. Fails against the MUST_CREATE version with "finalize_session failed for scid=...: calling datastore for finalize_session" and an active entry that never goes away.
Bolt4 requires us to provide a `channel_update` on any `UPDATE` type message which `TEMPORARY_CHANNEL_FAILURE` is. As we don't want to provide any `channel_update` we set the u16 `len` field to zero, according to Bolt4. Co-authored-by: Níckolas Goline <nickolas.goline@gmail.com>
Reduce flaky races when running tests with `VALGRIND=1` by waiting for a block to appear in the mempool. Co-authored-by: Níckolas Goline <nickolas.goline@gmail.com>
We had a default collect timeout of 5s. The timeout is triggered on the FSM when still collecting parts of a jit-channel payment. This introduced test races where CI wasn't fast enough to make it on various ocasions. This commit relaxes the timeout to the global pytest TIMEOUT and only limits it on the tests that actually test the collection timeout. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
pyln's postgres provider appends a suffix to the db names and postgres truncates identifiers to 63 bytes. This results in long names collapsing into the same db name which resulted in a DuplicateDatabase error. This commits shortens the test name that was too long. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
4d47562 to
3e1dce5
Compare
nGoline
left a comment
nGoline
left a comment
There was a problem hiding this comment.
Re-reviewed after the 2026-08-19 rebase with more context.
The FSM fixes it: poll_channel_ready's 120s deadline plus check_cltv_timeout bound the wait, and withhold:true means the failure path never broadcasts the funding tx at all.
Two things from my 2026-08-04 review are still open on 3e1dce5 and I've actually found 2 more blocking issues (manager.rs:194, manager.rs:314) (comments in-line).
| } | ||
| } | ||
|
|
||
| fn check_cltv_timeout( |
There was a problem hiding this comment.
A CLTV timeout in AwaitingChannelReady goes to Failed, not Abandoned. The FSM has no channel_id/funding_psbt yet, so it cannot emit AbandonSession, and the FundChannel task spawned at actor.rs:467 is never aborted. If poll_channel_ready then succeeds it sends ChannelReady into a dropped inbox: orphaned withheld channel, reserved inputs never released.
There was a problem hiding this comment.
Thanks @nGoline, this is an interesting finding, that only seems to be a problem on regtest. It can't really occure on mainnet and the real risk it points is already bounded.
JIT-channels in client-trusts-lsp mode are zero-conf channels that only get broadcasted once the LSP successfully deducted their fees from the first successful payment on this channel. Before that, the process involves no chain interaction. A cooperative client reaches CHANNELD_NORMAL the moment channel_ready is exchanged, so we leave AwaitChannelReady immediately.
The only way we linger there is a client that holds back channel_ready, and that window is bounded by the poll_channel_ready deadline of 120s in fund_channel, not by the block-based CLTV check. On mainnet approx 0-1 blocks are mined in that windwo, so check_cltv_timeout can't really fire before the poll deadline resolves the funding (ChannelReady or FundingFailed). The check_cltv_timeout is belts-and-suspenders. If there should ever be a situation in which the check fires, a node operator can still recover from this situation (close or unreserveinputs + delete the session from the datastore).
So the genuine underlying vector (a client that stalls during funding) is a bounded, self-healing, no-theft 120s resource lockup.
I'd defer any rework that states this more clearly (remove the betls-and-suspenders). We'll address this properly in the optimistic funding rework: a CLTV hit mid-funding will reject the current HTLCSs but keep the withheld channel alive for a grace period and let retries reuse it, rather than abandon it + a sweep for orphaned withheld channels.
| while let Some(actor_input) = self.inbox.recv().await { | ||
| let session_input = match &actor_input { | ||
| ActorInput::AddPart { .. } | ||
| | ActorInput::PaymentSettled { .. } | ||
| | ActorInput::PaymentFailed { .. } | ||
| | ActorInput::FundingBroadcasted { .. } => { | ||
| self.convert_input(actor_input).await | ||
| } | ||
| _ => continue, | ||
| }; | ||
|
|
||
| if let Some(input) = session_input { | ||
| if self.apply_and_execute(input) { | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
run_recovered drops NewBlock and ChannelClosed, and never starts a channel poll (start_channel_poll only fires from ForwardHtlcs, and a session recovered in AwaitingSettlement has no HTLCs to replay). Recovered sessions get no CLTV check and never abandon on channel death. It also has no cancel_channel_poll() on exit, unlike run().
| if let Some(handle) = sessions.get(&payment_hash) { | ||
| handle.clone() | ||
| } else { |
There was a problem hiding this comment.
on_part keys self.sessions by payment_hash and only looks at scid when no session exists. The payment hash is public in the client's bolt11. A second client can buy its own jit scid, send a first HTLC carrying victim A's payment hash to its own scid, and A's real payment then matches by hash and is forwarded into the attacker's channel with A's fee deducted. No preimage, so griefing rather than theft, but the wrong peer gets the payment and the channel state. Key on (scid, payment_hash), or have the actor reject a part whose scid is not its own.
| if entry.channel_id.is_some() { | ||
| return Err(ManagerError::SessionAlreadyFunded); | ||
| } |
There was a problem hiding this comment.
the SessionAlreadyFunded guard reads entry.channel_id, which convert_input only persists on ChannelReady, i.e. after up to 90s of connect_with_retry plus 120s of poll_channel_ready. During that window a second HTLC with a different payment hash on the same scid passes the guard and issues a second FundChannel. The jit scid is the invoice's route hint, so N HTLCs with N payment hashes means N zero-conf opens and N x capacity reserved. Both actors also save_session on the same scid key, so entry.payment_hash is clobbered and recovery can re-attach only one channel. Needs an in-memory funding-in-progress latch, not a datastore field.
| ForwardActivity::AllFailed => { | ||
| self.datastore | ||
| .finalize_session(&scid, SessionOutcome::Abandoned) | ||
| .await?; | ||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
ForwardActivity::AllFailed finalizes as Abandoned without calling close_and_unreserve, unlike the NoForwards branch right above it and unlike the live AbandonSession path. After a restart where every forward had failed, the withheld channel stays open and the inputs stay reserved, with the active entry deleted so nothing revisits it.
| let fee_base_msat = match self.payment_size_msat { | ||
| Some(size) => size.msat(), | ||
| None => parts_sum.msat(), | ||
| }; |
There was a problem hiding this comment.
min/max_payment_size_msat are only checked inside OpeningFeeParams::validate's if let Some(payment_size_msat), so a variable-amount buy never bounds parts_sum. The deleted htlc.rs checked both (step C). Regression: a payment above max_payment_size_msat is accepted against a capacity the policy sized without knowing the amount.
The session map is keyed by payment_hash, but the payment hash is public in the client's bolt11. A part arriving on a different jit scid than the one that owns the session was merged into it and forwarded, so a payment could be routed into another buy request's channel. Record the owning scid on the session handle and reject a part whose scid does not match.
channel_id is only persisted once funding completes, so the existing datastore guard leaves a window in which a second HTLC carrying a different payment hash on the same jit scid opens a second channel and reserves another set of inputs. Reject a new session for a scid that a live session already owns; dead handles, pruned lazily, are skipped so they cannot block a later legitimate funding.
A session recovered into AwaitingSettlement skipped the Collecting -> ForwardHtlcs path that starts the channel poll, and run_recovered dropped NewBlock and ChannelClosed inputs, so it never ran a CLTV check and never abandoned when its channel went away. Start the poll for a recovered session that already has a channel, route both inputs through the FSM, and cancel the poll on exit.
…failed The AllFailed recovery branch finalized as Abandoned without closing the withheld channel or unreserving its inputs, unlike the NoForwards branch and the live AbandonSession path. After a restart where every forward had failed, the channel stayed open and the inputs stayed reserved with the active entry deleted. Close and unreserve before finalizing.
A variable-amount buy fixes no payment_size at buy time, so the policy's min/max_payment_size_msat were only ever checked for fixed-amount buys. The first HTLC's amount is the effective payment size for a var-amount buy; enforce the range against it, failing with unknown_next_peer when it falls outside, so a payment larger than the sized capacity is refused.
Regression guard: a client that funds a JIT channel then withholds channel_ready must not hang the LSP forever. Drives the session into AwaitingChannelReady with channel_ready withheld and asserts the held HTLC is failed back within a bounded time, the session finalizes Failed, and the funding UTXO is released.
5 participants
Important
26.04 FREEZE March 11th: Non-bugfix PRs not ready by this date will wait for 26.06.
RC1 is scheduled on March 23rd
The final release is scheduled for April 15th.
Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:
tools/lightning-downgradeIntroduces a state-machine-based approach to managing LSPS2 JIT channel sessions, replacing the previous ad-hoc state tracking with a structured FSM that tracks payment collection from initial channel open through HTLC forwarding to completion.