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

Modernize the RPC server. #337

Merged
merged 1 commit into from Jan 29, 2016

Conversation

Projects
None yet
5 participants
@jrick
Member

jrick commented Dec 8, 2015

This is a rather monolithic commit that moves the old RPC server to
its own package (rpc/legacyrpc), introduces a new RPC server using
gRPC (rpc/rpcserver), and provides the ability to defer wallet loading
until request at a later time by an RPC (--noinitialload).

The legacy RPC server remains the default for now while the new gRPC
server is not enabled by default. Enabling the new server requires
setting a listen address (--experimenalrpclisten). This experimental
flag is used to effectively feature gate the server until it is ready
to use as a default. Both RPC servers can be run at the same time,
but require binding to different listen addresses.

In theory, with the legacy RPC server now living in its own package it
should become much easier to unit test the handlers. This will be
useful for any future changes to the package, as compatibility with
Core's wallet is still desired.

Type safety has also been improved in the legacy RPC server. Multiple
handler types are now used for methods that do and do not require the
RPC client as a dependency. This can statically help prevent nil
pointer dereferences, and was very useful for catching bugs during
refactoring.

To synchronize the wallet loading process between the main package
(the default) and through the gRPC WalletLoader service (with the
--noinitialload option), as well as increasing the loose coupling of
packages, a new wallet.Loader type has been added. All creating and
loading of existing wallets is done through a single Loader instance,
and callbacks can be attached to the instance to run after the wallet
has been opened. This is how the legacy RPC server is associated with
a loaded wallet, even after the wallet is loaded by a gRPC method in a
completely unrelated package.

Documentation for the new RPC server has been added to the
rpc/documentation directory. The documentation includes a
specification for the new RPC API, addresses how to make changes to
the server implementation, and provides short example clients in
several different languages.

Some of the new RPC methods are not implementated exactly as described
by the specification. These are considered bugs with the
implementation, not the spec. Known bugs are commented as such.

@jrick

This comment has been minimized.

Member

jrick commented Dec 8, 2015

I'm still working on finishing the new rpc methods but I feel the rest of this code is mature enough now for others to check out.

@jrick jrick force-pushed the jrick:grpc branch from e93fa29 to e0c460e Dec 8, 2015

@jrick

This comment has been minimized.

Member

jrick commented Dec 8, 2015

The old RPC code will be hard to review. Off the top of my head, here are the major changes made:

  • Handlers made more typesafe for the rpc client, and lookup map saves one or the other (or both when the rpc client is optional)
  • The Help handler now requests a HTTP POST client from an existing RPC client, instead of creating a new one itself (this is because it has no access to the global config)
  • The server no longer saves the active network. It uses the wallet's instead.
  • Method lookup and execution is simplified (see lazyApplyHandler). Closure type no longer requires passing in the request as a parameter.
  • The Stop method is now blocking and waits for shutdown to complete.
  • The stop RPC closes the channel returned by RequestProcessShutdown, and when the goroutine that calls that unblocks, an interrupt is simulated.
ShowVersion bool `short:"V" long:"version" description:"Display version information and exit"`
Create bool `long:"create" description:"Create the wallet if it does not exist"`
CreateTemp bool `long:"createtemp" description:"Create a temporary simulation wallet (pass=password) in the data directory indicated; must call with --datadir"`
DataDir string `short:"D" long:"datadir" description:"Directory to store wallets and transactions"`

This comment has been minimized.

@davecgh

davecgh Dec 8, 2015

Member

Everything else in the suite uses -b for the data directory. I think this should be consistent.

This comment has been minimized.

@jrick

jrick Dec 9, 2015

Member

Opened a new issue for this: #339

@davecgh

This comment has been minimized.

Member

davecgh commented Dec 8, 2015

I like the grouping of the options in the source. It would be nice (not in this PR, but just an observation) to see if go-flags will support grouping the options for display and config file purposes. I would certainly be nice for them to be grouped when shown with -h/--help.

@@ -0,0 +1,9 @@
package legacyrpc

type Options struct {

This comment has been minimized.

@davecgh

davecgh Dec 8, 2015

Member

Comments?

@jrick jrick referenced this pull request Dec 8, 2015

Closed

New RPC API #220

return &Server{wallet}
}

func (s *Server) Ping(cxt context.Context, req *pb.PingRequest) (*pb.PingResponse, error) {

This comment has been minimized.

@davecgh

davecgh Dec 9, 2015

Member

Shouldn't all these cxt be ctx? First, ctx matches the name used in api.pb.go and second, context is almost always abbreviated as ctx.

return &pb.PingResponse{}, nil
}

func (s *Server) Network(cxt context.Context, req *pb.NetworkRequest) (*pb.NetworkResponse, error) {

This comment has been minimized.

@davecgh

davecgh Dec 9, 2015

Member

All of these functions need comments.


// TODO: Spendable currently includes multisig outputs that may not
// actually be spendable without additional keys.
return &pb.BalanceResponse{Total: int64(total), Spendable: int64(spendable), ImmatureReward: immatureReward}, nil

This comment has been minimized.

@davecgh

davecgh Dec 9, 2015

Member

Not following standard style for overly long code lines:

return &pb.BalanceResponse{
    Total: int64(total),
    Spendable: int64(spendable),
    ImmatureReward: immatureReward,
}, nil
@davecgh

This comment has been minimized.

Member

davecgh commented Dec 9, 2015

Almost all of rpcserver/server.go needs to be scrubbed to match the standard project style as it's currently ignoring the standards in pretty much every function.

@@ -165,6 +167,14 @@ type accountInfo struct {
lastInternalAddr ManagedAddress
}

type AccountProperties struct {

This comment has been minimized.

@davecgh

davecgh Dec 9, 2015

Member

Exported type must be commented.

go w.txCreator()
go w.walletLocker()
}

func (w *Wallet) SynchronizeRPC(chainClient *chain.RPCClient) {

This comment has been minimized.

@davecgh

davecgh Dec 9, 2015

Member

Exported functions require comments.

I'll not comment on any more, but the whole PR needs to be scrubbed for this.

@jrick

This comment has been minimized.

Member

jrick commented Dec 9, 2015

Commenting for myself to fix later: SynchronizeRPC does nothing if the rpc client is already set, so the connection logic elsewhere needs a way to replace this client after a disconnect.

@jrick

This comment has been minimized.

Member

jrick commented Dec 10, 2015

I am working on adding documentation to cover the new RPC server's API and developer documentation for making changes to the server. This is being done on another branch so I can preview the changes on GitHub without adding significant noise to this PR.

https://github.com/jrick/btcwallet/tree/grpc_doco/rpc/documentation

@jrick

This comment has been minimized.

Member

jrick commented Dec 28, 2015

Here's a first pass at a spec for the new api: https://github.com/jrick/btcwallet/blob/grpc_doco/rpc/documentation/api.md

It's all unstable for now, and stability can be gradually added as we see fit, but it should nonetheless be reviewed before going in the repo.

I plan to include other documentation for client usage of the new API, and developer usage (making server/api changes) since both cases require some external tools for the grpc parts (except for Go clients, which could just import the btcwallet package, but that also must be documented).

All other comments in the implementation package should be to help future me and other developers by explaining how something works, why it's done that way, marking TODOs, etc. Documentation for the API itself needs to be maintained in a language-agnostic document.

@arnuschky

This comment has been minimized.

arnuschky commented Jan 5, 2016

How can we give feedback to this API? Here?
The btcwallet is limited to BIP44-style wallets, right?

@jrick

This comment has been minimized.

Member

jrick commented Jan 5, 2016

@arnuschky comments are welcome here. The account number is the BIP0044 account, but assuming other BIP0043 purposes are added later and also use an account number, it could reference those as well.

@jrick

This comment has been minimized.

Member

jrick commented Jan 6, 2016

Regarding the timeline for this PR, I'd like to get this committed to master either today or tomorrow, with an open source release of a Windows GUI making use of this API be done by Friday.

Remaining work:

  • Fix style nits in the rpcserver package.
  • Go back through the server impl and make sure it matches the api spec.
  • Scrub new code for TODOs
@jrick

This comment has been minimized.

Member

jrick commented Jan 7, 2016

While fixing up the new server implementation to match the API, some minor bugs (differences in behavior between the api spec and the implementation) were discovered that are still unfixed. These are commented with BUGS comments above the function. I plan on fixing these later after this is on master, since they require relatively large changes for a temporary, minor inconvenience.

Or, since the api is unstable, I can make the spec match, and then change it to how it is right now later... but I don't think that gains us anything.

@jrick

This comment has been minimized.

Member

jrick commented Jan 8, 2016

As an update to my last comment, I don't feel this is ready yet for master without more review. I still want to release the source for the Windows GUI tomorrow, so installation instructions for that will have to point out this PR and branch. That GUI has been updated for the API changes I made after writing the API spec and is now working well under my limited testing.

That being said, another round of review can begin now. I haven't yet gone through all of the new code and fixed all of the new TODOs, but everything else should be more or less ready.

@jrick jrick force-pushed the jrick:grpc branch from 12df3de to 103130e Jan 8, 2016

@jrick

This comment has been minimized.

Member

jrick commented Jan 8, 2016

I've squashed all changes since the first review to make the next review a little easier, and history a little cleaner.

@jrick

This comment has been minimized.

Member

jrick commented Jan 12, 2016

I took another look into the SynchronizeRPC issue I noted above and it should not actually be a problem. The wallet is stopped after rpc client disconnect and the rpc client pointer is set to nil there, so after starting it back up SynchronizeRPC will work as intended.

I very much dislike that code flow though and want to separate the wallet structures from the syncing code asap after this is in.

@jrick

This comment has been minimized.

Member

jrick commented Jan 14, 2016

@davecgh If you'd be ok with it, I'm considering disabling the new RPC server in this branch (making the legacy one the default again) to get this on master quicker but still keeping all of the new RPC code around (just not usable). I want new feature branches to be based off of this code, such as the separation of the syncing code.

@davecgh

This comment has been minimized.

Member

davecgh commented Jan 14, 2016

I'm ok with that.

@jrick jrick force-pushed the jrick:grpc branch from 103130e to be21920 Jan 14, 2016

@jrick

This comment has been minimized.

Member

jrick commented Jan 21, 2016

I've already started work on separating the websocket RPC syncing code from the wallet itself, and I am basing it off of this branch since it had already begun some of the work by moving the RPC client to the old RPC server's functions instead of using the wallet's. However, there's one snag I'm running into.

The current wallet service in the new gRPC server only has access to the wallet. This is fine for all but one of the new RPCs: PublishTransaction. Thoughts on moving this function to a new service specifically for bitcoin network-related services vs adding the RPC (or later, SPV) client to the rpcserver.walletServer type?

edit: will have to take the second option. PublishTransaction is also supposed to add the transaction to the wallet first before sending, if it is relevant, although this isn't done yet and is commented as a bug.

@jrick jrick force-pushed the jrick:grpc branch from 30ee424 to 2e04ab8 Jan 25, 2016

@jrick jrick force-pushed the jrick:grpc branch 2 times, most recently from 0ace7ec to 68a2334 Jan 26, 2016

Modernize the RPC server.
This is a rather monolithic commit that moves the old RPC server to
its own package (rpc/legacyrpc), introduces a new RPC server using
gRPC (rpc/rpcserver), and provides the ability to defer wallet loading
until request at a later time by an RPC (--noinitialload).

The legacy RPC server remains the default for now while the new gRPC
server is not enabled by default.  Enabling the new server requires
setting a listen address (--experimenalrpclisten).  This experimental
flag is used to effectively feature gate the server until it is ready
to use as a default.  Both RPC servers can be run at the same time,
but require binding to different listen addresses.

In theory, with the legacy RPC server now living in its own package it
should become much easier to unit test the handlers.  This will be
useful for any future changes to the package, as compatibility with
Core's wallet is still desired.

Type safety has also been improved in the legacy RPC server.  Multiple
handler types are now used for methods that do and do not require the
RPC client as a dependency.  This can statically help prevent nil
pointer dereferences, and was very useful for catching bugs during
refactoring.

To synchronize the wallet loading process between the main package
(the default) and through the gRPC WalletLoader service (with the
--noinitialload option), as well as increasing the loose coupling of
packages, a new wallet.Loader type has been added.  All creating and
loading of existing wallets is done through a single Loader instance,
and callbacks can be attached to the instance to run after the wallet
has been opened.  This is how the legacy RPC server is associated with
a loaded wallet, even after the wallet is loaded by a gRPC method in a
completely unrelated package.

Documentation for the new RPC server has been added to the
rpc/documentation directory.  The documentation includes a
specification for the new RPC API, addresses how to make changes to
the server implementation, and provides short example clients in
several different languages.

Some of the new RPC methods are not implementated exactly as described
by the specification.  These are considered bugs with the
implementation, not the spec.  Known bugs are commented as such.

@jrick jrick force-pushed the jrick:grpc branch from 68a2334 to 497ffc1 Jan 29, 2016

@davecgh

This comment has been minimized.

Member

davecgh commented Jan 29, 2016

Since this isn't enabled by default and it's needed to unblock some other PRs, let's go ahead and merge this to master. Any further cleanup and issues can be hacked out then before it's enabled by default.

@conformal-deploy conformal-deploy merged commit 497ffc1 into btcsuite:master Jan 29, 2016

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jrick jrick deleted the jrick:grpc branch Feb 26, 2016

@ysangkok

This comment has been minimized.

Contributor

ysangkok commented on rpc/documentation/README.md in 497ffc1 Mar 9, 2016

should be is out of scope

This comment has been minimized.

Member

jrick replied Mar 9, 2016

Nice catch. Mind opening a PR to fix this?

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