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

lnrpc: allow pubkey in SendToRoute #1944

Merged
merged 2 commits into from Nov 5, 2018

Conversation

Projects
None yet
4 participants
@joostjager
Collaborator

joostjager commented Sep 20, 2018

The information in the Route RPC message is not enough to directly send out the payment. First the local channel graph needs to be queried to complete the data. A lookup is executed to retrieve the public key of a node based on channel id. This does not work with private channels.

In this PR, we allow the hop pubkeys to be passed in so that execution of the payment becomes independent of the channel graph.

It is also a step towards the unbundling of lnd. (maybe, in the future, we get a node that doesn't track the channel graph at all anymore and is just a payment executor. mobile maybe?)

Fixes #2139.

@joostjager joostjager force-pushed the joostjager:sendroute-pubkey branch from af39bad to 73902e8 Oct 24, 2018

@joostjager joostjager changed the title from lnrpc: allow pubkey in SendToRoute [DO NOT REVIEW] to lnrpc: allow pubkey in SendToRoute Oct 24, 2018

@halseth halseth added this to the 0.5.2 milestone Oct 24, 2018

@Roasbeef Roasbeef requested a review from wpaulino Oct 26, 2018

@wpaulino

Cool change! We should add a godoc to each of the existing and new methods.

@joostjager joostjager force-pushed the joostjager:sendroute-pubkey branch from 73902e8 to 758638a Oct 29, 2018

@joostjager

This comment has been minimized.

Collaborator

joostjager commented Oct 29, 2018

godoc added

// If no pub key is given of the hop, the local channel
// graph needs to be queried to complete the information
// necessary for routing.
routeHop, err = unmarshallHopByChannelLookup(graph,

This comment has been minimized.

@halseth

halseth Oct 29, 2018

Collaborator

Regarding commit structure: it looks like this change (extracting logic into unmarshallHopByChannelLookup) could be done in an individual commit.

This comment has been minimized.

@joostjager

joostjager Oct 29, 2018

Collaborator

Commit added

"channel ID for hop (%d): %v", i, err)
var routeHop *routing.Hop
if hop.PubKey == "" {

This comment has been minimized.

@halseth

halseth Oct 29, 2018

Collaborator

I think I would prefer this check be done inside the unmarshal method. Shouldn't be a problem passing in the graph even though it might not be needed.

This comment has been minimized.

@joostjager

joostjager Oct 29, 2018

Collaborator

My goal was to have a version of unmarshall that does not have that dependency. I refactored it into a form that satisfies both our requirements.

@@ -3509,57 +3509,109 @@ func (r *rpcServer) marshallRoute(route *routing.Route) *lnrpc.Route {
Fee: int64(fee.ToSatoshis()),
FeeMsat: int64(fee),
Expiry: uint32(hop.OutgoingTimeLock),
// Include public key of hop in response, so that

This comment has been minimized.

@halseth

halseth Oct 29, 2018

Collaborator

I think this comment belongs by the definition of the PubKey field.

This comment has been minimized.

@joostjager

joostjager Oct 29, 2018

Collaborator

Moved

@joostjager joostjager force-pushed the joostjager:sendroute-pubkey branch from 758638a to 386738d Oct 29, 2018

@joostjager joostjager force-pushed the joostjager:sendroute-pubkey branch from 386738d to b2c8bfa Oct 29, 2018

@joostjager

This comment has been minimized.

Collaborator

joostjager commented Oct 29, 2018

Comments processed

@@ -3509,6 +3509,8 @@ func (r *rpcServer) marshallRoute(route *routing.Route) *lnrpc.Route {
Fee: int64(fee.ToSatoshis()),
FeeMsat: int64(fee),
Expiry: uint32(hop.OutgoingTimeLock),
PubKey: hex.EncodeToString(

This comment has been minimized.

@halseth

halseth Oct 29, 2018

Collaborator

hmm, strange wrapping?

This comment has been minimized.

@joostjager

joostjager Oct 29, 2018

Collaborator

I thought so too, but this is what gofmt does.

This comment has been minimized.

@halseth

halseth Oct 30, 2018

Collaborator

Maybe this works?

PubKey: hex.EncodeToString(
                hop.PubKeyBytes[:],
        ),
}

This comment has been minimized.

@joostjager

joostjager Oct 30, 2018

Collaborator

No

// unmarshallHop unmarshalls an rpc hop that may or may not contain a node
// pubkey.
func unmarshallHop(graph *channeldb.ChannelGraph, hop *lnrpc.Hop,
prevNodePubKey []byte) (*routing.Hop, error) {

This comment has been minimized.

@halseth

halseth Oct 30, 2018

Collaborator

prevNodePubKey should be made [33]byte

This comment has been minimized.

@joostjager

joostjager Oct 30, 2018

Collaborator

Why? This way there is less byte copying.

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

It makes the length clear, and makes sure it is non-nil. I don't think any extra byte copying makes a difference in this case.

This comment has been minimized.

@joostjager

joostjager Nov 1, 2018

Collaborator

Changed. Not sure it was worth it. It is a minor thing which to me is also a matter of preference.

lnrpc: accept pubkey in hop message
By passing a pubkey into SendToRoute, it becomes unnecessary for lnd to
query the channel graph to retrieve the hop pubkey. This allows routes
over private channels that are not present in the graph.

@joostjager joostjager force-pushed the joostjager:sendroute-pubkey branch from b2c8bfa to 28ae028 Nov 1, 2018

@halseth

halseth approved these changes Nov 1, 2018

LGTM 🏁

@Roasbeef

LGTM 🧶

@Roasbeef Roasbeef merged commit c0fadcc into lightningnetwork:master Nov 5, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.04%) to 55.869%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment