-
Notifications
You must be signed in to change notification settings - Fork 37.7k
lint: Find function calls in default arguments #30553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
For reference, the current CI failure output is: https://cirrus-ci.com/task/5936690097815552?logs=lint#L991
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
There was a problem hiding this 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.
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 |
Rebased on top of the fixes. Let's hope for a green CI 😅 |
CI is green now. Can be tested by reverting a bugfix, for example:
And then running the new linter. |
There was a problem hiding this 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.
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? |
Tested ACK fa9cff6 Left some nits, tested enforcement of 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 |
Addressed all feedback. Should be easy to re-ack |
reACK fac7b7f |
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/