Mistake in `feature_pruning.py` functional test?

sipa commented at 12:03 pm on April 11, 2025: member

Reported on IRC yesterday:

017:24:04 < PiRK_> Good evening. I was looking through a functional test and i'm wondering if this isn't a typo: 
1                  https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_pruning.py#L44
217:24:04 < PiRK_> "nTimes" vs `nTime`
317:27:08 < sipa> PiRK_: looks like it

The code is definitely not doing what the comment says, as it’s always resetting mine_large_blocks.nTime to 0, rather than only on the first run. However, if I fix it, the test does not pass anymore.

Filing this as an issue so someone who is more familiar with the test or feels like digging in can have a look.

sipa added the label Tests on Apr 11, 2025

sipa commented at 3:58 pm on April 15, 2025: member

For anyone wanting to fix this, note that feature_pruning.py is not run by default. Please verify locally that your changes don’t break the test.

maflcko commented at 3:56 pm on April 16, 2025: member

For reference, the “failing” test that relies on the typo was added in commit 4a1975008b602aeacdad9a74d1837a7455148074. That is, the following diff passes for me:

 0diff --git a/test/functional/feature_pruning.py b/test/functional/feature_pruning.py
 1index 8d924282cf..5e8a388ec6 100755
 2--- a/test/functional/feature_pruning.py
 3+++ b/test/functional/feature_pruning.py
 4@@ -25,7 +25,6 @@ from test_framework.util import (
 5     assert_equal,
 6     assert_greater_than,
 7     assert_raises_rpc_error,
 8-    try_rpc,
 9 )
10 
11 # Rescans start at the earliest block up to 2 hours before a key timestamp, so
12@@ -41,7 +40,7 @@ def mine_large_blocks(node, n):
13     # Set the nTime if this is the first time this function has been called.
14     # A static variable ensures that time is monotonicly increasing and is therefore
15     # different for each block created => blockhash is unique.
16-    if "nTimes" not in mine_large_blocks.__dict__:
17+    if "nTime" not in mine_large_blocks.__dict__:
18         mine_large_blocks.nTime = 0
19 
20     # Get the block parameters for the first block
21@@ -480,12 +479,8 @@ class PruneTest(BitcoinTestFramework):
22         self.log.info("Test invalid pruning command line options")
23         self.test_invalid_command_line_options()
24 
25-        self.log.info("Test scanblocks can not return pruned data")
26         self.test_scanblocks_pruned()
27 
28-        self.log.info("Test pruneheight reflects the presence of block and undo data")
29-        self.test_pruneheight_undo_presence()
30-
31         self.log.info("Done")
32 
33     def test_scanblocks_pruned(self):
34@@ -499,18 +494,5 @@ class PruneTest(BitcoinTestFramework):
35         assert_raises_rpc_error(-1, "Block not available (pruned data)", node.scanblocks,
36             "start", [{"desc": f"raw({false_positive_spk.hex()})"}], 0, 0, "basic", {"filter_false_positives": True})
37 
38-    def test_pruneheight_undo_presence(self):
39-        node = self.nodes[2]
40-        pruneheight = node.getblockchaininfo()["pruneheight"]
41-        fetch_block = node.getblockhash(pruneheight - 1)
42-
43-        self.connect_nodes(1, 2)
44-        peers = node.getpeerinfo()
45-        node.getblockfrompeer(fetch_block, peers[0]["id"])
46-        self.wait_until(lambda: not try_rpc(-1, "Block not available (pruned data)", node.getblock, fetch_block), timeout=5)
47-
48-        new_pruneheight = node.getblockchaininfo()["pruneheight"]
49-        assert_equal(pruneheight, new_pruneheight)
50-
51 if __name__ == '__main__':
52     PruneTest(__file__).main()

enirox001 commented at 6:28 pm on April 17, 2025: contributor

I’ll investigate this further and determine the best way to implement a fix.

fjahr commented at 9:31 pm on April 19, 2025: contributor

I’ll investigate this further and determine the best way to implement a fix. @EniRox001 I think we spoke on IRC but you went offline and I couldn’t send you this last message. I took a brief look at the failure and it seems to me that the problem is that node 2 is being used in the reorg test previously. That’s why it’s on a different chain than the other nodes and can’t fetch the last pruned block from the other nodes because these other nodes don’t know this block. Fixing the typo surfaces this problem because previously all the blocks were deterministic so all nodes were on the same chain. There is still node 5 in the test that is also pruning and not involved in the reorg test, so the undo test should work again if you just use node 5 where node 2 is used currently.

fjahr commented at 9:33 pm on April 19, 2025: contributor

@EniRox001 If you want to you could also introduce a new node that is not involved in any other test, some might prefer this as cleaner since it avoids such issues. But that’s up to you.

bitcoin deleted a comment on Apr 19, 2025

glozow referenced this in commit bac43b957e on May 13, 2025

fanquake commented at 9:03 am on May 14, 2025: member

Closed via #32312.

fanquake closed this on May 14, 2025

Mistake in feature_pruning.py functional test? #32249

Mistake in `feature_pruning.py` functional test? #32249