diff options
42 files changed, 621 insertions, 292 deletions
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8112f2828b..7961c3c900 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -104,8 +104,8 @@ jobs: HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK: 1 run: | # A workaround for "The `brew link` step did not complete successfully" error. - brew install python@3 || brew link --overwrite python@3 - brew install automake libtool pkg-config gnu-getopt ccache boost libevent miniupnpc libnatpmp zeromq qt@5 qrencode + brew install --quiet python@3 || brew link --overwrite python@3 + brew install --quiet automake libtool pkg-config gnu-getopt ccache boost libevent miniupnpc libnatpmp zeromq qt@5 qrencode - name: Set Ccache directory run: echo "CCACHE_DIR=${RUNNER_TEMP}/ccache_dir" >> "$GITHUB_ENV" diff --git a/ci/lint/04_install.sh b/ci/lint/04_install.sh index ff9206fb6f..acec2f32e9 100755 --- a/ci/lint/04_install.sh +++ b/ci/lint/04_install.sh @@ -53,6 +53,7 @@ ${CI_RETRY_EXE} pip3 install \ lief==0.13.2 \ mypy==1.4.1 \ pyzmq==25.1.0 \ + ruff==0.5.5 \ vulture==2.6 SHELLCHECK_VERSION=v0.8.0 diff --git a/contrib/devtools/copyright_header.py b/contrib/devtools/copyright_header.py index 3dddffe324..3c98ee7b20 100755 --- a/contrib/devtools/copyright_header.py +++ b/contrib/devtools/copyright_header.py @@ -19,7 +19,6 @@ EXCLUDE = [ 'src/qt/bitcoinstrings.cpp', 'src/chainparamsseeds.h', # other external copyrights: - 'src/reverse_iterator.h', 'src/test/fuzz/FuzzedDataProvider.h', 'src/tinyformat.h', 'src/bench/nanobench.h', diff --git a/contrib/guix/libexec/prelude.bash b/contrib/guix/libexec/prelude.bash index 5bc828625e..9cc151c706 100644 --- a/contrib/guix/libexec/prelude.bash +++ b/contrib/guix/libexec/prelude.bash @@ -51,7 +51,7 @@ fi time-machine() { # shellcheck disable=SC2086 guix time-machine --url=https://git.savannah.gnu.org/git/guix.git \ - --commit=efc26826400762207cde9f23802cfe75a737963c \ + --commit=7bf1d7aeaffba15c4f680f93ae88fbef25427252 \ --cores="$JOBS" \ --keep-failed \ --fallback \ diff --git a/doc/build-windows.md b/doc/build-windows.md index e35439bfe1..cba99bca23 100644 --- a/doc/build-windows.md +++ b/doc/build-windows.md @@ -66,7 +66,6 @@ is to temporarily disable WSL support for Win32 applications. Build using: - PATH=$(echo "$PATH" | sed -e 's/:\/mnt.*//g') # strip out problematic Windows %PATH% imported var sudo bash -c "echo 0 > /proc/sys/fs/binfmt_misc/status" # Disable WSL support for Win32 applications. cd depends make HOST=x86_64-w64-mingw32 diff --git a/doc/productivity.md b/doc/productivity.md index e9b7bc270c..323d86f7b8 100644 --- a/doc/productivity.md +++ b/doc/productivity.md @@ -33,7 +33,7 @@ The easiest way to faster compile times is to cache compiles. `ccache` is a way Install `ccache` through your distribution's package manager, and run `./configure` with your normal flags to pick it up. -To use ccache for all your C/C++ projects, follow the symlinks method [here](https://ccache.samba.org/manual/latest.html#_run_modes) to set it up. +To use ccache for all your C/C++ projects, follow the symlinks method [here](https://ccache.dev/manual/latest.html#_run_modes) to set it up. To get the most out of ccache, put something like this in `~/.ccache/ccache.conf`: diff --git a/doc/release-notes-29775.md b/doc/release-notes-29775.md new file mode 100644 index 0000000000..6cb3ed3e8d --- /dev/null +++ b/doc/release-notes-29775.md @@ -0,0 +1,10 @@ +Testnet4/BIP94 support +----- + +Support for Testnet4 as specified in [BIP94](https://github.com/bitcoin/bips/blob/master/bip-0094.mediawiki) +has been added. The network can be selected with the `-testnet4` option and +the section header is also named `[testnet4]`. + +While the intention is to phase out support for Testnet3 in an upcoming +version, support for it is still available via the known options in this +release. diff --git a/src/Makefile.am b/src/Makefile.am index 36de5dd150..1ccb5332c4 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -257,7 +257,6 @@ BITCOIN_CORE_H = \ random.h \ randomenv.h \ rest.h \ - reverse_iterator.h \ rpc/blockchain.h \ rpc/client.h \ rpc/mempool.h \ diff --git a/src/Makefile.test.include b/src/Makefile.test.include index e2d6f94b24..3a6d56c9e1 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -85,6 +85,7 @@ BITCOIN_TESTS =\ test/checkqueue_tests.cpp \ test/cluster_linearize_tests.cpp \ test/coins_tests.cpp \ + test/coinscachepair_tests.cpp \ test/coinstatsindex_tests.cpp \ test/common_url_tests.cpp \ test/compilerbug_tests.cpp \ diff --git a/src/chainparamsbase.cpp b/src/chainparamsbase.cpp index 2b57b4ca55..1c1b97430b 100644 --- a/src/chainparamsbase.cpp +++ b/src/chainparamsbase.cpp @@ -17,7 +17,7 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman) argsman.AddArg("-regtest", "Enter regression test mode, which uses a special chain in which blocks can be solved instantly. " "This is intended for regression testing tools and app development. Equivalent to -chain=regtest.", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS); argsman.AddArg("-testactivationheight=name@height.", "Set the activation height of 'name' (segwit, bip34, dersig, cltv, csv). (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); - argsman.AddArg("-testnet", "Use the testnet3 chain. Equivalent to -chain=test. Support for testnet3 is deprecated and will be removed with the next release. Consider moving to testnet4 now by using -testnet4.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); + argsman.AddArg("-testnet", "Use the testnet3 chain. Equivalent to -chain=test. Support for testnet3 is deprecated and will be removed in an upcoming release. Consider moving to testnet4 now by using -testnet4.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); argsman.AddArg("-testnet4", "Use the testnet4 chain. Equivalent to -chain=testnet4.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); argsman.AddArg("-vbparams=deployment:start:end[:min_activation_height]", "Use given start/end times and min_activation_height for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS); argsman.AddArg("-signet", "Use the signet chain. Equivalent to -chain=signet. Note that the network is defined by the -signetchallenge parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); diff --git a/src/coins.cpp b/src/coins.cpp index a4e4d4ad32..91e02331f5 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -12,7 +12,7 @@ bool CCoinsView::GetCoin(const COutPoint &outpoint, Coin &coin) const { return false; } uint256 CCoinsView::GetBestBlock() const { return uint256(); } std::vector<uint256> CCoinsView::GetHeadBlocks() const { return std::vector<uint256>(); } -bool CCoinsView::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { return false; } +bool CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return false; } std::unique_ptr<CCoinsViewCursor> CCoinsView::Cursor() const { return nullptr; } bool CCoinsView::HaveCoin(const COutPoint &outpoint) const @@ -27,14 +27,16 @@ bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base-> uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); } std::vector<uint256> CCoinsViewBacked::GetHeadBlocks() const { return base->GetHeadBlocks(); } void CCoinsViewBacked::SetBackend(CCoinsView &viewIn) { base = &viewIn; } -bool CCoinsViewBacked::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { return base->BatchWrite(mapCoins, hashBlock, erase); } +bool CCoinsViewBacked::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return base->BatchWrite(cursor, hashBlock); } std::unique_ptr<CCoinsViewCursor> CCoinsViewBacked::Cursor() const { return base->Cursor(); } size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); } CCoinsViewCache::CCoinsViewCache(CCoinsView* baseIn, bool deterministic) : CCoinsViewBacked(baseIn), m_deterministic(deterministic), cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/deterministic), CCoinsMap::key_equal{}, &m_cache_coins_memory_resource) -{} +{ + m_sentinel.second.SelfRef(m_sentinel); +} size_t CCoinsViewCache::DynamicMemoryUsage() const { return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsage; @@ -51,7 +53,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const if (ret->second.coin.IsSpent()) { // The parent only has an empty entry for this outpoint; we can consider our // version as fresh. - ret->second.flags = CCoinsCacheEntry::FRESH; + ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel); } cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage(); return ret; @@ -93,10 +95,10 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi // // If the coin doesn't exist in the current cache, or is spent but not // DIRTY, then it can be marked FRESH. - fresh = !(it->second.flags & CCoinsCacheEntry::DIRTY); + fresh = !it->second.IsDirty(); } it->second.coin = std::move(coin); - it->second.flags |= CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0); + it->second.AddFlags(CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0), *it, m_sentinel); cachedCoinsUsage += it->second.coin.DynamicMemoryUsage(); TRACE5(utxocache, add, outpoint.hash.data(), @@ -108,10 +110,13 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) { cachedCoinsUsage += coin.DynamicMemoryUsage(); - cacheCoins.emplace( + auto [it, inserted] = cacheCoins.emplace( std::piecewise_construct, std::forward_as_tuple(std::move(outpoint)), - std::forward_as_tuple(std::move(coin), CCoinsCacheEntry::DIRTY)); + std::forward_as_tuple(std::move(coin))); + if (inserted) { + it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel); + } } void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) { @@ -138,10 +143,10 @@ bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { if (moveout) { *moveout = std::move(it->second.coin); } - if (it->second.flags & CCoinsCacheEntry::FRESH) { + if (it->second.IsFresh()) { cacheCoins.erase(it); } else { - it->second.flags |= CCoinsCacheEntry::DIRTY; + it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel); it->second.coin.Clear(); } return true; @@ -178,42 +183,40 @@ void CCoinsViewCache::SetBestBlock(const uint256 &hashBlockIn) { hashBlock = hashBlockIn; } -bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn, bool erase) { - for (CCoinsMap::iterator it = mapCoins.begin(); - it != mapCoins.end(); - it = erase ? mapCoins.erase(it) : std::next(it)) { +bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlockIn) { + for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { // Ignore non-dirty entries (optimization). - if (!(it->second.flags & CCoinsCacheEntry::DIRTY)) { + if (!it->second.IsDirty()) { continue; } CCoinsMap::iterator itUs = cacheCoins.find(it->first); if (itUs == cacheCoins.end()) { // The parent cache does not have an entry, while the child cache does. // We can ignore it if it's both spent and FRESH in the child - if (!(it->second.flags & CCoinsCacheEntry::FRESH && it->second.coin.IsSpent())) { + if (!(it->second.IsFresh() && it->second.coin.IsSpent())) { // Create the coin in the parent cache, move the data up // and mark it as dirty. - CCoinsCacheEntry& entry = cacheCoins[it->first]; - if (erase) { - // The `move` call here is purely an optimization; we rely on the - // `mapCoins.erase` call in the `for` expression to actually remove - // the entry from the child map. + itUs = cacheCoins.try_emplace(it->first).first; + CCoinsCacheEntry& entry{itUs->second}; + if (cursor.WillErase(*it)) { + // Since this entry will be erased, + // we can move the coin into us instead of copying it entry.coin = std::move(it->second.coin); } else { entry.coin = it->second.coin; } cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); - entry.flags = CCoinsCacheEntry::DIRTY; + entry.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel); // We can mark it FRESH in the parent if it was FRESH in the child // Otherwise it might have just been flushed from the parent's cache // and already exist in the grandparent - if (it->second.flags & CCoinsCacheEntry::FRESH) { - entry.flags |= CCoinsCacheEntry::FRESH; + if (it->second.IsFresh()) { + entry.AddFlags(CCoinsCacheEntry::FRESH, *itUs, m_sentinel); } } } else { // Found the entry in the parent cache - if ((it->second.flags & CCoinsCacheEntry::FRESH) && !itUs->second.coin.IsSpent()) { + if (it->second.IsFresh() && !itUs->second.coin.IsSpent()) { // The coin was marked FRESH in the child cache, but the coin // exists in the parent cache. If this ever happens, it means // the FRESH flag was misapplied and there is a logic error in @@ -221,7 +224,7 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn throw std::logic_error("FRESH flag misapplied to coin that exists in parent cache"); } - if ((itUs->second.flags & CCoinsCacheEntry::FRESH) && it->second.coin.IsSpent()) { + if (itUs->second.IsFresh() && it->second.coin.IsSpent()) { // The grandparent cache does not have an entry, and the coin // has been spent. We can just delete it from the parent cache. cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage(); @@ -229,16 +232,15 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn } else { // A normal modification. cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage(); - if (erase) { - // The `move` call here is purely an optimization; we rely on the - // `mapCoins.erase` call in the `for` expression to actually remove - // the entry from the child map. + if (cursor.WillErase(*it)) { + // Since this entry will be erased, + // we can move the coin into us instead of copying it itUs->second.coin = std::move(it->second.coin); } else { itUs->second.coin = it->second.coin; } cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage(); - itUs->second.flags |= CCoinsCacheEntry::DIRTY; + itUs->second.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel); // NOTE: It isn't safe to mark the coin as FRESH in the parent // cache. If it already existed and was spent in the parent // cache then marking it FRESH would prevent that spentness @@ -251,12 +253,10 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn } bool CCoinsViewCache::Flush() { - bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/true); + auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/true)}; + bool fOk = base->BatchWrite(cursor, hashBlock); if (fOk) { - if (!cacheCoins.empty()) { - /* BatchWrite must erase all cacheCoins elements when erase=true. */ - throw std::logic_error("Not all cached coins were erased"); - } + cacheCoins.clear(); ReallocateCache(); } cachedCoinsUsage = 0; @@ -265,16 +265,12 @@ bool CCoinsViewCache::Flush() { bool CCoinsViewCache::Sync() { - bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/false); - // Instead of clearing `cacheCoins` as we would in Flush(), just clear the - // FRESH/DIRTY flags of any coin that isn't spent. - for (auto it = cacheCoins.begin(); it != cacheCoins.end(); ) { - if (it->second.coin.IsSpent()) { - cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); - it = cacheCoins.erase(it); - } else { - it->second.flags = 0; - ++it; + auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/false)}; + bool fOk = base->BatchWrite(cursor, hashBlock); + if (fOk) { + if (m_sentinel.second.Next() != &m_sentinel) { + /* BatchWrite must clear flags of all entries */ + throw std::logic_error("Not all unspent flagged entries were cleared"); } } return fOk; @@ -283,7 +279,7 @@ bool CCoinsViewCache::Sync() void CCoinsViewCache::Uncache(const COutPoint& hash) { CCoinsMap::iterator it = cacheCoins.find(hash); - if (it != cacheCoins.end() && it->second.flags == 0) { + if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) { cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); TRACE5(utxocache, uncache, hash.hash.data(), @@ -324,17 +320,33 @@ void CCoinsViewCache::ReallocateCache() void CCoinsViewCache::SanityCheck() const { size_t recomputed_usage = 0; + size_t count_flagged = 0; for (const auto& [_, entry] : cacheCoins) { unsigned attr = 0; - if (entry.flags & CCoinsCacheEntry::DIRTY) attr |= 1; - if (entry.flags & CCoinsCacheEntry::FRESH) attr |= 2; + if (entry.IsDirty()) attr |= 1; + if (entry.IsFresh()) attr |= 2; if (entry.coin.IsSpent()) attr |= 4; // Only 5 combinations are possible. assert(attr != 2 && attr != 4 && attr != 7); // Recompute cachedCoinsUsage. recomputed_usage += entry.coin.DynamicMemoryUsage(); + + // Count the number of entries we expect in the linked list. + if (entry.IsDirty() || entry.IsFresh()) ++count_flagged; + } + // Iterate over the linked list of flagged entries. + size_t count_linked = 0; + for (auto it = m_sentinel.second.Next(); it != &m_sentinel; it = it->second.Next()) { + // Verify linked list integrity. + assert(it->second.Next()->second.Prev() == it); + assert(it->second.Prev()->second.Next() == it); + // Verify they are actually flagged. + assert(it->second.IsDirty() || it->second.IsFresh()); + // Count the number of entries actually in the list. + ++count_linked; } + assert(count_linked == count_flagged); assert(recomputed_usage == cachedCoinsUsage); } diff --git a/src/coins.h b/src/coins.h index 76e64b641d..78b8eddacd 100644 --- a/src/coins.h +++ b/src/coins.h @@ -13,6 +13,7 @@ #include <serialize.h> #include <support/allocators/pool.h> #include <uint256.h> +#include <util/check.h> #include <util/hasher.h> #include <assert.h> @@ -86,6 +87,9 @@ public: } }; +struct CCoinsCacheEntry; +using CoinsCachePair = std::pair<const COutPoint, CCoinsCacheEntry>; + /** * A Coin in one level of the coins database caching hierarchy. * @@ -103,8 +107,29 @@ public: */ struct CCoinsCacheEntry { +private: + /** + * These are used to create a doubly linked list of flagged entries. + * They are set in AddFlags and unset in ClearFlags. + * A flagged entry is any entry that is either DIRTY, FRESH, or both. + * + * DIRTY entries are tracked so that only modified entries can be passed to + * the parent cache for batch writing. This is a performance optimization + * compared to giving all entries in the cache to the parent and having the + * parent scan for only modified entries. + * + * FRESH-but-not-DIRTY coins can not occur in practice, since that would + * mean a spent coin exists in the parent CCoinsView and not in the child + * CCoinsViewCache. Nevertheless, if a spent coin is retrieved from the + * parent cache, the FRESH-but-not-DIRTY coin will be tracked by the linked + * list and deleted when Sync or Flush is called on the CCoinsViewCache. + */ + CoinsCachePair* m_prev{nullptr}; + CoinsCachePair* m_next{nullptr}; + uint8_t m_flags{0}; + +public: Coin coin; // The actual cached data. - unsigned char flags; enum Flags { /** @@ -127,9 +152,59 @@ struct CCoinsCacheEntry FRESH = (1 << 1), }; - CCoinsCacheEntry() : flags(0) {} - explicit CCoinsCacheEntry(Coin&& coin_) : coin(std::move(coin_)), flags(0) {} - CCoinsCacheEntry(Coin&& coin_, unsigned char flag) : coin(std::move(coin_)), flags(flag) {} + CCoinsCacheEntry() noexcept = default; + explicit CCoinsCacheEntry(Coin&& coin_) noexcept : coin(std::move(coin_)) {} + ~CCoinsCacheEntry() + { + ClearFlags(); + } + + //! Adding a flag also requires a self reference to the pair that contains + //! this entry in the CCoinsCache map and a reference to the sentinel of the + //! flagged pair linked list. + inline void AddFlags(uint8_t flags, CoinsCachePair& self, CoinsCachePair& sentinel) noexcept + { + Assume(&self.second == this); + if (!m_flags && flags) { + m_prev = sentinel.second.m_prev; + m_next = &sentinel; + sentinel.second.m_prev = &self; + m_prev->second.m_next = &self; + } + m_flags |= flags; + } + inline void ClearFlags() noexcept + { + if (!m_flags) return; + m_next->second.m_prev = m_prev; + m_prev->second.m_next = m_next; + m_flags = 0; + } + inline uint8_t GetFlags() const noexcept { return m_flags; } + inline bool IsDirty() const noexcept { return m_flags & DIRTY; } + inline bool IsFresh() const noexcept { return m_flags & FRESH; } + + //! Only call Next when this entry is DIRTY, FRESH, or both + inline CoinsCachePair* Next() const noexcept { + Assume(m_flags); + return m_next; + } + + //! Only call Prev when this entry is DIRTY, FRESH, or both + inline CoinsCachePair* Prev() const noexcept { + Assume(m_flags); + return m_prev; + } + + //! Only use this for initializing the linked list sentinel + inline void SelfRef(CoinsCachePair& self) noexcept + { + Assume(&self.second == this); + m_prev = &self; + m_next = &self; + // Set sentinel to DIRTY so we can call Next on it + m_flags = DIRTY; + } }; /** @@ -144,8 +219,8 @@ using CCoinsMap = std::unordered_map<COutPoint, CCoinsCacheEntry, SaltedOutpointHasher, std::equal_to<COutPoint>, - PoolAllocator<std::pair<const COutPoint, CCoinsCacheEntry>, - sizeof(std::pair<const COutPoint, CCoinsCacheEntry>) + sizeof(void*) * 4>>; + PoolAllocator<CoinsCachePair, + sizeof(CoinsCachePair) + sizeof(void*) * 4>>; using CCoinsMapMemoryResource = CCoinsMap::allocator_type::ResourceType; @@ -168,6 +243,62 @@ private: uint256 hashBlock; }; +/** + * Cursor for iterating over the linked list of flagged entries in CCoinsViewCache. + * + * This is a helper struct to encapsulate the diverging logic between a non-erasing + * CCoinsViewCache::Sync and an erasing CCoinsViewCache::Flush. This allows the receiver + * of CCoinsView::BatchWrite to iterate through the flagged entries without knowing + * the caller's intent. + * + * However, the receiver can still call CoinsViewCacheCursor::WillErase to see if the + * caller will erase the entry after BatchWrite returns. If so, the receiver can + * perform optimizations such as moving the coin out of the CCoinsCachEntry instead + * of copying it. + */ +struct CoinsViewCacheCursor +{ + //! If will_erase is not set, iterating through the cursor will erase spent coins from the map, + //! and other coins will be unflagged (removing them from the linked list). + //! If will_erase is set, the underlying map and linked list will not be modified, + //! as the caller is expected to wipe the entire map anyway. + //! This is an optimization compared to erasing all entries as the cursor iterates them when will_erase is set. + //! Calling CCoinsMap::clear() afterwards is faster because a CoinsCachePair cannot be coerced back into a + //! CCoinsMap::iterator to be erased, and must therefore be looked up again by key in the CCoinsMap before being erased. + CoinsViewCacheCursor(size_t& usage LIFETIMEBOUND, + CoinsCachePair& sentinel LIFETIMEBOUND, + CCoinsMap& map LIFETIMEBOUND, + bool will_erase) noexcept + : m_usage(usage), m_sentinel(sentinel), m_map(map), m_will_erase(will_erase) {} + + inline CoinsCachePair* Begin() const noexcept { return m_sentinel.second.Next(); } + inline CoinsCachePair* End() const noexcept { return &m_sentinel; } + + //! Return the next entry after current, possibly erasing current + inline CoinsCachePair* NextAndMaybeErase(CoinsCachePair& current) noexcept + { + const auto next_entry{current.second.Next()}; + // If we are not going to erase the cache, we must still erase spent entries. + // Otherwise clear the flags on the entry. + if (!m_will_erase) { + if (current.second.coin.IsSpent()) { + m_usage -= current.second.coin.DynamicMemoryUsage(); + m_map.erase(current.first); + } else { + current.second.ClearFlags(); + } + } + return next_entry; + } + + inline bool WillErase(CoinsCachePair& current) const noexcept { return m_will_erase || current.second.coin.IsSpent(); } +private: + size_t& m_usage; + CoinsCachePair& m_sentinel; + CCoinsMap& m_map; + bool m_will_erase; +}; + /** Abstract view on the open txout dataset. */ class CCoinsView { @@ -191,8 +322,8 @@ public: virtual std::vector<uint256> GetHeadBlocks() const; //! Do a bulk modification (multiple Coin changes + BestBlock change). - //! The passed mapCoins can be modified. - virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true); + //! The passed cursor is used to iterate through the coins. + virtual bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock); //! Get a cursor to iterate over the whole state virtual std::unique_ptr<CCoinsViewCursor> Cursor() const; @@ -218,7 +349,7 @@ public: uint256 GetBestBlock() const override; std::vector<uint256> GetHeadBlocks() const override; void SetBackend(CCoinsView &viewIn); - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override; + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override; std::unique_ptr<CCoinsViewCursor> Cursor() const override; size_t EstimateSize() const override; }; @@ -237,6 +368,8 @@ protected: */ mutable uint256 hashBlock; mutable CCoinsMapMemoryResource m_cache_coins_memory_resource{}; + /* The starting sentinel of the flagged entry circular doubly linked list. */ + mutable CoinsCachePair m_sentinel; mutable CCoinsMap cacheCoins; /* Cached dynamic memory usage for the inner Coin objects. */ @@ -255,7 +388,7 @@ public: bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; void SetBestBlock(const uint256 &hashBlock); - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override; + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override; std::unique_ptr<CCoinsViewCursor> Cursor() const override { throw std::logic_error("CCoinsViewCache cursor iteration not supported."); } diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp index bbddc3dfec..98a52001ab 100644 --- a/src/kernel/chainparams.cpp +++ b/src/kernel/chainparams.cpp @@ -334,7 +334,7 @@ public: consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT; consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay - consensus.nMinimumChainWork = uint256{}; + consensus.nMinimumChainWork = uint256{"000000000000000000000000000000000000000000000056faca98a0cd9bdf5f"}; consensus.defaultAssumeValid = uint256{}; pchMessageStart[0] = 0x1c; @@ -685,6 +685,7 @@ std::optional<ChainType> GetNetworkForMagic(const MessageStartChars& message) { const auto mainnet_msg = CChainParams::Main()->MessageStart(); const auto testnet_msg = CChainParams::TestNet()->MessageStart(); + const auto testnet4_msg = CChainParams::TestNet4()->MessageStart(); const auto regtest_msg = CChainParams::RegTest({})->MessageStart(); const auto signet_msg = CChainParams::SigNet({})->MessageStart(); @@ -692,6 +693,8 @@ std::optional<ChainType> GetNetworkForMagic(const MessageStartChars& message) return ChainType::MAIN; } else if (std::equal(message.begin(), message.end(), testnet_msg.data())) { return ChainType::TESTNET; + } else if (std::equal(message.begin(), message.end(), testnet4_msg.data())) { + return ChainType::TESTNET4; } else if (std::equal(message.begin(), message.end(), regtest_msg.data())) { return ChainType::REGTEST; } else if (std::equal(message.begin(), message.end(), signet_msg.data())) { diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0b9b8fbbe2..13ea3a29be 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -32,7 +32,6 @@ #include <primitives/block.h> #include <primitives/transaction.h> #include <random.h> -#include <reverse_iterator.h> #include <scheduler.h> #include <streams.h> #include <sync.h> @@ -51,6 +50,7 @@ #include <future> #include <memory> #include <optional> +#include <ranges> #include <typeinfo> #include <utility> @@ -1518,9 +1518,20 @@ void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int co return; } - if (state->pindexLastCommonBlock == nullptr) { - // Bootstrap quickly by guessing a parent of our best tip is the forking point. - // Guessing wrong in either direction is not a problem. + // When we sync with AssumeUtxo and discover the snapshot is not in the peer's best chain, abort: + // We can't reorg to this chain due to missing undo data until the background sync has finished, + // so downloading blocks from it would be futile. + const CBlockIndex* snap_base{m_chainman.GetSnapshotBaseBlock()}; + if (snap_base && state->pindexBestKnownBlock->GetAncestor(snap_base->nHeight) != snap_base) { + LogDebug(BCLog::NET, "Not downloading blocks from peer=%d, which doesn't have the snapshot block in its best chain.\n", peer.m_id); + return; + } + + // Bootstrap quickly by guessing a parent of our best tip is the forking point. + // Guessing wrong in either direction is not a problem. + // Also reset pindexLastCommonBlock after a snapshot was loaded, so that blocks after the snapshot will be prioritised for download. + if (state->pindexLastCommonBlock == nullptr || + (snap_base && state->pindexLastCommonBlock->nHeight < snap_base->nHeight)) { state->pindexLastCommonBlock = m_chainman.ActiveChain()[std::min(state->pindexBestKnownBlock->nHeight, m_chainman.ActiveChain().Height())]; } @@ -2259,7 +2270,7 @@ void PeerManagerImpl::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlock for (auto& it : m_peer_map) { Peer& peer = *it.second; LOCK(peer.m_block_inv_mutex); - for (const uint256& hash : reverse_iterate(vHashes)) { + for (const uint256& hash : vHashes | std::views::reverse) { peer.m_blocks_for_headers_relay.push_back(hash); } } @@ -2958,7 +2969,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c } else { std::vector<CInv> vGetData; // Download as much as possible, from earliest to latest. - for (const CBlockIndex *pindex : reverse_iterate(vToFetch)) { + for (const CBlockIndex* pindex : vToFetch | std::views::reverse) { if (nodestate->vBlocksInFlight.size() >= MAX_BLOCKS_IN_TRANSIT_PER_PEER) { // Can't download any more from this peer break; diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 4cff587d51..96cf69927c 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -20,7 +20,6 @@ #include <primitives/block.h> #include <primitives/transaction.h> #include <random.h> -#include <reverse_iterator.h> #include <serialize.h> #include <signet.h> #include <span.h> @@ -38,6 +37,7 @@ #include <validation.h> #include <map> +#include <ranges> #include <unordered_map> namespace kernel { @@ -579,7 +579,7 @@ const CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data) { const MapCheckpoints& checkpoints = data.mapCheckpoints; - for (const MapCheckpoints::value_type& i : reverse_iterate(checkpoints)) { + for (const MapCheckpoints::value_type& i : checkpoints | std::views::reverse) { const uint256& hash = i.second; const CBlockIndex* pindex = LookupBlockIndex(hash); if (pindex) { diff --git a/src/node/utxo_snapshot.h b/src/node/utxo_snapshot.h index a7c4135787..e4eb6d60ad 100644 --- a/src/node/utxo_snapshot.h +++ b/src/node/utxo_snapshot.h @@ -28,16 +28,17 @@ class Chainstate; namespace node { //! Metadata describing a serialized version of a UTXO set from which an //! assumeutxo Chainstate can be constructed. +//! All metadata fields come from an untrusted file, so must be validated +//! before being used. Thus, new fields should be added only if needed. class SnapshotMetadata { - const uint16_t m_version{1}; - const std::set<uint16_t> m_supported_versions{1}; + inline static const uint16_t VERSION{2}; + const std::set<uint16_t> m_supported_versions{VERSION}; const MessageStartChars m_network_magic; public: //! The hash of the block that reflects the tip of the chain for the //! UTXO set contained in this snapshot. uint256 m_base_blockhash; - uint32_t m_base_blockheight; //! The number of coins in the UTXO set contained in this snapshot. Used @@ -50,19 +51,16 @@ public: SnapshotMetadata( const MessageStartChars network_magic, const uint256& base_blockhash, - const int base_blockheight, uint64_t coins_count) : m_network_magic(network_magic), m_base_blockhash(base_blockhash), - m_base_blockheight(base_blockheight), m_coins_count(coins_count) { } template <typename Stream> inline void Serialize(Stream& s) const { s << SNAPSHOT_MAGIC_BYTES; - s << m_version; + s << VERSION; s << m_network_magic; - s << m_base_blockheight; s << m_base_blockhash; s << m_coins_count; } @@ -98,7 +96,6 @@ public: } } - s >> m_base_blockheight; s >> m_base_blockhash; s >> m_coins_count; } diff --git a/src/reverse_iterator.h b/src/reverse_iterator.h deleted file mode 100644 index 4db001c04b..0000000000 --- a/src/reverse_iterator.h +++ /dev/null @@ -1,39 +0,0 @@ -// Taken from https://gist.github.com/arvidsson/7231973 - -#ifndef BITCOIN_REVERSE_ITERATOR_H -#define BITCOIN_REVERSE_ITERATOR_H - -/** - * Template used for reverse iteration in range-based for loops. - * - * std::vector<int> v = {1, 2, 3, 4, 5}; - * for (auto x : reverse_iterate(v)) - * std::cout << x << " "; - */ - -template <typename T> -class reverse_range -{ - T &m_x; - -public: - explicit reverse_range(T &x) : m_x(x) {} - - auto begin() const -> decltype(this->m_x.rbegin()) - { - return m_x.rbegin(); - } - - auto end() const -> decltype(this->m_x.rend()) - { - return m_x.rend(); - } -}; - -template <typename T> -reverse_range<T> reverse_iterate(T &x) -{ - return reverse_range<T>(x); -} - -#endif // BITCOIN_REVERSE_ITERATOR_H diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 02f9ecef34..f85561e0a8 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -2160,14 +2160,14 @@ static const auto scan_result_status_some = RPCResult{ static RPCHelpMan scantxoutset() { - // scriptPubKey corresponding to mainnet address 12cbQLTFMXRnSzktFkuoG3eHoMeFtpTu3S + // raw() descriptor corresponding to mainnet address 12cbQLTFMXRnSzktFkuoG3eHoMeFtpTu3S const std::string EXAMPLE_DESCRIPTOR_RAW = "raw(76a91411b366edfc0a8b66feebae5c2e25a7b6a5d1cf3188ac)#fm24fxxy"; return RPCHelpMan{"scantxoutset", "\nScans the unspent transaction output set for entries that match certain output descriptors.\n" "Examples of output descriptors are:\n" - " addr(<address>) Outputs whose scriptPubKey corresponds to the specified address (does not include P2PK)\n" - " raw(<hex script>) Outputs whose scriptPubKey equals the specified hex scripts\n" + " addr(<address>) Outputs whose output script corresponds to the specified address (does not include P2PK)\n" + " raw(<hex script>) Outputs whose output script equals the specified hex-encoded bytes\n" " combo(<pubkey>) P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH outputs for the given pubkey\n" " pkh(<pubkey>) P2PKH outputs for the given pubkey\n" " sh(multi(<n>,<pubkey>,<pubkey>,...)) P2SH-multisig outputs for the given threshold and pubkeys\n" @@ -2196,8 +2196,8 @@ static RPCHelpMan scantxoutset() { {RPCResult::Type::STR_HEX, "txid", "The transaction id"}, {RPCResult::Type::NUM, "vout", "The vout value"}, - {RPCResult::Type::STR_HEX, "scriptPubKey", "The script key"}, - {RPCResult::Type::STR, "desc", "A specialized descriptor for the matched scriptPubKey"}, + {RPCResult::Type::STR_HEX, "scriptPubKey", "The output script"}, + {RPCResult::Type::STR, "desc", "A specialized descriptor for the matched output script"}, {RPCResult::Type::STR_AMOUNT, "amount", "The total amount in " + CURRENCY_UNIT + " of the unspent output"}, {RPCResult::Type::BOOL, "coinbase", "Whether this is a coinbase output"}, {RPCResult::Type::NUM, "height", "Height of the unspent transaction output"}, @@ -2741,7 +2741,7 @@ UniValue CreateUTXOSnapshot( tip->nHeight, tip->GetBlockHash().ToString(), fs::PathToString(path), fs::PathToString(temppath))); - SnapshotMetadata metadata{chainstate.m_chainman.GetParams().MessageStart(), tip->GetBlockHash(), tip->nHeight, maybe_stats->coins_count}; + SnapshotMetadata metadata{chainstate.m_chainman.GetParams().MessageStart(), tip->GetBlockHash(), maybe_stats->coins_count}; afile << metadata; @@ -2865,10 +2865,12 @@ static RPCHelpMan loadtxoutset() throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Unable to load UTXO snapshot: %s. (%s)", util::ErrorString(activation_result).original, path.utf8string())); } + CBlockIndex& snapshot_index{*CHECK_NONFATAL(*activation_result)}; + UniValue result(UniValue::VOBJ); result.pushKV("coins_loaded", metadata.m_coins_count); - result.pushKV("tip_hash", metadata.m_base_blockhash.ToString()); - result.pushKV("base_height", metadata.m_base_blockheight); + result.pushKV("tip_hash", snapshot_index.GetBlockHash().ToString()); + result.pushKV("base_height", snapshot_index.nHeight); result.pushKV("path", fs::PathToString(path)); return result; }, diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp index 65a9be2762..01a9e59284 100644 --- a/src/rpc/output_script.cpp +++ b/src/rpc/output_script.cpp @@ -38,7 +38,7 @@ static RPCHelpMan validateaddress() { {RPCResult::Type::BOOL, "isvalid", "If the address is valid or not"}, {RPCResult::Type::STR, "address", /*optional=*/true, "The bitcoin address validated"}, - {RPCResult::Type::STR_HEX, "scriptPubKey", /*optional=*/true, "The hex-encoded scriptPubKey generated by the address"}, + {RPCResult::Type::STR_HEX, "scriptPubKey", /*optional=*/true, "The hex-encoded output script generated by the address"}, {RPCResult::Type::BOOL, "isscript", /*optional=*/true, "If the key is a script"}, {RPCResult::Type::BOOL, "iswitness", /*optional=*/true, "If the address is a witness address"}, {RPCResult::Type::NUM, "witness_version", /*optional=*/true, "The version number of the witness program"}, @@ -217,7 +217,7 @@ static RPCHelpMan deriveaddresses() " pkh(<pubkey>) P2PKH outputs for the given pubkey\n" " wpkh(<pubkey>) Native segwit P2PKH outputs for the given pubkey\n" " sh(multi(<n>,<pubkey>,<pubkey>,...)) P2SH-multisig outputs for the given threshold and pubkeys\n" - " raw(<hex script>) Outputs whose scriptPubKey equals the specified hex scripts\n" + " raw(<hex script>) Outputs whose output script equals the specified hex-encoded bytes\n" " tr(<pubkey>,multi_a(<n>,<pubkey>,<pubkey>,...)) P2TR-multisig outputs for the given threshold and pubkeys\n" "\nIn the above, <pubkey> either refers to a fixed public key in hexadecimal notation, or to an xpub/xprv optionally followed by one\n" "or more path elements separated by \"/\", where \"h\" represents a hardened child key.\n" diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index d7e2c6e24c..21bc0e52f1 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -737,7 +737,7 @@ static RPCHelpMan signrawtransactionwithkey() { {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"}, {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output number"}, - {"scriptPubKey", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "script key"}, + {"scriptPubKey", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "output script"}, {"redeemScript", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "(required for P2SH) redeem script"}, {"witnessScript", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "(required for P2WSH or P2SH-P2WSH) witness script"}, {"amount", RPCArg::Type::AMOUNT, RPCArg::Optional::OMITTED, "(required for Segwit inputs) the amount spent"}, @@ -1840,8 +1840,8 @@ static RPCHelpMan analyzepsbt() { {RPCResult::Type::STR_HEX, "keyid", "Public key ID, hash160 of the public key, of a public key whose signature is missing"}, }}, - {RPCResult::Type::STR_HEX, "redeemscript", /*optional=*/true, "Hash160 of the redeemScript that is missing"}, - {RPCResult::Type::STR_HEX, "witnessscript", /*optional=*/true, "SHA256 of the witnessScript that is missing"}, + {RPCResult::Type::STR_HEX, "redeemscript", /*optional=*/true, "Hash160 of the redeem script that is missing"}, + {RPCResult::Type::STR_HEX, "witnessscript", /*optional=*/true, "SHA256 of the witness script that is missing"}, }}, {RPCResult::Type::STR, "next", /*optional=*/true, "Role of the next person that this input needs to go to"}, }}, diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index a992e2fa03..fb929a2b0e 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -55,10 +55,10 @@ public: uint256 GetBestBlock() const override { return hashBestBlock_; } - bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock, bool erase = true) override + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) override { - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = erase ? mapCoins.erase(it) : std::next(it)) { - if (it->second.flags & CCoinsCacheEntry::DIRTY) { + for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)){ + if (it->second.IsDirty()) { // Same optimization used in CCoinsViewDB is to only write dirty entries. map_[it->first] = it->second.coin; if (it->second.coin.IsSpent() && InsecureRandRange(3) == 0) { @@ -78,7 +78,7 @@ class CCoinsViewCacheTest : public CCoinsViewCache public: explicit CCoinsViewCacheTest(CCoinsView* _base) : CCoinsViewCache(_base) {} - void SelfTest() const + void SelfTest(bool sanity_check = true) const { // Manually recompute the dynamic usage of the whole data, and compare it. size_t ret = memusage::DynamicUsage(cacheCoins); @@ -89,9 +89,13 @@ public: } BOOST_CHECK_EQUAL(GetCacheSize(), count); BOOST_CHECK_EQUAL(DynamicMemoryUsage(), ret); + if (sanity_check) { + SanityCheck(); + } } CCoinsMap& map() const { return cacheCoins; } + CoinsCachePair& sentinel() const { return m_sentinel; } size_t& usage() const { return cachedCoinsUsage; } }; @@ -576,7 +580,7 @@ static void SetCoinsValue(CAmount value, Coin& coin) } } -static size_t InsertCoinsMapEntry(CCoinsMap& map, CAmount value, char flags) +static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, CAmount value, char flags) { if (value == ABSENT) { assert(flags == NO_ENTRY); @@ -584,10 +588,10 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CAmount value, char flags) } assert(flags != NO_ENTRY); CCoinsCacheEntry entry; - entry.flags = flags; SetCoinsValue(value, entry.coin); auto inserted = map.emplace(OUTPOINT, std::move(entry)); assert(inserted.second); + inserted.first->second.AddFlags(flags, *inserted.first, sentinel); return inserted.first->second.coin.DynamicMemoryUsage(); } @@ -603,17 +607,20 @@ void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags, const C } else { value = it->second.coin.out.nValue; } - flags = it->second.flags; + flags = it->second.GetFlags(); assert(flags != NO_ENTRY); } } void WriteCoinsViewEntry(CCoinsView& view, CAmount value, char flags) { + CoinsCachePair sentinel{}; + sentinel.second.SelfRef(sentinel); CCoinsMapMemoryResource resource; CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; - InsertCoinsMapEntry(map, value, flags); - BOOST_CHECK(view.BatchWrite(map, {})); + auto usage{InsertCoinsMapEntry(map, sentinel, value, flags)}; + auto cursor{CoinsViewCacheCursor(usage, sentinel, map, /*will_erase=*/true)}; + BOOST_CHECK(view.BatchWrite(cursor, {})); } class SingleEntryCacheTest @@ -622,7 +629,7 @@ public: SingleEntryCacheTest(CAmount base_value, CAmount cache_value, char cache_flags) { WriteCoinsViewEntry(base, base_value, base_value == ABSENT ? NO_ENTRY : DIRTY); - cache.usage() += InsertCoinsMapEntry(cache.map(), cache_value, cache_flags); + cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), cache_value, cache_flags); } CCoinsView root; @@ -634,7 +641,7 @@ static void CheckAccessCoin(CAmount base_value, CAmount cache_value, CAmount exp { SingleEntryCacheTest test(base_value, cache_value, cache_flags); test.cache.AccessCoin(OUTPOINT); - test.cache.SelfTest(); + test.cache.SelfTest(/*sanity_check=*/false); CAmount result_value; char result_flags; @@ -803,7 +810,7 @@ void CheckWriteCoins(CAmount parent_value, CAmount child_value, CAmount expected char result_flags; try { WriteCoinsViewEntry(test.cache, child_value, child_flags); - test.cache.SelfTest(); + test.cache.SelfTest(/*sanity_check=*/false); GetCoinsMapEntry(test.cache.map(), result_value, result_flags); } catch (std::logic_error&) { result_value = FAIL; @@ -916,6 +923,7 @@ void TestFlushBehavior( // Flush in reverse order to ensure that flushes happen from children up. for (auto i = all_caches.rbegin(); i != all_caches.rend(); ++i) { auto& cache = *i; + cache->SanityCheck(); // hashBlock must be filled before flushing to disk; value is // unimportant here. This is normally done during connect/disconnect block. cache->SetBestBlock(InsecureRand256()); diff --git a/src/test/coinscachepair_tests.cpp b/src/test/coinscachepair_tests.cpp new file mode 100644 index 0000000000..61840f1f09 --- /dev/null +++ b/src/test/coinscachepair_tests.cpp @@ -0,0 +1,219 @@ +// Copyright (c) 2024-present 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 <coins.h> + +#include <boost/test/unit_test.hpp> + +#include <list> + +BOOST_AUTO_TEST_SUITE(coinscachepair_tests) + +static constexpr auto NUM_NODES{4}; + +std::list<CoinsCachePair> CreatePairs(CoinsCachePair& sentinel) +{ + std::list<CoinsCachePair> nodes; + for (auto i{0}; i < NUM_NODES; ++i) { + nodes.emplace_back(); + + auto node{std::prev(nodes.end())}; + node->second.AddFlags(CCoinsCacheEntry::DIRTY, *node, sentinel); + + BOOST_CHECK_EQUAL(node->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(node->second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*node)); + + if (i > 0) { + BOOST_CHECK_EQUAL(std::prev(node)->second.Next(), &(*node)); + BOOST_CHECK_EQUAL(node->second.Prev(), &(*std::prev(node))); + } + } + return nodes; +} + +BOOST_AUTO_TEST_CASE(linked_list_iteration) +{ + CoinsCachePair sentinel; + sentinel.second.SelfRef(sentinel); + auto nodes{CreatePairs(sentinel)}; + + // Check iterating through pairs is identical to iterating through a list + auto node{sentinel.second.Next()}; + for (const auto& expected : nodes) { + BOOST_CHECK_EQUAL(&expected, node); + node = node->second.Next(); + } + BOOST_CHECK_EQUAL(node, &sentinel); + + // Check iterating through pairs is identical to iterating through a list + // Clear the flags during iteration + node = sentinel.second.Next(); + for (const auto& expected : nodes) { + BOOST_CHECK_EQUAL(&expected, node); + auto next = node->second.Next(); + node->second.ClearFlags(); + node = next; + } + BOOST_CHECK_EQUAL(node, &sentinel); + // Check that sentinel's next and prev are itself + BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel); + + // Delete the nodes from the list to make sure there are no dangling pointers + for (auto it{nodes.begin()}; it != nodes.end(); it = nodes.erase(it)) { + BOOST_CHECK_EQUAL(it->second.GetFlags(), 0); + } +} + +BOOST_AUTO_TEST_CASE(linked_list_iterate_erase) +{ + CoinsCachePair sentinel; + sentinel.second.SelfRef(sentinel); + auto nodes{CreatePairs(sentinel)}; + + // Check iterating through pairs is identical to iterating through a list + // Erase the nodes as we iterate through, but don't clear flags + // The flags will be cleared by the CCoinsCacheEntry's destructor + auto node{sentinel.second.Next()}; + for (auto expected{nodes.begin()}; expected != nodes.end(); expected = nodes.erase(expected)) { + BOOST_CHECK_EQUAL(&(*expected), node); + node = node->second.Next(); + } + BOOST_CHECK_EQUAL(node, &sentinel); + + // Check that sentinel's next and prev are itself + BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel); +} + +BOOST_AUTO_TEST_CASE(linked_list_random_deletion) +{ + CoinsCachePair sentinel; + sentinel.second.SelfRef(sentinel); + auto nodes{CreatePairs(sentinel)}; + + // Create linked list sentinel->n1->n2->n3->n4->sentinel + auto n1{nodes.begin()}; + auto n2{std::next(n1)}; + auto n3{std::next(n2)}; + auto n4{std::next(n3)}; + + // Delete n2 + // sentinel->n1->n3->n4->sentinel + nodes.erase(n2); + // Check that n1 now points to n3, and n3 still points to n4 + // Also check that flags were not altered + BOOST_CHECK_EQUAL(n1->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(n1->second.Next(), &(*n3)); + BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4)); + BOOST_CHECK_EQUAL(n3->second.Prev(), &(*n1)); + + // Delete n1 + // sentinel->n3->n4->sentinel + nodes.erase(n1); + // Check that sentinel now points to n3, and n3 still points to n4 + // Also check that flags were not altered + BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3)); + BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4)); + BOOST_CHECK_EQUAL(n3->second.Prev(), &sentinel); + + // Delete n4 + // sentinel->n3->sentinel + nodes.erase(n4); + // Check that sentinel still points to n3, and n3 points to sentinel + // Also check that flags were not altered + BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3)); + BOOST_CHECK_EQUAL(n3->second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*n3)); + + // Delete n3 + // sentinel->sentinel + nodes.erase(n3); + // Check that sentinel's next and prev are itself + BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel); +} + +BOOST_AUTO_TEST_CASE(linked_list_add_flags) +{ + CoinsCachePair sentinel; + sentinel.second.SelfRef(sentinel); + CoinsCachePair n1; + CoinsCachePair n2; + + // Check that adding 0 flag has no effect + n1.second.AddFlags(0, n1, sentinel); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel); + + // Check that adding DIRTY flag inserts it into linked list and sets flags + n1.second.AddFlags(CCoinsCacheEntry::DIRTY, n1, sentinel); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1); + + // Check that adding FRESH flag on new node inserts it after n1 + n2.second.AddFlags(CCoinsCacheEntry::FRESH, n2, sentinel); + BOOST_CHECK_EQUAL(n2.second.GetFlags(), CCoinsCacheEntry::FRESH); + BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); + BOOST_CHECK_EQUAL(n1.second.Next(), &n2); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); + + // Check that adding 0 flag has no effect, and doesn't change position + n1.second.AddFlags(0, n1, sentinel); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(n1.second.Next(), &n2); + BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1); + BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); + + // Check that we can add extra flags, but they don't change our position + n1.second.AddFlags(CCoinsCacheEntry::FRESH, n1, sentinel); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH); + BOOST_CHECK_EQUAL(n1.second.Next(), &n2); + BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1); + BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); + + // Check that we can clear flags then re-add them + n1.second.ClearFlags(); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); + BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel); + + // Check that calling `ClearFlags` with 0 flags has no effect + n1.second.ClearFlags(); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); + BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel); + + // Adding 0 still has no effect + n1.second.AddFlags(0, n1, sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); + BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel); + + // But adding DIRTY re-inserts it after n2 + n1.second.AddFlags(CCoinsCacheEntry::DIRTY, n1, sentinel); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(n2.second.Next(), &n1); + BOOST_CHECK_EQUAL(n1.second.Prev(), &n2); + BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 41c971c237..368c69819a 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -120,12 +120,15 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) random_mutable_transaction = *opt_mutable_transaction; }, [&] { + CoinsCachePair sentinel{}; + sentinel.second.SelfRef(sentinel); + size_t usage{0}; CCoinsMapMemoryResource resource; CCoinsMap coins_map{0, SaltedOutpointHasher{/*deterministic=*/true}, CCoinsMap::key_equal{}, &resource}; LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000) { CCoinsCacheEntry coins_cache_entry; - coins_cache_entry.flags = fuzzed_data_provider.ConsumeIntegral<unsigned char>(); + const auto flags{fuzzed_data_provider.ConsumeIntegral<uint8_t>()}; if (fuzzed_data_provider.ConsumeBool()) { coins_cache_entry.coin = random_coin; } else { @@ -136,11 +139,14 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) } coins_cache_entry.coin = *opt_coin; } - coins_map.emplace(random_out_point, std::move(coins_cache_entry)); + auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first}; + it->second.AddFlags(flags, *it, sentinel); + usage += it->second.coin.DynamicMemoryUsage(); } bool expected_code_path = false; try { - coins_view_cache.BatchWrite(coins_map, fuzzed_data_provider.ConsumeBool() ? ConsumeUInt256(fuzzed_data_provider) : coins_view_cache.GetBestBlock()); + auto cursor{CoinsViewCacheCursor(usage, sentinel, coins_map, /*will_erase=*/true)}; + coins_view_cache.BatchWrite(cursor, fuzzed_data_provider.ConsumeBool() ? ConsumeUInt256(fuzzed_data_provider) : coins_view_cache.GetBestBlock()); expected_code_path = true; } catch (const std::logic_error& e) { if (e.what() == std::string{"FRESH flag misapplied to coin that exists in parent cache"}) { diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index 648e96b4a0..8e717e96b4 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -172,13 +172,13 @@ public: std::unique_ptr<CCoinsViewCursor> Cursor() const final { return {}; } size_t EstimateSize() const final { return m_data.size(); } - bool BatchWrite(CCoinsMap& data, const uint256&, bool erase) final + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256&) final { - for (auto it = data.begin(); it != data.end(); it = erase ? data.erase(it) : std::next(it)) { - if (it->second.flags & CCoinsCacheEntry::DIRTY) { + for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { + if (it->second.IsDirty()) { if (it->second.coin.IsSpent() && (it->first.n % 5) != 4) { m_data.erase(it->first); - } else if (erase) { + } else if (cursor.WillErase(*it)) { m_data[it->first] = std::move(it->second.coin); } else { m_data[it->first] = it->second.coin; diff --git a/src/test/fuzz/prevector.cpp b/src/test/fuzz/prevector.cpp index 9cea32e304..aeceb38a58 100644 --- a/src/test/fuzz/prevector.cpp +++ b/src/test/fuzz/prevector.cpp @@ -2,16 +2,14 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include <test/fuzz/FuzzedDataProvider.h> -#include <test/fuzz/fuzz.h> - #include <prevector.h> -#include <vector> - -#include <reverse_iterator.h> #include <serialize.h> #include <streams.h> +#include <test/fuzz/FuzzedDataProvider.h> +#include <test/fuzz/fuzz.h> +#include <ranges> +#include <vector> namespace { template <unsigned int N, typename T> @@ -47,7 +45,7 @@ public: assert(v == real_vector[pos]); ++pos; } - for (const T& v : reverse_iterate(pre_vector)) { + for (const T& v : pre_vector | std::views::reverse) { --pos; assert(v == real_vector[pos]); } @@ -55,7 +53,7 @@ public: assert(v == real_vector[pos]); ++pos; } - for (const T& v : reverse_iterate(const_pre_vector)) { + for (const T& v : const_pre_vector | std::views::reverse) { --pos; assert(v == real_vector[pos]); } diff --git a/src/test/fuzz/script.cpp b/src/test/fuzz/script.cpp index a23543d70e..07a49e039f 100644 --- a/src/test/fuzz/script.cpp +++ b/src/test/fuzz/script.cpp @@ -76,13 +76,11 @@ FUZZ_TARGET(script, .init = initialize_script) assert(which_type == TxoutType::PUBKEY || which_type == TxoutType::NONSTANDARD || which_type == TxoutType::NULL_DATA || - which_type == TxoutType::MULTISIG || - which_type == TxoutType::ANCHOR); + which_type == TxoutType::MULTISIG); } if (which_type == TxoutType::NONSTANDARD || which_type == TxoutType::NULL_DATA || - which_type == TxoutType::MULTISIG || - which_type == TxoutType::ANCHOR) { + which_type == TxoutType::MULTISIG) { assert(!extract_destination_ret); } diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp index d82f166765..1d90414443 100644 --- a/src/test/fuzz/utxo_snapshot.cpp +++ b/src/test/fuzz/utxo_snapshot.cpp @@ -57,7 +57,7 @@ FUZZ_TARGET(utxo_snapshot, .init = initialize_chain) int base_blockheight{fuzzed_data_provider.ConsumeIntegralInRange<int>(1, 2 * COINBASE_MATURITY)}; uint256 base_blockhash{g_chain->at(base_blockheight - 1)->GetHash()}; uint64_t m_coins_count{fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(1, 3 * COINBASE_MATURITY)}; - SnapshotMetadata metadata{msg_start, base_blockhash, base_blockheight, m_coins_count}; + SnapshotMetadata metadata{msg_start, base_blockhash, m_coins_count}; outfile << metadata; } // Coins diff --git a/src/test/prevector_tests.cpp b/src/test/prevector_tests.cpp index 1ac7abf492..f5f0cbee58 100644 --- a/src/test/prevector_tests.cpp +++ b/src/test/prevector_tests.cpp @@ -3,17 +3,16 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <prevector.h> -#include <vector> - -#include <reverse_iterator.h> #include <serialize.h> #include <streams.h> - #include <test/util/random.h> #include <test/util/setup_common.h> #include <boost/test/unit_test.hpp> +#include <ranges> +#include <vector> + BOOST_FIXTURE_TEST_SUITE(prevector_tests, TestingSetup) template<unsigned int N, typename T> @@ -58,14 +57,14 @@ class prevector_tester { for (const T& v : pre_vector) { local_check(v == real_vector[pos++]); } - for (const T& v : reverse_iterate(pre_vector)) { - local_check(v == real_vector[--pos]); + for (const T& v : pre_vector | std::views::reverse) { + local_check(v == real_vector[--pos]); } for (const T& v : const_pre_vector) { local_check(v == real_vector[pos++]); } - for (const T& v : reverse_iterate(const_pre_vector)) { - local_check(v == real_vector[--pos]); + for (const T& v : const_pre_vector | std::views::reverse) { + local_check(v == real_vector[--pos]); } DataStream ss1{}; DataStream ss2{}; diff --git a/src/txdb.cpp b/src/txdb.cpp index e4a4b3bf72..3865692b6a 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -88,7 +88,7 @@ std::vector<uint256> CCoinsViewDB::GetHeadBlocks() const { return vhashHeadBlocks; } -bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { +bool CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { CDBBatch batch(*m_db); size_t count = 0; size_t changed = 0; @@ -114,8 +114,8 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo batch.Erase(DB_BEST_BLOCK); batch.Write(DB_HEAD_BLOCKS, Vector(hashBlock, old_tip)); - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end();) { - if (it->second.flags & CCoinsCacheEntry::DIRTY) { + for (auto it{cursor.Begin()}; it != cursor.End();) { + if (it->second.IsDirty()) { CoinEntry entry(&it->first); if (it->second.coin.IsSpent()) batch.Erase(entry); @@ -124,7 +124,7 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo changed++; } count++; - it = erase ? mapCoins.erase(it) : std::next(it); + it = cursor.NextAndMaybeErase(*it); if (batch.SizeEstimate() > m_options.batch_write_bytes) { LogPrint(BCLog::COINDB, "Writing partial batch of %.2f MiB\n", batch.SizeEstimate() * (1.0 / 1048576.0)); m_db->WriteBatch(batch); diff --git a/src/txdb.h b/src/txdb.h index c9af0a091e..e0acb09e98 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -63,7 +63,7 @@ public: bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; std::vector<uint256> GetHeadBlocks() const override; - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override; + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override; std::unique_ptr<CCoinsViewCursor> Cursor() const override; //! Whether an unsupported database format is used. diff --git a/src/txmempool.cpp b/src/txmempool.cpp index f56da08e5f..b523c5fe09 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -15,7 +15,6 @@ #include <policy/policy.h> #include <policy/settings.h> #include <random.h> -#include <reverse_iterator.h> #include <tinyformat.h> #include <util/check.h> #include <util/feefrac.h> @@ -31,6 +30,7 @@ #include <cmath> #include <numeric> #include <optional> +#include <ranges> #include <string_view> #include <utility> @@ -121,7 +121,7 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256>& vHashes // This maximizes the benefit of the descendant cache and guarantees that // CTxMemPoolEntry::m_children will be updated, an assumption made in // UpdateForDescendants. - for (const uint256 &hash : reverse_iterate(vHashesToUpdate)) { + for (const uint256& hash : vHashesToUpdate | std::views::reverse) { // calculate children from mapNextTx txiter it = mapTx.find(hash); if (it == mapTx.end()) { diff --git a/src/validation.cpp b/src/validation.cpp index f7250778fc..25da81ae8a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -40,7 +40,6 @@ #include <primitives/block.h> #include <primitives/transaction.h> #include <random.h> -#include <reverse_iterator.h> #include <script/script.h> #include <script/sigcache.h> #include <signet.h> @@ -70,6 +69,7 @@ #include <deque> #include <numeric> #include <optional> +#include <ranges> #include <string> #include <tuple> #include <utility> @@ -107,6 +107,21 @@ const std::vector<std::string> CHECKLEVEL_DOC { * */ static constexpr int PRUNE_LOCK_BUFFER{10}; +/** + * Maximum number of seconds that the timestamp of the first + * block of a difficulty adjustment period is allowed to + * be earlier than the last block of the previous period (BIP94). + */ +static constexpr int64_t MAX_TIMEWARP{MAX_FUTURE_BLOCK_TIME}; + +/** + * If the timestamp of the last block in a difficulty period is set + * MAX_FUTURE_BLOCK_TIME seconds in the future, an honest miner should + * be able to mine the first block using the current time. This is not + * a consensus rule, but changing MAX_TIMEWARP should be done with caution. + */ +static_assert(MAX_FUTURE_BLOCK_TIME <= MAX_TIMEWARP); + GlobalMutex g_best_block_mutex; std::condition_variable g_best_block_cv; uint256 g_best_block; @@ -2902,7 +2917,7 @@ bool Chainstate::FlushStateToDisk( return FatalError(m_chainman.GetNotifications(), state, _("Disk space is too low!")); } // Flush the chainstate (which may refer to block index entries). - const auto empty_cache{(mode == FlushStateMode::ALWAYS) || fCacheLarge || fCacheCritical || fFlushForPrune}; + const auto empty_cache{(mode == FlushStateMode::ALWAYS) || fCacheLarge || fCacheCritical}; if (empty_cache ? !CoinsTip().Flush() : !CoinsTip().Sync()) { return FatalError(m_chainman.GetNotifications(), state, _("Failed to write to coin database.")); } @@ -3357,7 +3372,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex* nHeight = nTargetHeight; // Connect new blocks. - for (CBlockIndex* pindexConnect : reverse_iterate(vpindexToConnect)) { + for (CBlockIndex* pindexConnect : vpindexToConnect | std::views::reverse) { if (!ConnectTip(state, pindexConnect, pindexConnect == pindexMostWork ? pblock : std::shared_ptr<const CBlock>(), connectTrace, disconnectpool)) { if (state.IsInvalid()) { // The block violates a consensus rule. @@ -4188,7 +4203,7 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio // Check timestamp for the first block of each difficulty adjustment // interval, except the genesis block. if (nHeight % consensusParams.DifficultyAdjustmentInterval() == 0) { - if (block.GetBlockTime() < pindexPrev->GetBlockTime() - 60 * 60 * 2) { + if (block.GetBlockTime() < pindexPrev->GetBlockTime() - MAX_TIMEWARP) { return state.Invalid(BlockValidationResult::BLOCK_INVALID_HEADER, "time-timewarp-attack", "block's timestamp is too early on diff adjustment block"); } } @@ -5384,7 +5399,7 @@ void ChainstateManager::CheckBlockIndex() // and the transactions were not pruned (pindexFirstMissing // is null), it is a potential candidate. The check // excludes pruned blocks, because if any blocks were - // pruned between pindex the current chain tip, pindex will + // pruned between pindex and the current chain tip, pindex will // only temporarily be added to setBlockIndexCandidates, // before being moved to m_blocks_unlinked. This check // could be improved to verify that if all blocks between @@ -5657,31 +5672,31 @@ Chainstate& ChainstateManager::InitializeChainstate(CTxMemPool* mempool) return destroyed && !fs::exists(db_path); } -util::Result<void> ChainstateManager::ActivateSnapshot( +util::Result<CBlockIndex*> ChainstateManager::ActivateSnapshot( AutoFile& coins_file, const SnapshotMetadata& metadata, bool in_memory) { uint256 base_blockhash = metadata.m_base_blockhash; - int base_blockheight = metadata.m_base_blockheight; if (this->SnapshotBlockhash()) { return util::Error{Untranslated("Can't activate a snapshot-based chainstate more than once")}; } + CBlockIndex* snapshot_start_block{}; + { LOCK(::cs_main); if (!GetParams().AssumeutxoForBlockhash(base_blockhash).has_value()) { auto available_heights = GetParams().GetAvailableSnapshotHeights(); std::string heights_formatted = util::Join(available_heights, ", ", [&](const auto& i) { return util::ToString(i); }); - return util::Error{strprintf(Untranslated("assumeutxo block hash in snapshot metadata not recognized (hash: %s, height: %s). The following snapshot heights are available: %s"), + return util::Error{strprintf(Untranslated("assumeutxo block hash in snapshot metadata not recognized (hash: %s). The following snapshot heights are available: %s"), base_blockhash.ToString(), - base_blockheight, heights_formatted)}; } - CBlockIndex* snapshot_start_block = m_blockman.LookupBlockIndex(base_blockhash); + snapshot_start_block = m_blockman.LookupBlockIndex(base_blockhash); if (!snapshot_start_block) { return util::Error{strprintf(Untranslated("The base block header (%s) must appear in the headers chain. Make sure all headers are syncing, and call loadtxoutset again"), base_blockhash.ToString())}; @@ -5692,7 +5707,7 @@ util::Result<void> ChainstateManager::ActivateSnapshot( return util::Error{strprintf(Untranslated("The base block header (%s) is part of an invalid chain"), base_blockhash.ToString())}; } - if (!m_best_header || m_best_header->GetAncestor(base_blockheight) != snapshot_start_block) { + if (!m_best_header || m_best_header->GetAncestor(snapshot_start_block->nHeight) != snapshot_start_block) { return util::Error{Untranslated("A forked headers-chain with more work than the chain with the snapshot base block header exists. Please proceed to sync without AssumeUtxo.")}; } @@ -5806,7 +5821,7 @@ util::Result<void> ChainstateManager::ActivateSnapshot( m_snapshot_chainstate->CoinsTip().DynamicMemoryUsage() / (1000 * 1000)); this->MaybeRebalanceCaches(); - return {}; + return snapshot_start_block; } static void FlushSnapshotToDisk(CCoinsViewCache& coins_cache, bool snapshot_loaded) diff --git a/src/validation.h b/src/validation.h index eb43892b1a..f905d6e624 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1094,11 +1094,10 @@ public: //! - Verify that the hash of the resulting coinsdb matches the expected hash //! per assumeutxo chain parameters. //! - Wait for our headers chain to include the base block of the snapshot. - //! - "Fast forward" the tip of the new chainstate to the base of the snapshot, - //! faking nTx* block index data along the way. + //! - "Fast forward" the tip of the new chainstate to the base of the snapshot. //! - Move the new chainstate to `m_snapshot_chainstate` and make it our //! ChainstateActive(). - [[nodiscard]] util::Result<void> ActivateSnapshot( + [[nodiscard]] util::Result<CBlockIndex*> ActivateSnapshot( AutoFile& coins_file, const node::SnapshotMetadata& metadata, bool in_memory); //! Once the background validation chainstate has reached the height which diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 5b2fed7158..838d062108 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -529,7 +529,7 @@ RPCHelpMan getaddressinfo() RPCResult::Type::OBJ, "", "", { {RPCResult::Type::STR, "address", "The bitcoin address validated."}, - {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded scriptPubKey generated by the address."}, + {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded output script generated by the address."}, {RPCResult::Type::BOOL, "ismine", "If the address is yours."}, {RPCResult::Type::BOOL, "iswatchonly", "If the address is watchonly."}, {RPCResult::Type::BOOL, "solvable", "If we know how to spend coins sent to this address, ignoring the possible lack of private keys."}, diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 2cf94a5722..f1430a3c60 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -530,19 +530,19 @@ RPCHelpMan listunspent() {RPCResult::Type::NUM, "vout", "the vout value"}, {RPCResult::Type::STR, "address", /*optional=*/true, "the bitcoin address"}, {RPCResult::Type::STR, "label", /*optional=*/true, "The associated label, or \"\" for the default label"}, - {RPCResult::Type::STR, "scriptPubKey", "the script key"}, + {RPCResult::Type::STR, "scriptPubKey", "the output script"}, {RPCResult::Type::STR_AMOUNT, "amount", "the transaction output amount in " + CURRENCY_UNIT}, {RPCResult::Type::NUM, "confirmations", "The number of confirmations"}, {RPCResult::Type::NUM, "ancestorcount", /*optional=*/true, "The number of in-mempool ancestor transactions, including this one (if transaction is in the mempool)"}, {RPCResult::Type::NUM, "ancestorsize", /*optional=*/true, "The virtual transaction size of in-mempool ancestors, including this one (if transaction is in the mempool)"}, {RPCResult::Type::STR_AMOUNT, "ancestorfees", /*optional=*/true, "The total fees of in-mempool ancestors (including this one) with fee deltas used for mining priority in " + CURRENCY_ATOM + " (if transaction is in the mempool)"}, - {RPCResult::Type::STR_HEX, "redeemScript", /*optional=*/true, "The redeemScript if scriptPubKey is P2SH"}, - {RPCResult::Type::STR, "witnessScript", /*optional=*/true, "witnessScript if the scriptPubKey is P2WSH or P2SH-P2WSH"}, + {RPCResult::Type::STR_HEX, "redeemScript", /*optional=*/true, "The redeem script if the output script is P2SH"}, + {RPCResult::Type::STR, "witnessScript", /*optional=*/true, "witness script if the output script is P2WSH or P2SH-P2WSH"}, {RPCResult::Type::BOOL, "spendable", "Whether we have the private keys to spend this output"}, {RPCResult::Type::BOOL, "solvable", "Whether we know how to spend this output, ignoring the lack of keys"}, {RPCResult::Type::BOOL, "reused", /*optional=*/true, "(only present if avoid_reuse is set) Whether this output is reused/dirty (sent to an address that was previously spent from)"}, {RPCResult::Type::STR, "desc", /*optional=*/true, "(only when solvable) A descriptor for spending this output"}, - {RPCResult::Type::ARR, "parent_descs", /*optional=*/false, "List of parent descriptors for the scriptPubKey of this coin.", { + {RPCResult::Type::ARR, "parent_descs", /*optional=*/false, "List of parent descriptors for the output script of this coin.", { {RPCResult::Type::STR, "desc", "The descriptor string."}, }}, {RPCResult::Type::BOOL, "safe", "Whether this output is considered safe to spend. Unconfirmed transactions\n" diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index e0bb24ca70..601d8e4fa7 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -882,7 +882,7 @@ RPCHelpMan signrawtransactionwithwallet() { {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"}, {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output number"}, - {"scriptPubKey", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "script key"}, + {"scriptPubKey", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The output script"}, {"redeemScript", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "(required for P2SH) redeem script"}, {"witnessScript", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "(required for P2WSH or P2SH-P2WSH) witness script"}, {"amount", RPCArg::Type::AMOUNT, RPCArg::Optional::OMITTED, "(required for Segwit inputs) the amount spent"}, diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index 0dacfa808b..61cf36a6c1 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -431,7 +431,7 @@ static std::vector<RPCResult> TransactionDescriptionString() {RPCResult::Type::STR, "comment", /*optional=*/true, "If a comment is associated with the transaction, only present if not empty."}, {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no|unknown\") Whether this transaction signals BIP125 replaceability or has an unconfirmed ancestor signaling BIP125 replaceability.\n" "May be unknown for unconfirmed transactions not in the mempool because their unconfirmed ancestors are unknown."}, - {RPCResult::Type::ARR, "parent_descs", /*optional=*/true, "Only if 'category' is 'received'. List of parent descriptors for the scriptPubKey of this coin.", { + {RPCResult::Type::ARR, "parent_descs", /*optional=*/true, "Only if 'category' is 'received'. List of parent descriptors for the output script of this coin.", { {RPCResult::Type::STR, "desc", "The descriptor string."}, }}, }; @@ -729,7 +729,7 @@ RPCHelpMan gettransaction() {RPCResult::Type::STR_AMOUNT, "fee", /*optional=*/true, "The amount of the fee in " + CURRENCY_UNIT + ". This is negative and only available for the \n" "'send' category of transactions."}, {RPCResult::Type::BOOL, "abandoned", "'true' if the transaction has been abandoned (inputs are respendable)."}, - {RPCResult::Type::ARR, "parent_descs", /*optional=*/true, "Only if 'category' is 'received'. List of parent descriptors for the scriptPubKey of this coin.", { + {RPCResult::Type::ARR, "parent_descs", /*optional=*/true, "Only if 'category' is 'received'. List of parent descriptors for the output script of this coin.", { {RPCResult::Type::STR, "desc", "The descriptor string."}, }}, }}, diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 8c218ad766..ae1c67ef2a 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -68,7 +68,7 @@ static RPCHelpMan getwalletinfo() {RPCResult::Type::NUM, "duration", "elapsed seconds since scan start"}, {RPCResult::Type::NUM, "progress", "scanning progress percentage [0.0, 1.0]"}, }, /*skip_type_check=*/true}, - {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for scriptPubKey management"}, + {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for output script management"}, {RPCResult::Type::BOOL, "external_signer", "whether this wallet is configured to use an external signer such as a hardware wallet"}, {RPCResult::Type::BOOL, "blank", "Whether this wallet intentionally does not contain any keys, scripts, or descriptors"}, {RPCResult::Type::NUM_TIME, "birthtime", /*optional=*/true, "The start time for blocks scanning. It could be modified by (re)importing any descriptor with an earlier timestamp."}, diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py index 0acd4244a5..fd6a9518e8 100755 --- a/test/functional/feature_assumeutxo.py +++ b/test/functional/feature_assumeutxo.py @@ -71,7 +71,7 @@ class AssumeutxoTest(BitcoinTestFramework): assert_raises_rpc_error(parsing_error_code, "Unable to parse metadata: Invalid UTXO set snapshot magic bytes. Please check if this is indeed a snapshot file or if you are using an outdated snapshot format.", node.loadtxoutset, bad_snapshot_path) self.log.info(" - snapshot file with unsupported version") - for version in [0, 2]: + for version in [0, 1, 3]: with open(bad_snapshot_path, 'wb') as f: f.write(valid_snapshot_contents[:5] + version.to_bytes(2, "little") + valid_snapshot_contents[7:]) assert_raises_rpc_error(parsing_error_code, f"Unable to parse metadata: Version of snapshot {version} does not match any of the supported versions.", node.loadtxoutset, bad_snapshot_path) @@ -96,23 +96,20 @@ class AssumeutxoTest(BitcoinTestFramework): self.log.info(" - snapshot file referring to a block that is not in the assumeutxo parameters") prev_block_hash = self.nodes[0].getblockhash(SNAPSHOT_BASE_HEIGHT - 1) bogus_block_hash = "0" * 64 # Represents any unknown block hash - # The height is not used for anything critical currently, so we just - # confirm the manipulation in the error message - bogus_height = 1337 for bad_block_hash in [bogus_block_hash, prev_block_hash]: with open(bad_snapshot_path, 'wb') as f: - f.write(valid_snapshot_contents[:11] + bogus_height.to_bytes(4, "little") + bytes.fromhex(bad_block_hash)[::-1] + valid_snapshot_contents[47:]) + f.write(valid_snapshot_contents[:11] + bytes.fromhex(bad_block_hash)[::-1] + valid_snapshot_contents[43:]) - msg = f"Unable to load UTXO snapshot: assumeutxo block hash in snapshot metadata not recognized (hash: {bad_block_hash}, height: {bogus_height}). The following snapshot heights are available: 110, 200, 299." + msg = f"Unable to load UTXO snapshot: assumeutxo block hash in snapshot metadata not recognized (hash: {bad_block_hash}). The following snapshot heights are available: 110, 200, 299." assert_raises_rpc_error(-32603, msg, node.loadtxoutset, bad_snapshot_path) self.log.info(" - snapshot file with wrong number of coins") - valid_num_coins = int.from_bytes(valid_snapshot_contents[47:47 + 8], "little") + valid_num_coins = int.from_bytes(valid_snapshot_contents[43:43 + 8], "little") for off in [-1, +1]: with open(bad_snapshot_path, 'wb') as f: - f.write(valid_snapshot_contents[:47]) + f.write(valid_snapshot_contents[:43]) f.write((valid_num_coins + off).to_bytes(8, "little")) - f.write(valid_snapshot_contents[47 + 8:]) + f.write(valid_snapshot_contents[43 + 8:]) expected_error(msg="Bad snapshot - coins left over after deserializing 298 coins." if off == -1 else "Bad snapshot format or truncated snapshot after deserializing 299 coins.") self.log.info(" - snapshot file with alternated but parsable UTXO data results in different hash") @@ -130,10 +127,10 @@ class AssumeutxoTest(BitcoinTestFramework): for content, offset, wrong_hash, custom_message in cases: with open(bad_snapshot_path, "wb") as f: - # Prior to offset: Snapshot magic, snapshot version, network magic, height, hash, coins count - f.write(valid_snapshot_contents[:(5 + 2 + 4 + 4 + 32 + 8 + offset)]) + # Prior to offset: Snapshot magic, snapshot version, network magic, hash, coins count + f.write(valid_snapshot_contents[:(5 + 2 + 4 + 32 + 8 + offset)]) f.write(content) - f.write(valid_snapshot_contents[(5 + 2 + 4 + 4 + 32 + 8 + offset + len(content)):]) + f.write(valid_snapshot_contents[(5 + 2 + 4 + 32 + 8 + offset + len(content)):]) msg = custom_message if custom_message is not None else f"Bad snapshot content hash: expected a4bf3407ccb2cc0145c49ebba8fa91199f8a3903daf0883875941497d2493c27, got {wrong_hash}." expected_error(msg) diff --git a/test/functional/rpc_dumptxoutset.py b/test/functional/rpc_dumptxoutset.py index 0b7c468846..aa12da6ceb 100755 --- a/test/functional/rpc_dumptxoutset.py +++ b/test/functional/rpc_dumptxoutset.py @@ -43,7 +43,7 @@ class DumptxoutsetTest(BitcoinTestFramework): # UTXO snapshot hash should be deterministic based on mocked time. assert_equal( sha256sum_file(str(expected_path)).hex(), - '2f775f82811150d310527b5ff773f81fb0fb517e941c543c1f7c4d38fd2717b3') + '31fcdd0cf542a4b1dfc13c3c05106620ce48951ef62907dd8e5e8c15a0aa993b') assert_equal( out['txoutset_hash'], 'a0b7baa3bf5ccbd3279728f230d7ca0c44a76e9923fca8f32dbfd08d65ea496a') diff --git a/test/lint/lint-python-mutable-default-parameters.py b/test/lint/lint-python-mutable-default-parameters.py deleted file mode 100755 index 820595ea34..0000000000 --- a/test/lint/lint-python-mutable-default-parameters.py +++ /dev/null @@ -1,72 +0,0 @@ -#!/usr/bin/env python3 -# -# Copyright (c) 2019-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. - -""" -Detect when a mutable list or dict is used as a default parameter value in a Python function. -""" - -import subprocess -import sys - - -def main(): - command = [ - "git", - "grep", - "-E", - r"^\s*def [a-zA-Z0-9_]+\(.*=\s*(\[|\{)", - "--", - "*.py", - ] - output = subprocess.run(command, stdout=subprocess.PIPE, text=True) - if len(output.stdout) > 0: - error_msg = ( - "A mutable list or dict seems to be used as default parameter value:\n\n" - f"{output.stdout}\n" - f"{example()}" - ) - print(error_msg) - sys.exit(1) - else: - sys.exit(0) - - -def example(): - return """This is how mutable list and dict default parameter values behave: - ->>> def f(i, j=[], k={}): -... j.append(i) -... k[i] = True -... return j, k -... ->>> f(1) -([1], {1: True}) ->>> f(1) -([1, 1], {1: True}) ->>> f(2) -([1, 1, 2], {1: True, 2: True}) - -The intended behaviour was likely: - ->>> def f(i, j=None, k=None): -... if j is None: -... j = [] -... if k is None: -... k = {} -... j.append(i) -... k[i] = True -... return j, k -... ->>> f(1) -([1], {1: True}) ->>> f(1) -([1], {1: True}) ->>> f(2) -([2], {2: True})""" - - -if __name__ == "__main__": - main() diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index 2ba58a6da2..77388b78ff 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -36,6 +36,11 @@ fn get_linter_list() -> Vec<&'static Linter> { lint_fn: lint_markdown }, &Linter { + description: "Check the default arguments in python", + name: "py_mut_arg_default", + lint_fn: lint_py_mut_arg_default, + }, + &Linter { description: "Check that std::filesystem is not used directly", name: "std_filesystem", lint_fn: lint_std_filesystem @@ -180,6 +185,35 @@ fn lint_subtree() -> LintResult { } } +fn lint_py_mut_arg_default() -> LintResult { + let bin_name = "ruff"; + let checks = ["B006", "B008"] + .iter() + .map(|c| format!("--select={}", c)) + .collect::<Vec<_>>(); + let files = check_output( + git() + .args(["ls-files", "--", "*.py"]) + .args(get_pathspecs_exclude_subtrees()), + )?; + + let mut cmd = Command::new(bin_name); + cmd.arg("check").args(checks).args(files.lines()); + + match cmd.status() { + Ok(status) if status.success() => Ok(()), + Ok(_) => Err(format!("`{}` found errors!", bin_name)), + Err(e) if e.kind() == ErrorKind::NotFound => { + println!( + "`{}` was not found in $PATH, skipping those checks.", + bin_name + ); + Ok(()) + } + Err(e) => Err(format!("Error running `{}`: {}", bin_name, e)), + } +} + fn lint_std_filesystem() -> LintResult { let found = git() .args([ |