Safer modify new coins #9107

pull morcos wants to merge 1 commits into bitcoin:master from morcos:saferModifyNewCoins changing 4 files +181 −62
  1. morcos commented at 9:07 pm on November 8, 2016: member

    This makes ModifyNewCoins more conservative in when it marks newly created outputs as FRESH. The usage of ModifyNewCoins in the code base doesn’t actually require this because its only ever used on a new cache, however I think its dangerous code that is just asking for a consensus mistake to be used in the future. The comments in the code hopefully clarify a bit better the proper assumptions that need to be made going forward.

    EDIT: The changes to the unit test are a bit complicated, but they demonstrate the problem. They fail without the prior commit.

  2. laanwj added the label UTXO Db and Indexes on Nov 9, 2016
  3. laanwj commented at 11:06 am on November 21, 2016: member
    Concept ACK, thanks for improving documentation around this code.
  4. in src/test/coins_tests.cpp: in 3572fea7dc outdated 
    246@@ -227,77 +247,139 @@ BOOST_AUTO_TEST_CASE(updatecoins_simulation_test)
247     std::vector<CCoinsViewCacheTest*> stack; // A stack of CCoinsViewCaches on top.
248     stack.push_back(new CCoinsViewCacheTest(&base)); // Start with one cache.
249 
250-    // Track the txids we've used and whether they have been spent or not
251-    std::map<uint256, CAmount> coinbaseids;
252-    std::set<uint256> alltxids;
253+    // Track the txids we've used in various sets
254+    std::set<uint256> coinbaseids;
255+    std::set<uint256> disconnectedtxs;
    ryanofsky commented at 2:47 pm on November 21, 2016:
    nit: “ids” instead of “txs” would be more consistent with other names
  5. in src/test/coins_tests.cpp: in 3572fea7dc outdated 
    214@@ -211,6 +215,22 @@ BOOST_AUTO_TEST_CASE(coins_cache_simulation_test)
215     BOOST_CHECK(missed_an_entry);
216 }
217 
218+typedef std::tuple<CTransaction,CTxUndo,CCoins> TxData;
219+// Store of all necessary tx and undo data for next test
220+std::map<uint256, TxData> alltxs;
221+
222+TxData &FindRandomFrom(std::set<uint256> &txidset) {
    ryanofsky commented at 2:50 pm on November 21, 2016:
    Arg could be const reference
  6. in src/test/coins_tests.cpp: in 3572fea7dc outdated 
    394-            UpdateCoins(tx, *(stack.back()), height);
395+            // Update the expected result
396+            // Remove new outputs
397+            result[undohash].Clear();
398+            // If not coinbase restore prevout
399+            if (!tx.IsCoinBase()) {
    ryanofsky commented at 3:23 pm on November 21, 2016:
    Is this special case actually needed? Seems like origcoins/oldcoins will be empty in this case anyway.
    morcos commented at 9:33 pm on November 29, 2016:
    It seems cleaner to me to not be messing with tx.vin of coinbases
  7. in src/test/coins_tests.cpp: in 3572fea7dc outdated 
    376+
377+            // Track this tx and undo info to use later
378+            alltxs.insert(std::make_pair(tx.GetHash(),std::make_tuple(tx,undo,oldcoins)));
379+
380+            // Update the utxo set for future spends
381+            utxoset.insert(tx.GetHash());
    ryanofsky commented at 3:37 pm on November 21, 2016:
    Suggestion: Maybe move the utxoset updates here and elsewhere next to the result updates, since utxoset is essentially just a list of the keys present in the result map. If somebody is changing one of these, they probably need to update the other too.
    morcos commented at 9:29 pm on November 29, 2016:

    hmm.. I’ll move this above the alltxs insert, but I was trying to keep the order the same for each set of changes.

    • update expected result
    • modify cache the same way the production code does (these first two things are what we will compare against each other)
    • modify state used to run the test (which includes utxoset)
  8. ryanofsky approved
  9. ryanofsky commented at 3:39 pm on November 21, 2016: member
    Tested ACK 3572fea7dce3c20bfca237a53353cdf97c0f3669.
  10. sipa commented at 1:02 am on December 1, 2016: member
    utACK 743009682111aef5c4aab84c0cb5189e4afa4f06 Care to squash?
  11. morcos force-pushed on Dec 1, 2016
  12. morcos commented at 2:39 am on December 1, 2016: member
    Squashed identical code to 7430096 only nits and minor changes to address test comments from 3572fea
  13. in src/coins.cpp: in c9270c96e2 outdated 
    116@@ -117,17 +117,30 @@ CCoinsModifier CCoinsViewCache::ModifyCoins(const uint256 &txid) {
117     return CCoinsModifier(*this, ret.first, cachedCoinUsage);
118 }
119 
120-// ModifyNewCoins has to know whether the new outputs its creating are for a
121-// coinbase or not.  If they are for a coinbase, it can not mark them as fresh.
122-// This is to ensure that the historical duplicate coinbases before BIP30 was
123-// in effect will still be properly overwritten when spent.
124+/* ModifyNewCoins allows for faster coin modification when creating the new
125+ * outputs from a transaction.  It assumes that BIP 30 applies and has already
    TheBlueMatt commented at 2:44 am on December 1, 2016:
    I hate bip numbers…any chance you can also/or only state what this actually means?
    TheBlueMatt commented at 2:49 am on December 1, 2016:
    (I think you did for 30, but not for 34)
  14. TheBlueMatt commented at 2:48 am on December 1, 2016: member
    While you’re at it, can you add a comment to coins.cpp:200 (last else branch in BatchWrite) noting that it is very important that we are only swapping the coins itself, and keeping the flags set on the parent cache.
  15. TheBlueMatt commented at 2:53 am on December 1, 2016: member
    utACK code changes in c9270c96e2ff1676eb87a94ef9b872bea3653804, though I’d prefer more comments. Didnt review tests, but did spend a bunch of time looking at this with @morcos when he was deciding the best route to go for changes.
  16. morcos commented at 5:08 pm on December 1, 2016: member
    @TheBlueMatt tell me if this is what you had in mind? I’m not sure it’s worth it, but don’t mind squashing the additional comments if you want them. The whole design of coins would be broken if we were copying all the flags over, but I tried to flag the confusing case.
  17. TheBlueMatt commented at 10:19 pm on December 1, 2016: member
    Yea, looks good to me.
  18. morcos force-pushed on Dec 2, 2016
  19. morcos commented at 0:40 am on December 2, 2016: member
    squashed in additional comments
  20. in src/coins.cpp: in 8174c1dead outdated 
    137+ */
138 CCoinsModifier CCoinsViewCache::ModifyNewCoins(const uint256 &txid, bool coinbase) {
139     assert(!hasModifier);
140     std::pair<CCoinsMap::iterator, bool> ret = cacheCoins.insert(std::make_pair(txid, CCoinsCacheEntry()));
141-    ret.first->second.coins.Clear();
142     if (!coinbase) {
    ryanofsky commented at 11:20 pm on December 2, 2016:

    Just a suggestion, but I think another way you could write this section might be:

     0    if (!coinbase) {
 1        // New coins must not already exist.
 2        assert(ret.first->second.coins.IsPruned());
 3
 4        if (!(ret.first->second.flags & CCoinsCacheEntry::DIRTY)) {
 5            // If the coin is known to be pruned (have no unspent outputs) in
 6            // the current view and the cache entry is not dirty, we know the
 7            // coin also must be pruned in the base view as well, so it is safe
 8            // to mark this fresh.
 9            ret.first->second.flags |= CCoinsCacheEntry::FRESH;
10        }
11    }

    Differences from above:

    • Assert is broader and more prominent.
    • Comments are inline, which I think make this more understandable.
    • FRESH flag will continue to be set in the case of the non-dirty existing cache entry.
    morcos commented at 11:48 pm on December 2, 2016:
    heh. i’ll want to think about that 100 times, but that sounds right to me
  21. morcos force-pushed on Dec 3, 2016
  22. morcos commented at 2:16 pm on December 3, 2016: member

    dancing the dance of a clean rebase on the grave of main.cpp

    will address @ryanofsky’s suggestion separately

  23. morcos commented at 7:34 pm on December 5, 2016: member
    @sipa @TheBlueMatt Can you guys take a look one more time at the new logic. I’ll squash to one commit after you ACK. @fanquake Can you milestone for 0.14.0
  24. MarcoFalke added this to the milestone 0.14.0 on Dec 5, 2016
  25. in src/coins.cpp: in e5f93783cd outdated 
    157+            // coin also must be pruned in the parent view as well, so it is safe
158+            // to mark this fresh.
159+            ret.first->second.flags |= CCoinsCacheEntry::FRESH;
160         }
161     }
162     ret.first->second.coins.Clear();
    sipa commented at 10:47 pm on January 3, 2017:
    This line could move into an else branch.
    morcos commented at 2:32 am on January 4, 2017:
    Which line? The Clear()? Isn’t that needed for the historical coinbase overwrite cases?
  26. sipa commented at 10:48 pm on January 3, 2017: member
    utACK e5f93783cd4c55a4a040719a9006e897e44509a3
  27. sipa commented at 2:36 am on January 4, 2017: member
    The branch above starts with an assert that coins.IsPruned(), so a Clear() in that case would be a no-op (I think?).
  28. morcos commented at 2:39 am on January 4, 2017: member
    oh yes, confused myself.. i can change it if you think thats clearer, or a potential performance improvement?
  29. ryanofsky commented at 2:54 am on January 4, 2017: member
    Test fixes are here: https://github.com/ryanofsky/bitcoin/commits/pr/morcos-saferModifyNewCoins (in the two squash commits) if you see failures after rebasing.
  30. sipa commented at 2:45 pm on January 4, 2017: member
    @morcos No strong opinion, just pointing it out. I don’t think it will have a noticable performance impact.
  31. Fix dangerous condition in ModifyNewCoins. 
    We were marking coins FRESH before being sure they were not overwriting dirty undo data. This condition was never reached in existing code because undo data was always flushed before UpdateCoins was called with new transactions, but could have been exposed in an otherwise safe refactor.
Clarify in the comments the assumptions made in ModifyNewCoins.
Add ability to undo transactions to UpdateCoins unit test.
Thanks to Russ Yanofsky for suggestion on how to make logic clearer and fixing up the ccoins_modify_new test cases.
    b50cd7a67e
  32. morcos force-pushed on Jan 4, 2017
  33. morcos commented at 4:45 pm on January 4, 2017: member

    Rebased and squashed

    Ignored @sipa’s suggestion (sorry) Included @ryanofsky’s updates to the tests and switched the assert to throw std::logic_error so we could non-noisily test that code path.

    Diff is here: https://0bin.net/paste/RH2QCOc8C7lq505F#67QWes6s2d2bttLu6KXZqcKb7YJqEHzmY8pFv9+BV-M

  34. ryanofsky approved
  35. ryanofsky commented at 4:59 pm on January 4, 2017: member
    utACK b50cd7a67e71051db59199a4185e7c82b669c659
  36. sipa commented at 7:12 pm on January 4, 2017: member
    utACK b50cd7a67e71051db59199a4185e7c82b669c659
  37. sipa merged this on Jan 4, 2017
  38. sipa closed this on Jan 4, 2017
  39. sipa referenced this in commit 7dac1e5e9e on Jan 4, 2017
  40. sipa commented at 7:57 pm on January 4, 2017: member
    I ran the simulation test 1000x longer than the normal unit tests do.
  41. codablock referenced this in commit 4fa9920c34 on Sep 27, 2017
  42. codablock referenced this in commit 633c707969 on Oct 12, 2017
  43. codablock referenced this in commit 6c4902a46f on Oct 23, 2017
  44. codablock referenced this in commit a589c94a9a on Oct 23, 2017
  45. UdjinM6 referenced this in commit 7429a349fd on Nov 8, 2017
  46. random-zebra referenced this in commit af793b7bb9 on Aug 18, 2020
  47. MarcoFalke locked this on Sep 8, 2021


morcos laanwj ryanofsky sipa TheBlueMatt

Labels
UTXO Db and Indexes

Milestone
0.14.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-05 04:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me