The generate RPCs are special in that they should only be called by the test framework itself. This way, they will call the sync function on the nodes, which can avoid intermittent test issues. Also, when the sync is disabled, it will happen explicitly by setting the sync_fun.
Apply this rule here, so that all generate calls are written consistently.
<!--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/31403.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | rkrux, hodlinator, i-am-yuvi, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
any way you can see to prevent "regressions"?
I don't see a trivial way to enforce this (even more) in Python, but given that there are only 9 lines that needed to be touched, it seems that the majority of code is fine, which is nice.
324 | @@ -325,7 +325,7 @@ def test_reorgs(self): 325 | dusty_tx, _ = self.create_ephemeral_dust_package(tx_version=3) 326 | assert_raises_rpc_error(-26, "min relay fee not met", self.nodes[0].sendrawtransaction, dusty_tx["hex"]) 327 | 328 | - block_res = self.nodes[0].rpc.generateblock(self.wallet.get_address(), [dusty_tx["hex"]]) 329 | + block_res = self.generateblock(self.nodes[0], self.wallet.get_address(), [dusty_tx["hex"]], sync_fun=self.no_op)
sync_fun=self.no_op
Note for myself and other reviewers: Syncing is not done in this test because the nodes are disconnected on line 321 and hence, syncing all the nodes would result in assertion errors.
File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 769, in sync_blocks
assert (all([len(x.getpeerinfo()) for x in rpc_connections]))
~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
This way, they will call the sync function on the nodes, which can avoid intermittent test issues.
Guessing this is a proactive cleanup?
tACK fa6e599cf9fbacb393ac17e0a1b3572f217817f0
wallet_migration.py test fails on my system but I see there's an issue for it already here: https://github.com/bitcoin/bitcoin/issues/31447
ACK fa6e599cf9fbacb393ac17e0a1b3572f217817f0
Noticed how the calls were made through .rpc. while reviewing #30239 and thought that it was a neat way to express the "same thing" in a minimally terser way. :sweat_smile:
One could probably decrease the likelyhood of rogue invalid_call=False-calls creeping back in through a change like:
- def generatetoaddress(self, *args, invalid_call, **kwargs):
- assert not invalid_call
+ def generatetoaddress(self, *args, called_by_framework, **kwargs):
+ assert called_by_framework, "Should only be called by BitcoinTestFramework itself, not subclasses. Enforces syncing of nodes in order to stabilize tests."
One could probably decrease the likelyhood of rogue
invalid_call=False-calls creeping back in through a change like:
Looks good. I am happy to review such a change. My recommendation would be to reword it a bit, so that the solution to the problem is explained at the same time: Direct call of this mining RPC is discouraged. Please use one of the self.generate* methods on the test framework, which sync the nodes to avoid intermittent test issues. You may use sync_fun=self.no_op to disable the sync explicitly.
Tested ACK fa6e599cf9fbacb393ac17e0a1b3572f217817f0
In summary, this change would generate RPCs similarly to the previous implementation, but with a key difference: when called by the test framework, it would automatically trigger the sync function. This approach helps prevent intermittent test failures by ensuring consistent node synchronization.
ACK fa6e599cf9fbacb393ac17e0a1b3572f217817f0
The follow-up idea in #31403 (review) is up-for-grabs, if someone is looking for something to work on
I've opened a follow-up PR #31599