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

Support external RPC servers via IPC #1434

Merged
merged 15 commits into from
Jan 28, 2019

Conversation

cryptocode
Copy link
Contributor

@cryptocode cryptocode commented Dec 8, 2018

Remaining

  • Add tests (write an IPC client class for this)
  • Document the framing format in the code
  • Add ipc logging option with upgrade path
  • Config upgrade path (merge Error type and config handling #1416 first)
    After reviewing/testing, will also add this:
  • Replace boost::bind with lambdas
  • Utilize the new nano::timer class
  • Introduce session id for improved logging

This adds support for external RPC servers that communicate with the node via IPC (currently domain sockets and tcp).

Using external web servers can benefit service scalability and security. IPC connections are persistent, which should reduce time_wait (file descriptor exhaustion) issues on busy servers.

The internal RPC server can thus be disabled in the config, yet have the functionality exposed via IPC. The JSON format is exactly the same as the internal server - clients only need to change URL.

A proof-of-concept REST server in Go is available at https://github.com/nanocurrency/rpc-go.

The IPC gateway allows for other message payloads like protobuf (there's a PR for protobuf which would need some work to work with this PR)

Technically, the IPC gateway defines a simple framing format with an encoding flag. In this case, the encoding is 1 (json rpc) which expects a payload of 32-bit big-endian size + json, which is also the case for the response. The json payload is forwarded to the node's existing RPC action handler. More info will be added to the wiki.

Configuration

To test the rpc-go server, enable either tcp or local ipc in the node config. Do the corresponding change in rpc-go's config.json (which uses domain sockets by default)

The rpc_enable flag can still be active, to access the current RPC server.

@cryptocode cryptocode force-pushed the feature/rpc-externalization branch from 93e5ca9 to b7c4cca Compare December 8, 2018 03:27
@rkeene rkeene added enhancement major This item indicates the need for or supplies a major or notable change labels Dec 8, 2018
@cryptocode cryptocode force-pushed the feature/rpc-externalization branch 2 times, most recently from e894a49 to d697d70 Compare December 12, 2018 15:13
@cryptocode
Copy link
Contributor Author

@meltingice is working on an IPC lib and gRPC interface based on this PR

https://github.com/meltingice/nano-ipc-js
https://github.com/meltingice/nano-grpc

@rkeene rkeene force-pushed the feature/rpc-externalization branch from d697d70 to 151f565 Compare December 28, 2018 22:45
nano/nano_node/daemon.cpp Outdated Show resolved Hide resolved
@meltingice
Copy link

FYI if anyone else runs into this issue... after updating to the latest commit on this PR, the node could no longer deserialize the config. It was missing the node.logging.timing config option, which didn't get added by a config upgrade. Works fine once added. Default is false.

@cryptocode
Copy link
Contributor Author

Yeah, the timing upgrade was recently added to master

@meltingice
Copy link

meltingice commented Dec 29, 2018

I am getting an exception that's crashing the node when attempting to call account_representative_set via IPC on an unopened account. The crash does not happen when attempting the same call via the HTTP RPC.

The call I'm making is:

{
  "action": "account_representative_set",
  "wallet": "523325FC5B61CB450035B32AAE11FE7F02BEB5D3F736EF59FC3C7B0FAD35C46C",
  "account": "xrb_118fd6yj4mzc17k3q9ugygdqwj8cxqia8hgrxijb1efahsmnwzk8qs3foxnt",
  "representative": "xrb_1x7biz69cem95oo7gxkrw6kzhfywq4x5dupw4z1bdzkb74dk9kpxwzjbdhhs"
}

And the crash message is:

libc++abi.dylib: terminating with uncaught exception of type std::__1::bad_weak_ptr: bad_weak_ptr
[1]    11184 abort      ./nano_node --daemon --data_path=/Users/ryanlefevre/RaiBlocks/

I'm running this node on macOS 10.14.2.

@cryptocode
Copy link
Contributor Author

@meltingice thanks for the report. This seems to be related to action handlers with asynchronous responses (wallet->change_async in this case). I'll look into it.

@meltingice
Copy link

Just tested and appears to be fixed now. Thanks @cryptocode!

@cryptocode cryptocode added the incomplete This item is incomplete and should not be merged if it is a pull request label Dec 29, 2018
@zhyatt zhyatt added this to the V18.0 milestone Dec 31, 2018
@zhyatt zhyatt requested review from wezrule and clemahieu January 7, 2019 14:27
@cryptocode cryptocode force-pushed the feature/rpc-externalization branch from e225eac to 65d55a3 Compare January 10, 2019 03:00
@cryptocode cryptocode force-pushed the feature/rpc-externalization branch from 5ea4099 to 7bd3404 Compare January 17, 2019 18:54
@cryptocode cryptocode force-pushed the feature/rpc-externalization branch from 7bd3404 to bf83204 Compare January 17, 2019 22:40
@cryptocode cryptocode removed medium effort remaining incomplete This item is incomplete and should not be merged if it is a pull request labels Jan 20, 2019
Copy link
Contributor

@clemahieu clemahieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@cryptocode cryptocode force-pushed the feature/rpc-externalization branch from 9b68c4d to 13659df Compare January 23, 2019 19:03
nano/node/nodeconfig.hpp Outdated Show resolved Hide resolved
nano/node/ipc.cpp Outdated Show resolved Hide resolved
nano/node/ipc.cpp Outdated Show resolved Hide resolved
nano/node/ipc.cpp Outdated Show resolved Hide resolved
nano/node/ipc.cpp Outdated Show resolved Hide resolved
nano/node/ipc.hpp Outdated Show resolved Hide resolved
nano/node/ipc.hpp Outdated Show resolved Hide resolved
nano/node/ipc.cpp Outdated Show resolved Hide resolved
nano/node/ipc.hpp Show resolved Hide resolved
nano/node/ipc.cpp Outdated Show resolved Hide resolved
@cryptocode cryptocode merged commit f492cfc into nanocurrency:master Jan 28, 2019
@meltingice
Copy link

The JS client library has been updated to match the preamble changes. https://www.npmjs.com/package/nano-ipc

@zhyatt zhyatt mentioned this pull request Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement major This item indicates the need for or supplies a major or notable change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants