Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

single-shot, sender-side mpp via sendtoroute #3442

Merged
merged 11 commits into from Nov 5, 2019

Conversation

@cfromknecht
Copy link
Collaborator

cfromknecht commented Aug 30, 2019

This PR lays some more groundwork for our road to MPP payments in lnd. Here we add the ability to send MPP shards from the sender-side via SendToRoute, whose additional fields are properly encoded in the onion blob and delivered to the receiver.

Apart from verifying that mpp records are not received at intermediate hops, the receiver does not perform any additional validation of the actual fields contained within. The receiver side has been modified to deliver these values all the way down to the invoice registry, but they are not inspected. Thus, the receiver will process the payment as they would a traditional payment. The additional behavior to enforce the MPP fields will be added in #3415, at which point the behaviors will diverge.

For now, the integration tests have been expanded to cover the three send to route endpoints Lightning.SendToRoute, Lightning.SendToRouteSync, and RouterRPC.SendToRoute, as well as testing that each can properly pay with single MPP shards. Future changes will continue to expand the assertions as more pieces come into place.

@cfromknecht cfromknecht force-pushed the cfromknecht:router-registry branch from e077cc4 to 6c7f713 Aug 30, 2019
Copy link
Collaborator

joostjager left a comment

I also experienced that the onion blob decryption code needs to be moved out of htlcswitch so that it can be used in cnct without a circular reference. Will we move that into htlcswitch/payload too?

htlcswitch/payload/hop.go Outdated Show resolved Hide resolved
htlcswitch/payload/hop.go Outdated Show resolved Hide resolved
@cfromknecht cfromknecht force-pushed the cfromknecht:router-registry branch 7 times, most recently from d08300d to 0b70f6d Aug 30, 2019
@cfromknecht

This comment has been minimized.

Copy link
Collaborator Author

cfromknecht commented Aug 30, 2019

I also experienced that the onion blob decryption code needs to be moved out of htlcswitch so that it can be used in cnct without a circular reference. Will we move that into htlcswitch/payload too?

Discussed offline. Plan is to move decryption (HopIterator) into the hop package and replace with hop.Iterator

lnrpc/routerrpc/router.proto Outdated Show resolved Hide resolved
record/mpp.go Outdated Show resolved Hide resolved

/**
If non-zero, the total amount in satoshis being sent as part of a larger
multi-path payment. The caller is responsible for ensuring subpayments to

This comment has been minimized.

Copy link
@halseth

halseth Sep 30, 2019

Collaborator

This comment is a bit confusing, should say something like "the total amount in satoshis of the payment if this is a sub payment of smaller amount"? I don't know what the best way to put this..

routing/route/route.go Outdated Show resolved Hide resolved
@cfromknecht cfromknecht force-pushed the cfromknecht:router-registry branch 2 times, most recently from a8001c5 to 28e9d02 Oct 10, 2019
@cfromknecht cfromknecht force-pushed the cfromknecht:router-registry branch 3 times, most recently from 2dcb941 to b94383e Oct 12, 2019
@cfromknecht cfromknecht marked this pull request as ready for review Oct 15, 2019
@cfromknecht cfromknecht requested a review from Roasbeef as a code owner Oct 15, 2019
@cfromknecht cfromknecht requested a review from joostjager Oct 15, 2019
@joostjager joostjager added this to the 0.9.0 milestone Oct 31, 2019
@cfromknecht

This comment has been minimized.

Copy link
Collaborator Author

cfromknecht commented Oct 31, 2019

Also, PR desc is still empty.

@joostjager added

Copy link
Collaborator

joostjager left a comment

Ack on restructuring db to single tlv blob (also containing legacy payload fields) in a follow-up.

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Nov 1, 2019

ACK on using single blob in follow-up. I still have opinions about not having the optional MPP record be its own field on the struct, but that we can discuss in the follow-up.

@halseth
halseth approved these changes Nov 1, 2019
Copy link
Collaborator

halseth left a comment

LGTM

name string

// streaming tests streaming SendToRoute if true, otherwise tests
// synchronous SenToRoute.

This comment has been minimized.

Copy link
@halseth

halseth Nov 1, 2019

Collaborator

Hm, when is the time to get rid of one of these RPCs?

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Nov 2, 2019

Author Collaborator

i wouldn't be opposed to deprecating the streaming version and telling people to switch to SendToRouteSync, or RouterRPC.SendToRoute if they don't mind dealing with breaking changes.

lntest/itest/lnd_test.go Show resolved Hide resolved
@cfromknecht cfromknecht force-pushed the cfromknecht:router-registry branch from 8da65db to 44e6e80 Nov 2, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Nov 2, 2019

Needs a rebase! Then I think this is ready to land.

@cfromknecht

This comment has been minimized.

Copy link
Collaborator Author

cfromknecht commented Nov 2, 2019

@Roasbeef rebased!

@cfromknecht cfromknecht force-pushed the cfromknecht:router-registry branch from 4a9d077 to 3be2935 Nov 2, 2019
@joostjager joostjager moved this from Needs Review to Approved in v0.9.0-beta Nov 4, 2019
@cfromknecht cfromknecht force-pushed the cfromknecht:router-registry branch from 3be2935 to 3b5185d Nov 4, 2019
cfromknecht added 3 commits Nov 4, 2019
These encoders can be composed to create composite types without
incurring additional allocations that would be required to pass the
truncated types through the generic interface.
Used to encode/decode MPP tlv records
This commit also modifies the Router serialization to persist the MPP
struct when present, and properly restore it when loading from disk.
@cfromknecht cfromknecht force-pushed the cfromknecht:router-registry branch from 3b5185d to e2baeb3 Nov 4, 2019
cfromknecht added 8 commits Nov 4, 2019
This commit add mpp_total_amt_msat and mpp_payment_addr to the Hop
message. Doing so enables users submitting mpp payments via rpc to
set these parameters for the destination. In addition, it will allow us
to display these fields in rpc responses.
This commit parses mpp_total_amt_msat and mpp_payment_addr from the
SendToRoute rpc and populates an MPP record on the internal hop
reprsentation. When the router goes to encode the onion packet, these
fields will be serialized for the destination. We also populate the mpp
fields when marshalling routes in rpc responses.
We already wait for all channels to open before creating the payment
requests.
In this commit, we refactor the testSingleHopSendToRoute test to support
table driven tests for various endpoints and payment types. Currently
only the main rpcserver's SendToRoute is tested, so we also add
support the SendToRouteSync and the routerrpc's SendToRoute.

The tests are also modified to have each endpoint perform a single-hop,
single-shot MPP payment. This asserts that the Hop messages are being
properly unmarshalled and that setting correctly yields a successful
payment. At the momemnt the receiver does not actually verify or use the
MPP fields presented in the onion, though this test will be expanded
later as those pieces are assembled.
@cfromknecht cfromknecht force-pushed the cfromknecht:router-registry branch from e2baeb3 to 540e44e Nov 4, 2019
@Roasbeef Roasbeef merged commit 7ecb713 into lightningnetwork:master Nov 5, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 62.425%
Details
v0.9.0-beta automation moved this from Approved to Done Nov 5, 2019
@cfromknecht cfromknecht deleted the cfromknecht:router-registry branch Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.9.0-beta
  
Done
4 participants
You can’t perform that action at this time.