diff options
124 files changed, 1077 insertions, 882 deletions
diff --git a/.cirrus.yml b/.cirrus.yml index b3ac6d06cb..4f0ad2d04a 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -29,6 +29,7 @@ base_template: &BASE_TEMPLATE - if [ "$CIRRUS_PR" = "" ]; then exit 0; fi - git fetch $CIRRUS_REPO_CLONE_URL "pull/${CIRRUS_PR}/merge" - git checkout FETCH_HEAD # Use merged changes to detect silent merge conflicts + # Also, the merge commit is used to lint COMMIT_RANGE="HEAD~..HEAD" main_template: &MAIN_TEMPLATE timeout_in: 120m # https://cirrus-ci.org/faq/#instance-timed-out @@ -175,12 +176,12 @@ task: - ccache --show-stats check_script: - src\test_bitcoin.exe -l test_suite - - src\bench_bitcoin.exe --sanity-check > NUL + - src\bench_bitcoin.exe --sanity-check - python test\util\test_runner.py - python test\util\rpcauth-test.py functional_tests_script: # Increase the dynamic port range to the maximum allowed value to mitigate "OSError: [WinError 10048] Only one usage of each socket address (protocol/network address/port) is normally permitted". - # See: https://docs.microsoft.com/en-us/biztalk/technical-guides/settings-that-can-be-modified-to-improve-network-performance + # See: https://learn.microsoft.com/en-us/biztalk/technical-guides/settings-that-can-be-modified-to-improve-network-performance - netsh int ipv4 set dynamicport tcp start=1025 num=64511 - netsh int ipv6 set dynamicport tcp start=1025 num=64511 # Exclude feature_dbcrash for now due to timeout @@ -212,6 +213,8 @@ task: << : *GLOBAL_TASK_TEMPLATE container: image: quay.io/centos/centos:stream8 + # For faster CI feedback, immediately schedule one task that runs all tests + << : *CREDITS_TEMPLATE env: << : *CIRRUS_EPHEMERAL_WORKER_TEMPLATE_ENV PACKAGE_MANAGER_INSTALL: "yum install -y" diff --git a/build_msvc/README.md b/build_msvc/README.md index 7520700a34..ba6171fee7 100644 --- a/build_msvc/README.md +++ b/build_msvc/README.md @@ -17,7 +17,7 @@ Building with Visual Studio is an alternative to the Linux based [cross-compiler Prerequisites --------------------- To build [dependencies](../doc/dependencies.md) (except for [Qt](#qt)), -the default approach is to use the [vcpkg](https://docs.microsoft.com/en-us/cpp/vcpkg) package manager from Microsoft: +the default approach is to use the [vcpkg](https://vcpkg.io) package manager from Microsoft: 1. [Install](https://vcpkg.io/en/getting-started.html) vcpkg. @@ -69,7 +69,7 @@ Alternatively, open the `build_msvc/bitcoin.sln` file in Visual Studio. Security --------------------- -[Base address randomization](https://docs.microsoft.com/en-us/cpp/build/reference/dynamicbase-use-address-space-layout-randomization?view=msvc-160) is used to make Bitcoin Core more secure. When building Bitcoin using the `build_msvc` process base address randomization can be disabled by editing `common.init.vcproj` to change `RandomizedBaseAddress` from `true` to `false` and then rebuilding the project. +[Base address randomization](https://learn.microsoft.com/en-us/cpp/build/reference/dynamicbase-use-address-space-layout-randomization) is used to make Bitcoin Core more secure. When building Bitcoin using the `build_msvc` process base address randomization can be disabled by editing `common.init.vcproj` to change `RandomizedBaseAddress` from `true` to `false` and then rebuilding the project. To check if `bitcoind` has `RandomizedBaseAddress` enabled or disabled run diff --git a/ci/lint/04_install.sh b/ci/lint/04_install.sh index 9ccb1d53d1..b459b321f1 100755 --- a/ci/lint/04_install.sh +++ b/ci/lint/04_install.sh @@ -17,10 +17,10 @@ ${CI_RETRY_EXE} apt-get install -y python3-pip curl git gawk jq ) ${CI_RETRY_EXE} pip3 install codespell==2.2.1 -${CI_RETRY_EXE} pip3 install flake8==4.0.1 -${CI_RETRY_EXE} pip3 install mypy==0.942 -${CI_RETRY_EXE} pip3 install pyzmq==22.3.0 -${CI_RETRY_EXE} pip3 install vulture==2.3 +${CI_RETRY_EXE} pip3 install flake8==5.0.4 +${CI_RETRY_EXE} pip3 install mypy==0.971 +${CI_RETRY_EXE} pip3 install pyzmq==24.0.1 +${CI_RETRY_EXE} pip3 install vulture==2.6 SHELLCHECK_VERSION=v0.8.0 curl -sL "https://github.com/koalaman/shellcheck/releases/download/${SHELLCHECK_VERSION}/shellcheck-${SHELLCHECK_VERSION}.linux.x86_64.tar.xz" | tar --xz -xf - --directory /tmp/ diff --git a/ci/lint/06_script.sh b/ci/lint/06_script.sh index a1721a703f..c14d7473d3 100755 --- a/ci/lint/06_script.sh +++ b/ci/lint/06_script.sh @@ -6,9 +6,8 @@ export LC_ALL=C -GIT_HEAD=$(git rev-parse HEAD) if [ -n "$CIRRUS_PR" ]; then - COMMIT_RANGE="${CIRRUS_BASE_SHA}..$GIT_HEAD" + COMMIT_RANGE="HEAD~..HEAD" echo git log --no-merges --oneline "$COMMIT_RANGE" echo diff --git a/ci/test/00_setup_env_native_asan.sh b/ci/test/00_setup_env_native_asan.sh index 7f3421269c..66bbdb8cb5 100755 --- a/ci/test/00_setup_env_native_asan.sh +++ b/ci/test/00_setup_env_native_asan.sh @@ -23,4 +23,4 @@ export PACKAGES="systemtap-sdt-dev clang llvm python3-zmq qtbase5-dev qttools5-d export DOCKER_NAME_TAG=ubuntu:22.04 export NO_DEPENDS=1 export GOAL="install" -export BITCOIN_CONFIG="--enable-usdt --enable-zmq --with-incompatible-bdb --with-gui=qt5 CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER' --with-sanitizers=address,integer,undefined CC=clang CXX=clang++" +export BITCOIN_CONFIG="--enable-c++20 --enable-usdt --enable-zmq --with-incompatible-bdb --with-gui=qt5 CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER' --with-sanitizers=address,integer,undefined CC=clang CXX=clang++" diff --git a/ci/test/00_setup_env_native_fuzz_with_msan.sh b/ci/test/00_setup_env_native_fuzz_with_msan.sh index d05bb3bb11..82419708d2 100755 --- a/ci/test/00_setup_env_native_fuzz_with_msan.sh +++ b/ci/test/00_setup_env_native_fuzz_with_msan.sh @@ -12,7 +12,7 @@ export MSAN_FLAGS="-fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit LIBCXX_FLAGS="-nostdinc++ -stdlib=libc++ -L${LIBCXX_DIR}lib -lc++abi -I${LIBCXX_DIR}include -I${LIBCXX_DIR}include/c++/v1 -lpthread -Wl,-rpath,${LIBCXX_DIR}lib -Wno-unused-command-line-argument" export MSAN_AND_LIBCXX_FLAGS="${MSAN_FLAGS} ${LIBCXX_FLAGS}" -export CONTAINER_NAME="ci_native_msan" +export CONTAINER_NAME="ci_native_fuzz_msan" export PACKAGES="clang-12 llvm-12 cmake" # BDB generates false-positives and will be removed in future export DEP_OPTS="NO_BDB=1 NO_QT=1 CC='clang' CXX='clang++' CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}' libevent_cflags='${MSAN_FLAGS}' sqlite_cflags='${MSAN_FLAGS}' zeromq_cxxflags='-std=c++17 ${MSAN_AND_LIBCXX_FLAGS}'" diff --git a/ci/test/00_setup_env_native_tidy.sh b/ci/test/00_setup_env_native_tidy.sh index 11a12e336a..e4d3468473 100755 --- a/ci/test/00_setup_env_native_tidy.sh +++ b/ci/test/00_setup_env_native_tidy.sh @@ -15,5 +15,5 @@ export RUN_FUNCTIONAL_TESTS=false export RUN_FUZZ_TESTS=false export RUN_TIDY=true export GOAL="install" -export BITCOIN_CONFIG="CC=clang CXX=clang++ --enable-c++20 --with-incompatible-bdb --disable-hardening CFLAGS='-O0 -g0' CXXFLAGS='-O0 -g0'" +export BITCOIN_CONFIG="CC=clang CXX=clang++ --with-incompatible-bdb --disable-hardening CFLAGS='-O0 -g0' CXXFLAGS='-O0 -g0'" export CCACHE_SIZE=200M diff --git a/ci/test/00_setup_env_native_tsan.sh b/ci/test/00_setup_env_native_tsan.sh index 66d18b974c..0e0a4b8f18 100755 --- a/ci/test/00_setup_env_native_tsan.sh +++ b/ci/test/00_setup_env_native_tsan.sh @@ -11,4 +11,4 @@ export DOCKER_NAME_TAG=ubuntu:22.04 export PACKAGES="clang-13 llvm-13 libc++abi-13-dev libc++-13-dev python3-zmq" export DEP_OPTS="CC=clang-13 CXX='clang++-13 -stdlib=libc++'" export GOAL="install" -export BITCOIN_CONFIG="--enable-zmq CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER -DDEBUG_LOCKCONTENTION' CXXFLAGS='-g' --with-sanitizers=thread CC=clang-13 CXX='clang++-13 -stdlib=libc++'" +export BITCOIN_CONFIG="--enable-zmq CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER -DDEBUG_LOCKCONTENTION' CXXFLAGS='-g' --with-sanitizers=thread" diff --git a/ci/test/04_install.sh b/ci/test/04_install.sh index 627fb8484d..f8ff5aa37a 100755 --- a/ci/test/04_install.sh +++ b/ci/test/04_install.sh @@ -24,6 +24,7 @@ if [[ $BITCOIN_CONFIG = *--with-sanitizers=*address* ]]; then # If ran with (ASa fi export P_CI_DIR="$PWD" +export BINS_SCRATCH_DIR="${BASE_SCRATCH_DIR}/bins/" if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then echo "Creating $DOCKER_NAME_TAG container to run in" @@ -64,14 +65,16 @@ else fi CI_EXEC () { - $DOCKER_CI_CMD_PREFIX bash -c "export PATH=$BASE_SCRATCH_DIR/bins/:\$PATH && cd \"$P_CI_DIR\" && $*" + $DOCKER_CI_CMD_PREFIX bash -c "export PATH=${BINS_SCRATCH_DIR}:\$PATH && cd \"$P_CI_DIR\" && $*" } CI_EXEC_ROOT () { - $DOCKER_CI_CMD_PREFIX_ROOT bash -c "export PATH=$BASE_SCRATCH_DIR/bins/:\$PATH && cd \"$P_CI_DIR\" && $*" + $DOCKER_CI_CMD_PREFIX_ROOT bash -c "export PATH=${BINS_SCRATCH_DIR}:\$PATH && cd \"$P_CI_DIR\" && $*" } export -f CI_EXEC export -f CI_EXEC_ROOT +CI_EXEC mkdir -p "${BINS_SCRATCH_DIR}" + if [ -n "$DPKG_ADD_ARCH" ]; then CI_EXEC_ROOT dpkg --add-architecture "$DPKG_ADD_ARCH" fi @@ -155,12 +158,10 @@ fi if [ "$USE_BUSY_BOX" = "true" ]; then echo "Setup to use BusyBox utils" - CI_EXEC mkdir -p "${BASE_SCRATCH_DIR}/bins/" # tar excluded for now because it requires passing in the exact archive type in ./depends (fixed in later BusyBox version) - # find excluded for now because it does not recognize the -delete option in ./depends (fixed in later BusyBox version) # ar excluded for now because it does not recognize the -q option in ./depends (unknown if fixed) # shellcheck disable=SC1010 - CI_EXEC for util in \$\(busybox --list \| grep -v "^ar$" \| grep -v "^tar$" \| grep -v "^find$"\)\; do ln -s \$\(command -v busybox\) "${BASE_SCRATCH_DIR}/bins/\$util"\; done + CI_EXEC for util in \$\(busybox --list \| grep -v "^ar$" \| grep -v "^tar$" \)\; do ln -s \$\(command -v busybox\) "${BINS_SCRATCH_DIR}/\$util"\; done # Print BusyBox version CI_EXEC patch --help fi diff --git a/contrib/README.md b/contrib/README.md index 0252d668d9..3c6e978061 100644 --- a/contrib/README.md +++ b/contrib/README.md @@ -26,9 +26,6 @@ The [Debian](/contrib/debian) subfolder contains the copyright file. All other packaging related files can be found in the [bitcoin-core/packaging](https://github.com/bitcoin-core/packaging) repository. -### [Builder keys](/contrib/builder-keys) -PGP keys used for signing Bitcoin Core [release](/doc/release-process.md) results. - ### [MacDeploy](/contrib/macdeploy) ### Scripts and notes for Mac builds. diff --git a/contrib/builder-keys/README.md b/contrib/builder-keys/README.md deleted file mode 100644 index a6179d6012..0000000000 --- a/contrib/builder-keys/README.md +++ /dev/null @@ -1,33 +0,0 @@ -## PGP keys of builders and Developers - -The file `keys.txt` contains fingerprints of the public keys of builders and -active developers. - -The associated keys are mainly used to sign git commits or the build results -of Guix builds. - -The most recent version of each pgp key can be found on most pgp key servers. - -Fetch the latest version from the key server to see if any key was revoked in -the meantime. -To fetch the latest version of all pgp keys in your gpg homedir, - -```sh -gpg --refresh-keys -``` - -To fetch keys of builders and active developers, feed the list of fingerprints -of the primary keys into gpg: - -On \*NIX: -```sh -while read fingerprint keyholder_name; do gpg --keyserver hkps://keys.openpgp.org --recv-keys ${fingerprint}; done < ./keys.txt -``` - -On Windows (requires Gpg4win >= 4.0.0): -``` -FOR /F "tokens=1" %i IN (keys.txt) DO gpg --keyserver hkps://keys.openpgp.org --recv-keys %i -``` - -Add your key to the list if you provided Guix attestations for two major or -minor releases of Bitcoin Core. diff --git a/contrib/builder-keys/keys.txt b/contrib/builder-keys/keys.txt deleted file mode 100644 index b2b5b031f8..0000000000 --- a/contrib/builder-keys/keys.txt +++ /dev/null @@ -1,57 +0,0 @@ -982A193E3CE0EED535E09023188CBB2648416AD5 0xB10C (0xb10c) -9D3CC86A72F8494342EA5FD10A41BDC3F4FAFF1C Aaron Clauson (sipsorcery) -617C90010B3BD370B0AC7D424BB42E31C79111B8 Akira Takizawa (akx20000) -E944AE667CF960B1004BC32FCA662BE18B877A60 Andreas Schildbach (aschildbach) -152812300785C96444D3334D17565732E08E5E41 Andrew Chow (achow101) -590B7292695AFFA5B672CBB2E13FC145CD3F4304 Antoine Poinsot (darosior) -0AD83877C1F0CD1EE9BD660AD7CC770B81FD22A8 Ben Carman (benthecarman) -912FD3228387123DC97E0E57D5566241A0295FA9 BtcDrak (btcdrak) -04017A2A6D9A0CCDC81D8EC296AB007F1A7ED999 Carl Dong (dongcarl) -C519EBCF3B926298946783EFF6430754120EC2F4 Christian Decker (cdecker) -18AE2F798E0D239755DA4FD24B79F986CBDF8736 Chun Kuan Le (ken2812221) -101598DC823C1B5F9A6624ABA5E0907A0380E6C3 CoinForensics (CoinForensics) -F20F56EF6A067F70E8A5C99FFF95FAA971697405 centaur (centaur) -C060A6635913D98A3587D7DB1C2491FFEB0EF770 Cory Fields (cfields) -BF6273FAEF7CC0BA1F562E50989F6B3048A116B5 Dev Random (devrandom) -6D3170C1DC2C6FD0AEEBCA6743811D1A26623924 Douglas Roark (droark) -948444FCE03B05BA5AB0591EC37B1C1D44C786EE Duncan Dean (dunxen) -1C6621605EC50319C463D56C7F81D87985D61612 Emanuele Cisbani (cisba) -9A1689B60D1B3CCE9262307A2F40A9BF167FBA47 Erik Mossberg (erkmos) -D35176BE9264832E4ACA8986BF0792FBE95DC863 fivepiece (fivepiece) -6F993B250557E7B016ADE5713BDCDA2D87A881D9 Fuzzbawls (Fuzzbawls) -01CDF4627A3B88AAE4A571C87588242FBE38D3A8 Gavin Andresen (gavinandresen) -6B002C6EA3F91B1B0DF0C9BC8F617F1200A6D25C Gloria Zhao (glozow) -D1DBF2C4B96F2DEBF4C16654410108112E7EA81F Hennadii Stepanov (hebasto) -2688F5A9A4BE0F295E921E8A25F27A38A47AD566 James O'Beirne (jamesob) -D3F22A3A4C366C2DCB66D3722DA9C5A7FA81EA35 Jarol Rodriguez (jarolrod) -7480909378D544EA6B6DCEB7535B12980BB8A4D3 Jeffri H Frontz (jhfrontz) -D3CC177286005BB8FF673294C5242A1AB3936517 jl2012 (jl2012) -82921A4B88FD454B7EB8CE3C796C4109063D4EAF Jon Atack (jonatack) -32EE5C4C3FA15CCADB46ABE529D4BCB6416F53EC Jonas Schnelli (jonasschnelli) -4B4E840451149DD7FB0D633477DFAB5C3108B9A8 Jorge Timon (jtimon) -C42AFF7C61B3E44A1454CD3557AF762DB3353322 Karl-Johan Alm (kallewoof) -70A1D47DD44F59DF8B22244333E472FE870C7E5D Kristaps Kaupe (kristapsk) -30DE693AE0DE9E37B3E7EB6BBFF0F67810C1EED1 Lisa Neigut (niftynei) -E463A93F5F3117EEDE6C7316BD02942421F4889F Luke Dashjr (luke-jr) -B8B3F1C0E58C15DB6A81D30C3648A882F4316B9B Marco Falke (marco) -07DF3E57A548CCFB7530709189BBB8663E2E65CE Matt Corallo (BlueMatt) -CA03882CB1FC067B5D3ACFE4D300116E1C875A3D MeshCollider (meshcollider) -E777299FC265DD04793070EB944D35F9AC3DB76A Michael Ford (fanquake) -AD5764F4ADCE1B99BDFD179E12335A271D4D62EC Michael Tidwell (miketwenty1) -9692B91BBF0E8D34DFD33B1882C5C009628ECF0C Michagogo (michagogo) -C57E4B42223FDE851D4F69DD28DF2724F241D8EE midnightmagic (midnightmagic) -F4FC70F07310028424EFC20A8E4256593F177720 Oliver Gugger (guggero, Oliver Gugger) -D62A803E27E7F43486035ADBBCD04D8E9CCCAC2A Paul Rabahy (prab) -37EC7D7B0A217CDB4B4E007E7FAB114267E4FA04 Peter Todd (petertodd) -D762373D24904A3E42F33B08B9A408E71DAAC974 Pieter Wuille [Location: Leuven, Belgium] (sipa) -133EAC179436F14A5CF1B794860FEB804E669320 Pieter Wuille (sipa) -6A8F9C266528E25AEB1D7731C2371D91CB716EA7 Sebastian Falbesoner (theStack) -A8FC55F3B04BA3146F3492E79303B33A305224CB Sebastian Kung (TheCharlatan) -ED9BDF7AD6A55E232E84524257FF9BDBCC301009 Sjors Provoost (sjors) -867345026B6763E8B07EE73AB6737117397F5C4F Stephan Oeste (Emzy) -9EDAFF80E080659604F4A76B2EBB056FD847F8A7 Stephan Oeste (Emzy) -6DEEF79B050C4072509B743F8C275BC595448867 Tomas Kanocz (KanoczTomas) -AEC1884398647C47413C1C3FB1179EB7347DC10D Warren Togami (wtogami) -74E2DEF5D77260B98BC19438099BAD163C70FBFA Will Clark (will8clark) -79D00BAC68B56D422F945A8F8E3A8F3247DBCBBF Willy Ko (willyko) -71A3B16735405025D447E8F274810B012346C9A6 Wladimir J. van der Laan (laanwj) diff --git a/contrib/install_db4.sh b/contrib/install_db4.sh index 2850c4b993..c7d39f5b99 100755 --- a/contrib/install_db4.sh +++ b/contrib/install_db4.sh @@ -32,16 +32,15 @@ check_exists() { sha256_check() { # Args: <sha256_hash> <filename> # - if check_exists sha256sum; then - echo "${1} ${2}" | sha256sum -c + if [ "$(uname)" = "FreeBSD" ]; then + # sha256sum exists on FreeBSD, but takes different arguments than the GNU version + sha256 -c "${1}" "${2}" + elif check_exists sha256sum; then + echo "${1} ${2}" | sha256sum -c elif check_exists sha256; then - if [ "$(uname)" = "FreeBSD" ]; then - sha256 -c "${1}" "${2}" - else - echo "${1} ${2}" | sha256 -c - fi + echo "${1} ${2}" | sha256 -c else - echo "${1} ${2}" | shasum -a 256 -c + echo "${1} ${2}" | shasum -a 256 -c fi } diff --git a/contrib/linearize/linearize-hashes.py b/contrib/linearize/linearize-hashes.py index 804ab34d65..695bafad34 100755 --- a/contrib/linearize/linearize-hashes.py +++ b/contrib/linearize/linearize-hashes.py @@ -78,7 +78,7 @@ def get_block_hashes(settings, max_blocks_per_call=10000): if rpc.response_is_error(resp_obj): print('JSON-RPC: error at height', height+x, ': ', resp_obj['error'], file=sys.stderr) sys.exit(1) - assert(resp_obj['id'] == x) # assume replies are in-sequence + assert resp_obj['id'] == x # assume replies are in-sequence if settings['rev_hash_bytes'] == 'true': resp_obj['result'] = bytes.fromhex(resp_obj['result'])[::-1].hex() print(resp_obj['result']) diff --git a/contrib/seeds/generate-seeds.py b/contrib/seeds/generate-seeds.py index 44345e3987..a6f435af0d 100755 --- a/contrib/seeds/generate-seeds.py +++ b/contrib/seeds/generate-seeds.py @@ -70,13 +70,13 @@ def name_to_bip155(addr): if i == 0 or i == (len(addr)-1): # skip empty component at beginning or end continue x += 1 # :: skips to suffix - assert(x < 2) + assert x < 2 else: # two bytes per component val = int(comp, 16) sub[x].append(val >> 8) sub[x].append(val & 0xff) nullbytes = 16 - len(sub[0]) - len(sub[1]) - assert((x == 0 and nullbytes == 0) or (x == 1 and nullbytes > 0)) + assert (x == 0 and nullbytes == 0) or (x == 1 and nullbytes > 0) addr_bytes = bytes(sub[0] + ([0] * nullbytes) + sub[1]) if addr_bytes[0] == 0xfc: # Assume that seeds with fc00::/8 addresses belong to CJDNS, diff --git a/depends/packages/boost.mk b/depends/packages/boost.mk index e3625d97f5..ebc097d686 100644 --- a/depends/packages/boost.mk +++ b/depends/packages/boost.mk @@ -1,8 +1,8 @@ package=boost -$(package)_version=1.80.0 +$(package)_version=1.81.0 $(package)_download_path=https://boostorg.jfrog.io/artifactory/main/release/$($(package)_version)/source/ $(package)_file_name=boost_$(subst .,_,$($(package)_version)).tar.bz2 -$(package)_sha256_hash=1e19565d82e43bc59209a168f5ac899d3ba471d55c7610c677d4ccf2c9c500c0 +$(package)_sha256_hash=71feeed900fbccca04a3b4f2f84a7c217186f28a940ed8b7ed4725986baf99fa define $(package)_stage_cmds mkdir -p $($(package)_staging_prefix_dir)/include && \ diff --git a/doc/build-windows.md b/doc/build-windows.md index e35d3bcbd0..027e8f80f5 100644 --- a/doc/build-windows.md +++ b/doc/build-windows.md @@ -6,8 +6,8 @@ Below are some notes on how to build Bitcoin Core for Windows. The options known to work for building Bitcoin Core on Windows are: * On Linux, using the [Mingw-w64](https://www.mingw-w64.org/) cross compiler tool chain. -* On Windows, using [Windows Subsystem for Linux (WSL)](https://docs.microsoft.com/windows/wsl/about) and Mingw-w64. -* On Windows, using [Microsoft Visual Studio](https://www.visualstudio.com). See [README.md](/build_msvc/README.md). +* On Windows, using [Windows Subsystem for Linux (WSL)](https://learn.microsoft.com/en-us/windows/wsl/about) and Mingw-w64. +* On Windows, using [Microsoft Visual Studio](https://visualstudio.microsoft.com). See [README.md](/build_msvc/README.md). Other options which may work, but which have not been extensively tested are (please contribute instructions): @@ -16,7 +16,7 @@ Other options which may work, but which have not been extensively tested are (pl Installing Windows Subsystem for Linux --------------------------------------- -Follow the upstream installation instructions, available [here](https://docs.microsoft.com/windows/wsl/install-win10). +Follow the upstream installation instructions, available [here](https://learn.microsoft.com/en-us/windows/wsl/install). Cross-compilation for Ubuntu and Windows Subsystem for Linux ------------------------------------------------------------ @@ -66,7 +66,7 @@ Note that for WSL the Bitcoin Core source path MUST be somewhere in the default example /usr/src/bitcoin, AND not under /mnt/d/. If this is not the case the dependency autoconf scripts will fail. This means you cannot use a directory that is located directly on the host Windows file system to perform the build. -Additional WSL Note: WSL support for [launching Win32 applications](https://docs.microsoft.com/en-us/archive/blogs/wsl/windows-and-ubuntu-interoperability#launching-win32-applications-from-within-wsl) +Additional WSL Note: WSL support for [launching Win32 applications](https://learn.microsoft.com/en-us/archive/blogs/wsl/windows-and-ubuntu-interoperability#launching-win32-applications-from-within-wsl) results in `Autoconf` configure scripts being able to execute Windows Portable Executable files. This can cause unexpected behaviour during the build, such as Win32 error dialogs for missing libraries. The recommended approach is to temporarily disable WSL support for Win32 applications. diff --git a/doc/dependencies.md b/doc/dependencies.md index ef8faff06c..c1f641c162 100644 --- a/doc/dependencies.md +++ b/doc/dependencies.md @@ -17,7 +17,7 @@ You can find installation instructions in the `build-*.md` file for your platfor | Dependency | Releases | Version used | Minimum required | Runtime | | --- | --- | --- | --- | --- | -| [Boost](../depends/packages/boost.mk) | [link](https://www.boost.org/users/download/) | [1.80.0](https://github.com/bitcoin/bitcoin/pull/25873) | [1.64.0](https://github.com/bitcoin/bitcoin/pull/22320) | No | +| [Boost](../depends/packages/boost.mk) | [link](https://www.boost.org/users/download/) | [1.81.0](https://github.com/bitcoin/bitcoin/pull/26557) | [1.64.0](https://github.com/bitcoin/bitcoin/pull/22320) | No | | [libevent](../depends/packages/libevent.mk) | [link](https://github.com/libevent/libevent/releases) | [2.1.12-stable](https://github.com/bitcoin/bitcoin/pull/21991) | [2.1.8](https://github.com/bitcoin/bitcoin/pull/24681) | No | | glibc | [link](https://www.gnu.org/software/libc/) | N/A | [2.18](https://github.com/bitcoin/bitcoin/pull/23511) | Yes | | Linux Kernel | [link](https://www.kernel.org/) | N/A | 3.2.0 | Yes | diff --git a/doc/fuzzing.md b/doc/fuzzing.md index 9abfbc9213..b498f2f41b 100644 --- a/doc/fuzzing.md +++ b/doc/fuzzing.md @@ -283,8 +283,8 @@ $ sudo apt-get install libtool libtool-bin wget automake autoconf bison gdb ``` At this point, you must install the .NET core. The process differs, depending on your Linux distribution. -See [this link](https://docs.microsoft.com/en-us/dotnet/core/install/linux) for details. -On ubuntu 20.04, the following should work: +See [this link](https://learn.microsoft.com/en-us/dotnet/core/install/linux) for details. +On Ubuntu 20.04, the following should work: ```sh $ wget -q https://packages.microsoft.com/config/ubuntu/20.04/packages-microsoft-prod.deb diff --git a/doc/i2p.md b/doc/i2p.md index 8433bbeaa2..0432136554 100644 --- a/doc/i2p.md +++ b/doc/i2p.md @@ -16,8 +16,7 @@ enabled is required. Options include: Java - [i2pd (I2P Daemon)](https://github.com/PurpleI2P/i2pd) ([documentation](https://i2pd.readthedocs.io/en/latest)), a lighter - alternative in C++ (successfully tested with version 2.23 and up; version 2.36 - or later recommended) + alternative in C++ - [i2p-zero](https://github.com/i2p-zero/i2p-zero) - [other alternatives](https://en.wikipedia.org/wiki/I2P#Routers) @@ -33,12 +32,10 @@ Core configuration options: none) -i2pacceptincoming - If set and -i2psam is also set then incoming I2P connections are - accepted via the SAM proxy. If this is not set but -i2psam is set - then only outgoing connections will be made to the I2P network. - Ignored if -i2psam is not set. Listening for incoming I2P - connections is done through the SAM proxy, not by binding to a - local address and port (default: 1) + Whether to accept inbound I2P connections (default: 1). Ignored if + -i2psam is not set. Listening for inbound I2P connections is + done through the SAM proxy, not by binding to a local address and + port. ``` In a typical situation, this suffices: @@ -47,27 +44,6 @@ In a typical situation, this suffices: bitcoind -i2psam=127.0.0.1:7656 ``` -The first time Bitcoin Core connects to the I2P router, if -`-i2pacceptincoming=1`, then it will automatically generate a persistent I2P -address and its corresponding private key. The private key will be saved in a -file named `i2p_private_key` in the Bitcoin Core data directory. The persistent -I2P address is used for accepting incoming connections and for making outgoing -connections if `-i2pacceptincoming=1`. If `-i2pacceptincoming=0` then only -outbound I2P connections are made and a different transient I2P address is used -for each connection to improve privacy. - -## Persistent vs transient I2P addresses - -In I2P connections, the connection receiver sees the I2P address of the -connection initiator. This is unlike the Tor network where the recipient does -not know who is connecting to them and can't tell if two connections are from -the same peer or not. - -If an I2P node is not accepting incoming connections, then Bitcoin Core uses -random, one-time, transient I2P addresses for itself for outbound connections -to make it harder to discriminate, fingerprint or analyze it based on its I2P -address. - ## Additional configuration options related to I2P ``` @@ -100,7 +76,29 @@ In general, a node can be run with both onion and I2P hidden services (or any/all of IPv4/IPv6/onion/I2P/CJDNS), which can provide a potential fallback if one of the networks has issues. -## I2P-related information in Bitcoin Core +## Persistent vs transient I2P addresses + +The first time Bitcoin Core connects to the I2P router, it automatically +generates a persistent I2P address and its corresponding private key by default +or if `-i2pacceptincoming=1` is set. The private key is saved in a file named +`i2p_private_key` in the Bitcoin Core data directory. The persistent I2P +address is used for making outbound connections and accepting inbound +connections. + +In the I2P network, the receiver of an inbound connection sees the address of +the initiator. This is unlike the Tor network, where the recipient does not +know who is connecting to it. + +If your node is configured by setting `-i2pacceptincoming=0` to not accept +inbound I2P connections, then it will use a random transient I2P address for +itself on each outbound connection to make it harder to discriminate, +fingerprint or analyze it based on its I2P address. + +I2P addresses are designed to be long-lived. Waiting for tunnels to be built +for every peer connection adds delay to connection setup time. Therefore, I2P +listening should only be turned off if really needed. + +## Fetching I2P-related information from Bitcoin Core There are several ways to see your I2P address in Bitcoin Core if accepting incoming I2P connections (`-i2pacceptincoming`): @@ -136,14 +134,19 @@ port (`TO_PORT`) is always set to 0 and is not in the control of Bitcoin Core. ## Bandwidth -I2P routers may route a large amount of general network traffic with their -default settings. Check your router's configuration to limit the amount of this -traffic relayed, if desired. +By default, your node shares bandwidth and transit tunnels with the I2P network +in order to increase your anonymity with cover traffic, help the I2P router used +by your node integrate optimally with the network, and give back to the network. +It's important that the nodes of a popular application like Bitcoin contribute +as much to the I2P network as they consume. -With `i2pd`, the amount of bandwidth being shared with the wider network can be -adjusted with the `bandwidth`, `share` and `transittunnels` options in your -`i2pd.conf` file. For example, to limit total I2P traffic to 256KB/s and share -50% of this limit for a maximum of 20 transit tunnels: +It is possible, though strongly discouraged, to change your I2P router +configuration to limit the amount of I2P traffic relayed by your node. + +With `i2pd`, this can be done by adjusting the `bandwidth`, `share` and +`transittunnels` options in your `i2pd.conf` file. For example, to limit total +I2P traffic to 256KB/s and share 50% of this limit for a maximum of 20 transit +tunnels: ``` bandwidth = 256 @@ -153,9 +156,15 @@ share = 50 transittunnels = 20 ``` -If you prefer not to relay any public I2P traffic and only permit I2P traffic -from programs which are connecting via the SAM proxy, e.g. Bitcoin Core, you -can set the `notransit` option to `true`. - Similar bandwidth configuration options for the Java I2P router can be found in `http://127.0.0.1:7657/config` under the "Bandwidth" tab. + +Before doing this, please see the "Participating Traffic Considerations" section +in [Embedding I2P in your Application](https://geti2p.net/en/docs/applications/embedding). + +In most cases, the default router settings should work fine. + +## Bundling I2P in a Bitcoin application + +Please see the "General Guidance for Developers" section in https://geti2p.net/en/docs/api/samv3 +if you are developing a downstream application that may be bundling I2P with Bitcoin. diff --git a/doc/release-notes-26618.md b/doc/release-notes-26618.md new file mode 100644 index 0000000000..9d1ef3bd2e --- /dev/null +++ b/doc/release-notes-26618.md @@ -0,0 +1,4 @@ +RPC Wallet +---------- + +- RPC `unloadwallet` now fails if a rescan is in progress. (#26618) diff --git a/doc/release-process.md b/doc/release-process.md index 17a03f7dcd..59ac2e884a 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -134,7 +134,6 @@ Follow the relevant Guix README.md sections: ### Verify other builders' signatures to your own (optional) -- [Add other builders keys to your gpg keyring, and/or refresh keys](/contrib/builder-keys/README.md) - [Verifying build output attestations](/contrib/guix/README.md#verifying-build-output-attestations) ### Commit your non codesigned signature to guix.sigs @@ -193,7 +192,6 @@ popd ### Verify other builders' signatures to your own (optional) -- [Add other builders keys to your gpg keyring, and/or refresh keys](/contrib/builder-keys/README.md) - [Verifying build output attestations](/contrib/guix/README.md#verifying-build-output-attestations) ### Commit your codesigned signature to guix.sigs (for the signed macOS/Windows binaries) diff --git a/release-notes-26646.md b/release-notes-26646.md new file mode 100644 index 0000000000..7f94505a01 --- /dev/null +++ b/release-notes-26646.md @@ -0,0 +1,8 @@ +JSON-RPC +-------- + +The `testmempoolaccept` RPC now returns 2 additional results within the "fees" result: +"effective-feerate" is the feerate including fees and sizes of transactions validated together if +package validation was used, and also includes any modified fees from prioritisetransaction. The +"effective-includes" result lists the wtxids of transactions whose modified fees and sizes were used +in the effective-feerate (#26646). diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 74c30f1caf..1a29e9a47a 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -174,7 +174,6 @@ BITCOIN_TESTS += \ wallet/test/wallet_crypto_tests.cpp \ wallet/test/wallet_transaction_tests.cpp \ wallet/test/coinselector_tests.cpp \ - wallet/test/availablecoins_tests.cpp \ wallet/test/init_tests.cpp \ wallet/test/ismine_tests.cpp \ wallet/test/rpc_util_tests.cpp \ @@ -376,7 +375,7 @@ if TARGET_WINDOWS else if ENABLE_BENCH @echo "Running bench/bench_bitcoin (one iteration sanity check, only high priority)..." - $(BENCH_BINARY) -sanity-check -priority-level=high > /dev/null + $(BENCH_BINARY) -sanity-check -priority-level=high endif endif $(AM_V_at)$(MAKE) $(AM_MAKEFLAGS) -C secp256k1 check diff --git a/src/bench/bench.cpp b/src/bench/bench.cpp index 7db03aaab0..4374a63250 100644 --- a/src/bench/bench.cpp +++ b/src/bench/bench.cpp @@ -83,7 +83,7 @@ void BenchRunner::RunAll(const Args& args) std::smatch baseMatch; if (args.sanity_check) { - std::cout << "Running with --sanity-check option, benchmark results will be useless." << std::endl; + std::cout << "Running with -sanity-check option, output is being suppressed as benchmark results will be useless." << std::endl; } std::vector<ankerl::nanobench::Result> benchmarkResults; @@ -106,6 +106,7 @@ void BenchRunner::RunAll(const Args& args) Bench bench; if (args.sanity_check) { bench.epochs(1).epochIterations(1); + bench.output(nullptr); } bench.name(name); if (args.min_time > 0ms) { diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp index f983d46d67..06e32f684f 100644 --- a/src/bench/bench_bitcoin.cpp +++ b/src/bench/bench_bitcoin.cpp @@ -31,7 +31,7 @@ static void SetupBenchArgs(ArgsManager& argsman) argsman.AddArg("-min-time=<milliseconds>", strprintf("Minimum runtime per benchmark, in milliseconds (default: %d)", DEFAULT_MIN_TIME_MS), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); argsman.AddArg("-output-csv=<output.csv>", "Generate CSV file with the most important benchmark results", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-output-json=<output.json>", "Generate JSON file with all benchmark results", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); - argsman.AddArg("-sanity-check", "Run benchmarks for only one iteration", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-sanity-check", "Run benchmarks for only one iteration with no output", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-priority-level=<l1,l2,l3>", strprintf("Run benchmarks of one or multiple priority level(s) (%s), default: '%s'", benchmark::ListPriorities(), DEFAULT_PRIORITY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); } diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index d02ddff37d..087e1442fe 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -99,9 +99,9 @@ static CAmount make_hard_case(int utxos, std::vector<OutputGroup>& utxo_pool) utxo_pool.clear(); CAmount target = 0; for (int i = 0; i < utxos; ++i) { - target += (CAmount)1 << (utxos+i); - add_coin((CAmount)1 << (utxos+i), 2*i, utxo_pool); - add_coin(((CAmount)1 << (utxos+i)) + ((CAmount)1 << (utxos-1-i)), 2*i + 1, utxo_pool); + target += CAmount{1} << (utxos+i); + add_coin(CAmount{1} << (utxos+i), 2*i, utxo_pool); + add_coin((CAmount{1} << (utxos+i)) + (CAmount{1} << (utxos-1-i)), 2*i + 1, utxo_pool); } return target; } diff --git a/src/bench/nanobench.h b/src/bench/nanobench.h index 916a7d61ef..70e02083c9 100644 --- a/src/bench/nanobench.h +++ b/src/bench/nanobench.h @@ -1905,7 +1905,7 @@ PerformanceCounters& performanceCounters() { // Windows version of doNotOptimizeAway // see https://github.com/google/benchmark/blob/master/include/benchmark/benchmark.h#L307 // see https://github.com/facebook/folly/blob/master/folly/Benchmark.h#L280 -// see https://docs.microsoft.com/en-us/cpp/preprocessor/optimize +// see https://learn.microsoft.com/en-us/cpp/preprocessor/optimize # if defined(_MSC_VER) # pragma optimize("", off) void doNotOptimizeAwaySink(void const*) {} diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp index 3cbf2c9008..820c9d5d50 100644 --- a/src/bench/wallet_create_tx.cpp +++ b/src/bench/wallet_create_tx.cpp @@ -133,11 +133,51 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type }); } +static void AvailableCoins(benchmark::Bench& bench, const std::vector<OutputType>& output_type) +{ + const auto test_setup = MakeNoLogFileContext<const TestingSetup>(); + CWallet wallet{test_setup->m_node.chain.get(), "", gArgs, CreateMockWalletDatabase()}; + { + LOCK(wallet.cs_wallet); + wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + wallet.SetupDescriptorScriptPubKeyMans(); + if (wallet.LoadWallet() != DBErrors::LOAD_OK) assert(false); + } + + // Generate destinations + std::vector<CScript> dest_wallet; + for (auto type : output_type) { + dest_wallet.emplace_back(GetScriptForDestination(getNewDestination(wallet, type))); + } + + // Generate chain; each coinbase will have two outputs to fill-up the wallet + const auto& params = Params(); + unsigned int chain_size = 1000; + for (unsigned int i = 0; i < chain_size / dest_wallet.size(); ++i) { + for (const auto& dest : dest_wallet) { + generateFakeBlock(params, test_setup->m_node, wallet, dest); + } + } + + // Check available balance + auto bal = wallet::GetAvailableBalance(wallet); // Cache + assert(bal == 50 * COIN * (chain_size - COINBASE_MATURITY)); + + bench.epochIterations(2).run([&] { + LOCK(wallet.cs_wallet); + const auto& res = wallet::AvailableCoins(wallet); + assert(res.All().size() == (chain_size - COINBASE_MATURITY) * 2); + }); +} + static void WalletCreateTxUseOnlyPresetInputs(benchmark::Bench& bench) { WalletCreateTx(bench, OutputType::BECH32, /*allow_other_inputs=*/false, {{/*num_of_internal_inputs=*/4}}); } static void WalletCreateTxUsePresetInputsAndCoinSelection(benchmark::Bench& bench) { WalletCreateTx(bench, OutputType::BECH32, /*allow_other_inputs=*/true, {{/*num_of_internal_inputs=*/4}}); } +static void WalletAvailableCoins(benchmark::Bench& bench) { AvailableCoins(bench, {OutputType::BECH32M}); } + BENCHMARK(WalletCreateTxUseOnlyPresetInputs, benchmark::PriorityLevel::LOW) BENCHMARK(WalletCreateTxUsePresetInputsAndCoinSelection, benchmark::PriorityLevel::LOW) +BENCHMARK(WalletAvailableCoins, benchmark::PriorityLevel::LOW);
\ No newline at end of file diff --git a/src/compat/stdin.cpp b/src/compat/stdin.cpp index 4e00bbf450..c11763f68f 100644 --- a/src/compat/stdin.cpp +++ b/src/compat/stdin.cpp @@ -58,7 +58,7 @@ bool StdinReady() return false; #else struct pollfd fds; - fds.fd = 0; /* this is STDIN */ + fds.fd = STDIN_FILENO; fds.events = POLLIN; return poll(&fds, 1, 0) == 1; #endif diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h index 1209c0faa5..d2cf792cf3 100644 --- a/src/consensus/tx_verify.h +++ b/src/consensus/tx_verify.h @@ -63,8 +63,8 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime); /** * Calculates the block height and previous block's median time past at * which the transaction will be considered final in the context of BIP 68. - * Also removes from the vector of input heights any entries which did not - * correspond to sequence locked inputs as they do not affect the calculation. + * For each input that is not sequence locked, the corresponding entries in + * prevHeights are set to 0 as they do not affect the calculation. */ std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags, std::vector<int>& prevHeights, const CBlockIndex& block); diff --git a/src/crypto/siphash.cpp b/src/crypto/siphash.cpp index 2e90c393e1..2f7555d02e 100644 --- a/src/crypto/siphash.cpp +++ b/src/crypto/siphash.cpp @@ -119,10 +119,10 @@ uint64_t SipHashUint256(uint64_t k0, uint64_t k1, const uint256& val) SIPROUND; SIPROUND; v0 ^= d; - v3 ^= ((uint64_t)4) << 59; + v3 ^= (uint64_t{4}) << 59; SIPROUND; SIPROUND; - v0 ^= ((uint64_t)4) << 59; + v0 ^= (uint64_t{4}) << 59; v2 ^= 0xFF; SIPROUND; SIPROUND; @@ -159,7 +159,7 @@ uint64_t SipHashUint256Extra(uint64_t k0, uint64_t k1, const uint256& val, uint3 SIPROUND; SIPROUND; v0 ^= d; - d = (((uint64_t)36) << 56) | extra; + d = ((uint64_t{36}) << 56) | extra; v3 ^= d; SIPROUND; SIPROUND; diff --git a/src/cuckoocache.h b/src/cuckoocache.h index 61f553806e..6adcc74516 100644 --- a/src/cuckoocache.h +++ b/src/cuckoocache.h @@ -344,7 +344,7 @@ public: collection_flags.setup(size); epoch_flags.resize(size); // Set to 45% as described above - epoch_size = std::max((uint32_t)1, (45 * size) / 100); + epoch_size = std::max(uint32_t{1}, (45 * size) / 100); // Initially set to wait for a whole epoch epoch_heuristic_counter = epoch_size; return size; diff --git a/src/init.cpp b/src/init.cpp index bd0df90dbd..5160718eaa 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -126,8 +126,9 @@ using node::fPruneMode; using node::fReindex; using node::nPruneTarget; -static const bool DEFAULT_PROXYRANDOMIZE = true; -static const bool DEFAULT_REST_ENABLE = false; +static constexpr bool DEFAULT_PROXYRANDOMIZE{true}; +static constexpr bool DEFAULT_REST_ENABLE{false}; +static constexpr bool DEFAULT_I2P_ACCEPT_INCOMING{true}; #ifdef WIN32 // Win32 LevelDB doesn't use filedescriptors, and the ones used for @@ -473,7 +474,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target per 24h. Limit does not apply to peers with 'download' permission or blocks created within past week. 0 = no limit (default: %s). Optional suffix units [k|K|m|M|g|G|t|T] (default: M). Lowercase is 1000 base while uppercase is 1024 base", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-i2psam=<ip:port>", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); - argsman.AddArg("-i2pacceptincoming", "If set and -i2psam is also set then incoming I2P connections are accepted via the SAM proxy. If this is not set but -i2psam is set then only outgoing connections will be made to the I2P network. Ignored if -i2psam is not set. Listening for incoming I2P connections is done through the SAM proxy, not by binding to a local address and port (default: 1)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); + argsman.AddArg("-i2pacceptincoming", strprintf("Whether to accept inbound I2P connections (default: %i). Ignored if -i2psam is not set. Listening for inbound I2P connections is done through the SAM proxy, not by binding to a local address and port.", DEFAULT_I2P_ACCEPT_INCOMING), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-onlynet=<net>", "Make automatic outbound connections only to network <net> (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. It can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); @@ -1829,7 +1830,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) SetReachable(NET_I2P, false); } - connOptions.m_i2p_accept_incoming = args.GetBoolArg("-i2pacceptincoming", true); + connOptions.m_i2p_accept_incoming = args.GetBoolArg("-i2pacceptincoming", DEFAULT_I2P_ACCEPT_INCOMING); if (!node.connman->Start(*node.scheduler, connOptions)) { return false; diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 734bf0b70a..a3fa753a98 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -268,8 +268,8 @@ public: { public: virtual ~Notifications() {} - virtual void transactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) {} - virtual void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) {} + virtual void transactionAddedToMempool(const CTransactionRef& tx) {} + virtual void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {} virtual void blockConnected(const BlockInfo& block) {} virtual void blockDisconnected(const BlockInfo& block) {} virtual void updatedBlockTip() {} diff --git a/src/netaddress.cpp b/src/netaddress.cpp index 71a3580615..782b692d30 100644 --- a/src/netaddress.cpp +++ b/src/netaddress.cpp @@ -179,7 +179,7 @@ bool CNetAddr::SetInternal(const std::string &name) } namespace torv3 { -// https://gitweb.torproject.org/torspec.git/tree/rend-spec-v3.txt#n2135 +// https://gitweb.torproject.org/torspec.git/tree/rend-spec-v3.txt?id=7116c9cdaba248aae07a3f1d0e15d9dd102f62c5#n2175 static constexpr size_t CHECKSUM_LEN = 2; static const unsigned char VERSION[] = {3}; static constexpr size_t TOTAL_LEN = ADDR_TORV3_SIZE + CHECKSUM_LEN + sizeof(VERSION); diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 7fd5ffd6a6..4f3dc99bbf 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -420,11 +420,11 @@ public: virtual ~NotificationsProxy() = default; void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) override { - m_notifications->transactionAddedToMempool(tx, mempool_sequence); + m_notifications->transactionAddedToMempool(tx); } void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override { - m_notifications->transactionRemovedFromMempool(tx, reason, mempool_sequence); + m_notifications->transactionRemovedFromMempool(tx, reason); } void BlockConnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* index) override { @@ -672,12 +672,9 @@ public: if (!m_node.mempool) return true; LockPoints lp; CTxMemPoolEntry entry(tx, 0, 0, 0, false, 0, lp); - CTxMemPool::setEntries ancestors; const CTxMemPool::Limits& limits{m_node.mempool->m_limits}; - std::string unused_error_string; LOCK(m_node.mempool->cs); - return m_node.mempool->CalculateMemPoolAncestors( - entry, ancestors, limits, unused_error_string); + return m_node.mempool->CalculateMemPoolAncestors(entry, limits).has_value(); } CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc) override { @@ -779,7 +776,7 @@ public: if (!m_node.mempool) return; LOCK2(::cs_main, m_node.mempool->cs); for (const CTxMemPoolEntry& entry : m_node.mempool->mapTx) { - notifications.transactionAddedToMempool(entry.GetSharedTx(), 0 /* mempool_sequence */); + notifications.transactionAddedToMempool(entry.GetSharedTx()); } } bool hasAssumedValidChain() override diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 7179d1a341..dc6849e0d2 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -394,9 +394,7 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele continue; } - CTxMemPool::setEntries ancestors; - std::string dummy; - mempool.CalculateMemPoolAncestors(*iter, ancestors, CTxMemPool::Limits::NoLimits(), dummy, false); + auto ancestors{mempool.AssumeCalculateMemPoolAncestors(__func__, *iter, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)}; onlyUnconfirmed(ancestors); ancestors.insert(iter); diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 10f85b9510..d032b74008 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -22,8 +22,6 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) { AssertLockHeld(pool.cs); - CTxMemPool::setEntries ancestors; - // First check the transaction itself. if (SignalsOptInRBF(tx)) { return RBFTransactionState::REPLACEABLE_BIP125; @@ -37,9 +35,9 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) // If all the inputs have nSequence >= maxint-1, it still might be // signaled for RBF if any unconfirmed parents have signaled. - std::string dummy; - CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash()); - pool.CalculateMemPoolAncestors(entry, ancestors, CTxMemPool::Limits::NoLimits(), dummy, false); + const CTxMemPoolEntry entry{*pool.mapTx.find(tx.GetHash())}; + auto ancestors{pool.AssumeCalculateMemPoolAncestors(__func__, entry, CTxMemPool::Limits::NoLimits(), + /*fSearchForParents=*/false)}; for (CTxMemPool::txiter it : ancestors) { if (SignalsOptInRBF(it->GetTx())) { diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 1f36af31f4..097b2b0364 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -212,7 +212,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact return AmountExceedsBalance; } - { + try { CAmount nFeeRequired = 0; int nChangePosRet = -1; @@ -240,6 +240,11 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact if (nFeeRequired > m_wallet->getDefaultMaxTxFee()) { return AbsurdFee; } + } catch (const std::runtime_error& err) { + // Something unexpected happened, instruct user to report this bug. + Q_EMIT message(tr("Send Coins"), QString::fromStdString(err.what()), + CClientUIInterface::MSG_ERROR); + return TransactionCreationFailed; } return SendCoinsReturn(OK); diff --git a/src/random.h b/src/random.h index 911aac08ca..6223da0377 100644 --- a/src/random.h +++ b/src/random.h @@ -200,7 +200,7 @@ public: return rand64() >> (64 - bits); } else { if (bitbuf_size < bits) FillBitBuffer(); - uint64_t ret = bitbuf & (~(uint64_t)0 >> (64 - bits)); + uint64_t ret = bitbuf & (~uint64_t{0} >> (64 - bits)); bitbuf >>= bits; bitbuf_size -= bits; return ret; diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 7a0c361ae0..44bff55f96 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -23,6 +23,8 @@ #include <util/moneystr.h> #include <util/time.h> +#include <utility> + using kernel::DumpMempool; using node::DEFAULT_MAX_RAW_TX_FEE_RATE; @@ -124,6 +126,10 @@ static RPCHelpMan testmempoolaccept() {RPCResult::Type::OBJ, "fees", /*optional=*/true, "Transaction fees (only present if 'allowed' is true)", { {RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT}, + {RPCResult::Type::STR_AMOUNT, "effective-feerate", /*optional=*/false, "the effective feerate in " + CURRENCY_UNIT + " per KvB. May differ from the base feerate if, for example, there are modified fees from prioritisetransaction or a package feerate was used."}, + {RPCResult::Type::ARR, "effective-includes", /*optional=*/false, "transactions whose fees and vsizes are included in effective-feerate.", + {RPCResult{RPCResult::Type::STR_HEX, "", "transaction wtxid in hex"}, + }}, }}, {RPCResult::Type::STR, "reject-reason", /*optional=*/true, "Rejection string (only present when 'allowed' is false)"}, }}, @@ -215,6 +221,12 @@ static RPCHelpMan testmempoolaccept() result_inner.pushKV("vsize", virtual_size); UniValue fees(UniValue::VOBJ); fees.pushKV("base", ValueFromAmount(fee)); + fees.pushKV("effective-feerate", ValueFromAmount(tx_result.m_effective_feerate.value().GetFeePerK())); + UniValue effective_includes_res(UniValue::VARR); + for (const auto& wtxid : tx_result.m_wtxids_fee_calculations.value()) { + effective_includes_res.push_back(wtxid.ToString()); + } + fees.pushKV("effective-includes", effective_includes_res); result_inner.pushKV("fees", fees); } } else { @@ -449,19 +461,17 @@ static RPCHelpMan getmempoolancestors() throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction not in mempool"); } - CTxMemPool::setEntries setAncestors; - std::string dummy; - mempool.CalculateMemPoolAncestors(*it, setAncestors, CTxMemPool::Limits::NoLimits(), dummy, false); + auto ancestors{mempool.AssumeCalculateMemPoolAncestors(__func__, *it, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)}; if (!fVerbose) { UniValue o(UniValue::VARR); - for (CTxMemPool::txiter ancestorIt : setAncestors) { + for (CTxMemPool::txiter ancestorIt : ancestors) { o.push_back(ancestorIt->GetTx().GetHash().ToString()); } return o; } else { UniValue o(UniValue::VOBJ); - for (CTxMemPool::txiter ancestorIt : setAncestors) { + for (CTxMemPool::txiter ancestorIt : ancestors) { const CTxMemPoolEntry &e = *ancestorIt; const uint256& _hash = e.GetTx().GetHash(); UniValue info(UniValue::VOBJ); @@ -768,10 +778,13 @@ static RPCHelpMan submitpackage() {RPCResult::Type::NUM, "vsize", "Virtual transaction size as defined in BIP 141."}, {RPCResult::Type::OBJ, "fees", "Transaction fees", { {RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT}, + {RPCResult::Type::STR_AMOUNT, "effective-feerate", /*optional=*/true, "if the transaction was not already in the mempool, the effective feerate in " + CURRENCY_UNIT + " per KvB. For example, the package feerate and/or feerate with modified fees from prioritisetransaction."}, + {RPCResult::Type::ARR, "effective-includes", /*optional=*/true, "if effective-feerate is provided, the wtxids of the transactions whose fees and vsizes are included in effective-feerate.", + {{RPCResult::Type::STR_HEX, "", "transaction wtxid in hex"}, + }}, }}, }} }}, - {RPCResult::Type::STR_AMOUNT, "package-feerate", /*optional=*/true, "package feerate used for feerate checks in " + CURRENCY_UNIT + " per KvB. Excludes transactions which were deduplicated or accepted individually."}, {RPCResult::Type::ARR, "replaced-transactions", /*optional=*/true, "List of txids of replaced transactions", { {RPCResult::Type::STR_HEX, "", "The transaction id"}, @@ -856,6 +869,7 @@ static RPCHelpMan submitpackage() CHECK_NONFATAL(it != package_result.m_tx_results.end()); UniValue result_inner{UniValue::VOBJ}; result_inner.pushKV("txid", tx->GetHash().GetHex()); + const auto& tx_result = it->second; if (it->second.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS) { result_inner.pushKV("other-wtxid", it->second.m_other_wtxid.value().GetHex()); } @@ -864,6 +878,17 @@ static RPCHelpMan submitpackage() result_inner.pushKV("vsize", int64_t{it->second.m_vsize.value()}); UniValue fees(UniValue::VOBJ); fees.pushKV("base", ValueFromAmount(it->second.m_base_fees.value())); + if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID) { + // Effective feerate is not provided for MEMPOOL_ENTRY transactions even + // though modified fees is known, because it is unknown whether package + // feerate was used when it was originally submitted. + fees.pushKV("effective-feerate", ValueFromAmount(tx_result.m_effective_feerate.value().GetFeePerK())); + UniValue effective_includes_res(UniValue::VARR); + for (const auto& wtxid : tx_result.m_wtxids_fee_calculations.value()) { + effective_includes_res.push_back(wtxid.ToString()); + } + fees.pushKV("effective-includes", effective_includes_res); + } result_inner.pushKV("fees", fees); if (it->second.m_replaced_transactions.has_value()) { for (const auto& ptx : it->second.m_replaced_transactions.value()) { @@ -874,9 +899,6 @@ static RPCHelpMan submitpackage() tx_result_map.pushKV(tx->GetWitnessHash().GetHex(), result_inner); } rpc_result.pushKV("tx-results", tx_result_map); - if (package_result.m_package_feerate.has_value()) { - rpc_result.pushKV("package-feerate", ValueFromAmount(package_result.m_package_feerate.value().GetFeePerK())); - } UniValue replaced_list(UniValue::VARR); for (const uint256& hash : replaced_txids) replaced_list.push_back(hash.ToString()); rpc_result.pushKV("replaced-transactions", replaced_list); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 4611e03270..d430087358 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -741,6 +741,10 @@ static RPCHelpMan setban() const bool absolute{request.params[3].isNull() ? false : request.params[3].get_bool()}; + if (absolute && banTime < GetTime()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: Absolute timestamp is in the past"); + } + if (isSubnet) { node.banman->Ban(subNet, banTime, absolute); if (node.connman) { diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index b15fc2a576..a942ff349b 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1303,7 +1303,7 @@ public: // Serialize the nSequence if (nInput != nIn && (fHashSingle || fHashNone)) // let the others update at will - ::Serialize(s, (int)0); + ::Serialize(s, int{0}); else ::Serialize(s, txTo.vin[nInput].nSequence); } diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index faf6055b67..53fbc26e15 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -194,7 +194,7 @@ static void Correct_Queue_range(std::vector<size_t> range) BOOST_AUTO_TEST_CASE(test_CheckQueue_Correct_Zero) { std::vector<size_t> range; - range.push_back((size_t)0); + range.push_back(size_t{0}); Correct_Queue_range(range); } /** Test that 1 check is correct @@ -202,7 +202,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Correct_Zero) BOOST_AUTO_TEST_CASE(test_CheckQueue_Correct_One) { std::vector<size_t> range; - range.push_back((size_t)1); + range.push_back(size_t{1}); Correct_Queue_range(range); } /** Test that MAX check is correct diff --git a/src/test/crypto_tests.cpp b/src/test/crypto_tests.cpp index 6148edf115..9b369a5c50 100644 --- a/src/test/crypto_tests.cpp +++ b/src/test/crypto_tests.cpp @@ -723,14 +723,14 @@ BOOST_AUTO_TEST_CASE(countbits_tests) // Check handling of zero. BOOST_CHECK_EQUAL(CountBits(0), 0U); } else if (i < 10) { - for (uint64_t j = (uint64_t)1 << (i - 1); (j >> i) == 0; ++j) { + for (uint64_t j = uint64_t{1} << (i - 1); (j >> i) == 0; ++j) { // Exhaustively test up to 10 bits BOOST_CHECK_EQUAL(CountBits(j), i); } } else { for (int k = 0; k < 1000; k++) { // Randomly test 1000 samples of each length above 10 bits. - uint64_t j = ((uint64_t)1) << (i - 1) | ctx.randbits(i - 1); + uint64_t j = (uint64_t{1}) << (i - 1) | ctx.randbits(i - 1); BOOST_CHECK_EQUAL(CountBits(j), i); } } diff --git a/src/test/fuzz/p2p_transport_serialization.cpp b/src/test/fuzz/p2p_transport_serialization.cpp index 3eb370a858..96254aa222 100644 --- a/src/test/fuzz/p2p_transport_serialization.cpp +++ b/src/test/fuzz/p2p_transport_serialization.cpp @@ -24,7 +24,7 @@ void initialize_p2p_transport_serialization() FUZZ_TARGET_INIT(p2p_transport_serialization, initialize_p2p_transport_serialization) { // Construct deserializer, with a dummy NodeId - V1TransportDeserializer deserializer{Params(), (NodeId)0, SER_NETWORK, INIT_PROTO_VERSION}; + V1TransportDeserializer deserializer{Params(), NodeId{0}, SER_NETWORK, INIT_PROTO_VERSION}; V1TransportSerializer serializer{}; FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp index 95eb71099d..143027662f 100644 --- a/src/test/fuzz/versionbits.cpp +++ b/src/test/fuzz/versionbits.cpp @@ -55,7 +55,7 @@ public: bool Condition(int32_t version) const { - uint32_t mask = ((uint32_t)1) << m_bit; + uint32_t mask = (uint32_t{1}) << m_bit; return (((version & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) && (version & mask) != 0); } diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index 1a24448569..7e9079743e 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -202,10 +202,9 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) tx7.vout[1].scriptPubKey = CScript() << OP_11 << OP_EQUAL; tx7.vout[1].nValue = 1 * COIN; - CTxMemPool::setEntries setAncestorsCalculated; - std::string dummy; - BOOST_CHECK_EQUAL(pool.CalculateMemPoolAncestors(entry.Fee(2'000'000LL).FromTx(tx7), setAncestorsCalculated, CTxMemPool::Limits::NoLimits(), dummy), true); - BOOST_CHECK(setAncestorsCalculated == 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); @@ -261,9 +260,9 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) tx10.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; tx10.vout[0].nValue = 10 * COIN; - setAncestorsCalculated.clear(); - BOOST_CHECK_EQUAL(pool.CalculateMemPoolAncestors(entry.Fee(200'000LL).Time(NodeSeconds{4s}).FromTx(tx10), setAncestorsCalculated, CTxMemPool::Limits::NoLimits(), dummy), true); - BOOST_CHECK(setAncestorsCalculated == setAncestors); + 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/merkle_tests.cpp b/src/test/merkle_tests.cpp index 942d54ede8..bba103d1b0 100644 --- a/src/test/merkle_tests.cpp +++ b/src/test/merkle_tests.cpp @@ -50,7 +50,7 @@ static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot // For each of the lower bits in count that are 0, do 1 step. Each // corresponds to an inner value that existed before processing the // current leaf, and each needs a hash to combine it. - for (level = 0; !(count & (((uint32_t)1) << level)); level++) { + for (level = 0; !(count & ((uint32_t{1}) << level)); level++) { if (pbranch) { if (matchh) { pbranch->push_back(inner[level]); @@ -74,12 +74,12 @@ static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot int level = 0; // As long as bit number level in count is zero, skip it. It means there // is nothing left at this level. - while (!(count & (((uint32_t)1) << level))) { + while (!(count & ((uint32_t{1}) << level))) { level++; } uint256 h = inner[level]; bool matchh = matchlevel == level; - while (count != (((uint32_t)1) << level)) { + while (count != ((uint32_t{1}) << level)) { // If we reach this point, h is an inner value that is not the top. // We combine it with itself (Bitcoin's special rule for odd levels in // the tree) to produce a higher level one. @@ -89,10 +89,10 @@ static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot CHash256().Write(h).Write(h).Finalize(h); // Increment count to the value it would have if two entries at this // level had existed. - count += (((uint32_t)1) << level); + count += ((uint32_t{1}) << level); level++; // And propagate the result upwards accordingly. - while (!(count & (((uint32_t)1) << level))) { + while (!(count & ((uint32_t{1}) << level))) { if (pbranch) { if (matchh) { pbranch->push_back(inner[level]); diff --git a/src/test/random_tests.cpp b/src/test/random_tests.cpp index 54d5ab6794..db659af0b8 100644 --- a/src/test/random_tests.cpp +++ b/src/test/random_tests.cpp @@ -94,7 +94,7 @@ BOOST_AUTO_TEST_CASE(fastrandom_randbits) for (int j = 0; j < 1000; ++j) { uint64_t rangebits = ctx1.randbits(bits); BOOST_CHECK_EQUAL(rangebits >> bits, 0U); - uint64_t range = ((uint64_t)1) << bits | rangebits; + uint64_t range = (uint64_t{1}) << bits | rangebits; uint64_t rand = ctx2.randrange(range); BOOST_CHECK(rand < range); } diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp index 5f8e353dd6..c90ae38ae8 100644 --- a/src/test/serialize_tests.cpp +++ b/src/test/serialize_tests.cpp @@ -123,17 +123,17 @@ BOOST_AUTO_TEST_CASE(varints_bitpatterns) CDataStream ss(SER_DISK, 0); ss << VARINT_MODE(0, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "00"); ss.clear(); ss << VARINT_MODE(0x7f, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "7f"); ss.clear(); - ss << VARINT_MODE((int8_t)0x7f, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "7f"); ss.clear(); + ss << VARINT_MODE(int8_t{0x7f}, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "7f"); ss.clear(); ss << VARINT_MODE(0x80, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "8000"); ss.clear(); - ss << VARINT((uint8_t)0x80); BOOST_CHECK_EQUAL(HexStr(ss), "8000"); ss.clear(); + ss << VARINT(uint8_t{0x80}); BOOST_CHECK_EQUAL(HexStr(ss), "8000"); ss.clear(); ss << VARINT_MODE(0x1234, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "a334"); ss.clear(); - ss << VARINT_MODE((int16_t)0x1234, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "a334"); ss.clear(); + ss << VARINT_MODE(int16_t{0x1234}, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "a334"); ss.clear(); ss << VARINT_MODE(0xffff, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "82fe7f"); ss.clear(); - ss << VARINT((uint16_t)0xffff); BOOST_CHECK_EQUAL(HexStr(ss), "82fe7f"); ss.clear(); + ss << VARINT(uint16_t{0xffff}); BOOST_CHECK_EQUAL(HexStr(ss), "82fe7f"); ss.clear(); ss << VARINT_MODE(0x123456, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "c7e756"); ss.clear(); - ss << VARINT_MODE((int32_t)0x123456, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "c7e756"); ss.clear(); + ss << VARINT_MODE(int32_t{0x123456}, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "c7e756"); ss.clear(); ss << VARINT(0x80123456U); BOOST_CHECK_EQUAL(HexStr(ss), "86ffc7e756"); ss.clear(); - ss << VARINT((uint32_t)0x80123456U); BOOST_CHECK_EQUAL(HexStr(ss), "86ffc7e756"); ss.clear(); + ss << VARINT(uint32_t{0x80123456U}); BOOST_CHECK_EQUAL(HexStr(ss), "86ffc7e756"); ss.clear(); ss << VARINT(0xffffffff); BOOST_CHECK_EQUAL(HexStr(ss), "8efefefe7f"); ss.clear(); ss << VARINT_MODE(0x7fffffffffffffffLL, VarIntMode::NONNEGATIVE_SIGNED); BOOST_CHECK_EQUAL(HexStr(ss), "fefefefefefefefe7f"); ss.clear(); ss << VARINT(0xffffffffffffffffULL); BOOST_CHECK_EQUAL(HexStr(ss), "80fefefefefefefefe7f"); ss.clear(); diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index c4be3a4129..dce230ac10 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -144,10 +144,10 @@ BOOST_AUTO_TEST_CASE(bitstream_reader_writer) CDataStream data_copy(data); uint32_t serialized_int1; data >> serialized_int1; - BOOST_CHECK_EQUAL(serialized_int1, (uint32_t)0x7700C35A); // NOTE: Serialized as LE + BOOST_CHECK_EQUAL(serialized_int1, uint32_t{0x7700C35A}); // NOTE: Serialized as LE uint16_t serialized_int2; data >> serialized_int2; - BOOST_CHECK_EQUAL(serialized_int2, (uint16_t)0x1072); // NOTE: Serialized as LE + BOOST_CHECK_EQUAL(serialized_int2, uint16_t{0x1072}); // NOTE: Serialized as LE BitStreamReader<CDataStream> bit_reader(data_copy); BOOST_CHECK_EQUAL(bit_reader.Read(1), 0U); diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index b290a664f9..e438867d15 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -90,17 +90,21 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) const auto result_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, {tx_parent, tx_child}, /*test_accept=*/true); BOOST_CHECK_MESSAGE(result_parent_child.m_state.IsValid(), "Package validation unexpectedly failed: " << result_parent_child.m_state.GetRejectReason()); + BOOST_CHECK(result_parent_child.m_tx_results.size() == 2); auto it_parent = result_parent_child.m_tx_results.find(tx_parent->GetWitnessHash()); auto it_child = result_parent_child.m_tx_results.find(tx_child->GetWitnessHash()); BOOST_CHECK(it_parent != result_parent_child.m_tx_results.end()); BOOST_CHECK_MESSAGE(it_parent->second.m_state.IsValid(), "Package validation unexpectedly failed: " << it_parent->second.m_state.GetRejectReason()); + BOOST_CHECK(it_parent->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_parent)) == COIN); + BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().size(), 1); + BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().front(), tx_parent->GetWitnessHash()); BOOST_CHECK(it_child != result_parent_child.m_tx_results.end()); BOOST_CHECK_MESSAGE(it_child->second.m_state.IsValid(), "Package validation unexpectedly failed: " << it_child->second.m_state.GetRejectReason()); - BOOST_CHECK(result_parent_child.m_package_feerate.has_value()); - BOOST_CHECK(result_parent_child.m_package_feerate.value() == - CFeeRate(2 * COIN, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child))); + BOOST_CHECK(it_child->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_child)) == COIN); + BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().size(), 1); + BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().front(), tx_child->GetWitnessHash()); // A single, giant transaction submitted through ProcessNewPackage fails on single tx policy. CTransactionRef giant_ptx = create_placeholder_tx(999, 999); @@ -109,10 +113,10 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) BOOST_CHECK(result_single_large.m_state.IsInvalid()); BOOST_CHECK_EQUAL(result_single_large.m_state.GetResult(), PackageValidationResult::PCKG_TX); BOOST_CHECK_EQUAL(result_single_large.m_state.GetRejectReason(), "transaction failed"); + BOOST_CHECK(result_single_large.m_tx_results.size() == 1); auto it_giant_tx = result_single_large.m_tx_results.find(giant_ptx->GetWitnessHash()); BOOST_CHECK(it_giant_tx != result_single_large.m_tx_results.end()); BOOST_CHECK_EQUAL(it_giant_tx->second.m_state.GetRejectReason(), "tx-size"); - BOOST_CHECK(result_single_large.m_package_feerate == std::nullopt); // Check that mempool size hasn't changed. BOOST_CHECK_EQUAL(m_node.mempool->size(), initialPoolSize); @@ -233,7 +237,6 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetRejectReason(), "package-not-child-with-parents"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - BOOST_CHECK(result_unrelated_submit.m_package_feerate == std::nullopt); // Parent and Child (and Grandchild) Package Package package_parent_child; @@ -275,7 +278,30 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(result_3gen_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(result_3gen_submit.m_state.GetRejectReason(), "package-not-child-with-parents"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - BOOST_CHECK(result_3gen_submit.m_package_feerate == std::nullopt); + } + + // Parent and child package where transactions are invalid for reasons other than fee and + // missing inputs, so the package validation isn't expected to happen. + { + CScriptWitness bad_witness; + bad_witness.stack.push_back(std::vector<unsigned char>(1)); + CMutableTransaction mtx_parent_invalid{mtx_parent}; + mtx_parent_invalid.vin[0].scriptWitness = bad_witness; + CTransactionRef tx_parent_invalid = MakeTransactionRef(mtx_parent_invalid); + auto result_quit_early = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + {tx_parent_invalid, tx_child}, /*test_accept=*/ false); + BOOST_CHECK(result_quit_early.m_state.IsInvalid()); + BOOST_CHECK_EQUAL(result_quit_early.m_state.GetResult(), PackageValidationResult::PCKG_TX); + BOOST_CHECK(!result_quit_early.m_tx_results.empty()); + BOOST_CHECK_EQUAL(result_quit_early.m_tx_results.size(), 2); + auto it_parent = result_quit_early.m_tx_results.find(tx_parent_invalid->GetWitnessHash()); + auto it_child = result_quit_early.m_tx_results.find(tx_child->GetWitnessHash()); + BOOST_CHECK(it_parent != result_quit_early.m_tx_results.end()); + BOOST_CHECK(it_child != result_quit_early.m_tx_results.end()); + BOOST_CHECK_EQUAL(it_parent->second.m_state.GetResult(), TxValidationResult::TX_WITNESS_MUTATED); + BOOST_CHECK_EQUAL(it_parent->second.m_state.GetRejectReason(), "bad-witness-nonstandard"); + BOOST_CHECK_EQUAL(it_child->second.m_state.GetResult(), TxValidationResult::TX_MISSING_INPUTS); + BOOST_CHECK_EQUAL(it_child->second.m_state.GetRejectReason(), "bad-txns-inputs-missingorspent"); } // Child with missing parent. @@ -290,8 +316,6 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetRejectReason(), "package-not-child-with-unconfirmed-parents"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - - BOOST_CHECK(result_missing_parent.m_package_feerate == std::nullopt); } // Submit package with parent + child. @@ -301,20 +325,23 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) expected_pool_size += 2; BOOST_CHECK_MESSAGE(submit_parent_child.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_parent_child.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_parent_child.m_tx_results.size(), package_parent_child.size()); auto it_parent = submit_parent_child.m_tx_results.find(tx_parent->GetWitnessHash()); auto it_child = submit_parent_child.m_tx_results.find(tx_child->GetWitnessHash()); BOOST_CHECK(it_parent != submit_parent_child.m_tx_results.end()); BOOST_CHECK(it_parent->second.m_state.IsValid()); + BOOST_CHECK(it_parent->second.m_effective_feerate == CFeeRate(1 * COIN, GetVirtualTransactionSize(*tx_parent))); + BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().size(), 1); + BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().front(), tx_parent->GetWitnessHash()); BOOST_CHECK(it_child != submit_parent_child.m_tx_results.end()); BOOST_CHECK(it_child->second.m_state.IsValid()); + BOOST_CHECK(it_child->second.m_effective_feerate == CFeeRate(1 * COIN, GetVirtualTransactionSize(*tx_child))); + BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().size(), 1); + BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().front(), tx_child->GetWitnessHash()); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash()))); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child->GetHash()))); - - // Since both transactions have high feerates, they each passed validation individually. - // Package validation was unnecessary, so there is no package feerate. - BOOST_CHECK(submit_parent_child.m_package_feerate == std::nullopt); } // Already-in-mempool transactions should be detected and de-duplicated. @@ -323,6 +350,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) package_parent_child, /*test_accept=*/false); BOOST_CHECK_MESSAGE(submit_deduped.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_deduped.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_deduped.m_tx_results.size(), package_parent_child.size()); auto it_parent_deduped = submit_deduped.m_tx_results.find(tx_parent->GetWitnessHash()); auto it_child_deduped = submit_deduped.m_tx_results.find(tx_child->GetWitnessHash()); BOOST_CHECK(it_parent_deduped != submit_deduped.m_tx_results.end()); @@ -335,8 +363,6 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash()))); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child->GetHash()))); - - BOOST_CHECK(submit_deduped.m_package_feerate == std::nullopt); } } @@ -399,6 +425,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) {ptx_parent, ptx_child1}, /*test_accept=*/false); BOOST_CHECK_MESSAGE(submit_witness1.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_witness1.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_witness1.m_tx_results.size(), 2); auto it_parent1 = submit_witness1.m_tx_results.find(ptx_parent->GetWitnessHash()); auto it_child1 = submit_witness1.m_tx_results.find(ptx_child1->GetWitnessHash()); BOOST_CHECK(it_parent1 != submit_witness1.m_tx_results.end()); @@ -412,13 +439,11 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child1->GetHash()))); // Child2 would have been validated individually. - BOOST_CHECK(submit_witness1.m_package_feerate == std::nullopt); - const auto submit_witness2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, {ptx_parent, ptx_child2}, /*test_accept=*/false); - BOOST_CHECK(submit_witness2.m_package_feerate == std::nullopt); BOOST_CHECK_MESSAGE(submit_witness2.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_witness2.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_witness2.m_tx_results.size(), 2); auto it_parent2_deduped = submit_witness2.m_tx_results.find(ptx_parent->GetWitnessHash()); auto it_child2 = submit_witness2.m_tx_results.find(ptx_child2->GetWitnessHash()); BOOST_CHECK(it_parent2_deduped != submit_witness2.m_tx_results.end()); @@ -436,11 +461,11 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) {ptx_parent, ptx_child1}, /*test_accept=*/false); BOOST_CHECK_MESSAGE(submit_segwit_dedup.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_segwit_dedup.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_segwit_dedup.m_tx_results.size(), 2); auto it_parent_dup = submit_segwit_dedup.m_tx_results.find(ptx_parent->GetWitnessHash()); auto it_child_dup = submit_segwit_dedup.m_tx_results.find(ptx_child1->GetWitnessHash()); BOOST_CHECK(it_parent_dup->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); BOOST_CHECK(it_child_dup->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); - BOOST_CHECK(submit_witness2.m_package_feerate == std::nullopt); } // Try submitting Package1{child2, grandchild} where child2 is same-txid-different-witness as @@ -465,6 +490,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) {ptx_child2, ptx_grandchild}, /*test_accept=*/false); BOOST_CHECK_MESSAGE(submit_spend_ignored.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_spend_ignored.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_spend_ignored.m_tx_results.size(), 2); auto it_child2_ignored = submit_spend_ignored.m_tx_results.find(ptx_child2->GetWitnessHash()); auto it_grandchild = submit_spend_ignored.m_tx_results.find(ptx_grandchild->GetWitnessHash()); BOOST_CHECK(it_child2_ignored != submit_spend_ignored.m_tx_results.end()); @@ -475,9 +501,6 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child2->GetHash()))); BOOST_CHECK(!m_node.mempool->exists(GenTxid::Wtxid(ptx_child2->GetWitnessHash()))); BOOST_CHECK(m_node.mempool->exists(GenTxid::Wtxid(ptx_grandchild->GetWitnessHash()))); - - // Since child2 is ignored, grandchild would be validated individually. - BOOST_CHECK(submit_spend_ignored.m_package_feerate == std::nullopt); } // A package Package{parent1, parent2, parent3, child} where the parents are a mixture of @@ -568,6 +591,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) { const auto mixed_result = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_mixed, false); BOOST_CHECK_MESSAGE(mixed_result.m_state.IsValid(), mixed_result.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(mixed_result.m_tx_results.size(), package_mixed.size()); auto it_parent1 = mixed_result.m_tx_results.find(ptx_parent1->GetWitnessHash()); auto it_parent2 = mixed_result.m_tx_results.find(ptx_parent2_v1->GetWitnessHash()); auto it_parent3 = mixed_result.m_tx_results.find(ptx_parent3->GetWitnessHash()); @@ -590,11 +614,12 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_mixed_child->GetHash()))); // package feerate should include parent3 and child. It should not include parent1 or parent2_v1. - BOOST_CHECK(mixed_result.m_package_feerate.has_value()); const CFeeRate expected_feerate(1 * COIN, GetVirtualTransactionSize(*ptx_parent3) + GetVirtualTransactionSize(*ptx_mixed_child)); - BOOST_CHECK_MESSAGE(mixed_result.m_package_feerate.value() == expected_feerate, - strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), - mixed_result.m_package_feerate.value().ToString())); + BOOST_CHECK(it_parent3->second.m_effective_feerate.value() == expected_feerate); + BOOST_CHECK(it_child->second.m_effective_feerate.value() == expected_feerate); + std::vector<uint256> expected_wtxids({ptx_parent3->GetWitnessHash(), ptx_mixed_child->GetWitnessHash()}); + BOOST_CHECK(it_parent3->second.m_wtxids_fee_calculations.value() == expected_wtxids); + BOOST_CHECK(it_child->second.m_wtxids_fee_calculations.value() == expected_wtxids); } } @@ -638,16 +663,12 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); const auto submit_cpfp_deprio = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_cpfp, /*test_accept=*/ false); + BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_MESSAGE(submit_cpfp_deprio.m_state.IsInvalid(), "Package validation unexpectedly succeeded: " << submit_cpfp_deprio.m_state.GetRejectReason()); BOOST_CHECK(submit_cpfp_deprio.m_tx_results.empty()); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); const CFeeRate expected_feerate(0, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child)); - BOOST_CHECK(submit_cpfp_deprio.m_package_feerate.has_value()); - BOOST_CHECK(submit_cpfp_deprio.m_package_feerate.value() == CFeeRate{0}); - BOOST_CHECK_MESSAGE(submit_cpfp_deprio.m_package_feerate.value() == expected_feerate, - strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), - submit_cpfp_deprio.m_package_feerate.value().ToString())); } // Clear the prioritisation of the parent transaction. @@ -662,6 +683,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) expected_pool_size += 2; BOOST_CHECK_MESSAGE(submit_cpfp.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_cpfp.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_cpfp.m_tx_results.size(), package_cpfp.size()); auto it_parent = submit_cpfp.m_tx_results.find(tx_parent->GetWitnessHash()); auto it_child = submit_cpfp.m_tx_results.find(tx_child->GetWitnessHash()); BOOST_CHECK(it_parent != submit_cpfp.m_tx_results.end()); @@ -677,11 +699,12 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) const CFeeRate expected_feerate(coinbase_value - child_value, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child)); + BOOST_CHECK(it_parent->second.m_effective_feerate.value() == expected_feerate); + BOOST_CHECK(it_child->second.m_effective_feerate.value() == expected_feerate); + std::vector<uint256> expected_wtxids({tx_parent->GetWitnessHash(), tx_child->GetWitnessHash()}); + BOOST_CHECK(it_parent->second.m_wtxids_fee_calculations.value() == expected_wtxids); + BOOST_CHECK(it_child->second.m_wtxids_fee_calculations.value() == expected_wtxids); BOOST_CHECK(expected_feerate.GetFeePerK() > 1000); - BOOST_CHECK(submit_cpfp.m_package_feerate.has_value()); - BOOST_CHECK_MESSAGE(submit_cpfp.m_package_feerate.value() == expected_feerate, - strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), - submit_cpfp.m_package_feerate.value().ToString())); } // Just because we allow low-fee parents doesn't mean we allow low-feerate packages. @@ -716,10 +739,6 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) const CFeeRate expected_feerate(200, GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)); BOOST_CHECK(expected_feerate.GetFeePerK() < 1000); - BOOST_CHECK(submit_package_too_low.m_package_feerate.has_value()); - BOOST_CHECK_MESSAGE(submit_package_too_low.m_package_feerate.value() == expected_feerate, - strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), - submit_package_too_low.m_package_feerate.value().ToString())); } // Package feerate includes the modified fees of the transactions. @@ -734,10 +753,20 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) "Package validation unexpectedly failed" << submit_prioritised_package.m_state.GetRejectReason()); const CFeeRate expected_feerate(1 * COIN + 200, GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)); - BOOST_CHECK(submit_prioritised_package.m_package_feerate.has_value()); - BOOST_CHECK_MESSAGE(submit_prioritised_package.m_package_feerate.value() == expected_feerate, - strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), - submit_prioritised_package.m_package_feerate.value().ToString())); + BOOST_CHECK_EQUAL(submit_prioritised_package.m_tx_results.size(), package_still_too_low.size()); + auto it_parent = submit_prioritised_package.m_tx_results.find(tx_parent_cheap->GetWitnessHash()); + auto it_child = submit_prioritised_package.m_tx_results.find(tx_child_cheap->GetWitnessHash()); + BOOST_CHECK(it_parent != submit_prioritised_package.m_tx_results.end()); + BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK(it_parent->second.m_base_fees.value() == 0); + BOOST_CHECK(it_parent->second.m_effective_feerate.value() == expected_feerate); + BOOST_CHECK(it_child != submit_prioritised_package.m_tx_results.end()); + BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK(it_child->second.m_base_fees.value() == 200); + BOOST_CHECK(it_child->second.m_effective_feerate.value() == expected_feerate); + std::vector<uint256> expected_wtxids({tx_parent_cheap->GetWitnessHash(), tx_child_cheap->GetWitnessHash()}); + BOOST_CHECK(it_parent->second.m_wtxids_fee_calculations.value() == expected_wtxids); + BOOST_CHECK(it_child->second.m_wtxids_fee_calculations.value() == expected_wtxids); } // Package feerate is calculated without topology in mind; it's just aggregating fees and sizes. @@ -770,10 +799,6 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK_MESSAGE(submit_rich_parent.m_state.IsInvalid(), "Package validation unexpectedly succeeded"); // The child would have been validated on its own and failed, then submitted as a "package" of 1. - // The package feerate is just the child's feerate, which is 0sat/vb. - BOOST_CHECK(submit_rich_parent.m_package_feerate.has_value()); - BOOST_CHECK_MESSAGE(submit_rich_parent.m_package_feerate.value() == CFeeRate(), - "expected 0, got " << submit_rich_parent.m_package_feerate.value().ToString()); BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetRejectReason(), "package-fee-too-low"); @@ -783,6 +808,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK(it_parent->second.m_state.GetRejectReason() == ""); BOOST_CHECK_MESSAGE(it_parent->second.m_base_fees.value() == high_parent_fee, strprintf("rich parent: expected fee %s, got %s", high_parent_fee, it_parent->second.m_base_fees.value())); + BOOST_CHECK(it_parent->second.m_effective_feerate == CFeeRate(high_parent_fee, GetVirtualTransactionSize(*tx_parent_rich))); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_rich->GetHash()))); diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index 9a9ba8ea51..52c0b1d938 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -77,7 +77,9 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup) LOCK(cs_main); BOOST_CHECK(m_node.chainman->ActiveChain().Tip()->GetBlockHash() != block.GetHash()); } - m_node.mempool->clear(); + BOOST_CHECK_EQUAL(m_node.mempool->size(), 1U); + WITH_LOCK(m_node.mempool->cs, m_node.mempool->removeRecursive(CTransaction{spends[0]}, MemPoolRemovalReason::CONFLICT)); + BOOST_CHECK_EQUAL(m_node.mempool->size(), 0U); // Test 3: ... and should be rejected if spend2 is in the memory pool BOOST_CHECK(ToMemPool(spends[1])); @@ -86,7 +88,9 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup) LOCK(cs_main); BOOST_CHECK(m_node.chainman->ActiveChain().Tip()->GetBlockHash() != block.GetHash()); } - m_node.mempool->clear(); + BOOST_CHECK_EQUAL(m_node.mempool->size(), 1U); + WITH_LOCK(m_node.mempool->cs, m_node.mempool->removeRecursive(CTransaction{spends[1]}, MemPoolRemovalReason::CONFLICT)); + BOOST_CHECK_EQUAL(m_node.mempool->size(), 0U); // Final sanity test: first spend in *m_node.mempool, second in block, that's OK: std::vector<CMutableTransaction> oneSpend; diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 4d04c076e3..6b6bb18523 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -810,8 +810,8 @@ BOOST_AUTO_TEST_CASE(test_ParseInt64) BOOST_CHECK(ParseInt64("01234", &n) && n == 1234LL); // no octal BOOST_CHECK(ParseInt64("2147483647", &n) && n == 2147483647LL); BOOST_CHECK(ParseInt64("-2147483648", &n) && n == -2147483648LL); - BOOST_CHECK(ParseInt64("9223372036854775807", &n) && n == (int64_t)9223372036854775807); - BOOST_CHECK(ParseInt64("-9223372036854775808", &n) && n == (int64_t)-9223372036854775807-1); + BOOST_CHECK(ParseInt64("9223372036854775807", &n) && n == int64_t{9223372036854775807}); + BOOST_CHECK(ParseInt64("-9223372036854775808", &n) && n == int64_t{-9223372036854775807-1}); BOOST_CHECK(ParseInt64("-1234", &n) && n == -1234LL); // Invalid values BOOST_CHECK(!ParseInt64("", &n)); @@ -907,8 +907,8 @@ BOOST_AUTO_TEST_CASE(test_ParseUInt32) BOOST_CHECK(ParseUInt32("1234", &n) && n == 1234); BOOST_CHECK(ParseUInt32("01234", &n) && n == 1234); // no octal BOOST_CHECK(ParseUInt32("2147483647", &n) && n == 2147483647); - BOOST_CHECK(ParseUInt32("2147483648", &n) && n == (uint32_t)2147483648); - BOOST_CHECK(ParseUInt32("4294967295", &n) && n == (uint32_t)4294967295); + BOOST_CHECK(ParseUInt32("2147483648", &n) && n == uint32_t{2147483648}); + BOOST_CHECK(ParseUInt32("4294967295", &n) && n == uint32_t{4294967295}); BOOST_CHECK(ParseUInt32("+1234", &n) && n == 1234); BOOST_CHECK(ParseUInt32("00000000000000001234", &n) && n == 1234); BOOST_CHECK(ParseUInt32("00000000000000000000", &n) && n == 0); diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index c0d59d6136..c40481a95c 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -32,7 +32,7 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches) COutPoint outp{txid, 0}; newcoin.nHeight = 1; newcoin.out.nValue = InsecureRand32(); - newcoin.out.scriptPubKey.assign((uint32_t)56, 1); + newcoin.out.scriptPubKey.assign(uint32_t{56}, 1); coins_view.AddCoin(outp, std::move(newcoin), false); return outp; diff --git a/src/test/validation_flush_tests.cpp b/src/test/validation_flush_tests.cpp index a006d715cc..f2ff570ca6 100644 --- a/src/test/validation_flush_tests.cpp +++ b/src/test/validation_flush_tests.cpp @@ -31,7 +31,7 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate) COutPoint outp{txid, 0}; newcoin.nHeight = 1; newcoin.out.nValue = InsecureRand32(); - newcoin.out.scriptPubKey.assign((uint32_t)56, 1); + newcoin.out.scriptPubKey.assign(uint32_t{56}, 1); coins_view.AddCoin(outp, std::move(newcoin), false); return outp; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 83020117a7..aa04f8a4d0 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -17,12 +17,16 @@ #include <util/check.h> #include <util/moneystr.h> #include <util/overflow.h> +#include <util/result.h> #include <util/system.h> #include <util/time.h> +#include <util/translation.h> #include <validationinterface.h> #include <cmath> #include <optional> +#include <string_view> +#include <utility> bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp) { @@ -147,32 +151,29 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256>& vHashes } } -bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size, - size_t entry_count, - setEntries& setAncestors, - CTxMemPoolEntry::Parents& staged_ancestors, - const Limits& limits, - std::string &errString) const +util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateAncestorsAndCheckLimits( + size_t entry_size, + size_t entry_count, + CTxMemPoolEntry::Parents& staged_ancestors, + const Limits& limits) const { size_t totalSizeWithAncestors = entry_size; + setEntries ancestors; while (!staged_ancestors.empty()) { const CTxMemPoolEntry& stage = staged_ancestors.begin()->get(); txiter stageit = mapTx.iterator_to(stage); - setAncestors.insert(stageit); + ancestors.insert(stageit); staged_ancestors.erase(stage); totalSizeWithAncestors += stageit->GetTxSize(); if (stageit->GetSizeWithDescendants() + entry_size > static_cast<uint64_t>(limits.descendant_size_vbytes)) { - errString = strprintf("exceeds descendant size limit for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_size_vbytes); - return false; + return util::Error{Untranslated(strprintf("exceeds descendant size limit for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_size_vbytes))}; } else if (stageit->GetCountWithDescendants() + entry_count > static_cast<uint64_t>(limits.descendant_count)) { - errString = strprintf("too many descendants for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_count); - return false; + return util::Error{Untranslated(strprintf("too many descendants for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_count))}; } else if (totalSizeWithAncestors > static_cast<uint64_t>(limits.ancestor_size_vbytes)) { - errString = strprintf("exceeds ancestor size limit [limit: %u]", limits.ancestor_size_vbytes); - return false; + return util::Error{Untranslated(strprintf("exceeds ancestor size limit [limit: %u]", limits.ancestor_size_vbytes))}; } const CTxMemPoolEntry::Parents& parents = stageit->GetMemPoolParentsConst(); @@ -180,17 +181,16 @@ bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size, txiter parent_it = mapTx.iterator_to(parent); // If this is a new ancestor, add it. - if (setAncestors.count(parent_it) == 0) { + if (ancestors.count(parent_it) == 0) { staged_ancestors.insert(parent); } - if (staged_ancestors.size() + setAncestors.size() + entry_count > static_cast<uint64_t>(limits.ancestor_count)) { - errString = strprintf("too many unconfirmed ancestors [limit: %u]", limits.ancestor_count); - return false; + if (staged_ancestors.size() + ancestors.size() + entry_count > static_cast<uint64_t>(limits.ancestor_count)) { + return util::Error{Untranslated(strprintf("too many unconfirmed ancestors [limit: %u]", limits.ancestor_count))}; } } } - return true; + return ancestors; } bool CTxMemPool::CheckPackageLimits(const Package& package, @@ -215,20 +215,17 @@ bool CTxMemPool::CheckPackageLimits(const Package& package, // When multiple transactions are passed in, the ancestors and descendants of all transactions // considered together must be within limits even if they are not interdependent. This may be // stricter than the limits for each individual transaction. - setEntries setAncestors; - const auto ret = CalculateAncestorsAndCheckLimits(total_size, package.size(), - setAncestors, staged_ancestors, - limits, errString); + const auto ancestors{CalculateAncestorsAndCheckLimits(total_size, package.size(), + staged_ancestors, limits)}; // It's possible to overestimate the ancestor/descendant totals. - if (!ret) errString.insert(0, "possibly "); - return ret; + if (!ancestors.has_value()) errString = "possibly " + util::ErrorString(ancestors).original; + return ancestors.has_value(); } -bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, - setEntries &setAncestors, - const Limits& limits, - std::string &errString, - bool fSearchForParents /* = true */) const +util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateMemPoolAncestors( + const CTxMemPoolEntry &entry, + const Limits& limits, + bool fSearchForParents /* = true */) const { CTxMemPoolEntry::Parents staged_ancestors; const CTransaction &tx = entry.GetTx(); @@ -242,8 +239,7 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, if (piter) { staged_ancestors.insert(**piter); if (staged_ancestors.size() + 1 > static_cast<uint64_t>(limits.ancestor_count)) { - errString = strprintf("too many unconfirmed parents [limit: %u]", limits.ancestor_count); - return false; + return util::Error{Untranslated(strprintf("too many unconfirmed parents [limit: %u]", limits.ancestor_count))}; } } } @@ -254,9 +250,22 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, staged_ancestors = it->GetMemPoolParentsConst(); } - return CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1, - setAncestors, staged_ancestors, - limits, errString); + return CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1, staged_ancestors, + limits); +} + +CTxMemPool::setEntries CTxMemPool::AssumeCalculateMemPoolAncestors( + std::string_view calling_fn_name, + const CTxMemPoolEntry &entry, + const Limits& limits, + bool fSearchForParents /* = true */) const +{ + auto result{CalculateMemPoolAncestors(entry, limits, fSearchForParents)}; + if (!Assume(result)) { + LogPrintLevel(BCLog::MEMPOOL, BCLog::Level::Error, "%s: CalculateMemPoolAncestors failed unexpectedly, continuing with empty ancestor set (%s)\n", + calling_fn_name, util::ErrorString(result).original); + } + return std::move(result).value_or(CTxMemPool::setEntries{}); } void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors) @@ -320,9 +329,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b } } for (txiter removeIt : entriesToRemove) { - setEntries setAncestors; const CTxMemPoolEntry &entry = *removeIt; - std::string dummy; // Since this is a tx that is already in the mempool, we can call CMPA // with fSearchForParents = false. If the mempool is in a consistent // state, then using true or false should both be correct, though false @@ -342,10 +349,10 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b // mempool parents we'd calculate by searching, and it's important that // we use the cached notion of ancestor transactions as the set of // things to update for removal. - CalculateMemPoolAncestors(entry, setAncestors, Limits::NoLimits(), dummy, false); + auto ancestors{AssumeCalculateMemPoolAncestors(__func__, entry, Limits::NoLimits(), /*fSearchForParents=*/false)}; // Note that UpdateAncestorsOf severs the child links that point to // removeIt in the entries for the parents of removeIt. - UpdateAncestorsOf(false, removeIt, setAncestors); + UpdateAncestorsOf(false, removeIt, ancestors); } // After updating all the ancestor sizes, we can now sever the link between each // transaction being removed and any mempool children (ie, update CTxMemPoolEntry::m_parents @@ -389,7 +396,6 @@ CTxMemPool::CTxMemPool(const Options& opts) m_full_rbf{opts.full_rbf}, m_limits{opts.limits} { - _clear(); //lock free clear } bool CTxMemPool::isSpent(const COutPoint& outpoint) const @@ -627,26 +633,6 @@ void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigne blockSinceLastRollingFeeBump = true; } -void CTxMemPool::_clear() -{ - vTxHashes.clear(); - mapTx.clear(); - mapNextTx.clear(); - totalTxSize = 0; - m_total_fee = 0; - cachedInnerUsage = 0; - lastRollingFeeUpdate = GetTime(); - blockSinceLastRollingFeeBump = false; - rollingMinimumFeeRate = 0; - ++nTransactionsUpdated; -} - -void CTxMemPool::clear() -{ - LOCK(cs); - _clear(); -} - void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendheight) const { if (m_check_ratio == 0) return; @@ -695,15 +681,13 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei assert(setParentCheck.size() == it->GetMemPoolParentsConst().size()); assert(std::equal(setParentCheck.begin(), setParentCheck.end(), it->GetMemPoolParentsConst().begin(), comp)); // Verify ancestor state is correct. - setEntries setAncestors; - std::string dummy; - CalculateMemPoolAncestors(*it, setAncestors, Limits::NoLimits(), dummy); - uint64_t nCountCheck = setAncestors.size() + 1; + auto ancestors{AssumeCalculateMemPoolAncestors(__func__, *it, Limits::NoLimits())}; + uint64_t nCountCheck = ancestors.size() + 1; uint64_t nSizeCheck = it->GetTxSize(); CAmount nFeesCheck = it->GetModifiedFee(); int64_t nSigOpCheck = it->GetSigOpCost(); - for (txiter ancestorIt : setAncestors) { + for (txiter ancestorIt : ancestors) { nSizeCheck += ancestorIt->GetTxSize(); nFeesCheck += ancestorIt->GetModifiedFee(); nSigOpCheck += ancestorIt->GetSigOpCost(); @@ -858,10 +842,8 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD if (it != mapTx.end()) { mapTx.modify(it, [&nFeeDelta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(nFeeDelta); }); // Now update all ancestors' modified fees with descendants - setEntries setAncestors; - std::string dummy; - CalculateMemPoolAncestors(*it, setAncestors, Limits::NoLimits(), dummy, false); - for (txiter ancestorIt : setAncestors) { + auto ancestors{AssumeCalculateMemPoolAncestors(__func__, *it, Limits::NoLimits(), /*fSearchForParents=*/false)}; + for (txiter ancestorIt : ancestors) { mapTx.modify(ancestorIt, [=](CTxMemPoolEntry& e){ e.UpdateDescendantState(0, nFeeDelta, 0);}); } // Now update all descendants' modified fees with ancestors @@ -998,10 +980,8 @@ int CTxMemPool::Expire(std::chrono::seconds time) void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, bool validFeeEstimate) { - setEntries setAncestors; - std::string dummy; - CalculateMemPoolAncestors(entry, setAncestors, Limits::NoLimits(), dummy); - return addUnchecked(entry, setAncestors, validFeeEstimate); + auto ancestors{AssumeCalculateMemPoolAncestors(__func__, entry, Limits::NoLimits())}; + return addUnchecked(entry, ancestors, validFeeEstimate); } void CTxMemPool::UpdateChild(txiter entry, txiter child, bool add) diff --git a/src/txmempool.h b/src/txmempool.h index bd62a09a4c..c3bc86cc04 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -11,6 +11,7 @@ #include <optional> #include <set> #include <string> +#include <string_view> #include <utility> #include <vector> @@ -28,6 +29,7 @@ #include <sync.h> #include <util/epochguard.h> #include <util/hasher.h> +#include <util/result.h> #include <boost/multi_index/hashed_index.hpp> #include <boost/multi_index/ordered_index.hpp> @@ -317,14 +319,14 @@ protected: std::atomic<unsigned int> nTransactionsUpdated{0}; //!< Used by getblocktemplate to trigger CreateNewBlock() invocation CBlockPolicyEstimator* const minerPolicyEstimator; - uint64_t totalTxSize GUARDED_BY(cs); //!< sum of all mempool tx's virtual sizes. Differs from serialized tx size since witness data is discounted. Defined in BIP 141. - CAmount m_total_fee GUARDED_BY(cs); //!< sum of all mempool tx's fees (NOT modified fee) - uint64_t cachedInnerUsage GUARDED_BY(cs); //!< sum of dynamic memory usage of all the map elements (NOT the maps themselves) + uint64_t totalTxSize GUARDED_BY(cs){0}; //!< sum of all mempool tx's virtual sizes. Differs from serialized tx size since witness data is discounted. Defined in BIP 141. + CAmount m_total_fee GUARDED_BY(cs){0}; //!< sum of all mempool tx's fees (NOT modified fee) + uint64_t cachedInnerUsage GUARDED_BY(cs){0}; //!< sum of dynamic memory usage of all the map elements (NOT the maps themselves) - mutable int64_t lastRollingFeeUpdate GUARDED_BY(cs); - mutable bool blockSinceLastRollingFeeBump GUARDED_BY(cs); - mutable double rollingMinimumFeeRate GUARDED_BY(cs); //!< minimum fee to get into the pool, decreases exponentially - mutable Epoch m_epoch GUARDED_BY(cs); + mutable int64_t lastRollingFeeUpdate GUARDED_BY(cs){GetTime()}; + mutable bool blockSinceLastRollingFeeBump GUARDED_BY(cs){false}; + mutable double rollingMinimumFeeRate GUARDED_BY(cs){0}; //!< minimum fee to get into the pool, decreases exponentially + mutable Epoch m_epoch GUARDED_BY(cs){}; // In-memory counter for external mempool tracking purposes. // This number is incremented once every time a transaction @@ -428,24 +430,20 @@ private: /** * Helper function to calculate all in-mempool ancestors of staged_ancestors and apply ancestor - * and descendant limits (including staged_ancestors thsemselves, entry_size and entry_count). + * and descendant limits (including staged_ancestors themselves, entry_size and entry_count). * * @param[in] entry_size Virtual size to include in the limits. * @param[in] entry_count How many entries to include in the limits. - * @param[out] setAncestors Will be populated with all mempool ancestors. * @param[in] staged_ancestors Should contain entries in the mempool. * @param[in] limits Maximum number and size of ancestors and descendants - * @param[out] errString Populated with error reason if any limits are hit * - * @return true if no limits were hit and all in-mempool ancestors were calculated, false - * otherwise + * @return all in-mempool ancestors, or an error if any ancestor or descendant limits were hit */ - bool CalculateAncestorsAndCheckLimits(size_t entry_size, - size_t entry_count, - setEntries& setAncestors, - CTxMemPoolEntry::Parents &staged_ancestors, - const Limits& limits, - std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs); + util::Result<setEntries> CalculateAncestorsAndCheckLimits(size_t entry_size, + size_t entry_count, + CTxMemPoolEntry::Parents &staged_ancestors, + const Limits& limits + ) const EXCLUSIVE_LOCKS_REQUIRED(cs); public: indirectmap<COutPoint, const CTransaction*> mapNextTx GUARDED_BY(cs); @@ -502,8 +500,6 @@ public: void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); - void clear(); - void _clear() EXCLUSIVE_LOCKS_REQUIRED(cs); //lock free bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb, bool wtxid=false); void queryHashes(std::vector<uint256>& vtxid) const; bool isSpent(const COutPoint& outpoint) const; @@ -558,22 +554,37 @@ public: * (these are all calculated including the tx itself) * * @param[in] entry CTxMemPoolEntry of which all in-mempool ancestors are calculated - * @param[out] setAncestors Will be populated with all mempool ancestors. * @param[in] limits Maximum number and size of ancestors and descendants - * @param[out] errString Populated with error reason if any limits are hit * @param[in] fSearchForParents Whether to search a tx's vin for in-mempool parents, or look * up parents from mapLinks. Must be true for entries not in * the mempool * - * @return true if no limits were hit and all in-mempool ancestors were calculated, false - * otherwise + * @return all in-mempool ancestors, or an error if any ancestor or descendant limits were hit */ - bool CalculateMemPoolAncestors(const CTxMemPoolEntry& entry, - setEntries& setAncestors, + util::Result<setEntries> CalculateMemPoolAncestors(const CTxMemPoolEntry& entry, const Limits& limits, - std::string& errString, bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); + /** + * Same as CalculateMemPoolAncestors, but always returns a (non-optional) setEntries. + * Should only be used when it is assumed CalculateMemPoolAncestors would not fail. If + * CalculateMemPoolAncestors does unexpectedly fail, an empty setEntries is returned and the + * error is logged to BCLog::MEMPOOL with level BCLog::Level::Error. In debug builds, failure + * of CalculateMemPoolAncestors will lead to shutdown due to assertion failure. + * + * @param[in] calling_fn_name Name of calling function so we can properly log the call site + * + * @return a setEntries corresponding to the result of CalculateMemPoolAncestors or an empty + * setEntries if it failed + * + * @see CTXMemPool::CalculateMemPoolAncestors() + */ + setEntries AssumeCalculateMemPoolAncestors( + std::string_view calling_fn_name, + const CTxMemPoolEntry &entry, + const Limits& limits, + bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Calculate all in-mempool ancestors of a set of transactions not already in the mempool and * check ancestor and descendant limits. Heuristics are used to estimate the ancestor and * descendant count of all entries if the package were to be added to the mempool. The limits diff --git a/src/util/moneystr.cpp b/src/util/moneystr.cpp index 9ecc899d0c..9181329afc 100644 --- a/src/util/moneystr.cpp +++ b/src/util/moneystr.cpp @@ -34,7 +34,7 @@ std::string FormatMoney(const CAmount n) str.erase(str.size()-nTrim, nTrim); if (n < 0) - str.insert((unsigned int)0, 1, '-'); + str.insert(uint32_t{0}, 1, '-'); return str; } diff --git a/src/validation.cpp b/src/validation.cpp index 441784a24f..aff86c4736 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -64,6 +64,7 @@ #include <numeric> #include <optional> #include <string> +#include <utility> using kernel::CCoinsStats; using kernel::CoinStatsHashType; @@ -582,6 +583,11 @@ private: /** Total virtual size of all transactions being replaced. */ size_t m_conflicting_size{0}; + /** If we're doing package validation (i.e. m_package_feerates=true), the "effective" + * package feerate of this transaction is the total fees divided by the total size of + * transactions (which may include its ancestors and/or descendants). */ + CFeeRate m_package_feerate{0}; + const CTransactionRef& m_ptx; /** Txid. */ const uint256& m_hash; @@ -867,11 +873,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) m_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants(); } - std::string errString; - if (!m_pool.CalculateMemPoolAncestors(*entry, ws.m_ancestors, m_limits, errString)) { - ws.m_ancestors.clear(); + auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, m_limits)}; + if (!ancestors) { // If CalculateMemPoolAncestors fails second time, we want the original error string. - std::string dummy_err_string; // Contracting/payment channels CPFP carve-out: // If the new transaction is relatively small (up to 40k weight) // and has at most one ancestor (ie ancestor limit of 2, including @@ -889,14 +893,16 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) .descendant_count = m_limits.descendant_count + 1, .descendant_size_vbytes = m_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT, }; - if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || - !m_pool.CalculateMemPoolAncestors(*entry, ws.m_ancestors, - cpfp_carve_out_limits, - dummy_err_string)) { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", errString); + const auto error_message{util::ErrorString(ancestors).original}; + 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); } + ws.m_ancestors = *ancestors; + // A transaction that spends outputs that would be replaced by it is invalid. Now // that we have the set of all ancestors we can detect this // pathological case by making sure ws.m_conflicts and ws.m_ancestors don't @@ -1111,15 +1117,18 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>& // Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the // last calculation done in PreChecks, since package ancestors have already been submitted. - std::string unused_err_string; - if(!m_pool.CalculateMemPoolAncestors(*ws.m_entry, ws.m_ancestors, m_limits, unused_err_string)) { - results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); - // Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail. - Assume(false); - all_submitted = false; - package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR, - strprintf("BUG! Mempool ancestors or descendants were underestimated: %s", - ws.m_ptx->GetHash().ToString())); + { + auto ancestors{m_pool.CalculateMemPoolAncestors(*ws.m_entry, m_limits)}; + if(!ancestors) { + results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); + // Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail. + Assume(false); + all_submitted = false; + package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR, + strprintf("BUG! Mempool ancestors or descendants were underestimated: %s", + ws.m_ptx->GetHash().ToString())); + } + ws.m_ancestors = std::move(ancestors).value_or(ws.m_ancestors); } // If we call LimitMempoolSize() for each individual Finalize(), the mempool will not take // the transaction's descendant feerate into account because it hasn't seen them yet. Also, @@ -1140,12 +1149,21 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>& // make sure we haven't exceeded max mempool size. LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); + std::vector<uint256> all_package_wtxids; + all_package_wtxids.reserve(workspaces.size()); + std::transform(workspaces.cbegin(), workspaces.cend(), std::back_inserter(all_package_wtxids), + [](const auto& ws) { return ws.m_ptx->GetWitnessHash(); }); // Find the wtxids of the transactions that made it into the mempool. Allow partial submission, // but don't report success unless they all made it into the mempool. for (Workspace& ws : workspaces) { + const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : + CFeeRate{ws.m_modified_fees, static_cast<uint32_t>(ws.m_vsize)}; + const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : + std::vector<uint256>({ws.m_ptx->GetWitnessHash()}); if (m_pool.exists(GenTxid::Wtxid(ws.m_ptx->GetWitnessHash()))) { results.emplace(ws.m_ptx->GetWitnessHash(), - MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees)); + MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, + ws.m_base_fees, effective_feerate, effective_feerate_wtxids)); GetMainSignals().TransactionAddedToMempool(ws.m_ptx, m_pool.GetAndIncrementSequence()); } else { all_submitted = false; @@ -1173,16 +1191,20 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef if (!ConsensusScriptChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); + const CFeeRate effective_feerate{ws.m_modified_fees, static_cast<uint32_t>(ws.m_vsize)}; + const std::vector<uint256> single_wtxid{ws.m_ptx->GetWitnessHash()}; // Tx was accepted, but not added if (args.m_test_accept) { - return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees); + return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, + ws.m_base_fees, effective_feerate, single_wtxid); } if (!Finalize(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); GetMainSignals().TransactionAddedToMempool(ptx, m_pool.GetAndIncrementSequence()); - return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees); + return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, + effective_feerate, single_wtxid); } PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::vector<CTransactionRef>& txns, ATMPArgs& args) @@ -1229,7 +1251,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: if (args.m_package_feerates && !CheckFeeRate(m_total_vsize, m_total_modified_fees, placeholder_state)) { package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-fee-too-low"); - return PackageMempoolAcceptResult(package_state, package_feerate, {}); + return PackageMempoolAcceptResult(package_state, {}); } // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, @@ -1237,51 +1259,60 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // transactions, but this exemption is not extended to packages in CheckPackageLimits(). std::string err_string; if (txns.size() > 1 && !PackageMempoolChecks(txns, package_state)) { - return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); + return PackageMempoolAcceptResult(package_state, std::move(results)); } + std::vector<uint256> all_package_wtxids; + all_package_wtxids.reserve(workspaces.size()); + std::transform(workspaces.cbegin(), workspaces.cend(), std::back_inserter(all_package_wtxids), + [](const auto& ws) { return ws.m_ptx->GetWitnessHash(); }); for (Workspace& ws : workspaces) { + ws.m_package_feerate = package_feerate; if (!PolicyScriptChecks(args, ws)) { // Exit early to avoid doing pointless work. Update the failed tx result; the rest are unfinished. package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); - return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); + return PackageMempoolAcceptResult(package_state, std::move(results)); } if (args.m_test_accept) { - // When test_accept=true, transactions that pass PolicyScriptChecks are valid because there are - // no further mempool checks (passing PolicyScriptChecks implies passing ConsensusScriptChecks). + const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : + CFeeRate{ws.m_modified_fees, static_cast<uint32_t>(ws.m_vsize)}; + const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : + std::vector<uint256>{ws.m_ptx->GetWitnessHash()}; results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), - ws.m_vsize, ws.m_base_fees)); + ws.m_vsize, ws.m_base_fees, effective_feerate, + effective_feerate_wtxids)); } } - if (args.m_test_accept) return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); + if (args.m_test_accept) return PackageMempoolAcceptResult(package_state, std::move(results)); if (!SubmitPackage(args, workspaces, package_state, results)) { // PackageValidationState filled in by SubmitPackage(). - return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); + return PackageMempoolAcceptResult(package_state, std::move(results)); } - return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); + return PackageMempoolAcceptResult(package_state, std::move(results)); } PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, ATMPArgs& args) { AssertLockHeld(cs_main); - PackageValidationState package_state; + // Used if returning a PackageMempoolAcceptResult directly from this function. + PackageValidationState package_state_quit_early; // Check that the package is well-formed. If it isn't, we won't try to validate any of the // transactions and thus won't return any MempoolAcceptResults, just a package-wide error. // Context-free package checks. - if (!CheckPackage(package, package_state)) return PackageMempoolAcceptResult(package_state, {}); + if (!CheckPackage(package, package_state_quit_early)) return PackageMempoolAcceptResult(package_state_quit_early, {}); // All transactions in the package must be a parent of the last transaction. This is just an // opportunity for us to fail fast on a context-free check without taking the mempool lock. if (!IsChildWithParents(package)) { - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents"); - return PackageMempoolAcceptResult(package_state, {}); + package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents"); + return PackageMempoolAcceptResult(package_state_quit_early, {}); } // IsChildWithParents() guarantees the package is > 1 transactions. @@ -1313,15 +1344,16 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, return unconfirmed_parent_txids.count(input.prevout.hash) > 0 || m_view.HaveCoin(input.prevout); }; if (!std::all_of(child->vin.cbegin(), child->vin.cend(), package_or_confirmed)) { - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents"); - return PackageMempoolAcceptResult(package_state, {}); + package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents"); + return PackageMempoolAcceptResult(package_state_quit_early, {}); } // Protect against bugs where we pull more inputs from disk that miss being added to // coins_to_uncache. The backend will be connected again when needed in PreChecks. m_view.SetBackend(m_dummy); LOCK(m_pool.cs); - std::map<const uint256, const MempoolAcceptResult> results; + // Stores final results that won't change + std::map<const uint256, const MempoolAcceptResult> results_final; // Node operators are free to set their mempool policies however they please, nodes may receive // transactions in different orders, and malicious counterparties may try to take advantage of // policy differences to pin or delay propagation of transactions. As such, it's possible for @@ -1331,8 +1363,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // the new transactions. This ensures we don't double-count transaction counts and sizes when // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy. ATMPArgs single_args = ATMPArgs::SingleInPackageAccept(args); + // Results from individual validation. "Nonfinal" because if a transaction fails by itself but + // succeeds later (i.e. when evaluated with a fee-bumping child), the result changes (though not + // reflected in this map). If a transaction fails more than once, we want to return the first + // result, when it was considered on its own. So changes will only be from invalid -> valid. + std::map<uint256, MempoolAcceptResult> individual_results_nonfinal; bool quit_early{false}; - std::vector<CTransactionRef> txns_new; + std::vector<CTransactionRef> txns_package_eval; for (const auto& tx : package) { const auto& wtxid = tx->GetWitnessHash(); const auto& txid = tx->GetHash(); @@ -1343,7 +1380,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // Exact transaction already exists in the mempool. auto iter = m_pool.GetIter(txid); assert(iter != std::nullopt); - results.emplace(wtxid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee())); + results_final.emplace(wtxid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee())); } else if (m_pool.exists(GenTxid::Txid(txid))) { // Transaction with the same non-witness data but different witness (same txid, // different wtxid) already exists in the mempool. @@ -1355,7 +1392,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, auto iter = m_pool.GetIter(txid); assert(iter != std::nullopt); // Provide the wtxid of the mempool tx so that the caller can look it up in the mempool. - results.emplace(wtxid, MempoolAcceptResult::MempoolTxDifferentWitness(iter.value()->GetTx().GetWitnessHash())); + results_final.emplace(wtxid, MempoolAcceptResult::MempoolTxDifferentWitness(iter.value()->GetTx().GetWitnessHash())); } else { // Transaction does not already exist in the mempool. // Try submitting the transaction on its own. @@ -1364,7 +1401,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // The transaction succeeded on its own and is now in the mempool. Don't include it // in package validation, because its fees should only be "used" once. assert(m_pool.exists(GenTxid::Wtxid(wtxid))); - results.emplace(wtxid, single_res); + results_final.emplace(wtxid, single_res); } else if (single_res.m_state.GetResult() != TxValidationResult::TX_MEMPOOL_POLICY && single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { // Package validation policy only differs from individual policy in its evaluation @@ -1377,24 +1414,40 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // future. Continue individually validating the rest of the transactions, because // some of them may still be valid. quit_early = true; + package_state_quit_early.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); + individual_results_nonfinal.emplace(wtxid, single_res); } else { - txns_new.push_back(tx); + individual_results_nonfinal.emplace(wtxid, single_res); + txns_package_eval.push_back(tx); } } } - // Nothing to do if the entire package has already been submitted. - if (quit_early || txns_new.empty()) { - // No package feerate when no package validation was done. - return PackageMempoolAcceptResult(package_state, std::move(results)); + // Quit early because package validation won't change the result or the entire package has + // already been submitted. + if (quit_early || txns_package_eval.empty()) { + for (const auto& [wtxid, mempoolaccept_res] : individual_results_nonfinal) { + Assume(results_final.emplace(wtxid, mempoolaccept_res).second); + Assume(mempoolaccept_res.m_result_type == MempoolAcceptResult::ResultType::INVALID); + } + return PackageMempoolAcceptResult(package_state_quit_early, std::move(results_final)); } - // Validate the (deduplicated) transactions as a package. - auto submission_result = AcceptMultipleTransactions(txns_new, args); + // Validate the (deduplicated) transactions as a package. Note that submission_result has its + // own PackageValidationState; package_state_quit_early is unused past this point. + auto submission_result = AcceptMultipleTransactions(txns_package_eval, args); // Include already-in-mempool transaction results in the final result. - for (const auto& [wtxid, mempoolaccept_res] : results) { - submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res); + for (const auto& [wtxid, mempoolaccept_res] : results_final) { + Assume(submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res).second); + Assume(mempoolaccept_res.m_result_type != MempoolAcceptResult::ResultType::INVALID); + } + if (submission_result.m_state.GetResult() == PackageValidationResult::PCKG_TX) { + // Package validation failed because one or more transactions failed. Provide a result for + // each transaction; if AcceptMultipleTransactions() didn't return a result for a tx, + // include the previous individual failure reason. + submission_result.m_tx_results.insert(individual_results_nonfinal.cbegin(), + individual_results_nonfinal.cend()); + Assume(submission_result.m_tx_results.size() == package.size()); } - if (submission_result.m_state.IsValid()) assert(submission_result.m_package_feerate.has_value()); return submission_result; } diff --git a/src/validation.h b/src/validation.h index 63ff9247be..900f380563 100644 --- a/src/validation.h +++ b/src/validation.h @@ -134,6 +134,19 @@ struct MempoolAcceptResult { const std::optional<int64_t> m_vsize; /** Raw base fees in satoshis. */ const std::optional<CAmount> m_base_fees; + /** The feerate at which this transaction was considered. This includes any fee delta added + * using prioritisetransaction (i.e. modified fees). If this transaction was submitted as a + * package, this is the package feerate, which may also include its descendants and/or + * ancestors (see m_wtxids_fee_calculations below). + * Only present when m_result_type = ResultType::VALID. + */ + const std::optional<CFeeRate> m_effective_feerate; + /** Contains the wtxids of the transactions used for fee-related checks. Includes this + * transaction's wtxid and may include others if this transaction was validated as part of a + * package. This is not necessarily equivalent to the list of transactions passed to + * ProcessNewPackage(). + * Only present when m_result_type = ResultType::VALID. */ + const std::optional<std::vector<uint256>> m_wtxids_fee_calculations; // The following field is only present when m_result_type = ResultType::DIFFERENT_WITNESS /** The wtxid of the transaction in the mempool which has the same txid but different witness. */ @@ -143,8 +156,13 @@ struct MempoolAcceptResult { return MempoolAcceptResult(state); } - static MempoolAcceptResult Success(std::list<CTransactionRef>&& replaced_txns, int64_t vsize, CAmount fees) { - return MempoolAcceptResult(std::move(replaced_txns), vsize, fees); + static MempoolAcceptResult Success(std::list<CTransactionRef>&& replaced_txns, + int64_t vsize, + CAmount fees, + CFeeRate effective_feerate, + const std::vector<uint256>& wtxids_fee_calculations) { + return MempoolAcceptResult(std::move(replaced_txns), vsize, fees, + effective_feerate, wtxids_fee_calculations); } static MempoolAcceptResult MempoolTx(int64_t vsize, CAmount fees) { @@ -164,9 +182,17 @@ private: } /** Constructor for success case */ - explicit MempoolAcceptResult(std::list<CTransactionRef>&& replaced_txns, int64_t vsize, CAmount fees) + explicit MempoolAcceptResult(std::list<CTransactionRef>&& replaced_txns, + int64_t vsize, + CAmount fees, + CFeeRate effective_feerate, + const std::vector<uint256>& wtxids_fee_calculations) : m_result_type(ResultType::VALID), - m_replaced_transactions(std::move(replaced_txns)), m_vsize{vsize}, m_base_fees(fees) {} + m_replaced_transactions(std::move(replaced_txns)), + m_vsize{vsize}, + m_base_fees(fees), + m_effective_feerate(effective_feerate), + m_wtxids_fee_calculations(wtxids_fee_calculations) {} /** Constructor for already-in-mempool case. It wouldn't replace any transactions. */ explicit MempoolAcceptResult(int64_t vsize, CAmount fees) @@ -190,10 +216,6 @@ struct PackageMempoolAcceptResult * was a package-wide error (see result in m_state), m_tx_results will be empty. */ std::map<const uint256, const MempoolAcceptResult> m_tx_results; - /** Package feerate, defined as the aggregated modified fees divided by the total virtual size - * of all transactions in the package. May be unavailable if some inputs were not available or - * a transaction failure caused validation to terminate early. */ - std::optional<CFeeRate> m_package_feerate; explicit PackageMempoolAcceptResult(PackageValidationState state, std::map<const uint256, const MempoolAcceptResult>&& results) @@ -201,7 +223,7 @@ struct PackageMempoolAcceptResult explicit PackageMempoolAcceptResult(PackageValidationState state, CFeeRate feerate, std::map<const uint256, const MempoolAcceptResult>&& results) - : m_state{state}, m_tx_results(std::move(results)), m_package_feerate{feerate} {} + : m_state{state}, m_tx_results(std::move(results)) {} /** Constructor to create a PackageMempoolAcceptResult from a single MempoolAcceptResult */ explicit PackageMempoolAcceptResult(const uint256& wtxid, const MempoolAcceptResult& result) diff --git a/src/versionbits.cpp b/src/versionbits.cpp index 8463704e7c..fa9d1fe9c9 100644 --- a/src/versionbits.cpp +++ b/src/versionbits.cpp @@ -195,7 +195,7 @@ protected: public: explicit VersionBitsConditionChecker(Consensus::DeploymentPos id_) : id(id_) {} - uint32_t Mask(const Consensus::Params& params) const { return ((uint32_t)1) << params.vDeployments[id].bit; } + uint32_t Mask(const Consensus::Params& params) const { return (uint32_t{1}) << params.vDeployments[id].bit; } }; } // namespace diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 06650a73cc..4ec3ac2189 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -100,7 +100,7 @@ void BerkeleyEnvironment::Close() if (ret != 0) LogPrintf("BerkeleyEnvironment::Close: Error %d closing database environment: %s\n", ret, DbEnv::strerror(ret)); if (!fMockDb) - DbEnv((uint32_t)0).remove(strPath.c_str(), 0); + DbEnv(uint32_t{0}).remove(strPath.c_str(), 0); if (error_file) fclose(error_file); diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 4fba567b7f..e6ba89627c 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -88,13 +88,15 @@ std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_poo std::vector<size_t> best_selection; CAmount best_waste = MAX_MONEY; + bool is_feerate_high = utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee; + // Depth First search loop for choosing the UTXOs for (size_t curr_try = 0, utxo_pool_index = 0; curr_try < TOTAL_TRIES; ++curr_try, ++utxo_pool_index) { // Conditions for starting a backtrack bool backtrack = false; if (curr_value + curr_available_value < selection_target || // Cannot possibly reach target with the amount remaining in the curr_available_value. curr_value > selection_target + cost_of_change || // Selected value is out of range, go back and try other branch - (curr_waste > best_waste && (utxo_pool.at(0).fee - utxo_pool.at(0).long_term_fee) > 0)) { // Don't select things which we know will be more wasteful if the waste is increasing + (curr_waste > best_waste && is_feerate_high)) { // Don't select things which we know will be more wasteful if the waste is increasing backtrack = true; } else if (curr_value >= selection_target) { // Selected value is within range curr_waste += (curr_value - selection_target); // This is the excess value which is added to the waste for the below comparison @@ -446,7 +448,8 @@ void SelectionResult::Clear() void SelectionResult::AddInput(const OutputGroup& group) { - util::insert(m_selected_inputs, group.m_outputs); + // As it can fail, combine inputs first + InsertInputs(group.m_outputs); m_use_effective = !group.m_subtract_fee_outputs; m_weight += group.m_weight; @@ -454,7 +457,8 @@ void SelectionResult::AddInput(const OutputGroup& group) void SelectionResult::AddInputs(const std::set<COutput>& inputs, bool subtract_fee_outputs) { - util::insert(m_selected_inputs, inputs); + // As it can fail, combine inputs first + InsertInputs(inputs); m_use_effective = !subtract_fee_outputs; m_weight += std::accumulate(inputs.cbegin(), inputs.cend(), 0, [](int sum, const auto& coin) { @@ -464,16 +468,14 @@ void SelectionResult::AddInputs(const std::set<COutput>& inputs, bool subtract_f void SelectionResult::Merge(const SelectionResult& other) { - // Obtain the expected selected inputs count after the merge (for now, duplicates are not allowed) - const size_t expected_count = m_selected_inputs.size() + other.m_selected_inputs.size(); + // As it can fail, combine inputs first + InsertInputs(other.m_selected_inputs); m_target += other.m_target; m_use_effective |= other.m_use_effective; if (m_algo == SelectionAlgorithm::MANUAL) { m_algo = other.m_algo; } - util::insert(m_selected_inputs, other.m_selected_inputs); - assert(m_selected_inputs.size() == expected_count); m_weight += other.m_weight; } diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 2efeb8476d..9ff2011ce3 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -10,6 +10,8 @@ #include <policy/feerate.h> #include <primitives/transaction.h> #include <random.h> +#include <util/system.h> +#include <util/check.h> #include <optional> @@ -186,6 +188,7 @@ struct CoinEligibilityFilter /** When avoid_reuse=true and there are full groups (OUTPUT_GROUP_MAX_ENTRIES), whether or not to use any partial groups.*/ const bool m_include_partial_groups{false}; + CoinEligibilityFilter() = delete; CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_ancestors) {} CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants) {} CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants, bool include_partial) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants), m_include_partial_groups(include_partial) {} @@ -301,6 +304,17 @@ private: /** Total weight of the selected inputs */ int m_weight{0}; + template<typename T> + void InsertInputs(const T& inputs) + { + // Store sum of combined input sets to check that the results have no shared UTXOs + const size_t expected_count = m_selected_inputs.size() + inputs.size(); + util::insert(m_selected_inputs, inputs); + if (m_selected_inputs.size() != expected_count) { + throw std::runtime_error(STR_INTERNAL_BUG("Shared UTXOs among selection results")); + } + } + public: explicit SelectionResult(const CAmount target, SelectionAlgorithm algo) : m_target(target), m_algo(algo) {} diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 7b49bad3f5..95e1ba4dd9 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -43,9 +43,7 @@ RPCHelpMan getnewaddress() } // Parse the label first so we don't generate a key if there's an error - std::string label; - if (!request.params[0].isNull()) - label = LabelFromValue(request.params[0]); + const std::string label{LabelFromValue(request.params[0])}; OutputType output_type = pwallet->m_default_address_type; if (!request.params[1].isNull()) { @@ -140,7 +138,7 @@ RPCHelpMan setlabel() throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address"); } - std::string label = LabelFromValue(request.params[1]); + const std::string label{LabelFromValue(request.params[1])}; if (pwallet->IsMine(dest)) { pwallet->SetAddressBook(dest, label, "receive"); @@ -258,9 +256,7 @@ RPCHelpMan addmultisigaddress() LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore); - std::string label; - if (!request.params[2].isNull()) - label = LabelFromValue(request.params[2]); + const std::string label{LabelFromValue(request.params[2])}; int required = request.params[0].getInt<int>(); @@ -662,7 +658,7 @@ RPCHelpMan getaddressesbylabel() LOCK(pwallet->cs_wallet); - std::string label = LabelFromValue(request.params[0]); + const std::string label{LabelFromValue(request.params[0])}; // Find all addresses that have the given label UniValue ret(UniValue::VOBJ); diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 95117b6c19..64253789ec 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -156,9 +156,7 @@ RPCHelpMan importprivkey() EnsureWalletIsUnlocked(*pwallet); std::string strSecret = request.params[0].get_str(); - std::string strLabel; - if (!request.params[1].isNull()) - strLabel = request.params[1].get_str(); + const std::string strLabel{LabelFromValue(request.params[1])}; // Whether to perform rescan after import if (!request.params[2].isNull()) @@ -249,9 +247,7 @@ RPCHelpMan importaddress() EnsureLegacyScriptPubKeyMan(*pwallet, true); - std::string strLabel; - if (!request.params[1].isNull()) - strLabel = request.params[1].get_str(); + const std::string strLabel{LabelFromValue(request.params[1])}; // Whether to perform rescan after import bool fRescan = true; @@ -442,9 +438,7 @@ RPCHelpMan importpubkey() EnsureLegacyScriptPubKeyMan(*pwallet, true); - std::string strLabel; - if (!request.params[1].isNull()) - strLabel = request.params[1].get_str(); + const std::string strLabel{LabelFromValue(request.params[1])}; // Whether to perform rescan after import bool fRescan = true; @@ -1170,7 +1164,7 @@ static UniValue ProcessImport(CWallet& wallet, const UniValue& data, const int64 if (internal && data.exists("label")) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Internal addresses should not have a label"); } - const std::string& label = data.exists("label") ? data["label"].get_str() : ""; + const std::string label{LabelFromValue(data["label"])}; const bool add_keypool = data.exists("keypool") ? data["keypool"].get_bool() : false; // Add to keypool only works with privkeys disabled @@ -1464,7 +1458,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c const std::string& descriptor = data["desc"].get_str(); const bool active = data.exists("active") ? data["active"].get_bool() : false; const bool internal = data.exists("internal") ? data["internal"].get_bool() : false; - const std::string& label = data.exists("label") ? data["label"].get_str() : ""; + const std::string label{LabelFromValue(data["label"])}; // Parse descriptor string FlatSigningProvider keys; diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 5f31c1d72c..be1ad37592 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -763,13 +763,17 @@ RPCHelpMan fundrawtransaction() }, {"input_weights", RPCArg::Type::ARR, RPCArg::Optional::OMITTED_NAMED_ARG, "Inputs and their corresponding weights", { - {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"}, - {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output index"}, - {"weight", RPCArg::Type::NUM, RPCArg::Optional::NO, "The maximum weight for this input, " - "including the weight of the outpoint and sequence number. " - "Note that serialized signature sizes are not guaranteed to be consistent, " - "so the maximum DER signatures size of 73 bytes should be used when considering ECDSA signatures." - "Remember to convert serialized sizes to weight units when necessary."}, + {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", + { + {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"}, + {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output index"}, + {"weight", RPCArg::Type::NUM, RPCArg::Optional::NO, "The maximum weight for this input, " + "including the weight of the outpoint and sequence number. " + "Note that serialized signature sizes are not guaranteed to be consistent, " + "so the maximum DER signatures size of 73 bytes should be used when considering ECDSA signatures." + "Remember to convert serialized sizes to weight units when necessary."}, + }, + }, }, }, }, diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index cff371c55b..f571f8bcb2 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -416,7 +416,6 @@ static const std::vector<RPCResult> TransactionDescriptionString() }}, {RPCResult::Type::STR_HEX, "replaced_by_txid", /*optional=*/true, "The txid if this tx was replaced."}, {RPCResult::Type::STR_HEX, "replaces_txid", /*optional=*/true, "The txid if the tx replaces one."}, - {RPCResult::Type::STR, "comment", /*optional=*/true, ""}, {RPCResult::Type::STR, "to", /*optional=*/true, "If a comment to is associated with the transaction."}, {RPCResult::Type::NUM_TIME, "time", "The transaction time expressed in " + UNIX_EPOCH_TIME + "."}, {RPCResult::Type::NUM_TIME, "timereceived", "The time received expressed in " + UNIX_EPOCH_TIME + "."}, @@ -487,7 +486,7 @@ RPCHelpMan listtransactions() std::optional<std::string> filter_label; if (!request.params[0].isNull() && request.params[0].get_str() != "*") { - filter_label = request.params[0].get_str(); + filter_label.emplace(LabelFromValue(request.params[0])); if (filter_label.value().empty()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Label argument must be a valid label name or \"*\"."); } @@ -635,10 +634,9 @@ RPCHelpMan listsinceblock() bool include_removed = (request.params[3].isNull() || request.params[3].get_bool()); bool include_change = (!request.params[4].isNull() && request.params[4].get_bool()); + // Only set it if 'label' was provided. std::optional<std::string> filter_label; - if (!request.params[5].isNull()) { - filter_label = request.params[5].get_str(); - } + if (!request.params[5].isNull()) filter_label.emplace(LabelFromValue(request.params[5])); int depth = height ? wallet.GetLastBlockHeight() + 1 - *height : -1; diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index b80f86ddc9..31435a69ba 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -132,7 +132,10 @@ const LegacyScriptPubKeyMan& EnsureConstLegacyScriptPubKeyMan(const CWallet& wal std::string LabelFromValue(const UniValue& value) { - std::string label = value.get_str(); + static const std::string empty_string; + if (value.isNull()) return empty_string; + + const std::string& label{value.get_str()}; if (label == "*") throw JSONRPCError(RPC_WALLET_INVALID_LABEL_NAME, "Invalid label name"); return label; diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index f0ecd13f26..aeab9b7634 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -226,6 +226,14 @@ static RPCHelpMan loadwallet() bilingual_str error; std::vector<bilingual_str> warnings; std::optional<bool> load_on_start = request.params[1].isNull() ? std::nullopt : std::optional<bool>(request.params[1].get_bool()); + + { + LOCK(context.wallets_mutex); + if (std::any_of(context.wallets.begin(), context.wallets.end(), [&name](const auto& wallet) { return wallet->GetName() == name; })) { + throw JSONRPCError(RPC_WALLET_ALREADY_LOADED, "Wallet \"" + name + "\" is already loaded."); + } + } + std::shared_ptr<CWallet> const wallet = LoadWallet(context, name, load_on_start, options, status, error, warnings); HandleWalletError(wallet, status, error); @@ -437,13 +445,20 @@ static RPCHelpMan unloadwallet() throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded"); } - // Release the "main" shared pointer and prevent further notifications. - // Note that any attempt to load the same wallet would fail until the wallet - // is destroyed (see CheckUniqueFileid). std::vector<bilingual_str> warnings; - std::optional<bool> load_on_start = request.params[1].isNull() ? std::nullopt : std::optional<bool>(request.params[1].get_bool()); - if (!RemoveWallet(context, wallet, load_on_start, warnings)) { - throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded"); + { + WalletRescanReserver reserver(*wallet); + if (!reserver.reserve()) { + throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); + } + + // Release the "main" shared pointer and prevent further notifications. + // Note that any attempt to load the same wallet would fail until the wallet + // is destroyed (see CheckUniqueFileid). + std::optional<bool> load_on_start = request.params[1].isNull() ? std::nullopt : std::optional<bool>(request.params[1].get_bool()); + if (!RemoveWallet(context, wallet, load_on_start, warnings)) { + throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded"); + } } UnloadWallet(std::move(wallet)); diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index fbb52ea621..d8f34dd2b0 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -2123,7 +2123,7 @@ bool DescriptorScriptPubKeyMan::TopUp(unsigned int size) if (size > 0) { target_size = size; } else { - target_size = std::max(gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 1); + target_size = std::max(gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), int64_t{1}); } // Calculate the new range_end diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 3bb3c17119..8bb970936b 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -287,7 +287,7 @@ CoinsResult AvailableCoins(const CWallet& wallet, if (coinControl && coinControl->HasSelected() && coinControl->IsSelected(outpoint)) continue; - if (wallet.IsLockedCoin(outpoint)) + if (wallet.IsLockedCoin(outpoint) && params.skip_locked) continue; if (wallet.IsSpent(outpoint)) @@ -510,15 +510,20 @@ std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<C return groups_out; } -std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, +// Returns true if the result contains an error and the message is not empty +static bool HasErrorMsg(const util::Result<SelectionResult>& res) { return !util::ErrorString(res).empty(); } + +util::Result<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types) { // Run coin selection on each OutputType and compute the Waste Metric std::vector<SelectionResult> results; for (const auto& it : available_coins.coins) { - if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, it.second, coin_selection_params)}) { - results.push_back(*result); - } + auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, it.second, coin_selection_params)}; + // If any specific error message appears here, then something particularly wrong happened. + if (HasErrorMsg(result)) return result; // So let's return the specific error. + // Append the favorable result. + if (result) results.push_back(*result); } // If we have at least one solution for funding the transaction without mixing, choose the minimum one according to waste metric // and return the result @@ -528,16 +533,14 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm // over all available coins, which would allow mixing. // If TypesCount() <= 1, there is nothing to mix. if (allow_mixed_output_types && available_coins.TypesCount() > 1) { - if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.All(), coin_selection_params)}) { - return result; - } + return ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.All(), coin_selection_params); } // Either mixing is not allowed and we couldn't find a solution from any single OutputType, or mixing was allowed and we still couldn't // find a solution using all available coins - return std::nullopt; + return util::Error(); }; -std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins, const CoinSelectionParams& coin_selection_params) +util::Result<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins, const CoinSelectionParams& coin_selection_params) { // Vector of results. We will choose the best one based on waste. std::vector<SelectionResult> results; @@ -561,7 +564,7 @@ std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, cons if (results.empty()) { // No solution found - return std::nullopt; + return util::Error(); } std::vector<SelectionResult> eligible_results; @@ -571,7 +574,8 @@ std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, cons }); if (eligible_results.empty()) { - return std::nullopt; + return util::Error{_("The inputs size exceeds the maximum weight. " + "Please try sending a smaller amount or manually consolidating your wallet's UTXOs")}; } // Choose the result with the least waste @@ -580,15 +584,18 @@ std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, cons return best_result; } -std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const PreSelectedInputs& pre_set_inputs, - const CAmount& nTargetValue, const CCoinControl& coin_control, - const CoinSelectionParams& coin_selection_params) +util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const PreSelectedInputs& pre_set_inputs, + const CAmount& nTargetValue, const CCoinControl& coin_control, + const CoinSelectionParams& coin_selection_params) { // Deduct preset inputs amount from the search target CAmount selection_target = nTargetValue - pre_set_inputs.total_amount; // Return if automatic coin selection is disabled, and we don't cover the selection target - if (!coin_control.m_allow_other_inputs && selection_target > 0) return std::nullopt; + if (!coin_control.m_allow_other_inputs && selection_target > 0) { + return util::Error{_("The preselected coins total amount does not cover the transaction target. " + "Please allow other inputs to be automatically selected or include more coins manually")}; + } // Return if we can cover the target only with the preset inputs if (selection_target <= 0) { @@ -603,7 +610,7 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a CAmount available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.GetTotalAmount() : (available_coins.GetEffectiveTotalAmount().has_value() ? *available_coins.GetEffectiveTotalAmount() : 0); if (selection_target > available_coins_total_amount) { - return std::nullopt; // Insufficient funds + return util::Error(); // Insufficient funds } // Start wallet Coin Selection procedure @@ -622,7 +629,12 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a return op_selection_result; } -std::optional<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& value_to_select, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) +struct SelectionFilter { + CoinEligibilityFilter filter; + bool allow_mixed_output_types{true}; +}; + +util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& value_to_select, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) { unsigned int limit_ancestor_count = 0; unsigned int limit_descendant_count = 0; @@ -643,54 +655,56 @@ std::optional<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coi // 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. - std::optional<SelectionResult> res = [&] { - // If possible, fund the transaction with confirmed UTXOs only. Prefer at least six - // confirmations on outputs received from other wallets and only spend confirmed change. - if (auto r1{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(1, 6, 0), available_coins, coin_selection_params, /*allow_mixed_output_types=*/false)}) return r1; - // Allow mixing only if no solution from any single output type can be found - if (auto r2{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(1, 1, 0), available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) return r2; - + 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 + // confirmations on outputs received from other wallets and only spend confirmed change. + {CoinEligibilityFilter(1, 6, 0), /*allow_mixed_output_types=*/false}, + {CoinEligibilityFilter(1, 1, 0)}, + }; // Fall back to using zero confirmation change (but with as few ancestors in the mempool as // possible) if we cannot fund the transaction otherwise. if (wallet.m_spend_zero_conf_change) { - if (auto r3{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, 2), available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) return r3; - if (auto r4{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), - available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) { - return r4; - } - if (auto r5{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), - available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) { - return r5; - } + ordered_filters.push_back({CoinEligibilityFilter(0, 1, 2)}); + ordered_filters.push_back({CoinEligibilityFilter(0, 1, std::min(size_t{4}, max_ancestors/3), std::min(size_t{4}, max_descendants/3))}); + ordered_filters.push_back({CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2)}); // If partial groups are allowed, relax the requirement of spending OutputGroups (groups // of UTXOs sent to the same address, which are obviously controlled by a single wallet) // in their entirety. - if (auto r6{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, /*include_partial=*/true), - available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) { - return r6; - } + ordered_filters.push_back({CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, /*include_partial=*/true)}); // Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs // received from other wallets. if (coin_control.m_include_unsafe_inputs) { - if (auto r7{AttemptSelection(wallet, value_to_select, - CoinEligibilityFilter(/*conf_mine=*/0, /*conf_theirs=*/0, max_ancestors-1, max_descendants-1, /*include_partial=*/true), - available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) { - return r7; - } + ordered_filters.push_back({CoinEligibilityFilter(/*conf_mine=*/0, /*conf_theirs*/0, max_ancestors-1, max_descendants-1, /*include_partial=*/true)}); } // Try with unlimited ancestors/descendants. The transaction will still need to meet // mempool ancestor/descendant policy to be accepted to mempool and broadcasted, but // OutputGroups use heuristics that may overestimate ancestor/descendant counts. if (!fRejectLongChains) { - if (auto r8{AttemptSelection(wallet, value_to_select, - CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max(), std::numeric_limits<uint64_t>::max(), /*include_partial=*/true), - available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) { - return r8; - } + ordered_filters.push_back({CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max(), + std::numeric_limits<uint64_t>::max(), + /*include_partial=*/true)}); + } + } + + // Walk-through the filters until the solution gets found. + // If no solution is found, return the first detailed error (if any). + // future: add "error level" so the worst one can be picked instead. + std::vector<util::Result<SelectionResult>> res_detailed_errors; + for (const auto& select_filter : ordered_filters) { + if (auto res{AttemptSelection(wallet, value_to_select, select_filter.filter, available_coins, + coin_selection_params, select_filter.allow_mixed_output_types)}) { + return res; // result found + } else { + // 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); } } // Coin Selection failed. - return std::optional<SelectionResult>(); + return res_detailed_errors.empty() ? util::Result<SelectionResult>(util::Error()) : res_detailed_errors.front(); }(); return res; @@ -917,13 +931,16 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( } // Choose coins to use - std::optional<SelectionResult> result = SelectCoins(wallet, available_coins, preset_inputs, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); - if (!result) { - return util::Error{_("Insufficient funds")}; + auto select_coins_res = SelectCoins(wallet, available_coins, preset_inputs, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); + if (!select_coins_res) { + // 'SelectCoins' either returns a specific error message or, if empty, means a general "Insufficient funds". + const bilingual_str& err = util::ErrorString(select_coins_res); + return util::Error{err.empty() ?_("Insufficient funds") : err}; } - TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result->GetAlgo()).c_str(), result->GetTarget(), result->GetWaste(), result->GetSelectedValue()); + const SelectionResult& result = *select_coins_res; + TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result.GetAlgo()).c_str(), result.GetTarget(), result.GetWaste(), result.GetSelectedValue()); - const CAmount change_amount = result->GetChange(coin_selection_params.min_viable_change, coin_selection_params.m_change_fee); + const CAmount change_amount = result.GetChange(coin_selection_params.min_viable_change, coin_selection_params.m_change_fee); if (change_amount > 0) { CTxOut newTxOut(change_amount, scriptChange); if (nChangePosInOut == -1) { @@ -938,7 +955,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( } // Shuffle selected coins and fill in final vin - std::vector<COutput> selected_coins = result->GetShuffledInputVector(); + std::vector<COutput> selected_coins = result.GetShuffledInputVector(); // The sequence number is set to non-maxint so that DiscourageFeeSniping // works. @@ -963,7 +980,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( CAmount fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); const CAmount output_value = CalculateOutputValue(txNew); Assume(recipients_sum + change_amount == output_value); - CAmount current_fee = result->GetSelectedValue() - output_value; + CAmount current_fee = result.GetSelectedValue() - output_value; // Sanity check that the fee cannot be negative as that means we have more output value than input value if (current_fee < 0) { @@ -974,7 +991,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( if (nChangePosInOut != -1 && fee_needed < current_fee) { auto& change = txNew.vout.at(nChangePosInOut); change.nValue += current_fee - fee_needed; - current_fee = result->GetSelectedValue() - CalculateOutputValue(txNew); + current_fee = result.GetSelectedValue() - CalculateOutputValue(txNew); if (fee_needed != current_fee) { return util::Error{Untranslated(STR_INTERNAL_BUG("Change adjustment: Fee needed != fee paid"))}; } @@ -1013,7 +1030,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( } ++i; } - current_fee = result->GetSelectedValue() - CalculateOutputValue(txNew); + current_fee = result.GetSelectedValue() - CalculateOutputValue(txNew); if (fee_needed != current_fee) { return util::Error{Untranslated(STR_INTERNAL_BUG("SFFO: Fee needed != fee paid"))}; } diff --git a/src/wallet/spend.h b/src/wallet/spend.h index c612ed105e..5ffdd11813 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -76,6 +76,8 @@ struct CoinFilterParams { bool only_spendable{true}; // By default, do not include immature coinbase outputs bool include_immature_coinbase{false}; + // By default, skip locked UTXOs + bool skip_locked{true}; }; /** @@ -119,9 +121,11 @@ std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<C * param@[in] coin_selection_params Parameters for the coin selection * param@[in] allow_mixed_output_types Relax restriction that SelectionResults must be of the same OutputType * returns If successful, a SelectionResult containing the input set - * If failed, a nullopt + * If failed, returns (1) an empty error message if the target was not reached (general "Insufficient funds") + * or (2) an specific error message if there was something particularly wrong (e.g. a selection + * result that surpassed the tx max weight size). */ -std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, +util::Result<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types); /** @@ -135,9 +139,11 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm * param@[in] available_coins The struct of coins, organized by OutputType, available for selection prior to filtering * param@[in] coin_selection_params Parameters for the coin selection * returns If successful, a SelectionResult containing the input set - * If failed, a nullopt + * If failed, returns (1) an empty error message if the target was not reached (general "Insufficient funds") + * or (2) an specific error message if there was something particularly wrong (e.g. a selection + * result that surpassed the tx max weight size). */ -std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins, +util::Result<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins, const CoinSelectionParams& coin_selection_params); // User manually selected inputs that must be part of the transaction @@ -175,18 +181,20 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const * param@[in] coin_selection_params Parameters for this coin selection such as feerates, whether to avoid partial spends, * and whether to subtract the fee from the outputs. * returns If successful, a SelectionResult containing the selected coins - * If failed, a nullopt. + * If failed, returns (1) an empty error message if the target was not reached (general "Insufficient funds") + * or (2) an specific error message if there was something particularly wrong (e.g. a selection + * result that surpassed the tx max weight size). */ -std::optional<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CCoinControl& coin_control, +util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); /** * Select all coins from coin_control, and if coin_control 'm_allow_other_inputs=true', call 'AutomaticCoinSelection' to * select a set of coins such that nTargetValue - pre_set_inputs.total_amount is met. */ -std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const PreSelectedInputs& pre_set_inputs, - const CAmount& nTargetValue, const CCoinControl& coin_control, - const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); +util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const PreSelectedInputs& pre_set_inputs, + const CAmount& nTargetValue, const CCoinControl& coin_control, + const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); struct CreatedTransactionResult { diff --git a/src/wallet/test/availablecoins_tests.cpp b/src/wallet/test/availablecoins_tests.cpp deleted file mode 100644 index 2427a343d5..0000000000 --- a/src/wallet/test/availablecoins_tests.cpp +++ /dev/null @@ -1,107 +0,0 @@ -// Copyright (c) 2022 The Bitcoin Core developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or https://www.opensource.org/licenses/mit-license.php. - -#include <validation.h> -#include <wallet/coincontrol.h> -#include <wallet/spend.h> -#include <wallet/test/util.h> -#include <wallet/test/wallet_test_fixture.h> - -#include <boost/test/unit_test.hpp> - -namespace wallet { -BOOST_FIXTURE_TEST_SUITE(availablecoins_tests, WalletTestingSetup) -class AvailableCoinsTestingSetup : public TestChain100Setup -{ -public: - AvailableCoinsTestingSetup() - { - CreateAndProcessBlock({}, {}); - wallet = CreateSyncedWallet(*m_node.chain, m_node.chainman->ActiveChain(), m_args, coinbaseKey); - } - - ~AvailableCoinsTestingSetup() - { - wallet.reset(); - } - CWalletTx& AddTx(CRecipient recipient) - { - CTransactionRef tx; - CCoinControl dummy; - { - constexpr int RANDOM_CHANGE_POSITION = -1; - auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, dummy); - BOOST_CHECK(res); - tx = res->tx; - } - wallet->CommitTransaction(tx, {}, {}); - CMutableTransaction blocktx; - { - LOCK(wallet->cs_wallet); - blocktx = CMutableTransaction(*wallet->mapWallet.at(tx->GetHash()).tx); - } - CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - - LOCK(wallet->cs_wallet); - LOCK(m_node.chainman->GetMutex()); - wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, m_node.chainman->ActiveChain().Tip()->GetBlockHash()); - auto it = wallet->mapWallet.find(tx->GetHash()); - BOOST_CHECK(it != wallet->mapWallet.end()); - it->second.m_state = TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*index=*/1}; - return it->second; - } - - std::unique_ptr<CWallet> wallet; -}; - -BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup) -{ - CoinsResult available_coins; - util::Result<CTxDestination> dest{util::Error{}}; - LOCK(wallet->cs_wallet); - - // Verify our wallet has one usable coinbase UTXO before starting - // This UTXO is a P2PK, so it should show up in the Other bucket - available_coins = AvailableCoins(*wallet); - BOOST_CHECK_EQUAL(available_coins.Size(), 1U); - BOOST_CHECK_EQUAL(available_coins.coins[OutputType::UNKNOWN].size(), 1U); - - // We will create a self transfer for each of the OutputTypes and - // verify it is put in the correct bucket after running GetAvailablecoins - // - // For each OutputType, We expect 2 UTXOs in our wallet following the self transfer: - // 1. One UTXO as the recipient - // 2. One UTXO from the change, due to payment address matching logic - - // Bech32m - dest = wallet->GetNewDestination(OutputType::BECH32M, ""); - BOOST_ASSERT(dest); - AddTx(CRecipient{{GetScriptForDestination(*dest)}, 1 * COIN, /*fSubtractFeeFromAmount=*/true}); - available_coins = AvailableCoins(*wallet); - BOOST_CHECK_EQUAL(available_coins.coins[OutputType::BECH32M].size(), 2U); - - // Bech32 - dest = wallet->GetNewDestination(OutputType::BECH32, ""); - BOOST_ASSERT(dest); - AddTx(CRecipient{{GetScriptForDestination(*dest)}, 2 * COIN, /*fSubtractFeeFromAmount=*/true}); - available_coins = AvailableCoins(*wallet); - BOOST_CHECK_EQUAL(available_coins.coins[OutputType::BECH32].size(), 2U); - - // P2SH-SEGWIT - dest = wallet->GetNewDestination(OutputType::P2SH_SEGWIT, ""); - BOOST_ASSERT(dest); - AddTx(CRecipient{{GetScriptForDestination(*dest)}, 3 * COIN, /*fSubtractFeeFromAmount=*/true}); - available_coins = AvailableCoins(*wallet); - BOOST_CHECK_EQUAL(available_coins.coins[OutputType::P2SH_SEGWIT].size(), 2U); - - // Legacy (P2PKH) - dest = wallet->GetNewDestination(OutputType::LEGACY, ""); - BOOST_ASSERT(dest); - AddTx(CRecipient{{GetScriptForDestination(*dest)}, 4 * COIN, /*fSubtractFeeFromAmount=*/true}); - available_coins = AvailableCoins(*wallet); - BOOST_CHECK_EQUAL(available_coins.coins[OutputType::LEGACY].size(), 2U); -} - -BOOST_AUTO_TEST_SUITE_END() -} // namespace wallet diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index fdd56101b4..2e12b5b1d4 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -121,9 +121,9 @@ static CAmount make_hard_case(int utxos, std::vector<COutput>& utxo_pool) utxo_pool.clear(); CAmount target = 0; for (int i = 0; i < utxos; ++i) { - target += (CAmount)1 << (utxos+i); - add_coin((CAmount)1 << (utxos+i), 2*i, utxo_pool); - add_coin(((CAmount)1 << (utxos+i)) + ((CAmount)1 << (utxos-1-i)), 2*i + 1, utxo_pool); + target += CAmount{1} << (utxos+i); + add_coin(CAmount{1} << (utxos+i), 2*i, utxo_pool); + add_coin((CAmount{1} << (utxos+i)) + (CAmount{1} << (utxos-1-i)), 2*i + 1, utxo_pool); } return target; } @@ -302,6 +302,8 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_selection_params_bnb.m_change_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_output_size); coin_selection_params_bnb.m_cost_of_change = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size) + coin_selection_params_bnb.m_change_fee; coin_selection_params_bnb.min_viable_change = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size); + coin_selection_params_bnb.m_subtract_fee_outputs = true; + { std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase()); wallet->LoadWallet(); @@ -319,7 +321,6 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) available_coins.Clear(); add_coin(available_coins, *wallet, 1 * CENT, coin_selection_params_bnb.m_effective_feerate); available_coins.All().at(0).input_bytes = 40; - coin_selection_params_bnb.m_subtract_fee_outputs = true; const auto result9 = SelectCoinsBnB(GroupCoins(available_coins.All()), 1 * CENT, coin_selection_params_bnb.m_cost_of_change); BOOST_CHECK(result9); BOOST_CHECK_EQUAL(result9->GetSelectedValue(), 1 * CENT); @@ -931,7 +932,7 @@ BOOST_AUTO_TEST_CASE(effective_value_test) BOOST_CHECK_EQUAL(output5.GetEffectiveValue(), nValue); // The effective value should be equal to the absolute value if input_bytes is -1 } -static std::optional<SelectionResult> select_coins(const CAmount& target, const CoinSelectionParams& cs_params, const CCoinControl& cc, std::function<CoinsResult(CWallet&)> coin_setup, interfaces::Chain* chain, const ArgsManager& args) +static util::Result<SelectionResult> select_coins(const CAmount& target, const CoinSelectionParams& cs_params, const CCoinControl& cc, std::function<CoinsResult(CWallet&)> coin_setup, interfaces::Chain* chain, const ArgsManager& args) { std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(chain, "", args, CreateMockWalletDatabase()); wallet->LoadWallet(); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 7ae10c23ca..b6e50e961a 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -622,6 +622,46 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup) BOOST_CHECK_EQUAL(list.begin()->second.size(), 2U); } +void TestCoinsResult(ListCoinsTest& context, OutputType out_type, CAmount amount, + std::map<OutputType, size_t>& expected_coins_sizes) +{ + LOCK(context.wallet->cs_wallet); + util::Result<CTxDestination> dest = Assert(context.wallet->GetNewDestination(out_type, "")); + CWalletTx& wtx = context.AddTx(CRecipient{{GetScriptForDestination(*dest)}, amount, /*fSubtractFeeFromAmount=*/true}); + CoinFilterParams filter; + filter.skip_locked = false; + CoinsResult available_coins = AvailableCoins(*context.wallet, nullptr, std::nullopt, filter); + // Lock outputs so they are not spent in follow-up transactions + for (uint32_t i = 0; i < wtx.tx->vout.size(); i++) context.wallet->LockCoin({wtx.GetHash(), i}); + for (const auto& [type, size] : expected_coins_sizes) BOOST_CHECK_EQUAL(size, available_coins.coins[type].size()); +} + +BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, ListCoinsTest) +{ + std::map<OutputType, size_t> expected_coins_sizes; + for (const auto& out_type : OUTPUT_TYPES) { expected_coins_sizes[out_type] = 0U; } + + // Verify our wallet has one usable coinbase UTXO before starting + // This UTXO is a P2PK, so it should show up in the Other bucket + expected_coins_sizes[OutputType::UNKNOWN] = 1U; + CoinsResult available_coins = WITH_LOCK(wallet->cs_wallet, return AvailableCoins(*wallet)); + BOOST_CHECK_EQUAL(available_coins.Size(), expected_coins_sizes[OutputType::UNKNOWN]); + BOOST_CHECK_EQUAL(available_coins.coins[OutputType::UNKNOWN].size(), expected_coins_sizes[OutputType::UNKNOWN]); + + // We will create a self transfer for each of the OutputTypes and + // verify it is put in the correct bucket after running GetAvailablecoins + // + // For each OutputType, We expect 2 UTXOs in our wallet following the self transfer: + // 1. One UTXO as the recipient + // 2. One UTXO from the change, due to payment address matching logic + + for (const auto& out_type : OUTPUT_TYPES) { + if (out_type == OutputType::UNKNOWN) continue; + expected_coins_sizes[out_type] = 2U; + TestCoinsResult(*this, out_type, 1 * COIN, expected_coins_sizes); + } +} + BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) { { @@ -696,10 +736,10 @@ BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup) std::vector<unsigned char> malformed_record; CVectorWriter vw(0, 0, malformed_record, 0); vw << std::string("notadescriptor"); - vw << (uint64_t)0; - vw << (int32_t)0; - vw << (int32_t)0; - vw << (int32_t)1; + vw << uint64_t{0}; + vw << int32_t{0}; + vw << int32_t{0}; + vw << int32_t{1}; SpanReader vr{0, 0, malformed_record}; WalletDescriptor w_desc; @@ -946,7 +986,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup) mtx.vin.clear(); mtx.vin.push_back(CTxIn(tx_id_to_spend, 0)); - wallet.transactionAddedToMempool(MakeTransactionRef(mtx), 0); + wallet.transactionAddedToMempool(MakeTransactionRef(mtx)); const uint256& good_tx_id = mtx.GetHash(); { @@ -967,7 +1007,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup) static_cast<FailDatabase&>(wallet.GetDatabase()).m_pass = false; mtx.vin.clear(); mtx.vin.push_back(CTxIn(good_tx_id, 0)); - BOOST_CHECK_EXCEPTION(wallet.transactionAddedToMempool(MakeTransactionRef(mtx), 0), + BOOST_CHECK_EXCEPTION(wallet.transactionAddedToMempool(MakeTransactionRef(mtx)), std::runtime_error, HasReason("DB error adding transaction to wallet, write failed")); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 13b92431a6..0f761fb01a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -472,8 +472,7 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b error += strprintf(Untranslated("Unexpected exception: %s"), e.what()); } if (!wallet) { - fs::remove(wallet_file); - fs::remove(wallet_path); + fs::remove_all(wallet_path); } return wallet; @@ -1355,7 +1354,7 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, const SyncTxState& sta MarkInputsDirty(ptx); } -void CWallet::transactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) { +void CWallet::transactionAddedToMempool(const CTransactionRef& tx) { LOCK(cs_wallet); SyncTransaction(tx, TxStateInMempool{}); @@ -1365,7 +1364,7 @@ void CWallet::transactionAddedToMempool(const CTransactionRef& tx, uint64_t memp } } -void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) { +void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) { LOCK(cs_wallet); auto it = mapWallet.find(tx->GetHash()); if (it != mapWallet.end()) { @@ -1411,7 +1410,7 @@ void CWallet::blockConnected(const interfaces::BlockInfo& block) m_last_block_processed = block.hash; for (size_t index = 0; index < block.data->vtx.size(); index++) { SyncTransaction(block.data->vtx[index], TxStateConfirmed{block.hash, block.height, static_cast<int>(index)}); - transactionRemovedFromMempool(block.data->vtx[index], MemPoolRemovalReason::BLOCK, /*mempool_sequence=*/0); + transactionRemovedFromMempool(block.data->vtx[index], MemPoolRemovalReason::BLOCK); } } @@ -3184,6 +3183,24 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf if (tip_height && *tip_height != rescan_height) { + // No need to read and scan block if block was created before + // our wallet birthday (as adjusted for block time variability) + std::optional<int64_t> time_first_key; + for (auto spk_man : walletInstance->GetAllScriptPubKeyMans()) { + int64_t time = spk_man->GetTimeFirstKey(); + if (!time_first_key || time < *time_first_key) time_first_key = time; + } + if (time_first_key) { + FoundBlock found = FoundBlock().height(rescan_height); + chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, found); + if (!found.found) { + // We were unable to find a block that had a time more recent than our earliest timestamp + // or a height higher than the wallet was synced to, indicating that the wallet is newer than the + // current chain tip. Skip rescanning in this case. + rescan_height = *tip_height; + } + } + // Technically we could execute the code below in any case, but performing the // `while` loop below can make startup very slow, so only check blocks on disk // if necessary. @@ -3218,17 +3235,6 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf chain.initMessage(_("Rescanning…").translated); walletInstance->WalletLogPrintf("Rescanning last %i blocks (from block %i)...\n", *tip_height - rescan_height, rescan_height); - // No need to read and scan block if block was created before - // our wallet birthday (as adjusted for block time variability) - std::optional<int64_t> time_first_key; - for (auto spk_man : walletInstance->GetAllScriptPubKeyMans()) { - int64_t time = spk_man->GetTimeFirstKey(); - if (!time_first_key || time < *time_first_key) time_first_key = time; - } - if (time_first_key) { - chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, FoundBlock().height(rescan_height)); - } - { WalletRescanReserver reserver(*walletInstance); if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(chain.getBlockHash(rescan_height), rescan_height, /*max_height=*/{}, reserver, /*fUpdate=*/true, /*save_progress=*/true).status)) { @@ -4002,6 +4008,23 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) } } } + + // Persist added address book entries (labels, purpose) for watchonly and solvable wallets + auto persist_address_book = [](const CWallet& wallet) { + LOCK(wallet.cs_wallet); + WalletBatch batch{wallet.GetDatabase()}; + for (const auto& [destination, addr_book_data] : wallet.m_address_book) { + auto address{EncodeDestination(destination)}; + auto purpose{addr_book_data.purpose}; + auto label{addr_book_data.GetLabel()}; + // don't bother writing default values (unknown purpose, empty label) + if (purpose != "unknown") batch.WritePurpose(address, purpose); + if (!label.empty()) batch.WriteName(address, label); + } + }; + if (data.watchonly_wallet) persist_address_book(*data.watchonly_wallet); + if (data.solvable_wallet) persist_address_book(*data.solvable_wallet); + // Remove the things to delete if (dests_to_delete.size() > 0) { for (const auto& dest : dests_to_delete) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 57b5f7fc31..8b7e6dd526 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -516,7 +516,7 @@ public: */ CWalletTx* AddToWallet(CTransactionRef tx, const TxState& state, const UpdateWalletTxFn& update_wtx=nullptr, bool fFlushOnClose=true, bool rescanning_old_block = false); bool LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void transactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) override; + void transactionAddedToMempool(const CTransactionRef& tx) override; void blockConnected(const interfaces::BlockInfo& block) override; void blockDisconnected(const interfaces::BlockInfo& block) override; void updatedBlockTip() override; @@ -538,7 +538,7 @@ public: uint256 last_failed_block; }; ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress); - void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override; + void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) override; /** Set the next time this wallet should resend transactions to 12-36 hours from now, ~1 day on average. */ void SetNextResend() { m_next_resend = GetDefaultNextResend(); } /** Return true if all conditions for periodically resending transactions are met. */ diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index d560de863a..b393c35112 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -974,7 +974,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) return result; } -DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWalletTx>& vWtx) +DBErrors WalletBatch::FindWalletTxHashes(std::vector<uint256>& tx_hashes) { DBErrors result = DBErrors::LOAD_OK; @@ -1012,9 +1012,7 @@ DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWal if (strType == DBKeys::TX) { uint256 hash; ssKey >> hash; - vTxHash.push_back(hash); - vWtx.emplace_back(/*tx=*/nullptr, TxStateInactive{}); - ssValue >> vWtx.back(); + tx_hashes.push_back(hash); } } } catch (...) { @@ -1027,10 +1025,9 @@ DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWal DBErrors WalletBatch::ZapSelectTx(std::vector<uint256>& vTxHashIn, std::vector<uint256>& vTxHashOut) { - // build list of wallet TXs and hashes + // build list of wallet TX hashes std::vector<uint256> vTxHash; - std::list<CWalletTx> vWtx; - DBErrors err = FindWalletTx(vTxHash, vWtx); + DBErrors err = FindWalletTxHashes(vTxHash); if (err != DBErrors::LOAD_OK) { return err; } diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index c0d9de8bb3..97e8fad278 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -273,7 +273,7 @@ public: bool EraseActiveScriptPubKeyMan(uint8_t type, bool internal); DBErrors LoadWallet(CWallet* pwallet); - DBErrors FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWalletTx>& vWtx); + DBErrors FindWalletTxHashes(std::vector<uint256>& tx_hashes); DBErrors ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut); /* Function to determine if a certain KV/key-type is a key (cryptographical key) type */ static bool IsKeyType(const std::string& strType); diff --git a/test/functional/feature_coinstatsindex.py b/test/functional/feature_coinstatsindex.py index 30cf559c43..eff4d9b149 100755 --- a/test/functional/feature_coinstatsindex.py +++ b/test/functional/feature_coinstatsindex.py @@ -221,7 +221,7 @@ class CoinStatsIndexTest(BitcoinTestFramework): self.generate(index_node, 1, sync_fun=self.no_op) res10 = index_node.gettxoutsetinfo('muhash') - assert(res8['txouts'] < res10['txouts']) + assert res8['txouts'] < res10['txouts'] self.log.info("Test that the index works with -reindex") @@ -268,12 +268,12 @@ class CoinStatsIndexTest(BitcoinTestFramework): res2 = index_node.gettxoutsetinfo(hash_type='muhash', hash_or_height=112) assert_equal(res["bestblock"], block) assert_equal(res["muhash"], res2["muhash"]) - assert(res["muhash"] != res_invalid["muhash"]) + assert res["muhash"] != res_invalid["muhash"] # Test that requesting reorged out block by hash is still returning correct results res_invalid2 = index_node.gettxoutsetinfo(hash_type='muhash', hash_or_height=reorg_block) assert_equal(res_invalid2["muhash"], res_invalid["muhash"]) - assert(res["muhash"] != res_invalid2["muhash"]) + assert res["muhash"] != res_invalid2["muhash"] # Add another block, so we don't depend on reconsiderblock remembering which # blocks were touched by invalidateblock diff --git a/test/functional/feature_index_prune.py b/test/functional/feature_index_prune.py index d5baa080bb..77a056346a 100755 --- a/test/functional/feature_index_prune.py +++ b/test/functional/feature_index_prune.py @@ -62,7 +62,7 @@ class FeatureIndexPruneTest(BitcoinTestFramework): for node in filter_nodes: assert_greater_than(len(node.getblockfilter(tip)['filter']), 0) for node in stats_nodes: - assert(node.gettxoutsetinfo(hash_type="muhash", hash_or_height=tip)['muhash']) + assert node.gettxoutsetinfo(hash_type="muhash", hash_or_height=tip)['muhash'] self.mine_batches(500) self.sync_index(height=700) @@ -80,14 +80,14 @@ class FeatureIndexPruneTest(BitcoinTestFramework): for node in filter_nodes: assert_greater_than(len(node.getblockfilter(tip)['filter']), 0) for node in stats_nodes: - assert(node.gettxoutsetinfo(hash_type="muhash", hash_or_height=tip)['muhash']) + assert node.gettxoutsetinfo(hash_type="muhash", hash_or_height=tip)['muhash'] self.log.info("check if we can access the blockfilter and coinstats of a pruned block") height_hash = self.nodes[0].getblockhash(2) for node in filter_nodes: assert_greater_than(len(node.getblockfilter(height_hash)['filter']), 0) for node in stats_nodes: - assert(node.gettxoutsetinfo(hash_type="muhash", hash_or_height=height_hash)['muhash']) + assert node.gettxoutsetinfo(hash_type="muhash", hash_or_height=height_hash)['muhash'] # mine and sync index up to a height that will later be the pruneheight self.generate(self.nodes[0], 51) diff --git a/test/functional/feature_startupnotify.py b/test/functional/feature_startupnotify.py index f4672a2705..ff5272b281 100755 --- a/test/functional/feature_startupnotify.py +++ b/test/functional/feature_startupnotify.py @@ -29,9 +29,14 @@ class StartupNotifyTest(BitcoinTestFramework): self.wait_until(lambda: os.path.exists(tmpdir_file)) self.log.info("Test -startupnotify is executed once") - with open(tmpdir_file, "r", encoding="utf8") as f: - file_content = f.read() - assert_equal(file_content.count(FILE_NAME), 1) + + def get_count(): + with open(tmpdir_file, "r", encoding="utf8") as f: + file_content = f.read() + return file_content.count(FILE_NAME) + + self.wait_until(lambda: get_count() > 0) + assert_equal(get_count(), 1) self.log.info("Test node is fully started") assert_equal(self.nodes[0].getblockcount(), 200) diff --git a/test/functional/interface_usdt_utxocache.py b/test/functional/interface_usdt_utxocache.py index 2280de1479..e3b0b32f0d 100755 --- a/test/functional/interface_usdt_utxocache.py +++ b/test/functional/interface_usdt_utxocache.py @@ -357,8 +357,8 @@ class UTXOCacheTracepointTest(BitcoinTestFramework): "size": event.size }) # sanity checks only - assert(event.memory > 0) - assert(event.duration > 0) + assert event.memory > 0 + assert event.duration > 0 handle_flush_succeeds += 1 bpf["utxocache_flush"].open_perf_buffer(handle_utxocache_flush) diff --git a/test/functional/interface_usdt_validation.py b/test/functional/interface_usdt_validation.py index 8953dd023b..4323aef771 100755 --- a/test/functional/interface_usdt_validation.py +++ b/test/functional/interface_usdt_validation.py @@ -112,7 +112,7 @@ class ValidationTracepointTest(BitcoinTestFramework): assert_equal(len([tx["vin"] for tx in block["tx"]]), event.inputs) assert_equal(0, event.sigops) # no sigops in coinbase tx # only plausibility checks - assert(event.duration > 0) + assert event.duration > 0 del expected_blocks[block_hash] blocks_checked += 1 diff --git a/test/functional/interface_zmq.py b/test/functional/interface_zmq.py index 00cd3b4894..5357bdf2b8 100755 --- a/test/functional/interface_zmq.py +++ b/test/functional/interface_zmq.py @@ -22,6 +22,7 @@ from test_framework.messages import ( from test_framework.util import ( assert_equal, assert_raises_rpc_error, + p2p_port, ) from test_framework.wallet import ( MiniWallet, @@ -106,6 +107,7 @@ class ZMQTest (BitcoinTestFramework): # This test isn't testing txn relay/timing, so set whitelist on the # peers for instant txn relay. This speeds up the test run time 2-3x. self.extra_args = [["-whitelist=noban@127.0.0.1"]] * self.num_nodes + self.zmq_port_base = p2p_port(self.num_nodes + 1) def skip_test_if_missing_module(self): self.skip_if_no_py3_zmq() @@ -179,7 +181,7 @@ class ZMQTest (BitcoinTestFramework): # Invalid zmq arguments don't take down the node, see #17185. self.restart_node(0, ["-zmqpubrawtx=foo", "-zmqpubhashtx=bar"]) - address = 'tcp://127.0.0.1:28332' + address = f"tcp://127.0.0.1:{self.zmq_port_base}" subs = self.setup_zmq_test([(topic, address) for topic in ["hashblock", "hashtx", "rawblock", "rawtx"]]) hashblock = subs[0] @@ -246,7 +248,7 @@ class ZMQTest (BitcoinTestFramework): def test_reorg(self): - address = 'tcp://127.0.0.1:28333' + address = f"tcp://127.0.0.1:{self.zmq_port_base}" # Should only notify the tip if a reorg occurs hashblock, hashtx = self.setup_zmq_test( @@ -300,7 +302,7 @@ class ZMQTest (BitcoinTestFramework): <32-byte hash>A<8-byte LE uint> : Transactionhash added mempool """ self.log.info("Testing 'sequence' publisher") - [seq] = self.setup_zmq_test([("sequence", "tcp://127.0.0.1:28333")]) + [seq] = self.setup_zmq_test([("sequence", f"tcp://127.0.0.1:{self.zmq_port_base}")]) self.disconnect_nodes(0, 1) # Mempool sequence number starts at 1 @@ -444,7 +446,7 @@ class ZMQTest (BitcoinTestFramework): """ self.log.info("Testing 'mempool sync' usage of sequence notifier") - [seq] = self.setup_zmq_test([("sequence", "tcp://127.0.0.1:28333")]) + [seq] = self.setup_zmq_test([("sequence", f"tcp://127.0.0.1:{self.zmq_port_base}")]) # In-memory counter, should always start at 1 next_mempool_seq = self.nodes[0].getrawmempool(mempool_sequence=True)["mempool_sequence"] @@ -549,8 +551,8 @@ class ZMQTest (BitcoinTestFramework): # chain lengths on node0 and node1; for this test we only need node0, so # we can disable syncing blocks on the setup) subscribers = self.setup_zmq_test([ - ("hashblock", "tcp://127.0.0.1:28334"), - ("hashblock", "tcp://127.0.0.1:28335"), + ("hashblock", f"tcp://127.0.0.1:{self.zmq_port_base + 1}"), + ("hashblock", f"tcp://127.0.0.1:{self.zmq_port_base + 2}"), ], sync_blocks=False) # Generate 1 block in nodes[0] and receive all notifications @@ -567,7 +569,7 @@ class ZMQTest (BitcoinTestFramework): self.log.info("Testing IPv6") # Set up subscriber using IPv6 loopback address subscribers = self.setup_zmq_test([ - ("hashblock", "tcp://[::1]:28332") + ("hashblock", f"tcp://[::1]:{self.zmq_port_base}") ], ipv6=True) # Generate 1 block in nodes[0] diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index abbca15bed..19cb65be36 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -58,7 +58,11 @@ class MempoolAcceptanceTest(BitcoinTestFramework): """Wrapper to check result of testmempoolaccept on node_0's mempool""" result_test = self.nodes[0].testmempoolaccept(*args, **kwargs) for r in result_test: - r.pop('wtxid') # Skip check for now + # Skip these checks for now + r.pop('wtxid') + if "fees" in r: + r["fees"].pop("effective-feerate") + r["fees"].pop("effective-includes") assert_equal(result_expected, result_test) assert_equal(self.nodes[0].getmempoolinfo()['size'], self.mempool_size) # Must not change mempool state diff --git a/test/functional/mocks/invalid_signer.py b/test/functional/mocks/invalid_signer.py index 1d77e75215..7bfa9051ac 100755 --- a/test/functional/mocks/invalid_signer.py +++ b/test/functional/mocks/invalid_signer.py @@ -10,7 +10,7 @@ import json def perform_pre_checks(): mock_result_path = os.path.join(os.getcwd(), "mock_result") - if(os.path.isfile(mock_result_path)): + if os.path.isfile(mock_result_path): with open(mock_result_path, "r", encoding="utf8") as f: mock_result = f.read() if mock_result[0]: diff --git a/test/functional/mocks/signer.py b/test/functional/mocks/signer.py index 41ea541f7b..5f4fad6380 100755 --- a/test/functional/mocks/signer.py +++ b/test/functional/mocks/signer.py @@ -10,7 +10,7 @@ import json def perform_pre_checks(): mock_result_path = os.path.join(os.getcwd(), "mock_result") - if(os.path.isfile(mock_result_path)): + if os.path.isfile(mock_result_path): with open(mock_result_path, "r", encoding="utf8") as f: mock_result = f.read() if mock_result[0]: diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index e2e9b6dcb2..e002a520c6 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -49,7 +49,7 @@ class AddrReceiver(P2PInterface): def on_addr(self, message): for addr in message.addrs: self.num_ipv4_received += 1 - if(self.test_addr_contents): + if self.test_addr_contents: # relay_tests checks the content of the addr messages match # expectations based on the message creation in setup_addr_msg assert_equal(addr.nServices, 9) diff --git a/test/functional/p2p_blocksonly.py b/test/functional/p2p_blocksonly.py index c7297374b9..fa9ddf7ebe 100755 --- a/test/functional/p2p_blocksonly.py +++ b/test/functional/p2p_blocksonly.py @@ -104,7 +104,7 @@ class P2PBlocksOnly(BitcoinTestFramework): self.nodes[0].setmocktime(int(time.time()) + 60) conn.sync_send_with_ping() - assert(int(txid, 16) not in conn.get_invs()) + assert int(txid, 16) not in conn.get_invs() def check_p2p_inv_violation(self, peer): self.log.info("Check that tx-invs from P2P are rejected and result in disconnect") diff --git a/test/functional/p2p_disconnect_ban.py b/test/functional/p2p_disconnect_ban.py index 91c2a43932..7169cc8937 100755 --- a/test/functional/p2p_disconnect_ban.py +++ b/test/functional/p2p_disconnect_ban.py @@ -46,6 +46,9 @@ class DisconnectBanTest(BitcoinTestFramework): assert_raises_rpc_error(-30, "Error: Invalid IP/Subnet", self.nodes[1].setban, "127.0.0.1/42", "add") assert_equal(len(self.nodes[1].listbanned()), 1) # still only one banned ip because 127.0.0.1 is within the range of 127.0.0.0/24 + self.log.info("setban: fail to ban with past absolute timestamp") + assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, "127.27.0.1", "add", 123, True) + self.log.info("setban remove: fail to unban a non-banned subnet") assert_raises_rpc_error(-30, "Error: Unban failed", self.nodes[1].setban, "127.0.0.1", "remove") assert_equal(len(self.nodes[1].listbanned()), 1) @@ -66,9 +69,13 @@ class DisconnectBanTest(BitcoinTestFramework): self.nodes[1].setban("2001:4d48:ac57:400:cacf:e9ff:fe1d:9c63/19", "add", 1000) # ban for 1000 seconds listBeforeShutdown = self.nodes[1].listbanned() assert_equal("192.168.0.1/32", listBeforeShutdown[2]['address']) + + self.log.info("setban: test banning with absolute timestamp") + self.nodes[1].setban("192.168.0.2", "add", old_time + 120, True) + # Move time forward by 3 seconds so the third ban has expired self.nodes[1].setmocktime(old_time + 3) - assert_equal(len(self.nodes[1].listbanned()), 3) + assert_equal(len(self.nodes[1].listbanned()), 4) self.log.info("Test ban_duration and time_remaining") for ban in self.nodes[1].listbanned(): @@ -78,13 +85,17 @@ class DisconnectBanTest(BitcoinTestFramework): elif ban["address"] == "2001:4d48:ac57:400:cacf:e9ff:fe1d:9c63/19": assert_equal(ban["ban_duration"], 1000) assert_equal(ban["time_remaining"], 997) + elif ban["address"] == "192.168.0.2/32": + assert_equal(ban["ban_duration"], 120) + assert_equal(ban["time_remaining"], 117) self.restart_node(1) listAfterShutdown = self.nodes[1].listbanned() assert_equal("127.0.0.0/24", listAfterShutdown[0]['address']) assert_equal("127.0.0.0/32", listAfterShutdown[1]['address']) - assert_equal("/19" in listAfterShutdown[2]['address'], True) + assert_equal("192.168.0.2/32", listAfterShutdown[2]['address']) + assert_equal("/19" in listAfterShutdown[3]['address'], True) # Clear ban lists self.nodes[1].clearbanned() diff --git a/test/functional/p2p_getaddr_caching.py b/test/functional/p2p_getaddr_caching.py index 050183f19a..1c9ad7289b 100755 --- a/test/functional/p2p_getaddr_caching.py +++ b/test/functional/p2p_getaddr_caching.py @@ -60,7 +60,7 @@ class AddrTest(BitcoinTestFramework): # Need to make sure we hit MAX_ADDR_TO_SEND records in the addr response later because # only a fraction of all known addresses can be cached and returned. - assert(len(self.nodes[0].getnodeaddresses(0)) > int(MAX_ADDR_TO_SEND / (MAX_PCT_ADDR_TO_SEND / 100))) + assert len(self.nodes[0].getnodeaddresses(0)) > int(MAX_ADDR_TO_SEND / (MAX_PCT_ADDR_TO_SEND / 100)) last_response_on_local_bind = None last_response_on_onion_bind1 = None @@ -85,9 +85,9 @@ class AddrTest(BitcoinTestFramework): if i > 0: # Responses from different binds should be unique - assert(last_response_on_local_bind != addr_receiver_onion1.get_received_addrs()) - assert(last_response_on_local_bind != addr_receiver_onion2.get_received_addrs()) - assert(last_response_on_onion_bind1 != addr_receiver_onion2.get_received_addrs()) + assert last_response_on_local_bind != addr_receiver_onion1.get_received_addrs() + assert last_response_on_local_bind != addr_receiver_onion2.get_received_addrs() + assert last_response_on_onion_bind1 != addr_receiver_onion2.get_received_addrs() # Responses on from the same bind should be the same assert_equal(last_response_on_local_bind, addr_receiver_local.get_received_addrs()) assert_equal(last_response_on_onion_bind1, addr_receiver_onion1.get_received_addrs()) @@ -119,9 +119,9 @@ class AddrTest(BitcoinTestFramework): addr_receiver_onion2.wait_until(addr_receiver_onion2.addr_received) # new response is different - assert(set(last_response_on_local_bind) != set(addr_receiver_local.get_received_addrs())) - assert(set(last_response_on_onion_bind1) != set(addr_receiver_onion1.get_received_addrs())) - assert(set(last_response_on_onion_bind2) != set(addr_receiver_onion2.get_received_addrs())) + assert set(last_response_on_local_bind) != set(addr_receiver_local.get_received_addrs()) + assert set(last_response_on_onion_bind1) != set(addr_receiver_onion1.get_received_addrs()) + assert set(last_response_on_onion_bind2) != set(addr_receiver_onion2.get_received_addrs()) if __name__ == '__main__': AddrTest().main() diff --git a/test/functional/p2p_headers_sync_with_minchainwork.py b/test/functional/p2p_headers_sync_with_minchainwork.py index a69e7b5710..b07077c668 100755 --- a/test/functional/p2p_headers_sync_with_minchainwork.py +++ b/test/functional/p2p_headers_sync_with_minchainwork.py @@ -57,7 +57,7 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework): def check_node3_chaintips(num_tips, tip_hash, height): node3_chaintips = self.nodes[3].getchaintips() - assert(len(node3_chaintips) == num_tips) + assert len(node3_chaintips) == num_tips assert { 'height': height, 'hash': tip_hash, @@ -69,7 +69,7 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework): for node in self.nodes[1:3]: chaintips = node.getchaintips() - assert(len(chaintips) == 1) + assert len(chaintips) == 1 assert { 'height': 0, 'hash': '0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206', @@ -89,7 +89,7 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework): 'status': 'active', } in self.nodes[2].getchaintips() - assert(len(self.nodes[2].getchaintips()) == 1) + assert len(self.nodes[2].getchaintips()) == 1 self.log.info("Check that node3 accepted these headers as well") check_node3_chaintips(2, self.nodes[0].getbestblockhash(), NODE1_BLOCKS_REQUIRED) diff --git a/test/functional/p2p_i2p_sessions.py b/test/functional/p2p_i2p_sessions.py index 4e52522b81..9e7fdc6e14 100755 --- a/test/functional/p2p_i2p_sessions.py +++ b/test/functional/p2p_i2p_sessions.py @@ -23,12 +23,12 @@ class I2PSessions(BitcoinTestFramework): self.log.info("Ensure we create a persistent session when -i2pacceptincoming=1") node0 = self.nodes[0] - with node0.assert_debug_log(expected_msgs=[f"Creating persistent SAM session"]): + with node0.assert_debug_log(expected_msgs=["Creating persistent SAM session"]): node0.addnode(node=addr, command="onetry") self.log.info("Ensure we create a transient session when -i2pacceptincoming=0") node1 = self.nodes[1] - with node1.assert_debug_log(expected_msgs=[f"Creating transient SAM session"]): + with node1.assert_debug_log(expected_msgs=["Creating transient SAM session"]): node1.addnode(node=addr, command="onetry") diff --git a/test/functional/p2p_message_capture.py b/test/functional/p2p_message_capture.py index 495e9bf1da..3ab0b79ba2 100755 --- a/test/functional/p2p_message_capture.py +++ b/test/functional/p2p_message_capture.py @@ -36,7 +36,7 @@ def mini_parser(dat_file): """ with open(dat_file, 'rb') as f_in: # This should have at least one message in it - assert(os.fstat(f_in.fileno()).st_size >= TIME_SIZE + LENGTH_SIZE + MSGTYPE_SIZE) + assert os.fstat(f_in.fileno()).st_size >= TIME_SIZE + LENGTH_SIZE + MSGTYPE_SIZE while True: tmp_header_raw = f_in.read(TIME_SIZE + LENGTH_SIZE + MSGTYPE_SIZE) if not tmp_header_raw: @@ -44,7 +44,7 @@ def mini_parser(dat_file): tmp_header = BytesIO(tmp_header_raw) tmp_header.read(TIME_SIZE) # skip the timestamp field msgtype = tmp_header.read(MSGTYPE_SIZE).rstrip(b'\x00') - assert(msgtype in MESSAGEMAP) + assert msgtype in MESSAGEMAP length: int = int.from_bytes(tmp_header.read(LENGTH_SIZE), "little") data = f_in.read(length) assert_equal(len(data), length) diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 2ddc09b13b..b0900e49b8 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -622,8 +622,10 @@ class SegWitTest(BitcoinTestFramework): if not self.segwit_active: # Just check mempool acceptance, but don't add the transaction to the mempool, since witness is disallowed # in blocks and the tx is impossible to mine right now. - assert_equal( - self.nodes[0].testmempoolaccept([tx3.serialize_with_witness().hex()]), + testres3 = self.nodes[0].testmempoolaccept([tx3.serialize_with_witness().hex()]) + testres3[0]["fees"].pop("effective-feerate") + testres3[0]["fees"].pop("effective-includes") + assert_equal(testres3, [{ 'txid': tx3.hash, 'wtxid': tx3.getwtxid(), @@ -639,8 +641,10 @@ class SegWitTest(BitcoinTestFramework): tx3 = tx tx3.vout = [tx3_out] tx3.rehash() - assert_equal( - self.nodes[0].testmempoolaccept([tx3.serialize_with_witness().hex()]), + testres3_replaced = self.nodes[0].testmempoolaccept([tx3.serialize_with_witness().hex()]) + testres3_replaced[0]["fees"].pop("effective-feerate") + testres3_replaced[0]["fees"].pop("effective-includes") + assert_equal(testres3_replaced, [{ 'txid': tx3.hash, 'wtxid': tx3.getwtxid(), diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index e9c4abe2a8..19c73eebf0 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -370,7 +370,7 @@ class BlockchainTest(BitcoinTestFramework): # hash_type muhash should return a different UTXO set hash. res6 = node.gettxoutsetinfo(hash_type='muhash') assert 'muhash' in res6 - assert(res['hash_serialized_2'] != res6['muhash']) + assert res['hash_serialized_2'] != res6['muhash'] # muhash should not be returned unless requested. for r in [res, res2, res3, res4, res5]: diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index f1352f88d8..10388399ad 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -20,6 +20,7 @@ from test_framework.util import ( assert_raises_rpc_error, ) from test_framework.wallet import ( + COIN, DEFAULT_FEE, MiniWallet, ) @@ -239,11 +240,14 @@ class RPCPackagesTest(BitcoinTestFramework): coin = self.wallet.get_utxo() fee = Decimal("0.00125000") replaceable_tx = self.wallet.create_self_transfer(utxo_to_spend=coin, sequence=MAX_BIP125_RBF_SEQUENCE, fee = fee) - testres_replaceable = node.testmempoolaccept([replaceable_tx["hex"]]) - assert_equal(testres_replaceable, [ - {"txid": replaceable_tx["txid"], "wtxid": replaceable_tx["wtxid"], - "allowed": True, "vsize": replaceable_tx["tx"].get_vsize(), "fees": { "base": fee }} - ]) + testres_replaceable = node.testmempoolaccept([replaceable_tx["hex"]])[0] + assert_equal(testres_replaceable["txid"], replaceable_tx["txid"]) + assert_equal(testres_replaceable["wtxid"], replaceable_tx["wtxid"]) + assert testres_replaceable["allowed"] + assert_equal(testres_replaceable["vsize"], replaceable_tx["tx"].get_vsize()) + assert_equal(testres_replaceable["fees"]["base"], fee) + assert_fee_amount(fee, replaceable_tx["tx"].get_vsize(), testres_replaceable["fees"]["effective-feerate"]) + assert_equal(testres_replaceable["fees"]["effective-includes"], [replaceable_tx["wtxid"]]) # Replacement transaction is identical except has double the fee replacement_tx = self.wallet.create_self_transfer(utxo_to_spend=coin, sequence=MAX_BIP125_RBF_SEQUENCE, fee = 2 * fee) @@ -287,11 +291,13 @@ class RPCPackagesTest(BitcoinTestFramework): peer = node.add_p2p_connection(P2PTxInvStore()) package_txns = [] + presubmitted_wtxids = set() for _ in range(num_parents): parent_tx = self.wallet.create_self_transfer(fee=DEFAULT_FEE) package_txns.append(parent_tx) if partial_submit and random.choice([True, False]): node.sendrawtransaction(parent_tx["hex"]) + presubmitted_wtxids.add(parent_tx["wtxid"]) child_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[tx["new_utxo"] for tx in package_txns], fee_per_output=10000) #DEFAULT_FEE package_txns.append(child_tx) @@ -302,24 +308,19 @@ class RPCPackagesTest(BitcoinTestFramework): for package_txn in package_txns: tx = package_txn["tx"] assert tx.getwtxid() in submitpackage_result["tx-results"] - tx_result = submitpackage_result["tx-results"][tx.getwtxid()] - assert_equal(tx_result, { - "txid": package_txn["txid"], - "vsize": tx.get_vsize(), - "fees": { - "base": DEFAULT_FEE, - } - }) + wtxid = tx.getwtxid() + assert wtxid in submitpackage_result["tx-results"] + tx_result = submitpackage_result["tx-results"][wtxid] + assert_equal(tx_result["txid"], tx.rehash()) + assert_equal(tx_result["vsize"], tx.get_vsize()) + assert_equal(tx_result["fees"]["base"], DEFAULT_FEE) + if wtxid not in presubmitted_wtxids: + assert_fee_amount(DEFAULT_FEE, tx.get_vsize(), tx_result["fees"]["effective-feerate"]) + assert_equal(tx_result["fees"]["effective-includes"], [wtxid]) # submitpackage result should be consistent with testmempoolaccept and getmempoolentry self.assert_equal_package_results(node, testmempoolaccept_result, submitpackage_result) - # Package feerate is calculated for the remaining transactions after deduplication and - # individual submission. If only 0 or 1 transaction is left, e.g. because all transactions - # had high-feerates or were already in the mempool, no package feerate is provided. - # In this case, since all of the parents have high fees, each is accepted individually. - assert "package-feerate" not in submitpackage_result - # The node should announce each transaction. No guarantees for propagation. peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns]) self.generate(node, 1) @@ -328,8 +329,11 @@ class RPCPackagesTest(BitcoinTestFramework): node = self.nodes[0] peer = node.add_p2p_connection(P2PTxInvStore()) + # Package with 2 parents and 1 child. One parent pays for itself using modified fees, and + # another has 0 fees but is bumped by child. tx_poor = self.wallet.create_self_transfer(fee=0, fee_rate=0) - tx_rich = self.wallet.create_self_transfer(fee=DEFAULT_FEE) + tx_rich = self.wallet.create_self_transfer(fee=0, fee_rate=0) + node.prioritisetransaction(tx_rich["txid"], 0, int(DEFAULT_FEE * COIN)) package_txns = [tx_rich, tx_poor] coins = [tx["new_utxo"] for tx in package_txns] tx_child = self.wallet.create_self_transfer_multi(utxos_to_spend=coins, fee_per_output=10000) #DEFAULT_FEE @@ -340,14 +344,18 @@ class RPCPackagesTest(BitcoinTestFramework): rich_parent_result = submitpackage_result["tx-results"][tx_rich["wtxid"]] poor_parent_result = submitpackage_result["tx-results"][tx_poor["wtxid"]] child_result = submitpackage_result["tx-results"][tx_child["tx"].getwtxid()] - assert_equal(rich_parent_result["fees"]["base"], DEFAULT_FEE) + assert_equal(rich_parent_result["fees"]["base"], 0) assert_equal(poor_parent_result["fees"]["base"], 0) assert_equal(child_result["fees"]["base"], DEFAULT_FEE) - # Package feerate is calculated for the remaining transactions after deduplication and - # individual submission. Since this package had a 0-fee parent, package feerate must have - # been used and returned. - assert "package-feerate" in submitpackage_result - assert_fee_amount(DEFAULT_FEE, rich_parent_result["vsize"] + child_result["vsize"], submitpackage_result["package-feerate"]) + # The "rich" parent does not require CPFP so its effective feerate. + assert_fee_amount(DEFAULT_FEE, tx_rich["tx"].get_vsize(), rich_parent_result["fees"]["effective-feerate"]) + assert_equal(rich_parent_result["fees"]["effective-includes"], [tx_rich["wtxid"]]) + # The "poor" parent and child's effective feerates are the same, composed of the child's fee + # divided by their combined vsize. + assert_fee_amount(DEFAULT_FEE, tx_poor["tx"].get_vsize() + tx_child["tx"].get_vsize(), poor_parent_result["fees"]["effective-feerate"]) + assert_fee_amount(DEFAULT_FEE, tx_poor["tx"].get_vsize() + tx_child["tx"].get_vsize(), child_result["fees"]["effective-feerate"]) + assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], poor_parent_result["fees"]["effective-includes"]) + assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], child_result["fees"]["effective-includes"]) # The node will broadcast each transaction, still abiding by its peer's fee filter peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns]) diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 2a91e9d0de..a50e0fb244 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -120,7 +120,9 @@ class PSBTTest(BitcoinTestFramework): # If inputs are specified, do not automatically add more: utxo1 = self.nodes[0].listunspent()[0] - assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[0].walletcreatefundedpsbt, [{"txid": utxo1['txid'], "vout": utxo1['vout']}], {self.nodes[2].getnewaddress():90}) + assert_raises_rpc_error(-4, "The preselected coins total amount does not cover the transaction target. " + "Please allow other inputs to be automatically selected or include more coins manually", + self.nodes[0].walletcreatefundedpsbt, [{"txid": utxo1['txid'], "vout": utxo1['vout']}], {self.nodes[2].getnewaddress():90}) psbtx1 = self.nodes[0].walletcreatefundedpsbt([{"txid": utxo1['txid'], "vout": utxo1['vout']}], {self.nodes[2].getnewaddress():90}, 0, {"add_inputs": True})['psbt'] assert_equal(len(self.nodes[0].decodepsbt(psbtx1)['tx']['vin']), 2) diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py index c0a0e7ee17..1c91ab6f5f 100755 --- a/test/functional/rpc_rawtransaction.py +++ b/test/functional/rpc_rawtransaction.py @@ -216,13 +216,13 @@ class RawTransactionsTest(BitcoinTestFramework): if missing_fields: raise AssertionError(f"fields {', '.join(missing_fields)} are not in transaction") - assert(len(gottx['vin']) > 0) + assert len(gottx['vin']) > 0 if v == 1: - assert('fee' not in gottx) - assert('prevout' not in gottx['vin'][0]) + assert 'fee' not in gottx + assert 'prevout' not in gottx['vin'][0] if v == 2: - assert(isinstance(gottx['fee'], Decimal)) - assert('prevout' in gottx['vin'][0]) + assert isinstance(gottx['fee'], Decimal) + assert 'prevout' in gottx['vin'][0] prevout = gottx['vin'][0]['prevout'] script_pub_key = prevout['scriptPubKey'] @@ -235,11 +235,11 @@ class RawTransactionsTest(BitcoinTestFramework): raise AssertionError(f"fields {', '.join(missing_fields)} are not in transaction") # check verbosity 2 without blockhash but with txindex - assert('fee' in self.nodes[0].getrawtransaction(txid=tx, verbosity=2)) + assert 'fee' in self.nodes[0].getrawtransaction(txid=tx, verbosity=2) # check that coinbase has no fee or does not throw any errors for verbosity 2 coin_base = self.nodes[1].getblock(block1)['tx'][0] gottx = self.nodes[1].getrawtransaction(txid=coin_base, verbosity=2, blockhash=block1) - assert('fee' not in gottx) + assert 'fee' not in gottx def createrawtransaction_tests(self): self.log.info("Test createrawtransaction") diff --git a/test/functional/rpc_scanblocks.py b/test/functional/rpc_scanblocks.py index b449875c95..9a00518150 100755 --- a/test/functional/rpc_scanblocks.py +++ b/test/functional/rpc_scanblocks.py @@ -46,7 +46,7 @@ class ScanblocksTest(BitcoinTestFramework): self.wait_until(lambda: all(i["synced"] for i in node.getindexinfo().values())) out = node.scanblocks("start", [f"addr({addr_1})"]) - assert(blockhash in out['relevant_blocks']) + assert blockhash in out['relevant_blocks'] assert_equal(height, out['to_height']) assert_equal(0, out['from_height']) @@ -56,24 +56,24 @@ class ScanblocksTest(BitcoinTestFramework): # make sure the blockhash is not in the filter result if we set the start_height # to the just mined block (unlikely to hit a false positive) - assert(blockhash not in node.scanblocks( - "start", [f"addr({addr_1})"], height_new)['relevant_blocks']) + assert blockhash not in node.scanblocks( + "start", [f"addr({addr_1})"], height_new)['relevant_blocks'] # make sure the blockhash is present when using the first mined block as start_height - assert(blockhash in node.scanblocks( - "start", [f"addr({addr_1})"], height)['relevant_blocks']) + assert blockhash in node.scanblocks( + "start", [f"addr({addr_1})"], height)['relevant_blocks'] # also test the stop height - assert(blockhash in node.scanblocks( - "start", [f"addr({addr_1})"], height, height)['relevant_blocks']) + assert blockhash in node.scanblocks( + "start", [f"addr({addr_1})"], height, height)['relevant_blocks'] # use the stop_height to exclude the relevant block - assert(blockhash not in node.scanblocks( - "start", [f"addr({addr_1})"], 0, height - 1)['relevant_blocks']) + assert blockhash not in node.scanblocks( + "start", [f"addr({addr_1})"], 0, height - 1)['relevant_blocks'] # make sure the blockhash is present when using the first mined block as start_height - assert(blockhash in node.scanblocks( - "start", [{"desc": f"pkh({parent_key}/*)", "range": [0, 100]}], height)['relevant_blocks']) + assert blockhash in node.scanblocks( + "start", [{"desc": f"pkh({parent_key}/*)", "range": [0, 100]}], height)['relevant_blocks'] # check that false-positives are included in the result now; note that # finding a false-positive at runtime would take too long, hence we simply @@ -89,10 +89,10 @@ class ScanblocksTest(BitcoinTestFramework): false_positive_hash = bip158_basic_element_hash(false_positive_spk, 1, genesis_blockhash) assert_equal(genesis_coinbase_hash, false_positive_hash) - assert(genesis_blockhash in node.scanblocks( - "start", [{"desc": f"raw({genesis_coinbase_spk.hex()})"}], 0, 0)['relevant_blocks']) - assert(genesis_blockhash in node.scanblocks( - "start", [{"desc": f"raw({false_positive_spk.hex()})"}], 0, 0)['relevant_blocks']) + assert genesis_blockhash in node.scanblocks( + "start", [{"desc": f"raw({genesis_coinbase_spk.hex()})"}], 0, 0)['relevant_blocks'] + assert genesis_blockhash in node.scanblocks( + "start", [{"desc": f"raw({false_positive_spk.hex()})"}], 0, 0)['relevant_blocks'] # TODO: after an "accurate" mode for scanblocks is implemented (e.g. PR #26325) # check here that it filters out the false-positive diff --git a/test/functional/test_framework/key.py b/test/functional/test_framework/key.py index 68afc1383d..ad305ce1ef 100644 --- a/test/functional/test_framework/key.py +++ b/test/functional/test_framework/key.py @@ -139,7 +139,7 @@ class EllipticCurve: See https://en.wikibooks.org/wiki/Cryptography/Prime_Curve/Jacobian_Coordinates - Point Addition (with affine point)""" x1, y1, z1 = p1 x2, y2, z2 = p2 - assert(z2 == 1) + assert z2 == 1 # Adding to the point at infinity is a no-op if z1 == 0: return p2 @@ -262,7 +262,7 @@ class ECPubKey(): return self.valid def get_bytes(self): - assert(self.valid) + assert self.valid p = SECP256K1.affine(self.p) if p is None: return None @@ -276,7 +276,7 @@ class ECPubKey(): See https://en.wikipedia.org/wiki/Elliptic_Curve_Digital_Signature_Algorithm for the ECDSA verifier algorithm""" - assert(self.valid) + assert self.valid # Extract r and s from the DER formatted signature. Return false for # any DER encoding errors. @@ -349,7 +349,7 @@ class ECKey(): def set(self, secret, compressed): """Construct a private key object with given 32-byte secret and compressed flag.""" - assert(len(secret) == 32) + assert len(secret) == 32 secret = int.from_bytes(secret, 'big') self.valid = (secret > 0 and secret < SECP256K1_ORDER) if self.valid: @@ -362,7 +362,7 @@ class ECKey(): def get_bytes(self): """Retrieve the 32-byte representation of this key.""" - assert(self.valid) + assert self.valid return self.secret.to_bytes(32, 'big') @property @@ -375,7 +375,7 @@ class ECKey(): def get_pubkey(self): """Compute an ECPubKey object for this secret key.""" - assert(self.valid) + assert self.valid ret = ECPubKey() p = SECP256K1.mul([(SECP256K1_G, self.secret)]) ret.p = p @@ -388,7 +388,7 @@ class ECKey(): See https://en.wikipedia.org/wiki/Elliptic_Curve_Digital_Signature_Algorithm for the ECDSA signer algorithm.""" - assert(self.valid) + assert self.valid z = int.from_bytes(msg, 'big') # Note: no RFC6979 by default, but a simple random nonce (some tests rely on distinct transactions for the same operation) if rfc6979: diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py index ebff849cc9..f345bf02db 100644 --- a/test/functional/test_framework/script.py +++ b/test/functional/test_framework/script.py @@ -824,10 +824,10 @@ def taproot_tree_helper(scripts): if len(scripts) == 1: # One entry: treat as a leaf script = scripts[0] - assert(not callable(script)) + assert not callable(script) if isinstance(script, list): return taproot_tree_helper(script) - assert(isinstance(script, tuple)) + assert isinstance(script, tuple) version = LEAF_VERSION_TAPSCRIPT name = script[0] code = script[1] diff --git a/test/functional/test_framework/siphash.py b/test/functional/test_framework/siphash.py index 5ad245cf1b..884dbcab46 100644 --- a/test/functional/test_framework/siphash.py +++ b/test/functional/test_framework/siphash.py @@ -31,7 +31,7 @@ def siphash_round(v0, v1, v2, v3): def siphash(k0, k1, data): - assert(type(data) == bytes) + assert type(data) == bytes v0 = 0x736f6d6570736575 ^ k0 v1 = 0x646f72616e646f6d ^ k1 v2 = 0x6c7967656e657261 ^ k0 @@ -61,5 +61,5 @@ def siphash(k0, k1, data): def siphash256(k0, k1, num): - assert(type(num) == int) + assert type(num) == int return siphash(k0, k1, num.to_bytes(32, 'little')) diff --git a/test/functional/wallet_avoidreuse.py b/test/functional/wallet_avoidreuse.py index 402cb387c1..5601d81227 100755 --- a/test/functional/wallet_avoidreuse.py +++ b/test/functional/wallet_avoidreuse.py @@ -120,6 +120,8 @@ class AvoidReuseTest(BitcoinTestFramework): assert_raises_rpc_error(-8, "Wallet flag is already set to false", self.nodes[0].setwalletflag, 'avoid_reuse', False) assert_raises_rpc_error(-8, "Wallet flag is already set to true", self.nodes[1].setwalletflag, 'avoid_reuse', True) + assert_raises_rpc_error(-8, "Unknown wallet flag: abc", self.nodes[0].setwalletflag, 'abc', True) + # Create a wallet with avoid reuse, and test that disabling it afterwards persists self.nodes[1].createwallet(wallet_name="avoid_reuse_persist", avoid_reuse=True) w = self.nodes[1].get_wallet_rpc("avoid_reuse_persist") @@ -193,7 +195,7 @@ class AvoidReuseTest(BitcoinTestFramework): # getbalances should show no used, 10 btc trusted assert_balances(self.nodes[1], mine={"used": 0, "trusted": 10}) # node 0 should not show a used entry, as it does not enable avoid_reuse - assert("used" not in self.nodes[0].getbalances()["mine"]) + assert "used" not in self.nodes[0].getbalances()["mine"] self.nodes[1].sendtoaddress(retaddr, 5) self.generate(self.nodes[0], 1) diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py index ffe3c359a9..f55a3758ce 100755 --- a/test/functional/wallet_backwards_compatibility.py +++ b/test/functional/wallet_backwards_compatibility.py @@ -193,18 +193,18 @@ class BackwardsCompatibilityTest(BitcoinTestFramework): assert_equal(txs[1]["txid"], tx1_id) assert_equal(txs[2]["walletconflicts"], [tx1_id]) assert_equal(txs[1]["replaced_by_txid"], tx2_id) - assert not(txs[1]["abandoned"]) + assert not txs[1]["abandoned"] assert_equal(txs[1]["confirmations"], -1) assert_equal(txs[2]["blockindex"], 1) assert txs[3]["abandoned"] assert_equal(txs[4]["walletconflicts"], [tx3_id]) assert_equal(txs[3]["replaced_by_txid"], tx4_id) - assert not(hasattr(txs[3], "blockindex")) + assert not hasattr(txs[3], "blockindex") elif wallet_name == "w2": - assert(info['private_keys_enabled'] == False) + assert info['private_keys_enabled'] == False assert info['keypoolsize'] == 0 else: - assert(info['private_keys_enabled'] == True) + assert info['private_keys_enabled'] == True assert info['keypoolsize'] == 0 else: for node in legacy_nodes: diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index 2160408a55..1cb3f434e8 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -557,7 +557,7 @@ def test_unconfirmed_not_spendable(self, rbf_node, rbf_node_address): def test_bumpfee_metadata(self, rbf_node, dest_address): self.log.info('Test that bumped txn metadata persists to new txn record') - assert(rbf_node.getbalance() < 49) + assert rbf_node.getbalance() < 49 self.generatetoaddress(rbf_node, 101, rbf_node.getnewaddress()) rbfid = rbf_node.sendtoaddress(dest_address, 49, "comment value", "to value") bumped_tx = rbf_node.bumpfee(rbfid) diff --git a/test/functional/wallet_crosschain.py b/test/functional/wallet_crosschain.py index 9cc1f93729..6f93ad4e3b 100755 --- a/test/functional/wallet_crosschain.py +++ b/test/functional/wallet_crosschain.py @@ -36,20 +36,28 @@ class WalletCrossChain(BitcoinTestFramework): self.log.info("Creating wallets") node0_wallet = os.path.join(self.nodes[0].datadir, 'node0_wallet') + node0_wallet_backup = os.path.join(self.nodes[0].datadir, 'node0_wallet.bak') self.nodes[0].createwallet(node0_wallet) + self.nodes[0].backupwallet(node0_wallet_backup) self.nodes[0].unloadwallet(node0_wallet) node1_wallet = os.path.join(self.nodes[1].datadir, 'node1_wallet') + node1_wallet_backup = os.path.join(self.nodes[0].datadir, 'node1_wallet.bak') self.nodes[1].createwallet(node1_wallet) + self.nodes[1].backupwallet(node1_wallet_backup) self.nodes[1].unloadwallet(node1_wallet) - self.log.info("Loading wallets into nodes with a different genesis blocks") + self.log.info("Loading/restoring wallets into nodes with a different genesis block") if self.options.descriptors: assert_raises_rpc_error(-18, 'Wallet file verification failed.', self.nodes[0].loadwallet, node1_wallet) assert_raises_rpc_error(-18, 'Wallet file verification failed.', self.nodes[1].loadwallet, node0_wallet) + assert_raises_rpc_error(-18, 'Wallet file verification failed.', self.nodes[0].restorewallet, 'w', node1_wallet_backup) + assert_raises_rpc_error(-18, 'Wallet file verification failed.', self.nodes[1].restorewallet, 'w', node0_wallet_backup) else: assert_raises_rpc_error(-4, 'Wallet files should not be reused across chains.', self.nodes[0].loadwallet, node1_wallet) assert_raises_rpc_error(-4, 'Wallet files should not be reused across chains.', self.nodes[1].loadwallet, node0_wallet) + assert_raises_rpc_error(-4, 'Wallet files should not be reused across chains.', self.nodes[0].restorewallet, 'w', node1_wallet_backup) + assert_raises_rpc_error(-4, 'Wallet files should not be reused across chains.', self.nodes[1].restorewallet, 'w', node0_wallet_backup) if not self.options.descriptors: self.log.info("Override cross-chain wallet load protection") diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index 038002d2dc..98b0f70b01 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -27,6 +27,8 @@ from test_framework.util import ( ) from test_framework.wallet_util import bytes_to_wif +ERR_NOT_ENOUGH_PRESET_INPUTS = "The preselected coins total amount does not cover the transaction target. " \ + "Please allow other inputs to be automatically selected or include more coins manually" def get_unspent(listunspent, amount): for utx in listunspent: @@ -328,7 +330,7 @@ class RawTransactionsTest(BitcoinTestFramework): assert_equal("00", dec_tx['vin'][0]['scriptSig']['hex']) # Should fail without add_inputs: - assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) # add_inputs is enabled by default rawtxfund = self.nodes[2].fundrawtransaction(rawtx) @@ -360,7 +362,7 @@ class RawTransactionsTest(BitcoinTestFramework): assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) # Should fail without add_inputs: - assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {"add_inputs": True}) dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) @@ -394,7 +396,7 @@ class RawTransactionsTest(BitcoinTestFramework): assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) # Should fail without add_inputs: - assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {"add_inputs": True}) dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) @@ -989,7 +991,9 @@ class RawTransactionsTest(BitcoinTestFramework): outputs[recipient.getnewaddress()] = 0.1 wallet.sendmany("", outputs) self.generate(self.nodes[0], 10) - assert_raises_rpc_error(-4, "Insufficient funds", recipient.fundrawtransaction, rawtx) + assert_raises_rpc_error(-4, "The inputs size exceeds the maximum weight. " + "Please try sending a smaller amount or manually consolidating your wallet's UTXOs", + recipient.fundrawtransaction, rawtx) self.nodes[0].unloadwallet("large") def test_external_inputs(self): @@ -1130,7 +1134,7 @@ class RawTransactionsTest(BitcoinTestFramework): } ] } - assert_raises_rpc_error(-4, "Insufficient funds", wallet.send, outputs=[{addr1: 8}], options=options) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.send, outputs=[{addr1: 8}], options=options) # Case (3), Explicit add_inputs=true and preset inputs (with preset inputs not-covering the target amount) options["add_inputs"] = True @@ -1158,7 +1162,7 @@ class RawTransactionsTest(BitcoinTestFramework): # 6. Explicit add_inputs=false, no preset inputs: options = {"add_inputs": False} - assert_raises_rpc_error(-4, "Insufficient funds", wallet.send, outputs=[{addr1: 3}], options=options) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.send, outputs=[{addr1: 3}], options=options) ################################################ @@ -1175,7 +1179,7 @@ class RawTransactionsTest(BitcoinTestFramework): "vout": 1 # change position was hardcoded to index 0 }] outputs = {self.nodes[1].getnewaddress(): 8} - assert_raises_rpc_error(-4, "Insufficient funds", wallet.walletcreatefundedpsbt, inputs=inputs, outputs=outputs) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.walletcreatefundedpsbt, inputs=inputs, outputs=outputs) # Case (3), Explicit add_inputs=true and preset inputs (with preset inputs not-covering the target amount) options["add_inputs"] = True @@ -1202,7 +1206,7 @@ class RawTransactionsTest(BitcoinTestFramework): # Case (6). Explicit add_inputs=false, no preset inputs: options = {"add_inputs": False} - assert_raises_rpc_error(-4, "Insufficient funds", wallet.walletcreatefundedpsbt, inputs=[], outputs=outputs, options=options) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.walletcreatefundedpsbt, inputs=[], outputs=outputs, options=options) self.nodes[2].unloadwallet("test_preset_inputs") diff --git a/test/functional/wallet_implicitsegwit.py b/test/functional/wallet_implicitsegwit.py index 3ac0bbc17f..baa9bafb00 100755 --- a/test/functional/wallet_implicitsegwit.py +++ b/test/functional/wallet_implicitsegwit.py @@ -36,7 +36,7 @@ def check_implicit_transactions(implicit_keys, implicit_node): pubkey = implicit_keys[a] for b in address_types: b_address = key_to_address(pubkey, b) - assert(('receive', b_address) in tuple((tx['category'], tx['address']) for tx in txs)) + assert ('receive', b_address) in tuple((tx['category'], tx['address']) for tx in txs) class ImplicitSegwitTest(BitcoinTestFramework): def add_options(self, parser): diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py index 03b285e254..9e813166c5 100755 --- a/test/functional/wallet_importdescriptors.py +++ b/test/functional/wallet_importdescriptors.py @@ -487,8 +487,8 @@ class ImportDescriptorsTest(BitcoinTestFramework): assert_equal(addr, 'bcrt1qp8s25ckjl7gr6x2q3dx3tn2pytwp05upkjztk6ey857tt50r5aeqn6mvr9') # Derived at m/84'/0'/0'/1 change_addr = wmulti_pub.getrawchangeaddress('bech32') assert_equal(change_addr, 'bcrt1qzxl0qz2t88kljdnkzg4n4gapr6kte26390gttrg79x66nt4p04fssj53nl') - assert(send_txid in self.nodes[0].getrawmempool(True)) - assert(send_txid in (x['txid'] for x in wmulti_pub.listunspent(0))) + assert send_txid in self.nodes[0].getrawmempool(True) + assert send_txid in (x['txid'] for x in wmulti_pub.listunspent(0)) assert_equal(wmulti_pub.getwalletinfo()['keypoolsize'], 999) # generate some utxos for next tests diff --git a/test/functional/wallet_labels.py b/test/functional/wallet_labels.py index 34acaee563..a39700f73a 100755 --- a/test/functional/wallet_labels.py +++ b/test/functional/wallet_labels.py @@ -23,11 +23,49 @@ class WalletLabelsTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True - self.num_nodes = 1 + self.num_nodes = 2 def skip_test_if_missing_module(self): self.skip_if_no_wallet() + def invalid_label_name_test(self): + node = self.nodes[0] + address = node.getnewaddress() + pubkey = node.getaddressinfo(address)['pubkey'] + rpc_calls = [ + [node.getnewaddress], + [node.setlabel, address], + [node.getaddressesbylabel], + [node.importpubkey, pubkey], + [node.addmultisigaddress, 1, [pubkey]], + [node.getreceivedbylabel], + [node.listsinceblock, node.getblockhash(0), 1, False, True, False], + ] + if self.options.descriptors: + response = node.importdescriptors([{ + 'desc': f'pkh({pubkey})', + 'label': '*', + 'timestamp': 'now', + }]) + else: + rpc_calls.extend([ + [node.importprivkey, node.dumpprivkey(address)], + [node.importaddress, address], + ]) + + response = node.importmulti([{ + 'scriptPubKey': {'address': address}, + 'label': '*', + 'timestamp': 'now', + }]) + + assert_equal(response[0]['success'], False) + assert_equal(response[0]['error']['code'], -11) + assert_equal(response[0]['error']['message'], "Invalid label name") + + for rpc_call in rpc_calls: + assert_raises_rpc_error(-11, "Invalid label name", *rpc_call, "*") + def run_test(self): # Check that there's no UTXO on the node node = self.nodes[0] @@ -83,8 +121,14 @@ class WalletLabelsTest(BitcoinTestFramework): label.add_receive_address(address) label.verify(node) + # Check listlabels when passing 'purpose' + node2_addr = self.nodes[1].getnewaddress() + node.setlabel(node2_addr, "node2_addr") + assert_equal(node.listlabels(purpose="send"), ["node2_addr"]) + assert_equal(node.listlabels(purpose="receive"), sorted(['coinbase'] + [label.name for label in labels])) + # Check all labels are returned by listlabels. - assert_equal(node.listlabels(), sorted(['coinbase'] + [label.name for label in labels])) + assert_equal(node.listlabels(), sorted(['coinbase'] + [label.name for label in labels] + ["node2_addr"])) # Send a transaction to each label. for label in labels: @@ -138,6 +182,8 @@ class WalletLabelsTest(BitcoinTestFramework): # in the label. This is a no-op. change_label(node, labels[2].addresses[0], labels[2], labels[2]) + self.invalid_label_name_test() + if self.options.descriptors: # This is a descriptor wallet test because of segwit v1+ addresses self.log.info('Check watchonly labels') diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 93bd5b481a..688ac98617 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -258,7 +258,7 @@ class WalletMigrationTest(BitcoinTestFramework): self.log.info("Test migration of a wallet with watchonly imports") imports0 = self.create_legacy_wallet("imports0") - # Exteranl address label + # External address label imports0.setlabel(default.getnewaddress(), "external") # Normal non-watchonly tx @@ -311,6 +311,13 @@ class WalletMigrationTest(BitcoinTestFramework): assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", watchonly.gettransaction, received_txid) assert_equal(len(watchonly.listtransactions(include_watchonly=True)), 3) + # Check that labels were migrated and persisted to watchonly wallet + self.nodes[0].unloadwallet("imports0_watchonly") + self.nodes[0].loadwallet("imports0_watchonly") + labels = watchonly.listlabels() + assert "external" in labels + assert "imported" in labels + def test_no_privkeys(self): default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index a4ffb8df63..2faf6cad8b 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -303,12 +303,8 @@ class MultiWalletTest(BitcoinTestFramework): assert_raises_rpc_error(-18, "Wallet file verification failed. Failed to load database path '{}'. Path does not exist.".format(path), self.nodes[0].loadwallet, 'wallets') # Fail to load duplicate wallets - path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "w1", "wallet.dat") - if self.options.descriptors: - assert_raises_rpc_error(-4, f"Wallet file verification failed. SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another instance of {self.config['environment']['PACKAGE_NAME']}?", self.nodes[0].loadwallet, wallet_names[0]) - else: - assert_raises_rpc_error(-35, "Wallet file verification failed. Refusing to load database. Data file '{}' is already loaded.".format(path), self.nodes[0].loadwallet, wallet_names[0]) - + assert_raises_rpc_error(-35, "Wallet \"w1\" is already loaded.", self.nodes[0].loadwallet, wallet_names[0]) + if not self.options.descriptors: # This tests the default wallet that BDB makes, so SQLite wallet doesn't need to test this # Fail to load duplicate wallets by different ways (directory and filepath) path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "wallet.dat") diff --git a/test/functional/wallet_reorgsrestore.py b/test/functional/wallet_reorgsrestore.py index 2969da4059..1c79c6816c 100755 --- a/test/functional/wallet_reorgsrestore.py +++ b/test/functional/wallet_reorgsrestore.py @@ -94,11 +94,11 @@ class ReorgsRestoreTest(BitcoinTestFramework): tx_after_reorg = self.nodes[1].gettransaction(txid) # Check that normal confirmed tx is confirmed again but with different blockhash assert_equal(tx_after_reorg["confirmations"], 2) - assert(tx_before_reorg["blockhash"] != tx_after_reorg["blockhash"]) + assert tx_before_reorg["blockhash"] != tx_after_reorg["blockhash"] conflicted_after_reorg = self.nodes[1].gettransaction(conflicted_txid) # Check that conflicted tx is confirmed again with blockhash different than previously conflicting tx assert_equal(conflicted_after_reorg["confirmations"], 1) - assert(conflicting["blockhash"] != conflicted_after_reorg["blockhash"]) + assert conflicting["blockhash"] != conflicted_after_reorg["blockhash"] if __name__ == '__main__': ReorgsRestoreTest().main() diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index 7c83b42769..424834323f 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -279,11 +279,11 @@ class WalletSendTest(BitcoinTestFramework): self.log.info("Don't broadcast...") res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=False) - assert(res["hex"]) + assert res["hex"] self.log.info("Return PSBT...") res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, psbt=True) - assert(res["psbt"]) + assert res["psbt"] self.log.info("Create transaction that spends to address, but don't broadcast...") self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=False) @@ -412,10 +412,12 @@ class WalletSendTest(BitcoinTestFramework): assert res["complete"] utxo1 = w0.listunspent()[0] assert_equal(utxo1["amount"], 50) + ERR_NOT_ENOUGH_PRESET_INPUTS = "The preselected coins total amount does not cover the transaction target. " \ + "Please allow other inputs to be automatically selected or include more coins manually" self.test_send(from_wallet=w0, to_wallet=w1, amount=51, inputs=[utxo1], - expect_error=(-4, "Insufficient funds")) + expect_error=(-4, ERR_NOT_ENOUGH_PRESET_INPUTS)) self.test_send(from_wallet=w0, to_wallet=w1, amount=51, inputs=[utxo1], add_inputs=False, - expect_error=(-4, "Insufficient funds")) + expect_error=(-4, ERR_NOT_ENOUGH_PRESET_INPUTS)) res = self.test_send(from_wallet=w0, to_wallet=w1, amount=51, inputs=[utxo1], add_inputs=True, add_to_wallet=False) assert res["complete"] diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 01cd4f28fc..8d25044e43 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -174,7 +174,7 @@ class WalletSignerTest(BitcoinTestFramework): mock_psbt_signed = mock_wallet.walletprocesspsbt(psbt=mock_psbt, sign=True, sighashtype="ALL", bip32derivs=True) mock_psbt_final = mock_wallet.finalizepsbt(mock_psbt_signed["psbt"]) mock_tx = mock_psbt_final["hex"] - assert(mock_wallet.testmempoolaccept([mock_tx])[0]["allowed"]) + assert mock_wallet.testmempoolaccept([mock_tx])[0]["allowed"] # # Create a new wallet and populate with specific public keys, in order # # to work with the mock signed PSBT. @@ -203,7 +203,7 @@ class WalletSignerTest(BitcoinTestFramework): # assert_equal(result[1], {'success': True}) assert_equal(hww.getwalletinfo()["txcount"], 1) - assert(hww.testmempoolaccept([mock_tx])[0]["allowed"]) + assert hww.testmempoolaccept([mock_tx])[0]["allowed"] with open(os.path.join(self.nodes[1].cwd, "mock_psbt"), "w", encoding="utf8") as f: f.write(mock_psbt_signed["psbt"]) @@ -212,13 +212,13 @@ class WalletSignerTest(BitcoinTestFramework): # Don't broadcast transaction yet so the RPC returns the raw hex res = hww.send(outputs={dest:0.5},options={"add_to_wallet": False}) - assert(res["complete"]) + assert res["complete"] assert_equal(res["hex"], mock_tx) self.log.info('Test sendall using hww1') res = hww.sendall(recipients=[{dest:0.5}, hww.getrawchangeaddress()],options={"add_to_wallet": False}) - assert(res["complete"]) + assert res["complete"] assert_equal(res["hex"], mock_tx) # Broadcast transaction so we can bump the fee hww.sendrawtransaction(res["hex"]) diff --git a/test/functional/wallet_taproot.py b/test/functional/wallet_taproot.py index d95ad9e01f..b52892704f 100755 --- a/test/functional/wallet_taproot.py +++ b/test/functional/wallet_taproot.py @@ -244,7 +244,7 @@ class WalletTaprootTest(BitcoinTestFramework): desc_pub = self.make_desc(pattern, privmap, keys, True) assert_equal(self.nodes[0].getdescriptorinfo(desc)['descriptor'], desc_pub) result = addr_gen.importdescriptors([{"desc": desc_pub, "active": True, "timestamp": "now"}]) - assert(result[0]['success']) + assert result[0]['success'] address_type = "bech32m" if "tr" in pattern else "bech32" for i in range(4): addr_g = addr_gen.getnewaddress(address_type=address_type) @@ -260,9 +260,9 @@ class WalletTaprootTest(BitcoinTestFramework): # tr descriptors can be imported result = privs_tr_enabled.importdescriptors([{"desc": desc, "timestamp": "now"}]) - assert(result[0]["success"]) + assert result[0]['success'] result = pubs_tr_enabled.importdescriptors([{"desc": desc_pub, "timestamp": "now"}]) - assert(result[0]["success"]) + assert result[0]["success"] # Cleanup privs_tr_enabled.unloadwallet() @@ -284,9 +284,9 @@ class WalletTaprootTest(BitcoinTestFramework): assert_equal(self.nodes[0].getdescriptorinfo(desc_pay)['descriptor'], desc_pay_pub) assert_equal(self.nodes[0].getdescriptorinfo(desc_change)['descriptor'], desc_change_pub) result = rpc_online.importdescriptors([{"desc": desc_pay, "active": True, "timestamp": "now"}]) - assert(result[0]['success']) + assert result[0]['success'] result = rpc_online.importdescriptors([{"desc": desc_change, "active": True, "timestamp": "now", "internal": True}]) - assert(result[0]['success']) + assert result[0]['success'] address_type = "bech32m" if "tr" in pattern else "bech32" for i in range(4): addr_g = rpc_online.getnewaddress(address_type=address_type) @@ -302,12 +302,12 @@ class WalletTaprootTest(BitcoinTestFramework): # Increase fee_rate to compensate for the wallet's inability to estimate fees for script path spends. res = rpc_online.sendtoaddress(address=self.boring.getnewaddress(), amount=Decimal(ret_amnt) / 100000000, subtractfeefromamount=True, fee_rate=200) self.generatetoaddress(self.nodes[0], 1, self.boring.getnewaddress(), sync_fun=self.no_op) - assert(rpc_online.gettransaction(res)["confirmations"] > 0) + assert rpc_online.gettransaction(res)["confirmations"] > 0 # Cleanup txid = rpc_online.sendall(recipients=[self.boring.getnewaddress()])["txid"] self.generatetoaddress(self.nodes[0], 1, self.boring.getnewaddress(), sync_fun=self.no_op) - assert(rpc_online.gettransaction(txid)["confirmations"] > 0) + assert rpc_online.gettransaction(txid)["confirmations"] > 0 rpc_online.unloadwallet() def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change): @@ -329,16 +329,16 @@ class WalletTaprootTest(BitcoinTestFramework): assert_equal(self.nodes[0].getdescriptorinfo(desc_pay)['descriptor'], desc_pay_pub) assert_equal(self.nodes[0].getdescriptorinfo(desc_change)['descriptor'], desc_change_pub) result = psbt_online.importdescriptors([{"desc": desc_pay_pub, "active": True, "timestamp": "now"}]) - assert(result[0]['success']) + assert result[0]['success'] result = psbt_online.importdescriptors([{"desc": desc_change_pub, "active": True, "timestamp": "now", "internal": True}]) - assert(result[0]['success']) + assert result[0]['success'] result = psbt_offline.importdescriptors([{"desc": desc_pay, "active": True, "timestamp": "now"}]) - assert(result[0]['success']) + assert result[0]['success'] result = psbt_offline.importdescriptors([{"desc": desc_change, "active": True, "timestamp": "now", "internal": True}]) - assert(result[0]['success']) + assert result[0]['success'] for key in keys_pay + keys_change: result = key_only_wallet.importdescriptors([{"desc": descsum_create(f"wpkh({key['xprv']}/*)"), "timestamp":"now"}]) - assert(result[0]["success"]) + assert result[0]["success"] address_type = "bech32m" if "tr" in pattern else "bech32" for i in range(4): addr_g = psbt_online.getnewaddress(address_type=address_type) @@ -375,7 +375,7 @@ class WalletTaprootTest(BitcoinTestFramework): txid = self.nodes[0].sendrawtransaction(rawtx) self.generatetoaddress(self.nodes[0], 1, self.boring.getnewaddress(), sync_fun=self.no_op) - assert(psbt_online.gettransaction(txid)['confirmations'] > 0) + assert psbt_online.gettransaction(txid)['confirmations'] > 0 # Cleanup psbt = psbt_online.sendall(recipients=[self.boring.getnewaddress()], options={"psbt": True})["psbt"] @@ -383,7 +383,7 @@ class WalletTaprootTest(BitcoinTestFramework): rawtx = self.nodes[0].finalizepsbt(res['psbt'])['hex'] txid = self.nodes[0].sendrawtransaction(rawtx) self.generatetoaddress(self.nodes[0], 1, self.boring.getnewaddress(), sync_fun=self.no_op) - assert(psbt_online.gettransaction(txid)['confirmations'] > 0) + assert psbt_online.gettransaction(txid)['confirmations'] > 0 psbt_online.unloadwallet() psbt_offline.unloadwallet() diff --git a/test/lint/all-lint.py b/test/lint/all-lint.py index 34a7b9742a..c7889796c6 100755 --- a/test/lint/all-lint.py +++ b/test/lint/all-lint.py @@ -10,11 +10,12 @@ from glob import glob from pathlib import Path from subprocess import run +from sys import executable exit_code = 0 mod_path = Path(__file__).parent for lint in glob(f"{mod_path}/lint-*.py"): - result = run([lint]) + result = run([executable, lint]) if result.returncode != 0: print(f"^---- failure generated from {lint.split('/')[-1]}") exit_code |= result.returncode diff --git a/test/lint/lint-spelling.py b/test/lint/lint-spelling.py index 14d7d13a75..ac0bddeaa6 100755 --- a/test/lint/lint-spelling.py +++ b/test/lint/lint-spelling.py @@ -12,7 +12,7 @@ Note: Will exit successfully regardless of spelling errors. from subprocess import check_output, STDOUT, CalledProcessError IGNORE_WORDS_FILE = 'test/lint/spelling.ignore-words.txt' -FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/leveldb/", ":(exclude)src/crc32c/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)src/secp256k1/", ":(exclude)src/minisketch/", ":(exclude)contrib/builder-keys/keys.txt", ":(exclude)contrib/guix/patches"] +FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/leveldb/", ":(exclude)src/crc32c/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)src/secp256k1/", ":(exclude)src/minisketch/", ":(exclude)contrib/guix/patches"] def check_codespell_install(): |