Skip to content

Commit cd0498e

Browse files
l0rincandrewtoth
l0rinc
and
andrewtoth
committed
coins, refactor: Split up AddFlags to remove invalid states
CCoinsCacheEntry provided general access to its internal flags state, even though in reality it could only be clean, fresh, dirty or fresh|dirty. After it got dirtied we couldn't set the state back to clean by AddFlags(0) - tests were explicitly checking against that. This commit cleans up the public interface to make this distinction cleaner and invalid behavior impossible instead of just checked by tests. This includes the removal of redundant `inline` qualifiers (we're inside a struct). Also renamed `self` to `pair` to simplify the upcoming commits. Also modernized `EmplaceCoinInternalDANGER` since it was already modified. Co-authored-by: Andrew Toth <[email protected]>
1 parent 74fb193 commit cd0498e

File tree

5 files changed

+50
-68
lines changed

5 files changed

+50
-68
lines changed

‎src/coins.cpp

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const
4949
cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
5050
if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins
5151
// The parent only has an empty entry for this outpoint; we can consider our version as fresh.
52-
ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel);
52+
CCoinsCacheEntry::SetFresh(*ret, m_sentinel);
5353
}
5454
} else {
5555
cacheCoins.erase(ret);
@@ -95,7 +95,8 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
9595
fresh = !it->second.IsDirty();
9696
}
9797
it->second.coin = std::move(coin);
98-
it->second.AddFlags(CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0), *it, m_sentinel);
98+
CCoinsCacheEntry::SetDirty(*it, m_sentinel);
99+
if (fresh) CCoinsCacheEntry::SetFresh(*it, m_sentinel);
99100
cachedCoinsUsage += it->second.coin.DynamicMemoryUsage();
100101
TRACE5(utxocache, add,
101102
outpoint.hash.data(),
@@ -107,13 +108,8 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
107108

108109
void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) {
109110
cachedCoinsUsage += coin.DynamicMemoryUsage();
110-
auto [it, inserted] = cacheCoins.emplace(
111-
std::piecewise_construct,
112-
std::forward_as_tuple(std::move(outpoint)),
113-
std::forward_as_tuple(std::move(coin)));
114-
if (inserted) {
115-
it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel);
116-
}
111+
auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin));
112+
if (inserted) CCoinsCacheEntry::SetDirty(*it, m_sentinel);
117113
}
118114

119115
void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) {
@@ -143,7 +139,7 @@ bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) {
143139
if (it->second.IsFresh()) {
144140
cacheCoins.erase(it);
145141
} else {
146-
it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel);
142+
CCoinsCacheEntry::SetDirty(*it, m_sentinel);
147143
it->second.coin.Clear();
148144
}
149145
return true;
@@ -203,13 +199,11 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
203199
entry.coin = it->second.coin;
204200
}
205201
cachedCoinsUsage += entry.coin.DynamicMemoryUsage();
206-
entry.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel);
202+
CCoinsCacheEntry::SetDirty(*itUs, m_sentinel);
207203
// We can mark it FRESH in the parent if it was FRESH in the child
208204
// Otherwise it might have just been flushed from the parent's cache
209205
// and already exist in the grandparent
210-
if (it->second.IsFresh()) {
211-
entry.AddFlags(CCoinsCacheEntry::FRESH, *itUs, m_sentinel);
212-
}
206+
if (it->second.IsFresh()) CCoinsCacheEntry::SetFresh(*itUs, m_sentinel);
213207
}
214208
} else {
215209
// Found the entry in the parent cache
@@ -237,7 +231,7 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
237231
itUs->second.coin = it->second.coin;
238232
}
239233
cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage();
240-
itUs->second.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel);
234+
CCoinsCacheEntry::SetDirty(*itUs, m_sentinel);
241235
// NOTE: It isn't safe to mark the coin as FRESH in the parent
242236
// cache. If it already existed and was spent in the parent
243237
// cache then marking it FRESH would prevent that spentness

‎src/coins.h

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ struct CCoinsCacheEntry
110110
private:
111111
/**
112112
* These are used to create a doubly linked list of flagged entries.
113-
* They are set in AddFlags and unset in ClearFlags.
113+
* They are set in SetDirty, SetFresh, and unset in SetClean.
114114
* A flagged entry is any entry that is either DIRTY, FRESH, or both.
115115
*
116116
* DIRTY entries are tracked so that only modified entries can be passed to
@@ -156,52 +156,58 @@ struct CCoinsCacheEntry
156156
explicit CCoinsCacheEntry(Coin&& coin_) noexcept : coin(std::move(coin_)) {}
157157
~CCoinsCacheEntry()
158158
{
159-
ClearFlags();
159+
SetClean();
160160
}
161161

162162
//! Adding a flag also requires a self reference to the pair that contains
163163
//! this entry in the CCoinsCache map and a reference to the sentinel of the
164164
//! flagged pair linked list.
165-
inline void AddFlags(uint8_t flags, CoinsCachePair& self, CoinsCachePair& sentinel) noexcept
165+
void AddFlags(uint8_t flags, CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept
166166
{
167-
Assume(&self.second == this);
168-
if (!m_flags && flags) {
167+
Assume(flags & (DIRTY | FRESH));
168+
Assume(&pair.second == this);
169+
if (!m_flags) {
169170
m_prev = sentinel.second.m_prev;
170171
m_next = &sentinel;
171-
sentinel.second.m_prev = &self;
172-
m_prev->second.m_next = &self;
172+
sentinel.second.m_prev = &pair;
173+
m_prev->second.m_next = &pair;
173174
}
174175
m_flags |= flags;
175176
}
176-
inline void ClearFlags() noexcept
177+
static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { pair.second.AddFlags(DIRTY, pair, sentinel); }
178+
static void SetFresh(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { pair.second.AddFlags(FRESH, pair, sentinel); }
179+
180+
void SetClean() noexcept
177181
{
178182
if (!m_flags) return;
179183
m_next->second.m_prev = m_prev;
180184
m_prev->second.m_next = m_next;
181185
m_flags = 0;
182186
}
183-
inline uint8_t GetFlags() const noexcept { return m_flags; }
184-
inline bool IsDirty() const noexcept { return m_flags & DIRTY; }
185-
inline bool IsFresh() const noexcept { return m_flags & FRESH; }
187+
uint8_t GetFlags() const noexcept { return m_flags; }
188+
bool IsDirty() const noexcept { return m_flags & DIRTY; }
189+
bool IsFresh() const noexcept { return m_flags & FRESH; }
186190

187191
//! Only call Next when this entry is DIRTY, FRESH, or both
188-
inline CoinsCachePair* Next() const noexcept {
192+
CoinsCachePair* Next() const noexcept
193+
{
189194
Assume(m_flags);
190195
return m_next;
191196
}
192197

193198
//! Only call Prev when this entry is DIRTY, FRESH, or both
194-
inline CoinsCachePair* Prev() const noexcept {
199+
CoinsCachePair* Prev() const noexcept
200+
{
195201
Assume(m_flags);
196202
return m_prev;
197203
}
198204

199205
//! Only use this for initializing the linked list sentinel
200-
inline void SelfRef(CoinsCachePair& self) noexcept
206+
void SelfRef(CoinsCachePair& pair) noexcept
201207
{
202-
Assume(&self.second == this);
203-
m_prev = &self;
204-
m_next = &self;
208+
Assume(&pair.second == this);
209+
m_prev = &pair;
210+
m_next = &pair;
205211
// Set sentinel to DIRTY so we can call Next on it
206212
m_flags = DIRTY;
207213
}
@@ -279,13 +285,13 @@ struct CoinsViewCacheCursor
279285
{
280286
const auto next_entry{current.second.Next()};
281287
// If we are not going to erase the cache, we must still erase spent entries.
282-
// Otherwise clear the flags on the entry.
288+
// Otherwise, clear the state of the entry.
283289
if (!m_will_erase) {
284290
if (current.second.coin.IsSpent()) {
285291
m_usage -= current.second.coin.DynamicMemoryUsage();
286292
m_map.erase(current.first);
287293
} else {
288-
current.second.ClearFlags();
294+
current.second.SetClean();
289295
}
290296
}
291297
return next_entry;

‎src/test/coins_tests.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -596,7 +596,8 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, CAmo
596596
SetCoinsValue(value, entry.coin);
597597
auto inserted = map.emplace(OUTPOINT, std::move(entry));
598598
assert(inserted.second);
599-
inserted.first->second.AddFlags(flags, *inserted.first, sentinel);
599+
if (flags & DIRTY) CCoinsCacheEntry::SetDirty(*inserted.first, sentinel);
600+
if (flags & FRESH) CCoinsCacheEntry::SetFresh(*inserted.first, sentinel);
600601
return inserted.first->second.coin.DynamicMemoryUsage();
601602
}
602603

‎src/test/coinscachepair_tests.cpp

Lines changed: 10 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ std::list<CoinsCachePair> CreatePairs(CoinsCachePair& sentinel)
1919
nodes.emplace_back();
2020

2121
auto node{std::prev(nodes.end())};
22-
node->second.AddFlags(CCoinsCacheEntry::DIRTY, *node, sentinel);
22+
CCoinsCacheEntry::SetDirty(*node, sentinel);
2323

2424
BOOST_CHECK_EQUAL(node->second.GetFlags(), CCoinsCacheEntry::DIRTY);
2525
BOOST_CHECK_EQUAL(node->second.Next(), &sentinel);
@@ -53,7 +53,7 @@ BOOST_AUTO_TEST_CASE(linked_list_iteration)
5353
for (const auto& expected : nodes) {
5454
BOOST_CHECK_EQUAL(&expected, node);
5555
auto next = node->second.Next();
56-
node->second.ClearFlags();
56+
node->second.SetClean();
5757
node = next;
5858
}
5959
BOOST_CHECK_EQUAL(node, &sentinel);
@@ -146,69 +146,48 @@ BOOST_AUTO_TEST_CASE(linked_list_add_flags)
146146
CoinsCachePair n1;
147147
CoinsCachePair n2;
148148

149-
// Check that adding 0 flag has no effect
150-
n1.second.AddFlags(0, n1, sentinel);
151-
BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0);
152-
BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel);
153-
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel);
154-
155149
// Check that adding DIRTY flag inserts it into linked list and sets flags
156-
n1.second.AddFlags(CCoinsCacheEntry::DIRTY, n1, sentinel);
150+
CCoinsCacheEntry::SetDirty(n1, sentinel);
157151
BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY);
158152
BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel);
159153
BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
160154
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1);
161155
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1);
162156

163157
// Check that adding FRESH flag on new node inserts it after n1
164-
n2.second.AddFlags(CCoinsCacheEntry::FRESH, n2, sentinel);
158+
CCoinsCacheEntry::SetFresh(n2, sentinel);
165159
BOOST_CHECK_EQUAL(n2.second.GetFlags(), CCoinsCacheEntry::FRESH);
166160
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
167161
BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
168162
BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
169163
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
170164

171-
// Check that adding 0 flag has no effect, and doesn't change position
172-
n1.second.AddFlags(0, n1, sentinel);
173-
BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY);
174-
BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
175-
BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
176-
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1);
177-
BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
178-
179165
// Check that we can add extra flags, but they don't change our position
180-
n1.second.AddFlags(CCoinsCacheEntry::FRESH, n1, sentinel);
166+
CCoinsCacheEntry::SetFresh(n1, sentinel);
181167
BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH);
182168
BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
183169
BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
184170
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1);
185171
BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
186172

187173
// Check that we can clear flags then re-add them
188-
n1.second.ClearFlags();
174+
n1.second.SetClean();
189175
BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0);
190176
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
191177
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
192178
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
193179
BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
194180

195-
// Check that calling `ClearFlags` with 0 flags has no effect
196-
n1.second.ClearFlags();
181+
// Check that calling `SetClean` with 0 flags has no effect
182+
n1.second.SetClean();
197183
BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0);
198184
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
199185
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
200186
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
201187
BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
202188

203-
// Adding 0 still has no effect
204-
n1.second.AddFlags(0, n1, sentinel);
205-
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
206-
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
207-
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
208-
BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
209-
210-
// But adding DIRTY re-inserts it after n2
211-
n1.second.AddFlags(CCoinsCacheEntry::DIRTY, n1, sentinel);
189+
// Adding DIRTY re-inserts it after n2
190+
CCoinsCacheEntry::SetDirty(n1, sentinel);
212191
BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY);
213192
BOOST_CHECK_EQUAL(n2.second.Next(), &n1);
214193
BOOST_CHECK_EQUAL(n1.second.Prev(), &n2);

‎src/test/fuzz/coins_view.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
128128
LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000)
129129
{
130130
CCoinsCacheEntry coins_cache_entry;
131-
const auto flags{fuzzed_data_provider.ConsumeIntegral<uint8_t>()};
131+
const auto dirty{fuzzed_data_provider.ConsumeBool()};
132+
const auto fresh{fuzzed_data_provider.ConsumeBool()};
132133
if (fuzzed_data_provider.ConsumeBool()) {
133134
coins_cache_entry.coin = random_coin;
134135
} else {
@@ -140,7 +141,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
140141
coins_cache_entry.coin = *opt_coin;
141142
}
142143
auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first};
143-
it->second.AddFlags(flags, *it, sentinel);
144+
if (dirty) CCoinsCacheEntry::SetDirty(*it, sentinel);
145+
if (fresh) CCoinsCacheEntry::SetFresh(*it, sentinel);
144146
usage += it->second.coin.DynamicMemoryUsage();
145147
}
146148
bool expected_code_path = false;

0 commit comments

Comments
 (0)