Remove CTxMemPool parameter from AcceptToMemoryPool function, as suggested in #23437 (comment) .
This requires that CChainState has access to MockedTxPool in tx_pool.cpp as mentioned #23173 (review). So the MockedTxPool is attributed to CChainState::m_mempool before calling AcceptToMemoryPool.
Requires #23437.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
1153@@ -1153,9 +1154,9 @@ CChainState::CChainState(
1154 BlockManager& blockman,
1155 ChainstateManager& chainman,
1156 std::optional<uint256> from_snapshot_blockhash)
1157- : m_mempool(mempool),
1158- m_blockman(blockman),
1159+ : m_blockman(blockman),
[-Werror = reorder].
See https://stackoverflow.com/questions/12222417/why-should-i-initialize-member-variables-in-the-order-theyre-declared-in
CTxMemPool pool argument from AcceptToMemoryPool() since it can be obtained from CChainState active_chainstate.m_mempool
590@@ -596,6 +591,10 @@ class CChainState
591 /** Chain parameters for this chainstate */
592 const CChainParams& m_params;
593
594+ //! Optional mempool that is kept in sync with the chain.
595+ //! Only the active chainstate has a mempool.
596+ CTxMemPool* m_mempool;
988cfd8914f12ce87fb6ac6499326e140e840c1d
Instead of making this public, I think you should add a getter like CoinsTip() and an initialized like InitCoinsDB. Then you can revert what @stratospher asks in https://github.com/bitcoin/bitcoin/pull/23465/commits/988cfd8914f12ce87fb6ac6499326e140e840c1d#r756677595.
Code review ACK 988cfd8914f12ce87fb6ac6499326e140e840c1d.
This change simplifies ATMP since chain params and the mempool are accessible from the chainstate.
It also removes the intermediate AcceptToMemoryPoolWithTime causing all ATMP callers to explicit set accept_time, usually GetTime().
I changed the approach. Now, the m_mempool remains private and the getter and setter were added, as suggested by @stratospher in #23448#pullrequestreview-816441509 and @promag in #23465 (review) .
Not sure if this is idiomatic (c++ style). Otherwise, I can revert to original approach (with public m_mempool).
210@@ -211,7 +211,6 @@ struct PackageMempoolAcceptResult
211 * Try to add a transaction to the mempool. This is an internal function and is exposed only for testing.
212 * Client code should use ChainstateManager::ProcessTransaction()
213 *
214- * @param[in] pool Reference to the node's mempool.
215 * @param[in] active_chainstate Reference to the active chainstate.
216 * @param[in] tx The transaction to submit for mempool acceptance.
217 * @param[in] accept_time The timestamp for adding the transaction to the mempool. Usually
662+ }
663+
664+ /**
665+ * Update the mempool
666+ */
667+ CTxMemPool* SetMempool(CTxMemPool* mempool)
238@@ -230,7 +239,10 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
239 Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID ||
240 it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID);
241
242- const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */ false));
243+ DummyChainState& dummyChainState{ static_cast<DummyChainState&>(node.chainman->ActiveChainstate())};
chainstate symbol, so please use that. Also below it would be good to pass the same symbol to atmp, not the dummy one time and the “real” another time.
653@@ -656,6 +654,12 @@ class CChainState
654 return m_coins_views->m_dbview;
655 }
656
657+ //! @returns A reference to the mempool.
m_mempool may be null).
328@@ -329,11 +329,9 @@ void CChainState::MaybeUpdateMempoolForReorg(
329 // been previously seen in a block.
330 auto it = disconnectpool.queuedTx.get<insertion_order>().rbegin();
331 while (it != disconnectpool.queuedTx.get<insertion_order>().rend()) {
332+ auto accepted = AcceptToMemoryPool(*this, *it, GetTime(), /* bypass_limits= */true, /* test_accept= */false);
fAddToMempol is false or the transaction is a coinbase.
122@@ -114,7 +123,7 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
123 {
124 FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
125 const auto& node = g_setup->m_node;
126- auto& chainstate = node.chainman->ActiveChainstate();
127+ auto& chainstate{ static_cast<DummyChainState&>(node.chainman->ActiveChainstate())};
0 auto& chainstate{static_cast<DummyChainState&>(node.chainman->ActiveChainstate())};
28@@ -29,6 +29,15 @@ struct MockedTxPool : public CTxMemPool {
29 }
30 };
31
32+class DummyChainState final : public CChainState
33+{
34+public:
35+ CTxMemPool* SetMempool(CTxMemPool* mempool)
CTxMemPool, which never gets used. Can it just return void?
238@@ -230,7 +239,9 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
239 Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID ||
240 it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID);
241
242- const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */ false));
243+ chainstate.SetMempool(&tx_pool);
330@@ -331,9 +331,7 @@ void CChainState::MaybeUpdateMempoolForReorg(
331 while (it != disconnectpool.queuedTx.get<insertion_order>().rend()) {
332 // ignore validation errors in resurrected transactions
333 if (!fAddToMempool || (*it)->IsCoinBase() ||
334- AcceptToMemoryPool(*m_mempool, *this, *it, GetTime(),
335- /* bypass_limits= */true, /* test_accept= */ false).m_result_type !=
336- MempoolAcceptResult::ResultType::VALID) {
337+ AcceptToMemoryPool(*this, *it, GetTime(), /* bypass_limits= */true, /* test_accept= */false).m_result_type != MempoolAcceptResult::ResultType::VALID) {
m_mempool argument. 160 columns is too wide for many people to do a side-by-side diff.
4574@@ -4572,8 +4575,8 @@ bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mocka
4575 }
4576 if (nTime > nNow - nExpiryTimeout) {
4577 LOCK(cs_main);
4578- if (AcceptToMemoryPool(pool, active_chainstate, tx, nTime, /* bypass_limits= */ false,
4579- /* test_accept= */ false).m_result_type == MempoolAcceptResult::ResultType::VALID) {
4580+ auto accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, /* bypass_limits= */ false, /* test_accept= */ false);
0 const auto& accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, /*bypass_limits=*/false, /*test_accept=*/false);
MempoolAcceptResult class objectFeel free to ignore.
Co-authored-by: John Newbery <1063656+jnewbery@users.noreply.github.com>
Co-authored-by: Jon Atack <jon@atack.com>
1096 int64_t accept_time, bool bypass_limits, bool test_accept)
1097 EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1098 {
1099 const CChainParams& chainparams{active_chainstate.m_params};
1100+ assert(active_chainstate.GetMempool() != nullptr);
1101+ CTxMemPool& pool{*active_chainstate.GetMempool()};
0 CTxMemPool& pool{*Assert(active_chainstate.GetMempool())};
nit: Can be written shorter
28@@ -29,6 +29,15 @@ struct MockedTxPool : public CTxMemPool {
29 }
30 };
31
32+class DummyChainState final : public CChainState
33+{
34+public:
0struct DummyChainState final : public CChainState {
nit: Can be written shorter, but might be controversial to use struct here. I think in tests it is fine.
28@@ -29,6 +29,15 @@ struct MockedTxPool : public CTxMemPool {
29 }
30 };
31
32+class DummyChainState final : public CChainState
33+{
34+public:
35+ void SetMempool(CTxMemPool* mempool)
36+ {
37+ m_mempool = mempool;
nit: mempool is never null, so can use a reference
0 void SetMempool(CTxMemPool& mempool)
1 {
2 m_mempool = &mempool;
left some nits in the test, but no blockers
review ACK f1f10c0514fe81318c8b064f9dad0e2f9a2cd037 🔙
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK f1f10c0514fe81318c8b064f9dad0e2f9a2cd037 🔙
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUgHpQv/Zm11wrNJ2iGL1U2uf4cOLbafgx6DxBsY5Jf4VOBRhAaTSdluLEBEkhdY
8eTLy4lrXuzAhrs9SS8ogJYEUT2JWn5vmfPjXdsPIyJPRVT1IEUoeelarH03oOHEe
9+xW5ILkL7S8GlNGB+nUqXkq47Qw5jY+zr39v3tGvHJkrVHVhaDZOlFmPoXKDVrdv
10TnsQcV8GL+4ZL9gyg6+tM26uSeDOaTId8L/4xtmzkzgcPcZqPsT/LyXxKmKDjY5s
11KHyASsIIZG+Tb2M7HGAaZiPZelhhg1kyJqv0tq+woEfeE3qQ41U+r/if+kM10kmh
12cKy4Jfw8lR7em5e2fVE2NDf0va6i1TVJz/8ZPYSS0C5IK8aeC+0OK2CE0GKjlP8k
13n/F7rAU9BUOkerA4ccwq1T+l0FwN4yOa2d3EZobHHRDn8jSmITqwaXxZHIDyB7/3
14lIHrKEoNBvdlF1ohlQNqXGSJQeb+wQ1l9EWLM0Gj/U/8WjZTyswSxAn1UBJOEabH
15CzoLf3ep
16=M0GN
17-----END PGP SIGNATURE-----