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 ChainstateManager::AddChainstate() when prev_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 the bitcoin-chainstate tool and how the kernel API (#30595) behaved when loading a datadir containing assumeutxo data, during the time that PR was still under review. sedited suggested opening a PR to add this test coverage.

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	theStack, sedited, achow101
Concept ACK	yuvicc
Stale ACK	frankomosh

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33847 (kernel: Improve logging API by ryanofsky)

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.

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:6285 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?

sedited 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

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

sedited 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 .

sedited 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.

sedited approved

sedited 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

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.

sedited approved

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

re-ACK 67740dfe6cfe3f3f91aaad41018a7da9004dcd8c

fanquake removed review request from frankomosh on Nov 20, 2025

fanquake requested review from stickies-v on Nov 20, 2025

?

added_to_project_v2 sedited

?

project_v2_item_status_changed sedited

frankomosh commented at 6:13 am on December 9, 2025: contributor

re-ACK 67740df

stringintech force-pushed on Dec 9, 2025

stringintech commented at 10:38 am on December 9, 2025: contributor

67740df to 6cb63db - Rebased and removed unused variable block_tx in tool_bitcoin_chainstate.py. Also updated PR description.

sedited approved

sedited commented at 12:14 pm on December 9, 2025: contributor

Re-ACK 6cb63db8a97b8ea1dbee5edaff30e6da8b6ed108

DrahtBot requested review from frankomosh on Dec 9, 2025

in src/bitcoin-chainstate.cpp:148 in bfceb4c275 outdated

130@@ -131,16 +131,18 @@ class TestKernelNotifications : public KernelNotifications
131 int main(int argc, char* argv[])
132 {
133     // SETUP: Argument parsing and handling
134-    if (argc != 2) {
135+    const bool has_regtest_flag{argc == 3 && std::string(argv[1]) == "-regtest"};
136+    if (argc < 2 || argc > 3 || (argc == 3 && !has_regtest_flag)) {

sedited commented at 2:37 pm on December 10, 2025:

I’ve been thinking lately whether we can reuse some of our own common utilities for these small, kernel-based applications. These small, single use applications should be able to use our usual utilities, while still benefiting from a clearer kernel interface. I think we are at a point now, where we can confidently mix between the two without risking introducing an entanglement regression between what the kernel requires and what new code might introduce in it. For this reason, I added some basic argument handling on my branch over here: https://github.com/sedited/bitcoin/tree/bitcoin_chainstate_args , that might be interesting for this change here.

stringintech commented at 7:10 pm on December 10, 2025:

Makes sense, thanks! It seems I can cherry-pick your commit as is to replace bfceb4c?

sedited commented at 9:49 am on December 16, 2025:

Makes sense, thanks! It seems I can cherry-pick your commit as is to replace https://github.com/bitcoin/bitcoin/commit/bfceb4c275f7df403d7fbc2e114934988cc8111c?

Sure, if you think it is a good idea too :)

stringintech commented at 5:05 pm on December 18, 2025:

For reviewers:

As discussed offline, adding ArgsManager and the G_TRANSLATION_FUN definition that should be added because of it, is causing different compile/link issues since the symbol is also defined in the kernel library and @sedited is suggesting to have the symbol removed from there.

So it seems adapting bitcoin-chainstate.cpp to use the ArgsManager could be done in a follow-up.

?

project_v2_item_status_changed sedited

DrahtBot added the label Needs rebase on Dec 16, 2025

Add regtest support to bitcoin-chainstate tool

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

5f3d6bdb66

Fix `ChainstateManager::AddChainstate()` assertion crash

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

2bc3265649

test: Add bitcoin-chainstate test for assumeutxo functionality

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

7b5d256af4

stringintech force-pushed on Dec 18, 2025

stringintech commented at 5:14 pm on December 18, 2025: contributor

6cb63db to 7b5d256 - Rebased and resolved conflict with #30214.

DrahtBot removed the label Needs rebase on Dec 18, 2025

theStack approved

theStack commented at 8:27 am on January 5, 2026: contributor

Concept and code-review ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d

As a follow-up idea, it might make sense to deduplicate the deterministic generation of the snapshot chain between this test and feature_assumeutxo.py, by moving the logic into a test framework module.

DrahtBot requested review from sedited on Jan 5, 2026

sedited approved

sedited commented at 9:09 am on January 5, 2026: contributor

Re-ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d

achow101 commented at 10:19 pm on January 14, 2026: member

ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d

achow101 merged this on Jan 14, 2026

achow101 closed this on Jan 14, 2026

?

project_v2_item_status_changed github-project-automation[bot]

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

Code Coverage & Benchmarks

Reviews

Conflicts