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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
I don't think this description "Tweakable parameters of this instance." is very illuminating. Tweakable suggests to me that the user can modify these parameters, but that isn't the case here.
Changed the description in 034e827 .
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)
prefer snake case 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.
Changed nAcceptTime to accept_time and improved the field description in 034e827
Concept ACK, good cleanup.
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
"with a specified acceptance time" is redundant with the parameter documentation below.
Removed in 9a0b0fc
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:
MempoolAcceptResult AcceptToMemoryPool(const CChainParams& chainparams, CTxMemPool& pool,
CChainState& active_chainstate, const CTransactionRef &tx,
int64_t accept_time, bool bypass_limits, bool test_accept)
(We use clang-style AlignAfterOpenBracket: true)
Done in 9a0b0fc
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:
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)
Done in 9a0b0fc
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,
Again, please align new lines with the opening parens.
Done in 9a0b0fc
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.
Removed calls to global Params() in ATMP and in the tx_pool test file ( aac6548 )
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.
Concept ACK
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
-MempoolAcceptResult AcceptToMemoryPool(const CChainParams& chainparams, CTxMemPool& pool,
- CChainState& active_chainstate, const CTransactionRef& tx,
+MempoolAcceptResult AcceptToMemoryPool(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};
+ assert(active_chainstate.m_mempool != nullptr);
+ CTxMemPool& pool{*active_chainstate.m_mempool};
std::vector<COutPoint> coins_to_uncache;
MemPoolAccept::ATMPArgs args { chainparams, accept_time, bypass_limits, coins_to_uncache,
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:
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.
Agreed. Changed it to 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));
const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate.m_params, tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */false));
Done.
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.
This argument could be removed. AcceptToMemoryPool() could just take the m_params from the active_chainstate that's passed in.
Done.
As this PR is removing the 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:
* the current system time, but may be different.
Makes sense. Comment removed.
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:
struct MockedChainstate:public CChainstate{
SwapTxPool(CTxMempool*tx_pool){m_mempool=tx_pool;}
}
...
CTxMempool fuzzed_pool;
(MockedChainstate*)(&chainstate)->SwapTxPool(&fuzzed_pool);
Final commit removed.
I also addressed the tx_pool.cpp issue using a different approach in #23465.
The fuzz test performed successfully on local machine and CI.
- const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx_pool, tx, bypass_limits));
+ CChainState& chainstate{node.chainman->ActiveChainstate()};
+ chainstate.m_mempool = &tx_pool;
+ 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);
can this be const?
Refers to the 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.
It seems this change also implies changing MemPoolAccept::AcceptSingleTransaction. Maybe it can be a follow-up PR.
Just to clarify, I didn't mean to suggest that you change it - out of scope for this PR. Marking resolved.
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)
Curious why you removed default false value for test_accept?
Since this PR removes 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));
nit: sometimes throughout your changes you don't have a space between the last */ and the value, ie: /* test_accept= */false => /* test_accept= */ false
Thanks. I will fix it.
Fixed.
Concept and Approach ACK
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.
@theStack and @stratospher thanks for the review. The commit were squashed, as suggested.
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()};
I think this line could simply be removed, as it is merely shadowing another reference with the same name and content on top of the fuzz test?
Good catch. Line removed in 4ec8b96.
not addressed?
Yes, it was addressed in 4ec8b96.
In the future, it might be good to keep a clean history and not introduce unused stuff in one commit and remove it in the next.
re-ACK 4ec8b96e077a359c7ba877fc4f78d55ac7d5fb22
utACK 4ec8b96e077a359c7ba877fc4f78d55ac7d5fb22
Happy to rereview this once it's rebased.
re-ACK 123f5de8265af07ff1c95ed8078085af6cb589cb
reACK 123f5de8265af07ff1c95ed8078085af6cb589cb
light code review ACK 123f5de8265af07ff1c95ed8078085af6cb589cb
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.
nit: I don't think we need to explain what the argument means at the call site.