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

jnewbery · 2020-03-23T15:09:24Z

Add full commenting for spentness / DIRTYness / FRESHness and which combinations are valid
Remove the 'pruned' terminology, which doesn't make sense since per-txout chainstate db was merged (Switch chainstate db and cache to per-txout model #10195).
Rename potential_overwrite to possible_overwrite to standardize terminology (there were previously examples of both, which made searching the codebase difficult).
Make other minor improvements to the comments

src/test/coins_tests.cpp

jnewbery · 2020-03-23T15:36:30Z

These changes were extracted from #18113.

DrahtBot · 2020-03-23T18:28:39Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

laanwj · 2020-03-25T15:13:38Z

Thanks for improving the documentation here. I think this is a clear improvement.

ACK 97298e5

jonatack · 2020-03-26T19:33:30Z

Concept ACK

ryanofsky

Code review ACK 97298e5. Nice change, great to clean up old pruning references and add more of an explanation about possible_overwrite

ryanofsky · 2020-03-27T21:19:40Z

src/coins.cpp

        }
+        // If the coin exists in this cache as a spent coin and is DIRTY, then


In commit "[docs] Improve commenting in coins.cpp|h" (eeeb97b)

It seems like this comment is going into the weeds about when it is safe to set these flags without explaining what the code is trying to do. If I were trying to explain this code I think I'd start off with something like:

"As an optimization, try to set the FRESH flag when the caller has passed possible_overwrite=false and is guaranteeing that the coin was spent or never existed before this call. In this case, the FRESH flag can be applied as long as there's not a previous spent dirty cache entry (from disconnecting a block in a reorg and "spending" the coin to remove it), that hasn't yet been written to the base view."

Thanks for the suggestion. I find my version easier to parse (although I'm biased, of course). The second sentence in your version is:

"In [previous sentence] case, [thing can happen] as long as [condition] (from [cause]) that [subcondition]"

which I find to be a lot to arrange in my head at once. In my version, I've tried to keep the sentence structures simple like "IF [thing] THEN [thing]".

src/coins.cpp

ryanofsky · 2020-03-27T22:10:46Z

src/coins.h

     * already exist in the cache.
     */
-    void AddCoin(const COutPoint& outpoint, Coin&& coin, bool potential_overwrite);
+    void AddCoin(const COutPoint& outpoint, Coin&& coin, bool possible_overwrite);


In commit "[docs] use consistent naming for possible_overwrite" (97298e5)

Not for here, since it's too much change for a documentation PR, but it might be worth considering replacing this bool possible_overwrite param with an inverted bool fresh param. possible_overwrite=false would change to fresh=true, meaning it is safe for AddCoin to assume any existing coin is spent, and possible_overwrite=true would change to fresh=false, meaning it can't make that assumption

(This would be a partial step in the direction I was trying to go in #9384: instead of repeating fresh/dirty logic and implementing it slightly different ways in AddCoin, and SpendCoin, Batchwrite, just doing it one time one way in a CCoinsViewCache::Modifier class.)

Calling the param fresh instead of possible_overwrite would make it more clear this is doing the same freshness logic as BatchWrite et al, and not something different

I agree that this is too much for this PR. Additional documentation around possible_overwrite or whatever you decide to change it to would be very welcome!

ryanofsky · 2020-03-27T22:44:54Z

src/coins.cpp

+                // A coin can only be FRESH in the child cache if it doesn't exist
+                // in any of the ancestor caches. We can therefore mark it FRESH


In commit "[docs] Improve commenting in coins.cpp|h" (eeeb97b)

"A coin can only be FRESH in the child cache if it doesn't exist in any of the ancestor caches" is too broad a statement, not generally true. You can layer caches arbitrarily and a fresh flag in a child doesn't tell you about the state of the coin in every ancestor cache. It only directly tells you the state of the coin in the immediate parent cache. In this case, we only indirectly know the state of the grandparent coin due to this happening under the itUs == cacheCoins.end() branch.

Would write this comment more like: "We can mark parent cache entry FRESH here if child cache entry was FRESH. This works because parent cache entry not previously existing and child cache entry being FRESH indicates grandparent coin is spent or doesn't exist."

In this case, we only indirectly know the state of the grandparent coin due to this happening under the itUs == cacheCoins.end() branch.

I don't understand this statement. itUs == cacheCoins.end() simply tells us that the coin doesn't exist in the parent cache's CCoinsMap structure. It doesn't tell us anything about the grandparent cache.

In this case, we only indirectly know the state of the grandparent coin due to this happening under the itUs == cacheCoins.end() branch.

If entry has fresh flag and there is no parent entry (itUs == cacheCoins.end()) then the coin must be spent in the grandparent and therefore it's safe to re-apply the fresh flag

It doesn't tell us anything about the grandparent cache.

Of course, the grandparent might not even be a cache view. But whatever kind of view it is the coin must be spent there.

If entry has fresh flag and there is no parent entry (itUs == cacheCoins.end()) then the coin must be spent in the grandparent

I still don't understand this. Can you explain the logic?

(I still believe that no ancestor cache can contain the coin due to the way we build the cache hierarchy, but I don't understand how you reason about just the grandparent cache)

I don't see why we're going back and forth on one sentence in a code review comment I made explaining why I thought your code comment was making an unstated assumption. If you don't like my suggestion (which does not mention itUs), feel free to ignore!

My logic is just that in order for a cache entry to wind up with the fresh flag, the coin in its parent view (whether it's a cache or not, whether or not it has entries or is flushed if it is a cache) must be spent. If the parent view is a cache, and doesn't have an entry, the coin must be spent in the grandparent view (again whether or not it's a cache and regardless of whether entries are created).

I am using "spent" here as shorthand for "spent or never existed" in case that what may be confusing. My working definition of "fresh" is "spent in the parent view"

Maybe: "A coin can only be FRESH in the child cache if it doesn't exist (e.g. was spent or never existed) in any of the ancestor caches".

or: "A coin can only be FRESH in the child cache if it doesn't exist (e.g. was spent or never existed) in the parent view, nor in any of the ancestor caches due to the way we build the cache hierarchy."

I don't see why we're going back and forth on one sentence in a code review comment I made explaining why I thought your code comment was making an unstated assumption.

Because I'm trying to understand your logic and if there's a gap in my understanding. And if not, whether the comment is needlessly confusing.

in order for a cache entry to wind up with the fresh flag, the coin in its parent view [...] must be spent. If the parent view is a cache, and doesn't have an entry, the coin must be spent in the grandparent view

But isn't this transitive? Can I not equally say "If the grandparent view is a cache, and doesn't have an entry, the coin must be spent in the great-grandparent view" and so on?

maflcko · 2020-04-02T13:37:12Z

Why is this assigned 0.20.0? This is not a bugfix, and the rename might be controversial, so let's better wait for 0.21.0?

jonatack

Thanks for working to improve this! I like where it's going, modulo @ryanofsky's good comments and some suggestions and fixups below.

Despite knowing they are different flags I am still trying to avoid thinking of FRESH and DIRTY as mutually exclusive states 🕵️‍♂️

src/coins.h

src/test/coins_tests.cpp

instagibbs

I think the text is clearly better than before. Still hard to understand. Shouldn't have missed the PR review club :(

src/coins.h

michaelfolkson · 2020-04-14T20:55:57Z

Concept ACK. Definitely confused me during the PR review club.

Did you give any thought to more intuitive names than FRESH and DIRTY @jnewbery? Don't want to open up a bikeshedding Pandora's box but the additional comments proposed by @jonatack suggest the naming isn't optimal.

https://bitcoincore.reviews/18113.html#l-142

jnewbery · 2020-04-15T15:56:34Z

Did you give any thought to more intuitive names than FRESH and DIRTY @jnewbery? Don't want to open up a bikeshedding Pandora's box but the additional comments proposed by @jonatack suggest the naming isn't optimal.

I think that we could potentially rename FRESH to something less confusing (DIRTY is commonly used terminology for a cache entry that needs to be flushed to the parent. That discussion should happen in a separate issue or PR to not hold this up.

jnewbery · 2020-04-15T15:57:53Z

I believe I've now addressed all feedback given on this PR up to this point.

jonatack · 2020-04-15T22:12:48Z

ACK 98bee55 latest documentation changes in git diff 97298e5 98bee55 look good as well as the improved CCoinsCacheEntry Doxygen formatting; reverified all occurrences of potential_overwrite were changed to possible_overwrite.

I think that we could potentially rename FRESH to something less confusing (DIRTY is commonly used terminology for a cache entry that needs to be flushed to the parent. That discussion should happen in a separate issue or PR to not hold this up.

Agreed -- SGTM.

Thanks, John.

ryanofsky

Code review ACK 98bee55. No significant changes since last review, just minor copy editing for comments and more specific logic_error string

jnewbery · 2020-04-16T18:56:59Z

@instagibbs @MarcoFalke can you help get this over the line? 🙏

instagibbs · 2020-04-16T19:00:21Z

ACK 98bee55

not going to hold up a clear improvement

michaelfolkson · 2020-04-20T13:15:20Z

ACK 98bee55

(ideally with follow up PR renaming FRESH to something more intuitive like NEWCOIN)

maflcko

Approach ACK 98bee55 I read through once, some questions inline 🛁

Show signature and timestamp

Signature:

Timestamp of file with hash 1344d8e0d46a4a6d591e9f8a227b79d3b1b0a847972fd99a17aafbdf3e6d428c -

maflcko · 2020-04-20T12:09:40Z

src/coins.cpp

-                // Otherwise we will need to create it in the parent
-                // and move the data up and mark it as dirty
+                // Create the coin in the parent cache, move the data up
+                // and mark it as dirty.


in commit be549b3:

I don't think inline comments that translate the c++ source code into English are helpful. Instead of repeating the code, they should clarify its intent and provide motivation and background information that is not available from just reading the code. If there is nothing to clarify, no inline comment should be made.

Doxygen comments on the other hand may repeat what the code does on a abstract, higher level to give devs the convenience to not dig into the internal source code of a class or module.

I agree, although in this case, I was simply cleaning up a comment that already existed (using otherwise to continue a sentence over two comments is a bad idea because when the code changes comments like this often lose their meaning.

maflcko · 2020-04-20T12:36:50Z

src/coins.cpp

-                // we must not copy that FRESH flag to the parent as that
-                // pruned state likely still needs to be communicated to the
-                // grandparent.
+                // NOTE: It isn't safe to mark the coin as FRESH in the parent


in commit be549b3:

All inline comments are notes. Without loss of information, this comment can be pruned.

Suggested change

// NOTE: It isn't safe to mark the coin as FRESH in the parent

// It isn't safe to mark the coin as FRESH in the parent

I agree. Could probably be improved further.

maflcko · 2020-04-20T14:36:07Z

src/coins.cpp

                CCoinsCacheEntry& entry = cacheCoins[it->first];
                entry.coin = std::move(it->second.coin);
                cachedCoinsUsage += entry.coin.DynamicMemoryUsage();
                entry.flags = CCoinsCacheEntry::DIRTY;
-                // 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


Why are you removing the wording "flushed from the parent's cache"? Is it a requirement to flush the child before the parent? If yes, this comment was incorrect, if no, this comment should not be deleted.

This comment change seems to be controversial (see #18410 (comment)), so I've reverted it.

Potentially relevant context: #5863 (comment)

Remove references to 'pruned' coins, which don't exist since the move to per-txout coins db.

-BEGIN VERIFY SCRIPT- sed -i -e 's/PRUNED,/SPENT ,/g' ./src/test/coins_tests.cpp sed -i -e 's/PRUNED/SPENT/g' ./src/test/coins_tests.cpp -END VERIFY SCRIPT-

Required after scripted-diff in previous commit.

And other general comment improvements for adding coins.

jnewbery · 2020-04-21T18:23:39Z

@MarcoFalke - I've reverted the comment change that you and Russ don't like. Your other two review comments are stylistic, so I've not made any changes for those.

jonatack · 2020-04-21T20:05:45Z

Re-ACK 21fa0a4 per git diff 98bee55 21fa0a4 the only change since my previous review is the following code commenting diff in src/coins.cpp::L177-179; rebuilt/ran unit tests anyway as a sanity check on the unit test changes.

-                // A coin can only be FRESH in the child cache if it doesn't exist
-                // in any of the ancestor caches. We can therefore mark it FRESH
-                // when flushing to the parent cache.
+                // 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.flags & CCoinsCacheEntry::FRESH) {

Summary: Remove references to 'pruned' coins, which don't exist since the move to per-txout coins db. This is a backport of Core [[bitcoin/bitcoin#18410 | PR18410]] [1/3] bitcoin/bitcoin@c205979 Test Plan: proof-reading Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8953

Summary: > sed -i -e 's/PRUNED/SPENT/g' ./src/test/coins_tests.cpp This is a backport of Core [[bitcoin/bitcoin#18410 | PR18410]] [2/3] bitcoin/bitcoin@e993696 Depends on D8953 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8954

Summary: And other general comment improvements for adding coins. This is a backport of Core [[bitcoin/bitcoin#18410 | PR18410]] [3/3] bitcoin/bitcoin@21fa0a4 Depends on D8954 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8955

21fa0a4 [docs] use consistent naming for possible_overwrite (John Newbery) 2685c21 [tests] small whitespace fixup (John Newbery) e993696 scripted-diff: Rename PRUNED to SPENT in coins tests (John Newbery) c205979 [docs] Improve commenting in coins.cpp|h (John Newbery) Pull request description: - Add full commenting for spentness / DIRTYness / FRESHness and which combinations are valid - Remove the 'pruned' terminology, which doesn't make sense since per-txout chainstate db was merged (bitcoin#10195). - Rename `potential_overwrite` to `possible_overwrite` to standardize terminology (there were previously examples of both, which made searching the codebase difficult). - Make other minor improvements to the comments ACKs for top commit: jonatack: Re-ACK 21fa0a4 per `git diff 98bee55 21fa0a4` the only change since my previous review is the following code commenting diff in `src/coins.cpp::L177-179`; rebuilt/ran unit tests anyway as a sanity check on the unit test changes. Tree-SHA512: 391e01588ef5edb417250080cec17361f982c4454bc5f8c6d78bbd528c68a2bb94373297760691295c24660ce1022ad3ef7599762f736c8eed772ce096d38c3d

maflcko reviewed Mar 23, 2020

View reviewed changes

src/test/coins_tests.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-03-coins-comments branch from 0e04b2c to 97298e5 Compare March 23, 2020 15:41

DrahtBot added Docs Tests UTXO Db and Indexes Validation labels Mar 23, 2020

maflcko removed Tests UTXO Db and Indexes Validation labels Mar 23, 2020

This was referenced Mar 23, 2020

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

Closed

wallet: reduce loading time by using unordered maps #16910

Closed

CCoinsViewCache code cleanup & deduplication #9384

Closed

laanwj added this to the 0.20.0 milestone Mar 26, 2020

ryanofsky approved these changes Mar 27, 2020

View reviewed changes

maflcko removed this from the 0.20.0 milestone Apr 2, 2020

jonatack reviewed Apr 14, 2020

View reviewed changes

src/coins.h Outdated Show resolved Hide resolved

src/coins.h Show resolved Hide resolved

src/coins.h Show resolved Hide resolved

src/test/coins_tests.cpp Outdated Show resolved Hide resolved

instagibbs reviewed Apr 14, 2020

View reviewed changes

src/coins.h Outdated Show resolved Hide resolved

src/coins.h Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-03-coins-comments branch from 97298e5 to 98bee55 Compare April 15, 2020 15:57

ryanofsky approved these changes Apr 16, 2020

View reviewed changes

maflcko reviewed Apr 20, 2020

View reviewed changes

jnewbery added 4 commits April 21, 2020 14:18

[docs] Improve commenting in coins.cpp|h

c205979

Remove references to 'pruned' coins, which don't exist since the move to per-txout coins db.

scripted-diff: Rename PRUNED to SPENT in coins tests

e993696

-BEGIN VERIFY SCRIPT- sed -i -e 's/PRUNED,/SPENT ,/g' ./src/test/coins_tests.cpp sed -i -e 's/PRUNED/SPENT/g' ./src/test/coins_tests.cpp -END VERIFY SCRIPT-

[tests] small whitespace fixup

2685c21

Required after scripted-diff in previous commit.

[docs] use consistent naming for possible_overwrite

21fa0a4

And other general comment improvements for adding coins.

jnewbery force-pushed the 2020-03-coins-comments branch from 98bee55 to 21fa0a4 Compare April 21, 2020 18:19

laanwj merged commit 9e8e813 into bitcoin:master Apr 22, 2020

jnewbery deleted the 2020-03-coins-comments branch April 22, 2020 19:23

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

		}
		// If the coin exists in this cache as a spent coin and is DIRTY, then

		// A coin can only be FRESH in the child cache if it doesn't exist
		// in any of the ancestor caches. We can therefore mark it FRESH

	// NOTE: It isn't safe to mark the coin as FRESH in the parent
	// It isn't safe to mark the coin as FRESH in the parent

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

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

Uh oh!

Conversation

jnewbery commented Mar 23, 2020

Uh oh!

Uh oh!

jnewbery commented Mar 23, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

laanwj commented Mar 25, 2020

Uh oh!

jonatack commented Mar 26, 2020

Uh oh!

ryanofsky left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 27, 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!

ryanofsky Apr 15, 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!

maflcko commented Apr 2, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michaelfolkson commented Apr 14, 2020

Uh oh!

jnewbery commented Apr 15, 2020

Uh oh!

jnewbery commented Apr 15, 2020

Uh oh!

jonatack commented Apr 15, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 16, 2020

Uh oh!

instagibbs commented Apr 16, 2020

Uh oh!

michaelfolkson commented Apr 20, 2020

Uh oh!

DrahtBot commented Mar 23, 2020 •

edited

Loading

ryanofsky Mar 27, 2020 •

edited

Loading

ryanofsky Apr 15, 2020 •

edited

Loading