Remove mempool global #19556

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:Mf1808-txpoolGlobal changing 9 files +38 −28
  1. MarcoFalke commented at 4:20 PM on July 19, 2020: member

    This refactor unlocks some nice potential features, such as, but not limited to:

    • Removing the fee estimates global (would avoid slightly fragile workarounds such as #18766)
    • Making the mempool optional for a "blocksonly" operation mode

    Even absent those features, the new code without the global should be easier to maintain, read and write tests for.

  2. MarcoFalke added the label Refactoring on Jul 19, 2020
  3. hebasto commented at 4:25 PM on July 19, 2020: member

    Concept ACK.

  4. jonatack commented at 4:57 PM on July 19, 2020: contributor

    Concept/approach ACK.

  5. JeremyRubin commented at 5:24 PM on July 19, 2020: contributor

    Concept ACK on this PR.

    lite approach NACK on "Making the mempool optional for a "blocksonly" operation mode", it should be in it's own flag as there are some other reasons to keep a mempool around even in blocksonly.

  6. MarcoFalke force-pushed on Jul 19, 2020
  7. DrahtBot commented at 12:02 AM on July 20, 2020: 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:

    • #19872 (Avoid locking CTxMemPool::cs recursively in some cases by hebasto)
    • #19865 (scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion by ryanofsky)
    • #19848 (Remove mempool global from interfaces by MarcoFalke)
    • #19791 ([net processing] Move Misbehaving() to PeerManager by jnewbery)
    • #19677 (Switch BlockMap to use an unordered_set under the hood by JeremyRubin)
    • #19572 (ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs)
    • #15719 (Wallet passive startup by ryanofsky)
    • #12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)
    • #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

    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.

  8. DrahtBot cross-referenced this on Jul 20, 2020 from issue net: Add -networkactive option by hebasto
  9. DrahtBot cross-referenced this on Jul 20, 2020 from issue refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto
  10. DrahtBot cross-referenced this on Jul 20, 2020 from issue validation: Warm coins cache during prevalidation to connect blocks faster by andrewtoth
  11. DrahtBot cross-referenced this on Jul 20, 2020 from issue refactor: remove ::vpwallets and related global variables by ryanofsky
  12. DrahtBot cross-referenced this on Jul 20, 2020 from issue refactor: Move wallet methods out of chain.h and node.h by ryanofsky
  13. DrahtBot cross-referenced this on Jul 20, 2020 from issue test: Remove duplicate NodeContext hacks by ryanofsky
  14. DrahtBot cross-referenced this on Jul 20, 2020 from issue refactor: Cleanup thread ctor calls by hebasto
  15. DrahtBot cross-referenced this on Jul 20, 2020 from issue refactor: Make CCheckQueue RAII-styled by hebasto
  16. DrahtBot cross-referenced this on Jul 20, 2020 from issue Add local thread pool to CCheckQueue by hebasto
  17. DrahtBot cross-referenced this on Jul 20, 2020 from issue coins: allow cache resize after init by jamesob
  18. DrahtBot cross-referenced this on Jul 20, 2020 from issue rpc: remove deprecated CRPCCommand constructor by MarcoFalke
  19. DrahtBot cross-referenced this on Jul 20, 2020 from issue Use shared pointers only in validation interface by bvbfan
  20. DrahtBot cross-referenced this on Jul 20, 2020 from issue Use wtxid for transaction relay by sdaftuar
  21. DrahtBot cross-referenced this on Jul 20, 2020 from issue Wallet passive startup by ryanofsky
  22. DrahtBot cross-referenced this on Jul 20, 2020 from issue feature: Added ability for users to add a startup command by benthecarman
  23. DrahtBot cross-referenced this on Jul 20, 2020 from issue RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr
  24. DrahtBot cross-referenced this on Jul 20, 2020 from issue Add fee_est tool for debugging fee estimation code by ryanofsky
  25. naumenkogs commented at 7:16 AM on July 20, 2020: member

    Concept ACK.

  26. jnewbery commented at 10:51 AM on July 20, 2020: member

    Concept ACK

  27. in src/policy/rbf.cpp:22 in fad153aff3 outdated 
      18 | @@ -18,16 +19,16 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
  19 |  
  20 |      // If this transaction is not in our mempool, then we can't be sure
  21 |      // we will know about all its inputs.
  22 | -    if (!pool.exists(tx.GetHash())) {
  23 | +    if (!pool || !pool->exists(tx.GetHash())) {
    ariard commented at 10:21 PM on July 20, 2020:

    In fact I think you can do better than returning an UNKNOWN state if you check that all parents are part of coin cache and transaction doesn't explicitly signal you can return a FINAL state ?

  28. ariard commented at 10:44 PM on July 20, 2020: member

    Concept ACK

    As it has been raised above you may want to keep an empty mempool to test correctness of your transactions wrt to policy (testmempoolaccept). If user is willingly to run in blocksonly, turning on another flag doesn't seem a blocker if you're also willingly to disable fee-estimation.

  29. MarcoFalke commented at 7:34 AM on July 21, 2020: member

    ... empty mempool to test correctness of your transactions wrt to policy (testmempoolaccept)

    I don't modify testmempoolaccept behavior in this refactoring pull at all. If you think it should change, I suggest opening a separate behavior-changing pull reqeust. Though, with an empty mempool, a lot of the benefits of testmempoolaccept will be lost. So if the behavior is changed, I recommend that users will have to opt-in to the less-useful empty-mempool behavior.

    In fact I think you can do better than returning an UNKNOWN state if you check that all parents are part of coin cache and transaction doesn't explicitly signal you can return a FINAL state ?

    This function is only called by the wallet and I think it is not feasible to operate a wallet without a mempool, so accommodating that use case seems a bit overkill. Regardless, I suggest to handle this in a separate pull request from this refactor.

  30. in src/init.cpp:683 in faab5976a6 outdated 
     679 | @@ -680,7 +680,7 @@ static void CleanupBlockRevFiles()
 680 |      }
 681 |  }
 682 |  
 683 | -static void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFiles)
 684 | +static void ThreadImport(ChainstateManager& chainman, CTxMemPool& mempool, std::vector<fs::path> vImportFiles)
    jnewbery commented at 1:38 PM on July 21, 2020:

    Why not make this a pointer now if the idea is to eventually make mempool optional?

    promag commented at 8:46 AM on July 23, 2020:

    fa571a9d36fe6f54ff15e5bc9b14df6317fdf53d

    ATM the caller has the mempool so this looks fine to me.

    MarcoFalke commented at 5:16 PM on July 26, 2020:

    Thx, changed to optional mempool

  31. in src/validation.h:167 in faab5976a6 outdated 
     164 | +void UnloadBlockIndex(CTxMemPool& mempool);
 165 |  /** Run an instance of the script checking thread */
 166 |  void ThreadScriptCheck(int worker_num);
 167 |  /** Retrieve a transaction (from memory pool, or from disk, if possible) */
 168 | -bool GetTransaction(const uint256& hash, CTransactionRef& tx, const Consensus::Params& params, uint256& hashBlock, const CBlockIndex* const blockIndex = nullptr);
 169 | +bool GetTransaction(const CTxMemPool& mempool, const uint256& hash, CTransactionRef& tx, const Consensus::Params& params, uint256& hashBlock, const CBlockIndex* const blockIndex = nullptr);
    jnewbery commented at 1:52 PM on July 21, 2020:

    It seems like a slightly odd interface for other components to be passing a mempool reference to a validation method. I think ideally, this would be part of a public interface for validation, which would hold its own mempool reference. For now, this is an improvement over having a mempool global.

  32. in src/rpc/rawtransaction.cpp:243 in faab5976a6 outdated 
     239 | @@ -238,6 +240,8 @@ static UniValue gettxoutproof(const JSONRPCRequest& request)
 240 |                  RPCExamples{""},
 241 |              }.Check(request);
 242 |  
 243 | +    const CTxMemPool& mempool = EnsureMemPool(request.context);
    jnewbery commented at 1:54 PM on July 21, 2020:

    It's odd that we'd be fetching a mempool reference for a function that never needs a mempool (gettxoutproof always refers to transactions that are confirmed in the chain, not in the mempool). Presumably this will be removed at some point in the future when the GetTransaction() interface is cleaned up, or when mempool becomes optional? Would it be worth having a comment here for now?

    MarcoFalke commented at 7:38 AM on July 26, 2020:

    Instead of a comment, I fixed the confusion in #19589

  33. ariard commented at 2:31 PM on July 21, 2020: member

    I don't modify testmempoolaccept behavior in this refactoring pull at all.

    I was aware of this PR not changing behavior , my comment was more an opinion on whether or not we should tight blocksonly and withoutmempool features under same flag in future works.

  34. jnewbery commented at 2:49 PM on July 21, 2020: member

    Code review ACK faab5976a6b168178137675342e94cc4bfbf4595

  35. in src/policy/rbf.cpp:9 in fad153aff3 outdated 
       4 | @@ -5,9 +5,10 @@
   5 |  #include <policy/rbf.h>
   6 |  #include <util/rbf.h>
   7 |  
   8 | -RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
   9 | +namespace {
  10 | +RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool* const pool) NO_THREAD_SAFETY_ANALYSIS
    hebasto commented at 3:39 AM on July 22, 2020:

    fad153aff300d310dcd65539bbcaafbf00fbd167 Could this function differ from https://github.com/bitcoin/bitcoin/blob/fad153aff300d310dcd65539bbcaafbf00fbd167/src/policy/rbf.cpp#L42 by name, not only by signature?

    MarcoFalke commented at 5:17 PM on July 26, 2020:

    why? And, any suggestions I could take?

  36. hebasto approved
  37. hebasto commented at 4:06 AM on July 22, 2020: member

    ACK faab5976a6b168178137675342e94cc4bfbf4595, tested on Linux Mint 20 (x86_64).

  38. DrahtBot added the label Needs rebase on Jul 22, 2020
  39. promag commented at 8:52 AM on July 23, 2020: member

    Concept ACK.

  40. darosior commented at 10:26 AM on July 23, 2020: member

    Concept ACK

  41. practicalswift commented at 5:01 PM on July 25, 2020: contributor

    Concept ACK: decoupling is good

  42. MarcoFalke cross-referenced this on Jul 26, 2020 from issue rpc: Avoid useless mempool query in gettxoutproof by MarcoFalke
  43. MarcoFalke force-pushed on Jul 26, 2020
  44. DrahtBot cross-referenced this on Jul 26, 2020 from issue p2p, refactor: add `CInv` transaction message helpers; use in net processing by jonatack
  45. DrahtBot removed the label Needs rebase on Jul 26, 2020
  46. DrahtBot cross-referenced this on Jul 27, 2020 from issue Enable fetching of orphan parents from wtxid peers by sipa
  47. MarcoFalke force-pushed on Jul 28, 2020
  48. MarcoFalke force-pushed on Jul 28, 2020
  49. MarcoFalke force-pushed on Jul 28, 2020
  50. MarcoFalke cross-referenced this on Jul 28, 2020 from issue Pass mempool pointer to UnloadBlockIndex/GetCoinsCacheSizeState by MarcoFalke
  51. fanquake referenced this in commit a1da180b1b on Jul 28, 2020
  52. DrahtBot cross-referenced this on Jul 28, 2020 from issue Preserve the `LockData` initial state if "potential deadlock detected" exception thrown by hebasto
  53. DrahtBot cross-referenced this on Jul 28, 2020 from issue sync: detect double lock from the same thread by vasild
  54. MarcoFalke force-pushed on Jul 28, 2020
  55. jamesob commented at 2:32 PM on July 28, 2020: member

    Concept ACK, will make some time to review this soon.

  56. MarcoFalke marked this as a draft on Jul 28, 2020
  57. DrahtBot cross-referenced this on Jul 28, 2020 from issue p2p: change `CInv::type` from `int` to `uint32_t`, fix UBSan warning by jonatack
  58. DrahtBot cross-referenced this on Jul 28, 2020 from issue p2p: refactor AlreadyHave(), CInv::type, INV/TX processing by jonatack
  59. sidhujag referenced this in commit 74e94ca8ae on Jul 28, 2020
  60. darosior cross-referenced this on Jul 29, 2020 from issue Disable fee estimation in blocksonly mode (by removing the fee estimates global) by darosior
  61. MarcoFalke referenced this in commit ad2952d17a on Jul 30, 2020
  62. MarcoFalke force-pushed on Jul 30, 2020
  63. MarcoFalke force-pushed on Jul 30, 2020
  64. MarcoFalke force-pushed on Jul 30, 2020
  65. MarcoFalke cross-referenced this on Jul 30, 2020 from issue refactor: Keep mempool interface in validation by MarcoFalke
  66. MarcoFalke force-pushed on Jul 30, 2020
  67. DrahtBot cross-referenced this on Jul 31, 2020 from issue Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp by ryanofsky
  68. practicalswift commented at 3:27 PM on August 14, 2020: contributor

    Concept ACK

  69. MarcoFalke cross-referenced this on Aug 28, 2020 from issue Pass mempool reference to chainstate constructor by MarcoFalke
  70. MarcoFalke referenced this in commit 5c910a6b7a on Aug 31, 2020
  71. MarcoFalke force-pushed on Aug 31, 2020
  72. DrahtBot cross-referenced this on Aug 31, 2020 from issue [net processing] Move Misbehaving() to PeerManager by jnewbery
  73. MarcoFalke cross-referenced this on Aug 31, 2020 from issue Remove mempool global from interfaces by MarcoFalke
  74. MarcoFalke force-pushed on Aug 31, 2020
  75. jnewbery commented at 3:37 PM on August 31, 2020: member

    plz sir, when ready for review?

  76. MarcoFalke commented at 3:40 PM on August 31, 2020: member

    yall pls review #19848

  77. DrahtBot cross-referenced this on Aug 31, 2020 from issue Switch BlockMap to use an unordered_set under the hood by JeremyRubin
  78. sidhujag referenced this in commit db891f64e9 on Aug 31, 2020
  79. DrahtBot cross-referenced this on Sep 1, 2020 from issue ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs
  80. DrahtBot cross-referenced this on Sep 4, 2020 from issue scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion by ryanofsky
  81. DrahtBot cross-referenced this on Sep 4, 2020 from issue Avoid locking CTxMemPool::cs recursively in some cases by hebasto
  82. MarcoFalke referenced this in commit 3ba25e3bdd on Sep 5, 2020
  83. Remove mempool global from init
    Can be reviewed with the git diff options

--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --ignore-all-space
    eeee1104d7
  84. Remove mempool global from p2p fa0359c5b3
  85. Remove mempool global fafb381af8
  86. MarcoFalke force-pushed on Sep 5, 2020
  87. MarcoFalke marked this as ready for review on Sep 5, 2020
  88. MarcoFalke commented at 5:39 PM on September 5, 2020: member

    This is now ready for review

  89. hebasto approved
  90. hebasto commented at 7:45 AM on September 6, 2020: member

    ACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9, I have reviewed the code and it looks OK, I agree it can be merged.

    Hope these changes will make work on transit CTxMemPool::cs from RecursiveMutex to Mutex easier and more straightforward.

  91. in src/validation.cpp:4232 in eeee1104d7 outdated 
    4226 | @@ -4227,6 +4227,14 @@ bool static LoadBlockIndexDB(ChainstateManager& chainman, const CChainParams& ch
4227 |      return true;
4228 |  }
4229 |  
4230 | +void CChainState::LoadMempool(const ArgsManager& args)
4231 | +{
4232 | +    if (args.GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {
    darosior commented at 2:09 PM on September 6, 2020:

    nit: could this be passed as a persistentMempool bool parameter to try to contain the configuration/command line argument logic in init.cpp ? (Even if there is gArgs logic elsewhere in validation.cpp ..)

    MarcoFalke commented at 3:07 PM on September 6, 2020:

    Actually, module-specific arg parsing and configuration should be moved out of init.cpp. For example, the wallet settings have been moved out of init as well.

    darosior commented at 3:22 PM on September 6, 2020:

    Ok, thanks.

  92. in src/validation.cpp:4233 in eeee1104d7 outdated 
    4226 | @@ -4227,6 +4227,14 @@ bool static LoadBlockIndexDB(ChainstateManager& chainman, const CChainParams& ch
4227 |      return true;
4228 |  }
4229 |  
4230 | +void CChainState::LoadMempool(const ArgsManager& args)
4231 | +{
4232 | +    if (args.GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {
4233 | +        ::LoadMempool(m_mempool);
    darosior commented at 2:12 PM on September 6, 2020:

    This is the only call to the standalone LoadMempool function, so --in a follow-up-- its body could be moved here (or as a CTxMempool method) ?

    MarcoFalke commented at 3:04 PM on September 6, 2020:

    There is a fuzzer that uses the standalone function. (Pull might not be merged yet, though)

    MarcoFalke commented at 3:05 PM on September 6, 2020:

    #19259

  93. darosior approved
  94. darosior commented at 8:07 PM on September 6, 2020: member

    ACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9

  95. jnewbery commented at 8:40 PM on September 6, 2020: member

    utACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9

  96. in src/init.cpp:1364 in fafb381af8 
    1359 | @@ -1368,10 +1360,18 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1360 |      node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
1361 |      assert(!node.connman);
1362 |      node.connman = MakeUnique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), args.GetBoolArg("-networkactive", true));
1363 | +
1364 |      // Make mempool generally available in the node context. For example the connection manager, wallet, or RPC threads,
    jnewbery commented at 8:45 PM on September 6, 2020:

    This comment is outdated (the mempool is being constructed, not just made available). If you retouch the PR, can you update the comment?

    MarcoFalke commented at 4:58 AM on September 7, 2020:

    Good point. The comment should probably be removed because now that it is no longer a global, it is clear from the code where it is passed in.

  97. MarcoFalke merged this on Sep 7, 2020
  98. MarcoFalke closed this on Sep 7, 2020
  99. promag commented at 7:56 AM on September 7, 2020: member

    Code review ACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9.

  100. MarcoFalke deleted the branch on Sep 7, 2020
  101. sidhujag referenced this in commit 5cab891acb on Sep 9, 2020
  102. jnewbery cross-referenced this on Apr 29, 2021 from issue Mempool Update Cut-Through Optimization by JeremyRubin
  103. Fabcien referenced this in commit eb27969520 on Jul 8, 2021
  104. deadalnix referenced this in commit 0158b5aeeb on Jul 15, 2021
  105. deadalnix referenced this in commit d23ae6f6ee on Jul 15, 2021
  106. deadalnix referenced this in commit d4e8a4c13f on Jul 15, 2021
  107. Fabcien referenced this in commit d5099f490d on Jul 15, 2021
  108. Fabcien referenced this in commit ae2cf3e1b0 on Jul 15, 2021
  109. Fabcien referenced this in commit 08e10206cf on Jul 15, 2021
  110. bitcoin locked this on Feb 15, 2022
Contributors
MarcoFalkehebastojonatackJeremyRubinDrahtBotnaumenkogsjnewberyariardpromagdarosiorpracticalswiftjamesob
Labels
Refactoring
Linked (view graph)
#18766 Disable fee estimation in blocksonly mode (by removing the fee estimates global)#19259 fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...)#19589 rpc: Avoid useless mempool query in gettxoutproof#19604 Pass mempool pointer to UnloadBlockIndex/GetCoinsCacheSizeState#19629 refactor: Keep mempool interface in validation #19826 Pass mempool reference to chainstate constructor#19848 Remove mempool global from interfaces#21464 Mempool Update Cut-Through Optimization
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