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

yuvicc · 2025-05-22T13:26:44Z

Updated 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. For more details see #32531.

Fixes #32531

DrahtBot · 2025-05-22T13:26:48Z

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	BrandonOdiwuor, enirox001
Stale ACK	Prabhat1308, itokichi7777, instagibbs

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33629 (Cluster mempool by sdaftuar)
#33616 (policy: don't CheckEphemeralSpends on reorg by instagibbs)
#33591 (Cluster mempool followups by sdaftuar)

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.

instagibbs · 2025-05-22T13:27:33Z

feel free to ping me when it's ready

yuvicc · 2025-05-22T13:30:51Z

feel free to ping me when it's ready

Sure!

DrahtBot · 2025-05-30T06:35:15Z

🚧 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.}

Hints

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 · 2025-06-03T07:10:01Z

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

test/functional/test_framework/blocktools.py

Prabhat1308

ACK with the approach . Left some nits

test/functional/mempool_ephemeral_dust.py

test/functional/test_framework/blocktools.py

test/functional/mempool_ephemeral_dust.py

yuvicc · 2025-06-10T13:39:55Z

Addressed some nits and rebased with master!

instagibbs

LGTM c98f361

feel free to add more cases

test/functional/mempool_ephemeral_dust.py

yuvicc · 2025-07-18T22:01:25Z

Fixed conflicts and rebased to master.

Prabhat1308

ACK cec7ce0

changes since I last reviewed

the PR has been extended to 5 more tests other than mempool_reorg.py
Tested and reviewed the added tests.

instagibbs · 2025-09-27T17:51:47Z

forgot about this, definitely worthwhile to get merged soon

yuvicc · 2025-10-03T13:04:32Z

Rebased to 1ed00a0.

yuvicc · 2025-10-03T13:42:32Z

Fixed some linter errors. No change in tests.

fanquake · 2025-10-03T14:18:39Z

@instagibbs @mzumsande want to circle back here for a look?

instagibbs · 2025-10-06T14:19:54Z

ACK 0f1f029

git range-diff master a161f058089dfe5f3aa7d3b9635f0400f0147b5d 0f1f02995b715a83f71b1b8fc92fa520625ec427

itokichi7777 · 2025-10-11T11:27:34Z

tACK 0f1f029

instagibbs · 2025-10-14T14:42:34Z

#33616 (comment)

looks like you forgot to remove the definition from mempool_updatefromblock.py

glozow

Looks pretty good, just a few comments.

A nice-to-have: in the functional tests with multiple nodes, it would be a bit more realistic / interesting if the two forks were mined by different nodes.

test/functional/mempool_updatefromblock.py

test/functional/mempool_ephemeral_dust.py

test/functional/mempool_truc.py

test/functional/mempool_packages.py

glozow · 2025-10-14T20:56:48Z

test/functional/mempool_packages.py

            assert_equal(entry1['fees']['base'], entry0['fees']['base'])
            assert_equal(entry1['vsize'], entry0['vsize'])
            assert_equal(entry1['depends'], entry0['depends'])
+


There is still an invalidateblock in this test?

ping, could you respond to this @yuvicc

have you had a chance to look at this?

…py module to enable reuse across multiple tests.

yuvicc · 2025-10-15T06:57:18Z

Thank you @glozow and @instagibbs for the review.

Addressed feedback:

Removed the create_empty_fork method per @instagibbs's comment
Modified changes in the next commit as suggested by @glozow (comment)
Made FORK_LENGTH a default argument per @glozow's comment
Removed duplicate trigger re-org for both nodes; now asserting they are on the same chain per @glozow's comment

Additionally, updated the mempool_updatefromblock test to use the trigger_reorg method for consistency with other tests.

instagibbs

reACK 7defbc7 modulo question about remaining invalidateblock usages.

git range-diff master 0f1f029 7defbc7

test/functional/mempool_packages.py

instagibbs · 2025-10-15T12:59:44Z

test/functional/mempool_packages.py

            assert_equal(entry1['fees']['base'], entry0['fees']['base'])
            assert_equal(entry1['vsize'], entry0['vsize'])
            assert_equal(entry1['depends'], entry0['depends'])
+


ping, could you respond to this @yuvicc

test/functional/mempool_reorg.py

yuvicc · 2025-10-16T04:53:27Z

Addresses @instagibbs 's comment on adding a note for future reviewers.
Addressed @instagibbs 's comment on removing the redundant assert.

DrahtBot mentioned this pull request May 23, 2025

rpc: generateblock to allow multiple outputs #32468

Open

yuvicc force-pushed the 2025-05-update_test_reorg_behaviour branch 3 times, most recently from 7bbdba1 to 516c6ce Compare May 30, 2025 06:35

DrahtBot added the CI failed label May 30, 2025

yuvicc force-pushed the 2025-05-update_test_reorg_behaviour branch from 516c6ce to b70c907 Compare June 2, 2025 16:19

DrahtBot removed the CI failed label Jun 2, 2025

yuvicc marked this pull request as ready for review June 3, 2025 07:08

maflcko reviewed Jun 3, 2025

View reviewed changes

test/functional/test_framework/blocktools.py Outdated Show resolved Hide resolved

yuvicc changed the title ~~[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 Jun 3, 2025

DrahtBot added the Tests label Jun 3, 2025

yuvicc force-pushed the 2025-05-update_test_reorg_behaviour branch from b70c907 to 9565415 Compare June 4, 2025 07:21

yuvicc mentioned this pull request Jun 9, 2025

avoid using invalidateblock to directly test reorg behavior #32531

Open

Prabhat1308 reviewed Jun 9, 2025

View reviewed changes

test/functional/mempool_ephemeral_dust.py Outdated Show resolved Hide resolved

test/functional/mempool_ephemeral_dust.py Outdated Show resolved Hide resolved

instagibbs reviewed Jun 9, 2025

View reviewed changes

test/functional/test_framework/blocktools.py Outdated Show resolved Hide resolved

test/functional/mempool_ephemeral_dust.py Outdated Show resolved Hide resolved

yuvicc force-pushed the 2025-05-update_test_reorg_behaviour branch from 9565415 to 45fad54 Compare June 10, 2025 13:39

yuvicc force-pushed the 2025-05-update_test_reorg_behaviour branch 2 times, most recently from 35414a8 to c98f361 Compare June 10, 2025 20:54

instagibbs reviewed Jun 11, 2025

View reviewed changes

test/functional/mempool_ephemeral_dust.py Show resolved Hide resolved

yuvicc force-pushed the 2025-05-update_test_reorg_behaviour branch from c98f361 to 3012d6d Compare June 15, 2025 17:12

yuvicc mentioned this pull request Jun 15, 2025

PR Review Club: test: Fix reorg patterns in mempool tests to use proper fork-based approach Bitshala/BitcoinCore-PR-Review-Club#126

Closed

yuvicc changed the title ~~test: Fix reorg patterns in mempool tests to use proper fork-based approach~~ test: Fix reorg patterns in tests to use proper fork-based approach Jun 16, 2025

yuvicc force-pushed the 2025-05-update_test_reorg_behaviour branch 2 times, most recently from e186193 to f079571 Compare June 18, 2025 07:25

Prabhat1308 reviewed Jul 20, 2025

View reviewed changes

DrahtBot requested review from BrandonOdiwuor and instagibbs July 20, 2025 20:46

fanquake added this to the 31.0 milestone Sep 29, 2025

DrahtBot mentioned this pull request Sep 30, 2025

Mempool: Do not enforce TRUC checks on reorg #33504

Merged

DrahtBot added the Needs rebase label Oct 2, 2025

yuvicc force-pushed the 2025-05-update_test_reorg_behaviour branch from cec7ce0 to e4c4d7f Compare October 3, 2025 13:03

yuvicc force-pushed the 2025-05-update_test_reorg_behaviour branch from e4c4d7f to 0f1f029 Compare October 3, 2025 13:40

DrahtBot removed the Needs rebase label Oct 3, 2025

DrahtBot requested a review from Prabhat1308 October 6, 2025 14:20

DrahtBot mentioned this pull request Oct 10, 2025

Cluster mempool followups #33591

Open

instagibbs mentioned this pull request Oct 13, 2025

policy: don't CheckEphemeralSpends on reorg #33616

Open

glozow reviewed Oct 14, 2025

View reviewed changes

yuvicc force-pushed the 2025-05-update_test_reorg_behaviour branch from 0f1f029 to 48b3819 Compare October 15, 2025 06:28

Move the create_empty_fork method to the test framework's blocktools.…

80fba3a

…py module to enable reuse across multiple tests.

yuvicc force-pushed the 2025-05-update_test_reorg_behaviour branch from 48b3819 to 7defbc7 Compare October 15, 2025 06:54

DrahtBot mentioned this pull request Oct 15, 2025

Cluster mempool #33629

Open

instagibbs reviewed Oct 15, 2025

View reviewed changes

fix: reorg behaviour in mempool tests to match real one

f0717d0

yuvicc force-pushed the 2025-05-update_test_reorg_behaviour branch from 7defbc7 to f0717d0 Compare October 16, 2025 04:51

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

Are you sure you want to change the base?

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

Conversation

yuvicc commented May 22, 2025 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

instagibbs commented May 22, 2025

Uh oh!

yuvicc commented May 22, 2025

Uh oh!

DrahtBot commented May 30, 2025

Uh oh!

yuvicc commented Jun 3, 2025

Uh oh!

Uh oh!

Prabhat1308 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yuvicc commented Jun 10, 2025

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yuvicc commented Jul 18, 2025

Uh oh!

Prabhat1308 left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Sep 27, 2025

Uh oh!

yuvicc commented Oct 3, 2025

Uh oh!

yuvicc commented Oct 3, 2025

Uh oh!

fanquake commented Oct 3, 2025

Uh oh!

instagibbs commented Oct 6, 2025

Uh oh!

itokichi7777 commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Oct 14, 2025

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

instagibbs Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

instagibbs Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

yuvicc commented Oct 15, 2025

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

instagibbs Oct 15, 2025

Choose a reason for hiding this comment

yuvicc commented May 22, 2025 •

edited by fanquake

Loading

DrahtBot commented May 22, 2025 •

edited

Loading

itokichi7777 commented Oct 11, 2025 •

edited

Loading