From 32c80413fdb063199f3bee719c4651bd63f05fce Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 20 Dec 2023 15:41:05 -0500 Subject: [PATCH 01/55] bench: add benchmark for checkblockindex --- src/Makefile.bench.include | 1 + src/bench/checkblockindex.cpp | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 src/bench/checkblockindex.cpp diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index 7ba0111fa686f..2ba72c9e76b7f 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -23,6 +23,7 @@ bench_bench_bitcoin_SOURCES = \ bench/ccoins_caching.cpp \ bench/chacha20.cpp \ bench/checkblock.cpp \ + bench/checkblockindex.cpp \ bench/checkqueue.cpp \ bench/crypto_hash.cpp \ bench/data.cpp \ diff --git a/src/bench/checkblockindex.cpp b/src/bench/checkblockindex.cpp new file mode 100644 index 0000000000000..e8a848dbd409b --- /dev/null +++ b/src/bench/checkblockindex.cpp @@ -0,0 +1,20 @@ +// Copyright (c) 2023-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include + +static void CheckBlockIndex(benchmark::Bench& bench) +{ + auto testing_setup{MakeNoLogFileContext()}; + // Mine some more blocks + testing_setup->mineBlocks(1000); + bench.run([&] { + testing_setup->m_node.chainman->CheckBlockIndex(); + }); +} + + +BENCHMARK(CheckBlockIndex, benchmark::PriorityLevel::HIGH); From d5a631b9597e5029a5048d9b8ad84ea4536bbac0 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 23 Aug 2023 14:05:42 -0400 Subject: [PATCH 02/55] validation: improve performance of CheckBlockIndex by not saving all indexes in a std::multimap, but only those that are not part of the best header chain. The indexes of the best header chain are stored in a vector, which, in the typical case of a mostly linear chain with a few forks, results in a much smaller multimap, and increases performance noticeably for long chains. This does not change the actual consistency checks that are being performed for each index, just the way the block index tree is stored and traversed. Co-authored-by: Ryan Ofsky --- src/validation.cpp | 50 +++++++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index b6d0c38f391da..9dad39e6579e5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5050,19 +5050,28 @@ void ChainstateManager::CheckBlockIndex() return; } - // Build forward-pointing map of the entire block tree. + // Build forward-pointing data structure for the entire block tree. + // For performance reasons, indexes of the best header chain are stored in a vector (within CChain). + // All remaining blocks are stored in a multimap. + // The best header chain can differ from the active chain: E.g. its entries may belong to blocks that + // are not yet validated. + CChain best_hdr_chain; + assert(m_best_header); + best_hdr_chain.SetTip(*m_best_header); + std::multimap forward; for (auto& [_, block_index] : m_blockman.m_block_index) { - forward.emplace(block_index.pprev, &block_index); + // Only save indexes in forward that are not part of the best header chain. + if (!best_hdr_chain.Contains(&block_index)) { + // Only genesis, which must be part of the best header chain, can have a nullptr parent. + assert(block_index.pprev); + forward.emplace(block_index.pprev, &block_index); + } } + assert(forward.size() + best_hdr_chain.Height() + 1 == m_blockman.m_block_index.size()); - assert(forward.size() == m_blockman.m_block_index.size()); - - std::pair::iterator,std::multimap::iterator> rangeGenesis = forward.equal_range(nullptr); - CBlockIndex *pindex = rangeGenesis.first->second; - rangeGenesis.first++; - assert(rangeGenesis.first == rangeGenesis.second); // There is only one index entry with parent nullptr. - + CBlockIndex* pindex = best_hdr_chain[0]; + assert(pindex); // Iterate over the entire block tree, using depth-first search. // Along the way, remember whether there are blocks on the path from genesis // block being explored which are the first to have certain properties. @@ -5274,14 +5283,21 @@ void ChainstateManager::CheckBlockIndex() // assert(pindex->GetBlockHash() == pindex->GetBlockHeader().GetHash()); // Perhaps too slow // End: actual consistency checks. - // Try descending into the first subnode. + + // Try descending into the first subnode. Always process forks first and the best header chain after. snap_update_firsts(); std::pair::iterator,std::multimap::iterator> range = forward.equal_range(pindex); if (range.first != range.second) { - // A subnode was found. + // A subnode not part of the best header chain was found. pindex = range.first->second; nHeight++; continue; + } else if (best_hdr_chain.Contains(pindex)) { + // Descend further into best header chain. + nHeight++; + pindex = best_hdr_chain[nHeight]; + if (!pindex) break; // we are finished, since the best header chain is always processed last + continue; } // This is a leaf node. // Move upwards until we reach a node of which we have not yet visited the last child. @@ -5307,9 +5323,15 @@ void ChainstateManager::CheckBlockIndex() // Proceed to the next one. rangePar.first++; if (rangePar.first != rangePar.second) { - // Move to the sibling. + // Move to a sibling not part of the best header chain. pindex = rangePar.first->second; break; + } else if (pindexPar == best_hdr_chain[nHeight - 1]) { + // Move to pindex's sibling on the best-chain, if it has one. + pindex = best_hdr_chain[nHeight]; + // There will not be a next block if (and only if) parent block is the best header. + assert((pindex == nullptr) == (pindexPar == best_hdr_chain.Tip())); + break; } else { // Move up further. pindex = pindexPar; @@ -5319,8 +5341,8 @@ void ChainstateManager::CheckBlockIndex() } } - // Check that we actually traversed the entire map. - assert(nNodes == forward.size()); + // Check that we actually traversed the entire block index. + assert(nNodes == forward.size() + best_hdr_chain.Height() + 1); } std::string Chainstate::ToString() From 5bc2077e8f592442b089affdf0b5795fbc053bb8 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Thu, 21 Dec 2023 17:24:07 -0500 Subject: [PATCH 03/55] validation: allow to specify frequency for -checkblockindex This makes it similar to -checkaddrman and -checkmempool, which also allow to run the check occasionally instead of always / never. Co-authored-by: Ryan Ofsky --- src/init.cpp | 2 +- src/kernel/chainstatemanager_opts.h | 2 +- src/node/chainstatemanager_args.cpp | 5 ++++- src/test/util/setup_common.cpp | 2 +- src/validation.cpp | 8 ++++++++ src/validation.h | 2 +- 6 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 0aa04755cbeba..927786e3d5950 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -604,7 +604,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-checkblocks=", strprintf("How many blocks to check at startup (default: %u, 0 = all)", DEFAULT_CHECKBLOCKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checklevel=", strprintf("How thorough the block verification of -checkblocks is: %s (0-4, default: %u)", Join(CHECKLEVEL_DOC, ", "), DEFAULT_CHECKLEVEL), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); - argsman.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, chainstate, and other validation data structures occasionally. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, chainstate, and other validation data structures every operations. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkaddrman=", strprintf("Run addrman consistency checks every operations. Use 0 to disable. (default: %u)", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkmempool=", strprintf("Run mempool consistency checks every transactions. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %u)", defaultChainParams->Checkpoints().GetHeight(), DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h index de5f78494a261..076841c3c9d4a 100644 --- a/src/kernel/chainstatemanager_opts.h +++ b/src/kernel/chainstatemanager_opts.h @@ -33,7 +33,7 @@ namespace kernel { struct ChainstateManagerOpts { const CChainParams& chainparams; fs::path datadir; - std::optional check_block_index{}; + std::optional check_block_index{}; bool checkpoints_enabled{DEFAULT_CHECKPOINTS_ENABLED}; //! If set, it will override the minimum work we will assume exists on some valid chain. std::optional minimum_chain_work{}; diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp index 1cc126cb051c3..bc4a815a3edf8 100644 --- a/src/node/chainstatemanager_args.cpp +++ b/src/node/chainstatemanager_args.cpp @@ -24,7 +24,10 @@ namespace node { util::Result ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts) { - if (auto value{args.GetBoolArg("-checkblockindex")}) opts.check_block_index = *value; + if (auto value{args.GetIntArg("-checkblockindex")}) { + // Interpret bare -checkblockindex argument as 1 instead of 0. + opts.check_block_index = args.GetArg("-checkblockindex")->empty() ? 1 : *value; + } if (auto value{args.GetBoolArg("-checkpoints")}) opts.checkpoints_enabled = *value; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2c18184261e48..666a7a03034fd 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -238,7 +238,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto const ChainstateManager::Options chainman_opts{ .chainparams = chainparams, .datadir = m_args.GetDataDirNet(), - .check_block_index = true, + .check_block_index = 1, .notifications = *m_node.notifications, .signals = m_node.validation_signals.get(), .worker_threads_num = 2, diff --git a/src/validation.cpp b/src/validation.cpp index 9dad39e6579e5..a547614fc140e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5034,6 +5034,14 @@ void ChainstateManager::LoadExternalBlockFile( LogPrintf("Loaded %i blocks from external file in %dms\n", nLoaded, Ticks(SteadyClock::now() - start)); } +bool ChainstateManager::ShouldCheckBlockIndex() const +{ + // Assert to verify Flatten() has been called. + if (!*Assert(m_options.check_block_index)) return false; + if (GetRand(*m_options.check_block_index) >= 1) return false; + return true; +} + void ChainstateManager::CheckBlockIndex() { if (!ShouldCheckBlockIndex()) { diff --git a/src/validation.h b/src/validation.h index bcf153719af9a..15a1cbdc8de85 100644 --- a/src/validation.h +++ b/src/validation.h @@ -933,7 +933,7 @@ class ChainstateManager const CChainParams& GetParams() const { return m_options.chainparams; } const Consensus::Params& GetConsensus() const { return m_options.chainparams.GetConsensus(); } - bool ShouldCheckBlockIndex() const { return *Assert(m_options.check_block_index); } + bool ShouldCheckBlockIndex() const; const arith_uint256& MinimumChainWork() const { return *Assert(m_options.minimum_chain_work); } const uint256& AssumedValidBlock() const { return *Assert(m_options.assumed_valid_block); } kernel::Notifications& GetNotifications() const { return m_options.notifications; }; From e208fb5d3bea4c1fb750cb0028819635ecdeb415 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sat, 27 Apr 2024 18:09:12 -0400 Subject: [PATCH 04/55] cli: Sanitize ports in rpcconnect and rpcport Adds error handling of invalid ports to rpcconnect and rpcport, with associated functional tests. --- src/bitcoin-cli.cpp | 31 ++++++++++++++- test/functional/interface_bitcoin_cli.py | 49 ++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index c7ba2204c3fda..eb1ee6134a935 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -743,8 +743,35 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co // 2. port in -rpcconnect (ie following : in ipv4 or ]: in ipv6) // 3. default port for chain uint16_t port{BaseParams().RPCPort()}; - SplitHostPort(gArgs.GetArg("-rpcconnect", DEFAULT_RPCCONNECT), port, host); - port = static_cast(gArgs.GetIntArg("-rpcport", port)); + { + uint16_t rpcconnect_port{0}; + const std::string rpcconnect_str = gArgs.GetArg("-rpcconnect", DEFAULT_RPCCONNECT); + if (!SplitHostPort(rpcconnect_str, rpcconnect_port, host)) { + // Uses argument provided as-is + // (rather than value parsed) + // to aid the user in troubleshooting + throw std::runtime_error(strprintf("Invalid port provided in -rpcconnect: %s", rpcconnect_str)); + } else { + if (rpcconnect_port != 0) { + // Use the valid port provided in rpcconnect + port = rpcconnect_port; + } // else, no port was provided in rpcconnect (continue using default one) + } + + if (std::optional rpcport_arg = gArgs.GetArg("-rpcport")) { + // -rpcport was specified + const uint16_t rpcport_int{ToIntegral(rpcport_arg.value()).value_or(0)}; + if (rpcport_int == 0) { + // Uses argument provided as-is + // (rather than value parsed) + // to aid the user in troubleshooting + throw std::runtime_error(strprintf("Invalid port provided in -rpcport: %s", rpcport_arg.value())); + } + + // Use the valid port provided + port = rpcport_int; + } + } // Obtain event base raii_event_base base = obtain_event_base(); diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py index 83bb5121e5b4a..a6628dcbf3f88 100755 --- a/test/functional/interface_bitcoin_cli.py +++ b/test/functional/interface_bitcoin_cli.py @@ -8,6 +8,7 @@ import re from test_framework.blocktools import COINBASE_MATURITY +from test_framework.netutil import test_ipv6_local from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -15,6 +16,7 @@ assert_raises_process_error, assert_raises_rpc_error, get_auth_cookie, + rpc_port, ) import time @@ -107,6 +109,53 @@ def run_test(self): self.log.info("Test connecting to a non-existing server") assert_raises_process_error(1, "Could not connect to the server", self.nodes[0].cli('-rpcport=1').echo) + self.log.info("Test handling of invalid ports in rpcconnect") + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:notaport", self.nodes[0].cli("-rpcconnect=127.0.0.1:notaport").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:-1", self.nodes[0].cli("-rpcconnect=127.0.0.1:-1").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:0", self.nodes[0].cli("-rpcconnect=127.0.0.1:0").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:65536", self.nodes[0].cli("-rpcconnect=127.0.0.1:65536").echo) + + self.log.info("Checking for IPv6") + have_ipv6 = test_ipv6_local() + if not have_ipv6: + self.log.info("Skipping IPv6 tests") + + if have_ipv6: + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: [::1]:notaport", self.nodes[0].cli("-rpcconnect=[::1]:notaport").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: [::1]:-1", self.nodes[0].cli("-rpcconnect=[::1]:-1").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: [::1]:0", self.nodes[0].cli("-rpcconnect=[::1]:0").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: [::1]:65536", self.nodes[0].cli("-rpcconnect=[::1]:65536").echo) + + self.log.info("Test handling of invalid ports in rpcport") + assert_raises_process_error(1, "Invalid port provided in -rpcport: notaport", self.nodes[0].cli("-rpcport=notaport").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcport: -1", self.nodes[0].cli("-rpcport=-1").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcport: 0", self.nodes[0].cli("-rpcport=0").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcport: 65536", self.nodes[0].cli("-rpcport=65536").echo) + + self.log.info("Test port usage preferences") + node_rpc_port = rpc_port(self.nodes[0].index) + # Prevent bitcoin-cli from using existing rpcport in conf + conf_rpcport = "rpcport=" + str(node_rpc_port) + self.nodes[0].replace_in_config([(conf_rpcport, "#" + conf_rpcport)]) + # prefer rpcport over rpcconnect + assert_raises_process_error(1, "Could not connect to the server 127.0.0.1:1", self.nodes[0].cli(f"-rpcconnect=127.0.0.1:{node_rpc_port}", "-rpcport=1").echo) + if have_ipv6: + assert_raises_process_error(1, "Could not connect to the server ::1:1", self.nodes[0].cli(f"-rpcconnect=[::1]:{node_rpc_port}", "-rpcport=1").echo) + + assert_equal(BLOCKS, self.nodes[0].cli("-rpcconnect=127.0.0.1:18999", f'-rpcport={node_rpc_port}').getblockcount()) + if have_ipv6: + assert_equal(BLOCKS, self.nodes[0].cli("-rpcconnect=[::1]:18999", f'-rpcport={node_rpc_port}').getblockcount()) + + # prefer rpcconnect port over default + assert_equal(BLOCKS, self.nodes[0].cli(f"-rpcconnect=127.0.0.1:{node_rpc_port}").getblockcount()) + if have_ipv6: + assert_equal(BLOCKS, self.nodes[0].cli(f"-rpcconnect=[::1]:{node_rpc_port}").getblockcount()) + + # prefer rpcport over default + assert_equal(BLOCKS, self.nodes[0].cli(f'-rpcport={node_rpc_port}').getblockcount()) + # Re-enable rpcport in conf if present + self.nodes[0].replace_in_config([("#" + conf_rpcport, conf_rpcport)]) + self.log.info("Test connecting with non-existing RPC cookie file") assert_raises_process_error(1, "Could not locate RPC credentials", self.nodes[0].cli('-rpccookiefile=does-not-exist', '-rpcpassword=').echo) From 24bc46c83b39149f4845a575a82337eb46d91bdb Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sat, 27 Apr 2024 18:10:57 -0400 Subject: [PATCH 05/55] cli: Add warning for duplicate port definition Adds a warning when both -rpcconnect and -rpcport define a port to be used. --- src/bitcoin-cli.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index eb1ee6134a935..6352044ba7340 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -770,6 +770,12 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co // Use the valid port provided port = rpcport_int; + + // If there was a valid port provided in rpcconnect, + // rpcconnect_port is non-zero. + if (rpcconnect_port != 0) { + tfm::format(std::cerr, "Warning: Port specified in both -rpcconnect and -rpcport. Using -rpcport %u\n", port); + } } } From ffa27af24da81a97d6c4912ae0e10bc5b6f17f69 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 10 May 2024 16:06:03 -0400 Subject: [PATCH 06/55] test: Add check-deps.sh script to check for unexpected library dependencies --- contrib/devtools/check-deps.sh | 203 +++++++++++++++++++++++++++++++++ 1 file changed, 203 insertions(+) create mode 100755 contrib/devtools/check-deps.sh diff --git a/contrib/devtools/check-deps.sh b/contrib/devtools/check-deps.sh new file mode 100755 index 0000000000000..9d2eebe14d8de --- /dev/null +++ b/contrib/devtools/check-deps.sh @@ -0,0 +1,203 @@ +#!/usr/bin/env bash + +export LC_ALL=C +set -Eeuo pipefail + +# Declare paths to libraries +declare -A LIBS +LIBS[cli]="libbitcoin_cli.a" +LIBS[common]="libbitcoin_common.a" +LIBS[consensus]="libbitcoin_consensus.a" +LIBS[crypto]="crypto/.libs/libbitcoin_crypto_base.a crypto/.libs/libbitcoin_crypto_x86_shani.a crypto/.libs/libbitcoin_crypto_sse41.a crypto/.libs/libbitcoin_crypto_avx2.a" +LIBS[node]="libbitcoin_node.a" +LIBS[util]="libbitcoin_util.a" +LIBS[wallet]="libbitcoin_wallet.a" +LIBS[wallet_tool]="libbitcoin_wallet_tool.a" + +# Declare allowed dependencies "X Y" where X is allowed to depend on Y. This +# list is taken from doc/design/libraries.md. +ALLOWED_DEPENDENCIES=( + "cli common" + "cli util" + "common consensus" + "common crypto" + "common util" + "consensus crypto" + "node common" + "node consensus" + "node crypto" + "node kernel" + "node util" + "util crypto" + "wallet common" + "wallet crypto" + "wallet util" + "wallet_tool util" + "wallet_tool wallet" +) + +# Add minor dependencies omitted from doc/design/libraries.md to keep the +# dependency diagram simple. +ALLOWED_DEPENDENCIES+=( + "wallet consensus" + "wallet_tool common" + "wallet_tool crypto" +) + +# Declare list of known errors that should be suppressed. +declare -A SUPPRESS +# init.cpp file currently calls Berkeley DB sanity check function on startup, so +# there is an undocumented dependency of the node library on the wallet library. +SUPPRESS["libbitcoin_node_a-init.o libbitcoin_wallet_a-bdb.o _ZN6wallet27BerkeleyDatabaseSanityCheckEv"]=1 +# init/common.cpp file calls InitError and InitWarning from interface_ui which +# is currently part of the node library. interface_ui should just be part of the +# common library instead, and is moved in +# https://github.com/bitcoin/bitcoin/issues/10102 +SUPPRESS["libbitcoin_common_a-common.o libbitcoin_node_a-interface_ui.o _Z11InitWarningRK13bilingual_str"]=1 +SUPPRESS["libbitcoin_common_a-common.o libbitcoin_node_a-interface_ui.o _Z9InitErrorRK13bilingual_str"]=1 +# rpc/external_signer.cpp adds defines node RPC methods but is built as part of the +# common library. It should be moved to the node library instead. +SUPPRESS["libbitcoin_common_a-external_signer.o libbitcoin_node_a-server.o _ZN9CRPCTable13appendCommandERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEPK11CRPCCommand"]=1 + +usage() { + echo "Usage: $(basename "${BASH_SOURCE[0]}") [BUILD_DIR]" +} + +# Output makefile targets, converting library .a paths to libtool .la targets +lib_targets() { + for lib in "${!LIBS[@]}"; do + for lib_path in ${LIBS[$lib]}; do + # shellcheck disable=SC2001 + sed 's:/.libs/\(.*\)\.a$:/\1.la:g' <<<"$lib_path" + done + done +} + +# Extract symbol names and object names and write to text files +extract_symbols() { + local temp_dir="$1" + for lib in "${!LIBS[@]}"; do + for lib_path in ${LIBS[$lib]}; do + nm -o "$lib_path" | grep ' T ' | awk '{print $3, $1}' >> "${temp_dir}/${lib}_exports.txt" + nm -o "$lib_path" | grep ' U ' | awk '{print $3, $1}' >> "${temp_dir}/${lib}_imports.txt" + awk '{print $1}' "${temp_dir}/${lib}_exports.txt" | sort -u > "${temp_dir}/${lib}_exported_symbols.txt" + awk '{print $1}' "${temp_dir}/${lib}_imports.txt" | sort -u > "${temp_dir}/${lib}_imported_symbols.txt" + done + done +} + +# Lookup object name(s) corresponding to symbol name in text file +obj_names() { + local symbol="$1" + local txt_file="$2" + sed -n "s/^$symbol [^:]\\+:\\([^:]\\+\\):[^:]*\$/\\1/p" "$txt_file" | sort -u +} + +# Iterate through libraries and find disallowed dependencies +check_libraries() { + local temp_dir="$1" + local result=0 + for src in "${!LIBS[@]}"; do + for dst in "${!LIBS[@]}"; do + if [ "$src" != "$dst" ] && ! is_allowed "$src" "$dst"; then + if ! check_disallowed "$src" "$dst" "$temp_dir"; then + result=1 + fi + fi + done + done + check_not_suppressed + return $result +} + +# Return whether src library is allowed to depend on dst. +is_allowed() { + local src="$1" + local dst="$2" + for allowed in "${ALLOWED_DEPENDENCIES[@]}"; do + if [ "$src $dst" = "$allowed" ]; then + return 0 + fi + done + return 1 +} + +# Return whether src library imports any symbols from dst, assuming src is not +# allowed to depend on dst. +check_disallowed() { + local src="$1" + local dst="$2" + local temp_dir="$3" + local result=0 + + # Loop over symbol names exported by dst and imported by src + while read symbol; do + local dst_obj + dst_obj=$(obj_names "$symbol" "${temp_dir}/${dst}_exports.txt") + while read src_obj; do + if ! check_suppress "$src_obj" "$dst_obj" "$symbol"; then + echo "Error: $src_obj depends on $dst_obj symbol '$(c++filt "$symbol")', can suppess with:" + echo " SUPPRESS[\"$src_obj $dst_obj $symbol\"]=1" + result=1 + fi + done < <(obj_names "$symbol" "${temp_dir}/${src}_imports.txt") + done < <(comm -12 "${temp_dir}/${dst}_exported_symbols.txt" "${temp_dir}/${src}_imported_symbols.txt") + return $result +} + +# Declare array to track errors which were suppressed. +declare -A SUPPRESSED + +# Return whether error should be suppressed and record suppresssion in +# SUPPRESSED array. +check_suppress() { + local src_obj="$1" + local dst_obj="$2" + local symbol="$3" + for suppress in "${!SUPPRESS[@]}"; do + read suppress_src suppress_dst suppress_pattern <<<"$suppress" + if [[ "$src_obj" == "$suppress_src" && "$dst_obj" == "$suppress_dst" && "$symbol" =~ $suppress_pattern ]]; then + SUPPRESSED["$suppress"]=1 + return 0 + fi + done + return 1 +} + +# Warn about error which were supposed to be suppress, but were not encountered. +check_not_suppressed() { + for suppress in "${!SUPPRESS[@]}"; do + if [[ ! -v SUPPRESSED[$suppress] ]]; then + echo >&2 "Warning: suppression '$suppress' was ignored, consider deleting." + fi + done +} + +# Check arguments. +if [ "$#" = 0 ]; then + BUILD_DIR="$(dirname "${BASH_SOURCE[0]}")/../../src" +elif [ "$#" = 1 ]; then + BUILD_DIR="$1" +else + echo >&2 "Error: wrong number of arguments." + usage >&2 + exit 1 +fi +if [ ! -f "$BUILD_DIR/Makefile" ]; then + echo >&2 "Error: directory '$BUILD_DIR' does not contain a makefile, please specify path to build directory for library targets." + usage >&2 + exit 1 +fi + +# Build libraries and run checks. +cd "$BUILD_DIR" +# shellcheck disable=SC2046 +make -j"$(nproc)" $(lib_targets) +TEMP_DIR="$(mktemp -d)" +extract_symbols "$TEMP_DIR" +if check_libraries "$TEMP_DIR"; then + echo "Success! No unexpected dependencies were detected." +else + echo >&2 "Error: Unexpected dependencies were detected. Check previous output." +fi +rm -r "$TEMP_DIR" From 5b9309420cc9721a0d5745b6ad3166a4bdbd1508 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 10 May 2024 15:45:07 -0400 Subject: [PATCH 07/55] build: move chainparamsbase from util to common Move chainparamsbase from util to common, because util library should not depend on the common library and chainparamsbase uses the ArgsManager class in common. --- src/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Makefile.am b/src/Makefile.am index b749651b726d2..1fd0cdd7a80cc 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -669,6 +669,7 @@ libbitcoin_common_a_SOURCES = \ addresstype.cpp \ base58.cpp \ bech32.cpp \ + chainparamsbase.cpp \ chainparams.cpp \ coins.cpp \ common/args.cpp \ @@ -719,7 +720,6 @@ libbitcoin_util_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) libbitcoin_util_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) libbitcoin_util_a_SOURCES = \ support/lockedpool.cpp \ - chainparamsbase.cpp \ clientversion.cpp \ logging.cpp \ random.cpp \ From cc5f29fbea15d33e4d1aa95591253c6b86953fe7 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 10 May 2024 15:45:07 -0400 Subject: [PATCH 08/55] build: move memory_cleanse from util to crypto Move memory_cleanse from util to crypto because the crypto library should not depend on other libraries, and it calls memory_cleanse. --- src/Makefile.am | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 1fd0cdd7a80cc..bb61f2d0276b0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -587,7 +587,8 @@ crypto_libbitcoin_crypto_base_la_SOURCES = \ crypto/sha512.cpp \ crypto/sha512.h \ crypto/siphash.cpp \ - crypto/siphash.h + crypto/siphash.h \ + support/cleanse.cpp # See explanation for -static in crypto_libbitcoin_crypto_base_la's LDFLAGS and # CXXFLAGS above @@ -725,7 +726,6 @@ libbitcoin_util_a_SOURCES = \ random.cpp \ randomenv.cpp \ streams.cpp \ - support/cleanse.cpp \ sync.cpp \ util/asmap.cpp \ util/batchpriority.cpp \ @@ -968,7 +968,6 @@ libbitcoinkernel_la_SOURCES = \ script/solver.cpp \ signet.cpp \ streams.cpp \ - support/cleanse.cpp \ support/lockedpool.cpp \ sync.cpp \ txdb.cpp \ From 6861f954f8ff42c87ad638037adae86a5bd89600 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 10 May 2024 15:45:07 -0400 Subject: [PATCH 09/55] util: move util/message to common/signmessage Move util/message to common/signmessage so it is named more clearly, and because the util library is not supposed to depend on other libraries besides the crypto library. The signmessage functions use CKey, CPubKey, PKHash, and DecodeDestination functions in the consensus and common libraries. --- src/Makefile.am | 4 ++-- src/{util/message.cpp => common/signmessage.cpp} | 2 +- src/{util/message.h => common/signmessage.h} | 6 +++--- src/interfaces/wallet.h | 2 +- src/qt/signverifymessagedialog.cpp | 2 +- src/rpc/signmessage.cpp | 2 +- src/test/fuzz/message.cpp | 2 +- src/test/util_tests.cpp | 2 +- src/wallet/rpc/signmessage.cpp | 2 +- src/wallet/scriptpubkeyman.h | 2 +- src/wallet/wallet.cpp | 2 +- 11 files changed, 14 insertions(+), 14 deletions(-) rename src/{util/message.cpp => common/signmessage.cpp} (98%) rename src/{util/message.h => common/signmessage.h} (95%) diff --git a/src/Makefile.am b/src/Makefile.am index bb61f2d0276b0..54000a7eb19d6 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -144,6 +144,7 @@ BITCOIN_CORE_H = \ compat/cpuid.h \ compat/endian.h \ common/settings.h \ + common/signmessage.h \ common/system.h \ compressor.h \ consensus/consensus.h \ @@ -308,7 +309,6 @@ BITCOIN_CORE_H = \ util/hasher.h \ util/insert.h \ util/macros.h \ - util/message.h \ util/moneystr.h \ util/overflow.h \ util/overloaded.h \ @@ -680,6 +680,7 @@ libbitcoin_common_a_SOURCES = \ common/interfaces.cpp \ common/run_command.cpp \ common/settings.cpp \ + common/signmessage.cpp \ common/system.cpp \ common/url.cpp \ compressor.cpp \ @@ -742,7 +743,6 @@ libbitcoin_util_a_SOURCES = \ util/hasher.cpp \ util/sock.cpp \ util/syserror.cpp \ - util/message.cpp \ util/moneystr.cpp \ util/rbf.cpp \ util/readwritefile.cpp \ diff --git a/src/util/message.cpp b/src/common/signmessage.cpp similarity index 98% rename from src/util/message.cpp rename to src/common/signmessage.cpp index 1afb28cc10dc5..1612751e44dd6 100644 --- a/src/util/message.cpp +++ b/src/common/signmessage.cpp @@ -3,12 +3,12 @@ // 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 diff --git a/src/util/message.h b/src/common/signmessage.h similarity index 95% rename from src/util/message.h rename to src/common/signmessage.h index d0e2422574b04..215b563bbb215 100644 --- a/src/util/message.h +++ b/src/common/signmessage.h @@ -3,8 +3,8 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#ifndef BITCOIN_UTIL_MESSAGE_H -#define BITCOIN_UTIL_MESSAGE_H +#ifndef BITCOIN_COMMON_SIGNMESSAGE_H +#define BITCOIN_COMMON_SIGNMESSAGE_H #include @@ -74,4 +74,4 @@ uint256 MessageHash(const std::string& message); std::string SigningResultString(const SigningResult res); -#endif // BITCOIN_UTIL_MESSAGE_H +#endif // BITCOIN_COMMON_SIGNMESSAGE_H diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index c41f35829d7e7..f1bfffa7d465f 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -6,13 +6,13 @@ #define BITCOIN_INTERFACES_WALLET_H #include +#include #include #include #include #include