fadbcd51fc77a3f4e877851463f3c7425fb751d2 bench: Remove redundant logging benchmarks (MarcoFalke)
fa8dd952e279a87f6027ddd2e2119bf2ae2f9943 bench: Use LogInfo instead of the deprecated alias LogPrintf (MarcoFalke)
Pull request description:
`LogPrint*ThreadNames` is redundant with `LogWith(out)ThreadNames`,
because they all measure toggling the thread names (and check that it
has no effect on performance).
Fix it by removing the redundant ones. This also allows to drop a deprecated logging alias.
ACKs for top commit:
stickies-v:
ACK fadbcd51fc77a3f4e877851463f3c7425fb751d2
Tree-SHA512: 4fe137f374aa4ee1aa0e1da4a1f9839c0e52c23dbb93198ecafee98de39d311cc47304bba4191f3807aa00c51b1eae543e3f270f03d341c84910e5e341a1d475
LogPrint*ThreadNames is redundant with LogWith(out)ThreadNames, because
they all measure toggling the thread names (and check that it has no
effect on performance).
This also allows to remove unused and deprecated macros.
fafdb7df34507eee735893aa871da6ae529e6372 lint: Speed up flake8 checks (MarcoFalke)
faf17df7fb88590d936d10c471a9ea6a2ce4454d lint: Document missing py_lint dependency (MarcoFalke)
faebeb828f5f0ec68d90e7f76add66bc562f6fa3 lint: Remove python whitespace and shadowing lint rules (MarcoFalke)
77770478355ce6c1ab077dbc12ec898875ec5620 lint: Remove python lint rules that are SyntaxError (MarcoFalke)
faaf3e53f09c73278e36674db0af14a262f0bd94 test: [refactor] Fix F841 flake8 (MarcoFalke)
444421db69539b74077306b6d0cb23e82afeb891 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 fafdb7df34507eee735893aa871da6ae529e6372
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).
fa09451f8e6799682d7e7c863f25334fd1c7dce3 Add lint check for bitcoin-config.h include IWYU pragma (MarcoFalke)
dddd40ba8267dea11a3eb03d5cf8b51dbb99be5d 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 fa09451f8e6799682d7e7c863f25334fd1c7dce3
TheCharlatan:
ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3
hebasto:
re-ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3, 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
3bf4f8db669e1e274ce2633cf84add2938b9914b 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 0d6d2b650d1017691f48c9109a6cd020ab46aa73 uses `git grep --perl-regexp`.
ACKs for top commit:
hernanmarino:
cr ACK 3bf4f8db669e1e274ce2633cf84add2938b9914b
maflcko:
utACK 3bf4f8db669e1e274ce2633cf84add2938b9914b
achow101:
ACK 3bf4f8db669e1e274ce2633cf84add2938b9914b
alfonsoromanz:
Tested ACK 3bf4f8db669e1e274ce2633cf84add2938b9914b
kristapsk:
cr utACK 3bf4f8db669e1e274ce2633cf84add2938b9914b
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.
80fa7da21c470302165c47cc4a6a62fb44f997ef test: Refactor subtree exclusion in lint tests (Brandon Odiwuor)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/17413
Refactor subtree exclusion in lint tests to one place
Second attempt after PR: https://github.com/bitcoin/bitcoin/pull/24435
ACKs for top commit:
fjahr:
re-ACK 80fa7da21c470302165c47cc4a6a62fb44f997ef
maflcko:
lgtm ACK 80fa7da21c470302165c47cc4a6a62fb44f997ef
davidgumberg:
ACK 80fa7da21c
Tree-SHA512: deff7457dd19ca5ea440d3d53feae047e8863b9ddeb6494a3c94605a5d16edc91db8f99a435b4fab2ef89aedee42439562be006da647fb85bbf3def903a3ce50
e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c logging: Replace uses of LogPrintfCategory (Anthony Towns)
f7ce5ac08c669ac763e275bb7c82dcfb2b1b6c33 logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace (Anthony Towns)
fbd7642c8e5b70327e019382320f5ef0a651ecc5 logging: add -loglevelalways=1 option (Anthony Towns)
782bb6a05663ad7a53908e910d0f42b49b881e09 logging: treat BCLog::ALL like BCLog::NONE (Anthony Towns)
667ce3e3297645527b07314e1d5a82275fb25845 logging: Drop BCLog::Level::None (Anthony Towns)
ab34dc6012351e7b8aab871dd9d2b38ade1cd9bc logging: Log Info messages unconditionally (Anthony Towns)
dfe98b6874da04e45f68d17575c1e8a5431ca9bc logging: make [cat:debug] and [info] implicit (Anthony Towns)
c5c76dc615677d226c9f6b3f2b66d833315d40da 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 e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c 🌚
achow101:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
stickies-v:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
jamesob:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c ([`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.
fa01f884d3ac128f09bfa57ac2648a19a19d854e ci: Add missing COPY for ./test/lint/test_runner (MarcoFalke)
faff3e3b4604519375e122c103b156ec13eef80f 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 fa01f884d3ac128f09bfa57ac2648a19a19d854e
Tree-SHA512: c0f3110f2907d87e29c755e3b77a67dfae1f8a25833fe6ef8f2f2c58cfecf1aa46f1a20881576b62252b04930140a9e416c78b4edba0780d3c4fa7aaebabba81
a478c817b2f62b7334b36e331a2e37fe8380c754 test: replace `Callable`/`Iterable` with their `collections.abc` alternative (PEP 585) (stickies-v)
4b9afb18e6b9e16d7b299820f3a1382986a451d4 scripted-diff: use PEP 585 built-in collection types for verify-binary script (Sebastian Falbesoner)
d516cf83ed2da86dfefb395cd46f8a894907b88e 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 a478c817b2f62b7334b36e331a2e37fe8380c754
fanquake:
ACK a478c817b2f62b7334b36e331a2e37fe8380c754
Tree-SHA512: 6948c905f6abd644d84f09fcb3661d7edb2742e8f2b28560008697d251d77a61a1146ab4b070e65b0d27acede7a5256703da7bf6eb1c7c3a897755478c76c6e8
43de4d3630274e1287179c86896ed4c2d8b9eff4 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 43de4d3630274e1287179c86896ed4c2d8b9eff4
Tree-SHA512: c032fe86cb49c924a468385653b31f309a9db68c478d70335bba3e65a1ff3826abe80284fe00a090ab5a509e1edbf17e476f6922fb15d055e50f1103dad2ccb0
