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

routerrpc: async payment rpc #2973

Merged
merged 9 commits into from Jun 5, 2019

Conversation

joostjager
Copy link
Collaborator

@joostjager joostjager commented Apr 18, 2019

This PR adds a new SendPayment payment call to the routerrpc sub server. Notable changes:

  • Configurable payment timeout (this is not to "cancel" htlcs in flight, which is impossible, but controls what the point is after which a new payment attempt isn't started anymore)

  • Fee limit can only be specified as an absolute value. There is no default anymore. Specifying a value of zero means that no fees may be paid. In that case, only a payment to a direct peer would succeed.

  • Only payment level failures are reported. This is currently limited to timeout and no_routes, but will be extended in a follow up pr routerrpc: add more failure reasons and route hints #3156. The goal is to report terminal failures like payment_hash_unknown, but not to expose the result of the last payment attempt anymore.

In addition to that, a new rpc call TrackPayment is added. Using TrackPayment an in-flight payment can be picked up after the rpc connection was lost on SendPayment. When #2762 is merged, this also allows picking up payments after a restart of lnd.

@joostjager joostjager force-pushed the newer-payment-rpc branch 3 times, most recently from 3aee088 to 25bc701 Compare April 18, 2019 14:12
@Roasbeef Roasbeef added this to the 0.7 milestone Apr 18, 2019
@Roasbeef Roasbeef requested a review from halseth April 18, 2019 23:30
@Roasbeef Roasbeef added gRPC incomplete WIP PR, not fully finalized, but light review possible payments Related to invoices/payments routing labels Apr 20, 2019
@Roasbeef
Copy link
Member

Shouldn't this be branched off of #2761?

lnrpc/routerrpc/router_backend.go Outdated Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router.proto Show resolved Hide resolved
lnrpc/routerrpc/router_backend.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_server.go Show resolved Hide resolved
@joostjager
Copy link
Collaborator Author

Shouldn't this be branched off of #2761?

Yes, this is still wip. Should have added [wip] to the title.

@joostjager joostjager changed the title routerrpc: new payment rpc routerrpc: new payment rpc [wip] Apr 30, 2019
@joostjager joostjager force-pushed the newer-payment-rpc branch 8 times, most recently from ecfdf21 to 17a98d4 Compare May 3, 2019 07:29
@Roasbeef
Copy link
Member

Can be rebased now that #2761 is in.

@joostjager joostjager force-pushed the newer-payment-rpc branch 7 times, most recently from ed4fd68 to 3a60e31 Compare May 28, 2019 11:45
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Awesome to see the latest changes! It was right to move subscription handling into routing by moving the ControlTower interface there; the separation is clean 👍

I also like the new APIs, they are easy to reason about.

lntypes/hash.go Show resolved Hide resolved
routing/control_tower.go Show resolved Hide resolved
routing/control_tower.go Outdated Show resolved Hide resolved
// succeeded, new subscriber is registered and will immediately receive
// the succeeded status, this function sends out the succeeded
// notification again to the same subscriber.
p.hashMutex.Lock(string(paymentHash[:]))
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like lock can acquired after DB transaction is over.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason the lock needs to be acquired before the db transaction is to prevent a new subscription happening after the db transaction, but before notifyFinalEvent, as is described in the comment above. Maybe I misunderstand you suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

From reading the comment that sounds alright though?

Payment status is updated to succeeded, new subscriber comes in and (acquire lock) retrieves succeed status and sends notification (release lock), this function (acquire lock) finds no one to send notification to (release lock).

Or is there another order of actions that require us to hold it while we update the DB?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated code to only use a global lock. It indeed seems to be sufficient.

routing/control_tower.go Show resolved Hide resolved
lnrpc/routerrpc/router_server.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_server.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_server.go Show resolved Hide resolved
rpcserver.go Show resolved Hide resolved
cmd/lncli/commands.go Show resolved Hide resolved
@joostjager
Copy link
Collaborator Author

@halseth ptal

@joostjager joostjager force-pushed the newer-payment-rpc branch 3 times, most recently from d9745ed to 9e51fac Compare May 30, 2019 21:23
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM after removal pf last commit 👍

routing/control_tower.go Outdated Show resolved Hide resolved
routing/control_tower.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router.proto Show resolved Hide resolved
if err != nil {
log.Debugf("SendPayment async result for hash %x: %v",
Copy link
Contributor

Choose a reason for hiding this comment

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

Errorf?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed using different log level for expected and unexpected errors.

cmd/lncli/commands.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the newer-payment-rpc branch 5 times, most recently from 71f1c03 to 74e4574 Compare June 3, 2019 22:03
@joostjager
Copy link
Collaborator Author

Brought back the payment update stream and the IN_FLIGHT payment state.

ptal @halseth @Roasbeef

Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

@@ -187,9 +192,12 @@ 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) error {
preimage lntypes.Preimage) (*route.Route, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid having to do this change, one could instead provinde the route as a param to the Success method in the routing ControlTower.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if that is better. It puts a new requirement on the caller of ControlTower.

p.subscribers[paymentHash], c,
)

return true, c, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, this is much better now that we don't have to return this from the DB.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It isn't new, I just restored code that I had previously. Do you mean it is better because the bool isn't set in the db tx?

Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM 💯not blocking, but we have been trying to remove all go-errors imports, and just use the std errors package. sometimes editors will mistakenly add that as the import automatically, since it is used in the integration tests to wrap errors.

routing/control_tower.go Outdated Show resolved Hide resolved
This commit lifts default setting up to the rpc server level, in line
with other payment defaults.
Add missing parameters to routerrpc version of SendPayment.
This commit creates an empty shall for control tower in the routing
package. It is a preparation for adding event notification.
Allows other sub-systems to subscribe to payment success and fail
events.
Modify the routerrpc SendPayment api to asynchronous. This allows
callers to pick up a payment after the rpc connection was lost or lnd
was restarted.
This commit prepares lncli for moving over to routerrpc payment calls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gRPC incomplete WIP PR, not fully finalized, but light review possible payments Related to invoices/payments routing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants