fa58ae74ea67485495dbc2d003adfbca68d6869b refactor: Add missing include for USE_BDB, USE_SQLITE to bench/wallet_ismine.cpp (MarcoFalke)
fa31908ea848488ff842f1b9fce6235bb8855ec7 lint: Check for missing or redundant bitcoin-config.h includes (MarcoFalke)
fa63b0e351dee4782ee19ad46603957ef8d020eb lint: Make lint error easier to spot in output (MarcoFalke)
fa770fd368e32950dd727e111a5d66e1dbb93716 doc: Add missing RUST_BACKTRACE=1 (MarcoFalke)
fa100512677587b4e84a8be2235cf6d49b6a0134 lint: Add get_subtrees() helper (MarcoFalke)
Pull request description:
Missing `bitcoin-config.h` includes are problematic, because the build could silently pass, but produce an unintended result. For example, a slower fallback algorithm could be picked, even though `bitcoin-config.h` indicates that a faster feature is available and should be used.
As the build succeeds silently, this problem is not possible to detect with iwyu.
Thus, fix this by using a linter based on grepping the source code.
ACKs for top commit:
theuni:
Weak ACK fa58ae74ea67485495dbc2d003adfbca68d6869b.
TheCharlatan:
ACK fa58ae74ea67485495dbc2d003adfbca68d6869b
hebasto:
ACK fa58ae74ea67485495dbc2d003adfbca68d6869b, tested on Ubuntu 23.10 -- it catches bugs properly. I didn't review rust code changes.
Tree-SHA512: cf4346f81ea5b8c215da6004cb2403d1aaf569589613c305d8ba00329b82b3841da94fe1a69815ce15f2edecbef9b031758ec9b6433564976190e3cf91ec8181
Drop `git diff` command so it is easier to run CI locally if git checkout is a
worktree. Currently it fails because the directory is not recognized as a git
repository.
The `git diff` command was added recently in #28359 commit
fa07ac48d804beac38a98d23be2167f78cadefae and can be avoided just by teeing the
patch to stdout
540282905dc6137a307273188d6d9291809f0ee9 docs: ci multi-arch requires qemu (Max Edwards)
Pull request description:
On a fresh Debian system qemu isn't installed and therefore the multi-architecture CI system doesn't run.
This documentation notes that qemu is required and how to install it.
ACKs for top commit:
maflcko:
lgtm ACK 540282905dc6137a307273188d6d9291809f0ee9
willcl-ark:
utACK 540282905dc6137a307273188d6d9291809f0ee9
Tree-SHA512: 903c12cf7b16f2146b99a952577c5550d60faf65f9e72b9f4d9479b52228118ab46349e5130de5281b39da05d3bc0b4ae8a8165601e62ce145647a98ef197131
fa91bf2559d2e839592bf1dc1d423d5fb1c3573e ci: Skip git install if it is already installed (MarcoFalke)
c65fde483133a04964cc8757c96005b78d9e8ca8 ci: vary /tmp/env (Sjors Provoost)
Pull request description:
* Currently, running separate CI tasks at the same time may intermittently fail, because they race to read/write `/tmp/env`. Fix this by adding `$CONTAINER_NAME` to the file name.
* Also, add `$USER`, while touching the line, to allow different users to run the same CI task at the same time.
* Also, skip the git install if there is no need.
Ref: https://github.com/bitcoin/bitcoin/pull/29274
ACKs for top commit:
Sjors:
ACK fa91bf2559d2e839592bf1dc1d423d5fb1c3573e
BrandonOdiwuor:
ACK fa91bf2559d2e839592bf1dc1d423d5fb1c3573e
hebasto:
ACK fa91bf2559d2e839592bf1dc1d423d5fb1c3573e.
Tree-SHA512: 9a8479255a2afb6618f9d0796488d9430ba95266b90ce39536a9817c1974ca4049beeaab5355a38b25171f76fc386dbec06b1919aaa079f08a5a0c0a146232c8
4756114e505cff8848fb6344ef9a48d8822066c1 [depends] Allow PATH with spaces in directory names. (Mark Friedenbach)
Pull request description:
The goal of this PR is to help close https://github.com/bitcoin/bitcoin/pull/28733. I reverted the change on `depends/config.guess` based on the feedback provided in the previous PR. I've also incorporated the test mentioned by maflcko
ACKs for top commit:
maflcko:
lgtm ACK 4756114e505cff8848fb6344ef9a48d8822066c1
hebasto:
ACK 4756114e505cff8848fb6344ef9a48d8822066c1, successfully built depends on Ubuntu 22.04.
TheCharlatan:
ACK 4756114e505cff8848fb6344ef9a48d8822066c1
Tree-SHA512: ee257f6efd235839156bc236384f08d77b91debc3c257168368a71e70742639f28a3289572b8693609c1109062dc9968e461103d1f4f5679906506e94b54e649
aaaace2fd1299939c755c281b787df0bbf1747a0 fuzz: Assume presence of __builtin_*_overflow, without checks (MarcoFalke)
fa223ba5eb764fe822229a58d4d44d3ea83d0793 Revert "build: Fix undefined reference to __mulodi4" (MarcoFalke)
fa7c751bd923cd9fb4790fe7fb51fafa2faa1db6 build: Bump clang minimum supported version to 14 (MarcoFalke)
Pull request description:
Most supported operating systems ship with clang-14 (or later), so bump the minimum to that and allow new code to drop workarounds for previous clang bugs.
For reference:
* https://packages.debian.org/bookworm/clang (`clang-14`)
* https://packages.ubuntu.com/jammy/clang (`clang-14`)
* CentOS-like 8/9 Stream: All Clang versions from 15 to 17
* FreeBSD 12/13: All Clang versions from 15 to 16
* OpenSuse Tumbleweed ships with https://software.opensuse.org/package/clang (`clang17`); No idea about OpenSuse Leap
On operating systems where the clang version is not shipped by default, the user would have to use GCC, or install clang in a different way. For example:
* https://packages.debian.org/bullseye/g++ (g++-10)
* https://packages.ubuntu.com/focal/g++-10
* https://apt.llvm.org/, or nix, or guix, or compile clang from source, ...
ACKs for top commit:
fanquake:
ACK aaaace2fd1299939c755c281b787df0bbf1747a0
Tree-SHA512: 81d066b14cc568d27312f1cc814b09540b038a10a0a8e9d71fc9745b024fb6c32a959af673e6819b817ea7cef98da4abfa63dff16cffb7821b40083016b0291f
This is already used in multiple CIs, and will soon become a requirement
for most CIs, i.e when we migrate depends packages to use CMake, for
example:
https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885576324.
Some of the CIs in 21778 are failing because CMake isn't available, so
just break this out and make CMake globally available.
The `ALLOW_HOST_PACKAGES` variable was introduced in bitcoin#10508 "to
speed up build and avoid timeout".
It is no longer the case for our CI infrastructure, which uses self-
hosted persistent workers and depends caching.
In the current circumstances, it does not seem worth porting this
feature to the upcoming CMake-based build system.
308aec3e5655327d98e0428d8205d246f24d6af5 build: disable external-signer for Windows (fanquake)
35537318a19360ddf1ea8f0c1e6d8ad49e635516 ci: remove --enable-external-signer from win64 job (fanquake)
Pull request description:
It's come to light that Boost ASIO (a Boost Process sub dep) has in some
instances, been quietly initialising our network stack on Windows (see
PR https://github.com/bitcoin/bitcoin/pull/28486 and discussion in https://github.com/bitcoin/bitcoin/issues/28940).
This has been shielding a bug in our own code, but the larger issue
is that Boost Process/ASIO is running code before main, and doing things
like setting up networking. This undermines our own assumptions about
how our binary works, happens before we run any sanity checks,
and before we call our own code to setup networking. Note that ASIO also
calls WSAStartup with version `2.0`, whereas we call with `2.2`.
It's also not clear why a feature like external signer would have a
dependency that would be doing anything network/socket related,
given it only exists to spawn a local process.
See also the discussion in https://github.com/bitcoin/bitcoin/issues/24907. Note that the maintaince of Boost Process in general,
has not really improved. For example, rather than fixing bugs like https://github.com/boostorg/process/issues/111,
i.e, https://github.com/boostorg/process/pull/317, the maintainer chooses to just wrap exception causing overflows
in try-catch blocks: 0c42a58eac. These changes get merged in large,
unreviewed PRs, i.e https://github.com/boostorg/process/pull/319.
This PR disables external-signer on Windows for now. If, in future, someone
changes how Boost Process works, or replaces it entirely with some
properly reviewed and maintained code, we could reenable this feature on
Windows.
ACKs for top commit:
hebasto:
re-ACK 308aec3e5655327d98e0428d8205d246f24d6af5.
TheCharlatan:
ACK 308aec3e5655327d98e0428d8205d246f24d6af5
Tree-SHA512: 7405f7fc9833eeaacd6836c4e5b1c1a7845a40c1fdd55c1060152f8d8189e4777464fde650e11eb1539556a75dddf49667105987078b1457493ee772945da66e
8ea45e626e5a0482ee11d4376f961d8126ce7c7b build: use macOS 14 SDK (Xcode 15.0) (fanquake)
51c97ffb6989a4cf56ad966d360a9fa0426e174c build: patch boost process for macOS 14 SDK (fanquake)
423949a13b39a193dff8b2758d23d6691d11dbc3 depends: add -platform_version to macOS build flags (fanquake)
Pull request description:
This fixes: https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1748515277 (cross-compiling with C++20 for macOS). See https://developer.apple.com/xcode/cpp/#c++20 for C++20 support in Apples libc++, some features landed with Xcode 14.3, although many more landed with Xcode 15.0.
ACKs for top commit:
hebasto:
ACK 8ea45e626e5a0482ee11d4376f961d8126ce7c7b.
TheCharlatan:
ACK 8ea45e626e5a0482ee11d4376f961d8126ce7c7b
Tree-SHA512: 274ce2c9b9f8e4d755c07b8d0d4897a7f92708ac64e6afb7a3f75bdb485e863fc7f40badf3a88b129ce36f6cca72f768dc2ed7fba2bdf0bb6da2bf0c8fedee10
fad2392c5861a88a87cb8a03d2fc9773e178feb8 ci: Use Ubuntu 24.04 Noble for asan (MarcoFalke)
fa83b65ef8934b44fbac02da8dbc27fc0bc230e6 ci: Use Ubuntu 24.04 Noble for tsan,tidy,fuzz (MarcoFalke)
Pull request description:
23.10 will be EOL mid next year, so a bump is needed before then for the `master` branch (and possibly the `26.x` branch).
Doing the bump now is fine, because the clang version is pinned to 17 inside the CI tasks. So a default clang version change in the system image should not affect the tasks. Once clang-18 is available and the default in April next year (https://discourse.ubuntu.com/t/noble-numbat-release-schedule/35649#planned-and-potentially-disruptive-archive-wide-activities-2), the pinned version could be bumped (for CI tasks that require a pin, like tidy), or the pin can be removed (for CI tasks that usually do not require a pin, like fuzz or the sanitizers).
ACKs for top commit:
fanquake:
ACK fad2392c5861a88a87cb8a03d2fc9773e178feb8
Tree-SHA512: c40aede4e2281a5d539d5f65d2c08a57bf92e4a00b4f45a4260b57b7443a63d1a0603115da4a3bbd100ac5f6ade3f2eda0916e4b565573741162a76294ec0ac5
It is confusing to treat commands as a single string. This change is
also required to support paths and strings with spaces in them in the
future.
This requires replacing TEST_RUNNER_ENV with a global export, because it
no longer works. See:
```bash
$ export ENV="A=1" && $ENV ls
bash: A=1: command not found...
```
Or in the CI task:
+ DIR_UNIT_TEST_DATA=/ci_container_base/ci/scratch/qa-assets/unit_test_data/
+ LD_LIBRARY_PATH=/ci_container_base/depends/i686-pc-linux-gnu/lib
+ BITCOIND=bitcoin-node make -j10 check VERBOSE=1
/ci_container_base/ci/test/03_test_script.sh: line 166: BITCOIND=bitcoin-node: command not found
https://github.com/bitcoin/bitcoin/pull/28954/checks?check_run_id=19096858944https://cirrus-ci.com/task/6718317604372480
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
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
bbbbdb0cd57d75a06357d2811363d30a498f4499 ci: Add filesystem lint check (MarcoFalke)
fada2f91108a56cc5c447bd6b6fac411e4d5cdca 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 bbbbdb0cd57d75a06357d2811363d30a498f4499
fanquake:
ACK bbbbdb0cd57d75a06357d2811363d30a498f4499 🦀
Tree-SHA512: 0e2d49742b08eb2635e6fce41485277cb9c40fe20b81017c391d3472a43787db1278a236825714ca1e41c9d2f59913865cfb0c649e3c8ab1fb598c849f80c660
fca0a8938e34cb4f6c400e1d1d0be02f027d80c5 ci: remove "--exclude banman" for fuzzing in mac (brunoerg)
f9b286353f79cdb5e55e2ff4ca47d73e14f9da48 fuzz: call lookup functions before calling `Ban` (brunoerg)
Pull request description:
Fixes#27924
To not have any discrepancy, it's required to call lookup functions before calling `Ban`. If we don't do it, the assertion `assert(banmap == banmap_read);` may fail because `BanMapFromJson` will call `LookupSubNet` and cause the discrepancy between the banned and the loaded one. It happens especially in MacOS (#27924).
Also, calling lookup functions before banning is what RPC `setban` does.
ACKs for top commit:
maflcko:
lgtm ACK fca0a8938e34cb4f6c400e1d1d0be02f027d80c5
dergoegge:
ACK fca0a8938e34cb4f6c400e1d1d0be02f027d80c5
Tree-SHA512: a3d635088a556df4507e65542157f10b41d4f87dce42927b58c3b812f262f4544b6b57f3384eef1097ffdd7c32b8dd1556aae201254960cbfbf48d45551200f7
9f208c017174132dbefbc917aa9824c279382597 ci: Switch IWYU to `clang_17` branch (Hennadii Stepanov)
Pull request description:
The IWYU version [0.21](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.21) has been tagged, and the `clang_17` branch is available now.
ACKs for top commit:
maflcko:
lgtm ACK 9f208c017174132dbefbc917aa9824c279382597
Tree-SHA512: 8b8f8743d1c2719b6383b5a6a48356ac02a301d1ce9cee77f93cc04c12de22e9ac6b59e23550a589540e68292cfac0d85bacedc9ca26f6b589011d36ee1d38cf
It's not completely clear to me why this needs to be explicitly
specified in some environments, and not in others, while at the same time
that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues
outlined in #28147.
Use `LLVM_SYMBOLIZER_PATH` as the env var, as that is somewhat also used
inside LLVM, but not consistently, i.e it's checked for in the asan_symbolize
script, but not in in the ubsan_symbolize script, or from in compiler-rt.
fa25e8b0a1610553014c786428f146ef9c694678 doc: Recommend lint image build on every call (MarcoFalke)
faf70c1f330a92612cf381d32c791e9ba445d3f2 Bump python minimum version to 3.9 (MarcoFalke)
fa8996b930886da712c09ffe4b58016b36c2ae5b ci: Bump i686_multiprocess.sh to latest Ubuntu LTS (MarcoFalke)
Pull request description:
All supported operating systems ship with python 3.9 (or later), so bumping the minimum should not cause any issues. A bump will allow new code to use new python 3.9 features.
For reference:
* https://packages.debian.org/bullseye/python3
* https://packages.ubuntu.com/focal/python3.9
* FreeBSD 12/13 also ships with 3.9
* CentOS-like 8/9 also ships with 3.9 (and 3.11)
* OpenSuse Leap also ships with 3.9 (and 3.11) https://software.opensuse.org/package/python311-base
This is for Bitcoin Core 27.0 in 2024 (next year), not the soon upcoming 26.0 next month.
ACKs for top commit:
Sjors:
ACK fa25e8b0a1610553014c786428f146ef9c694678
jamesob:
ACK fa25e8b0a1610553014c786428f146ef9c694678 ([`jamesob/ackr/28211.1.MarcoFalke.bump_python_minimum_supp`](https://github.com/jamesob/bitcoin/tree/ackr/28211.1.MarcoFalke.bump_python_minimum_supp))
Tree-SHA512: 86c9f6ac4b5ba94a62ee6a6062dd48a8295d8611a39cdb5829f4f0dbc77aaa1a51edccc7a99275bf699143ad3a6fe826de426d413e5a465e3b0e82b86d10c32e
Since all bitcoin-ci-test images are now labeled, we can always
prune all dangling images, regardless of whether we are in
RESTART_CI_DOCKER_BEFORE_RUN.
To be safe, still prune all images if RESTART_CI_DOCKER_BEFORE_RUN
in case the filtering doesn't work, or if images were created on
an earlier version that did not assign labels.
