channeldb+invoices: add set id index for AMP #4338

cfromknecht · 2020-05-31T19:51:13Z

This PR adds an additional invoice index to track invoices by the set id of AMP style payments. The control paths for accepting real AMP payments are not enabled by this PR, but it does expose the necessary data structures and validation logic for accept AMP HTLC sets.

Summary of changes:

persist additional AMP related fields on each InvoiceHTLC
introduce HTLCSet method to compute the Accepted HTLCs belonging to a particular set id
adds an invoice index over set id, mapping non-nil set ids to invoices with HTLCs that carry a particular set id. this is can be used by users to lookup, cancel, or settle AMP HTLC sets.
add additional safety checks to invoice state transitions
- ensure that HTLCs can only be persisted when added through UpdateInvoice
- ensure that an invoice can only enter the Accepted or Settled state when targeting a non-empty HTLC set
- validate the per-HTLC hash/preimage pairs for AMP HTLCs when transitioning an invoice to Accepted or Settled. this will be more relevant when the secret share reconstruction is fully integrated.
assert that the exepcted AMP-related fields are exposed via RPC but unpopulated for regular payments

Roasbeef

Completed an initial pass mainly to build up my mental model on how everything will fit together in the end. So far things are looking pretty good! Will do another deeper pass once the PR leaves the draft state.

invoices/update.go

channeldb/invoices.go

invoices/invoiceregistry.go

invoices/update.go

joostjager

It definitely gets a bit heavy with all these payment types. Dropping some comments now and also need to do another pass. One question for now is how to test? Can it be done with manual custom record sending?

channeldb/invoices.go

channeldb/invoice_test.go

channeldb/invoices.go

invoices/invoiceregistry.go

Roasbeef · 2020-06-05T01:06:25Z

Can it be done with manual custom record sending?

I think one way would be to manually create a new "compliant" key-send and verify that it goes thru the new code paths. Also that once it's settled, then the relevant fields show in in ListInvoices on the other end.

channeldb/invoices.go

invoices/invoiceregistry.go

cfromknecht · 2020-06-05T08:38:42Z

Can it be done with manual custom record sending?

Mock payloads? https://github.com/lightningnetwork/lnd/pull/4338/files#diff-912650820f0598a654610a053ad16954R26

joostjager · 2020-06-05T08:43:53Z

So currently the only way this PR is not dead code is for keysend right?

cfromknecht · 2020-06-05T12:05:44Z

Yes without the last commit that promotes keysend this be mostly dead code. Some minor things would not be, e.g. the way HTLC sets are now computed, but most of it would

htlcswitch/hop/payload.go

channeldb/invoices.go

invoices/invoiceregistry.go

Roasbeef

LGTM 🗿

halseth · 2021-02-25T09:48:58Z

@cfromknecht Please re-request review when ready.

Roasbeef

LGTM 🍫

halseth

Great work! Only a last lingering question from me, when that is cleared up this LGTM 🌷

channeldb/invoices.go

halseth · 2021-03-03T09:57:56Z

channeldb/invoices.go

+					htlc.Hash)
+			}
+
+			htlcState = HtlcStateSettled


Below comments trySettle should be no-op for non-AMP HTLCs. This looks like is not?

The NOP is referring to the check that we can settle the HTLC portion, but i agree it could be more clear.

channeldb/invoices.go

halseth · 2021-03-03T10:16:03Z

channeldb/invoices.go

+	// the process by updating the state transitions for individual HTLCs
+	// and recalculate the total amount paid to the invoice.
+	var amtPaid lnwire.MilliSatoshi
+	for _, htlc := range invoice.Htlcs {


Earlier we would iterate through all the invoice's HTLCs, calling cancelSingleHtlc on those canceled by the update, and calling updateHtlc on the rest.

With this commit we are still calling cancelSingleHtlc on canceled HTLCs, but we are calling updateHtlc for ALL, regardless of canceled or non-canceled.

I'm wondering if this is an intentional change.

Mainly affects ResolveTime which can be 0 before its settled.

Prior to AMP, there could only be one HTLC set. Now that there can be multiple, we introduce the inherent risk that we might be persisting a settle/accept of the wrong HTLC set. To migitigate this risk, we add a check to assert that an invoice can only be transitioned into an Accepted or Settled state if the specified HTLC set is non-empty, i.e. has accepted HTLCs. To do this check, we unroll the loops in UpdateInvoice to first process any added or canceled HTLCs. This ensures that the set of accepted HTLCs is up-to-date when we got to transition the invoice into a new state, at which point we can accurately check that the HTLC set is non-empty. Previously this sort of check wasn't possible because a newly added HTLC wouldn't be populated on the invoice when going to call updateInvoiceState. Once the invoice-level transition checks have completed, we complete a final pass to move the HTLCs into their final states and recompute the amount paid. With this refactor, it is now possible to make stronger assertions about the invoice and the state transitions being made by the invoiceregistry before those changes are ultimately persisted.

Roasbeef · 2021-03-04T03:29:46Z

Type mismatch related assertion failure?

    lnd_single_hop_invoice_test.go:178: 
        	Error Trace:	lnd_single_hop_invoice_test.go:178
        	            				test_harness.go:112
        	            				lnd_test.go:14583
        	Error:      	Not equal: 
        	            	expected: int(0)
        	            	actual  : uint64(0x0)
        	Test:       	TestLightningNetworkDaemon/15-of-71/btcd/single_hop_invoice

We also test that legacy keysend payments are promoted to AMP payments on the receiver-sdie by asserting basic properties of the fields returned via the rpc.

cfromknecht · 2021-03-04T18:12:04Z

@Roasbeef weird how this never showed up before? fixed now

cfromknecht added amp v0.11 labels May 31, 2020

cfromknecht added this to the 0.11.0 milestone May 31, 2020

cfromknecht force-pushed the set-id-index branch from 03f5dbe to b3c886d Compare May 31, 2020 20:03

Roasbeef requested changes Jun 1, 2020

View reviewed changes

cfromknecht force-pushed the set-id-index branch 2 times, most recently from f86d202 to 6003dae Compare June 2, 2020 10:24

cfromknecht mentioned this pull request Jun 2, 2020

invoices/invoiceregistry: generate random payaddr for keysend #4343

Closed

cfromknecht force-pushed the set-id-index branch 2 times, most recently from 1c3edee to b447b1f Compare June 3, 2020 10:55

cfromknecht marked this pull request as ready for review June 3, 2020 20:02

cfromknecht force-pushed the set-id-index branch from b447b1f to 239362c Compare June 3, 2020 23:20

cfromknecht mentioned this pull request Jun 4, 2020

multi: hold keysend payments #4167

Merged

joostjager reviewed Jun 4, 2020

View reviewed changes

channeldb/invoices.go Show resolved Hide resolved

channeldb/invoices.go Outdated Show resolved Hide resolved

channeldb/invoice_test.go Outdated Show resolved Hide resolved

channeldb/invoices.go Show resolved Hide resolved

invoices/invoiceregistry.go Outdated Show resolved Hide resolved

Roasbeef reviewed Jun 5, 2020

View reviewed changes

channeldb/invoices.go Show resolved Hide resolved

channeldb/invoices.go Show resolved Hide resolved

invoices/invoiceregistry.go Show resolved Hide resolved

invoices/invoiceregistry.go Outdated Show resolved Hide resolved

joostjager self-requested a review June 8, 2020 07:46

joostjager reviewed Jun 8, 2020

View reviewed changes

cfromknecht force-pushed the set-id-index branch 4 times, most recently from 0d62721 to 0b91cb8 Compare June 9, 2020 03:37

Roasbeef approved these changes Jun 9, 2020

View reviewed changes

cfromknecht force-pushed the set-id-index branch 2 times, most recently from 05343cd to 355088f Compare June 9, 2020 05:15

cfromknecht force-pushed the set-id-index branch from 4270d14 to 7861ed0 Compare February 25, 2021 23:18

cfromknecht requested review from halseth and Roasbeef March 2, 2021 18:43

Roasbeef approved these changes Mar 3, 2021

View reviewed changes

halseth approved these changes Mar 3, 2021

View reviewed changes

cfromknecht force-pushed the set-id-index branch 2 times, most recently from d3657c9 to 9733cd8 Compare March 3, 2021 23:41

cfromknecht added 11 commits March 3, 2021 16:26

invoices: expose AMPRecord() method on Payload

464dff0

channeldb/invoice: persist optional AMP fields on InvoiceHTLC

7f05c9d

invoices+channeldb: add HTLCSet method to invoice

b223470

channeldb: ensure invoices can't be added with HTLCs

da049eb

channeldb/invoice: map zero-value timestamps to 0

7c0d1e0

Mainly affects ResolveTime which can be 0 before its settled.

channeldb/invoices: add invoices to set id index on update

fad25f3

channeldb/invoices: update AMP invoice htlcs

e1b0fe5

channeldb/invoices: continue cancel loop early

5f34880

invoices: filter HTLC set by set id from AMP payload

9da3e7f

invoices/invoiceregistry: unbundle invalid preimage error

137258f

cfromknecht force-pushed the set-id-index branch from 9733cd8 to 3663130 Compare March 4, 2021 00:28

cfromknecht added 2 commits March 4, 2021 10:11

lnrpc+itest: expose AMP fields on InvoiceHTLC

d004442

We also test that legacy keysend payments are promoted to AMP payments on the receiver-sdie by asserting basic properties of the fields returned via the rpc.

invoices: replace all asserts with requires

68688d6

cfromknecht force-pushed the set-id-index branch from 3663130 to 68688d6 Compare March 4, 2021 18:11

cfromknecht merged commit 56b6107 into lightningnetwork:master Mar 4, 2021

cfromknecht deleted the set-id-index branch March 4, 2021 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

channeldb+invoices: add set id index for AMP #4338

channeldb+invoices: add set id index for AMP #4338

cfromknecht commented May 31, 2020 •

edited

Loading

Roasbeef left a comment

joostjager left a comment

Roasbeef commented Jun 5, 2020

cfromknecht commented Jun 5, 2020

joostjager commented Jun 5, 2020

cfromknecht commented Jun 5, 2020

Roasbeef left a comment

halseth commented Feb 25, 2021

Roasbeef left a comment

halseth left a comment

halseth Mar 3, 2021

cfromknecht Mar 3, 2021

halseth Mar 3, 2021

Roasbeef commented Mar 4, 2021

cfromknecht commented Mar 4, 2021

channeldb+invoices: add set id index for AMP #4338

channeldb+invoices: add set id index for AMP #4338

Conversation

cfromknecht commented May 31, 2020 • edited Loading

Roasbeef left a comment

Choose a reason for hiding this comment

joostjager left a comment

Choose a reason for hiding this comment

Roasbeef commented Jun 5, 2020

cfromknecht commented Jun 5, 2020

joostjager commented Jun 5, 2020

cfromknecht commented Jun 5, 2020

Roasbeef left a comment

Choose a reason for hiding this comment

halseth commented Feb 25, 2021

Roasbeef left a comment

Choose a reason for hiding this comment

halseth left a comment

Choose a reason for hiding this comment

halseth Mar 3, 2021

Choose a reason for hiding this comment

cfromknecht Mar 3, 2021

Choose a reason for hiding this comment

halseth Mar 3, 2021

Choose a reason for hiding this comment

Roasbeef commented Mar 4, 2021

cfromknecht commented Mar 4, 2021

cfromknecht commented May 31, 2020 •

edited

Loading