test: Check datadir cleanup after assumeutxo was successful

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

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

648+            # Ensure the assumutxo node has cleaned up the background chainstate
649+            cleanup_msg = ["cleaning up unneeded background chainstate"] if i != 0 else []
650+            with n.assert_debug_log(cleanup_msg):
651+                self.restart_node(i, extra_args=self.extra_args[i])
652+            assert not (n.datadir_path / "regtest" / "chainstate_snapshot").exists()
653

aiswaryasankar commented at 0:31 am on March 12, 2025:

Ensures proper cleanup of background chainstate after assumeutxo node restart to prevent disk space leaks

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

Tested the PR with the following steps:

On master:

0python3 feature_assumeutxo.py
1...
2TestFramework (INFO): Tests successful

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

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

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

0python3 feature_assumeutxo.py
1...
2TestFramework (INFO): Tests successful

On PR branch:

0python3 feature_assumeutxo.py
1...
2TestFramework (INFO): Tests successful

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

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

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

TheCharlatan approved

TheCharlatan 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

TheCharlatan 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?

0diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
1index 89cf703fce..2166c4505d 100755
2--- a/test/functional/feature_assumeutxo.py
3+++ b/test/functional/feature_assumeutxo.py
4@@ -645,0 +646,2 @@ class AssumeutxoTest(BitcoinTestFramework):
5+            if i != 0:
6+                assert (n.datadir_path / "regtest" / "chainstate_snapshot").exists()
7@@ -728,0 +731,2 @@ class AssumeutxoTest(BitcoinTestFramework):
8+            if i != 0:
9+                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: none

tACK 0d10f1a

DrahtBot requested review from TheCharlatan 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

TheCharlatan 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):

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

TheCharlatan approved

TheCharlatan 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

TheCharlatan approved

TheCharlatan 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: none

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: none

Post ACK 52482cb

test: Check datadir cleanup after assumeutxo was successful #32033

Code Coverage & Benchmarks

Reviews

Conflicts

On master:

On PR branch:

NOTE