From ef92c3065cab1e5361d6f9161e5f7b376c3978fa Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Mon, 12 Apr 2021 23:31:13 +0200 Subject: [PATCH 01/12] Merge #21663: ci: Fix macOS brew install command b7381552cd4f965c45f1560d9cfc2fb09dbfcc1d ci: Fix macOS brew install command (Hennadii Stepanov) Pull request description: A solution for https://bintray.com shutdown. Details: https://github.com/Homebrew/discussions/discussions/691. ACKs for top commit: jamesob: ACK https://github.com/bitcoin/bitcoin/pull/21663/commits/b7381552cd4f965c45f1560d9cfc2fb09dbfcc1d Tree-SHA512: 4570c297421ae66043348acbe2f1e50b000e700406b38bc813c80714d4f109c6cadecff571ecbd8c2f0094ebdc959486fc55022ba6545bd592683a421e9c0655 --- .cirrus.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index 04af90a764..fc66b55a1d 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -157,8 +157,9 @@ task: task: name: 'macOS 11 native [gui] [no depends]' - macos_brew_addon_script: - - brew install boost libevent berkeley-db4 qt miniupnpc ccache zeromq qrencode sqlite libtool automake pkg-config gnu-getopt + brew_install_script: + - brew update + - brew install boost libevent berkeley-db4 qt@5 miniupnpc ccache zeromq qrencode sqlite libtool automake pkg-config gnu-getopt << : *GLOBAL_TASK_TEMPLATE macos_instance: # Use latest image, but hardcode version to avoid silent upgrades (and breaks) From e9450a8b368db16f033bb13bee4c0f5830133b80 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 14 Apr 2021 09:20:45 +0200 Subject: [PATCH 02/12] Merge #21669: test: Remove spurious double lock tsan suppressions by bumping to clang-12 fadea0bf371a38620b7f1f93f87d1da76d3314e0 Revert "test: Add tsan supp for leveldb::DBImpl::DeleteObsoleteFiles" (MarcoFalke) fadbd9988590ba94e3fd2d87d773f3b09d73ef46 test: Remove spurious double lock tsan suppressions by bumping to clang-12 (MarcoFalke) Pull request description: The double lock warnings appeared in #19041, but they didn't make any sense. Also, our sync module would detect double locks, if there were any. Bumping to clang-12 allows us to remove the spurious suppressions needed to run the tests, so do that. ACKs for top commit: practicalswift: cr ACK fadea0bf371a38620b7f1f93f87d1da76d3314e0 assuming CI passes and more specifically that newer Clang agrees that these TSan suppressions are no longer needed. Tree-SHA512: c411221a4b74d0af6ca8d686639b4f40b41c15906ccbb6647e8d569d6ab088264faafe075e1ac9523d5c0024b85f15a597bb3eedc7f07d4f5816245f75cfc08b --- test/sanitizer_suppressions/tsan | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/test/sanitizer_suppressions/tsan b/test/sanitizer_suppressions/tsan index b59ef7febc..2cf4851c6c 100644 --- a/test/sanitizer_suppressions/tsan +++ b/test/sanitizer_suppressions/tsan @@ -6,30 +6,12 @@ # Data races from zmq namespace race:zmq::* -# double locks (TODO fix) -mutex:g_genesis_wait_mutex -mutex:Interrupt -mutex:CThreadInterrupt -mutex:CConnman::Interrupt -mutex:CConnman::WakeMessageHandler -mutex:CConnman::ThreadOpenConnections -mutex:CConnman::ThreadOpenAddedConnections -mutex:CConnman::SocketHandler -mutex:UpdateTip -mutex:PeerLogicValidation::UpdatedBlockTip -mutex:g_best_block_mutex # race (TODO fix) -race:CConnman::WakeMessageHandler -race:CConnman::ThreadMessageHandler -race:fHaveGenesis -race:ProcessNewBlock -race:ThreadImport race:LoadWallet race:WalletBatch::WriteHDChain race:BerkeleyBatch race:BerkeleyDatabase race:DatabaseBatch -race:leveldb::DBImpl::DeleteObsoleteFiles race:zmq::* race:bitcoin-qt # deadlock (TODO fix) From 274068cdbcbd4e9cfffc2475bfa20edef8fa3111 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 23 Apr 2021 10:02:39 +0200 Subject: [PATCH 03/12] fix: follow-up bitcoin/bitcoin#21732 - minor missing typo --- src/init.cpp | 2 +- src/init/common.cpp | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 9389320e13..ce758e8522 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -718,7 +718,7 @@ void SetupServerArgs(NodeContext& node) argsman.AddArg("-disablegovernance", strprintf("Disable governance validation (0-1, default: %u)", 0), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-maxsigcachesize=", strprintf("Limit sum of signature cache and script execution cache sizes to MiB (default: %u)", DEFAULT_MAX_SIG_CACHE_SIZE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-maxtipage=", strprintf("Maximum tip age in seconds to consider node in initial block download (default: %u)", DEFAULT_MAX_TIP_AGE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); - argsman.AddArg("-mocktime=", "Replace actual time with " + UNIX_EPOCH_TIME + "(default: 0)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-mocktime=", "Replace actual time with " + UNIX_EPOCH_TIME + " (default: 0)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-minsporkkeys=", "Overrides minimum spork signers to change spork value. Only useful for regtest and devnet. Using this on mainnet or testnet will ban you.", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-printpriority", strprintf("Log transaction fee per kB when mining blocks (default: %u)", DEFAULT_PRINTPRIORITY), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-pushversion", "Protocol version to report to other nodes", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); diff --git a/src/init/common.cpp b/src/init/common.cpp index dd3e78bde1..069057dc86 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -12,7 +12,6 @@ #include #include #include -#include #include #include #include From f2b7ee73dbf7ec126a5aa526ff74a220480e79c6 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Sat, 13 Jul 2024 22:10:34 +0700 Subject: [PATCH 04/12] fix: follow-up bitcoin#15402 - removed dead code This code is not supposed to be ever added in dash, but it is removed in bitcoin#21009 which is DNM. Let's do it just here --- src/validation.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/validation.h b/src/validation.h index 7803865b8c..6dad511145 100644 --- a/src/validation.h +++ b/src/validation.h @@ -724,9 +724,6 @@ private: //! Mark a block as conflicting bool MarkConflictingBlock(BlockValidationState& state, CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - //! Mark a block as not having block data - void EraseBlockData(CBlockIndex* index) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - void CheckForkWarningConditions() EXCLUSIVE_LOCKS_REQUIRED(cs_main); void InvalidChainFound(CBlockIndex* pindexNew) EXCLUSIVE_LOCKS_REQUIRED(cs_main); void ConflictingChainFound(CBlockIndex* pindexNew) EXCLUSIVE_LOCKS_REQUIRED(cs_main); From 970048d9173bb017ee57b003557f81c051ff4f06 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Sat, 13 Jul 2024 22:35:25 +0700 Subject: [PATCH 05/12] fix: missing changes from bitcoin#19267 - run multiprocess on CI They run multiprocess build for ALL PRs in travis since that commit ```diff - if: type != pull_request OR commit_message =~ /depends:|multiprocess:/ # Skip on non-depends, non-multiprocess PRs ``` --- .gitlab-ci.yml | 51 +++++++++++++++++++++++--------------------------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index bd5c877dfa..b4a4e68427 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -203,16 +203,13 @@ x86_64-pc-linux-gnu-nowallet: HOST: x86_64-pc-linux-gnu DEP_OPTS: "NO_WALLET=1" -## TODO: # Skip on non-depends, non-multiprocess PRs -## if: type != pull_request OR commit_message =~ /depends:|multiprocess:/ -#x86_64-pc-linux-gnu-multiprocess: -# extends: -# - .build-depends-template -# - .skip-in-fast-mode-template -# variables: -# HOST: x86_64-pc-linux-gnu -# DEP_OPTS: "MULTIPROCESS=1" -# +x86_64-pc-linux-gnu-multiprocess: + extends: + - .build-depends-template + - .skip-in-fast-mode-template + variables: + HOST: x86_64-pc-linux-gnu + DEP_OPTS: "MULTIPROCESS=1" x86_64-apple-darwin: extends: @@ -309,14 +306,14 @@ linux64_nowallet-build: variables: BUILD_TARGET: linux64_nowallet -#linux64_multiprocess-build: -# extends: -# - .build-template -# - .skip-in-fast-mode-template -# needs: -# - x86_64-pc-linux-gnu-multiprocess -# variables: -# BUILD_TARGET: linux64_multiprocess +linux64_multiprocess-build: + extends: + - .build-template + - .skip-in-fast-mode-template + needs: + - x86_64-pc-linux-gnu-multiprocess + variables: + BUILD_TARGET: linux64_multiprocess #linux64_valgrind-build: # extends: @@ -381,16 +378,14 @@ linux64_ubsan-test: variables: BUILD_TARGET: linux64_ubsan -# TODO: enable multiprocess back in CI once it has any value -# or in case if any new backports to test -#linux64_multiprocess-test: -# extends: -# - .test-template -# - .skip-in-fast-mode-template -# needs: -# - linux64_multiprocess-build -# variables: -# BUILD_TARGET: linux64_multiprocess +linux64_multiprocess-test: + extends: + - .test-template + - .skip-in-fast-mode-template + needs: + - linux64_multiprocess-build + variables: + BUILD_TARGET: linux64_multiprocess #linux64_valgrind-test: # extends: From 3411577473296f7a145bf02f238be62e840707c6 Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Tue, 27 Apr 2021 10:17:19 +0200 Subject: [PATCH 06/12] Merge bitcoin/bitcoin#19160: multiprocess: Add basic spawn and IPC support 84934bf70e11fe4cda1cfda60113a54895d4fdd5 multiprocess: Add echoipc RPC method and test (Russell Yanofsky) 7d76cf667eff512043a28d4407cc89f58796c42b multiprocess: Add comments and documentation (Russell Yanofsky) ddf7ecc8dfc64cf121099fb047e1ac871de94f4c multiprocess: Add bitcoin-node process spawning support (Russell Yanofsky) 10afdf0280fa93bfffb0a7665c60dc155cd84514 multiprocess: Add Ipc interface implementation (Russell Yanofsky) 745c9cebd50fea1664efef571dc1ee1bddc96102 multiprocess: Add Ipc and Init interface definitions (Russell Yanofsky) 5d62d7f6cd48bbc4e9f37ecc369f38d5e1e0036c Update libmultiprocess library (Russell Yanofsky) Pull request description: This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). --- This PR adds basic process spawning and IPC method call support to `bitcoin-node` executables built with `--enable-multiprocess`[*]. These changes are used in https://github.com/bitcoin/bitcoin/pull/10102 to let node, gui, and wallet functionality run in different processes, and extended in https://github.com/bitcoin/bitcoin/pull/19460 and https://github.com/bitcoin/bitcoin/pull/19461 after that to allow gui and wallet processes to be started and stopped independently and connect to the node over a socket. These changes can also be used to implement new functionality outside the `bitcoin-node` process like external indexes or pluggable transports (https://github.com/bitcoin/bitcoin/pull/18988). The `Ipc::spawnProcess` and `Ipc::serveProcess` methods added here are entry points for spawning a child process and serving a parent process, and being able to make bidirectional, multithreaded method calls between the processes. A simple example of this is implemented in commit "Add echoipc RPC method and test." Changes in this PR aside from the echo test were originally part of #10102, but have been split and moved here for easier review, and so they can be used for other applications like external plugins. Additional notes about this PR can be found at https://bitcoincore.reviews/19160 [*] Note: the `--enable-multiprocess` feature is still experimental, and not enabled by default, and not yet supported on windows. More information can be found in [doc/multiprocess.md](https://github.com/bitcoin/bitcoin/blob/master/doc/multiprocess.md) ACKs for top commit: fjahr: re-ACK 84934bf70e11fe4cda1cfda60113a54895d4fdd5 ariard: ACK 84934bf. Changes since last ACK fixes the silent merge conflict about `EnsureAnyNodeContext()`. Rebuilt and checked again debug command `echoipc`. Tree-SHA512: 52a948b5e18a26d7d7a09b83003eaae9b1ed2981978c36c959fe9a55abf70ae6a627c4ff913a3428be17400a3dace30c58b5057fa75c319662c3be98f19810c6 --- depends/packages/native_libmultiprocess.mk | 4 +- doc/multiprocess.md | 39 +++++++++- src/Makefile.am | 47 ++++++++++- src/bitcoind.cpp | 15 +++- src/init/bitcoin-node.cpp | 45 +++++++++++ src/init/bitcoind.cpp | 29 +++++++ src/interfaces/README.md | 6 +- src/interfaces/echo.cpp | 18 +++++ src/interfaces/echo.h | 26 +++++++ src/interfaces/init.cpp | 17 ++++ src/interfaces/init.h | 52 +++++++++++++ src/interfaces/ipc.h | 71 +++++++++++++++++ src/ipc/capnp/.gitignore | 2 + src/ipc/capnp/echo.capnp | 17 ++++ src/ipc/capnp/init-types.h | 10 +++ src/ipc/capnp/init.capnp | 20 +++++ src/ipc/capnp/protocol.cpp | 90 ++++++++++++++++++++++ src/ipc/capnp/protocol.h | 17 ++++ src/ipc/exception.h | 20 +++++ src/ipc/interfaces.cpp | 77 ++++++++++++++++++ src/ipc/process.cpp | 61 +++++++++++++++ src/ipc/process.h | 42 ++++++++++ src/ipc/protocol.h | 39 ++++++++++ src/logging.cpp | 1 + src/logging.h | 1 + src/node/context.h | 3 + src/rpc/misc.cpp | 41 ++++++++++ test/functional/rpc_misc.py | 5 +- test/lint/lint-include-guards.sh | 2 +- 29 files changed, 803 insertions(+), 14 deletions(-) create mode 100644 src/init/bitcoin-node.cpp create mode 100644 src/init/bitcoind.cpp create mode 100644 src/interfaces/echo.cpp create mode 100644 src/interfaces/echo.h create mode 100644 src/interfaces/init.cpp create mode 100644 src/interfaces/init.h create mode 100644 src/interfaces/ipc.h create mode 100644 src/ipc/capnp/.gitignore create mode 100644 src/ipc/capnp/echo.capnp create mode 100644 src/ipc/capnp/init-types.h create mode 100644 src/ipc/capnp/init.capnp create mode 100644 src/ipc/capnp/protocol.cpp create mode 100644 src/ipc/capnp/protocol.h create mode 100644 src/ipc/exception.h create mode 100644 src/ipc/interfaces.cpp create mode 100644 src/ipc/process.cpp create mode 100644 src/ipc/process.h create mode 100644 src/ipc/protocol.h diff --git a/depends/packages/native_libmultiprocess.mk b/depends/packages/native_libmultiprocess.mk index 0790e7137e..6e600c5720 100644 --- a/depends/packages/native_libmultiprocess.mk +++ b/depends/packages/native_libmultiprocess.mk @@ -1,8 +1,8 @@ package=native_libmultiprocess -$(package)_version=5741d750a04e644a03336090d8979c6d033e32c0 +$(package)_version=d576d975debdc9090bd2582f83f49c76c0061698 $(package)_download_path=https://github.com/chaincodelabs/libmultiprocess/archive $(package)_file_name=$($(package)_version).tar.gz -$(package)_sha256_hash=ac848db49a6ed53e423c62d54bd87f1f08cbb0326254a8667e10bbfe5bf032a4 +$(package)_sha256_hash=9f8b055c8bba755dc32fe799b67c20b91e7b13e67cadafbc54c0f1def057a370 $(package)_dependencies=native_capnp define $(package)_config_cmds diff --git a/doc/multiprocess.md b/doc/multiprocess.md index e315343a4a..3106e57c2a 100644 --- a/doc/multiprocess.md +++ b/doc/multiprocess.md @@ -15,7 +15,7 @@ Specific next steps after backporting [bitcoin#10102](https://github.com/bitcoin ## Debugging -After backporting [bitcoin#10102](https://github.com/bitcoin/bitcoin/pull/10102), the `-debug=ipc` command line option can be used to see requests and responses between processes. +The `-debug=ipc` command line option can be used to see requests and responses between processes. ## Installation @@ -33,3 +33,40 @@ DASHD=dash-node test/functional/test_runner.py The configure script will pick up settings and library locations from the depends directory, so there is no need to pass `--enable-multiprocess` as a separate flag when using the depends system (it's controlled by the `MULTIPROCESS=1` option). Alternately, you can install [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) packages on your system, and just run `./configure --enable-multiprocess` without using the depends system. The configure script will be able to locate the installed packages via [pkg-config](https://www.freedesktop.org/wiki/Software/pkg-config/). See [Installation](https://github.com/chaincodelabs/libmultiprocess#installation) section of the libmultiprocess readme for install steps. See [build-unix.md](build-unix.md) and [build-osx.md](build-osx.md) for information about installing dependencies in general. + +## IPC implementation details + +Cross process Node, Wallet, and Chain interfaces are defined in +[`src/interfaces/`](../src/interfaces/). These are C++ classes which follow +[conventions](developer-notes.md#internal-interface-guidelines), like passing +serializable arguments so they can be called from different processes, and +making methods pure virtual so they can have proxy implementations that forward +calls between processes. + +When Wallet, Node, and Chain code is running in the same process, calling any +interface method invokes the implementation directly. When code is running in +different processes, calling an interface method invokes a proxy interface +implementation that communicates with a remote process and invokes the real +implementation in the remote process. The +[libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) code +generation tool internally generates proxy client classes and proxy server +classes for this purpose that are thin wrappers around Cap'n Proto +[client](https://capnproto.org/cxxrpc.html#clients) and +[server](https://capnproto.org/cxxrpc.html#servers) classes, which handle the +actual serialization and socket communication. + +As much as possible, calls between processes are meant to work the same as +calls within a single process without adding limitations or requiring extra +implementation effort. Processes communicate with each other by calling regular +[C++ interface methods](../src/interfaces/README.md). Method arguments and +return values are automatically serialized and sent between processes. Object +references and `std::function` arguments are automatically tracked and mapped +to allow invoked code to call back into invoking code at any time, and there is +a 1:1 threading model where any thread invoking a method in another process has +a corresponding thread in the invoked process responsible for executing all +method calls from the source thread, without blocking I/O or holding up another +call, and using the same thread local variables, locks, and callbacks between +calls. The forwarding, tracking, and threading is implemented inside the +[libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) library +which has the design goal of making calls between processes look like calls in +the same process to the extent possible. diff --git a/src/Makefile.am b/src/Makefile.am index 52e783ff45..406d402509 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -94,6 +94,7 @@ EXTRA_LIBRARIES += \ $(LIBBITCOIN_CONSENSUS) \ $(LIBBITCOIN_SERVER) \ $(LIBBITCOIN_CLI) \ + $(LIBBITCOIN_IPC) \ $(LIBBITCOIN_WALLET) \ $(LIBBITCOIN_WALLET_TOOL) \ $(LIBBITCOIN_ZMQ) @@ -219,7 +220,10 @@ BITCOIN_CORE_H = \ init/common.h \ interfaces/chain.h \ interfaces/coinjoin.h \ + interfaces/echo.h \ interfaces/handler.h \ + interfaces/init.h \ + interfaces/ipc.h \ interfaces/node.h \ interfaces/wallet.h \ key.h \ @@ -407,6 +411,8 @@ obj/build.h: FORCE "$(abs_top_srcdir)" libbitcoin_util_a-clientversion.$(OBJEXT): obj/build.h +ipc/capnp/libbitcoin_ipc_a-ipc.$(OBJEXT): $(libbitcoin_ipc_mpgen_input:=.h) + # server: shared between dashd and dash-qt # Contains code accessing mempool and chain state that is meant to be separated # from wallet and gui code (see node/README.md). Shared code should go in @@ -772,7 +778,9 @@ libbitcoin_util_a_SOURCES = \ clientversion.cpp \ compat/strnlen.cpp \ fs.cpp \ + interfaces/echo.cpp \ interfaces/handler.cpp \ + interfaces/init.cpp \ logging.cpp \ messagesigner.cpp \ random.cpp \ @@ -851,17 +859,17 @@ bitcoin_bin_ldadd = \ bitcoin_bin_ldadd += $(BACKTRACE_LIB) $(BOOST_LIBS) $(BDB_LIBS) $(MINIUPNPC_LIBS) $(NATPMP_LIBS) $(SQLITE_LIBS) $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS) $(ZMQ_LIBS) $(GMP_LIBS) -dashd_SOURCES = $(bitcoin_daemon_sources) +dashd_SOURCES = $(bitcoin_daemon_sources) init/bitcoind.cpp dashd_CPPFLAGS = $(bitcoin_bin_cppflags) dashd_CXXFLAGS = $(bitcoin_bin_cxxflags) dashd_LDFLAGS = $(bitcoin_bin_ldflags) dashd_LDADD = $(LIBBITCOIN_SERVER) $(bitcoin_bin_ldadd) -dash_node_SOURCES = $(bitcoin_daemon_sources) +dash_node_SOURCES = $(bitcoin_daemon_sources) init/bitcoin-node.cpp dash_node_CPPFLAGS = $(bitcoin_bin_cppflags) dash_node_CXXFLAGS = $(bitcoin_bin_cxxflags) dash_node_LDFLAGS = $(bitcoin_bin_ldflags) -dash_node_LDADD = $(LIBBITCOIN_SERVER) $(bitcoin_bin_ldadd) +dash_node_LDADD = $(LIBBITCOIN_SERVER) $(bitcoin_bin_ldadd) $(LIBBITCOIN_IPC) $(LIBMULTIPROCESS_LIBS) # dash-cli binary # dash_cli_SOURCES = bitcoin-cli.cpp @@ -1001,6 +1009,39 @@ endif osx_debug: $(bin_PROGRAMS) for i in $(bin_PROGRAMS); do mkdir -p $$i.dSYM/Contents/Resources/DWARF && $(DSYMUTIL_FLAT) -o $$i.dSYM/Contents/Resources/DWARF/$$(basename $$i) $$i &> /dev/null ; done +libbitcoin_ipc_mpgen_input = \ + ipc/capnp/echo.capnp \ + ipc/capnp/init.capnp +EXTRA_DIST += $(libbitcoin_ipc_mpgen_input) +%.capnp: + +if BUILD_MULTIPROCESS +LIBBITCOIN_IPC=libbitcoin_ipc.a +libbitcoin_ipc_a_SOURCES = \ + ipc/capnp/init-types.h \ + ipc/capnp/protocol.cpp \ + ipc/capnp/protocol.h \ + ipc/exception.h \ + ipc/interfaces.cpp \ + ipc/process.cpp \ + ipc/process.h \ + ipc/protocol.h +libbitcoin_ipc_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) +libbitcoin_ipc_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) $(LIBMULTIPROCESS_CFLAGS) + +include $(MPGEN_PREFIX)/include/mpgen.mk +libbitcoin_ipc_mpgen_output = \ + $(libbitcoin_ipc_mpgen_input:=.c++) \ + $(libbitcoin_ipc_mpgen_input:=.h) \ + $(libbitcoin_ipc_mpgen_input:=.proxy-client.c++) \ + $(libbitcoin_ipc_mpgen_input:=.proxy-server.c++) \ + $(libbitcoin_ipc_mpgen_input:=.proxy-types.c++) \ + $(libbitcoin_ipc_mpgen_input:=.proxy-types.h) \ + $(libbitcoin_ipc_mpgen_input:=.proxy.h) +nodist_libbitcoin_ipc_a_SOURCES = $(libbitcoin_ipc_mpgen_output) +CLEANFILES += $(libbitcoin_ipc_mpgen_output) +endif + include Makefile.crc32c.include include Makefile.leveldb.include include Makefile.test_util.include diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index ce8384026e..a790ef1cc1 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -109,10 +110,8 @@ int fork_daemon(bool nochdir, bool noclose, TokenPipeEnd& endpoint) #endif -static bool AppInit(int argc, char* argv[]) +static bool AppInit(NodeContext& node, int argc, char* argv[]) { - NodeContext node; - bool fRet = false; util::ThreadSetInternalName("init"); @@ -264,10 +263,18 @@ MAIN_FUNCTION util::WinCmdLineArgs winArgs; std::tie(argc, argv) = winArgs.get(); #endif + + NodeContext node; + int exit_status; + std::unique_ptr init = interfaces::MakeNodeInit(node, argc, argv, exit_status); + if (!init) { + return exit_status; + } + SetupEnvironment(); // Connect dashd signal handlers noui_connect(); - return (AppInit(argc, argv) ? EXIT_SUCCESS : EXIT_FAILURE); + return (AppInit(node, argc, argv) ? EXIT_SUCCESS : EXIT_FAILURE); } diff --git a/src/init/bitcoin-node.cpp b/src/init/bitcoin-node.cpp new file mode 100644 index 0000000000..437f112317 --- /dev/null +++ b/src/init/bitcoin-node.cpp @@ -0,0 +1,45 @@ +// Copyright (c) 2021 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 + +namespace init { +namespace { +const char* EXE_NAME = "dash-node"; + +class BitcoinNodeInit : public interfaces::Init +{ +public: + BitcoinNodeInit(NodeContext& node, const char* arg0) + : m_node(node), + m_ipc(interfaces::MakeIpc(EXE_NAME, arg0, *this)) + { + m_node.init = this; + } + std::unique_ptr makeEcho() override { return interfaces::MakeEcho(); } + interfaces::Ipc* ipc() override { return m_ipc.get(); } + NodeContext& m_node; + std::unique_ptr m_ipc; +}; +} // namespace +} // namespace init + +namespace interfaces { +std::unique_ptr MakeNodeInit(NodeContext& node, int argc, char* argv[], int& exit_status) +{ + auto init = std::make_unique(node, argc > 0 ? argv[0] : ""); + // Check if bitcoin-node is being invoked as an IPC server. If so, then + // bypass normal execution and just respond to requests over the IPC + // channel and return null. + if (init->m_ipc->startSpawnedProcess(argc, argv, exit_status)) { + return nullptr; + } + return init; +} +} // namespace interfaces diff --git a/src/init/bitcoind.cpp b/src/init/bitcoind.cpp new file mode 100644 index 0000000000..1e17ce4d3c --- /dev/null +++ b/src/init/bitcoind.cpp @@ -0,0 +1,29 @@ +// Copyright (c) 2021 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 + +namespace init { +namespace { +class BitcoindInit : public interfaces::Init +{ +public: + BitcoindInit(NodeContext& node) : m_node(node) + { + m_node.init = this; + } + NodeContext& m_node; +}; +} // namespace +} // namespace init + +namespace interfaces { +std::unique_ptr MakeNodeInit(NodeContext& node, int argc, char* argv[], int& exit_status) +{ + return std::make_unique(node); +} +} // namespace interfaces diff --git a/src/interfaces/README.md b/src/interfaces/README.md index f77d172153..c7b49337d0 100644 --- a/src/interfaces/README.md +++ b/src/interfaces/README.md @@ -12,6 +12,8 @@ The following interfaces are defined here: * [`Handler`](handler.h) — returned by `handleEvent` methods on interfaces above and used to manage lifetimes of event handlers. -* [`Init`](init.h) — used by multiprocess code to access interfaces above on startup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102). +* [`Init`](init.h) — used by multiprocess code to access interfaces above on startup. Added in [#6143](https://github.com/dashpay/dash/pull/6143). -The interfaces above define boundaries between major components of bitcoin code (node, wallet, and gui), making it possible for them to run in different processes, and be tested, developed, and understood independently. These interfaces are not currently designed to be stable or to be used externally. +* [`Ipc`](ipc.h) — used by multiprocess code to access `Init` interface across processes. Added in [#6143](https://github.com/dashpay/dash/pull/6143). + +The interfaces above define boundaries between major components of bitcoin code (node, wallet, and gui), making it possible for them to run in [different processes](../../doc/multiprocess.md), and be tested, developed, and understood independently. These interfaces are not currently designed to be stable or to be used externally. diff --git a/src/interfaces/echo.cpp b/src/interfaces/echo.cpp new file mode 100644 index 0000000000..9bbb42217b --- /dev/null +++ b/src/interfaces/echo.cpp @@ -0,0 +1,18 @@ +// Copyright (c) 2021 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 + +namespace interfaces { +namespace { +class EchoImpl : public Echo +{ +public: + std::string echo(const std::string& echo) override { return echo; } +}; +} // namespace +std::unique_ptr MakeEcho() { return std::make_unique(); } +} // namespace interfaces diff --git a/src/interfaces/echo.h b/src/interfaces/echo.h new file mode 100644 index 0000000000..5578d9d9e6 --- /dev/null +++ b/src/interfaces/echo.h @@ -0,0 +1,26 @@ +// Copyright (c) 2021 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_INTERFACES_ECHO_H +#define BITCOIN_INTERFACES_ECHO_H + +#include +#include + +namespace interfaces { +//! Simple string echoing interface for testing. +class Echo +{ +public: + virtual ~Echo() {} + + //! Echo provided string. + virtual std::string echo(const std::string& echo) = 0; +}; + +//! Return implementation of Echo interface. +std::unique_ptr MakeEcho(); +} // namespace interfaces + +#endif // BITCOIN_INTERFACES_ECHO_H diff --git a/src/interfaces/init.cpp b/src/interfaces/init.cpp new file mode 100644 index 0000000000..f0f8aa5fed --- /dev/null +++ b/src/interfaces/init.cpp @@ -0,0 +1,17 @@ +// Copyright (c) 2021 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 + +namespace interfaces { +std::unique_ptr Init::makeNode() { return {}; } +std::unique_ptr Init::makeChain() { return {}; } +std::unique_ptr Init::makeWalletLoader(Chain& chain) { return {}; } +std::unique_ptr Init::makeEcho() { return {}; } +Ipc* Init::ipc() { return nullptr; } +} // namespace interfaces diff --git a/src/interfaces/init.h b/src/interfaces/init.h new file mode 100644 index 0000000000..a4ecf4b5d1 --- /dev/null +++ b/src/interfaces/init.h @@ -0,0 +1,52 @@ +// Copyright (c) 2021 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_INTERFACES_INIT_H +#define BITCOIN_INTERFACES_INIT_H + +#include + +struct NodeContext; + +namespace interfaces { +class Chain; +class Echo; +class Ipc; +class Node; +class WalletLoader; + +//! Initial interface created when a process is first started, and used to give +//! and get access to other interfaces (Node, Chain, Wallet, etc). +//! +//! There is a different Init interface implementation for each process +//! (bitcoin-gui, bitcoin-node, bitcoin-wallet, bitcoind, bitcoin-qt) and each +//! implementation can implement the make methods for interfaces it supports. +//! The default make methods all return null. +class Init +{ +public: + virtual ~Init() = default; + virtual std::unique_ptr makeNode(); + virtual std::unique_ptr makeChain(); + virtual std::unique_ptr makeWalletLoader(Chain& chain); + virtual std::unique_ptr makeEcho(); + virtual Ipc* ipc(); +}; + +//! Return implementation of Init interface for the node process. If the argv +//! indicates that this is a child process spawned to handle requests from a +//! parent process, this blocks and handles requests, then returns null and a +//! status code to exit with. If this returns non-null, the caller can start up +//! normally and use the Init object to spawn and connect to other processes +//! while it is running. +std::unique_ptr MakeNodeInit(NodeContext& node, int argc, char* argv[], int& exit_status); + +//! Return implementation of Init interface for the wallet process. +std::unique_ptr MakeWalletInit(int argc, char* argv[], int& exit_status); + +//! Return implementation of Init interface for the gui process. +std::unique_ptr MakeGuiInit(int argc, char* argv[]); +} // namespace interfaces + +#endif // BITCOIN_INTERFACES_INIT_H diff --git a/src/interfaces/ipc.h b/src/interfaces/ipc.h new file mode 100644 index 0000000000..e9e6c78053 --- /dev/null +++ b/src/interfaces/ipc.h @@ -0,0 +1,71 @@ +// Copyright (c) 2021 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_INTERFACES_IPC_H +#define BITCOIN_INTERFACES_IPC_H + +#include +#include +#include + +namespace interfaces { +class Init; + +//! Interface providing access to interprocess-communication (IPC) +//! functionality. The IPC implementation is responsible for establishing +//! connections between a controlling process and a process being controlled. +//! When a connection is established, the process being controlled returns an +//! interfaces::Init pointer to the controlling process, which the controlling +//! process can use to get access to other interfaces and functionality. +//! +//! When spawning a new process, the steps are: +//! +//! 1. The controlling process calls interfaces::Ipc::spawnProcess(), which +//! calls ipc::Process::spawn(), which spawns a new process and returns a +//! socketpair file descriptor for communicating with it. +//! interfaces::Ipc::spawnProcess() then calls ipc::Protocol::connect() +//! passing the socketpair descriptor, which returns a local proxy +//! interfaces::Init implementation calling remote interfaces::Init methods. +//! 2. The spawned process calls interfaces::Ipc::startSpawnProcess(), which +//! calls ipc::Process::checkSpawned() to read command line arguments and +//! determine whether it is a spawned process and what socketpair file +//! descriptor it should use. It then calls ipc::Protocol::serve() to handle +//! incoming requests from the socketpair and invoke interfaces::Init +//! interface methods, and exit when the socket is closed. +//! 3. The controlling process calls local proxy interfaces::Init object methods +//! to make other proxy objects calling other remote interfaces. It can also +//! destroy the initial interfaces::Init object to close the connection and +//! shut down the spawned process. +class Ipc +{ +public: + virtual ~Ipc() = default; + + //! Spawn a child process returning pointer to its Init interface. + virtual std::unique_ptr spawnProcess(const char* exe_name) = 0; + + //! If this is a spawned process, block and handle requests from the parent + //! process by forwarding them to this process's Init interface, then return + //! true. If this is not a spawned child process, return false. + virtual bool startSpawnedProcess(int argc, char* argv[], int& exit_status) = 0; + + //! Add cleanup callback to remote interface that will run when the + //! interface is deleted. + template + void addCleanup(Interface& iface, std::function cleanup) + { + addCleanup(typeid(Interface), &iface, std::move(cleanup)); + } + +protected: + //! Internal implementation of public addCleanup method (above) as a + //! type-erased virtual function, since template functions can't be virtual. + virtual void addCleanup(std::type_index type, void* iface, std::function cleanup) = 0; +}; + +//! Return implementation of Ipc interface. +std::unique_ptr MakeIpc(const char* exe_name, const char* process_argv0, Init& init); +} // namespace interfaces + +#endif // BITCOIN_INTERFACES_IPC_H diff --git a/src/ipc/capnp/.gitignore b/src/ipc/capnp/.gitignore new file mode 100644 index 0000000000..036df1430c --- /dev/null +++ b/src/ipc/capnp/.gitignore @@ -0,0 +1,2 @@ +# capnp generated files +*.capnp.* diff --git a/src/ipc/capnp/echo.capnp b/src/ipc/capnp/echo.capnp new file mode 100644 index 0000000000..df36ee0de3 --- /dev/null +++ b/src/ipc/capnp/echo.capnp @@ -0,0 +1,17 @@ +# Copyright (c) 2021 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +@0x888b4f7f51e691f7; + +using Cxx = import "/capnp/c++.capnp"; +$Cxx.namespace("ipc::capnp::messages"); + +using Proxy = import "/mp/proxy.capnp"; +$Proxy.include("interfaces/echo.h"); +$Proxy.include("ipc/capnp/echo.capnp.h"); + +interface Echo $Proxy.wrap("interfaces::Echo") { + destroy @0 (context :Proxy.Context) -> (); + echo @1 (context :Proxy.Context, echo: Text) -> (result :Text); +} diff --git a/src/ipc/capnp/init-types.h b/src/ipc/capnp/init-types.h new file mode 100644 index 0000000000..42031441b5 --- /dev/null +++ b/src/ipc/capnp/init-types.h @@ -0,0 +1,10 @@ +// Copyright (c) 2021 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_INIT_TYPES_H +#define BITCOIN_IPC_CAPNP_INIT_TYPES_H + +#include + +#endif // BITCOIN_IPC_CAPNP_INIT_TYPES_H diff --git a/src/ipc/capnp/init.capnp b/src/ipc/capnp/init.capnp new file mode 100644 index 0000000000..e6d358c665 --- /dev/null +++ b/src/ipc/capnp/init.capnp @@ -0,0 +1,20 @@ +# Copyright (c) 2021 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +@0xf2c5cfa319406aa6; + +using Cxx = import "/capnp/c++.capnp"; +$Cxx.namespace("ipc::capnp::messages"); + +using Proxy = import "/mp/proxy.capnp"; +$Proxy.include("interfaces/echo.h"); +$Proxy.include("interfaces/init.h"); +$Proxy.includeTypes("ipc/capnp/init-types.h"); + +using Echo = import "echo.capnp"; + +interface Init $Proxy.wrap("interfaces::Init") { + construct @0 (threadMap: Proxy.ThreadMap) -> (threadMap :Proxy.ThreadMap); + makeEcho @1 (context :Proxy.Context) -> (result :Echo.Echo); +} diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp new file mode 100644 index 0000000000..74c66c899a --- /dev/null +++ b/src/ipc/capnp/protocol.cpp @@ -0,0 +1,90 @@ +// Copyright (c) 2021 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 +#include + +namespace ipc { +namespace capnp { +namespace { +void IpcLogFn(bool raise, std::string message) +{ + LogPrint(BCLog::IPC, "%s\n", message); + if (raise) throw Exception(message); +} + +class CapnpProtocol : public Protocol +{ +public: + ~CapnpProtocol() noexcept(true) + { + if (m_loop) { + std::unique_lock lock(m_loop->m_mutex); + m_loop->removeClient(lock); + } + if (m_loop_thread.joinable()) m_loop_thread.join(); + assert(!m_loop); + }; + std::unique_ptr connect(int fd, const char* exe_name) override + { + startLoop(exe_name); + return mp::ConnectStream(*m_loop, fd); + } + void serve(int fd, const char* exe_name, interfaces::Init& init) override + { + assert(!m_loop); + mp::g_thread_context.thread_name = mp::ThreadName(exe_name); + m_loop.emplace(exe_name, &IpcLogFn, nullptr); + mp::ServeStream(*m_loop, fd, init); + m_loop->loop(); + m_loop.reset(); + } + void addCleanup(std::type_index type, void* iface, std::function cleanup) override + { + mp::ProxyTypeRegister::types().at(type)(iface).cleanup.emplace_back(std::move(cleanup)); + } + void startLoop(const char* exe_name) + { + if (m_loop) return; + std::promise promise; + m_loop_thread = std::thread([&] { + util::ThreadRename("capnp-loop"); + m_loop.emplace(exe_name, &IpcLogFn, nullptr); + { + std::unique_lock lock(m_loop->m_mutex); + m_loop->addClient(lock); + } + promise.set_value(); + m_loop->loop(); + m_loop.reset(); + }); + promise.get_future().wait(); + } + std::thread m_loop_thread; + std::optional m_loop; +}; +} // namespace + +std::unique_ptr MakeCapnpProtocol() { return std::make_unique(); } +} // namespace capnp +} // namespace ipc diff --git a/src/ipc/capnp/protocol.h b/src/ipc/capnp/protocol.h new file mode 100644 index 0000000000..eb057949d2 --- /dev/null +++ b/src/ipc/capnp/protocol.h @@ -0,0 +1,17 @@ +// Copyright (c) 2021 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_PROTOCOL_H +#define BITCOIN_IPC_CAPNP_PROTOCOL_H + +#include + +namespace ipc { +class Protocol; +namespace capnp { +std::unique_ptr MakeCapnpProtocol(); +} // namespace capnp +} // namespace ipc + +#endif // BITCOIN_IPC_CAPNP_PROTOCOL_H diff --git a/src/ipc/exception.h b/src/ipc/exception.h new file mode 100644 index 0000000000..53dee8124a --- /dev/null +++ b/src/ipc/exception.h @@ -0,0 +1,20 @@ +// Copyright (c) 2021 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_EXCEPTION_H +#define BITCOIN_IPC_EXCEPTION_H + +#include + +namespace ipc { +//! Exception class thrown when a call to remote method fails due to an IPC +//! error, like a socket getting disconnected. +class Exception : public std::runtime_error +{ +public: + using std::runtime_error::runtime_error; +}; +} // namespace ipc + +#endif // BITCOIN_IPC_EXCEPTION_H diff --git a/src/ipc/interfaces.cpp b/src/ipc/interfaces.cpp new file mode 100644 index 0000000000..ad4b78ed81 --- /dev/null +++ b/src/ipc/interfaces.cpp @@ -0,0 +1,77 @@ +// Copyright (c) 2021 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 + +namespace ipc { +namespace { +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()) + { + } + std::unique_ptr spawnProcess(const char* new_exe_name) override + { + int pid; + int fd = m_process->spawn(new_exe_name, m_process_argv0, pid); + LogPrint(::BCLog::IPC, "Process %s pid %i launched\n", new_exe_name, pid); + auto init = m_protocol->connect(fd, m_exe_name); + Ipc::addCleanup(*init, [this, new_exe_name, pid] { + int status = m_process->waitSpawned(pid); + LogPrint(::BCLog::IPC, "Process %s pid %i exited with status %i\n", new_exe_name, pid, status); + }); + return init; + } + 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)) { + return false; + } + m_protocol->serve(fd, m_exe_name, m_init); + exit_status = EXIT_SUCCESS; + return true; + } + void addCleanup(std::type_index type, void* iface, std::function cleanup) override + { + m_protocol->addCleanup(type, iface, std::move(cleanup)); + } + const char* m_exe_name; + const char* m_process_argv0; + interfaces::Init& m_init; + std::unique_ptr m_protocol; + std::unique_ptr m_process; +}; +} // namespace +} // namespace ipc + +namespace interfaces { +std::unique_ptr MakeIpc(const char* exe_name, const char* process_argv0, Init& init) +{ + return std::make_unique(exe_name, process_argv0, init); +} +} // namespace interfaces diff --git a/src/ipc/process.cpp b/src/ipc/process.cpp new file mode 100644 index 0000000000..43ed1f1bae --- /dev/null +++ b/src/ipc/process.cpp @@ -0,0 +1,61 @@ +// Copyright (c) 2021 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 + +namespace ipc { +namespace { +class ProcessImpl : public Process +{ +public: + int spawn(const std::string& new_exe_name, const fs::path& argv0_path, int& pid) override + { + return mp::SpawnProcess(pid, [&](int fd) { + fs::path path = argv0_path; + path.remove_filename(); + path.append(new_exe_name); + return std::vector{path.string(), "-ipcfd", strprintf("%i", fd)}; + }); + } + int waitSpawned(int 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 + // not a process spawned by the spawn() call above, so return false and + // do not try to serve requests. + if (argc != 3 || strcmp(argv[1], "-ipcfd") != 0) { + return false; + } + // If a single -ipcfd argument was provided, return true and get the + // file descriptor so Protocol::serve() can be called to handle + // requests from the parent process. The -ipcfd argument is not valid + // 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. + if (!ParseInt32(argv[2], &fd)) { + throw std::runtime_error(strprintf("Invalid -ipcfd number '%s'", argv[2])); + } + return true; + } +}; +} // namespace + +std::unique_ptr MakeProcess() { return std::make_unique(); } +} // namespace ipc diff --git a/src/ipc/process.h b/src/ipc/process.h new file mode 100644 index 0000000000..4bb2930d9c --- /dev/null +++ b/src/ipc/process.h @@ -0,0 +1,42 @@ +// Copyright (c) 2021 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_PROCESS_H +#define BITCOIN_IPC_PROCESS_H + +#include +#include + +namespace ipc { +class Protocol; + +//! IPC process interface for spawning bitcoin processes and serving requests +//! in processes that have been spawned. +//! +//! There will be different implementations of this interface depending on the +//! platform (e.g. unix, windows). +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, int& pid) = 0; + + //! Wait for spawned process to exit and return its exit code. + virtual int waitSpawned(int 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 + //! with the parent process. + virtual bool checkSpawned(int argc, char* argv[], int& fd) = 0; +}; + +//! Constructor for Process interface. Implementation will vary depending on +//! the platform (unix or windows). +std::unique_ptr MakeProcess(); +} // namespace ipc + +#endif // BITCOIN_IPC_PROCESS_H diff --git a/src/ipc/protocol.h b/src/ipc/protocol.h new file mode 100644 index 0000000000..af955b0007 --- /dev/null +++ b/src/ipc/protocol.h @@ -0,0 +1,39 @@ +// Copyright (c) 2021 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_PROTOCOL_H +#define BITCOIN_IPC_PROTOCOL_H + +#include + +#include +#include +#include + +namespace ipc { +//! IPC protocol interface for calling IPC methods over sockets. +//! +//! There may be different implementations of this interface for different IPC +//! protocols (e.g. Cap'n Proto, gRPC, JSON-RPC, or custom protocols). +class Protocol +{ +public: + virtual ~Protocol() = default; + + //! Return Init interface that forwards requests over given socket descriptor. + //! Socket communication is handled on a background thread. + virtual std::unique_ptr connect(int fd, const char* exe_name) = 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. + virtual void serve(int fd, const char* exe_name, interfaces::Init& init) = 0; + + //! Add cleanup callback to interface that will run when the interface is + //! deleted. + virtual void addCleanup(std::type_index type, void* iface, std::function cleanup) = 0; +}; +} // namespace ipc + +#endif // BITCOIN_IPC_PROTOCOL_H diff --git a/src/logging.cpp b/src/logging.cpp index 32a3add84a..bbea5ea2b3 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -158,6 +158,7 @@ const CLogCategoryDesc LogCategories[] = {BCLog::LEVELDB, "leveldb"}, {BCLog::VALIDATION, "validation"}, {BCLog::I2P, "i2p"}, + {BCLog::IPC, "ipc"}, {BCLog::LOCK, "lock"}, {BCLog::ALL, "1"}, {BCLog::ALL, "all"}, diff --git a/src/logging.h b/src/logging.h index 6cf696f3d0..fbb3e90be5 100644 --- a/src/logging.h +++ b/src/logging.h @@ -59,6 +59,7 @@ namespace BCLog { LEVELDB = (1 << 20), VALIDATION = (1 << 21), I2P = (1 << 22), + IPC = (1 << 23), LOCK = (1 << 24), //Start Dash diff --git a/src/node/context.h b/src/node/context.h index 129c6d73ec..0c1b60f192 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -36,6 +36,7 @@ struct LLMQContext; namespace interfaces { class Chain; class ChainClient; +class Init; class WalletLoader; namespace CoinJoin { class Loader; @@ -53,6 +54,8 @@ class Loader; //! any member functions. It should just be a collection of references that can //! be used without pulling in unwanted dependencies or functionality. struct NodeContext { + //! Init interface for initializing current process and connecting to other processes. + interfaces::Init* init{nullptr}; std::unique_ptr addrman; std::unique_ptr connman; std::unique_ptr mempool; diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp index 55e3f66823..37ccc16186 100644 --- a/src/rpc/misc.cpp +++ b/src/rpc/misc.cpp @@ -15,6 +15,9 @@ #include #include #include +#include +#include +#include #include #include #include @@ -1465,6 +1468,43 @@ static RPCHelpMan getindexinfo() static RPCHelpMan echo() { return echo("echo"); } static RPCHelpMan echojson() { return echo("echojson"); } +static RPCHelpMan echoipc() +{ + return RPCHelpMan{ + "echoipc", + "\nEcho back the input argument, passing it through a spawned process in a multiprocess build.\n" + "This command is for testing.\n", + {{"arg", RPCArg::Type::STR, RPCArg::Optional::NO, "The string to echo",}}, + RPCResult{RPCResult::Type::STR, "echo", "The echoed string."}, + RPCExamples{HelpExampleCli("echo", "\"Hello world\"") + + HelpExampleRpc("echo", "\"Hello world\"")}, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { + std::unique_ptr echo; + if (interfaces::Ipc* ipc = Assert(EnsureAnyNodeContext(request.context).init)->ipc()) { + // Spawn a new bitcoin-node process and call makeEcho to get a + // client pointer to a interfaces::Echo instance running in + // that process. This is just for testing. A slightly more + // realistic test spawning a different executable instead of + // the same executable would add a new bitcoin-echo executable, + // and spawn bitcoin-echo below instead of bitcoin-node. But + // using bitcoin-node avoids the need to build and install a + // new executable just for this one test. + auto init = ipc->spawnProcess("dash-node"); + echo = init->makeEcho(); + ipc->addCleanup(*echo, [init = init.release()] { delete init; }); + } else { + // IPC support is not available because this is a bitcoind + // process not a bitcoind-node process, so just create a local + // interfaces::Echo object and return it so the `echoipc` RPC + // method will work, and the python test calling `echoipc` + // can expect the same result. + echo = interfaces::MakeEcho(); + } + return echo->echo(request.params[0].get_str()); + }, + }; +} + void RegisterMiscRPCCommands(CRPCTable &t) { static const CRPCCommand commands[] = @@ -1499,6 +1539,7 @@ static const CRPCCommand commands[] = { "hidden", &mockscheduler, }, { "hidden", &echo, }, { "hidden", &echojson, }, + { "hidden", &echoipc, }, { "hidden", &mnauth, }, }; // clang-format on diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py index 8e94691bfc..c5aeabd8ca 100755 --- a/test/functional/rpc_misc.py +++ b/test/functional/rpc_misc.py @@ -57,7 +57,7 @@ class RpcMiscTest(BitcoinTestFramework): self.log.info("test logging rpc and help") # Test logging RPC returns the expected number of logging categories. - assert_equal(len(node.logging()), 37) + assert_equal(len(node.logging()), 38) # Test toggling a logging category on/off/on with the logging RPC. assert_equal(node.logging()['qt'], True) @@ -75,6 +75,9 @@ class RpcMiscTest(BitcoinTestFramework): logging_help = self.nodes[0].help('logging') assert f"valid logging categories are: {categories}" in logging_help + self.log.info("test echoipc (testing spawned process in multiprocess build)") + assert_equal(node.echoipc("hello"), "hello") + self.log.info("test getindexinfo") self.restart_node(0, ["-txindex=0"]) # Without any indices running the RPC returns an empty object diff --git a/test/lint/lint-include-guards.sh b/test/lint/lint-include-guards.sh index 28a77e1fd3..200e479498 100755 --- a/test/lint/lint-include-guards.sh +++ b/test/lint/lint-include-guards.sh @@ -15,7 +15,7 @@ REGEXP_EXCLUDE_FILES_WITH_PREFIX="src/(crypto/ctaes/|dashbls/|immer/|leveldb/|cr EXIT_CODE=0 for HEADER_FILE in $(git ls-files -- "*.h" | grep -vE "^${REGEXP_EXCLUDE_FILES_WITH_PREFIX}") do - HEADER_ID_BASE=$(cut -f2- -d/ <<< "${HEADER_FILE}" | sed "s/\.h$//g" | tr / _ | tr "[:lower:]" "[:upper:]" | tr - _) + HEADER_ID_BASE=$(cut -f2- -d/ <<< "${HEADER_FILE}" | sed "s/\.h$//g" | tr / _ | tr - _ | tr "[:lower:]" "[:upper:]") HEADER_ID="${HEADER_ID_PREFIX}${HEADER_ID_BASE}${HEADER_ID_SUFFIX}" if [[ $(grep -cE "^#(ifndef|define) ${HEADER_ID}" "${HEADER_FILE}") != 2 ]]; then echo "${HEADER_FILE} seems to be missing the expected include guard:" From b34514191f9fd8233552c45512070129c3ca9caa Mon Sep 17 00:00:00 2001 From: fanquake Date: Thu, 29 Apr 2021 18:11:35 +0800 Subject: [PATCH 07/12] Merge bitcoin/bitcoin#21738: test: Use clang-12 for ASAN, Add missing suppression fa00bb2c5ca64c7eb9e1846ffedc7829859812ca test: Add missing shift-base:nanobench.h suppression (MarcoFalke) 00004565ccdbaf6bf337e10a5f5ae463bd0ccf9a ci: Use clang-12 for asan task (MarcoFalke) Pull request description: ACKs for top commit: fanquake: ACK fa00bb2c5ca64c7eb9e1846ffedc7829859812ca Tree-SHA512: fe7cd1ad9f3e73c09f7f84dfb0f276d0cda603c4d591b9338a0914bf1276b0247fd2faee7052f5962c3ae3280e7fa8b72f5b773b84c2a8882a89ed1f8c08256c --- .cirrus.yml | 4 ++-- test/sanitizer_suppressions/ubsan | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index fc66b55a1d..bb3d4acd70 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -116,10 +116,10 @@ task: FILE_ENV: "./ci/test/00_setup_env_native_msan.sh" task: - name: '[no depends, sanitizers: address/leak (ASan + LSan) + undefined (UBSan) + integer] [focal]' + name: '[no depends, sanitizers: address/leak (ASan + LSan) + undefined (UBSan) + integer] [hirsute]' << : *GLOBAL_TASK_TEMPLATE container: - image: ubuntu:focal + image: ubuntu:hirsute env: FILE_ENV: "./ci/test/00_setup_env_native_asan.sh" diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index e6ae66d275..af17ca71d3 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -96,6 +96,7 @@ implicit-unsigned-integer-truncation:test/fuzz/crypto_diff_fuzz_chacha20.cpp # std::variant warning fixed in https://github.com/gcc-mirror/gcc/commit/074436cf8cdd2a9ce75cadd36deb8301f00e55b9 implicit-unsigned-integer-truncation:std::__detail::__variant::_Variant_storage shift-base:xoroshiro128plusplus.h +shift-base:nanobench.h shift-base:*/include/c++/ shift-base:arith_uint256.cpp shift-base:crypto/ From 23b83109eaacb64872be21b460b1bc8758cb4fd2 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 3 May 2021 08:13:49 +0200 Subject: [PATCH 08/12] Merge bitcoin/bitcoin#21750: net: remove unnecessary check of CNode::cs_vSend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 9096b13a4764873511b65f32a005ce4738b0d81c net: remove unnecessary check of CNode::cs_vSend (Vasil Dimov) Pull request description: It is not possible to have a node in `CConnman::vNodesDisconnected` and its reference count to be incremented - all `CNode::AddRef()` are done either before the node is added to `CConnman::vNodes` or while holding `CConnman::cs_vNodes` and the object being in `CConnman::vNodes`. So, the object being in `CConnman::vNodesDisconnected` and its reference count being zero means that it is not and will not start to be used by other threads. So, the lock of `CNode::cs_vSend` in `CConnman::DisconnectNodes()` will always succeed and is not necessary. Indeed all locks of `CNode::cs_vSend` are done either when the reference count is >0 or under the protection of `CConnman::cs_vNodes` and the node being in `CConnman::vNodes`. ACKs for top commit: MarcoFalke: review ACK 9096b13a4764873511b65f32a005ce4738b0d81c 🏧 jnewbery: utACK 9096b13a4764873511b65f32a005ce4738b0d81c Tree-SHA512: 910899cdcdc8934642eb0c40fcece8c3b01b7e20a0b023966b9d6972db6a885cb3a9a04e9562bae14d5833967e45e2ecb3687b94d495060c3da4b1f2afb0ac8f --- src/net.cpp | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 6ae5c88943..86605bb5ef 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1566,21 +1566,11 @@ void CConnman::DisconnectNodes() for (auto it = m_nodes_disconnected.begin(); it != m_nodes_disconnected.end(); ) { CNode* pnode = *it; - // wait until threads are done using it - bool fDelete = false; + // Destroy the object only after other threads have stopped using it. if (pnode->GetRefCount() <= 0) { - { - TRY_LOCK(pnode->cs_vSend, lockSend); - if (lockSend) { - fDelete = true; - } - } - if (fDelete) { - it = m_nodes_disconnected.erase(it); - DeleteNode(pnode); - } - } - if (!fDelete) { + it = m_nodes_disconnected.erase(it); + DeleteNode(pnode); + } else { ++it; } } From 334496ea7eea0735e5d354bc2756d5cc4b63cd19 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 3 May 2021 11:13:38 +0200 Subject: [PATCH 09/12] Merge bitcoin/bitcoin#21775: p2p: Limit m_block_inv_mutex fac96d026511f22f0202ce3631a38be0e990555f p2p: Limit m_block_inv_mutex (MarcoFalke) Pull request description: Keeping the lock longer than needed is confusing to reviewers and thread analysis. For example, keeping the lock while appending tx-invs, which requires the mempool lock, will tell thread analysis tools an incorrect lock order of `(1) m_block_inv_mutex, (2) pool.cs`. ACKs for top commit: Crypt-iQ: crACK fac96d026511f22f0202ce3631a38be0e990555f jnewbery: utACK fac96d026511f22f0202ce3631a38be0e990555f theStack: Code-Review ACK fac96d026511f22f0202ce3631a38be0e990555f Tree-SHA512: fcfac0f1f8b16df7522513abf716b2eed3d2fc9153f231c8cb61f451e342f29c984a5c872deca6bab3e601e5d651874cc229146c9370e46811b4520747a21f2b --- src/net_processing.cpp | 257 ++++++++++++++++++++--------------------- 1 file changed, 128 insertions(+), 129 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index fa3f4d0004..7680696d37 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5642,155 +5642,154 @@ bool PeerManagerImpl::SendMessages(CNode* pto) } } peer->m_blocks_for_inv_relay.clear(); + } - auto queueAndMaybePushInv = [this, pto, peer, &vInv, &msgMaker](const CInv& invIn) { - AssertLockHeld(peer->m_tx_relay->m_tx_inventory_mutex); - peer->m_tx_relay->m_tx_inventory_known_filter.insert(invIn.hash); - LogPrint(BCLog::NET, "SendMessages -- queued inv: %s index=%d peer=%d\n", invIn.ToString(), vInv.size(), pto->GetId()); - // Responses to MEMPOOL requests bypass the m_recently_announced_invs filter. - vInv.push_back(invIn); - if (vInv.size() == MAX_INV_SZ) { - LogPrint(BCLog::NET, "SendMessages -- pushing invs: count=%d peer=%d\n", vInv.size(), pto->GetId()); - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); - vInv.clear(); + auto queueAndMaybePushInv = [this, pto, peer, &vInv, &msgMaker](const CInv& invIn) { + AssertLockHeld(peer->m_tx_relay->m_tx_inventory_mutex); + peer->m_tx_relay->m_tx_inventory_known_filter.insert(invIn.hash); + LogPrint(BCLog::NET, "SendMessages -- queued inv: %s index=%d peer=%d\n", invIn.ToString(), vInv.size(), pto->GetId()); + // Responses to MEMPOOL requests bypass the m_recently_announced_invs filter. + vInv.push_back(invIn); + if (vInv.size() == MAX_INV_SZ) { + LogPrint(BCLog::NET, "SendMessages -- pushing invs: count=%d peer=%d\n", vInv.size(), pto->GetId()); + m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + vInv.clear(); + } + }; + + if (!pto->IsBlockOnlyConn()) { + LOCK(peer->m_tx_relay->m_tx_inventory_mutex); + // Check whether periodic sends should happen + // Note: If this node is running in a Masternode mode, it makes no sense to delay outgoing txes + // because we never produce any txes ourselves i.e. no privacy is lost in this case. + bool fSendTrickle = pto->HasPermission(NetPermissionFlags::NoBan) || is_masternode; + if (peer->m_tx_relay->m_next_inv_send_time < current_time) { + fSendTrickle = true; + if (pto->IsInboundConn()) { + peer->m_tx_relay->m_next_inv_send_time = m_connman.PoissonNextSendInbound(current_time, INBOUND_INVENTORY_BROADCAST_INTERVAL); + } else { + // Use half the delay for Masternode outbound peers, as there is less privacy concern for them. + peer->m_tx_relay->m_next_inv_send_time = pto->GetVerifiedProRegTxHash().IsNull() ? + PoissonNextSend(current_time, OUTBOUND_INVENTORY_BROADCAST_INTERVAL) : + PoissonNextSend(current_time, OUTBOUND_INVENTORY_BROADCAST_INTERVAL / 2); } - }; + } - if (!pto->IsBlockOnlyConn()) { - LOCK(peer->m_tx_relay->m_tx_inventory_mutex); - // Check whether periodic sends should happen - // Note: If this node is running in a Masternode mode, it makes no sense to delay outgoing txes - // because we never produce any txes ourselves i.e. no privacy is lost in this case. - bool fSendTrickle = pto->HasPermission(NetPermissionFlags::NoBan) || is_masternode; - if (peer->m_tx_relay->m_next_inv_send_time < current_time) { - fSendTrickle = true; - if (pto->IsInboundConn()) { - peer->m_tx_relay->m_next_inv_send_time = m_connman.PoissonNextSendInbound(current_time, INBOUND_INVENTORY_BROADCAST_INTERVAL); - } else { - // Use half the delay for Masternode outbound peers, as there is less privacy concern for them. - peer->m_tx_relay->m_next_inv_send_time = pto->GetVerifiedProRegTxHash().IsNull() ? - PoissonNextSend(current_time, OUTBOUND_INVENTORY_BROADCAST_INTERVAL) : - PoissonNextSend(current_time, OUTBOUND_INVENTORY_BROADCAST_INTERVAL / 2); - } + // Time to send but the peer has requested we not relay transactions. + if (fSendTrickle) { + LOCK(peer->m_tx_relay->m_bloom_filter_mutex); + if (!peer->m_tx_relay->m_relay_txs) peer->m_tx_relay->m_tx_inventory_to_send.clear(); + } + + // Respond to BIP35 mempool requests + if (fSendTrickle && peer->m_tx_relay->m_send_mempool) { + auto vtxinfo = m_mempool.infoAll(); + peer->m_tx_relay->m_send_mempool = false; + + LOCK(peer->m_tx_relay->m_bloom_filter_mutex); + + // Send invs for txes and corresponding IS-locks + for (const auto& txinfo : vtxinfo) { + const uint256& hash = txinfo.tx->GetHash(); + peer->m_tx_relay->m_tx_inventory_to_send.erase(hash); + if (peer->m_tx_relay->m_bloom_filter && !peer->m_tx_relay->m_bloom_filter->IsRelevantAndUpdate(*txinfo.tx)) continue; + + int nInvType = m_cj_ctx->dstxman->GetDSTX(hash) ? MSG_DSTX : MSG_TX; + queueAndMaybePushInv(CInv(nInvType, hash)); + + const auto islock = m_llmq_ctx->isman->GetInstantSendLockByTxid(hash); + if (islock == nullptr) continue; + if (pto->nVersion < ISDLOCK_PROTO_VERSION) continue; + queueAndMaybePushInv(CInv(MSG_ISDLOCK, ::SerializeHash(*islock))); } - // Time to send but the peer has requested we not relay transactions. - if (fSendTrickle) { - LOCK(peer->m_tx_relay->m_bloom_filter_mutex); - if (!peer->m_tx_relay->m_relay_txs) peer->m_tx_relay->m_tx_inventory_to_send.clear(); + // Send an inv for the best ChainLock we have + const auto& clsig = m_llmq_ctx->clhandler->GetBestChainLock(); + if (!clsig.IsNull()) { + uint256 chainlockHash = ::SerializeHash(clsig); + queueAndMaybePushInv(CInv(MSG_CLSIG, chainlockHash)); } + peer->m_tx_relay->m_last_mempool_req = std::chrono::duration_cast(current_time); + } - // Respond to BIP35 mempool requests - if (fSendTrickle && peer->m_tx_relay->m_send_mempool) { - auto vtxinfo = m_mempool.infoAll(); - peer->m_tx_relay->m_send_mempool = false; + // Determine transactions to relay + if (fSendTrickle) { + LOCK(peer->m_tx_relay->m_bloom_filter_mutex); - LOCK(peer->m_tx_relay->m_bloom_filter_mutex); - - // Send invs for txes and corresponding IS-locks - for (const auto& txinfo : vtxinfo) { - const uint256& hash = txinfo.tx->GetHash(); - peer->m_tx_relay->m_tx_inventory_to_send.erase(hash); - if (peer->m_tx_relay->m_bloom_filter && !peer->m_tx_relay->m_bloom_filter->IsRelevantAndUpdate(*txinfo.tx)) continue; - - int nInvType = m_cj_ctx->dstxman->GetDSTX(hash) ? MSG_DSTX : MSG_TX; - queueAndMaybePushInv(CInv(nInvType, hash)); - - const auto islock = m_llmq_ctx->isman->GetInstantSendLockByTxid(hash); - if (islock == nullptr) continue; - if (pto->nVersion < ISDLOCK_PROTO_VERSION) continue; - queueAndMaybePushInv(CInv(MSG_ISDLOCK, ::SerializeHash(*islock))); - } - - // Send an inv for the best ChainLock we have - const auto& clsig = m_llmq_ctx->clhandler->GetBestChainLock(); - if (!clsig.IsNull()) { - uint256 chainlockHash = ::SerializeHash(clsig); - queueAndMaybePushInv(CInv(MSG_CLSIG, chainlockHash)); - } - peer->m_tx_relay->m_last_mempool_req = std::chrono::duration_cast(current_time); + // Produce a vector with all candidates for sending + std::vector::iterator> vInvTx; + vInvTx.reserve(peer->m_tx_relay->m_tx_inventory_to_send.size()); + for (std::set::iterator it = peer->m_tx_relay->m_tx_inventory_to_send.begin(); it != peer->m_tx_relay->m_tx_inventory_to_send.end(); it++) { + vInvTx.push_back(it); } + // Topologically and fee-rate sort the inventory we send for privacy and priority reasons. + // A heap is used so that not all items need sorting if only a few are being sent. + CompareInvMempoolOrder compareInvMempoolOrder(&m_mempool); + std::make_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder); + // No reason to drain out at many times the network's capacity, + // especially since we have many peers and some will draw much shorter delays. + unsigned int nRelayedTransactions = 0; + size_t broadcast_max{INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK * MaxBlockSize() / 1000000 + (peer->m_tx_relay->m_tx_inventory_to_send.size()/1000)*5}; + broadcast_max = std::min(1000, broadcast_max); - // Determine transactions to relay - if (fSendTrickle) { - LOCK(peer->m_tx_relay->m_bloom_filter_mutex); - - // Produce a vector with all candidates for sending - std::vector::iterator> vInvTx; - vInvTx.reserve(peer->m_tx_relay->m_tx_inventory_to_send.size()); - for (std::set::iterator it = peer->m_tx_relay->m_tx_inventory_to_send.begin(); it != peer->m_tx_relay->m_tx_inventory_to_send.end(); it++) { - vInvTx.push_back(it); + while (!vInvTx.empty() && nRelayedTransactions < broadcast_max) { + // Fetch the top element from the heap + std::pop_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder); + std::set::iterator it = vInvTx.back(); + vInvTx.pop_back(); + uint256 hash = *it; + // Remove it from the to-be-sent set + peer->m_tx_relay->m_tx_inventory_to_send.erase(it); + // Check if not in the filter already + if (peer->m_tx_relay->m_tx_inventory_known_filter.contains(hash)) { + continue; } - // Topologically and fee-rate sort the inventory we send for privacy and priority reasons. - // A heap is used so that not all items need sorting if only a few are being sent. - CompareInvMempoolOrder compareInvMempoolOrder(&m_mempool); - std::make_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder); - // No reason to drain out at many times the network's capacity, - // especially since we have many peers and some will draw much shorter delays. - unsigned int nRelayedTransactions = 0; - size_t broadcast_max{INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK * MaxBlockSize() / 1000000 + (peer->m_tx_relay->m_tx_inventory_to_send.size()/1000)*5}; - broadcast_max = std::min(1000, broadcast_max); - - while (!vInvTx.empty() && nRelayedTransactions < broadcast_max) { - // Fetch the top element from the heap - std::pop_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder); - std::set::iterator it = vInvTx.back(); - vInvTx.pop_back(); - uint256 hash = *it; - // Remove it from the to-be-sent set - peer->m_tx_relay->m_tx_inventory_to_send.erase(it); - // Check if not in the filter already - if (peer->m_tx_relay->m_tx_inventory_known_filter.contains(hash)) { - continue; - } - // Not in the mempool anymore? don't bother sending it. - auto txinfo = m_mempool.info(hash); - if (!txinfo.tx) { - continue; - } - if (peer->m_tx_relay->m_bloom_filter && !peer->m_tx_relay->m_bloom_filter->IsRelevantAndUpdate(*txinfo.tx)) continue; - // Send - State(pto->GetId())->m_recently_announced_invs.insert(hash); - nRelayedTransactions++; + // Not in the mempool anymore? don't bother sending it. + auto txinfo = m_mempool.info(hash); + if (!txinfo.tx) { + continue; + } + if (peer->m_tx_relay->m_bloom_filter && !peer->m_tx_relay->m_bloom_filter->IsRelevantAndUpdate(*txinfo.tx)) continue; + // Send + State(pto->GetId())->m_recently_announced_invs.insert(hash); + nRelayedTransactions++; + { + // Expire old relay messages + while (!g_relay_expiration.empty() && g_relay_expiration.front().first < current_time) { - // Expire old relay messages - while (!g_relay_expiration.empty() && g_relay_expiration.front().first < current_time) - { - mapRelay.erase(g_relay_expiration.front().second); - g_relay_expiration.pop_front(); - } - - auto ret = mapRelay.emplace(hash, std::move(txinfo.tx)); - if (ret.second) { - g_relay_expiration.emplace_back(current_time + RELAY_TX_CACHE_TIME, ret.first); - } + mapRelay.erase(g_relay_expiration.front().second); + g_relay_expiration.pop_front(); + } + + auto ret = mapRelay.emplace(hash, std::move(txinfo.tx)); + if (ret.second) { + g_relay_expiration.emplace_back(current_time + RELAY_TX_CACHE_TIME, ret.first); } - int nInvType = m_cj_ctx->dstxman->GetDSTX(hash) ? MSG_DSTX : MSG_TX; - queueAndMaybePushInv(CInv(nInvType, hash)); } + int nInvType = m_cj_ctx->dstxman->GetDSTX(hash) ? MSG_DSTX : MSG_TX; + queueAndMaybePushInv(CInv(nInvType, hash)); } } + } + { + // Send non-tx/non-block inventory items + LOCK2(peer->m_tx_relay->m_tx_inventory_mutex, peer->m_tx_relay->m_bloom_filter_mutex); - { - // Send non-tx/non-block inventory items - LOCK2(peer->m_tx_relay->m_tx_inventory_mutex, peer->m_tx_relay->m_bloom_filter_mutex); + bool fSendIS = peer->m_tx_relay->m_relay_txs && !pto->IsBlockRelayOnly(); - bool fSendIS = peer->m_tx_relay->m_relay_txs && !pto->IsBlockRelayOnly(); - - for (const auto& inv : peer->m_tx_relay->vInventoryOtherToSend) { - if (!peer->m_tx_relay->m_relay_txs && NetMessageViolatesBlocksOnly(inv.GetCommand())) { - continue; - } - if (peer->m_tx_relay->m_tx_inventory_known_filter.contains(inv.hash)) { - continue; - } - if (!fSendIS && inv.type == MSG_ISDLOCK) { - continue; - } - queueAndMaybePushInv(inv); + for (const auto& inv : peer->m_tx_relay->vInventoryOtherToSend) { + if (!peer->m_tx_relay->m_relay_txs && NetMessageViolatesBlocksOnly(inv.GetCommand())) { + continue; } - peer->m_tx_relay->vInventoryOtherToSend.clear(); + if (peer->m_tx_relay->m_tx_inventory_known_filter.contains(inv.hash)) { + continue; + } + if (!fSendIS && inv.type == MSG_ISDLOCK) { + continue; + } + queueAndMaybePushInv(inv); } + peer->m_tx_relay->vInventoryOtherToSend.clear(); } if (!vInv.empty()) m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); From 86b76d19b6a495578781faba13fedbfaa7ec3f2e Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 4 May 2021 19:18:09 +0800 Subject: [PATCH 10/12] Merge bitcoin/bitcoin#21812: ci: Enable D_GLIBCXX_DEBUG for multiprocess task fa44f5119a0b412f0d46cad02f638727d140b451 ci: Clarify that previous_releases task is using DEBUG (MarcoFalke) fad0f21c3caba129106799fe6c14aff323ef99f2 ci: Use clang in multiprocess task to avoid OOM (MarcoFalke) faeabef4f386009847a0f91041d44e6f31eec618 ci: Enable D_GLIBCXX_DEBUG for multiprocess task (MarcoFalke) Pull request description: Enable `-D_GLIBCXX_DEBUG` via the depends `DEBUG` flag. Also `--enable-debug` to get debug symbols in traces. ACKs for top commit: hebasto: ACK fa44f5119a0b412f0d46cad02f638727d140b451, I have reviewed the code and it looks OK, I agree it can be merged, and CI is green. Tree-SHA512: ab2a216bb44ee462f9dd181ec9025962502bd4201a1118ff52b6a193398e7ea3ca465a45a5eb341e308758fc3ef34ea3521f8a1f85ed64478ef3c1f6c1b8b141 --- .cirrus.yml | 4 ++-- ci/test/00_setup_env_native_multiprocess.sh | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index bb3d4acd70..a5f780728a 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -87,7 +87,7 @@ task: FILE_ENV: "./ci/test/00_setup_env_i686_centos.sh" task: - name: '[previous releases, uses qt5 dev package and some depends packages] [unsigned char] [focal]' + name: '[previous releases, uses qt5 dev package and some depends packages, DEBUG] [unsigned char] [focal]' # For faster CI feedback, immediately schedule a task that compiles most modules << : *CREDITS_TEMPLATE << : *GLOBAL_TASK_TEMPLATE @@ -132,7 +132,7 @@ task: FILE_ENV: "./ci/test/00_setup_env_native_fuzz.sh" task: - name: '[multiprocess] [focal]' + name: '[multiprocess, DEBUG] [focal]' << : *GLOBAL_TASK_TEMPLATE container: image: ubuntu:focal diff --git a/ci/test/00_setup_env_native_multiprocess.sh b/ci/test/00_setup_env_native_multiprocess.sh index 74c57081a7..c070da4ef0 100755 --- a/ci/test/00_setup_env_native_multiprocess.sh +++ b/ci/test/00_setup_env_native_multiprocess.sh @@ -7,8 +7,8 @@ export LC_ALL=C.UTF-8 export CONTAINER_NAME=ci_native_multiprocess -export PACKAGES="cmake python3" -export DEP_OPTS="MULTIPROCESS=1" +export PACKAGES="cmake python3 llvm clang" +export DEP_OPTS="DEBUG=1 MULTIPROCESS=1" export GOAL="install" -export BITCOIN_CONFIG="--with-boost-process" +export BITCOIN_CONFIG="--with-boost-process --enable-debug CC=clang CXX=clang++" # Use clang to avoid OOM export TEST_RUNNER_ENV="BITCOIND=dash-node" From 0431a33919e72c72c851a1137b3a6e9840f8bee3 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Sun, 14 Jul 2024 23:46:55 +0700 Subject: [PATCH 11/12] fix: follow-up changes for bitcoin#14193. net_processing should not lock mempool directly, otherwise it causes wrong-order-lock: Assertion failed: detected inconsistent lock order for 'cs' in txmempool.cpp:1179 (in thread 'msghand'), details in debug log. Posix Signal: Aborted 0#: (0x60322D700254) stl_vector.h:115 - std::_Vector_base >::_Vector_impl_data::_M_copy_data(std::_Vector_base >::_Vector_impl_data const&) 1#: (0x60322D700254) stl_vector.h:127 - std::_Vector_base >::_Vector_impl_data::_M_swap_data(std::_Vector_base >::_Vector_impl_data&) 2#: (0x60322D700254) stl_vector.h:1959 - std::vector >::_M_move_assign(std::vector >&&, std::integral_constant) 3#: (0x60322D700254) stl_vector.h:768 - std::vector >::operator=(std::vector >&&) 4#: (0x60322D700254) stacktraces.cpp:777 - HandlePosixSignal 5#: (0x7FF820442990) libc_sigaction.c - ??? 6#: (0x7FF820499A1B) pthread_kill.c:44 - __pthread_kill_implementation 7#: (0x7FF820499A1B) pthread_kill.c:78 - __pthread_kill_internal 8#: (0x7FF820499A1B) pthread_kill.c:89 - __GI___pthread_kill 9#: (0x7FF8204428E6) raise.c:27 - __GI_raise 10#: (0x7FF8204268B7) abort.c:81 - __GI_abort 11#: (0x60322D705D56) basic_string.h:390 - std::__cxx11::basic_string, std::allocator >::_M_check_length(unsigned long, unsigned long, char const*) const 12#: (0x60322D705D56) basic_string.h:1461 - std::__cxx11::basic_string, std::allocator >::append(char const*) 13#: (0x60322D705D56) basic_string.h:1365 - std::__cxx11::basic_string, std::allocator >::operator+=(char const*) 14#: (0x60322D705D56) sync.cpp:114 - potential_deadlock_detected 15#: (0x60322D70C7DE) sync.cpp:188 - push_lock 16#: (0x60322D70C7DE) sync.cpp:212 - void EnterCritical(char const*, char const*, int, std::recursive_mutex*, bool) 17#: (0x60322CFAA98C) unique_lock.h:150 - std::unique_lock::try_lock() 18#: (0x60322CFAA98C) sync.h:162 - UniqueLock, std::unique_lock >::Enter(char const*, char const*, int) 19#: (0x60322D243A99) hasher.h:22 - SaltedTxidHasher::operator()(uint256 const&) const 20#: (0x60322D243A99) hashed_index.hpp:1605 - boost::multi_index::detail::hashed_index_iterator > > > > >, boost::multi_index::detail::bucket_array >, boost::multi_index::detail::hashed_unique_tag, boost::multi_index::detail::hashed_index_global_iterator_tag> boost::multi_index::detail::hashed_index, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByAncestorFee>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, std::allocator >, boost::mpl::vector0, boost::multi_index::detail::hashed_unique_tag>::find >(uint256 const&, SaltedTxidHasher const&, std::equal_to const&, mpl_::bool_) const 21#: (0x60322D243A99) hashed_index.hpp:541 - boost::multi_index::detail::hashed_index_iterator > > > > >, boost::multi_index::detail::bucket_array >, boost::multi_index::detail::hashed_unique_tag, boost::multi_index::detail::hashed_index_global_iterator_tag> boost::multi_index::detail::hashed_index, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByAncestorFee>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, std::allocator >, boost::mpl::vector0, boost::multi_index::detail::hashed_unique_tag>::find >(uint256 const&, SaltedTxidHasher const&, std::equal_to const&) const 22#: (0x60322D243A99) hashed_index.hpp:531 - boost::multi_index::detail::hashed_index_iterator > > > > >, boost::multi_index::detail::bucket_array >, boost::multi_index::detail::hashed_unique_tag, boost::multi_index::detail::hashed_index_global_iterator_tag> boost::multi_index::detail::hashed_index, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique, boost::multi_index::identity, CompareTxMemPoolEntryByAncestorFee>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, std::allocator >, boost::mpl::vector0, boost::multi_index::detail::hashed_unique_tag>::find(uint256 const&) const 23#: (0x60322D243A99) txmempool.cpp:1180 - CTxMemPool::CompareDepthAndScore(uint256 const&, uint256 const&) 24#: (0x60322CFFE799) stl_heap.h:140 - __adjust_heap<__gnu_cxx::__normal_iterator*, std::vector > >, long int, std::_Rb_tree_const_iterator, __gnu_cxx::__ops::_Iter_comp_iter<(anonymous namespace)::CompareInvMempoolOrder> > 25#: (0x60322D0241D7) stl_heap.h:358 - __make_heap<__gnu_cxx::__normal_iterator*, std::vector > >, __gnu_cxx::__ops::_Iter_comp_iter<(anonymous namespace)::CompareInvMempoolOrder> > 26#: (0x60322D0241D7) stl_heap.h:413 - make_heap<__gnu_cxx::__normal_iterator*, std::vector > >, (anonymous namespace)::CompareInvMempoolOrder> 27#: (0x60322D0241D7) net_processing.cpp:5728 - SendMessages 28#: (0x60322CFD00C3) sync.h:199 - UniqueLock, std::unique_lock >::~UniqueLock() 29#: (0x60322CFD00C3) net.cpp:3181 - CConnman::ThreadMessageHandler() 30#: (0x60322D75C3D5) basic_string.h:639 - std::__cxx11::basic_string, std::allocator >::basic_string >(char const*, std::allocator const&) 31#: (0x60322D75C3D5) thread.cpp:19 - util::TraceThread(char const*, std::function) 32#: (0x60322CFC2C7C) std_function.h:243 - std::_Function_base::~_Function_base() 33#: (0x60322CFC2C7C) std_function.h:334 - std::function::~function() 34#: (0x60322CFC2C7C) invoke.h:61 - __invoke_impl), char const*, CConnman::Start(CDeterministicMNManager&, CMasternodeMetaMan&, CMasternodeSync&, CScheduler&, const Options&):: > 35#: (0x60322CFC2C7C) invoke.h:96 - __invoke), char const*, CConnman::Start(CDeterministicMNManager&, CMasternodeMetaMan&, CMasternodeSync&, CScheduler&, const Options&):: > 36#: (0x60322CFC2C7C) std_thread.h:292 - _M_invoke<0, 1, 2> 37#: (0x60322CFC2C7C) std_thread.h:299 - operator() 38#: (0x60322CFC2C7C) std_thread.h:244 - _M_run 39#: (0x7FF8208E6333) - ??? --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 7680696d37..3c1f399496 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5622,7 +5622,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // std::vector vInv; { - LOCK2(m_mempool.cs, peer->m_block_inv_mutex); + LOCK(peer->m_block_inv_mutex); size_t reserve = INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK * MaxBlockSize() / 1000000; if (!pto->IsBlockOnlyConn()) { From f4cb0fbfe10b070eee36a2a6833a705aaf533ce0 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 15 Jul 2024 02:08:56 +0700 Subject: [PATCH 12/12] fix: no need to relay quorum commitment in case of block undo It fixes potentiall deadlock: Assertion failed: detected inconsistent lock order for 'peer.m_tx_relay->m_tx_inventory_mutex' in net_processing.cpp:971 (in thread 'httpworker.0'), details in debug log. Posix Signal: Aborted 0#: (0x5BE0A9B78F04) stl_vector.h:115 - std::_Vector_base >::_Vector_impl_data::_M_copy_data(std::_Vector_base >::_Vector_impl_data const&) 1#: (0x5BE0A9B78F04) stl_vector.h:127 - std::_Vector_base >::_Vector_impl_data::_M_swap_data(std::_Vector_base >::_Vector_impl_data&) 2#: (0x5BE0A9B78F04) stl_vector.h:1959 - std::vector >::_M_move_assign(std::vector >&&, std::integral_constant) 3#: (0x5BE0A9B78F04) stl_vector.h:768 - std::vector >::operator=(std::vector >&&) 4#: (0x5BE0A9B78F04) stacktraces.cpp:777 - HandlePosixSignal 5#: (0x733859C42990) libc_sigaction.c - ??? 6#: (0x733859C99A1B) pthread_kill.c:44 - __pthread_kill_implementation 7#: (0x733859C99A1B) pthread_kill.c:78 - __pthread_kill_internal 8#: (0x733859C99A1B) pthread_kill.c:89 - __GI___pthread_kill 9#: (0x733859C428E6) raise.c:27 - __GI_raise 10#: (0x733859C268B7) abort.c:81 - __GI_abort 11#: (0x5BE0A9B7EA06) basic_string.h:390 - std::__cxx11::basic_string, std::allocator >::_M_check_length(unsigned long, unsigned long, char const*) const 12#: (0x5BE0A9B7EA06) basic_string.h:1461 - std::__cxx11::basic_string, std::allocator >::append(char const*) 13#: (0x5BE0A9B7EA06) basic_string.h:1365 - std::__cxx11::basic_string, std::allocator >::operator+=(char const*) 14#: (0x5BE0A9B7EA06) sync.cpp:114 - potential_deadlock_detected 15#: (0x5BE0A9B8548E) sync.cpp:188 - push_lock 16#: (0x5BE0A9B8548E) sync.cpp:212 - void EnterCritical(char const*, char const*, int, std::recursive_mutex*, bool) 17#: (0x5BE0A935C582) unique_lock.h:150 - std::unique_lock::try_lock() 18#: (0x5BE0A935C582) sync.h:162 - UniqueLock, std::unique_lock >::Enter(char const*, char const*, int) 19#: (0x5BE0A935C582) sync.h:183 - UniqueLock, std::unique_lock >::UniqueLock(AnnotatedMixin&, char const*, char const*, int, bool) 20#: (0x5BE0A9487A92) net_processing.cpp:972 - PushInv 21#: (0x5BE0A94896E5) shared_ptr_base.h:1070 - std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() 22#: (0x5BE0A94896E5) shared_ptr_base.h:1524 - ~__shared_ptr 23#: (0x5BE0A94896E5) shared_ptr.h:175 - ~shared_ptr 24#: (0x5BE0A94896E5) net_processing.cpp:2276 - operator() 25#: (0x5BE0A94896E5) net.h:1051 - ForEachNode&> 26#: (0x5BE0A94896E5) net.h:1058 - ForEachNode<(anonymous namespace)::PeerManagerImpl::RelayInv(CInv&, int):: > 27#: (0x5BE0A94896E5) net_processing.cpp:2269 - RelayInv 28#: (0x5BE0A98B7C03) blockprocessor.cpp:683 - llmq::CQuorumBlockProcessor::AddMineableCommitment(llmq::CFinalCommitment const&) 29#: (0x5BE0A98BE2E1) blockprocessor.cpp:338 - llmq::CQuorumBlockProcessor::UndoBlock(CBlock const&, gsl::not_null) 30#: (0x5BE0A9809EA9) specialtxman.cpp:264 - CSpecialTxProcessor::UndoSpecialTxsInBlock(CBlock const&, CBlockIndex const*, std::optional&) 31#: (0x5BE0A96FFB84) validation.cpp:1693 - CChainState::DisconnectBlock(CBlock const&, CBlockIndex const*, CCoinsViewCache&) 32#: (0x5BE0A9701481) validation.cpp:2726 - CChainState::DisconnectTip(BlockValidationState&, DisconnectedBlockTransactions*) --- src/llmq/blockprocessor.cpp | 14 ++++++++++---- src/llmq/blockprocessor.h | 4 +++- src/llmq/dkgsessionhandler.cpp | 2 +- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index 7e0246346b..98c827ef6c 100644 --- a/src/llmq/blockprocessor.cpp +++ b/src/llmq/blockprocessor.cpp @@ -138,7 +138,7 @@ PeerMsgRet CQuorumBlockProcessor::ProcessMessage(const CNode& peer, std::string_ LogPrint(BCLog::LLMQ, "CQuorumBlockProcessor::%s -- received commitment for quorum %s:%d, validMembers=%d, signers=%d, peer=%d\n", __func__, qc.quorumHash.ToString(), ToUnderlying(qc.llmqType), qc.CountValidMembers(), qc.CountSigners(), peer.GetId()); - AddMineableCommitment(qc); + AddMineableCommitmentAndRelay(qc); return {}; } @@ -636,7 +636,7 @@ bool CQuorumBlockProcessor::HasMineableCommitment(const uint256& hash) const return minableCommitments.count(hash) != 0; } -void CQuorumBlockProcessor::AddMineableCommitment(const CFinalCommitment& fqc) +std::optional CQuorumBlockProcessor::AddMineableCommitment(const CFinalCommitment& fqc) { const uint256 commitmentHash = ::SerializeHash(fqc); @@ -662,9 +662,15 @@ void CQuorumBlockProcessor::AddMineableCommitment(const CFinalCommitment& fqc) return false; }(); + return relay ? std::make_optional(commitmentHash) : std::nullopt; +} + +void CQuorumBlockProcessor::AddMineableCommitmentAndRelay(const CFinalCommitment& fqc) +{ + const auto commitmentHashOpt = AddMineableCommitment(fqc); // We only relay the new commitment if it's new or better then the old one - if (relay) { - CInv inv(MSG_QUORUM_FINAL_COMMITMENT, commitmentHash); + if (commitmentHashOpt) { + CInv inv(MSG_QUORUM_FINAL_COMMITMENT, *commitmentHashOpt); Assert(m_peerman)->RelayInv(inv); } } diff --git a/src/llmq/blockprocessor.h b/src/llmq/blockprocessor.h index 8d912b530b..2deeebfd90 100644 --- a/src/llmq/blockprocessor.h +++ b/src/llmq/blockprocessor.h @@ -59,7 +59,7 @@ public: bool ProcessBlock(const CBlock& block, gsl::not_null pindex, BlockValidationState& state, bool fJustCheck, bool fBLSChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool UndoBlock(const CBlock& block, gsl::not_null pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - void AddMineableCommitment(const CFinalCommitment& fqc); + void AddMineableCommitmentAndRelay(const CFinalCommitment& fqc); bool HasMineableCommitment(const uint256& hash) const; bool GetMineableCommitmentByHash(const uint256& commitmentHash, CFinalCommitment& ret) const; std::optional> GetMineableCommitments(const Consensus::LLMQParams& llmqParams, int nHeight) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -74,6 +74,8 @@ public: std::vector> GetLastMinedCommitmentsPerQuorumIndexUntilBlock(Consensus::LLMQType llmqType, const CBlockIndex* pindex, size_t cycle) const; std::optional GetLastMinedCommitmentsByQuorumIndexUntilBlock(Consensus::LLMQType llmqType, const CBlockIndex* pindex, int quorumIndex, size_t cycle) const; private: + //! it returns hash of commitment if it should be relay, otherwise nullopt + std::optional AddMineableCommitment(const CFinalCommitment& fqc); static bool GetCommitmentsFromBlock(const CBlock& block, gsl::not_null pindex, std::multimap& ret, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool ProcessCommitment(int nHeight, const uint256& blockHash, const CFinalCommitment& qc, BlockValidationState& state, bool fJustCheck, bool fBLSChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); static bool IsMiningPhase(const Consensus::LLMQParams& llmqParams, const CChain& active_chain, int nHeight) EXCLUSIVE_LOCKS_REQUIRED(cs_main); diff --git a/src/llmq/dkgsessionhandler.cpp b/src/llmq/dkgsessionhandler.cpp index c0a500b1e3..e6f301cfa0 100644 --- a/src/llmq/dkgsessionhandler.cpp +++ b/src/llmq/dkgsessionhandler.cpp @@ -570,7 +570,7 @@ void CDKGSessionHandler::HandleDKGRound() auto finalCommitments = curSession->FinalizeCommitments(); for (const auto& fqc : finalCommitments) { - quorumBlockProcessor.AddMineableCommitment(fqc); + quorumBlockProcessor.AddMineableCommitmentAndRelay(fqc); } }