diff options
34 files changed, 464 insertions, 184 deletions
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c1e171c0e3..928ab3db0e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -127,8 +127,8 @@ jobs: CCACHE_MAXSIZE: '200M' CI_CCACHE_VERSION: '4.7.5' CI_QT_CONF: '-release -silent -opensource -confirm-license -opengl desktop -static -static-runtime -mp -qt-zlib -qt-pcre -qt-libpng -nomake examples -nomake tests -nomake tools -no-angle -no-dbus -no-gif -no-gtk -no-ico -no-icu -no-libjpeg -no-libudev -no-sql-sqlite -no-sql-odbc -no-sqlite -no-vulkan -skip qt3d -skip qtactiveqt -skip qtandroidextras -skip qtcharts -skip qtconnectivity -skip qtdatavis3d -skip qtdeclarative -skip doc -skip qtdoc -skip qtgamepad -skip qtgraphicaleffects -skip qtimageformats -skip qtlocation -skip qtlottie -skip qtmacextras -skip qtmultimedia -skip qtnetworkauth -skip qtpurchasing -skip qtquick3d -skip qtquickcontrols -skip qtquickcontrols2 -skip qtquicktimeline -skip qtremoteobjects -skip qtscript -skip qtscxml -skip qtsensors -skip qtserialbus -skip qtserialport -skip qtspeech -skip qtsvg -skip qtvirtualkeyboard -skip qtwayland -skip qtwebchannel -skip qtwebengine -skip qtwebglplugin -skip qtwebsockets -skip qtwebview -skip qtx11extras -skip qtxmlpatterns -no-openssl -no-feature-bearermanagement -no-feature-printdialog -no-feature-printer -no-feature-printpreviewdialog -no-feature-printpreviewwidget -no-feature-sql -no-feature-sqlmodel -no-feature-textbrowser -no-feature-textmarkdownwriter -no-feature-textodfwriter -no-feature-xml' - CI_QT_DIR: 'qt-everywhere-src-5.15.10' - CI_QT_URL: 'https://download.qt.io/official_releases/qt/5.15/5.15.10/single/qt-everywhere-opensource-src-5.15.10.zip' + CI_QT_DIR: 'qt-everywhere-src-5.15.11' + CI_QT_URL: 'https://download.qt.io/official_releases/qt/5.15/5.15.11/single/qt-everywhere-opensource-src-5.15.11.zip' PYTHONUTF8: 1 TEST_RUNNER_TIMEOUT_FACTOR: 40 diff --git a/build_msvc/README.md b/build_msvc/README.md index 8206620c3b..cc2cd91e13 100644 --- a/build_msvc/README.md +++ b/build_msvc/README.md @@ -32,7 +32,7 @@ Qt --------------------- 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.10.zip`](https://download.qt.io/official_releases/qt/5.15/5.15.10/single/qt-everywhere-opensource-src-5.15.10.zip)), and expand it into a dedicated folder. The following instructions assume that this folder is `C:\dev\qt-source`. +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`. 2. Open "x64 Native Tools Command Prompt for VS 2022", and input the following commands: ```cmd diff --git a/configure.ac b/configure.ac index 6dfd2abfb2..6add570d00 100644 --- a/configure.ac +++ b/configure.ac @@ -1424,15 +1424,13 @@ dnl Check for libminiupnpc (optional) if test "$use_upnp" != "no"; then TEMP_CPPFLAGS="$CPPFLAGS" CPPFLAGS="$CPPFLAGS $MINIUPNPC_CPPFLAGS" - AC_CHECK_HEADERS( - [miniupnpc/miniupnpc.h miniupnpc/upnpcommands.h miniupnpc/upnperrors.h], - [AC_CHECK_LIB([miniupnpc], [upnpDiscover], [MINIUPNPC_LIBS="$MINIUPNPC_LIBS -lminiupnpc"], [have_miniupnpc=no], [$MINIUPNPC_LIBS])], - [have_miniupnpc=no] - ) + AC_CHECK_HEADERS([miniupnpc/miniupnpc.h miniupnpc/upnpcommands.h miniupnpc/upnperrors.h], [], [have_miniupnpc=no]) - dnl The minimum supported miniUPnPc API version is set to 17. This excludes - dnl versions with known vulnerabilities. if test "$have_miniupnpc" != "no"; then + AC_CHECK_LIB([miniupnpc], [upnpDiscover], [MINIUPNPC_LIBS="$MINIUPNPC_LIBS -lminiupnpc"], [have_miniupnpc=no], [$MINIUPNPC_LIBS]) + + dnl The minimum supported miniUPnPc API version is set to 17. This excludes + dnl versions with known vulnerabilities. AC_MSG_CHECKING([whether miniUPnPc API version is supported]) AC_PREPROC_IFELSE([AC_LANG_PROGRAM([[ @%:@include <miniupnpc/miniupnpc.h> @@ -1457,9 +1455,12 @@ dnl Check for libnatpmp (optional). if test "$use_natpmp" != "no"; then TEMP_CPPFLAGS="$CPPFLAGS" CPPFLAGS="$CPPFLAGS $NATPMP_CPPFLAGS" - AC_CHECK_HEADERS([natpmp.h], - [AC_CHECK_LIB([natpmp], [initnatpmp], [NATPMP_LIBS="$NATPMP_LIBS -lnatpmp"], [have_natpmp=no], [$NATPMP_LIBS])], - [have_natpmp=no]) + AC_CHECK_HEADERS([natpmp.h], [], [have_natpmp=no]) + + if test "$have_natpmp" != "no"; then + AC_CHECK_LIB([natpmp], [initnatpmp], [NATPMP_LIBS="$NATPMP_LIBS -lnatpmp"], [have_natpmp=no], [$NATPMP_LIBS]) + fi + CPPFLAGS="$TEMP_CPPFLAGS" fi diff --git a/contrib/guix/libexec/codesign.sh b/contrib/guix/libexec/codesign.sh index 0b5f77d01e..54edfecb26 100755 --- a/contrib/guix/libexec/codesign.sh +++ b/contrib/guix/libexec/codesign.sh @@ -86,7 +86,11 @@ mkdir -p "$DISTSRC" signapple apply dist/Bitcoin-Qt.app codesignatures/osx/dist # Make a .zip from dist/ - zip "${OUTDIR}/${DISTNAME}-${HOST}.zip" dist/* + cd dist/ + find . -print0 \ + | xargs -0r touch --no-dereference --date="@${SOURCE_DATE_EPOCH}" + find . | sort \ + | zip -X@ "${OUTDIR}/${DISTNAME}-${HOST}.zip" ;; *) exit 1 diff --git a/contrib/guix/manifest.scm b/contrib/guix/manifest.scm index 3c4ad6cdbc..12f57fcc45 100644 --- a/contrib/guix/manifest.scm +++ b/contrib/guix/manifest.scm @@ -19,7 +19,6 @@ ((gnu packages python) #:select (python-minimal)) ((gnu packages python-build) #:select (python-tomli)) ((gnu packages python-crypto) #:select (python-asn1crypto)) - ((gnu packages python-web) #:select (python-requests)) ((gnu packages tls) #:select (openssl)) ((gnu packages version-control) #:select (git-minimal)) (guix build-system cmake) @@ -445,7 +444,7 @@ and endian independent.") (license license:expat))) (define-public python-signapple - (let ((commit "8a945a2e7583be2665cf3a6a89d665b70ecd1ab6")) + (let ((commit "7a96b4171a360abf0f0f56e499f8f9ed2116280d")) (package (name "python-signapple") (version (git-version "0.1" "1" commit)) @@ -458,14 +457,13 @@ and endian independent.") (file-name (git-file-name name commit)) (sha256 (base32 - "0fr1hangvfyiwflca6jg5g8zvg3jc9qr7vd2c12ff89pznf38dlg")))) + "0aa4k180jnpal15yhncnm3g3z9gzmi7qb25q5l0kaj444a1p2pm4")))) (build-system python-build-system) (propagated-inputs `(("python-asn1crypto" ,python-asn1crypto) ("python-oscrypto" ,python-oscrypto) ("python-certvalidator" ,python-certvalidator) ("python-elfesteem" ,python-elfesteem) - ("python-requests" ,python-requests) ("python-macholib" ,python-macholib))) ;; There are no tests, but attempting to run python setup.py test leads to ;; problems, just disable the test diff --git a/depends/packages/qt.mk b/depends/packages/qt.mk index 047d1d5aee..e42d7bbef9 100644 --- a/depends/packages/qt.mk +++ b/depends/packages/qt.mk @@ -1,9 +1,9 @@ package=qt -$(package)_version=5.15.10 +$(package)_version=5.15.11 $(package)_download_path=https://download.qt.io/official_releases/qt/5.15/$($(package)_version)/submodules $(package)_suffix=everywhere-opensource-src-$($(package)_version).tar.xz $(package)_file_name=qtbase-$($(package)_suffix) -$(package)_sha256_hash=c0d06cb18d20f10bf7ad53552099e097ec39362d30a5d6f104724f55fa1c8fb9 +$(package)_sha256_hash=425ad301acd91ca66c10c0dabee0704e2d0cd2801a6b670115800cbb95f84846 $(package)_linux_dependencies=freetype fontconfig libxcb libxkbcommon libxcb_util libxcb_util_render libxcb_util_keysyms libxcb_util_image libxcb_util_wm $(package)_qt_libs=corelib network widgets gui plugins testlib $(package)_linguist_tools = lrelease lupdate lconvert @@ -25,10 +25,10 @@ $(package)_patches += memory_resource.patch $(package)_patches += windows_lto.patch $(package)_qttranslations_file_name=qttranslations-$($(package)_suffix) -$(package)_qttranslations_sha256_hash=38b942bc7e62794dd072945c8a92bb9dfffed24070aea300327a3bb42f855609 +$(package)_qttranslations_sha256_hash=a31785948c640b7c66d9fe2db4993728ca07f64e41c560b3625ad191b276ff20 $(package)_qttools_file_name=qttools-$($(package)_suffix) -$(package)_qttools_sha256_hash=66f46c9729c831dce431778a9c561cca32daceaede1c7e58568d7a5898167dae +$(package)_qttools_sha256_hash=7cd847ae6ff09416df617136eadcaf0eb98e3bc9b89979219a3ea8111fb8d339 $(package)_extra_sources = $($(package)_qttranslations_file_name) $(package)_extra_sources += $($(package)_qttools_file_name) diff --git a/depends/patches/qt/fix_android_jni_static.patch b/depends/patches/qt/fix_android_jni_static.patch index 7dbd68fe9c..89c96026fb 100644 --- a/depends/patches/qt/fix_android_jni_static.patch +++ b/depends/patches/qt/fix_android_jni_static.patch @@ -1,6 +1,6 @@ --- old/qtbase/src/plugins/platforms/android/androidjnimain.cpp +++ new/qtbase/src/plugins/platforms/android/androidjnimain.cpp -@@ -980,6 +980,14 @@ Q_DECL_EXPORT jint JNICALL JNI_OnLoad(JavaVM *vm, void */*reserved*/) +@@ -979,6 +979,14 @@ Q_DECL_EXPORT jint JNICALL JNI_OnLoad(JavaVM *vm, void */*reserved*/) __android_log_print(ANDROID_LOG_FATAL, "Qt", "registerNatives failed"); return -1; } diff --git a/doc/dependencies.md b/doc/dependencies.md index 9d9380bf79..41079b93ab 100644 --- a/doc/dependencies.md +++ b/doc/dependencies.md @@ -30,7 +30,7 @@ You can find installation instructions in the `build-*.md` file for your platfor | [Fontconfig](../depends/packages/fontconfig.mk) | [link](https://www.freedesktop.org/wiki/Software/fontconfig/) | [2.12.6](https://github.com/bitcoin/bitcoin/pull/23495) | 2.6 | Yes | | [FreeType](../depends/packages/freetype.mk) | [link](https://freetype.org) | [2.11.0](https://github.com/bitcoin/bitcoin/commit/01544dd78ccc0b0474571da854e27adef97137fb) | 2.3.0 | Yes | | [qrencode](../depends/packages/qrencode.mk) | [link](https://fukuchi.org/works/qrencode/) | [4.1.1](https://github.com/bitcoin/bitcoin/pull/27312) | | No | -| [Qt](../depends/packages/qt.mk) | [link](https://download.qt.io/official_releases/qt/) | [5.15.10](https://github.com/bitcoin/bitcoin/pull/28561) | [5.11.3](https://github.com/bitcoin/bitcoin/pull/24132) | No | +| [Qt](../depends/packages/qt.mk) | [link](https://download.qt.io/official_releases/qt/) | [5.15.11](https://github.com/bitcoin/bitcoin/pull/28769) | [5.11.3](https://github.com/bitcoin/bitcoin/pull/24132) | No | ### Networking | Dependency | Releases | Version used | Minimum required | Runtime | diff --git a/src/Makefile.am b/src/Makefile.am index 8508d13b34..99b2184cf2 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -404,6 +404,7 @@ libbitcoin_node_a_SOURCES = \ kernel/coinstats.cpp \ kernel/context.cpp \ kernel/cs_main.cpp \ + kernel/disconnected_transactions.cpp \ kernel/mempool_persist.cpp \ kernel/mempool_removal_reason.cpp \ mapport.cpp \ @@ -944,6 +945,7 @@ libbitcoinkernel_la_SOURCES = \ kernel/coinstats.cpp \ kernel/context.cpp \ kernel/cs_main.cpp \ + kernel/disconnected_transactions.cpp \ kernel/mempool_persist.cpp \ kernel/mempool_removal_reason.cpp \ key.cpp \ diff --git a/src/Makefile.test.include b/src/Makefile.test.include index b610dabd07..4a42557372 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -92,6 +92,7 @@ BITCOIN_TESTS =\ test/dbwrapper_tests.cpp \ test/denialofservice_tests.cpp \ test/descriptor_tests.cpp \ + test/disconnected_transactions.cpp \ test/flatfile_tests.cpp \ test/fs_tests.cpp \ test/getarg_tests.cpp \ diff --git a/src/addrman.cpp b/src/addrman.cpp index b001365ab3..5a11526471 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -610,8 +610,9 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, std::c ClearNew(nUBucket, nUBucketPos); pinfo->nRefCount++; vvNew[nUBucket][nUBucketPos] = nId; - LogPrint(BCLog::ADDRMAN, "Added %s mapped to AS%i to new[%i][%i]\n", - addr.ToStringAddrPort(), m_netgroupman.GetMappedAS(addr), nUBucket, nUBucketPos); + const auto mapped_as{m_netgroupman.GetMappedAS(addr)}; + LogPrint(BCLog::ADDRMAN, "Added %s%s to new[%i][%i]\n", + addr.ToStringAddrPort(), (mapped_as ? strprintf(" mapped to AS%i", mapped_as) : ""), nUBucket, nUBucketPos); } else { if (pinfo->nRefCount == 0) { Delete(nId); @@ -669,8 +670,9 @@ bool AddrManImpl::Good_(const CService& addr, bool test_before_evict, NodeSecond } else { // move nId to the tried tables MakeTried(info, nId); - LogPrint(BCLog::ADDRMAN, "Moved %s mapped to AS%i to tried[%i][%i]\n", - addr.ToStringAddrPort(), m_netgroupman.GetMappedAS(addr), tried_bucket, tried_bucket_pos); + const auto mapped_as{m_netgroupman.GetMappedAS(addr)}; + LogPrint(BCLog::ADDRMAN, "Moved %s%s to tried[%i][%i]\n", + addr.ToStringAddrPort(), (mapped_as ? strprintf(" mapped to AS%i", mapped_as) : ""), tried_bucket, tried_bucket_pos); return true; } } diff --git a/src/banman.cpp b/src/banman.cpp index a96b7e3c53..9f668d76a3 100644 --- a/src/banman.cpp +++ b/src/banman.cpp @@ -28,7 +28,7 @@ BanMan::~BanMan() void BanMan::LoadBanlist() { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated); @@ -52,16 +52,17 @@ void BanMan::DumpBanlist() banmap_t banmap; { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); SweepBanned(); - if (!BannedSetIsDirty()) return; + if (!m_is_dirty) return; banmap = m_banned; - SetBannedSetDirty(false); + m_is_dirty = false; } const auto start{SteadyClock::now()}; if (!m_ban_db.Write(banmap)) { - SetBannedSetDirty(true); + LOCK(m_banned_mutex); + m_is_dirty = true; } LogPrint(BCLog::NET, "Flushed %d banned node addresses/subnets to disk %dms\n", banmap.size(), @@ -71,7 +72,7 @@ void BanMan::DumpBanlist() void BanMan::ClearBanned() { { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); m_banned.clear(); m_is_dirty = true; } @@ -81,14 +82,14 @@ void BanMan::ClearBanned() bool BanMan::IsDiscouraged(const CNetAddr& net_addr) { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); return m_discouraged.contains(net_addr.GetAddrBytes()); } bool BanMan::IsBanned(const CNetAddr& net_addr) { auto current_time = GetTime(); - LOCK(m_cs_banned); + LOCK(m_banned_mutex); for (const auto& it : m_banned) { CSubNet sub_net = it.first; CBanEntry ban_entry = it.second; @@ -103,7 +104,7 @@ bool BanMan::IsBanned(const CNetAddr& net_addr) bool BanMan::IsBanned(const CSubNet& sub_net) { auto current_time = GetTime(); - LOCK(m_cs_banned); + LOCK(m_banned_mutex); banmap_t::iterator i = m_banned.find(sub_net); if (i != m_banned.end()) { CBanEntry ban_entry = (*i).second; @@ -122,7 +123,7 @@ void BanMan::Ban(const CNetAddr& net_addr, int64_t ban_time_offset, bool since_u void BanMan::Discourage(const CNetAddr& net_addr) { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); m_discouraged.insert(net_addr.GetAddrBytes()); } @@ -139,7 +140,7 @@ void BanMan::Ban(const CSubNet& sub_net, int64_t ban_time_offset, bool since_uni ban_entry.nBanUntil = (normalized_since_unix_epoch ? 0 : GetTime()) + normalized_ban_time_offset; { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); if (m_banned[sub_net].nBanUntil < ban_entry.nBanUntil) { m_banned[sub_net] = ban_entry; m_is_dirty = true; @@ -161,7 +162,7 @@ bool BanMan::Unban(const CNetAddr& net_addr) bool BanMan::Unban(const CSubNet& sub_net) { { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); if (m_banned.erase(sub_net) == 0) return false; m_is_dirty = true; } @@ -172,7 +173,7 @@ bool BanMan::Unban(const CSubNet& sub_net) void BanMan::GetBanned(banmap_t& banmap) { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); // Sweep the banlist so expired bans are not returned SweepBanned(); banmap = m_banned; //create a thread safe copy @@ -180,7 +181,7 @@ void BanMan::GetBanned(banmap_t& banmap) void BanMan::SweepBanned() { - AssertLockHeld(m_cs_banned); + AssertLockHeld(m_banned_mutex); int64_t now = GetTime(); bool notify_ui = false; @@ -203,15 +204,3 @@ void BanMan::SweepBanned() m_client_interface->BannedListChanged(); } } - -bool BanMan::BannedSetIsDirty() -{ - LOCK(m_cs_banned); - return m_is_dirty; -} - -void BanMan::SetBannedSetDirty(bool dirty) -{ - LOCK(m_cs_banned); //reuse m_banned lock for the m_is_dirty flag - m_is_dirty = dirty; -} diff --git a/src/banman.h b/src/banman.h index 5a5f5677b0..c6df7ec3c3 100644 --- a/src/banman.h +++ b/src/banman.h @@ -60,40 +60,37 @@ class BanMan public: ~BanMan(); BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time); - void Ban(const CNetAddr& net_addr, int64_t ban_time_offset = 0, bool since_unix_epoch = false); - void Ban(const CSubNet& sub_net, int64_t ban_time_offset = 0, bool since_unix_epoch = false); - void Discourage(const CNetAddr& net_addr); - void ClearBanned(); + void Ban(const CNetAddr& net_addr, int64_t ban_time_offset = 0, bool since_unix_epoch = false) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void Ban(const CSubNet& sub_net, int64_t ban_time_offset = 0, bool since_unix_epoch = false) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void Discourage(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void ClearBanned() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //! Return whether net_addr is banned - bool IsBanned(const CNetAddr& net_addr); + bool IsBanned(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //! Return whether sub_net is exactly banned - bool IsBanned(const CSubNet& sub_net); + bool IsBanned(const CSubNet& sub_net) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //! Return whether net_addr is discouraged. - bool IsDiscouraged(const CNetAddr& net_addr); + bool IsDiscouraged(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); - bool Unban(const CNetAddr& net_addr); - bool Unban(const CSubNet& sub_net); - void GetBanned(banmap_t& banmap); - void DumpBanlist(); + bool Unban(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + bool Unban(const CSubNet& sub_net) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void GetBanned(banmap_t& banmap) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void DumpBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); private: - void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned); - bool BannedSetIsDirty(); - //!set the "dirty" flag for the banlist - void SetBannedSetDirty(bool dirty = true); + void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //!clean unused entries (if bantime has expired) - void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_cs_banned); + void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_banned_mutex); - RecursiveMutex m_cs_banned; - banmap_t m_banned GUARDED_BY(m_cs_banned); - bool m_is_dirty GUARDED_BY(m_cs_banned){false}; + Mutex m_banned_mutex; + banmap_t m_banned GUARDED_BY(m_banned_mutex); + bool m_is_dirty GUARDED_BY(m_banned_mutex){false}; CClientUIInterface* m_client_interface = nullptr; CBanDB m_ban_db; const int64_t m_default_ban_time; - CRollingBloomFilter m_discouraged GUARDED_BY(m_cs_banned) {50000, 0.000001}; + CRollingBloomFilter m_discouraged GUARDED_BY(m_banned_mutex) {50000, 0.000001}; }; #endif // BITCOIN_BANMAN_H diff --git a/src/bench/disconnected_transactions.cpp b/src/bench/disconnected_transactions.cpp index 264c0aa1e8..91ce904dd9 100644 --- a/src/bench/disconnected_transactions.cpp +++ b/src/bench/disconnected_transactions.cpp @@ -73,7 +73,7 @@ static ReorgTxns CreateBlocks(size_t num_not_shared) static void Reorg(const ReorgTxns& reorg) { - DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_SIZE * 1000}; + DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_BYTES}; // Disconnect block const auto evicted = disconnectpool.AddTransactionsFromBlock(reorg.disconnected_txns); assert(evicted.empty()); diff --git a/src/kernel/disconnected_transactions.cpp b/src/kernel/disconnected_transactions.cpp new file mode 100644 index 0000000000..f865fed688 --- /dev/null +++ b/src/kernel/disconnected_transactions.cpp @@ -0,0 +1,90 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include <kernel/disconnected_transactions.h> + +#include <assert.h> +#include <core_memusage.h> +#include <memusage.h> +#include <primitives/transaction.h> +#include <util/hasher.h> + +#include <memory> +#include <utility> + +// It's almost certainly a logic bug if we don't clear out queuedTx before +// destruction, as we add to it while disconnecting blocks, and then we +// need to re-process remaining transactions to ensure mempool consistency. +// For now, assert() that we've emptied out this object on destruction. +// This assert() can always be removed if the reorg-processing code were +// to be refactored such that this assumption is no longer true (for +// instance if there was some other way we cleaned up the mempool after a +// reorg, besides draining this object). +DisconnectedBlockTransactions::~DisconnectedBlockTransactions() +{ + assert(queuedTx.empty()); + assert(iters_by_txid.empty()); + assert(cachedInnerUsage == 0); +} + +std::vector<CTransactionRef> DisconnectedBlockTransactions::LimitMemoryUsage() +{ + std::vector<CTransactionRef> evicted; + + while (!queuedTx.empty() && DynamicMemoryUsage() > m_max_mem_usage) { + evicted.emplace_back(queuedTx.front()); + cachedInnerUsage -= RecursiveDynamicUsage(queuedTx.front()); + iters_by_txid.erase(queuedTx.front()->GetHash()); + queuedTx.pop_front(); + } + return evicted; +} + +size_t DisconnectedBlockTransactions::DynamicMemoryUsage() const +{ + return cachedInnerUsage + memusage::DynamicUsage(iters_by_txid) + memusage::DynamicUsage(queuedTx); +} + +[[nodiscard]] std::vector<CTransactionRef> DisconnectedBlockTransactions::AddTransactionsFromBlock(const std::vector<CTransactionRef>& vtx) +{ + iters_by_txid.reserve(iters_by_txid.size() + vtx.size()); + for (auto block_it = vtx.rbegin(); block_it != vtx.rend(); ++block_it) { + auto it = queuedTx.insert(queuedTx.end(), *block_it); + auto [_, inserted] = iters_by_txid.emplace((*block_it)->GetHash(), it); + assert(inserted); // callers may never pass multiple transactions with the same txid + cachedInnerUsage += RecursiveDynamicUsage(*block_it); + } + return LimitMemoryUsage(); +} + +void DisconnectedBlockTransactions::removeForBlock(const std::vector<CTransactionRef>& vtx) +{ + // Short-circuit in the common case of a block being added to the tip + if (queuedTx.empty()) { + return; + } + for (const auto& tx : vtx) { + auto iter = iters_by_txid.find(tx->GetHash()); + if (iter != iters_by_txid.end()) { + auto list_iter = iter->second; + iters_by_txid.erase(iter); + cachedInnerUsage -= RecursiveDynamicUsage(*list_iter); + queuedTx.erase(list_iter); + } + } +} + +void DisconnectedBlockTransactions::clear() +{ + cachedInnerUsage = 0; + iters_by_txid.clear(); + queuedTx.clear(); +} + +std::list<CTransactionRef> DisconnectedBlockTransactions::take() +{ + std::list<CTransactionRef> ret = std::move(queuedTx); + clear(); + return ret; +} diff --git a/src/kernel/disconnected_transactions.h b/src/kernel/disconnected_transactions.h index 7db39ba5ca..401ec435e6 100644 --- a/src/kernel/disconnected_transactions.h +++ b/src/kernel/disconnected_transactions.h @@ -5,8 +5,6 @@ #ifndef BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H #define BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H -#include <core_memusage.h> -#include <memusage.h> #include <primitives/transaction.h> #include <util/hasher.h> @@ -14,8 +12,8 @@ #include <unordered_map> #include <vector> -/** Maximum kilobytes for transactions to store for processing during reorg */ -static const unsigned int MAX_DISCONNECTED_TX_POOL_SIZE = 20'000; +/** Maximum bytes for transactions to store for processing during reorg */ +static const unsigned int MAX_DISCONNECTED_TX_POOL_BYTES{20'000'000}; /** * DisconnectedBlockTransactions @@ -38,8 +36,7 @@ static const unsigned int MAX_DISCONNECTED_TX_POOL_SIZE = 20'000; */ class DisconnectedBlockTransactions { private: - /** Cached dynamic memory usage for the CTransactions (memory for the shared pointers is - * included in the container calculations). */ + /** Cached dynamic memory usage for the `CTransactionRef`s */ uint64_t cachedInnerUsage = 0; const size_t m_max_mem_usage; std::list<CTransactionRef> queuedTx; @@ -47,39 +44,15 @@ private: std::unordered_map<uint256, TxList::iterator, SaltedTxidHasher> iters_by_txid; /** Trim the earliest-added entries until we are within memory bounds. */ - std::vector<CTransactionRef> LimitMemoryUsage() - { - std::vector<CTransactionRef> evicted; - - while (!queuedTx.empty() && DynamicMemoryUsage() > m_max_mem_usage) { - evicted.emplace_back(queuedTx.front()); - cachedInnerUsage -= RecursiveDynamicUsage(*queuedTx.front()); - iters_by_txid.erase(queuedTx.front()->GetHash()); - queuedTx.pop_front(); - } - return evicted; - } + std::vector<CTransactionRef> LimitMemoryUsage(); public: - DisconnectedBlockTransactions(size_t max_mem_usage) : m_max_mem_usage{max_mem_usage} {} + DisconnectedBlockTransactions(size_t max_mem_usage) + : m_max_mem_usage{max_mem_usage} {} - // It's almost certainly a logic bug if we don't clear out queuedTx before - // destruction, as we add to it while disconnecting blocks, and then we - // need to re-process remaining transactions to ensure mempool consistency. - // For now, assert() that we've emptied out this object on destruction. - // This assert() can always be removed if the reorg-processing code were - // to be refactored such that this assumption is no longer true (for - // instance if there was some other way we cleaned up the mempool after a - // reorg, besides draining this object). - ~DisconnectedBlockTransactions() { - assert(queuedTx.empty()); - assert(iters_by_txid.empty()); - assert(cachedInnerUsage == 0); - } + ~DisconnectedBlockTransactions(); - size_t DynamicMemoryUsage() const { - return cachedInnerUsage + memusage::DynamicUsage(iters_by_txid) + memusage::DynamicUsage(queuedTx); - } + size_t DynamicMemoryUsage() const; /** Add transactions from the block, iterating through vtx in reverse order. Callers should call * this function for blocks in descending order by block height. @@ -88,50 +61,16 @@ public: * corresponding entry in iters_by_txid. * @returns vector of transactions that were evicted for size-limiting. */ - [[nodiscard]] std::vector<CTransactionRef> AddTransactionsFromBlock(const std::vector<CTransactionRef>& vtx) - { - iters_by_txid.reserve(iters_by_txid.size() + vtx.size()); - for (auto block_it = vtx.rbegin(); block_it != vtx.rend(); ++block_it) { - auto it = queuedTx.insert(queuedTx.end(), *block_it); - iters_by_txid.emplace((*block_it)->GetHash(), it); - cachedInnerUsage += RecursiveDynamicUsage(**block_it); - } - return LimitMemoryUsage(); - } + [[nodiscard]] std::vector<CTransactionRef> AddTransactionsFromBlock(const std::vector<CTransactionRef>& vtx); /** Remove any entries that are in this block. */ - void removeForBlock(const std::vector<CTransactionRef>& vtx) - { - // Short-circuit in the common case of a block being added to the tip - if (queuedTx.empty()) { - return; - } - for (const auto& tx : vtx) { - auto iter = iters_by_txid.find(tx->GetHash()); - if (iter != iters_by_txid.end()) { - auto list_iter = iter->second; - iters_by_txid.erase(iter); - cachedInnerUsage -= RecursiveDynamicUsage(**list_iter); - queuedTx.erase(list_iter); - } - } - } + void removeForBlock(const std::vector<CTransactionRef>& vtx); size_t size() const { return queuedTx.size(); } - void clear() - { - cachedInnerUsage = 0; - iters_by_txid.clear(); - queuedTx.clear(); - } + void clear(); /** Clear all data structures and return the list of transactions. */ - std::list<CTransactionRef> take() - { - std::list<CTransactionRef> ret = std::move(queuedTx); - clear(); - return ret; - } + std::list<CTransactionRef> take(); }; #endif // BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 87bfa4cfc3..9557594622 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -260,6 +260,11 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal, unsigned int curFarBucket = maxbucketindex; unsigned int bestFarBucket = maxbucketindex; + // We'll always group buckets into sets that meet sufficientTxVal -- + // this ensures that we're using consistent groups between different + // confirmation targets. + double partialNum = 0; + bool foundAnswer = false; unsigned int bins = unconfTxs.size(); bool newBucketRange = true; @@ -275,6 +280,7 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal, } curFarBucket = bucket; nConf += confAvg[periodTarget - 1][bucket]; + partialNum += txCtAvg[bucket]; totalNum += txCtAvg[bucket]; failNum += failAvg[periodTarget - 1][bucket]; for (unsigned int confct = confTarget; confct < GetMaxConfirms(); confct++) @@ -284,7 +290,14 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal, // we can test for success // (Only count the confirmed data points, so that each confirmation count // will be looking at the same amount of data and same bucket breaks) - if (totalNum >= sufficientTxVal / (1 - decay)) { + + if (partialNum < sufficientTxVal / (1 - decay)) { + // the buckets we've added in this round aren't sufficient + // so keep adding + continue; + } else { + partialNum = 0; // reset for the next range we'll add + double curPct = nConf / (totalNum + failNum + extraNum); // Check to see if we are no longer getting confirmed at the success rate diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp index 67af62285d..7e24dbd3ec 100644 --- a/src/qt/transactionview.cpp +++ b/src/qt/transactionview.cpp @@ -531,6 +531,9 @@ void TransactionView::showDetails() TransactionDescDialog *dlg = new TransactionDescDialog(selection.at(0)); dlg->setAttribute(Qt::WA_DeleteOnClose); m_opened_dialogs.append(dlg); + connect(dlg, &QObject::destroyed, [this, dlg] { + m_opened_dialogs.removeOne(dlg); + }); dlg->show(); } } diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index a11366bd47..639237132b 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -20,6 +20,7 @@ #include <util/string.h> #include <util/translation.h> +#include <string_view> #include <tuple> const std::string UNIX_EPOCH_TIME = "UNIX epoch time"; @@ -74,29 +75,29 @@ CAmount AmountFromValue(const UniValue& value, int decimals) return amount; } -uint256 ParseHashV(const UniValue& v, std::string strName) +uint256 ParseHashV(const UniValue& v, std::string_view name) { const std::string& strHex(v.get_str()); if (64 != strHex.length()) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", strName, 64, strHex.length(), strHex)); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, 64, strHex.length(), strHex)); if (!IsHex(strHex)) // Note: IsHex("") is false - throw JSONRPCError(RPC_INVALID_PARAMETER, strName+" must be hexadecimal string (not '"+strHex+"')"); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be hexadecimal string (not '%s')", name, strHex)); return uint256S(strHex); } -uint256 ParseHashO(const UniValue& o, std::string strKey) +uint256 ParseHashO(const UniValue& o, std::string_view strKey) { return ParseHashV(o.find_value(strKey), strKey); } -std::vector<unsigned char> ParseHexV(const UniValue& v, std::string strName) +std::vector<unsigned char> ParseHexV(const UniValue& v, std::string_view name) { std::string strHex; if (v.isStr()) strHex = v.get_str(); if (!IsHex(strHex)) - throw JSONRPCError(RPC_INVALID_PARAMETER, strName+" must be hexadecimal string (not '"+strHex+"')"); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be hexadecimal string (not '%s')", name, strHex)); return ParseHex(strHex); } -std::vector<unsigned char> ParseHexO(const UniValue& o, std::string strKey) +std::vector<unsigned char> ParseHexO(const UniValue& o, std::string_view strKey) { return ParseHexV(o.find_value(strKey), strKey); } diff --git a/src/rpc/util.h b/src/rpc/util.h index 392540ffad..addf9000d0 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -26,6 +26,7 @@ #include <map> #include <optional> #include <string> +#include <string_view> #include <type_traits> #include <utility> #include <variant> @@ -91,10 +92,10 @@ void RPCTypeCheckObj(const UniValue& o, * Utilities: convert hex-encoded Values * (throws error if not hex). */ -uint256 ParseHashV(const UniValue& v, std::string strName); -uint256 ParseHashO(const UniValue& o, std::string strKey); -std::vector<unsigned char> ParseHexV(const UniValue& v, std::string strName); -std::vector<unsigned char> ParseHexO(const UniValue& o, std::string strKey); +uint256 ParseHashV(const UniValue& v, std::string_view name); +uint256 ParseHashO(const UniValue& o, std::string_view strKey); +std::vector<unsigned char> ParseHexV(const UniValue& v, std::string_view name); +std::vector<unsigned char> ParseHexO(const UniValue& o, std::string_view strKey); /** * Validate and return a CAmount from a UniValue number or string. diff --git a/src/test/disconnected_transactions.cpp b/src/test/disconnected_transactions.cpp new file mode 100644 index 0000000000..d4dc124b7b --- /dev/null +++ b/src/test/disconnected_transactions.cpp @@ -0,0 +1,95 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. +// +#include <boost/test/unit_test.hpp> +#include <core_memusage.h> +#include <kernel/disconnected_transactions.h> +#include <test/util/setup_common.h> + +BOOST_FIXTURE_TEST_SUITE(disconnected_transactions, TestChain100Setup) + +//! Tests that DisconnectedBlockTransactions limits its own memory properly +BOOST_AUTO_TEST_CASE(disconnectpool_memory_limits) +{ + // Use the coinbase transactions from TestChain100Setup. It doesn't matter whether these + // transactions would realistically be in a block together, they just need distinct txids and + // uniform size for this test to work. + std::vector<CTransactionRef> block_vtx(m_coinbase_txns); + BOOST_CHECK_EQUAL(block_vtx.size(), 100); + + // Roughly estimate sizes to sanity check that DisconnectedBlockTransactions::DynamicMemoryUsage + // is within an expected range. + + // Overhead for the hashmap depends on number of buckets + std::unordered_map<uint256, CTransaction*, SaltedTxidHasher> temp_map; + temp_map.reserve(1); + const size_t MAP_1{memusage::DynamicUsage(temp_map)}; + temp_map.reserve(100); + const size_t MAP_100{memusage::DynamicUsage(temp_map)}; + + const size_t TX_USAGE{RecursiveDynamicUsage(block_vtx.front())}; + for (const auto& tx : block_vtx) + BOOST_CHECK_EQUAL(RecursiveDynamicUsage(tx), TX_USAGE); + + // Our overall formula is unordered map overhead + usage per entry. + // Implementations may vary, but we're trying to guess the usage of data structures. + const size_t ENTRY_USAGE_ESTIMATE{ + TX_USAGE + // list entry: 2 pointers (next pointer and prev pointer) + element itself + + memusage::MallocUsage((2 * sizeof(void*)) + sizeof(decltype(block_vtx)::value_type)) + // unordered map: 1 pointer for the hashtable + key and value + + memusage::MallocUsage(sizeof(void*) + sizeof(decltype(temp_map)::key_type) + + sizeof(decltype(temp_map)::value_type))}; + + // DisconnectedBlockTransactions that's just big enough for 1 transaction. + { + DisconnectedBlockTransactions disconnectpool{MAP_1 + ENTRY_USAGE_ESTIMATE}; + // Add just 2 (and not all 100) transactions to keep the unordered map's hashtable overhead + // to a minimum and avoid all (instead of all but 1) transactions getting evicted. + std::vector<CTransactionRef> two_txns({block_vtx.at(0), block_vtx.at(1)}); + auto evicted_txns{disconnectpool.AddTransactionsFromBlock(two_txns)}; + BOOST_CHECK(disconnectpool.DynamicMemoryUsage() <= MAP_1 + ENTRY_USAGE_ESTIMATE); + + // Only 1 transaction can be kept + BOOST_CHECK_EQUAL(1, evicted_txns.size()); + // Transactions are added from back to front and eviction is FIFO. + BOOST_CHECK_EQUAL(block_vtx.at(1), evicted_txns.front()); + + disconnectpool.clear(); + } + + // DisconnectedBlockTransactions with a comfortable maximum memory usage so that nothing is evicted. + // Record usage so we can check size limiting in the next test. + size_t usage_full{0}; + { + const size_t USAGE_100_OVERESTIMATE{MAP_100 + ENTRY_USAGE_ESTIMATE * 100}; + DisconnectedBlockTransactions disconnectpool{USAGE_100_OVERESTIMATE}; + auto evicted_txns{disconnectpool.AddTransactionsFromBlock(block_vtx)}; + BOOST_CHECK_EQUAL(evicted_txns.size(), 0); + BOOST_CHECK(disconnectpool.DynamicMemoryUsage() <= USAGE_100_OVERESTIMATE); + + usage_full = disconnectpool.DynamicMemoryUsage(); + + disconnectpool.clear(); + } + + // DisconnectedBlockTransactions that's just a little too small for all of the transactions. + { + const size_t MAX_MEMUSAGE_99{usage_full - sizeof(void*)}; + DisconnectedBlockTransactions disconnectpool{MAX_MEMUSAGE_99}; + auto evicted_txns{disconnectpool.AddTransactionsFromBlock(block_vtx)}; + BOOST_CHECK(disconnectpool.DynamicMemoryUsage() <= MAX_MEMUSAGE_99); + + // Only 1 transaction needed to be evicted + BOOST_CHECK_EQUAL(1, evicted_txns.size()); + + // Transactions are added from back to front and eviction is FIFO. + // The last transaction of block_vtx should be the first to be evicted. + BOOST_CHECK_EQUAL(block_vtx.back(), evicted_txns.front()); + + disconnectpool.clear(); + } +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 7220c5d997..8d316134cc 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -257,15 +257,6 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) const auto result_package = WITH_LOCK(::cs_main, return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit)); - // If something went wrong due to a package-specific policy, it might not return a - // validation result for the transaction. - if (result_package.m_state.GetResult() != PackageValidationResult::PCKG_POLICY) { - auto it = result_package.m_tx_results.find(txs.back()->GetWitnessHash()); - Assert(it != result_package.m_tx_results.end()); - Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID || - it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID || - it->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); - } const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, txs.back(), GetTime(), bypass_limits, /*test_accept=*/!single_submit)); const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID; @@ -281,6 +272,12 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) Assert(added.size() == 1); Assert(txs.back() == *added.begin()); } + } else if (result_package.m_state.GetResult() != PackageValidationResult::PCKG_POLICY) { + // We don't know anything about the validity since transactions were randomly generated, so + // just use result_package.m_state here. This makes the expect_valid check meaningless, but + // we can still verify that the contents of m_tx_results are consistent with m_state. + const bool expect_valid{result_package.m_state.IsValid()}; + Assert(!CheckPackageMempoolAcceptResult(txs, result_package, expect_valid, nullptr)); } else { // This is empty if it fails early checks, or "full" if transactions are looked at deeper Assert(result_package.m_tx_results.size() == txs.size() || result_package.m_tx_results.empty()); diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 5ec3e89d1e..66e537a57b 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -131,6 +131,53 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte return CTxMemPool{mempool_opts}; } +void CheckATMPInvariants(const MempoolAcceptResult& res, bool txid_in_mempool, bool wtxid_in_mempool) +{ + + switch (res.m_result_type) { + case MempoolAcceptResult::ResultType::VALID: + { + Assert(txid_in_mempool); + Assert(wtxid_in_mempool); + Assert(res.m_state.IsValid()); + Assert(!res.m_state.IsInvalid()); + Assert(res.m_replaced_transactions); + Assert(res.m_vsize); + Assert(res.m_base_fees); + Assert(res.m_effective_feerate); + Assert(res.m_wtxids_fee_calculations); + Assert(!res.m_other_wtxid); + break; + } + case MempoolAcceptResult::ResultType::INVALID: + { + // It may be already in the mempool since in ATMP cases we don't set MEMPOOL_ENTRY or DIFFERENT_WITNESS + Assert(!res.m_state.IsValid()); + Assert(res.m_state.IsInvalid()); + Assert(!res.m_replaced_transactions); + Assert(!res.m_vsize); + Assert(!res.m_base_fees); + // Unable or unwilling to calculate fees + Assert(!res.m_effective_feerate); + Assert(!res.m_wtxids_fee_calculations); + Assert(!res.m_other_wtxid); + break; + } + case MempoolAcceptResult::ResultType::MEMPOOL_ENTRY: + { + // ATMP never sets this; only set in package settings + Assert(false); + break; + } + case MempoolAcceptResult::ResultType::DIFFERENT_WITNESS: + { + // ATMP never sets this; only set in package settings + Assert(false); + break; + } + } +} + FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); @@ -258,9 +305,11 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool) SyncWithValidationInterfaceQueue(); UnregisterSharedValidationInterface(txr); + bool txid_in_mempool = tx_pool.exists(GenTxid::Txid(tx->GetHash())); + bool wtxid_in_mempool = tx_pool.exists(GenTxid::Wtxid(tx->GetWitnessHash())); + CheckATMPInvariants(res, txid_in_mempool, wtxid_in_mempool); + Assert(accepted != added.empty()); - Assert(accepted == res.m_state.IsValid()); - Assert(accepted != res.m_state.IsInvalid()); if (accepted) { Assert(added.size() == 1); // For now, no package acceptance Assert(tx == *added.begin()); diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index c945f35d79..c4fbc8dbb3 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -11,6 +11,7 @@ #include <util/check.h> #include <util/time.h> #include <util/translation.h> +#include <validation.h> using node::NodeContext; @@ -36,3 +37,80 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransactionRef& tx) const { return CTxMemPoolEntry{tx, nFee, TicksSinceEpoch<std::chrono::seconds>(time), nHeight, m_sequence, spendsCoinbase, sigOpCost, lp}; } + +std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns, + const PackageMempoolAcceptResult& result, + bool expect_valid, + const CTxMemPool* mempool) +{ + if (expect_valid) { + if (result.m_state.IsInvalid()) { + return strprintf("Package validation unexpectedly failed: %s", result.m_state.ToString()); + } + } else { + if (result.m_state.IsValid()) { + strprintf("Package validation unexpectedly succeeded. %s", result.m_state.ToString()); + } + } + if (result.m_state.GetResult() != PackageValidationResult::PCKG_POLICY && txns.size() != result.m_tx_results.size()) { + strprintf("txns size %u does not match tx results size %u", txns.size(), result.m_tx_results.size()); + } + for (const auto& tx : txns) { + const auto& wtxid = tx->GetWitnessHash(); + if (result.m_tx_results.count(wtxid) == 0) { + return strprintf("result not found for tx %s", wtxid.ToString()); + } + + const auto& atmp_result = result.m_tx_results.at(wtxid); + const bool valid{atmp_result.m_result_type == MempoolAcceptResult::ResultType::VALID}; + if (expect_valid && atmp_result.m_state.IsInvalid()) { + return strprintf("tx %s unexpectedly failed: %s", wtxid.ToString(), atmp_result.m_state.ToString()); + } + + //m_replaced_transactions should exist iff the result was VALID + if (atmp_result.m_replaced_transactions.has_value() != valid) { + return strprintf("tx %s result should %shave m_replaced_transactions", + wtxid.ToString(), valid ? "" : "not "); + } + + // m_vsize and m_base_fees should exist iff the result was VALID or MEMPOOL_ENTRY + const bool mempool_entry{atmp_result.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY}; + if (atmp_result.m_base_fees.has_value() != (valid || mempool_entry)) { + return strprintf("tx %s result should %shave m_base_fees", wtxid.ToString(), valid || mempool_entry ? "" : "not "); + } + if (atmp_result.m_vsize.has_value() != (valid || mempool_entry)) { + return strprintf("tx %s result should %shave m_vsize", wtxid.ToString(), valid || mempool_entry ? "" : "not "); + } + + // m_other_wtxid should exist iff the result was DIFFERENT_WITNESS + const bool diff_witness{atmp_result.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS}; + if (atmp_result.m_other_wtxid.has_value() != diff_witness) { + return strprintf("tx %s result should %shave m_other_wtxid", wtxid.ToString(), diff_witness ? "" : "not "); + } + + // m_effective_feerate and m_wtxids_fee_calculations should exist iff the result was valid + if (atmp_result.m_effective_feerate.has_value() != valid) { + return strprintf("tx %s result should %shave m_effective_feerate", + wtxid.ToString(), valid ? "" : "not "); + } + if (atmp_result.m_wtxids_fee_calculations.has_value() != valid) { + return strprintf("tx %s result should %shave m_effective_feerate", + wtxid.ToString(), valid ? "" : "not "); + } + + if (mempool) { + // The tx by txid should be in the mempool iff the result was not INVALID. + const bool txid_in_mempool{atmp_result.m_result_type != MempoolAcceptResult::ResultType::INVALID}; + if (mempool->exists(GenTxid::Txid(tx->GetHash())) != txid_in_mempool) { + strprintf("tx %s should %sbe in mempool", wtxid.ToString(), txid_in_mempool ? "" : "not "); + } + // Additionally, if the result was DIFFERENT_WITNESS, we shouldn't be able to find the tx in mempool by wtxid. + if (tx->HasWitness() && atmp_result.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS) { + if (mempool->exists(GenTxid::Wtxid(wtxid))) { + strprintf("wtxid %s should not be in mempool", wtxid.ToString()); + } + } + } + } + return std::nullopt; +} diff --git a/src/test/util/txmempool.h b/src/test/util/txmempool.h index 4b0daf0d42..a866d1ce74 100644 --- a/src/test/util/txmempool.h +++ b/src/test/util/txmempool.h @@ -5,12 +5,14 @@ #ifndef BITCOIN_TEST_UTIL_TXMEMPOOL_H #define BITCOIN_TEST_UTIL_TXMEMPOOL_H +#include <policy/packages.h> #include <txmempool.h> #include <util/time.h> namespace node { struct NodeContext; } +struct PackageMempoolAcceptResult; CTxMemPool::Options MemPoolOptionsForTest(const node::NodeContext& node); @@ -36,4 +38,12 @@ struct TestMemPoolEntryHelper { TestMemPoolEntryHelper& SigOpsCost(unsigned int _sigopsCost) { sigOpCost = _sigopsCost; return *this; } }; +/** Check expected properties for every PackageMempoolAcceptResult, regardless of value. Returns + * a string if an error occurs with error populated, nullopt otherwise. If mempool is provided, + * checks that the expected transactions are in mempool (this should be set to nullptr for a test_accept). +*/ +std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns, + const PackageMempoolAcceptResult& result, + bool expect_valid, + const CTxMemPool* mempool); #endif // BITCOIN_TEST_UTIL_TXMEMPOOL_H diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 227d7d4633..e2541a74fd 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -579,7 +579,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) // it will initialize instead of attempting to complete validation. // // Note that this is not a realistic use of DisconnectTip(). - DisconnectedBlockTransactions unused_pool{MAX_DISCONNECTED_TX_POOL_SIZE * 1000}; + DisconnectedBlockTransactions unused_pool{MAX_DISCONNECTED_TX_POOL_BYTES}; BlockValidationState unused_state; { LOCK2(::cs_main, bg_chainstate.MempoolMutex()); diff --git a/src/util/strencodings.h b/src/util/strencodings.h index d792562735..439678c24a 100644 --- a/src/util/strencodings.h +++ b/src/util/strencodings.h @@ -260,7 +260,6 @@ bool TimingResistantEqual(const T& a, const T& b) } /** Parse number as fixed point according to JSON number syntax. - * See https://json.org/number.gif * @returns true on success, false on error. * @note The result must be in the range (-10^18,10^18), otherwise an overflow error will trigger. */ diff --git a/src/validation.cpp b/src/validation.cpp index 2f8c0a9f04..8d7e366125 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5,10 +5,6 @@ #include <validation.h> -#include <kernel/chain.h> -#include <kernel/coinstats.h> -#include <kernel/mempool_persist.h> - #include <arith_uint256.h> #include <chain.h> #include <checkqueue.h> @@ -22,7 +18,9 @@ #include <cuckoocache.h> #include <flatfile.h> #include <hash.h> +#include <kernel/chain.h> #include <kernel/chainparams.h> +#include <kernel/coinstats.h> #include <kernel/disconnected_transactions.h> #include <kernel/mempool_entry.h> #include <kernel/messagestartchars.h> @@ -3075,7 +3073,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex* // Disconnect active blocks which are no longer in the best chain. bool fBlocksDisconnected = false; - DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_SIZE * 1000}; + DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_BYTES}; while (m_chain.Tip() && m_chain.Tip() != pindexFork) { if (!DisconnectTip(state, &disconnectpool)) { // This is likely a fatal error, but keep the mempool consistent, @@ -3433,7 +3431,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // ActivateBestChain considers blocks already in m_chain // unconditionally valid already, so force disconnect away from it. - DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_SIZE * 1000}; + DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_BYTES}; bool ret = DisconnectTip(state, &disconnectpool); // DisconnectTip will add transactions to disconnectpool. // Adjust the mempool to be consistent with the new tip, adding diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index 63cd10896d..2adcaf178c 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -270,15 +270,16 @@ class AddrTest(BitcoinTestFramework): full_outbound_peer.sync_with_ping() assert full_outbound_peer.getaddr_received() - self.log.info('Check that we do not send a getaddr message upon connecting to a block-relay-only peer') + self.log.info('Check that we do not send a getaddr message to a block-relay-only or inbound peer') block_relay_peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=1, connection_type="block-relay-only") block_relay_peer.sync_with_ping() assert_equal(block_relay_peer.getaddr_received(), False) - self.log.info('Check that we answer getaddr messages only from inbound peers') inbound_peer = self.nodes[0].add_p2p_connection(AddrReceiver(send_getaddr=False)) inbound_peer.sync_with_ping() + assert_equal(inbound_peer.getaddr_received(), False) + self.log.info('Check that we answer getaddr messages only from inbound peers') # Add some addresses to addrman for i in range(1000): first_octet = i >> 8 diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 2fb88b828f..4916d36ab7 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -216,7 +216,7 @@ class InvalidMessagesTest(BitcoinTestFramework): self.test_addrv2('unrecognized network', [ 'received: addrv2 (25 bytes)', - '9.9.9.9:8333 mapped', + '9.9.9.9:8333', 'Added 1 addresses', ], bytes.fromhex( diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index 53163720bb..8eb9f3aeb1 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -58,6 +58,7 @@ TIME_RANGE_STEP = 600 # ten-minute steps TIME_RANGE_MTP = TIME_GENESIS_BLOCK + (HEIGHT - 6) * TIME_RANGE_STEP TIME_RANGE_TIP = TIME_GENESIS_BLOCK + (HEIGHT - 1) * TIME_RANGE_STEP TIME_RANGE_END = TIME_GENESIS_BLOCK + HEIGHT * TIME_RANGE_STEP +DIFFICULTY_ADJUSTMENT_INTERVAL = 2016 class BlockchainTest(BitcoinTestFramework): @@ -451,6 +452,15 @@ class BlockchainTest(BitcoinTestFramework): # This should be 2 hashes every 10 minutes or 1/300 assert abs(hashes_per_second * 300 - 1) < 0.0001 + # Test setting the first param of getnetworkhashps to negative value returns the average network + # hashes per second from the last difficulty change. + current_block_height = self.nodes[0].getmininginfo()['blocks'] + blocks_since_last_diff_change = current_block_height % DIFFICULTY_ADJUSTMENT_INTERVAL + 1 + expected_hashes_per_second_since_diff_change = self.nodes[0].getnetworkhashps(blocks_since_last_diff_change) + + assert_equal(self.nodes[0].getnetworkhashps(-1), expected_hashes_per_second_since_diff_change) + assert_equal(self.nodes[0].getnetworkhashps(-2), expected_hashes_per_second_since_diff_change) + def _test_stopatheight(self): self.log.info("Test stopping at height") assert_equal(self.nodes[0].getblockcount(), HEIGHT) diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index be4ed624fc..352becb367 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -456,7 +456,8 @@ class P2PInterface(P2PConnection): self.send_message(msg_verack()) self.nServices = message.nServices self.relay = message.relay - self.send_message(msg_getaddr()) + if self.p2p_connected_to_node: + self.send_message(msg_getaddr()) # Connection helper methods diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index c77cfbdd91..b6af71d85c 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -644,6 +644,7 @@ class TestNode(): if 'dstaddr' not in kwargs: kwargs['dstaddr'] = '127.0.0.1' + p2p_conn.p2p_connected_to_node = True p2p_conn.peer_connect(**kwargs, net=self.chain, timeout_factor=self.timeout_factor)() self.p2ps.append(p2p_conn) p2p_conn.wait_until(lambda: p2p_conn.is_connected, check_connected=False) @@ -689,6 +690,7 @@ class TestNode(): self.log.debug("Connecting to %s:%d %s" % (address, port, connection_type)) self.addconnection('%s:%d' % (address, port), connection_type) + p2p_conn.p2p_connected_to_node = False p2p_conn.peer_accept_connection(connect_cb=addconnection_callback, connect_id=p2p_idx + 1, net=self.chain, timeout_factor=self.timeout_factor, **kwargs)() if connection_type == "feeler": diff --git a/test/lint/lint-circular-dependencies.py b/test/lint/lint-circular-dependencies.py index e366a08bd2..6f9a633807 100755 --- a/test/lint/lint-circular-dependencies.py +++ b/test/lint/lint-circular-dependencies.py @@ -21,7 +21,6 @@ EXPECTED_CIRCULAR_DEPENDENCIES = ( "qt/transactiontablemodel -> qt/walletmodel -> qt/transactiontablemodel", "wallet/wallet -> wallet/walletdb -> wallet/wallet", "kernel/coinstats -> validation -> kernel/coinstats", - "kernel/mempool_persist -> validation -> kernel/mempool_persist", # Temporary, removed in followup https://github.com/bitcoin/bitcoin/pull/24230 "index/base -> node/context -> net_processing -> index/blockfilterindex -> index/base", |