test: Fix reorg patterns in mempool tests to use proper fork-based approach

yuvicc commented at 1:26 pm on May 22, 2025: contributor

Addresses #32531

Currently updating other functional tests to replace direct use of invalidateblock with proper fork-based reorg behaviour. The direct invalidation approach bypasses important validation checks and has depth limitations(10 block) that don’t match real-world reorg scenarios.

If you want to review then please review mempool_ephemeral_dust.py test whether it matches real reorg pattern.

Plan after Concept ACK:

Fix mempool_ephemeral_dust.py reorg patterns
Audit and fix other tests to use real reorg patterns

DrahtBot commented at 1:26 pm on May 22, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32587.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	Prabhat1308

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

instagibbs commented at 1:27 pm on May 22, 2025: member

feel free to ping me when it’s ready

yuvicc commented at 1:30 pm on May 22, 2025: contributor

feel free to ping me when it’s ready

Sure!

yuvicc force-pushed on May 23, 2025

yuvicc force-pushed on May 30, 2025

DrahtBot added the label CI failed on May 30, 2025

DrahtBot commented at 6:35 am on May 30, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/43164565056 LLM reason (✨ experimental): The CI failure is primarily due to errors from the lint check, specifically from ruff, which identified and flagged Python code issues such as unused imports and trailing whitespace.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

yuvicc force-pushed on Jun 2, 2025

DrahtBot removed the label CI failed on Jun 2, 2025

yuvicc marked this as ready for review on Jun 3, 2025

yuvicc commented at 7:10 am on June 3, 2025: contributor

@instagibbs can you review this, will add other tests once this one gets ACK’ed

in test/functional/test_framework/blocktools.py:121 in b70c907bc2 outdated

116+        Creates a fork using first node's chaintip as the starting point.
117+        Returns a list of blocks to submit in order.
118+    '''
119+    tip = int(self.nodes[0].getbestblockhash(), 16)
120+    height = self.nodes[0].getblockcount()
121+    block_time = self.nodes[0].getblock(self.nodes[0].getbestblockhash())['time'] + 1

maflcko commented at 7:44 am on June 3, 2025:

passing self into a standalone function doesn’t really make sense. This should just be the node

yuvicc commented at 8:10 am on June 3, 2025:

got it!

yuvicc renamed this:
~~[WIP] test: Fix reorg patterns in mempool tests to use proper fork-based approach~~
test: Fix reorg patterns in mempool tests to use proper fork-based approach
on Jun 3, 2025

DrahtBot added the label Tests on Jun 3, 2025

yuvicc force-pushed on Jun 4, 2025

in test/functional/mempool_ephemeral_dust.py:335 in 9565415592 outdated

330@@ -323,49 +331,68 @@ def test_reorgs(self):
331 
332         # Get dusty tx mined, then check that it makes it back into mempool on reorg
333         # due to bypass_limits allowing 0-fee individually
334+
335+        # Prep for fork with empty blocks to not use invalidateblock directly

Prabhat1308 commented at 12:04 pm on June 9, 2025:

0        # Prep for fork with empty blocks

I think this should be included in the PR description or commit description rather than in the code. We would not like to refer to a old function we used here which anyway could be easily found if we backtracked to the PR.

yuvicc commented at 10:33 am on June 10, 2025:

Agree, I had the same thought! I will remove that for now!

in test/functional/mempool_ephemeral_dust.py:350 in 9565415592 outdated

347         # Create a sweep that has dust of its own and leaves dusty_tx's dust unspent
348         sweep_tx = self.wallet.create_self_transfer_multi(fee_per_output=0, utxos_to_spend=[dusty_tx["new_utxos"][0]], version=3)
349         self.add_output_to_create_multi_result(sweep_tx)
350         assert_raises_rpc_error(-26, "min relay fee not met", self.nodes[0].sendrawtransaction, sweep_tx["hex"])
351 
352+        # Prep for fork with empty blocks to not use invalidateblock directly

Prabhat1308 commented at 12:04 pm on June 9, 2025:

same here

Prabhat1308 commented at 12:04 pm on June 9, 2025: contributor

ACK with the approach . Left some nits

in test/functional/test_framework/blocktools.py:119 in 3e5f125968 outdated

113@@ -114,6 +114,26 @@ def create_block(hashprev=None, coinbase=None, ntime=None, *, version=None, tmpl
114     block.calc_sha256()
115     return block
116 
117+def create_empty_fork(node, fork_length):
118+    '''
119+        Creates a fork using first node's chaintip as the starting point.

instagibbs commented at 1:07 pm on June 9, 2025:

not “first” node anymore

in test/functional/mempool_ephemeral_dust.py:339 in 9565415592 outdated

330@@ -323,49 +331,68 @@ def test_reorgs(self):
331 
332         # Get dusty tx mined, then check that it makes it back into mempool on reorg
333         # due to bypass_limits allowing 0-fee individually
334+
335+        # Prep for fork with empty blocks to not use invalidateblock directly
336+        fork_blocks = create_empty_fork(self.nodes[0], fork_length=10)

instagibbs commented at 1:09 pm on June 9, 2025:

if fork length is always 10 because it’s sufficient and easy, just make a constant like FORK_LENGTH with a quick comment to describe what it’s for

yuvicc commented at 10:34 am on June 10, 2025:

Correct!

yuvicc commented at 10:34 am on June 10, 2025:

Correct!

move the method create_empty_fork to test_framework (blocktools.py) so that it can be used by other tests as well 19428b08ac

yuvicc force-pushed on Jun 10, 2025

yuvicc commented at 1:39 pm on June 10, 2025: contributor

Addressed some nits and rebased with master!

yuvicc force-pushed on Jun 10, 2025

fix: reorg behaviour in mempool tests to match real one

we prepare for fork with 10 empty blocks to not use invalidateblock directly

c98f3615e7

yuvicc force-pushed on Jun 10, 2025

in test/functional/mempool_ephemeral_dust.py:76 in c98f3615e7

69@@ -64,6 +70,11 @@ def create_ephemeral_dust_package(self, *, tx_version, dust_tx_fee=0, dust_value
70 
71         return dusty_tx, sweep_tx
72 
73+    def trigger_reorg(self, fork_blocks):
74+        """Trigger reorg of the fork blocks."""
75+        for block in fork_blocks:
76+            self.nodes[0].submitblock(block.serialize().hex())

instagibbs commented at 12:47 pm on June 11, 2025:

could assert that the node’s new chaintip indeed is the last submitted block to avoid regressions in the test

instagibbs commented at 12:48 pm on June 11, 2025: member

LGTM c98f3615e7eb6774b8f49fe7b2aeed8c056622c7

feel free to add more cases

test: Fix reorg patterns in mempool tests to use proper fork-based approach #32587

Code Coverage & Benchmarks

Reviews

Conflicts