channeldb+invoices: add spontaneous AMP receiving + sending via SendToRoute #5108

cfromknecht · 2021-03-15T23:29:33Z

This PR adds the ability to receive spontaneous AMP payments, and send them by crafting the appropriate request using SendToRoute. This PR is opened as a Draft since not all feature bits/TLV record types are decided.

PLEASE DO NOT RUN THIS ON A PRODUCTION NODE

Summary of changes:

Adds support for parsing AMP record from TLV onion payload
Adds (non-final) AMP feature bits
Add InvoiceRef support for querying by pay_addr only
Add JIT AMP-invoice insertion when receiving AMP HTLC
Validation + testing around HTLC-level preimage persistence
Reconstruction of HTLCSet child preimage/hashes using amp package
Integration test demonstrating multi-path AMP via SendToRoute

TODO:

expand itest assertions
add more unit testing around AMP invoice settlement w/ invalid HTLC sets
fix invoice parsing for AMP invoices

Roasbeef

Completed an initial high level pass......my body is ready 😈

record/amp.go

htlcswitch/hop/payload.go

lnwire/features.go

invoices/update.go

channeldb/invoices.go

invoices/update.go

Roasbeef · 2021-03-17T22:16:19Z

lntest/itest/lnd_amp_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func testSendToRouteAMP(net *lntest.NetworkHarness, t *harnessTest) {


IT LIVES!!!! 🤘

andrewshvv · 2021-03-17T23:08:26Z

lntest/itest/lnd_test_list_on_test.go

@@ -266,6 +266,10 @@ var allTestCases = []*testCase{
 		name: "sendtoroute multi path payment",
 		test: testSendToRouteMultiPath,
 	},
+	{
+		name: "sendtoroute amp",


Roasbeef · 2021-03-19T04:03:02Z

invoices/update.go

@@ -206,15 +229,91 @@ func updateMpp(ctx *invoiceUpdateCtx,
 		return &update, ctx.acceptRes(resultAccepted), nil
 	}

+	var (


If we add a check for an invalid AMP payload, before the the check for the total amount, then it would be possible to send a corrupt share to cancel all HTLCs w/ that set ID. This lets the sender bail out if they know things won't work for w/e reason. It would also be useful for more iterative probing.

it isn't really possible to send an invalid AMP payload, and it's only possible to know if the shares are invalid after receiving all of them. if the receiver tries to reconstruct early they will always be invalid. given that the amount is the termination condition, i don't think there's any getting around that.

the current iteration does fail back all HTLCs if the reconstruction fails tho, to support the probing use case.

Hmm, yeah I get what you mean. Perhaps this can be done by extending the ValidateParsedPayloadTypes checks: if the sender sends a payload w/o an amount, but with a valid set ID of already held HTLCs, they call get cancelled back?

given that the amount is the termination condition, i don't think there's any getting around that.

Another idea: we reserve a special value of the childIndex that can be used to signal intent to bail the payment early.

The motivation here is to support a fast-fail mode w/o having to wait for the normal MPP timeout. This can be useful in contexts like attempting to Loop In wherein the user doesn't really know how much they can Loop In, and even if we fail w/ probing the full amount, if can get some additional feedback, then we can offer a: "Loop In as much as you can" mode.

Stepping back a bit, perhaps this can just be implemented by moving one layer higher: a special custom record that just says "ok, can you guys cancel things back now". Applications would be able to use the to-be-designed API for handling hodl AMP payments to handle this, or we'd grow to do it automatically.

Updated the PR to include immediate cancellation when seed reconstruction fails as well as some tests.

Stepping back a bit, perhaps this can just be implemented by moving one layer higher: a special custom record that just says "ok, can you guys cancel things back now". Applications would be able to use the to-be-designed API for handling hodl AMP payments to handle this, or we'd grow to do it automatically.

This idea sounds pretty workable! Seems like a pretty contained change as well, also wouldn't need reserved values, special cases, etc in the base protocol

So the receiver gets the cancel signal and cancels all htlcs back, but does the sender learn from that which htlcs were actually held by the receiver vs the ones that were still in-flight or didn't even reach the receiver? I suppose the sender can inspect the failure message to see what the origin of the failure is. Only when an htlc arrives at the receiver after the set has already been cancelled, and is then cancelled back, what does this look like to the sender? Will it be clear that that final htlc wasn't part of the set and reached the receiver "independently", possibly reusing liquidity that was just released?

lntest/itest/lnd_amp_test.go

halseth · 2021-03-25T10:12:42Z

invoices/invoiceregistry.go

+// processAMP just-in-time inserts an invoice if this htlc is a keysend
+// htlc.
+func (i *InvoiceRegistry) processAMP(ctx invoiceUpdateCtx) error {
+	// Only allow keysend for non-mpp payments.


comment is wrong?

Yeah the comment is dated. IIUC, this now allows a sender to send an invalid payload that causes a failure to occur for the HTLC with the invalid payload. Still getting through the commits to check if this'll end up cancelling back the entire HTLC set w/ the same setID or not....

this is just a sanity check since we can't identify an HTLC set without (pay_addr, set_id), this does not cancel back the entire set

halseth · 2021-03-25T10:14:34Z

invoices/invoiceregistry.go

+	payAddr := ctx.mpp.PaymentAddr()
+
+	// Create placeholder invoice.
+	invoice := &channeldb.Invoice{


Maybe it is nice to keep around AMP payments that were attempted but didn't complete?

halseth · 2021-03-25T10:15:02Z

invoices/invoiceregistry.go

+		},
+	}
+
+	if i.cfg.KeysendHoldTime != 0 {


IIUC, as is, if this flag is set then we won't immeidatley settle things after all teh shards arrive. However this is only useful if the caller is able to manually settle the payment using the invoice RPC APIs. The existing settle invoice API is based on the caller knowing the pre-image (sub-swaps or they made it themselves) while in this case, such an action isn't possible as the code will attempt to look up the invoice via it's payment hash as a ref: https://github.com/lightningnetwork/lnd/blob/master/invoices/invoiceregistry.go#L1030

So I think we can remove this clause all together since it can't properly be used w/ the existing API. This was originally added for hodl keysend, but in that case there's a single pre-image that's returned via the subscription API once the invoice gets "just in time" inserted into the database.

sounds good, we can add it back one we have the ability to settle cancel the set via RPC

channeldb/invoices.go

Roasbeef

Latest set of changes are looking really good! I have a final pass planned to focus on the added uni test coverage.

lnwire/features.go

Roasbeef · 2021-03-26T23:34:52Z

invoices/invoiceregistry.go

+		},
+	}
+
+	if i.cfg.KeysendHoldTime != 0 {


IIUC, as is, if this flag is set then we won't immeidatley settle things after all teh shards arrive. However this is only useful if the caller is able to manually settle the payment using the invoice RPC APIs. The existing settle invoice API is based on the caller knowing the pre-image (sub-swaps or they made it themselves) while in this case, such an action isn't possible as the code will attempt to look up the invoice via it's payment hash as a ref: https://github.com/lightningnetwork/lnd/blob/master/invoices/invoiceregistry.go#L1030

So I think we can remove this clause all together since it can't properly be used w/ the existing API. This was originally added for hodl keysend, but in that case there's a single pre-image that's returned via the subscription API once the invoice gets "just in time" inserted into the database.

invoices/invoiceregistry_test.go

Roasbeef · 2021-03-26T23:38:25Z

invoices/invoiceregistry.go

+// processAMP just-in-time inserts an invoice if this htlc is a keysend
+// htlc.
+func (i *InvoiceRegistry) processAMP(ctx invoiceUpdateCtx) error {
+	// Only allow keysend for non-mpp payments.


Yeah the comment is dated. IIUC, this now allows a sender to send an invalid payload that causes a failure to occur for the HTLC with the invalid payload. Still getting through the commits to check if this'll end up cancelling back the entire HTLC set w/ the same setID or not....

invoices/invoiceregistry.go

halseth

LGTM! I've also been building on top of this branch, and it's been working great 👍

halseth · 2021-03-30T07:34:12Z

invoices/invoiceregistry.go

+	rawFeatures := lnwire.NewRawFeatureVector(
+		lnwire.TLVOnionPayloadOptional,
+		lnwire.PaymentAddrOptional,
+		lnwire.AMPRequired,


If AMP is required, can't we also make paymentaddr and TLV required? (not that it really matters)

halseth · 2021-03-30T07:42:32Z

invoices/resolution_result.go

+func (s FailResolutionResult) IsSetFailure() bool {
+	switch s {
+	case
+		ResultAmpReconstruction,


return ResultAmp.. || ResultHtlc.. || ...?

joostjager · 2021-03-30T14:14:12Z

invoices/invoiceregistry.go

+	payAddr := ctx.mpp.PaymentAddr()
+
+	// Create placeholder invoice.
+	invoice := &channeldb.Invoice{


To what extent is this an attack vector? With keysend there always a single shard, so every invoice created will at least have a corresponding settled htlc with real money attached to it. With AMP, someone can endlessly send htlcs with different payment addresses. All of them are cancelled back but leave behind invoices in the database.

joostjager · 2021-03-30T14:26:11Z

invoices/update.go

@@ -206,15 +229,91 @@ func updateMpp(ctx *invoiceUpdateCtx,
 		return &update, ctx.acceptRes(resultAccepted), nil
 	}

+	var (


So the receiver gets the cancel signal and cancels all htlcs back, but does the sender learn from that which htlcs were actually held by the receiver vs the ones that were still in-flight or didn't even reach the receiver? I suppose the sender can inspect the failure message to see what the origin of the failure is. Only when an htlc arrives at the receiver after the set has already been cancelled, and is then cancelled back, what does this look like to the sender? Will it be clear that that final htlc wasn't part of the set and reached the receiver "independently", possibly reusing liquidity that was just released?

joostjager · 2021-03-30T14:34:21Z

invoices/update.go

+
+	// Using the child descriptors, reconstruct the root seed and derive the
+	// child hash/preimage pairs for each of the HTLCs.
+	children := amp.ReconstructChildren(childDescs...)


I had one thing come to my mind about constructing the shares. Maybe it is trivial because I am not completely understandig the tree concept yet.

Suppose the sender sends three htlcs. The share tree then looks like this:

R /\ / \ A / \ / \ B C

C arrives, but A and B fail. Then the sender decides to replace A and B by a single htlc. They didn't do this in the first place, because the amount for that single htlc forces them to take an expensive route. How is the share for this new htlc constructed? I guess it is just A^B?

If the single replacement htlc fails then too and another thing needs to be tried, is the previous tree 'restored'?

yes the new share is constructed as A^B, then the two HTLCs would be (A^B)^C = R.

If the single replacement htlc fails then too and another thing needs to be tried, is the previous tree 'restored'?

No there is not need to restore the tree, the algo can continue splitting using A^B as the parent node and derive two child shares.

Yes, looking at @halseth's branch clarified it. I think I've been seeing more of a tree than it really is.

cfromknecht · 2021-03-30T18:50:53Z

So the receiver gets the cancel signal and cancels all htlcs back, but does the sender learn from that which htlcs were actually held by the receiver vs the ones that were still in-flight or didn't even reach the receiver? I suppose the sender can inspect the failure message to see what the origin of the failure is.

Yes, the source of the failure will tell the sender that all HTLCs were held by the receiver. If the sender intentionally fails reconstruction, they should get incorrect_payment_details on all of them as the reply.

Only when an htlc arrives at the receiver after the set has already been cancelled, and is then cancelled back, what does this look like to the sender? Will it be clear that that final htlc wasn't part of the set and reached the receiver "independently", possibly reusing liquidity that was just released?

The set is never cancelled unless the receiver has HTLCs satisfying the full amount. If any of the individual HTLCs are canceled early, that would happen with mpp_timeout.

joostjager · 2021-03-30T19:12:57Z

The set is never cancelled unless the receiver has HTLCs satisfying the full amount. If any of the individual HTLCs are canceled early, that would happen with mpp_timeout.

Yes, so mean the sender-requested early cancellation of the set that was discussed ("ok, can you guys cancel things back now"). For example:

sender sends htlc A, B and C
htlc A and C reach the receiver
C contains the cancel-early flag
A and C are failed back with incorrect_payment_details
htlc B reaches the receiver
B is also failed back with incorrect_payment_details?

Doesn't it appear to the sender as if there is liquidity for A, B and C, while in reality it could be that there isn't enough for all three at the same time?

Roasbeef · 2021-03-31T18:36:10Z

Yes, so mean the sender-requested early cancellation of the set that was discussed ("ok, can you guys cancel things back now")

It may have changed in this diff, but last I checked the early cancel isn't yet implemented, though it was something we realized we could. The main gate to reconstruction is still having the full amount present across the set of HTLCS: https://github.com/cfromknecht/lnd/blob/sendtoroute-amp/invoices/update.go#L216.

edit: Though I think rn it's possible to still trigger the behavior by sending an mpp payload that has the wrong total amount that mismatches the others.

Re your question: I guess it depends on how the sender interprets the failures and if it's path finding is aware of multiple HTLCs sharing the same path or not. Or maybe I don't understand the scenario.

Roasbeef · 2021-04-06T03:20:04Z

Just needs a rebase!

joostjager · 2021-04-06T09:07:43Z

invoices/invoiceregistry.go

+	// to create our AMP invoice.
+	payAddr := ctx.mpp.PaymentAddr()
+
+	// Create placeholder invoice.


After looking at lnd flame graphs for some time, I now realize that first creating and then updating the invoice is not ideal for maximum throughput. Everything database shows up prominently on the graph. Yes, for multiple shards it matters less, because multiple updates are needed anyway (although - isn't it possible to keep the invoice states fully in-memory until the set is complete? the htlcs are replayed anyway after a restart). But I can imagine that the majority of the cases will be successful single shard AMP payments. Especially when AMP turns out to be the platform of choice for implementing money streaming.

although - isn't it possible to keep the invoice states fully in-memory until the set is complete? the htlcs are replayed anyway after a restart

I guess we do it somewhat for record keeping purposes as well, since by updating it incrementally we can track the arrival, settle, and thus the "hodl" time for a given invoice across all the HTLCs. At a glance though, if we're willing to sacrifice that information, or collect it in an alternative manner (that may potentially be more lossy), I think the same potential optimization can be applied to MPP given that we'll also update the invoice incrementally (the state) as new HTLCs start to arrive.

This was also considered here: https://github.com/lightningnetwork/lnd/pull/5108/files/7a87a4c6627b6727facdfe1eb52aaeb5353f650a#r604347123

Worthy of a follow up if we don't think it's useful to know the history of prior failed attempts of a spontaneous payment.

This is to not conflict with trampoline BOLT.

Currently we use the AMP record type, this allows us to change it easily as we add new known records. We also bump this to 0x0c instead of 0x0a.

Currently we support queries by payHash or payHash+payAddr. For handling of AMP HTLCs, we only need to support querying by payAddr.

Also refactor existing unit tests to use them.

Adds a set of test cases that exercise the spontaneous AMP payment flow with valid and invalid reconstructions, as well as with single and multiple HTLCs. This also asserts that spontaneous AMP is gated behind the existing AcceptKeysend flag.

This will be reused by the amp itest.

cfromknecht force-pushed the sendtoroute-amp branch from bc85984 to a1443d7 Compare March 16, 2021 06:21

Roasbeef reviewed Mar 17, 2021

View reviewed changes

andrewshvv reviewed Mar 17, 2021

View reviewed changes

Roasbeef reviewed Mar 19, 2021

View reviewed changes

cfromknecht force-pushed the sendtoroute-amp branch 2 times, most recently from 1deef44 to e2c684e Compare March 22, 2021 19:46

Roasbeef added the amp label Mar 23, 2021

Roasbeef added this to the 0.13.0 milestone Mar 23, 2021

halseth self-requested a review March 24, 2021 19:32

cfromknecht force-pushed the sendtoroute-amp branch 2 times, most recently from c27b375 to 0dd4856 Compare March 25, 2021 02:53

cfromknecht requested a review from Roasbeef March 25, 2021 02:54

cfromknecht force-pushed the sendtoroute-amp branch from 0dd4856 to 4b98eec Compare March 25, 2021 03:09

cfromknecht marked this pull request as ready for review March 25, 2021 03:10

cfromknecht force-pushed the sendtoroute-amp branch from 4b98eec to 2ec35b6 Compare March 25, 2021 06:51

halseth suggested changes Mar 25, 2021

View reviewed changes

Roasbeef reviewed Mar 26, 2021

View reviewed changes

Roasbeef mentioned this pull request Mar 29, 2021

Add keysend invoice lightning/bolts#857

Closed

cfromknecht force-pushed the sendtoroute-amp branch from 2ec35b6 to 29786c5 Compare March 30, 2021 01:05

cfromknecht requested review from Roasbeef and halseth March 30, 2021 01:05

halseth approved these changes Mar 30, 2021

View reviewed changes

joostjager reviewed Mar 30, 2021

View reviewed changes

cfromknecht force-pushed the sendtoroute-amp branch from 29786c5 to 7a87a4c Compare March 30, 2021 18:57

halseth mentioned this pull request Mar 31, 2021

AMP support for SendPaymentV2 #5159

Merged

2 tasks

Roasbeef enabled auto-merge April 6, 2021 03:19

joostjager reviewed Apr 6, 2021

View reviewed changes

cfromknecht added 22 commits April 7, 2021 12:08

record: convert child_index from uint16 to uint32

8fe4de8

record/amp: use switch from 10 -> 14 for AMP TLV type

82b4f11

This is to not conflict with trampoline BOLT.

htlcswitch/hop: make unknown required type const

135a0a9

Currently we use the AMP record type, this allows us to change it easily as we add new known records. We also bump this to 0x0c instead of 0x0a.

htlcswitch/hop: parse and validate AMP records

c2729cb

features: define temporary AMP feature bits 30/31

438b03a

channeldb/invoice: add InvoiceRefByAddr

be66984

channeldb: make payhash on InvoiceRef optional

174d577

Currently we support queries by payHash or payHash+payAddr. For handling of AMP HTLCs, we only need to support querying by payAddr.

channeldb: refactor InvoiceRef.String() with all optional fields

7bed359

channeldb: validate feature dependencies when adding invoice

7e2f5a1

invoices: add checkSettleResolution and checkFailResolution

3fb70dd

Also refactor existing unit tests to use them.

invoices: add processAMP

88b72ab

channeldb/invoice_test: only set htlc-level preimage on accept

24d283e

channeldb/invoices: set AMP HTLC preimages when settling invoice

6780f74

channeldb/invoice: make Copy() a member of InvoiceHTLC

0b5be85

channeldb/invoices: rigorously test updateHtlc for MPP/AMP scenarios

2a49b59

invoices: reconstruct AMP child preimages

90a2550

invoices: add TestSpontaneousAmpPayment

ea934e1

Adds a set of test cases that exercise the spontaneous AMP payment flow with valid and invalid reconstructions, as well as with single and multiple HTLCs. This also asserts that spontaneous AMP is gated behind the existing AcceptKeysend flag.

channeldb/invoice: allow creating AMP invoices w/o preiamge

cfa9e95

lnrpc: add AMPRecord to Hop

00581ef

lnrpc: add UnmarshalAMP decoding

352ce10

lntest: make buildRoute method on mppTestContext

888af51

This will be reused by the amp itest.

lntest: add AMP itest

730b718

cfromknecht force-pushed the sendtoroute-amp branch from 7a87a4c to 730b718 Compare April 7, 2021 19:10

Roasbeef merged commit c998264 into lightningnetwork:master Apr 7, 2021

cfromknecht deleted the sendtoroute-amp branch April 13, 2021 17:01

cfromknecht mentioned this pull request Apr 13, 2021

channeldb/invoice: only allow pay-addr lookup w/ no query hash #5209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

channeldb+invoices: add spontaneous AMP receiving + sending via SendToRoute #5108

channeldb+invoices: add spontaneous AMP receiving + sending via SendToRoute #5108

cfromknecht commented Mar 15, 2021 •

edited

Loading

Roasbeef left a comment

Roasbeef Mar 17, 2021

andrewshvv Mar 17, 2021

Roasbeef Mar 19, 2021

cfromknecht Mar 22, 2021

Roasbeef Mar 22, 2021

cfromknecht Mar 25, 2021

joostjager Mar 30, 2021

halseth Mar 25, 2021

Roasbeef Mar 26, 2021

cfromknecht Mar 29, 2021

halseth Mar 25, 2021

halseth Mar 25, 2021

Roasbeef Mar 26, 2021

cfromknecht Mar 29, 2021

Roasbeef left a comment

Roasbeef Mar 26, 2021

Roasbeef Mar 26, 2021

halseth left a comment

halseth Mar 30, 2021

halseth Mar 30, 2021

joostjager Mar 30, 2021

joostjager Mar 30, 2021

joostjager Mar 30, 2021

cfromknecht Mar 30, 2021

joostjager Apr 1, 2021

cfromknecht commented Mar 30, 2021

joostjager commented Mar 30, 2021

Roasbeef commented Mar 31, 2021 •

edited

Loading

Roasbeef commented Apr 6, 2021

joostjager Apr 6, 2021

Roasbeef Apr 7, 2021

Roasbeef Apr 7, 2021

channeldb+invoices: add spontaneous AMP receiving + sending via SendToRoute #5108

channeldb+invoices: add spontaneous AMP receiving + sending via SendToRoute #5108

Conversation

cfromknecht commented Mar 15, 2021 • edited Loading

Roasbeef 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

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth 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

cfromknecht commented Mar 30, 2021

joostjager commented Mar 30, 2021

Roasbeef commented Mar 31, 2021 • edited Loading

Roasbeef commented Apr 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht commented Mar 15, 2021 •

edited

Loading

Roasbeef commented Mar 31, 2021 •

edited

Loading