Skip to content

Conversation

@l0rinc
Copy link
Contributor

@l0rinc l0rinc commented Dec 20, 2025

Problem

CCoinsView serves as the interface for the coins database layer, but historically had a dual nature that caused confusion:

  • It provided default noop implementations (returning std::nullopt, uint256(), false, nullptr) instead of being pure virtual
  • Callers could instantiate a bare CCoinsView and get silent no-op behavior
  • Mixing the interface definition with a built-in dummy implementation blurred the abstraction boundary

Context

This is part of ongoing coins caching cleanup efforts such as #34132, #33866, #33018, and #33512.

Fix

This PR makes CCoinsView a pure abstract interface and introduces an explicit noop implementation, in tiny incremental steps to ease review:

  • Add CCoinsViewEmpty as an explicit noop coins view for tests, benchmarks, and dummy backends
  • Replace all direct CCoinsView instantiations with CCoinsViewEmpty
  • Incrementally make all CCoinsView methods pure virtual (GetCoin, GetBestBlock, BatchWrite, EstimateSize, Cursor, GetHeadBlocks, HaveCoin)
  • Remove the legacy default implementations from coins.cpp
  • Update the coins view fuzzer to switch between a real database-backed view and CCoinsViewEmpty

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 20, 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/34124.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK andrewtoth, ajtowns, sipa

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:

  • #34165 (coins: don't mutate main cache when connecting block by andrewtoth)
  • #34164 (validation: add reusable coins view for ConnectBlock by andrewtoth)
  • #34132 (refactor: inline CCoinsViewErrorCatcher into CCoinsViewDB by l0rinc)
  • #33512 (coins: use number of dirty cache entries in flush warnings/logs by l0rinc)
  • #31132 (validation: fetch block inputs on parallel threads 3x faster IBD by andrewtoth)
  • #29060 (Policy: Report debug message why inputs are non standard by ismaelsadeeq)
  • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

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
Copy link
Contributor

Concept ACK

Makes sense to make an empty view explicit, and having the base class be purely virtual.

@l0rinc l0rinc changed the title refactor: make CCoinsView a purely abstract base class refactor: make CCoinsView a purely virtual abstract base class Dec 20, 2025
Copy link
Contributor

@ajtowns ajtowns 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, this seemed better separated as an interface and a dummy implementation to me too.

I think squashing all the commits after the first into a single commit would be better though.

bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
{
return GetCoin(outpoint).has_value();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I could see keeping this one; it seems like a reasonable default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this method because of the specialized database access which doesn't need to deserialize the read value:

  • bitcoin/src/txdb.cpp

    Lines 74 to 76 in fa5f297

    bool CCoinsViewDB::HaveCoin(const COutPoint &outpoint) const {
    return m_db->Exists(CoinEntry(&outpoint));
    }

Otherwise we could of course replace it with !!GetCoin(outpoint):

  • bitcoin/src/coins.cpp

    Lines 166 to 169 in 7f295e1

    bool CCoinsViewCache::HaveCoin(const COutPoint &outpoint) const {
    CCoinsMap::const_iterator it = FetchCoin(outpoint);
    return (it != cacheCoins.end() && !it->second.coin.IsSpent());
    }

We're not even really using the former in production code anyway (see #34132), we could probably easily inline the !!GetCoin(outpoint) to the few remaining call sites instead. Regardless, the current change should simplify usage and we can investigate that later.


Also, the underlying problem is that having a separate CCoinsView makes adding defaults weird, there's no logical default behavior.
But we could inline it into CCoinsViewBacked (and delete CCoinsView and rename CCoinsViewBacked back to CCoinsView) and use that as the base for the other caches, with the default implementation simply delegating to base, something like:

diff --git a/src/coins.cpp b/src/coins.cpp
index 9bdcb42f45..4f66ed447b 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -14,7 +14,7 @@ TRACEPOINT_SEMAPHORE(utxocache, spent);
 TRACEPOINT_SEMAPHORE(utxocache, uncache);
 
 CCoinsViewCache::CCoinsViewCache(CCoinsView* baseIn, bool deterministic) :
-    CCoinsViewBacked(baseIn), m_deterministic(deterministic),
+    CCoinsView(baseIn), m_deterministic(deterministic),
     cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/deterministic), CCoinsMap::key_equal{}, &m_cache_coins_memory_resource)
 {
     m_sentinel.second.SelfRef(m_sentinel);
@@ -362,10 +362,10 @@ static ReturnType ExecuteBackedWrapper(Func func, const std::vector<std::functio
 
 std::optional<Coin> CCoinsViewErrorCatcher::GetCoin(const COutPoint& outpoint) const
 {
-    return ExecuteBackedWrapper<std::optional<Coin>>([&]() { return CCoinsViewBacked::GetCoin(outpoint); }, m_err_callbacks);
+    return ExecuteBackedWrapper<std::optional<Coin>>([&]() { return CCoinsView::GetCoin(outpoint); }, m_err_callbacks);
 }
 
 bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint& outpoint) const
 {
-    return ExecuteBackedWrapper<bool>([&]() { return CCoinsViewBacked::HaveCoin(outpoint); }, m_err_callbacks);
+    return ExecuteBackedWrapper<bool>([&]() { return CCoinsView::HaveCoin(outpoint); }, m_err_callbacks);
 }
diff --git a/src/coins.h b/src/coins.h
index a64f30376d..7e8120f710 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -303,37 +303,46 @@ private:
     bool m_will_erase;
 };
 
-/** Pure abstract view on the open txout dataset. */
+/** Coins view backed by another CCoinsView. */
 class CCoinsView
 {
+protected:
+    CCoinsView* base{nullptr};
+
+    CCoinsView() = default;
+
 public:
+    explicit CCoinsView(CCoinsView* viewIn) : base(Assert(viewIn)) {}
+
     //! As we use CCoinsViews polymorphically, have a virtual destructor
     virtual ~CCoinsView() = default;
 
+    void SetBackend(CCoinsView& viewIn) { base = &viewIn; }
+
     //! Retrieve the Coin (unspent transaction output) for a given outpoint.
-    virtual std::optional<Coin> GetCoin(const COutPoint& outpoint) const = 0;
+    virtual std::optional<Coin> GetCoin(const COutPoint& outpoint) const { return base->GetCoin(outpoint); }
 
     //! Just check whether a given outpoint is unspent.
-    virtual bool HaveCoin(const COutPoint& outpoint) const = 0;
+    virtual bool HaveCoin(const COutPoint& outpoint) const { return base->HaveCoin(outpoint); }
 
     //! Retrieve the block hash whose state this CCoinsView currently represents
-    virtual uint256 GetBestBlock() const = 0;
+    virtual uint256 GetBestBlock() const { return base->GetBestBlock(); }
 
     //! Retrieve the range of blocks that may have been only partially written.
     //! If the database is in a consistent state, the result is the empty vector.
     //! Otherwise, a two-element vector is returned consisting of the new and
     //! the old block hash, in that order.
-    virtual std::vector<uint256> GetHeadBlocks() const = 0;
+    virtual std::vector<uint256> GetHeadBlocks() const { return base->GetHeadBlocks(); }
 
     //! Do a bulk modification (multiple Coin changes + BestBlock change).
     //! The passed cursor is used to iterate through the coins.
-    virtual bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) = 0;
+    virtual bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) { return base->BatchWrite(cursor, hashBlock); }
 
     //! Get a cursor to iterate over the whole state
-    virtual std::unique_ptr<CCoinsViewCursor> Cursor() const = 0;
+    virtual std::unique_ptr<CCoinsViewCursor> Cursor() const { return base->Cursor(); }
 
     //! Estimate database size
-    virtual size_t EstimateSize() const = 0;
+    virtual size_t EstimateSize() const { return base->EstimateSize(); }
 };
 
 /** Noop coins view. */
@@ -361,28 +370,8 @@ public:
     size_t EstimateSize() const override { return 0; }
 };
 
-/** CCoinsView backed by another CCoinsView */
-class CCoinsViewBacked : public CCoinsView
-{
-protected:
-    CCoinsView* base;
-
-public:
-    explicit CCoinsViewBacked(CCoinsView* viewIn) : base{viewIn} {}
-
-    void SetBackend(CCoinsView& viewIn) { base = &viewIn; }
-
-    std::optional<Coin> GetCoin(const COutPoint& outpoint) const override { return base->GetCoin(outpoint); }
-    bool HaveCoin(const COutPoint& outpoint) const override { return base->HaveCoin(outpoint); }
-    uint256 GetBestBlock() const override { return base->GetBestBlock(); }
-    std::vector<uint256> GetHeadBlocks() const override { return base->GetHeadBlocks(); }
-    bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) override { return base->BatchWrite(cursor, hashBlock); }
-    std::unique_ptr<CCoinsViewCursor> Cursor() const override { return base->Cursor(); }
-    size_t EstimateSize() const override { return base->EstimateSize(); }
-};
-
 /** CCoinsView that adds a memory cache for transactions to another CCoinsView */
-class CCoinsViewCache : public CCoinsViewBacked
+class CCoinsViewCache : public CCoinsView
 {
 private:
     const bool m_deterministic;
@@ -534,10 +523,10 @@ const Coin& AccessByTxid(const CCoinsViewCache& cache, const Txid& txid);
  *
  * Writes do not need similar protection, as failure to write is handled by the caller.
 */
-class CCoinsViewErrorCatcher final : public CCoinsViewBacked
+class CCoinsViewErrorCatcher final : public CCoinsView
 {
 public:
-    explicit CCoinsViewErrorCatcher(CCoinsView* view) : CCoinsViewBacked(view) {}
+    explicit CCoinsViewErrorCatcher(CCoinsView* view) : CCoinsView(view) {}
 
     void AddReadErrCallback(std::function<void()> f) {
         m_err_callbacks.emplace_back(std::move(f));
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 4dc692bfd8..3e47428e8e 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -720,7 +720,7 @@ bool CTxMemPool::HasNoInputsOf(const CTransaction &tx) const
     return true;
 }
 
-CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn) : CCoinsViewBacked(baseIn), mempool(mempoolIn) { }
+CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn) : CCoinsView(baseIn), mempool(mempoolIn) { }
 
 std::optional<Coin> CCoinsViewMemPool::GetCoin(const COutPoint& outpoint) const
 {
diff --git a/src/txmempool.h b/src/txmempool.h
index 166a024823..4c24766c77 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -778,7 +778,7 @@ public:
  * signrawtransactionwithkey and signrawtransactionwithwallet,
  * as long as the conflicting transaction is not yet confirmed.
  */
-class CCoinsViewMemPool : public CCoinsViewBacked
+class CCoinsViewMemPool : public CCoinsView
 {
     /**
     * Coins made available by transactions being validated. Tracking these allows for package

This would introduce a sensible default as the base class behavior (always delegating) and we'd instantiate it with the empty sigleton, which would always do nothin'.
I haven't included this yet, we can discuss if it's a good idea.

@l0rinc
Copy link
Contributor Author

l0rinc commented Dec 28, 2025

This change also revealed a potential UB in MemPoolAccept member init order. Since class members are constructed in declaration order (not initializer-list order), calls to a virtual method (having m_view depend on m_dummy but declared before it) can result in a virtual call through an uninitialized vptr and crash.
Since the constructor didn't dereference m_dummy during m_view construction, the latent ordering hazard stayed dormant. This cleanup accidentally fixed it, but to make it explicit, I have extracted it to the first commit as a temporary fix.

src/coins.cpp Outdated
CCoinsViewBacked(baseIn), m_deterministic(deterministic),
cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/deterministic), CCoinsMap::key_equal{}, &m_cache_coins_memory_resource)
{
(void)base->GetBestBlock(); // Sanity check validity of base
Copy link
Contributor

@andrewtoth andrewtoth Dec 29, 2025

Choose a reason for hiding this comment

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

I'm not sure about this sanity check. We're checking that a call to this method will work? Is this better than an assertion that baseIn is non-null? Why not refactor baseIn to be a reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not null, it will give illegal access since it points to a random address. Try reverting and running any of the mentioned tests from the commit message.

Copy link
Contributor

@andrewtoth andrewtoth Dec 29, 2025

Choose a reason for hiding this comment

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

Ok, but if it's a pointer it is legal for it to be a nullptr. So we should check first if it is not nullptr.
Also, instead of accessing a method, we could do a dynamic cast to CCoinsView* and assert the result.

Suggested change
(void)base->GetBestBlock(); // Sanity check validity of base
assert(!base || dynamic_cast<CCoinsView*>(base));

Copy link
Contributor Author

@l0rinc l0rinc Dec 29, 2025

Choose a reason for hiding this comment

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

assert(!base || dynamic_cast<CCoinsView*>(base)); doesn't trigger the failure.

Reverting m_dummy (which I did accidentally in the first commit already):

diff --git a/src/validation.cpp b/src/validation.cpp
--- a/src/validation.cpp	(revision 5f5ff9798098a7cf9c5311dc54c615245fec1a3b)
+++ b/src/validation.cpp	(date 1767039346685)
@@ -722,11 +722,6 @@
     }
 
 private:
-    /** When m_view is connected to m_dummy, it can no longer look up coins from the mempool or UTXO set (meaning no disk
-     * operations happen), but can still return coins it accessed previously. Useful for keeping track of which coins
-     * were pulled from disk. */
-    CCoinsView m_dummy;
-
     CTxMemPool& m_pool;
 
     /** Holds a cached view of available coins from the UTXO set, mempool, and artificial temporary coins (to enable package validation).
@@ -746,6 +741,10 @@
     /** When m_view is connected to m_viewmempool as its backend, it can pull coins from the mempool and from the UTXO
      * set. This is also where temporary coins are stored. */
     CCoinsViewMemPool m_viewmempool;
+    /** When m_view is connected to m_dummy, it can no longer look up coins from the mempool or UTXO set (meaning no disk
+     * operations happen), but can still return coins it accessed previously. Useful for keeping track of which coins
+     * were pulled from disk. */
+    CCoinsView m_dummy;
 
     Chainstate& m_active_chainstate;

And running:

cmake -B build -DCMAKE_BUILD_TYPE=Debug && cmake --build build && ./build/bin/test_bitcoin --run_test=wallet_tests

fails correctly with:

Running 13 test cases...
******** errors disabling the alternate stack:
#error:22
Invalid argument
unknown location:0: fatal error: in "wallet_tests/CreateWallet": memory access violation at address: 0x5f4d1884: invalid permissions
wallet/test/wallet_tests.cpp:622: last checkpoint

Moved the proper removal to the first commit: https://github.com/bitcoin/bitcoin/compare/784769b5b1a8465d1a09ec489a303c4616d11896..a69ea84b83d22c66ddeeba6d27b761036d6fec40

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm dynamic casting uninitialized memory should be UB as well, but I guess it doesn't trigger a memory access violation.
But, we still need to check that base is not nullptr before accessing, otherwise passing nullptr would trigger undefined behavior.

Copy link
Contributor Author

@l0rinc l0rinc Dec 30, 2025

Choose a reason for hiding this comment

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

I don't think that is ideal, as the compiler is allowed to assume it won't happen and optimize it away.

I needed something to demonstrate the UB - and that it's fixed now, so I have removed this reproducer in the second commit. Would be cool if we could instrument the code with something like -fsanitize=vptr or -Wreorder-ctor (haven't tried any of them, not sure they would have found this one).

I've also added the Assert to prohibit nullptr bases in CCoinsViewBacked - we should use CCoinsViewEmpty::Get() from now on to stop the propagation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need this commit at all, since the PR deletes m_dummy and doesn't keep this line either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is explain in the commit message:

In a follow-up m_dummy will be completely removed, this commit was meant to demonstrate why that was necessary.

I.e. I don't want the fix to be accidental - fix first, replace after.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it's not necessary. There is no UB unless you add this commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's why I added that commit to demonstrate that we're fixing a potential UB as well here.
It may not be UB currently since it may not be triggered by any compiler we're using currently - but I don't want to think about those, it's why I'm demonstrating the problem explicitly.

@l0rinc l0rinc force-pushed the l0rinc/pure-virtual-CCoinsView branch from 784769b to a69ea84 Compare December 29, 2025 20:21
Copy link
Member

@sipa sipa 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

src/coins.cpp Outdated
CCoinsViewBacked(baseIn), m_deterministic(deterministic),
cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/deterministic), CCoinsMap::key_equal{}, &m_cache_coins_memory_resource)
{
(void)base->GetBestBlock(); // Sanity check validity of base
Copy link
Member

Choose a reason for hiding this comment

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

As I understand it, this is sanity testing through triggering UB in the case the bug is present. I don't think that is ideal, as the compiler is allowed to assume it won't happen and optimize it away. This isn't too far fetched - if it could be inferred that (all implementations of) CCoinsView::GetBestBlock are side-effect free, then the statement (void)base->GetBestBlock(); could just be optimized away.

Testing for bugs due to initialization order seems more like a job for UBSan. Unfortunately, it seems none of the current fuzz tests (with UBSan enabled) trigger with the following patch (on master):

diff --git a/src/coins.cpp b/src/coins.cpp
index 14381ecb0aa..c7206c22554 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -38,6 +38,7 @@ CCoinsViewCache::CCoinsViewCache(CCoinsView* baseIn, bool deterministic) :
     CCoinsViewBacked(baseIn), m_deterministic(deterministic),
     cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/deterministic), CCoinsMap::key_equal{}, &m_cache_coins_memory_resource)
 {
+    assert(!base || dynamic_cast<CCoinsView*>(base));
     m_sentinel.second.SelfRef(m_sentinel);
 }

I'm not objecting to this change, but I think it would be better if we could test for bugs of this class in a more systematic way.

@l0rinc l0rinc force-pushed the l0rinc/pure-virtual-CCoinsView branch from a69ea84 to b23d784 Compare December 30, 2025 23:14
src/coins.cpp Outdated
CCoinsViewBacked(baseIn), m_deterministic(deterministic),
cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/deterministic), CCoinsMap::key_equal{}, &m_cache_coins_memory_resource)
{
(void)base->GetBestBlock(); // Sanity check validity of base
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need this commit at all, since the PR deletes m_dummy and doesn't keep this line either.

[&] {
if (fuzzed_data_provider.ConsumeBool()) {
backend_coins_view = CCoinsView{};
if (is_db || fuzzed_data_provider.ConsumeBool()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to keep the behavior of resetting the backend.

Suggested change
if (is_db || fuzzed_data_provider.ConsumeBool()) {
if (is_db || fuzzed_data_provider.ConsumeBool()) {
auto db_params = DBParams{
.path = "",
.cache_bytes = 1_MiB,
.memory_only = true,
};
backend_coins_view = CCoinsViewDB{std::move(db_params), CoinsViewOptions{}};
}
coins_view_cache.SetBackend(backend_coins_view);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain why you think so? If we have a database backing the cache than we should switch it to another one? in

m_view.SetBackend(CCoinsViewEmpty::Get());
we have a normal db switched to an empty one. Here we're either setting the provided backend_coins_view or creating a new one - is that not what you think we should do?

Copy link
Contributor

Choose a reason for hiding this comment

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

We are making assertions on backend_coins_view below, where we assume it is what is set as the backend of coins_view_cache. So if we set the backend as the empty view singleton and don't update backend_coins_view those assertions will fail.

Copy link
Contributor Author

@l0rinc l0rinc Jan 1, 2026

Choose a reason for hiding this comment

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

Edit: pushed something, what do you think?

@l0rinc l0rinc force-pushed the l0rinc/pure-virtual-CCoinsView branch 2 times, most recently from 50245ca to e3d4435 Compare January 1, 2026 21:36
l0rinc added 2 commits January 3, 2026 14:09
`MemPoolAccept` constructs `m_view` with a backend pointer to a `CCoinsView`.
Since class members are constructed in declaration order (not initializer-list order), calls to a virtual method (having `m_view` declared before `m_dummy`) can result in a virtual call through an uninitialized vptr and crash.

Add a small `CCoinsViewCache` constructor sanity check (`base->GetBestBlock()`) so uninitialized backends are caught immediately. This is meant to demonstrate a possible UB and will be removed in a follow-up commit after the field is initialized by a singleton instead.

Without the move:
```
The following tests FAILED:
        112 - txdownload_tests (SEGFAULT)
        115 - txpackage_tests (SEGFAULT)
        118 - txvalidation_tests (SEGFAULT)
        119 - txvalidationcache_tests (SEGFAULT)
        127 - validation_block_tests (SEGFAULT)
        146 - wallet_tests (SEGFAULT)
```

Move `m_dummy` before `m_view` so it is constructed first.
In a follow-up `m_dummy` will be completely removed, this commit was meant to demonstrate why that was necessary.
This makes the `backend_coins_view` parameter relocatable in a follow-up commit.
Besides the parameter and field renames and reformats, the `CCoinsViewBacked` constructor was also made explicit, and the `SetBackend` method moved up higher. The `CCoinsViews` destructor was also moved to the top.

This should derisk and simplify follow-up commits by reducing noise.
@l0rinc l0rinc force-pushed the l0rinc/pure-virtual-CCoinsView branch from e3d4435 to d82eedd Compare January 3, 2026 13:19
@l0rinc
Copy link
Contributor Author

l0rinc commented Jan 3, 2026

A slightly more involved rebase after #33866.
Most of the conflicts were around moving BatchWrite around.
I have also separated the second commit into 3, to address reviewer feedback regarding cleaning up naming and formatting of affected methods. This way - after a boring refactor commit - the non-trivial follow-up commits are easier to review.
Can be reviewed via git range-diff -w -U0 master e3d4435 3b800c9.

l0rinc and others added 7 commits January 3, 2026 14:27
… testing

This enables making `CCoinsView` purely virtual - which is done step-by-step in the following commits.

`CCoinsViewEmpty` is now only constructible via `CCoinsViewEmpty::Get()`.
Simplified all usages to delegate to the stateless singleton instead.

The fuzzer was also updated to switch randomly between real db backing and empty view.

Also removed the sanity check from `CCoinsViewCache` constructor since the initialization isn't done via a local field anymore.

Co-authored-by: Andrew Toth <[email protected]>
It's a simple delegate, the cpp is already really crowded, let's do these inline.

Also note that the `CCoinsViewBacked` constructor is asserted to not take any `nullptr` values anymore, since the reason why `base` was stored as a pointer was that references cannot be reallocated, and we need to call `SetBackend` in a few places - which takes a reference, also proving that `nullptr` was never the goal.
@l0rinc l0rinc force-pushed the l0rinc/pure-virtual-CCoinsView branch from d82eedd to 3b800c9 Compare January 3, 2026 13:30
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 3, 2026

🚧 At least one of the CI tasks failed.
Task ASan + LSan + UBSan + integer: https://github.com/bitcoin/bitcoin/actions/runs/20677817262/job/59367841784
LLM reason (✨ experimental): Compiler error in fuzz/coins_view.cpp: attempted assignment with no viable overloaded operator=, causing the build to fail.

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.

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