Merge bitcoin#23437: refactor: AcceptToMemoryPool

123f5de Remove calls to global Params() in tx_pool test (lsilva01)
9360778 Remove AcceptToMemoryPoolWithTime (lsilva01)

Pull request description:

  This PR refactors AcceptToMemoryPool.

  . Remove `AcceptToMemoryPoolWithTime` (after bitcoin#23173, this function is no longer needed).
  . Remove the `CChainParams chainparams` parameter from ATMP as they can be inferred from the current chain state.
  . Update the `tx_pool` test with new function signature.

ACKs for top commit:
  jnewbery:
    reACK 123f5de
  glozow:
    light code review ACK 123f5de
  theStack:
    re-ACK 123f5de

Tree-SHA512: b672d9f091f5fcb4d163c0a443aa469b2ce6f37201136803201c620c3fa329ffb41abd50c5b528787fd1c47e8e09af5ec20ddaa0a4235352c9c619e5691dddb6

Author: MarcoFalke

src/test/fuzz/tx_pool.cpp

@@ -86,7 +86,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, CCh
         BlockAssembler::Options options;
         options.nBlockMaxWeight = fuzzed_data_provider.ConsumeIntegralInRange(0U, MAX_BLOCK_WEIGHT);
         options.blockMinFeeRate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)};
-        auto assembler = BlockAssembler{chainstate, *static_cast<CTxMemPool*>(&tx_pool), ::Params(), options};
+        auto assembler = BlockAssembler{chainstate, *static_cast<CTxMemPool*>(&tx_pool), chainstate.m_params, options};
         auto block_template = assembler.CreateNewBlock(CScript{} << OP_TRUE);
         Assert(block_template->block.vtx.size() >= 1);
     }
@@ -224,13 +224,13 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
         // Make sure ProcessNewPackage on one transaction works and always fully validates the transaction.
         // The result is not guaranteed to be the same as what is returned by ATMP.
         const auto result_package = WITH_LOCK(::cs_main,
-                                    return ProcessNewPackage(node.chainman->ActiveChainstate(), tx_pool, {tx}, true));
+                                    return ProcessNewPackage(chainstate, tx_pool, {tx}, true));
         auto it = result_package.m_tx_results.find(tx->GetWitnessHash());
         Assert(it != result_package.m_tx_results.end());
         Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID ||
                it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID);
 
-        const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx_pool, tx, bypass_limits));
+        const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */ false));
         const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID;
         SyncWithValidationInterfaceQueue();
         UnregisterSharedValidationInterface(txr);
@@ -330,7 +330,7 @@ FUZZ_TARGET_INIT(tx_pool, initialize_tx_pool)
         const auto tx = MakeTransactionRef(mut_tx);
         const bool bypass_limits = fuzzed_data_provider.ConsumeBool();
         ::fRequireStandard = fuzzed_data_provider.ConsumeBool();
-        const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(node.chainman->ActiveChainstate(), tx_pool, tx, bypass_limits));
+        const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */ false));
         const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID;
         if (accepted) {
             txids.push_back(tx->GetHash());

src/validation.cpp

@@ -349,8 +349,8 @@ void CChainState::MaybeUpdateMempoolForReorg(
     while (it != disconnectpool.queuedTx.get<insertion_order>().rend()) {
         // ignore validation errors in resurrected transactions
         if (!fAddToMempool || (*it)->IsCoinBase() ||
-            AcceptToMemoryPool(
-                *this, *m_mempool, *it, true /* bypass_limits */).m_result_type !=
+            AcceptToMemoryPool(*m_mempool, *this, *it, GetTime(),
+                /* bypass_limits= */true, /* test_accept= */ false).m_result_type !=
                     MempoolAcceptResult::ResultType::VALID) {
             // If the transaction doesn't make it in to the mempool, remove any
             // transactions that depend on it (which would now be orphans).
@@ -1078,15 +1078,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
 
 } // anon namespace
 
-/** (try to) add transaction to memory pool with a specified acceptance time **/
-static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool,
-                                                      CChainState& active_chainstate,
-                                                      const CTransactionRef &tx, int64_t nAcceptTime,
-                                                      bool bypass_limits, bool test_accept)
-                                                      EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+MempoolAcceptResult AcceptToMemoryPool(CTxMemPool& pool, CChainState& active_chainstate, const CTransactionRef& tx,
+                                       int64_t accept_time, bool bypass_limits, bool test_accept)
+    EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
+    const CChainParams& chainparams{active_chainstate.m_params};
     std::vector<COutPoint> coins_to_uncache;
-    auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, nAcceptTime, bypass_limits, coins_to_uncache, test_accept);
+    auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, accept_time, bypass_limits, coins_to_uncache, test_accept);
     const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args);
     if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
         // Remove coins that were not present in the coins cache before calling
@@ -1103,12 +1101,6 @@ static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainp
     return result;
 }
 
-MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx,
-                                       bool bypass_limits, bool test_accept)
-{
-    return AcceptToMemoryPoolWithTime(Params(), pool, active_chainstate, tx, GetTime(), bypass_limits, test_accept);
-}
-
 PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTxMemPool& pool,
                                                    const Package& package, bool test_accept)
 {
@@ -1161,8 +1153,8 @@ CChainState::CChainState(
     ChainstateManager& chainman,
     std::optional<uint256> from_snapshot_blockhash)
     : m_mempool(mempool),
-      m_params(::Params()),
       m_blockman(blockman),
+      m_params(::Params()),
       m_chainman(chainman),
       m_from_snapshot_blockhash(from_snapshot_blockhash) {}
 
@@ -3500,7 +3492,7 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef&
         state.Invalid(TxValidationResult::TX_NO_MEMPOOL, "no-mempool");
         return MempoolAcceptResult::Failure(state);
     }
-    auto result = AcceptToMemoryPool(active_chainstate, *active_chainstate.m_mempool, tx, /*bypass_limits=*/ false, test_accept);
+    auto result = AcceptToMemoryPool(*active_chainstate.m_mempool, active_chainstate, tx, GetTime(), /* bypass_limits= */ false, test_accept);
     active_chainstate.m_mempool->check(active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1);
     return result;
 }
@@ -4530,7 +4522,6 @@ static const uint64_t MEMPOOL_DUMP_VERSION = 1;
 
 bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mockable_fopen_function)
 {
-    const CChainParams& chainparams = Params();
     int64_t nExpiryTimeout = gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60;
     FILE* filestr{mockable_fopen_function(gArgs.GetDataDirNet() / "mempool.dat", "rb")};
     CAutoFile file(filestr, SER_DISK, CLIENT_VERSION);
@@ -4568,8 +4559,8 @@ bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mocka
             }
             if (nTime > nNow - nExpiryTimeout) {
                 LOCK(cs_main);
-                if (AcceptToMemoryPoolWithTime(chainparams, pool, active_chainstate, tx, nTime, false /* bypass_limits */,
-                                               false /* test_accept */).m_result_type == MempoolAcceptResult::ResultType::VALID) {
+                if (AcceptToMemoryPool(pool, active_chainstate, tx, nTime, /* bypass_limits= */ false,
+                                               /* test_accept= */ false).m_result_type == MempoolAcceptResult::ResultType::VALID) {
                     ++count;
                 } else {
                     // mempool may contain the transaction already, e.g. from

src/validation.h

@@ -208,19 +208,23 @@ struct PackageMempoolAcceptResult
 };
 
 /**
- * Try to add a transaction to the mempool. This is an internal function and is
- * exposed only for testing. Client code should use ChainstateManager::ProcessTransaction()
+ * Try to add a transaction to the mempool. This is an internal function and is exposed only for testing.
+ * Client code should use ChainstateManager::ProcessTransaction()
  *
- * @param[in]  active_chainstate  Reference to the active chainstate.
  * @param[in]  pool               Reference to the node's mempool.
+ * @param[in]  active_chainstate  Reference to the active chainstate.
  * @param[in]  tx                 The transaction to submit for mempool acceptance.
+ * @param[in]  accept_time        The timestamp for adding the transaction to the mempool. Usually
+ *                                the current system time, but may be different.
+ *                                It is also used to determine when the entry expires.
  * @param[in]  bypass_limits      When true, don't enforce mempool fee and capacity limits.
  * @param[in]  test_accept        When true, run validation checks but don't submit to mempool.
  *
  * @returns a MempoolAcceptResult indicating whether the transaction was accepted/rejected with reason.
  */
-MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx,
-                                       bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+MempoolAcceptResult AcceptToMemoryPool(CTxMemPool& pool, CChainState& active_chainstate, const CTransactionRef& tx,
+                                       int64_t accept_time, bool bypass_limits, bool test_accept)
+    EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
 /**
 * Atomically test acceptance of a package. If the package only contains one tx, package rules still
@@ -581,8 +585,6 @@ class CChainState
     //! Only the active chainstate has a mempool.
     CTxMemPool* m_mempool;
 
-    const CChainParams& m_params;
-
     //! Manages the UTXO set, which is a reflection of the contents of `m_chain`.
     std::unique_ptr<CoinsViews> m_coins_views;
 
@@ -591,6 +593,9 @@ class CChainState
     //! CChainState instances.
     BlockManager& m_blockman;
 
+    /** Chain parameters for this chainstate */
+    const CChainParams& m_params;
+
     //! The chainstate manager that owns this chainstate. The reference is
     //! necessary so that this instance can check whether it is the active
     //! chainstate within deeply nested method calls.