This cleans up the miner tests:
clear, see https://github.com/bitcoin/bitcoin/pull/19909
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
CheckSequenceLocksAtTip() by hebasto)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.
Code review ACK dddd32eedfb8fe92a83c533ebdbb3fdff510a0fb
My only criticism is on the last commit: before, it was clear that the CreateNewBlock_validity test was done with the main chain params, after the change it is probably still doing this but implicitly.
Hmm. For reference, https://github.com/bitcoin/bitcoin/pull/24595/commits/38860f93b680f152fc6fc3d9ae574a4c0659e775#diff-b12066a688b85bea440eb38f079fb8ecc1984318470631bca7e3547a98effaa9 is doing the same change. It seems confusing to imply that the chainparams may differ in subsequent calls of the same function.
Happy to add an assert that the “global” chainparams are the main params, if that helps?
git range-diff bitcoin-core/master dddd32 fabdc1902e.
88@@ -89,8 +89,10 @@ static CBlockIndex CreateBlockIndex(int nHeight, CBlockIndex* active_chain_tip)
89 // Test suite for ancestor feerate transaction selection.
90 // Implemented as an additional function, rather than a separate test case,
91 // to allow reusing the blockchain created in CreateNewBlock_validity.
92-void MinerTestingSetup::TestPackageSelection(const CChainParams& chainparams, const CScript& scriptPubKey, const std::vector<CTransactionRef>& txFirst)
93+void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const std::vector<CTransactionRef>& txFirst)
94 {
95+ CTxMemPool tx_mempool{/*estimator=*/nullptr, /*check_ratio=*/1};
chainparams. Not sure if I understand why separate mempools that aren’t m_node.mempool is beneficial. Seems like clearing would be sufficient, but the goal is to remove clear()?
Not sure if I understand why separate mempools that aren’t m_node.mempool is beneficial. Seems like clearing would be sufficient, but the goal is to remove clear()?
Yes, that is the goal. And there should be no downside using separate mempools, since the goal of the unit tests is to run independently.
I think one of the main things that stopped me from introducing more local mempools was that chainstate still has a mempool member, and sometimes operations that you think would operate on the local mempool would actually operate on chainstate’s mempool member. It seemed like a somewhat subtle thing that might cause us to miss bugs. I don’t see any calls here that would access chainstate’s mempool member but this might change in the future.
A few improvements for the future:
TestBasicMining could be split into its own test case, since they largely don’t depend on each other (that’s why we call clear), this would also make it so that each test can use its fixture’s mempool and eliminate the aforementioned possible local vs chainstate member mempool footgunReplaceMempool method to TestingSetup to fix the CChainState member like we do in DummyChainState? Not sure…
Each block of TestBasicMining could be split into its own test case, since they largely don’t depend on each other
The mempools don’t depend on each other, but I think all tests share the same chainstate with the same main-chain blocks?
Perhaps we add a ReplaceMempool method to TestingSetup to fix the CChainState member like we do in DummyChainState? Not sure…
Good idea. I’ve started something like that in the last commit: The correctness of the changes in this pull are now enforced by valgrind. Happy to move it to the test util lib if you think it is useful for other tests.
Good idea. I’ve started something like that in the last commit: The correctness of the changes in this pull are now enforced by valgrind.
Oh nice! I think that’s good for this PR, I suppose we could do something like a TestingSetup::ReplaceMempool(std::unique_ptr<CTxMemPool>) in the future, but there’s no rush to generalize too early on.
Approach ACK!
m_node.mempool everywhere even if we still want to use shared mempools for now, to be able to make tests more lightweight later. Other changes look good too.
636@@ -604,9 +637,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
637 }
638
639 LOCK(cs_main);
chainParams and chainparams still exist at the top of the CreateNewBlock_validity test case.~ nvm, I see you’ve done it in the next commit
nit: could squash fa8f828e8f98caf3b1f1e08127d16959ec7d1b43 into fa81221bea231dc12cf74482a5878be524e43c48, it has an unused variable warning
595@@ -596,8 +596,6 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const
596 BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
597 {
598 // Note that by default, these tests run with size accounting enabled.
599- const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::MAIN);
37@@ -38,9 +38,14 @@ struct MinerTestingSetup : public TestingSetup {
38 CCoinsViewMemPool view_mempool{&m_node.chainman->ActiveChainstate().CoinsTip(), tx_mempool};
39 return CheckSequenceLocksAtTip(m_node.chainman->ActiveChain().Tip(), view_mempool, tx);
40 }
41- CTxMemPool MakeMempool()
42+ CTxMemPool& MakeMempool()
MakeMempool() return a reference from the start? Right now you’re declaring a bunch of CTxMemPool&s in the first commit, then changing them to CTxMemPool in the subsequent commits, and then changing them back to CTxMemPool&s in the last commit.
657- m_node.mempool->clear();
658
659- TestPrioritisedMining(chainparams, scriptPubKey, txFirst);
660+ TestPrioritisedMining(scriptPubKey, txFirst);
661
662 fCheckpointsEnabled = true;
fCheckPointsEnabled assignments seem to have no impact on this test? Could probably be cleaned up?
65+BlockAssembler MinerTestingSetup::AssemblerForTest(CTxMemPool& tx_mempool)
66 {
67 BlockAssembler::Options options;
68
69 options.nBlockMaxWeight = MAX_BLOCK_WEIGHT;
70 options.blockMinFeeRate = blockMinFeeRate;
BlockAssembler::Options is not needed? If you use the ctor without options, it constructs a default one which also uses DEFAULT_BLOCK_MIN_TX_FEE and min(DEFAULT_BLOCK_MAX_WEIGHT, MAX_BLOCK_WEIGHT - 4000), so the result is the same.
606 CScript scriptPubKey = CScript() << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f") << OP_CHECKSIG;
607 std::unique_ptr<CBlockTemplate> pblocktemplate;
608
609 fCheckpointsEnabled = false;
610
611+ CTxMemPool& tx_mempool{*m_node.mempool};
CTxMemPool& tx_mempool{MakeMempool()}; ?
No need for a shared mempool. Also remove unused chainparams parameter.
No need for a shared mempool. Also remove unused chainparams parameter.
No need for a shared mempool. Also remove unused chainparams parameter.
Can be reviewed with --ignore-all-space
