b5ef1419ec ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321) (Vasil Stoyanov)
Pull request description:
Basically it removes the above-mentioned env-vars as per MarcoFalke's instructions. The only deviation from the plan laid out there was that I double-quoted the last instance of $ANDROID_HOME for the sake of consistency and future-proofing and the rest of the non-quoted vars due to lint failing the build.
Fixes#27321.
ACKs for top commit:
josibake:
ACK b5ef1419ec
hernanmarino:
untested ACK b5ef1419ec. LGTM
Tree-SHA512: a79776bf64a2fa8b38195cc84445e171fd689f156aac5a1e5d39040300567eb9f4c2ebd00fbf3fa0e55b68793f8f752d94f7d817f6097ed9dd3a8ea57651b981
This is a minimal extraction of a single function, but also the only use
of std::exception in util/system.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving the function out of system.h allows including it from a separate
source file without including the ArgsManager definitions from system.h.
fa1b4e5c32 Use steady clock in FlushStateToDisk (MarcoFalke)
1111e2f8b4 Use steady clock in SeedStrengthen and FindBestImplementation (MarcoFalke)
Pull request description:
There may be a theoretical deadlock for the duration of the offset when the system clock is adjusted into a past time while executing `SeedStrengthen`.
Fix this by using steady clock.
Do the same in `FindBestImplementation`, which shouldn't be affected, because it discards outlier measurements. However, doing the same there for consistency seems fine.
Do the same in `FlushStateToDisk`, which should make the flushes more steady, if the system clock is adjusted by a large offset.
ACKs for top commit:
john-moffett:
ACK fa1b4e5c32
willcl-ark:
ACK fa1b4e5c3
Tree-SHA512: cc625e796b186accd53222bd64eb57d0512bc7e588312d254349b542bbc5e5daac348ff2b3b3f7dc5ae0bbbae2ec11fdbf3022cf2164211633765a4b0108e83e
802cc1ef53 Deduplicate bitcoind and bitcoin-qt init code (Ryan Ofsky)
d172b5c671 Add InitError(error, details) overload (Ryan Ofsky)
3db2874bd7 Extend bilingual_str support for tinyformat (Ryan Ofsky)
c361df90b9 scripted-diff: Remove double newlines after some init errors (Ryan Ofsky)
Pull request description:
Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code reading config files and creating the datadir.
Noticed the duplicate code while reviewing #27073 and want to remove it because difference in bitcoin-qt and bitcoind behavior make it hard to evaluate changes like #27073
There are a few minor changes in behavior:
- In bitcoin-qt, when there is a problem reading the configuration file, the GUI error text has changed from "Error: Cannot parse configuration file:" to "Error reading configuration file:" to be consistent with bitcoind.
- In bitcoind, when there is a problem reading the settings.json file, the error text has changed from "Failed loading settings file" to "Settings file could not be read" to be consistent with bitcoin-qt.
- In bitcoind, when there is a problem writing the settings.json file, the error text has changed from "Failed saving settings file" to "Settings file could not be written" to be consistent with bitcoin-qt.
- In bitcoin-qt, if there datadir is not accessible (e.g. no permission to read), there is an normal error dialog showing "Error: filesystem error: status: Permission denied [.../settings.json]", instead of an uncaught exception.
ACKs for top commit:
Sjors:
Light review ACK 802cc1ef53
TheCharlatan:
ACK 802cc1ef53
achow101:
ACK 802cc1ef53
Tree-SHA512: 9c78d277e9ed595fa8ce286b97d2806e1ec06ddbbe7bd3434bd9dd7b456faf8d989f71231e97311f36edb9caaec645a50c730bd7514b8e0fe6e6f7741b13d981
Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code
reading config files and creating the datadir.
There are a few minor changes in behavior:
- In bitcoin-qt, when there is a problem reading the configuration file, the
GUI error text has changed from "Error: Cannot parse configuration file:" to
"Error reading configuration file:" to be consistent with bitcoind.
- In bitcoind, when there is a problem reading the settings.json file, the
error text has changed from "Failed loading settings file" to "Settings
file could not be read" to be consistent with bitcoin-qt.
- In bitcoind, when there is a problem writing the settings.json file, the
error text has changed from "Failed saving settings file" to "Settings file
could not be written" to be consistent with bitcoin-qt.
- In bitcoin-qt, if there datadir is not accessible (e.g. no permission to read),
there is an normal error dialog showing "Error: filesystem error: status:
Permission denied [.../settings.json]", instead of an uncaught exception
4275195606 De-duplicate add_coin methods to a test util helper (Jon Atack)
9d92c3d7f4 Create InsecureRandMoneyAmount() test util helper (Jon Atack)
81f5ade2a3 Move random test util code from setup_common to random (Jon Atack)
Pull request description:
- Move random test utilities from `setup_common` to a new `random` file, as many tests don't use this code.
- Create a helper to generate semi-random CAmounts up to `MONEY_RANGE` rather than only uint32, and use the helper in the unit tests.
- De-duplicate a shared `add_coin` method by extracting it to a `coins` test utility.
ACKs for top commit:
pinheadmz:
ACK 4275195606
achow101:
ACK 4275195606
john-moffett:
ACK 4275195606
Tree-SHA512: 3ed974251149c7417f935ef2f8865aa0dcc33b281b47522b0f96f1979dff94bb8527957f098fe4d210f40d715c00f29512f2ffe189097102229023b7284a3a27
fa8e92c022 doc: Update ci docs (721217.xyz)
5fffff54e9 ci: Cache stuff in volumes, not host folders (MarcoFalke)
Pull request description:
Storing cached stuff in host system folders may lead to unexpected issues when the ci-built stuff is used for a non-ci build or a ci task leaks into another ci task.
ACKs for top commit:
john-moffett:
ACK fa8e92c022
Tree-SHA512: 8b0c9019452fbe507a272c1037c3dce3c178c21f85ab1096ed3372ad9d4b3c7aa27d89e5bf80c9a6260ea652e0268be0cbe61d6a4fcb3add569fa38076d32287
935acdcc79 refactor: modernize the implementation of uint256.* (pasta)
Pull request description:
- Constructors of uint256 to utilize Span instead of requiring a std::vector
- converts m_data into a std::array
- Prefers using `WIDTH` instead of `sizeof(m_data)`
- make all the things constexpr
- replace C style functions with c++ equivalents
- memset -> std::fill
This may also be replaced by std::memset, but I think that std::fill is more idiomatic of modern c++ and readable.
- memcpy -> std::copy
Note: In practice, implementations of std::copy avoid multiple assignments and use bulk copy functions such as std::memmove if the value type is TriviallyCopyable and the iterator types satisfy LegacyContiguousIterator. (https://en.cppreference.com/w/cpp/algorithm/copy)
This could also likely be replaced by std::memcpy, but as said above, I believe the using std::copy is the more c++ way to do anything and is almost guaranteed to compile to the same asm
- memcmp -> std::memcmp
ACKs for top commit:
achow101:
ACK 935acdcc79
hebasto:
Approach ACK 935acdcc79.
aureleoules:
reACK 935acdcc79
john-moffett:
ACK 935acdcc79
stickies-v:
Approach ACK 935acdcc7
Tree-SHA512: 4f1ba54ff2198eea0e505d41e73d552c84c60f6878d5c85a94a8ab57f39afc94ef8d79258e7afd01fa84ec2a99f4404bb877eecd671f65e1ee9273f3129fc650
fa6986a66b ci: Print iwyu patch in git diff format (MarcoFalke)
Pull request description:
Seems more dev friendly to also have a patch to copy-paste
ACKs for top commit:
hebasto:
ACK fa6986a66b, tested on Ubuntu 22.04 locally.
fanquake:
ACK fa6986a66b - did not test but example CI output looks ok.
stickies-v:
utACK fa6986a66b
Tree-SHA512: 7cfd8584bf12e03c28af23f4712c6bcafd648d87ddb92788b9cd35455b2db49f4bd4aef8ad4711f75c7f11ad2bb2492c2eb6044007086c20e36016575c060603
fa486de212 ci: Cache package manager install step (MarcoFalke)
Pull request description:
Use the local podman or docker image cache to skip the slow `apt` step
ACKs for top commit:
jamesob:
ACK fa486de212 ([`jamesob/ackr/26976.1.MarcoFalke.ci_cache_package_manager`](https://github.com/jamesob/bitcoin/tree/ackr/26976.1.MarcoFalke.ci_cache_package_manager))
Tree-SHA512: 3495346c6c862b63296d2691cc492bf52a0a99ee7fae798887c792609904546013eba788045cd508a5f669f2c52e3479c122c18a5275c87af38237a1b5c9da17
Don't enable `-Werror` (in the CI) for compilers at least older than
our current release compiler (GCC 10). It provides little-to-no value,
other than turning compiler bugs & false positives into build failures,
and we aren't going to mutate perfectly fine/working code, for the sake
of avoid a warning that shouldn't even exist.
I also do not see the point of playing whack-a-mole and turning off various
warnings/trying to further work around the broken compiler, just to
acheive warningless builds for the sake of warningless builds.
One anecdote from "How SQLite Is Tested":
> Static analysis has found a few bugs in SQLite, but those are the
> exceptions. More bugs have been introduced into SQLite while trying
> to get it to compile without warnings than have been found by static
> analysis.
https://www.sqlite.org/testing.html.
6d58117a31 build: Build minisketch test in `make check`, not in `make` (Hennadii Stepanov)
Pull request description:
On master (d1e42659bb):
```
$ ./autogen.sh && ./configure --without-gui --disable-wallet && make clean
$ make 2>&1 | grep LD | grep -v .la
CXXLD bitcoind
CXXLD bitcoin-cli
CXXLD bitcoin-tx
CXXLD bitcoin-util
CXXLD test/test_bitcoin
CXXLD bench/bench_bitcoin
CXXLD minisketch/test
CXXLD test/fuzz/fuzz
CXXLD univalue/test/object
CXXLD univalue/test/unitester
$ make check 2>&1 | grep LD
CCLD exhaustive_tests
CCLD tests
```
With this PR:
```
$ ./autogen.sh && ./configure --without-gui --disable-wallet && make clean
$ make 2>&1 | grep LD | grep -v .la
CXXLD bitcoind
CXXLD bitcoin-cli
CXXLD bitcoin-tx
CXXLD bitcoin-util
CXXLD test/test_bitcoin
CXXLD bench/bench_bitcoin
CXXLD test/fuzz/fuzz
CXXLD univalue/test/object
CXXLD univalue/test/unitester
$ make check 2>&1 | grep LD
CXXLD minisketch/test
CCLD exhaustive_tests
CCLD tests
```
In fact, this PR restores behavior that was before bitcoin/bitcoin#22646, and that behavior looks more optimal.
As an outcome, the `contrib/guix/libexec/build.sh` does not spend resources to build binaries which are not a part of the release package.
ACKs for top commit:
TheCharlatan:
ACK 6d58117a31
Tree-SHA512: 4957c8f88a01aca005813bf4c1c26f433756bf68ea0c958481c638ead229fa8e23ecae3a8ac31ea555876ba6f2cc10ecd91caf2e2f664de5cb529ec05fb38fa7
faba08b5b4 refactor: Remove stray cs_main redundant declaration (MarcoFalke)
fa02591edf doc: Export threadsafety.h from sync.h (MarcoFalke)
Pull request description:
Looks like this was forgotten when introducing kernel/cs_main ?
Also, there is a commit to export threadsafety.h from sync.h.
ACKs for top commit:
hebasto:
ACK faba08b5b4
Tree-SHA512: 0aa58e7693b6fcd504f9da7339f8baa463a6407f67b27f68002db705f4642321ac3765f16c3d906c925ee24085591b79160a62fa5f4aaf6f2e5dcc788411800d
a3a2bd9e8a ci: Drop no longer needed package-specific flags (Hennadii Stepanov)
071eef1e97 build: Propagate user-defined flags to host packages (Hennadii Stepanov)
Pull request description:
On master (4f8b1f8759) `{CPP,C,CXX,LD}FLAGS` that are specified in the command line are not propagated to packages:
```
$ make --no-print-directory -C depends print-libevent_cxxflags CXXFLAGS=-some-fancy-flag
libevent_cxxflags=-pipe -O2
```
This PR:
- propagates `{CPP,C,CXX,LD}FLAGS` to host packages:
```
$ make --no-print-directory -C depends print-libevent_cxxflags CXXFLAGS=-some-fancy-flag
libevent_cxxflags= -some-fancy-flag
```
- does not propagate `{CPP,C,CXX,LD}FLAGS` to native packages:
```
$ make --no-print-directory -C depends print-native_b2_cxxflags CXXFLAGS=-some-fancy-flag
native_b2_cxxflags=
```
- actually addresses the https://github.com/bitcoin/bitcoin/pull/23551#issuecomment-973896518
ACKs for top commit:
TheCharlatan:
Code review ACK a3a2bd9e8a
Tree-SHA512: 243d6b1b0e9c5de46debc36de62a77b6b4d6f638940fd530040c219956ec624e321b0c25290fed164e3a8c88befa7b97b20f765d7b9a428c269b3720f21da099
b68e5a7fef lint: specify the right commit range when running locally (James O'Beirne)
dff7ed5732 test: add an easy way to run linters locally (James O'Beirne)
Pull request description:
Adds a Dockerfile configuration ~~(originally written mostly by fanquake)~~ that allows straightforward running of linters with compatible versions locally. This removes a ton of annoyance when trying to appease CI, because many of the linter versions are quite old and difficult to maintain locally.
I realize that people may not be thrilled to add more ancillary tooling to the repo, but I think this makes a lot of sense given the linter versions listed in this container configuration are dictated by this repo (within the CI configuration), so having these things live in two separate places is a recipe for version mismatches.
Eventually we can likely just use this container on CI directly to avoid any chance of inconsistencies between local dev experience and CI.
ACKs for top commit:
aureleoules:
ACK b68e5a7fef
stickies-v:
ACK b68e5a7fe
john-moffett:
ACK b68e5a7fef
Tree-SHA512: 7ef7a5dae023d81fdb6296d5d92dfa074ee321c7993e607c9f014d0f21c91558611aa00fc3ce1edc7b5e68371aea0d27fa1931291a79bb867a6c783bb536775f
Adds a Dockerfile configuration
that allows straightforward running of linters with compatible versions
locally. This removes a ton of annoyance when trying to appease CI,
because many of the linter versions are quite old and difficult to
maintain locally.
I realize that people may not be thrilled to more ancillary tooling to
the repo, but I think this makes a lot of sense given the linter
versions listed in this container configuration are dictated by this
repo (within the CI configuration), so having these things live in
two separate places is a recipe for version mismatches.
Eventually we can likely just use this container on CI directly to avoid
any chance of inconsistencies between local dev experience and CI.
fa87d71872 ci: Add missing lint dependency (MarcoFalke)
Pull request description:
Also, document each dependency.
Adding `gpg` avoids errors when running a release or dev branch in the CI:
01ec5308bf/ci/lint/06_script.sh (L30-L42)
```
bash: line 1: gpg: command not found
```
https://cirrus-ci.com/task/4582854860996608?logs=lint#L185
ACKs for top commit:
hebasto:
ACK fa87d71872
Tree-SHA512: 869a3d2feab764b2c8d47d481359680a1d2c54a33b13ca26c5f8ce56cf2f368d4c74637dcbc53fdbf323f10940965c1c0e592e2fb4ce725d5cd467e77e62b6e5
123043e99c ci: Bump lint task image to Ubuntu Jammy (Hennadii Stepanov)
9b86114058 ci: Use pyenv's `python-build` to install Python in lint task (Hennadii Stepanov)
Pull request description:
This PR:
- is an alternative to bitcoin/bitcoin#26581 and bitcoin/bitcoin#26637
- closesbitcoin/bitcoin#26548
Key advantages of this PR over others:
- it uses pyenv's `python-build` [standalone](https://github.com/pyenv/pyenv/tree/master/plugins/python-build#using-python-build-standalone)
- requires no additional computational resources
Note for testing. The lint task must success regardless of whether the `python_cache` is populated or invalidated.
ACKs for top commit:
MarcoFalke:
ACK 123043e99c
fanquake:
ACK 123043e99c
Tree-SHA512: ba0fcdd4f2939a59692b173dcd1f5704444cfcfbb8111538c6f8160056d0536bba250e4f9b0f8c66f8b454e52034bf36ffe6afae76cdc0f7cc5b58b576d790ba
DOCKER in names is confusingly used as synonym for "image", "container",
and "ci". Fix the confusion by picking the term that fits the context.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:$1:$2:g" $( git grep -l "$1" ) ; }
ren DOCKER_PACKAGES CI_BASE_PACKAGES
# This better reflects that they are the common base for all CI
# containers.
ren DOCKER_ID CI_CONTAINER_ID
# This is according to the documentation of "--detach , -d: Run
# container in background and print container ID".
ren DOCKER_NAME_TAG CI_IMAGE_NAME_TAG
# This avoids confusing with CONTAINER_NAME and clarifies that it is an
# image.
ren DOCKER_ADMIN CI_CONTAINER_CAP
# This clarifies that it is a capability added to the container.
ren DOCKER_CI_CMD_PREFIX CI_EXEC_CMD_PREFIX
# This brings it in line with the CI_EXEC naming.
-END VERIFY SCRIPT-
fa5cbf2290 ci: Properly set COMMIT_RANGE in lint task (MarcoFalke)
Pull request description:
Currently the variable holds (apart from the commits in the pull request) all commits to master since the pull was opened.
This is problematic, because already merged commits are linted in unrelated pulls, leading to:
* Wasted resources. For example, currently the lint task may take 9 minutes, when it should take 1. See https://cirrus-ci.com/task/6032782770569216?logs=lint#L1449
* False failures. For example, when a "wrong" commit is in master it can lead to some pulls failing unrelatedly, and others not.
Now that the CI has the `/merge` commit (since commit fad7281d78), `COMMIT_RANGE` can simply be set to `HEAD~..HEAD` to only hold the changes in the pull.
ACKs for top commit:
fanquake:
ACK fa5cbf2290
Tree-SHA512: e85fca4ca9d2615ddd2544403485e06885769a3f70bca297e23eefda2a1d28f47c5271f6adfa6ce0e5e972335c78098b76e0db4b109f59d0986bf508cef7528f
fabb6af850 ci: Remove duplicate CC and CXX from tsan task (MarcoFalke)
fa5d9a0e24 Revert "ci: Use clang-15 in tsan task" (MarcoFalke)
faa835e7e5 Revert "test: Drop no longer needed `race:epoll_ctl` TSan suppression" (MarcoFalke)
Pull request description:
Looks like there are still bugs in clang-15, so we need to roll back all the way to the previously used version (clang-13).
ACKs for top commit:
hebasto:
ACK fabb6af850, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: d62203049847ab9095ee3fc89e18bdd721d1d9d5a7ef7a9f524c80e6be58d1d9f6aa2f14533df1ea77eb59597fba6fa9b987b17eb03b2c3f7cb577ab59cd59c0
- Constructors of uint256 to utilize Span instead of requiring a std::vector
- converts m_data into a std::array
- Prefers using `WIDTH` instead of `sizeof(m_data)`
- make all the things constexpr
- replace C style functions with c++ equivalents
- memset -> std::fill
- memcpy -> std::copy
Note: In practice, implementations of std::copy avoid multiple assignments and use bulk copy functions such as std::memmove if the value type is TriviallyCopyable and the iterator types satisfy LegacyContiguousIterator. (https://en.cppreference.com/w/cpp/algorithm/copy)
- memcmp -> std::memcmp
2811f40f30 ci: only run USDT interface tests on CirrusCI (0xb10c)
Pull request description:
As mentioned in #26571, the task running the USDT interface tests fail when run in docker. cc7335edc8 in #25528 added that the tests are run in a **VM** in Cirrus CI. Running them locally in docker containers might not work:
- We use [bcc] as tracing toolkit which requires the kernel headers to compile the BPF bytecode. As docker containers use the hosts kernel and don't run their own, there is a potential for mismatches between kernel headers available in the container and the host kernel. This results in a failure loading the BPF byte code.
- Privilges are required to load the BPF byte code into the kernel. Normally, the docker containers aren't run with these.
- We currently use an untrusted third-party PPA to install the bpfcc-tools package on Ubuntu 22.04. Using this on a local dev system could be a security risk.
To not hinder the ASan + LSan + UBSan part of the CI task, the USDT tests are disabled on non-CirrusCI runs.
[bcc]: https://github.com/iovisor/bcc
Top commit has no ACKs.
Tree-SHA512: 7c159b59630b36d5ed927b501fa0ad6f54ff3d98d0902f975cc8122b4147a7f828175807d40a470a85f0f3b6eeb79307d3465a287dbd2f3d75b803769ad0b6e7
As mentioned in #26571, the task running the USDT interface tests
fail when run in docker. cc7335edc8
in #25528 added that the tests are run in a **VM** in Cirrus CI.
Running them locally in docker containers might not work:
- We use [bcc] as tracing toolkit which requires the kernel headers
to compile the BPF bytecode. As docker containers use the hosts
kernel and don't run their own, there is a potential for mismatches
between kernel headers available in the container and the host
kernel. This results in a failure loading the BPF byte code.
- Privilges are required to load the BPF byte code into the kernel.
Normally, the docker containers aren't run with these.
- We currently use an untrusted third-party PPA to install the
bpfcc-tools package on Ubuntu 22.04. Using this on a local dev
system could be a security risk.
To not hinder the ASan + LSan + UBSan part of the CI task, the USDT
tests are disabled on non-CirrusCI runs.
[bcc]: https://github.com/iovisor/bcc
fad1c55301 lint: Skip COMMIT_RANGE if no CIRRUS_PR (MarcoFalke)
Pull request description:
It doesn't make sense to run this for non-PRs, because:
* There are known whitespace "violations" in previous commits, so the lint may fail
* Once the changes are merged, it is too late to fix them up (force pushes are illegal)
* It isn't possible to determine which commits to run on if there is no reference branch (target branch of the pull request)
Moreover, the test fails on non-master:
* https://github.com/bitcoin/bitcoin/runs/8664441400
Fix all issues by skipping it.
ACKs for top commit:
hebasto:
ACK fad1c55301, also tested in my personal Cirrus account.
Tree-SHA512: be15f00e2b2a9069583833545883e0e5968a33d2455dad59e6fb47c1102b4dd16ef932e9ba945e29e9d941e6c17bd531a02c66b0491097801be6bda476875537
54dd8f51ce ci: use ci_exec_root for clang install (josibake)
Pull request description:
fixes a bug introduced in #25900 ; see https://github.com/bitcoin/bitcoin/pull/25900#issuecomment-1327311069
the general idea of #25900 was to use a non-root user as much as possible to avoid modifying the user's local filesystem. however, it appears the root user is needed to correctly install clang.
ACKs for top commit:
hebasto:
ACK 54dd8f51ce, tested on Ubuntu 22.04.
Tree-SHA512: beb01d4b6127fbba3c8d18e85cf7ec7d1b2ec93ea05c475ab51bcaa04ef1b0591d886f1a7e0732c5ae86806013f022c0b44027380d2b0cfb1bfdc843e40f99b4
849f20a6d3 ci: create and use non-root user for docker image (josibake)
Pull request description:
Previously, everything in the ci docker image ran as the root user. This would lead to certain directories (`ci/scratch`, `depends`) being owned by `root` after running the ci locally which would lead to annoying behavior such as subsequent guix builds failing due to `depends/` being owned by root.
This PR adds a non-root user in the container and chowns the mounted working directory. All the `docker exec` commands now run as the non-root user, except for the few that still need to run as root (mainly, installing packages).
To test this I checked out a fresh copy of the repo, applied my changes, ran the CI, and verified all the local file permissions were unchanged after the CI was finished running.
ACKs for top commit:
hebasto:
ACK 849f20a6d3, tested on Ubuntu 22.04 by running commands as follows:
Tree-SHA512: 734dca0f36157fce5fab243b4ff657fc17ba980e8e4e4644305f41002ff21bd5cef02c306ea1e0b5c841d4c07c095e8e4be16722e6a38c890717c60a3f5ec62a
b89530483d util: move threadinterrupt into util (fanquake)
Pull request description:
Alongside thread and threadnames. It's part of libbitcoin_util.
ACKs for top commit:
ryanofsky:
Code review ACK b89530483d. No changes since last review other than rebase
theuni:
ACK b89530483d.
Tree-SHA512: 0421f4d1881ec295272446804b27d16bf63e6b62b272f8bb52bfecde9ae6605e8109ed16294690d3e3ce4b15cc5e7c4046f99442df73adb10bdf069d3fb165aa
Running all commands as the root user in the docker image
will change local file permissions in the ci and depends directory.
Add a non-root user to the container and use this user whenever
possible when running docker exec commands.
da16893474 ci: Use `macos-ventura-xcode:14.1` image for "macOS native" task (Hennadii Stepanov)
702836530f ci: Make `getopt` path architecture agnostic (Hennadii Stepanov)
Pull request description:
The "macOS native" CI task always uses the recent OS image.
This PR updates it up to the recent macOS release.
Cirrus Labs [stopped](https://github.com/bitcoin/bitcoin/pull/25160#issuecomment-1162829773) updating macOS images for `x86_64`, therefore, an `arm64` image been used.
Also `make test-security-check` has been dropped as it ["isn't even expected to pass"](https://github.com/bitcoin/bitcoin/issues/26386#issuecomment-1290318628) on `arm64` in CI.
ACKs for top commit:
Sjors:
utACK da16893
Tree-SHA512: 36785d33b7f11b3cdbc53bcfbf97d88bf821fad248c825982dd9f8e3413809a4ef11190eaf950e60fdf479b62ff66920c35d9ea42d534723f015742eec7e19b6
fa6054e952 ci: Allow PIP_PACKAGES on centos (MacroFake)
fac085a05c ci: Remove unused package (MacroFake)
Pull request description:
This was added in 7fc5e865b9 but I can't see a reason why this should be forbidden.
This is also needed for other changes (bumping the minimum python version).
ACKs for top commit:
hebasto:
re-ACK fa6054e952
Tree-SHA512: e8ead9ee00079024eb1e8c6e7b31c78cf2a3392159b444765c2ea9a58bed2a7550bf71083210692a45bb8ed7896cb882b72bf70baa13a2384864b2b510b73005
21a9e94dbb ci: remove hardcoded tag list from ci scripts (josibake)
d530ba390e doc: update test/README.md (josibake)
614d4682ba script: default to necessary tags in get_previous_releases.py (josibake)
Pull request description:
Almost every time I need to use this script, I forget the tag list is needed and that a specific set of tags is needed for the backwards compatibility tests to work. I end up wasting time reading through the script and googling to find the tag list before remembering it is in `test/README.md`
I assume (hope) I'm not the only one this happens to, so I figured it would make more sense to have the script default to downloading/building the necessary tags. This has the added benefit of making the script the source of truth: the script already needs to be updated with the SHA256_SUM of the binary for every new tag that is added, so it makes sense to use `SHA256_SUMS` list as the necessary tag list. This means there is less risk of the README and the script drifting (i.e updating the readme with a new tag and forgetting to update the script, or updating the script and forgetting to update the README). Now all that needs to happen is to update the `SHA256_SUMS` list in the script and everything Just Works (TM)
ACKs for top commit:
Sjors:
re-tACK 21a9e94dbb
Tree-SHA512: 97b488227a89a6827584edd251820a7074fad75dfd7f26f1aa5f858e2521d2e02effd0f11e6dc4676e1155d3d5aba6ff94a4b58ffef80dc201376afd5927deb9
Also:
- Make DEFAULT_MAX_SIG_CACHE_SIZE into constexpr
DEFAULT_MAX_SIG_CACHE_BYTES to utilize the compile-time integer
arithmetic overflow checking available to constexpr.
- Fix comment (MiB instead of MB) for DEFAULT_MAX_SIG_CACHE_BYTES.
- Pass in max_size_bytes parameter to InitS*Cache(), modify log line to
no longer allude to maxsigcachesize being split evenly between the two
validation caches.
- Fix possible integer truncation and add a comment.
[META] I've kept the integer types as int64_t in order to not introduce
unintended behaviour changes, in the next commit we will make
them size_t.
It is part of the node library. Also, it won't be moved to the kernel
lib, as it will be pruned of ArgsManager.
-BEGIN VERIFY SCRIPT-
# Move module
git mv src/mempool_args.cpp src/node/
git mv src/mempool_args.h src/node/
# Replacements
sed -i 's:mempool_args\.h:node/mempool_args.h:g' $(git grep -l mempool_args)
sed -i 's:mempool_args\.cpp:node/mempool_args.cpp:g' $(git grep -l mempool_args)
sed -i 's:MEMPOOL_ARGS_H:NODE_MEMPOOL_ARGS_H:g' $(git grep -l MEMPOOL_ARGS_H)
-END VERIFY SCRIPT-
cc7335edc8 ci: run USDT interface test in a VM (0xb10c)
dba6f82342 test: adopt USDT utxocache interface tests (0xb10c)
220a5a2841 test: hook into PID in tracing tests (0xb10c)
Pull request description:
Changes a CI task that runs test the previously not run `test/functional/interface_usdt_*.py` functional tests (added in https://github.com/bitcoin/bitcoin/pull/24358).
This task is run as CirussCI `compute_engine_instance` VM as hooking into the tracepoints is not possible in CirrusCI docker containers (https://github.com/bitcoin/bitcoin/issues/23296#issuecomment-1024920845). We use an unoffical PPA and untrusted `bpfcc-tools` package in the CI as the Ubuntu jammy and Debian bullseye packages are outdated. We hope use an official package when new Ubuntu/Debian releases are available for the use with Google Compute Engine.
We make sure to hook into `bitcoind` binaries in USDT interface tests via their PID, instead of their path. This makes sure multiple functional tests running in parallel don't interfere with each other.
The utxocache USDT interface tests is adopted to a change of the functional test framework that wasn't detected as the tests weren't run in the CI. As the tracepoints expose internals, it can happen that we need to adopt the interface test when internals change. This is a bit awkward, and if it happens to frequently, we should consider generalizing the tests a bit more. For now it's fine, I think.
See the individual commit messages for more details on the changes.
Fixes https://github.com/bitcoin/bitcoin/issues/24782
Fixes https://github.com/bitcoin/bitcoin/issues/23296
I'd like to hear from reviewers:
- Are we OK with using the [`hadret/bpfcc`](https://launchpad.net/~hadret/+archive/ubuntu/bpfcc) PPA for now? There is a clear plan when to drop it and as is currently, it could only impact the newly added VM task.
- ~~Adding a new task increases CI runtime and costs. Should an existing `container` CI task be ported to a VM and reused instead?~~ Yes, see https://github.com/bitcoin/bitcoin/pull/25528#issuecomment-1179509525
ACKs for top commit:
MarcoFalke:
cr ACK cc7335edc8
Tree-SHA512: b7fddccc0a77d82371229d048abe0bf2c4ccaa45906497ef3040cf99e7f05561890aef4c253c40e4afc96bb838c9787fae81c8454c6fd9db583276e005a4ccb3
fa74e726c4 refactor: Make FEELER_SLEEP_WINDOW type safe (std::chrono) (MacroFake)
fa3b3cb9b5 Expose underlying clock in CThreadInterrupt (MacroFake)
Pull request description:
This gets rid of the `value*1000` manual conversion.
ACKs for top commit:
naumenkogs:
utACK fa74e726c4
dergoegge:
Code review ACK fa74e726c4
Tree-SHA512: 90409c05c25f0dd2f1c4dead78f707ebfd78b7d84ea4db9fcefd9c4958a1a3338ac657cd9e99eb8b47d52d4485fa3c947dce4ee1559fb56ae65878685e1ed9a3
cb3e9a1e3f Move {Load,Dump}Mempool to kernel namespace (Carl Dong)
aa30676541 Move DEFAULT_PERSIST_MEMPOOL out of libbitcoinkernel (Carl Dong)
06b88ffb8a LoadMempool: Pass in load_path, stop using gArgs (Carl Dong)
b857ac60d9 test/fuzz: Invoke LoadMempool via CChainState (Carl Dong)
b3267258b0 Move FopenFn to fsbridge namespace (Carl Dong)
ae1e8e3756 mempool: Use NodeClock+friends for LoadMempool (Carl Dong)
f9e8e5719f mempool: Improve comments for [GS]etLoadTried (Carl Dong)
813962da0b scripted-diff: Rename m_is_loaded -> m_load_tried (Carl Dong)
413f4bb52b DumpMempool: Pass in dump_path, stop using gArgs (Carl Dong)
bd4407817e DumpMempool: Use std::chrono instead of weird int64_t arthmetics (Carl Dong)
c84390b741 test/mempool_persist: Test manual savemempool when -persistmempool=0 (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
-----
This PR moves `{Dump,Load}Mempool` into its own `kernel/mempool_persist` module and introduces `ArgsManager` `node::` helpers in `node/mempool_persist_args`to remove the scattered calls to `GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)`.
More context can be gleaned from the commit messages.
-----
One thing I was reflecting on as I wrote this was that in the long run, I think we should probably invert the validation <-> mempool relationship. Instead of mempool not depending on validation, it might make more sense to have validation not depend on mempool. Not super urgent since `libbitcoinkernel` will include both validation and mempool, but perhaps something for the future.
ACKs for top commit:
glozow:
re ACK cb3e9a1e3f via `git range-diff 7ae032e...cb3e9a1`
MarcoFalke:
ACK cb3e9a1e3f🔒
ryanofsky:
Code review ACK cb3e9a1e3f
Tree-SHA512: 979d7237c3abb5a1dd9b5ad3dbf3b954f906a6d8320ed7b923557f41a4472deccae3e8a6bca0018c8e7a3c4a93afecc502acd1e26756f2054f157f1c0edd939d
Overloading sleep_for is not needed, as
* seconds and minutes can be converted to milliseconds by the compiler,
not needing a duration_cast
* std::condition_variable::wait_for will convert milliseconds to the
duration type of the underlying clock
So simply expose the clock.
Our CI tasks are run by CirrusCI in Docker containers in a Google
Compute Engine based Kubernetes environment. These containers have
limited capabilities - especially CAP_SYS_ADMIN is missing. See
https://github.com/bitcoin/bitcoin/issues/23296#issuecomment-1024920845
We need elevated privileges to hook into the USDT tracepoints. We use a
CirrusCI "compute_engine_instance" (a VM, not a container) where we have
the required privileges. The ubunut-mininmal-2204-lts was choosen with
debian-11 being an alternative. Both pack an outdated 'bpfcc-tools'
package (v0.18.0) from 2020. This version prints warnings to stderr
during BPF bytecode compilation, which causes our functional test runner
to fail. This is fixed in newer verison.
Until debian-12 or a newer Ubuntu release is avaliable as image in GCE
(https://cloud.google.com/compute/docs/images/os-details), we use a
third-party and untrusted PPA that releases up-to-date versions of the
package.
The official iovisor (authors of BCC) PPA is outdated too. An
alternative would be to compile BCC from source in the CI.
Co-authored-by: MacroFake <falke.marco@gmail.com>
These files change infrequently, and not much header shuffling is required.
We don't add everything in src/util/ yet, because IWYU makes some
dubious suggestions, which I'm going to follow up with upstream.
6eb0909cb7 fuzz: add low-level target for txorphanage (chinggg)
Pull request description:
This adds a low-level fuzz target for orphan transaction handling by creating random transactions and calling all functions in `TxOrphanage`.
It cannot simulate real-world `orphan/unorphan` scenarios effectively since it does not maintain any state about the node and the chain. A high-level fuzz target which construct well-designed transaction graphs will be added later.
ACKs for top commit:
MarcoFalke:
review ACK 6eb0909cb7🐈
Tree-SHA512: b4d64f5941df77d13981f75ec170cef6ffabe782797c982ede7f34134be01dc0026dd7c0bee614bc1d64715e90a933d2a8c95974d402e32eaba8e24cc928299e
Mostly changes to remove src/univalue exceptions from the various linters,
and the required code changes to make them happy. As well as minor doc
changes.
fa4068b4e2 Move minRelayTxFee to policy/settings (MacroFake)
Pull request description:
Seems a bit confusing to put policy stuff into validation, so fix that.
Also fix includes via `iwyu`.
ACKs for top commit:
ariard:
ACK fa4068b, the includes move compiles well locally.
ryanofsky:
Code review ACK fa4068b4e2. Make sense to move the global variable to policy/settings and the default constant to policy/policy. Ariard points out other constants that could be moved, which seems fine, but it seems like moving the global variable to be with other related global variables is more significant.
Tree-SHA512: adf9619002610d1877f3aef0a9e6115fc4c2ad64135a3e5100824c650b560c47f47ac28894c6214a50a7888355252a9f6f7cec98c23a771a1964160ef1ca77de
f26a496dfd test: clean up all-lint.py (Martin Leitner-Ankerl)
64d72c4c87 test: rename lint-all.py to all-lint.py (Martin Leitner-Ankerl)
Pull request description:
When running `./test/lint/lint-all.py`, the script runs all tests but
also calls itself because the comparison with `__file__` doesn't work.
Comparing resolved paths gives reliable comparison, and lint-all.py doesn't call itself any more
ACKs for top commit:
laanwj:
Code review ACK f26a496dfd
Tree-SHA512: b44abdd685f7b48a6a9f48e96d97138b635c31c1c7ab543cb5636b5f49690ccd56fa6fec01ae7fcc16af01a613372ee77632f70c32059919b373aa8051953791
fa753abd7c rpc: Move fee estimation RPCs to separate file (MacroFake)
Pull request description:
Fee estimation is generally used by wallets when creating txs. It doesn't have anything to do with creating or submitting blocks.
ACKs for top commit:
pk-b2:
ACK fa753abd7c
brunoerg:
crACK fa753abd7c
Tree-SHA512: 81e0edc936198a0baf0f5bfa8cfedc12db51759c7873bb0082dfc5f0040d7f275b35f639c6f5b86fa1ea03397b0d5e757c2ce1b6b16f1029880a39b9c3aaceda
fa60169811 rpc: Move signmessage RPC util to new file (MacroFake)
fa9425177e Remove cs_main from verifymessage (MacroFake)
Pull request description:
The `verifymessage` RPC has several issues:
* It takes `cs_main` for no reason, blocking progress on removing the `cs_main` global mutex.
* It is located in a file called `misc`, which is not a very helpful name.
Fix all issues.
ACKs for top commit:
vincenzopalazzo:
ACK fa60169811
Tree-SHA512: c71a1f481b828e0a544405fecbbc7ca44e66ea46b498d7aed1f1c584d6a99724deb13e89d90b9d5cdeecbce293e6a41e9f7ae299543f6d761bf9e7a839b6c7f3
035fa1f07a build: Remove LIBTOOL_APP_LDFLAGS for bitcoin-chainstate (Cory Fields)
3f0595095d docs: Add libbitcoinkernel_la_SOURCES explanation (Carl Dong)
94ad45deb2 ci: Build libbitcoinkernel (Carl Dong)
26b2e7ffb3 build: Extract the libbitcoinkernel library (Carl Dong)
1df44dd20c b-cs: Define G_TRANSLATION_FUN in bitcoinkernel.cpp (Carl Dong)
83a0bb7cc9 build: Separate lib_LTLIBRARIES initialization (Carl Dong)
c1e16cb31f build: Create .la library for bitcoincrypto (Carl Dong)
8bdfe057c7 build: Create .la library for leveldb (Carl Dong)
05d1525b6d build: Create .la library for crc32c (Carl Dong)
64caf94479 build: Remove vestigial LIBLEVELDB_SSE42 (Carl Dong)
1392e8e2d8 build: Don't add unrelated libs to LIBTEST_* (Carl Dong)
Pull request description:
Part of: #24303
This PR introduces a `libbitcoinkernel` static library linking in the minimal list of files necessary to use our consensus engine as-is. `bitcoin-chainstate` introduced in #24304 now will link against `libbitcoinkernel`.
Most of the changes are related to the build system.
Please read the commit messages for more details.
ACKs for top commit:
theuni:
This may be my favorite PR ever. It's a privilege to ACK 035fa1f07a.
Tree-SHA512: b755edc3471c7c1098847e9b16ab182a6abb7582563d9da516de376a770ac7543c6fdb24238ddd4d3d2d458f905a0c0614b8667aab182aa7e6b80c1cca7090bc
29f44fed36 Converting `lint-all.sh` to `lint-all.py`. (hiago)
Pull request description:
This PR is converting `test/lint/lint-all.sh` to `test/lint/lint-assertions.py`. It's an item of https://github.com/bitcoin/bitcoin/issues/24783.
ACKs for top commit:
laanwj:
Tested ACK 29f44fed36
Tree-SHA512: 5427936aaa8e009613048448cbd79d9225675bdcadcf4fbb70fd091e0aab85e350ef1678da1b388f70d62ca16f40409409f90da3f6bbf057480522cd50fde811
9b0a13a289 tidy: Add include-what-you-use (fanquake)
74cd038e30 refactor: fix includes in src/init (fanquake)
c79ad935f0 refactor: fix includes in src/compat (fanquake)
Pull request description:
We recently added a [`clang-tidy` job](https://github.com/bitcoin/bitcoin/blob/master/ci/test/00_setup_env_native_tidy.sh) to the CI, which generates a compilation database. We can leverage that now existing database to begin running [include-what-you-use](https://include-what-you-use.org/) over the codebase.
This PR demonstrates using a mapping_file to indicate fixups / includes that may differ from IWYU suggestions. In this case, I've added some fixups for glibc includes that I've [upstreamed changes for](https://github.com/include-what-you-use/include-what-you-use/pull/1026):
```bash
# Fixups / upstreamed changes
[
{ include: [ "<bits/termios-c_lflag.h>", private, "<termios.h>", public ] },
{ include: [ "<bits/termios-struct.h>", private, "<termios.h>", public ] },
{ include: [ "<bits/termios-tcflow.h>", private, "<termios.h>", public ] },
]
```
The include "fixing" commits of this PR:
* Adds missing includes.
* Swaps C headers for their C++ counterparts.
* Removes the pointless / unmaintainable `//for abc, xyz` comments. When using IWYU, if anyone wants to see / generate those comments, to see why something is included, it is trivial to do so (IWYU outputs them by default). i.e:
```cpp
// The full include-list for compat/stdin.cpp:
#include <compat/stdin.h>
#include <poll.h> // for poll, pollfd, POLLIN
#include <termios.h> // for tcgetattr, tcsetattr
#include <unistd.h> // for isatty, STDIN_FILENO
```
TODO:
- [ ] Qt mapping_file. There is one in the IWYU repo, but it's for Qt 5.11. Needs testing.
- [ ] Boost mapping_file. There is one in the IWYU repo, but it's for Boost 1.75. Needs testing.
I'm not suggesting we turn this on the for entire codebase, or immediately go-nuts refactoring all includes. However I think our dependency includes are now slim enough, and our CI infrastructure in place such that we can start doing this in some capacity, and just automate away include fixups / refactorings etc.
ACKs for top commit:
MarcoFalke:
review ACK 9b0a13a289
jonatack:
ACK 9b0a13a289 reviewed changes and run CI output in https://cirrus-ci.com/task/4750910332076032
Tree-SHA512: 00beab5a5f2a6fc179abf08321a15391ecccaa91ab56f3c50c511e7b29a0d7c95d8bb43eac2c31489711086f6f77319d43d803cf8ea458e7cd234a780d9ae69e
fa9112aac0 Remove utxo db upgrade code (MarcoFalke)
Pull request description:
It is not possible to upgrade Bitcoin Core pre-segwit (pre-0.13.1) to a recent version without a full IBD from scratch after commit 19a56d1519 (released in version 22.0).
Any Bitcoin Core version with the new database format after commit 1088b02f0c (released in version 0.15), can upgrade to any version that is supported as of today.
This leaves the versions 0.13.1-0.14.x. Even though those versions are unsupported, some users with an existing datadir may want to upgrade to a recent version. However, it seems reasonable to simply ask them to `-reindex` to run a full IBD from scratch. This allows us to remove the utxo db upgrade code.
ACKs for top commit:
Sjors:
re-ACK fa9112aac0
laanwj:
Code review ACK fa9112aac0
Tree-SHA512: 4243bb35df9ac4892f9fad30fe486d338745952bcff4160bcb0937c772d57b13b800647da14695e21e3655e85ee0d95fa3dc7789ee309d59ad84f422297fecb8
fa73f8a469 ci: Use clang-12 and libcxx-12 for msan (MarcoFalke)
Pull request description:
Run the latest sanitizers to get the most implemented features
ACKs for top commit:
fanquake:
ACK fa73f8a469 - `--disable-hardening` matches what was just added to oss-fuzz.
Tree-SHA512: 2e533bb9273c97600176be2e41069a03f425aa586f9f32b8ed5f0c9844215a3a41e95a8edd58d044386e350807d6a1df09008a7da35428abd185a509ca71bd82
fa76b2fbe3 ci: Avoid boost test warnings (MarcoFalke)
Pull request description:
This removes boost test warnings from the CI log.
For example https://cirrus-ci.com/task/6029158399606784?logs=ci#L4060:
```
Boost.Test WARNING: token "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin-addrman_tests.o" does not correspond to the Boost.Test argument
and should be placed after all Boost.Test arguments and the -- separator.
For example: test_bitcoin --random -- /tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin-addrman_tests.o
Boost.Test WARNING: token "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin-allocator_tests.o" does not correspond to the Boost.Test argument
and should be placed after all Boost.Test arguments and the -- separator.
For example: test_bitcoin --random -- /tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin-allocator_tests.o
Boost.Test WARNING: token "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin-amount_tests.o" does not correspond to the Boost.Test argument
and should be placed after all Boost.Test arguments and the -- separator.
...
ACKs for top commit:
fanquake:
ACK fa76b2fbe3 - checked that tests are also still being run in other jobs.
Tree-SHA512: c00fe1242ee6b9ef92d511f1e86305c1731d8894bb89ec8fcdf30069959831483933a93a6bfd33d9f2d15706bdeedbba29e82c409eed266abfc57aabfdaf4841
fae20e6b50 Revert "Avoid the use of P0083R3 std::set::merge" (MarcoFalke)
fab53b5fd4 ci/doc: Set minimum required clang/libc++ version to 8.0 (MarcoFalke)
Pull request description:
This is not for 23.0, but for 24.0. It comes with the following benefits:
* Can use C++17 P0083R3 std::set::merge from libc++ 8.0
* No longer need to provide support for clang-7, which already fails to compile on some architectures (https://github.com/bitcoin/bitcoin/issues/21294#issuecomment-998098483)
This should be fine, given that all supported operating systems ship with at least clang-10:
* CentOS 8: clang-12
* Stretch: https://packages.debian.org/stretch/clang-11
* Buster: https://packages.debian.org/buster-backports/clang-11
* Bionic: https://packages.ubuntu.com/bionic-updates/clang-10
* Focal: https://packages.ubuntu.com/focal/clang-10
ACKs for top commit:
fanquake:
ACK fae20e6b50 - I think this is fine to do. I would be surprised if in another 6 months time someone was stuck on a system we supported, needing to compile Core, and only had access to Clang 7 or older. As mentioned in the PR description, all systems we currently support, already support multiple newer versions of Clang.
hebasto:
ACK fae20e6b50, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 3b4c6c130ff40dd7e84934af076863415e5dd661d823c72e3e3832566c65be6e877a7ef9164bbcf394bcea4b897fc29a48db0f231c22ace0e2c9b5638659a628
Self-compiled BDB was added to this job as opposed to using depends BDB
due to linking issues, however the compiled BDB is not actually used.
Remove it for now, given we don't actually lose any coverage (note that
BDB is also no used the MSAN fuzz job), and in future, we can use
depends BDB.
2c03cec2ff ci: Build bitcoin-chainstate (Carl Dong)
095aa6ca37 build: Add example bitcoin-chainstate executable (Carl Dong)
Pull request description:
Part of: #24303
This PR introduces an example/demo `bitcoin-chainstate` executable using said library which can print out information about a datadir and take in new blocks on stdin.
Please read the commit messages for more details.
-----
#### You may ask: WTF?! Why is `index/*.cpp`, etc. being linked in?
This PR is meant only to capture the state of dependencies in our consensus engine as of right now. There are many things to decouple from consensus, which will be done in subsequent PRs. Listing the files out right now in `bitcoin_chainstate_SOURCES` is purely to give us a clear picture of the task at hand, it is **not** to say that these dependencies _belongs_ there in any way.
### TODO
1. Clean up `bitcoin-chainstate.cpp`
It is quite ugly, with a lot of comments I've left for myself, I should clean it up to the best of my abilities (the ugliness of our init/shutdown might be the upper bound on cleanliness here...)
ACKs for top commit:
ajtowns:
ACK 2c03cec2ff
ryanofsky:
Code review ACK 2c03cec2ff. Just rebase, comments, formatting change since last review
MarcoFalke:
re-ACK 2c03cec2ff 🏔
Tree-SHA512: 86e7fb5718caa577df8abc8288c754f4a590650d974df9d2f6476c87ed25c70f923c4db651c6963f33498fc7a3a31f6692b9a75cbc996bf4888c5dac2f34a13b
774323e378 ci: Force `--enable-external-signer` to prevent future regressions (Hennadii Stepanov)
69978858a4 build: Fix Boost.Process check for Boost 1.73 and older (Hennadii Stepanov)
2199ef79cb build: Fix a non-portable use of `test` (Hennadii Stepanov)
d436c488d4 build, refactor: Replace tabs with spaces (Hennadii Stepanov)
Pull request description:
On master (5f44c5c428) Boost.Process check false fails without the `-lpthread` flag.
```
$ grep -C 2 pthread_detach config.log
/usr/bin/ld: /tmp/cczCQfQv.o: in function `boost::asio::detail::posix_global_impl<boost::asio::system_context>::~posix_global_impl()':
conftest.cpp:(.text._ZN5boost4asio6detail17posix_global_implINS0_14system_contextEED2Ev[_ZN5boost4asio6detail17posix_global_implINS0_14system_contextEED5Ev]+0xa3): undefined reference to `pthread_join'
/usr/bin/ld: conftest.cpp:(.text._ZN5boost4asio6detail17posix_global_implINS0_14system_contextEED2Ev[_ZN5boost4asio6detail17posix_global_implINS0_14system_contextEED5Ev]+0xc4): undefined reference to `pthread_detach'
collect2: error: ld returned 1 exit status
configure:26674: $? = 1
```
Not required for Boost 1.74+.
ACKs for top commit:
laanwj:
Code review ACK 774323e378, is a bugfix/workaround, seems fine to merge last minute for 23.0.
Tree-SHA512: 2a9d4b67fd8910e107af972d8c223286b7c933bc310616f86c8b6d8c903438916980fc76bd7e37f2698f6ce5361dc706cbf2933d1ac2c081bcabe1b83ca7d6b6
fa27745ccb ci: Bump fuzz tasks to jammy (MarcoFalke)
fab8cd5f87 Revert "ci: Run fuzzer task for the master branch only" (MarcoFalke)
Pull request description:
This reverts commit 5a9e255e5a.
I think we should attempt to maintain the fuzz tasks for release branches as well.
If it is too difficult for one branch, it could make sense to disable it for that branch, but not for all branches unconditionally.
Also, bump to jammy.
ACKs for top commit:
fanquake:
ACK fa27745ccb - we'll see how we go with the 23.x release branch.
Tree-SHA512: d6d08e7dce0884b556c51ff1896aebbbb5a805c22decd58af81a04192d19876978696017b489ec55886ddfd5c022963baaab5f11022369ae5291016826ff8017
a1515cdd96 ci: use Ubuntu Jammy for Windows CI (fanquake)
Pull request description:
This means we'll compile using [GCC 10.3.x](https://packages.ubuntu.com/jammy/g++-mingw-w64) and [mingw-w64 8.0.0](https://packages.ubuntu.com/jammy/mingw-w64) which better matches our Guix release environment.
ACKs for top commit:
MarcoFalke:
cr ACK a1515cdd96
hebasto:
ACK a1515cdd96, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: a57cce1874324c9dd00e5d8989996d214facbdd561440471c15e6cc1808bca1c6fd758abe7a1b87378b2e7f9c25e7c9d8242df911cd1ef6cfbe49718adc3be5d
...to make sure that the linker errors that arise from coupling
regressions are caught by CI.
Adding to the "no wallet" ci job as suggested by MarcoFalke.
This change is a prerequisite for the following bumping Qt minimum
version to 5.11.3. It is required as bionic has Qt 5.9.5.
Effectively, this also changes:
- gcc from 8.4.0 to 8.3.0
- python from 3.6.5 to 3.7.3
1362d6173f scripted-diff: Insert missed copyright headers (Hennadii Stepanov)
f47dda2c58 scripted-diff: Bump copyright headers (Hennadii Stepanov)
c29105efdc script: Fix copyright_header.py (Hennadii Stepanov)
Pull request description:
This PR is an alternative to #23903.
It bumps the existing copyright headers as we did every year, and adds the missed copyright headers.
A small fix has been applied to the `copyright_header.py` in order to prevent such weird bumping as `2021` --> `2021-2017`.
ACKs for top commit:
MarcoFalke:
ACK 1362d6173f
Tree-SHA512: 204d970fe8c51546b26b8f03fe4297db8a9bef5101df851540b7b9eddbd3a09677ee81fdd882c60937d732407f42c9883165bd978272200cff8f90190f075905
Our minimum required GCC is GCC 8, and this change in required for
changes like #23839 which take advantage of flags introduced in that
version of GCC.
This should have been included as part of
182de7ba10.
2f356a0ca8 scripted-diff: Drop Darwin version for better maintainability (Hennadii Stepanov)
Pull request description:
After this PR, any macOS tools version bumping in the future will touch fewer files in the repo.
Pointing a Darwin version for the `--host` system does not matter for the following reasons:
- in terms of the resulted binaries, we should only care about the minimum supported macOS version which is a separated parameter in our build system.
- in terms of the build system itself, the usage of the `$(host)` variable is self-consistent enough. Btw `$(host_os)` value already has the version dropped:
```
$ make -C depends --no-print-directory print-host_os HOST=x86_64-apple-darwin19
host_os=darwin
```
ACKs for top commit:
gruve-p:
ACK 2f356a0ca8
promag:
ACK 2f356a0ca8.
fanquake:
ACK 2f356a0ca8
Tree-SHA512: 374896ab0ba02b0d8b4b21431fe963bd213b0d09586e0898c13a4c5fa294c1b693f1b2c92880c245c4157c14217b4825b36522f461930477f4d2a727086ebb2a
fafa66e424 ci: Replace soon EOL hirsute with jammy (MarcoFalke)
Pull request description:
`hirsute` will be EOL in about 1.5 months, at which point the package servers may be shut down. Avoid this by hopping to the next LTS release 22.04 (Jammy Jellyfish).
While the release is currently in development, it seems unlikely that anything will break for us. I am doing the hirsute->jammy hop to avoid a hirsute->impish->jammy hop, but if anything does break, we can fall back to that "double hop".
ACKs for top commit:
fanquake:
ACK fafa66e424
Zero-1729:
crACK fafa66e424
Tree-SHA512: a3626b72519c7f54f260477d04265321af7aefe25cc2a7d653dba77f79caca10db3a6aa4249a934184dcdc99832f6b4c6e50330d7630e58720ab0aba3624ab8a
29173d6c6c ubsan: add minisketch exceptions (Cory Fields)
54b5e1aeab Add thin Minisketch wrapper to pick best implementation (Pieter Wuille)
ee9dc71c1b Add basic minisketch tests (Pieter Wuille)
0659f12b13 Add minisketch dependency (Gleb Naumenko)
0eb7928ab8 Add MSVC build configuration for libminisketch (Pieter Wuille)
8bc166d5b1 build: add minisketch build file and include it (Cory Fields)
b2904ceb85 build: add configure checks for minisketch (Cory Fields)
b6487dc4ef Squashed 'src/minisketch/' content from commit 89629eb2c7 (fanquake)
Pull request description:
This takes over #21859, which has [recently switched](https://github.com/bitcoin/bitcoin/pull/21859#issuecomment-921899200) to my integration branch. A few more build issues came up (and have been fixed) since, and after discussing with sipa it was decided I would open a PR to shepherd any final changes through.
> This adds a `src/minisketch` subtree, taken from the master branch of https://github.com/sipa/minisketch, to prepare for Erlay implementation (see #21515). It gets configured for just supporting 32-bit fields (the only ones we're interested in in the context of Erlay), and some code on top is added:
> * A very basic unit test (just to make sure compilation & running works; actual correctness checking is done through minisketch's own tests).
> * A wrapper in `minisketchwrapper.{cpp,h}` that runs a benchmark to determine which field implementation to use.
Only changes since my last update to the branch in the previous PR have been rebasing on master and fixing an issue with a header in an introduced file.
ACKs for top commit:
naumenkogs:
ACK 29173d6c6c
Tree-SHA512: 1217d3228db1dd0de12c2919314e1c3626c18a416cf6291fec99d37e34fb6eec8e28d9e9fb935f8590273b8836cbadac313a15f05b4fd9f9d3024c8ce2c80d02
095f07744c ci: Do not print `git log` for empty COMMIT_RANGE (Hennadii Stepanov)
Pull request description:
On master (77a2f5d30c) a CI lint task [log](https://api.cirrus-ci.com/v1/task/4817858858319872/logs/lint.log) exceeds 20K lines.
This PR fixes this issue.
ACKs for top commit:
MarcoFalke:
cr ACK 095f07744c
Tree-SHA512: 89180018aeccf1599cdf218924cbab12dcbae0f6674bb90e13b64e342cdd908a880b885039c23f0d1d03493e55a94fe04abf39481616ae6550c6a759f5ca9a35
0f95247246 Integrate univalue into our buildsystem (Cory Fields)
9b49ed656f Squashed 'src/univalue/' changes from 98fadc0909..a44caf65fe (fanquake)
Pull request description:
This PR more tightly integrates building Univalue into our build system. This follows the same approach we use for [LevelDB](https://github.com/bitcoin-core/leveldb/), ([`Makefile.leveldb.include`](https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.leveldb.include)), and [CRC32C](https://github.com/bitcoin-core/crc32c) ([`Makefile.crc32c.include`](https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.crc32c.include)), and will be the same approach we use for [minisketch](https://github.com/sipa/minisketch); see #23114.
This approach yields a number of benefits, including:
* Faster configuration due to one less subconfigure being run during `./configure` i.e 22s with this PR vs 26s
* Faster autoconf i.e 13s with this PR vs 17s
* Improved caching
* No more issues with compiler flags i.e https://github.com/bitcoin/bitcoin/pull/12467
* More direct control means we can build exactly the objects we want
There might be one argument against making this change, which is that builders should have the option to use "proper shared/system libraries". However, I think that falls down for a few reasons. The first being that we already don't support building with a number of system libraries (secp256k1, leveldb, crc32c); some for good reason. Univalue is really the odd one out at the moment.
Note that the only fork of Core I'm aware of, that actively patches in support for using system libs, also explicitly marks them as ["DANGEROUS"](a886811721/configure.ac (L1430)) and ["NOT SUPPORTED"](a886811721/configure.ac (L1312)). So it would seem they exist more to satisfy a distro requirement, as opposed to something that anyone should, or would actually use in practice.
PRs like #22412 highlight the "issue" with us operating with our own Univalue fork, where we actively fix bugs, and make improvements, when upstream (https://github.com/jgarzik/univalue) may not be taking those improvements, and by all accounts, is not currently actively maintained. Bitcoin Core should not be hamstrung into not being able to fix bugs in a library, and/or have to litter our source with "workarounds", i.e #22412, for bugs we've already fixed, based on the fact that an upstream project is not actively being maintained. Allowing builders to use system libs is really only exacerbating this problem, with little benefit to our project. Bitcoin Core is not quite like your average piece of distro packaged software.
There is the potential for us to give the same treatment to libsecp256k1, however it seems doing that is currently less straightforward.
ACKs for top commit:
dongcarl:
ACK 0f95247246 less my comment above, always nice to have an include-able `sources.mk` which makes integration easier.
theuni:
ACK 0f95247246. Thanks fanquake for keeping this going.
Tree-SHA512: a7f2e41ee7cba06ae72388638e86b264eca1b9a8b81c15d1d7b45df960c88c3b91578b4ade020f8cc61d75cf8d16914575f9a78fa4cef9c12be63504ed804b99
This addresses issues like the one in #12467, where some of our compiler flags
end up being dropped during the subconfigure of Univalue. Specifically, we're
still using the compiler-default c++ version rather than forcing c++17.
We can drop the need subconfigure completely in favor of a tighter build
integration, where the sources are listed separately from the build recipes,
so that they may be included directly by upstream projects. This is
similar to the way leveldb build integration works in Core.
Core benefits of this approach include:
- Better caching (for ex. ccache and autoconf)
- No need for a slow subconfigure
- Faster autoconf
- No more missing compile flags
- Compile only the objects needed
There are no benefits to Univalue itself that I can think of. These changes
should be a no-op there, and to downstreams as well until they take advantage
of the new sources.mk.
This also removes the option to use an external univalue to avoid similar ABI
issues with mystery binaries.
Co-authored-by: fanquake <fanquake@gmail.com>
4747da3a5b Add syscall sandboxing (seccomp-bpf) (practicalswift)
Pull request description:
Add experimental syscall sandboxing using seccomp-bpf (Linux secure computing mode).
Enable filtering of system calls using seccomp-bpf: allow only explicitly allowlisted (expected) syscalls to be called.
The syscall sandboxing implemented in this PR is an experimental feature currently available only under Linux x86-64.
To enable the experimental syscall sandbox the `-sandbox=<mode>` option must be passed to `bitcoind`:
```
-sandbox=<mode>
Use the experimental syscall sandbox in the specified mode
(-sandbox=log-and-abort or -sandbox=abort). Allow only expected
syscalls to be used by bitcoind. Note that this is an
experimental new feature that may cause bitcoind to exit or crash
unexpectedly: use with caution. In the "log-and-abort" mode the
invocation of an unexpected syscall results in a debug handler
being invoked which will log the incident and terminate the
program (without executing the unexpected syscall). In the
"abort" mode the invocation of an unexpected syscall results in
the entire process being killed immediately by the kernel without
executing the unexpected syscall.
```
The allowed syscalls are defined on a per thread basis.
I've used this feature since summer 2020 and I find it to be a helpful testing/debugging addition which makes it much easier to reason about the actual capabilities required of each type of thread in Bitcoin Core.
---
Quick start guide:
```
$ ./configure
$ src/bitcoind -regtest -debug=util -sandbox=log-and-abort
…
2021-06-09T12:34:56Z Experimental syscall sandbox enabled (-sandbox=log-and-abort): bitcoind will terminate if an unexpected (not allowlisted) syscall is invoked.
…
2021-06-09T12:34:56Z Syscall filter installed for thread "addcon"
2021-06-09T12:34:56Z Syscall filter installed for thread "dnsseed"
2021-06-09T12:34:56Z Syscall filter installed for thread "net"
2021-06-09T12:34:56Z Syscall filter installed for thread "msghand"
2021-06-09T12:34:56Z Syscall filter installed for thread "opencon"
2021-06-09T12:34:56Z Syscall filter installed for thread "init"
…
# A simulated execve call to show the sandbox in action:
2021-06-09T12:34:56Z ERROR: The syscall "execve" (syscall number 59) is not allowed by the syscall sandbox in thread "msghand". Please report.
…
Aborted (core dumped)
$
```
---
[About seccomp and seccomp-bpf](https://en.wikipedia.org/wiki/Seccomp):
> In computer security, seccomp (short for secure computing mode) is a facility in the Linux kernel. seccomp allows a process to make a one-way transition into a "secure" state where it cannot make any system calls except exit(), sigreturn(), and read() and write() to already-open file descriptors. Should it attempt any other system calls, the kernel will terminate the process with SIGKILL or SIGSYS. In this sense, it does not virtualize the system's resources but isolates the process from them entirely.
>
> […]
>
> seccomp-bpf is an extension to seccomp that allows filtering of system calls using a configurable policy implemented using Berkeley Packet Filter rules. It is used by OpenSSH and vsftpd as well as the Google Chrome/Chromium web browsers on Chrome OS and Linux. (In this regard seccomp-bpf achieves similar functionality, but with more flexibility and higher performance, to the older systrace—which seems to be no longer supported for Linux.)
ACKs for top commit:
laanwj:
Code review and lightly tested ACK 4747da3a5b
Tree-SHA512: e1c28e323eb4409a46157b7cc0fc29a057ba58d1ee2de268962e2ade28ebd4421b5c2536c64a3af6e9bd3f54016600fec88d016adb49864b63edea51ad838e17
a43b8e9555 build: set OSX_MIN_VERSION to 10.15 (fanquake)
Pull request description:
Taken out of #20744, as splitting up some of the build changes was mentioned [here](https://github.com/bitcoin/bitcoin/pull/22937#discussion_r707303172).
This is required to use `std::filesystem` on macOS, as support for it only landed in the libc++.dylib shipped with 10.15. So if we want to move to using `std::filesystem` for `23.0`, this bump is required.
See also: https://developer.apple.com/documentation/xcode-release-notes/xcode-11-release-notes
> Clang now supports the C++17 \<filesystem\> library for iOS 13, macOS 10.15, watchOS 6, and tvOS 13.
macOS 10.15 was released in October 2019. macOS OS's seem to have a life of about 3 years, so it's possible that 10.14 will become officially unsupported by the end of 2021 and prior to the release of 23.0.
Guix builds:
```bash
bash-5.1# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
abc8b749be65f1339dcdf44bd1ed6ade2533b8e3b5030ad1dde0ae0cede78136 guix-build-a43b8e955558/output/dist-archive/bitcoin-a43b8e955558.tar.gz
1edcc301eb4c02f3baa379beb8d4c78e661abc24a293813bc9d900cf7255b790 guix-build-a43b8e955558/output/x86_64-apple-darwin19/SHA256SUMS.part
e9dbb5594a664519da778dde9ed861c3f0f631525672e17a67eeda599f16ff44 guix-build-a43b8e955558/output/x86_64-apple-darwin19/bitcoin-a43b8e955558-osx-unsigned.dmg
11b23a17c630dddc7594c25625eea3de42db50f355733b9ce9ade2d8eba3a8f3 guix-build-a43b8e955558/output/x86_64-apple-darwin19/bitcoin-a43b8e955558-osx-unsigned.tar.gz
257ba64a327927f94d9aa0a68da3a2695cf880b3ed1a0113c5a966dcc426eb5e guix-build-a43b8e955558/output/x86_64-apple-darwin19/bitcoin-a43b8e955558-osx64.tar.gz
```
ACKs for top commit:
hebasto:
ACK a43b8e9555
jarolrod:
ACK a43b8e9
Tree-SHA512: 9ac77be7cb56c068578860a3b2b8b7487c9e18b71b14aedd77a9c663f5d4bb19756d551770c02ddd12f1797beea5757b261588e7b67fb53509bb998ee8022369
* Bump to debian:bookworm to avoid crash in the zmq functional test
(bitcoind: line 2: 33011 Illegal instruction (core dumped)
qemu-s390x)
* Remove RUN_UNIT_TESTS=true, because it is the default
* Add TEST_RUNNER_EXTRA --exclude to skip failing tests
fa0a5fa744 ci: Fuzz 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:
practicalswift:
cr ACK fa0a5fa744
Tree-SHA512: ed6be953cd99eadb1ba245ba30170747eff66be54d2773c8d26a3a6aee0fdcd6967c596f4f4ab1d238de6a6526623dac5211f0ba77f1986639395d7921bdc19f
ab9c34237a release: remove gitian (fanquake)
Pull request description:
Note that this doesn't yet touch any glibc back compat related code.
ACKs for top commit:
laanwj:
Code review ACK ab9c34237a
Tree-SHA512: 8e2fe3ec1097f54bb11ab9136b43818d90eab5dbb0a663ad6a552966ada4bdb49cc12ff4e66f0ec0ec5400bda5c81f3a3ce70a9ebb6fe1e0db612da9f00a51a7
fa001602cd ci: Re-enable verify-commits.py check (MarcoFalke)
fa880b10d6 ci: Unconditionally set the global git author name in cirrys.yml (MarcoFalke)
Pull request description:
Might be useful to detect bugs in the script itself or an accidentally missed signature.
ACKs for top commit:
josibake:
ACK fa001602cd
Zero-1729:
tACK fa001602cd
fanquake:
untested ACK fa001602cd
Tree-SHA512: 8a13a67d325f2477f4088d1034f0d5e4e04937a01ee3c738435fe66394c02b9f33225529952ad331b0ba19b63ca4b2f26911cb5d264890159840cf3e09085969
b367745cfe ci: Make Cirrus CI Windows build with --enable-werror (Hennadii Stepanov)
c713bb2b24 Fix Windows build with --enable-werror on Ubuntu Focal (Hennadii Stepanov)
Pull request description:
This PR makes possible to cross-compile Windows build with `--enable-werror --enable-suppress-external-warnings`.
Some problems are fixed, others are silenced.
Also `--enable-werror` is enabled for Cirrus CI Windows build (the last one on Cirrus CI without `--enable-werror`).
ACKs for top commit:
practicalswift:
cr ACK b367745cfe: patch looks correct
laanwj:
Code review ACK b367745cfe
vasild:
ACK b367745cfe
jarolrod:
ACK b367745cfe
Tree-SHA512: 64f5c99b7dad4c0efce80cd45d7074f275bd8411235dc9e0841287bdab64b812c6f8f9d632c35531d0b8210148531f53aaaac77be7699b29d2d6aaae304dbee0
fa80e10d94 test: Add feature_taproot.py --previous_release (MarcoFalke)
85ccffa266 test: move releases download incantation to README (Sjors Provoost)
29d6b1da2a test: previous releases: add v0.20.1 (Sjors Provoost)
Pull request description:
Disabling the new consensus code at runtime is fine, but potentially fragile and incomplete. Fix that by giving the option to run with a version that has been compiled without any taproot code.
ACKs for top commit:
Sjors:
tACK fa80e10
NelsonGaldeman:
tACK fa80e10d94
Tree-SHA512: 1a1feef823f08c05268759645a8974e1b2d39a024258f5e6acecbe25097aae3fa9302c27262978b40f1aa8e7b525b60c0047199010f2a5d6017dd6434b4066f0
During each CI run, for macos native environment, python packages lief
and zmq are rebuilt everytime which wastes a lot of resources and time.
The latest version of pip directly fetches pre-built binaries. Through
this commit pip version is upgraded in macos environment before
installation of these packages.
