test: Avoid CScript() as default function argument #30554
Conversation
…natureMsg 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. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
faf81f0 to
fa33f64
Compare
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.
fa33f64 to
fa46a1b
Compare
|
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 |
| self.sign_tx(tx, spend_tx) | ||
| tx.rehash() | ||
| return tx | ||
|
|
||
| def next_block(self, number, spend=None, additional_coinbase_value=0, script=CScript([OP_TRUE]), *, version=4): | ||
| def next_block(self, number, spend=None, additional_coinbase_value=0, *, script=None, version=4): |
There was a problem hiding this comment.
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, *, script=None, version=4): | |
| def next_block(self, number, spend=None, additional_coinbase_value=0, *, coinbase_output_script=None, version=4): |
There was a problem hiding this comment.
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.
| if output_script is None: | ||
| output_script = CScript([OP_TRUE]) |
There was a problem hiding this comment.
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?
| if output_script is None: | |
| output_script = CScript([OP_TRUE]) |
After removing the check here and next_block the tests still passes
There was a problem hiding this comment.
same check
No? They are different scripts.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Yeah, sounds good.
The reason was to avoid too small transaction sizes (See 364bae5)
Padding the others isn't needed, but it can't hurt, I guess.
There was a problem hiding this comment.
(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.
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.