diff options
Diffstat (limited to 'src')
76 files changed, 838 insertions, 921 deletions
diff --git a/src/Makefile.am b/src/Makefile.am index 7673d2f545..c2e0c7b5b8 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -320,7 +320,7 @@ BITCOIN_CORE_H = \ util/sock.h \ util/spanparsing.h \ util/string.h \ - util/subprocess.hpp \ + util/subprocess.h \ util/syserror.h \ util/task_runner.h \ util/thread.h \ @@ -679,6 +679,7 @@ libbitcoin_common_a_SOURCES = \ common/run_command.cpp \ common/settings.cpp \ common/system.cpp \ + common/url.cpp \ compressor.cpp \ core_read.cpp \ core_write.cpp \ @@ -711,11 +712,6 @@ libbitcoin_common_a_SOURCES = \ script/solver.cpp \ warnings.cpp \ $(BITCOIN_CORE_H) - -if USE_LIBEVENT -libbitcoin_common_a_CPPFLAGS += $(EVENT_CFLAGS) -libbitcoin_common_a_SOURCES += common/url.cpp -endif # # util # diff --git a/src/Makefile.minisketch.include b/src/Makefile.minisketch.include index 1363bec34e..c6f894f0ca 100644 --- a/src/Makefile.minisketch.include +++ b/src/Makefile.minisketch.include @@ -12,10 +12,6 @@ LIBMINISKETCH_CPPFLAGS += -DHAVE_CLMUL MINISKETCH_LIBS += $(LIBMINISKETCH_CLMUL) endif -if HAVE_CLZ -LIBMINISKETCH_CPPFLAGS += -DHAVE_CLZ -endif - EXTRA_LIBRARIES += $(MINISKETCH_LIBS) minisketch_libminisketch_clmul_a_SOURCES = $(MINISKETCH_FIELD_CLMUL_SOURCES_INT) $(MINISKETCH_FIELD_CLMUL_HEADERS_INT) diff --git a/src/Makefile.test.include b/src/Makefile.test.include index cf88a02b95..942e0bf69b 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -85,6 +85,7 @@ BITCOIN_TESTS =\ test/checkqueue_tests.cpp \ test/coins_tests.cpp \ test/coinstatsindex_tests.cpp \ + test/common_url_tests.cpp \ test/compilerbug_tests.cpp \ test/compress_tests.cpp \ test/crypto_tests.cpp \ diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index 129deeec60..8901d10ef6 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -11,7 +11,6 @@ #include <clientversion.h> #include <common/args.h> #include <common/system.h> -#include <common/url.h> #include <compat/compat.h> #include <compat/stdin.h> #include <policy/feerate.h> @@ -51,7 +50,6 @@ using CliClock = std::chrono::system_clock; const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr; -UrlDecodeFn* const URL_DECODE = urlDecode; static const char DEFAULT_RPCCONNECT[] = "127.0.0.1"; static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900; @@ -551,7 +549,7 @@ public: peer.is_outbound ? "out" : "in", ConnectionTypeForNetinfo(peer.conn_type), peer.network, - peer.transport_protocol_type.starts_with('v') ? peer.transport_protocol_type[1] : ' ', + (peer.transport_protocol_type.size() == 2 && peer.transport_protocol_type[0] == 'v') ? peer.transport_protocol_type[1] : ' ', PingTimeToString(peer.min_ping), PingTimeToString(peer.ping), peer.last_send ? ToString(time_now - peer.last_send) : "", diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index d5dfbbec27..fe90958a5f 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -11,7 +11,6 @@ #include <clientversion.h> #include <common/args.h> #include <common/system.h> -#include <common/url.h> #include <compat/compat.h> #include <interfaces/init.h> #include <key.h> @@ -28,7 +27,6 @@ #include <tuple> const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr; -UrlDecodeFn* const URL_DECODE = nullptr; static void SetupWalletToolArgs(ArgsManager& argsman) { diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index 4f0a816388..54796c5abb 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -12,7 +12,6 @@ #include <common/args.h> #include <common/init.h> #include <common/system.h> -#include <common/url.h> #include <compat/compat.h> #include <init.h> #include <interfaces/chain.h> @@ -35,7 +34,6 @@ using node::NodeContext; const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr; -UrlDecodeFn* const URL_DECODE = urlDecode; #if HAVE_DECL_FORK diff --git a/src/common/run_command.cpp b/src/common/run_command.cpp index e5356490ef..347b486095 100644 --- a/src/common/run_command.cpp +++ b/src/common/run_command.cpp @@ -12,7 +12,7 @@ #include <univalue.h> #ifdef ENABLE_EXTERNAL_SIGNER -#include <util/subprocess.hpp> +#include <util/subprocess.h> #endif // ENABLE_EXTERNAL_SIGNER UniValue RunCommandParseJSON(const std::string& str_command, const std::string& str_std_in) diff --git a/src/common/url.cpp b/src/common/url.cpp index 053e1a825c..ecf88d07ea 100644 --- a/src/common/url.cpp +++ b/src/common/url.cpp @@ -4,19 +4,36 @@ #include <common/url.h> -#include <event2/http.h> - -#include <cstdlib> +#include <charconv> #include <string> +#include <string_view> +#include <system_error> -std::string urlDecode(const std::string &urlEncoded) { +std::string UrlDecode(std::string_view url_encoded) +{ std::string res; - if (!urlEncoded.empty()) { - char *decoded = evhttp_uridecode(urlEncoded.c_str(), false, nullptr); - if (decoded) { - res = std::string(decoded); - free(decoded); + res.reserve(url_encoded.size()); + + for (size_t i = 0; i < url_encoded.size(); ++i) { + char c = url_encoded[i]; + // Special handling for percent which should be followed by two hex digits + // representing an octet values, see RFC 3986, Section 2.1 Percent-Encoding + if (c == '%' && i + 2 < url_encoded.size()) { + unsigned int decoded_value{0}; + auto [p, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16); + + // Only if there is no error and the pointer is set to the end of + // the string, we can be sure both characters were valid hex + if (ec == std::errc{} && p == url_encoded.data() + i + 3) { + res += static_cast<char>(decoded_value); + // Next two characters are part of the percent encoding + i += 2; + continue; + } + // In case of invalid percent encoding, add the '%' and continue } + res += c; } + return res; } diff --git a/src/common/url.h b/src/common/url.h index b16b8241af..203f41c70f 100644 --- a/src/common/url.h +++ b/src/common/url.h @@ -6,9 +6,12 @@ #define BITCOIN_COMMON_URL_H #include <string> +#include <string_view> -using UrlDecodeFn = std::string(const std::string& url_encoded); -UrlDecodeFn urlDecode; -extern UrlDecodeFn* const URL_DECODE; +/* Decode a URL. + * + * Notably this implementation does not decode a '+' to a ' '. + */ +std::string UrlDecode(std::string_view url_encoded); #endif // BITCOIN_COMMON_URL_H diff --git a/src/core_read.cpp b/src/core_read.cpp index e32e46d1b9..5956d9df5f 100644 --- a/src/core_read.cpp +++ b/src/core_read.cpp @@ -256,6 +256,6 @@ util::Result<int> SighashFromStr(const std::string& sighash) if (it != map_sighash_values.end()) { return it->second; } else { - return util::Error{Untranslated(sighash + " is not a valid sighash parameter.")}; + return util::Error{Untranslated("'" + sighash + "' is not a valid sighash parameter.")}; } } diff --git a/src/index/base.cpp b/src/index/base.cpp index a203ce4a9f..e66c89f9e4 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -160,12 +160,24 @@ void BaseIndex::Sync() } const CBlockIndex* pindex_next = WITH_LOCK(cs_main, return NextSyncBlock(pindex, m_chainstate->m_chain)); + // If pindex_next is null, it means pindex is the chain tip, so + // commit data indexed so far. if (!pindex_next) { SetBestBlockIndex(pindex); // No need to handle errors in Commit. See rationale above. Commit(); - m_synced = true; - break; + + // If pindex is still the chain tip after committing, exit the + // sync loop. It is important for cs_main to be locked while + // setting m_synced = true, otherwise a new block could be + // attached while m_synced is still false, and it would not be + // indexed. + LOCK(::cs_main); + pindex_next = NextSyncBlock(pindex, m_chainstate->m_chain); + if (!pindex_next) { + m_synced = true; + break; + } } if (pindex_next->pprev != pindex && !Rewind(pindex, pindex_next->pprev)) { FatalErrorf("%s: Failed to rewind index %s to a previous chain tip", __func__, GetName()); diff --git a/src/init.cpp b/src/init.cpp index 885c0673dd..47d649be3d 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -553,16 +553,12 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control host and port to use if onion listening enabled (default: %s). If no port is specified, the default port of %i will be used.", DEFAULT_TOR_CONTROL, DEFAULT_TOR_CONTROL_PORT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-torpassword=<pass>", "Tor control port password (default: empty)", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::CONNECTION); #ifdef USE_UPNP -#if USE_UPNP - argsman.AddArg("-upnp", "Use UPnP to map the listening port (default: 1 when listening and no -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); -#else - argsman.AddArg("-upnp", strprintf("Use UPnP to map the listening port (default: %u)", 0), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); -#endif + argsman.AddArg("-upnp", strprintf("Use UPnP to map the listening port (default: %u)", DEFAULT_UPNP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); #else hidden_args.emplace_back("-upnp"); #endif #ifdef USE_NATPMP - argsman.AddArg("-natpmp", strprintf("Use NAT-PMP to map the listening port (default: %s)", DEFAULT_NATPMP ? "1 when listening and no -proxy" : "0"), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); + argsman.AddArg("-natpmp", strprintf("Use NAT-PMP to map the listening port (default: %u)", DEFAULT_NATPMP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); #else hidden_args.emplace_back("-natpmp"); #endif // USE_NATPMP @@ -996,10 +992,8 @@ bool AppInitParameterInteraction(const ArgsManager& args) InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections)); // ********************************************************* Step 3: parameter-to-internal-flags - auto result = init::SetLoggingCategories(args); - if (!result) return InitError(util::ErrorString(result)); - result = init::SetLoggingLevel(args); - if (!result) return InitError(util::ErrorString(result)); + if (auto result{init::SetLoggingCategories(args)}; !result) return InitError(util::ErrorString(result)); + if (auto result{init::SetLoggingLevel(args)}; !result) return InitError(util::ErrorString(result)); nConnectTimeout = args.GetIntArg("-timeout", DEFAULT_CONNECT_TIMEOUT); if (nConnectTimeout <= 0) { @@ -1305,30 +1299,33 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } } - for (const std::string port_option : { - "-i2psam", - "-onion", - "-proxy", - "-rpcbind", - "-torcontrol", - "-whitebind", - "-zmqpubhashblock", - "-zmqpubhashtx", - "-zmqpubrawblock", - "-zmqpubrawtx", - "-zmqpubsequence", + for (const auto &port_option : std::vector<std::pair<std::string, bool>>{ + // arg name UNIX socket support + {"-i2psam", false}, + {"-onion", true}, + {"-proxy", true}, + {"-rpcbind", false}, + {"-torcontrol", false}, + {"-whitebind", false}, + {"-zmqpubhashblock", true}, + {"-zmqpubhashtx", true}, + {"-zmqpubrawblock", true}, + {"-zmqpubrawtx", true}, + {"-zmqpubsequence", true} }) { - for (const std::string& socket_addr : args.GetArgs(port_option)) { + const std::string arg{port_option.first}; + const bool unix{port_option.second}; + for (const std::string& socket_addr : args.GetArgs(arg)) { std::string host_out; uint16_t port_out{0}; if (!SplitHostPort(socket_addr, port_out, host_out)) { #if HAVE_SOCKADDR_UN - // Allow unix domain sockets for -proxy and -onion e.g. unix:/some/file/path - if ((port_option != "-proxy" && port_option != "-onion") || socket_addr.find(ADDR_PREFIX_UNIX) != 0) { - return InitError(InvalidPortErrMsg(port_option, socket_addr)); + // Allow unix domain sockets for some options e.g. unix:/some/file/path + if (!unix || socket_addr.find(ADDR_PREFIX_UNIX) != 0) { + return InitError(InvalidPortErrMsg(arg, socket_addr)); } #else - return InitError(InvalidPortErrMsg(port_option, socket_addr)); + return InitError(InvalidPortErrMsg(arg, socket_addr)); #endif } } diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 6114236623..c41f35829d 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -127,7 +127,7 @@ public: virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0; //! Display address on external signer - virtual bool displayAddress(const CTxDestination& dest) = 0; + virtual util::Result<void> displayAddress(const CTxDestination& dest) = 0; //! Lock coin. virtual bool lockCoin(const COutPoint& output, const bool write_to_db) = 0; diff --git a/src/minisketch/.cirrus.yml b/src/minisketch/.cirrus.yml index 4a5353f137..5ceefee2cf 100644 --- a/src/minisketch/.cirrus.yml +++ b/src/minisketch/.cirrus.yml @@ -36,17 +36,6 @@ env_matrix_snippet: &ENV_MATRIX_VALGRIND TESTRUNS: 1 BUILD: -env_matrix_snippet: &ENV_MATRIX_SAN - - env: - ENABLE_FIELDS: 28 - - env: - BUILD: distcheck - - env: - CXXFLAGS: "-fsanitize=undefined -fno-omit-frame-pointer" - LDFLAGS: "-fsanitize=undefined -fno-omit-frame-pointer" - UBSAN_OPTIONS: "print_stacktrace=1:halt_on_error=1" - BENCH: no - env_matrix_snippet: &ENV_MATRIX_SAN_VALGRIND - env: ENABLE_FIELDS: "11,64,37" @@ -72,9 +61,9 @@ task: << : *ENV_MATRIX_SAN_VALGRIND matrix: - env: - CC: gcc + CXX: g++ - env: - CC: clang + CXX: clang++ -gdwarf-4 << : *MERGE_BASE test_script: - ./ci/cirrus.sh @@ -92,30 +81,45 @@ task: << : *ENV_MATRIX_VALGRIND matrix: - env: - CC: i686-linux-gnu-gcc + CXX: i686-linux-gnu-g++ - env: - CC: clang --target=i686-pc-linux-gnu -isystem /usr/i686-linux-gnu/include + CXX: clang++ --target=i686-linux-gnu -gdwarf-4 + CXXFLAGS: -g -O2 -isystem /usr/i686-linux-gnu/include -isystem /usr/i686-linux-gnu/include/c++/10/i686-linux-gnu test_script: - ./ci/cirrus.sh << : *CAT_LOGS task: - name: "x86_64: macOS Catalina" + name: "arm64: macOS Monterey" macos_instance: - image: catalina-base + image: ghcr.io/cirruslabs/macos-monterey-base:latest env: - # Cirrus gives us a fixed number of 12 virtual CPUs. - MAKEFLAGS: -j13 - matrix: - << : *ENV_MATRIX_SAN + # Cirrus gives us a fixed number of 4 virtual CPUs. + MAKEFLAGS: -j5 matrix: - env: - CC: gcc-9 + CXX: g++-11 + # Homebrew's gcc for arm64 has no libubsan. + matrix: + - env: + ENABLE_FIELDS: 28 + - env: + BUILD: distcheck - env: - CC: clang + CXX: clang++ + matrix: + - env: + ENABLE_FIELDS: 28 + - env: + BUILD: distcheck + - env: + CXXFLAGS: "-fsanitize=undefined -fno-omit-frame-pointer" + LDFLAGS: "-fsanitize=undefined -fno-omit-frame-pointer" + UBSAN_OPTIONS: "print_stacktrace=1:halt_on_error=1" + BENCH: no brew_script: - brew update - - brew install automake libtool gcc@9 + - brew install automake libtool gcc@11 << : *MERGE_BASE test_script: - ./ci/cirrus.sh @@ -128,13 +132,11 @@ task: cpu: 4 memory: 2G env: - EXEC_CMD: qemu-s390x -L /usr/s390x-linux-gnu + EXEC_CMD: qemu-s390x HOST: s390x-linux-gnu BUILD: << : *MERGE_BASE test_script: - # https://sourceware.org/bugzilla/show_bug.cgi?id=27008 - - rm /etc/ld.so.cache - ./ci/cirrus.sh << : *CAT_LOGS @@ -146,6 +148,7 @@ task: memory: 2G env: EXEC_CMD: wine + EXEC_EXT: .exe HOST: x86_64-w64-mingw32 BUILD: << : *MERGE_BASE diff --git a/src/minisketch/ci/cirrus.sh b/src/minisketch/ci/cirrus.sh index 02f737ca7f..36250d1651 100755 --- a/src/minisketch/ci/cirrus.sh +++ b/src/minisketch/ci/cirrus.sh @@ -7,7 +7,7 @@ export LC_ALL=C env >> test_env.log -$CC -v || true +$CXX -v || true valgrind --version || true ./autogen.sh @@ -32,10 +32,10 @@ then fi if [ -n "$EXEC_CMD" ]; then - $EXEC_CMD ./test $TESTRUNS - $EXEC_CMD ./test-verify $TESTRUNS + $EXEC_CMD "./test$EXEC_EXT" $TESTRUNS + $EXEC_CMD "./test-verify$EXEC_EXT" $TESTRUNS fi if [ "$BENCH" = "yes" ]; then - $EXEC_CMD ./bench + $EXEC_CMD "./bench$EXEC_EXT" fi diff --git a/src/minisketch/ci/linux-debian.Dockerfile b/src/minisketch/ci/linux-debian.Dockerfile index 63e5412ee7..122af36e1f 100644 --- a/src/minisketch/ci/linux-debian.Dockerfile +++ b/src/minisketch/ci/linux-debian.Dockerfile @@ -8,10 +8,10 @@ RUN apt-get update RUN apt-get install --no-install-recommends --no-upgrade -y \ git ca-certificates \ make automake libtool pkg-config dpkg-dev valgrind qemu-user \ - gcc g++ clang libc6-dbg \ + gcc g++ clang libclang-rt-dev libc6-dbg \ gcc-i686-linux-gnu g++-i686-linux-gnu libc6-dev-i386-cross libc6-dbg:i386 \ - g++-s390x-linux-gnu gcc-s390x-linux-gnu libc6-dev-s390x-cross libc6-dbg:s390x \ - wine g++-mingw-w64-x86-64 + g++-s390x-linux-gnu libstdc++6:s390x gcc-s390x-linux-gnu libc6-dev-s390x-cross libc6-dbg:s390x \ + wine wine64 g++-mingw-w64-x86-64 # Run a dummy command in wine to make it set up configuration RUN wine true || true diff --git a/src/minisketch/configure.ac b/src/minisketch/configure.ac index 83910448a2..cd52d7f412 100644 --- a/src/minisketch/configure.ac +++ b/src/minisketch/configure.ac @@ -104,11 +104,6 @@ esac AX_CHECK_COMPILE_FLAG([-Wall],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wall"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-fvisibility=hidden],[CXXFLAGS="$CXXFLAGS -fvisibility=hidden"],[],[$CXXFLAG_WERROR]) -## Some compilers (gcc) ignore unknown -Wno-* options, but warn about all -## unknown options if any other warning is produced. Test the -Wfoo case, and -## set the -Wno-foo case if it works. -AX_CHECK_COMPILE_FLAG([-Wshift-count-overflow],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-shift-count-overflow"],,[[$CXXFLAG_WERROR]]) - if test "x$use_ccache" != "xno"; then AC_MSG_CHECKING(if ccache should be used) if test x$CCACHE = x; then @@ -119,7 +114,6 @@ if test "x$use_ccache" != "xno"; then fi else use_ccache=yes - CC="$ac_cv_path_CCACHE $CC" CXX="$ac_cv_path_CCACHE $CXX" fi AC_MSG_RESULT($use_ccache) diff --git a/src/minisketch/src/int_utils.h b/src/minisketch/src/int_utils.h index d21ba56f33..2b3d8cb402 100644 --- a/src/minisketch/src/int_utils.h +++ b/src/minisketch/src/int_utils.h @@ -7,13 +7,16 @@ #ifndef _MINISKETCH_INT_UTILS_H_ #define _MINISKETCH_INT_UTILS_H_ +#include <stdint.h> #include <stdlib.h> #include <limits> #include <algorithm> #include <type_traits> -#ifdef _MSC_VER +#if defined(__cpp_lib_int_pow2) && __cpp_lib_int_pow2 >= 202002L +# include <bit> +#elif defined(_MSC_VER) # include <intrin.h> #endif @@ -54,11 +57,10 @@ class BitWriter { int offset = 0; unsigned char* out; -public: - BitWriter(unsigned char* output) : out(output) {} - template<int BITS, typename I> - inline void Write(I val) { + inline void WriteInner(I val) { + // We right shift by up to 8 bits below. Verify that's well defined for the type I. + static_assert(std::numeric_limits<I>::digits > 8, "BitWriter::WriteInner needs I > 8 bits"); int bits = BITS; if (bits + offset >= 8) { state |= ((val & ((I(1) << (8 - offset)) - 1)) << offset); @@ -77,6 +79,19 @@ public: offset += bits; } + +public: + BitWriter(unsigned char* output) : out(output) {} + + template<int BITS, typename I> + inline void Write(I val) { + // If I is smaller than an unsigned int, invoke WriteInner with argument converted to unsigned. + using compute_type = typename std::conditional< + (std::numeric_limits<I>::digits < std::numeric_limits<unsigned>::digits), + unsigned, I>::type; + return WriteInner<BITS, compute_type>(val); + } + inline void Flush() { if (offset) { *(out++) = state; @@ -129,7 +144,11 @@ constexpr inline I Mask() { return ((I((I(-1)) << (std::numeric_limits<I>::digit /** Compute the smallest power of two that is larger than val. */ template<typename I> static inline int CountBits(I val, int max) { -#ifdef _MSC_VER +#if defined(__cpp_lib_int_pow2) && __cpp_lib_int_pow2 >= 202002L + // c++20 impl + (void)max; + return std::bit_width(val); +#elif defined(_MSC_VER) (void)max; unsigned long index; unsigned char ret; @@ -175,6 +194,7 @@ public: } static constexpr inline bool IsZero(I a) { return a == 0; } + static constexpr inline bool IsOne(I a) { return a == 1; } static constexpr inline I Mask(I val) { return val & MASK; } static constexpr inline I Shift(I val, int bits) { return ((val << bits) & MASK); } static constexpr inline I UnsafeShift(I val, int bits) { return (val << bits); } @@ -233,7 +253,7 @@ template<typename I, int N, typename L, typename F> inline constexpr I GFMul(con template<typename I, typename F, int BITS, uint32_t MOD> inline I InvExtGCD(I x) { - if (F::IsZero(x)) return x; + if (F::IsZero(x) || F::IsOne(x)) return x; I t(0), newt(1); I r(MOD), newr = x; int rlen = BITS + 1, newrlen = F::Bits(newr, BITS); diff --git a/src/net.cpp b/src/net.cpp index e388f05b03..4801f5c1f9 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -412,6 +412,9 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo // Resolve const uint16_t default_port{pszDest != nullptr ? GetDefaultPort(pszDest) : m_params.GetDefaultPort()}; + + // Collection of addresses to try to connect to: either all dns resolved addresses if a domain name (pszDest) is provided, or addrConnect otherwise. + std::vector<CAddress> connect_to{}; if (pszDest) { std::vector<CService> resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)}; if (!resolved.empty()) { @@ -432,8 +435,16 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort()); return nullptr; } + // Add the address to the resolved addresses vector so we can try to connect to it later on + connect_to.push_back(addrConnect); } + } else { + // For resolution via proxy + connect_to.push_back(addrConnect); } + } else { + // Connect via addrConnect directly + connect_to.push_back(addrConnect); } // Connect @@ -443,94 +454,99 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo assert(!addr_bind.IsValid()); std::unique_ptr<i2p::sam::Session> i2p_transient_session; - if (addrConnect.IsValid()) { - const bool use_proxy{GetProxy(addrConnect.GetNetwork(), proxy)}; - bool proxyConnectionFailed = false; + for (auto& target_addr: connect_to) { + if (target_addr.IsValid()) { + const bool use_proxy{GetProxy(target_addr.GetNetwork(), proxy)}; + bool proxyConnectionFailed = false; - if (addrConnect.IsI2P() && use_proxy) { - i2p::Connection conn; - bool connected{false}; + if (target_addr.IsI2P() && use_proxy) { + i2p::Connection conn; + bool connected{false}; - if (m_i2p_sam_session) { - connected = m_i2p_sam_session->Connect(addrConnect, conn, proxyConnectionFailed); - } else { - { - LOCK(m_unused_i2p_sessions_mutex); - if (m_unused_i2p_sessions.empty()) { - i2p_transient_session = - std::make_unique<i2p::sam::Session>(proxy, &interruptNet); - } else { - i2p_transient_session.swap(m_unused_i2p_sessions.front()); - m_unused_i2p_sessions.pop(); + if (m_i2p_sam_session) { + connected = m_i2p_sam_session->Connect(target_addr, conn, proxyConnectionFailed); + } else { + { + LOCK(m_unused_i2p_sessions_mutex); + if (m_unused_i2p_sessions.empty()) { + i2p_transient_session = + std::make_unique<i2p::sam::Session>(proxy, &interruptNet); + } else { + i2p_transient_session.swap(m_unused_i2p_sessions.front()); + m_unused_i2p_sessions.pop(); + } } - } - connected = i2p_transient_session->Connect(addrConnect, conn, proxyConnectionFailed); - if (!connected) { - LOCK(m_unused_i2p_sessions_mutex); - if (m_unused_i2p_sessions.size() < MAX_UNUSED_I2P_SESSIONS_SIZE) { - m_unused_i2p_sessions.emplace(i2p_transient_session.release()); + connected = i2p_transient_session->Connect(target_addr, conn, proxyConnectionFailed); + if (!connected) { + LOCK(m_unused_i2p_sessions_mutex); + if (m_unused_i2p_sessions.size() < MAX_UNUSED_I2P_SESSIONS_SIZE) { + m_unused_i2p_sessions.emplace(i2p_transient_session.release()); + } } } - } - if (connected) { - sock = std::move(conn.sock); - addr_bind = CAddress{conn.me, NODE_NONE}; + if (connected) { + sock = std::move(conn.sock); + addr_bind = CAddress{conn.me, NODE_NONE}; + } + } else if (use_proxy) { + LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "Using proxy: %s to connect to %s\n", proxy.ToString(), target_addr.ToStringAddrPort()); + sock = ConnectThroughProxy(proxy, target_addr.ToStringAddr(), target_addr.GetPort(), proxyConnectionFailed); + } else { + // no proxy needed (none set for target network) + sock = ConnectDirectly(target_addr, conn_type == ConnectionType::MANUAL); } - } else if (use_proxy) { - LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "Using proxy: %s to connect to %s:%s\n", proxy.ToString(), addrConnect.ToStringAddr(), addrConnect.GetPort()); - sock = ConnectThroughProxy(proxy, addrConnect.ToStringAddr(), addrConnect.GetPort(), proxyConnectionFailed); - } else { - // no proxy needed (none set for target network) - sock = ConnectDirectly(addrConnect, conn_type == ConnectionType::MANUAL); + if (!proxyConnectionFailed) { + // If a connection to the node was attempted, and failure (if any) is not caused by a problem connecting to + // the proxy, mark this as an attempt. + addrman.Attempt(target_addr, fCountFailure); + } + } else if (pszDest && GetNameProxy(proxy)) { + std::string host; + uint16_t port{default_port}; + SplitHostPort(std::string(pszDest), port, host); + bool proxyConnectionFailed; + sock = ConnectThroughProxy(proxy, host, port, proxyConnectionFailed); + } + // Check any other resolved address (if any) if we fail to connect + if (!sock) { + continue; } - if (!proxyConnectionFailed) { - // If a connection to the node was attempted, and failure (if any) is not caused by a problem connecting to - // the proxy, mark this as an attempt. - addrman.Attempt(addrConnect, fCountFailure); + + NetPermissionFlags permission_flags = NetPermissionFlags::None; + std::vector<NetWhitelistPermissions> whitelist_permissions = conn_type == ConnectionType::MANUAL ? vWhitelistedRangeOutgoing : std::vector<NetWhitelistPermissions>{}; + AddWhitelistPermissionFlags(permission_flags, target_addr, whitelist_permissions); + + // Add node + NodeId id = GetNewNodeId(); + uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); + if (!addr_bind.IsValid()) { + addr_bind = GetBindAddress(*sock); } - } else if (pszDest && GetNameProxy(proxy)) { - std::string host; - uint16_t port{default_port}; - SplitHostPort(std::string(pszDest), port, host); - bool proxyConnectionFailed; - sock = ConnectThroughProxy(proxy, host, port, proxyConnectionFailed); - } - if (!sock) { - return nullptr; - } + CNode* pnode = new CNode(id, + std::move(sock), + target_addr, + CalculateKeyedNetGroup(target_addr), + nonce, + addr_bind, + pszDest ? pszDest : "", + conn_type, + /*inbound_onion=*/false, + CNodeOptions{ + .permission_flags = permission_flags, + .i2p_sam_session = std::move(i2p_transient_session), + .recv_flood_size = nReceiveFloodSize, + .use_v2transport = use_v2transport, + }); + pnode->AddRef(); - NetPermissionFlags permission_flags = NetPermissionFlags::None; - std::vector<NetWhitelistPermissions> whitelist_permissions = conn_type == ConnectionType::MANUAL ? vWhitelistedRangeOutgoing : std::vector<NetWhitelistPermissions>{}; - AddWhitelistPermissionFlags(permission_flags, addrConnect, whitelist_permissions); + // We're making a new connection, harvest entropy from the time (and our peer count) + RandAddEvent((uint32_t)id); - // Add node - NodeId id = GetNewNodeId(); - uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); - if (!addr_bind.IsValid()) { - addr_bind = GetBindAddress(*sock); + return pnode; } - CNode* pnode = new CNode(id, - std::move(sock), - addrConnect, - CalculateKeyedNetGroup(addrConnect), - nonce, - addr_bind, - pszDest ? pszDest : "", - conn_type, - /*inbound_onion=*/false, - CNodeOptions{ - .permission_flags = permission_flags, - .i2p_sam_session = std::move(i2p_transient_session), - .recv_flood_size = nReceiveFloodSize, - .use_v2transport = use_v2transport, - }); - pnode->AddRef(); - - // We're making a new connection, harvest entropy from the time (and our peer count) - RandAddEvent((uint32_t)id); - return pnode; + return nullptr; } void CNode::CloseSocketDisconnect() @@ -2256,7 +2272,11 @@ void CConnman::ThreadDNSAddressSeed() if (!resolveSource.SetInternal(host)) { continue; } - unsigned int nMaxIPs = 256; // Limits number of IPs learned from a DNS seed + // Limit number of IPs learned from a single DNS seed. This limit exists to prevent the results from + // one DNS seed from dominating AddrMan. Note that the number of results from a UDP DNS query is + // bounded to 33 already, but it is possible for it to use TCP where a larger number of results can be + // returned. + unsigned int nMaxIPs = 32; const auto addresses{LookupHost(host, nMaxIPs, true)}; if (!addresses.empty()) { for (const CNetAddr& ip : addresses) { diff --git a/src/netbase.cpp b/src/netbase.cpp index 3ca1a5227a..f0fa298378 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -632,10 +632,7 @@ std::unique_ptr<Sock> ConnectDirectly(const CService& dest, bool manual_connecti std::unique_ptr<Sock> Proxy::Connect() const { - if (!IsValid()) { - LogPrintf("Cannot connect to invalid Proxy\n"); - return {}; - } + if (!IsValid()) return {}; if (!m_is_unix_socket) return ConnectDirectly(proxy, /*manual_connection=*/true); @@ -656,7 +653,6 @@ std::unique_ptr<Sock> Proxy::Connect() const socklen_t len = sizeof(addrun); if(!ConnectToSocket(*sock, (struct sockaddr*)&addrun, len, path, /*manual_connection=*/true)) { - LogPrintf("Cannot connect to socket for %s\n", path); return {}; } diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 84c3352b9d..2ad79b6f99 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -191,13 +191,13 @@ std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram( int64_t replacement_vsize) { // Require that the replacement strictly improves the mempool's feerate diagram. - const auto diagram_results{pool.CalculateFeerateDiagramsForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)}; + const auto chunk_results{pool.CalculateChunksForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)}; - if (!diagram_results.has_value()) { - return std::make_pair(DiagramCheckError::UNCALCULABLE, util::ErrorString(diagram_results).original); + if (!chunk_results.has_value()) { + return std::make_pair(DiagramCheckError::UNCALCULABLE, util::ErrorString(chunk_results).original); } - if (!std::is_gt(CompareFeerateDiagram(diagram_results.value().second, diagram_results.value().first))) { + if (!std::is_gt(CompareChunks(chunk_results.value().second, chunk_results.value().first))) { return std::make_pair(DiagramCheckError::FAILURE, "insufficient feerate: does not improve feerate diagram"); } return std::nullopt; diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index c52ef7cd67..efdc3966d1 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -369,21 +369,22 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con else if(type == Receive) { // Generate a new address to associate with given label - auto op_dest = walletModel->wallet().getNewDestination(address_type, strLabel); - if (!op_dest) { + if (auto dest{walletModel->wallet().getNewDestination(address_type, strLabel)}) { + strAddress = EncodeDestination(*dest); + } else { WalletModel::UnlockContext ctx(walletModel->requestUnlock()); if (!ctx.isValid()) { // Unlock wallet failed or was cancelled editStatus = WALLET_UNLOCK_FAILURE; return QString(); } - op_dest = walletModel->wallet().getNewDestination(address_type, strLabel); - if (!op_dest) { + if (auto dest_retry{walletModel->wallet().getNewDestination(address_type, strLabel)}) { + strAddress = EncodeDestination(*dest_retry); + } else { editStatus = KEY_GENERATION_FAILURE; return QString(); } } - strAddress = EncodeDestination(*op_dest); } else { diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp index 084b9a6615..1d1a84e375 100644 --- a/src/qt/guiutil.cpp +++ b/src/qt/guiutil.cpp @@ -109,22 +109,26 @@ QFont fixedPitchFont(bool use_embedded_font) return QFontDatabase::systemFont(QFontDatabase::FixedFont); } -// Just some dummy data to generate a convincing random-looking (but consistent) address -static const uint8_t dummydata[] = {0xeb,0x15,0x23,0x1d,0xfc,0xeb,0x60,0x92,0x58,0x86,0xb6,0x7d,0x06,0x52,0x99,0x92,0x59,0x15,0xae,0xb1,0x72,0xc0,0x66,0x47}; - -// Generate a dummy address with invalid CRC, starting with the network prefix. +// Return a pre-generated dummy bech32m address (P2TR) with invalid checksum. static std::string DummyAddress(const CChainParams ¶ms) { - std::vector<unsigned char> sourcedata = params.Base58Prefix(CChainParams::PUBKEY_ADDRESS); - sourcedata.insert(sourcedata.end(), dummydata, dummydata + sizeof(dummydata)); - for(int i=0; i<256; ++i) { // Try every trailing byte - std::string s = EncodeBase58(sourcedata); - if (!IsValidDestinationString(s)) { - return s; - } - sourcedata[sourcedata.size()-1] += 1; - } - return ""; + std::string addr; + switch (params.GetChainType()) { + case ChainType::MAIN: + addr = "bc1p35yvjel7srp783ztf8v6jdra7dhfzk5jaun8xz2qp6ws7z80n4tq2jku9f"; + break; + case ChainType::SIGNET: + case ChainType::TESTNET: + addr = "tb1p35yvjel7srp783ztf8v6jdra7dhfzk5jaun8xz2qp6ws7z80n4tqa6qnlg"; + break; + case ChainType::REGTEST: + addr = "bcrt1p35yvjel7srp783ztf8v6jdra7dhfzk5jaun8xz2qp6ws7z80n4tqsr2427"; + break; + } // no default case, so the compiler can warn about missing cases + assert(!addr.empty()); + + if (Assume(!IsValidDestinationString(addr))) return addr; + return {}; } void setupAddressWidget(QValidatedLineEdit *widget, QWidget *parent) diff --git a/src/qt/main.cpp b/src/qt/main.cpp index ded057dbfa..16befd99e8 100644 --- a/src/qt/main.cpp +++ b/src/qt/main.cpp @@ -4,7 +4,6 @@ #include <qt/bitcoin.h> -#include <common/url.h> #include <compat/compat.h> #include <util/translation.h> @@ -17,7 +16,6 @@ extern const std::function<std::string(const char*)> G_TRANSLATION_FUN = [](const char* psz) { return QCoreApplication::translate("bitcoin-core", psz).toStdString(); }; -UrlDecodeFn* const URL_DECODE = urlDecode; const std::function<std::string()> G_TEST_GET_FULL_NAME{}; diff --git a/src/qt/notificator.cpp b/src/qt/notificator.cpp index 2021e5f9dc..551c0ffd13 100644 --- a/src/qt/notificator.cpp +++ b/src/qt/notificator.cpp @@ -112,10 +112,10 @@ FreedesktopImage::FreedesktopImage(const QImage &img): for(unsigned int ptr = 0; ptr < num_pixels; ++ptr) { - image[ptr*BYTES_PER_PIXEL+0] = data[ptr] >> 16; // R - image[ptr*BYTES_PER_PIXEL+1] = data[ptr] >> 8; // G - image[ptr*BYTES_PER_PIXEL+2] = data[ptr]; // B - image[ptr*BYTES_PER_PIXEL+3] = data[ptr] >> 24; // A + image[ptr * BYTES_PER_PIXEL + 0] = char(data[ptr] >> 16); // R + image[ptr * BYTES_PER_PIXEL + 1] = char(data[ptr] >> 8); // G + image[ptr * BYTES_PER_PIXEL + 2] = char(data[ptr]); // B + image[ptr * BYTES_PER_PIXEL + 3] = char(data[ptr] >> 24); // A } } diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 1bdf94d3b5..fe000bcbb8 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -569,16 +569,17 @@ bool WalletModel::bumpFee(uint256 hash, uint256& new_hash) return true; } -bool WalletModel::displayAddress(std::string sAddress) const +void WalletModel::displayAddress(std::string sAddress) const { CTxDestination dest = DecodeDestination(sAddress); - bool res = false; try { - res = m_wallet->displayAddress(dest); + util::Result<void> result = m_wallet->displayAddress(dest); + if (!result) { + QMessageBox::warning(nullptr, tr("Signer error"), QString::fromStdString(util::ErrorString(result).translated)); + } } catch (const std::runtime_error& e) { QMessageBox::critical(nullptr, tr("Can't display address"), e.what()); } - return res; } bool WalletModel::isWalletEnabled() diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 503ee16823..ab2096c1fe 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -130,7 +130,7 @@ public: UnlockContext requestUnlock(); bool bumpFee(uint256 hash, uint256& new_hash); - bool displayAddress(std::string sAddress) const; + void displayAddress(std::string sAddress) const; static bool isWalletEnabled(); diff --git a/src/qt/walletview.cpp b/src/qt/walletview.cpp index 09e0771534..4926d1e80b 100644 --- a/src/qt/walletview.cpp +++ b/src/qt/walletview.cpp @@ -134,7 +134,7 @@ void WalletView::processNewTransaction(const QModelIndex& parent, int start, int return; QString date = ttm->index(start, TransactionTableModel::Date, parent).data().toString(); - qint64 amount = ttm->index(start, TransactionTableModel::Amount, parent).data(Qt::EditRole).toULongLong(); + qint64 amount = ttm->index(start, TransactionTableModel::Amount, parent).data(Qt::EditRole).toLongLong(); QString type = ttm->index(start, TransactionTableModel::Type, parent).data().toString(); QModelIndex index = ttm->index(start, 0, parent); QString address = ttm->data(index, TransactionTableModel::AddressRole).toString(); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index a1135c27d4..eed004806a 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -2174,7 +2174,8 @@ static RPCHelpMan scantxoutset() [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { UniValue result(UniValue::VOBJ); - if (request.params[0].get_str() == "status") { + const auto action{self.Arg<std::string>("action")}; + if (action == "status") { CoinsViewScanReserver reserver; if (reserver.reserve()) { // no scan in progress @@ -2182,7 +2183,7 @@ static RPCHelpMan scantxoutset() } result.pushKV("progress", g_scan_progress.load()); return result; - } else if (request.params[0].get_str() == "abort") { + } else if (action == "abort") { CoinsViewScanReserver reserver; if (reserver.reserve()) { // reserve was possible which means no scan was running @@ -2191,7 +2192,7 @@ static RPCHelpMan scantxoutset() // set the abort flag g_should_abort_scan = true; return true; - } else if (request.params[0].get_str() == "start") { + } else if (action == "start") { CoinsViewScanReserver reserver; if (!reserver.reserve()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Scan already in progress, use action \"abort\" or \"status\""); @@ -2260,7 +2261,7 @@ static RPCHelpMan scantxoutset() result.pushKV("unspents", unspents); result.pushKV("total_amount", ValueFromAmount(total_in)); } else { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid action '%s'", request.params[0].get_str())); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid action '%s'", action)); } return result; }, diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index f7cdbf52dd..454c262803 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -122,7 +122,7 @@ static RPCHelpMan getnetworkhashps() { ChainstateManager& chainman = EnsureAnyChainman(request.context); LOCK(cs_main); - return GetNetworkHashPS(self.Arg<int>(0), self.Arg<int>(1), chainman.ActiveChain()); + return GetNetworkHashPS(self.Arg<int>("nblocks"), self.Arg<int>("height"), chainman.ActiveChain()); }, }; } @@ -229,12 +229,12 @@ static RPCHelpMan generatetodescriptor() "\nGenerate 11 blocks to mydesc\n" + HelpExampleCli("generatetodescriptor", "11 \"mydesc\"")}, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - const auto num_blocks{self.Arg<int>(0)}; - const auto max_tries{self.Arg<uint64_t>(2)}; + const auto num_blocks{self.Arg<int>("num_blocks")}; + const auto max_tries{self.Arg<uint64_t>("maxtries")}; CScript coinbase_script; std::string error; - if (!getScriptFromDescriptor(self.Arg<std::string>(1), coinbase_script, error)) { + if (!getScriptFromDescriptor(self.Arg<std::string>("descriptor"), coinbase_script, error)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error); } diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index f935a3b08f..49789f538b 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -322,7 +322,7 @@ static RPCHelpMan addnode() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - const std::string command{request.params[1].get_str()}; + const auto command{self.Arg<std::string>("command")}; if (command != "onetry" && command != "add" && command != "remove") { throw std::runtime_error( self.ToString()); @@ -331,9 +331,9 @@ static RPCHelpMan addnode() NodeContext& node = EnsureAnyNodeContext(request.context); CConnman& connman = EnsureConnman(node); - const std::string node_arg{request.params[0].get_str()}; + const auto node_arg{self.Arg<std::string>("node")}; bool node_v2transport = connman.GetLocalServices() & NODE_P2P_V2; - bool use_v2transport = self.MaybeArg<bool>(2).value_or(node_v2transport); + bool use_v2transport = self.MaybeArg<bool>("v2transport").value_or(node_v2transport); if (use_v2transport && !node_v2transport) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: v2transport requested but not enabled (see -v2transport)"); diff --git a/src/rpc/node.cpp b/src/rpc/node.cpp index b8c0080aef..ffc2ee5ab0 100644 --- a/src/rpc/node.cpp +++ b/src/rpc/node.cpp @@ -26,6 +26,7 @@ #include <univalue.h> #include <util/any.h> #include <util/check.h> +#include <util/time.h> #include <stdint.h> #ifdef HAVE_MALLOC_INFO @@ -58,9 +59,11 @@ static RPCHelpMan setmocktime() LOCK(cs_main); const int64_t time{request.params[0].getInt<int64_t>()}; - if (time < 0) { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Mocktime cannot be negative: %s.", time)); + constexpr int64_t max_time{Ticks<std::chrono::seconds>(std::chrono::nanoseconds::max())}; + if (time < 0 || time > max_time) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Mocktime must be in the range [0, %s], not %s.", max_time, time)); } + SetMockTime(time); const NodeContext& node_context{EnsureAnyNodeContext(request.context)}; for (const auto& chain_client : node_context.chain_clients) { diff --git a/src/rpc/signmessage.cpp b/src/rpc/signmessage.cpp index 8c752ba1fd..9f3c24efcf 100644 --- a/src/rpc/signmessage.cpp +++ b/src/rpc/signmessage.cpp @@ -38,9 +38,9 @@ static RPCHelpMan verifymessage() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::string strAddress = request.params[0].get_str(); - std::string strSign = request.params[1].get_str(); - std::string strMessage = request.params[2].get_str(); + std::string strAddress = self.Arg<std::string>("address"); + std::string strSign = self.Arg<std::string>("signature"); + std::string strMessage = self.Arg<std::string>("message"); switch (MessageVerify(strAddress, strSign, strMessage)) { case MessageVerificationResult::ERR_INVALID_ADDRESS: diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 6e332e3855..f683878054 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -24,6 +24,8 @@ #include <util/string.h> #include <util/translation.h> +#include <algorithm> +#include <iterator> #include <string_view> #include <tuple> @@ -729,6 +731,16 @@ std::vector<std::pair<std::string, bool>> RPCHelpMan::GetArgNames() const return ret; } +size_t RPCHelpMan::GetParamIndex(std::string_view key) const +{ + auto it{std::find_if( + m_args.begin(), m_args.end(), [&key](const auto& arg) { return arg.GetName() == key;} + )}; + + CHECK_NONFATAL(it != m_args.end()); // TODO: ideally this is checked at compile time + return std::distance(m_args.begin(), it); +} + std::string RPCHelpMan::ToString() const { std::string ret; diff --git a/src/rpc/util.h b/src/rpc/util.h index f6ee6a317a..177af90c05 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -404,18 +404,25 @@ public: UniValue HandleRequest(const JSONRPCRequest& request) const; /** - * Helper to get a request argument. - * This function only works during m_fun(), i.e. it should only be used in - * RPC method implementations. The helper internally checks whether the - * user-passed argument isNull() and parses (from JSON) and returns the - * user-passed argument, or the default value derived from the RPCArg - * documentation, or a falsy value if no default was given. + * @brief Helper to get a required or default-valued request argument. * - * Use Arg<Type>(i) to get the argument or its default value. Otherwise, - * use MaybeArg<Type>(i) to get the optional argument or a falsy value. + * Use this function when the argument is required or when it has a default value. If the + * argument is optional and may not be provided, use MaybeArg instead. * - * The Type passed to this helper must match the corresponding - * RPCArg::Type. + * This function only works during m_fun(), i.e., it should only be used in + * RPC method implementations. It internally checks whether the user-passed + * argument isNull() and parses (from JSON) and returns the user-passed argument, + * or the default value derived from the RPCArg documentation. + * + * There are two overloads of this function: + * - Use Arg<Type>(size_t i) to get the argument (or the default value) by index. + * - Use Arg<Type>(const std::string& key) to get the argument (or the default value) by key. + * + * The Type passed to this helper must match the corresponding RPCArg::Type. + * + * @return The value of the RPC argument (or the default value) cast to type Type. + * + * @see MaybeArg for handling optional arguments without default values. */ template <typename R> auto Arg(size_t i) const @@ -429,6 +436,34 @@ public: return ArgValue<const R&>(i); } } + template<typename R> + auto Arg(std::string_view key) const + { + return Arg<R>(GetParamIndex(key)); + } + /** + * @brief Helper to get an optional request argument. + * + * Use this function when the argument is optional and does not have a default value. If the + * argument is required or has a default value, use Arg instead. + * + * This function only works during m_fun(), i.e., it should only be used in + * RPC method implementations. It internally checks whether the user-passed + * argument isNull() and parses (from JSON) and returns the user-passed argument, + * or a falsy value if no argument was passed. + * + * There are two overloads of this function: + * - Use MaybeArg<Type>(size_t i) to get the optional argument by index. + * - Use MaybeArg<Type>(const std::string& key) to get the optional argument by key. + * + * The Type passed to this helper must match the corresponding RPCArg::Type. + * + * @return For integral and floating-point types, a std::optional<Type> is returned. + * For other types, a Type* pointer to the argument is returned. If the + * argument is not provided, std::nullopt or a null pointer is returned. + * + * @see Arg for handling arguments that are required or have a default value. + */ template <typename R> auto MaybeArg(size_t i) const { @@ -441,6 +476,11 @@ public: return ArgValue<const R*>(i); } } + template<typename R> + auto MaybeArg(std::string_view key) const + { + return MaybeArg<R>(GetParamIndex(key)); + } std::string ToString() const; /** Return the named args that need to be converted from string to another JSON type */ UniValue GetArgMap() const; @@ -460,6 +500,8 @@ private: mutable const JSONRPCRequest* m_req{nullptr}; // A pointer to the request for the duration of m_fun() template <typename R> R ArgValue(size_t i) const; + //! Return positional index of a parameter using its name as key. + size_t GetParamIndex(std::string_view key) const; }; /** diff --git a/src/script/sign.cpp b/src/script/sign.cpp index be4b357568..22ac062a63 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -295,7 +295,7 @@ struct TapSatisfier: Satisfier<XOnlyPubKey> { //! Conversion from a raw xonly public key. template <typename I> std::optional<XOnlyPubKey> FromPKBytes(I first, I last) const { - CHECK_NONFATAL(last - first == 32); + if (last - first != 32) return {}; XOnlyPubKey pubkey; std::copy(first, last, pubkey.begin()); return pubkey; diff --git a/src/test/common_url_tests.cpp b/src/test/common_url_tests.cpp new file mode 100644 index 0000000000..065c7d97bc --- /dev/null +++ b/src/test/common_url_tests.cpp @@ -0,0 +1,75 @@ +// Copyright (c) 2024-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +#include <common/url.h> + +#include <string> + +#include <boost/test/unit_test.hpp> + +BOOST_AUTO_TEST_SUITE(common_url_tests) + +// These test vectors were ported from test/regress.c in the libevent library +// which used to be a dependency of the UrlDecode function. + +BOOST_AUTO_TEST_CASE(encode_decode_test) { + BOOST_CHECK_EQUAL(UrlDecode("Hello"), "Hello"); + BOOST_CHECK_EQUAL(UrlDecode("99"), "99"); + BOOST_CHECK_EQUAL(UrlDecode(""), ""); + BOOST_CHECK_EQUAL(UrlDecode("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ123456789-.~_"), + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ123456789-.~_"); + BOOST_CHECK_EQUAL(UrlDecode("%20"), " "); + BOOST_CHECK_EQUAL(UrlDecode("%FF%F0%E0"), "\xff\xf0\xe0"); + BOOST_CHECK_EQUAL(UrlDecode("%01%19"), "\x01\x19"); + BOOST_CHECK_EQUAL(UrlDecode("http%3A%2F%2Fwww.ietf.org%2Frfc%2Frfc3986.txt"), + "http://www.ietf.org/rfc/rfc3986.txt"); + BOOST_CHECK_EQUAL(UrlDecode("1%2B2%3D3"), "1+2=3"); +} + +BOOST_AUTO_TEST_CASE(decode_malformed_test) { + BOOST_CHECK_EQUAL(UrlDecode("%%xhello th+ere \xff"), "%%xhello th+ere \xff"); + + BOOST_CHECK_EQUAL(UrlDecode("%"), "%"); + BOOST_CHECK_EQUAL(UrlDecode("%%"), "%%"); + BOOST_CHECK_EQUAL(UrlDecode("%%%"), "%%%"); + BOOST_CHECK_EQUAL(UrlDecode("%%%%"), "%%%%"); + + BOOST_CHECK_EQUAL(UrlDecode("+"), "+"); + BOOST_CHECK_EQUAL(UrlDecode("++"), "++"); + + BOOST_CHECK_EQUAL(UrlDecode("?"), "?"); + BOOST_CHECK_EQUAL(UrlDecode("??"), "??"); + + BOOST_CHECK_EQUAL(UrlDecode("%G1"), "%G1"); + BOOST_CHECK_EQUAL(UrlDecode("%2"), "%2"); + BOOST_CHECK_EQUAL(UrlDecode("%ZX"), "%ZX"); + + BOOST_CHECK_EQUAL(UrlDecode("valid%20string%G1"), "valid string%G1"); + BOOST_CHECK_EQUAL(UrlDecode("%20invalid%ZX"), " invalid%ZX"); + BOOST_CHECK_EQUAL(UrlDecode("%20%G1%ZX"), " %G1%ZX"); + + BOOST_CHECK_EQUAL(UrlDecode("%1 "), "%1 "); + BOOST_CHECK_EQUAL(UrlDecode("% 9"), "% 9"); + BOOST_CHECK_EQUAL(UrlDecode(" %Z "), " %Z "); + BOOST_CHECK_EQUAL(UrlDecode(" % X"), " % X"); + + BOOST_CHECK_EQUAL(UrlDecode("%%ffg"), "%\xffg"); + BOOST_CHECK_EQUAL(UrlDecode("%fg"), "%fg"); + + BOOST_CHECK_EQUAL(UrlDecode("%-1"), "%-1"); + BOOST_CHECK_EQUAL(UrlDecode("%1-"), "%1-"); +} + +BOOST_AUTO_TEST_CASE(decode_lowercase_hex_test) { + BOOST_CHECK_EQUAL(UrlDecode("%f0%a0%b0"), "\xf0\xa0\xb0"); +} + +BOOST_AUTO_TEST_CASE(decode_internal_nulls_test) { + std::string result1{"\0\0x\0\0", 5}; + BOOST_CHECK_EQUAL(UrlDecode("%00%00x%00%00"), result1); + std::string result2{"abc\0\0", 5}; + BOOST_CHECK_EQUAL(UrlDecode("abc%00%00"), result2); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/feefrac_tests.cpp b/src/test/feefrac_tests.cpp index 2e015b382e..5af3c3d7ed 100644 --- a/src/test/feefrac_tests.cpp +++ b/src/test/feefrac_tests.cpp @@ -82,43 +82,4 @@ BOOST_AUTO_TEST_CASE(feefrac_operators) } -BOOST_AUTO_TEST_CASE(build_diagram_test) -{ - FeeFrac p1{1000, 100}; - FeeFrac empty{0, 0}; - FeeFrac zero_fee{0, 1}; - FeeFrac oversized_1{4611686000000, 4000000}; - FeeFrac oversized_2{184467440000000, 100000}; - - // Diagram-building will reorder chunks - std::vector<FeeFrac> chunks; - chunks.push_back(p1); - chunks.push_back(zero_fee); - chunks.push_back(empty); - chunks.push_back(oversized_1); - chunks.push_back(oversized_2); - - // Caller in charge of sorting chunks in case chunk size limit - // differs from cluster size limit - std::sort(chunks.begin(), chunks.end(), [](const FeeFrac& a, const FeeFrac& b) { return a > b; }); - - // Chunks are now sorted in reverse order (largest first) - BOOST_CHECK(chunks[0] == empty); // empty is considered "highest" fee - BOOST_CHECK(chunks[1] == oversized_2); - BOOST_CHECK(chunks[2] == oversized_1); - BOOST_CHECK(chunks[3] == p1); - BOOST_CHECK(chunks[4] == zero_fee); - - std::vector<FeeFrac> generated_diagram{BuildDiagramFromChunks(chunks)}; - BOOST_CHECK(generated_diagram.size() == 1 + chunks.size()); - - // Prepended with an empty, then the chunks summed in order as above - BOOST_CHECK(generated_diagram[0] == empty); - BOOST_CHECK(generated_diagram[1] == empty); - BOOST_CHECK(generated_diagram[2] == oversized_2); - BOOST_CHECK(generated_diagram[3] == oversized_2 + oversized_1); - BOOST_CHECK(generated_diagram[4] == oversized_2 + oversized_1 + p1); - BOOST_CHECK(generated_diagram[5] == oversized_2 + oversized_1 + p1 + zero_fee); -} - BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/addition_overflow.cpp b/src/test/fuzz/addition_overflow.cpp index 5100b6f438..071e5fb029 100644 --- a/src/test/fuzz/addition_overflow.cpp +++ b/src/test/fuzz/addition_overflow.cpp @@ -24,12 +24,14 @@ void TestAdditionOverflow(FuzzedDataProvider& fuzzed_data_provider) assert(is_addition_overflow_custom == AdditionOverflow(j, i)); assert(maybe_add == CheckedAdd(j, i)); assert(sat_add == SaturatingAdd(j, i)); +#ifndef _MSC_VER T result_builtin; const bool is_addition_overflow_builtin = __builtin_add_overflow(i, j, &result_builtin); assert(is_addition_overflow_custom == is_addition_overflow_builtin); if (!is_addition_overflow_custom) { assert(i + j == result_builtin); } +#endif if (is_addition_overflow_custom) { assert(sat_add == std::numeric_limits<T>::min() || sat_add == std::numeric_limits<T>::max()); } else { diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp index ebc5673e71..c9a3bc86ac 100644 --- a/src/test/fuzz/deserialize.cpp +++ b/src/test/fuzz/deserialize.cpp @@ -33,7 +33,6 @@ #include <optional> #include <stdexcept> #include <stdint.h> -#include <unistd.h> using node::SnapshotMetadata; diff --git a/src/test/fuzz/feeratediagram.cpp b/src/test/fuzz/feeratediagram.cpp index 6d710093cb..1a9c5ee946 100644 --- a/src/test/fuzz/feeratediagram.cpp +++ b/src/test/fuzz/feeratediagram.cpp @@ -16,6 +16,20 @@ namespace { +/** Takes the pre-computed and topologically-valid chunks and generates a fee diagram which starts at FeeFrac of (0, 0) */ +std::vector<FeeFrac> BuildDiagramFromChunks(const Span<const FeeFrac> chunks) +{ + std::vector<FeeFrac> diagram; + diagram.reserve(chunks.size() + 1); + + diagram.emplace_back(0, 0); + for (auto& chunk : chunks) { + diagram.emplace_back(diagram.back() + chunk); + } + return diagram; +} + + /** Evaluate a diagram at a specific size, returning the fee as a fraction. * * Fees in diagram cannot exceed 2^32, as the returned evaluation could overflow @@ -103,7 +117,7 @@ FUZZ_TARGET(build_and_compare_feerate_diagram) assert(diagram1.front() == empty); assert(diagram2.front() == empty); - auto real = CompareFeerateDiagram(diagram1, diagram2); + auto real = CompareChunks(chunks1, chunks2); auto sim = CompareDiagrams(diagram1, diagram2); assert(real == sim); diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp index a8e490b459..f9915187bd 100644 --- a/src/test/fuzz/fuzz.cpp +++ b/src/test/fuzz/fuzz.cpp @@ -25,7 +25,6 @@ #include <memory> #include <string> #include <tuple> -#include <unistd.h> #include <utility> #include <vector> @@ -135,9 +134,9 @@ void initialize() #if defined(PROVIDE_FUZZ_MAIN_FUNCTION) static bool read_stdin(std::vector<uint8_t>& data) { - uint8_t buffer[1024]; - ssize_t length = 0; - while ((length = read(STDIN_FILENO, buffer, 1024)) > 0) { + std::istream::char_type buffer[1024]; + std::streamsize length; + while ((std::cin.read(buffer, 1024), length = std::cin.gcount()) > 0) { data.insert(data.end(), buffer, buffer + length); } return length == 0; diff --git a/src/test/fuzz/multiplication_overflow.cpp b/src/test/fuzz/multiplication_overflow.cpp index aeef4f24b7..a762a4dfe3 100644 --- a/src/test/fuzz/multiplication_overflow.cpp +++ b/src/test/fuzz/multiplication_overflow.cpp @@ -17,12 +17,18 @@ void TestMultiplicationOverflow(FuzzedDataProvider& fuzzed_data_provider) const T i = fuzzed_data_provider.ConsumeIntegral<T>(); const T j = fuzzed_data_provider.ConsumeIntegral<T>(); const bool is_multiplication_overflow_custom = MultiplicationOverflow(i, j); +#ifndef _MSC_VER T result_builtin; const bool is_multiplication_overflow_builtin = __builtin_mul_overflow(i, j, &result_builtin); assert(is_multiplication_overflow_custom == is_multiplication_overflow_builtin); if (!is_multiplication_overflow_custom) { assert(i * j == result_builtin); } +#else + if (!is_multiplication_overflow_custom) { + (void)(i * j); + } +#endif } } // namespace diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp index ab75afe066..2bf47930f4 100644 --- a/src/test/fuzz/partially_downloaded_block.cpp +++ b/src/test/fuzz/partially_downloaded_block.cpp @@ -72,7 +72,7 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb) available.insert(i); } - if (add_to_mempool) { + if (add_to_mempool && !pool.exists(GenTxid::Txid(tx->GetHash()))) { LOCK2(cs_main, pool.cs); pool.addUnchecked(ConsumeTxMemPoolEntry(fuzzed_data_provider, *tx)); available.insert(i); diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp index 754aff4e70..64785948f6 100644 --- a/src/test/fuzz/rbf.cpp +++ b/src/test/fuzz/rbf.cpp @@ -82,17 +82,6 @@ FUZZ_TARGET(rbf, .init = initialize_rbf) } } -void CheckDiagramConcave(std::vector<FeeFrac>& diagram) -{ - // Diagrams are in monotonically-decreasing feerate order. - FeeFrac last_chunk = diagram.front(); - for (size_t i = 1; i<diagram.size(); ++i) { - FeeFrac next_chunk = diagram[i] - diagram[i-1]; - assert(next_chunk <= last_chunk); - last_chunk = next_chunk; - } -} - FUZZ_TARGET(package_rbf, .init = initialize_package_rbf) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); @@ -107,6 +96,12 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf) std::vector<CTransaction> mempool_txs; size_t iter{0}; + int32_t replacement_vsize = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(1, 1000000); + + // Keep track of the total vsize of CTxMemPoolEntry's being added to the mempool to avoid overflow + // Add replacement_vsize since this is added to new diagram during RBF check + int64_t running_vsize_total{replacement_vsize}; + LOCK2(cs_main, pool.cs); LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), NUM_ITERS) @@ -114,19 +109,33 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf) // Make sure txns only have one input, and that a unique input is given to avoid circular references std::optional<CMutableTransaction> parent = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider, TX_WITH_WITNESS); if (!parent) { - continue; + return; } assert(iter <= g_outpoints.size()); parent->vin.resize(1); parent->vin[0].prevout = g_outpoints[iter++]; mempool_txs.emplace_back(*parent); - pool.addUnchecked(ConsumeTxMemPoolEntry(fuzzed_data_provider, mempool_txs.back())); + const auto parent_entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, mempool_txs.back()); + running_vsize_total += parent_entry.GetTxSize(); + if (running_vsize_total > std::numeric_limits<int32_t>::max()) { + // We aren't adding this final tx to mempool, so we don't want to conflict with it + mempool_txs.pop_back(); + break; + } + pool.addUnchecked(parent_entry); if (fuzzed_data_provider.ConsumeBool() && !child->vin.empty()) { child->vin[0].prevout = COutPoint{mempool_txs.back().GetHash(), 0}; } mempool_txs.emplace_back(*child); - pool.addUnchecked(ConsumeTxMemPoolEntry(fuzzed_data_provider, mempool_txs.back())); + const auto child_entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, mempool_txs.back()); + running_vsize_total += child_entry.GetTxSize(); + if (running_vsize_total > std::numeric_limits<int32_t>::max()) { + // We aren't adding this final tx to mempool, so we don't want to conflict with it + mempool_txs.pop_back(); + break; + } + pool.addUnchecked(child_entry); if (fuzzed_data_provider.ConsumeBool()) { pool.PrioritiseTransaction(mempool_txs.back().GetHash().ToUint256(), fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(-100000, 100000)); @@ -147,33 +156,33 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf) pool.CalculateDescendants(txiter, all_conflicts); } - // Calculate the feerate diagrams for a replacement. + // Calculate the chunks for a replacement. CAmount replacement_fees = ConsumeMoney(fuzzed_data_provider); - int64_t replacement_vsize = fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(1, 1000000); - auto calc_results{pool.CalculateFeerateDiagramsForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)}; + auto calc_results{pool.CalculateChunksForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)}; if (calc_results.has_value()) { - // Sanity checks on the diagrams. + // Sanity checks on the chunks. - // Diagrams start at 0. - assert(calc_results->first.front().size == 0); - assert(calc_results->first.front().fee == 0); - assert(calc_results->second.front().size == 0); - assert(calc_results->second.front().fee == 0); - - CheckDiagramConcave(calc_results->first); - CheckDiagramConcave(calc_results->second); + // Feerates are monotonically decreasing. + FeeFrac first_sum; + for (size_t i = 0; i < calc_results->first.size(); ++i) { + first_sum += calc_results->first[i]; + if (i) assert(!(calc_results->first[i - 1] << calc_results->first[i])); + } + FeeFrac second_sum; + for (size_t i = 0; i < calc_results->second.size(); ++i) { + second_sum += calc_results->second[i]; + if (i) assert(!(calc_results->second[i - 1] << calc_results->second[i])); + } - CAmount replaced_fee{0}; - int64_t replaced_size{0}; + FeeFrac replaced; for (auto txiter : all_conflicts) { - replaced_fee += txiter->GetModifiedFee(); - replaced_size += txiter->GetTxSize(); + replaced.fee += txiter->GetModifiedFee(); + replaced.size += txiter->GetTxSize(); } - // The total fee of the new diagram should be the total fee of the old - // diagram - replaced_fee + replacement_fees - assert(calc_results->first.back().fee - replaced_fee + replacement_fees == calc_results->second.back().fee); - assert(calc_results->first.back().size - replaced_size + replacement_vsize == calc_results->second.back().size); + // The total fee & size of the new diagram minus replaced fee & size should be the total + // fee & size of the old diagram minus replacement fee & size. + assert((first_sum - replaced) == (second_sum - FeeFrac{replacement_fees, replacement_vsize})); } // If internals report error, wrapper should too @@ -182,10 +191,12 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf) assert(err_tuple.value().first == DiagramCheckError::UNCALCULABLE); } else { // Diagram check succeeded + auto old_sum = std::accumulate(calc_results->first.begin(), calc_results->first.end(), FeeFrac{}); + auto new_sum = std::accumulate(calc_results->second.begin(), calc_results->second.end(), FeeFrac{}); if (!err_tuple.has_value()) { // New diagram's final fee should always match or exceed old diagram's - assert(calc_results->first.back().fee <= calc_results->second.back().fee); - } else if (calc_results->first.back().fee > calc_results->second.back().fee) { + assert(old_sum.fee <= new_sum.fee); + } else if (old_sum.fee > new_sum.fee) { // Or it failed, and if old diagram had higher fees, it should be a failure assert(err_tuple.value().first == DiagramCheckError::FAILURE); } diff --git a/src/test/fuzz/string.cpp b/src/test/fuzz/string.cpp index e81efac6e0..631da13803 100644 --- a/src/test/fuzz/string.cpp +++ b/src/test/fuzz/string.cpp @@ -90,7 +90,7 @@ FUZZ_TARGET(string) (void)ToUpper(random_string_1); (void)TrimString(random_string_1); (void)TrimString(random_string_1, random_string_2); - (void)urlDecode(random_string_1); + (void)UrlDecode(random_string_1); (void)ContainsNoNUL(random_string_1); (void)_(random_string_1.c_str()); try { diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index 217e4a6d22..9f8d434213 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -202,9 +202,11 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) tx7.vout[1].scriptPubKey = CScript() << OP_11 << OP_EQUAL; tx7.vout[1].nValue = 1 * COIN; - auto ancestors_calculated{pool.CalculateMemPoolAncestors(entry.Fee(2000000LL).FromTx(tx7), CTxMemPool::Limits::NoLimits())}; - BOOST_REQUIRE(ancestors_calculated.has_value()); - BOOST_CHECK(*ancestors_calculated == setAncestors); + { + auto ancestors_calculated{pool.CalculateMemPoolAncestors(entry.Fee(2000000LL).FromTx(tx7), CTxMemPool::Limits::NoLimits())}; + BOOST_REQUIRE(ancestors_calculated.has_value()); + BOOST_CHECK(*ancestors_calculated == setAncestors); + } pool.addUnchecked(entry.FromTx(tx7), setAncestors); BOOST_CHECK_EQUAL(pool.size(), 7U); @@ -260,9 +262,11 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) tx10.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; tx10.vout[0].nValue = 10 * COIN; - ancestors_calculated = pool.CalculateMemPoolAncestors(entry.Fee(200000LL).Time(NodeSeconds{4s}).FromTx(tx10), CTxMemPool::Limits::NoLimits()); - BOOST_REQUIRE(ancestors_calculated); - BOOST_CHECK(*ancestors_calculated == setAncestors); + { + auto ancestors_calculated{pool.CalculateMemPoolAncestors(entry.Fee(200000LL).Time(NodeSeconds{4s}).FromTx(tx10), CTxMemPool::Limits::NoLimits())}; + BOOST_REQUIRE(ancestors_calculated); + BOOST_CHECK(*ancestors_calculated == setAncestors); + } pool.addUnchecked(entry.FromTx(tx10), setAncestors); diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp index ed33969710..19e45c550a 100644 --- a/src/test/rbf_tests.cpp +++ b/src/test/rbf_tests.cpp @@ -426,21 +426,21 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) // Replacement of size 1 { - const auto replace_one{pool.CalculateFeerateDiagramsForRBF(/*replacement_fees=*/0, /*replacement_vsize=*/1, {entry_low}, {entry_low})}; + const auto replace_one{pool.CalculateChunksForRBF(/*replacement_fees=*/0, /*replacement_vsize=*/1, {entry_low}, {entry_low})}; BOOST_CHECK(replace_one.has_value()); - std::vector<FeeFrac> expected_old_diagram{FeeFrac(0, 0), FeeFrac(low_fee, low_size)}; - BOOST_CHECK(replace_one->first == expected_old_diagram); - std::vector<FeeFrac> expected_new_diagram{FeeFrac(0, 0), FeeFrac(0, 1)}; - BOOST_CHECK(replace_one->second == expected_new_diagram); + std::vector<FeeFrac> expected_old_chunks{{low_fee, low_size}}; + BOOST_CHECK(replace_one->first == expected_old_chunks); + std::vector<FeeFrac> expected_new_chunks{{0, 1}}; + BOOST_CHECK(replace_one->second == expected_new_chunks); } // Non-zero replacement fee/size { - const auto replace_one_fee{pool.CalculateFeerateDiagramsForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {entry_low}, {entry_low})}; + const auto replace_one_fee{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {entry_low}, {entry_low})}; BOOST_CHECK(replace_one_fee.has_value()); - std::vector<FeeFrac> expected_old_diagram{FeeFrac(0, 0), FeeFrac(low_fee, low_size)}; + std::vector<FeeFrac> expected_old_diagram{{low_fee, low_size}}; BOOST_CHECK(replace_one_fee->first == expected_old_diagram); - std::vector<FeeFrac> expected_new_diagram{FeeFrac(0, 0), FeeFrac(high_fee, low_size)}; + std::vector<FeeFrac> expected_new_diagram{{high_fee, low_size}}; BOOST_CHECK(replace_one_fee->second == expected_new_diagram); } @@ -451,22 +451,22 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) const auto high_size = entry_high->GetTxSize(); { - const auto replace_single_chunk{pool.CalculateFeerateDiagramsForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {entry_low}, {entry_low, entry_high})}; + const auto replace_single_chunk{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {entry_low}, {entry_low, entry_high})}; BOOST_CHECK(replace_single_chunk.has_value()); - std::vector<FeeFrac> expected_old_diagram{FeeFrac(0, 0), FeeFrac(low_fee + high_fee, low_size + high_size)}; - BOOST_CHECK(replace_single_chunk->first == expected_old_diagram); - std::vector<FeeFrac> expected_new_diagram{FeeFrac(0, 0), FeeFrac(high_fee, low_size)}; - BOOST_CHECK(replace_single_chunk->second == expected_new_diagram); + std::vector<FeeFrac> expected_old_chunks{{low_fee + high_fee, low_size + high_size}}; + BOOST_CHECK(replace_single_chunk->first == expected_old_chunks); + std::vector<FeeFrac> expected_new_chunks{{high_fee, low_size}}; + BOOST_CHECK(replace_single_chunk->second == expected_new_chunks); } // Conflict with the 2nd tx, resulting in new diagram with three entries { - const auto replace_cpfp_child{pool.CalculateFeerateDiagramsForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {entry_high}, {entry_high})}; + const auto replace_cpfp_child{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {entry_high}, {entry_high})}; BOOST_CHECK(replace_cpfp_child.has_value()); - std::vector<FeeFrac> expected_old_diagram{FeeFrac(0, 0), FeeFrac(low_fee + high_fee, low_size + high_size)}; - BOOST_CHECK(replace_cpfp_child->first == expected_old_diagram); - std::vector<FeeFrac> expected_new_diagram{FeeFrac(0, 0), FeeFrac(high_fee, low_size), FeeFrac(low_fee + high_fee, low_size + low_size)}; - BOOST_CHECK(replace_cpfp_child->second == expected_new_diagram); + std::vector<FeeFrac> expected_old_chunks{{low_fee + high_fee, low_size + high_size}}; + BOOST_CHECK(replace_cpfp_child->first == expected_old_chunks); + std::vector<FeeFrac> expected_new_chunks{{high_fee, low_size}, {low_fee, low_size}}; + BOOST_CHECK(replace_cpfp_child->second == expected_new_chunks); } // third transaction causes the topology check to fail @@ -476,7 +476,7 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) const auto normal_size = entry_normal->GetTxSize(); { - const auto replace_too_large{pool.CalculateFeerateDiagramsForRBF(/*replacement_fees=*/normal_fee, /*replacement_vsize=*/normal_size, {entry_low}, {entry_low, entry_high, entry_normal})}; + const auto replace_too_large{pool.CalculateChunksForRBF(/*replacement_fees=*/normal_fee, /*replacement_vsize=*/normal_size, {entry_low}, {entry_low, entry_high, entry_normal})}; BOOST_CHECK(!replace_too_large.has_value()); BOOST_CHECK_EQUAL(util::ErrorString(replace_too_large).original, strprintf("%s has 2 descendants, max 1 allowed", low_tx->GetHash().GetHex())); } @@ -493,12 +493,12 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) const auto low_size_2 = entry_low_2->GetTxSize(); { - const auto replace_two_chunks_single_cluster{pool.CalculateFeerateDiagramsForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {entry_high_2}, {entry_high_2, entry_low_2})}; + const auto replace_two_chunks_single_cluster{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {entry_high_2}, {entry_high_2, entry_low_2})}; BOOST_CHECK(replace_two_chunks_single_cluster.has_value()); - std::vector<FeeFrac> expected_old_diagram{FeeFrac(0, 0), FeeFrac(high_fee, high_size_2), FeeFrac(low_fee + high_fee, low_size_2 + high_size_2)}; - BOOST_CHECK(replace_two_chunks_single_cluster->first == expected_old_diagram); - std::vector<FeeFrac> expected_new_diagram{FeeFrac(0, 0), FeeFrac(high_fee, low_size_2)}; - BOOST_CHECK(replace_two_chunks_single_cluster->second == expected_new_diagram); + std::vector<FeeFrac> expected_old_chunks{{high_fee, high_size_2}, {low_fee, low_size_2}}; + BOOST_CHECK(replace_two_chunks_single_cluster->first == expected_old_chunks); + std::vector<FeeFrac> expected_new_chunks{{high_fee, low_size_2}}; + BOOST_CHECK(replace_two_chunks_single_cluster->second == expected_new_chunks); } // You can have more than two direct conflicts if the there are multiple affected clusters, all of size 2 or less @@ -515,10 +515,10 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) const auto conflict_3_entry = pool.GetIter(conflict_3->GetHash()).value(); { - const auto replace_multiple_clusters{pool.CalculateFeerateDiagramsForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {conflict_1_entry, conflict_2_entry, conflict_3_entry}, {conflict_1_entry, conflict_2_entry, conflict_3_entry})}; + const auto replace_multiple_clusters{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {conflict_1_entry, conflict_2_entry, conflict_3_entry}, {conflict_1_entry, conflict_2_entry, conflict_3_entry})}; BOOST_CHECK(replace_multiple_clusters.has_value()); - BOOST_CHECK(replace_multiple_clusters->first.size() == 4); - BOOST_CHECK(replace_multiple_clusters->second.size() == 2); + BOOST_CHECK(replace_multiple_clusters->first.size() == 3); + BOOST_CHECK(replace_multiple_clusters->second.size() == 1); } // Add a child transaction to conflict_1 and make it cluster size 2, two chunks due to same feerate @@ -527,11 +527,11 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) const auto conflict_1_child_entry = pool.GetIter(conflict_1_child->GetHash()).value(); { - const auto replace_multiple_clusters_2{pool.CalculateFeerateDiagramsForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {conflict_1_entry, conflict_2_entry, conflict_3_entry}, {conflict_1_entry, conflict_2_entry, conflict_3_entry, conflict_1_child_entry})}; + const auto replace_multiple_clusters_2{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {conflict_1_entry, conflict_2_entry, conflict_3_entry}, {conflict_1_entry, conflict_2_entry, conflict_3_entry, conflict_1_child_entry})}; BOOST_CHECK(replace_multiple_clusters_2.has_value()); - BOOST_CHECK(replace_multiple_clusters_2->first.size() == 5); - BOOST_CHECK(replace_multiple_clusters_2->second.size() == 2); + BOOST_CHECK(replace_multiple_clusters_2->first.size() == 4); + BOOST_CHECK(replace_multiple_clusters_2->second.size() == 1); } // Add another descendant to conflict_1, making the cluster size > 2 should fail at this point. @@ -540,86 +540,86 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) const auto conflict_1_grand_child_entry = pool.GetIter(conflict_1_child->GetHash()).value(); { - const auto replace_cluster_size_3{pool.CalculateFeerateDiagramsForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {conflict_1_entry, conflict_2_entry, conflict_3_entry}, {conflict_1_entry, conflict_2_entry, conflict_3_entry, conflict_1_child_entry, conflict_1_grand_child_entry})}; + const auto replace_cluster_size_3{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {conflict_1_entry, conflict_2_entry, conflict_3_entry}, {conflict_1_entry, conflict_2_entry, conflict_3_entry, conflict_1_child_entry, conflict_1_grand_child_entry})}; BOOST_CHECK(!replace_cluster_size_3.has_value()); BOOST_CHECK_EQUAL(util::ErrorString(replace_cluster_size_3).original, strprintf("%s has 2 descendants, max 1 allowed", conflict_1->GetHash().GetHex())); } } -BOOST_AUTO_TEST_CASE(feerate_diagram_utilities) +BOOST_AUTO_TEST_CASE(feerate_chunks_utilities) { - // Sanity check the correctness of the feerate diagram comparison. + // Sanity check the correctness of the feerate chunks comparison. // A strictly better case. - std::vector<FeeFrac> old_diagram{{FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}}; - std::vector<FeeFrac> new_diagram{{FeeFrac{0, 0}, FeeFrac{1000, 300}, FeeFrac{1050, 400}}}; + std::vector<FeeFrac> old_chunks{{{950, 300}, {100, 100}}}; + std::vector<FeeFrac> new_chunks{{{1000, 300}, {50, 100}}}; - BOOST_CHECK(std::is_lt(CompareFeerateDiagram(old_diagram, new_diagram))); - BOOST_CHECK(std::is_gt(CompareFeerateDiagram(new_diagram, old_diagram))); + BOOST_CHECK(std::is_lt(CompareChunks(old_chunks, new_chunks))); + BOOST_CHECK(std::is_gt(CompareChunks(new_chunks, old_chunks))); // Incomparable diagrams - old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; - new_diagram = {FeeFrac{0, 0}, FeeFrac{1000, 300}, FeeFrac{1000, 400}}; + old_chunks = {{950, 300}, {100, 100}}; + new_chunks = {{1000, 300}, {0, 100}}; - BOOST_CHECK(CompareFeerateDiagram(old_diagram, new_diagram) == std::partial_ordering::unordered); - BOOST_CHECK(CompareFeerateDiagram(new_diagram, old_diagram) == std::partial_ordering::unordered); + BOOST_CHECK(CompareChunks(old_chunks, new_chunks) == std::partial_ordering::unordered); + BOOST_CHECK(CompareChunks(new_chunks, old_chunks) == std::partial_ordering::unordered); // Strictly better but smaller size. - old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; - new_diagram = {FeeFrac{0, 0}, FeeFrac{1100, 300}}; + old_chunks = {{950, 300}, {100, 100}}; + new_chunks = {{1100, 300}}; - BOOST_CHECK(std::is_lt(CompareFeerateDiagram(old_diagram, new_diagram))); - BOOST_CHECK(std::is_gt(CompareFeerateDiagram(new_diagram, old_diagram))); + BOOST_CHECK(std::is_lt(CompareChunks(old_chunks, new_chunks))); + BOOST_CHECK(std::is_gt(CompareChunks(new_chunks, old_chunks))); // New diagram is strictly better due to the first chunk, even though // second chunk contributes no fees - old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; - new_diagram = {FeeFrac{0, 0}, FeeFrac{1100, 100}, FeeFrac{1100, 200}}; + old_chunks = {{950, 300}, {100, 100}}; + new_chunks = {{1100, 100}, {0, 100}}; - BOOST_CHECK(std::is_lt(CompareFeerateDiagram(old_diagram, new_diagram))); - BOOST_CHECK(std::is_gt(CompareFeerateDiagram(new_diagram, old_diagram))); + BOOST_CHECK(std::is_lt(CompareChunks(old_chunks, new_chunks))); + BOOST_CHECK(std::is_gt(CompareChunks(new_chunks, old_chunks))); // Feerate of first new chunk is better with, but second chunk is worse - old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; - new_diagram = {FeeFrac{0, 0}, FeeFrac{750, 100}, FeeFrac{999, 350}, FeeFrac{1150, 1000}}; + old_chunks = {{950, 300}, {100, 100}}; + new_chunks = {{750, 100}, {249, 250}, {151, 650}}; - BOOST_CHECK(CompareFeerateDiagram(old_diagram, new_diagram) == std::partial_ordering::unordered); - BOOST_CHECK(CompareFeerateDiagram(new_diagram, old_diagram) == std::partial_ordering::unordered); + BOOST_CHECK(CompareChunks(old_chunks, new_chunks) == std::partial_ordering::unordered); + BOOST_CHECK(CompareChunks(new_chunks, old_chunks) == std::partial_ordering::unordered); // If we make the second chunk slightly better, the new diagram now wins. - old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; - new_diagram = {FeeFrac{0, 0}, FeeFrac{750, 100}, FeeFrac{1000, 350}, FeeFrac{1150, 500}}; + old_chunks = {{950, 300}, {100, 100}}; + new_chunks = {{750, 100}, {250, 250}, {150, 150}}; - BOOST_CHECK(std::is_lt(CompareFeerateDiagram(old_diagram, new_diagram))); - BOOST_CHECK(std::is_gt(CompareFeerateDiagram(new_diagram, old_diagram))); + BOOST_CHECK(std::is_lt(CompareChunks(old_chunks, new_chunks))); + BOOST_CHECK(std::is_gt(CompareChunks(new_chunks, old_chunks))); // Identical diagrams, cannot be strictly better - old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; - new_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; + old_chunks = {{950, 300}, {100, 100}}; + new_chunks = {{950, 300}, {100, 100}}; - BOOST_CHECK(std::is_eq(CompareFeerateDiagram(old_diagram, new_diagram))); - BOOST_CHECK(std::is_eq(CompareFeerateDiagram(new_diagram, old_diagram))); + BOOST_CHECK(std::is_eq(CompareChunks(old_chunks, new_chunks))); + BOOST_CHECK(std::is_eq(CompareChunks(new_chunks, old_chunks))); // Same aggregate fee, but different total size (trigger single tail fee check step) - old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 399}}; - new_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; + old_chunks = {{950, 300}, {100, 99}}; + new_chunks = {{950, 300}, {100, 100}}; // No change in evaluation when tail check needed. - BOOST_CHECK(std::is_gt(CompareFeerateDiagram(old_diagram, new_diagram))); - BOOST_CHECK(std::is_lt(CompareFeerateDiagram(new_diagram, old_diagram))); + BOOST_CHECK(std::is_gt(CompareChunks(old_chunks, new_chunks))); + BOOST_CHECK(std::is_lt(CompareChunks(new_chunks, old_chunks))); // Trigger multiple tail fee check steps - old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 399}}; - new_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}, FeeFrac{1050, 401}, FeeFrac{1050, 402}}; + old_chunks = {{950, 300}, {100, 99}}; + new_chunks = {{950, 300}, {100, 100}, {0, 1}, {0, 1}}; - BOOST_CHECK(std::is_gt(CompareFeerateDiagram(old_diagram, new_diagram))); - BOOST_CHECK(std::is_lt(CompareFeerateDiagram(new_diagram, old_diagram))); + BOOST_CHECK(std::is_gt(CompareChunks(old_chunks, new_chunks))); + BOOST_CHECK(std::is_lt(CompareChunks(new_chunks, old_chunks))); // Multiple tail fee check steps, unordered result - new_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}, FeeFrac{1050, 401}, FeeFrac{1050, 402}, FeeFrac{1051, 403}}; - BOOST_CHECK(CompareFeerateDiagram(old_diagram, new_diagram) == std::partial_ordering::unordered); - BOOST_CHECK(CompareFeerateDiagram(new_diagram, old_diagram) == std::partial_ordering::unordered); + new_chunks = {{950, 300}, {100, 100}, {0, 1}, {0, 1}, {1, 1}}; + BOOST_CHECK(CompareChunks(old_chunks, new_chunks) == std::partial_ordering::unordered); + BOOST_CHECK(CompareChunks(new_chunks, old_chunks) == std::partial_ordering::unordered); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index 0d2460c606..acacb6257d 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -291,6 +291,7 @@ BOOST_AUTO_TEST_CASE(rpc_parse_monetary_values) BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("1e-8")), COIN/100000000); BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.1e-7")), COIN/100000000); BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.01e-6")), COIN/100000000); + BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.00000000000000000000000000000000000001e+30")), 1); BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.0000000000000000000000000000000000000000000000000000000000000000000000000001e+68")), COIN/100000000); BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("10000000000000000000000000000000000000000000000000000000000000000e-64")), COIN); BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000e64")), COIN); @@ -581,4 +582,72 @@ BOOST_AUTO_TEST_CASE(help_example) BOOST_CHECK_NE(HelpExampleRpcNamed("foo", {{"arg", true}}), HelpExampleRpcNamed("foo", {{"arg", "true"}})); } +static void CheckRpc(const std::vector<RPCArg>& params, const UniValue& args, RPCHelpMan::RPCMethodImpl test_impl) +{ + auto null_result{RPCResult{RPCResult::Type::NONE, "", "None"}}; + const RPCHelpMan rpc{"dummy", "dummy description", params, null_result, RPCExamples{""}, test_impl}; + JSONRPCRequest req; + req.params = args; + + rpc.HandleRequest(req); +} + +BOOST_AUTO_TEST_CASE(rpc_arg_helper) +{ + constexpr bool DEFAULT_BOOL = true; + constexpr auto DEFAULT_STRING = "default"; + constexpr uint64_t DEFAULT_UINT64_T = 3; + + //! Parameters with which the RPCHelpMan is instantiated + const std::vector<RPCArg> params{ + // Required arg + {"req_int", RPCArg::Type::NUM, RPCArg::Optional::NO, ""}, + {"req_str", RPCArg::Type::STR, RPCArg::Optional::NO, ""}, + // Default arg + {"def_uint64_t", RPCArg::Type::NUM, RPCArg::Default{DEFAULT_UINT64_T}, ""}, + {"def_string", RPCArg::Type::STR, RPCArg::Default{DEFAULT_STRING}, ""}, + {"def_bool", RPCArg::Type::BOOL, RPCArg::Default{DEFAULT_BOOL}, ""}, + // Optional arg without default + {"opt_double", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, ""}, + {"opt_string", RPCArg::Type::STR, RPCArg::Optional::OMITTED, ""} + }; + + //! Check that `self.Arg` returns the same value as the `request.params` accessors + RPCHelpMan::RPCMethodImpl check_positional = [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { + BOOST_CHECK_EQUAL(self.Arg<int>(0), request.params[0].getInt<int>()); + BOOST_CHECK_EQUAL(self.Arg<std::string>(1), request.params[1].get_str()); + BOOST_CHECK_EQUAL(self.Arg<uint64_t>(2), request.params[2].isNull() ? DEFAULT_UINT64_T : request.params[2].getInt<uint64_t>()); + BOOST_CHECK_EQUAL(self.Arg<std::string>(3), request.params[3].isNull() ? DEFAULT_STRING : request.params[3].get_str()); + BOOST_CHECK_EQUAL(self.Arg<bool>(4), request.params[4].isNull() ? DEFAULT_BOOL : request.params[4].get_bool()); + if (!request.params[5].isNull()) { + BOOST_CHECK_EQUAL(self.MaybeArg<double>(5).value(), request.params[5].get_real()); + } else { + BOOST_CHECK(!self.MaybeArg<double>(5)); + } + if (!request.params[6].isNull()) { + BOOST_CHECK(self.MaybeArg<std::string>(6)); + BOOST_CHECK_EQUAL(*self.MaybeArg<std::string>(6), request.params[6].get_str()); + } else { + BOOST_CHECK(!self.MaybeArg<std::string>(6)); + } + return UniValue{}; + }; + CheckRpc(params, UniValue{JSON(R"([5, "hello", null, null, null, null, null])")}, check_positional); + CheckRpc(params, UniValue{JSON(R"([5, "hello", 4, "test", true, 1.23, "world"])")}, check_positional); + + //! Check that `self.Arg` returns the same value when using index and key + RPCHelpMan::RPCMethodImpl check_named = [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { + BOOST_CHECK_EQUAL(self.Arg<int>(0), self.Arg<int>("req_int")); + BOOST_CHECK_EQUAL(self.Arg<std::string>(1), self.Arg<std::string>("req_str")); + BOOST_CHECK_EQUAL(self.Arg<uint64_t>(2), self.Arg<uint64_t>("def_uint64_t")); + BOOST_CHECK_EQUAL(self.Arg<std::string>(3), self.Arg<std::string>("def_string")); + BOOST_CHECK_EQUAL(self.Arg<bool>(4), self.Arg<bool>("def_bool")); + BOOST_CHECK(self.MaybeArg<double>(5) == self.MaybeArg<double>("opt_double")); + BOOST_CHECK(self.MaybeArg<std::string>(6) == self.MaybeArg<std::string>("opt_string")); + return UniValue{}; + }; + CheckRpc(params, UniValue{JSON(R"([5, "hello", null, null, null, null, null])")}, check_named); + CheckRpc(params, UniValue{JSON(R"([5, "hello", 4, "test", true, 1.23, "world"])")}, check_named); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp index 0af2fdce08..e4142e203c 100644 --- a/src/test/script_tests.cpp +++ b/src/test/script_tests.cpp @@ -1254,6 +1254,30 @@ BOOST_AUTO_TEST_CASE(script_combineSigs) BOOST_CHECK(combined.scriptSig == partial3c); } +/** + * Reproduction of an exception incorrectly raised when parsing a public key inside a TapMiniscript. + */ +BOOST_AUTO_TEST_CASE(sign_invalid_miniscript) +{ + FillableSigningProvider keystore; + SignatureData sig_data; + CMutableTransaction prev, curr; + + // Create a Taproot output which contains a leaf in which a non-32 bytes push is used where a public key is expected + // by the Miniscript parser. This offending Script was found by the RPC fuzzer. + const auto invalid_pubkey{ParseHex("173d36c8c9c9c9ffffffffffff0200000000021e1e37373721361818181818181e1e1e1e19000000000000000000b19292929292926b006c9b9b9292")}; + TaprootBuilder builder; + builder.Add(0, {invalid_pubkey}, 0xc0); + XOnlyPubKey nums{ParseHex("50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0")}; + builder.Finalize(nums); + prev.vout.emplace_back(0, GetScriptForDestination(builder.GetOutput())); + curr.vin.emplace_back(COutPoint{prev.GetHash(), 0}); + sig_data.tr_spenddata = builder.GetSpendData(); + + // SignSignature can fail but it shouldn't raise an exception (nor crash). + BOOST_CHECK(!SignSignature(keystore, CTransaction(prev), curr, 0, SIGHASH_ALL, sig_data)); +} + BOOST_AUTO_TEST_CASE(script_standard_push) { ScriptError err; diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp index 8aab2b565c..2de147deea 100644 --- a/src/test/system_tests.cpp +++ b/src/test/system_tests.cpp @@ -11,7 +11,7 @@ #include <univalue.h> #ifdef ENABLE_EXTERNAL_SIGNER -#include <util/subprocess.hpp> +#include <util/subprocess.h> #endif // ENABLE_EXTERNAL_SIGNER #include <boost/test/unit_test.hpp> diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2c18184261..38350b33cc 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -14,7 +14,6 @@ #include <banman.h> #include <chainparams.h> #include <common/system.h> -#include <common/url.h> #include <consensus/consensus.h> #include <consensus/params.h> #include <consensus/validation.h> @@ -81,7 +80,6 @@ using node::RegenerateCommitments; using node::VerifyLoadedChainstate; const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr; -UrlDecodeFn* const URL_DECODE = nullptr; /** Random context to get unique temp data dirs. Separate from g_insecure_rand_ctx, which can be seeded from a const env var */ static FastRandomContext g_insecure_rand_ctx_temp_path; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 82eec6241f..06066e38b2 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1280,7 +1280,7 @@ std::optional<std::string> CTxMemPool::CheckConflictTopology(const setEntries& d return std::nullopt; } -util::Result<std::pair<std::vector<FeeFrac>, std::vector<FeeFrac>>> CTxMemPool::CalculateFeerateDiagramsForRBF(CAmount replacement_fees, int64_t replacement_vsize, const setEntries& direct_conflicts, const setEntries& all_conflicts) +util::Result<std::pair<std::vector<FeeFrac>, std::vector<FeeFrac>>> CTxMemPool::CalculateChunksForRBF(CAmount replacement_fees, int64_t replacement_vsize, const setEntries& direct_conflicts, const setEntries& all_conflicts) { Assume(replacement_vsize > 0); @@ -1335,7 +1335,6 @@ util::Result<std::pair<std::vector<FeeFrac>, std::vector<FeeFrac>>> CTxMemPool:: // No topology restrictions post-chunking; sort std::sort(old_chunks.begin(), old_chunks.end(), std::greater()); - std::vector<FeeFrac> old_diagram = BuildDiagramFromChunks(old_chunks); std::vector<FeeFrac> new_chunks; @@ -1365,6 +1364,5 @@ util::Result<std::pair<std::vector<FeeFrac>, std::vector<FeeFrac>>> CTxMemPool:: // No topology restrictions post-chunking; sort std::sort(new_chunks.begin(), new_chunks.end(), std::greater()); - std::vector<FeeFrac> new_diagram = BuildDiagramFromChunks(new_chunks); - return std::make_pair(old_diagram, new_diagram); + return std::make_pair(old_chunks, new_chunks); } diff --git a/src/txmempool.h b/src/txmempool.h index 9dd4d56da7..52a3dc2d7d 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -738,7 +738,7 @@ public: } /** - * Calculate the old and new mempool feerate diagrams relating to the + * Calculate the sorted chunks for the old and new mempool relating to the * clusters that would be affected by a potential replacement transaction. * (replacement_fees, replacement_vsize) values are gathered from a * proposed set of replacement transactions that are considered as a single @@ -752,7 +752,7 @@ public: * @param[in] all_conflicts All transactions that would be removed * @return old and new diagram pair respectively, or an error string if the conflicts don't match a calculable topology */ - util::Result<std::pair<std::vector<FeeFrac>, std::vector<FeeFrac>>> CalculateFeerateDiagramsForRBF(CAmount replacement_fees, int64_t replacement_vsize, const setEntries& direct_conflicts, const setEntries& all_conflicts) EXCLUSIVE_LOCKS_REQUIRED(cs); + util::Result<std::pair<std::vector<FeeFrac>, std::vector<FeeFrac>>> CalculateChunksForRBF(CAmount replacement_fees, int64_t replacement_vsize, const setEntries& direct_conflicts, const setEntries& all_conflicts) EXCLUSIVE_LOCKS_REQUIRED(cs); /* Check that all direct conflicts are in a cluster size of two or less. Each * direct conflict may be in a separate cluster. diff --git a/src/univalue/test/object.cpp b/src/univalue/test/object.cpp index 8b90448b36..1c724555f3 100644 --- a/src/univalue/test/object.cpp +++ b/src/univalue/test/object.cpp @@ -421,7 +421,7 @@ void univalue_readwrite() // Valid, with leading or trailing whitespace BOOST_CHECK(v.read(" 1.0") && (v.get_real() == 1.0)); BOOST_CHECK(v.read("1.0 ") && (v.get_real() == 1.0)); - BOOST_CHECK(v.read("0.00000000000000000000000000000000000001e+30 ") && v.get_real() == 1e-8); + BOOST_CHECK(v.read("0.00000000000000000000000000000000000001e+30 ")); BOOST_CHECK(!v.read(".19e-6")); //should fail, missing leading 0, therefore invalid JSON // Invalid, initial garbage diff --git a/src/util/feefrac.cpp b/src/util/feefrac.cpp index 68fb836936..5b6173835c 100644 --- a/src/util/feefrac.cpp +++ b/src/util/feefrac.cpp @@ -7,39 +7,26 @@ #include <array> #include <vector> -std::vector<FeeFrac> BuildDiagramFromChunks(const Span<const FeeFrac> chunks) -{ - std::vector<FeeFrac> diagram; - diagram.reserve(chunks.size() + 1); - - diagram.emplace_back(0, 0); - for (auto& chunk : chunks) { - diagram.emplace_back(diagram.back() + chunk); - } - return diagram; -} - -std::partial_ordering CompareFeerateDiagram(Span<const FeeFrac> dia0, Span<const FeeFrac> dia1) +std::partial_ordering CompareChunks(Span<const FeeFrac> chunks0, Span<const FeeFrac> chunks1) { /** Array to allow indexed access to input diagrams. */ - const std::array<Span<const FeeFrac>, 2> dias = {dia0, dia1}; + const std::array<Span<const FeeFrac>, 2> chunk = {chunks0, chunks1}; /** How many elements we have processed in each input. */ - size_t next_index[2] = {1, 1}; + size_t next_index[2] = {0, 0}; + /** Accumulated fee/sizes in diagrams, up to next_index[i] - 1. */ + FeeFrac accum[2]; /** Whether the corresponding input is strictly better than the other at least in one place. */ bool better_somewhere[2] = {false, false}; /** Get the first unprocessed point in diagram number dia. */ - const auto next_point = [&](int dia) { return dias[dia][next_index[dia]]; }; + const auto next_point = [&](int dia) { return chunk[dia][next_index[dia]] + accum[dia]; }; /** Get the last processed point in diagram number dia. */ - const auto prev_point = [&](int dia) { return dias[dia][next_index[dia] - 1]; }; - - // Diagrams should be non-empty, and first elements zero in size and fee - Assert(!dia0.empty() && !dia1.empty()); - Assert(prev_point(0).IsEmpty()); - Assert(prev_point(1).IsEmpty()); + const auto prev_point = [&](int dia) { return accum[dia]; }; + /** Move to the next point in diagram number dia. */ + const auto advance = [&](int dia) { accum[dia] += chunk[dia][next_index[dia]++]; }; do { - bool done_0 = next_index[0] == dias[0].size(); - bool done_1 = next_index[1] == dias[1].size(); + bool done_0 = next_index[0] == chunk[0].size(); + bool done_1 = next_index[1] == chunk[1].size(); if (done_0 && done_1) break; // Determine which diagram has the first unprocessed point. If a single side is finished, use the @@ -69,17 +56,16 @@ std::partial_ordering CompareFeerateDiagram(Span<const FeeFrac> dia0, Span<const // If B and P have the same size, B can be marked as processed (in addition to P, see // below), as we've already performed a comparison at this size. - if (point_b.size == point_p.size) ++next_index[!unproc_side]; + if (point_b.size == point_p.size) advance(!unproc_side); } // If P lies above AB, unproc_side is better in P. If P lies below AB, then !unproc_side is // better in P. if (std::is_gt(cmp)) better_somewhere[unproc_side] = true; if (std::is_lt(cmp)) better_somewhere[!unproc_side] = true; - ++next_index[unproc_side]; + advance(unproc_side); // If both diagrams are better somewhere, they are incomparable. if (better_somewhere[0] && better_somewhere[1]) return std::partial_ordering::unordered; - } while(true); // Otherwise compare the better_somewhere values. diff --git a/src/util/feefrac.h b/src/util/feefrac.h index 7102f85f88..9772162010 100644 --- a/src/util/feefrac.h +++ b/src/util/feefrac.h @@ -146,15 +146,14 @@ struct FeeFrac } }; -/** Takes the pre-computed and topologically-valid chunks and generates a fee diagram which starts at FeeFrac of (0, 0) */ -std::vector<FeeFrac> BuildDiagramFromChunks(Span<const FeeFrac> chunks); - -/** Compares two feerate diagrams. The shorter one is implicitly - * extended with a horizontal straight line. +/** Compare the feerate diagrams implied by the provided sorted chunks data. + * + * The implied diagram for each starts at (0, 0), then contains for each chunk the cumulative fee + * and size up to that chunk, and then extends infinitely to the right with a horizontal line. * - * A feerate diagram consists of a list of (fee, size) points with the property that size - * is strictly increasing and that the first entry is (0, 0). + * The caller must guarantee that the sum of the FeeFracs in either of the chunks' data set do not + * overflow (so sum fees < 2^63, and sum sizes < 2^31). */ -std::partial_ordering CompareFeerateDiagram(Span<const FeeFrac> dia0, Span<const FeeFrac> dia1); +std::partial_ordering CompareChunks(Span<const FeeFrac> chunks0, Span<const FeeFrac> chunks1); #endif // BITCOIN_UTIL_FEEFRAC_H diff --git a/src/util/result.h b/src/util/result.h index b99995c7e5..122a7638fa 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -39,6 +39,16 @@ private: std::variant<bilingual_str, T> m_variant; + //! Disallow copy constructor, require Result to be moved for efficiency. + Result(const Result&) = delete; + + //! Disallow operator= to avoid confusion in the future when the Result + //! class gains support for richer error reporting, and callers should have + //! ability to set a new result value without clearing existing error + //! messages. + Result& operator=(const Result&) = delete; + Result& operator=(Result&&) = delete; + template <typename FT> friend bilingual_str ErrorString(const Result<FT>& result); @@ -46,6 +56,8 @@ public: Result() : m_variant{std::in_place_index_t<1>{}, std::monostate{}} {} // constructor for void Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {} Result(Error error) : m_variant{std::in_place_index_t<0>{}, std::move(error.message)} {} + Result(Result&&) = default; + ~Result() = default; //! std::optional methods, so functions returning optional<T> can change to //! return Result<T> with minimal changes to existing code, and vice versa. diff --git a/src/util/subprocess.hpp b/src/util/subprocess.h index dd53a8fbb4..4acfa8ff83 100644 --- a/src/util/subprocess.hpp +++ b/src/util/subprocess.h @@ -1,6 +1,8 @@ +// Based on the https://github.com/arun11299/cpp-subprocess project. + /*! -Documentation for C++ subprocessing libraray. +Documentation for C++ subprocessing library. @copyright The code is licensed under the [MIT License](http://opensource.org/licenses/MIT): @@ -31,8 +33,10 @@ Documentation for C++ subprocessing libraray. @version 1.0.0 */ -#ifndef SUBPROCESS_HPP -#define SUBPROCESS_HPP +#ifndef BITCOIN_UTIL_SUBPROCESS_H +#define BITCOIN_UTIL_SUBPROCESS_H + +#include <util/syserror.h> #include <algorithm> #include <cassert> @@ -106,7 +110,7 @@ namespace subprocess { // from pipe static const size_t SP_MAX_ERR_BUF_SIZ = 1024; -// Default buffer capcity for OutBuffer and ErrBuffer. +// Default buffer capacity for OutBuffer and ErrBuffer. // If the data exceeds this capacity, the buffer size is grown // by 1.5 times its previous capacity static const size_t DEFAULT_BUF_CAP_BYTES = 8192; @@ -148,24 +152,11 @@ class OSError: public std::runtime_error { public: OSError(const std::string& err_msg, int err_code): - std::runtime_error( err_msg + ": " + std::strerror(err_code) ) + std::runtime_error(err_msg + ": " + SysErrorString(err_code)) {} }; //-------------------------------------------------------------------- - -//Environment Variable types -#ifndef _MSC_VER - using env_string_t = std::string; - using env_char_t = char; -#else - using env_string_t = std::wstring; - using env_char_t = wchar_t; -#endif -using env_map_t = std::map<env_string_t, env_string_t>; -using env_vector_t = std::vector<env_char_t>; - -//-------------------------------------------------------------------- namespace util { template <typename R> @@ -303,100 +294,6 @@ namespace util if (!SetHandleInformation(*child_handle, HANDLE_FLAG_INHERIT, 0)) throw OSError("SetHandleInformation", 0); } - - // env_map_t MapFromWindowsEnvironment() - // * Imports current Environment in a C-string table - // * Parses the strings by splitting on the first "=" per line - // * Creates a map of the variables - // * Returns the map - inline env_map_t MapFromWindowsEnvironment(){ - wchar_t *variable_strings_ptr; - wchar_t *environment_strings_ptr; - std::wstring delimeter(L"="); - int del_len = delimeter.length(); - env_map_t mapped_environment; - - // Get a pointer to the environment block. - environment_strings_ptr = GetEnvironmentStringsW(); - // If the returned pointer is NULL, exit. - if (environment_strings_ptr == NULL) - { - throw OSError("GetEnvironmentStringsW", 0); - } - - // Variable strings are separated by NULL byte, and the block is - // terminated by a NULL byte. - - variable_strings_ptr = (wchar_t *) environment_strings_ptr; - - //Since the environment map ends with a null, we can loop until we find it. - while (*variable_strings_ptr) - { - // Create a string from Variable String - env_string_t current_line(variable_strings_ptr); - // Find the first "equals" sign. - auto pos = current_line.find(delimeter); - // Assuming it's not missing ... - if(pos!=std::wstring::npos){ - // ... parse the key and value. - env_string_t key = current_line.substr(0, pos); - env_string_t value = current_line.substr(pos + del_len); - // Map the entry. - mapped_environment[key] = value; - } - // Jump to next line in the environment map. - variable_strings_ptr += std::wcslen(variable_strings_ptr) + 1; - } - // We're done with the old environment map buffer. - FreeEnvironmentStringsW(environment_strings_ptr); - - // Return the map. - return mapped_environment; - } - - // env_vector_t WindowsEnvironmentVectorFromMap(const env_map_t &source_map) - // * Creates a vector buffer for the new environment string table - // * Copies in the mapped variables - // * Returns the vector - inline env_vector_t WindowsEnvironmentVectorFromMap(const env_map_t &source_map) - { - // Make a new environment map buffer. - env_vector_t environment_map_buffer; - // Give it some space. - environment_map_buffer.reserve(4096); - - // And fill'er up. - for(auto kv: source_map){ - // Create the line - env_string_t current_line(kv.first); current_line += L"="; current_line += kv.second; - // Add the line to the buffer. - std::copy(current_line.begin(), current_line.end(), std::back_inserter(environment_map_buffer)); - // Append a null - environment_map_buffer.push_back(0); - } - // Append one last null because of how Windows does it's environment maps. - environment_map_buffer.push_back(0); - - return environment_map_buffer; - } - - // env_vector_t CreateUpdatedWindowsEnvironmentVector(const env_map_t &changes_map) - // * Merges host environment with new mapped variables - // * Creates and returns string vector based on map - inline env_vector_t CreateUpdatedWindowsEnvironmentVector(const env_map_t &changes_map){ - // Import the environment map - env_map_t environment_map = MapFromWindowsEnvironment(); - // Merge in the changes with overwrite - for(auto& it: changes_map) - { - environment_map[it.first] = it.second; - } - // Create a Windows-usable Environment Map Buffer - env_vector_t environment_map_strings_vector = WindowsEnvironmentVectorFromMap(environment_map); - - return environment_map_strings_vector; - } - #endif /*! @@ -429,26 +326,6 @@ namespace util } - /*! - * Function: join - * Parameters: - * [in] vec : Vector of strings which needs to be joined to form - * a single string with words seperated by a seperator char. - * [in] sep : String used to seperate 2 words in the joined string. - * Default constructed to ' ' (space). - * [out] string: Joined string. - */ - static inline - std::string join(const std::vector<std::string>& vec, - const std::string& sep = " ") - { - std::string res; - for (auto& elem : vec) res.append(elem + sep); - res.erase(--res.end()); - return res; - } - - #ifndef __USING_WINDOWS__ /*! * Function: set_clo_on_exec @@ -650,56 +527,6 @@ namespace util */ /*! - * The buffer size of the stdin/stdout/stderr - * streams of the child process. - * Default value is 0. - */ -struct bufsize { - explicit bufsize(int siz): bufsiz(siz) {} - int bufsiz = 0; -}; - -/*! - * Option to defer spawning of the child process - * till `Popen::start_process` API is called. - * Default value is false. - */ -struct defer_spawn { - explicit defer_spawn(bool d): defer(d) {} - bool defer = false; -}; - -/*! - * Option to close all file descriptors - * when the child process is spawned. - * The close fd list does not include - * input/output/error if they are explicitly - * set as part of the Popen arguments. - * - * Default value is false. - */ -struct close_fds { - explicit close_fds(bool c): close_all(c) {} - bool close_all = false; -}; - -/*! - * Option to make the child process as the - * session leader and thus the process - * group leader. - * Default value is false. - */ -struct session_leader { - explicit session_leader(bool sl): leader_(sl) {} - bool leader_ = false; -}; - -struct shell { - explicit shell(bool s): shell_(s) {} - bool shell_ = false; -}; - -/*! * Base class for all arguments involving string value. */ struct string_arg @@ -711,7 +538,7 @@ struct string_arg }; /*! - * Option to specify the executable name seperately + * Option to specify the executable name separately * from the args sequence. * In this case the cmd args must only contain the * options required for this executable. @@ -725,34 +552,6 @@ struct executable: string_arg }; /*! - * Option to set the current working directory - * of the spawned process. - * - * Eg: cwd{"/som/path/x"} - */ -struct cwd: string_arg -{ - template <typename T> - cwd(T&& arg): string_arg(std::forward<T>(arg)) {} -}; - -/*! - * Option to specify environment variables required by - * the spawned process. - * - * Eg: environment{{ {"K1", "V1"}, {"K2", "V2"},... }} - */ -struct environment -{ - environment(env_map_t&& env): - env_(std::move(env)) {} - explicit environment(const env_map_t& env): - env_(env) {} - env_map_t env_; -}; - - -/*! * Used for redirecting input/output/error */ enum IOTYPE { @@ -852,7 +651,7 @@ struct error wr_ch_ = fd; } explicit error(IOTYPE typ) { - assert ((typ == PIPE || typ == STDOUT) && "STDERR not aloowed"); + assert ((typ == PIPE || typ == STDOUT) && "STDERR not allowed"); if (typ == PIPE) { #ifndef __USING_WINDOWS__ std::tie(rd_ch_, wr_ch_) = util::pipe_cloexec(); @@ -868,44 +667,6 @@ struct error int wr_ch_ = -1; }; -// Impoverished, meager, needy, truly needy -// version of type erasure to store function pointers -// needed to provide the functionality of preexec_func -// ATTN: Can be used only to execute functions with no -// arguments and returning void. -// Could have used more efficient methods, ofcourse, but -// that wont yield me the consistent syntax which I am -// aiming for. If you know, then please do let me know. - -class preexec_func -{ -public: - preexec_func() {} - - template <typename Func> - explicit preexec_func(Func f): holder_(new FuncHolder<Func>(std::move(f))) - {} - - void operator()() { - (*holder_)(); - } - -private: - struct HolderBase { - virtual void operator()() const = 0; - virtual ~HolderBase(){}; - }; - template <typename T> - struct FuncHolder: HolderBase { - FuncHolder(T func): func_(std::move(func)) {} - void operator()() const override { func_(); } - // The function pointer/reference - T func_; - }; - - std::unique_ptr<HolderBase> holder_ = nullptr; -}; - // ~~~~ End Popen Args ~~~~ @@ -967,8 +728,8 @@ namespace detail { // Metaprogram for searching a type within // a variadic parameter pack // This is particularly required to do a compile time -// checking of the arguments provided to 'check_ouput' function -// wherein the user is not expected to provide an 'ouput' option. +// checking of the arguments provided to 'check_output' function +// wherein the user is not expected to provide an 'output' option. template <typename... T> struct param_pack{}; @@ -995,7 +756,7 @@ struct has_type<F, param_pack<H,T...>> { /*! * A helper class to Popen class for setting * options as provided in the Popen constructor - * or in check_ouput arguments. + * or in check_output arguments. * This design allows us to _not_ have any fixed position * to any arguments and specify them in a way similar to what * can be done in python. @@ -1005,17 +766,9 @@ struct ArgumentDeducer ArgumentDeducer(Popen* p): popen_(p) {} void set_option(executable&& exe); - void set_option(cwd&& cwdir); - void set_option(bufsize&& bsiz); - void set_option(environment&& env); - void set_option(defer_spawn&& defer); - void set_option(shell&& sh); void set_option(input&& inp); void set_option(output&& out); void set_option(error&& err); - void set_option(close_fds&& cfds); - void set_option(preexec_func&& prefunc); - void set_option(session_leader&& sleader); private: Popen* popen_ = nullptr; @@ -1166,9 +919,6 @@ public:// Yes they are public HANDLE g_hChildStd_ERR_Wr = nullptr; #endif - // Buffer size for the IO streams - int bufsiz_ = 0; - // Pipes for communicating with child // Emulates stdin @@ -1198,9 +948,9 @@ private: * interface to the client. * * API's provided by the class: - * 1. Popen({"cmd"}, output{..}, error{..}, cwd{..}, ....) + * 1. Popen({"cmd"}, output{..}, error{..}, ....) * Command provided as a sequence. - * 2. Popen("cmd arg1"m output{..}, error{..}, cwd{..}, ....) + * 2. Popen("cmd arg1"m output{..}, error{..}, ....) * Command provided in a single string. * 3. wait() - Wait for the child to exit. * 4. retcode() - The return code of the exited child. @@ -1211,13 +961,11 @@ private: * 9. communicate(...) - Get the output/error from the child and close the channels * from the parent side. *10. input() - Get the input channel/File pointer. Can be used for - * cutomizing the way of sending input to child. + * customizing the way of sending input to child. *11. output() - Get the output channel/File pointer. Usually used in case of redirection. See piping examples. - *12. error() - Get the error channel/File poiner. Usually used + *12. error() - Get the error channel/File pointer. Usually used in case of redirection. - *13. start_process() - Start the child process. Only to be used when - * `defer_spawn` option was provided in Popen constructor. */ class Popen { @@ -1235,7 +983,7 @@ public: // Setup the communication channels of the Popen class stream_.setup_comm_channels(); - if (!defer_process_start_) execute_process(); + execute_process(); } template <typename... Args> @@ -1247,7 +995,7 @@ public: // Setup the communication channels of the Popen class stream_.setup_comm_channels(); - if (!defer_process_start_) execute_process(); + execute_process(); } template <typename... Args> @@ -1258,7 +1006,7 @@ public: // Setup the communication channels of the Popen class stream_.setup_comm_channels(); - if (!defer_process_start_) execute_process(); + execute_process(); } /* @@ -1270,8 +1018,6 @@ public: } */ - void start_process() noexcept(false); - int pid() const noexcept { return child_pid_; } int retcode() const noexcept { return retcode_; } @@ -1345,20 +1091,11 @@ private: std::future<void> cleanup_future_; #endif - bool defer_process_start_ = false; - bool close_fds_ = false; - bool has_preexec_fn_ = false; - bool shell_ = false; - bool session_leader_ = false; - std::string exe_name_; - std::string cwd_; - env_map_t env_; - preexec_func preexec_fn_; // Command in string format std::string args_; - // Comamnd provided as sequence + // Command provided as sequence std::vector<std::string> vargs_; std::vector<char*> cargv_; @@ -1389,20 +1126,6 @@ inline void Popen::populate_c_argv() cargv_.push_back(nullptr); } -inline void Popen::start_process() noexcept(false) -{ - // The process was started/tried to be started - // in the constructor itself. - // For explicitly calling this API to start the - // process, 'defer_spawn' argument must be set to - // true in the constructor. - if (!defer_process_start_) { - assert (0); - return; - } - execute_process(); -} - inline int Popen::wait() noexcept(false) { #ifdef __USING_WINDOWS__ @@ -1481,8 +1204,7 @@ inline void Popen::kill(int sig_num) throw OSError("TerminateProcess", 0); } #else - if (session_leader_) killpg(child_pid_, sig_num); - else ::kill(child_pid_, sig_num); + ::kill(child_pid_, sig_num); #endif } @@ -1490,17 +1212,6 @@ inline void Popen::kill(int sig_num) inline void Popen::execute_process() noexcept(false) { #ifdef __USING_WINDOWS__ - if (this->shell_) { - throw OSError("shell not currently supported on windows", 0); - } - - void* environment_string_table_ptr = nullptr; - env_vector_t environment_string_vector; - if(this->env_.size()){ - environment_string_vector = util::CreateUpdatedWindowsEnvironmentVector(env_); - environment_string_table_ptr = (void*)environment_string_vector.data(); - } - if (exe_name_.length()) { this->vargs_.insert(this->vargs_.begin(), this->exe_name_); this->populate_c_argv(); @@ -1547,7 +1258,7 @@ inline void Popen::execute_process() noexcept(false) NULL, // primary thread security attributes TRUE, // handles are inherited creation_flags, // creation flags - environment_string_table_ptr, // use provided environment + NULL, // use parent's environment NULL, // use parent's current directory &siStartInfo, // STARTUPINFOW pointer &piProcInfo); // receives PROCESS_INFORMATION @@ -1586,14 +1297,6 @@ inline void Popen::execute_process() noexcept(false) int err_rd_pipe, err_wr_pipe; std::tie(err_rd_pipe, err_wr_pipe) = util::pipe_cloexec(); - if (shell_) { - auto new_cmd = util::join(vargs_); - vargs_.clear(); - vargs_.insert(vargs_.begin(), {"/bin/sh", "-c"}); - vargs_.push_back(new_cmd); - populate_c_argv(); - } - if (exe_name_.length()) { vargs_.insert(vargs_.begin(), exe_name_); populate_c_argv(); @@ -1660,30 +1363,6 @@ namespace detail { popen_->exe_name_ = std::move(exe.arg_value); } - inline void ArgumentDeducer::set_option(cwd&& cwdir) { - popen_->cwd_ = std::move(cwdir.arg_value); - } - - inline void ArgumentDeducer::set_option(bufsize&& bsiz) { - popen_->stream_.bufsiz_ = bsiz.bufsiz; - } - - inline void ArgumentDeducer::set_option(environment&& env) { - popen_->env_ = std::move(env.env_); - } - - inline void ArgumentDeducer::set_option(defer_spawn&& defer) { - popen_->defer_process_start_ = defer.defer; - } - - inline void ArgumentDeducer::set_option(shell&& sh) { - popen_->shell_ = sh.shell_; - } - - inline void ArgumentDeducer::set_option(session_leader&& sleader) { - popen_->session_leader_ = sleader.leader_; - } - inline void ArgumentDeducer::set_option(input&& inp) { if (inp.rd_ch_ != -1) popen_->stream_.read_from_parent_ = inp.rd_ch_; if (inp.wr_ch_ != -1) popen_->stream_.write_to_child_ = inp.wr_ch_; @@ -1706,15 +1385,6 @@ namespace detail { if (err.rd_ch_ != -1) popen_->stream_.err_read_ = err.rd_ch_; } - inline void ArgumentDeducer::set_option(close_fds&& cfds) { - popen_->close_fds_ = cfds.close_all; - } - - inline void ArgumentDeducer::set_option(preexec_func&& prefunc) { - popen_->preexec_fn_ = std::move(prefunc); - popen_->has_preexec_fn_ = true; - } - inline void Child::execute_child() { #ifndef __USING_WINDOWS__ @@ -1761,41 +1431,8 @@ namespace detail { if (stream.err_write_ != -1 && stream.err_write_ > 2) close(stream.err_write_); - // Close all the inherited fd's except the error write pipe - if (parent_->close_fds_) { - int max_fd = sysconf(_SC_OPEN_MAX); - if (max_fd == -1) throw OSError("sysconf failed", errno); - - for (int i = 3; i < max_fd; i++) { - if (i == err_wr_pipe_) continue; - close(i); - } - } - - // Change the working directory if provided - if (parent_->cwd_.length()) { - sys_ret = chdir(parent_->cwd_.c_str()); - if (sys_ret == -1) throw OSError("chdir failed", errno); - } - - if (parent_->has_preexec_fn_) { - parent_->preexec_fn_(); - } - - if (parent_->session_leader_) { - sys_ret = setsid(); - if (sys_ret == -1) throw OSError("setsid failed", errno); - } - // Replace the current image with the executable - if (parent_->env_.size()) { - for (auto& kv : parent_->env_) { - setenv(kv.first.c_str(), kv.second.c_str(), 1); - } - sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data()); - } else { - sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data()); - } + sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data()); if (sys_ret == -1) throw OSError("execve failed", errno); @@ -1838,16 +1475,7 @@ namespace detail { for (auto& h : handles) { if (h == nullptr) continue; - switch (bufsiz_) { - case 0: - setvbuf(h, nullptr, _IONBF, BUFSIZ); - break; - case 1: - setvbuf(h, nullptr, _IONBF, BUFSIZ); - break; - default: - setvbuf(h, nullptr, _IOFBF, bufsiz_); - }; + setvbuf(h, nullptr, _IONBF, BUFSIZ); } #endif } @@ -1983,4 +1611,4 @@ namespace detail { } -#endif // SUBPROCESS_HPP +#endif // BITCOIN_UTIL_SUBPROCESS_H diff --git a/src/util/time.cpp b/src/util/time.cpp index 456662bd84..f08eb5300a 100644 --- a/src/util/time.cpp +++ b/src/util/time.cpp @@ -16,11 +16,11 @@ void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); } -static std::atomic<int64_t> nMockTime(0); //!< For testing +static std::atomic<std::chrono::seconds> g_mock_time{}; //!< For testing NodeClock::time_point NodeClock::now() noexcept { - const std::chrono::seconds mocktime{nMockTime.load(std::memory_order_relaxed)}; + const auto mocktime{g_mock_time.load(std::memory_order_relaxed)}; const auto ret{ mocktime.count() ? mocktime : @@ -29,20 +29,16 @@ NodeClock::time_point NodeClock::now() noexcept return time_point{ret}; }; -void SetMockTime(int64_t nMockTimeIn) -{ - Assert(nMockTimeIn >= 0); - nMockTime.store(nMockTimeIn, std::memory_order_relaxed); -} - +void SetMockTime(int64_t nMockTimeIn) { SetMockTime(std::chrono::seconds{nMockTimeIn}); } void SetMockTime(std::chrono::seconds mock_time_in) { - nMockTime.store(mock_time_in.count(), std::memory_order_relaxed); + Assert(mock_time_in >= 0s); + g_mock_time.store(mock_time_in, std::memory_order_relaxed); } std::chrono::seconds GetMockTime() { - return std::chrono::seconds(nMockTime.load(std::memory_order_relaxed)); + return g_mock_time.load(std::memory_order_relaxed); } int64_t GetTime() { return GetTime<std::chrono::seconds>().count(); } diff --git a/src/validation.cpp b/src/validation.cpp index 048c97529a..f57851b4f7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -946,8 +946,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) maybe_rbf_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants(); } - auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, maybe_rbf_limits)}; - if (!ancestors) { + if (auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, maybe_rbf_limits)}) { + ws.m_ancestors = std::move(*ancestors); + } else { // If CalculateMemPoolAncestors fails second time, we want the original error string. // Contracting/payment channels CPFP carve-out: // If the new transaction is relatively small (up to 40k weight) @@ -970,11 +971,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); } - ancestors = m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits); - if (!ancestors) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); + if (auto ancestors_retry{m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits)}) { + ws.m_ancestors = std::move(*ancestors_retry); + } else { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); + } } - ws.m_ancestors = *ancestors; // Even though just checking direct mempool parents for inheritance would be sufficient, we // check using the full ancestor set here because it's more convenient to use what we have // already calculated. @@ -6142,13 +6145,14 @@ bool ChainstateManager::DeleteSnapshotChainstate() Assert(m_snapshot_chainstate); Assert(m_ibd_chainstate); - fs::path snapshot_datadir = GetSnapshotCoinsDBPath(*m_snapshot_chainstate); + fs::path snapshot_datadir = Assert(node::FindSnapshotChainstateDir(m_options.datadir)).value(); if (!DeleteCoinsDBFromDisk(snapshot_datadir, /*is_snapshot=*/ true)) { LogPrintf("Deletion of %s failed. Please remove it manually to continue reindexing.\n", fs::PathToString(snapshot_datadir)); return false; } m_active_chainstate = m_ibd_chainstate.get(); + m_active_chainstate->m_mempool = m_snapshot_chainstate->m_mempool; m_snapshot_chainstate.reset(); return true; } diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index a71f8f9fbc..b5703fa54a 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -9,9 +9,11 @@ #include <wallet/external_signer_scriptpubkeyman.h> #include <iostream> +#include <key_io.h> #include <memory> #include <stdexcept> #include <string> +#include <univalue.h> #include <utility> #include <vector> @@ -51,15 +53,26 @@ ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() { return signers[0]; } -bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const +util::Result<void> ExternalSignerScriptPubKeyMan::DisplayAddress(const CTxDestination& dest, const ExternalSigner &signer) const { // TODO: avoid the need to infer a descriptor from inside a descriptor wallet + const CScript& scriptPubKey = GetScriptForDestination(dest); auto provider = GetSolvingProvider(scriptPubKey); auto descriptor = InferDescriptor(scriptPubKey, *provider); - signer.DisplayAddress(descriptor->ToString()); - // TODO inspect result - return true; + const UniValue& result = signer.DisplayAddress(descriptor->ToString()); + + const UniValue& error = result.find_value("error"); + if (error.isStr()) return util::Error{strprintf(_("Signer returned error: %s"), error.getValStr())}; + + const UniValue& ret_address = result.find_value("address"); + if (!ret_address.isStr()) return util::Error{_("Signer did not echo address")}; + + if (ret_address.getValStr() != EncodeDestination(dest)) { + return util::Error{strprintf(_("Signer echoed unexpected address %s"), ret_address.getValStr())}; + } + + return util::Result<void>(); } // If sign is true, transaction must previously have been filled diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index c052ce6129..44286456b6 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -9,6 +9,8 @@ #include <memory> +struct bilingual_str; + namespace wallet { class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan { @@ -27,7 +29,11 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan static ExternalSigner GetExternalSigner(); - bool DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const; + /** + * Display address on the device and verify that the returned value matches. + * @returns nothing or an error message + */ + util::Result<void> DisplayAddress(const CTxDestination& dest, const ExternalSigner& signer) const; TransactionError FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; }; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index d33e6f3873..0c1cae7253 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -247,7 +247,7 @@ public: return value.empty() ? m_wallet->EraseAddressReceiveRequest(batch, dest, id) : m_wallet->SetAddressReceiveRequest(batch, dest, id, value); } - bool displayAddress(const CTxDestination& dest) override + util::Result<void> displayAddress(const CTxDestination& dest) override { LOCK(m_wallet->cs_wallet); return m_wallet->DisplayAddress(dest); @@ -286,7 +286,7 @@ public: if (!res) return util::Error{util::ErrorString(res)}; const auto& txr = *res; fee = txr.fee; - change_pos = txr.change_pos ? *txr.change_pos : -1; + change_pos = txr.change_pos ? int(*txr.change_pos) : -1; return txr.tx; } diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 7f068c05ef..bed9ec029a 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -789,9 +789,8 @@ RPCHelpMan walletdisplayaddress() throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address"); } - if (!pwallet->DisplayAddress(dest)) { - throw JSONRPCError(RPC_MISC_ERROR, "Failed to display address"); - } + util::Result<void> res = pwallet->DisplayAddress(dest); + if (!res) throw JSONRPCError(RPC_MISC_ERROR, util::ErrorString(res).original); UniValue result(UniValue::VOBJ); result.pushKV("address", request.params[0].get_str()); diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 0cb0891141..b6c7396f4b 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -194,15 +194,12 @@ RPCHelpMan getbalance() LOCK(pwallet->cs_wallet); - const auto dummy_value{self.MaybeArg<std::string>(0)}; + const auto dummy_value{self.MaybeArg<std::string>("dummy")}; if (dummy_value && *dummy_value != "*") { throw JSONRPCError(RPC_METHOD_DEPRECATED, "dummy first argument must be excluded or set to \"*\"."); } - int min_depth = 0; - if (!request.params[1].isNull()) { - min_depth = request.params[1].getInt<int>(); - } + const auto min_depth{self.Arg<int>("minconf")}; bool include_watchonly = ParseIncludeWatchonly(request.params[2], *pwallet); diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index 06ec7db1bc..1252843e9d 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -11,6 +11,7 @@ #include <wallet/context.h> #include <wallet/wallet.h> +#include <string_view> #include <univalue.h> #include <boost/date_time/posix_time/posix_time.hpp> @@ -61,9 +62,9 @@ bool ParseIncludeWatchonly(const UniValue& include_watchonly, const CWallet& wal bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name) { - if (URL_DECODE && request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) { + if (request.URI.starts_with(WALLET_ENDPOINT_BASE)) { // wallet endpoint was used - wallet_name = URL_DECODE(request.URI.substr(WALLET_ENDPOINT_BASE.size())); + wallet_name = UrlDecode(std::string_view{request.URI}.substr(WALLET_ENDPOINT_BASE.size())); return true; } return false; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 4575881d96..2c1ab8d44a 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -27,7 +27,6 @@ #include <unordered_map> enum class OutputType; -struct bilingual_str; namespace wallet { struct MigrationData; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 5d23ebd35a..9a7e166e68 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -683,11 +683,11 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co // Vector of results. We will choose the best one based on waste. std::vector<SelectionResult> results; std::vector<util::Result<SelectionResult>> errors; - auto append_error = [&] (const util::Result<SelectionResult>& result) { + auto append_error = [&] (util::Result<SelectionResult>&& result) { // If any specific error message appears here, then something different from a simple "no selection found" happened. // Let's save it, so it can be retrieved to the user if no other selection algorithm succeeded. if (HasErrorMsg(result)) { - errors.emplace_back(result); + errors.emplace_back(std::move(result)); } }; @@ -698,7 +698,7 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co if (!coin_selection_params.m_subtract_fee_outputs) { if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change, max_inputs_weight)}) { results.push_back(*bnb_result); - } else append_error(bnb_result); + } else append_error(std::move(bnb_result)); } // As Knapsack and SRD can create change, also deduce change weight. @@ -707,25 +707,25 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co // 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. if (auto knapsack_result{KnapsackSolver(groups.mixed_group, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast, max_inputs_weight)}) { results.push_back(*knapsack_result); - } else append_error(knapsack_result); + } else append_error(std::move(knapsack_result)); if (coin_selection_params.m_effective_feerate > CFeeRate{3 * coin_selection_params.m_long_term_feerate}) { // Minimize input set for feerates of at least 3×LTFRE (default: 30 ṩ/vB+) if (auto cg_result{CoinGrinder(groups.positive_group, nTargetValue, coin_selection_params.m_min_change_target, max_inputs_weight)}) { cg_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); results.push_back(*cg_result); } else { - append_error(cg_result); + append_error(std::move(cg_result)); } } if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, coin_selection_params.m_change_fee, coin_selection_params.rng_fast, max_inputs_weight)}) { results.push_back(*srd_result); - } else append_error(srd_result); + } else append_error(std::move(srd_result)); if (results.empty()) { // No solution found, retrieve the first explicit error (if any). // future: add 'severity level' to errors so the worst one can be retrieved instead of the first one. - return errors.empty() ? util::Error() : errors.front(); + return errors.empty() ? util::Error() : std::move(errors.front()); } // If the chosen input set has unconfirmed inputs, check for synergies from overlapping ancestry @@ -818,7 +818,7 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin // Coin Selection attempts to select inputs from a pool of eligible UTXOs to fund the // transaction at a target feerate. If an attempt fails, more attempts may be made using a more // permissive CoinEligibilityFilter. - util::Result<SelectionResult> res = [&] { + { // Place coins eligibility filters on a scope increasing order. std::vector<SelectionFilter> ordered_filters{ // If possible, fund the transaction with confirmed UTXOs only. Prefer at least six @@ -866,9 +866,9 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin if (CAmount total_amount = available_coins.GetTotalAmount() - total_discarded < value_to_select) { // Special case, too-long-mempool cluster. if (total_amount + total_unconf_long_chain > value_to_select) { - return util::Result<SelectionResult>({_("Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool")}); + return util::Error{_("Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool")}; } - return util::Result<SelectionResult>(util::Error()); // General "Insufficient Funds" + return util::Error{}; // General "Insufficient Funds" } // Walk-through the filters until the solution gets found. @@ -885,19 +885,17 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin // If any specific error message appears here, then something particularly wrong might have happened. // Save the error and continue the selection process. So if no solutions gets found, we can return // the detailed error to the upper layers. - if (HasErrorMsg(res)) res_detailed_errors.emplace_back(res); + if (HasErrorMsg(res)) res_detailed_errors.emplace_back(std::move(res)); } } // Return right away if we have a detailed error - if (!res_detailed_errors.empty()) return res_detailed_errors.front(); + if (!res_detailed_errors.empty()) return std::move(res_detailed_errors.front()); // General "Insufficient Funds" - return util::Result<SelectionResult>(util::Error()); - }(); - - return res; + return util::Error{}; + } } static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256& block_hash) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 297432de9e..331590df7f 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -291,7 +291,10 @@ FUZZ_TARGET(coinselection) } std::vector<COutput> utxos; - std::vector<util::Result<SelectionResult>> results{result_srd, result_knapsack, result_bnb}; + std::vector<util::Result<SelectionResult>> results; + results.emplace_back(std::move(result_srd)); + results.emplace_back(std::move(result_knapsack)); + results.emplace_back(std::move(result_bnb)); CAmount new_total_balance{CreateCoins(fuzzed_data_provider, utxos, coin_params, next_locktime)}; if (new_total_balance > 0) { std::set<std::shared_ptr<COutput>> new_utxo_pool; diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 9a515828fe..792079e6c6 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -106,13 +106,11 @@ struct FuzzedWallet { CTxDestination GetDestination(FuzzedDataProvider& fuzzed_data_provider) { auto type{fuzzed_data_provider.PickValueInArray(OUTPUT_TYPES)}; - util::Result<CTxDestination> op_dest{util::Error{}}; if (fuzzed_data_provider.ConsumeBool()) { - op_dest = wallet->GetNewDestination(type, ""); + return *Assert(wallet->GetNewDestination(type, "")); } else { - op_dest = wallet->GetNewChangeDestination(type); + return *Assert(wallet->GetNewChangeDestination(type)); } - return *Assert(op_dest); } CScript GetScriptPubKey(FuzzedDataProvider& fuzzed_data_provider) { return GetScriptForDestination(GetDestination(fuzzed_data_provider)); } void FundTx(FuzzedDataProvider& fuzzed_data_provider, CMutableTransaction tx) diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index 3509bc116f..963c0f838b 100644 --- a/src/wallet/test/spend_tests.cpp +++ b/src/wallet/test/spend_tests.cpp @@ -97,13 +97,11 @@ BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup) // so that the recipient's amount is no longer equal to the user's selected target of 299 BTC. // First case, use 'subtract_fee_from_outputs=true' - util::Result<CreatedTransactionResult> res_tx = CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control); - BOOST_CHECK(!res_tx.has_value()); + BOOST_CHECK(!CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control)); // Second case, don't use 'subtract_fee_from_outputs'. recipients[0].fSubtractFeeFromAmount = false; - res_tx = CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control); - BOOST_CHECK(!res_tx.has_value()); + BOOST_CHECK(!CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control)); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 96c4397504..8f4171eb15 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2667,7 +2667,7 @@ void ReserveDestination::ReturnDestination() address = CNoDestination(); } -bool CWallet::DisplayAddress(const CTxDestination& dest) +util::Result<void> CWallet::DisplayAddress(const CTxDestination& dest) { CScript scriptPubKey = GetScriptForDestination(dest); for (const auto& spk_man : GetScriptPubKeyMans(scriptPubKey)) { @@ -2676,9 +2676,9 @@ bool CWallet::DisplayAddress(const CTxDestination& dest) continue; } ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); - return signer_spk_man->DisplayAddress(scriptPubKey, signer); + return signer_spk_man->DisplayAddress(dest, signer); } - return false; + return util::Error{_("There is no ScriptPubKeyManager for this address")}; } bool CWallet::LockCoin(const COutPoint& output, WalletBatch* batch) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index b49b5a7d0d..6a998fa398 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -537,8 +537,8 @@ public: bool IsSpentKey(const CScript& scriptPubKey) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set<CTxDestination>& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - /** Display address on an external signer. Returns false if external signer support is not compiled */ - bool DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Display address on an external signer. */ + util::Result<void> DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool IsLockedCoin(const COutPoint& output) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool LockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index d10db046f5..536e471053 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -8,6 +8,7 @@ #include <kernel/chain.h> #include <kernel/mempool_entry.h> #include <logging.h> +#include <netbase.h> #include <primitives/block.h> #include <primitives/transaction.h> #include <validationinterface.h> @@ -57,7 +58,12 @@ std::unique_ptr<CZMQNotificationInterface> CZMQNotificationInterface::Create(std { std::string arg("-zmq" + entry.first); const auto& factory = entry.second; - for (const std::string& address : gArgs.GetArgs(arg)) { + for (std::string& address : gArgs.GetArgs(arg)) { + // libzmq uses prefix "ipc://" for UNIX domain sockets + if (address.substr(0, ADDR_PREFIX_UNIX.length()) == ADDR_PREFIX_UNIX) { + address.replace(0, ADDR_PREFIX_UNIX.length(), ADDR_PREFIX_IPC); + } + std::unique_ptr<CZMQAbstractNotifier> notifier = factory(); notifier->SetType(entry.first); notifier->SetAddress(address); diff --git a/src/zmq/zmqutil.h b/src/zmq/zmqutil.h index 334b51aa91..bec48c0a56 100644 --- a/src/zmq/zmqutil.h +++ b/src/zmq/zmqutil.h @@ -9,4 +9,7 @@ void zmqError(const std::string& str); +/** Prefix for unix domain socket addresses (which are local filesystem paths) */ +const std::string ADDR_PREFIX_IPC = "ipc://"; // used by libzmq, example "ipc:///root/path/to/file" + #endif // BITCOIN_ZMQ_ZMQUTIL_H |