fix: intercept self hold payments based on invoice rather than payment hash

[rolznz]

This PR also fixes a bug where we will fail saying the invoice has already been paid (I think would block wrapped invoices working with LND right now)

TODOs

• undo expiry hack and regenerate invoices
• test in sandbox locally (both wrapped and normal HOLD invoices)
• test with LND (Polar - tested with JS SDK: chore: add example for wrapped invoices js-sdk#529)
• test standard HOLD invoices with LDK (Mutinynet) NOTE: wrapped invoices are not supported yet by LDK-Node.
• test keysend self-payments

Follow ups - move to new issue:

• update Alby Hub to support setting the min_final_cltv_expiry_delta via nwc (needs exposing in our ldk node fork) - moved to Fully Implement NWC HOLD invoices spec #2065

Summary by CodeRabbit

• Bug Fixes

  • Improved hold invoice tracking and self-payment handling by refining internal payment identification mechanisms.
  • Enhanced accuracy of payment state transitions and deduplication for complex payment scenarios.

• Tests

  • Added comprehensive test coverage for wrapped invoice and multi-app payment flows.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR refactors the payment handling system to use payment request identifiers instead of payment hashes for hold invoice self-payments. It introduces timeout constants, enhances mock testing infrastructure, and adds comprehensive test coverage for wrapped invoice flows across multiple interacting applications.

Changes

Cohort / File(s)	Summary
Timeout Configuration lnclient/lnd/lnd.go	Introduces package-level constant SEND_PAYMENT_TIMEOUT = 50 and applies it to SendPaymentSync and SendKeysend operations, removing hardcoded timeout values.
Testing Infrastructure tests/mock_ln_client.go	Adds MakeInvoiceResponses and MakeInvoiceErrors fields to MockLn struct, enabling queued response behavior for both MakeInvoice and MakeHoldInvoice methods in test scenarios.
Event Consumer Refactoring transactions/hold_invoice_self_payment_consumer.go	Replaces PaymentHash with PaymentRequest identifier throughout the hold invoice updated consumer, changing event routing logic to reference the new PaymentRequest field on db.Transaction.
Core Service Logic transactions/transactions_service.go	Extensively refactors payment handling to use payment_request as the primary identifier for hold invoice self-payments; updates method signatures (markHoldInvoiceAccepted, interceptSelfPayment, interceptSelfHoldPayment), DB query conditions, and log field references; extends settlement deduplication logic to include PaymentRequest.
Test Data Updates transactions/payments_test.go	Augments test fixtures in TestSendPaymentSync_Duplicate_AlreadyPaid and TestSendPaymentSync_Duplicate_Pending to include PaymentRequest field alongside existing PaymentHash and AmountMsat fields.
Test Coverage Expansion transactions/self_hold_payments_test.go	Adds new comprehensive test TestWrappedInvoice demonstrating end-to-end flow where Alice pays Bob's wrapped invoice, Bob settles via Charlie's invoice, and verifies hold/settlement states and self-payment flags across multiple app instances.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Poem

🐰 A payment request hops where hashes once danced,
Through invoices wrapped, self-payments enhance,
From Charlie to Bob to Alice they leap,
In timeouts we trust, in requests run deep! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: refactoring self hold payment interception to use invoice/payment request instead of payment hash.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/self-payments-wrapped-invoice

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[rolznz]

@im-adithya @frnandu in the latest version of Polar (LND v20) payments were failing until I set a timeout seconds (which should have been set anyway. I set it to 50s)

[im-adithya]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[im-adithya]

utACK

[im-adithya]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

🧹 Nitpick comments (3)

transactions/self_hold_payments_test.go (2)

254-256: time.Sleep for synchronization can cause flaky tests.

Using time.Sleep(10 * time.Millisecond) to wait for Alice's payment to be accepted is timing-dependent and could fail on slower CI environments. As the TODO suggests, consider using event-based synchronization or a channel with timeout.

♻️ Example approach using channels

// Consider adding a channel to MockLn or using event subscription
// to wait for the hold invoice accepted state rather than sleeping

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transactions/self_hold_payments_test.go` around lines 254 - 256, The test
uses time.Sleep(10 * time.Millisecond) for synchronization which is flaky;
replace the sleep by waiting on an event or channel from the mock Lightning node
instead: add an event subscription or channel on MockLn (e.g., a
NotifyInvoiceAccepted / InvoiceAcceptedCh) and have the test block on that with
a timeout (context.WithTimeout) to fail reliably if the accept event never
arrives; update the test (transactions/self_hold_payments_test.go) to wait on
that channel or call MockLn.WaitForInvoiceAccepted (or similar) instead of
time.Sleep.

---

194-200: Mock preimage doesn't match Charlie's invoice preimage.

The PayInvoiceResponses use MockLNClientHoldTransaction.Preimage, which differs from Charlie's actual invoice preimage (fcf200c74d9900dc77af17eb1f57c02eec0f94b5b169d3eee23df9a216a3411b). While acceptable for testing the self-payment flow mechanics, this means the test doesn't validate the full preimage chain from Charlie → Bob → Alice settlement.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transactions/self_hold_payments_test.go` around lines 194 - 200, The test
currently sets preimages = []string{tests.MockLNClientHoldTransaction.Preimage,
...} and configures svc.LNClient.(*tests.MockLn).PayInvoiceResponses with those
values, but that preimage does not match Charlie’s real invoice preimage
(fcf200c74d9900dc77af17eb1f57c02eec0f94b5b169d3eee23df9a216a3411b), so update
the mock to use Charlie’s preimage (or include it as the first element) when
constructing preimages and PayInvoiceResponses in the test (reference variables:
preimages, tests.MockLNClientHoldTransaction.Preimage,
svc.LNClient.(*tests.MockLn).PayInvoiceResponses) so the test validates the full
preimage chain Charlie → Bob → Alice.

tests/mock_ln_client.go (1)

80-81: Shared queue for MakeInvoice and MakeHoldInvoice may cause confusion.

Both MakeInvoice and MakeHoldInvoice consume from the same MakeInvoiceResponses queue. While this works for the current TestWrappedInvoice test where both are queued in order, it could lead to subtle bugs if tests inadvertently interleave calls. Consider whether separate queues (e.g., MakeHoldInvoiceResponses) would provide clearer test intent.

Also applies to: 122-141

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/mock_ln_client.go` around lines 80 - 81, The tests share a single
response/error queue (MakeInvoiceResponses and MakeInvoiceErrors) for both
MakeInvoice and MakeHoldInvoice which can lead to ordering bugs; split them by
adding distinct MakeHoldInvoiceResponses and MakeHoldInvoiceErrors slices and
update the mock methods MakeInvoice and MakeHoldInvoice to consume from their
respective queues (preserving current behaviour for MakeInvoice by leaving it
unchanged) and update any test setup in tests that rely on interleaving to push
responses into the new MakeHoldInvoiceResponses queue so each API uses its own
explicit queue.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/mock_ln_client.go`:
- Around line 80-81: The tests share a single response/error queue
(MakeInvoiceResponses and MakeInvoiceErrors) for both MakeInvoice and
MakeHoldInvoice which can lead to ordering bugs; split them by adding distinct
MakeHoldInvoiceResponses and MakeHoldInvoiceErrors slices and update the mock
methods MakeInvoice and MakeHoldInvoice to consume from their respective queues
(preserving current behaviour for MakeInvoice by leaving it unchanged) and
update any test setup in tests that rely on interleaving to push responses into
the new MakeHoldInvoiceResponses queue so each API uses its own explicit queue.

In `@transactions/self_hold_payments_test.go`:
- Around line 254-256: The test uses time.Sleep(10 * time.Millisecond) for
synchronization which is flaky; replace the sleep by waiting on an event or
channel from the mock Lightning node instead: add an event subscription or
channel on MockLn (e.g., a NotifyInvoiceAccepted / InvoiceAcceptedCh) and have
the test block on that with a timeout (context.WithTimeout) to fail reliably if
the accept event never arrives; update the test
(transactions/self_hold_payments_test.go) to wait on that channel or call
MockLn.WaitForInvoiceAccepted (or similar) instead of time.Sleep.
- Around line 194-200: The test currently sets preimages =
[]string{tests.MockLNClientHoldTransaction.Preimage, ...} and configures
svc.LNClient.(*tests.MockLn).PayInvoiceResponses with those values, but that
preimage does not match Charlie’s real invoice preimage
(fcf200c74d9900dc77af17eb1f57c02eec0f94b5b169d3eee23df9a216a3411b), so update
the mock to use Charlie’s preimage (or include it as the first element) when
constructing preimages and PayInvoiceResponses in the test (reference variables:
preimages, tests.MockLNClientHoldTransaction.Preimage,
svc.LNClient.(*tests.MockLn).PayInvoiceResponses) so the test validates the full
preimage chain Charlie → Bob → Alice.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2174f02 and fd56016.

📒 Files selected for processing (6)

• lnclient/lnd/lnd.go
• tests/mock_ln_client.go
• transactions/hold_invoice_self_payment_consumer.go
• transactions/payments_test.go
• transactions/self_hold_payments_test.go
• transactions/transactions_service.go

[im-adithya]

Comments are only on tests, LGTM!