coins: remove SetFresh method from CCoinsCacheEntry

andrewtoth commented at 7:21 pm on July 19, 2025: contributor

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 added the label UTXO Db and Indexes on Jul 19, 2025

DrahtBot commented at 7:21 pm on July 19, 2025: contributor

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

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32313 (coins: fix cachedCoinsUsage accounting in CCoinsViewCache 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.

andrewtoth force-pushed on Jul 20, 2025

in src/test/fuzz/coinscache_sim.cpp:155 in 8eb06adaaa outdated

152+            assert(!it->second.IsSpent());
153+            return it->second;
154+        }
155         return std::nullopt;
156     }
157

l0rinc commented at 7:01 pm on July 20, 2025:

We should be able to remove HaveCoin now, since

0bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
1{
2    return GetCoin(outpoint).has_value();
3}

is more accurate (especially with the new extra assert)

andrewtoth commented at 1:29 am on July 21, 2025:

Done.

l0rinc commented at 1:37 am on July 21, 2025:

Isn’t that the same as the parent in CCoinsView?

andrewtoth commented at 2:21 am on July 21, 2025:

Ah right, removed.

in src/coins.cpp:54 in 8eb06adaaa outdated

50@@ -51,10 +51,6 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const
51         if (auto coin{base->GetCoin(outpoint)}) {
52             ret->second.coin = std::move(*coin);
53             cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
54-            if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins

l0rinc commented at 7:01 pm on July 20, 2025:

Now that we’ve removed it we should at least add an Assume(!ret->second.coin.IsSpent()); here - or even an assert, since if we’re not sure about this we shouldn’t do it in the first place

andrewtoth commented at 1:29 am on July 21, 2025:

Done.

in src/coins.h:183 in 8eb06adaaa outdated

176@@ -188,14 +177,14 @@ struct CCoinsCacheEntry
177     bool IsDirty() const noexcept { return m_flags & DIRTY; }
178     bool IsFresh() const noexcept { return m_flags & FRESH; }
179 
180-    //! Only call Next when this entry is DIRTY, FRESH, or both
181+    //! Only call Next when this entry is DIRTY or DIRTY-and-FRESH
182     CoinsCachePair* Next() const noexcept
183     {
184         Assume(m_flags);

l0rinc commented at 7:02 pm on July 20, 2025:

I understand if you think it may modify too many things here, but I’d prefer an actual Assume instead of the comment:

0    CoinsCachePair* Next() const noexcept
1    {
2        Assume(IsDirty());
3        return m_next;
4    }

(same for Prev)

andrewtoth commented at 1:30 am on July 21, 2025:

I don’t see how that is different. m_flags being set implies IsDirty() now, so it is equivalent. I prefer not to change this.

l0rinc commented at 1:34 am on July 21, 2025:

Assume(m_flags); would also be true for fresh only - Assume(IsDirty()); is stricter

andrewtoth commented at 1:47 am on July 21, 2025:

It’s not possible to set fresh only now.

l0rinc commented at 1:51 am on July 21, 2025:

Exactly, hence my recommendation

in src/coins.h:134 in 8eb06adaaa outdated

120@@ -121,21 +121,6 @@ struct CCoinsCacheEntry
121     CoinsCachePair* m_next{nullptr};
122     uint8_t m_flags{0};
123 
124-    //! Adding a flag requires a reference to the sentinel of the flagged pair linked list.
125-    static void AddFlags(uint8_t flags, CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept
126-    {
127-        Assume(flags & (DIRTY | FRESH));

l0rinc commented at 7:03 pm on July 20, 2025:

we might want to mention in the commit message why we can safely remove this now

andrewtoth commented at 2:04 am on July 21, 2025:

I don’t see what can be explained more than the code? This seems obvious to me.

l0rinc commented at 2:05 am on July 21, 2025:

Ok, agree, please resolve the comment

in src/coins.h:166 in 8eb06adaaa outdated

162+            sentinel.second.m_prev = &pair;
163+            pair.second.m_prev->second.m_next = &pair;
164+        }
165+        Assume(pair.second.m_prev && pair.second.m_next);
166+        pair.second.m_flags |= DIRTY;
167+        if (fresh) pair.second.m_flags |= FRESH;

l0rinc commented at 7:04 pm on July 20, 2025:

What should happen when fresh is false? Shouldn’t we assert something like:

0if (fresh) {
1    pair.second.m_flags |= FRESH; 
2} else {
3    assert(!(pair.second.m_flags & FRESH));
4}

andrewtoth commented at 2:22 am on July 21, 2025:

I suppose its legal to call SetDirty on a coin that is already DIRTY-and-FRESH. I’m not sure we need to do anything here in that case.

l0rinc commented at 2:24 am on July 21, 2025:

Should we remove its freshness in that case?

andrewtoth commented at 2:29 am on July 21, 2025:

No, that would make this a behavior change rather than a refactor. We only added flags before, so we must only do that now as well.

l0rinc commented at 2:34 am on July 21, 2025:

But we have prohibited one of the states here, we can document that with code now instead of comments. If we think it’s a behavior change the PR is incorrect, isn’t it?

andrewtoth commented at 2:45 am on July 21, 2025:

Your suggestion is to assert that if fresh is false then the entry cannot already be fresh. This is not the case. An entry can be made DIRTY-and-FRESH, and then it can be made DIRTY again while already DIRTY-and-FRESH. The current behavior allows this to happen and keeps the entry DIRTY-and-FRESH when SetDirty is called. If we assert that we can’t call SetDirty on a DIRTY-and-FRESH entry then it is a behavior change.

l0rinc commented at 2:47 am on July 21, 2025:

Nah, I’m just on a phone and thought I’m commenting on SetDirty and SetFresh.

in src/test/coinscachepair_tests.cpp:159 in 8eb06adaaa outdated

153@@ -154,16 +154,16 @@ BOOST_AUTO_TEST_CASE(linked_list_set_state)
154     BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1);
155     BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1);
156 
157-    // Check that setting FRESH on new node inserts it after n1
158-    CCoinsCacheEntry::SetFresh(n2, sentinel);
159-    BOOST_CHECK(n2.second.IsFresh() && !n2.second.IsDirty());
160+    // Check that setting DIRTY and FRESH on new node inserts it after n1
161+    CCoinsCacheEntry::SetDirty(n2, sentinel, /*fresh=*/true);
162+    BOOST_CHECK(n2.second.IsFresh() && n2.second.IsDirty());

l0rinc commented at 7:06 pm on July 20, 2025:

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

0    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.

in src/test/fuzz/coins_view.cpp:153 in 8eb06adaaa outdated

149@@ -150,8 +150,7 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
150                         coins_cache_entry.coin = *opt_coin;
151                     }
152                     auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first};
153-                    if (dirty) CCoinsCacheEntry::SetDirty(*it, sentinel);
154-                    if (fresh) CCoinsCacheEntry::SetFresh(*it, sentinel);
155+                    if (dirty) CCoinsCacheEntry::SetDirty(*it, sentinel, fresh);

l0rinc commented at 7:07 pm on July 20, 2025:

definition of fresh should likely include dirty now:

0const auto fresh{dirty && fuzzed_data_provider.ConsumeBool()};

andrewtoth commented at 1:59 am on July 21, 2025:

Done.

in src/coins.h:113 in 8eb06adaaa outdated

111     /**
112      * These are used to create a doubly linked list of flagged entries.
113-     * They are set in SetDirty, SetFresh, and unset in SetClean.
114-     * A flagged entry is any entry that is either DIRTY, FRESH, or both.
115+     * They are set in SetDirty and unset in SetClean.
116+     * A flagged entry is any entry that is DIRTY or DIRTY and FRESH.

l0rinc commented at 7:08 pm on July 20, 2025:

0     * A flagged entry is any entry that is DIRTY or DIRTY-and-FRESH.

(the or/and combination is confusing otherwise)

andrewtoth commented at 1:56 am on July 21, 2025:

Done.

in src/test/fuzz/coinscache_sim.cpp:149 in 8eb06adaaa outdated

145@@ -148,8 +146,10 @@ class CoinsViewBottom final : public CCoinsView
146 public:
147     std::optional<Coin> GetCoin(const COutPoint& outpoint) const final
148     {
149-        // TODO GetCoin shouldn't return spent coins
150-        if (auto it = m_data.find(outpoint); it != m_data.end()) return it->second;
151+        if (auto it = m_data.find(outpoint); it != m_data.end()) {

l0rinc commented at 7:10 pm on July 20, 2025:

nit: for consistency with other overrides:

0        if (auto it{m_data.find(outpoint)}; it != m_data.end()) {

andrewtoth commented at 1:57 am on July 21, 2025:

Done.

in src/coins.cpp:320 in 8eb06adaaa outdated

314@@ -321,8 +315,8 @@ void CCoinsViewCache::SanityCheck() const
315         if (entry.IsDirty()) attr |= 1;
316         if (entry.IsFresh()) attr |= 2;
317         if (entry.coin.IsSpent()) attr |= 4;
318-        // Only 5 combinations are possible.
319-        assert(attr != 2 && attr != 4 && attr != 7);
320+        // Only 4 combinations are possible.
321+        assert(attr != 2 && attr != 4 && attr != 6 && attr != 7);

l0rinc commented at 7:17 pm on July 20, 2025:

great, now that we have this, can we call SanityCheck in more tests? I understood that we couldn’t do it until now since we still had invalid states in fuzz/unit tests

andrewtoth commented at 1:57 am on July 21, 2025:

We can’t since we are still calling this with spent, dirty, and fresh entries. These would be deleted in production code. That can also be tackled in a follow-up.

l0rinc commented at 1:59 am on July 21, 2025:

You did enable it for a few additional cases, that’s already a win 👍

andrewtoth commented at 2:01 am on July 21, 2025:

Err, I did but didn’t test locally before pushing. I will have to revert. Tests are failing.

l0rinc commented at 2:04 am on July 21, 2025:

Can we fix those cases instead? Having a sanitycheck which isn’t run just gives us a false sense of security otherwise

andrewtoth commented at 2:34 am on July 21, 2025:

I can make a follow-up to remove spent fresh and dirty entries from the test cases as well. I don’t think that should happen in this PR though to keep it focused. The previous PR also did this.

l0rinc commented at 7:20 pm on July 20, 2025: contributor

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.

in src/coins.cpp:206 in 8eb06adaaa outdated

197@@ -203,11 +198,10 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
198                     entry.coin = it->second.coin;
199                 }
200                 cachedCoinsUsage += entry.coin.DynamicMemoryUsage();
201-                CCoinsCacheEntry::SetDirty(*itUs, m_sentinel);

l0rinc commented at 1:11 am on July 21, 2025:

note: the cursor contains dirty entries now, so if (!it->second.IsDirty()) { on line 186 is also a noop - could we add a TODO there to signal that we’re aware of it but want to tackle it in a follow-up?

andrewtoth commented at 1:57 am on July 21, 2025:

There are quite a few places where we can clean up production and test logic to not have to deal with non-dirty entries in BatchWrite. I think there would be too many TODOs.

andrewtoth force-pushed on Jul 21, 2025

Lalaweazel commented at 2:06 am on July 21, 2025: none

’ DQ at need f cc 2iooo9 to be in work iiiiiiii iroieeieeieowwiereo W e

On Mon, 21 July 2025, 12:02 pm Andrew Toth, @.***> wrote:

@.**** commented on this pull request.

In src/coins.h https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218067867:

spent, not FRESH, DIRTY (e.g. a coin is spent and spentness needs to be flushed to the parent) / struct CCoinsCacheEntry { private: /* * These are used to create a doubly linked list of flagged entries.
* They are set in SetDirty, SetFresh, and unset in SetClean.
* A flagged entry is any entry that is either DIRTY, FRESH, or both.
* They are set in SetDirty and unset in SetClean.
* A flagged entry is any entry that is DIRTY or DIRTY and FRESH.
Done.

— Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218067867, or unsubscribe https://github.com/notifications/unsubscribe-auth/BLCZ66TZ63CTQEG3FDJLXXD3JRC23AVCNFSM6AAAAACB46C4HCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTAMZWGMZDKOBZGM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

test: coins returned from GetCoin cannot be spent

This is already not possible for any production GetCoin
implementation, so this aligns tests with production code.

9786869a2f

coins: remove SetFresh in FetchCoin

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.

3ea6410467

test: remove FRESH-but-not-DIRTY state from coins_tests

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.

c325ed3644

test: assert no FRESH-but-not-DIRTY coins can occur 7645a75043

coins: remove SetFresh method from CCoinsCacheEntry

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.

fdf8f070ee

coins: inline CCoinsViewCache::AddFlags into SetDirty

There is only one callsite now for AddFlags, so we can inline
it into SetDirty.

99b243d53e

andrewtoth force-pushed on Jul 21, 2025

DrahtBot added the label CI failed on Jul 21, 2025

DrahtBot commented at 2:21 am on July 21, 2025: 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.

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.

DrahtBot removed the label CI failed on Jul 21, 2025

coins: remove SetFresh method from CCoinsCacheEntry #33018

Code Coverage & Benchmarks

Reviews

Conflicts

@.**** commented on this pull request.