foster/bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-03-05 14:06:27 -05:00
Code Issues Activity

coins, refactor: Split up AddFlags to remove invalid states

Browse source 
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 <andrewstoth@gmail.com>
This commit is contained in:
Lőrinc 2024-09-13 10:58:16 +02:00
parent 74fb19317a
commit cd0498eabc
5 changed files with 50 additions and 68 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

24
src/coins.cpp
View file

@ -49,7 +49,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const
cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage(); cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins
// The parent only has an empty entry for this outpoint; we can consider our version as fresh. // The parent only has an empty entry for this outpoint; we can consider our version as fresh.
ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel); CCoinsCacheEntry::SetFresh(*ret, m_sentinel);
} }
} else { } else {
cacheCoins.erase(ret); cacheCoins.erase(ret);
@ -95,7 +95,8 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
fresh = !it->second.IsDirty(); fresh = !it->second.IsDirty();
} }
it->second.coin = std::move(coin); it->second.coin = std::move(coin);
it->second.AddFlags(CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0), *it, m_sentinel); CCoinsCacheEntry::SetDirty(*it, m_sentinel);
if (fresh) CCoinsCacheEntry::SetFresh(*it, m_sentinel);
cachedCoinsUsage += it->second.coin.DynamicMemoryUsage(); cachedCoinsUsage += it->second.coin.DynamicMemoryUsage();
TRACE5(utxocache, add, TRACE5(utxocache, add,
outpoint.hash.data(), outpoint.hash.data(),
@ -107,13 +108,8 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) { void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) {
cachedCoinsUsage += coin.DynamicMemoryUsage(); cachedCoinsUsage += coin.DynamicMemoryUsage();
auto [it, inserted] = cacheCoins.emplace( auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin));
std::piecewise_construct, if (inserted) CCoinsCacheEntry::SetDirty(*it, m_sentinel);
std::forward_as_tuple(std::move(outpoint)),
std::forward_as_tuple(std::move(coin)));
if (inserted) {
it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel);
}
} }
void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) { 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) {
if (it->second.IsFresh()) { if (it->second.IsFresh()) {
cacheCoins.erase(it); cacheCoins.erase(it);
} else { } else {
it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel); CCoinsCacheEntry::SetDirty(*it, m_sentinel);
it->second.coin.Clear(); it->second.coin.Clear();
} }
return true; return true;
@ -203,13 +199,11 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
entry.coin = it->second.coin; entry.coin = it->second.coin;
} }
cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); cachedCoinsUsage += entry.coin.DynamicMemoryUsage();
entry.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel); CCoinsCacheEntry::SetDirty(*itUs, m_sentinel);
// We can mark it FRESH in the parent if it was FRESH in the child // We can mark it FRESH in the parent if it was FRESH in the child
// Otherwise it might have just been flushed from the parent's cache // Otherwise it might have just been flushed from the parent's cache
// and already exist in the grandparent // and already exist in the grandparent
if (it->second.IsFresh()) { if (it->second.IsFresh()) CCoinsCacheEntry::SetFresh(*itUs, m_sentinel);
entry.AddFlags(CCoinsCacheEntry::FRESH, *itUs, m_sentinel);
}
} }
} else { } else {
// Found the entry in the parent cache // Found the entry in the parent cache
@ -237,7 +231,7 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
itUs->second.coin = it->second.coin; itUs->second.coin = it->second.coin;
} }
cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage(); cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage();
itUs->second.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel); CCoinsCacheEntry::SetDirty(*itUs, m_sentinel);
// NOTE: It isn't safe to mark the coin as FRESH in the parent // NOTE: It isn't safe to mark the coin as FRESH in the parent
// cache. If it already existed and was spent in the parent // cache. If it already existed and was spent in the parent
// cache then marking it FRESH would prevent that spentness // cache then marking it FRESH would prevent that spentness

44
src/coins.h
View file

@ -110,7 +110,7 @@ struct CCoinsCacheEntry
private: private:
/** /**
* These are used to create a doubly linked list of flagged entries. * These are used to create a doubly linked list of flagged entries.
* They are set in AddFlags and unset in ClearFlags. * They are set in SetDirty, SetFresh, and unset in SetClean.
* A flagged entry is any entry that is either DIRTY, FRESH, or both. * A flagged entry is any entry that is either DIRTY, FRESH, or both.
* *
* DIRTY entries are tracked so that only modified entries can be passed to * DIRTY entries are tracked so that only modified entries can be passed to
@ -156,52 +156,58 @@ public:
explicit CCoinsCacheEntry(Coin&& coin_) noexcept : coin(std::move(coin_)) {} explicit CCoinsCacheEntry(Coin&& coin_) noexcept : coin(std::move(coin_)) {}
~CCoinsCacheEntry() ~CCoinsCacheEntry()
{ {
ClearFlags(); SetClean();
} }
//! Adding a flag also requires a self reference to the pair that contains //! Adding a flag also requires a self reference to the pair that contains
//! this entry in the CCoinsCache map and a reference to the sentinel of the //! this entry in the CCoinsCache map and a reference to the sentinel of the
//! flagged pair linked list. //! flagged pair linked list.
inline void AddFlags(uint8_t flags, CoinsCachePair& self, CoinsCachePair& sentinel) noexcept void AddFlags(uint8_t flags, CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept
{ {
Assume(&self.second == this); Assume(flags & (DIRTY | FRESH));
if (!m_flags && flags) { Assume(&pair.second == this);
if (!m_flags) {
m_prev = sentinel.second.m_prev; m_prev = sentinel.second.m_prev;
m_next = &sentinel; m_next = &sentinel;
sentinel.second.m_prev = &self; sentinel.second.m_prev = &pair;
m_prev->second.m_next = &self; m_prev->second.m_next = &pair;
} }
m_flags |= flags; m_flags |= flags;
} }
inline void ClearFlags() noexcept static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { pair.second.AddFlags(DIRTY, pair, sentinel); }
static void SetFresh(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { pair.second.AddFlags(FRESH, pair, sentinel); }
void SetClean() noexcept
{ {
if (!m_flags) return; if (!m_flags) return;
m_next->second.m_prev = m_prev; m_next->second.m_prev = m_prev;
m_prev->second.m_next = m_next; m_prev->second.m_next = m_next;
m_flags = 0; m_flags = 0;
} }
inline uint8_t GetFlags() const noexcept { return m_flags; } uint8_t GetFlags() const noexcept { return m_flags; }
inline bool IsDirty() const noexcept { return m_flags & DIRTY; } bool IsDirty() const noexcept { return m_flags & DIRTY; }
inline bool IsFresh() const noexcept { return m_flags & FRESH; } bool IsFresh() const noexcept { return m_flags & FRESH; }
//! Only call Next when this entry is DIRTY, FRESH, or both //! Only call Next when this entry is DIRTY, FRESH, or both
inline CoinsCachePair* Next() const noexcept { CoinsCachePair* Next() const noexcept
{
Assume(m_flags); Assume(m_flags);
return m_next; return m_next;
} }
//! Only call Prev when this entry is DIRTY, FRESH, or both //! Only call Prev when this entry is DIRTY, FRESH, or both
inline CoinsCachePair* Prev() const noexcept { CoinsCachePair* Prev() const noexcept
{
Assume(m_flags); Assume(m_flags);
return m_prev; return m_prev;
} }
//! Only use this for initializing the linked list sentinel //! Only use this for initializing the linked list sentinel
inline void SelfRef(CoinsCachePair& self) noexcept void SelfRef(CoinsCachePair& pair) noexcept
{ {
Assume(&self.second == this); Assume(&pair.second == this);
m_prev = &self; m_prev = &pair;
m_next = &self; m_next = &pair;
// Set sentinel to DIRTY so we can call Next on it // Set sentinel to DIRTY so we can call Next on it
m_flags = DIRTY; m_flags = DIRTY;
} }
@ -279,13 +285,13 @@ struct CoinsViewCacheCursor
{ {
const auto next_entry{current.second.Next()}; const auto next_entry{current.second.Next()};
// If we are not going to erase the cache, we must still erase spent entries. // If we are not going to erase the cache, we must still erase spent entries.
// Otherwise clear the flags on the entry. // Otherwise, clear the state of the entry.
if (!m_will_erase) { if (!m_will_erase) {
if (current.second.coin.IsSpent()) { if (current.second.coin.IsSpent()) {
m_usage -= current.second.coin.DynamicMemoryUsage(); m_usage -= current.second.coin.DynamicMemoryUsage();
m_map.erase(current.first); m_map.erase(current.first);
} else { } else {
current.second.ClearFlags(); current.second.SetClean();
} }
} }
return next_entry; return next_entry;

3
src/test/coins_tests.cpp
View file

@ -596,7 +596,8 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, CAmo
SetCoinsValue(value, entry.coin); SetCoinsValue(value, entry.coin);
auto inserted = map.emplace(OUTPOINT, std::move(entry)); auto inserted = map.emplace(OUTPOINT, std::move(entry));
assert(inserted.second); assert(inserted.second);
inserted.first->second.AddFlags(flags, *inserted.first, sentinel); if (flags & DIRTY) CCoinsCacheEntry::SetDirty(*inserted.first, sentinel);
if (flags & FRESH) CCoinsCacheEntry::SetFresh(*inserted.first, sentinel);
return inserted.first->second.coin.DynamicMemoryUsage(); return inserted.first->second.coin.DynamicMemoryUsage();
} }

41
src/test/coinscachepair_tests.cpp
View file

@ -19,7 +19,7 @@ std::list<CoinsCachePair> CreatePairs(CoinsCachePair& sentinel)
nodes.emplace_back(); nodes.emplace_back();
auto node{std::prev(nodes.end())}; auto node{std::prev(nodes.end())};
node->second.AddFlags(CCoinsCacheEntry::DIRTY, *node, sentinel); CCoinsCacheEntry::SetDirty(*node, sentinel);
BOOST_CHECK_EQUAL(node->second.GetFlags(), CCoinsCacheEntry::DIRTY); BOOST_CHECK_EQUAL(node->second.GetFlags(), CCoinsCacheEntry::DIRTY);
BOOST_CHECK_EQUAL(node->second.Next(), &sentinel); BOOST_CHECK_EQUAL(node->second.Next(), &sentinel);
@ -53,7 +53,7 @@ BOOST_AUTO_TEST_CASE(linked_list_iteration)
for (const auto& expected : nodes) { for (const auto& expected : nodes) {
BOOST_CHECK_EQUAL(&expected, node); BOOST_CHECK_EQUAL(&expected, node);
auto next = node->second.Next(); auto next = node->second.Next();
node->second.ClearFlags(); node->second.SetClean();
node = next; node = next;
} }
BOOST_CHECK_EQUAL(node, &sentinel); BOOST_CHECK_EQUAL(node, &sentinel);
@ -146,14 +146,8 @@ BOOST_AUTO_TEST_CASE(linked_list_add_flags)
CoinsCachePair n1; CoinsCachePair n1;
CoinsCachePair n2; CoinsCachePair n2;
// Check that adding 0 flag has no effect
n1.second.AddFlags(0, n1, sentinel);
BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0);
BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel);
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel);
// Check that adding DIRTY flag inserts it into linked list and sets flags // Check that adding DIRTY flag inserts it into linked list and sets flags
n1.second.AddFlags(CCoinsCacheEntry::DIRTY, n1, sentinel); CCoinsCacheEntry::SetDirty(n1, sentinel);
BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY);
BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel); BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel);
BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel); BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
@ -161,23 +155,15 @@ BOOST_AUTO_TEST_CASE(linked_list_add_flags)
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1);
// Check that adding FRESH flag on new node inserts it after n1 // Check that adding FRESH flag on new node inserts it after n1
n2.second.AddFlags(CCoinsCacheEntry::FRESH, n2, sentinel); CCoinsCacheEntry::SetFresh(n2, sentinel);
BOOST_CHECK_EQUAL(n2.second.GetFlags(), CCoinsCacheEntry::FRESH); BOOST_CHECK_EQUAL(n2.second.GetFlags(), CCoinsCacheEntry::FRESH);
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
BOOST_CHECK_EQUAL(n1.second.Next(), &n2); BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
// Check that adding 0 flag has no effect, and doesn't change position
n1.second.AddFlags(0, n1, sentinel);
BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY);
BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1);
BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
// Check that we can add extra flags, but they don't change our position // Check that we can add extra flags, but they don't change our position
n1.second.AddFlags(CCoinsCacheEntry::FRESH, n1, sentinel); CCoinsCacheEntry::SetFresh(n1, sentinel);
BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH); BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH);
BOOST_CHECK_EQUAL(n1.second.Next(), &n2); BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel); BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
@ -185,30 +171,23 @@ BOOST_AUTO_TEST_CASE(linked_list_add_flags)
BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
// Check that we can clear flags then re-add them // Check that we can clear flags then re-add them
n1.second.ClearFlags(); n1.second.SetClean();
BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0);
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2); BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel); BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
// Check that calling `ClearFlags` with 0 flags has no effect // Check that calling `SetClean` with 0 flags has no effect
n1.second.ClearFlags(); n1.second.SetClean();
BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0);
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2); BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel); BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
// Adding 0 still has no effect // Adding DIRTY re-inserts it after n2
n1.second.AddFlags(0, n1, sentinel); CCoinsCacheEntry::SetDirty(n1, sentinel);
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
// But adding DIRTY re-inserts it after n2
n1.second.AddFlags(CCoinsCacheEntry::DIRTY, n1, sentinel);
BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY);
BOOST_CHECK_EQUAL(n2.second.Next(), &n1); BOOST_CHECK_EQUAL(n2.second.Next(), &n1);
BOOST_CHECK_EQUAL(n1.second.Prev(), &n2); BOOST_CHECK_EQUAL(n1.second.Prev(), &n2);

6
src/test/fuzz/coins_view.cpp
View file

@ -128,7 +128,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000) LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000)
{ {
CCoinsCacheEntry coins_cache_entry; CCoinsCacheEntry coins_cache_entry;
const auto flags{fuzzed_data_provider.ConsumeIntegral<uint8_t>()}; const auto dirty{fuzzed_data_provider.ConsumeBool()};
const auto fresh{fuzzed_data_provider.ConsumeBool()};
if (fuzzed_data_provider.ConsumeBool()) { if (fuzzed_data_provider.ConsumeBool()) {
coins_cache_entry.coin = random_coin; coins_cache_entry.coin = random_coin;
} else { } else {
@ -140,7 +141,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
coins_cache_entry.coin = *opt_coin; coins_cache_entry.coin = *opt_coin;
} }
auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first}; auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first};
it->second.AddFlags(flags, *it, sentinel); if (dirty) CCoinsCacheEntry::SetDirty(*it, sentinel);
if (fresh) CCoinsCacheEntry::SetFresh(*it, sentinel);
usage += it->second.coin.DynamicMemoryUsage(); usage += it->second.coin.DynamicMemoryUsage();
} }
bool expected_code_path = false; bool expected_code_path = false;