multiprocess: Add basic spawn and IPC support

ryanofsky commented at 3:35 pm on June 3, 2020: contributor

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 (https://github.com/bitcoin/bitcoin/pull/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 added the label Build system on Jun 3, 2020

DrahtBot added the label Docs on Jun 3, 2020

DrahtBot added the label GUI on Jun 3, 2020

DrahtBot added the label RPC/REST/ZMQ on Jun 3, 2020

DrahtBot added the label Tests on Jun 3, 2020

DrahtBot added the label Wallet on Jun 3, 2020

ryanofsky added this to the "In progress" column in a project

MarcoFalke removed the label Docs on Jun 3, 2020

MarcoFalke removed the label GUI on Jun 3, 2020

MarcoFalke removed the label Tests on Jun 3, 2020

MarcoFalke removed the label RPC/REST/ZMQ on Jun 3, 2020

MarcoFalke removed the label Wallet on Jun 3, 2020

ryanofsky force-pushed on Jun 3, 2020

DrahtBot commented at 0:27 am on June 4, 2020: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20487 (Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
#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 commented at 1:41 pm on June 5, 2020: contributor

Updated d38f790138089b2956207b5e98b74e1e0e116c5c -> 99562989e749e002c07d3cda1f95a5781160b187 (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 99562989e749e002c07d3cda1f95a5781160b187 -> 885f0d104141f0f157f05c02c9e4fb4c2f5c6d57 (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 885f0d104141f0f157f05c02c9e4fb4c2f5c6d57 -> 52192725a3ec1f814cf40049de45efffc226cd5b (pr/ipc-echo.3 -> pr/ipc-echo.4, compare) due to conflict with #19176 and reducing Init class dependencies

DrahtBot added the label Needs rebase on Jun 9, 2020

ryanofsky force-pushed on Jun 11, 2020

DrahtBot removed the label Needs rebase on Jun 11, 2020

hebasto commented at 9:41 am on June 11, 2020: member

Concept ACK.

in src/interfaces/base.h:49 in b12041f74d outdated

45+    void close();
46+
47+    //! Add close hook.
48+    void addCloseHook(std::unique_ptr<CloseHook> close_hook);
49+
50+    std::unique_ptr<CloseHook> m_close_hook;

ariard commented at 10:50 pm on June 15, 2020:

b12041f

AFAICT, addCloseHook stacks new hook as prefix to previous added one, thus call order is reverse than registering order ?

ryanofsky commented at 5:29 pm on June 29, 2020:

re: #19160 (review)

b12041f

AFAICT, addCloseHook stacks new hook as prefix to previous added one, thus call order is reverse than registering order ?

Added comment, this is analogous to how when multiple variables are declared in a scope, the later variables are destroyed first so destructors can depend on the earlier variables not being destroyed yet. Here later close hooks can rely on earlier close hooks not being called yet.

in src/interfaces/init.h:72 in 643d602b3f outdated

24+public:
25+    virtual ~Init() = default;
26+};
27+
28+//! Specialization of Init for current process.
29+class LocalInit : public Init

ariard commented at 10:54 pm on June 15, 2020:

643d602

LocalInit isn’t really meaningful as a name IMO, if understands well it must implemented both by parent and child processes, but methods called will diverge ? IPCWrapper , IPCSupervisor, IPCAccess, … ?

ryanofsky commented at 5:30 pm on June 29, 2020:

re: #19160 (review)

643d602

LocalInit isn’t really meaningful as a name IMO, if understands well it must implemented both by parent and child processes, but methods called will diverge ? IPCWrapper , IPCSupervisor, IPCAccess, … ?

I removed LocalInit SpawnProcess method and made it a standalone method to avoid confusion here and added comments describing Init and LocalInit classes. Init interface is just an initial interface exposed by a remote process when you spawn or connect to it. You call Init interface methods to get access to other interfaces. LocalInit is a specialization of Init for the local process. It gives access to the same Init methods, but can also provide local information like the current NodeContext pointer and current executable name.

in src/interfaces/ipc.cpp:40 in 643d602b3f outdated

32+        });
33+    }
34+    int wait(int pid) override { return mp::WaitProcess(pid); }
35+    bool serve(int& exit_status) override
36+    {
37+        if (m_argc != 3 || strcmp(m_argv[1], "-ipcfd") != 0) {

ariard commented at 10:57 pm on June 15, 2020:

643d602

Document -ipcfd usage ?

ryanofsky commented at 5:30 pm on June 29, 2020:

re: #19160 (review)

643d602

Document -ipcfd usage ?

Added comment here. It might be good to add to -help output too, but I can’t easily use ArgsManager::AddArg without losing “Invalid parameter -ipcfd” checking, and I don’t think there are any uses for -ipcfd that wouldn’t go through IpcProcess::spawn, where it’s just an implementation detail.

in src/interfaces/ipc.h:68 in 643d602b3f outdated

63+    //!
64+    //! @note: If this method is called, it needs be called before connect()
65+    //! because for ease of implementation it's inflexible and always runs the
66+    //! event loop in the foreground thread. It can share its event loop with
67+    //! connect() but can't share an event loop that was created by connect().
68+    //! This isn't really a problem because serve() is only called by spawned

ariard commented at 11:03 pm on June 15, 2020:

643d602

Maybe the connection flow between parent and child process could be documented, what should be the order between parent serve, parent connect, child serve, child connect ? in echoipc demonstration, I don’t see serve being called and thus that’s a bit confusing with regards to documentation.

ryanofsky commented at 11:45 pm on June 15, 2020:

re: #19160 (review)

643d602

Maybe the connection flow between parent and child process could be documented, what should be the order between parent serve, parent connect, child serve, child connect ? in echoipc demonstration, I don’t see serve being called and thus that’s a bit confusing with regards to documentation.

Thanks for the review! I’ll address the other comments, but just in case there is any question about serve() it’s called in main() here in the bitcoin-node executable:

https://github.com/ryanofsky/bitcoin/blob/pr/ipc-echo.4/src/bitcoind.cpp#L178-L185

and here in the bitcoin-wallet executable:

https://github.com/ryanofsky/bitcoin/blob/pr/ipc.113/src/bitcoin-wallet.cpp#L80-L88

If we wanted to have a separate bitcoin-echo executable, it could have a main() function just like those examples. I didn’t want to have a separate bitcoin-echo executable though, just because I thought it’d be confusing to have a kind of pointless bitcoin-echo program installed along with bitcoin-gui bitcoin-node and bitcoin-wallet, so I just put the echo functionality inside bitcoin-node.

ryanofsky commented at 5:31 pm on June 29, 2020:

re: #19160 (review)

643d602

Maybe the connection flow between parent and child process could be documented, what should be the order between parent serve, parent connect, child serve, child connect ? in echoipc demonstration, I don’t see serve being called and thus that’s a bit confusing with regards to documentation.

Good idea, added a description of this to the Init interface comment.

ariard commented at 11:11 pm on June 15, 2020: member

Concept ACK

Awesome work, I still have to dig into libmultiprocess before to go further and test demonstration case. AFAICT, based on --enable-multiprocess, slaved process is going to either init as usual or connect through ipc mechanism to a parent process to which fd is currently passed manually.

I confirm it could be reused easily for AltNet, both between bitcoin-node and bitcoin-altnet, and between bitcoin-altnet and each individual driver a process of its own.

ryanofsky force-pushed on Jun 29, 2020

ryanofsky commented at 10:42 pm on June 29, 2020: contributor

Updated 52192725a3ec1f814cf40049de45efffc226cd5b -> 4bc863fc88d8351bc2df64bfa1a0a9f1302b487b (pr/ipc-echo.4 -> pr/ipc-echo.5, compare) with more documentation and cleanups Rebased 4bc863fc88d8351bc2df64bfa1a0a9f1302b487b -> 5a70cf96410f78172254f9ba710a9c2ca4c5d622 (pr/ipc-echo.5 -> pr/ipc-echo.6, compare) after #19422 workaround to avoid TSAN closedir BerkeleyBatch error https://travis-ci.org/github/bitcoin/bitcoin/jobs/703336215 Rebased 5a70cf96410f78172254f9ba710a9c2ca4c5d622 -> 0602f87a085c733f1da38b3b82210c81ef018fd5 (pr/ipc-echo.6 -> pr/ipc-echo.7, compare) due to conflict with #19323

ryanofsky requested review from ariard on Jun 29, 2020

ryanofsky force-pushed on Jul 10, 2020

DrahtBot added the label Needs rebase on Jul 14, 2020

ryanofsky force-pushed on Jul 14, 2020

DrahtBot removed the label Needs rebase on Jul 14, 2020

in doc/multiprocess.md:18 in aa4d626db1 outdated

14@@ -15,7 +15,7 @@ Specific next steps after [#10102](https://github.com/bitcoin/bitcoin/pull/10102
15 
16 ## Debugging
17 
18-After [#10102](https://github.com/bitcoin/bitcoin/pull/10102), the `-debug=ipc` command line option can be used to see requests and responses between processes.
19+The `-debug=ipc` command line option can be used to see requests and responses between processes.

ariard commented at 8:59 pm on August 4, 2020:

FYI, the logs I get, are they the ones you expect ?

 02020-08-04T20:58:04Z {bitcoin-node-12666/b-httpworker.0-12684} IPC client first request from current thread, constructing waiter                                                                                  
 12020-08-04T20:58:04Z {bitcoin-node-12666/b-httpworker.0-12684} IPC client send Init.construct$Params (threadMap = <external capability>)                                                                          
 22020-08-04T20:58:04Z {bitcoin-node-12666/b-httpworker.0-12684} IPC client recv Init.construct$Results (threadMap = <external capability>)                                                                         
 32020-08-04T20:58:04Z {bitcoin-node-12666/b-httpworker.0-12684} IPC client send Init.makeEcho$Params (context = (thread = <external capability>, callbackThread = <external capability>))                          
 42020-08-04T20:58:04Z {bitcoin-node-12666/b-httpworker.0-12684} IPC client recv Init.makeEcho$Results (result = <external capability>)                                                                             
 52020-08-04T20:58:04Z {bitcoin-node-12666/b-httpworker.0-12684} IPC client send Echo.echo$Params (context = (thread = <external capability>, callbackThread = <external capability>), echo = "hello_russ")         
 62020-08-04T20:58:04Z {bitcoin-node-12666/b-httpworker.0-12684} IPC client recv Echo.echo$Results (result = "hello_russ")                                                                                          
 72020-08-04T20:58:04Z
 82020-08-04T20:58:04Z {bitcoin-node-12666/b-httpworker.0-12684} IPC client destroy N2mp11ProxyClientIN10interfaces5capnp8messages4EchoEEE                                                                          
 92020-08-04T20:58:04Z {bitcoin-node-12666/b-httpworker.0-12684} IPC client send Echo.destroy$Params (context = (thread = <external capability>, callbackThread = <external capability>))                           
102020-08-04T20:58:04Z {bitcoin-node-12666/b-httpworker.0-12684} IPC client recv Echo.destroy$Results ()
112020-08-04T20:58:04Z
122020-08-04T20:58:04Z {bitcoin-node-12666/b-httpworker.0-12684} IPC client destroy N2mp11ProxyClientIN10interfaces5capnp8messages4InitEEE                                                                          
132020-08-04T20:58:04Z bitcoin-node pid 12889 exited with status 0

ryanofsky commented at 6:59 pm on August 10, 2020:

re: #19160 (review)

FYI, the logs I get, are they the ones you expect ?

Yes, those show the requests being sent, received, and responded to.

in doc/multiprocess.md:20 in aa4d626db1 outdated

14@@ -15,7 +15,7 @@ Specific next steps after [#10102](https://github.com/bitcoin/bitcoin/pull/10102
15 
16 ## Debugging
17 
18-After [#10102](https://github.com/bitcoin/bitcoin/pull/10102), the `-debug=ipc` command line option can be used to see requests and responses between processes.
19+The `-debug=ipc` command line option can be used to see requests and responses between processes.
20 
21 ## Installation

ariard commented at 9:09 pm on August 4, 2020:

Shoud the threading model part of the documentation ? AFAICT I can tell you’re doing mention to background/foreground threads around IpcProtocol documentation. Isn’t this dependent of IpcProtocolImpl internals ? AFAII, e.g, the client process is going to call interfaces (like Chain::getHeight) with overriden virtual methods sending calls to remote server process ? On the client-side, is flow execution blocked until remote answer back ?

ryanofsky commented at 6:59 pm on August 10, 2020:

re: #19160 (review)

Shoud the threading model part of the documentation ?

Sure, I added a mention of the 1:1 threading model here, linking to the libmultiprocess library.

AFAICT I can tell you’re doing mention to background/foreground threads around IpcProtocol documentation. Isn’t this dependent of IpcProtocolImpl internals ?

Yes, the threading requirements come from bitcoin code, and the way it uses locks and callbacks, so they would be the same even if IpcProtocolImpl used a completely different protocol or implementation. There is some documentation about the libmultiprocess implementation in proxy.capnp’s ThreadMap, Thread and Context descriptions.

in src/interfaces/init.h:83 in aa4d626db1 outdated

53+class LocalInit : public Init
54+{
55+public:
56+    LocalInit(const char* exe_name, const char* log_suffix);
57+    ~LocalInit() override;
58+    virtual NodeContext& node();

ariard commented at 9:21 pm on August 4, 2020:

Why does LocalInit take a node() method ? How is it used ? A bitcoin-wallet will never have such context ?

ryanofsky commented at 7:00 pm on August 10, 2020:

re: #19160 (review)

Why does LocalInit take a node() method ? How is it used ? A bitcoin-wallet will never have such context ?

Added comment. This is called one place in AppInit() in bitcoind, bitcoin-qt, and bitcoin-node processes that run node code. It is not called in bitcoin-wallet or bitcoin-gui processes that don’t run node code. There will be a WalletContext returning method too but it’s not needed in this PR because bitcoin-wallet tool doesn’t currently do anything that requires a wallet context.

in src/interfaces/init.h:22 in aa4d626db1 outdated

16+namespace interfaces {
17+class Base;
18+class IpcProcess;
19+class IpcProtocol;
20+
21+//! Init interface providing access to other interfaces. The Init interface is

ariard commented at 9:23 pm on August 4, 2020:

The documentation you wrote down in review club notes (“The Init interface is similar to other cross-process C++ interfaces like interfaces::Node, interfaces::Wallet, interfaces::Chain and interfaces::ChainClient, providing virtual methods that can be called from other processes….”) is describing well design goal of Init IMO. Maybe just pick it up ?

ryanofsky commented at 6:59 pm on August 10, 2020:

re: #19160 (review)

The documentation you wrote down in review club notes (“The Init interface is similar to other cross-process C++ interfaces like interfaces::Node, interfaces::Wallet, interfaces::Chain and interfaces::ChainClient, providing virtual methods that can be called from other processes….”) is describing well design goal of Init IMO. Maybe just pick it up ?

Thanks, added.

in src/interfaces/init.h:43 in aa4d626db1 outdated

37+//!    descriptor it should use. It calls IpcProtocol::serve() to handle
38+//!    incoming requests from the socketpair and call Init interface methods, or
39+//!    destroy the Init interface and shut down the process.
40+//!
41+//! When connecting to an existing process, the steps are similar to spawning a
42+//! new process, except a domain socket is created instead of a socketpair, and

ariard commented at 9:24 pm on August 4, 2020:

IIRC a socketpair has a default and only-one domain (AF_UNIX), you mean a new connection is free to pick up its domain as there is no the default parent-child socketpair ready to use ?

ryanofsky commented at 0:23 am on August 11, 2020:

re: #19160 (review)

IIRC a socketpair has a default and only-one domain (AF_UNIX), you mean a new connection is free to pick up its domain as there is no the default parent-child socketpair ready to use ?

Thanks, replaced domain socket with just socket. I didn’t realize socketpairs could have domains.

in src/interfaces/init.h:39 in aa4d626db1 outdated

33+//!    proxy objects calling other remote interfaces. It can also destroy the
34+//!    Init object to shut down the spawned process.
35+//! 4. Spawned process calls IpcProcess::serve(), to read command line arguments
36+//!    and determine whether it is a spawned process and what socketpair file
37+//!    descriptor it should use. It calls IpcProtocol::serve() to handle
38+//!    incoming requests from the socketpair and call Init interface methods, or

ariard commented at 9:29 pm on August 4, 2020:

It’s unclear who is the “it” here calling Init interface methods, still the spawned subprocess ? Or do you mean the spawned subprocess has the option to shutdown itself ?

ryanofsky commented at 6:59 pm on August 10, 2020:

re: #19160 (review)

It’s unclear who is the “it” here calling Init interface methods, still the spawned subprocess ? Or do you mean the spawned subprocess has the option to shutdown itself ?

Thanks, replaced “it” with “spawned process” and clarified it shuts down when then connection is closed.

in src/interfaces/README.md:17 in 352a5564e6 outdated

13@@ -14,4 +14,6 @@ The following interfaces are defined here:
14 
15 * [`Init`](init.h) — used by multiprocess code to access interfaces above on startup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102).
16 
17+* [`Base`](base.h) — base interface class used by multiprocess code for bookkeeping and cleanup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102).

ariard commented at 9:32 pm on August 4, 2020:

I think init.h is actually introduced in next commit. Also document echo.h if it’s a long-standing debug interface ?

fjahr commented at 1:00 pm on August 5, 2020:

And also base.h is added here, so update commit number on it as well?

jnewbery commented at 2:17 pm on August 5, 2020:

Added in 19160?

ryanofsky commented at 6:58 pm on August 10, 2020:

#19160 (review)

I think init.h is actually introduced in next commit. Also document echo.h if it’s a long-standing debug interface ?

And also base.h is added here, so update commit number on it as well?

Thanks, changed

ryanofsky commented at 0:25 am on August 11, 2020:

re: #19160 (review)

Added in 19160?

Thanks, there were some earlier comments about this too. Switched

ariard commented at 9:35 pm on August 4, 2020: member

Few mores comments, I think I need to dig into #10192 or further PR to understand well the threading model.

The echoipc seems to work fine on my side :)

in src/interfaces/chain.h:8 in 352a5564e6 outdated

4@@ -5,6 +5,8 @@
5 #ifndef BITCOIN_INTERFACES_CHAIN_H
6 #define BITCOIN_INTERFACES_CHAIN_H
7 
8+#include <interfaces/wallet.h>

fjahr commented at 12:59 pm on August 5, 2020:

0#include <interfaces/base.h>

ryanofsky commented at 6:58 pm on August 10, 2020:

re: #19160 (review)

#include <interfaces/base.h>

Thanks, switched

in src/interfaces/init.cpp:15 in aa4d626db1 outdated

 9+#include <logging.h>
10+#include <util/memory.h>
11+
12+namespace interfaces {
13+namespace {
14+//! Close hook that encapsulate and deletes a moveable object.

fjahr commented at 1:58 pm on August 5, 2020:

This one could be a little more explicit I think?

0//! Close hook that encapsulate a function to be called on close.

ryanofsky commented at 6:59 pm on August 10, 2020:

re: #19160 (review)

This one could be a little more explicit I think?

Thanks, updated

in test/functional/rpc_misc.py:63 in 0602f87a08 outdated

60@@ -61,6 +61,7 @@ def run_test(self):
61         node.logging(include=['qt'])
62         assert_equal(node.logging()['qt'], True)
63

fjahr commented at 2:31 pm on August 5, 2020:

0
1        self.log.info("test echoipc (testing spawned process in multiprocess built)")

ryanofsky commented at 7:00 pm on August 10, 2020:

re: #19160 (review)

self.log.info(“test echoipc (testing spawned process in multiprocess built)”)

Thanks, added

fjahr commented at 5:01 pm on August 5, 2020: contributor

Concept ACK

plus some initial observations.

DrahtBot added the label Needs rebase on Aug 7, 2020

ryanofsky force-pushed on Aug 10, 2020

ryanofsky commented at 11:48 pm on August 10, 2020: contributor

Rebased 0602f87a085c733f1da38b3b82210c81ef018fd5 -> 22a2d3f93efd4c0eb689078e300bca8da9b1ea8a (pr/ipc-echo.7 -> pr/ipc-echo.8, compare) due to conflict with #19098 Updated 22a2d3f93efd4c0eb689078e300bca8da9b1ea8a -> 8d807a22e98eb43f8f01409f5a071d5c641d0470 (pr/ipc-echo.8 -> pr/ipc-echo.9, compare) with suggested changes Rebased 8d807a22e98eb43f8f01409f5a071d5c641d0470 -> 805680ca6ffd0b61cddf853fb9feda9a72bc06b4 (pr/ipc-echo.9 -> pr/ipc-echo.10, compare) due to conflict with #19528 Rebased 805680ca6ffd0b61cddf853fb9feda9a72bc06b4 -> 7471a776630bb1af843afd7d8e0d8eacc054194e (pr/ipc-echo.10 -> pr/ipc-echo.11, compare) due to conflict with #19550 Rebased 7471a776630bb1af843afd7d8e0d8eacc054194e -> c453881a17a9fb16424715928c0388aa8c922cb4 (pr/ipc-echo.11 -> pr/ipc-echo.12, compare) due to conflict with https://github.com/bitcoin-core/gui/pull/35 Rebased c453881a17a9fb16424715928c0388aa8c922cb4 -> 3d87403a58010ee844db39c60ba8a9382a532aca (pr/ipc-echo.12 -> pr/ipc-echo.13, compare) due to conflict with #19099 Rebased 3d87403a58010ee844db39c60ba8a9382a532aca -> e181d9929ff8ed260ab713d1c5ffea7d80e157e0 (pr/ipc-echo.13 -> pr/ipc-echo.14, compare) due to conflict with #19993 Updated e181d9929ff8ed260ab713d1c5ffea7d80e157e0 -> ff0058470b313115d50b5285dbfb66f4e0aa6a69 (pr/ipc-echo.14 -> pr/ipc-echo.15, compare) fixing macos cmake error https://travis-ci.org/github/bitcoin/bitcoin/jobs/730006940, see #20046 (comment) Updated ff0058470b313115d50b5285dbfb66f4e0aa6a69 -> b8a2b29f2b11747323ad614066e17cc9d03d75f5 (pr/ipc-echo.15 -> pr/ipc-echo.16, compare) fixing broken rpath error fix error https://travis-ci.org/github/bitcoin/bitcoin/jobs/731808080#L2547-L2548 Updated b8a2b29f2b11747323ad614066e17cc9d03d75f5 -> e99129343d137410827ec7c26ae48a995e0cf1f8 (pr/ipc-echo.16 -> pr/ipc-echo.17, compare) tweaking cmake config to limit rpath override

ryanofsky force-pushed on Aug 11, 2020

DrahtBot removed the label Needs rebase on Aug 11, 2020

DrahtBot added the label Needs rebase on Aug 14, 2020

ryanofsky force-pushed on Aug 17, 2020

DrahtBot removed the label Needs rebase on Aug 17, 2020

DrahtBot added the label Needs rebase on Aug 20, 2020

ryanofsky force-pushed on Aug 24, 2020

DrahtBot removed the label Needs rebase on Aug 24, 2020

DrahtBot added the label Needs rebase on Aug 26, 2020

ryanofsky force-pushed on Aug 26, 2020

DrahtBot removed the label Needs rebase on Aug 26, 2020

DrahtBot added the label Needs rebase on Aug 31, 2020

ryanofsky force-pushed on Sep 1, 2020

DrahtBot removed the label Needs rebase on Sep 1, 2020

DrahtBot added the label Needs rebase on Sep 23, 2020

ryanofsky force-pushed on Sep 24, 2020

DrahtBot removed the label Needs rebase on Sep 24, 2020

jonatack commented at 7:58 pm on September 26, 2020: contributor

Concept ACK, deserves review.

meshcollider commented at 12:50 pm on September 29, 2020: contributor

Concept ACK

ryanofsky force-pushed on Oct 1, 2020

ryanofsky force-pushed on Oct 2, 2020

in src/interfaces/init.h:38 in e99129343d outdated

33+//! spawned, and it is the initial interface returned by the
34+//! IpcProtocol::connect(fd) method, allowing the parent process to control the
35+//! child process after the connection is established. The interfaces::Init
36+//! interface has methods that allow the parent process to get access to every
37+//! interface supported by the child process, and when the parent process frees
38+//! the interfaces::Init object, the child process shuts down.

ariard commented at 6:03 pm on October 15, 2020:

I think this comment can be improved:

“Init interface providing access to other interfaces”, this should underscores access is provided across process boundaries
“providing virtual methods that can be called from other processes”, AFAIU, this is confusing as interfaces::Init provides both process control method (e.g makeEchoIpc(), in the future makeWallet) and an interface IpcProtocol wrapping a libmultiprocess ProxyClient/ProxyServer. I would be better to document that both control and data exchange functions are supported by interfaces::Init

Overall, I still think Init/LocalInit are loosely-defining names. The names doesn’t indicate what is initiated nor who is local and remote. IPCSupervisor/IPCWrapper as suggested would be better.

ryanofsky commented at 2:50 pm on October 21, 2020:

re: #19160 (review)

I think this comment can be improved:

“Init interface providing access to other interfaces”, this should underscores access is provided across process boundaries.

Could you be specific about the change you would suggest here? The next sentence after the quote in your comment says how the interface is used connecting to another process, so I don’t know how I would emphasize the point more.

Also, every interface in src/interfaces/ is called across process boundaries. From https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines: “Interface classes are written in a particular style so node, wallet, and GUI code doesn’t need to run in the same process”

Also, there is nothing special about the Init interface returning pointers to other interfaces. Node and Wallet and Chain interface methods all return pointers to other interfaces as well.

The only special thing about the Init interface is that it is the first interface pointer you get access to when you make a connection to a process. It’s the initial interface.

“providing virtual methods that can be called from other processes”, AFAIU, this is confusing as interfaces::Init provides both process control method (e.g makeEchoIpc(), in the future makeWallet) and an interface IpcProtocol wrapping a libmultiprocess ProxyClient/ProxyServer. I would be better to document that both control and data exchange functions are supported by interfaces::Init

Could you be more specific about what should be documented? This comment seems to be referring to LocalInit methods not Init methods. The Init methods are makeEcho, makeNode, makeChain, and makeWalletClient. These are all methods called to get access to other interfaces.

LocalInit is a different class from Init. It’s a connector between src/init code and IPC code. It could be broken up or renamed, but I’d be reluctant to make changes without knowing specific design goals.

Overall, I still think Init/LocalInit are loosely-defining names. The names doesn’t indicate what is initiated nor who is local and remote. IPCSupervisor/IPCWrapper as suggested would be better.

There is no special supervision or wrapping happening in the Init interface that isn’t happening in all the other interfaces (Node/Wallet/Chain) when IPC is enabled. The only unique thing about the Init interface is that it is the first, initial interface that a process gets access to before it can access other interfaces it actually wants to use. Other reasonable names for interfaces::Init might be interfaces::Initial or interfaces::Factory or interfaces::Process or interfaces::Global.

The LocalInit class is different than the Init class. For one thing it is an actual class with implemented methods and not a pure interface. Maybe it could be called interfaces::InitImpl or interfaces::Startup or interfaces::LocalProcess. Maybe it could be structured differently or not inherit from Init.

I do think neither Init nor LocalInit classes should contain IPC in their names, because it would be misleading in places where they are not implemented to use IPC. The LocalInit class is used in startup in code in src/init, src/wallet/, and src/qt/ to create initial interfaces pointers whether or not there is any IPC. In bitcoind and bitcoin-qt binaries where there is no IPC, the LocalInit implementations do not contain IPC code and they don’t do any supervision or wrapping, so naming these IPCSupervisor/IPCWrapper would be more misleading than illuminating.

ariard commented at 5:59 pm on October 28, 2020:

I think my main concern is about lighting the interfaces hierarchy, where they’re produced, and when they’re consumed. See my graphical representation proposal as a way to improve it.

Other reasonable names for interfaces::Init might be interfaces::Initial or interfaces::Factory or interfaces::Process or interfaces::Global.

+1 for interfaces::Factory.

Maybe it could be called interfaces::InitImpl or interfaces::Startup or interfaces::LocalProcess

+1 for interfaces::Startup.

Maybe it could be structured differently or not inherit from Init.

At least, I think LocalInitImpl which is different for bitcoin-gui/bitcoin-node/bitcoin-wallet,bitcoind should be prefixed like GuiInit, NodeInit, …

ryanofsky commented at 0:26 am on November 13, 2020:

re: #19160 (review)

I think my main concern is about lighting the interfaces hierarchy, where they’re produced, and when they’re consumed. See my graphical representation proposal as a way to improve it.

LocalInit class is gone, so the hierarchy is flat now. Init class is now just used for initialization, and is exposed over IPC, but is no longer used to set up IPC.

in src/interfaces/init.h:55 in e99129343d outdated

50+//! 4. Spawned process calls IpcProcess::serve(), to read command line arguments
51+//!    and determine that it is a spawned process and what socketpair file
52+//!    descriptor it should use. The spawned process then calls
53+//!    IpcProtocol::serve() to handle incoming requests from the socketpair and
54+//!    invoke Init interface methods and eventually shut down and destroy the
55+//!    Init interface when the connection is closed.

ariard commented at 6:25 pm on October 15, 2020:

I think an in-depth walk-through needs to be documented somewhere for actual and future reviewers.

An example would be :

Bitcoin GUI process is started, in src/qt/bitcoin.cpp, it initializes its own interface LocalInit from src/interfaces/init_bitcoin-gui.cpp
From this LocalInit interface, it calls makeNode(), which trigger a child bitcoin-node spawning through IpcProcess.spawn. It also register hook cleaning through base.addCloseHook
From this LocalInit interface, it calls connect, which start a new thread capnp-loop with a libmultiprocess mp::EventLoop
Bitcoin Node is started with -ipcfd passing a socketpair for communications, it initializes its own interface LocalInit from src/interfaces/init_bitcoin-node.cpp
From this LocalInit interface, it calls serve() to start servicing Bitcoin GUI on its owned interface Chain. This is achieved through the abstract interface IpcProtocol which is currently Cap’n Proto (MakeCapnpProtocol)
When Bitcoin GUI request a Chain method like hasBlocks, this is redirected to mp::ProxyClient
From its own capnp-loop thread embedding a mp::ProxyServer, Bitcoin Node internal methods are called to fulfill the request

Okay it’s likely a fuzzy walkthrough but this reflects my understanding right now.

What do you think about joining an ASCII art ? I can write one, but I don’t know if graphical representation documentation has been favored by Core though it may be helpful for architecture-level stuff.

ryanofsky commented at 2:50 pm on October 21, 2020:

re: #19160 (review)

I think an in-depth walk-through needs to be documented somewhere for actual and future reviewers.

I added some more information about what happens when IPC methods are called in https://github.com/ryanofsky/bitcoin/blob/pr/ipc-echo/doc/multiprocess.md#ipc-implementation-details. Maybe we can add walkthrough you posted someplace, too. It seems like it could be expressed as a sequence diagram. I think it’s useful to see ordering of when functions are called there, and which functions call other functions.

What do you think about joining an ASCII art ? I can write one, but I don’t know if graphical representation documentation has been favored by Core though it may be helpful for architecture-level stuff.

Maybe a text representation that is easy to update but can also rendered as a diagram would be good. There are lots of tools that can generate sequence diagrams from simple text, but I haven’t used any recently enough to know which to recommend. http://www.mcternan.me.uk/mscgen/ is an older one, https://mermaid-js.github.io/mermaid/diagrams-and-syntax-and-examples/sequenceDiagram.html seems more modern and featureful, https://www.websequencediagrams.com/ is a simple online one

ariard commented at 5:41 pm on October 27, 2020:

Thanks for the new documentation in multiprocess.md.

For the text representation I was thinking a simple ASCII art underlying relations between interfaces, implementations, process and threads. Akin to RL’s ARCH.md, which is hopefully straightforward and doesn’t rely on a third-party tool.

I’m working on a proposal, if it will be easier to give your opinion.

ryanofsky commented at 6:38 pm on October 27, 2020:

re: #19160 (review)

I’m working on a proposal, if it will be easier to give your opinion.

Description at https://github.com/rust-bitcoin/rust-lightning/blob/main/ARCH.md seems good, but I don’t get the diagram. It seems kind of like a word cloud or mind map useful for memorization more than a picture which explains something. I tend to be more of a verbal than a visual person, so I wouldn’t come up with something like this, but wouldn’t mind including something like it.

in src/interfaces/init.h:80 in e99129343d outdated

73+    LocalInit(const char* exe_name, const char* log_suffix);
74+    ~LocalInit() override;
75+    std::unique_ptr<Echo> makeEcho() override;
76+    //! Make echo implementation for `echoipc` test RPC. Spawn new process if
77+    //! supported.
78+    virtual std::unique_ptr<Echo> makeEchoIpc();

ariard commented at 6:28 pm on October 15, 2020:

I think you could add a temporary comment hinting this interface as the future location of makeNode, makeWalletClient, etc, it’s more helpful for reviewers to understand what it would look like after #10102.

ryanofsky commented at 2:50 pm on October 21, 2020:

re: #19160 (review)

I think you could add a temporary comment hinting this interface as the future location of makeNode, makeWalletClient, etc, it’s more helpful for reviewers to understand what it would look like after #10102.

Good idea, added this.

in src/interfaces/README.md:18 in e99129343d outdated

11@@ -12,6 +12,8 @@ The following interfaces are defined here:
12 
13 * [`Handler`](handler.h) — returned by `handleEvent` methods on interfaces above and used to manage lifetimes of event handlers.
14 
15-* [`Init`](init.h) — used by multiprocess code to access interfaces above on startup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102).
16+* [`Init`](init.h) — used by multiprocess code to access interfaces above on startup. Added in [#19160](https://github.com/bitcoin/bitcoin/pull/19160).
17+
18+* [`Base`](base.h) — base interface class used by multiprocess code for bookkeeping and cleanup. Added in [#19160](https://github.com/bitcoin/bitcoin/pull/19160).
19

ariard commented at 6:33 pm on October 15, 2020:

It would be great to extend the README with a new section explaining how someone would add a new IpcProtocol implementation. As of right now src/interfaces/capnp/init.capnp is pretty empty which makes it hard to understand for reviewers without looking first on #10102 branch.

Purely reading libmultiprocess README, I didn’t grasp at first than one IpcProtocol must define a ProxyClient/ProxyServer` pair for each supported interface.

ryanofsky commented at 6:36 pm on October 20, 2020:

re: #19160 (review)

It would be great to extend the README with a new section explaining how someone would add a new IpcProtocol implementation. As of right now src/interfaces/capnp/init.capnp is pretty empty which makes it hard to understand for reviewers without looking first on #10102 branch.

I added an overview of the IPC implementation and more information about what happens when IPC methods are called in https://github.com/ryanofsky/bitcoin/blob/pr/ipc-echo/doc/multiprocess.md#ipc-implementation-details.

I’m not sure what change is suggested by the observation about init.capnp. I hope it’s clear that things in the src/interfaces/capnp/ directory are specific to the capnp protocol. I hope it’s also clear that c++ interfaces in the src/interfaces/ and just abstract classes with pure virtual methods, and they can be implemented in any way to make calls with any protocol.

The IpcProtocol interface is a simple interface with 2 methods: connect and serve that is heavily documented and defined in src/interfaces/ipc.h. In #19460 a third listen method is added to support listening for incoming connections on a socket, and not just communicating with the parent process.

Purely reading libmultiprocess README, I didn’t grasp at first than one IpcProtocol must define a ProxyClient/ProxyServer` pair for each supported interface.

It’s not the case that an IPC protocol needs to be implemented with pairs of client and server classes. The server classes that are generated are specific to capnp, and the Client class naming was also just chosen to be consistent with capnp names. If I were implementing a custom protocol, I wouldn’t write any proxy server classes. I would write socket code that calls implementation methods directly, not socket code that calls proxy server class methods that call implementation class methods. If I were implementing a non-capnp protocol, I probably wouldn’t use the client naming either. For example, if I were implementing a protocol for the Wallet interface, I’d make use of the existing WalletImpl class implementation in the local process and name the implementation that does remote IPC RemoteWalletImpl.

ariard commented at 6:08 pm on October 27, 2020:

I’m not sure what change is suggested by the observation about init.capnp. I hope it’s clear that things in the src/interfaces/capnp/ directory are specific to the capnp protocol.

I think it’s clearer for me now about the distinction between abstract interfaces and their implementations. IMO, three further improvements to make it better:

rename IpcProtocolImpl to CapnpProtocol, the constructor is already called MakeCapnpProtocol and IpcProcessImpl to LibmpProcess. Or any more accurate names hinting the specific implementation
document IpcProtocol/IpcProcess in src/interfaces/README.md, underscoring the current available implementation IpcProtocolImpl/IpcProcessImpl for each of them
reorganize directory to src/interfaces/capnp/ src/interfaces/ipcproto/capnp underscoring that capnp is only one implementation among potential several ones.

It’s not the case that an IPC protocol needs to be implemented with pairs of client and server classes.

I did looked again on the #10102 and this point is more straightforward to understand. That said, for this current PR, I believe it will helper reviewers to make a reference on how pairs of clients and server are added for capnp implementation.

I don’t know if a src/interfaces/capnp/README.md with implementation details would help but for example I still don’t grasp how .capnp files are consumed by the code generator. I might need to read libmultiprocess/README.md more attentively though.

ryanofsky commented at 6:42 pm on October 27, 2020:

re: #19160 (review)

IMO, three further improvements to make it better:

rename IpcProtocolImpl to CapnpProtocol, the constructor is already called MakeCapnpProtocol and IpcProcessImpl to LibmpProcess. Or any more accurate names hinting the specific implementation

Renamed CapnpProtocol, but kept ProcessImpl because the process implementation happens to make a few libmultiprocess helper calls but is not focused around libmultiprocess (it could call fork/exec instead of mp::SpawnProcess for example). It might help to look at the complete ProcessImpl class in https://github.com/ryanofsky/bitcoin/blob/ipc-export/src/ipc/process.cpp which supports establishing connections between existing processes instead of just basic spawning implemented here.

I was thinking of renaming ProcessImpl to UnixProcess and having an alternate WindowsProcess implementation, and maybe I will do this but currently I’m not sure how much platform specific code there will be here. It’s possible more platform specific code will move into libmultiprocess and then it would make sense to call this LibmpProcess. But if there are significant differences between unix sockets and windows pipes, having two classes instead of one would make more sense. For now ProcessImpl like NodeImpl ChainImpl WalletImpl is pretty straightforward, I think.

document IpcProtocol/IpcProcess in src/interfaces/README.md, underscoring the current available implementation IpcProtocolImpl/IpcProcessImpl for each of them

IPC classes are moved out of src/interfaces/ to src/ipc/ so hopefully this is clearer. I think all the classes have pretty good doxygen documentation and organization is straightforward. src/ipc/ is just a normal directory like src/wallet/ or src/qt/ grouping related classes and functions.

reorganize directory to src/interfaces/capnp/ src/interfaces/ipcproto/capnp underscoring that capnp is only one implementation among potential several ones.

Thanks, renamed to src/ipc/capnp/

It’s not the case that an IPC protocol needs to be implemented with pairs of client and server classes.

I did looked again on the #10102 and this point is more straightforward to understand. That said, for this current PR, I believe it will helper reviewers to make a reference on how pairs of clients and server are added for capnp implementation.

Now eliminated more uses of “client classes”, switching to “proxy classes” instead. The client/server terminology is more specific to capnp and less helpful to talk about generically. In particular proxy server classes are a capnp-specific detail and I doubt a different protocol implementation would even have wrapper classes on the server side.

I don’t know if a src/interfaces/capnp/README.md with implementation details would help but for example I still don’t grasp how .capnp files are consumed by the code generator. I might need to read libmultiprocess/README.md more attentively though.

Maybe it would be helpful to describe generated files. This is a low level detail, though, and not something you need to care about implementing an IPC interface or calling one. The idea is that to call an interface from a different process, all you need to do is describe it, and the proxy code is generated from the description. The generated files should be easy to browse and are all named like src/ipc/capnp/*.capnp.*

re: #19160 (comment)

@ryanofsky See my graphical representation proposal here : https://github.com/ariard/bitcoin/tree/ipc-echo-review/src/interfaces..

Joint with a walk-through for both the interfaces initialization and a message round-trip.

I don’t know actually how to understand that diagram. I don’t know what the arrows mean, and I’m not sure it’s good for it to mix low level and high level details together. For example, the capnp event loop thread seems a low-level implementation detail. I’m not sure why you would care if you’re just connecting or calling a proxy method whether capnp does the socket communication on the calling thread or a different event loop thread.

But I wouldn’t be opposed to including a diagram like this as an illustration if it’s helpful

ariard commented at 6:42 pm on October 15, 2020: member

Back to this after 0.21 high-priority reviews, I’m really eager to help to make this more ready-to-review once branch split-off is reached :)

I started again reviewing from scratch as I didn’t understand anymore all the new interfaces/abstract class inter-dependencies (Init, IpcProtocol, IpcProcess, linking with libmultiprocess). I think the review challenge is to write-up some detailed walkthrough giving a high-level picture of how every component is working when multiple processes are effectively used. See my specific comment for a starter, likely it could be in src/interfaces/README.md ?

Note, following the current #Installation section from doc/multiprocess doesn’t work on Debian 10.2, the generared Makefile doesn’t incorporate a bitcoin-node target. I’m inquiring on this.

in src/interfaces/init.cpp:42 in 2f521b2c24 outdated

37+    int fd = process.spawn(new_exe_name, pid);
38+    std::unique_ptr<Init> init = protocol.connect(fd);
39+    Base& base = make_client(*init);
40+    base.addCloseHook(MakeUnique<CloseFn>([&process, new_exe_name, pid] {
41+        int status = process.wait(pid);
42+        LogPrint(::BCLog::IPC, "%s pid %i exited with status %i\n", new_exe_name, pid, status);

Sjors commented at 6:59 pm on October 15, 2020:

Might be worth logging the pid on launch as well.

ryanofsky commented at 2:50 pm on October 21, 2020:

re: #19160 (review)

Might be worth logging the pid on launch as well.

Good idea, added log

Sjors commented at 7:10 pm on October 15, 2020: member

Thanks for adding the echoipc RPC; it should make it easier to wrap my head around stuff.

On macOS I’m having difficulty compiling (with --enable-werror; otherwise they’re just warnings) when using capnp 0.8.0 via homebrew and a freshly built libmultiprocess, e.g. error: parameter 'connection' shadows member inherited from type 'InvokeContext'. Here’s a log.

ryanofsky force-pushed on Oct 21, 2020

ryanofsky commented at 7:55 pm on October 21, 2020: contributor

Updated e99129343d137410827ec7c26ae48a995e0cf1f8 -> ee7b3cdc72df06ce674a83b980cfb7e6ef657fc6 (pr/ipc-echo.17 -> pr/ipc-echo.18, compare) with suggested documentation and log updates

ariard commented at 5:49 pm on October 28, 2020: member

@ryanofsky See my graphical representation proposal here : https://github.com/ariard/bitcoin/tree/ipc-echo-review/src/interfaces..

Joint with a walk-through for both the interfaces initialization and a message round-trip.

ryanofsky force-pushed on Nov 25, 2020

ryanofsky commented at 3:26 pm on November 25, 2020: contributor

In response to feedback, I did a big reorganization of this PR and will also update the other PRs building on this. The most noticeable changes are moves and renames: src/interfaces/ directory is now now more minimal and now only contains interface definitions, with functionality that was previously in src/interfaces/ is moved into src/node/ and src/wallet/ and new src/init/ and src/ipc/ directories. The confusing LocalInit class is now gone and IPC setup happens explicitly through interfaces::Ipc calls in node, wallet, and RPC code instead of the previous confusing approach where the Init class was responsible for setting up IPC connections, and it was less obvious which objects were remote and local. There are also a lot of other cleanups and documentation improvements, and a rebase on new base PR #20494

Rebased ee7b3cdc72df06ce674a83b980cfb7e6ef657fc6 -> a3d7a9864b16432b567a1c31613e5e5fe9eb1231 (pr/ipc-echo.18 -> pr/ipc-echo.19, compare)

MarcoFalke referenced this in commit 24e4857b29 on Dec 1, 2020

ryanofsky force-pushed on Dec 8, 2020

DrahtBot added the label Needs rebase on Dec 8, 2020

ryanofsky force-pushed on Dec 8, 2020

DrahtBot removed the label Needs rebase on Dec 8, 2020

ryanofsky commented at 9:36 pm on December 8, 2020: contributor

Rebased a3d7a9864b16432b567a1c31613e5e5fe9eb1231 -> 5951eea6753433c20d690fb0d4716dd3eeff4ab4 (pr/ipc-echo.19 -> pr/ipc-echo.20, compare) after base PR #20494 merge with include fixes to fix appveyor “use of undefined type ‘interfaces::Echo’” failure https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/36507325

Rebased 5951eea6753433c20d690fb0d4716dd3eeff4ab4 -> 9de6e8d7d9b2bb48045d82690ae5e57471e6d5fb (pr/ipc-echo.20 -> pr/ipc-echo.21, compare) due to conflict with #19425 with build fix for appveyor “MSB8027: Two or more files with the name of bitcoind.cpp” failure https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/36723264, and libmultiprocess update https://github.com/chaincodelabs/libmultiprocess/pull/40 to fix cirrus “-Werror=suggest-override” failure https://cirrus-ci.com/task/6000489311502336?command=ci#L4294

Rebased 9de6e8d7d9b2bb48045d82690ae5e57471e6d5fb -> 6e6fc35540c6e1eb1f5c8c001be8a89de7f317b7 (pr/ipc-echo.21 -> pr/ipc-echo.22, compare) after base PR #20046 merge

Rebased 6e6fc35540c6e1eb1f5c8c001be8a89de7f317b7 -> e9f438870169e04edda15badf8c4d0f174e32970 (pr/ipc-echo.22 -> pr/ipc-echo.23, compare) due to conflict with #20605

fanquake referenced this in commit 17918a987a on Dec 10, 2020

sidhujag referenced this in commit 7512d370e3 on Dec 10, 2020

ryanofsky force-pushed on Dec 11, 2020

DrahtBot added the label Needs rebase on Dec 16, 2020

ryanofsky force-pushed on Dec 18, 2020

DrahtBot removed the label Needs rebase on Dec 18, 2020

jamesob commented at 3:22 am on January 14, 2021: member

Concept ACK. Will start review tomorrow evening.

ryanofsky commented at 6:03 pm on January 16, 2021: contributor

Took a fresh look at this (thanks for prompting, James!) and had some ideas for making this easier to review:

I think I can entirely drop “multiprocess: Add interfaces base class with addCloseHook method” commit 543070ea18a501cd2bb895bc3ec062ffc473891b
I can drop the little stub files in the “multiprocess: Add echoipc RPC method and test” commit e9f438870169e04edda15badf8c4d0f174e32970. These are customization points used for more complicated #10102 node/wallet/gui interactions not needed for the simple echo method call
I will to split up the main “Add basic spawn and IPC support” commit fe0a68197aed9f2df05ab3a051b3f3aa3f630e88 some more, particularly moving the long documentation comments out to a separate commit. They bog down and hide the actual changes too much making them seem much more weighty than they are, going too deep into details of each component before you see all the components.

jonatack commented at 6:09 pm on January 16, 2021: contributor

Yes, been meaning to get to this for a while and will do so ASAP.

dongcarl commented at 9:46 pm on January 22, 2021: contributor

Light Code-Review ACK e9f438870169e04edda15badf8c4d0f174e32970

Nit: I’m not 100% sure how to approach this, but perhaps there’s a more ergonomic way to get the final interface out from spawnProcess than doing “out-param-through-lamdba-capturing”?

Thanks to ryanofsky’s offline help, I wrote down some notes on how this all works, hope it helps other reviewers (please also let me know if I got anything wrong!)

Parent’s Perspective

When a parent node starts up, it constructs (using interfaces::MakeNodeInit) an interfaces::Init that is placed in NodeContext.init. The parent node can then call spawnProcess on the newly constructed interfaces::Init’s interfaces::Ipc to:

Start a child process
Create an interface for the parent to communicate with said child

The default implementation for interfaces::Ipc::spawnProcess works by utilizing:

ipc::Process::connect, which encapsulates the logic needed to
1. Start the child process
2. Create a protocol-neutral file descriptor for the parent to communicate with said child process
ipc::Protocol::connect, which encapsulates the logic needed to
1. Create an interface for the parent to communicate with said child from the protocol-neutral file descriptor

Child’s perspective

When the child is spawned by the parent calling ipc::Process::connect as described above, the child also calls interfaces::MakeNodeInit to construct an interfaces::Init.

What’s important here is that interfaces::MakeNodeInit calls serveProcess (yin to spawnProcess’s yang) on its newly constructed interfaces::Init before returning. This serveProcess behaves differently when called by a parent process vs. a child process:

In the case of a parent process, this immediately returns false and the newly constructed interfaces::Init is placed in NodeContext.init for later use (described above).
In the case of a spawned child process, this serveProcess will block, listen, and respond to calls from the parent process on a loop.

Note: serveProcess distinguished between the parent vs. child by checking for an -ipcfd flag on the command line, which is added when a parent process spawns a child process in ipc::Process::spawn.

The default implementation for interfaces::Ipc::serveProcess work by utilizing:

ipc::Process::serve, which encapsulates the logic needed to
1. Parse the -ipcfd flag for the protocol-neutral file descriptor on which the parent will send requests
2. Call ipc::Protocol::serve, which encapsulates the logic needed to
  1. Respond to requests from the parent process on said file descriptor
3. Capture the correct exit status from ipc::Protocol::serve and bubble it up

A few snippets that were elucidating for me:

DrahtBot added the label Needs rebase on Jan 28, 2021

laanwj added this to the "Blockers" column in a project

ryanofsky referenced this in commit 35d2091134 on Jan 28, 2021

ryanofsky force-pushed on Jan 28, 2021

ryanofsky commented at 9:19 pm on January 28, 2021: contributor

Rebased due to conflict, also made all the cleanups suggested #19160 (comment) and was able to get rid of confusing lambda @dongcarl pointed out #19160 (comment)

Rebased e9f438870169e04edda15badf8c4d0f174e32970 -> f08e238945342151d39e18d577cd6a94603e4397 (pr/ipc-echo.23 -> pr/ipc-echo.24, compare) due to conflict with #20012, also making various cleanups

DrahtBot removed the label Needs rebase on Jan 28, 2021

jonatack commented at 5:06 pm on February 2, 2021: contributor

Seeing the following issue when attempting to build f08e238945342151d39e18d577cd6a94603e4397 with gcc 10 and --enable-multiprocess:

0  GEN      ipc/capnp/echo.capnp.c++
1ipc/capnp/echo.capnp:11:8-15: error: 'Proxy' has no member named 'include'
2ipc/capnp/echo.capnp:12:8-15: error: 'Proxy' has no member named 'include'
3terminate called after throwing an instance of 'std::runtime_error'
4  what():  Invoking /usr/bin/capnp failed
5make[2]: *** [/usr/local/include/mpgen.mk:4: ipc/capnp/echo.capnp.c++] Aborted
6make[2]: *** Waiting for unfinished jobs....

It builds fine without --enable-multiprocess

I then followed the instructions in doc/multiprocess.md…

0cd <BITCOIN_SOURCE_DIRECTORY>
1make -C depends NO_QT=1 MULTIPROCESS=1
2./configure --prefix=$PWD/depends/x86_64-pc-linux-gnu
3make

and at the make step, am seeing:

 0  CXX      ipc/capnp/libbitcoin_ipc_a-protocol.o
 1ipc/capnp/protocol.cpp:6:10: fatal error: ipc/capnp/init.capnp.h: No such file or directory
 2    6 | #include <ipc/capnp/init.capnp.h>
 3      |          ^~~~~~~~~~~~~~~~~~~~~~~~
 4compilation terminated.
 5make[2]: *** [Makefile:8366: ipc/capnp/libbitcoin_ipc_a-protocol.o] Error 1
 6make[2]: *** Waiting for unfinished jobs....
 7make[2]: Leaving directory '/home/jon/projects/bitcoin/bitcoin/src'
 8make[1]: *** [Makefile:15483: all-recursive] Error 1
 9make[1]: Leaving directory '/home/jon/projects/bitcoin/bitcoin/src'
10make: *** [Makefile:812: all-recursive] Error 1

Tried each step a couple times. Any tips?

jonatack commented at 5:16 pm on February 2, 2021: contributor

Ah, reinstalled manually per the instructions in https://github.com/chaincodelabs/libmultiprocess/blob/master/README.md and now this pull builds with --enable-multiprocess. I had already installed in September 2020 but it appears I needed to do it again.

ryanofsky commented at 5:49 pm on February 2, 2021: contributor

Thanks for testing the manual install! Note it’s also possible to test this with depends using the commands from doc/multiprocess.md#installation

dongcarl commented at 10:09 pm on February 5, 2021: contributor

Reviewed the diff https://github.com/ryanofsky/bitcoin/compare/pr/ipc-echo.23-rebase..pr/ipc-echo.24

Base interface removed
Added methods to make Node, Chain, WalletClient interfaces
Changed call semantic of Ipc::spawnProcess to return the Init interface
Added Ipc::addCleanup to attach cleanup function to interface

ryanofsky: Wondering what pr/ipc-echo.23-rebase..pr/ipc-echo.24#diff-a1848eb83b is for?

ryanofsky commented at 10:29 pm on February 5, 2021: contributor

ryanofsky: Wondering what pr/ipc-echo.23-rebase..pr/ipc-echo.24#diff-a1848eb83b is for?

The new annotations add #include statements to generated files. The annotations are defined at https://github.com/ryanofsky/libmultiprocess/blob/2ccc479f8666d680bd2c2b157554ec5a7ce39d33/include/mp/proxy.capnp#L10-L12 and are needed after https://github.com/chaincodelabs/libmultiprocess/pull/43. That PR stopped automatically adding #include lines to generated files to avoid the need for basically empty header files to be created in some cases.

in doc/multiprocess.md:18 in f08e238945 outdated

14@@ -15,7 +15,7 @@ Specific next steps after [#10102](https://github.com/bitcoin/bitcoin/pull/10102
15 
16 ## Debugging
17 
18-After [#10102](https://github.com/bitcoin/bitcoin/pull/10102), the `-debug=ipc` command line option can be used to see requests and responses between processes.
19+The `-debug=ipc` command line option can be used to see requests and responses between processes. As much as possible, calls between processes are meant to work the same as calls within a single process without adding limitations or requiring extra implementation effort. Processes communicate with each other by calling regular [C++ interface methods](../src/interfaces/README.md). Method arguments and return values are automatically serialized and sent between processes. Object references and `std::function` arguments are automatically tracked and mapped to allow invoked code to call back into invoking code at any time, and there is a 1:1 threading model where any thread invoking a method in another process has a corresponding thread in the invoked process responsible for executing all method calls from the source thread, without blocking I/O or holding up another calls, and using the same thread local variables, locks, and callbacks between calls. The forwarding, tracking, and threading is implemented inside the [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) library which has the design goal of making calls between processes look exactly like calls in the same process to the extent possible.

ariard commented at 3:21 pm on February 12, 2021:

Starting at “As much as possible…”, I think you can move the remainder at the beginning of the README or its in own “Design” section.

ryanofsky commented at 2:52 pm on February 18, 2021:

re: #19160 (review)

Starting at “As much as possible…”, I think you can move the remainder at the beginning of the README or its in own “Design” section.

Thanks! Moved to new section.

in src/interfaces/init.h:43 in f08e238945 outdated

33+//! indicates that this is a child process spawned to handle requests from a
34+//! parent process, this blocks and handles requests, then returns null and a
35+//! status code to exit with. If this returns non-null, the caller can just
36+//! start up normally and use the Init object to spawn and connect to other
37+//! processes while it is running.
38+std::unique_ptr<Init> MakeNodeInit(NodeContext& node, int argc, char* argv[], int& exit_status);

ariard commented at 4:16 pm on February 12, 2021:

Reading this comment and the call chain IpcImpl::serveProcess -> ProcessImpl::serve, I don’t grasp what’s the expected behavior.

AFAIU bitcoin-node, it should only be called with ipcfd=$FD, spawned by the parent process. Any other case should be a hint of an error in `bitcoin-node usage ?

If bitcoin-node behavior is only regular when it’s called with ipcfd, why this comment is saying “If the argv indicates that this is child process…then returns null and a status code to exit with ? To me, this is expected behavior and shouldn’t be subject to early exit.

Further, the code sounds to do something different.

If this process is called without ipcfd, ProcessImpl::serve will returns false, the same for IpcImpl and finally MakeNodeInit will return a init::BitcoinNodeInit to main, pursuing process operations further.

If this process is called with ipcfd with an valid fd, ProcessImpl::serve will return true, the same for IpcImpl::serveProcess and finally MakeNodeInit will return a nullptr to main, provoking early exit of the process. Note, the return values are the same if the ipcfd is called with an invalid fd.

I think this comment should detail what’s a valid invocation of bitcoin-node, the pathological cases and what MakeNodeInit will return for each.

ryanofsky commented at 2:54 pm on February 18, 2021:

re: #19160 (review)

Reading this comment and the call chain IpcImpl::serveProcess -> ProcessImpl::serve, I don’t grasp what’s the expected behavior.

AFAIU bitcoin-node, it should only be called with ipcfd=$FD, spawned by the parent process. Any other case should be a hint of an error in `bitcoin-node usage ?

I think all of this should be more clear after #10102, but bitcoin-node is a drop-in replacement for bitcoind that just uses multiple processes internally instead of a single process. bitcoin-node takes all the same arguments that bitcoind takes and can be used in the same ways. (It also gains a new -ipcbind argument in #19460.)

If bitcoin-node is started with -ipcfd=$FD, it’s not a normal invocation, but it’s not a “pathological case” either. It’s just a special invocation that lets the interfaces::Ipc::startSpawnedProcess() method detect if it’s being called from a spawned process.

in src/ipc/interfaces.cpp:50 in f08e238945 outdated

45+            int status = m_process->wait(pid);
46+            LogPrint(::BCLog::IPC, "Process %s pid %i exited with status %i\n", exe_name, pid, status);
47+        });
48+        return init;
49+    }
50+    bool serveProcess(const char* exe_name, int argc, char* argv[], int& exit_status) override

ariard commented at 5:01 pm on February 12, 2021:

I think you can drop arg and argv parameters, they’re already passed in constructors, and those ones should make authority if they differ ?

ryanofsky commented at 2:35 pm on February 18, 2021:

re: #19160 (review)

I think you can drop arg and argv parameters, they’re already passed in constructors, and those ones should make authority if they differ ?

Thanks! Dropped the constructor arguments and removed more stored state from these classes.

in src/ipc/protocol.h:35 in f08e238945 outdated

30+    //! Socket communication is handled on a background thread.
31+    virtual std::unique_ptr<interfaces::Init> connect(int fd) = 0;
32+
33+    //! Handle requests on provided socket descriptor. Socket communication is
34+    //! handled on the current thread. This blocks until the client closes the socket.
35+    virtual void serve(int fd) = 0;

ariard commented at 5:14 pm on February 12, 2021:

Can we rename this method differently from Process:serve, even if currently the Process one is calling the Protocol one that’s a bit confusing. Like handle_request ?

ryanofsky commented at 2:51 pm on February 18, 2021:

re: #19160 (review)

Can we rename this method differently from Process:serve, even if currently the Process one is calling the Protocol one that’s a bit confusing. Like handle_request ?

I renamed the other method if that helps. I don’t like using the name “handle” here because “handleXYZ” methods are used in other interfaces to register event handlers. Also, this interface is extended in #19460 to have connect / serve / listen methods, which should be pretty common terms dealing with socket connections.

in src/ipc/process.h:33 in f08e238945 outdated

28+    //! Wait for spawned process to exit and return its exit code.
29+    virtual int wait(int pid) = 0;
30+
31+    //! Serve requests if current process is a spawned subprocess. Blocks until
32+    //! socket for communicating with the parent process is disconnected.
33+    virtual bool serve(int& exit_status) = 0;

ariard commented at 5:21 pm on February 12, 2021:

For each method of this interface it might be good to prefix them by which side of the process relationship it’s expected to be used, parent_spawn, parent_wait, spawn_serve.

But I don’t think it makes sense to split it further because we might have in the future set of process with grandchild relationship bitcoin-gui -> bitcoin-wallet -> bitcoin-keyholder ?

ryanofsky commented at 2:36 pm on February 18, 2021:

re: #19160 (review)

For each method of this interface it might be good to prefix them by which side of the process relationship it’s expected to be used, parent_spawn, parent_wait, spawn_serve.

I don’t think I understand the reason for this suggestion. Maybe there is something misleading about the word spawn? The naming here comes from the POSIX spawn API (https://man7.org/linux/man-pages/man3/posix_spawn.3.html). Any process that spawns another process is a parent, and any process that is spawned is a child. A process can be both a parent and a child. When bitcoin-gui spawns bitcoin-node which spawns bitcoin-wallet in #10102, bitcoin-node is a both a parent and a child.

ariard commented at 4:47 pm on February 26, 2021:

New documentation is better, reading them I grasped those methods usage at first read!

ariard commented at 5:23 pm on February 12, 2021: member

Thanks to reworking this PR, it’s much much clearer! I still want to test it manually, maybe suggest doc improvement and to look again at libmultiprocess before to ACK.

dongcarl commented at 8:30 pm on February 16, 2021: contributor

A few more notes from reviewing the code today:

Trivial implementations found in src/interfaces/init.cpp for interfaces::Init methods are required so that classes which inherit from interfaces::Init won’t be considered abstract classes and can be allocated and returned in interfaces::MakeNodeInit.
Polymorphism for interfaces::MakeNodeInit (e.g. whether the one in src/init/bitcoind.cpp is called or the one in src/init/bitcoin-node.cpp is called) is achieved through the build system: https://github.com/bitcoin/bitcoin/blob/f08e238945342151d39e18d577cd6a94603e4397/src/Makefile.am#L626 https://github.com/bitcoin/bitcoin/blob/f08e238945342151d39e18d577cd6a94603e4397/src/Makefile.am#L632

ryanofsky force-pushed on Feb 18, 2021

ryanofsky commented at 0:31 am on February 19, 2021: contributor

Thanks Antoine and Carl for review! Made a few tweaks based on comments.

Updated f08e238945342151d39e18d577cd6a94603e4397 -> 5e55c5a29633c19df2008653bf9285b338ff1088 (pr/ipc-echo.24 -> pr/ipc-echo.25, compare) with changes based on suggestions.

re: #19160 (comment)

A few more notes from reviewing the code today:

Trivial implementations found in src/interfaces/init.cpp for interfaces::Init methods are required so that classes which inherit from interfaces::Init won’t be considered abstract classes and can be allocated and returned in interfaces::MakeNodeInit.

Polymorphism for interfaces::MakeNodeInit (e.g. whether the one in src/init/bitcoind.cpp is called or the one in src/init/bitcoin-node.cpp is called) is achieved through the build system

Curious if you would suggest changes to interfaces/init.h to make this more clear. The Make function declarations at the bottom are meant to suggest the different init implementations.

Also, to be sure, interfaces::Init methods could all be abstract. Making them not abstract just avoids having to repeatedly define the same make methods in subclasses that all return null. This avoids extra boilerplate in #10102 which has BitcoinNodeInit, BitcoinWalletInit, BitcoinGuiInit, BitcoinQtInit, and BitcoindInit

ryanofsky force-pushed on Feb 19, 2021

ryanofsky commented at 6:13 pm on February 19, 2021: contributor

Updated 5e55c5a29633c19df2008653bf9285b338ff1088 -> b2abe3f062c69663ae97bed239aa0a118e6a081c (pr/ipc-echo.25 -> pr/ipc-echo.26, compare) with a few more tweaks, mostly comment changes.

in doc/multiprocess.md:58 in b2abe3f062 outdated

53+classes for this purpose that are thin wrappers around Cap'n Proto
54+[client](https://capnproto.org/cxxrpc.html#clients) and
55+[server](https://capnproto.org/cxxrpc.html#servers) classes, which handle the
56+actual serialization and socket communication.
57+
58+## Design

ariard commented at 3:07 pm on February 26, 2021:

From a UX standpoint, what do you think about the following order of sections : Design - IPC implementation - Installation - Debugging - Next steps. I would say before to be interested in upcoming next steps a reader might be willingly to grash what multiprocess is about.

ryanofsky commented at 4:51 pm on March 9, 2021:

re: #19160 (review)

From a UX standpoint, what do you think about the following order of sections : Design - IPC implementation - Installation - Debugging - Next steps. I would say before to be interested in upcoming next steps a reader might be willingly to grash what multiprocess is about.

Combined implementation details and design sections for now. I wouldn’t mind improving this document in a different PR, but at most I want to add a little new text in this PR, not reorganize docs here.

On the idea, this document is mainly targeted at users, not developers so I wouldn’t put design information up front. I think a good organization might be:

Multiprocess bitcoin - description what multiprocess binaries currently do Next steps - description of what multiprocess binaries are meant to do and will do when next PRs are merged Installation - steps to build and run Debugging - options to monitor behavior Implementation details - pointers for developers

in src/ipc/interfaces.cpp:65 in b2abe3f062 outdated

60+    void addCleanup(std::type_index type, void* iface, std::function<void()> cleanup) override
61+    {
62+        m_protocol->addCleanup(type, iface, std::move(cleanup));
63+    }
64+    const char* m_exe_name;
65+    const char* m_arg0;

ariard commented at 3:27 pm on February 26, 2021:

AFAICT, this is used to indicate parent process path to mp::SpawnProcess and spawns the new child under the same one. What about calling this variable m_process_path or m_path, more meaningful than m_arg0 ?

ryanofsky commented at 4:53 pm on March 9, 2021:

re: #19160 (review)

AFAICT, this is used to indicate parent process path to mp::SpawnProcess and spawns the new child under the same one. What about calling this variable m_process_path or m_path, more meaningful than m_arg0 ?

Thanks, renamed to m_process_argv0. This code is really just trying to pass argv[0] along without assuming it has any value, so did avoid referring to paths still.

in src/interfaces/README.md:15 in b2abe3f062 outdated

11@@ -12,6 +12,6 @@ The following interfaces are defined here:
12 
13 * [`Handler`](handler.h) — returned by `handleEvent` methods on interfaces above and used to manage lifetimes of event handlers.
14 
15-* [`Init`](init.h) — used by multiprocess code to access interfaces above on startup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102).
16+* [`Init`](init.h) — used by multiprocess code to access interfaces above on startup. Added in [#19160](https://github.com/bitcoin/bitcoin/pull/19160).

ariard commented at 3:35 pm on February 26, 2021:

Should the new src/interfaces/ipc.h be mentioned ?

ryanofsky commented at 4:52 pm on March 9, 2021:

re: #19160 (review)

Should the new src/interfaces/ipc.h be mentioned ?

Thanks, added to list

ariard commented at 4:43 pm on February 26, 2021: member

Code and documentation sounds good to me. Still want to review libmultiprocess internals one more time before to ACK.

in src/ipc/capnp/echo.capnp:1 in b2abe3f062 outdated

0@@ -0,0 +1,17 @@
1+# Copyright (c) 2021 The Bitcoin Core developers

jamesob commented at 10:16 pm on February 28, 2021:

b2abe3f062c69663ae97bed239aa0a118e6a081c

Apologies for the remedial question, but this code isn’t autogenerated, right?

ryanofsky commented at 12:53 pm on March 4, 2021:

re: #19160 (review)

this code isn’t autogenerated, right?

Right, echo.capnp isn’t a generated file, it’s the source of generated files: echo.capnp.h, echo.capnp.cpp, and so on. Generated files aren’t committed to git, they are just build outputs.

For an interface to be shared across processses, it needs two definitions:

An API definition declaring how the interface is called. Examples: node.h, wallet.h, echo.h, init.h
A data definition declaring how interface calls get sent across the wire. Examples: node.capnp, wallet.capnp, echo.capnp, init.capnp

Since API and data definitions contain some of the same information, they have to be kept in sync manually when methods or parameters change. This is designed to be straightforward and safe. Straightforward because there is no need to write manual serialization code or use awkward intermediate types like UniValue instead of native types. Safe because if there are any inconsistencies between API and data definitions (even minor ones like using a narrow int data type for a wider int API input), there are errors at build time instead of errors or bugs at runtime.

In the future, it would be possible to combine API and data definitions together using C++ attributes. To do this we would add attributes to the API definition files, and then generate the data definitions from the API definitions and attributes. I didn’t take this approach mostly because it would be extra work, but also because until c++ standardizes reflection, this would require either hooking into compiler APIs like https://github.com/RosettaCommons/binder, or parsing c++ code manually like http://www.swig.org/.

I’ll think about where I can document this stuff better. There’s a https://github.com/chaincodelabs/libmultiprocess/#example section where some of this may be appropriate.

fjahr commented at 7:05 pm on March 29, 2021:

For an interface to be shared across processses, it needs two definitions:

An API definition declaring how the interface is called. Examples: node.h, wallet.h, echo.h, init.h

A data definition declaring how interface calls get sent across the wire. Examples: node.capnp, wallet.capnp, echo.capnp, init.capnp

Since API and data definitions contain some of the same information, they have to be kept in sync manually when methods or parameters change. This is designed to be straightforward and safe. Straightforward because there is no need to write manual serialization code or use awkward intermediate types like UniValue instead of native types. Safe because if there are any inconsistencies between API and data definitions (even minor ones like using a narrow int data type for a wider int API input), there are errors at build time instead of errors or bugs at runtime.

This was helpful to me, a version of this info could go into doc/multiprocess.md IMO.

ryanofsky commented at 12:51 pm on March 30, 2021:

re: #19160 (review)

This was helpful to me, a version of this info could go into doc/multiprocess.md IMO.

That’s good to know. I’ll link to the PR when it’s posted, but I’m working on libmultiprocess documentation and adding a version of this information there.

EDIT: Added this documentation in #19160 (review)

jamesob commented at 10:31 pm on February 28, 2021: member

Took a read through the commits, which look very clean, but haven’t fully digested the innards of the change; as @ariard alluded to, the content here in large part resides within libmultiprocess. I’d like to give that a thorough read-through, and I think we should consider moving it in-tree going forward (apologies if that’s resurrecting a discussion that was already hashed out).

I’m going to build and test this locally and will report back with a more thorough review.

The code here is fairly simple and easy to follow, though I would recommend to reviewers that they read the documentation commit first (8e85076174) for some general context. Some of the classes and machinery are a little abstract and so benefit from some upfront background.

I’d like to emphasize here that this is an important project; it’s upstream of many changes that have the potential to substantially increase bitcoind’s resilience (e.g. decoupling consensus runtime from extraneous functionality, pluggable alternate transports). For that reason I hope to see continued review effort here, and will try to do a better job of that myself!

Review notes

29e574aed8 Update libmultiprocess library

Changes out hash for a newer libmultiprocess.
0ffd07f870 multiprocess: Add Ipc and Init interface definitions

Adds some abstract interfaces for spawning and cleaning up processes (essentially just wrappers around libmultiprocess machinery) as well as some specific init interfaces for making node, chain, and wallet client processes.
e4cda24487 multiprocess: Add Ipc interface implementation

CapnpProtocol() - an Capnp-transport-specific implementation that can enqueue IPC requests, execute them, and communicate the results out to a file descriptor.
c80a46c20f multiprocess: Add bitcoin-node process spawning support

Adds an interfaces::Init implementation for spawning a bitcoin node process. Defines the headers for the wallet and GUI init implementations, but doesn’t specify the implementations for those.
8e85076174 multiprocess: Add comments and documentation

For anyone reviewing, probably worth reading this first.
b2abe3f062 multiprocess: Add echoipc RPC method and test

Nice commit that demonstrates implementations of a simple echo process that spawns and services requests, as well as a system test which (I assume for the moment) runs degenerately in a single process.

DrahtBot added the label Needs rebase on Mar 2, 2021

ryanofsky force-pushed on Mar 4, 2021

DrahtBot removed the label Needs rebase on Mar 4, 2021

ryanofsky commented at 2:18 pm on March 4, 2021: contributor

re: #19160#pullrequestreview-600311944

Thanks for review and summaries. I need to go through your feedback more and figure out how to update commit messages and documention.

I think we should consider moving it in-tree going forward (apologies if that’s resurrecting a discussion that was already hashed out).

I’d like to move the libmultiprocess repo from the chaincode org to the bitcoin-core org, but preferably not make it part of the bitcoin repo. There are a few reasons. One reason is that there’s nothing bitcoin-specific about the project. It can be used by any c++ application or library that wants to easily call functions across processes or pass callable objects between processes. Another more petty reason is that it uses a modern build system which would be nice not to backport to autoconf. Would be doable though.

system test which (I assume for the moment) runs degenerately in a single process.

Yes, if you run rpc_misc.py by default it won’t test multiprocess support because bitcoind doesn’t have multiprocess support. But if you run BITCOIND=bitcoin-node test/functional/rpc_misc.py, the bitcoin-node process will spawn another bitcoin-node process internally to do the test. (Cirrus CI tests this in its multiprocess build).

Rebased b2abe3f062c69663ae97bed239aa0a118e6a081c -> e1d4b13d4ecb2755ed2bc33e1ed0ce6df393b90d (pr/ipc-echo.26 -> pr/ipc-echo.27, compare) due to conflict with #20685

dongcarl commented at 11:19 pm on March 5, 2021: contributor

Code Review ACK e1d4b13d4ecb2755ed2bc33e1ed0ce6df393b90d

Read through all the commits and played around with changing the code to convince myself this was correct.

One thing that I noticed through reviewing and re-reviewing is that it seems the abstractions are deliberately designed in a way such that if we really wanted to, we can switch between different transports and even completely replace libmultiprocess in the future.

in src/interfaces/init.h:33 in e1d4b13d4e outdated

28+public:
29+    virtual ~Init() = default;
30+    virtual std::unique_ptr<Node> makeNode();
31+    virtual std::unique_ptr<Chain> makeChain();
32+    virtual std::unique_ptr<WalletClient> makeWalletClient(Chain& chain);
33+    virtual std::unique_ptr<Echo> makeEcho();

ariard commented at 2:18 pm on March 9, 2021:

In fact those make methods could only be implemented on the subclasses. E.g makeEcho only on BitcoinNodeInit or makeNode only on BitcoinGuiInit ? What purpose does it serve to have default methods ?

ryanofsky commented at 4:52 pm on March 9, 2021:

re: #19160 (review)

In fact those make methods could only be implemented on the subclasses. E.g makeEcho only on BitcoinNodeInit or makeNode only on BitcoinGuiInit ? What purpose does it serve to have default methods ?

This is mainly for #10102. Having default methods avoids the need to implement 20 make overrides in #10102 that would mostly return null (20 = 4 make methods * 5 init classes). In this PR there are only 2 init classes, but #10102 adds more. As examples it would be tedious to force bitcoin-wallet init class to implement makeNode and makeChain methods or force bitcoin-gui init class to implement makeWalletClient and other make methods.

In general it is good to avoid default methods, because they can mask problems and be confusing, but in this case I think the advantages outweigh the disadvantages.

in src/interfaces/ipc.h:26 in e1d4b13d4e outdated

21+//!
22+//! When spawning a new process, the steps are:
23+//!
24+//! 1. The controlling process calls spawnProcess(), which calls
25+//!    ipc::Process::spawn(), which spawns a new process and returns a
26+//!    socketpair file descriptor for communicating with it. spawnProcess then

ariard commented at 2:21 pm on March 9, 2021:

interfaces::Ipc::spawnProcess() ?

ryanofsky commented at 4:52 pm on March 9, 2021:

re: #19160 (review)

interfaces::Ipc::spawnProcess() ?

Thanks, that’s more consistent. Made a similar tweak below too.

in src/ipc/capnp/init.capnp:5 in e1d4b13d4e outdated

0@@ -0,0 +1,20 @@
1+# Copyright (c) 2021 The Bitcoin Core developers
2+# Distributed under the MIT software license, see the accompanying
3+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+@0xf2c5cfa319406aa6;

ariard commented at 2:41 pm on March 9, 2021:

Those values are identifiers for Cap’n Proto right ? Curious on how do you select them ?

ryanofsky commented at 4:53 pm on March 9, 2021:

re: #19160 (review)

Those values are identifiers for Cap’n Proto right ? Curious on how do you select them ?

You need to run the capnp id command line tool to generate an id, see https://capnproto.org/capnp-tool.html, https://capnproto.org/language.html. IIRC, the ids have some checking so you do need the tool and can’t pick a completely random id.

ariard commented at 3:59 pm on March 9, 2021: member

Code Review ACK e1d4b13

I went through all the changes of this branch, tested them manually. Also did a light code review of libmultiprocess (d576d97). Of course the library could be documented further but a) in the future it can be decoupled from cap’n proto and b) it’s relatively small (~5300 LoC).

I’ve a WIP branch here on how future interfaces can be added and the multiprocess framework leveraged for future uses (pluggable alternative transports). So far, it’s convenient enough to use!

Feel free to take comments if you have to make other changes.

ryanofsky force-pushed on Mar 9, 2021

ryanofsky commented at 10:38 pm on March 9, 2021: contributor

re: #19160#pullrequestreview-607456445

Thanks for reviews! I love your altnet branch too (easy diff link)! Updated this PR with some of the new suggestions.

Updated e1d4b13d4ecb2755ed2bc33e1ed0ce6df393b90d -> 36f1fbf50a0f2c96dfd1f7fe84d407f39a1906bd (pr/ipc-echo.27 -> pr/ipc-echo.28, compare) with a few review suggestions

DrahtBot added the label Needs rebase on Mar 11, 2021

ariard commented at 3:24 pm on March 11, 2021: member

Code Review ACK 36f1fbf, thanks for taking the suggestions.

Needs rebase.

in src/init/bitcoin-node.cpp:37 in 36f1fbf50a outdated

32+} // namespace init
33+
34+namespace interfaces {
35+std::unique_ptr<Init> MakeNodeInit(NodeContext& node, int argc, char* argv[], int& exit_status)
36+{
37+    auto init = MakeUnique<init::BitcoinNodeInit>(node, argc > 0 ? argv[0] : "");

fanquake commented at 0:44 am on March 12, 2021:

Please use std::make_unique in new code.

dongcarl commented at 7:32 pm on March 12, 2021: contributor

re-ACK 36f1fbf50a0f2c96dfd1f7fe84d407f39a1906bd

Noticed changes (no apparent behavioural changes):

Documentation updates
Renamed arg0 parameter to process_arg0

jamesob commented at 3:30 pm on March 16, 2021: member

I’ve spent the last day or so reading through libmultiprocess (evidenced by a few breadcrumbs) and while I feel I have lexically parsed the contents of that repo and have found nothing obviously wrong or malicious, I certainly wouldn’t be able to recreate the work there and can’t attest to its correctness. The contents of, say, the type marshalling unit are out of my paygrade from a C++ knowhow standpoint so while the code seems fine (and certainly well designed by @ryanofsky), I cannot take a full accounting of its correctness or safety.

That said, --enable-multiprocess is still an opt-in, off-by-default feature and the approach taken using libmultiprocess is probably the least intrusive from a code standpoint in terms of mitigating the changes necessary to make multiprocess operation an option alongside the standard monolith process operation. Even if the approach taken in libmultiprocess is more general/complex than necessary in the longterm and even if we may be able to come up with a simpler mechanism to enable multiprocess coordination, I think getting multiprocess operation fleshed out and working is the really important thing at this point. So while I think it would be nice if some of the cpp gurus on this repo gave libmultiprocess a thorough once-over (if they haven’t already), I am ACK on using that library to proof out process separation.

Once we get process separation working and thoroughly tested, we can think about whether the generalized mechanisms for object (de)serialization and function calling in libmultiprocess are necessary in lieu of something simpler like message passing with a more specialized, straightforward protocol over a socket.

I hope to complete a final review and testing of this PR today.

jamesob approved

jamesob commented at 9:40 pm on March 16, 2021: member

ACK 36f1fbf50a0f2c96dfd1f7fe84d407f39a1906bd (jamesob/ackr/19160.1.ryanofsky.multiprocess_add_basic_s)

Needs rebase, but luckily it’s a pretty easy one.

97232e7c62 Update libmultiprocess library

Verified hash matches reviewed version of libmultiprocess. Verified curl -L https://github.com/chaincodelabs/libmultiprocess/archive/$(git rev-parse HEAD).tar.gz | sha256sum matches the expected hash.
547cb012ac multiprocess: Add Ipc and Init interface definitions
98981da508 multiprocess: Add Ipc interface implementation
daf366f6ef multiprocess: Add bitcoin-node process spawning support
d9274f0bd2 multiprocess: Add comments and documentation
36f1fbf50a multiprocess: Add echoipc RPC method and test

Manual testing

Cloned and built libmultiprocess independently; installed using cmake -DCMAKE_INSTALL_PREFIX=/home/james/.local ... Installation using sudo make install without the local prefix led to an error during bitcoin build time.

Had to then run ./configure with PKG_CONFIG_PATH=${HOME}/.local/lib/pkgconfig ./configure ... -L${HOME}/.local/lib ... -I${HOME}/.local/include ...

Built bitcoin, started bitcoin-node with preexisting pruned datadir. Operation as usual. Executed ./src/bitcoin-cli echoipc wowowow a few times:

 02021-03-16T21:33:45Z [httpworker.2] Process bitcoin-node pid 62867 launched
 12021-03-16T21:33:45Z [httpworker.2] {bitcoin-node-58227/b-httpworker.2-58239} IPC client first request from current thread, constructing waiter
 22021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client send Init.construct$Params (threadMap = <external capability>)
 32021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client recv Init.construct$Results (threadMap = <external capability>)
 42021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client send Init.makeEcho$Params (context = (thread = <external capability>, callbackThread = <external capability>))
 52021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client recv Init.makeEcho$Results (result = <external capability>)
 62021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client send Echo.echo$Params (context = (thread = <external capability>, callbackThread = <external capability>), echo = "wowowow")
 72021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client recv Echo.echo$Results (result = "wowowow")
 82021-03-16T21:33:45Z [httpworker.2]
 92021-03-16T21:33:45Z [httpworker.2] {bitcoin-node-58227/b-httpworker.2-58239} IPC client destroy N2mp11ProxyClientIN3ipc5capnp8messages4EchoEEE
102021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client send Echo.destroy$Params (context = (thread = <external capability>, callbackThread = <external capability>))
112021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client recv Echo.destroy$Results ()
122021-03-16T21:33:45Z [httpworker.2]
132021-03-16T21:33:45Z [httpworker.2] {bitcoin-node-58227/b-httpworker.2-58239} IPC client destroy N2mp11ProxyClientIN3ipc5capnp8messages4InitEEE
142021-03-16T21:33:45Z [httpworker.2] Process bitcoin-node pid 62867 exited with status 0

0Tested on Linux-5.10.0-4-amd64-x86_64-with-glibc2.29
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ -L/home/james/.local/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ -I/home/james/.local/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default --enable-multiprocess
3
4Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2  i
5
6Compiler version:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 36f1fbf50a0f2c96dfd1f7fe84d407f39a1906bd ([`jamesob/ackr/19160.1.ryanofsky.multiprocess_add_basic_s`](https://github.com/jamesob/bitcoin/tree/ackr/19160.1.ryanofsky.multiprocess_add_basic_s))
 4
 5Needs rebase, but luckily it's a pretty easy one.
 6
 7- - [x] 97232e7c62 Update libmultiprocess library
 8
 9  Verified hash matches reviewed version of libmultiprocess. 
10  Verified `curl -L https://github.com/chaincodelabs/libmultiprocess/archive/$(git rev-parse HEAD).tar.gz | sha256sum`
11  matches the expected hash.
12  
13- - [x] 547cb012ac multiprocess: Add Ipc and Init interface definitions
14- - [x] 98981da508 multiprocess: Add Ipc interface implementation
15- - [x] daf366f6ef multiprocess: Add bitcoin-node process spawning support
16- - [x] d9274f0bd2 multiprocess: Add comments and documentation
17- - [x] 36f1fbf50a multiprocess: Add echoipc RPC method and test
18
19
20### Manual testing
21
22Cloned and built libmultiprocess independently; installed using `cmake
23- -DCMAKE_INSTALL_PREFIX=/home/james/.local ..`.
24Installation using `sudo make install` without the local prefix led to an error during
25bitcoin build time.
26
27Had to then run `./configure` with `PKG_CONFIG_PATH=${HOME}/.local/lib/pkgconfig
28./configure ... -L${HOME}/.local/lib ... -I${HOME}/.local/include ...`
29
30Built bitcoin, started `bitcoin-node` with preexisting pruned datadir. Operation as
31usual. Executed `./src/bitcoin-cli echoipc wowowow` a few times:

2021-03-16T21:33:45Z [httpworker.2] Process bitcoin-node pid 62867 launched 2021-03-16T21:33:45Z [httpworker.2] {bitcoin-node-58227/b-httpworker.2-58239} IPC client first request from current thread, constructing waiter 2021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client send Init.construct$Params (threadMap = ) 2021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client recv Init.construct$Results (threadMap = ) 2021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client send Init.makeEcho$Params (context = (thread = , callbackThread = )) 2021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client recv Init.makeEcho$Results (result = ) 2021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client send Echo.echo$Params (context = (thread = , callbackThread = ), echo = “wowowow”) 2021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client recv Echo.echo$Results (result = “wowowow”) 2021-03-16T21:33:45Z [httpworker.2] 2021-03-16T21:33:45Z [httpworker.2] {bitcoin-node-58227/b-httpworker.2-58239} IPC client destroy N2mp11ProxyClientIN3ipc5capnp8messages4EchoEEE 2021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client send Echo.destroy$Params (context = (thread = , callbackThread = )) 2021-03-16T21:33:45Z [capnp-loop] {bitcoin-node-58227/b-httpworker.2-58239} IPC client recv Echo.destroy$Results () 2021-03-16T21:33:45Z [httpworker.2] 2021-03-16T21:33:45Z [httpworker.2] {bitcoin-node-58227/b-httpworker.2-58239} IPC client destroy N2mp11ProxyClientIN3ipc5capnp8messages4InitEEE 2021-03-16T21:33:45Z [httpworker.2] Process bitcoin-node pid 62867 exited with status 0

0
1
2<details><summary>Show platform data</summary>
3<p>

Tested on Linux-5.10.0-4-amd64-x86_64-with-glibc2.29

Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ -L/home/james/.local/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ -I/home/james/.local/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis –enable-wallet –enable-debug –with-daemon –enable-natpmp-default –enable-multiprocess

Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2 i

Compiler version:

 0
 1</p></details>
 2
 3-----BEGIN PGP SIGNATURE-----
 4
 5iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmBRJYkACgkQepNdrbLE
 6TwXItBAAj7COPnY6sf+ZXXds+5gDwCYKs50FYqrC1nsDsfHjEawnxT8z6h60z1lf
 7v2d7LB+U4970hrBxmRyt7K4Huv4q+R+0+8W5G8XlhyMLdR9Jv238eGOtYfsm3Sj9
 8LXV1L5MNNLe7moNYBe2/06n+HoyjDvOxXu6YKXLTIkVjAmY4JriMGBdjwshzp5OJ
 9NApYBiDjHP23I7JjOKljfLUOZuc+IQy2kelA4csFJIry14zWJFHYaCzOJA1T5GKD
10KuDyokApdq6msNg/t+ycVOYzux2lYZO35D/Xm7j1PTFkvkb1Hwp+7hEgK0WwC4Qr
11GSTrYsIb/ZREhTqlkZ7B3Qf9Kr6/hqIg2QuMaorSaRBMSjvrC1LdP/PSDsFVt3iu
127YEBFxc2agWAvkiaoxp1blecoGOJoj2Oyz37OLq8gZnlOgen19xL029Fz5wcTTfh
13hpZPvaTMZLFuPf5mSr1YM6Jb5AChRGDyH9Dn0oEEfI6WB8ID+PU0C23HOrvoqQPg
14HWjjO4BsvdL4j5aH2XC1EYIA/LH1EcZhMeiphkAOsLByZ20RIY97zPc7ZuB0DNmt
15+x/GuY/0gfhz5+VJIgzypPmz/uizmNN7N/b2kc4qLKFJje066J6wyC1JUKORL5BE
16TrjqrpLN4P12Njb142OPTw11GIiIlCMBA59U+biAfBh7wdQJqc8=
17=74Fd
18-----END PGP SIGNATURE-----

ryanofsky force-pushed on Mar 17, 2021

ryanofsky commented at 8:00 pm on March 17, 2021: contributor

re: #19160 (comment)

Thanks for reviewing this and also taking the time to go through the guts of (it seems like the entire?) libmultiprocess library. I know it needs to be documented better, and I’m working on this. I started adding a standalone example program https://github.com/chaincodelabs/libmultiprocess/pull/49 to show how the library is meant to be used without the added complexity of bitcoin integration in this PR. I don’t necessarily know where things are more clear and less clear, and would encourage freely opening issues https://github.com/chaincodelabs/libmultiprocess/issues/new to suggest improvements, ask questions, or leave feedback. There’s high level and low level documentation that I’m happy to write if it seems like someone will read it, and I know what to write about.

The contents of, say, the type marshalling unit are out of my paygrade from a C++ knowhow standpoint so while the code seems fine (and certainly well designed by @ryanofsky), I cannot take a full accounting of its correctness or safety.

I think this should be fixable, and filed https://github.com/chaincodelabs/libmultiprocess/issues/50 to track. The only thing the code actually does is assign input values to output values, but are there are a lot of ways this can happen in c++ with input & output arguments, return values, and emplaces, and a lot of types to make it work with. The code did sprawl over time supporting new usages and types, and I’m pretty sure it could be simpler.

libmultiprocess is more general/complex than necessary in the longterm and even if we may be able to come up with a simpler mechanism to enable multiprocess coordination, I think getting multiprocess operation fleshed out and working is the really important thing at this point.

As I see it, there’s a tradeoff between how much C++ template magic you want to accept vs how much boilerplate you want to write when you add a new IPC method or parameter, and the current approach leans towards having more template magic and less boilerplate. The implications should be confined to the src/ipc/ directory, though, so this is less significant than something like serialize.h template magic which is used in many parts of the codebase. Also as mentioned #10102 (comment), it will always be possible to build bitcoin without any src/ipc/ code whatsoever (it’s not built unless you use --enable-multiprocess), and if you want to replace the current IPC implementation with a different IPC implementation you can write a interfaces::Ipc subclass with overridden spawnProcess, etc methods that don’t use libmultiprocess.

Rebased 36f1fbf50a0f2c96dfd1f7fe84d407f39a1906bd -> 6a2951a7c7690e4f974fb938e1434ca836762207 (pr/ipc-echo.28 -> pr/ipc-echo.29, compare) due to conflict with #21007

DrahtBot removed the label Needs rebase on Mar 17, 2021

jamesob commented at 3:14 pm on March 22, 2021: member

ACK https://github.com/bitcoin/bitcoin/pull/19160/commits/6a2951a7c7690e4f974fb938e1434ca836762207

Verified that rebase consists of MakeUnique -> std::make_unique, removal of an unnecessary util/memory.h import. Built locally and did the echoipc test on a different machine than last ACK.

Thanks for the detailed response, @ryanofsky. I guess this is sort of tantamount to the familiar issue of using an object-relational mapper vs. writing raw SQL. In the former case, client code is very simple and easy to read but there’s good deal of generality and complexity in the underlying library; in the latter case there is a lot of rote code on the clientside but little ambiguity about what is happening under the covers.

In any case, I’m happy to do what I can to help improve libmultiprocess in the meantime and defer big discussions for IPC mechanism until multiprocess operation is humming along (in an opt-in way) with this architecture.

ryanofsky commented at 4:38 pm on March 22, 2021: contributor

re: #19160 (comment)

I guess this is sort of tantamount to the familiar issue of using an object-relational mapper vs. writing raw SQL.

I never thought of it, but this seems like a perfect analogy.

The difference between using capnp with libmultiprocess and capnp without libmultiprocess is like the difference between using an ORM with a database and not using an ORM.

And the difference between using capnp to communicate over the socket vs reading/writing over the socket directly is like the difference between using postgres/mysql client libraries vs using the postgres/mysql client protocols (assuming they are simple but probably finnicky protocols).

One thing that isn’t really analogous (and reason this should be less scary than choosing an ORM) is that when you use an ORM it can become deeply embedded and tied to your application logic. But this PR and #10102 are just adding an IPC implemention of existing interfaces. So if you wanted to drop a layer in the IPC stack, or change the IPC protocol, changes would be confined to src/ipc/ and not affect anything outside.

in src/ipc/process.h:17 in 437e20cacb outdated

12+class Protocol;
13+
14+//! IPC process interface for spawning bitcoin processes and serving requests
15+//! in processes that have been spawned.
16+//!
17+//! There will different implementations of this interface depending on the

fjahr commented at 10:09 pm on March 22, 2021:

nit: …will be different…

ryanofsky commented at 12:51 pm on March 30, 2021:

re: #19160 (review)

nit: …will be different…

Thanks, fixed!

ariard commented at 4:07 pm on March 26, 2021: member

ACK 6a2951a, notable code change switch to std::make_unique

I built locally the multiple processes and tried few echo string through the debug echoipc. Works as expected with ipc messages debug displayed through --debug=ipc

dongcarl commented at 8:05 pm on March 26, 2021: contributor

ACK 6a2951a, switched to std::make_unique

Is there anything remaining here that’s needed before it’s ready for merge?

ryanofsky commented at 8:24 pm on March 26, 2021: contributor

Is there anything remaining here that’s needed before it’s ready for merge?

Just in case this question is for me, I don’t think any issues are outstanding, and whether this is ready for merge is just a question of whether it needs more review. I can’t say if it does, but I appreciate all the review it’s had so far, and thanks for keeping up with this!

in doc/multiprocess.md:68 in 66937157bc outdated

63+references and `std::function` arguments are automatically tracked and mapped
64+to allow invoked code to call back into invoking code at any time, and there is
65+a 1:1 threading model where any thread invoking a method in another process has
66+a corresponding thread in the invoked process responsible for executing all
67+method calls from the source thread, without blocking I/O or holding up another
68+calls, and using the same thread local variables, locks, and callbacks between

fjahr commented at 3:20 pm on March 28, 2021:

nit: …holding up another call, …

ryanofsky commented at 12:50 pm on March 30, 2021:

re: #19160 (review)

nit: …holding up another call, …

Thanks, fixed!

in src/init/bitcoin-node.cpp:14 in 360821c035 outdated

 8+
 9+#include <memory>
10+
11+namespace init {
12+namespace {
13+const char* EXE_NAME = "bitcoin-node";

fjahr commented at 5:39 pm on March 28, 2021:

This string is repeated in rpc/misc.cpp in the echo RPC. I guess these exe names should be global constants or organized in some other way in the future. But that can be kept for a follow-up (and probably already is included in one, I just haven’t checked).

ryanofsky commented at 12:50 pm on March 30, 2021:

re: #19160 (review)

This string is repeated in rpc/misc.cpp in the echo RPC. I guess these exe names should be global constants or organized in some other way in the future. But that can be kept for a follow-up (and probably already is included in one, I just haven’t checked).

That’s a good idea. Init exe names and spawnProcess exe names might be more readable (even if there’s an extra mental hoop to jump through) written as constants like NODE_EXE_NAME, WALLET_EXE_NAME, GUI_EXE_NAME. Will think about it more for #10102. Unclear if these should be configurable, maybe at build time or run time. Also, the exe names in these init files are just used for thread naming and logging and could potentially be eliminated, while the exe names used in spawnProcess calls are used to actually build command lines, so might be more important.

ariard commented at 2:03 pm on March 29, 2021: member

@fjahr @hebasto @Sjors @jonatack You previously had a look on this PR. If you have still keen interest in this work, I would say it’s pretty mature for review :)

fjahr commented at 7:02 pm on March 29, 2021: contributor

ACK 6a2951a7c7690e4f974fb938e1434ca836762207

I have reviewed all the code included in this PR, including the code changes in libmultiprocess. I have only scanned the rest of the code of libmultiprocess so far but plan to do more in depth review there until I review the follow-up this PR (FWIW, I will try to help out with the documentation on libmultiprocess while I get more into it). However, I have also scanned the generated files for the echoipc example and didn’t see any issues there. I have build this PR with --enable-multiprocess, ran a testnet node with it over several days and tested the echoipc RPC multiple times, checking the generated logs.

Obviously, please ignore my very minor nits unless you have to retouch.

Note for other reviewers: It helped me especially in this review to look at the commits in reverse order at least once, i.e. going from usage to implementation and lower-level stuff.

achow101 commented at 8:06 pm on March 29, 2021: member

light ACK 6a2951a7c7690e4f974fb938e1434ca836762207

Reviewed and tested the changes here, but did not look at libmultiprocess or the .capnp. files. I did check that echoipc for bitcoin-node spawned another bitcoin-node process and did not for bitcoind.

DrahtBot added the label Needs rebase on Mar 30, 2021

ryanofsky force-pushed on Mar 30, 2021

DrahtBot removed the label Needs rebase on Mar 30, 2021

ryanofsky commented at 1:44 pm on March 30, 2021: contributor

Rebased 6a2951a7c7690e4f974fb938e1434ca836762207 -> 1290ccf8c70f5f11148683c3f69044fac9956e05 (pr/ipc-echo.29 -> pr/ipc-echo.30, compare) due to conflict with #20228. Also implemented review suggestions. All small changes.

fjahr commented at 8:09 pm on March 30, 2021: contributor

re-ACK 1290ccf8c70f5f11148683c3f69044fac9956e05

Only changes since last review were rebasing, explicit memory include in interfaces/echo.cpp and small doc improvements.

jamesob commented at 7:31 pm on March 31, 2021: member

ACK 1290ccf8c70f5f11148683c3f69044fac9956e05 (jamesob/ackr/19160.3.ryanofsky.multiprocess_add_basic_s)

Examined range-diff to ensure changes since last ACK only reflect rebase.

Didn’t complete rebuild due to clang thread-safety-analysis failures (on master?).

 0net_processing.cpp:1260:17: error: calling function 'EraseForBlock' requires negative capability '!g_cs_orphans' [-Werror,-Wthread-safety-analysis]
 1    m_orphanage.EraseForBlock(*pblock);
 2                ^
 3net_processing.cpp:1444:21: error: calling function 'HaveTx' requires negative capability '!g_cs_orphans' [-Werror,-Wthread-safety-analysis]
 4    if (m_orphanage.HaveTx(gtxid)) return true;
 5                    ^
 6net_processing.cpp:2788:13: error: calling function 'ProcessGetData' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
 7            ProcessGetData(pfrom, *peer, interruptMsgProc);
 8            ^
 9net_processing.cpp:3856:13: error: calling function 'ProcessGetData' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
10            ProcessGetData(*pfrom, *peer, interruptMsgProc);
11            ^
124 errors generated.

0Tested on Linux-4.19.0-16-amd64-x86_64-with-glibc2.28
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default
3
4Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2  i
5
6Compiler version:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 1290ccf8c70f5f11148683c3f69044fac9956e05 ([`jamesob/ackr/19160.3.ryanofsky.multiprocess_add_basic_s`](https://github.com/jamesob/bitcoin/tree/ackr/19160.3.ryanofsky.multiprocess_add_basic_s))
 4
 5Examined range-diff to ensure changes since last ACK only reflect rebase.
 6
 7Didn't complete rebuild due to clang thread-safety-analysis failures (on master?).
 8
 9<details><summary>Show platform data</summary>
10<p>

Tested on Linux-4.19.0-16-amd64-x86_64-with-glibc2.28

Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis –enable-wallet –enable-debug –with-daemon –enable-natpmp-default

Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2 i

Compiler version:

 0
 1</p></details>
 2
 3-----BEGIN PGP SIGNATURE-----
 4
 5iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmBkzd4ACgkQepNdrbLE
 6TwWEoQ/9EIAQoLU18/k5dRr3yA0qxLICzBUKUkqSdmKEsUiQyotzEsRiObFXdFKj
 7IsynndrGVhnIxYy0v6+41cF0tJTzol8B/4YvP2cP1n3KxJO727JZUArSN88F1qeh
 8LUCJ4PmuBXnoT4zMqOl+AM/pJwfFLD1Flv6UiHfKr9uXYf/orSFMZ5QwEH4wV6yi
 9wQllVHoZqdZQjE54kB1jTGc5N4GWB4gVKLipfdztRa8JV76kGetb4uiuuUSCslr+
10kdfoRIGDQtAzC6OZdkFVSE49hPMgZO1NC+zUx7e0/o6MmKnsS8jvdV49HY2DevKX
118iDRXtYV9VXy82SgBc5c8sOb3/uYV2Y1CtML45GzJjdoThoJDLodFV1PXUIn91Qa
12AnXyOIm7U2S3utjagO9Wy6AeuTZAyCHPG9JpcjJx1dZ1up0uO+6raZdbTWRVNkuX
13TAugEEDkkQfvkavJsmtnd3CdqNZNT5F2M6U1gp4DiZjAmG/UgozlF3PN/oUc09+3
14yyRyriKY7QCnEZlFtqyUA7vE/rn1FLpBCrqv8YJT4XSRz+NamapPgNfKEUp3HTZh
15/zqFRmcdTP2h+cprkJJzE0gvH5jS/WUpuyRzb1x8oy6u1qD1u3zS+FTkWONIA0eQ
16kXdeLIPxMgoo5fk3LWnFhExWA3keOFh34aUKAuNruK4ItY+PraY=
17=8dw0
18-----END PGP SIGNATURE-----

ryanofsky commented at 7:38 pm on March 31, 2021: contributor

Didn’t complete rebuild due to clang thread-safety-analysis failures (on master?).

0net_processing.cpp:1260:17: error: calling function 'EraseForBlock' requires negative capability '!g_cs_orphans' [-Werror,-Wthread-safety-analysis]
1    m_orphanage.EraseForBlock(*pblock);

Thanks for rechecking. I’d be surprised if these errors weren’t happening on master since this PR isn’t changing net_processing.cpp or anything adjacent. These look like the same errors reported #20272 (comment)

ariard commented at 7:57 pm on April 1, 2021: member

ACK 1290ccf

Built well and able to test echoipc on my side ?

promag commented at 9:33 pm on April 5, 2021: member

Tested ACK 1290ccf8c70f5f11148683c3f69044fac9956e05 on bionic and macOS. ~~I’m having issues building on macOS but probably something is bad on my system, I’ll check later.~~

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

ryanofsky commented at 10:27 pm on April 5, 2021: contributor

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.

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 : https://github.com/ryanofsky/bitcoin/commit/30f5c5a1768777ca7a9dd822c47e0bd8a5950d05. 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.

promag commented at 9:20 am on April 6, 2021: member

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 https://github.com/chaincodelabs/libmultiprocess/issues/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 https://github.com/ryanofsky/bitcoin/commit/30f5c5a1768777ca7a9dd822c47e0bd8a5950d05 or similar in a follow up and relevant instructions.

dongcarl commented at 7:32 pm on April 16, 2021: contributor

re-ACK 1290ccf8c70f5f11148683c3f69044fac9956e05

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 commented at 8:56 pm on April 16, 2021: contributor

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 commented at 8:35 pm on April 22, 2021: member

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

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

EnsureNodeContext was removed in 038854f31e3511e8bb6e163305cab0a96783d25b (#21391)

Update libmultiprocess library

Fix "Disable GCC suggest-override warnings for proxy clients" https://github.com/chaincodelabs/libmultiprocess/pull/40 is needed to prevent cirrus GCC failure https://cirrus-ci.com/task/6000489311502336?command=ci#L4294

This also includes other recent changes

https://github.com/chaincodelabs/libmultiprocess/pull/35 Fix README.md markdown
https://github.com/chaincodelabs/libmultiprocess/pull/37 Add "make check" target to build and run tests
https://github.com/chaincodelabs/libmultiprocess/pull/38 Add "extends" inherited method support
https://github.com/chaincodelabs/libmultiprocess/pull/41 Avoid depending on argument default constructors
https://github.com/chaincodelabs/libmultiprocess/pull/42 Support attaching custom cleanup functions to proxy client and server classes
https://github.com/chaincodelabs/libmultiprocess/pull/43 Drop hardcoded #include lines in generated files

5d62d7f6cd

multiprocess: Add Ipc and Init interface definitions 745c9cebd5

multiprocess: Add Ipc interface implementation 10afdf0280

multiprocess: Add bitcoin-node process spawning support

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

ddf7ecc8df

multiprocess: Add comments and documentation 7d76cf667e

multiprocess: Add echoipc RPC method and test

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.

84934bf70e

ryanofsky force-pushed on Apr 23, 2021

ryanofsky commented at 6:13 pm on April 23, 2021: contributor

#19160 (comment)

Looks like there has been a silent merge conflict

Thanks! Rebased and updated the call site.

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

fjahr commented at 7:45 pm on April 24, 2021: contributor

re-ACK 84934bf70e11fe4cda1cfda60113a54895d4fdd5

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 commented at 5:43 pm on April 26, 2021: member

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

laanwj merged this on Apr 27, 2021

laanwj closed this on Apr 27, 2021

laanwj removed this from the "Blockers" column in a project

sidhujag referenced this in commit 714e1f2c40 on Apr 27, 2021

fanquake moved this from the "In progress" to the "Done" column in a project

ryanofsky referenced this in commit 1a3dc8d263 on Jul 20, 2021

fanquake referenced this in commit 07dcf1a76e on Feb 18, 2022

MarcoFalke referenced this in commit 28aa0e3ca0 on Feb 19, 2022

hmel referenced this in commit 7ab17fd1c5 on Feb 20, 2022

ryanofsky referenced this in commit 4063a06213 on May 25, 2022

ryanofsky referenced this in commit 6e1c16c144 on May 25, 2022

fanquake referenced this in commit 345457b542 on May 27, 2022

gwillen referenced this in commit 5f0c45b568 on Jun 1, 2022

bitcoin locked this on Aug 16, 2022

multiprocess: Add basic spawn and IPC support #19160

Conflicts

Parent’s Perspective

Child’s perspective

Review notes

Manual testing