multiprocess: Add basic spawn and IPC support #19160

ryanofsky · 2020-06-03T15:35:41Z

This PR is part of the process separation project.

This PR adds basic process spawning and IPC method call support to bitcoin-node executables built with --enable-multiprocess[*].

These changes are used in #10102 to let node, gui, and wallet functionality run in different processes, and extended in #19460 and #19461 after that to allow gui and wallet processes to be started and stopped independently and connect to the node over a socket.

These changes can also be used to implement new functionality outside the bitcoin-node process like external indexes or pluggable transports (#18988). The Ipc::spawnProcess and Ipc::serveProcess methods added here are entry points for spawning a child process and serving a parent process, and being able to make bidirectional, multithreaded method calls between the processes. A simple example of this is implemented in commit "Add echoipc RPC method and test."

Changes in this PR aside from the echo test were originally part of #10102, but have been split and moved here for easier review, and so they can be used for other applications like external plugins.

Additional notes about this PR can be found at https://bitcoincore.reviews/19160

[*] Note: the --enable-multiprocess feature is still experimental, and not enabled by default, and not yet supported on windows. More information can be found in doc/multiprocess.md

DrahtBot · 2020-06-04T00:27:49Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) #20487 (Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift)
multiprocess: Add bitcoin-gui -ipcconnect option #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
multiprocess: Add bitcoin-wallet -ipcconnect option #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
Log RPC parameters (arguments) if -debug=rpcparams #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ryanofsky · 2020-06-05T13:41:03Z

Updated d38f790 -> 9956298 (pr/ipc-echo.1 -> pr/ipc-echo.2, compare) fixing missing include causing build failures https://travis-ci.org/github/bitcoin/bitcoin/jobs/694309633#L2311
Updated 9956298 -> 885f0d1 (pr/ipc-echo.2 -> pr/ipc-echo.3, compare) to fix missing include error https://travis-ci.org/github/bitcoin/bitcoin/jobs/694434815#L2574
Rebased 885f0d1 -> 5219272 (pr/ipc-echo.3 -> pr/ipc-echo.4, compare) due to conflict with #19176 and reducing Init class dependencies

promag · 2021-04-06T09:20:46Z

Thanks for reviewing!

Tested ACK 1290ccf on bionic. I'm having issues building on macOS but probably something is bad on my system, I'll check later.

I'd definitely encourage you to post an issue with any details to https://github.com/chaincodelabs/libmultiprocess/issues/new. Even if the issue isn't strictly related to libmultiprocess library, or even if it's not really a bug, all posts are welcome there and it's a good place to discuss problems and possible improvements without getting lost in PR review discussion here.

Submitted chaincodelabs/libmultiprocess#52

I wonder if is it possible to debug on the spawned processes so that b echoipc would work?

It can be awkward to try to attach to a short lived process with GDB. If you just want to set one breakpoint like that, that the easiest way may be to use the follow-fork-mode child GDB option. In other circumstances, I've been using the following patch to debug with GDB : ryanofsky@30f5c5a. This lets you set a STOP=node or STOP=wallet environment variable that newly spawned bitcoin-node or bitcoin-wallet processes will look for, and if they detect it, they will halt themselves on startup and print gdb command lines that can be used to attach to these processes and manually resume them.

Right, both approaches are suggested by looking around. I think it might make sense to include ryanofsky@30f5c5a or similar in a follow up and relevant instructions.

dongcarl · 2021-04-16T19:32:46Z

re-ACK 1290ccf

Reviewed git range-diff origin/master 6a2951a7c7690e4f974fb938e1434ca836762207 1290ccf8c70f5f11148683c3f69044fac9956e05 and it was mostly comment changes.

Question that has no bearing on merge-ability: why was #include <memory> added to src/interfaces/echo.cpp when it is already included in src/interfaces/echo.h?

ryanofsky · 2021-04-16T20:56:16Z

Thanks for rechecking!

Question that has no bearing on merge-ability: why was #include <memory> added to src/interfaces/echo.cpp when it is already included in src/interfaces/echo.h?

Just preference, I guess. If you want to read a treatise on includes see https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md. But I think if you use something in a file, you should include it or forward declare it, and more complicated rules are counterproductive.

laanwj · 2021-04-22T20:35:44Z

Looks like there has been a silent merge conflict, unfortunately, while building locally i get the following error:

…/bitcoin/src/rpc/misc.cpp:662:47: error: use of undeclared identifier 'EnsureNodeContext'; did you mean 'EnsureAnyNodeContext'?
            if (interfaces::Ipc* ipc = Assert(EnsureNodeContext(request.context).init)->ipc()) {
                                              ^~~~~~~~~~~~~~~~~
                                              EnsureAnyNodeContext

EnsureNodeContext was removed in 038854f (#21391)

Fix "Disable GCC suggest-override warnings for proxy clients" chaincodelabs/libmultiprocess#40 is needed to prevent cirrus GCC failure https://cirrus-ci.com/task/6000489311502336?command=ci#L4294 This also includes other recent changes chaincodelabs/libmultiprocess#35 Fix README.md markdown chaincodelabs/libmultiprocess#37 Add "make check" target to build and run tests chaincodelabs/libmultiprocess#38 Add "extends" inherited method support chaincodelabs/libmultiprocess#41 Avoid depending on argument default constructors chaincodelabs/libmultiprocess#42 Support attaching custom cleanup functions to proxy client and server classes chaincodelabs/libmultiprocess#43 Drop hardcoded #include lines in generated files

Add bitcoin-node startup code to let it spawn and be spawned by other processes

Add simple interfaces::Echo IPC interface with one method that just takes and returns a string, to test multiprocess framework and provide an example of how it can be used to spawn and call between processes.

ryanofsky · 2021-04-23T18:13:28Z

#19160 (comment)

Looks like there has been a silent merge conflict

Thanks! Rebased and updated the call site.

Rebased 1290ccf -> 84934bf (pr/ipc-echo.30 -> pr/ipc-echo.31, compare) due to reported conflict with #21391

fjahr · 2021-04-24T19:45:50Z

re-ACK 84934bf

Checked range-diff that only changes since last review were rebasing and fixing the silent merge conflict. Also built and ran tests locally and tested echo RPC manually.

ariard

ACK 84934bf. Changes since last ACK fixes the silent merge conflict about EnsureAnyNodeContext(). Rebuilt and checked again debug command echoipc.

From bitcoin/bitcoin#19160 (comment)

Looks like this hasn't been needed since chaincodelabs/libmultiprocess#25 and was just missed in bitcoin#19160.

07dcf1a build: remove boost dep from libmultiprocess (fanquake) Pull request description: Looks like this hasn't been needed since chaincodelabs/libmultiprocess#25 and was just missed in #19160. ACKs for top commit: ryanofsky: Code review ACK 07dcf1a. Should probably wait for GUIX build results, but I think this should be fine hebasto: ACK 07dcf1a Tree-SHA512: 7988efd4aaf6ad512d60cfd33f350df56090daf88aac3aed2a1d400e80bc723dc27d27f5fa5d75359f9fae60d04b87d4b120d4e79e3079df8631956ab6c3b83c

Looks like this hasn't been needed since chaincodelabs/libmultiprocess#25 and was just missed in bitcoin#19160.

…rectory Error was reported by SatoriHoshiAiko in bitcoin#25207 and happens unpredictably because make doesn't always build dependencies in the same order. The source file src/ipc/capnp/protocol.cpp includes some generated headers so needs to have an explicit dependency specified in the makefile so the headers will be generated before the file is compiled. bitcoin#19160 added the explicit dependency, but it was incorrect because it referred to an old file path from before the source file was renamed (ipc.cpp -> protocol.cpp)

…tory Error was reported by SatoriHoshiAiko in bitcoin#25207 and happens unpredictably because make doesn't always build dependencies in the same order. The source file src/ipc/capnp/protocol.cpp includes some generated headers so needs to have an explicit dependency specified in the makefile so the headers will be generated before the file is compiled. bitcoin#19160 added the explicit dependency, but it was incorrect because it referred to an old file path from before the source file was renamed (ipc.cpp -> protocol.cpp)

… file or directory 44904aa multiprocess build cleanup: comment on manual dependencies (Ryan Ofsky) 6e1c16c multiprocess build fix: ipc/capnp/init.capnp.h: No such file or directory (Ryan Ofsky) Pull request description: Error was reported by SatoriHoshiAiko in #25207 and happens unpredictably because make doesn't always build dependencies in the same order. The source file `src/ipc/capnp/protocol.cpp` includes some generated headers so needs to have an explicit dependency specified in the makefile so the headers will be generated before the file is compiled. #19160 added the explicit dependency, but it was incorrect because it referred to an old file path from before the source file was renamed (`ipc.cpp` -> `protocol.cpp`) ACKs for top commit: hebasto: re-ACK 44904aa Tree-SHA512: 578ef4ef35a97e9d8d4e6d4edf39e57a32674234bf145e8159268324cb6ba15b420517afc07f6f42bb11a562954ea74af686c3fb92ce178c1fc378421b352531

hebasto · 2023-07-13T17:35:08Z

src/bitcoind.cpp

+    int exit_status;
+    std::unique_ptr<interfaces::Init> init = interfaces::MakeNodeInit(node, argc, argv, exit_status);
+    if (!init) {
+        return exit_status;
+    }


I apologize for revisiting this old PR, but I couldn't resist asking to clarify things for me.

As far as I understand, interfaces::MakeNodeInit returns a std::unique_ptr that owns an instance of init::BitcoindInit or throws an exception. Given this understanding, it seems unclear how the return exit_status; statement could ever execute.

As far as I understand, interfaces::MakeNodeInit returns a std::unique_ptr that owns an instance of init::BitcoindInit or throws an exception. Given this understanding, it seems unclear how the return exit_status; statement could ever execute.

There are two interfaces:MakeNodeInit functions. The one in src/init/bitcoind.cpp which is linked into bitcoind will leave exit_status unchanged, as you say. The one in src/init/bitcoin-node.cpp which is linked into bitcoin-node will set the exit status if there is an IPC error.

Right. Thank you.

DrahtBot added Build system Docs GUI RPC/REST/ZMQ Tests Wallet labels Jun 3, 2020

maflcko removed Docs GUI Tests RPC/REST/ZMQ Wallet labels Jun 3, 2020

ryanofsky force-pushed the pr/ipc-echo branch 2 times, most recently from 9956298 to 885f0d1 Compare June 3, 2020 23:58

DrahtBot mentioned this pull request Jun 5, 2020

Reduce cs_main lock accumulation during GUI startup #19011

Merged

DrahtBot added the Needs rebase label Jun 9, 2020

ryanofsky force-pushed the pr/ipc-echo branch from 885f0d1 to 5219272 Compare June 11, 2020 03:52

DrahtBot removed the Needs rebase label Jun 11, 2020

ryanofsky mentioned this pull request Apr 6, 2021

Build fail in bitcoin core on macOS chaincodelabs/libmultiprocess#52

Closed

ryanofsky added 6 commits April 23, 2021 03:02

multiprocess: Add Ipc and Init interface definitions

745c9ce

multiprocess: Add Ipc interface implementation

10afdf0

multiprocess: Add bitcoin-node process spawning support

ddf7ecc

Add bitcoin-node startup code to let it spawn and be spawned by other processes

multiprocess: Add comments and documentation

7d76cf6

multiprocess: Add echoipc RPC method and test

84934bf

Add simple interfaces::Echo IPC interface with one method that just takes and returns a string, to test multiprocess framework and provide an example of how it can be used to spawn and call between processes.

ryanofsky force-pushed the pr/ipc-echo branch from 1290ccf to 84934bf Compare April 23, 2021 18:10

ariard reviewed Apr 26, 2021

View reviewed changes

laanwj merged commit ac219dc into bitcoin:master Apr 27, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 27, 2021

Merge bitcoin#19160: multiprocess: Add basic spawn and IPC support

714e1f2

ryanofsky added a commit to ryanofsky/libmultiprocess that referenced this pull request Jul 20, 2021

Add documentation about interface definitions

1a3dc8d

From bitcoin/bitcoin#19160 (comment)

ryanofsky mentioned this pull request Jul 20, 2021

Add doc/ folder, split up readme and add more usage documentation chaincodelabs/libmultiprocess#55

Merged

fanquake added a commit to fanquake/bitcoin that referenced this pull request Feb 18, 2022

build: remove boost dep from libmultiprocess

07dcf1a

Looks like this hasn't been needed since chaincodelabs/libmultiprocess#25 and was just missed in bitcoin#19160.

fanquake mentioned this pull request Feb 18, 2022

build: remove boost dep from libmultiprocess #24385

Merged

hmel pushed a commit to hmel/bitcoin that referenced this pull request Feb 20, 2022

build: remove boost dep from libmultiprocess

7ab17fd

Looks like this hasn't been needed since chaincodelabs/libmultiprocess#25 and was just missed in bitcoin#19160.

ryanofsky mentioned this pull request May 25, 2022

multiprocess build fix: ipc/capnp/init.capnp.h: No such file or directory #25214

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge ac219dc into merged_master (Bitcoin PR bitcoin/bitcoin#19160)

5f0c45b

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

hebasto reviewed Jul 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multiprocess: Add basic spawn and IPC support #19160

multiprocess: Add basic spawn and IPC support #19160

ryanofsky commented Jun 3, 2020 •

edited

Loading

DrahtBot commented Jun 4, 2020 •

edited

Loading

ryanofsky commented Jun 5, 2020 •

edited

Loading

promag commented Apr 6, 2021

dongcarl commented Apr 16, 2021 •

edited

Loading

ryanofsky commented Apr 16, 2021

laanwj commented Apr 22, 2021

ryanofsky commented Apr 23, 2021

fjahr commented Apr 24, 2021 •

edited

Loading

ariard left a comment

hebasto Jul 13, 2023

ryanofsky Jul 13, 2023

hebasto Jul 13, 2023

multiprocess: Add basic spawn and IPC support #19160

multiprocess: Add basic spawn and IPC support #19160

Conversation

ryanofsky commented Jun 3, 2020 • edited Loading

DrahtBot commented Jun 4, 2020 • edited Loading

Conflicts

ryanofsky commented Jun 5, 2020 • edited Loading

promag commented Apr 6, 2021

dongcarl commented Apr 16, 2021 • edited Loading

ryanofsky commented Apr 16, 2021

laanwj commented Apr 22, 2021

ryanofsky commented Apr 23, 2021

fjahr commented Apr 24, 2021 • edited Loading

ariard left a comment

Choose a reason for hiding this comment

hebasto Jul 13, 2023

Choose a reason for hiding this comment

ryanofsky Jul 13, 2023

Choose a reason for hiding this comment

hebasto Jul 13, 2023

Choose a reason for hiding this comment

ryanofsky commented Jun 3, 2020 •

edited

Loading

DrahtBot commented Jun 4, 2020 •

edited

Loading

ryanofsky commented Jun 5, 2020 •

edited

Loading

dongcarl commented Apr 16, 2021 •

edited

Loading

fjahr commented Apr 24, 2021 •

edited

Loading