ModifyNewCoins saves database lookups #6932

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

2. 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
3. 
   gmaxwell commented at 10:09 pm on November 3, 2015: contributor

   ConceptACK, will review further and test.
4. gmaxwell added this to the milestone 0.12.0 on Nov 5, 2015
5. laanwj added the label UTXO Db and Indexes on Nov 10, 2015
6. 
   sipa commented at 7:59 am on November 11, 2015: member

   Untested, code review ACK. Needs a unit test, though.
7. 
   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.
8. Make CCoinsViewTest behave like CCoinsViewDB 03c82826f9
9. 
   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.

10. 
    sipa commented at 2:16 pm on November 12, 2015: member

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

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

    ACK
15. 
    jgarzik commented at 3:26 pm on November 12, 2015: contributor

    lightly tested ACK
16. in src/test/coins_tests.cpp: 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 :)
17. in src/test/coins_tests.cpp: 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 :)

18. 
    laanwj commented at 1:05 pm on November 13, 2015: member

    Code review ACK
19. laanwj merged this on Nov 18, 2015
20. laanwj closed this on Nov 18, 2015

    ---

21. laanwj referenced this in commit 73fa5e6043 on Nov 18, 2015
22. zkbot referenced this in commit 9217c8e093 on May 14, 2018
23. zkbot referenced this in commit 8ce4bc1425 on May 14, 2018
24. zkbot referenced this in commit 03532b173c on May 31, 2018
25. zkbot referenced this in commit 9cd74866c7 on Nov 30, 2018
26. milesmanley referenced this in commit 67d2fb18d2 on Mar 5, 2019
27. random-zebra referenced this in commit 3c767c46b5 on Aug 8, 2020
28. MarcoFalke locked this on Sep 8, 2021

Contributors
morcos gmaxwell sipa jgarzik laanwj

Labels
UTXO Db and Indexes

Milestone
0.12.0