AMP support for SendPaymentV2 #5159

halseth · 2021-03-31T13:09:21Z

This PR implements AMP payment support for the SendPaymentV2 API. Note that all details must still be specified manually, no payment request support is added yet.

Builds on #5108

TODO

Itest
Unit tests

halseth · 2021-03-31T13:11:36Z

routing/router.go

+// payments this will be the used SetID.
+func (l *LightningPayment) Identifier() [32]byte {
+	if l.amp != nil {
+		return l.amp.SetID


so the question is whether this is an okay identifier for the payment. This is what ill be used by the control tower to only allow a single payment using this ID. We could as discussed use this with a combination of the paymentAddress, but I think we want to allow several payments going to the same payment address?

yeah i think just using the setid is fine, before we discussed doing (pay_addr, set_id) but i think this is actually better. enforces that no two payments will ever have the same set_id, even if they are to different pay_addrs

cfromknecht

did a quick pass, so far changes look awesome 🔥

amp/shard_tracker.go

cfromknecht · 2021-03-31T18:48:56Z

routing/router.go

+
+	// If this is an AMP payment, we'll use the AMP shard tracker.
+	case payment.amp != nil:
+		shardTracker = amp.NewShardTracker(


lntest/itest/lnd_amp_test.go

lnrpc/routerrpc/router_backend.go

cfromknecht · 2021-03-31T18:59:15Z

routing/router.go

+// payments this will be the used SetID.
+func (l *LightningPayment) Identifier() [32]byte {
+	if l.amp != nil {
+		return l.amp.SetID


yeah i think just using the setid is fine, before we discussed doing (pay_addr, set_id) but i think this is actually better. enforces that no two payments will ever have the same set_id, even if they are to different pay_addrs

cfromknecht · 2021-03-31T19:00:30Z

lntest/itest/lnd_amp_test.go

+	require.NoError(t.t, err)
+
+	ctxt, _ := context.WithTimeout(context.Background(), 4*defaultTimeout)
+	payment := sendAndAssertSuccess(


really digging how concise this itest is, almost as if we skipped a step 😎

joostjager · 2021-04-01T06:43:15Z

amp/shard_tracker.go

+	}
+
+	// Use a random child index.
+	var childIndex [4]byte


With a 32-bit random value, isn't the risk of a collision too high? Maybe the htlc id or the pid can be used here or a set-specific incrementing id.

I guess it depends on how it is being used. Currently a child gets created, and if it fails we xor the share back into the parent. This means that it will be split again most likely (which adds 32 byte randomness) before a new shard is sent.

In the scenario where it's the last shard that is being created, we can end up deriving multiple children from it. However, you must send a lot of attempts before a collision is likely. Maybe that potential privacy leak is not worth optimizing out? (since I think using the same index only would be a privacy concern)

Yes, just wanted to bring it up. I think it is safe to assume that overall the vast majority of the payments will be single shot and in a post-hyperbitcoinization world with money streaming from every netflix subscriber - maybe collisions happen.

And what about adding a counter field to the last share struct? That doesn't leak info about other payments to the receiver.

at 32-bits, we expect a payment hash collision after 2^16 retries of the same HTLC. even at 1 sec per attempt, that's continually resending the same AMP HTLC for 18 hours. AMP HTLCs with different shares shouldn't ever collide in practice because they have plenty of entropy.

if we are really concerned about collisions, we could use a CTR mode (random offset + increment) and then we'd only get collisions on wrap around. in the prior example—after 136 years of sending the same HTLC. in the end, the worst that happens is payment hash reuse. if a new use case comes along that might require this sort of sustained retry behavior then we can easily switch to CTR without any protocol changes

But what is wrong with my suggestion to use a counter to not have this problem at all, without sacrificing privacy?

More, real work, for only a theoretical gain? :p

in a post-hyperbitcoinization world with money streaming from every netflix subscriber - maybe collisions happen.

In case it isn't clear, this index is just within the context of a given setID. So the ensure that all the world streaming netflix payment doesn't collide, we just need to ensure that the setID is unique, which easy given the negligible probability of colliding 256-bit integers.

Ultimately, with the route Johan has taken here with the implementation, since we just XOR out the failed shares rather than increment the childIndex (which would result in a distinct payment hash for a same share), we don't really need to worry about this colliding in practice. The alternative would've been to get another random child index, for the same share, then try again (uses a diff payment hash, but same share). This alternative route would result in more collisions in practice.

The context was clear. I just meant to say that if massive amounts of payments happen, the overall chance of a collision somewhere on the earth increases. It was totally clear that independent payments cannot collide. Let's say in some time frame, 2^32 independent payments happen that need a second attempt. There will then be on average one user that experiences a collision.

The out-xor'ing does not apply to the final share and I'd say that almost all AMP payments will be single-shot and have only a final share that may be retried with a different child index.

The alternative would've been to get another random child index, for the same share, then try again (uses a diff payment hash, but same share). This alternative route would result in more collisions in practice.

So what I am proposing is to just keep a counter for each (split) share that is generated. So there will be a counter within the context of a final share. Then keep incrementing that with each attempt. Then there is no risk of collision, so in my opinion a better option.

Is this like something you had in mind? #5255

However there is still the case that after 2^32 "last shards" the counter will wrap around and collisions will start happening.

joostjager · 2021-04-01T06:51:05Z

amp/shard_tracker.go

+	s.shards[pid] = child
+
+	mpp := record.NewMPP(s.totalAmt, s.paymentAddr)
+	amp := record.NewAMP(


Question about record.NewAMP: Why is the first parameter named rootShare? Shouldn't it be just share?

at the time i think I used that as short hand for shareOfRoot, but yes just using share would be better

joostjager · 2021-04-01T07:25:18Z

amp/shard_tracker.go

+		// zero share to indicate we cannot split it further.
+		s.sharer.curr = zeroShare
+	} else {
+		left, sharer, err := s.sharer.Split()


Minor comment: The naming left and right led me to think that the xor tree is more than it really is. Somehow I expected left and right to be different. That there is some kind of condition that determines what's left and what's right. Could be that I am the only one that has this.

Perhaps something like share and remainder could work too.

It can be hard to grok at first as there's actually less enforced structure than one might think. For example, there's no requirement that we split using our current right-skewed binary tree. Instead we could opt to make it more balanced. With the way Split() works, left is a random value while right is the parent xor'd with that random value. Rather then incrementing the child index for left (which will generate a new payment hash but keep the same share), this PR instead "destroys" left by XOR'ing that value out of right, right can then be split again to get new randomness and retry the payment.

Yeah, I found it easier to just visualize it as a set off pebbles that we can split and merge at will.

A tree is perhaps not the best description, as we can also merge leafs from different sub-branches IIUC? @cfromknecht

Indeed, something like a cord that you can cut and glue together again matches much better with how I assume it works.

joostjager · 2021-04-01T07:33:48Z

amp/shard_tracker.go

+			return nil, err
+		}
+
+		s.sharer = sharer.(*SeedSharer)


Another option could be that sharer.Split splits itself (maybe sharer.Spawn?). It returns a single share and updates its internal share to be the remainder. Then it isn't necessary to create a new instance. I think it could be easier to understand how it works that way.

htlcswitch/switch.go

routing/payment_lifecycle.go

routing/shards/shard_tracker.go

amp/shard_tracker.go

Roasbeef · 2021-04-02T00:38:53Z

lnrpc/routerrpc/router_backend.go

+		if features != nil {
+			err = feature.ValidateDeps(features)
+			if err != nil {
+				return nil, err


This is an opportunity to start returning better information back to the caller to explain why the payment can't succeed. As a test I tried to pay Conner's AMP invoice from my unupgraded node and got "no routes found" on the command line rather than "payment type unsupported" or something like that.

In order to do this, we'd need to add the new error return type in path finding itself if we fail to find a path due to a destination node's feature incompatibility. Alternatively, we can do a check for dest feature bit compatibility much earlier in the cycle.

I think that would mean it actually did not find a route? Since if it did, the destination node should send an error message because of the missing AMP payload, which would be the error you would see.

Yeah so I think we'll end up doing this in the follow up PR that adds the basic invoicing information. In either case, this would've ideally been done in the prior release so older nodes will, know they can't pay an invoice since it's AMP, and not that there doesn't exist a path.

lnrpc/routerrpc/router_backend.go

lntest/itest/lnd_amp_test.go

Roasbeef · 2021-04-09T03:38:56Z

Did some payments on testnet using this branch, here're my findinsg:

Works as advertised!
This PR is missing basic CLI support, here's a hacky commit to enable that: eaa8a10
We don't set the AMP records in the LIstPayments response, so there's no way to distinguish an AMP payment from a regular one from the PoV of that API.
Right now the --accept-keysend flag is needed in order to be able to receive, we should mirror it with something AMP specific to make it a bit clearer.
The ListInvoices response has a is_keysend field, do we also want to mirror this w/ AMP? If some users are relying on this field right now (like maybe Sphinx Chat?), then perhaps we should also always set for AMP payments to smooth the transition.

cfromknecht · 2021-04-09T22:46:44Z

The ListInvoices response has a is_keysend field, do we also want to mirror this w/ AMP? If some users are relying on this field right now (like maybe Sphinx Chat?), then perhaps we should also always set for AMP payments to smooth the transition.

I have this item in a separate branch, but we can move them to this PR if we would rather have it here.

EDIT: scratch that, my CLI commit is an --amp flag for the AddInvoice side, still need the one for SendPayment 👍

halseth · 2021-04-12T14:12:38Z

Ok, this is now ready for final review, pushed unit tests for the new modules.

This PR is missing basic CLI support, here's a hacky commit to enable that: eaa8a10

We don't set the AMP records in the LIstPayments response, so there's no way to distinguish an AMP payment from a regular one from the PoV of that API.

Right now the --accept-keysend flag is needed in order to be able to receive, we should mirror it with something AMP specific to make it a bit clearer.

The ListInvoices response has a is_keysend field, do we also want to mirror this w/ AMP? If some users are relying on this field right now (like maybe Sphinx Chat?), then perhaps we should also always set for AMP payments to smooth the transition.

Yeah, lets create another PR for the CLI/RPC changes.

Roasbeef · 2021-04-13T22:39:46Z

@halseth SGTM, so then will we address the above comments in @cfromknecht's upcoming PR?

Roasbeef · 2021-04-14T22:31:02Z

Linter failing rn:

amp/derivation_test.go:15:2: U1000: field `zeroShare` is unused (unused)
	zeroShare bool
	^
amp/derivation_test.go:102:3: SA4006: this value of `sharer` is never used (staticcheck)
		sharer = sharer.Zero()

Roasbeef

LGTM 🍃

Roasbeef · 2021-04-09T01:52:33Z

routing/router.go

@@ -165,17 +165,21 @@ type PaymentAttemptDispatcher interface {
 	// denoted by its public key. A non-nil error is to be returned if the
 	// payment was unsuccessful.
 	SendHTLC(firstHop lnwire.ShortChannelID,
-		paymentID uint64,
+		attemptID uint64,


Solid change 👍

Roasbeef · 2021-04-09T02:05:29Z

routing/payment_lifecycle.go

+	// Now that we are failing this payment attempt, cancel the shard with
+	// the ShardTracker such that it can derive the correct hash for the
+	// next attempt.
+	if err := p.shardTracker.CancelShard(attempt.AttemptID); err != nil {


+1 for adding this to the main interface, makes the control flow a lot easier to fllow

Roasbeef · 2021-04-14T22:32:33Z

amp/shard_tracker_test.go

+
+// TestAMPShardTracker tests that we can derive and cancel shards at will using
+// the AMP shard tracker.
+func TestAMPShardTracker(t *testing.T) {


routing/router.go

Roasbeef · 2021-04-14T22:44:32Z

amp/shard_tracker.go

+	}
+
+	// Use a random child index.
+	var childIndex [4]byte


in a post-hyperbitcoinization world with money streaming from every netflix subscriber - maybe collisions happen.

In case it isn't clear, this index is just within the context of a given setID. So the ensure that all the world streaming netflix payment doesn't collide, we just need to ensure that the setID is unique, which easy given the negligible probability of colliding 256-bit integers.

Ultimately, with the route Johan has taken here with the implementation, since we just XOR out the failed shares rather than increment the childIndex (which would result in a distinct payment hash for a same share), we don't really need to worry about this colliding in practice. The alternative would've been to get another random child index, for the same share, then try again (uses a diff payment hash, but same share). This alternative route would result in more collisions in practice.

halseth · 2021-04-15T13:03:22Z

fixed, but travis having a bad day

Roasbeef · 2021-04-26T20:41:02Z

Needs a rebase!

We might as well return all info, and we'll need the individual HTLCs in later commits.

To distinguish the attempt's unique ID from the overall payment identifier, we name it attemptID everywhere, and note that the paymentHash argument won't be the actual payment hash for AMP payments.

We'll use this to keep track of the outstanding shards and which preimages we are using for each. For now this is a simple map from attempt ID to hash, but later we'll hide the AMP child derivation behind this interface.

We'll let the payment's lifecycle register each shard it's sending with the ShardTracker, canceling failed shards. This will be the foundation for correct AMP derivation for each shard we'll send.

For AMP payments the hash used for each HTLC will differ, and we will need to retrive it after a restart. We therefore persist it with each attempt.

We'll use this AMP-specific ShardTracker for AMP payments. It will be used to derive hashes for each HTLC attempt using the underlying AMP derivation scheme.

We'll use the AMP-specific ShardTracker for payments having non-nil AMPOptions.

For now this is how you indicate you want to perform an AMP payment to the destination.

Since we want to support AMP payment using a different unique payment identifier (AMP payments don't go to one specific hash), we change the nomenclature to be Identifier instead of PaymentHash.

…endPayment

cfromknecht

Did another pass, I think this is ready to land! LGTM 🥳

halseth requested review from cfromknecht and joostjager March 31, 2021 13:09

halseth commented Mar 31, 2021

View reviewed changes

halseth requested review from Roasbeef and removed request for joostjager March 31, 2021 14:05

Roasbeef added this to the 0.13.0 milestone Mar 31, 2021

Roasbeef added amp enhancement Improvements to existing features / behaviour routing labels Mar 31, 2021

cfromknecht reviewed Mar 31, 2021

View reviewed changes

joostjager reviewed Apr 1, 2021

View reviewed changes

Roasbeef reviewed Apr 2, 2021

View reviewed changes

halseth force-pushed the amp-sendpayment branch 6 times, most recently from 8a7aa7c to b492a7f Compare April 8, 2021 19:34

halseth force-pushed the amp-sendpayment branch 2 times, most recently from 0c2d86b to f86e3a9 Compare April 12, 2021 14:05

halseth force-pushed the amp-sendpayment branch from f86e3a9 to 67190e0 Compare April 14, 2021 17:04

Roasbeef approved these changes Apr 14, 2021

View reviewed changes

halseth force-pushed the amp-sendpayment branch from 67190e0 to f3ab538 Compare April 15, 2021 09:16

halseth force-pushed the amp-sendpayment branch from f3ab538 to b6a1413 Compare April 15, 2021 18:59

Roasbeef requested a review from cfromknecht April 21, 2021 15:51

halseth added 3 commits April 27, 2021 08:27

channeldb: return full payment for inflight payments

7795353

We might as well return all info, and we'll need the individual HTLCs in later commits.

router+switch: rename paymentID->attemptID

a9f19b1

To distinguish the attempt's unique ID from the overall payment identifier, we name it attemptID everywhere, and note that the paymentHash argument won't be the actual payment hash for AMP payments.

routing/shards: add ShardTracker interface

6474b25

We'll use this to keep track of the outstanding shards and which preimages we are using for each. For now this is a simple map from attempt ID to hash, but later we'll hide the AMP child derivation behind this interface.

halseth force-pushed the amp-sendpayment branch from b6a1413 to 75f0cd6 Compare April 27, 2021 06:56

halseth added 14 commits April 27, 2021 09:43

routing/payment_lifecycle: use ShardTracker to track shards

41ae353

We'll let the payment's lifecycle register each shard it's sending with the ShardTracker, canceling failed shards. This will be the foundation for correct AMP derivation for each shard we'll send.

channedb/mp_payment: add Hash to individual HTLCs

06f045f

For AMP payments the hash used for each HTLC will differ, and we will need to retrive it after a restart. We therefore persist it with each attempt.

routing/router: use attempt's unique hash if set on restart

e1399fb

amp: add Merge and Zero to Sharer interface

c1e82e5

amp: create amp.ShardTracker

2d397b1

We'll use this AMP-specific ShardTracker for AMP payments. It will be used to derive hashes for each HTLC attempt using the underlying AMP derivation scheme.

routing: use AMP shard tracker

5531b81

We'll use the AMP-specific ShardTracker for payments having non-nil AMPOptions.

routerrpc: add amp bool to SendPayment

6104d12

For now this is how you indicate you want to perform an AMP payment to the destination.

routing: use Identifier in place of PaymentHash

f07c9d0

Since we want to support AMP payment using a different unique payment identifier (AMP payments don't go to one specific hash), we change the nomenclature to be Identifier instead of PaymentHash.

router: use SetID as identifier for SendToRoute AMP-payments

56a2c65

lnrpc: add AMP feature bits

0b9137c

routerrpc: validate DestFeatures from RPC

8f57dcf

routerrpc+routing: set AMP options for payments specified as AMP in S…

c4fc72d

…endPayment

itest: make sendAndAssertSuccess take context

a2a61a1

itest: add SendPayment AMP itest

13c0012

halseth force-pushed the amp-sendpayment branch from 75f0cd6 to 13c0012 Compare April 27, 2021 07:47

cfromknecht approved these changes Apr 27, 2021

View reviewed changes

cfromknecht merged commit d771ed7 into lightningnetwork:master Apr 27, 2021

halseth mentioned this pull request Apr 28, 2021

amp: add counter for child index #5255

Draft

AMP support for SendPaymentV2 #5159

AMP support for SendPaymentV2 #5159

Conversation

halseth commented Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht 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

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 commented Apr 9, 2021 • edited Loading

cfromknecht commented Apr 9, 2021 • edited Loading

halseth commented Apr 12, 2021

Roasbeef commented Apr 13, 2021

Roasbeef commented Apr 14, 2021

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

halseth commented Apr 15, 2021

Roasbeef commented Apr 26, 2021

cfromknecht left a comment

Choose a reason for hiding this comment

halseth commented Mar 31, 2021 •

edited

Loading

Roasbeef commented Apr 9, 2021 •

edited

Loading

cfromknecht commented Apr 9, 2021 •

edited

Loading