fac395e5eb2cd3210ba6345f777a586a9bec84e3 ci: Bump ci/lint/Dockerfile (MarcoFalke)
fa6eb6516727a8675dc6e46634d8343e282528ab test: Use python3.8 pow() (MarcoFalke)
88881cf7ac029aea660c2413ca8e2a5136fcd41b Bump python minimum version to 3.8 (MarcoFalke)
Pull request description:
There is no pressing reason to drop support for 3.7, however there are several maintenance issues:
* There is no supported operating system that ships 3.7 by default. (debian:buster is EOL and unmaintained to the extent that it doesn't run in the CI environment. See https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1484988445)
* Compiling python 3.7 from source is also unsupported on at least macos, according to https://github.com/bitcoin/bitcoin/pull/24017#issuecomment-1107820790
* Recent versions of lief require 3.8, see https://github.com/bitcoin/bitcoin/pull/27507#issuecomment-1517561645
Fix all maintenance issues by bumping the minimum.
ACKs for top commit:
RandyMcMillan:
ACK fac395e
fjahr:
ACK fac395e5eb2cd3210ba6345f777a586a9bec84e3
fanquake:
ACK fac395e5eb2cd3210ba6345f777a586a9bec84e3
Tree-SHA512: c198decdbbe29d186d73ea3f6549d8a38479383495d14a965a2f9211ce39637b43f13a4c2a5d3bf56e2d468be4bbe49b4ee8e8e19ec69936ff43ddf2b714c712
be55f545d53d44fdcf2d8ae802e9eae551d120c6 move-only: Extract common/args and common/config.cpp from util/system (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is https://github.com/bitcoin/bitcoin/pull/27254.
The pull request contains an extraction of ArgsManager related functions from util/system into their own common/ file.
The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager. The ArgsManager belongs into the common library, since the kernel library should not depend on it. See [doc/design/libraries.md](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) for more information on this rationale.
ACKs for top commit:
MarcoFalke:
re-ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6 🚲
ryanofsky:
Code review ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6. Just small cleanups since the last review.
hebasto:
ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 90eb03334af0155b823030b4f2ecf286d35058d700ee2ddbbaa445be19e31eb0fe982656f35bd14ecee3ad2c3d0db3746855cb8f3777eff7253713e42873e111
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
fa4a46de0b3c1a5895e95dba7e95278932fbfc2c ci: Bump nowallet_libbitcoinkernel task to ubuntu:focal (MarcoFalke)
fabc7d90a90d46af181bc08def43d861062f6dfa ci: Use credits in more tasks (MarcoFalke)
facae3b149d7dfe84ef46c69c1d6fb586c08848d ci: Use Cirrus CI dockerfile env (MarcoFalke)
Pull request description:
Currently the CI env has many intermittent issues:
* The Ubuntu package servers are frequently down
* Occasionally other stuff is down, such as dnf, pip, or the android sdk
* Installing packages is slower than downloading them, at least on Cirrus, which has a fast download speed
Fix all issues by using the Cirrus CI dockerfile env.
ACKs for top commit:
josibake:
code review ACK fa4a46de0b
Tree-SHA512: fea5663f7b6dc1c4ea9f87188026ec542b9269bac8ee3398cd58d4df6c86a0af9d275f1876e03f92fb1f6166ec49b817d9e588e6fe1ed54b77592502c2eccd9d
This fixes some cases, i.e under --no-install-recommends, where
libclang-rt-dev wouldn't be installed, and configuring would then fail.
Followup to #27444.
This should avoid errors when running it twice. For example, network
errors on the second invocation of 'apt update'; or unguarded
modifications such as APPEND_APT_SOURCES_LIST, which will append the
same string repeatedly.
The base install may be run twice in Cirrus CI with dockerfiles, or
locally when running twice with DANGER_RUN_CI_ON_HOST specified.
ed4a8339b8fe796b4668e206d7fb9c2b120f8eb2 ci: fix git dubious permissions error (josibake)
Pull request description:
fixes https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1496449588
this appears to be caused by a more recent version of git being sensitive to mismatched permissions on directories. we didn't notice this before because we were using two separate user accounts to fix up dir permissions in the container , but the second account was removed in #27376
there might be a more elegant way to do this, but this does the trick and seems to be the way others are fixing this issue around the internets.
ACKs for top commit:
RandyMcMillan:
ACK ed4a833
hebasto:
re-ACK ed4a8339b8fe796b4668e206d7fb9c2b120f8eb2
Tree-SHA512: dad467deca101a24f3ed34b3e26a1db5099a5bd5c3e9c9a22771c59848f7d7e7843c7386348e6fdf86d5a556e4706e5e20005d7a6637193e1c8aef7a5ff7fb19
b5ef1419ece77db77cc196eb541f99a72360a607 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 b5ef1419ece77db77cc196eb541f99a72360a607. 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.
fa1b4e5c3294fc9aec033892a4a4d7b5cfc015aa Use steady clock in FlushStateToDisk (MarcoFalke)
1111e2f8b43cd9ed62dcf6b571a224b84fc421fd 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 fa1b4e5c3294fc9aec033892a4a4d7b5cfc015aa
willcl-ark:
ACK fa1b4e5c3
Tree-SHA512: cc625e796b186accd53222bd64eb57d0512bc7e588312d254349b542bbc5e5daac348ff2b3b3f7dc5ae0bbbae2ec11fdbf3022cf2164211633765a4b0108e83e
802cc1ef536e11944608fe9ab782d3e962037703 Deduplicate bitcoind and bitcoin-qt init code (Ryan Ofsky)
d172b5c6718b69200c8ad211fe709860081bd692 Add InitError(error, details) overload (Ryan Ofsky)
3db2874bd71d2391747b7385cabcbfef67218c4c Extend bilingual_str support for tinyformat (Ryan Ofsky)
c361df90b9fd34e50bbf1db43b866930e5498357 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 802cc1ef536e11944608fe9ab782d3e962037703
TheCharlatan:
ACK 802cc1ef536e11944608fe9ab782d3e962037703
achow101:
ACK 802cc1ef536e11944608fe9ab782d3e962037703
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
4275195606e6f42466d9a8ef766b3035833df4d5 De-duplicate add_coin methods to a test util helper (Jon Atack)
9d92c3d7f42c18939a9a6aa1ee185f1c958360a0 Create InsecureRandMoneyAmount() test util helper (Jon Atack)
81f5ade2a324167c03c5ce765a26bd42ed652723 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 4275195606e6f42466d9a8ef766b3035833df4d5
achow101:
ACK 4275195606e6f42466d9a8ef766b3035833df4d5
john-moffett:
ACK 4275195606e6f42466d9a8ef766b3035833df4d5
Tree-SHA512: 3ed974251149c7417f935ef2f8865aa0dcc33b281b47522b0f96f1979dff94bb8527957f098fe4d210f40d715c00f29512f2ffe189097102229023b7284a3a27
fa8e92c022057adcb8b98647bde626ed9c054df2 doc: Update ci docs (721217.xyz)
5fffff54e9fcf154c722dc421025a567fa0c5c97 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 fa8e92c022057adcb8b98647bde626ed9c054df2
Tree-SHA512: 8b0c9019452fbe507a272c1037c3dce3c178c21f85ab1096ed3372ad9d4b3c7aa27d89e5bf80c9a6260ea652e0268be0cbe61d6a4fcb3add569fa38076d32287
935acdcc79d1dc5ac04a83b92e5919ddbfa29329 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 935acdcc79d1dc5ac04a83b92e5919ddbfa29329
hebasto:
Approach ACK 935acdcc79d1dc5ac04a83b92e5919ddbfa29329.
aureleoules:
reACK 935acdcc79d1dc5ac04a83b92e5919ddbfa29329
john-moffett:
ACK 935acdcc79d1dc5ac04a83b92e5919ddbfa29329
stickies-v:
Approach ACK 935acdcc7
Tree-SHA512: 4f1ba54ff2198eea0e505d41e73d552c84c60f6878d5c85a94a8ab57f39afc94ef8d79258e7afd01fa84ec2a99f4404bb877eecd671f65e1ee9273f3129fc650
fa6986a66b451f532a1aa2bd72c956fcd8c0d042 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 fa6986a66b451f532a1aa2bd72c956fcd8c0d042, tested on Ubuntu 22.04 locally.
fanquake:
ACK fa6986a66b451f532a1aa2bd72c956fcd8c0d042 - did not test but example CI output looks ok.
stickies-v:
utACK fa6986a66b451f532a1aa2bd72c956fcd8c0d042
Tree-SHA512: 7cfd8584bf12e03c28af23f4712c6bcafd648d87ddb92788b9cd35455b2db49f4bd4aef8ad4711f75c7f11ad2bb2492c2eb6044007086c20e36016575c060603
fa486de212108b4609d7c247d2a578f0b4df9703 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 fa486de212108b4609d7c247d2a578f0b4df9703 ([`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.
6d58117a31a88eec3f0a103f9d1fc26cf0b48348 build: Build minisketch test in `make check`, not in `make` (Hennadii Stepanov)
Pull request description:
On master (d1e42659bbdd8da170542d8c638242cd94f71a7d):
```
$ ./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 6d58117a31a88eec3f0a103f9d1fc26cf0b48348
Tree-SHA512: 4957c8f88a01aca005813bf4c1c26f433756bf68ea0c958481c638ead229fa8e23ecae3a8ac31ea555876ba6f2cc10ecd91caf2e2f664de5cb529ec05fb38fa7
faba08b5b4dd5dedb457db18696de6e15839c696 refactor: Remove stray cs_main redundant declaration (MarcoFalke)
fa02591edff0255c5120b5acb2366542a61c251f 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 faba08b5b4dd5dedb457db18696de6e15839c696
Tree-SHA512: 0aa58e7693b6fcd504f9da7339f8baa463a6407f67b27f68002db705f4642321ac3765f16c3d906c925ee24085591b79160a62fa5f4aaf6f2e5dcc788411800d
a3a2bd9e8ad360a63cc8bdfc365d8bfd25ecc720 ci: Drop no longer needed package-specific flags (Hennadii Stepanov)
071eef1e974f128131afe6c6b5c68a430c64687a build: Propagate user-defined flags to host packages (Hennadii Stepanov)
Pull request description:
On master (4f8b1f8759301d2553183e14f72444a0f1d80725) `{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 a3a2bd9e8ad360a63cc8bdfc365d8bfd25ecc720
Tree-SHA512: 243d6b1b0e9c5de46debc36de62a77b6b4d6f638940fd530040c219956ec624e321b0c25290fed164e3a8c88befa7b97b20f765d7b9a428c269b3720f21da099
