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

Send to route rpc #188 #747

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@t4sk
Contributor

t4sk commented Feb 9, 2018

  • This commit adds a RPC command to send a payment through a user defined route
  • Addresses #188
  • Related work: #219, @cfromknecht's branch swapz

At the moment SendToRoute DOES NOT WORK with the output of QueryRoutes for 2 reasons:

  • QueryRoutes returns hops with time lock delta of 9. But htlcswitch/link.go checks for a time lock delta of 144.
  • QueryRoutes returns a fee in satoshi. hltcswitch/link.go checks fee in millisatoshi. When I try to send a 2-hop payment of 1000 satoshi, a fee of 1001 millisatoshi is expected but the output from QueryRoute converts the fee to 1000 millisatoshi, which makes the fee insufficient.

Tests in lnd_test are written with the issues above in mind. But it would be ideal if the output of QueryRoutes can be used as an input for SendToRoute.

This is my first PR, feedback welcomed.

Thanks to @afederigo and @cfromknecht for the heavy lifting :)

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Feb 9, 2018

QueryRoutes returns hops with time lock delta of 9. But htlcswitch/link.go checks for a time lock delta of 144.

A timelock delta of 9 is the default value for the final hop assuming that they don't specify anything in the invoice. Our default policy is to use a value of 144, hence the check in link.go (which isn't hard coded but part of the current policy and can be changed on a per-channel basis). So seems like the solution here is to take a final_cltv_value along with queryroutes.

QueryRoutes returns a fee in satoshi. hltcswitch/link.go checks fee in millisatoshi. When I try to send a 2-hop payment of 1000 satoshi, a fee of 1001 millisatoshi is expected but the output from QueryRoute converts the fee to 1000 millisatoshi, which makes the fee insufficient.

Yep, so queryroutes should be modified to return everything in mSAT.

@t4sk

This comment has been minimized.

Contributor

t4sk commented Feb 12, 2018

@Roasbeef Thanks for your feedback! It's very informative 👍

But can you clarify what needs to be done here.

So seems like the solution here is to take a final_cltv_value along with queryroutes.

Should queryroutes be modified to take a final_cltv_delta ?

@t4sk t4sk force-pushed the t4sk:send-to-route-rpc-#188 branch from 9135d36 to fd1056a Feb 12, 2018

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Feb 15, 2018

Should queryroutes be modified to take a final_cltv_delta ?

Yeah, so it would be an optional argument. There's an "assumed" default of 9 blocks within the network. In scenarios where the use case is to pipe the result through sendtoroute, then the user would know what the final_cltv_delta value should be.

// succeeds, then a non-nil Route will be returned which describes the
// path the successful payment traversed within the network to reach the
// destination. Additionally, the payment preimage will also be returned.
func (r *ChannelRouter) SendToRoute(routes []*Route,

This comment has been minimized.

@halseth

halseth Feb 15, 2018

Collaborator

This adds a lot of code duplication. Instead should SendPayment be modified to use SendToRoute internally.

This comment has been minimized.

@t4sk

t4sk Feb 27, 2018

Contributor

@halseth thanks for reviewing! I've considered your suggestion but decided to create a private method sendPayment which is used by SendPayment and SendToRoute.

// invocation creates a persistent bi-directional stream allowing clients to
// rapidly send payments through the Lightning Network with a single
// persistent connection.
func (r *rpcServer) SendToRoute(sendToStream lnrpc.Lightning_SendToRouteServer) error {

This comment has been minimized.

@halseth

halseth Feb 15, 2018

Collaborator

here we should also try to reuse common code from SendPayment, an approach is mentioned in the comment here: https://github.com/lightningnetwork/lnd/pull/219/files#diff-01ada5680aaa18fc79cbf78b2d55a37bR1683

@t4sk t4sk force-pushed the t4sk:send-to-route-rpc-#188 branch from fd1056a to e410fa8 Feb 15, 2018

@Roasbeef Roasbeef referenced this pull request Feb 18, 2018

Closed

Send to route #219

@t4sk t4sk force-pushed the t4sk:send-to-route-rpc-#188 branch 6 times, most recently from ab7932d to 84cfecd Feb 26, 2018

@t4sk

This comment has been minimized.

Contributor

t4sk commented Mar 3, 2018

Feedback addressed. Please re-review :)

@halseth

The changes to rpcserver are a bit intrusive I'm afraid. Should be possible to simplify significantly?

// We'll continue until our payment succeeds, we encounter a critical error
// or all routes failed to send the payment.
i := -1
getRoute := func(paySession *paymentSession) (*Route, error) {

This comment has been minimized.

@halseth

halseth Apr 5, 2018

Collaborator

I like the approach of wrapping the routes slice in a paymentSession! I think we can make it even cleaner though, if we make paymentSession an interface, we can have two implementations of that interface: one backed by missionControl, and one backed by the slice of routes.

This comment has been minimized.

@t4sk

t4sk May 11, 2018

Contributor

Great idea! Done that. Please review.

}
// SendPayment dispatches a bi-directional streaming RPC for sending payments
// through the Lightning Network. A single RPC invocation creates a persistent
// bi-directional stream allowing clients to rapidly send payments through the
// Lightning Network with a single persistent connection.
func (r *rpcServer) SendPayment(paymentStream lnrpc.Lightning_SendPaymentServer) error {
func (r *rpcServer) SendPayment(stream lnrpc.Lightning_SendPaymentServer) error {
return r.sendPayment(&paymentStream{

This comment has been minimized.

@halseth

halseth Apr 5, 2018

Collaborator

It is very hard to reason about what's going on here unfortunately... Possible to minimize the changes, by not using a closure callback, and instead letting sendPayment take an optional set of routes?

This comment has been minimized.

@t4sk

t4sk May 11, 2018

Contributor

I've tried your suggestion but ran into one problem:

  • cannot pass a common interface for stream to sendPayment because lnrpc.Lightning_SendPaymentServer and lnrpc.Lightning_SendToRouteServer are also interfaces.
@@ -2813,16 +2813,16 @@ func (r *rpcServer) QueryRoutes(ctx context.Context,
func marshallRoute(route *routing.Route) *lnrpc.Route {
resp := &lnrpc.Route{
TotalTimeLock: route.TotalTimeLock,
TotalFees: int64(route.TotalFees.ToSatoshis()),
TotalAmt: int64(route.TotalAmount.ToSatoshis()),
TotalFees: int64(route.TotalFees),

This comment has been minimized.

@halseth

halseth Apr 5, 2018

Collaborator

The changes to QueryRoutes could be made its own PR, with its own set of added tests. Will make it easier to review! 😀

This comment has been minimized.

@t4sk

t4sk Apr 11, 2018

Contributor

@halseth Thanks! Currently working on a PR. Should lnrpc.Route be extended with additional fields TotalFeesMSat and TotalAmtMSat?

This comment has been minimized.

@halseth

halseth Apr 11, 2018

Collaborator

Hm, good question. Adding the fields wouldn't break usage of the protos, while changing the names would. So maybe just add them? :)

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Apr 19, 2018

#1081 has been merged now. This needs a rebase now.

@t4sk

This comment has been minimized.

Contributor

t4sk commented Apr 19, 2018

@Roasbeef Before rebasing this PR, should I create a separate PR for the commits which modifies queryroutes to take final_cltv_delta as an optional param?

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Apr 19, 2018

@t4sk nah I think the commit structure is fine as is.

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Apr 26, 2018

Friendly ping on rebase 🤗

@t4sk

This comment has been minimized.

Contributor

t4sk commented Apr 27, 2018

@Roasbeef Rebasing rpcserver.go has been giving me a headache. Do you mind if I delete some commits and rework them?

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Apr 27, 2018

@t4sk t4sk force-pushed the t4sk:send-to-route-rpc-#188 branch 4 times, most recently from 79c2d19 to 35af66c May 1, 2018

@t4sk t4sk force-pushed the t4sk:send-to-route-rpc-#188 branch from 35af66c to 4c637f4 May 8, 2018

@t4sk t4sk force-pushed the t4sk:send-to-route-rpc-#188 branch 2 times, most recently from bb1cf85 to d793669 May 10, 2018

t4sk added some commits Jan 25, 2018

cmd/lncli: add sendtoroute command
This commit adds sendtoroute command to lncli. RPC command SendToRoute
is not yet implemented.
tests: add integration tests for send to route
This commit add 3 integration tests for send-to-route RPC call.
One test ensures that single-hop payments are processed. Another test
checks that payments through a multi-hop route are processed. Lastly,
there is a test to check error propagation while sending payments via fake
predefined route.
rpcserver: add SendToRoute methods
rpcserver: add SendToRoute handler

@t4sk t4sk force-pushed the t4sk:send-to-route-rpc-#188 branch from d793669 to 8f6e63c May 10, 2018

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jun 7, 2018

Thanks so much for getting this started! I've added some minor style related commits on top, and committed it as e4e8b86137a43b21ac169f0f13de3a504ab87b30.

@Roasbeef Roasbeef closed this Jun 7, 2018

@Roasbeef Roasbeef referenced this pull request Jun 7, 2018

Closed

Add a SendToRoute RPC command #188

0 of 3 tasks complete

@Roasbeef Roasbeef added this to the 0.5 milestone Jun 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment