From 2221c8814d765c1e6109a6f0ecec00d5b4af2ec6 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Sun, 26 Jan 2025 14:13:47 -0500 Subject: [PATCH 1/2] depends: Update libmultiprocess library before converting to subtree This should be the final update to the libmultiprocess package via the depends system. It brings in the libmultiprocess cmake changes from https://github.com/chaincodelabs/libmultiprocess/pull/136 needed to support building as subtree. After this, a followup PR will add libmultiprocess as a git subtree and depends will just use the git subtree instead of hardcoding its own version hash. Since there have been libmultiprocess API changes since the last update, this commit also updates bitcoin code to be compatible with them. This update brings in the following changes: https://github.com/chaincodelabs/libmultiprocess/pull/121 ProxyClientBase: avoid static_cast to partially constructed object https://github.com/chaincodelabs/libmultiprocess/pull/120 proxy-types.h: add static_assert to detect int/enum size mismatch https://github.com/chaincodelabs/libmultiprocess/pull/127 ProxyClientBase: avoid static_cast to partially destructed object https://github.com/chaincodelabs/libmultiprocess/pull/129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races. https://github.com/chaincodelabs/libmultiprocess/pull/130 refactor: Add CleanupRun function to dedup clean list code https://github.com/chaincodelabs/libmultiprocess/pull/131 doc: fix startAsyncThread comment https://github.com/chaincodelabs/libmultiprocess/pull/133 Fix debian "libatomic not found" error in downstream builds https://github.com/chaincodelabs/libmultiprocess/pull/94 c++ 20 cleanups https://github.com/chaincodelabs/libmultiprocess/pull/135 refactor: proxy-types.h API cleanup https://github.com/chaincodelabs/libmultiprocess/pull/136 cmake: Support being included with add_subdirectory https://github.com/chaincodelabs/libmultiprocess/pull/137 doc: Fix broken markdown links --- depends/packages/native_libmultiprocess.mk | 4 +- src/ipc/capnp/common-types.h | 59 +++++----------------- src/ipc/capnp/echo-types.h | 12 +++++ src/ipc/capnp/echo.capnp | 2 +- src/ipc/capnp/protocol.cpp | 2 +- 5 files changed, 29 insertions(+), 50 deletions(-) create mode 100644 src/ipc/capnp/echo-types.h diff --git a/depends/packages/native_libmultiprocess.mk b/depends/packages/native_libmultiprocess.mk index 7c69e0f0c6..74d22207a0 100644 --- a/depends/packages/native_libmultiprocess.mk +++ b/depends/packages/native_libmultiprocess.mk @@ -1,8 +1,8 @@ package=native_libmultiprocess -$(package)_version=abe254b9734f2e2b220d1456de195532d6e6ac1e +$(package)_version=07c917f7ca910d66abc6d3873162fc9061704074 $(package)_download_path=https://github.com/chaincodelabs/libmultiprocess/archive $(package)_file_name=$($(package)_version).tar.gz -$(package)_sha256_hash=85777073259fdc75d24ac5777a19991ec1156c5f12db50b252b861c95dcb4f46 +$(package)_sha256_hash=ac9db311e3b22aac3c7b7b7b3f6b7fee5cf3043ebb3c3bf412049e8b17166de8 $(package)_dependencies=native_capnp define $(package)_config_cmds diff --git a/src/ipc/capnp/common-types.h b/src/ipc/capnp/common-types.h index 51af6a5f0a..934dae5faf 100644 --- a/src/ipc/capnp/common-types.h +++ b/src/ipc/capnp/common-types.h @@ -14,6 +14,19 @@ #include #include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include #include #include @@ -94,26 +107,6 @@ requires ipc::capnp::Deserializable return read_dest.construct(::deserialize, wrapper); } -//! Overload CustomBuildField and CustomReadField to serialize std::chrono -//! parameters and return values as numbers. -template -void CustomBuildField(TypeList>, Priority<1>, InvokeContext& invoke_context, Value&& value, - Output&& output) -{ - static_assert(std::numeric_limits::lowest() <= std::numeric_limits::lowest(), - "capnp type does not have enough range to hold lowest std::chrono::duration value"); - static_assert(std::numeric_limits::max() >= std::numeric_limits::max(), - "capnp type does not have enough range to hold highest std::chrono::duration value"); - output.set(value.count()); -} - -template -decltype(auto) CustomReadField(TypeList>, Priority<1>, InvokeContext& invoke_context, - Input&& input, ReadDest&& read_dest) -{ - return read_dest.construct(input.get()); -} - //! Overload CustomBuildField and CustomReadField to serialize UniValue //! parameters and return values as JSON strings. template @@ -134,32 +127,6 @@ decltype(auto) CustomReadField(TypeList, Priority<1>, InvokeContext& i }); } -//! Generic ::capnp::Data field builder for any C++ type that can be converted -//! to a span of bytes, like std::vector or std::array, or custom -//! blob types like uint256 or PKHash with data() and size() methods pointing to -//! bytes. -//! -//! Note: it might make sense to move this function into libmultiprocess, since -//! it is fairly generic. However this would require decreasing its priority so -//! it can be overridden, which would require more changes inside -//! libmultiprocess to avoid conflicting with the Priority<1> CustomBuildField -//! function it already provides for std::vector. Also, it might make sense to -//! provide a CustomReadField counterpart to this function, which could be -//! called to read C++ types that can be constructed from spans of bytes from -//! ::capnp::Data fields. But so far there hasn't been a need for this. -template -void CustomBuildField(TypeList, Priority<2>, InvokeContext& invoke_context, Value&& value, Output&& output) -requires - (std::is_same_v) && - (std::convertible_to> || - std::convertible_to> || - std::convertible_to> || - std::convertible_to>) -{ - auto data = std::span{value}; - auto result = output.init(data.size()); - memcpy(result.begin(), data.data(), data.size()); -} } // namespace mp #endif // BITCOIN_IPC_CAPNP_COMMON_TYPES_H diff --git a/src/ipc/capnp/echo-types.h b/src/ipc/capnp/echo-types.h new file mode 100644 index 0000000000..14970c1adb --- /dev/null +++ b/src/ipc/capnp/echo-types.h @@ -0,0 +1,12 @@ +// Copyright (c) 2025 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_CAPNP_ECHO_TYPES_H +#define BITCOIN_IPC_CAPNP_ECHO_TYPES_H + +#include +#include +#include + +#endif // BITCOIN_IPC_CAPNP_ECHO_TYPES_H diff --git a/src/ipc/capnp/echo.capnp b/src/ipc/capnp/echo.capnp index df36ee0de3..03c40f906e 100644 --- a/src/ipc/capnp/echo.capnp +++ b/src/ipc/capnp/echo.capnp @@ -9,7 +9,7 @@ $Cxx.namespace("ipc::capnp::messages"); using Proxy = import "/mp/proxy.capnp"; $Proxy.include("interfaces/echo.h"); -$Proxy.include("ipc/capnp/echo.capnp.h"); +$Proxy.includeTypes("ipc/capnp/echo-types.h"); interface Echo $Proxy.wrap("interfaces::Echo") { destroy @0 (context :Proxy.Context) -> (); diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp index 4b67a5bd1e..691bdf5f92 100644 --- a/src/ipc/capnp/protocol.cpp +++ b/src/ipc/capnp/protocol.cpp @@ -73,7 +73,7 @@ public: } void addCleanup(std::type_index type, void* iface, std::function cleanup) override { - mp::ProxyTypeRegister::types().at(type)(iface).cleanup.emplace_back(std::move(cleanup)); + 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) From 4e0aa1835b3e980ceda29ec90e7115d7fef53f51 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 24 Jan 2025 19:01:49 -0500 Subject: [PATCH 2/2] test: Add test for IPC serialization bug Add regression test for serialization bug in IPC mining code that is not currently being called anywhere reported: https://github.com/Sjors/bitcoin/issues/71 https://github.com/chaincodelabs/libmultiprocess/issues/122 --- src/test/ipc_test.capnp | 1 + src/test/ipc_test.cpp | 4 ++++ src/test/ipc_test.h | 2 ++ 3 files changed, 7 insertions(+) diff --git a/src/test/ipc_test.capnp b/src/test/ipc_test.capnp index 46cd08b94a..7fd59cf588 100644 --- a/src/test/ipc_test.capnp +++ b/src/test/ipc_test.capnp @@ -20,4 +20,5 @@ interface FooInterface $Proxy.wrap("FooImplementation") { passTransaction @3 (arg :Data) -> (result :Data); passVectorChar @4 (arg :Data) -> (result :Data); passBlockState @5 (arg :Mining.BlockValidationState) -> (result :Mining.BlockValidationState); + passScript @6 (arg :Data) -> (result :Data); } diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp index af37434980..d78d6695e7 100644 --- a/src/test/ipc_test.cpp +++ b/src/test/ipc_test.cpp @@ -121,6 +121,10 @@ void IpcPipeTest() BOOST_CHECK_EQUAL(bs3.GetRejectReason(), bs4.GetRejectReason()); BOOST_CHECK_EQUAL(bs3.GetDebugMessage(), bs4.GetDebugMessage()); + auto script1{CScript() << OP_11}; + auto script2{foo->passScript(script1)}; + BOOST_CHECK_EQUAL(HexStr(script1), HexStr(script2)); + // Test cleanup: disconnect pipe and join thread disconnect_client(); thread.join(); diff --git a/src/test/ipc_test.h b/src/test/ipc_test.h index 2d215a20f1..2304d8a4c0 100644 --- a/src/test/ipc_test.h +++ b/src/test/ipc_test.h @@ -6,6 +6,7 @@ #define BITCOIN_TEST_IPC_TEST_H #include +#include