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

pull ryanofsky wants to merge 4 commits into bitcoin:master from ryanofsky:pr/ipc-bind changing 21 files +359 −18
  1. 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.

  2. 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.

  3. 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.
  4. 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

  5. ryanofsky force-pushed on Jul 24, 2024
  6. 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.

  7. 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

  8. 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.

  9. 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.
  10. 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)

  11. 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

  12. 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.

  13. TheCharlatan commented at 3:45 pm on August 7, 2024: contributor
    lgtm , I hope we can finally get this project under way now.
  14. ryanofsky force-pushed on Aug 7, 2024
  15. 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

  16. 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!

  17. 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.

  18. DrahtBot added the label CI failed on Aug 7, 2024
  19. 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.

  20. ryanofsky referenced this in commit febe6d2492 on Aug 9, 2024
  21. DrahtBot removed the label CI failed on Aug 9, 2024
  22. ryanofsky referenced this in commit 8ba0d03b44 on Aug 9, 2024
  23. ryanofsky referenced this in commit c1b4ab4eb8 on Aug 9, 2024
  24. ryanofsky force-pushed on Aug 9, 2024
  25. 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)
  26. 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
  27. 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
  28. 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.

  29. 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.
  30. 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.

  31. 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

  32. 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

  33. 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.

  34. 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”.

  35. ryanofsky force-pushed on Aug 19, 2024
  36. 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.
  37. Sjors commented at 9:11 am on August 20, 2024: member
    re-ACK af24810eeed397c2fe5f61ef9769e1b7ee0ecdf9
  38. 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

  39. 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.
  40. 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.

  41. TheCharlatan approved
  42. 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.

  43. DrahtBot added the label Needs rebase on Sep 2, 2024
  44. ryanofsky force-pushed on Sep 6, 2024
  45. 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.

  46. DrahtBot removed the label Needs rebase on Sep 6, 2024
  47. TheCharlatan approved
  48. TheCharlatan commented at 6:37 am on September 6, 2024: contributor
    Re-ACK e225f7cbc3e6842f8e7f1c482c2aacd810e99c1b
  49. DrahtBot requested review from Sjors on Sep 6, 2024
  50. 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.

  51. 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
  52. multiprocess: Add IPC connectAddress and listenAddress methods
    Allow listening on and connecting to unix sockets.
    955d4077aa
  53. multiprocess: Add unit tests for connect, serve, and listen functions 73fe7d7230
  54. 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
  55. ryanofsky force-pushed on Sep 6, 2024
  56. 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
  57. TheCharlatan approved
  58. TheCharlatan commented at 7:58 pm on September 6, 2024: contributor
    Re-ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d
  59. DrahtBot added the label CI failed on Sep 6, 2024
  60. itornaza approved
  61. 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.

  62. achow101 commented at 9:06 pm on September 9, 2024: member
    ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d
  63. achow101 merged this on Sep 9, 2024
  64. achow101 closed this on Sep 9, 2024

  65. Sjors commented at 6:52 am on September 10, 2024: member
    Post-merge re-utACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d
  66. 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


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-18 04:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me