Fire TransactionRemovedFromMempool callbacks from mempool #14384

pull l2a5b1 wants to merge 1 commits into bitcoin:master from l2a5b1:patch/validationinterface-resolve-circular-dependencies changing 5 files +15 −36
  1. l2a5b1 commented at 8:43 PM on October 3, 2018: contributor

    This pull request fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code.

    It also resolves the txmempool -> validation -> validationinterface -> txmempool circular dependency.

    Ideally, validationinterface is a dumb component that doesn't have any knowledge of the sub-systems it sends its notifications to. The commit that aims to resolve this circular dependency by moving txmempool specific code out of validationinterface to txmempool where it belongs.

  2. l2a5b1 force-pushed on Oct 3, 2018
  3. l2a5b1 force-pushed on Oct 3, 2018
  4. DrahtBot commented at 10:11 PM on October 3, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16688 (log: Add validation interface logging by jkczyz)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  5. DrahtBot cross-referenced this on Oct 3, 2018 from issue Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations by practicalswift
  6. DrahtBot cross-referenced this on Oct 3, 2018 from issue Utxoscriptindex by mgrychow
  7. fanquake added the label Refactoring on Oct 3, 2018
  8. DrahtBot cross-referenced this on Oct 3, 2018 from issue refactor: Replace boost::bind with std::bind by ken2812221
  9. DrahtBot cross-referenced this on Oct 4, 2018 from issue Refactor: separate wallet from node by ryanofsky
  10. DrahtBot cross-referenced this on Oct 4, 2018 from issue Multiprocess bitcoin by ryanofsky
  11. l2a5b1 force-pushed on Oct 4, 2018
  12. in src/txmempool.cpp:341 in a60a187158 outdated 
     344 | +}
 345 | +
 346 | +void CTxMemPool::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason reason)
 347 | +{
 348 | +    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
 349 | +        GetMainSignals().TransactionRemovedFromMempool(ptx);
    practicalswift commented at 9:35 AM on October 4, 2018:

    Could ptx be moved to avoid unnecessary copies?

    l2a5b1 commented at 1:15 PM on April 11, 2019:

    Thanks @practicalswift. Good point. When ptx was passed by value I suspect we could have done that. However, per your suggestion #14384 (review) ptx is now passed by reference.

  13. in src/validationinterface.h:176 in a60a187158 outdated 
     175 | -    /** Unregister with mempool */
 176 | -    void UnregisterWithMempoolSignals(CTxMemPool& pool);
 177 |  
 178 |      void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload);
 179 |      void TransactionAddedToMempool(const CTransactionRef &);
 180 | +    void TransactionRemovedFromMempool(const CTransactionRef tx);
    practicalswift commented at 9:36 AM on October 4, 2018:

    Should be named ptx to match definition :-)

    l2a5b1 commented at 8:10 PM on October 4, 2018:

    Thanks @practicalswift fixed!

  14. in src/validationinterface.cpp:134 in a60a187158 outdated 
     130 | @@ -148,6 +131,12 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
 131 |      });
 132 |  }
 133 |  
 134 | +void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef ptx) {
    practicalswift commented at 9:36 AM on October 4, 2018:

    Could be made const reference :-)

  15. practicalswift commented at 9:37 AM on October 4, 2018: contributor

    Concept ACK

    Let's get rid of these ugly circular dependencies! :-)

  16. l2a5b1 force-pushed on Oct 4, 2018
  17. l2a5b1 force-pushed on Oct 4, 2018
  18. l2a5b1 force-pushed on Oct 4, 2018
  19. l2a5b1 force-pushed on Oct 4, 2018
  20. l2a5b1 force-pushed on Oct 4, 2018
  21. l2a5b1 force-pushed on Oct 4, 2018
  22. DrahtBot cross-referenced this on Oct 9, 2018 from issue Refactor: Start to separate wallet from node by ryanofsky
  23. DrahtBot cross-referenced this on Oct 23, 2018 from issue Move util files to directory by jimpo
  24. DrahtBot added the label Needs rebase on Nov 5, 2018
  25. l2a5b1 force-pushed on Nov 5, 2018
  26. DrahtBot removed the label Needs rebase on Nov 5, 2018
  27. l2a5b1 force-pushed on Nov 5, 2018
  28. DrahtBot added the label Needs rebase on Nov 9, 2018
  29. l2a5b1 force-pushed on Nov 13, 2018
  30. DrahtBot removed the label Needs rebase on Nov 13, 2018
  31. l2a5b1 force-pushed on Nov 13, 2018
  32. l2a5b1 commented at 10:26 PM on November 13, 2018: contributor

    Rebased b58bbc9

  33. DrahtBot added the label Needs rebase on Dec 29, 2018
  34. l2a5b1 force-pushed on Apr 10, 2019
  35. DrahtBot removed the label Needs rebase on Apr 10, 2019
  36. l2a5b1 commented at 10:26 AM on April 10, 2019: contributor

    rebased fe6d863

    thanks @practicalswift for the review - fe6d863 addresses your feedback.

    To answer your question #14384 (review): I don't think it is necessary to move the shared pointer as it is now passed by reference as per your suggestion: #14384 (review) .

  37. DrahtBot added the label Needs rebase on Jun 6, 2019
  38. l2a5b1 force-pushed on Jun 6, 2019
  39. DrahtBot removed the label Needs rebase on Jun 6, 2019
  40. l2a5b1 commented at 7:44 PM on June 6, 2019: contributor

    Rebased 35a63b2.

    Updated the pull request description, since the validation -> validationinterface -> validation circular dependency was resolved in #16129 and removed from this pull request.

    Travis build error seems unrelated.

  41. DrahtBot added the label Needs rebase on Jun 21, 2019
  42. l2a5b1 force-pushed on Jun 23, 2019
  43. DrahtBot removed the label Needs rebase on Jun 23, 2019
  44. l2a5b1 commented at 11:39 AM on June 23, 2019: contributor

    Rebased 80c3659

  45. in src/txmempool.cpp:24 in 80c365973f outdated 
      15 | @@ -16,6 +16,12 @@
  16 |  #include <util/system.h>
  17 |  #include <util/moneystr.h>
  18 |  #include <util/time.h>
  19 | +#include <validationinterface.h>
  20 | +
  21 | +// This map has to a separate global instead of a member of MainSignalsInstance,
  22 | +// because RegisterWithMempoolSignals is currently called before RegisterBackgroundSignalScheduler,
  23 | +// so MainSignalsInstance hasn't been created yet.
  24 | +static std::unordered_map<CTxMemPool*, boost::signals2::scoped_connection> g_connNotifyEntryRemoved;
    MarcoFalke commented at 12:12 AM on July 23, 2019:

    Why is this not a CTxMemPool member?

    MarcoFalke commented at 1:17 PM on July 23, 2019:

    Actually, this can be removed in one go:

    07c8420f4ddddde7103d8a564f918389357904c9

    l2a5b1 commented at 6:42 PM on July 23, 2019:

    Thanks @MarcoFalke! Really appreciate your feedback. Please see 04390b2.

    l2a5b1 commented at 6:55 PM on July 23, 2019:

    nit: Does it makes sense to remove CTxMemPool::EntryRemoved's method and move its implementation to CTxMemPool::removeUnchecked ?

    void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
{
    NotifyEntryRemoved(ptx, reason);
    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
        GetMainSignals().TransactionRemovedFromMempool(ptx);
    }
    ...
}
    MarcoFalke commented at 7:34 PM on July 23, 2019:

    nit: Does it makes sense to remove CTxMemPool::EntryRemoved's method and move its implementation to CTxMemPool::removeUnchecked ?

    Sure, if you wish so

  46. in src/txmempool.cpp:342 in 80c365973f outdated 
     338 | @@ -333,6 +339,24 @@ CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator) :
 339 |      nCheckFrequency = 0;
 340 |  }
 341 |  
 342 | +void CTxMemPool::RegisterSignals() {
    MarcoFalke commented at 12:12 AM on July 23, 2019:

    Why can't this be called in the mempool constructor?

  47. bitcoin deleted a comment on Jul 23, 2019
  48. l2a5b1 force-pushed on Jul 23, 2019
  49. l2a5b1 commented at 6:42 PM on July 23, 2019: contributor

    04390b2 addresses feedback from @MarcoFalke.

  50. MarcoFalke renamed this:
    Resolve validationinterface circular dependencies
    Fire TransactionRemovedFromMempool callbacks from mempool, remove RegisterWithMempoolSignals
    on Jul 23, 2019
  51. MarcoFalke renamed this:
    Fire TransactionRemovedFromMempool callbacks from mempool, remove RegisterWithMempoolSignals
    Fire TransactionRemovedFromMempool callbacks from mempool
    on Jul 23, 2019
  52. MarcoFalke cross-referenced this on Jul 23, 2019 from issue Always connect NotifyEntryRemoved with MempoolEntryRemoved by MarcoFalke
  53. MarcoFalke commented at 7:36 PM on July 23, 2019: member

    Concept ACK.

    TransactionRemovedFromMempool is used to notify of removed tx, excluding txs that were already notified via BlockConnected. However, we will miss those notifications, if we forgot to call RegisterWithMempoolSignals. This happened in the unit tests (setup_common).

    This pull fixes that and cleans up a bunch of code on the way.

  54. l2a5b1 force-pushed on Jul 23, 2019
  55. l2a5b1 force-pushed on Jul 23, 2019
  56. l2a5b1 commented at 9:52 PM on July 23, 2019: contributor

    28403ac addresses a nit (https://github.com/bitcoin/bitcoin/pull/14384#discussion_r306480219) and updates the commit message.

  57. l2a5b1 commented at 1:13 PM on October 3, 2019: contributor

    Happy 1y anniversary! 🥳🎂

  58. DrahtBot added the label Needs rebase on Oct 29, 2019
  59. MarcoFalke commented at 5:12 PM on November 8, 2019: member

    Can we get this for 0.20, pls? Needs rebase

  60. l2a5b1 force-pushed on Nov 8, 2019
  61. DrahtBot removed the label Needs rebase on Nov 8, 2019
  62. l2a5b1 commented at 8:01 PM on November 8, 2019: contributor

    @MarcoFalke sure, rebased 37fd92ba96224cfe11bacea069df02cead0c34e2

  63. MarcoFalke commented at 8:17 PM on November 8, 2019: member

    ACK 37fd92ba96224cfe11bacea069df02cead0c34e2

  64. 0xB10C commented at 9:29 AM on November 10, 2019: contributor

    Concept ACK.

  65. jnewbery commented at 6:38 PM on November 10, 2019: member

    Concept ACK. I've skimmed the code and it looks good. Will fully review and test next week.

    I think a follow-up to this would be to remove the pvtxConflicted vector in BlockConnected and instead just call TransactionRemovedFromMempool() for conflicted transactions directly from removeUnchecked(). I don't see anywhere in the client code that requires the conflicted txs to be reported in the same notification as the block.

    Doing that would allow us to remove the NotifyEntryRemoved signal from CTxMempool, the conflictedTxs from the ConnectTrace class, and some other code clean-up in validation.cpp.

  66. in src/validationinterface.cpp:130 in 37fd92ba96 outdated 
     126 | @@ -150,6 +127,12 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
 127 |      });
 128 |  }
 129 |  
 130 | +void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx) {
    jnewbery commented at 12:02 AM on November 11, 2019:

    Can you keep the MemPoolRemovealReason reason as an argument to this function please? #16688 adds validation interface logging, including logging the reason for the mempool removal.

    I also think it makes sense for the client to be notified of the reason for the mempool reason.

    jnewbery commented at 5:14 PM on November 11, 2019:

    Can be done in a follow-up, or in #16688 when this gets merged.

  67. jnewbery commented at 5:13 PM on November 11, 2019: member

    utACK 37fd92ba96224cfe11bacea069df02cead0c34e2

    re #14384 (review): On second thoughts, there's no need to change TransationRemovedFromMempool() to take a MemPoolRemovalReason as an argument. If this PR gets merged before #16688 , then that PR should just add the argument there.

    I think a follow-up to this would be to remove the pvtxConflicted vector in BlockConnected

    I have a branch that does that here: https://github.com/jnewbery/bitcoin/tree/2019-11-remove-mempool-signals2. Will PR once this is merged.

  68. l2a5b1 commented at 7:44 PM on November 11, 2019: contributor

    @jnewbery thanks for the review! 😃

  69. jnewbery cross-referenced this on Nov 14, 2019 from issue Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals by jnewbery
  70. ariard approved
  71. ariard commented at 7:48 PM on November 14, 2019: member

    Tested ACK 37fd92b

  72. jamesob commented at 6:27 PM on November 18, 2019: member

    ACK https://github.com/bitcoin/bitcoin/pull/14384/commits/37fd92ba96224cfe11bacea069df02cead0c34e2

    This is basically some helpful code-shuffling; the change removes unneeded code to absorb the TransactionRemovedFromMempool signal into the existing CValidationInterface callback registration code, and avoids including mempool-specific header files into the more general validationinterface.

  73. jonatack commented at 6:43 PM on November 18, 2019: contributor

    Concept ACK, kudos @l2a5b1 for staying with this.

    Built 37fd92ba96224cfe11bacea069df02cead0c34e2 rebased onto current master at 30521302f90e4856a7516867b32a4576fa6d98b3, tests pass, bitcoind running.

    Now reviewing the code.

  74. jonatack commented at 7:31 PM on November 18, 2019: contributor

    ACK 37fd92ba96224cfe11bacea069df02cead0c34e2

    Nice cleanup that improves separation of concerns and enables removing MempoolEntryRemoved, RegisterWithMempoolSignals, and UnregisterWithMempoolSignals.

  75. TheBlueMatt commented at 8:31 PM on November 18, 2019: contributor

    This isn't strictly worse, but its still super awkward....CTxMemPool is (mostly) a "dumb datastructure", with all the adding/removing happening in validation.cpp. Currently its double awkward cause they're signals that go back to validation so that validation can fire appropriate events, but just firing the events inside a datastructure is also really awkward. Is there an equally-easy way to move the event-firing back into validation.cpp?

  76. jnewbery commented at 9:14 PM on November 18, 2019: member

    Is there an equally-easy way to move the event-firing back into validation.cpp?

    I don't think there is. That would require that every site that validation calls a CTxMemPool method that could result in transactions being removed would need to be passed back a vector of transactions that it should fire the events for. A quick search shows at least the following sites would need to be updated:

    • validation's UpdateMempoolForReorg() calling into mempool.removeRecursive()
    • validation's LimitMempoolSize() calling into pool.Expire()
    • validation's LimitMempoolSize() calling into pool.TrimToSize()

    There are probably more. I don't think those call sites have ever handled firing the events, and adding that functionality would be new data structures, function signatures and logic.

    This seems to be a strict improvement to me. The events are being fired from exactly the same place as before, except that there's no weird indirection in the validation interface.

  77. laanwj added this to the "Blockers" column in a project
  78. MarcoFalke commented at 7:30 PM on November 19, 2019: member

    Re-run ci

  79. MarcoFalke closed this on Nov 19, 2019
  80. MarcoFalke reopened this on Nov 19, 2019
  81. TheBlueMatt commented at 2:46 AM on November 21, 2019: contributor

    Right, certainly don't interpret my comment as a NACK, then. Longer-term it would be nice, indeed, to move the event-firing into those callsites, cause the responsibility right now is very awkwardly split (hence why we're checking the reason in the first place - validation fires some similar things), but this isn't worse than master and if other PR's need this, do it!

  82. laanwj commented at 6:42 PM on November 21, 2019: member

    argh, needs trivial rebase for the linter test/lint/lint-circular-dependencies.sh

  83. DrahtBot added the label Needs rebase on Nov 21, 2019
  84. Fire TransactionRemovedFromMempool from mempool
    This commit fires TransactionRemovedFromMempool callbacks from the
mempool and cleans up a bunch of code.
    e20c72f9f0
  85. l2a5b1 force-pushed on Nov 21, 2019
  86. DrahtBot removed the label Needs rebase on Nov 21, 2019
  87. l2a5b1 commented at 6:43 AM on November 22, 2019: contributor

    argh @laanwj lol, no worries, rebased e20c72f9f076681def325b5b5fa53bccda2b0eab

  88. jnewbery commented at 2:59 PM on November 22, 2019: member

    ACK e20c72f9f076681def325b5b5fa53bccda2b0eab

  89. laanwj referenced this in commit bb862d7864 on Nov 22, 2019
  90. laanwj merged this on Nov 22, 2019
  91. laanwj closed this on Nov 22, 2019
  92. ariard commented at 4:16 PM on November 22, 2019: member

    ACK e20c72f, only rebased.

  93. jonatack commented at 5:13 PM on November 22, 2019: contributor

    Posthumous ACK e20c72f9f076681def325b5b5fa53bccda2b0eab

  94. jkczyz cross-referenced this on Nov 22, 2019 from issue log: Add validation interface logging by jkczyz
  95. sidhujag referenced this in commit 25b2afe377 on Nov 22, 2019
  96. jkczyz referenced this in commit 34d6486de3 on Nov 22, 2019
  97. meshcollider removed this from the "Blockers" column in a project
  98. MarkLTZ cross-referenced this on Apr 4, 2020 from issue Bitcoin PR tracking by MarkLTZ
  99. deadalnix referenced this in commit d66d5d7efd on Jun 8, 2020
  100. sidhujag referenced this in commit 59b9555630 on Nov 10, 2020
  101. furszy cross-referenced this on Mar 9, 2021 from issue Killing wallet and GUI cs_main locks by furszy
  102. furszy cross-referenced this on Mar 10, 2021 from issue [wallet] Conflicted transactions external listeners notification fix by furszy
  103. random-zebra referenced this in commit e0350eda5b on Apr 14, 2021
  104. bitcoin locked this on Dec 16, 2021
Contributors
l2a5b1DrahtBotpracticalswiftMarcoFalke0xB10CjnewberyariardjamesobjonatackTheBlueMattlaanwj
Labels
Refactoring
Linked (view graph)
#16129 refactor: Remove unused includes#16437 Always connect NotifyEntryRemoved with MempoolEntryRemoved#16688 log: Add validation interface logging#17477 Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals
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: 2026-05-20 06:54 UTC