Reduce CTxMemPool constructor call sites
ClosedPublic
Actions

Authored by PiRK on Dec 4 2023, 12:55.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABCf4c252de3fdd: Reduce CTxMemPool constructor call sites

Summary

This PR reduces the number of call sites where we explicitly construct CTxMemPool. This is done in preparation for later PRs which decouple the mempool module from ArgsManager, eventually all of libbitcoinkernel will be decoupled from ArgsManager.

The changes in this PR:

Allows us to have less code churn as we modify CTxMemPool's constructor in later PRs
In many cases, we can make use of existing CTxMemPool instances, getting rid of extraneous constructions
In other cases, we construct a ChainTestingSetup and use the CTxMemPool there, so that we can rely on the logic in setup_common to set things up correctly

Backport note:

see D6451 for why the policy_estimator_test suite already uses a TestingSetup (which inherits ChainTestingSetup)
fixed an inconsistent lock order in a Bitcoin ABC specific test case in mempool_tests

This is a backport of core#25215

Test Plan

With clang and debug:

ninja all check-all bench-bitcoin

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Dec 4 2023, 12:55

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 4 2023, 12:55

PiRK requested review of this revision.Dec 4 2023, 12:55

teamcity edited the summary of this revision. (Show Details)Dec 4 2023, 12:55

Harbormaster completed remote builds in B25813: Diff 43382.Dec 4 2023, 13:08

Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Tail of the build log:

[0m[1;30mchronik_avalanche.py                       | ○ Skipped | 0 s
[0m[1;30mchronik_block.py                           | ○ Skipped | 0 s
[0m[1;30mchronik_block_info.py                      | ○ Skipped | 0 s
[0m[1;30mchronik_block_txs.py                       | ○ Skipped | 0 s
[0m[1;30mchronik_blockchain_info.py                 | ○ Skipped | 0 s
[0m[1;30mchronik_blocks.py                          | ○ Skipped | 0 s
[0m[1;30mchronik_chronik_info.py                    | ○ Skipped | 0 s
[0m[1;30mchronik_disallow_prune.py                  | ○ Skipped | 0 s
[0m[1;30mchronik_pause.py                           | ○ Skipped | 0 s
[0m[1;30mchronik_raw_tx.py                          | ○ Skipped | 0 s
[0m[1;30mchronik_resync.py                          | ○ Skipped | 0 s
[0m[1;30mchronik_script_confirmed_txs.py            | ○ Skipped | 0 s
[0m[1;30mchronik_script_history.py                  | ○ Skipped | 0 s
[0m[1;30mchronik_script_unconfirmed_txs.py          | ○ Skipped | 0 s
[0m[1;30mchronik_script_utxos.py                    | ○ Skipped | 0 s
[0m[1;30mchronik_serve.py                           | ○ Skipped | 0 s
[0m[1;30mchronik_spent_by.py                        | ○ Skipped | 0 s
[0m[1;30mchronik_tx.py                              | ○ Skipped | 0 s
[0m[1;30mchronik_ws.py                              | ○ Skipped | 0 s
[0m[1;30mchronik_ws_script.py                       | ○ Skipped | 0 s
[0m[1;30mfeature_bind_port_discover.py              | ○ Skipped | 0 s
[0m[1;30mfeature_bind_port_externalip.py            | ○ Skipped | 0 s
[0m[1;30minterface_usdt_net.py                      | ○ Skipped | 0 s
[0m[1;30minterface_usdt_utxocache.py                | ○ Skipped | 0 s
[0m[1;30minterface_usdt_validation.py               | ○ Skipped | 0 s
[0m[1m
ALL                                        | ✓ Passed  | 2105 s (accumulated) 
[0mRuntime: 422 s

[163/490] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.009s

OK
[165/490] cd /work/contrib/devtools/chainparams && /usr/bin/python3.9 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[314/490] bitcoin: testing mempool_tests
FAILED: src/test/CMakeFiles/check-bitcoin-mempool_tests 
cd /work/abc-ci-builds/build-debug/src/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-debug/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-debug/test/log && /usr/bin/cmake -E env /work/cmake/utils/log-and-print-on-failure.sh /work/abc-ci-builds/build-debug/test/log/bitcoin-mempool_tests.log /work/abc-ci-builds/build-debug/src/test/test_bitcoin --run_test=mempool_tests --logger=HRF,message:JUNIT,message,bitcoin-mempool_tests.xml --catch_system_errors=no
Running 7 test cases...
Assertion failed: detected inconsistent lock order for 'cs_main' in ../../src/test/mempool_tests.cpp:463 (in thread ''), details in debug log.
Aborted (core dumped)
[326/490] Running secp256k1 test suite
PASSED: secp256k1 test suite
[448/490] Running pow test suite
PASSED: pow test suite
[461/490] Running seeder test suite
PASSED: seeder test suite
[466/490] Running avalanche test suite
PASSED: avalanche test suite
[477/490] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[487/490] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1

[13:07:10]
[Step 1/1] FAILED: src/test/CMakeFiles/check-bitcoin-mempool_tests
[13:07:10]
[Step 1/1] cd /work/abc-ci-builds/build-debug/src/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-debug/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-debug/test/log && /usr/bin/cmake -E env /work/cmake/utils/log-and-print-on-failure.sh /work/abc-ci-builds/build-debug/test/log/bitcoin-mempool_tests.log /work/abc-ci-builds/build-debug/src/test/test_bitcoin --run_test=mempool_tests --logger=HRF,message:JUNIT,message,bitcoin-mempool_tests.xml --catch_system_errors=no
[13:07:10]
[Step 1/1] Running 7 test cases...
[13:07:10]
[Step 1/1] Assertion failed: detected inconsistent lock order for 'cs_main' in ../../src/test/mempool_tests.cpp:463 (in thread ''), details in debug log.

fixed an inconsistent lock order in a Bitcoin ABC specific test case. It looks like this was not an issue when using a brand new mempool, but is now an issue when we use the testing setup's pool

PiRK edited the test plan for this revision. (Show Details)Dec 5 2023, 07:56

Harbormaster completed remote builds in B25821: Diff 43395.Dec 5 2023, 08:17

Fabien accepted this revision.Dec 5 2023, 09:39

This revision is now accepted and ready to land.Dec 5 2023, 09:39

Closed by commit rABCf4c252de3fdd: Reduce CTxMemPool constructor call sites (authored by Carl Dong <contact@carldong.me>, committed by PiRK). · Explain WhyDec 5 2023, 12:48

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABCf4c252de3fdd: Reduce CTxMemPool constructor call sites.