From d88c0d8440cf640ef4f2c7a40b8b8b31bfd38f23 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 18 Jan 2022 19:02:05 +0200 Subject: [PATCH 0001/1751] refactor: Get rid of `BanMan::BannedSetIsDirty()` --- src/banman.cpp | 8 +------- src/banman.h | 1 - 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/banman.cpp b/src/banman.cpp index 2a6e0e010fe0a..50dc0750e1f09 100644 --- a/src/banman.cpp +++ b/src/banman.cpp @@ -53,7 +53,7 @@ void BanMan::DumpBanlist() { LOCK(m_cs_banned); SweepBanned(); - if (!BannedSetIsDirty()) return; + if (!m_is_dirty) return; banmap = m_banned; SetBannedSetDirty(false); } @@ -203,12 +203,6 @@ void BanMan::SweepBanned() } } -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 diff --git a/src/banman.h b/src/banman.h index 77b043f081b5e..7f3c74733ed16 100644 --- a/src/banman.h +++ b/src/banman.h @@ -81,7 +81,6 @@ class BanMan private: void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned); - bool BannedSetIsDirty(); //!set the "dirty" flag for the banlist void SetBannedSetDirty(bool dirty = true); //!clean unused entries (if bantime has expired) From 46709c5f27bf6cbc8eba1298b04bd079da2cdded Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 18 Jan 2022 19:18:02 +0200 Subject: [PATCH 0002/1751] refactor: Get rid of `BanMan::SetBannedSetDirty()` --- src/banman.cpp | 11 +++-------- src/banman.h | 2 -- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/banman.cpp b/src/banman.cpp index 50dc0750e1f09..5b2a1795433bf 100644 --- a/src/banman.cpp +++ b/src/banman.cpp @@ -55,12 +55,13 @@ void BanMan::DumpBanlist() SweepBanned(); if (!m_is_dirty) return; banmap = m_banned; - SetBannedSetDirty(false); + m_is_dirty = false; } int64_t n_start = GetTimeMillis(); if (!m_ban_db.Write(banmap)) { - SetBannedSetDirty(true); + LOCK(m_cs_banned); + m_is_dirty = true; } LogPrint(BCLog::NET, "Flushed %d banned node addresses/subnets to disk %dms\n", banmap.size(), @@ -202,9 +203,3 @@ void BanMan::SweepBanned() m_client_interface->BannedListChanged(); } } - -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 7f3c74733ed16..7a032dfdd0109 100644 --- a/src/banman.h +++ b/src/banman.h @@ -81,8 +81,6 @@ class BanMan private: void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned); - //!set the "dirty" flag for the banlist - void SetBannedSetDirty(bool dirty = true); //!clean unused entries (if bantime has expired) void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_cs_banned); From 784c316f9cb664c9577cbfed1873bae573efd1b4 Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Tue, 18 Jan 2022 03:26:37 -0300 Subject: [PATCH 0003/1751] scripted-diff: rename m_cs_banned -> m_banned_mutex -BEGIN VERIFY SCRIPT- s() { sed -i 's/m_cs_banned/m_banned_mutex/g' $1; } s src/banman.cpp s src/banman.h -END VERIFY SCRIPT- --- src/banman.cpp | 24 ++++++++++++------------ src/banman.h | 12 ++++++------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/banman.cpp b/src/banman.cpp index 5b2a1795433bf..3286ca8965d25 100644 --- a/src/banman.cpp +++ b/src/banman.cpp @@ -27,7 +27,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); @@ -51,7 +51,7 @@ void BanMan::DumpBanlist() banmap_t banmap; { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); SweepBanned(); if (!m_is_dirty) return; banmap = m_banned; @@ -60,7 +60,7 @@ void BanMan::DumpBanlist() int64_t n_start = GetTimeMillis(); if (!m_ban_db.Write(banmap)) { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); m_is_dirty = true; } @@ -71,7 +71,7 @@ void BanMan::DumpBanlist() void BanMan::ClearBanned() { { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); m_banned.clear(); m_is_dirty = true; } @@ -81,14 +81,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 +103,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 +122,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 +139,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 +161,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 +172,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 +180,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; diff --git a/src/banman.h b/src/banman.h index 7a032dfdd0109..e03772274443b 100644 --- a/src/banman.h +++ b/src/banman.h @@ -80,17 +80,17 @@ class BanMan void DumpBanlist(); private: - void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned); + 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}; + RecursiveMutex 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 From 0fb29087080a4e60d7c709ff5edf14e830ef3a69 Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Tue, 18 Jan 2022 03:29:14 -0300 Subject: [PATCH 0004/1751] refactor: replace RecursiveMutex m_banned_mutex with Mutex --- src/banman.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/banman.h b/src/banman.h index e03772274443b..c0e07b866dcfb 100644 --- a/src/banman.h +++ b/src/banman.h @@ -84,7 +84,7 @@ class BanMan //!clean unused entries (if bantime has expired) void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_banned_mutex); - RecursiveMutex m_banned_mutex; + 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; From 37d150d8c5ffcb2bddcd99951a739e97571194c7 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 24 May 2022 10:27:30 +0200 Subject: [PATCH 0005/1751] refactor: Add more negative `!m_banned_mutex` thread safety annotations Could be verified with $ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative' $ make clean $ make 2>&1 | grep m_banned_mutex --- src/banman.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/banman.h b/src/banman.h index c0e07b866dcfb..9200f07aaf408 100644 --- a/src/banman.h +++ b/src/banman.h @@ -60,24 +60,24 @@ 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_banned_mutex); From d90ad5a42ec6f48d0e504edc16d41c8ef266cc1d Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sat, 21 Aug 2021 20:22:41 +0300 Subject: [PATCH 0006/1751] build: Include qt sources for parsing with extract_strings.py --- src/Makefile.qt.include | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include index 72037b3db2df4..e57b064679a2e 100644 --- a/src/Makefile.qt.include +++ b/src/Makefile.qt.include @@ -357,7 +357,10 @@ SECONDARY: $(QT_QM) $(srcdir)/qt/bitcoinstrings.cpp: FORCE @test -n $(XGETTEXT) || echo "xgettext is required for updating translations" - $(AM_V_GEN) cd $(srcdir); XGETTEXT=$(XGETTEXT) COPYRIGHT_HOLDERS="$(COPYRIGHT_HOLDERS)" $(PYTHON) ../share/qt/extract_strings_qt.py $(libbitcoin_node_a_SOURCES) $(libbitcoin_wallet_a_SOURCES) $(libbitcoin_common_a_SOURCES) $(libbitcoin_zmq_a_SOURCES) $(libbitcoin_consensus_a_SOURCES) $(libbitcoin_util_a_SOURCES) + $(AM_V_GEN) cd $(srcdir); XGETTEXT=$(XGETTEXT) COPYRIGHT_HOLDERS="$(COPYRIGHT_HOLDERS)" $(PYTHON) ../share/qt/extract_strings_qt.py \ + $(libbitcoin_node_a_SOURCES) $(libbitcoin_wallet_a_SOURCES) $(libbitcoin_common_a_SOURCES) \ + $(libbitcoin_zmq_a_SOURCES) $(libbitcoin_consensus_a_SOURCES) $(libbitcoin_util_a_SOURCES) \ + $(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM) # The resulted bitcoin_en.xlf source file should follow Transifex requirements. # See: https://docs.transifex.com/formats/xliff#how-to-distinguish-between-a-source-file-and-a-translation-file From b59b31ae0b04054c5cf225dad87046d3771707fc Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sat, 21 Aug 2021 20:45:09 +0300 Subject: [PATCH 0007/1751] build: Drop redundant qt/bitcoin.cpp This file was included in #9457, but now it is a part of the BITCOIN_QT_BASE_CPP. --- src/Makefile.qt.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include index e57b064679a2e..658fa7df55c15 100644 --- a/src/Makefile.qt.include +++ b/src/Makefile.qt.include @@ -364,7 +364,7 @@ $(srcdir)/qt/bitcoinstrings.cpp: FORCE # The resulted bitcoin_en.xlf source file should follow Transifex requirements. # See: https://docs.transifex.com/formats/xliff#how-to-distinguish-between-a-source-file-and-a-translation-file -translate: $(srcdir)/qt/bitcoinstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) qt/bitcoin.cpp $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM) +translate: $(srcdir)/qt/bitcoinstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM) @test -n $(LUPDATE) || echo "lupdate is required for updating translations" $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(LUPDATE) -no-obsolete -I $(srcdir) -locations relative $^ -ts $(srcdir)/qt/locale/bitcoin_en.ts @test -n $(LCONVERT) || echo "lconvert is required for updating translations" From a5e39d325da4eeb9273fb7c919fcbfbc721ed75d Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Sat, 13 Feb 2021 17:38:34 +1000 Subject: [PATCH 0008/1751] Fee estimation: extend bucket ranges consistently When calculating a median fee for a confirmation target at a particular threshold, we analyse buckets in ranges rather than individually in case some buckets have very little data. This patch ensures the breaks between ranges are independent of the the confirmation target. --- src/policy/fees.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 2b940be07ed07..6a83f4980a9ba 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -259,6 +259,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; @@ -274,6 +279,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++) @@ -283,7 +289,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 From 7fe537f7a48675b1d25542bee6f390d665547580 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A8le=20Oul=C3=A8s?= Date: Tue, 18 Oct 2022 11:20:06 +0200 Subject: [PATCH 0009/1751] Implement CCoinsViewErrorCatcher::HaveCoin --- src/coins.cpp | 16 +++++++++++++--- src/coins.h | 1 + 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 5983a8a39fc75..37dd71874be07 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -292,11 +292,13 @@ const Coin& AccessByTxid(const CCoinsViewCache& view, const uint256& txid) return coinEmpty; } -bool CCoinsViewErrorCatcher::GetCoin(const COutPoint &outpoint, Coin &coin) const { +template +static bool ExecuteBackedWrapper(Func func, const std::vector>& err_callbacks) +{ try { - return CCoinsViewBacked::GetCoin(outpoint, coin); + return func(); } catch(const std::runtime_error& e) { - for (const auto& f : m_err_callbacks) { + for (const auto& f : err_callbacks) { f(); } LogPrintf("Error reading from database: %s\n", e.what()); @@ -307,3 +309,11 @@ bool CCoinsViewErrorCatcher::GetCoin(const COutPoint &outpoint, Coin &coin) cons std::abort(); } } + +bool CCoinsViewErrorCatcher::GetCoin(const COutPoint &outpoint, Coin &coin) const { + return ExecuteBackedWrapper([&]() { return CCoinsViewBacked::GetCoin(outpoint, coin); }, m_err_callbacks); +} + +bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint &outpoint) const { + return ExecuteBackedWrapper([&]() { return CCoinsViewBacked::HaveCoin(outpoint); }, m_err_callbacks); +} diff --git a/src/coins.h b/src/coins.h index 67fecc9785ddc..a4f477638635f 100644 --- a/src/coins.h +++ b/src/coins.h @@ -349,6 +349,7 @@ class CCoinsViewErrorCatcher final : public CCoinsViewBacked } bool GetCoin(const COutPoint &outpoint, Coin &coin) const override; + bool HaveCoin(const COutPoint &outpoint) const override; private: /** A list of callbacks to execute upon leveldb read error. */ From ed52e71176fc97c6ed01e3eebd85acdec54b4448 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A8le=20Oul=C3=A8s?= Date: Tue, 18 Oct 2022 10:59:37 +0200 Subject: [PATCH 0010/1751] Periodically check disk space to avoid corruption --- src/init.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/init.cpp b/src/init.cpp index 8ffab64622673..57646c3eebaba 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1159,6 +1159,15 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) RandAddPeriodic(); }, std::chrono::minutes{1}); + // Check disk space every 5 minutes to avoid db corruption. + node.scheduler->scheduleEvery([&args]{ + constexpr uint64_t min_disk_space = 50 << 20; // 50 MB + if (!CheckDiskSpace(args.GetBlocksDirPath(), min_disk_space)) { + LogPrintf("Shutting down due to lack of disk space!\n"); + StartShutdown(); + } + }, std::chrono::minutes{5}); + GetMainSignals().RegisterBackgroundSignalScheduler(*node.scheduler); // Create client interfaces for wallets that are supposed to be loaded From 0e21b56a44d53cec9080edb04410a692717f1ddc Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Thu, 5 Jan 2023 17:35:14 -0500 Subject: [PATCH 0011/1751] assumeutxo: catch and log fs::remove error instead of two exist checks --- src/validation.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index e24d39170e17c..e1ba8b96d263f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4859,15 +4859,15 @@ static bool DeleteCoinsDBFromDisk(const fs::path db_path, bool is_snapshot) if (is_snapshot) { fs::path base_blockhash_path = db_path / node::SNAPSHOT_BLOCKHASH_FILENAME; - if (fs::exists(base_blockhash_path)) { - bool removed = fs::remove(base_blockhash_path); - if (!removed) { - LogPrintf("[snapshot] failed to remove file %s\n", - fs::PathToString(base_blockhash_path)); + try { + bool existed = fs::remove(base_blockhash_path); + if (!existed) { + LogPrintf("[snapshot] snapshot chainstate dir being removed lacks %s file\n", + fs::PathToString(node::SNAPSHOT_BLOCKHASH_FILENAME)); } - } else { - LogPrintf("[snapshot] snapshot chainstate dir being removed lacks %s file\n", - fs::PathToString(node::SNAPSHOT_BLOCKHASH_FILENAME)); + } catch (const fs::filesystem_error& e) { + LogPrintf("[snapshot] failed to remove file %s: %s\n", + fs::PathToString(base_blockhash_path), fsbridge::get_filesystem_error_message(e)); } } From 541012e621386cd824eed81295206a34ba3ba497 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 3 Feb 2023 21:42:57 +0100 Subject: [PATCH 0012/1751] Build: Use AM_V_GEN in Makefiles where appropriate When generating new files as part of the Makefile the recipe is sometimes suppressed with $(AM_V_GEN) and sometimes with `@`. We should prefer $(AM_V_GEN), since this also prints the lines in silent mode. This is arguably more in style with the current recipe echoing. Before: Generated test/data/script_tests.json.h Now: GEN test/data/script_tests.json.h A side effect of this change is that the recipe for generating build.h is now echoed on each make run. Arguably this makes its generation more transparent. --- src/Makefile.am | 5 ++--- src/Makefile.test.include | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 5830090ada072..39f5a29aa6c6a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -346,7 +346,7 @@ BITCOIN_CORE_H = \ obj/build.h: FORCE @$(MKDIR_P) $(builddir)/obj - @$(top_srcdir)/share/genbuild.sh "$(abs_top_builddir)/src/obj/build.h" \ + $(AM_V_GEN) $(top_srcdir)/share/genbuild.sh "$(abs_top_builddir)/src/obj/build.h" \ "$(abs_top_srcdir)" libbitcoin_util_a-clientversion.$(OBJEXT): obj/build.h @@ -1085,12 +1085,11 @@ endif %.raw.h: %.raw @$(MKDIR_P) $(@D) - @{ \ + $(AM_V_GEN) { \ echo "static unsigned const char $(*F)_raw[] = {" && \ $(HEXDUMP) -v -e '8/1 "0x%02x, "' -e '"\n"' $< | $(SED) -e 's/0x ,//g' && \ echo "};"; \ } > "$@.new" && mv -f "$@.new" "$@" - @echo "Generated $@" include Makefile.minisketch.include diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 4d867fdc2f1fd..1c03c6892bbbb 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -414,10 +414,9 @@ endif %.json.h: %.json @$(MKDIR_P) $(@D) - @{ \ + $(AM_V_GEN) { \ echo "namespace json_tests{" && \ echo "static unsigned const char $(*F)[] = {" && \ $(HEXDUMP) -v -e '8/1 "0x%02x, "' -e '"\n"' $< | $(SED) -e 's/0x ,//g' && \ echo "};};"; \ } > "$@.new" && mv -f "$@.new" "$@" - @echo "Generated $@" From 1b1ffbd014b931afb9435ec10911b9a7c130d3e5 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 3 Feb 2023 22:18:54 +0100 Subject: [PATCH 0013/1751] Build: Log when test -f fails in Makefile Silently emitting an error makes it a bit harder to debug. Instead, print a helpful log message to point the developer in the right direction. Alternatively this could have been implemented by just removing the recipe echo suppression (@), but the subsequent make output became too noisy. --- src/Makefile.am | 2 +- src/Makefile.qt.include | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 39f5a29aa6c6a..7246a61891d4c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1030,7 +1030,7 @@ clean-local: -rm -rf test/__pycache__ .rc.o: - @test -f $(WINDRES) + @test -f $(WINDRES) || (echo "windres $(WINDRES) not found, but is required to compile windows resource files"; exit 1) ## FIXME: How to get the appropriate modulename_CPPFLAGS in here? $(AM_V_GEN) $(WINDRES) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(CPPFLAGS) -DWINDRES_PREPROC -i $< -o $@ diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include index 602a1182598c3..7852d1a2fa901 100644 --- a/src/Makefile.qt.include +++ b/src/Makefile.qt.include @@ -371,13 +371,13 @@ translate: $(srcdir)/qt/bitcoinstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCO @rm -f $(srcdir)/qt/locale/bitcoin_en.xlf.old $(QT_QRC_LOCALE_CPP): $(QT_QRC_LOCALE) $(QT_QM) - @test -f $(RCC) + @test -f $(RCC) || (echo "rcc $(RCC) not found, but is required for generating qrc cpp files"; exit 1) @cp -f $< $(@D)/temp_$( $@ @rm $(@D)/temp_$( $@ CLEAN_QT = $(nodist_qt_libbitcoinqt_a_SOURCES) $(QT_QM) $(QT_FORMS_H) qt/*.gcda qt/*.gcno qt/temp_bitcoin_locale.qrc @@ -404,7 +404,7 @@ bitcoin_qt_apk: FORCE cd qt/android && ./gradlew build ui_%.h: %.ui - @test -f $(UIC) + @test -f $(UIC) || (echo "uic $(UIC) not found, but is required for generating ui headers"; exit 1) @$(MKDIR_P) $(@D) $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(UIC) -o $@ $< || (echo "Error creating $@"; false) @@ -415,6 +415,6 @@ moc_%.cpp: %.h $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(MOC) $(DEFAULT_INCLUDES) $(QT_INCLUDES_UNSUPPRESSED) $(MOC_DEFS) $< > $@ %.qm: %.ts - @test -f $(LRELEASE) + @test -f $(LRELEASE) || (echo "lrelease $(LRELEASE) not found, but is required for generating translations"; exit 1) @$(MKDIR_P) $(@D) $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(LRELEASE) -silent $< -qm $@ From 3df37e0c78c3d5139c963a74eda56c331355ef72 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 17 Feb 2023 11:37:56 +0100 Subject: [PATCH 0014/1751] doc: clarify that LOCK() does AssertLockNotHeld() internally Constructs like ```cpp AssertLockNotHeld(m); LOCK(m); ``` are equivalent to ```cpp LOCK(m); ``` for non-recursive mutexes, so it is ok to omit `AssertLockNotHeld()` in such cases. --- doc/developer-notes.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index e2e54e13d397b..d41543ab1c569 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -941,7 +941,9 @@ Threads and synchronization internal to a class (private or protected) rather than public. - Combine annotations in function declarations with run-time asserts in - function definitions: + function definitions (`AssertLockNotHeld()` can be omitted if `LOCK()` is + called unconditionally after it because `LOCK()` does the same check as + `AssertLockNotHeld()` internally, for non-recursive mutexes): ```C++ // txmempool.h From 91d08889218e06631f43a3dab0bae576aa46e43c Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 16 Feb 2023 14:33:57 +0100 Subject: [PATCH 0015/1751] sync: unpublish LocksHeld() which is used only in sync.cpp --- src/sync.cpp | 2 +- src/sync.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index 46218056539be..58752a9f182de 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -246,7 +246,7 @@ void LeaveCritical() pop_lock(); } -std::string LocksHeld() +static std::string LocksHeld() { LockData& lockdata = GetLockData(); std::lock_guard lock(lockdata.dd_mutex); diff --git a/src/sync.h b/src/sync.h index 7242a793abe46..09ec0d1255daf 100644 --- a/src/sync.h +++ b/src/sync.h @@ -57,7 +57,6 @@ template void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false); void LeaveCritical(); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line); -std::string LocksHeld(); template void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs); template From 65e3abcbf2b9e818f3b9f1ba35f3cfe7df5e3811 Mon Sep 17 00:00:00 2001 From: willcl-ark Date: Tue, 7 Mar 2023 22:53:23 +0000 Subject: [PATCH 0016/1751] doc: document json rpc endpoints fixes #20246 Document both JSON-RPC endpoints, when they are active and which types of requests they are able to service. Adds two example curl requests, one for each endpoint. --- doc/JSON-RPC-interface.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/doc/JSON-RPC-interface.md b/doc/JSON-RPC-interface.md index ab5db58cdd181..6cbb6ebd721ac 100644 --- a/doc/JSON-RPC-interface.md +++ b/doc/JSON-RPC-interface.md @@ -5,6 +5,41 @@ The headless daemon `bitcoind` has the JSON-RPC API enabled by default, the GUI option. In the GUI it is possible to execute RPC methods in the Debug Console Dialog. +## Endpoints + +There are two JSON-RPC endpoints on the server: + +1. `/` +2. `/wallet//` + +### `/` endpoint + +This endpoint is always active. +It can always service non-wallet requests and can service wallet requests when +exactly one wallet is loaded. + +### `/wallet//` endpoint + +This endpoint is only activated when the wallet component has been compiled in. +It can service both wallet and non-wallet requests. +It MUST be used for wallet requests when two or more wallets are loaded. + +This is the endpoint used by bitcoin-cli when a `-rpcwallet=` parameter is passed in. + +Best practice would dictate using the `/wallet//` endpoint for ALL +requests when multiple wallets are in use. + +### Examples + +```sh +# Get block count from the / endpoint when rpcuser=alice and rpcport=38332 +$ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getblockcount", "params": []}' -H 'content-type: text/plain;' localhost:38332/ + +# Get balance from the /wallet/walletname endpoint when rpcuser=alice, rpcport=38332 and rpcwallet=desc-wallet +$ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getbalance", "params": []}' -H 'content-type: text/plain;' localhost:38332/wallet/desc-wallet + +``` + ## Parameter passing The JSON-RPC server supports both _by-position_ and _by-name_ [parameter From 6c8bde6d54d03224709dce54b8ba32b8c3e37ac7 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 3 Mar 2023 15:07:06 +0000 Subject: [PATCH 0017/1751] test: move coverage on ParseNonRFCJSONValue() to UniValue::read() Preparation to deprecate ParseNonRFCJSONValue() but keep test coverage on the underlying UniValue::read() unaffected. The test coverage on AmountFromValue is no longer included, since that is already tested in the rpc_parse_monetary_values test case. Fuzzing coverage on ParseNonRFCJSONValue() was duplicated between string.cpp and parse_univalue.cpp, only the one in parse_univalue.cpp is kept. --- src/test/fuzz/parse_univalue.cpp | 9 +++------ src/test/fuzz/string.cpp | 4 ---- src/test/rpc_tests.cpp | 31 +------------------------------ src/univalue/test/object.cpp | 27 +++++++++++++++++++++++++++ 4 files changed, 31 insertions(+), 40 deletions(-) diff --git a/src/test/fuzz/parse_univalue.cpp b/src/test/fuzz/parse_univalue.cpp index 16486f6b96d20..5b95c20928196 100644 --- a/src/test/fuzz/parse_univalue.cpp +++ b/src/test/fuzz/parse_univalue.cpp @@ -21,12 +21,9 @@ FUZZ_TARGET_INIT(parse_univalue, initialize_parse_univalue) const std::string random_string(buffer.begin(), buffer.end()); bool valid = true; const UniValue univalue = [&] { - try { - return ParseNonRFCJSONValue(random_string); - } catch (const std::runtime_error&) { - valid = false; - return UniValue{}; - } + UniValue uv; + if (!uv.read(random_string)) valid = false; + return valid ? uv : UniValue{}; }(); if (!valid) { return; diff --git a/src/test/fuzz/string.cpp b/src/test/fuzz/string.cpp index 9890e4c0e54fd..5634c02b2469e 100644 --- a/src/test/fuzz/string.cpp +++ b/src/test/fuzz/string.cpp @@ -159,10 +159,6 @@ FUZZ_TARGET(string) const util::Settings settings; (void)OnlyHasDefaultSectionSetting(settings, random_string_1, random_string_2); (void)ParseNetwork(random_string_1); - try { - (void)ParseNonRFCJSONValue(random_string_1); - } catch (const std::runtime_error&) { - } (void)ParseOutputType(random_string_1); (void)RemovePrefix(random_string_1, random_string_2); (void)ResolveErrMsg(random_string_1, random_string_2); diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index 791c9ddf31203..9d380595f1924 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -278,6 +278,7 @@ BOOST_AUTO_TEST_CASE(rpc_parse_monetary_values) BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.00000001000000")), 1LL); //should pass, cut trailing 0 BOOST_CHECK_THROW(AmountFromValue(ValueFromString("19e-9")), UniValue); //should fail BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.19e-6")), 19); //should pass, leading 0 is present + BOOST_CHECK_EXCEPTION(AmountFromValue(".19e-6"), UniValue, HasJSON(R"({"code":-3,"message":"Invalid amount"})")); //should fail, no leading 0 BOOST_CHECK_THROW(AmountFromValue(ValueFromString("92233720368.54775808")), UniValue); //overflow error BOOST_CHECK_THROW(AmountFromValue(ValueFromString("1e+11")), UniValue); //overflow error @@ -285,36 +286,6 @@ BOOST_AUTO_TEST_CASE(rpc_parse_monetary_values) BOOST_CHECK_THROW(AmountFromValue(ValueFromString("93e+9")), UniValue); //overflow error } -BOOST_AUTO_TEST_CASE(json_parse_errors) -{ - // Valid - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("1.0").get_real(), 1.0); - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("true").get_bool(), true); - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("[false]")[0].get_bool(), false); - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("{\"a\": true}")["a"].get_bool(), true); - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("{\"1\": \"true\"}")["1"].get_str(), "true"); - // Valid, with leading or trailing whitespace - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue(" 1.0").get_real(), 1.0); - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("1.0 ").get_real(), 1.0); - - BOOST_CHECK_THROW(AmountFromValue(ParseNonRFCJSONValue(".19e-6")), std::runtime_error); //should fail, missing leading 0, therefore invalid JSON - BOOST_CHECK_EQUAL(AmountFromValue(ParseNonRFCJSONValue("0.00000000000000000000000000000000000001e+30 ")), 1); - // Invalid, initial garbage - BOOST_CHECK_THROW(ParseNonRFCJSONValue("[1.0"), std::runtime_error); - BOOST_CHECK_THROW(ParseNonRFCJSONValue("a1.0"), std::runtime_error); - // Invalid, trailing garbage - BOOST_CHECK_THROW(ParseNonRFCJSONValue("1.0sds"), std::runtime_error); - BOOST_CHECK_THROW(ParseNonRFCJSONValue("1.0]"), std::runtime_error); - // Invalid, keys have to be names - BOOST_CHECK_THROW(ParseNonRFCJSONValue("{1: \"true\"}"), std::runtime_error); - BOOST_CHECK_THROW(ParseNonRFCJSONValue("{true: 1}"), std::runtime_error); - BOOST_CHECK_THROW(ParseNonRFCJSONValue("{[1]: 1}"), std::runtime_error); - BOOST_CHECK_THROW(ParseNonRFCJSONValue("{{\"a\": \"a\"}: 1}"), std::runtime_error); - // BTC addresses should fail parsing - BOOST_CHECK_THROW(ParseNonRFCJSONValue("175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W"), std::runtime_error); - BOOST_CHECK_THROW(ParseNonRFCJSONValue("3J98t1WpEZ73CNmQviecrnyiWrnqRhWNL"), std::runtime_error); -} - BOOST_AUTO_TEST_CASE(rpc_ban) { BOOST_CHECK_NO_THROW(CallRPC(std::string("clearbanned"))); diff --git a/src/univalue/test/object.cpp b/src/univalue/test/object.cpp index 5ddf30039360b..5fb973c67bfb3 100644 --- a/src/univalue/test/object.cpp +++ b/src/univalue/test/object.cpp @@ -412,6 +412,33 @@ void univalue_readwrite() BOOST_CHECK_EQUAL(strJson1, v.write()); + // Valid + BOOST_CHECK(v.read("1.0") && (v.get_real() == 1.0)); + BOOST_CHECK(v.read("true") && v.get_bool()); + BOOST_CHECK(v.read("[false]") && !v[0].get_bool()); + BOOST_CHECK(v.read("{\"a\": true}") && v["a"].get_bool()); + BOOST_CHECK(v.read("{\"1\": \"true\"}") && (v["1"].get_str() == "true")); + // Valid, with leading or trailing whitespace + BOOST_CHECK(v.read(" 1.0") && (v.get_real() == 1.0)); + BOOST_CHECK(v.read("1.0 ") && (v.get_real() == 1.0)); + BOOST_CHECK(v.read("0.00000000000000000000000000000000000001e+30 ") && v.get_real() == 1e-8); + + BOOST_CHECK(!v.read(".19e-6")); //should fail, missing leading 0, therefore invalid JSON + // Invalid, initial garbage + BOOST_CHECK(!v.read("[1.0")); + BOOST_CHECK(!v.read("a1.0")); + // Invalid, trailing garbage + BOOST_CHECK(!v.read("1.0sds")); + BOOST_CHECK(!v.read("1.0]")); + // Invalid, keys have to be names + BOOST_CHECK(!v.read("{1: \"true\"}")); + BOOST_CHECK(!v.read("{true: 1}")); + BOOST_CHECK(!v.read("{[1]: 1}")); + BOOST_CHECK(!v.read("{{\"a\": \"a\"}: 1}")); + // BTC addresses should fail parsing + BOOST_CHECK(!v.read("175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W")); + BOOST_CHECK(!v.read("3J98t1WpEZ73CNmQviecrnyiWrnqRhWNL")); + /* Check for (correctly reporting) a parsing error if the initial JSON construct is followed by more stuff. Note that whitespace is, of course, exempt. */ From cfbc8a623b5133f1d0b0c0c9be73b2b107e0d687 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 3 Mar 2023 15:07:31 +0000 Subject: [PATCH 0018/1751] refactor: rpc: hide and rename ParseNonRFCJSONValue() As per https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059, this function is no longer necessary and we can use UniValue::read() directly. To avoid code duplication, we keep the function to throw on invalid input data but rename it to Parse() and remove it from the header. --- src/rpc/client.cpp | 22 ++++++++++------------ src/rpc/client.h | 5 ----- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 9449b9d197ef7..2b517e77c81a9 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -222,6 +222,14 @@ static const CRPCConvertParam vRPCConvertParams[] = }; // clang-format on +/** Parse string to UniValue or throw runtime_error if string contains invalid JSON */ +static UniValue Parse(std::string_view raw) +{ + UniValue parsed; + if (!parsed.read(raw)) throw std::runtime_error(tfm::format("Error parsing JSON: %s", raw)); + return parsed; +} + class CRPCConvertTable { private: @@ -234,13 +242,13 @@ class CRPCConvertTable /** Return arg_value as UniValue, and first parse it if it is a non-string parameter */ UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, int param_idx) { - return members.count({method, param_idx}) > 0 ? ParseNonRFCJSONValue(arg_value) : arg_value; + return members.count({method, param_idx}) > 0 ? Parse(arg_value) : arg_value; } /** Return arg_value as UniValue, and first parse it if it is a non-string parameter */ UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, const std::string& param_name) { - return membersByName.count({method, param_name}) > 0 ? ParseNonRFCJSONValue(arg_value) : arg_value; + return membersByName.count({method, param_name}) > 0 ? Parse(arg_value) : arg_value; } }; @@ -254,16 +262,6 @@ CRPCConvertTable::CRPCConvertTable() static CRPCConvertTable rpcCvtTable; -/** Non-RFC4627 JSON parser, accepts internal values (such as numbers, true, false, null) - * as well as objects and arrays. - */ -UniValue ParseNonRFCJSONValue(std::string_view raw) -{ - UniValue parsed; - if (!parsed.read(raw)) throw std::runtime_error(tfm::format("Error parsing JSON: %s", raw)); - return parsed; -} - UniValue RPCConvertValues(const std::string &strMethod, const std::vector &strParams) { UniValue params(UniValue::VARR); diff --git a/src/rpc/client.h b/src/rpc/client.h index 3c5c4fc4d6239..b67cd27fdfef0 100644 --- a/src/rpc/client.h +++ b/src/rpc/client.h @@ -17,9 +17,4 @@ UniValue RPCConvertValues(const std::string& strMethod, const std::vector& strParams); -/** Non-RFC4627 JSON parser, accepts internal values (such as numbers, true, false, null) - * as well as objects and arrays. - */ -UniValue ParseNonRFCJSONValue(std::string_view raw); - #endif // BITCOIN_RPC_CLIENT_H From d380d2877ed45cf1e75a87d822b30e4e1e21e3d4 Mon Sep 17 00:00:00 2001 From: Martin Leitner-Ankerl Date: Sun, 26 Mar 2023 15:19:43 +0200 Subject: [PATCH 0019/1751] bench: Add benchmark for prevector usage in std::vector --- src/bench/prevector.cpp | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/bench/prevector.cpp b/src/bench/prevector.cpp index 59c4af086e314..2524e215e4c2d 100644 --- a/src/bench/prevector.cpp +++ b/src/bench/prevector.cpp @@ -80,6 +80,30 @@ static void PrevectorDeserialize(benchmark::Bench& bench) }); } +template +static void PrevectorFillVectorDirect(benchmark::Bench& bench) +{ + bench.run([&] { + std::vector> vec; + for (size_t i = 0; i < 260; ++i) { + vec.emplace_back(); + } + }); +} + + +template +static void PrevectorFillVectorIndirect(benchmark::Bench& bench) +{ + bench.run([&] { + std::vector> vec; + for (size_t i = 0; i < 260; ++i) { + // force allocation + vec.emplace_back(29, T{}); + } + }); +} + #define PREVECTOR_TEST(name) \ static void Prevector##name##Nontrivial(benchmark::Bench& bench) \ { \ @@ -96,3 +120,5 @@ PREVECTOR_TEST(Clear) PREVECTOR_TEST(Destructor) PREVECTOR_TEST(Resize) PREVECTOR_TEST(Deserialize) +PREVECTOR_TEST(FillVectorDirect) +PREVECTOR_TEST(FillVectorIndirect) From 81f67977f543faca2dcc35846f73e2917375ae79 Mon Sep 17 00:00:00 2001 From: Martin Leitner-Ankerl Date: Sun, 26 Mar 2023 15:39:20 +0200 Subject: [PATCH 0020/1751] util: prevector's move ctor and move assignment is `noexcept` Move operations already are `noexcept`, so add the keyword to the methods. This makes the `PrevectorFillVectorIndirect...` benchmarks about twice as fast on my machine, because otherwise `std::vector` has to use a copy when the vector resizes. --- src/prevector.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/prevector.h b/src/prevector.h index f36cfe4ff6cda..d3e4b8fd0d28f 100644 --- a/src/prevector.h +++ b/src/prevector.h @@ -264,7 +264,7 @@ class prevector { fill(item_ptr(0), other.begin(), other.end()); } - prevector(prevector&& other) { + prevector(prevector&& other) noexcept { swap(other); } @@ -276,7 +276,7 @@ class prevector { return *this; } - prevector& operator=(prevector&& other) { + prevector& operator=(prevector&& other) noexcept { swap(other); return *this; } From fffc86f49f4eeb811b8438bc1b7f8d9e05882c6f Mon Sep 17 00:00:00 2001 From: Martin Leitner-Ankerl Date: Sun, 26 Mar 2023 15:39:53 +0200 Subject: [PATCH 0021/1751] test: CScriptCheck is used a lot in std::vector, make sure that's efficient Adds a few static_asserts so CScriptCheck stays is_nothrow_move_assignable, is_nothrow_move_constructible, and is_nothrow_destructible --- src/validation.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/validation.h b/src/validation.h index aba863db093e5..b6d1995c5bb45 100644 --- a/src/validation.h +++ b/src/validation.h @@ -42,6 +42,7 @@ #include #include #include +#include #include #include @@ -331,6 +332,11 @@ class CScriptCheck ScriptError GetScriptError() const { return error; } }; +// CScriptCheck is used a lot in std::vector, make sure that's efficient +static_assert(std::is_nothrow_move_assignable_v); +static_assert(std::is_nothrow_move_constructible_v); +static_assert(std::is_nothrow_destructible_v); + /** Initializes the script-execution cache */ [[nodiscard]] bool InitScriptExecutionCache(size_t max_size_bytes); From bfb9291a8661fe5b26c14ed755cfa89d27c37110 Mon Sep 17 00:00:00 2001 From: Martin Leitner-Ankerl Date: Sun, 26 Mar 2023 14:40:32 +0200 Subject: [PATCH 0022/1751] util: implement prevector's move ctor & move assignment Using swap() was rather wasteful because it had to copy the whole direct memory data twice. Also, due to the swap() in move assignment the moved-from object might hold on to unused memory for longer than necessary. --- src/prevector.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/prevector.h b/src/prevector.h index d3e4b8fd0d28f..bcab1ff00cd22 100644 --- a/src/prevector.h +++ b/src/prevector.h @@ -264,8 +264,10 @@ class prevector { fill(item_ptr(0), other.begin(), other.end()); } - prevector(prevector&& other) noexcept { - swap(other); + prevector(prevector&& other) noexcept + : _union(std::move(other._union)), _size(other._size) + { + other._size = 0; } prevector& operator=(const prevector& other) { @@ -277,7 +279,12 @@ class prevector { } prevector& operator=(prevector&& other) noexcept { - swap(other); + if (!is_direct()) { + free(_union.indirect_contents.indirect); + } + _union = std::move(other._union); + _size = other._size; + other._size = 0; return *this; } From 56484f0fdc44261e723563f59df886d5acdd851f Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 21 Feb 2022 11:48:10 +0000 Subject: [PATCH 0023/1751] =?UTF-8?q?[mempool]=20find=20connected=20mempoo?= =?UTF-8?q?l=20entries=20with=20GatherClusters(=E2=80=A6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We limit GatherClusters’s result to a maximum of 500 transactions as clusters can be made arbitrarily large by third parties. Co-authored-by: Murch --- src/txmempool.cpp | 42 +++++++++++++++++++++++++++++++++++++++++- src/txmempool.h | 15 ++++++++++++++- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 378123ce0febf..2bac419f84c70 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -898,6 +899,19 @@ CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set& hashes) c return ret; } +std::vector CTxMemPool::GetIterVec(const std::vector& txids) const +{ + AssertLockHeld(cs); + std::vector ret; + ret.reserve(txids.size()); + for (const auto& txid : txids) { + const auto it{GetIter(txid)}; + if (!it) return {}; + ret.push_back(*it); + } + return ret; +} + bool CTxMemPool::HasNoInputsOf(const CTransaction &tx) const { for (unsigned int i = 0; i < tx.vin.size(); i++) @@ -1127,7 +1141,6 @@ void CTxMemPool::SetLoadTried(bool load_tried) m_load_tried = load_tried; } - std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept { switch (r) { @@ -1140,3 +1153,30 @@ std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept } assert(false); } + +std::vector CTxMemPool::GatherClusters(const std::vector& txids) const +{ + AssertLockHeld(cs); + std::vector clustered_txs{GetIterVec(txids)}; + // Use epoch: visiting an entry means we have added it to the clustered_txs vector. It does not + // necessarily mean the entry has been processed. + WITH_FRESH_EPOCH(m_epoch); + for (const auto& it : clustered_txs) { + visited(it); + } + // i = index of where the list of entries to process starts + for (size_t i{0}; i < clustered_txs.size(); ++i) { + // DoS protection: if there are 500 or more entries to process, just quit. + if (clustered_txs.size() > 500) return {}; + const txiter& tx_iter = clustered_txs.at(i); + for (const auto& entries : {tx_iter->GetMemPoolParentsConst(), tx_iter->GetMemPoolChildrenConst()}) { + for (const CTxMemPoolEntry& entry : entries) { + const auto entry_it = mapTx.iterator_to(entry); + if (!visited(entry_it)) { + clustered_txs.push_back(entry_it); + } + } + } + } + return clustered_txs; +} diff --git a/src/txmempool.h b/src/txmempool.h index 2c3cb7e9dbd4d..769b7f69eac34 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -522,9 +522,16 @@ class CTxMemPool /** Returns an iterator to the given hash, if found */ std::optional GetIter(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs); - /** Translate a set of hashes into a set of pool iterators to avoid repeated lookups */ + /** Translate a set of hashes into a set of pool iterators to avoid repeated lookups. + * Does not require that all of the hashes correspond to actual transactions in the mempool, + * only returns the ones that exist. */ setEntries GetIterSet(const std::set& hashes) const EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Translate a list of hashes into a list of mempool iterators to avoid repeated lookups. + * The nth element in txids becomes the nth element in the returned vector. If any of the txids + * don't actually exist in the mempool, returns an empty vector. */ + std::vector GetIterVec(const std::vector& txids) const EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Remove a set of transactions from the mempool. * If a transaction is in this set, then all in-mempool descendants must * also be in the set, unless this transaction is being removed for being @@ -585,6 +592,12 @@ class CTxMemPool const Limits& limits, bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Collect the entire cluster of connected transactions for each transaction in txids. + * All txids must correspond to transaction entries in the mempool, otherwise this returns an + * empty vector. This call will also exit early and return an empty vector if it collects 500 or + * more transactions as a DoS protection. */ + std::vector GatherClusters(const std::vector& txids) const EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Calculate all in-mempool ancestors of a set of transactions not already in the mempool and * check ancestor and descendant limits. Heuristics are used to estimate the ancestor and * descendant count of all entries if the package were to be added to the mempool. The limits From 552684976b6df34ce563458f73812e6e494e3b0e Mon Sep 17 00:00:00 2001 From: dimitaracev Date: Tue, 28 Mar 2023 22:43:18 +0200 Subject: [PATCH 0024/1751] validation: Move warningcache to ChainstateManager --- src/validation.cpp | 9 +-------- src/validation.h | 2 ++ 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index e82fead89e6fa..c09c85662e89c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2005,8 +2005,6 @@ class WarningBitsConditionChecker : public AbstractThresholdConditionChecker } }; -static std::array warningcache GUARDED_BY(cs_main); - static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const ChainstateManager& chainman) { const Consensus::Params& consensusparams = chainman.GetConsensus(); @@ -2662,7 +2660,7 @@ void Chainstate::UpdateTip(const CBlockIndex* pindexNew) const CBlockIndex* pindex = pindexNew; for (int bit = 0; bit < VERSIONBITS_NUM_BITS; bit++) { WarningBitsConditionChecker checker(m_chainman, bit); - ThresholdState state = checker.GetStateFor(pindex, params.GetConsensus(), warningcache.at(bit)); + ThresholdState state = checker.GetStateFor(pindex, params.GetConsensus(), m_chainman.m_warningcache.at(bit)); if (state == ThresholdState::ACTIVE || state == ThresholdState::LOCKED_IN) { const bilingual_str warning = strprintf(_("Unknown new rules activated (versionbit %i)"), bit); if (state == ThresholdState::ACTIVE) { @@ -5593,11 +5591,6 @@ ChainstateManager::~ChainstateManager() LOCK(::cs_main); m_versionbitscache.Clear(); - - // TODO: The warning cache should probably become non-global - for (auto& i : warningcache) { - i.clear(); - } } bool ChainstateManager::DetectSnapshotChainstate(CTxMemPool* mempool) diff --git a/src/validation.h b/src/validation.h index aba863db093e5..cb66b94954bcb 100644 --- a/src/validation.h +++ b/src/validation.h @@ -936,6 +936,8 @@ class ChainstateManager //! nullopt. std::optional GetSnapshotBaseHeight() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + std::array m_warningcache GUARDED_BY(::cs_main); + //! Return true if a chainstate is considered usable. //! //! This is false when a background validation chainstate has completed its From 59afcc83548ea67a863dac7b75d000bc8f6a7023 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 4 Aug 2022 15:37:50 +0100 Subject: [PATCH 0025/1751] Implement Mini version of BlockAssembler to calculate mining scores Rewrite the same algo instead of reusing BlockAssembler because we have a few extra requirements that would make the changes invasive and difficult to review: - Only operate on the relevant transactions rather than full mempool - Remove transactions that will be replaced so they can't bump their ancestors - Don't hold mempool lock outside of the constructor - Skip things like max block weight and IsFinalTx - Additionally calculate fees to bump remaining ancestor packages to target feerate Co-authored-by: Murch --- src/Makefile.am | 2 + src/node/mini_miner.cpp | 366 ++++++++++++++++++++++++++++++++++++++++ src/node/mini_miner.h | 121 +++++++++++++ 3 files changed, 489 insertions(+) create mode 100644 src/node/mini_miner.cpp create mode 100644 src/node/mini_miner.h diff --git a/src/Makefile.am b/src/Makefile.am index 5830090ada072..3f68ac03f09ef 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -211,6 +211,7 @@ BITCOIN_CORE_H = \ node/mempool_args.h \ node/mempool_persist_args.h \ node/miner.h \ + node/mini_miner.h \ node/minisketchwrapper.h \ node/psbt.h \ node/transaction.h \ @@ -396,6 +397,7 @@ libbitcoin_node_a_SOURCES = \ node/mempool_args.cpp \ node/mempool_persist_args.cpp \ node/miner.cpp \ + node/mini_miner.cpp \ node/minisketchwrapper.cpp \ node/psbt.cpp \ node/transaction.cpp \ diff --git a/src/node/mini_miner.cpp b/src/node/mini_miner.cpp new file mode 100644 index 0000000000000..71ae9d23c79de --- /dev/null +++ b/src/node/mini_miner.cpp @@ -0,0 +1,366 @@ +// 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 + +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +namespace node { + +MiniMiner::MiniMiner(const CTxMemPool& mempool, const std::vector& outpoints) +{ + LOCK(mempool.cs); + // Find which outpoints to calculate bump fees for. + // Anything that's spent by the mempool is to-be-replaced + // Anything otherwise unavailable just has a bump fee of 0 + for (const auto& outpoint : outpoints) { + if (!mempool.exists(GenTxid::Txid(outpoint.hash))) { + // This UTXO is either confirmed or not yet submitted to mempool. + // If it's confirmed, no bump fee is required. + // If it's not yet submitted, we have no information, so return 0. + m_bump_fees.emplace(outpoint, 0); + continue; + } + + // UXTO is created by transaction in mempool, add to map. + // Note: This will either create a missing entry or add the outpoint to an existing entry + m_requested_outpoints_by_txid[outpoint.hash].push_back(outpoint); + + if (const auto ptx{mempool.GetConflictTx(outpoint)}) { + // This outpoint is already being spent by another transaction in the mempool. We + // assume that the caller wants to replace this transaction and its descendants. It + // would be unusual for the transaction to have descendants as the wallet won’t normally + // attempt to replace transactions with descendants. If the outpoint is from a mempool + // transaction, we still need to calculate its ancestors bump fees (added to + // m_requested_outpoints_by_txid below), but after removing the to-be-replaced entries. + // + // Note that the descendants of a transaction include the transaction itself. Also note, + // that this is only calculating bump fees. RBF fee rules should be handled separately. + CTxMemPool::setEntries descendants; + mempool.CalculateDescendants(mempool.GetIter(ptx->GetHash()).value(), descendants); + for (const auto& desc_txiter : descendants) { + m_to_be_replaced.insert(desc_txiter->GetTx().GetHash()); + } + } + } + + // No unconfirmed UTXOs, so nothing mempool-related needs to be calculated. + if (m_requested_outpoints_by_txid.empty()) return; + + // Calculate the cluster and construct the entry map. + std::vector txids_needed; + txids_needed.reserve(m_requested_outpoints_by_txid.size()); + for (const auto& [txid, _]: m_requested_outpoints_by_txid) { + txids_needed.push_back(txid); + } + const auto cluster = mempool.GatherClusters(txids_needed); + if (cluster.empty()) { + // An empty cluster means that at least one of the transactions is missing from the mempool + // (should not be possible given processing above) or DoS limit was hit. + m_ready_to_calculate = false; + return; + } + + // Add every entry to m_entries_by_txid and m_entries, except the ones that will be replaced. + for (const auto& txiter : cluster) { + if (!m_to_be_replaced.count(txiter->GetTx().GetHash())) { + auto [mapiter, success] = m_entries_by_txid.emplace(txiter->GetTx().GetHash(), MiniMinerMempoolEntry(txiter)); + m_entries.push_back(mapiter); + } else { + auto outpoints_it = m_requested_outpoints_by_txid.find(txiter->GetTx().GetHash()); + if (outpoints_it != m_requested_outpoints_by_txid.end()) { + // This UTXO is the output of a to-be-replaced transaction. Bump fee is 0; spending + // this UTXO is impossible as it will no longer exist after the replacement. + for (const auto& outpoint : outpoints_it->second) { + m_bump_fees.emplace(outpoint, 0); + } + m_requested_outpoints_by_txid.erase(outpoints_it); + } + } + } + + // Build the m_descendant_set_by_txid cache. + for (const auto& txiter : cluster) { + const auto& txid = txiter->GetTx().GetHash(); + // Cache descendants for future use. Unlike the real mempool, a descendant MiniMinerMempoolEntry + // will not exist without its ancestor MiniMinerMempoolEntry, so these sets won't be invalidated. + std::vector cached_descendants; + const bool remove{m_to_be_replaced.count(txid) > 0}; + CTxMemPool::setEntries descendants; + mempool.CalculateDescendants(txiter, descendants); + Assume(descendants.count(txiter) > 0); + for (const auto& desc_txiter : descendants) { + const auto txid_desc = desc_txiter->GetTx().GetHash(); + const bool remove_desc{m_to_be_replaced.count(txid_desc) > 0}; + auto desc_it{m_entries_by_txid.find(txid_desc)}; + Assume((desc_it == m_entries_by_txid.end()) == remove_desc); + if (remove) Assume(remove_desc); + // It's possible that remove=false but remove_desc=true. + if (!remove && !remove_desc) { + cached_descendants.push_back(desc_it); + } + } + if (remove) { + Assume(cached_descendants.empty()); + } else { + m_descendant_set_by_txid.emplace(txid, cached_descendants); + } + } + + // Release the mempool lock; we now have all the information we need for a subset of the entries + // we care about. We will solely operate on the MiniMinerMempoolEntry map from now on. + Assume(m_in_block.empty()); + Assume(m_requested_outpoints_by_txid.size() <= outpoints.size()); + SanityCheck(); +} + +// Compare by min(ancestor feerate, individual feerate), then iterator +// +// Under the ancestor-based mining approach, high-feerate children can pay for parents, but high-feerate +// parents do not incentive inclusion of their children. Therefore the mining algorithm only considers +// transactions for inclusion on basis of the minimum of their own feerate or their ancestor feerate. +struct AncestorFeerateComparator +{ + template + bool operator()(const I& a, const I& b) const { + auto min_feerate = [](const MiniMinerMempoolEntry& e) -> CFeeRate { + const CAmount ancestor_fee{e.GetModFeesWithAncestors()}; + const int64_t ancestor_size{e.GetSizeWithAncestors()}; + const CAmount tx_fee{e.GetModifiedFee()}; + const int64_t tx_size{e.GetTxSize()}; + // Comparing ancestor feerate with individual feerate: + // ancestor_fee / ancestor_size <= tx_fee / tx_size + // Avoid division and possible loss of precision by + // multiplying both sides by the sizes: + return ancestor_fee * tx_size < tx_fee * ancestor_size ? + CFeeRate(ancestor_fee, ancestor_size) : + CFeeRate(tx_fee, tx_size); + }; + CFeeRate a_feerate{min_feerate(a->second)}; + CFeeRate b_feerate{min_feerate(b->second)}; + if (a_feerate != b_feerate) { + return a_feerate > b_feerate; + } + // Use txid as tiebreaker for stable sorting + return a->first < b->first; + } +}; + +void MiniMiner::DeleteAncestorPackage(const std::set& ancestors) +{ + Assume(ancestors.size() >= 1); + // "Mine" all transactions in this ancestor set. + for (auto& anc : ancestors) { + Assume(m_in_block.count(anc->first) == 0); + m_in_block.insert(anc->first); + m_total_fees += anc->second.GetModifiedFee(); + m_total_vsize += anc->second.GetTxSize(); + auto it = m_descendant_set_by_txid.find(anc->first); + // Each entry’s descendant set includes itself + Assume(it != m_descendant_set_by_txid.end()); + for (auto& descendant : it->second) { + // If these fail, we must be double-deducting. + Assume(descendant->second.GetModFeesWithAncestors() >= anc->second.GetModifiedFee()); + Assume(descendant->second.vsize_with_ancestors >= anc->second.GetTxSize()); + descendant->second.fee_with_ancestors -= anc->second.GetModifiedFee(); + descendant->second.vsize_with_ancestors -= anc->second.GetTxSize(); + } + } + // Delete these entries. + for (const auto& anc : ancestors) { + m_descendant_set_by_txid.erase(anc->first); + // The above loop should have deducted each ancestor's size and fees from each of their + // respective descendants exactly once. + Assume(anc->second.GetModFeesWithAncestors() == 0); + Assume(anc->second.GetSizeWithAncestors() == 0); + auto vec_it = std::find(m_entries.begin(), m_entries.end(), anc); + Assume(vec_it != m_entries.end()); + m_entries.erase(vec_it); + m_entries_by_txid.erase(anc); + } +} + +void MiniMiner::SanityCheck() const +{ + // m_entries, m_entries_by_txid, and m_descendant_set_by_txid all same size + Assume(m_entries.size() == m_entries_by_txid.size()); + Assume(m_entries.size() == m_descendant_set_by_txid.size()); + // Cached ancestor values should be at least as large as the transaction's own fee and size + Assume(std::all_of(m_entries.begin(), m_entries.end(), [](const auto& entry) { + return entry->second.GetSizeWithAncestors() >= entry->second.GetTxSize() && + entry->second.GetModFeesWithAncestors() >= entry->second.GetModifiedFee();})); + // None of the entries should be to-be-replaced transactions + Assume(std::all_of(m_to_be_replaced.begin(), m_to_be_replaced.end(), + [&](const auto& txid){return m_entries_by_txid.find(txid) == m_entries_by_txid.end();})); +} + +void MiniMiner::BuildMockTemplate(const CFeeRate& target_feerate) +{ + while (!m_entries_by_txid.empty()) { + // Sort again, since transaction removal may change some m_entries' ancestor feerates. + std::sort(m_entries.begin(), m_entries.end(), AncestorFeerateComparator()); + + // Pick highest ancestor feerate entry. + auto best_iter = m_entries.begin(); + Assume(best_iter != m_entries.end()); + const auto ancestor_package_size = (*best_iter)->second.GetSizeWithAncestors(); + const auto ancestor_package_fee = (*best_iter)->second.GetModFeesWithAncestors(); + // Stop here. Everything that didn't "make it into the block" has bumpfee. + if (ancestor_package_fee < target_feerate.GetFee(ancestor_package_size)) { + break; + } + + // Calculate ancestors on the fly. This lookup should be fairly cheap, and ancestor sets + // change at every iteration, so this is more efficient than maintaining a cache. + std::set ancestors; + { + std::set to_process; + to_process.insert(*best_iter); + while (!to_process.empty()) { + auto iter = to_process.begin(); + Assume(iter != to_process.end()); + ancestors.insert(*iter); + for (const auto& input : (*iter)->second.GetTx().vin) { + if (auto parent_it{m_entries_by_txid.find(input.prevout.hash)}; parent_it != m_entries_by_txid.end()) { + if (ancestors.count(parent_it) == 0) { + to_process.insert(parent_it); + } + } + } + to_process.erase(iter); + } + } + DeleteAncestorPackage(ancestors); + SanityCheck(); + } + Assume(m_in_block.empty() || m_total_fees >= target_feerate.GetFee(m_total_vsize)); + // Do not try to continue building the block template with a different feerate. + m_ready_to_calculate = false; +} + +std::map MiniMiner::CalculateBumpFees(const CFeeRate& target_feerate) +{ + if (!m_ready_to_calculate) return {}; + // Build a block template until the target feerate is hit. + BuildMockTemplate(target_feerate); + + // Each transaction that "made it into the block" has a bumpfee of 0, i.e. they are part of an + // ancestor package with at least the target feerate and don't need to be bumped. + for (const auto& txid : m_in_block) { + // Not all of the block transactions were necessarily requested. + auto it = m_requested_outpoints_by_txid.find(txid); + if (it != m_requested_outpoints_by_txid.end()) { + for (const auto& outpoint : it->second) { + m_bump_fees.emplace(outpoint, 0); + } + m_requested_outpoints_by_txid.erase(it); + } + } + + // A transactions and its ancestors will only be picked into a block when + // both the ancestor set feerate and the individual feerate meet the target + // feerate. + // + // We had to convince ourselves that after running the mini miner and + // picking all eligible transactions into our MockBlockTemplate, there + // could still be transactions remaining that have a lower individual + // feerate than their ancestor feerate. So here is an example: + // + // ┌─────────────────┐ + // │ │ + // │ Grandparent │ + // │ 1700 vB │ + // │ 1700 sats │ Target feerate: 10 s/vB + // │ 1 s/vB │ GP Ancestor Set Feerate (ASFR): 1 s/vB + // │ │ P1_ASFR: 9.84 s/vB + // └──────▲───▲──────┘ P2_ASFR: 2.47 s/vB + // │ │ C_ASFR: 10.27 s/vB + // ┌───────────────┐ │ │ ┌──────────────┐ + // │ ├────┘ └────┤ │ ⇒ C_FR < TFR < C_ASFR + // │ Parent 1 │ │ Parent 2 │ + // │ 200 vB │ │ 200 vB │ + // │ 17000 sats │ │ 3000 sats │ + // │ 85 s/vB │ │ 15 s/vB │ + // │ │ │ │ + // └───────────▲───┘ └───▲──────────┘ + // │ │ + // │ ┌───────────┐ │ + // └────┤ ├────┘ + // │ Child │ + // │ 100 vB │ + // │ 900 sats │ + // │ 9 s/vB │ + // │ │ + // └───────────┘ + // + // We therefore calculate both the bump fee that is necessary to elevate + // the individual transaction to the target feerate: + // target_feerate × tx_size - tx_fees + // and the bump fee that is necessary to bump the entire ancestor set to + // the target feerate: + // target_feerate × ancestor_set_size - ancestor_set_fees + // By picking the maximum from the two, we ensure that a transaction meets + // both criteria. + for (const auto& [txid, outpoints] : m_requested_outpoints_by_txid) { + auto it = m_entries_by_txid.find(txid); + Assume(it != m_entries_by_txid.end()); + if (it != m_entries_by_txid.end()) { + Assume(target_feerate.GetFee(it->second.GetSizeWithAncestors()) > std::min(it->second.GetModifiedFee(), it->second.GetModFeesWithAncestors())); + CAmount bump_fee_with_ancestors = target_feerate.GetFee(it->second.GetSizeWithAncestors()) - it->second.GetModFeesWithAncestors(); + CAmount bump_fee_individual = target_feerate.GetFee(it->second.GetTxSize()) - it->second.GetModifiedFee(); + const CAmount bump_fee{std::max(bump_fee_with_ancestors, bump_fee_individual)}; + Assume(bump_fee >= 0); + for (const auto& outpoint : outpoints) { + m_bump_fees.emplace(outpoint, bump_fee); + } + } + } + return m_bump_fees; +} + +std::optional MiniMiner::CalculateTotalBumpFees(const CFeeRate& target_feerate) +{ + if (!m_ready_to_calculate) return std::nullopt; + // Build a block template until the target feerate is hit. + BuildMockTemplate(target_feerate); + + // All remaining ancestors that are not part of m_in_block must be bumped, but no other relatives + std::set ancestors; + std::set to_process; + for (const auto& [txid, outpoints] : m_requested_outpoints_by_txid) { + // Skip any ancestors that already have a miner score higher than the target feerate + // (already "made it" into the block) + if (m_in_block.count(txid)) continue; + auto iter = m_entries_by_txid.find(txid); + if (iter == m_entries_by_txid.end()) continue; + to_process.insert(iter); + ancestors.insert(iter); + } + while (!to_process.empty()) { + auto iter = to_process.begin(); + const CTransaction& tx = (*iter)->second.GetTx(); + for (const auto& input : tx.vin) { + if (auto parent_it{m_entries_by_txid.find(input.prevout.hash)}; parent_it != m_entries_by_txid.end()) { + to_process.insert(parent_it); + ancestors.insert(parent_it); + } + } + to_process.erase(iter); + } + const auto ancestor_package_size = std::accumulate(ancestors.cbegin(), ancestors.cend(), int64_t{0}, + [](int64_t sum, const auto it) {return sum + it->second.GetTxSize();}); + const auto ancestor_package_fee = std::accumulate(ancestors.cbegin(), ancestors.cend(), CAmount{0}, + [](CAmount sum, const auto it) {return sum + it->second.GetModifiedFee();}); + return target_feerate.GetFee(ancestor_package_size) - ancestor_package_fee; +} +} // namespace node diff --git a/src/node/mini_miner.h b/src/node/mini_miner.h new file mode 100644 index 0000000000000..db07e6d1bf0df --- /dev/null +++ b/src/node/mini_miner.h @@ -0,0 +1,121 @@ +// Copyright (c) 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. + +#ifndef BITCOIN_NODE_MINI_MINER_H +#define BITCOIN_NODE_MINI_MINER_H + +#include + +#include +#include +#include + +namespace node { + +// Container for tracking updates to ancestor feerate as we include ancestors in the "block" +class MiniMinerMempoolEntry +{ + const CAmount fee_individual; + const CTransactionRef tx; + const int64_t vsize_individual; + +// This class must be constructed while holding mempool.cs. After construction, the object's +// methods can be called without holding that lock. +public: + CAmount fee_with_ancestors; + int64_t vsize_with_ancestors; + explicit MiniMinerMempoolEntry(CTxMemPool::txiter entry) : + fee_individual{entry->GetModifiedFee()}, + tx{entry->GetSharedTx()}, + vsize_individual(entry->GetTxSize()), + fee_with_ancestors{entry->GetModFeesWithAncestors()}, + vsize_with_ancestors(entry->GetSizeWithAncestors()) + { } + + CAmount GetModifiedFee() const { return fee_individual; } + CAmount GetModFeesWithAncestors() const { return fee_with_ancestors; } + int64_t GetTxSize() const { return vsize_individual; } + int64_t GetSizeWithAncestors() const { return vsize_with_ancestors; } + const CTransaction& GetTx() const LIFETIMEBOUND { return *tx; } +}; + +// Comparator needed for std::set +struct IteratorComparator +{ + template + bool operator()(const I& a, const I& b) const + { + return &(*a) < &(*b); + } +}; + +/** A minimal version of BlockAssembler. Allows us to run the mining algorithm on a subset of + * mempool transactions, ignoring consensus rules, to calculate mining scores. */ +class MiniMiner +{ + // When true, a caller may use CalculateBumpFees(). Becomes false if we failed to retrieve + // mempool entries (i.e. cluster size too large) or bump fees have already been calculated. + bool m_ready_to_calculate{true}; + + // Set once per lifetime, fill in during initialization. + // txids of to-be-replaced transactions + std::set m_to_be_replaced; + + // If multiple argument outpoints correspond to the same transaction, cache them together in + // a single entry indexed by txid. Then we can just work with txids since all outpoints from + // the same tx will have the same bumpfee. Excludes non-mempool transactions. + std::map> m_requested_outpoints_by_txid; + + // What we're trying to calculate. + std::map m_bump_fees; + + // The constructed block template + std::set m_in_block; + + // Information on the current status of the block + CAmount m_total_fees{0}; + int32_t m_total_vsize{0}; + + /** Main data structure holding the entries, can be indexed by txid */ + std::map m_entries_by_txid; + using MockEntryMap = decltype(m_entries_by_txid); + + /** Vector of entries, can be sorted by ancestor feerate. */ + std::vector m_entries; + + /** Map of txid to its descendants. Should be inclusive. */ + std::map> m_descendant_set_by_txid; + + /** Consider this ancestor package "mined" so remove all these entries from our data structures. */ + void DeleteAncestorPackage(const std::set& ancestors); + + /** Perform some checks. */ + void SanityCheck() const; + +public: + /** Returns true if CalculateBumpFees may be called, false if not. */ + bool IsReadyToCalculate() const { return m_ready_to_calculate; } + + /** Build a block template until the target feerate is hit. */ + void BuildMockTemplate(const CFeeRate& target_feerate); + + /** Returns set of txids in the block template if one has been constructed. */ + std::set GetMockTemplateTxids() const { return m_in_block; } + + MiniMiner(const CTxMemPool& mempool, const std::vector& outpoints); + + /** Construct a new block template and, for each outpoint corresponding to a transaction that + * did not make it into the block, calculate the cost of bumping those transactions (and their + * ancestors) to the minimum feerate. Returns a map from outpoint to bump fee, or an empty map + * if they cannot be calculated. */ + std::map CalculateBumpFees(const CFeeRate& target_feerate); + + /** Construct a new block template and, calculate the cost of bumping all transactions that did + * not make it into the block to the target feerate. Returns the total bump fee, or std::nullopt + * if it cannot be calculated. */ + std::optional CalculateTotalBumpFees(const CFeeRate& target_feerate); +}; +} // namespace node + +#endif // BITCOIN_NODE_MINI_MINER_H From 5197660e947435e510ef3ef72be8be8dee3ffa41 Mon Sep 17 00:00:00 2001 From: Martin Leitner-Ankerl Date: Mon, 3 Apr 2023 07:24:31 +0200 Subject: [PATCH 0026/1751] tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings Fixes #26916 by disabling the warning `-Wgnu-zero-variadic-macro-arguments` when clang is used as the compiler. --- src/util/trace.h | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/src/util/trace.h b/src/util/trace.h index 7a63f39c83f94..051921a0d215b 100644 --- a/src/util/trace.h +++ b/src/util/trace.h @@ -9,19 +9,28 @@ #include -#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) +// 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 #else From 3f3f2d59ea2946a7b7cc8cb0222fb602d62645d0 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 11 Aug 2022 09:58:39 +0100 Subject: [PATCH 0027/1751] [unit test] GatherClusters and MiniMiner unit tests Co-authored-by: Murch Co-authored-by: theStack Co-authored-by: furszy --- src/Makefile.test.include | 1 + src/test/miniminer_tests.cpp | 477 +++++++++++++++++++++++++++++++++++ 2 files changed, 478 insertions(+) create mode 100644 src/test/miniminer_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index d6992640ff2cf..3e9d6ad9e3dc3 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -106,6 +106,7 @@ BITCOIN_TESTS =\ test/merkle_tests.cpp \ test/merkleblock_tests.cpp \ test/miner_tests.cpp \ + test/miniminer_tests.cpp \ test/miniscript_tests.cpp \ test/minisketch_tests.cpp \ test/multisig_tests.cpp \ diff --git a/src/test/miniminer_tests.cpp b/src/test/miniminer_tests.cpp new file mode 100644 index 0000000000000..3f4a5fbe747ea --- /dev/null +++ b/src/test/miniminer_tests.cpp @@ -0,0 +1,477 @@ +// 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. +#include +#include +#include +#include + +#include +#include + +#include +#include +#include + +BOOST_FIXTURE_TEST_SUITE(miniminer_tests, TestingSetup) + +static inline CTransactionRef make_tx(const std::vector& inputs, size_t num_outputs) +{ + CMutableTransaction tx = CMutableTransaction(); + tx.vin.resize(inputs.size()); + tx.vout.resize(num_outputs); + for (size_t i = 0; i < inputs.size(); ++i) { + tx.vin[i].prevout = inputs[i]; + } + for (size_t i = 0; i < num_outputs; ++i) { + tx.vout[i].scriptPubKey = CScript() << OP_11 << OP_EQUAL; + // The actual input and output values of these transactions don't really + // matter, since all accounting will use the entries' cached fees. + tx.vout[i].nValue = COIN; + } + return MakeTransactionRef(tx); +} + +static inline bool sanity_check(const std::vector& transactions, + const std::map& bumpfees) +{ + // No negative bumpfees. + for (const auto& [outpoint, fee] : bumpfees) { + if (fee < 0) return false; + if (fee == 0) continue; + auto outpoint_ = outpoint; // structured bindings can't be captured in C++17, so we need to use a variable + const bool found = std::any_of(transactions.cbegin(), transactions.cend(), [&](const auto& tx) { + return outpoint_.hash == tx->GetHash() && outpoint_.n < tx->vout.size(); + }); + if (!found) return false; + } + for (const auto& tx : transactions) { + // If tx has multiple outputs, they must all have the same bumpfee (if they exist). + if (tx->vout.size() > 1) { + std::set distinct_bumpfees; + for (size_t i{0}; i < tx->vout.size(); ++i) { + const auto bumpfee = bumpfees.find(COutPoint{tx->GetHash(), static_cast(i)}); + if (bumpfee != bumpfees.end()) distinct_bumpfees.insert(bumpfee->second); + } + if (distinct_bumpfees.size() > 1) return false; + } + } + return true; +} + +template +Value Find(const std::map& map, const Key& key) +{ + auto it = map.find(key); + BOOST_CHECK_MESSAGE(it != map.end(), strprintf("Cannot find %s", key.ToString())); + return it->second; +} + +BOOST_FIXTURE_TEST_CASE(miniminer_1p1c, TestChain100Setup) +{ + CTxMemPool& pool = *Assert(m_node.mempool); + LOCK2(::cs_main, pool.cs); + TestMemPoolEntryHelper entry; + + const CAmount low_fee{CENT/2000}; + const CAmount normal_fee{CENT/200}; + const CAmount high_fee{CENT/10}; + + // Create a parent tx1 and child tx2 with normal fees: + const auto tx1 = make_tx({COutPoint{m_coinbase_txns[0]->GetHash(), 0}}, /*num_outputs=*/2); + pool.addUnchecked(entry.Fee(normal_fee).FromTx(tx1)); + const auto tx2 = make_tx({COutPoint{tx1->GetHash(), 0}}, /*num_outputs=*/1); + pool.addUnchecked(entry.Fee(normal_fee).FromTx(tx2)); + + // Create a low-feerate parent tx3 and high-feerate child tx4 (cpfp) + const auto tx3 = make_tx({COutPoint{m_coinbase_txns[1]->GetHash(), 0}}, /*num_outputs=*/2); + pool.addUnchecked(entry.Fee(low_fee).FromTx(tx3)); + const auto tx4 = make_tx({COutPoint{tx3->GetHash(), 0}}, /*num_outputs=*/1); + pool.addUnchecked(entry.Fee(high_fee).FromTx(tx4)); + + // Create a parent tx5 and child tx6 where both have low fees + const auto tx5 = make_tx({COutPoint{m_coinbase_txns[2]->GetHash(), 0}}, /*num_outputs=*/2); + pool.addUnchecked(entry.Fee(low_fee).FromTx(tx5)); + const auto tx6 = make_tx({COutPoint{tx5->GetHash(), 0}}, /*num_outputs=*/1); + pool.addUnchecked(entry.Fee(low_fee).FromTx(tx6)); + // Make tx6's modified fee much higher than its base fee. This should cause it to pass + // the fee-related checks despite being low-feerate. + pool.PrioritiseTransaction(tx6->GetHash(), CENT/100); + + // Create a high-feerate parent tx7, low-feerate child tx8 + const auto tx7 = make_tx({COutPoint{m_coinbase_txns[3]->GetHash(), 0}}, /*num_outputs=*/2); + pool.addUnchecked(entry.Fee(high_fee).FromTx(tx7)); + const auto tx8 = make_tx({COutPoint{tx7->GetHash(), 0}}, /*num_outputs=*/1); + pool.addUnchecked(entry.Fee(low_fee).FromTx(tx8)); + + std::vector all_unspent_outpoints({ + COutPoint{tx1->GetHash(), 1}, + COutPoint{tx2->GetHash(), 0}, + COutPoint{tx3->GetHash(), 1}, + COutPoint{tx4->GetHash(), 0}, + COutPoint{tx5->GetHash(), 1}, + COutPoint{tx6->GetHash(), 0}, + COutPoint{tx7->GetHash(), 1}, + COutPoint{tx8->GetHash(), 0} + }); + for (const auto& outpoint : all_unspent_outpoints) BOOST_CHECK(!pool.isSpent(outpoint)); + + std::vector all_spent_outpoints({ + COutPoint{tx1->GetHash(), 0}, + COutPoint{tx3->GetHash(), 0}, + COutPoint{tx5->GetHash(), 0}, + COutPoint{tx7->GetHash(), 0} + }); + for (const auto& outpoint : all_spent_outpoints) BOOST_CHECK(pool.GetConflictTx(outpoint) != nullptr); + + std::vector all_parent_outputs({ + COutPoint{tx1->GetHash(), 0}, + COutPoint{tx1->GetHash(), 1}, + COutPoint{tx3->GetHash(), 0}, + COutPoint{tx3->GetHash(), 1}, + COutPoint{tx5->GetHash(), 0}, + COutPoint{tx5->GetHash(), 1}, + COutPoint{tx7->GetHash(), 0}, + COutPoint{tx7->GetHash(), 1} + }); + + + std::vector all_transactions{tx1, tx2, tx3, tx4, tx5, tx6, tx7, tx8}; + struct TxDimensions { + size_t vsize; CAmount mod_fee; CFeeRate feerate; + }; + std::map tx_dims; + for (const auto& tx : all_transactions) { + const auto it = pool.GetIter(tx->GetHash()).value(); + tx_dims.emplace(tx->GetHash(), TxDimensions{it->GetTxSize(), it->GetModifiedFee(), + CFeeRate(it->GetModifiedFee(), it->GetTxSize())}); + } + + const std::vector various_normal_feerates({CFeeRate(0), CFeeRate(500), CFeeRate(999), + CFeeRate(1000), CFeeRate(2000), CFeeRate(2500), + CFeeRate(3333), CFeeRate(7800), CFeeRate(11199), + CFeeRate(23330), CFeeRate(50000), CFeeRate(5*CENT)}); + + // All nonexistent entries have a bumpfee of zero, regardless of feerate + std::vector nonexistent_outpoints({ COutPoint{GetRandHash(), 0}, COutPoint{GetRandHash(), 3} }); + for (const auto& outpoint : nonexistent_outpoints) BOOST_CHECK(!pool.isSpent(outpoint)); + for (const auto& feerate : various_normal_feerates) { + node::MiniMiner mini_miner(pool, nonexistent_outpoints); + BOOST_CHECK(mini_miner.IsReadyToCalculate()); + auto bump_fees = mini_miner.CalculateBumpFees(feerate); + BOOST_CHECK(!mini_miner.IsReadyToCalculate()); + BOOST_CHECK(sanity_check(all_transactions, bump_fees)); + BOOST_CHECK(bump_fees.size() == nonexistent_outpoints.size()); + for (const auto& outpoint: nonexistent_outpoints) { + auto it = bump_fees.find(outpoint); + BOOST_CHECK(it != bump_fees.end()); + BOOST_CHECK_EQUAL(it->second, 0); + } + } + + // Gather bump fees for all available UTXOs. + for (const auto& target_feerate : various_normal_feerates) { + node::MiniMiner mini_miner(pool, all_unspent_outpoints); + BOOST_CHECK(mini_miner.IsReadyToCalculate()); + auto bump_fees = mini_miner.CalculateBumpFees(target_feerate); + BOOST_CHECK(!mini_miner.IsReadyToCalculate()); + BOOST_CHECK(sanity_check(all_transactions, bump_fees)); + BOOST_CHECK_EQUAL(bump_fees.size(), all_unspent_outpoints.size()); + + // Check tx1 bumpfee: no other bumper. + const TxDimensions& tx1_dimensions = tx_dims.find(tx1->GetHash())->second; + CAmount bumpfee1 = Find(bump_fees, COutPoint{tx1->GetHash(), 1}); + if (target_feerate <= tx1_dimensions.feerate) { + BOOST_CHECK_EQUAL(bumpfee1, 0); + } else { + // Difference is fee to bump tx1 from current to target feerate. + BOOST_CHECK_EQUAL(bumpfee1, target_feerate.GetFee(tx1_dimensions.vsize) - tx1_dimensions.mod_fee); + } + + // Check tx3 bumpfee: assisted by tx4. + const TxDimensions& tx3_dimensions = tx_dims.find(tx3->GetHash())->second; + const TxDimensions& tx4_dimensions = tx_dims.find(tx4->GetHash())->second; + const CFeeRate tx3_feerate = CFeeRate(tx3_dimensions.mod_fee + tx4_dimensions.mod_fee, tx3_dimensions.vsize + tx4_dimensions.vsize); + CAmount bumpfee3 = Find(bump_fees, COutPoint{tx3->GetHash(), 1}); + if (target_feerate <= tx3_feerate) { + // As long as target feerate is below tx4's ancestor feerate, there is no bump fee. + BOOST_CHECK_EQUAL(bumpfee3, 0); + } else { + // Difference is fee to bump tx3 from current to target feerate, without tx4. + BOOST_CHECK_EQUAL(bumpfee3, target_feerate.GetFee(tx3_dimensions.vsize) - tx3_dimensions.mod_fee); + } + + // If tx6’s modified fees are sufficient for tx5 and tx6 to be picked + // into the block, our prospective new transaction would not need to + // bump tx5 when using tx5’s second output. If however even tx6’s + // modified fee (which essentially indicates "effective feerate") is + // not sufficient to bump tx5, using the second output of tx5 would + // require our transaction to bump tx5 from scratch since we evaluate + // transaction packages per ancestor sets and do not consider multiple + // children’s fees. + const TxDimensions& tx5_dimensions = tx_dims.find(tx5->GetHash())->second; + const TxDimensions& tx6_dimensions = tx_dims.find(tx6->GetHash())->second; + const CFeeRate tx5_feerate = CFeeRate(tx5_dimensions.mod_fee + tx6_dimensions.mod_fee, tx5_dimensions.vsize + tx6_dimensions.vsize); + CAmount bumpfee5 = Find(bump_fees, COutPoint{tx5->GetHash(), 1}); + if (target_feerate <= tx5_feerate) { + // As long as target feerate is below tx6's ancestor feerate, there is no bump fee. + BOOST_CHECK_EQUAL(bumpfee5, 0); + } else { + // Difference is fee to bump tx5 from current to target feerate, without tx6. + BOOST_CHECK_EQUAL(bumpfee5, target_feerate.GetFee(tx5_dimensions.vsize) - tx5_dimensions.mod_fee); + } + } + // Spent outpoints should usually not be requested as they would not be + // considered available. However, when they are explicitly requested, we + // can calculate their bumpfee to facilitate RBF-replacements + for (const auto& target_feerate : various_normal_feerates) { + node::MiniMiner mini_miner_all_spent(pool, all_spent_outpoints); + BOOST_CHECK(mini_miner_all_spent.IsReadyToCalculate()); + auto bump_fees_all_spent = mini_miner_all_spent.CalculateBumpFees(target_feerate); + BOOST_CHECK(!mini_miner_all_spent.IsReadyToCalculate()); + BOOST_CHECK_EQUAL(bump_fees_all_spent.size(), all_spent_outpoints.size()); + node::MiniMiner mini_miner_all_parents(pool, all_parent_outputs); + BOOST_CHECK(mini_miner_all_parents.IsReadyToCalculate()); + auto bump_fees_all_parents = mini_miner_all_parents.CalculateBumpFees(target_feerate); + BOOST_CHECK(!mini_miner_all_parents.IsReadyToCalculate()); + BOOST_CHECK_EQUAL(bump_fees_all_parents.size(), all_parent_outputs.size()); + for (auto& bump_fees : {bump_fees_all_parents, bump_fees_all_spent}) { + // For all_parents case, both outputs from the parent should have the same bump fee, + // even though only one of them is in a to-be-replaced transaction. + BOOST_CHECK(sanity_check(all_transactions, bump_fees)); + + // Check tx1 bumpfee: no other bumper. + const TxDimensions& tx1_dimensions = tx_dims.find(tx1->GetHash())->second; + CAmount it1_spent = Find(bump_fees, COutPoint{tx1->GetHash(), 0}); + if (target_feerate <= tx1_dimensions.feerate) { + BOOST_CHECK_EQUAL(it1_spent, 0); + } else { + // Difference is fee to bump tx1 from current to target feerate. + BOOST_CHECK_EQUAL(it1_spent, target_feerate.GetFee(tx1_dimensions.vsize) - tx1_dimensions.mod_fee); + } + + // Check tx3 bumpfee: no other bumper, because tx4 is to-be-replaced. + const TxDimensions& tx3_dimensions = tx_dims.find(tx3->GetHash())->second; + const CFeeRate tx3_feerate_unbumped = tx3_dimensions.feerate; + auto it3_spent = Find(bump_fees, COutPoint{tx3->GetHash(), 0}); + if (target_feerate <= tx3_feerate_unbumped) { + BOOST_CHECK_EQUAL(it3_spent, 0); + } else { + // Difference is fee to bump tx3 from current to target feerate, without tx4. + BOOST_CHECK_EQUAL(it3_spent, target_feerate.GetFee(tx3_dimensions.vsize) - tx3_dimensions.mod_fee); + } + + // Check tx5 bumpfee: no other bumper, because tx6 is to-be-replaced. + const TxDimensions& tx5_dimensions = tx_dims.find(tx5->GetHash())->second; + const CFeeRate tx5_feerate_unbumped = tx5_dimensions.feerate; + auto it5_spent = Find(bump_fees, COutPoint{tx5->GetHash(), 0}); + if (target_feerate <= tx5_feerate_unbumped) { + BOOST_CHECK_EQUAL(it5_spent, 0); + } else { + // Difference is fee to bump tx5 from current to target feerate, without tx6. + BOOST_CHECK_EQUAL(it5_spent, target_feerate.GetFee(tx5_dimensions.vsize) - tx5_dimensions.mod_fee); + } + } + } +} + +BOOST_FIXTURE_TEST_CASE(miniminer_overlap, TestChain100Setup) +{ + CTxMemPool& pool = *Assert(m_node.mempool); + LOCK2(::cs_main, pool.cs); + TestMemPoolEntryHelper entry; + + const CAmount low_fee{CENT/2000}; + const CAmount med_fee{CENT/200}; + const CAmount high_fee{CENT/10}; + + // Create 3 parents of different feerates, and 1 child spending from all 3. + const auto tx1 = make_tx({COutPoint{m_coinbase_txns[0]->GetHash(), 0}}, /*num_outputs=*/2); + pool.addUnchecked(entry.Fee(low_fee).FromTx(tx1)); + const auto tx2 = make_tx({COutPoint{m_coinbase_txns[1]->GetHash(), 0}}, /*num_outputs=*/2); + pool.addUnchecked(entry.Fee(med_fee).FromTx(tx2)); + const auto tx3 = make_tx({COutPoint{m_coinbase_txns[2]->GetHash(), 0}}, /*num_outputs=*/2); + pool.addUnchecked(entry.Fee(high_fee).FromTx(tx3)); + const auto tx4 = make_tx({COutPoint{tx1->GetHash(), 0}, COutPoint{tx2->GetHash(), 0}, COutPoint{tx3->GetHash(), 0}}, /*num_outputs=*/3); + pool.addUnchecked(entry.Fee(high_fee).FromTx(tx4)); + + // Create 1 grandparent and 1 parent, then 2 children. + const auto tx5 = make_tx({COutPoint{m_coinbase_txns[3]->GetHash(), 0}}, /*num_outputs=*/2); + pool.addUnchecked(entry.Fee(high_fee).FromTx(tx5)); + const auto tx6 = make_tx({COutPoint{tx5->GetHash(), 0}}, /*num_outputs=*/3); + pool.addUnchecked(entry.Fee(low_fee).FromTx(tx6)); + const auto tx7 = make_tx({COutPoint{tx6->GetHash(), 0}}, /*num_outputs=*/2); + pool.addUnchecked(entry.Fee(med_fee).FromTx(tx7)); + const auto tx8 = make_tx({COutPoint{tx6->GetHash(), 1}}, /*num_outputs=*/2); + pool.addUnchecked(entry.Fee(high_fee).FromTx(tx8)); + + std::vector all_transactions{tx1, tx2, tx3, tx4, tx5, tx6, tx7, tx8}; + std::vector tx_vsizes; + tx_vsizes.reserve(all_transactions.size()); + for (const auto& tx : all_transactions) tx_vsizes.push_back(GetVirtualTransactionSize(*tx)); + + std::vector all_unspent_outpoints({ + COutPoint{tx1->GetHash(), 1}, + COutPoint{tx2->GetHash(), 1}, + COutPoint{tx3->GetHash(), 1}, + COutPoint{tx4->GetHash(), 0}, + COutPoint{tx4->GetHash(), 1}, + COutPoint{tx4->GetHash(), 2}, + COutPoint{tx5->GetHash(), 1}, + COutPoint{tx6->GetHash(), 2}, + COutPoint{tx7->GetHash(), 0}, + COutPoint{tx8->GetHash(), 0} + }); + for (const auto& outpoint : all_unspent_outpoints) BOOST_CHECK(!pool.isSpent(outpoint)); + + const auto tx3_feerate = CFeeRate(high_fee, tx_vsizes[2]); + const auto tx4_feerate = CFeeRate(high_fee, tx_vsizes[3]); + // tx4's feerate is lower than tx3's. same fee, different weight. + BOOST_CHECK(tx3_feerate > tx4_feerate); + const auto tx4_anc_feerate = CFeeRate(low_fee + med_fee + high_fee, tx_vsizes[0] + tx_vsizes[1] + tx_vsizes[3]); + const auto tx5_feerate = CFeeRate(high_fee, tx_vsizes[4]); + const auto tx7_anc_feerate = CFeeRate(low_fee + med_fee, tx_vsizes[5] + tx_vsizes[6]); + const auto tx8_anc_feerate = CFeeRate(low_fee + high_fee, tx_vsizes[5] + tx_vsizes[7]); + BOOST_CHECK(tx5_feerate > tx7_anc_feerate); + BOOST_CHECK(tx5_feerate > tx8_anc_feerate); + + // Extremely high feerate: everybody's bumpfee is from their full ancestor set. + { + node::MiniMiner mini_miner(pool, all_unspent_outpoints); + const CFeeRate very_high_feerate(COIN); + BOOST_CHECK(tx4_anc_feerate < very_high_feerate); + BOOST_CHECK(mini_miner.IsReadyToCalculate()); + auto bump_fees = mini_miner.CalculateBumpFees(very_high_feerate); + BOOST_CHECK_EQUAL(bump_fees.size(), all_unspent_outpoints.size()); + BOOST_CHECK(!mini_miner.IsReadyToCalculate()); + BOOST_CHECK(sanity_check(all_transactions, bump_fees)); + const auto tx1_bumpfee = bump_fees.find(COutPoint{tx1->GetHash(), 1}); + BOOST_CHECK(tx1_bumpfee != bump_fees.end()); + BOOST_CHECK_EQUAL(tx1_bumpfee->second, very_high_feerate.GetFee(tx_vsizes[0]) - low_fee); + const auto tx4_bumpfee = bump_fees.find(COutPoint{tx4->GetHash(), 0}); + BOOST_CHECK(tx4_bumpfee != bump_fees.end()); + BOOST_CHECK_EQUAL(tx4_bumpfee->second, + very_high_feerate.GetFee(tx_vsizes[0] + tx_vsizes[1] + tx_vsizes[2] + tx_vsizes[3]) - (low_fee + med_fee + high_fee + high_fee)); + const auto tx7_bumpfee = bump_fees.find(COutPoint{tx7->GetHash(), 0}); + BOOST_CHECK(tx7_bumpfee != bump_fees.end()); + BOOST_CHECK_EQUAL(tx7_bumpfee->second, + very_high_feerate.GetFee(tx_vsizes[4] + tx_vsizes[5] + tx_vsizes[6]) - (high_fee + low_fee + med_fee)); + const auto tx8_bumpfee = bump_fees.find(COutPoint{tx8->GetHash(), 0}); + BOOST_CHECK(tx8_bumpfee != bump_fees.end()); + BOOST_CHECK_EQUAL(tx8_bumpfee->second, + very_high_feerate.GetFee(tx_vsizes[4] + tx_vsizes[5] + tx_vsizes[7]) - (high_fee + low_fee + high_fee)); + // Total fees: if spending multiple outputs from tx4 don't double-count fees. + node::MiniMiner mini_miner_total_tx4(pool, {COutPoint{tx4->GetHash(), 0}, COutPoint{tx4->GetHash(), 1}}); + BOOST_CHECK(mini_miner_total_tx4.IsReadyToCalculate()); + const auto tx4_bump_fee = mini_miner_total_tx4.CalculateTotalBumpFees(very_high_feerate); + BOOST_CHECK(!mini_miner_total_tx4.IsReadyToCalculate()); + BOOST_CHECK(tx4_bump_fee.has_value()); + BOOST_CHECK_EQUAL(tx4_bump_fee.value(), + very_high_feerate.GetFee(tx_vsizes[0] + tx_vsizes[1] + tx_vsizes[2] + tx_vsizes[3]) - (low_fee + med_fee + high_fee + high_fee)); + // Total fees: if spending both tx7 and tx8, don't double-count fees. + node::MiniMiner mini_miner_tx7_tx8(pool, {COutPoint{tx7->GetHash(), 0}, COutPoint{tx8->GetHash(), 0}}); + BOOST_CHECK(mini_miner_tx7_tx8.IsReadyToCalculate()); + const auto tx7_tx8_bumpfee = mini_miner_tx7_tx8.CalculateTotalBumpFees(very_high_feerate); + BOOST_CHECK(!mini_miner_tx7_tx8.IsReadyToCalculate()); + BOOST_CHECK(tx7_tx8_bumpfee.has_value()); + BOOST_CHECK_EQUAL(tx7_tx8_bumpfee.value(), + very_high_feerate.GetFee(tx_vsizes[4] + tx_vsizes[5] + tx_vsizes[6] + tx_vsizes[7]) - (high_fee + low_fee + med_fee + high_fee)); + } + // Feerate just below tx5: tx7 and tx8 have different bump fees. + { + const auto just_below_tx5 = CFeeRate(tx5_feerate.GetFeePerK() - 5); + node::MiniMiner mini_miner(pool, all_unspent_outpoints); + BOOST_CHECK(mini_miner.IsReadyToCalculate()); + auto bump_fees = mini_miner.CalculateBumpFees(just_below_tx5); + BOOST_CHECK(!mini_miner.IsReadyToCalculate()); + BOOST_CHECK_EQUAL(bump_fees.size(), all_unspent_outpoints.size()); + BOOST_CHECK(sanity_check(all_transactions, bump_fees)); + const auto tx7_bumpfee = bump_fees.find(COutPoint{tx7->GetHash(), 0}); + BOOST_CHECK(tx7_bumpfee != bump_fees.end()); + BOOST_CHECK_EQUAL(tx7_bumpfee->second, just_below_tx5.GetFee(tx_vsizes[5] + tx_vsizes[6]) - (low_fee + med_fee)); + const auto tx8_bumpfee = bump_fees.find(COutPoint{tx8->GetHash(), 0}); + BOOST_CHECK(tx8_bumpfee != bump_fees.end()); + BOOST_CHECK_EQUAL(tx8_bumpfee->second, just_below_tx5.GetFee(tx_vsizes[5] + tx_vsizes[7]) - (low_fee + high_fee)); + // Total fees: if spending both tx7 and tx8, don't double-count fees. + node::MiniMiner mini_miner_tx7_tx8(pool, {COutPoint{tx7->GetHash(), 0}, COutPoint{tx8->GetHash(), 0}}); + BOOST_CHECK(mini_miner_tx7_tx8.IsReadyToCalculate()); + const auto tx7_tx8_bumpfee = mini_miner_tx7_tx8.CalculateTotalBumpFees(just_below_tx5); + BOOST_CHECK(!mini_miner_tx7_tx8.IsReadyToCalculate()); + BOOST_CHECK(tx7_tx8_bumpfee.has_value()); + BOOST_CHECK_EQUAL(tx7_tx8_bumpfee.value(), just_below_tx5.GetFee(tx_vsizes[5] + tx_vsizes[6]) - (low_fee + med_fee)); + } + // Feerate between tx7 and tx8's ancestor feerates: don't need to bump tx6 because tx8 already does. + { + const auto just_above_tx7 = CFeeRate(med_fee + 10, tx_vsizes[6]); + BOOST_CHECK(just_above_tx7 <= CFeeRate(low_fee + high_fee, tx_vsizes[5] + tx_vsizes[7])); + node::MiniMiner mini_miner(pool, all_unspent_outpoints); + BOOST_CHECK(mini_miner.IsReadyToCalculate()); + auto bump_fees = mini_miner.CalculateBumpFees(just_above_tx7); + BOOST_CHECK(!mini_miner.IsReadyToCalculate()); + BOOST_CHECK_EQUAL(bump_fees.size(), all_unspent_outpoints.size()); + BOOST_CHECK(sanity_check(all_transactions, bump_fees)); + const auto tx7_bumpfee = bump_fees.find(COutPoint{tx7->GetHash(), 0}); + BOOST_CHECK(tx7_bumpfee != bump_fees.end()); + BOOST_CHECK_EQUAL(tx7_bumpfee->second, just_above_tx7.GetFee(tx_vsizes[6]) - (med_fee)); + const auto tx8_bumpfee = bump_fees.find(COutPoint{tx8->GetHash(), 0}); + BOOST_CHECK(tx8_bumpfee != bump_fees.end()); + BOOST_CHECK_EQUAL(tx8_bumpfee->second, 0); + } +} +BOOST_FIXTURE_TEST_CASE(calculate_cluster, TestChain100Setup) +{ + CTxMemPool& pool = *Assert(m_node.mempool); + LOCK2(cs_main, pool.cs); + + // Add chain of size 500 + TestMemPoolEntryHelper entry; + std::vector chain_txids; + auto& lasttx = m_coinbase_txns[0]; + for (auto i{0}; i < 500; ++i) { + const auto tx = make_tx({COutPoint{lasttx->GetHash(), 0}}, /*num_outputs=*/1); + pool.addUnchecked(entry.Fee(CENT).FromTx(tx)); + chain_txids.push_back(tx->GetHash()); + lasttx = tx; + } + const auto cluster_500tx = pool.GatherClusters({lasttx->GetHash()}); + CTxMemPool::setEntries cluster_500tx_set{cluster_500tx.begin(), cluster_500tx.end()}; + BOOST_CHECK_EQUAL(cluster_500tx.size(), cluster_500tx_set.size()); + const auto vec_iters_500 = pool.GetIterVec(chain_txids); + for (const auto& iter : vec_iters_500) BOOST_CHECK(cluster_500tx_set.count(iter)); + + // GatherClusters stops at 500 transactions. + const auto tx_501 = make_tx({COutPoint{lasttx->GetHash(), 0}}, /*num_outputs=*/1); + pool.addUnchecked(entry.Fee(CENT).FromTx(tx_501)); + const auto cluster_501 = pool.GatherClusters({tx_501->GetHash()}); + BOOST_CHECK_EQUAL(cluster_501.size(), 0); + + // Zig Zag cluster: + // txp0 txp1 txp2 ... txp48 txp49 + // \ / \ / \ \ / + // txc0 txc1 txc2 ... txc48 + // Note that each transaction's ancestor size is 1 or 3, and each descendant size is 1, 2 or 3. + // However, all of these transactions are in the same cluster. + std::vector zigzag_txids; + for (auto p{0}; p < 50; ++p) { + const auto txp = make_tx({COutPoint{GetRandHash(), 0}}, /*num_outputs=*/2); + pool.addUnchecked(entry.Fee(CENT).FromTx(txp)); + zigzag_txids.push_back(txp->GetHash()); + } + for (auto c{0}; c < 49; ++c) { + const auto txc = make_tx({COutPoint{zigzag_txids[c], 1}, COutPoint{zigzag_txids[c+1], 0}}, /*num_outputs=*/1); + pool.addUnchecked(entry.Fee(CENT).FromTx(txc)); + zigzag_txids.push_back(txc->GetHash()); + } + const auto vec_iters_zigzag = pool.GetIterVec(zigzag_txids); + // It doesn't matter which tx we calculate cluster for, everybody is in it. + const std::vector indeces{0, 22, 72, zigzag_txids.size() - 1}; + for (const auto index : indeces) { + const auto cluster = pool.GatherClusters({zigzag_txids[index]}); + BOOST_CHECK_EQUAL(cluster.size(), zigzag_txids.size()); + CTxMemPool::setEntries clusterset{cluster.begin(), cluster.end()}; + BOOST_CHECK_EQUAL(cluster.size(), clusterset.size()); + for (const auto& iter : vec_iters_zigzag) BOOST_CHECK(clusterset.count(iter)); + } +} + +BOOST_AUTO_TEST_SUITE_END() From 6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 16 Feb 2023 15:31:44 +0000 Subject: [PATCH 0028/1751] [fuzz] Add MiniMiner target + diff fuzz against BlockAssembler Co-authored-by: dergoegge Co-authored-by: mzumsande Co-authored-by: Murch --- src/Makefile.test.include | 1 + src/test/fuzz/mini_miner.cpp | 192 +++++++++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+) create mode 100644 src/test/fuzz/mini_miner.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 3e9d6ad9e3dc3..26fd6287708b7 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -283,6 +283,7 @@ test_fuzz_fuzz_SOURCES = \ test/fuzz/message.cpp \ test/fuzz/miniscript.cpp \ test/fuzz/minisketch.cpp \ + test/fuzz/mini_miner.cpp \ test/fuzz/muhash.cpp \ test/fuzz/multiplication_overflow.cpp \ test/fuzz/net.cpp \ diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp new file mode 100644 index 0000000000000..f49d940393199 --- /dev/null +++ b/src/test/fuzz/mini_miner.cpp @@ -0,0 +1,192 @@ +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include +#include + +namespace { + +const TestingSetup* g_setup; +std::deque g_available_coins; +void initialize_miner() +{ + static const auto testing_setup = MakeNoLogFileContext(); + g_setup = testing_setup.get(); + for (uint32_t i = 0; i < uint32_t{100}; ++i) { + g_available_coins.push_back(COutPoint{uint256::ZERO, i}); + } +} + +// Test that the MiniMiner can run with various outpoints and feerates. +FUZZ_TARGET_INIT(mini_miner, initialize_miner) +{ + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + CTxMemPool pool{CTxMemPool::Options{}}; + std::vector outpoints; + std::deque available_coins = g_available_coins; + LOCK2(::cs_main, pool.cs); + // Cluster size cannot exceed 500 + LIMITED_WHILE(!available_coins.empty(), 500) + { + CMutableTransaction mtx = CMutableTransaction(); + const size_t num_inputs = fuzzed_data_provider.ConsumeIntegralInRange(1, available_coins.size()); + const size_t num_outputs = fuzzed_data_provider.ConsumeIntegralInRange(1, 50); + for (size_t n{0}; n < num_inputs; ++n) { + auto prevout = available_coins.front(); + mtx.vin.push_back(CTxIn(prevout, CScript())); + available_coins.pop_front(); + } + for (uint32_t n{0}; n < num_outputs; ++n) { + mtx.vout.push_back(CTxOut(100, P2WSH_OP_TRUE)); + } + CTransactionRef tx = MakeTransactionRef(mtx); + TestMemPoolEntryHelper entry; + const CAmount fee{ConsumeMoney(fuzzed_data_provider, /*max=*/MAX_MONEY/100000)}; + assert(MoneyRange(fee)); + pool.addUnchecked(entry.Fee(fee).FromTx(tx)); + + // All outputs are available to spend + for (uint32_t n{0}; n < num_outputs; ++n) { + if (fuzzed_data_provider.ConsumeBool()) { + available_coins.push_back(COutPoint{tx->GetHash(), n}); + } + } + + if (fuzzed_data_provider.ConsumeBool() && !tx->vout.empty()) { + // Add outpoint from this tx (may or not be spent by a later tx) + outpoints.push_back(COutPoint{tx->GetHash(), + (uint32_t)fuzzed_data_provider.ConsumeIntegralInRange(0, tx->vout.size())}); + } else { + // Add some random outpoint (will be interpreted as confirmed or not yet submitted + // to mempool). + auto outpoint = ConsumeDeserializable(fuzzed_data_provider); + if (outpoint.has_value() && std::find(outpoints.begin(), outpoints.end(), *outpoint) == outpoints.end()) { + outpoints.push_back(*outpoint); + } + } + + } + + const CFeeRate target_feerate{CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/MAX_MONEY/1000)}}; + std::optional total_bumpfee; + CAmount sum_fees = 0; + { + node::MiniMiner mini_miner{pool, outpoints}; + assert(mini_miner.IsReadyToCalculate()); + const auto bump_fees = mini_miner.CalculateBumpFees(target_feerate); + for (const auto& outpoint : outpoints) { + auto it = bump_fees.find(outpoint); + assert(it != bump_fees.end()); + assert(it->second >= 0); + sum_fees += it->second; + } + assert(!mini_miner.IsReadyToCalculate()); + } + { + node::MiniMiner mini_miner{pool, outpoints}; + assert(mini_miner.IsReadyToCalculate()); + total_bumpfee = mini_miner.CalculateTotalBumpFees(target_feerate); + assert(total_bumpfee.has_value()); + assert(!mini_miner.IsReadyToCalculate()); + } + // Overlapping ancestry across multiple outpoints can only reduce the total bump fee. + assert (sum_fees >= *total_bumpfee); +} + +// Test that MiniMiner and BlockAssembler build the same block given the same transactions and constraints. +FUZZ_TARGET_INIT(mini_miner_selection, initialize_miner) +{ + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + CTxMemPool pool{CTxMemPool::Options{}}; + // Make a copy to preserve determinism. + std::deque available_coins = g_available_coins; + std::vector transactions; + + LOCK2(::cs_main, pool.cs); + LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100) + { + CMutableTransaction mtx = CMutableTransaction(); + const size_t num_inputs = 2; + const size_t num_outputs = fuzzed_data_provider.ConsumeIntegralInRange(2, 5); + for (size_t n{0}; n < num_inputs; ++n) { + auto prevout = available_coins.front(); + mtx.vin.push_back(CTxIn(prevout, CScript())); + available_coins.pop_front(); + } + for (uint32_t n{0}; n < num_outputs; ++n) { + mtx.vout.push_back(CTxOut(100, P2WSH_OP_TRUE)); + } + CTransactionRef tx = MakeTransactionRef(mtx); + + // First 2 outputs are available to spend. The rest are added to outpoints to calculate bumpfees. + // There is no overlap between spendable coins and outpoints passed to MiniMiner because the + // MiniMiner interprets spent coins as to-be-replaced and excludes them. + for (uint32_t n{0}; n < num_outputs - 1; ++n) { + if (fuzzed_data_provider.ConsumeBool()) { + available_coins.push_front(COutPoint{tx->GetHash(), n}); + } else { + available_coins.push_back(COutPoint{tx->GetHash(), n}); + } + } + + // Stop if pool reaches DEFAULT_BLOCK_MAX_WEIGHT because BlockAssembler will stop when the + // block template reaches that, but the MiniMiner will keep going. + if (pool.GetTotalTxSize() + GetVirtualTransactionSize(*tx) >= DEFAULT_BLOCK_MAX_WEIGHT) break; + TestMemPoolEntryHelper entry; + const CAmount fee{ConsumeMoney(fuzzed_data_provider, /*max=*/MAX_MONEY/100000)}; + assert(MoneyRange(fee)); + pool.addUnchecked(entry.Fee(fee).FromTx(tx)); + transactions.push_back(tx); + } + std::vector outpoints; + for (const auto& coin : g_available_coins) { + if (!pool.GetConflictTx(coin)) outpoints.push_back(coin); + } + for (const auto& tx : transactions) { + assert(pool.exists(GenTxid::Txid(tx->GetHash()))); + for (uint32_t n{0}; n < tx->vout.size(); ++n) { + COutPoint coin{tx->GetHash(), n}; + if (!pool.GetConflictTx(coin)) outpoints.push_back(coin); + } + } + const CFeeRate target_feerate{ConsumeMoney(fuzzed_data_provider, /*max=*/MAX_MONEY/100000)}; + + node::BlockAssembler::Options miner_options; + miner_options.blockMinFeeRate = target_feerate; + miner_options.nBlockMaxWeight = DEFAULT_BLOCK_MAX_WEIGHT; + miner_options.test_block_validity = false; + + node::BlockAssembler miner{g_setup->m_node.chainman->ActiveChainstate(), &pool, miner_options}; + node::MiniMiner mini_miner{pool, outpoints}; + assert(mini_miner.IsReadyToCalculate()); + + CScript spk_placeholder = CScript() << OP_0; + // Use BlockAssembler as oracle. BlockAssembler and MiniMiner should select the same + // transactions, stopping once packages do not meet target_feerate. + const auto blocktemplate{miner.CreateNewBlock(spk_placeholder)}; + mini_miner.BuildMockTemplate(target_feerate); + assert(!mini_miner.IsReadyToCalculate()); + auto mock_template_txids = mini_miner.GetMockTemplateTxids(); + // MiniMiner doesn't add a coinbase tx. + assert(mock_template_txids.count(blocktemplate->block.vtx[0]->GetHash()) == 0); + mock_template_txids.emplace(blocktemplate->block.vtx[0]->GetHash()); + assert(mock_template_txids.size() <= blocktemplate->block.vtx.size()); + assert(mock_template_txids.size() >= blocktemplate->block.vtx.size()); + assert(mock_template_txids.size() == blocktemplate->block.vtx.size()); + for (const auto& tx : blocktemplate->block.vtx) { + assert(mock_template_txids.count(tx->GetHash())); + } +} +} // namespace From 11bb31c1c43b5da36ca8509b5747abfb3278ffcd Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 14 Apr 2023 11:01:43 -0700 Subject: [PATCH 0029/1751] p2p: "skip netgroup diversity of new connections for tor/i2p/cjdns" follow-up In PR 27374, the semantics of the `setConnected` data structure in CConnman::ThreadOpenConnections changed from the set of outbound peer netgroups to those of outbound IPv4/6 peers only. This commit updates a code comment in this regard about feeler connections and updates the naming of `setConnected` to `outbound_ipv46_peer_netgroups` to reflect its new role. --- src/net.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 903fedb2fb659..33afec1c8f38d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1707,7 +1707,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) int nOutboundFullRelay = 0; int nOutboundBlockRelay = 0; int outbound_privacy_network_peers = 0; - std::set> setConnected; // netgroups of our ipv4/ipv6 outbound peers + std::set> outbound_ipv46_peer_netgroups; { LOCK(m_nodes_mutex); @@ -1729,7 +1729,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) case ConnectionType::MANUAL: case ConnectionType::OUTBOUND_FULL_RELAY: case ConnectionType::BLOCK_RELAY: - CAddress address{pnode->addr}; + const CAddress address{pnode->addr}; if (address.IsTor() || address.IsI2P() || address.IsCJDNS()) { // Since our addrman-groups for these networks are // random, without relation to the route we @@ -1740,7 +1740,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) // these networks. ++outbound_privacy_network_peers; } else { - setConnected.insert(m_netgroupman.GetGroup(address)); + outbound_ipv46_peer_netgroups.insert(m_netgroupman.GetGroup(address)); } } // no default case, so the compiler can warn about missing cases } @@ -1815,7 +1815,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) m_anchors.pop_back(); if (!addr.IsValid() || IsLocal(addr) || !IsReachable(addr) || !HasAllDesirableServiceFlags(addr.nServices) || - setConnected.count(m_netgroupman.GetGroup(addr))) continue; + outbound_ipv46_peer_netgroups.count(m_netgroupman.GetGroup(addr))) continue; addrConnect = addr; LogPrint(BCLog::NET, "Trying to make an anchor connection to %s\n", addrConnect.ToStringAddrPort()); break; @@ -1855,8 +1855,8 @@ void CConnman::ThreadOpenConnections(const std::vector connect) std::tie(addr, addr_last_try) = addrman.Select(); } - // Require outbound connections, other than feelers, to be to distinct network groups - if (!fFeeler && setConnected.count(m_netgroupman.GetGroup(addr))) { + // Require outbound IPv4/IPv6 connections, other than feelers, to be to distinct network groups + if (!fFeeler && outbound_ipv46_peer_netgroups.count(m_netgroupman.GetGroup(addr))) { break; } @@ -1902,8 +1902,9 @@ void CConnman::ThreadOpenConnections(const std::vector connect) // Record addrman failure attempts when node has at least 2 persistent outbound connections to peers with // different netgroups in ipv4/ipv6 networks + all peers in Tor/I2P/CJDNS networks. // Don't record addrman failure attempts when node is offline. This can be identified since all local - // network connections(if any) belong in the same netgroup and size of setConnected would only be 1. - OpenNetworkConnection(addrConnect, (int)setConnected.size() + outbound_privacy_network_peers >= std::min(nMaxConnections - 1, 2), &grant, nullptr, conn_type); + // network connections (if any) belong in the same netgroup, and the size of `outbound_ipv46_peer_netgroups` would only be 1. + const bool count_failures{((int)outbound_ipv46_peer_netgroups.size() + outbound_privacy_network_peers) >= std::min(nMaxConnections - 1, 2)}; + OpenNetworkConnection(addrConnect, count_failures, &grant, /*strDest=*/nullptr, conn_type); } } } From 398c3719b02197ad92fded20f6ff83b364747297 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 23 Mar 2023 10:29:35 -0400 Subject: [PATCH 0030/1751] lint: Fix lint-format-strings false positives when format specifiers have argument positions Do not error on valid format specifications like strprintf("arg2=%2$s arg1=%1$s arg2=%2$s", arg1, arg2); Needed to avoid lint error in upcoming commit: https://cirrus-ci.com/task/4755032734695424?logs=lint#L221 Additionally tested with python -m doctest test/lint/run-lint-format-strings.py --- test/lint/run-lint-format-strings.py | 30 +++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/test/lint/run-lint-format-strings.py b/test/lint/run-lint-format-strings.py index 91915f05f9f20..d1896dba840c2 100755 --- a/test/lint/run-lint-format-strings.py +++ b/test/lint/run-lint-format-strings.py @@ -241,20 +241,32 @@ def count_format_specifiers(format_string): 3 >>> count_format_specifiers("foo %d bar %i foo %% foo %*d foo") 4 + >>> count_format_specifiers("foo %5$d") + 5 + >>> count_format_specifiers("foo %5$*7$d") + 7 """ assert type(format_string) is str format_string = format_string.replace('%%', 'X') - n = 0 - in_specifier = False - for i, char in enumerate(format_string): - if char == "%": - in_specifier = True + n = max_pos = 0 + for m in re.finditer("%(.*?)[aAcdeEfFgGinopsuxX]", format_string, re.DOTALL): + # Increase the max position if the argument has a position number like + # "5$", otherwise increment the argument count. + pos_num, = re.match(r"(?:(^\d+)\$)?", m.group(1)).groups() + if pos_num is not None: + max_pos = max(max_pos, int(pos_num)) + else: n += 1 - elif char in "aAcdeEfFgGinopsuxX": - in_specifier = False - elif in_specifier and char == "*": + + # Increase the max position if there is a "*" width argument with a + # position like "*7$", and increment the argument count if there is a + # "*" width argument with no position. + star, star_pos_num = re.match(r"(?:.*?(\*(?:(\d+)\$)?)|)", m.group(1)).groups() + if star_pos_num is not None: + max_pos = max(max_pos, int(star_pos_num)) + elif star is not None: n += 1 - return n + return max(n, max_pos) def main(): From 3746f00be1b732a04976fc70cbb0661f97bbbd99 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 21 Mar 2023 13:14:53 -0400 Subject: [PATCH 0031/1751] init: Error if ignored bitcoin.conf file is found Show an error on startup if a bitcoin datadir that is being used contains a `bitcoin.conf` file that is ignored. There are two cases where this could happen: - One case reported in https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043 happens when a bitcoin.conf file in the default datadir (e.g. $HOME/.bitcoin/bitcoin.conf) has a "datadir=/path" line that sets different datadir containing a second bitcoin.conf file. Currently the second bitcoin.conf file is ignored with no warning. - Another way this could happen is if a -conf= command line argument points to a configuration file with a "datadir=/path" line and that specified path contains a bitcoin.conf file, which is currently ignored. This change only adds an error message and doesn't change anything about way settings are applied. It also doesn't trigger errors if there are redundant -datadir or -conf settings pointing at the same configuration file, only if they are pointing at different files and one file is being ignored. --- doc/release-notes-27302.md | 4 ++ src/common/init.cpp | 40 ++++++++++++++ src/init.cpp | 1 + test/functional/feature_config_args.py | 58 ++++++++++++++++++++- test/functional/test_framework/test_node.py | 4 +- test/functional/test_framework/util.py | 20 ++++++- 6 files changed, 124 insertions(+), 3 deletions(-) create mode 100644 doc/release-notes-27302.md diff --git a/doc/release-notes-27302.md b/doc/release-notes-27302.md new file mode 100644 index 0000000000000..e67a6c8b061e2 --- /dev/null +++ b/doc/release-notes-27302.md @@ -0,0 +1,4 @@ +Configuration +--- + +- `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. diff --git a/src/common/init.cpp b/src/common/init.cpp index 6ffa44847ac23..60df14de9abe9 100644 --- a/src/common/init.cpp +++ b/src/common/init.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -20,6 +21,19 @@ std::optional InitConfig(ArgsManager& args, SettingsAbortFn setting if (!CheckDataDirOption(args)) { return ConfigError{ConfigStatus::FAILED, strprintf(_("Specified data directory \"%s\" does not exist."), args.GetArg("-datadir", ""))}; } + + // Record original datadir and config paths before parsing the config + // file. It is possible for the config file to contain a datadir= line + // that changes the datadir path after it is parsed. This is useful for + // CLI tools to let them use a different data storage location without + // needing to pass it every time on the command line. (It is not + // possible for the config file to cause another configuration to be + // used, though. Specifying a conf= option in the config file causes a + // parse error, and specifying a datadir= location containing another + // bitcoin.conf file just ignores the other file.) + const fs::path orig_datadir_path{args.GetDataDirBase()}; + const fs::path orig_config_path = args.GetConfigFilePath(); + std::string error; if (!args.ReadConfigFiles(error, true)) { return ConfigError{ConfigStatus::FAILED, strprintf(_("Error reading configuration file: %s"), error)}; @@ -48,6 +62,32 @@ std::optional InitConfig(ArgsManager& args, SettingsAbortFn setting fs::create_directories(net_path / "wallets"); } + // Show an error or warning if there is a bitcoin.conf file in the + // datadir that is being ignored. + const fs::path base_config_path = base_path / BITCOIN_CONF_FILENAME; + if (fs::exists(base_config_path) && !fs::equivalent(orig_config_path, base_config_path)) { + const std::string cli_config_path = args.GetArg("-conf", ""); + const std::string config_source = cli_config_path.empty() + ? strprintf("data directory %s", fs::quoted(fs::PathToString(orig_datadir_path))) + : strprintf("command line argument %s", fs::quoted("-conf=" + cli_config_path)); + const std::string error = strprintf( + "Data directory %1$s contains a %2$s file which is ignored, because a different configuration file " + "%3$s from %4$s is being used instead. Possible ways to address this would be to:\n" + "- Delete or rename the %2$s file in data directory %1$s.\n" + "- Change datadir= or conf= options to specify one configuration file, not two, and use " + "includeconf= to include any other configuration files.\n" + "- Set allowignoredconf=1 option to treat this condition as a warning, not an error.", + fs::quoted(fs::PathToString(base_path)), + fs::quoted(BITCOIN_CONF_FILENAME), + fs::quoted(fs::PathToString(orig_config_path)), + config_source); + if (args.GetBoolArg("-allowignoredconf", false)) { + LogPrintf("Warning: %s\n", error); + } else { + return ConfigError{ConfigStatus::FAILED, Untranslated(error)}; + } + } + // Create settings.json if -nosettings was not specified. if (args.GetSettingsPath()) { std::vector details; diff --git a/src/init.cpp b/src/init.cpp index 525648b81202b..78db4a7a82095 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -441,6 +441,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS); argsman.AddArg("-dbcache=", strprintf("Maximum database cache size MiB (%d to %d, default: %d). In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-includeconf=", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-allowignoredconf", strprintf("For backwards compatibility, treat an unused %s file in the datadir as a warning, not an error.", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-loadblock=", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxmempool=", strprintf("Keep the transaction memory pool below megabytes (default: %u)", DEFAULT_MAX_MEMPOOL_SIZE_MB), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxorphantx=", strprintf("Keep at most unconnectable transactions in memory (default: %u)", DEFAULT_MAX_ORPHAN_TRANSACTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index f9730b48c58a8..a45888513f786 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -5,9 +5,14 @@ """Test various command line arguments and configuration file parameters.""" import os +import pathlib +import re +import sys +import tempfile import time from test_framework.test_framework import BitcoinTestFramework +from test_framework.test_node import ErrorMatch from test_framework import util @@ -74,7 +79,7 @@ def test_config_file_parser(self): util.write_config(main_conf_file_path, n=0, chain='', extra_config=f'includeconf={inc_conf_file_path}\n') with open(inc_conf_file_path, 'w', encoding='utf-8') as conf: conf.write('acceptnonstdtxn=1\n') - self.nodes[0].assert_start_raises_init_error(extra_args=[f"-conf={main_conf_file_path}"], expected_msg='Error: acceptnonstdtxn is not currently supported for main chain') + self.nodes[0].assert_start_raises_init_error(extra_args=[f"-conf={main_conf_file_path}", "-allowignoredconf"], expected_msg='Error: acceptnonstdtxn is not currently supported for main chain') with open(inc_conf_file_path, 'w', encoding='utf-8') as conf: conf.write('nono\n') @@ -282,6 +287,55 @@ def test_connect_with_seednode(self): unexpected_msgs=seednode_ignored): self.restart_node(0, extra_args=[connect_arg, '-seednode=fakeaddress2']) + def test_ignored_conf(self): + self.log.info('Test error is triggered when the datadir in use contains a bitcoin.conf file that would be ignored ' + 'because a conflicting -conf file argument is passed.') + node = self.nodes[0] + with tempfile.NamedTemporaryFile(dir=self.options.tmpdir, mode="wt", delete=False) as temp_conf: + temp_conf.write(f"datadir={node.datadir}\n") + node.assert_start_raises_init_error([f"-conf={temp_conf.name}"], re.escape( + f'Error: Data directory "{node.datadir}" contains a "bitcoin.conf" file which is ignored, because a ' + f'different configuration file "{temp_conf.name}" from command line argument "-conf={temp_conf.name}" ' + f'is being used instead.') + r"[\s\S]*", match=ErrorMatch.FULL_REGEX) + + # Test that passing a redundant -conf command line argument pointing to + # the same bitcoin.conf that would be loaded anyway does not trigger an + # error. + self.start_node(0, [f'-conf={node.datadir}/bitcoin.conf']) + self.stop_node(0) + + def test_ignored_default_conf(self): + # Disable this test for windows currently because trying to override + # the default datadir through the environment does not seem to work. + if sys.platform == "win32": + return + + self.log.info('Test error is triggered when bitcoin.conf in the default data directory sets another datadir ' + 'and it contains a different bitcoin.conf file that would be ignored') + + # Create a temporary directory that will be treated as the default data + # directory by bitcoind. + env, default_datadir = util.get_temp_default_datadir(pathlib.Path(self.options.tmpdir, "home")) + default_datadir.mkdir(parents=True) + + # Write a bitcoin.conf file in the default data directory containing a + # datadir= line pointing at the node datadir. This will trigger a + # startup error because the node datadir contains a different + # bitcoin.conf that would be ignored. + node = self.nodes[0] + (default_datadir / "bitcoin.conf").write_text(f"datadir={node.datadir}\n") + + # Drop the node -datadir= argument during this test, because if it is + # specified it would take precedence over the datadir setting in the + # config file. + node_args = node.args + node.args = [arg for arg in node.args if not arg.startswith("-datadir=")] + node.assert_start_raises_init_error([], re.escape( + f'Error: Data directory "{node.datadir}" contains a "bitcoin.conf" file which is ignored, because a ' + f'different configuration file "{default_datadir}/bitcoin.conf" from data directory "{default_datadir}" ' + f'is being used instead.') + r"[\s\S]*", env=env, match=ErrorMatch.FULL_REGEX) + node.args = node_args + def run_test(self): self.test_log_buffer() self.test_args_log() @@ -291,6 +345,8 @@ def run_test(self): self.test_config_file_parser() self.test_invalid_command_line_options() + self.test_ignored_conf() + self.test_ignored_default_conf() # Remove the -datadir argument so it doesn't override the config file self.nodes[0].args = [arg for arg in self.nodes[0].args if not arg.startswith("-datadir")] diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 56abe5f26af2d..51bd697e819f6 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -190,7 +190,7 @@ def __getattr__(self, name): assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection") return getattr(RPCOverloadWrapper(self.rpc, descriptors=self.descriptors), name) - def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, **kwargs): + def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, env=None, **kwargs): """Start the node.""" if extra_args is None: extra_args = self.extra_args @@ -213,6 +213,8 @@ def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, **kwargs # add environment variable LIBC_FATAL_STDERR_=1 so that libc errors are written to stderr and not the terminal subp_env = dict(os.environ, LIBC_FATAL_STDERR_="1") + if env is not None: + subp_env.update(env) self.process = subprocess.Popen(self.args + extra_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs) diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index a1b90860f67bc..c75db60afe48a 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -12,14 +12,16 @@ import json import logging import os +import pathlib import random import re +import sys import time import unittest from . import coverage from .authproxy import AuthServiceProxy, JSONRPCException -from typing import Callable, Optional +from typing import Callable, Optional, Tuple logger = logging.getLogger("TestFramework.utils") @@ -420,6 +422,22 @@ def get_datadir_path(dirname, n): return os.path.join(dirname, "node" + str(n)) +def get_temp_default_datadir(temp_dir: pathlib.Path) -> Tuple[dict, pathlib.Path]: + """Return os-specific environment variables that can be set to make the + GetDefaultDataDir() function return a datadir path under the provided + temp_dir, as well as the complete path it would return.""" + if sys.platform == "win32": + env = dict(APPDATA=str(temp_dir)) + datadir = temp_dir / "Bitcoin" + else: + env = dict(HOME=str(temp_dir)) + if sys.platform == "darwin": + datadir = temp_dir / "Library/Application Support/Bitcoin" + else: + datadir = temp_dir / ".bitcoin" + return env, datadir + + def append_config(datadir, options): with open(os.path.join(datadir, "bitcoin.conf"), 'a', encoding='utf8') as f: for option in options: From eefe56967b4eb4b5144325cde4f40fc1cbde3e65 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 27 Mar 2023 10:17:57 -0400 Subject: [PATCH 0032/1751] bugfix: Fix incorrect debug.log config file path Currently debug.log will show the wrong bitcoin.conf config file path when bitcoind is invoked without -conf or -datadir arguments, and there's a default bitcoin.conf file which specifies another datadir= location. When this happens, the debug.log will include an incorrect "Config file:" line referring to a bitcoin.conf file in the other datadir, instead of the referring to the actual configuration file in the default datadir which was parsed. The bad log print was reported and originally fixed in https://github.com/bitcoin/bitcoin/pull/27303 by Matthew Zipkin This PR takes a slightly different approach to fixing the bug, trying to avoid future bugs by not allowing the GetConfigFilePath function to be called before the the configuration is parsed, and deleting GetConfigFile function which could be confused with GetConfigFilePath. It also includes a test for the bug which the original fix did not have. Co-authored-by: Matthew Zipkin --- src/common/args.cpp | 3 ++- src/common/args.h | 2 +- src/common/config.cpp | 8 ++---- src/common/init.cpp | 2 +- src/qt/test/test_main.cpp | 3 +++ test/functional/feature_config_args.py | 36 ++++++++++++++++++++++++++ 6 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/common/args.cpp b/src/common/args.cpp index d29b8648bf493..ccd49eb8a3d2e 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -714,7 +714,8 @@ bool CheckDataDirOption(const ArgsManager& args) fs::path ArgsManager::GetConfigFilePath() const { - return GetConfigFile(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME)); + LOCK(cs_args); + return *Assert(m_config_path); } std::string ArgsManager::GetChainName() const diff --git a/src/common/args.h b/src/common/args.h index 430c392e2bc97..1e463e98d7c15 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -26,7 +26,6 @@ extern const char * const BITCOIN_SETTINGS_FILENAME; // Return true if -datadir option points to a valid directory or is not specified. bool CheckDataDirOption(const ArgsManager& args); -fs::path GetConfigFile(const ArgsManager& args, const fs::path& configuration_file_path); /** * Most paths passed as configuration arguments are treated as relative to @@ -136,6 +135,7 @@ class ArgsManager std::map> m_available_args GUARDED_BY(cs_args); bool m_accept_any_command GUARDED_BY(cs_args){true}; std::list m_config_sections GUARDED_BY(cs_args); + std::optional m_config_path GUARDED_BY(cs_args); mutable fs::path m_cached_blocks_path GUARDED_BY(cs_args); mutable fs::path m_cached_datadir_path GUARDED_BY(cs_args); mutable fs::path m_cached_network_datadir_path GUARDED_BY(cs_args); diff --git a/src/common/config.cpp b/src/common/config.cpp index 747503ad2ac2f..6bc71ffa0df3a 100644 --- a/src/common/config.cpp +++ b/src/common/config.cpp @@ -26,11 +26,6 @@ #include #include -fs::path GetConfigFile(const ArgsManager& args, const fs::path& configuration_file_path) -{ - return AbsPathForConfigVal(args, configuration_file_path, /*net_specific=*/false); -} - static bool GetConfigOptions(std::istream& stream, const std::string& filepath, std::string& error, std::vector>& options, std::list& sections) { std::string str, prefix; @@ -125,6 +120,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) LOCK(cs_args); m_settings.ro_config.clear(); m_config_sections.clear(); + m_config_path = AbsPathForConfigVal(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false); } const auto conf_path{GetConfigFilePath()}; @@ -175,7 +171,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) const size_t default_includes = add_includes({}); for (const std::string& conf_file_name : conf_file_names) { - std::ifstream conf_file_stream{GetConfigFile(*this, fs::PathFromString(conf_file_name))}; + std::ifstream conf_file_stream{AbsPathForConfigVal(*this, fs::PathFromString(conf_file_name), /*net_specific=*/false)}; if (conf_file_stream.good()) { if (!ReadConfigStream(conf_file_stream, conf_file_name, error, ignore_invalid_keys)) { return false; diff --git a/src/common/init.cpp b/src/common/init.cpp index 60df14de9abe9..f5a412b1a1388 100644 --- a/src/common/init.cpp +++ b/src/common/init.cpp @@ -32,7 +32,7 @@ std::optional InitConfig(ArgsManager& args, SettingsAbortFn setting // parse error, and specifying a datadir= location containing another // bitcoin.conf file just ignores the other file.) const fs::path orig_datadir_path{args.GetDataDirBase()}; - const fs::path orig_config_path = args.GetConfigFilePath(); + const fs::path orig_config_path{AbsPathForConfigVal(args, args.GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false)}; std::string error; if (!args.ReadConfigFiles(error, true)) { diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp index 2d069f76a0dba..a0cf80cd3111d 100644 --- a/src/qt/test/test_main.cpp +++ b/src/qt/test/test_main.cpp @@ -70,6 +70,9 @@ int main(int argc, char* argv[]) gArgs.ForceSetArg("-upnp", "0"); gArgs.ForceSetArg("-natpmp", "0"); + std::string error; + if (!gArgs.ReadConfigFiles(error, true)) QWARN(error.c_str()); + // Prefer the "minimal" platform for the test instead of the normal default // platform ("xcb", "windows", or "cocoa") so tests can't unintentionally // interfere with any background GUIs and don't require extra resources. diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index a45888513f786..2257605870a82 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -113,6 +113,41 @@ def test_config_file_parser(self): with open(inc_conf_file2_path, 'w', encoding='utf-8') as conf: conf.write('') # clear + def test_config_file_log(self): + # Disable this test for windows currently because trying to override + # the default datadir through the environment does not seem to work. + if sys.platform == "win32": + return + + self.log.info('Test that correct configuration path is changed when configuration file changes the datadir') + + # Create a temporary directory that will be treated as the default data + # directory by bitcoind. + env, default_datadir = util.get_temp_default_datadir(pathlib.Path(self.options.tmpdir, "test_config_file_log")) + default_datadir.mkdir(parents=True) + + # Write a bitcoin.conf file in the default data directory containing a + # datadir= line pointing at the node datadir. + node = self.nodes[0] + conf_text = pathlib.Path(node.bitcoinconf).read_text() + conf_path = default_datadir / "bitcoin.conf" + conf_path.write_text(f"datadir={node.datadir}\n{conf_text}") + + # Drop the node -datadir= argument during this test, because if it is + # specified it would take precedence over the datadir setting in the + # config file. + node_args = node.args + node.args = [arg for arg in node.args if not arg.startswith("-datadir=")] + + # Check that correct configuration file path is actually logged + # (conf_path, not node.bitcoinconf) + with self.nodes[0].assert_debug_log(expected_msgs=[f"Config file: {conf_path}"]): + self.start_node(0, ["-allowignoredconf"], env=env) + self.stop_node(0) + + # Restore node arguments after the test + node.args = node_args + def test_invalid_command_line_options(self): self.nodes[0].assert_start_raises_init_error( expected_msg='Error: Error parsing command line arguments: Can not set -proxy with no value. Please specify value with -proxy=value.', @@ -344,6 +379,7 @@ def run_test(self): self.test_connect_with_seednode() self.test_config_file_parser() + self.test_config_file_log() self.test_invalid_command_line_options() self.test_ignored_conf() self.test_ignored_default_conf() From 096487c4dcfadebe5ca959927f5426cae1c304d5 Mon Sep 17 00:00:00 2001 From: ishaanam Date: Tue, 18 Apr 2023 14:42:18 -0400 Subject: [PATCH 0033/1751] wallet: introduce generic recursive tx state updating function This commit also changed `MarkConflicted` and `AbandonTransaction` to use this new function Co-authored-by: ariard --- src/wallet/wallet.cpp | 93 ++++++++++++++++++++----------------------- src/wallet/wallet.h | 7 ++++ 2 files changed, 51 insertions(+), 49 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 56bd25b90a19e..86cea876c2616 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1264,11 +1264,6 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) { LOCK(cs_wallet); - WalletBatch batch(GetDatabase()); - - std::set todo; - std::set done; - // Can't mark abandoned if confirmed or in mempool auto it = mapWallet.find(hashTx); assert(it != mapWallet.end()); @@ -1277,44 +1272,25 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) return false; } - todo.insert(hashTx); - - while (!todo.empty()) { - uint256 now = *todo.begin(); - todo.erase(now); - done.insert(now); - auto it = mapWallet.find(now); - assert(it != mapWallet.end()); - CWalletTx& wtx = it->second; - int currentconfirm = GetTxDepthInMainChain(wtx); - // If the orig tx was not in block, none of its spends can be - assert(currentconfirm <= 0); - // if (currentconfirm < 0) {Tx and spends are already conflicted, no need to abandon} - if (currentconfirm == 0 && !wtx.isAbandoned()) { - // If the orig tx was not in block/mempool, none of its spends can be in mempool - assert(!wtx.InMempool()); + auto try_updating_state = [](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { + // If the orig tx was not in block/mempool, none of its spends can be. + assert(!wtx.isConfirmed()); + assert(!wtx.InMempool()); + // If already conflicted or abandoned, no need to set abandoned + if (!wtx.isConflicted() && !wtx.isAbandoned()) { wtx.m_state = TxStateInactive{/*abandoned=*/true}; - wtx.MarkDirty(); - batch.WriteTx(wtx); - NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED); - // Iterate over all its outputs, and mark transactions in the wallet that spend them abandoned too. - // States are not permanent, so these transactions can become unabandoned if they are re-added to the - // mempool, or confirmed in a block, or conflicted. - // Note: If the reorged coinbase is re-added to the main chain, the descendants that have not had their - // states change will remain abandoned and will require manual broadcast if the user wants them. - for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) { - std::pair range = mapTxSpends.equal_range(COutPoint(now, i)); - for (TxSpends::const_iterator iter = range.first; iter != range.second; ++iter) { - if (!done.count(iter->second)) { - todo.insert(iter->second); - } - } - } - // If a transaction changes 'conflicted' state, that changes the balance - // available of the outputs it spends. So force those to be recomputed - MarkInputsDirty(wtx.tx); + return TxUpdate::NOTIFY_CHANGED; } - } + return TxUpdate::UNCHANGED; + }; + + // Iterate over all its outputs, and mark transactions in the wallet that spend them abandoned too. + // States are not permanent, so these transactions can become unabandoned if they are re-added to the + // mempool, or confirmed in a block, or conflicted. + // Note: If the reorged coinbase is re-added to the main chain, the descendants that have not had their + // states change will remain abandoned and will require manual broadcast if the user wants them. + + RecursiveUpdateTxState(hashTx, try_updating_state); return true; } @@ -1331,13 +1307,29 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c if (conflictconfirms >= 0) return; + auto try_updating_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { + if (conflictconfirms < GetTxDepthInMainChain(wtx)) { + // Block is 'more conflicted' than current confirm; update. + // Mark transaction as conflicted with this block. + wtx.m_state = TxStateConflicted{hashBlock, conflicting_height}; + return TxUpdate::CHANGED; + } + return TxUpdate::UNCHANGED; + }; + + // Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too. + RecursiveUpdateTxState(hashTx, try_updating_state); + +} + +void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { // Do not flush the wallet here for performance reasons WalletBatch batch(GetDatabase(), false); std::set todo; std::set done; - todo.insert(hashTx); + todo.insert(tx_hash); while (!todo.empty()) { uint256 now = *todo.begin(); @@ -1346,14 +1338,12 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c auto it = mapWallet.find(now); assert(it != mapWallet.end()); CWalletTx& wtx = it->second; - int currentconfirm = GetTxDepthInMainChain(wtx); - if (conflictconfirms < currentconfirm) { - // Block is 'more conflicted' than current confirm; update. - // Mark transaction as conflicted with this block. - wtx.m_state = TxStateConflicted{hashBlock, conflicting_height}; + + TxUpdate update_state = try_updating_state(wtx); + if (update_state != TxUpdate::UNCHANGED) { wtx.MarkDirty(); batch.WriteTx(wtx); - // Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too + // Iterate over all its outputs, and update those tx states as well (if applicable) for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) { std::pair range = mapTxSpends.equal_range(COutPoint(now, i)); for (TxSpends::const_iterator iter = range.first; iter != range.second; ++iter) { @@ -1362,7 +1352,12 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c } } } - // If a transaction changes 'conflicted' state, that changes the balance + + if (update_state == TxUpdate::NOTIFY_CHANGED) { + NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED); + } + + // If a transaction changes its tx state, that usually changes the balance // available of the outputs it spends. So force those to be recomputed MarkInputsDirty(wtx.tx); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 581a6bd9cb136..e304570fc832a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -325,6 +325,13 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati /** Mark a transaction (and its in-wallet descendants) as conflicting with a particular block. */ void MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx); + enum class TxUpdate { UNCHANGED, CHANGED, NOTIFY_CHANGED }; + + using TryUpdatingStateFn = std::function; + + /** Mark a transaction (and its in-wallet descendants) as a particular tx state. */ + void RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Mark a transaction's inputs dirty, thus forcing the outputs to be recomputed */ void MarkInputsDirty(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From ea7b8528490d330f0f4e34e9b26ab00ba528f546 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 30 Oct 2022 12:54:37 +0000 Subject: [PATCH 0034/1751] build: Use newest `config.{guess,sub}` available --- autogen.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/autogen.sh b/autogen.sh index de16260b56327..d0ac7ef7ed3f1 100755 --- a/autogen.sh +++ b/autogen.sh @@ -14,3 +14,12 @@ fi command -v autoreconf >/dev/null || \ (echo "configuration failed, please install autoconf first" && exit 1) autoreconf --install --force --warnings=all + +if expr "'$(build-aux/config.guess --timestamp)" \< "'$(depends/config.guess --timestamp)" > /dev/null; then + cp depends/config.guess build-aux + cp depends/config.guess src/secp256k1/build-aux +fi +if expr "'$(build-aux/config.sub --timestamp)" \< "'$(depends/config.sub --timestamp)" > /dev/null; then + cp depends/config.sub build-aux + cp depends/config.sub src/secp256k1/build-aux +fi From 53c990ad3406ee945305af84af98d2f020e5f316 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Tue, 25 Apr 2023 08:19:50 +0200 Subject: [PATCH 0035/1751] test: fix `feature_addrman.py` on big-endian systems The test `feature_addrman.py` currently serializes the addrdb without specifying endianness for `int`s, so the machine's native byte order is used (see https://docs.python.org/3/library/struct.html#byte-order-size-and-alignment) and the generated `peers.dat` would be invalid on big-endian systems. Fix this by explicitly specifying little-endian serialization via the `<` character in `struct.pack(...)`. --- test/functional/feature_addrman.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py index 28c3880513288..57a26b50302bc 100755 --- a/test/functional/feature_addrman.py +++ b/test/functional/feature_addrman.py @@ -32,12 +32,12 @@ def serialize_addrman( r += struct.pack("B", format) r += struct.pack("B", INCOMPATIBILITY_BASE + lowest_compatible) r += ser_uint256(bucket_key) - r += struct.pack("i", len_new or len(new)) - r += struct.pack("i", len_tried or len(tried)) + r += struct.pack(" Date: Sun, 30 Apr 2023 18:18:33 +0200 Subject: [PATCH 0036/1751] fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()` Co-authored-by: Amiti Uttarwar Co-authored-by: Martin Zumsande --- src/test/fuzz/addrman.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 5ad7a25c53d8e..d1ba654212729 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -299,12 +299,20 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman) }); } const AddrMan& const_addr_man{addr_man}; + std::optional network; + if (fuzzed_data_provider.ConsumeBool()) { + network = fuzzed_data_provider.PickValueInArray(ALL_NETWORKS); + } (void)const_addr_man.GetAddr( /*max_addresses=*/fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), /*max_pct=*/fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), - /*network=*/std::nullopt); - (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool()); - (void)const_addr_man.Size(); + network); + (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network); + std::optional in_new; + if (fuzzed_data_provider.ConsumeBool()) { + in_new = fuzzed_data_provider.ConsumeBool(); + } + (void)const_addr_man.Size(network, in_new); CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION); data_stream << const_addr_man; } From 33c6245ac1ecdfe25b1ee4fd9e93c43393634ae3 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 12 Dec 2022 14:54:01 -0500 Subject: [PATCH 0037/1751] Introduce MockableDatabase for wallet unit tests MockableDatabase is a WalletDatabase that allows us to interact with the records to change them independently from the wallet, as well as changing the return values from within the tests. This will give us greater flexibility in testing the wallet. --- .../libtest_util/libtest_util.vcxproj.in | 1 + src/wallet/test/util.cpp | 90 +++++++++++++++++++ src/wallet/test/util.h | 74 +++++++++++++++ src/wallet/test/wallet_tests.cpp | 53 +---------- 4 files changed, 167 insertions(+), 51 deletions(-) diff --git a/build_msvc/libtest_util/libtest_util.vcxproj.in b/build_msvc/libtest_util/libtest_util.vcxproj.in index b5e844010e919..64cfa82dccd50 100644 --- a/build_msvc/libtest_util/libtest_util.vcxproj.in +++ b/build_msvc/libtest_util/libtest_util.vcxproj.in @@ -8,6 +8,7 @@ StaticLibrary + @SOURCE_FILES@ diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index b7bf312edf1a2..7d0a814ed4475 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -79,4 +80,93 @@ CTxDestination getNewDestination(CWallet& w, OutputType output_type) return *Assert(w.GetNewDestination(output_type, "")); } +DatabaseCursor::Status MockableCursor::Next(DataStream& key, DataStream& value) +{ + if (!m_pass) { + return Status::FAIL; + } + if (m_cursor == m_cursor_end) { + return Status::DONE; + } + const auto& [key_data, value_data] = *m_cursor; + key.write(key_data); + value.write(value_data); + m_cursor++; + return Status::MORE; +} + +bool MockableBatch::ReadKey(DataStream&& key, DataStream& value) +{ + if (!m_pass) { + return false; + } + SerializeData key_data{key.begin(), key.end()}; + const auto& it = m_records.find(key_data); + if (it == m_records.end()) { + return false; + } + value.write(it->second); + return true; +} + +bool MockableBatch::WriteKey(DataStream&& key, DataStream&& value, bool overwrite) +{ + if (!m_pass) { + return false; + } + SerializeData key_data{key.begin(), key.end()}; + SerializeData value_data{value.begin(), value.end()}; + auto [it, inserted] = m_records.emplace(key_data, value_data); + if (!inserted && overwrite) { // Overwrite if requested + it->second = value_data; + inserted = true; + } + return inserted; +} + +bool MockableBatch::EraseKey(DataStream&& key) +{ + if (!m_pass) { + return false; + } + SerializeData key_data{key.begin(), key.end()}; + m_records.erase(key_data); + return true; +} + +bool MockableBatch::HasKey(DataStream&& key) +{ + if (!m_pass) { + return false; + } + SerializeData key_data{key.begin(), key.end()}; + return m_records.count(key_data) > 0; +} + +bool MockableBatch::ErasePrefix(Span prefix) +{ + if (!m_pass) { + return false; + } + auto it = m_records.begin(); + while (it != m_records.end()) { + auto& key = it->first; + if (key.size() < prefix.size() || std::search(key.begin(), key.end(), prefix.begin(), prefix.end()) != key.begin()) { + it++; + continue; + } + it = m_records.erase(it); + } + return true; +} + +std::unique_ptr CreateMockableWalletDatabase(std::map records) +{ + return std::make_unique(records); +} + +MockableDatabase& GetMockableDatabase(CWallet& wallet) +{ + return dynamic_cast(wallet.GetDatabase()); +} } // namespace wallet diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index d726517e21112..92405107cf23a 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -6,6 +6,8 @@ #define BITCOIN_WALLET_TEST_UTIL_H #include