test: deduplicate reorg test code #33959

 207 | @@ -208,17 +208,17 @@ def mine_reorg(self, length):
 208 |          to-be-reorged-out blocks are mined, so that we don't break later tests.
 209 |          return the list of block hashes newly mined."""
 210 |  
 211 | +        # Prep for fork with empty blocks
 212 | +        fork_blocks = create_empty_fork(self.nodes[1], fork_length=length + 1)
 213 | +
 214 |          # make sure all invalidated blocks are node0's

instagibbs commented at 1:39 PM on July 27, 2026:

stale comment with "invalid"

 240 | @@ -237,13 +241,17 @@ def test_balances(*, fee_node_1=0):
 241 |          self.sync_all()
 242 |          self.nodes[1].sendrawtransaction(hexstring=tx_replace, maxfeerate=0)
 243 |  
 244 | +        # Prep for fork
 245 | +        fork_blocks0 = create_empty_fork(self.nodes[0])

instagibbs commented at 1:41 PM on July 27, 2026:

are these nodes disconnected? I think it can be prepped once and used to trigger reorg on both sides?

yuvicc commented at 4:30 AM on July 28, 2026:

Correct, they are connected. Will do the changes.

 188 | @@ -184,7 +189,10 @@ def create_tx(**kwargs):
 189 |          result = node0.gettxspendingprevout([prevout(tx1['txid'], vout=0)], return_spending_tx=True)
 190 |          assert_equal(result, [spent_out_in_block(tx1['txid'], vout=0, spending_tx_id=tx2["txid"], blockhash=blockhash, spending_tx=tx2['hex'])])
 191 |  
 192 | -        # replace tx1 with tx3
 193 | +        # replace tx1 with tx3. We use invalidateblock (rather than a fork-based

instagibbs commented at 1:43 PM on July 27, 2026:

suggestion for wording:

      # Disconnect tx1's block so tx1 can be replaced by tx3 below, which also
      # evicts tx1's child tx2. Unlike the case above, use invalidateblock rather
      # than a fork-based reorg: the index is only rewound when a block connects
      # so disconnecting without connecting anything lets us check below that tx2 is still indexed until the next block.

 245 | +        fork_blocks0 = create_empty_fork(self.nodes[0])
 246 | +        fork_blocks1 = create_empty_fork(self.nodes[1])
 247 | +
 248 |          # Now confirm tx_replace
 249 | -        block_reorg = self.generatetoaddress(self.nodes[1], 1, ADDRESS_WATCHONLY)[0]
 250 | +        self.generatetoaddress(self.nodes[1], 1, ADDRESS_WATCHONLY)[0]

instagibbs commented at 1:51 PM on July 27, 2026:

no need for [0] subscript anymore

  43 | @@ -47,8 +44,7 @@ def transaction_graph_test(self, size, *, n_tx_to_mine, fee=100_000):
  44 |          """
  45 |          wallet = MiniWallet(self.nodes[0])
  46 |  
  47 | -        # Prep for fork with empty blocks to not use invalidateblock directly
  48 | -        # for reorg case. The rpc has different codepath

instagibbs commented at 1:52 PM on July 27, 2026:

wondering if there's a good place to add this removed comment to have people not reintroduce this pattern

yuvicc commented at 4:20 AM on July 28, 2026:

Maybe we can add in create_empty_fork docstring?

<details>

def create_empty_fork(node, fork_length=FORK_LENGTH):
    '''
        Creates a fork using node's chaintip as the starting point.
        Returns a list of blocks to submit in order.

        Prefer this over calling the invalidateblock RPC to simulate a reorg:
        invalidateblock takes a different codepath than a reorg caused by a
        competing chain arriving, so tests using it are not exercising the
        behaviour they mean to.
    '''

</details>