Skip to content

Enhance hold TLC settlement handling for received invoices#1289

Open
chenyukang wants to merge 2 commits intodevelopfrom
fix/trampoline-routing-race-same-invoice
Open

Enhance hold TLC settlement handling for received invoices#1289
chenyukang wants to merge 2 commits intodevelopfrom
fix/trampoline-routing-race-same-invoice

Conversation

@chenyukang
Copy link
Copy Markdown
Collaborator

@chenyukang chenyukang commented Apr 20, 2026

Fixes #1161

Fix flaky test_trampoline_routing_race_same_invoice .

  • Add a dedicated SettleReceivedHoldTlcSet path for hold invoices that are already Received and later get a preimage.
  • Keep SettleHoldTlcSet for newly-arrived hold TLCs, and reject them when the invoice is already Received to avoid duplicate same-invoice fulfillment.
  • Route invoice TLCs found by CheckChannels through SettleTlcSetCommand instead of directly fulfilling them with a known preimage.
  • Add unit coverage for both duplicate Received hold TLC rejection and valid post-preimage hold invoice settlement.

For reviwer:
The first commit is bugfix, the second commit is code refactoring.

@chenyukang
Copy link
Copy Markdown
Collaborator Author

cc @doitian

@chenyukang chenyukang force-pushed the fix/trampoline-routing-race-same-invoice branch 2 times, most recently from d210a2f to 6ef6538 Compare April 20, 2026 10:01
@chenyukang chenyukang force-pushed the fix/trampoline-routing-race-same-invoice branch from 6ef6538 to 9479003 Compare April 20, 2026 10:46
Comment thread crates/fiber-lib/src/fiber/network.rs
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses flaky hold-invoice settlement behavior when multiple TLCs race on the same received invoice by introducing a dedicated settlement path for “invoice already Received + preimage arrives later”, and refactors CheckChannels to route invoice TLC settlement through SettleTlcSetCommand rather than directly fulfilling with a preimage.

Changes:

  • Add SettleReceivedHoldTlcSet and refine SettleTlcSetCommand to only allow settling Received invoices on an explicit retry/preimage-reveal path.
  • Refactor NetworkActorCommand::CheckChannels TLC handling into helper functions and route invoice TLCs through SettleTlcSetCommand.
  • Update/extend unit tests to cover duplicate Received hold TLC rejection and valid post-preimage settlement; add extra waits to reduce test flakiness.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/fiber-lib/src/fiber/settle_tlc_set_command.rs Adds allow_received_invoice gating and constructors to distinguish new-hold vs received-hold retry settlement paths.
crates/fiber-lib/src/fiber/network.rs Introduces SettleReceivedHoldTlcSet, refactors CheckChannels TLC processing, and centralizes RemoveTlc sending/applying settlements.
crates/fiber-lib/src/fiber/tests/settle_tlc_set_command_tests.rs Adds/updates tests for rejecting duplicate hold TLCs on Received and allowing settlement after preimage reveal.
crates/fiber-lib/src/fiber/tests/trampoline.rs Updates restart test to use SettleReceivedHoldTlcSet after preimage insertion.
crates/fiber-lib/src/fiber/tests/network.rs Adds waits for graph readiness to reduce timing flakiness in sync tests.

) {
for (_tlc_id, id, payment_hash, is_waiting_forward_result) in committed_tlcs {
if self.store.get_invoice(&payment_hash).is_some() {
if self.store.get_invoice_status(&payment_hash) == Some(CkbInvoiceStatus::Open) {
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

In CheckChannels, invoice TLCs are enqueued for settlement solely based on invoice_status == Open. This includes hold invoices before their preimage is available. When these TLCs are later processed via SettleTlcSet, SettleTlcSetCommand will mark the invoice as Received even though no preimage exists (it will then skip settlement). If the channel actor’s SettleHoldTlcSet for that same hold TLC runs afterwards, it will now see invoice_status == Received and reject the hold TLC set as a duplicate, failing an otherwise valid hold-invoice payment.

To avoid this, CheckChannels should not enqueue Open invoices for settlement unless a preimage is already available (i.e. non-hold invoice), or otherwise explicitly exclude hold-invoice TLCs here and let the hold-TLC paths (SettleHoldTlcSet / SettleReceivedHoldTlcSet via retry_hold_tlc_sets) handle them.

Suggested change
if self.store.get_invoice_status(&payment_hash) == Some(CkbInvoiceStatus::Open) {
if self.store.get_invoice_status(&payment_hash) == Some(CkbInvoiceStatus::Open)
&& self.store.get_preimage(&payment_hash).is_some()
{

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: test_trampoline_routing_race_same_invoice

3 participants