test: Check datadir cleanup after assumeutxo was successful #32033

fjahr commented at 1:45 PM on March 11, 2025: contributor

I noticed that the proper datadir cleanup after a successful restart of an assumutxo node does not seem to be covered in our tests. This is added here.

DrahtBot commented at 1:45 PM on March 11, 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/32033.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, l0rinc, mabu44, Prabhat1308

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29700 (kernel, refactor: return error status on all fatal errors 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.

DrahtBot added the label Tests on Mar 11, 2025

mabu44 commented at 7:48 PM on March 12, 2025: none

Tested the PR with the following steps:

On master:

python3 feature_assumeutxo.py
...
TestFramework (INFO): Tests successful

Modified validation.cpp (line 6237, function MaybeCompleteSnapshotValidation):

- return SnapshotCompletionResult::SUCCESS;
+ return SnapshotCompletionResult::SKIPPED;

This should avoid the clean-up of the snapshot at startup. But the test is not affected:

python3 feature_assumeutxo.py
...
TestFramework (INFO): Tests successful

On PR branch:

python3 feature_assumeutxo.py
...
TestFramework (INFO): Tests successful

Modified validation.cpp as before. This time the test fails as it should:

python3 feature_assumeutxo.py
...
TestFramework (ERROR): Assertion failed
...
AssertionError: [node 1] Expected messages "['cleaning up unneeded background chainstate']" does not partially match log:
...

NOTE

The change that I did to validation.cpp causes the "validation_chainstatemanager_tests" unit test to fail, so that line is covered. I don't know if there is a simple way to change the code that would result in success in all tests in master. Having the cleaning code covered in a functional test seems like a good idea anyway.

tACK dfd0a76fc1b46ddd1f4dab388ec2ca7fc3f76a2b

sedited approved

sedited commented at 9:14 PM on March 13, 2025: contributor

ACK dfd0a76fc1b46ddd1f4dab388ec2ca7fc3f76a2b

in test/functional/feature_assumeutxo.py:647 in dfd0a76fc1 outdated

 643 | @@ -644,7 +644,11 @@ def check_tx_counts(final: bool) -> None:
 644 |          for i in (0, 1):
 645 |              n = self.nodes[i]
 646 |              self.log.info(f"Restarting node {i} to ensure (Check|Load)BlockIndex passes")
 647 | -            self.restart_node(i, extra_args=self.extra_args[i])
 648 | +            # Ensure the assumutxo node has cleaned up the background chainstate

sedited commented at 9:16 PM on March 13, 2025:

Nit: Might it be worthwhile to also check existence beforehand to guard against naming changes, or the behaviour of the indexed nodes changing?

diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index 89cf703fce..2166c4505d 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -645,0 +646,2 @@ class AssumeutxoTest(BitcoinTestFramework):
+            if i != 0:
+                assert (n.datadir_path / "regtest" / "chainstate_snapshot").exists()
@@ -728,0 +731,2 @@ class AssumeutxoTest(BitcoinTestFramework):
+            if i != 0:
+                assert (n.datadir_path / "regtest" / "chainstate_snapshot").exists()

fjahr commented at 10:40 PM on March 14, 2025:

That's a good point, we might be getting the name wrong later and the test becomes meaningless. I addressed this and took that opportunity to refactor it a bit and remove the duplication.

fjahr force-pushed on Mar 14, 2025

Prabhat1308 commented at 6:50 AM on March 16, 2025: contributor

tACK 0d10f1a

DrahtBot requested review from sedited on Mar 16, 2025

in test/functional/feature_assumeutxo.py:344 in 0d10f1a664 outdated

 337 | @@ -337,6 +338,22 @@ def assert_only_network_limited_service(self, node):
 338 |          assert 'NETWORK' not in node_services
 339 |          assert 'NETWORK_LIMITED' in node_services
 340 |  
 341 | +    @contextlib.contextmanager
 342 | +    def assert_disk_cleanup(self, node, assumeutxo_used):
 343 | +        """
 344 | +        Ensure an assumutxo node is cleaning up the background chainstate

sedited commented at 12:30 PM on March 16, 2025:

Nit: s/assumutxo/assumeutxo/.

fjahr commented at 5:48 PM on March 16, 2025:

fixed

in test/functional/feature_assumeutxo.py:664 in 0d10f1a664 outdated

 660 | @@ -644,7 +661,8 @@ def check_tx_counts(final: bool) -> None:
 661 |          for i in (0, 1):
 662 |              n = self.nodes[i]
 663 |              self.log.info(f"Restarting node {i} to ensure (Check|Load)BlockIndex passes")
 664 | -            self.restart_node(i, extra_args=self.extra_args[i])
 665 | +            with self.assert_disk_cleanup(n, True if i == 1 else False):

sedited commented at 12:32 PM on March 16, 2025:

Nit: I think you can just pass in i==1 here, without the if else (and similarly below).

fjahr commented at 5:48 PM on March 16, 2025:

Indeed, nice.

sedited approved

sedited commented at 12:37 PM on March 16, 2025: contributor

Re-ACK 0d10f1a66436cd2ddab6b04247bcd6c4747cccc3

test: Check datadir cleanup after assumeutxo was successful 52482cb244

fjahr force-pushed on Mar 16, 2025

fjahr commented at 5:49 PM on March 16, 2025: contributor

Addressed @TheCharlatan 's comments, sorry for invalidating the ACKs again but diff should be easy to review: https://github.com/bitcoin/bitcoin/compare/0d10f1a66436cd2ddab6b04247bcd6c4747cccc3..52482cb24400f8c44ba9628aaaecb7c04b11beb2

sedited approved

sedited commented at 6:17 PM on March 16, 2025: contributor

Re-ACK 52482cb24400f8c44ba9628aaaecb7c04b11beb2

DrahtBot requested review from Prabhat1308 on Mar 16, 2025

in test/functional/feature_assumeutxo.py:349 in 52482cb244

 344 | +        Ensure an assumeutxo node is cleaning up the background chainstate
 345 | +        """
 346 | +        msg = []
 347 | +        if assumeutxo_used:
 348 | +            # Check that the snapshot actually existed before restart
 349 | +            assert (node.datadir_path / node.chain / "chainstate_snapshot").exists()

l0rinc commented at 8:08 PM on March 16, 2025:

nit: node.datadir_path / node.chain / "chainstate_snapshot" could be extracted to obviate that it's checked multiple times

fjahr commented at 8:57 PM on March 16, 2025:

Will do if I have to retouch

l0rinc commented at 8:10 PM on March 16, 2025: contributor

utACK 52482cb24400f8c44ba9628aaaecb7c04b11beb2

The build failure seems unrelated

mabu44 commented at 11:32 PM on March 16, 2025: none

Re-ACK 52482cb24400f8c44ba9628aaaecb7c04b11beb2

Prabhat1308 commented at 7:45 AM on March 17, 2025: contributor

re-ACK 52482cb

hebasto merged this on Mar 17, 2025

hebasto closed this on Mar 17, 2025

musaHaruna commented at 11:03 AM on March 18, 2025: contributor

Post ACK 52482cb

stickies-v referenced this in commit 5443a6eac4 on Mar 21, 2025

stickies-v referenced this in commit a8bd94de4e on Mar 21, 2025

stickies-v referenced this in commit 5571e63c54 on Mar 21, 2025

sedited referenced this in commit 50d4368616 on Mar 24, 2025

yuvicc referenced this in commit 069643f094 on Jul 6, 2025

bug-castercv502 referenced this in commit ff634b425c on Sep 28, 2025

Kino1994 referenced this in commit 5e9046cd19 on Jun 28, 2026

BigcoinBGC referenced this in commit 7e224642a5 on Jun 30, 2026

PastaPastaPasta referenced this in commit 66e5d5ddd3 on Jul 12, 2026

PastaPastaPasta referenced this in commit 62be0d212a on Jul 12, 2026

PastaPastaPasta referenced this in commit 6b814910ce on Jul 14, 2026

PastaPastaPasta referenced this in commit 7667bd2e6c on Jul 14, 2026

PastaPastaPasta referenced this in commit c243c95f71 on Jul 20, 2026

bitcoin locked this on Jul 30, 2026