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+lnd+rpc: add new Signer RPC service, and sub RPC server infrastructure #2081

Merged
merged 14 commits into from Nov 29, 2018

Conversation

Projects
None yet
4 participants
@Roasbeef
Copy link
Member

Roasbeef commented Oct 23, 2018

In this commit, we add a new sub RPC server, the SignerService, along with infrastructure to allow developers to easily create these new sub-servers in the future. With a goal of making thighs fully generalized, and new RPC service (after this RPC), only needs to do the following:

  1. Add a new package, and within the init() method call `lnrpc.RegisterSubServer``
  2. Add a new case statement within the new sub-server config parsing to properly set the arguments of the config of the sub-server.

In order to achieve the above, we utilize the reflect package and build flags heavily throughout the scaffolding.

New signrpc Package

In this PR, we introduce a new sub-package within the greater RPC package. This new sub-package will house a new set of sub-RPC servers to expose experimental features behind build flags for upstream consumers. In this commit, we add the first config for the service, which will simply expose the lnwallet.Signer interface over RPC.

In the default file, we have what the config will be if the build tag (signerrpc) is off. In this case, the config parser won't detect any times, and if specified will error out. In the active file, we have the true config that the server will use. With this new set up, we'll exploit these build flags heavily in order to create a generalized framework for adding additional sub RPC servers.

The package also defines a new tool for generating protos, and also the new protos for the SIgner service itself.

Sub RPC Server Registration System

In this PR, we add the scaffolding for the future sub-server RPC system. The idea is that each sub server will implement this particular interface. From there on, a "root" RPC server is able to query this registry, and dynamically create each sub-sever instance without knowing the details of each sub-server.

In the init() method of the package of a sub-server, the sub-server is to call: RegisterSubServer to claim its namespace. Afterwards, the root RPC server can use the RegisteredSubServers() method to obtain a slice of ALL registered sub-servers. Once this list is obtained, it can use the New() method of the SubServerDriver struct to create a new sub-server instance.

Each sub-server needs to be able to locate it's primary config using the SubServerConfigDispatcher interface. This can be a map of maps, or a regular config struct. The main requirement is that the sub-server be able to find a config under the same name that it registered with. This string of abstractions will allow the main RPC server to find, create, and run each sub-server without knowing the details of its configuration or its role.

New SignerServer gRPC Service Implementation

In this PR, we add a full implementation of the new SignerServer sub RPC service within the main root RPC service. This service is able to fully manage its macaroons, and service any connected clients. Atm, this service only has a single method: SignOutputRaw which mimics the existing lnwallet.Signer interface within lnd itself. As the API's are so similar, it will be possible for a client to directly use the lnwallet.Signer interface, and have a proxy that sends the request over RPC, and translates the proto layer on both sides. To the client, it doesn't know that it's using a remote, or local RPC.

The server uses the new driver registration system to register itself as a relevant sub RPC server at the lnrpc package level. We also do a bit of validation on the config passed in, in order to ensure it has all the items we need to function.

Dynamic Sub Server Config Parsing

In this commit, we add the glue infrastructure to make the sub RPC server system work properly. Our high level goal is the following: using only the lnrpc package (with no visibility into the sub RPC servers), the RPC server is able to find, create, run, and manage the entire set of present and future sub RPC servers. In order to achieve this, we use the reflect package and build tags heavily to permit a loosely coupled configuration parsing system for the sub RPC servers.

We start with a new subRpcServerConfigs struct which is always present. This struct has its own group, and will house a series of sub-configs, one for each sub RPC server. Each sub-config is actually gated behind a build flag, and can be used to allow users on the command line or in the config to specify arguments related to the sub-server. If the config isn't present, then we don't attempt to parse it at all, if it is, then that means the RPC server has been registered, and we should parse the contents of its config.

The subRpcServerConfigs struct has two main methods: PopulateDependancies and FetchConfig. The PopulateDependancies is used to dynamically locate and set the config fields for each new sub-server. As the config may not actually have any fields (if the build flag is off), we use the reflect pacakge to determine if things are compiled in or not, then if so, we dynamically set each of the config parameters. The PopulateDependancies method implements the lnrpc.SubServerConfigDispatcher interface. Our goal is to allow sub servers to look up their actual config in this main config struct. We achieve this by using reflect to look up the target field as if it were a key in a map. If the field is found, then we check if it has any actual attributes (it won't if the build flag is off), if it is, then we return it as we expect it to be populated already.

The Current RPC Server as the Root gRPC Server

In this PR, we modify the existing rpcServer to fully manage the macaroons, gRPC server, and also seek out and create all sub-servers. With this change, the RPC server gains more responsibility, as it becomes the "root" server in the hierarchy of gRPC sub-servers.

In addition to creating each sub-server, it will also merge the set of macaroon permissions for each sub-server, with the permissions of the rest of the RPC infra. As a result, each sub-server is able to
independently specify what it needs w.r.t macaroon permissions and have that taken care of by the RPC server. In order to achieve this, we need to unify the creation of the RPC interceptors, and also fully manage the gRPC server ourselves.

As a bonus, on shutdown, the RPC server will now call rpcServer.GracefulStop(), which will stop it from accepting new connections, and wait for existing RPC's to execute before shutting down the entire server.

Some examples with various build configs:

⛰i  make build
 Building debug lnd and lncli.
go build -v -tags="dev" -o lnd-debug -ldflags "-X github.com/lightningnetwork/lnd/build.Commit=v0.5-beta-143-gb2069914c4b76109b7c59320dc48f8a5f30deb75-dirty" github.com/lightningnetwork/lnd
go build -v -tags="dev" -o lncli-debug -ldflags "-X github.com/lightningnetwork/lnd/build.Commit=v0.5-beta-143-gb2069914c4b76109b7c59320dc48f8a5f30deb75-dirty" github.com/lightningnetwork/lnd/cmd/lncli

⛰i  ./lnd-debug --debuglevel=debug --signrpc.signermacaroonpath=~/sign.macaroon
unknown flag `signrpc.signermacaroonpath'
unknown flag `signrpc.signermacaroonpath'

⛰i  make build tags=signerrpc
 Building debug lnd and lncli.
go build -v -tags="dev signerrpc" -o lnd-debug -ldflags "-X github.com/lightningnetwork/lnd/build.Commit=v0.5-beta-143-gb2069914c4b76109b7c59320dc48f8a5f30deb75-dirty" github.com/lightningnetwork/lnd
go build -v -tags="dev signerrpc" -o lncli-debug -ldflags "-X github.com/lightningnetwork/lnd/build.Commit=v0.5-beta-143-gb2069914c4b76109b7c59320dc48f8a5f30deb75-dirty" github.com/lightningnetwork/lnd/cmd/lncli

⛰i  ./lnd-debug --debuglevel=debug --signrpc.signermacaroonpath=~/sign.macaroon
2018-10-22 17:31:01.132 [INF] LTND: Version: 0.5.0-beta commit=v0.5-beta-143-gb2069914c4b76109b7c59320dc48f8a5f30deb75-dirty, build=development, logging=default
2018-10-22 17:31:01.133 [INF] LTND: Active chain: Bitcoin (network=simnet)
2018-10-22 17:31:01.140 [INF] CHDB: Checking for schema update: latest_version=6, db_version=6
2018-10-22 17:31:01.236 [INF] LTND: Primary chain is set to: bitcoin
2018-10-22 17:31:02.391 [INF] LNWL: Opened wallet
2018-10-22 17:31:03.315 [INF] LNWL: The wallet has been unlocked without a time limit
2018-10-22 17:31:03.315 [INF] LTND: LightningWallet opened
2018-10-22 17:31:03.319 [INF] LNWL: Catching up block hashes to height 3060, this will take a while...
2018-10-22 17:31:03.320 [INF] HSWC: Restoring in-memory circuit state from disk
2018-10-22 17:31:03.320 [INF] LNWL: Done catching up block hashes
2018-10-22 17:31:03.320 [INF] HSWC: Payment circuits loaded: num_pending=0, num_open=0
2018-10-22 17:31:03.322 [DBG] LTND: Populating dependencies for sub RPC server: Signrpc

As for the config, an example is:

[signrpc]
signrpc.signermacaroonpath=~/signer.macaroon
@@ -0,0 +1,33 @@
// +build signerrpc

This comment has been minimized.

@cfromknecht

cfromknecht Oct 23, 2018

Collaborator

make build flag = signrpc so it matches the package name?

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Fixed!

// DisableLog disables all library log output. Logging output is disabled
// by default until UseLogger is called.
func DisableLog() {
log = btclog.Disabled

This comment has been minimized.

@cfromknecht

cfromknecht Oct 23, 2018

Collaborator

log file needs to be updated post-build package, should use UseLogger(build.NewSubLogger("SGNR", nil)) and DisableLog should call UseLogger like https://github.com/lightningnetwork/lnd/blob/master/lnwallet/log.go

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Fixed!

// proxy will use to connect to the main gRPC server.
restServerOpts []grpc.DialOption

// tlsCfg is the TLS configu that allows the REST server proxy to

This comment has been minimized.

@cfromknecht

cfromknecht Oct 23, 2018

Collaborator

s/configu/config

This comment has been minimized.

@Roasbeef
@@ -0,0 +1,128 @@
package main

This comment has been minimized.

@cfromknecht

cfromknecht Oct 23, 2018

Collaborator

Seems like something that could be moved to lnrpc package?

This comment has been minimized.

@Roasbeef

Roasbeef Oct 23, 2018

Member

At a future point possibly? Things aren't yet decoupled enough to make the move.

This comment has been minimized.

@cfromknecht

cfromknecht Oct 23, 2018

Collaborator

the imports in this file are minimal and usage seems to be done exclusively through public methods, which part isn't decoupled?

This comment has been minimized.

@cfromknecht

cfromknecht Oct 23, 2018

Collaborator

chain control?

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

As in, there're gonna be future sub-servers added in the next week or so. At that point, we'll have a good idea w.r.t all their dependancies and can make one global move to integrate them all.

I lean against putting it in the lnrpc package though, as this also contains items which will be parsed from either the command line, or the the config files.

case *signrpc.Config:
subCfgValue := extractReflectValue(cfg)

subCfgValue.FieldByName("MacService").Set(

This comment has been minimized.

@cfromknecht

cfromknecht Oct 23, 2018

Collaborator

It doesn't seem like reflect is necessary here. Is it not possible to set the fields directly? Reflect seems useful in determining if the struct has fields, but we already know they exist at this point. Is this equivalent to

			cfg.MacService = macService
			cfg.NetworkDir = networkDir
			cfg.Signer = cc.signer

This comment has been minimized.

@Roasbeef

Roasbeef Oct 23, 2018

Member

Nope, they won't always exist if the build tag is off. This always compiles regardless of if the build tags are on or not.

This comment has been minimized.

@cfromknecht

cfromknecht Oct 23, 2018

Collaborator

Gotcha, makes sense

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

A way to make this possible, is to move the actual population to the build tag protected config files, see b9c74ff.

This lets us remove this file entirely, and the use of reflection with it.

@halseth
Copy link
Collaborator

halseth left a comment

Stoked to get this in! make microservices hot again!


package signrpc

// Config is the primary configuration struct for the signer RPC server. It

This comment has been minimized.

@halseth

halseth Oct 26, 2018

Collaborator

Make comment be `// Config is empty for non-signrpc builds." or similar.?

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Fixed!


message KeyDescriptor {
/**
The raw bytes of the key being identified. Either this or the KeyLocator

This comment has been minimized.

@halseth

halseth Oct 26, 2018

Collaborator

make oneof?

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Both can be specified FWIW. oneofs are a bit awkward within Go IMO.

This comment has been minimized.

@joostjager

joostjager Oct 31, 2018

Collaborator

This is also happening in for example in rpc.proto in the ChannelPoint message. Do we really need to offer this service to accept data in two formats? To me it bloats the interface while the client itself could also do the conversion to the single accepted format.

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

Agree that its better to either accept only one format, or use a oneof to make the API clearer.

@@ -0,0 +1,6 @@
#!/bin/sh

protoc -I/usr/local/include -I. \

This comment has been minimized.

@halseth

halseth Oct 26, 2018

Collaborator

as mentioned, would be nice to modify the top level script to just recursively compile all protos, as we wouldn't need to add a script for each sub-service, and changes to any of the sub-services would be updated by running the top-level script.

This comment has been minimized.

@cfromknecht

cfromknecht Oct 29, 2018

Collaborator

on a similar line of thought, should this be added to the make rpc target?

This comment has been minimized.

@halseth

halseth Oct 30, 2018

Collaborator

Example on the recursive version: halseth@f3f8129

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Made recursive version (leaves out REST etc for now)

package lnrpc

import (
fmt "fmt"

This comment has been minimized.

@halseth

halseth Oct 26, 2018

Collaborator

doesn't seem necessary to name this import

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Fixed, dunno what added that...

"gopkg.in/macaroon-bakery.v2/bakery"
)

// MacaroonPerms is a map from the FullMethod of an invoked gRPC command. to

This comment has been minimized.

@halseth

halseth Oct 26, 2018

Collaborator

extra period

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Fixed! Sentence somehow was chopped off, filled that in properly

// server given the main config dispatcher method. If we're unable to find the
// config that is meant for us in the config dispatcher, then we'll exit with
// an error.
func createNewSubServer(configRegistry lnrpc.SubServerConfigDispatcher) (lnrpc.SubServer, lnrpc.MacaroonPerms, error) {

This comment has been minimized.

@halseth

halseth Oct 26, 2018

Collaborator

nit: long line

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

lol yeah, I don't really see a clean way of breaking it up though

One route would be to rename SubServerConfigDispatcher perhaps?

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

What about

func createNewSubServer(configRegistry lnrpc.SubServerConfigDispatcher) (
            lnrpc.SubServer, lnrpc.MacaroonPerms, error) {

?

This comment has been minimized.

@Roasbeef

Roasbeef Nov 2, 2018

Member

Adopted

}

// Before we try to make the new signer service instance, we'll perform
// some sanity checks on the arguments to ensure taht they're useable.

This comment has been minimized.

@halseth

halseth Oct 26, 2018

Collaborator

typo: taht

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Fixed!

Signrpc *signrpc.Config `group:"signrpc" namespace:"signrpc"`
}

// PopulateDependancies attempts to iterate through all the sub-server configs

This comment has been minimized.

@halseth

halseth Oct 26, 2018

Collaborator

typo: PopulateDependancies

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Fixed!

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

s/PopulateDependancies/PopulateDependencies

This comment has been minimized.

@Roasbeef

Roasbeef Nov 2, 2018

Member

actually, actually fixed this time :)

err := subServer.RegisterWithRootServer(grpcServer)
if err != nil {
return nil, fmt.Errorf("unable to register "+
"sub-server %v with root: %v", err)

This comment has been minimized.

@halseth

halseth Oct 26, 2018

Collaborator

missing first param

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Fixed!

// Before we create any of the sub-servers, we need to ensure that all
// the dependencies they need are properly populated within each sub
// server configuration struct.
err := subServerCgs.PopulateDependancies(

This comment has been minimized.

@halseth

halseth Oct 26, 2018

Collaborator

Why is this level of indirection needed? Seems like we could just pass these parameters to subserver.New() or have a common subserverConfig passed to New() instead of using reflection.

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Atm not all of the parameters are exported as a public package, and we don't yet know all the dependancies that the remaining sub servers will require.

In a prior version of this, that didn't have the explicit server registration, this was required as the gRPC server couldn't be created until we had all the macaroon information from each of the sub-servers from New(). This has changed though, so I guess the question is then how does the rpcserver know which config to pass in? As is now, it doesn't even know how many sub-servers there are, or which configs they need.

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

This will still have the problem of needing to pass in more parameters in the future, if new subservers require them in their configs.

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

One possibility is to use a map from subserver name -> subserverconfig, see b9c74ff

This comment has been minimized.

@Roasbeef

Roasbeef Nov 2, 2018

Member

That ignores that the sub-servers also may have command line arguments that we may need to parse out which are in the config.

This comment has been minimized.

@halseth
// into the main RPC server that lnd exposes. Special methods are present on
// this struct to allow the main RPC server to create and manipulate the
// sub-RPC servers in a generalized manner.
type subRpcServerConfigs struct {

This comment has been minimized.

@joostjager

joostjager Oct 29, 2018

Collaborator

I think using a separate config file per sub server in the .lnd directory is simpler. No reflection needed and less references from lnd to the sub services. Are there serious downsides? I can even imagine a separate config to be more in line with the plan to unbundle lnd into separate processes.

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Config file per sub-server adds more overhead for the end users. Rather than maintain a single file, they need N files. In a world where the sub-server is packaged up as its own binary (standalone signer on some remote box), I think this make sense.

The main thing the reflection allows us to do, is reference items which may be conditionally compiled in. Even if we had a single config file per sub-server, the inner config file may not have any fields in the default build flag, as a result, nothing would compile.

switch {
case config.MacService == nil:
return nil, nil, fmt.Errorf("MacService must be set to create " +
"Signrpc")

This comment has been minimized.

@joostjager

joostjager Oct 29, 2018

Collaborator

This doesn't allow running with NoMacaroons

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Which is ok I think?

This comment has been minimized.

@joostjager

joostjager Oct 31, 2018

Collaborator

I don't think so. The idea of NoMacaroons is that you can use lnd without macaroons? It probably comes back to why we have the NoMacaroons flag in the first place. I use it during dev to make it easier to switch between alice, bob and carol on regtest and testnet.

This comment has been minimized.

@Roasbeef

Roasbeef Nov 2, 2018

Member

Latest version is now usable with no macaroons!

// to generate more granular macaroons.
// genMacaroons generates three macaroon files; one admin-level, one for
// invoice access and one read-only. These can also be used to generate more
// granular macaroons.
func genMacaroons(ctx context.Context, svc *macaroons.Service,

This comment has been minimized.

@joostjager

joostjager Oct 29, 2018

Collaborator

How to generate an admin macaroon that can access all sub services? In signer, a new entity and action is introduced. I think admin should be able to have permissions to access signer?

Hack: joostjager@61b6453

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Yeah the only one that that makes a new namespace is the signer. We can include this in the admin macaroon, but then that would invalidate all existing created admin macaroons. We can do this eventually IMO, but as this is behind an experimental build flag, I'd prefer now to.

This comment has been minimized.

@joostjager

joostjager Oct 31, 2018

Collaborator

So the solution then now is to use two macaroons from the client (admin and signer)? I think this also means that two tcp connections need to be created?

This comment has been minimized.

@Roasbeef

Roasbeef Nov 2, 2018

Member

It should be possible to do it with a single TCP connection. I'd need to double check the docs, but it should be possible to switch to a per-call credential model, rather than a connection wide credential mode as we use widely atm.

This comment has been minimized.

@joostjager

joostjager Nov 3, 2018

Collaborator

Is this something that can be controlled client side, or is a change on the server needed?

This comment has been minimized.

@joostjager

joostjager Nov 28, 2018

Collaborator

I did a quick try passing multiple macaroon credentials to the dial call, but that is not going to work. PerRPCCredentials, didn't try that, but it also means we need to pass around those macaroons. Is it worth the effort at this point? Otherwise I'd say, just make signing possible with the admin macaroon for now.

This comment has been minimized.

@Roasbeef

Roasbeef Nov 29, 2018

Member

Why do you need to pass multiple macaroon credentials? You should only need to pass the signer.macaroon if you're interacting with the signer sub-server. The systems that interact w/ the sub-servers will need to get used to managing their various macaroons, as we can easily implemented domain isolation using them. For example, merchants are already used to using the invoice.macaroon everywhere.

It's possible to create custom macarons that can access only what's needed (so signer and new address for example), as there's an open PR for this, but it isn't yet merged.

This comment has been minimized.

@joostjager

joostjager Nov 29, 2018

Collaborator

So I need to open multiple tcp connections then? Swaplet needs signer, walletkit and the main rpc.

This comment has been minimized.

@Roasbeef

Roasbeef Dec 1, 2018

Member

Possibly, will look into if we can modify the way we manually set the credentials and get back to you!

type subRpcServerConfigs struct {
// Signrpc is a sub-RPC server that exposes signing of arbitrary inputs
// as a gRPC service.
Signrpc *signrpc.Config `group:"signrpc" namespace:"signrpc"`

This comment has been minimized.

@cfromknecht

cfromknecht Oct 29, 2018

Collaborator

nit: name should be SignRpc or SignRPC

This comment has been minimized.

@Roasbeef

Roasbeef Oct 31, 2018

Member

Went with the latter.

@@ -0,0 +1,6 @@
#!/bin/sh

protoc -I/usr/local/include -I. \

This comment has been minimized.

@cfromknecht

cfromknecht Oct 29, 2018

Collaborator

on a similar line of thought, should this be added to the make rpc target?

Show resolved Hide resolved lnrpc/signrpc/log.go
func init() {
subServer := &lnrpc.SubServerDriver{
SubServerName: subServerName,
New: func(c lnrpc.SubServerConfigDispatcher) (lnrpc.SubServer, lnrpc.MacaroonPerms, error) {

This comment has been minimized.

@cfromknecht

cfromknecht Oct 29, 2018

Collaborator

nit: long line

This comment has been minimized.

@Roasbeef

Roasbeef Nov 2, 2018

Member

Fixed

// server given the main config dispatcher method. If we're unable to find the
// config that is meant for us in the config dispatcher, then we'll exit with
// an error.
func createNewSubServer(configRegistry lnrpc.SubServerConfigDispatcher) (lnrpc.SubServer, lnrpc.MacaroonPerms, error) {

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

What about

func createNewSubServer(configRegistry lnrpc.SubServerConfigDispatcher) (
            lnrpc.SubServer, lnrpc.MacaroonPerms, error) {

?

const (
// subServerName is the name of the sub rpc server. We'll use this name
// to register ourselves, and we also require that the main
// SubServerConfigDispatcher instance recognize as the name of our

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

unfinished sentence,

This comment has been minimized.

@Roasbeef

Roasbeef Nov 2, 2018

Member

Fixed


message KeyDescriptor {
/**
The raw bytes of the key being identified. Either this or the KeyLocator

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

Agree that its better to either accept only one format, or use a oneof to make the API clearer.

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

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

I initially thought that all services would require the same macaroon creation, but if they might require multiple in the future probably better to leave this as is for now. Can extract into common logic when entering the multiservice era.

Signrpc *signrpc.Config `group:"signrpc" namespace:"signrpc"`
}

// PopulateDependancies attempts to iterate through all the sub-server configs

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

s/PopulateDependancies/PopulateDependencies

// Before we create any of the sub-servers, we need to ensure that all
// the dependencies they need are properly populated within each sub
// server configuration struct.
err := subServerCgs.PopulateDependancies(

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

This will still have the problem of needing to pass in more parameters in the future, if new subservers require them in their configs.

// Before we create any of the sub-servers, we need to ensure that all
// the dependencies they need are properly populated within each sub
// server configuration struct.
err := subServerCgs.PopulateDependancies(

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

One possibility is to use a map from subserver name -> subserverconfig, see b9c74ff

rpcsLog.Infof("Stopping %v Sub-RPC Server",
subServer.Name())

if err := subServer.Stop(); err != nil {

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

print error?

This comment has been minimized.

@Roasbeef

Roasbeef Nov 2, 2018

Member

Now printed

case *signrpc.Config:
subCfgValue := extractReflectValue(cfg)

subCfgValue.FieldByName("MacService").Set(

This comment has been minimized.

@halseth

halseth Oct 31, 2018

Collaborator

A way to make this possible, is to move the actual population to the build tag protected config files, see b9c74ff.

This lets us remove this file entirely, and the use of reflection with it.

@Roasbeef Roasbeef force-pushed the Roasbeef:signer-service branch from fc10625 to 023fb23 Nov 2, 2018

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Nov 2, 2018

Pushed out a new version with the following major changes:

  • now passes linter so should be buildable
  • recursive proto generation script
  • proper line wrapping as necessary
return nil
}

close(s.quit)

This comment has been minimized.

@cfromknecht

cfromknecht Nov 2, 2018

Collaborator

we don't use quit anywhere besides here atm. should be fine if both Start and Stop return nil? also means we can remove started and stopped

This comment has been minimized.

@Roasbeef

Roasbeef Nov 2, 2018

Member

Good call! Removed and simplified.

@Roasbeef Roasbeef force-pushed the Roasbeef:signer-service branch 2 times, most recently from 6ca5209 to 4d6e6e7 Nov 2, 2018

@halseth
Copy link
Collaborator

halseth left a comment

Commit police 👮 🙅‍♀️

@@ -1,4 +1,4 @@
// +build signerrpc
// +build signrpc

This comment has been minimized.

@halseth

halseth Nov 5, 2018

Collaborator

This commit should be squashed with the first.

@@ -34,10 +34,13 @@ func createNewSubServer(configRegistry lnrpc.SubServerConfigDispatcher) (
&Config{}, signServerConf)
}

// Before we try to make the new signer service instance, we'll perform

This comment has been minimized.

@halseth

halseth Nov 5, 2018

Collaborator

this should be squashed with the commit where driver was introduced.

@@ -1,4 +1,4 @@
// +build signerrpc
// +build signrpc

This comment has been minimized.

@halseth

halseth Nov 5, 2018

Collaborator

should be fixup commit also

// Before we create any of the sub-servers, we need to ensure that all
// the dependencies they need are properly populated within each sub
// server configuration struct.
err := subServerCgs.PopulateDependancies(

This comment has been minimized.

@halseth
@@ -0,0 +1,6 @@
#!/bin/sh

This comment has been minimized.

@halseth

halseth Nov 5, 2018

Collaborator

Commit can be deleted.

@@ -221,7 +221,7 @@ type config struct {

Tor *torConfig `group:"Tor" namespace:"tor"`

SubRpcServers *subRpcServerConfigs `group:"subrpc"`
SubRPCServers *subRPCServerConfigs `group:"subrpc"`

This comment has been minimized.

@halseth

halseth Nov 5, 2018

Collaborator

squash changes with relevant commits.

The raw bytes of the key being identified. Either this or the KeyLocator
must be specified.
*/
bytes raw_key_bytes = 1;

This comment has been minimized.

@joostjager

joostjager Nov 6, 2018

Collaborator

What is the use case for passing in raw_key_bytes?

This comment has been minimized.

@Roasbeef

Roasbeef Nov 29, 2018

Member

When you know the pubkey, but not the key locator information.

@cfromknecht
Copy link
Collaborator

cfromknecht left a comment

looking good! just need to be squashed according to @halseth's feedback

@@ -572,6 +572,8 @@ func (r *rpcServer) Stop() error {
subServer.Name())

if err := subServer.Stop(); err != nil {
rpcsLog.Errorf("unable to stop sub-server %v: %v",

This comment has been minimized.

@cfromknecht

cfromknecht Nov 14, 2018

Collaborator

nit: capitalise log message

@halseth halseth referenced this pull request Nov 23, 2018

Open

[WIP] Bos score enabled Autopilot #2212

4 of 6 tasks complete

Roasbeef added some commits Oct 23, 2018

lnrpc/signrpc: create new signrpc package with build-flag guarded config
In this commit, we introduce a new sub-package within the greater RPC
package.  This new sub-package will house a new set of sub-RPC servers
to expose experimental features behind build flags for upstream
consumers. In this commit, we add the first config for the service,
which will simply expose the lnwallet.Signer interface over RPC.

In the default file, we have what the config will be if the build tag
(signerrpc) is off. In this case, the config parser won't detect any
times, and if specified will error out. In the active file, we have the
true config that the server will use. With this new set up, we'll
exploit these build flags heavily in order to create a generalized
framework for adding additional sub RPC servers.
lnrpc/signrpc: add new gen_protos.sh to generate minimal protos
In this commit, we add a new proto generation script to match the one in
the main lnrpc package. This script differs, as we don't need to
generate the REST proxy stuff (for now).
lnrpc: add new sub RPC server registration system
In this commit, we add the scafolding for the future sub-server RPC
system. The idea is that each sub server will implement this particular
interface. From there on, a "root" RPC server is able to query this
registry, and dynamically create each  sub-sever instance without
knowing the details of each sub-server.

In the init() method of the pacakge of a sub-server, the sub-server is
to call: RegisterSubServer to claim its namespace. Afterwards, the root
RPC server can use the RegisteredSubServers() method to obtain a slice
of ALL regsitered sub-servers. Once this list is obtained, it can use
the New() method of the SubServerDriver struct to create a new
sub-server instance.

Each sub-server needs to be able to locate it's primary config using the
SubServerConfigDispatcher interface. This can be a map of maps, or a
regular config structr. The main requirement is that the sub-server be
able to find a config under the same name that it registered with. This
string of abstractions will allow the main RPC server to find, create,
and run each sub-server without knowing the details of its configuration
or its role.

Roasbeef added some commits Oct 23, 2018

lnrpc/signrpc: implement new SignerServer sub RPC server
In this commit, we add a full implementation of the new SignerServer sub
RPC service within the main root RPC service. This service is able to
fully manage its macaroons, and service any connected clients. Atm, this
service only has a single method: SignOutputRaw which mimics the
existing lnwallet.Signer interface within lnd itself. As the API's are
so similar, it will be possible for a client to directly use the
lnwallet.Signer interface, and have a proxy that sends the request over
RPC, and translates the proto layer on both sides. To the client, it
doesn't know that it's using a remote, or local RPC.
lnrpc/signrpc: add lnrpc.SubServerDriver for signrpc
In this commit, we create a lnrpc.SubServerDriver for signrpc. Note that
this file will only have its init() method executed if the proper build
flag is on. As a result, only if the build flag is set, will the RPC
server be registered, and visible at the packge lnrpc level for the root
server to manipulate.
config+rpc: create new generalized dynamic sub rpc server config fram…
…ework

In this commit, we add the glue infrastructure to make the sub RPC
server system work properly. Our high level goal is the following: using
only the lnrpc package (with no visibility into the sub RPC servers),
the RPC server is able to find, create, run, and manage the entire set
of present and future sub RPC servers. In order to achieve this, we use
the reflect package and build tags heavily to permit a loosely coupled
configuration parsing system for the sub RPC servers.

We start with a new `subRpcServerConfigs` struct which is _always_
present.  This struct has its own group, and will house a series of
sub-configs, one for each sub RPC server. Each sub-config is actually
gated behind a build flag, and can be used to allow users on the command
line or in the config to specify arguments related to the sub-server. If
the config isn't present, then we don't attempt to parse it at all, if
it is, then that means the RPC server has been registered, and we should
parse the contents of its config.

The `subRpcServerConfigs` struct has two main methods:
`PopulateDependancies` and `FetchConfig`. The `PopulateDependancies` is
used to dynamically locate and set the config fields for each new
sub-server. As the config may not actually have any fields (if the build
flag is off), we use the reflect pacakge to determine if things are
compiled in or not, then if so, we dynamically set each of the config
parameters. The `PopulateDependancies` method implements the
`lnrpc.SubServerConfigDispatcher` interface. Our goal is to allow sub
servers to look up their actual config in this main config struct. We
achieve this by using reflect to look up the target field _as if it were
a key in a map_. If the field is found, then we check if it has any
actual attributes (it won't if the build flag is off), if it is, then we
return it as we expect it to be populated already.
lnd+rpc: modify rpcServer to fully manaage listeners and gRPC, handle…
… sub-servers

In this commit, we modify the existing rpcServer to fully manage the
macaroons, gRPC server, and also seek out and create all sub-servers.
With this change, the RPC server gains more responsibility, as it
becomes the "root" server in the hierarchy of gRPC sub-servers.

In addition to creating each sub-server, it will also merge the set of
macaroon permissions for each sub-server, with the permissions of the
rest of the RPC infra. As a result, each sub-server is able to
independently specify what it needs w.r.t macaroon permissions and have
that taken care of by the RPC server. In order to achieve this, we need
to unify the creation of the RPC interceptors, and also fully manage the
gRPC server ourselves.

Some examples with various build configs:
```
i  make build
 Building debug lnd and lncli.
go build -v -tags="dev" -o lnd-debug -ldflags "-X github.com/lightningnetwork/lnd/build.Commit=v0.5-beta-143-gb2069914c4b76109b7c59320dc48f8a5f30deb75-dirty" github.com/lightningnetwork/lnd
go build -v -tags="dev" -o lncli-debug -ldflags "-X github.com/lightningnetwork/lnd/build.Commit=v0.5-beta-143-gb2069914c4b76109b7c59320dc48f8a5f30deb75-dirty" github.com/lightningnetwork/lnd/cmd/lncli

i  ./lnd-debug --debuglevel=debug --signrpc.signermacaroonpath=~/sign.macaroon
unknown flag `signrpc.signermacaroonpath'
unknown flag `signrpc.signermacaroonpath'

i  make build tags=signerrpc
 Building debug lnd and lncli.
go build -v -tags="dev signerrpc" -o lnd-debug -ldflags "-X github.com/lightningnetwork/lnd/build.Commit=v0.5-beta-143-gb2069914c4b76109b7c59320dc48f8a5f30deb75-dirty" github.com/lightningnetwork/lnd
go build -v -tags="dev signerrpc" -o lncli-debug -ldflags "-X github.com/lightningnetwork/lnd/build.Commit=v0.5-beta-143-gb2069914c4b76109b7c59320dc48f8a5f30deb75-dirty" github.com/lightningnetwork/lnd/cmd/lncli

i  ./lnd-debug --debuglevel=debug --signrpc.signermacaroonpath=~/sign.macaroon
2018-10-22 17:31:01.132 [INF] LTND: Version: 0.5.0-beta commit=v0.5-beta-143-gb2069914c4b76109b7c59320dc48f8a5f30deb75-dirty, build=development, logging=default
2018-10-22 17:31:01.133 [INF] LTND: Active chain: Bitcoin (network=simnet)
2018-10-22 17:31:01.140 [INF] CHDB: Checking for schema update: latest_version=6, db_version=6
2018-10-22 17:31:01.236 [INF] LTND: Primary chain is set to: bitcoin
2018-10-22 17:31:02.391 [INF] LNWL: Opened wallet
2018-10-22 17:31:03.315 [INF] LNWL: The wallet has been unlocked without a time limit
2018-10-22 17:31:03.315 [INF] LTND: LightningWallet opened
2018-10-22 17:31:03.319 [INF] LNWL: Catching up block hashes to height 3060, this will take a while...
2018-10-22 17:31:03.320 [INF] HSWC: Restoring in-memory circuit state from disk
2018-10-22 17:31:03.320 [INF] LNWL: Done catching up block hashes
2018-10-22 17:31:03.320 [INF] HSWC: Payment circuits loaded: num_pending=0, num_open=0
2018-10-22 17:31:03.322 [DBG] LTND: Populating dependencies for sub RPC server: Signrpc
```

As for the config, an example is:
```
[signrpc]
signrpc.signermacaroonpath=~/signer.macaroon
```
lnrpc: add new recursive proto generation script
In this commit, we add a recursive proto generation script. This avoids
having to add a new script for each upcoming sub-server.
lnrpc/signrpc: add ComputeInputScript to the Signer sub-server
In this commit, we add the ComputeInputScript which will allow callers
to obtain witnesses for all outputs under control of the wallet. This
allows external scripting of things like coin join, etc.

@Roasbeef Roasbeef force-pushed the Roasbeef:signer-service branch from 4d6e6e7 to b0a7a57 Nov 29, 2018

@Roasbeef Roasbeef merged commit fc4fe07 into lightningnetwork:master Nov 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
}

message KeyDescriptor {
oneof key {

This comment has been minimized.

@joostjager

joostjager Nov 29, 2018

Collaborator

This oneof construct is not working with walletkit.DeriveNextKey. It returns both the locator and the bytes.

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