ModifyNewCoins saves database lookups #6932

pull morcos wants to merge 3 commits into bitcoin:master from morcos:newCoinsThinAir changing 4 files +170 −9

morcos commented at 2:33 AM on November 3, 2015: member

When processing a new transaction, in addition to spending the Coins of its txin's, it creates a new Coins for its outputs. The existing ModifyCoins function will first make sure this Coins does not already exist. It can not exist due to BIP 30, but because of that, the lookup can't be cached and always has to go to the database. Since we are creating the Coins to match the new tx anyway, there is no point in checking if it exists first anyway. However this should not be used for coinbase tx's in order to preserve the historical behavior of overwriting the two existing duplicate tx pairs.

In conjunction with #6931 this will help ConnectBlock be much more efficient with caching access to the database.

This still needs unit tests which exercise the new functionality.

ModifyNewCoins saves database lookups

When processing a new transaction, in addition to spending the Coins of its txin's it creates a new Coins for its outputs.  The existing ModifyCoins function will first make sure this Coins does not already exist.  It can not exist due to BIP 30, but because of that the lookup can't be cached and always has to go to the database.  Since we are creating the coins to match the new tx anyway, there is no point in checking if they exist first anyway.  However this should not be used for coinbase tx's in order to preserve the historical behavior of overwriting the two existing duplicate tx pairs.

14470f9aa6

morcos cross-referenced this on Nov 3, 2015 from issue Skip BIP 30 verification where not necessary by morcos
gmaxwell commented at 10:09 PM on November 3, 2015: contributor

ConceptACK, will review further and test.
gmaxwell added this to the milestone 0.12.0 on Nov 5, 2015
morcos cross-referenced this on Nov 9, 2015 from issue Summary of potential performance improvements by morcos
laanwj added the label UTXO Db and Indexes on Nov 10, 2015
sipa commented at 7:59 AM on November 11, 2015: member

Untested, code review ACK. Needs a unit test, though.
sipa commented at 8:22 AM on November 11, 2015: member

Together with #5967 it should be possible to also avoid the db read that's still done for coinbase transactions.
Make CCoinsViewTest behave like CCoinsViewDB 03c82826f9
morcos commented at 2:34 AM on November 12, 2015: member

@sipa I'm not sure if this was the kind of unit test that you had in mind? I thought it was important to actually test UpdateCoins instead of modifyNewCoins directly because it's how it is used that matters.

This test passes on master, passes on this PR, but fails when UpdateCoins is changed to just mark coinbases as un-FRESH but still skip the lookup (assuming the assert is commented out to permit this).

However if #5967 is merged then the test passes once again.
sipa commented at 2:16 PM on November 12, 2015: member

@morcos Awesome test.
Add unit test for UpdateCoins 1cf3dd80a6
morcos force-pushed on Nov 12, 2015
morcos commented at 2:57 PM on November 12, 2015: member

ok fixed the mess with the random nValue.
sipa commented at 3:04 PM on November 12, 2015: member

ACK
jgarzik commented at 3:26 PM on November 12, 2015: contributor

lightly tested ACK
in src/test/coins_tests.cpp:None in 1cf3dd80a6
```
 303 | +                 }
 304 | +            }
 305 | +        }
 306 | +
 307 | +        if (insecure_rand() % 100 == 0) {
 308 | +            // Every 100 iterations, change the cache stack.
```
laanwj commented at 12:39 PM on November 13, 2015:

If the purpose is to do this every 100 iterations, why use random? Either the comment or the code is wrong :)

sipa commented at 12:43 PM on November 13, 2015:

It's copied from the test above that I wrote. It's intended to be random, so the comment is wrong :)
in src/test/coins_tests.cpp:None in 1cf3dd80a6
```
 306 | +
 307 | +        if (insecure_rand() % 100 == 0) {
 308 | +            // Every 100 iterations, change the cache stack.
 309 | +            if (stack.size() > 0 && insecure_rand() % 2 == 0) {
 310 | +                stack.back()->Flush();
 311 | +                delete stack.back();
```
laanwj commented at 1:04 PM on November 13, 2015:

If this was in production code I'd prefer to use a RAII pointer type (such as boost::shared_ptr) inside stack instead of explicit delete, because of exception safety and memory leaks. As it is just the tests, mehh.

sipa commented at 1:06 PM on November 13, 2015:

I believe I originally tried using boost::unique_ptr, but you can't use that inside stl containers.

laanwj commented at 1:19 PM on November 13, 2015:

Right - I think shared_ptr is the only boost pointer you can use inside containers.

Edit: with c++11 unique_ptr it should be possible, though, I guess we can change it by then :)
laanwj commented at 1:05 PM on November 13, 2015: member

Code review ACK
laanwj merged this on Nov 18, 2015
laanwj closed this on Nov 18, 2015
laanwj referenced this in commit 73fa5e6043 on Nov 18, 2015
morcos cross-referenced this on Nov 18, 2015 from issue Alter assumptions in CCoinsViewCache::BatchWrite by morcos
dagurval cross-referenced this on Dec 21, 2017 from issue Add test for UpdateCoins by dagurval
str4d cross-referenced this on May 14, 2018 from issue Bitcoin 0.12 performance improvements by str4d
zkbot referenced this in commit 9217c8e093 on May 14, 2018
zkbot referenced this in commit 8ce4bc1425 on May 14, 2018
zkbot referenced this in commit 03532b173c on May 31, 2018
zkbot referenced this in commit 9cd74866c7 on Nov 30, 2018
milesmanley referenced this in commit 67d2fb18d2 on Mar 5, 2019
random-zebra cross-referenced this on Jul 31, 2020 from issue [Core] ModifyNewCoins saves database lookups by random-zebra
random-zebra referenced this in commit 3c767c46b5 on Aug 8, 2020
nuttycom cross-referenced this on Feb 11, 2021 from issue Backport of bitcoin/bitcoin#10195 by nuttycom
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.12.0

Linked (view graph)

#5967 Alter assumptions in CCoinsViewCache::BatchWrite #6931 Skip BIP 30 verification where not necessary #6976 Summary of potential performance improvements