Skip to content

Commit

Permalink
Merge bitcoin/bitcoin#30906: refactor: prohibit direct flags access i…
Browse files Browse the repository at this point in the history 
…n CCoinsCacheEntry and remove invalid tests

50cce20 test, refactor: Compact ccoins_access and ccoins_spend (Lőrinc)
0a159f0 test, refactor: Remove remaining unbounded flags from coins_tests (Lőrinc)
c0b4b2c test: Validate error messages on fail (Lőrinc)
d5f8d60 test: Group values and states in tests into CoinEntry wrappers (Lőrinc)
ca74aa7 test, refactor: Migrate GetCoinsMapEntry to return MaybeCoin (Lőrinc)
15aaa81 coins, refactor: Remove direct GetFlags access (Lőrinc)
6b73369 coins, refactor: Assume state after SetClean in AddFlags to prevent dangling pointers (Lőrinc)
fc8c282 coins, refactor: Make AddFlags, SetDirty, SetFresh static (Lőrinc)
cd0498e coins, refactor: Split up AddFlags to remove invalid states (Lőrinc)

Pull request description:

  Similarly to bitcoin/bitcoin#30849, this cleanup is intended to de-risk bitcoin/bitcoin#30673 (comment) by simplifying the coin cache public interface.

  `CCoinsCacheEntry` provided general access to its internal flags state, even though, in reality, it could only be `clean`, `fresh`, `dirty`, or `fresh|dirty` (in the follow-up, we will remove `fresh` without `dirty`).

  Once it was marked as `dirty`, we couldn’t set the state back to clean with `AddFlags(0)`—tests explicitly checked against that.

  This PR refines the public interface to make this distinction clearer and to make invalid behavior impossible, rather than just checked by tests. We don't need extensive access to the internals of `CCoinsCacheEntry`, as many tests were simply validating invalid combinations in this way.

  The last few commits contain significant test refactorings to make `coins_tests` easier to change in follow-ups.

ACKs for top commit:
  andrewtoth:
    Code Review ACK 50cce20
  laanwj:
    Code review ACK 50cce20
  ryanofsky:
    Code review ACK 50cce20. Looks good! Thanks for the followups.

Tree-SHA512: c0d65f1c7680b4bb9cd368422b218f2473c2ec75a32c7350a6e11e8a1601c81d3c0ae651b9f1dae08400fb4e5d43431d9e4ccca305a718183f9a936fe47c1a6c
  • Loading branch information
@ryanofsky
ryanofsky committed Dec 4, 2024
2 parents 11f68cc + 50cce20 commit 17372d7
Show file tree
Hide file tree
Showing 5 changed files with 300 additions and 366 deletions.
24 changes: 9 additions & 15 deletions src/coins.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const
cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
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.
ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel);
CCoinsCacheEntry::SetFresh(*ret, m_sentinel);
}
} else {
cacheCoins.erase(ret);
Expand Down Expand Up @@ -99,7 +99,8 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
fresh = !it->second.IsDirty();
}
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();
TRACEPOINT(utxocache, add,
outpoint.hash.data(),
Expand All @@ -111,13 +112,8 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi

void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) {
cachedCoinsUsage += coin.DynamicMemoryUsage();
auto [it, inserted] = cacheCoins.emplace(
std::piecewise_construct,
std::forward_as_tuple(std::move(outpoint)),
std::forward_as_tuple(std::move(coin)));
if (inserted) {
it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel);
}
auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin));
if (inserted) CCoinsCacheEntry::SetDirty(*it, m_sentinel);
}

void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) {
Expand Down Expand Up @@ -147,7 +143,7 @@ bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) {
if (it->second.IsFresh()) {
cacheCoins.erase(it);
} else {
it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel);
CCoinsCacheEntry::SetDirty(*it, m_sentinel);
it->second.coin.Clear();
}
return true;
Expand Down Expand Up @@ -207,13 +203,11 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
entry.coin = it->second.coin;
}
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
// Otherwise it might have just been flushed from the parent's cache
// and already exist in the grandparent
if (it->second.IsFresh()) {
entry.AddFlags(CCoinsCacheEntry::FRESH, *itUs, m_sentinel);
}
if (it->second.IsFresh()) CCoinsCacheEntry::SetFresh(*itUs, m_sentinel);
}
} else {
// Found the entry in the parent cache
Expand Down Expand Up @@ -241,7 +235,7 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
itUs->second.coin = it->second.coin;
}
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
// cache. If it already existed and was spent in the parent
// cache then marking it FRESH would prevent that spentness
Expand Down
62 changes: 34 additions & 28 deletions src/coins.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ struct CCoinsCacheEntry
private:
/**
* 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.
*
* DIRTY entries are tracked so that only modified entries can be passed to
Expand All @@ -128,6 +128,21 @@ struct CCoinsCacheEntry
CoinsCachePair* m_next{nullptr};
uint8_t m_flags{0};

//! Adding a flag requires a reference to the sentinel of the flagged pair linked list.
static void AddFlags(uint8_t flags, CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept
{
Assume(flags & (DIRTY | FRESH));
if (!pair.second.m_flags) {
Assume(!pair.second.m_prev && !pair.second.m_next);
pair.second.m_prev = sentinel.second.m_prev;
pair.second.m_next = &sentinel;
sentinel.second.m_prev = &pair;
pair.second.m_prev->second.m_next = &pair;
}
Assume(pair.second.m_prev && pair.second.m_next);
pair.second.m_flags |= flags;
}

public:
Coin coin; // The actual cached data.

Expand Down Expand Up @@ -156,52 +171,43 @@ struct CCoinsCacheEntry
explicit CCoinsCacheEntry(Coin&& coin_) noexcept : coin(std::move(coin_)) {}
~CCoinsCacheEntry()
{
ClearFlags();
SetClean();
}

//! 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
//! flagged pair linked list.
inline void AddFlags(uint8_t flags, CoinsCachePair& self, CoinsCachePair& sentinel) noexcept
{
Assume(&self.second == this);
if (!m_flags && flags) {
m_prev = sentinel.second.m_prev;
m_next = &sentinel;
sentinel.second.m_prev = &self;
m_prev->second.m_next = &self;
}
m_flags |= flags;
}
inline void ClearFlags() noexcept
static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(DIRTY, pair, sentinel); }
static void SetFresh(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(FRESH, pair, sentinel); }

void SetClean() noexcept
{
if (!m_flags) return;
m_next->second.m_prev = m_prev;
m_prev->second.m_next = m_next;
m_flags = 0;
m_prev = m_next = nullptr;
}
inline uint8_t GetFlags() const noexcept { return m_flags; }
inline bool IsDirty() const noexcept { return m_flags & DIRTY; }
inline bool IsFresh() const noexcept { return m_flags & FRESH; }
bool IsDirty() const noexcept { return m_flags & DIRTY; }
bool IsFresh() const noexcept { return m_flags & FRESH; }

//! Only call Next when this entry is DIRTY, FRESH, or both
inline CoinsCachePair* Next() const noexcept {
CoinsCachePair* Next() const noexcept
{
Assume(m_flags);
return m_next;
}

//! Only call Prev when this entry is DIRTY, FRESH, or both
inline CoinsCachePair* Prev() const noexcept {
CoinsCachePair* Prev() const noexcept
{
Assume(m_flags);
return m_prev;
}

//! Only use this for initializing the linked list sentinel
inline void SelfRef(CoinsCachePair& self) noexcept
void SelfRef(CoinsCachePair& pair) noexcept
{
Assume(&self.second == this);
m_prev = &self;
m_next = &self;
Assume(&pair.second == this);
m_prev = &pair;
m_next = &pair;
// Set sentinel to DIRTY so we can call Next on it
m_flags = DIRTY;
}
Expand Down Expand Up @@ -279,13 +285,13 @@ struct CoinsViewCacheCursor
{
const auto next_entry{current.second.Next()};
// 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 (current.second.coin.IsSpent()) {
m_usage -= current.second.coin.DynamicMemoryUsage();
m_map.erase(current.first);
} else {
current.second.ClearFlags();
current.second.SetClean();
}
}
return next_entry;
Expand Down
Loading

0 comments on commit 17372d7

Please sign in to comment.