Updates the cmake logic to generate a separate test for each
BOOST_FIXTURE_TEST_SUITE declaration in a file, and splits coins_tests.cpp
into three separate suites so that they can be run in parallel. Also
updates the convention enforced by test/lint/lint-tests.py.
fac00d4ed361e5e8c8989b2bb5a4a22dd54e2c72 doc: Move CI-must-pass requirement into readme section (MarcoFalke)
fab79c1a250db252baa206f59d7e46986e21a57c doc: Clarify and move "hygienic commit" note (MarcoFalke)
fac8b051979911f036f9156b2d4e7afbe2853336 doc: Clarify strprintf size specifier note (MarcoFalke)
faaf34ad7253e3d347af986305e405e6ef35f459 doc: Remove section about RPC alias via function pointer (MarcoFalke)
2222d61e1ce5c9efd099f33b5dda934bc9d2d57f doc: Remove section about RPC arg names in table (MarcoFalke)
fa00b8c02c9de6dca833b83dc6447ba31d72ca57 doc: Remove section about include guards (MarcoFalke)
fad6cd739b638cf7e8c86cdccf98df070e64a0f9 doc: Remove dev note section on includes (MarcoFalke)
fa6623d85af192c8aa1d0380d4a5452e0779c64e doc: Remove file name section (MarcoFalke)
7777fb8bc749e18c178ef460b65219187e676128 doc: Remove shebang section (MarcoFalke)
faf65f05312be7647f485f088ba00fef97f47bf4 doc: Remove .gitignore section (MarcoFalke)
faf2094f2511a5322d68d2352f244b5bc89d5fee doc: Remove note about removed ParsePrechecks (MarcoFalke)
fa69c5b170f56d554fcb0d0887bd27f961fe3e74 doc: Remove -disablewallet from dev notes (MarcoFalke)
Pull request description:
This removes sections that I've been collecting as stale or overly redundant over the years. The rationale for each removal is in the commit message.
ACKs for top commit:
yuvicc:
ACK fac00d4ed361e5e8c8989b2bb5a4a22dd54e2c72
janb84:
LGTM ACK fac00d4ed361e5e8c8989b2bb5a4a22dd54e2c72
glozow:
ACK fac00d4ed361e5e8c8989b2bb5a4a22dd54e2c72, all lgtm
Tree-SHA512: 17a5b4277fb30d265959d1230a705b36d8501a64c0f4a7f272ea5d9c22031421f95c491144f6d6f714dc7927df667d96ece9ceb43e0a07317d76fdcc4769aaa7
800b7cc42ca63f2a6b245a4d327c7092289da6e1 cmake: Add missed `SSE41_CXXFLAGS` (Hennadii Stepanov)
028476e71fdcca5ea780fcfecfa7cf06c323beab cmake: Remove `ENABLE_ARM_SHANI` from `bitcoin-build-config.h` (Hennadii Stepanov)
1e900528d2450177a50eed15b36d2e6c226c2f4e cmake: Remove `ENABLE_X86_SHANI` from `bitcoin-build-config.h` (Hennadii Stepanov)
8689628e2e36d70aed16f4c44d6503888600ab90 cmake: Remove `ENABLE_AVX2` from `bitcoin-build-config.h` (Hennadii Stepanov)
a8e2342dca5e9cd771d883d4906cce6d2bb3de69 cmake: Remove `ENABLE_SSE41` from `bitcoin-build-config.h` (Hennadii Stepanov)
Pull request description:
`ENABLE_{SSE41,AVX2,X86_SHANI,ARM_SHANI}` are already conditionally defined for the [`bitcoin_crypto`](https://github.com/bitcoin/bitcoin/blob/master/src/crypto/CMakeLists.txt) target, and they are not used by any other targets. Defining them globally in `bitcoin-build-config.h` is therefore redundant.
Additionally, the previously missing `SSE41_CXXFLAGS` variable has been [added](https://github.com/bitcoin/bitcoin/pull/32550#issuecomment-2890918551).
ACKs for top commit:
fanquake:
ACK 800b7cc42ca63f2a6b245a4d327c7092289da6e1
Tree-SHA512: da792a0b780c67b432b09c9288ca98d62545315c721fed13510d1c11f8bb0cddd9a4ed7a009b4d052471dda19d0641bbc1eae4805fc306d23bf9b4ef510089c8
This is already checked by test/lint/lint-files.py
There is no need to reword all linters into the dev notes.
Also, allow scripts in Rust (there are already some).
fa24fdcb7f474e6959ae4d8fe9759d365c6c021b lint: Remove string exclusion from locale check (MarcoFalke)
Pull request description:
The exclusion isn't needed. In fact, it prevents detection of `"bla" + wrong()`.
For example, the following is not detected:
```diff
diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index 1c2951deee..c1209013e5 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -336,7 +336,8 @@ RPCHelpMan addmultisigaddress()
RPCHelpMan keypoolrefill()
{
return RPCHelpMan{"keypoolrefill",
- "\nFills the keypool."+
+ "\nRefills each descriptor keypool in the wallet up to the specified number of new keys.\n"
+ "By default, descriptor wallets have 4 active ranged descriptors (\"legacy\", \"p2sh-segwit\", \"bech32\", and \"bech32m\"), each with " + std::to_string(DEFAULT_KEYPOOL_SIZE) + " entries.\n" +
HELP_REQUIRING_PASSPHRASE,
{
{"newsize", RPCArg::Type::NUM, RPCArg::DefaultHint{strprintf("%u, or as set by -keypool", DEFAULT_KEYPOOL_SIZE)}, "The new keypool size"},
```
Fix the script by detecting it.
ACKs for top commit:
laanwj:
Code review ACK fa24fdcb7f474e6959ae4d8fe9759d365c6c021b.
rkrux:
ACK fa24fdcb7f474e6959ae4d8fe9759d365c6c021b
w0xlt:
ACK fa24fdcb7f
Tree-SHA512: cb7e6ed9fec5d2089e94031329ebf26b83a1814ffbbbca94f7527c127bc759d13c0f4ea79b71ff7f5f009d071dcf01958c8921163d6dc5e1ae6256cc40b57eea
The string exclusion would fail to detect `"bla" + wrong()`.
Also, remove /* */ comment exclusion, which would fail to detect stuff
like `/* bla */ wrong()`.
Instead, require the function to be called by adding \\( to the regex.
Finally, also remove the section in the dev notes, because:
* It was outdated and missing some functions such as std::to_string in
the list.
* The maintenance overhead of having to update two places is fragile and
questionable.
* Many other linters are also not mentioned in the dev notes, even
though they are important.
* A dev (and CI) is more likely to run the linters than to read the dev
notes.
* The dev notes are more than 1000 lines of dense information. It would
be easier to digest if they focused on the important stuff that is not
checked by automated tools.
e3014017bacff42d8d69f3061ce1ee621aaa450a test: add IsActiveAfter tests for versionbits (Anthony Towns)
60950f77c35e54e2884cfc14ab67623f3e325099 versionbits: docstrings for BIP9Info (Anthony Towns)
7565563bc7a5bb98ebf03a7d6881912a74d3f302 tests: refactor versionbits fuzz test (Anthony Towns)
2e4e9b9608c722aaf767638e9dba498d8dc3e772 tests: refactor versionbits unit test (Anthony Towns)
525c00f91bb27d0f2a1b2e5532aebec7fac97d3a versionbits: Expose VersionBitsConditionChecker via impl header (Anthony Towns)
e74a7049b477d1853191ded75fdf25024a6e233f versionbits: Expose StateName function (Anthony Towns)
d00d1ed52c8ee95eeed665d68d6715a694bd4c1f versionbits: Split out internal details into impl header (Anthony Towns)
37b9b67a39554465104c9cf1a74690f40019dbad versionbits: Simplify VersionBitsCache API (Anthony Towns)
1198e7d2fd665bf2bc49fd26773d4fd5fbc2b716 versionbits: Move BIP9 status logic for getblocktemplate to versionbits (Anthony Towns)
b1e967c3ec92738affb22d3b58483ebcdd8dfea2 versionbits: Move getdeploymentinfo logic to versionbits (Anthony Towns)
3bd32c20550e69688a4ff02409fb34b9a637b9c4 versionbits: Move WarningBits logic from validation to versionbits (Anthony Towns)
5da119e5d0e61f0b583f0fe21b9a00ee815a3e46 versionbits: Change BIP9Stats to uint32_t types (Anthony Towns)
a679040ec19ef17f3f03988a52207f1c03af701e consensus/params: Move version bits period/threshold to bip9 param (Anthony Towns)
e9d617095d4ce9525a4337d33624cac9d6b4abe6 versionbits: Remove params from AbstractThresholdConditionChecker (Anthony Towns)
9bc41f1b48b2e0cc6abf9714e860a29989d7809c versionbits: Use std::array instead of C-style arrays (Anthony Towns)
Pull request description:
Increases the encapsulation/modularity of the versionbits code, moving more of the logic into the versionbits module rather than having it scattered across validation and rpc code. Updates unit/fuzz tests to test the actual code used rather than just a close approximation of it.
ACKs for top commit:
achow101:
ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
TheCharlatan:
Re-ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
darosior:
ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
Tree-SHA512: 2978db5038354b56fa1dd6aafd511099e9c16504d6a88daeac2ff2702c87bcf3e55a32e2f0a7697e3de76963b68b9d5ede7976ee007e45862fa306911194496d
Without this change linter produces errors about:
- Use of std::filesystem the libmultiprocess example program.
- Use of locale-dependent functions in example program, in the build time code
generator, and in the runtime library for debug logging.
- Include guards not beginning with BITCOIN_
faf8fc5487d409eeff7b7b260eabb6929a7b7a5f lint: Call lint_commit_msg from test_runner (MarcoFalke)
fa99728b0c8b3cac7056fa554fab7a8a4624a2de lint: Move commit range printing to test_runner (MarcoFalke)
fa673cf3449f4e71501814bf99c2e2bbb49b8fcb lint: Call lint_scripted_diff from test_runner (MarcoFalke)
Pull request description:
The lint `commit-script-check.sh` can not be called from the test_runner at all and must be called manually. Also, some checks require `COMMIT_RANGE` to be set.
Fix all issues by moving two lint checks into the test_runner. Also, the proper commit range is passed to the checks by the test_runner, so that the user no longer has to do it.
ACKs for top commit:
kevkevinpal:
reACK [faf8fc5](faf8fc5487)
willcl-ark:
tACK faf8fc5487d409eeff7b7b260eabb6929a7b7a5f
Tree-SHA512: 78018adc618d997508c226c9eee0a4fada3899cdfd91587132ab1c0389aea69127bafc3a900e90e30aca2c6bae9dcd6e6188ef287e91413bc63ee66fb078b1af
Allowing to call the check from the test_runner allows for consistent
error messages and better UX by having a single test_runner for all
checks.
This requires the env var to be set for now. The next commit makes the
commit range optional.
The linter has many implementation bugs and missing features.
Also, it is completely redundant with FormatStringCheck, which
constructs from ConstevalFormatString or a runtime format string.
- No empty line separating errors and arrows ("^^^"). Keeping them together signals they are related.
- No empty line separating error message and linter failure line (not completely empty, it contains several spaces left over from Rust multi-line literal).
- Keep the linter description on the same line as the failure line, otherwise it looks like it's a description for the following step.
On failure, this makes the output more consistent with the other linter.
Each failure will be marked with an '⚠️ ' emoji and explanation, making
it easier to spot.
Also, add --line-number to the filesystem linter.
Also, add newlines after each failing check, to visually separate
different failures from each other.
Can be reviewed with:
"--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space"
c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1 test: Add missing %c character test (Hodlinator)
76cca4aa6fcd363dee821c837b1b627ea137b7a4 test: Document non-parity between tinyformat and ConstevalFormatstring (Hodlinator)
533013cba20664e3581a8e4633cc188d5be3175a test: Prove+document ConstevalFormatString/tinyformat parity (Hodlinator)
b81a4659950a6c4e22316f66b55cae8afc4f4d9a refactor test: Profit from using namespace + using detail function (Hodlinator)
Pull request description:
Clarifies and puts the extent of parity under test.
Broken out from #30546 based on https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263 and https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304.
ACKs for top commit:
maflcko:
re-ACK c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1 🗜
l0rinc:
ACK c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1
ryanofsky:
Code review ACK c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1. Just a few cleanups tweaking function declarations and commit comments and consolidating some test cases since last review.
Tree-SHA512: 5ecc893b26cf2761c0009861be392ec4c4fceb0ef95052a2f6f9df76b2e459cfb3f9e257f61be07c3bb2ecc6e525e72c5ca853be1f63b70b52785323d3db6b42
- For "%n", which is supposed to write to the argument for printf.
- For string/integer mismatches of width/precision specifiers.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
fae76393bdbf867176e65447799d6ee3d3567b18 test: Avoid F541 (f-string without any placeholders) (MarcoFalke)
Pull request description:
An extra `f` string-prefix is mostly harmless, but could be confusing or hint to a mistake where a format argument was forgotten.
Try to avoid the confusion and mistakes by applying the `F541` linter rule.
ACKs for top commit:
lucasbalieiro:
**Tested ACK** [fae7639](fae76393bd)
danielabrozzoni:
ACK fae76393bdbf867176e65447799d6ee3d3567b18
tdb3:
Code review ACK fae76393bdbf867176e65447799d6ee3d3567b18
Tree-SHA512: 4992a74fcf0c19b32e4d95f7333e087b4269b5c5259c556789fb86721617db81c7a4fe210ae136c92824976f07f71ad0f374655e7008b1967c02c73324862d9a
cccca8a77f3c1b22fb0ea825ca92aebd63b16d77 test: Avoid logging error when logging error (MarcoFalke)
Pull request description:
Currently a logging error in the form of `--- Logging error ---` happens when an error is logged in the `_on_data` helper.
Fix it by properly logging the error.
Also, treat pylint errors as errors, to avoid this problem in the future.
Can be tested by running `p2p_addrv2_relay.py` with the following example diff:
```diff
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index 523e1bd068..0f1eb29d13 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -137,7 +137,7 @@ MESSAGEMAP = {
b"notfound": msg_notfound,
b"ping": msg_ping,
b"pong": msg_pong,
- b"sendaddrv2": msg_sendaddrv2,
+ #b"sendaddrv2": msg_sendaddrv2,
b"sendcmpct": msg_sendcmpct,
b"sendheaders": msg_sendheaders,
b"sendtxrcncl": msg_sendtxrcncl,
ACKs for top commit:
fanquake:
ACK cccca8a77f3c1b22fb0ea825ca92aebd63b16d77
Tree-SHA512: dd19f3feed0093246cb205903529fb9ebd5ad9a6c9330cfc5987c0154253c9dcec8d0e25ff99e4ac806a464ff58c3787a205378b8dfb7a1a521da25eac429136
Fixes: #31044
This MLC update includes a change which will ignore files being ignored
by git, and help avoid false-positives when linting in this repo.
fa3e074304780549b1e7972217930e34fa55f59a refactor: Tidy fixups (MarcoFalke)
fa72646f2b197810a324cb0544d9a1fac37d3f9c move-only: Detail_CheckNumFormatSpecifiers and G_TRANSLATION_FUN (MarcoFalke)
faff8403f0aac3b5ec26d3c7fc98240f879f9906 refactor: Pick translated string after format (MarcoFalke)
Pull request description:
The changes are required for https://github.com/bitcoin/bitcoin/pull/31061, however they also make sense on their own. For example, they are fixing up an `inline namespace`, which lead to compile errors otherwise (can be tested by observing the compile error after reverting the changes to `src/util/strencodings.h`). Also, a unit test comment is fixed.
ACKs for top commit:
ryanofsky:
Code review ACK fa3e074304780549b1e7972217930e34fa55f59a. Nice changes! These should allow related PRs to be simpler.
l0rinc:
ACK fa3e074304780549b1e7972217930e34fa55f59a
hodlinator:
cr-ACK fa3e074304780549b1e7972217930e34fa55f59a
Tree-SHA512: 37371181a348610442186b5fbb7a6032d0caf70aae566002ad60be329a3131a2b89f28f6c51e10872079f987986925dc8c0611bde639057bee4f572d2b9ba92a
33a28e252a7349c0aa284005aee97873b965fcfe Change default help arg to `-help` and mention `-h` and `-?` as alternatives (Lőrinc)
f0130ab1a1e65583637b6a362b879ea3253e7bb7 doc: replace `-?` with `-h` for bench_bitcoin help (Lőrinc)
Pull request description:
The question mark is interpreted as a wildcard for any single character in Zsh (see https://www.techrepublic.com/article/globbing-wildcard-characters-with-zsh), so `bench_bitcoin -?` will not show the help message on systems using Zsh, such as macOS.
Since `-h` provides equivalent help functionality (as defined in https://github.com/bitcoin/bitcoin/blob/master/src/common/args.cpp#L684-L693), the `benchmarking.md` documentation has been updated to ensure compatibility with macOS.
----
### -?
> % cmake -B build -DBUILD_BENCH=ON && cmake --build build && build/src/bench/bench_bitcoin -?
zsh: no matches found: -?
### -h
> % cmake -B build -DBUILD_BENCH=ON && cmake --build build && build/src/bench/bench_bitcoin -h
Usage: bench_bitcoin [options]
Options:
...
----
Based on the comments the args help default was also changed to `-help`, mentioning `-h` and `-?` (instead of `-?` being the default)
ACKs for top commit:
edilmedeiros:
tACK 33a28e252a7349c0aa284005aee97873b965fcfe
maflcko:
lgtm ACK 33a28e252a7349c0aa284005aee97873b965fcfe
achow101:
ACK 33a28e252a7349c0aa284005aee97873b965fcfe
rkrux:
tACK 33a28e252a
laanwj:
Code review ACK 33a28e252a7349c0aa284005aee97873b965fcfe
Tree-SHA512: 8c6e27488462be9ba9186b34abe6249c1d93026b3963acc0f42c75496f39407563766ae518cf1839156039cc0047e29d91f70d191cfb97e0fbde85665e88c71e
fac6cfe5ac06547c90da6f976d7c8bed20da8bac lint: commit-script-check.sh: echo to stderr (MarcoFalke)
Pull request description:
This makes it easier to redirect the produced `git diff` on failure. On success, it shouldn't hurt, because the same output is still present, just on stderr.
Can be tested by introducing a fault in any scripted diff and then calling `commit-script-check.sh HEAD~..HEAD > any_file.txt`. Previously the file contained the full output, now it contains just the diff.
ACKs for top commit:
TheCharlatan:
ACK fac6cfe5ac06547c90da6f976d7c8bed20da8bac
Tree-SHA512: b4dfad10a4a902729a7ad7533ed0ef86b9e79761083f2ec623d448a551462b268fe04bdba387ca62160dae9ef7b1781e005dec60f18b111d9bfa6b97357108e6
