diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 740d31ae56..df1d12e86c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -76,7 +76,7 @@ jobs: macos-native-arm64: name: ${{ matrix.job-name }} - # Use latest image, but hardcode version to avoid silent upgrades (and breaks). + # Use any image to support the xcode-select below, but hardcode version to avoid silent upgrades (and breaks). # See: https://github.com/actions/runner-images#available-images. runs-on: macos-14 @@ -111,6 +111,10 @@ jobs: - name: Clang version run: | + # Use the earliest Xcode supported by the version of macOS denoted in + # doc/release-notes-empty-template.md and providing at least the + # minimum clang version denoted in doc/dependencies.md. + # See: https://developer.apple.com/documentation/xcode-release-notes/xcode-15-release-notes sudo xcode-select --switch /Applications/Xcode_15.0.app clang --version diff --git a/CMakeLists.txt b/CMakeLists.txt index 2dba6f255d..e323686601 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,7 +22,7 @@ set(CLIENT_VERSION_MINOR 99) set(CLIENT_VERSION_BUILD 0) set(CLIENT_VERSION_RC 0) set(CLIENT_VERSION_IS_RELEASE "false") -set(COPYRIGHT_YEAR "2024") +set(COPYRIGHT_YEAR "2025") # During the enabling of the CXX and CXXOBJ languages, we modify # CMake's compiler/linker invocation strings by appending the content diff --git a/COPYING b/COPYING index e6d6e9fe57..23dc5e905b 100644 --- a/COPYING +++ b/COPYING @@ -1,7 +1,7 @@ The MIT License (MIT) -Copyright (c) 2009-2024 The Bitcoin Core developers -Copyright (c) 2009-2024 Bitcoin Developers +Copyright (c) 2009-2025 The Bitcoin Core developers +Copyright (c) 2009-2025 Bitcoin Developers Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal diff --git a/ci/test/00_setup_env_native_msan.sh b/ci/test/00_setup_env_native_msan.sh index c6b3d68be6..cd4ac99942 100755 --- a/ci/test/00_setup_env_native_msan.sh +++ b/ci/test/00_setup_env_native_msan.sh @@ -27,4 +27,3 @@ export BITCOIN_CONFIG="\ -DAPPEND_CPPFLAGS='-U_FORTIFY_SOURCE' \ " export USE_MEMORY_SANITIZER="true" -export RUN_FUNCTIONAL_TESTS="false" diff --git a/ci/test/01_base_install.sh b/ci/test/01_base_install.sh index e026b919e3..a46f9ffabb 100755 --- a/ci/test/01_base_install.sh +++ b/ci/test/01_base_install.sh @@ -49,7 +49,7 @@ if [ -n "$PIP_PACKAGES" ]; then fi if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then - ${CI_RETRY_EXE} git clone --depth=1 https://github.com/llvm/llvm-project -b "llvmorg-19.1.0" /msan/llvm-project + ${CI_RETRY_EXE} git clone --depth=1 https://github.com/llvm/llvm-project -b "llvmorg-19.1.6" /msan/llvm-project cmake -G Ninja -B /msan/clang_build/ \ -DLLVM_ENABLE_PROJECTS="clang" \ @@ -74,6 +74,7 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then -DLLVM_TARGETS_TO_BUILD=Native \ -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF \ -DLIBCXXABI_USE_LLVM_UNWINDER=OFF \ + -DLIBCXX_ABI_DEFINES="_LIBCPP_ABI_BOUNDED_ITERATORS;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STD_ARRAY;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR;_LIBCPP_ABI_BOUNDED_UNIQUE_PTR" \ -DLIBCXX_HARDENING_MODE=debug \ -S /msan/llvm-project/runtimes diff --git a/contrib/debian/copyright b/contrib/debian/copyright index dafc92f8ad..af8a1bc0c1 100644 --- a/contrib/debian/copyright +++ b/contrib/debian/copyright @@ -5,7 +5,7 @@ Upstream-Contact: Satoshi Nakamoto Source: https://github.com/bitcoin/bitcoin Files: * -Copyright: 2009-2024, Bitcoin Core Developers +Copyright: 2009-2025, Bitcoin Core Developers License: Expat Comment: The Bitcoin Core Developers encompasses all contributors to the project, listed in the release notes or the git log. diff --git a/contrib/tracing/log_raw_p2p_msgs.py b/contrib/tracing/log_raw_p2p_msgs.py index 9cda7fd08d..094888c13e 100755 --- a/contrib/tracing/log_raw_p2p_msgs.py +++ b/contrib/tracing/log_raw_p2p_msgs.py @@ -41,7 +41,8 @@ program = """ #include -#define MIN(a,b) ({ __typeof__ (a) _a = (a); __typeof__ (b) _b = (b); _a < _b ? _a : _b; }) +// A min() macro. Prefixed with _TRACEPOINT_TEST to avoid collision with other MIN macros. +#define _TRACEPOINT_TEST_MIN(a,b) ({ __typeof__ (a) _a = (a); __typeof__ (b) _b = (b); _a < _b ? _a : _b; }) // Maximum possible allocation size // from include/linux/percpu.h in the Linux kernel @@ -88,7 +89,7 @@ bpf_usdt_readarg_p(3, ctx, &msg->peer_conn_type, MAX_PEER_CONN_TYPE_LENGTH); bpf_usdt_readarg_p(4, ctx, &msg->msg_type, MAX_MSG_TYPE_LENGTH); bpf_usdt_readarg(5, ctx, &msg->msg_size); - bpf_usdt_readarg_p(6, ctx, &msg->msg, MIN(msg->msg_size, MAX_MSG_DATA_LENGTH)); + bpf_usdt_readarg_p(6, ctx, &msg->msg, _TRACEPOINT_TEST_MIN(msg->msg_size, MAX_MSG_DATA_LENGTH)); inbound_messages.perf_submit(ctx, msg, sizeof(*msg)); return 0; @@ -108,7 +109,7 @@ bpf_usdt_readarg_p(3, ctx, &msg->peer_conn_type, MAX_PEER_CONN_TYPE_LENGTH); bpf_usdt_readarg_p(4, ctx, &msg->msg_type, MAX_MSG_TYPE_LENGTH); bpf_usdt_readarg(5, ctx, &msg->msg_size); - bpf_usdt_readarg_p(6, ctx, &msg->msg, MIN(msg->msg_size, MAX_MSG_DATA_LENGTH)); + bpf_usdt_readarg_p(6, ctx, &msg->msg, _TRACEPOINT_TEST_MIN(msg->msg_size, MAX_MSG_DATA_LENGTH)); outbound_messages.perf_submit(ctx, msg, sizeof(*msg)); return 0; diff --git a/depends/Makefile b/depends/Makefile index a2a9f6823e..9ba8213a92 100644 --- a/depends/Makefile +++ b/depends/Makefile @@ -141,7 +141,7 @@ include packages/packages.mk # 2. Before including packages/*.mk (excluding packages/packages.mk), since # they rely on the build_id variables # -build_id:=$(shell env CC='$(build_CC)' C_STANDARD='$(C_STANDARD)' CXX='$(build_CXX)' CXX_STANDARD='$(CXX_STANDARD)' AR='$(build_AR) 'NM='$(build_NM)' RANLIB='$(build_RANLIB)' STRIP='$(build_STRIP)' SHA256SUM='$(build_SHA256SUM)' DEBUG='$(DEBUG)' LTO='$(LTO)' NO_HARDEN='$(NO_HARDEN)' ./gen_id '$(BUILD_ID_SALT)' 'GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))') +build_id:=$(shell env CC='$(build_CC)' C_STANDARD='$(C_STANDARD)' CXX='$(build_CXX)' CXX_STANDARD='$(CXX_STANDARD)' AR='$(build_AR)' NM='$(build_NM)' RANLIB='$(build_RANLIB)' STRIP='$(build_STRIP)' SHA256SUM='$(build_SHA256SUM)' DEBUG='$(DEBUG)' LTO='$(LTO)' NO_HARDEN='$(NO_HARDEN)' ./gen_id '$(BUILD_ID_SALT)' 'GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))') $(host_arch)_$(host_os)_id:=$(shell env CC='$(host_CC)' C_STANDARD='$(C_STANDARD)' CXX='$(host_CXX)' CXX_STANDARD='$(CXX_STANDARD)' AR='$(host_AR)' NM='$(host_NM)' RANLIB='$(host_RANLIB)' STRIP='$(host_STRIP)' SHA256SUM='$(build_SHA256SUM)' DEBUG='$(DEBUG)' LTO='$(LTO)' NO_HARDEN='$(NO_HARDEN)' ./gen_id '$(HOST_ID_SALT)' 'GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))') boost_packages_$(NO_BOOST) = $(boost_packages) diff --git a/depends/packages/native_capnp.mk b/depends/packages/native_capnp.mk index d6e6b4cadb..e67b103716 100644 --- a/depends/packages/native_capnp.mk +++ b/depends/packages/native_capnp.mk @@ -1,9 +1,9 @@ package=native_capnp -$(package)_version=1.0.2 +$(package)_version=1.1.0 $(package)_download_path=https://capnproto.org/ $(package)_download_file=capnproto-c++-$($(package)_version).tar.gz $(package)_file_name=capnproto-cxx-$($(package)_version).tar.gz -$(package)_sha256_hash=9057dbc0223366b74bbeca33a05de164a229b0377927f1b7ef3828cdd8cb1d7e +$(package)_sha256_hash=07167580e563f5e821e3b2af1c238c16ec7181612650c5901330fa9a0da50939 define $(package)_set_vars $(package)_config_opts := -DBUILD_TESTING=OFF diff --git a/doc/build-freebsd.md b/doc/build-freebsd.md index 2456abd57b..694224621e 100644 --- a/doc/build-freebsd.md +++ b/doc/build-freebsd.md @@ -96,7 +96,7 @@ There is an included test suite that is useful for testing code changes when dev To run the test suite (recommended), you will need to have Python 3 installed: ```bash -pkg install python3 databases/py-sqlite3 +pkg install python3 databases/py-sqlite3 net/py-pyzmq ``` --- diff --git a/doc/build-netbsd.md b/doc/build-netbsd.md index 63bfbd61db..988f3b93a7 100644 --- a/doc/build-netbsd.md +++ b/doc/build-netbsd.md @@ -1,6 +1,6 @@ # NetBSD Build Guide -**Updated for NetBSD [10.0](https://netbsd.org/releases/formal-10/NetBSD-10.0.html)** +**Updated for NetBSD [10.1](https://netbsd.org/releases/formal-10/NetBSD-10.1.html)** This guide describes how to build bitcoind, command-line utilities, and GUI on NetBSD. @@ -83,6 +83,13 @@ pkgin install qrencode Otherwise, if you don't need QR encoding support, use the `-DWITH_QRENCODE=OFF` option to disable this feature in order to compile the GUI. +#### Notifications +###### ZeroMQ + +Bitcoin Core can provide notifications via ZeroMQ. If the package is installed, support will be compiled in. +```bash +pkgin zeromq +``` #### Test Suite Dependencies @@ -90,10 +97,10 @@ There is an included test suite that is useful for testing code changes when dev To run the test suite (recommended), you will need to have Python 3 installed: ```bash -pkgin install python39 +pkgin install python310 py310-zmq ``` -### Building Bitcoin Core +## Building Bitcoin Core ### 1. Configuration diff --git a/doc/release-notes-empty-template.md b/doc/release-notes-empty-template.md index bd4600b395..5853fc60f3 100644 --- a/doc/release-notes-empty-template.md +++ b/doc/release-notes-empty-template.md @@ -43,7 +43,7 @@ Compatibility ============== Bitcoin Core is supported and tested on operating systems using the -Linux Kernel 3.17+, macOS 13.0+, and Windows 10 and newer. Bitcoin +Linux Kernel 3.17+, macOS 13+, and Windows 10+. Bitcoin Core should also work on most other Unix-like systems but is not as frequently tested on them. It is not recommended to use Bitcoin Core on unsupported systems. diff --git a/src/arith_uint256.h b/src/arith_uint256.h index 38b7453034..60b371f6d3 100644 --- a/src/arith_uint256.h +++ b/src/arith_uint256.h @@ -26,6 +26,7 @@ class base_uint protected: static_assert(BITS / 32 > 0 && BITS % 32 == 0, "Template parameter BITS must be a positive multiple of 32."); static constexpr int WIDTH = BITS / 32; + /** Big integer represented with 32-bit digits, least-significant first. */ uint32_t pn[WIDTH]; public: diff --git a/src/init.cpp b/src/init.cpp index b5adcf6476..cb4ff733a5 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1245,7 +1245,6 @@ static ChainstateLoadResult InitAndLoadChainstate( "", CClientUIInterface::MSG_ERROR); }; uiInterface.InitMessage(_("Loading block index…").translated); - const auto load_block_index_start_time{SteadyClock::now()}; auto catch_exceptions = [](auto&& f) { try { return f(); @@ -1263,7 +1262,7 @@ static ChainstateLoadResult InitAndLoadChainstate( } std::tie(status, error) = catch_exceptions([&] { return VerifyLoadedChainstate(chainman, options); }); if (status == node::ChainstateLoadStatus::SUCCESS) { - LogPrintf(" block index %15dms\n", Ticks(SteadyClock::now() - load_block_index_start_time)); + LogInfo("Block index and chainstate loaded"); } } return {status, error}; diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 8954f0ebc5..1ea976b479 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -425,6 +425,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda } ++nPackagesSelected; + pblocktemplate->m_package_feerates.emplace_back(packageFees, static_cast(packageSize)); // Update transactions that depend on each of these nDescendantsUpdated += UpdatePackagesForAdded(mempool, ancestors, mapModifiedTx); diff --git a/src/node/miner.h b/src/node/miner.h index f6461a8d55..3c4c66b0ba 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -39,6 +40,9 @@ struct CBlockTemplate std::vector vTxFees; std::vector vTxSigOpsCost; std::vector vchCoinbaseCommitment; + /* A vector of package fee rates, ordered by the sequence in which + * packages are selected for inclusion in the block template.*/ + std::vector m_package_feerates; }; // Container for tracking updates to ancestor feerate as we include (parent) diff --git a/src/policy/packages.h b/src/policy/packages.h index 3050320122..4b2350edac 100644 --- a/src/policy/packages.h +++ b/src/policy/packages.h @@ -89,8 +89,9 @@ bool IsChildWithParents(const Package& package); */ bool IsChildWithParentsTree(const Package& package); -/** Get the hash of these transactions' wtxids, concatenated in lexicographical order (treating the - * wtxids as little endian encoded uint256, smallest to largest). */ +/** Get the hash of the concatenated wtxids of transactions, with wtxids + * treated as a little-endian numbers and sorted in ascending numeric order. + */ uint256 GetPackageHash(const std::vector& transactions); #endif // BITCOIN_POLICY_PACKAGES_H diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index ad946888d4..e3a2d2c81d 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -636,8 +636,8 @@ static RPCHelpMan getblocktemplate() {RPCResult::Type::OBJ, "", "", { {RPCResult::Type::STR_HEX, "data", "transaction data encoded in hexadecimal (byte-for-byte)"}, - {RPCResult::Type::STR_HEX, "txid", "transaction id encoded in little-endian hexadecimal"}, - {RPCResult::Type::STR_HEX, "hash", "hash encoded in little-endian hexadecimal (including witness data)"}, + {RPCResult::Type::STR_HEX, "txid", "transaction hash excluding witness data, shown in byte-reversed hex"}, + {RPCResult::Type::STR_HEX, "hash", "transaction hash including witness data, shown in byte-reversed hex"}, {RPCResult::Type::ARR, "depends", "array of numbers", { {RPCResult::Type::NUM, "", "transactions before this one (by 1-based index in 'transactions' list) that must be present in the final block if this one is"}, diff --git a/src/support/allocators/secure.h b/src/support/allocators/secure.h index 4395567722..e7ffc9e201 100644 --- a/src/support/allocators/secure.h +++ b/src/support/allocators/secure.h @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2021 The Bitcoin Core developers +// Copyright (c) 2009-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. @@ -74,7 +74,7 @@ secure_unique_ptr make_secure_unique(Args&&... as) // initialize in place, and return as secure_unique_ptr try { - return secure_unique_ptr(new (p) T(std::forward(as)...)); + return secure_unique_ptr(new (p) T(std::forward(as)...)); } catch (...) { secure_allocator().deallocate(p, 1); throw; diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp index e4e4723c74..5b1d0cda7a 100644 --- a/src/test/fuzz/fuzz.cpp +++ b/src/test/fuzz/fuzz.cpp @@ -116,6 +116,9 @@ void initialize() // - Creating a BasicTestingSetup or derived class will switch to a random seed. SeedRandomStateForTest(SeedRand::ZEROS); + // Set time to the genesis block timestamp for deterministic initialization. + SetMockTime(1231006505); + // Terminate immediately if a fuzzing harness ever tries to create a socket. // Individual tests can override this by pointing CreateSock to a mocked alternative. CreateSock = [](int, int, int) -> std::unique_ptr { std::terminate(); }; diff --git a/src/test/fuzz/load_external_block_file.cpp b/src/test/fuzz/load_external_block_file.cpp index 6460261f0f..b5293dd11a 100644 --- a/src/test/fuzz/load_external_block_file.cpp +++ b/src/test/fuzz/load_external_block_file.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -27,6 +28,7 @@ void initialize_load_external_block_file() FUZZ_TARGET(load_external_block_file, .init = initialize_load_external_block_file) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); FuzzedFileProvider fuzzed_file_provider{fuzzed_data_provider}; AutoFile fuzzed_block_file{fuzzed_file_provider.open()}; if (fuzzed_block_file.IsNull()) { diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp index a5bccd103d..817bc0f18e 100644 --- a/src/test/fuzz/mini_miner.cpp +++ b/src/test/fuzz/mini_miner.cpp @@ -1,3 +1,7 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + #include #include #include @@ -13,6 +17,7 @@ #include #include #include +#include #include #include @@ -36,6 +41,7 @@ FUZZ_TARGET(mini_miner, .init = initialize_miner) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); bilingual_str error; CTxMemPool pool{CTxMemPool::Options{}, error}; Assert(error.empty()); @@ -115,6 +121,7 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); bilingual_str error; CTxMemPool pool{CTxMemPool::Options{}, error}; Assert(error.empty()); diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 4cec96274e..1a0de7aa36 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -79,6 +80,7 @@ FUZZ_TARGET(net, .init = initialize_net) FUZZ_TARGET(local_address, .init = initialize_net) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + SetMockTime(ConsumeTime(fuzzed_data_provider)); CService service{ConsumeService(fuzzed_data_provider)}; CNode node{ConsumeNode(fuzzed_data_provider)}; { diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp index 873eb2b1cc..ed7041ad1f 100644 --- a/src/test/fuzz/p2p_headers_presync.cpp +++ b/src/test/fuzz/p2p_headers_presync.cpp @@ -154,14 +154,15 @@ void initialize() FUZZ_TARGET(p2p_headers_presync, .init = initialize) { SeedRandomStateForTest(SeedRand::ZEROS); + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); + ChainstateManager& chainman = *g_testing_setup->m_node.chainman; LOCK(NetEventsInterface::g_msgproc_mutex); g_testing_setup->ResetAndInitialize(); - FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; - CBlockHeader base{chainman.GetParams().GenesisBlock()}; SetMockTime(base.nTime); diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp index 8a42807be8..82d781cd53 100644 --- a/src/test/fuzz/partially_downloaded_block.cpp +++ b/src/test/fuzz/partially_downloaded_block.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -46,6 +47,7 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); auto block{ConsumeDeserializable(fuzzed_data_provider, TX_WITH_WITNESS)}; if (!block || block->vtx.size() == 0 || diff --git a/src/test/fuzz/socks5.cpp b/src/test/fuzz/socks5.cpp index 17d1787586..2e6e2a94ac 100644 --- a/src/test/fuzz/socks5.cpp +++ b/src/test/fuzz/socks5.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -29,6 +30,7 @@ void initialize_socks5() FUZZ_TARGET(socks5, .init = initialize_socks5) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); ProxyCredentials proxy_credentials; proxy_credentials.username = fuzzed_data_provider.ConsumeRandomLengthString(512); proxy_credentials.password = fuzzed_data_provider.ConsumeRandomLengthString(512); diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp index 4917e8b405..bb1331c37c 100644 --- a/src/test/fuzz/txdownloadman.cpp +++ b/src/test/fuzz/txdownloadman.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -167,6 +168,7 @@ FUZZ_TARGET(txdownloadman, .init = initialize) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + SetMockTime(ConsumeTime(fuzzed_data_provider)); // Initialize txdownloadman bilingual_str error; @@ -297,6 +299,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + SetMockTime(ConsumeTime(fuzzed_data_provider)); // Initialize a TxDownloadManagerImpl bilingual_str error; diff --git a/src/test/fuzz/util/check_globals.cpp b/src/test/fuzz/util/check_globals.cpp index fbc5a55598..f91a965afc 100644 --- a/src/test/fuzz/util/check_globals.cpp +++ b/src/test/fuzz/util/check_globals.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -16,6 +17,8 @@ struct CheckGlobalsImpl { { g_used_g_prng = false; g_seeded_g_prng_zero = false; + g_used_system_time = false; + SetMockTime(0s); } ~CheckGlobalsImpl() { @@ -34,6 +37,19 @@ struct CheckGlobalsImpl { << std::endl; std::abort(); // Abort, because AFL may try to recover from a std::exit } + + if (g_used_system_time) { + std::cerr << "\n\n" + "The current fuzz target accessed system time.\n\n" + + "This is acceptable, but requires the fuzz target to call \n" + "SetMockTime() at the beginning of processing the fuzz input.\n\n" + + "Without setting mock time, time-dependent behavior can lead \n" + "to non-reproducible bugs or inefficient fuzzing.\n\n" + << std::endl; + std::abort(); + } } }; diff --git a/src/test/fuzz/util/check_globals.h b/src/test/fuzz/util/check_globals.h index 79f247535a..12d39c2daf 100644 --- a/src/test/fuzz/util/check_globals.h +++ b/src/test/fuzz/util/check_globals.h @@ -5,10 +5,13 @@ #ifndef BITCOIN_TEST_FUZZ_UTIL_CHECK_GLOBALS_H #define BITCOIN_TEST_FUZZ_UTIL_CHECK_GLOBALS_H +#include #include #include #include +extern std::atomic g_used_system_time; + struct CheckGlobalsImpl; struct CheckGlobals { CheckGlobals(); diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp index 9fe922cfaf..5eb6a32320 100644 --- a/src/test/fuzz/utxo_snapshot.cpp +++ b/src/test/fuzz/utxo_snapshot.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -72,6 +73,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + SetMockTime(ConsumeTime(fuzzed_data_provider, /*min=*/1296688602)); // regtest genesis block timestamp auto& setup{*g_setup}; bool dirty_chainman{false}; // Re-use the global chainman, but reset it when it is dirty auto& chainman{*setup.m_node.chainman}; diff --git a/src/test/fuzz/utxo_total_supply.cpp b/src/test/fuzz/utxo_total_supply.cpp index 84d82f71e2..e574c5b85c 100644 --- a/src/test/fuzz/utxo_total_supply.cpp +++ b/src/test/fuzz/utxo_total_supply.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include using node::BlockAssembler; @@ -22,18 +23,22 @@ using node::BlockAssembler; FUZZ_TARGET(utxo_total_supply) { SeedRandomStateForTest(SeedRand::ZEROS); + FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + const auto mock_time{ConsumeTime(fuzzed_data_provider, /*min=*/1296688602)}; // regtest genesis block timestamp /** The testing setup that creates a chainman only (no chainstate) */ ChainTestingSetup test_setup{ ChainType::REGTEST, { - .extra_args = {"-testactivationheight=bip34@2"}, + .extra_args = { + "-testactivationheight=bip34@2", + strprintf("-mocktime=%d", mock_time).c_str() + }, }, }; // Create chainstate test_setup.LoadVerifyActivateChainstate(); auto& node{test_setup.m_node}; auto& chainman{*Assert(test_setup.m_node.chainman)}; - FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); const auto ActiveHeight = [&]() { LOCK(chainman.GetMutex()); diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 8a264f3fd3..d7bc790d0e 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -15,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -24,12 +26,14 @@ #include #include +#include #include using namespace util::hex_literals; +using interfaces::BlockTemplate; +using interfaces::Mining; using node::BlockAssembler; -using node::CBlockTemplate; namespace miner_tests { struct MinerTestingSetup : public TestingSetup { @@ -54,7 +58,10 @@ struct MinerTestingSetup : public TestingSetup { Assert(error.empty()); return *m_node.mempool; } - BlockAssembler AssemblerForTest(CTxMemPool& tx_mempool, BlockAssembler::Options options); + std::unique_ptr MakeMining() + { + return interfaces::MakeMining(m_node); + } }; } // namespace miner_tests @@ -62,13 +69,6 @@ BOOST_FIXTURE_TEST_SUITE(miner_tests, MinerTestingSetup) static CFeeRate blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE); -BlockAssembler MinerTestingSetup::AssemblerForTest(CTxMemPool& tx_mempool, BlockAssembler::Options options) -{ - options.nBlockMaxWeight = MAX_BLOCK_WEIGHT; - options.blockMinFeeRate = blockMinFeeRate; - return BlockAssembler{m_node.chainman->ActiveChainstate(), &tx_mempool, options}; -} - constexpr static struct { unsigned char extranonce; unsigned int nonce; @@ -106,6 +106,10 @@ static std::unique_ptr CreateBlockIndex(int nHeight, CBlockIndex* a void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const std::vector& txFirst) { CTxMemPool& tx_mempool{MakeMempool()}; + auto mining{MakeMining()}; + BlockAssembler::Options options; + options.coinbase_output_script = scriptPubKey; + LOCK(tx_mempool.cs); // Test the ancestor feerate transaction selection. TestMemPoolEntryHelper entry; @@ -121,27 +125,45 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout[0].nValue = 5000000000LL - 1000; // This tx has a low fee: 1000 satoshis Txid hashParentTx = tx.GetHash(); // save this txid for later use - AddToMempool(tx_mempool, entry.Fee(1000).Time(Now()).SpendsCoinbase(true).FromTx(tx)); + const auto parent_tx{entry.Fee(1000).Time(Now()).SpendsCoinbase(true).FromTx(tx)}; + AddToMempool(tx_mempool, parent_tx); // This tx has a medium fee: 10000 satoshis tx.vin[0].prevout.hash = txFirst[1]->GetHash(); tx.vout[0].nValue = 5000000000LL - 10000; Txid hashMediumFeeTx = tx.GetHash(); - AddToMempool(tx_mempool, entry.Fee(10000).Time(Now()).SpendsCoinbase(true).FromTx(tx)); + const auto medium_fee_tx{entry.Fee(10000).Time(Now()).SpendsCoinbase(true).FromTx(tx)}; + AddToMempool(tx_mempool, medium_fee_tx); // This tx has a high fee, but depends on the first transaction tx.vin[0].prevout.hash = hashParentTx; tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 50k satoshi fee Txid hashHighFeeTx = tx.GetHash(); - AddToMempool(tx_mempool, entry.Fee(50000).Time(Now()).SpendsCoinbase(false).FromTx(tx)); - - BlockAssembler::Options options; - options.coinbase_output_script = scriptPubKey; - std::unique_ptr pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 4U); - BOOST_CHECK(pblocktemplate->block.vtx[1]->GetHash() == hashParentTx); - BOOST_CHECK(pblocktemplate->block.vtx[2]->GetHash() == hashHighFeeTx); - BOOST_CHECK(pblocktemplate->block.vtx[3]->GetHash() == hashMediumFeeTx); + const auto high_fee_tx{entry.Fee(50000).Time(Now()).SpendsCoinbase(false).FromTx(tx)}; + AddToMempool(tx_mempool, high_fee_tx); + + std::unique_ptr block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + CBlock block{block_template->getBlock()}; + BOOST_REQUIRE_EQUAL(block.vtx.size(), 4U); + BOOST_CHECK(block.vtx[1]->GetHash() == hashParentTx); + BOOST_CHECK(block.vtx[2]->GetHash() == hashHighFeeTx); + BOOST_CHECK(block.vtx[3]->GetHash() == hashMediumFeeTx); + + // Test the inclusion of package feerates in the block template and ensure they are sequential. + const auto block_package_feerates = BlockAssembler{m_node.chainman->ActiveChainstate(), &tx_mempool, options}.CreateNewBlock()->m_package_feerates; + BOOST_CHECK(block_package_feerates.size() == 2); + + // parent_tx and high_fee_tx are added to the block as a package. + const auto combined_txs_fee = parent_tx.GetFee() + high_fee_tx.GetFee(); + const auto combined_txs_size = parent_tx.GetTxSize() + high_fee_tx.GetTxSize(); + FeeFrac package_feefrac{combined_txs_fee, combined_txs_size}; + // The package should be added first. + BOOST_CHECK(block_package_feerates[0] == package_feefrac); + + // The medium_fee_tx should be added next. + FeeFrac medium_tx_feefrac{medium_fee_tx.GetFee(), medium_fee_tx.GetTxSize()}; + BOOST_CHECK(block_package_feerates[1] == medium_tx_feefrac); // Test that a package below the block min tx fee doesn't get included tx.vin[0].prevout.hash = hashHighFeeTx; @@ -158,11 +180,13 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout[0].nValue = 5000000000LL - 1000 - 50000 - feeToUse; Txid hashLowFeeTx = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(feeToUse).FromTx(tx)); - pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); + block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); // Verify that the free tx and the low fee tx didn't get selected - for (size_t i=0; iblock.vtx.size(); ++i) { - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashFreeTx); - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashLowFeeTx); + for (size_t i=0; iGetHash() != hashFreeTx); + BOOST_CHECK(block.vtx[i]->GetHash() != hashLowFeeTx); } // Test that packages above the min relay fee do get included, even if one @@ -172,10 +196,12 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout[0].nValue -= 2; // Now we should be just over the min relay fee hashLowFeeTx = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(feeToUse + 2).FromTx(tx)); - pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 6U); - BOOST_CHECK(pblocktemplate->block.vtx[4]->GetHash() == hashFreeTx); - BOOST_CHECK(pblocktemplate->block.vtx[5]->GetHash() == hashLowFeeTx); + block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); + BOOST_REQUIRE_EQUAL(block.vtx.size(), 6U); + BOOST_CHECK(block.vtx[4]->GetHash() == hashFreeTx); + BOOST_CHECK(block.vtx[5]->GetHash() == hashLowFeeTx); // Test that transaction selection properly updates ancestor fee // calculations as ancestor transactions get included in a block. @@ -194,12 +220,14 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout[0].nValue = 5000000000LL - 100000000 - feeToUse; Txid hashLowFeeTx2 = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(feeToUse).SpendsCoinbase(false).FromTx(tx)); - pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); + block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); // Verify that this tx isn't selected. - for (size_t i=0; iblock.vtx.size(); ++i) { - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashFreeTx2); - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashLowFeeTx2); + for (size_t i=0; iGetHash() != hashFreeTx2); + BOOST_CHECK(block.vtx[i]->GetHash() != hashLowFeeTx2); } // This tx will be mineable, and should cause hashLowFeeTx2 to be selected @@ -207,9 +235,11 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vin[0].prevout.n = 1; tx.vout[0].nValue = 100000000 - 10000; // 10k satoshi fee AddToMempool(tx_mempool, entry.Fee(10000).FromTx(tx)); - pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 9U); - BOOST_CHECK(pblocktemplate->block.vtx[8]->GetHash() == hashLowFeeTx2); + block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); + BOOST_REQUIRE_EQUAL(block.vtx.size(), 9U); + BOOST_CHECK(block.vtx[8]->GetHash() == hashLowFeeTx2); } void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::vector& txFirst, int baseheight) @@ -225,6 +255,9 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: const CAmount HIGHFEE = COIN; const CAmount HIGHERFEE = 4 * COIN; + auto mining{MakeMining()}; + BOOST_REQUIRE(mining); + BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; @@ -233,8 +266,9 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: LOCK(tx_mempool.cs); // Just to make sure we can still make simple blocks - auto pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_CHECK(pblocktemplate); + auto block_template{mining->createNewBlock(options)}; + BOOST_REQUIRE(block_template); + CBlock block{block_template->getBlock()}; // block sigops > limit: 1000 CHECKMULTISIG + 1 tx.vin.resize(1); @@ -253,7 +287,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vin[0].prevout.hash = hash; } - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("bad-blk-sigops")); + BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("bad-blk-sigops")); } { @@ -270,7 +304,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(spendsCoinbase).SigOpsCost(80).FromTx(tx)); tx.vin[0].prevout.hash = hash; } - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_REQUIRE(mining->createNewBlock(options)); } { @@ -294,7 +328,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); tx.vin[0].prevout.hash = hash; } - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_REQUIRE(mining->createNewBlock(options)); } { @@ -304,7 +338,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: // orphan in tx_mempool, template creation fails hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).FromTx(tx)); - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("bad-txns-inputs-missingorspent")); + BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("bad-txns-inputs-missingorspent")); } { @@ -325,7 +359,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vout[0].nValue = tx.vout[0].nValue + BLOCKSUBSIDY - HIGHERFEE; // First txn output + fresh coinbase - new txn fee hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(HIGHERFEE).Time(Now()).SpendsCoinbase(true).FromTx(tx)); - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_REQUIRE(mining->createNewBlock(options)); } { @@ -341,7 +375,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: // give it a fee so it'll get mined AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(false).FromTx(tx)); // Should throw bad-cb-multiple - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("bad-cb-multiple")); + BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("bad-cb-multiple")); } { @@ -358,7 +392,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vout[0].scriptPubKey = CScript() << OP_2; hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(HIGHFEE).Time(Now()).SpendsCoinbase(true).FromTx(tx)); - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("bad-txns-inputs-missingorspent")); + BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("bad-txns-inputs-missingorspent")); } { @@ -378,7 +412,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: next->BuildSkip(); m_node.chainman->ActiveChain().SetTip(*next); } - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_REQUIRE(mining->createNewBlock(options)); // Extend to a 210000-long block chain. while (m_node.chainman->ActiveChain().Tip()->nHeight < 210000) { CBlockIndex* prev = m_node.chainman->ActiveChain().Tip(); @@ -390,7 +424,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: next->BuildSkip(); m_node.chainman->ActiveChain().SetTip(*next); } - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_REQUIRE(mining->createNewBlock(options)); // invalid p2sh txn in tx_mempool, template creation fails tx.vin[0].prevout.hash = txFirst[0]->GetHash(); @@ -406,7 +440,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vout[0].nValue -= LOWFEE; hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(false).FromTx(tx)); - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("mandatory-script-verify-flag-failed")); + BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("mandatory-script-verify-flag-failed")); // Delete the dummy blocks again. while (m_node.chainman->ActiveChain().Tip()->nHeight > nHeight) { @@ -508,14 +542,15 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vin[0].nSequence = CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG | 1; BOOST_CHECK(!TestSequenceLocks(CTransaction{tx}, tx_mempool)); // Sequence locks fail - auto pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_CHECK(pblocktemplate); + auto block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); // None of the of the absolute height/time locked tx should have made // it into the template because we still check IsFinalTx in CreateNewBlock, // but relative locked txs will if inconsistently added to mempool. // For now these will still generate a valid template until BIP68 soft fork - BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3U); + CBlock block{block_template->getBlock()}; + BOOST_CHECK_EQUAL(block.vtx.size(), 3U); // However if we advance height by 1 and time by SEQUENCE_LOCK_TIME, all of them should be mined for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) { CBlockIndex* ancestor{Assert(m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i))}; @@ -524,12 +559,17 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: m_node.chainman->ActiveChain().Tip()->nHeight++; SetMockTime(m_node.chainman->ActiveChain().Tip()->GetMedianTimePast() + 1); - BOOST_CHECK(pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock()); - BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 5U); + block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); + BOOST_CHECK_EQUAL(block.vtx.size(), 5U); } void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const std::vector& txFirst) { + auto mining{MakeMining()}; + BOOST_REQUIRE(mining); + BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; @@ -594,34 +634,34 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const Txid hashFreeGrandchild = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(0).SpendsCoinbase(false).FromTx(tx)); - auto pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 6U); - BOOST_CHECK(pblocktemplate->block.vtx[1]->GetHash() == hashFreeParent); - BOOST_CHECK(pblocktemplate->block.vtx[2]->GetHash() == hashFreePrioritisedTx); - BOOST_CHECK(pblocktemplate->block.vtx[3]->GetHash() == hashParentTx); - BOOST_CHECK(pblocktemplate->block.vtx[4]->GetHash() == hashPrioritsedChild); - BOOST_CHECK(pblocktemplate->block.vtx[5]->GetHash() == hashFreeChild); - for (size_t i=0; iblock.vtx.size(); ++i) { + auto block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + CBlock block{block_template->getBlock()}; + BOOST_REQUIRE_EQUAL(block.vtx.size(), 6U); + BOOST_CHECK(block.vtx[1]->GetHash() == hashFreeParent); + BOOST_CHECK(block.vtx[2]->GetHash() == hashFreePrioritisedTx); + BOOST_CHECK(block.vtx[3]->GetHash() == hashParentTx); + BOOST_CHECK(block.vtx[4]->GetHash() == hashPrioritsedChild); + BOOST_CHECK(block.vtx[5]->GetHash() == hashFreeChild); + for (size_t i=0; iblock.vtx[i]->GetHash() != hashFreeGrandchild); + BOOST_CHECK(block.vtx[i]->GetHash() != hashFreeGrandchild); // De-prioritised transaction should not be included. - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashMediumFeeTx); + BOOST_CHECK(block.vtx[i]->GetHash() != hashMediumFeeTx); } } // NOTE: These tests rely on CreateNewBlock doing its own self-validation! BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) { + auto mining{MakeMining()}; + BOOST_REQUIRE(mining); + // Note that by default, these tests run with size accounting enabled. CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG; - std::unique_ptr pblocktemplate; - BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; - - CTxMemPool& tx_mempool{*m_node.mempool}; - // Simple block creation, nothing special yet: - BOOST_CHECK(pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock()); + std::unique_ptr block_template; // We can't make transactions until we have inputs // Therefore, load 110 blocks :) @@ -629,27 +669,48 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) int baseheight = 0; std::vector txFirst; for (const auto& bi : BLOCKINFO) { - CBlock *pblock = &pblocktemplate->block; // pointer for convenience + const int current_height{mining->getTip()->height}; + + // Simple block creation, nothing special yet: + block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + + CBlock block{block_template->getBlock()}; + CMutableTransaction txCoinbase(*block.vtx[0]); { LOCK(cs_main); - pblock->nVersion = VERSIONBITS_TOP_BITS; - pblock->nTime = m_node.chainman->ActiveChain().Tip()->GetMedianTimePast()+1; - CMutableTransaction txCoinbase(*pblock->vtx[0]); + block.nVersion = VERSIONBITS_TOP_BITS; + block.nTime = Assert(m_node.chainman)->ActiveChain().Tip()->GetMedianTimePast()+1; txCoinbase.version = 1; - txCoinbase.vin[0].scriptSig = CScript{} << (m_node.chainman->ActiveChain().Height() + 1) << bi.extranonce; + txCoinbase.vin[0].scriptSig = CScript{} << (current_height + 1) << bi.extranonce; txCoinbase.vout.resize(1); // Ignore the (optional) segwit commitment added by CreateNewBlock (as the hardcoded nonces don't account for this) txCoinbase.vout[0].scriptPubKey = CScript(); - pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase)); + block.vtx[0] = MakeTransactionRef(txCoinbase); if (txFirst.size() == 0) - baseheight = m_node.chainman->ActiveChain().Height(); + baseheight = current_height; if (txFirst.size() < 4) - txFirst.push_back(pblock->vtx[0]); - pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); - pblock->nNonce = bi.nonce; + txFirst.push_back(block.vtx[0]); + block.hashMerkleRoot = BlockMerkleRoot(block); + block.nNonce = bi.nonce; + } + std::shared_ptr shared_pblock = std::make_shared(block); + // Alternate calls between Chainman's ProcessNewBlock and submitSolution + // via the Mining interface. The former is used by net_processing as well + // as the submitblock RPC. + if (current_height % 2 == 0) { + BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, /*force_processing=*/true, /*min_pow_checked=*/true, nullptr)); + } else { + BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, MakeTransactionRef(txCoinbase))); + } + { + LOCK(cs_main); + // The above calls don't guarantee the tip is actually updated, so + // we explictly check this. + auto maybe_new_tip{Assert(m_node.chainman)->ActiveChain().Tip()}; + BOOST_REQUIRE_EQUAL(maybe_new_tip->GetBlockHash(), block.GetHash()); } - std::shared_ptr shared_pblock = std::make_shared(*pblock); - BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, true, true, nullptr)); - pblock->hashPrevBlock = pblock->GetHash(); + // This just adds coverage + mining->waitTipChanged(block.hashPrevBlock); } LOCK(cs_main); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 96a9ae2c2f..b58ad7b6b8 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -200,7 +200,9 @@ BasicTestingSetup::~BasicTestingSetup() { m_node.ecc_context.reset(); m_node.kernel.reset(); - SetMockTime(0s); // Reset mocktime for following tests + if constexpr (!G_FUZZING) { + SetMockTime(0s); // Reset mocktime for following tests + } LogInstance().DisconnectTestLogger(); if (m_has_custom_datadir) { // Only remove the lock file, preserve the data directory. diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 950004044e..3a5a3fb306 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -442,7 +442,7 @@ void CTxMemPool::Apply(ChangeSet* changeset) std::optional ancestors; if (i == 0) { // Note: ChangeSet::CalculateMemPoolAncestors() will return a - // cached value if mempool ancestors for this tranaction were + // cached value if mempool ancestors for this transaction were // previously calculated. // We can only use a cached ancestor calculation for the first // transaction in a package, because in-package parents won't be diff --git a/src/txmempool.h b/src/txmempool.h index e505c87f09..10acb2aa22 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -810,7 +810,7 @@ class CTxMemPool * mempool. * * CalculateMemPoolAncestors() calculates the in-mempool (not including - * what is in the change set itself) ancestors of a given transacion. + * what is in the change set itself) ancestors of a given transaction. * * Apply() will apply the removals and additions that are staged into the * mempool. diff --git a/src/uint256.h b/src/uint256.h index 8223787041..708f0c2ff1 100644 --- a/src/uint256.h +++ b/src/uint256.h @@ -69,16 +69,27 @@ class base_blob /** @name Hex representation * - * The reverse-byte hex representation is a convenient way to view the blob - * as a number, because it is consistent with the way the base_uint class - * converts blobs to numbers. + * The hex representation used by GetHex(), ToString(), FromHex() and + * SetHexDeprecated() is unusual, since it shows bytes of the base_blob in + * reverse order. For example, a 4-byte blob {0x12, 0x34, 0x56, 0x78} is + * represented as "78563412" instead of the more typical "12345678" + * representation that would be shown in a hex editor or used by typical + * byte-array / hex conversion functions like python's bytes.hex() and + * bytes.fromhex(). + * + * The nice thing about the reverse-byte representation, even though it is + * unusual, is that if a blob contains an arithmetic number in little endian + * format (with least significant bytes first, and most significant bytes + * last), the GetHex() output will match the way the number would normally + * be written in base-16 (with most significant digits first and least + * significant digits last). + * + * This means, for example, that ArithToUint256(num).GetHex() can be used to + * display an arith_uint256 num value as a number, because + * ArithToUint256() converts the number to a blob in little-endian format, + * so the arith_uint256 class doesn't need to have its own number parsing + * and formatting functions. * - * @note base_uint treats the blob as an array of bytes with the numerically - * least significant byte first and the most significant byte last. Because - * numbers are typically written with the most significant digit first and - * the least significant digit last, the reverse hex display of the blob - * corresponds to the same numeric value that base_uint interprets from the - * blob. * @{*/ std::string GetHex() const; /** Unlike FromHex this accepts any invalid input, thus it is fragile and deprecated! diff --git a/src/util/time.cpp b/src/util/time.cpp index e20f30a474..00f0f47392 100644 --- a/src/util/time.cpp +++ b/src/util/time.cpp @@ -20,10 +20,14 @@ void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); } static std::atomic g_mock_time{}; //!< For testing +std::atomic g_used_system_time{false}; NodeClock::time_point NodeClock::now() noexcept { const auto mocktime{g_mock_time.load(std::memory_order_relaxed)}; + if (!mocktime.count()) { + g_used_system_time = true; + } const auto ret{ mocktime.count() ? mocktime : diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 23e2257b1e..cc11500844 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -525,6 +525,12 @@ bool LegacyScriptPubKeyMan::HavePrivateKeys() const return !mapKeys.empty() || !mapCryptedKeys.empty(); } +bool LegacyScriptPubKeyMan::HaveCryptedKeys() const +{ + LOCK(cs_KeyStore); + return !mapCryptedKeys.empty(); +} + void LegacyScriptPubKeyMan::RewriteDB() { LOCK(cs_KeyStore); @@ -2411,6 +2417,12 @@ bool DescriptorScriptPubKeyMan::HavePrivateKeys() const return m_map_keys.size() > 0 || m_map_crypted_keys.size() > 0; } +bool DescriptorScriptPubKeyMan::HaveCryptedKeys() const +{ + LOCK(cs_desc_man); + return !m_map_crypted_keys.empty(); +} + std::optional DescriptorScriptPubKeyMan::GetOldestKeyPoolTime() const { // This is only used for getwalletinfo output and isn't relevant to descriptor wallets. diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index d8b6c90178..9b62a35bed 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -221,6 +221,7 @@ class ScriptPubKeyMan virtual bool Upgrade(int prev_version, int new_version, bilingual_str& error) { return true; } virtual bool HavePrivateKeys() const { return false; } + virtual bool HaveCryptedKeys() const { return false; } //! The action to do when the DB needs rewrite virtual void RewriteDB() {} @@ -473,6 +474,7 @@ class LegacyScriptPubKeyMan : public LegacyDataSPKM bool Upgrade(int prev_version, int new_version, bilingual_str& error) override; bool HavePrivateKeys() const override; + bool HaveCryptedKeys() const override; void RewriteDB() override; @@ -661,6 +663,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan bool HasPrivKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); //! Retrieve the particular key if it is available. Returns nullopt if the key is not in the wallet, or if the wallet is locked. std::optional GetKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); + bool HaveCryptedKeys() const override; std::optional GetOldestKeyPoolTime() const override; unsigned int GetKeyPoolSize() const override; diff --git a/src/wallet/test/CMakeLists.txt b/src/wallet/test/CMakeLists.txt index 8b442b262b..f14a78ca1d 100644 --- a/src/wallet/test/CMakeLists.txt +++ b/src/wallet/test/CMakeLists.txt @@ -8,6 +8,7 @@ target_sources(test_bitcoin PRIVATE init_test_fixture.cpp wallet_test_fixture.cpp + db_tests.cpp coinselector_tests.cpp feebumper_tests.cpp group_outputs_tests.cpp @@ -22,10 +23,4 @@ target_sources(test_bitcoin walletdb_tests.cpp walletload_tests.cpp ) -if(USE_BDB) - target_sources(test_bitcoin - PRIVATE - db_tests.cpp - ) -endif() target_link_libraries(test_bitcoin bitcoin_wallet) diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp index 41951e84c8..56a39c8d5f 100644 --- a/src/wallet/test/db_tests.cpp +++ b/src/wallet/test/db_tests.cpp @@ -62,6 +62,7 @@ static void CheckPrefix(DatabaseBatch& batch, Span prefix, Mock BOOST_FIXTURE_TEST_SUITE(db_tests, BasicTestingSetup) +#ifdef USE_BDB static std::shared_ptr GetWalletEnv(const fs::path& path, fs::path& database_filename) { fs::path data_file = BDBDataFile(path); @@ -124,6 +125,7 @@ BOOST_AUTO_TEST_CASE(getwalletenv_g_dbenvs_free_instance) BOOST_CHECK(env_1_a != env_1_b); BOOST_CHECK(env_2_a == env_2_b); } +#endif static std::vector> TestDatabases(const fs::path& path_root) { diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 7b85fb26fc..b1302e3b35 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -58,6 +59,7 @@ FUZZ_TARGET(wallet_notifications, .init = initialize_setup) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); // The total amount, to be distributed to the wallets a and b in txs // without fee. Thus, the balance of the wallets should always equal the // total amount. diff --git a/src/wallet/test/fuzz/scriptpubkeyman.cpp b/src/wallet/test/fuzz/scriptpubkeyman.cpp index b0d8628a91..0c17a6bf7a 100644 --- a/src/wallet/test/fuzz/scriptpubkeyman.cpp +++ b/src/wallet/test/fuzz/scriptpubkeyman.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -87,6 +88,7 @@ FUZZ_TARGET(scriptpubkeyman, .init = initialize_spkm) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); const auto& node{g_setup->m_node}; Chainstate& chainstate{node.chainman->ActiveChainstate()}; std::unique_ptr wallet_ptr{std::make_unique(node.chain.get(), "", CreateMockableWalletDatabase())}; diff --git a/src/wallet/test/fuzz/spend.cpp b/src/wallet/test/fuzz/spend.cpp index c4c04bce4b..552364a667 100644 --- a/src/wallet/test/fuzz/spend.cpp +++ b/src/wallet/test/fuzz/spend.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -32,6 +33,7 @@ FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); const auto& node = g_setup->m_node; Chainstate& chainstate{node.chainman->ActiveChainstate()}; ArgsManager& args = *node.args; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b971be5ddd..e7e06aec01 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -490,7 +490,9 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& return wallet; } -std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) +// Re-creates wallet from the backup file by renaming and moving it into the wallet's directory. +// If 'load_after_restore=true', the wallet object will be fully initialized and appended to the context. +std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings, bool load_after_restore) { DatabaseOptions options; ReadDatabaseArgs(*context.args, options); @@ -515,13 +517,17 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b fs::copy_file(backup_file, wallet_file, fs::copy_options::none); - wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings); + if (load_after_restore) { + wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings); + } } catch (const std::exception& e) { assert(!wallet); if (!error.empty()) error += Untranslated("\n"); error += Untranslated(strprintf("Unexpected exception: %s", e.what())); } - if (!wallet) { + + // Remove created wallet path only when loading fails + if (load_after_restore && !wallet) { fs::remove_all(wallet_path); } @@ -3706,6 +3712,14 @@ bool CWallet::HasEncryptionKeys() const return !mapMasterKeys.empty(); } +bool CWallet::HaveCryptedKeys() const +{ + for (const auto& spkm : GetAllScriptPubKeyMans()) { + if (spkm->HaveCryptedKeys()) return true; + } + return false; +} + void CWallet::ConnectScriptPubKeyManNotifiers() { for (const auto& spk_man : GetActiveScriptPubKeyMans()) { @@ -4527,7 +4541,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle } if (!success) { // Migration failed, cleanup - // Copy the backup to the actual wallet dir + // Before deleting the wallet's directory, copy the backup file to the top-level wallets dir fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename); fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none); @@ -4564,17 +4578,24 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle } // Restore the backup - DatabaseStatus status; - std::vector warnings; - if (!RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, error, warnings)) { - error += _("\nUnable to restore backup of wallet."); + // Convert the backup file to the wallet db file by renaming it and moving it into the wallet's directory. + // Reload it into memory if the wallet was previously loaded. + bilingual_str restore_error; + const auto& ptr_wallet = RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/was_loaded); + if (!restore_error.empty()) { + error += restore_error + _("\nUnable to restore backup of wallet."); return util::Error{error}; } - // Move the backup to the wallet dir + // The wallet directory has been restored, but just in case, copy the previously created backup to the wallet dir fs::copy_file(temp_backup_location, backup_path, fs::copy_options::none); fs::remove(temp_backup_location); + // Verify that there is no dangling wallet: when the wallet wasn't loaded before, expect null. + // This check is performed after restoration to avoid an early error before saving the backup. + bool wallet_reloaded = ptr_wallet != nullptr; + assert(was_loaded == wallet_reloaded); + return util::Error{error}; } return res; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 14dea81c46..2deb747603 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -95,7 +95,7 @@ std::shared_ptr GetDefaultWallet(WalletContext& context, size_t& count) std::shared_ptr GetWallet(WalletContext& context, const std::string& name); std::shared_ptr LoadWallet(WalletContext& context, const std::string& name, std::optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); std::shared_ptr CreateWallet(WalletContext& context, const std::string& name, std::optional load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); -std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); +std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings, bool load_after_restore = true); std::unique_ptr HandleLoadWallet(WalletContext& context, LoadWalletFn load_wallet); void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr& wallet); std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); @@ -973,6 +973,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool WithEncryptionKey(std::function cb) const override; bool HasEncryptionKeys() const override; + bool HaveCryptedKeys() const; /** Get last block processed height */ int GetLastBlockHeight() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 368415da12..801c2ab97b 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -154,6 +154,11 @@ bool WalletBatch::WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey) return WriteIC(std::make_pair(DBKeys::MASTER_KEY, nID), kMasterKey, true); } +bool WalletBatch::EraseMasterKey(unsigned int id) +{ + return EraseIC(std::make_pair(DBKeys::MASTER_KEY, id)); +} + bool WalletBatch::WriteCScript(const uint160& hash, const CScript& redeemScript) { return WriteIC(std::make_pair(DBKeys::CSCRIPT, hash), redeemScript, false); @@ -1241,6 +1246,21 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) result = DBErrors::CORRUPT; } + // Since it was accidentally possible to "encrypt" a wallet with private keys disabled, we should check if this is + // such a wallet and remove the encryption key records to avoid any future issues. + // Although wallets without private keys should not have *ckey records, we should double check that. + // Removing the mkey records is only safe if there are no *ckey records. + if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && pwallet->HasEncryptionKeys() && !pwallet->HaveCryptedKeys()) { + pwallet->WalletLogPrintf("Detected extraneous encryption keys in this wallet without private keys. Removing extraneous encryption keys.\n"); + for (const auto& [id, _] : pwallet->mapMasterKeys) { + if (!EraseMasterKey(id)) { + pwallet->WalletLogPrintf("Error: Unable to remove extraneous encryption key '%u'. Wallet corrupt.\n", id); + return DBErrors::CORRUPT; + } + } + pwallet->mapMasterKeys.clear(); + } + return result; } diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 32c3c29b5e..70d6987012 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -243,6 +243,7 @@ class WalletBatch bool WriteKey(const CPubKey& vchPubKey, const CPrivKey& vchPrivKey, const CKeyMetadata &keyMeta); bool WriteCryptedKey(const CPubKey& vchPubKey, const std::vector& vchCryptedSecret, const CKeyMetadata &keyMeta); bool WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey); + bool EraseMasterKey(unsigned int id); bool WriteCScript(const uint160& hash, const CScript& redeemScript); diff --git a/test/functional/mempool_ephemeral_dust.py b/test/functional/mempool_ephemeral_dust.py index 10cacd9539..e614a9e607 100755 --- a/test/functional/mempool_ephemeral_dust.py +++ b/test/functional/mempool_ephemeral_dust.py @@ -126,7 +126,7 @@ def test_node_restart(self): assert_equal(len(self.nodes[0].getrawmempool()), 2) assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]]) - # Node restart; doesn't allow allow ephemeral transaction back in due to individual submission + # Node restart; doesn't allow ephemeral transaction back in due to individual submission # resulting in 0-fee. Supporting re-submission of CPFP packages on restart is desired but not # yet implemented. self.restart_node(0) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 7e8c40cf16..16069e522f 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -526,6 +526,15 @@ def get_bin_from_version(version, bin_name, bin_default): binary = [get_bin_from_version(v, 'bitcoind', self.options.bitcoind) for v in versions] if binary_cli is None: binary_cli = [get_bin_from_version(v, 'bitcoin-cli', self.options.bitcoincli) for v in versions] + # Fail test if any of the needed release binaries is missing + bins_missing = False + for bin_path in binary + binary_cli: + if shutil.which(bin_path) is None: + self.log.error(f"Binary not found: {bin_path}") + bins_missing = True + if bins_missing: + raise AssertionError("At least one release binary is missing. " + "Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.") assert_equal(len(extra_confs), num_nodes) assert_equal(len(extra_args), num_nodes) assert_equal(len(versions), num_nodes) diff --git a/test/functional/wallet_encryption.py b/test/functional/wallet_encryption.py index 5a5fb3e5be..b30a1478ab 100755 --- a/test/functional/wallet_encryption.py +++ b/test/functional/wallet_encryption.py @@ -5,7 +5,9 @@ """Test Wallet encryption""" import time +import subprocess +from test_framework.messages import hash256 from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_raises_rpc_error, @@ -100,6 +102,65 @@ def run_test(self): sig = self.nodes[0].signmessage(address, msg) assert self.nodes[0].verifymessage(address, sig, msg) + self.log.info("Test that wallets without private keys cannot be encrypted") + self.nodes[0].createwallet(wallet_name="noprivs", disable_private_keys=True) + noprivs_wallet = self.nodes[0].get_wallet_rpc("noprivs") + assert_raises_rpc_error(-16, "Error: wallet does not contain private keys, nothing to encrypt.", noprivs_wallet.encryptwallet, "pass") + + if self.is_wallet_tool_compiled(): + self.log.info("Test that encryption keys in wallets without privkeys are removed") + + def do_wallet_tool(*args): + proc = subprocess.Popen( + [self.options.bitcoinwallet, f"-datadir={self.nodes[0].datadir_path}", f"-chain={self.chain}"] + list(args), + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + stdout, stderr = proc.communicate() + assert_equal(proc.poll(), 0) + assert_equal(stderr, "") + + # Since it is no longer possible to encrypt a wallet without privkeys, we need to force one into the wallet + # 1. Make a dump of the wallet + # 2. Add mkey record to the dump + # 3. Create a new wallet from the dump + + # Make the dump + noprivs_wallet.unloadwallet() + dumpfile_path = self.nodes[0].datadir_path / "noprivs.dump" + do_wallet_tool("-wallet=noprivs", f"-dumpfile={dumpfile_path}", "dump") + + # Modify the dump + with open(dumpfile_path, "r", encoding="utf-8") as f: + dump_content = f.readlines() + # Drop the checksum line + dump_content = dump_content[:-1] + # Insert a valid mkey line. This corresponds to a passphrase of "pass". + dump_content.append("046d6b657901000000,300dc926f3b3887aad3d5d5f5a0fc1b1a4a1722f9284bd5c6ff93b64a83902765953939c58fe144013c8b819f42cf698b208e9911e5f0c544fa300000000cc52050000\n") + with open(dumpfile_path, "w", encoding="utf-8") as f: + contents = "".join(dump_content) + f.write(contents) + checksum = hash256(contents.encode()) + f.write(f"checksum,{checksum.hex()}\n") + + # Load the dump into a new wallet + do_wallet_tool("-wallet=noprivs_enc", f"-dumpfile={dumpfile_path}", "createfromdump") + # Load the wallet and make sure it is no longer encrypted + with self.nodes[0].assert_debug_log(["Detected extraneous encryption keys in this wallet without private keys. Removing extraneous encryption keys."]): + self.nodes[0].loadwallet("noprivs_enc") + noprivs_wallet = self.nodes[0].get_wallet_rpc("noprivs_enc") + assert_raises_rpc_error(-15, "Error: running with an unencrypted wallet, but walletpassphrase was called.", noprivs_wallet.walletpassphrase, "pass", 1) + noprivs_wallet.unloadwallet() + + # Make a new dump and check that there are no mkeys + dumpfile_path = self.nodes[0].datadir_path / "noprivs_enc.dump" + do_wallet_tool("-wallet=noprivs_enc", f"-dumpfile={dumpfile_path}", "dump") + with open(dumpfile_path, "r", encoding="utf-8") as f: + # Check theres nothing with an 'mkey' prefix + assert_equal(all([not line.startswith("046d6b6579") for line in f]), True) + if __name__ == '__main__': WalletEncryptionTest(__file__).main() diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 3a56050731..5be56cec29 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -896,9 +896,7 @@ def test_failed_migration_cleanup(self): shutil.copytree(self.old_node.wallets_path / "failed", self.master_node.wallets_path / "failed") assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, "failed") - assert "failed" in self.master_node.listwallets() - assert "failed_watchonly" not in self.master_node.listwallets() - assert "failed_solvables" not in self.master_node.listwallets() + assert all(wallet not in self.master_node.listwallets() for wallet in ["failed", "failed_watchonly", "failed_solvables"]) assert not (self.master_node.wallets_path / "failed_watchonly").exists() # Since the file in failed_solvables is one that we put there, migration shouldn't touch it @@ -912,6 +910,22 @@ def test_failed_migration_cleanup(self): _, _, magic = struct.unpack("QII", data) assert_equal(magic, BTREE_MAGIC) + #################################################### + # Perform the same test with a loaded legacy wallet. + # The wallet should remain loaded after the failure. + # + # This applies only when BDB is enabled, as the user + # cannot interact with the legacy wallet database + # without BDB support. + if self.is_bdb_compiled() is not None: + # Advance time to generate a different backup name + self.master_node.setmocktime(self.master_node.getblockheader(self.master_node.getbestblockhash())['time'] + 100) + assert "failed" not in self.master_node.listwallets() + self.master_node.loadwallet("failed") + assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, "failed") + wallets = self.master_node.listwallets() + assert "failed" in wallets and all(wallet not in wallets for wallet in ["failed_watchonly", "failed_solvables"]) + def test_blank(self): self.log.info("Test that a blank wallet is migrated") wallet = self.create_legacy_wallet("blank", blank=True) diff --git a/test/lint/lint-assertions.py b/test/lint/lint-assertions.py deleted file mode 100755 index 5d01b13fd4..0000000000 --- a/test/lint/lint-assertions.py +++ /dev/null @@ -1,54 +0,0 @@ -#!/usr/bin/env python3 -# -# Copyright (c) 2018-2022 The Bitcoin Core developers -# Distributed under the MIT software license, see the accompanying -# file COPYING or http://www.opensource.org/licenses/mit-license.php. -# -# Check for assertions with obvious side effects. - -import sys -import subprocess - - -def git_grep(params: [], error_msg: ""): - try: - output = subprocess.check_output(["git", "grep", *params], text=True, encoding="utf8") - print(error_msg) - print(output) - return 1 - except subprocess.CalledProcessError as ex1: - if ex1.returncode > 1: - raise ex1 - return 0 - - -def main(): - # Aborting the whole process is undesirable for RPC code. So nonfatal - # checks should be used over assert. See: src/util/check.h - # src/rpc/server.cpp is excluded from this check since it's mostly meta-code. - exit_code = git_grep([ - "--line-number", - "--extended-regexp", - r"\<(A|a)ss(ume|ert)\(", - "--", - "src/rpc/", - "src/wallet/rpc*", - ":(exclude)src/rpc/server.cpp", - ], "CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.") - - # The `BOOST_ASSERT` macro requires to `#include boost/assert.hpp`, - # which is an unnecessary Boost dependency. - exit_code |= git_grep([ - "--line-number", - "--extended-regexp", - r"BOOST_ASSERT\(", - "--", - "*.cpp", - "*.h", - ], "BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK.") - - sys.exit(exit_code) - - -if __name__ == "__main__": - main() diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index f4fb1c36e5..8479fd2d64 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -48,6 +48,16 @@ fn get_linter_list() -> Vec<&'static Linter> { name: "std_filesystem", lint_fn: lint_std_filesystem }, + &Linter { + description: "Check that fatal assertions are not used in RPC code", + name: "rpc_assert", + lint_fn: lint_rpc_assert + }, + &Linter { + description: "Check that boost assertions are not used", + name: "boost_assert", + lint_fn: lint_boost_assert + }, &Linter { description: "Check that release note snippets are in the right folder", name: "doc_release_note_snippets", @@ -237,7 +247,7 @@ fn lint_py_lint() -> LintResult { "F822", // undefined name name in __all__ "F823", // local variable name … referenced before assignment "F841", // local variable 'foo' is assigned to but never used - "PLE", // Pylint errors + "PLE", // Pylint errors "W191", // indentation contains tabs "W291", // trailing whitespace "W292", // no newline at end of file @@ -273,6 +283,7 @@ fn lint_std_filesystem() -> LintResult { let found = git() .args([ "grep", + "--line-number", "std::filesystem", "--", "./src/", @@ -283,10 +294,66 @@ fn lint_std_filesystem() -> LintResult { .success(); if found { Err(r#" -^^^ Direct use of std::filesystem may be dangerous and buggy. Please include and use the fs:: namespace, which has unsafe filesystem functions marked as deleted. "# + .trim() + .to_string()) + } else { + Ok(()) + } +} + +fn lint_rpc_assert() -> LintResult { + let found = git() + .args([ + "grep", + "--line-number", + "--extended-regexp", + r"\<(A|a)ss(ume|ert)\(", + "--", + "src/rpc/", + "src/wallet/rpc*", + ":(exclude)src/rpc/server.cpp", + // src/rpc/server.cpp is excluded from this check since it's mostly meta-code. + ]) + .status() + .expect("command error") + .success(); + if found { + Err(r#" +CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code. + +Aborting the whole process is undesirable for RPC code. So nonfatal +checks should be used over assert. See: src/util/check.h + "# + .trim() + .to_string()) + } else { + Ok(()) + } +} + +fn lint_boost_assert() -> LintResult { + let found = git() + .args([ + "grep", + "--line-number", + "--extended-regexp", + r"BOOST_ASSERT\(", + "--", + "*.cpp", + "*.h", + ]) + .status() + .expect("command error") + .success(); + if found { + Err(r#" +BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unnecessary +include of the boost/assert.hpp dependency. + "# + .trim() .to_string()) } else { Ok(()) @@ -303,17 +370,15 @@ fn lint_doc_release_note_snippets() -> LintResult { if non_release_notes.is_empty() { Ok(()) } else { - Err(format!( - r#" -{} -^^^ + println!("{non_release_notes}"); + Err(r#" Release note snippets and other docs must be put into the doc/ folder directly. The doc/release-notes/ folder is for archived release notes of previous releases only. Snippets are expected to follow the naming "/doc/release-notes-.md". - "#, - non_release_notes - )) + "# + .trim() + .to_string()) } } @@ -356,7 +421,6 @@ fn lint_trailing_whitespace() -> LintResult { .success(); if trailing_space { Err(r#" -^^^ Trailing whitespace (including Windows line endings [CR LF]) is problematic, because git may warn about it, or editors may remove it by default, forcing developers in the future to either undo the changes manually or spend time on review. @@ -366,6 +430,7 @@ Thus, it is best to remove the trailing space now. Please add any false positives, such as subtrees, Windows-related files, patch files, or externally sourced files to the exclude list. "# + .trim() .to_string()) } else { Ok(()) @@ -382,7 +447,6 @@ fn lint_tabs_whitespace() -> LintResult { .success(); if tabs { Err(r#" -^^^ Use of tabs in this codebase is problematic, because existing code uses spaces and tabs will cause display issues and conflict with editor settings. @@ -390,6 +454,7 @@ Please remove the tabs. Please add any false positives, such as subtrees, or externally sourced files to the exclude list. "# + .trim() .to_string()) } else { Ok(()) @@ -464,7 +529,6 @@ fn lint_includes_build_config() -> LintResult { if missing { return Err(format!( r#" -^^^ One or more files use a symbol declared in the bitcoin-build-config.h header. However, they are not including the header. This is problematic, because the header may or may not be indirectly included. If the indirect include were to be intentionally or accidentally removed, the build could @@ -480,12 +544,13 @@ include again. #include // IWYU pragma: keep "#, defines_regex - )); + ) + .trim() + .to_string()); } let redundant = print_affected_files(false); if redundant { return Err(r#" -^^^ None of the files use a symbol declared in the bitcoin-build-config.h header. However, they are including the header. Consider removing the unused include. "# @@ -538,7 +603,9 @@ Markdown link errors found: {} "#, stderr - )) + ) + .trim() + .to_string()) } Err(e) if e.kind() == ErrorKind::NotFound => { println!("`mlc` was not found in $PATH, skipping markdown lint check."); @@ -590,10 +657,9 @@ fn main() -> ExitCode { env::set_current_dir(&git_root).unwrap(); if let Err(err) = (linter.lint_fn)() { println!( - "{err}\n^---- ⚠️ Failure generated from lint check '{}'!", - linter.name + "^^^\n{err}\n^---- ⚠️ Failure generated from lint check '{}' ({})!\n\n", + linter.name, linter.description, ); - println!("{}", linter.description); test_failed = true; } }