channeldb: index payments by payment addr, use payment hash as fallback #4285

cfromknecht · 2020-05-13T22:49:55Z

This PR adds a new persistent index on invoices to allow efficient
lookups and updates by payment address. This is stepping stone
to enable AMP payments where the payment hash will not be
associated with the target invoice.

Since 0.9, all new invoices include a random payment address,
however older invoices do not have this value. New invoices will
be added to the payment address index on a rolling basis, but
the payment hash index will continue to serve as a fallback in case
the target invoice was created prior to this commit. As such,
no migration is needed for invoices created prior to this commit,
but we include one to bump the db version number and initialize
the top-level index bucket.

This also unblocks #4167, since it allows us to insert a random
payment address for experimental keysend payments. In the future,
where keysend is rolled into the final AMP proposal, this ensures that
all keysend (experimental or otherwise) have a payment address
associated with the invoice. This is desired since we will be using
payment addresses entirely to identify and update AMP payments, so
we ensure the code paths between AMP and legacy keysend are
identical.

Further off, once payment addresses are required, we can stop inserting
invoices into the payment hash index entirely. Currently however, we rely
on invoices being indexed to support other operations, e.g. hodl settle/cancel
or legacy senders that don't understand MPP.

joostjager · 2020-05-14T05:00:05Z

One thought that came up: (hodl) keysend payments and AMP payments can be settled by payment address. But how does that work for recurring payments to the same address? How to keep those apart when you want to settle or cancel? I think the set_id is needed? But does that also require another index?

channeldb/invoices.go

joostjager · 2020-05-18T04:39:03Z

channeldb/invoices.go

@@ -538,7 +553,9 @@ func (d *DB) InvoicesAddedSince(sinceAddIndex uint64) ([]Invoice, error) {
 // full invoice is returned. Before setting the incoming HTLC, the values
 // SHOULD be checked to ensure the payer meets the agreed upon contractual
 // terms of the payment.
-func (d *DB) LookupInvoice(paymentHash [32]byte) (Invoice, error) {
+func (d *DB) LookupInvoice(paymentHash [32]byte,
+	payAddr *[32]byte) (Invoice, error) {


Is there a use case for specifying both?

Or if both are specified, is a sanity check useful to see if hash is always present and pointing to the same invoice if addr is present too?

the reason to use both is so that we don't have to add a migration for existing invoices. with AMP the hash won't align with any particular invoice, but this wil be worked around by providing a nil hash in the InvoiceRef.

if we do a full migration we'd end up supporting either (hash, nil) or (nil, pay_addr). if we don't, then we need to support (hash, nil), (nil, pay_addr), or (hash, pay_addr), where the latter has a sanity check that they point to the same invoice.

atm, there isn't really a business use case for doing the full migration, it would only serve to clean up this bit of code. this may change in the in the future tho (if we begin exposing invoice lookups by payment addr and we want to expose these slightly older invoices), but it is still possible to do this migration retroactively when that time comes. it is not terribly difficult, but it is time consuming in the critical path

InvoiceRef added, supporting (hash, nil) and (hash, pay_addr). the final variant will be added once the related AMP changes go in

channeldb/invoices.go

invoices/invoiceregistry.go

invoices/test_utils_test.go

channeldb/db.go

channeldb/invoice_test.go

channeldb/db.go

channeldb/invoice_test.go

channeldb/invoices.go

joostjager

Great improvement with InvoiceRef!

joostjager · 2020-05-22T06:17:10Z

channeldb/db.go

-		err = tx.DeleteTopLevelBucket(graphMetaBucket)
-		if err != nil && err != kvdb.ErrBucketNotFound {
-			return err
+		for _, tlb := range topLevelBuckets {


If we're removing everything, can't it be a for each on the tx?

it's less atomic, but also only used for testing afaia, what would we gain?

Hmm so it should really be moved to a _test.go file then.

Yeah it's something left over from super early development, when the easiest thing to do was just delete the entire database and start over lol.

channeldb/db.go

channeldb/migration/create_tlb_test.go

channeldb/migration/log.go

channeldb/invoices.go

channeldb/db.go

channeldb/invoice_test.go

joostjager · 2020-05-22T06:31:07Z

channeldb/invoices.go

+	// hash, but pre 0.8 invoices do not have one at all. When this value is
+	// known it will be used as the primary identifier, falling back to
+	// payHash if no value is known.
+	payAddr *[32]byte


Not necessarily for this PR, but I can see an advantage of making an lntypes.Hash style datatype for payment address, especially if we are going to use it a lot

yes i've been thinking maybe we should, but held off because we don't use it for anything other than comparing equality. definitely something we can look into tho, it would be nice if they were unified

htlcswitch/test_utils.go

joostjager · 2020-05-25T18:11:48Z

channeldb/db.go

-		err = tx.DeleteTopLevelBucket(graphMetaBucket)
-		if err != nil && err != kvdb.ErrBucketNotFound {
-			return err
+		for _, tlb := range topLevelBuckets {


Hmm so it should really be moved to a _test.go file then.

channeldb/db.go

channeldb/migration/log.go

go.sum

channeldb/invoices.go

channeldb/invoice_test.go

Roasbeef · 2020-05-27T00:46:45Z

Test failure on travis:

--- FAIL: TestChannelLinkBatchPreimageWrite (7.15s)
    --- FAIL: TestChannelLinkBatchPreimageWrite/flush_on_disconnect (6.20s)
        link_test.go:4746: unable to find preimages: expected to find witness: true, got false for hash=7888bd181c582842696baa9d3a55f8e324637da1c91dc89690aa71e3a04c91d0

Not sure if related.

Roasbeef · 2020-05-27T00:48:02Z

New trigger in the error log filtration for the itests:

PASS
ok  	github.com/lightningnetwork/lnd/lntest/itest	990.562s
lntest/itest/log_check_errors.sh
<time> [ERR] NTFN: unable to get hash from block with height 790

Roasbeef

LGTM 🗿

Merge pending resolution of those two build issues I mentioned (log one is easy, other one may just be something else entirely, and thus out of scope).

Roasbeef · 2020-05-27T00:50:14Z

channeldb/invoices.go

@@ -142,6 +142,32 @@ const (
 	amtPaidType     tlv.Type = 13
 )

+// InvoiceRef is an identifier for invoices supporting queries by payment hash.


invoices/invoiceregistry.go

channeldb/invoice_test.go

channeldb/invoices.go

htlcswitch/test_utils.go

This line was incorrectly moved when the migtest package was created for migration 12. This PR introduces a negative test for CreateTLB which surfaced this.

This commit creates a generic migration for creating top-level buckets.

Neutrino can get slow towards the end of the itests.

githorray · 2020-05-28T01:56:52Z

Getting an error after this merge when trying to build lnd.

$ make install tags="experimental signrpc walletrpc chainrpc invoicesrpc"

github.com/lightningnetwork/lnd/channeldb
# github.com/lightningnetwork/lnd/channeldb
channeldb/invoices.go:657:54: undefined: kvdb.ReadBucket
Makefile:115: recipe for target 'build' failed
make: *** [build] Error 2```

We recently renamed kvdb.ReadBucket to kvdb.RBucket in lightningnetwork#4252, but this was not detected in lightningnetwork#4285 since this new code didn't produce a conflict.

cfromknecht requested review from joostjager and Roasbeef as code owners May 13, 2020 22:49

cfromknecht added the v0.11 label May 13, 2020

cfromknecht added this to the 0.11.0 milestone May 13, 2020

cfromknecht added database Related to the database/storage of LND migration payments Related to invoices/payments labels May 13, 2020

cfromknecht mentioned this pull request May 13, 2020

multi: hold keysend payments #4167

Merged

cfromknecht force-pushed the pay-addr-index branch 2 times, most recently from 58ffb70 to 46ebc79 Compare May 14, 2020 07:18

joostjager reviewed May 18, 2020

View reviewed changes

Roasbeef requested changes May 19, 2020

View reviewed changes

channeldb/db.go Show resolved Hide resolved

channeldb/invoice_test.go Show resolved Hide resolved

channeldb/invoice_test.go Outdated Show resolved Hide resolved

channeldb/invoices.go Outdated Show resolved Hide resolved

cfromknecht force-pushed the pay-addr-index branch 4 times, most recently from 2c6e0b8 to 18d887a Compare May 20, 2020 22:34

cfromknecht requested review from joostjager and Roasbeef May 20, 2020 22:34

cfromknecht force-pushed the pay-addr-index branch 4 times, most recently from 02fc031 to d4eb6f3 Compare May 21, 2020 23:12

joostjager reviewed May 22, 2020

View reviewed changes

cfromknecht force-pushed the pay-addr-index branch from d4eb6f3 to 920bfd0 Compare May 22, 2020 22:51

cfromknecht requested a review from joostjager May 22, 2020 22:53

joostjager approved these changes May 25, 2020

View reviewed changes

cfromknecht force-pushed the pay-addr-index branch from 920bfd0 to 532b1a4 Compare May 26, 2020 19:25

Roasbeef approved these changes May 27, 2020

View reviewed changes

cfromknecht force-pushed the pay-addr-index branch from 532b1a4 to dacdbce Compare May 27, 2020 01:12

cfromknecht added 6 commits May 26, 2020 18:55

channeldb/migtest: log migration failure instead of failing

d343575

This line was incorrectly moved when the migtest package was created for migration 12. This PR introduces a negative test for CreateTLB which surfaced this.

channeldb: consolidate top-level bucket create/wipe

24cce7a

channeldb/migration: add generic CreateTLB migration

e80e21d

This commit creates a generic migration for creating top-level buckets.

invoices/invoiceregistry: rename updateCtx to ctx

2799202

channeldb+invoices: track invoices by InvoiceRef

3522f09

channeldb+invoices: use payment addr as primary index

cbf71b5

cfromknecht force-pushed the pay-addr-index branch from dacdbce to 0730e39 Compare May 27, 2020 01:56

cfromknecht added 2 commits May 26, 2020 19:49

lntest: update error whitelist

5c4ab4b

itest: increase mempool timeout

152d153

Neutrino can get slow towards the end of the itests.

cfromknecht force-pushed the pay-addr-index branch from a072b5a to 152d153 Compare May 27, 2020 02:49

cfromknecht merged commit 0f3ab77 into lightningnetwork:master May 28, 2020

cfromknecht mentioned this pull request Jun 23, 2020

invoices/invoiceregistry: special-case payaddr index insertion for keysend #4407

Merged

cfromknecht deleted the pay-addr-index branch July 31, 2020 02:38

C-Otto mentioned this pull request Dec 7, 2020

RPC: Document "payment_addr" usage #4839

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

channeldb: index payments by payment addr, use payment hash as fallback #4285

channeldb: index payments by payment addr, use payment hash as fallback #4285

cfromknecht commented May 13, 2020 •

edited

Loading

joostjager commented May 14, 2020

joostjager May 18, 2020

joostjager May 18, 2020

cfromknecht May 18, 2020 •

edited

Loading

cfromknecht May 20, 2020

joostjager left a comment

joostjager May 22, 2020

cfromknecht May 22, 2020

joostjager May 25, 2020

Roasbeef May 27, 2020

joostjager May 22, 2020

cfromknecht May 22, 2020

joostjager May 25, 2020

Roasbeef commented May 27, 2020

Roasbeef commented May 27, 2020

Roasbeef left a comment

Roasbeef May 27, 2020

githorray commented May 28, 2020

channeldb: index payments by payment addr, use payment hash as fallback #4285

channeldb: index payments by payment addr, use payment hash as fallback #4285

Conversation

cfromknecht commented May 13, 2020 • edited Loading

joostjager commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager 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

Roasbeef commented May 27, 2020

Roasbeef commented May 27, 2020

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

githorray commented May 28, 2020

cfromknecht commented May 13, 2020 •

edited

Loading

cfromknecht May 18, 2020 •

edited

Loading