multiprocess: Add -ipcbind option to bitcoin-node

ryanofsky commented at 2:34 pm on July 23, 2024: contributor

Add -ipcbind option to bitcoin-node to make it listen on a unix socket and accept connections from other processes. The default socket path is <datadir>/node.sock, but this can be customized.

This option lets potential wallet, gui, index, and mining processes connect to the node and control it. See examples in #19460, #19461, and #30437.

Motivation for this PR, in combination with #30510, is be able to release a bitcoin core node binary that can generate block templates for a separate Stratum v2 mining service, like the one being implemented in https://github.com/Sjors/bitcoin/pull/48, that connects over IPC.

Other things to know about this PR:

While the -ipcbind option lets other processes to connect to the bitcoin-node process, the only thing they can actually do after connecting is call methods on the Init interface which is currently very limited and doesn’t do much. But PRs #30510, #29409, and #10102 expand the Init interface to expose mining, wallet, and gui functionality respectively.
This PR is not needed for #10102, which runs GUI, node, and wallet code in different processes, because #10102 does not use unix sockets or allow outside processes to connect to existing processes. #10102 lets parent and child processes communicate over internal socketpairs, not externally accessible sockets.

This PR is part of the process separation project.

DrahtBot commented at 2:34 pm on July 23, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, itornaza, achow101
Stale ACK	Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30635 (rpc: add optional blockhash to waitfornewblock by Sjors)
#30510 (multiprocess: Add IPC wrapper for Mining interface by ryanofsky)
#30409 (Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block by Sjors)

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.

Sjors commented at 3:03 pm on July 23, 2024: member

One thing that may be useful to clarify, IIUC, although you can connect, nothing can be done via these connections yet. That is what #30510 introduces specifically for the Mining interface.

ryanofsky commented at 3:28 pm on July 23, 2024: contributor

One thing that may be useful to clarify, IIUC, although you can connect, nothing can be done via these connections yet. That is what #30510 introduces specifically for the Mining interface.

Thank you, added this to the description

ryanofsky force-pushed on Jul 24, 2024

ryanofsky commented at 5:48 pm on July 24, 2024: contributor

re: #30509#issue-2425377062

The default socket path is currently <datadir>/sockets/bitcoin-node.sock, but this can be customized.

I changed the default socket path to <datadir>/node.sock after running into some “exceeded maximum socket path length” errors. The maximum unix socket path length is only 107 characters, so I thought a shorter suffix might avoid problems on systems with long data directory or home directory paths.

Another possible approach could be to use $XDG_RUNTIME_DIR/bitcoin-node.sock as the default socket path, since $XDG_RUNTIME_DIR should resolve to a short path like /run/user/1000. But this could lead to surprises on systems with multiple data directories since the socket path is no longer inside the data directory, and it could cause security problems since the socket would no longer be shielded by permissions of the data directory.

Updated 3d9a8cb4e194399fa9b2ce8af6bd2a8b8ef970c6 -> efdc120795e7c3ce914ee0677c3b69803ea5de1e (pr/ipc-bind.1 -> pr/ipc-bind.2, compare) with this change and a test simplification.

in src/ipc/process.cpp:70 in 9debfa694d outdated

65+                std::string& address,
66+                std::string& error) override;
67+    int bind(const fs::path& data_dir, const std::string& exe_name, std::string& address, std::string& error) override;
68 };
69+
70+bool ParseAddress(std::string& address,

TheCharlatan commented at 10:16 am on August 7, 2024:

Nit: Could be declared static?

ryanofsky commented at 4:16 pm on August 7, 2024:

re: #30509 (review)

Nit: Could be declared static?

Sure, added static

in src/init.cpp:672 in efdc120795 outdated

667@@ -667,6 +668,9 @@ void SetupServerArgs(ArgsManager& argsman)
668     argsman.AddArg("-rpcwhitelistdefault", "Sets default behavior for rpc whitelisting. Unless rpcwhitelistdefault is set to 0, if any -rpcwhitelist is set, the rpc server acts as if all rpc users are subject to empty-unless-otherwise-specified whitelists. If rpcwhitelistdefault is set to 1 and no -rpcwhitelist is set, rpc server acts as if all rpc users are subject to empty whitelists.", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
669     argsman.AddArg("-rpcworkqueue=<n>", strprintf("Set the depth of the work queue to service RPC calls (default: %d)", DEFAULT_HTTP_WORKQUEUE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
670     argsman.AddArg("-server", "Accept command line and JSON-RPC commands", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
671+    if (can_listen_ipc) {
672+        argsman.AddArg("-ipcbind=<address>", "Bind to Unix socket address and listen for incoming connections. Valid address values are \"unix\" to listen on the default path, <datadir>/node.sock, or \"unix:/custom/path\" to specify a custom path. Can be specified multiple times to listen on multiple paths. Default behavior is not to listen on any path.", ArgsManager::ALLOW_ANY, OptionsCategory::IPC);

TheCharlatan commented at 12:06 pm on August 7, 2024:

If a relative path is provided, the path of the data directory is appended. If the parent directories in the provided path don’t exist, they will be created. Should that be documented here?

ryanofsky commented at 4:16 pm on August 7, 2024:

re: #30509 (review)

If a relative path is provided, the path of the data directory is appended. If the parent directories in the provided path don’t exist, they will be created. Should that be documented here?

Sure added both things.

in src/ipc/process.cpp:75 in 9debfa694d outdated

71+                  const fs::path& data_dir,
72+                  const std::string& dest_exe_name,
73+                  struct sockaddr_un& addr,
74+                  std::string& error)
75+{
76+    if (address.compare(0, 4, "unix") == 0 && (address.size() == 4 || address[4] == ':')) {

TheCharlatan commented at 12:38 pm on August 7, 2024:

Nit (personal style opinion): I feel like this (and the other functions in this file) would be a bit easier to read if the error conditions would return early. Then all this logic would not have to be indented and the final return of the function would carry the “good” value.

ryanofsky commented at 4:16 pm on August 7, 2024:

re: #30509 (review)

Nit (personal style opinion): I feel like this (and the other functions in this file) would be a bit easier to read if the error conditions would return early. Then all this logic would not have to be indented and the final return of the function would carry the “good” value.

I could be missing something, but I think all the functions in this file are returning or throwing as soon as there is an error.

In this case it should not be an error if the address type isn’t unix. The idea is for this to support multiple address types and look like

 0if (address type is "unix") {
 1   // parse unix socket path
 2   return true;
 3} else if (address type is "tcp") {
 4   // parse tcp address
 5   return true;
 6} else if (address type is "ntpipe") {
 7   // parse windows named pipe path
 8   return true;
 9} else if (address type is "fd") {
10   // parse file descriptor number
11   return true;
12}
13error = "unrecognized address type"
14return false;

TheCharlatan commented at 5:39 pm on August 7, 2024:

Ah, that makes sense, I did not think about this function being expanded over time.

in src/init/bitcoin-gui.cpp:41 in efdc120795 outdated

34@@ -35,6 +35,7 @@ class BitcoinGuiInit : public interfaces::Init
35     std::unique_ptr<interfaces::Echo> makeEcho() override { return interfaces::MakeEcho(); }
36     interfaces::Ipc* ipc() override { return m_ipc.get(); }
37     node::NodeContext m_node;
38+    bool canListenIpc() override { return true; }

TheCharlatan commented at 3:06 pm on August 7, 2024:

Just a question: Intuitively I was under the impression that only the node should be listening. In which scenario should the gui be able to listen as well? Do we need a bi-directional connection here for some callbacks?

ryanofsky commented at 4:16 pm on August 7, 2024:

re: #30509 (review)

Just a question: Intuitively I was under the impression that only the node should be listening. In which scenario should the gui be able to listen as well? Do we need a bi-directional connection here for some callbacks?

Added comment. You are right that gui does not listen on a socket, but bitcoin-gui accepts all the same arguments bitcoin-node does and more. If bitcoin-gui is started and a node processs is not already running (see #19461), a node process started with the provided arguments, and that node process can listen on one more more unix sockets.

Sjors commented at 4:34 pm on August 12, 2024:

4137419b0170fa1e9ff9daacde57933f7c995b76: In the mean time this has the nice side-effect of being able to run src/bitcoin-gui -ipcbind=unix (instead of bitcoin-node) and then connect to it with e.g. the miner demo -ipcconnect=unix:gui.sock.

See #30437 (comment)

in src/test/ipc_test.cpp:124 in 2dd88257ab outdated

119+    //   Address 'unix' path '"/tmp/test_common_Bitcoin Core/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff/test_bitcoin.sock"' exceeded maximum socket path length
120+    const std::string bind_address{strprintf("unix:%s", TempPath("bitcoin_sock_XXXXXX"))};
121+    std::unique_ptr<interfaces::Init> init{std::make_unique<TestInit>()};
122+    std::unique_ptr<ipc::Protocol> protocol{ipc::capnp::MakeCapnpProtocol()};
123+    std::unique_ptr<ipc::Process> process{ipc::MakeProcess()};
124+    {

TheCharlatan commented at 3:37 pm on August 7, 2024:

Since multiple binds are supported, I would suggest to add a test case for binding and connecting on different addresses in the same process.

ryanofsky commented at 4:17 pm on August 7, 2024:

re: #30509 (review)

Since multiple binds are supported, I would suggest to add a test case for binding and connecting on different addresses in the same process.

Thanks, added

in src/test/ipc_test.cpp:142 in 2dd88257ab outdated

137+    BOOST_CHECK_EQUAL(error, "");
138+    std::unique_ptr<interfaces::Init> remote_init{protocol->connect(connect_fd, "test-connect")};
139+    std::unique_ptr<interfaces::Echo> remote_echo{remote_init->makeEcho()};
140+    BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test");
141+    remote_echo.reset();
142+    remote_init.reset();

TheCharlatan commented at 3:38 pm on August 7, 2024:

Is there a reason for manually resetting these here? I guess in the other test resetting is required, because otherwise the thread will never join.

ryanofsky commented at 4:17 pm on August 7, 2024:

re: #30509 (review)

Is there a reason for manually resetting these here? I guess in the other test resetting is required, because otherwise the thread will never join.

Nice catch, these are not needed. An initial version of this test did use a thread but it’s gone and these are no longer necessary.

TheCharlatan commented at 3:45 pm on August 7, 2024: contributor

lgtm , I hope we can finally get this project under way now.

ryanofsky force-pushed on Aug 7, 2024

ryanofsky commented at 5:26 pm on August 7, 2024: contributor

Thanks for the review!

Updated efdc120795e7c3ce914ee0677c3b69803ea5de1e -> cf9aaf4b7f27b38bf201aa39c56c9967f3b2a904 (pr/ipc-bind.2 -> pr/ipc-bind.3, compare) with suggestions

in src/init.cpp:681 in cf9aaf4b7f outdated

667@@ -667,6 +668,9 @@ void SetupServerArgs(ArgsManager& argsman)
668     argsman.AddArg("-rpcwhitelistdefault", "Sets default behavior for rpc whitelisting. Unless rpcwhitelistdefault is set to 0, if any -rpcwhitelist is set, the rpc server acts as if all rpc users are subject to empty-unless-otherwise-specified whitelists. If rpcwhitelistdefault is set to 1 and no -rpcwhitelist is set, rpc server acts as if all rpc users are subject to empty whitelists.", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
669     argsman.AddArg("-rpcworkqueue=<n>", strprintf("Set the depth of the work queue to service RPC calls (default: %d)", DEFAULT_HTTP_WORKQUEUE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
670     argsman.AddArg("-server", "Accept command line and JSON-RPC commands", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
671+    if (can_listen_ipc) {
672+        argsman.AddArg("-ipcbind=<address>", "Bind to Unix socket address and listen for incoming connections. Valid address values are \"unix\" to listen on the default path, <datadir>/node.sock, or \"unix:/custom/path\" to specify a custom path. Can be specified multiple times to listen on multiple paths. Default behavior is not to listen on any path. If a relative paths are specified, they are interpreted relative to the network data directory. If paths include any parent directory components and the parent directories do not exist, they will be created.", ArgsManager::ALLOW_ANY, OptionsCategory::IPC);

TheCharlatan commented at 5:38 pm on August 7, 2024:

Typo nit: s/if a relative paths are/if relative paths/

ryanofsky commented at 3:44 pm on August 19, 2024:

re: #30509 (review)

Typo nit: s/if a relative paths are/if relative paths/

Thanks, fixed!

DrahtBot commented at 6:42 pm on August 7, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28475408210

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Aug 7, 2024

ryanofsky commented at 10:00 pm on August 8, 2024: contributor

The test failure https://cirrus-ci.com/task/5102088831631360 in the new test which tests binding and connecting on multiple sockets turns out to reveal a corner case bug in libmultiprocess. The bug does not happen locally for me, but it does happen if I run the CI scripts inside a container, I think maybe because the container is 32 bit.

Basically what happens is the test disconnects and reconnects immediately, which causes an mp::Connection instance to be freed and a new one to be allocated right away. In the CI container, the new mp::Connection object is allocated at the same address as the previous object and this causes the client connection try to reuse a remote thread associated with the previous connection which no longer exists. This happens because the client tracks threads to run on in a map std::map<Connection*, ProxyClient<Thread>> request_threads; which is keyed by Connection pointer, with the assumption that that two different connections will not have the same pointer. Which is not the case when this test case is run in the CI container. I started implementing a fix for this which cleans up the map entry when the connection is destroyed, which is probably a good thing to do anyway so the map does not have stale entries in it.

ryanofsky referenced this in commit febe6d2492 on Aug 9, 2024

DrahtBot removed the label CI failed on Aug 9, 2024

ryanofsky referenced this in commit 8ba0d03b44 on Aug 9, 2024

ryanofsky referenced this in commit c1b4ab4eb8 on Aug 9, 2024

ryanofsky force-pushed on Aug 9, 2024

ryanofsky commented at 3:42 pm on August 9, 2024: contributor

Rebased cf9aaf4b7f27b38bf201aa39c56c9967f3b2a904 -> 4137419b0170fa1e9ff9daacde57933f7c995b76 (pr/ipc-bind.3 -> pr/ipc-bind.4, compare) with fix for new test that was failing when run in the multiprocess CI container #30509 (comment)

Sjors commented at 3:39 pm on August 12, 2024: member

<datadir>/node.sock seems fine, although on macOS with the longest permitted username you would go over the limit:

/Users/willem-alexanderclausgeorgeferdinand-koning-der-nederlanden-jonkheer-van-amsberg-67/Library/Application\ Support/Bitcoin/testnet4/bitcoin-node.sock

in depends/packages/native_libmultiprocess.mk:2 in ef57db8711 outdated

0@@ -1,8 +1,8 @@
1 package=native_libmultiprocess
2-$(package)_version=6aca5f389bacf2942394b8738bbe15d6c9edfb9b
3+$(package)_version=c1b4ab4eb897d3af09bc9b3cc30e2e6fff87f3e2

Sjors commented at 3:58 pm on August 12, 2024:

ef57db8711c7fafb6c4f09b880fd4263bc6dc9dd: I also tested building this outside depends on macOS 14.6.1

ryanofsky commented at 4:17 pm on August 12, 2024: contributor

<datadir>/node.sock seems fine, although on macOS with the longest permitted username you would go over the limit: /Users/willem-alexanderclausgeorgeferdinand-koning-der-nederlanden-jonkheer-van-amsberg-67/Library/Application\ Support/Bitcoin/testnet4/bitcoin-node.sock

Ha! It is pretty easy to go over the socket path limit on linux too since it is only something like 100 characters. I think it might actually be possible to work around this by forking the process, changing the working directory, opening the socket with a relative path, and then passing back a file descriptor the parent process, but not sure about that and hopefully it would not be necessary.

in src/init.cpp:1211 in 4137419b01 outdated

1203@@ -1200,6 +1204,16 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1204     g_wallet_init_interface.Construct(node);
1205     uiInterface.InitWallet();
1206 
1207+    if (interfaces::Ipc* ipc = node.init->ipc()) {
1208+        for (std::string address : gArgs.GetArgs("-ipcbind")) {
1209+            std::string error;
1210+            if (!ipc->listenAddress(address, error)) {
1211+                return InitError(strprintf(Untranslated("Unable to bind to IPC address '%s'. %s"), address, error));

Sjors commented at 4:40 pm on August 12, 2024:

4137419b0170fa1e9ff9daacde57933f7c995b76: I have a hard hitting this, instead I tend to get a crash (on intel macOS):

For an existing path:

0 -ipcbind=unix://tmp
1...
2************************
3EXCEPTION: NSt3__112system_errorE       
4Address already in use       
5bitcoin in AppInit()

Non-existing path that I don’t have permission for:

0-ipcbind=unix://root
1...
2************************
3EXCEPTION: NSt3__112system_errorE       
4Read-only file system       
5bitcoin in AppInit()

ryanofsky commented at 3:44 pm on August 19, 2024:

re: #30509 (review)

4137419: I have a hard hitting this, instead I tend to get a crash (on intel macOS):

Nice catch, thanks for testing this. The error should be handled cleanly now instead of crashing. The connect and listen code was trying to make it easier to distinguish between different types of errors like connections that failed because nothing was listening vs connections that failed for other reasons, but the approach didn’t really make sense. Error handling should be simpler and more reliable now.

Sjors commented at 9:08 am on August 20, 2024:

Works now.

in src/test/ipc_test.cpp:100 in a8d4adfea4 outdated

90@@ -65,3 +91,71 @@ void IpcTest()
91     disconnect_client();
92     thread.join();
93 }
94+
95+//! Test ipc::Protocol connect() and serve() methods connecting over a socketpair.
96+void IpcSocketPairTest()
97+{
98+    int fds[2];
99+    BOOST_CHECK_EQUAL(socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0);

Sjors commented at 5:33 pm on August 12, 2024:

a8d4adfea4b50f452d80bc1e7ee322945d886c78: @vasild’s #30205 is potentially useful here. Though as long as the test is not brittle, there’s no need to wait for that PR to be merged.

ryanofsky commented at 3:45 pm on August 19, 2024:

re: #30509 (review)

a8d4adf: @vasild’s #30205 is potentially useful here. Though as long as the test is not brittle, there’s no need to wait for that PR to be merged.

Agree it could be useful to test what happens when invalid data is sent or received on the socket. All the current tests are using real sockets and sending and receiving valid data.

in src/test/ipc_test.cpp:157 in a8d4adfea4 outdated

152+    // Bind and listen on multiple addresses
153+    for (const auto& address : addresses) {
154+        bind_and_listen(address);
155+    }
156+
157+    // Connect and test each address

Sjors commented at 6:04 pm on August 12, 2024:

a8d4adfea4b50f452d80bc1e7ee322945d886c78: maybe test what happens when there’s a second connection attempt to the same address.

ryanofsky commented at 3:45 pm on August 19, 2024:

re: #30509 (review)

a8d4adf: maybe test what happens when there’s a second connection attempt to the same address.

Good idea, added more connections

in src/ipc/capnp/protocol.cpp:59 in 52917ad4be outdated

52@@ -51,6 +53,14 @@ class CapnpProtocol : public Protocol
53         startLoop(exe_name);
54         return mp::ConnectStream<messages::Init>(*m_loop, fd);
55     }
56+    void listen(int listen_fd, const char* exe_name, interfaces::Init& init) override
57+    {
58+        startLoop(exe_name);
59+        if (::listen(listen_fd, 5 /* backlog */) != 0) {

Sjors commented at 6:18 pm on August 12, 2024:

52917ad4be1ca574543e854fdc154e88d65ce57f: nit /*backlog=*/5

ryanofsky commented at 3:44 pm on August 19, 2024:

re: #30509 (review)

52917ad: nit /*backlog=*/5

Thanks, fixed

in src/ipc/process.cpp:69 in 52917ad4be outdated

65+                std::string& address,
66+                std::string& error) override;
67+    int bind(const fs::path& data_dir, const std::string& exe_name, std::string& address, std::string& error) override;
68 };
69+
70+static bool ParseAddress(std::string& address,

Sjors commented at 6:24 pm on August 12, 2024:

52917ad4be1ca574543e854fdc154e88d65ce57f: this could use a test.

Or maybe we can introduce something akin to CNetAddr for socket addresses? Fortunately this input can only be provided by the user, so we don’t have to be overzealous about sanitising.

ryanofsky commented at 3:44 pm on August 19, 2024:

re: #30509 (review)

Added a new parse_address_test to give this function good test coverage. I think adding a new address data type could make sense in the future if more functions use addresses, but for now only connect and listen functions use them, so adding an intermediate type now doesn’t seem as simple as just passing address strings.

Sjors commented at 6:29 pm on August 12, 2024: member

ACK 4137419b0170fa1e9ff9daacde57933f7c995b76

I only lightly looked at the new ProcessImpl method bodies.

I tested that I’m able to connect the bitcoin-mine demo #30437, but that (as expected) it immediately fails with a “remote exception: Method not implemented”.

ryanofsky force-pushed on Aug 19, 2024

ryanofsky commented at 6:29 pm on August 19, 2024: contributor

Updated 4137419b0170fa1e9ff9daacde57933f7c995b76 -> af24810eeed397c2fe5f61ef9769e1b7ee0ecdf9 (pr/ipc-bind.4 -> pr/ipc-bind.5, compare) with suggested changes simplifying error handling and adding more tests.

Sjors commented at 9:11 am on August 20, 2024: member

re-ACK af24810eeed397c2fe5f61ef9769e1b7ee0ecdf9

in src/test/ipc_test.cpp:145 in 637924109b outdated

140+    // directory path is so long that the default socket address and any other
141+    // addresses in the data directory would fail with errors like:
142+    //   Address 'unix' path '"/tmp/test_common_Bitcoin Core/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff/test_bitcoin.sock"' exceeded maximum socket path length
143+    std::vector<std::string> addresses{
144+       strprintf("unix:%s", TempPath("bitcoin_sock0_XXXXXX")),
145+       strprintf("unix:%s", TempPath("bitcoin_sock1_XXXXXX")),

TheCharlatan commented at 12:03 pm on August 27, 2024:

Nit: There is a problem with the indentation here.

ryanofsky commented at 3:44 am on September 6, 2024:

re: #30509 (review)

Nit: There is a problem with the indentation here.

Thanks! Fixed

in src/common/args.cpp:638 in af24810eee outdated

616@@ -617,6 +617,9 @@ std::string ArgsManager::GetHelpMessage() const
617             case OptionsCategory::RPC:
618                 usage += HelpMessageGroup("RPC server options:");
619                 break;
620+            case OptionsCategory::IPC:

TheCharlatan commented at 8:19 pm on August 27, 2024:

I think this should be added to the rpc_help.py tests.

ryanofsky commented at 3:45 am on September 6, 2024:

re: #30509 (review)

I think this should be added to the rpc_help.py tests.

Is there mistake in this suggestion? This shouldn’t be related to RPC

TheCharlatan commented at 6:34 am on September 6, 2024:

Huh, no idea what I was thinking here, but rpc_help.py is obviously wrong. Maybe I copy-pasted a wrong value? I think where it could go is the system.cpp fuzz tests.

in src/test/ipc_test.cpp:122 in af24810eee outdated

116+void IpcSocketTest(const fs::path& datadir)
117+{
118+    std::unique_ptr<interfaces::Init> init{std::make_unique<TestInit>()};
119+    std::unique_ptr<ipc::Protocol> protocol{ipc::capnp::MakeCapnpProtocol()};
120+    std::unique_ptr<ipc::Process> process{ipc::MakeProcess()};
121+

TheCharlatan commented at 8:41 pm on August 27, 2024:

I think it would be good to sanity-check that invalid addresses make bind and connect throw:

 0diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp
 1index f6addb2697..a5f6efbfb7 100644
 2--- a/src/test/ipc_test.cpp
 3+++ b/src/test/ipc_test.cpp
 4@@ -16,0 +17 @@
 5+#include <stdexcept>
 6@@ -121,0 +123,4 @@ void IpcSocketTest(const fs::path& datadir)
 7+    std::string invalid_bind{"invalid:"};
 8+    BOOST_CHECK_THROW(process->bind(datadir, "test_bitcoin", invalid_bind), std::invalid_argument);
 9+    BOOST_CHECK_THROW(process->connect(datadir, "test_bitcoin", invalid_bind), std::invalid_argument);
10+

ryanofsky commented at 3:46 am on September 6, 2024:

re: #30509 (review)

I think it would be good to sanity-check that invalid addresses make bind and connect throw:

Thanks added suggested tests.

TheCharlatan approved

TheCharlatan commented at 8:44 pm on August 27, 2024: contributor

ACK af24810eeed397c2fe5f61ef9769e1b7ee0ecdf9

This looks good to me. Thanks for following up on my earlier suggestions, I left a couple more. It would be nice to add some functional tests too, but that is easily done in a follow-up.

DrahtBot added the label Needs rebase on Sep 2, 2024

ryanofsky force-pushed on Sep 6, 2024

ryanofsky commented at 3:50 am on September 6, 2024: contributor

Rebased af24810eeed397c2fe5f61ef9769e1b7ee0ecdf9 -> e225f7cbc3e6842f8e7f1c482c2aacd810e99c1b (pr/ipc-bind.5 -> pr/ipc-bind.6, compare) implementing suggestions and updating to use cmake

re: #30509#pullrequestreview-2263113816

It would be nice to add some functional tests too, but that is easily done in a follow-up.

Yeah it is a little harder to do now because this is only adding a bitcoin-node socket listening option, and there isn’t an executable right now that connects to this socket. But #30437 would be an easier place to add this because it adds an executable that can connect.

DrahtBot removed the label Needs rebase on Sep 6, 2024

TheCharlatan approved

TheCharlatan commented at 6:37 am on September 6, 2024: contributor

Re-ACK e225f7cbc3e6842f8e7f1c482c2aacd810e99c1b

DrahtBot requested review from Sjors on Sep 6, 2024

Sjors commented at 8:45 am on September 6, 2024: member

re-utACK e225f7cbc3e6842f8e7f1c482c2aacd810e99c1b

I plan to test this with https://github.com/Sjors/bitcoin/pull/48 but atm the rebases involved are a bit too much headache.

depends: Update libmultiprocess library for CustomMessage function and ThreadContext bugfix

The CustomMessage functions allow simplifying custom IPC type code, and the
bugfix is needed to prevent in a crash in a new test which creates and destroys
connections in a loop. Upstream PRs are:

https://github.com/chaincodelabs/libmultiprocess/pull/105 types: Add Custom{Build,Read,Pass}Message hooks
https://github.com/chaincodelabs/libmultiprocess/pull/106 Bugfix: Clean up ThreadContext pointers when Connection is destroyed

4da20434d4

multiprocess: Add IPC connectAddress and listenAddress methods

Allow listening on and connecting to unix sockets.

955d4077aa

multiprocess: Add unit tests for connect, serve, and listen functions 73fe7d7230

multiprocess: Add -ipcbind option to bitcoin-node

Add `-ipcbind` option to `bitcoin-node` to listen on an IPC socket and accept
connections from other processes. In the future, there will be an `-ipcconnect`
option added to `bitcoin-wallet` and `bitcoin-node` to allow wallet and gui
processes to connect to the node and access it.

Example usage:

    src/bitcoin-node -regtest -debug -ipcbind=unix
    src/bitcoin-wallet -regtest -ipcconnect=unix info
    src/bitcoin-gui -regtest -ipcconnect=unix
    src/bitcoin-mine -regtest -ipcconnect=unix

30073e6b3a

ryanofsky force-pushed on Sep 6, 2024

ryanofsky commented at 3:09 pm on September 6, 2024: contributor

Rebased e225f7cbc3e6842f8e7f1c482c2aacd810e99c1b -> 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d (pr/ipc-bind.6 -> pr/ipc-bind.7, compare) with no changes other than updating system.cpp fuzz test

TheCharlatan approved

TheCharlatan commented at 7:58 pm on September 6, 2024: contributor

Re-ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d

DrahtBot added the label CI failed on Sep 6, 2024

itornaza approved

itornaza commented at 6:07 pm on September 9, 2024: contributor

Code review ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d

Followed step by step the implementation through the code and especially the src/ipc/* changed files and it looks solid. Great that it provides yet another step to facilitate the stratum v2 integration into core.

achow101 commented at 9:06 pm on September 9, 2024: member

ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d

achow101 merged this on Sep 9, 2024

achow101 closed this on Sep 9, 2024

Sjors commented at 6:52 am on September 10, 2024: member

Post-merge re-utACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d

in src/ipc/process.cpp:117 in 30073e6b3a

112+    if (::connect(fd, (struct sockaddr*)&addr, sizeof(addr)) == 0) {
113+        return fd;
114+    }
115+    int connect_error = errno;
116+    if (::close(fd) != 0) {
117+        LogPrintf("Error closing file descriptor %i '%s': %s\n", fd, address, SysErrorString(errno));

maflcko commented at 9:20 am on September 10, 2024:

style nit for LogPrintf("Error here and below:

For new code, it would be good to use a non-deprecated log function (like LogInfo, LogWarning, or LogError). Otherwise, there may be confusion whether the info severity was picked on purpose or if something more suitable should be used instead.

ryanofsky commented at 1:41 pm on September 10, 2024:

For new code

Thanks, this code was written 2018-08-23

multiprocess: Add -ipcbind option to bitcoin-node #30509

Code Coverage

Reviews

Conflicts