Docs: Improve commenting for coins.cpp|h

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

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 (#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

in src/test/coins_tests.cpp:809 in 0e04b2c44e outdated

804@@ -805,42 +805,42 @@ BOOST_AUTO_TEST_CASE(ccoins_write)
805      *              Value   Value   Value   Flags        Flags        Flags
806      */
807     CheckWriteCoins(ABSENT, ABSENT, ABSENT, NO_ENTRY   , NO_ENTRY   , NO_ENTRY   );
808-    CheckWriteCoins(ABSENT, PRUNED, PRUNED, NO_ENTRY   , DIRTY      , DIRTY      );
809-    CheckWriteCoins(ABSENT, PRUNED, ABSENT, NO_ENTRY   , DIRTY|FRESH, NO_ENTRY   );
810+    CheckWriteCoins(ABSENT, SPENT , SPENT , NO_ENTRY   , DIRTY      , DIRTY      );
811+    CheckWriteCoins(ABSENT, SPENT , ABSENT , NO_ENTRY   , DIRTY|FRESH, NO_ENTRY   );

MarcoFalke commented at 3:30 pm on March 23, 2020:

0    CheckWriteCoins(ABSENT, SPENT , ABSENT, NO_ENTRY   , DIRTY|FRESH, NO_ENTRY   );

jnewbery commented at 3:41 pm on March 23, 2020:

oops. Fixed. Thanks Marco

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

These changes were extracted from #18113.

jnewbery force-pushed on Mar 23, 2020

DrahtBot added the label Docs on Mar 23, 2020

DrahtBot added the label Tests on Mar 23, 2020

DrahtBot added the label UTXO Db and Indexes on Mar 23, 2020

DrahtBot added the label Validation on Mar 23, 2020

MarcoFalke removed the label Tests on Mar 23, 2020

MarcoFalke removed the label UTXO Db and Indexes on Mar 23, 2020

MarcoFalke removed the label Validation on Mar 23, 2020

DrahtBot commented at 6:28 pm on March 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:

#18113 ([WIP] Consensus: Don’t allow a coin to be spent and FRESH. 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.

laanwj commented at 3:13 pm on March 25, 2020: member

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

ACK 97298e536d09724952f84837b562e5e714acbf23

laanwj added this to the milestone 0.20.0 on Mar 26, 2020

jonatack commented at 7:33 pm on March 26, 2020: member

Concept ACK

in src/coins.cpp:82 in eeeb97b6da outdated

76@@ -77,8 +77,21 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
77     }
78     if (!possible_overwrite) {
79         if (!it->second.coin.IsSpent()) {
80-            throw std::logic_error("Adding new coin that replaces non-pruned entry");
81+            throw std::logic_error("Attempted to overwrite an unspent coin");
82         }
83+        // If the coin exists in this cache as a spent coin and is DIRTY, then

ryanofsky commented at 9:19 pm on March 27, 2020:

In commit “[docs] Improve commenting in coins.cpp|h” (eeeb97b6da16fed5701ff04d3b9a622eb759fa5a)

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

jnewbery commented at 3:13 pm on April 15, 2020:

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]”.

in src/coins.cpp:80 in eeeb97b6da outdated

76@@ -77,8 +77,21 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
77     }
78     if (!possible_overwrite) {
79         if (!it->second.coin.IsSpent()) {
80-            throw std::logic_error("Adding new coin that replaces non-pruned entry");
81+            throw std::logic_error("Attempted to overwrite an unspent coin");

ryanofsky commented at 9:38 pm on March 27, 2020:

In commit “[docs] Improve commenting in coins.cpp|h” (eeeb97b6da16fed5701ff04d3b9a622eb759fa5a)

I think it’s not finding an unspent coin that’s a problem, but finding an unspent coin when possible_overwrite=false that’s a problem. logic_error(“Found unexpected coin in cache with possible_overwrite=false”) might make it clearer the error’s more of an unexpected state than a failed attempt to write something

jnewbery commented at 3:31 pm on April 15, 2020:

I don’t think this is necessary. Assert messages are for developers, who’ll be able to easily see that this assert is in the !possible_overwrite branch. I’ll add a parenthesis (when possible_overwrite is false) to make this clear though.

in src/coins.h:290 in 97298e536d outdated

278@@ -279,10 +279,10 @@ class CCoinsViewCache : public CCoinsViewBacked
279     const Coin& AccessCoin(const COutPoint &output) const;
280 
281     /**
282-     * Add a coin. Set potential_overwrite to true if an unspent version may
283+     * Add a coin. Set possible_overwrite to true if an unspent version may
284      * already exist in the cache.
285      */
286-    void AddCoin(const COutPoint& outpoint, Coin&& coin, bool potential_overwrite);
287+    void AddCoin(const COutPoint& outpoint, Coin&& coin, bool possible_overwrite);

ryanofsky commented at 10:10 pm on March 27, 2020:

In commit “[docs] use consistent naming for possible_overwrite” (97298e536d09724952f84837b562e5e714acbf23)

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

jnewbery commented at 3:37 pm on April 15, 2020:

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!

in src/coins.cpp:178 in eeeb97b6da outdated

180                 entry.flags = CCoinsCacheEntry::DIRTY;
181-                // We can mark it FRESH in the parent if it was FRESH in the child
182-                // Otherwise it might have just been flushed from the parent's cache
183-                // and already exist in the grandparent
184+                // A coin can only be FRESH in the child cache if it doesn't exist
185+                // in any of the ancestor caches. We can therefore mark it FRESH

ryanofsky commented at 10:44 pm on March 27, 2020:

In commit “[docs] Improve commenting in coins.cpp|h” (eeeb97b6da16fed5701ff04d3b9a622eb759fa5a)

“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.”

jnewbery commented at 3:42 pm on April 15, 2020:

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.

ryanofsky commented at 4:41 pm on April 15, 2020:

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.

jnewbery commented at 5:21 pm on April 15, 2020:

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)

ryanofsky commented at 5:38 pm on April 15, 2020:

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”

jonatack commented at 10:34 pm on April 15, 2020:

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

jonatack commented at 10:40 pm on April 15, 2020:

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

jnewbery commented at 2:18 pm on April 20, 2020:

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?

ryanofsky approved

ryanofsky commented at 10:58 pm on March 27, 2020: member

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

in src/coins.h:142 in eeeb97b6da outdated

144+        DIRTY = (1 << 0),
145+        //! The parent cache does not have this coin (or it is a spent coin in
146+        //! the parent cache). If a FRESH coin is later spent in this cache,
147+        //! then it can be deleted entirely and doesn't ever need to be flushed
148+        //! to the parent. This is a performance optimization.
149+        //! Marking a coin FRESH which exists in the parent view will

instagibbs commented at 4:30 pm on April 1, 2020:

s/which exists/which exists unspent/ ?

jnewbery commented at 3:53 pm on April 15, 2020:

Done

in src/coins.h:138 in eeeb97b6da outdated

140+        //! This cache entry is potentially different from the version in the parent cache.
141+        //! Failing to mark a coin as DIRTY when it is potentially different from the
142+        //! parent view will cause a consensus failure, since the coin's state won't get
143+        //! written to the parent when the cache is flushed.
144+        DIRTY = (1 << 0),
145+        //! The parent cache does not have this coin (or it is a spent coin in

instagibbs commented at 4:32 pm on April 1, 2020:

Remove the parenthetical here? List the two conditions since they’re both authoritative.

Something like “Either the parent cache does not have this coin, or it has it in a spent state”

instagibbs commented at 4:33 pm on April 1, 2020:

Can we define this coin in the comments here?

Identifying “this coin” versus “changes” to “this coin”. I have some guesses but would have to actually read the underlying code to figure it out.

jnewbery commented at 3:55 pm on April 15, 2020:

Remove the parenthetical here?

Done

Can we define this coin in the comments here?

This coin is the CCoinsCacheEntry that the flag is applied to.

instagibbs commented at 3:58 pm on April 15, 2020:

In other words, IIRC, “This coin” means “If the underlying OutPoint is the same”, rather than other details like flags.

jnewbery commented at 5:41 pm on April 15, 2020:

Sorry, I understand what you’re saying now. The caches are indexed by COutPoint, so if I say “this coin exists in the parent cache”, I mean “there is an entry in the parent cache with a COutPoint that matches this coin’s COutPoint”. It seems overly verbose to put that in the comment.

MarcoFalke commented at 1:37 pm on April 2, 2020: member

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?

MarcoFalke removed this from the milestone 0.20.0 on Apr 2, 2020

in src/coins.h:125 in 97298e536d outdated

120+ * Out of these 2^3 = 8 states, only some combinations are valid:
121+ * - unspent, FRESH, DIRTY (eg a new coin created in the cache)
122+ * - unspent, not FRESH, DIRTY (eg a coin is changed in the cache during a reorg)
123+ * - unspent, not FRESH, not DIRTY (eg an unspent coin fetched from the parent cache)
124+ * - spent, FRESH, not DIRTY (eg a spent coin fetched from the parent cache)
125+ * - spent, not FRESH, DIRTY (eg a coin is spent and spentness needs to be flused to the parent)

jonatack commented at 7:41 pm on April 14, 2020:

s/flused/flushed/

jnewbery commented at 3:43 pm on April 15, 2020:

fixed

in src/coins.h:126 in 97298e536d outdated

121+ * - unspent, FRESH, DIRTY (eg a new coin created in the cache)
122+ * - unspent, not FRESH, DIRTY (eg a coin is changed in the cache during a reorg)
123+ * - unspent, not FRESH, not DIRTY (eg an unspent coin fetched from the parent cache)
124+ * - spent, FRESH, not DIRTY (eg a spent coin fetched from the parent cache)
125+ * - spent, not FRESH, DIRTY (eg a coin is spent and spentness needs to be flused to the parent)
126+ */

jonatack commented at 7:46 pm on April 14, 2020:

a few suggestions:

 0  * - FRESH or not FRESH
 1  *
 2+ * Despite what the naming may suggest, FRESH and DIRTY are independent flags
 3+ * and not mutually exclusive states.
 4+ *
 5  * Out of these 2^3 = 8 states, only some combinations are valid:
 6- * - unspent, FRESH, DIRTY (eg a new coin created in the cache)
 7- * - unspent, not FRESH, DIRTY (eg a coin is changed in the cache during a reorg)
 8- * - unspent, not FRESH, not DIRTY (eg an unspent coin fetched from the parent cache)
 9- * - spent, FRESH, not DIRTY (eg a spent coin fetched from the parent cache)
10- * - spent, not FRESH, DIRTY (eg a coin is spent and spentness needs to be flused to the parent)
11+ * - unspent, FRESH, DIRTY (e.g. a new coin created in the cache)
12+ * - unspent, not FRESH, DIRTY (e.g. a coin changed in the cache during a reorg)
13+ * - unspent, not FRESH, not DIRTY (e.g. an unspent coin fetched from the parent cache)
14+ * - spent, FRESH, not DIRTY (e.g. a spent coin fetched from the parent cache)
15+ * - spent, not FRESH, DIRTY (e.g. a coin is spent and spentness needs to be flushed to the parent)
16  */

jnewbery commented at 3:46 pm on April 15, 2020:

Despite what the naming may suggest, FRESH and DIRTY are independent flags and not mutually exclusive states.

I think this is already quite explicit from the rest of the comment. Let’s keep any further discussion for FRESH/DIRTY naming for a future PR.

I’ve taken the other suggestions.

in src/coins.h:150 in 97298e536d outdated

147+        //! then it can be deleted entirely and doesn't ever need to be flushed
148+        //! to the parent. This is a performance optimization.
149+        //! Marking a coin FRESH which exists in the parent view will
150+        //! cause a consensus failure, since it might not be deleted from the
151+        //! parent when this cache is flushed.
152+        FRESH = (1 << 1),

jonatack commented at 7:50 pm on April 14, 2020:

suggestion:

 0     enum Flags {
 1-        //! This cache entry is potentially different from the version in the parent cache.
 2-        //! Failing to mark a coin as DIRTY when it is potentially different from the
 3-        //! parent view will cause a consensus failure, since the coin's state won't get
 4-        //! written to the parent when the cache is flushed.
 5+        //! DIRTY means the CCoinsCacheEntry is potentially different from the
 6+        //! version in the parent cache.  Failure to mark a coin as DIRTY when
 7+        //! it is potentially different from the parent cache will cause a
 8+        //! consensus failure, since the coin's state won't get written to the
 9+        //! parent when the CCoinsCacheEntry is flushed.
10         DIRTY = (1 << 0),
11-        //! The parent cache does not have this coin (or it is a spent coin in
12-        //! the parent cache). If a FRESH coin is later spent in this cache,
13-        //! then it can be deleted entirely and doesn't ever need to be flushed
14-        //! to the parent. This is a performance optimization.
15-        //! Marking a coin FRESH which exists in the parent view will
16-        //! cause a consensus failure, since it might not be deleted from the
17-        //! parent when this cache is flushed.
18+        //! FRESH means the parent cache does not have this coin or that it is a
19+        //! spent coin in the parent cache.  If a FRESH coin in the
20+        //! CCoinsCacheEntry is later spent, it can be deleted entirely and
21+        //! doesn't ever need to be flushed to the parent. This is a performance
22+        //! optimization.  Marking a coin as FRESH when it exists in the parent
23+        //! cache will cause a consensus failure, since it might not be deleted
24+        //! from the parent when the CCoinsCacheEntry is flushed.
25         FRESH = (1 << 1),

jnewbery commented at 3:52 pm on April 15, 2020:

Thanks. I’ve taken some of these suggested changes.

in src/test/coins_tests.cpp:754 in 97298e536d outdated

750@@ -751,21 +751,21 @@ BOOST_AUTO_TEST_CASE(ccoins_add)
751     /* Check AddCoin behavior, requesting a new coin from a cache view,
752      * writing a modification to the coin, and then checking the resulting
753      * entry in the cache after the modification. Verify behavior with the
754-     * with the AddCoin potential_overwrite argument set to false, and to true.
755+     * with the AddCoin possible_overwrite argument set to false, and to true.

jonatack commented at 8:03 pm on April 14, 2020:

nit: can remove the redundant “with the” (already present in the preceding line).

jnewbery commented at 3:53 pm on April 15, 2020:

done

jonatack commented at 8:30 pm on April 14, 2020: member

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 :male_detective:

instagibbs commented at 8:31 pm on April 14, 2020: member

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

michaelfolkson commented at 8:55 pm on April 14, 2020: contributor

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 commented at 3:56 pm on April 15, 2020: member

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 force-pushed on Apr 15, 2020

jnewbery commented at 3:57 pm on April 15, 2020: member

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

jonatack commented at 10:12 pm on April 15, 2020: member

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 approved

ryanofsky commented at 3:35 pm on April 16, 2020: member

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

jnewbery commented at 6:56 pm on April 16, 2020: member

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

instagibbs commented at 7:00 pm on April 16, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/18410/commits/98bee558700a10afd819cbb685df0b402522ccfa

not going to hold up a clear improvement

in src/coins.cpp:172 in be549b3c04 outdated

171+            // We can ignore it if it's both spent and FRESH in the child
172             if (!(it->second.flags & CCoinsCacheEntry::FRESH && it->second.coin.IsSpent())) {
173-                // Otherwise we will need to create it in the parent
174-                // and move the data up and mark it as dirty
175+                // Create the coin in the parent cache, move the data up
176+                // and mark it as dirty.

MarcoFalke commented at 12:09 pm on April 20, 2020:

in commit be549b3c04584e41b00f20a1c7be8a0b137ccff7:

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.

jnewbery commented at 6:21 pm on April 21, 2020:

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.

in src/coins.cpp:205 in be549b3c04 outdated

201@@ -190,11 +202,10 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn
202                 itUs->second.coin = std::move(it->second.coin);
203                 cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage();
204                 itUs->second.flags |= CCoinsCacheEntry::DIRTY;
205-                // NOTE: It is possible the child has a FRESH flag here in
206-                // the event the entry we found in the parent is pruned. But
207-                // we must not copy that FRESH flag to the parent as that
208-                // pruned state likely still needs to be communicated to the
209-                // grandparent.
210+                // NOTE: It isn't safe to mark the coin as FRESH in the parent

MarcoFalke commented at 12:36 pm on April 20, 2020:

in commit be549b3c04584e41b00f20a1c7be8a0b137ccff7:

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

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

jnewbery commented at 6:22 pm on April 21, 2020:

I agree. Could probably be improved further.

michaelfolkson commented at 1:15 pm on April 20, 2020: contributor

ACK 98bee558700a10afd819cbb685df0b402522ccfa

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

in src/coins.cpp:166 in 98bee55870 outdated

178                 entry.coin = std::move(it->second.coin);
179                 cachedCoinsUsage += entry.coin.DynamicMemoryUsage();
180                 entry.flags = CCoinsCacheEntry::DIRTY;
181-                // We can mark it FRESH in the parent if it was FRESH in the child
182-                // Otherwise it might have just been flushed from the parent's cache
183-                // and already exist in the grandparent

MarcoFalke commented at 2:36 pm on April 20, 2020:

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.

jnewbery commented at 6:22 pm on April 21, 2020:

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

MarcoFalke commented at 6:27 pm on April 21, 2020:

Potentially relevant context: #5863 (comment)

MarcoFalke commented at 2:36 pm on April 20, 2020: member

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

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Approach ACK 98bee558700a10afd819cbb685df0b402522ccfa I read through once, some questions inline 🛁
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjJtgv/VzpBCTOCZbrPOWO33QFvXJjKuCzzTHTShN3o1ssfPyNdl8AOcVRS2H21
 8GaSkzJRvvUF2XY3Kin7H8ve76VgZATdYrJqQIwiGvDnHTqWKZUauydYhoywwZDMW
 9gsJdw/G/hM2uRpd5Jdws14LSv+npMYc2L4sw8Y961OOoG/YQUsppwy8/xZVicKgn
10ZE0HtfJtrWfoP8SxsSvmQzzfNXNEgMl+0fsAJjBJCr60xkkmqRAh0+LdJoHPgSLo
11yFO3xrtLaIKzrzOZycU+Ezrp+58judAhzdQEfwHza6z8/doMxZ5l8y55JyymKnmq
12bwEgsIDtSR3H12WKbtiNRnfxxYA7yNzMljCUQKleqPIuP8gTTsUFLZO6ADzXFoJb
13kSKj/gm5gmQo538j/4Ge4YN5zk5uw4MFlk1Q0uYYt3KTjCaqEggXxdPIJQOiG5hY
14y1zOlEOGt0oG/QKLWVIHehqaeHXFWJYLsbncpDJl/IB5iphSsER8LroEvzFjiIbv
15Aatmonqh
16=WXZO
17-----END PGP SIGNATURE-----

Timestamp of file with hash 1344d8e0d46a4a6d591e9f8a227b79d3b1b0a847972fd99a17aafbdf3e6d428c -

[docs] Improve commenting in coins.cpp|h

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

c205979031

scripted-diff: Rename PRUNED to SPENT in coins tests

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

e9936966c0

[tests] small whitespace fixup

Required after scripted-diff in previous commit.

2685c214cc

[docs] use consistent naming for possible_overwrite

And other general comment improvements for adding coins.

21fa0a44ab

jnewbery force-pushed on Apr 21, 2020

jnewbery commented at 6:23 pm on April 21, 2020: member

@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 commented at 8:05 pm on April 21, 2020: member

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.

0-                // A coin can only be FRESH in the child cache if it doesn't exist
1-                // in any of the ancestor caches. We can therefore mark it FRESH
2-                // when flushing to the parent cache.
3+                // We can mark it FRESH in the parent if it was FRESH in the child
4+                // Otherwise it might have just been flushed from the parent's cache
5+                // and already exist in the grandparent
6                 if (it->second.flags & CCoinsCacheEntry::FRESH) {

laanwj merged this on Apr 22, 2020

laanwj closed this on Apr 22, 2020

jnewbery deleted the branch on Apr 22, 2020

Fabcien referenced this in commit 77a9c8d991 on Jan 19, 2021

Fabcien referenced this in commit 9cbbaa7163 on Jan 19, 2021

Fabcien referenced this in commit b16ee26666 on Jan 19, 2021

PastaPastaPasta referenced this in commit ab0ab96a3b on Sep 17, 2021

PastaPastaPasta referenced this in commit 436b52eb99 on Sep 17, 2021

PastaPastaPasta referenced this in commit d0b0f08432 on Sep 18, 2021

thelazier referenced this in commit bc1e2ebbb5 on Sep 25, 2021

DrahtBot locked this on Feb 15, 2022

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

Conflicts