Looks like recent versions of vulture are triggering false positives on while True loops with internal breaks.
Failure: https://github.com/bitcoin/bitcoin/actions/runs/22981056848/job/66720754466?pr=34628
Upstream issue: https://github.com/jendrikseipp/vulture/issues/412
I guess the triggering change was: https://github.com/jendrikseipp/vulture/pull/378
I wonder if this tool ever found a real true positive, given that we are running it at confidence=100%, which can only find very limited things? Sure, dead code in tests isn't ideal and possibly scary, but if this only finds the plain obvious cases, that are unlikely to be written, and trivial to catch during manual review, maybe it could be removed?
No strong opinion, just wondering. In the meantime, I've submitted https://github.com/bitcoin/bitcoin/pull/34814
I was hoping that it's functionality would just be ingested by ruff at some point: https://github.com/astral-sh/ruff/issues/872
Yes I've been hoping astral would pick this up too for some time, but it seems like for now, all astral has on this front (via ty) is unused variable detection and a few other easy-to-spot types. But no "unused classes or functions". AFAIU there seems to be some barriers on how to implement sanely for python, as there's not an obious way to do it outside of parsing all files for all usage of all classes etc. as everything in python in public.
ty has a similar issue here: https://github.com/astral-sh/ty/issues/2829, and I might slightly expect it to be added there rather than ruff...
all astral has on this front (via
ty) is unused variable detection and a few other easy-to-spot types. But no "unused classes or functions".
I think this is also what vulture does for us, no? Unused classes and functions need to be manually removed, see 5c005363a880c136cc44ff2456a402e398fcbf44
I guess it could help to have a set of MWE what vulture catches today for use, and what ty catches. If they are the same, it could make sense to use ty.
I guess it could help to have a set of MWE what vulture catches today for use, and what ty catches. If they are the same, it could make sense to use ty.
I could try and work something like that out. I do have #34547 open to switch to ty (amongst other things) already :)
(Also, GitHub changed the merge commit refresh schedule https://github.blog/changelog/2026-02-19-changes-to-test-merge-commit-generation-for-pull-requests/, since the current commit was 10h old, not 12h, I closed and re-opened the pull to re-trigger)
I guess it could help to have a set of MWE what vulture catches today for use, and what ty catches. If they are the same, it could make sense to use ty.
Ok so ty does not have this capability at all yet, my bad. Ruff can detect some per-file items (unused imports/local variables/arguments), but nothing cross-file like vulture.
If we dropped vulture for ruff we'd lose:
We could enable unused argument rules:
"ARG001", # unused function argument
"ARG002", # unused method argument
"ARG003", # unused classmethod argument
"ARG004", # unused staticmethod argument
"ARG005", # unused lambda argument
which when enabled (on top of 34547) find 44 "infractions":
<details><summary>Details</summary> <p>
core/worktrees/modernise-linter on modernise-linter [$!?] via △ v4.1.2 via 🐍 v3.13.12 via ❄️ impure (nix-shell-env)
❯ ruff check .
ARG001 Unused function argument: `size`
--> contrib/tracing/log_raw_p2p_msgs.py:159:33
|
158 | # BCC: perf buffer handle function for inbound_messages
159 | def handle_inbound(_, data, size):
| ^^^^
160 | """ Inbound message handler.
|
ARG001 Unused function argument: `size`
--> contrib/tracing/log_raw_p2p_msgs.py:169:34
|
167 | # BCC: perf buffer handle function for outbound_messages
168 |
169 | def handle_outbound(_, data, size):
| ^^^^
170 | """ Outbound message handler.
|
ARG001 Unused function argument: `size`
--> contrib/tracing/log_utxocache_flush.py:84:31
|
82 | b = BPF(text=program, usdt_contexts=[bitcoind_with_usdts])
83 |
84 | def handle_flush(_, data, size):
| ^^^^
85 | """ Coins Flush handler.
86 | Called each time coin caches and indexes are flushed."""
|
ARG001 Unused function argument: `size`
--> contrib/tracing/mempool_monitor.py:140:31
|
138 | return datetime.now(timezone.utc)
139 |
140 | def handle_added(_, data, size):
| ^^^^
141 | event = bpf["added_events"].event(data)
142 | events.append((get_timestamp(), "added", event))
|
ARG001 Unused function argument: `size`
--> contrib/tracing/mempool_monitor.py:144:33
|
142 | events.append((get_timestamp(), "added", event))
143 |
144 | def handle_removed(_, data, size):
| ^^^^
145 | event = bpf["removed_events"].event(data)
146 | events.append((get_timestamp(), "removed", event))
|
ARG001 Unused function argument: `size`
--> contrib/tracing/mempool_monitor.py:148:34
|
146 | events.append((get_timestamp(), "removed", event))
147 |
148 | def handle_rejected(_, data, size):
| ^^^^
149 | event = bpf["rejected_events"].event(data)
150 | events.append((get_timestamp(), "rejected", event))
|
ARG001 Unused function argument: `size`
--> contrib/tracing/mempool_monitor.py:152:34
|
150 | events.append((get_timestamp(), "rejected", event))
151 |
152 | def handle_replaced(_, data, size):
| ^^^^
153 | event = bpf["replaced_events"].event(data)
154 | events.append((get_timestamp(), "replaced", event))
|
ARG001 Unused function argument: `size`
--> contrib/tracing/p2p_monitor.py:140:33
|
139 | # BCC: perf buffer handle function for inbound_messages
140 | def handle_inbound(_, data, size):
| ^^^^
141 | """ Inbound message handler.
|
ARG001 Unused function argument: `size`
--> contrib/tracing/p2p_monitor.py:153:34
|
152 | # BCC: perf buffer handle function for outbound_messages
153 | def handle_outbound(_, data, size):
| ^^^^
154 | """ Outbound message handler.
|
ARG001 Unused function argument: `kwargs`
--> src/crc32c/.ycm_extra_conf.py:125:30
|
125 | def FlagsForFile(filename, **kwargs):
| ^^^^^^
126 | """Implements the YouCompleteMe API."""
|
ARG002 Unused method argument: `success`
--> test/functional/feature_segwit.py:347:52
|
345 | return [p2wpkh, p2sh_p2wpkh, p2pk, p2pkh, p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh]
346 |
347 | def create_and_mine_tx_from_txids(self, txids, success=True):
| ^^^^^^^
348 | tx = CTransaction()
349 | for i in txids:
|
ARG005 Unused lambda argument: `h`
--> test/functional/feature_taproot.py:877:16
|
875 | lambda h: h,
876 | # Combine with hash 0
877 | lambda h: bytes([0 for _ in range(32)]),
| ^
878 | # Combine with hash 2^256-1
879 | lambda h: bytes([0xff for _ in range(32)]),
|
ARG005 Unused lambda argument: `h`
--> test/functional/feature_taproot.py:879:16
|
877 | lambda h: bytes([0 for _ in range(32)]),
878 | # Combine with hash 2^256-1
879 | lambda h: bytes([0xff for _ in range(32)]),
| ^
880 | # Combine with itself-1 (BE)
881 | lambda h: (int.from_bytes(h, 'big') - 1).to_bytes(32, 'big'),
|
ARG005 Unused lambda argument: `h`
--> test/functional/feature_taproot.py:1217:22
|
1215 | # == Test case for [#24765](/bitcoin-bitcoin/24765/) ==
1216 |
1217 | zero_fn = lambda h: bytes([0 for _ in range(32)])
| ^
1218 | tap = taproot_construct(pubs[0], [("leaf", CScript([pubs[1], OP_CHECKSIG, pubs[1], OP_CHECKSIGADD, OP_2, OP_EQUAL])), zero_fn])
1219 | add_spender(spenders, "case24765", tap=tap, leaf="leaf", inputs=[getter("sign"), getter("sign")], key=secs[1], no_fail=True)
|
ARG005 Unused lambda argument: `idx`
--> test/functional/interface_rpc.py:148:40
|
146 | def test_batch_requests(self):
147 | self.log.info("Testing empty batch request...")
148 | self.test_batch_request(lambda idx: None)
| ^^^
149 |
150 | self.log.info("Testing basic JSON-RPC 2.0 batch request...")
|
ARG005 Unused lambda argument: `idx`
--> test/functional/interface_rpc.py:151:40
|
150 | self.log.info("Testing basic JSON-RPC 2.0 batch request...")
151 | self.test_batch_request(lambda idx: BatchOptions(version=2))
| ^^^
152 |
153 | self.log.info("Testing JSON-RPC 2.0 batch with notifications...")
|
ARG005 Unused lambda argument: `idx`
--> test/functional/interface_rpc.py:157:40
|
156 | self.log.info("Testing JSON-RPC 2.0 batch of ALL notifications...")
157 | self.test_batch_request(lambda idx: BatchOptions(version=2, notification=True))
| ^^^
158 |
159 | # JSONRPC 1.1 does not support batch requests, but test them for backwards compatibility.
|
ARG005 Unused lambda argument: `idx`
--> test/functional/interface_rpc.py:161:40
|
159 | # JSONRPC 1.1 does not support batch requests, but test them for backwards compatibility.
160 | self.log.info("Testing nonstandard JSON-RPC 1.1 batch request...")
161 | self.test_batch_request(lambda idx: BatchOptions(version=1))
| ^^^
162 |
163 | self.log.info("Testing nonstandard mixed JSON-RPC 1.1/2.0 batch request...")
|
ARG005 Unused lambda argument: `idx`
--> test/functional/interface_rpc.py:167:40
|
166 | self.log.info("Testing nonstandard batch request without version numbers...")
167 | self.test_batch_request(lambda idx: BatchOptions())
| ^^^
168 |
169 | self.log.info("Testing nonstandard batch request without version numbers or ids...")
|
ARG005 Unused lambda argument: `idx`
--> test/functional/interface_rpc.py:170:40
|
169 | self.log.info("Testing nonstandard batch request without version numbers or ids...")
170 | self.test_batch_request(lambda idx: BatchOptions(notification=True))
| ^^^
171 |
172 | self.log.info("Testing nonstandard jsonrpc 1.0 version number is accepted...")
|
ARG005 Unused lambda argument: `idx`
--> test/functional/interface_rpc.py:173:40
|
172 | self.log.info("Testing nonstandard jsonrpc 1.0 version number is accepted...")
173 | self.test_batch_request(lambda idx: BatchOptions(request_fields={"jsonrpc": "1.0"}))
| ^^^
174 |
175 | self.log.info("Testing unrecognized jsonrpc version number is rejected...")
|
ARG005 Unused lambda argument: `idx`
--> test/functional/interface_rpc.py:176:40
|
175 | self.log.info("Testing unrecognized jsonrpc version number is rejected...")
176 | self.test_batch_request(lambda idx: BatchOptions(
| ^^^
177 | request_fields={"jsonrpc": "2.1"},
178 | response_fields={"result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "JSON-RPC version not supported"}}))
|
ARG001 Unused function argument: `args`
--> test/functional/mocks/invalid_signer.py:20:15
|
18 | sys.exit(int(mock_result[0]))
19 |
20 | def enumerate(args):
| ^^^^
21 | sys.stdout.write(json.dumps([{"fingerprint": "b3c19bfc", "type": "trezor", "model": "trezor_t"}]))
|
ARG001 Unused function argument: `args`
--> test/functional/mocks/multi_signers.py:10:15
|
8 | import sys
9 |
10 | def enumerate(args):
| ^^^^
11 | sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"},
12 | {"fingerprint": "00000002", "type": "trezor", "model": "trezor_one"}]))
|
ARG001 Unused function argument: `args`
--> test/functional/mocks/no_signer.py:10:15
|
8 | import sys
9 |
10 | def enumerate(args):
| ^^^^
11 | sys.stdout.write(json.dumps([]))
|
ARG001 Unused function argument: `args`
--> test/functional/mocks/signer.py:20:15
|
18 | sys.exit(int(mock_result[0]))
19 |
20 | def enumerate(args):
| ^^^^
21 | sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}]))
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_add_connections.py:23:26
|
22 | class P2PFeelerReceiver(P2PInterface):
23 | def on_version(self, message):
| ^^^^^^^
24 | # The bitcoind node closes feeler connections as soon as a version
25 | # message is received from the test framework. Don't send any responses
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_addr_relay.py:60:26
|
58 | assert addr.ip.startswith('123.123.')
59 |
60 | def on_getaddr(self, message):
| ^^^^^^^
61 | # When the node sends us a getaddr, it increments the addr relay tokens for the connection by 1000
62 | self._tokens += 1000
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_addr_relay.py:77:26
|
75 | return self.num_ipv4_received != 0
76 |
77 | def on_version(self, message):
| ^^^^^^^
78 | self.send_version()
79 | self.send_without_ping(msg_verack())
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_compactblocks.py:82:29
|
80 | self.last_sendcmpct.append(message)
81 |
82 | def on_cmpctblock(self, message):
| ^^^^^^^
83 | self.block_announced = True
84 | self.announced_blockhashes.add(self.last_message["cmpctblock"].header_and_shortids.header.hash_int)
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_compactblocks.py:86:26
|
84 | self.announced_blockhashes.add(self.last_message["cmpctblock"].header_and_shortids.header.hash_int)
85 |
86 | def on_headers(self, message):
| ^^^^^^^
87 | self.block_announced = True
88 | for x in self.last_message["headers"].headers:
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_compactblocks.py:91:22
|
89 | self.announced_blockhashes.add(x.hash_int)
90 |
91 | def on_inv(self, message):
| ^^^^^^^
92 | for x in self.last_message["inv"].inv:
93 | if x.type == MSG_BLOCK:
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_feefilter.py:19:28
|
17 | feefilter_received = False
18 |
19 | def on_feefilter(self, message):
| ^^^^^^^
20 | self.feefilter_received = True
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_filter.py:67:30
|
65 | self.send_without_ping(want)
66 |
67 | def on_merkleblock(self, message):
| ^^^^^^^
68 | self._merkleblock_received = True
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_filter.py:70:21
|
68 | self._merkleblock_received = True
69 |
70 | def on_tx(self, message):
| ^^^^^^^
71 | self._tx_received = True
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_leak.py:71:29
|
69 | def on_getblocktxn(self, message): self.bad_message(message)
70 | def on_blocktxn(self, message): self.bad_message(message)
71 | def on_wtxidrelay(self, message): self.got_wtxidrelay = True
| ^^^^^^^
72 | def on_sendaddrv2(self, message): self.got_sendaddrv2 = True
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_leak.py:72:29
|
70 | def on_blocktxn(self, message): self.bad_message(message)
71 | def on_wtxidrelay(self, message): self.got_wtxidrelay = True
72 | def on_sendaddrv2(self, message): self.got_sendaddrv2 = True
| ^^^^^^^
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_leak.py:85:26
|
83 | # node will give us a message that it shouldn't. This is not an exhaustive
84 | # list!
85 | def on_version(self, message):
| ^^^^^^^
86 | self.version_received = True
87 | self.send_without_ping(msg_ping())
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_sendtxrcncl.py:49:26
|
48 | class P2PFeelerReceiver(SendTxrcnclReceiver):
49 | def on_version(self, message):
| ^^^^^^^
50 | # feeler connections can not send any message other than their own version
51 | self.send_version()
|
ARG002 Unused method argument: `message`
--> test/functional/p2p_tx_privacy.py:41:26
|
39 | self.all_invs = []
40 |
41 | def on_version(self, message):
| ^^^^^^^
42 | self.send_without_ping(msg_wtxidrelay())
|
ARG002 Unused method argument: `wallet`
--> test/functional/rpc_getdescriptoractivity.py:133:45
|
131 | node.getdescriptoractivity, [invalid_blockhash], [f"addr({addr_1})"], True)
132 |
133 | def test_invalid_descriptor(self, node, wallet):
| ^^^^^^
134 | self.log.info("Test that an invalid descriptor raises the correct RPC error")
135 | blockhash = self.generate(node, 1)[0]
|
ARG002 Unused method argument: `wallet`
--> test/functional/rpc_getdescriptoractivity.py:207:37
|
205 | [blockhash_1, blockhash_2, blockhash_2], [wallet.get_descriptor()], True))
206 |
207 | def test_no_address(self, node, wallet):
| ^^^^^^
208 | self.log.info("Test that activity is still reported for scripts without an associated address")
209 | raw_wallet = MiniWallet(self.nodes[0], mode=MiniWalletMode.RAW_P2PK)
|
ARG002 Unused method argument: `node`
--> test/functional/wallet_listdescriptors.py:29:30
|
28 | # do not create any wallet by default
29 | def init_wallet(self, *, node):
| ^^^^
30 | return
|
ARG002 Unused method argument: `split`
--> test/functional/wallet_simulaterawtx.py:25:29
|
23 | self.skip_if_no_wallet()
24 |
25 | def setup_network(self, split=False):
| ^^^^^
26 | self.setup_nodes()
|
Found 44 errors.
</p> </details>
However I think all but 1 are false-positives due to callback interfaces and method overrides though, so not too useful for us...
I wonder if this tool ever found a real true positive, given that we are running it at confidence=100%, which can only find very limited things?
Ah, that explains why it cannot find unused functions like https://github.com/bitcoin/bitcoin/blob/136132e075fa79f1b74b48447c2d76734f57754b/test/functional/p2p_v2_encrypted.py#L49-L52 then I guess... I wondered about that for a while. (edit: ty foudn this one somehow in this branch, but I don't recall how now)
So as per the vulture readme at 100% we only find
But we don't find unused functions, classes, methods, variables, or imports, as those are rated 60-90% confidence.
So at --min-confidence 100 we lose the cross-file dead code detection, which is the main thing vulture offers over ruff...
We could enable unused argument rules:
I wouldn't mind enabling them, if the error message provides a clear solution like: Remove it or prefix it with an underscore (haven't tried if this works). Otherwise, it could be confusing for devs, not knowing what possible options exist.
So as per the vulture readme at 100% we only find
1. Unreachable code after return/break/continue/raise 2. Unused function arguments
I guess "unused function arguments" aren't at 100% either, because ruff ARG* finds some?
And trivially unreachable code like
return 1
return 2
Seems unlikely to be written/reviewed while passing tests. So if they still happen, it should mostly be harmless text, which is annoying, but not severe? So we may consider removing vulture?
So we may consider removing vulture?
I believe so
Ok suggested that in #34816
Enabling https://docs.astral.sh/ruff/rules/#flake8-unused-arguments-arg for ruff, or ty stuff can be done in a separate pull.