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}; 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)); 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; } 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); 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; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index de260f8e49d7..b648159845fb 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2413,8 +2413,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()