[WIP] Consensus: Don't allow a coin to be spent and FRESH.

jnewbery commented at 0:25 am on February 11, 2020: member

This change prevents coins from existing in a coins cache which are both FRESH and spent. FRESH coins should be removed from a cache when they are spent, and this PR also stops us from fetching spent coins from a parent cache and marking them FRESH.

fanquake added the label UTXO Db and Indexes on Feb 11, 2020

jnewbery force-pushed on Feb 11, 2020

DrahtBot commented at 5:48 am on February 11, 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:

#18410 (Docs: Improve commenting for coins.cpp|h by jnewbery)
#16910 (wallet: reduce loading time by using unordered maps by achow101)
#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.

jnewbery force-pushed on Feb 11, 2020

jnewbery commented at 4:38 pm on February 11, 2020: member

Fixed a whitespace oops that our linters didn’t like.

fanquake renamed this:
~~[coins] Don't allow a coin to spent and FRESH. Improve commenting.~~
coins: Don't allow a coin to be spent and FRESH. Improve commenting.
on Feb 12, 2020

jonatack commented at 7:31 pm on February 19, 2020: member

Concept ACK, will review (this was discussed in part in the meeting log of https://bitcoincore.reviews/17487.html)

in src/coins.h:128 in 9288d747e8 outdated

130+        DIRTY = (1 << 0),
131+        //! The parent cache does not have this coin (or it is spent in the parent cache).
132+        //! If a FRESH coin is spent in this cache, then it can be deleted entirely
133+        //! and doesn't ever need to be flushed to the parent. This is a performance
134+        //! optimization. Note that a coin cannot be both FRESH and empty.
135+        //! Marking a coin FRESH which is does exist in the parent view will

amitiuttarwar commented at 5:03 am on March 18, 2020:

grammar issue- “which is does exist” maybe- “which actually does exist”

also, I find this sentence a bit confusing, mostly because of the phrasing of “will” vs “might”. If a coin exists in the parent view & gets incorrectly marked as FRESH, is there a way it could still be deleted from the parent when the cache is flushed?

jkczyz commented at 5:21 am on March 18, 2020:

grammar issue- “which is does exist” maybe- “which actually does exist”

Or simply “exists”?

jnewbery commented at 1:39 pm on March 23, 2020:

fixed

in src/coins.cpp:92 in 9288d747e8 outdated

88+        // Re-adding a spent coin can happen in the case of a re-org (the coin
89+        // is 'spent' when the block containing it is disconnected and then
90+        // re-added if it also appears in the newly connected blocks). It can
91+        // also happen in the case of duplicate txids. In practice, BIP30 and
92+        // BIP34 ensure that the only duplicate transactions before block
93+        // 1,983,702 are the two pairs of coinbase txs at heights 91812/91842

pinheadmz commented at 3:16 pm on March 18, 2020:

Block 1,983,702 – about 26 years from now? Is that a timestamp rolling over or something? I thought BIP34 (block height in coinbase) will prevent this from ever happening since it was enforced:

Block number 227,835 (timestamp 2013-03-24 15:49:13 GMT) was the last version 1 block.

fjahr commented at 8:52 pm on March 18, 2020:

Take a look at the comments here: https://github.com/bitcoin/bitcoin/blob/a421e0a22f1230abd69b4661a019bed39b72205f/src/validation.cpp#L1989

pinheadmz commented at 10:02 pm on March 18, 2020:

Oh THAT large comment in connectBlock() thanks ;-)

in src/coins.cpp:76 in 78b81673b5 outdated

74@@ -75,8 +75,26 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
75     }
76     if (!possible_overwrite) {

fjahr commented at 3:18 pm on March 18, 2020:

I think you could clean up and clarify this parameter here as well. First of all, it is sometimes called possible_overwrite and sometimes potential_overwrite in the code base, making it harder to grep etc. And I think part of the comment below could be moved here or there could be an additional comment here to explain what this does. I still found it confusing at first after reading comments in coins.h and validation.cpp.

fjahr commented at 3:25 pm on March 18, 2020:

Or, I should rather say, the combination of the comments confused me more. From validation.cpp where AddCoin is called: “The potential_overwrite parameter to AddCoin is only allowed to be false if we know for sure that the coin did not already exist in the cache.” When it is false we end up in this code branch and your comment starts with “If the coin exists in this cache…”

in src/coins.h:122 in 9288d747e8 outdated

124-         */
125+        //! This cache entry is potentially different from the version in the parent cache.
126+        //! Failing to mark a coin as DIRTY when it is potentially different from the
127+        //! parent view will cause a consensus failure, since the coin's state won't get
128+        //! written to the parent when the cache is flushed.
129+        //! Note that all empty coins in the cache MUST be DIRTY.

pinheadmz commented at 3:27 pm on March 18, 2020:

What is an “empty” coin?

fjahr commented at 4:15 pm on March 18, 2020:

I think this references emptyCoin (coins.h:132) which calls an empty constructor of Coin (coins.h:53).

jkczyz commented at 4:52 pm on March 18, 2020:

I think this references emptyCoin (coins.h:132) which calls an empty contractor of Coin (coins.h:53).

I think this is suppose to say coinEmpty (coins.cpp:132).

jnewbery commented at 1:38 pm on March 23, 2020:

You’re right that this is unclear. Empty here means that the coin has been spent (see Coin.Clear() above which nulls out a coin when it is spent). I’ve changed this to “Note that a spent coin MUST be DIRTY”, which I think is much clearer.

ghost commented at 4:19 pm on March 18, 2020: none

Reading about it in details now. Not sure if I can suggest anything in next few hours because this looks more complex than I thought.

Will join the Bitcoin Core PR Review Club Meeting on IRC today to understand more and research later in the week : https://bitcoincore.reviews/18113.html

in src/coins.cpp:154 in 7053da36bf outdated

164-                // and already exist in the grandparent
165-                if (it->second.flags & CCoinsCacheEntry::FRESH) {
166-                    entry.flags |= CCoinsCacheEntry::FRESH;
167-                }
168+            // The parent cache does not have an entry, while the child does.
169+

fjahr commented at 4:20 pm on March 18, 2020:

nit: empty line could be removed

fjahr commented at 4:23 pm on March 18, 2020: member

Concept ACK

Code looks good but I still need to think about reorgs a little more.

jnewbery added the label Review club on Mar 18, 2020

meshcollider commented at 7:24 pm on March 18, 2020: contributor

Code review ACK 9288d747e849b126930ef9e2f623ce6746555ed4

Agree the terminology between FRESH and DIRTY is a bit confusing as people brought up in the PR review meeting this morning.

fjahr commented at 8:54 pm on March 18, 2020: member

Code review ACK 9288d747e849b126930ef9e2f623ce6746555ed4

jonatack commented at 10:24 pm on March 18, 2020: member

ACK 9288d747e849b126930ef9e2f623ce6746555ed4 modulo terminology and timing

I think “FRESH” and “DIRTY” generally imply mutually exclusive states to people, and non-intuitive meanings/behavior could cause issues in the future. Happy to re-review/re-ACK any proposed further improvements.
As this touches consensus, perhaps best to merge after the impending 0.20 branch-off to give the changes some time.

jkczyz commented at 10:36 pm on March 18, 2020: contributor

Agree the terminology between FRESH and DIRTY is a bit confusing as people brought up in the PR review meeting this morning.

Given that there are four valid states but eight possible combinations (see @amitiuttarwar’s summarization), one option is to refactor the code to use an enum with four variants named appropriately. This removes the possibility of getting into an invalid states. However, I’m not sure how practical this refactor would be or how difficult it would be to come up with meaningful names.

fjahr commented at 0:12 am on March 19, 2020: member

Given that there are four valid states but eight possible combinations (see @amitiuttarwar’s summarization), one option is to refactor the code to use an enum with four variants named appropriately. This removes the possibility of getting into an invalid states. However, I’m not sure how practical this refactor would be or how difficult it would be to come up with meaningful names.

interesting idea but then I think renaming the flags to NEWCOIN and MUSTFLUSH as suggest by @jnewbery is a more practical change.

andrewtoth commented at 4:39 am on March 19, 2020: contributor

Concept ACK

Reiterating my comments from the PR review club, I think this PR might benefit from being split up. It’s attempting two separate things, one of which is a consensus change. I think review on that important change might be more focused if this was split into a name and comment update, and then just the logic change.

jnewbery force-pushed on Mar 23, 2020

jnewbery renamed this:
~~coins: Don't allow a coin to be spent and FRESH. Improve commenting.~~
[WIP] Don't allow a coin to be spent and FRESH.
on Mar 23, 2020

jnewbery commented at 3:36 pm on March 23, 2020: member

I’ve moved all the comment changes to ~~#18113~~ #18410, which this PR now builds off. Please review #18113 first.

jnewbery renamed this:
~~[WIP] Don't allow a coin to be spent and FRESH.~~
[WIP] Consensus: Don't allow a coin to be spent and FRESH.
on Mar 23, 2020

jnewbery force-pushed on Mar 24, 2020

DrahtBot commented at 2:13 pm on April 22, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Apr 22, 2020

[consensus] Don't add spent coins to the cache

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

c9849da148

jnewbery force-pushed on Apr 22, 2020

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

This PR originally included changes to comments and the change to not fetch spend coins from the cache. The comments changes have been merged in ~~#18113~~ #18410. Most of the review comments here are about those comments changes, so I’m closing this and using #18746 to PR the code changes.

jnewbery closed this on Apr 22, 2020

jnewbery deleted the branch on Apr 22, 2020

MarcoFalke commented at 2:57 pm on May 24, 2020: member

I think there is a typo in the last two comments 18113 should be 18410. #18410

jnewbery commented at 9:14 pm on May 27, 2020: member

Thanks Marco. I fixed those comments.

DrahtBot locked this on Feb 15, 2022

[WIP] Consensus: Don’t allow a coin to be spent and FRESH. #18113

Conflicts