test: Avoid CScript() as default function argument #30554

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2407-test-cscript changing 5 files +24 −16
  1. maflcko commented at 8:00 am on July 31, 2024: member

    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.

  2. test: Make leaf_script mandatory when scriptpath is set in TaprootSignatureMsg
    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)
    fadf621825
  3. DrahtBot commented at 8:00 am on July 31, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    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.

  4. DrahtBot renamed this:
    test: Avoid CScript() as default function argument
    test: Avoid CScript() as default function argument
    on Jul 31, 2024
  5. DrahtBot added the label Tests on Jul 31, 2024
  6. maflcko force-pushed on Jul 31, 2024
  7. maflcko force-pushed on Jul 31, 2024
  8. DrahtBot added the label CI failed on Jul 31, 2024
  9. DrahtBot removed the label CI failed on Jul 31, 2024
  10. in test/functional/feature_block.py:1330 in fa33f64baa outdated
    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)
    


    instagibbs commented at 12:52 pm on July 31, 2024:

    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)
    

    maflcko commented at 1:19 pm on July 31, 2024:
    Agree, this is more flexible if the arg is used more than once. Also, the code is less crammed.

    instagibbs commented at 2:57 pm on July 31, 2024:
    awesome thanks
  11. test: Avoid CScript() as default function argument
    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.
    fa46a1b74b
  12. maflcko force-pushed on Jul 31, 2024
  13. instagibbs approved
  14. instagibbs commented at 2:57 pm on July 31, 2024: member

    utACK fa46a1b74bd35371036af17b2df2036dbc993ce1

    (didn’t run the linter check)

  15. theStack approved
  16. theStack commented at 9:30 am on August 1, 2024: contributor
    lgtm ACK fa46a1b74bd35371036af17b2df2036dbc993ce1
  17. Thoragh commented at 9:35 am on August 2, 2024: contributor
    Have you investigated using https://docs.astral.sh/ruff/settings/#lint_flake8-bugbear_extend-immutable-calls instead of changing the python code?
  18. maflcko commented at 9:59 am on August 2, 2024: member

    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.

  19. in test/functional/feature_block.py:1351 in fa46a1b74b
    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):
    


    ismaelsadeeq commented at 11:48 am on August 2, 2024:

    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):
    

    maflcko commented at 12:28 pm on August 2, 2024:
    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.
  20. in test/functional/feature_block.py:1345 in fa46a1b74b
    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])
    


    ismaelsadeeq commented at 12:02 pm on August 2, 2024:

    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


    maflcko commented at 12:28 pm on August 2, 2024:

    same check

    No? They are different scripts.


    ismaelsadeeq commented at 12:41 pm on August 2, 2024:
    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 ?

    maflcko commented at 12:50 pm on August 2, 2024:

    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.


    maflcko commented at 12:54 pm on August 2, 2024:

    (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.

  21. ismaelsadeeq commented at 12:07 pm on August 2, 2024: member

    Tested ACK fa46a1b74bd35371036af17b2df2036dbc993ce1

    Just a single question and a nit.

  22. fanquake merged this on Aug 2, 2024
  23. fanquake closed this on Aug 2, 2024

  24. maflcko deleted the branch on Aug 2, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-06-18 21:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me