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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
Code Review ACK 988cfd8914f12ce87fb6ac6499326e140e840c1d
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),
Is there any particular reason for reordering the initialiser list?
Member variables must be initialized in the order in which they are declared. Otherwise, CI will throw [-Werror = reorder].
See https://stackoverflow.com/questions/12222417/why-should-i-initialize-member-variables-in-the-order-theyre-declared-in
Thanks!
Concept ACK. This PR removes 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
maybe remove the sentence that starts with "usually"? #23437 (review)
Done in 3f7d1c1
662 | + } 663 | + 664 | + /** 665 | + * Update the mempool 666 | + */ 667 | + CTxMemPool* SetMempool(CTxMemPool* mempool)
Not a fan of adding test-only helpers to the real code. Might be better to move this to the test code.
Good catch. Done in 3f7d1c1
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())};
there is already a 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.
Done in 83b3697 . The same symbol is being passed to ATMP.
653 | @@ -656,6 +654,12 @@ class CChainState 654 | return m_coins_views->m_dbview; 655 | } 656 | 657 | + //! @returns A reference to the mempool.
This is wrong. It's returning a pointer, not a reference (and can't return a reference, since 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);
This is a behaviour chage. With this change, ATMP() is always called, even if 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())};
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)
Why does this return a pointer to the 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);
Does this still work if this line is moved up to l145 (i.e. immediately after the mempool is constructed). I think the test is easier to understand if the mempool pointer is set at the beginning and not updated.
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) {
I don't understand why you keep changing the formatting on this line, where all you need to do is remove the m_mempool argument. 160 columns is too wide for many people to do a side-by-side diff.
Done in 2cc5a38
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);
const auto& accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, /*bypass_limits=*/false, /*test_accept=*/false);
MempoolAcceptResult class objectFeel free to ignore.
Approach ACK 2cc5a38f3890636e7707362ae0f1bcada82b505f, will review properly.
Co-authored-by: John Newbery <1063656+jnewbery@users.noreply.github.com>
Co-authored-by: Jon Atack <jon@atack.com>
utACK f1f10c0514fe81318c8b064f9dad0e2f9a2cd037
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()};
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:
struct DummyChainState final : public CChainState {
nit: Can be written shorter, but might be controversial to use struct here. I think in tests it is fine.
I'd prefer to declare this as a class, even if it requires an additional line for the access specifier (since it's a class and not just a data structure).
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
void SetMempool(CTxMemPool& mempool)
{
m_mempool = &mempool;
left some nits in the test, but no blockers
review ACK f1f10c0514fe81318c8b064f9dad0e2f9a2cd037 🔙
<details><summary>Show signature</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK f1f10c0514fe81318c8b064f9dad0e2f9a2cd037 🔙
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgHpQv/Zm11wrNJ2iGL1U2uf4cOLbafgx6DxBsY5Jf4VOBRhAaTSdluLEBEkhdY
eTLy4lrXuzAhrs9SS8ogJYEUT2JWn5vmfPjXdsPIyJPRVT1IEUoeelarH03oOHEe
+xW5ILkL7S8GlNGB+nUqXkq47Qw5jY+zr39v3tGvHJkrVHVhaDZOlFmPoXKDVrdv
TnsQcV8GL+4ZL9gyg6+tM26uSeDOaTId8L/4xtmzkzgcPcZqPsT/LyXxKmKDjY5s
KHyASsIIZG+Tb2M7HGAaZiPZelhhg1kyJqv0tq+woEfeE3qQ41U+r/if+kM10kmh
cKy4Jfw8lR7em5e2fVE2NDf0va6i1TVJz/8ZPYSS0C5IK8aeC+0OK2CE0GKjlP8k
n/F7rAU9BUOkerA4ccwq1T+l0FwN4yOa2d3EZobHHRDn8jSmITqwaXxZHIDyB7/3
lIHrKEoNBvdlF1ohlQNqXGSJQeb+wQ1l9EWLM0Gj/U/8WjZTyswSxAn1UBJOEabH
CzoLf3ep
=M0GN
-----END PGP SIGNATURE-----
</details>
Post-merge ACK, modulo that I'm not sure if the CChainState::GetMempool() getter adds value. I've proposed a follow-up to remove it that picks up some of Marco's review feedback.