#33959 test: deduplicate reorg test code
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #33959 test: deduplicate reorg test code | Ready | 0 | Crickets | 0 + 104 tests | |||
|
|
|
|
| |||
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
Centralizes the `trigger_reorg` helper method into `test_framework/blocktools.py` and removes duplicated copies across functional tests. Converts remaining tests that simulated reorgs using `invalidateblock` to realistic fork-based reorgs using `create_empty_fork` and `trigger_reorg`.
Problem: Multiple functional tests independently reimplemented `trigger_reorg()`, and several tests simulated reorgs by calling the `invalidateblock` RPC, which bypasses the standard code path for competing chains arriving.
Evidence of importance:
- PR description: 'trigger_reorg() was copy-pasted across several functional tests. This moves it into blocktools.py... The remaining tests that still simulated reorgs with the invalidateblock RPC are converted to use them, since invalidateblock takes a different codepath than a reorg caused by a competing chain arriving.'
Risk of a mistake: low (tests). Only modifies functional test framework helpers and test execution flows; does not affect production node code.
Changed since the previous assessment: Clarified in test/functional/rpc_gettxspendingprevout.py why one instance of invalidateblock remains intentionally in place to test index rewinding when disconnecting without connecting a new block.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: review of the rebased PR 2026-10-28
Agreement
Crickets
no substantive comment either way
Thread notes: The author rebased onto master following sedited's rebase request.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| instagibbs (member) | suggestion, approach | drop the added abstraction beyond centralizing trigger_reorg | fixed · fix pushed · author removed Forkgenerator class and kept only trigger_reorg in blocktools | answered | yes | 2026-07-24: 'I think this might be a bit of overengineering. Centralizing code is nice, but really I don't see the value of the added abstraction aside from centralizing trigger_reorg?' Status from: 2026-07-27: yuvicc: 'Addressed @instagibbs suggestion to only keep `trigger_reorg` method in blocktools.py.' |
| instagibbs (member) | suggestion, maintenance | remove or update stale comment containing 'invalid' | open | answered | no | 2026-07-27: 'stale comment with "invalid"' |
| instagibbs (member) | suggestion, approach | prep the fork once and use it to trigger reorg on both sides if nodes are connected | answered · author confirmed the nodes are connected and agreed to make the change | answered | yes | 2026-07-27: 'are these nodes disconnected? I think it can be prepped once and used to trigger reorg on both sides?' Status from: 2026-07-28: yuvicc: 'Correct, they are connected. Will do the changes.' |
| instagibbs (member) | suggestion, maintenance | remove the unnecessary [0] subscript | open | answered | no | 2026-07-27: 'no need for [0] subscript anymore' |
| instagibbs (member) | suggestion, maintenance | preserve the removed comment explaining why not to use invalidateblock somewhere to prevent reintroducing the pattern | answered · author suggested adding it to create_empty_fork docstring | answered | yes | 2026-07-27: 'wondering if there's a good place to add this removed comment to have people not reintroduce this pattern' Status from: 2026-07-28: yuvicc: 'Maybe we can add in `create_empty_fork` docstring?' |
Participants:
- maflcko (member, neutral, 1 statements 2026-05-21 to 2026-05-21): asked if the PR was still being worked on
- instagibbs (member, neutral, 6 statements 2026-07-24 to 2026-07-27): suggested dropping extra abstraction and provided inline review comments
- sedited (member, neutral, 2 statements 2026-07-24 to 2026-10-26): asked instagibbs to review and requested a rebase
Files
194 lines under test/bench/ci.
- test/functional/mempool_ephemeral_dust.py +7/-12
- test/functional/mempool_updatefromblock.py +7/-12
- test/functional/rpc_gettxspendingprevout.py +14/-5
- test/functional/mempool_packages.py +7/-10
- test/functional/test_framework/blocktools.py +16/-1
- test/functional/mempool_resurrect.py +6/-9
- test/functional/p2p_orphan_handling.py +10/-5
- test/functional/wallet_balance.py +11/-4
- test/functional/mempool_truc.py +5/-9
- test/functional/p2p_sendheaders.py +7/-7
- test/functional/wallet_rescan_unconfirmed.py +10/-2
- test/functional/mempool_reorg.py +2/-7
- test/functional/wallet_v3_txs.py +2/-7
Card
The short text the ranking pass compares across PRs.
Centralizes the `trigger_reorg` helper in `test_framework/blocktools.py` and removes duplicated implementations across functional tests. Replaces instances of the `invalidateblock` RPC in wallet, P2P, and RPC tests with fork-based reorganizations using `create_empty_fork` and `trigger_reorg`. This ensures functional tests exercise the actual chain-switching path triggered by competing chains rather than manual invalidation.
Data
thread read through 2026-10-28 (16 statements), head ee536ffe36
labels: Tests
Categories
Test infrastructure
P3 · cleanup (ranking pass: Moved down: de-duplication plus more realistic reorg paths is valuable but ranks below coverage of high-risk code and framework capability.)
Deduplicates copy-pasted reorg helper code into the shared test framework and improves test realism. As noted in the description, several functional tests previously copy-pasted `trigger_reorg()` or used `invalidateblock`, which 'takes a different codepath than a reorg caused by a competing chain arriving'.
Membership: Has the Tests label and adds a centralized `trigger_reorg` helper to `test_framework/blocktools.py`.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1