#36470 test: make p2p_eviction actually exercise block-time protection

ReviewabilityReviewsAgreementSize
#36470 brunoerg test: make p2p_eviction actually exercise block-time protectionReady1 (+1)Strong0 + 57 tests
  • Ensure the peer eviction functional test actually exercises block-time protection for block-relaying peers
  • Catch regressions that currently pass unnoticed because connection ordering masked broken eviction logic
  • Ready to review
  • All objections and reviewer suggestions have been addressed
  • Substantive support with no open objections
  • Verified with mutation testing that disabled block-time protection now fails as expected (trakshan-mishra)
  • Supported because using mocktime makes the test deterministic and twice as fast (maflcko)
  • 0 lines added or modified outside tests
  • 57 lines added or modified in tests
  • 43 lines removed in total
  • 1 files, 1 commits
P2P#59 of 71
P3 · test coverage
  • P3 because it fixes a test gap where inbound peer eviction logic for block relayers was completely bypassed
  • Validates eviction protection without modifying production peer-to-peer code
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Restructures peer connection order and ping timings in the `p2p_eviction.py` functional test so that peer eviction protection for block-relaying peers is actually exercised. Eviction candidates are connected first and assigned lower ping times than block and transaction relayers using mocktime-controlled pong responses.

Problem: `p2p_eviction.py` passes even if block-time eviction protection is broken or completely disabled. Because block-sending peers were connected first, fallback logic in `CompareNodeBlockTime` protected them as the oldest inbound connections regardless of whether they sent blocks.

Evidence of importance:

  • PR description: "This can be shown by deleting the line where the peers send their block (L71 on master): block_peer.send_blocks_and_test([block], node, success=True)" and the test continues to pass.

Risk of a mistake: low (tests). A mistake can only cause functional test failures or CI flakiness.

Changed since the previous assessment: No structural changes; the implementation maintains the approach of connecting eviction candidates first with mocktime-based ping delays.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • nothing: all objections and suggestions addressed; PR has ACKs and no pending requests

Agreement

Strong

substantive support, no open objection (trakshan-mishra, maflcko)

Thread notes: Author responded to maflcko's suggestion about peer connection ordering and mutation testing, opting to keep the current implementation as-is unless other reviewers ask for it.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
instagibbs (member)objection, correctnessthe regression test does not catch mutants under libc++ or MSVC due to sort implementation details when peers share a netgroupfixed · fix pushed · author force-pushed addressing the issue for libc++/MSVCchangedyes2026-10-13: 'regression test doesn't fail under mutants for libc++/MSVC due to sort implementation details with all peers falling under the same netgroup'
Status from: 2026-10-14: brunoerg: 'Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/36470#discussion_r4263718474 to cover the libc++/MSVC ca'
maflcko (member)objection, correctnessmanual sleeps may cause test unreliability and failures in slow or racy test environments such as QEMU VMs or Bedrockaccepted · fix pushed · maflcko confirmed the mocktime fix makes the test deterministic and twice as fastansweredyes2026-10-16: 'not sure this is reliable. the tests run also in qemu in vms, which may be slow or racy. Also, if someone runs them in bedrock or similar, this may not hold, so the test may fail?'
Status from: 2026-10-20: maflcko: 'The test should now be deterministic and twice as fast with mocktime.'
maflcko (member)suggestion, approachconnect candidate peers first, then shuffle protected peer types to satisfy mutation testinganswered · author declined to change connection order for now unless other reviewers request itansweredyes2026-10-20: 'Ralph complains, but to make it happy, one would have to first connect candidates peers, then connect shuffle(tx, block, fast-ping)? Not sure if this is worth it or if it makes Ralph happy. Up to you.'
Status from: 2026-10-26: brunoerg: 'I'm not sure is worth, will leave as-is for now unless other reviewers prefer that.'

Support:

  • trakshan-mishra (none) (Tested ACK, 2026-10-09) [tested]: verified through mutation testing that the test fails under this PR when block-time protection logic is removed or bypassed, unlike on master where it passes unconditionally
  • maflcko (member) (ACK, 2026-10-12): review ACK c75cdce1f9430cab029a329fd86c7e6bef7bf444 [not substantive]
  • maflcko (member) (ACK, 2026-10-20) [read]: the test should now be deterministic and twice as fast with mocktime

Participants:

  • trakshan-mishra (none, support, 1 statements 2026-10-09 to 2026-10-09): Tested ACK verifying that the test catches disabled block-time protection via mutation testing
  • maflcko (member, support, 8 statements 2026-10-12 to 2026-10-20): review ACKed after mocktime resolved flakiness concerns; noted optional connection ordering suggestion for mutation testing
  • instagibbs (member, objection, 1 statements 2026-10-13 to 2026-10-13): reported that the test does not catch mutants on libc++/MSVC due to sort implementation details
  • Crypt-iQ (member, neutral, 1 statements 2026-10-16 to 2026-10-16): noted that a change fixes a scheduling flake in Bedrock

Files

100 lines under test/bench/ci.

  • test/functional/p2p_eviction.py +57/-43

Card

The short text the ranking pass compares across PRs.

Fixes a test flaw in p2p_eviction.py where the test passed even when block-time eviction protection was disabled or bypassed. Because block-relaying peers were connected first, fallback logic in CompareNodeBlockTime protected them as the oldest connections anyway. The test now connects eviction candidate peers first with lower mocktime ping responses so block and transaction protection steps are genuinely required. Test-only change.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-27T01:27, confidence high, input hash 969fa12720d8a825
thread read through 2026-10-26 (20 statements), head 2fd9be21e2
labels: Tests

Categories

P2P

P3 · test coverage

Fixes a test gap where inbound peer eviction logic for block relayers was completely bypassed due to connection order fallback. As verified by mutation testing, master passed even when block-time protection was removed, which this PR corrects without altering production p2p code.

Membership: Modifies test/functional/p2p_eviction.py to properly test connection management and inbound peer eviction protection.

Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 0