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

Move RPC server to a new process #1857

Closed
wants to merge 35 commits into from

Conversation

Projects
None yet
3 participants
@wezrule
Copy link
Collaborator

commented Mar 25, 2019

This solves #1827

The RPC server has now been moved to a new executable. If rpc_enable is set to true a new rpc_path config option will be added which points to the nano_rpc executable, and this will be launched as a child process of the nano_node executable. When rpc_enable is false the RPC executable will not be started, however it can be started manually with nano_rpc --daemon either before or after nano_node --daemon. In the event that the node crashes or needs upgrading but the RPC process doesn't, the RPC process can try and reconnect to the node on the next RPC command fired so it does not require restarting as well.

There are no API changes required as the current JSON requests are simply passed via IPC transport mechanism to be handled inside the node now. This required removing anything related to the node from the RPC process as they are now separate services and don't know about each other (theoretically).

A new json file has been created (rpc_config.json) for it and upon upgrade the settings from config.json are migrated. enable_sign_hash and max_work_generate_difficulty have been moved to the ipc config as the node itself needed to know about it rather than the RPC server. log_rpc has currently just been removed and no explicit logging config options have been added to the RPC server, it now has a log_rpc.*.log file in similar format to the existing log file.

The dependencies of a few things were a bit messed up, causing me all sorts of issues.
Circular dependency between and lib <-> ed25519. We declare our own functions in nano/lib/interface.cpp but ed25519 is dependent on this, but lib is also dependent on ed25519 functions, so to resolve this I have created a new library crypto_lib for lack of a better name, which includes the interface.cpp functions. Annoyingly it only showed on travis linux, and I could not reproduce it locally with MSVC, Mac/Clang and Ubuntu Clang Gcc versions 5.4 (used by travis), 5.5 or 7.3. It's not clear why some executables required the ed25519 library when linking in the new crypto_lib when they don't use the ed25519 functions (static libs should only pull in symbols they need). I ended up just adding the link libraries in each executable to resolve the travis Linux linker errors. There were also circular references with the secure and lib libraries which has been refactored out as well.

A few IPC connections are made, defaulting to 8 on the live network which is user configurable with the rpc_config.json option "num_ipc_connections". The new class rpc_request_processor queues up requests and dispatches them to any available connections.

Notes:

  • At the time of writing there there are some TSAN issues with udp_channel which can affect this, most noticeably the nano-load-test which makes simultaneous requests, it worked fine before merging but something to keep in mind.
  • log_ipc has been disabled in the load-tests as there was an assertion when logging ipc calls in timer::stop () assert (state == nano::timer_state::started);. Not sure if this is related to the above problem or something completely different.
  • rpc_secure.cpp didn't compile before and some changes were made to make it compile, but have not been tested

When testing:
Try stop RPC command. It should close down both nano-node and rpc programs. If the RPC server is started successfuly and the node goes down, restarting the node should allow the RPC server to connect to it again (it started separately).
Try it with the gui wallet.
Try config changes and upgrades with both node and gui.
Test with secure RPC (-DNANO_SECURE_RPC=ON)

wezrule added some commits Mar 14, 2019

@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented Mar 25, 2019

Seems like all handlers are now in ipc.cpp file. In my opinion, IPC's responsibility should be restricted to be a gateway over various transports and manage those connections.

Would it be possible to factor out all the handler stuff and detach that from IPC? json_handler.cpp or something like that, and then IPC forwards the request there if it has the proper encoding.

In the future we may have non-JSON handlers, like protobuf, flatbuffer and whatnot.

@zhyatt zhyatt added this to CP2 (2019-03-27) in V19 Mar 25, 2019


namespace
{
std::unordered_set<std::string> create_rpc_control_impls ()

This comment has been minimized.

Copy link
@cryptocode

cryptocode Mar 26, 2019

Collaborator

I wonder if moving this to the RPC process is problematic, because there are/will be other IPC clients. Instead of controlling RPCs, I suspect we should control actions in the node. Then it doesn't matter how the action is called (rpc, raw ipc, future protobufs, ec).

I'm thinking the external RPC process should do as little such logic as possible - basically be Web server frontend to IPC.

@@ -207,7 +207,7 @@ status (parse_status::success)

void nano::message_parser::deserialize_buffer (uint8_t const * buffer_a, size_t size_a)
{
static nano::network_params network_params;
static nano::network_constants network_constants;

This comment has been minimized.

Copy link
@cryptocode

cryptocode Mar 26, 2019

Collaborator

Is the network_params changes unrelated to this PR and possible to put in a separate PR? Not important, but could reduce the size of this patch a bit if that's the case.

@zhyatt zhyatt requested a review from clemahieu Mar 26, 2019

@wezrule

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 29, 2019

This was split in various PRs, the main functionality being in: #1857

@wezrule wezrule closed this Mar 29, 2019

@wezrule wezrule deleted the wezrule:out_of_process_rpc branch Mar 29, 2019

@zhyatt zhyatt removed this from CP2 (2019-03-27) in V19 Apr 1, 2019

@zhyatt zhyatt removed this from the V19.0 milestone Apr 1, 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.