Make m_mempool optional in CChainState #22415

jamesob commented at 7:55 PM on July 7, 2021: member

Make CChainState::m_mempool optional by making it a pointer instead of a reference. This will allow a simplification to assumeutxo semantics (see #15606#pullrequestreview-692965905) and help facilitate the -nomempool option.

jamesob force-pushed on Jul 7, 2021

DrahtBot added the label Validation on Jul 7, 2021

in src/validation.cpp:2576 in 349ab2843e outdated

2569 | @@ -2565,12 +2570,12 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, CBlockIndex
2570 |          }
2571 |      }
2572 |  
2573 | -    if (fBlocksDisconnected) {
2574 | +    if (fBlocksDisconnected && m_mempool) {
2575 |          // If any blocks were disconnected, disconnectpool may be non empty.  Add
2576 |          // any disconnected transactions back to the mempool.
2577 | -        UpdateMempoolForReorg(*this, m_mempool, disconnectpool, true);
2578 | +        mempoolHandleReorg(disconnectpool, false);

MarcoFalke commented at 8:54 PM on July 7, 2021:

why the change from true -> false?

jamesob commented at 1:31 AM on July 8, 2021:

Hey somebody's gotta keep review fun!

in src/validation.cpp:2651 in 349ab2843e outdated

2646 | @@ -2642,7 +2647,8 @@ bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr
2647 |  
2648 |          {
2649 |              LOCK(cs_main);
2650 | -            LOCK(m_mempool.cs); // Lock transaction pool for at least as long as it takes for connectTrace to be consumed
2651 | +            // Lock transaction pool for at least as long as it takes for connectTrace to be consumed
2652 | +            if (m_mempool) LOCK(m_mempool->cs);

MarcoFalke commented at 8:55 PM on July 7, 2021:

Nice try, but leaving out the {} won't prevent the lock from being closed in the same line.

jamesob commented at 1:32 AM on July 8, 2021:

Oh yeah d'oh. Sure could've used those missing annotations there.

in src/validation.cpp:2808 in 349ab2843e outdated

2802 | @@ -2792,7 +2803,9 @@ bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pind
2803 |          LimitValidationInterfaceQueue();
2804 |  
2805 |          LOCK(cs_main);
2806 | -        LOCK(m_mempool.cs); // Lock for as long as disconnectpool is in scope to make sure UpdateMempoolForReorg is called after DisconnectTip without unlocking in between
2807 | +        // Lock for as long as disconnectpool is in scope to make sure UpdateMempoolForReorg is
2808 | +        // called after DisconnectTip without unlocking in between
2809 | +        if (m_mempool) LOCK(m_mempool->cs);

MarcoFalke commented at 8:56 PM on July 7, 2021:

same

in src/validation.cpp:4509 in 349ab2843e outdated

4505 | @@ -4492,6 +4506,41 @@ bool CChainState::ResizeCoinsCaches(size_t coinstip_size, size_t coinsdb_size)
4506 |      return ret;
4507 |  }
4508 |  
4509 | +void CChainState::mempoolHandleReorg(DisconnectedBlockTransactions& disconnectpool, bool add_to_mempool) {

MarcoFalke commented at 8:56 PM on July 7, 2021:

void CChainState::mempoolHandleReorg(DisconnectedBlockTransactions& disconnectpool, bool add_to_mempool)
{

properly format new code?

in src/validation.h:604 in 349ab2843e outdated

 600 | @@ -600,7 +601,7 @@ class CChainState
 601 |      //! CChainState instances.
 602 |      BlockManager& m_blockman;
 603 |  
 604 | -    explicit CChainState(CTxMemPool& mempool, BlockManager& blockman, std::optional<uint256> from_snapshot_blockhash = std::nullopt);
 605 | +    explicit CChainState(CTxMemPool* mempool, BlockManager& blockman, std::optional<uint256> from_snapshot_blockhash = std::nullopt);

MarcoFalke commented at 8:57 PM on July 7, 2021:

Would it be possible to combine the args so that either a mempool, or a hash can be passed, but not both? I think std::variant can do this

jamesob commented at 4:18 PM on July 8, 2021:

Not going this route for reasons mentioned in a recent comment: passing both is a valid approach if we are initializing a not-yet-to-tip snapshot chainstate in init.cpp.

MarcoFalke commented at 8:58 PM on July 7, 2021: member

Concept ACK

ryanofsky commented at 11:24 PM on July 7, 2021: member

Concept ACK. I think you can drop the second commit and add a LOCK_RETURNED helper method to be able to keep using lock annotations when m_mempool is null:

RecursiveMutex* MempoolMutex() const LOCK_RETURNED(m_mempool->cs) { return m_mempool ? &m_mempool->cs : nullptr; }

Seems to work for me in commit 8db5953e2d666fb9c25ae8f0df5b4c5c5897cd58

DrahtBot commented at 12:48 AM on July 8, 2021: 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:

#22371 by MarcoFalke
#21526 by jamesob
#21464 by JeremyRubin

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.

jamesob force-pushed on Jul 8, 2021

jamesob commented at 2:05 AM on July 8, 2021: member

Rebased to remove the second commit per Russ' suggestion, which also addresses most of Marco's feedback.

Seems to work for me in commit 8db5953

Wow, that was wizardly. Thanks @ryanofsky!

I think std::variant can do this

I'll look into this tomorrow.

in src/validation.h:923 in b17bb4c37d outdated

 912 | @@ -907,7 +913,7 @@ class ChainstateManager
 913 |      //                                  constructor
 914 |      //! @param[in] snapshot_blockhash   If given, signify that this chainstate
 915 |      //!                                 is based on a snapshot.
 916 | -    CChainState& InitializeChainstate(CTxMemPool& mempool, const std::optional<uint256>& snapshot_blockhash = std::nullopt)
 917 | +    CChainState& InitializeChainstate(CTxMemPool* mempool, const std::optional<uint256>& snapshot_blockhash = std::nullopt)

naumenkogs commented at 9:45 AM on July 8, 2021:

Similarly to this comment, can we somehow reflect that if mempool is nullptr, the second argument doesn't make sense?

naumenkogs commented at 9:46 AM on July 8, 2021: member

Light code review ACK b17bb4c37d58ca49e877983a632bfcaa493be993

in src/validation.h:803 in b17bb4c37d outdated

 798 | @@ -798,6 +799,11 @@ class CChainState
 799 |  
 800 |      bool LoadBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 801 |  
 802 | +    //! Indirection necessary to make lock annotations work with an optional mempool.
 803 | +    RecursiveMutex* MempoolMutex() const LOCK_RETURNED(m_mempool->cs) {

MarcoFalke commented at 12:27 PM on July 8, 2021:

    RecursiveMutex* MempoolMutex() const LOCK_RETURNED(m_mempool->cs)
    {

clang-format?

MarcoFalke commented at 12:27 PM on July 8, 2021: member

Nice solution with the LOCK_RETURNED

in src/init.cpp:1352 in b17bb4c37d outdated

1348 | @@ -1349,7 +1349,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1349 |              const int64_t load_block_index_start_time = GetTimeMillis();
1350 |              try {
1351 |                  LOCK(cs_main);
1352 | -                chainman.InitializeChainstate(*Assert(node.mempool));
1353 | +                chainman.InitializeChainstate(Assert(node.mempool).get());

ryanofsky commented at 2:31 PM on July 8, 2021:

In commit "validation: make CChainState::m_mempool optional" (b17bb4c37d58ca49e877983a632bfcaa493be993)

Might be a little more obviously safe to write Assert(node.mempool.get()) instead of Assert(node.mempool).get() since that version looks like the Assert function might be returning a temporary unique_ptr that would delete the mempool.

jamesob commented at 4:18 PM on July 8, 2021:

Fixed, thanks.

jonatack commented at 2:52 PM on July 8, 2021: member

Concept ACK, will review on next push.

in src/validation.cpp:2252 in b17bb4c37d outdated

2256 | @@ -2254,7 +2257,7 @@ static void UpdateTip(CTxMemPool& mempool, const CBlockIndex* pindexNew, const C
2257 |  bool CChainState::DisconnectTip(BlockValidationState& state, DisconnectedBlockTransactions* disconnectpool)
2258 |  {
2259 |      AssertLockHeld(cs_main);
2260 | -    AssertLockHeld(m_mempool.cs);
2261 | +    if (m_mempool) AssertLockHeld(m_mempool->cs);

ryanofsky commented at 2:56 PM on July 8, 2021:

In commit "validation: make CChainState::m_mempool optional" (b17bb4c37d58ca49e877983a632bfcaa493be993)

Note: It could be nice to be able to write AssertLockHeld(MempoolMutex() for consistency with LOCK(MempoolMutex()) and to avoid the if statement. But it'd be beyond scope of this PR since it'd require changing AssertLockHeld and there are other issues with that function anyway.

in src/validation.cpp:4775 in b17bb4c37d outdated

4771 | @@ -4763,7 +4772,7 @@ bool ChainstateManager::ActivateSnapshot(
4772 |      }
4773 |  
4774 |      auto snapshot_chainstate = WITH_LOCK(::cs_main, return std::make_unique<CChainState>(
4775 | -            this->ActiveChainstate().m_mempool, m_blockman, base_blockhash));
4776 | +            nullptr, m_blockman, base_blockhash));

ryanofsky commented at 2:59 PM on July 8, 2021:

In commit "validation: make CChainState::m_mempool optional" (b17bb4c37d58ca49e877983a632bfcaa493be993)

Note: Naively I'd expect the mempool pointer for the activated snapshot to be non-null, but I guess it could be set later when the snapshot is fully initialized.

jonatack commented at 4:57 PM on July 8, 2021:

(if you retouch)

            /* mempool */ nullptr, m_blockman, base_blockhash));

jnewbery commented at 11:09 AM on July 12, 2021:

I was also surprised by this. Is the snapshot chainstate the one that eventually becomes our tip? In that case, don't we want it to have a mempool?

jamesob commented at 5:13 PM on July 12, 2021:

Yeah, this is a little confusing absent context, but basically the idea per @ryanofsky's feedback is that mempool presence should be used to indicate chainstate activeness, so we don't want to give the snapshot chainstate a mempool until we consider it active.

ryanofsky approved

ryanofsky commented at 3:08 PM on July 8, 2021: member

Code review ACK b17bb4c37d58ca49e877983a632bfcaa493be993

jamesob force-pushed on Jul 8, 2021

jamesob commented at 4:13 PM on July 8, 2021: member

Rebased.

Note: Naively I'd expect the mempool pointer for the activated snapshot to be non-null, but I guess it could be set later when the snapshot is fully initialized.

This is a good point. I'd come up with a commit that implements the std::variant feedback (and am glad to have for learning about that), but @ryanofsky's point here makes that change inappropriate: with assumeutxo, we may actually want the snapshot chain to have a non-null mempool if we're in, say, init.cpp and loading a snapshot chainstate that is active but not yet at tip.

I've covered all of the other feedback aside from the lock assertion for the reasons Russ mentioned; maybe we can get around to it later.

ryanofsky approved

ryanofsky commented at 4:19 PM on July 8, 2021: member

Code review ACK 8511429f3bc480ddeef06c23810b3c47fa8513ee. Just some minor tweaks since last review: whitespace, unique_ptr assert, null mempool args

jonatack commented at 5:10 PM on July 8, 2021: member

ACK 8511429f3bc480ddeef06c23810b3c47fa8513ee code review, debug build, running bitcoind on signet as a sanity check, operation nominal

in src/validation.h:923 in 8511429f3b outdated

 913 | @@ -907,7 +914,7 @@ class ChainstateManager
 914 |      //                                  constructor
 915 |      //! @param[in] snapshot_blockhash   If given, signify that this chainstate
 916 |      //!                                 is based on a snapshot.
 917 | -    CChainState& InitializeChainstate(CTxMemPool& mempool, const std::optional<uint256>& snapshot_blockhash = std::nullopt)
 918 | +    CChainState& InitializeChainstate(CTxMemPool* mempool, const std::optional<uint256>& snapshot_blockhash = std::nullopt)

naumenkogs commented at 6:41 AM on July 9, 2021:

What do you think about making this std::optional<CTxMemPool, std::optional>?

In this case, snapshot_blockhash can't be possibly specified without mempool, and that's what I originally suggested.

jamesob commented at 5:21 PM on July 9, 2021:

There are cases when we want to call this with a snapshot blockhash but not a mempool - during snapshot load time when we need a chainstate instance to populate/validate but don't want to yet make it the active chainstate in case loading the UTXO snapshot fails.

in src/validation.cpp:2057 in 8511429f3b outdated

2053 | @@ -2053,7 +2054,7 @@ bool CChainState::FlushStateToDisk(
2054 |          bool fFlushForPrune = false;
2055 |          bool fDoFullFlush = false;
2056 |  
2057 | -        CoinsCacheSizeState cache_state = GetCoinsCacheSizeState(&m_mempool);
2058 | +        CoinsCacheSizeState cache_state = GetCoinsCacheSizeState(m_mempool);

jnewbery commented at 10:59 AM on July 9, 2021:

Since you're updating both call sites for CChainState::GetCoinsCacheSizeState(const CTxMemPool* tx_pool), maybe it makes sense to just remove the argument. There's no need to pass a data member of CChainState to a member function of CChainState - it can just use its own m_mempool.

jamesob commented at 5:16 PM on July 9, 2021:

Good call! Done.

in src/validation.cpp:2299 in 8511429f3b outdated

2290 | @@ -2288,7 +2291,7 @@ bool CChainState::DisconnectTip(BlockValidationState& state, DisconnectedBlockTr
2291 |          while (disconnectpool->DynamicMemoryUsage() > MAX_DISCONNECTED_TX_POOL_SIZE * 1000) {
2292 |              // Drop the earliest entry, and remove its children from the mempool.
2293 |              auto it = disconnectpool->queuedTx.get<insertion_order>().begin();
2294 | -            m_mempool.removeRecursive(**it, MemPoolRemovalReason::REORG);
2295 | +            if (m_mempool) m_mempool->removeRecursive(**it, MemPoolRemovalReason::REORG);

jnewbery commented at 11:13 AM on July 9, 2021:

This if (m_mempool) condition can be at the level of the if (disconnectpool) above:

         return false;
     }
 
-    if (disconnectpool) {
+    if (m_mempool && disconnectpool) {
         // Save transactions to re-add to mempool at end of reorg
         for (auto it = block.vtx.rbegin(); it != block.vtx.rend(); ++it) {
             disconnectpool->addTransaction(*it);
@@ -2291,7 +2291,7 @@ bool CChainState::DisconnectTip(BlockValidationState& state, DisconnectedBlockTr
         while (disconnectpool->DynamicMemoryUsage() > MAX_DISCONNECTED_TX_POOL_SIZE * 1000) {
             // Drop the earliest entry, and remove its children from the mempool.
             auto it = disconnectpool->queuedTx.get<insertion_order>().begin();
-            if (m_mempool) m_mempool->removeRecursive(**it, MemPoolRemovalReason::REORG);
+            m_mempool->removeRecursive(**it, MemPoolRemovalReason::REORG);
             disconnectpool->removeEntry(it);
         }
     }

That would potentially save some work updating the disconnectpool data structure, which will never get used.

Even better (but not necessarily part of this PR) would be to only create a DisconnectedBlockTransactions if m_mempool is non-null, and then update UpdateMempoolForReorg() and DisconnectedBlockTransactions() to take optional DisconnectedBlockTransactions*. There's no need to create and maintain a DisconnectedBlockTransactions if we're never going to use it.

jamesob commented at 5:18 PM on July 9, 2021:

Yup, also a good call. Fixed. Initially wasn't sure if there was some other purpose we might be using the disconnectpool for (since it's sometimes passed in as a mutable parameter) but after looking around it's clearly just used for mempool maintenance.

in src/validation.cpp:2209 in 8511429f3b outdated

2205 | @@ -2205,11 +2206,13 @@ static void AppendWarning(bilingual_str& res, const bilingual_str& warn)
2206 |  }
2207 |  
2208 |  /** Check warning conditions and do some notifications on new chain tip set. */
2209 | -static void UpdateTip(CTxMemPool& mempool, const CBlockIndex* pindexNew, const CChainParams& chainParams, CChainState& active_chainstate)
2210 | +static void UpdateTip(CTxMemPool* mempool, const CBlockIndex* pindexNew, const CChainParams& chainParams, CChainState& active_chainstate)

jnewbery commented at 11:16 AM on July 9, 2021:

This could be a member of CChainState to avoid passing mempool, chainParams and active_chainstate.

jamesob commented at 5:18 PM on July 9, 2021:

Agreed, done.

in src/validation.cpp:2408 in 8511429f3b outdated

2403 | @@ -2401,7 +2404,7 @@ bool CChainState::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew
2404 |      int64_t nTime5 = GetTimeMicros(); nTimeChainState += nTime5 - nTime4;
2405 |      LogPrint(BCLog::BENCH, "  - Writing chainstate: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime5 - nTime4) * MILLI, nTimeChainState * MICRO, nTimeChainState * MILLI / nBlocksTotal);
2406 |      // Remove conflicting transactions from the mempool.;
2407 | -    m_mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight);
2408 | +    if (m_mempool) m_mempool->removeForBlock(blockConnecting.vtx, pindexNew->nHeight);
2409 |      disconnectpool.removeForBlock(blockConnecting.vtx);

jnewbery commented at 11:19 AM on July 9, 2021:

No need to update disconnectpool if the mempool doesn't exist:

    if (m_mempool) {
        m_mempool->removeForBlock(blockConnecting.vtx, pindexNew->nHeight);
        disconnectpool.removeForBlock(blockConnecting.vtx);
    }

jamesob commented at 5:18 PM on July 9, 2021:

:+1:

in src/validation.cpp:2513 in 8511429f3b outdated

2509 | @@ -2507,7 +2510,7 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, CBlockIndex
2510 |          if (!DisconnectTip(state, &disconnectpool)) {
2511 |              // This is likely a fatal error, but keep the mempool consistent,
2512 |              // just in case. Only remove from the mempool in this case.
2513 | -            UpdateMempoolForReorg(*this, m_mempool, disconnectpool, false);
2514 | +            if (m_mempool) UpdateMempoolForReorg(*this, *m_mempool, disconnectpool, false);

jnewbery commented at 11:30 AM on July 9, 2021:

What do you think about updating UpdateMempoolForReorg to either be a member of CChainState or take CTxMemPool as an optional pointer, and then return immediately if mempool is null. That would avoid scattering so many if (m_mempool) checks across multiple validation functions.

jamesob commented at 5:19 PM on July 9, 2021:

I went ahead and threw it onto CChainState since I think that makes sense; can strip off the commit if there are any objections.

jnewbery commented at 11:33 AM on July 9, 2021: member

Concept ACK. A few non-blocking suggestions inline.

jamesob force-pushed on Jul 9, 2021

jamesob commented at 5:24 PM on July 9, 2021: member

Thanks for the prompt attention, good people. I've pushed a rebase that includes pretty much all of @jnewbery's feedback. This widens the scope of the PR to encompass a bit of refactoring, but I think it's all reasonable stuff to include here.

in src/validation.cpp:346 in 7cdcd08d66 outdated

 341 | @@ -342,11 +342,15 @@ static bool IsCurrentForFeeEstimation(CChainState& active_chainstate) EXCLUSIVE_
 342 |   * Passing fAddToMempool=false will skip trying to add the transactions back,
 343 |   * and instead just erase from the mempool as needed.
 344 |   */
 345 | -
 346 | -static void UpdateMempoolForReorg(CChainState& active_chainstate, CTxMemPool& mempool, DisconnectedBlockTransactions& disconnectpool, bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, mempool.cs)
 347 | +void CChainState::UpdateMempoolForReorg(
 348 | +    CChainState& active_chainstate,

ryanofsky commented at 6:21 PM on July 9, 2021:

In commit "refactor: move UpdateMempoolForReorg into CChainState" (7cdcd08d665570167b06a93e003369f7fa833cec)

active_chainstate is always *this right? Can the parameter be dropped?

jamesob commented at 6:49 PM on July 9, 2021:

Good catch, thanks. Always weary to assume active_chainstate == *this, but in this case (as you probably realized) only the active ever has a mempool, so it's a fine substitution.

ryanofsky commented at 6:28 PM on July 9, 2021: member

Code review ACK 29acd850fb0539b8358a451089ef7b1c583dd1a1. Simplifications from John make the change nicer.

jamesob force-pushed on Jul 9, 2021

jamesob commented at 6:50 PM on July 9, 2021: member

Rebase to incorporate minor changes from @ryanofsky's feedback (https://github.com/bitcoin/bitcoin/pull/22415#discussion_r667134941).

in src/validation.cpp:2294 in ad911753a1 outdated

2291 |          }
2292 |          while (disconnectpool->DynamicMemoryUsage() > MAX_DISCONNECTED_TX_POOL_SIZE * 1000) {
2293 |              // Drop the earliest entry, and remove its children from the mempool.
2294 |              auto it = disconnectpool->queuedTx.get<insertion_order>().begin();
2295 | -            m_mempool.removeRecursive(**it, MemPoolRemovalReason::REORG);
2296 | +            if (m_mempool) m_mempool->removeRecursive(**it, MemPoolRemovalReason::REORG);

naumenkogs commented at 8:02 AM on July 12, 2021:

This new if here is not needed, it's augmented by the check you added above (if (disconnectpool && m_mempool) {)?

jamesob commented at 6:43 PM on July 12, 2021:

Good catch, thanks.

in src/validation.cpp:345 in 1e0262e040 outdated

 341 | @@ -342,11 +342,14 @@ static bool IsCurrentForFeeEstimation(CChainState& active_chainstate) EXCLUSIVE_
 342 |   * Passing fAddToMempool=false will skip trying to add the transactions back,
 343 |   * and instead just erase from the mempool as needed.
 344 |   */
 345 | -
 346 | -static void UpdateMempoolForReorg(CChainState& active_chainstate, CTxMemPool& mempool, DisconnectedBlockTransactions& disconnectpool, bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, mempool.cs)
 347 | +void CChainState::UpdateMempoolForReorg(

naumenkogs commented at 8:09 AM on July 12, 2021:

Mixed feelings about this change. I like params reduction, but I think a reader might get confused: "UpdateMempoolForReorg? But I don't have mempool."

I would slightly prefer changing the name to UpdateMempoolForReorgIfExists or TryUpdateMempoolForReorg, but feel free to ignore.

jamesob commented at 6:43 PM on July 12, 2021:

Sounds good, fixed.

in src/validation.h:592 in e34a992bae outdated

 586 | @@ -587,8 +587,9 @@ class CChainState
 587 |       */
 588 |      mutable std::atomic<bool> m_cached_finished_ibd{false};
 589 |  
 590 | -    //! mempool that is kept in sync with the chain
 591 | -    CTxMemPool& m_mempool;
 592 | +    //! Optional mempool that is kept in sync with the chain.
 593 | +    //! Only the active chainstate has a mempool.
 594 | +    CTxMemPool* m_mempool;

jnewbery commented at 11:03 AM on July 12, 2021:

This could be a const pointer:

    CTxMemPool* const m_mempool;

since it's set in the initializer list and never modified.

jamesob commented at 5:16 PM on July 12, 2021:

The pointer may actually be nulled out e.g. when we transition the initial IBD chainstate to the background (and give its mempool to the snapshot chainstate, which becomes our active).

jnewbery commented at 5:33 PM on July 12, 2021:

ah, ok! I'm not totally up to date on how the different chainstate objects work in assumeutxo.

in src/validation.cpp:332 in e34a992bae outdated

 328 | @@ -329,7 +329,8 @@ static bool IsCurrentForFeeEstimation(CChainState& active_chainstate) EXCLUSIVE_
 329 |      return true;
 330 |  }
 331 |  
 332 | -/* Make mempool consistent after a reorg, by re-adding or recursively erasing
 333 | +/**

jnewbery commented at 11:11 AM on July 12, 2021:

nit: doxygen function comments should go with the function declaration. Do you mind moving this to the header file now that the declaration is there?

jamesob commented at 6:42 PM on July 12, 2021:

Fixed, thanks!

in src/validation.h:811 in e34a992bae outdated

 806 | +    void UpdateMempoolForReorg(
 807 | +        DisconnectedBlockTransactions& disconnectpool,
 808 | +        bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
 809 | +
 810 | +    /** Check warning conditions and do some notifications on new chain tip set. */
 811 | +    void UpdateTip(const CBlockIndex* pindexNew, CChainState& active_chainstate)

jnewbery commented at 11:18 AM on July 12, 2021:

No need for the CChainState& active_chainstate argument - this is a private method in CChainState where the active_chainstate arg is always *this. You can remove the argument and use the implicit this inside the function.

jamesob commented at 6:00 PM on July 12, 2021:

Good call - weird, thought I had made that change... maybe lost in rebase shuffle.

jnewbery commented at 11:18 AM on July 12, 2021: member

Looking good. Just a few small comments inline.

jamesob force-pushed on Jul 12, 2021

ryanofsky approved

ryanofsky commented at 8:56 PM on July 12, 2021: member

Code review ACK c13e832af4a044a5c4beb4694022df53fa2eaf6d

in src/validation.h:822 in c13e832af4 outdated

 817 | +     * in-mempool descendants of such transactions would be removed).
 818 | +     *
 819 | +     * Passing fAddToMempool=false will skip trying to add the transactions back,
 820 | +     * and instead just erase from the mempool as needed.
 821 | +     */
 822 | +    void TryUpdateMempoolForReorg(

jnewbery commented at 12:06 PM on July 13, 2021:

This new name seems worse to me than the original (UpdateMempoolForReorg()). The Try suggests that this operation may fail or that it's best-effort. In fact, if a mempool exists then it really will be updated to be internally consistent.

jamesob commented at 1:42 PM on July 13, 2021:

If you felt this way it would have been helpful for you to chime in on @naumenkogs's original comment; doing commits back and forth on relatively cosmetic things like method names is not a good use of energy.

jnewbery commented at 2:20 PM on July 13, 2021:

Sorry - I didn't see the original comment. Ultimately it's up to you what you put in your PR. I don't think that renaming this function is necessary, and is actually worse for the reasons I gave here, but if other people prefer the new name, that's also fine!

in src/validation.cpp:2216 in c13e832af4 outdated

2212 | @@ -2218,11 +2213,11 @@ static void UpdateTip(CTxMemPool& mempool, const CBlockIndex* pindexNew, const C
2213 |      }
2214 |  
2215 |      bilingual_str warning_messages;
2216 | -    if (!active_chainstate.IsInitialBlockDownload()) {
2217 | +    if (!this->IsInitialBlockDownload()) {

jnewbery commented at 12:09 PM on July 13, 2021:

There's no need to use this-> to call member functions. this-> is implicitly added before the names: https://en.cppreference.com/w/cpp/language/this.

jamesob commented at 1:26 PM on July 13, 2021:

I prefer the this-> prefix to be explicit about a member function call; last time I talked about this with others it was uncontroversial.

jnewbery commented at 1:57 PM on July 13, 2021:

Curious. I've never come across this style before.

in src/validation.cpp:2235 in c13e832af4 outdated

2231 | @@ -2237,14 +2232,14 @@ static void UpdateTip(CTxMemPool& mempool, const CBlockIndex* pindexNew, const C
2232 |        pindexNew->GetBlockHash().ToString(), pindexNew->nHeight, pindexNew->nVersion,
2233 |        log(pindexNew->nChainWork.getdouble())/log(2.0), (unsigned long)pindexNew->nChainTx,
2234 |        FormatISO8601DateTime(pindexNew->GetBlockTime()),
2235 | -      GuessVerificationProgress(chainParams.TxData(), pindexNew), active_chainstate.CoinsTip().DynamicMemoryUsage() * (1.0 / (1<<20)), active_chainstate.CoinsTip().GetCacheSize(),
2236 | +      GuessVerificationProgress(m_params.TxData(), pindexNew), this->CoinsTip().DynamicMemoryUsage() * (1.0 / (1<<20)), this->CoinsTip().GetCacheSize(),

jnewbery commented at 12:09 PM on July 13, 2021:

As above, no need for this-> here.

in src/validation.h:605 in c13e832af4 outdated

 600 | @@ -600,7 +601,10 @@ class CChainState
 601 |      //! CChainState instances.
 602 |      BlockManager& m_blockman;
 603 |  
 604 | -    explicit CChainState(CTxMemPool& mempool, BlockManager& blockman, std::optional<uint256> from_snapshot_blockhash = std::nullopt);
 605 | +    explicit CChainState(
 606 | +        CTxMemPool* const mempool,

jnewbery commented at 12:35 PM on July 13, 2021:

These new const keywords on the CChainstate::CChainstate and InitializeChainstate arguments are harmless but unnecessary. When you're passing by value (which is what happens when you pass a pointer), making that argument const doesn't change the interface, since the argument is copied to a locally scoped parameter inside the function. It's only when passing by reference that marking arguments const matters.

You can tell that it doesn't make a difference because the function declaration here has the first arg as CTxMemPool* const, and the function definition in the .cpp file has CTxMemPool* (non const pointer). The compiler doesn't complain because the const-ness of a pass by value arg isn't part of the signature.

jamesob commented at 1:30 PM on July 13, 2021:

Right but at least this makes it clear that we aren't going to do any pointer arithmetic on mempool in the body of the function, right?

jnewbery commented at 2:00 PM on July 13, 2021:

this makes it clear that we aren't going to do any pointer arithmetic on mempool

mempool inside the function is a copy of the pointer, and so whether or not it's mutated is not part of the contract with the caller, and so doesn't need to be in the function declaration. Like I said it's harmless, but you'll see that there are almost no examples in the code of pass by value parameters being marked const.

in src/validation.cpp:375 in c13e832af4 outdated

 373 | +    m_mempool->removeForReorg(*this, STANDARD_LOCKTIME_VERIFY_FLAGS);
 374 |      // Re-limit mempool size, in case we added any transactions
 375 | -    LimitMempoolSize(mempool, active_chainstate.CoinsTip(), gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, std::chrono::hours{gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)});
 376 | +    LimitMempoolSize(
 377 | +        *m_mempool,
 378 | +        this->CoinsTip(),

jnewbery commented at 12:40 PM on July 13, 2021:

No need for this->

in src/test/validation_flush_tests.cpp:23 in c13e832af4 outdated

  19 | @@ -20,7 +20,7 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate)
  20 |  {
  21 |      CTxMemPool mempool;
  22 |      BlockManager blockman{};
  23 | -    CChainState chainstate{mempool, blockman};
  24 | +    CChainState chainstate{&mempool, blockman};

jnewbery commented at 12:43 PM on July 13, 2021:

Any idea why there's a different tx_pool from mempool in this test? It's not used anywhere so presumably can be removed?

jamesob commented at 1:55 PM on July 13, 2021:

Yep, looks like it. I think when I wrote these I wanted to cover the fact that you could pass in a mempool reference that wasn't owned by the chainstate, but I don't think there's any point in doing that now.

jamesob commented at 1:29 PM on July 13, 2021: member

Anyone have any idea what the deal is with libsecp tests failing on ARM here?

This doesn't look like a spurious failure.

ryanofsky commented at 2:22 PM on July 13, 2021: member

@MarcoFalke would probably be most knowledgeable about the qemu-arm coredump #22415 (comment) https://cirrus-ci.com/task/6754077301276672?logs=ci#L3494, I was curious about that too. It seems like it is happening in a libsecp256k1 test so not related? But maybe there are ways to debug.

(I don't want to bikeshed the style issues, since they don't actually matter at all. But just for fun I'll say I do agree with John in not liking mostly spurious const. Also would drop Try prefix or replace it with Maybe since Try suggests some kind of failure would be expected. I do think having this-> prefixes is more readable in current code to distinguish method calls from function calls, but in non-legacy code it'd be cleaner to use lowerCaseMethod() and UpperCaseFunction() names to avoid the ambiguity.)

MarcoFalke commented at 2:40 PM on July 13, 2021: member

The (~x[m][shift]) << (64 - (1 << usebits))) == 0 failure is expected: https://github.com/bitcoin-core/secp256k1/issues/610#issuecomment-482803876

MarcoFalke commented at 2:41 PM on July 13, 2021: member

Could edit OP to remove the section that no longer applies to make sure it doesn't end up in the final merge commit?

validation: make CChainState::m_mempool optional

Since we now have multiple chainstate objects, only one of them is active at any given
time. An active chainstate has a mempool, but there's no point to others having one.

This change will simplify proposed assumeutxo semantics. See the discussion here:
https://github.com/bitcoin/bitcoin/pull/15606#pullrequestreview-692965905

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

617661703a

refactor: move UpdateMempoolForReorg into CChainState

Allows fewer arguments and simplification of call sites.

Co-authored-by: John Newbery <john@johnnewbery.com>

46e3efd1e4

jamesob force-pushed on Jul 13, 2021

jamesob commented at 3:14 PM on July 13, 2021: member

Pushed a rebase:

TryUpdateMempool... -> MaybeUpdateMempool...
Removed unnecessary consts
Removed outdated content from OP

refactor: no mempool arg to GetCoinsCacheSizeState

Unnecessary argument since we can make use of this->m_mempool

Co-authored-by: John Newbery <john@johnnewbery.com>

4abf0779d6

refactor: move UpdateTip into CChainState

Makes sense and saves on arguments.

Co-authored-by: John Newbery <john@johnnewbery.com>

ceb7b35a39

jamesob force-pushed on Jul 13, 2021

jamesob commented at 3:28 PM on July 13, 2021: member

unnecessary tx_pool removed

jnewbery commented at 3:32 PM on July 13, 2021: member

ACK ceb7b35a39145717e2d9d356fd382bd1f95d2a5a

ryanofsky approved

ryanofsky commented at 3:35 PM on July 13, 2021: member

Code review ACK ceb7b35a39145717e2d9d356fd382bd1f95d2a5a (just minor style and test tweaks since last review)

lsilva01 approved

lsilva01 commented at 9:57 PM on July 14, 2021: contributor

Code review ACK and tested on Signet ACK https://github.com/bitcoin/bitcoin/pull/22415/commits/ceb7b35a39145717e2d9d356fd382bd1f95d2a5a

naumenkogs commented at 8:51 AM on July 15, 2021: member

ACK ceb7b35a39145717e2d9d356fd382bd1f95d2a5a

MarcoFalke approved

MarcoFalke commented at 11:33 AM on July 15, 2021: member

review ACK ceb7b35a39145717e2d9d356fd382bd1f95d2a5a 😌

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 74406e84f80bcb1056122ddab2c038117cd73fd1e1a79cd5df1958eacc8b9b20 -

</details>

MarcoFalke merged this on Jul 15, 2021

MarcoFalke closed this on Jul 15, 2021

sidhujag referenced this in commit 0f92a6056f on Jul 23, 2021

gwillen referenced this in commit 40d1f3ffc5 on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022