fa3b5e5e57 ci: Use nproc over MAKEJOBS in 01_base_install (MarcoFalke)
Pull request description:
Currently `$MAKEJOBS` is the default value in `01_base_install.sh` when building the container image.
This problem can't be fixed (see below), so just use `nproc` for now.
Other solutions would be bad:
* Passing in the `MAKEJOBS` as a dockerfile env would create a new image if the number of tasks are changed, seems verbose and confusing.
* Leaving `master` as-is would leave CPUs unused if there are more than `4`.
ACKs for top commit:
fanquake:
ACK - I think this is fine as-is, it's an improvement over the current state fa3b5e5e57
Tree-SHA512: 15014eb7b548945737b0fed5cd46f0cd4b72b1d54d044f60fce628f914053a2de6518878428a54822d236d08d6f257d8c464d3fb589400f4be56022d2ec8676d
de8f9123af test: cover read-only blockstore (Matthew Zipkin)
5c2185b3b6 ci: enable chattr +i capability inside containers (Matthew Zipkin)
e573f24202 unit test: add coverage for BlockManager (Matthew Zipkin)
Pull request description:
This PR adds unit and functional tests to cover the behavior described in #2039. In particular, that bitcoind will crash on startup if a reindex is requested but the `blk` files are read-only. Eventually this behavior can be updated with https://github.com/bitcoin/bitcoin/pull/27039. This PR just commits the test coverage from #27039 as suggested in https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1584915782
ACKs for top commit:
jonatack:
ACK de8f9123af modulo suggestions in https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1319010039, tested on macOS, but not on Linux for the Linux-related change in the last push
achow101:
ACK de8f9123af
MarcoFalke:
lgtm ACK de8f9123af📶
Tree-SHA512: b9bd684035dcea11c901b649fc39f397a2155a9a8459f3348e67947e387e45312fddeccb52981aef486f8a31deebb5356a7901c1bb94b78f82c24192a369af73
fa07ac48d8 ci: Asan with -ftrivial-auto-var-init=pattern (MarcoFalke)
Pull request description:
This makes memory bugs deterministic. `-ftrivial-auto-var-init=pattern` is incompatible with other memory sanitizers (like valgrind and msan), but that is irrelevant here, because the address sanitizer in this fuzz CI config is already incompatible with them.
`-ftrivial-auto-var-init=pattern` goes well with `-fsanitize=bool` and `-fsanitize=enum`, but those are already enabled via `-fsanitize=undefined`. See https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#available-checks
ACKs for top commit:
fanquake:
ACK fa07ac48d8 - going to get back to fixing up the cxxflags usage in CI, but not a blocker here:
Tree-SHA512: 2ea6c5128a9cd262bdd1b1475c7e1be23ce2c520fad05f6c2aace6c29e203573323c0564d272e25da35ad5ff46fde488a5eae1ed14d3349e793d14a5e2533fb1
fa70cbd969 ci: Remove unused TEST_RUNNER_ENV="LC_ALL=C" from s390x task (MarcoFalke)
fa33354dcb ci: Remove /ro_base bind mount (MarcoFalke)
fa0df9d4c4 doc: Remove sudo from command that is already run as root (MarcoFalke)
Pull request description:
Remove some CI stuff no longer needed.
ACKs for top commit:
fanquake:
ACK fa70cbd969 - did not test the s390x job.
Tree-SHA512: 3a6ed0cfc855a92c2f834e59494c0a19a5647510247aece5e40a1aa78074894fe7454e684a1ea1f8f0662c50ac1caf2e390398b0fcfbf81544e6488fa9b8915e
fab7f5c01d ci: Add missing docker.io prefix to CI_IMAGE_NAME_TAG (MarcoFalke)
Pull request description:
Currently, the CI system may pick the wrong (non-native) architecture due to the missing prefix.
For example, assuming the CI_IMAGE_NAME_TAG is `debian:bookworm` and the user has previously pulled an s390x image:
```
$ podman run --rm 'docker.io/s390x/debian:bookworm' dpkg --print-architecture
exec /usr/bin/dpkg: exec format error
```
Now, `debian:bookworm` will refer to the same image:
```
$ podman run --rm 'debian:bookworm' dpkg --print-architecture
exec /usr/bin/dpkg: exec format error
```
However, `docker.io/debian:bookworm` works fine:
```
$ podman run --rm 'docker.io/debian:bookworm' dpkg --print-architecture
arm64
ACKs for top commit:
hebasto:
ACK fab7f5c01d.
Tree-SHA512: c423c5cd454a95fa3e67081411ca08d316b8c680a5bba49196c514b91df65d9cc46a47700cc00d9579327842615f98146d0ac50abb016616a9b17d042598dab6
fa56d17a4b ci: Add missing amd64 to win64-cross task (MarcoFalke)
Pull request description:
Currently the task will fail if run on non-`x86_64`.
Fix this by adding the missing `amd64`, similar to
7bf078f2b7/ci/test/00_setup_env_i686_multiprocess.sh (L11)
ACKs for top commit:
hebasto:
ACK fa56d17a4b
Tree-SHA512: faab1c5b945283b7e8d080bbcc8e9379c480cf6973506149ace5990cb4d04673f83f4bc36d08d5b4e9cb17a86fdbe23ac97ef4eab0e842616b367b8138229c58
This should fix the macOS-cross build on Cirrus CI containers.
Locally this was already working, because the SDK was cached in
/ci_container_base/ in the image, which is also the folder used for a
later CI run.
However, on Cirrus CI, when using an image *and* a custom BASE_ROOT_DIR,
the SDK will not be found in /ci_base_install/, nor in BASE_ROOT_DIR.
Fix this by normalizing *all* folders to /ci_container_base/.
9658d0dc17 ci: Run "macOS native x86_64" job on GitHub Actions (Hennadii Stepanov)
Pull request description:
From https://github.com/bitcoin/bitcoin/issues/28098:
> Thus, someone would have to sponsor an amount of roughly 5kUSD/mo for those two tasks.
> If the goal is to stay on a free plan, I think the only option is GitHub Actions CI.
---
**IMPORTANT NOTE**. We currently ship macOS release binaries for both architectures: `x86_64` and `arm64`. If this PR gets merged, only `x86_64` architecture will be tested on CI, which implies some [drawbacks](https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1658077549).
However, it has never been the case that our CI tested both architectures simultaneously. And we hope that GitHub Actions will soon host macOS `arm64` runners.
Historically, we moved from `x86_64` to `arm64` in https://github.com/bitcoin/bitcoin/pull/26388 less than a year ago.
---
Security concerns:
- https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651432106
- https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651688197
`GITHUB_TOKEN` permissions (from the build log in my personal repo):
```
2023-07-27T07:30:17.8313534Z ##[group]GITHUB_TOKEN Permissions
2023-07-27T07:30:17.8314113Z Contents: read
2023-07-27T07:30:17.8314608Z Metadata: read
2023-07-27T07:30:17.8314957Z Packages: read
2023-07-27T07:30:17.8315233Z ##[endgroup]
```
Comparison of resources:
| Resource | Current, Cirrus CI | Suggested, GitHub Actions |
|---|:-:|:-:|
| CPU | 4 | 4 \*\* |
| RAM, GB | 8 | 14 |
**\*\* NOTE**: However, [docs](https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources) are mentioning:
> 3-core CPU (x86_64)
ACKs for top commit:
MarcoFalke:
re-ACK 9658d0dc17🏂
achow101:
ACK 9658d0dc17
jarolrod:
ACK 9658d0dc17
Tree-SHA512: 6123e68e6784cdf4e53c3e77b435709261db21f09091af2c22e667d3816a305fffb9d617297a5bc1bda18aaba84a6e210cec6a75c52afa7746a3780a67b69865
1c976c691c tidy: Integrate bicoin-tidy clang-tidy plugin (fanquake)
7de23cceb8 refactor: fix unterminated LogPrintf()s (fanquake)
0a1029aa29 lint: remove /* Continued */ markers from codebase (fanquake)
910007995d lint: remove lint-logs.py (fanquake)
d86a83d6b8 lint: drop DIR_IWYU global (fanquake)
Pull request description:
Demo of integrating the [bitcoin-tidy](https://github.com/theuni/bitcoin-tidy-plugin), [clang-tidy plugin](https://clang.llvm.org/extra/clang-tidy/) written by theuni into our tidy CI job.
The plugin currently has a single check, `bitcoin-unterminated-logprintf`. This would replace our current Python driven, `git-grep`-based, `.cpp` file only, lint-logs linter.
ACKs for top commit:
TheCharlatan:
ACK 1c976c691c
theuni:
ACK 1c976c691c
MarcoFalke:
re-ACK 1c976c691c👠
Tree-SHA512: 725b45c70e431d48e6f276671e05c694e10b6047cae1a31906ac3ee9093bc8105fb226b36a5bac6709557526ca6007222112d66aecec05a574434edc4897e4b8
This change aims to:
1) Remove our own `CCACHE_SIZE` environment variable that violates
Ccache's `CCACHE_*` namespace.
2) Introduce the `CCACHE_MAXSIZE` environment variable that is
documented since v3.3, which makes its usage consistent with other ones,
such as `CCACHE_DIR` and `CCACHE_NOHASHDIR`.
fabc04a4d9 ci: Keep system env vars as-is (MarcoFalke)
fa8dcdcc8b ci: Set PATH inside the CI env (MarcoFalke)
fac229ab1f ci: Remove P_CI_DIR and --workdir (MarcoFalke)
Pull request description:
This fixes a bug where the `$PATH` from the host is used inside the container. This will lead to bugs when the `$PATH` is different. For example on a host of Fedora 38, and a container of `debian:bullseye`.
This can be tested with the `FILE_ENV=./ci/test/00_setup_env_arm.sh` CI env. On master:
```
Error: crun: executable file `bash` not found in $PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found
```
On this pull:
(everything passes)
ACKs for top commit:
TheCharlatan:
lgtm ACK fabc04a4d9
Tree-SHA512: 51d2affed91624d0e5b43a3ee1e696313f934e05fde6b5a19e8ac4e6666c1e7b667a444bf3de3f77190bcd00e81efb7576196afb0f6b5e788d1a50e7ddb28d7e
This is needed for the next commit.
This also requires dropping CI_RETRY from the docker build step, which
is fine, because CI_RETRY should be called inside the build script, not
outside.
Also, fix a doc typo.
`noreturn` attributes have been added to the mingw-w64 headers, meaning
that from 11.0.0 onwards, you'll no-longer see `-Wreturn-type` warnings
when using assert(false):
1690994f51.
Add -Wno-return-type to the Windows CI, where is should have been all
along, and document why it's required. This can be dropped when we are
using the fixed version of the mingw-w64 headers there.
Drop the -Werror -Wno-return-type special case from our build system.
-Wreturn-type is on by default in Clang and GCC.
These work for me now. If they still don't work in other setups,
maybe we can better document the issues.
```bash
time FILE_ENV="./ci/test/00_setup_env_s390x.sh" ./ci/test_run_all.sh
...
Running tests: coins_tests from test/coins_tests.cpp
PASS: qt/test/test_bitcoin-qt
Running tests: coinstatsindex_tests from test/coinstatsindex_tests.cpp
...
Stop and remove CI container by ID
+ docker container kill 617bef8accb87530e5fbb03ff07b3b9f0aa9e3030d4da424c9612d153ab98dbf
617bef8accb87530e5fbb03ff07b3b9f0aa9e3030d4da424c9612d153ab98dbf
real 51m37.809s
```
fae7c50d20 test: Run fuzz tests on macOS (MarcoFalke)
Pull request description:
Any reason not to?
ACKs for top commit:
jamesob:
Github ACK fae7c50d20
dergoegge:
utACK fae7c50d20
Tree-SHA512: e45122d73fafb17cea312258314b826cb0745e08daadd28465f687ec02d4c127d2f8cbe20179a9fff5712038850c02c968abb4838fa088b7555e28709317d3a3
32e2ffc393 Remove the syscall sandbox (fanquake)
Pull request description:
After initially being merged in #20487, it's no-longer clear that an internal syscall sandboxing mechanism is something that Bitcoin Core should have/maintain, especially when compared to better maintained/supported alterantives, i.e [firejail](https://github.com/netblue30/firejail).
There is more related discussion in #24771.
Note that given where it's used, the sandbox also gets dragged into the kernel.
If it's removed, this should not require any sort of deprecation, as this was only ever an opt-in, experimental feature.
Closes#24771.
ACKs for top commit:
davidgumberg:
crACK 32e2ffc393
achow101:
ACK 32e2ffc393
dergoegge:
ACK 32e2ffc393
Tree-SHA512: 8cf71c5623bb642cb515531d4a2545d806e503b9d57bfc15a996597632b06103d60d985fd7f843a3c1da6528bc38d0298d6b8bcf0be6f851795a8040d71faf16
Also, fix a few bugs:
* Error: RPC command "enumeratesigners" not found in RPC_COMMANDS_SAFE_FOR_FUZZING or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update test/fuzz/rpc.cpp.
* in run_once: ...format(" ".join(result.args), ... TypeError: sequence item 2: expected str instance, PosixPath found
cbee1d7091 depends: modernize clang flags (Cory Fields)
2a85857ce5 ci: disable false-positive warnings for now (Cory Fields)
Pull request description:
This is a cleaner and simpler alternative to #25098. Inspired by [this conversation](https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1562543301). The diff is large but the change itself is quite small.
Fixes builds with llvm >= 11 in guix by working around the problem. As a bonus, this is much cleaner and more maintainable than what we had before.
See the updated comment for more info. At a high level: rather than playing tricks and trying to work around clang's default includes, disable them and re-add what we want.
ACKs for top commit:
fanquake:
ACK cbee1d7091 - tested Guix and the depends cross-compile. Would like to move this along, to unblock #27676, which itself might be a blocker for #27897. Note that macOS might seem somewhat in flux for the moment, but once we finish the migration to LLVM Clang + LLD, things will be must simpler, and ultimately more maintainable.
TheCharlatan:
ACK cbee1d7091
Tree-SHA512: 5a8300be528f550e15ab23d869e77df7a62201c6d40c0384795a9eecee38118a676e0b79b2b76c5e597597181443caada54a01b75a544dbcde76da1deba8e3a4
0000f55293 ci: Run fuzz target even if input folder is empty (MarcoFalke)
Pull request description:
This should catch trivial integer sanitizer bugs if the author and all reviewers forget to look for them.
ACKs for top commit:
brunoerg:
reACK 0000f55293
dergoegge:
reACK 0000f55293
Tree-SHA512: f139b9d56f0cf1aae339c2890721c77c88d1fea77b73d492c1386ec99b4f393c5b664029919ff4a22e4e8a2929f085699a148c6acc2cc3e40df8a72fd39ff474
clang <=17 warns on -nostdlibinc, which causes an error on our -Werror builds.
Note that this breaks the "-fPIE" check in configure because it relies on
catching warnings, but that is not a problem for macOS.
After initially being merged in #20487, it's no-longer clear that an
internal syscall sandboxing mechanism is something that Bitcoin Core
should have/maintain, especially when compared to better
maintained/supported alterantives, i.e firejail.
Note that given where it's used, the sandbox also gets dragged into the
kernel.
There is some related discussion in #24771.
This should not require any sort of deprecation, as this was only ever
an opt-in, experimental feature.
Closes#24771.
5763b232e6 ci: return to using Ubuntu 22.04 in MSAN jobs (fanquake)
d3cbcbf626 ci: compile clang and compiler-rt in MSAN jobs (fanquake)
796bd1d0d1 ci: use LLVM 16.0.4 in MSAN jobs (fanquake)
883bc9f561 ci: remove extra CC & CXX from MSAN jobs (fanquake)
2d4f4b8f29 ci: standardize custom libc++ usage in MSAN jobs (fanquake)
Pull request description:
This reworks the MSAN CIs, to first compile Clang and compiler-rt (using GCC 12), and then, compile an MSAN instrumented libc++ using the just-built Clang 16. This fixes the `native_fuzz_with_msan` job, working around https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1005341, by not using the Debian provided Clang/LLVM.
Also included are changes to streamline how we use our "custom libc++", according to upstream: https://releases.llvm.org/16.0.0/projects/libcxx/docs/UsingLibcxx.html#using-a-custom-built-libc, as well as other minor cleanups in the CI configs.
An example job is currently running in the qa-assets repo: https://github.com/bitcoin-core/qa-assets/pull/129 (https://cirrus-ci.com/task/4632561431871488).
ACKs for top commit:
dergoegge:
utACK 5763b232e6
Tree-SHA512: 4f2a6e0b796bb1830b8346dd1e55eaa86a79037b8b4f16a336c1e29f4fc460acca2ecba076635459370bcbb4009333cb79d27ef1521c1fb5db7599cd5bdf558c
fa3ab45203 ci: Enable float-divide-by-zero check (MarcoFalke)
Pull request description:
Enable it, because
* It is enabled on OSS-Fuzz, so to be able to catch bugs earlier, enable it here as well.
* It makes sense to enable, because when a float is divided by zero, it may be a logic bug in our code, so it should be suppressed in the suppressions file.
ACKs for top commit:
willcl-ark:
utACK fa3ab45203
dergoegge:
ACK fa3ab45203
Tree-SHA512: 2c2c025af4fe3ec267b3cfa38f25495e9da678cf6c529a6438ec923ef09a06ad37fa4503c30cbacc83578ac2856a7f729ef70a24befffd61d10ec075132d1ee0
fa123077bc ci: Use podman for persistent workers (MarcoFalke)
fa9c65a74c ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN (MarcoFalke)
Pull request description:
This should prevent the persistent workers from running out of disk space. Containers are already removed, but not images. This is required since CI images are built and cached.
ACKs for top commit:
hebasto:
ACK fa123077bc
Tree-SHA512: 07c4faec57d659d1762e4e6d776c882ee48d4bac6ce6d438d56d9ab13277be3e39d6aa38816165a5a3e0938ac5d47674ee2921b6e115a4bb54e3e4910b34c4b6
59c8944749 build: disable boost multi index safe mode (willcl-ark)
Pull request description:
Fixes#27586
Disable boost multi index safe mode by default when configuring with
--enable-debug.
This option can cause transactions to take a long time to be accepted
into the mempool under certain conditions; iterator destruction takes
O(n) time vs O(1) as they are stored in a singly linked list. See
27586 and the [boost docs](https://www.boost.org/doc/libs/1_58_0/boost/multi_index/detail/safe_mode.hpp) for more information.
Re-enable it on the CI builds which previously had it enabled.
Re-enable it on the msan fuzz task so that we have fuzz tasks testing
with it enabled and disabled in this repo.
ACKs for top commit:
hebasto:
~ACK 59c89447499bd9d6202269879555b8bc37373aa2~
fanquake:
ACK 59c8944749
Tree-SHA512: ed654f63dbebdd02e4414d1f81147d92a4d490dbb5a2e0376858e3129097645f3a2df45191d6b40c410a76e803b0d28796d1a01c1d2fd995b94e8b7eb3949027
1f97572b9c Fix `#include`s in `src/wallet` (Hennadii Stepanov)
Pull request description:
This PR is a minimum required changes to fix https://github.com/bitcoin/bitcoin/pull/27571#discussion_r1195497290.
ACKs for top commit:
MarcoFalke:
lgtm ACK 1f97572b9c
Tree-SHA512: de885210076d23f3394c42ca50e6ae2470c0ae6523399a2fa3ebb7c06383bdacef9c26166fa19747200396bed796c8772165e24416eb30ed8edd024e3394b2fe
Disable boost multi index safe mode by default when configuring with
--enable-debug.
This option can cause transactions to take a long time to be accepted
into the mempool under certain conditions; iterator destruction takes
O(n) time vs O(1) as they are stored in a singly linked list. See
27586 for more information.
Re-enable it on the CI builds which previously had it enabled.
Re-enable it on the msan fuzz target so that we have fuzz tasks testing
with it enabeld and disabled in this repo.
fa1b3abc83 ci: Log qa-assets repo last commit (MarcoFalke)
fa22966f33 fuzz: Print error message when FUZZ is missing (MarcoFalke)
Pull request description:
Some trivial UX improvements.
* Change the exit code for `PRINT_ALL_FUZZ_TARGETS_AND_ABORT` and `WRITE_ALL_FUZZ_TARGETS_AND_ABORT` to `EXIT_SUCCESS` instead of `Aborted (core dumped)`.
* Print readable error message when `FUZZ` is missing instead of `Aborted (core dumped)`.
* Clarify that a fuzz target needs to be compiled into the executable.
ACKs for top commit:
dergoegge:
ACK fa1b3abc83
Tree-SHA512: 065ef8920449c64b3516f89a61cb397b505eccf531318c4f3830895d5ff6cd7ae2525cb857320481e3d0ed0b2f8a522cd8f7835e69f021241b6ec297a6102fc8
5228223e1f ci: remove MSAN getrandom syscall workaround (fanquake)
d5e06919db random: switch to using getrandom() directly (fanquake)
c2ba3f5b0c random: add [[maybe_unused]] to GetDevURandom (fanquake)
c13c97dbf8 random: getentropy on macOS does not need unistd.h (fanquake)
Pull request description:
This requires a linux kernel of `3.17`+, which seems entirely
reasonable. `3.17` went EOL in 2015, and the last supported `3.x` kernel
(`3.16`) went EOL > 4 years ago, in 2020. For reference, the current
oldest maintained kernel is `4.14` (released 2017, going EOL Jan 2024).
Support for `getrandom()` (and `getentropy()`) was added to
glibc `2.25` https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00079.html:
> * The getentropy and getrandom functions, and the <sys/random.h> header
file have been added.
and we already require `2.27` or later.
All that being said, I don't think you would encounter a current day (+~6 months from now)
system, running with kernel headers older than 3.17 (released 2014) but also having a
glibc of 2.27+ (released 2018)?
Removing this (our only) use of `syscall()` also means we can drop a workaround in our MSAN jobs.
If this is merged, I'll drop the [same workaround in oss-fuzz](25946a5448/projects/bitcoin-core/build.sh (L49-L56)).
ACKs for top commit:
josibake:
ACK 5228223e1f
hebasto:
ACK 5228223e1f, I've tested build system changes on Ubuntu 22.04 and macOS Monterey 12.6.6 (x86_64).
Tree-SHA512: cc978e08510c461b875ca8c08ae176b4519fa1108f0efd74dcb7474518945357e0184e54423282c9a496de195e4ddc3e221ee78623bd63e24c50cc86acdf32e2
6a936580d1 ci: remove RUN_SECURITY_TESTS (fanquake)
Pull request description:
We no-longer run any security/symbol checks in the CI, and doubt we will in future (if we do, it'll be via Guix, where this var would be redundant in any case). The CI environment doesn't (exactly) match the release build environment (and is semi-regularly changing), and the binaries produced in the CI don't match how we build release binaries, so there is no point trying to run these checks, especially as we add more involved tests, i.e #26953.
ACKs for top commit:
josibake:
code review ACK 6a936580d1
TheCharlatan:
ACK 6a936580d1
Tree-SHA512: c0eec61a4b873bac487ba9321b50116a215b4796bd7d416d98ffcd09969dbf635c2cb5aeb225c89d1e6462838fa2a48565048ebe730f48d76d3db46b64855a91
