channeldb: split `UpdateInvoice` logic #7377

positiveblue · 2023-02-02T14:56:38Z

The InvoiceDB interface has one method a method that takes care of updating an invoice. An invoice can be updated in multiple ways, it is up to the caller to decide how an invoice need to be updated and persisted using one of the function arguments.

The method will load the current invoice state from the db and apply a callback provided by the user. The callback needs to return an InvoiceUpdateDesc. Depending on the kind of update, the InvoiceUpdateDesc will be populated with information that will be used to update the db entry. Because UpdateInvoice is used in multiple scenarios (cancel an htlc, settle an invoice, cancel an invoice etc...) the InvoiceUpdateDesc bundles parameters used in each type of update. No update will ever use all the fields of the struct.

Because the flow for every kind of update + invoice type (bolt11 vs AMP) is mixed it is hard to follow what constraints the InvoiceUpdateDesc fields have + what parts get modified in every type of call.

The objective of this PR is to split the logic of each type of update that is allowed. This would allow keep better track of the each update flow, logically decouple the flows of each update type and possibly prepare the path to split this method in multiple ones.

Now that the InvoiceDB interface is exposed and other people can code and plug their own implementations it may make sense to have a clear API to what needs to be persisted in each method while keeping the checks of what is allowed in the invoices package.

NOTE: things to take into account

We never add and cancel htlcs in the same call.
We never add/cancel more than once htlc in one call. For AMP invoices though cancelling one htlc may trigger canceling all the htlcs with the same SetID.
We never add/cancel htlcs belonging to more than one SetID.
Adding an HTLC may trigger settleing an invoice. State changes are part of the data bundled in InvoiceUpdateDesc so the callback needs to take care of setting them.
ContractAccepted is only used in hold invoices

bitromortac

I see the necessity to break this up and make the current invoice update handling more separated 💯. It feels quite difficult to get all things right at once here, especially with AMP in the mix. Could one extract AMP code paths perhaps in their own functions as well?

channeldb/invoices.go

sputn1ck

Easy to follow PR 🥇 Still trying to grok some of what is happening. Have a Q regarding using explicit types.

invoices/invoices.go

bitromortac

This looks close to me 🚀, nice commit restructures!

invoices/invoices.go

channeldb/invoices.go

docs/release-notes/release-notes-0.16.0.md

channeldb/invoices.go

lightninglabs-deploy · 2023-03-06T13:07:19Z

@bhandras: review reminder
@sputn1ck: review reminder
@positiveblue, remember to re-request review from reviewers when ready

yyforyongyu

First round looks good. Very nice commit structure, easy to follow👍 So happy we are refactoring this part of the code, which makes the following fix in #7463 easier🙏
Left some comments and questions. Will read more about AMP and then come for a second round!

yyforyongyu · 2023-03-13T11:04:01Z

invoices/invoices.go

+
+	// SettleHodlInvoiceUpdate indicates that this update settles one or
+	// more HTLCs from a hodl invoice.
+	SettleHodlInvoiceUpdate


Haven't read the full PR, just one more state pops into my head. Maybe there needs to be a SettleHTLCsUpdate? Since we have CancelHTLCsUpdate vs CancelInvoiceUpdate.

invoices/invoices.go

channeldb/invoices.go

yyforyongyu · 2023-03-13T11:33:52Z

channeldb/invoices.go

+	update *invpkg.InvoiceUpdateDesc) (*invpkg.Invoice, error) {
+
+	var setID *[32]byte
+	invoiceIsAMP := invoice.IsAMP()


Just thinking out of the box...I wonder if it's easier to create two functions, one for adding htlcs for AMP addHTLCsAMP, and the other for MPP, since they have a different logical flow.

Incoming in the next PR, I did it this way so this PR did not need to include changes in the tests (easier to review + build knowledge about the related code)

I did it this way so this PR did not need to include changes in the tests

what did you mean by this way? probably should review the next PR now...😂

Not changing the interface

channeldb/invoices.go

Make the kind of update explicit in the `InvoiceUpdateDesc` struct.

bitromortac

LGTM 🎉

yyforyongyu

Left some questions/comments, once addressed this is good to go⛵️

yyforyongyu · 2023-03-14T11:44:14Z

channeldb/invoices.go

+
+	invoiceIsAMP = invoice.IsAMP()
+	if invoiceIsAMP {
+		setID = update.SetID


So when we cancel an htlc for an amp invoice we set it in the top level

so this is to reflect the above comment right? basically the line setID = (*[32]byte)(update.SetID)

channeldb/invoices.go

yyforyongyu · 2023-03-14T12:23:42Z

channeldb/invoices.go

-	}
-
-	if err := invoices.Put(invoiceNum, buf.Bytes()); err != nil {
+	err := d.serializeAndStoreInvoice(invoices, invoiceNum, invoice)


non-blocking: are there any reasons that we call this serializeAndStoreInvoice at multiple places? Instead of using it in cancelHTLCs, addHTLCs and settleHodlInvoice separately, I think we could just call it outside the switch update.UpdateType {, and return d.serializeAndStoreInvoice, seems like a few more lines were saved and fewer params used.

Or, do you plan to further make these actions into independent db tx(which is nice IMO)?

Each one will be its own db interface method. I would make the change if this code was going to survive but it won't 🎯

yyforyongyu · 2023-03-14T12:26:48Z

channeldb/invoices.go

 	)

 	// We can either get the set ID from the main state update (if the
 	// state is changing), or via the hint passed in returned by the update
 	// call back.
 	if update.State != nil {
 		setID = update.State.SetID
-		newState = update.State.NewState


nit: this seems to be the wrong commit to remove it, was it because it was previously used in cancelSingleHtlc and now the compiler is complaining if it's not removed?

mmm.. are we talking about the second commit? I am able to compile and run the tests (make unit) without any problem 👀

docs/release-notes/release-notes-0.16.0.md

channeldb/invoice_test.go

yyforyongyu · 2023-03-14T12:50:02Z

channeldb/invoices.go

+	update *invpkg.InvoiceUpdateDesc) (*invpkg.Invoice, error) {
+
+	var setID *[32]byte
+	invoiceIsAMP := invoice.IsAMP()


I did it this way so this PR did not need to include changes in the tests

what did you mean by this way? probably should review the next PR now...😂

channeldb/invoices.go

Previous to this commit we were able to call `UpdateInvoice` with an updated that added and cancelled htlcs at the same time. The function returned an error if there was overlapping between the two htlc set. However, that behavior was not used in the LND code itself. Eventually we want to split this method in multiple ones, among them one for canceling htlcs and another one for adding them. For that reason, this behavior is not supported anymore.

All the updates type are now catched in a switch statement, we can delete the rest of the body of this function + add a default path for not matched flows.

yyforyongyu

LGTM🎉 Confirmed the itest failures are flake.

yyforyongyu · 2023-03-15T17:36:38Z

One more question: should this be based on 0-17-0-staging branch instead of master?

positiveblue changed the title ~~channeldb: splite UpdateInvoice logic~~ channeldb: split UpdateInvoice logic Feb 2, 2023

positiveblue requested review from bhandras, bitromortac and sputn1ck February 2, 2023 14:59

saubyk added this to the v0.16.0 milestone Feb 2, 2023

saubyk added database Related to the database/storage of LND invoices sql labels Feb 2, 2023

saubyk assigned positiveblue Feb 4, 2023

positiveblue force-pushed the split-update-invoice branch 2 times, most recently from d6fa424 to 89b3671 Compare February 8, 2023 16:10

bitromortac reviewed Feb 13, 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

saubyk modified the milestones: v0.16.0, v0.16.1 Feb 14, 2023

sputn1ck reviewed Feb 21, 2023

View reviewed changes

invoices/invoices.go Outdated Show resolved Hide resolved

invoices/invoices.go Show resolved Hide resolved

positiveblue force-pushed the split-update-invoice branch from 89b3671 to 22f45d7 Compare February 23, 2023 14:55

positiveblue requested review from bitromortac and sputn1ck February 23, 2023 22:02

bitromortac reviewed Feb 27, 2023

View reviewed changes

positiveblue force-pushed the split-update-invoice branch from 22f45d7 to 40ce742 Compare March 7, 2023 00:26

positiveblue requested a review from bitromortac March 7, 2023 00:41

saubyk requested a review from yyforyongyu March 9, 2023 15:27

yyforyongyu requested changes Mar 13, 2023

View reviewed changes

saubyk modified the milestones: v0.16.1, v0.17.0 Mar 13, 2023

invoices: add UpdateType to InvoiceUpdateDesc

8438799

Make the kind of update explicit in the `InvoiceUpdateDesc` struct.

positiveblue force-pushed the split-update-invoice branch from 40ce742 to 3f4bb48 Compare March 14, 2023 07:04

positiveblue requested a review from yyforyongyu March 14, 2023 07:07

bitromortac approved these changes Mar 14, 2023

View reviewed changes

yyforyongyu requested changes Mar 14, 2023

View reviewed changes

positiveblue added 7 commits March 14, 2023 18:56

channeldb: split settleHodlInvoice logic in the UpdateInvoice method

cbaec43

channeldb: split cancelInvoice logic in the UpdateInvoice method

27fbc2f

channeldb: split addHTLCs logic in the UpdateInvoice method

6ff6c45

channeldb: clean updateInvoice func

65fc8d7

All the updates type are now catched in a switch statement, we can delete the rest of the body of this function + add a default path for not matched flows.

channeldb: add serializeAndStoreInvoice helper func

09e87ea

docs: add release notes

034853a

positiveblue force-pushed the split-update-invoice branch from 3f4bb48 to 034853a Compare March 15, 2023 02:41

positiveblue requested a review from yyforyongyu March 15, 2023 06:49

yyforyongyu approved these changes Mar 15, 2023

View reviewed changes

positiveblue changed the base branch from master to 0-17-0-staging March 16, 2023 06:26

guggero merged commit b7697a7 into lightningnetwork:0-17-0-staging Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

channeldb: split `UpdateInvoice` logic #7377

channeldb: split `UpdateInvoice` logic #7377

positiveblue commented Feb 2, 2023

bitromortac left a comment

sputn1ck left a comment

bitromortac left a comment

lightninglabs-deploy commented Mar 6, 2023

yyforyongyu left a comment

yyforyongyu Mar 13, 2023

yyforyongyu Mar 13, 2023

positiveblue Mar 14, 2023

yyforyongyu Mar 14, 2023

positiveblue Mar 15, 2023

bitromortac left a comment

yyforyongyu left a comment

yyforyongyu Mar 14, 2023

positiveblue Mar 15, 2023

yyforyongyu Mar 14, 2023

positiveblue Mar 15, 2023 •

edited

yyforyongyu Mar 14, 2023

positiveblue Mar 15, 2023

yyforyongyu Mar 14, 2023

yyforyongyu left a comment

yyforyongyu commented Mar 15, 2023

channeldb: split UpdateInvoice logic #7377

channeldb: split UpdateInvoice logic #7377

Conversation

positiveblue commented Feb 2, 2023

bitromortac left a comment

Choose a reason for hiding this comment

sputn1ck left a comment

Choose a reason for hiding this comment

bitromortac left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Mar 6, 2023

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

bitromortac 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

positiveblue Mar 15, 2023 • edited

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

yyforyongyu commented Mar 15, 2023

channeldb: split `UpdateInvoice` logic #7377

channeldb: split `UpdateInvoice` logic #7377

positiveblue Mar 15, 2023 •

edited