Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jul 31, 2024

This type of bug in the test code keeps biting back regularly, is hard to debug, and wastes review cycles: #30543 (comment) .

Fix all issues by catching it with a linter that checks for rule B008: https://docs.astral.sh/ruff/rules/function-call-in-default-argument/

This also allows to drop the hand-written linter that checks for rule B006: https://docs.astral.sh/ruff/rules/mutable-argument-default/

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 31, 2024

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 davidgumberg
Concept ACK mzumsande
Approach ACK vasild

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29965 (Lint: improve subtree exclusion by davidgumberg)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@maflcko
Copy link
Member Author

maflcko commented Jul 31, 2024

For reference, the current CI failure output is:

https://cirrus-ci.com/task/5936690097815552?logs=lint#L991

test/functional/feature_block.py:1329:52: B008 Do not perform function call `CScript` in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
     |
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])):
     |                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B008
1330 |         return create_tx_with_script(spend_tx, n, amount=value, script_pub_key=script)
     |
test/functional/feature_block.py:1341:67: B008 Do not perform function call `CScript` in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
     |
1339 |         sign_input_legacy(tx, 0, spend_tx.vout[0].scriptPubKey, self.coinbase_key)
1340 | 
1341 |     def create_and_sign_transaction(self, spend_tx, value, script=CScript([OP_TRUE])):
     |                                                                   ^^^^^^^^^^^^^^^^^^ B008
1342 |         tx = self.create_tx(spend_tx, 0, value, script)
1343 |         self.sign_tx(tx, spend_tx)
     |
test/functional/feature_block.py:1347:82: B008 Do not perform function call `CScript` in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
     |
1345 |         return tx
1346 | 
1347 |     def next_block(self, number, spend=None, additional_coinbase_value=0, script=CScript([OP_TRUE]), *, version=4):
     |                                                                                  ^^^^^^^^^^^^^^^^^^ B008
1348 |         if self.tip is None:
1349 |             base_block_hash = self.genesis_hash
     |
test/functional/test_framework/blocktools.py:157:80: B008 Do not perform function call `CScript` in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
    |
155 |     return coinbase
156 | 
157 | def create_tx_with_script(prevtx, n, script_sig=b"", *, amount, script_pub_key=CScript()):
    |                                                                                ^^^^^^^^^ B008
158 |     """Return one-input, one-output transaction object
159 |        spending the prevtx's n-th output with the given amount.
    |
test/functional/test_framework/messages.py:1297:27: B008 Do not perform function call `CTransaction` in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
     |
1295 |     msgtype = b"tx"
1296 | 
1297 |     def __init__(self, tx=CTransaction()):
     |                           ^^^^^^^^^^^^^^ B008
1298 |         self.tx = tx
     |
test/functional/test_framework/script.py:813:101: B008 Do not perform function call `CScript` in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
    |
811 |     return sha256(b"".join(o.serialize() for o in txTo.vout))
812 | 
813 | def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index = 0, scriptpath = False, script = CScript(), codeseparator_pos = -1, annex = None, leaf_ver = LEAF_VERSION_TAPSCRIPT):
    |                                                                                                     ^^^^^^^^^ B008
814 |     assert (len(txTo.vin) == len(spent_utxos))
815 |     assert (input_index < len(txTo.vin))
    |
Found 6 errors.
`ruff` found errors!
^---- ⚠️ Failure generated from lint check 'py_mut_arg_default'!
Check the default arguments in python

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Debug: https://github.com/bitcoin/bitcoin/runs/28143370126

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK as of 27b7d27. I am not proficient enough in Rust for a full ACK.

I confirm that this detects the problem resolved by #30552 and finds a bunch of other issues.

[patch] Fix all B008 violations
diff --git i/test/functional/feature_block.py w/test/functional/feature_block.py
index 172af27848..8fad4ef4d3 100755
--- i/test/functional/feature_block.py
+++ w/test/functional/feature_block.py
@@ -1323,31 +1323,37 @@ class FullBlockTest(BitcoinTestFramework):
 
     def add_transactions_to_block(self, block, tx_list):
         [tx.rehash() for tx in tx_list]
         block.vtx.extend(tx_list)
 
     # this is a little handier to use than the version in blocktools.py
-    def create_tx(self, spend_tx, n, value, script=CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])):
+    def create_tx(self, spend_tx, n, value, script=None):
+        if script is None:
+            script = CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])
         return create_tx_with_script(spend_tx, n, amount=value, script_pub_key=script)
 
     # sign a transaction, using the key we know about
     # this signs input 0 in tx, which is assumed to be spending output 0 in spend_tx
     def sign_tx(self, tx, spend_tx):
         scriptPubKey = bytearray(spend_tx.vout[0].scriptPubKey)
         if (scriptPubKey[0] == OP_TRUE):  # an anyone-can-spend
             tx.vin[0].scriptSig = CScript()
             return
         sign_input_legacy(tx, 0, spend_tx.vout[0].scriptPubKey, self.coinbase_key)
 
-    def create_and_sign_transaction(self, spend_tx, value, script=CScript([OP_TRUE])):
+    def create_and_sign_transaction(self, spend_tx, value, script=None):
+        if script is None:
+            script = CScript([OP_TRUE])
         tx = self.create_tx(spend_tx, 0, value, script)
         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):
+        if script is None:
+            script = CScript([OP_TRUE])
         if self.tip is None:
             base_block_hash = self.genesis_hash
             block_time = int(time.time()) + 1
         else:
             base_block_hash = self.tip.sha256
             block_time = self.tip.nTime + 1
diff --git i/test/functional/test_framework/blocktools.py w/test/functional/test_framework/blocktools.py
index 338b7fa3b0..f2f01e7b7c 100644
--- i/test/functional/test_framework/blocktools.py
+++ w/test/functional/test_framework/blocktools.py
@@ -151,18 +151,20 @@ def create_coinbase(height, pubkey=None, *, script_pubkey=None, extra_output_scr
         coinbaseoutput2.nValue = 0
         coinbaseoutput2.scriptPubKey = extra_output_script
         coinbase.vout.append(coinbaseoutput2)
     coinbase.calc_sha256()
     return coinbase
 
-def create_tx_with_script(prevtx, n, script_sig=b"", *, amount, script_pub_key=CScript()):
+def create_tx_with_script(prevtx, n, script_sig=b"", *, amount, script_pub_key=None):
     """Return one-input, one-output transaction object
        spending the prevtx's n-th output with the given amount.
 
        Can optionally pass scriptPubKey and scriptSig, default is anyone-can-spend output.
     """
+    if script_pub_key is None:
+        script_pub_key = CScript()
     tx = CTransaction()
     assert n < len(prevtx.vout)
     tx.vin.append(CTxIn(COutPoint(prevtx.sha256, n), script_sig, SEQUENCE_FINAL))
     tx.vout.append(CTxOut(amount, script_pub_key))
     tx.calc_sha256()
     return tx
diff --git i/test/functional/test_framework/script.py w/test/functional/test_framework/script.py
index 97d62f957b..b8ecd48d4e 100644
--- i/test/functional/test_framework/script.py
+++ w/test/functional/test_framework/script.py
@@ -807,15 +807,17 @@ def BIP341_sha_scriptpubkeys(spent_utxos):
 def BIP341_sha_sequences(txTo):
     return sha256(b"".join(i.nSequence.to_bytes(4, "little") for i in txTo.vin))
 
 def BIP341_sha_outputs(txTo):
     return sha256(b"".join(o.serialize() for o in txTo.vout))
 
-def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index = 0, scriptpath = False, script = CScript(), codeseparator_pos = -1, annex = None, leaf_ver = LEAF_VERSION_TAPSCRIPT):
+def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index = 0, scriptpath = False, script = None, codeseparator_pos = -1, annex = None, leaf_ver = LEAF_VERSION_TAPSCRIPT):
     assert (len(txTo.vin) == len(spent_utxos))
     assert (input_index < len(txTo.vin))
+    if script is None:
+        script = CScript()
     out_type = SIGHASH_ALL if hash_type == 0 else hash_type & 3
     in_type = hash_type & SIGHASH_ANYONECANPAY
     spk = spent_utxos[input_index].scriptPubKey
     ss = bytes([0, hash_type]) # epoch, hash_type
     ss += txTo.version.to_bytes(4, "little")
     ss += txTo.nLockTime.to_bytes(4, "little")

That should make CI green.

b008.diff.txt

@maflcko
Copy link
Member Author

maflcko commented Jul 31, 2024

I confirm that this detects the problem resolved by #30552 and finds a bunch of other issues.

Thanks for double checking. None of them are issues, because CScript in the functional tests should be immutable (similar to how a tuple is immutable in Python). However, I submitted a refactoring change here: #30554

@maflcko maflcko marked this pull request as draft July 31, 2024 09:31
@maflcko maflcko marked this pull request as ready for review August 2, 2024 13:05
@maflcko
Copy link
Member Author

maflcko commented Aug 2, 2024

Rebased on top of the fixes. Let's hope for a green CI 😅

@DrahtBot DrahtBot removed the CI failed label Aug 2, 2024
@maflcko
Copy link
Member Author

maflcko commented Aug 5, 2024

CI is green now. Can be tested by reverting a bugfix, for example:

git show ec5e294e4b830766dcc4a80add0613d3705c1794 | git apply --reverse

And then running the new linter.

Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, but someone who knows Rust better than I do will have to review it.

@maflcko
Copy link
Member Author

maflcko commented Aug 8, 2024

I just copy-pasted the logic flow from the mlc function (https://github.com/bitcoin/bitcoin/blame/fa9cff60fca74a7ff60042c5e632502b6c4c2899/test/lint/test_runner/src/main.rs#L432-L472) So maybe @willcl-ark may be qualified to review this?

@davidgumberg
Copy link
Contributor

davidgumberg commented Aug 8, 2024

Tested ACK fa9cff6 Left some nits, tested enforcement of B008 by reverting ec5e294e4b830766dcc4a80add0613d3705c1794, and B006 by inserting into one of the functional tests:

def b006_violator(x, mutable_default=[]):
    mutable_default.append(x)

The lint check added here complains.

Test output
$ git diff
diff --git a/test/functional/example_test.py b/test/functional/example_test.py
index 39cea2962f..a44ec61b03 100755
--- a/test/functional/example_test.py
+++ b/test/functional/example_test.py
@@ -34,6 +34,9 @@ from test_framework.util import (
     assert_equal,
 )
 
+def b006_violator(x, mutable_default=[]):
+    mutable_default.append(x)
+
 # P2PInterface is a class containing callbacks to be executed when a P2P
 # message is received from the node-under-test. Subclass P2PInterface and
 # override the on_*() methods if you need custom behaviour.
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index 1f566a1348..005f7546a8 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -1294,11 +1294,8 @@ class msg_tx:
     __slots__ = ("tx",)
     msgtype = b"tx"
 
-    def __init__(self, tx=None):
-        if tx is None:
-            self.tx = CTransaction()
-        else:
-            self.tx = tx
+    def __init__(self, tx=CTransaction()):
+        self.tx = tx
 
     def deserialize(self, f):
         self.tx.deserialize(f)

$ cargo run -- --lint=py_mut_arg_default
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.00s
     Running `target/debug/test_runner --lint=py_mut_arg_default`
test/functional/example_test.py:37:38: B006 Do not use mutable data structures for argument defaults
   |
35 | )
36 |
37 | def b006_violator(x, mutable_default=[]):
   |                                      ^^ B006
38 |     mutable_default.append(x)
   |
   = help: Replace with `None`; initialize within function

test/functional/test_framework/messages.py:1297:27: B008 Do not perform function call `CTransaction` in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
     |
1295 |     msgtype = b"tx"
1296 |
1297 |     def __init__(self, tx=CTransaction()):
     |                           ^^^^^^^^^^^^^^ B008
1298 |         self.tx = tx
     |

Found 2 errors.
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).
`ruff` found errors!
^---- ⚠️  Failure generated from lint check 'py_mut_arg_default'!
Check the default arguments in python

@DrahtBot DrahtBot requested review from mzumsande and vasild August 8, 2024 21:38
@maflcko
Copy link
Member Author

maflcko commented Aug 9, 2024

Addressed all feedback. Should be easy to re-ack

@davidgumberg
Copy link
Contributor

reACK fac7b7f

@fanquake fanquake merged commit 37a6d76 into bitcoin:master Aug 12, 2024
16 checks passed
@maflcko maflcko deleted the 2407-lint-py branch August 12, 2024 17:25
@bitcoin bitcoin locked and limited conversation to collaborators Aug 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants