doc: explain AcceptToMemoryPoolWorker's coins_to_uncache #15267

pull jamesob wants to merge 1 commits into bitcoin:master from jamesob:2019-01-atmpw-uncache-coins-doc changing 2 files +20 −0

jamesob commented at 9:34 PM on January 25, 2019: member

I found ATMPW's coins_to_uncache a little hard to understand (see #15264). This adds some doc for posterity.
fanquake added the label Docs on Jan 25, 2019

in src/coins.h:298 in 3ad03db471 outdated

 293 | @@ -294,6 +294,10 @@ class CCoinsViewCache : public CCoinsViewBacked
 294 |      bool HaveInputs(const CTransaction& tx) const;
 295 |  
 296 |  private:
 297 | +    /**
 298 | +     * Note: this is marked const, but may actually append to `cacheCoins`, increasing

laanwj commented at 9:59 AM on January 31, 2019:

@note maybe for doxygen?

fanquake requested review from sdaftuar on Feb 21, 2019
in src/coins.h:300 in 3ad03db471 outdated
```
 293 | @@ -294,6 +294,10 @@ class CCoinsViewCache : public CCoinsViewBacked
 294 |      bool HaveInputs(const CTransaction& tx) const;
 295 |  
 296 |  private:
 297 | +    /**
 298 | +     * Note: this is marked const, but may actually append to `cacheCoins`, increasing
 299 | +     * memory usage.
 300 | +     */
```
sdaftuar commented at 1:56 PM on February 21, 2019:

Not sure if we need this comment here given that we also have the comment at line 204 above? I don't feel strongly though.

jnewbery commented at 3:35 PM on February 22, 2019:

I think it's ok to add this. Someone looking at the declaration of FetchCoin here might not look up to the comment next to the mutable members.

in src/validation.cpp:561 in 3ad03db471 outdated

 555 | @@ -556,6 +556,9 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, CValidationSt
 556 |              assert(txFrom->vout.size() > txin.prevout.n);
 557 |              assert(txFrom->vout[txin.prevout.n] == coin.out);
 558 |          } else {
 559 | +            // Note: this call incidentally adds txin.prevout to the coins cache
 560 | +            // (`pcoinsTip.cacheCoins`) by way of `FetchCoin()`. It should be removed
 561 | +            // later if this tx turns out to be invalid.

sdaftuar commented at 1:58 PM on February 21, 2019:

I don't believe that we should ever be pulling coins into the cache at this point in the code -- ATMP is designed so that we first pull the coins into the cache early in validation, and if we make it to this point in the code the coins should already be cached.

A comment like this would make more sense at line 671 or so below.

jamesob commented at 4:07 PM on February 22, 2019:

Ah right, good catch.

sdaftuar changes_requested
sdaftuar commented at 2:01 PM on February 21, 2019: member

Concept ACK, I'm all in favor of making stuff like this more understandable.

in src/validation.cpp:574 in 3ad03db471 outdated

 567 | @@ -565,6 +568,11 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, CValidationSt
 568 |      return CheckInputs(tx, state, view, true, flags, cacheSigStore, true, txdata);
 569 |  }
 570 |  
 571 | +/**
 572 | + * @param[out] coins_to_uncache   Return any outpoints which were not previously present in the
 573 | + *                                coins cache, but were added as a result of validating the tx
 574 | + *                                for mempool acceptance. This is used for DoS protection.

jnewbery commented at 3:33 PM on February 22, 2019:

I suggest removing the last sentence 'This is used for DoS protection.' Without further context, it's not clear how this is used for DoS protection. Your new comment below gives that context and is easy to find (since ATMPW is only called in one place).

jamesob commented at 4:20 PM on February 22, 2019:

Changed it to avoid mentioning DoS protection but instead articulate why the caller might make use of it.

jnewbery commented at 3:35 PM on February 22, 2019: member

utACK 3ad03db4719ce5f949b2ebe359ef745b79e3db2e
jamesob force-pushed on Feb 22, 2019
jamesob commented at 4:20 PM on February 22, 2019: member

Thanks for the review. Pushed an update incorporating feedback.
jnewbery commented at 5:07 PM on February 22, 2019: member

Looks good to me! ACK 560cacacaea4ada97b29af83a5c4881108a2e80f
DrahtBot commented at 12:41 AM on March 8, 2019: member



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



Conflicts

No conflicts as of last run.
jnewbery commented at 4:41 PM on March 21, 2019: member

@laanwj @sdaftuar care to reACK? Should be harmless to merge this if you're happy with the comments.
doc: explain AcceptToMemoryPoolWorker's coins_to_uncache 5d26205272

in src/validation.cpp:671 in 560cacacae outdated

 666 | @@ -660,6 +667,10 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
 667 |              if (!pcoinsTip->HaveCoinInCache(txin.prevout)) {
 668 |                  coins_to_uncache.push_back(txin.prevout);
 669 |              }
 670 | +
 671 | +            // Note: this call incidentally adds txin.prevout to the coins cache

sdaftuar commented at 5:32 PM on March 21, 2019:

Technically this only adds txin.prevout to the cache if it’s not already there (hence the call above to HaveCoinInCache). Perhaps rephrase to “... this call may add txin.prevout...” ?

jamesob commented at 2:02 PM on April 26, 2019:

Updated this, thanks!

jamesob force-pushed on Apr 26, 2019
sdaftuar commented at 2:15 PM on April 26, 2019: member

Thanks for updating, utACK
jnewbery commented at 2:38 PM on April 26, 2019: member

ACK 5d262052728acdaa2d108a35ba9921a23b3d761a
MarcoFalke referenced this in commit d76b72a454 on Apr 26, 2019
MarcoFalke merged this on Apr 26, 2019
MarcoFalke closed this on Apr 26, 2019
deadalnix referenced this in commit 061f629c7e on Jul 5, 2020
vijaydasmp referenced this in commit 459498b69c on Oct 19, 2021
vijaydasmp referenced this in commit 946d87924c on Oct 19, 2021
vijaydasmp referenced this in commit 2366480563 on Oct 20, 2021
pravblockc referenced this in commit c6aa34ede2 on Nov 18, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#15264 validation: remove useless uncache accounting in ATMPW

doc: explain AcceptToMemoryPoolWorker's coins_to_uncache #15267

Conflicts