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.
6eb0909cb7d5883a258f76ad6cf2c989fc6f892f 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 6eb0909cb7d5883a258f76ad6cf2c989fc6f892f 🐈
Tree-SHA512: b4d64f5941df77d13981f75ec170cef6ffabe782797c982ede7f34134be01dc0026dd7c0bee614bc1d64715e90a933d2a8c95974d402e32eaba8e24cc928299e
fa4068b4e2192f168bb120624eca5735f0dadf6f 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 fa4068b4e2192f168bb120624eca5735f0dadf6f. 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
fa753abd7cffa05548ad5f21f2e8f9f6b06a7b04 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 fa753abd7cffa05548ad5f21f2e8f9f6b06a7b04
Tree-SHA512: 81e0edc936198a0baf0f5bfa8cfedc12db51759c7873bb0082dfc5f0040d7f275b35f639c6f5b86fa1ea03397b0d5e757c2ce1b6b16f1029880a39b9c3aaceda
fa60169811d6991a116bd37e1ff58049d2beee77 rpc: Move signmessage RPC util to new file (MacroFake)
fa9425177e6be2c53fb5c1333636fa4d678ab401 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
035fa1f07aacb7bce74c0884ae28c8cf00fe3b1b build: Remove LIBTOOL_APP_LDFLAGS for bitcoin-chainstate (Cory Fields)
3f0595095dd6d230dc661641227937e3ab4ca8d3 docs: Add libbitcoinkernel_la_SOURCES explanation (Carl Dong)
94ad45deb257a95b4e98aa85da0371fb072fcd4c ci: Build libbitcoinkernel (Carl Dong)
26b2e7ffb3471a4712e5b9e50e066e0e3218f0dd build: Extract the libbitcoinkernel library (Carl Dong)
1df44dd20ca9e6e55eb353824b27d11bd1878c59 b-cs: Define G_TRANSLATION_FUN in bitcoinkernel.cpp (Carl Dong)
83a0bb7cc9907dbe089409ed5a417277ed63ed95 build: Separate lib_LTLIBRARIES initialization (Carl Dong)
c1e16cb31f4d8edde8fea310011189b8b272cb07 build: Create .la library for bitcoincrypto (Carl Dong)
8bdfe057c796dde1cd2e5a37a73e87a879e9fe56 build: Create .la library for leveldb (Carl Dong)
05d1525b6d4412f68ff4c5460cd1daa6fb49969b build: Create .la library for crc32c (Carl Dong)
64caf944797bc35c3044fe5675389656f9511a41 build: Remove vestigial LIBLEVELDB_SSE42 (Carl Dong)
1392e8e2d8cfe4115f0a152aca16ffe3f0f4573a 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 035fa1f07aacb7bce74c0884ae28c8cf00fe3b1b.
Tree-SHA512: b755edc3471c7c1098847e9b16ab182a6abb7582563d9da516de376a770ac7543c6fdb24238ddd4d3d2d458f905a0c0614b8667aab182aa7e6b80c1cca7090bc
9b0a13a2891641a3d12e525cee8ddddb1aa1bc73 tidy: Add include-what-you-use (fanquake)
74cd038e300bfbe2473295fc3b0c3a4f3e853a07 refactor: fix includes in src/init (fanquake)
c79ad935f0412bac3e19a6b925efdb390eb00bd9 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 9b0a13a2891641a3d12e525cee8ddddb1aa1bc73
jonatack:
ACK 9b0a13a2891641a3d12e525cee8ddddb1aa1bc73 reviewed changes and run CI output in https://cirrus-ci.com/task/4750910332076032
Tree-SHA512: 00beab5a5f2a6fc179abf08321a15391ecccaa91ab56f3c50c511e7b29a0d7c95d8bb43eac2c31489711086f6f77319d43d803cf8ea458e7cd234a780d9ae69e
fa9112aac07dc371bfda437d40eb1b841f36f392 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 19a56d1519fb493c3e1bd5cad55360b6b80fa52b (released in version 22.0).
Any Bitcoin Core version with the new database format after commit 1088b02f0ccd7358d2b7076bb9e122d59d502d02 (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 fa9112aac07dc371bfda437d40eb1b841f36f392
laanwj:
Code review ACK fa9112aac07dc371bfda437d40eb1b841f36f392
Tree-SHA512: 4243bb35df9ac4892f9fad30fe486d338745952bcff4160bcb0937c772d57b13b800647da14695e21e3655e85ee0d95fa3dc7789ee309d59ad84f422297fecb8
fa73f8a4695d5bcbd054f4c68ee2a648c808afd2 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 fa73f8a4695d5bcbd054f4c68ee2a648c808afd2 - `--disable-hardening` matches what was just added to oss-fuzz.
Tree-SHA512: 2e533bb9273c97600176be2e41069a03f425aa586f9f32b8ed5f0c9844215a3a41e95a8edd58d044386e350807d6a1df09008a7da35428abd185a509ca71bd82
fa76b2fbe343af5679263ac9bb3f1bd72fd82b5e 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 fa76b2fbe343af5679263ac9bb3f1bd72fd82b5e - checked that tests are also still being run in other jobs.
Tree-SHA512: c00fe1242ee6b9ef92d511f1e86305c1731d8894bb89ec8fcdf30069959831483933a93a6bfd33d9f2d15706bdeedbba29e82c409eed266abfc57aabfdaf4841
fae20e6b50306f91c74037e915aa0ab75a0a6b3b Revert "Avoid the use of P0083R3 std::set::merge" (MarcoFalke)
fab53b5fd45cf55a1d4d313e46ffce7396c9590e 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 fae20e6b50306f91c74037e915aa0ab75a0a6b3b - 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 fae20e6b50306f91c74037e915aa0ab75a0a6b3b, 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.
