test: check unconnecting headers don't reset the getheaders rate limit #36422

1. 
   brunoerg commented at 5:11 PM on October 2, 2026: contributor

   Since 9f66ac7cf1, only empty headers, headers that connect to the block index, or a valid low-work sync continuation count as a response to an outstanding getheaders. Part 5 of p2p_sendheaders always sends an empty headers message before each unconnecting header, so nothing checked that an unconnecting header on its own leaves m_last_getheaders_timestamp alone.

   It adds a test case that sends an unconnecting header again right after the previous getheaders and check that no new getheaders goes out. Then advance mocktime past HEADERS_RESPONSE_TIME and check that a getheaders is sent again.

   It kills the mutant https://mutanthub.space/mutants/3744.

2. test: check unconnecting headers don't reset the getheaders rate limit

   Since 9f66ac7cf1, only empty headers, headers that connect to the block
   index, or a valid low-work sync continuation count as a response to an
   outstanding getheaders. Part 5 of p2p_sendheaders always sends an empty
   headers message before each unconnecting header, so nothing checked that
   an unconnecting header on its own leaves m_last_getheaders_timestamp
   alone.
   
   Send an unconnecting header again right after the previous getheaders
   and check that no new getheaders goes out. Then advance mocktime past
   HEADERS_RESPONSE_TIME and check that a getheaders is sent again.

   a7acf277dc
3. DrahtBot added the label Tests on Oct 2, 2026
4. 
   DrahtBot commented at 5:11 PM on October 2, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--006a51241073e994b41acfe9ec718e94-->

   External sites

   • Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36422
   • Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36422.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36422#author-ralph

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline and AI policy for information on the review process.

   Type	Reviewers
   ACK	ViniciusCestarii
   Concept ACK	StephenChi-hi, Hamza1610, w0xlt

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #33959 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/33959.svg">} (test: deduplicate reorg test code by yuvicc)

   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.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

5. 
   StephenChi-hi commented at 8:54 AM on October 3, 2026: none

   ACK, this is a nice catch to prevent bugs that can allow receiving an unconnecting block header could bypass or even reset the rate-limiting HEADERS_RESPONSE_TIME timer on getheaders requests.

6. 
   ViniciusCestarii commented at 5:19 PM on October 7, 2026: contributor

   tACK a7acf277dc39011b5d06234e8d6e12d8c31c1678

   Applied the mutant and confirmed the new test case kills it

7. DrahtBot requested review from StephenChi-hi on Oct 7, 2026
8. in test/functional/p2p_sendheaders.py:587 in a7acf277dc

    582 | +        test_node.send_header_for_blocks([blocks[NUM_HEADERS]])
    583 | +        test_node.sync_with_ping()
    584 | +        with p2p_lock:
    585 | +            assert "getheaders" not in test_node.last_message
    586 | +        # Once HEADERS_RESPONSE_TIME has passed, a new getheaders can go out.
    587 | +        self.nodes[0].setmocktime(int(time.time()) + HEADERS_RESPONSE_TIME + 1)
   

   ---

   ---

   

   Hamza1610 commented at 5:21 PM on October 7, 2026:

   I would like to understand why you are using the int(time.time(). It seems like there is an existing test framework node time self.mocktime, using it makes node clock and mocktime correlate.

9. in test/functional/p2p_sendheaders.py:590 in a7acf277dc

    585 | +            assert "getheaders" not in test_node.last_message
    586 | +        # Once HEADERS_RESPONSE_TIME has passed, a new getheaders can go out.
    587 | +        self.nodes[0].setmocktime(int(time.time()) + HEADERS_RESPONSE_TIME + 1)
    588 | +        test_node.send_header_for_blocks([blocks[NUM_HEADERS]])
    589 | +        test_node.wait_for_getheaders(block_hash=expected_hash)
    590 | +        self.nodes[0].setmocktime(0)
   

   ---

   ---

   

   Hamza1610 commented at 6:25 PM on October 7, 2026:

   Resettingsetmocktime(0)can trigger peer disconnection during test shutdown. Although many test cases have been written in similar way.

10. 
    Hamza1610 commented at 6:28 PM on October 7, 2026: none

    Concept ACK

11. 
    w0xlt commented at 12:41 AM on October 8, 2026: contributor

    Concept ACK

Review Requested
StephenChi-hi