The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32587.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Prabhat1308 |
| Concept ACK | BrandonOdiwuor, enirox001 |
| Stale ACK | instagibbs |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
feel free to ping me when it’s ready
Sure!
🚧 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.
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
self into a standalone function doesn’t really make sense. This should just be the node
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
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.
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
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.
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)
FORK_LENGTH with a quick comment to describe what it’s for
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())
LGTM c98f3615e7eb6774b8f49fe7b2aeed8c056622c7
feel free to add more cases
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/44317225556
LLM reason (✨ experimental): Lint errors caused the CI failure.
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.
invalidateblock usage with a proper fork-based approach for simulating reorgs in tests, as this better reflects realistic network behavior
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/44585514502
LLM reason (✨ experimental): Lint check failed due to style and whitespace errors in the Python code.
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.
While fixing the reorg pattern in mempool_truc.py test, I observed an interesting behavior. During reorg the test_2child_rbf test fails when using fork blocks(10) pattern, which later found was due to 1-child policy during mempool re-entry after a reorg. Basically the parent txn was mined in a block, later in next block 2 child txns were mined and after a reorg, the 1-child policy holds true here, but when using invalidateblock() this doesn’t hold true.
For now, I have removed the assertion for ancestor_tx to exceed the 1-child limit as it doesn’t happened during re-org testing using fork block pattern.
Not sure if I am correct here, it would be helpful for me if someone could review the commit https://github.com/bitcoin/bitcoin/pull/32587/commits/33e9f3fd1bcdfc79eecfe377a608113f1bda8bae. If this is the correct behavior then adding a check for 1-child limit during reorg would be better IMO.
cc @glozow
Basically the parent txn was mined in a block, later in next block 2 child txns were mined and after a reorg, the 1-child policy holds true here, but when using invalidateblock() this doesn’t hold true.
I haven’t reviewed in depth, but I don’t think the children are supposed to be mined and then reorged, only the parent is. The children should already be in mempool when the disconnectpool is being resubmitted. When the parent comes in, we don’t check it for TRUC children limits (the implementation assumes no in-mempool children which is true in all cases except reorgs), which is why we end up with 1p2c.
I haven’t reviewed in depth, but I don’t think the children are supposed to be mined and then reorged, only the parent is.
Agree! I shall change the test to match this!
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/44857399027
LLM reason (✨ experimental): The CI failure is caused by errors detected during the Python linting step, specifically unused imports and an unused variable, which are treated as errors.
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.
195- node.invalidateblock(last_block[0])
196- self.log.info("The time-locked transaction is now too immature and has been removed from the mempool")
197+ self.trigger_reorg(fork_blocks, self.nodes[0])
198+ self.sync_blocks()
199+
200+ # self.log.info("The time-locked transaction is now too immature and has been removed from the mempool")
490- self.generate(node, 1)
491- self.check_mempool([])
492-
493 # Create a reorg, causing ancestor_tx to exceed the 1-child limit
494- node.invalidateblock(block)
495+ self.trigger_reorg(fork_blocks)
ACK 0feecd78e52aede26c07391b9f361410e6a8a4ad
though I think the last two commits can be squashed
though I think the last two commits can be squashed
Done!
reACK eb330b58d8d20bb4a5002906cb48bb03c5fa1a10
squashes and a couple errant comments deleted
196+ self.sync_blocks()
197+
198 self.log.info("The time-locked transaction is now too immature and has been removed from the mempool")
199 self.log.info("spend_3_1 has been re-orged out of the chain and is back in the mempool")
200- assert_equal(set(self.nodes[0].getrawmempool()), {spend_1_id, spend_2_1_id, spend_3_1_id})
201+ assert_equal(set(self.nodes[0].getrawmempool()), {spend_1_id, spend_2_1_id, spend_3_1_id, timelock_tx_id})
actually this is wrong, the test is checking that timelock_tx_id is not in mempool. If we don’t use invalidateblock it’s essentially impossible to trigger this behavior since the chain should not move backwards in height ever.
I re-read this PR after writing up #32838
Let me think about how to best do this test case and enhancing it possibly
actually ended up being pretty easy to remove for a time-based timelock
0diff --git a/test/functional/mempool_reorg.py b/test/functional/mempool_reorg.py
1index 3906117e8b..0f6124f9ab 100755
2--- a/test/functional/mempool_reorg.py
3+++ b/test/functional/mempool_reorg.py
4@@ -28,5 +28,6 @@ from test_framework.blocktools import (
5
6 # Number of blocks to create in temporary blockchain branch for reorg testing
7-FORK_LENGTH = 10
8+# Needs to be long enough to allow MTP to move arbitrarily forward
9+FORK_LENGTH = 20
10
11 class MempoolCoinbaseTest(BitcoinTestFramework):
12@@ -125,4 +126,8 @@ class MempoolCoinbaseTest(BitcoinTestFramework):
13 wallet = self.wallet
14
15+ # Prevent clock from moving blocks further forward in time
16+ now = int(time.time())
17+ self.nodes[0].setmocktime(now)
18+
19 # Start with a 200 block chain
20 assert_equal(self.nodes[0].getblockcount(), 200)
21@@ -148,9 +153,11 @@ class MempoolCoinbaseTest(BitcoinTestFramework):
22 spend_3 = wallet.create_self_transfer(utxo_to_spend=utxo_3)
23
24- self.log.info("Create another transaction which is time-locked to two blocks in the future")
25+ future = now + 300
26+
27+ self.log.info("Create another transaction which is time-locked to 300 seconds in the future")
28 utxo = wallet.get_utxo(txid=coinbase_txids[0])
29 timelock_tx = wallet.create_self_transfer(
30 utxo_to_spend=utxo,
31- locktime=self.nodes[0].getblockcount() + 2,
32+ locktime=future,
33 )['hex']
34
35@@ -174,7 +181,13 @@ class MempoolCoinbaseTest(BitcoinTestFramework):
36 self.log.info("Generate a block")
37
38- # Prep for fork
39+ # Prep for fork, only go FORK_LENGTH seconds into the MTP future max
40 fork_blocks = create_empty_fork(self.nodes[0], fork_length=FORK_LENGTH)
41- self.generate(self.nodes[0], 1)
42+
43+ # Jump node and MTP 300 seconds and generate a slighty weaker chain than reorg one
44+ self.nodes[0].setmocktime(future)
45+ self.generate(self.nodes[0], FORK_LENGTH - 1)
46+ block_time = self.nodes[0].getblock(self.nodes[0].getbestblockhash())['time']
47+ assert(block_time >= now + 300)
48+
49 # generate() implicitly syncs blocks, so that peer 1 gets the block before timelock_tx
50 # Otherwise, peer 1 would put the timelock_tx in m_lazy_recent_rejects
51@@ -193,7 +206,11 @@ class MempoolCoinbaseTest(BitcoinTestFramework):
52 self.sync_blocks()
53
54+ # We went backwards in time to boot timelock_tx_id
55+ fork_block_time = self.nodes[0].getblock(self.nodes[0].getbestblockhash())['time']
56+ assert fork_block_time < block_time
57+
58 self.log.info("The time-locked transaction is now too immature and has been removed from the mempool")
59 self.log.info("spend_3_1 has been re-orged out of the chain and is back in the mempool")
60- assert_equal(set(self.nodes[0].getrawmempool()), {spend_1_id, spend_2_1_id, spend_3_1_id, timelock_tx_id})
61+ assert_equal(set(self.nodes[0].getrawmempool()), {spend_1_id, spend_2_1_id, spend_3_1_id})
62
63 self.log.info("Use invalidateblock to re-org back and make all those coinbase spends immature/invalid")
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/45340830150
LLM reason (✨ experimental): The CI failure is caused by errors detected during the lint check, specifically a whitespace issue on a blank line.
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.
177@@ -160,10 +178,18 @@ def run_test(self):
178 self.log.info("Broadcast and mine spend_3_1")
179 spend_3_1_id = self.nodes[0].sendrawtransaction(spend_3_1['hex'])
180 self.log.info("Generate a block")
181- last_block = self.generate(self.nodes[0], 1)
182+
183+ # Prep for fork, only go FORK_LENGTH seconds into the MTP future max
184+ fork_blocks = create_empty_fork(self.nodes[0], fork_length=FORK_LENGTH)
185+
186+ # Jump node and MTP 300 seconds and generate a slighty weaker chain than reorg one
in mempool_reorg.py: “slighty” -> “slightly” [correct spelling]
275+ self.trigger_reorg(fork_blocks, self.nodes[0])
276+
277+ # Check if the txs are returned to the mempool
278+ assert_equal(self.nodes[0].getrawmempool(), mempool0)
279+
280+ # Clean-up the mempool
reACK 3afc73134bf652d01e25232701c2ef192b3fea0b
just a reminder that you don’t need to rebase on master unless a silent merge conflict is detected or drahtbot yells at you about an issue merging
git range-diff master 4ddc577953a7407ea44a3e3d8d9a4635929307ca 3afc73134bf652d01e25232701c2ef192b3fea0b
code review and tested-ACK
This PR is well designed: the refactoring to centralize fork creation and the switch to realistic reorg behavior both improve test clarity and fidelity.
create_empty_fork into blocktools.py, enabling reuse across functional tests.invalidateblock(), which better matches real‐world chain behavior.One remaining question on duplication:
Across several test files, trigger_reorg is defined twice:
0# Variation A: explicit node parameter
1def trigger_reorg(fork_blocks, node):
2 for block in fork_blocks:
3 node.submitblock(block.serialize().hex())
4 assert_equal(node.getbestblockhash(), fork_blocks[-1].hash)
5
6# Variation B: hardcoded to self.nodes[0]
7def trigger_reorg(self, fork_blocks):
8 for block in fork_blocks:
9 self.nodes[0].submitblock(block.serialize().hex())
10 assert_equal(self.nodes[0].getbestblockhash(), fork_blocks[-1].hash)
Only the node lookup differs. Would it make sense to extract a single trigger_reorg(fork_blocks, node) utility in test_framework/blocktools.py and have all tests call that?
Across several test files,
trigger_reorgis defined twice:
I am not sure if you have reviewed the code nicely, there isn’t any duplication but rather a different approach using time-based timelock, see comment
Only the node lookup differs. Would it make sense to extract a single trigger_reorg(fork_blocks, node) utility in test_framework/blocktools.py and have all tests call that?
For now, I think we can keep like this. There are lot’s of other tests as well which needs the reorg correction and can be done in a follow-up pr.
Thanks for the clarification! You’re absolutely right - I should have reviewed the implementation details more carefully. I appreciate you pointing out that these are different approaches (including the time-based timelock approach) rather than simple duplication.
Also, you’re correct about my phrasing - “defined twice” was unclear when it’s actually “defined multiple times across files.”
Makes sense to address this in a follow-up PR along with other tests that need reorg corrections.
reACK a161f058089dfe5f3aa7d3b9635f0400f0147b5d
spelling fix
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/46275827051
LLM reason (✨ experimental): The CI failure is caused by errors detected by clang-tidy in the Python linter, specifically undefined names create_block and create_coinbase.
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.
ACK cec7ce0
changes since I last reviewed
mempool_reorg.py