Remove txConflicted #9240

1. 
   morcos commented at 11:03 pm on November 29, 2016: member

   In the block connection logic we were tracking a vector of transactions found via the mempool that are now conflicted by transactions newly in blocks. This allowed us to potentially find transactions that couldn’t be found through only the wallets own conflict detection. However we were not doing anything useful with this information, essentially only marking the cached credits/debits of the inputs dirty. This wouldn’t change balance calculations though as we weren’t marking txs detected this way as conflicted in the wallet.

   In the future we might try to improve the best efforts detection of conflicted txs to use the mempool, but we can always resurrect this code if we find it useful. In the meantime it serves as a confusion since it has no practical effect. Getting conflict tracking correct is very difficult as transactions needed to make the connection may no longer be in the mempool and it’s also important to be able to re-find any txs marked as conflicted if they become unconflicted.

   See #8692 for more context.

   Please note that the wallet still marks as conflicted txs it can find (anything linked only through wallet txs) and that the mempool still removes all conflicts (and dependents in the mempool).

2. 
   sipa commented at 8:15 pm on November 30, 2016: member

   @TheBlueMatt Comments on how this should interact with #9014? Perhaps we want to keep the ConnectTrace struct there, but with just blocksConnected in it, or remove the struct entirely?
3. in src/test/mempool_tests.cpp: in 6f7ef9a16e outdated

   411@@ -412,7 +412,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest)
   412     /* after tx6 is mined, tx7 should move up in the sort */
   413     std::vector<CTransactionRef> vtx;
   414     vtx.push_back(MakeTransactionRef(tx6));
   415-    pool.removeForBlock(vtx, 1, NULL, false);
   416+    pool.removeForBlock(vtx, 1);
   

   ---

   ---

   

   sipa commented at 0:17 am on December 1, 2016:

   This changes the fCurrentEstimate parameter value from false to true. Intentional?

   ---

   

   morcos commented at 1:02 am on December 1, 2016:

   Yeah, there was no reason for that to be set. It’s only for fee estimates which are useless in this test.
4. 
   sipa commented at 0:19 am on December 1, 2016: member

   utACK
5. 
   TheBlueMatt commented at 1:59 am on December 1, 2016: member

   I’d prefer to keep the ConnectTrace in #9014 as it might make fixing #9027 easier later. One will have to be rebased on the other, though…
6. morcos force-pushed on Dec 3, 2016
7. 
   morcos commented at 2:09 pm on December 3, 2016: member

   dance dance dance on the grave of main.cpp
8. remove external usage of mempool conflict tracking bf663f8e93
9. remove internal tracking of mempool conflicts for reporting to wallet a874ab5ccf
10. morcos force-pushed on Dec 5, 2016
11. 
    morcos commented at 7:06 pm on December 5, 2016: member

    Rebased after #9014
12. 
    sipa commented at 7:57 am on December 6, 2016: member

    reutACK a874ab5ccf7839edb445830f81591fa608d85fa6
13. in src/validation.cpp: in a874ab5ccf

    2157- * applied to the UTXO state as a part of a single ActivateBestChainStep call.
    2158+ * Used to track blocks whose transactions were applied to the UTXO state as a
    2159+ * part of a single ActivateBestChainStep call.
    2160  */
    2161 struct ConnectTrace {
    2162-    std::vector<CTransactionRef> txConflicted;
    

    ---

    ---

    

    jonasschnelli commented at 7:16 pm on December 8, 2016:

    Should we also get rid of the ConnectTrace struct?

    ---

    

    morcos commented at 8:18 pm on December 8, 2016:

    @TheBlueMatt was in favor of keeping it for now. I have no objection.
14. jonasschnelli approved
15. 
    jonasschnelli commented at 7:17 pm on December 8, 2016: contributor

    utACK a874ab5ccf7839edb445830f81591fa608d85fa6
16. 
    gmaxwell commented at 5:16 pm on December 9, 2016: contributor

    utACK
17. sipa merged this on Dec 10, 2016
18. sipa closed this on Dec 10, 2016

    ---

19. sipa referenced this in commit a1dcf2e108 on Dec 10, 2016
20. MarcoFalke added the label Refactoring on Dec 10, 2016
21. 
    ryanofsky commented at 8:38 pm on December 14, 2016: member

    There was discussion about this change in IRC: https://botbot.me/freenode/bitcoin-core-dev/2016-12-14/?msg=77971517&page=2.

    Summary: #9240 caused an unintended change where “-walletnotify isn’t fired on transactions that were in the mempool and then evicted due to a tx in a new block conflicting them.” One response might be to revert #9240, another response might be to add new calls to SyncTransaction for all transactions dropped from the mempool, which would add back the dropped -walletnotify calls and more.

22. domob1812 referenced this in commit 548b9dd259 on Dec 17, 2016
23. codablock referenced this in commit a3f6060b60 on Jan 16, 2018
24. codablock referenced this in commit 5580711a96 on Jan 16, 2018
25. codablock referenced this in commit 04f6a7367d on Jan 17, 2018
26. gladcow referenced this in commit a41a0c7bdd on Mar 5, 2018
27. gladcow referenced this in commit dd13891632 on Mar 8, 2018
28. gladcow referenced this in commit 373109320c on Mar 13, 2018
29. gladcow referenced this in commit dd28b17e94 on Mar 14, 2018
30. gladcow referenced this in commit a8d858f72b on Mar 15, 2018
31. gladcow referenced this in commit d0641d5e6d on Mar 15, 2018
32. gladcow referenced this in commit 9997da4ccf on Mar 15, 2018
33. gladcow referenced this in commit a444005dc7 on Mar 15, 2018
34. gladcow referenced this in commit 274cd192a9 on Mar 24, 2018
35. gladcow referenced this in commit 5af46eec47 on Apr 4, 2018
36. UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018
37. andvgal referenced this in commit 7600c61d7e on Jan 6, 2019
38. andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019
39. CryptoCentric referenced this in commit 736d903b6b on Feb 25, 2019
40. CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019
41. ryanofsky referenced this in commit 2c1c162cd5 on May 5, 2020
42. ryanofsky referenced this in commit f963a68051 on May 13, 2020
43. laanwj referenced this in commit 99c03728c0 on May 13, 2020
44. sidhujag referenced this in commit a84dd1fa2e on May 14, 2020
45. fanquake referenced this in commit 9c193e4d0a on May 14, 2020
46. fanquake referenced this in commit ed0afe8c1f on May 15, 2020
47. metalicjames referenced this in commit 09af10dec5 on Aug 5, 2020
48. janus referenced this in commit f690d20f65 on Nov 15, 2020
49. furszy referenced this in commit 6143f803b2 on Jan 30, 2021
50. backpacker69 referenced this in commit 364a2d33dd on Mar 28, 2021
51. DrahtBot locked this on Sep 8, 2021

Contributors
morcos sipa TheBlueMatt jonasschnelli gmaxwell ryanofsky

Labels
Refactoring