test: Fix feature_pruning test after nTime typo fix

enirox001 commented at 10:41 am on April 20, 2025: contributor

This PR contains two commits:

Fixes a typo in feature_pruning.py where ’nTimes’ was incorrectly used instead of ’nTime’. This typo caused the test to always reset mine_large_blocks.nTime to 0, rather than only on the first run.
Fixes the test failure exposed by the typo fix. The test_pruneheight_undo_presence test was failing because it was using node 2, which is involved in reorg testing and could be on a different chain than other nodes. The solution switches to using node 5, which is also a pruned node but isn’t involved in reorg testing.

Testing:

Ran test/functional/feature_pruning.py multiple times to verify consistent passing
Verified that the test now passes with the correct nTime variable name
Confirmed the test behavior matches the intended functionality of verifying pruned block availability
Ran the full test suite to ensure the changes did not introduce any regressions or affect other tests

Thanks to fjahr for his assistance in diagnosing the issue and suggesting the solution.

This fixes the test failure reported in #32249

DrahtBot commented at 10:41 am on April 20, 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/32312.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr, naiyoma, maflcko, stratospher

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

DrahtBot added the label Tests on Apr 20, 2025

fjahr commented at 7:07 pm on April 20, 2025: contributor

Code change looks good to me. The commit message doesn’t seem to say anything about the typo fix though. It should be mentioned there and that the rest of the change is fixing a secondary issue caused by this. You could also split this in two separate commits.

test: Fix nTimes typo in feature_pruning test

Fix incorrect variable name in comment (nTimes -> nTime) in
feature_pruning.py. This typo caused the test to always reset
mine_large_blocks.nTime to 0, rather than only on the first run
as intended.

772ba7f9ce

test: Use uninvolved pruned node in feature_pruning undo test

After fixing the nTime variable name, the test_pruneheight_undo_presence
test began failing because node 2, which is involved in reorg testing,
could be on a different chain than other nodes. This caused failures
when trying to fetch blocks from other nodes that didn't recognize
node 2's chain.

Switch to using node 5 instead, which is also a pruned node but isn't
involved in reorg testing, ensuring it stays on the same chain as the
other nodes. This allows the block fetching to work as intended in the
test.

2aa63d511a

enirox001 force-pushed on Apr 21, 2025

enirox001 commented at 3:07 pm on April 21, 2025: contributor

Code change looks good to me. The commit message doesn’t seem to say anything about the typo fix though. It should be mentioned there and that the rest of the change is fixing a secondary issue caused by this. You could also split this in two separate commits.

Thanks for the review! I’ve split the changes into two logical commits as suggested:

First commit fixes the nTimes -> nTime typo and explains its impact on the test behavior
Second commit addresses the test failure that was exposed by the typo fix

I’ve also updated the PR description to clearly explain both changes and their relationship. Please let me know if you’d like me to make any further adjustments to the commit messages or description.

fjahr commented at 8:55 pm on April 21, 2025: contributor

tACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c

Just a note: If this test was run by CI we would need to flip the order of the commits because the first commit would fail without the second but the second would succeed without the first. There is a CI job that runs every commit individually. But since this test isn’t executed by CI it doesn’t matter here.

naiyoma commented at 6:02 pm on May 13, 2025: contributor

tACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c I don’t think this is a reorg issue. I believe the test fails because node 1 invalidates blocks in reorg_test -> https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_pruning.py#L179. So basically, when node 2 tries to getblockfrompeer from node 1, node 1 has already invalidated the block we’re trying to fetch. Node 2 and Node 5 are on the same chain tip even after Node 2 has been reorged. The test passes based on whether or not the block request by node 5 or node 2 is invalid or valid on Node 1. I tested by using other blocks and not necessarily changing the node that is requesting blocks from node1. For example,

0fetch_block = node.getblockhash(773)

maflcko commented at 6:08 pm on May 13, 2025: member

lgtm ACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c

stratospher commented at 6:19 pm on May 13, 2025: contributor

tested ACK 2aa63d5. verified that nTime is being incremented now.

agree with @naiyoma - this appears a “block being requested is valid/invalid in node1 problem” and not a “which node to connect to problem”.

node2 and node5 have identical chain tips (aside from differing prune heights: node2 at 1246, node5 at 774).
on master: node2 requests block 1245 from node1. but in node1, that block is marked BLOCK_FAILED_VALID, so the request is ignored. I see this log in node1: ignoring request from peer=18 for old block that isn't in the main chain
on the PR: node5 requests block 773 instead, which is valid - so node1 serves it successfully. (node2 requesting block 773 (and not 1245) would pass successfully too!)

1 thing that made this test confusing to debug is how leaky it is - a lot of context bleeds over from previous test logic. We’re kind of doing this again since we’re breaking this initial assumption now - # Create node 5 to test wallet in prune mode, but do not connect

and this might cause confusion for new tests in the future.

my personal preference would be to make node2 request block 773 (or some other valid block) from node1. but yes, this works :)

fanquake added the label Needs backport (29.x) on May 13, 2025

glozow merged this on May 13, 2025

glozow closed this on May 13, 2025

fanquake referenced this in commit edd4073d70 on May 14, 2025

fanquake referenced this in commit 6c4e3de2ac on May 14, 2025

fanquake removed the label Needs backport (29.x) on May 14, 2025

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

Backported to 29.x in #32292.

fjahr commented at 9:59 am on May 14, 2025: contributor

I tested by using other blocks and not necessarily changing the node that is requesting blocks from node1.

my personal preference would be to make node2 request block 773 (or some other valid block) from node1. but yes, this works :)

I don’t think this is a good idea because it’s not testing the same behavior that the test currently does. The test intends to check that the prune height doesn’t change when the block data is available, but not the undo data of these blocks. To do this, the test has to fetch the last pruned block, causing it to have the block data but not the pruned data for that block that could potentially change the prune height shown by the RPCs. Before #29668 (which also added the test in question) the test would have failed because the prune height would have changed since it only checked for block data. If you fetch any older block instead, the prune height would not change either way because there are other blocks between the first unpruned block and the fetched block for which the node doesn’t have any data. This more general test for interactions between pruning and getblockfrompeer should be covered in rpc_getblockfrompeer.py as well.

Happy to review if someone wants to add documentation that makes the intention of the test more clear. And if you want to reduce leakiness I would suggest adding a separate node just for this test case, as mentioned here: #32249 (comment)

test: Fix feature_pruning test after nTime typo fix #32312

Code Coverage & Benchmarks

Reviews