test: Cleanup miner_tests

[maflcko]

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 refactor: Remove unused CTxMemPool::clear() helper #19909

[DrahtBot]

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]

Code review ACK dddd32e

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]

Hmm. For reference, 38860f9#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?

[maflcko]

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

[maflcko]

Fixed feedback and rebased

[glozow]

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

[dongcarl]

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!

[ryanofsky]

Code review ACK fac107b. 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.

[glozow]

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

[maflcko]

Rebased, Squashed, and replied to comments

[glozow]

utACK faa1552