From ee1e40f58000921e95f08bcb199a452eb5c4d9b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sat, 3 Jan 2026 20:26:04 +0100 Subject: [PATCH 1/6] txdb: assert `CCoinsViewDB::GetCoin` only returns unspent coins The chainstate UTXO database only stores unspent outputs; spent entries are removed. Assert after reading a `Coin` so corruption or misuse cannot propagate a spent coin through the `GetCoin()` interface. --- src/txdb.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/txdb.cpp b/src/txdb.cpp index 5d61388b0781..141c31c5d514 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -67,7 +67,10 @@ void CCoinsViewDB::ResizeCache(size_t new_cache_size) std::optional CCoinsViewDB::GetCoin(const COutPoint& outpoint) const { - if (Coin coin; m_db->Read(CoinEntry(&outpoint), coin)) return coin; + if (Coin coin; m_db->Read(CoinEntry(&outpoint), coin)) { + Assert(!coin.IsSpent()); // The UTXO database should never contain spent coins + return coin; + } return std::nullopt; } From 3e4155fcefe0aafcc9cb84640e303e05477605a3 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sat, 3 Jan 2026 20:26:33 +0100 Subject: [PATCH 2/6] test: do not return spent coins from `CCoinsViewTest::GetCoin` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Production `GetCoin()` implementations only return unspent coins. Update the `CCoinsView` test backend to match that contract, so tests stop exercising cache states that cannot occur with `CCoinsViewCache` or `CCoinsViewDB`. Co-authored-by: Lőrinc --- src/test/coins_tests.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 6396fce60ac3..63024054fa6c 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -48,11 +48,7 @@ class CCoinsViewTest : public CCoinsView std::optional GetCoin(const COutPoint& outpoint) const override { - if (auto it{map_.find(outpoint)}; it != map_.end()) { - if (!it->second.IsSpent() || m_rng.randbool()) { - return it->second; // TODO spent coins shouldn't be returned - } - } + if (auto it{map_.find(outpoint)}; it != map_.end() && !it->second.IsSpent()) return it->second; return std::nullopt; } From eec551aaf1dff4cccc15e486d5618a8a44d8314c Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sat, 3 Jan 2026 20:28:38 +0100 Subject: [PATCH 3/6] fuzz: keep `coinscache_sim` backend free of spent coins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CoinsViewBottom` roughly simulates a memory-backed `CCoinsViewDB`, which never stores spent coins. Stop returning spent coins from `GetCoin()`, erase spent entries in `BatchWrite()`, and tighten comparisons to expect `std::nullopt` when the simulator has no coin. Co-authored-by: Lőrinc --- src/test/fuzz/coinscache_sim.cpp | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index f57c25210e3c..c6102c2a65ce 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -138,8 +138,6 @@ struct CacheLevel /** Class for the base of the hierarchy (roughly simulating a memory-backed CCoinsViewDB). * * The initial state consists of the empty UTXO set. - * Coins whose output index is 4 (mod 5) have GetCoin() always succeed after being spent. - * This exercises code paths with spent, non-DIRTY cache entries. */ class CoinsViewBottom final : public CCoinsView { @@ -148,16 +146,13 @@ class CoinsViewBottom final : public CCoinsView public: std::optional GetCoin(const COutPoint& outpoint) const final { - // TODO GetCoin shouldn't return spent coins - if (auto it = m_data.find(outpoint); it != m_data.end()) return it->second; + if (auto it{m_data.find(outpoint)}; it != m_data.end()) { + assert(!it->second.IsSpent()); + return it->second; + } return std::nullopt; } - bool HaveCoin(const COutPoint& outpoint) const final - { - return m_data.contains(outpoint); - } - uint256 GetBestBlock() const final { return {}; } std::vector GetHeadBlocks() const final { return {}; } std::unique_ptr Cursor() const final { return {}; } @@ -167,18 +162,20 @@ class CoinsViewBottom final : public CCoinsView { 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) { + if (it->second.coin.IsSpent()) { m_data.erase(it->first); - } else if (cursor.WillErase(*it)) { - m_data[it->first] = std::move(it->second.coin); } else { - m_data[it->first] = it->second.coin; + if (cursor.WillErase(*it)) { + m_data[it->first] = std::move(it->second.coin); + } else { + m_data[it->first] = it->second.coin; + } } } else { /* For non-dirty entries being written, compare them with what we have. */ auto it2 = m_data.find(it->first); if (it->second.coin.IsSpent()) { - assert(it2 == m_data.end() || it2->second.IsSpent()); + assert(it2 == m_data.end()); } else { assert(it2 != m_data.end()); assert(it->second.coin.out == it2->second.out); @@ -263,7 +260,7 @@ FUZZ_TARGET(coinscache_sim) auto realcoin = caches.back()->GetCoin(data.outpoints[outpointidx]); // Compare results. if (!sim.has_value()) { - assert(!realcoin || realcoin->IsSpent()); + assert(!realcoin); } else { assert(realcoin && !realcoin->IsSpent()); const auto& simcoin = data.coins[sim->first]; @@ -449,7 +446,7 @@ FUZZ_TARGET(coinscache_sim) auto realcoin = bottom.GetCoin(data.outpoints[outpointidx]); auto sim = lookup(outpointidx, 0); if (!sim.has_value()) { - assert(!realcoin || realcoin->IsSpent()); + assert(!realcoin); } else { assert(realcoin && !realcoin->IsSpent()); assert(realcoin->out == data.coins[sim->first].out); From 2ee7f9b259059d59e127852ea898b58183604b46 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sat, 3 Jan 2026 20:28:55 +0100 Subject: [PATCH 4/6] coins: assume `GetCoin` only returns unspent coins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CCoinsViewCache::FetchCoin()` had special handling for a spent `Coin` returned by the parent view. Production parents (`CCoinsViewCache` and `CCoinsViewDB`) do not return spent coins, so this path is unreachable. Replace it with an `Assume(!coin.IsSpent())`, drop outdated documentation about spent+FRESH cache entries, and simplify `SanityCheck()` to assert the remaining possible state invariants. This is safe because it does not change behavior for valid backends and will fail fast if the `GetCoin()` contract is violated. Co-authored-by: Lőrinc --- src/coins.cpp | 26 +++++++++++--------------- src/coins.h | 7 ------- 2 files changed, 11 insertions(+), 22 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 7f2ffc38efa0..ccd6d605ba6e 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -55,10 +55,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const if (auto coin{base->GetCoin(outpoint)}) { ret->second.coin = std::move(*coin); cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage(); - if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins - // The parent only has an empty entry for this outpoint; we can consider our version as fresh. - CCoinsCacheEntry::SetFresh(*ret, m_sentinel); - } + Assert(!ret->second.coin.IsSpent()); } else { cacheCoins.erase(ret); return cacheCoins.end(); @@ -277,7 +274,7 @@ void CCoinsViewCache::Sync() void CCoinsViewCache::Uncache(const COutPoint& hash) { CCoinsMap::iterator it = cacheCoins.find(hash); - if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) { + if (it != cacheCoins.end() && !it->second.IsDirty()) { cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); TRACEPOINT(utxocache, uncache, hash.hash.data(), @@ -318,20 +315,19 @@ void CCoinsViewCache::ReallocateCache() void CCoinsViewCache::SanityCheck() const { size_t recomputed_usage = 0; - size_t count_flagged = 0; + size_t count_dirty = 0; for (const auto& [_, entry] : cacheCoins) { - unsigned attr = 0; - 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); + if (entry.coin.IsSpent()) { + assert(entry.IsDirty() && !entry.IsFresh()); // A spent coin must be dirty and cannot be fresh + } else { + assert(entry.IsDirty() || !entry.IsFresh()); // An unspent coin must not be fresh if not dirty + } // 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; + if (entry.IsDirty()) ++count_dirty; } // Iterate over the linked list of flagged entries. size_t count_linked = 0; @@ -340,11 +336,11 @@ void CCoinsViewCache::SanityCheck() const 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()); + assert(it->second.IsDirty()); // Count the number of entries actually in the list. ++count_linked; } - assert(count_linked == count_flagged); + assert(count_linked == count_dirty); assert(recomputed_usage == cachedCoinsUsage); } diff --git a/src/coins.h b/src/coins.h index 6da53829996d..66d712be34da 100644 --- a/src/coins.h +++ b/src/coins.h @@ -102,7 +102,6 @@ using CoinsCachePair = std::pair; * - unspent, FRESH, DIRTY (e.g. a new coin created in the cache) * - unspent, not FRESH, DIRTY (e.g. a coin changed in the cache during a reorg) * - unspent, not FRESH, not DIRTY (e.g. an unspent coin fetched from the parent cache) - * - spent, FRESH, not DIRTY (e.g. a spent coin fetched from the parent cache) * - spent, not FRESH, DIRTY (e.g. a coin is spent and spentness needs to be flushed to the parent) */ struct CCoinsCacheEntry @@ -117,12 +116,6 @@ struct CCoinsCacheEntry * 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}; From 1f60ca360eb83fa7982b1aac402eaaf477294197 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 21 Jan 2026 08:20:44 +0700 Subject: [PATCH 5/6] wallet: fix removeprunedfunds bug with conflicting transactions removeprunedfunds removes all entries from mapTxSpends for the inputs of the pruned tx. However, this is incorrect, because there could be multiple entries from conflicting transactions (that shouldn't be removed as well). This could lead to the wallet creating invalid transactions, trying to double spend utxos. The bug persists when the conflicting tx was mined, because the wallet trusts its internal accounting instead of calling AddToSpends again. --- src/wallet/wallet.cpp | 11 ++++++-- test/functional/wallet_importprunedfunds.py | 28 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index abbb6c1267d0..a24fc908a128 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2398,8 +2398,15 @@ util::Result CWallet::RemoveTxs(WalletBatch& batch, std::vector& txs for (const auto& it : erased_txs) { const Txid hash{it->first}; wtxOrdered.erase(it->second.m_it_wtxOrdered); - for (const auto& txin : it->second.tx->vin) - mapTxSpends.erase(txin.prevout); + for (const auto& txin : it->second.tx->vin) { + auto range = mapTxSpends.equal_range(txin.prevout); + for (auto iter = range.first; iter != range.second; ++iter) { + if (iter->second == hash) { + mapTxSpends.erase(iter); + break; + } + } + } for (unsigned int i = 0; i < it->second.tx->vout.size(); ++i) { m_txos.erase(COutPoint(hash, i)); } diff --git a/test/functional/wallet_importprunedfunds.py b/test/functional/wallet_importprunedfunds.py index 72896052b5f5..1948cb9278b0 100755 --- a/test/functional/wallet_importprunedfunds.py +++ b/test/functional/wallet_importprunedfunds.py @@ -14,6 +14,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, + assert_not_equal, assert_raises_rpc_error, wallet_importprivkey, ) @@ -129,6 +130,33 @@ def run_test(self): mb.header.nTime += 1 # modify arbitrary block header field to change block hash assert_raises_rpc_error(-5, "Block not found in chain", w1.importprunedfunds, rawtxn1, mb.serialize().hex()) + self.log.info("Test removeprunedfunds with conflicting transactions") + node = self.nodes[0] + + # Create a transaction + utxo = node.listunspent()[0] + addr = node.getnewaddress() + tx1_id = node.send(outputs=[{addr: 1}], inputs=[utxo])["txid"] + tx1_fee = node.gettransaction(tx1_id)["fee"] + + # Create a conflicting tx with a larger fee (tx1_fee is negative) + output_value = utxo["amount"] + tx1_fee - Decimal("0.00001") + raw_tx2 = node.createrawtransaction(inputs=[utxo], outputs=[{addr: output_value}]) + signed_tx2 = node.signrawtransactionwithwallet(raw_tx2) + tx2_id = node.sendrawtransaction(signed_tx2["hex"]) + assert_not_equal(tx2_id, tx1_id) + + # Both txs should be in the wallet, tx2 replaced tx1 in mempool + assert tx1_id in [tx["txid"] for tx in node.listtransactions()] + assert tx2_id in [tx["txid"] for tx in node.listtransactions()] + + # Remove the replaced tx from wallet + node.removeprunedfunds(tx1_id) + + # The UTXO should still be considered spent (by tx2) + available_utxos = [u["txid"] for u in node.listunspent(minconf=0)] + assert utxo["txid"] not in available_utxos, "UTXO should still be spent by conflicting tx" + if __name__ == '__main__': ImportPrunedFundsTest(__file__).main() From e770392084aa52e5568cf001da4d537fda1d71b3 Mon Sep 17 00:00:00 2001 From: Bruno Garcia Date: Wed, 14 Jan 2026 22:10:18 -0300 Subject: [PATCH 6/6] test: addrman: test self-announcement time penalty handling Verify that addresses announcing themselves (addr == source) are exempt from time penalties, while addresses announced by others receive the expected penalty. --- src/test/addrman_tests.cpp | 40 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 4debdc16a33b..49a9e2022505 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -17,6 +17,7 @@ #include +#include #include #include @@ -106,6 +107,45 @@ BOOST_AUTO_TEST_CASE(addrman_simple) BOOST_CHECK(addrman->Size() >= 1); } +BOOST_AUTO_TEST_CASE(addrman_penalty_self_announcement) +{ + SetMockTime(Now()); + auto addrman = std::make_unique(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node)); + + const auto base_time{Now() - 10000s}; + CService addr1 = ResolveService("250.1.1.1", 8333); + CNetAddr source1 = ResolveIP("250.1.1.1"); // Same as addr1 - self announcement + + CAddress caddr1(addr1, NODE_NONE); + caddr1.nTime = base_time; + + const auto time_penalty{3600s}; + + BOOST_CHECK(addrman->Add({caddr1}, source1, time_penalty)); + + auto addr_pos1{addrman->FindAddressEntry(caddr1)}; + BOOST_REQUIRE(addr_pos1.has_value()); + + std::vector addresses{addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt)}; + BOOST_REQUIRE_EQUAL(addresses.size(), 1U); + + BOOST_CHECK(addresses[0].nTime == base_time); + + CService addr2{ResolveService("250.1.1.2", 8333)}; + CNetAddr source2{ResolveIP("250.1.1.3")}; // Different from addr2 - not self announcement + + CAddress caddr2(addr2, NODE_NONE); + caddr2.nTime = base_time; + + BOOST_CHECK(addrman->Add({caddr2}, source2, time_penalty)); + + addresses = addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt); + BOOST_REQUIRE_EQUAL(addresses.size(), 2U); + + CAddress retrieved_addr2{addresses[0]}; + BOOST_CHECK(retrieved_addr2.nTime == base_time - time_penalty); +} + BOOST_AUTO_TEST_CASE(addrman_ports) { auto addrman = std::make_unique(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node));