<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32587.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | theStack, instagibbs |
| Concept ACK | BrandonOdiwuor, enirox001 |
| Stale ACK | Prabhat1308, itokichi7777 |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
feel free to ping me when it's ready
feel free to ping me when it's ready
Sure!
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/43164565056</sub>
<sub>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.</sub>
<details><summary>Hints</summary>
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.
</details>
@instagibbs can you review this, will add other tests once this one gets ACK'ed
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
passing self into a standalone function doesn't really make sense. This should just be the node
got it!
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
# 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.
Agree, I had the same thought! I will remove that for now!
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
same here
ACK with the approach . Left some nits
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.
not "first" node anymore
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)
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
Correct!
Correct!
Addressed some nits and rebased with master!
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())
could assert that the node's new chaintip indeed is the last submitted block to avoid regressions in the test
Done
LGTM c98f3615e7eb6774b8f49fe7b2aeed8c056622c7
feel free to add more cases
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/44317225556</sub>
<sub>LLM reason (✨ experimental): Lint errors caused the CI failure.</sub>
<details><summary>Hints</summary>
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.
</details>
Concept ACK in favor of replacing direct invalidateblock usage with a proper fork-based approach for simulating reorgs in tests, as this better reflects realistic network behavior
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/44585514502</sub>
<sub>LLM reason (✨ experimental): Lint check failed due to style and whitespace errors in the Python code.</sub>
<details><summary>Hints</summary>
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.
</details>
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!
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/44857399027</sub>
<sub>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.</sub>
<details><summary>Hints</summary>
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.
</details>
Ready for review!
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")
accidental comment?
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)
this changes behavior a bit but should give the coverage we want still, as all we care about is getting 1p2c into mempool with TRUC
@yuvicc can you un-resolve this? I'd like other reviewers to see it and yell at me if I'm wrong :)
Right, the children aren't from the disconnected blocks but I agree it's fine.
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
It can move backwards in height if reorg goes across difficulty adustments (impossible on regtest) or if we mine blocks with varying MTP and use time-based timelock on the tx
Given that the coinbase maturity tests also use invalidateblock, let's just keep this particular test untouched for now. This test setup is going to have to be a bit more advanced and can be done in a follow-up
actually ended up being pretty easy to remove for a time-based timelock
diff --git a/test/functional/mempool_reorg.py b/test/functional/mempool_reorg.py
index 3906117e8b..0f6124f9ab 100755
--- a/test/functional/mempool_reorg.py
+++ b/test/functional/mempool_reorg.py
@@ -28,5 +28,6 @@ from test_framework.blocktools import (
# Number of blocks to create in temporary blockchain branch for reorg testing
-FORK_LENGTH = 10
+# Needs to be long enough to allow MTP to move arbitrarily forward
+FORK_LENGTH = 20
class MempoolCoinbaseTest(BitcoinTestFramework):
@@ -125,4 +126,8 @@ class MempoolCoinbaseTest(BitcoinTestFramework):
wallet = self.wallet
+ # Prevent clock from moving blocks further forward in time
+ now = int(time.time())
+ self.nodes[0].setmocktime(now)
+
# Start with a 200 block chain
assert_equal(self.nodes[0].getblockcount(), 200)
@@ -148,9 +153,11 @@ class MempoolCoinbaseTest(BitcoinTestFramework):
spend_3 = wallet.create_self_transfer(utxo_to_spend=utxo_3)
- self.log.info("Create another transaction which is time-locked to two blocks in the future")
+ future = now + 300
+
+ self.log.info("Create another transaction which is time-locked to 300 seconds in the future")
utxo = wallet.get_utxo(txid=coinbase_txids[0])
timelock_tx = wallet.create_self_transfer(
utxo_to_spend=utxo,
- locktime=self.nodes[0].getblockcount() + 2,
+ locktime=future,
)['hex']
@@ -174,7 +181,13 @@ class MempoolCoinbaseTest(BitcoinTestFramework):
self.log.info("Generate a block")
- # Prep for fork
+ # Prep for fork, only go FORK_LENGTH seconds into the MTP future max
fork_blocks = create_empty_fork(self.nodes[0], fork_length=FORK_LENGTH)
- self.generate(self.nodes[0], 1)
+
+ # Jump node and MTP 300 seconds and generate a slighty weaker chain than reorg one
+ self.nodes[0].setmocktime(future)
+ self.generate(self.nodes[0], FORK_LENGTH - 1)
+ block_time = self.nodes[0].getblock(self.nodes[0].getbestblockhash())['time']
+ assert(block_time >= now + 300)
+
# generate() implicitly syncs blocks, so that peer 1 gets the block before timelock_tx
# Otherwise, peer 1 would put the timelock_tx in m_lazy_recent_rejects
@@ -193,7 +206,11 @@ class MempoolCoinbaseTest(BitcoinTestFramework):
self.sync_blocks()
+ # We went backwards in time to boot timelock_tx_id
+ fork_block_time = self.nodes[0].getblock(self.nodes[0].getbestblockhash())['time']
+ assert fork_block_time < block_time
+
self.log.info("The time-locked transaction is now too immature and has been removed from the mempool")
self.log.info("spend_3_1 has been re-orged out of the chain and is back in the mempool")
- assert_equal(set(self.nodes[0].getrawmempool()), {spend_1_id, spend_2_1_id, spend_3_1_id, timelock_tx_id})
+ assert_equal(set(self.nodes[0].getrawmempool()), {spend_1_id, spend_2_1_id, spend_3_1_id})
self.log.info("Use invalidateblock to re-org back and make all those coinbase spends immature/invalid")
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/45340830150</sub>
<sub>LLM reason (✨ experimental): The CI failure is caused by errors detected during the lint check, specifically a whitespace issue on a blank line.</sub>
<details><summary>Hints</summary>
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.
</details>
Addressed @instagibbs suggestion to use time-based timelock instead of n block method.
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]the typo wasn't fixed in the recent force pushes, fwiw
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
mempool is already clean?
update: rebased to master!
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:
# Variation A: explicit node parameter
def trigger_reorg(fork_blocks, node):
for block in fork_blocks:
node.submitblock(block.serialize().hex())
assert_equal(node.getbestblockhash(), fork_blocks[-1].hash)
# Variation B: hardcoded to self.nodes[0]
def trigger_reorg(self, fork_blocks):
for block in fork_blocks:
self.nodes[0].submitblock(block.serialize().hex())
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.
update: in mempool_reorg.py: “slighty” -> “slightly” [correct spelling]
reACK a161f058089dfe5f3aa7d3b9635f0400f0147b5d
spelling fix
Rebased -> master@672c85cb1ea0d76ef9e596b2a964b2311b11b815
think you need to fix issues first
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/46275827051</sub>
<sub>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.</sub>
<details><summary>Hints</summary>
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.
</details>
Fixed conflicts and rebased to master.
ACK cec7ce0
changes since I last reviewed
mempool_reorg.py
forgot about this, definitely worthwhile to get merged soon
Fixed some linter errors. No change in tests.
@instagibbs @mzumsande want to circle back here for a look?
ACK 0f1f02995b715a83f71b1b8fc92fa520625ec427
git range-diff master a161f058089dfe5f3aa7d3b9635f0400f0147b5d 0f1f02995b715a83f71b1b8fc92fa520625ec427
tACK 0f1f029
looks like you forgot to remove the definition from mempool_updatefromblock.py
11 | @@ -12,8 +12,7 @@ 12 | import time 13 | 14 | from test_framework.blocktools import ( 15 | - create_block, 16 | - create_coinbase, 17 | + create_empty_fork, create_block, create_coinbase
nit or something to consider for the future: The style convention is to put them in alphabetical order on separate lines so that diffs are smaller/clearer when you add or remove. This change, for instance, changes a lot more lines than necessary.
67 | @@ -69,7 +68,7 @@ def transaction_graph_test(self, size, *, n_tx_to_mine, fee=100_000): 68 | 69 | # Prep for fork with empty blocks to not use invalidateblock directly 70 | # for reorg case. The rpc has different codepath 71 | - fork_blocks = self.create_empty_fork(fork_length=7) 72 | + fork_blocks = create_empty_fork(self.nodes[0], fork_length=7)
nit: these changes are in f1ede0e61129fbef82e7d7c261cdb8dd8a5c0aa1 when they should be in 0f1f02995b715a83f71b1b8fc92fa520625ec427
19 | @@ -20,6 +20,12 @@ 20 | from test_framework.wallet import ( 21 | MiniWallet, 22 | ) 23 | +from test_framework.blocktools import ( 24 | + create_empty_fork, 25 | +) 26 | + 27 | +# Number of blocks to create in temporary blockchain branch for reorg testing 28 | +FORK_LENGTH = 10
nit: This is defined in all the test files and is always 10 except in one place. I'd probably make it a default arg?
312 | @@ -293,8 +313,8 @@ def run_test(self): 313 | self.sync_mempools() 314 | 315 | # Now try to disconnect the tip on each node... 316 | - self.nodes[1].invalidateblock(self.nodes[1].getbestblockhash()) 317 | - self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash()) 318 | + self.trigger_reorg(fork_blocks, self.nodes[0]) 319 | + self.trigger_reorg(fork_blocks, self.nodes[1])
Seems strange to have them both submit the fork blocks. In a real reorg, node1 will receive the blocks from node0 and should process them in the same way. We should test that that's true.
265 | @@ -257,11 +266,19 @@ def run_test(self): 266 | assert_equal(entry1['fees']['base'], entry0['fees']['base']) 267 | assert_equal(entry1['vsize'], entry0['vsize']) 268 | assert_equal(entry1['depends'], entry0['depends']) 269 | +
There is still an invalidateblock in this test?
ping, could you respond to this @yuvicc
have you had a chance to look at this?
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.
Thank you @glozow and @instagibbs for the review.
Addressed feedback:
create_empty_fork method per @instagibbs's commentFORK_LENGTH a default argument per @glozow's commentAdditionally, updated the mempool_updatefromblock test to use the trigger_reorg method for consistency with other tests.
309 | @@ -293,9 +310,11 @@ def run_test(self): 310 | self.sync_mempools() 311 | 312 | # Now try to disconnect the tip on each node... 313 | - self.nodes[1].invalidateblock(self.nodes[1].getbestblockhash()) 314 | - self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash()) 315 | + self.trigger_reorg(fork_blocks, self.nodes[0]) 316 | self.sync_blocks() 317 | 318 | + # Now both nodes should be on the same chain
sync_blocks already ensures this no?
204 | - for node in self.nodes: 205 | - node.invalidateblock(last_block[0]) 206 | + self.trigger_reorg(fork_blocks, self.nodes[0]) 207 | + self.sync_blocks() 208 | + 209 | + # We went backwards in time to boot timelock_tx_id
nit: I've now forgotten the exact reason, but we're using invalidateblock here because of MTP reasons, yeah? Might want to leave a note for future reviewers wondering, like me who has forgotten already.
reACK 7defbc7dc6155dabfab6a0ccddac69260d6d2d54 modulo question about remaining invalidateblock usages.
git range-diff master 0f1f02995b715a83f71b1b8fc92fa520625ec427 7defbc7dc6155dabfab6a0ccddac69260d6d2d54
Concept ACK
Looks like half of the create_empty_fork method moving slipped into the second commit?
#32587 (review) still has invalidateblock in test/functional/mempool_packages.py ? If you don't have the time to carry the patch I can take over, just let me know.
Friendly ping @instagibbs @glozow! in comment, I think we are testing the prioritization feature so its fine if we use invalidateblock here. We would only want to replace invalidateblock during re-org testing to use fork based approach.
Why invalidateblock is OK here? It's just a test utility mechanism to quickly return a mined transaction to the mempool to verify prioritization settings persist.
Line 183 - 188:
# Prioritise a transaction that has been mined, then add it back to the
# mempool by using invalidateblock.
self.nodes[0].prioritisetransaction(txid=chain[-1], fee_delta=2000)
self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash())
# Keep node1's tip synced with node0
self.nodes[1].invalidateblock(self.nodes[1].getbestblockhash())
reACK b1581924a79e120f166719943646484ba00ff21b
git range-diff master 7defbc7dc6155dabfab6a0ccddac69260d6d2d54 b1581924a79e120f166719943646484ba00ff21b
ACK b1581924a79e120f166719943646484ba00ff21b
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.
Agree that deduplicating this as a follow-up is a good idea. Maybe even something like a fancy ForkGenerator class would make sense, with methods like .prepare_fork and .trigger_reorg (where e.g. the latter would only be allowed to be called after the former, checking that blocks have been mined in-between to ensure that an actual reorg happens etc.).
Agree that deduplicating this as a follow-up is a good idea. Maybe even something like a fancy ForkGenerator class would make sense
I think it would also be good to have optional args that make non-empty forks to test mix-and-match cases of things re-entering the mempool / already in.
For now I think this is RFM.
Please rebase this.
Rebased.
reACK 2a64851c34fcf9955a673eb77463f68ab583a7bd
git range-diff master b1581924a79e120f166719943646484ba00ff21b 2a64851c34fcf9955a673eb77463f68ab583a7bd
re-ACK 2a64851c34fcf9955a673eb77463f68ab583a7bd
meta nit: the create_empty_fork removal slipped again from first into second commit. Still fine to merge as-is, as far as I'm concerned.
Fixed nit related to moving of create_empty_fork.
re-ACK 70d9e8f0a15d07a27ae37befb5c1bce71c98d8de
reACK 70d9e8f0a15d07a27ae37befb5c1bce71c98d8de
I've opened a draft follow-up PR #33959 to address the duplication of trigger_reorg code.
CC @instagibbs @theStack