-
Notifications
You must be signed in to change notification settings - Fork 38.7k
coins: remove SetFresh method from CCoinsCacheEntry #33018
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/33018. 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. |
ed4a961 to
8eb06ad
Compare
l0rinc
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
Very glad this is getting cleanup up, I found it worrying that a lot of tests were exercising invalid states in the first place - not the best foundation.
Quickly went over the changes, I find the small commits and excellent commit messages easy to follow.
I'll run a full IBD over this (maybe with some extra SanityCheck sprinkled around for extra measure) to make sure real usage doesn't trigger any invalid states we're not aware of.
| BOOST_CHECK(n2.second.IsFresh() && !n2.second.IsDirty()); | ||
| // Check that setting DIRTY and FRESH on new node inserts it after n1 | ||
| CCoinsCacheEntry::SetDirty(n2, sentinel, /*fresh=*/true); | ||
| BOOST_CHECK(n2.second.IsFresh() && n2.second.IsDirty()); |
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.
nit: for consistency with other SetDirty calls we could swap the parameter order:
| BOOST_CHECK(n2.second.IsFresh() && n2.second.IsDirty()); | |
| BOOST_CHECK(n2.second.IsDirty() && n2.second.IsFresh()); |
Alternatively we could rename IsFresh to IsDirtyAndFresh and assert that if fresh is true, so is dirty - which would simplify this line to asserting just a single method.
8eb06ad to
95f0ff0
Compare
This comment was marked as spam.
This comment was marked as spam.
95f0ff0 to
99b243d
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. |
99b243d to
98698b4
Compare
sedited
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
4564e57 to
8cc8458
Compare
This is already not possible for any production GetCoin implementation, so this aligns tests with production code.
This is the only place in production code where SetFresh is called when not preceded by SetDirty. We can see that coin is retrieved from base->GetCoin, and base can only be another CCoinsViewCache or CCoinsViewDB. CCoinsViewCache::GetCoin does not return spent coins, and CCoinsViewDB does not store any spent coins to return. Thus, we can conclude that this call to SetFresh is dead code and can be safely removed.
The FRESH-but-not-DIRTY state cannot occur in production code. There are never any calls to SetFresh not preceded by SetDirty. We should remove these test cases to simplify the test code and better align it with production code.
8cc8458 to
e9d793d
Compare
|
Rebased due to #34095 modifying code that is deleted in this change. |
|
|
||
| static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(DIRTY, pair, sentinel); } | ||
| static void SetFresh(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(FRESH, pair, sentinel); } | ||
| static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel, bool fresh = false) noexcept |
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 think it's important to add a docstring to this function. Would also be good to explain what happens if a non-fresh entry is marked fresh by accident (and vice-versa), similar to the previous brief explainer for the FRESH enum variant (don't have a good suggestion yet though).
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 working on a PR to reduce the doubly-linked list to a single one - it will likely change these methods anyway.
If I finish it in time, maybe I can provide feedback about that here...
But let's also step back and notice that we're trying to add a comment for a method that we concede is hard to understand. Can we refactor instead of admitting defeat and commenting?
I will review this in detail to see if I have a better idea.
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.
Added a docstring to this function and to SetClean().
e9d793d to
65e673c
Compare
There are no instances where SetFresh is called without being preceded by SetDirty. This replaces SetFresh with a boolean flag passed to SetDirty to indicate whether the DIRTY coin should also be set to FRESH. This removes the possibility of setting a FRESH-but-not-DIRTY cached coin state.
There is only one callsite now for AddFlags, so we can inline it into SetDirty.
A cache entry is now always DIRTY if it is in the linked list. We can use linked list membership instead of a flag to determine dirtiness. Thus, the only state we need to track is if an entry is FRESH. This can be represented by a simple bool instead of a bitfield.
65e673c to
ca03517
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. |
l0rinc
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.
Re-reviewed and extracted a subset of the changes that aren't strictly related to freshness to #34207
Left a few comments about leftover commit messages and comments or symbols still referencing the old flags
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.
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.
c15e0bc
this commit fixes two TODOs, we could push it as a separate PR!
We can even merge it with the sanity check commit - which should go here, since this commit enabled it.
| unsigned attr = 0; | ||
| if (entry.IsDirty()) attr |= 1; | ||
| if (entry.IsFresh()) attr |= 2; | ||
| if (entry.coin.IsSpent()) attr |= 4; | ||
| // Only 5 combinations are possible. | ||
| assert(attr != 2 && attr != 4 && attr != 7); | ||
| // Only 4 combinations are possible. | ||
| assert(attr != 2 && attr != 4 && attr != 6 && attr != 7); |
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 getting simpler to list what we do support now, doesn't it?
unsigned attr = 0;
if (entry.IsDirty()) attr |= 1;
if (entry.IsFresh()) attr |= 2;
if (entry.coin.IsSpent()) attr |= 4;
assert(attr == 0 /*unspent*/ ||
attr == 1 /*unspent+DIRTY*/ ||
attr == 3 /*unspent+DIRTY+FRESH*/ ||
attr == 5 /*spent+DIRTY*/);So basically assert not spent, but if it is, it should be dirty. If not spent, it's not dirty, but if it is, it should be fresh as well, right?
| assert(attr != 2 && attr != 4 && attr != 6 && attr != 7); | |
| assert(!entry.coin.IsSpent() || entry.IsDirty()); | |
| assert(!entry.IsFresh() || (entry.IsDirty() && !entry.coin.IsSpent())); |
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.
That's a commit from the previous PR bcf4ec0.
| void CCoinsViewCache::SanityCheck() const | ||
| { | ||
| size_t recomputed_usage = 0; | ||
| size_t count_flagged = 0; |
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.
They're not flags anymore, we should update a few other places:
diff --git a/src/coins.cpp b/src/coins.cpp
index dac9540b1d..39ad74daa6 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -264,8 +264,8 @@ void CCoinsViewCache::Sync()
auto cursor{CoinsViewCacheCursor(m_sentinel, cacheCoins, /*will_erase=*/false)};
base->BatchWrite(cursor, hashBlock);
if (m_sentinel.second.Next() != &m_sentinel) {
- /* BatchWrite must clear flags of all entries */
- throw std::logic_error("Not all unspent flagged entries were cleared");
+ /* BatchWrite must clear dirty state of all entries */
+ throw std::logic_error("Not all unspent dirty entries were cleared");
}
}
@@ -313,7 +313,7 @@ void CCoinsViewCache::ReallocateCache()
void CCoinsViewCache::SanityCheck() const
{
size_t recomputed_usage = 0;
- size_t count_flagged = 0;
+ size_t count_dirty = 0;
for (const auto& [_, entry] : cacheCoins) {
unsigned attr = 0;
if (entry.IsDirty()) attr |= 1;
@@ -328,18 +328,18 @@ void CCoinsViewCache::SanityCheck() const
// Count the number of entries we expect in the linked list.
if (entry.IsDirty() || entry.IsFresh()) ++count_flagged;
}
- // Iterate over the linked list of flagged entries.
+ // Iterate over the linked list of dirty entries.
size_t count_linked = 0;
for (auto it = m_sentinel.second.Next(); it != &m_sentinel; it = it->second.Next()) {
// Verify linked list integrity.
assert(it->second.Next()->second.Prev() == it);
assert(it->second.Prev()->second.Next() == it);
- // Verify they are actually flagged.
+ // Verify they are actually dirty.
assert(it->second.IsDirty() || it->second.IsFresh());
// Count the number of entries actually in the list.
++count_linked;
}
- assert(count_linked == count_flagged);
+ assert(count_linked == count_dirty);
assert(recomputed_usage == cachedCoinsUsage);
}
diff --git a/src/coins.h b/src/coins.h
index bc291f1b68..99efa70eec 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -252,11 +252,11 @@ private:
};
/**
- * Cursor for iterating over the linked list of flagged entries in CCoinsViewCache.
+ * Cursor for iterating over the linked list of dirty entries in CCoinsViewCache.
*
* This is a helper struct to encapsulate the diverging logic between a non-erasing
* CCoinsViewCache::Sync and an erasing CCoinsViewCache::Flush. This allows the receiver
- * of CCoinsView::BatchWrite to iterate through the flagged entries without knowing
+ * of CCoinsView::BatchWrite to iterate through the dirty entries without knowing
* the caller's intent.
*
* However, the receiver can still call CoinsViewCacheCursor::WillErase to see if the
@@ -267,7 +267,7 @@ private:
struct CoinsViewCacheCursor
{
//! If will_erase is not set, iterating through the cursor will erase spent coins from the map,
- //! and other coins will be unflagged (removing them from the linked list).
+ //! and other coins will be unmarked dirty (removing them from the linked list).
//! If will_erase is set, the underlying map and linked list will not be modified,
//! as the caller is expected to wipe the entire map anyway.
//! This is an optimization compared to erasing all entries as the cursor iterates them when will_erase is set.
@@ -371,7 +371,7 @@ protected:
*/
mutable uint256 hashBlock;
mutable CCoinsMapMemoryResource m_cache_coins_memory_resource{};
- /* The starting sentinel of the flagged entry circular doubly linked list. */
+ /* The starting sentinel of the dirty entry circular doubly linked list. */
mutable CoinsCachePair m_sentinel;
mutable CCoinsMap cacheCoins;
| // The parent only has an empty entry for this outpoint; we can consider our version as fresh. | ||
| CCoinsCacheEntry::SetFresh(*ret, m_sentinel); | ||
| } | ||
| Assume(!ret->second.coin.IsSpent()); |
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.
There are a few leftover spentness checks:
diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp
--- a/src/test/fuzz/coinscache_sim.cpp (revision e34fdb04462b88efdaa116c099c1aeed1022cdb5)
+++ b/src/test/fuzz/coinscache_sim.cpp (date 1767696554322)
@@ -173,7 +173,7 @@
/* For non-dirty entries being written, compare them with what we have. */
auto it2 = m_data.find(it->first);
if (it->second.coin.IsSpent()) {
- assert(it2 == m_data.end() || it2->second.IsSpent());
+ assert(it2 == m_data.end());
} else {
assert(it2 != m_data.end());
assert(it->second.coin.out == it2->second.out);
@@ -258,7 +258,7 @@
auto realcoin = caches.back()->GetCoin(data.outpoints[outpointidx]);
// Compare results.
if (!sim.has_value()) {
- assert(!realcoin || realcoin->IsSpent());
+ assert(!realcoin);
} else {
assert(realcoin && !realcoin->IsSpent());
const auto& simcoin = data.coins[sim->first];
@@ -444,7 +444,7 @@
auto realcoin = bottom.GetCoin(data.outpoints[outpointidx]);
auto sim = lookup(outpointidx, 0);
if (!sim.has_value()) {
- assert(!realcoin || realcoin->IsSpent());
+ assert(!realcoin);
} else {
assert(realcoin && !realcoin->IsSpent());
assert(realcoin->out == data.coins[sim->first].out);
Index: src/coins.cpp
diff --git a/src/coins.cpp b/src/coins.cpp
--- a/src/coins.cpp (revision e34fdb04462b88efdaa116c099c1aeed1022cdb5)
+++ b/src/coins.cpp (date 1767696597975)
@@ -272,7 +272,7 @@
void CCoinsViewCache::Uncache(const COutPoint& hash)
{
CCoinsMap::iterator it = cacheCoins.find(hash);
- if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) {
+ if (it != cacheCoins.end() && !it->second.IsDirty()) {
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
TRACEPOINT(utxocache, uncache,
hash.hash.data(),
@@ -326,7 +326,7 @@
recomputed_usage += entry.coin.DynamicMemoryUsage();
// Count the number of entries we expect in the linked list.
- if (entry.IsDirty() || entry.IsFresh()) ++count_flagged;
+ if (entry.IsDirty()) ++count_flagged;
}
// Iterate over the linked list of dirty entries.
size_t count_linked = 0;
@@ -335,7 +335,7 @@
assert(it->second.Next()->second.Prev() == it);
assert(it->second.Prev()->second.Next() == it);
// Verify they are actually dirty.
- assert(it->second.IsDirty() || it->second.IsFresh());
+ assert(it->second.IsDirty());
// Count the number of entries actually in the list.
++count_linked;
}
There is plenty of test code that exercises the code path for FRESH-but-not-DIRTY coins in CCoinsViewCache. However, we can see there is only one place in production code where we call
SetFreshthat is not preceded bySetDirty. This is inCCoinsViewCache::FetchCoinand we can see that this is called if the coin retrieved frombase->GetCoinis spent. Thebasein this case can be anotherCCoinsViewCacheor aCCoinsViewDB. InCCoinsViewCache::GetCoinwe can see that we do not return spent coins, and inCCoinsViewDBwe don't ever store spent coins so there are none to return.Thus, we can safely remove this dead code, and now we can see that there are never any calls to
SetFreshnot preceded bySetDirty. This PR removes this dead code and replacesSetFreshby passing afreshboolean flag toSetDirty. This simplifies the logic ofCCoinsViewCacheand lets us remove test cases checking for FRESH-but-not-DIRTY coins. It removes the possibility of FRESH-but-not-DIRTY coins being called from any code path, so we can eliminate this state entirely and reduce the cognitive load of having to consider it.This is pulled out from #30673 to make the change simpler.