test: Check that reindex with prune wipes blk files

maflcko commented at 10:58 am on January 21, 2025: member

This adds missing test coverage for CleanupBlockRevFiles.

test: Use high-level python types

Using the built-in open() and pathlib is identical and requires less code.

Also, remove redundant sync_blocks call.

fa9593efc2

DrahtBot commented at 10:58 am on January 21, 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/31696.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, l0rinc, tdb3

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

DrahtBot renamed this:
~~test: Check that reindex with prune wipes blk files~~
test: Check that reindex with prune wipes blk files
on Jan 21, 2025

DrahtBot added the label Tests on Jan 21, 2025

in test/functional/feature_remove_pruned_files_on_startup.py:54 in fa975d1075 outdated

65-        assert not os.path.exists(blk0)
66-        assert not os.path.exists(rev1)
67+        assert not blk0.exists()
68+        assert not rev1.exists()
69+
70+        self.log.info("Check that a reindex will wipe all files")

sedited commented at 11:44 am on January 21, 2025:

Might it be good to check the file count here before deletion during the reindex?

maflcko force-pushed on Jan 21, 2025

sedited approved

sedited commented at 12:28 pm on January 21, 2025: contributor

ACK fab411ab37df97d93a23feb2a2714863687c8800

maflcko commented at 1:22 pm on January 21, 2025: member

ACK fa975d1

Looks like this was a stale commit?

sedited commented at 1:49 pm on January 21, 2025: contributor

Looks like this was a stale commit?

Fixed.

in test/functional/feature_remove_pruned_files_on_startup.py:62 in fab411ab37 outdated

57+            return sum(
58+                entry.is_file() and any(map(entry.name.startswith, ["rev", "blk"]))
59+                for entry in self.nodes[0].blocks_path.iterdir()
60+            )
61+
62+        assert_equal(file_count(), 4)

tdb3 commented at 3:08 am on January 22, 2025:

nit: If retouching, could increase readability with a comment or log explaining that the check is for presence of non-00000/00001 files (blk00002.dat/rev00002.dat/blk00003.dat/rev00003.dat)

in test/functional/feature_remove_pruned_files_on_startup.py:66 in fab411ab37 outdated

61+
62+        assert_equal(file_count(), 4)
63+        self.restart_node(0, extra_args=self.extra_args[0] + ["-reindex"])
64+        assert_equal(self.nodes[0].getblockcount(), 0)
65+        self.stop_node(0)  # Stop node to flush the two newly created files
66+        assert_equal(file_count(), 2)

tdb3 commented at 3:08 am on January 22, 2025:

nit: and for presence of blk00000.dat/rev00000.dat

maflcko commented at 8:25 am on January 22, 2025:

thx, used an exact match here on the filenames

tdb3 approved

tdb3 commented at 3:09 am on January 22, 2025: contributor

ACK fab411ab37df97d93a23feb2a2714863687c8800

Left some non-block readability nits. Feel free to ignore.

PRADACANDI18 approved

PRADACANDI18 commented at 4:05 am on January 22, 2025: none

fa9593efc2e19d9dfa9a62c8a3d796aad888baa0

test: Check that reindex with prune wipes blk files fa9aced800

maflcko force-pushed on Jan 22, 2025

sedited approved

sedited commented at 7:59 am on January 23, 2025: contributor

Re-ACK fa9aced8006bd9b10b9239b43140c37162c9d8f2

DrahtBot requested review from tdb3 on Jan 23, 2025

in test/functional/feature_remove_pruned_files_on_startup.py:46 in fa9593efc2 outdated

55+            assert not rev0.exists()
56+            assert not blk1.exists()
57+            assert rev1.exists()
58 
59-        # Check that the files are removed on restart once the fds are closed
60+        self.log.info("Check that the files are removed on restart once the fds are closed")

l0rinc commented at 9:39 am on January 23, 2025:

👍

in test/functional/feature_remove_pruned_files_on_startup.py:21 in fa9593efc2 outdated

17@@ -18,38 +18,38 @@ def mine_batches(self, blocks):
18         for _ in range(n):
19             self.generate(self.nodes[0], 250)
20         self.generate(self.nodes[0], blocks % 250)
21-        self.sync_blocks()

l0rinc commented at 9:40 am on January 23, 2025:

we don’t need this because of num_nodes = 1, right?

in test/functional/feature_remove_pruned_files_on_startup.py:29 in fa9593efc2 outdated

29-        fo1 = os.open(blk0, os.O_RDONLY)
30-        fo2 = os.open(rev1, os.O_RDONLY)
31-        fd1 = os.fdopen(fo1)
32-        fd2 = os.fdopen(fo2)
33+
34+        self.log.info("Open some files to check that this may delay deletion")

l0rinc commented at 9:46 am on January 23, 2025:

Wasn’t sure what “may delay deletion” referred to, had to read the whole test to understand it, maybe this could help:

0        self.log.info("Check that opening files may delay their deletion")

in test/functional/feature_remove_pruned_files_on_startup.py:62 in fa9aced800

73+            ls = [
74+                entry.name
75+                for entry in self.nodes[0].blocks_path.iterdir()
76+                if entry.is_file() and any(map(entry.name.startswith, ["rev", "blk"]))
77+            ]
78+            return sorted(ls)

l0rinc commented at 10:24 am on January 23, 2025:

Given that this pr was about identical and requires less code, it seems to me we can express this in a more compact form as well:

0        ls_files = lambda: sorted(f.name for f in self.nodes[0].blocks_path.glob("*0*.dat"))

maflcko commented at 10:38 am on January 23, 2025:

Given that this pr was about identical and requires less code, it seems to me we can express this in a more compact form as well:

No, this pr is about “This adds missing test coverage” (see the description). The other commit isn’t needed and I am happy to drop it, if you want.

Also, I don’t think your glob is identical, because it can match dat files other than rev/blk (also hidden files). This may or may not be fine right now, but I am even less sure about the future.

l0rinc commented at 10:48 am on January 23, 2025:

Also, I don’t think your glob is identical, because it can match dat files other than rev/blk

Got it, what do you think about:

0ls_files = lambda: sorted(f.name for f in self.nodes[0].blocks_path.iterdir() if f.name.startswith(("rev", "blk")))

l0rinc commented at 10:50 am on January 23, 2025:

I am happy to drop it, if you want.

Of course not, what I want is maintainable code with high signal-to-noise ratio. But if you don’t think my alternative suggestions make the code more maintainable, I’m not forcing you to accept it, I’ve already ACK-ed.

maflcko commented at 11:31 am on January 23, 2025:

This already has two acks, so I think it is better to leave it as-is and keep the [ ] for now, even if they are redundant.

l0rinc commented at 10:27 am on January 23, 2025: contributor

ACK fa9aced8006bd9b10b9239b43140c37162c9d8f2

Left a few nits

tdb3 approved

tdb3 commented at 12:20 pm on January 23, 2025: contributor

re ACK fa9aced8006bd9b10b9239b43140c37162c9d8f2

fanquake merged this on Jan 23, 2025

fanquake closed this on Jan 23, 2025

maflcko deleted the branch on Jan 23, 2025

sedited referenced this in commit e1405325c9 on Feb 3, 2025

stickies-v referenced this in commit d760fd3dda on Mar 17, 2025

stickies-v referenced this in commit cc83553352 on Mar 17, 2025

stickies-v referenced this in commit 2614933f06 on Mar 17, 2025

stickies-v referenced this in commit b70418c5fc on Mar 17, 2025

stickies-v referenced this in commit 69f8a1fe50 on Mar 17, 2025

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

bitcoin locked this on Jan 23, 2026

test: Check that reindex with prune wipes blk files #31696

Code Coverage & Benchmarks

Reviews