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

wezrule
Copy link
Contributor

@wezrule wezrule 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 30 commits March 14, 2019 19:37
… script to connect with IPC server correctly
@wezrule wezrule added enhancement universe This item indicates the need for or supplies changes caused by external factors build-error rpc nonbreaking change tool Introduces or updates a tool or command ipc labels Mar 25, 2019
@wezrule wezrule added this to the V19.0 milestone Mar 25, 2019
@wezrule wezrule self-assigned this Mar 25, 2019
@wezrule wezrule requested review from argakiig and cryptocode March 25, 2019 15:17
@cryptocode
Copy link
Contributor

cryptocode 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 requested a review from clemahieu March 26, 2019 15:30
@wezrule
Copy link
Contributor Author

wezrule 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 out_of_process_rpc branch March 29, 2019 11:13
@zhyatt zhyatt removed this from the V19.0 milestone Apr 1, 2019
@zhyatt zhyatt added rpc Changes related to Remote Procedure Calls and removed rpc nonbreaking change labels Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-error enhancement ipc rpc Changes related to Remote Procedure Calls tool Introduces or updates a tool or command universe This item indicates the need for or supplies changes caused by external factors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants