mempool_reorg.py is a bit confusing:
trigger_reorg helper, but it is not used consistently.So fix all issues by using a single node and the Python helpers.
This avoids having to disconnect nodes, and to treat nodes as holding
chains.
Using the existing create_empty_fork and trigger_reorg is easier.
The test used two nodes, which is confusing:
* node[0] uses immediate tx relay via noban@127.0.0.1; However, this
will only apply to incoming connections, not to outgoing ones.
* The second test requires a peer without immediate tx relay, so it
would be cleaner to not have any global setting for faster tx relay
here.
* Two nodes is not required for testing here. Creating and submitting
forks, or p2p connections can be done with Python helpers.
Using a single node also allows to remove a bunch of sync and connect
calls.
Also fix inverted comment: "valid" should say "invalid".
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
ACK fa3aeb51cfb1f664627730ccfded040a16ad2e91
ACK fa3aeb51cfb1f664627730ccfded040a16ad2e91
The new fork setup gives the same reorg shape as before, with the block containing tx_disconnected replaced by three empty fork blocks. I also confirmed the comment fix, since after the final invalidatedblock, sendrawtransaction(timelock_tx) is rejected as non-final.
Removing the whitelist was also necessary, because putting -whitelist=noban@127.0.0.1 back makes peer1 receive 2 invs early and the relay test fails.
utACK fa3aeb51cfb1f664627730ccfded040a16ad2e91
ACK fa3aeb5
Ran mempool_reorg.py locally on each of the three commits and it passes on all of them. Over three runs each runtime went from about 1.8s on master to about 0.86s with this PR, which makes sense with one node instead of two.
As a sanity check, changing fork_length=3 to fork_length=1 in test_reorg_relay makes the assertion in trigger_reorg fail, confirming the fork is what drives the reorg.
Non-blocking nits, could be a follow-up:
test_reorg, the second self.log.info("Generate a block") is not followed by any block generation.test_reorg_relay, the comment "Create a child of that transaction" sits right below tx_before_reorg, but tx_child actually spends tx_disconnected. Naming the parent explicitly would make it clearer.
Non-blocking nits, could be a follow-up:
Both are unrelated to the changes here, so I'll leave this as-is for now to avoid review churn.