From 794940469e77b82462db33011fa4d5e8f533e543 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 16 Apr 2026 15:09:29 -0400 Subject: [PATCH 01/11] ipc, moveonly: combine ipc_test.cpp and ipc_tests.cpp Previously ipc_test.cpp contained tests which depended on libmultiprocess and ipc_tests.cpp contained tests which didn't. Separation was needed because libmultiprocess tests need to be built with additional include and link paths, and cmake only has good support for setting these on libraries, not source files. The separation also allowed the add_boost_test custom cmake function to work with no changes, because it could find the boost test registration in ipc_tests.cpp, and then ipc_tests.cpp would run the tests in ipc_test.cpp without them needing to be registered in boost. But with windows support being added, the parse address test can't easily avoid a dependecy on libmultiprocess, because it depends on the ipc/process.h header, and ipc/process.h header will now need platform-specific ProcessId and SocketId types defined by libmultiprocess, rather than plain ints. With all ipc tests depending on libmultiprocess, there is not really a rationale for having separate test files anymore, so this change combines them, and move the cmake add_boost_test function definition so it can be used instead of target_sources to register ipc_tests.cpp with ctest. The change prevents CI errors from including ipc/process.h in ipc_tests.cpp: In file included from /Users/runner/work/bitcoin/bitcoin/repo_archive/src/ipc/test/ipc_tests.cpp:5: In file included from /Users/runner/work/bitcoin/bitcoin/repo_archive/src/ipc/process.h:11: /Users/runner/work/bitcoin/bitcoin/repo_archive/src/ipc/util.h:14:10: fatal error: 'kj/debug.h' file not found 14 | #include https://github.com/bitcoin/bitcoin/actions/runs/24465865499/job/71492617687?pr=35084 --- src/ipc/CMakeLists.txt | 4 +- src/ipc/test/CMakeLists.txt | 9 +- src/ipc/test/ipc_test.cpp | 193 ------------------------------------ src/ipc/test/ipc_test.h | 4 - src/ipc/test/ipc_tests.cpp | 187 +++++++++++++++++++++++++++++++++- src/test/CMakeLists.txt | 16 +-- 6 files changed, 198 insertions(+), 215 deletions(-) delete mode 100644 src/ipc/test/ipc_test.cpp diff --git a/src/ipc/CMakeLists.txt b/src/ipc/CMakeLists.txt index e9bdf0b39b1..caf1776e1fa 100644 --- a/src/ipc/CMakeLists.txt +++ b/src/ipc/CMakeLists.txt @@ -31,8 +31,8 @@ if(BUILD_TESTS) # compiler only allows importing by relative path when the importing and # imported files are underneath the same compilation source prefix, so the # source prefix must be src/ipc, not src/ipc/test/ - add_library(bitcoin_ipc_test STATIC EXCLUDE_FROM_ALL - test/ipc_test.cpp + add_library(bitcoin_ipc_test OBJECT EXCLUDE_FROM_ALL + test/ipc_tests.cpp ) target_capnp_sources(bitcoin_ipc_test ${CMAKE_CURRENT_SOURCE_DIR} test/ipc_test.capnp diff --git a/src/ipc/test/CMakeLists.txt b/src/ipc/test/CMakeLists.txt index e71bc2bc6a1..91aa2a5e512 100644 --- a/src/ipc/test/CMakeLists.txt +++ b/src/ipc/test/CMakeLists.txt @@ -2,11 +2,6 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or https://opensource.org/license/mit/. -# Do not use generator expressions in test sources because the -# SOURCES property is processed to gather test suite macros. -target_sources(test_bitcoin - PRIVATE - ipc_tests.cpp -) - target_link_libraries(test_bitcoin bitcoin_ipc_test bitcoin_ipc) + +add_boost_test(${CMAKE_CURRENT_SOURCE_DIR}/ipc_tests.cpp) diff --git a/src/ipc/test/ipc_test.cpp b/src/ipc/test/ipc_test.cpp deleted file mode 100644 index d5c689501da..00000000000 --- a/src/ipc/test/ipc_test.cpp +++ /dev/null @@ -1,193 +0,0 @@ -// Copyright (c) 2023-present The Bitcoin Core developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#include -#include -#include -#include -#include -#include - -#include - -static_assert(ipc::capnp::messages::MAX_MONEY == MAX_MONEY); -static_assert(ipc::capnp::messages::MAX_DOUBLE == std::numeric_limits::max()); -static_assert(ipc::capnp::messages::DEFAULT_BLOCK_RESERVED_WEIGHT == DEFAULT_BLOCK_RESERVED_WEIGHT); -static_assert(ipc::capnp::messages::DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS == DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS); - -//! Remote init class. -class TestInit : public interfaces::Init -{ -public: - std::unique_ptr makeEcho() override { return interfaces::MakeEcho(); } -}; - -//! Generate a temporary path with temp_directory_path and mkstemp -static std::string TempPath(std::string_view pattern) -{ - std::string temp{fs::PathToString(fs::path{fs::temp_directory_path()} / fs::PathFromString(std::string{pattern}))}; - temp.push_back('\0'); - int fd{mkstemp(temp.data())}; - BOOST_CHECK_GE(fd, 0); - BOOST_CHECK_EQUAL(close(fd), 0); - temp.resize(temp.size() - 1); - fs::remove(fs::PathFromString(temp)); - return temp; -} - -//! Unit test that tests execution of IPC calls without actually creating a -//! separate process. This test is primarily intended to verify behavior of type -//! conversion code that converts C++ objects to Cap'n Proto messages and vice -//! versa. -//! -//! The test creates a thread which creates a FooImplementation object (defined -//! in ipc_test.h) and a two-way pipe accepting IPC requests which call methods -//! on the object through FooInterface (defined in ipc_test.capnp). -void IpcPipeTest() -{ - // Setup: create FooImplementation object and listen for FooInterface requests - std::promise>> foo_promise; - std::thread thread([&]() { - mp::EventLoop loop("IpcPipeTest", [](bool raise, const std::string& log) { LogInfo("LOG%i: %s", raise, log); }); - auto pipe = loop.m_io_context.provider->newTwoWayPipe(); - - auto connection_client = std::make_unique(loop, kj::mv(pipe.ends[0])); - auto foo_client = std::make_unique>( - connection_client->m_rpc_system->bootstrap(mp::ServerVatId().vat_id).castAs(), - connection_client.get(), /* destroy_connection= */ true); - (void)connection_client.release(); - foo_promise.set_value(std::move(foo_client)); - - auto connection_server = std::make_unique(loop, kj::mv(pipe.ends[1]), [&](mp::Connection& connection) { - auto foo_server = kj::heap>(std::make_shared(), connection); - return capnp::Capability::Client(kj::mv(foo_server)); - }); - connection_server->onDisconnect([&] { connection_server.reset(); }); - loop.loop(); - }); - std::unique_ptr> foo{foo_promise.get_future().get()}; - - // Test: make sure arguments were sent and return value is received - BOOST_CHECK_EQUAL(foo->add(1, 2), 3); - - COutPoint txout1{Txid::FromUint256(uint256{100}), 200}; - COutPoint txout2{foo->passOutPoint(txout1)}; - BOOST_CHECK(txout1 == txout2); - - UniValue uni1{UniValue::VOBJ}; - uni1.pushKV("i", 1); - uni1.pushKV("s", "two"); - UniValue uni2{foo->passUniValue(uni1)}; - BOOST_CHECK_EQUAL(uni1.write(), uni2.write()); - - CMutableTransaction mtx; - mtx.version = 2; - mtx.nLockTime = 3; - mtx.vin.emplace_back(txout1); - mtx.vout.emplace_back(COIN, CScript()); - CTransactionRef tx1{MakeTransactionRef(mtx)}; - CTransactionRef tx2{foo->passTransaction(tx1)}; - BOOST_CHECK(*Assert(tx1) == *Assert(tx2)); - - std::vector txs1; - txs1.push_back(tx1); - txs1.push_back(nullptr); - std::vector txs2(foo->passTransactions(txs1)); - BOOST_CHECK_EQUAL(txs2.size(), 2); - BOOST_CHECK(*Assert(txs1[0]) == *Assert(txs2[0])); - BOOST_CHECK(!txs2[1]); - - std::vector vec1{'H', 'e', 'l', 'l', 'o'}; - std::vector vec2{foo->passVectorChar(vec1)}; - BOOST_CHECK_EQUAL(std::string_view(vec1.begin(), vec1.end()), std::string_view(vec2.begin(), vec2.end())); - - auto script1{CScript() << OP_11}; - auto script2{foo->passScript(script1)}; - BOOST_CHECK_EQUAL(HexStr(script1), HexStr(script2)); - - // Test cleanup: disconnect and join thread - foo.reset(); - thread.join(); -} - -//! Test ipc::Protocol connect() and serve() methods connecting over a socketpair. -void IpcSocketPairTest() -{ - int fds[2]; - BOOST_CHECK_EQUAL(socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); - std::unique_ptr init{std::make_unique()}; - std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol()}; - std::promise promise; - std::thread thread([&]() { - protocol->serve(fds[0], "test-serve", *init, [&] { promise.set_value(); }); - }); - promise.get_future().wait(); - std::unique_ptr remote_init{protocol->connect(fds[1], "test-connect")}; - std::unique_ptr remote_echo{remote_init->makeEcho()}; - BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test"); - remote_echo.reset(); - remote_init.reset(); - thread.join(); -} - -//! Test ipc::Process bind() and connect() methods connecting over a unix socket. -void IpcSocketTest(const fs::path& datadir) -{ - std::unique_ptr init{std::make_unique()}; - std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol()}; - std::unique_ptr process{ipc::MakeProcess()}; - - std::string invalid_bind{"invalid:"}; - BOOST_CHECK_THROW(process->bind(datadir, "test_bitcoin", invalid_bind), std::invalid_argument); - BOOST_CHECK_THROW(process->connect(datadir, "test_bitcoin", invalid_bind), std::invalid_argument); - - auto bind_and_listen{[&](const std::string& bind_address) { - std::string address{bind_address}; - int serve_fd = process->bind(datadir, "test_bitcoin", address); - BOOST_CHECK_GE(serve_fd, 0); - BOOST_CHECK_EQUAL(address, bind_address); - protocol->listen(serve_fd, "test-serve", *init); - }}; - - auto connect_and_test{[&](const std::string& connect_address) { - std::string address{connect_address}; - int connect_fd{process->connect(datadir, "test_bitcoin", address)}; - BOOST_CHECK_EQUAL(address, connect_address); - std::unique_ptr remote_init{protocol->connect(connect_fd, "test-connect")}; - std::unique_ptr remote_echo{remote_init->makeEcho()}; - BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test"); - }}; - - // Need to specify explicit socket addresses outside the data directory, because the data - // directory path is so long that the default socket address and any other - // addresses in the data directory would fail with errors like: - // Address 'unix' path '"/tmp/test_common_Bitcoin Core/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff/test_bitcoin.sock"' exceeded maximum socket path length - std::vector addresses{ - strprintf("unix:%s", TempPath("bitcoin_sock0_XXXXXX")), - strprintf("unix:%s", TempPath("bitcoin_sock1_XXXXXX")), - }; - - // Bind and listen on multiple addresses - for (const auto& address : addresses) { - bind_and_listen(address); - } - - // Connect and test each address multiple times. - for (int i : {0, 1, 0, 0, 1}) { - connect_and_test(addresses[i]); - } -} diff --git a/src/ipc/test/ipc_test.h b/src/ipc/test/ipc_test.h index 392f2b48826..c0a81150809 100644 --- a/src/ipc/test/ipc_test.h +++ b/src/ipc/test/ipc_test.h @@ -24,8 +24,4 @@ public: CScript passScript(CScript s) { return s; } }; -void IpcPipeTest(); -void IpcSocketPairTest(); -void IpcSocketTest(const fs::path& datadir); - #endif // BITCOIN_IPC_TEST_IPC_TEST_H diff --git a/src/ipc/test/ipc_tests.cpp b/src/ipc/test/ipc_tests.cpp index ebe4b397afa..b261792058e 100644 --- a/src/ipc/test/ipc_tests.cpp +++ b/src/ipc/test/ipc_tests.cpp @@ -2,13 +2,198 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include +#include +#include #include +#include +#include +#include #include - +#include #include #include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + #include +static_assert(ipc::capnp::messages::MAX_MONEY == MAX_MONEY); +static_assert(ipc::capnp::messages::MAX_DOUBLE == std::numeric_limits::max()); +static_assert(ipc::capnp::messages::DEFAULT_BLOCK_RESERVED_WEIGHT == DEFAULT_BLOCK_RESERVED_WEIGHT); +static_assert(ipc::capnp::messages::DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS == DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS); + +//! Remote init class. +class TestInit : public interfaces::Init +{ +public: + std::unique_ptr makeEcho() override { return interfaces::MakeEcho(); } +}; + +//! Generate a temporary path with temp_directory_path and mkstemp +static std::string TempPath(std::string_view pattern) +{ + std::string temp{fs::PathToString(fs::path{fs::temp_directory_path()} / fs::PathFromString(std::string{pattern}))}; + temp.push_back('\0'); + int fd{mkstemp(temp.data())}; + BOOST_CHECK_GE(fd, 0); + BOOST_CHECK_EQUAL(close(fd), 0); + temp.resize(temp.size() - 1); + fs::remove(fs::PathFromString(temp)); + return temp; +} + +//! Unit test that tests execution of IPC calls without actually creating a +//! separate process. This test is primarily intended to verify behavior of type +//! conversion code that converts C++ objects to Cap'n Proto messages and vice +//! versa. +//! +//! The test creates a thread which creates a FooImplementation object (defined +//! in ipc_test.h) and a two-way pipe accepting IPC requests which call methods +//! on the object through FooInterface (defined in ipc_test.capnp). +void IpcPipeTest() +{ + // Setup: create FooImplementation object and listen for FooInterface requests + std::promise>> foo_promise; + std::thread thread([&]() { + mp::EventLoop loop("IpcPipeTest", [](bool raise, const std::string& log) { LogInfo("LOG%i: %s", raise, log); }); + auto pipe = loop.m_io_context.provider->newTwoWayPipe(); + + auto connection_client = std::make_unique(loop, kj::mv(pipe.ends[0])); + auto foo_client = std::make_unique>( + connection_client->m_rpc_system->bootstrap(mp::ServerVatId().vat_id).castAs(), + connection_client.get(), /* destroy_connection= */ true); + (void)connection_client.release(); + foo_promise.set_value(std::move(foo_client)); + + auto connection_server = std::make_unique(loop, kj::mv(pipe.ends[1]), [&](mp::Connection& connection) { + auto foo_server = kj::heap>(std::make_shared(), connection); + return capnp::Capability::Client(kj::mv(foo_server)); + }); + connection_server->onDisconnect([&] { connection_server.reset(); }); + loop.loop(); + }); + std::unique_ptr> foo{foo_promise.get_future().get()}; + + // Test: make sure arguments were sent and return value is received + BOOST_CHECK_EQUAL(foo->add(1, 2), 3); + + COutPoint txout1{Txid::FromUint256(uint256{100}), 200}; + COutPoint txout2{foo->passOutPoint(txout1)}; + BOOST_CHECK(txout1 == txout2); + + UniValue uni1{UniValue::VOBJ}; + uni1.pushKV("i", 1); + uni1.pushKV("s", "two"); + UniValue uni2{foo->passUniValue(uni1)}; + BOOST_CHECK_EQUAL(uni1.write(), uni2.write()); + + CMutableTransaction mtx; + mtx.version = 2; + mtx.nLockTime = 3; + mtx.vin.emplace_back(txout1); + mtx.vout.emplace_back(COIN, CScript()); + CTransactionRef tx1{MakeTransactionRef(mtx)}; + CTransactionRef tx2{foo->passTransaction(tx1)}; + BOOST_CHECK(*Assert(tx1) == *Assert(tx2)); + + std::vector txs1; + txs1.push_back(tx1); + txs1.push_back(nullptr); + std::vector txs2(foo->passTransactions(txs1)); + BOOST_CHECK_EQUAL(txs2.size(), 2); + BOOST_CHECK(*Assert(txs1[0]) == *Assert(txs2[0])); + BOOST_CHECK(!txs2[1]); + + std::vector vec1{'H', 'e', 'l', 'l', 'o'}; + std::vector vec2{foo->passVectorChar(vec1)}; + BOOST_CHECK_EQUAL(std::string_view(vec1.begin(), vec1.end()), std::string_view(vec2.begin(), vec2.end())); + + auto script1{CScript() << OP_11}; + auto script2{foo->passScript(script1)}; + BOOST_CHECK_EQUAL(HexStr(script1), HexStr(script2)); + + // Test cleanup: disconnect and join thread + foo.reset(); + thread.join(); +} + +//! Test ipc::Protocol connect() and serve() methods connecting over a socketpair. +void IpcSocketPairTest() +{ + int fds[2]; + BOOST_CHECK_EQUAL(socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); + std::unique_ptr init{std::make_unique()}; + std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol()}; + std::promise promise; + std::thread thread([&]() { + protocol->serve(fds[0], "test-serve", *init, [&] { promise.set_value(); }); + }); + promise.get_future().wait(); + std::unique_ptr remote_init{protocol->connect(fds[1], "test-connect")}; + std::unique_ptr remote_echo{remote_init->makeEcho()}; + BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test"); + remote_echo.reset(); + remote_init.reset(); + thread.join(); +} + +//! Test ipc::Process bind() and connect() methods connecting over a unix socket. +void IpcSocketTest(const fs::path& datadir) +{ + std::unique_ptr init{std::make_unique()}; + std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol()}; + std::unique_ptr process{ipc::MakeProcess()}; + + std::string invalid_bind{"invalid:"}; + BOOST_CHECK_THROW(process->bind(datadir, "test_bitcoin", invalid_bind), std::invalid_argument); + BOOST_CHECK_THROW(process->connect(datadir, "test_bitcoin", invalid_bind), std::invalid_argument); + + auto bind_and_listen{[&](const std::string& bind_address) { + std::string address{bind_address}; + int serve_fd = process->bind(datadir, "test_bitcoin", address); + BOOST_CHECK_GE(serve_fd, 0); + BOOST_CHECK_EQUAL(address, bind_address); + protocol->listen(serve_fd, "test-serve", *init); + }}; + + auto connect_and_test{[&](const std::string& connect_address) { + std::string address{connect_address}; + int connect_fd{process->connect(datadir, "test_bitcoin", address)}; + BOOST_CHECK_EQUAL(address, connect_address); + std::unique_ptr remote_init{protocol->connect(connect_fd, "test-connect")}; + std::unique_ptr remote_echo{remote_init->makeEcho()}; + BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test"); + }}; + + // Need to specify explicit socket addresses outside the data directory, because the data + // directory path is so long that the default socket address and any other + // addresses in the data directory would fail with errors like: + // Address 'unix' path '"/tmp/test_common_Bitcoin Core/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff/test_bitcoin.sock"' exceeded maximum socket path length + std::vector addresses{ + strprintf("unix:%s", TempPath("bitcoin_sock0_XXXXXX")), + strprintf("unix:%s", TempPath("bitcoin_sock1_XXXXXX")), + }; + + // Bind and listen on multiple addresses + for (const auto& address : addresses) { + bind_and_listen(address); + } + + // Connect and test each address multiple times. + for (int i : {0, 1, 0, 0, 1}) { + connect_and_test(addresses[i]); + } +} + BOOST_FIXTURE_TEST_SUITE(ipc_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(ipc_tests) { diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt index b5a12f7277d..41fd0526ff9 100644 --- a/src/test/CMakeLists.txt +++ b/src/test/CMakeLists.txt @@ -170,14 +170,6 @@ target_link_libraries(test_bitcoin $ ) -if(ENABLE_WALLET) - add_subdirectory(${PROJECT_SOURCE_DIR}/src/wallet/test wallet) -endif() - -if(ENABLE_IPC) - add_subdirectory(${PROJECT_SOURCE_DIR}/src/ipc/test ipc) -endif() - function(add_boost_test source_file) if(NOT EXISTS ${source_file}) return() @@ -216,6 +208,14 @@ function(add_all_test_targets) endforeach() endfunction() +if(ENABLE_WALLET) + add_subdirectory(${PROJECT_SOURCE_DIR}/src/wallet/test wallet) +endif() + +if(ENABLE_IPC) + add_subdirectory(${PROJECT_SOURCE_DIR}/src/ipc/test ipc) +endif() + add_all_test_targets() install_binary_component(test_bitcoin INTERNAL) From 33d37f3c35efaac136863253b91799bf2711fd46 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 30 Apr 2025 08:39:29 -0400 Subject: [PATCH 02/11] ipc, refactor: Drop connect/listen/serve exe_name parameters Pass exe_name parameter to ipc::Protocol class constructor instead. It never really made sense to have exe parameters as part of the protocol interface and removing them makes adding new features like windows support easier. The exe name values are only used for logging and debuggging purposes to distinguish log messages from different processes. --- src/ipc/capnp/protocol.cpp | 22 ++++++++++++---------- src/ipc/capnp/protocol.h | 2 +- src/ipc/interfaces.cpp | 10 +++++----- src/ipc/protocol.h | 6 +++--- src/ipc/test/ipc_tests.cpp | 12 ++++++------ 5 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp index e39c5039da9..7fc893fc385 100644 --- a/src/ipc/capnp/protocol.cpp +++ b/src/ipc/capnp/protocol.cpp @@ -71,34 +71,35 @@ void IpcLogFn(mp::LogMessage message) class CapnpProtocol : public Protocol { public: + CapnpProtocol(const char* exe_name) : m_exe_name{exe_name} {} ~CapnpProtocol() noexcept(true) { m_loop_ref.reset(); if (m_loop_thread.joinable()) m_loop_thread.join(); assert(!m_loop); }; - std::unique_ptr connect(int fd, const char* exe_name) override + std::unique_ptr connect(int fd) override { - startLoop(exe_name); + startLoop(); return mp::ConnectStream(*m_loop, fd); } - void listen(int listen_fd, const char* exe_name, interfaces::Init& init) override + void listen(int listen_fd, interfaces::Init& init) override { - startLoop(exe_name); + startLoop(); if (::listen(listen_fd, /*backlog=*/5) != 0) { throw std::system_error(errno, std::system_category()); } mp::ListenConnections(*m_loop, listen_fd, init); } - void serve(int fd, const char* exe_name, interfaces::Init& init, const std::function& ready_fn = {}) override + void serve(int fd, interfaces::Init& init, const std::function& ready_fn = {}) override { assert(!m_loop); - mp::g_thread_context.thread_name = mp::ThreadName(exe_name); + mp::g_thread_context.thread_name = mp::ThreadName(m_exe_name); mp::LogOptions opts = { .log_fn = IpcLogFn, .log_level = GetRequestedIPCLogLevel() }; - m_loop.emplace(exe_name, std::move(opts), &m_context); + m_loop.emplace(m_exe_name, std::move(opts), &m_context); if (ready_fn) ready_fn(); mp::ServeStream(*m_loop, fd, init); m_parent_connection = &m_loop->m_incoming_connections.back(); @@ -120,7 +121,7 @@ public: mp::ProxyTypeRegister::types().at(type)(iface).cleanup_fns.emplace_back(std::move(cleanup)); } Context& context() override { return m_context; } - void startLoop(const char* exe_name) + void startLoop() { if (m_loop) return; std::promise promise; @@ -130,7 +131,7 @@ public: .log_fn = IpcLogFn, .log_level = GetRequestedIPCLogLevel() }; - m_loop.emplace(exe_name, std::move(opts), &m_context); + m_loop.emplace(m_exe_name, std::move(opts), &m_context); m_loop_ref.emplace(*m_loop); promise.set_value(); m_loop->loop(); @@ -138,6 +139,7 @@ public: }); promise.get_future().wait(); } + const char* m_exe_name; Context m_context; std::thread m_loop_thread; //! EventLoop object which manages I/O events for all connections. @@ -151,6 +153,6 @@ public: }; } // namespace -std::unique_ptr MakeCapnpProtocol() { return std::make_unique(); } +std::unique_ptr MakeCapnpProtocol(const char* exe_name) { return std::make_unique(exe_name); } } // namespace capnp } // namespace ipc diff --git a/src/ipc/capnp/protocol.h b/src/ipc/capnp/protocol.h index 54a8ae6efc2..ef828cd4ef5 100644 --- a/src/ipc/capnp/protocol.h +++ b/src/ipc/capnp/protocol.h @@ -10,7 +10,7 @@ namespace ipc { class Protocol; namespace capnp { -std::unique_ptr MakeCapnpProtocol(); +std::unique_ptr MakeCapnpProtocol(const char* exe_name); } // namespace capnp } // namespace ipc diff --git a/src/ipc/interfaces.cpp b/src/ipc/interfaces.cpp index 32febd35526..3345a92172e 100644 --- a/src/ipc/interfaces.cpp +++ b/src/ipc/interfaces.cpp @@ -54,7 +54,7 @@ class IpcImpl : public interfaces::Ipc public: IpcImpl(const char* exe_name, const char* process_argv0, interfaces::Init& init) : m_exe_name(exe_name), m_process_argv0(process_argv0), m_init(init), - m_protocol(ipc::capnp::MakeCapnpProtocol()), m_process(ipc::MakeProcess()) + m_protocol(ipc::capnp::MakeCapnpProtocol(exe_name)), m_process(ipc::MakeProcess()) { } std::unique_ptr spawnProcess(const char* new_exe_name) override @@ -62,7 +62,7 @@ public: int pid; int fd = m_process->spawn(new_exe_name, m_process_argv0, pid); LogDebug(::BCLog::IPC, "Process %s pid %i launched\n", new_exe_name, pid); - auto init = m_protocol->connect(fd, m_exe_name); + auto init = m_protocol->connect(fd); Ipc::addCleanup(*init, [this, new_exe_name, pid] { int status = m_process->waitSpawned(pid); LogDebug(::BCLog::IPC, "Process %s pid %i exited with status %i\n", new_exe_name, pid, status); @@ -77,7 +77,7 @@ public: return false; } IgnoreCtrlC(strprintf("[%s] SIGINT received — waiting for parent to shut down.\n", m_exe_name)); - m_protocol->serve(fd, m_exe_name, m_init); + m_protocol->serve(fd, m_init); exit_status = EXIT_SUCCESS; return true; } @@ -106,12 +106,12 @@ public: } else { fd = m_process->connect(gArgs.GetDataDirNet(), "bitcoin-node", address); } - return m_protocol->connect(fd, m_exe_name); + return m_protocol->connect(fd); } void listenAddress(std::string& address) override { int fd = m_process->bind(gArgs.GetDataDirNet(), m_exe_name, address); - m_protocol->listen(fd, m_exe_name, m_init); + m_protocol->listen(fd, m_init); } void disconnectIncoming() override { diff --git a/src/ipc/protocol.h b/src/ipc/protocol.h index e7d66887a1a..d14c7f20013 100644 --- a/src/ipc/protocol.h +++ b/src/ipc/protocol.h @@ -32,12 +32,12 @@ public: //! up its own state (calling ProxyServer destructors, etc) on disconnect, //! and any client calls will just throw ipc::Exception errors after a //! disconnect. - virtual std::unique_ptr connect(int fd, const char* exe_name) = 0; + virtual std::unique_ptr connect(int fd) = 0; //! Listen for connections on provided socket descriptor, accept them, and //! handle requests on accepted connections. This method doesn't block, and //! performs I/O on a background thread. - virtual void listen(int listen_fd, const char* exe_name, interfaces::Init& init) = 0; + virtual void listen(int listen_fd, interfaces::Init& init) = 0; //! Handle requests on provided socket descriptor, forwarding them to the //! provided Init interface. Socket communication is handled on the @@ -56,7 +56,7 @@ public: //! client connections from another thread as soon as the event loop is //! available, but should not be necessary in normal code which starts //! clients and servers independently. - virtual void serve(int fd, const char* exe_name, interfaces::Init& init, const std::function& ready_fn = {}) = 0; + virtual void serve(int fd, interfaces::Init& init, const std::function& ready_fn = {}) = 0; //! Disconnect any incoming connections that are still connected. virtual void disconnectIncoming() = 0; diff --git a/src/ipc/test/ipc_tests.cpp b/src/ipc/test/ipc_tests.cpp index b261792058e..75b9544a147 100644 --- a/src/ipc/test/ipc_tests.cpp +++ b/src/ipc/test/ipc_tests.cpp @@ -132,13 +132,13 @@ void IpcSocketPairTest() int fds[2]; BOOST_CHECK_EQUAL(socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); std::unique_ptr init{std::make_unique()}; - std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol()}; + std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol("IpcSocketPairTest")}; std::promise promise; std::thread thread([&]() { - protocol->serve(fds[0], "test-serve", *init, [&] { promise.set_value(); }); + protocol->serve(fds[0], *init, [&] { promise.set_value(); }); }); promise.get_future().wait(); - std::unique_ptr remote_init{protocol->connect(fds[1], "test-connect")}; + std::unique_ptr remote_init{protocol->connect(fds[1])}; std::unique_ptr remote_echo{remote_init->makeEcho()}; BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test"); remote_echo.reset(); @@ -150,7 +150,7 @@ void IpcSocketPairTest() void IpcSocketTest(const fs::path& datadir) { std::unique_ptr init{std::make_unique()}; - std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol()}; + std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol("IpcSocketTest")}; std::unique_ptr process{ipc::MakeProcess()}; std::string invalid_bind{"invalid:"}; @@ -162,14 +162,14 @@ void IpcSocketTest(const fs::path& datadir) int serve_fd = process->bind(datadir, "test_bitcoin", address); BOOST_CHECK_GE(serve_fd, 0); BOOST_CHECK_EQUAL(address, bind_address); - protocol->listen(serve_fd, "test-serve", *init); + protocol->listen(serve_fd, *init); }}; auto connect_and_test{[&](const std::string& connect_address) { std::string address{connect_address}; int connect_fd{process->connect(datadir, "test_bitcoin", address)}; BOOST_CHECK_EQUAL(address, connect_address); - std::unique_ptr remote_init{protocol->connect(connect_fd, "test-connect")}; + std::unique_ptr remote_init{protocol->connect(connect_fd)}; std::unique_ptr remote_echo{remote_init->makeEcho()}; BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test"); }}; From 00287b9a34047a7ae75aa128d3b0f05feab06098 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 30 Apr 2025 08:39:29 -0400 Subject: [PATCH 03/11] ipc, refactor: Change Protocol class field order This just changes Protocol class field order to make sure class members are not destroyed before the event loop thread exits. There is no change in behavior. The change is just being made to clarify intent and avoid potential bugs. --- src/ipc/capnp/protocol.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp index 7fc893fc385..ca183603e73 100644 --- a/src/ipc/capnp/protocol.cpp +++ b/src/ipc/capnp/protocol.cpp @@ -141,7 +141,6 @@ public: } const char* m_exe_name; Context m_context; - std::thread m_loop_thread; //! EventLoop object which manages I/O events for all connections. std::optional m_loop; //! Reference to the same EventLoop. Increments the loop’s refcount on @@ -150,6 +149,7 @@ public: std::optional m_loop_ref; //! Connection to parent, if this is a child process spawned by a parent process. mp::Connection* m_parent_connection{nullptr}; + std::thread m_loop_thread; }; } // namespace From 7c86d4834ed2767049b7c9a62456ad45dcbb572b Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 30 Apr 2025 08:39:29 -0400 Subject: [PATCH 04/11] ipc, refactor: use native path separators in test Avoid hardcoded forward slashes is ParseAddress test, use native path separators instead. --- src/ipc/test/ipc_tests.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/ipc/test/ipc_tests.cpp b/src/ipc/test/ipc_tests.cpp index 75b9544a147..08f2aa06e4d 100644 --- a/src/ipc/test/ipc_tests.cpp +++ b/src/ipc/test/ipc_tests.cpp @@ -216,12 +216,13 @@ BOOST_AUTO_TEST_CASE(parse_address_test) } BOOST_CHECK_EQUAL(address, expect_address); }}; - check_address("unix", "unix:/var/empty/notexist/test_bitcoin.sock", ""); - check_address("unix:", "unix:/var/empty/notexist/test_bitcoin.sock", ""); - check_address("unix:path.sock", "unix:/var/empty/notexist/path.sock", ""); + std::string prefix{fs::PathToString(datadir / "")}; + check_address("unix", "unix:" + prefix + "test_bitcoin.sock", ""); + check_address("unix:", "unix:" + prefix + "test_bitcoin.sock", ""); + check_address("unix:path.sock", "unix:" + prefix + "path.sock", ""); check_address("unix:0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.sock", - "unix:/var/empty/notexist/0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.sock", - "Unix address path \"/var/empty/notexist/0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.sock\" exceeded maximum socket path length"); + "unix:" + prefix + "0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.sock", + "Unix address path \"" + prefix + "0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.sock\" exceeded maximum socket path length"); check_address("invalid", "invalid", "Unrecognized address 'invalid'"); } From dbcc192dce6147f1397baa72d5dc98fde9528638 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 30 Apr 2025 08:39:29 -0400 Subject: [PATCH 05/11] ipc, refactor: fix include order Keep standard headers separate from posix headers --- src/ipc/process.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ipc/process.cpp b/src/ipc/process.cpp index b60718e9931..dcde50e220a 100644 --- a/src/ipc/process.cpp +++ b/src/ipc/process.cpp @@ -18,11 +18,12 @@ #include #include #include +#include +#include + #include #include #include -#include -#include using util::RemovePrefixView; From 344979714189bb7cab20f77dd23256c08e235b84 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Sun, 21 Jun 2026 20:47:58 -0400 Subject: [PATCH 06/11] ipc: Avoid 'unistd.h' error with MSVC Avoid compile error from MSVC: D:\a\bitcoin\bitcoin\src\ipc\interfaces.cpp(24,1): error C1083: Cannot open include file: 'unistd.h': No such file or directory MinGW provides this header but MSVC does not. Header is unneeded on windows because HandleCtrlC code that uses it is not compiled on windows. --- src/ipc/interfaces.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ipc/interfaces.cpp b/src/ipc/interfaces.cpp index 3345a92172e..71f1f4ee499 100644 --- a/src/ipc/interfaces.cpp +++ b/src/ipc/interfaces.cpp @@ -21,10 +21,13 @@ #include #include #include -#include #include #include +#ifndef WIN32 +#include +#endif + namespace ipc { namespace { #ifndef WIN32 From 2ee9b69c7a1f27f4e5dbe94a55fb62cd636d205a Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 30 Apr 2025 08:39:29 -0400 Subject: [PATCH 07/11] ipc, refactor: Add ProcessId type alias and use it Use ProcessId type instead of int to represent process ids to be compatible with an upcoming version of libmultiprocess which adds windows support. --- src/ipc/interfaces.cpp | 2 +- src/ipc/process.cpp | 4 ++-- src/ipc/process.h | 5 +++-- src/ipc/util.h | 21 +++++++++++++++++++++ 4 files changed, 27 insertions(+), 5 deletions(-) create mode 100644 src/ipc/util.h diff --git a/src/ipc/interfaces.cpp b/src/ipc/interfaces.cpp index 71f1f4ee499..73a616bfb95 100644 --- a/src/ipc/interfaces.cpp +++ b/src/ipc/interfaces.cpp @@ -62,7 +62,7 @@ public: } std::unique_ptr spawnProcess(const char* new_exe_name) override { - int pid; + mp::ProcessId pid; int fd = m_process->spawn(new_exe_name, m_process_argv0, pid); LogDebug(::BCLog::IPC, "Process %s pid %i launched\n", new_exe_name, pid); auto init = m_protocol->connect(fd); diff --git a/src/ipc/process.cpp b/src/ipc/process.cpp index dcde50e220a..ec658a9af26 100644 --- a/src/ipc/process.cpp +++ b/src/ipc/process.cpp @@ -32,7 +32,7 @@ namespace { class ProcessImpl : public Process { public: - int spawn(const std::string& new_exe_name, const fs::path& argv0_path, int& pid) override + int spawn(const std::string& new_exe_name, const fs::path& argv0_path, mp::ProcessId& pid) override { return mp::SpawnProcess(pid, [&](int fd) { fs::path path = argv0_path; @@ -41,7 +41,7 @@ public: return std::vector{fs::PathToString(path), "-ipcfd", strprintf("%i", fd)}; }); } - int waitSpawned(int pid) override { return mp::WaitProcess(pid); } + int waitSpawned(mp::ProcessId pid) override { return mp::WaitProcess(pid); } bool checkSpawned(int argc, char* argv[], int& fd) override { // If this process was not started with a single -ipcfd argument, it is diff --git a/src/ipc/process.h b/src/ipc/process.h index 67c69593abd..13a004d7ca3 100644 --- a/src/ipc/process.h +++ b/src/ipc/process.h @@ -8,6 +8,7 @@ #include #include +#include #include namespace ipc { @@ -25,10 +26,10 @@ public: //! Spawn process and return socket file descriptor for communicating with //! it. - virtual int spawn(const std::string& new_exe_name, const fs::path& argv0_path, int& pid) = 0; + virtual int spawn(const std::string& new_exe_name, const fs::path& argv0_path, mp::ProcessId& pid) = 0; //! Wait for spawned process to exit and return its exit code. - virtual int waitSpawned(int pid) = 0; + virtual int waitSpawned(mp::ProcessId pid) = 0; //! Parse command line and determine if current process is a spawned child //! process. If so, return true and a file descriptor for communicating diff --git a/src/ipc/util.h b/src/ipc/util.h new file mode 100644 index 00000000000..58912a8575d --- /dev/null +++ b/src/ipc/util.h @@ -0,0 +1,21 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_IPC_UTIL_H +#define BITCOIN_IPC_UTIL_H + +#include +#include +#include + +namespace mp { +// Definitions that can be deleted when libmultiprocess subtree is updated to +// v14. Having these allows Bitcoin Core changes to be decoupled from +// libmultiprocess changes so they don't have to be reviewed in a single PR. +#if MP_MAJOR_VERSION < 14 +using ProcessId = int; +#endif +} // namespace mp + +#endif // BITCOIN_IPC_UTIL_H From 3859805f05e6fb642140c89e6d7378203e04df2c Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 30 Apr 2025 08:39:29 -0400 Subject: [PATCH 08/11] ipc, refactor: Add SocketId type alias and use it Use SocketId type instead of int to represent socket ids to be compatible with an upcoming version of libmultiprocess which adds windows support. --- src/ipc/capnp/protocol.cpp | 10 +++++----- src/ipc/interfaces.cpp | 12 ++++++------ src/ipc/process.cpp | 22 +++++++++++----------- src/ipc/process.h | 17 ++++++++--------- src/ipc/protocol.h | 11 ++++++----- src/ipc/test/ipc_tests.cpp | 8 ++++---- src/ipc/util.h | 2 ++ 7 files changed, 42 insertions(+), 40 deletions(-) diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp index ca183603e73..0790986d90b 100644 --- a/src/ipc/capnp/protocol.cpp +++ b/src/ipc/capnp/protocol.cpp @@ -78,12 +78,12 @@ public: if (m_loop_thread.joinable()) m_loop_thread.join(); assert(!m_loop); }; - std::unique_ptr connect(int fd) override + std::unique_ptr connect(mp::SocketId socket) override { startLoop(); - return mp::ConnectStream(*m_loop, fd); + return mp::ConnectStream(*m_loop, socket); } - void listen(int listen_fd, interfaces::Init& init) override + void listen(mp::SocketId listen_fd, interfaces::Init& init) override { startLoop(); if (::listen(listen_fd, /*backlog=*/5) != 0) { @@ -91,7 +91,7 @@ public: } mp::ListenConnections(*m_loop, listen_fd, init); } - void serve(int fd, interfaces::Init& init, const std::function& ready_fn = {}) override + void serve(mp::SocketId socket, interfaces::Init& init, const std::function& ready_fn = {}) override { assert(!m_loop); mp::g_thread_context.thread_name = mp::ThreadName(m_exe_name); @@ -101,7 +101,7 @@ public: }; m_loop.emplace(m_exe_name, std::move(opts), &m_context); if (ready_fn) ready_fn(); - mp::ServeStream(*m_loop, fd, init); + mp::ServeStream(*m_loop, socket, init); m_parent_connection = &m_loop->m_incoming_connections.back(); m_loop->loop(); m_loop.reset(); diff --git a/src/ipc/interfaces.cpp b/src/ipc/interfaces.cpp index 73a616bfb95..66b5e8ec4f6 100644 --- a/src/ipc/interfaces.cpp +++ b/src/ipc/interfaces.cpp @@ -63,7 +63,7 @@ public: std::unique_ptr spawnProcess(const char* new_exe_name) override { mp::ProcessId pid; - int fd = m_process->spawn(new_exe_name, m_process_argv0, pid); + mp::SocketId fd = m_process->spawn(new_exe_name, m_process_argv0, pid); LogDebug(::BCLog::IPC, "Process %s pid %i launched\n", new_exe_name, pid); auto init = m_protocol->connect(fd); Ipc::addCleanup(*init, [this, new_exe_name, pid] { @@ -75,19 +75,19 @@ public: bool startSpawnedProcess(int argc, char* argv[], int& exit_status) override { exit_status = EXIT_FAILURE; - int32_t fd = -1; - if (!m_process->checkSpawned(argc, argv, fd)) { + mp::SocketId socket{mp::SocketError}; + if (!m_process->checkSpawned(argc, argv, socket)) { return false; } IgnoreCtrlC(strprintf("[%s] SIGINT received — waiting for parent to shut down.\n", m_exe_name)); - m_protocol->serve(fd, m_init); + m_protocol->serve(socket, m_init); exit_status = EXIT_SUCCESS; return true; } std::unique_ptr connectAddress(std::string& address) override { if (address.empty() || address == "0") return nullptr; - int fd; + mp::SocketId fd; if (address == "auto") { // Treat "auto" the same as "unix" except don't treat it an as error // if the connection is not accepted. Just return null so the caller @@ -113,7 +113,7 @@ public: } void listenAddress(std::string& address) override { - int fd = m_process->bind(gArgs.GetDataDirNet(), m_exe_name, address); + mp::SocketId fd = m_process->bind(gArgs.GetDataDirNet(), m_exe_name, address); m_protocol->listen(fd, m_init); } void disconnectIncoming() override diff --git a/src/ipc/process.cpp b/src/ipc/process.cpp index ec658a9af26..d7f040780d5 100644 --- a/src/ipc/process.cpp +++ b/src/ipc/process.cpp @@ -32,7 +32,7 @@ namespace { class ProcessImpl : public Process { public: - int spawn(const std::string& new_exe_name, const fs::path& argv0_path, mp::ProcessId& pid) override + mp::SocketId spawn(const std::string& new_exe_name, const fs::path& argv0_path, mp::ProcessId& pid) override { return mp::SpawnProcess(pid, [&](int fd) { fs::path path = argv0_path; @@ -42,7 +42,7 @@ public: }); } int waitSpawned(mp::ProcessId pid) override { return mp::WaitProcess(pid); } - bool checkSpawned(int argc, char* argv[], int& fd) override + bool checkSpawned(int argc, char* argv[], mp::SocketId& socket) override { // If this process was not started with a single -ipcfd argument, it is // not a process spawned by the spawn() call above, so return false and @@ -60,13 +60,13 @@ public: if (!maybe_fd) { throw std::runtime_error(strprintf("Invalid -ipcfd number '%s'", argv[2])); } - fd = *maybe_fd; + socket = *maybe_fd; return true; } - int connect(const fs::path& data_dir, + mp::SocketId connect(const fs::path& data_dir, const std::string& dest_exe_name, std::string& address) override; - int bind(const fs::path& data_dir, const std::string& exe_name, std::string& address) override; + mp::SocketId bind(const fs::path& data_dir, const std::string& exe_name, std::string& address) override; }; static bool ParseAddress(std::string& address, @@ -98,7 +98,7 @@ static bool ParseAddress(std::string& address, return false; } -int ProcessImpl::connect(const fs::path& data_dir, +mp::SocketId ProcessImpl::connect(const fs::path& data_dir, const std::string& dest_exe_name, std::string& address) { @@ -108,8 +108,8 @@ int ProcessImpl::connect(const fs::path& data_dir, throw std::invalid_argument(error); } - int fd; - if ((fd = ::socket(addr.sun_family, SOCK_STREAM, 0)) == -1) { + mp::SocketId fd; + if ((fd = ::socket(addr.sun_family, SOCK_STREAM, 0)) == mp::SocketError) { throw std::system_error(errno, std::system_category()); } if (::connect(fd, (struct sockaddr*)&addr, sizeof(addr)) == 0) { @@ -122,7 +122,7 @@ int ProcessImpl::connect(const fs::path& data_dir, throw std::system_error(connect_error, std::system_category()); } -int ProcessImpl::bind(const fs::path& data_dir, const std::string& exe_name, std::string& address) +mp::SocketId ProcessImpl::bind(const fs::path& data_dir, const std::string& exe_name, std::string& address) { struct sockaddr_un addr; std::string error; @@ -138,8 +138,8 @@ int ProcessImpl::bind(const fs::path& data_dir, const std::string& exe_name, std } } - int fd; - if ((fd = ::socket(addr.sun_family, SOCK_STREAM, 0)) == -1) { + mp::SocketId fd; + if ((fd = ::socket(addr.sun_family, SOCK_STREAM, 0)) == mp::SocketError) { throw std::system_error(errno, std::system_category()); } diff --git a/src/ipc/process.h b/src/ipc/process.h index 13a004d7ca3..54ca204cd55 100644 --- a/src/ipc/process.h +++ b/src/ipc/process.h @@ -24,25 +24,24 @@ class Process public: virtual ~Process() = default; - //! Spawn process and return socket file descriptor for communicating with - //! it. - virtual int spawn(const std::string& new_exe_name, const fs::path& argv0_path, mp::ProcessId& pid) = 0; + //! Spawn process and return socket id for communicating with it. + virtual mp::SocketId spawn(const std::string& new_exe_name, const fs::path& argv0_path, mp::ProcessId& pid) = 0; //! Wait for spawned process to exit and return its exit code. virtual int waitSpawned(mp::ProcessId pid) = 0; //! Parse command line and determine if current process is a spawned child - //! process. If so, return true and a file descriptor for communicating + //! process. If so, return true and a socket id for communicating //! with the parent process. - virtual bool checkSpawned(int argc, char* argv[], int& fd) = 0; + virtual bool checkSpawned(int argc, char* argv[], mp::SocketId& socket) = 0; - //! Canonicalize and connect to address, returning socket descriptor. - virtual int connect(const fs::path& data_dir, + //! Canonicalize and connect to address, returning socket id. + virtual mp::SocketId connect(const fs::path& data_dir, const std::string& dest_exe_name, std::string& address) = 0; - //! Create listening socket, bind and canonicalize address, and return socket descriptor. - virtual int bind(const fs::path& data_dir, + //! Create listening socket, bind and canonicalize address, and return socket id. + virtual mp::SocketId bind(const fs::path& data_dir, const std::string& exe_name, std::string& address) = 0; }; diff --git a/src/ipc/protocol.h b/src/ipc/protocol.h index d14c7f20013..4c3c1bdc8d9 100644 --- a/src/ipc/protocol.h +++ b/src/ipc/protocol.h @@ -6,6 +6,7 @@ #define BITCOIN_IPC_PROTOCOL_H #include +#include #include #include @@ -32,12 +33,12 @@ public: //! up its own state (calling ProxyServer destructors, etc) on disconnect, //! and any client calls will just throw ipc::Exception errors after a //! disconnect. - virtual std::unique_ptr connect(int fd) = 0; + virtual std::unique_ptr connect(mp::SocketId fd) = 0; - //! Listen for connections on provided socket descriptor, accept them, and - //! handle requests on accepted connections. This method doesn't block, and + //! Listen for connections on provided socket id, accept them, and handle + //! requests on accepted connections. This method doesn't block, and //! performs I/O on a background thread. - virtual void listen(int listen_fd, interfaces::Init& init) = 0; + virtual void listen(mp::SocketId listen_fd, interfaces::Init& init) = 0; //! Handle requests on provided socket descriptor, forwarding them to the //! provided Init interface. Socket communication is handled on the @@ -56,7 +57,7 @@ public: //! client connections from another thread as soon as the event loop is //! available, but should not be necessary in normal code which starts //! clients and servers independently. - virtual void serve(int fd, interfaces::Init& init, const std::function& ready_fn = {}) = 0; + virtual void serve(mp::SocketId fd, interfaces::Init& init, const std::function& ready_fn = {}) = 0; //! Disconnect any incoming connections that are still connected. virtual void disconnectIncoming() = 0; diff --git a/src/ipc/test/ipc_tests.cpp b/src/ipc/test/ipc_tests.cpp index 08f2aa06e4d..f09c2d161d3 100644 --- a/src/ipc/test/ipc_tests.cpp +++ b/src/ipc/test/ipc_tests.cpp @@ -129,7 +129,7 @@ void IpcPipeTest() //! Test ipc::Protocol connect() and serve() methods connecting over a socketpair. void IpcSocketPairTest() { - int fds[2]; + mp::SocketId fds[2]; BOOST_CHECK_EQUAL(socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); std::unique_ptr init{std::make_unique()}; std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol("IpcSocketPairTest")}; @@ -159,15 +159,15 @@ void IpcSocketTest(const fs::path& datadir) auto bind_and_listen{[&](const std::string& bind_address) { std::string address{bind_address}; - int serve_fd = process->bind(datadir, "test_bitcoin", address); - BOOST_CHECK_GE(serve_fd, 0); + mp::SocketId serve_fd = process->bind(datadir, "test_bitcoin", address); + BOOST_CHECK_NE(serve_fd, mp::SocketError); BOOST_CHECK_EQUAL(address, bind_address); protocol->listen(serve_fd, *init); }}; auto connect_and_test{[&](const std::string& connect_address) { std::string address{connect_address}; - int connect_fd{process->connect(datadir, "test_bitcoin", address)}; + mp::SocketId connect_fd{process->connect(datadir, "test_bitcoin", address)}; BOOST_CHECK_EQUAL(address, connect_address); std::unique_ptr remote_init{protocol->connect(connect_fd)}; std::unique_ptr remote_echo{remote_init->makeEcho()}; diff --git a/src/ipc/util.h b/src/ipc/util.h index 58912a8575d..5e591d7b21c 100644 --- a/src/ipc/util.h +++ b/src/ipc/util.h @@ -15,6 +15,8 @@ namespace mp { // libmultiprocess changes so they don't have to be reviewed in a single PR. #if MP_MAJOR_VERSION < 14 using ProcessId = int; +using SocketId = int; +constexpr SocketId SocketError{-1}; #endif } // namespace mp From e9f19815caa3e129020f116bef43d409cb50a475 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 30 Apr 2025 08:39:29 -0400 Subject: [PATCH 09/11] ipc, refactor: Add Stream type alias and use it Use Stream type to abstract socket ids and be compatible with updated mp::ConnectStream() and mp::ServeStream() functions that use streams instead of socket ids in an upcoming version of libmultiprocess which adds windows support. Since creating Stream objects from socket ids can require the event loop to be running, the ipc::Protocol::serve() method is also updated to accept the server stream though a callback parameter instead of a normal parameter. --- src/ipc/capnp/protocol.cpp | 14 +++++++++----- src/ipc/interfaces.cpp | 6 +++--- src/ipc/protocol.h | 38 +++++++++++++++++++------------------- src/ipc/test/ipc_tests.cpp | 14 +++++++++----- src/ipc/util.h | 17 +++++++++++++++++ 5 files changed, 57 insertions(+), 32 deletions(-) diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp index 0790986d90b..64b1e9a9f19 100644 --- a/src/ipc/capnp/protocol.cpp +++ b/src/ipc/capnp/protocol.cpp @@ -78,10 +78,10 @@ public: if (m_loop_thread.joinable()) m_loop_thread.join(); assert(!m_loop); }; - std::unique_ptr connect(mp::SocketId socket) override + std::unique_ptr connect(mp::Stream stream) override { startLoop(); - return mp::ConnectStream(*m_loop, socket); + return mp::ConnectStream(*m_loop, std::move(stream)); } void listen(mp::SocketId listen_fd, interfaces::Init& init) override { @@ -91,7 +91,7 @@ public: } mp::ListenConnections(*m_loop, listen_fd, init); } - void serve(mp::SocketId socket, interfaces::Init& init, const std::function& ready_fn = {}) override + void serve(interfaces::Init& init, const std::function& make_stream) override { assert(!m_loop); mp::g_thread_context.thread_name = mp::ThreadName(m_exe_name); @@ -100,8 +100,7 @@ public: .log_level = GetRequestedIPCLogLevel() }; m_loop.emplace(m_exe_name, std::move(opts), &m_context); - if (ready_fn) ready_fn(); - mp::ServeStream(*m_loop, socket, init); + mp::ServeStream(*m_loop, make_stream(), init); m_parent_connection = &m_loop->m_incoming_connections.back(); m_loop->loop(); m_loop.reset(); @@ -116,6 +115,11 @@ public: m_loop->m_incoming_connections.remove_if([this](mp::Connection& c) { return &c != m_parent_connection; }); }); } + mp::Stream makeStream(mp::SocketId socket) override + { + startLoop(); + return mp::MakeStream(*m_loop, socket); + } void addCleanup(std::type_index type, void* iface, std::function cleanup) override { mp::ProxyTypeRegister::types().at(type)(iface).cleanup_fns.emplace_back(std::move(cleanup)); diff --git a/src/ipc/interfaces.cpp b/src/ipc/interfaces.cpp index 66b5e8ec4f6..75a854c2b70 100644 --- a/src/ipc/interfaces.cpp +++ b/src/ipc/interfaces.cpp @@ -65,7 +65,7 @@ public: mp::ProcessId pid; mp::SocketId fd = m_process->spawn(new_exe_name, m_process_argv0, pid); LogDebug(::BCLog::IPC, "Process %s pid %i launched\n", new_exe_name, pid); - auto init = m_protocol->connect(fd); + auto init = m_protocol->connect(m_protocol->makeStream(fd)); Ipc::addCleanup(*init, [this, new_exe_name, pid] { int status = m_process->waitSpawned(pid); LogDebug(::BCLog::IPC, "Process %s pid %i exited with status %i\n", new_exe_name, pid, status); @@ -80,7 +80,7 @@ public: return false; } IgnoreCtrlC(strprintf("[%s] SIGINT received — waiting for parent to shut down.\n", m_exe_name)); - m_protocol->serve(socket, m_init); + m_protocol->serve(m_init, [&] { return m_protocol->makeStream(socket); } ); exit_status = EXIT_SUCCESS; return true; } @@ -109,7 +109,7 @@ public: } else { fd = m_process->connect(gArgs.GetDataDirNet(), "bitcoin-node", address); } - return m_protocol->connect(fd); + return m_protocol->connect(m_protocol->makeStream(fd)); } void listenAddress(std::string& address) override { diff --git a/src/ipc/protocol.h b/src/ipc/protocol.h index 4c3c1bdc8d9..66aab9fba3b 100644 --- a/src/ipc/protocol.h +++ b/src/ipc/protocol.h @@ -24,8 +24,8 @@ class Protocol public: virtual ~Protocol() = default; - //! Return Init interface that forwards requests over given socket descriptor. - //! Socket communication is handled on a background thread. + //! Return Init interface that forwards requests over given connection + //! stream. Socket communication is handled on a background thread. //! //! @note It could be potentially useful in the future to add //! std::function on_disconnect callback argument here. But there @@ -33,31 +33,31 @@ public: //! up its own state (calling ProxyServer destructors, etc) on disconnect, //! and any client calls will just throw ipc::Exception errors after a //! disconnect. - virtual std::unique_ptr connect(mp::SocketId fd) = 0; + virtual std::unique_ptr connect(mp::Stream stream) = 0; //! Listen for connections on provided socket id, accept them, and handle //! requests on accepted connections. This method doesn't block, and //! performs I/O on a background thread. virtual void listen(mp::SocketId listen_fd, interfaces::Init& init) = 0; - //! Handle requests on provided socket descriptor, forwarding them to the - //! provided Init interface. Socket communication is handled on the - //! current thread, and this call blocks until the socket is closed. + //! Handle requests from a stream provided by the make_stream callback, + //! forwarding them to the provided Init interface. Socket communication is + //! handled on the current thread, and this call blocks until the socket is + //! closed. A callback is used to specify the stream because this method + //! initializes the event loop and it may not be possible to create the + //! stream before the event loop is initialized. //! - //! @note: If this method is called, it needs be called before connect() or - //! listen() methods, because for ease of implementation it's inflexible and - //! always runs the event loop in the foreground thread. It can share its - //! event loop with the other methods but can't share an event loop that was - //! created by them. This isn't really a problem because serve() is only - //! called by spawned child processes that call it immediately to + //! @note: If this method is called, it needs to be called before connect() + //! or listen() methods, because for ease of implementation this method is + //! inflexible and always runs the event loop in the foreground thread. It + //! can share its event loop with the other methods but can't share an event + //! loop that was created by them. This isn't a problem because serve() is + //! only called by spawned child processes that call it immediately to //! communicate back with parent processes. - // - //! The optional `ready_fn` callback will be called after the event loop is - //! created but before it is started. This can be useful in tests to trigger - //! client connections from another thread as soon as the event loop is - //! available, but should not be necessary in normal code which starts - //! clients and servers independently. - virtual void serve(mp::SocketId fd, interfaces::Init& init, const std::function& ready_fn = {}) = 0; + virtual void serve(interfaces::Init& init, const std::function& make_stream) = 0; + + //! Make stream object from socket id. + virtual mp::Stream makeStream(mp::SocketId socket) = 0; //! Disconnect any incoming connections that are still connected. virtual void disconnectIncoming() = 0; diff --git a/src/ipc/test/ipc_tests.cpp b/src/ipc/test/ipc_tests.cpp index f09c2d161d3..e353a7ee7c1 100644 --- a/src/ipc/test/ipc_tests.cpp +++ b/src/ipc/test/ipc_tests.cpp @@ -129,16 +129,20 @@ void IpcPipeTest() //! Test ipc::Protocol connect() and serve() methods connecting over a socketpair. void IpcSocketPairTest() { - mp::SocketId fds[2]; - BOOST_CHECK_EQUAL(socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); std::unique_ptr init{std::make_unique()}; std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol("IpcSocketPairTest")}; + mp::Stream client_stream; std::promise promise; std::thread thread([&]() { - protocol->serve(fds[0], *init, [&] { promise.set_value(); }); + protocol->serve(*init, [&] { + auto pair{mp::SocketPair()}; + client_stream = protocol->makeStream(pair[0]); + promise.set_value(); + return protocol->makeStream(pair[1]); + }); }); promise.get_future().wait(); - std::unique_ptr remote_init{protocol->connect(fds[1])}; + std::unique_ptr remote_init{protocol->connect(std::move(client_stream))}; std::unique_ptr remote_echo{remote_init->makeEcho()}; BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test"); remote_echo.reset(); @@ -169,7 +173,7 @@ void IpcSocketTest(const fs::path& datadir) std::string address{connect_address}; mp::SocketId connect_fd{process->connect(datadir, "test_bitcoin", address)}; BOOST_CHECK_EQUAL(address, connect_address); - std::unique_ptr remote_init{protocol->connect(connect_fd)}; + std::unique_ptr remote_init{protocol->connect(protocol->makeStream(connect_fd))}; std::unique_ptr remote_echo{remote_init->makeEcho()}; BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test"); }}; diff --git a/src/ipc/util.h b/src/ipc/util.h index 5e591d7b21c..ec36b418c71 100644 --- a/src/ipc/util.h +++ b/src/ipc/util.h @@ -5,18 +5,35 @@ #ifndef BITCOIN_IPC_UTIL_H #define BITCOIN_IPC_UTIL_H +#include #include +#include #include #include +#include namespace mp { // Definitions that can be deleted when libmultiprocess subtree is updated to // v14. Having these allows Bitcoin Core changes to be decoupled from // libmultiprocess changes so they don't have to be reviewed in a single PR. #if MP_MAJOR_VERSION < 14 +class EventLoop; using ProcessId = int; using SocketId = int; constexpr SocketId SocketError{-1}; + +using Stream = SocketId; +inline Stream MakeStream(EventLoop&, SocketId socket) +{ + return socket; +} + +inline std::array SocketPair() +{ + int pair[2]; + KJ_SYSCALL(socketpair(AF_UNIX, SOCK_STREAM, 0, pair)); + return {pair[0], pair[1]}; +} #endif } // namespace mp From 2d3f72fd3fa45ab4e399094c39bfa93bb5a87323 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 17 Jul 2026 08:28:00 -0400 Subject: [PATCH 10/11] ipc, refactor: Update mp::SpawnProcess call Use new SpawnProcess and StartSpawned functions to be compatible with an upcoming version of libmultiprocess which adds windows support. --- src/ipc/interfaces.cpp | 5 ++--- src/ipc/process.cpp | 14 +++++++------- src/ipc/process.h | 2 +- src/ipc/util.h | 18 ++++++++++++++++++ 4 files changed, 28 insertions(+), 11 deletions(-) diff --git a/src/ipc/interfaces.cpp b/src/ipc/interfaces.cpp index 75a854c2b70..40cddb4b662 100644 --- a/src/ipc/interfaces.cpp +++ b/src/ipc/interfaces.cpp @@ -62,10 +62,9 @@ public: } std::unique_ptr spawnProcess(const char* new_exe_name) override { - mp::ProcessId pid; - mp::SocketId fd = m_process->spawn(new_exe_name, m_process_argv0, pid); + const auto [pid, socket] = m_process->spawn(new_exe_name, m_process_argv0); LogDebug(::BCLog::IPC, "Process %s pid %i launched\n", new_exe_name, pid); - auto init = m_protocol->connect(m_protocol->makeStream(fd)); + auto init = m_protocol->connect(m_protocol->makeStream(socket)); Ipc::addCleanup(*init, [this, new_exe_name, pid] { int status = m_process->waitSpawned(pid); LogDebug(::BCLog::IPC, "Process %s pid %i exited with status %i\n", new_exe_name, pid, status); diff --git a/src/ipc/process.cpp b/src/ipc/process.cpp index d7f040780d5..a9aa47aedbc 100644 --- a/src/ipc/process.cpp +++ b/src/ipc/process.cpp @@ -32,13 +32,13 @@ namespace { class ProcessImpl : public Process { public: - mp::SocketId spawn(const std::string& new_exe_name, const fs::path& argv0_path, mp::ProcessId& pid) override + std::tuple spawn(const std::string& new_exe_name, const fs::path& argv0_path) override { - return mp::SpawnProcess(pid, [&](int fd) { + return mp::SpawnProcess([&](std::string connect_info) { fs::path path = argv0_path; path.remove_filename(); path /= fs::PathFromString(new_exe_name); - return std::vector{fs::PathToString(path), "-ipcfd", strprintf("%i", fd)}; + return std::vector{fs::PathToString(path), "-ipcfd", std::move(connect_info)}; }); } int waitSpawned(mp::ProcessId pid) override { return mp::WaitProcess(pid); } @@ -56,11 +56,11 @@ public: // in combination with other arguments because the parent process // should be able to control the child process through the IPC protocol // without passing information out of band. - const auto maybe_fd{ToIntegral(argv[2])}; - if (!maybe_fd) { - throw std::runtime_error(strprintf("Invalid -ipcfd number '%s'", argv[2])); + try { + socket = mp::StartSpawned(argv[2]); + } catch (const std::exception& e) { + throw std::runtime_error(strprintf("Invalid -ipcfd number '%s' (%s)", argv[2], e.what())); } - socket = *maybe_fd; return true; } mp::SocketId connect(const fs::path& data_dir, diff --git a/src/ipc/process.h b/src/ipc/process.h index 54ca204cd55..ac597cb042d 100644 --- a/src/ipc/process.h +++ b/src/ipc/process.h @@ -25,7 +25,7 @@ public: virtual ~Process() = default; //! Spawn process and return socket id for communicating with it. - virtual mp::SocketId spawn(const std::string& new_exe_name, const fs::path& argv0_path, mp::ProcessId& pid) = 0; + virtual std::tuple spawn(const std::string& new_exe_name, const fs::path& argv0_path) = 0; //! Wait for spawned process to exit and return its exit code. virtual int waitSpawned(mp::ProcessId pid) = 0; diff --git a/src/ipc/util.h b/src/ipc/util.h index ec36b418c71..3fd3ff160f3 100644 --- a/src/ipc/util.h +++ b/src/ipc/util.h @@ -5,8 +5,12 @@ #ifndef BITCOIN_IPC_UTIL_H #define BITCOIN_IPC_UTIL_H +#include +#include + #include #include +#include #include #include #include @@ -34,6 +38,20 @@ inline std::array SocketPair() KJ_SYSCALL(socketpair(AF_UNIX, SOCK_STREAM, 0, pair)); return {pair[0], pair[1]}; } + +inline std::tuple SpawnProcess(const std::function(std::string)>& spawn_argv) +{ + ProcessId pid; + SocketId socket = SpawnProcess(pid, [&](int fd) { return spawn_argv(strprintf("%d", fd)); }); + return {pid, socket}; +} + +inline SocketId StartSpawned(const std::string& connect_info) +{ + auto socket = ToIntegral(connect_info); + if (!socket) throw std::invalid_argument(strprintf("Invalid socket descriptor '%s'", connect_info)); + return *socket; +} #endif } // namespace mp From d3d74e701f761f21d276959c15fb91eaca3c5607 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 22 Jul 2026 11:22:09 -0400 Subject: [PATCH 11/11] ipc, refactor: Update mp::g_thread_context references Use new CurrentThread function to be compatible with windows mingw bug workaround https://github.com/bitcoin-core/libmultiprocess/pull/318 --- src/ipc/capnp/protocol.cpp | 2 +- src/ipc/test/fuzz/ipc.cpp | 9 +++++---- src/ipc/util.h | 6 ++++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp index 64b1e9a9f19..e7eaf643011 100644 --- a/src/ipc/capnp/protocol.cpp +++ b/src/ipc/capnp/protocol.cpp @@ -94,7 +94,7 @@ public: void serve(interfaces::Init& init, const std::function& make_stream) override { assert(!m_loop); - mp::g_thread_context.thread_name = mp::ThreadName(m_exe_name); + mp::CurrentThread().thread_name = mp::ThreadName(m_exe_name); mp::LogOptions opts = { .log_fn = IpcLogFn, .log_level = GetRequestedIPCLogLevel() diff --git a/src/ipc/test/fuzz/ipc.cpp b/src/ipc/test/fuzz/ipc.cpp index 1c19faf258a..5935e9afb51 100644 --- a/src/ipc/test/fuzz/ipc.cpp +++ b/src/ipc/test/fuzz/ipc.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -78,10 +79,10 @@ static void initialize_ipc() static const auto testing_setup = MakeNoLogFileContext<>(); (void)testing_setup; - // Ensure g_thread_context is destroyed after the IPC setup, since C++ - // destroys thread_local objects in reverse construction order. - mp::ThreadContext& thread_context{mp::g_thread_context}; - (void)thread_context; + // Ensure the thread's ThreadContext is created before the IPC setup, so + // it is destroyed after it, since C++ destroys thread_local objects in + // reverse construction order. + mp::CurrentThread(); thread_local static IpcFuzzSetup ipc; // NOLINT(bitcoin-nontrivial-threadlocal) g_ipc = &ipc; diff --git a/src/ipc/util.h b/src/ipc/util.h index 3fd3ff160f3..6352f981746 100644 --- a/src/ipc/util.h +++ b/src/ipc/util.h @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -52,6 +53,11 @@ inline SocketId StartSpawned(const std::string& connect_info) if (!socket) throw std::invalid_argument(strprintf("Invalid socket descriptor '%s'", connect_info)); return *socket; } + +inline ThreadContext& CurrentThread() +{ + return g_thread_context; +} #endif } // namespace mp