fafdb7df34 lint: Speed up flake8 checks (MarcoFalke)
faf17df7fb lint: Document missing py_lint dependency (MarcoFalke)
faebeb828f lint: Remove python whitespace and shadowing lint rules (MarcoFalke)
7777047835 lint: Remove python lint rules that are SyntaxError (MarcoFalke)
faaf3e53f0 test: [refactor] Fix F841 flake8 (MarcoFalke)
444421db69 test: [refactor] Fix E714 pycodestyle (MarcoFalke)
Pull request description:
The checks have many issues:
* Some checks that could in theory hide bugs are not applied -> Fix them and apply them going forward
* Some checks are redundant Python 2 checks, or of low value -> Remove them
* The checks are slow -> Speed them up from ~10 seconds to about ~20 milliseconds
ACKs for top commit:
davidgumberg:
review and tested reACK fafdb7df34
kevkevinpal:
ACK [fafdb7d](fafdb7df34)
achow101:
ACK fafdb7df34
Tree-SHA512: a0488b722cfaf7071bd6848cd3be002e0b6c38af80d8b5cbb08613c0b174ef63277289f960db8ac31adb09fe563a4973203b8fb10b83cbcfdc6f0ef39bd04410
Previously they may have taken more than 10 seconds. Now they should
finish in less than one second.
This also allows to drop one dependency to be installed.
The rules have many issues:
* Most are redundant, because Python already has a built-in
IndentationError, a subclass of SyntaxError, to enforce whitespace.
* They are not enforced consistently anyway, see for examples [1][2]
below.
* They are stylistic rules where the author intentionally formatted the
code to be easier to read. Starting to enforce them now would make the
code harder to read and create frustration in the future.
Fix all issues by removing them.
[1]:
test/functional/feature_cltv.py:63:35: E272 [*] Multiple spaces before keyword
|
61 | # | Script to prepend to scriptSig | nSequence | nLockTime |
62 | # +-------------------------------------------------+------------+--------------+
63 | [[OP_CHECKLOCKTIMEVERIFY], None, None],
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E272
[2]:
contrib/asmap/asmap.py:395:13: E306 [*] Expected 1 blank line before a nested definition, found 0
|
393 | prefix.pop()
394 | hole = not fill and (lhole or rhole)
395 | def candidate(ctx: Optional[int], res0: Optional[list[ASNEntry]],
| ^^^ E306
Any kind of syntax error is already reported, so there is no need to
enumerate all possible types of syntax errors of ancient versions of
Python 2 or 3.
Updating to MLC v0.18.0 includes a new feature which will ignore all
files ignored by git: `--gitignore`.
This helps avoid false-positives flagged by this linter in non-project
files, such as a developer might expect to have in their directory (e.g.
guix-builds, python venvs, etc.)
This adds a markdown hyperlink check task to the lint test_runner. It
relies on having the [`mlc`](https://crates.io/crates/mlc) binary found
on $PATH, but will fail with `success` if the binary is not found.
`mlc` is also added to the ci/04_install.sh script run by the
containerfile.
Note that broken markdown hyperlinks will be detected in untracked
markdown files found in a dirty working directory (including e.g.
.venv).
fa09451f8e Add lint check for bitcoin-config.h include IWYU pragma (MarcoFalke)
dddd40ba82 scripted-diff: Add IWYU pragma keep to bitcoin-config.h includes (MarcoFalke)
Pull request description:
The `bitcoin-config.h` includes have issues:
* The header is incompatible with iwyu, because symbols may be defined or not defined. So the `IWYU pragma: keep` is needed to keep the include when a symbol is not defined on a platform. Compare the previous discussion in https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948959711
* Guarding the includes by `HAVE_CONFIG_H` is verbose and brittle. Now that all build config dependencies have been removed from low level headers, the benefits are questionable, and the guard can be removed. The linter could also be tricked by guarding the include by `#if defined(HAVE_C0NFIG_H)` (`O` replaced by `0`). Compare the previous discussion in https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1483189853 .
ACKs for top commit:
achow101:
ACK fa09451f8e
TheCharlatan:
ACK fa09451f8e
hebasto:
re-ACK fa09451f8e, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-2028864535) (`timedata.cpp` removed in https://github.com/bitcoin/bitcoin/pull/29623).
Tree-SHA512: 47cb973f7f24bc625acc4e78683371863675d186780236d55d886cf4130e05a78bb04f1d731aae7088313b8e963a9677cc77cf518187dbd99d776f6421ca9b52
3bf4f8db66 lint: scripted-diff verification also requires GNU grep (Sjors Provoost)
Pull request description:
I noticed while trying to verify all historical `scripted-diff:` commits on macOS that some scripts require GNU sed.
For example 0d6d2b650d uses `git grep --perl-regexp`.
ACKs for top commit:
hernanmarino:
cr ACK 3bf4f8db66
maflcko:
utACK 3bf4f8db66
achow101:
ACK 3bf4f8db66
alfonsoromanz:
Tested ACK 3bf4f8db66
kristapsk:
cr utACK 3bf4f8db66
Tree-SHA512: 09a060ab1bafad03df60d0f20c3dd1451850868dbd66ea38b18178b6230c1f06cf48622db82d9c51422d5689962ee0cd7aae0a31f84bd6d878215e6d73c1d47e
Upstream repo: https://github.com/arun11299/cpp-subprocess
Commit: 4025693decacaceb9420efedbf4967a04cb028e7
The "Convenience Functions" section is unused in our codebase, so it has
been removed.
e60fc7d5d3 logging: Replace uses of LogPrintfCategory (Anthony Towns)
f7ce5ac08c logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace (Anthony Towns)
fbd7642c8e logging: add -loglevelalways=1 option (Anthony Towns)
782bb6a056 logging: treat BCLog::ALL like BCLog::NONE (Anthony Towns)
667ce3e329 logging: Drop BCLog::Level::None (Anthony Towns)
ab34dc6012 logging: Log Info messages unconditionally (Anthony Towns)
dfe98b6874 logging: make [cat:debug] and [info] implicit (Anthony Towns)
c5c76dc615 logging: refactor: pull prefix code out (Anthony Towns)
Pull request description:
Replace `LogPrint*` functions with severity based logging functions:
* `LogInfo(...)`, `LogWarning(...)`, `LogError(...)` for unconditional (uncategorised) logging (replaces `LogPrintf`)
* `LogDebug(CATEGORY, ...)` and `LogTrace(CATEGORY, ...)` for conditional logging (replaces `LogPrint`)
* `LogPrintLevel(CATEGORY, LEVEL, ...)` for when the level isn't known in advance, or a category needs to be added for an info/warning/error log message (mostly unchanged, but rarely needed)
Logs look roughly as they do now with `LogInfo` not having an `[info]` prefix, and `LogDebug` having a `[cat]` prefix, rather than a `[cat:debug]` prefix. This removes `BCLog::Level::None` entirely -- for `LogFlags::NONE` just use `Level::Info`, for any actual category, use `Level::Debug`.
Adds docs to developer-notes about when to use which level.
Adds `-loglevelalways=1` option so that you get `[net:debug]`, `[all:info]`, `[all:warning]` etc, which might be helpful for automated parsing, or just if you like everything to be consistent. Defaults to off to reduce noise in the default config, and to avoid unnecessary changes on upgrades.
Changes the behaviour of `LogPrintLevel(CATEGORY, BCLog::Level::Info, ...)` to be logged unconditionally, rather than only being an additional optional logging level in addition to trace and debug. Does not change the behaviour of `LogPrintLevel(NONE, Debug, ...)` and `LogPrintLevel(NONE, Trace, ...)` being no-ops.
ACKs for top commit:
maflcko:
re-ACK e60fc7d5d3🌚
achow101:
ACK e60fc7d5d3
stickies-v:
ACK e60fc7d5d3
jamesob:
ACK e60fc7d5d3 ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))
Tree-SHA512: e7a4588779b148242495b7b6f64198a00c314cd57100affab11c43e9d39c9bbf85118ee2002792087fdcffdea08c84576e20844b3079f27083e26ddd7ca15d7f
These provide simple and clear ways to write the most common logging
operations:
LogInfo("msg");
LogDebug(BCLog::LogFlags::NET, "msg");
LogError("msg");
LogWarning("msg");
LogTrace(BCLog::LogFlags::NET, "msg");
For cases where the level cannot be hardcoded, LogPrintLevel(category,
level, ...) remains available.
fa01f884d3 ci: Add missing COPY for ./test/lint/test_runner (MarcoFalke)
faff3e3b46 lint: Report all lint errors instead of early exit (MarcoFalke)
Pull request description:
`all-lint.py` currently collects all failures. However, the `06_script.sh` does not, since July this year (https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268115806).
Fix this by printing all failures before exiting.
Can be tested by modifying (for example) two subtrees in the same commit and then running the linters.
ACKs for top commit:
kevkevinpal:
ACK [fa01f88](fa01f884d3)
TheCharlatan:
lgtm ACK fa01f884d3
Tree-SHA512: c0f3110f2907d87e29c755e3b77a67dfae1f8a25833fe6ef8f2f2c58cfecf1aa46f1a20881576b62252b04930140a9e416c78b4edba0780d3c4fa7aaebabba81
a478c817b2 test: replace `Callable`/`Iterable` with their `collections.abc` alternative (PEP 585) (stickies-v)
4b9afb18e6 scripted-diff: use PEP 585 built-in collection types for verify-binary script (Sebastian Falbesoner)
d516cf83ed test: use built-in collection types for type hints (Python 3.9 / PEP 585) (Sebastian Falbesoner)
Pull request description:
With Python 3.9 / [PEP 585](https://peps.python.org/pep-0585/), [type hinting has become a little less awkward](https://docs.python.org/3.9/whatsnew/3.9.html#type-hinting-generics-in-standard-collections), as for collection types one doesn't need to import the corresponding capitalized types (`Dict`, `List`, `Set`, `Tuple`, ...) anymore, but can use the built-in types directly (see https://peps.python.org/pep-0585/#implementation for the full list).
This PR applies the replacement for all Python scripts (i.e. in the contrib and test folders) for the basic types, i.e.:
- typing.Dict -> dict
- typing.List -> list
- typing.Set -> set
- typing.Tuple -> tuple
For an additional check, I ran mypy 1.6.1 on both master and the PR branch via
```
$ mypy --ignore-missing-imports --explicit-package-bases $(git ls-files "*.py")
```
and verified that the output is identical -- (from the 22 identified problems, most look like false-positives, it's probably worth it to go deeper here and address them in a follow-up though).
ACKs for top commit:
stickies-v:
ACK a478c817b2
fanquake:
ACK a478c817b2
Tree-SHA512: 6948c905f6abd644d84f09fcb3661d7edb2742e8f2b28560008697d251d77a61a1146ab4b070e65b0d27acede7a5256703da7bf6eb1c7c3a897755478c76c6e8
43de4d3630 doc: fix typos (Sjors Provoost)
Pull request description:
This PR fixes typos found by lint-spelling.py using codespell 2.2.6.
Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now.
ACKs for top commit:
pablomartin4btc:
re ACK 43de4d3630
Tree-SHA512: c032fe86cb49c924a468385653b31f309a9db68c478d70335bba3e65a1ff3826abe80284fe00a090ab5a509e1edbf17e476f6922fb15d055e50f1103dad2ccb0
bbbbdb0cd5 ci: Add filesystem lint check (MarcoFalke)
fada2f9110 refactor: Replace <filesystem> with <util/fs.h> (MarcoFalke)
Pull request description:
Using `std::filesystem` is problematic:
* There is a `fs` namespace wrapper for it. So having two ways to achieve the same is confusing.
* Not using the `fs` wrapper is dangerous and buggy, because it disables known bugs by deleting problematic functions.
Fix all issues by removing use of it and adding a linter to avoid using it again in the future.
ACKs for top commit:
TheCharlatan:
ACK bbbbdb0cd5
fanquake:
ACK bbbbdb0cd5🦀
Tree-SHA512: 0e2d49742b08eb2635e6fce41485277cb9c40fe20b81017c391d3472a43787db1278a236825714ca1e41c9d2f59913865cfb0c649e3c8ab1fb598c849f80c660
The `-zapwallettxes` functionality has been removed in v0.21.0
(see commit 3340dbadd3 / PR #19671),
with the parameter being kept as hidden option, to inform users via
an exit error that `abandontransaction` should be used instead.
As any guides that still suggest to use `-zapwallettxes` would refer to
a Bitcoin Core version that is EOL since many years (i.e. <= v0.20.x),
it is highly unlikely that the error caused by the option is still
relevant for any user, hence it seems fine to remove it now.
d9cc99d04e [test] MiniMiner::Linearize and manual construction (glozow)
dfd6a3788c [refactor] unify fee amounts in miniminer_tests (glozow)
f4b1b24a3b [MiniMiner] track inclusion order and add Linearize() function (glozow)
004075963f [test] add case for MiniMiner working with negative fee txns (glozow)
fe6332c0ba [MiniMiner] make target_feerate optional (glozow)
5a83f55c96 [MiniMiner] allow manual construction with non-mempool txns (glozow)
e3b2e630b2 [refactor] change MiniMinerMempoolEntry ctor to take values, update includes (glozow)
4aa98b79b2 [lint] update expected boost includes (glozow)
Pull request description:
This is part of #27463. It splits off the `MiniMiner`-specific changes from #26711 for ease of review, as suggested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253.
- Allow using `MiniMiner` on transactions that aren't in the mempool.
- Make `target_feerate` param of `BuildMockTemplate` optional, meaning "don't stop building the template until all the transactions have been selected."
- Add clarification for how this is different from `target_feerate=0` (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1377019133)
- Track the order in which transactions are included in the template to get the "linearization order" of the transactions.
- Tests
Reviewers can take a look at #26711 to see how these functions are used to linearize the `AncestorPackage` there.
ACKs for top commit:
TheCharlatan:
ACK d9cc99d04e
kevkevinpal:
reACK [d9cc99d](d9cc99d04e)
achow101:
re-ACK d9cc99d04e
Tree-SHA512: 32b80064b6679536ac573d674825c5ca0cd6245e49c2fd5eaf260dc535335a57683c74ddd7ce1f249b5b12b2683de4362a7b0f1fc0814c3b3b9f14c682665583
