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

channeldb+rpcserver: expose legacy payments as multi-path payments #3499

Merged
merged 9 commits into from Nov 22, 2019

Conversation

cfromknecht
Copy link
Collaborator

@cfromknecht cfromknecht commented Sep 12, 2019

Builds on #3442

This PR extends the ListPayments, RouterRPC.SendPayment, and RouterRPC.TrackPayment to include an experimental field for displaying HTLC attempts for a given payment. This will us to display multiple HTLCs when MPP is fully integrated. Legacy payments will have at most one attempt, and we simply convert them into a more generic MPPayment struct after loading from disk.

In the future we will perform a migration to convert the legacy on-disk format into real MPP payments on disk, but this allows us to fleshing out the API before the internal work is completed.

channeldb/payments.go Outdated Show resolved Hide resolved
channeldb/payments.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
channeldb/mp_payment.go Outdated Show resolved Hide resolved
channeldb/mp_payment.go Outdated Show resolved Hide resolved
lnrpc/rpc.proto Outdated Show resolved Hide resolved
channeldb/mp_payment.go Outdated Show resolved Hide resolved
channeldb/payments.go Show resolved Hide resolved
lnrpc/rpc.proto Outdated Show resolved Hide resolved
lnrpc/rpc.proto Show resolved Hide resolved
@cfromknecht cfromknecht force-pushed the mpp-payments-rpc branch 3 times, most recently from 24852d6 to 0369eb7 Compare Sep 16, 2019
Copy link
Collaborator

@halseth halseth left a comment

Nice, I really like the limited scope of this PR! Makes it easy to review, and it is a great first step :)

Gave it a high level look, looks more or less good to me, but will need to see it in tandem with upcoming MPP PRs to make sure things fit 👍

channeldb/payments.go Outdated Show resolved Hide resolved
lnrpc/rpc.proto Outdated Show resolved Hide resolved
@Roasbeef Roasbeef added routing database P2 rpc amp and removed routing labels Oct 3, 2019
channeldb/mp_payment.go Outdated Show resolved Hide resolved
channeldb/mp_payment.go Outdated Show resolved Hide resolved
Copy link
Member

@Roasbeef Roasbeef left a comment

Solid base groundwork towards multi-pathing all the things 👍

lnrpc/rpc.proto Outdated Show resolved Hide resolved
lnrpc/rpc.proto Outdated Show resolved Hide resolved
cmd/lncli/types.go Outdated Show resolved Hide resolved
Copy link
Member

@Roasbeef Roasbeef left a comment

What about the PaymentStatus call in the router sub-server?

@cfromknecht cfromknecht added this to the 0.9.0 milestone Nov 2, 2019
@cfromknecht cfromknecht added this to WIP in v0.9.0-beta via automation Nov 2, 2019
@cfromknecht cfromknecht moved this from WIP to Needs Review in v0.9.0-beta Nov 2, 2019
@cfromknecht cfromknecht force-pushed the mpp-payments-rpc branch 2 times, most recently from 5ce2ebc to b1db05a Compare Nov 2, 2019
@cfromknecht
Copy link
Collaborator Author

@cfromknecht cfromknecht commented Nov 2, 2019

What about the PaymentStatus call in the router sub-server?

Last few commits now add the HTLC attempts to PaymentStatus

I rebased everything over #3442, which also allowed me test that the MPP record is being set properly in the new sendtoroute itests. This actually pointed out a bug in the test where we would send before receiving the node ann, causing us to downgrade to legacy payments for some of the htlcs. That bug has been fixed in #3442, and here we extend the itest with additional checks on the listpayments rpc.

@cfromknecht cfromknecht force-pushed the mpp-payments-rpc branch 2 times, most recently from 6b01397 to 36798d3 Compare Nov 2, 2019
channeldb/payments.go Show resolved Hide resolved
channeldb/mp_payment.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_backend.go Show resolved Hide resolved
@joostjager joostjager changed the title channeldb+rcpserver: expose legacy payments as mutli-path payments channeldb+rpcserver: expose legacy payments as mutli-path payments Nov 6, 2019
@joostjager joostjager changed the title channeldb+rpcserver: expose legacy payments as mutli-path payments channeldb+rpcserver: expose legacy payments as multi-path payments Nov 6, 2019
Copy link
Collaborator

@halseth halseth left a comment

I think this is very close, no major comments :)

channeldb/mp_payment.go Show resolved Hide resolved
channeldb/mp_payment.go Outdated Show resolved Hide resolved
channeldb/payments.go Show resolved Hide resolved
lnrpc/rpc.proto Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
routing/control_tower.go Show resolved Hide resolved
routing/control_tower.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@halseth halseth left a comment

LGTM 😱

channeldb/mp_payment.go Show resolved Hide resolved
channeldb/payments.go Outdated Show resolved Hide resolved
channeldb/payments.go Show resolved Hide resolved
@@ -192,16 +191,17 @@ func (p *PaymentControl) RegisterAttempt(paymentHash lntypes.Hash,
// duplicate payments to the same payment hash. The provided preimage is
// atomically saved to the DB for record keeping.
func (p *PaymentControl) Success(paymentHash lntypes.Hash,
preimage lntypes.Preimage) (*route.Route, error) {
preimage lntypes.Preimage) (*MPPayment, error) {
Copy link
Collaborator

@halseth halseth Nov 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would be nice if these channeldb changes could get their own commit

Copy link
Collaborator Author

@cfromknecht cfromknecht Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i attempted to do so originally, but it ended up causing more code shuffling than it was worth. the extraction code ends up being duplicated and move around, the tests end up double testing various aspects, etc over 4 commits. so i squashed it all into one since it's i found it easier to just look at the whole change

lnrpc/routerrpc/router_server.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_server.go Outdated Show resolved Hide resolved
channeldb/mp_payment.go Show resolved Hide resolved
channeldb/mp_payment.go Outdated Show resolved Hide resolved
channeldb/payments.go Outdated Show resolved Hide resolved
lnrpc/rpc.proto Show resolved Hide resolved
lnrpc/routerrpc/router_backend.go Show resolved Hide resolved
routing/control_tower_test.go Outdated Show resolved Hide resolved
cfromknecht added 9 commits Nov 20, 2019
Preliminary step to exposing PaymentPreimage() as a method that can be
shared between legacy payments and mutli-path payments.
This commit modifies the FetchPayment method to return MPPayment structs
converted from the legacy on-disk format. This allows us to attach the
HTLCs to the events given to clients subscribing to the outcome of an
HTLC.

This commit also bubbles up to the routerrpc/router_server, by
populating HTLCAttempts in the response and extracting the legacy route
field from the HTLCAttempts.
Copy link
Collaborator

@joostjager joostjager left a comment

LGTM. Only wondering how relevant this change is now that we aim for single shot mpp first?

case htlc.Failure != nil && result.FailureReason ==
channeldb.FailureReasonPaymentDetails:

legacyRoute = &htlc.Route
Copy link
Collaborator

@joostjager joostjager Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better than just an if statement? Or at least use fallthrough?

Route route.Route

// AttemptTime is the time at which this HTLC was attempted.
AttemptTime time.Time
Copy link
Collaborator

@joostjager joostjager Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field doesn't seem to be set at all in this pr. Remove and introduce in the follow up?

Copy link
Collaborator

@joostjager joostjager left a comment

Discussed offline, proceeding in parallel with single shot mpp

@joostjager joostjager moved this from Needs Review to Approved in v0.9.0-beta Nov 20, 2019
@Roasbeef Roasbeef merged commit 6f9fcfa into lightningnetwork:master Nov 22, 2019
1 of 2 checks passed
v0.9.0-beta automation moved this from Approved to Done Nov 22, 2019
@cfromknecht cfromknecht deleted the mpp-payments-rpc branch Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
amp database P2 rpc v0.9.0
Projects
No open projects
v0.9.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants