test: Add bitcoin-chainstate test for assumeutxo functionality

stringintech commented at 9:44 pm on October 28, 2025: contributor

This PR adds functional test coverage for the bitcoin-chainstate tool loading a datadir initialized with an assumeutxo snapshot.

The PR also includes:

Fix for assertion crash in ActivateExistingSnapshot() when active chainstate has no initialized mempool (required for the test to pass)
-regtest flag support for bitcoin-chainstate to enable the testing

This work started while experimenting with how the current state of the kernel API (#30595) behaves when loading a datadir containing assumeutxo data, and @TheCharlatan suggested adding the same test coverage for the current state of the bitcoin-chainstate tool.

DrahtBot added the label Tests on Oct 28, 2025

DrahtBot commented at 9:44 pm on October 28, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33728.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan
Concept ACK	yuvicc
Stale ACK	frankomosh

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

in src/bitcoin-chainstate.cpp:65 in 986eb77560

61@@ -61,6 +62,8 @@ int main(int argc, char* argv[])
62     fs::path abs_datadir{fs::absolute(argv[1])};
63     fs::create_directories(abs_datadir);
64 
65+    const bool use_regtest{(argc == 3 && std::string(argv[2]) == "-regtest")};

frankomosh commented at 6:38 am on October 29, 2025:

If user passes 3 args where argv[2] != "-regtest"(or generally malformed argument combinations), could it silently set use_regtest = false without warning?

If thats the case then I think there should atleast be a warning

stringintech commented at 3:14 pm on October 29, 2025:

Yes I think it is better to have validation on this.

in src/validation.cpp:6340 in 986eb77560 outdated

6320@@ -6321,7 +6321,7 @@ Chainstate& ChainstateManager::ActivateExistingSnapshot(uint256 base_blockhash)
6321     LogInfo("[snapshot] switching active chainstate to %s", m_snapshot_chainstate->ToString());
6322 
6323     // Mempool is empty at this point because we're still in IBD.
6324-    Assert(m_active_chainstate->m_mempool->size() == 0);
6325+    Assert(!m_active_chainstate->m_mempool || m_active_chainstate->m_mempool->size() == 0);

frankomosh commented at 6:41 am on October 29, 2025:

Is this now weaker that the original assertion, because I think it will now silently accepts a null mempool pointer?

TheCharlatan commented at 12:33 pm on October 29, 2025:

It is weaker, yes, but we similarly guard in other situations. We also still have the assertion in ActivateSnapshot to guard this condition at a more critical point in time.

frankomosh commented at 6:49 am on October 29, 2025: contributor

Concept ACK I think its a nice addition as it adds valuable functional coverage for bitcoin-chainstate. Few nits and inquiries inline;

in src/bitcoin-chainstate.cpp:54 in 986eb77560

48@@ -49,10 +49,11 @@ int main(int argc, char* argv[])
49     LogInstance().DisableLogging();
50 
51     // SETUP: Argument parsing and handling
52-    if (argc != 2) {
53+    if (argc < 2 || argc > 3) {
54         std::cerr
55-            << "Usage: " << argv[0] << " DATADIR" << std::endl
56+            << "Usage: " << argv[0] << " DATADIR [-regtest]" << std::endl

TheCharlatan commented at 11:19 am on October 29, 2025:

Since this is an optional positional argument, I think I would drop the - prefix.

stringintech commented at 3:13 pm on October 29, 2025:

I thought if I want to treat it as boolean, it might be better to introduce it as a flag with the dash prefix. Would it make more sense to you if I drop the dash and treat it as a chain name instead (where only regtest and mainnet are supported, and if absent, defaults to mainnet)?

TheCharlatan commented at 9:34 pm on October 29, 2025:

Ah, I see. I think I would change the order then, since typically named optional args precede positional arguments / operands in command line applications: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_01 .

TheCharlatan commented at 12:34 pm on October 29, 2025: contributor

Concept ACK

Consolidating the tests into the regtest case seems fine to me, especially since this is just a utility to showcase and doogfood some of the kernel work.

stringintech force-pushed on Oct 30, 2025

stringintech commented at 11:12 am on October 30, 2025: contributor

986eb77 to 3e7d272 - reversed the order of -regtest flag and input DATADIR as suggested by @TheCharlatan and added simple validation for the flag as suggested by @frankomosh.

TheCharlatan approved

TheCharlatan commented at 2:01 pm on October 30, 2025: contributor

ACK 3e7d272b2747f1937010b8fc1fefafcfa0ce3213

DrahtBot requested review from frankomosh on Oct 30, 2025

yuvicc commented at 2:37 pm on October 30, 2025: contributor

Concept ACK

frankomosh commented at 6:40 am on November 4, 2025: contributor

ACK 3e7d272

Add regtest support to bitcoin-chainstate tool

Adds -regtest flag to enable testing with regtest chain parameters.

f481d99229

Fix `ActivateExistingSnapshot()` assertion crash

Check mempool exists before accessing size when active chainstate doesn't have initialized mempool.

e1339bb4b5

test: Add bitcoin-chainstate test for assumeutxo functionality

Adds functional test coverage for bitcoin-chainstate tool loading a datadir initialized with an assumeutxo snapshot

67740dfe6c

stringintech force-pushed on Nov 4, 2025

stringintech commented at 4:28 pm on November 4, 2025: contributor

3e7d272 to 67740df - rebased and resolved conflict with #30595.

TheCharlatan approved

TheCharlatan commented at 10:15 am on November 5, 2025: contributor

re-ACK 67740dfe6cfe3f3f91aaad41018a7da9004dcd8c

test: Add bitcoin-chainstate test for assumeutxo functionality #33728

Code Coverage & Benchmarks

Reviews

Conflicts