test: Cleanup miner_tests #25073

maflcko commented at 12:17 PM on May 6, 2022: member

This cleans up the miner tests:

Removes duplicate/redundant and thus confusing chainparams object.
Uses a fresh mempool for each subtest instead of using the "global" one from the testing setup. This makes it easier to follow the tests in smaller scopes. Also it makes sure the mempool is truly cleared by reconstructing it. Finally, this removes calls to clear, see https://github.com/bitcoin/bitcoin/pull/19909

fanquake added the label Tests on May 6, 2022

DrahtBot commented at 8:21 PM on May 6, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24567 (Enforce BIP 68 from genesis by MarcoFalke)
#23897 (refactor: Move calculation logic out from 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.

laanwj commented at 12:16 PM on May 10, 2022: member

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.

maflcko commented at 12:26 PM on May 10, 2022: member

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?

DrahtBot added the label Needs rebase on May 13, 2022

maflcko force-pushed on May 13, 2022

DrahtBot removed the label Needs rebase on May 13, 2022

DrahtBot added the label Needs rebase on May 20, 2022

maflcko force-pushed on May 27, 2022

maflcko commented at 2:31 PM on May 27, 2022: member

Rebased. Should still be trivial to re-ACK with git range-diff bitcoin-core/master dddd32 fabdc1902e.

DrahtBot removed the label Needs rebase on May 27, 2022

DrahtBot added the label Needs rebase on Jun 15, 2022

maflcko force-pushed on Jun 17, 2022

DrahtBot removed the label Needs rebase on Jun 17, 2022

in src/test/miner_tests.cpp:94 in faf2450f07 outdated

  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};

laanwj commented at 6:44 PM on June 17, 2022:

I think this conflicts with @dongcarl 's intent to construct less mempools (although maybe this isn't a problem as this is instantiated with special parameters).

maflcko commented at 1:48 PM on June 22, 2022:

Good point. Fixed by constructing a mempool in only one place

maflcko force-pushed on Jun 22, 2022

maflcko commented at 3:59 PM on June 22, 2022: member

Fixed feedback and rebased

maflcko force-pushed on Jul 22, 2022

fanquake requested review from glozow on Jul 22, 2022

glozow commented at 10:23 AM on July 25, 2022: member

Concept ACK to removing unused 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()?

maflcko commented at 10:28 AM on July 25, 2022: member

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.

fanquake requested review from instagibbs on Jul 25, 2022

dongcarl commented at 5:56 PM on July 25, 2022: contributor

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:

Each block of 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 footgun
Perhaps we add a ReplaceMempool method to TestingSetup to fix the CChainState member like we do in DummyChainState? Not sure...

maflcko force-pushed on Jul 26, 2022

maflcko commented at 6:34 AM on July 26, 2022: member

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.

dongcarl commented at 4:10 PM on July 26, 2022: contributor

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!

DrahtBot added the label Needs rebase on Aug 4, 2022

maflcko force-pushed on Aug 5, 2022

DrahtBot removed the label Needs rebase on Aug 5, 2022

DrahtBot added the label Needs rebase on Aug 12, 2022

maflcko force-pushed on Aug 12, 2022

fanquake requested review from glozow on Aug 12, 2022

DrahtBot removed the label Needs rebase on Aug 12, 2022

ryanofsky approved

ryanofsky commented at 6:05 PM on August 18, 2022: contributor

Code review ACK fac107b9cc275e46e78602bad1ea0d2124c7adbb. Seems good to add scopes and stop hardcoding 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.

in src/test/miner_tests.cpp:639 in fa81221bea outdated

 636 | @@ -604,9 +637,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
 637 |      }
 638 |  
 639 |      LOCK(cs_main);

glozow commented at 8:25 AM on August 19, 2022:

~in fa81221bea231dc12cf74482a5878be524e43c48, the now-unused declarations of 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

maflcko commented at 11:40 AM on October 5, 2022:

Thanks, squashed

in src/test/miner_tests.cpp:556 in fa8f828e8f outdated

 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);

glozow commented at 9:39 AM on August 19, 2022:

Could remove the chainparams include since it's no longer used after fa8f828e8f98caf3b1f1e08127d16959ec7d1b43?

maflcko commented at 11:40 AM on October 5, 2022:

Thanks, removed include

in src/test/miner_tests.cpp:40 in fac107b9cc outdated

  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()

glozow commented at 9:40 AM on August 19, 2022:

Seems like fac107b9cc275e46e78602bad1ea0d2124c7adbb could have been squashed into fa5d4c3d98e5ddcd59a8ac5b8f296ceb245e5fce and have 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.

maflcko commented at 11:40 AM on October 5, 2022:

Thanks, squashed

in src/test/miner_tests.cpp:656 in fac107b9cc outdated

 657 | -    m_node.mempool->clear();
 658 |  
 659 | -    TestPrioritisedMining(chainparams, scriptPubKey, txFirst);
 660 | +    TestPrioritisedMining(scriptPubKey, txFirst);
 661 |  
 662 |      fCheckpointsEnabled = true;

glozow commented at 9:51 AM on August 19, 2022:

Not touched in this PR, but these fCheckPointsEnabled assignments seem to have no impact on this test? Could probably be cleaned up?

maflcko commented at 12:10 PM on August 19, 2022:

Correct. See commit "test: Remove unused fCheckpointsEnabled from miner_tests" from #25704

maflcko commented at 11:40 AM on October 5, 2022:

Thanks, rebased (now gone in master)

in src/test/miner_tests.cpp:62 in fac107b9cc outdated

  65 | +BlockAssembler MinerTestingSetup::AssemblerForTest(CTxMemPool& tx_mempool)
  66 |  {
  67 |      BlockAssembler::Options options;
  68 |  
  69 |      options.nBlockMaxWeight = MAX_BLOCK_WEIGHT;
  70 |      options.blockMinFeeRate = blockMinFeeRate;

glozow commented at 10:01 AM on August 19, 2022:

Seems the 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.

maflcko commented at 11:41 AM on October 5, 2022:

Is this related to my changes? If not, maybe ping me for review on a separate pull?

glozow commented at 11:47 AM on October 6, 2022:

Unrelated, sorry.

in src/test/miner_tests.cpp:606 in fac107b9cc outdated

 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};

glozow commented at 10:06 AM on August 19, 2022:

Question: why doesn't this end up as CTxMemPool& tx_mempool{MakeMempool()}; ?

maflcko commented at 12:12 PM on August 19, 2022:

I don't know. There shouldn't be any difference. Happy to add a commit to change it.

maflcko commented at 11:46 AM on October 5, 2022:

There is only one test case in this file and this is the first use of the mempool, so we can use the existing one from the testing setup, or a freshly made one. Both should be "clean", so either can be used. Maybe someone can flip a coin?

glozow commented at 10:09 AM on August 19, 2022: member

Seems fine to me. I had a few questions and wonder if other cleanups are possible too...

DrahtBot added the label Needs rebase on Oct 4, 2022

test: Pass mempool reference to AssemblerForTest fa29218285

test: Use dedicated mempool in TestPrioritisedMining

No need for a shared mempool. Also remove unused chainparams parameter.

fa4055d79c

test: Use dedicated mempool in TestPackageSelection

No need for a shared mempool. Also remove unused chainparams parameter.

fafab384a0

test: Use dedicated mempool in TestBasicMining

No need for a shared mempool. Also remove unused chainparams parameter.

Can be reviewed with --ignore-all-space

faa15527d7

maflcko force-pushed on Oct 5, 2022

maflcko commented at 11:47 AM on October 5, 2022: member

Rebased, Squashed, and replied to comments

DrahtBot removed the label Needs rebase on Oct 5, 2022

glozow commented at 11:49 AM on October 6, 2022: member

utACK faa15527d7e0c7923ff9c0fad7bab4648705ed0f

fanquake merged this on Oct 10, 2022

fanquake closed this on Oct 10, 2022

maflcko deleted the branch on Oct 10, 2022

sidhujag referenced this in commit 1d932c8c4c on Oct 10, 2022

bitcoin locked this on Oct 10, 2023