Re-add dead code detection

[flack]

This re-adds unreachable code detection for Python based on vulture.

Effectively, this reverts f4beb49. The difference to the previous version is that this runs with the --min-confidence 100 setting. From https://pypi.org/project/vulture/:

Use --min-confidence 100 to only report code that is guaranteed to be unused within the analyzed files.

So this should avoid the previous issues where static analysis had wrong positives due to the dynamic nature of Python code by only reporting things that are unambiguous (such as code after a return statement). As such, there is not suppressions list.

My motivation was mainly #21081 which would have been caught by this (as can be seen by the CI run failing). This is still marked as draft because #21081 is needed to get the linter to pass. Also, there is a second problem that this found (see https://github.com/bitcoin/bitcoin/pull/19509/files#r571454691). From what I can tell, this is a spurious type comment that could just be removed (or if that line has no side effects it could also be deleted altogether?). I could add a commit here to fix it, but I wanted to see if there is interest in having this linter again in the first place

[flack]

meh, turns out clicked the wrong button, and now I can't mark it as Draft anymore... modified the title instead

[flack]

#21081 has been merged and the other issue I mentioned has been addressed in #21107, so this should now be ready for review. I also bumped the vulture dependency to 2.3 since it turns out Python 3.5 support is no longer needed

[maflcko]

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

[flack]

@MarcoFalke done

[fanquake]

Seems like at --min-confidence=100 vulture will just report unreachable code.
You have to drop it to 90 for it to report unused imports:

diff --git a/test/functional/test_framework/key.py b/test/functional/test_framework/key.py
index e0cbab45c..d0f3472e9 100644
--- a/test/functional/test_framework/key.py
+++ b/test/functional/test_framework/key.py
@@ -14,6 +14,10 @@ import unittest
 
 from .util import modinv
 
+import nonsense
+
 def TaggedHash(tag, data):
     ss = hashlib.sha256(tag.encode('utf-8')).digest()
     ss += ss

bitcoin/test/functional/test_framework/key.py:17: unused import 'nonsense' (90% confidence)

or 60 before it will report unused functions, i.e xor_bytes in #21100:

bitcoin/test/functional/test_framework/key.py:23: unused function 'xor_bytes' (60% confidence)

Looking at the vulture source this makes sense, because the default confidence for basically all checks is 60.

[laanwj]

NACK on this. I still stand by discussion in #16961. Even having to fine-tune the threshold value just seems a recipe for endless frustration.

[flack]

@laanwj I'm not going to spend too much time arguing, but the rationale for #16961 was to avoid false positives. --min-confidence=100 accomplished that, since it is guaranteed to only find dead code (inside of if (false), after return statement or similar). So there isn't any need to fine-tune anything. This is just a little sanity check that even on my five year old laptop completes in less than one second, and it would have caught an instance of xkcd 221 in the taproot tests, which is probably one of the better-reviewed PRs. What's not to like?

[practicalswift]

Concept ACK

If simply specifying --min-confidence 100 means that the false positives we've seen in the past are avoided then this should be an obvious win, no? :)

@laanwj, is your NACK made assuming that --min-confidence 100 is not sufficient to solve the false positive issues we've seen in the past? Not questioning: just trying to understand the reasoning :)

[laanwj]

Okay retracted the NACK if everyone else wants this it's okay with me I just would really prefer if it doesn't come up as issue again.

[practicalswift]

Should this be $(vulture --min-confidence 100 -- $(git ls-files -- "*.py")) instead to make sure only files in the repo are checked?

[flack]

Could be changed if people think that's better. The rev-parse version is what the previous incarnation of the linter had. I was thinking maybe you want to run the linter against a new file before you git add it?

AFAICT, vulture automatically filters by file extension (https://github.com/jendrikseipp/vulture/blob/master/vulture/config.py#L105), so *.py should be implicit

[laanwj]

don't think this is a serious issue but, ostensibly a developer could, unwisely, keep their own .py scripts inside the repo directory, which would make no sense to check here, so i think ls-files to only check the committed ones makes sense

[flack]

Alright, changed to ls-files as requested

[practicalswift]

ACK 3f8776a

[maflcko]

Tested ACK by reintroducing the bug, didn't review:

/tmp/cirrus-ci-build/test/functional/feature_taproot.py:521: unreachable code after 'return' (100% confidence)
^---- failure generated from test/lint/lint-python-dead-code.sh