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")

TheCharlatan 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

TheCharlatan approved

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

TheCharlatan 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

TheCharlatan approved

TheCharlatan 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

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

Code Coverage & Benchmarks

Reviews