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

rpc+lnd: add new sub RPC server, Router service #2083

Merged
merged 3 commits into from Feb 21, 2019

Conversation

4 participants
@Roasbeef
Copy link
Member

Roasbeef commented Oct 23, 2018

In this PR, we implement the full RouterServer as specified by the
newly added sub-service as defined in router.proto. This new sub-server
has its own macaroon state (but overlapping permissions which can be
combined with the current admin.macaroon), and gives users a simplified
interface for a gRPC service that is able to simply send payment. Much
of the error reporting atm, is a place holder, and a follow up commit
will put up the infrastructure for a proper set of errors.

NOTE: The only new functionality in this PR are the last 3 commits, which show how one can add a new sub-server given the existing scaffolding.

Depends on #2081.

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Nov 2, 2018

Is this also the venue where we are going to stage #1662?

return nil
}

close(s.quit)

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Nov 2, 2018

Collaborator

similar comment here wrt removing quit, started, and stopped

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 11, 2019

Collaborator

bump

// Now that we know the full path of the router macaroon, we can check
// to see if we need to create it or not.
macFilePath := cfg.RouterMacPath
if !fileExists(macFilePath) && cfg.MacService != nil {

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Nov 2, 2018

Collaborator

would be great if this logic were abstracted in lnrpc, so we don't have to reimplement it with each new subserver. i imagine all subservers we add will have to use macaroons anyway

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Nov 2, 2018

@Roasbeef Roasbeef force-pushed the Roasbeef:ln-router-service branch from 1ff7655 to 91c7a42 Dec 1, 2018

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Dec 1, 2018

Pushed up a new rebased version, also added a new call to obtain a routing fee quote for a (dest, amt) pair.

network.
*/
int64 routing_fee_sat = 1;
}

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 1, 2018

Collaborator

should this return the time lock too? I'd like to have that too, so the user can consent to it (times a factor) and I can pass it as a limit to sendpayment (once the timelock limit pr is in)

so it isn't a lower bound of the fee, but a lower bound of the weight function including the tradeoff fee/timelock.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 5, 2018

Author Member

Done!

@joostjager
Copy link
Collaborator

joostjager left a comment

gives users a simplified interface for a gRPC service that is able to simply send payment

General question: where is this requirement coming from?

Show resolved Hide resolved lnrpc/routerpc/config_active.go Outdated
Show resolved Hide resolved lnrpc/routerpc/log.go Outdated
Show resolved Hide resolved lnrpc/routerpc/router.proto Outdated
@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Dec 5, 2018

General question: where is this requirement coming from?

Part of the sub-server architecture that enables things like managing a cluster of lnd nodes with one primarily sending payments (due to better graph placement and liquidity) and for example a set of other nodes (even may not be fully fledged lnd nodes) which handle on-chain actions. By abstracting behavior at the service level, we gain a greater degree of flexibility and are able to implement things like rolling restarts and also ensure we have service level redundancy.

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Dec 5, 2018

By abstracting behavior at the service level, we gain a greater degree of flexibility and are able to implement things like rolling restarts and also ensure we have service level redundancy.

Yes, this part I understand. My question was about why it needs to be a 'simplified' interface? Or do you mean with simplified the isolation of several calls in the sub server rather than simplify the calls themselves?

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Dec 18, 2018

Simplified w.r.t isolation! A side effect of the sub-server system it that it forces us to not just pile new things (like extensive business logic outside of marshaling) into the rpcserver.go file.

@Roasbeef Roasbeef added this to In progress in High Priority via automation Dec 18, 2018

@Roasbeef Roasbeef moved this from In progress to Final Testing -- Ready For Merge in High Priority Dec 18, 2018

@Roasbeef Roasbeef force-pushed the Roasbeef:ln-router-service branch 2 times, most recently from b178b0b to d114c46 Jan 3, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Jan 3, 2019

Rebased on top of the latest master.

Show resolved Hide resolved lnrpc/routerpc/router.proto Outdated
Show resolved Hide resolved lnrpc/routerpc/driver.go Outdated
Show resolved Hide resolved lnrpc/routerpc/router_server.go Outdated
Show resolved Hide resolved lnrpc/routerpc/router_server.go Outdated
Show resolved Hide resolved subrpcserver_config.go Outdated

@Roasbeef Roasbeef force-pushed the Roasbeef:ln-router-service branch from d114c46 to 77d8d56 Jan 4, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Jan 4, 2019

Comments addressed, PTAL @halseth

Show resolved Hide resolved lnrpc/routerpc/router.proto Outdated
Show resolved Hide resolved lnrpc/routerpc/config_active.go Outdated
Show resolved Hide resolved lnrpc/routerpc/router_server.go Outdated
Show resolved Hide resolved lnrpc/routerpc/router_server.go Outdated
Show resolved Hide resolved lnrpc/routerpc/router.proto Outdated

@Roasbeef Roasbeef force-pushed the Roasbeef:ln-router-service branch 2 times, most recently from 80a3db9 to 7a0a900 Jan 5, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Jan 5, 2019

Latest instance returns fee quotes in msat.

@halseth
Copy link
Collaborator

halseth left a comment

LGTM 🚋

Show resolved Hide resolved lnrpc/routerpc/router.proto Outdated

High Priority automation moved this from Final Testing -- Ready For Merge to Needs review Jan 11, 2019

Show resolved Hide resolved lnrpc/routerpc/router.proto Outdated
return nil
}

close(s.quit)

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 11, 2019

Collaborator

bump

Show resolved Hide resolved subrpcserver_config.go Outdated

@Roasbeef Roasbeef added this to the 0.6 milestone Jan 16, 2019

Show resolved Hide resolved lnrpc/routerpc/config_default.go Outdated
Show resolved Hide resolved lnrpc/routerpc/router_server.go Outdated
Show resolved Hide resolved lnrpc/routerpc/router_server.go Outdated
Show resolved Hide resolved lnrpc/routerpc/router_server.go Outdated

@Roasbeef Roasbeef force-pushed the Roasbeef:ln-router-service branch from 7a0a900 to 41aec88 Jan 23, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Jan 23, 2019

Rebased with latest comments addressed, PTAL!

@halseth
Copy link
Collaborator

halseth left a comment

One re-naming nit, otherwise LGTM

Show resolved Hide resolved lnrpc/routerpc/log.go Outdated

@Roasbeef Roasbeef force-pushed the Roasbeef:ln-router-service branch from 41aec88 to bec5af9 Jan 29, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Jan 29, 2019

I think it is better to take out EstimateFee completely. We currently have no way to calculate a realistic fee estimate. Just returning the minimum fee as we do now is of limited practical use. The rpc promises functionality that we currently cannot deliver. Besides, it is hardly more than a wrapper around QueryRoutes.

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Feb 11, 2019

Needs rebase.

Roasbeef added some commits Oct 23, 2018

lnrpc/routerrpc: add config, implement full RouterServer
In this commit, we implement the full RouterServer as specified by the
newly added sub-service as defined in router.proto. This new sub-server
has its own macaroon state (but overlapping permissions which can be
combined with the current admin.macaroon), and gives users a simplified
interface for a gRPC service that is able to simply send payment. Much
of the error reporting atm, is a place holder, and a follow up commit
will put up the infrastructure for a proper set of errors.

@Roasbeef Roasbeef force-pushed the Roasbeef:ln-router-service branch from bec5af9 to 88252d7 Feb 21, 2019

@Roasbeef Roasbeef merged commit e1382bd into lightningnetwork:master Feb 21, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.05%) to 60.074%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

High Priority automation moved this from Needs review to Done Feb 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.