invoices: refactor `InvoiceDB` to eliminate `ScanInvoices` #8081

bhandras · 2023-10-11T06:23:54Z

This PR removes ScanInvoices from InvoiceDB (and the channeldb implementation) and adds two specific methods that can be expressed in SQL more efficiently. Builds on top of #8066 and is a pre-requirement for #8052

The downside of the refactor is that for k/v implementations we'll need to scan invoices twice, but since we're fully migrating to SQL this is just a temporary issue.

Part of #6288

ellemouton

Very slick PR 🔥

Just one comment regarding returning nil instead of an error from DeleteCanceledInvoices so that we dont error on startup if there are no invoices yet.

Other than that - LGTM

channeldb/invoices.go

invoices/invoiceregistry.go

ellemouton

Very nice 🔥 LGTM

yyforyongyu

Very nice refactoring PR! My major comment is to change the deletion behavior of DeleteCanceledInvoices, that it shouldn't perform queries check to return an error and instead should be an noop like other Delete methods.

yyforyongyu · 2023-10-18T12:34:07Z

channeldb/invoice_test.go

+
+	db, err := MakeTestInvoiceDB(t, OptionClock(testClock))
+	require.NoError(t, err, "unable to make test db")
+


can we also add a case here to test the empty slice returned from FetchPendingInvoices?

Yeah good idea! Done.

channeldb/invoice_test.go

yyforyongyu · 2023-10-18T12:46:23Z

channeldb/invoices.go

+			invoiceIndexBucket,
+		)
+		if invoiceIndex == nil {
+			return invpkg.ErrNoInvoicesCreated


Think we should unify the behaviors here - in FetchPendingInvoices we'd ignore this error, and we should. do the same here? In general I think DeleteCanceledInvoices should be a NOOP if there are no invoices in the db, instead of returning an error.

This case is different though right? We do return nil above when we "open" the read-write bucket invoices however if that exist, we should assume that both the the invoice index and add index exists.

could it be the case that this is just an empty bucket? like all the invoices are deleted?

No, if the bucket is empty we'd still get back the handle.

Cool - then I'm curious in what case will this ErrNoInvoicesCreated be returned?

I think it's there to give us some reassurance that about expected existence of those buckets. Similar checks are done for other channeldb methods too.

cool then I interpret it as a db corruption check.

yyforyongyu · 2023-10-18T12:52:23Z

channeldb/invoices.go

@@ -2761,6 +2761,102 @@ func delAMPSettleIndex(invoiceNum []byte, invoices,
 	return nil
 }

+// DeleteCanceledInvoices deletes all canceled invoices from the database.
+func (d *DB) DeleteCanceledInvoices(_ context.Context) error {
+	return kvdb.Update(d, func(tx kvdb.RwTx) error {


can we use batch here?

I think it's probably not a good idea since we scan through all the invoices here and bolt's Batch method assumes that the closure can be executed multiple times since it'll only try to parallelize optimistically (given bolt has no idea about what we do here).

I think it depends on whether there's an error and how we handle the error, from this bench test, if we distinguish the errors here, each closure is executed exactly once.

yyforyongyu · 2023-10-18T12:56:29Z

channeldb/invoice_test.go

@@ -3092,6 +3079,65 @@ func TestDeleteInvoices(t *testing.T) {
 	assertInvoiceCount(0)
 }

+// TestDeleteCanceledInvoices tests that deleting canceled invoices with the
+// specific DeleteCanceledInvoices method works correctly.
+func TestDeleteCanceledInvoices(t *testing.T) {


I think it'd be great if we could add tests to cover more branches in this method, but guess it's too much work as most stuff here is not mocked.

Yeah, maybe we should return to the test coverage question later on once we have both SQL and KV implementations run against this test suite since I'm afraid many of the branches we'd test will go away as we deprecate the KV implementation. Similarly TestDeleteInvoices is also very barebones, so perhaps a bigger overhaul is necessary.

…s method

In this commit we change how we select invoices to follow or delete when starting the InvoiceRegistry to instead of using the deperecated scan func from channeldb, use specific functions to gather pending and delete canceled invoices.

yyforyongyu

LGTM! Also love the green CI!

saubyk

Ack

bhandras added this to the v0.18.0 milestone Oct 11, 2023

bhandras force-pushed the invoicedb-refactor branch from 113b633 to 7151759 Compare October 11, 2023 12:42

Roasbeef requested review from yyforyongyu and ellemouton October 11, 2023 18:11

saubyk assigned bhandras Oct 12, 2023

ellemouton reviewed Oct 17, 2023

View reviewed changes

channeldb/invoices.go Outdated Show resolved Hide resolved

channeldb/invoices.go Outdated Show resolved Hide resolved

channeldb/invoices.go Outdated Show resolved Hide resolved

invoices/invoiceregistry.go Show resolved Hide resolved

invoices/invoiceregistry.go Show resolved Hide resolved

bhandras mentioned this pull request Oct 17, 2023

channeldb: refactor UpdateInvoice to make it simpler to create SQL specific implementation #8100

Merged

bhandras force-pushed the invoicedb-refactor branch from 7151759 to 8f051f3 Compare October 17, 2023 19:11

bhandras requested a review from ellemouton October 17, 2023 19:12

ellemouton approved these changes Oct 18, 2023

View reviewed changes

yyforyongyu requested changes Oct 18, 2023

View reviewed changes

bhandras added 5 commits October 18, 2023 16:15

channeldb: add k/v implementation for InvoiceDB.FetchPendingInvoices

1f8065d

channeldb: add k/v implementation for InvoiceDB.DeleteCanceledInvoice…

8ab267a

…s method

invoices: remove the now unused ScanInvoices method

8bf7503

docs: update release notes for 0.18.0

bedafca

bhandras force-pushed the invoicedb-refactor branch from 8f051f3 to bedafca Compare October 18, 2023 14:16

bhandras requested a review from yyforyongyu October 18, 2023 14:16

yyforyongyu approved these changes Oct 18, 2023

View reviewed changes

bhandras enabled auto-merge October 18, 2023 17:19

saubyk approved these changes Oct 19, 2023

View reviewed changes

guggero disabled auto-merge October 19, 2023 09:27

guggero merged commit 812f4d9 into lightningnetwork:master Oct 19, 2023
24 of 25 checks passed

bhandras deleted the invoicedb-refactor branch October 19, 2023 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

invoices: refactor `InvoiceDB` to eliminate `ScanInvoices` #8081

invoices: refactor `InvoiceDB` to eliminate `ScanInvoices` #8081

bhandras commented Oct 11, 2023 •

edited

Loading

ellemouton left a comment •

edited

Loading

ellemouton left a comment

yyforyongyu left a comment

yyforyongyu Oct 18, 2023

bhandras Oct 18, 2023

yyforyongyu Oct 18, 2023

bhandras Oct 18, 2023

yyforyongyu Oct 18, 2023

bhandras Oct 18, 2023

yyforyongyu Oct 18, 2023

bhandras Oct 18, 2023

yyforyongyu Oct 18, 2023

yyforyongyu Oct 18, 2023

bhandras Oct 18, 2023

yyforyongyu Oct 18, 2023

yyforyongyu Oct 18, 2023

bhandras Oct 18, 2023

yyforyongyu left a comment

saubyk left a comment


		db, err := MakeTestInvoiceDB(t, OptionClock(testClock))
		require.NoError(t, err, "unable to make test db")

invoices: refactor InvoiceDB to eliminate ScanInvoices #8081

invoices: refactor InvoiceDB to eliminate ScanInvoices #8081

Conversation

bhandras commented Oct 11, 2023 • edited Loading

ellemouton left a comment • edited Loading

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

saubyk left a comment

Choose a reason for hiding this comment

invoices: refactor `InvoiceDB` to eliminate `ScanInvoices` #8081

invoices: refactor `InvoiceDB` to eliminate `ScanInvoices` #8081

bhandras commented Oct 11, 2023 •

edited

Loading

ellemouton left a comment •

edited

Loading