test: fix catchup loop in outbound eviction functional test

theStack commented at 0:07 am on June 13, 2025: contributor

In the course of working on an equivalent of #32421 for the CBlockHeader class, I noticed that the catchup loop in the outbound eviction functional test currently has a small flaw: the contained waiting for a getheaders message

https://github.com/bitcoin/bitcoin/blob/19765dca197a6334e5ad3985a30c7836374d5c26/test/functional/p2p_outbound_eviction.py#L98-L99

only waits for any such message instead of one with the intended block hash after the first iteration. The reason is that the prev_prev_hash variable is set incorrectly, since the tip_header instance is not updated and its field .hash is None [1]. Fix that by updating tip_header after generating a new block and also use the correct field on it – we want the tip header’s previous hash (.hashPrevBlock), which will be the previous-previous hash in the next iteration as intended.

Can be demonstrated by adding a debug output for prev_prev_hash, e.g.

 0diff --git a/test/functional/p2p_outbound_eviction.py b/test/functional/p2p_outbound_eviction.py
 1index 30ac85e32f..9886a49512 100755
 2--- a/test/functional/p2p_outbound_eviction.py
 3+++ b/test/functional/p2p_outbound_eviction.py
 4@@ -85,6 +85,7 @@ class P2POutEvict(BitcoinTestFramework):
 5
 6         self.log.info("Keep catching up with the old tip and check that we are not evicted")
 7         for i in range(10):
 8+            print(f"i={i}, prev_prev_hash={prev_prev_hash}")
 9             # Generate an additional block so the peers is 2 blocks behind
10             prev_header = from_hex(CBlockHeader(), node.getblockheader(best_block_hash, False))
11             best_block_hash = self.generateblock(node, output="raw(42)", transactions=[])["hash"]

master branch

 0...
 1i=0, prev_prev_hash=21722572577213525620063947414919931742473663114977483853465070858884938201585
 2i=1, prev_prev_hash=None
 3i=2, prev_prev_hash=None
 4i=3, prev_prev_hash=None
 5i=4, prev_prev_hash=None
 6i=5, prev_prev_hash=None
 7i=6, prev_prev_hash=None
 8i=7, prev_prev_hash=None
 9i=8, prev_prev_hash=None
10i=9, prev_prev_hash=None
11...

PR branch

 0...
 1i=0, prev_prev_hash=21722572577213525620063947414919931742473663114977483853465070858884938201585
 2i=1, prev_prev_hash=23204083306104595181276643925327085197417756603258684897360269464191973063397
 3i=2, prev_prev_hash=18117007775254206852722585270408843074799046031613422902091537272077477361634
 4i=3, prev_prev_hash=30556804635951812756130312631227721973553160707632138130845362630877961299882
 5i=4, prev_prev_hash=16476515948153779819467376247405243058769281687868039119037064816106574626111
 6i=5, prev_prev_hash=14965506521435221774966695805624206855826023174786191695076697927307467053159
 7i=6, prev_prev_hash=14510815979277079515923749862202324542606166669768865640616202929053689167149
 8i=7, prev_prev_hash=15360268707191667685151951417759114642582372006627142890517655217275478262166
 9i=8, prev_prev_hash=55984929479619644661389829786223559362979512070332438490054115824374865094074
10i=9, prev_prev_hash=6591573629906616262191232272909118561529534571119028248829355592878183757083
11...

[1] that’s in my opinion another example how caching hashes is confusing and easy to be misused; it’s better to remove it and just compute the hash on-the-fly, so returning None is not even possible anymore

test: fix catchup loop in outbound eviction functional test

The catchup loop in the outbound eviction functional test currently has
a small flaw, as the contained waiting for a `getheaders` message just
waits for any such message instead of one with the intended block hash.
The reason is that the `prev_prev_hash` variable is set incorrectly,
since the `tip_header` instance is not updated and its field `.hash` is
None. Fix that by updating `tip_header` and use the correct field -- we
want the tip header's previous hash (`.hashPrevBlock`).

dd8447f70f

DrahtBot commented at 0:07 am on June 13, 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/32742.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32468 (rpc: generateblock to allow multiple outputs by polespinasa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Jun 13, 2025

in test/functional/p2p_outbound_eviction.py:101 in dd8447f70f

100@@ -100,7 +101,7 @@ def test_outbound_eviction_unprotected(self):
101

maflcko commented at 8:11 am on June 13, 2025:

wouldn’t it be better to remove the symbol and just inline prev_header.hashPrevBlock one line above?

maflcko commented at 8:14 am on June 13, 2025: member

lgtm ACK dd8447f70faf6419b4617da3c1b57098e9cd66a6

the hash field is wrong either way, simply due to being the wrong type (it is an optional hex string), as opposed to an optional int.

test: fix catchup loop in outbound eviction functional test #32742

Code Coverage & Benchmarks

Reviews

Conflicts