-
Notifications
You must be signed in to change notification settings - Fork 38.6k
refactor: make CCoinsView a purely virtual abstract base class
#34124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34124. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK Makes sense to make an empty view explicit, and having the base class be purely virtual. |
CCoinsView a purely abstract base classCCoinsView a purely virtual abstract base class
ajtowns
left a comment
There was a problem hiding this 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(); | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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):
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.
38ae408 to
d5c77c7
Compare
d5c77c7 to
784769b
Compare
|
This change also revealed a potential UB in |
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| (void)base->GetBestBlock(); // Sanity check validity of base | |
| assert(!base || dynamic_cast<CCoinsView*>(base)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_dummywill 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
784769b to
a69ea84
Compare
sipa
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
a69ea84 to
b23d784
Compare
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 |
There was a problem hiding this comment.
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.
src/test/fuzz/coins_view.cpp
Outdated
| [&] { | ||
| if (fuzzed_data_provider.ConsumeBool()) { | ||
| backend_coins_view = CCoinsView{}; | ||
| if (is_db || fuzzed_data_provider.ConsumeBool()) { |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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
Line 880 in 7817197
| m_view.SetBackend(CCoinsViewEmpty::Get()); |
backend_coins_view or creating a new one - is that not what you think we should do?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
50245ca to
e3d4435
Compare
`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.
e3d4435 to
d82eedd
Compare
|
A slightly more involved rebase after #33866. |
… 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]>
…tual in `CCoinsView`
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.
d82eedd to
3b800c9
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Problem
CCoinsViewserves as the interface for the coins database layer, but historically had a dual nature that caused confusion:std::nullopt,uint256(),false,nullptr) instead of being pure virtualCCoinsViewand get silent no-op behaviorContext
This is part of ongoing coins caching cleanup efforts such as #34132, #33866, #33018, and #33512.
Fix
This PR makes
CCoinsViewa pure abstract interface and introduces an explicit noop implementation, in tiny incremental steps to ease review:CCoinsViewEmptyas an explicit noop coins view for tests, benchmarks, and dummy backendsCCoinsViewinstantiations withCCoinsViewEmptyCCoinsViewmethods pure virtual (GetCoin,GetBestBlock,BatchWrite,EstimateSize,Cursor,GetHeadBlocks,HaveCoin)coins.cppCCoinsViewEmpty