coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries #30673

andrewtoth · 2024-08-18T21:31:11Z

Following up from #28280 (comment), which suggested a revival of #18746.

GetCoin will never return true for a spent entry, so we can safely assume that any entry we fetch will not be spent. This lets us remove the only non-test code path which adds a FRESH-but-not-DIRTY entry to the flagged linked list. This in turn ensures all entries being sent to BatchWrite are DIRTY entries.

A corollary is that all spent coins must be DIRTY. The only time a coin can be spent and not DIRTY is when the CCoinsViewCacheEntry is created with an empty coin. The last commit makes this more clear by checking for freshness if inserted instead of if spent and not DIRTY.

This is a pure refactor removing dead code which handles a non-existent corner case of a FRESH-but-not-DIRTY entry in the CCoinsViewCache or a spent entry in CCoinsViewDB. There is a lot of test code which tries to exercise this corner case, which is also updated in this PR to behave like non-test code.

DrahtBot · 2024-08-18T21:31:13Z

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/30673.

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.

DrahtBot · 2024-08-18T21:39:33Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/28917867890}

Hints

Make sure to run all tests locally, according to the documentation.

The failure may 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.

l0rinc

The code became a lot simpler now - have a few suggestion I'd like us to consider, but I understand if you think it's out of scope.

Once we get to a state that is ACK-worthy, I'll enable hard assertions everywhere and run an IBD and check if it crashes anywhere.

src/coins.cpp

src/test/fuzz/coinscache_sim.cpp

src/coins.h

src/test/coins_tests.cpp

src/coins.cpp

andrewtoth · 2024-08-20T18:36:46Z

Once we get to a state that is ACK-worthy, I'll enable hard assertions everywhere and run an IBD and check if it crashes anywhere.

I think for a change like this, spending time fuzzing would be more valuable. I believe it already runs in debug mode for that so Assumes would trigger crashes.

l0rinc

Dug deeper and added more relevant questions - bear with me if they're outside the scope of this PR.

src/coins.h

src/test/fuzz/coinscache_sim.cpp

src/test/coins_tests.cpp

src/coins.cpp

l0rinc

The unreliability of many of our tests worries me, would you be open to fixing them in a PR before this one?

src/coins.h

l0rinc · 2024-09-01T14:23:11Z

src/coins.cpp

-        // we would remove it from this cache and would never flush spentness
-        // to the parent cache.
+        // A spent FRESH coin cannot exist in the cache because a FRESH coin
+        // is simply erased when it is spent.
        //
        // Re-adding a spent coin can happen in the case of a re-org (the coin


can this still happen during a reorg? The comment here is really scary, do we really need so much context here? I'd prefer a test that fails when the code is wrong instead of a really long description...

Yes, it can happen during a reorg. But we know we can't have a spent coin that is not DIRTY, so the comments being updated here are making this not be redundant. When the coin is spent in DisconnectBlock, it is either erased if FRESH or set DIRTY. The only time we would get into this block if the coin is not DIRTY is if it was just inserted.

src/coins.cpp

l0rinc · 2024-09-01T14:51:11Z

src/coins.cpp

-        // DIRTY, then it can be marked FRESH.
-        fresh = !it->second.IsDirty();
+        // If the coin doesn't exist in the current cache then it can be marked FRESH.
+        fresh = inserted;


The freshness flag depend on inserted, possible_overwrite and it->second.coin.IsSpent(), assigned and validated in different places.
Could we obviate the freshness flag calculation instead with something like:

bool fresh_flag = inserted && !possible_overwrite ? CCoinsCacheEntry::FRESH : 0;

Either here or in a follow-up we could simplify & modernize the method to something like:

void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possible_overwrite) { assert(!coin.IsSpent()); if (coin.out.scriptPubKey.IsUnspendable()) return; auto [it, inserted] = cacheCoins.try_emplace(outpoint); if (!inserted) { if (!possible_overwrite && !it->second.coin.IsSpent()) throw std::logic_error("Attempted to overwrite an unspent coin (when possible_overwrite is false)"); cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); } cachedCoinsUsage += coin.DynamicMemoryUsage(); it->second.coin = std::move(coin); bool fresh_flag = inserted && !possible_overwrite ? CCoinsCacheEntry::FRESH : 0; it->second.AddFlags(CCoinsCacheEntry::DIRTY | fresh_flag, *it, m_sentinel); TRACE5(utxocache, add, outpoint.hash.data(), (uint32_t) outpoint.n, (uint32_t) it->second.coin.nHeight, (int64_t) it->second.coin.out.nValue, (bool) it->second.coin.IsCoinBase()); }

Let's leave this for a follow-up.

src/coins.cpp

l0rinc · 2024-09-01T15:30:41Z

src/test/coins_tests.cpp

@@ -793,7 +787,7 @@ BOOST_AUTO_TEST_CASE(ccoins_add)
     */


unrelated: could coins_cache_simulation_testbe moved into a fuzz test - or would it be too difficult to track all those states?.
If you think any of these recommendations can be done in a parallel PR, let me know and I'll do them myself before this is merged

I'm not sure. It might just be redundant to our current fuzz harness. It's also valuable to have these tests run on every CI.

src/test/coins_tests.cpp

src/coins.cpp

src/test/fuzz/coinscache_sim.cpp

src/coins.cpp

src/test/coins_tests.cpp

src/coins.cpp

src/coins.h

src/coins.cpp

l0rinc · 2024-12-13T14:46:26Z

src/coins.cpp

@@ -311,12 +311,13 @@ void CCoinsViewCache::SanityCheck() const
    size_t recomputed_usage = 0;
    size_t count_flagged = 0;
    for (const auto& [_, entry] : cacheCoins) {


It seems to me this can also be merged with the related SanityCheck commit

I merged it with aaef2d6 instead.

src/test/fuzz/coins_view.cpp

l0rinc · 2025-07-06T08:43:36Z

This is still important - @andrewtoth, how can I help?

DrahtBot · 2025-07-09T01:22:50Z

🚧 At least one of the CI tasks failed.
_{Task TSan, depends, gui: https://github.com/bitcoin/bitcoin/runs/45603902258}
_{LLM reason (✨ experimental): The CI failure is caused by a syntax error in coinscache_sim.cpp, specifically an expected '}' which leads to undeclared identifier errors during compilation.}

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.

There are no code paths which add a non-DIRTY entry to the cursor in BatchWrite, so we can safely make this a logic error and test for it. There are no code paths which add a spent and FRESH coin to the cursor in BatchWrite, so we can safely make this a logic error and test for it.

It is no longer possible to get non-DIRTY entries in BatchWrite

It is not possible for an entry to be FRESH if not already DIRTY.

A spent coins must be DIRTY, so remove references to spent but not DIRTY coins. The only way a spent coin can be not DIRTY is when creating the CCoinsCacheEntry with an empty coin. This can be made more clear by setting fresh if inserted, instead of checking if an unspent coin is not DIRTY.

andrewtoth · 2025-07-09T13:37:41Z

@l0rinc rebased. Thank you for your reviews! I have tried to address all your comments. There are some older comments that I'm unsure are still relevant. Please let me know if there is still anything outstanding that needs to be addressed.

It is not possible to have a FRESH CCoinsCacheEntry that is not also DIRTY. Test code uses the SetFresh method out of order to simulate entries that are FRESH but not DIRTY. By removing the method entirely we can simplify the test code and make the production code easier to understand.

l0rinc

The src/coins.cpp prod changes are a bit scary - while it's a very important change, based on the lack of enthusiasm by others, we may want to separate the tests and small refactors into PRs that will get us closer to this.

One could remove the invalid test states - without removing the invalid production code states, just potentially adding TODOs there in case it's missing.
The SetFresh and flag removal could be next and if all of those are merged, we can continue with the scary FetchCoin, AddCoin, BatchWrite and Uncache.

Left a few new comments quickly, let me know what you think.

l0rinc · 2025-07-14T23:25:44Z

src/test/fuzz/coinscache_sim.cpp

@@ -148,14 +146,13 @@ class CoinsViewBottom final : public CCoinsView
 public:
    std::optional<Coin> GetCoin(const COutPoint& outpoint) const final


nit: the class is already final + other overrides specify that explicitly for clarity

Suggested change

std::optional<Coin> GetCoin(const COutPoint& outpoint) const final

std::optional<Coin> GetCoin(const COutPoint& outpoint) const override

and

bool HaveCoin(const COutPoint& outpoint) const override

And most other such methods in the file - but I understand if you'd prefer to do in a follow-up instead

l0rinc · 2025-07-15T01:27:36Z

src/coins.h

+    static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel, bool fresh = false) noexcept
+    {
+        AddFlags(fresh ? FRESH | DIRTY : DIRTY, pair, sentinel);
+    }


Since it's only called here, we could inline and simplify it now:

Suggested change

static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel, bool fresh = false) noexcept

{

AddFlags(fresh ? FRESH | DIRTY : DIRTY, pair, sentinel);

}

static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel, bool fresh = false) noexcept

{

if (!pair.second.m_flags) {

Assume(!pair.second.m_prev && !pair.second.m_next);

pair.second.m_prev = sentinel.second.m_prev;

pair.second.m_next = &sentinel;

sentinel.second.m_prev = &pair;

pair.second.m_prev->second.m_next = &pair;

}

Assume(pair.second.m_prev && pair.second.m_next);

pair.second.m_flags |= fresh ? FRESH | DIRTY : DIRTY;

}

Not sure, but do the flags still make sense after this or would a bool is_fresh also suffice? The dirtiness can be deduced from the m_next/m_prev pair so the only unknown is the freshness which doesn't necessitate a flag anymore - right?

l0rinc · 2025-07-15T01:29:52Z

src/coins.h

@@ -188,17 +183,15 @@ struct CCoinsCacheEntry
    bool IsDirty() const noexcept { return m_flags & DIRTY; }
    bool IsFresh() const noexcept { return m_flags & FRESH; }


we don't have Fresh anymore, only IsDirtyAndFresh, right?

Suggested change

bool IsFresh() const noexcept { return m_flags & FRESH; }

bool IsDirtyAndFresh() const noexcept

{

const bool is_fresh = m_flags & FRESH;

Assume(IsDirty() || !is_fresh);

return is_fresh;

}

l0rinc · 2025-07-15T01:37:19Z

src/test/coins_tests.cpp

-                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;


Given the lack of reviews, we may want to split out the test cleanups (removing invalid states) and refactors to a separate PR, where this will be a tracking PR for where we want to get to in safer/smaller steps

l0rinc · 2025-07-15T01:38:25Z

src/test/coinscachepair_tests.cpp

-    CCoinsCacheEntry::SetFresh(n2, sentinel);
-    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, true);


Suggested change

CCoinsCacheEntry::SetDirty(n2, sentinel, true);

CCoinsCacheEntry::SetDirty(n2, sentinel, /*fresh=*/true);

l0rinc · 2025-07-15T01:39:22Z

src/test/fuzz/coinscache_sim.cpp

        return std::nullopt;
    }

    bool HaveCoin(const COutPoint& outpoint) const final
    {
-        return m_data.count(outpoint);
+        return GetCoin(outpoint).has_value();


we should be able to do this safely in a prefactor PR regardless of the other changes

andrewtoth · 2025-07-19T19:22:10Z

Closing in favor of #33018.

DrahtBot added the UTXO Db and Indexes label Aug 18, 2024

andrewtoth force-pushed the no-spent-and-fresh branch from 3f032cf to b393b0d Compare August 18, 2024 21:39

DrahtBot added the CI failed label Aug 18, 2024

andrewtoth force-pushed the no-spent-and-fresh branch from b393b0d to feaea74 Compare August 18, 2024 22:08

andrewtoth mentioned this pull request Aug 18, 2024

Don't empty dbcache on prune flushes: >30% faster IBD #28280

Merged

DrahtBot removed the CI failed label Aug 18, 2024

This was referenced Aug 19, 2024

test: [refactor] Use m_rng directly #30571

Merged

kernel, logging: Pass Logger instances to kernel objects #30342

Draft

andrewtoth force-pushed the no-spent-and-fresh branch from feaea74 to 1f2f8e5 Compare August 19, 2024 12:38

l0rinc reviewed Aug 19, 2024

View reviewed changes

src/coins.cpp Outdated Show resolved Hide resolved

src/coins.cpp Outdated Show resolved Hide resolved

src/test/fuzz/coinscache_sim.cpp Show resolved Hide resolved

src/coins.h Outdated Show resolved Hide resolved

src/test/coins_tests.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 19, 2024

scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] #29641

Draft

l0rinc reviewed Aug 19, 2024

View reviewed changes

src/coins.cpp Outdated Show resolved Hide resolved

l0rinc suggested changes Aug 26, 2024

View reviewed changes

DrahtBot added the Needs rebase label Aug 28, 2024

andrewtoth force-pushed the no-spent-and-fresh branch 2 times, most recently from 82b6263 to 7d2b499 Compare August 31, 2024 19:43

DrahtBot added CI failed and removed Needs rebase CI failed labels Aug 31, 2024

DrahtBot mentioned this pull request Sep 1, 2024

scripted-diff: LogPrint -> LogDebug #30750

Merged

l0rinc suggested changes Sep 1, 2024

View reviewed changes

DrahtBot added the Needs rebase label Sep 2, 2024

andrewtoth force-pushed the no-spent-and-fresh branch from 7d2b499 to 34a101d Compare September 2, 2024 23:47

DrahtBot removed the Needs rebase label Sep 3, 2024

andrewtoth referenced this pull request in l0rinc/bitcoin Sep 7, 2024

refactor: Rely on returned value of GetCoin instead of parameter

de0d22e

l0rinc reviewed Sep 8, 2024

View reviewed changes

src/test/fuzz/coinscache_sim.cpp Outdated Show resolved Hide resolved

src/test/fuzz/coinscache_sim.cpp Outdated Show resolved Hide resolved

src/coins.cpp Outdated Show resolved Hide resolved

l0rinc reviewed Sep 8, 2024

View reviewed changes

src/test/coins_tests.cpp Outdated Show resolved Hide resolved

l0rinc reviewed Dec 13, 2024

View reviewed changes

DrahtBot mentioned this pull request Jan 22, 2025

Use number of dirty cache entries in flush warnings/logs #31703

Open

l0rinc reviewed Jan 23, 2025

View reviewed changes

src/test/fuzz/coins_view.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 15, 2025

WIP: speed up BatchWrite by sorting the batches in descending order #31875

Closed

DrahtBot mentioned this pull request Mar 24, 2025

Draft: CCoinMap Experiments #32128

Draft

DrahtBot mentioned this pull request Apr 1, 2025

coins: replace manual CDBBatch size estimation with LevelDB's native ApproximateSize #32185

Merged

DrahtBot added the Needs rebase label Apr 8, 2025

andrewtoth force-pushed the no-spent-and-fresh branch from 6956ee9 to 7f4a3bf Compare July 9, 2025 01:09

DrahtBot added the CI failed label Jul 9, 2025

DrahtBot removed the Needs rebase label Jul 9, 2025

andrewtoth added 7 commits July 8, 2025 21:42

coins: coins returned from GetCoin cannot be spent

320482e

coins: remove redundant IsDirty checks in BatchWrite

a421be4

It is no longer possible to get non-DIRTY entries in BatchWrite

coins: remove IsFresh check in Uncache

aaef2d6

It is not possible for an entry to be FRESH if not already DIRTY.

coins: assume entry is dirty in Next and Prev

b2622c4

test: simplify coins sanity check

bcf4ec0

andrewtoth force-pushed the no-spent-and-fresh branch from 7f4a3bf to 53dbebd Compare July 9, 2025 01:42

DrahtBot removed the CI failed label Jul 9, 2025

DrahtBot mentioned this pull request Jul 9, 2025

coins: fix cachedCoinsUsage accounting in CCoinsViewCache #32313

Open

andrewtoth force-pushed the no-spent-and-fresh branch from 5f4095f to fd2338d Compare July 10, 2025 18:29

l0rinc reviewed Jul 15, 2025

View reviewed changes

l0rinc mentioned this pull request Jul 15, 2025

test: Improve getbalance minconf behavior documentation and testing #32974

Closed

andrewtoth mentioned this pull request Jul 19, 2025

coins: remove SetFresh method from CCoinsCacheEntry #33018

Open

andrewtoth closed this Jul 19, 2025

		@@ -148,14 +146,13 @@ class CoinsViewBottom final : public CCoinsView
		public:
		std::optional<Coin> GetCoin(const COutPoint& outpoint) const final

-    static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel, bool fresh = false) noexcept
-    {
-        AddFlags(fresh ? FRESH | DIRTY : DIRTY, pair, sentinel);
-    }
+    static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel, bool fresh = false) noexcept
+    {
+        if (!pair.second.m_flags) {
+            Assume(!pair.second.m_prev && !pair.second.m_next);
+            pair.second.m_prev = sentinel.second.m_prev;
+            pair.second.m_next = &sentinel;
+            sentinel.second.m_prev = &pair;
+            pair.second.m_prev->second.m_next = &pair;
+        }
+        Assume(pair.second.m_prev && pair.second.m_next);
+        pair.second.m_flags |= fresh ? FRESH | DIRTY : DIRTY;
+    }

		@@ -188,17 +183,15 @@ struct CCoinsCacheEntry
		bool IsDirty() const noexcept { return m_flags & DIRTY; }
		bool IsFresh() const noexcept { return m_flags & FRESH; }

-    bool IsFresh() const noexcept { return m_flags & FRESH; }
+    bool IsDirtyAndFresh() const noexcept
+    {
+        const bool is_fresh = m_flags & FRESH;
+        Assume(IsDirty() || !is_fresh);
+        return is_fresh;
+    }

	CCoinsCacheEntry::SetDirty(n2, sentinel, true);
	CCoinsCacheEntry::SetDirty(n2, sentinel, /fresh=/true);

coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries #30673

coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries #30673

Uh oh!

Conversation

andrewtoth commented Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Aug 18, 2024

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewtoth commented Aug 20, 2024

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

l0rinc Sep 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

l0rinc commented Jul 6, 2025

Uh oh!

DrahtBot commented Jul 9, 2025

andrewtoth commented Aug 18, 2024 •

edited

Loading

DrahtBot commented Aug 18, 2024 •

edited

Loading

l0rinc Sep 1, 2024 •

edited

Loading

l0rinc left a comment •

edited

Loading

l0rinc Jul 15, 2025 •

edited

Loading