diff options
48 files changed, 1590 insertions, 486 deletions
diff --git a/ci/test/00_setup_env.sh b/ci/test/00_setup_env.sh index ab830b8ec0..e6905b6dbe 100755 --- a/ci/test/00_setup_env.sh +++ b/ci/test/00_setup_env.sh @@ -44,7 +44,6 @@ export RUN_SECURITY_TESTS=${RUN_SECURITY_TESTS:-false} export TEST_RUNNER_TIMEOUT_FACTOR=${TEST_RUNNER_TIMEOUT_FACTOR:-40} export TEST_RUNNER_ENV=${TEST_RUNNER_ENV:-} export RUN_FUZZ_TESTS=${RUN_FUZZ_TESTS:-false} -export EXPECTED_TESTS_DURATION_IN_SECONDS=${EXPECTED_TESTS_DURATION_IN_SECONDS:-1000} export CONTAINER_NAME=${CONTAINER_NAME:-ci_unnamed} export CI_IMAGE_NAME_TAG=${CI_IMAGE_NAME_TAG:-ubuntu:20.04} diff --git a/ci/test/00_setup_env_win64.sh b/ci/test/00_setup_env_win64.sh index 817ee724c2..3adfbf6e47 100755 --- a/ci/test/00_setup_env_win64.sh +++ b/ci/test/00_setup_env_win64.sh @@ -13,4 +13,4 @@ export DPKG_ADD_ARCH="i386" export PACKAGES="python3 nsis g++-mingw-w64-x86-64-posix wine-binfmt wine64 wine32 file" export RUN_FUNCTIONAL_TESTS=false export GOAL="deploy" -export BITCOIN_CONFIG="--enable-reduce-exports --disable-external-signer --disable-gui-tests" +export BITCOIN_CONFIG="--enable-reduce-exports --enable-external-signer --disable-gui-tests" diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh index 115d727ca3..7d47f6651c 100755 --- a/ci/test/06_script_b.sh +++ b/ci/test/06_script_b.sh @@ -42,6 +42,7 @@ if [ "${RUN_TIDY}" = "true" ]; then ( CI_EXEC run-clang-tidy -quiet "${MAKEJOBS}" ) | grep -C5 "error" export P_CI_DIR="${BASE_BUILD_DIR}/bitcoin-$HOST/" CI_EXEC "python3 ${DIR_IWYU}/include-what-you-use/iwyu_tool.py"\ + " src/common/init.cpp"\ " src/common/url.cpp"\ " src/compat"\ " src/dbwrapper.cpp"\ @@ -50,12 +51,14 @@ if [ "${RUN_TIDY}" = "true" ]; then " src/node/chainstate.cpp"\ " src/node/chainstatemanager_args.cpp"\ " src/node/mempool_args.cpp"\ + " src/node/minisketchwrapper.cpp"\ " src/node/utxo_snapshot.cpp"\ " src/node/validation_cache_args.cpp"\ " src/policy/feerate.cpp"\ " src/policy/packages.cpp"\ " src/policy/settings.cpp"\ " src/primitives/transaction.cpp"\ + " src/random.cpp"\ " src/rpc/fees.cpp"\ " src/rpc/signmessage.cpp"\ " src/test/fuzz/txorphan.cpp"\ diff --git a/configure.ac b/configure.ac index 5f09c8a5b6..cbe3dbcf19 100644 --- a/configure.ac +++ b/configure.ac @@ -1492,45 +1492,45 @@ if test "$use_boost" = "yes"; then fi if test "$use_external_signer" != "no"; then - case $host in - *mingw*) - dnl Boost Process uses Boost Filesystem when targeting Windows. Also, - dnl since Boost 1.71.0, Process does not work with mingw-w64 without - dnl workarounds. See 67669ab425b52a2b6be3d2f3b3b7e3939b676a2c. - if test "$use_external_signer" = "yes"; then - AC_MSG_ERROR([External signing is not supported on Windows]) - fi - use_external_signer="no"; - ;; - *) - AC_MSG_CHECKING([whether Boost.Process can be used]) - TEMP_CXXFLAGS="$CXXFLAGS" - dnl Boost 1.78 requires the following workaround. - dnl See: https://github.com/boostorg/process/issues/235 - CXXFLAGS="$CXXFLAGS -Wno-error=narrowing" - TEMP_CPPFLAGS="$CPPFLAGS" - CPPFLAGS="$CPPFLAGS $BOOST_CPPFLAGS" - TEMP_LDFLAGS="$LDFLAGS" - dnl Boost 1.73 and older require the following workaround. - LDFLAGS="$LDFLAGS $PTHREAD_CFLAGS" - AC_LINK_IFELSE([AC_LANG_PROGRAM([[#include <boost/process.hpp>]])], - [have_boost_process="yes"], - [have_boost_process="no"]) - LDFLAGS="$TEMP_LDFLAGS" - CPPFLAGS="$TEMP_CPPFLAGS" - CXXFLAGS="$TEMP_CXXFLAGS" - AC_MSG_RESULT([$have_boost_process]) - if test "$have_boost_process" = "yes"; then - use_external_signer="yes" - AC_DEFINE([ENABLE_EXTERNAL_SIGNER], [1], [Define if external signer support is enabled]) - else - if test "$use_external_signer" = "yes"; then - AC_MSG_ERROR([External signing is not supported for this Boost version]) - fi - use_external_signer="no"; - fi - ;; - esac + AC_MSG_CHECKING([whether Boost.Process can be used]) + TEMP_CXXFLAGS="$CXXFLAGS" + dnl Boost 1.78 requires the following workaround. + dnl See: https://github.com/boostorg/process/issues/235 + CXXFLAGS="$CXXFLAGS -Wno-error=narrowing" + TEMP_CPPFLAGS="$CPPFLAGS" + CPPFLAGS="$CPPFLAGS $BOOST_CPPFLAGS" + TEMP_LDFLAGS="$LDFLAGS" + dnl Boost 1.73 and older require the following workaround. + LDFLAGS="$LDFLAGS $PTHREAD_CFLAGS" + AC_LINK_IFELSE([AC_LANG_PROGRAM([[ + #define BOOST_PROCESS_USE_STD_FS + #include <boost/process.hpp> + ]],[[ + namespace bp = boost::process; + bp::opstream stdin_stream; + bp::ipstream stdout_stream; + bp::child c("dummy", bp::std_out > stdout_stream, bp::std_err > stdout_stream, bp::std_in < stdin_stream); + stdin_stream << std::string{"test"} << std::endl; + if (c.running()) c.terminate(); + c.wait(); + c.exit_code(); + ]])], + [have_boost_process="yes"], + [have_boost_process="no"]) + LDFLAGS="$TEMP_LDFLAGS" + CPPFLAGS="$TEMP_CPPFLAGS" + CXXFLAGS="$TEMP_CXXFLAGS" + AC_MSG_RESULT([$have_boost_process]) + if test "$have_boost_process" = "yes"; then + use_external_signer="yes" + AC_DEFINE([ENABLE_EXTERNAL_SIGNER], [1], [Define if external signer support is enabled]) + AC_DEFINE([BOOST_PROCESS_USE_STD_FS], [1], [Defined to avoid Boost::Process trying to use Boost Filesystem]) + else + if test "$use_external_signer" = "yes"; then + AC_MSG_ERROR([External signing is not supported for this Boost version]) + fi + use_external_signer="no"; + fi fi AM_CONDITIONAL([ENABLE_EXTERNAL_SIGNER], [test "$use_external_signer" = "yes"]) diff --git a/doc/design/assumeutxo.md b/doc/design/assumeutxo.md index ea51b1b87f..469c551536 100644 --- a/doc/design/assumeutxo.md +++ b/doc/design/assumeutxo.md @@ -3,9 +3,9 @@ Assumeutxo is a feature that allows fast bootstrapping of a validating bitcoind instance with a very similar security model to assumevalid. -The RPC commands `dumptxoutset` and `loadtxoutset` are used to respectively generate -and load UTXO snapshots. The utility script `./contrib/devtools/utxo_snapshot.sh` may -be of use. +The RPC commands `dumptxoutset` and `loadtxoutset` (yet to be merged) are used to +respectively generate and load UTXO snapshots. The utility script +`./contrib/devtools/utxo_snapshot.sh` may be of use. ## General background @@ -22,10 +22,6 @@ be of use. chainstate running asynchronously in the background. We also use this flag to control which index entries are added to setBlockIndexCandidates during LoadBlockIndex(). -- Indexing implementations via BaseIndex can no longer assume that indexation happens - sequentially, since background validation chainstates can submit BlockConnected - events out of order with the active chain. - - The concept of UTXO snapshots is treated as an implementation detail that lives behind the ChainstateManager interface. The external presentation of the changes required to facilitate the use of UTXO snapshots is the understanding that there are @@ -76,9 +72,15 @@ original chainstate remains in use as active. Once the snapshot chainstate is loaded and validated, it is promoted to active chainstate and a sync to tip begins. A new chainstate directory is created in the -datadir for the snapshot chainstate called `chainstate_snapshot`. When this directory -is present in the datadir, the snapshot chainstate will be detected and loaded as -active on node startup (via `DetectSnapshotChainstate()`). +datadir for the snapshot chainstate called `chainstate_snapshot`. + +When this directory is present in the datadir, the snapshot chainstate will be detected +and loaded as active on node startup (via `DetectSnapshotChainstate()`). + +A special file is created within that directory, `base_blockhash`, which contains the +serialized `uint256` of the base block of the snapshot. This is used to reinitialize +the snapshot chainstate on subsequent inits. Otherwise, the directory is a normal +leveldb database. | | | | ---------- | ----------- | @@ -88,7 +90,7 @@ active on node startup (via `DetectSnapshotChainstate()`). The snapshot begins to sync to tip from its base block, technically in parallel with the original chainstate, but it is given priority during block download and is allocated most of the cache (see `MaybeRebalanceCaches()` and usages) as our chief -consideration is getting to network tip. +goal is getting to network tip. **Failure consideration:** if shutdown happens at any point during this phase, both chainstates will be detected during the next init and the process will resume. @@ -107,33 +109,32 @@ sequentially. ### Background chainstate hits snapshot base block Once the tip of the background chainstate hits the base block of the snapshot -chainstate, we stop use of the background chainstate by setting `m_stop_use` (not yet -committed - see #15606), in `CompleteSnapshotValidation()`, which is checked in -`ActivateBestChain()`). We hash the background chainstate's UTXO set contents and -ensure it matches the compiled value in `CMainParams::m_assumeutxo_data`. - -The background chainstate data lingers on disk until shutdown, when in -`ChainstateManager::Reset()`, the background chainstate is cleaned up with -`ValidatedSnapshotShutdownCleanup()`, which renames the `chainstate_[hash]` datadir as -`chainstate`. +chainstate, we stop use of the background chainstate by setting `m_disabled`, in +`CompleteSnapshotValidation()`, which is checked in `ActivateBestChain()`). We hash the +background chainstate's UTXO set contents and ensure it matches the compiled value in +`CMainParams::m_assumeutxo_data`. | | | | ---------- | ----------- | -| number of chainstates | 2 (ibd has `m_stop_use=true`) | +| number of chainstates | 2 (ibd has `m_disabled=true`) | | active chainstate | snapshot | -**Failure consideration:** if bitcoind unexpectedly halts after `m_stop_use` is set on -the background chainstate but before `CompleteSnapshotValidation()` can finish, the -need to complete snapshot validation will be detected on subsequent init by -`ChainstateManager::CheckForUncleanShutdown()`. +The background chainstate data lingers on disk until the program is restarted. ### Bitcoind restarts sometime after snapshot validation has completed -When bitcoind initializes again, what began as the snapshot chainstate is now -indistinguishable from a chainstate that has been built from the traditional IBD -process, and will be initialized as such. +After a shutdown and subsequent restart, `LoadChainstate()` cleans up the background +chainstate with `ValidatedSnapshotCleanup()`, which renames the `chainstate_snapshot` +datadir as `chainstate` and removes the now unnecessary background chainstate data. | | | | ---------- | ----------- | | number of chainstates | 1 | -| active chainstate | ibd | +| active chainstate | ibd (was snapshot, but is now fully validated) | + +What began as the snapshot chainstate is now indistinguishable from a chainstate that +has been built from the traditional IBD process, and will be initialized as such. + +A file will be left in `chainstate/base_blockhash`, which indicates that the +chainstate, even though now fully validated, was originally started from a snapshot +with the corresponding base blockhash. diff --git a/doc/release-note-26194.md b/doc/release-note-26194.md new file mode 100644 index 0000000000..b72dbf9a23 --- /dev/null +++ b/doc/release-note-26194.md @@ -0,0 +1,4 @@ +Add `next_index` in `listdescriptors` RPC +----------------- + +- Added a new `next_index` field in the response in `listdescriptors` to have the same format as `importdescriptors` diff --git a/src/Makefile.am b/src/Makefile.am index 72e7db5334..7dc5594cf2 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -134,6 +134,7 @@ BITCOIN_CORE_H = \ clientversion.h \ coins.h \ common/bloom.h \ + common/init.h \ common/run_command.h \ common/url.h \ compat/assumptions.h \ @@ -640,6 +641,7 @@ libbitcoin_common_a_SOURCES = \ chainparams.cpp \ coins.cpp \ common/bloom.cpp \ + common/init.cpp \ common/interfaces.cpp \ common/run_command.cpp \ compressor.cpp \ diff --git a/src/Makefile.test.include b/src/Makefile.test.include index fa77e28736..a39b0abd9d 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -147,6 +147,7 @@ BITCOIN_TESTS =\ test/timedata_tests.cpp \ test/torcontrol_tests.cpp \ test/transaction_tests.cpp \ + test/translation_tests.cpp \ test/txindex_tests.cpp \ test/txpackage_tests.cpp \ test/txreconciliation_tests.cpp \ @@ -179,7 +180,8 @@ BITCOIN_TESTS += \ wallet/test/ismine_tests.cpp \ wallet/test/rpc_util_tests.cpp \ wallet/test/scriptpubkeyman_tests.cpp \ - wallet/test/walletload_tests.cpp + wallet/test/walletload_tests.cpp \ + wallet/test/group_outputs_tests.cpp FUZZ_SUITE_LD_COMMON +=\ $(SQLITE_LIBS) \ diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 11b0e0dee2..265d4bf655 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -75,8 +75,9 @@ static void CoinSelection(benchmark::Bench& bench) /*tx_noinputs_size=*/ 0, /*avoid_partial=*/ false, }; + auto group = wallet::GroupOutputs(wallet, available_coins, coin_selection_params, {{filter_standard}})[filter_standard]; bench.run([&] { - auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, available_coins, coin_selection_params, /*allow_mixed_output_types=*/true); + auto result = AttemptSelection(1003 * COIN, group, coin_selection_params, /*allow_mixed_output_types=*/true); assert(result); assert(result->GetSelectedValue() == 1003 * COIN); assert(result->GetInputSet().size() == 2); @@ -91,7 +92,7 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector<OutputGroup> tx.vout[nInput].nValue = nValue; COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 0, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ true, /*fees=*/ 0); set.emplace_back(); - set.back().Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false); + set.back().Insert(std::make_shared<COutput>(output), /*ancestors=*/ 0, /*descendants=*/ 0); } // Copied from src/wallet/test/coinselector_tests.cpp static CAmount make_hard_case(int utxos, std::vector<OutputGroup>& utxo_pool) diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index 6851f86297..b69913dddb 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -9,6 +9,7 @@ #include <chainparams.h> #include <clientversion.h> +#include <common/init.h> #include <common/url.h> #include <compat/compat.h> #include <init.h> @@ -120,7 +121,7 @@ static bool AppInit(NodeContext& node, int argc, char* argv[]) SetupServerArgs(args); std::string error; if (!args.ParseParameters(argc, argv, error)) { - return InitError(Untranslated(strprintf("Error parsing command line arguments: %s\n", error))); + return InitError(Untranslated(strprintf("Error parsing command line arguments: %s", error))); } // Process help and version before taking care about datadir @@ -150,31 +151,17 @@ static bool AppInit(NodeContext& node, int argc, char* argv[]) std::any context{&node}; try { - if (!CheckDataDirOption(args)) { - return InitError(Untranslated(strprintf("Specified data directory \"%s\" does not exist.\n", args.GetArg("-datadir", "")))); - } - if (!args.ReadConfigFiles(error, true)) { - return InitError(Untranslated(strprintf("Error reading configuration file: %s\n", error))); - } - // Check for chain settings (Params() calls are only valid after this clause) - try { - SelectParams(args.GetChainName()); - } catch (const std::exception& e) { - return InitError(Untranslated(strprintf("%s\n", e.what()))); + if (auto error = common::InitConfig(args)) { + return InitError(error->message, error->details); } // Error out when loose non-argument tokens are encountered on command line for (int i = 1; i < argc; i++) { if (!IsSwitchChar(argv[i][0])) { - return InitError(Untranslated(strprintf("Command line contains unexpected token '%s', see bitcoind -h for a list of options.\n", argv[i]))); + return InitError(Untranslated(strprintf("Command line contains unexpected token '%s', see bitcoind -h for a list of options.", argv[i]))); } } - if (!args.InitSettings(error)) { - InitError(Untranslated(error)); - return false; - } - // -server defaults to true for bitcoind but not for the GUI so do this here args.SoftSetBoolArg("-server", true); // Set this early so that parameter interactions go to console @@ -210,7 +197,7 @@ static bool AppInit(NodeContext& node, int argc, char* argv[]) } break; case -1: // Error happened. - return InitError(Untranslated(strprintf("fork_daemon() failed: %s\n", SysErrorString(errno)))); + return InitError(Untranslated(strprintf("fork_daemon() failed: %s", SysErrorString(errno)))); default: { // Parent: wait and exit. int token = daemon_ep.TokenRead(); if (token) { // Success @@ -222,7 +209,7 @@ static bool AppInit(NodeContext& node, int argc, char* argv[]) } } #else - return InitError(Untranslated("-daemon is not supported on this operating system\n")); + return InitError(Untranslated("-daemon is not supported on this operating system")); #endif // HAVE_DECL_FORK } // Lock data directory after daemonization diff --git a/src/common/init.cpp b/src/common/init.cpp new file mode 100644 index 0000000000..159eb7e2ef --- /dev/null +++ b/src/common/init.cpp @@ -0,0 +1,74 @@ +// Copyright (c) 2023 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 <common/init.h> +#include <chainparams.h> +#include <fs.h> +#include <tinyformat.h> +#include <util/system.h> +#include <util/translation.h> + +#include <algorithm> +#include <exception> +#include <optional> + +namespace common { +std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn settings_abort_fn) +{ + try { + if (!CheckDataDirOption(args)) { + return ConfigError{ConfigStatus::FAILED, strprintf(_("Specified data directory \"%s\" does not exist."), args.GetArg("-datadir", ""))}; + } + std::string error; + if (!args.ReadConfigFiles(error, true)) { + return ConfigError{ConfigStatus::FAILED, strprintf(_("Error reading configuration file: %s"), error)}; + } + + // Check for chain settings (Params() calls are only valid after this clause) + SelectParams(args.GetChainName()); + + // Create datadir if it does not exist. + const auto base_path{args.GetDataDirBase()}; + if (!fs::exists(base_path)) { + // When creating a *new* datadir, also create a "wallets" subdirectory, + // whether or not the wallet is enabled now, so if the wallet is enabled + // in the future, it will use the "wallets" subdirectory for creating + // and listing wallets, rather than the top-level directory where + // wallets could be mixed up with other files. For backwards + // compatibility, wallet code will use the "wallets" subdirectory only + // if it already exists, but never create it itself. There is discussion + // in https://github.com/bitcoin/bitcoin/issues/16220 about ways to + // change wallet code so it would no longer be necessary to create + // "wallets" subdirectories here. + fs::create_directories(base_path / "wallets"); + } + const auto net_path{args.GetDataDirNet()}; + if (!fs::exists(net_path)) { + fs::create_directories(net_path / "wallets"); + } + + // Create settings.json if -nosettings was not specified. + if (args.GetSettingsPath()) { + std::vector<std::string> details; + if (!args.ReadSettingsFile(&details)) { + const bilingual_str& message = _("Settings file could not be read"); + if (!settings_abort_fn) { + return ConfigError{ConfigStatus::FAILED, message, details}; + } else if (settings_abort_fn(message, details)) { + return ConfigError{ConfigStatus::ABORTED, message, details}; + } else { + details.clear(); // User chose to ignore the error and proceed. + } + } + if (!args.WriteSettingsFile(&details)) { + const bilingual_str& message = _("Settings file could not be written"); + return ConfigError{ConfigStatus::FAILED_WRITE, message, details}; + } + } + } catch (const std::exception& e) { + return ConfigError{ConfigStatus::FAILED, Untranslated(e.what())}; + } + return {}; +} +} // namespace common diff --git a/src/common/init.h b/src/common/init.h new file mode 100644 index 0000000000..380ac3ac7e --- /dev/null +++ b/src/common/init.h @@ -0,0 +1,39 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_COMMON_INIT_H +#define BITCOIN_COMMON_INIT_H + +#include <util/translation.h> + +#include <functional> +#include <optional> +#include <string> +#include <vector> + +class ArgsManager; + +namespace common { +enum class ConfigStatus { + FAILED, //!< Failed generically. + FAILED_WRITE, //!< Failed to write settings.json + ABORTED, //!< Aborted by user +}; + +struct ConfigError { + ConfigStatus status; + bilingual_str message{}; + std::vector<std::string> details{}; +}; + +//! Callback function to let the user decide whether to abort loading if +//! settings.json file exists and can't be parsed, or to ignore the error and +//! overwrite the file. +using SettingsAbortFn = std::function<bool(const bilingual_str& message, const std::vector<std::string>& details)>; + +/* Read config files, and create datadir and settings.json if they don't exist. */ +std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn settings_abort_fn = nullptr); +} // namespace common + +#endif // BITCOIN_COMMON_INIT_H diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 4e4f27f1be..942caa042d 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -21,12 +21,14 @@ #include <util/threadnames.h> #include <util/translation.h> +#include <condition_variable> #include <cstdio> #include <cstdlib> #include <deque> #include <memory> #include <optional> #include <string> +#include <unordered_set> #include <sys/types.h> #include <sys/stat.h> @@ -34,6 +36,7 @@ #include <event2/buffer.h> #include <event2/bufferevent.h> #include <event2/http.h> +#include <event2/http_struct.h> #include <event2/keyvalq_struct.h> #include <event2/thread.h> #include <event2/util.h> @@ -146,6 +149,10 @@ static GlobalMutex g_httppathhandlers_mutex; static std::vector<HTTPPathHandler> pathHandlers GUARDED_BY(g_httppathhandlers_mutex); //! Bound listening sockets static std::vector<evhttp_bound_socket *> boundSockets; +//! Track active requests +static GlobalMutex g_requests_mutex; +static std::condition_variable g_requests_cv; +static std::unordered_set<evhttp_request*> g_requests GUARDED_BY(g_requests_mutex); /** Check if a network address is allowed to access the HTTP server */ static bool ClientAllowed(const CNetAddr& netaddr) @@ -207,6 +214,17 @@ std::string RequestMethodString(HTTPRequest::RequestMethod m) /** HTTP request callback */ static void http_request_cb(struct evhttp_request* req, void* arg) { + // Track requests and notify when a request is completed. + { + WITH_LOCK(g_requests_mutex, g_requests.insert(req)); + g_requests_cv.notify_all(); + evhttp_request_set_on_complete_cb(req, [](struct evhttp_request* req, void*) { + auto n{WITH_LOCK(g_requests_mutex, return g_requests.erase(req))}; + assert(n == 1); + g_requests_cv.notify_all(); + }, nullptr); + } + // Disable reading to work around a libevent bug, fixed in 2.2.0. if (event_get_version_number() >= 0x02010600 && event_get_version_number() < 0x02020001) { evhttp_connection* conn = evhttp_request_get_connection(req); @@ -458,15 +476,27 @@ void StopHTTPServer() evhttp_del_accept_socket(eventHTTP, socket); } boundSockets.clear(); - if (eventBase) { - LogPrint(BCLog::HTTP, "Waiting for HTTP event thread to exit\n"); - if (g_thread_http.joinable()) g_thread_http.join(); + { + WAIT_LOCK(g_requests_mutex, lock); + if (!g_requests.empty()) { + LogPrint(BCLog::HTTP, "Waiting for %d requests to stop HTTP server\n", g_requests.size()); + } + g_requests_cv.wait(lock, []() EXCLUSIVE_LOCKS_REQUIRED(g_requests_mutex) { + return g_requests.empty(); + }); } if (eventHTTP) { - evhttp_free(eventHTTP); - eventHTTP = nullptr; + // Schedule a callback to call evhttp_free in the event base thread, so + // that evhttp_free does not need to be called again after the handling + // of unfinished request connections that follows. + event_base_once(eventBase, -1, EV_TIMEOUT, [](evutil_socket_t, short, void*) { + evhttp_free(eventHTTP); + eventHTTP = nullptr; + }, nullptr, nullptr); } if (eventBase) { + LogPrint(BCLog::HTTP, "Waiting for HTTP event thread to exit\n"); + if (g_thread_http.joinable()) g_thread_http.join(); event_base_free(eventBase); eventBase = nullptr; } diff --git a/src/index/base.cpp b/src/index/base.cpp index 6f2ce2efe4..7c570d4534 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -35,7 +35,7 @@ static void FatalError(const char* fmt, const Args&... args) std::string strMessage = tfm::format(fmt, args...); SetMiscWarning(Untranslated(strMessage)); LogPrintf("*** %s\n", strMessage); - AbortError(_("A fatal internal error occurred, see debug.log for details")); + InitError(_("A fatal internal error occurred, see debug.log for details")); StartShutdown(); } diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 626010d26f..125d6de5a5 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -28,38 +28,13 @@ #include <vector> namespace node { -ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes, - const ChainstateLoadOptions& options) +// Complete initialization of chainstates after the initial call has been made +// to ChainstateManager::InitializeChainstate(). +static ChainstateLoadResult CompleteChainstateInitialization( + ChainstateManager& chainman, + const CacheSizes& cache_sizes, + const ChainstateLoadOptions& options) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { - auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { - return options.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull(); - }; - - if (!chainman.AssumedValidBlock().IsNull()) { - LogPrintf("Assuming ancestors of block %s have valid signatures.\n", chainman.AssumedValidBlock().GetHex()); - } else { - LogPrintf("Validating signatures for all blocks.\n"); - } - LogPrintf("Setting nMinimumChainWork=%s\n", chainman.MinimumChainWork().GetHex()); - if (chainman.MinimumChainWork() < UintToArith256(chainman.GetConsensus().nMinimumChainWork)) { - LogPrintf("Warning: nMinimumChainWork set below default value of %s\n", chainman.GetConsensus().nMinimumChainWork.GetHex()); - } - if (chainman.m_blockman.GetPruneTarget() == std::numeric_limits<uint64_t>::max()) { - LogPrintf("Block pruning enabled. Use RPC call pruneblockchain(height) to manually prune block and undo files.\n"); - } else if (chainman.m_blockman.GetPruneTarget()) { - LogPrintf("Prune configured to target %u MiB on disk for block and undo files.\n", chainman.m_blockman.GetPruneTarget() / 1024 / 1024); - } - - LOCK(cs_main); - chainman.m_total_coinstip_cache = cache_sizes.coins; - chainman.m_total_coinsdb_cache = cache_sizes.coins_db; - - // Load the fully validated chainstate. - chainman.InitializeChainstate(options.mempool); - - // Load a chain created from a UTXO snapshot, if any exist. - chainman.DetectSnapshotChainstate(options.mempool); - auto& pblocktree{chainman.m_blockman.m_block_tree_db}; // new CBlockTreeDB tries to delete the existing file, which // fails if it's still open from the previous loop. Close it first: @@ -111,6 +86,13 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize return {ChainstateLoadStatus::FAILURE, _("Error initializing block database")}; } + auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + return options.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull(); + }; + + assert(chainman.m_total_coinstip_cache > 0); + assert(chainman.m_total_coinsdb_cache > 0); + // Conservative value which is arbitrarily chosen, as it will ultimately be changed // by a call to `chainman.MaybeRebalanceCaches()`. We just need to make sure // that the sum of the two caches (40%) does not exceed the allowable amount @@ -175,6 +157,84 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize return {ChainstateLoadStatus::SUCCESS, {}}; } +ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes, + const ChainstateLoadOptions& options) +{ + if (!chainman.AssumedValidBlock().IsNull()) { + LogPrintf("Assuming ancestors of block %s have valid signatures.\n", chainman.AssumedValidBlock().GetHex()); + } else { + LogPrintf("Validating signatures for all blocks.\n"); + } + LogPrintf("Setting nMinimumChainWork=%s\n", chainman.MinimumChainWork().GetHex()); + if (chainman.MinimumChainWork() < UintToArith256(chainman.GetConsensus().nMinimumChainWork)) { + LogPrintf("Warning: nMinimumChainWork set below default value of %s\n", chainman.GetConsensus().nMinimumChainWork.GetHex()); + } + if (chainman.m_blockman.GetPruneTarget() == std::numeric_limits<uint64_t>::max()) { + LogPrintf("Block pruning enabled. Use RPC call pruneblockchain(height) to manually prune block and undo files.\n"); + } else if (chainman.m_blockman.GetPruneTarget()) { + LogPrintf("Prune configured to target %u MiB on disk for block and undo files.\n", chainman.m_blockman.GetPruneTarget() / 1024 / 1024); + } + + LOCK(cs_main); + + chainman.m_total_coinstip_cache = cache_sizes.coins; + chainman.m_total_coinsdb_cache = cache_sizes.coins_db; + + // Load the fully validated chainstate. + chainman.InitializeChainstate(options.mempool); + + // Load a chain created from a UTXO snapshot, if any exist. + chainman.DetectSnapshotChainstate(options.mempool); + + auto [init_status, init_error] = CompleteChainstateInitialization(chainman, cache_sizes, options); + if (init_status != ChainstateLoadStatus::SUCCESS) { + return {init_status, init_error}; + } + + // If a snapshot chainstate was fully validated by a background chainstate during + // the last run, detect it here and clean up the now-unneeded background + // chainstate. + // + // Why is this cleanup done here (on subsequent restart) and not just when the + // snapshot is actually validated? Because this entails unusual + // filesystem operations to move leveldb data directories around, and that seems + // too risky to do in the middle of normal runtime. + auto snapshot_completion = chainman.MaybeCompleteSnapshotValidation(); + + if (snapshot_completion == SnapshotCompletionResult::SKIPPED) { + // do nothing; expected case + } else if (snapshot_completion == SnapshotCompletionResult::SUCCESS) { + LogPrintf("[snapshot] cleaning up unneeded background chainstate, then reinitializing\n"); + if (!chainman.ValidatedSnapshotCleanup()) { + AbortNode("Background chainstate cleanup failed unexpectedly."); + } + + // Because ValidatedSnapshotCleanup() has torn down chainstates with + // ChainstateManager::ResetChainstates(), reinitialize them here without + // duplicating the blockindex work above. + assert(chainman.GetAll().empty()); + assert(!chainman.IsSnapshotActive()); + assert(!chainman.IsSnapshotValidated()); + + chainman.InitializeChainstate(options.mempool); + + // A reload of the block index is required to recompute setBlockIndexCandidates + // for the fully validated chainstate. + chainman.ActiveChainstate().UnloadBlockIndex(); + + auto [init_status, init_error] = CompleteChainstateInitialization(chainman, cache_sizes, options); + if (init_status != ChainstateLoadStatus::SUCCESS) { + return {init_status, init_error}; + } + } else { + return {ChainstateLoadStatus::FAILURE, _( + "UTXO snapshot failed to validate. " + "Restart to resume normal initial block download, or try loading a different snapshot.")}; + } + + return {ChainstateLoadStatus::SUCCESS, {}}; +} + ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options) { auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { diff --git a/src/node/interface_ui.cpp b/src/node/interface_ui.cpp index 08d1e03541..9dd1e7d9cf 100644 --- a/src/node/interface_ui.cpp +++ b/src/node/interface_ui.cpp @@ -4,6 +4,7 @@ #include <node/interface_ui.h> +#include <util/string.h> #include <util/translation.h> #include <boost/signals2/optional_last_value.hpp> @@ -62,6 +63,18 @@ bool InitError(const bilingual_str& str) return false; } +bool InitError(const bilingual_str& str, const std::vector<std::string>& details) +{ + // For now just flatten the list of error details into a string to pass to + // the base InitError overload. In the future, if more init code provides + // error details, the details could be passed separately from the main + // message for rich display in the GUI. But currently the only init + // 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))); +} + void InitWarning(const bilingual_str& str) { uiInterface.ThreadSafeMessageBox(str, "", CClientUIInterface::MSG_WARNING); diff --git a/src/node/interface_ui.h b/src/node/interface_ui.h index 9f6503b4a1..22c241cb78 100644 --- a/src/node/interface_ui.h +++ b/src/node/interface_ui.h @@ -116,7 +116,7 @@ void InitWarning(const bilingual_str& str); /** Show error message **/ bool InitError(const bilingual_str& str); -constexpr auto AbortError = InitError; +bool InitError(const bilingual_str& str, const std::vector<std::string>& details); extern CClientUIInterface uiInterface; diff --git a/src/node/minisketchwrapper.cpp b/src/node/minisketchwrapper.cpp index 67e823cb68..96707f7a0a 100644 --- a/src/node/minisketchwrapper.cpp +++ b/src/node/minisketchwrapper.cpp @@ -23,17 +23,17 @@ static constexpr uint32_t BITS = 32; uint32_t FindBestImplementation() { - std::optional<std::pair<int64_t, uint32_t>> best; + std::optional<std::pair<SteadyClock::duration, uint32_t>> best; uint32_t max_impl = Minisketch::MaxImplementation(); for (uint32_t impl = 0; impl <= max_impl; ++impl) { - std::vector<int64_t> benches; + std::vector<SteadyClock::duration> benches; uint64_t offset = 0; /* Run a little benchmark with capacity 32, adding 184 entries, and decoding 11 of them once. */ for (int b = 0; b < 11; ++b) { if (!Minisketch::ImplementationSupported(BITS, impl)) break; Minisketch sketch(BITS, impl, 32); - auto start = GetTimeMicros(); + auto start = SteadyClock::now(); for (uint64_t e = 0; e < 100; ++e) { sketch.Add(e*1337 + b*13337 + offset); } @@ -41,7 +41,7 @@ uint32_t FindBestImplementation() sketch.Add(e*1337 + b*13337 + offset); } offset += (*sketch.Decode(32))[0]; - auto stop = GetTimeMicros(); + auto stop = SteadyClock::now(); benches.push_back(stop - start); } /* Remember which implementation has the best median benchmark time. */ diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 99faa51ea0..5244b72689 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -9,6 +9,7 @@ #include <qt/bitcoin.h> #include <chainparams.h> +#include <common/init.h> #include <init.h> #include <interfaces/handler.h> #include <interfaces/init.h> @@ -165,54 +166,36 @@ static void initTranslations(QTranslator &qtTranslatorBase, QTranslator &qtTrans } } -static bool InitSettings() +static bool ErrorSettingsRead(const bilingual_str& error, const std::vector<std::string>& details) { - gArgs.EnsureDataDir(); - if (!gArgs.GetSettingsPath()) { - return true; // Do nothing if settings file disabled. - } - - std::vector<std::string> errors; - if (!gArgs.ReadSettingsFile(&errors)) { - std::string error = QT_TRANSLATE_NOOP("bitcoin-core", "Settings file could not be read"); - std::string error_translated = QCoreApplication::translate("bitcoin-core", error.c_str()).toStdString(); - InitError(Untranslated(strprintf("%s:\n%s\n", error, MakeUnorderedList(errors)))); - - QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error_translated)), QMessageBox::Reset | QMessageBox::Abort); - /*: Explanatory text shown on startup when the settings file cannot be read. - Prompts user to make a choice between resetting or aborting. */ - messagebox.setInformativeText(QObject::tr("Do you want to reset settings to default values, or to abort without making changes?")); - messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(errors))); - messagebox.setTextFormat(Qt::PlainText); - messagebox.setDefaultButton(QMessageBox::Reset); - switch (messagebox.exec()) { - case QMessageBox::Reset: - break; - case QMessageBox::Abort: - return false; - default: - assert(false); - } - } - - errors.clear(); - if (!gArgs.WriteSettingsFile(&errors)) { - std::string error = QT_TRANSLATE_NOOP("bitcoin-core", "Settings file could not be written"); - std::string error_translated = QCoreApplication::translate("bitcoin-core", error.c_str()).toStdString(); - InitError(Untranslated(strprintf("%s:\n%s\n", error, MakeUnorderedList(errors)))); - - QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error_translated)), QMessageBox::Ok); - /*: Explanatory text shown on startup when the settings file could not be written. - Prompts user to check that we have the ability to write to the file. - Explains that the user has the option of running without a settings file.*/ - messagebox.setInformativeText(QObject::tr("A fatal error occurred. Check that settings file is writable, or try running with -nosettings.")); - messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(errors))); - messagebox.setTextFormat(Qt::PlainText); - messagebox.setDefaultButton(QMessageBox::Ok); - messagebox.exec(); + QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Reset | QMessageBox::Abort); + /*: Explanatory text shown on startup when the settings file cannot be read. + Prompts user to make a choice between resetting or aborting. */ + messagebox.setInformativeText(QObject::tr("Do you want to reset settings to default values, or to abort without making changes?")); + messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(details))); + messagebox.setTextFormat(Qt::PlainText); + messagebox.setDefaultButton(QMessageBox::Reset); + switch (messagebox.exec()) { + case QMessageBox::Reset: return false; + case QMessageBox::Abort: + return true; + default: + assert(false); } - return true; +} + +static void ErrorSettingsWrite(const bilingual_str& error, const std::vector<std::string>& details) +{ + QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Ok); + /*: Explanatory text shown on startup when the settings file could not be written. + Prompts user to check that we have the ability to write to the file. + Explains that the user has the option of running without a settings file.*/ + messagebox.setInformativeText(QObject::tr("A fatal error occurred. Check that settings file is writable, or try running with -nosettings.")); + messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(details))); + messagebox.setTextFormat(Qt::PlainText); + messagebox.setDefaultButton(QMessageBox::Ok); + messagebox.exec(); } /* qDebug() message handler --> debug.log */ @@ -546,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\n"), error)); + InitError(strprintf(Untranslated("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, PACKAGE_NAME, // message cannot be translated because translations have not been initialized @@ -587,34 +570,23 @@ int GuiMain(int argc, char* argv[]) // Gracefully exit if the user cancels if (!Intro::showIfNeeded(did_show_intro, prune_MiB)) return EXIT_SUCCESS; - /// 6a. Determine availability of data directory - if (!CheckDataDirOption(gArgs)) { - InitError(strprintf(Untranslated("Specified data directory \"%s\" does not exist.\n"), gArgs.GetArg("-datadir", ""))); - QMessageBox::critical(nullptr, PACKAGE_NAME, - QObject::tr("Error: Specified data directory \"%1\" does not exist.").arg(QString::fromStdString(gArgs.GetArg("-datadir", "")))); - return EXIT_FAILURE; - } - try { - /// 6b. Parse bitcoin.conf - /// - Do not call gArgs.GetDataDirNet() before this step finishes - if (!gArgs.ReadConfigFiles(error, true)) { - InitError(strprintf(Untranslated("Error reading configuration file: %s\n"), error)); - QMessageBox::critical(nullptr, PACKAGE_NAME, - QObject::tr("Error: Cannot parse configuration file: %1.").arg(QString::fromStdString(error))); - return EXIT_FAILURE; + /// 6-7. Parse bitcoin.conf, determine network, switch to network specific + /// options, and create datadir and settings.json. + // - Do not call gArgs.GetDataDirNet() before this step finishes + // - Do not call Params() before this step + // - QSettings() will use the new application name after this, resulting in network-specific settings + // - Needs to be done before createOptionsModel + if (auto error = common::InitConfig(gArgs, ErrorSettingsRead)) { + InitError(error->message, error->details); + if (error->status == common::ConfigStatus::FAILED_WRITE) { + // Show a custom error message to provide more information in the + // case of a datadir write error. + ErrorSettingsWrite(error->message, error->details); + } else if (error->status != common::ConfigStatus::ABORTED) { + // Show a generic message in other cases, and no additional error + // message in the case of a read error if the user decided to abort. + QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error: %1").arg(QString::fromStdString(error->message.translated))); } - - /// 7. Determine network (and switch to network specific options) - // - Do not call Params() before this step - // - Do this after parsing the configuration file, as the network can be switched there - // - QSettings() will use the new application name after this, resulting in network-specific settings - // - Needs to be done before createOptionsModel - - // Check for chain settings (Params() calls are only valid after this clause) - SelectParams(gArgs.GetChainName()); - } catch(std::exception &e) { - InitError(Untranslated(strprintf("%s\n", e.what()))); - QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error: %1").arg(e.what())); return EXIT_FAILURE; } #ifdef ENABLE_WALLET @@ -622,10 +594,6 @@ int GuiMain(int argc, char* argv[]) PaymentServer::ipcParseCommandLine(argc, argv); #endif - if (!InitSettings()) { - return EXIT_FAILURE; - } - QScopedPointer<const NetworkStyle> networkStyle(NetworkStyle::instantiate(Params().NetworkIDString())); assert(!networkStyle.isNull()); // Allow for separate UI settings for testnets diff --git a/src/random.cpp b/src/random.cpp index 432592589a..f4c51574cc 100644 --- a/src/random.cpp +++ b/src/random.cpp @@ -221,14 +221,14 @@ static void SeedHardwareSlow(CSHA512& hasher) noexcept { } /** Use repeated SHA512 to strengthen the randomness in seed32, and feed into hasher. */ -static void Strengthen(const unsigned char (&seed)[32], int microseconds, CSHA512& hasher) noexcept +static void Strengthen(const unsigned char (&seed)[32], SteadyClock::duration dur, CSHA512& hasher) noexcept { CSHA512 inner_hasher; inner_hasher.Write(seed, sizeof(seed)); // Hash loop unsigned char buffer[64]; - int64_t stop = GetTimeMicros() + microseconds; + const auto stop{SteadyClock::now() + dur}; do { for (int i = 0; i < 1000; ++i) { inner_hasher.Finalize(buffer); @@ -238,7 +238,7 @@ static void Strengthen(const unsigned char (&seed)[32], int microseconds, CSHA51 // Benchmark operation and feed it into outer hasher. int64_t perf = GetPerformanceCounter(); hasher.Write((const unsigned char*)&perf, sizeof(perf)); - } while (GetTimeMicros() < stop); + } while (SteadyClock::now() < stop); // Produce output from inner state and feed it to outer hasher. inner_hasher.Finalize(buffer); @@ -492,13 +492,13 @@ static void SeedSlow(CSHA512& hasher, RNGState& rng) noexcept } /** Extract entropy from rng, strengthen it, and feed it into hasher. */ -static void SeedStrengthen(CSHA512& hasher, RNGState& rng, int microseconds) noexcept +static void SeedStrengthen(CSHA512& hasher, RNGState& rng, SteadyClock::duration dur) noexcept { // Generate 32 bytes of entropy from the RNG, and a copy of the entropy already in hasher. unsigned char strengthen_seed[32]; rng.MixExtract(strengthen_seed, sizeof(strengthen_seed), CSHA512(hasher), false); // Strengthen the seed, and feed it into hasher. - Strengthen(strengthen_seed, microseconds, hasher); + Strengthen(strengthen_seed, dur, hasher); } static void SeedPeriodic(CSHA512& hasher, RNGState& rng) noexcept @@ -518,7 +518,7 @@ static void SeedPeriodic(CSHA512& hasher, RNGState& rng) noexcept LogPrint(BCLog::RAND, "Feeding %i bytes of dynamic environment data into RNG\n", hasher.Size() - old_size); // Strengthen for 10 ms - SeedStrengthen(hasher, rng, 10000); + SeedStrengthen(hasher, rng, 10ms); } static void SeedStartup(CSHA512& hasher, RNGState& rng) noexcept @@ -538,7 +538,7 @@ static void SeedStartup(CSHA512& hasher, RNGState& rng) noexcept LogPrint(BCLog::RAND, "Feeding %i bytes of environment data into RNG\n", hasher.Size() - old_size); // Strengthen for 100 ms - SeedStrengthen(hasher, rng, 100000); + SeedStrengthen(hasher, rng, 100ms); } enum class RNGLevel { diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 28a619fe54..9459801c2c 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -2106,6 +2106,10 @@ static RPCHelpMan scantxoutset() " combo(<pubkey>) P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH outputs for the given pubkey\n" " pkh(<pubkey>) P2PKH outputs for the given pubkey\n" " sh(multi(<n>,<pubkey>,<pubkey>,...)) P2SH-multisig outputs for the given threshold and pubkeys\n" + " tr(<pubkey>) P2TR\n" + " tr(<pubkey>,{pk(<pubkey>)}) P2TR with single fallback pubkey in tapscript\n" + " rawtr(<pubkey>) P2TR with the specified key as output key rather than inner\n" + " wsh(and_v(v:pk(<pubkey>),after(2))) P2WSH miniscript with mandatory pubkey and a timelock\n" "\nIn the above, <pubkey> either refers to a fixed public key in hexadecimal notation, or to an xpub/xprv optionally followed by one\n" "or more path elements separated by \"/\", and optionally ending in \"/*\" (unhardened), or \"/*'\" or \"/*h\" (hardened) to specify all\n" "unhardened or hardened child keys.\n" diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 5ed8aee9ea..21d49fda9d 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -216,10 +216,10 @@ static RPCHelpMan getrawtransaction() {RPCResult::Type::NUM, "fee", /*optional=*/true, "transaction fee in " + CURRENCY_UNIT + ", omitted if block undo data is not available"}, {RPCResult::Type::ARR, "vin", "", { - {RPCResult::Type::OBJ, "", "utxo being spent, omitted if block undo data is not available", + {RPCResult::Type::OBJ, "", "utxo being spent", { {RPCResult::Type::ELISION, "", "Same output as verbosity = 1"}, - {RPCResult::Type::OBJ, "prevout", /*optional=*/true, "Only if undo information is available)", + {RPCResult::Type::OBJ, "prevout", /*optional=*/true, "The previous output, omitted if block undo data is not available", { {RPCResult::Type::BOOL, "generated", "Coinbase or not"}, {RPCResult::Type::NUM, "height", "The height of the prevout"}, diff --git a/src/shutdown.cpp b/src/shutdown.cpp index 57d6d2325d..2fffc0663c 100644 --- a/src/shutdown.cpp +++ b/src/shutdown.cpp @@ -27,7 +27,7 @@ bool AbortNode(const std::string& strMessage, bilingual_str user_message) if (user_message.empty()) { user_message = _("A fatal internal error occurred, see debug.log for details"); } - AbortError(user_message); + InitError(user_message); StartShutdown(); return false; } diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp index f2dc6ee739..c0a2566959 100644 --- a/src/test/system_tests.cpp +++ b/src/test/system_tests.cpp @@ -34,6 +34,13 @@ BOOST_AUTO_TEST_CASE(dummy) BOOST_AUTO_TEST_CASE(run_command) { +#ifdef WIN32 + // https://www.winehq.org/pipermail/wine-devel/2008-September/069387.html + auto hntdll = GetModuleHandleA("ntdll.dll"); + assert(hntdll); + const bool wine_runtime = GetProcAddress(hntdll, "wine_get_version"); +#endif + { const UniValue result = RunCommandParseJSON(""); BOOST_CHECK(result.isNull()); @@ -51,21 +58,27 @@ BOOST_AUTO_TEST_CASE(run_command) } { // An invalid command is handled by Boost +#ifdef WIN32 + const int expected_error{wine_runtime ? 6 : 2}; +#else + const int expected_error{2}; +#endif BOOST_CHECK_EXCEPTION(RunCommandParseJSON("invalid_command"), boost::process::process_error, [&](const boost::process::process_error& e) { BOOST_CHECK(std::string(e.what()).find("RunCommandParseJSON error:") == std::string::npos); - BOOST_CHECK_EQUAL(e.code().value(), 2); + BOOST_CHECK_EQUAL(e.code().value(), expected_error); return true; }); } { // Return non-zero exit code, no output to stderr #ifdef WIN32 - const std::string command{"cmd.exe /c call"}; + const std::string command{"cmd.exe /c exit 1"}; #else const std::string command{"false"}; #endif BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) { - BOOST_CHECK(std::string(e.what()).find(strprintf("RunCommandParseJSON error: process(%s) returned 1: \n", command)) != std::string::npos); + const std::string what{e.what()}; + BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned 1: \n", command)) != std::string::npos); return true; }); } @@ -73,7 +86,7 @@ BOOST_AUTO_TEST_CASE(run_command) // Return non-zero exit code, with error message for stderr #ifdef WIN32 const std::string command{"cmd.exe /c dir nosuchfile"}; - const std::string expected{"File Not Found"}; + const std::string expected{wine_runtime ? "File not found." : "File Not Found"}; #else const std::string command{"ls nosuchfile"}; const std::string expected{"No such file or directory"}; diff --git a/src/test/translation_tests.cpp b/src/test/translation_tests.cpp new file mode 100644 index 0000000000..bda5dfd099 --- /dev/null +++ b/src/test/translation_tests.cpp @@ -0,0 +1,21 @@ +// Copyright (c) 2023 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 <tinyformat.h> +#include <util/translation.h> + +#include <boost/test/unit_test.hpp> + +BOOST_AUTO_TEST_SUITE(translation_tests) + +BOOST_AUTO_TEST_CASE(translation_namedparams) +{ + bilingual_str arg{"original", "translated"}; + bilingual_str format{"original [%s]", "translated [%s]"}; + bilingual_str result{strprintf(format, arg)}; + BOOST_CHECK_EQUAL(result.original, "original [original]"); + BOOST_CHECK_EQUAL(result.translated, "translated [translated]"); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 78301c7c14..6fc9d0fa51 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -474,9 +474,10 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) //! Ensure that snapshot chainstates initialize properly when found on disk. BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) { - this->SetupSnapshot(); - ChainstateManager& chainman = *Assert(m_node.chainman); + Chainstate& bg_chainstate = chainman.ActiveChainstate(); + + this->SetupSnapshot(); fs::path snapshot_chainstate_dir = *node::FindSnapshotChainstateDir(); BOOST_CHECK(fs::exists(snapshot_chainstate_dir)); @@ -489,6 +490,20 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) auto all_chainstates = chainman.GetAll(); BOOST_CHECK_EQUAL(all_chainstates.size(), 2); + // "Rewind" the background chainstate so that its tip is not at the + // base block of the snapshot - this is so after simulating a node restart, + // it will initialize instead of attempting to complete validation. + // + // Note that this is not a realistic use of DisconnectTip(). + DisconnectedBlockTransactions unused_pool; + BlockValidationState unused_state; + { + LOCK2(::cs_main, bg_chainstate.MempoolMutex()); + BOOST_CHECK(bg_chainstate.DisconnectTip(unused_state, &unused_pool)); + unused_pool.clear(); // to avoid queuedTx assertion errors on teardown + } + BOOST_CHECK_EQUAL(bg_chainstate.m_chain.Height(), 109); + // Test that simulating a shutdown (resetting ChainstateManager) and then performing // chainstate reinitializing successfully cleans up the background-validation // chainstate data, and we end up with a single chainstate that is at tip. @@ -520,10 +535,160 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) // chainstate. for (Chainstate* cs : chainman_restarted.GetAll()) { if (cs != &chainman_restarted.ActiveChainstate()) { - BOOST_CHECK_EQUAL(cs->m_chain.Height(), 110); + BOOST_CHECK_EQUAL(cs->m_chain.Height(), 109); } } } } +BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup) +{ + this->SetupSnapshot(); + + ChainstateManager& chainman = *Assert(m_node.chainman); + Chainstate& active_cs = chainman.ActiveChainstate(); + auto tip_cache_before_complete = active_cs.m_coinstip_cache_size_bytes; + auto db_cache_before_complete = active_cs.m_coinsdb_cache_size_bytes; + + SnapshotCompletionResult res; + auto mock_shutdown = [](bilingual_str msg) {}; + + fs::path snapshot_chainstate_dir = *node::FindSnapshotChainstateDir(); + BOOST_CHECK(fs::exists(snapshot_chainstate_dir)); + BOOST_CHECK_EQUAL(snapshot_chainstate_dir, gArgs.GetDataDirNet() / "chainstate_snapshot"); + + BOOST_CHECK(chainman.IsSnapshotActive()); + const uint256 snapshot_tip_hash = WITH_LOCK(chainman.GetMutex(), + return chainman.ActiveTip()->GetBlockHash()); + + res = WITH_LOCK(::cs_main, + return chainman.MaybeCompleteSnapshotValidation(mock_shutdown)); + BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SUCCESS); + + WITH_LOCK(::cs_main, BOOST_CHECK(chainman.IsSnapshotValidated())); + BOOST_CHECK(chainman.IsSnapshotActive()); + + // Cache should have been rebalanced and reallocated to the "only" remaining + // chainstate. + BOOST_CHECK(active_cs.m_coinstip_cache_size_bytes > tip_cache_before_complete); + BOOST_CHECK(active_cs.m_coinsdb_cache_size_bytes > db_cache_before_complete); + + auto all_chainstates = chainman.GetAll(); + BOOST_CHECK_EQUAL(all_chainstates.size(), 1); + BOOST_CHECK_EQUAL(all_chainstates[0], &active_cs); + + // Trying completion again should return false. + res = WITH_LOCK(::cs_main, + return chainman.MaybeCompleteSnapshotValidation(mock_shutdown)); + BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SKIPPED); + + // The invalid snapshot path should not have been used. + fs::path snapshot_invalid_dir = gArgs.GetDataDirNet() / "chainstate_snapshot_INVALID"; + BOOST_CHECK(!fs::exists(snapshot_invalid_dir)); + // chainstate_snapshot should still exist. + BOOST_CHECK(fs::exists(snapshot_chainstate_dir)); + + // Test that simulating a shutdown (reseting ChainstateManager) and then performing + // chainstate reinitializing successfully cleans up the background-validation + // chainstate data, and we end up with a single chainstate that is at tip. + ChainstateManager& chainman_restarted = this->SimulateNodeRestart(); + + BOOST_TEST_MESSAGE("Performing Load/Verify/Activate of chainstate"); + + // This call reinitializes the chainstates, and should clean up the now unnecessary + // background-validation leveldb contents. + this->LoadVerifyActivateChainstate(); + + BOOST_CHECK(!fs::exists(snapshot_invalid_dir)); + // chainstate_snapshot should now *not* exist. + BOOST_CHECK(!fs::exists(snapshot_chainstate_dir)); + + const Chainstate& active_cs2 = chainman_restarted.ActiveChainstate(); + + { + LOCK(chainman_restarted.GetMutex()); + BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 1); + BOOST_CHECK(!chainman_restarted.IsSnapshotActive()); + BOOST_CHECK(!chainman_restarted.IsSnapshotValidated()); + BOOST_CHECK(active_cs2.m_coinstip_cache_size_bytes > tip_cache_before_complete); + BOOST_CHECK(active_cs2.m_coinsdb_cache_size_bytes > db_cache_before_complete); + + BOOST_CHECK_EQUAL(chainman_restarted.ActiveTip()->GetBlockHash(), snapshot_tip_hash); + BOOST_CHECK_EQUAL(chainman_restarted.ActiveHeight(), 210); + } + + BOOST_TEST_MESSAGE( + "Ensure we can mine blocks on top of the \"new\" IBD chainstate"); + mineBlocks(10); + { + LOCK(chainman_restarted.GetMutex()); + BOOST_CHECK_EQUAL(chainman_restarted.ActiveHeight(), 220); + } +} + +BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, SnapshotTestSetup) +{ + auto chainstates = this->SetupSnapshot(); + Chainstate& validation_chainstate = *std::get<0>(chainstates); + ChainstateManager& chainman = *Assert(m_node.chainman); + SnapshotCompletionResult res; + auto mock_shutdown = [](bilingual_str msg) {}; + + // Test tampering with the IBD UTXO set with an extra coin to ensure it causes + // snapshot completion to fail. + CCoinsViewCache& ibd_coins = WITH_LOCK(::cs_main, + return validation_chainstate.CoinsTip()); + Coin badcoin; + badcoin.out.nValue = InsecureRand32(); + badcoin.nHeight = 1; + badcoin.out.scriptPubKey.assign(InsecureRandBits(6), 0); + uint256 txid = InsecureRand256(); + ibd_coins.AddCoin(COutPoint(txid, 0), std::move(badcoin), false); + + fs::path snapshot_chainstate_dir = gArgs.GetDataDirNet() / "chainstate_snapshot"; + BOOST_CHECK(fs::exists(snapshot_chainstate_dir)); + + res = WITH_LOCK(::cs_main, + return chainman.MaybeCompleteSnapshotValidation(mock_shutdown)); + BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::HASH_MISMATCH); + + auto all_chainstates = chainman.GetAll(); + BOOST_CHECK_EQUAL(all_chainstates.size(), 1); + BOOST_CHECK_EQUAL(all_chainstates[0], &validation_chainstate); + BOOST_CHECK_EQUAL(&chainman.ActiveChainstate(), &validation_chainstate); + + fs::path snapshot_invalid_dir = gArgs.GetDataDirNet() / "chainstate_snapshot_INVALID"; + BOOST_CHECK(fs::exists(snapshot_invalid_dir)); + + // Test that simulating a shutdown (reseting ChainstateManager) and then performing + // chainstate reinitializing successfully loads only the fully-validated + // chainstate data, and we end up with a single chainstate that is at tip. + ChainstateManager& chainman_restarted = this->SimulateNodeRestart(); + + BOOST_TEST_MESSAGE("Performing Load/Verify/Activate of chainstate"); + + // This call reinitializes the chainstates, and should clean up the now unnecessary + // background-validation leveldb contents. + this->LoadVerifyActivateChainstate(); + + BOOST_CHECK(fs::exists(snapshot_invalid_dir)); + BOOST_CHECK(!fs::exists(snapshot_chainstate_dir)); + + { + LOCK(::cs_main); + BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 1); + BOOST_CHECK(!chainman_restarted.IsSnapshotActive()); + BOOST_CHECK(!chainman_restarted.IsSnapshotValidated()); + BOOST_CHECK_EQUAL(chainman_restarted.ActiveHeight(), 210); + } + + BOOST_TEST_MESSAGE( + "Ensure we can mine blocks on top of the \"new\" IBD chainstate"); + mineBlocks(10); + { + LOCK(::cs_main); + BOOST_CHECK_EQUAL(chainman_restarted.ActiveHeight(), 220); + } +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/util/strencodings.cpp b/src/util/strencodings.cpp index 03459dcf20..a54f408496 100644 --- a/src/util/strencodings.cpp +++ b/src/util/strencodings.cpp @@ -95,8 +95,8 @@ std::optional<std::vector<Byte>> TryParseHex(std::string_view str) } return vch; } -template std::vector<std::byte> ParseHex(std::string_view); -template std::vector<uint8_t> ParseHex(std::string_view); +template std::optional<std::vector<std::byte>> TryParseHex(std::string_view); +template std::optional<std::vector<uint8_t>> TryParseHex(std::string_view); bool SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut) { diff --git a/src/util/system.cpp b/src/util/system.cpp index 58afd264ae..5b1a1659bf 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -438,27 +438,6 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const return path; } -void ArgsManager::EnsureDataDir() const -{ - /** - * "/wallets" subdirectories are created in all **new** - * datadirs, because wallet code will create new wallets in the "wallets" - * subdirectory only if exists already, otherwise it will create them in - * the top-level datadir where they could interfere with other files. - * Wallet init code currently avoids creating "wallets" directories itself - * for backwards compatibility, but this be changed in the future and - * wallet code here could go away (#16220). - */ - auto path{GetDataDir(false)}; - if (!fs::exists(path)) { - fs::create_directories(path / "wallets"); - } - path = GetDataDir(true); - if (!fs::exists(path)) { - fs::create_directories(path / "wallets"); - } -} - void ArgsManager::ClearPathCache() { LOCK(cs_args); @@ -502,25 +481,6 @@ bool ArgsManager::IsArgSet(const std::string& strArg) const return !GetSetting(strArg).isNull(); } -bool ArgsManager::InitSettings(std::string& error) -{ - EnsureDataDir(); - if (!GetSettingsPath()) { - return true; // Do nothing if settings file disabled. - } - - std::vector<std::string> errors; - if (!ReadSettingsFile(&errors)) { - error = strprintf("Failed loading settings file:\n%s\n", MakeUnorderedList(errors)); - return false; - } - if (!WriteSettingsFile(&errors)) { - error = strprintf("Failed saving settings file:\n%s\n", MakeUnorderedList(errors)); - return false; - } - return true; -} - bool ArgsManager::GetSettingsPath(fs::path* filepath, bool temp, bool backup) const { fs::path settings = GetPathArg("-settings", BITCOIN_SETTINGS_FILENAME); diff --git a/src/util/system.h b/src/util/system.h index 3eb0a0f2b8..f7bebe1f2a 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -435,13 +435,6 @@ protected: std::optional<unsigned int> GetArgFlags(const std::string& name) const; /** - * Read and update settings file with saved settings. This needs to be - * called after SelectParams() because the settings file location is - * network-specific. - */ - bool InitSettings(std::string& error); - - /** * Get settings file path, or return false if read-write settings were * disabled with -nosettings. */ @@ -480,12 +473,6 @@ protected: */ void LogArgs() const; - /** - * If datadir does not exist, create it along with wallets/ - * subdirectory(s). - */ - void EnsureDataDir() const; - private: /** * Get data directory path diff --git a/src/util/time.h b/src/util/time.h index d45baaa378..fcf85c1e03 100644 --- a/src/util/time.h +++ b/src/util/time.h @@ -8,7 +8,7 @@ #include <compat/compat.h> -#include <chrono> +#include <chrono> // IWYU pragma: export #include <cstdint> #include <string> diff --git a/src/util/translation.h b/src/util/translation.h index 05e7da0b5a..d2b49d00b0 100644 --- a/src/util/translation.h +++ b/src/util/translation.h @@ -47,11 +47,24 @@ inline bilingual_str operator+(bilingual_str lhs, const bilingual_str& rhs) /** Mark a bilingual_str as untranslated */ inline bilingual_str Untranslated(std::string original) { return {original, original}; } +// Provide an overload of tinyformat::format which can take bilingual_str arguments. namespace tinyformat { +inline std::string TranslateArg(const bilingual_str& arg, bool translated) +{ + return translated ? arg.translated : arg.original; +} + +template <typename T> +inline T const& TranslateArg(const T& arg, bool translated) +{ + return arg; +} + template <typename... Args> bilingual_str format(const bilingual_str& fmt, const Args&... args) { - return bilingual_str{format(fmt.original, args...), format(fmt.translated, args...)}; + return bilingual_str{format(fmt.original, TranslateArg(args, false)...), + format(fmt.translated, TranslateArg(args, true)...)}; } } // namespace tinyformat diff --git a/src/validation.cpp b/src/validation.cpp index 0674454883..f3c0401c0f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2478,12 +2478,12 @@ bool Chainstate::FlushStateToDisk( } } } - const auto nNow = GetTime<std::chrono::microseconds>(); + const auto nNow{SteadyClock::now()}; // Avoid writing/flushing immediately after startup. - if (m_last_write.count() == 0) { + if (m_last_write == decltype(m_last_write){}) { m_last_write = nNow; } - if (m_last_flush.count() == 0) { + if (m_last_flush == decltype(m_last_flush){}) { m_last_flush = nNow; } // The cache is large and we're within 10% and 10 MiB of the limit, but we have time now (not in the middle of a block processing). @@ -2544,7 +2544,7 @@ bool Chainstate::FlushStateToDisk( m_last_flush = nNow; full_flush_completed = true; TRACE5(utxocache, flush, - (int64_t)(GetTimeMicros() - nNow.count()), // in microseconds (µs) + int64_t{Ticks<std::chrono::microseconds>(SteadyClock::now() - nNow)}, (uint32_t)mode, (uint64_t)coins_count, (uint64_t)coins_mem_usage, @@ -2875,6 +2875,14 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, Ticks<SecondsDouble>(time_total), Ticks<MillisecondsDouble>(time_total) / num_blocks_total); + // If we are the background validation chainstate, check to see if we are done + // validating the snapshot (i.e. our tip has reached the snapshot's base block). + if (this != &m_chainman.ActiveChainstate()) { + // This call may set `m_disabled`, which is referenced immediately afterwards in + // ActivateBestChain, so that we stop connecting blocks past the snapshot base. + m_chainman.MaybeCompleteSnapshotValidation(); + } + connectTrace.BlockConnected(pindexNew, std::move(pthisBlock)); return true; } @@ -3097,6 +3105,14 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< // we use m_chainstate_mutex to enforce mutual exclusion so that only one caller may execute this function at a time LOCK(m_chainstate_mutex); + // Belt-and-suspenders check that we aren't attempting to advance the background + // chainstate past the snapshot base block. + if (WITH_LOCK(::cs_main, return m_disabled)) { + LogPrintf("m_disabled is set - this chainstate should not be in operation. " /* Continued */ + "Please report this as a bug. %s\n", PACKAGE_BUGREPORT); + return false; + } + CBlockIndex *pindexMostWork = nullptr; CBlockIndex *pindexNewTip = nullptr; int nStopAtHeight = gArgs.GetIntArg("-stopatheight", DEFAULT_STOPATHEIGHT); @@ -3147,6 +3163,15 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< assert(trace.pblock && trace.pindex); GetMainSignals().BlockConnected(trace.pblock, trace.pindex); } + + // This will have been toggled in + // ActivateBestChainStep -> ConnectTip -> MaybeCompleteSnapshotValidation, + // if at all, so we should catch it here. + // + // Break this do-while to ensure we don't advance past the base snapshot. + if (m_disabled) { + break; + } } while (!m_chain.Tip() || (starting_tip && CBlockIndexWorkComparator()(m_chain.Tip(), starting_tip))); if (!blocks_connected) return true; @@ -3167,6 +3192,11 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< if (nStopAtHeight && pindexNewTip && pindexNewTip->nHeight >= nStopAtHeight) StartShutdown(); + if (WITH_LOCK(::cs_main, return m_disabled)) { + // Background chainstate has reached the snapshot base block, so exit. + break; + } + // We check shutdown only after giving ActivateBestChainStep a chance to run once so that we // never shutdown before connecting the genesis block during LoadChainTip(). Previously this // caused an assert() failure during shutdown in such cases as the UTXO DB flushing checks @@ -4372,6 +4402,8 @@ bool ChainstateManager::LoadBlockIndex() assert(any_chain([](auto chainstate) { return !chainstate->reliesOnAssumedValid(); })); first_assumed_valid_height = block->nHeight; + LogPrintf("Saw first assumedvalid block at height %d (%s)\n", + first_assumed_valid_height, block->ToString()); break; } } @@ -4908,12 +4940,8 @@ std::vector<Chainstate*> ChainstateManager::GetAll() LOCK(::cs_main); std::vector<Chainstate*> out; - if (!IsSnapshotValidated() && m_ibd_chainstate) { - out.push_back(m_ibd_chainstate.get()); - } - - if (m_snapshot_chainstate) { - out.push_back(m_snapshot_chainstate.get()); + for (Chainstate* cs : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) { + if (this->IsUsable(cs)) out.push_back(cs); } return out; @@ -5099,6 +5127,19 @@ static void FlushSnapshotToDisk(CCoinsViewCache& coins_cache, bool snapshot_load coins_cache.Flush(); } +struct StopHashingException : public std::exception +{ + const char* what() const throw() override + { + return "ComputeUTXOStats interrupted by shutdown."; + } +}; + +static void SnapshotUTXOHashBreakpoint() +{ + if (ShutdownRequested()) throw StopHashingException(); +} + bool ChainstateManager::PopulateAndValidateSnapshot( Chainstate& snapshot_chainstate, AutoFile& coins_file, @@ -5222,13 +5263,18 @@ bool ChainstateManager::PopulateAndValidateSnapshot( assert(coins_cache.GetBestBlock() == base_blockhash); - auto breakpoint_fnc = [] { /* TODO insert breakpoint here? */ }; - // As above, okay to immediately release cs_main here since no other context knows // about the snapshot_chainstate. CCoinsViewDB* snapshot_coinsdb = WITH_LOCK(::cs_main, return &snapshot_chainstate.CoinsDB()); - const std::optional<CCoinsStats> maybe_stats = ComputeUTXOStats(CoinStatsHashType::HASH_SERIALIZED, snapshot_coinsdb, m_blockman, breakpoint_fnc); + std::optional<CCoinsStats> maybe_stats; + + try { + maybe_stats = ComputeUTXOStats( + CoinStatsHashType::HASH_SERIALIZED, snapshot_coinsdb, m_blockman, SnapshotUTXOHashBreakpoint); + } catch (StopHashingException const&) { + return false; + } if (!maybe_stats.has_value()) { LogPrintf("[snapshot] failed to generate coins stats\n"); return false; @@ -5296,6 +5342,149 @@ bool ChainstateManager::PopulateAndValidateSnapshot( return true; } +// Currently, this function holds cs_main for its duration, which could be for +// multiple minutes due to the ComputeUTXOStats call. This hold is necessary +// because we need to avoid advancing the background validation chainstate +// farther than the snapshot base block - and this function is also invoked +// from within ConnectTip, i.e. from within ActivateBestChain, so cs_main is +// held anyway. +// +// Eventually (TODO), we could somehow separate this function's runtime from +// maintenance of the active chain, but that will either require +// +// (i) setting `m_disabled` immediately and ensuring all chainstate accesses go +// through IsUsable() checks, or +// +// (ii) giving each chainstate its own lock instead of using cs_main for everything. +SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation( + std::function<void(bilingual_str)> shutdown_fnc) +{ + AssertLockHeld(cs_main); + if (m_ibd_chainstate.get() == &this->ActiveChainstate() || + !this->IsUsable(m_snapshot_chainstate.get()) || + !this->IsUsable(m_ibd_chainstate.get()) || + !m_ibd_chainstate->m_chain.Tip()) { + // Nothing to do - this function only applies to the background + // validation chainstate. + return SnapshotCompletionResult::SKIPPED; + } + const int snapshot_tip_height = this->ActiveHeight(); + const int snapshot_base_height = *Assert(this->GetSnapshotBaseHeight()); + const CBlockIndex& index_new = *Assert(m_ibd_chainstate->m_chain.Tip()); + + if (index_new.nHeight < snapshot_base_height) { + // Background IBD not complete yet. + return SnapshotCompletionResult::SKIPPED; + } + + assert(SnapshotBlockhash()); + uint256 snapshot_blockhash = *Assert(SnapshotBlockhash()); + + auto handle_invalid_snapshot = [&]() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + bilingual_str user_error = strprintf(_( + "%s failed to validate the -assumeutxo snapshot state. " + "This indicates a hardware problem, or a bug in the software, or a " + "bad software modification that allowed an invalid snapshot to be " + "loaded. As a result of this, the node will shut down and stop using any " + "state that was built on the snapshot, resetting the chain height " + "from %d to %d. On the next " + "restart, the node will resume syncing from %d " + "without using any snapshot data. " + "Please report this incident to %s, including how you obtained the snapshot. " + "The invalid snapshot chainstate has been left on disk in case it is " + "helpful in diagnosing the issue that caused this error."), + PACKAGE_NAME, snapshot_tip_height, snapshot_base_height, snapshot_base_height, PACKAGE_BUGREPORT + ); + + LogPrintf("[snapshot] !!! %s\n", user_error.original); + LogPrintf("[snapshot] deleting snapshot, reverting to validated chain, and stopping node\n"); + + m_active_chainstate = m_ibd_chainstate.get(); + m_snapshot_chainstate->m_disabled = true; + assert(!this->IsUsable(m_snapshot_chainstate.get())); + assert(this->IsUsable(m_ibd_chainstate.get())); + + m_snapshot_chainstate->InvalidateCoinsDBOnDisk(); + + shutdown_fnc(user_error); + }; + + if (index_new.GetBlockHash() != snapshot_blockhash) { + LogPrintf("[snapshot] supposed base block %s does not match the " /* Continued */ + "snapshot base block %s (height %d). Snapshot is not valid.", + index_new.ToString(), snapshot_blockhash.ToString(), snapshot_base_height); + handle_invalid_snapshot(); + return SnapshotCompletionResult::BASE_BLOCKHASH_MISMATCH; + } + + assert(index_new.nHeight == snapshot_base_height); + + int curr_height = m_ibd_chainstate->m_chain.Height(); + + assert(snapshot_base_height == curr_height); + assert(snapshot_base_height == index_new.nHeight); + assert(this->IsUsable(m_snapshot_chainstate.get())); + assert(this->GetAll().size() == 2); + + CCoinsViewDB& ibd_coins_db = m_ibd_chainstate->CoinsDB(); + m_ibd_chainstate->ForceFlushStateToDisk(); + + auto maybe_au_data = ExpectedAssumeutxo(curr_height, ::Params()); + if (!maybe_au_data) { + LogPrintf("[snapshot] assumeutxo data not found for height " /* Continued */ + "(%d) - refusing to validate snapshot\n", curr_height); + handle_invalid_snapshot(); + return SnapshotCompletionResult::MISSING_CHAINPARAMS; + } + + const AssumeutxoData& au_data = *maybe_au_data; + std::optional<CCoinsStats> maybe_ibd_stats; + LogPrintf("[snapshot] computing UTXO stats for background chainstate to validate " /* Continued */ + "snapshot - this could take a few minutes\n"); + try { + maybe_ibd_stats = ComputeUTXOStats( + CoinStatsHashType::HASH_SERIALIZED, + &ibd_coins_db, + m_blockman, + SnapshotUTXOHashBreakpoint); + } catch (StopHashingException const&) { + return SnapshotCompletionResult::STATS_FAILED; + } + + // XXX note that this function is slow and will hold cs_main for potentially minutes. + if (!maybe_ibd_stats) { + LogPrintf("[snapshot] failed to generate stats for validation coins db\n"); + // While this isn't a problem with the snapshot per se, this condition + // prevents us from validating the snapshot, so we should shut down and let the + // user handle the issue manually. + handle_invalid_snapshot(); + return SnapshotCompletionResult::STATS_FAILED; + } + const auto& ibd_stats = *maybe_ibd_stats; + + // Compare the background validation chainstate's UTXO set hash against the hard-coded + // assumeutxo hash we expect. + // + // TODO: For belt-and-suspenders, we could cache the UTXO set + // hash for the snapshot when it's loaded in its chainstate's leveldb. We could then + // reference that here for an additional check. + if (AssumeutxoHash{ibd_stats.hashSerialized} != au_data.hash_serialized) { + LogPrintf("[snapshot] hash mismatch: actual=%s, expected=%s\n", + ibd_stats.hashSerialized.ToString(), + au_data.hash_serialized.ToString()); + handle_invalid_snapshot(); + return SnapshotCompletionResult::HASH_MISMATCH; + } + + LogPrintf("[snapshot] snapshot beginning at %s has been fully validated\n", + snapshot_blockhash.ToString()); + + m_ibd_chainstate->m_disabled = true; + this->MaybeRebalanceCaches(); + + return SnapshotCompletionResult::SUCCESS; +} + Chainstate& ChainstateManager::ActiveChainstate() const { LOCK(::cs_main); @@ -5312,17 +5501,22 @@ bool ChainstateManager::IsSnapshotActive() const void ChainstateManager::MaybeRebalanceCaches() { AssertLockHeld(::cs_main); - if (m_ibd_chainstate && !m_snapshot_chainstate) { + bool ibd_usable = this->IsUsable(m_ibd_chainstate.get()); + bool snapshot_usable = this->IsUsable(m_snapshot_chainstate.get()); + assert(ibd_usable || snapshot_usable); + + if (ibd_usable && !snapshot_usable) { LogPrintf("[snapshot] allocating all cache to the IBD chainstate\n"); // Allocate everything to the IBD chainstate. m_ibd_chainstate->ResizeCoinsCaches(m_total_coinstip_cache, m_total_coinsdb_cache); } - else if (m_snapshot_chainstate && !m_ibd_chainstate) { + else if (snapshot_usable && !ibd_usable) { + // If background validation has completed and snapshot is our active chain... LogPrintf("[snapshot] allocating all cache to the snapshot chainstate\n"); // Allocate everything to the snapshot chainstate. m_snapshot_chainstate->ResizeCoinsCaches(m_total_coinstip_cache, m_total_coinsdb_cache); } - else if (m_ibd_chainstate && m_snapshot_chainstate) { + else if (ibd_usable && snapshot_usable) { // If both chainstates exist, determine who needs more cache based on IBD status. // // Note: shrink caches first so that we don't inadvertently overwhelm available memory. @@ -5414,3 +5608,141 @@ bool IsBIP30Unspendable(const CBlockIndex& block_index) return (block_index.nHeight==91722 && block_index.GetBlockHash() == uint256S("0x00000000000271a2dc26e7667f8419f2e15416dc6955e5a6c6cdf3f2574dd08e")) || (block_index.nHeight==91812 && block_index.GetBlockHash() == uint256S("0x00000000000af0aed4792b1acee3d966af36cf5def14935db8de83d6f9306f2f")); } + +void Chainstate::InvalidateCoinsDBOnDisk() +{ + AssertLockHeld(::cs_main); + // Should never be called on a non-snapshot chainstate. + assert(m_from_snapshot_blockhash); + auto storage_path_maybe = this->CoinsDB().StoragePath(); + // Should never be called with a non-existent storage path. + assert(storage_path_maybe); + fs::path snapshot_datadir = *storage_path_maybe; + + // Coins views no longer usable. + m_coins_views.reset(); + + auto invalid_path = snapshot_datadir + "_INVALID"; + std::string dbpath = fs::PathToString(snapshot_datadir); + std::string target = fs::PathToString(invalid_path); + LogPrintf("[snapshot] renaming snapshot datadir %s to %s\n", dbpath, target); + + // The invalid snapshot datadir is simply moved and not deleted because we may + // want to do forensics later during issue investigation. The user is instructed + // accordingly in MaybeCompleteSnapshotValidation(). + try { + fs::rename(snapshot_datadir, invalid_path); + } catch (const fs::filesystem_error& e) { + auto src_str = fs::PathToString(snapshot_datadir); + auto dest_str = fs::PathToString(invalid_path); + + LogPrintf("%s: error renaming file '%s' -> '%s': %s\n", + __func__, src_str, dest_str, e.what()); + AbortNode(strprintf( + "Rename of '%s' -> '%s' failed. " + "You should resolve this by manually moving or deleting the invalid " + "snapshot directory %s, otherwise you will encounter the same error again " + "on the next startup.", + src_str, dest_str, src_str)); + } +} + +const CBlockIndex* ChainstateManager::GetSnapshotBaseBlock() const +{ + const auto blockhash_op = this->SnapshotBlockhash(); + if (!blockhash_op) return nullptr; + return Assert(m_blockman.LookupBlockIndex(*blockhash_op)); +} + +std::optional<int> ChainstateManager::GetSnapshotBaseHeight() const +{ + const CBlockIndex* base = this->GetSnapshotBaseBlock(); + return base ? std::make_optional(base->nHeight) : std::nullopt; +} + +bool ChainstateManager::ValidatedSnapshotCleanup() +{ + AssertLockHeld(::cs_main); + auto get_storage_path = [](auto& chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) -> std::optional<fs::path> { + if (!(chainstate && chainstate->HasCoinsViews())) { + return {}; + } + return chainstate->CoinsDB().StoragePath(); + }; + std::optional<fs::path> ibd_chainstate_path_maybe = get_storage_path(m_ibd_chainstate); + std::optional<fs::path> snapshot_chainstate_path_maybe = get_storage_path(m_snapshot_chainstate); + + if (!this->IsSnapshotValidated()) { + // No need to clean up. + return false; + } + // If either path doesn't exist, that means at least one of the chainstates + // is in-memory, in which case we can't do on-disk cleanup. You'd better be + // in a unittest! + if (!ibd_chainstate_path_maybe || !snapshot_chainstate_path_maybe) { + LogPrintf("[snapshot] snapshot chainstate cleanup cannot happen with " /* Continued */ + "in-memory chainstates. You are testing, right?\n"); + return false; + } + + const auto& snapshot_chainstate_path = *snapshot_chainstate_path_maybe; + const auto& ibd_chainstate_path = *ibd_chainstate_path_maybe; + + // Since we're going to be moving around the underlying leveldb filesystem content + // for each chainstate, make sure that the chainstates (and their constituent + // CoinsViews members) have been destructed first. + // + // The caller of this method will be responsible for reinitializing chainstates + // if they want to continue operation. + this->ResetChainstates(); + + // No chainstates should be considered usable. + assert(this->GetAll().size() == 0); + + LogPrintf("[snapshot] deleting background chainstate directory (now unnecessary) (%s)\n", + fs::PathToString(ibd_chainstate_path)); + + fs::path tmp_old{ibd_chainstate_path + "_todelete"}; + + auto rename_failed_abort = []( + fs::path p_old, + fs::path p_new, + const fs::filesystem_error& err) { + LogPrintf("%s: error renaming file (%s): %s\n", + __func__, fs::PathToString(p_old), err.what()); + AbortNode(strprintf( + "Rename of '%s' -> '%s' failed. " + "Cannot clean up the background chainstate leveldb directory.", + fs::PathToString(p_old), fs::PathToString(p_new))); + }; + + try { + fs::rename(ibd_chainstate_path, tmp_old); + } catch (const fs::filesystem_error& e) { + rename_failed_abort(ibd_chainstate_path, tmp_old, e); + throw; + } + + LogPrintf("[snapshot] moving snapshot chainstate (%s) to " /* Continued */ + "default chainstate directory (%s)\n", + fs::PathToString(snapshot_chainstate_path), fs::PathToString(ibd_chainstate_path)); + + try { + fs::rename(snapshot_chainstate_path, ibd_chainstate_path); + } catch (const fs::filesystem_error& e) { + rename_failed_abort(snapshot_chainstate_path, ibd_chainstate_path, e); + throw; + } + + if (!DeleteCoinsDBFromDisk(tmp_old, /*is_snapshot=*/false)) { + // No need to AbortNode because once the unneeded bg chainstate data is + // moved, it will not interfere with subsequent initialization. + LogPrintf("Deletion of %s failed. Please remove it manually, as the " /* Continued */ + "directory is now unnecessary.\n", + fs::PathToString(tmp_old)); + } else { + LogPrintf("[snapshot] deleted background chainstate directory (%s)\n", + fs::PathToString(ibd_chainstate_path)); + } + return true; +} diff --git a/src/validation.h b/src/validation.h index 067d2ea6d2..b0cef0d37b 100644 --- a/src/validation.h +++ b/src/validation.h @@ -24,6 +24,7 @@ #include <policy/packages.h> #include <policy/policy.h> #include <script/script_error.h> +#include <shutdown.h> #include <sync.h> #include <txdb.h> #include <txmempool.h> // For CTxMemPool::cs @@ -493,6 +494,19 @@ protected: //! Manages the UTXO set, which is a reflection of the contents of `m_chain`. std::unique_ptr<CoinsViews> m_coins_views; + //! This toggle exists for use when doing background validation for UTXO + //! snapshots. + //! + //! In the expected case, it is set once the background validation chain reaches the + //! same height as the base of the snapshot and its UTXO set is found to hash to + //! the expected assumeutxo value. It signals that we should no longer connect + //! blocks to the background chainstate. When set on the background validation + //! chainstate, it signifies that we have fully validated the snapshot chainstate. + //! + //! In the unlikely case that the snapshot chainstate is found to be invalid, this + //! is set to true on the snapshot chainstate. + bool m_disabled GUARDED_BY(::cs_main) {false}; + public: //! Reference to a BlockManager instance which itself is shared across all //! Chainstate instances. @@ -560,15 +574,15 @@ public: CCoinsViewCache& CoinsTip() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); - assert(m_coins_views->m_cacheview); - return *m_coins_views->m_cacheview.get(); + Assert(m_coins_views); + return *Assert(m_coins_views->m_cacheview); } //! @returns A reference to the on-disk UTXO set database. CCoinsViewDB& CoinsDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); - return m_coins_views->m_dbview; + return Assert(m_coins_views)->m_dbview; } //! @returns A pointer to the mempool. @@ -582,12 +596,15 @@ public: CCoinsViewErrorCatcher& CoinsErrorCatcher() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); - return m_coins_views->m_catcherview; + return Assert(m_coins_views)->m_catcherview; } //! Destructs all objects related to accessing the UTXO set. void ResetCoinsViews() { m_coins_views.reset(); } + //! Does this chainstate have a UTXO set attached? + bool HasCoinsViews() const { return (bool)m_coins_views; } + //! The cache size of the on-disk coins view. size_t m_coinsdb_cache_size_bytes{0}; @@ -667,6 +684,12 @@ public: * May not be called with cs_main held. May not be called in a * validationinterface callback. * + * Note that if this is called while a snapshot chainstate is active, and if + * it is called on a background chainstate whose tip has reached the base block + * of the snapshot, its execution will take *MINUTES* while it hashes the + * background UTXO set to verify the assumeutxo value the snapshot was activated + * with. `cs_main` will be held during this time. + * * @returns true unless a system error occurred */ bool ActivateBestChain( @@ -745,6 +768,12 @@ public: std::string ToString() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! Indirection necessary to make lock annotations work with an optional mempool. + RecursiveMutex* MempoolMutex() const LOCK_RETURNED(m_mempool->cs) + { + return m_mempool ? &m_mempool->cs : nullptr; + } + private: bool ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs); bool ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs); @@ -758,12 +787,6 @@ private: void CheckForkWarningConditions() EXCLUSIVE_LOCKS_REQUIRED(cs_main); void InvalidChainFound(CBlockIndex* pindexNew) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - //! Indirection necessary to make lock annotations work with an optional mempool. - RecursiveMutex* MempoolMutex() const LOCK_RETURNED(m_mempool->cs) - { - return m_mempool ? &m_mempool->cs : nullptr; - } - /** * Make mempool consistent after a reorg, by re-adding or recursively erasing * disconnected block transactions from the mempool, and also removing any @@ -785,12 +808,40 @@ private: void UpdateTip(const CBlockIndex* pindexNew) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - std::chrono::microseconds m_last_write{0}; - std::chrono::microseconds m_last_flush{0}; + SteadyClock::time_point m_last_write{}; + SteadyClock::time_point m_last_flush{}; + + /** + * In case of an invalid snapshot, rename the coins leveldb directory so + * that it can be examined for issue diagnosis. + */ + void InvalidateCoinsDBOnDisk() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); friend ChainstateManager; }; + +enum class SnapshotCompletionResult { + SUCCESS, + SKIPPED, + + // Expected assumeutxo configuration data is not found for the height of the + // base block. + MISSING_CHAINPARAMS, + + // Failed to generate UTXO statistics (to check UTXO set hash) for the background + // chainstate. + STATS_FAILED, + + // The UTXO set hash of the background validation chainstate does not match + // the one expected by assumeutxo chainparams. + HASH_MISMATCH, + + // The blockhash of the current tip of the background validation chainstate does + // not match the one expected by the snapshot chainstate. + BASE_BLOCKHASH_MISMATCH, +}; + /** * Provides an interface for creating and interacting with one or two * chainstates: an IBD chainstate generated by downloading blocks, and @@ -860,10 +911,6 @@ private: //! that call. Chainstate* m_active_chainstate GUARDED_BY(::cs_main) {nullptr}; - //! If true, the assumed-valid chainstate has been fully validated - //! by the background validation chainstate. - bool m_snapshot_validated GUARDED_BY(::cs_main){false}; - CBlockIndex* m_best_invalid GUARDED_BY(::cs_main){nullptr}; //! Internal helper for ActivateSnapshot(). @@ -889,6 +936,22 @@ private: /** Most recent headers presync progress update, for rate-limiting. */ std::chrono::time_point<std::chrono::steady_clock> m_last_presync_update GUARDED_BY(::cs_main) {}; + //! Returns nullptr if no snapshot has been loaded. + const CBlockIndex* GetSnapshotBaseBlock() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + + //! Return the height of the base block of the snapshot in use, if one exists, else + //! nullopt. + std::optional<int> GetSnapshotBaseHeight() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + + //! Return true if a chainstate is considered usable. + //! + //! This is false when a background validation chainstate has completed its + //! validation of an assumed-valid chainstate, or when a snapshot + //! chainstate has been found to be invalid. + bool IsUsable(const Chainstate* const cs) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + return cs && !cs->m_disabled; + } + public: using Options = kernel::ChainstateManagerOpts; @@ -976,6 +1039,18 @@ public: [[nodiscard]] bool ActivateSnapshot( AutoFile& coins_file, const node::SnapshotMetadata& metadata, bool in_memory); + //! Once the background validation chainstate has reached the height which + //! is the base of the UTXO snapshot in use, compare its coins to ensure + //! they match those expected by the snapshot. + //! + //! If the coins match (expected), then mark the validation chainstate for + //! deletion and continue using the snapshot chainstate as active. + //! Otherwise, revert to using the ibd chainstate and shutdown. + SnapshotCompletionResult MaybeCompleteSnapshotValidation( + std::function<void(bilingual_str)> shutdown_fnc = + [](bilingual_str msg) { AbortNode(msg.original, msg); }) + EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! The most-work chain. Chainstate& ActiveChainstate() const; CChain& ActiveChain() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChainstate().m_chain; } @@ -1000,7 +1075,10 @@ public: std::optional<uint256> SnapshotBlockhash() const; //! Is there a snapshot in use and has it been fully validated? - bool IsSnapshotValidated() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { return m_snapshot_validated; } + bool IsSnapshotValidated() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) + { + return m_snapshot_chainstate && m_ibd_chainstate && m_ibd_chainstate->m_disabled; + } /** * Process an incoming block. This only returns after the best known valid @@ -1080,6 +1158,17 @@ public: Chainstate& ActivateExistingSnapshot(CTxMemPool* mempool, uint256 base_blockhash) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! If we have validated a snapshot chain during this runtime, copy its + //! chainstate directory over to the main `chainstate` location, completing + //! validation of the snapshot. + //! + //! If the cleanup succeeds, the caller will need to ensure chainstates are + //! reinitialized, since ResetChainstates() will be called before leveldb + //! directories are moved or deleted. + //! + //! @sa node/chainstate:LoadChainstate() + bool ValidatedSnapshotCleanup() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + ~ChainstateManager(); }; diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index e6ba89627c..3fd0280b8b 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -333,12 +333,9 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, ******************************************************************************/ -void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descendants, bool positive_only) { - // Filter for positive only here before adding the coin - if (positive_only && output.GetEffectiveValue() <= 0) return; - +void OutputGroup::Insert(const std::shared_ptr<COutput>& output, size_t ancestors, size_t descendants) { m_outputs.push_back(output); - COutput& coin = m_outputs.back(); + auto& coin = *m_outputs.back(); fee += coin.GetFee(); @@ -358,8 +355,8 @@ void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descend // coin itself; thus, this value is counted as the max, not the sum m_descendants = std::max(m_descendants, descendants); - if (output.input_bytes > 0) { - m_weight += output.input_bytes * WITNESS_SCALE_FACTOR; + if (output->input_bytes > 0) { + m_weight += output->input_bytes * WITNESS_SCALE_FACTOR; } } @@ -375,7 +372,29 @@ CAmount OutputGroup::GetSelectionAmount() const return m_subtract_fee_outputs ? m_value : effective_value; } -CAmount GetSelectionWaste(const std::set<COutput>& inputs, CAmount change_cost, CAmount target, bool use_effective_value) +void OutputGroupTypeMap::Push(const OutputGroup& group, OutputType type, bool insert_positive, bool insert_mixed) +{ + if (group.m_outputs.empty()) return; + + Groups& groups = groups_by_type[type]; + if (insert_positive && group.GetSelectionAmount() > 0) { + groups.positive_group.emplace_back(group); + all_groups.positive_group.emplace_back(group); + } + if (insert_mixed) { + groups.mixed_group.emplace_back(group); + all_groups.mixed_group.emplace_back(group); + } +} + +std::optional<Groups> OutputGroupTypeMap::Find(OutputType type) +{ + auto it_by_type = groups_by_type.find(type); + if (it_by_type == groups_by_type.end()) return std::nullopt; + return it_by_type->second; +} + +CAmount GetSelectionWaste(const std::set<std::shared_ptr<COutput>>& inputs, CAmount change_cost, CAmount target, bool use_effective_value) { // This function should not be called with empty inputs as that would mean the selection failed assert(!inputs.empty()); @@ -383,7 +402,8 @@ CAmount GetSelectionWaste(const std::set<COutput>& inputs, CAmount change_cost, // Always consider the cost of spending an input now vs in the future. CAmount waste = 0; CAmount selected_effective_value = 0; - for (const COutput& coin : inputs) { + for (const auto& coin_ptr : inputs) { + const COutput& coin = *coin_ptr; waste += coin.GetFee() - coin.long_term_fee; selected_effective_value += use_effective_value ? coin.GetEffectiveValue() : coin.txout.nValue; } @@ -431,12 +451,12 @@ CAmount SelectionResult::GetWaste() const CAmount SelectionResult::GetSelectedValue() const { - return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin.txout.nValue; }); + return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin->txout.nValue; }); } CAmount SelectionResult::GetSelectedEffectiveValue() const { - return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin.GetEffectiveValue(); }); + return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin->GetEffectiveValue(); }); } void SelectionResult::Clear() @@ -455,14 +475,14 @@ void SelectionResult::AddInput(const OutputGroup& group) m_weight += group.m_weight; } -void SelectionResult::AddInputs(const std::set<COutput>& inputs, bool subtract_fee_outputs) +void SelectionResult::AddInputs(const std::set<std::shared_ptr<COutput>>& inputs, bool subtract_fee_outputs) { // As it can fail, combine inputs first InsertInputs(inputs); m_use_effective = !subtract_fee_outputs; m_weight += std::accumulate(inputs.cbegin(), inputs.cend(), 0, [](int sum, const auto& coin) { - return sum + std::max(coin.input_bytes, 0) * WITNESS_SCALE_FACTOR; + return sum + std::max(coin->input_bytes, 0) * WITNESS_SCALE_FACTOR; }); } @@ -480,14 +500,14 @@ void SelectionResult::Merge(const SelectionResult& other) m_weight += other.m_weight; } -const std::set<COutput>& SelectionResult::GetInputSet() const +const std::set<std::shared_ptr<COutput>>& SelectionResult::GetInputSet() const { return m_selected_inputs; } -std::vector<COutput> SelectionResult::GetShuffledInputVector() const +std::vector<std::shared_ptr<COutput>> SelectionResult::GetShuffledInputVector() const { - std::vector<COutput> coins(m_selected_inputs.begin(), m_selected_inputs.end()); + std::vector<std::shared_ptr<COutput>> coins(m_selected_inputs.begin(), m_selected_inputs.end()); Shuffle(coins.begin(), coins.end(), FastRandomContext()); return coins; } diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 9ff2011ce3..3abd22c207 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -7,6 +7,7 @@ #include <consensus/amount.h> #include <consensus/consensus.h> +#include <outputtype.h> #include <policy/feerate.h> #include <primitives/transaction.h> #include <random.h> @@ -192,13 +193,18 @@ struct CoinEligibilityFilter CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_ancestors) {} CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants) {} CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants, bool include_partial) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants), m_include_partial_groups(include_partial) {} + + bool operator<(const CoinEligibilityFilter& other) const { + return std::tie(conf_mine, conf_theirs, max_ancestors, max_descendants, m_include_partial_groups) + < std::tie(other.conf_mine, other.conf_theirs, other.max_ancestors, other.max_descendants, other.m_include_partial_groups); + } }; /** A group of UTXOs paid to the same output script. */ struct OutputGroup { /** The list of UTXOs contained in this output group. */ - std::vector<COutput> m_outputs; + std::vector<std::shared_ptr<COutput>> m_outputs; /** Whether the UTXOs were sent by the wallet to itself. This is relevant because we may want at * least a certain number of confirmations on UTXOs received from outside wallets while trusting * our own UTXOs more. */ @@ -237,11 +243,37 @@ struct OutputGroup m_subtract_fee_outputs(params.m_subtract_fee_outputs) {} - void Insert(const COutput& output, size_t ancestors, size_t descendants, bool positive_only); + void Insert(const std::shared_ptr<COutput>& output, size_t ancestors, size_t descendants); bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const; CAmount GetSelectionAmount() const; }; +struct Groups { + // Stores 'OutputGroup' containing only positive UTXOs (value > 0). + std::vector<OutputGroup> positive_group; + // Stores 'OutputGroup' which may contain both positive and negative UTXOs. + std::vector<OutputGroup> mixed_group; +}; + +/** Stores several 'Groups' whose were mapped by output type. */ +struct OutputGroupTypeMap +{ + // Maps output type to output groups. + std::map<OutputType, Groups> groups_by_type; + // All inserted groups, no type distinction. + Groups all_groups; + + // Based on the insert flag; appends group to the 'mixed_group' and, if value > 0, to the 'positive_group'. + // This affects both; the groups filtered by type and the overall groups container. + void Push(const OutputGroup& group, OutputType type, bool insert_positive, bool insert_mixed); + // Retrieves 'Groups' filtered by type + std::optional<Groups> Find(OutputType type); + // Different output types count + size_t TypesCount() { return groups_by_type.size(); } +}; + +typedef std::map<CoinEligibilityFilter, OutputGroupTypeMap> FilteredOutputGroups; + /** Compute the waste for this result given the cost of change * and the opportunity cost of spending these inputs now vs in the future. * If change exists, waste = change_cost + inputs * (effective_feerate - long_term_feerate) @@ -259,7 +291,7 @@ struct OutputGroup * @param[in] use_effective_value Whether to use the input's effective value (when true) or the real value (when false). * @return The waste */ -[[nodiscard]] CAmount GetSelectionWaste(const std::set<COutput>& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true); +[[nodiscard]] CAmount GetSelectionWaste(const std::set<std::shared_ptr<COutput>>& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true); /** Choose a random change target for each transaction to make it harder to fingerprint the Core @@ -292,7 +324,7 @@ struct SelectionResult { private: /** Set of inputs selected by the algorithm to use in the transaction */ - std::set<COutput> m_selected_inputs; + std::set<std::shared_ptr<COutput>> m_selected_inputs; /** The target the algorithm selected for. Equal to the recipient amount plus non-input fees */ CAmount m_target; /** The algorithm used to produce this result */ @@ -329,7 +361,7 @@ public: void Clear(); void AddInput(const OutputGroup& group); - void AddInputs(const std::set<COutput>& inputs, bool subtract_fee_outputs); + void AddInputs(const std::set<std::shared_ptr<COutput>>& inputs, bool subtract_fee_outputs); /** Calculates and stores the waste for this selection via GetSelectionWaste */ void ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee); @@ -344,9 +376,9 @@ public: void Merge(const SelectionResult& other); /** Get m_selected_inputs */ - const std::set<COutput>& GetInputSet() const; + const std::set<std::shared_ptr<COutput>>& GetInputSet() const; /** Get the vector of COutputs that will be used to fill in a CTransaction's vin */ - std::vector<COutput> GetShuffledInputVector() const; + std::vector<std::shared_ptr<COutput>> GetShuffledInputVector() const; bool operator<(SelectionResult other) const; diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 744537cfbd..09cfc07bc2 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -1760,7 +1760,8 @@ RPCHelpMan listdescriptors() {RPCResult::Type::NUM, "", "Range start inclusive"}, {RPCResult::Type::NUM, "", "Range end inclusive"}, }}, - {RPCResult::Type::NUM, "next", /*optional=*/true, "The next index to generate addresses from; defined only for ranged descriptors"}, + {RPCResult::Type::NUM, "next", /*optional=*/true, "Same as next_index field. Kept for compatibility reason."}, + {RPCResult::Type::NUM, "next_index", /*optional=*/true, "The next index to generate addresses from; defined only for ranged descriptors"}, }}, }} }}, @@ -1837,6 +1838,7 @@ RPCHelpMan listdescriptors() range.push_back(info.range->second - 1); spk.pushKV("range", range); spk.pushKV("next", info.next_index); + spk.pushKV("next_index", info.next_index); } descriptors.push_back(spk); } diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 865f6e41a6..a8ecce119a 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -404,28 +404,37 @@ std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet) return result; } -std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<COutput>& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only) +FilteredOutputGroups GroupOutputs(const CWallet& wallet, + const CoinsResult& coins, + const CoinSelectionParams& coin_sel_params, + const std::vector<SelectionFilter>& filters) { - std::vector<OutputGroup> groups_out; + FilteredOutputGroups filtered_groups; if (!coin_sel_params.m_avoid_partial_spends) { - // Allowing partial spends means no grouping. Each COutput gets its own OutputGroup. - for (const COutput& output : outputs) { - // Skip outputs we cannot spend - if (!output.spendable) continue; - - size_t ancestors, descendants; - wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); - - // Make an OutputGroup containing just this output - OutputGroup group{coin_sel_params}; - group.Insert(output, ancestors, descendants, positive_only); - - // Check the OutputGroup's eligibility. Only add the eligible ones. - if (positive_only && group.GetSelectionAmount() <= 0) continue; - if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter)) groups_out.push_back(group); + // Allowing partial spends means no grouping. Each COutput gets its own OutputGroup + for (const auto& [type, outputs] : coins.coins) { + for (const COutput& output : outputs) { + // Skip outputs we cannot spend + if (!output.spendable) continue; + + // Get mempool info + size_t ancestors, descendants; + wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); + + // Create a new group per output and add it to the all groups vector + OutputGroup group(coin_sel_params); + group.Insert(std::make_shared<COutput>(output), ancestors, descendants); + + // Each filter maps to a different set of groups + for (const auto& sel_filter : filters) { + const auto& filter = sel_filter.filter; + if (!group.EligibleForSpending(filter)) continue; + filtered_groups[filter].Push(group, type, /*insert_positive=*/true, /*insert_mixed=*/true); + } + } } - return groups_out; + return filtered_groups; } // We want to combine COutputs that have the same scriptPubKey into single OutputGroups @@ -434,16 +443,12 @@ std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<C // For each COutput, we check if the scriptPubKey is in the map, and if it is, the COutput is added // to the last OutputGroup in the vector for the scriptPubKey. When the last OutputGroup has // OUTPUT_GROUP_MAX_ENTRIES COutputs, a new OutputGroup is added to the end of the vector. - std::map<CScript, std::vector<OutputGroup>> spk_to_groups_map; - for (const auto& output : outputs) { - // Skip outputs we cannot spend - if (!output.spendable) continue; - - size_t ancestors, descendants; - wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); - CScript spk = output.txout.scriptPubKey; - - std::vector<OutputGroup>& groups = spk_to_groups_map[spk]; + typedef std::map<std::pair<CScript, OutputType>, std::vector<OutputGroup>> ScriptPubKeyToOutgroup; + const auto& group_outputs = []( + const COutput& output, OutputType type, size_t ancestors, size_t descendants, + ScriptPubKeyToOutgroup& groups_map, const CoinSelectionParams& coin_sel_params, + bool positive_only) { + std::vector<OutputGroup>& groups = groups_map[std::make_pair(output.txout.scriptPubKey,type)]; if (groups.size() == 0) { // No OutputGroups for this scriptPubKey yet, add one @@ -462,42 +467,69 @@ std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<C group = &groups.back(); } - // Add the output to group - group->Insert(output, ancestors, descendants, positive_only); - } - - // Now we go through the entire map and pull out the OutputGroups - for (const auto& spk_and_groups_pair: spk_to_groups_map) { - const std::vector<OutputGroup>& groups_per_spk= spk_and_groups_pair.second; + // Filter for positive only before adding the output to group + if (!positive_only || output.GetEffectiveValue() > 0) { + group->Insert(std::make_shared<COutput>(output), ancestors, descendants); + } + }; - // Go through the vector backwards. This allows for the first item we deal with being the partial group. - for (auto group_it = groups_per_spk.rbegin(); group_it != groups_per_spk.rend(); group_it++) { - const OutputGroup& group = *group_it; + ScriptPubKeyToOutgroup spk_to_groups_map; + ScriptPubKeyToOutgroup spk_to_positive_groups_map; + for (const auto& [type, outs] : coins.coins) { + for (const COutput& output : outs) { + // Skip outputs we cannot spend + if (!output.spendable) continue; - // Don't include partial groups if there are full groups too and we don't want partial groups - if (group_it == groups_per_spk.rbegin() && groups_per_spk.size() > 1 && !filter.m_include_partial_groups) { - continue; - } + size_t ancestors, descendants; + wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); - // Check the OutputGroup's eligibility. Only add the eligible ones. - if (positive_only && group.GetSelectionAmount() <= 0) continue; - if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter)) groups_out.push_back(group); + group_outputs(output, type, ancestors, descendants, spk_to_groups_map, coin_sel_params, /*positive_only=*/ false); + group_outputs(output, type, ancestors, descendants, spk_to_positive_groups_map, + coin_sel_params, /*positive_only=*/ true); } } - return groups_out; + // Now we go through the entire maps and pull out the OutputGroups + const auto& push_output_groups = [&](const ScriptPubKeyToOutgroup& groups_map, bool positive_only) { + for (const auto& [script, groups] : groups_map) { + // Go through the vector backwards. This allows for the first item we deal with being the partial group. + for (auto group_it = groups.rbegin(); group_it != groups.rend(); group_it++) { + const OutputGroup& group = *group_it; + + // Each filter maps to a different set of groups + for (const auto& sel_filter : filters) { + const auto& filter = sel_filter.filter; + if (!group.EligibleForSpending(filter)) continue; + + // Don't include partial groups if there are full groups too and we don't want partial groups + if (group_it == groups.rbegin() && groups.size() > 1 && !filter.m_include_partial_groups) { + continue; + } + + OutputType type = script.second; + // Either insert the group into the positive-only groups or the mixed ones. + filtered_groups[filter].Push(group, type, positive_only, /*insert_mixed=*/!positive_only); + } + } + } + }; + + push_output_groups(spk_to_groups_map, /*positive_only=*/ false); + push_output_groups(spk_to_positive_groups_map, /*positive_only=*/ true); + + return filtered_groups; } // Returns true if the result contains an error and the message is not empty static bool HasErrorMsg(const util::Result<SelectionResult>& res) { return !util::ErrorString(res).empty(); } -util::Result<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, +util::Result<SelectionResult> AttemptSelection(const CAmount& nTargetValue, OutputGroupTypeMap& groups, const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types) { // Run coin selection on each OutputType and compute the Waste Metric std::vector<SelectionResult> results; - for (const auto& it : available_coins.coins) { - auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, it.second, coin_selection_params)}; + for (auto& [type, group] : groups.groups_by_type) { + auto result{ChooseSelectionResult(nTargetValue, group, coin_selection_params)}; // If any specific error message appears here, then something particularly wrong happened. if (HasErrorMsg(result)) return result; // So let's return the specific error. // Append the favorable result. @@ -510,32 +542,30 @@ util::Result<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmo // If we can't fund the transaction from any individual OutputType, run coin selection one last time // over all available coins, which would allow mixing. // If TypesCount() <= 1, there is nothing to mix. - if (allow_mixed_output_types && available_coins.TypesCount() > 1) { - return ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.All(), coin_selection_params); + if (allow_mixed_output_types && groups.TypesCount() > 1) { + return ChooseSelectionResult(nTargetValue, groups.all_groups, coin_selection_params); } // Either mixing is not allowed and we couldn't find a solution from any single OutputType, or mixing was allowed and we still couldn't // find a solution using all available coins return util::Error(); }; -util::Result<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins, const CoinSelectionParams& coin_selection_params) +util::Result<SelectionResult> ChooseSelectionResult(const CAmount& nTargetValue, Groups& groups, const CoinSelectionParams& coin_selection_params) { // Vector of results. We will choose the best one based on waste. std::vector<SelectionResult> results; - std::vector<OutputGroup> positive_groups = GroupOutputs(wallet, available_coins, coin_selection_params, eligibility_filter, /*positive_only=*/true); - if (auto bnb_result{SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change)}) { + if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change)}) { results.push_back(*bnb_result); } // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here. - std::vector<OutputGroup> all_groups = GroupOutputs(wallet, available_coins, coin_selection_params, eligibility_filter, /*positive_only=*/false); - if (auto knapsack_result{KnapsackSolver(all_groups, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast)}) { + if (auto knapsack_result{KnapsackSolver(groups.mixed_group, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast)}) { knapsack_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); results.push_back(*knapsack_result); } - if (auto srd_result{SelectCoinsSRD(positive_groups, nTargetValue, coin_selection_params.rng_fast)}) { + if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, coin_selection_params.rng_fast)}) { srd_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); results.push_back(*srd_result); } @@ -607,11 +637,6 @@ util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& av return op_selection_result; } -struct SelectionFilter { - CoinEligibilityFilter filter; - bool allow_mixed_output_types{true}; -}; - util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& value_to_select, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) { unsigned int limit_ancestor_count = 0; @@ -666,12 +691,17 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin } } + // Group outputs and map them by coin eligibility filter + FilteredOutputGroups filtered_groups = GroupOutputs(wallet, available_coins, coin_selection_params, ordered_filters); + // Walk-through the filters until the solution gets found. // If no solution is found, return the first detailed error (if any). // future: add "error level" so the worst one can be picked instead. std::vector<util::Result<SelectionResult>> res_detailed_errors; for (const auto& select_filter : ordered_filters) { - if (auto res{AttemptSelection(wallet, value_to_select, select_filter.filter, available_coins, + auto it = filtered_groups.find(select_filter.filter); + if (it == filtered_groups.end()) continue; + if (auto res{AttemptSelection(value_to_select, it->second, coin_selection_params, select_filter.allow_mixed_output_types)}) { return res; // result found } else { @@ -933,7 +963,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( } // Shuffle selected coins and fill in final vin - std::vector<COutput> selected_coins = result.GetShuffledInputVector(); + std::vector<std::shared_ptr<COutput>> selected_coins = result.GetShuffledInputVector(); // The sequence number is set to non-maxint so that DiscourageFeeSniping // works. @@ -945,7 +975,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( // behavior." const uint32_t nSequence{coin_control.m_signal_bip125_rbf.value_or(wallet.m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL}; for (const auto& coin : selected_coins) { - txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence)); + txNew.vin.push_back(CTxIn(coin->outpoint, CScript(), nSequence)); } DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight()); diff --git a/src/wallet/spend.h b/src/wallet/spend.h index ad122b7ef3..b8bc82db7a 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -106,17 +106,27 @@ const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint& */ std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); -std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<COutput>& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only); +struct SelectionFilter { + CoinEligibilityFilter filter; + bool allow_mixed_output_types{true}; +}; + +/** +* Group coins by the provided filters. +*/ +FilteredOutputGroups GroupOutputs(const CWallet& wallet, + const CoinsResult& coins, + const CoinSelectionParams& coin_sel_params, + const std::vector<SelectionFilter>& filters); + /** * Attempt to find a valid input set that preserves privacy by not mixing OutputTypes. * `ChooseSelectionResult()` will be called on each OutputType individually and the best * the solution (according to the waste metric) will be chosen. If a valid input cannot be found from any * single OutputType, fallback to running `ChooseSelectionResult()` over all available coins. * - * param@[in] wallet The wallet which provides solving data for the coins * param@[in] nTargetValue The target value - * param@[in] eligilibity_filter A filter containing rules for which coins are allowed to be included in this selection - * param@[in] available_coins The struct of coins, organized by OutputType, available for selection prior to filtering + * param@[in] groups The grouped outputs mapped by coin eligibility filters * param@[in] coin_selection_params Parameters for the coin selection * param@[in] allow_mixed_output_types Relax restriction that SelectionResults must be of the same OutputType * returns If successful, a SelectionResult containing the input set @@ -124,7 +134,7 @@ std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<C * or (2) an specific error message if there was something particularly wrong (e.g. a selection * result that surpassed the tx max weight size). */ -util::Result<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, +util::Result<SelectionResult> AttemptSelection(const CAmount& nTargetValue, OutputGroupTypeMap& groups, const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types); /** @@ -132,23 +142,20 @@ util::Result<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmo * Multiple coin selection algorithms will be run and the input set that produces the least waste * (according to the waste metric) will be chosen. * - * param@[in] wallet The wallet which provides solving data for the coins * param@[in] nTargetValue The target value - * param@[in] eligilibity_filter A filter containing rules for which coins are allowed to be included in this selection - * param@[in] available_coins The struct of coins, organized by OutputType, available for selection prior to filtering + * param@[in] groups The struct containing the outputs grouped by script and divided by (1) positive only outputs and (2) all outputs (positive + negative). * param@[in] coin_selection_params Parameters for the coin selection * returns If successful, a SelectionResult containing the input set * If failed, returns (1) an empty error message if the target was not reached (general "Insufficient funds") * or (2) an specific error message if there was something particularly wrong (e.g. a selection * result that surpassed the tx max weight size). */ -util::Result<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins, - const CoinSelectionParams& coin_selection_params); +util::Result<SelectionResult> ChooseSelectionResult(const CAmount& nTargetValue, Groups& groups, const CoinSelectionParams& coin_selection_params); // User manually selected inputs that must be part of the transaction struct PreSelectedInputs { - std::set<COutput> coins; + std::set<std::shared_ptr<COutput>> coins; // If subtract fee from outputs is disabled, the 'total_amount' // will be the sum of each output effective value // instead of the sum of the outputs amount @@ -161,7 +168,7 @@ struct PreSelectedInputs } else { total_amount += output.GetEffectiveValue(); } - coins.insert(output); + coins.insert(std::make_shared<COutput>(output)); } }; diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index 4af49db609..8d8a7ab2a2 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -23,9 +23,6 @@ namespace wallet { static constexpr int32_t WALLET_SCHEMA_VERSION = 0; -static GlobalMutex g_sqlite_mutex; -static int g_sqlite_count GUARDED_BY(g_sqlite_mutex) = 0; - static void ErrorLogCallback(void* arg, int code, const char* msg) { // From sqlite3_config() documentation for the SQLITE_CONFIG_LOG option: @@ -83,6 +80,9 @@ static void SetPragma(sqlite3* db, const std::string& key, const std::string& va } } +Mutex SQLiteDatabase::g_sqlite_mutex; +int SQLiteDatabase::g_sqlite_count = 0; + SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock) : WalletDatabase(), m_mock(mock), m_dir_path(fs::PathToString(dir_path)), m_file_path(fs::PathToString(file_path)), m_use_unsafe_sync(options.use_unsafe_sync) { @@ -145,6 +145,8 @@ SQLiteDatabase::~SQLiteDatabase() void SQLiteDatabase::Cleanup() noexcept { + AssertLockNotHeld(g_sqlite_mutex); + Close(); LOCK(g_sqlite_mutex); diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h index c6745d7a7e..5745a1d4cf 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_WALLET_SQLITE_H #define BITCOIN_WALLET_SQLITE_H +#include <sync.h> #include <wallet/db.h> #include <sqlite3.h> @@ -69,7 +70,16 @@ private: const std::string m_file_path; - void Cleanup() noexcept; + /** + * This mutex protects SQLite initialization and shutdown. + * sqlite3_config() and sqlite3_shutdown() are not thread-safe (sqlite3_initialize() is). + * Concurrent threads that execute SQLiteDatabase::SQLiteDatabase() should have just one + * of them do the init and the rest wait for it to complete before all can proceed. + */ + static Mutex g_sqlite_mutex; + static int g_sqlite_count GUARDED_BY(g_sqlite_mutex); + + void Cleanup() noexcept EXCLUSIVE_LOCKS_REQUIRED(!g_sqlite_mutex); public: SQLiteDatabase() = delete; diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 1c731b95e5..8f626addde 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -29,7 +29,7 @@ BOOST_FIXTURE_TEST_SUITE(coinselector_tests, WalletTestingSetup) // we repeat those tests this many times and only complain if all iterations of the test fail #define RANDOM_REPEATS 5 -typedef std::set<COutput> CoinSet; +typedef std::set<std::shared_ptr<COutput>> CoinSet; static const CoinEligibilityFilter filter_standard(1, 6, 0); static const CoinEligibilityFilter filter_confirmed(1, 1, 0); @@ -53,7 +53,7 @@ static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result) tx.nLockTime = nextLockTime++; // so all transactions get different hashes COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, /*fees=*/ 0); OutputGroup group; - group.Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ true); + group.Insert(std::make_shared<COutput>(output), /*ancestors=*/ 0, /*descendants=*/ 0); result.AddInput(group); } @@ -65,7 +65,7 @@ static void add_coin(const CAmount& nValue, int nInput, CoinSet& set, CAmount fe tx.nLockTime = nextLockTime++; // so all transactions get different hashes COutput coin(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ 148, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, fee); coin.long_term_fee = long_term_fee; - set.insert(coin); + set.insert(std::make_shared<COutput>(coin)); } static void add_coin(CoinsResult& available_coins, CWallet& wallet, const CAmount& nValue, CFeeRate feerate = CFeeRate(0), int nAge = 6*24, bool fIsFromMe = false, int nInput =0, bool spendable = false) @@ -94,10 +94,10 @@ static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b) std::vector<CAmount> a_amts; std::vector<CAmount> b_amts; for (const auto& coin : a.GetInputSet()) { - a_amts.push_back(coin.txout.nValue); + a_amts.push_back(coin->txout.nValue); } for (const auto& coin : b.GetInputSet()) { - b_amts.push_back(coin.txout.nValue); + b_amts.push_back(coin->txout.nValue); } std::sort(a_amts.begin(), a_amts.end()); std::sort(b_amts.begin(), b_amts.end()); @@ -110,8 +110,8 @@ static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b) static bool EqualResult(const SelectionResult& a, const SelectionResult& b) { std::pair<CoinSet::iterator, CoinSet::iterator> ret = std::mismatch(a.GetInputSet().begin(), a.GetInputSet().end(), b.GetInputSet().begin(), - [](const COutput& a, const COutput& b) { - return a.outpoint == b.outpoint; + [](const std::shared_ptr<COutput>& a, const std::shared_ptr<COutput>& b) { + return a->outpoint == b->outpoint; }); return ret.first == a.GetInputSet().end() && ret.second == b.GetInputSet().end(); } @@ -134,12 +134,12 @@ inline std::vector<OutputGroup>& GroupCoins(const std::vector<COutput>& availabl static_groups.clear(); for (auto& coin : available_coins) { static_groups.emplace_back(); - static_groups.back().Insert(coin, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false); + static_groups.back().Insert(std::make_shared<COutput>(coin), /*ancestors=*/ 0, /*descendants=*/ 0); } return static_groups; } -inline std::vector<OutputGroup>& KnapsackGroupOutputs(const std::vector<COutput>& available_coins, CWallet& wallet, const CoinEligibilityFilter& filter) +inline std::vector<OutputGroup>& KnapsackGroupOutputs(const CoinsResult& available_coins, CWallet& wallet, const CoinEligibilityFilter& filter) { FastRandomContext rand{}; CoinSelectionParams coin_selection_params{ @@ -153,9 +153,9 @@ inline std::vector<OutputGroup>& KnapsackGroupOutputs(const std::vector<COutput> /*tx_noinputs_size=*/ 0, /*avoid_partial=*/ false, }; - static std::vector<OutputGroup> static_groups; - static_groups = GroupOutputs(wallet, available_coins, coin_selection_params, filter, /*positive_only=*/false); - return static_groups; + static OutputGroupTypeMap static_groups; + static_groups = GroupOutputs(wallet, available_coins, coin_selection_params, {{filter}})[filter]; + return static_groups.all_groups.mixed_group; } // Branch and bound coin selection tests @@ -418,25 +418,25 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) available_coins.Clear(); // with an empty wallet we can't even pay one cent - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 1 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 1 * CENT, CENT)); add_coin(available_coins, *wallet, 1*CENT, CFeeRate(0), 4); // add a new 1 cent coin // with a new 1 cent coin, we still can't find a mature 1 cent - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 1 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 1 * CENT, CENT)); // but we can find a new 1 cent - const auto result1 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result1 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result1); BOOST_CHECK_EQUAL(result1->GetSelectedValue(), 1 * CENT); add_coin(available_coins, *wallet, 2*CENT); // add a mature 2 cent coin // we can't make 3 cents of mature coins - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 3 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 3 * CENT, CENT)); // we can make 3 cents of new coins - const auto result2 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 3 * CENT, CENT); + const auto result2 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 3 * CENT, CENT); BOOST_CHECK(result2); BOOST_CHECK_EQUAL(result2->GetSelectedValue(), 3 * CENT); @@ -447,38 +447,38 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // now we have new: 1+10=11 (of which 10 was self-sent), and mature: 2+5+20=27. total = 38 // we can't make 38 cents only if we disallow new coins: - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 38 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 38 * CENT, CENT)); // we can't even make 37 cents if we don't allow new coins even if they're from us - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard_extra), 38 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard_extra), 38 * CENT, CENT)); // but we can make 37 cents if we accept new coins from ourself - const auto result3 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 37 * CENT, CENT); + const auto result3 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 37 * CENT, CENT); BOOST_CHECK(result3); BOOST_CHECK_EQUAL(result3->GetSelectedValue(), 37 * CENT); // and we can make 38 cents if we accept all new coins - const auto result4 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 38 * CENT, CENT); + const auto result4 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 38 * CENT, CENT); BOOST_CHECK(result4); BOOST_CHECK_EQUAL(result4->GetSelectedValue(), 38 * CENT); // try making 34 cents from 1,2,5,10,20 - we can't do it exactly - const auto result5 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 34 * CENT, CENT); + const auto result5 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 34 * CENT, CENT); BOOST_CHECK(result5); BOOST_CHECK_EQUAL(result5->GetSelectedValue(), 35 * CENT); // but 35 cents is closest BOOST_CHECK_EQUAL(result5->GetInputSet().size(), 3U); // the best should be 20+10+5. it's incredibly unlikely the 1 or 2 got included (but possible) // when we try making 7 cents, the smaller coins (1,2,5) are enough. We should see just 2+5 - const auto result6 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 7 * CENT, CENT); + const auto result6 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 7 * CENT, CENT); BOOST_CHECK(result6); BOOST_CHECK_EQUAL(result6->GetSelectedValue(), 7 * CENT); BOOST_CHECK_EQUAL(result6->GetInputSet().size(), 2U); // when we try making 8 cents, the smaller coins (1,2,5) are exactly enough. - const auto result7 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 8 * CENT, CENT); + const auto result7 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 8 * CENT, CENT); BOOST_CHECK(result7); BOOST_CHECK(result7->GetSelectedValue() == 8 * CENT); BOOST_CHECK_EQUAL(result7->GetInputSet().size(), 3U); // when we try making 9 cents, no subset of smaller coins is enough, and we get the next bigger coin (10) - const auto result8 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 9 * CENT, CENT); + const auto result8 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 9 * CENT, CENT); BOOST_CHECK(result8); BOOST_CHECK_EQUAL(result8->GetSelectedValue(), 10 * CENT); BOOST_CHECK_EQUAL(result8->GetInputSet().size(), 1U); @@ -493,12 +493,12 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 30*CENT); // now we have 6+7+8+20+30 = 71 cents total // check that we have 71 and not 72 - const auto result9 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 71 * CENT, CENT); + const auto result9 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 71 * CENT, CENT); BOOST_CHECK(result9); - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 72 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 72 * CENT, CENT)); // now try making 16 cents. the best smaller coins can do is 6+7+8 = 21; not as good at the next biggest coin, 20 - const auto result10 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 16 * CENT, CENT); + const auto result10 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 16 * CENT, CENT); BOOST_CHECK(result10); BOOST_CHECK_EQUAL(result10->GetSelectedValue(), 20 * CENT); // we should get 20 in one coin BOOST_CHECK_EQUAL(result10->GetInputSet().size(), 1U); @@ -506,7 +506,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 5*CENT); // now we have 5+6+7+8+20+30 = 75 cents total // now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, better than the next biggest coin, 20 - const auto result11 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 16 * CENT, CENT); + const auto result11 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 16 * CENT, CENT); BOOST_CHECK(result11); BOOST_CHECK_EQUAL(result11->GetSelectedValue(), 18 * CENT); // we should get 18 in 3 coins BOOST_CHECK_EQUAL(result11->GetInputSet().size(), 3U); @@ -514,13 +514,13 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 18*CENT); // now we have 5+6+7+8+18+20+30 // and now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, the same as the next biggest coin, 18 - const auto result12 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 16 * CENT, CENT); + const auto result12 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 16 * CENT, CENT); BOOST_CHECK(result12); BOOST_CHECK_EQUAL(result12->GetSelectedValue(), 18 * CENT); // we should get 18 in 1 coin BOOST_CHECK_EQUAL(result12->GetInputSet().size(), 1U); // because in the event of a tie, the biggest coin wins // now try making 11 cents. we should get 5+6 - const auto result13 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 11 * CENT, CENT); + const auto result13 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 11 * CENT, CENT); BOOST_CHECK(result13); BOOST_CHECK_EQUAL(result13->GetSelectedValue(), 11 * CENT); BOOST_CHECK_EQUAL(result13->GetInputSet().size(), 2U); @@ -530,12 +530,12 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 2*COIN); add_coin(available_coins, *wallet, 3*COIN); add_coin(available_coins, *wallet, 4*COIN); // now we have 5+6+7+8+18+20+30+100+200+300+400 = 1094 cents - const auto result14 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 95 * CENT, CENT); + const auto result14 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 95 * CENT, CENT); BOOST_CHECK(result14); BOOST_CHECK_EQUAL(result14->GetSelectedValue(), 1 * COIN); // we should get 1 BTC in 1 coin BOOST_CHECK_EQUAL(result14->GetInputSet().size(), 1U); - const auto result15 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 195 * CENT, CENT); + const auto result15 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 195 * CENT, CENT); BOOST_CHECK(result15); BOOST_CHECK_EQUAL(result15->GetSelectedValue(), 2 * COIN); // we should get 2 BTC in 1 coin BOOST_CHECK_EQUAL(result15->GetInputSet().size(), 1U); @@ -551,7 +551,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // try making 1 * CENT from the 1.5 * CENT // we'll get change smaller than CENT whatever happens, so can expect CENT exactly - const auto result16 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT, CENT); + const auto result16 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), CENT, CENT); BOOST_CHECK(result16); BOOST_CHECK_EQUAL(result16->GetSelectedValue(), CENT); @@ -559,7 +559,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 1111*CENT); // try making 1 from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 + 1111 = 1112.5 - const auto result17 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result17 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result17); BOOST_CHECK_EQUAL(result17->GetSelectedValue(), 1 * CENT); // we should get the exact amount @@ -568,7 +568,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 7 / 10); // and try again to make 1.0 * CENT - const auto result18 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result18 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result18); BOOST_CHECK_EQUAL(result18->GetSelectedValue(), 1 * CENT); // we should get the exact amount @@ -578,7 +578,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int j = 0; j < 20; j++) add_coin(available_coins, *wallet, 50000 * COIN); - const auto result19 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 500000 * COIN, CENT); + const auto result19 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 500000 * COIN, CENT); BOOST_CHECK(result19); BOOST_CHECK_EQUAL(result19->GetSelectedValue(), 500000 * COIN); // we should get the exact amount BOOST_CHECK_EQUAL(result19->GetInputSet().size(), 10U); // in ten coins @@ -592,7 +592,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 6 / 10); add_coin(available_coins, *wallet, CENT * 7 / 10); add_coin(available_coins, *wallet, 1111 * CENT); - const auto result20 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result20 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result20); BOOST_CHECK_EQUAL(result20->GetSelectedValue(), 1111 * CENT); // we get the bigger coin BOOST_CHECK_EQUAL(result20->GetInputSet().size(), 1U); @@ -603,7 +603,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 6 / 10); add_coin(available_coins, *wallet, CENT * 8 / 10); add_coin(available_coins, *wallet, 1111 * CENT); - const auto result21 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT, CENT); + const auto result21 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), CENT, CENT); BOOST_CHECK(result21); BOOST_CHECK_EQUAL(result21->GetSelectedValue(), CENT); // we should get the exact amount BOOST_CHECK_EQUAL(result21->GetInputSet().size(), 2U); // in two coins 0.4+0.6 @@ -615,13 +615,13 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 100); // trying to make 100.01 from these three coins - const auto result22 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT * 10001 / 100, CENT); + const auto result22 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), CENT * 10001 / 100, CENT); BOOST_CHECK(result22); BOOST_CHECK_EQUAL(result22->GetSelectedValue(), CENT * 10105 / 100); // we should get all coins BOOST_CHECK_EQUAL(result22->GetInputSet().size(), 3U); // but if we try to make 99.9, we should take the bigger of the two small coins to avoid small change - const auto result23 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT * 9990 / 100, CENT); + const auto result23 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), CENT * 9990 / 100, CENT); BOOST_CHECK(result23); BOOST_CHECK_EQUAL(result23->GetSelectedValue(), 101 * CENT); BOOST_CHECK_EQUAL(result23->GetInputSet().size(), 2U); @@ -636,7 +636,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // We only create the wallet once to save time, but we still run the coin selection RUN_TESTS times. for (int i = 0; i < RUN_TESTS; i++) { - const auto result24 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 2000, CENT); + const auto result24 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 2000, CENT); BOOST_CHECK(result24); if (amt - 2000 < CENT) { @@ -727,7 +727,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) add_coin(available_coins, *wallet, 1000 * COIN); add_coin(available_coins, *wallet, 3 * COIN); - const auto result = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 1003 * COIN, CENT, rand); + const auto result = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 1003 * COIN, CENT, rand); BOOST_CHECK(result); BOOST_CHECK_EQUAL(result->GetSelectedValue(), 1003 * COIN); BOOST_CHECK_EQUAL(result->GetInputSet().size(), 2U); @@ -943,7 +943,7 @@ static util::Result<SelectionResult> select_coins(const CAmount& target, const C static bool has_coin(const CoinSet& set, CAmount amount) { - return std::any_of(set.begin(), set.end(), [&](const auto& coin) { return coin.GetEffectiveValue() == amount; }); + return std::any_of(set.begin(), set.end(), [&](const auto& coin) { return coin->GetEffectiveValue() == amount; }); } BOOST_AUTO_TEST_CASE(check_max_weight) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 1a0708c43a..304190eec1 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -29,8 +29,10 @@ static void GroupCoins(FuzzedDataProvider& fuzzed_data_provider, const std::vect auto output_group = OutputGroup(coin_params); bool valid_outputgroup{false}; for (auto& coin : coins) { - output_group.Insert(coin, /*ancestors=*/0, /*descendants=*/0, positive_only); - // If positive_only was specified, nothing may have been inserted, leading to an empty output group + if (!positive_only || (positive_only && coin.GetEffectiveValue() > 0)) { + output_group.Insert(std::make_shared<COutput>(coin), /*ancestors=*/0, /*descendants=*/0); + } + // If positive_only was specified, nothing was inserted, leading to an empty output group // that would be invalid for the BnB algorithm valid_outputgroup = !positive_only || output_group.GetSelectionAmount() > 0; if (valid_outputgroup && fuzzed_data_provider.ConsumeBool()) { diff --git a/src/wallet/test/group_outputs_tests.cpp b/src/wallet/test/group_outputs_tests.cpp new file mode 100644 index 0000000000..283e87989c --- /dev/null +++ b/src/wallet/test/group_outputs_tests.cpp @@ -0,0 +1,234 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#include <test/util/setup_common.h> + +#include <wallet/coinselection.h> +#include <wallet/spend.h> +#include <wallet/wallet.h> + +#include <boost/test/unit_test.hpp> + +namespace wallet { +BOOST_FIXTURE_TEST_SUITE(group_outputs_tests, TestingSetup) + +static int nextLockTime = 0; + +static std::shared_ptr<CWallet> NewWallet(const node::NodeContext& m_node) +{ + std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), "", CreateMockWalletDatabase()); + wallet->LoadWallet(); + LOCK(wallet->cs_wallet); + wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + wallet->SetupDescriptorScriptPubKeyMans(); + return wallet; +} + +static void addCoin(CoinsResult& coins, + CWallet& wallet, + const CTxDestination& dest, + const CAmount& nValue, + bool is_from_me, + CFeeRate fee_rate = CFeeRate(0), + int depth = 6) +{ + CMutableTransaction tx; + tx.nLockTime = nextLockTime++; // so all transactions get different hashes + tx.vout.resize(1); + tx.vout[0].nValue = nValue; + tx.vout[0].scriptPubKey = GetScriptForDestination(dest); + + const uint256& txid = tx.GetHash(); + LOCK(wallet.cs_wallet); + auto ret = wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateInactive{})); + assert(ret.second); + CWalletTx& wtx = (*ret.first).second; + const auto& txout = wtx.tx->vout.at(0); + coins.Add(*Assert(OutputTypeFromDestination(dest)), + {COutPoint(wtx.GetHash(), 0), + txout, + depth, + CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), + /*spendable=*/ true, + /*solvable=*/ true, + /*safe=*/ true, + wtx.GetTxTime(), + is_from_me, + fee_rate}); +} + + CoinSelectionParams makeSelectionParams(FastRandomContext& rand, bool avoid_partial_spends) +{ + return CoinSelectionParams{ + rand, + /*change_output_size=*/ 0, + /*change_spend_size=*/ 0, + /*min_change_target=*/ CENT, + /*effective_feerate=*/ CFeeRate(0), + /*long_term_feerate=*/ CFeeRate(0), + /*discard_feerate=*/ CFeeRate(0), + /*tx_noinputs_size=*/ 0, + /*avoid_partial=*/ avoid_partial_spends, + }; +} + +class GroupVerifier +{ +public: + std::shared_ptr<CWallet> wallet{nullptr}; + CoinsResult coins_pool; + FastRandomContext rand; + + void GroupVerify(const OutputType type, + const CoinEligibilityFilter& filter, + bool avoid_partial_spends, + bool positive_only, + int expected_size) + { + OutputGroupTypeMap groups = GroupOutputs(*wallet, coins_pool, makeSelectionParams(rand, avoid_partial_spends), {{filter}})[filter]; + std::vector<OutputGroup>& groups_out = positive_only ? groups.groups_by_type[type].positive_group : + groups.groups_by_type[type].mixed_group; + BOOST_CHECK_EQUAL(groups_out.size(), expected_size); + } + + void GroupAndVerify(const OutputType type, + const CoinEligibilityFilter& filter, + int expected_with_partial_spends_size, + int expected_without_partial_spends_size, + bool positive_only) + { + // First avoid partial spends + GroupVerify(type, filter, /*avoid_partial_spends=*/false, positive_only, expected_with_partial_spends_size); + // Second don't avoid partial spends + GroupVerify(type, filter, /*avoid_partial_spends=*/true, positive_only, expected_without_partial_spends_size); + } +}; + +BOOST_AUTO_TEST_CASE(outputs_grouping_tests) +{ + const auto& wallet = NewWallet(m_node); + GroupVerifier group_verifier; + group_verifier.wallet = wallet; + + const CoinEligibilityFilter& BASIC_FILTER{1, 6, 0}; + + // ################################################################################# + // 10 outputs from different txs going to the same script + // 1) if partial spends is enabled --> must not be grouped + // 2) if partial spends is not enabled --> must be grouped into a single OutputGroup + // ################################################################################# + + unsigned long GROUP_SIZE = 10; + const CTxDestination dest = *Assert(wallet->GetNewDestination(OutputType::BECH32, "")); + for (unsigned long i = 0; i < GROUP_SIZE; i++) { + addCoin(group_verifier.coins_pool, *wallet, dest, 10 * COIN, /*is_from_me=*/true); + } + + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE, + /*expected_without_partial_spends_size=*/ 1, + /*positive_only=*/ true); + + // #################################################################################### + // 3) 10 more UTXO are added with a different script --> must be grouped into a single + // group for avoid partial spends and 10 different output groups for partial spends + // #################################################################################### + + const CTxDestination dest2 = *Assert(wallet->GetNewDestination(OutputType::BECH32, "")); + for (unsigned long i = 0; i < GROUP_SIZE; i++) { + addCoin(group_verifier.coins_pool, *wallet, dest2, 5 * COIN, /*is_from_me=*/true); + } + + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2, + /*expected_without_partial_spends_size=*/ 2, + /*positive_only=*/ true); + + // ################################################################################ + // 4) Now add a negative output --> which will be skipped if "positive_only" is set + // ################################################################################ + + const CTxDestination dest3 = *Assert(wallet->GetNewDestination(OutputType::BECH32, "")); + addCoin(group_verifier.coins_pool, *wallet, dest3, 1, true, CFeeRate(100)); + BOOST_CHECK(group_verifier.coins_pool.coins[OutputType::BECH32].back().GetEffectiveValue() <= 0); + + // First expect no changes with "positive_only" enabled + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2, + /*expected_without_partial_spends_size=*/ 2, + /*positive_only=*/ true); + + // Then expect changes with "positive_only" disabled + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1, + /*expected_without_partial_spends_size=*/ 3, + /*positive_only=*/ false); + + + // ############################################################################## + // 5) Try to add a non-eligible UTXO (due not fulfilling the min depth target for + // "not mine" UTXOs) --> it must not be added to any group + // ############################################################################## + + const CTxDestination dest4 = *Assert(wallet->GetNewDestination(OutputType::BECH32, "")); + addCoin(group_verifier.coins_pool, *wallet, dest4, 6 * COIN, + /*is_from_me=*/false, CFeeRate(0), /*depth=*/5); + + // Expect no changes from this round and the previous one (point 4) + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1, + /*expected_without_partial_spends_size=*/ 3, + /*positive_only=*/ false); + + + // ############################################################################## + // 6) Try to add a non-eligible UTXO (due not fulfilling the min depth target for + // "mine" UTXOs) --> it must not be added to any group + // ############################################################################## + + const CTxDestination dest5 = *Assert(wallet->GetNewDestination(OutputType::BECH32, "")); + addCoin(group_verifier.coins_pool, *wallet, dest5, 6 * COIN, + /*is_from_me=*/true, CFeeRate(0), /*depth=*/0); + + // Expect no changes from this round and the previous one (point 5) + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1, + /*expected_without_partial_spends_size=*/ 3, + /*positive_only=*/ false); + + // ########################################################################################### + // 7) Surpass the OUTPUT_GROUP_MAX_ENTRIES and verify that a second partial group gets created + // ########################################################################################### + + const CTxDestination dest7 = *Assert(wallet->GetNewDestination(OutputType::BECH32, "")); + uint16_t NUM_SINGLE_ENTRIES = 101; + for (unsigned long i = 0; i < NUM_SINGLE_ENTRIES; i++) { // OUTPUT_GROUP_MAX_ENTRIES{100} + addCoin(group_verifier.coins_pool, *wallet, dest7, 9 * COIN, /*is_from_me=*/true); + } + + // Exclude partial groups only adds one more group to the previous test case (point 6) + int PREVIOUS_ROUND_COUNT = GROUP_SIZE * 2 + 1; + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, + /*expected_with_partial_spends_size=*/ PREVIOUS_ROUND_COUNT + NUM_SINGLE_ENTRIES, + /*expected_without_partial_spends_size=*/ 4, + /*positive_only=*/ false); + + // Include partial groups should add one more group inside the "avoid partial spends" count + const CoinEligibilityFilter& avoid_partial_groups_filter{1, 6, 0, 0, /*include_partial=*/ true}; + group_verifier.GroupAndVerify(OutputType::BECH32, + avoid_partial_groups_filter, + /*expected_with_partial_spends_size=*/ PREVIOUS_ROUND_COUNT + NUM_SINGLE_ENTRIES, + /*expected_without_partial_spends_size=*/ 5, + /*positive_only=*/ false); +} + +BOOST_AUTO_TEST_SUITE_END() +} // end namespace wallet diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e5c03849af..61851ce4ba 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1648,8 +1648,8 @@ bool DummySignInput(const SigningProvider& provider, CTxIn &tx_in, const CTxOut const CScript& scriptPubKey = txout.scriptPubKey; SignatureData sigdata; - // Use max sig if watch only inputs were used or if this particular input is an external input - // to ensure a sufficient fee is attained for the requested feerate. + // Use max sig if watch only inputs were used, if this particular input is an external input, + // or if this wallet uses an external signer, to ensure a sufficient fee is attained for the requested feerate. const bool use_max_sig = coin_control && (coin_control->fAllowWatchOnly || coin_control->IsExternalSelected(tx_in.prevout) || !can_grind_r); if (!ProduceSignature(provider, use_max_sig ? DUMMY_MAXIMUM_SIGNATURE_CREATOR : DUMMY_SIGNATURE_CREATOR, scriptPubKey, sigdata)) { return false; diff --git a/test/functional/feature_abortnode.py b/test/functional/feature_abortnode.py index 32cf4a47f4..fa1bb6506a 100755 --- a/test/functional/feature_abortnode.py +++ b/test/functional/feature_abortnode.py @@ -19,7 +19,6 @@ class AbortNodeTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 2 - self.rpc_timeout = 240 def setup_network(self): self.setup_nodes() @@ -41,7 +40,7 @@ class AbortNodeTest(BitcoinTestFramework): # Check that node0 aborted self.log.info("Waiting for crash") - self.nodes[0].wait_until_stopped(timeout=200) + self.nodes[0].wait_until_stopped(timeout=5) self.log.info("Node crashed - now verifying restart fails") self.nodes[0].assert_start_raises_init_error() diff --git a/test/functional/mining_getblocktemplate_longpoll.py b/test/functional/mining_getblocktemplate_longpoll.py index ec492f9e72..53182eb79e 100755 --- a/test/functional/mining_getblocktemplate_longpoll.py +++ b/test/functional/mining_getblocktemplate_longpoll.py @@ -4,7 +4,6 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test longpolling with getblocktemplate.""" -from decimal import Decimal import random import threading @@ -47,9 +46,9 @@ class GetBlockTemplateLPTest(BitcoinTestFramework): thr.join(5) # wait 5 seconds or until thread exits assert thr.is_alive() - miniwallets = [MiniWallet(node) for node in self.nodes] + self.miniwallet = MiniWallet(self.nodes[0]) self.log.info("Test that longpoll will terminate if another node generates a block") - self.generate(miniwallets[1], 1) # generate a block on another node + self.generate(self.nodes[1], 1) # generate a block on another node # check that thread will exit now that new transaction entered mempool thr.join(5) # wait 5 seconds or until thread exits assert not thr.is_alive() @@ -57,18 +56,15 @@ class GetBlockTemplateLPTest(BitcoinTestFramework): self.log.info("Test that longpoll will terminate if we generate a block ourselves") thr = LongpollThread(self.nodes[0]) thr.start() - self.generate(miniwallets[0], 1) # generate a block on own node + self.generate(self.nodes[0], 1) # generate a block on own node thr.join(5) # wait 5 seconds or until thread exits assert not thr.is_alive() self.log.info("Test that introducing a new transaction into the mempool will terminate the longpoll") thr = LongpollThread(self.nodes[0]) thr.start() - # generate a random transaction and submit it - min_relay_fee = self.nodes[0].getnetworkinfo()["relayfee"] - fee_rate = min_relay_fee + Decimal('0.00000010') * random.randint(0,20) - miniwallets[0].send_self_transfer(from_node=random.choice(self.nodes), - fee_rate=fee_rate) + # generate a transaction and submit it + self.miniwallet.send_self_transfer(from_node=random.choice(self.nodes)) # after one minute, every 10 seconds the mempool is probed, so in 80 seconds it should have returned thr.join(60 + 20) assert not thr.is_alive() diff --git a/test/functional/wallet_listdescriptors.py b/test/functional/wallet_listdescriptors.py index fb2156bda1..c5479089c6 100755 --- a/test/functional/wallet_listdescriptors.py +++ b/test/functional/wallet_listdescriptors.py @@ -54,7 +54,7 @@ class ListDescriptorsTest(BitcoinTestFramework): assert_equal(4, len([d for d in result['descriptors'] if d['internal']])) for item in result['descriptors']: assert item['desc'] != '' - assert item['next'] == 0 + assert item['next_index'] == 0 assert item['range'] == [0, 0] assert item['timestamp'] is not None @@ -78,7 +78,8 @@ class ListDescriptorsTest(BitcoinTestFramework): 'timestamp': TIME_GENESIS_BLOCK, 'active': False, 'range': [0, 0], - 'next': 0}, + 'next': 0, + 'next_index': 0}, ], } assert_equal(expected, wallet.listdescriptors()) @@ -92,7 +93,8 @@ class ListDescriptorsTest(BitcoinTestFramework): 'timestamp': TIME_GENESIS_BLOCK, 'active': False, 'range': [0, 0], - 'next': 0}, + 'next': 0, + 'next_index': 0}, ], } assert_equal(expected_private, wallet.listdescriptors(True)) diff --git a/test/lint/run-lint-format-strings.py b/test/lint/run-lint-format-strings.py index 57eefb00f2..91915f05f9 100755 --- a/test/lint/run-lint-format-strings.py +++ b/test/lint/run-lint-format-strings.py @@ -17,6 +17,7 @@ FALSE_POSITIVES = [ ("src/index/base.cpp", "FatalError(const char* fmt, const Args&... args)"), ("src/netbase.cpp", "LogConnectFailure(bool manual_connection, const char* fmt, const Args&... args)"), ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"), + ("src/test/translation_tests.cpp", "strprintf(format, arg)"), ("src/validationinterface.cpp", "LogPrint(BCLog::VALIDATION, fmt \"\\n\", __VA_ARGS__)"), ("src/wallet/wallet.h", "WalletLogPrintf(std::string fmt, Params... parameters)"), ("src/wallet/wallet.h", "LogPrintf((\"%s \" + fmt).c_str(), GetDisplayName(), parameters...)"), |