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

jnewbery · 2020-02-11T00:25:12Z

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.

DrahtBot · 2020-02-11T05:48:45Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Docs: Improve commenting for coins.cpp|h #18410 (Docs: Improve commenting for coins.cpp|h by jnewbery)
wallet: reduce loading time by using unordered maps #16910 (wallet: reduce loading time by using unordered maps by achow101)
CCoinsViewCache code cleanup & deduplication #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 · 2020-02-11T16:38:12Z

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

jonatack · 2020-02-19T19:31:11Z

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

amitiuttarwar · 2020-03-18T05:03:26Z

src/coins.h

+        //! If a FRESH coin is spent in this cache, then it can be deleted entirely
+        //! and doesn't ever need to be flushed to the parent. This is a performance
+        //! optimization. Note that a coin cannot be both FRESH and empty.
+        //! Marking a coin FRESH which is does exist in the parent view will


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?

grammar issue- "which is does exist"
maybe- "which actually does exist"

Or simply "exists"?

pinheadmz · 2020-03-18T15:16:25Z

src/coins.cpp

+        // re-added if it also appears in the newly connected blocks). It can
+        // also happen in the case of duplicate txids. In practice, BIP30 and
+        // BIP34 ensure that the only duplicate transactions before block
+        // 1,983,702 are the two pairs of coinbase txs at heights 91812/91842


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.

Take a look at the comments here:

bitcoin/src/validation.cpp

Line 1989 in a421e0a

// Once BIP34 activated it was not possible to create new duplicate coinbases and thus other than starting

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

pinheadmz · 2020-03-18T15:27:47Z

src/coins.h

+        //! Failing to mark a coin as DIRTY when it is potentially different from the
+        //! parent view will cause a consensus failure, since the coin's state won't get
+        //! written to the parent when the cache is flushed.
+        //! Note that all empty coins in the cache MUST be DIRTY.


What is an "empty" coin?

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

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).

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 · 2020-03-18T16:19:51Z

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

fjahr

Concept ACK

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

fjahr · 2020-03-18T15:18:52Z

src/coins.cpp

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


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.

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..."

fjahr · 2020-03-18T16:15:34Z

src/coins.h

+        //! Failing to mark a coin as DIRTY when it is potentially different from the
+        //! parent view will cause a consensus failure, since the coin's state won't get
+        //! written to the parent when the cache is flushed.
+        //! Note that all empty coins in the cache MUST be DIRTY.


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

fjahr · 2020-03-18T16:20:58Z

src/coins.cpp

-                    entry.flags |= CCoinsCacheEntry::FRESH;
-                }
+            // The parent cache does not have an entry, while the child does.
+


nit: empty line could be removed

meshcollider · 2020-03-18T19:24:58Z

Code review ACK 9288d74

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

fjahr · 2020-03-18T20:54:20Z

Code review ACK 9288d74

jonatack

ACK 9288d74 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 · 2020-03-18T22:36:57Z

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 · 2020-03-19T00:12:00Z

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 · 2020-03-19T04:39:04Z

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 · 2020-03-23T15:36:55Z

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

DrahtBot · 2020-04-22T14:13:13Z

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

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

jnewbery · 2020-04-22T22:30:16Z

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.

maflcko · 2020-05-24T14:57:00Z

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

jnewbery · 2020-05-27T21:14:57Z

Thanks Marco. I fixed those comments.

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

jnewbery force-pushed the 2020-01-prune-pruned branch from f95e620 to 5150e82 Compare February 11, 2020 00:29

jnewbery force-pushed the 2020-01-prune-pruned branch from 5150e82 to 9288d74 Compare February 11, 2020 16:32

This was referenced Feb 11, 2020

coins: allow write to disk without cache drop #17487

Merged

CCoinsViewCache code cleanup & deduplication #9384

Closed

fanquake changed the title ~~[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. Feb 12, 2020

amitiuttarwar reviewed Mar 18, 2020

View reviewed changes

pinheadmz reviewed Mar 18, 2020

View reviewed changes

fjahr reviewed Mar 18, 2020

View reviewed changes

jnewbery added the Review club label Mar 18, 2020

jonatack reviewed Mar 18, 2020

View reviewed changes

jnewbery force-pushed the 2020-01-prune-pruned branch from 9288d74 to 2ba4885 Compare March 23, 2020 15:35

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

jnewbery mentioned this pull request Mar 23, 2020

Docs: Improve commenting for coins.cpp|h #18410

Merged

jnewbery changed the title ~~[WIP] Don't allow a coin to be spent and FRESH.~~ [WIP] Consensus: Don't allow a coin to be spent and FRESH. Mar 23, 2020

DrahtBot mentioned this pull request Mar 23, 2020

wallet: reduce loading time by using unordered maps #16910

Closed

jnewbery force-pushed the 2020-01-prune-pruned branch from 2ba4885 to 0786c60 Compare March 24, 2020 15:04

DrahtBot mentioned this pull request Apr 14, 2020

assumeutxo #15606

Closed

18 tasks

DrahtBot added the Needs rebase label Apr 22, 2020

jnewbery force-pushed the 2020-01-prune-pruned branch from 0786c60 to c9849da Compare April 22, 2020 22:07

jnewbery closed this Apr 22, 2020

jnewbery deleted the 2020-01-prune-pruned branch April 22, 2020 22:30

michaelfolkson mentioned this pull request May 17, 2021

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

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

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

Uh oh!

Conversation

jnewbery commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Feb 11, 2020

Uh oh!

jonatack commented Feb 19, 2020

Uh oh!

amitiuttarwar Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjahr Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 18, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjahr Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjahr Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Mar 18, 2020

Uh oh!

fjahr commented Mar 18, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz commented Mar 18, 2020

Uh oh!

fjahr commented Mar 19, 2020

Uh oh!

andrewtoth commented Mar 19, 2020

Uh oh!

jnewbery commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 22, 2020

Uh oh!

jnewbery commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 24, 2020

Uh oh!

jnewbery commented Feb 11, 2020 •

edited

Loading

DrahtBot commented Feb 11, 2020 •

edited

Loading

amitiuttarwar Mar 18, 2020 •

edited

Loading

fjahr Mar 18, 2020 •

edited

Loading

ghost commented Mar 18, 2020 •

edited by ghost

Loading

fjahr Mar 18, 2020 •

edited

Loading

fjahr Mar 18, 2020 •

edited

Loading

jnewbery commented Mar 23, 2020 •

edited

Loading

jnewbery commented Apr 22, 2020 •

edited

Loading