56750c4f87d089c6a3f093eb2bf2edd07170d4a8 iwyu, clang-format: Sort includes (Hennadii Stepanov)
2c78814e0e182853ce44d9fd63d24ee6cab5223e ci: Add IWYU job (Hennadii Stepanov)
94e4f04d7cf4b0fef9a28d3771e73f1dc9fb0528 cmake: Fix target name (Hennadii Stepanov)
0f81e005197fa4201a38e635ddf8c5dcc12a3878 cmake: Make `codegen` target dependent on `generate_build_info` (Hennadii Stepanov)
73f7844cdb1e225099223a355d88da0522d7d69b iwyu: Add patch to prefer C++ headers over C counterparts (Hennadii Stepanov)
7a65437e23706e4820392dc456c3acccbf196dd6 iwyu: Add patch to prefer angled brackets over quotes for includes (Hennadii Stepanov)
Pull request description:
This PR separates the IWYU checks into its own CI job to provide faster feedback to developers. No other changes are made to the treatment of IWYU warnings. The existing “tidy” CI job will no longer run IWYU.
See also the discussion of https://github.com/bitcoin/bitcoin/pull/33779, specifically this [comment](https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491515263):
> Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.
Based on ideas from https://github.com/bitcoin/bitcoin/pull/32953.
ACKs for top commit:
maflcko:
review ACK 56750c4f87d089c6a3f093eb2bf2edd07170d4a8 🌄
sedited:
ACK 56750c4f87d089c6a3f093eb2bf2edd07170d4a8
Tree-SHA512: af15326b6d0c5d1e11346ac64939644936c65eb9466cd1a17ab5da347d39aef10f7ab33b39fbca31ad291b0b4b54639b147b24410f4f86197e4a776049882694
Split the repeated `SipHash` v[0..3] initialization into a small `SipHashState` helper that is used by both `CSipHasher` and `PresaltedSipHasher`.
Added explanatory comments to clarify behavior, documenting the equivalence of `PresaltedSipHasher` `operator()` overloads to `CSipHasher` usage.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Replaces standalone `SipHashUint256` with an `operator()` overload in `PresaltedSipHasher`.
Updates all hasher classes (`SaltedUint256Hasher`, `SaltedTxidHasher`, `SaltedWtxidHasher`) to use `PresaltedSipHasher` internally, enabling the same constant-state caching optimization while keeping behavior unchanged.
Benchmark was also adjusted to cache the salting part.
The `SHA256AutoDetect` return output is used, among other use cases, to
name benchmarks. Using a comma breaks the CSV output.
This change replaces the comma with a semicolon, which fixes the issue.
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
This can be reproduced according to the developer notes with something
like
( cd ./src/ && ../contrib/devtools/run-clang-tidy.py -p ../bld-cmake -fix -j $(nproc) )
Also, the header related changes were done manually.
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs.
They are also wrong, i.e DEFAULT_SCRIPTCHECK_THREADS is not in
validation.h.
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~1 )
-END VERIFY SCRIPT-
f5883286e32b625aab3dd80c74d6adb4f37f0a80 Add a fuzz test for Num3072 multiplication and inversion (Pieter Wuille)
a26ce628942243fc9848a63bfdfa5e61f5e936f3 Safegcd based modular inverse for Num3072 (Pieter Wuille)
91ce8cef2d8955d980ab7e89fbf74e8b29adf178 Add benchmark for MuHash finalization (Pieter Wuille)
Pull request description:
This implements a safegcd-based modular inverse for MuHash3072. It is a fairly straightforward translation of [the libsecp256k1 implementation](https://github.com/bitcoin-core/secp256k1/pull/831), with the following changes:
* Generic for 32-bit and 64-bit
* Specialized for the specific MuHash3072 modulus (2^3072 - 1103717).
* A bit more C++ish
* Far fewer sanity checks
A benchmark is also included for MuHash3072::Finalize. The new implementation is around 100x faster on x86_64 for me (from 5.8 ms to 57 μs); for 32-bit code the factor is likely even larger.
For more information:
* [Original paper](https://gcd.cr.yp.to/papers.html) by Daniel J. Bernstein and Bo-Yin Yang
* [Implementation](https://github.com/bitcoin-core/secp256k1/pull/767) for libsecp256k1 by Peter Dettman; and the [final](https://github.com/bitcoin-core/secp256k1/pull/831) version
* [Explanation](https://github.com/bitcoin-core/secp256k1/blob/master/doc/safegcd_implementation.md) of the algorithm using Python snippets
* [Analysis](https://github.com/sipa/safegcd-bounds) of the maximum number of iterations the algorithm needs
* [Formal proof in Coq](https://medium.com/blockstream/a-formal-proof-of-safegcd-bounds-695e1735a348) by Russell O'Connor (for the 256-bit version of the algorithm; here we use a 3072-bit one).
ACKs for top commit:
achow101:
ACK f5883286e32b625aab3dd80c74d6adb4f37f0a80
TheCharlatan:
Re-ACK f5883286e32b625aab3dd80c74d6adb4f37f0a80
dergoegge:
tACK f5883286e32b625aab3dd80c74d6adb4f37f0a80
Tree-SHA512: 275872c61d30817a82901dee93fc7153afca55c32b72a95b8768f3fd464da1b09b36f952f30e70225e766b580751cfb9b874b2feaeb73ffaa6943c8062aee19a
It is UB to apply a distance to a pointer or iterator further than the
end itself, even if the distance is (partially) revoked later on.
Fix the issue by advancing the data pointer at most to the end.
These cause compile failures with _LIBCPP_REMOVE_TRANSITIVE_INCLUDES.
i.e:
```bash
In file included from init.cpp:8:
./init.h:46:54: error: no template named 'atomic' in namespace 'std'
46 | bool AppInitBasicSetup(const ArgsManager& args, std::atomic<int>& exit_status);
| ~~~~~^
1 error generated.
```
See: https://libcxx.llvm.org/DesignDocs/HeaderRemovalPolicy.html.
d440f13db02c82c842000abe4fe4d0c721a4ad3b crypto: Guard code with `ENABLE_SSE41` macro (Hennadii Stepanov)
6ec1ca7c85a4009b77e149a798a331592b96ea42 build: Fix test for SSE4.1 intrinsics (Hennadii Stepanov)
Pull request description:
1. Fix the test for SSE4.1 intrinsics during build system configuration, which currently can be false positive, for example, when `CXXFLAGS="-mno-sse4.1"` provided.
This PR fixes the test by adding the `_mm_blend_epi16` SSE4.1 function used in our codebase.
2. Guard `sha_x86_shani.cpp` code with `ENABLE_SSE41` macro as it uses the `_mm_blend_epi16` function from
the SSE4.1 instruction set.
It is possible that SHA-NI is enabled even when SSE4.1 is disabled, which causes compile errors in the master branch.
Closes https://github.com/bitcoin/bitcoin/issues/28864.
ACKs for top commit:
sipa:
utACK d440f13db02c82c842000abe4fe4d0c721a4ad3b
willcl-ark:
tACK d440f13db02c82c842000abe4fe4d0c721a4ad3b
theuni:
utACK d440f13db02c82c842000abe4fe4d0c721a4ad3b
Tree-SHA512: a6e1e8c94e1b94874ff51846815ef445e6135cbdb01b08eb695b3548115f2340dd835ebe53673ae46a553fe6be4815e68d8642c34235dd7af5106c4b7c9ea6f3
Move HexStr and HexDigit functions from util to crypto. The crypto library does
not actually use these functions, but the consensus library does. The consensus
and util libraries not allowed to depend on each other, but are allowed to
depend on the cryto library, so the crypto library is a reasonable put these.
The consensus library uses HexStr and HexDigit in script.cpp, transaction.cpp,
and uint256.cpp.
The util library does not use HexStr but does use HexDigit in strencodings.cpp
to parse integers.
Looking at apple/freebsd/openbsd sources, their implementations match our naive
fallback. It's not worth the hassle of using a platform-specific function for
no gain.
The code in `sha_x86_shani.cpp` uses the `_mm_blend_epi16` function from
the SSE4.1 instruction set. However, it is possible that SHA-NI is
enabled even when SSE4.1 is disabled.
This changes avoid compilation errors in such a condition.
86b7f28d6c507155a9d3a15487ee883989b88943 serialization: use internal endian conversion functions (Cory Fields)
432b18ca8d0654318a8d882b28b20af2cb2d2e5d serialization: detect byteswap builtins without autoconf tests (Cory Fields)
297367b3bb062c57142747719ac9bf2e12717ce9 crypto: replace CountBits with std::bit_width (Cory Fields)
52f9bba889fd9b50a0543fd9fedc389592cdc7e5 crypto: replace non-standard CLZ builtins with c++20's bit_width (Cory Fields)
Pull request description:
This replaces #28674, #29036, and #29057. Now ready for testing and review.
Replaces platform-specific endian and byteswap functions. This is especially useful for kernel, as it means that our deep serialization code no longer requires bitcoin-config.h.
I apologize for the size of the last commit, but it's hard to avoid making those changes at once.
All platforms now use our internal functions rather than libc or platform-specific ones, with the exception of MSVC.
Sadly, benchmarking showed that not all compilers are capable of detecting and optimizing byteswap functions, so compiler builtins are instead used where possible. However, they're now detected via macros rather than autoconf checks.
This[ matches how libc++ implements std::byteswap for c++23](https://github.com/llvm/llvm-project/blob/main/libcxx/include/__bit/byteswap.h#L26).
I suggest we move/rename `compat/endian.h`, but I left that out of this PR to avoid bikeshedding.
#29057 pointed out some irregularities in benchmarks. After messing with various compilers and configs for a few weeks with these changes, I'm of the opinion that we can't win on every platform every time, so we should take the code that makes sense going forward. That said, if any real-world slowdowns are caused here, we should obviously investigate.
ACKs for top commit:
maflcko:
ACK 86b7f28d6c507155a9d3a15487ee883989b88943 📘
fanquake:
ACK 86b7f28d6c507155a9d3a15487ee883989b88943 - we can finish pruning out the __builtin_clz* checks/usage once the minisketch code has been updated. This is more good cleanup pre-CMake & for the kernal.
Tree-SHA512: 715a32ec190c70505ffbce70bfe81fc7b6aa33e376b60292e801f60cf17025aabfcab4e8c53ebb2e28ffc5cf4c20b74fe3dd8548371ad772085c13aec8b7970e
1. It didn't actually disable asm usage in our code. Regardless of the setting,
asm is used in random.cpp and support/cleanse.cpp.
2. The value wasn't forwarded to libsecp as a user might have reasonably
expected.
3. We now have the DISABLE_OPTIMIZED_SHA256 define which is what disable-asm
actually did in practice.
If there is any desire, we can hook DISABLE_OPTIMIZED_SHA256 up to a new
configure option that actually does what it says.
These replace our platform-specific mess in favor of c++20 endian detection
via std::endian and internal byteswap functions when necessary.
They no longer rely on autoconf detection.
-BEGIN VERIFY SCRIPT-
regex_string='^(?!//).*(AC_APPLE_UNIVERSAL_BUILD|BOOST_PROCESS_USE_STD_FS|CHAR_EQUALS_INT8|CLIENT_VERSION_BUILD|CLIENT_VERSION_IS_RELEASE|CLIENT_VERSION_MAJOR|CLIENT_VERSION_MINOR|COPYRIGHT_HOLDERS|COPYRIGHT_HOLDERS_FINAL|COPYRIGHT_HOLDERS_SUBSTITUTION|COPYRIGHT_YEAR|ENABLE_ARM_SHANI|ENABLE_AVX2|ENABLE_EXTERNAL_SIGNER|ENABLE_SSE41|ENABLE_TRACING|ENABLE_WALLET|ENABLE_X86_SHANI|ENABLE_ZMQ|HAVE_BOOST|HAVE_BUILTIN_CLZL|HAVE_BUILTIN_CLZLL|HAVE_BYTESWAP_H|HAVE_CLMUL|HAVE_CONSENSUS_LIB|HAVE_CXX20|HAVE_DECL_BE16TOH|HAVE_DECL_BE32TOH|HAVE_DECL_BE64TOH|HAVE_DECL_BSWAP_16|HAVE_DECL_BSWAP_32|HAVE_DECL_BSWAP_64|HAVE_DECL_FORK|HAVE_DECL_FREEIFADDRS|HAVE_DECL_GETIFADDRS|HAVE_DECL_HTOBE16|HAVE_DECL_HTOBE32|HAVE_DECL_HTOBE64|HAVE_DECL_HTOLE16|HAVE_DECL_HTOLE32|HAVE_DECL_HTOLE64|HAVE_DECL_LE16TOH|HAVE_DECL_LE32TOH|HAVE_DECL_LE64TOH|HAVE_DECL_PIPE2|HAVE_DECL_SETSID|HAVE_DECL_STRERROR_R|HAVE_DEFAULT_VISIBILITY_ATTRIBUTE|HAVE_DLFCN_H|HAVE_DLLEXPORT_ATTRIBUTE|HAVE_ENDIAN_H|HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR|HAVE_FDATASYNC|HAVE_GETENTROPY_RAND|HAVE_GETRANDOM|HAVE_GMTIME_R|HAVE_INTTYPES_H|HAVE_LIBADVAPI32|HAVE_LIBCOMCTL32|HAVE_LIBCOMDLG32|HAVE_LIBGDI32|HAVE_LIBIPHLPAPI|HAVE_LIBKERNEL32|HAVE_LIBOLE32|HAVE_LIBOLEAUT32|HAVE_LIBSHELL32|HAVE_LIBSHLWAPI|HAVE_LIBUSER32|HAVE_LIBUUID|HAVE_LIBWINMM|HAVE_LIBWS2_32|HAVE_MALLOC_INFO|HAVE_MALLOPT_ARENA_MAX|HAVE_MINIUPNPC_MINIUPNPC_H|HAVE_MINIUPNPC_UPNPCOMMANDS_H|HAVE_MINIUPNPC_UPNPERRORS_H|HAVE_NATPMP_H|HAVE_O_CLOEXEC|HAVE_POSIX_FALLOCATE|HAVE_PTHREAD|HAVE_PTHREAD_PRIO_INHERIT|HAVE_STDINT_H|HAVE_STDIO_H|HAVE_STDLIB_H|HAVE_STRERROR_R|HAVE_STRINGS_H|HAVE_STRING_H|HAVE_STRONG_GETAUXVAL|HAVE_SYSCTL|HAVE_SYSCTL_ARND|HAVE_SYSTEM|HAVE_SYS_ENDIAN_H|HAVE_SYS_PRCTL_H|HAVE_SYS_RESOURCES_H|HAVE_SYS_SELECT_H|HAVE_SYS_STAT_H|HAVE_SYS_SYSCTL_H|HAVE_SYS_TYPES_H|HAVE_SYS_VMMETER_H|HAVE_THREAD_LOCAL|HAVE_TIMINGSAFE_BCMP|HAVE_UNISTD_H|HAVE_VM_VM_PARAM_H|LT_OBJDIR|PACKAGE_BUGREPORT|PACKAGE_NAME|PACKAGE_STRING|PACKAGE_TARNAME|PACKAGE_URL|PACKAGE_VERSION|PTHREAD_CREATE_JOINABLE|QT_QPA_PLATFORM_ANDROID|QT_QPA_PLATFORM_COCOA|QT_QPA_PLATFORM_MINIMAL|QT_QPA_PLATFORM_WINDOWS|QT_QPA_PLATFORM_XCB|QT_STATICPLUGIN|STDC_HEADERS|STRERROR_R_CHAR_P|USE_ASM|USE_BDB|USE_DBUS|USE_NATPMP|USE_QRCODE|USE_SQLITE|USE_UPNP|_FILE_OFFSET_BITS|_LARGE_FILES)'
exclusion_files=":(exclude)src/minisketch :(exclude)src/crc32c :(exclude)src/secp256k1 :(exclude)src/crypto/sha256_arm_shani.cpp :(exclude)src/crypto/sha256_avx2.cpp :(exclude)src/crypto/sha256_sse41.cpp :(exclude)src/crypto/sha256_x86_shani.cpp"
git grep --perl-regexp --files-with-matches "$regex_string" -- '*.cpp' $exclusion_files | xargs git grep -L "bitcoin-config.h" | while read -r file; do line_number=$(awk -v my_file="$file" '/\/\/ file COPYING or https?:\/\/www.opensource.org\/licenses\/mit-license.php\./ {line = NR} /^\/\// && NR == line + 1 {while(getline && /^\/\//) line = NR} END {print line+1}' "$file"); sed -i "${line_number}i\\\\n\#if defined(HAVE_CONFIG_H)\\n#include <config/bitcoin-config.h>\\n\#endif" "$file"; done;
git grep --perl-regexp --files-with-matches "$regex_string" -- '*.h' $exclusion_files | xargs git grep -L "bitcoin-config.h" | while read -r file; do sed -i "/#define.*_H/a \\\\n\#if defined(HAVE_CONFIG_H)\\n#include <config/bitcoin-config.h>\\n\#endif" "$file"; done;
for file in $(git grep --files-with-matches 'bitcoin-config.h' -- '*.cpp' '*.h' $exclusion_files); do if ! grep -q --perl-regexp "$regex_string" $file; then sed -i '/HAVE_CONFIG_H/{N;N;N;d;}' $file; fi; done;
-END VERIFY SCRIPT-
The first command creates a regular expression for matching all bitcoin-config.h symbols in the following form: ^(?!//).*(AC_APPLE_UNIVERSAL_BUILD|BOOST_PROCESS_USE_STD_FS|...|_LARGE_FILES). It was generated with:
./autogen.sh && printf '^(?!//).*(%s)' $(awk '/^#undef/ {print $2}' src/config/bitcoin-config.h.in | paste -sd "|" -)
The second command holds a list of files and directories that should not be processed. These include subtree directories as well as some crypto files that already get their symbols through the makefile.
The third command checks for missing bitcoin-config headers in .cpp files and adds the header if it is missing.
The fourth command checks for missing bitcoin-config headers in .h files and adds the header if it is missing.
The fifth command checks for unneeded bitcoin-config headers in sources files and removes the header if it is unneeded.
bbf218d06164b7247f5e9df5ba143383022fbf74 crypto: remove sha256_sse4 from the base crypto helper lib (Cory Fields)
4dbd0475d8c16ed10c309bf6badc17f2d2eaaa69 crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 (Cory Fields)
Pull request description:
Replace it with a more explicit `DISABLE_OPTIMIZED_SHA256` and clean up some.
The macro was originally used by libbitcoinconsensus which opts out of optimized sha256 for the sake of simplicity.
Also remove the `BUILD_BITCOIN_INTERNAL` define from libbitcoinkernel for now as it does not export an api. When it does we can pick a less confusing define to control its exports.
Removing the define should have the effect of enabling sha256 optimizations for the kernel.
ACKs for top commit:
TheCharlatan:
Re-ACK bbf218d06164b7247f5e9df5ba143383022fbf74
hebasto:
re-ACK bbf218d06164b7247f5e9df5ba143383022fbf74
Tree-SHA512: 7c17592bb2d3e671779f96903cb36887c5785408213bffbda1ae37b66e6bcfaffaefd0c1bf2d1a407060cd377e3d4881cde3a73c429a1aacb677f370314a066a
Replace it with a more explicit DISABLE_OPTIMIZED_SHA256 and clean up some.
The macro was originally used by libbitcoinconsensus which opts out of
optimized sha256 for the sake of simplicity.
Also remove the BUILD_BITCOIN_INTERNAL define from libbitcoinkernel for now
as it does not export an api. When it does we can pick a less confusing define
to control its exports.
Removing the define should have the effect of enabling sha256 optimizations
for the kernel.
