Unlike other function calls in default arguments, CScript should not cause any issues in the tests, because they are const.
However, this change allows to enable the "function-call-in-default-argument (B008)" lint rule, which will help to catch severe test bugs, such as #30543 (comment) .
The lint rule will be enabled in a follow-up, when all violations are fixed.
This removes the default value, because there should not be a use-case
to fall back to a an empty leaf_script by default. (If there was, it
could trivially be added back)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | instagibbs, theStack, ismaelsadeeq |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
1325 | @@ -1326,8 +1326,8 @@ def add_transactions_to_block(self, block, tx_list): 1326 | block.vtx.extend(tx_list) 1327 | 1328 | # this is a little handier to use than the version in blocktools.py 1329 | - def create_tx(self, spend_tx, n, value, script=CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])): 1330 | - return create_tx_with_script(spend_tx, n, amount=value, script_pub_key=script) 1331 | + def create_tx(self, spend_tx, n, value, output_script=None): 1332 | + return create_tx_with_script(spend_tx, n, amount=value, output_script=CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE]) if output_script is None else output_script)
slight preference for this pattern unless I'm missing a subtlety
if output_script is None:
output_script = CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])
return create_tx_with_script(spend_tx, n, amount=value, output_script=output_script)
Agree, this is more flexible if the arg is used more than once. Also, the code is less crammed.
awesome thanks
This does not cause any issues, because CScript in the tests are const.
However, this change allows to enable the
"function-call-in-default-argument (B008)" lint rule.
utACK fa46a1b74bd35371036af17b2df2036dbc993ce1
(didn't run the linter check)
lgtm ACK fa46a1b74bd35371036af17b2df2036dbc993ce1
Have you investigated using https://docs.astral.sh/ruff/settings/#lint_flake8-bugbear_extend-immutable-calls instead of changing the python code?
Have you investigated using https://docs.astral.sh/ruff/settings/#lint_flake8-bugbear_extend-immutable-calls instead of changing the python code?
Yes, but there is nothing that enforces that CScript remains immutable in the future. It just seems too fragile to allow it. A +18-10 test-only diff seems preferable in any case.
1349 | self.sign_tx(tx, spend_tx) 1350 | tx.rehash() 1351 | return tx 1352 | 1353 | - def next_block(self, number, spend=None, additional_coinbase_value=0, script=CScript([OP_TRUE]), *, version=4): 1354 | + def next_block(self, number, spend=None, additional_coinbase_value=0, *, script=None, version=4):
non-blocking nit:
since you are changed script_pub_key to output_script will be nice to be explicit here also
def next_block(self, number, spend=None, additional_coinbase_value=0, *, coinbase_output_script=None, version=4):
Happy to push a commit, if someone creates one. Also, I am happy to review a follow-up doing this, if someone creates one, but I'll leave it as-is for now.
1339 | @@ -1338,13 +1340,17 @@ def sign_tx(self, tx, spend_tx): 1340 | return 1341 | sign_input_legacy(tx, 0, spend_tx.vout[0].scriptPubKey, self.coinbase_key) 1342 | 1343 | - def create_and_sign_transaction(self, spend_tx, value, script=CScript([OP_TRUE])): 1344 | - tx = self.create_tx(spend_tx, 0, value, script) 1345 | + def create_and_sign_transaction(self, spend_tx, value, output_script=None): 1346 | + if output_script is None: 1347 | + output_script = CScript([OP_TRUE])
We are doing the same check for a None output_script in create_tx,
should we instead delete this and rely on the check in create_tx?
After removing the check here and next_block the tests still passes
same check
No? They are different scripts.
Yes, but both are anyone-can-spend scripts. Is there a reason to prefer CScript([OP_TRUE]) for create_and_sign_transaction and next_block? We can rely on the check in create_tx to avoid redundancy ?
Yeah, sounds good.
The reason was to avoid too small transaction sizes (See 364bae5f7a6b16eef63990154e48f19e7e693039)
Padding the others isn't needed, but it can't hurt, I guess.
(MIN_STANDARD_TX_NONWITNESS_SIZE was dropped in the meantime from 82 to 64, so the padding may or may not be needed)
Currently I don't have the time to check it, and it seems unrelated anyway? If you want to change the padding to make it larger or smaller, it would be a larger pull request, than just a default argument change.
Tested ACK fa46a1b74bd35371036af17b2df2036dbc993ce1
Just a single question and a nit.