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)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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
0 if output_script is None:
1 output_script = CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])
2 return create_tx_with_script(spend_tx, n, amount=value, output_script=output_script)
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)
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
0 def next_block(self, number, spend=None, additional_coinbase_value=0, *, coinbase_output_script=None, version=4):
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.
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.