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

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

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

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?

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