[Part 7|*] Addressing TODOs left from the SQL payments implementation #10373

ziggie1984 · 2025-11-15T07:53:21Z

This PR cleansup some TODOs of the payment sql series, before tackling the dispatcher and the migration.

ziggie1984 · 2025-11-24T19:37:06Z

Converage of the SQL store after this PR is merged:

ok  	github.com/lightningnetwork/lnd/payments/db	7.066s	coverage: 36.0% of statements
github.com/lightningnetwork/lnd/payments/db/sql_store.go:115:			NewSQLStore				66.7%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:141:			fetchPaymentWithCompleteData		80.0%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:167:			batchLoadPayments			71.4%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:199:			batchLoadpaymentsBaseData		83.3%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:264:			batchLoadPaymentCustomRecords		100.0%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:298:			batchLoadHtlcAttempts			100.0%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:331:			batchLoadHopsForAttempts		100.0%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:365:			batchLoadHopCustomRecords		100.0%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:399:			batchLoadRouteCustomRecords		42.9%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:438:			batchLoadPaymentResolutions		81.8%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:486:			loadPaymentResolutions			75.0%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:503:			computePaymentStatusFromResolutions	100.0%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:545:			batchLoadPaymentDetailsData		72.7%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:625:			buildPaymentFromBatchData		92.3%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:724:			QueryPayments				92.0%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:896:			fetchPaymentByHash			83.3%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:920:			FetchPayment				91.7%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:970:			FetchInFlightPayments			91.9%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:1100:			DeleteFailedAttempts			92.3%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:1137:			computePaymentStatusFromDB		75.0%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:1189:			DeletePayment				86.7%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:1250:			InitPayment				78.6%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:1357:			insertRouteHops				46.9%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:1481:			RegisterAttempt				75.8%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:1607:			SettleAttempt				81.0%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:1680:			FailAttempt				65.4%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:1767:			Fail					81.0%
github.com/lightningnetwork/lnd/payments/db/sql_store.go:1851:			DeletePayments				90.9%

A context is added to failPaymentAndAttempt and its dependant function calls.

We make the TestDeleteNonInFlight and separate all the logic out for the duplicate payment test case. The deletion of duplicate payments is now tested in isolation only for the kv backend.

Previously we had db and application logic mixed on the db level. We now move the config option KeepFailedPaymentAttempts to the ChannelRouter level and move it out of the db level.

We add a test which tests the retrieval of first hop data like the first hop amount or the custom records on the route level.

We add a couple of additional tests to increase the unit test coverage of the sql store but also the kv store. We only create db agnostic unit tests so both backends are tested effectively.

yyforyongyu

LGTM ✈️

We still have this question from Part 6, where we need to decide whether we need to use a timeout ctx or not?

yyforyongyu · 2025-12-02T13:11:46Z

routing/router.go

 	log.Debugf("Scanning finished, found %d inflight payments",
 		len(payments))

+	// TODO(ziggie): Also check for payments which have no HTLCs at all


Why do we want to delete it? I think the payment in this case can be retried?

so currently we would not allow to start a new payment when the payment has no attempts attached to it. We could hit this error when checking the status of the payment:

lnd/payments/db/payment_status.go

Line 246 in 2047348

case !htlcFailed:

Which when calling initializable => would give us an error.

But I would prefer to delete them to garbage collect them, in case the user never retries this payment.

routing/payment_lifecycle.go

routing/payment_lifecycle_test.go

yyforyongyu · 2025-12-02T13:15:09Z

routing/payment_lifecycle_test.go

+
+			// Conditionally expect DeleteFailedAttempts to be
+			// called based on the configuration.
+			if tc.expectDeleteCalled {


nit: can use !tc.keepFailedPaymentAttempts

ellemouton

lgtm!

routing/payment_lifecycle.go

ziggie1984 · 2025-12-03T18:46:04Z

Re the ctx, in general for db write txns, we always want it to be finished. And for reads, we want it to be canceled if the parent ctx is canceled. We could rename cleanupCtx to be writeCtx to make it clear, or create a struct to hold two contexts - one from the caller, and one inherits the caller ctx which is used for writing. Tho I don't think it's urgent.

So I don't think we can go in this direction, because at the end if something fails in this loop, we would exit it, and then we would not resolve potential infight attempts.

One last thing is what Elle has brought up before, that for the inherited context, we should add a timeout, just in case the db is locked, we need to funnel that error to the caller. But this is an open question, as we have a config for db timeout already, so I wander if we should use that here. There's also the question that, if the postgres is configured with a timeout, it seems unnecessary to add a timeout to the ctx as the call will return an error anyway?

So currently for the native payment sql store we do not use the timeout value for the transactions, we thread the context from upper level callers, and the intial context has no timeout value (mostly a context.TODO()). Given that concurrent access to a single payment is not really possible (because we would not allow another payment before the old one is not in a terminal state) there is not much risk in adding a timeout in the cleanupCtx. Only cases currently when this call would hang is, that our db probably freezes, but then I would say we have more problems across the daemon not just cleaning up this payment. So I think our current assumption of the codebase is that db transactions should always succeed to guarantee a consistent db state and application code state but probably worth reinvestigating the context question and the transaction timeout for the native sql architecture.

ziggie1984 changed the base branch from master to elle-payment-sql-series-new November 15, 2025 07:53

ziggie1984 added payments Related to invoices/payments sql labels Nov 15, 2025

ziggie1984 added this to v0.21 Nov 15, 2025

ziggie1984 added this to the v0.21.0 milestone Nov 15, 2025

ziggie1984 self-assigned this Nov 15, 2025

ziggie1984 moved this to Ready in v0.21 Nov 15, 2025

ziggie1984 force-pushed the introduce-sql-schema-payments-part-7 branch from e00c320 to 4f1babb Compare November 18, 2025 00:06

ziggie1984 added the no-changelog label Nov 18, 2025

ziggie1984 force-pushed the introduce-sql-schema-payments-part-7 branch from 4f1babb to a6c9e12 Compare November 18, 2025 08:15

ziggie1984 marked this pull request as ready for review November 18, 2025 08:16

ziggie1984 moved this from Ready to In progress in v0.21 Nov 18, 2025

ziggie1984 force-pushed the elle-payment-sql-series-new branch 2 times, most recently from 47606dd to 01d9986 Compare November 20, 2025 13:26

ziggie1984 mentioned this pull request Nov 20, 2025

[feature]: Introduce Payment SQL storage #9863

Open

8 tasks

ziggie1984 force-pushed the introduce-sql-schema-payments-part-7 branch from a6c9e12 to d543390 Compare November 24, 2025 19:35

ziggie1984 mentioned this pull request Nov 24, 2025

[Part 4|*] Add unit tests for the previous introduced SQL Backend methods #10292

Merged

ziggie1984 added 12 commits November 27, 2025 08:45

routing: Add context to requestRoute

ae9c31f

multi: thread context through payment lifecyle functions

d11b762

routing: Thread context through failPaymentAndAttempt

91fd00a

A context is added to failPaymentAndAttempt and its dependant function calls.

routing: add context to failAttempt

ea33726

routing: add context to reloadInflightAttempts

abcd834

routing: add context to reloadPayment method

6aefe5b

multi: thread context through SendPayment

b60e698

docs: add release-notes

aae5a33

paymentsdb: make delete payments test db agnostic

e3d8d23

We make the TestDeleteNonInFlight and separate all the logic out for the duplicate payment test case. The deletion of duplicate payments is now tested in isolation only for the kv backend.

routing: add TODO to also delete payments without HTLCs

f4a7dae

multi: move failed attempt cfg option to the router subsytem

4f2d1bc

Previously we had db and application logic mixed on the db level. We now move the config option KeepFailedPaymentAttempts to the ChannelRouter level and move it out of the db level.

paymentsdb: refactor test helpers

967977a

ziggie1984 added 3 commits November 28, 2025 09:08

paymentsdb: add additional test for first hop data

7d89e21

We add a test which tests the retrieval of first hop data like the first hop amount or the custom records on the route level.

paymentsdb: add more unit tests to increase coverage

3c6f16b

We add a couple of additional tests to increase the unit test coverage of the sql store but also the kv store. We only create db agnostic unit tests so both backends are tested effectively.

docs: add release-notes

fb82814

ziggie1984 force-pushed the introduce-sql-schema-payments-part-7 branch from 2e6f53f to fb82814 Compare November 28, 2025 08:09

ziggie1984 requested review from ellemouton and yyforyongyu November 28, 2025 08:12

ziggie1984 moved this from In progress to In review in v0.21 Nov 28, 2025

yyforyongyu approved these changes Dec 2, 2025

View reviewed changes

ellemouton approved these changes Dec 3, 2025

View reviewed changes

routing/payment_lifecycle.go Show resolved Hide resolved

ziggie1984 merged commit d8531d2 into lightningnetwork:elle-payment-sql-series-new Dec 3, 2025
39 checks passed

github-project-automation bot moved this from In review to Done in v0.21 Dec 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Part 7|*] Addressing TODOs left from the SQL payments implementation #10373

[Part 7|*] Addressing TODOs left from the SQL payments implementation #10373

Uh oh!

ziggie1984 commented Nov 15, 2025 •

edited

Loading

Uh oh!

ziggie1984 commented Nov 24, 2025

Uh oh!

yyforyongyu left a comment

Uh oh!

yyforyongyu Dec 2, 2025

Uh oh!

ziggie1984 Dec 3, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

yyforyongyu Dec 2, 2025

Uh oh!

ellemouton left a comment

Uh oh!

Uh oh!

ziggie1984 commented Dec 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Part 7|*] Addressing TODOs left from the SQL payments implementation #10373

[Part 7|*] Addressing TODOs left from the SQL payments implementation #10373

Uh oh!

Conversation

ziggie1984 commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ziggie1984 commented Nov 24, 2025

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

yyforyongyu Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

ziggie1984 Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yyforyongyu Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ziggie1984 commented Dec 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ziggie1984 commented Nov 15, 2025 •

edited

Loading

ziggie1984 Dec 3, 2025 •

edited

Loading