Pass mempool reference to chainstate constructor #19826

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2008-valMemRef changing 7 files +55 −37
  1. MarcoFalke commented at 8:43 AM on August 28, 2020: member

    Next step toward #19556

    Instead of relying on the mempool global, each chainstate is given a reference to a mempool to keep up to date with the tip (block connections, disconnections, reorgs, ...)

  2. Pass mempool reference to chainstate constructor fa0572d0f3
  3. MarcoFalke added the label Refactoring on Aug 28, 2020
  4. MarcoFalke added the label Validation on Aug 28, 2020
  5. darosior approved
  6. darosior commented at 10:21 AM on August 28, 2020: member

    Concept ACK

    Will review / test soon.

    edit: didn't want to make it an "approve" review...

  7. in src/validation.cpp:2870 in fa0572d0f3 
    2874 | @@ -2867,7 +2875,8 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
2875 |          LimitValidationInterfaceQueue();
2876 |  
2877 |          {
2878 | -            LOCK2(cs_main, ::mempool.cs); // Lock transaction pool for at least as long as it takes for connectTrace to be consumed
    promag commented at 10:36 AM on August 28, 2020:

    We want to ditch LOCK2?

    MarcoFalke commented at 10:44 AM on August 28, 2020:

    No it can stay, but here LOCK2 doesn't support pointers or interfaces, so if someone wants to pick up #19629 again, LOCK(cs_main) will likely end up to be separate anyway.

  8. DrahtBot commented at 11:34 AM on August 28, 2020: member

    <!--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:

    • #19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
    • #18731 (refactor: Make CCheckQueue RAII-styled by hebasto)

    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.

  9. hebasto commented at 8:46 AM on August 29, 2020: member

    Concept ACK

  10. practicalswift commented at 9:13 AM on August 29, 2020: contributor

    Concept ACK: decoupling is good^Wgreat!

  11. promag commented at 11:08 AM on August 29, 2020: member

    Code review ACK fa0572d0f3b083b4c8e2e883a66e2b198c6779f1.

  12. darosior approved
  13. darosior commented at 2:21 PM on August 29, 2020: member

    ACK fa0572d0f3b083b4c8e2e883a66e2b198c6779f1

    Reviewed, and tested against #18766 rebased on top of (a rebased version of) #19556. A nice upside of using a reference is that if you intend to do the same for the node context's mempool, it would substantially simplify #18766 :).

  14. in src/validation.cpp:373 in fa0572d0f3 
     369 | @@ -370,9 +370,10 @@ static bool IsCurrentForFeeEstimation() EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 370 |   * and instead just erase from the mempool as needed.
 371 |   */
 372 |  
 373 | -static void UpdateMempoolForReorg(DisconnectedBlockTransactions& disconnectpool, bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, ::mempool.cs)
 374 | +static void UpdateMempoolForReorg(CTxMemPool& mempool, DisconnectedBlockTransactions& disconnectpool, bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, mempool.cs)
    hebasto commented at 9:38 AM on August 30, 2020:

    While touching this line maybe explicitly mention the global namespace for cs_main:

    static void UpdateMempoolForReorg(CTxMemPool& mempool, DisconnectedBlockTransactions& disconnectpool, bool fAddToMempool)
    EXCLUSIVE_LOCKS_REQUIRED(::cs_main, mempool.cs)

    ?

    MarcoFalke commented at 10:13 AM on August 30, 2020:

    Thanks. Will add the :: when I have to retouch or on the next commit that touches those lines.

  15. in src/validation.cpp:2491 in fa0572d0f3 
    2485 | @@ -2485,8 +2486,11 @@ void static UpdateTip(const CBlockIndex* pindexNew, const CChainParams& chainPar
2486 |    * disconnectpool (note that the caller is responsible for mempool consistency
2487 |    * in any case).
2488 |    */
2489 | -bool CChainState::DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions *disconnectpool)
2490 | +bool CChainState::DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool)
2491 |  {
2492 | +    AssertLockHeld(cs_main);
    hebasto commented at 9:41 AM on August 30, 2020:

    nit:

        AssertLockHeld(::cs_main);
    MarcoFalke commented at 10:13 AM on August 30, 2020:

    Thanks. Will add the :: when I have to retouch or on the next commit that touches those lines.

  16. in src/validation.cpp:2592 in fa0572d0f3 
    2588 | @@ -2585,6 +2589,9 @@ class ConnectTrace {
2589 |   */
2590 |  bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool)
2591 |  {
2592 | +    AssertLockHeld(cs_main);
    hebasto commented at 9:42 AM on August 30, 2020:

    nit:

        AssertLockHeld(::cs_main);
    MarcoFalke commented at 10:13 AM on August 30, 2020:

    Thanks. Will add the :: when I have to retouch or on the next commit that touches those lines.

  17. in src/validation.cpp:2878 in fa0572d0f3 
    2874 | @@ -2867,7 +2875,8 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
2875 |          LimitValidationInterfaceQueue();
2876 |  
2877 |          {
2878 | -            LOCK2(cs_main, ::mempool.cs); // Lock transaction pool for at least as long as it takes for connectTrace to be consumed
2879 | +            LOCK(cs_main);
    hebasto commented at 9:42 AM on August 30, 2020:

    nit:

                LOCK(::cs_main);
    MarcoFalke commented at 10:13 AM on August 30, 2020:

    Thanks. Will add the :: when I have to retouch or on the next commit that touches those lines.

  18. in src/validation.cpp:4529 in fa0572d0f3 
    4525 | @@ -4517,7 +4526,8 @@ bool CChainState::RewindBlockIndex(const CChainParams& params)
4526 |      // Loop until the tip is below nHeight, or we reach a pruned block.
4527 |      while (!ShutdownRequested()) {
4528 |          {
4529 | -            LOCK2(cs_main, ::mempool.cs);
4530 | +            LOCK(cs_main);
    hebasto commented at 9:43 AM on August 30, 2020:

    nit:

                LOCK(::cs_main);
    MarcoFalke commented at 10:13 AM on August 30, 2020:

    Thanks. Will add the :: when I have to retouch or on the next commit that touches those lines.

  19. in src/validation.h:648 in fa0572d0f3 
     644 | @@ -642,7 +645,7 @@ class CChainState {
 645 |                        CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 646 |  
 647 |      // Apply the effects of a block disconnection on the UTXO set.
 648 | -    bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, ::mempool.cs);
 649 | +    bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs);
    hebasto commented at 9:44 AM on August 30, 2020:

    nit:

        bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool)
        EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_mempool.cs);
    MarcoFalke commented at 10:13 AM on August 30, 2020:

    Thanks. Will add the :: when I have to retouch or on the next commit that touches those lines.

  20. in src/validation.h:692 in fa0572d0f3 
     687 | @@ -685,8 +688,8 @@ class CChainState {
 688 |      std::string ToString() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 689 |  
 690 |  private:
 691 | -    bool ActivateBestChainStep(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, ::mempool.cs);
 692 | -    bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, ::mempool.cs);
 693 | +    bool ActivateBestChainStep(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs);
 694 | +    bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs);
    hebasto commented at 9:45 AM on August 30, 2020:

    nit:

        bool ActivateBestChainStep(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace)
        EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_mempool.cs);
    bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool)
        EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_mempool.cs);
    MarcoFalke commented at 10:14 AM on August 30, 2020:

    Thanks. Will add the :: when I have to retouch or on the next commit that touches those lines.

  21. hebasto approved
  22. hebasto commented at 9:48 AM on August 30, 2020: member

    ACK fa0572d0f3b083b4c8e2e883a66e2b198c6779f1, reviewed and tested on Linux Mint 20 (x86_64).

  23. fjahr commented at 4:02 PM on August 30, 2020: member

    Code review ACK b224f98033992cf6d84c5e8d1d81c4e361b88135

  24. MarcoFalke merged this on Aug 31, 2020
  25. MarcoFalke closed this on Aug 31, 2020
  26. MarcoFalke deleted the branch on Aug 31, 2020
  27. sidhujag referenced this in commit db891f64e9 on Aug 31, 2020
  28. deadalnix referenced this in commit 0158b5aeeb on Jul 15, 2021
  29. DrahtBot locked this on Feb 15, 2022
Contributors
MarcoFalkedarosiorpromagDrahtBothebastopracticalswiftfjahr
Labels
RefactoringValidation
Linked (view graph)
#19556 Remove mempool global
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-02 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me