highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-07-06 16:36:57 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#32742: test: fix catchup loop in outbound eviction functional test

Browse Source 
dd8447f70f test: fix catchup loop in outbound eviction functional test (Sebastian Falbesoner)

Pull request description:

  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](19765dca19/test/functional/p2p_outbound_eviction.py (L86-L103)) currently has a small flaw: the contained waiting for a `getheaders` message

  19765dca19/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.

  ```diff
  diff --git a/test/functional/p2p_outbound_eviction.py b/test/functional/p2p_outbound_eviction.py
  index 30ac85e32f..9886a49512 100755
  --- a/test/functional/p2p_outbound_eviction.py
  +++ b/test/functional/p2p_outbound_eviction.py
  @@ -85,6 +85,7 @@ class P2POutEvict(BitcoinTestFramework):

           self.log.info("Keep catching up with the old tip and check that we are not evicted")
           for i in range(10):
  +            print(f"i={i}, prev_prev_hash={prev_prev_hash}")
               # Generate an additional block so the peers is 2 blocks behind
               prev_header = from_hex(CBlockHeader(), node.getblockheader(best_block_hash, False))
               best_block_hash = self.generateblock(node, output="raw(42)", transactions=[])["hash"]
  ```

  master branch
  ```
  ...
  i=0, prev_prev_hash=21722572577213525620063947414919931742473663114977483853465070858884938201585
  i=1, prev_prev_hash=None
  i=2, prev_prev_hash=None
  i=3, prev_prev_hash=None
  i=4, prev_prev_hash=None
  i=5, prev_prev_hash=None
  i=6, prev_prev_hash=None
  i=7, prev_prev_hash=None
  i=8, prev_prev_hash=None
  i=9, prev_prev_hash=None
  ...
  ```

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

  [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

ACKs for top commit:
  maflcko:
    lgtm ACK dd8447f70f
  mzumsande:
    Code Review ACK dd8447f70f
  pablomartin4btc:
    cr-ACK dd8447f70f

Tree-SHA512: bd8e786b52e3e96661453006140d6b8fad5a35f1c8d38243c61df52b19c97cd3800404745a2f9603bcdf0006e9780b4f15f8f7e4fa34ff07d52dba04d87b68d0
This commit is contained in:
merge-script
2025-06-25 18:13:33 -04:00
parent f27898c8bf dd8447f70f
commit e5f9218b6a
1 changed files with 2 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
test/functional/p2p_outbound_eviction.py
View File

@ -88,6 +88,7 @@ class P2POutEvict(BitcoinTestFramework):
# Generate an additional block so the peers is 2 blocks behind # Generate an additional block so the peers is 2 blocks behind
prev_header = from_hex(CBlockHeader(), node.getblockheader(best_block_hash, False)) prev_header = from_hex(CBlockHeader(), node.getblockheader(best_block_hash, False))
best_block_hash = self.generateblock(node, output="raw(42)", transactions=[])["hash"] best_block_hash = self.generateblock(node, output="raw(42)", transactions=[])["hash"]
tip_header = from_hex(CBlockHeader(), node.getblockheader(best_block_hash, False))
peer.sync_with_ping() peer.sync_with_ping()
# Advance time but not enough to evict the peer # Advance time but not enough to evict the peer
@ -100,7 +101,7 @@ class P2POutEvict(BitcoinTestFramework):
# Send a header with the previous tip (so we go back to 1 block behind) # Send a header with the previous tip (so we go back to 1 block behind)
peer.send_and_ping(msg_headers([prev_header])) peer.send_and_ping(msg_headers([prev_header]))
prev_prev_hash = tip_header.hash prev_prev_hash = tip_header.hashPrevBlock
self.log.info("Create an outbound connection and take some time to catch up, but do it in time") self.log.info("Create an outbound connection and take some time to catch up, but do it in time")
# Check that if the peer manages to catch up within time, the timeouts are removed (and the peer is not disconnected) # Check that if the peer manages to catch up within time, the timeouts are removed (and the peer is not disconnected)