[WIP] Invocies: refactor `InvoiceDB` methods #7542

positiveblue · 2023-03-27T15:27:18Z

This PR includes multiple changes. Some of them are specific to the InvoiceDB, some to the Invoice domain logic in general and others to the new DB interfaces.

Changes to the InvoiceDB:
- New set of methods that remove most of the domain logic from the db implementation. The new methods are "dumber" and they are supposed to be used to simply store/retrieve/delete data from the db.
- For each new method two versions have been added. One with a explicit tx and another one without it. When the new set of interfaces for storage (last commit) is finished only the ones without explicit tx will survive (WitTx will also be removed)

Changes to the Invoice domain logic:
- Most of the logic to check if an invoice could be updated and what changes where needed to be applied given a new event and the current invoice status have been removed from the channeldb package.
- In order to add an htlc, settle an invoice or canceling it we can use now its struct methods. Those methods implement the flow for all the invoice types (regular, hodl, amp...). We may want to add a new interface for invoices in the future. Until then most of the logic is separated in specific methods for each type while the public method simply implements acts as dispatcher.

New DB interfaces:
- The idea is to have a TxExecutor that is able to bind a tx to a db interface and injects it to the users custom logic. This is similar to what we have in other projects (but generic not only for sql/sqlc). You can check the latest commit for more.

Part of #6288

bitromortac

Have done a high-level review up to invoiceregistry: refactor CancelInvoice. I like the work towards a more CRUD-based approach 🥇 , breaking the interface methods apart, thereby decoupling concerns.
I'm not sure about duplication of the interface methods, although I can understand the usability concerns, but probably would prefer to have only the tx-based ones.

bitromortac · 2023-04-05T11:55:07Z

database/tx.go

+	Rollback() error
+}
+
+type TxExecutor interface {


This could already use a docstring to make the motivation clearer

bitromortac · 2023-04-05T12:03:14Z

invoices/interface.go

+	//
+	// NOTE: This function will be executed in its own transaction and will
+	// use the default timeout.
+	AddInvoice(invoice *Invoice) error


If those methods stay, could they be implemented by wrapping the tx-based one with WithTx?

The WithTx will be removed from the interface. Under the hood they will be implemented with the WithTx though

invoices/interface.go

bitromortac · 2023-04-05T13:31:17Z

channeldb/invoices.go

-// AddIndex on newInvoice.
-func (d *DB) AddInvoice(newInvoice *invpkg.Invoice, paymentHash lntypes.Hash) (
-	uint64, error) {
+// WithTx is a helper method that allows callers to execute a function within a


nit: commit message should start with channeldb+invoices

This is actually I doubt that I have because i am not sure if we can use the "component" that we are updating in the commit message instead of going "folder/package" based.

channeldb/invoices.go

bitromortac · 2023-04-06T12:12:07Z

channeldb/invoices.go

+
+			// To ensure consistency check that the already fetched
+			// invoice key matches the one in the settle index.
+			key := settleIndex.Get(settleIndexKey[:])


Can settleIndex be nil (the original code had a nil check, see also the comment above settleIndex)?

not here. Before it could be nil because i was a point to a ref. Now we are using the value of an uint64, which will always have a value. The value 0 is considered "nil" or not-set yet

channeldb/invoice_test.go

invoices/invoiceregistry.go

The `DBTx` interface abstracts database transactions across different backends.

The new field will be used in cases where we do not know the preimage of an invoice, like in hodl invoices.

In the next commits we will move each one of the methods in the temporary interface to `InvoiceDB`. This commit exists as a "declaration of intentions" for the reviewers. The new interface improves the API design and adds context support For each operation we have two versions: one method that executes the changes in a tx with only that query and a method that allows to execute those operations in a given transaction.

The new method assumes that we can always get the payment hash from the invoice itself, using the preimage/hash in the contract terms. That allows us to have less parameters in the method but we need to ensure that `Terms.PaymentHash` is not nil when `Terms.PaymentPreimage` is nil. We do not return the addIndex anymore but directly modify the passed parameter, as before.

This method will substitute the old `LookupInvoice`. The only difference is that `HtlcSetOnlyModifier` is not needed anymore to signal that you want to fetch only the htlcs related to that SetID. Using `GetInvoice` with a SetID it will always return only the htlcs related to that SetID. For fetching an AMP invoice with all its htlcs, an InvRef with the relevant `PayAddr` needs to be used. Another change is that if an `InvRef` refears a SetID that does not exist the `GetInvoice` function will not fail automatically but will try to fetch the invoice using other params like `PayAddr`. The rational behind this is that when we are adding an htlc to an AMP invoice we want to fetch the invoice and the relevant SetID. If this is the first htlc for that set, we want the invoice with no htlcs.

The new method uses the normal `InvoiceRef` to know what invoice needs to be deleted.

Many times adding a new htlc to an invoice does not trigger an update in the invoice other than adding the htlc to the htlc set. In other cases, adding the HTLC settles/cancels the invoice. This new method helps domain logic to make this a two step process: add a new htlc and later on if the invocie is settle/canceled update the invoice and all the relevant htlcs. Because right now the htlcs are stored serialized with the invocie this method duplicates some logic that will live in the domain layer (ex: `invoice.AddHTLC`)

The methods has been renamed to `UpdateInvoiceV2` to avoid breaking specific unit tests for the db part (store/retrieve).

In this case the implementation can be a noop method. That's because everytime that we update the htlcs of an invoice we will also call the `UpdateInvoice`. Because of how we were serializing/storing invocies in channeldb, updating an invoice will also store the htlcs.

Put all the domain logic to perform different action to an invoice in invoice methods.

Use the new invoicedb interface.

Roasbeef · 2023-04-12T17:58:05Z

invoices/interface.go

+	//
+	// TODO(positiveblue): delete this method from this interface and use
+	// something like the executor patter that we use in other projects.
+	WithTx(ctx context.Context, funcs ...func(tx database.DBTx) error) error


I think this can be removed now?

Roasbeef · 2023-04-12T17:59:05Z

invoices/interface.go

+		htlc *InvoiceHTLC) error
+
+	// AddHtlcWithTx adds new htlc to an existing invoice.
+	AddHtlcWithTx(ctx context.Context, dbTx database.DBTx, ref InvoiceRef,


Also we no longer need any of the WithTx suffix methods, right?

bhandras

My main comment is compatibility with existing backends and also that currently the diff is rather large and it's hard to tell which lines are new which are just copied over. Could the PR be redone such that the diff is focused on the changed parts while still remains compatible? Is the plan to use existing test coverage to cover all of this PR or will there be new test coverage added too?

bhandras · 2023-04-17T12:51:16Z

invoices/invoices.go

@@ -262,6 +262,11 @@ type ContractTerm struct {
 	// extended. Set to nil if the preimage isn't known yet.
 	PaymentPreimage *lntypes.Preimage

+	// PaymentHash is the hash of the preimage associated to this invoice.


nit: associated to => with

bhandras · 2023-04-17T13:02:54Z

channeldb/invoice_test.go

-		t.Fatalf("unable to find invoice: %v", err)
-	}
+	err = db.AddInvoice(testInvoice)
+	require.NoError(t, err)


Maybe worth adding a commit once the PR is close to change those too.

bhandras · 2023-04-17T13:33:19Z

channeldb/invoices.go

+// AddInvoice inserts the targeted invoice into the database.
+//
+// NOTE: If the invoice is added to the database this will set the next
+// avaialbe value for the AddIndex field.


nit: avaialbe => available

bhandras · 2023-04-17T14:16:02Z

channeldb/invoices.go

-			}
-		} else {
-			invoiceNum = byteOrder.Uint32(invoiceCounter)
+	case invoice.Terms.PaymentHash != nil:


nit: I think this case should come first so we don't recompute the hash from the preimage all the time if the preimage is set.

bhandras · 2023-04-17T14:24:02Z

channeldb/invoices.go

+// NOTE: This function will be executed in its own transaction and will
+// use the default timeout.
+func (d *DB) AddInvoice(invoice *invpkg.Invoice) error {
+	tx, err := d.BeginReadWriteTx()


This way we will break etcd compatibility. A better way would be to use a closure like kvdb interace's Update and View in order to allow the ...WithTx function to repeat if commit fails. (the etcd "driver" uses an STM to ensure compatibility with the bolt/kvdb interface).

bhandras · 2023-04-17T14:25:21Z

channeldb/invoices.go

+	}
+
+	payAddrIndex := tx.ReadBucket(payAddrIndexBucket)
+	setIDIndex := tx.ReadWriteBucket(setIDIndexBucket)


nit: does this need to be an rw bucket?

bhandras · 2023-04-17T14:26:15Z

channeldb/invoices.go

-		if invoiceNumBySetID == nil {
-			return nil, invpkg.ErrInvoiceNotFound
+		if invoiceNumBySetID != nil {
+			return invoiceNumBySetID, nil


Why the change?

bhandras · 2023-04-17T14:27:20Z

channeldb/invoice_test.go

@@ -187,14 +187,14 @@ func testInvoiceWorkflow(t *testing.T, test invWorkflowTest) {
 	// Attempt to retrieve the invoice which was just added to the
 	// database. It should be found, and the invoice returned should be
 	// identical to the one created above.
-	dbInvoice, err := db.LookupInvoice(ref)
+	dbInvoice, err := db.GetInvoice(ref)


nit: can you please explain in more detail why remove LookUpInvoice makes sense in the commit message?

bhandras · 2023-04-17T14:30:53Z

channeldb/invoices.go

+}
+
+// DeleteInvoicesWithTx updates the invoice identified by the passed InvoiceRef.
+func (d *DB) DeleteInvoicesWithTx(ctx context.Context, dbTx database.DBTx,


For all these ...WithTx functions maybe it'd make sense to first rename them in place and do the changes there to reduce the diff size. Right now it's not easy to see what changed in the function bodies.

positiveblue · 2023-06-09T17:53:48Z

Closed in favor of #7354 #7343 #7357

saubyk added this to the v0.17.0 milestone Mar 27, 2023

positiveblue force-pushed the invoicedb-withTx branch 2 times, most recently from 3d131d9 to ea2771b Compare April 6, 2023 08:10

bitromortac reviewed Apr 6, 2023

View reviewed changes

saubyk requested a review from bhandras April 6, 2023 15:24

positiveblue force-pushed the invoicedb-withTx branch 3 times, most recently from f092585 to edc98cd Compare April 12, 2023 04:27

positiveblue added 16 commits April 12, 2023 00:01

database: New DBTxinterface

12fd7f9

The `DBTx` interface abstracts database transactions across different backends.

invoices: Add PaymentHash field to ContractTerm

e053e34

The new field will be used in cases where we do not know the preimage of an invoice, like in hodl invoices.

invoicedb: stop using LookupInvoice in favor of GetInvoice

50a0e96

invoicedb: implement the new DeleteInvoices method

e8319c7

The new method uses the normal `InvoiceRef` to know what invoice needs to be deleted.

invoicedb: implement the new UpdateInvocie method

be089d3

The methods has been renamed to `UpdateInvoiceV2` to avoid breaking specific unit tests for the db part (store/retrieve).

invoicedb: implement temporal WithTx method

f814b17

invoices: add specific domain methods for invoice actions

50289a9

Put all the domain logic to perform different action to an invoice in invoice methods.

invoiceRegistry: refactor settleHodlInvoice

b64c66a

Use the new invoicedb interface.

invoiceRegistry: refactor cancelInvoiceImpl

4cdedfc

Use the new invoicedb interface.

invoiceRegistry: refactor notifyExitHopHtlcLocked

ef89d1a

Use the new invoicedb interface.

wip: add database interfaces

c4a3a74

positiveblue requested a review from bitromortac April 12, 2023 07:22

positiveblue force-pushed the invoicedb-withTx branch from edc98cd to c4a3a74 Compare April 12, 2023 07:32

saubyk assigned positiveblue Apr 12, 2023

Roasbeef reviewed Apr 12, 2023

View reviewed changes

bhandras reviewed Apr 17, 2023

View reviewed changes

positiveblue added advanced Issues suitable for very experienced developers epic Issues created to track large feature development labels Apr 20, 2023

Roasbeef added the sql label Jun 5, 2023

positiveblue closed this Jun 9, 2023

saubyk removed this from the v0.17.0 milestone Jun 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Invocies: refactor `InvoiceDB` methods #7542

[WIP] Invocies: refactor `InvoiceDB` methods #7542

positiveblue commented Mar 27, 2023 •

edited

bitromortac left a comment

bitromortac Apr 5, 2023

bitromortac Apr 5, 2023

positiveblue Apr 12, 2023

bitromortac Apr 5, 2023

positiveblue Apr 12, 2023

bitromortac Apr 6, 2023

positiveblue Apr 12, 2023

Roasbeef Apr 12, 2023

Roasbeef Apr 12, 2023

bhandras left a comment

bhandras Apr 17, 2023

bhandras Apr 17, 2023

bhandras Apr 17, 2023

bhandras Apr 17, 2023

bhandras Apr 17, 2023

bhandras Apr 17, 2023

bhandras Apr 17, 2023

bhandras Apr 17, 2023

bhandras Apr 17, 2023

positiveblue commented Jun 9, 2023

[WIP] Invocies: refactor InvoiceDB methods #7542

[WIP] Invocies: refactor InvoiceDB methods #7542

Conversation

positiveblue commented Mar 27, 2023 • edited

bitromortac 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

bhandras 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

positiveblue commented Jun 9, 2023

[WIP] Invocies: refactor `InvoiceDB` methods #7542

[WIP] Invocies: refactor `InvoiceDB` methods #7542

positiveblue commented Mar 27, 2023 •

edited