56750c4f87d089c6a3f093eb2bf2edd07170d4a8 iwyu, clang-format: Sort includes (Hennadii Stepanov)
2c78814e0e182853ce44d9fd63d24ee6cab5223e ci: Add IWYU job (Hennadii Stepanov)
94e4f04d7cf4b0fef9a28d3771e73f1dc9fb0528 cmake: Fix target name (Hennadii Stepanov)
0f81e005197fa4201a38e635ddf8c5dcc12a3878 cmake: Make `codegen` target dependent on `generate_build_info` (Hennadii Stepanov)
73f7844cdb1e225099223a355d88da0522d7d69b iwyu: Add patch to prefer C++ headers over C counterparts (Hennadii Stepanov)
7a65437e23706e4820392dc456c3acccbf196dd6 iwyu: Add patch to prefer angled brackets over quotes for includes (Hennadii Stepanov)
Pull request description:
This PR separates the IWYU checks into its own CI job to provide faster feedback to developers. No other changes are made to the treatment of IWYU warnings. The existing “tidy” CI job will no longer run IWYU.
See also the discussion of https://github.com/bitcoin/bitcoin/pull/33779, specifically this [comment](https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491515263):
> Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.
Based on ideas from https://github.com/bitcoin/bitcoin/pull/32953.
ACKs for top commit:
maflcko:
review ACK 56750c4f87d089c6a3f093eb2bf2edd07170d4a8 🌄
sedited:
ACK 56750c4f87d089c6a3f093eb2bf2edd07170d4a8
Tree-SHA512: af15326b6d0c5d1e11346ac64939644936c65eb9466cd1a17ab5da347d39aef10f7ab33b39fbca31ad291b0b4b54639b147b24410f4f86197e4a776049882694
This should avoid having to include interfaces/chain.h from a kernel
module. interfaces/chain.h in turn includes a bunch of non-kernel
headers, that break the desired library topology and might introduce
entanglement regressions.
Specifically gets rid of batchpriority, chainparams, script/sign.h and
system includes.
Also take the opportunity of cleaning up the headers for the effected
files and adding them to the iwyu-enforced set.
It contains a large `bash -c` string, which is hard to parse. So pull
out components:
* CI_EXEC is only called with absolute folders as args, so the `cd` is
not needed in CI_EXEC. It is only needed to specify the working dir of
running the tests in 03_test_script.sh, so move it there.
* The PATH modification is only needed after commit
4756114e505cff8848fb6344ef9a48d8822066c1 to check that depends does
work properly, even when the PATH contains a space.
* This allows to also drop the `bash -c` and use the proper and safer
"$@" to forward args without the risk of word splitting.
The option was fine, but now that there is a dedicated Alpine Linux
task, which uses BusyBox, it seems redundant.
(See: ci/test/00_setup_env_native_alpine_musl.sh)
So remove the USE_BUSY_BOX option, along with the BINS_SCRATCH_DIR env
var.
Also, enable pipefail in the ci/test/00_setup_env.sh script, while
touching it.
02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d ci, iwyu: Treat warnings as errors for specific directories (Hennadii Stepanov)
57a3eac387bd26689aed7682b248b648dba42779 refactor: Fix includes in `index` directory (Hennadii Stepanov)
bdb8eadcdc193f398ebad83911d3297b5257e721 refactor: Fix includes in `crypto` directory (Hennadii Stepanov)
56f2a689a2016ba2ae9cc40833447dff648af809 ci: Do not patch `leveldb` to workaround UB in "tidy" CI job (Hennadii Stepanov)
Pull request description:
This PR is the first step towards treating IWYU warnings as errors. At this stage, it applies only to the `crypto` and `index` directories.
ACKs for top commit:
maflcko:
re-ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d 💮
ryanofsky:
Code review ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d. Just rebased and update tidy patch comment again since last review
willcl-ark:
ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d
Tree-SHA512: 1c966e01c47bf3e7d225faa3b819367f757430e2d71e9582fa82d67307aabe3f0d76f69346ee180192e7f5ab194ecc58d2b8ecf178eab26ba3309a6b55bff4b6
444409ff2b78d8f3e541bd6e883af8da7adfd264 ci: Reduce Alpine musl task to md runner size (MarcoFalke)
fa6b2e9efece2d728bdc257c36c95db03e1a7bc4 ci: Turn centos config into alpine musl config (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/33437
Historically, the centos task was added to add CI coverage for old packages and 32-bit depends builds, but both are now covered by different tasks.
The CentOS task aligns with Ubuntu/Debian CI tasks in terms of libc usage, but (slightly) differs in package naming and update philosophy. I am not aware of the task ever discovering a centos-related issue, so it seems fine to recycle it into an Alpine Linux task.
The main difference would be that musl libc is now used. Also, busybox is used in Alpine, so in theory the busybox install could be removed from the arm CI task in the future.
Packaging considerations: All packages should roughly be the same (gcc remains at version 14, python remains at version 3.12, etc). Also, all packages are from the Alpine main track, coming with 2 years of support. The only exception is the py3-pip package (https://pkgs.alpinelinux.org/packages?name=py3-pip&branch=v3.22&repo=&arch=riscv64) from the community track, however, I don't expect any issues arising from that.
ACKs for top commit:
janb84:
reACK 444409ff2b78d8f3e541bd6e883af8da7adfd264
willcl-ark:
ACK 444409ff2b78d8f3e541bd6e883af8da7adfd264
Tree-SHA512: fd1a1da0fd766591e44a57dbdb84f9b3b47ca92113a429bba139ee5fef54714b8fe509c321e7b3a470c29b4af7d9eab9786e1660b9effb862ecea52824f458aa
9b76eef2d2b42703e2a30952d4c3474b533e360a ci: cd into BASE_BUILD_DIR for GetCMakeLogFiles (Sjors Provoost)
Pull request description:
When a bug is introduced in cmake, we render its logs, which was broken:
https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3248645770
ACKs for top commit:
maflcko:
lgtm ACK 9b76eef2d2b42703e2a30952d4c3474b533e360a 💽
hebasto:
ACK 9b76eef2d2b42703e2a30952d4c3474b533e360a, [obviously](https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3248645770).
Tree-SHA512: 97a440efcaa058af1144d66257ac0157d499b66fd9a00fa01e0eaa013267af1643cb062ec6c74366fab88cb706c9cf9d5fb0316195e79582679c7ae8d39ec200
When using hosted runners in combination with cached docker images,
there is the possibility that the host runner image is updated,
rendering the linux-headers package (stored in the cached docker image)
incompatible.
Fix this by doing a re-install of the headers package in
03_test_script.sh.
If the underlying runner kernel has not changed thie has no effect, but
prevents the job from failing if it has.
f49840dd902cd9b14b6aadb431b16a4aeb719c3f doc: Fix typo in files.md (Ryan Ofsky)
f5cf0b1ccc8fd426135809a8a4becdae2d797bb5 bitcoin wrapper: improve help output (Ryan Ofsky)
c810b168b89dc07017e9feaec1a8746a449a60b1 doc: Add description of installed files to files.md (Ryan Ofsky)
94ffd01a0294afbe045f1b17a77e4a3caf21e674 doc: Add release notes describing libexec/ binaries (Ryan Ofsky)
cd97905ebc564b8b095099a28d1d5437951927c4 cmake: Move internal binaries from bin/ to libexec/ (Ryan Ofsky)
Pull request description:
This change moves binaries that are not typically invoked directly by users from the `bin/` directory to the `libexec/` directory in CMake installs and binary releases. The goal of the PR is to introduce a distinction between internal and external binaries so starting with #31802, we can use IPC to implement features in new binaries without adding those binaries to the CLI. The change also helps reduce clutter in `bin/`, making it easier for users to identify useful tools to run. Summary of changes:
- For **source builds** (i.e. developer builds) — There are no changes.
- For **source installs** (i.e. `cmake --install` result) — `test_bitcoin`, `test_bitcoin-qt`, and `bench_bitcoin` are installed in `${CMAKE_PREFIX_PATH}/libexec` instead of `${CMAKE_PREFIX_PATH}/bin`, so they are no longer on the system `PATH`. However, they can still be invoked from the `libexec/` directory, or from the CLI as `bitcoin test`, `bitcoin test-gui`, and `bitcoin bench`, respectively.
- For **binary releases** — Since `test_bitcoin` is the only test binary enabled in releases, the only change is moving `test_bitcoin` from `bin/` to `libexec/`.
<details><summary>Details</summary>
<p>
The table below shows the install location of each binary after this change, and the availability of each binary.
| Binary | Location | Availability | Change |
|----------------------|--------------|----------------------|-------------------------------|
| `bitcoin` | `bin/` | 📦 Binary release (since #31375) | Unchanged |
| `bitcoin-cli` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoind` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoin-qt` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoin-tx` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoin-util` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoin-wallet` | `bin/` | 📦 Binary release | Unchanged |
| `bench_bitcoin` | `libexec/` | 🛠 Source build only | Moved from `bin/` |
| `bitcoin-chainstate` | `libexec/` | 🛠 Source build only | Newly installed (was built) |
| `bitcoin-gui` | `libexec/` | 🛠 Source build only (until #31802) | Moved from `bin/` |
| `bitcoin-node` | `libexec/` | 🛠 Source build only (until #31802) | Moved from `bin/` |
| `test_bitcoin` | `libexec/` | 📦 Binary release | Moved from `bin/` |
| `test_bitcoin-qt` | `libexec/` | 🛠 Source build only | Moved from `bin/` |
</p>
</details>
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
l0rinc:
re-ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
Sjors:
re-ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
achow101:
ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
janb84:
re ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
BrandonOdiwuor:
Tested ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
hodlinator:
re-ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
willcl-ark:
utACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
Tree-SHA512: 858a2e1a53db11ee3c5c759bfdeea566f242b9ce5e8a898fa435222e41662b8184577c0dc2c4c058294b4de41d8cb3ba3e5d24c748c280efa4a3f84e3ec4344d
fad191ff48b15832a90c19d560a7c0525c146be3 ci: Avoid cd into build dir (MarcoFalke)
Pull request description:
Changing into the build dir is confusing and brittle, because the following commands implicitly assume it. So they could break on unrelated changes.
The changes are required for stuff like:
* cmake presets (see https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344031208)
* meta ci tests (like https://github.com/bitcoin/bitcoin/pull/32874)
So remove the `cd` and just make the build dir explicit.
ACKs for top commit:
hebasto:
ACK fad191ff48b15832a90c19d560a7c0525c146be3, I have reviewed the code and it looks OK.
Tree-SHA512: a88a9341445ffe28a0dac3815f235ec8eb0459d10a91a80829fd3184762d3c807d0f68c56243b20c04a6efa5becd8a7fad568f43c2b1e6af1ff8ba07b140ef87
Changing into the build dir is confusing and brittle.
This can be reviewed using the git option `--word-diff-regex=.`.
Also:
* add missing -j1 to the fallback that prints a verbose build failure
* remove quotes around $GOAL in the fallback
941b8f54c0d35d3243bb6083f3b52681d1b9a555 ci: run get_previous_releases as part of test cross win job (Max Edwards)
5e2182140bcd26afbe89b13d1d83ece6d5a89731 test: increment mocked time for migrating wallet backups (Max Edwards)
5174565802f426db1e9c8200a9421f82f61a5c99 ci: disable feature_unsupported_utxo_db functional test (Max Edwards)
3dc90d69a64f8bb39af27fa755683589b1bc76a7 test: remove mempool.dat before copying (Max Edwards)
67a6b20d5030ee51bf6d5e0fd77c9bdc8bafa96b test: add windows support to get previous releases script (Max Edwards)
1a1b478ca31be1f670754a47da17863271e46b7b scripted-diff: rename tarball to archive (Max Edwards)
4f06dc848460c887ad8337702ed900ba78725906 test: remove building from source from get prev releases script (Max Edwards)
Pull request description:
This PR updates the `test/get_previous_releases.py` script to also work on Windows by changing to be pure python rather than using unix tools such as `curl` and `tar`.
This enables additional functional tests to run such as `wallet_migration.py`, `mempool_compatability.py` and `wallet_backwards_compatibility.py`.
Unfortunately `feature_unsupported_utxo_db.py` _could_ run but this test requires Bitcoin `v0.14.3` which will not run under windows with emojis in the data directory (as the functional test runner has by default) . This test could be run as it's own step in the ci workflow file and would pass but as it's quite an old version / feature I have assumed it's not worth worrying about and best just to exclude.
Two tests needed to be slightly modified to run under windows. Both were issues with trying to overwrite a file that already exists which windows seems to be more strict on than the unix based systems.
Finally, building from source has been dropped from the `get_previous_releases.py` script. This had not been updated after the move to cmake and so it was assumed that nobody could have been using that feature.
ACKs for top commit:
maflcko:
re-ACK 941b8f54c0d35d3243bb6083f3b52681d1b9a555 🍪
achow101:
ACK 941b8f54c0d35d3243bb6083f3b52681d1b9a555
hodlinator:
re-ACK 941b8f54c0d35d3243bb6083f3b52681d1b9a555
Tree-SHA512: 22933d0ec278b9b0ffcd2a8e90026e1a3631b00186e7f78bd65be925049021e319367d488c36a82ab526a07b264bac18c2777f87ca1174b231ed49fed56d11cb
Using the get_previous_releases.py script to build from source only works for
releases prior to v29 due to removal of Autotools (in favor of CMake). It also
does not support building on Windows, and we are adding support for downloading
Windows release binaries in later commits of this PR.
As there were no complaints during review, it is assumed nobody uses this
functionality.
This change moves binaries that are not typically invoked directly by users
from the `bin/` directory to the `libexec/` directory in CMake installs and
binary releases. The goal is to simplify the contents of `bin/` for end users
while still making all binaries available when needed. After this change, the
binaries remaining in `bin/` are:
- bitcoin
- bitcoin-cli
- bitcoind
- bitcoin-qt
- bitcoin-tx
- bitcoin-util
- bitcoin-wallet
And the binaries that are moved to `libexec/` are:
- bench_bitcoin
- bitcoin-chainstate(*)
- bitcoin-gui(***)
- bitcoin-node(***)
- test_bitcoin(**)
- test_bitcoin-qt
(*) bitcoin-chainstate was previously missing an install rule and was actually
not installed even when it was enabled.
(**) test_bitcoin is the only libexec/ binary that is currently included in
bitcoin binary releases. The others are only installed when building from
source with relevant cmake options enabled.
(***) bitcoin-node and bitcoin-gui are not currently built by default or
included in binary releases but both of these changes are planned and
implemented in #31802
This is mentioned in the developer notes, but isn't present in
`TSAN_OPTIONS`, resulting in:
```bash
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=60508)
Cycle in lock order graph: M0 (0xffff98e02208) => M1 (0xffff98e0cbe8) => M2 (0xffff98e0cd98) => M0
<snip>
Hint: use TSAN_OPTIONS=second_deadlock_stack=1 to get more informative warning message
```
Add it, for (potentially) more informative output, when failures
occur. Checked that adding does output more information.
fa1cffacae61264ac89e30a069ba093495cb3216 ci: Install missing nproc in macos task (MarcoFalke)
faf7a2bccc747f2d509710bd7eeb315e75b6c649 ci: Add missing -DWERROR=ON to test-each-commit (MarcoFalke)
Pull request description:
Found by in Sjors in https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1785860610 (Thanks!)
Also, includes an unrelated commit to simplify the ci scripts, by assuming nproc exists on macos as well. (Having more than one commit is also required to trigger the `test-each-commit` task)
ACKs for top commit:
theuni:
utACK fa1cffacae61264ac89e30a069ba093495cb3216.
Tree-SHA512: 881df6964da31c3f7161b763bdf74029743567272c7a4db24ded10909a8cedad39b494a9f486f0b059bbeb0beb190dcd91369650d014e1fc3b5f7ae07cd54771
This should be long enough (with headroom) for our longest running tests,
which even under MSAN, TSAN, Valgrind, etc max out at about 800s.
i.e under Valgrind I see the longer runtimes as:
```bash
135/136 Test #8: bench_sanity_check_high_priority ..... Passed 371.19 sec
136/136 Test #122: coinselector_tests ................... Passed 343.39 sec
```
In the CI `tests` under TSAN:
```bash
tests ................................ Passed 795.20 sec
```
and MSAN:
```bash
tests ................................ Passed 658.48 sec
```
This will also prevent the current issue we are seeing of `ctest`
running until it reaches the CI timeout, see #30969.
However, we still need to figure out what underlying issue is causing
the tests to (sometimes) run for so long, but in the mean time, this
will stop `ctest` wasting our CI CPU.
3ae35b427fe59bc9ab24d07c1adb46faa702de20 ci: run check-deps.sh as part of clang-tidy job (Ryan Ofsky)
0aaa1298a08f898318916661f2317b2e755206e6 contrib: fix check-deps.sh when libraries do not import symbols (Ryan Ofsky)
3c99f5a38a47e4e10a0daab3a114b5e476fcacfa contrib: fix check-deps.sh to check for weak symbols (Ryan Ofsky)
86c80e9cf296f6560f2f846bd4e7286f7b958b93 contrib: make check-deps.sh script work with cmake (Ryan Ofsky)
Pull request description:
Fix check-deps.sh to check for weak symbols so it can detect when an exported template function like is used from another library.
Also update the script to work with cmake and configure it to run as part of CI.
Problem was reported by hebasto in https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2209258843
ACKs for top commit:
TheCharlatan:
Re-ACK 3ae35b427fe59bc9ab24d07c1adb46faa702de20
hebasto:
ACK 3ae35b427fe59bc9ab24d07c1adb46faa702de20, I have reviewed the code and it looks OK. Also I've tested it locally.
Tree-SHA512: c3b58175450b675e6e848549b81bcfe42930ea9bcd693063867ce3f0ac3999c98cd2c3e961f163ff06641e8288f3a4e81530936a296a83d45d33364f27489521
8131bf7483c0ea10d3573c9f2e977d19d8569b7f ci: parse TEST_RUNNER_EXTRA into an array (Max Edwards)
c4762b0aa06f2654d108bc7ca05887ffd88cf6f8 test: allow excluding func test by name and arg (Max Edwards)
Pull request description:
While working on CI I wanted to disable some functional tests so I used the `TEST_RUNNER_EXTRA` var. The problem I had was tests that have flags such as `rpc_bind.py --ipv6` must be passed in quotes otherwise the `--ipv6` portion will be considered an argument to `test_runner.py` rather than a test name.
This change allows proper parsing of quotes and complex values such as:
```shell
TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6,feature_proxy.py"'
```
Update:
While testing this it was noticed that `test_runner.py` when given `--exclude "rpc_bind.py --ipv6"` will exclude all `rpc_bind.py` tests so this PR has been updated to include a change to the test runner to only exclude the specific test if you pass an arg or exclude all tests of that name if you do not pass an arg. `--exclude rpc_bind.py` will exclude all three variants and `--exclude rpc_bind --ipv6` will only exclude the IPV6 variant.
ACKs for top commit:
maflcko:
ACK 8131bf7483c0ea10d3573c9f2e977d19d8569b7f
achow101:
ACK 8131bf7483c0ea10d3573c9f2e977d19d8569b7f
hebasto:
ACK 8131bf7483c0ea10d3573c9f2e977d19d8569b7f, tested on Ubuntu 23.10 and Windows 11.
Tree-SHA512: 82b73f12d627f533d8e5be4a518d455ef4427a755bbe03ccd11d0bb70c7ff3cee76220b0264fcfb236661c4cf5deba034cbfc2372b96d5861f3436c21eae8264
