This PR refactors AcceptToMemoryPool.
. Remove AcceptToMemoryPoolWithTime (after #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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
210- * exposed only for testing. Client code should use ChainstateManager::ProcessTransaction()
211+ * Try to add a transaction to the mempool with a specified acceptance time. This is an internal
212+ * function and is exposed only for testing. Client code should use ChainstateManager::ProcessTransaction()
213 *
214- * @param[in] active_chainstate Reference to the active chainstate.
215+ * @param[in] chainparams Tweakable parameters of this instance.
Concept ACK
I also have a branch that does this (amongst other things) here: https://github.com/jnewbery/bitcoin/tree/2021-09-process-transaction-2. Feel free to take anything you find useful from that branch.
224 */
225-MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx,
226- bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
227+MempoolAcceptResult AcceptToMemoryPool(const CChainParams& chainparams, CTxMemPool& pool,
228+ CChainState& active_chainstate, const CTransactionRef &tx,
229+ int64_t nAcceptTime, bool bypass_limits, bool test_accept)
accept_time. Doxygen comment should be more descriptice, e.g. that this is the time that the mempool entry will record and will be used to determine when the entry expires.
nAcceptTime to accept_time and improved the field description in 034e827
205@@ -206,12 +206,26 @@ struct PackageMempoolAcceptResult
206 };
207
208 /**
209- * (Try to) add a transaction to the memory pool.
210- * @param[in] bypass_limits When true, don't enforce mempool fee limits.
211- * @param[in] test_accept When true, run validation checks but don't submit to mempool.
212+ * Try to add a transaction to the mempool with a specified acceptance time. This is an internal
227 */
228-MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx,
229- bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
230+MempoolAcceptResult AcceptToMemoryPool(const CChainParams& chainparams, CTxMemPool& pool,
231+ CChainState& active_chainstate, const CTransactionRef &tx,
232+ int64_t accept_time, bool bypass_limits, bool test_accept)
Please align arguments on new lines after the opening parens:
0MempoolAcceptResult AcceptToMemoryPool(const CChainParams& chainparams, CTxMemPool& pool,
1 CChainState& active_chainstate, const CTransactionRef &tx,
2 int64_t accept_time, bool bypass_limits, bool test_accept)
(We use clang-style AlignAfterOpenBracket: true)
229@@ -230,7 +230,7 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
230 Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID ||
231 it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID);
232
233- const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx_pool, tx, bypass_limits));
234+ const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(Params(), tx_pool, chainstate, tx, GetTime(), bypass_limits, false /* test_accept */));
Please use clang-tidy style bugprone-argument-comment:
0 const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(Params(), tx_pool, chainstate, tx, GetTime(), bypass_limits, /*test_accept=*/false));
See also #22981
(same comment below)
1011@@ -1011,15 +1012,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1012
1013 } // anon namespace
1014
1015-/** (try to) add transaction to memory pool with a specified acceptance time **/
1016-static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool,
1017- CChainState& active_chainstate,
1018- const CTransactionRef &tx, int64_t nAcceptTime,
1019- bool bypass_limits, bool test_accept)
1020+MempoolAcceptResult AcceptToMemoryPool(const CChainParams& chainparams, CTxMemPool& pool,
348@@ -349,8 +349,9 @@ void CChainState::MaybeUpdateMempoolForReorg(
349 while (it != disconnectpool.queuedTx.get<insertion_order>().rend()) {
350 // ignore validation errors in resurrected transactions
351 if (!fAddToMempool || (*it)->IsCoinBase() ||
352- AcceptToMemoryPool(
353- *this, *m_mempool, *it, true /* bypass_limits */).m_result_type !=
354+ AcceptToMemoryPool(Params(),
Consider using the global namespace operator ::Params() to explicitly show that you’re calling a global function. I think the long-term goal is to remove these global calls to make it easier to unit test.
Same comment for the ATMP calls below.
A few more comments inline
As these changes are already implemented in another branch, with more other commits, I can close this PR if it’s better.
No, please continue with this PR! Just feel free to take anything from the other branch that you find useful.
Possible follow-up idea that came to my mind while reviewing this PR: couldn’t we simplify the interface of AcceptToMemoryPool by getting rid of both the CChainParams and CTxMemPool parameters, since those can be deduced from the chainstate anyway? The change would look something like
0-MempoolAcceptResult AcceptToMemoryPool(const CChainParams& chainparams, CTxMemPool& pool,
1- CChainState& active_chainstate, const CTransactionRef& tx,
2+MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, const CTransactionRef& tx,
3 int64_t accept_time, bool bypass_limits, bool test_accept)
4 EXCLUSIVE_LOCKS_REQUIRED(cs_main)
5 {
6+ const CChainParams& chainparams{active_chainstate.m_params};
7+ assert(active_chainstate.m_mempool != nullptr);
8+ CTxMemPool& pool{*active_chainstate.m_mempool};
9 std::vector<COutPoint> coins_to_uncache;
10 MemPoolAccept::ATMPArgs args { chainparams, accept_time, bypass_limits, coins_to_uncache,
11 test_accept, /* m_allow_bip125_replacement */ true };
I couldn’t think of a case where passing chainparams or a mempool instance that doesn’t belong to the active chainstate makes much sense. Drawback though: currently the member m_mempool is protected and thus not accessible from outside, that one would need to move to the public section, like it was done for m_params in the last commit of this PR.
229@@ -230,7 +230,8 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
230 Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID ||
231 it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID);
232
233- const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx_pool, tx, bypass_limits));
234+ CChainState& chainstate{node.chainman->ActiveChainstate()};
235+ const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate.m_params, tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */false));
Its seems odd to change this to:
0 const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(::Params(), tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */false));
in the first commit, and then update it again to use chainstate.m_params in the second commit. You can just go straight to using chainstate.m_params in the first commit.
330@@ -330,7 +331,7 @@ FUZZ_TARGET_INIT(tx_pool, initialize_tx_pool)
331 const auto tx = MakeTransactionRef(mut_tx);
332 const bool bypass_limits = fuzzed_data_provider.ConsumeBool();
333 ::fRequireStandard = fuzzed_data_provider.ConsumeBool();
334- const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(node.chainman->ActiveChainstate(), tx_pool, tx, bypass_limits));
335+ const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate.m_params, tx_pool, node.chainman->ActiveChainstate(), tx, GetTime(), bypass_limits, /* test_accept= */false));
0 const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate.m_params, tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */false));
212- * exposed only for testing. Client code should use ChainstateManager::ProcessTransaction()
213+ * Try to add a transaction to the mempool. This is an internal function and is exposed only for testing.
214+ * Client code should use ChainstateManager::ProcessTransaction()
215 *
216- * @param[in] active_chainstate Reference to the active chainstate.
217+ * @param[in] chainparams Chain paramenters.
AcceptToMemoryPool() could just take the m_params from the active_chainstate that’s passed in.
CChainParams& chainparams from ATMP (commit 863703c), I also added a new commit to remove CTxMemPool& pool as suggested in #23437 (comment).
218 * @param[in] pool Reference to the node's mempool.
219+ * @param[in] active_chainstate Reference to the active chainstate.
220 * @param[in] tx The transaction to submit for mempool acceptance.
221+ * @param[in] accept_time The timestamp for adding the transaction to the mempool. Usually
222+ * the current system time, but may be different (eg when loading
223+ * mempool txs from disk on startup).
It’s generally not a good idea to document how the function is called in the function comment (since that may change over time, and this comment would be out of date:
0 * the current system time, but may be different.
212- * exposed only for testing. Client code should use ChainstateManager::ProcessTransaction()
213+ * Try to add a transaction to the mempool. This is an internal function and is exposed only for testing.
214+ * Client code should use ChainstateManager::ProcessTransaction()
215 *
216 * @param[in] active_chainstate Reference to the active chainstate.
217- * @param[in] pool Reference to the node's mempool.
This causes the tx_pool fuzz test to fail since it operates on a mempool that isn’t referenced by the CChainState object. See #23173 (review).
That should probably be fixed in the fuzz test, but it can be left for a future PR. For now, I suggest you just remove the final commit.
For reference, this can trivially be fixed by something like:
0struct MockedChainstate:public CChainstate{
1SwapTxPool(CTxMempool*tx_pool){m_mempool=tx_pool;}
2}
3
4...
5
6 CTxMempool fuzzed_pool;
7 (MockedChainstate*)(&chainstate)->SwapTxPool(&fuzzed_pool);
I also addressed the tx_pool.cpp issue using a different approach in #23465.
The fuzz test performed successfully on local machine and CI.
0- const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx_pool, tx, bypass_limits));
1+ CChainState& chainstate{node.chainman->ActiveChainstate()};
2+ chainstate.m_mempool = &tx_pool;
3
4+ const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx, GetTime(), bypass_limits, /* test_accept= */false));
1090+ int64_t accept_time, bool bypass_limits, bool test_accept)
1091+ EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1092 {
1093 std::vector<COutPoint> coins_to_uncache;
1094- auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, nAcceptTime, bypass_limits, coins_to_uncache, test_accept);
1095+ auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, accept_time, bypass_limits, coins_to_uncache, test_accept);
const?
args variable?
No - contrary to what the name args suggests, it cannot be const because it contains coins_to_uncache, which is populated with coins and returned. If coins_to_uncache were removed from ATMPArgs then it could be const.
As a side thought, I personally think it would be nice/more RAII-like if uncaching would be handled by MemPoolAccept (see #21146), which is one approach to removing it from args.
MemPoolAccept::AcceptSingleTransaction. Maybe it can be a follow-up PR.
228 */
229-MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx,
230- bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
231+MempoolAcceptResult AcceptToMemoryPool(const CChainParams& chainparams, CTxMemPool& pool,
232+ CChainState& active_chainstate, const CTransactionRef& tx,
233+ int64_t accept_time, bool bypass_limits, bool test_accept)
false value for test_accept?
AcceptToMemoryPoolWithTime where bool test_accept has no default value, I think making it explicit is better.
229@@ -230,7 +230,7 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
230 Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID ||
231 it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID);
232
233- const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx_pool, tx, bypass_limits));
234+ const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(::Params(), tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */false));
*/ and the value, ie: /* test_accept= */false => /* test_accept= */ false
ACK 863703c.
m_params is made a public member of CChainParams.AcceptToMemoryPool() along with accept_time and call sites updated.AcceptToMemoryPoolWithTime() is removed.::Params() is replaced with CChainStateobject’s m_params.m_params is removed as a parameter from AcceptToMemoryPool() since it can be derived from the CChainState object parameter.
Code-review ACK 863703c63311bf816b2dcdb7edd70be0bf069014 🍵
Tiny nit: there is an intermediate typo in the doxygen description for the chainparams parameter in AcceptToMemoryPool (=> s/paramenters/parameters). The first and third commits could also simply be squashed, in order to avoid introducing a temporary parameter (and its doxygen description) and hence also lower the review burden a bit. Not sure if its worth invalidating ACKs now, but I’m happy to re-review in case.
229@@ -230,7 +230,8 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
230 Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID ||
231 it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID);
232
233- const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx_pool, tx, bypass_limits));
234+ CChainState& chainstate{node.chainman->ActiveChainstate()};
utACK 4ec8b96e077a359c7ba877fc4f78d55ac7d5fb22
Happy to rereview this once it’s rebased.
216- * @param[in] active_chainstate Reference to the active chainstate.
217 * @param[in] pool Reference to the node's mempool.
218+ * @param[in] active_chainstate Reference to the active chainstate.
219 * @param[in] tx The transaction to submit for mempool acceptance.
220+ * @param[in] accept_time The timestamp for adding the transaction to the mempool. Usually
221+ * the current system time, but may be different.