test: Avoid intermittent error in assert_equal(pruneheight

maflcko commented at 3:57 pm on December 11, 2024: member

The test uses the P2P network to sync blocks, which has no inherent guarantee that the blocks are sent and received in the right order, assuming the headers are received first.

This can mean that the first block file is flushed with block at height 249 and block at height 248 is added to the second file. In the log it looks like: Leaving block file 0: CBlockFileInfo(blocks=249, size=65319, heights=0...249, time=2011-02-02...2024-12-03) (onto 1) (height 248). The test assumes that the height of the last pruned block in the first file is 248, expecting it to look like: Leaving block file 0: CBlockFileInfo(blocks=249, size=65319, heights=0...248, time=2011-02-02...2024-12-09) (onto 1) (height 249) .

Fix the issue by using a linear dumb sync.

DrahtBot commented at 3:57 pm on December 11, 2024: 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/31468.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande, i-am-yuvi, fjahr, achow101

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 Dec 11, 2024

in test/functional/feature_index_prune.py:32 in faf62ad6ec outdated

28+
29+    def linear_sync(self, node_from, *, height_from=None):
30+        # Linear sync over RPC, because P2P sync may not be linear
31+        to_height = node_from.getblockcount()
32+        for n in self.nodes:
33+            for i in range(height_from or n.getblockcount(), to_height + 1):

mzumsande commented at 6:16 pm on December 12, 2024:

could start at n.getblockcount() + 1, nodes will already have the block at n.getblockcount().

maflcko commented at 10:07 am on December 13, 2024:

Thx, done. Also made the test about as fast as before.

mzumsande commented at 6:29 pm on December 12, 2024: contributor

Code Review ACK faf62ad6ecc8905db75a1e772d2b033081058f1b

This makes the test slower (by a factor ~2), but it’s already in the list of extended tests anyway, so probably not a big issue.

test: Avoid intermittent error in assert_equal(pruneheight_new, 248) fa0998f0a0

maflcko force-pushed on Dec 13, 2024

mzumsande commented at 6:34 pm on December 17, 2024: contributor

Code Review ACK fa0998f0a028161fe7264d0e81af36ffdcb43384

i-am-yuvi approved

i-am-yuvi commented at 7:55 am on December 24, 2024: none

Code Review ACK fa0998f0a028161fe7264d0e81af36ffdcb43384

I was thinking of using a batch syncing process instead of one-by-one to be memory efficient, but it ended up being a lot slower. Anyway, nice work, @maflcko.

Here are the test results:

0Temporary test directory at /var/folders/jb/wlbrz0t95vl58wzxjt75wqmh0000gn/T/test_runner_₿_🏃_20241224_132141
1Remaining jobs: [feature_index_prune.py]
21/1 - feature_index_prune.py passed, Duration: 34 s                 
3
4TEST                   | STATUS    | DURATION
5
6feature_index_prune.py | ✓ Passed  | 34 s
7
8ALL                    | ✓ Passed  | 34 s (accumulated) 
9Runtime: 34 s

maflcko commented at 10:26 am on December 24, 2024: member

I was thinking of using a batch syncing process instead of one-by-one to be memory efficient, but it ended up being a lot slower. Anyway, nice work, @maflcko.

For fun, I had written another async alternative that reads the next block while the current block is submitted, which was faster than this pull by a few seconds, but I think trivial code is more important in tests than the fastest solution.

i-am-yuvi commented at 1:03 pm on December 24, 2024: none

I was thinking of using a batch syncing process instead of one-by-one to be memory efficient, but it ended up being a lot slower. Anyway, nice work, @maflcko.

For fun, I had written another async alternative that reads the next block while the current block is submitted, which was faster than this pull by a few seconds, but I think trivial code is more important in tests than the fastest solution.

Interesting!! I Would love to see that as well. Can you share that!!?

maflcko commented at 1:09 pm on December 24, 2024: member

0         with concurrent.futures.ThreadPoolExecutor(max_workers=self.num_nodes) as rpc_threads:
1-            for i in range(height_from, to_height + 1):
2-                b = node_from.getblock(blockhash=node_from.getblockhash(i), verbosity=0)
3-                list(rpc_threads.map(lambda n: n.submitblock(b), self.nodes))
4+         fut = rpc_threads.submit(lambda:node_from.getblock(blockhash=node_from.getblockhash(height_from), verbosity=0))
5+         for i in range(height_from , to_height + 1):
6+            b = fut.result()
7+            it = rpc_threads.map(lambda n: n.submitblock(b), [n for n in self.nodes if n is not node_from])
8+            fut = rpc_threads.submit(lambda:node_from.getblock(blockhash=node_from.getblockhash(i+1), verbosity=0))
9+            list(it)

in test/functional/feature_index_prune.py:30 in fa0998f0a0

26 
27+    def setup_network(self):
28+        self.setup_nodes()  # No P2P connection, so that linear_sync works
29+
30+    def linear_sync(self, node_from, *, height_from=None):
31+        # Linear sync over RPC, because P2P sync may not be linear

fjahr commented at 4:07 pm on December 30, 2024:

I think a little more detail would be helpful since it may not be obvious what the issue is.

0# Linear sync over RPC, because P2P sync may lead to blocks being received and saved to disk out of order which means prune heights would non-deterministic.

fjahr commented at 4:11 pm on December 30, 2024: contributor

Code review ACK fa0998f0a028161fe7264d0e81af36ffdcb43384

I think adding detail to the comment would be good but it’s not a blocker.

achow101 commented at 7:11 pm on December 30, 2024: member

ACK fa0998f0a028161fe7264d0e81af36ffdcb43384

achow101 merged this on Dec 30, 2024

achow101 closed this on Dec 30, 2024

maflcko deleted the branch on Jan 2, 2025

test: Avoid intermittent error in assert_equal(pruneheight_new, 248) #31468

Code Coverage & Benchmarks

Reviews