consensus: Don't add spent coins to the cache

jnewbery commented at 10:28 pm on April 22, 2020: member

When fetching a coin from a parent cache, don’t add it to the child cache if it’s already spent, since there’s no benefit to adding it.

This adds some invariant properties to the coins cache:

a spent coin cannot be FRESH
a spent coin must be DIRTY

fanquake added the label Consensus on Apr 22, 2020

DrahtBot commented at 10:02 am on April 23, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17487 (coins: allow write to disk without cache drop by jamesob)
#9384 (CCoinsViewCache code cleanup & deduplication by ryanofsky)

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.

promag commented at 2:51 pm on April 26, 2020: member

Concept ACK, and code change looks good. No strong background in coins management to ACK though.

brakmic commented at 10:22 am on May 1, 2020: contributor

New logic makes sense. Concept ACK (as I have no deep knowledge on coin management).

in src/coins.cpp:49 in c9849da148 outdated

43@@ -44,14 +44,12 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const
44     if (it != cacheCoins.end())
45         return it;
46     Coin tmp;
47-    if (!base->GetCoin(outpoint, tmp))
48+    if (!base->GetCoin(outpoint, tmp) || tmp.IsSpent()) {
49+        // If we find a spent coin in the parent cache, no
50+        // need to fetch it into this cache.

Sjors commented at 5:13 pm on May 8, 2020:

“no need to put it into this cache” (we already fetched it)

fjahr commented at 8:16 pm on May 25, 2020:

The whole function we are in here is called FetchCoin so I would understand “fetching” as everything that is going on in his function and not the fetch into a temp variable, so for me this is ok.

in src/coins.cpp:176 in c9849da148 outdated

178-                if (it->second.flags & CCoinsCacheEntry::FRESH) {
179-                    entry.flags |= CCoinsCacheEntry::FRESH;
180-                }
181+            if (it->second.flags & CCoinsCacheEntry::FRESH && it->second.coin.IsSpent()) {
182+                // A coin shouldn't be marked FRESH and spent (when a FRESH coin is spent,
183+                // it should be removed from the cache).

Sjors commented at 5:34 pm on May 8, 2020:

Maybe add: “A spent coin should never be added to the cache.”

Sjors commented at 5:57 pm on May 8, 2020: member

Concept ACK. c9849da1485458824851e56995b5470288d0372a looks correct.

This may be a good time to document FetchCoin() in the header.

Maybe GetCoin(), which calls FetchCoin(), can be made more clear:

0bool CCoinsViewCache::GetCoin(const COutPoint &outpoint, Coin &coin) const {
1    CCoinsMap::const_iterator it = FetchCoin(outpoint);
2    // If coin was not found in cache, or if it was not cached but already spent:
3    if (it == cacheCoins.end()) return false;
4    coin = it->second.coin;
5    return !coin.IsSpent();
6}

SpendCoin() also uses this early return pattern (could use the same comment).

jamesob commented at 6:57 pm on May 19, 2020: member

What’s the rationale for changing this? Even if it’s stylistically cleaner, I don’t think it’s worth changing semantics here unless there’s a demonstrable performance benefit.

jnewbery commented at 7:35 pm on May 19, 2020: member

What’s the rationale for changing this?

There’s an inconsistency between how we treat spending a fresh coin in the cache (erase it ie don’t allow the coin to be spent and FRESH), and what we do when we encounter a spent coin in the parent cache (add it to the cache as spent and FRESH). This change removes that inconsistency and adds the invariant that no coin can be spent and FRESH in any cache.

There’s unlikely to be any noticeable performance change here. After all, it’s very rare that we’ll fetch a spent coin from a parent cache (which I’d argue is all the more reason to remove the special case code for that).

I think this is an improvement to clarity, but you may be right that it’s not worth changing.

fjahr approved

fjahr commented at 8:30 pm on May 25, 2020: member

Code review ACK c9849da1485458824851e56995b5470288d0372a

I like changes that clarify the code or make it more consistent. It is totally understandable that there is a bias against changes that have seemingly low impact but create a high review burden because they touch consensus code. I tend to focus on the compounding value of these changes for the far future, making it easier to understand the code for those studying it and making it less likely to introduce bugs when the code is changed again. Still, it needs to be evaluated on a case-by-case basis but in this case, the trade-off seems worth it for me :)

Sjors commented at 8:46 am on May 28, 2020: member

I think this is an improvement to clarity, but you may be right that it’s not worth changing.

I think it is, because we’re changing this code for UTXO snapshots. The increased clarity should make those reviews (slightly) easier, something I bumped into myself: #17487 (review)

jamesob commented at 2:15 pm on May 28, 2020: member

Touching the heart of consensus-critical code and modifying behavior therein, even if just for an unlikely corner case, should IMHO come with very substantial motivation.

This is right on the knife’s edge. I agree that it’s unlikely this change in behavior would change consensus, but it sets a precedent that I’m not totally comfortable with.

To be honest I’d rather abandon the UTXO snapshot-related optimization changes (https://github.com/bitcoin/bitcoin/pull/17487) than invite behavioral changes to coins cache solely on the basis of readability or clarity.

jnewbery commented at 3:11 pm on May 28, 2020: member

There aren’t any dependencies between this and #17487, so I’m not sure that discussion of that PR is relevant here. I do however think it’d be a shame to abandon that PR. It’s received substantial review from >10 contributors and the general reception was positive. In general, the decision to proceed with or abandon a PR should be based on the merit of that PR alone.

robot-dreams commented at 0:13 am on October 17, 2020: contributor

Is the FetchCoin change covered by any testing here? I think it might not be, given that:

CCoinsViewCache::GetCoin already returns false if the coin was spent: https://github.com/bitcoin/bitcoin/blob/2f71a1ea35667b3873197201531e7ae198ec5bf4/src/coins.cpp#L61-L64
SingleEntryCacheTest in coins_test.cpp uses a (subclass of) CCoinsViewCache for both the parent and the child: https://github.com/bitcoin/bitcoin/blob/2f71a1ea35667b3873197201531e7ae198ec5bf4/src/test/coins_tests.cpp#L627-L628

This test case existed and passed even before your change:

                  Base    Cache   Result  Cache        Result
                  Value   Value   Value   Flags        Flags
  CheckAccessCoin(SPENT , ABSENT, ABSENT, NO_ENTRY   , NO_ENTRY   );

jnewbery renamed this:
~~[consensus] Don't add spent coins to the cache~~
consensus: Don't add spent coins to the cache
on Oct 23, 2020

jnewbery force-pushed on May 17, 2021

[tests] Always return false from CCoinsViewTest.GetCoin() if the coin is spent.

CCoinsViewTest.GetCoins() was added in ed27e53c9, when a per-tx model
was used.  PR 10195 changed our model to be per-txout, so
CCoinsViewTest.GetCoins() was changed to GetCoin(). GetCoins() would
randomly return false if the tx had been pruned from the coins view. In
the per-txout model, the real implementation of coins view will always
return false if the coin IsSpent(), so it doesn't make sense for
CCoinsViewTest.GetCoin() to sometimes return true.

e5a688dbde

[consensus] Clarify that spent coins are not added to the cache.

When fetching a coin from a parent cache, it doesn't get added to the
child cache if it's already spent, since there's no benefit to adding
it.

Clarify the invariant properties of the coins cache:
- a spent coin cannot be FRESH
- a spent coin must be DIRTY

b5f17602b3

jnewbery force-pushed on May 17, 2021

jnewbery commented at 2:56 pm on May 17, 2021: member

Thanks @robot-dreams. Excellent observation!

I’ve updated CCoinsViewCache::FetchCoin() to assert that the coin returned from GetCoin() is not spent.

I’ve also had to add a commit that changes the coins_tests unit test, since CCoinsViewTest::GetCoin() would sometimes return true even if the coin returned was empty (IsSpent() == true).

I’m fairly confident that this is a good change, and I very much like adding invariants that remove the possibility of the program being in an inconsistent state. However, I’m going to close this PR for now since it hasn’t received much attention over the year it’s been open. We can always re-open it later if there’s demand to clean up the coins interface. I also think if we were redesigning these interfaces now, we’d probably return a std::optional<> type to avoid having to return a bool and have an in/out param, and also to avoid having a special coinEmpty marker.

jnewbery closed this on May 17, 2021

michaelfolkson commented at 4:53 pm on May 17, 2021: contributor

For the benefit of anyone who does revisit this closed PR at a later date (including perhaps myself), this PR was spun out of #18113 (the comments part was merged in #18410 and the consensus code changes were in this PR).

#18113 was covered in this Bitcoin Core PR review club and includes a handy explanation of the validity of possible states (combinations of spent/unspent, fresh/not fresh, dirty/not dirty).

DrahtBot locked this on Aug 18, 2022

consensus: Don’t add spent coins to the cache #18746

Conflicts