Hmm, we’re we’re duplicating the FetchCoin implementation here - and also making it virtual which introduces a slight regression on the hot path.
Could we extract only the part that changes?
I think we can simplify it by adding a virtual GetCoinFromBase which either calls base->GetCoin(outpoint) (default impl) or in CoinsViewCacheNonMutating it could be overwritten with the new base->PeekCoin(outpoint). That way only the if (inserted) branch would pay for virtual dispatch and there would be no duplicated FetchCoin logic, something like:
0diff --git a/src/coins.cpp b/src/coins.cpp
1--- a/src/coins.cpp (revision 85b20448640f8209e453019c3e1494c1ae229d74)
2+++ b/src/coins.cpp (revision c3b34ec796a3fd6e34e0dfb862c61bc298ba8d6f)
3@@ -59,10 +59,15 @@
4 return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsage;
5 }
6
7+std::optional<Coin> CCoinsViewCache::GetCoinFromBase(const COutPoint& outpoint) const
8+{
9+ return base->GetCoin(outpoint);
10+}
11+
12 CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const {
13 const auto [ret, inserted] = cacheCoins.try_emplace(outpoint);
14 if (inserted) {
15- if (auto coin{base->GetCoin(outpoint)}) {
16+ if (auto coin{GetCoinFromBase(outpoint)}) {
17 ret->second.coin = std::move(*coin);
18 cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
19 if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins
20diff --git a/src/coins.h b/src/coins.h
21--- a/src/coins.h (revision 85b20448640f8209e453019c3e1494c1ae229d74)
22+++ b/src/coins.h (revision c3b34ec796a3fd6e34e0dfb862c61bc298ba8d6f)
23@@ -382,6 +382,8 @@
24 /* Cached dynamic memory usage for the inner Coin objects. */
25 mutable size_t cachedCoinsUsage{0};
26
27+ virtual std::optional<Coin> GetCoinFromBase(const COutPoint& outpoint) const;
28+
29 public:
30 CCoinsViewCache(CCoinsView *baseIn, bool deterministic = false);
31
32@@ -489,7 +491,7 @@
33 * [@note](/bitcoin-bitcoin/contributor/note/) this is marked const, but may actually append to `cacheCoins`, increasing
34 * memory usage.
35 */
36- virtual CCoinsMap::iterator FetchCoin(const COutPoint &outpoint) const;
37+ CCoinsMap::iterator FetchCoin(const COutPoint &outpoint) const;
38 };
39
40 //! Utility function to add all of a transaction's outputs to a cache.
41diff --git a/src/coinsviewcachenonmutating.h b/src/coinsviewcachenonmutating.h
42--- a/src/coinsviewcachenonmutating.h (revision 85b20448640f8209e453019c3e1494c1ae229d74)
43+++ b/src/coinsviewcachenonmutating.h (revision c3b34ec796a3fd6e34e0dfb862c61bc298ba8d6f)
44@@ -9,28 +9,16 @@
45 #include <primitives/transaction.h>
46
47 /**
48- * CCoinsViewCache subclass that does not call GetCoin on base via FetchCoin, so base will not be mutated before Flush.
49+ * CCoinsViewCache subclass that does not call GetCoin on base, so base will not be mutated before Flush.
50 * Only used in ConnectBlock to pass as an ephemeral view that can be reset if the block is invalid.
51- * It provides the same interface as CCoinsViewCache, overriding the FetchCoin private method.
52+ * It provides the same interface as CCoinsViewCache, overriding GetCoinFromBase().
53 */
54 class CoinsViewCacheNonMutating : public CCoinsViewCache
55 {
56 private:
57- CCoinsMap::iterator FetchCoin(const COutPoint& outpoint) const override
58+ std::optional<Coin> GetCoinFromBase(const COutPoint& outpoint) const override
59 {
60- auto [ret, inserted]{cacheCoins.try_emplace(outpoint)};
61- if (!inserted) return ret;
62-
63- // TODO: Remove spent checks once we no longer return spent coins in coinscache_sim CoinsViewBottom.
64- if (auto coin{base->PeekCoin(outpoint)}; coin && !coin->IsSpent()) {
65- ret->second.coin = std::move(*coin);
66- } else {
67- cacheCoins.erase(ret);
68- return cacheCoins.end();
69- }
70-
71- cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
72- return ret;
73+ return base->PeekCoin(outpoint);
74 }
75
76 public:
