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, mzumsande, pablomartin4btc

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?

theStack commented at 12:18 pm on June 16, 2025:

Tried that, but it doesn’t seem to work in the first loop iteration, as the expected getheaders message at this point is still based on the tip’s prev-header hash as of before the two new blocks have been generated.

maflcko commented at 2:37 pm on June 17, 2025:

Ugh, that seems a bit misleading of the test. Obviously no need to change here (seems pre-existing and unrelated), but it could make sense to just properly check the getheaders message at the time when it is sent, and not when the loop has started or advanced to the next iteration?

theStack commented at 4:47 pm on June 17, 2025:

That seems to be better, yeah. Btw, another issue I spotted is that the wait_for_getheaders call before the loop passes even with the following patch, even if it shouldn’t:

 0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index c4fe412fb8..68a53f8f75 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -5120,9 +5120,9 @@ void PeerManagerImpl::ConsiderEviction(CNode& pto, Peer& peer, std::chrono::seco
 5                 // because it'll either go out or be skipped because of a
 6                 // getheaders in-flight already, in which case the peer should
 7                 // still respond to us with a sufficiently high work chain tip.
 8-                MaybeSendGetHeaders(pto,
 9-                        GetLocator(state.m_chain_sync.m_work_header->pprev),
10-                        peer);
11+                //MaybeSendGetHeaders(pto,
12+                //        GetLocator(state.m_chain_sync.m_work_header->pprev),
13+                //        peer);
14                 LogDebug(BCLog::NET, "sending getheaders to outbound peer=%d to verify chain work (current best known block:%s, benchmark blockhash: %s)\n", pto.GetId(), state.pindexBestKnownBlock != nullptr ? state.pindexBestKnownBlock->GetBlockHash().ToString() : "<none>", state.m_chain_sync.m_work_header->GetBlockHash().ToString());
15                 state.m_chain_sync.m_sent_getheaders = true;
16                 // Bump the timeout to allow a response, which could clear the timeout

This is due to the initial getheaders message sent after peer connection, still being in the last_message dict. So this would be another point to be put in a PR that improves the structure of this (sub-)test in general.

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.

maflcko added this to the milestone 30.0 on Jun 17, 2025

fanquake requested review from mzumsande on Jun 24, 2025

mzumsande commented at 4:35 pm on June 25, 2025: contributor

Code Review ACK dd8447f70faf6419b4617da3c1b57098e9cd66a6

I also think that this part of the test would benefit from additional cleanups / more comments, it’s pretty hard to understand what’s going on.

pablomartin4btc commented at 9:55 pm on June 25, 2025: member

cr-ACK dd8447f70faf6419b4617da3c1b57098e9cd66a6

As I verified, tip_header was never updated since the top of the test and prev_prev_hash had the wrong value assigned (tip_header.hash which was None), but the test was passing due to wait_for_getheaders when the block_hash is None (same behaviour as if nothing is passed to the function), checks for last message of getheaders type received which existed from before entering the cycle.

An observation as why tip_header.hash is None, when the CBlockHeader is being created from node.getblockheader(), CBlockHeader’s constructor (with header=None as in this case) and later the deserialize(), both set the hash property to None. not sure if this is an issue but tip_header.hash (after this change) will be only used in this file later at test_outbound_eviction_blocks_relay_only() following the same patter as above, calling wait_for_getheaders with block_hash=tip_header.hash being tip_header.hash=None, should that be fixed too?

glozow merged this on Jun 25, 2025

glozow closed this on Jun 25, 2025

theStack deleted the branch on Jun 25, 2025

fanquake referenced this in commit 5987c1b6ab on Jun 26, 2025

fanquake commented at 10:26 am on June 26, 2025: member

Backported to 29.x in #32810.

theStack commented at 7:44 pm on June 26, 2025: contributor

@pablomartin4btc:

not sure if this is an issue but tip_header.hash (after this change) will be only used in this file later at test_outbound_eviction_blocks_relay_only() following the same patter as above, calling wait_for_getheaders with block_hash=tip_header.hash being tip_header.hash=None, should that be fixed too?

Good catch, just verified that this is indeed another instance where we pass None unintentionally, accepting any GETHEADERS message instead of the one with the tip header hash (my bad, should have caught and fixed this in the same PR). A follow-up PR would be appreciated, feel free to open one.

danielabrozzoni commented at 8:58 pm on June 26, 2025: contributor

post merge ACK dd8447f70faf6419b4617da3c1b57098e9cd66a6

Everything looks good, I started reviewing without noticing this was merged already :)

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

Code Coverage & Benchmarks

Reviews

Conflicts