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
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
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
faf2450f07outdated
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};
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).
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
maflcko force-pushed
on Aug 5, 2022
maflcko force-pushed
on Aug 5, 2022
maflcko force-pushed
on Aug 5, 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 ACKfac107b9cc275e46e78602bad1ea0d2124c7adbb. 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
fa81221beaoutdated
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
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.
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.
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 AssemblerForTestfa29218285
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
utACKfaa15527d7e0c7923ff9c0fad7bab4648705ed0f
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
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-05-08 03:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me