Skip to content

Conversation

@andrewtoth
Copy link
Contributor

@andrewtoth andrewtoth commented Jul 19, 2025

There is plenty of test code that exercises the code path for FRESH-but-not-DIRTY coins in CCoinsViewCache. However, we can see there is only one place in production code where we call SetFresh that is not preceded by SetDirty. This is in CCoinsViewCache::FetchCoin and we can see that this is called if the coin retrieved from base->GetCoin is spent. The base in this case can be another CCoinsViewCache or a CCoinsViewDB. In CCoinsViewCache::GetCoin we can see that we do not return spent coins, and in CCoinsViewDB we don't ever store spent coins so there are none to return.

Thus, we can safely remove this dead code, and now we can see that there are never any calls to SetFresh not preceded by SetDirty. This PR removes this dead code and replaces SetFresh by passing a fresh boolean flag to SetDirty. This simplifies the logic of CCoinsViewCache and lets us remove test cases checking for FRESH-but-not-DIRTY coins. It removes the possibility of FRESH-but-not-DIRTY coins being called from any code path, so we can eliminate this state entirely and reduce the cognitive load of having to consider it.

This is pulled out from #30673 to make the change simpler.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 19, 2025

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33018.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK l0rinc, sedited

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #34207 (coins/refactor: enforce GetCoin() returns only unspent coins by l0rinc)
  • #33512 (coins: use number of dirty cache entries in flush warnings/logs by l0rinc)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Contributor

@l0rinc l0rinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

Very glad this is getting cleanup up, I found it worrying that a lot of tests were exercising invalid states in the first place - not the best foundation.
Quickly went over the changes, I find the small commits and excellent commit messages easy to follow.
I'll run a full IBD over this (maybe with some extra SanityCheck sprinkled around for extra measure) to make sure real usage doesn't trigger any invalid states we're not aware of.

BOOST_CHECK(n2.second.IsFresh() && !n2.second.IsDirty());
// Check that setting DIRTY and FRESH on new node inserts it after n1
CCoinsCacheEntry::SetDirty(n2, sentinel, /*fresh=*/true);
BOOST_CHECK(n2.second.IsFresh() && n2.second.IsDirty());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for consistency with other SetDirty calls we could swap the parameter order:

Suggested change
BOOST_CHECK(n2.second.IsFresh() && n2.second.IsDirty());
BOOST_CHECK(n2.second.IsDirty() && n2.second.IsFresh());

Alternatively we could rename IsFresh to IsDirtyAndFresh and assert that if fresh is true, so is dirty - which would simplify this line to asserting just a single method.

@Lalaweazel

This comment was marked as spam.

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Task CentOS, depends, gui: https://github.com/bitcoin/bitcoin/runs/46353382966
LLM reason (✨ experimental): The CI failure is caused by an assertion failure in coins.cpp during the coins_tests, leading to subprocess abortion.

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@andrewtoth
Copy link
Contributor Author

@sedited @l0rinc thank you both for your reviews! I rebased and updated with your suggestions.

This is already not possible for any production GetCoin
implementation, so this aligns tests with production code.
This is the only place in production code where SetFresh is called
when not preceded by SetDirty.
We can see that coin is retrieved from base->GetCoin, and base can
only be another CCoinsViewCache or CCoinsViewDB.
CCoinsViewCache::GetCoin does not return spent coins, and CCoinsViewDB
does not store any spent coins to return.
Thus, we can conclude that this call to SetFresh is dead code and can
be safely removed.
The FRESH-but-not-DIRTY state cannot occur in production code.
There are never any calls to SetFresh not preceded by SetDirty.
We should remove these test cases to simplify the test code and
better align it with production code.
@andrewtoth
Copy link
Contributor Author

Rebased due to #34095 modifying code that is deleted in this change.


static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(DIRTY, pair, sentinel); }
static void SetFresh(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(FRESH, pair, sentinel); }
static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel, bool fresh = false) noexcept
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important to add a docstring to this function. Would also be good to explain what happens if a non-fresh entry is marked fresh by accident (and vice-versa), similar to the previous brief explainer for the FRESH enum variant (don't have a good suggestion yet though).

Copy link
Contributor

@l0rinc l0rinc Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on a PR to reduce the doubly-linked list to a single one - it will likely change these methods anyway.
If I finish it in time, maybe I can provide feedback about that here...
But let's also step back and notice that we're trying to add a comment for a method that we concede is hard to understand. Can we refactor instead of admitting defeat and commenting?
I will review this in detail to see if I have a better idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a docstring to this function and to SetClean().

There are no instances where SetFresh is called without being
preceded by SetDirty. This replaces SetFresh with a boolean flag
passed to SetDirty to indicate whether the DIRTY coin should also
be set to FRESH. This removes the possibility of setting a
FRESH-but-not-DIRTY cached coin state.
There is only one callsite now for AddFlags, so we can inline
it into SetDirty.
A cache entry is now always DIRTY if it is in the linked list.
We can use linked list membership instead of a flag to determine
dirtiness.
Thus, the only state we need to track is if an entry is FRESH.
This can be represented by a simple bool instead of a bitfield.
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 4, 2026

🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/actions/runs/20696479226/job/59412536506
LLM reason (✨ experimental): Lint failure due to trailing whitespace detected in src/coins.h (trailing_whitespace check).

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Copy link
Contributor

@l0rinc l0rinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed and extracted a subset of the changes that aren't strictly related to freshness to #34207

Left a few comments about leftover commit messages and comments or symbols still referencing the old flags

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e9d793d: commit message has a typo.
882e942: AddFlags is in CCoinsCacheEntry

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c15e0bc
this commit fixes two TODOs, we could push it as a separate PR!
We can even merge it with the sanity check commit - which should go here, since this commit enabled it.

Comment on lines +321 to +326
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);
// Only 4 combinations are possible.
assert(attr != 2 && attr != 4 && attr != 6 && attr != 7);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's getting simpler to list what we do support now, doesn't it?

unsigned attr = 0;
if (entry.IsDirty()) attr |= 1;
if (entry.IsFresh()) attr |= 2;
if (entry.coin.IsSpent()) attr |= 4;
assert(attr == 0 /*unspent*/ ||
       attr == 1 /*unspent+DIRTY*/ ||
       attr == 3 /*unspent+DIRTY+FRESH*/ ||
       attr == 5 /*spent+DIRTY*/);

So basically assert not spent, but if it is, it should be dirty. If not spent, it's not dirty, but if it is, it should be fresh as well, right?

Suggested change
assert(attr != 2 && attr != 4 && attr != 6 && attr != 7);
assert(!entry.coin.IsSpent() || entry.IsDirty());
assert(!entry.IsFresh() || (entry.IsDirty() && !entry.coin.IsSpent()));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a commit from the previous PR bcf4ec0.

void CCoinsViewCache::SanityCheck() const
{
size_t recomputed_usage = 0;
size_t count_flagged = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're not flags anymore, we should update a few other places:

diff --git a/src/coins.cpp b/src/coins.cpp
index dac9540b1d..39ad74daa6 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -264,8 +264,8 @@ void CCoinsViewCache::Sync()
     auto cursor{CoinsViewCacheCursor(m_sentinel, cacheCoins, /*will_erase=*/false)};
     base->BatchWrite(cursor, hashBlock);
     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");
+        /* BatchWrite must clear dirty state of all entries */
+        throw std::logic_error("Not all unspent dirty entries were cleared");
     }
 }
 
@@ -313,7 +313,7 @@ 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;
@@ -328,18 +328,18 @@ void CCoinsViewCache::SanityCheck() const
         // 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.
+    // Iterate over the linked list of dirty 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.
+        // Verify they are actually dirty.
         assert(it->second.IsDirty() || it->second.IsFresh());
         // 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 bc291f1b68..99efa70eec 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -252,11 +252,11 @@ private:
 };
 
 /**
- * Cursor for iterating over the linked list of flagged entries in CCoinsViewCache.
+ * Cursor for iterating over the linked list of dirty 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
+ * of CCoinsView::BatchWrite to iterate through the dirty entries without knowing
  * the caller's intent.
  *
  * However, the receiver can still call CoinsViewCacheCursor::WillErase to see if the
@@ -267,7 +267,7 @@ private:
 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).
+    //! and other coins will be unmarked dirty (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.
@@ -371,7 +371,7 @@ protected:
      */
     mutable uint256 hashBlock;
     mutable CCoinsMapMemoryResource m_cache_coins_memory_resource{};
-    /* The starting sentinel of the flagged entry circular doubly linked list. */
+    /* The starting sentinel of the dirty entry circular doubly linked list. */
     mutable CoinsCachePair m_sentinel;
     mutable CCoinsMap cacheCoins;
 

// The parent only has an empty entry for this outpoint; we can consider our version as fresh.
CCoinsCacheEntry::SetFresh(*ret, m_sentinel);
}
Assume(!ret->second.coin.IsSpent());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few leftover spentness checks:

diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp
--- a/src/test/fuzz/coinscache_sim.cpp	(revision e34fdb04462b88efdaa116c099c1aeed1022cdb5)
+++ b/src/test/fuzz/coinscache_sim.cpp	(date 1767696554322)
@@ -173,7 +173,7 @@
                 /* 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);
@@ -258,7 +258,7 @@
                 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];
@@ -444,7 +444,7 @@
         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);
Index: src/coins.cpp
diff --git a/src/coins.cpp b/src/coins.cpp
--- a/src/coins.cpp	(revision e34fdb04462b88efdaa116c099c1aeed1022cdb5)
+++ b/src/coins.cpp	(date 1767696597975)
@@ -272,7 +272,7 @@
 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(),
@@ -326,7 +326,7 @@
         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_flagged;
     }
     // Iterate over the linked list of dirty entries.
     size_t count_linked = 0;
@@ -335,7 +335,7 @@
         assert(it->second.Next()->second.Prev() == it);
         assert(it->second.Prev()->second.Next() == it);
         // Verify they are actually dirty.
-        assert(it->second.IsDirty() || it->second.IsFresh());
+        assert(it->second.IsDirty());
         // Count the number of entries actually in the list.
         ++count_linked;
     }

@l0rinc l0rinc mentioned this pull request Jan 14, 2026
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants