Merge GetCoins and HaveCoins into CCoinsViewCache::AccessCoins #4822

1. 
   sipa commented at 7:34 pm on September 2, 2014: member

   Both GetCoins and HaveCoins require a hashtable lookup, while most often we need both. Combine them into AccessCoins which returns a pointer rather than a reference (and gets rid of the overloaded name).

   Also replace some calls to the (copying) GetCoins with AccessCoins.

2. sipa renamed this:
   Combine CCoinsViewCache's HaveCoins and const GetCoins into AccessCoins
   Merge GetCoins and HaveCoins into CCoinsViewCache::AccessCoins
   on Sep 2, 2014
3. sipa force-pushed on Sep 2, 2014
4. in src/coins.cpp: in 494f003d44 outdated

   165@@ -162,9 +166,9 @@ unsigned int CCoinsViewCache::GetCacheSize() const {
   166 
   167 const CTxOut &CCoinsViewCache::GetOutputFor(const CTxIn& input) const
   168 {
   169-    const CCoins &coins = GetCoins(input.prevout.hash);
   170-    assert(coins.IsAvailable(input.prevout.n));
   171-    return coins.vout[input.prevout.n];
   172+    const CCoins* coins = AccessCoins(input.prevout.hash);
   

   ---

   ---

   

   laanwj commented at 6:25 am on September 3, 2014:

   Now needs a nullpointer check (IIRC null-ness was previously catched using an assert() ).

   ---

   

   sipa commented at 7:08 am on September 3, 2014:

   Done (and in several other places).
5. 
   laanwj commented at 6:30 am on September 3, 2014: member

   utACK, makes sense to distinguish “access to read” and “access to modify”
6. in src/coins.cpp: in 494f003d44 outdated

   203@@ -207,10 +204,10 @@ double CCoinsViewCache::GetPriority(const CTransaction &tx, int nHeight) const
   204     double dResult = 0.0;
   205     BOOST_FOREACH(const CTxIn& txin, tx.vin)
   206     {
   207-        const CCoins &coins = GetCoins(txin.prevout.hash);
   208-        if (!coins.IsAvailable(txin.prevout.n)) continue;
   209-        if (coins.nHeight < nHeight) {
   210-            dResult += coins.vout[txin.prevout.n].nValue * (nHeight-coins.nHeight);
   211+        const CCoins* coins = AccessCoins(txin.prevout.hash);
   212+        if (!coins->IsAvailable(txin.prevout.n)) continue;
   

   ---

   ---

   

   laanwj commented at 6:31 am on September 3, 2014:

   Same here (many more below, won’t comment them all)
7. sipa force-pushed on Sep 3, 2014
8. sipa force-pushed on Sep 3, 2014
9. Combine CCoinsViewCache's HaveCoins and const GetCoins into AccessCoins.

   The efficient version of CCoinsViewCache::GetCoins only works for known-to-exist
   cache entries, requiring a separate HaveCoins call beforehand. This is
   inefficient as both perform a hashtable lookup.
   
   Replace the non-mutable GetCoins with AccessCoins, which returns a potentially-NULL
   pointer. This also decreases the overloading of GetCoins.
   
   Also replace some copying (inefficient) GetCoins calls with equivalent AccessCoins,
   decreasing the copying.

   629d75faac
10. sipa force-pushed on Sep 3, 2014
11. 
    BitcoinPullTester commented at 10:34 am on September 4, 2014: none

    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4822_629d75faac84bc0a00533d01dd291a4e6394a51f/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
12. 
    TheBlueMatt commented at 5:23 am on September 9, 2014: member

    utACK
13. 
    laanwj commented at 3:14 pm on September 9, 2014: member

    Tested ACK
14. sipa merged this on Sep 9, 2014
15. sipa closed this on Sep 9, 2014

    ---

16. sipa referenced this in commit f7cdcb80ab on Sep 9, 2014
17. MarcoFalke locked this on Sep 8, 2021

Contributors
sipa laanwj BitcoinPullTester TheBlueMatt