Skip to content

Forward channel closures and failed opens to the LSPS2 service - #1233

Open
tnull wants to merge 5 commits into
lightningdevkit:mainfrom
tnull:2026-11-lsps2-forward-channel-closed
Open

Forward channel closures and failed opens to the LSPS2 service#1233
tnull wants to merge 5 commits into
lightningdevkit:mainfrom
tnull:2026-11-lsps2-forward-channel-closed

Conversation

@tnull

@tnull tnull commented Nov 12, 2026

Copy link
Copy Markdown
Collaborator

lightning-liquidity expects LSPS2 services to forward Event::ChannelClosed
and to report failed JIT channel opens. We did neither:

  • Closed JIT channels were never pruned, so the service kept (and persisted)
    the state of every client that ever paid for a channel. That state also
    counts towards the service's global peer limit.
  • If a JIT channel open failed, the intercepted HTLCs were held until they
    timed out, and the JIT state stayed stuck waiting for the channel, so
    retrying the same invoice could never trigger another open.

This PR fixes both, one commit each:

  1. Forward ChannelClosed to LSPS2ServiceHandler::channel_closed.
  2. Call LSPS2ServiceHandler::channel_open_failed when we can't open the
    channel (peer not connected, insufficient on-chain funds, create_channel
    failing) and when the channel closes before becoming ready. The held HTLCs
    are failed back right away and the payer can retry.

tnull added 2 commits November 12, 2026 09:11
@tnull
Forward channel closures to the LSPS2 service
dac4213 
`lightning-liquidity` requires `Event::ChannelClosed` to be forwarded
to the LSPS2 service handler, which we never did. The service hence
kept the state of every JIT channel that ever got paid for, and with
it the persisted state of the respective client, forever. That state
also counts towards the service's global peer limit.

We now hand closed channels to the LSPS2 service, allowing it to prune
the state of JIT channels that are gone.

Co-Authored-By: HAL 9000
@tnull
Fail LSPS2 payments when the channel open fails
3a3dafb 
When we couldn't open a JIT channel as an LSPS2 service, or when the
channel closed before becoming ready, we never told
`lightning-liquidity` about it. The intercepted HTLCs were therefore
held until they timed out, and as the JIT channel state stayed stuck
waiting for the channel, no later payment to the same invoice could
trigger another attempt.

We now report failed channel opens to the LSPS2 service, which fails
the held HTLCs back right away and lets the payer retry.

Co-Authored-By: HAL 9000
@tnull tnull added this to the 0.8 milestone Nov 12, 2026
@tnull
tnull requested a review from jkczyz November 12, 2026 08:49
@ldk-reviews-bot

ldk-reviews-bot commented Nov 12, 2026 •
edited
Loading

Copy link
Copy Markdown

👋 Thanks for assigning @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@tnull
tnull force-pushed the 2026-11-lsps2-forward-channel-closed branch from 049f1a4 to 3a3dafb Compare November 12, 2026 08:49
jkczyz
jkczyz reviewed Nov 12, 2026
View reviewed changes
Comment thread src/liquidity/service/lsps2.rs Resolved
Comment thread src/liquidity/service/lsps2.rs
);
}

if let Err(e) = lsps2_service_handler.channel_closed(channel_id).await {

@jkczyz jkczyz Nov 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 lightning-liquidity's channel_closed only prunes JIT channels in PaymentForwarded. A JIT channel that closes after channel_ready but before the payment is forwarded (PendingPaymentForward / PendingPayment) is still never pruned, and channel_open_failed rejects it too. Is that intended, or a follow-up upstream?

@tnull tnull Nov 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have related PRs open:

Comment thread src/liquidity/service/lsps2.rs Resolved
Comment thread src/liquidity/service/lsps2.rs Resolved
@jkczyz jkczyz mentioned this pull request Nov 13, 2026
Open
1 task
@jkczyz

jkczyz commented Nov 13, 2026

Copy link
Copy Markdown
Contributor

🤖 Two interactions with #999 (same merge base) worth coordinating:

  1. The three new tests call receive_via_jit_channel with the max_total_lsp_fee_limit_msat argument that Add BOLT12 LSPS2 JIT receive support #999 removes. The merge is textually clean (git merge-tree finds no conflict), so whichever PR lands second breaks the build until the calls drop the trailing None.

  2. I built that merged tree and ran the new tests: lsps2_service_prunes_closed_channel_state fails ("the service should prune the state of the closed JIT channel: Elapsed") because Add BOLT12 LSPS2 JIT receive support #999 schedules a lease refill on every JIT receive, and the refilled lease keeps a PendingInitialPayment entry — and with it the peer's persisted state — at the LSP for the 48h parameter validity. The assertion would need to check that the closed channel's SCID mapping is gone rather than that the whole peer state is empty.

A related nuance: the retry these tests demonstrate is BOLT11-only. A BOLT12 payer can't re-send a JIT invoice — the failure is recorded against the invoice's single blinded path, and a new send requests a new invoice — so with #999 the reset SCID is orphaned and keeps counting toward MAX_PENDING_REQUESTS_PER_PEER until its parameters expire. Removing the state instead of resetting it might serve BOLT12 better; possibly an upstream question.

@tnull

tnull commented Nov 13, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Two interactions with #999 (same merge base) worth coordinating:

  1. The three new tests call receive_via_jit_channel with the max_total_lsp_fee_limit_msat argument that Add BOLT12 LSPS2 JIT receive support #999 removes. The merge is textually clean (git merge-tree finds no conflict), so whichever PR lands second breaks the build until the calls drop the trailing None.
  2. I built that merged tree and ran the new tests: lsps2_service_prunes_closed_channel_state fails ("the service should prune the state of the closed JIT channel: Elapsed") because Add BOLT12 LSPS2 JIT receive support #999 schedules a lease refill on every JIT receive, and the refilled lease keeps a PendingInitialPayment entry — and with it the peer's persisted state — at the LSP for the 48h parameter validity. The assertion would need to check that the closed channel's SCID mapping is gone rather than that the whole peer state is empty.

A related nuance: the retry these tests demonstrate is BOLT11-only. A BOLT12 payer can't re-send a JIT invoice — the failure is recorded against the invoice's single blinded path, and a new send requests a new invoice — so with #999 the reset SCID is orphaned and keeps counting toward MAX_PENDING_REQUESTS_PER_PEER until its parameters expire. Removing the state instead of resetting it might serve BOLT12 better; possibly an upstream question.

That just says we need to rebase and resolve conflicts after either PR lands, no?

@tnull
tnull requested a review from jkczyz November 13, 2026 10:21
tnull added 3 commits November 13, 2026 11:23
@tnull
f Test failing payments to a disconnected client
052834d 
Failing the payment back when the client isn't connected was only
covered for the insufficient-funds case, leaving the disconnected-peer
path untested.

Add an integration test that pays a JIT invoice while the client is
offline, expects the payment to fail, and checks that a retry succeeds
once the client is back.

Co-Authored-By: HAL 9000
@tnull
f Notify LSPS2 of closures after queueing
699949a 
We told the LSPS2 service about a closed channel before queueing the
`ChannelClosed` event. If queueing failed, the event was replayed and
the failed channel open reported a second time, which would fail the
HTLCs of a payment the payer retried in the meantime.

Tell the LSPS2 service only once the event was queued, so that it
hears of each closure once.

Co-Authored-By: HAL 9000
@tnull
Fix duplicated "not" in log message
a9629d3 
The log message for a failed JIT channel open to a disconnected peer
read "not being not connected".

Reword it to say that the peer is not connected.

Co-Authored-By: HAL 9000
@tnull
tnull force-pushed the 2026-11-lsps2-forward-channel-closed branch from 73461fa to a9629d3 Compare November 13, 2026 10:25
@jkczyz

jkczyz commented Nov 13, 2026

Copy link
Copy Markdown
Contributor

That just says we need to rebase and resolve conflicts after either PR lands, no?

There won't be a merge conflict if #999 lands first, but there will be a compilation error since it removes an argument from receive_via_jit_channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkczyz jkczyz Awaiting requested review from jkczyz

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

backport 0.7

Projects

None yet

Milestone

0.8

Development

Successfully merging this pull request may close these issues.

3 participants

@tnull @ldk-reviews-bot @jkczyz