Pass mempool reference to chainstate constructor

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, …)

Pass mempool reference to chainstate constructor fa0572d0f3

MarcoFalke added the label Refactoring on Aug 28, 2020

MarcoFalke added the label Validation on Aug 28, 2020

darosior approved

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…

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.

DrahtBot commented at 11:34 am on August 28, 2020: member

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

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.

hebasto commented at 8:46 am on August 29, 2020: member

Concept ACK

practicalswift commented at 9:13 am on August 29, 2020: contributor

Concept ACK: decoupling is good^Wgreat!

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

Code review ACK fa0572d0f3b083b4c8e2e883a66e2b198c6779f1.

darosior approved

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 :).

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:

0static void UpdateMempoolForReorg(CTxMemPool& mempool, DisconnectedBlockTransactions& disconnectpool, bool fAddToMempool)
1    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.

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:

0    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.

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:

0    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.

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:

0            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.

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:

0            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.

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:

0    bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool)
1        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.

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:

0    bool ActivateBestChainStep(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace)
1        EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_mempool.cs);
2    bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool)
3        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.

hebasto approved

hebasto commented at 9:48 am on August 30, 2020: member

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

fjahr commented at 4:02 pm on August 30, 2020: member

Code review ACK b224f98033992cf6d84c5e8d1d81c4e361b88135

MarcoFalke merged this on Aug 31, 2020

MarcoFalke closed this on Aug 31, 2020

MarcoFalke deleted the branch on Aug 31, 2020

sidhujag referenced this in commit db891f64e9 on Aug 31, 2020

deadalnix referenced this in commit 0158b5aeeb on Jul 15, 2021

DrahtBot locked this on Feb 15, 2022

Pass mempool reference to chainstate constructor #19826

Conflicts