Support client_trusts_lsp on LSPS2

[martinsaposnic]

The feature

lightningdevkit/ldk-node#479

Currently, our LSPS2 service implementation only supports the lsp_trusts_client model, which means that "If the client does not claim the payment, and the funding transaction confirms, then the LSP will have its funds locked in a channel that was not paid for."

On a client_trusts_lsp model, the LSP will NOT broadcast the funding transaction until the client claims the payment.

The plan:

(This plan was validated and acknowledged by @tnull in private). There are differences between the plan and the implementation, but it roughly describes the approach.

• LSPS2 Process & Events:
These are handled the same way as before. No changes here.

• When the OpenChannel event is emitted, ldk-node calls create_channel as usual. The key difference is: If client_trusts_lsp = true, after emitting the OpenChannel event, we start tracking the HTLC that our client will eventually claim.

• Funding Transaction Broadcast Logic:
The batch_funding_transaction_generated_intern function decides whether the funding transaction should be broadcast automatically. There are two existing funding types:

  • Checked: Validates and auto-broadcasts.
  • Unchecked: Skips validation and requires manual broadcast.

  I will introduce a third type:

  • CheckedManualBroadcast: Validates, but sets manual broadcast.

• With this:

  • If client_trusts_lsp = false, ldk-node uses funding_transaction_generated (default behavior).
  • If client_trusts_lsp = true, it uses funding_transaction_generated_with_delayed_broadcast, which sets the funding type to CheckedManualBroadcast (validates but sets manual broadcast).

• lsps2_service on
ldk-node will now interact with lsps2_service on rust-lightning in two new key moments:

  • When it receives the FundingTxBroadcastSafe event, it notifies lsps2_service on rust-lightning, which marks a flag as true.
  • When the client claims the HTLC (::PaymentClaimed), ldk-node notifies lsps2_service on rust-lightning, which marks another flag as true.
  • Once both flags are true, we are ok to proceed to broadcast the funding transaction.

Changes:

• a new FundingType called CheckedManualBroadcast, which behaves the same as the FundingType::Checked, but it returns false when is_manual_broadcast is called. Essentially, it validates everything (same as the FundingType::Checked) but it does not automatically broadcast (same as FundingType::Unchecked)
• new public function funding_transaction_generated_manual_broadcast on channel_manager. Uses FundingType::CheckedManualBroadcast, which validates but does not automatically broadcast
• new public function channel_needs_manual_broadcast. This is used by ldk-node to know if funding_transaction_generated or funding_transaction_generated_manual_broadcast should be called when FundingGenerationReady event is triggered
• new public function store_funding_transaction. This is used by ldk-node when the funding transaction is created. We need to store it because the broadcast of the funding transaction may be deferred.
• new public function funding_tx_broadcast_safe. This is used by ldk-node when the FundingTxBroadcastSafe event is triggered
• a new enum TrustModel is introduced, which depending on the trust model that the user chose, it will have a few properties to track:
  • if the client claimed the htlc,
  • the status of funding_tx_broadcast_safe.
  • hold the funding_tx for later use
• This new TrustModel enum will be created when the OpenChannel is triggered, and will be saved in the OutboundJITChannelState
• Every time the TrustModel changes values, the function broadcast_transaction_if_applies is called, so it checks if it's time to broadcast the funding transaction
• An unsafe_broadcast_transaction function is also introduced on channel_manager, which will be called by broadcast_transaction_if_applies from the lsps2/service

LDK Node integration

In this PR lightningdevkit/ldk-node#572 on ldk-node, you can see that 2 tests are created that demonstrates the funcionality described above.

• First test:

client_trusts_lsp=true

In the test, receive_via_jit_channel_manual_claim is called, the mempool is checked to assert that the funding transaction was not broadcasted yet (it should not because client_trusts_lsp=true, and the client has not claimed the htlc yet).

Then the client calls claim_for_hash, and the mempool is checked again, and now the funding transaction should be there

• Second test:

client_trusts_lsp=false

In the test, receive_via_jit_channel_manual_claim is called, the mempool is checked to assert that the funding transaction was broadcasted (because the LSP trusts the client), even though the client has not claimed the htlc yet. In this case, the LSP was tricked, and it will have its funds locked in a channel that was not paid for.

Side note: for the tests to work I had to create a new receive_via_jit_channel_manual_claim so the client can manually claim the htlc using the claim_for_hash.

[ldk-reviews-bot]

👋 Thanks for assigning @tnull 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.

[codecov]

Codecov Report

Attention: Patch coverage is 21.93548% with 242 lines in your changes missing coverage. Please review.

Project coverage is 89.69%. Comparing base (0848e7a) to head (a038ab6).

Files with missing lines	Patch %	Lines
lightning-liquidity/src/lsps2/service.rs	22.83%	222 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	9.52%	18 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3838      +/-   ##
==========================================
- Coverage   89.88%   89.69%   -0.19%     
==========================================
  Files         160      160              
  Lines      129654   129945     +291     
  Branches   129654   129945     +291     
==========================================
+ Hits       116534   116551      +17     
- Misses      10425    10687     +262     
- Partials     2695     2707      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[martinsaposnic]

A few extra concerns:

HTLCs routed over 0-conf channels might hit CLTV timeouts if the channel should be confirmed but isn’t yet, so if the user takes forever to claim the payment, then there could be some trouble there.

Also, I'm not sure if it would be better to have new possible states to explicitly model the idea of having sent the channel_ready but the funding_tx is not broadcasted yet. Also sharing the trust_model state between 4 of the 5 possible states is not something I'm convinced. I have a few different options for this, but I'm open to comments and suggestions

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.