diff --git a/.cirrus.yml b/.cirrus.yml index 7f7a882cee..2a3cb1c6d9 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -131,7 +131,7 @@ task: FILE_ENV: "./ci/test/00_setup_env_arm.sh" task: - name: 'Win64, unit tests, no gui tests, no functional tests' + name: 'Win64-cross' << : *GLOBAL_TASK_TEMPLATE persistent_worker: labels: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8021bdffe4..740d31ae56 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -120,8 +120,7 @@ jobs: run: | # A workaround for "The `brew link` step did not complete successfully" error. brew install --quiet python@3 || brew link --overwrite python@3 - # Update to install pkgconf, once the GHA image has been updated. - brew install --quiet coreutils ninja gnu-getopt ccache boost libevent zeromq qt@5 qrencode + brew install --quiet coreutils ninja pkgconf gnu-getopt ccache boost libevent zeromq qt@5 qrencode - name: Set Ccache directory run: echo "CCACHE_DIR=${RUNNER_TEMP}/ccache_dir" >> "$GITHUB_ENV" diff --git a/ci/lint/04_install.sh b/ci/lint/04_install.sh index d899c0c67a..9ef1f37c73 100755 --- a/ci/lint/04_install.sh +++ b/ci/lint/04_install.sh @@ -59,7 +59,7 @@ curl -sL "https://github.com/koalaman/shellcheck/releases/download/${SHELLCHECK_ tar --xz -xf - --directory /tmp/ mv "/tmp/shellcheck-${SHELLCHECK_VERSION}/shellcheck" /usr/bin/ -MLC_VERSION=v0.18.0 +MLC_VERSION=v0.19.0 MLC_BIN=mlc-x86_64-linux curl -sL "https://github.com/becheran/mlc/releases/download/${MLC_VERSION}/${MLC_BIN}" -o "/usr/bin/mlc" chmod +x /usr/bin/mlc diff --git a/ci/test/00_setup_env_native_tidy.sh b/ci/test/00_setup_env_native_tidy.sh index 9068045f93..b85fae859c 100755 --- a/ci/test/00_setup_env_native_tidy.sh +++ b/ci/test/00_setup_env_native_tidy.sh @@ -8,7 +8,8 @@ export LC_ALL=C.UTF-8 export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04" export CONTAINER_NAME=ci_native_tidy -export TIDY_LLVM_V="18" +export TIDY_LLVM_V="19" +export APT_LLVM_V="${TIDY_LLVM_V}" export PACKAGES="clang-${TIDY_LLVM_V} libclang-${TIDY_LLVM_V}-dev llvm-${TIDY_LLVM_V}-dev libomp-${TIDY_LLVM_V}-dev clang-tidy-${TIDY_LLVM_V} jq libevent-dev libboost-dev libzmq3-dev systemtap-sdt-dev qtbase5-dev qttools5-dev qttools5-dev-tools libqrencode-dev libsqlite3-dev libdb++-dev" export NO_DEPENDS=1 export RUN_UNIT_TESTS=false diff --git a/ci/test/00_setup_env_win64.sh b/ci/test/00_setup_env_win64.sh index 15b9e407b6..25da64c524 100755 --- a/ci/test/00_setup_env_win64.sh +++ b/ci/test/00_setup_env_win64.sh @@ -11,6 +11,9 @@ export CI_IMAGE_NAME_TAG="docker.io/amd64/debian:bookworm" # Check that https:/ export HOST=x86_64-w64-mingw32 export DPKG_ADD_ARCH="i386" export PACKAGES="nsis g++-mingw-w64-x86-64-posix wine-binfmt wine64 wine32 file" +# Install wine, but do not run unit tests, as they surface frequent +# false-positives. +export RUN_UNIT_TESTS=${RUN_UNIT_TESTS:-false} export RUN_FUNCTIONAL_TESTS=false export GOAL="deploy" # Prior to 11.0.0, the mingw-w64 headers were missing noreturn attributes, causing warnings when diff --git a/cmake/module/TryAppendCXXFlags.cmake b/cmake/module/TryAppendCXXFlags.cmake index c07455e89e..dc0a9b7f8f 100644 --- a/cmake/module/TryAppendCXXFlags.cmake +++ b/cmake/module/TryAppendCXXFlags.cmake @@ -72,7 +72,8 @@ function(try_append_cxx_flags flags) target_compile_options(${TACXXF_TARGET} INTERFACE ${TACXXF_IF_CHECK_PASSED}) endif() if(DEFINED TACXXF_VAR) - string(STRIP "${${TACXXF_VAR}} ${TACXXF_IF_CHECK_PASSED}" ${TACXXF_VAR}) + list(JOIN TACXXF_IF_CHECK_PASSED " " flags_if_check_passed_as_string) + string(STRIP "${${TACXXF_VAR}} ${flags_if_check_passed_as_string}" ${TACXXF_VAR}) endif() else() if(DEFINED TACXXF_TARGET) diff --git a/cmake/module/TryAppendLinkerFlag.cmake b/cmake/module/TryAppendLinkerFlag.cmake index 8cbd83678d..be41a2e1cc 100644 --- a/cmake/module/TryAppendLinkerFlag.cmake +++ b/cmake/module/TryAppendLinkerFlag.cmake @@ -48,7 +48,8 @@ function(try_append_linker_flag flag) target_link_options(${TALF_TARGET} INTERFACE ${TALF_IF_CHECK_PASSED}) endif() if(DEFINED TALF_VAR) - string(STRIP "${${TALF_VAR}} ${TALF_IF_CHECK_PASSED}" ${TALF_VAR}) + list(JOIN TALF_IF_CHECK_PASSED " " flags_if_check_passed_as_string) + string(STRIP "${${TALF_VAR}} ${flags_if_check_passed_as_string}" ${TALF_VAR}) endif() else() if(DEFINED TALF_TARGET) diff --git a/contrib/devtools/iwyu/bitcoin.core.imp b/contrib/devtools/iwyu/bitcoin.core.imp index befc949f18..c4c4ba4ceb 100644 --- a/contrib/devtools/iwyu/bitcoin.core.imp +++ b/contrib/devtools/iwyu/bitcoin.core.imp @@ -1,4 +1,3 @@ -# Fixups / upstreamed changes +# Nothing for now. [ - { include: [ "", private, "", public ] }, ] diff --git a/contrib/guix/README.md b/contrib/guix/README.md index 5e05e1016e..2c9056ce9c 100644 --- a/contrib/guix/README.md +++ b/contrib/guix/README.md @@ -31,7 +31,7 @@ section](#choosing-your-security-model) before proceeding to perform a build. In order to perform a build for macOS (which is included in the default set of platform triples to build), you'll need to extract the macOS SDK tarball using -tools found in the [`macdeploy` directory](../macdeploy/README.md). +tools found in the [`macdeploy` directory](../macdeploy/README.md#sdk-extraction). You can then either point to the SDK using the `SDK_PATH` environment variable: @@ -68,7 +68,7 @@ following from the top of a clean repository: The `guix-codesign` command attaches codesignatures (produced by codesigners) to existing non-codesigned outputs. Please see the [release process -documentation](/doc/release-process.md) for more context. +documentation](/doc/release-process.md#codesigning) for more context. It respects many of the same environment variable flags as `guix-build`, with 2 crucial differences: diff --git a/contrib/tracing/log_raw_p2p_msgs.py b/contrib/tracing/log_raw_p2p_msgs.py index bd51c2eb7d..9cda7fd08d 100755 --- a/contrib/tracing/log_raw_p2p_msgs.py +++ b/contrib/tracing/log_raw_p2p_msgs.py @@ -117,9 +117,9 @@ def print_message(event, inbound): - print(f"%s %s msg '%s' from peer %d (%s, %s) with %d bytes: %s" % - ( - f"Warning: incomplete message (only %d out of %d bytes)!" % ( + print("{} {} msg '{}' from peer {} ({}, {}) with {} bytes: {}".format( + + "Warning: incomplete message (only {} out of {} bytes)!".format( len(event.msg), event.msg_size) if len(event.msg) < event.msg_size else "", "inbound" if inbound else "outbound", event.msg_type.decode("utf-8"), diff --git a/depends/toolchain.cmake.in b/depends/toolchain.cmake.in index 69ff3cfb74..e72e6e29e2 100644 --- a/depends/toolchain.cmake.in +++ b/depends/toolchain.cmake.in @@ -55,12 +55,21 @@ set(DEPENDS_COMPILE_DEFINITIONS_DEBUG @CPPFLAGS_DEBUG@) if(NOT DEFINED CMAKE_EXE_LINKER_FLAGS_INIT) set(CMAKE_EXE_LINKER_FLAGS_INIT "@LDFLAGS@") endif() +if(NOT DEFINED CMAKE_SHARED_LINKER_FLAGS_INIT) + set(CMAKE_SHARED_LINKER_FLAGS_INIT "@LDFLAGS@") +endif() if(NOT DEFINED CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO_INIT) set(CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO_INIT "@LDFLAGS_RELEASE@") endif() +if(NOT DEFINED CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO_INIT) + set(CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO_INIT "@LDFLAGS_RELEASE@") +endif() if(NOT DEFINED CMAKE_EXE_LINKER_FLAGS_DEBUG_INIT) set(CMAKE_EXE_LINKER_FLAGS_DEBUG_INIT "@LDFLAGS_DEBUG@") endif() +if(NOT DEFINED CMAKE_SHARED_LINKER_FLAGS_DEBUG_INIT) + set(CMAKE_SHARED_LINKER_FLAGS_DEBUG_INIT "@LDFLAGS_DEBUG@") +endif() set(CMAKE_AR "@AR@") set(CMAKE_RANLIB "@RANLIB@") diff --git a/doc/fuzzing.md b/doc/fuzzing.md index 927b0dc8d5..365314a592 100644 --- a/doc/fuzzing.md +++ b/doc/fuzzing.md @@ -15,7 +15,7 @@ $ FUZZ=process_message build_fuzz/src/test/fuzz/fuzz # abort fuzzing using ctrl-c ``` -One can use `--prefix=libfuzzer-nosan` to do the same without common sanitizers enabled. +One can use `--preset=libfuzzer-nosan` to do the same without common sanitizers enabled. See [further](#run-without-sanitizers-for-increased-throughput) for more information. There is also a runner script to execute all fuzz targets. Refer to diff --git a/doc/policy/packages.md b/doc/policy/packages.md index ff2539497d..9bd9becfdc 100644 --- a/doc/policy/packages.md +++ b/doc/policy/packages.md @@ -74,7 +74,7 @@ The following rules are only enforced for packages to be submitted to the mempoo enforced for test accepts): * Packages must be child-with-unconfirmed-parents packages. This also means packages must contain at - least 2 transactions. (#22674) + least 1 transaction. (#31096) - *Rationale*: This allows for fee-bumping by CPFP. Allowing multiple parents makes it possible to fee-bump a batch of transactions. Restricting packages to a defined topology is easier to diff --git a/doc/release-notes-31175.md b/doc/release-notes-31175.md new file mode 100644 index 0000000000..84a04a7426 --- /dev/null +++ b/doc/release-notes-31175.md @@ -0,0 +1,8 @@ +RPC +--- + +Duplicate blocks submitted with `submitblock` will now persist their block data +even if it was previously pruned. If pruning is activated, the data will be +pruned again eventually once the block file it is persisted in is selected for +pruning. This is consistent with the behaviour of `getblockfrompeer` where the +block is persisted as well even when pruning. diff --git a/doc/release-process.md b/doc/release-process.md index 3769991c4d..197ed8c0a4 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -134,7 +134,7 @@ git -C ./guix.sigs pull ### Create the macOS SDK tarball (first time, or when SDK version changes) Create the macOS SDK tarball, see the [macdeploy -instructions](/contrib/macdeploy/README.md#deterministic-macos-app-notes) for +instructions](/contrib/macdeploy/README.md#sdk-extraction) for details. ### Build and attest to build outputs diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 59e47a1ee0..40d1ce74b9 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -73,6 +73,9 @@ if(DEFINED ENV{CFLAGS}) endif() set(CMAKE_EXPORT_COMPILE_COMMANDS OFF) add_subdirectory(secp256k1) +set_target_properties(secp256k1 PROPERTIES + EXCLUDE_FROM_ALL TRUE +) set(CMAKE_EXPORT_COMPILE_COMMANDS ON) string(APPEND CMAKE_C_COMPILE_OBJECT " ${APPEND_CPPFLAGS} ${APPEND_CFLAGS}") @@ -299,8 +302,8 @@ target_link_libraries(bitcoin_node minisketch univalue Boost::headers - libevent::core - libevent::extra + $ + $ $ $ ) diff --git a/src/bench/checkqueue.cpp b/src/bench/checkqueue.cpp index d1454f3634..8134154eb1 100644 --- a/src/bench/checkqueue.cpp +++ b/src/bench/checkqueue.cpp @@ -34,9 +34,9 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench) explicit PrevectorJob(FastRandomContext& insecure_rand){ p.resize(insecure_rand.randrange(PREVECTOR_SIZE*2)); } - bool operator()() + std::optional operator()() { - return true; + return std::nullopt; } }; @@ -62,7 +62,7 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench) } // control waits for completion by RAII, but // it is done explicitly here for clarity - control.Wait(); + control.Complete(); }); } BENCHMARK(CCheckQueueSpeedPrevectorJob, benchmark::PriorityLevel::HIGH); diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 4bb463acf5..bab09f13d9 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -180,27 +180,6 @@ int main(int argc, char* argv[]) break; } - if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { - std::cerr << "Block does not start with a coinbase" << std::endl; - break; - } - - uint256 hash = block.GetHash(); - { - LOCK(cs_main); - const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash); - if (pindex) { - if (pindex->IsValid(BLOCK_VALID_SCRIPTS)) { - std::cerr << "duplicate" << std::endl; - break; - } - if (pindex->nStatus & BLOCK_FAILED_MASK) { - std::cerr << "duplicate-invalid" << std::endl; - break; - } - } - } - { LOCK(cs_main); const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock); diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index dd7b21e567..5c5965245b 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -82,7 +82,7 @@ static void SetupCliArgs(ArgsManager& argsman) argsman.AddArg("-version", "Print version and exit", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-conf=", strprintf("Specify configuration file. Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); - argsman.AddArg("-datadir=", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-datadir=", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); argsman.AddArg("-generate", strprintf("Generate blocks, equivalent to RPC getnewaddress followed by RPC generatetoaddress. Optional positional integer " "arguments are number of blocks to generate (default: %s) and maximum iterations to try (default: %s), equivalent to " @@ -94,7 +94,7 @@ static void SetupCliArgs(ArgsManager& argsman) argsman.AddArg("-netinfo", strprintf("Get network peer connection information from the remote server. An optional argument from 0 to %d can be passed for different peers listings (default: 0). If a non-zero value is passed, an additional \"outonly\" (or \"o\") argument can be passed to see outbound peers only. Pass \"help\" (or \"h\") for detailed help documentation.", NETINFO_MAX_LEVEL), ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS); SetupChainParamsBaseOptions(argsman); - argsman.AddArg("-color=", strprintf("Color setting for CLI output (default: %s). Valid values: always, auto (add color codes when standard output is connected to a terminal and OS is not WIN32), never.", DEFAULT_COLOR_SETTING), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); + argsman.AddArg("-color=", strprintf("Color setting for CLI output (default: %s). Valid values: always, auto (add color codes when standard output is connected to a terminal and OS is not WIN32), never. Only applies to the output of -getinfo.", DEFAULT_COLOR_SETTING), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); argsman.AddArg("-named", strprintf("Pass named instead of positional arguments (default: %s)", DEFAULT_NAMED), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-rpcclienttimeout=", strprintf("Timeout in seconds during HTTP requests, or 0 for no timeout. (default: %d)", DEFAULT_HTTP_CLIENT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-rpcconnect=", strprintf("Send commands to node running on (default: %s)", DEFAULT_RPCCONNECT), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); @@ -145,10 +145,10 @@ static int AppInitRPC(int argc, char* argv[]) tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error); return EXIT_FAILURE; } - if (argc < 2 || HelpRequested(gArgs) || gArgs.IsArgSet("-version")) { + if (argc < 2 || HelpRequested(gArgs) || gArgs.GetBoolArg("-version", false)) { std::string strUsage = CLIENT_NAME " RPC client version " + FormatFullVersion() + "\n"; - if (gArgs.IsArgSet("-version")) { + if (gArgs.GetBoolArg("-version", false)) { strUsage += FormatParagraph(LicenseInfo()); } else { strUsage += "\n" diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp index 7f96c626b1..a627c24e86 100644 --- a/src/bitcoin-tx.cpp +++ b/src/bitcoin-tx.cpp @@ -105,11 +105,11 @@ static int AppInitRawTx(int argc, char* argv[]) fCreateBlank = gArgs.GetBoolArg("-create", false); - if (argc < 2 || HelpRequested(gArgs) || gArgs.IsArgSet("-version")) { + if (argc < 2 || HelpRequested(gArgs) || gArgs.GetBoolArg("-version", false)) { // First part of help message is specific to this utility std::string strUsage = CLIENT_NAME " bitcoin-tx utility version " + FormatFullVersion() + "\n"; - if (gArgs.IsArgSet("-version")) { + if (gArgs.GetBoolArg("-version", false)) { strUsage += FormatParagraph(LicenseInfo()); } else { strUsage += "\n" diff --git a/src/bitcoin-util.cpp b/src/bitcoin-util.cpp index c7d10b6622..ff2e4fb800 100644 --- a/src/bitcoin-util.cpp +++ b/src/bitcoin-util.cpp @@ -50,11 +50,11 @@ static int AppInitUtil(ArgsManager& args, int argc, char* argv[]) return EXIT_FAILURE; } - if (HelpRequested(args) || args.IsArgSet("-version")) { + if (HelpRequested(args) || args.GetBoolArg("-version", false)) { // First part of help message is specific to this utility std::string strUsage = CLIENT_NAME " bitcoin-util utility version " + FormatFullVersion() + "\n"; - if (args.IsArgSet("-version")) { + if (args.GetBoolArg("-version", false)) { strUsage += FormatParagraph(LicenseInfo()); } else { strUsage += "\n" diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index be31620945..033cf7a0f3 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -34,7 +34,7 @@ static void SetupWalletToolArgs(ArgsManager& argsman) SetupChainParamsBaseOptions(argsman); argsman.AddArg("-version", "Print version and exit", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); - argsman.AddArg("-datadir=", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-datadir=", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); argsman.AddArg("-wallet=", "Specify wallet name", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::OPTIONS); argsman.AddArg("-dumpfile=", "When used with 'dump', writes out the records to this file. When used with 'createfromdump', loads the records into a new wallet.", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); argsman.AddArg("-debug=", "Output debugging information (default: 0).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); @@ -60,10 +60,10 @@ static std::optional WalletAppInit(ArgsManager& args, int argc, char* argv[ return EXIT_FAILURE; } const bool missing_args{argc < 2}; - if (missing_args || HelpRequested(args) || args.IsArgSet("-version")) { + if (missing_args || HelpRequested(args) || args.GetBoolArg("-version", false)) { std::string strUsage = strprintf("%s bitcoin-wallet utility version", CLIENT_NAME) + " " + FormatFullVersion() + "\n"; - if (args.IsArgSet("-version")) { + if (args.GetBoolArg("-version", false)) { strUsage += FormatParagraph(LicenseInfo()); } else { strUsage += "\n" diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index 09a35d4bad..5d5f06127d 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -135,10 +135,10 @@ static bool ParseArgs(NodeContext& node, int argc, char* argv[]) static bool ProcessInitCommands(ArgsManager& args) { // Process help and version before taking care about datadir - if (HelpRequested(args) || args.IsArgSet("-version")) { + if (HelpRequested(args) || args.GetBoolArg("-version", false)) { std::string strUsage = CLIENT_NAME " daemon version " + FormatFullVersion() + "\n"; - if (args.IsArgSet("-version")) { + if (args.GetBoolArg("-version", false)) { strUsage += FormatParagraph(LicenseInfo()); } else { strUsage += "\n" diff --git a/src/checkqueue.h b/src/checkqueue.h index a1de000714..934f672ae3 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -5,25 +5,31 @@ #ifndef BITCOIN_CHECKQUEUE_H #define BITCOIN_CHECKQUEUE_H +#include #include #include #include #include #include +#include #include /** * Queue for verifications that have to be performed. * The verifications are represented by a type T, which must provide an - * operator(), returning a bool. + * operator(), returning an std::optional. + * + * The overall result of the computation is std::nullopt if all invocations + * return std::nullopt, or one of the other results otherwise. * * One thread (the master) is assumed to push batches of verifications * onto the queue, where they are processed by N-1 worker threads. When * the master is done adding work, it temporarily joins the worker pool * as an N'th worker, until all jobs are done. + * */ -template +template ()().value())>> class CCheckQueue { private: @@ -47,7 +53,7 @@ class CCheckQueue int nTotal GUARDED_BY(m_mutex){0}; //! The temporary evaluation result. - bool fAllOk GUARDED_BY(m_mutex){true}; + std::optional m_result GUARDED_BY(m_mutex); /** * Number of verifications that haven't completed yet. @@ -62,24 +68,28 @@ class CCheckQueue std::vector m_worker_threads; bool m_request_stop GUARDED_BY(m_mutex){false}; - /** Internal function that does bulk of the verification work. */ - bool Loop(bool fMaster) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + /** Internal function that does bulk of the verification work. If fMaster, return the final result. */ + std::optional Loop(bool fMaster) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) { std::condition_variable& cond = fMaster ? m_master_cv : m_worker_cv; std::vector vChecks; vChecks.reserve(nBatchSize); unsigned int nNow = 0; - bool fOk = true; + std::optional local_result; + bool do_work; do { { WAIT_LOCK(m_mutex, lock); // first do the clean-up of the previous loop run (allowing us to do it in the same critsect) if (nNow) { - fAllOk &= fOk; + if (local_result.has_value() && !m_result.has_value()) { + std::swap(local_result, m_result); + } nTodo -= nNow; - if (nTodo == 0 && !fMaster) + if (nTodo == 0 && !fMaster) { // We processed the last element; inform the master it can exit and return the result m_master_cv.notify_one(); + } } else { // first iteration nTotal++; @@ -88,18 +98,19 @@ class CCheckQueue while (queue.empty() && !m_request_stop) { if (fMaster && nTodo == 0) { nTotal--; - bool fRet = fAllOk; + std::optional to_return = std::move(m_result); // reset the status for new work later - fAllOk = true; + m_result = std::nullopt; // return the current status - return fRet; + return to_return; } nIdle++; cond.wait(lock); // wait nIdle--; } if (m_request_stop) { - return false; + // return value does not matter, because m_request_stop is only set in the destructor. + return std::nullopt; } // Decide how many work units to process now. @@ -112,12 +123,15 @@ class CCheckQueue vChecks.assign(std::make_move_iterator(start_it), std::make_move_iterator(queue.end())); queue.erase(start_it, queue.end()); // Check whether we need to do work at all - fOk = fAllOk; + do_work = !m_result.has_value(); } // execute work - for (T& check : vChecks) - if (fOk) - fOk = check(); + if (do_work) { + for (T& check : vChecks) { + local_result = check(); + if (local_result.has_value()) break; + } + } vChecks.clear(); } while (true); } @@ -130,6 +144,7 @@ class CCheckQueue explicit CCheckQueue(unsigned int batch_size, int worker_threads_num) : nBatchSize(batch_size) { + LogInfo("Script verification uses %d additional threads", worker_threads_num); m_worker_threads.reserve(worker_threads_num); for (int n = 0; n < worker_threads_num; ++n) { m_worker_threads.emplace_back([this, n]() { @@ -146,8 +161,9 @@ class CCheckQueue CCheckQueue(CCheckQueue&&) = delete; CCheckQueue& operator=(CCheckQueue&&) = delete; - //! Wait until execution finishes, and return whether all evaluations were successful. - bool Wait() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + //! Join the execution until completion. If at least one evaluation wasn't successful, return + //! its error. + std::optional Complete() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) { return Loop(true /* master thread */); } @@ -188,11 +204,11 @@ class CCheckQueue * RAII-style controller object for a CCheckQueue that guarantees the passed * queue is finished before continuing. */ -template +template ()().value())>> class CCheckQueueControl { private: - CCheckQueue * const pqueue; + CCheckQueue * const pqueue; bool fDone; public: @@ -207,13 +223,12 @@ class CCheckQueueControl } } - bool Wait() + std::optional Complete() { - if (pqueue == nullptr) - return true; - bool fRet = pqueue->Wait(); + if (pqueue == nullptr) return std::nullopt; + auto ret = pqueue->Complete(); fDone = true; - return fRet; + return ret; } void Add(std::vector&& vChecks) @@ -226,7 +241,7 @@ class CCheckQueueControl ~CCheckQueueControl() { if (!fDone) - Wait(); + Complete(); if (pqueue != nullptr) { LEAVE_CRITICAL_SECTION(pqueue->m_control_mutex); } diff --git a/src/clientversion.cpp b/src/clientversion.cpp index 6fa3c700ee..af58c3023f 100644 --- a/src/clientversion.cpp +++ b/src/clientversion.cpp @@ -71,7 +71,7 @@ std::string FormatSubVersion(const std::string& name, int nClientVersion, const std::string CopyrightHolders(const std::string& strPrefix) { - const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION); + const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION).translated; std::string strCopyrightHolders = strPrefix + copyright_devs; // Make sure Bitcoin Core copyright is not removed by accident @@ -85,15 +85,17 @@ std::string LicenseInfo() { const std::string URL_SOURCE_CODE = ""; - return CopyrightHolders(strprintf(_("Copyright (C) %i-%i").translated, 2009, COPYRIGHT_YEAR) + " ") + "\n" + + return CopyrightHolders(strprintf(_("Copyright (C) %i-%i"), 2009, COPYRIGHT_YEAR).translated + " ") + "\n" + "\n" + strprintf(_("Please contribute if you find %s useful. " - "Visit %s for further information about the software.").translated, CLIENT_NAME, "<" CLIENT_URL ">") + + "Visit %s for further information about the software."), + CLIENT_NAME, "<" CLIENT_URL ">") + .translated + "\n" + - strprintf(_("The source code is available from %s.").translated, URL_SOURCE_CODE) + + strprintf(_("The source code is available from %s."), URL_SOURCE_CODE).translated + "\n" + "\n" + _("This is experimental software.").translated + "\n" + - strprintf(_("Distributed under the MIT software license, see the accompanying file %s or %s").translated, "COPYING", "") + + strprintf(_("Distributed under the MIT software license, see the accompanying file %s or %s"), "COPYING", "").translated + "\n"; } diff --git a/src/coins.cpp b/src/coins.cpp index 75d11b4f26..24a102b0bc 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -53,7 +53,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage(); if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins // The parent only has an empty entry for this outpoint; we can consider our version as fresh. - ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel); + CCoinsCacheEntry::SetFresh(*ret, m_sentinel); } } else { cacheCoins.erase(ret); @@ -99,7 +99,8 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi fresh = !it->second.IsDirty(); } it->second.coin = std::move(coin); - it->second.AddFlags(CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0), *it, m_sentinel); + CCoinsCacheEntry::SetDirty(*it, m_sentinel); + if (fresh) CCoinsCacheEntry::SetFresh(*it, m_sentinel); cachedCoinsUsage += it->second.coin.DynamicMemoryUsage(); TRACEPOINT(utxocache, add, outpoint.hash.data(), @@ -111,13 +112,8 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) { cachedCoinsUsage += coin.DynamicMemoryUsage(); - auto [it, inserted] = cacheCoins.emplace( - std::piecewise_construct, - std::forward_as_tuple(std::move(outpoint)), - std::forward_as_tuple(std::move(coin))); - if (inserted) { - it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel); - } + auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin)); + if (inserted) CCoinsCacheEntry::SetDirty(*it, m_sentinel); } void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) { @@ -147,7 +143,7 @@ bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { if (it->second.IsFresh()) { cacheCoins.erase(it); } else { - it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel); + CCoinsCacheEntry::SetDirty(*it, m_sentinel); it->second.coin.Clear(); } return true; @@ -207,13 +203,11 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha entry.coin = it->second.coin; } cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); - entry.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel); + CCoinsCacheEntry::SetDirty(*itUs, m_sentinel); // We can mark it FRESH in the parent if it was FRESH in the child // Otherwise it might have just been flushed from the parent's cache // and already exist in the grandparent - if (it->second.IsFresh()) { - entry.AddFlags(CCoinsCacheEntry::FRESH, *itUs, m_sentinel); - } + if (it->second.IsFresh()) CCoinsCacheEntry::SetFresh(*itUs, m_sentinel); } } else { // Found the entry in the parent cache @@ -241,7 +235,7 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha itUs->second.coin = it->second.coin; } cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage(); - itUs->second.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel); + CCoinsCacheEntry::SetDirty(*itUs, m_sentinel); // NOTE: It isn't safe to mark the coin as FRESH in the parent // cache. If it already existed and was spent in the parent // cache then marking it FRESH would prevent that spentness diff --git a/src/coins.h b/src/coins.h index a2449e1b81..61fb4af642 100644 --- a/src/coins.h +++ b/src/coins.h @@ -110,7 +110,7 @@ struct CCoinsCacheEntry private: /** * These are used to create a doubly linked list of flagged entries. - * They are set in AddFlags and unset in ClearFlags. + * They are set in SetDirty, SetFresh, and unset in SetClean. * A flagged entry is any entry that is either DIRTY, FRESH, or both. * * DIRTY entries are tracked so that only modified entries can be passed to @@ -128,6 +128,21 @@ struct CCoinsCacheEntry CoinsCachePair* m_next{nullptr}; uint8_t m_flags{0}; + //! Adding a flag requires a reference to the sentinel of the flagged pair linked list. + static void AddFlags(uint8_t flags, CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept + { + Assume(flags & (DIRTY | FRESH)); + if (!pair.second.m_flags) { + Assume(!pair.second.m_prev && !pair.second.m_next); + pair.second.m_prev = sentinel.second.m_prev; + pair.second.m_next = &sentinel; + sentinel.second.m_prev = &pair; + pair.second.m_prev->second.m_next = &pair; + } + Assume(pair.second.m_prev && pair.second.m_next); + pair.second.m_flags |= flags; + } + public: Coin coin; // The actual cached data. @@ -156,52 +171,43 @@ struct CCoinsCacheEntry explicit CCoinsCacheEntry(Coin&& coin_) noexcept : coin(std::move(coin_)) {} ~CCoinsCacheEntry() { - ClearFlags(); + SetClean(); } - //! Adding a flag also requires a self reference to the pair that contains - //! this entry in the CCoinsCache map and a reference to the sentinel of the - //! flagged pair linked list. - inline void AddFlags(uint8_t flags, CoinsCachePair& self, CoinsCachePair& sentinel) noexcept - { - Assume(&self.second == this); - if (!m_flags && flags) { - m_prev = sentinel.second.m_prev; - m_next = &sentinel; - sentinel.second.m_prev = &self; - m_prev->second.m_next = &self; - } - m_flags |= flags; - } - inline void ClearFlags() noexcept + static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(DIRTY, pair, sentinel); } + static void SetFresh(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(FRESH, pair, sentinel); } + + void SetClean() noexcept { if (!m_flags) return; m_next->second.m_prev = m_prev; m_prev->second.m_next = m_next; m_flags = 0; + m_prev = m_next = nullptr; } - inline uint8_t GetFlags() const noexcept { return m_flags; } - inline bool IsDirty() const noexcept { return m_flags & DIRTY; } - inline bool IsFresh() const noexcept { return m_flags & FRESH; } + bool IsDirty() const noexcept { return m_flags & DIRTY; } + bool IsFresh() const noexcept { return m_flags & FRESH; } //! Only call Next when this entry is DIRTY, FRESH, or both - inline CoinsCachePair* Next() const noexcept { + CoinsCachePair* Next() const noexcept + { Assume(m_flags); return m_next; } //! Only call Prev when this entry is DIRTY, FRESH, or both - inline CoinsCachePair* Prev() const noexcept { + CoinsCachePair* Prev() const noexcept + { Assume(m_flags); return m_prev; } //! Only use this for initializing the linked list sentinel - inline void SelfRef(CoinsCachePair& self) noexcept + void SelfRef(CoinsCachePair& pair) noexcept { - Assume(&self.second == this); - m_prev = &self; - m_next = &self; + Assume(&pair.second == this); + m_prev = &pair; + m_next = &pair; // Set sentinel to DIRTY so we can call Next on it m_flags = DIRTY; } @@ -279,13 +285,13 @@ struct CoinsViewCacheCursor { const auto next_entry{current.second.Next()}; // If we are not going to erase the cache, we must still erase spent entries. - // Otherwise clear the flags on the entry. + // Otherwise, clear the state of the entry. if (!m_will_erase) { if (current.second.coin.IsSpent()) { m_usage -= current.second.coin.DynamicMemoryUsage(); m_map.erase(current.first); } else { - current.second.ClearFlags(); + current.second.SetClean(); } } return next_entry; diff --git a/src/common/config.cpp b/src/common/config.cpp index 98223fc3e3..fac4aa314c 100644 --- a/src/common/config.cpp +++ b/src/common/config.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -128,12 +129,18 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) } const auto conf_path{GetConfigFilePath()}; - std::ifstream stream{conf_path}; - - // not ok to have a config file specified that cannot be opened - if (IsArgSet("-conf") && !stream.good()) { - error = strprintf("specified config file \"%s\" could not be opened.", fs::PathToString(conf_path)); - return false; + std::ifstream stream; + if (!conf_path.empty()) { // path is empty when -noconf is specified + if (fs::is_directory(conf_path)) { + error = strprintf("Config file \"%s\" is a directory.", fs::PathToString(conf_path)); + return false; + } + stream = std::ifstream{conf_path}; + // If the file is explicitly specified, it must be readable + if (IsArgSet("-conf") && !stream.good()) { + error = strprintf("specified config file \"%s\" could not be opened.", fs::PathToString(conf_path)); + return false; + } } // ok to not have a config file if (stream.good()) { @@ -175,7 +182,12 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) const size_t default_includes = add_includes({}); for (const std::string& conf_file_name : conf_file_names) { - std::ifstream conf_file_stream{AbsPathForConfigVal(*this, fs::PathFromString(conf_file_name), /*net_specific=*/false)}; + const auto include_conf_path{AbsPathForConfigVal(*this, fs::PathFromString(conf_file_name), /*net_specific=*/false)}; + if (fs::is_directory(include_conf_path)) { + error = strprintf("Included config file \"%s\" is a directory.", fs::PathToString(include_conf_path)); + return false; + } + std::ifstream conf_file_stream{include_conf_path}; if (conf_file_stream.good()) { if (!ReadConfigStream(conf_file_stream, conf_file_name, error, ignore_invalid_keys)) { return false; @@ -213,7 +225,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) fs::path AbsPathForConfigVal(const ArgsManager& args, const fs::path& path, bool net_specific) { - if (path.is_absolute()) { + if (path.is_absolute() || path.empty()) { return path; } return fsbridge::AbsPathJoin(net_specific ? args.GetDataDirNet() : args.GetDataDirBase(), path); diff --git a/src/common/init.cpp b/src/common/init.cpp index 412d73aec7..5a70440468 100644 --- a/src/common/init.cpp +++ b/src/common/init.cpp @@ -62,29 +62,36 @@ std::optional InitConfig(ArgsManager& args, SettingsAbortFn setting fs::create_directories(net_path / "wallets"); } - // Show an error or warning if there is a bitcoin.conf file in the + // Show an error or warn/log if there is a bitcoin.conf file in the // datadir that is being ignored. const fs::path base_config_path = base_path / BITCOIN_CONF_FILENAME; - if (fs::exists(base_config_path) && !fs::equivalent(orig_config_path, base_config_path)) { - const std::string cli_config_path = args.GetArg("-conf", ""); - const std::string config_source = cli_config_path.empty() - ? strprintf("data directory %s", fs::quoted(fs::PathToString(orig_datadir_path))) - : strprintf("command line argument %s", fs::quoted("-conf=" + cli_config_path)); - const std::string error = strprintf( - "Data directory %1$s contains a %2$s file which is ignored, because a different configuration file " - "%3$s from %4$s is being used instead. Possible ways to address this would be to:\n" - "- Delete or rename the %2$s file in data directory %1$s.\n" - "- Change datadir= or conf= options to specify one configuration file, not two, and use " - "includeconf= to include any other configuration files.\n" - "- Set allowignoredconf=1 option to treat this condition as a warning, not an error.", - fs::quoted(fs::PathToString(base_path)), - fs::quoted(BITCOIN_CONF_FILENAME), - fs::quoted(fs::PathToString(orig_config_path)), - config_source); - if (args.GetBoolArg("-allowignoredconf", false)) { - LogPrintf("Warning: %s\n", error); - } else { - return ConfigError{ConfigStatus::FAILED, Untranslated(error)}; + if (fs::exists(base_config_path)) { + if (orig_config_path.empty()) { + LogInfo( + "Data directory %s contains a %s file which is explicitly ignored using -noconf.", + fs::quoted(fs::PathToString(base_path)), + fs::quoted(BITCOIN_CONF_FILENAME)); + } else if (!fs::equivalent(orig_config_path, base_config_path)) { + const std::string cli_config_path = args.GetArg("-conf", ""); + const std::string config_source = cli_config_path.empty() + ? strprintf("data directory %s", fs::quoted(fs::PathToString(orig_datadir_path))) + : strprintf("command line argument %s", fs::quoted("-conf=" + cli_config_path)); + std::string error = strprintf( + "Data directory %1$s contains a %2$s file which is ignored, because a different configuration file " + "%3$s from %4$s is being used instead. Possible ways to address this would be to:\n" + "- Delete or rename the %2$s file in data directory %1$s.\n" + "- Change datadir= or conf= options to specify one configuration file, not two, and use " + "includeconf= to include any other configuration files.", + fs::quoted(fs::PathToString(base_path)), + fs::quoted(BITCOIN_CONF_FILENAME), + fs::quoted(fs::PathToString(orig_config_path)), + config_source); + if (args.GetBoolArg("-allowignoredconf", false)) { + LogWarning("%s", error); + } else { + error += "\n- Set allowignoredconf=1 option to treat this condition as a warning, not an error."; + return ConfigError{ConfigStatus::FAILED, Untranslated(error)}; + } } } diff --git a/src/config/.empty b/src/config/.empty deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/src/core_read.cpp b/src/core_read.cpp index 23f341c230..273dfb17e8 100644 --- a/src/core_read.cpp +++ b/src/core_read.cpp @@ -39,7 +39,7 @@ class OpCodeParser } mapOpNames[strName] = static_cast(op); // Convenience: OP_ADD and just ADD are both recognized: - if (strName.compare(0, 3, "OP_") == 0) { // strName starts with "OP_" + if (strName.starts_with("OP_")) { mapOpNames[strName.substr(3)] = static_cast(op); } } diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp index e0f153fd61..8fb366515a 100644 --- a/src/dbwrapper.cpp +++ b/src/dbwrapper.cpp @@ -147,6 +147,7 @@ static leveldb::Options GetOptions(size_t nCacheSize) // on corruption in later versions. options.paranoid_checks = true; } + options.max_file_size = std::max(options.max_file_size, DBWRAPPER_MAX_FILE_SIZE); SetMaxOpenFiles(&options); return options; } diff --git a/src/dbwrapper.h b/src/dbwrapper.h index 63c2f99d2a..dd5daa7a1f 100644 --- a/src/dbwrapper.h +++ b/src/dbwrapper.h @@ -22,6 +22,7 @@ static const size_t DBWRAPPER_PREALLOC_KEY_SIZE = 64; static const size_t DBWRAPPER_PREALLOC_VALUE_SIZE = 1024; +static const size_t DBWRAPPER_MAX_FILE_SIZE = 32 << 20; // 32 MiB //! User-controlled performance and debug options. struct DBOptions { diff --git a/src/httprpc.cpp b/src/httprpc.cpp index 69dd821dc0..5d906ffa0c 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -134,8 +134,6 @@ static bool multiUserAuthorized(std::string strUserPass) static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUsernameOut) { - if (strRPCUserColonPass.empty()) // Belt-and-suspenders measure if InitRPCAuthentication was not called - return false; if (strAuth.substr(0, 6) != "Basic ") return false; std::string_view strUserPass64 = TrimStringView(std::string_view{strAuth}.substr(6)); @@ -147,8 +145,9 @@ static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUserna if (strUserPass.find(':') != std::string::npos) strAuthUsernameOut = strUserPass.substr(0, strUserPass.find(':')); - //Check if authorized under single-user field - if (TimingResistantEqual(strUserPass, strRPCUserColonPass)) { + // Check if authorized under single-user field. + // (strRPCUserColonPass is empty when -norpccookiefile is specified). + if (!strRPCUserColonPass.empty() && TimingResistantEqual(strUserPass, strRPCUserColonPass)) { return true; } return multiUserAuthorized(strUserPass); @@ -294,22 +293,26 @@ static bool InitRPCAuthentication() { if (gArgs.GetArg("-rpcpassword", "") == "") { - LogInfo("Using random cookie authentication.\n"); - std::optional cookie_perms{std::nullopt}; auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")}; if (cookie_perms_arg) { auto perm_opt = InterpretPermString(*cookie_perms_arg); if (!perm_opt) { - LogInfo("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.\n", *cookie_perms_arg); + LogError("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.", *cookie_perms_arg); return false; } cookie_perms = *perm_opt; } + assert(strRPCUserColonPass.empty()); // Only support initializing once if (!GenerateAuthCookie(&strRPCUserColonPass, cookie_perms)) { return false; } + if (strRPCUserColonPass.empty()) { + LogInfo("RPC authentication cookie file generation is disabled."); + } else { + LogInfo("Using random cookie authentication."); + } } else { LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth auth generation.\n"); strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", ""); diff --git a/src/httpserver.cpp b/src/httpserver.cpp index e37bc21dea..88e640c377 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -227,7 +227,7 @@ static bool InitHTTPAllowList() const CSubNet subnet{LookupSubNet(strAllow)}; if (!subnet.IsValid()) { uiInterface.ThreadSafeMessageBox( - strprintf(Untranslated("Invalid -rpcallowip subnet specification: %s. Valid are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24)."), strAllow), + Untranslated(strprintf("Invalid -rpcallowip subnet specification: %s. Valid are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24).", strAllow)), "", CClientUIInterface::MSG_ERROR); return false; } diff --git a/src/index/base.cpp b/src/index/base.cpp index 1a7eb9cd5e..a8f9073d9f 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -106,7 +106,7 @@ bool BaseIndex::Init() // best chain, we will rewind to the fork point during index sync const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.at(0))}; if (!locator_index) { - return InitError(strprintf(Untranslated("%s: best block of the index not found. Please rebuild the index."), GetName())); + return InitError(Untranslated(strprintf("%s: best block of the index not found. Please rebuild the index.", GetName()))); } SetBestBlockIndex(locator_index); } diff --git a/src/init.cpp b/src/init.cpp index f79ebe881d..a4ad2771cc 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -175,6 +175,8 @@ static fs::path GetPidFile(const ArgsManager& args) [[nodiscard]] static bool CreatePidFile(const ArgsManager& args) { + if (args.IsArgNegated("-pid")) return true; + std::ofstream file{GetPidFile(args)}; if (file) { #ifdef WIN32 @@ -483,7 +485,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) argsman.AddArg("-blocksonly", strprintf("Whether to reject transactions from network peers. Disables automatic broadcast and rebroadcast of transactions, unless the source peer has the 'forcerelay' permission. RPC transactions are not affected. (default: %u)", DEFAULT_BLOCKSONLY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-coinstatsindex", strprintf("Maintain coinstats index used by the gettxoutsetinfo RPC (default: %u)", DEFAULT_COINSTATSINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-conf=", strprintf("Specify path to read-only configuration file. Relative paths will be prefixed by datadir location (only useable from command line, not configuration file) (default: %s)", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); - argsman.AddArg("-datadir=", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-datadir=", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS); argsman.AddArg("-dbcache=", strprintf("Maximum database cache size MiB (minimum %d, default: %d). Make sure you have enough RAM. In addition, unused memory allocated to the mempool is shared with this cache (see -maxmempool).", nMinDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-includeconf=", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); @@ -884,7 +886,7 @@ bool AppInitParameterInteraction(const ArgsManager& args) } bilingual_str errors; for (const auto& arg : args.GetUnsuitableSectionOnlyArgs()) { - errors += strprintf(_("Config setting for %s only applied on %s network when in [%s] section.") + Untranslated("\n"), arg, ChainTypeToString(chain), ChainTypeToString(chain)); + errors += strprintf(_("Config setting for %s only applied on %s network when in [%s] section."), arg, ChainTypeToString(chain), ChainTypeToString(chain)) + Untranslated("\n"); } if (!errors.empty()) { @@ -899,7 +901,7 @@ bool AppInitParameterInteraction(const ArgsManager& args) // Warn if unrecognized section name are present in the config file. bilingual_str warnings; for (const auto& section : args.GetUnrecognizedSections()) { - warnings += strprintf(Untranslated("%s:%i ") + _("Section [%s] is not recognized.") + Untranslated("\n"), section.m_file, section.m_line, section.m_name); + warnings += Untranslated(strprintf("%s:%i ", section.m_file, section.m_line)) + strprintf(_("Section [%s] is not recognized."), section.m_name) + Untranslated("\n"); } if (!warnings.empty()) { @@ -1206,7 +1208,7 @@ static ChainstateLoadResult InitAndLoadChainstate( try { node.chainman = std::make_unique(*Assert(node.shutdown_signal), chainman_opts, blockman_opts); } catch (std::exception& e) { - return {ChainstateLoadStatus::FAILURE_FATAL, strprintf(Untranslated("Failed to initialize ChainstateManager: %s"), e.what())}; + return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated(strprintf("Failed to initialize ChainstateManager: %s", e.what()))}; } ChainstateManager& chainman = *node.chainman; // This is defined and set here instead of inline in validation.h to avoid a hard @@ -1337,7 +1339,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) try { ipc->listenAddress(address); } catch (const std::exception& e) { - return InitError(strprintf(Untranslated("Unable to bind to IPC address '%s'. %s"), address, e.what())); + return InitError(Untranslated(strprintf("Unable to bind to IPC address '%s'. %s", address, e.what()))); } LogPrintf("Listening for IPC requests on address %s\n", address); } @@ -2042,7 +2044,7 @@ bool StartIndexBackgroundSync(NodeContext& node) const CBlockIndex* start_block = *indexes_start_block; if (!start_block) start_block = chainman.ActiveChain().Genesis(); if (!chainman.m_blockman.CheckBlockDataAvailability(*index_chain.Tip(), *Assert(start_block))) { - return InitError(strprintf(Untranslated("%s best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)"), older_index_name)); + return InitError(Untranslated(strprintf("%s best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)", older_index_name))); } } diff --git a/src/init/common.cpp b/src/init/common.cpp index 8d0b008fb5..70c4230cfd 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include @@ -111,8 +112,8 @@ bool StartLogging(const ArgsManager& args) } } if (!LogInstance().StartLogging()) { - return InitError(strprintf(Untranslated("Could not open debug log file %s"), - fs::PathToString(LogInstance().m_file_path))); + return InitError(Untranslated(strprintf("Could not open debug log file %s", + fs::PathToString(LogInstance().m_file_path)))); } if (!LogInstance().m_log_timestamps) @@ -122,10 +123,13 @@ bool StartLogging(const ArgsManager& args) // Only log conf file usage message if conf file actually exists. fs::path config_file_path = args.GetConfigFilePath(); - if (fs::exists(config_file_path)) { + if (args.IsArgNegated("-conf")) { + LogInfo("Config file: "); + } else if (fs::is_directory(config_file_path)) { + LogWarning("Config file: %s (is directory, not file)", fs::PathToString(config_file_path)); + } else if (fs::exists(config_file_path)) { LogPrintf("Config file: %s\n", fs::PathToString(config_file_path)); } else if (args.IsArgSet("-conf")) { - // Warn if no conf file exists at path provided by user InitWarning(strprintf(_("The specified config file %s does not exist"), fs::PathToString(config_file_path))); } else { // Not categorizing as "Warning" because it's the default behavior diff --git a/src/net.cpp b/src/net.cpp index 0f2e7e23d9..802dc947be 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3058,14 +3058,14 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError, socklen_t len = sizeof(sockaddr); if (!addrBind.GetSockAddr((struct sockaddr*)&sockaddr, &len)) { - strError = strprintf(Untranslated("Bind address family for %s not supported"), addrBind.ToStringAddrPort()); + strError = Untranslated(strprintf("Bind address family for %s not supported", addrBind.ToStringAddrPort())); LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); return false; } std::unique_ptr sock = CreateSock(addrBind.GetSAFamily(), SOCK_STREAM, IPPROTO_TCP); if (!sock) { - strError = strprintf(Untranslated("Couldn't open socket for incoming connections (socket returned error %s)"), NetworkErrorString(WSAGetLastError())); + strError = Untranslated(strprintf("Couldn't open socket for incoming connections (socket returned error %s)", NetworkErrorString(WSAGetLastError()))); LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); return false; } @@ -3073,7 +3073,7 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError, // Allow binding if the port is still in TIME_WAIT state after // the program was closed and restarted. if (sock->SetSockOpt(SOL_SOCKET, SO_REUSEADDR, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) { - strError = strprintf(Untranslated("Error setting SO_REUSEADDR on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError())); + strError = Untranslated(strprintf("Error setting SO_REUSEADDR on socket: %s, continuing anyway", NetworkErrorString(WSAGetLastError()))); LogPrintf("%s\n", strError.original); } @@ -3082,14 +3082,14 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError, if (addrBind.IsIPv6()) { #ifdef IPV6_V6ONLY if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_V6ONLY, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) { - strError = strprintf(Untranslated("Error setting IPV6_V6ONLY on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError())); + strError = Untranslated(strprintf("Error setting IPV6_V6ONLY on socket: %s, continuing anyway", NetworkErrorString(WSAGetLastError()))); LogPrintf("%s\n", strError.original); } #endif #ifdef WIN32 int nProtLevel = PROTECTION_LEVEL_UNRESTRICTED; if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_PROTECTION_LEVEL, (const char*)&nProtLevel, sizeof(int)) == SOCKET_ERROR) { - strError = strprintf(Untranslated("Error setting IPV6_PROTECTION_LEVEL on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError())); + strError = Untranslated(strprintf("Error setting IPV6_PROTECTION_LEVEL on socket: %s, continuing anyway", NetworkErrorString(WSAGetLastError()))); LogPrintf("%s\n", strError.original); } #endif diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp index b86d0b2991..0ac96c5514 100644 --- a/src/node/chainstatemanager_args.cpp +++ b/src/node/chainstatemanager_args.cpp @@ -35,7 +35,7 @@ util::Result ApplyArgsManOptions(const ArgsManager& args, ChainstateManage if (auto min_work{uint256::FromUserHex(*value)}) { opts.minimum_chain_work = UintToArith256(*min_work); } else { - return util::Error{strprintf(Untranslated("Invalid minimum work specified (%s), must be up to %d hex digits"), *value, uint256::size() * 2)}; + return util::Error{Untranslated(strprintf("Invalid minimum work specified (%s), must be up to %d hex digits", *value, uint256::size() * 2))}; } } @@ -43,7 +43,7 @@ util::Result ApplyArgsManOptions(const ArgsManager& args, ChainstateManage if (auto block_hash{uint256::FromUserHex(*value)}) { opts.assumed_valid_block = *block_hash; } else { - return util::Error{strprintf(Untranslated("Invalid assumevalid block hash specified (%s), must be up to %d hex digits (or 0 to disable)"), *value, uint256::size() * 2)}; + return util::Error{Untranslated(strprintf("Invalid assumevalid block hash specified (%s), must be up to %d hex digits (or 0 to disable)", *value, uint256::size() * 2))}; } } @@ -60,8 +60,7 @@ util::Result ApplyArgsManOptions(const ArgsManager& args, ChainstateManage script_threads += GetNumCores(); } // Subtract 1 because the main thread counts towards the par threads. - opts.worker_threads_num = std::clamp(script_threads - 1, 0, MAX_SCRIPTCHECK_THREADS); - LogPrintf("Script verification uses %d additional threads\n", opts.worker_threads_num); + opts.worker_threads_num = script_threads - 1; if (auto max_size = args.GetIntArg("-maxsigcachesize")) { // 1. When supplied with a max_size of 0, both the signature cache and diff --git a/src/node/chainstatemanager_args.h b/src/node/chainstatemanager_args.h index b2cdba68b8..af13aa8d3c 100644 --- a/src/node/chainstatemanager_args.h +++ b/src/node/chainstatemanager_args.h @@ -10,8 +10,6 @@ class ArgsManager; -/** Maximum number of dedicated script-checking threads allowed */ -static constexpr int MAX_SCRIPTCHECK_THREADS{15}; /** -par default (number of script-checking threads, 0 = auto) */ static constexpr int DEFAULT_SCRIPTCHECK_THREADS{0}; diff --git a/src/node/interface_ui.cpp b/src/node/interface_ui.cpp index 4f4d240d1b..c9dcd7a7fb 100644 --- a/src/node/interface_ui.cpp +++ b/src/node/interface_ui.cpp @@ -74,7 +74,7 @@ bool InitError(const bilingual_str& str, const std::vector& details // functions which provide error details are ones that run during early init // before the GUI uiInterface is registered, so there's no point passing // main messages and details separately to uiInterface yet. - return InitError(details.empty() ? str : strprintf(Untranslated("%s:\n%s"), str, MakeUnorderedList(details))); + return InitError(details.empty() ? str : str + Untranslated(strprintf(":\n%s", MakeUnorderedList(details)))); } void InitWarning(const bilingual_str& str) diff --git a/src/node/mempool_args.cpp b/src/node/mempool_args.cpp index 0b6725ee1b..e3bf5fb57c 100644 --- a/src/node/mempool_args.cpp +++ b/src/node/mempool_args.cpp @@ -89,7 +89,7 @@ util::Result ApplyArgsManOptions(const ArgsManager& argsman, const CChainP mempool_opts.require_standard = !argsman.GetBoolArg("-acceptnonstdtxn", DEFAULT_ACCEPT_NON_STD_TXN); if (!chainparams.IsTestChain() && !mempool_opts.require_standard) { - return util::Error{strprintf(Untranslated("acceptnonstdtxn is not currently supported for %s chain"), chainparams.GetChainTypeString())}; + return util::Error{Untranslated(strprintf("acceptnonstdtxn is not currently supported for %s chain", chainparams.GetChainTypeString()))}; } mempool_opts.persist_v1_dat = argsman.GetBoolArg("-persistmempoolv1", mempool_opts.persist_v1_dat); diff --git a/src/qt/CMakeLists.txt b/src/qt/CMakeLists.txt index e8e3a146fa..9f660264f9 100644 --- a/src/qt/CMakeLists.txt +++ b/src/qt/CMakeLists.txt @@ -35,6 +35,7 @@ endfunction() # - https://doc.qt.io/qt-5/cmake-manual.html set(CMAKE_AUTOMOC ON) +set(CMAKE_AUTOMOC_MOC_OPTIONS "-p${CMAKE_CURRENT_SOURCE_DIR}") set(CMAKE_AUTORCC ON) set(CMAKE_AUTOUIC ON) set(CMAKE_AUTOUIC_SEARCH_PATHS forms) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 5c1ae895b1..eaaa9fc6fd 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -529,7 +529,7 @@ int GuiMain(int argc, char* argv[]) SetupUIArgs(gArgs); std::string error; if (!gArgs.ParseParameters(argc, argv, error)) { - InitError(strprintf(Untranslated("Error parsing command line arguments: %s"), error)); + InitError(Untranslated(strprintf("Error parsing command line arguments: %s", error))); // Create a message box, because the gui has neither been created nor has subscribed to core signals QMessageBox::critical(nullptr, CLIENT_NAME, // message cannot be translated because translations have not been initialized @@ -582,8 +582,8 @@ int GuiMain(int argc, char* argv[]) // Show help message immediately after parsing command-line options (for "-lang") and setting locale, // but before showing splash screen. - if (HelpRequested(gArgs) || gArgs.IsArgSet("-version")) { - HelpMessageDialog help(nullptr, gArgs.IsArgSet("-version")); + if (HelpRequested(gArgs) || gArgs.GetBoolArg("-version", false)) { + HelpMessageDialog help(nullptr, gArgs.GetBoolArg("-version", false)); help.showOrPrint(); return EXIT_SUCCESS; } diff --git a/src/qt/test/CMakeLists.txt b/src/qt/test/CMakeLists.txt index 8ce571c150..e1e617661b 100644 --- a/src/qt/test/CMakeLists.txt +++ b/src/qt/test/CMakeLists.txt @@ -2,6 +2,8 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or https://opensource.org/license/mit/. +set(CMAKE_AUTOMOC_MOC_OPTIONS "-p${CMAKE_CURRENT_SOURCE_DIR}") + add_executable(test_bitcoin-qt apptests.cpp optiontests.cpp diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 74f2249c07..6d462a7e85 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -926,7 +926,7 @@ static RPCHelpMan submitpackage() , { {"package", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of raw transactions.\n" - "The package must solely consist of a child and its parents. None of the parents may depend on each other.\n" + "The package must solely consist of a child transaction and all of its unconfirmed parents, if any. None of the parents may depend on each other.\n" "The package must be topologically sorted, with the child being the last element in the array.", { {"rawtx", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, ""}, @@ -968,15 +968,15 @@ static RPCHelpMan submitpackage() }, }, RPCExamples{ - HelpExampleRpc("submitpackage", R"(["rawtx1", "rawtx2"])") + - HelpExampleCli("submitpackage", R"('["rawtx1", "rawtx2"]')") + HelpExampleRpc("submitpackage", R"(["raw-parent-tx-1", "raw-parent-tx-2", "raw-child-tx"])") + + HelpExampleCli("submitpackage", R"('["raw-tx-without-unconfirmed-parents"]')") }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { const UniValue raw_transactions = request.params[0].get_array(); - if (raw_transactions.size() < 2 || raw_transactions.size() > MAX_PACKAGE_COUNT) { + if (raw_transactions.empty() || raw_transactions.size() > MAX_PACKAGE_COUNT) { throw JSONRPCError(RPC_INVALID_PARAMETER, - "Array must contain between 2 and " + ToString(MAX_PACKAGE_COUNT) + " transactions."); + "Array must contain between 1 and " + ToString(MAX_PACKAGE_COUNT) + " transactions."); } // Fee check needs to be run with chainstate and package context @@ -1007,7 +1007,8 @@ static RPCHelpMan submitpackage() txns.emplace_back(MakeTransactionRef(std::move(mtx))); } - if (!IsChildWithParentsTree(txns)) { + CHECK_NONFATAL(!txns.empty()); + if (txns.size() > 1 && !IsChildWithParentsTree(txns)) { throw JSONRPCTransactionError(TransactionError::INVALID_PACKAGE, "package topology disallowed. not child-with-parents or parents depend on each other."); } diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 386e686628..6bec4cd3b4 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -1027,25 +1027,7 @@ static RPCHelpMan submitblock() throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block decode failed"); } - if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block does not start with a coinbase"); - } - ChainstateManager& chainman = EnsureAnyChainman(request.context); - uint256 hash = block.GetHash(); - { - LOCK(cs_main); - const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash); - if (pindex) { - if (pindex->IsValid(BLOCK_VALID_SCRIPTS)) { - return "duplicate"; - } - if (pindex->nStatus & BLOCK_FAILED_MASK) { - return "duplicate-invalid"; - } - } - } - { LOCK(cs_main); const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock); diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp index afd98f8875..c0ae94bc4f 100644 --- a/src/rpc/request.cpp +++ b/src/rpc/request.cpp @@ -86,6 +86,9 @@ static const char* const COOKIEAUTH_FILE = ".cookie"; static fs::path GetAuthCookieFile(bool temp=false) { fs::path arg = gArgs.GetPathArg("-rpccookiefile", COOKIEAUTH_FILE); + if (arg.empty()) { + return {}; // -norpccookiefile was specified + } if (temp) { arg += ".tmp"; } @@ -106,9 +109,12 @@ bool GenerateAuthCookie(std::string* cookie_out, std::optional cookie */ std::ofstream file; fs::path filepath_tmp = GetAuthCookieFile(true); + if (filepath_tmp.empty()) { + return true; // -norpccookiefile + } file.open(filepath_tmp); if (!file.is_open()) { - LogInfo("Unable to open cookie authentication file %s for writing\n", fs::PathToString(filepath_tmp)); + LogWarning("Unable to open cookie authentication file %s for writing", fs::PathToString(filepath_tmp)); return false; } file << cookie; @@ -116,14 +122,14 @@ bool GenerateAuthCookie(std::string* cookie_out, std::optional cookie fs::path filepath = GetAuthCookieFile(false); if (!RenameOver(filepath_tmp, filepath)) { - LogInfo("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath)); + LogWarning("Unable to rename cookie authentication file %s to %s", fs::PathToString(filepath_tmp), fs::PathToString(filepath)); return false; } if (cookie_perms) { std::error_code code; fs::permissions(filepath, cookie_perms.value(), fs::perm_options::replace, code); if (code) { - LogInfo("Unable to set permissions on cookie authentication file %s\n", fs::PathToString(filepath_tmp)); + LogWarning("Unable to set permissions on cookie authentication file %s", fs::PathToString(filepath)); return false; } } @@ -142,6 +148,9 @@ bool GetAuthCookie(std::string *cookie_out) std::ifstream file; std::string cookie; fs::path filepath = GetAuthCookieFile(); + if (filepath.empty()) { + return true; // -norpccookiefile + } file.open(filepath); if (!file.is_open()) return false; diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index c4f58ebecf..ddb1d5b43f 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -448,10 +448,21 @@ BOOST_AUTO_TEST_CASE(getaddr_unfiltered) CNetAddr source = ResolveIP("250.1.2.1"); BOOST_CHECK(addrman->Add({addr1, addr2}, source)); - // Filtered GetAddr should only return addr1 + // Set time on this addr so isTerrible = false + CAddress addr3 = CAddress(ResolveService("250.251.2.3", 9998), NODE_NONE); + addr3.nTime = Now(); + addrman->Good(addr3, /*time=*/Now()); + BOOST_CHECK(addrman->Add({addr3}, source)); + // The time is set, but after ADDRMAN_RETRIES unsuccessful attempts not + // retried in the last minute, this addr should be isTerrible = true + for (size_t i = 0; i < 3; ++i) { + addrman->Attempt(addr3, /*fCountFailure=*/true, /*time=*/Now() - 61s); + } + + // GetAddr filtered by quality (i.e. not IsTerrible) should only return addr1 BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt).size(), 1U); - // Unfiltered GetAddr should return addr1 and addr2 - BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt, /*filtered=*/false).size(), 2U); + // Unfiltered GetAddr should return all addrs + BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt, /*filtered=*/false).size(), 3U); } BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket_legacy) diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index b7014db4a5..2463ce6da5 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -42,28 +42,26 @@ static const unsigned int QUEUE_BATCH_SIZE = 128; static const int SCRIPT_CHECK_THREADS = 3; struct FakeCheck { - bool operator()() const + std::optional operator()() const { - return true; + return std::nullopt; } }; struct FakeCheckCheckCompletion { static std::atomic n_calls; - bool operator()() + std::optional operator()() { n_calls.fetch_add(1, std::memory_order_relaxed); - return true; + return std::nullopt; } }; -struct FailingCheck { - bool fails; - FailingCheck(bool _fails) : fails(_fails){}; - bool operator()() const - { - return !fails; - } +struct FixedCheck +{ + std::optional m_result; + FixedCheck(std::optional result) : m_result(result){}; + std::optional operator()() const { return m_result; } }; struct UniqueCheck { @@ -71,11 +69,11 @@ struct UniqueCheck { static std::unordered_multiset results GUARDED_BY(m); size_t check_id; UniqueCheck(size_t check_id_in) : check_id(check_id_in){}; - bool operator()() + std::optional operator()() { LOCK(m); results.insert(check_id); - return true; + return std::nullopt; } }; @@ -83,9 +81,9 @@ struct UniqueCheck { struct MemoryCheck { static std::atomic fake_allocated_memory; bool b {false}; - bool operator()() const + std::optional operator()() const { - return true; + return std::nullopt; } MemoryCheck(const MemoryCheck& x) { @@ -110,9 +108,9 @@ struct FrozenCleanupCheck { static std::condition_variable cv; static std::mutex m; bool should_freeze{true}; - bool operator()() const + std::optional operator()() const { - return true; + return std::nullopt; } FrozenCleanupCheck() = default; ~FrozenCleanupCheck() @@ -149,7 +147,7 @@ std::atomic MemoryCheck::fake_allocated_memory{0}; // Queue Typedefs typedef CCheckQueue Correct_Queue; typedef CCheckQueue Standard_Queue; -typedef CCheckQueue Failing_Queue; +typedef CCheckQueue Fixed_Queue; typedef CCheckQueue Unique_Queue; typedef CCheckQueue Memory_Queue; typedef CCheckQueue FrozenCleanup_Queue; @@ -174,7 +172,7 @@ void CheckQueueTest::Correct_Queue_range(std::vector range) total -= vChecks.size(); control.Add(std::move(vChecks)); } - BOOST_REQUIRE(control.Wait()); + BOOST_REQUIRE(!control.Complete().has_value()); BOOST_REQUIRE_EQUAL(FakeCheckCheckCompletion::n_calls, i); } } @@ -217,27 +215,27 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Correct_Random) } -/** Test that failing checks are caught */ +/** Test that distinct failing checks are caught */ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure) { - auto fail_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); + auto fixed_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); for (size_t i = 0; i < 1001; ++i) { - CCheckQueueControl control(fail_queue.get()); + CCheckQueueControl control(fixed_queue.get()); size_t remaining = i; while (remaining) { size_t r = m_rng.randrange(10); - std::vector vChecks; + std::vector vChecks; vChecks.reserve(r); for (size_t k = 0; k < r && remaining; k++, remaining--) - vChecks.emplace_back(remaining == 1); + vChecks.emplace_back(remaining == 1 ? std::make_optional(17 * i) : std::nullopt); control.Add(std::move(vChecks)); } - bool success = control.Wait(); + auto result = control.Complete(); if (i > 0) { - BOOST_REQUIRE(!success); - } else if (i == 0) { - BOOST_REQUIRE(success); + BOOST_REQUIRE(result.has_value() && *result == static_cast(17 * i)); + } else { + BOOST_REQUIRE(!result.has_value()); } } } @@ -245,17 +243,17 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure) // future blocks, ie, the bad state is cleared. BOOST_AUTO_TEST_CASE(test_CheckQueue_Recovers_From_Failure) { - auto fail_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); + auto fail_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); for (auto times = 0; times < 10; ++times) { for (const bool end_fails : {true, false}) { - CCheckQueueControl control(fail_queue.get()); + CCheckQueueControl control(fail_queue.get()); { - std::vector vChecks; - vChecks.resize(100, false); - vChecks[99] = end_fails; + std::vector vChecks; + vChecks.resize(100, FixedCheck(std::nullopt)); + vChecks[99] = FixedCheck(end_fails ? std::make_optional(2) : std::nullopt); control.Add(std::move(vChecks)); } - bool r =control.Wait(); + bool r = !control.Complete().has_value(); BOOST_REQUIRE(r != end_fails); } } @@ -329,8 +327,8 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup) CCheckQueueControl control(queue.get()); std::vector vChecks(1); control.Add(std::move(vChecks)); - bool waitResult = control.Wait(); // Hangs here - assert(waitResult); + auto result = control.Complete(); // Hangs here + assert(!result); }); { std::unique_lock l(FrozenCleanupCheck::m); diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index ec4720966b..c46144b34b 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -15,6 +15,8 @@ #include #include +#include +#include #include #include @@ -559,21 +561,58 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization) } const static COutPoint OUTPOINT; -const static CAmount SPENT = -1; -const static CAmount ABSENT = -2; -const static CAmount FAIL = -3; -const static CAmount VALUE1 = 100; -const static CAmount VALUE2 = 200; -const static CAmount VALUE3 = 300; -const static char DIRTY = CCoinsCacheEntry::DIRTY; -const static char FRESH = CCoinsCacheEntry::FRESH; -const static char NO_ENTRY = -1; - -const static auto FLAGS = {char(0), FRESH, DIRTY, char(DIRTY | FRESH)}; -const static auto CLEAN_FLAGS = {char(0), FRESH}; -const static auto ABSENT_FLAGS = {NO_ENTRY}; - -static void SetCoinsValue(CAmount value, Coin& coin) +constexpr CAmount SPENT {-1}; +constexpr CAmount ABSENT{-2}; +constexpr CAmount VALUE1{100}; +constexpr CAmount VALUE2{200}; +constexpr CAmount VALUE3{300}; + +struct CoinEntry { + enum class State { CLEAN, DIRTY, FRESH, DIRTY_FRESH }; + + const CAmount value; + const State state; + + constexpr CoinEntry(const CAmount v, const State s) : value{v}, state{s} {} + + bool operator==(const CoinEntry& o) const = default; + friend std::ostream& operator<<(std::ostream& os, const CoinEntry& e) { return os << e.value << ", " << e.state; } + + constexpr bool IsDirtyFresh() const { return state == State::DIRTY_FRESH; } + constexpr bool IsDirty() const { return state == State::DIRTY || IsDirtyFresh(); } + constexpr bool IsFresh() const { return state == State::FRESH || IsDirtyFresh(); } + + static constexpr State ToState(const bool is_dirty, const bool is_fresh) { + if (is_dirty && is_fresh) return State::DIRTY_FRESH; + if (is_dirty) return State::DIRTY; + if (is_fresh) return State::FRESH; + return State::CLEAN; + } +}; + +using MaybeCoin = std::optional; +using CoinOrError = std::variant; + +constexpr MaybeCoin MISSING {std::nullopt}; +constexpr MaybeCoin SPENT_DIRTY {{SPENT, CoinEntry::State::DIRTY}}; +constexpr MaybeCoin SPENT_DIRTY_FRESH {{SPENT, CoinEntry::State::DIRTY_FRESH}}; +constexpr MaybeCoin SPENT_FRESH {{SPENT, CoinEntry::State::FRESH}}; +constexpr MaybeCoin SPENT_CLEAN {{SPENT, CoinEntry::State::CLEAN}}; +constexpr MaybeCoin VALUE1_DIRTY {{VALUE1, CoinEntry::State::DIRTY}}; +constexpr MaybeCoin VALUE1_DIRTY_FRESH{{VALUE1, CoinEntry::State::DIRTY_FRESH}}; +constexpr MaybeCoin VALUE1_FRESH {{VALUE1, CoinEntry::State::FRESH}}; +constexpr MaybeCoin VALUE1_CLEAN {{VALUE1, CoinEntry::State::CLEAN}}; +constexpr MaybeCoin VALUE2_DIRTY {{VALUE2, CoinEntry::State::DIRTY}}; +constexpr MaybeCoin VALUE2_DIRTY_FRESH{{VALUE2, CoinEntry::State::DIRTY_FRESH}}; +constexpr MaybeCoin VALUE2_FRESH {{VALUE2, CoinEntry::State::FRESH}}; +constexpr MaybeCoin VALUE2_CLEAN {{VALUE2, CoinEntry::State::CLEAN}}; +constexpr MaybeCoin VALUE3_DIRTY {{VALUE3, CoinEntry::State::DIRTY}}; +constexpr MaybeCoin VALUE3_DIRTY_FRESH{{VALUE3, CoinEntry::State::DIRTY_FRESH}}; + +constexpr auto EX_OVERWRITE_UNSPENT{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}; +constexpr auto EX_FRESH_MISAPPLIED {"FRESH flag misapplied to coin that exists in parent cache"}; + +static void SetCoinsValue(const CAmount value, Coin& coin) { assert(value != ABSENT); coin.Clear(); @@ -585,45 +624,34 @@ static void SetCoinsValue(CAmount value, Coin& coin) } } -static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, CAmount value, char flags) +static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, const CoinEntry& cache_coin) { - if (value == ABSENT) { - assert(flags == NO_ENTRY); - return 0; - } - assert(flags != NO_ENTRY); CCoinsCacheEntry entry; - SetCoinsValue(value, entry.coin); - auto inserted = map.emplace(OUTPOINT, std::move(entry)); - assert(inserted.second); - inserted.first->second.AddFlags(flags, *inserted.first, sentinel); - return inserted.first->second.coin.DynamicMemoryUsage(); + SetCoinsValue(cache_coin.value, entry.coin); + auto [iter, inserted] = map.emplace(OUTPOINT, std::move(entry)); + assert(inserted); + if (cache_coin.IsDirty()) CCoinsCacheEntry::SetDirty(*iter, sentinel); + if (cache_coin.IsFresh()) CCoinsCacheEntry::SetFresh(*iter, sentinel); + return iter->second.coin.DynamicMemoryUsage(); } -void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags, const COutPoint& outp = OUTPOINT) +static MaybeCoin GetCoinsMapEntry(const CCoinsMap& map, const COutPoint& outp = OUTPOINT) { - auto it = map.find(outp); - if (it == map.end()) { - value = ABSENT; - flags = NO_ENTRY; - } else { - if (it->second.coin.IsSpent()) { - value = SPENT; - } else { - value = it->second.coin.out.nValue; - } - flags = it->second.GetFlags(); - assert(flags != NO_ENTRY); + if (auto it{map.find(outp)}; it != map.end()) { + return CoinEntry{ + it->second.coin.IsSpent() ? SPENT : it->second.coin.out.nValue, + CoinEntry::ToState(it->second.IsDirty(), it->second.IsFresh())}; } + return MISSING; } -void WriteCoinsViewEntry(CCoinsView& view, CAmount value, char flags) +static void WriteCoinsViewEntry(CCoinsView& view, const MaybeCoin& cache_coin) { CoinsCachePair sentinel{}; sentinel.second.SelfRef(sentinel); CCoinsMapMemoryResource resource; CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; - auto usage{InsertCoinsMapEntry(map, sentinel, value, flags)}; + auto usage{cache_coin ? InsertCoinsMapEntry(map, sentinel, *cache_coin) : 0}; auto cursor{CoinsViewCacheCursor(usage, sentinel, map, /*will_erase=*/true)}; BOOST_CHECK(view.BatchWrite(cursor, {})); } @@ -631,10 +659,11 @@ void WriteCoinsViewEntry(CCoinsView& view, CAmount value, char flags) class SingleEntryCacheTest { public: - SingleEntryCacheTest(CAmount base_value, CAmount cache_value, char cache_flags) + SingleEntryCacheTest(const CAmount base_value, const MaybeCoin& cache_coin) { - WriteCoinsViewEntry(base, base_value, base_value == ABSENT ? NO_ENTRY : DIRTY); - cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), cache_value, cache_flags); + auto base_cache_coin{base_value == ABSENT ? MISSING : CoinEntry{base_value, CoinEntry::State::DIRTY}}; + WriteCoinsViewEntry(base, base_cache_coin); + if (cache_coin) cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), *cache_coin); } CCoinsView root; @@ -642,17 +671,13 @@ class SingleEntryCacheTest CCoinsViewCacheTest cache{&base}; }; -static void CheckAccessCoin(CAmount base_value, CAmount cache_value, CAmount expected_value, char cache_flags, char expected_flags) +static void CheckAccessCoin(const CAmount base_value, const MaybeCoin& cache_coin, const MaybeCoin& expected) { - SingleEntryCacheTest test(base_value, cache_value, cache_flags); - test.cache.AccessCoin(OUTPOINT); + SingleEntryCacheTest test{base_value, cache_coin}; + auto& coin = test.cache.AccessCoin(OUTPOINT); + BOOST_CHECK_EQUAL(coin.IsSpent(), !test.cache.GetCoin(OUTPOINT)); test.cache.SelfTest(/*sanity_check=*/false); - - CAmount result_value; - char result_flags; - GetCoinsMapEntry(test.cache.map(), result_value, result_flags); - BOOST_CHECK_EQUAL(result_value, expected_value); - BOOST_CHECK_EQUAL(result_flags, expected_flags); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); } BOOST_AUTO_TEST_CASE(ccoins_access) @@ -660,121 +685,65 @@ BOOST_AUTO_TEST_CASE(ccoins_access) /* Check AccessCoin behavior, requesting a coin from a cache view layered on * top of a base view, and checking the resulting entry in the cache after * the access. - * - * Base Cache Result Cache Result - * Value Value Value Flags Flags + * Base Cache Expected */ - CheckAccessCoin(ABSENT, ABSENT, ABSENT, NO_ENTRY , NO_ENTRY ); - CheckAccessCoin(ABSENT, SPENT , SPENT , 0 , 0 ); - CheckAccessCoin(ABSENT, SPENT , SPENT , FRESH , FRESH ); - CheckAccessCoin(ABSENT, SPENT , SPENT , DIRTY , DIRTY ); - CheckAccessCoin(ABSENT, SPENT , SPENT , DIRTY|FRESH, DIRTY|FRESH); - CheckAccessCoin(ABSENT, VALUE2, VALUE2, 0 , 0 ); - CheckAccessCoin(ABSENT, VALUE2, VALUE2, FRESH , FRESH ); - CheckAccessCoin(ABSENT, VALUE2, VALUE2, DIRTY , DIRTY ); - CheckAccessCoin(ABSENT, VALUE2, VALUE2, DIRTY|FRESH, DIRTY|FRESH); - CheckAccessCoin(SPENT , ABSENT, ABSENT, NO_ENTRY , NO_ENTRY ); - CheckAccessCoin(SPENT , SPENT , SPENT , 0 , 0 ); - CheckAccessCoin(SPENT , SPENT , SPENT , FRESH , FRESH ); - CheckAccessCoin(SPENT , SPENT , SPENT , DIRTY , DIRTY ); - CheckAccessCoin(SPENT , SPENT , SPENT , DIRTY|FRESH, DIRTY|FRESH); - CheckAccessCoin(SPENT , VALUE2, VALUE2, 0 , 0 ); - CheckAccessCoin(SPENT , VALUE2, VALUE2, FRESH , FRESH ); - CheckAccessCoin(SPENT , VALUE2, VALUE2, DIRTY , DIRTY ); - CheckAccessCoin(SPENT , VALUE2, VALUE2, DIRTY|FRESH, DIRTY|FRESH); - CheckAccessCoin(VALUE1, ABSENT, VALUE1, NO_ENTRY , 0 ); - CheckAccessCoin(VALUE1, SPENT , SPENT , 0 , 0 ); - CheckAccessCoin(VALUE1, SPENT , SPENT , FRESH , FRESH ); - CheckAccessCoin(VALUE1, SPENT , SPENT , DIRTY , DIRTY ); - CheckAccessCoin(VALUE1, SPENT , SPENT , DIRTY|FRESH, DIRTY|FRESH); - CheckAccessCoin(VALUE1, VALUE2, VALUE2, 0 , 0 ); - CheckAccessCoin(VALUE1, VALUE2, VALUE2, FRESH , FRESH ); - CheckAccessCoin(VALUE1, VALUE2, VALUE2, DIRTY , DIRTY ); - CheckAccessCoin(VALUE1, VALUE2, VALUE2, DIRTY|FRESH, DIRTY|FRESH); + for (auto base_value : {ABSENT, SPENT, VALUE1}) { + CheckAccessCoin(base_value, MISSING, base_value == VALUE1 ? VALUE1_CLEAN : MISSING); + + CheckAccessCoin(base_value, SPENT_CLEAN, SPENT_CLEAN ); + CheckAccessCoin(base_value, SPENT_FRESH, SPENT_FRESH ); + CheckAccessCoin(base_value, SPENT_DIRTY, SPENT_DIRTY ); + CheckAccessCoin(base_value, SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH ); + + CheckAccessCoin(base_value, VALUE2_CLEAN, VALUE2_CLEAN ); + CheckAccessCoin(base_value, VALUE2_FRESH, VALUE2_FRESH ); + CheckAccessCoin(base_value, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckAccessCoin(base_value, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); + } } -static void CheckSpendCoins(CAmount base_value, CAmount cache_value, CAmount expected_value, char cache_flags, char expected_flags) +static void CheckSpendCoins(const CAmount base_value, const MaybeCoin& cache_coin, const MaybeCoin& expected) { - SingleEntryCacheTest test(base_value, cache_value, cache_flags); + SingleEntryCacheTest test{base_value, cache_coin}; test.cache.SpendCoin(OUTPOINT); test.cache.SelfTest(); - - CAmount result_value; - char result_flags; - GetCoinsMapEntry(test.cache.map(), result_value, result_flags); - BOOST_CHECK_EQUAL(result_value, expected_value); - BOOST_CHECK_EQUAL(result_flags, expected_flags); -}; + BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); +} BOOST_AUTO_TEST_CASE(ccoins_spend) { /* Check SpendCoin behavior, requesting a coin from a cache view layered on * top of a base view, spending, and then checking * the resulting entry in the cache after the modification. - * - * Base Cache Result Cache Result - * Value Value Value Flags Flags + * Base Cache Expected */ - CheckSpendCoins(ABSENT, ABSENT, ABSENT, NO_ENTRY , NO_ENTRY ); - CheckSpendCoins(ABSENT, SPENT , SPENT , 0 , DIRTY ); - CheckSpendCoins(ABSENT, SPENT , ABSENT, FRESH , NO_ENTRY ); - CheckSpendCoins(ABSENT, SPENT , SPENT , DIRTY , DIRTY ); - CheckSpendCoins(ABSENT, SPENT , ABSENT, DIRTY|FRESH, NO_ENTRY ); - CheckSpendCoins(ABSENT, VALUE2, SPENT , 0 , DIRTY ); - CheckSpendCoins(ABSENT, VALUE2, ABSENT, FRESH , NO_ENTRY ); - CheckSpendCoins(ABSENT, VALUE2, SPENT , DIRTY , DIRTY ); - CheckSpendCoins(ABSENT, VALUE2, ABSENT, DIRTY|FRESH, NO_ENTRY ); - CheckSpendCoins(SPENT , ABSENT, ABSENT, NO_ENTRY , NO_ENTRY ); - CheckSpendCoins(SPENT , SPENT , SPENT , 0 , DIRTY ); - CheckSpendCoins(SPENT , SPENT , ABSENT, FRESH , NO_ENTRY ); - CheckSpendCoins(SPENT , SPENT , SPENT , DIRTY , DIRTY ); - CheckSpendCoins(SPENT , SPENT , ABSENT, DIRTY|FRESH, NO_ENTRY ); - CheckSpendCoins(SPENT , VALUE2, SPENT , 0 , DIRTY ); - CheckSpendCoins(SPENT , VALUE2, ABSENT, FRESH , NO_ENTRY ); - CheckSpendCoins(SPENT , VALUE2, SPENT , DIRTY , DIRTY ); - CheckSpendCoins(SPENT , VALUE2, ABSENT, DIRTY|FRESH, NO_ENTRY ); - CheckSpendCoins(VALUE1, ABSENT, SPENT , NO_ENTRY , DIRTY ); - CheckSpendCoins(VALUE1, SPENT , SPENT , 0 , DIRTY ); - CheckSpendCoins(VALUE1, SPENT , ABSENT, FRESH , NO_ENTRY ); - CheckSpendCoins(VALUE1, SPENT , SPENT , DIRTY , DIRTY ); - CheckSpendCoins(VALUE1, SPENT , ABSENT, DIRTY|FRESH, NO_ENTRY ); - CheckSpendCoins(VALUE1, VALUE2, SPENT , 0 , DIRTY ); - CheckSpendCoins(VALUE1, VALUE2, ABSENT, FRESH , NO_ENTRY ); - CheckSpendCoins(VALUE1, VALUE2, SPENT , DIRTY , DIRTY ); - CheckSpendCoins(VALUE1, VALUE2, ABSENT, DIRTY|FRESH, NO_ENTRY ); + for (auto base_value : {ABSENT, SPENT, VALUE1}) { + CheckSpendCoins(base_value, MISSING, base_value == VALUE1 ? SPENT_DIRTY : MISSING); + + CheckSpendCoins(base_value, SPENT_CLEAN, SPENT_DIRTY); + CheckSpendCoins(base_value, SPENT_FRESH, MISSING ); + CheckSpendCoins(base_value, SPENT_DIRTY, SPENT_DIRTY); + CheckSpendCoins(base_value, SPENT_DIRTY_FRESH, MISSING ); + + CheckSpendCoins(base_value, VALUE2_CLEAN, SPENT_DIRTY); + CheckSpendCoins(base_value, VALUE2_FRESH, MISSING ); + CheckSpendCoins(base_value, VALUE2_DIRTY, SPENT_DIRTY); + CheckSpendCoins(base_value, VALUE2_DIRTY_FRESH, MISSING ); + } } -static void CheckAddCoinBase(CAmount base_value, CAmount cache_value, CAmount modify_value, CAmount expected_value, char cache_flags, char expected_flags, bool coinbase) +static void CheckAddCoin(const CAmount base_value, const MaybeCoin& cache_coin, const CAmount modify_value, const CoinOrError& expected, const bool coinbase) { - SingleEntryCacheTest test(base_value, cache_value, cache_flags); - - CAmount result_value; - char result_flags; - try { - CTxOut output; - output.nValue = modify_value; - test.cache.AddCoin(OUTPOINT, Coin(std::move(output), 1, coinbase), coinbase); + SingleEntryCacheTest test{base_value, cache_coin}; + bool possible_overwrite{coinbase}; + auto add_coin{[&] { test.cache.AddCoin(OUTPOINT, Coin{CTxOut{modify_value, CScript{}}, 1, coinbase}, possible_overwrite); }}; + if (auto* expected_coin{std::get_if(&expected)}) { + add_coin(); test.cache.SelfTest(); - GetCoinsMapEntry(test.cache.map(), result_value, result_flags); - } catch (std::logic_error&) { - result_value = FAIL; - result_flags = NO_ENTRY; + BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), *expected_coin); + } else { + BOOST_CHECK_EXCEPTION(add_coin(), std::logic_error, HasReason(std::get(expected))); } - - BOOST_CHECK_EQUAL(result_value, expected_value); - BOOST_CHECK_EQUAL(result_flags, expected_flags); -} - -// Simple wrapper for CheckAddCoinBase function above that loops through -// different possible base_values, making sure each one gives the same results. -// This wrapper lets the coins_add test below be shorter and less repetitive, -// while still verifying that the CoinsViewCache::AddCoin implementation -// ignores base values. -template -static void CheckAddCoin(Args&&... args) -{ - for (const CAmount base_value : {ABSENT, SPENT, VALUE1}) - CheckAddCoinBase(base_value, std::forward(args)...); } BOOST_AUTO_TEST_CASE(ccoins_add) @@ -782,48 +751,44 @@ BOOST_AUTO_TEST_CASE(ccoins_add) /* Check AddCoin behavior, requesting a new coin from a cache view, * writing a modification to the coin, and then checking the resulting * entry in the cache after the modification. Verify behavior with the - * AddCoin possible_overwrite argument set to false, and to true. - * - * Cache Write Result Cache Result possible_overwrite - * Value Value Value Flags Flags + * AddCoin coinbase argument set to false, and to true. + * Base Cache Write Expected Coinbase */ - CheckAddCoin(ABSENT, VALUE3, VALUE3, NO_ENTRY , DIRTY|FRESH, false); - CheckAddCoin(ABSENT, VALUE3, VALUE3, NO_ENTRY , DIRTY , true ); - CheckAddCoin(SPENT , VALUE3, VALUE3, 0 , DIRTY|FRESH, false); - CheckAddCoin(SPENT , VALUE3, VALUE3, 0 , DIRTY , true ); - CheckAddCoin(SPENT , VALUE3, VALUE3, FRESH , DIRTY|FRESH, false); - CheckAddCoin(SPENT , VALUE3, VALUE3, FRESH , DIRTY|FRESH, true ); - CheckAddCoin(SPENT , VALUE3, VALUE3, DIRTY , DIRTY , false); - CheckAddCoin(SPENT , VALUE3, VALUE3, DIRTY , DIRTY , true ); - CheckAddCoin(SPENT , VALUE3, VALUE3, DIRTY|FRESH, DIRTY|FRESH, false); - CheckAddCoin(SPENT , VALUE3, VALUE3, DIRTY|FRESH, DIRTY|FRESH, true ); - CheckAddCoin(VALUE2, VALUE3, FAIL , 0 , NO_ENTRY , false); - CheckAddCoin(VALUE2, VALUE3, VALUE3, 0 , DIRTY , true ); - CheckAddCoin(VALUE2, VALUE3, FAIL , FRESH , NO_ENTRY , false); - CheckAddCoin(VALUE2, VALUE3, VALUE3, FRESH , DIRTY|FRESH, true ); - CheckAddCoin(VALUE2, VALUE3, FAIL , DIRTY , NO_ENTRY , false); - CheckAddCoin(VALUE2, VALUE3, VALUE3, DIRTY , DIRTY , true ); - CheckAddCoin(VALUE2, VALUE3, FAIL , DIRTY|FRESH, NO_ENTRY , false); - CheckAddCoin(VALUE2, VALUE3, VALUE3, DIRTY|FRESH, DIRTY|FRESH, true ); + for (auto base_value : {ABSENT, SPENT, VALUE1}) { + CheckAddCoin(base_value, MISSING, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, MISSING, VALUE3, VALUE3_DIRTY, true ); + + CheckAddCoin(base_value, SPENT_CLEAN, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, SPENT_CLEAN, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, SPENT_FRESH, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, SPENT_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); + CheckAddCoin(base_value, SPENT_DIRTY, VALUE3, VALUE3_DIRTY, false); + CheckAddCoin(base_value, SPENT_DIRTY, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, SPENT_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, SPENT_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); + + CheckAddCoin(base_value, VALUE2_CLEAN, VALUE3, EX_OVERWRITE_UNSPENT, false); + CheckAddCoin(base_value, VALUE2_CLEAN, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, VALUE2_FRESH, VALUE3, EX_OVERWRITE_UNSPENT, false); + CheckAddCoin(base_value, VALUE2_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); + CheckAddCoin(base_value, VALUE2_DIRTY, VALUE3, EX_OVERWRITE_UNSPENT, false); + CheckAddCoin(base_value, VALUE2_DIRTY, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, VALUE2_DIRTY_FRESH, VALUE3, EX_OVERWRITE_UNSPENT, false); + CheckAddCoin(base_value, VALUE2_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); + } } -void CheckWriteCoins(CAmount parent_value, CAmount child_value, CAmount expected_value, char parent_flags, char child_flags, char expected_flags) +static void CheckWriteCoins(const MaybeCoin& parent, const MaybeCoin& child, const CoinOrError& expected) { - SingleEntryCacheTest test(ABSENT, parent_value, parent_flags); - - CAmount result_value; - char result_flags; - try { - WriteCoinsViewEntry(test.cache, child_value, child_flags); + SingleEntryCacheTest test{ABSENT, parent}; + auto write_coins{[&] { WriteCoinsViewEntry(test.cache, child); }}; + if (auto* expected_coin{std::get_if(&expected)}) { + write_coins(); test.cache.SelfTest(/*sanity_check=*/false); - GetCoinsMapEntry(test.cache.map(), result_value, result_flags); - } catch (std::logic_error&) { - result_value = FAIL; - result_flags = NO_ENTRY; + BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), *expected_coin); + } else { + BOOST_CHECK_EXCEPTION(write_coins(), std::logic_error, HasReason(std::get(expected))); } - - BOOST_CHECK_EQUAL(result_value, expected_value); - BOOST_CHECK_EQUAL(result_flags, expected_flags); } BOOST_AUTO_TEST_CASE(ccoins_write) @@ -831,68 +796,74 @@ BOOST_AUTO_TEST_CASE(ccoins_write) /* Check BatchWrite behavior, flushing one entry from a child cache to a * parent cache, and checking the resulting entry in the parent cache * after the write. - * - * Parent Child Result Parent Child Result - * Value Value Value Flags Flags Flags + * Parent Child Expected */ - CheckWriteCoins(ABSENT, ABSENT, ABSENT, NO_ENTRY , NO_ENTRY , NO_ENTRY ); - CheckWriteCoins(ABSENT, SPENT , SPENT , NO_ENTRY , DIRTY , DIRTY ); - CheckWriteCoins(ABSENT, SPENT , ABSENT, NO_ENTRY , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(ABSENT, VALUE2, VALUE2, NO_ENTRY , DIRTY , DIRTY ); - CheckWriteCoins(ABSENT, VALUE2, VALUE2, NO_ENTRY , DIRTY|FRESH, DIRTY|FRESH); - CheckWriteCoins(SPENT , ABSENT, SPENT , 0 , NO_ENTRY , 0 ); - CheckWriteCoins(SPENT , ABSENT, SPENT , FRESH , NO_ENTRY , FRESH ); - CheckWriteCoins(SPENT , ABSENT, SPENT , DIRTY , NO_ENTRY , DIRTY ); - CheckWriteCoins(SPENT , ABSENT, SPENT , DIRTY|FRESH, NO_ENTRY , DIRTY|FRESH); - CheckWriteCoins(SPENT , SPENT , SPENT , 0 , DIRTY , DIRTY ); - CheckWriteCoins(SPENT , SPENT , SPENT , 0 , DIRTY|FRESH, DIRTY ); - CheckWriteCoins(SPENT , SPENT , ABSENT, FRESH , DIRTY , NO_ENTRY ); - CheckWriteCoins(SPENT , SPENT , ABSENT, FRESH , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(SPENT , SPENT , SPENT , DIRTY , DIRTY , DIRTY ); - CheckWriteCoins(SPENT , SPENT , SPENT , DIRTY , DIRTY|FRESH, DIRTY ); - CheckWriteCoins(SPENT , SPENT , ABSENT, DIRTY|FRESH, DIRTY , NO_ENTRY ); - CheckWriteCoins(SPENT , SPENT , ABSENT, DIRTY|FRESH, DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(SPENT , VALUE2, VALUE2, 0 , DIRTY , DIRTY ); - CheckWriteCoins(SPENT , VALUE2, VALUE2, 0 , DIRTY|FRESH, DIRTY ); - CheckWriteCoins(SPENT , VALUE2, VALUE2, FRESH , DIRTY , DIRTY|FRESH); - CheckWriteCoins(SPENT , VALUE2, VALUE2, FRESH , DIRTY|FRESH, DIRTY|FRESH); - CheckWriteCoins(SPENT , VALUE2, VALUE2, DIRTY , DIRTY , DIRTY ); - CheckWriteCoins(SPENT , VALUE2, VALUE2, DIRTY , DIRTY|FRESH, DIRTY ); - CheckWriteCoins(SPENT , VALUE2, VALUE2, DIRTY|FRESH, DIRTY , DIRTY|FRESH); - CheckWriteCoins(SPENT , VALUE2, VALUE2, DIRTY|FRESH, DIRTY|FRESH, DIRTY|FRESH); - CheckWriteCoins(VALUE1, ABSENT, VALUE1, 0 , NO_ENTRY , 0 ); - CheckWriteCoins(VALUE1, ABSENT, VALUE1, FRESH , NO_ENTRY , FRESH ); - CheckWriteCoins(VALUE1, ABSENT, VALUE1, DIRTY , NO_ENTRY , DIRTY ); - CheckWriteCoins(VALUE1, ABSENT, VALUE1, DIRTY|FRESH, NO_ENTRY , DIRTY|FRESH); - CheckWriteCoins(VALUE1, SPENT , SPENT , 0 , DIRTY , DIRTY ); - CheckWriteCoins(VALUE1, SPENT , FAIL , 0 , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, SPENT , ABSENT, FRESH , DIRTY , NO_ENTRY ); - CheckWriteCoins(VALUE1, SPENT , FAIL , FRESH , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, SPENT , SPENT , DIRTY , DIRTY , DIRTY ); - CheckWriteCoins(VALUE1, SPENT , FAIL , DIRTY , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, SPENT , ABSENT, DIRTY|FRESH, DIRTY , NO_ENTRY ); - CheckWriteCoins(VALUE1, SPENT , FAIL , DIRTY|FRESH, DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, VALUE2, VALUE2, 0 , DIRTY , DIRTY ); - CheckWriteCoins(VALUE1, VALUE2, FAIL , 0 , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, VALUE2, VALUE2, FRESH , DIRTY , DIRTY|FRESH); - CheckWriteCoins(VALUE1, VALUE2, FAIL , FRESH , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, VALUE2, VALUE2, DIRTY , DIRTY , DIRTY ); - CheckWriteCoins(VALUE1, VALUE2, FAIL , DIRTY , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, VALUE2, VALUE2, DIRTY|FRESH, DIRTY , DIRTY|FRESH); - CheckWriteCoins(VALUE1, VALUE2, FAIL , DIRTY|FRESH, DIRTY|FRESH, NO_ENTRY ); - - // The checks above omit cases where the child flags are not DIRTY, since + CheckWriteCoins(MISSING, MISSING, MISSING ); + CheckWriteCoins(MISSING, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(MISSING, SPENT_DIRTY_FRESH, MISSING ); + CheckWriteCoins(MISSING, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(MISSING, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH ); + CheckWriteCoins(SPENT_CLEAN, MISSING, SPENT_CLEAN ); + CheckWriteCoins(SPENT_FRESH, MISSING, SPENT_FRESH ); + CheckWriteCoins(SPENT_DIRTY, MISSING, SPENT_DIRTY ); + CheckWriteCoins(SPENT_DIRTY_FRESH, MISSING, SPENT_DIRTY_FRESH ); + + CheckWriteCoins(SPENT_CLEAN, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(SPENT_CLEAN, SPENT_DIRTY_FRESH, SPENT_DIRTY ); + CheckWriteCoins(SPENT_FRESH, SPENT_DIRTY, MISSING ); + CheckWriteCoins(SPENT_FRESH, SPENT_DIRTY_FRESH, MISSING ); + CheckWriteCoins(SPENT_DIRTY, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(SPENT_DIRTY, SPENT_DIRTY_FRESH, SPENT_DIRTY ); + CheckWriteCoins(SPENT_DIRTY_FRESH, SPENT_DIRTY, MISSING ); + CheckWriteCoins(SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH, MISSING ); + + CheckWriteCoins(SPENT_CLEAN, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(SPENT_CLEAN, VALUE2_DIRTY_FRESH, VALUE2_DIRTY ); + CheckWriteCoins(SPENT_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH ); + CheckWriteCoins(SPENT_FRESH, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH ); + CheckWriteCoins(SPENT_DIRTY, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(SPENT_DIRTY, VALUE2_DIRTY_FRESH, VALUE2_DIRTY ); + CheckWriteCoins(SPENT_DIRTY_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH ); + CheckWriteCoins(SPENT_DIRTY_FRESH, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH ); + + CheckWriteCoins(VALUE1_CLEAN, MISSING, VALUE1_CLEAN ); + CheckWriteCoins(VALUE1_FRESH, MISSING, VALUE1_FRESH ); + CheckWriteCoins(VALUE1_DIRTY, MISSING, VALUE1_DIRTY ); + CheckWriteCoins(VALUE1_DIRTY_FRESH, MISSING, VALUE1_DIRTY_FRESH ); + CheckWriteCoins(VALUE1_CLEAN, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(VALUE1_CLEAN, SPENT_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_FRESH, SPENT_DIRTY, MISSING ); + CheckWriteCoins(VALUE1_FRESH, SPENT_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_DIRTY, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(VALUE1_DIRTY, SPENT_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_DIRTY_FRESH, SPENT_DIRTY, MISSING ); + CheckWriteCoins(VALUE1_DIRTY_FRESH, SPENT_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + + CheckWriteCoins(VALUE1_CLEAN, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(VALUE1_CLEAN, VALUE2_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH ); + CheckWriteCoins(VALUE1_FRESH, VALUE2_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_DIRTY, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(VALUE1_DIRTY, VALUE2_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_DIRTY_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH ); + CheckWriteCoins(VALUE1_DIRTY_FRESH, VALUE2_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + + // The checks above omit cases where the child state is not DIRTY, since // they would be too repetitive (the parent cache is never updated in these // cases). The loop below covers these cases and makes sure the parent cache // is always left unchanged. - for (const CAmount parent_value : {ABSENT, SPENT, VALUE1}) - for (const CAmount child_value : {ABSENT, SPENT, VALUE2}) - for (const char parent_flags : parent_value == ABSENT ? ABSENT_FLAGS : FLAGS) - for (const char child_flags : child_value == ABSENT ? ABSENT_FLAGS : CLEAN_FLAGS) - CheckWriteCoins(parent_value, child_value, parent_value, parent_flags, child_flags, parent_flags); + for (const MaybeCoin& parent : {MISSING, + SPENT_CLEAN, SPENT_DIRTY, SPENT_FRESH, SPENT_DIRTY_FRESH, + VALUE1_CLEAN, VALUE1_DIRTY, VALUE1_FRESH, VALUE1_DIRTY_FRESH}) { + for (const MaybeCoin& child : {MISSING, + SPENT_CLEAN, SPENT_FRESH, + VALUE2_CLEAN, VALUE2_FRESH}) { + auto expected{CoinOrError{parent}}; // TODO test failure cases as well + CheckWriteCoins(parent, child, expected); + } + } } - struct FlushTest : BasicTestingSetup { Coin MakeCoin() { @@ -920,8 +891,6 @@ void TestFlushBehavior( std::vector>& all_caches, bool do_erasing_flush) { - CAmount value; - char flags; size_t cache_usage; size_t cache_size; @@ -955,9 +924,7 @@ void TestFlushBehavior( BOOST_CHECK(!base.HaveCoin(outp)); BOOST_CHECK(view->HaveCoin(outp)); - GetCoinsMapEntry(view->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, coin.out.nValue); - BOOST_CHECK_EQUAL(flags, DIRTY|FRESH); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CoinEntry::State::DIRTY_FRESH)); // --- 2. Flushing all caches (without erasing) // @@ -969,9 +936,7 @@ void TestFlushBehavior( // --- 3. Ensuring the entry still exists in the cache and has been written to parent // - GetCoinsMapEntry(view->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, coin.out.nValue); - BOOST_CHECK_EQUAL(flags, 0); // Flags should have been wiped. + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CoinEntry::State::CLEAN)); // State should have been wiped. // Both views should now have the coin. BOOST_CHECK(base.HaveCoin(outp)); @@ -989,14 +954,9 @@ void TestFlushBehavior( // --- 5. Ensuring the entry is no longer in the cache // - GetCoinsMapEntry(view->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, ABSENT); - BOOST_CHECK_EQUAL(flags, NO_ENTRY); - + BOOST_CHECK(!GetCoinsMapEntry(view->map(), outp)); view->AccessCoin(outp); - GetCoinsMapEntry(view->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, coin.out.nValue); - BOOST_CHECK_EQUAL(flags, 0); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CoinEntry::State::CLEAN)); } // Can't overwrite an entry without specifying that an overwrite is @@ -1010,9 +970,7 @@ void TestFlushBehavior( BOOST_CHECK(view->SpendCoin(outp)); // The coin should be in the cache, but spent and marked dirty. - GetCoinsMapEntry(view->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, SPENT); - BOOST_CHECK_EQUAL(flags, DIRTY); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), SPENT_DIRTY); BOOST_CHECK(!view->HaveCoin(outp)); // Coin should be considered spent in `view`. BOOST_CHECK(base.HaveCoin(outp)); // But coin should still be unspent in `base`. @@ -1063,10 +1021,7 @@ void TestFlushBehavior( all_caches[0]->AddCoin(outp, std::move(coin), false); // Coin should be FRESH in the cache. - GetCoinsMapEntry(all_caches[0]->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, coin_val); - BOOST_CHECK_EQUAL(flags, DIRTY|FRESH); - + BOOST_CHECK_EQUAL(GetCoinsMapEntry(all_caches[0]->map(), outp), CoinEntry(coin_val, CoinEntry::State::DIRTY_FRESH)); // Base shouldn't have seen coin. BOOST_CHECK(!base.HaveCoin(outp)); @@ -1074,9 +1029,7 @@ void TestFlushBehavior( all_caches[0]->Sync(); // Ensure there is no sign of the coin after spend/flush. - GetCoinsMapEntry(all_caches[0]->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, ABSENT); - BOOST_CHECK_EQUAL(flags, NO_ENTRY); + BOOST_CHECK(!GetCoinsMapEntry(all_caches[0]->map(), outp)); BOOST_CHECK(!all_caches[0]->HaveCoinInCache(outp)); BOOST_CHECK(!base.HaveCoin(outp)); } diff --git a/src/test/coinscachepair_tests.cpp b/src/test/coinscachepair_tests.cpp index 61840f1f09..0c208e93df 100644 --- a/src/test/coinscachepair_tests.cpp +++ b/src/test/coinscachepair_tests.cpp @@ -19,9 +19,9 @@ std::list CreatePairs(CoinsCachePair& sentinel) nodes.emplace_back(); auto node{std::prev(nodes.end())}; - node->second.AddFlags(CCoinsCacheEntry::DIRTY, *node, sentinel); + CCoinsCacheEntry::SetDirty(*node, sentinel); - BOOST_CHECK_EQUAL(node->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK(node->second.IsDirty() && !node->second.IsFresh()); BOOST_CHECK_EQUAL(node->second.Next(), &sentinel); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*node)); @@ -48,12 +48,12 @@ BOOST_AUTO_TEST_CASE(linked_list_iteration) BOOST_CHECK_EQUAL(node, &sentinel); // Check iterating through pairs is identical to iterating through a list - // Clear the flags during iteration + // Clear the state during iteration node = sentinel.second.Next(); for (const auto& expected : nodes) { BOOST_CHECK_EQUAL(&expected, node); auto next = node->second.Next(); - node->second.ClearFlags(); + node->second.SetClean(); node = next; } BOOST_CHECK_EQUAL(node, &sentinel); @@ -63,7 +63,7 @@ BOOST_AUTO_TEST_CASE(linked_list_iteration) // Delete the nodes from the list to make sure there are no dangling pointers for (auto it{nodes.begin()}; it != nodes.end(); it = nodes.erase(it)) { - BOOST_CHECK_EQUAL(it->second.GetFlags(), 0); + BOOST_CHECK(!it->second.IsDirty() && !it->second.IsFresh()); } } @@ -74,8 +74,8 @@ BOOST_AUTO_TEST_CASE(linked_list_iterate_erase) auto nodes{CreatePairs(sentinel)}; // Check iterating through pairs is identical to iterating through a list - // Erase the nodes as we iterate through, but don't clear flags - // The flags will be cleared by the CCoinsCacheEntry's destructor + // Erase the nodes as we iterate through, but don't clear state + // The state will be cleared by the CCoinsCacheEntry's destructor auto node{sentinel.second.Next()}; for (auto expected{nodes.begin()}; expected != nodes.end(); expected = nodes.erase(expected)) { BOOST_CHECK_EQUAL(&(*expected), node); @@ -104,10 +104,10 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion) // sentinel->n1->n3->n4->sentinel nodes.erase(n2); // Check that n1 now points to n3, and n3 still points to n4 - // Also check that flags were not altered - BOOST_CHECK_EQUAL(n1->second.GetFlags(), CCoinsCacheEntry::DIRTY); + // Also check that state was not altered + BOOST_CHECK(n1->second.IsDirty() && !n1->second.IsFresh()); BOOST_CHECK_EQUAL(n1->second.Next(), &(*n3)); - BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK(n3->second.IsDirty() && !n3->second.IsFresh()); BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4)); BOOST_CHECK_EQUAL(n3->second.Prev(), &(*n1)); @@ -115,8 +115,8 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion) // sentinel->n3->n4->sentinel nodes.erase(n1); // Check that sentinel now points to n3, and n3 still points to n4 - // Also check that flags were not altered - BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY); + // Also check that state was not altered + BOOST_CHECK(n3->second.IsDirty() && !n3->second.IsFresh()); BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3)); BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4)); BOOST_CHECK_EQUAL(n3->second.Prev(), &sentinel); @@ -125,8 +125,8 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion) // sentinel->n3->sentinel nodes.erase(n4); // Check that sentinel still points to n3, and n3 points to sentinel - // Also check that flags were not altered - BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY); + // Also check that state was not altered + BOOST_CHECK(n3->second.IsDirty() && !n3->second.IsFresh()); BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3)); BOOST_CHECK_EQUAL(n3->second.Next(), &sentinel); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*n3)); @@ -139,77 +139,56 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion) BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel); } -BOOST_AUTO_TEST_CASE(linked_list_add_flags) +BOOST_AUTO_TEST_CASE(linked_list_set_state) { CoinsCachePair sentinel; sentinel.second.SelfRef(sentinel); CoinsCachePair n1; CoinsCachePair n2; - // Check that adding 0 flag has no effect - n1.second.AddFlags(0, n1, sentinel); - BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); - BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel); - BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel); - - // Check that adding DIRTY flag inserts it into linked list and sets flags - n1.second.AddFlags(CCoinsCacheEntry::DIRTY, n1, sentinel); - BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); + // Check that setting DIRTY inserts it into linked list and sets state + CCoinsCacheEntry::SetDirty(n1, sentinel); + BOOST_CHECK(n1.second.IsDirty() && !n1.second.IsFresh()); BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel); BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel); BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1); - // Check that adding FRESH flag on new node inserts it after n1 - n2.second.AddFlags(CCoinsCacheEntry::FRESH, n2, sentinel); - BOOST_CHECK_EQUAL(n2.second.GetFlags(), CCoinsCacheEntry::FRESH); + // Check that setting FRESH on new node inserts it after n1 + CCoinsCacheEntry::SetFresh(n2, sentinel); + BOOST_CHECK(n2.second.IsFresh() && !n2.second.IsDirty()); BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); BOOST_CHECK_EQUAL(n1.second.Next(), &n2); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); - // Check that adding 0 flag has no effect, and doesn't change position - n1.second.AddFlags(0, n1, sentinel); - BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); + // Check that we can set extra state, but they don't change our position + CCoinsCacheEntry::SetFresh(n1, sentinel); + BOOST_CHECK(n1.second.IsDirty() && n1.second.IsFresh()); BOOST_CHECK_EQUAL(n1.second.Next(), &n2); BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel); BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1); BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); - // Check that we can add extra flags, but they don't change our position - n1.second.AddFlags(CCoinsCacheEntry::FRESH, n1, sentinel); - BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH); - BOOST_CHECK_EQUAL(n1.second.Next(), &n2); - BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel); - BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1); - BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); - - // Check that we can clear flags then re-add them - n1.second.ClearFlags(); - BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); - BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2); - BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); - BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); - BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel); - - // Check that calling `ClearFlags` with 0 flags has no effect - n1.second.ClearFlags(); - BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); + // Check that we can clear state then re-set it + n1.second.SetClean(); + BOOST_CHECK(!n1.second.IsDirty() && !n1.second.IsFresh()); BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel); - // Adding 0 still has no effect - n1.second.AddFlags(0, n1, sentinel); + // Calling `SetClean` a second time has no effect + n1.second.SetClean(); + BOOST_CHECK(!n1.second.IsDirty() && !n1.second.IsFresh()); BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel); - // But adding DIRTY re-inserts it after n2 - n1.second.AddFlags(CCoinsCacheEntry::DIRTY, n1, sentinel); - BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); + // Adding DIRTY re-inserts it after n2 + CCoinsCacheEntry::SetDirty(n1, sentinel); + BOOST_CHECK(n1.second.IsDirty() && !n1.second.IsFresh()); BOOST_CHECK_EQUAL(n2.second.Next(), &n1); BOOST_CHECK_EQUAL(n1.second.Prev(), &n2); BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel); diff --git a/src/test/fuzz/CMakeLists.txt b/src/test/fuzz/CMakeLists.txt index 1bf05ee4fb..f65ed62b2d 100644 --- a/src/test/fuzz/CMakeLists.txt +++ b/src/test/fuzz/CMakeLists.txt @@ -76,6 +76,7 @@ add_executable(fuzz p2p_transport_serialization.cpp package_eval.cpp parse_hd_keypath.cpp + parse_iso8601.cpp parse_numbers.cpp parse_script.cpp parse_univalue.cpp diff --git a/src/test/fuzz/checkqueue.cpp b/src/test/fuzz/checkqueue.cpp index 6320b500b6..6b93886c71 100644 --- a/src/test/fuzz/checkqueue.cpp +++ b/src/test/fuzz/checkqueue.cpp @@ -19,9 +19,10 @@ struct DumbCheck { { } - bool operator()() const + std::optional operator()() const { - return result; + if (result) return std::nullopt; + return 1; } }; } // namespace @@ -45,7 +46,7 @@ FUZZ_TARGET(checkqueue) check_queue_1.Add(std::move(checks_1)); } if (fuzzed_data_provider.ConsumeBool()) { - (void)check_queue_1.Wait(); + (void)check_queue_1.Complete(); } CCheckQueueControl check_queue_control{&check_queue_2}; @@ -53,6 +54,6 @@ FUZZ_TARGET(checkqueue) check_queue_control.Add(std::move(checks_2)); } if (fuzzed_data_provider.ConsumeBool()) { - (void)check_queue_control.Wait(); + (void)check_queue_control.Complete(); } } diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 54bfb93b49..9c6aa6e7a1 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -128,7 +128,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000) { CCoinsCacheEntry coins_cache_entry; - const auto flags{fuzzed_data_provider.ConsumeIntegral()}; + const auto dirty{fuzzed_data_provider.ConsumeBool()}; + const auto fresh{fuzzed_data_provider.ConsumeBool()}; if (fuzzed_data_provider.ConsumeBool()) { coins_cache_entry.coin = random_coin; } else { @@ -140,7 +141,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) coins_cache_entry.coin = *opt_coin; } auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first}; - it->second.AddFlags(flags, *it, sentinel); + if (dirty) CCoinsCacheEntry::SetDirty(*it, sentinel); + if (fresh) CCoinsCacheEntry::SetFresh(*it, sentinel); usage += it->second.coin.DynamicMemoryUsage(); } bool expected_code_path = false; diff --git a/src/wallet/test/fuzz/parse_iso8601.cpp b/src/test/fuzz/parse_iso8601.cpp similarity index 60% rename from src/wallet/test/fuzz/parse_iso8601.cpp rename to src/test/fuzz/parse_iso8601.cpp index c1bafc1073..7e51f57905 100644 --- a/src/wallet/test/fuzz/parse_iso8601.cpp +++ b/src/test/fuzz/parse_iso8601.cpp @@ -1,11 +1,10 @@ -// Copyright (c) 2019-2022 The Bitcoin Core developers +// Copyright (c) 2019-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include #include #include -#include #include #include @@ -21,14 +20,8 @@ FUZZ_TARGET(parse_iso8601) const std::string iso8601_datetime = FormatISO8601DateTime(random_time); (void)FormatISO8601Date(random_time); - const int64_t parsed_time_1 = wallet::ParseISO8601DateTime(iso8601_datetime); - if (random_time >= 0) { - assert(parsed_time_1 >= 0); - if (iso8601_datetime.length() == 20) { - assert(parsed_time_1 == random_time); - } - } + const int64_t parsed_time_1{ParseISO8601DateTime(iso8601_datetime).value()}; + assert(parsed_time_1 == random_time); - const int64_t parsed_time_2 = wallet::ParseISO8601DateTime(random_string); - assert(parsed_time_2 >= 0); + (void)ParseISO8601DateTime(random_string); } diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index 425b9559a7..d700e5ac97 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2021-2022 The Bitcoin Core developers +// Copyright (c) 2021-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. @@ -34,8 +34,8 @@ CAmount ConsumeMoney(FuzzedDataProvider& fuzzed_data_provider, const std::option int64_t ConsumeTime(FuzzedDataProvider& fuzzed_data_provider, const std::optional& min, const std::optional& max) noexcept { // Avoid t=0 (1970-01-01T00:00:00Z) since SetMockTime(0) disables mocktime. - static const int64_t time_min{946684801}; // 2000-01-01T00:00:01Z - static const int64_t time_max{4133980799}; // 2100-12-31T23:59:59Z + static const int64_t time_min{ParseISO8601DateTime("2000-01-01T00:00:01Z").value()}; + static const int64_t time_max{ParseISO8601DateTime("2100-12-31T23:59:59Z").value()}; return fuzzed_data_provider.ConsumeIntegralInRange(min.value_or(time_min), max.value_or(time_max)); } diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index f6ae6549bc..c5ecf7022e 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -403,8 +403,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)); - // Should throw block-validation-failed - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool).CreateNewBlock(scriptPubKey), std::runtime_error, HasReason("block-validation-failed")); + BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool).CreateNewBlock(scriptPubKey), std::runtime_error, HasReason("mandatory-script-verify-flag-failed")); // Delete the dummy blocks again. while (m_node.chainman->ActiveChain().Tip()->nHeight > nHeight) { diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp index 6a23a7b895..4284864422 100644 --- a/src/test/result_tests.cpp +++ b/src/test/result_tests.cpp @@ -42,7 +42,7 @@ util::Result IntFn(int i, bool success) util::Result StrFn(bilingual_str s, bool success) { if (success) return s; - return util::Error{strprintf(Untranslated("str %s error."), s.original)}; + return util::Error{Untranslated(strprintf("str %s error.", s.original))}; } util::Result NoCopyFn(int i, bool success) @@ -63,7 +63,8 @@ void ExpectSuccess(const util::Result& result, const bilingual_str& str, Args { ExpectResult(result, true, str); BOOST_CHECK_EQUAL(result.has_value(), true); - BOOST_CHECK_EQUAL(result.value(), T{std::forward(args)...}); + T expected{std::forward(args)...}; + BOOST_CHECK_EQUAL(result.value(), expected); BOOST_CHECK_EQUAL(&result.value(), &*result); } diff --git a/src/test/script_p2sh_tests.cpp b/src/test/script_p2sh_tests.cpp index 096de0724f..bb408b7b0f 100644 --- a/src/test/script_p2sh_tests.cpp +++ b/src/test/script_p2sh_tests.cpp @@ -121,7 +121,7 @@ BOOST_AUTO_TEST_CASE(sign) { CScript sigSave = txTo[i].vin[0].scriptSig; txTo[i].vin[0].scriptSig = txTo[j].vin[0].scriptSig; - bool sigOK = CScriptCheck(txFrom.vout[txTo[i].vin[0].prevout.n], CTransaction(txTo[i]), signature_cache, 0, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, false, &txdata)(); + bool sigOK = !CScriptCheck(txFrom.vout[txTo[i].vin[0].prevout.n], CTransaction(txTo[i]), signature_cache, 0, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, false, &txdata)().has_value(); if (i == j) BOOST_CHECK_MESSAGE(sigOK, strprintf("VerifySignature %d %d", i, j)); else diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index f38c015f6d..8beeec4991 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -588,7 +588,7 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) control.Add(std::move(vChecks)); } - bool controlCheck = control.Wait(); + bool controlCheck = !control.Complete().has_value(); assert(controlCheck); } diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index ecee82c48a..3c36b78260 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -283,6 +283,8 @@ BOOST_AUTO_TEST_CASE(noncontextual_package_tests) BOOST_CHECK(GetPackageHash({tx_parent}) != GetPackageHash({tx_child})); BOOST_CHECK(GetPackageHash({tx_child, tx_child}) != GetPackageHash({tx_child})); BOOST_CHECK(GetPackageHash({tx_child, tx_parent}) != GetPackageHash({tx_child, tx_child})); + BOOST_CHECK(!IsChildWithParents({})); + BOOST_CHECK(!IsChildWithParentsTree({})); } // 24 Parents and 1 Child @@ -492,6 +494,97 @@ BOOST_AUTO_TEST_CASE(package_submission_tests) } } +// Tests for packages containing a single transaction +BOOST_AUTO_TEST_CASE(package_single_tx) +{ + // Mine blocks to mature coinbases. + mineBlocks(3); + LOCK(cs_main); + auto expected_pool_size{m_node.mempool->size()}; + + const CAmount high_fee{1000}; + + // No unconfirmed parents + CKey single_key = GenerateRandomKey(); + CScript single_locking_script = GetScriptForDestination(PKHash(single_key.GetPubKey())); + auto mtx_single = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*input_vout=*/0, + /*input_height=*/0, /*input_signing_key=*/coinbaseKey, + /*output_destination=*/single_locking_script, + /*output_amount=*/CAmount(49 * COIN), /*submit=*/false); + CTransactionRef tx_single = MakeTransactionRef(mtx_single); + Package package_tx_single{tx_single}; + const auto result_single_tx = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_tx_single, /*test_accept=*/false, /*client_maxfeerate=*/{}); + expected_pool_size += 1; + BOOST_CHECK_MESSAGE(result_single_tx.m_state.IsValid(), + "Package validation unexpectedly failed: " << result_single_tx.m_state.ToString()); + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + + // Parent and Child. Both submitted by themselves through the ProcessNewPackage interface. + CKey parent_key = GenerateRandomKey(); + CScript parent_locking_script = GetScriptForDestination(WitnessV0KeyHash(parent_key.GetPubKey())); + auto mtx_parent = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[1], /*input_vout=*/0, + /*input_height=*/0, /*input_signing_key=*/coinbaseKey, + /*output_destination=*/parent_locking_script, + /*output_amount=*/CAmount(50 * COIN) - high_fee, /*submit=*/false); + CTransactionRef tx_parent = MakeTransactionRef(mtx_parent); + Package package_just_parent{tx_parent}; + const auto result_just_parent = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_just_parent, /*test_accept=*/false, /*client_maxfeerate=*/{}); + if (auto err_parent_child{CheckPackageMempoolAcceptResult(package_just_parent, result_just_parent, /*expect_valid=*/true, nullptr)}) { + BOOST_ERROR(err_parent_child.value()); + } else { + auto it_parent = result_just_parent.m_tx_results.find(tx_parent->GetWitnessHash()); + BOOST_CHECK_MESSAGE(it_parent->second.m_state.IsValid(), it_parent->second.m_state.ToString()); + BOOST_CHECK(it_parent->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_parent)) == high_fee); + BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().size(), 1); + BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().front(), tx_parent->GetWitnessHash()); + } + expected_pool_size += 1; + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + + CKey child_key = GenerateRandomKey(); + CScript child_locking_script = GetScriptForDestination(WitnessV0KeyHash(child_key.GetPubKey())); + auto mtx_child = CreateValidMempoolTransaction(/*input_transaction=*/tx_parent, /*input_vout=*/0, + /*input_height=*/101, /*input_signing_key=*/parent_key, + /*output_destination=*/child_locking_script, + /*output_amount=*/CAmount(50 * COIN) - 2 * high_fee, /*submit=*/false); + CTransactionRef tx_child = MakeTransactionRef(mtx_child); + Package package_just_child{tx_child}; + const auto result_just_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_just_child, /*test_accept=*/false, /*client_maxfeerate=*/{}); + if (auto err_parent_child{CheckPackageMempoolAcceptResult(package_just_child, result_just_child, /*expect_valid=*/true, nullptr)}) { + BOOST_ERROR(err_parent_child.value()); + } else { + auto it_child = result_just_child.m_tx_results.find(tx_child->GetWitnessHash()); + BOOST_CHECK_MESSAGE(it_child->second.m_state.IsValid(), it_child->second.m_state.ToString()); + BOOST_CHECK(it_child->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_child)) == high_fee); + BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().size(), 1); + BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().front(), tx_child->GetWitnessHash()); + } + expected_pool_size += 1; + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + + // Too-low fee to RBF tx_single + auto mtx_single_low_fee = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*input_vout=*/0, + /*input_height=*/0, /*input_signing_key=*/coinbaseKey, + /*output_destination=*/single_locking_script, + /*output_amount=*/CAmount(49 * COIN - 1), /*submit=*/false); + CTransactionRef tx_single_low_fee = MakeTransactionRef(mtx_single_low_fee); + Package package_tx_single_low_fee{tx_single_low_fee}; + const auto result_single_tx_low_fee = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_tx_single_low_fee, /*test_accept=*/false, /*client_maxfeerate=*/{}); + + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + + BOOST_CHECK(!result_single_tx_low_fee.m_state.IsValid()); + BOOST_CHECK_EQUAL(result_single_tx_low_fee.m_state.GetResult(), PackageValidationResult::PCKG_TX); + auto it_low_fee = result_single_tx_low_fee.m_tx_results.find(tx_single_low_fee->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_low_fee->second.m_state.GetResult(), TxValidationResult::TX_RECONSIDERABLE); + if (auto err_single{CheckPackageMempoolAcceptResult(package_tx_single_low_fee, result_single_tx_low_fee, /*expect_valid=*/false, m_node.mempool.get())}) { + BOOST_ERROR(err_single.value()); + } + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); +} + // Tests for packages containing transactions that have same-txid-different-witness equivalents in // the mempool. BOOST_AUTO_TEST_CASE(package_witness_swap_tests) diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp index 9f5b702acd..fbab9e7aba 100644 --- a/src/test/util_string_tests.cpp +++ b/src/test/util_string_tests.cpp @@ -16,13 +16,13 @@ BOOST_AUTO_TEST_SUITE(util_string_tests) template inline void PassFmt(util::ConstevalFormatString fmt) { - // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused. - decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt); + // Execute compile-time check again at run-time to get code coverage stats + util::detail::CheckNumFormatSpecifiers(fmt.fmt); } template inline void FailFmtWithError(const char* wrong_fmt, std::string_view error) { - BOOST_CHECK_EXCEPTION(util::ConstevalFormatString::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason(error)); + BOOST_CHECK_EXCEPTION(util::detail::CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason{error}); } BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 1624fb8b5b..3f6e5b1b66 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2011-2022 The Bitcoin Core developers +// Copyright (c) 2011-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. @@ -323,20 +323,65 @@ BOOST_AUTO_TEST_CASE(util_TrimString) BOOST_CHECK_EQUAL(TrimStringView(std::string("\x05\x04\x03\x02\x01\x00", 6), std::string("\x05\x04\x03\x02\x01\x00", 6)), ""); } +BOOST_AUTO_TEST_CASE(util_ParseISO8601DateTime) +{ + BOOST_CHECK_EQUAL(ParseISO8601DateTime("1969-12-31T23:59:59Z").value(), -1); + BOOST_CHECK_EQUAL(ParseISO8601DateTime("1970-01-01T00:00:00Z").value(), 0); + BOOST_CHECK_EQUAL(ParseISO8601DateTime("1970-01-01T00:00:01Z").value(), 1); + BOOST_CHECK_EQUAL(ParseISO8601DateTime("2000-01-01T00:00:01Z").value(), 946684801); + BOOST_CHECK_EQUAL(ParseISO8601DateTime("2011-09-30T23:36:17Z").value(), 1317425777); + BOOST_CHECK_EQUAL(ParseISO8601DateTime("2100-12-31T23:59:59Z").value(), 4133980799); + BOOST_CHECK_EQUAL(ParseISO8601DateTime("9999-12-31T23:59:59Z").value(), 253402300799); + + // Accept edge-cases, where the time overflows. They are not produced by + // FormatISO8601DateTime, so this can be changed in the future, if needed. + // For now, keep compatibility with the previous implementation. + BOOST_CHECK_EQUAL(ParseISO8601DateTime("2000-01-01T99:00:00Z").value(), 947041200); + BOOST_CHECK_EQUAL(ParseISO8601DateTime("2000-01-01T00:99:00Z").value(), 946690740); + BOOST_CHECK_EQUAL(ParseISO8601DateTime("2000-01-01T00:00:99Z").value(), 946684899); + BOOST_CHECK_EQUAL(ParseISO8601DateTime("2000-01-01T99:99:99Z").value(), 947047239); + + // Reject date overflows. + BOOST_CHECK(!ParseISO8601DateTime("2000-99-01T00:00:00Z")); + BOOST_CHECK(!ParseISO8601DateTime("2000-01-99T00:00:00Z")); + + // Reject out-of-range years + BOOST_CHECK(!ParseISO8601DateTime("32768-12-31T23:59:59Z")); + BOOST_CHECK(!ParseISO8601DateTime("32767-12-31T23:59:59Z")); + BOOST_CHECK(!ParseISO8601DateTime("32767-12-31T00:00:00Z")); + BOOST_CHECK(!ParseISO8601DateTime("999-12-31T00:00:00Z")); + + // Reject invalid format + const std::string valid{"2000-01-01T00:00:01Z"}; + BOOST_CHECK(ParseISO8601DateTime(valid).has_value()); + for (auto mut{0U}; mut < valid.size(); ++mut) { + std::string invalid{valid}; + invalid[mut] = 'a'; + BOOST_CHECK(!ParseISO8601DateTime(invalid)); + } +} + BOOST_AUTO_TEST_CASE(util_FormatISO8601DateTime) { BOOST_CHECK_EQUAL(FormatISO8601DateTime(971890963199), "32767-12-31T23:59:59Z"); BOOST_CHECK_EQUAL(FormatISO8601DateTime(971890876800), "32767-12-31T00:00:00Z"); - BOOST_CHECK_EQUAL(FormatISO8601DateTime(1317425777), "2011-09-30T23:36:17Z"); + + BOOST_CHECK_EQUAL(FormatISO8601DateTime(-1), "1969-12-31T23:59:59Z"); BOOST_CHECK_EQUAL(FormatISO8601DateTime(0), "1970-01-01T00:00:00Z"); + BOOST_CHECK_EQUAL(FormatISO8601DateTime(1), "1970-01-01T00:00:01Z"); + BOOST_CHECK_EQUAL(FormatISO8601DateTime(946684801), "2000-01-01T00:00:01Z"); + BOOST_CHECK_EQUAL(FormatISO8601DateTime(1317425777), "2011-09-30T23:36:17Z"); + BOOST_CHECK_EQUAL(FormatISO8601DateTime(4133980799), "2100-12-31T23:59:59Z"); + BOOST_CHECK_EQUAL(FormatISO8601DateTime(253402300799), "9999-12-31T23:59:59Z"); } BOOST_AUTO_TEST_CASE(util_FormatISO8601Date) { BOOST_CHECK_EQUAL(FormatISO8601Date(971890963199), "32767-12-31"); BOOST_CHECK_EQUAL(FormatISO8601Date(971890876800), "32767-12-31"); - BOOST_CHECK_EQUAL(FormatISO8601Date(1317425777), "2011-09-30"); + BOOST_CHECK_EQUAL(FormatISO8601Date(0), "1970-01-01"); + BOOST_CHECK_EQUAL(FormatISO8601Date(1317425777), "2011-09-30"); } BOOST_AUTO_TEST_CASE(util_FormatMoney) diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index 58fc1bdf2a..8d14b6ce2e 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -356,7 +356,7 @@ void TorController::get_socks_cb(TorControlConnection& _conn, const TorControlRe std::string socks_location; if (reply.code == 250) { for (const auto& line : reply.lines) { - if (0 == line.compare(0, 20, "net/listeners/socks=")) { + if (line.starts_with("net/listeners/socks=")) { const std::string port_list_str = line.substr(20); std::vector port_list = SplitString(port_list_str, ' '); @@ -367,7 +367,7 @@ void TorController::get_socks_cb(TorControlConnection& _conn, const TorControlRe if (portstr.empty()) continue; } socks_location = portstr; - if (0 == portstr.compare(0, 10, "127.0.0.1:")) { + if (portstr.starts_with("127.0.0.1:")) { // Prefer localhost - ignore other ports break; } diff --git a/src/util/bitset.h b/src/util/bitset.h index 6f9e808c37..17ebc709eb 100644 --- a/src/util/bitset.h +++ b/src/util/bitset.h @@ -366,7 +366,7 @@ class MultiIntBitSet if (count) { unsigned i = 0; while (count > LIMB_BITS) { - ret.m_val[i++] = ~I{0}; + ret.m_val[i++] = I(~I{0}); count -= LIMB_BITS; } ret.m_val[i] = I(~I{0}) >> (LIMB_BITS - count); diff --git a/src/util/strencodings.h b/src/util/strencodings.h index 1543de03ab..d0809162fa 100644 --- a/src/util/strencodings.h +++ b/src/util/strencodings.h @@ -377,6 +377,24 @@ consteval uint8_t ConstevalHexDigit(const char c) throw "Only lowercase hex digits are allowed, for consistency"; } +namespace detail { +template +struct Hex { + std::array bytes{}; + consteval Hex(const char (&hex_str)[N]) + // 2 hex digits required per byte + implicit null terminator + requires(N % 2 == 1) + { + if (hex_str[N - 1]) throw "null terminator required"; + for (std::size_t i = 0; i < bytes.size(); ++i) { + bytes[i] = static_cast( + (ConstevalHexDigit(hex_str[2 * i]) << 4) | + ConstevalHexDigit(hex_str[2 * i + 1])); + } + } +}; +} // namespace detail + /** * ""_hex is a compile-time user-defined literal returning a * `std::array`, equivalent to ParseHex(). Variants provided: @@ -407,25 +425,6 @@ consteval uint8_t ConstevalHexDigit(const char c) * time/runtime barrier. */ inline namespace hex_literals { -namespace detail { - -template -struct Hex { - std::array bytes{}; - consteval Hex(const char (&hex_str)[N]) - // 2 hex digits required per byte + implicit null terminator - requires(N % 2 == 1) - { - if (hex_str[N - 1]) throw "null terminator required"; - for (std::size_t i = 0; i < bytes.size(); ++i) { - bytes[i] = static_cast( - (ConstevalHexDigit(hex_str[2 * i]) << 4) | - ConstevalHexDigit(hex_str[2 * i + 1])); - } - } -}; - -} // namespace detail template constexpr auto operator""_hex() { return str.bytes; } diff --git a/src/util/string.h b/src/util/string.h index c9e33e6592..b523e6ef4e 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -17,6 +17,69 @@ #include namespace util { +namespace detail { +template +constexpr static void CheckNumFormatSpecifiers(const char* str) +{ + unsigned count_normal{0}; // Number of "normal" specifiers, like %s + unsigned count_pos{0}; // Max number in positional specifier, like %8$s + for (auto it{str}; *it != '\0'; ++it) { + if (*it != '%' || *++it == '%') continue; // Skip escaped %% + + auto add_arg = [&] { + unsigned maybe_num{0}; + while ('0' <= *it && *it <= '9') { + maybe_num *= 10; + maybe_num += *it - '0'; + ++it; + } + + if (*it == '$') { + ++it; + // Positional specifier, like %8$s + if (maybe_num == 0) throw "Positional format specifier must have position of at least 1"; + count_pos = std::max(count_pos, maybe_num); + } else { + // Non-positional specifier, like %s + ++count_normal; + } + }; + + // Increase argument count and consume positional specifier, if present. + add_arg(); + + // Consume flags. + while (*it == '#' || *it == '0' || *it == '-' || *it == ' ' || *it == '+') ++it; + + auto parse_size = [&] { + if (*it == '*') { + ++it; + add_arg(); + } else { + while ('0' <= *it && *it <= '9') ++it; + } + }; + + // Consume dynamic or static width value. + parse_size(); + + // Consume dynamic or static precision value. + if (*it == '.') { + ++it; + parse_size(); + } + + if (*it == '\0') throw "Format specifier incorrectly terminated by end of string"; + + // Length and type in "[flags][width][.precision][length]type" + // is not checked. Parsing continues with the next '%'. + } + if (count_normal && count_pos) throw "Format specifiers must be all positional or all non-positional!"; + unsigned count{count_normal | count_pos}; + if (num_params != count) throw "Format specifier count must match the argument count!"; +} +} // namespace detail + /** * @brief A wrapper for a compile-time partially validated format string * @@ -28,66 +91,7 @@ namespace util { template struct ConstevalFormatString { const char* const fmt; - consteval ConstevalFormatString(const char* str) : fmt{str} { Detail_CheckNumFormatSpecifiers(fmt); } - constexpr static void Detail_CheckNumFormatSpecifiers(const char* str) - { - unsigned count_normal{0}; // Number of "normal" specifiers, like %s - unsigned count_pos{0}; // Max number in positional specifier, like %8$s - for (auto it{str}; *it != '\0'; ++it) { - if (*it != '%' || *++it == '%') continue; // Skip escaped %% - - auto add_arg = [&] { - unsigned maybe_num{0}; - while ('0' <= *it && *it <= '9') { - maybe_num *= 10; - maybe_num += *it - '0'; - ++it; - } - - if (*it == '$') { - ++it; - // Positional specifier, like %8$s - if (maybe_num == 0) throw "Positional format specifier must have position of at least 1"; - count_pos = std::max(count_pos, maybe_num); - } else { - // Non-positional specifier, like %s - ++count_normal; - } - }; - - // Increase argument count and consume positional specifier, if present. - add_arg(); - - // Consume flags. - while (*it == '#' || *it == '0' || *it == '-' || *it == ' ' || *it == '+') ++it; - - auto parse_size = [&] { - if (*it == '*') { - ++it; - add_arg(); - } else { - while ('0' <= *it && *it <= '9') ++it; - } - }; - - // Consume dynamic or static width value. - parse_size(); - - // Consume dynamic or static precision value. - if (*it == '.') { - ++it; - parse_size(); - } - - if (*it == '\0') throw "Format specifier incorrectly terminated by end of string"; - - // Length and type in "[flags][width][.precision][length]type" - // is not checked. Parsing continues with the next '%'. - } - if (count_normal && count_pos) throw "Format specifiers must be all positional or all non-positional!"; - unsigned count{count_normal | count_pos}; - if (num_params != count) throw "Format specifier count must match the argument count!"; - } + consteval ConstevalFormatString(const char* str) : fmt{str} { detail::CheckNumFormatSpecifiers(fmt); } }; void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute); diff --git a/src/util/time.cpp b/src/util/time.cpp index f08eb5300a..e20f30a474 100644 --- a/src/util/time.cpp +++ b/src/util/time.cpp @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2022 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. @@ -8,10 +8,13 @@ #include #include #include +#include #include #include +#include #include +#include #include void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); } @@ -60,6 +63,30 @@ std::string FormatISO8601Date(int64_t nTime) return strprintf("%04i-%02u-%02u", signed{ymd.year()}, unsigned{ymd.month()}, unsigned{ymd.day()}); } +std::optional ParseISO8601DateTime(std::string_view str) +{ + constexpr auto FMT_SIZE{std::string_view{"2000-01-01T01:01:01Z"}.size()}; + if (str.size() != FMT_SIZE || str[4] != '-' || str[7] != '-' || str[10] != 'T' || str[13] != ':' || str[16] != ':' || str[19] != 'Z') { + return {}; + } + const auto year{ToIntegral(str.substr(0, 4))}; + const auto month{ToIntegral(str.substr(5, 2))}; + const auto day{ToIntegral(str.substr(8, 2))}; + const auto hour{ToIntegral(str.substr(11, 2))}; + const auto min{ToIntegral(str.substr(14, 2))}; + const auto sec{ToIntegral(str.substr(17, 2))}; + if (!year || !month || !day || !hour || !min || !sec) { + return {}; + } + const std::chrono::year_month_day ymd{std::chrono::year{*year}, std::chrono::month{*month}, std::chrono::day{*day}}; + if (!ymd.ok()) { + return {}; + } + const auto time{std::chrono::hours{*hour} + std::chrono::minutes{*min} + std::chrono::seconds{*sec}}; + const auto tp{std::chrono::sys_days{ymd} + time}; + return int64_t{TicksSinceEpoch(tp)}; +} + struct timeval MillisToTimeval(int64_t nTimeout) { struct timeval timeout; diff --git a/src/util/time.h b/src/util/time.h index 108560e0e0..27cbe50581 100644 --- a/src/util/time.h +++ b/src/util/time.h @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2022 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. @@ -8,7 +8,9 @@ #include // IWYU pragma: export #include +#include #include +#include using namespace std::chrono_literals; @@ -105,6 +107,7 @@ T GetTime() */ std::string FormatISO8601DateTime(int64_t nTime); std::string FormatISO8601Date(int64_t nTime); +std::optional ParseISO8601DateTime(std::string_view str); /** * Convert milliseconds to a struct timeval for e.g. select. diff --git a/src/util/translation.h b/src/util/translation.h index 6effe102f9..7c734a1766 100644 --- a/src/util/translation.h +++ b/src/util/translation.h @@ -10,6 +10,9 @@ #include #include +/** Translate a message to the native language of the user. */ +const extern std::function G_TRANSLATION_FUN; + /** * Bilingual messages: * - in GUI: user's native language + untranslated (i.e. English) @@ -64,9 +67,6 @@ bilingual_str format(const bilingual_str& fmt, const Args&... args) } } // namespace tinyformat -/** Translate a message to the native language of the user. */ -const extern std::function G_TRANSLATION_FUN; - struct ConstevalStringLiteral { const char* const lit; consteval ConstevalStringLiteral(const char* str) : lit{str} {} diff --git a/src/validation.cpp b/src/validation.cpp index 791acb0a58..b1e13e8850 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1686,10 +1686,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptSubPackage(const std::vector 1 transactions. - assert(package.size() > 1); - // The package must be 1 child with all of its unconfirmed parents. The package is expected to - // be sorted, so the last transaction is the child. - const auto& child = package.back(); - std::unordered_set unconfirmed_parent_txids; - std::transform(package.cbegin(), package.cend() - 1, - std::inserter(unconfirmed_parent_txids, unconfirmed_parent_txids.end()), - [](const auto& tx) { return tx->GetHash(); }); - - // All child inputs must refer to a preceding package transaction or a confirmed UTXO. The only - // way to verify this is to look up the child's inputs in our current coins view (not including - // mempool), and enforce that all parents not present in the package be available at chain tip. - // Since this check can bring new coins into the coins cache, keep track of these coins and - // uncache them if we don't end up submitting this package to the mempool. - const CCoinsViewCache& coins_tip_cache = m_active_chainstate.CoinsTip(); - for (const auto& input : child->vin) { - if (!coins_tip_cache.HaveCoinInCache(input.prevout)) { - args.m_coins_to_uncache.push_back(input.prevout); - } - } - // Using the MemPoolAccept m_view cache allows us to look up these same coins faster later. - // This should be connecting directly to CoinsTip, not to m_viewmempool, because we specifically - // require inputs to be confirmed if they aren't in the package. - m_view.SetBackend(m_active_chainstate.CoinsTip()); - const auto package_or_confirmed = [this, &unconfirmed_parent_txids](const auto& input) { - return unconfirmed_parent_txids.count(input.prevout.hash) > 0 || m_view.HaveCoin(input.prevout); - }; - if (!std::all_of(child->vin.cbegin(), child->vin.cend(), package_or_confirmed)) { - package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents"); - return PackageMempoolAcceptResult(package_state_quit_early, {}); + if (package.size() > 1) { + // All transactions in the package must be a parent of the last transaction. This is just an + // opportunity for us to fail fast on a context-free check without taking the mempool lock. + if (!IsChildWithParents(package)) { + package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents"); + return PackageMempoolAcceptResult(package_state_quit_early, {}); + } + + // IsChildWithParents() guarantees the package is > 1 transactions. + assert(package.size() > 1); + // The package must be 1 child with all of its unconfirmed parents. The package is expected to + // be sorted, so the last transaction is the child. + const auto& child = package.back(); + std::unordered_set unconfirmed_parent_txids; + std::transform(package.cbegin(), package.cend() - 1, + std::inserter(unconfirmed_parent_txids, unconfirmed_parent_txids.end()), + [](const auto& tx) { return tx->GetHash(); }); + + // All child inputs must refer to a preceding package transaction or a confirmed UTXO. The only + // way to verify this is to look up the child's inputs in our current coins view (not including + // mempool), and enforce that all parents not present in the package be available at chain tip. + // Since this check can bring new coins into the coins cache, keep track of these coins and + // uncache them if we don't end up submitting this package to the mempool. + const CCoinsViewCache& coins_tip_cache = m_active_chainstate.CoinsTip(); + for (const auto& input : child->vin) { + if (!coins_tip_cache.HaveCoinInCache(input.prevout)) { + args.m_coins_to_uncache.push_back(input.prevout); + } + } + // Using the MemPoolAccept m_view cache allows us to look up these same coins faster later. + // This should be connecting directly to CoinsTip, not to m_viewmempool, because we specifically + // require inputs to be confirmed if they aren't in the package. + m_view.SetBackend(m_active_chainstate.CoinsTip()); + const auto package_or_confirmed = [this, &unconfirmed_parent_txids](const auto& input) { + return unconfirmed_parent_txids.count(input.prevout.hash) > 0 || m_view.HaveCoin(input.prevout); + }; + if (!std::all_of(child->vin.cbegin(), child->vin.cend(), package_or_confirmed)) { + package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents"); + return PackageMempoolAcceptResult(package_state_quit_early, {}); + } + // Protect against bugs where we pull more inputs from disk that miss being added to + // coins_to_uncache. The backend will be connected again when needed in PreChecks. + m_view.SetBackend(m_dummy); } - // Protect against bugs where we pull more inputs from disk that miss being added to - // coins_to_uncache. The backend will be connected again when needed in PreChecks. - m_view.SetBackend(m_dummy); LOCK(m_pool.cs); // Stores results from which we will create the returned PackageMempoolAcceptResult. @@ -1749,6 +1755,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // this transaction. "Nonfinal" because if a transaction fails by itself but succeeds later // (i.e. when evaluated with a fee-bumping child), the result in this map may be discarded. std::map individual_results_nonfinal; + // Tracks whether we think package submission could result in successful entry to the mempool bool quit_early{false}; std::vector txns_package_eval; for (const auto& tx : package) { @@ -1790,8 +1797,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // in package validation, because its fees should only be "used" once. assert(m_pool.exists(GenTxid::Wtxid(wtxid))); results_final.emplace(wtxid, single_res); - } else if (single_res.m_state.GetResult() != TxValidationResult::TX_RECONSIDERABLE && - single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { + } else if (package.size() == 1 || // If there is only one transaction, no need to retry it "as a package" + (single_res.m_state.GetResult() != TxValidationResult::TX_RECONSIDERABLE && + single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS)) { // Package validation policy only differs from individual policy in its evaluation // of feerate. For example, if a transaction fails here due to violation of a // consensus rule, the result will not change when it is submitted as part of a @@ -2156,10 +2164,16 @@ void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo &txund AddCoins(inputs, tx, nHeight); } -bool CScriptCheck::operator()() { +std::optional> CScriptCheck::operator()() { const CScript &scriptSig = ptxTo->vin[nIn].scriptSig; const CScriptWitness *witness = &ptxTo->vin[nIn].scriptWitness; - return VerifyScript(scriptSig, m_tx_out.scriptPubKey, witness, nFlags, CachingTransactionSignatureChecker(ptxTo, nIn, m_tx_out.nValue, cacheStore, *m_signature_cache, *txdata), &error); + ScriptError error{SCRIPT_ERR_UNKNOWN_ERROR}; + if (VerifyScript(scriptSig, m_tx_out.scriptPubKey, witness, nFlags, CachingTransactionSignatureChecker(ptxTo, nIn, m_tx_out.nValue, cacheStore, *m_signature_cache, *txdata), &error)) { + return std::nullopt; + } else { + auto debug_str = strprintf("input %i of %s (wtxid %s), spending %s:%i", nIn, ptxTo->GetHash().ToString(), ptxTo->GetWitnessHash().ToString(), ptxTo->vin[nIn].prevout.hash.ToString(), ptxTo->vin[nIn].prevout.n); + return std::make_pair(error, std::move(debug_str)); + } } ValidationCache::ValidationCache(const size_t script_execution_cache_bytes, const size_t signature_cache_bytes) @@ -2248,9 +2262,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, CScriptCheck check(txdata.m_spent_outputs[i], tx, validation_cache.m_signature_cache, i, flags, cacheSigStore, &txdata); if (pvChecks) { pvChecks->emplace_back(std::move(check)); - } else if (!check()) { - ScriptError error{check.GetScriptError()}; - + } else if (auto result = check(); result.has_value()) { if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) { // Check whether the failure was caused by a // non-mandatory script verification check, such as @@ -2262,21 +2274,23 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, // data providers. CScriptCheck check2(txdata.m_spent_outputs[i], tx, validation_cache.m_signature_cache, i, flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata); - if (check2()) - return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError()))); - - // If the second check failed, it failed due to a mandatory script verification - // flag, but the first check might have failed on a non-mandatory script - // verification flag. - // - // Avoid reporting a mandatory script check failure with a non-mandatory error - // string by reporting the error from the second check. - error = check2.GetScriptError(); + auto mandatory_result = check2(); + if (!mandatory_result.has_value()) { + return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(result->first)), result->second); + } else { + // If the second check failed, it failed due to a mandatory script verification + // flag, but the first check might have failed on a non-mandatory script + // verification flag. + // + // Avoid reporting a mandatory script check failure with a non-mandatory error + // string by reporting the error from the second check. + result = mandatory_result; + } } // MANDATORY flag failures correspond to // TxValidationResult::TX_CONSENSUS. - return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(error))); + return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second); } } @@ -2649,8 +2663,8 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, for (const auto& tx : block.vtx) { for (size_t o = 0; o < tx->vout.size(); o++) { if (view.HaveCoin(COutPoint(tx->GetHash(), o))) { - LogPrintf("ERROR: ConnectBlock(): tried to overwrite transaction\n"); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-BIP30"); + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-BIP30", + "tried to overwrite transaction"); } } } @@ -2689,6 +2703,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, blockundo.vtxundo.reserve(block.vtx.size() - 1); for (unsigned int i = 0; i < block.vtx.size(); i++) { + if (!state.IsValid()) break; const CTransaction &tx = *(block.vtx[i]); nInputs += tx.vin.size(); @@ -2700,14 +2715,15 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) { // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, - tx_state.GetRejectReason(), tx_state.GetDebugMessage()); - LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString()); - return false; + tx_state.GetRejectReason(), + tx_state.GetDebugMessage() + " in transaction " + tx.GetHash().ToString()); + break; } nFees += txfee; if (!MoneyRange(nFees)) { - LogPrintf("ERROR: %s: accumulated fee in the block out of range.\n", __func__); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-accumulated-fee-outofrange"); + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-accumulated-fee-outofrange", + "accumulated fee in the block out of range"); + break; } // Check that transaction is BIP68 final @@ -2719,8 +2735,9 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, } if (!SequenceLocks(tx, nLockTimeFlags, prevheights, *pindex)) { - LogPrintf("ERROR: %s: contains a non-BIP68-final transaction\n", __func__); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-nonfinal"); + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-nonfinal", + "contains a non-BIP68-final transaction " + tx.GetHash().ToString()); + break; } } @@ -2730,8 +2747,8 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // * witness (when witness enabled in flags and excludes coinbase) nSigOpsCost += GetTransactionSigOpCost(tx, view, flags); if (nSigOpsCost > MAX_BLOCK_SIGOPS_COST) { - LogPrintf("ERROR: ConnectBlock(): too many sigops\n"); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops"); + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops", "too many sigops"); + break; } if (!tx.IsCoinBase()) @@ -2743,9 +2760,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), tx_state.GetDebugMessage()); - LogError("ConnectBlock(): CheckInputScripts on %s failed with %s\n", - tx.GetHash().ToString(), state.ToString()); - return false; + break; } control.Add(std::move(vChecks)); } @@ -2765,14 +2780,18 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, Ticks(m_chainman.time_connect) / m_chainman.num_blocks_total); CAmount blockReward = nFees + GetBlockSubsidy(pindex->nHeight, params.GetConsensus()); - if (block.vtx[0]->GetValueOut() > blockReward) { - LogPrintf("ERROR: ConnectBlock(): coinbase pays too much (actual=%d vs limit=%d)\n", block.vtx[0]->GetValueOut(), blockReward); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-amount"); + if (block.vtx[0]->GetValueOut() > blockReward && state.IsValid()) { + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-amount", + strprintf("coinbase pays too much (actual=%d vs limit=%d)", block.vtx[0]->GetValueOut(), blockReward)); } - if (!control.Wait()) { - LogPrintf("ERROR: %s: CheckQueue failed\n", __func__); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "block-validation-failed"); + auto parallel_result = control.Complete(); + if (parallel_result.has_value() && state.IsValid()) { + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(parallel_result->first)), parallel_result->second); + } + if (!state.IsValid()) { + LogInfo("Block validation error: %s", state.ToString()); + return false; } const auto time_4{SteadyClock::now()}; m_chainman.time_verify += time_4 - time_2; @@ -2782,8 +2801,9 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, Ticks(m_chainman.time_verify), Ticks(m_chainman.time_verify) / m_chainman.num_blocks_total); - if (fJustCheck) + if (fJustCheck) { return true; + } if (!m_blockman.WriteUndoDataForBlock(blockundo, state, *pindex)) { return false; @@ -4377,7 +4397,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida *ppindex = pindex; if (pindex->nStatus & BLOCK_FAILED_MASK) { LogDebug(BCLog::VALIDATION, "%s: block %s is marked invalid\n", __func__, hash.ToString()); - return state.Invalid(BlockValidationResult::BLOCK_CACHED_INVALID, "duplicate"); + return state.Invalid(BlockValidationResult::BLOCK_CACHED_INVALID, "duplicate-invalid"); } return true; } @@ -5765,20 +5785,20 @@ util::Result ChainstateManager::ActivateSnapshot( if (!GetParams().AssumeutxoForBlockhash(base_blockhash).has_value()) { auto available_heights = GetParams().GetAvailableSnapshotHeights(); std::string heights_formatted = util::Join(available_heights, ", ", [&](const auto& i) { return util::ToString(i); }); - return util::Error{strprintf(Untranslated("assumeutxo block hash in snapshot metadata not recognized (hash: %s). The following snapshot heights are available: %s"), + return util::Error{Untranslated(strprintf("assumeutxo block hash in snapshot metadata not recognized (hash: %s). The following snapshot heights are available: %s", base_blockhash.ToString(), - heights_formatted)}; + heights_formatted))}; } snapshot_start_block = m_blockman.LookupBlockIndex(base_blockhash); if (!snapshot_start_block) { - return util::Error{strprintf(Untranslated("The base block header (%s) must appear in the headers chain. Make sure all headers are syncing, and call loadtxoutset again"), - base_blockhash.ToString())}; + return util::Error{Untranslated(strprintf("The base block header (%s) must appear in the headers chain. Make sure all headers are syncing, and call loadtxoutset again", + base_blockhash.ToString()))}; } bool start_block_invalid = snapshot_start_block->nStatus & BLOCK_FAILED_MASK; if (start_block_invalid) { - return util::Error{strprintf(Untranslated("The base block header (%s) is part of an invalid chain"), base_blockhash.ToString())}; + return util::Error{Untranslated(strprintf("The base block header (%s) is part of an invalid chain", base_blockhash.ToString()))}; } if (!m_best_header || m_best_header->GetAncestor(snapshot_start_block->nHeight) != snapshot_start_block) { @@ -5857,7 +5877,7 @@ util::Result ChainstateManager::ActivateSnapshot( if (auto res{this->PopulateAndValidateSnapshot(*snapshot_chainstate, coins_file, metadata)}; !res) { LOCK(::cs_main); - return cleanup_bad_snapshot(strprintf(Untranslated("Population failed: %s"), util::ErrorString(res))); + return cleanup_bad_snapshot(Untranslated(strprintf("Population failed: %s", util::ErrorString(res).original))); } LOCK(::cs_main); // cs_main required for rest of snapshot activation. @@ -5938,16 +5958,16 @@ util::Result ChainstateManager::PopulateAndValidateSnapshot( if (!snapshot_start_block) { // Needed for ComputeUTXOStats to determine the // height and to avoid a crash when base_blockhash.IsNull() - return util::Error{strprintf(Untranslated("Did not find snapshot start blockheader %s"), - base_blockhash.ToString())}; + return util::Error{Untranslated(strprintf("Did not find snapshot start blockheader %s", + base_blockhash.ToString()))}; } int base_height = snapshot_start_block->nHeight; const auto& maybe_au_data = GetParams().AssumeutxoForHeight(base_height); if (!maybe_au_data) { - return util::Error{strprintf(Untranslated("Assumeutxo height in snapshot metadata not recognized " - "(%d) - refusing to load snapshot"), base_height)}; + return util::Error{Untranslated(strprintf("Assumeutxo height in snapshot metadata not recognized " + "(%d) - refusing to load snapshot", base_height))}; } const AssumeutxoData& au_data = *maybe_au_data; @@ -5985,12 +6005,12 @@ util::Result ChainstateManager::PopulateAndValidateSnapshot( if (coin.nHeight > base_height || outpoint.n >= std::numeric_limits::max() // Avoid integer wrap-around in coinstats.cpp:ApplyHash ) { - return util::Error{strprintf(Untranslated("Bad snapshot data after deserializing %d coins"), - coins_count - coins_left)}; + return util::Error{Untranslated(strprintf("Bad snapshot data after deserializing %d coins", + coins_count - coins_left))}; } if (!MoneyRange(coin.out.nValue)) { - return util::Error{strprintf(Untranslated("Bad snapshot data after deserializing %d coins - bad tx out value"), - coins_count - coins_left)}; + return util::Error{Untranslated(strprintf("Bad snapshot data after deserializing %d coins - bad tx out value", + coins_count - coins_left))}; } coins_cache.EmplaceCoinInternalDANGER(std::move(outpoint), std::move(coin)); @@ -6028,8 +6048,8 @@ util::Result ChainstateManager::PopulateAndValidateSnapshot( } } } catch (const std::ios_base::failure&) { - return util::Error{strprintf(Untranslated("Bad snapshot format or truncated snapshot after deserializing %d coins"), - coins_processed)}; + return util::Error{Untranslated(strprintf("Bad snapshot format or truncated snapshot after deserializing %d coins", + coins_processed))}; } } @@ -6049,8 +6069,8 @@ util::Result ChainstateManager::PopulateAndValidateSnapshot( out_of_coins = true; } if (!out_of_coins) { - return util::Error{strprintf(Untranslated("Bad snapshot - coins left over after deserializing %d coins"), - coins_count)}; + return util::Error{Untranslated(strprintf("Bad snapshot - coins left over after deserializing %d coins", + coins_count))}; } LogPrintf("[snapshot] loaded %d (%.2f MB) coins from snapshot %s\n", @@ -6081,8 +6101,8 @@ util::Result ChainstateManager::PopulateAndValidateSnapshot( // Assert that the deserialized chainstate contents match the expected assumeutxo value. if (AssumeutxoHash{maybe_stats->hashSerialized} != au_data.hash_serialized) { - return util::Error{strprintf(Untranslated("Bad snapshot content hash: expected %s, got %s"), - au_data.hash_serialized.ToString(), maybe_stats->hashSerialized.ToString())}; + return util::Error{Untranslated(strprintf("Bad snapshot content hash: expected %s, got %s", + au_data.hash_serialized.ToString(), maybe_stats->hashSerialized.ToString()))}; } snapshot_chainstate.m_chain.SetTip(*snapshot_start_block); @@ -6188,7 +6208,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() auto rename_result = m_snapshot_chainstate->InvalidateCoinsDBOnDisk(); if (!rename_result) { - user_error = strprintf(Untranslated("%s\n%s"), user_error, util::ErrorString(rename_result)); + user_error += Untranslated("\n") + util::ErrorString(rename_result); } GetNotifications().fatalError(user_error); @@ -6340,7 +6360,7 @@ static ChainstateManager::Options&& Flatten(ChainstateManager::Options&& opts) } ChainstateManager::ChainstateManager(const util::SignalInterrupt& interrupt, Options options, node::BlockManager::Options blockman_options) - : m_script_check_queue{/*batch_size=*/128, options.worker_threads_num}, + : m_script_check_queue{/*batch_size=*/128, std::clamp(options.worker_threads_num, 0, MAX_SCRIPTCHECK_THREADS)}, m_interrupt{interrupt}, m_options{Flatten(std::move(options))}, m_blockman{interrupt, std::move(blockman_options)}, diff --git a/src/validation.h b/src/validation.h index 2f38c70802..be4a9fb169 100644 --- a/src/validation.h +++ b/src/validation.h @@ -78,6 +78,9 @@ static constexpr int DEFAULT_CHECKLEVEL{3}; // Setting the target to >= 550 MiB will make it likely we can respect the target. static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024; +/** Maximum number of dedicated script-checking threads allowed */ +static constexpr int MAX_SCRIPTCHECK_THREADS{15}; + /** Current sync state passed to tip changed callbacks. */ enum class SynchronizationState { INIT_REINDEX, @@ -335,7 +338,6 @@ class CScriptCheck unsigned int nIn; unsigned int nFlags; bool cacheStore; - ScriptError error{SCRIPT_ERR_UNKNOWN_ERROR}; PrecomputedTransactionData *txdata; SignatureCache* m_signature_cache; @@ -348,9 +350,7 @@ class CScriptCheck CScriptCheck(CScriptCheck&&) = default; CScriptCheck& operator=(CScriptCheck&&) = default; - bool operator()(); - - ScriptError GetScriptError() const { return error; } + std::optional> operator()(); }; // CScriptCheck is used a lot in std::vector, make sure that's efficient diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 3184d0f3b0..b875461c9f 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -24,29 +24,29 @@ namespace wallet { static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWalletTx& wtx, bool require_mine, std::vector& errors) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { if (wallet.HasWalletSpend(wtx.tx)) { - errors.push_back(Untranslated("Transaction has descendants in the wallet")); + errors.emplace_back(Untranslated("Transaction has descendants in the wallet")); return feebumper::Result::INVALID_PARAMETER; } { if (wallet.chain().hasDescendantsInMempool(wtx.GetHash())) { - errors.push_back(Untranslated("Transaction has descendants in the mempool")); + errors.emplace_back(Untranslated("Transaction has descendants in the mempool")); return feebumper::Result::INVALID_PARAMETER; } } if (wallet.GetTxDepthInMainChain(wtx) != 0) { - errors.push_back(Untranslated("Transaction has been mined, or is conflicted with a mined transaction")); + errors.emplace_back(Untranslated("Transaction has been mined, or is conflicted with a mined transaction")); return feebumper::Result::WALLET_ERROR; } if (!SignalsOptInRBF(*wtx.tx)) { - errors.push_back(Untranslated("Transaction is not BIP 125 replaceable")); + errors.emplace_back(Untranslated("Transaction is not BIP 125 replaceable")); return feebumper::Result::WALLET_ERROR; } if (wtx.mapValue.count("replaced_by_txid")) { - errors.push_back(strprintf(Untranslated("Cannot bump transaction %s which was already bumped by transaction %s"), wtx.GetHash().ToString(), wtx.mapValue.at("replaced_by_txid"))); + errors.push_back(Untranslated(strprintf("Cannot bump transaction %s which was already bumped by transaction %s", wtx.GetHash().ToString(), wtx.mapValue.at("replaced_by_txid")))); return feebumper::Result::WALLET_ERROR; } @@ -55,7 +55,7 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet // if not, we can't bump the fee, because the wallet has no way of knowing the value of the other inputs (thus the fee) isminefilter filter = wallet.GetLegacyScriptPubKeyMan() && wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE; if (!AllInputsMine(wallet, *wtx.tx, filter)) { - errors.push_back(Untranslated("Transaction contains inputs that don't belong to this wallet")); + errors.emplace_back(Untranslated("Transaction contains inputs that don't belong to this wallet")); return feebumper::Result::WALLET_ERROR; } } @@ -74,10 +74,10 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans CFeeRate minMempoolFeeRate = wallet.chain().mempoolMinFee(); if (newFeerate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) { - errors.push_back(strprintf( - Untranslated("New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool -- "), + errors.push_back(Untranslated( + strprintf("New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool -- ", FormatMoney(newFeerate.GetFeePerK()), - FormatMoney(minMempoolFeeRate.GetFeePerK()))); + FormatMoney(minMempoolFeeRate.GetFeePerK())))); return feebumper::Result::WALLET_ERROR; } @@ -89,7 +89,7 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans std::optional combined_bump_fee = wallet.chain().calculateCombinedBumpFee(reused_inputs, newFeerate); if (!combined_bump_fee.has_value()) { - errors.push_back(strprintf(Untranslated("Failed to calculate bump fees, because unconfirmed UTXOs depend on enormous cluster of unconfirmed transactions."))); + errors.push_back(Untranslated(strprintf("Failed to calculate bump fees, because unconfirmed UTXOs depend on enormous cluster of unconfirmed transactions."))); } CAmount new_total_fee = newFeerate.GetFee(maxTxSize) + combined_bump_fee.value(); @@ -99,23 +99,23 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans CAmount minTotalFee = old_fee + incrementalRelayFee.GetFee(maxTxSize); if (new_total_fee < minTotalFee) { - errors.push_back(strprintf(Untranslated("Insufficient total fee %s, must be at least %s (oldFee %s + incrementalFee %s)"), - FormatMoney(new_total_fee), FormatMoney(minTotalFee), FormatMoney(old_fee), FormatMoney(incrementalRelayFee.GetFee(maxTxSize)))); + errors.push_back(Untranslated(strprintf("Insufficient total fee %s, must be at least %s (oldFee %s + incrementalFee %s)", + FormatMoney(new_total_fee), FormatMoney(minTotalFee), FormatMoney(old_fee), FormatMoney(incrementalRelayFee.GetFee(maxTxSize))))); return feebumper::Result::INVALID_PARAMETER; } CAmount requiredFee = GetRequiredFee(wallet, maxTxSize); if (new_total_fee < requiredFee) { - errors.push_back(strprintf(Untranslated("Insufficient total fee (cannot be less than required fee %s)"), - FormatMoney(requiredFee))); + errors.push_back(Untranslated(strprintf("Insufficient total fee (cannot be less than required fee %s)", + FormatMoney(requiredFee)))); return feebumper::Result::INVALID_PARAMETER; } // Check that in all cases the new fee doesn't violate maxTxFee const CAmount max_tx_fee = wallet.m_default_max_tx_fee; if (new_total_fee > max_tx_fee) { - errors.push_back(strprintf(Untranslated("Specified or calculated fee %s is too high (cannot be higher than -maxtxfee %s)"), - FormatMoney(new_total_fee), FormatMoney(max_tx_fee))); + errors.push_back(Untranslated(strprintf("Specified or calculated fee %s is too high (cannot be higher than -maxtxfee %s)", + FormatMoney(new_total_fee), FormatMoney(max_tx_fee)))); return feebumper::Result::WALLET_ERROR; } @@ -167,7 +167,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo { // For now, cannot specify both new outputs to use and an output index to send change if (!outputs.empty() && original_change_index.has_value()) { - errors.push_back(Untranslated("The options 'outputs' and 'original_change_index' are incompatible. You can only either specify a new set of outputs, or designate a change output to be recycled.")); + errors.emplace_back(Untranslated("The options 'outputs' and 'original_change_index' are incompatible. You can only either specify a new set of outputs, or designate a change output to be recycled.")); return Result::INVALID_PARAMETER; } @@ -178,14 +178,14 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo errors.clear(); auto it = wallet.mapWallet.find(txid); if (it == wallet.mapWallet.end()) { - errors.push_back(Untranslated("Invalid or non-wallet transaction id")); + errors.emplace_back(Untranslated("Invalid or non-wallet transaction id")); return Result::INVALID_ADDRESS_OR_KEY; } const CWalletTx& wtx = it->second; // Make sure that original_change_index is valid if (original_change_index.has_value() && original_change_index.value() >= wtx.tx->vout.size()) { - errors.push_back(Untranslated("Change position is out of range")); + errors.emplace_back(Untranslated("Change position is out of range")); return Result::INVALID_PARAMETER; } @@ -201,7 +201,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo for (const CTxIn& txin : wtx.tx->vin) { const Coin& coin = coins.at(txin.prevout); if (coin.out.IsNull()) { - errors.push_back(Untranslated(strprintf("%s:%u is already spent", txin.prevout.hash.GetHex(), txin.prevout.n))); + errors.emplace_back(Untranslated(strprintf("%s:%u is already spent", txin.prevout.hash.GetHex(), txin.prevout.n))); return Result::MISC_ERROR; } PreselectedInput& preset_txin = new_coin_control.Select(txin.prevout); @@ -319,7 +319,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo auto res = CreateTransaction(wallet, recipients, /*change_pos=*/std::nullopt, new_coin_control, false); if (!res) { - errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res)); + errors.emplace_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res)); return Result::WALLET_ERROR; } @@ -361,7 +361,7 @@ Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransacti } auto it = txid.IsNull() ? wallet.mapWallet.end() : wallet.mapWallet.find(txid); if (it == wallet.mapWallet.end()) { - errors.push_back(Untranslated("Invalid or non-wallet transaction id")); + errors.emplace_back(Untranslated("Invalid or non-wallet transaction id")); return Result::MISC_ERROR; } const CWalletTx& oldWtx = it->second; @@ -382,7 +382,7 @@ Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransacti // mark the original tx as bumped bumped_txid = tx->GetHash(); if (!wallet.MarkReplaced(oldWtx.GetHash(), bumped_txid)) { - errors.push_back(Untranslated("Created new bumpfee transaction but could not mark the original transaction as replaced")); + errors.emplace_back(Untranslated("Created new bumpfee transaction but could not mark the original transaction as replaced")); } return Result::OK; } diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 835b9b1b9f..bfd7249c04 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2009-2022 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. @@ -549,7 +549,7 @@ RPCHelpMan importwallet() continue; CKey key = DecodeSecret(vstr[0]); if (key.IsValid()) { - int64_t nTime = ParseISO8601DateTime(vstr[1]); + int64_t nTime{ParseISO8601DateTime(vstr[1]).value_or(0)}; std::string strLabel; bool fLabel = true; for (unsigned int nStr = 2; nStr < vstr.size(); nStr++) { @@ -569,7 +569,7 @@ RPCHelpMan importwallet() } else if(IsHex(vstr[0])) { std::vector vData(ParseHex(vstr[0])); CScript script = CScript(vData.begin(), vData.end()); - int64_t birth_time = ParseISO8601DateTime(vstr[1]); + int64_t birth_time{ParseISO8601DateTime(vstr[1]).value_or(0)}; if (birth_time > 0) nTimeBegin = std::min(nTimeBegin, birth_time); scripts.emplace_back(script, birth_time); } diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index ec3b7c1085..414f0deeb2 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2011-2022 The Bitcoin Core developers +// Copyright (c) 2011-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. @@ -14,26 +14,10 @@ #include #include -#include - namespace wallet { static const std::string WALLET_ENDPOINT_BASE = "/wallet/"; const std::string HELP_REQUIRING_PASSPHRASE{"\nRequires wallet passphrase to be set with walletpassphrase call if wallet is encrypted.\n"}; -int64_t ParseISO8601DateTime(const std::string& str) -{ - static const boost::posix_time::ptime epoch = boost::posix_time::from_time_t(0); - static const std::locale loc(std::locale::classic(), - new boost::posix_time::time_input_facet("%Y-%m-%dT%H:%M:%SZ")); - std::istringstream iss(str); - iss.imbue(loc); - boost::posix_time::ptime ptime(boost::date_time::not_a_date_time); - iss >> ptime; - if (ptime.is_not_a_date_time() || epoch > ptime) - return 0; - return (ptime - epoch).total_seconds(); -} - bool GetAvoidReuseFlag(const CWallet& wallet, const UniValue& param) { bool can_avoid_reuse = wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE); bool avoid_reuse = param.isNull() ? can_avoid_reuse : param.get_bool(); diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h index 2fdba04352..002d0355e5 100644 --- a/src/wallet/rpc/util.h +++ b/src/wallet/rpc/util.h @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2022 The Bitcoin Core developers +// Copyright (c) 2017-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. @@ -51,7 +51,6 @@ std::string LabelFromValue(const UniValue& value); void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey, UniValue& entry); void HandleWalletError(const std::shared_ptr wallet, DatabaseStatus& status, bilingual_str& error); -int64_t ParseISO8601DateTime(const std::string& str); void AppendLastProcessedBlock(UniValue& entry, const CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); } // namespace wallet diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index 0ac1b66897..b924239073 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -100,7 +100,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil newFilename.c_str(), DB_AUTO_COMMIT); if (result != 0) { - error = strprintf(Untranslated("Failed to rename %s to %s"), filename, newFilename); + error = Untranslated(strprintf("Failed to rename %s to %s", filename, newFilename)); return false; } @@ -117,10 +117,10 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil Db db(env->dbenv.get(), 0); result = db.verify(newFilename.c_str(), nullptr, &strDump, DB_SALVAGE | DB_AGGRESSIVE); if (result == DB_VERIFY_BAD) { - warnings.push_back(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable.")); + warnings.emplace_back(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable.")); } if (result != 0 && result != DB_VERIFY_BAD) { - error = strprintf(Untranslated("Salvage: Database salvage failed with result %d."), result); + error = Untranslated(strprintf("Salvage: Database salvage failed with result %d.", result)); return false; } @@ -144,7 +144,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil break; getline(strDump, valueHex); if (valueHex == DATA_END) { - warnings.push_back(Untranslated("Salvage: WARNING: Number of keys in data does not match number of values.")); + warnings.emplace_back(Untranslated("Salvage: WARNING: Number of keys in data does not match number of values.")); break; } salvagedData.emplace_back(ParseHex(keyHex), ParseHex(valueHex)); @@ -153,7 +153,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil bool fSuccess; if (keyHex != DATA_END) { - warnings.push_back(Untranslated("Salvage: WARNING: Unexpected end of file while reading salvage output.")); + warnings.emplace_back(Untranslated("Salvage: WARNING: Unexpected end of file while reading salvage output.")); fSuccess = false; } else { fSuccess = (result == 0); @@ -161,7 +161,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil if (salvagedData.empty()) { - error = strprintf(Untranslated("Salvage(aggressive) found no records in %s."), newFilename); + error = Untranslated(strprintf("Salvage(aggressive) found no records in %s.", newFilename)); return false; } @@ -173,7 +173,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil DB_CREATE, // Flags 0); if (ret > 0) { - error = strprintf(Untranslated("Cannot create database file %s"), filename); + error = Untranslated(strprintf("Cannot create database file %s", filename)); pdbCopy->close(0); return false; } @@ -204,7 +204,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil if (!fReadOK) { - warnings.push_back(strprintf(Untranslated("WARNING: WalletBatch::Recover skipping %s: %s"), strType, strErr)); + warnings.push_back(Untranslated(strprintf("WARNING: WalletBatch::Recover skipping %s: %s", strType, strErr))); continue; } Dbt datKey(row.first.data(), row.first.size()); diff --git a/src/wallet/test/CMakeLists.txt b/src/wallet/test/CMakeLists.txt index 7332674242..8b442b262b 100644 --- a/src/wallet/test/CMakeLists.txt +++ b/src/wallet/test/CMakeLists.txt @@ -14,7 +14,6 @@ target_sources(test_bitcoin init_tests.cpp ismine_tests.cpp psbt_wallet_tests.cpp - rpc_util_tests.cpp scriptpubkeyman_tests.cpp spend_tests.cpp wallet_crypto_tests.cpp diff --git a/src/wallet/test/fuzz/CMakeLists.txt b/src/wallet/test/fuzz/CMakeLists.txt index 4e663977c2..7b071e2f12 100644 --- a/src/wallet/test/fuzz/CMakeLists.txt +++ b/src/wallet/test/fuzz/CMakeLists.txt @@ -9,7 +9,6 @@ target_sources(fuzz crypter.cpp fees.cpp $<$:${CMAKE_CURRENT_LIST_DIR}/notifications.cpp> - parse_iso8601.cpp $<$:${CMAKE_CURRENT_LIST_DIR}/scriptpubkeyman.cpp> spend.cpp wallet_bdb_parser.cpp diff --git a/src/wallet/test/rpc_util_tests.cpp b/src/wallet/test/rpc_util_tests.cpp deleted file mode 100644 index 32f6f5ab46..0000000000 --- a/src/wallet/test/rpc_util_tests.cpp +++ /dev/null @@ -1,24 +0,0 @@ -// Copyright (c) 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. - -#include - -#include - -namespace wallet { - -BOOST_AUTO_TEST_SUITE(wallet_util_tests) - -BOOST_AUTO_TEST_CASE(util_ParseISO8601DateTime) -{ - BOOST_CHECK_EQUAL(ParseISO8601DateTime("1970-01-01T00:00:00Z"), 0); - BOOST_CHECK_EQUAL(ParseISO8601DateTime("1960-01-01T00:00:00Z"), 0); - BOOST_CHECK_EQUAL(ParseISO8601DateTime("2000-01-01T00:00:01Z"), 946684801); - BOOST_CHECK_EQUAL(ParseISO8601DateTime("2011-09-30T23:36:17Z"), 1317425777); - BOOST_CHECK_EQUAL(ParseISO8601DateTime("2100-12-31T23:59:59Z"), 4133980799); -} - -BOOST_AUTO_TEST_SUITE_END() - -} // namespace wallet diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 82c0d2caf1..0961aebc7c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -290,7 +290,7 @@ std::shared_ptr LoadWalletInternal(WalletContext& context, const std::s // Legacy wallets are being deprecated, warn if the loaded wallet is legacy if (!wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { - warnings.push_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet.")); + warnings.emplace_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet.")); } NotifyWalletLoaded(context, wallet); @@ -483,7 +483,7 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& // Legacy wallets are being deprecated, warn if a newly created wallet is legacy if (!(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) { - warnings.push_back(_("Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future.")); + warnings.emplace_back(_("Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future.")); } status = DatabaseStatus::SUCCESS; @@ -519,7 +519,7 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b } catch (const std::exception& e) { assert(!wallet); if (!error.empty()) error += Untranslated("\n"); - error += strprintf(Untranslated("Unexpected exception: %s"), e.what()); + error += Untranslated(strprintf("Unexpected exception: %s", e.what())); } if (!wallet) { fs::remove_all(wallet_path); @@ -4407,6 +4407,9 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle if (!wallet_path) { return util::Error{util::ErrorString(wallet_path)}; } + if (!fs::exists(*wallet_path)) { + return util::Error{_("Error: Wallet does not exist")}; + } if (!IsBDBFile(BDBDataFile(*wallet_path))) { return util::Error{_("Error: This wallet is already a descriptor wallet")}; } diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index b09a0e40eb..368415da12 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1013,7 +1013,7 @@ static DBErrors LoadAddressBookRecords(CWallet* pwallet, DatabaseBatch& batch) E // "1" or "p" for present (which was written prior to // f5ba424cd44619d9b9be88b8593d69a7ba96db26). pwallet->LoadAddressPreviouslySpent(dest); - } else if (strKey.compare(0, 2, "rr") == 0) { + } else if (strKey.starts_with("rr")) { // Load "rr##" keys where ## is a decimal number, and strValue // is a serialized RecentRequestEntry object. pwallet->LoadAddressReceiveRequest(dest, strKey.substr(2), strValue); diff --git a/test/functional/combine_logs.py b/test/functional/combine_logs.py index 33c81bde13..998cb20831 100755 --- a/test/functional/combine_logs.py +++ b/test/functional/combine_logs.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -# Copyright (c) 2017-2021 The Bitcoin Core developers +# Copyright (c) 2017-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. """Combine logs from multiple bitcoin nodes as well as the test_framework log. @@ -79,14 +79,16 @@ def read_logs(tmp_dir): Delegates to generator function get_log_events() to provide individual log events for each of the input log files.""" - # Find out what the folder is called that holds the debug.log file - glob = pathlib.Path(tmp_dir).glob('node0/**/debug.log') - path = next(glob, None) - if path: - assert next(glob, None) is None # more than one debug.log, should never happen - chain = re.search(r'node0/(.+?)/debug\.log$', path.as_posix()).group(1) # extract the chain name - else: - chain = 'regtest' # fallback to regtest (should only happen when none exists) + # Find out what the folder is called that holds node 0's debug.log file + debug_logs = list(pathlib.Path(tmp_dir).glob('node0/**/debug.log')) + match len(debug_logs): + case 0: + chain = 'regtest' # fallback to regtest + case 1: + chain = re.search(r'node0/(.+?)/debug\.log$', debug_logs[0].as_posix()).group(1) + case _: + raise RuntimeError('Max one debug.log is supported, found several:\n\t' + + '\n\t'.join(map(str, debug_logs))) files = [("test", "%s/test_framework.log" % tmp_dir)] for i in itertools.count(): diff --git a/test/functional/feature_anchors.py b/test/functional/feature_anchors.py index 5ca46c1366..154461e739 100755 --- a/test/functional/feature_anchors.py +++ b/test/functional/feature_anchors.py @@ -102,7 +102,7 @@ def run_test(self): self.nodes[0].addconnection(ONION_ADDR, 'block-relay-only', v2transport=False) self.log.debug("Stop node") - with self.nodes[0].assert_debug_log([f"DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat"]): + with self.nodes[0].assert_debug_log(["DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat"]): self.stop_node(0) # Manually close keep_alive proxy connection onion_proxy.stop() diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py index 882c7a6e75..24adb01a08 100755 --- a/test/functional/feature_assumeutxo.py +++ b/test/functional/feature_assumeutxo.py @@ -165,8 +165,8 @@ def expected_error(log_msg="", error_msg=""): with self.nodes[0].assert_debug_log([log_msg]): self.nodes[0].assert_start_raises_init_error(expected_msg=error_msg) - expected_error_msg = f"Error: A fatal internal error occurred, see debug.log for details: Assumeutxo data not found for the given blockhash '7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a'." - error_details = f"Assumeutxo data not found for the given blockhash" + expected_error_msg = "Error: A fatal internal error occurred, see debug.log for details: Assumeutxo data not found for the given blockhash '7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a'." + error_details = "Assumeutxo data not found for the given blockhash" expected_error(log_msg=error_details, error_msg=expected_error_msg) # resurrect node again @@ -417,7 +417,7 @@ def check_dump_output(output): assert_equal(n0.getblockchaininfo()["blocks"], FINAL_HEIGHT) - self.log.info(f"Check that dumptxoutset works for past block heights") + self.log.info("Check that dumptxoutset works for past block heights") # rollback defaults to the snapshot base height dump_output2 = n0.dumptxoutset('utxos2.dat', "rollback") check_dump_output(dump_output2) diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py index 5e78a6fb88..f024f30866 100755 --- a/test/functional/feature_block.py +++ b/test/functional/feature_block.py @@ -85,7 +85,6 @@ def set_test_params(self): self.extra_args = [[ '-acceptnonstdtxn=1', # This is a consensus block test, we don't care about tx policy '-testactivationheight=bip34@2', - '-par=1', # Until https://github.com/bitcoin/bitcoin/issues/30960 is fixed ]] def run_test(self): diff --git a/test/functional/feature_cltv.py b/test/functional/feature_cltv.py index 0839acc8ea..1838d8f54d 100755 --- a/test/functional/feature_cltv.py +++ b/test/functional/feature_cltv.py @@ -87,7 +87,6 @@ def set_test_params(self): self.noban_tx_relay = True self.extra_args = [[ f'-testactivationheight=cltv@{CLTV_HEIGHT}', - '-par=1', # Use only one script thread to get the exact reject reason for testing '-acceptnonstdtxn=1', # cltv_invalidate is nonstandard ]] self.setup_clean_chain = True @@ -175,7 +174,7 @@ def run_test(self): block.hashMerkleRoot = block.calc_merkle_root() block.solve() - with self.nodes[0].assert_debug_log(expected_msgs=[f'CheckInputScripts on {block.vtx[-1].hash} failed with {expected_cltv_reject_reason}']): + with self.nodes[0].assert_debug_log(expected_msgs=[f'Block validation error: {expected_cltv_reject_reason}']): peer.send_and_ping(msg_block(block)) assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip) peer.sync_with_ping() diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index 44c7edf962..31847b3fbf 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -# Copyright (c) 2017-2022 The Bitcoin Core developers +# Copyright (c) 2017-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. """Test various command line arguments and configuration file parameters.""" @@ -27,13 +27,78 @@ def set_test_params(self): self.wallet_names = [] self.disable_autoconnect = False + # Overridden to avoid attempt to sync not yet started nodes. + def setup_network(self): + self.setup_nodes() + + # Overridden to not start nodes automatically - doing so is the + # responsibility of each test function. + def setup_nodes(self): + self.add_nodes(self.num_nodes, self.extra_args) + # Ensure a log file exists as TestNode.assert_debug_log() expects it. + self.nodes[0].debug_log_path.parent.mkdir() + self.nodes[0].debug_log_path.touch() + + def test_dir_config(self): + self.log.info('Error should be emitted if config file is a directory') + conf_path = self.nodes[0].datadir_path / 'bitcoin.conf' + os.rename(conf_path, conf_path.with_suffix('.confbkp')) + conf_path.mkdir() + self.stop_node(0) + self.nodes[0].assert_start_raises_init_error( + extra_args=['-regtest'], + expected_msg=f'Error: Error reading configuration file: Config file "{conf_path}" is a directory.', + ) + conf_path.rmdir() + os.rename(conf_path.with_suffix('.confbkp'), conf_path) + + self.log.debug('Verifying includeconf directive pointing to directory is caught') + with open(conf_path, 'a', encoding='utf-8') as conf: + conf.write(f'includeconf={self.nodes[0].datadir_path}\n') + self.nodes[0].assert_start_raises_init_error( + extra_args=['-regtest'], + expected_msg=f'Error: Error reading configuration file: Included config file "{self.nodes[0].datadir_path}" is a directory.', + ) + + self.nodes[0].replace_in_config([(f'includeconf={self.nodes[0].datadir_path}', '')]) + + def test_negated_config(self): + self.log.info('Disabling configuration via -noconf') + + conf_path = self.nodes[0].datadir_path / 'bitcoin.conf' + with open(conf_path, encoding='utf-8') as conf: + settings = [f'-{line.rstrip()}' for line in conf if len(line) > 1 and line[0] != '['] + os.rename(conf_path, conf_path.with_suffix('.confbkp')) + + self.log.debug('Verifying garbage in config can be detected') + with open(conf_path, 'a', encoding='utf-8') as conf: + conf.write('garbage\n') + self.nodes[0].assert_start_raises_init_error( + extra_args=['-regtest'], + expected_msg='Error: Error reading configuration file: parse error on line 1: garbage', + ) + + self.log.debug('Verifying that disabling of the config file means garbage inside of it does ' \ + 'not prevent the node from starting, and message about existing config file is logged') + ignored_file_message = [f'[InitConfig] Data directory "{self.nodes[0].datadir_path}" contains a "bitcoin.conf" file which is explicitly ignored using -noconf.'] + with self.nodes[0].assert_debug_log(timeout=60, expected_msgs=ignored_file_message): + self.start_node(0, extra_args=settings + ['-noconf']) + self.stop_node(0) + + self.log.debug('Verifying no message appears when removing config file') + os.remove(conf_path) + with self.nodes[0].assert_debug_log(timeout=60, expected_msgs=[], unexpected_msgs=ignored_file_message): + self.start_node(0, extra_args=settings + ['-noconf']) + self.stop_node(0) + + os.rename(conf_path.with_suffix('.confbkp'), conf_path) + def test_config_file_parser(self): self.log.info('Test config file parser') - self.stop_node(0) # Check that startup fails if conf= is set in bitcoin.conf or in an included conf file bad_conf_file_path = self.nodes[0].datadir_path / "bitcoin_bad.conf" - util.write_config(bad_conf_file_path, n=0, chain='', extra_config=f'conf=some.conf\n') + util.write_config(bad_conf_file_path, n=0, chain='', extra_config='conf=some.conf\n') conf_in_config_file_err = 'Error: Error reading configuration file: conf cannot be set in the configuration file; use includeconf= if you want to include additional config files' self.nodes[0].assert_start_raises_init_error( extra_args=[f'-conf={bad_conf_file_path}'], @@ -162,12 +227,11 @@ def test_invalid_command_line_options(self): ) def test_log_buffer(self): - self.stop_node(0) with self.nodes[0].assert_debug_log(expected_msgs=['Warning: parsed potentially confusing double-negative -connect=0\n']): self.start_node(0, extra_args=['-noconnect=0']) + self.stop_node(0) def test_args_log(self): - self.stop_node(0) self.log.info('Test config args logging') with self.nodes[0].assert_debug_log( expected_msgs=[ @@ -196,10 +260,10 @@ def test_args_log(self): '-rpcuser=secret-rpcuser', '-torpassword=secret-torpassword', ]) + self.stop_node(0) def test_networkactive(self): self.log.info('Test -networkactive option') - self.stop_node(0) with self.nodes[0].assert_debug_log(expected_msgs=['SetNetworkActive: true\n']): self.start_node(0) @@ -222,16 +286,12 @@ def test_networkactive(self): self.stop_node(0) with self.nodes[0].assert_debug_log(expected_msgs=['SetNetworkActive: false\n']): self.start_node(0, extra_args=['-nonetworkactive=1']) + self.stop_node(0) def test_seed_peers(self): self.log.info('Test seed peers') default_data_dir = self.nodes[0].datadir_path peer_dat = default_data_dir / 'peers.dat' - # Only regtest has no fixed seeds. To avoid connections to random - # nodes, regtest is the only network where it is safe to enable - # -fixedseeds in tests - util.assert_equal(self.nodes[0].getblockchaininfo()['chain'],'regtest') - self.stop_node(0) # No peers.dat exists and -dnsseed=1 # We expect the node will use DNS Seeds, but Regtest mode does not have @@ -248,6 +308,12 @@ def test_seed_peers(self): timeout=10, ): self.start_node(0, extra_args=['-dnsseed=1', '-fixedseeds=1', f'-mocktime={start}']) + + # Only regtest has no fixed seeds. To avoid connections to random + # nodes, regtest is the only network where it is safe to enable + # -fixedseeds in tests + util.assert_equal(self.nodes[0].getblockchaininfo()['chain'],'regtest') + with self.nodes[0].assert_debug_log(expected_msgs=[ "Adding fixed seeds as 60 seconds have passed and addrman is empty", ]): @@ -294,13 +360,13 @@ def test_seed_peers(self): "Adding fixed seeds as 60 seconds have passed and addrman is empty", ]): self.nodes[0].setmocktime(start + 65) + self.stop_node(0) def test_connect_with_seednode(self): self.log.info('Test -connect with -seednode') seednode_ignored = ['-seednode is ignored when -connect is used\n'] dnsseed_ignored = ['-dnsseed is ignored when -connect is used and -proxy is specified\n'] addcon_thread_started = ['addcon thread start\n'] - self.stop_node(0) # When -connect is supplied, expanding addrman via getaddr calls to ADDR_FETCH(-seednode) # nodes is irrelevant and -seednode is ignored. @@ -325,6 +391,7 @@ def test_connect_with_seednode(self): with self.nodes[0].assert_debug_log(expected_msgs=addcon_thread_started, unexpected_msgs=seednode_ignored): self.restart_node(0, extra_args=[connect_arg, '-seednode=fakeaddress2']) + self.stop_node(0) def test_ignored_conf(self): self.log.info('Test error is triggered when the datadir in use contains a bitcoin.conf file that would be ignored ' @@ -423,6 +490,8 @@ def run_test(self): self.test_networkactive() self.test_connect_with_seednode() + self.test_dir_config() + self.test_negated_config() self.test_config_file_parser() self.test_config_file_log() self.test_invalid_command_line_options() diff --git a/test/functional/feature_csv_activation.py b/test/functional/feature_csv_activation.py index df02bcc6ad..d5b6d0dba9 100755 --- a/test/functional/feature_csv_activation.py +++ b/test/functional/feature_csv_activation.py @@ -99,7 +99,6 @@ def set_test_params(self): self.noban_tx_relay = True self.extra_args = [[ f'-testactivationheight=csv@{CSV_ACTIVATION_HEIGHT}', - '-par=1', # Use only one script thread to get the exact reject reason for testing ]] self.supports_cli = False diff --git a/test/functional/feature_dersig.py b/test/functional/feature_dersig.py index c83650fa33..e0b7fb78df 100755 --- a/test/functional/feature_dersig.py +++ b/test/functional/feature_dersig.py @@ -51,7 +51,6 @@ def set_test_params(self): self.noban_tx_relay = True self.extra_args = [[ f'-testactivationheight=dersig@{DERSIG_HEIGHT}', - '-par=1', # Use only one script thread to get the exact log msg for testing ]] self.setup_clean_chain = True self.rpc_timeout = 240 @@ -131,7 +130,7 @@ def run_test(self): block.hashMerkleRoot = block.calc_merkle_root() block.solve() - with self.nodes[0].assert_debug_log(expected_msgs=[f'CheckInputScripts on {block.vtx[-1].hash} failed with mandatory-script-verify-flag-failed (Non-canonical DER signature)']): + with self.nodes[0].assert_debug_log(expected_msgs=['Block validation error: mandatory-script-verify-flag-failed (Non-canonical DER signature)']): peer.send_and_ping(msg_block(block)) assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip) peer.sync_with_ping() diff --git a/test/functional/feature_framework_miniwallet.py b/test/functional/feature_framework_miniwallet.py index f723f7f31e..464f18eb48 100755 --- a/test/functional/feature_framework_miniwallet.py +++ b/test/functional/feature_framework_miniwallet.py @@ -34,7 +34,7 @@ def test_tx_padding(self): def test_wallet_tagging(self): """Verify that tagged wallet instances are able to send funds.""" - self.log.info(f"Test tagged wallet instances...") + self.log.info("Test tagged wallet instances...") node = self.nodes[0] untagged_wallet = self.wallets[0][1] for i in range(10): diff --git a/test/functional/feature_loadblock.py b/test/functional/feature_loadblock.py index 1519c132b9..0e6ec14c77 100755 --- a/test/functional/feature_loadblock.py +++ b/test/functional/feature_loadblock.py @@ -51,8 +51,8 @@ def run_test(self): cfg.write(f"port={node_url.port}\n") cfg.write(f"host={node_url.hostname}\n") cfg.write(f"output_file={bootstrap_file}\n") - cfg.write(f"max_height=100\n") - cfg.write(f"netmagic=fabfb5da\n") + cfg.write("max_height=100\n") + cfg.write("netmagic=fabfb5da\n") cfg.write(f"input={blocks_dir}\n") cfg.write(f"genesis={genesis_block}\n") cfg.write(f"hashlist={hash_list.name}\n") diff --git a/test/functional/feature_nulldummy.py b/test/functional/feature_nulldummy.py index a53f78c13d..885bc4855b 100755 --- a/test/functional/feature_nulldummy.py +++ b/test/functional/feature_nulldummy.py @@ -57,7 +57,6 @@ def set_test_params(self): self.extra_args = [[ f'-testactivationheight=segwit@{COINBASE_MATURITY + 5}', '-addresstype=legacy', - '-par=1', # Use only one script thread to get the exact reject reason for testing ]] def create_transaction(self, *, txid, input_details=None, addr, amount, privkey): diff --git a/test/functional/feature_taproot.py b/test/functional/feature_taproot.py index 443c1c33da..daf11a9ae6 100755 --- a/test/functional/feature_taproot.py +++ b/test/functional/feature_taproot.py @@ -1285,7 +1285,6 @@ def skip_test_if_missing_module(self): def set_test_params(self): self.num_nodes = 1 self.setup_clean_chain = True - self.extra_args = [["-par=1"]] def block_submit(self, node, txs, msg, err_msg, cb_pubkey=None, fees=0, sigops_weight=0, witness=False, accept=False): diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py index 3fe6570dd1..134b4e566b 100755 --- a/test/functional/interface_bitcoin_cli.py +++ b/test/functional/interface_bitcoin_cli.py @@ -164,6 +164,9 @@ def run_test(self): 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) + self.log.info("Test connecting without RPC cookie file and with password arg") + assert_equal(BLOCKS, self.nodes[0].cli('-norpccookiefile', f'-rpcuser={user}', f'-rpcpassword={password}').getblockcount()) + self.log.info("Test -getinfo with arguments fails") assert_raises_process_error(1, "-getinfo takes no arguments", self.nodes[0].cli('-getinfo').help) diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py index 9aa7190c79..bc3268cd9b 100755 --- a/test/functional/interface_rest.py +++ b/test/functional/interface_rest.py @@ -211,7 +211,7 @@ def run_test(self): self.test_rest_request(f"/getutxos/{spending[0]}-+1", ret_type=RetType.OBJ, status=400) self.test_rest_request(f"/getutxos/{spending[0]}--1", ret_type=RetType.OBJ, status=400) self.test_rest_request(f"/getutxos/{spending[0]}aa-1234", ret_type=RetType.OBJ, status=400) - self.test_rest_request(f"/getutxos/aa-1234", ret_type=RetType.OBJ, status=400) + self.test_rest_request("/getutxos/aa-1234", ret_type=RetType.OBJ, status=400) # Test limits long_uri = '/'.join([f"{txid}-{n_}" for n_ in range(20)]) diff --git a/test/functional/interface_usdt_net.py b/test/functional/interface_usdt_net.py index 7b55259b63..5468ddf858 100755 --- a/test/functional/interface_usdt_net.py +++ b/test/functional/interface_usdt_net.py @@ -40,7 +40,8 @@ MAX_MSG_TYPE_LENGTH, MAX_MSG_DATA_LENGTH ) + """ -#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; }) struct p2p_message { @@ -60,7 +61,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; } @@ -73,7 +74,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/test/functional/interface_usdt_utxocache.py b/test/functional/interface_usdt_utxocache.py index ad98a3a162..1617c580f3 100755 --- a/test/functional/interface_usdt_utxocache.py +++ b/test/functional/interface_usdt_utxocache.py @@ -393,7 +393,7 @@ def handle_utxocache_flush(_, data, __): bpf = BPF(text=utxocache_flushes_program, usdt_contexts=[ctx], debug=0, cflags=["-Wno-error=implicit-function-declaration"]) bpf["utxocache_flush"].open_perf_buffer(handle_utxocache_flush) - self.log.info(f"prune blockchain to trigger a flush for pruning") + self.log.info("prune blockchain to trigger a flush for pruning") expected_flushes.append({"mode": "NONE", "for_prune": True, "size": 0}) self.nodes[0].pruneblockchain(315) @@ -401,7 +401,7 @@ def handle_utxocache_flush(_, data, __): bpf.cleanup() self.log.info( - f"check that we don't expect additional flushes and that the handle_* function succeeded") + "check that we don't expect additional flushes and that the handle_* function succeeded") assert_equal(0, len(expected_flushes)) assert_equal(EXPECTED_HANDLE_FLUSH_SUCCESS, handle_flush_succeeds) diff --git a/test/functional/mempool_datacarrier.py b/test/functional/mempool_datacarrier.py index ed6ad8461a..48e636caa2 100755 --- a/test/functional/mempool_datacarrier.py +++ b/test/functional/mempool_datacarrier.py @@ -26,7 +26,7 @@ def set_test_params(self): [], ["-datacarrier=0"], ["-datacarrier=1", f"-datacarriersize={MAX_OP_RETURN_RELAY - 1}"], - ["-datacarrier=1", f"-datacarriersize=2"], + ["-datacarrier=1", "-datacarriersize=2"], ] def test_null_data_transaction(self, node: TestNode, data, success: bool) -> None: diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py index ff75428326..435b61f24f 100755 --- a/test/functional/mempool_truc.py +++ b/test/functional/mempool_truc.py @@ -231,7 +231,7 @@ def test_nondefault_package_limits(self): assert_greater_than_or_equal(TRUC_CHILD_MAX_VSIZE, tx_v3_child_large2["tx"].get_vsize()) assert_greater_than(tx_v3_parent_large2["tx"].get_vsize() + tx_v3_child_large2["tx"].get_vsize(), 10000) - assert_raises_rpc_error(-26, f"too-long-mempool-chain, exceeds ancestor size limit", node.sendrawtransaction, tx_v3_child_large2["hex"]) + assert_raises_rpc_error(-26, "too-long-mempool-chain, exceeds ancestor size limit", node.sendrawtransaction, tx_v3_child_large2["hex"]) self.check_mempool([tx_v3_parent_large2["txid"]]) @cleanup(extra_args=["-datacarriersize=1000"]) diff --git a/test/functional/mining_basic.py b/test/functional/mining_basic.py index 4c75a637be..e9b0442e4c 100755 --- a/test/functional/mining_basic.py +++ b/test/functional/mining_basic.py @@ -58,7 +58,12 @@ def assert_template(node, block, expect, rehash=True): class MiningTest(BitcoinTestFramework): def set_test_params(self): - self.num_nodes = 2 + self.num_nodes = 3 + self.extra_args = [ + [], + [], + ["-fastprune", "-prune=1"] + ] self.setup_clean_chain = True self.supports_cli = False @@ -171,6 +176,21 @@ def test_timewarp(self): bad_block.solve() node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()) + def test_pruning(self): + self.log.info("Test that submitblock stores previously pruned block") + prune_node = self.nodes[2] + self.generate(prune_node, 400, sync_fun=self.no_op) + pruned_block = prune_node.getblock(prune_node.getblockhash(2), verbosity=0) + pruned_height = prune_node.pruneblockchain(400) + assert_greater_than_or_equal(pruned_height, 2) + pruned_blockhash = prune_node.getblockhash(2) + + assert_raises_rpc_error(-1, 'Block not available (pruned data)', prune_node.getblock, pruned_blockhash) + + result = prune_node.submitblock(pruned_block) + assert_equal(result, "inconclusive") + assert_equal(prune_node.getblock(pruned_blockhash, verbosity=0), pruned_block) + def run_test(self): node = self.nodes[0] self.wallet = MiniWallet(node) @@ -242,9 +262,19 @@ def assert_submitblock(block, result_str_1, result_str_2=None): bad_block.vtx[0].rehash() assert_template(node, bad_block, 'bad-cb-missing') - self.log.info("submitblock: Test invalid coinbase transaction") - assert_raises_rpc_error(-22, "Block does not start with a coinbase", node.submitblock, CBlock().serialize().hex()) - assert_raises_rpc_error(-22, "Block does not start with a coinbase", node.submitblock, bad_block.serialize().hex()) + self.log.info("submitblock: Test bad input hash for coinbase transaction") + bad_block.solve() + assert_equal("bad-cb-missing", node.submitblock(hexdata=bad_block.serialize().hex())) + + self.log.info("submitblock: Test block with no transactions") + no_tx_block = copy.deepcopy(block) + no_tx_block.vtx.clear() + no_tx_block.hashMerkleRoot = 0 + no_tx_block.solve() + assert_equal("bad-blk-length", node.submitblock(hexdata=no_tx_block.serialize().hex())) + + self.log.info("submitblock: Test empty block") + assert_equal('high-hash', node.submitblock(hexdata=CBlock().serialize().hex())) self.log.info("getblocktemplate: Test truncated final transaction") assert_raises_rpc_error(-22, "Block decode failed", node.getblocktemplate, { @@ -379,6 +409,7 @@ def chain_tip(b_hash, *, status='headers-only', branchlen=1): self.test_blockmintxfee_parameter() self.test_timewarp() + self.test_pruning() if __name__ == '__main__': diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py index 2af51fefbd..fcc13d8ff1 100755 --- a/test/functional/p2p_compactblocks.py +++ b/test/functional/p2p_compactblocks.py @@ -734,12 +734,12 @@ def test_invalid_tx_in_compactblock(self, test_node): # Now send the compact block with all transactions prefilled, and # verify that we don't get disconnected. comp_block = HeaderAndShortIDs() - comp_block.initialize_from_block(block, prefill_list=[0, 1, 2, 3, 4], use_witness=True) + comp_block.initialize_from_block(block, prefill_list=list(range(len(block.vtx))), use_witness=True) msg = msg_cmpctblock(comp_block.to_p2p()) test_node.send_and_ping(msg) # Check that the tip didn't advance - assert int(node.getbestblockhash(), 16) is not block.sha256 + assert int(node.getbestblockhash(), 16) != block.sha256 test_node.sync_with_ping() # Helper for enabling cb announcements diff --git a/test/functional/p2p_handshake.py b/test/functional/p2p_handshake.py index 18307a2824..4148790c19 100755 --- a/test/functional/p2p_handshake.py +++ b/test/functional/p2p_handshake.py @@ -86,7 +86,7 @@ def run_test(self): DESIRABLE_SERVICE_FLAGS_PRUNED, expect_disconnect=False) self.log.info("Check that feeler connections get disconnected immediately") - with node.assert_debug_log([f"feeler connection completed"]): + with node.assert_debug_log(["feeler connection completed"]): self.add_outbound_connection(node, "feeler", NODE_NONE, wait_for_disconnect=True) self.log.info("Check that connecting to ourself leads to immediate disconnect") diff --git a/test/functional/p2p_ibd_stalling.py b/test/functional/p2p_ibd_stalling.py index fa07873929..4348bf7ee9 100755 --- a/test/functional/p2p_ibd_stalling.py +++ b/test/functional/p2p_ibd_stalling.py @@ -74,6 +74,7 @@ def run_test(self): self.log.info("Check that a staller does not get disconnected if the 1024 block lookahead buffer is filled") self.mocktime = int(time.time()) + 1 + node.setmocktime(self.mocktime) for id in range(NUM_PEERS): peers.append(node.add_outbound_p2p_connection(P2PStaller(stall_block), p2p_idx=id, connection_type="outbound-full-relay")) peers[-1].block_store = block_dict diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py index bcb4dd7f8d..0864deeb4f 100755 --- a/test/functional/p2p_orphan_handling.py +++ b/test/functional/p2p_orphan_handling.py @@ -339,7 +339,7 @@ def test_orphans_overlapping_parents(self): # The wtxid and txid need to be the same for the node to recognize that the missing input # and in-flight request for inflight_parent_AB are the same transaction. - assert_equal(inflight_parent_AB["txid"], inflight_parent_AB["tx"].getwtxid()) + assert_equal(inflight_parent_AB["txid"], inflight_parent_AB["wtxid"]) # Announce inflight_parent_AB and wait for getdata peer_txrequest.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=int(inflight_parent_AB["tx"].getwtxid(), 16))])) @@ -365,6 +365,10 @@ def test_orphans_overlapping_parents(self): peer_orphans.wait_for_parent_requests([int(missing_parent_B["txid"], 16)]) peer_orphans.assert_never_requested(int(inflight_parent_AB["txid"], 16)) + # But inflight_parent_AB will be requested eventually if original peer doesn't respond + node.bumpmocktime(GETDATA_TX_INTERVAL) + peer_orphans.wait_for_parent_requests([int(inflight_parent_AB["txid"], 16)]) + @cleanup def test_orphan_of_orphan(self): node = self.nodes[0] diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index f4c6b9cea4..ca3a8cd6d8 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -214,6 +214,9 @@ def set_test_params(self): self.noban_tx_relay = True # This test tests SegWit both pre and post-activation, so use the normal BIP9 activation. self.extra_args = [ + # -par=1 should not affect validation outcome or logging/reported failures. It is kept + # here to exercise the code path still (as it is distinct for multithread script + # validation). ["-acceptnonstdtxn=1", f"-testactivationheight=segwit@{SEGWIT_HEIGHT}", "-par=1"], ["-acceptnonstdtxn=0", f"-testactivationheight=segwit@{SEGWIT_HEIGHT}"], ] @@ -509,10 +512,6 @@ def test_v0_outputs_arent_spendable(self): # When the block is serialized without witness, validation fails because the transaction is # invalid (transactions are always validated with SCRIPT_VERIFY_WITNESS so a segwit v0 transaction # without a witness is invalid). - # Note: The reject reason for this failure could be - # 'block-validation-failed' (if script check threads > 1) or - # 'mandatory-script-verify-flag-failed (Witness program was passed an - # empty witness)' (otherwise). test_witness_block(self.nodes[0], self.test_node, block, accepted=False, with_witness=False, reason='mandatory-script-verify-flag-failed (Witness program was passed an empty witness)') @@ -1017,7 +1016,7 @@ def test_extra_witness_data(self): tx2.vout.append(CTxOut(tx.vout[0].nValue, CScript([OP_TRUE]))) tx2.wit.vtxinwit.extend([CTxInWitness(), CTxInWitness()]) tx2.wit.vtxinwit[0].scriptWitness.stack = [CScript([CScriptNum(1)]), CScript([CScriptNum(1)]), witness_script] - tx2.wit.vtxinwit[1].scriptWitness.stack = [CScript([OP_TRUE])] + tx2.wit.vtxinwit[1].scriptWitness.stack = [] block = self.build_next_block() self.update_witness_block_with_transactions(block, [tx2]) diff --git a/test/functional/rpc_dumptxoutset.py b/test/functional/rpc_dumptxoutset.py index 6edac9a21f..8a188a84cc 100755 --- a/test/functional/rpc_dumptxoutset.py +++ b/test/functional/rpc_dumptxoutset.py @@ -67,14 +67,14 @@ def run_test(self): assert_raises_rpc_error( -8, "Couldn't open file {}.incomplete for writing".format(invalid_path), node.dumptxoutset, invalid_path, "latest") - self.log.info(f"Test that dumptxoutset with unknown dump type fails") + self.log.info("Test that dumptxoutset with unknown dump type fails") assert_raises_rpc_error( -8, 'Invalid snapshot type "bogus" specified. Please specify "rollback" or "latest"', node.dumptxoutset, 'utxos.dat', "bogus") - self.log.info(f"Test that dumptxoutset failure does not leave the network activity suspended when it was on previously") + self.log.info("Test that dumptxoutset failure does not leave the network activity suspended when it was on previously") self.check_expected_network(node, True) - self.log.info(f"Test that dumptxoutset failure leaves the network activity suspended when it was off") + self.log.info("Test that dumptxoutset failure leaves the network activity suspended when it was off") node.setnetworkactive(False) self.check_expected_network(node, False) node.setnetworkactive(True) diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index 539ff155cf..f5d42d0c34 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -377,8 +377,8 @@ def test_submitpackage(self): assert txid_list[0] not in node.getrawmempool() assert txid_list[1] not in node.getrawmempool() - self.log.info("Submitpackage valid packages with 1 child and some number of parents") - for num_parents in [1, 2, 24]: + self.log.info("Submitpackage valid packages with 1 child and some number of parents (or none)") + for num_parents in [0, 1, 2, 24]: self.test_submit_child_with_parents(num_parents, False) self.test_submit_child_with_parents(num_parents, True) @@ -389,10 +389,9 @@ def test_submitpackage(self): assert_raises_rpc_error(-25, "package topology disallowed", node.submitpackage, chain_hex) assert_equal(legacy_pool, node.getrawmempool()) - assert_raises_rpc_error(-8, f"Array must contain between 2 and {MAX_PACKAGE_COUNT} transactions.", node.submitpackage, []) - assert_raises_rpc_error(-8, f"Array must contain between 2 and {MAX_PACKAGE_COUNT} transactions.", node.submitpackage, [chain_hex[0]] * 1) + assert_raises_rpc_error(-8, f"Array must contain between 1 and {MAX_PACKAGE_COUNT} transactions.", node.submitpackage, []) assert_raises_rpc_error( - -8, f"Array must contain between 2 and {MAX_PACKAGE_COUNT} transactions.", + -8, f"Array must contain between 1 and {MAX_PACKAGE_COUNT} transactions.", node.submitpackage, [chain_hex[0]] * (MAX_PACKAGE_COUNT + 1) ) diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py index 49eb64abad..75507877d5 100755 --- a/test/functional/rpc_users.py +++ b/test/functional/rpc_users.py @@ -22,13 +22,13 @@ from typing import Optional -def call_with_auth(node, user, password): +def call_with_auth(node, user, password, method="getbestblockhash"): url = urllib.parse.urlparse(node.url) headers = {"Authorization": "Basic " + str_to_b64str('{}:{}'.format(user, password))} conn = http.client.HTTPConnection(url.hostname, url.port) conn.connect() - conn.request('POST', '/', '{"method": "getbestblockhash"}', headers) + conn.request('POST', '/', f'{{"method": "{method}"}}', headers) resp = conn.getresponse() conn.close() return resp @@ -121,6 +121,25 @@ def test_perm(perm: Optional[str]): for perm in ["owner", "group", "all"]: test_perm(perm) + def test_norpccookiefile(self, node0_cookie_path): + assert self.nodes[0].is_node_stopped(), "We expect previous test to stopped the node" + assert not node0_cookie_path.exists() + + self.log.info('Starting with -norpccookiefile') + # Start, but don't wait for RPC connection as TestNode.wait_for_rpc_connection() requires the cookie. + with self.nodes[0].busy_wait_for_debug_log([b'init message: Done loading']): + self.nodes[0].start(extra_args=["-norpccookiefile"]) + assert not node0_cookie_path.exists() + + self.log.info('Testing user/password authentication still works without cookie file') + assert_equal(200, call_with_auth(self.nodes[0], "rt", self.rtpassword).status) + # After confirming that we could log in, check that cookie file does not exist. + assert not node0_cookie_path.exists() + + # Need to shut down in slightly unorthodox way since cookie auth can't be used + assert_equal(200, call_with_auth(self.nodes[0], "rt", self.rtpassword, method="stop").status) + self.nodes[0].wait_until_stopped() + def run_test(self): self.conf_setup() self.log.info('Check correctness of the rpcauth config option') @@ -166,11 +185,19 @@ def run_test(self): self.stop_node(0) self.log.info('Check that failure to write cookie file will abort the node gracefully') - (self.nodes[0].chain_path / ".cookie.tmp").mkdir() + cookie_path = self.nodes[0].chain_path / ".cookie" + cookie_path_tmp = self.nodes[0].chain_path / ".cookie.tmp" + cookie_path_tmp.mkdir() self.nodes[0].assert_start_raises_init_error(expected_msg=init_error) + cookie_path_tmp.rmdir() + assert not cookie_path.exists() + self.restart_node(0) + assert cookie_path.exists() + self.stop_node(0) self.test_rpccookieperms() + self.test_norpccookiefile(cookie_path) if __name__ == '__main__': HTTPBasicsTest(__file__).main() diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index 00ef1289b8..eb39c45438 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -332,7 +332,7 @@ def deserialize_v2(self, f): elif self.net == self.NET_CJDNS: self.ip = socket.inet_ntop(socket.AF_INET6, addr_bytes) else: - raise Exception(f"Address type not supported") + raise Exception("Address type not supported") self.port = int.from_bytes(f.read(2), "big") @@ -359,7 +359,7 @@ def serialize_v2(self): elif self.net == self.NET_CJDNS: r += socket.inet_pton(socket.AF_INET6, self.ip) else: - raise Exception(f"Address type not supported") + raise Exception("Address type not supported") r += self.port.to_bytes(2, "big") return r diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index bfa982df40..0b54fbf276 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 # Copyright (c) 2010 ArtForz -- public domain half-a-node # Copyright (c) 2012 Jeff Garzik -# Copyright (c) 2010-2022 The Bitcoin Core developers +# Copyright (c) 2010-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. """Test objects for interacting with a bitcoind node over the p2p protocol. @@ -369,7 +369,7 @@ def _on_data(self): self.on_message(t) except Exception as e: if not self.reconnect: - logger.exception('Error reading message:', repr(e)) + logger.exception(f"Error reading message: {repr(e)}") raise def on_message(self, message): @@ -659,7 +659,7 @@ def wait_for_getheaders(self, block_hash=None, *, timeout=60): def test_function(): last_getheaders = self.last_message.pop("getheaders", None) if block_hash is None: - return last_getheaders + return last_getheaders if last_getheaders is None: return False return block_hash == last_getheaders.locator.vHave[0] diff --git a/test/functional/test_framework/socks5.py b/test/functional/test_framework/socks5.py index 33726eaaae..0cd16a3ff5 100644 --- a/test/functional/test_framework/socks5.py +++ b/test/functional/test_framework/socks5.py @@ -195,7 +195,7 @@ def handle(self): if not self.serv.keep_alive: self.conn.close() else: - logger.debug(f"Keeping client connection alive") + logger.debug("Keeping client connection alive") class Socks5Server(): def __init__(self, conf): diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index bc8cd0e82c..7e8c40cf16 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -129,7 +129,11 @@ def main(self): try: self.setup() - self.run_test() + if self.options.test_methods: + self.run_test_methods() + else: + self.run_test() + except JSONRPCException: self.log.exception("JSONRPC error") self.success = TestStatus.FAILED @@ -155,6 +159,13 @@ def main(self): exit_code = self.shutdown() sys.exit(exit_code) + def run_test_methods(self): + for method_name in self.options.test_methods: + self.log.info(f"Attempting to execute method: {method_name}") + method = getattr(self, method_name) + method() + self.log.info(f"Method '{method_name}' executed successfully.") + def parse_args(self, test_file): previous_releases_path = os.getenv("PREVIOUS_RELEASES_DIR") or os.getcwd() + "/releases" parser = argparse.ArgumentParser(usage="%(prog)s [options]") @@ -194,6 +205,8 @@ def parse_args(self, test_file): help="use BIP324 v2 connections between all nodes by default") parser.add_argument("--v1transport", dest="v1transport", default=False, action="store_true", help="Explicitly use v1 transport (can be used to overwrite global --v2transport option)") + parser.add_argument("--test_methods", dest="test_methods", nargs='*', + help="Run specified test methods sequentially instead of the full test. Use only for methods that do not depend on any context set up in run_test or other methods.") self.add_options(parser) # Running TestShell in a Jupyter notebook causes an additional -f argument diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 988c4316a4..c63def6491 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -96,6 +96,10 @@ 'feature_fee_estimation.py', 'feature_taproot.py', 'feature_block.py', + 'mempool_ephemeral_dust.py', + 'wallet_conflicts.py --legacy-wallet', + 'wallet_conflicts.py --descriptors', + 'p2p_opportunistic_1p1c.py', 'p2p_node_network_limited.py --v1transport', 'p2p_node_network_limited.py --v2transport', # vv Tests less than 2m vv @@ -146,6 +150,7 @@ 'p2p_feefilter.py', 'feature_csv_activation.py', 'p2p_sendheaders.py', + 'feature_config_args.py', 'wallet_listtransactions.py --legacy-wallet', 'wallet_listtransactions.py --descriptors', 'wallet_miniscript.py --descriptors', @@ -199,7 +204,6 @@ 'rpc_getchaintips.py', 'rpc_misc.py', 'p2p_1p1c_network.py', - 'p2p_opportunistic_1p1c.py', 'interface_rest.py', 'mempool_spend_coinbase.py', 'wallet_avoid_mixing_output_types.py --descriptors', @@ -214,8 +218,6 @@ 'wallet_reindex.py --legacy-wallet', 'wallet_reindex.py --descriptors', 'wallet_reorgsrestore.py', - 'wallet_conflicts.py --legacy-wallet', - 'wallet_conflicts.py --descriptors', 'interface_http.py', 'interface_rpc.py', 'interface_usdt_coinselection.py', @@ -395,13 +397,11 @@ 'feature_remove_pruned_files_on_startup.py', 'p2p_i2p_ports.py', 'p2p_i2p_sessions.py', - 'feature_config_args.py', 'feature_presegwit_node_upgrade.py', 'feature_settings.py', 'rpc_getdescriptorinfo.py', 'rpc_mempool_info.py', 'rpc_help.py', - 'mempool_ephemeral_dust.py', 'p2p_handshake.py', 'p2p_handshake.py --v2transport', 'feature_dirsymlinks.py', diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index 83267f77e1..7c88f64dcf 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -157,7 +157,7 @@ def test_pruned_wallet_backup(self): node.pruneblockchain(250) # The backup should be updated with the latest height (locator) for # the backup to load successfully this close to the prune height - node.restorewallet(f'pruned', node.datadir_path / 'wallet_pruned.bak') + node.restorewallet('pruned', node.datadir_path / 'wallet_pruned.bak') def run_test(self): self.log.info("Generating initial blockchain") diff --git a/test/functional/wallet_fast_rescan.py b/test/functional/wallet_fast_rescan.py index 4ac441516e..6cee0d3660 100755 --- a/test/functional/wallet_fast_rescan.py +++ b/test/functional/wallet_fast_rescan.py @@ -49,7 +49,7 @@ def run_test(self): assert_equal(len(descriptors), NUM_DESCRIPTORS) w.backupwallet(WALLET_BACKUP_FILENAME) - self.log.info(f"Create txs sending to end range address of each descriptor, triggering top-ups") + self.log.info("Create txs sending to end range address of each descriptor, triggering top-ups") for i in range(NUM_BLOCKS): self.log.info(f"Block {i+1}/{NUM_BLOCKS}") for desc_info in w.listdescriptors()['descriptors']: diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 48ad8e4f2a..5abb43e3b9 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -36,42 +36,37 @@ def add_options(self, parser): def set_test_params(self): self.setup_clean_chain = True - self.num_nodes = 1 - self.extra_args = [[]] + self.num_nodes = 2 self.supports_cli = False + self.extra_args = [[], ["-deprecatedrpc=create_bdb"]] def skip_test_if_missing_module(self): self.skip_if_no_wallet() - self.skip_if_no_sqlite() - self.skip_if_no_bdb() + self.skip_if_no_previous_releases() + + def setup_nodes(self): + self.add_nodes(self.num_nodes, versions=[ + None, + 280000, + ]) + self.start_nodes() + self.init_wallet(node=0) def assert_is_sqlite(self, wallet_name): - wallet_file_path = self.nodes[0].wallets_path / wallet_name / self.wallet_data_filename + wallet_file_path = self.master_node.wallets_path / wallet_name / self.wallet_data_filename with open(wallet_file_path, 'rb') as f: file_magic = f.read(16) assert_equal(file_magic, b'SQLite format 3\x00') - assert_equal(self.nodes[0].get_wallet_rpc(wallet_name).getwalletinfo()["format"], "sqlite") + assert_equal(self.master_node.get_wallet_rpc(wallet_name).getwalletinfo()["format"], "sqlite") def create_legacy_wallet(self, wallet_name, **kwargs): - self.nodes[0].createwallet(wallet_name=wallet_name, descriptors=False, **kwargs) - wallet = self.nodes[0].get_wallet_rpc(wallet_name) + self.old_node.createwallet(wallet_name=wallet_name, descriptors=False, **kwargs) + wallet = self.old_node.get_wallet_rpc(wallet_name) info = wallet.getwalletinfo() assert_equal(info["descriptors"], False) assert_equal(info["format"], "bdb") return wallet - def migrate_wallet(self, wallet_rpc, *args, **kwargs): - # Helper to ensure that only migration happens - # Since we may rescan on loading of a wallet, make sure that the best block - # is written before beginning migration - # Reload to force write that record - wallet_name = wallet_rpc.getwalletinfo()["walletname"] - wallet_rpc.unloadwallet() - self.nodes[0].loadwallet(wallet_name) - # Migrate, checking that rescan does not occur - with self.nodes[0].assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]): - return wallet_rpc.migratewallet(*args, **kwargs) - def assert_addr_info_equal(self, addr_info, addr_info_old): assert_equal(addr_info["address"], addr_info_old["address"]) assert_equal(addr_info["scriptPubKey"], addr_info_old["scriptPubKey"]) @@ -99,8 +94,25 @@ def check_address(self, wallet, addr, is_mine, is_change, label): else: assert_equal(addr_info['labels'], []), + def migrate_and_get_rpc(self, wallet_name, **kwargs): + # Since we may rescan on loading of a wallet, make sure that the best block + # is written before beginning migration + # Reload to force write that record + self.old_node.unloadwallet(wallet_name) + self.old_node.loadwallet(wallet_name) + # Now unload so we can copy it to the master node for the migration test + self.old_node.unloadwallet(wallet_name) + if wallet_name == "": + shutil.copyfile(self.old_node.wallets_path / "wallet.dat", self.master_node.wallets_path / "wallet.dat") + else: + shutil.copytree(self.old_node.wallets_path / wallet_name, self.master_node.wallets_path / wallet_name) + # Migrate, checking that rescan does not occur + with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]): + migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs) + return migrate_info, self.master_node.get_wallet_rpc(wallet_name) + def test_basic(self): - default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + default = self.master_node.get_wallet_rpc(self.default_wallet_name) self.log.info("Test migration of a basic keys only wallet without balance") basic0 = self.create_legacy_wallet("basic0") @@ -116,7 +128,7 @@ def test_basic(self): assert_equal(old_change_addr_info["hdkeypath"], "m/0'/1'/0'") # Note: migration could take a while. - self.migrate_wallet(basic0) + _, basic0 = self.migrate_and_get_rpc("basic0") # Verify created descriptors assert_equal(basic0.getwalletinfo()["descriptors"], True) @@ -147,35 +159,36 @@ def test_basic(self): for _ in range(0, 10): default.sendtoaddress(basic1.getnewaddress(), 1) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) for _ in range(0, 5): basic1.sendtoaddress(default.getnewaddress(), 0.5) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) bal = basic1.getbalance() txs = basic1.listtransactions() addr_gps = basic1.listaddressgroupings() - basic1_migrate = self.migrate_wallet(basic1) + basic1_migrate, basic1 = self.migrate_and_get_rpc("basic1") assert_equal(basic1.getwalletinfo()["descriptors"], True) self.assert_is_sqlite("basic1") assert_equal(basic1.getbalance(), bal) self.assert_list_txs_equal(basic1.listtransactions(), txs) self.log.info("Test backup file can be successfully restored") - self.nodes[0].restorewallet("basic1_restored", basic1_migrate['backup_path']) - basic1_restored = self.nodes[0].get_wallet_rpc("basic1_restored") + self.old_node.restorewallet("basic1_restored", basic1_migrate['backup_path']) + basic1_restored = self.old_node.get_wallet_rpc("basic1_restored") basic1_restored_wi = basic1_restored.getwalletinfo() assert_equal(basic1_restored_wi['balance'], bal) assert_equal(basic1_restored.listaddressgroupings(), addr_gps) self.assert_list_txs_equal(basic1_restored.listtransactions(), txs) - # restart node and verify that everything is still there + # restart master node and verify that everything is still there self.restart_node(0) - default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) - self.nodes[0].loadwallet("basic1") - basic1 = self.nodes[0].get_wallet_rpc("basic1") + self.connect_nodes(0, 1) + default = self.master_node.get_wallet_rpc(self.default_wallet_name) + self.master_node.loadwallet("basic1") + basic1 = self.master_node.get_wallet_rpc("basic1") assert_equal(basic1.getwalletinfo()["descriptors"], True) self.assert_is_sqlite("basic1") assert_equal(basic1.getbalance(), bal) @@ -193,12 +206,12 @@ def test_basic(self): send_value = random.randint(1, 4) default.sendtoaddress(addr, send_value) basic2_balance += send_value - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) assert_equal(basic2.getbalance(), basic2_balance) basic2_txs = basic2.listtransactions() - # Now migrate and test that we still see have the same balance/transactions - self.migrate_wallet(basic2) + # Now migrate and test that we still have the same balance/transactions + _, basic2 = self.migrate_and_get_rpc("basic2") assert_equal(basic2.getwalletinfo()["descriptors"], True) self.assert_is_sqlite("basic2") assert_equal(basic2.getbalance(), basic2_balance) @@ -210,10 +223,10 @@ def test_basic(self): self.log.info("Test \"nothing to migrate\" when the user tries to migrate an unloaded wallet with no legacy data") basic2.unloadwallet() - assert_raises_rpc_error(-4, "Error: This wallet is already a descriptor wallet", self.nodes[0].migratewallet, "basic2") + assert_raises_rpc_error(-4, "Error: This wallet is already a descriptor wallet", self.master_node.migratewallet, "basic2") def test_multisig(self): - default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + default = self.master_node.get_wallet_rpc(self.default_wallet_name) # Contrived case where all the multisig keys are in a single wallet self.log.info("Test migration of a wallet with all keys for a multisig") @@ -224,14 +237,14 @@ def test_multisig(self): ms_info = multisig0.addmultisigaddress(2, [addr1, addr2, addr3]) - self.migrate_wallet(multisig0) + _, multisig0 = self.migrate_and_get_rpc("multisig0") assert_equal(multisig0.getwalletinfo()["descriptors"], True) self.assert_is_sqlite("multisig0") ms_addr_info = multisig0.getaddressinfo(ms_info["address"]) assert_equal(ms_addr_info["ismine"], True) assert_equal(ms_addr_info["desc"], ms_info["descriptor"]) - assert_equal("multisig0_watchonly" in self.nodes[0].listwallets(), False) - assert_equal("multisig0_solvables" in self.nodes[0].listwallets(), False) + assert_equal("multisig0_watchonly" in self.master_node.listwallets(), False) + assert_equal("multisig0_solvables" in self.master_node.listwallets(), False) pub1 = multisig0.getaddressinfo(addr1)["pubkey"] pub2 = multisig0.getaddressinfo(addr2)["pubkey"] @@ -249,7 +262,7 @@ def test_multisig(self): assert_equal(multisig1.getaddressinfo(addr1)["ismine"], False) assert_equal(multisig1.getaddressinfo(addr1)["iswatchonly"], True) assert_equal(multisig1.getaddressinfo(addr1)["solvable"], True) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) multisig1.gettransaction(txid) assert_equal(multisig1.getbalances()["watchonly"]["trusted"], 10) assert_equal(multisig1.getaddressinfo(addr2)["ismine"], False) @@ -259,7 +272,7 @@ def test_multisig(self): # Migrating multisig1 should see the multisig is no longer part of multisig1 # A new wallet multisig1_watchonly is created which has the multisig address # Transaction to multisig is in multisig1_watchonly and not multisig1 - self.migrate_wallet(multisig1) + _, multisig1 = self.migrate_and_get_rpc("multisig1") assert_equal(multisig1.getwalletinfo()["descriptors"], True) self.assert_is_sqlite("multisig1") assert_equal(multisig1.getaddressinfo(addr1)["ismine"], False) @@ -269,8 +282,8 @@ def test_multisig(self): assert_equal(multisig1.getbalance(), 0) assert_equal(multisig1.listtransactions(), []) - assert_equal("multisig1_watchonly" in self.nodes[0].listwallets(), True) - ms1_watchonly = self.nodes[0].get_wallet_rpc("multisig1_watchonly") + assert_equal("multisig1_watchonly" in self.master_node.listwallets(), True) + ms1_watchonly = self.master_node.get_wallet_rpc("multisig1_watchonly") ms1_wallet_info = ms1_watchonly.getwalletinfo() assert_equal(ms1_wallet_info['descriptors'], True) assert_equal(ms1_wallet_info['private_keys_enabled'], False) @@ -286,8 +299,8 @@ def test_multisig(self): # Migrating multisig1 should see the second multisig is no longer part of multisig1 # A new wallet multisig1_solvables is created which has the second address # This should have no transactions - assert_equal("multisig1_solvables" in self.nodes[0].listwallets(), True) - ms1_solvable = self.nodes[0].get_wallet_rpc("multisig1_solvables") + assert_equal("multisig1_solvables" in self.master_node.listwallets(), True) + ms1_solvable = self.master_node.get_wallet_rpc("multisig1_solvables") ms1_wallet_info = ms1_solvable.getwalletinfo() assert_equal(ms1_wallet_info['descriptors'], True) assert_equal(ms1_wallet_info['private_keys_enabled'], False) @@ -301,7 +314,7 @@ def test_multisig(self): def test_other_watchonly(self): - default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + default = self.master_node.get_wallet_rpc(self.default_wallet_name) # Wallet with an imported address. Should be the same thing as the multisig test self.log.info("Test migration of a wallet with watchonly imports") @@ -332,7 +345,7 @@ def test_other_watchonly(self): # Tx that has both a watchonly and spendable output watchonly_spendable_txid = default.send(outputs=[{received_addr: 1}, {import_addr:1}])["txid"] - self.generate(self.nodes[0], 2) + self.generate(self.master_node, 2) received_watchonly_tx_info = imports0.gettransaction(received_watchonly_txid, True) received_sent_watchonly_tx_info = imports0.gettransaction(received_sent_watchonly_utxo["txid"], True) @@ -342,10 +355,10 @@ def test_other_watchonly(self): assert_equal(len(imports0.listtransactions(include_watchonly=True)), 6) # Mock time forward a bit so we can check that tx metadata is preserved - self.nodes[0].setmocktime(int(time.time()) + 100) + self.master_node.setmocktime(int(time.time()) + 100) # Migrate - self.migrate_wallet(imports0) + _, imports0 = self.migrate_and_get_rpc("imports0") assert_equal(imports0.getwalletinfo()["descriptors"], True) self.assert_is_sqlite("imports0") assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", imports0.gettransaction, received_watchonly_txid) @@ -356,8 +369,8 @@ def test_other_watchonly(self): imports0.gettransaction(watchonly_spendable_txid) assert_equal(imports0.getbalance(), spendable_bal) - assert_equal("imports0_watchonly" in self.nodes[0].listwallets(), True) - watchonly = self.nodes[0].get_wallet_rpc("imports0_watchonly") + assert_equal("imports0_watchonly" in self.master_node.listwallets(), True) + watchonly = self.master_node.get_wallet_rpc("imports0_watchonly") watchonly_info = watchonly.getwalletinfo() assert_equal(watchonly_info["descriptors"], True) self.assert_is_sqlite("imports0_watchonly") @@ -375,14 +388,14 @@ def test_other_watchonly(self): assert_equal(len(watchonly.listtransactions(include_watchonly=True)), 4) # Check that labels were migrated and persisted to watchonly wallet - self.nodes[0].unloadwallet("imports0_watchonly") - self.nodes[0].loadwallet("imports0_watchonly") + self.master_node.unloadwallet("imports0_watchonly") + self.master_node.loadwallet("imports0_watchonly") labels = watchonly.listlabels() assert "external" in labels assert "imported" in labels def test_no_privkeys(self): - default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + default = self.master_node.get_wallet_rpc(self.default_wallet_name) # Migrating an actual watchonly wallet should not create a new watchonly wallet self.log.info("Test migration of a pure watchonly wallet") @@ -398,10 +411,10 @@ def test_no_privkeys(self): }]) assert_equal(res[0]['success'], True) default.sendtoaddress(addr, 10) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) - self.migrate_wallet(watchonly0) - assert_equal("watchonly0_watchonly" in self.nodes[0].listwallets(), False) + _, watchonly0 = self.migrate_and_get_rpc("watchonly0") + assert_equal("watchonly0_watchonly" in self.master_node.listwallets(), False) info = watchonly0.getwalletinfo() assert_equal(info["descriptors"], True) assert_equal(info["private_keys_enabled"], False) @@ -432,7 +445,7 @@ def test_no_privkeys(self): # Before migrating, we can fetch addr1 from the keypool assert_equal(watchonly1.getnewaddress(address_type="bech32"), addr1) - self.migrate_wallet(watchonly1) + _, watchonly1 = self.migrate_and_get_rpc("watchonly1") info = watchonly1.getwalletinfo() assert_equal(info["descriptors"], True) assert_equal(info["private_keys_enabled"], False) @@ -448,36 +461,34 @@ def test_pk_coinbases(self): addr_info = wallet.getaddressinfo(addr) desc = descsum_create("pk(" + addr_info["pubkey"] + ")") - self.nodes[0].generatetodescriptor(1, desc, invalid_call=False) + self.master_node.generatetodescriptor(1, desc, invalid_call=False) bals = wallet.getbalances() - self.migrate_wallet(wallet) + _, wallet = self.migrate_and_get_rpc("pkcb") assert_equal(bals, wallet.getbalances()) def test_encrypted(self): self.log.info("Test migration of an encrypted wallet") wallet = self.create_legacy_wallet("encrypted") - default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + default = self.master_node.get_wallet_rpc(self.default_wallet_name) wallet.encryptwallet("pass") addr = wallet.getnewaddress() txid = default.sendtoaddress(addr, 1) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) bals = wallet.getbalances() - assert_raises_rpc_error(-4, "Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect", wallet.migratewallet) - assert_raises_rpc_error(-4, "Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect", wallet.migratewallet, None, "badpass") - assert_raises_rpc_error(-4, "The passphrase contains a null character", wallet.migratewallet, None, "pass\0with\0null") - - # Check the wallet is still active post-migration failure. - # If not, it will throw an exception and abort the test. - wallet.walletpassphrase("pass", 99999) - wallet.getnewaddress() + # Use self.migrate_and_get_rpc to test this error to get everything copied over to the master node + assert_raises_rpc_error(-4, "Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect", self.migrate_and_get_rpc, "encrypted") + # Use the RPC directly on the master node for the rest of these checks + assert_raises_rpc_error(-4, "Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect", self.master_node.migratewallet, "encrypted", "badpass") + assert_raises_rpc_error(-4, "The passphrase contains a null character", self.master_node.migratewallet, "encrypted", "pass\0with\0null") # Verify we can properly migrate the encrypted wallet - self.migrate_wallet(wallet, passphrase="pass") + self.master_node.migratewallet("encrypted", passphrase="pass") + wallet = self.master_node.get_wallet_rpc("encrypted") info = wallet.getwalletinfo() assert_equal(info["descriptors"], True) @@ -487,46 +498,30 @@ def test_encrypted(self): assert_equal(bals, wallet.getbalances()) - def test_unloaded(self): - self.log.info("Test migration of a wallet that isn't loaded") - wallet = self.create_legacy_wallet("notloaded") - default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) - - addr = wallet.getnewaddress() - txid = default.sendtoaddress(addr, 1) - self.generate(self.nodes[0], 1) - bals = wallet.getbalances() - - wallet.unloadwallet() - - assert_raises_rpc_error(-8, "RPC endpoint wallet and wallet_name parameter specify different wallets", wallet.migratewallet, "someotherwallet") - assert_raises_rpc_error(-8, "Either RPC endpoint wallet or wallet_name parameter must be provided", self.nodes[0].migratewallet) - self.nodes[0].migratewallet("notloaded") - - info = wallet.getwalletinfo() - assert_equal(info["descriptors"], True) - assert_equal(info["format"], "sqlite") - wallet.gettransaction(txid) - - assert_equal(bals, wallet.getbalances()) + def test_nonexistent(self): + self.log.info("Check migratewallet errors for nonexistent wallets") + default = self.master_node.get_wallet_rpc(self.default_wallet_name) + assert_raises_rpc_error(-8, "RPC endpoint wallet and wallet_name parameter specify different wallets", default.migratewallet, "someotherwallet") + assert_raises_rpc_error(-8, "Either RPC endpoint wallet or wallet_name parameter must be provided", self.master_node.migratewallet) + assert_raises_rpc_error(-4, "Error: Wallet does not exist", self.master_node.migratewallet, "notawallet") def test_unloaded_by_path(self): self.log.info("Test migration of a wallet that isn't loaded, specified by path") wallet = self.create_legacy_wallet("notloaded2") - default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + default = self.master_node.get_wallet_rpc(self.default_wallet_name) addr = wallet.getnewaddress() txid = default.sendtoaddress(addr, 1) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) bals = wallet.getbalances() wallet.unloadwallet() - wallet_file_path = self.nodes[0].wallets_path / "notloaded2" - self.nodes[0].migratewallet(wallet_file_path) + wallet_file_path = self.old_node.wallets_path / "notloaded2" + self.master_node.migratewallet(wallet_file_path) # Because we gave the name by full path, the loaded wallet's name is that path too. - wallet = self.nodes[0].get_wallet_rpc(str(wallet_file_path)) + wallet = self.master_node.get_wallet_rpc(str(wallet_file_path)) info = wallet.getwalletinfo() assert_equal(info["descriptors"], True) @@ -541,9 +536,10 @@ def test_default_wallet(self): # Set time to verify backup existence later curr_time = int(time.time()) - wallet.setmocktime(curr_time) + self.master_node.setmocktime(curr_time) - res = self.migrate_wallet(wallet) + res, wallet = self.migrate_and_get_rpc("") + self.master_node.setmocktime(0) info = wallet.getwalletinfo() assert_equal(info["descriptors"], True) assert_equal(info["format"], "sqlite") @@ -553,38 +549,35 @@ def test_default_wallet(self): # Check backup existence and its non-empty wallet filename backup_filename = f"default_wallet_{curr_time}.legacy.bak" - backup_path = self.nodes[0].wallets_path / backup_filename + backup_path = self.master_node.wallets_path / backup_filename assert backup_path.exists() assert_equal(str(backup_path), res['backup_path']) assert {"name": backup_filename} not in walletdir_list["wallets"] + self.master_node.setmocktime(0) + def test_direct_file(self): self.log.info("Test migration of a wallet that is not in a wallet directory") wallet = self.create_legacy_wallet("plainfile") wallet.unloadwallet() - wallets_dir = self.nodes[0].wallets_path - wallet_path = wallets_dir / "plainfile" - wallet_dat_path = wallet_path / "wallet.dat" - shutil.copyfile(wallet_dat_path, wallets_dir / "plainfile.bak") - shutil.rmtree(wallet_path) - shutil.move(wallets_dir / "plainfile.bak", wallet_path) - - self.nodes[0].loadwallet("plainfile") - info = wallet.getwalletinfo() - assert_equal(info["descriptors"], False) - assert_equal(info["format"], "bdb") + shutil.copyfile( + self.old_node.wallets_path / "plainfile" / "wallet.dat" , + self.master_node.wallets_path / "plainfile" + ) + assert (self.master_node.wallets_path / "plainfile").is_file() - self.migrate_wallet(wallet) + self.master_node.migratewallet("plainfile") + wallet = self.master_node.get_wallet_rpc("plainfile") info = wallet.getwalletinfo() assert_equal(info["descriptors"], True) assert_equal(info["format"], "sqlite") - assert wallet_path.is_dir() - assert wallet_dat_path.is_file() + assert (self.master_node.wallets_path / "plainfile").is_dir() + assert (self.master_node.wallets_path / "plainfile" / "wallet.dat").is_file() def test_addressbook(self): - df_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + df_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) self.log.info("Test migration of address book data") wallet = self.create_legacy_wallet("legacy_addrbook") @@ -602,7 +595,7 @@ def test_addressbook(self): wallet.importpubkey(df_wallet.getaddressinfo(multi_addr3)["pubkey"]) ms_addr_info = wallet.addmultisigaddress(2, [multi_addr1, multi_addr2, multi_addr3]) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) # Test vectors addr_external = { @@ -650,7 +643,7 @@ def test_addressbook(self): # To store the change address in the addressbook need to send coins to it wallet.send(outputs=[{wallet.getnewaddress(): 2}], options={"change_address": change_address['addr']}) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) # Util wrapper func for 'addr_info' def check(info, node): @@ -663,9 +656,9 @@ def check(info, node): check(addr_info, wallet) # Migrate wallet - info_migration = self.migrate_wallet(wallet) - wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"]) - wallet_solvables = self.nodes[0].get_wallet_rpc(info_migration["solvables_name"]) + info_migration, wallet = self.migrate_and_get_rpc("legacy_addrbook") + wallet_wo = self.master_node.get_wallet_rpc(info_migration["watchonly_name"]) + wallet_solvables = self.master_node.get_wallet_rpc(info_migration["solvables_name"]) ######################### # Post migration checks # @@ -690,28 +683,28 @@ def check(info, node): ######################################################################################## # First the main wallet - self.nodes[0].unloadwallet("legacy_addrbook") - self.nodes[0].loadwallet("legacy_addrbook") + self.master_node.unloadwallet("legacy_addrbook") + self.master_node.loadwallet("legacy_addrbook") for addr_info in [addr_external, addr_external_with_label, addr_internal, addr_internal_with_label, change_address, ms_addr]: check(addr_info, wallet) # Watch-only wallet - self.nodes[0].unloadwallet(info_migration["watchonly_name"]) - self.nodes[0].loadwallet(info_migration["watchonly_name"]) + self.master_node.unloadwallet(info_migration["watchonly_name"]) + self.master_node.loadwallet(info_migration["watchonly_name"]) self.check_address(wallet_wo, watch_only_addr['addr'], is_mine=True, is_change=watch_only_addr['is_change'], label=watch_only_addr["label"]) for addr_info in [addr_external, addr_external_with_label, ms_addr]: check(addr_info, wallet_wo) # Solvables wallet - self.nodes[0].unloadwallet(info_migration["solvables_name"]) - self.nodes[0].loadwallet(info_migration["solvables_name"]) + self.master_node.unloadwallet(info_migration["solvables_name"]) + self.master_node.loadwallet(info_migration["solvables_name"]) self.check_address(wallet_solvables, ms_addr['addr'], is_mine=True, is_change=ms_addr['is_change'], label=ms_addr["label"]) for addr_info in [addr_external, addr_external_with_label]: check(addr_info, wallet_solvables) def test_migrate_raw_p2sh(self): self.log.info("Test migration of watch-only raw p2sh script") - df_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + df_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) wallet = self.create_legacy_wallet("raw_p2sh") def send_to_script(script, amount): @@ -721,7 +714,7 @@ def send_to_script(script, amount): hex_tx = df_wallet.fundrawtransaction(tx.serialize().hex())['hex'] signed_tx = df_wallet.signrawtransactionwithwallet(hex_tx) df_wallet.sendrawtransaction(signed_tx['hex']) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) # Craft sh(pkh(key)) script and send coins to it pubkey = df_wallet.getaddressinfo(df_wallet.getnewaddress())["pubkey"] @@ -758,8 +751,8 @@ def send_to_script(script, amount): wallet.rpc.importaddress(address=script_sh_pkh.hex(), label=label_sh_pkh, rescan=False, p2sh=True) # Migrate wallet and re-check balance - info_migration = self.migrate_wallet(wallet) - wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"]) + info_migration, wallet = self.migrate_and_get_rpc("raw_p2sh") + wallet_wo = self.master_node.get_wallet_rpc(info_migration["watchonly_name"]) # Watch-only balance is under "mine". assert_equal(wallet_wo.getbalances()['mine']['trusted'], 5) @@ -781,17 +774,17 @@ def send_to_script(script, amount): assert_equal(next((it['desc'] for it in wallet_wo.listdescriptors()['descriptors'] if it['desc'] == desc_invalid), None), None) # Just in case, also verify wallet restart - self.nodes[0].unloadwallet(info_migration["watchonly_name"]) - self.nodes[0].loadwallet(info_migration["watchonly_name"]) + self.master_node.unloadwallet(info_migration["watchonly_name"]) + self.master_node.loadwallet(info_migration["watchonly_name"]) assert_equal(wallet_wo.getbalances()['mine']['trusted'], 5) def test_conflict_txs(self): self.log.info("Test migration when wallet contains conflicting transactions") - def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) wallet = self.create_legacy_wallet("conflicts") def_wallet.sendtoaddress(wallet.getnewaddress(), 10) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) # parent tx parent_txid = wallet.sendtoaddress(wallet.getnewaddress(), 9) @@ -799,12 +792,12 @@ def test_conflict_txs(self): conflict_utxo = wallet.gettransaction(txid=parent_txid, verbose=True)["decoded"]["vin"][0] # The specific assertion in MarkConflicted being tested requires that the parent tx is already loaded - # by the time the child tx is loaded. Since transactions end up being loaded in txid order due to how both - # and sqlite store things, we can just grind the child tx until it has a txid that is greater than the parent's. + # by the time the child tx is loaded. Since transactions end up being loaded in txid order due to how + # sqlite stores things, we can just grind the child tx until it has a txid that is greater than the parent's. locktime = 500000000 # Use locktime as nonce, starting at unix timestamp minimum addr = wallet.getnewaddress() while True: - child_send_res = wallet.send(outputs=[{addr: 8}], add_to_wallet=False, locktime=locktime) + child_send_res = wallet.send(outputs=[{addr: 8}], options={"add_to_wallet": False, "locktime": locktime}) child_txid = child_send_res["txid"] child_txid_bytes = bytes.fromhex(child_txid)[::-1] if (child_txid_bytes > parent_txid_bytes): @@ -813,15 +806,15 @@ def test_conflict_txs(self): locktime += 1 # conflict with parent - conflict_unsigned = self.nodes[0].createrawtransaction(inputs=[conflict_utxo], outputs=[{wallet.getnewaddress(): 9.9999}]) + conflict_unsigned = self.master_node.createrawtransaction(inputs=[conflict_utxo], outputs=[{wallet.getnewaddress(): 9.9999}]) conflict_signed = wallet.signrawtransactionwithwallet(conflict_unsigned)["hex"] - conflict_txid = self.nodes[0].sendrawtransaction(conflict_signed) - self.generate(self.nodes[0], 1) + conflict_txid = self.master_node.sendrawtransaction(conflict_signed) + self.generate(self.master_node, 1) assert_equal(wallet.gettransaction(txid=parent_txid)["confirmations"], -1) assert_equal(wallet.gettransaction(txid=child_txid)["confirmations"], -1) assert_equal(wallet.gettransaction(txid=conflict_txid)["confirmations"], 1) - self.migrate_wallet(wallet) + _, wallet = self.migrate_and_get_rpc("conflicts") assert_equal(wallet.gettransaction(txid=parent_txid)["confirmations"], -1) assert_equal(wallet.gettransaction(txid=child_txid)["confirmations"], -1) assert_equal(wallet.gettransaction(txid=conflict_txid)["confirmations"], 1) @@ -854,7 +847,7 @@ def test_hybrid_pubkey(self): p2wpkh_addr = key_to_p2wpkh(hybrid_pubkey) wallet.importaddress(p2wpkh_addr) - migrate_info = self.migrate_wallet(wallet) + migrate_info, wallet = self.migrate_and_get_rpc("hybrid_keys") # Both addresses should only appear in the watchonly wallet p2pkh_addr_info = wallet.getaddressinfo(p2pkh_addr) @@ -864,7 +857,7 @@ def test_hybrid_pubkey(self): assert_equal(p2wpkh_addr_info["iswatchonly"], False) assert_equal(p2wpkh_addr_info["ismine"], False) - watchonly_wallet = self.nodes[0].get_wallet_rpc(migrate_info["watchonly_name"]) + watchonly_wallet = self.master_node.get_wallet_rpc(migrate_info["watchonly_name"]) watchonly_p2pkh_addr_info = watchonly_wallet.getaddressinfo(p2pkh_addr) assert_equal(watchonly_p2pkh_addr_info["iswatchonly"], False) assert_equal(watchonly_p2pkh_addr_info["ismine"], True) @@ -887,31 +880,32 @@ def test_failed_migration_cleanup(self): # Make a copy of the wallet with the solvables wallet name so that we are unable # to create the solvables wallet when migrating, thus failing to migrate wallet.unloadwallet() - solvables_path = self.nodes[0].wallets_path / "failed_solvables" - shutil.copytree(self.nodes[0].wallets_path / "failed", solvables_path) + solvables_path = self.master_node.wallets_path / "failed_solvables" + shutil.copytree(self.old_node.wallets_path / "failed", solvables_path) original_shasum = sha256sum_file(solvables_path / "wallet.dat") - self.nodes[0].loadwallet("failed") + self.old_node.loadwallet("failed") # Add a multisig so that a solvables wallet is created wallet.addmultisigaddress(2, [wallet.getnewaddress(), get_generate_key().pubkey]) wallet.importaddress(get_generate_key().p2pkh_addr) - assert_raises_rpc_error(-4, "Failed to create database", wallet.migratewallet) + self.old_node.unloadwallet("failed") + 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.nodes[0].listwallets() - assert "failed_watchonly" not in self.nodes[0].listwallets() - assert "failed_solvables" not in self.nodes[0].listwallets() + 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 not (self.nodes[0].wallets_path / "failed_watchonly").exists() + 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 assert solvables_path.exists() new_shasum = sha256sum_file(solvables_path / "wallet.dat") assert_equal(original_shasum, new_shasum) - wallet.unloadwallet() # Check the wallet we tried to migrate is still BDB - with open(self.nodes[0].wallets_path / "failed" / "wallet.dat", "rb") as f: + with open(self.master_node.wallets_path / "failed" / "wallet.dat", "rb") as f: data = f.read(16) _, _, magic = struct.unpack("QII", data) assert_equal(magic, BTREE_MAGIC) @@ -920,13 +914,13 @@ def test_blank(self): self.log.info("Test that a blank wallet is migrated") wallet = self.create_legacy_wallet("blank", blank=True) assert_equal(wallet.getwalletinfo()["blank"], True) - wallet.migratewallet() + _, wallet = self.migrate_and_get_rpc("blank") assert_equal(wallet.getwalletinfo()["blank"], True) assert_equal(wallet.getwalletinfo()["descriptors"], True) def test_avoidreuse(self): self.log.info("Test that avoidreuse persists after migration") - def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) wallet = self.create_legacy_wallet("avoidreuse") wallet.setwalletflag("avoid_reuse", True) @@ -941,12 +935,12 @@ def test_avoidreuse(self): reused_addr = wallet.getnewaddress() def_wallet.sendtoaddress(reused_addr, 2) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) # Send funds from the test wallet with both its own and the imported wallet.sendall([def_wallet.getnewaddress()]) def_wallet.sendall(recipients=[def_wallet.getnewaddress()], inputs=imported_utxos) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) balances = wallet.getbalances() assert_equal(balances["mine"]["trusted"], 0) assert_equal(balances["watchonly"]["trusted"], 0) @@ -954,7 +948,7 @@ def test_avoidreuse(self): # Reuse the addresses def_wallet.sendtoaddress(reused_addr, 1) def_wallet.sendtoaddress(reused_imported_addr, 1) - self.generate(self.nodes[0], 1) + self.generate(self.master_node, 1) balances = wallet.getbalances() assert_equal(balances["mine"]["used"], 1) # Reused watchonly will not show up in balances @@ -968,8 +962,8 @@ def test_avoidreuse(self): assert_equal(utxo["reused"], True) # Migrate - migrate_res = wallet.migratewallet() - watchonly_wallet = self.nodes[0].get_wallet_rpc(migrate_res["watchonly_name"]) + _, wallet = self.migrate_and_get_rpc("avoidreuse") + watchonly_wallet = self.master_node.get_wallet_rpc("avoidreuse_watchonly") # One utxo in each wallet, marked used utxos = wallet.listunspent() @@ -981,13 +975,13 @@ def test_avoidreuse(self): def test_preserve_tx_extra_info(self): self.log.info("Test that tx extra data is preserved after migration") - def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) # Create and fund wallet wallet = self.create_legacy_wallet("persist_comments") def_wallet.sendtoaddress(wallet.getnewaddress(), 2) - self.generate(self.nodes[0], 6) + self.generate(self.master_node, 6) # Create tx and bump it to store 'replaced_by_txid' and 'replaces_txid' data within the transactions. # Additionally, store an extra comment within the original tx. @@ -1006,7 +1000,7 @@ def check_comments(): # Pre-migration verification check_comments() # Migrate - wallet.migratewallet() + _, wallet = self.migrate_and_get_rpc("persist_comments") # Post-migration verification check_comments() @@ -1014,7 +1008,10 @@ def check_comments(): def run_test(self): - self.generate(self.nodes[0], 101) + self.master_node = self.nodes[0] + self.old_node = self.nodes[1] + + self.generate(self.master_node, 101) # TODO: Test the actual records in the wallet for these tests too. The behavior may be correct, but the data written may not be what we actually want self.test_basic() @@ -1023,7 +1020,7 @@ def run_test(self): self.test_no_privkeys() self.test_pk_coinbases() self.test_encrypted() - self.test_unloaded() + self.test_nonexistent() self.test_unloaded_by_path() self.test_default_wallet() self.test_direct_file() diff --git a/test/functional/wallet_multisig_descriptor_psbt.py b/test/functional/wallet_multisig_descriptor_psbt.py index 9bd3e63219..6a2c5664f9 100755 --- a/test/functional/wallet_multisig_descriptor_psbt.py +++ b/test/functional/wallet_multisig_descriptor_psbt.py @@ -53,8 +53,8 @@ def participants_create_multisigs(self, external_xpubs, internal_xpubs): for i, node in enumerate(self.nodes): node.createwallet(wallet_name=f"{self.name}_{i}", blank=True, descriptors=True, disable_private_keys=True) multisig = node.get_wallet_rpc(f"{self.name}_{i}") - external = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f','.join(external_xpubs)}))") - internal = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f','.join(internal_xpubs)}))") + external = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{','.join(external_xpubs)}))") + internal = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{','.join(internal_xpubs)}))") result = multisig.importdescriptors([ { # receiving addresses (internal: False) "desc": external["descriptor"], diff --git a/test/get_previous_releases.py b/test/get_previous_releases.py index 1d397e721e..1f8d92c980 100755 --- a/test/get_previous_releases.py +++ b/test/get_previous_releases.py @@ -98,6 +98,14 @@ "fe6e347a66043946920c72c9c4afca301968101e6b82fb90a63d7885ebcceb32": {"tag": "v25.0", "tarball": "bitcoin-25.0-riscv64-linux-gnu.tar.gz"}, "5708fc639cdfc27347cccfd50db9b73b53647b36fb5f3a4a93537cbe8828c27f": {"tag": "v25.0", "tarball": "bitcoin-25.0-x86_64-apple-darwin.tar.gz"}, "33930d432593e49d58a9bff4c30078823e9af5d98594d2935862788ce8a20aec": {"tag": "v25.0", "tarball": "bitcoin-25.0-x86_64-linux-gnu.tar.gz"}, + + "7fa582d99a25c354d23e371a5848bd9e6a79702870f9cbbf1292b86e647d0f4e": {"tag": "v28.0", "tarball": "bitcoin-28.0-aarch64-linux-gnu.tar.gz"}, + "e004b7910bedd6dd18b6c52b4eef398d55971da666487a82cd48708d2879727e": {"tag": "v28.0", "tarball": "bitcoin-28.0-arm-linux-gnueabihf.tar.gz"}, + "c8108f30dfcc7ddffab33f5647d745414ef9d3298bfe67d243fe9b9cb4df4c12": {"tag": "v28.0", "tarball": "bitcoin-28.0-arm64-apple-darwin.tar.gz"}, + "756df50d8f0c2a3d4111389a7be5f4849e0f5014dd5bfcbc37a8c3aaaa54907b": {"tag": "v28.0", "tarball": "bitcoin-28.0-powerpc64-linux-gnu.tar.gz"}, + "6ee1a520b638132a16725020146abea045db418ce91c02493f02f541cd53062a": {"tag": "v28.0", "tarball": "bitcoin-28.0-riscv64-linux-gnu.tar.gz"}, + "77e931bbaaf47771a10c376230bf53223f5380864bad3568efc7f4d02e40a0f7": {"tag": "v28.0", "tarball": "bitcoin-28.0-x86_64-apple-darwin.tar.gz"}, + "7fe294b02b25b51acb8e8e0a0eb5af6bbafa7cd0c5b0e5fcbb61263104a82fbc": {"tag": "v28.0", "tarball": "bitcoin-28.0-x86_64-linux-gnu.tar.gz"}, } diff --git a/test/lint/lint-includes.py b/test/lint/lint-includes.py index 90884299d5..05780c3074 100755 --- a/test/lint/lint-includes.py +++ b/test/lint/lint-includes.py @@ -20,7 +20,7 @@ EXCLUDED_DIRS = ["contrib/devtools/bitcoin-tidy/", ] + SHARED_EXCLUDED_SUBTREES -EXPECTED_BOOST_INCLUDES = ["boost/date_time/posix_time/posix_time.hpp", +EXPECTED_BOOST_INCLUDES = [ "boost/multi_index/detail/hash_index_iterator.hpp", "boost/multi_index/hashed_index.hpp", "boost/multi_index/identity.hpp", diff --git a/test/lint/run-lint-format-strings.py b/test/lint/run-lint-format-strings.py index d3c0ac92e5..4402ec2d57 100755 --- a/test/lint/run-lint-format-strings.py +++ b/test/lint/run-lint-format-strings.py @@ -13,7 +13,7 @@ import sys FALSE_POSITIVES = [ - ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"), + ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION)"), ("src/test/translation_tests.cpp", "strprintf(format, arg)"), ] diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index 42c880052e..f4fb1c36e5 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -226,6 +226,7 @@ fn lint_py_lint() -> LintResult { "F405", // foo_function may be undefined, or defined from star imports: bar_module "F406", // "from module import *" only allowed at module level "F407", // an undefined __future__ feature name was imported + "F541", // f-string without any placeholders "F601", // dictionary key name repeated with different values "F602", // dictionary key variable name repeated with different values "F621", // too many expressions in an assignment with star-unpacking @@ -236,6 +237,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 "W191", // indentation contains tabs "W291", // trailing whitespace "W292", // no newline at end of file @@ -516,6 +518,7 @@ fn lint_markdown() -> LintResult { "--ignore-path", md_ignore_path_str.as_str(), "--gitignore", + "--gituntracked", "--root-dir", ".", ]) @@ -529,7 +532,7 @@ fn lint_markdown() -> LintResult { r#" One or more markdown links are broken. -Relative links are preferred (but not required) as jumping to file works natively within Emacs. +Note: relative links are preferred as jump-to-file works natively within Emacs, but they are not required. Markdown link errors found: {} diff --git a/vcpkg.json b/vcpkg.json index 69cfedeae5..d5dfb05ebe 100644 --- a/vcpkg.json +++ b/vcpkg.json @@ -3,7 +3,6 @@ "$schema": "https://raw.githubusercontent.com/microsoft/vcpkg-tool/main/docs/vcpkg.schema.json", "builtin-baseline": "c82f74667287d3dc386bce81e44964370c91a289", "dependencies": [ - "boost-date-time", "boost-multi-index", "boost-signals2", "libevent"