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/
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 | 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.
Reviewers, 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
0test/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
1 |
21328 | # this is a little handier to use than the version in blocktools.py
31329 | def create_tx(self, spend_tx, n, value, script=CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])):
4 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B008
51330 | return create_tx_with_script(spend_tx, n, amount=value, script_pub_key=script)
6 |
7test/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
8 |
91339 | sign_input_legacy(tx, 0, spend_tx.vout[0].scriptPubKey, self.coinbase_key)
101340 |
111341 | def create_and_sign_transaction(self, spend_tx, value, script=CScript([OP_TRUE])):
12 | ^^^^^^^^^^^^^^^^^^ B008
131342 | tx = self.create_tx(spend_tx, 0, value, script)
141343 | self.sign_tx(tx, spend_tx)
15 |
16test/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
17 |
181345 | return tx
191346 |
201347 | def next_block(self, number, spend=None, additional_coinbase_value=0, script=CScript([OP_TRUE]), *, version=4):
21 | ^^^^^^^^^^^^^^^^^^ B008
221348 | if self.tip is None:
231349 | base_block_hash = self.genesis_hash
24 |
25test/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
26 |
27155 | return coinbase
28156 |
29157 | def create_tx_with_script(prevtx, n, script_sig=b"", *, amount, script_pub_key=CScript()):
30 | ^^^^^^^^^ B008
31158 | """Return one-input, one-output transaction object
32159 | spending the prevtx's n-th output with the given amount.
33 |
34test/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
35 |
361295 | msgtype = b"tx"
371296 |
381297 | def __init__(self, tx=CTransaction()):
39 | ^^^^^^^^^^^^^^ B008
401298 | self.tx = tx
41 |
42test/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
43 |
44811 | return sha256(b"".join(o.serialize() for o in txTo.vout))
45812 |
46813 | def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index = 0, scriptpath = False, script = CScript(), codeseparator_pos = -1, annex = None, leaf_ver = LEAF_VERSION_TAPSCRIPT):
47 | ^^^^^^^^^ B008
48814 | assert (len(txTo.vin) == len(spent_utxos))
49815 | assert (input_index < len(txTo.vin))
50 |
51Found 6 errors.
52`ruff` found errors!
53^---- ⚠️ Failure generated from lint check 'py_mut_arg_default'!
54Check the default arguments in python
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28143370126
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.
Approach ACK as of 27b7d27400cc9a7d816b522822fca2361d463d67. 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.
0diff --git i/test/functional/feature_block.py w/test/functional/feature_block.py
1index 172af27848..8fad4ef4d3 100755
2--- i/test/functional/feature_block.py
3+++ w/test/functional/feature_block.py
4@@ -1323,31 +1323,37 @@ class FullBlockTest(BitcoinTestFramework):
5
6 def add_transactions_to_block(self, block, tx_list):
7 [tx.rehash() for tx in tx_list]
8 block.vtx.extend(tx_list)
9
10 # this is a little handier to use than the version in blocktools.py
11- def create_tx(self, spend_tx, n, value, script=CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])):
12+ def create_tx(self, spend_tx, n, value, script=None):
13+ if script is None:
14+ script = CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])
15 return create_tx_with_script(spend_tx, n, amount=value, script_pub_key=script)
16
17 # sign a transaction, using the key we know about
18 # this signs input 0 in tx, which is assumed to be spending output 0 in spend_tx
19 def sign_tx(self, tx, spend_tx):
20 scriptPubKey = bytearray(spend_tx.vout[0].scriptPubKey)
21 if (scriptPubKey[0] == OP_TRUE): # an anyone-can-spend
22 tx.vin[0].scriptSig = CScript()
23 return
24 sign_input_legacy(tx, 0, spend_tx.vout[0].scriptPubKey, self.coinbase_key)
25
26- def create_and_sign_transaction(self, spend_tx, value, script=CScript([OP_TRUE])):
27+ def create_and_sign_transaction(self, spend_tx, value, script=None):
28+ if script is None:
29+ script = CScript([OP_TRUE])
30 tx = self.create_tx(spend_tx, 0, value, script)
31 self.sign_tx(tx, spend_tx)
32 tx.rehash()
33 return tx
34
35- def next_block(self, number, spend=None, additional_coinbase_value=0, script=CScript([OP_TRUE]), *, version=4):
36+ def next_block(self, number, spend=None, additional_coinbase_value=0, script=None, *, version=4):
37+ if script is None:
38+ script = CScript([OP_TRUE])
39 if self.tip is None:
40 base_block_hash = self.genesis_hash
41 block_time = int(time.time()) + 1
42 else:
43 base_block_hash = self.tip.sha256
44 block_time = self.tip.nTime + 1
45diff --git i/test/functional/test_framework/blocktools.py w/test/functional/test_framework/blocktools.py
46index 338b7fa3b0..f2f01e7b7c 100644
47--- i/test/functional/test_framework/blocktools.py
48+++ w/test/functional/test_framework/blocktools.py
49@@ -151,18 +151,20 @@ def create_coinbase(height, pubkey=None, *, script_pubkey=None, extra_output_scr
50 coinbaseoutput2.nValue = 0
51 coinbaseoutput2.scriptPubKey = extra_output_script
52 coinbase.vout.append(coinbaseoutput2)
53 coinbase.calc_sha256()
54 return coinbase
55
56-def create_tx_with_script(prevtx, n, script_sig=b"", *, amount, script_pub_key=CScript()):
57+def create_tx_with_script(prevtx, n, script_sig=b"", *, amount, script_pub_key=None):
58 """Return one-input, one-output transaction object
59 spending the prevtx's n-th output with the given amount.
60
61 Can optionally pass scriptPubKey and scriptSig, default is anyone-can-spend output.
62 """
63+ if script_pub_key is None:
64+ script_pub_key = CScript()
65 tx = CTransaction()
66 assert n < len(prevtx.vout)
67 tx.vin.append(CTxIn(COutPoint(prevtx.sha256, n), script_sig, SEQUENCE_FINAL))
68 tx.vout.append(CTxOut(amount, script_pub_key))
69 tx.calc_sha256()
70 return tx
71diff --git i/test/functional/test_framework/script.py w/test/functional/test_framework/script.py
72index 97d62f957b..b8ecd48d4e 100644
73--- i/test/functional/test_framework/script.py
74+++ w/test/functional/test_framework/script.py
75@@ -807,15 +807,17 @@ def BIP341_sha_scriptpubkeys(spent_utxos):
76 def BIP341_sha_sequences(txTo):
77 return sha256(b"".join(i.nSequence.to_bytes(4, "little") for i in txTo.vin))
78
79 def BIP341_sha_outputs(txTo):
80 return sha256(b"".join(o.serialize() for o in txTo.vout))
81
82-def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index = 0, scriptpath = False, script = CScript(), codeseparator_pos = -1, annex = None, leaf_ver = LEAF_VERSION_TAPSCRIPT):
83+def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index = 0, scriptpath = False, script = None, codeseparator_pos = -1, annex = None, leaf_ver = LEAF_VERSION_TAPSCRIPT):
84 assert (len(txTo.vin) == len(spent_utxos))
85 assert (input_index < len(txTo.vin))
86+ if script is None:
87+ script = CScript()
88 out_type = SIGHASH_ALL if hash_type == 0 else hash_type & 3
89 in_type = hash_type & SIGHASH_ANYONECANPAY
90 spk = spent_utxos[input_index].scriptPubKey
91 ss = bytes([0, hash_type]) # epoch, hash_type
92 ss += txTo.version.to_bytes(4, "little")
93 ss += txTo.nLockTime.to_bytes(4, "little")
That should make CI green.
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: https://github.com/bitcoin/bitcoin/pull/30554
CI is green now. Can be tested by reverting a bugfix, for example:
0git show ec5e294e4b830766dcc4a80add0613d3705c1794 | git apply --reverse
And then running the new linter.
189+ let bin_name = "ruff";
190+ let checks = ["B006", "B008"]
191+ .iter()
192+ .map(|c| format!("--select={}", c))
193+ .collect::<Vec<_>>();
194+ let files = check_output(git().args(["ls-files", "--", "*.py"]))?;
0 let files = check_output(
1 git()
2 .args(["ls-files", "--", "*.py"])
3 .args(get_pathspecs_exclude_subtrees()),
4 )?;
nit: exclude subtrees from this check
I am trying to preserve the behavior of the previous check based on git grep --extended-regexp, which didn’t exclude subtrees either.
However, I applied your suggestion.
199+ match cmd.status() {
200+ Ok(status) if status.success() => Ok(()),
201+ Ok(_) => Err(format!("`{}` found errors!", bin_name)),
202+ Err(e) if e.kind() == ErrorKind::NotFound => {
203+ println!(
204+ "`{}` was not found in $PATH, skipping those check.",
nit:
0 "`{}` was not found in $PATH, skipping this check.",
203+ println!(
204+ "`{}` was not found in $PATH, skipping those check.",
205+ bin_name
206+ );
207+ Ok(())
208+ }
Feel free to disregard nit: I’m not 100% sure if it makes sense for this check to just warn when the binary is not found. A lint check returning Err() or Ok() only really matters for CI checking the return value of the runner, so not a big deal either way, but it would make this slightly shorter:
0 Err(e) if e.kind() == ErrorKind::NotFound => Err(format!(
1 "`{}` was not found in $PATH, skipping this check.",
2 bin_name
3 )),
192+ .map(|c| format!("--select={}", c))
193+ .collect::<Vec<_>>();
194+ let files = check_output(git().args(["ls-files", "--", "*.py"]))?;
195+
196+ let mut cmd = Command::new(bin_name);
197+ cmd.arg("check").args(checks).args(files.lines());
nit:
0 cmd.arg("check").args(checks).args(files.lines()).arg("--quiet");
This would silence unhelpful messages about fixes not being available.
Tested ACK https://github.com/bitcoin/bitcoin/commit/fa9cff60fca74a7ff60042c5e632502b6c4c2899 Left some nits, tested enforcement of B008 by reverting ec5e294e4b830766dcc4a80add0613d3705c1794, and B006 by inserting into one of the functional tests:
0def b006_violator(x, mutable_default=[]):
1 mutable_default.append(x)
The lint check added here complains.
0$ git diff
1diff --git a/test/functional/example_test.py b/test/functional/example_test.py
2index 39cea2962f..a44ec61b03 100755
3--- a/test/functional/example_test.py
4+++ b/test/functional/example_test.py
5@@ -34,6 +34,9 @@ from test_framework.util import (
6 assert_equal,
7 )
8
9+def b006_violator(x, mutable_default=[]):
10+ mutable_default.append(x)
11+
12 # P2PInterface is a class containing callbacks to be executed when a P2P
13 # message is received from the node-under-test. Subclass P2PInterface and
14 # override the on_*() methods if you need custom behaviour.
15diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
16index 1f566a1348..005f7546a8 100755
17--- a/test/functional/test_framework/messages.py
18+++ b/test/functional/test_framework/messages.py
19@@ -1294,11 +1294,8 @@ class msg_tx:
20 __slots__ = ("tx",)
21 msgtype = b"tx"
22
23- def __init__(self, tx=None):
24- if tx is None:
25- self.tx = CTransaction()
26- else:
27- self.tx = tx
28+ def __init__(self, tx=CTransaction()):
29+ self.tx = tx
30
31 def deserialize(self, f):
32 self.tx.deserialize(f)
33
34$ cargo run -- --lint=py_mut_arg_default
35 Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.00s
36 Running `target/debug/test_runner --lint=py_mut_arg_default`
37test/functional/example_test.py:37:38: B006 Do not use mutable data structures for argument defaults
38 |
3935 | )
4036 |
4137 | def b006_violator(x, mutable_default=[]):
42 | ^^ B006
4338 | mutable_default.append(x)
44 |
45 = help: Replace with `None`; initialize within function
46
47test/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
48 |
491295 | msgtype = b"tx"
501296 |
511297 | def __init__(self, tx=CTransaction()):
52 | ^^^^^^^^^^^^^^ B008
531298 | self.tx = tx
54 |
55
56Found 2 errors.
57No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).
58`ruff` found errors!
59^---- ⚠️ Failure generated from lint check 'py_mut_arg_default'!
60Check the default arguments in python