diff options
136 files changed, 1903 insertions, 821 deletions
diff --git a/.cirrus.yml b/.cirrus.yml index 5c5942e2d5..eb2414dc0a 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -43,7 +43,7 @@ env: # Global defaults # The following specific types should exist, with the following requirements: # - small: For an x86_64 machine, recommended to have 2 CPUs and 8 GB of memory. # - medium: For an x86_64 machine, recommended to have 4 CPUs and 16 GB of memory. -# - mantic: For a machine running the Linux kernel shipped with exaclty Ubuntu Mantic 23.10. The machine is recommended to have 4 CPUs and 16 GB of memory. +# - noble: For a machine running the Linux kernel shipped with exaclty Ubuntu Noble 24.04. The machine is recommended to have 4 CPUs and 16 GB of memory. # - arm64: For an aarch64 machine, recommended to have 2 CPUs and 8 GB of memory. # https://cirrus-ci.org/guide/tips-and-tricks/#sharing-configuration-between-tasks @@ -168,7 +168,7 @@ task: << : *GLOBAL_TASK_TEMPLATE persistent_worker: labels: - type: mantic # Must use this specific worker (needed for USDT functional tests) + type: noble # Must use this specific worker (needed for USDT functional tests) env: FILE_ENV: "./ci/test/00_setup_env_native_asan.sh" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cdf8e8e6c5..c8c4cfa583 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,12 +62,12 @@ jobs: echo "TEST_BASE=$(git rev-list -n$((${{ env.MAX_COUNT }} + 1)) --reverse HEAD ^$(git rev-list -n1 --merges HEAD)^@ | head -1)" >> "$GITHUB_ENV" - run: | sudo apt-get update - sudo apt-get install clang ccache build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev systemtap-sdt-dev libminiupnpc-dev libnatpmp-dev libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools qtwayland5 libqrencode-dev -y + sudo apt-get install clang-15 ccache build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev systemtap-sdt-dev libminiupnpc-dev libnatpmp-dev libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools qtwayland5 libqrencode-dev -y - name: Compile and run tests run: | # Run tests on commits after the last merge commit and before the PR head commit # Use clang++, because it is a bit faster and uses less memory than g++ - git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && ./autogen.sh && CC=clang CXX=clang++ ./configure && make clean && make -j $(nproc) check && ./test/functional/test_runner.py -j $(( $(nproc) * 2 ))" ${{ env.TEST_BASE }} + git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && ./autogen.sh && CC=clang-15 CXX=clang++-15 ./configure && make clean && make -j $(nproc) check && ./test/functional/test_runner.py -j $(( $(nproc) * 2 ))" ${{ env.TEST_BASE }} macos-native-x86_64: name: 'macOS 13 native, x86_64, no depends, sqlite only, gui' @@ -282,10 +282,6 @@ jobs: run: py -3 test\util\rpcauth-test.py - name: Run functional tests - # Don't run functional tests for pull requests. - # The test suit regularly fails to complete in windows native github - # actions as a child process stops making progress. The root cause has - # not yet been determined. - # Discussed in https://github.com/bitcoin/bitcoin/pull/28509 - if: github.event_name != 'pull_request' - run: py -3 test\functional\test_runner.py --jobs $env:NUMBER_OF_PROCESSORS --ci --quiet --tmpdirprefix=$env:RUNNER_TEMP --combinedlogslen=99999999 --timeout-factor=$env:TEST_RUNNER_TIMEOUT_FACTOR --extended + env: + TEST_RUNNER_EXTRA: ${{ github.event_name != 'pull_request' && '--extended' || '' }} + run: py -3 test\functional\test_runner.py --jobs $env:NUMBER_OF_PROCESSORS --ci --quiet --tmpdirprefix=$env:RUNNER_TEMP --combinedlogslen=99999999 --timeout-factor=$env:TEST_RUNNER_TIMEOUT_FACTOR $env:TEST_RUNNER_EXTRA diff --git a/build-aux/m4/ax_cxx_compile_stdcxx.m4 b/build-aux/m4/ax_cxx_compile_stdcxx.m4 index 51a35054d0..8a2df5627f 100644 --- a/build-aux/m4/ax_cxx_compile_stdcxx.m4 +++ b/build-aux/m4/ax_cxx_compile_stdcxx.m4 @@ -983,7 +983,7 @@ m4_define([_AX_CXX_COMPILE_STDCXX_testbody_new_in_20], [[ #error "This is not a C++ compiler" -#elif __cplusplus < 202002L +#elif __cplusplus < 201709L // Temporary patch on top of upstream to allow g++-10 #error "This is not a C++20 compiler" diff --git a/build_msvc/README.md b/build_msvc/README.md index cc2cd91e13..f97c7ca59c 100644 --- a/build_msvc/README.md +++ b/build_msvc/README.md @@ -34,6 +34,8 @@ To build Bitcoin Core with the GUI, a static build of Qt is required. 1. Download a single ZIP archive of Qt source code from https://download.qt.io/official_releases/qt/ (e.g., [`qt-everywhere-opensource-src-5.15.11.zip`](https://download.qt.io/official_releases/qt/5.15/5.15.11/single/qt-everywhere-opensource-src-5.15.11.zip)), and expand it into a dedicated folder. The following instructions assume that this folder is `C:\dev\qt-source`. +> 💡 **Tip:** If you use the default path with "Extract All" for the Qt source code zip file, and end up with something like `C:\dev\qt-everywhere-opensource-src-5.15.11\qt-everywhere-src-5.15.11`, you are likely to encounter a "path too long" error when building. To fix the problem move the source files to a shorter path such as the recommended `C:\dev\qt-source`. + 2. Open "x64 Native Tools Command Prompt for VS 2022", and input the following commands: ```cmd cd C:\dev\qt-source diff --git a/build_msvc/common.init.vcxproj.in b/build_msvc/common.init.vcxproj.in index d54e559c9f..950cc37f0a 100644 --- a/build_msvc/common.init.vcxproj.in +++ b/build_msvc/common.init.vcxproj.in @@ -57,7 +57,7 @@ <ItemDefinitionGroup Condition="'$(Configuration)'=='Release'"> <ClCompile> - <Optimization>Disabled</Optimization> + <Optimization>MaxSpeed</Optimization> <WholeProgramOptimization>false</WholeProgramOptimization> <FunctionLevelLinking>true</FunctionLevelLinking> <IntrinsicFunctions>true</IntrinsicFunctions> diff --git a/build_msvc/common.qt.init.vcxproj b/build_msvc/common.qt.init.vcxproj index cc8063e545..dabbec707f 100644 --- a/build_msvc/common.qt.init.vcxproj +++ b/build_msvc/common.qt.init.vcxproj @@ -13,4 +13,10 @@ <QtDebugLibraries>$(QtPluginsLibraryDir)\platforms\qwindowsd.lib;$(QtPluginsLibraryDir)\platforms\qminimald.lib;$(QtPluginsLibraryDir)\styles\qwindowsvistastyled.lib;$(QtLibraryDir)\*d.lib;Wtsapi32.lib;crypt32.lib;userenv.lib;netapi32.lib;imm32.lib;Dwmapi.lib;version.lib;winmm.lib;UxTheme.lib</QtDebugLibraries> </PropertyGroup> + <ItemDefinitionGroup> + <ClCompile> + <PreprocessorDefinitions>QT_NO_KEYWORDS;QT_USE_QSTRINGBUILDER;%(PreprocessorDefinitions)</PreprocessorDefinitions> + </ClCompile> + </ItemDefinitionGroup> + </Project> diff --git a/build_msvc/libleveldb/libleveldb.vcxproj b/build_msvc/libleveldb/libleveldb.vcxproj index 2914eb2cfb..eacfbb2641 100644 --- a/build_msvc/libleveldb/libleveldb.vcxproj +++ b/build_msvc/libleveldb/libleveldb.vcxproj @@ -51,7 +51,7 @@ <ItemDefinitionGroup> <ClCompile> <PreprocessorDefinitions>HAVE_CRC32C=0;HAVE_SNAPPY=0;LEVELDB_IS_BIG_ENDIAN=0;_UNICODE;UNICODE;_CRT_NONSTDC_NO_DEPRECATE;LEVELDB_PLATFORM_WINDOWS;%(PreprocessorDefinitions)</PreprocessorDefinitions> - <DisableSpecificWarnings>4244;4267</DisableSpecificWarnings> + <DisableSpecificWarnings>4244;4267;4722</DisableSpecificWarnings> <AdditionalIncludeDirectories>..\..\src\leveldb;..\..\src\leveldb\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> </ClCompile> </ItemDefinitionGroup> diff --git a/build_msvc/test_bitcoin/test_bitcoin.vcxproj b/build_msvc/test_bitcoin/test_bitcoin.vcxproj index de836bc01d..2a78f6f2a1 100644 --- a/build_msvc/test_bitcoin/test_bitcoin.vcxproj +++ b/build_msvc/test_bitcoin/test_bitcoin.vcxproj @@ -59,6 +59,11 @@ <Project>{18430fef-6b61-4c53-b396-718e02850f1b}</Project> </ProjectReference> </ItemGroup> + <ItemDefinitionGroup> + <ClCompile> + <DisableSpecificWarnings>4018;4244;4267;4703;4715;4805</DisableSpecificWarnings> + </ClCompile> + </ItemDefinitionGroup> <Target Name="RawBenchHeaderGen" BeforeTargets="PrepareForBuild"> <PropertyGroup> <ErrorText>There was an error executing the JSON test header generation task.</ErrorText> diff --git a/ci/test/00_setup_env_mac.sh b/ci/test/00_setup_env_mac.sh index b952d49074..1651c5ec26 100755 --- a/ci/test/00_setup_env_mac.sh +++ b/ci/test/00_setup_env_mac.sh @@ -11,9 +11,9 @@ export SDK_URL=${SDK_URL:-https://bitcoincore.org/depends-sources/sdks} export CONTAINER_NAME=ci_macos_cross export CI_IMAGE_NAME_TAG="docker.io/ubuntu:22.04" export HOST=x86_64-apple-darwin -export PACKAGES="cmake libz-dev zip" -export XCODE_VERSION=12.2 -export XCODE_BUILD_ID=12B45b +export PACKAGES="cmake zip" +export XCODE_VERSION=15.0 +export XCODE_BUILD_ID=15A240d export RUN_UNIT_TESTS=false export RUN_FUNCTIONAL_TESTS=false export GOAL="deploy" diff --git a/ci/test/00_setup_env_native_asan.sh b/ci/test/00_setup_env_native_asan.sh index 93d84bf17e..60486f8f61 100755 --- a/ci/test/00_setup_env_native_asan.sh +++ b/ci/test/00_setup_env_native_asan.sh @@ -6,6 +6,7 @@ export LC_ALL=C.UTF-8 +export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04" # Only install BCC tracing packages in Cirrus CI. if [[ "${CIRRUS_CI}" == "true" ]]; then BPFCC_PACKAGE="bpfcc-tools linux-headers-$(uname --kernel-release)" @@ -17,7 +18,6 @@ fi export CONTAINER_NAME=ci_native_asan export PACKAGES="systemtap-sdt-dev clang-17 llvm-17 libclang-rt-17-dev python3-zmq qtbase5-dev qttools5-dev-tools libevent-dev libboost-dev libdb5.3++-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev libqrencode-dev libsqlite3-dev ${BPFCC_PACKAGE}" -export CI_IMAGE_NAME_TAG="docker.io/ubuntu:23.10" # This version will reach EOL in Jul 2024, and can be replaced by "ubuntu:24.04" (or anything else that ships the wanted clang version). export NO_DEPENDS=1 export GOAL="install" export BITCOIN_CONFIG="--enable-c++20 --enable-usdt --enable-zmq --with-incompatible-bdb --with-gui=qt5 \ diff --git a/ci/test/00_setup_env_native_fuzz.sh b/ci/test/00_setup_env_native_fuzz.sh index 3585b2b417..abee3c1541 100755 --- a/ci/test/00_setup_env_native_fuzz.sh +++ b/ci/test/00_setup_env_native_fuzz.sh @@ -6,7 +6,7 @@ export LC_ALL=C.UTF-8 -export CI_IMAGE_NAME_TAG="docker.io/ubuntu:23.10" # This version will reach EOL in Jul 2024, and can be replaced by "ubuntu:24.04" (or anything else that ships the wanted clang version). +export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04" export CONTAINER_NAME=ci_native_fuzz export PACKAGES="clang-17 llvm-17 libclang-rt-17-dev libevent-dev libboost-dev libsqlite3-dev" export NO_DEPENDS=1 diff --git a/ci/test/00_setup_env_native_tidy.sh b/ci/test/00_setup_env_native_tidy.sh index 6b0c708f19..c03392af06 100755 --- a/ci/test/00_setup_env_native_tidy.sh +++ b/ci/test/00_setup_env_native_tidy.sh @@ -6,7 +6,7 @@ export LC_ALL=C.UTF-8 -export CI_IMAGE_NAME_TAG="docker.io/ubuntu:23.10" # This version will reach EOL in Jul 2024, and can be replaced by "ubuntu:24.04" (or anything else that ships the wanted clang version). +export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04" export CONTAINER_NAME=ci_native_tidy export TIDY_LLVM_V="17" export PACKAGES="clang-${TIDY_LLVM_V} libclang-${TIDY_LLVM_V}-dev llvm-${TIDY_LLVM_V}-dev libomp-${TIDY_LLVM_V}-dev clang-tidy-${TIDY_LLVM_V} jq bear cmake libevent-dev libboost-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev systemtap-sdt-dev libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools libqrencode-dev libsqlite3-dev libdb++-dev" diff --git a/ci/test/00_setup_env_native_tsan.sh b/ci/test/00_setup_env_native_tsan.sh index 67c29d4bc8..aa23bad809 100755 --- a/ci/test/00_setup_env_native_tsan.sh +++ b/ci/test/00_setup_env_native_tsan.sh @@ -7,7 +7,7 @@ export LC_ALL=C.UTF-8 export CONTAINER_NAME=ci_native_tsan -export CI_IMAGE_NAME_TAG="docker.io/ubuntu:23.10" # This version will reach EOL in Jul 2024, and can be replaced by "ubuntu:24.04" (or anything else that ships the wanted clang version). +export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04" export PACKAGES="clang-17 llvm-17 libclang-rt-17-dev libc++abi-17-dev libc++-17-dev python3-zmq" export DEP_OPTS="CC=clang-17 CXX='clang++-17 -stdlib=libc++'" export GOAL="install" diff --git a/configure.ac b/configure.ac index fd9e10d44a..18b00a2b16 100644 --- a/configure.ac +++ b/configure.ac @@ -96,18 +96,8 @@ case $host in ;; esac -AC_ARG_ENABLE([c++20], - [AS_HELP_STRING([--enable-c++20], - [enable compilation in c++20 mode (disabled by default)])], - [use_cxx20=$enableval], - [use_cxx20=no]) - -dnl Require C++17 compiler (no GNU extensions) -if test "$use_cxx20" = "no"; then -AX_CXX_COMPILE_STDCXX([17], [noext], [mandatory]) -else +dnl Require C++20 compiler (no GNU extensions) AX_CXX_COMPILE_STDCXX([20], [noext], [mandatory]) -fi dnl Unless the user specified OBJCXX, force it to be the same as CXX. This ensures dnl that we get the same -std flags for both. @@ -446,8 +436,8 @@ if test "$CXXFLAGS_overridden" = "no"; then AX_CHECK_COMPILE_FLAG([-Wlogical-op], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wlogical-op"], [], [$CXXFLAG_WERROR]) AX_CHECK_COMPILE_FLAG([-Woverloaded-virtual], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Woverloaded-virtual"], [], [$CXXFLAG_WERROR]) AX_CHECK_COMPILE_FLAG([-Wsuggest-override], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsuggest-override"], [], [$CXXFLAG_WERROR]) - AX_CHECK_COMPILE_FLAG([-Wunreachable-code-loop-increment], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code-loop-increment"], [], [$CXXFLAG_WERROR]) AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wimplicit-fallthrough"], [], [$CXXFLAG_WERROR]) + AX_CHECK_COMPILE_FLAG([-Wunreachable-code], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code"], [], [$CXXFLAG_WERROR]) if test "$suppress_external_warnings" != "no" ; then AX_CHECK_COMPILE_FLAG([-Wdocumentation], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdocumentation"], [], [$CXXFLAG_WERROR]) diff --git a/contrib/devtools/symbol-check.py b/contrib/devtools/symbol-check.py index e3e8e398c2..ff1678116d 100755 --- a/contrib/devtools/symbol-check.py +++ b/contrib/devtools/symbol-check.py @@ -235,7 +235,7 @@ def check_MACHO_min_os(binary) -> bool: return False def check_MACHO_sdk(binary) -> bool: - if binary.build_version.sdk == [11, 0, 0]: + if binary.build_version.sdk == [14, 0, 0]: return True return False diff --git a/contrib/devtools/utxo_snapshot.sh b/contrib/devtools/utxo_snapshot.sh index ad2ec26651..fbb8591965 100755 --- a/contrib/devtools/utxo_snapshot.sh +++ b/contrib/devtools/utxo_snapshot.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # -# Copyright (c) 2019 The Bitcoin Core developers +# Copyright (c) 2019-2023 The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. # @@ -8,6 +8,8 @@ export LC_ALL=C set -ueo pipefail +NETWORK_DISABLED=false + if (( $# < 3 )); then echo 'Usage: utxo_snapshot.sh <generate-at-height> <snapshot-out-path> <bitcoin-cli-call ...>' echo @@ -26,9 +28,60 @@ OUTPUT_PATH="${1}"; shift; # Most of the calls we make take a while to run, so pad with a lengthy timeout. BITCOIN_CLI_CALL="${*} -rpcclienttimeout=9999999" +# Check if the node is pruned and get the pruned block height +PRUNED=$( ${BITCOIN_CLI_CALL} getblockchaininfo | awk '/pruneheight/ {print $2}' | tr -d ',' ) + +if (( GENERATE_AT_HEIGHT < PRUNED )); then + echo "Error: The requested snapshot height (${GENERATE_AT_HEIGHT}) should be greater than the pruned block height (${PRUNED})." + exit 1 +fi + +# Early exit if file at OUTPUT_PATH already exists +if [[ -e "$OUTPUT_PATH" ]]; then + (>&2 echo "Error: $OUTPUT_PATH already exists or is not a valid path.") + exit 1 +fi + +# Validate that the path is correct +if [[ "${OUTPUT_PATH}" != "-" && ! -d "$(dirname "${OUTPUT_PATH}")" ]]; then + (>&2 echo "Error: The directory $(dirname "${OUTPUT_PATH}") does not exist.") + exit 1 +fi + +function cleanup { + (>&2 echo "Restoring chain to original height; this may take a while") + ${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}" + + if $NETWORK_DISABLED; then + (>&2 echo "Restoring network activity") + ${BITCOIN_CLI_CALL} setnetworkactive true + fi +} + +function early_exit { + (>&2 echo "Exiting due to Ctrl-C") + cleanup + exit 1 +} + +# Prompt the user to disable network activity +read -p "Do you want to disable network activity (setnetworkactive false) before running invalidateblock? (Y/n): " -r +if [[ "$REPLY" =~ ^[Yy]*$ || -z "$REPLY" ]]; then + # User input is "Y", "y", or Enter key, proceed with the action + NETWORK_DISABLED=true + (>&2 echo "Disabling network activity") + ${BITCOIN_CLI_CALL} setnetworkactive false +else + (>&2 echo "Network activity remains enabled") +fi + # Block we'll invalidate/reconsider to rewind/fast-forward the chain. PIVOT_BLOCKHASH=$($BITCOIN_CLI_CALL getblockhash $(( GENERATE_AT_HEIGHT + 1 )) ) +# Trap for normal exit and Ctrl-C +trap cleanup EXIT +trap early_exit INT + (>&2 echo "Rewinding chain back to height ${GENERATE_AT_HEIGHT} (by invalidating ${PIVOT_BLOCKHASH}); this may take a while") ${BITCOIN_CLI_CALL} invalidateblock "${PIVOT_BLOCKHASH}" @@ -39,6 +92,3 @@ else (>&2 echo "Generating UTXO snapshot...") ${BITCOIN_CLI_CALL} dumptxoutset "${OUTPUT_PATH}" fi - -(>&2 echo "Restoring chain to original height; this may take a while") -${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}" diff --git a/contrib/macdeploy/README.md b/contrib/macdeploy/README.md index 16fb0dad21..ea599df3d8 100644 --- a/contrib/macdeploy/README.md +++ b/contrib/macdeploy/README.md @@ -14,51 +14,45 @@ When complete, it will have produced `Bitcoin-Core.zip`. A free Apple Developer Account is required to proceed. -Our current macOS SDK -(`Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers.tar.gz`) -can be extracted from -[Xcode_12.2.xip](https://download.developer.apple.com/Developer_Tools/Xcode_12.2/Xcode_12.2.xip). +Our macOS SDK can be extracted from +[Xcode_15.xip](https://download.developer.apple.com/Developer_Tools/Xcode_15/Xcode_15.xip). Alternatively, after logging in to your account go to 'Downloads', then 'More' -and search for [`Xcode 12.2`](https://developer.apple.com/download/all/?q=Xcode%2012.2). +and search for [`Xcode 15`](https://developer.apple.com/download/all/?q=Xcode%2015). An Apple ID and cookies enabled for the hostname are needed to download this. -The `sha256sum` of the downloaded XIP archive should be `28d352f8c14a43d9b8a082ac6338dc173cb153f964c6e8fb6ba389e5be528bd0`. +The `sha256sum` of the downloaded XIP archive should be `4daaed2ef2253c9661779fa40bfff50655dc7ec45801aba5a39653e7bcdde48e`. -After Xcode version 7.x, Apple started shipping the `Xcode.app` in a `.xip` -archive. This makes the SDK less-trivial to extract on non-macOS machines. One -approach (tested on Debian Buster) is outlined below: +To extract the `.xip` on Linux: ```bash # Install/clone tools needed for extracting Xcode.app apt install cpio git clone https://github.com/bitcoin-core/apple-sdk-tools.git -# Unpack Xcode_12.2.xip and place the resulting Xcode.app in your current +# Unpack the .xip and place the resulting Xcode.app in your current # working directory -python3 apple-sdk-tools/extract_xcode.py -f Xcode_12.2.xip | cpio -d -i +python3 apple-sdk-tools/extract_xcode.py -f Xcode_15.xip | cpio -d -i ``` -On macOS the process is more straightforward: +On macOS: ```bash -xip -x Xcode_12.2.xip +xip -x Xcode_15.xip ``` -### Step 2: Generating `Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers.tar.gz` from `Xcode.app` +### Step 2: Generating the SDK tarball from `Xcode.app` -To generate `Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers.tar.gz`, run -the script [`gen-sdk`](./gen-sdk) with the path to `Xcode.app` (extracted in the -previous stage) as the first argument. +To generate the SDK, run the script [`gen-sdk`](./gen-sdk) with the +path to `Xcode.app` (extracted in the previous stage) as the first argument. ```bash -# Generate a Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers.tar.gz from -# the supplied Xcode.app ./contrib/macdeploy/gen-sdk '/path/to/Xcode.app' ``` -The `sha256sum` of the generated TAR.GZ archive should be `df75d30ecafc429e905134333aeae56ac65fac67cb4182622398fd717df77619`. +The generated archive should be: `Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz`. +The `sha256sum` should be `c0c2e7bb92c1fee0c4e9f3a485e4530786732d6c6dd9e9f418c282aa6892f55d`. ## Deterministic macOS App Notes diff --git a/contrib/macdeploy/gen-sdk b/contrib/macdeploy/gen-sdk index 6efaaccb8e..b73f5cba14 100755 --- a/contrib/macdeploy/gen-sdk +++ b/contrib/macdeploy/gen-sdk @@ -62,9 +62,6 @@ def run(): out_name = "Xcode-{xcode_version}-{xcode_build_id}-extracted-SDK-with-libcxx-headers".format(xcode_version=xcode_version, xcode_build_id=xcode_build_id) - xcode_libcxx_dir = xcode_app.joinpath("Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1") - assert xcode_libcxx_dir.is_dir() - if args.out_sdktgz: out_sdktgz_path = pathlib.Path(args.out_sdktgz_path) else: @@ -72,7 +69,7 @@ def run(): out_sdktgz_path = pathlib.Path("./{}.tar.gz".format(out_name)) def tarfp_add_with_base_change(tarfp, dir_to_add, alt_base_dir): - """Add all files in dir_to_add to tarfp, but prepent MEMBERPREFIX to the files' + """Add all files in dir_to_add to tarfp, but prepent alt_base_dir to the files' names e.g. if the only file under /root/bazdir is /root/bazdir/qux, invoking: @@ -107,8 +104,6 @@ def run(): with tarfile.open(mode="w", fileobj=gzf, format=tarfile.GNU_FORMAT) as tarfp: print("Adding MacOSX SDK {} files...".format(sdk_version)) tarfp_add_with_base_change(tarfp, sdk_dir, out_name) - print("Adding libc++ headers...") - tarfp_add_with_base_change(tarfp, xcode_libcxx_dir, "{}/usr/include/c++/v1".format(out_name)) print("Done! Find the resulting gzipped tarball at:") print(out_sdktgz_path.resolve()) diff --git a/depends/Makefile b/depends/Makefile index 3169117633..319c3498df 100644 --- a/depends/Makefile +++ b/depends/Makefile @@ -49,7 +49,7 @@ NO_HARDEN ?= FALLBACK_DOWNLOAD_PATH ?= https://bitcoincore.org/depends-sources C_STANDARD ?= c11 -CXX_STANDARD ?= c++17 +CXX_STANDARD ?= c++20 BUILD = $(shell ./config.guess) HOST ?= $(BUILD) diff --git a/depends/README.md b/depends/README.md index 41b70264c3..8af5e36bfd 100644 --- a/depends/README.md +++ b/depends/README.md @@ -48,7 +48,7 @@ The paths are automatically configured and no other options are needed unless ta #### For macOS cross compilation - sudo apt-get install curl bsdmainutils cmake libz-dev zip + sudo apt-get install curl bsdmainutils cmake zip Note: You must obtain the macOS SDK before proceeding with a cross-compile. Under the depends directory, create a subdirectory named `SDKs`. @@ -98,7 +98,7 @@ The following can be set when running make: `make FOO=bar` - `SDK_PATH`: Path where SDKs can be found (used by macOS) - `FALLBACK_DOWNLOAD_PATH`: If a source file can't be fetched, try here before giving up - `C_STANDARD`: Set the C standard version used. Defaults to `c11`. -- `CXX_STANDARD`: Set the C++ standard version used. Defaults to `c++17`. +- `CXX_STANDARD`: Set the C++ standard version used. Defaults to `c++20`. - `NO_BOOST`: Don't download/build/cache Boost - `NO_LIBEVENT`: Don't download/build/cache Libevent - `NO_QT`: Don't download/build/cache Qt and its dependencies diff --git a/depends/hosts/darwin.mk b/depends/hosts/darwin.mk index ecd45540cf..a89c82e408 100644 --- a/depends/hosts/darwin.mk +++ b/depends/hosts/darwin.mk @@ -1,7 +1,7 @@ OSX_MIN_VERSION=11.0 -OSX_SDK_VERSION=11.0 -XCODE_VERSION=12.2 -XCODE_BUILD_ID=12B45b +OSX_SDK_VERSION=14.0 +XCODE_VERSION=15.0 +XCODE_BUILD_ID=15A240d LD64_VERSION=711 OSX_SDK=$(SDK_PATH)/Xcode-$(XCODE_VERSION)-$(XCODE_BUILD_ID)-extracted-SDK-with-libcxx-headers @@ -71,6 +71,10 @@ $(foreach TOOL,$(cctools_TOOLS),$(eval darwin_$(TOOL) = $$(build_prefix)/bin/$$( # # Adds the desired paths from the SDK # +# -platform_version +# +# Indicate to the linker the platform, the oldest supported version, +# and the SDK used. darwin_CC=env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH \ -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH \ @@ -91,6 +95,7 @@ darwin_CXX=env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH \ darwin_CFLAGS=-pipe -std=$(C_STANDARD) darwin_CXXFLAGS=-pipe -std=$(CXX_STANDARD) +darwin_LDFLAGS=-Wl,-platform_version,macos,$(OSX_MIN_VERSION),$(OSX_SDK_VERSION) ifneq ($(LTO),) darwin_CFLAGS += -flto diff --git a/depends/packages/boost.mk b/depends/packages/boost.mk index ebc097d686..ab43764b38 100644 --- a/depends/packages/boost.mk +++ b/depends/packages/boost.mk @@ -3,6 +3,11 @@ $(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=71feeed900fbccca04a3b4f2f84a7c217186f28a940ed8b7ed4725986baf99fa +$(package)_patches=process_macos_sdk.patch + +define $(package)_preprocess_cmds + patch -p1 < $($(package)_patch_dir)/process_macos_sdk.patch +endef define $(package)_stage_cmds mkdir -p $($(package)_staging_prefix_dir)/include && \ diff --git a/depends/packages/capnp.mk b/depends/packages/capnp.mk index 47df202771..2465c8091b 100644 --- a/depends/packages/capnp.mk +++ b/depends/packages/capnp.mk @@ -4,18 +4,20 @@ $(package)_download_path=$(native_$(package)_download_path) $(package)_download_file=$(native_$(package)_download_file) $(package)_file_name=$(native_$(package)_file_name) $(package)_sha256_hash=$(native_$(package)_sha256_hash) -$(package)_dependencies=native_$(package) +# Hardcode library install path to "lib" to match the PKG_CONFIG_PATH +# setting in depends/config.site.in, which also hardcodes "lib". +# Without this setting, cmake by default would use the OS library +# directory, which might be "lib64" or something else, not "lib", on multiarch systems. define $(package)_set_vars := -$(package)_config_opts := --with-external-capnp -$(package)_config_opts += --without-openssl -$(package)_config_opts += CAPNP="$$(native_capnp_prefixbin)/capnp" -$(package)_config_opts += CAPNP_CXX="$$(native_capnp_prefixbin)/capnp-c++" -$(package)_config_opts_android := --disable-shared + $(package)_config_opts := -DBUILD_TESTING=OFF + $(package)_config_opts += -DWITH_OPENSSL=OFF + $(package)_config_opts += -DWITH_ZLIB=OFF + $(package)_config_opts += -DCMAKE_INSTALL_LIBDIR=lib/ endef define $(package)_config_cmds - $($(package)_autoconf) + $($(package)_cmake) . endef define $(package)_build_cmds @@ -25,3 +27,7 @@ endef define $(package)_stage_cmds $(MAKE) DESTDIR=$($(package)_staging_dir) install endef + +define $(package)_postprocess_cmds + rm -rf lib/pkgconfig +endef diff --git a/depends/packages/libmultiprocess.mk b/depends/packages/libmultiprocess.mk index 765d649377..d237f52dbb 100644 --- a/depends/packages/libmultiprocess.mk +++ b/depends/packages/libmultiprocess.mk @@ -8,7 +8,13 @@ ifneq ($(host),$(build)) $(package)_dependencies += native_capnp endif +# Hardcode library install path to "lib" to match the PKG_CONFIG_PATH +# setting in depends/config.site.in, which also hardcodes "lib". +# Without this setting, cmake by default would use the OS library +# directory, which might be "lib64" or something else, not "lib", on multiarch systems. define $(package)_set_vars := +$(package)_config_opts += -DCMAKE_INSTALL_LIBDIR=lib/ +$(package)_config_opts += -DCMAKE_POSITION_INDEPENDENT_CODE=ON ifneq ($(host),$(build)) $(package)_config_opts := -DCAPNP_EXECUTABLE="$$(native_capnp_prefixbin)/capnp" $(package)_config_opts += -DCAPNPC_CXX_EXECUTABLE="$$(native_capnp_prefixbin)/capnpc-c++" diff --git a/depends/packages/native_capnp.mk b/depends/packages/native_capnp.mk index ad87eed354..484e78d5d9 100644 --- a/depends/packages/native_capnp.mk +++ b/depends/packages/native_capnp.mk @@ -6,11 +6,13 @@ $(package)_file_name=capnproto-cxx-$($(package)_version).tar.gz $(package)_sha256_hash=0f7f4b8a76a2cdb284fddef20de8306450df6dd031a47a15ac95bc43c3358e09 define $(package)_set_vars - $(package)_config_opts = --without-openssl + $(package)_config_opts := -DBUILD_TESTING=OFF + $(package)_config_opts += -DWITH_OPENSSL=OFF + $(package)_config_opts += -DWITH_ZLIB=OFF endef define $(package)_config_cmds - $($(package)_autoconf) + $($(package)_cmake) . endef define $(package)_build_cmds @@ -20,3 +22,7 @@ endef define $(package)_stage_cmds $(MAKE) DESTDIR=$($(package)_staging_dir) install endef + +define $(package)_postprocess_cmds + rm -rf lib/pkgconfig +endef diff --git a/depends/packages/qt.mk b/depends/packages/qt.mk index b6d8864383..ecf3334aa5 100644 --- a/depends/packages/qt.mk +++ b/depends/packages/qt.mk @@ -23,6 +23,7 @@ $(package)_patches += guix_cross_lib_path.patch $(package)_patches += fix-macos-linker.patch $(package)_patches += memory_resource.patch $(package)_patches += windows_lto.patch +$(package)_patches += fix-minimum-macos.patch $(package)_qttranslations_file_name=qttranslations-$($(package)_suffix) $(package)_qttranslations_sha256_hash=a31785948c640b7c66d9fe2db4993728ca07f64e41c560b3625ad191b276ff20 @@ -40,7 +41,7 @@ $(package)_config_opts_release += -silent $(package)_config_opts_debug = -debug $(package)_config_opts_debug += -optimized-tools $(package)_config_opts += -bindir $(build_prefix)/bin -$(package)_config_opts += -c++std c++17 +$(package)_config_opts += -c++std c++2a $(package)_config_opts += -confirm-license $(package)_config_opts += -hostprefix $(build_prefix) $(package)_config_opts += -no-compile-examples @@ -239,6 +240,7 @@ endef define $(package)_preprocess_cmds cp $($(package)_patch_dir)/qt.pro qt.pro && \ cp $($(package)_patch_dir)/qttools_src.pro qttools/src/src.pro && \ + patch -p1 -i $($(package)_patch_dir)/fix-minimum-macos.patch && \ patch -p1 -i $($(package)_patch_dir)/fix-macos-linker.patch && \ patch -p1 -i $($(package)_patch_dir)/dont_hardcode_pwd.patch && \ patch -p1 -i $($(package)_patch_dir)/fix_qt_pkgconfig.patch && \ diff --git a/depends/patches/boost/process_macos_sdk.patch b/depends/patches/boost/process_macos_sdk.patch new file mode 100644 index 0000000000..ebc556d972 --- /dev/null +++ b/depends/patches/boost/process_macos_sdk.patch @@ -0,0 +1,27 @@ +Fix Boost Process compilation with macOS 14 SDK. +Can be dropped with Boost 1.84.0. +https://github.com/boostorg/process/pull/343. +https://github.com/boostorg/process/issues/342. + +diff --git a/boost/process/detail/posix/handles.hpp b/boost/process/detail/posix/handles.hpp +index cd9e1ce5a..304e77b1c 100644 +--- a/boost/process/detail/posix/handles.hpp ++++ b/boost/process/detail/posix/handles.hpp +@@ -33,7 +33,7 @@ inline std::vector<native_handle_type> get_handles(std::error_code & ec) + else + ec.clear(); + +- auto my_fd = ::dirfd(dir.get()); ++ auto my_fd = dirfd(dir.get()); + + struct ::dirent * ent_p; + +@@ -117,7 +117,7 @@ struct limit_handles_ : handler_base_ext + return; + } + +- auto my_fd = ::dirfd(dir); ++ auto my_fd = dirfd(dir); + struct ::dirent * ent_p; + + while ((ent_p = readdir(dir)) != nullptr) diff --git a/depends/patches/qt/fix-minimum-macos.patch b/depends/patches/qt/fix-minimum-macos.patch new file mode 100644 index 0000000000..ecaa2ca308 --- /dev/null +++ b/depends/patches/qt/fix-minimum-macos.patch @@ -0,0 +1,18 @@ +Ensure that Qt handles the minimum macOS version properly + +This patch can be dropped for LLVM Clang 17+, after commit +https://github.com/llvm/llvm-project/commit/c8e2dd8c6f490b68e41fe663b44535a8a21dfeab + + +--- a/qtbase/src/corelib/global/qsystemdetection.h ++++ b/qtbase/src/corelib/global/qsystemdetection.h +@@ -220,6 +220,9 @@ + # include <Availability.h> + # include <AvailabilityMacros.h> + # ++# undef __MAC_OS_X_VERSION_MIN_REQUIRED ++# define __MAC_OS_X_VERSION_MIN_REQUIRED MAC_OS_X_VERSION_MIN_REQUIRED ++# + # ifdef Q_OS_MACOS + # if !defined(__MAC_OS_X_VERSION_MIN_REQUIRED) || __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_6 + # undef __MAC_OS_X_VERSION_MIN_REQUIRED diff --git a/doc/reduce-traffic.md b/doc/reduce-traffic.md index 86943b1f72..8926a8361a 100644 --- a/doc/reduce-traffic.md +++ b/doc/reduce-traffic.md @@ -3,10 +3,10 @@ Reduce Traffic Some node operators need to deal with bandwidth caps imposed by their ISPs. -By default, Bitcoin Core allows up to 125 connections to different peers, 10 of -which are outbound. You can therefore, have at most 115 inbound connections. -Of the 10 outbound peers, there can be 8 full-relay connections and 2 -block-relay-only ones. +By default, Bitcoin Core allows up to 125 connections to different peers, 11 of +which are outbound. You can therefore, have at most 114 inbound connections. +Of the 11 outbound peers, there can be 8 full-relay connections, 2 +block-relay-only ones and occasionally 1 short-lived feeler or an extra block-relay-only connection. The default settings can result in relatively significant traffic consumption. @@ -28,7 +28,7 @@ calculating the target. ## 2. Disable "listening" (`-listen=0`) -Disabling listening will result in fewer nodes connected (remember the maximum of 10 +Disabling listening will result in fewer nodes connected (remember the maximum of 11 outbound peers). Fewer nodes will result in less traffic usage as you are relaying blocks and transactions to fewer nodes. diff --git a/doc/release-notes-empty-template.md b/doc/release-notes-empty-template.md index 887104548b..96e28c3763 100644 --- a/doc/release-notes-empty-template.md +++ b/doc/release-notes-empty-template.md @@ -36,9 +36,9 @@ Compatibility ============== Bitcoin Core is supported and extensively tested on operating systems -using the Linux kernel, macOS 11.0+, and Windows 7 and newer. Bitcoin +using the Linux Kernel 3.17+, macOS 11.0+, and Windows 7 and newer. Bitcoin Core should also work on most other Unix-like systems but is not as -frequently tested on them. It is not recommended to use Bitcoin Core on +frequently tested on them. It is not recommended to use Bitcoin Core on unsupported systems. Notable changes diff --git a/doc/release-notes/release-notes-26.0.md b/doc/release-notes/release-notes-26.0.md new file mode 100644 index 0000000000..b7c7c35f65 --- /dev/null +++ b/doc/release-notes/release-notes-26.0.md @@ -0,0 +1,357 @@ +26.0 Release Notes +================== + +Bitcoin Core version 26.0 is now available from: + + <https://bitcoincore.org/bin/bitcoin-core-26.0/> + +This release includes new features, various bug fixes and performance +improvements, as well as updated translations. + +Please report bugs using the issue tracker at GitHub: + + <https://github.com/bitcoin/bitcoin/issues> + +To receive security and update notifications, please subscribe to: + + <https://bitcoincore.org/en/list/announcements/join/> + +How to Upgrade +============== + +If you are running an older version, shut it down. Wait until it has completely +shut down (which might take a few minutes in some cases), then run the +installer (on Windows) or just copy over `/Applications/Bitcoin-Qt` (on macOS) +or `bitcoind`/`bitcoin-qt` (on Linux). + +Upgrading directly from a version of Bitcoin Core that has reached its EOL is +possible, but it might take some time if the data directory needs to be migrated. Old +wallet versions of Bitcoin Core are generally supported. + +Compatibility +============== + +Bitcoin Core is supported and extensively tested on operating systems +using the Linux kernel, macOS 11.0+, and Windows 7 and newer. Bitcoin +Core should also work on most other Unix-like systems but is not as +frequently tested on them. It is not recommended to use Bitcoin Core on +unsupported systems. + +Notable changes +=============== + +P2P and network changes +----------------------- + +- Experimental support for the v2 transport protocol defined in + [BIP324](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki) was added. + It is off by default, but when enabled using `-v2transport` it will be negotiated + on a per-connection basis with other peers that support it too. The existing + v1 transport protocol remains fully supported. + +- Nodes with multiple reachable networks will actively try to have at least one + outbound connection to each network. This improves individual resistance to + eclipse attacks and network level resistance to partition attacks. Users no + longer need to perform active measures to ensure being connected to multiple + enabled networks. (#27213) + +Pruning +------- + +- When using assumeutxo with `-prune`, the prune budget may be exceeded if it is set + lower than 1100MB (i.e. `MIN_DISK_SPACE_FOR_BLOCK_FILES * 2`). Prune budget is normally + split evenly across each chainstate, unless the resulting prune budget per chainstate + is beneath `MIN_DISK_SPACE_FOR_BLOCK_FILES` in which case that value will be used. (#27596) + +Updated RPCs +------------ + +- Setting `-rpcserialversion=0` is deprecated and will be removed in + a future release. It can currently still be used by also adding + the `-deprecatedrpc=serialversion` option. (#28448) + +- The `hash_serialized_2` value has been removed from `gettxoutsetinfo` since the value it + calculated contained a bug and did not take all data into account. It is superseded by + `hash_serialized_3` which provides the same functionality but serves the correctly calculated hash. (#28685) + +- New fields `transport_protocol_type` and `session_id` were added to the `getpeerinfo` RPC to indicate + whether the v2 transport protocol is in use, and if so, what the session id is. + +- A new argument `v2transport` was added to the `addnode` RPC to indicate whether a v2 transaction connection + is to be attempted with the peer. + +- [Miniscript](https://bitcoin.sipa.be/miniscript/) expressions can now be used in Taproot descriptors for all RPCs working with descriptors. (#27255) + +- `finalizepsbt` is now able to finalize a PSBT with inputs spending [Miniscript](https://bitcoin.sipa.be/miniscript/)-compatible Taproot leaves. (#27255) + +Changes to wallet related RPCs can be found in the Wallet section below. + +New RPCs +-------- + +- `loadtxoutset` has been added, which allows loading a UTXO snapshot of the format + generated by `dumptxoutset`. Once this snapshot is loaded, its contents will be + deserialized into a second chainstate data structure, which is then used to sync to + the network's tip. + + Meanwhile, the original chainstate will complete the initial block download process in + the background, eventually validating up to the block that the snapshot is based upon. + + The result is a usable bitcoind instance that is current with the network tip in a + matter of minutes rather than hours. UTXO snapshot are typically obtained via + third-party sources (HTTP, torrent, etc.) which is reasonable since their contents + are always checked by hash. + + You can find more information on this process in the `assumeutxo` design + document (<https://github.com/bitcoin/bitcoin/blob/master/doc/design/assumeutxo.md>). + + `getchainstates` has been added to aid in monitoring the assumeutxo sync process. + +- A new `getprioritisedtransactions` RPC has been added. It returns a map of all fee deltas created by the + user with prioritisetransaction, indexed by txid. The map also indicates whether each transaction is + present in the mempool. (#27501) + +- A new RPC, `submitpackage`, has been added. It can be used to submit a list of raw hex +transactions to the mempool to be evaluated as a package using consensus and mempool policy rules. +These policies include package CPFP, allowing a child with high fees to bump a parent below the +mempool minimum feerate (but not minimum relay feerate). (#27609) + + - Warning: successful submission does not mean the transactions will propagate throughout the + network, as package relay is not supported. + + - Not all features are available. The package is limited to a child with all of its + unconfirmed parents, and no parent may spend the output of another parent. Also, package + RBF is not supported. Refer to doc/policy/packages.md for more details on package policies + and limitations. + + - This RPC is experimental. Its interface may change. + +- A new RPC `getaddrmaninfo` has been added to view the distribution of addresses in the new and tried table of the + node's address manager across different networks(ipv4, ipv6, onion, i2p, cjdns). The RPC returns count of addresses + in new and tried table as well as their sum for all networks. (#27511) + +- A new `importmempool` RPC has been added. It loads a valid `mempool.dat` file and attempts to + add its contents to the mempool. This can be useful to import mempool data from another node + without having to modify the datadir contents and without having to restart the node. (#27460) + - Warning: Importing untrusted files is dangerous, especially if metadata from the file is taken over. + - If you want to apply fee deltas, it is recommended to use the `getprioritisedtransactions` and + `prioritisetransaction` RPCs instead of the `apply_fee_delta_priority` option to avoid + double-prioritising any already-prioritised transactions in the mempool. + +Updated settings +---------------- + +- `bitcoind` and `bitcoin-qt` will now raise an error on startup + if a datadir that is being used contains a bitcoin.conf file that + will be ignored, which can happen when a datadir= line is used in + a bitcoin.conf file. The error message is just a diagnostic intended + to prevent accidental misconfiguration, and it can be disabled to + restore the previous behavior of using the datadir while ignoring + the bitcoin.conf contained in it. (#27302) + +- Passing an invalid `-debug`, `-debugexclude`, or `-loglevel` logging configuration + option now raises an error, rather than logging an easily missed warning. (#27632) + +Changes to GUI or wallet related settings can be found in the GUI or Wallet section below. + +New settings +------------ + +Tools and Utilities +------------------- + +- A new `bitcoinconsensus_verify_script_with_spent_outputs` function is available in libconsensus which optionally accepts the spent outputs of the transaction being verified. +- A new `bitcoinconsensus_SCRIPT_FLAGS_VERIFY_TAPROOT` flag is available in libconsensus that will verify scripts with the Taproot spending rules. + +Wallet +------ + +- Wallet loading has changed in this release. Wallets with some corrupted records that could be + previously loaded (with warnings) may no longer load. For example, wallets with corrupted + address book entries may no longer load. If this happens, it is recommended + load the wallet in a previous version of Bitcoin Core and import the data into a new wallet. + Please also report an issue to help improve the software and make wallet loading more robust + in these cases. (#24914) + +- The `createwallet` RPC will no longer create legacy (BDB) wallets when + setting `descriptors=false` without also providing the + `-deprecatedrpc=create_bdb` option. This is because the legacy wallet is + being deprecated in a future release. (#28597) + +- The `gettransaction`, `listtransactions`, `listsinceblock` RPCs now return + the `abandoned` field for all transactions. Previously, the "abandoned" field + was only returned for sent transactions. (#25158) + +- The `listdescriptors`, `decodepsbt` and similar RPC methods now show `h` rather than apostrophe (`'`) to indicate + hardened derivation. This does not apply when using the `private` parameter, which + matches the marker used when descriptor was generated or imported. Newly created + wallets use `h`. This change makes it easier to handle descriptor strings manually. + E.g. the `importdescriptors` RPC call is easiest to use `h` as the marker: `'["desc": ".../0h/..."]'`. + With this change `listdescriptors` will use `h`, so you can copy-paste the result, + without having to add escape characters or switch `'` to 'h' manually. + Note that this changes the descriptor checksum. + For legacy wallets the `hdkeypath` field in `getaddressinfo` is unchanged, + nor is the serialization format of wallet dumps. (#26076) + +- The `getbalances` RPC now returns a `lastprocessedblock` JSON object which contains the wallet's last processed block + hash and height at the time the balances were calculated. This result shouldn't be cached because importing new keys could invalidate it. (#26094) + +- The `gettransaction` RPC now returns a `lastprocessedblock` JSON object which contains the wallet's last processed block + hash and height at the time the transaction information was generated. (#26094) + +- The `getwalletinfo` RPC now returns a `lastprocessedblock` JSON object which contains the wallet's last processed block + hash and height at the time the wallet information was generated. (#26094) + +- Coin selection and transaction building now accounts for unconfirmed low-feerate ancestor transactions. When it is necessary to spend unconfirmed outputs, the wallet will add fees to ensure that the new transaction with its ancestors will achieve a mining score equal to the feerate requested by the user. (#26152) + +- For RPC methods which accept `options` parameters ((`importmulti`, `listunspent`, + `fundrawtransaction`, `bumpfee`, `send`, `sendall`, `walletcreatefundedpsbt`, + `simulaterawtransaction`), it is now possible to pass the options as named + parameters without the need for a nested object. (#26485) + +This means it is possible make calls like: + +```sh +src/bitcoin-cli -named bumpfee txid fee_rate=100 +``` + +instead of + +```sh +src/bitcoin-cli -named bumpfee txid options='{"fee_rate": 100}' +``` + +- The `deprecatedrpc=walletwarningfield` configuration option has been removed. + The `createwallet`, `loadwallet`, `restorewallet` and `unloadwallet` RPCs no + longer return the "warning" string field. The same information is provided + through the "warnings" field added in v25.0, which returns a JSON array of + strings. The "warning" string field was deprecated also in v25.0. (#27757) + +- The `signrawtransactionwithkey`, `signrawtransactionwithwallet`, + `walletprocesspsbt` and `descriptorprocesspsbt` calls now return the more + specific RPC_INVALID_PARAMETER error instead of RPC_MISC_ERROR if their + sighashtype argument is malformed. (#28113) + +- RPC `walletprocesspsbt`, and `descriptorprocesspsbt` return + object now includes field `hex` (if the transaction + is complete) containing the serialized transaction + suitable for RPC `sendrawtransaction`. (#28414) + +- It's now possible to use [Miniscript](https://bitcoin.sipa.be/miniscript/) inside Taproot leaves for descriptor wallets. (#27255) + +Descriptors +----------- + +- The usage of hybrid public keys in output descriptors has been removed. Hybrid + public keys are an exotic public key encoding not supported by output descriptors + (as specified in BIP380 and documented in doc/descriptors.md). Bitcoin Core would + previously incorrectly accept descriptors containing such hybrid keys. (#28587) + +GUI changes +----------- + +- The transaction list in the GUI no longer provides a special category for "payment to yourself". Now transactions that have both inputs and outputs that affect the wallet are displayed on separate lines for spending and receiving. (gui#119) + +- A new menu option allows migrating a legacy wallet based on keys and implied output script types stored in BerkeleyDB (BDB) to a modern wallet that uses descriptors stored in SQLite. (gui#738) + +- The PSBT operations dialog marks outputs paying your own wallet with "own address". (gui#740) + +- The ability to create legacy wallets is being removed. (gui#764) + +Contrib +------- + +- Bash completion files have been renamed from `bitcoin*.bash-completion` to + `bitcoin*.bash`. This means completions can be automatically loaded on demand + based on invoked commands' names when they are put into the completion + directory (found with `pkg-config --variable=completionsdir + bash-completion`) without requiring renaming. (#28507) + +Low-level changes +================= + +Tests +----- + +- Non-standard transactions are now disabled by default on testnet + for relay and mempool acceptance. The previous behaviour can be + re-enabled by setting `-acceptnonstdtxn=1`. (#28354) + +Credits +======= + +Thanks to everyone who directly contributed to this release: + +- 0xb10c +- Amiti Uttarwar +- Andrew Chow +- Andrew Toth +- Anthony Towns +- Antoine Poinsot +- Antoine Riard +- Ari +- Aurèle Oulès +- Ayush Singh +- Ben Woosley +- Brandon Odiwuor +- Brotcrunsher +- brunoerg +- Bufo +- Carl Dong +- Casey Carter +- Cory Fields +- David Álvarez Rosa +- dergoegge +- dhruv +- dimitaracev +- Erik Arvstedt +- Erik McKelvey +- Fabian Jahr +- furszy +- glozow +- Greg Sanders +- Harris +- Hennadii Stepanov +- Hernan Marino +- ishaanam +- ismaelsadeeq +- Jake Rawsthorne +- James O'Beirne +- John Moffett +- Jon Atack +- josibake +- kevkevin +- Kiminuo +- Larry Ruane +- Luke Dashjr +- MarcoFalke +- Marnix +- Martin Leitner-Ankerl +- Martin Zumsande +- Matthew Zipkin +- Michael Ford +- Michael Tidwell +- mruddy +- Murch +- ns-xvrn +- pablomartin4btc +- Pieter Wuille +- Reese Russell +- Rhythm Garg +- Ryan Ofsky +- Sebastian Falbesoner +- Sjors Provoost +- stickies-v +- stratospher +- Suhas Daftuar +- TheCharlatan +- Tim Neubauer +- Tim Ruffing +- Vasil Dimov +- virtu +- vuittont60 +- willcl-ark +- Yusuf Sahin HAMZA + +As well as to everyone that helped with translations on +[Transifex](https://www.transifex.com/bitcoin/bitcoin/). diff --git a/doc/release-process.md b/doc/release-process.md index c70b0194ab..95ef08a26f 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -74,7 +74,9 @@ Release Process #### Before final release - Merge the release notes from [the wiki](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/) into the branch. -- Ensure the "Needs release note" label is removed from all relevant pull requests and issues. +- Ensure the "Needs release note" label is removed from all relevant pull + requests and issues: + https://github.com/bitcoin/bitcoin/issues?q=label%3A%22Needs+release+note%22 #### Tagging a release (candidate) diff --git a/src/.clang-format b/src/.clang-format index 791b3b8f9f..2e5d5c6449 100644 --- a/src/.clang-format +++ b/src/.clang-format @@ -43,5 +43,5 @@ SpacesInAngles: false SpacesInContainerLiterals: true SpacesInCStyleCastParentheses: false SpacesInParentheses: false -Standard: c++17 +Standard: c++20 UseTab: Never diff --git a/src/Makefile.am b/src/Makefile.am index 27f947d7a5..d25b27dd3e 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -304,7 +304,6 @@ BITCOIN_CORE_H = \ util/fees.h \ util/fs.h \ util/fs_helpers.h \ - util/getuniquepath.h \ util/golombrice.h \ util/hash_type.h \ util/hasher.h \ @@ -741,7 +740,6 @@ libbitcoin_util_a_SOURCES = \ util/fees.cpp \ util/fs.cpp \ util/fs_helpers.cpp \ - util/getuniquepath.cpp \ util/hasher.cpp \ util/sock.cpp \ util/syserror.cpp \ @@ -954,7 +952,6 @@ libbitcoinkernel_la_SOURCES = \ node/chainstate.cpp \ node/utxo_snapshot.cpp \ policy/feerate.cpp \ - policy/fees.cpp \ policy/packages.cpp \ policy/policy.cpp \ policy/rbf.cpp \ @@ -985,7 +982,6 @@ libbitcoinkernel_la_SOURCES = \ util/exception.cpp \ util/fs.cpp \ util/fs_helpers.cpp \ - util/getuniquepath.cpp \ util/hasher.cpp \ util/moneystr.cpp \ util/rbf.cpp \ diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index 28b779a5a8..217f123b16 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -84,6 +84,7 @@ endif if ENABLE_WALLET bench_bench_bitcoin_SOURCES += bench/coin_selection.cpp bench_bench_bitcoin_SOURCES += bench/wallet_balance.cpp +bench_bench_bitcoin_SOURCES += bench/wallet_create.cpp bench_bench_bitcoin_SOURCES += bench/wallet_loading.cpp bench_bench_bitcoin_SOURCES += bench/wallet_create_tx.cpp bench_bench_bitcoin_LDADD += $(BDB_LIBS) $(SQLITE_LIBS) diff --git a/src/addrman.cpp b/src/addrman.cpp index 5a11526471..a8206de6ee 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -802,7 +802,7 @@ int AddrManImpl::GetEntry(bool use_tried, size_t bucket, size_t position) const return -1; } -std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network) const +std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const { AssertLockHeld(cs); @@ -832,7 +832,7 @@ std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct if (network != std::nullopt && ai.GetNetClass() != network) continue; // Filter for quality - if (ai.IsTerrible(now)) continue; + if (ai.IsTerrible(now) && filtered) continue; addresses.push_back(ai); } @@ -1216,11 +1216,11 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, std::optiona return addrRet; } -std::vector<CAddress> AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const +std::vector<CAddress> AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const { LOCK(cs); Check(); - auto addresses = GetAddr_(max_addresses, max_pct, network); + auto addresses = GetAddr_(max_addresses, max_pct, network, filtered); Check(); return addresses; } @@ -1319,9 +1319,9 @@ std::pair<CAddress, NodeSeconds> AddrMan::Select(bool new_only, std::optional<Ne return m_impl->Select(new_only, network); } -std::vector<CAddress> AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const +std::vector<CAddress> AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const { - return m_impl->GetAddr(max_addresses, max_pct, network); + return m_impl->GetAddr(max_addresses, max_pct, network, filtered); } std::vector<std::pair<AddrInfo, AddressPosition>> AddrMan::GetEntries(bool use_tried) const diff --git a/src/addrman.h b/src/addrman.h index 67dc7604a4..ef9c766eff 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -164,10 +164,11 @@ public: * @param[in] max_addresses Maximum number of addresses to return (0 = all). * @param[in] max_pct Maximum percentage of addresses to return (0 = all). * @param[in] network Select only addresses of this network (nullopt = all). + * @param[in] filtered Select only addresses that are considered good quality (false = all). * * @return A vector of randomly selected addresses from vRandom. */ - std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const; + std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const; /** * Returns an information-location pair for all addresses in the selected addrman table. diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 512f085a21..867c894d01 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -129,7 +129,7 @@ public: std::pair<CAddress, NodeSeconds> Select(bool new_only, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(!cs); - std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const + std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const EXCLUSIVE_LOCKS_REQUIRED(!cs); std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries(bool from_tried) const @@ -261,7 +261,7 @@ private: * */ int GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs); - std::vector<CAddress> GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::vector<CAddress> GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries_(bool from_tried) const EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/arith_uint256.cpp b/src/arith_uint256.cpp index 3776cfb6de..0d5b3d5b0e 100644 --- a/src/arith_uint256.cpp +++ b/src/arith_uint256.cpp @@ -8,14 +8,7 @@ #include <uint256.h> #include <crypto/common.h> - -template <unsigned int BITS> -base_uint<BITS>::base_uint(const std::string& str) -{ - static_assert(BITS/32 > 0 && BITS%32 == 0, "Template parameter BITS must be a positive multiple of 32."); - - SetHex(str); -} +#include <cassert> template <unsigned int BITS> base_uint<BITS>& base_uint<BITS>::operator<<=(unsigned int shift) @@ -154,22 +147,6 @@ std::string base_uint<BITS>::GetHex() const } template <unsigned int BITS> -void base_uint<BITS>::SetHex(const char* psz) -{ - base_blob<BITS> b; - b.SetHex(psz); - for (int x = 0; x < this->WIDTH; ++x) { - this->pn[x] = ReadLE32(b.begin() + x*4); - } -} - -template <unsigned int BITS> -void base_uint<BITS>::SetHex(const std::string& str) -{ - SetHex(str.c_str()); -} - -template <unsigned int BITS> std::string base_uint<BITS>::ToString() const { return GetHex(); diff --git a/src/arith_uint256.h b/src/arith_uint256.h index c710fe9471..ba36cebbdc 100644 --- a/src/arith_uint256.h +++ b/src/arith_uint256.h @@ -6,10 +6,10 @@ #ifndef BITCOIN_ARITH_UINT256_H #define BITCOIN_ARITH_UINT256_H +#include <cstdint> #include <cstring> #include <limits> #include <stdexcept> -#include <stdint.h> #include <string> class uint256; @@ -56,8 +56,6 @@ public: pn[i] = 0; } - explicit base_uint(const std::string& str); - base_uint operator~() const { base_uint ret; @@ -219,8 +217,6 @@ public: friend inline bool operator!=(const base_uint& a, uint64_t b) { return !a.EqualTo(b); } std::string GetHex() const; - void SetHex(const char* psz); - void SetHex(const std::string& str); std::string ToString() const; unsigned int size() const @@ -247,7 +243,6 @@ public: arith_uint256() {} arith_uint256(const base_uint<256>& b) : base_uint<256>(b) {} arith_uint256(uint64_t b) : base_uint<256>(b) {} - explicit arith_uint256(const std::string& str) : base_uint<256>(str) {} /** * The "compact" format is a representation of a whole diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp index 2416d40798..713853e8c5 100644 --- a/src/bench/rpc_blockchain.cpp +++ b/src/bench/rpc_blockchain.cpp @@ -41,7 +41,7 @@ static void BlockToJsonVerbose(benchmark::Bench& bench) { TestBlockAndIndex data; bench.run([&] { - auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, &data.blockindex, &data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT); + auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, data.blockindex, data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT); ankerl::nanobench::doNotOptimizeAway(univalue); }); } @@ -51,7 +51,7 @@ BENCHMARK(BlockToJsonVerbose, benchmark::PriorityLevel::HIGH); static void BlockToJsonVerboseWrite(benchmark::Bench& bench) { TestBlockAndIndex data; - auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, &data.blockindex, &data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT); + auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, data.blockindex, data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT); bench.run([&] { auto str = univalue.write(); ankerl::nanobench::doNotOptimizeAway(str); diff --git a/src/bench/wallet_create.cpp b/src/bench/wallet_create.cpp new file mode 100644 index 0000000000..ba3c25d25e --- /dev/null +++ b/src/bench/wallet_create.cpp @@ -0,0 +1,55 @@ +// Copyright (c) 2023-present 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 <bench/bench.h> +#include <node/context.h> +#include <random.h> +#include <test/util/setup_common.h> +#include <wallet/context.h> +#include <wallet/wallet.h> + +namespace wallet { +static void WalletCreate(benchmark::Bench& bench, bool encrypted) +{ + auto test_setup = MakeNoLogFileContext<TestingSetup>(); + FastRandomContext random; + + WalletContext context; + context.args = &test_setup->m_args; + context.chain = test_setup->m_node.chain.get(); + + DatabaseOptions options; + options.require_format = DatabaseFormat::SQLITE; + options.require_create = true; + options.create_flags = WALLET_FLAG_DESCRIPTORS; + + if (encrypted) { + options.create_passphrase = random.rand256().ToString(); + } + + DatabaseStatus status; + bilingual_str error_string; + std::vector<bilingual_str> warnings; + + fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet_%d", random.rand32()).c_str(); + bench.run([&] { + auto wallet = CreateWallet(context, wallet_path.u8string(), /*load_on_start=*/std::nullopt, options, status, error_string, warnings); + assert(status == DatabaseStatus::SUCCESS); + assert(wallet != nullptr); + + // Cleanup + wallet.reset(); + fs::remove_all(wallet_path); + }); +} + +static void WalletCreatePlain(benchmark::Bench& bench) { WalletCreate(bench, /*encrypted=*/false); } +static void WalletCreateEncrypted(benchmark::Bench& bench) { WalletCreate(bench, /*encrypted=*/true); } + +#ifdef USE_SQLITE +BENCHMARK(WalletCreatePlain, benchmark::PriorityLevel::LOW); +BENCHMARK(WalletCreateEncrypted, benchmark::PriorityLevel::LOW); +#endif + +} // namespace wallet diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp index 632918c0ca..c0ca8f983d 100644 --- a/src/bench/wallet_create_tx.cpp +++ b/src/bench/wallet_create_tx.cpp @@ -16,7 +16,6 @@ using wallet::CWallet; using wallet::CreateMockableWalletDatabase; -using wallet::DBErrors; using wallet::WALLET_FLAG_DESCRIPTORS; struct TipBlock @@ -90,7 +89,6 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type LOCK(wallet.cs_wallet); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); wallet.SetupDescriptorScriptPubKeyMans(); - if (wallet.LoadWallet() != DBErrors::LOAD_OK) assert(false); } // Generate destinations @@ -146,7 +144,6 @@ static void AvailableCoins(benchmark::Bench& bench, const std::vector<OutputType LOCK(wallet.cs_wallet); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); wallet.SetupDescriptorScriptPubKeyMans(); - if (wallet.LoadWallet() != DBErrors::LOAD_OK) assert(false); } // Generate destinations diff --git a/src/init.cpp b/src/init.cpp index 57a94dc7c6..d1a0ca1c3b 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -155,6 +155,11 @@ static const char* DEFAULT_ASMAP_FILENAME="ip_asn.map"; * The PID file facilities. */ static const char* BITCOIN_PID_FILENAME = "bitcoind.pid"; +/** + * True if this process has created a PID file. + * Used to determine whether we should remove the PID file on shutdown. + */ +static bool g_generated_pid{false}; static fs::path GetPidFile(const ArgsManager& args) { @@ -170,12 +175,24 @@ static fs::path GetPidFile(const ArgsManager& args) #else tfm::format(file, "%d\n", getpid()); #endif + g_generated_pid = true; return true; } else { return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno))); } } +static void RemovePidFile(const ArgsManager& args) +{ + if (!g_generated_pid) return; + const auto pid_path{GetPidFile(args)}; + if (std::error_code error; !fs::remove(pid_path, error)) { + std::string msg{error ? error.message() : "File does not exist"}; + LogPrintf("Unable to remove PID file (%s): %s\n", fs::PathToString(pid_path), msg); + } +} + + ////////////////////////////////////////////////////////////////////////////// // // Shutdown @@ -284,8 +301,12 @@ void Shutdown(NodeContext& node) DumpMempool(*node.mempool, MempoolPath(*node.args)); } - // Drop transactions we were still watching, and record fee estimations. - if (node.fee_estimator) node.fee_estimator->Flush(); + // Drop transactions we were still watching, record fee estimations and Unregister + // fee estimator from validation interface. + if (node.fee_estimator) { + node.fee_estimator->Flush(); + UnregisterValidationInterface(node.fee_estimator.get()); + } // FlushStateToDisk generates a ChainStateFlushed callback, which we should avoid missing if (node.chainman) { @@ -348,13 +369,7 @@ void Shutdown(NodeContext& node) node.scheduler.reset(); node.kernel.reset(); - try { - if (!fs::remove(GetPidFile(*node.args))) { - LogPrintf("%s: Unable to remove PID file: File does not exist\n", __func__); - } - } catch (const fs::filesystem_error& e) { - LogPrintf("%s: Unable to remove PID file: %s\n", __func__, fsbridge::get_filesystem_error_message(e)); - } + RemovePidFile(*node.args); LogPrintf("%s: done\n", __func__); } @@ -1032,13 +1047,14 @@ static bool LockDataDirectory(bool probeOnly) { // Make sure only a single Bitcoin process is using the data directory. const fs::path& datadir = gArgs.GetDataDirNet(); - if (!DirIsWritable(datadir)) { + switch (util::LockDirectory(datadir, ".lock", probeOnly)) { + case util::LockResult::ErrorWrite: return InitError(strprintf(_("Cannot write to data directory '%s'; check permissions."), fs::PathToString(datadir))); - } - if (!LockDirectory(datadir, ".lock", probeOnly)) { + case util::LockResult::ErrorLock: return InitError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running."), fs::PathToString(datadir), PACKAGE_NAME)); - } - return true; + case util::LockResult::Success: return true; + } // no default case, so the compiler can warn about missing cases + assert(false); } bool AppInitSanityChecks(const kernel::Context& kernel) @@ -1239,6 +1255,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // Flush estimates to disk periodically CBlockPolicyEstimator* fee_estimator = node.fee_estimator.get(); node.scheduler->scheduleEvery([fee_estimator] { fee_estimator->FlushFeeEstimates(); }, FEE_FLUSH_INTERVAL); + RegisterValidationInterface(fee_estimator); } // Check port numbers @@ -1452,7 +1469,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) assert(!node.chainman); CTxMemPool::Options mempool_opts{ - .estimator = node.fee_estimator.get(), .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0, }; auto result{ApplyArgsManOptions(args, chainparams, mempool_opts)}; diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index b5c0499012..bd39c9cc5f 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -190,4 +190,63 @@ public: using CTxMemPoolEntryRef = CTxMemPoolEntry::CTxMemPoolEntryRef; +struct TransactionInfo { + const CTransactionRef m_tx; + /* The fee the transaction paid */ + const CAmount m_fee; + /** + * The virtual transaction size. + * + * This is a policy field which considers the sigop cost of the + * transaction as well as its weight, and reinterprets it as bytes. + * + * It is the primary metric by which the mining algorithm selects + * transactions. + */ + const int64_t m_virtual_transaction_size; + /* The block height the transaction entered the mempool */ + const unsigned int txHeight; + + TransactionInfo(const CTransactionRef& tx, const CAmount& fee, const int64_t vsize, const unsigned int height) + : m_tx{tx}, + m_fee{fee}, + m_virtual_transaction_size{vsize}, + txHeight{height} {} +}; + +struct RemovedMempoolTransactionInfo { + TransactionInfo info; + explicit RemovedMempoolTransactionInfo(const CTxMemPoolEntry& entry) + : info{entry.GetSharedTx(), entry.GetFee(), entry.GetTxSize(), entry.GetHeight()} {} +}; + +struct NewMempoolTransactionInfo { + TransactionInfo info; + /* + * This boolean indicates whether the transaction was added + * without enforcing mempool fee limits. + */ + const bool m_from_disconnected_block; + /* This boolean indicates whether the transaction is part of a package. */ + const bool m_submitted_in_package; + /* + * This boolean indicates whether the blockchain is up to date when the + * transaction is added to the mempool. + */ + const bool m_chainstate_is_current; + /* Indicates whether the transaction has unconfirmed parents. */ + const bool m_has_no_mempool_parents; + + explicit NewMempoolTransactionInfo(const CTransactionRef& tx, const CAmount& fee, + const int64_t vsize, const unsigned int height, + const bool from_disconnected_block, const bool submitted_in_package, + const bool chainstate_is_current, + const bool has_no_mempool_parents) + : info{tx, fee, vsize, height}, + m_from_disconnected_block{from_disconnected_block}, + m_submitted_in_package{submitted_in_package}, + m_chainstate_is_current{chainstate_is_current}, + m_has_no_mempool_parents{has_no_mempool_parents} {} +}; + #endif // BITCOIN_KERNEL_MEMPOOL_ENTRY_H diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index d09fd2ba35..753aebd455 100644 --- a/src/kernel/mempool_options.h +++ b/src/kernel/mempool_options.h @@ -13,8 +13,6 @@ #include <cstdint> #include <optional> -class CBlockPolicyEstimator; - /** Default for -maxmempool, maximum megabytes of mempool memory usage */ static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300}; /** Default for -maxmempool when blocksonly is set */ @@ -37,8 +35,6 @@ namespace kernel { * Most of the time, this struct should be referenced as CTxMemPool::Options. */ struct MemPoolOptions { - /* Used to estimate appropriate transaction fees. */ - CBlockPolicyEstimator* estimator{nullptr}; /* The ratio used to determine how often sanity checks will run. */ int check_ratio{0}; int64_t max_size_bytes{DEFAULT_MAX_MEMPOOL_SIZE_MB * 1'000'000}; diff --git a/src/net.cpp b/src/net.cpp index dc76fdfb44..102d81579f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3278,6 +3278,12 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) // Dump network addresses scheduler.scheduleEvery([this] { DumpAddresses(); }, DUMP_PEERS_INTERVAL); + // Run the ASMap Health check once and then schedule it to run every 24h. + if (m_netgroupman.UsingASMap()) { + ASMapHealthCheck(); + scheduler.scheduleEvery([this] { ASMapHealthCheck(); }, ASMAP_HEALTH_CHECK_INTERVAL); + } + return true; } @@ -3383,9 +3389,9 @@ CConnman::~CConnman() Stop(); } -std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network) const +std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const { - std::vector<CAddress> addresses = addrman.GetAddr(max_addresses, max_pct, network); + std::vector<CAddress> addresses = addrman.GetAddr(max_addresses, max_pct, network, filtered); if (m_banman) { addresses.erase(std::remove_if(addresses.begin(), addresses.end(), [this](const CAddress& addr){return m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr);}), @@ -3840,6 +3846,19 @@ void CConnman::PerformReconnections() } } +void CConnman::ASMapHealthCheck() +{ + const std::vector<CAddress> v4_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV4, /*filtered=*/ false)}; + const std::vector<CAddress> v6_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV6, /*filtered=*/ false)}; + std::vector<CNetAddr> clearnet_addrs; + clearnet_addrs.reserve(v4_addrs.size() + v6_addrs.size()); + std::transform(v4_addrs.begin(), v4_addrs.end(), std::back_inserter(clearnet_addrs), + [](const CAddress& addr) { return static_cast<CNetAddr>(addr); }); + std::transform(v6_addrs.begin(), v6_addrs.end(), std::back_inserter(clearnet_addrs), + [](const CAddress& addr) { return static_cast<CNetAddr>(addr); }); + m_netgroupman.ASMapHealthCheck(clearnet_addrs); +} + // Dump binary message to file, with timestamp. static void CaptureMessageToFile(const CAddress& addr, const std::string& msg_type, @@ -88,6 +88,8 @@ static const bool DEFAULT_BLOCKSONLY = false; static const int64_t DEFAULT_PEER_CONNECT_TIMEOUT = 60; /** Number of file descriptors required for message capture **/ static const int NUM_FDS_MESSAGE_CAPTURE = 1; +/** Interval for ASMap Health Check **/ +static constexpr std::chrono::hours ASMAP_HEALTH_CHECK_INTERVAL{24}; static constexpr bool DEFAULT_FORCEDNSSEED{false}; static constexpr bool DEFAULT_DNSSEED{true}; @@ -1112,6 +1114,7 @@ public: void SetNetworkActive(bool active); void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant&& grant_outbound, const char* strDest, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); bool CheckIncomingNonce(uint64_t nonce); + void ASMapHealthCheck(); // alias for thread safety annotations only, not defined RecursiveMutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex); @@ -1146,8 +1149,9 @@ public: * @param[in] max_addresses Maximum number of addresses to return (0 = all). * @param[in] max_pct Maximum percentage of addresses to return (0 = all). * @param[in] network Select only addresses of this network (nullopt = all). + * @param[in] filtered Select only addresses that are considered high quality (false = all). */ - std::vector<CAddress> GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network) const; + std::vector<CAddress> GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const; /** * Cache is used to minimize topology leaks, so it should * be used for all non-trusted calls, for example, p2p. diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1067341495..df54a62f28 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4298,7 +4298,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, m_txrequest.ForgetTxHash(tx.GetWitnessHash()); // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) - m_orphanage.LimitOrphans(m_opts.max_orphan_txs); + m_orphanage.LimitOrphans(m_opts.max_orphan_txs, m_rng); } else { LogPrint(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s (wtxid=%s)\n", tx.GetHash().ToString(), diff --git a/src/netgroup.cpp b/src/netgroup.cpp index a03927b152..0ae229b3f3 100644 --- a/src/netgroup.cpp +++ b/src/netgroup.cpp @@ -5,6 +5,7 @@ #include <netgroup.h> #include <hash.h> +#include <logging.h> #include <util/asmap.h> uint256 NetGroupManager::GetAsmapChecksum() const @@ -109,3 +110,23 @@ uint32_t NetGroupManager::GetMappedAS(const CNetAddr& address) const uint32_t mapped_as = Interpret(m_asmap, ip_bits); return mapped_as; } + +void NetGroupManager::ASMapHealthCheck(const std::vector<CNetAddr>& clearnet_addrs) const { + std::set<uint32_t> clearnet_asns{}; + int unmapped_count{0}; + + for (const auto& addr : clearnet_addrs) { + uint32_t asn = GetMappedAS(addr); + if (asn == 0) { + ++unmapped_count; + continue; + } + clearnet_asns.insert(asn); + } + + LogPrintf("ASMap Health Check: %i clearnet peers are mapped to %i ASNs with %i peers being unmapped\n", clearnet_addrs.size(), clearnet_asns.size(), unmapped_count); +} + +bool NetGroupManager::UsingASMap() const { + return m_asmap.size() > 0; +} diff --git a/src/netgroup.h b/src/netgroup.h index 2dd63ec66b..5aa6ef7742 100644 --- a/src/netgroup.h +++ b/src/netgroup.h @@ -41,6 +41,16 @@ public: */ uint32_t GetMappedAS(const CNetAddr& address) const; + /** + * Analyze and log current health of ASMap based buckets. + */ + void ASMapHealthCheck(const std::vector<CNetAddr>& clearnet_addrs) const; + + /** + * Indicates whether ASMap is being used for clearnet bucketing. + */ + bool UsingASMap() const; + private: /** Compressed IP->ASN mapping, loaded from a file when a node starts. * diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index ebc08d7567..e6164c2e59 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -582,10 +582,10 @@ const CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data) return nullptr; } -bool BlockManager::IsBlockPruned(const CBlockIndex* pblockindex) +bool BlockManager::IsBlockPruned(const CBlockIndex& block) { AssertLockHeld(::cs_main); - return (m_have_pruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0); + return m_have_pruned && !(block.nStatus & BLOCK_HAVE_DATA) && (block.nTx > 0); } const CBlockIndex* BlockManager::GetFirstStoredBlock(const CBlockIndex& upper_block, const CBlockIndex* lower_block) diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index d540406ae5..ce514cc645 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -344,7 +344,7 @@ public: bool m_have_pruned = false; //! Check whether the block associated with this index entry is pruned or not. - bool IsBlockPruned(const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + bool IsBlockPruned(const CBlockIndex& block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); //! Create or update a prune lock identified by its name void UpdatePruneLock(const std::string& name, const PruneLockInfo& lock_info) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index f4ecfeb9d5..b5691f0e57 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -428,9 +428,9 @@ public: explicit NotificationsProxy(std::shared_ptr<Chain::Notifications> notifications) : m_notifications(std::move(notifications)) {} virtual ~NotificationsProxy() = default; - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) override + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) override { - m_notifications->transactionAddedToMempool(tx); + m_notifications->transactionAddedToMempool(tx.info.m_tx); } void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override { diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 654c4cf0ce..74c688060d 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -515,15 +515,10 @@ void TxConfirmStats::removeTx(unsigned int entryHeight, unsigned int nBestSeenHe } } -// This function is called from CTxMemPool::removeUnchecked to ensure -// txs removed from the mempool for any reason are no longer -// tracked. Txs that were part of a block have already been removed in -// processBlockTx to ensure they are never double tracked, but it is -// of no harm to try to remove them again. -bool CBlockPolicyEstimator::removeTx(uint256 hash, bool inBlock) +bool CBlockPolicyEstimator::removeTx(uint256 hash) { LOCK(m_cs_fee_estimator); - return _removeTx(hash, inBlock); + return _removeTx(hash, /*inBlock=*/false); } bool CBlockPolicyEstimator::_removeTx(const uint256& hash, bool inBlock) @@ -579,11 +574,26 @@ CBlockPolicyEstimator::CBlockPolicyEstimator(const fs::path& estimation_filepath CBlockPolicyEstimator::~CBlockPolicyEstimator() = default; -void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate) +void CBlockPolicyEstimator::TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /*unused*/) +{ + processTransaction(tx); +} + +void CBlockPolicyEstimator::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason /*unused*/, uint64_t /*unused*/) +{ + removeTx(tx->GetHash()); +} + +void CBlockPolicyEstimator::MempoolTransactionsRemovedForBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block, unsigned int nBlockHeight) +{ + processBlock(txs_removed_for_block, nBlockHeight); +} + +void CBlockPolicyEstimator::processTransaction(const NewMempoolTransactionInfo& tx) { LOCK(m_cs_fee_estimator); - unsigned int txHeight = entry.GetHeight(); - uint256 hash = entry.GetTx().GetHash(); + const unsigned int txHeight = tx.info.txHeight; + const auto& hash = tx.info.m_tx->GetHash(); if (mapMemPoolTxs.count(hash)) { LogPrint(BCLog::ESTIMATEFEE, "Blockpolicy error mempool tx %s already being tracked\n", hash.ToString()); @@ -597,31 +607,37 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo // It will be synced next time a block is processed. return; } + // This transaction should only count for fee estimation if: + // - it's not being re-added during a reorg which bypasses typical mempool fee limits + // - the node is not behind + // - the transaction is not dependent on any other transactions in the mempool + // - it's not part of a package. + const bool validForFeeEstimation = !tx.m_from_disconnected_block && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents; // Only want to be updating estimates when our blockchain is synced, // otherwise we'll miscalculate how many blocks its taking to get included. - if (!validFeeEstimate) { + if (!validForFeeEstimation) { untrackedTxs++; return; } trackedTxs++; // Feerates are stored and reported as BTC-per-kb: - CFeeRate feeRate(entry.GetFee(), entry.GetTxSize()); + const CFeeRate feeRate(tx.info.m_fee, tx.info.m_virtual_transaction_size); mapMemPoolTxs[hash].blockHeight = txHeight; - unsigned int bucketIndex = feeStats->NewTx(txHeight, (double)feeRate.GetFeePerK()); + unsigned int bucketIndex = feeStats->NewTx(txHeight, static_cast<double>(feeRate.GetFeePerK())); mapMemPoolTxs[hash].bucketIndex = bucketIndex; - unsigned int bucketIndex2 = shortStats->NewTx(txHeight, (double)feeRate.GetFeePerK()); + unsigned int bucketIndex2 = shortStats->NewTx(txHeight, static_cast<double>(feeRate.GetFeePerK())); assert(bucketIndex == bucketIndex2); - unsigned int bucketIndex3 = longStats->NewTx(txHeight, (double)feeRate.GetFeePerK()); + unsigned int bucketIndex3 = longStats->NewTx(txHeight, static_cast<double>(feeRate.GetFeePerK())); assert(bucketIndex == bucketIndex3); } -bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry) +bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const RemovedMempoolTransactionInfo& tx) { AssertLockHeld(m_cs_fee_estimator); - if (!_removeTx(entry->GetTx().GetHash(), true)) { + if (!_removeTx(tx.info.m_tx->GetHash(), true)) { // This transaction wasn't being tracked for fee estimation return false; } @@ -629,7 +645,7 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM // How many blocks did it take for miners to include this transaction? // blocksToConfirm is 1-based, so a transaction included in the earliest // possible block has confirmation count of 1 - int blocksToConfirm = nBlockHeight - entry->GetHeight(); + int blocksToConfirm = nBlockHeight - tx.info.txHeight; if (blocksToConfirm <= 0) { // This can't happen because we don't process transactions from a block with a height // lower than our greatest seen height @@ -638,16 +654,16 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM } // Feerates are stored and reported as BTC-per-kb: - CFeeRate feeRate(entry->GetFee(), entry->GetTxSize()); + CFeeRate feeRate(tx.info.m_fee, tx.info.m_virtual_transaction_size); - feeStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK()); - shortStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK()); - longStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK()); + feeStats->Record(blocksToConfirm, static_cast<double>(feeRate.GetFeePerK())); + shortStats->Record(blocksToConfirm, static_cast<double>(feeRate.GetFeePerK())); + longStats->Record(blocksToConfirm, static_cast<double>(feeRate.GetFeePerK())); return true; } -void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, - std::vector<const CTxMemPoolEntry*>& entries) +void CBlockPolicyEstimator::processBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block, + unsigned int nBlockHeight) { LOCK(m_cs_fee_estimator); if (nBlockHeight <= nBestSeenHeight) { @@ -676,8 +692,8 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, unsigned int countedTxs = 0; // Update averages with data points from current block - for (const auto& entry : entries) { - if (processBlockTx(nBlockHeight, entry)) + for (const auto& tx : txs_removed_for_block) { + if (processBlockTx(nBlockHeight, tx)) countedTxs++; } @@ -688,7 +704,7 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, LogPrint(BCLog::ESTIMATEFEE, "Blockpolicy estimates updated by %u of %u block txs, since last block %u of %u tracked, mempool map size %u, max target %u from %s\n", - countedTxs, entries.size(), trackedTxs, trackedTxs + untrackedTxs, mapMemPoolTxs.size(), + countedTxs, txs_removed_for_block.size(), trackedTxs, trackedTxs + untrackedTxs, mapMemPoolTxs.size(), MaxUsableEstimate(), HistoricalBlockSpan() > BlockSpan() ? "historical" : "current"); trackedTxs = 0; diff --git a/src/policy/fees.h b/src/policy/fees.h index 69bda195be..f34f66d3f0 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -12,6 +12,7 @@ #include <threadsafety.h> #include <uint256.h> #include <util/fs.h> +#include <validationinterface.h> #include <array> #include <chrono> @@ -35,8 +36,9 @@ static constexpr std::chrono::hours MAX_FILE_AGE{60}; static constexpr bool DEFAULT_ACCEPT_STALE_FEE_ESTIMATES{false}; class AutoFile; -class CTxMemPoolEntry; class TxConfirmStats; +struct RemovedMempoolTransactionInfo; +struct NewMempoolTransactionInfo; /* Identifier for each of the 3 different TxConfirmStats which will track * history over different time horizons. */ @@ -143,7 +145,7 @@ struct FeeCalculation * a certain number of blocks. Every time a block is added to the best chain, this class records * stats on the transactions included in that block */ -class CBlockPolicyEstimator +class CBlockPolicyEstimator : public CValidationInterface { private: /** Track confirm delays up to 12 blocks for short horizon */ @@ -198,19 +200,19 @@ private: public: /** Create new BlockPolicyEstimator and initialize stats tracking classes with default values */ CBlockPolicyEstimator(const fs::path& estimation_filepath, const bool read_stale_estimates); - ~CBlockPolicyEstimator(); + virtual ~CBlockPolicyEstimator(); /** Process all the transactions that have been included in a block */ - void processBlock(unsigned int nBlockHeight, - std::vector<const CTxMemPoolEntry*>& entries) + void processBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block, + unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Process a transaction accepted to the mempool*/ - void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate) + void processTransaction(const NewMempoolTransactionInfo& tx) EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); - /** Remove a transaction from the mempool tracking stats*/ - bool removeTx(uint256 hash, bool inBlock) + /** Remove a transaction from the mempool tracking stats for non BLOCK removal reasons*/ + bool removeTx(uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** DEPRECATED. Return a feerate estimate */ @@ -260,6 +262,15 @@ public: /** Calculates the age of the file, since last modified */ std::chrono::hours GetFeeEstimatorFileAge(); +protected: + /** Overridden from CValidationInterface. */ + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /*unused*/) override + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); + void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason /*unused*/, uint64_t /*unused*/) override + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); + void MempoolTransactionsRemovedForBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block, unsigned int nBlockHeight) override + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); + private: mutable Mutex m_cs_fee_estimator; @@ -290,7 +301,7 @@ private: std::map<double, unsigned int> bucketMap GUARDED_BY(m_cs_fee_estimator); // Map of bucket upper-bound to index into all vectors by bucket /** Process a transaction confirmed in a block*/ - bool processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry) EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator); + bool processBlockTx(unsigned int nBlockHeight, const RemovedMempoolTransactionInfo& tx) EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator); /** Helper for estimateSmartFee */ double estimateCombinedFee(unsigned int confTarget, double successThreshold, bool checkShorterHorizon, EstimationResult *result) const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator); diff --git a/src/rest.cpp b/src/rest.cpp index bb54e780b2..e47b52fb53 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -264,7 +264,7 @@ static bool rest_headers(const std::any& context, case RESTResponseFormat::JSON: { UniValue jsonHeaders(UniValue::VARR); for (const CBlockIndex *pindex : headers) { - jsonHeaders.push_back(blockheaderToJSON(tip, pindex)); + jsonHeaders.push_back(blockheaderToJSON(*tip, *pindex)); } std::string strJSON = jsonHeaders.write() + "\n"; req->WriteHeader("Content-Type", "application/json"); @@ -304,10 +304,9 @@ static bool rest_block(const std::any& context, if (!pblockindex) { return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found"); } - - if (chainman.m_blockman.IsBlockPruned(pblockindex)) + if (chainman.m_blockman.IsBlockPruned(*pblockindex)) { return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)"); - + } } if (!chainman.m_blockman.ReadBlockFromDisk(block, *pblockindex)) { @@ -334,7 +333,7 @@ static bool rest_block(const std::any& context, } case RESTResponseFormat::JSON: { - UniValue objBlock = blockToJSON(chainman.m_blockman, block, tip, pblockindex, tx_verbosity); + UniValue objBlock = blockToJSON(chainman.m_blockman, block, *tip, *pblockindex, tx_verbosity); std::string strJSON = objBlock.write() + "\n"; req->WriteHeader("Content-Type", "application/json"); req->WriteReply(HTTP_OK, strJSON); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index be6a8c47fd..6f20b1031c 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -73,13 +73,11 @@ static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange); /* Calculate the difficulty for a given block index. */ -double GetDifficulty(const CBlockIndex* blockindex) +double GetDifficulty(const CBlockIndex& blockindex) { - CHECK_NONFATAL(blockindex); - - int nShift = (blockindex->nBits >> 24) & 0xff; + int nShift = (blockindex.nBits >> 24) & 0xff; double dDiff = - (double)0x0000ffff / (double)(blockindex->nBits & 0x00ffffff); + (double)0x0000ffff / (double)(blockindex.nBits & 0x00ffffff); while (nShift < 29) { @@ -95,14 +93,14 @@ double GetDifficulty(const CBlockIndex* blockindex) return dDiff; } -static int ComputeNextBlockAndDepth(const CBlockIndex* tip, const CBlockIndex* blockindex, const CBlockIndex*& next) +static int ComputeNextBlockAndDepth(const CBlockIndex& tip, const CBlockIndex& blockindex, const CBlockIndex*& next) { - next = tip->GetAncestor(blockindex->nHeight + 1); - if (next && next->pprev == blockindex) { - return tip->nHeight - blockindex->nHeight + 1; + next = tip.GetAncestor(blockindex.nHeight + 1); + if (next && next->pprev == &blockindex) { + return tip.nHeight - blockindex.nHeight + 1; } next = nullptr; - return blockindex == tip ? 1 : -1; + return &blockindex == &tip ? 1 : -1; } static const CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateManager& chainman) @@ -133,36 +131,36 @@ static const CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateMan } } -UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex) +UniValue blockheaderToJSON(const CBlockIndex& tip, const CBlockIndex& blockindex) { // Serialize passed information without accessing chain state of the active chain! AssertLockNotHeld(cs_main); // For performance reasons UniValue result(UniValue::VOBJ); - result.pushKV("hash", blockindex->GetBlockHash().GetHex()); + result.pushKV("hash", blockindex.GetBlockHash().GetHex()); const CBlockIndex* pnext; int confirmations = ComputeNextBlockAndDepth(tip, blockindex, pnext); result.pushKV("confirmations", confirmations); - result.pushKV("height", blockindex->nHeight); - result.pushKV("version", blockindex->nVersion); - result.pushKV("versionHex", strprintf("%08x", blockindex->nVersion)); - result.pushKV("merkleroot", blockindex->hashMerkleRoot.GetHex()); - result.pushKV("time", (int64_t)blockindex->nTime); - result.pushKV("mediantime", (int64_t)blockindex->GetMedianTimePast()); - result.pushKV("nonce", (uint64_t)blockindex->nNonce); - result.pushKV("bits", strprintf("%08x", blockindex->nBits)); + result.pushKV("height", blockindex.nHeight); + result.pushKV("version", blockindex.nVersion); + result.pushKV("versionHex", strprintf("%08x", blockindex.nVersion)); + result.pushKV("merkleroot", blockindex.hashMerkleRoot.GetHex()); + result.pushKV("time", blockindex.nTime); + result.pushKV("mediantime", blockindex.GetMedianTimePast()); + result.pushKV("nonce", blockindex.nNonce); + result.pushKV("bits", strprintf("%08x", blockindex.nBits)); result.pushKV("difficulty", GetDifficulty(blockindex)); - result.pushKV("chainwork", blockindex->nChainWork.GetHex()); - result.pushKV("nTx", (uint64_t)blockindex->nTx); + result.pushKV("chainwork", blockindex.nChainWork.GetHex()); + result.pushKV("nTx", blockindex.nTx); - if (blockindex->pprev) - result.pushKV("previousblockhash", blockindex->pprev->GetBlockHash().GetHex()); + if (blockindex.pprev) + result.pushKV("previousblockhash", blockindex.pprev->GetBlockHash().GetHex()); if (pnext) result.pushKV("nextblockhash", pnext->GetBlockHash().GetHex()); return result; } -UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, TxVerbosity verbosity) +UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIndex& tip, const CBlockIndex& blockindex, TxVerbosity verbosity) { UniValue result = blockheaderToJSON(tip, blockindex); @@ -182,7 +180,7 @@ UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIn case TxVerbosity::SHOW_DETAILS_AND_PREVOUT: CBlockUndo blockUndo; const bool is_not_pruned{WITH_LOCK(::cs_main, return !blockman.IsBlockPruned(blockindex))}; - const bool have_undo{is_not_pruned && blockman.UndoReadFromDisk(blockUndo, *blockindex)}; + const bool have_undo{is_not_pruned && blockman.UndoReadFromDisk(blockUndo, blockindex)}; for (size_t i = 0; i < block.vtx.size(); ++i) { const CTransactionRef& tx = block.vtx.at(i); @@ -418,7 +416,7 @@ static RPCHelpMan getdifficulty() { ChainstateManager& chainman = EnsureAnyChainman(request.context); LOCK(cs_main); - return GetDifficulty(chainman.ActiveChain().Tip()); + return GetDifficulty(*CHECK_NONFATAL(chainman.ActiveChain().Tip())); }, }; } @@ -571,22 +569,22 @@ static RPCHelpMan getblockheader() return strHex; } - return blockheaderToJSON(tip, pblockindex); + return blockheaderToJSON(*tip, *pblockindex); }, }; } -static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex* pblockindex) +static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex& blockindex) { CBlock block; { LOCK(cs_main); - if (blockman.IsBlockPruned(pblockindex)) { + if (blockman.IsBlockPruned(blockindex)) { throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)"); } } - if (!blockman.ReadBlockFromDisk(block, *pblockindex)) { + if (!blockman.ReadBlockFromDisk(block, blockindex)) { // Block not found on disk. This could be because we have the block // header in our index but not yet have the block or did not accept the // block. Or if the block was pruned right after we released the lock above. @@ -596,21 +594,21 @@ static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex* pblocki return block; } -static CBlockUndo GetUndoChecked(BlockManager& blockman, const CBlockIndex* pblockindex) +static CBlockUndo GetUndoChecked(BlockManager& blockman, const CBlockIndex& blockindex) { CBlockUndo blockUndo; // The Genesis block does not have undo data - if (pblockindex->nHeight == 0) return blockUndo; + if (blockindex.nHeight == 0) return blockUndo; { LOCK(cs_main); - if (blockman.IsBlockPruned(pblockindex)) { + if (blockman.IsBlockPruned(blockindex)) { throw JSONRPCError(RPC_MISC_ERROR, "Undo data not available (pruned data)"); } } - if (!blockman.UndoReadFromDisk(blockUndo, *pblockindex)) { + if (!blockman.UndoReadFromDisk(blockUndo, blockindex)) { throw JSONRPCError(RPC_MISC_ERROR, "Can't read undo data from disk"); } @@ -736,7 +734,7 @@ static RPCHelpMan getblock() } } - const CBlock block{GetBlockChecked(chainman.m_blockman, pblockindex)}; + const CBlock block{GetBlockChecked(chainman.m_blockman, *pblockindex)}; if (verbosity <= 0) { @@ -755,7 +753,7 @@ static RPCHelpMan getblock() tx_verbosity = TxVerbosity::SHOW_DETAILS_AND_PREVOUT; } - return blockToJSON(chainman.m_blockman, block, tip, pblockindex, tx_verbosity); + return blockToJSON(chainman.m_blockman, block, *tip, *pblockindex, tx_verbosity); }, }; } @@ -1257,7 +1255,7 @@ RPCHelpMan getblockchaininfo() obj.pushKV("blocks", height); obj.pushKV("headers", chainman.m_best_header ? chainman.m_best_header->nHeight : -1); obj.pushKV("bestblockhash", tip.GetBlockHash().GetHex()); - obj.pushKV("difficulty", GetDifficulty(&tip)); + obj.pushKV("difficulty", GetDifficulty(tip)); obj.pushKV("time", tip.GetBlockTime()); obj.pushKV("mediantime", tip.GetMedianTimePast()); obj.pushKV("verificationprogress", GuessVerificationProgress(chainman.GetParams().TxData(), &tip)); @@ -1815,8 +1813,8 @@ static RPCHelpMan getblockstats() } } - const CBlock& block = GetBlockChecked(chainman.m_blockman, &pindex); - const CBlockUndo& blockUndo = GetUndoChecked(chainman.m_blockman, &pindex); + const CBlock& block = GetBlockChecked(chainman.m_blockman, pindex); + const CBlockUndo& blockUndo = GetUndoChecked(chainman.m_blockman, pindex); const bool do_all = stats.size() == 0; // Calculate everything if nothing selected (default) const bool do_mediantxsize = do_all || stats.count("mediantxsize") != 0; @@ -2275,8 +2273,8 @@ public: static bool CheckBlockFilterMatches(BlockManager& blockman, const CBlockIndex& blockindex, const GCSFilter::ElementSet& needles) { - const CBlock block{GetBlockChecked(blockman, &blockindex)}; - const CBlockUndo block_undo{GetUndoChecked(blockman, &blockindex)}; + const CBlock block{GetBlockChecked(blockman, blockindex)}; + const CBlockUndo block_undo{GetUndoChecked(blockman, blockindex)}; // Check if any of the outputs match the scriptPubKey for (const auto& tx : block.vtx) { @@ -2845,7 +2843,7 @@ return RPCHelpMan{ data.pushKV("blocks", (int)chain.Height()); data.pushKV("bestblockhash", tip->GetBlockHash().GetHex()); - data.pushKV("difficulty", (double)GetDifficulty(tip)); + data.pushKV("difficulty", GetDifficulty(*tip)); data.pushKV("verificationprogress", GuessVerificationProgress(Params().TxData(), tip)); data.pushKV("coins_db_cache_bytes", cs.m_coinsdb_cache_size_bytes); data.pushKV("coins_tip_cache_bytes", cs.m_coinstip_cache_size_bytes); diff --git a/src/rpc/blockchain.h b/src/rpc/blockchain.h index 0a86085db0..c2021c3608 100644 --- a/src/rpc/blockchain.h +++ b/src/rpc/blockchain.h @@ -32,16 +32,16 @@ static constexpr int NUM_GETBLOCKSTATS_PERCENTILES = 5; * @return A floating point number that is a multiple of the main net minimum * difficulty (4295032833 hashes). */ -double GetDifficulty(const CBlockIndex* blockindex); +double GetDifficulty(const CBlockIndex& blockindex); /** Callback for when block tip changed. */ void RPCNotifyBlockChange(const CBlockIndex*); /** Block description to JSON */ -UniValue blockToJSON(node::BlockManager& blockman, const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, TxVerbosity verbosity) LOCKS_EXCLUDED(cs_main); +UniValue blockToJSON(node::BlockManager& blockman, const CBlock& block, const CBlockIndex& tip, const CBlockIndex& blockindex, TxVerbosity verbosity) LOCKS_EXCLUDED(cs_main); /** Block header to JSON */ -UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex) LOCKS_EXCLUDED(cs_main); +UniValue blockheaderToJSON(const CBlockIndex& tip, const CBlockIndex& blockindex) LOCKS_EXCLUDED(cs_main); /** Used by getblockstats to get feerates at different percentiles by weight */ void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES], std::vector<std::pair<CAmount, int64_t>>& scores, int64_t total_weight); diff --git a/src/rpc/fees.cpp b/src/rpc/fees.cpp index 62396d4c58..57ba486ed9 100644 --- a/src/rpc/fees.cpp +++ b/src/rpc/fees.cpp @@ -14,6 +14,7 @@ #include <txmempool.h> #include <univalue.h> #include <util/fees.h> +#include <validationinterface.h> #include <algorithm> #include <array> @@ -67,6 +68,7 @@ static RPCHelpMan estimatesmartfee() const NodeContext& node = EnsureAnyNodeContext(request.context); const CTxMemPool& mempool = EnsureMemPool(node); + SyncWithValidationInterfaceQueue(); unsigned int max_target = fee_estimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); unsigned int conf_target = ParseConfirmTarget(request.params[0], max_target); bool conservative = true; @@ -155,6 +157,7 @@ static RPCHelpMan estimaterawfee() { CBlockPolicyEstimator& fee_estimator = EnsureAnyFeeEstimator(request.context); + SyncWithValidationInterfaceQueue(); unsigned int max_target = fee_estimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); unsigned int conf_target = ParseConfirmTarget(request.params[0], max_target); double threshold = 0.95; diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 6cbc96c5ec..04bd7fa928 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -818,7 +818,7 @@ static RPCHelpMan submitpackage() return RPCHelpMan{"submitpackage", "Submit a package of raw transactions (serialized, hex-encoded) to local node.\n" "The package must consist of a child with its parents, and none of the parents may depend on one another.\n" - "The package will be validated according to consensus and mempool policy rules. If all transactions pass, they will be accepted to mempool.\n" + "The package will be validated according to consensus and mempool policy rules. If any transaction passes, it will be accepted to mempool.\n" "This RPC is experimental and the interface may be unstable. Refer to doc/policy/packages.md for documentation on package policies.\n" "Warning: successful submission does not mean the transactions will propagate throughout the network.\n" , @@ -832,19 +832,21 @@ static RPCHelpMan submitpackage() RPCResult{ RPCResult::Type::OBJ, "", "", { + {RPCResult::Type::STR, "package_msg", "The transaction package result message. \"success\" indicates all transactions were accepted into or are already in the mempool."}, {RPCResult::Type::OBJ_DYN, "tx-results", "transaction results keyed by wtxid", { {RPCResult::Type::OBJ, "wtxid", "transaction wtxid", { {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"}, {RPCResult::Type::STR_HEX, "other-wtxid", /*optional=*/true, "The wtxid of a different transaction with the same txid but different witness found in the mempool. This means the submitted transaction was ignored."}, - {RPCResult::Type::NUM, "vsize", "Virtual transaction size as defined in BIP 141."}, - {RPCResult::Type::OBJ, "fees", "Transaction fees", { + {RPCResult::Type::NUM, "vsize", /*optional=*/true, "Sigops-adjusted virtual transaction size."}, + {RPCResult::Type::OBJ, "fees", /*optional=*/true, "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, "error", /*optional=*/true, "The transaction error string, if it was rejected by the mempool"}, }} }}, {RPCResult::Type::ARR, "replaced-transactions", /*optional=*/true, "List of txids of replaced transactions", @@ -884,57 +886,77 @@ static RPCHelpMan submitpackage() Chainstate& chainstate = EnsureChainman(node).ActiveChainstate(); const auto package_result = WITH_LOCK(::cs_main, return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/ false)); - // First catch any errors. + std::string package_msg = "success"; + + // First catch package-wide errors, continue if we can switch(package_result.m_state.GetResult()) { - case PackageValidationResult::PCKG_RESULT_UNSET: break; - case PackageValidationResult::PCKG_POLICY: + case PackageValidationResult::PCKG_RESULT_UNSET: { - throw JSONRPCTransactionError(TransactionError::INVALID_PACKAGE, - package_result.m_state.GetRejectReason()); + // Belt-and-suspenders check; everything should be successful here + CHECK_NONFATAL(package_result.m_tx_results.size() == txns.size()); + for (const auto& tx : txns) { + CHECK_NONFATAL(mempool.exists(GenTxid::Txid(tx->GetHash()))); + } + break; } case PackageValidationResult::PCKG_MEMPOOL_ERROR: { + // This only happens with internal bug; user should stop and report throw JSONRPCTransactionError(TransactionError::MEMPOOL_ERROR, package_result.m_state.GetRejectReason()); } + case PackageValidationResult::PCKG_POLICY: case PackageValidationResult::PCKG_TX: { - for (const auto& tx : txns) { - auto it = package_result.m_tx_results.find(tx->GetWitnessHash()); - if (it != package_result.m_tx_results.end() && it->second.m_state.IsInvalid()) { - throw JSONRPCTransactionError(TransactionError::MEMPOOL_REJECTED, - strprintf("%s failed: %s", tx->GetHash().ToString(), it->second.m_state.GetRejectReason())); - } - } - // If a PCKG_TX error was returned, there must have been an invalid transaction. - NONFATAL_UNREACHABLE(); + // Package-wide error we want to return, but we also want to return individual responses + package_msg = package_result.m_state.GetRejectReason(); + CHECK_NONFATAL(package_result.m_tx_results.size() == txns.size() || + package_result.m_tx_results.empty()); + break; } } + size_t num_broadcast{0}; for (const auto& tx : txns) { + // We don't want to re-submit the txn for validation in BroadcastTransaction + if (!mempool.exists(GenTxid::Txid(tx->GetHash()))) { + continue; + } + + // We do not expect an error here; we are only broadcasting things already/still in mempool std::string err_string; const auto err = BroadcastTransaction(node, tx, err_string, /*max_tx_fee=*/0, /*relay=*/true, /*wait_callback=*/true); if (err != TransactionError::OK) { throw JSONRPCTransactionError(err, - strprintf("transaction broadcast failed: %s (all transactions were submitted, %d transactions were broadcast successfully)", + strprintf("transaction broadcast failed: %s (%d transactions were broadcast successfully)", err_string, num_broadcast)); } num_broadcast++; } + UniValue rpc_result{UniValue::VOBJ}; + rpc_result.pushKV("package_msg", package_msg); UniValue tx_result_map{UniValue::VOBJ}; std::set<uint256> replaced_txids; for (const auto& tx : txns) { - auto it = package_result.m_tx_results.find(tx->GetWitnessHash()); - CHECK_NONFATAL(it != package_result.m_tx_results.end()); UniValue result_inner{UniValue::VOBJ}; result_inner.pushKV("txid", tx->GetHash().GetHex()); + auto it = package_result.m_tx_results.find(tx->GetWitnessHash()); + if (it == package_result.m_tx_results.end()) { + // No results, report error and continue + result_inner.pushKV("error", "unevaluated"); + continue; + } const auto& tx_result = it->second; - if (it->second.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS) { + switch(it->second.m_result_type) { + case MempoolAcceptResult::ResultType::DIFFERENT_WITNESS: result_inner.pushKV("other-wtxid", it->second.m_other_wtxid.value().GetHex()); - } - if (it->second.m_result_type == MempoolAcceptResult::ResultType::VALID || - it->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY) { + break; + case MempoolAcceptResult::ResultType::INVALID: + result_inner.pushKV("error", it->second.m_state.ToString()); + break; + case MempoolAcceptResult::ResultType::VALID: + case MempoolAcceptResult::ResultType::MEMPOOL_ENTRY: 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())); @@ -955,6 +977,7 @@ static RPCHelpMan submitpackage() replaced_txids.insert(ptx->GetHash()); } } + break; } tx_result_map.pushKV(tx->GetWitnessHash().GetHex(), result_inner); } diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 3d80656507..b2332c5fdc 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -441,7 +441,7 @@ static RPCHelpMan getmininginfo() obj.pushKV("blocks", active_chain.Height()); if (BlockAssembler::m_last_block_weight) obj.pushKV("currentblockweight", *BlockAssembler::m_last_block_weight); if (BlockAssembler::m_last_block_num_txs) obj.pushKV("currentblocktx", *BlockAssembler::m_last_block_num_txs); - obj.pushKV("difficulty", (double)GetDifficulty(active_chain.Tip())); + obj.pushKV("difficulty", GetDifficulty(*CHECK_NONFATAL(active_chain.Tip()))); obj.pushKV("networkhashps", getnetworkhashps().HandleRequest(request)); obj.pushKV("pooledtx", (uint64_t)mempool.size()); obj.pushKV("chain", chainman.GetParams().GetChainTypeString()); diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 2df2999523..b1e6d677f8 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -394,7 +394,7 @@ static RPCHelpMan getrawtransaction() // If request is verbosity >= 1 but no blockhash was given, then look up the blockindex if (request.params[2].isNull()) { LOCK(cs_main); - blockindex = chainman.m_blockman.LookupBlockIndex(hash_block); + blockindex = chainman.m_blockman.LookupBlockIndex(hash_block); // May be nullptr for mempool transactions } if (verbosity == 1) { TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate()); @@ -403,10 +403,8 @@ static RPCHelpMan getrawtransaction() CBlockUndo blockUndo; CBlock block; - const bool is_block_pruned{WITH_LOCK(cs_main, return chainman.m_blockman.IsBlockPruned(blockindex))}; - if (tx->IsCoinBase() || - !blockindex || is_block_pruned || + if (tx->IsCoinBase() || !blockindex || WITH_LOCK(::cs_main, return chainman.m_blockman.IsBlockPruned(*blockindex)) || !(chainman.m_blockman.UndoReadFromDisk(blockUndo, *blockindex) && chainman.m_blockman.ReadBlockFromDisk(block, *blockindex))) { TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate()); return result; diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp index 4c67da8b70..b7acd62ee3 100644 --- a/src/rpc/request.cpp +++ b/src/rpc/request.cpp @@ -80,6 +80,8 @@ static fs::path GetAuthCookieFile(bool temp=false) return AbsPathForConfigVal(gArgs, arg); } +static bool g_generated_cookie = false; + bool GenerateAuthCookie(std::string *cookie_out) { const size_t COOKIE_SIZE = 32; @@ -105,6 +107,7 @@ bool GenerateAuthCookie(std::string *cookie_out) LogPrintf("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath)); return false; } + g_generated_cookie = true; LogPrintf("Generated RPC authentication cookie %s\n", fs::PathToString(filepath)); if (cookie_out) @@ -131,7 +134,10 @@ bool GetAuthCookie(std::string *cookie_out) void DeleteAuthCookie() { try { - fs::remove(GetAuthCookieFile()); + if (g_generated_cookie) { + // Delete the cookie file if it was generated by this process + fs::remove(GetAuthCookieFile()); + } } catch (const fs::filesystem_error& e) { LogPrintf("%s: Unable to remove random auth cookie file: %s\n", __func__, fsbridge::get_filesystem_error_message(e)); } diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index bfefc3ff97..5bd4f271cd 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -429,6 +429,24 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr) BOOST_CHECK_EQUAL(addrman->Size(), 2006U); } +BOOST_AUTO_TEST_CASE(getaddr_unfiltered) +{ + auto addrman = std::make_unique<AddrMan>(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node)); + + // Set time on this addr so isTerrible = false + CAddress addr1 = CAddress(ResolveService("250.250.2.1", 8333), NODE_NONE); + addr1.nTime = Now<NodeSeconds>(); + // Not setting time so this addr should be isTerrible = true + CAddress addr2 = CAddress(ResolveService("250.251.2.2", 9999), NODE_NONE); + + CNetAddr source = ResolveIP("250.1.2.1"); + BOOST_CHECK(addrman->Add({addr1, addr2}, source)); + + // Filtered GetAddr should only return addr1 + BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt).size(), 1U); + // Unfiltered GetAddr should return addr1 and addr2 + BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt, /*filtered=*/false).size(), 2U); +} BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket_legacy) { diff --git a/src/test/arith_uint256_tests.cpp b/src/test/arith_uint256_tests.cpp index 6a37b7d83b..10028c7c93 100644 --- a/src/test/arith_uint256_tests.cpp +++ b/src/test/arith_uint256_tests.cpp @@ -22,6 +22,7 @@ static inline arith_uint256 arith_uint256V(const std::vector<unsigned char>& vch { return UintToArith256(uint256(vch)); } +static inline arith_uint256 arith_uint256S(const std::string& str) { return UintToArith256(uint256S(str)); } const unsigned char R1Array[] = "\x9c\x52\x4a\xdb\xcf\x56\x11\x12\x2b\x29\x12\x5e\x5d\x35\xd2\xd2" @@ -95,25 +96,25 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality BOOST_CHECK(ZeroL == (OneL << 256)); // String Constructor and Copy Constructor - BOOST_CHECK(arith_uint256("0x"+R1L.ToString()) == R1L); - BOOST_CHECK(arith_uint256("0x"+R2L.ToString()) == R2L); - BOOST_CHECK(arith_uint256("0x"+ZeroL.ToString()) == ZeroL); - BOOST_CHECK(arith_uint256("0x"+OneL.ToString()) == OneL); - BOOST_CHECK(arith_uint256("0x"+MaxL.ToString()) == MaxL); - BOOST_CHECK(arith_uint256(R1L.ToString()) == R1L); - BOOST_CHECK(arith_uint256(" 0x"+R1L.ToString()+" ") == R1L); - BOOST_CHECK(arith_uint256("") == ZeroL); - BOOST_CHECK(R1L == arith_uint256(R1ArrayHex)); + BOOST_CHECK(arith_uint256S("0x" + R1L.ToString()) == R1L); + BOOST_CHECK(arith_uint256S("0x" + R2L.ToString()) == R2L); + BOOST_CHECK(arith_uint256S("0x" + ZeroL.ToString()) == ZeroL); + BOOST_CHECK(arith_uint256S("0x" + OneL.ToString()) == OneL); + BOOST_CHECK(arith_uint256S("0x" + MaxL.ToString()) == MaxL); + BOOST_CHECK(arith_uint256S(R1L.ToString()) == R1L); + BOOST_CHECK(arith_uint256S(" 0x" + R1L.ToString() + " ") == R1L); + BOOST_CHECK(arith_uint256S("") == ZeroL); + BOOST_CHECK(R1L == arith_uint256S(R1ArrayHex)); BOOST_CHECK(arith_uint256(R1L) == R1L); BOOST_CHECK((arith_uint256(R1L^R2L)^R2L) == R1L); BOOST_CHECK(arith_uint256(ZeroL) == ZeroL); BOOST_CHECK(arith_uint256(OneL) == OneL); // uint64_t constructor - BOOST_CHECK( (R1L & arith_uint256("0xffffffffffffffff")) == arith_uint256(R1LLow64)); + BOOST_CHECK((R1L & arith_uint256S("0xffffffffffffffff")) == arith_uint256(R1LLow64)); BOOST_CHECK(ZeroL == arith_uint256(0)); BOOST_CHECK(OneL == arith_uint256(1)); - BOOST_CHECK(arith_uint256("0xffffffffffffffff") == arith_uint256(0xffffffffffffffffULL)); + BOOST_CHECK(arith_uint256S("0xffffffffffffffff") == arith_uint256(0xffffffffffffffffULL)); // Assignment (from base_uint) arith_uint256 tmpL = ~ZeroL; BOOST_CHECK(tmpL == ~ZeroL); @@ -282,7 +283,7 @@ BOOST_AUTO_TEST_CASE( comparison ) // <= >= < > BOOST_AUTO_TEST_CASE( plusMinus ) { arith_uint256 TmpL = 0; - BOOST_CHECK(R1L+R2L == arith_uint256(R1LplusR2L)); + BOOST_CHECK(R1L + R2L == arith_uint256S(R1LplusR2L)); TmpL += R1L; BOOST_CHECK(TmpL == R1L); TmpL += R2L; @@ -346,8 +347,8 @@ BOOST_AUTO_TEST_CASE( multiply ) BOOST_AUTO_TEST_CASE( divide ) { - arith_uint256 D1L("AD7133AC1977FA2B7"); - arith_uint256 D2L("ECD751716"); + arith_uint256 D1L{arith_uint256S("AD7133AC1977FA2B7")}; + arith_uint256 D2L{arith_uint256S("ECD751716")}; BOOST_CHECK((R1L / D1L).ToString() == "00000000000000000b8ac01106981635d9ed112290f8895545a7654dde28fb3a"); BOOST_CHECK((R1L / D2L).ToString() == "000000000873ce8efec5b67150bad3aa8c5fcb70e947586153bf2cec7c37c57a"); BOOST_CHECK(R1L / OneL == R1L); @@ -368,7 +369,7 @@ static bool almostEqual(double d1, double d2) return fabs(d1-d2) <= 4*fabs(d1)*std::numeric_limits<double>::epsilon(); } -BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHex size() GetLow64 GetSerializeSize, Serialize, Unserialize +BOOST_AUTO_TEST_CASE(methods) // GetHex operator= size() GetLow64 GetSerializeSize, Serialize, Unserialize { BOOST_CHECK(R1L.GetHex() == R1L.ToString()); BOOST_CHECK(R2L.GetHex() == R2L.ToString()); @@ -376,11 +377,14 @@ BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHex size() GetLow64 GetSerializeSiz BOOST_CHECK(MaxL.GetHex() == MaxL.ToString()); arith_uint256 TmpL(R1L); BOOST_CHECK(TmpL == R1L); - TmpL.SetHex(R2L.ToString()); BOOST_CHECK(TmpL == R2L); - TmpL.SetHex(ZeroL.ToString()); BOOST_CHECK(TmpL == 0); - TmpL.SetHex(HalfL.ToString()); BOOST_CHECK(TmpL == HalfL); + TmpL = R2L; + BOOST_CHECK(TmpL == R2L); + TmpL = ZeroL; + BOOST_CHECK(TmpL == 0); + TmpL = HalfL; + BOOST_CHECK(TmpL == HalfL); - TmpL.SetHex(R1L.ToString()); + TmpL = R1L; BOOST_CHECK(R1L.size() == 32); BOOST_CHECK(R2L.size() == 32); BOOST_CHECK(ZeroL.size() == 32); diff --git a/src/test/blockchain_tests.cpp b/src/test/blockchain_tests.cpp index b590467a43..be515a9eac 100644 --- a/src/test/blockchain_tests.cpp +++ b/src/test/blockchain_tests.cpp @@ -41,7 +41,7 @@ static void RejectDifficultyMismatch(double difficulty, double expected_difficul static void TestDifficulty(uint32_t nbits, double expected_difficulty) { CBlockIndex* block_index = CreateBlockIndexWithNbits(nbits); - double difficulty = GetDifficulty(block_index); + double difficulty = GetDifficulty(*block_index); delete block_index; RejectDifficultyMismatch(difficulty, expected_difficulty); diff --git a/src/test/fs_tests.cpp b/src/test/fs_tests.cpp index 7cfecb2b22..0d25428b33 100644 --- a/src/test/fs_tests.cpp +++ b/src/test/fs_tests.cpp @@ -5,7 +5,6 @@ #include <test/util/setup_common.h> #include <util/fs.h> #include <util/fs_helpers.h> -#include <util/getuniquepath.h> #include <boost/test/unit_test.hpp> @@ -101,29 +100,14 @@ BOOST_AUTO_TEST_CASE(fsbridge_fstream) BOOST_CHECK_EQUAL(tmpfile1, fsbridge::AbsPathJoin(tmpfile1, "")); BOOST_CHECK_EQUAL(tmpfile1, fsbridge::AbsPathJoin(tmpfile1, {})); } - { - fs::path p1 = GetUniquePath(tmpfolder); - fs::path p2 = GetUniquePath(tmpfolder); - fs::path p3 = GetUniquePath(tmpfolder); - - // Ensure that the parent path is always the same. - BOOST_CHECK_EQUAL(tmpfolder, p1.parent_path()); - BOOST_CHECK_EQUAL(tmpfolder, p2.parent_path()); - BOOST_CHECK_EQUAL(tmpfolder, p3.parent_path()); - - // Ensure that generated paths are actually different. - BOOST_CHECK(p1 != p2); - BOOST_CHECK(p2 != p3); - BOOST_CHECK(p1 != p3); - } } BOOST_AUTO_TEST_CASE(rename) { const fs::path tmpfolder{m_args.GetDataDirBase()}; - const fs::path path1{GetUniquePath(tmpfolder)}; - const fs::path path2{GetUniquePath(tmpfolder)}; + const fs::path path1{tmpfolder / "a"}; + const fs::path path2{tmpfolder / "b"}; const std::string path1_contents{"1111"}; const std::string path2_contents{"2222"}; @@ -158,13 +142,13 @@ BOOST_AUTO_TEST_CASE(create_directories) // Test fs::create_directories workaround. const fs::path tmpfolder{m_args.GetDataDirBase()}; - const fs::path dir{GetUniquePath(tmpfolder)}; + const fs::path dir{tmpfolder / "a"}; fs::create_directory(dir); BOOST_CHECK(fs::exists(dir)); BOOST_CHECK(fs::is_directory(dir)); BOOST_CHECK(!fs::create_directories(dir)); - const fs::path symlink{GetUniquePath(tmpfolder)}; + const fs::path symlink{tmpfolder / "b"}; fs::create_directory_symlink(dir, symlink); BOOST_CHECK(fs::exists(symlink)); BOOST_CHECK(fs::is_symlink(symlink)); diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 1b11ff6fdf..ece396aadf 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -286,7 +286,8 @@ FUZZ_TARGET(addrman, .init = initialize_addrman) (void)const_addr_man.GetAddr( /*max_addresses=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096), /*max_pct=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096), - network); + network, + /*filtered=*/fuzzed_data_provider.ConsumeBool()); (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network); std::optional<bool> in_new; if (fuzzed_data_provider.ConsumeBool()) { diff --git a/src/test/fuzz/bitdeque.cpp b/src/test/fuzz/bitdeque.cpp index 65f5cb3fd0..d5cc9cfd34 100644 --- a/src/test/fuzz/bitdeque.cpp +++ b/src/test/fuzz/bitdeque.cpp @@ -53,21 +53,11 @@ FUZZ_TARGET(bitdeque, .init = InitRandData) --initlen; } - LIMITED_WHILE(provider.remaining_bytes() > 0, 900) + const auto iter_limit{maxlen > 6000 ? 90U : 900U}; + LIMITED_WHILE(provider.remaining_bytes() > 0, iter_limit) { - { - assert(deq.size() == bitdeq.size()); - auto it = deq.begin(); - auto bitit = bitdeq.begin(); - auto itend = deq.end(); - while (it != itend) { - assert(*it == *bitit); - ++it; - ++bitit; - } - } - - CallOneOf(provider, + CallOneOf( + provider, [&] { // constructor() deq = std::deque<bool>{}; @@ -535,7 +525,17 @@ FUZZ_TARGET(bitdeque, .init = InitRandData) assert(it == deq.begin() + before); assert(bitit == bitdeq.begin() + before); } - } - ); + }); + } + { + assert(deq.size() == bitdeq.size()); + auto it = deq.begin(); + auto bitit = bitdeq.begin(); + auto itend = deq.end(); + while (it != itend) { + assert(*it == *bitit); + ++it; + ++bitit; + } } } diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 551e1c526d..24f91abd25 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -88,7 +88,8 @@ FUZZ_TARGET(connman, .init = initialize_connman) (void)connman.GetAddresses( /*max_addresses=*/fuzzed_data_provider.ConsumeIntegral<size_t>(), /*max_pct=*/fuzzed_data_provider.ConsumeIntegral<size_t>(), - /*network=*/std::nullopt); + /*network=*/std::nullopt, + /*filtered=*/fuzzed_data_provider.ConsumeBool()); }, [&] { (void)connman.GetAddresses( diff --git a/src/test/fuzz/fuzz.h b/src/test/fuzz/fuzz.h index 1f0fa5527a..ca74d53de7 100644 --- a/src/test/fuzz/fuzz.h +++ b/src/test/fuzz/fuzz.h @@ -33,11 +33,7 @@ struct FuzzTargetOptions { void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target, FuzzTargetOptions opts); -#if defined(__clang__) -#define FUZZ_TARGET(...) _Pragma("clang diagnostic push") _Pragma("clang diagnostic ignored \"-Wgnu-zero-variadic-macro-arguments\"") DETAIL_FUZZ(__VA_ARGS__) _Pragma("clang diagnostic pop") -#else #define FUZZ_TARGET(...) DETAIL_FUZZ(__VA_ARGS__) -#endif #define DETAIL_FUZZ(name, ...) \ void name##_fuzz_target(FuzzBufferType); \ diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 064930c5aa..5a08d0ff44 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -55,13 +55,13 @@ struct OutpointsUpdater final : public CValidationInterface { explicit OutpointsUpdater(std::set<COutPoint>& r) : m_mempool_outpoints{r} {} - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t /* mempool_sequence */) override + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /* mempool_sequence */) override { // for coins spent we always want to be able to rbf so they're not removed // outputs from this tx can now be spent - for (uint32_t index{0}; index < tx->vout.size(); ++index) { - m_mempool_outpoints.insert(COutPoint{tx->GetHash(), index}); + for (uint32_t index{0}; index < tx.info.m_tx->vout.size(); ++index) { + m_mempool_outpoints.insert(COutPoint{tx.info.m_tx->GetHash(), index}); } } @@ -85,10 +85,10 @@ struct TransactionsDelta final : public CValidationInterface { explicit TransactionsDelta(std::set<CTransactionRef>& a) : m_added{a} {} - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t /* mempool_sequence */) override + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /* mempool_sequence */) override { // Transactions may be entered and booted any number of times - m_added.insert(tx); + m_added.insert(tx.info.m_tx); } void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override @@ -121,7 +121,6 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte mempool_opts.expiry = std::chrono::hours{fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 999)}; nBytesPerSigOp = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(1, 999); - mempool_opts.estimator = nullptr; mempool_opts.check_ratio = 1; mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool(); diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index d421eed9ff..40a1fc80f0 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -44,9 +44,16 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) return; } const CTransaction tx{*mtx}; - block_policy_estimator.processTransaction(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx), fuzzed_data_provider.ConsumeBool()); + const CTxMemPoolEntry& entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, tx); + const auto tx_info = NewMempoolTransactionInfo(entry.GetSharedTx(), entry.GetFee(), + entry.GetTxSize(), entry.GetHeight(), + /* m_from_disconnected_block */ false, + /* m_submitted_in_package */ false, + /* m_chainstate_is_current */ true, + /* m_has_no_mempool_parents */ fuzzed_data_provider.ConsumeBool()); + block_policy_estimator.processTransaction(tx_info); if (fuzzed_data_provider.ConsumeBool()) { - (void)block_policy_estimator.removeTx(tx.GetHash(), /*inBlock=*/fuzzed_data_provider.ConsumeBool()); + (void)block_policy_estimator.removeTx(tx.GetHash()); } }, [&] { @@ -61,15 +68,15 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) const CTransaction tx{*mtx}; mempool_entries.emplace_back(CTxMemPoolEntry::ExplicitCopy, ConsumeTxMemPoolEntry(fuzzed_data_provider, tx)); } - std::vector<const CTxMemPoolEntry*> ptrs; - ptrs.reserve(mempool_entries.size()); + std::vector<RemovedMempoolTransactionInfo> txs; + txs.reserve(mempool_entries.size()); for (const CTxMemPoolEntry& mempool_entry : mempool_entries) { - ptrs.push_back(&mempool_entry); + txs.emplace_back(mempool_entry); } - block_policy_estimator.processBlock(fuzzed_data_provider.ConsumeIntegral<unsigned int>(), ptrs); + block_policy_estimator.processBlock(txs, fuzzed_data_provider.ConsumeIntegral<unsigned int>()); }, [&] { - (void)block_policy_estimator.removeTx(ConsumeUInt256(fuzzed_data_provider), /*inBlock=*/fuzzed_data_provider.ConsumeBool()); + (void)block_policy_estimator.removeTx(ConsumeUInt256(fuzzed_data_provider)); }, [&] { block_policy_estimator.FlushUnconfirmed(); diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index 6392f03d4e..acb03ac5fc 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -79,14 +79,23 @@ FUZZ_TARGET(process_message, .init = initialize_process_message) const auto mock_time = ConsumeTime(fuzzed_data_provider); SetMockTime(mock_time); + CSerializedNetMsg net_msg; + net_msg.m_type = random_message_type; // fuzzed_data_provider is fully consumed after this call, don't use it - DataStream random_bytes_data_stream{fuzzed_data_provider.ConsumeRemainingBytes<unsigned char>()}; - try { - g_setup->m_node.peerman->ProcessMessage(p2p_node, random_message_type, random_bytes_data_stream, - GetTime<std::chrono::microseconds>(), std::atomic<bool>{false}); - } catch (const std::ios_base::failure&) { + net_msg.data = fuzzed_data_provider.ConsumeRemainingBytes<unsigned char>(); + + connman.FlushSendBuffer(p2p_node); + (void)connman.ReceiveMsgFrom(p2p_node, std::move(net_msg)); + + bool more_work{true}; + while (more_work) { + p2p_node.fPauseSend = false; + try { + more_work = connman.ProcessMessagesOnce(p2p_node); + } catch (const std::ios_base::failure&) { + } + g_setup->m_node.peerman->SendMessages(&p2p_node); } - g_setup->m_node.peerman->SendMessages(&p2p_node); SyncWithValidationInterfaceQueue(); g_setup->m_node.connman->StopNodes(); } diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index b7e2e04b4b..3f722f60ee 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -78,13 +78,17 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages) connman.FlushSendBuffer(random_node); (void)connman.ReceiveMsgFrom(random_node, std::move(net_msg)); - random_node.fPauseSend = false; - try { - connman.ProcessMessagesOnce(random_node); - } catch (const std::ios_base::failure&) { + bool more_work{true}; + while (more_work) { // Ensure that every message is eventually processed in some way or another + random_node.fPauseSend = false; + + try { + more_work = connman.ProcessMessagesOnce(random_node); + } catch (const std::ios_base::failure&) { + } + g_setup->m_node.peerman->SendMessages(&random_node); } - g_setup->m_node.peerman->SendMessages(&random_node); } SyncWithValidationInterfaceQueue(); g_setup->m_node.connman->StopNodes(); diff --git a/src/test/fuzz/rpc.cpp b/src/test/fuzz/rpc.cpp index 2189dc0067..2325bf0941 100644 --- a/src/test/fuzz/rpc.cpp +++ b/src/test/fuzz/rpc.cpp @@ -380,9 +380,7 @@ FUZZ_TARGET(rpc, .init = initialize_rpc) rpc_testing_setup->CallRPC(rpc_command, arguments); } catch (const UniValue& json_rpc_error) { const std::string error_msg{json_rpc_error.find_value("message").get_str()}; - // Once c++20 is allowed, starts_with can be used. - // if (error_msg.starts_with("Internal bug detected")) { - if (0 == error_msg.rfind("Internal bug detected", 0)) { + if (error_msg.starts_with("Internal bug detected")) { // Only allow the intentional internal bug assert(error_msg.find("trigger_internal_bug") != std::string::npos); } diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index ffa0c1216e..4ad0956201 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -59,9 +59,9 @@ struct TransactionsDelta final : public CValidationInterface { explicit TransactionsDelta(std::set<CTransactionRef>& r, std::set<CTransactionRef>& a) : m_removed{r}, m_added{a} {} - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t /* mempool_sequence */) override + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /* mempool_sequence */) override { - Assert(m_added.insert(tx).second); + Assert(m_added.insert(tx.info.m_tx).second); } void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override @@ -123,7 +123,6 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)}; // ...override specific options for this specific fuzz suite - mempool_opts.estimator = nullptr; mempool_opts.check_ratio = 1; mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool(); diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index e9ceb299fe..5423ba8920 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -33,6 +33,7 @@ void initialize_orphanage() FUZZ_TARGET(txorphan, .init = initialize_orphanage) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + FastRandomContext limit_orphans_rng{/*fDeterministic=*/true}; SetMockTime(ConsumeTime(fuzzed_data_provider)); TxOrphanage orphanage; @@ -91,13 +92,13 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) { CTransactionRef ref = orphanage.GetTxToReconsider(peer_id); if (ref) { - bool have_tx = orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(ref->GetHash())); + bool have_tx = orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(ref->GetWitnessHash())); Assert(have_tx); } } }, [&] { - bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash())); + bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); // AddTx should return false if tx is too big or already have it // tx weight is unknown, we only check when tx is already in orphanage { @@ -105,7 +106,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) // have_tx == true -> add_tx == false Assert(!have_tx || !add_tx); } - have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash())); + have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); { bool add_tx = orphanage.AddTx(tx, peer_id); // if have_tx is still false, it must be too big @@ -114,12 +115,12 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) } }, [&] { - bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash())); + bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); // EraseTx should return 0 if m_orphans doesn't have the tx { Assert(have_tx == orphanage.EraseTx(tx->GetHash())); } - have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash())); + have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); // have_tx should be false and EraseTx should fail { Assert(!have_tx && !orphanage.EraseTx(tx->GetHash())); @@ -132,7 +133,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) // test mocktime and expiry SetMockTime(ConsumeTime(fuzzed_data_provider)); auto limit = fuzzed_data_provider.ConsumeIntegral<unsigned int>(); - orphanage.LimitOrphans(limit); + orphanage.LimitOrphans(limit, limit_orphans_rng); Assert(orphanage.Size() <= limit); }); } diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index bf465c0c64..4231fcc909 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -129,11 +129,12 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) } // Test LimitOrphanTxSize() function: - orphanage.LimitOrphans(40); + FastRandomContext rng{/*fDeterministic=*/true}; + orphanage.LimitOrphans(40, rng); BOOST_CHECK(orphanage.CountOrphans() <= 40); - orphanage.LimitOrphans(10); + orphanage.LimitOrphans(10, rng); BOOST_CHECK(orphanage.CountOrphans() <= 10); - orphanage.LimitOrphans(0); + orphanage.LimitOrphans(0, rng); BOOST_CHECK(orphanage.CountOrphans() == 0); } diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index bc9c672560..13ec89663a 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -8,6 +8,7 @@ #include <txmempool.h> #include <uint256.h> #include <util/time.h> +#include <validationinterface.h> #include <test/util/setup_common.h> @@ -19,7 +20,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) { CBlockPolicyEstimator& feeEst = *Assert(m_node.fee_estimator); CTxMemPool& mpool = *Assert(m_node.mempool); - LOCK2(cs_main, mpool.cs); + RegisterValidationInterface(&feeEst); TestMemPoolEntryHelper entry; CAmount basefee(2000); CAmount deltaFee(100); @@ -59,8 +60,23 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int j = 0; j < 10; j++) { // For each fee for (int k = 0; k < 4; k++) { // add 4 fee txs tx.vin[0].prevout.n = 10000*blocknum+100*j+k; // make transaction unique + { + LOCK2(cs_main, mpool.cs); + mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now<NodeSeconds>()).Height(blocknum).FromTx(tx)); + // Since TransactionAddedToMempool callbacks are generated in ATMP, + // not addUnchecked, we cheat and create one manually here + const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); + const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), + feeV[j], + virtual_size, + entry.nHeight, + /* m_from_disconnected_block */ false, + /* m_submitted_in_package */ false, + /* m_chainstate_is_current */ true, + /* m_has_no_mempool_parents */ true)}; + GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); + } uint256 hash = tx.GetHash(); - mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now<NodeSeconds>()).Height(blocknum).FromTx(tx)); txHashes[j].push_back(hash); } } @@ -76,10 +92,17 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) txHashes[9-h].pop_back(); } } - mpool.removeForBlock(block, ++blocknum); + + { + LOCK(mpool.cs); + mpool.removeForBlock(block, ++blocknum); + } + block.clear(); // Check after just a few txs that combining buckets works as expected if (blocknum == 3) { + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); // At this point we should need to combine 3 buckets to get enough data points // So estimateFee(1) should fail and estimateFee(2) should return somewhere around // 9*baserate. estimateFee(2) %'s are 100,100,90 = average 97% @@ -114,8 +137,13 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) // Mine 50 more blocks with no transactions happening, estimates shouldn't change // We haven't decayed the moving average enough so we still have enough data points in every bucket - while (blocknum < 250) + while (blocknum < 250) { + LOCK(mpool.cs); mpool.removeForBlock(block, ++blocknum); + } + + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); BOOST_CHECK(feeEst.estimateFee(1) == CFeeRate(0)); for (int i = 2; i < 10;i++) { @@ -130,14 +158,35 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int j = 0; j < 10; j++) { // For each fee multiple for (int k = 0; k < 4; k++) { // add 4 fee txs tx.vin[0].prevout.n = 10000*blocknum+100*j+k; + { + LOCK2(cs_main, mpool.cs); + mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now<NodeSeconds>()).Height(blocknum).FromTx(tx)); + // Since TransactionAddedToMempool callbacks are generated in ATMP, + // not addUnchecked, we cheat and create one manually here + const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); + const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), + feeV[j], + virtual_size, + entry.nHeight, + /* m_from_disconnected_block */ false, + /* m_submitted_in_package */ false, + /* m_chainstate_is_current */ true, + /* m_has_no_mempool_parents */ true)}; + GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); + } uint256 hash = tx.GetHash(); - mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now<NodeSeconds>()).Height(blocknum).FromTx(tx)); txHashes[j].push_back(hash); } } - mpool.removeForBlock(block, ++blocknum); + { + LOCK(mpool.cs); + mpool.removeForBlock(block, ++blocknum); + } } + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); + for (int i = 1; i < 10;i++) { BOOST_CHECK(feeEst.estimateFee(i) == CFeeRate(0) || feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee); } @@ -152,8 +201,16 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) txHashes[j].pop_back(); } } - mpool.removeForBlock(block, 266); + + { + LOCK(mpool.cs); + mpool.removeForBlock(block, 266); + } block.clear(); + + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); + BOOST_CHECK(feeEst.estimateFee(1) == CFeeRate(0)); for (int i = 2; i < 10;i++) { BOOST_CHECK(feeEst.estimateFee(i) == CFeeRate(0) || feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee); @@ -165,17 +222,39 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int j = 0; j < 10; j++) { // For each fee multiple for (int k = 0; k < 4; k++) { // add 4 fee txs tx.vin[0].prevout.n = 10000*blocknum+100*j+k; + { + LOCK2(cs_main, mpool.cs); + mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now<NodeSeconds>()).Height(blocknum).FromTx(tx)); + // Since TransactionAddedToMempool callbacks are generated in ATMP, + // not addUnchecked, we cheat and create one manually here + const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); + const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), + feeV[j], + virtual_size, + entry.nHeight, + /* m_from_disconnected_block */ false, + /* m_submitted_in_package */ false, + /* m_chainstate_is_current */ true, + /* m_has_no_mempool_parents */ true)}; + GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); + } uint256 hash = tx.GetHash(); - mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now<NodeSeconds>()).Height(blocknum).FromTx(tx)); CTransactionRef ptx = mpool.get(hash); if (ptx) block.push_back(ptx); } } - mpool.removeForBlock(block, ++blocknum); + + { + LOCK(mpool.cs); + mpool.removeForBlock(block, ++blocknum); + } + block.clear(); } + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); BOOST_CHECK(feeEst.estimateFee(1) == CFeeRate(0)); for (int i = 2; i < 9; i++) { // At 9, the original estimate was already at the bottom (b/c scale = 2) BOOST_CHECK(feeEst.estimateFee(i).GetFeePerK() < origFeeEst[i-1] - deltaFee); diff --git a/src/test/pow_tests.cpp b/src/test/pow_tests.cpp index 5bd14f22c6..3a44d1da49 100644 --- a/src/test/pow_tests.cpp +++ b/src/test/pow_tests.cpp @@ -177,7 +177,7 @@ void sanity_check_chainparams(const ArgsManager& args, ChainType chain_type) // check max target * 4*nPowTargetTimespan doesn't overflow -- see pow.cpp:CalculateNextWorkRequired() if (!consensus.fPowNoRetargeting) { - arith_uint256 targ_max("0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"); + arith_uint256 targ_max{UintToArith256(uint256S("0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"))}; targ_max /= consensus.nPowTargetTimespan*4; BOOST_CHECK(UintToArith256(consensus.powLimit) < targ_max); } diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp index 740f461548..6a96b60db0 100644 --- a/src/test/system_tests.cpp +++ b/src/test/system_tests.cpp @@ -90,7 +90,13 @@ BOOST_AUTO_TEST_CASE(run_command) }); } { - BOOST_REQUIRE_THROW(RunCommandParseJSON("echo \"{\""), std::runtime_error); // Unable to parse JSON + // Unable to parse JSON +#ifdef WIN32 + const std::string command{"cmd.exe /c echo {"}; +#else + const std::string command{"echo {"}; +#endif + BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, HasReason("Unable to parse JSON: {")); } // Test std::in, except for Windows #ifndef WIN32 diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp index 3373f00741..5746961550 100644 --- a/src/test/uint256_tests.cpp +++ b/src/test/uint256_tests.cpp @@ -259,8 +259,8 @@ BOOST_AUTO_TEST_CASE( conversion ) BOOST_CHECK(UintToArith256(OneL) == 1); BOOST_CHECK(ArithToUint256(0) == ZeroL); BOOST_CHECK(ArithToUint256(1) == OneL); - BOOST_CHECK(arith_uint256(R1L.GetHex()) == UintToArith256(R1L)); - BOOST_CHECK(arith_uint256(R2L.GetHex()) == UintToArith256(R2L)); + BOOST_CHECK(arith_uint256(UintToArith256(uint256S(R1L.GetHex()))) == UintToArith256(R1L)); + BOOST_CHECK(arith_uint256(UintToArith256(uint256S(R2L.GetHex()))) == UintToArith256(R2L)); BOOST_CHECK(R1L.GetHex() == UintToArith256(R1L).GetHex()); BOOST_CHECK(R2L.GetHex() == UintToArith256(R2L).GetHex()); } @@ -278,6 +278,34 @@ BOOST_AUTO_TEST_CASE( operator_with_self ) BOOST_CHECK(v == UintToArith256(uint256S("0"))); } +BOOST_AUTO_TEST_CASE(parse) +{ + { + std::string s_12{"0000000000000000000000000000000000000000000000000000000000000012"}; + BOOST_CHECK_EQUAL(uint256S("12\0").GetHex(), s_12); + BOOST_CHECK_EQUAL(uint256S(std::string{"12\0", 3}).GetHex(), s_12); + BOOST_CHECK_EQUAL(uint256S("0x12").GetHex(), s_12); + BOOST_CHECK_EQUAL(uint256S(" 0x12").GetHex(), s_12); + BOOST_CHECK_EQUAL(uint256S(" 12").GetHex(), s_12); + } + { + std::string s_1{uint256::ONE.GetHex()}; + BOOST_CHECK_EQUAL(uint256S("1\0").GetHex(), s_1); + BOOST_CHECK_EQUAL(uint256S(std::string{"1\0", 2}).GetHex(), s_1); + BOOST_CHECK_EQUAL(uint256S("0x1").GetHex(), s_1); + BOOST_CHECK_EQUAL(uint256S(" 0x1").GetHex(), s_1); + BOOST_CHECK_EQUAL(uint256S(" 1").GetHex(), s_1); + } + { + std::string s_0{uint256::ZERO.GetHex()}; + BOOST_CHECK_EQUAL(uint256S("\0").GetHex(), s_0); + BOOST_CHECK_EQUAL(uint256S(std::string{"\0", 1}).GetHex(), s_0); + BOOST_CHECK_EQUAL(uint256S("0x").GetHex(), s_0); + BOOST_CHECK_EQUAL(uint256S(" 0x").GetHex(), s_0); + BOOST_CHECK_EQUAL(uint256S(" ").GetHex(), s_0); + } +} + BOOST_AUTO_TEST_CASE( check_ONE ) { uint256 one = uint256S("0000000000000000000000000000000000000000000000000000000000000001"); diff --git a/src/test/util/net.h b/src/test/util/net.h index 59c4ddb4b1..d91d801132 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -70,7 +70,10 @@ struct ConnmanTestMsg : public CConnman { bool relay_txs) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex); - void ProcessMessagesOnce(CNode& node) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) { m_msgproc->ProcessMessages(&node, flagInterruptMsgProc); } + bool ProcessMessagesOnce(CNode& node) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) + { + return m_msgproc->ProcessMessages(&node, flagInterruptMsgProc); + } void NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const; diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index 6d3bb01be8..379c3c9329 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -18,7 +18,6 @@ using node::NodeContext; CTxMemPool::Options MemPoolOptionsForTest(const NodeContext& node) { CTxMemPool::Options mempool_opts{ - .estimator = node.fee_estimator.get(), // Default to always checking mempool regardless of // chainparams.DefaultConsistencyChecks for tests .check_ratio = 1, diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 4d812544bd..47808a2a58 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -12,7 +12,6 @@ #include <util/bitdeque.h> #include <util/fs.h> #include <util/fs_helpers.h> -#include <util/getuniquepath.h> #include <util/message.h> // For MessageSign(), MessageVerify(), MESSAGE_MAGIC #include <util/moneystr.h> #include <util/overflow.h> @@ -1106,15 +1105,16 @@ BOOST_AUTO_TEST_CASE(test_ParseFixedPoint) BOOST_CHECK(!ParseFixedPoint("31.999999999999999999999", 3, &amount)); } -static void TestOtherThread(fs::path dirname, fs::path lockname, bool *result) -{ - *result = LockDirectory(dirname, lockname); -} - #ifndef WIN32 // Cannot do this test on WIN32 due to lack of fork() static constexpr char LockCommand = 'L'; static constexpr char UnlockCommand = 'U'; static constexpr char ExitCommand = 'X'; +enum : char { + ResSuccess = 2, // Start with 2 to avoid accidental collision with common values 0 and 1 + ResErrorWrite, + ResErrorLock, + ResUnlockSuccess, +}; [[noreturn]] static void TestOtherProcess(fs::path dirname, fs::path lockname, int fd) { @@ -1122,15 +1122,22 @@ static constexpr char ExitCommand = 'X'; while (true) { int rv = read(fd, &ch, 1); // Wait for command assert(rv == 1); - switch(ch) { + switch (ch) { case LockCommand: - ch = LockDirectory(dirname, lockname); + ch = [&] { + switch (util::LockDirectory(dirname, lockname)) { + case util::LockResult::Success: return ResSuccess; + case util::LockResult::ErrorWrite: return ResErrorWrite; + case util::LockResult::ErrorLock: return ResErrorLock; + } // no default case, so the compiler can warn about missing cases + assert(false); + }(); rv = write(fd, &ch, 1); assert(rv == 1); break; case UnlockCommand: ReleaseDirectoryLocks(); - ch = true; // Always succeeds + ch = ResUnlockSuccess; // Always succeeds rv = write(fd, &ch, 1); assert(rv == 1); break; @@ -1165,53 +1172,58 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory) TestOtherProcess(dirname, lockname, fd[0]); } BOOST_CHECK_EQUAL(close(fd[0]), 0); // Parent: close child end + + char ch; + // Lock on non-existent directory should fail + BOOST_CHECK_EQUAL(write(fd[1], &LockCommand, 1), 1); + BOOST_CHECK_EQUAL(read(fd[1], &ch, 1), 1); + BOOST_CHECK_EQUAL(ch, ResErrorWrite); #endif // Lock on non-existent directory should fail - BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname), false); + BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::ErrorWrite); fs::create_directories(dirname); // Probing lock on new directory should succeed - BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), true); + BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname, true), util::LockResult::Success); // Persistent lock on new directory should succeed - BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname), true); + BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::Success); // Another lock on the directory from the same thread should succeed - BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname), true); + BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::Success); // Another lock on the directory from a different thread within the same process should succeed - bool threadresult; - std::thread thr(TestOtherThread, dirname, lockname, &threadresult); + util::LockResult threadresult; + std::thread thr([&] { threadresult = util::LockDirectory(dirname, lockname); }); thr.join(); - BOOST_CHECK_EQUAL(threadresult, true); + BOOST_CHECK_EQUAL(threadresult, util::LockResult::Success); #ifndef WIN32 // Try to acquire lock in child process while we're holding it, this should fail. - char ch; BOOST_CHECK_EQUAL(write(fd[1], &LockCommand, 1), 1); BOOST_CHECK_EQUAL(read(fd[1], &ch, 1), 1); - BOOST_CHECK_EQUAL((bool)ch, false); + BOOST_CHECK_EQUAL(ch, ResErrorLock); // Give up our lock ReleaseDirectoryLocks(); // Probing lock from our side now should succeed, but not hold on to the lock. - BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), true); + BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname, true), util::LockResult::Success); // Try to acquire the lock in the child process, this should be successful. BOOST_CHECK_EQUAL(write(fd[1], &LockCommand, 1), 1); BOOST_CHECK_EQUAL(read(fd[1], &ch, 1), 1); - BOOST_CHECK_EQUAL((bool)ch, true); + BOOST_CHECK_EQUAL(ch, ResSuccess); // When we try to probe the lock now, it should fail. - BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), false); + BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname, true), util::LockResult::ErrorLock); // Unlock the lock in the child process BOOST_CHECK_EQUAL(write(fd[1], &UnlockCommand, 1), 1); BOOST_CHECK_EQUAL(read(fd[1], &ch, 1), 1); - BOOST_CHECK_EQUAL((bool)ch, true); + BOOST_CHECK_EQUAL(ch, ResUnlockSuccess); // When we try to probe the lock now, it should succeed. - BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), true); + BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname, true), util::LockResult::Success); // Re-lock the lock in the child process, then wait for it to exit, check // successful return. After that, we check that exiting the process @@ -1224,7 +1236,7 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory) BOOST_CHECK_EQUAL(write(fd[1], &ExitCommand, 1), 1); BOOST_CHECK_EQUAL(waitpid(pid, &processstatus, 0), pid); BOOST_CHECK_EQUAL(processstatus, 0); - BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), true); + BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname, true), util::LockResult::Success); // Restore SIGCHLD signal(SIGCHLD, old_handler); @@ -1235,22 +1247,6 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory) fs::remove_all(dirname); } -BOOST_AUTO_TEST_CASE(test_DirIsWritable) -{ - // Should be able to write to the data dir. - fs::path tmpdirname = m_args.GetDataDirBase(); - BOOST_CHECK_EQUAL(DirIsWritable(tmpdirname), true); - - // Should not be able to write to a non-existent dir. - tmpdirname = GetUniquePath(tmpdirname); - BOOST_CHECK_EQUAL(DirIsWritable(tmpdirname), false); - - fs::create_directory(tmpdirname); - // Should be able to write to it now. - BOOST_CHECK_EQUAL(DirIsWritable(tmpdirname), true); - fs::remove(tmpdirname); -} - BOOST_AUTO_TEST_CASE(test_ToLower) { BOOST_CHECK_EQUAL(ToLower('@'), '@'); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index c53431cf8a..b783181bb8 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -12,9 +12,9 @@ #include <consensus/tx_verify.h> #include <consensus/validation.h> #include <logging.h> -#include <policy/fees.h> #include <policy/policy.h> #include <policy/settings.h> +#include <random.h> #include <reverse_iterator.h> #include <util/check.h> #include <util/moneystr.h> @@ -402,7 +402,6 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, CTxMemPool::CTxMemPool(const Options& opts) : m_check_ratio{opts.check_ratio}, - minerPolicyEstimator{opts.estimator}, m_max_size_bytes{opts.max_size_bytes}, m_expiry{opts.expiry}, m_incremental_relay_feerate{opts.incremental_relay_feerate}, @@ -433,7 +432,7 @@ void CTxMemPool::AddTransactionsUpdated(unsigned int n) nTransactionsUpdated += n; } -void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAncestors, bool validFeeEstimate) +void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAncestors) { // Add to memory pool without checking anything. // Used by AcceptToMemoryPool(), which DOES do @@ -477,9 +476,6 @@ void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAnces nTransactionsUpdated++; totalTxSize += entry.GetTxSize(); m_total_fee += entry.GetFee(); - if (minerPolicyEstimator) { - minerPolicyEstimator->processTransaction(entry, validFeeEstimate); - } txns_randomized.emplace_back(newit->GetSharedTx()); newit->idx_randomized = txns_randomized.size() - 1; @@ -512,11 +508,10 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) std::chrono::duration_cast<std::chrono::duration<std::uint64_t>>(it->GetTime()).count() ); - const uint256 hash = it->GetTx().GetHash(); for (const CTxIn& txin : it->GetTx().vin) mapNextTx.erase(txin.prevout); - RemoveUnbroadcastTx(hash, true /* add logging because unchecked */ ); + RemoveUnbroadcastTx(it->GetTx().GetHash(), true /* add logging because unchecked */); if (txns_randomized.size() > 1) { // Update idx_randomized of the to-be-moved entry. @@ -535,7 +530,6 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) cachedInnerUsage -= memusage::DynamicUsage(it->GetMemPoolParentsConst()) + memusage::DynamicUsage(it->GetMemPoolChildrenConst()); mapTx.erase(it); nTransactionsUpdated++; - if (minerPolicyEstimator) {minerPolicyEstimator->removeTx(hash, false);} } // Calculates descendants of entry that are not already in setDescendants, and adds to @@ -636,33 +630,26 @@ void CTxMemPool::removeConflicts(const CTransaction &tx) } /** - * Called when a block is connected. Removes from mempool and updates the miner fee estimator. + * Called when a block is connected. Removes from mempool. */ void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) { AssertLockHeld(cs); - std::vector<const CTxMemPoolEntry*> entries; - for (const auto& tx : vtx) - { - uint256 hash = tx->GetHash(); - - indexed_transaction_set::iterator i = mapTx.find(hash); - if (i != mapTx.end()) - entries.push_back(&*i); - } - // Before the txs in the new block have been removed from the mempool, update policy estimates - if (minerPolicyEstimator) {minerPolicyEstimator->processBlock(nBlockHeight, entries);} + std::vector<RemovedMempoolTransactionInfo> txs_removed_for_block; + txs_removed_for_block.reserve(vtx.size()); for (const auto& tx : vtx) { txiter it = mapTx.find(tx->GetHash()); if (it != mapTx.end()) { setEntries stage; stage.insert(it); + txs_removed_for_block.emplace_back(*it); RemoveStaged(stage, true, MemPoolRemovalReason::BLOCK); } removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); } + GetMainSignals().MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; } @@ -1092,10 +1079,10 @@ int CTxMemPool::Expire(std::chrono::seconds time) return stage.size(); } -void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, bool validFeeEstimate) +void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry) { auto ancestors{AssumeCalculateMemPoolAncestors(__func__, entry, Limits::NoLimits())}; - return addUnchecked(entry, ancestors, validFeeEstimate); + return addUnchecked(entry, ancestors); } void CTxMemPool::UpdateChild(txiter entry, txiter child, bool add) diff --git a/src/txmempool.h b/src/txmempool.h index aed6acd9da..bac7a445d6 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -202,8 +202,6 @@ struct entry_time {}; struct ancestor_score {}; struct index_by_wtxid {}; -class CBlockPolicyEstimator; - /** * Information about a mempool transaction. */ @@ -303,7 +301,6 @@ class CTxMemPool protected: const int m_check_ratio; //!< Value n means that 1 times in n we check. std::atomic<unsigned int> nTransactionsUpdated{0}; //!< Used by getblocktemplate to trigger CreateNewBlock() invocation - CBlockPolicyEstimator* const minerPolicyEstimator; 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) @@ -472,8 +469,8 @@ public: // Note that addUnchecked is ONLY called from ATMP outside of tests // and any other callers may break wallet's in-mempool tracking (due to // lack of CValidationInterface::TransactionAddedToMempool callbacks). - void addUnchecked(const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); - void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); + void addUnchecked(const CTxMemPoolEntry& entry) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); + void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); /** After reorg, filter the entries that would no longer be valid in the next block, and update diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 16f54644c2..e907fffd4f 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -113,7 +113,7 @@ void TxOrphanage::EraseForPeer(NodeId peer) if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer); } -void TxOrphanage::LimitOrphans(unsigned int max_orphans) +void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) { LOCK(m_mutex); @@ -138,7 +138,6 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans) nNextSweep = nMinExpTime + ORPHAN_TX_EXPIRE_INTERVAL; if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx due to expiration\n", nErased); } - FastRandomContext rng; while (m_orphans.size() > max_orphans) { // Evict a random orphan: diff --git a/src/txorphanage.h b/src/txorphanage.h index 2196ed4c85..2fd14e6fd2 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -43,7 +43,7 @@ public: void EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); /** Limit the orphanage to the given maximum */ - void LimitOrphans(unsigned int max_orphans) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + void LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); /** Add any orphans that list a particular tx as a parent into the from peer's work set */ void AddChildrenToWorkSet(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);; diff --git a/src/txrequest.cpp b/src/txrequest.cpp index b9a41f26ff..ce5fbd9a7f 100644 --- a/src/txrequest.cpp +++ b/src/txrequest.cpp @@ -73,7 +73,7 @@ struct Announcement { const bool m_is_wtxid : 1; /** What state this announcement is in. */ - State m_state : 3; + State m_state : 3 {State::CANDIDATE_DELAYED}; State GetState() const { return m_state; } void SetState(State state) { m_state = state; } @@ -97,9 +97,9 @@ struct Announcement { /** Construct a new announcement from scratch, initially in CANDIDATE_DELAYED state. */ Announcement(const GenTxid& gtxid, NodeId peer, bool preferred, std::chrono::microseconds reqtime, - SequenceNumber sequence) : - m_txhash(gtxid.GetHash()), m_time(reqtime), m_peer(peer), m_sequence(sequence), m_preferred(preferred), - m_is_wtxid{gtxid.IsWtxid()}, m_state{State::CANDIDATE_DELAYED} {} + SequenceNumber sequence) + : m_txhash(gtxid.GetHash()), m_time(reqtime), m_peer(peer), m_sequence(sequence), m_preferred(preferred), + m_is_wtxid{gtxid.IsWtxid()} {} }; //! Type alias for priorities. diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp index 8aa7493aa8..4de8833a3f 100644 --- a/src/util/fs_helpers.cpp +++ b/src/util/fs_helpers.cpp @@ -12,7 +12,6 @@ #include <logging.h> #include <sync.h> #include <util/fs.h> -#include <util/getuniquepath.h> #include <util/syserror.h> #include <cerrno> @@ -51,31 +50,35 @@ static GlobalMutex cs_dir_locks; * is called. */ static std::map<std::string, std::unique_ptr<fsbridge::FileLock>> dir_locks GUARDED_BY(cs_dir_locks); - -bool LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only) +namespace util { +LockResult LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only) { LOCK(cs_dir_locks); fs::path pathLockFile = directory / lockfile_name; // If a lock for this directory already exists in the map, don't try to re-lock it if (dir_locks.count(fs::PathToString(pathLockFile))) { - return true; + return LockResult::Success; } // Create empty lock file if it doesn't exist. - FILE* file = fsbridge::fopen(pathLockFile, "a"); - if (file) fclose(file); + if (auto created{fsbridge::fopen(pathLockFile, "a")}) { + std::fclose(created); + } else { + return LockResult::ErrorWrite; + } auto lock = std::make_unique<fsbridge::FileLock>(pathLockFile); if (!lock->TryLock()) { - return error("Error while attempting to lock directory %s: %s", fs::PathToString(directory), lock->GetReason()); + error("Error while attempting to lock directory %s: %s", fs::PathToString(directory), lock->GetReason()); + return LockResult::ErrorLock; } if (!probe_only) { // Lock successful and we're not just probing, put it into the map dir_locks.emplace(fs::PathToString(pathLockFile), std::move(lock)); } - return true; + return LockResult::Success; } - +} // namespace util void UnlockDirectory(const fs::path& directory, const fs::path& lockfile_name) { LOCK(cs_dir_locks); @@ -88,19 +91,6 @@ void ReleaseDirectoryLocks() dir_locks.clear(); } -bool DirIsWritable(const fs::path& directory) -{ - fs::path tmpFile = GetUniquePath(directory); - - FILE* file = fsbridge::fopen(tmpFile, "a"); - if (!file) return false; - - fclose(file); - remove(tmpFile); - - return true; -} - bool CheckDiskSpace(const fs::path& dir, uint64_t additional_bytes) { constexpr uint64_t min_disk_space = 52428800; // 50 MiB diff --git a/src/util/fs_helpers.h b/src/util/fs_helpers.h index e7db01a89b..ea3778eac3 100644 --- a/src/util/fs_helpers.h +++ b/src/util/fs_helpers.h @@ -35,9 +35,15 @@ void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length); */ [[nodiscard]] bool RenameOver(fs::path src, fs::path dest); -bool LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only = false); +namespace util { +enum class LockResult { + Success, + ErrorWrite, + ErrorLock, +}; +[[nodiscard]] LockResult LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only = false); +} // namespace util void UnlockDirectory(const fs::path& directory, const fs::path& lockfile_name); -bool DirIsWritable(const fs::path& directory); bool CheckDiskSpace(const fs::path& dir, uint64_t additional_bytes = 0); /** Get the size of a file by scanning it. diff --git a/src/util/getuniquepath.cpp b/src/util/getuniquepath.cpp deleted file mode 100644 index 105b4d52d2..0000000000 --- a/src/util/getuniquepath.cpp +++ /dev/null @@ -1,14 +0,0 @@ -// Copyright (c) 2021-2022 The Bitcoin Core developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#include <random.h> -#include <util/fs.h> -#include <util/strencodings.h> - -fs::path GetUniquePath(const fs::path& base) -{ - FastRandomContext rnd; - fs::path tmpFile = base / fs::u8path(HexStr(rnd.randbytes(8))); - return tmpFile; -}
\ No newline at end of file diff --git a/src/util/getuniquepath.h b/src/util/getuniquepath.h deleted file mode 100644 index 1563652300..0000000000 --- a/src/util/getuniquepath.h +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright (c) 2021 The Bitcoin Core developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#ifndef BITCOIN_UTIL_GETUNIQUEPATH_H -#define BITCOIN_UTIL_GETUNIQUEPATH_H - -#include <util/fs.h> - -/** - * Helper function for getting a unique path - * - * @param[in] base Base path - * @returns base joined with a random 8-character long string. - * @post Returned path is unique with high probability. - */ -fs::path GetUniquePath(const fs::path& base); - -#endif // BITCOIN_UTIL_GETUNIQUEPATH_H
\ No newline at end of file diff --git a/src/util/trace.h b/src/util/trace.h index 1fe743f043..b7c275f19b 100644 --- a/src/util/trace.h +++ b/src/util/trace.h @@ -13,28 +13,19 @@ #include <sys/sdt.h> -// Disabling this warning can be removed once we switch to C++20 -#if defined(__clang__) && __cplusplus < 202002L -#define BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH _Pragma("clang diagnostic push") _Pragma("clang diagnostic ignored \"-Wgnu-zero-variadic-macro-arguments\"") -#define BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP _Pragma("clang diagnostic pop") -#else -#define BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH -#define BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#endif - -#define TRACE(context, event) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE(context, event) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE1(context, event, a) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE1(context, event, a) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE2(context, event, a, b) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE2(context, event, a, b) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE3(context, event, a, b, c) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE3(context, event, a, b, c) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE4(context, event, a, b, c, d) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE4(context, event, a, b, c, d) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE5(context, event, a, b, c, d, e) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE5(context, event, a, b, c, d, e) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE6(context, event, a, b, c, d, e, f) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE6(context, event, a, b, c, d, e, f) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE7(context, event, a, b, c, d, e, f, g) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE7(context, event, a, b, c, d, e, f, g) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE8(context, event, a, b, c, d, e, f, g, h) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE8(context, event, a, b, c, d, e, f, g, h) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE9(context, event, a, b, c, d, e, f, g, h, i) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE9(context, event, a, b, c, d, e, f, g, h, i) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE10(context, event, a, b, c, d, e, f, g, h, i, j) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE10(context, event, a, b, c, d, e, f, g, h, i, j) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE11(context, event, a, b, c, d, e, f, g, h, i, j, k) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE11(context, event, a, b, c, d, e, f, g, h, i, j, k) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE12(context, event, a, b, c, d, e, f, g, h, i, j, k, l) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE12(context, event, a, b, c, d, e, f, g, h, i, j, k, l) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP +#define TRACE(context, event) DTRACE_PROBE(context, event) +#define TRACE1(context, event, a) DTRACE_PROBE1(context, event, a) +#define TRACE2(context, event, a, b) DTRACE_PROBE2(context, event, a, b) +#define TRACE3(context, event, a, b, c) DTRACE_PROBE3(context, event, a, b, c) +#define TRACE4(context, event, a, b, c, d) DTRACE_PROBE4(context, event, a, b, c, d) +#define TRACE5(context, event, a, b, c, d, e) DTRACE_PROBE5(context, event, a, b, c, d, e) +#define TRACE6(context, event, a, b, c, d, e, f) DTRACE_PROBE6(context, event, a, b, c, d, e, f) +#define TRACE7(context, event, a, b, c, d, e, f, g) DTRACE_PROBE7(context, event, a, b, c, d, e, f, g) +#define TRACE8(context, event, a, b, c, d, e, f, g, h) DTRACE_PROBE8(context, event, a, b, c, d, e, f, g, h) +#define TRACE9(context, event, a, b, c, d, e, f, g, h, i) DTRACE_PROBE9(context, event, a, b, c, d, e, f, g, h, i) +#define TRACE10(context, event, a, b, c, d, e, f, g, h, i, j) DTRACE_PROBE10(context, event, a, b, c, d, e, f, g, h, i, j) +#define TRACE11(context, event, a, b, c, d, e, f, g, h, i, j, k) DTRACE_PROBE11(context, event, a, b, c, d, e, f, g, h, i, j, k) +#define TRACE12(context, event, a, b, c, d, e, f, g, h, i, j, k, l) DTRACE_PROBE12(context, event, a, b, c, d, e, f, g, h, i, j, k, l) #else diff --git a/src/validation.cpp b/src/validation.cpp index 8a5bb93ef8..0501499004 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1126,17 +1126,8 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) ws.m_replaced_transactions.push_back(it->GetSharedTx()); } m_pool.RemoveStaged(ws.m_all_conflicting, false, MemPoolRemovalReason::REPLACED); - - // This transaction should only count for fee estimation if: - // - it's not being re-added during a reorg which bypasses typical mempool fee limits - // - the node is not behind - // - the transaction is not dependent on any other transactions in the mempool - // - it's not part of a package. Since package relay is not currently supported, this - // transaction has not necessarily been accepted to miners' mempools. - bool validForFeeEstimation = !bypass_limits && !args.m_package_submission && IsCurrentForFeeEstimation(m_active_chainstate) && m_pool.HasNoInputsOf(tx); - // Store transaction in memory - m_pool.addUnchecked(*entry, ws.m_ancestors, validForFeeEstimation); + m_pool.addUnchecked(*entry, ws.m_ancestors); // trim mempool and check if tx was trimmed // If we are validating a package, don't trim here because we could evict a previous transaction @@ -1222,7 +1213,13 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>& results.emplace(ws.m_ptx->GetWitnessHash(), 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()); + const CTransaction& tx = *ws.m_ptx; + const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, + ws.m_vsize, ws.m_entry->GetHeight(), + args.m_bypass_limits, args.m_package_submission, + IsCurrentForFeeEstimation(m_active_chainstate), + m_pool.HasNoInputsOf(tx)); + GetMainSignals().TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); } return all_submitted; } @@ -1265,7 +1262,13 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), {ws.m_ptx->GetWitnessHash()}); } - GetMainSignals().TransactionAddedToMempool(ptx, m_pool.GetAndIncrementSequence()); + const CTransaction& tx = *ws.m_ptx; + const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, + ws.m_vsize, ws.m_entry->GetHeight(), + args.m_bypass_limits, args.m_package_submission, + IsCurrentForFeeEstimation(m_active_chainstate), + m_pool.HasNoInputsOf(tx)); + GetMainSignals().TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, single_wtxid); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 9241395ad5..5e944a7c47 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -9,6 +9,7 @@ #include <chain.h> #include <consensus/validation.h> #include <kernel/chain.h> +#include <kernel/mempool_entry.h> #include <logging.h> #include <primitives/block.h> #include <primitives/transaction.h> @@ -205,13 +206,14 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd fInitialDownload); } -void CMainSignals::TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) { +void CMainSignals::TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) +{ auto event = [tx, mempool_sequence, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(tx, mempool_sequence); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s", __func__, - tx->GetHash().ToString(), - tx->GetWitnessHash().ToString()); + tx.info.m_tx->GetHash().ToString(), + tx.info.m_tx->GetWitnessHash().ToString()); } void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) { @@ -233,6 +235,16 @@ void CMainSignals::BlockConnected(ChainstateRole role, const std::shared_ptr<con pindex->nHeight); } +void CMainSignals::MempoolTransactionsRemovedForBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block, unsigned int nBlockHeight) +{ + auto event = [txs_removed_for_block, nBlockHeight, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); }); + }; + ENQUEUE_AND_LOG_EVENT(event, "%s: block height=%s txs removed=%s", __func__, + nBlockHeight, + txs_removed_for_block.size()); +} + void CMainSignals::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex) { auto event = [pblock, pindex, this] { diff --git a/src/validationinterface.h b/src/validationinterface.h index eb15aa4d5f..e1d6869fab 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -21,6 +21,8 @@ struct CBlockLocator; class CValidationInterface; class CScheduler; enum class MemPoolRemovalReason; +struct RemovedMempoolTransactionInfo; +struct NewMempoolTransactionInfo; /** Register subscriber */ void RegisterValidationInterface(CValidationInterface* callbacks); @@ -60,10 +62,10 @@ void CallFunctionInValidationInterfaceQueue(std::function<void ()> func); void SyncWithValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main); /** - * Implement this to subscribe to events generated in validation + * Implement this to subscribe to events generated in validation and mempool * * Each CValidationInterface() subscriber will receive event callbacks - * in the order in which the events were generated by validation. + * in the order in which the events were generated by validation and mempool. * Furthermore, each ValidationInterface() subscriber may assume that * callbacks effectively run in a single thread with single-threaded * memory consistency. That is, for a given ValidationInterface() @@ -96,7 +98,7 @@ protected: * * Called on a background thread. */ - virtual void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) {} + virtual void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) {} /** * Notifies listeners of a transaction leaving mempool. @@ -113,7 +115,7 @@ protected: * This does not fire for transactions that are removed from the mempool * because they have been included in a block. Any client that is interested * in transactions removed from the mempool for inclusion in a block can learn - * about those transactions from the BlockConnected notification. + * about those transactions from the MempoolTransactionsRemovedForBlock notification. * * Transactions that are removed from the mempool because they conflict * with a transaction in the new block will have @@ -131,6 +133,14 @@ protected: * Called on a background thread. */ virtual void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) {} + /* + * Notifies listeners of transactions removed from the mempool as + * as a result of new block being connected. + * MempoolTransactionsRemovedForBlock will be fired before BlockConnected. + * + * Called on a background thread. + */ + virtual void MempoolTransactionsRemovedForBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block, unsigned int nBlockHeight) {} /** * Notifies listeners of a block being connected. * Provides a vector of transactions evicted from the mempool as a result. @@ -140,6 +150,7 @@ protected: virtual void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock> &block, const CBlockIndex *pindex) {} /** * Notifies listeners of a block being disconnected + * Provides the block that was connected. * * Called on a background thread. Only called for the active chainstate, since * background chainstates should never disconnect blocks. @@ -200,8 +211,9 @@ public: void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); - void TransactionAddedToMempool(const CTransactionRef&, uint64_t mempool_sequence); + void TransactionAddedToMempool(const NewMempoolTransactionInfo&, uint64_t mempool_sequence); void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason, uint64_t mempool_sequence); + void MempoolTransactionsRemovedForBlock(const std::vector<RemovedMempoolTransactionInfo>&, unsigned int nBlockHeight); void BlockConnected(ChainstateRole, const std::shared_ptr<const CBlock> &, const CBlockIndex *pindex); void BlockDisconnected(const std::shared_ptr<const CBlock> &, const CBlockIndex* pindex); void ChainStateFlushed(ChainstateRole, const CBlockLocator &); diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 9ea43ca67c..cbf6c9b1ea 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -149,7 +149,7 @@ bool BerkeleyEnvironment::Open(bilingual_str& err) fs::path pathIn = fs::PathFromString(strPath); TryCreateDirectories(pathIn); - if (!LockDirectory(pathIn, ".walletlock")) { + if (util::LockDirectory(pathIn, ".walletlock") != util::LockResult::Success) { LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance may be using it.\n", strPath); err = strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory()))); return false; diff --git a/src/wallet/coincontrol.cpp b/src/wallet/coincontrol.cpp index 2087119db9..873c5ab383 100644 --- a/src/wallet/coincontrol.cpp +++ b/src/wallet/coincontrol.cpp @@ -14,69 +14,142 @@ CCoinControl::CCoinControl() bool CCoinControl::HasSelected() const { - return !m_selected_inputs.empty(); + return !m_selected.empty(); } -bool CCoinControl::IsSelected(const COutPoint& output) const +bool CCoinControl::IsSelected(const COutPoint& outpoint) const { - return m_selected_inputs.count(output) > 0; + return m_selected.count(outpoint) > 0; } -bool CCoinControl::IsExternalSelected(const COutPoint& output) const +bool CCoinControl::IsExternalSelected(const COutPoint& outpoint) const { - return m_external_txouts.count(output) > 0; + const auto it = m_selected.find(outpoint); + return it != m_selected.end() && it->second.HasTxOut(); } std::optional<CTxOut> CCoinControl::GetExternalOutput(const COutPoint& outpoint) const { - const auto ext_it = m_external_txouts.find(outpoint); - if (ext_it == m_external_txouts.end()) { + const auto it = m_selected.find(outpoint); + if (it == m_selected.end() || !it->second.HasTxOut()) { return std::nullopt; } + return it->second.GetTxOut(); +} - return std::make_optional(ext_it->second); +PreselectedInput& CCoinControl::Select(const COutPoint& outpoint) +{ + auto& input = m_selected[outpoint]; + input.SetPosition(m_selection_pos); + ++m_selection_pos; + return input; +} +void CCoinControl::UnSelect(const COutPoint& outpoint) +{ + m_selected.erase(outpoint); } -void CCoinControl::Select(const COutPoint& output) +void CCoinControl::UnSelectAll() { - m_selected_inputs.insert(output); + m_selected.clear(); } -void CCoinControl::SelectExternal(const COutPoint& outpoint, const CTxOut& txout) +std::vector<COutPoint> CCoinControl::ListSelected() const { - m_selected_inputs.insert(outpoint); - m_external_txouts.emplace(outpoint, txout); + std::vector<COutPoint> outpoints; + std::transform(m_selected.begin(), m_selected.end(), std::back_inserter(outpoints), + [](const std::map<COutPoint, PreselectedInput>::value_type& pair) { + return pair.first; + }); + return outpoints; } -void CCoinControl::UnSelect(const COutPoint& output) +void CCoinControl::SetInputWeight(const COutPoint& outpoint, int64_t weight) { - m_selected_inputs.erase(output); + m_selected[outpoint].SetInputWeight(weight); } -void CCoinControl::UnSelectAll() +std::optional<int64_t> CCoinControl::GetInputWeight(const COutPoint& outpoint) const { - m_selected_inputs.clear(); + const auto it = m_selected.find(outpoint); + return it != m_selected.end() ? it->second.GetInputWeight() : std::nullopt; } -std::vector<COutPoint> CCoinControl::ListSelected() const +std::optional<uint32_t> CCoinControl::GetSequence(const COutPoint& outpoint) const { - return {m_selected_inputs.begin(), m_selected_inputs.end()}; + const auto it = m_selected.find(outpoint); + return it != m_selected.end() ? it->second.GetSequence() : std::nullopt; } -void CCoinControl::SetInputWeight(const COutPoint& outpoint, int64_t weight) +std::pair<std::optional<CScript>, std::optional<CScriptWitness>> CCoinControl::GetScripts(const COutPoint& outpoint) const +{ + const auto it = m_selected.find(outpoint); + return it != m_selected.end() ? m_selected.at(outpoint).GetScripts() : std::make_pair(std::nullopt, std::nullopt); +} + +void PreselectedInput::SetTxOut(const CTxOut& txout) +{ + m_txout = txout; +} + +CTxOut PreselectedInput::GetTxOut() const +{ + assert(m_txout.has_value()); + return m_txout.value(); +} + +bool PreselectedInput::HasTxOut() const +{ + return m_txout.has_value(); +} + +void PreselectedInput::SetInputWeight(int64_t weight) +{ + m_weight = weight; +} + +std::optional<int64_t> PreselectedInput::GetInputWeight() const +{ + return m_weight; +} + +void PreselectedInput::SetSequence(uint32_t sequence) +{ + m_sequence = sequence; +} + +std::optional<uint32_t> PreselectedInput::GetSequence() const +{ + return m_sequence; +} + +void PreselectedInput::SetScriptSig(const CScript& script) +{ + m_script_sig = script; +} + +void PreselectedInput::SetScriptWitness(const CScriptWitness& script_wit) +{ + m_script_witness = script_wit; +} + +bool PreselectedInput::HasScripts() const +{ + return m_script_sig.has_value() || m_script_witness.has_value(); +} + +std::pair<std::optional<CScript>, std::optional<CScriptWitness>> PreselectedInput::GetScripts() const { - m_input_weights[outpoint] = weight; + return {m_script_sig, m_script_witness}; } -bool CCoinControl::HasInputWeight(const COutPoint& outpoint) const +void PreselectedInput::SetPosition(unsigned int pos) { - return m_input_weights.count(outpoint) > 0; + m_pos = pos; } -int64_t CCoinControl::GetInputWeight(const COutPoint& outpoint) const +std::optional<unsigned int> PreselectedInput::GetPosition() const { - auto it = m_input_weights.find(outpoint); - assert(it != m_input_weights.end()); - return it->second; + return m_pos; } } // namespace wallet diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index 71593e236f..b2f813383d 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -24,6 +24,58 @@ const int DEFAULT_MAX_DEPTH = 9999999; //! Default for -avoidpartialspends static constexpr bool DEFAULT_AVOIDPARTIALSPENDS = false; +class PreselectedInput +{ +private: + //! The previous output being spent by this input + std::optional<CTxOut> m_txout; + //! The input weight for spending this input + std::optional<int64_t> m_weight; + //! The sequence number for this input + std::optional<uint32_t> m_sequence; + //! The scriptSig for this input + std::optional<CScript> m_script_sig; + //! The scriptWitness for this input + std::optional<CScriptWitness> m_script_witness; + //! The position in the inputs vector for this input + std::optional<unsigned int> m_pos; + +public: + /** + * Set the previous output for this input. + * Only necessary if the input is expected to be an external input. + */ + void SetTxOut(const CTxOut& txout); + /** Retrieve the previous output for this input. */ + CTxOut GetTxOut() const; + /** Return whether the previous output is set for this input. */ + bool HasTxOut() const; + + /** Set the weight for this input. */ + void SetInputWeight(int64_t weight); + /** Retrieve the input weight for this input. */ + std::optional<int64_t> GetInputWeight() const; + + /** Set the sequence for this input. */ + void SetSequence(uint32_t sequence); + /** Retrieve the sequence for this input. */ + std::optional<uint32_t> GetSequence() const; + + /** Set the scriptSig for this input. */ + void SetScriptSig(const CScript& script); + /** Set the scriptWitness for this input. */ + void SetScriptWitness(const CScriptWitness& script_wit); + /** Return whether either the scriptSig or scriptWitness are set for this input. */ + bool HasScripts() const; + /** Retrieve both the scriptSig and the scriptWitness. */ + std::pair<std::optional<CScript>, std::optional<CScriptWitness>> GetScripts() const; + + /** Store the position of this input. */ + void SetPosition(unsigned int pos); + /** Retrieve the position of this input. */ + std::optional<unsigned int> GetPosition() const; +}; + /** Coin Control Features. */ class CCoinControl { @@ -59,6 +111,10 @@ public: int m_max_depth = DEFAULT_MAX_DEPTH; //! SigningProvider that has pubkeys and scripts to do spend size estimation for external inputs FlatSigningProvider m_external_provider; + //! Locktime + std::optional<uint32_t> m_locktime; + //! Version + std::optional<uint32_t> m_version; CCoinControl(); @@ -69,11 +125,11 @@ public: /** * Returns true if the given output is pre-selected. */ - bool IsSelected(const COutPoint& output) const; + bool IsSelected(const COutPoint& outpoint) const; /** * Returns true if the given output is selected as an external input. */ - bool IsExternalSelected(const COutPoint& output) const; + bool IsExternalSelected(const COutPoint& outpoint) const; /** * Returns the external output for the given outpoint if it exists. */ @@ -82,16 +138,11 @@ public: * Lock-in the given output for spending. * The output will be included in the transaction even if it's not the most optimal choice. */ - void Select(const COutPoint& output); - /** - * Lock-in the given output as an external input for spending because it is not in the wallet. - * The output will be included in the transaction even if it's not the most optimal choice. - */ - void SelectExternal(const COutPoint& outpoint, const CTxOut& txout); + PreselectedInput& Select(const COutPoint& outpoint); /** * Unselects the given output. */ - void UnSelect(const COutPoint& output); + void UnSelect(const COutPoint& outpoint); /** * Unselects all outputs. */ @@ -105,22 +156,32 @@ public: */ void SetInputWeight(const COutPoint& outpoint, int64_t weight); /** - * Returns true if the input weight is set. - */ - bool HasInputWeight(const COutPoint& outpoint) const; - /** * Returns the input weight. */ - int64_t GetInputWeight(const COutPoint& outpoint) const; + std::optional<int64_t> GetInputWeight(const COutPoint& outpoint) const; + /** Retrieve the sequence for an input */ + std::optional<uint32_t> GetSequence(const COutPoint& outpoint) const; + /** Retrieves the scriptSig and scriptWitness for an input. */ + std::pair<std::optional<CScript>, std::optional<CScriptWitness>> GetScripts(const COutPoint& outpoint) const; + + bool HasSelectedOrder() const + { + return m_selection_pos > 0; + } + + std::optional<unsigned int> GetSelectionPos(const COutPoint& outpoint) const + { + const auto it = m_selected.find(outpoint); + if (it == m_selected.end()) { + return std::nullopt; + } + return it->second.GetPosition(); + } private: //! Selected inputs (inputs that will be used, regardless of whether they're optimal or not) - std::set<COutPoint> m_selected_inputs; - //! Map of external inputs to include in the transaction - //! These are not in the wallet, so we need to track them separately - std::map<COutPoint, CTxOut> m_external_txouts; - //! Map of COutPoints to the maximum weight for that input - std::map<COutPoint, int64_t> m_input_weights; + std::map<COutPoint, PreselectedInput> m_selected; + unsigned int m_selection_pos{0}; }; } // namespace wallet diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index 6d026d02c1..a71f8f9fbc 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -16,7 +16,7 @@ #include <vector> namespace wallet { -bool ExternalSignerScriptPubKeyMan::SetupDescriptor(std::unique_ptr<Descriptor> desc) +bool ExternalSignerScriptPubKeyMan::SetupDescriptor(WalletBatch& batch, std::unique_ptr<Descriptor> desc) { LOCK(cs_desc_man); assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); @@ -29,13 +29,12 @@ bool ExternalSignerScriptPubKeyMan::SetupDescriptor(std::unique_ptr<Descriptor> m_wallet_descriptor = w_desc; // Store the descriptor - WalletBatch batch(m_storage.GetDatabase()); if (!batch.WriteDescriptor(GetID(), m_wallet_descriptor)) { throw std::runtime_error(std::string(__func__) + ": writing descriptor failed"); } // TopUp - TopUp(); + TopUpWithDB(batch); m_storage.UnsetBlankWalletFlag(batch); return true; diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 01dc80b1ca..c052ce6129 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -23,7 +23,7 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan /** Provide a descriptor at setup time * Returns false if already setup or setup fails, true if setup is successful */ - bool SetupDescriptor(std::unique_ptr<Descriptor>desc); + bool SetupDescriptor(WalletBatch& batch, std::unique_ptr<Descriptor>desc); static ExternalSigner GetExternalSigner(); diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index d9a08310a8..c6ed0fe11c 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -203,10 +203,9 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo errors.push_back(Untranslated(strprintf("%s:%u is already spent", txin.prevout.hash.GetHex(), txin.prevout.n))); return Result::MISC_ERROR; } - if (wallet.IsMine(txin.prevout)) { - new_coin_control.Select(txin.prevout); - } else { - new_coin_control.SelectExternal(txin.prevout, coin.out); + PreselectedInput& preset_txin = new_coin_control.Select(txin.prevout); + if (!wallet.IsMine(txin.prevout)) { + preset_txin.SetTxOut(coin.out); } input_value += coin.out.nValue; spent_outputs.push_back(coin.out); @@ -317,8 +316,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo // We cannot source new unconfirmed inputs(bip125 rule 2) new_coin_control.m_min_depth = 1; - constexpr int RANDOM_CHANGE_POSITION = -1; - auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, new_coin_control, false); + auto res = CreateTransaction(wallet, recipients, std::nullopt, new_coin_control, false); if (!res) { errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res)); return Result::WALLET_ERROR; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 4ca5e29229..d15273dfc9 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -281,12 +281,12 @@ public: CAmount& fee) override { LOCK(m_wallet->cs_wallet); - auto res = CreateTransaction(*m_wallet, recipients, change_pos, + auto res = CreateTransaction(*m_wallet, recipients, change_pos == -1 ? std::nullopt : std::make_optional(change_pos), coin_control, sign); if (!res) return util::Error{util::ErrorString(res)}; const auto& txr = *res; fee = txr.fee; - change_pos = txr.change_pos; + change_pos = txr.change_pos ? *txr.change_pos : -1; return txr.tx; } diff --git a/src/wallet/rpc/encrypt.cpp b/src/wallet/rpc/encrypt.cpp index 0226d15698..e237286603 100644 --- a/src/wallet/rpc/encrypt.cpp +++ b/src/wallet/rpc/encrypt.cpp @@ -220,7 +220,11 @@ RPCHelpMan encryptwallet() "After this, any calls that interact with private keys such as sending or signing \n" "will require the passphrase to be set prior the making these calls.\n" "Use the walletpassphrase call for this, and then walletlock call.\n" - "If the wallet is already encrypted, use the walletpassphrasechange call.\n", + "If the wallet is already encrypted, use the walletpassphrasechange call.\n" + "** IMPORTANT **\n" + "For security reasons, the encryption process will generate a new HD seed, resulting\n" + "in the creation of a fresh set of active descriptors. Therefore, it is crucial to\n" + "securely back up the newly generated wallet file using the backupwallet RPC.\n", { {"passphrase", RPCArg::Type::STR, RPCArg::Optional::NO, "The pass phrase to encrypt the wallet with. It must be at least 1 character, but should be long."}, }, @@ -268,7 +272,7 @@ RPCHelpMan encryptwallet() throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: Failed to encrypt the wallet."); } - return "wallet encrypted; The keypool has been flushed and a new HD seed was generated (if you are using HD). You need to make a new backup."; + return "wallet encrypted; The keypool has been flushed and a new HD seed was generated. You need to make a new backup with the backupwallet RPC."; }, }; } diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index e7884af8b0..b121c8e1a7 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -155,8 +155,7 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto std::shuffle(recipients.begin(), recipients.end(), FastRandomContext()); // Send - constexpr int RANDOM_CHANGE_POSITION = -1; - auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, coin_control, true); + auto res = CreateTransaction(wallet, recipients, std::nullopt, coin_control, true); if (!res) { throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, util::ErrorString(res).original); } @@ -489,13 +488,13 @@ static std::vector<RPCArg> FundTxDoc(bool solving_data = true) return args; } -void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl, bool override_min_fee) +CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const UniValue& options, CCoinControl& coinControl, bool override_min_fee) { // Make sure the results are valid at least up to the most recent block // the user could have gotten from another RPC command prior to now wallet.BlockUntilSyncedToCurrentChain(); - change_position = -1; + std::optional<unsigned int> change_position; bool lockUnspents = false; UniValue subtractFeeFromOutputs; std::set<int> setSubtractFeeFromOutputs; @@ -553,7 +552,11 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, } if (options.exists("changePosition") || options.exists("change_position")) { - change_position = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt<int>(); + int pos = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt<int>(); + if (pos < 0 || (unsigned int)pos > tx.vout.size()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); + } + change_position = (unsigned int)pos; } if (options.exists("change_type")) { @@ -703,9 +706,6 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, if (tx.vout.size() == 0) throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output"); - if (change_position != -1 && (change_position < 0 || (unsigned int)change_position > tx.vout.size())) - throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); - for (unsigned int idx = 0; idx < subtractFeeFromOutputs.size(); idx++) { int pos = subtractFeeFromOutputs[idx].getInt<int>(); if (setSubtractFeeFromOutputs.count(pos)) @@ -717,11 +717,11 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, setSubtractFeeFromOutputs.insert(pos); } - bilingual_str error; - - if (!FundTransaction(wallet, tx, fee_out, change_position, error, lockUnspents, setSubtractFeeFromOutputs, coinControl)) { - throw JSONRPCError(RPC_WALLET_ERROR, error.original); + auto txr = FundTransaction(wallet, tx, change_position, lockUnspents, setSubtractFeeFromOutputs, coinControl); + if (!txr) { + throw JSONRPCError(RPC_WALLET_ERROR, ErrorString(txr).original); } + return *txr; } static void SetOptionsInputWeights(const UniValue& inputs, UniValue& options) @@ -844,17 +844,15 @@ RPCHelpMan fundrawtransaction() throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); } - CAmount fee; - int change_position; CCoinControl coin_control; // Automatically select (additional) coins. Can be overridden by options.add_inputs. coin_control.m_allow_other_inputs = true; - FundTransaction(*pwallet, tx, fee, change_position, request.params[1], coin_control, /*override_min_fee=*/true); + auto txr = FundTransaction(*pwallet, tx, request.params[1], coin_control, /*override_min_fee=*/true); UniValue result(UniValue::VOBJ); - result.pushKV("hex", EncodeHexTx(CTransaction(tx))); - result.pushKV("fee", ValueFromAmount(fee)); - result.pushKV("changepos", change_position); + result.pushKV("hex", EncodeHexTx(*txr.tx)); + result.pushKV("fee", ValueFromAmount(txr.fee)); + result.pushKV("changepos", txr.change_pos ? (int)*txr.change_pos : -1); return result; }, @@ -1276,8 +1274,6 @@ RPCHelpMan send() PreventOutdatedOptions(options); - CAmount fee; - int change_position; bool rbf{options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf}; CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf); CCoinControl coin_control; @@ -1285,9 +1281,9 @@ RPCHelpMan send() // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(options["inputs"], options); - FundTransaction(*pwallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/false); + auto txr = FundTransaction(*pwallet, rawTx, options, coin_control, /*override_min_fee=*/false); - return FinishTransaction(pwallet, options, rawTx); + return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx)); } }; } @@ -1712,8 +1708,6 @@ RPCHelpMan walletcreatefundedpsbt() UniValue options{request.params[3].isNull() ? UniValue::VOBJ : request.params[3]}; - CAmount fee; - int change_position; const UniValue &replaceable_arg = options["replaceable"]; const bool rbf{replaceable_arg.isNull() ? wallet.m_signal_rbf : replaceable_arg.get_bool()}; CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf); @@ -1722,10 +1716,10 @@ RPCHelpMan walletcreatefundedpsbt() // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(request.params[0], options); - FundTransaction(wallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/true); + auto txr = FundTransaction(wallet, rawTx, options, coin_control, /*override_min_fee=*/true); // Make a blank psbt - PartiallySignedTransaction psbtx(rawTx); + PartiallySignedTransaction psbtx(CMutableTransaction(*txr.tx)); // Fill transaction with out data but don't sign bool bip32derivs = request.params[4].isNull() ? true : request.params[4].get_bool(); @@ -1741,8 +1735,8 @@ RPCHelpMan walletcreatefundedpsbt() UniValue result(UniValue::VOBJ); result.pushKV("psbt", EncodeBase64(ssTx.str())); - result.pushKV("fee", ValueFromAmount(fee)); - result.pushKV("changepos", change_position); + result.pushKV("fee", ValueFromAmount(txr.fee)); + result.pushKV("changepos", txr.change_pos ? (int)*txr.change_pos : -1); return result; }, }; diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index d2b2801aa8..0b4800b848 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -333,7 +333,8 @@ bool LegacyScriptPubKeyMan::TopUpInactiveHDChain(const CKeyID seed_id, int64_t i chain.m_next_external_index = std::max(chain.m_next_external_index, index + 1); } - TopUpChain(chain, 0); + WalletBatch batch(m_storage.GetDatabase()); + TopUpChain(batch, chain, 0); return true; } @@ -1274,19 +1275,22 @@ bool LegacyScriptPubKeyMan::TopUp(unsigned int kpSize) return false; } - if (!TopUpChain(m_hd_chain, kpSize)) { + WalletBatch batch(m_storage.GetDatabase()); + if (!batch.TxnBegin()) return false; + if (!TopUpChain(batch, m_hd_chain, kpSize)) { return false; } for (auto& [chain_id, chain] : m_inactive_hd_chains) { - if (!TopUpChain(chain, kpSize)) { + if (!TopUpChain(batch, chain, kpSize)) { return false; } } + if (!batch.TxnCommit()) throw std::runtime_error(strprintf("Error during keypool top up. Cannot commit changes for wallet %s", m_storage.GetDisplayName())); NotifyCanGetAddressesChanged(); return true; } -bool LegacyScriptPubKeyMan::TopUpChain(CHDChain& chain, unsigned int kpSize) +bool LegacyScriptPubKeyMan::TopUpChain(WalletBatch& batch, CHDChain& chain, unsigned int kpSize) { LOCK(cs_KeyStore); @@ -1318,7 +1322,6 @@ bool LegacyScriptPubKeyMan::TopUpChain(CHDChain& chain, unsigned int kpSize) missingInternal = 0; } bool internal = false; - WalletBatch batch(m_storage.GetDatabase()); for (int64_t i = missingInternal + missingExternal; i--;) { if (i < missingInternal) { internal = true; @@ -2136,6 +2139,15 @@ std::map<CKeyID, CKey> DescriptorScriptPubKeyMan::GetKeys() const bool DescriptorScriptPubKeyMan::TopUp(unsigned int size) { + WalletBatch batch(m_storage.GetDatabase()); + if (!batch.TxnBegin()) return false; + bool res = TopUpWithDB(batch, size); + if (!batch.TxnCommit()) throw std::runtime_error(strprintf("Error during descriptors keypool top up. Cannot commit changes for wallet %s", m_storage.GetDisplayName())); + return res; +} + +bool DescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, unsigned int size) +{ LOCK(cs_desc_man); unsigned int target_size; if (size > 0) { @@ -2157,7 +2169,6 @@ bool DescriptorScriptPubKeyMan::TopUp(unsigned int size) FlatSigningProvider provider; provider.keys = GetKeys(); - WalletBatch batch(m_storage.GetDatabase()); uint256 id = GetID(); for (int32_t i = m_max_cached_index + 1; i < new_range_end; ++i) { FlatSigningProvider out_keys; @@ -2264,7 +2275,7 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const } } -bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type, bool internal) +bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal) { LOCK(cs_desc_man); assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); @@ -2325,7 +2336,6 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_ m_wallet_descriptor = w_desc; // Store the master private key, and descriptor - WalletBatch batch(m_storage.GetDatabase()); if (!AddDescriptorKeyWithDB(batch, master_key.key, master_key.key.GetPubKey())) { throw std::runtime_error(std::string(__func__) + ": writing descriptor master private key failed"); } @@ -2334,7 +2344,7 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_ } // TopUp - TopUp(); + TopUpWithDB(batch); m_storage.UnsetBlankWalletFlag(batch); return true; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 7c0eca1475..dccbf3ced6 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -366,7 +366,7 @@ private: */ bool TopUpInactiveHDChain(const CKeyID seed_id, int64_t index, bool internal); - bool TopUpChain(CHDChain& chain, unsigned int size); + bool TopUpChain(WalletBatch& batch, CHDChain& chain, unsigned int size); public: LegacyScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) : ScriptPubKeyMan(storage), m_keypool_size(keypool_size) {} @@ -583,7 +583,10 @@ private: std::unique_ptr<FlatSigningProvider> GetSigningProvider(int32_t index, bool include_private = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); protected: - WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man); + WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man); + + //! Same as 'TopUp' but designed for use within a batch transaction context + bool TopUpWithDB(WalletBatch& batch, unsigned int size = 0); public: DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size) @@ -618,12 +621,7 @@ public: bool IsHDEnabled() const override; //! Setup descriptors based on the given CExtkey - bool SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type, bool internal); - - /** Provide a descriptor at setup time - * Returns false if already setup or setup fails, true if setup is successful - */ - bool SetupDescriptor(std::unique_ptr<Descriptor>desc); + bool SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal); bool HavePrivateKeys() const override; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 35583642a5..e97e658f38 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -117,8 +117,9 @@ static std::optional<int64_t> GetSignedTxinWeight(const CWallet* wallet, const C const bool can_grind_r) { // If weight was provided, use that. - if (coin_control && coin_control->HasInputWeight(txin.prevout)) { - return coin_control->GetInputWeight(txin.prevout); + std::optional<int64_t> weight; + if (coin_control && (weight = coin_control->GetInputWeight(txin.prevout))) { + return weight.value(); } // Otherwise, use the maximum satisfaction size provided by the descriptor. @@ -261,7 +262,10 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const const bool can_grind_r = wallet.CanGrindR(); std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().calculateIndividualBumpFees(coin_control.ListSelected(), coin_selection_params.m_effective_feerate); for (const COutPoint& outpoint : coin_control.ListSelected()) { - int input_bytes = -1; + int64_t input_bytes = coin_control.GetInputWeight(outpoint).value_or(-1); + if (input_bytes != -1) { + input_bytes = GetVirtualTransactionSize(input_bytes, 0, 0); + } CTxOut txout; if (auto ptr_wtx = wallet.GetWalletTx(outpoint.hash)) { // Clearly invalid input, fail @@ -269,7 +273,9 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const return util::Error{strprintf(_("Invalid pre-selected input %s"), outpoint.ToString())}; } txout = ptr_wtx->tx->vout.at(outpoint.n); - input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control); + if (input_bytes == -1) { + input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control); + } } else { // The input is external. We did not find the tx in mapWallet. const auto out{coin_control.GetExternalOutput(outpoint)}; @@ -284,11 +290,6 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const input_bytes = CalculateMaximumSignedInputSize(txout, outpoint, &coin_control.m_external_provider, can_grind_r, &coin_control); } - // If available, override calculated size with coin control specified size - if (coin_control.HasInputWeight(outpoint)) { - input_bytes = GetVirtualTransactionSize(coin_control.GetInputWeight(outpoint), 0, 0); - } - if (input_bytes == -1) { return util::Error{strprintf(_("Not solvable pre-selected input %s"), outpoint.ToString())}; // Not solvable, can't estimate size for fee } @@ -693,9 +694,12 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co // Maximum allowed weight int max_inputs_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR); - if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change, max_inputs_weight)}) { - results.push_back(*bnb_result); - } else append_error(bnb_result); + // SFFO frequently causes issues in the context of changeless input sets: skip BnB when SFFO is active + if (!coin_selection_params.m_subtract_fee_outputs) { + if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change, max_inputs_weight)}) { + results.push_back(*bnb_result); + } else append_error(bnb_result); + } // As Knapsack and SRD can create change, also deduce change weight. max_inputs_weight -= (coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR); @@ -964,18 +968,19 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng static util::Result<CreatedTransactionResult> CreateTransactionInternal( CWallet& wallet, const std::vector<CRecipient>& vecSend, - int change_pos, + std::optional<unsigned int> change_pos, const CCoinControl& coin_control, bool sign) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { AssertLockHeld(wallet.cs_wallet); - // out variables, to be packed into returned result structure - int nChangePosInOut = change_pos; - FastRandomContext rng_fast; CMutableTransaction txNew; // The resulting transaction that we make + if (coin_control.m_version) { + txNew.nVersion = coin_control.m_version.value(); + } + CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; coin_selection_params.m_include_unsafe_inputs = coin_control.m_include_unsafe_inputs; @@ -1127,20 +1132,39 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( 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) { + if (!change_pos) { // Insert change txn at random position: - nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1); - } else if ((unsigned int)nChangePosInOut > txNew.vout.size()) { + change_pos = rng_fast.randrange(txNew.vout.size() + 1); + } else if ((unsigned int)*change_pos > txNew.vout.size()) { return util::Error{_("Transaction change output index out of range")}; } - txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); + txNew.vout.insert(txNew.vout.begin() + *change_pos, newTxOut); } else { - nChangePosInOut = -1; + change_pos = std::nullopt; } // Shuffle selected coins and fill in final vin std::vector<std::shared_ptr<COutput>> selected_coins = result.GetShuffledInputVector(); + if (coin_control.HasSelected() && coin_control.HasSelectedOrder()) { + // When there are preselected inputs, we need to move them to be the first UTXOs + // and have them be in the order selected. We can use stable_sort for this, where we + // compare with the positions stored in coin_control. The COutputs that have positions + // will be placed before those that don't, and those positions will be in order. + std::stable_sort(selected_coins.begin(), selected_coins.end(), + [&coin_control](const std::shared_ptr<COutput>& a, const std::shared_ptr<COutput>& b) { + auto a_pos = coin_control.GetSelectionPos(a->outpoint); + auto b_pos = coin_control.GetSelectionPos(b->outpoint); + if (a_pos.has_value() && b_pos.has_value()) { + return a_pos.value() < b_pos.value(); + } else if (a_pos.has_value() && !b_pos.has_value()) { + return true; + } else { + return false; + } + }); + } + // The sequence number is set to non-maxint so that DiscourageFeeSniping // works. // @@ -1149,11 +1173,32 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( // to avoid conflicting with other possible uses of nSequence, // and in the spirit of "smallest possible change from prior // behavior." - const uint32_t nSequence{coin_control.m_signal_bip125_rbf.value_or(wallet.m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL}; + bool use_anti_fee_sniping = true; + const uint32_t default_sequence{coin_control.m_signal_bip125_rbf.value_or(wallet.m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL}; for (const auto& coin : selected_coins) { - txNew.vin.emplace_back(coin->outpoint, CScript(), nSequence); + std::optional<uint32_t> sequence = coin_control.GetSequence(coin->outpoint); + if (sequence) { + // If an input has a preset sequence, we can't do anti-fee-sniping + use_anti_fee_sniping = false; + } + txNew.vin.emplace_back(coin->outpoint, CScript{}, sequence.value_or(default_sequence)); + + auto scripts = coin_control.GetScripts(coin->outpoint); + if (scripts.first) { + txNew.vin.back().scriptSig = *scripts.first; + } + if (scripts.second) { + txNew.vin.back().scriptWitness = *scripts.second; + } + } + if (coin_control.m_locktime) { + txNew.nLockTime = coin_control.m_locktime.value(); + // If we have a locktime set, we can't use anti-fee-sniping + use_anti_fee_sniping = false; + } + if (use_anti_fee_sniping) { + DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight()); } - DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight()); // Calculate the transaction fee TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control); @@ -1172,8 +1217,8 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( } // If there is a change output and we overpay the fees then increase the change to match the fee needed - if (nChangePosInOut != -1 && fee_needed < current_fee) { - auto& change = txNew.vout.at(nChangePosInOut); + if (change_pos && fee_needed < current_fee) { + auto& change = txNew.vout.at(*change_pos); change.nValue += current_fee - fee_needed; current_fee = result.GetSelectedValue() - CalculateOutputValue(txNew); if (fee_needed != current_fee) { @@ -1184,11 +1229,11 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( // Reduce output values for subtractFeeFromAmount if (coin_selection_params.m_subtract_fee_outputs) { CAmount to_reduce = fee_needed - current_fee; - int i = 0; + unsigned int i = 0; bool fFirst = true; for (const auto& recipient : vecSend) { - if (i == nChangePosInOut) { + if (change_pos && i == *change_pos) { ++i; } CTxOut& txout = txNew.vout[i]; @@ -1227,7 +1272,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( } // Give up if change keypool ran out and change is required - if (scriptChange.empty() && nChangePosInOut != -1) { + if (scriptChange.empty() && change_pos) { return util::Error{error}; } @@ -1260,6 +1305,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( // accidental reuse. reservedest.KeepDestination(); + wallet.WalletLogPrintf("Coin Selection: Algorithm:%s, Waste Metric Score:%d\n", GetAlgorithmName(result.GetAlgo()), result.GetWaste()); wallet.WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n", current_fee, nBytes, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay, feeCalc.est.pass.start, feeCalc.est.pass.end, @@ -1268,13 +1314,13 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( feeCalc.est.fail.start, feeCalc.est.fail.end, (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) > 0.0 ? 100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) : 0.0, feeCalc.est.fail.withinTarget, feeCalc.est.fail.totalConfirmed, feeCalc.est.fail.inMempool, feeCalc.est.fail.leftMempool); - return CreatedTransactionResult(tx, current_fee, nChangePosInOut, feeCalc); + return CreatedTransactionResult(tx, current_fee, change_pos, feeCalc); } util::Result<CreatedTransactionResult> CreateTransaction( CWallet& wallet, const std::vector<CRecipient>& vecSend, - int change_pos, + std::optional<unsigned int> change_pos, const CCoinControl& coin_control, bool sign) { @@ -1290,7 +1336,7 @@ util::Result<CreatedTransactionResult> CreateTransaction( auto res = CreateTransactionInternal(wallet, vecSend, change_pos, coin_control, sign); TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), bool(res), - res ? res->fee : 0, res ? res->change_pos : 0); + res ? res->fee : 0, res && res->change_pos.has_value() ? *res->change_pos : 0); if (!res) return res; const auto& txr_ungrouped = *res; // try with avoidpartialspends unless it's enabled already @@ -1300,16 +1346,15 @@ util::Result<CreatedTransactionResult> CreateTransaction( tmp_cc.m_avoid_partial_spends = true; // Reuse the change destination from the first creation attempt to avoid skipping BIP44 indexes - const int ungrouped_change_pos = txr_ungrouped.change_pos; - if (ungrouped_change_pos != -1) { - ExtractDestination(txr_ungrouped.tx->vout[ungrouped_change_pos].scriptPubKey, tmp_cc.destChange); + if (txr_ungrouped.change_pos) { + ExtractDestination(txr_ungrouped.tx->vout[*txr_ungrouped.change_pos].scriptPubKey, tmp_cc.destChange); } auto txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, tmp_cc, sign); // if fee of this alternative one is within the range of the max fee, we use this one const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped.fee + wallet.m_max_aps_fee) : false}; TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, txr_grouped.has_value(), - txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() ? txr_grouped->change_pos : 0); + txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? *txr_grouped->change_pos : 0); if (txr_grouped) { wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", txr_ungrouped.fee, txr_grouped->fee, use_aps ? "grouped" : "non-grouped"); @@ -1319,7 +1364,7 @@ util::Result<CreatedTransactionResult> CreateTransaction( return res; } -bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl) +util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional<unsigned int> change_pos, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl) { std::vector<CRecipient> vecSend; @@ -1332,6 +1377,12 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, vecSend.push_back(recipient); } + // Set the user desired locktime + coinControl.m_locktime = tx.nLockTime; + + // Set the user desired version + coinControl.m_version = tx.nVersion; + // Acquire the locks to prevent races to the new locked unspents between the // CreateTransaction call and LockCoin calls (when lockUnspents is true). LOCK(wallet.cs_wallet); @@ -1346,50 +1397,31 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, for (const CTxIn& txin : tx.vin) { const auto& outPoint = txin.prevout; - if (wallet.IsMine(outPoint)) { - // The input was found in the wallet, so select as internal - coinControl.Select(outPoint); - } else if (coins[outPoint].out.IsNull()) { - error = _("Unable to find UTXO for external input"); - return false; - } else { + PreselectedInput& preset_txin = coinControl.Select(outPoint); + if (!wallet.IsMine(outPoint)) { + if (coins[outPoint].out.IsNull()) { + return util::Error{_("Unable to find UTXO for external input")}; + } + // The input was not in the wallet, but is in the UTXO set, so select as external - coinControl.SelectExternal(outPoint, coins[outPoint].out); + preset_txin.SetTxOut(coins[outPoint].out); } + preset_txin.SetSequence(txin.nSequence); + preset_txin.SetScriptSig(txin.scriptSig); + preset_txin.SetScriptWitness(txin.scriptWitness); } - auto res = CreateTransaction(wallet, vecSend, nChangePosInOut, coinControl, false); + auto res = CreateTransaction(wallet, vecSend, change_pos, coinControl, false); if (!res) { - error = util::ErrorString(res); - return false; - } - const auto& txr = *res; - CTransactionRef tx_new = txr.tx; - nFeeRet = txr.fee; - nChangePosInOut = txr.change_pos; - - if (nChangePosInOut != -1) { - tx.vout.insert(tx.vout.begin() + nChangePosInOut, tx_new->vout[nChangePosInOut]); - } - - // Copy output sizes from new transaction; they may have had the fee - // subtracted from them. - for (unsigned int idx = 0; idx < tx.vout.size(); idx++) { - tx.vout[idx].nValue = tx_new->vout[idx].nValue; + return res; } - // Add new txins while keeping original txin scriptSig/order. - for (const CTxIn& txin : tx_new->vin) { - if (!coinControl.IsSelected(txin.prevout)) { - tx.vin.push_back(txin); - - } - if (lockUnspents) { + if (lockUnspents) { + for (const CTxIn& txin : res->tx->vin) { wallet.LockCoin(txin.prevout); } - } - return true; + return res; } } // namespace wallet diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 407627b5f1..504c078b80 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -207,9 +207,9 @@ struct CreatedTransactionResult CTransactionRef tx; CAmount fee; FeeCalculation fee_calc; - int change_pos; + std::optional<unsigned int> change_pos; - CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, int _change_pos, const FeeCalculation& _fee_calc) + CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, std::optional<unsigned int> _change_pos, const FeeCalculation& _fee_calc) : tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos) {} }; @@ -218,13 +218,13 @@ struct CreatedTransactionResult * selected by SelectCoins(); Also create the change output, when needed * @note passing change_pos as -1 will result in setting a random position */ -util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, int change_pos, const CCoinControl& coin_control, bool sign = true); +util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, std::optional<unsigned int> change_pos, const CCoinControl& coin_control, bool sign = true); /** * Insert additional inputs into the transaction by * calling CreateTransaction(); */ -bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl); +util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional<unsigned int> change_pos, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl); } // namespace wallet #endif // BITCOIN_WALLET_SPEND_H diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 9569210ba0..9fea14145f 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -320,7 +320,6 @@ 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 = NewWallet(m_node); @@ -345,6 +344,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) CoinsResult available_coins; + coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); add_coin(available_coins, *wallet, 5 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); add_coin(available_coins, *wallet, 3 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); add_coin(available_coins, *wallet, 2 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); @@ -355,7 +355,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) PreSelectedInputs selected_input; selected_input.Insert(select_coin, coin_selection_params_bnb.m_subtract_fee_outputs); available_coins.Erase({available_coins.coins[OutputType::BECH32].begin()->outpoint}); - coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); + LOCK(wallet->cs_wallet); const auto result10 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(result10); @@ -370,12 +370,14 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_selection_params_bnb.m_effective_feerate = CFeeRate(5000); coin_selection_params_bnb.m_long_term_feerate = CFeeRate(3000); - add_coin(available_coins, *wallet, 10 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); - add_coin(available_coins, *wallet, 9 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); - add_coin(available_coins, *wallet, 1 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + // Add selectable outputs, increasing their raw amounts by their input fee to make the effective value equal to the raw amount + CAmount input_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(/*num_bytes=*/68); // bech32 input size (default test output type) + add_coin(available_coins, *wallet, 10 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 9 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 1 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); expected_result.Clear(); - add_coin(10 * CENT, 2, expected_result); + add_coin(10 * CENT + input_fee, 2, expected_result); CCoinControl coin_control; const auto result11 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result11)); @@ -385,13 +387,15 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_selection_params_bnb.m_effective_feerate = CFeeRate(3000); coin_selection_params_bnb.m_long_term_feerate = CFeeRate(5000); - add_coin(available_coins, *wallet, 10 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); - add_coin(available_coins, *wallet, 9 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); - add_coin(available_coins, *wallet, 1 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + // Add selectable outputs, increasing their raw amounts by their input fee to make the effective value equal to the raw amount + input_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(/*num_bytes=*/68); // bech32 input size (default test output type) + add_coin(available_coins, *wallet, 10 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 9 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 1 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); expected_result.Clear(); - add_coin(9 * CENT, 2, expected_result); - add_coin(1 * CENT, 2, expected_result); + add_coin(9 * CENT + input_fee, 2, expected_result); + add_coin(1 * CENT + input_fee, 2, expected_result); const auto result12 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result12)); available_coins.Clear(); @@ -400,13 +404,15 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_selection_params_bnb.m_effective_feerate = CFeeRate(5000); coin_selection_params_bnb.m_long_term_feerate = CFeeRate(3000); - add_coin(available_coins, *wallet, 10 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); - add_coin(available_coins, *wallet, 9 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); - add_coin(available_coins, *wallet, 1 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + // Add selectable outputs, increasing their raw amounts by their input fee to make the effective value equal to the raw amount + input_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(/*num_bytes=*/68); // bech32 input size (default test output type) + add_coin(available_coins, *wallet, 10 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 9 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 1 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); expected_result.Clear(); - add_coin(9 * CENT, 2, expected_result); - add_coin(1 * CENT, 2, expected_result); + add_coin(9 * CENT + input_fee, 2, expected_result); + add_coin(1 * CENT + input_fee, 2, expected_result); coin_control.m_allow_other_inputs = true; COutput select_coin = available_coins.All().at(1); // pre select 9 coin coin_control.Select(select_coin.outpoint); @@ -449,6 +455,44 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) } } +BOOST_AUTO_TEST_CASE(bnb_sffo_restriction) +{ + // Verify the coin selection process does not produce a BnB solution when SFFO is enabled. + // This is currently problematic because it could require a change output. And BnB is specialized on changeless solutions. + std::unique_ptr<CWallet> wallet = NewWallet(m_node); + WITH_LOCK(wallet->cs_wallet, wallet->SetLastBlockProcessed(300, uint256{})); // set a high block so internal UTXOs are selectable + + FastRandomContext rand{}; + CoinSelectionParams params{ + rand, + /*change_output_size=*/ 31, // unused value, p2wpkh output size (wallet default change type) + /*change_spend_size=*/ 68, // unused value, p2wpkh input size (high-r signature) + /*min_change_target=*/ 0, // dummy, set later + /*effective_feerate=*/ CFeeRate(3000), + /*long_term_feerate=*/ CFeeRate(1000), + /*discard_feerate=*/ CFeeRate(1000), + /*tx_noinputs_size=*/ 0, + /*avoid_partial=*/ false, + }; + params.m_subtract_fee_outputs = true; + params.m_change_fee = params.m_effective_feerate.GetFee(params.change_output_size); + params.m_cost_of_change = params.m_discard_feerate.GetFee(params.change_spend_size) + params.m_change_fee; + params.m_min_change_target = params.m_cost_of_change + 1; + // Add spendable coin at the BnB selection upper bound + CoinsResult available_coins; + add_coin(available_coins, *wallet, COIN + params.m_cost_of_change, /*feerate=*/params.m_effective_feerate, /*nAge=*/6, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true); + add_coin(available_coins, *wallet, 0.5 * COIN + params.m_cost_of_change, /*feerate=*/params.m_effective_feerate, /*nAge=*/6, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true); + add_coin(available_coins, *wallet, 0.5 * COIN, /*feerate=*/params.m_effective_feerate, /*nAge=*/6, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true); + // Knapsack will only find a changeless solution on an exact match to the satoshi, SRD doesn’t look for changeless + // If BnB were run, it would produce a single input solution with the best waste score + auto result = WITH_LOCK(wallet->cs_wallet, return SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, COIN, /*coin_control=*/{}, params)); + BOOST_CHECK(result.has_value()); + BOOST_CHECK_NE(result->GetAlgo(), SelectionAlgorithm::BNB); + BOOST_CHECK(result->GetInputSet().size() == 2); + // We have only considered BnB, SRD, and Knapsack. Test needs to be reevaluated if new algo is added + BOOST_CHECK(result->GetAlgo() == SelectionAlgorithm::SRD || result->GetAlgo() == SelectionAlgorithm::KNAPSACK); +} + BOOST_AUTO_TEST_CASE(knapsack_solver_test) { FastRandomContext rand{}; @@ -1282,7 +1326,7 @@ BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test) cc.m_allow_other_inputs = false; COutput output = available_coins.All().at(0); cc.SetInputWeight(output.outpoint, 148); - cc.SelectExternal(output.outpoint, output.txout); + cc.Select(output.outpoint).SetTxOut(output.txout); LOCK(wallet->cs_wallet); const auto preset_inputs = *Assert(FetchSelectedInputs(*wallet, cc, cs_params)); diff --git a/src/wallet/test/fuzz/coincontrol.cpp b/src/wallet/test/fuzz/coincontrol.cpp index 0f71f28df2..f1efbc1cb8 100644 --- a/src/wallet/test/fuzz/coincontrol.cpp +++ b/src/wallet/test/fuzz/coincontrol.cpp @@ -60,7 +60,7 @@ FUZZ_TARGET(coincontrol, .init = initialize_coincontrol) }, [&] { const CTxOut tx_out{ConsumeMoney(fuzzed_data_provider), ConsumeScript(fuzzed_data_provider)}; - (void)coin_control.SelectExternal(out_point, tx_out); + (void)coin_control.Select(out_point).SetTxOut(tx_out); }, [&] { (void)coin_control.UnSelect(out_point); @@ -76,10 +76,7 @@ FUZZ_TARGET(coincontrol, .init = initialize_coincontrol) (void)coin_control.SetInputWeight(out_point, weight); }, [&] { - // Condition to avoid the assertion in GetInputWeight - if (coin_control.HasInputWeight(out_point)) { - (void)coin_control.GetInputWeight(out_point); - } + (void)coin_control.GetInputWeight(out_point); }); } } diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 4caf96b18d..ade3ec3f60 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -116,7 +116,8 @@ FUZZ_TARGET(coinselection) } // Run coinselection algorithms - auto result_bnb = SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, MAX_STANDARD_TX_WEIGHT); + auto result_bnb = coin_params.m_subtract_fee_outputs ? util::Error{Untranslated("BnB disabled when SFFO is enabled")} : + SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, MAX_STANDARD_TX_WEIGHT); if (result_bnb) { assert(result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0); assert(result_bnb->GetSelectedValue() >= target); diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 82cdd32302..203ab5f606 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -156,10 +156,9 @@ struct FuzzedWallet { coin_control.fOverrideFeeRate = fuzzed_data_provider.ConsumeBool(); // Add solving data (m_external_provider and SelectExternal)? - CAmount fee_out; int change_position{fuzzed_data_provider.ConsumeIntegralInRange<int>(-1, tx.vout.size() - 1)}; bilingual_str error; - (void)FundTransaction(*wallet, tx, fee_out, change_position, error, /*lockUnspents=*/false, subtract_fee_from_outputs, coin_control); + (void)FundTransaction(*wallet, tx, change_position, /*lockUnspents=*/false, subtract_fee_from_outputs, coin_control); } }; diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index 5926d88129..b2d252b3f9 100644 --- a/src/wallet/test/spend_tests.cpp +++ b/src/wallet/test/spend_tests.cpp @@ -28,13 +28,12 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup) // instead of the miner. auto check_tx = [&wallet](CAmount leftover_input_amount) { CRecipient recipient{PubKeyDestination({}), 50 * COIN - leftover_input_amount, /*subtract_fee=*/true}; - constexpr int RANDOM_CHANGE_POSITION = -1; CCoinControl coin_control; coin_control.m_feerate.emplace(10000); coin_control.fOverrideFeeRate = true; // We need to use a change type with high cost of change so that the leftover amount will be dropped to fee instead of added as a change output coin_control.m_change_type = OutputType::LEGACY; - auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, coin_control); + auto res = CreateTransaction(*wallet, {recipient}, std::nullopt, coin_control); BOOST_CHECK(res); const auto& txr = *res; BOOST_CHECK_EQUAL(txr.tx->vout.size(), 1); diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index ad8613d515..cbf3ccd1ec 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -24,7 +24,6 @@ std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cc LOCK2(wallet->cs_wallet, ::cs_main); wallet->SetLastBlockProcessed(cchain.Height(), cchain.Tip()->GetBlockHash()); } - wallet->LoadWallet(); { LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index dd43705a84..3d1cbe36a8 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -558,8 +558,7 @@ public: CTransactionRef tx; CCoinControl dummy; { - constexpr int RANDOM_CHANGE_POSITION = -1; - auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, dummy); + auto res = CreateTransaction(*wallet, {recipient}, std::nullopt, dummy); BOOST_CHECK(res); tx = res->tx; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c34bf96b5e..892b9d51e4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2292,6 +2292,8 @@ DBErrors CWallet::LoadWallet() { LOCK(cs_wallet); + Assert(m_spk_managers.empty()); + Assert(m_wallet_flags == 0); DBErrors nLoadWalletRet = WalletBatch(GetDatabase()).LoadWallet(this); if (nLoadWalletRet == DBErrors::NEED_REWRITE) { @@ -3535,6 +3537,10 @@ void CWallet::SetupDescriptorScriptPubKeyMans(const CExtKey& master_key) { AssertLockHeld(cs_wallet); + // Create single batch txn + WalletBatch batch(GetDatabase()); + if (!batch.TxnBegin()) throw std::runtime_error("Error: cannot create db transaction for descriptors setup"); + for (bool internal : {false, true}) { for (OutputType t : OUTPUT_TYPES) { auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, m_keypool_size)); @@ -3542,16 +3548,19 @@ void CWallet::SetupDescriptorScriptPubKeyMans(const CExtKey& master_key) if (IsLocked()) { throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors"); } - if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, nullptr)) { + if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, &batch)) { throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors"); } } - spk_manager->SetupDescriptorGeneration(master_key, t, internal); + spk_manager->SetupDescriptorGeneration(batch, master_key, t, internal); uint256 id = spk_manager->GetID(); AddScriptPubKeyMan(id, std::move(spk_manager)); - AddActiveScriptPubKeyMan(id, t, internal); + AddActiveScriptPubKeyManWithDb(batch, id, t, internal); } } + + // Ensure information is committed to disk + if (!batch.TxnCommit()) throw std::runtime_error("Error: cannot commit db transaction for descriptors setup"); } void CWallet::SetupDescriptorScriptPubKeyMans() @@ -3578,6 +3587,10 @@ void CWallet::SetupDescriptorScriptPubKeyMans() UniValue signer_res = signer.GetDescriptors(account); if (!signer_res.isObject()) throw std::runtime_error(std::string(__func__) + ": Unexpected result"); + + WalletBatch batch(GetDatabase()); + if (!batch.TxnBegin()) throw std::runtime_error("Error: cannot create db transaction for descriptors import"); + for (bool internal : {false, true}) { const UniValue& descriptor_vals = signer_res.find_value(internal ? "internal" : "receive"); if (!descriptor_vals.isArray()) throw std::runtime_error(std::string(__func__) + ": Unexpected result"); @@ -3594,18 +3607,26 @@ void CWallet::SetupDescriptorScriptPubKeyMans() } OutputType t = *desc->GetOutputType(); auto spk_manager = std::unique_ptr<ExternalSignerScriptPubKeyMan>(new ExternalSignerScriptPubKeyMan(*this, m_keypool_size)); - spk_manager->SetupDescriptor(std::move(desc)); + spk_manager->SetupDescriptor(batch, std::move(desc)); uint256 id = spk_manager->GetID(); AddScriptPubKeyMan(id, std::move(spk_manager)); - AddActiveScriptPubKeyMan(id, t, internal); + AddActiveScriptPubKeyManWithDb(batch, id, t, internal); } } + + // Ensure imported descriptors are committed to disk + if (!batch.TxnCommit()) throw std::runtime_error("Error: cannot commit db transaction for descriptors import"); } } void CWallet::AddActiveScriptPubKeyMan(uint256 id, OutputType type, bool internal) { WalletBatch batch(GetDatabase()); + return AddActiveScriptPubKeyManWithDb(batch, id, type, internal); +} + +void CWallet::AddActiveScriptPubKeyManWithDb(WalletBatch& batch, uint256 id, OutputType type, bool internal) +{ if (!batch.WriteActiveScriptPubKeyMan(static_cast<uint8_t>(type), id, internal)) { throw std::runtime_error(std::string(__func__) + ": writing active ScriptPubKeyMan id failed"); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index bc45010200..11b7964316 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -419,6 +419,9 @@ private: // Must be the only method adding data to it. void AddScriptPubKeyMan(const uint256& id, std::unique_ptr<ScriptPubKeyMan> spkm_man); + // Same as 'AddActiveScriptPubKeyMan' but designed for use within a batch transaction context + void AddActiveScriptPubKeyManWithDb(WalletBatch& batch, uint256 id, OutputType type, bool internal); + /** * Catch wallet up to current chain, scanning new blocks, updating the best * block locator and m_last_block_processed, and registering for diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 96999de97b..ba453b47e7 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1499,17 +1499,19 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas if (format == DatabaseFormat::SQLITE) { #ifdef USE_SQLITE return MakeSQLiteDatabase(path, options, status, error); -#endif +#else error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path))); status = DatabaseStatus::FAILED_BAD_FORMAT; return nullptr; +#endif } #ifdef USE_BDB return MakeBerkeleyDatabase(path, options, status, error); -#endif +#else error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path))); status = DatabaseStatus::FAILED_BAD_FORMAT; return nullptr; +#endif } } // namespace wallet diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 03aae86577..63c2737706 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -6,6 +6,7 @@ #include <common/args.h> #include <kernel/chain.h> +#include <kernel/mempool_entry.h> #include <logging.h> #include <primitives/block.h> #include <primitives/transaction.h> @@ -152,9 +153,9 @@ void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, co }); } -void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx, uint64_t mempool_sequence) +void CZMQNotificationInterface::TransactionAddedToMempool(const NewMempoolTransactionInfo& ptx, uint64_t mempool_sequence) { - const CTransaction& tx = *ptx; + const CTransaction& tx = *(ptx.info.m_tx); TryForEachAndRemoveFailed(notifiers, [&tx, mempool_sequence](CZMQAbstractNotifier* notifier) { return notifier->NotifyTransaction(tx) && notifier->NotifyTransactionAcceptance(tx, mempool_sequence); diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 4246c53bd3..45d0982bd3 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -16,6 +16,7 @@ class CBlock; class CBlockIndex; class CZMQAbstractNotifier; +struct NewMempoolTransactionInfo; class CZMQNotificationInterface final : public CValidationInterface { @@ -31,7 +32,7 @@ protected: void Shutdown(); // CValidationInterface - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) override; + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) override; void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override; void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected) override; void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected) override; diff --git a/test/functional/feature_asmap.py b/test/functional/feature_asmap.py index 9cff8042a8..ae483fe449 100755 --- a/test/functional/feature_asmap.py +++ b/test/functional/feature_asmap.py @@ -111,6 +111,14 @@ class AsmapTest(BitcoinTestFramework): self.node.assert_start_raises_init_error(extra_args=['-asmap'], expected_msg=msg) os.remove(self.default_asmap) + def test_asmap_health_check(self): + self.log.info('Test bitcoind -asmap logs ASMap Health Check with basic stats') + shutil.copyfile(self.asmap_raw, self.default_asmap) + msg = "ASMap Health Check: 2 clearnet peers are mapped to 1 ASNs with 0 peers being unmapped" + with self.node.assert_debug_log(expected_msgs=[msg]): + self.start_node(0, extra_args=['-asmap']) + os.remove(self.default_asmap) + def run_test(self): self.node = self.nodes[0] self.datadir = self.node.chain_path @@ -124,6 +132,7 @@ class AsmapTest(BitcoinTestFramework): self.test_asmap_interaction_with_addrman_containing_entries() self.test_default_asmap_with_missing_file() self.test_empty_asmap() + self.test_asmap_health_check() if __name__ == '__main__': diff --git a/test/functional/feature_filelock.py b/test/functional/feature_filelock.py index 24a68a04bf..567207915e 100755 --- a/test/functional/feature_filelock.py +++ b/test/functional/feature_filelock.py @@ -30,6 +30,12 @@ class FilelockTest(BitcoinTestFramework): expected_msg = f"Error: Cannot obtain a lock on data directory {datadir}. {self.config['environment']['PACKAGE_NAME']} is probably already running." self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir_path}', '-noserver'], expected_msg=expected_msg) + self.log.info("Check that cookie and PID file are not deleted when attempting to start a second bitcoind using the same datadir") + cookie_file = datadir / ".cookie" + assert cookie_file.exists() # should not be deleted during the second bitcoind instance shutdown + pid_file = datadir / "bitcoind.pid" + assert pid_file.exists() + if self.is_wallet_compiled(): def check_wallet_filelock(descriptors): wallet_name = ''.join([random.choice(string.ascii_lowercase) for _ in range(6)]) diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index a1147f70f3..6215610c31 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -125,8 +125,9 @@ class MempoolLimitTest(BitcoinTestFramework): utxo_to_spend=tx_B["new_utxo"], confirmed_only=True ) - - assert_raises_rpc_error(-26, "too-long-mempool-chain", node.submitpackage, [tx_B["hex"], tx_C["hex"]]) + res = node.submitpackage([tx_B["hex"], tx_C["hex"]]) + assert_equal(res["package_msg"], "transaction failed") + assert "too-long-mempool-chain" in res["tx-results"][tx_C["wtxid"]]["error"] def test_mid_package_eviction(self): node = self.nodes[0] @@ -205,7 +206,7 @@ class MempoolLimitTest(BitcoinTestFramework): # Package should be submitted, temporarily exceeding maxmempool, and then evicted. with node.assert_debug_log(expected_msgs=["rolling minimum fee bumped"]): - assert_raises_rpc_error(-26, "mempool full", node.submitpackage, package_hex) + assert_equal(node.submitpackage(package_hex)["package_msg"], "transaction failed") # Maximum size must never be exceeded. assert_greater_than(node.getmempoolinfo()["maxmempool"], node.getmempoolinfo()["bytes"]) @@ -273,7 +274,9 @@ class MempoolLimitTest(BitcoinTestFramework): package_hex = [cpfp_parent["hex"], replacement_tx["hex"], child["hex"]] # Package should be submitted, temporarily exceeding maxmempool, and then evicted. - assert_raises_rpc_error(-26, "bad-txns-inputs-missingorspent", node.submitpackage, package_hex) + res = node.submitpackage(package_hex) + assert_equal(res["package_msg"], "transaction failed") + assert len([tx_res for _, tx_res in res["tx-results"].items() if "error" in tx_res and tx_res["error"] == "bad-txns-inputs-missingorspent"]) # Maximum size must never be exceeded. assert_greater_than(node.getmempoolinfo()["maxmempool"], node.getmempoolinfo()["bytes"]) @@ -321,6 +324,7 @@ class MempoolLimitTest(BitcoinTestFramework): package_txns.append(tx_child) submitpackage_result = node.submitpackage([tx["hex"] for tx in package_txns]) + assert_equal(submitpackage_result["package_msg"], "success") rich_parent_result = submitpackage_result["tx-results"][tx_rich["wtxid"]] poor_parent_result = submitpackage_result["tx-results"][tx_poor["wtxid"]] @@ -366,7 +370,9 @@ class MempoolLimitTest(BitcoinTestFramework): assert_greater_than(worst_feerate_btcvb, (parent_fee + child_fee) / (tx_parent_just_below["tx"].get_vsize() + tx_child_just_above["tx"].get_vsize())) assert_greater_than(mempoolmin_feerate, (parent_fee) / (tx_parent_just_below["tx"].get_vsize())) assert_greater_than((parent_fee + child_fee) / (tx_parent_just_below["tx"].get_vsize() + tx_child_just_above["tx"].get_vsize()), mempoolmin_feerate / 1000) - assert_raises_rpc_error(-26, "mempool full", node.submitpackage, [tx_parent_just_below["hex"], tx_child_just_above["hex"]]) + res = node.submitpackage([tx_parent_just_below["hex"], tx_child_just_above["hex"]]) + for wtxid in [tx_parent_just_below["wtxid"], tx_child_just_above["wtxid"]]: + assert_equal(res["tx-results"][wtxid]["error"], "mempool full") self.log.info('Test passing a value below the minimum (5 MB) to -maxmempool throws an error') self.stop_node(0) diff --git a/test/functional/mempool_sigoplimit.py b/test/functional/mempool_sigoplimit.py index fbec6d0dc8..14dd9cc80e 100755 --- a/test/functional/mempool_sigoplimit.py +++ b/test/functional/mempool_sigoplimit.py @@ -34,7 +34,6 @@ from test_framework.util import ( assert_equal, assert_greater_than, assert_greater_than_or_equal, - assert_raises_rpc_error, ) from test_framework.wallet import MiniWallet from test_framework.wallet_util import generate_keypair @@ -169,7 +168,8 @@ class BytesPerSigOpTest(BitcoinTestFramework): assert_equal([x["package-error"] for x in packet_test], ["package-mempool-limits", "package-mempool-limits"]) # When we actually try to submit, the parent makes it into the mempool, but the child would exceed ancestor vsize limits - assert_raises_rpc_error(-26, "too-long-mempool-chain", self.nodes[0].submitpackage, [tx_parent.serialize().hex(), tx_child.serialize().hex()]) + res = self.nodes[0].submitpackage([tx_parent.serialize().hex(), tx_child.serialize().hex()]) + assert "too-long-mempool-chain" in res["tx-results"][tx_child.getwtxid()]["error"] assert tx_parent.rehash() in self.nodes[0].getrawmempool() # Transactions are tiny in weight diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py index 665f57365f..62d55cc101 100755 --- a/test/functional/p2p_filter.py +++ b/test/functional/p2p_filter.py @@ -11,6 +11,7 @@ from test_framework.messages import ( COIN, MAX_BLOOM_FILTER_SIZE, MAX_BLOOM_HASH_FUNCS, + MSG_WTX, MSG_BLOCK, MSG_FILTERED_BLOCK, msg_filteradd, @@ -135,14 +136,22 @@ class FilterTest(BitcoinTestFramework): self.log.info("Check that a node with bloom filters enabled services p2p mempool messages") filter_peer = P2PBloomFilter() - self.log.debug("Create a tx relevant to the peer before connecting") - txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=filter_peer.watch_script_pubkey, amount=9 * COIN)["txid"] + self.log.info("Create two tx before connecting, one relevant to the node another that is not") + rel_txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=filter_peer.watch_script_pubkey, amount=1 * COIN)["txid"] + irr_result = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=2 * COIN) + irr_txid = irr_result["txid"] + irr_wtxid = irr_result["wtxid"] - self.log.debug("Send a mempool msg after connecting and check that the tx is received") + self.log.info("Send a mempool msg after connecting and check that the relevant tx is announced") self.nodes[0].add_p2p_connection(filter_peer) filter_peer.send_and_ping(filter_peer.watch_filter_init) filter_peer.send_message(msg_mempool()) - filter_peer.wait_for_tx(txid) + filter_peer.wait_for_tx(rel_txid) + + self.log.info("Request the irrelevant transaction even though it was not announced") + filter_peer.send_message(msg_getdata([CInv(t=MSG_WTX, h=int(irr_wtxid, 16))])) + self.log.info("We should get it anyway because it was in the mempool on connection to peer") + filter_peer.wait_for_tx(irr_txid) def test_frelay_false(self, filter_peer): self.log.info("Check that a node with fRelay set to false does not receive invs until the filter is set") diff --git a/test/functional/p2p_v2_transport.py b/test/functional/p2p_v2_transport.py index 1a3b4a6d0a..72d22cb77f 100755 --- a/test/functional/p2p_v2_transport.py +++ b/test/functional/p2p_v2_transport.py @@ -133,9 +133,8 @@ class V2TransportTest(BitcoinTestFramework): V1_PREFIX = MAGIC_BYTES["regtest"] + b"version\x00\x00\x00\x00\x00" assert_equal(len(V1_PREFIX), 16) with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - num_peers = len(self.nodes[0].getpeerinfo()) - s.connect(("127.0.0.1", p2p_port(0))) - self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == num_peers + 1) + with self.nodes[0].wait_for_new_peer(): + s.connect(("127.0.0.1", p2p_port(0))) s.sendall(V1_PREFIX[:-1]) assert_equal(self.nodes[0].getpeerinfo()[-1]["transport_protocol_type"], "detecting") s.sendall(bytes([V1_PREFIX[-1]])) # send out last prefix byte @@ -144,22 +143,23 @@ class V2TransportTest(BitcoinTestFramework): # Check wrong network prefix detection (hits if the next 12 bytes correspond to a v1 version message) wrong_network_magic_prefix = MAGIC_BYTES["signet"] + V1_PREFIX[4:] with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - s.connect(("127.0.0.1", p2p_port(0))) + with self.nodes[0].wait_for_new_peer(): + s.connect(("127.0.0.1", p2p_port(0))) with self.nodes[0].assert_debug_log(["V2 transport error: V1 peer with wrong MessageStart"]): s.sendall(wrong_network_magic_prefix + b"somepayload") # Check detection of missing garbage terminator (hits after fixed amount of data if terminator never matches garbage) MAX_KEY_GARB_AND_GARBTERM_LEN = 64 + 4095 + 16 with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - num_peers = len(self.nodes[0].getpeerinfo()) - s.connect(("127.0.0.1", p2p_port(0))) - self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == num_peers + 1) + with self.nodes[0].wait_for_new_peer(): + s.connect(("127.0.0.1", p2p_port(0))) s.sendall(b'\x00' * (MAX_KEY_GARB_AND_GARBTERM_LEN - 1)) self.wait_until(lambda: self.nodes[0].getpeerinfo()[-1]["bytesrecv"] == MAX_KEY_GARB_AND_GARBTERM_LEN - 1) with self.nodes[0].assert_debug_log(["V2 transport error: missing garbage terminator"]): + peer_id = self.nodes[0].getpeerinfo()[-1]["id"] s.sendall(b'\x00') # send out last byte # should disconnect immediately - self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == num_peers) + self.wait_until(lambda: not peer_id in [p["id"] for p in self.nodes[0].getpeerinfo()]) if __name__ == '__main__': diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index b193ffd462..e1820b0f55 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -9,6 +9,7 @@ Tests correspond to code in rpc/net.cpp. from decimal import Decimal from itertools import product +import platform import time import test_framework.messages @@ -110,7 +111,7 @@ class NetTest(BitcoinTestFramework): no_version_peer_id = 2 no_version_peer_conntime = int(time.time()) self.nodes[0].setmocktime(no_version_peer_conntime) - with self.nodes[0].assert_debug_log([f"Added connection peer={no_version_peer_id}"]): + with self.nodes[0].wait_for_new_peer(): no_version_peer = self.nodes[0].add_p2p_connection(P2PInterface(), send_version=False, wait_for_verack=False) self.nodes[0].setmocktime(0) peer_info = self.nodes[0].getpeerinfo()[no_version_peer_id] @@ -220,8 +221,10 @@ class NetTest(BitcoinTestFramework): ip_port = "127.0.0.1:{}".format(p2p_port(2)) self.nodes[0].addnode(node=ip_port, command='add') # try to add an equivalent ip - ip_port2 = "127.1:{}".format(p2p_port(2)) - assert_raises_rpc_error(-23, "Node already added", self.nodes[0].addnode, node=ip_port2, command='add') + # (note that OpenBSD doesn't support the IPv4 shorthand notation with omitted zero-bytes) + if platform.system() != "OpenBSD": + ip_port2 = "127.1:{}".format(p2p_port(2)) + assert_raises_rpc_error(-23, "Node already added", self.nodes[0].addnode, node=ip_port2, command='add') # check that the node has indeed been added added_nodes = self.nodes[0].getaddednodeinfo() assert_equal(len(added_nodes), 1) diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index 5644a9f5a8..664f2df3f1 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -304,6 +304,7 @@ class RPCPackagesTest(BitcoinTestFramework): submitpackage_result = node.submitpackage(package=[tx["hex"] for tx in package_txns]) # Check that each result is present, with the correct size and fees + assert_equal(submitpackage_result["package_msg"], "success") for package_txn in package_txns: tx = package_txn["tx"] assert tx.getwtxid() in submitpackage_result["tx-results"] @@ -334,9 +335,26 @@ class RPCPackagesTest(BitcoinTestFramework): self.log.info("Submitpackage only allows packages of 1 child with its parents") # Chain of 3 transactions has too many generations - chain_hex = [t["hex"] for t in self.wallet.create_self_transfer_chain(chain_length=25)] + legacy_pool = node.getrawmempool() + chain_hex = [t["hex"] for t in self.wallet.create_self_transfer_chain(chain_length=3)] assert_raises_rpc_error(-25, "package topology disallowed", node.submitpackage, chain_hex) + assert_equal(legacy_pool, node.getrawmempool()) + # Create a transaction chain such as only the parent gets accepted (by making the child's + # version non-standard). Make sure the parent does get broadcast. + self.log.info("If a package is partially submitted, transactions included in mempool get broadcast") + peer = node.add_p2p_connection(P2PTxInvStore()) + txs = self.wallet.create_self_transfer_chain(chain_length=2) + bad_child = tx_from_hex(txs[1]["hex"]) + bad_child.nVersion = -1 + hex_partial_acceptance = [txs[0]["hex"], bad_child.serialize().hex()] + res = node.submitpackage(hex_partial_acceptance) + assert_equal(res["package_msg"], "transaction failed") + first_wtxid = txs[0]["tx"].getwtxid() + assert "error" not in res["tx-results"][first_wtxid] + sec_wtxid = bad_child.getwtxid() + assert_equal(res["tx-results"][sec_wtxid]["error"], "version") + peer.wait_for_broadcast([first_wtxid]) if __name__ == "__main__": RPCPackagesTest().main() diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py index 2395935620..c12865b5e3 100755 --- a/test/functional/rpc_rawtransaction.py +++ b/test/functional/rpc_rawtransaction.py @@ -32,6 +32,7 @@ from test_framework.script import ( from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, + assert_greater_than, assert_raises_rpc_error, ) from test_framework.wallet import ( @@ -70,7 +71,7 @@ class RawTransactionsTest(BitcoinTestFramework): self.extra_args = [ ["-txindex"], ["-txindex"], - [], + ["-fastprune", "-prune=1"], ] # whitelist all peers to speed up tx relay / mempool sync for args in self.extra_args: @@ -85,7 +86,6 @@ class RawTransactionsTest(BitcoinTestFramework): self.wallet = MiniWallet(self.nodes[0]) self.getrawtransaction_tests() - self.getrawtransaction_verbosity_tests() self.createrawtransaction_tests() self.sendrawtransaction_tests() self.sendrawtransaction_testmempoolaccept_tests() @@ -94,6 +94,8 @@ class RawTransactionsTest(BitcoinTestFramework): if self.is_specified_wallet_compiled() and not self.options.descriptors: self.import_deterministic_coinbase_privkeys() self.raw_multisig_transaction_legacy_tests() + self.getrawtransaction_verbosity_tests() + def getrawtransaction_tests(self): tx = self.wallet.send_self_transfer(from_node=self.nodes[0]) @@ -243,6 +245,13 @@ class RawTransactionsTest(BitcoinTestFramework): 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 + # check that verbosity 2 for a mempool tx will fallback to verbosity 1 + # Do this with a pruned chain, as a regression test for https://github.com/bitcoin/bitcoin/pull/29003 + self.generate(self.nodes[2], 400) + assert_greater_than(self.nodes[2].pruneblockchain(250), 0) + mempool_tx = self.wallet.send_self_transfer(from_node=self.nodes[2])['txid'] + gottx = self.nodes[2].getrawtransaction(txid=mempool_tx, verbosity=2) + assert 'fee' not in gottx def createrawtransaction_tests(self): self.log.info("Test createrawtransaction") diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 850aa20db2..90c1213deb 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -520,6 +520,24 @@ class TestNode(): str(expected_msgs), print_log)) @contextlib.contextmanager + def wait_for_new_peer(self, timeout=5): + """ + Wait until the node is connected to at least one new peer. We detect this + by watching for an increased highest peer id, using the `getpeerinfo` RPC call. + Note that the simpler approach of only accounting for the number of peers + suffers from race conditions, as disconnects from unrelated previous peers + could happen anytime in-between. + """ + def get_highest_peer_id(): + peer_info = self.getpeerinfo() + return peer_info[-1]["id"] if peer_info else -1 + + initial_peer_id = get_highest_peer_id() + yield + wait_until_helper_internal(lambda: get_highest_peer_id() > initial_peer_id, + timeout=timeout, timeout_factor=self.timeout_factor) + + @contextlib.contextmanager def profile_with_perf(self, profile_name: str): """ Context manager that allows easy profiling of node activity using `perf`. diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 6016a482f8..bf52ba6d93 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -565,8 +565,7 @@ def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, enable_coverage= test_framework_tests.addTest(unittest.TestLoader().loadTestsFromName("test_framework.{}".format(module))) result = unittest.TextTestRunner(verbosity=1, failfast=True).run(test_framework_tests) if not result.wasSuccessful(): - logging.debug("Early exiting after failure in TestFramework unit tests") - sys.exit(False) + sys.exit("Early exiting after failure in TestFramework unit tests") flags = ['--cachedir={}'.format(cache_dir)] + args |