test: Use BasicTestingSetup when sufficient

hodlinator commented at 8:39 pm on March 25, 2026: contributor

Improves run-times (4 warmups + 60 runs each):

test	improvement
checkqueue_tests	1.12
merkle_tests	1.14
orphanage_tests	1.48
prevector_tests	1.07
rbf_tests	1.01
validation_tests	1.60

Removed overhead is barely measurable on full test suite runtime sequential test_bitcoinor parallel ctest --test-dir build.

Initial version of PR also extracted a DataDirTestingSetup from BasicTestingSetup, making the latter not use the disk, but the added complexity didn’t deliver sufficient speedups to clearly justify itself.

Follow-up to #34562 (review) Arguably in a similar vein as #22086.

refactor(test): txdownload - Specify TestChain100Setup once 742309b9e2

test: Use BasicTestingSetup when TestingSetup is not necessary 794ba77ada

DrahtBot added the label Tests on Mar 25, 2026

DrahtBot commented at 8:39 pm on March 25, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, w0xlt

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

maflcko commented at 8:46 am on March 26, 2026: member

Interesting approach, but I wonder if all the code is worth it for a few ms of savings, that aren’t usually visible at all?

hodlinator commented at 10:31 am on March 26, 2026: contributor

Interesting approach, but I wonder if all the code is worth it for a few ms of savings, that aren’t usually visible at all?

Thanks for checking it out!

Yeah, I’m a bit underwhelmed by the small difference as well. I’ve only tested on NVMe, but nobody should be running tests on bare HDD anyway.

I’ve re-run benchmarks to check the difference from just the first 2 commits (replacing TestingSetup with BasicTestingSetup).

test	improvement
checkqueue_tests	1.12
merkle_tests	1.14
orphanage_tests	1.48
prevector_tests	1.07
rbf_tests	1.01
validation_tests	1.60

(Not sure why orphanage_tests was measured to run even quicker, did 60 runs).

Maybe it makes sense to shave off the latter commits and just go for the bare minimum. Although it might be interesting to also measure the effect of lighter weight BasicTestingSetup on fuzz-tests. Any ideas on how to measure that are very much welcome.

maflcko commented at 10:49 am on March 26, 2026: member

I’ve re-run benchmarks to check the difference from just the first 2 commits (replacing TestingSetup with BasicTestingSetup).

I wonder if there is a trivial heuristic to use here, so that ideally tests are self-validating and fail at runtime if their setup is too expensive. But I guess this again will be extra code, which may not be worth it.

So maybe just pick the two commits, which should be correct, if the test passes, right?

fuzz-tests. Any ideas on how to measure that are very much welcome.

I think the fuzz tests use an expensive init function and run that only once, after which they can run for minutes/hours/days. Saving a few milliseconds there won’t even show up when running for a minute.

hodlinator force-pushed on Mar 26, 2026

hodlinator renamed this:
~~test: Make *TestingSetup more lightweight~~
test: Use BasicTestingSetup when sufficient
on Mar 26, 2026

hodlinator commented at 11:39 am on March 26, 2026: contributor

I wonder if there is a trivial heuristic to use here, so that ideally tests are self-validating and fail at runtime if their setup is too expensive. But I guess this again will be extra code, which may not be worth it.

Yeah, would be cool if one could use code coverage and some lightweight code annotation to figure out unused test dependencies.

So maybe just pick the two commits, which should be correct, if the test passes, right?

Done!

I think the fuzz tests use an expensive init function and run that only once, after which they can run for minutes/hours/days. Saving a few milliseconds there won’t even show up when running for a minute.

True, it would only be somewhat measurable when doing something like running ./build_fuzz/test/fuzz/test_runner.py without --empty_min_time which uses -runs=1, and that’s not very interesting.

I’m a bit mystified by why I felt such an inclination towards optimizing this. It’s not like it’s helping end-users running nodes on HDD. At least it made me get a feel for how cheap the NodeContext used by BasicTestingSetup actually is.

maflcko commented at 6:41 pm on March 26, 2026: member

lgtm, should be fine when the tests pass. If there was an issue, I’d expect a nullptr deref.

review ACK 794ba77adafcb2d0ba138cc17da5f6bd1ff38c0e 😼

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 794ba77adafcb2d0ba138cc17da5f6bd1ff38c0e 😼
3yHFa8OqrKx1adaEIY/R4OOCeVcFBsS+1rvX/qktzt7Vs0zL0GQFo0nWNUSG0Q96byNIglhcpxs3U5V7UvpbnAg==

stickies-v commented at 7:13 pm on March 26, 2026: contributor

~0. Looks like a nice little clean up but I’m not sure how I can make it trivial to assure myself that the TestingSetup and ChainTestingSetup constructors didn’t do any initialization that might affect the tests, and I’m not sure it’s worth digging through the code for such small gains? So I don’t think I’ll revisit this until the benefits obviously outweight the costs.

edit: to be clear, I’m not against this PR. If people more familiar with the affected code find this trivial to review, it absolutely should make progress. For me, the trade-off just isn’t there.

w0xlt commented at 8:32 pm on March 26, 2026: contributor

lgtm ACK 794ba77adafcb2d0ba138cc17da5f6bd1ff38c0e

hodlinator commented at 8:59 pm on March 26, 2026: contributor

@stickies-v, thanks for having a glance and taking the time to communicate your impression.

I’m not sure how I can make it trivial to assure myself that the TestingSetup and ChainTestingSetup constructors didn’t do any initialization that might affect the tests, and I’m not sure it’s worth digging through the code for such small gains? So I don’t think I’ll revisit this until the benefits obviously outweight the costs.

Here is an overview:

check_queue_tests.cpp - Tests properties of a fairly self-contained data structure.
merkle_tests.cpp - Tests the pure functions from consensus/merkle.h, the only #include outside of test headers.
orphanage_tests.cpp - Focuses on individual transactions, two test cases call orphanage->EraseForBlock(block) but the block’s are not part of a chain.
prevector_tests.cpp - No comment
rbf_tests.cpp - All test cases override the fixture to use TestChain100Setup except the last one (“feerate_chunks_utilities”), which is trivial to review.
txdownload_tests.cpp - Pure refactor, uses the same *TestingSetup as on master.
validation_tests.cpp - These are quite clear, verifying different properties of consensus / chain params. Maybe this one would be the most consequential to break. I could potentially drop it if you insist?

It’s unfortunate that the code base often uses globals rather than dependency injection. But it is generally written in a style where tests would trigger nullptr deref as pointed out in #34922 (comment). In early prototypes of what later became this PR I was even trying to make a LogTestingSetup that didn’t create an ECC_Context nor select chain params, both lead to clear test failures when they were required by a test.

in src/test/txdownload_tests.cpp:20 in 742309b9e2

16@@ -17,7 +17,7 @@
17 
18 #include <boost/test/unit_test.hpp>
19 
20-BOOST_FIXTURE_TEST_SUITE(txdownload_tests, TestingSetup)
21+BOOST_FIXTURE_TEST_SUITE(txdownload_tests, TestChain100Setup)

furszy commented at 0:45 am on March 27, 2026:

I’m not sure about this initial commit. It looks like it could be overlooked, causing any new test in this file to unnecessarily generate the chain when they might not need it.

hodlinator commented at 9:26 am on March 27, 2026:

True, there is a window between this PR being created and merged in which someone could start adding a test case to this file and we end up with a kind of silent merge-conflict. As you say, in the unlikely event that it does happen, the net result would be that the added test case(s) unnecessarily generate chains. I guess there’s also a very small risk that it would lead to false positives in tests.

Have you seen anyone working on tests in this area lately?

furszy commented at 1:44 pm on March 27, 2026:

I wasn’t talking about the merge window. It was more like: “When writing a test, I rarely check the base class used in BOOST_FIXTURE_TEST_SUITE, which in this case means the new test would create an extra chain for no reason”.

I’m not strong on this, it just seems slightly better to be specific when using TestChain100Setup. It is also more descriptive for the particular test case.

hodlinator commented at 8:58 pm on March 27, 2026:

Hm… sounds like it might be worth adding a linter to prevent test suites from directly specifying the costly TestChain100Setup at that level, instead requiring it to be specified on the test case level? Only found 2 violations on relatively recent commit.

0₿ git grep "BOOST_FIXTURE_TEST_SUITE.*TestChain100Setup"
1src/test/disconnected_transactions.cpp:BOOST_FIXTURE_TEST_SUITE(disconnected_transactions, TestChain100Setup)
2src/test/interfaces_tests.cpp:BOOST_FIXTURE_TEST_SUITE(interfaces_tests, TestChain100Setup)

test: Use BasicTestingSetup when sufficient #34922

Reviews