738a53720e [fuzz] Apply fuzz env (suppressions, etc.) when fetching harness list (dergoegge)
Pull request description:
The fuzz test runner does not add the UBSan suppressions when fetching the harness list. We can observe this in CI as lots of UBSan errors prior to the harnesses actually executing: https://api.cirrus-ci.com/v1/task/5678606140047360/logs/ci.log
```
+ test/fuzz/test_runner.py -j10 -l DEBUG /ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/ --empty_min_time=60
/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/string_view:578:38: runtime error: unsigned integer overflow: 12 - 23 cannot be represented in type 'size_type' (aka 'unsigned long')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/string_view:578:38 in
/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/string_view:578:33: runtime error: implicit conversion from type 'size_type' (aka 'unsigned long') of value 18446744073709551605 (64-bit, unsigned) to type 'const difference_type' (aka 'const long') changed the value to -11 (64-bit, signed)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/string_view:578:33 in
crypto/sha256.cpp:75:57: runtime error: left shift of 1359893119 by 26 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:75:57 in
crypto/sha256.cpp:75:79: runtime error: left shift of 1359893119 by 21 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:75:79 in
crypto/sha256.cpp:75:101: runtime error: left shift of 1359893119 by 7 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:75:101 in
crypto/sha256.cpp:82:47: runtime error: unsigned integer overflow: 2968370640 + 2483695512 cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:82:47 in
crypto/sha256.cpp:74:57: runtime error: left shift of 1779033703 by 30 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:74:57 in
crypto/sha256.cpp:74:79: runtime error: left shift of 1779033703 by 19 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:74:79 in
crypto/sha256.cpp:74:101: runtime error: left shift of 1779033703 by 10 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:74:101 in
crypto/sha256.cpp:83:29: runtime error: unsigned integer overflow: 3458249854 + 980412007 cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:83:29 in
crypto/sha256.cpp:82:21: runtime error: unsigned integer overflow: 528734635 + 4228187651 cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:82:21 in
crypto/sha256.cpp:84:7: runtime error: unsigned integer overflow: 1013904242 + 3720769133 cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:84:7 in
crypto/sha256.cpp:85:12: runtime error: unsigned integer overflow: 3720769133 + 2654153126 cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:85:12 in
crypto/sha256.cpp:82:33: runtime error: unsigned integer overflow: 4165002546 + 1259303586 cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:82:33 in
crypto/sha256.cpp:125:50: runtime error: unsigned integer overflow: 3835390401 + 1367343104 cannot be represented in type 'unsigned int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:125:50 in
crypto/sha256.cpp:77:58: runtime error: left shift of 1367343104 by 15 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
...
```
To fix this we simply apply the usual fuzz env variables (that apply the suppressions) when fetching the harness list as well.
ACKs for top commit:
ismaelsadeeq:
Tested ACK 738a53720e
fanquake:
ACK 738a53720e
Tree-SHA512: befebaeb4ee5f2eddca67fc6dc69e997c6a250ea54844e5e6e93d1f6a13be49364a3ace31eaa942b02dcf73612af29ec4ace86c9eb7567b92f6f5dc3ea14dc11
8b7630cb1f build: ignore deprecated-declaration warnings in objc++ macOS code (fanquake)
bd8f0354ba build: Add missed definition for `AM_OBJCXXFLAGS` (Hennadii Stepanov)
Pull request description:
These come from GUI code, and haven't/aren't being fixed, see discussion in https://github.com/bitcoin-core/gui/issues/112. For now, just ignore them entirely. Note that this only applies to ObjCXX code, so will not hide any relevant warnings coming from C or CXX code (and they would be unlikely in any case).
Alternative to https://github.com/bitcoin/bitcoin/pull/29362 (which disables all compiler warnings, for macOS builds in the CI). This PR includes one commit from that PR.
Relevant output:
```bash
qt/macnotificationhandler.mm:27:9: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
NSUserNotification* userNotification = [[NSUserNotification alloc] init];
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
^
qt/macnotificationhandler.mm:27:50: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
NSUserNotification* userNotification = [[NSUserNotification alloc] init];
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
^
qt/macnotificationhandler.mm:30:11: warning: 'NSUserNotificationCenter' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
[[NSUserNotificationCenter defaultUserNotificationCenter] deliverNotification: userNotification];
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:118:12: note: 'NSUserNotificationCenter' has been explicitly marked deprecated here
@interface NSUserNotificationCenter : NSObject {
^
3 warnings generated.
```
ACKs for top commit:
hebasto:
re-ACK 8b7630cb1f.
Tree-SHA512: 2f1fec97d9aa46aa23989d9fb283fc574dff9dc8f44847bb273b8fcf942f56f64c6d93dfcd7af8fbb52bf152e0fe76818118ce416d8cec5de852c32b6697a243
These come from GUI code, and haven't/aren't being fixed, see discussion
in https://github.com/bitcoin-core/gui/issues/112. For now, just ignore
them entirely. Note that this only applies to ObjCXX code, so will not
hide any relevant warnings coming from C or CXX code (and they would be
unlikely in any case).
Alternative to #29362, which disables all compiler warnings, for macOS
builds in the CI.
Relevant output:
```bash
qt/macnotificationhandler.mm:27:9: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
NSUserNotification* userNotification = [[NSUserNotification alloc] init];
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
^
qt/macnotificationhandler.mm:27:50: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
NSUserNotification* userNotification = [[NSUserNotification alloc] init];
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
^
qt/macnotificationhandler.mm:30:11: warning: 'NSUserNotificationCenter' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
[[NSUserNotificationCenter defaultUserNotificationCenter] deliverNotification: userNotification];
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:118:12: note: 'NSUserNotificationCenter' has been explicitly marked deprecated here
@interface NSUserNotificationCenter : NSObject {
^
3 warnings generated.
```
7ab54397f8 seeds: Update testnet seeds (Ava Chow)
34a233b6d8 seeds: Update mainnet seeds (Ava Chow)
9701bc435f makeseeds: Check i2p seeds too (Ava Chow)
a8ec9eede4 makeseeds: Update PATTERN_AGENT (Ava Chow)
Pull request description:
The ipv4 and ipv6 seeds are updated from sipa's crawler, as outlined in contrib/seeds/README.md. The onion and i2p seeds are pulled from my node's addrman using `getrawaddrman` and then a connection was made to each node to retrieve the current service flags, block height, and user agent string before filtering through makeseeds.py. The CJDNS nodes were not updated as my node is not connected to that network.
makeseeds.py is also updated for more recent user agent strings as well as being able to handle i2p addresses.
Also updated the testnet seeds.
ACKs for top commit:
fanquake:
ACK 7ab54397f8
Tree-SHA512: 5edba63d51116e5d9a8ae23561ba5a311f4df88c555c60b2d7a6066e63f8cdfd256be7dac9acea4b370879d0d3c3a4b55328c15de4284b5f0d86e6cac2e5ba9b
53ffd5a410 docs: Fix broken reference to CI setup in test/lint/README.md (naiyoma)
Pull request description:
The current [reference](https://github.com/bitcoin/bitcoin/blob/master/test/ci/lint/04_install.sh
) for CI setup in /test/lint#readme returns a 404.
ACKs for top commit:
fanquake:
ACK 53ffd5a410
Tree-SHA512: 813c19a145f09e7da11963598b70dc438acba784eb722e509d6af59dc3af8f5da97628c454bed2b03cc919689603e070796de2db8d784d9162ae34e7b85a77d9
e67ab174c9 test: fix flaky wallet_send functional test (Max Edwards)
3c49e69670 test: fix weight estimates in functional tests (Max Edwards)
Pull request description:
Fixes: https://github.com/bitcoin/bitcoin/issues/25164
The wallet_send functional test has been flaky due to a slightly overestimated weight calculation. This PR makes the weight calculation more accurate, although occasionally, due to how ECDSA signatures can be different lengths it might slightly over estimate. The assertion in the test can handle this slight variation and so should continue passing.
Update:
Because the signature can be shorter that is used in the weight estimation or the final transaction the estimate could be both slightly smaller or slightly larger.
ACKs for top commit:
achow101:
ACK e67ab174c9
S3RK:
Code review ACK e67ab174c9
Tree-SHA512: 3bf73b355309dce860fa1520afb8461e94268e4bcf0e92a8273c279b41b058c44472cf59daafa15a515529b50bd665b5d498bbe4d934f2315dbe810a05bc73f9
A test suite should not leave any artifacts except for those explicitly
expected.
This change is easy to review with `git diff --ignore-all-space`
command.
If `setOrganizationName()` and `setApplicationName()` has not been
previously called, the `QSettings` object will not be able to read or
write any settings.
6e5eda83ad doc: remove rel note fragments (fanquake)
Pull request description:
These have been added to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/27.0-Release-Notes-Draft, where they can be improved further.
ACKs for top commit:
stickies-v:
ACK 6e5eda83ad
Tree-SHA512: 66874fe9a64ac3a99a15a602ac68ae0a9e08f52a0fe732e48136b245c2127ed04e8217f86c44459696b03b01532a926ab8d41101c6e670902c1fc31e583b4dc9
6962c66b4a build, msvc: Do not compile redundant sources (Hennadii Stepanov)
Pull request description:
The `test\util\setup_common.cpp` and `wallet\test\util.cpp` sources are already compiled and included in the `libtest_util` library, which is linked to the `test_bitcoin-qt.exe` binary. This PR follows the same logic as `Makefile.qttest.include`.
Useful for comparing project files across the master branch and the developing [cmake-staging](https://github.com/hebasto/bitcoin/tree/cmake-staging) branch.
ACKs for top commit:
sipsorcery:
utACK 6962c66b4a.
Tree-SHA512: 5c40f52f3c7df3fff994fb136d4b2779ade3857fa14cf167d3f8600f28e821294e3779ebd4f4ab10ad57bdc8e952f99f6eb211e746a986ec24e26c7d1a74c04f
b7aa717cdd refactor: gui, simplify boost signals disconnection (furszy)
f3a612f901 gui: guard accessing a nullptr 'clientModel' (furszy)
Pull request description:
Fixing #800.
During shutdown, already queue events dispatched from the backend such
'numConnectionsChanged' and 0networkActiveChanged' could try to access
the clientModel object, which might not exist because we manually delete
it inside 'BitcoinApplication::requestShutdown()'.
This happen because boost does not clears the queued events when they arise
concurrently with the signal disconnection (see https://www.boost.org/doc/libs/1_55_0/doc/html/signals2/thread-safety.html).
From the docs:
1) "Note that since we unlock the connection's mutex before executing its associated slot, it is possible a slot will still be executing after it has been disconnected by a [connection::disconnect](https://www.boost.org/doc/libs/1_55_0/doc/html/boost/signals2/connection.html#idp89761576-bb)(), if the disconnect was called concurrently with signal invocation."
2) "The fact that concurrent signal invocations use the same combiner object means you need to insure any custom combiner you write is thread-safe"
So, we need to guard `clientModel` before accessing it at the handler side.
ACKs for top commit:
hebasto:
re-ACK b7aa717cdd
Tree-SHA512: f1a21d69248628f6a13556a9438c9e4ea9f0a3678aab09ddfe836e78e4eee405a6730d37d39f1445068ada3a110b655b619cf0e090fc2d0cdf99bed061364aeb
The `test\util\setup_common.cpp` and `wallet\test\util.cpp` sources are
already compiled and included in the `libtest_util` library, which is
linked to the `test_bitcoin-qt.exe` binary. This change follows the same
logic as `Makefile.qttest.include`.
86b7f28d6c serialization: use internal endian conversion functions (Cory Fields)
432b18ca8d serialization: detect byteswap builtins without autoconf tests (Cory Fields)
297367b3bb crypto: replace CountBits with std::bit_width (Cory Fields)
52f9bba889 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 86b7f28d6c📘
fanquake:
ACK 86b7f28d6c - 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
efb70cd645 doc: correct function name in AssumeUTXO design docs (jrakibi)
Pull request description:
Corrected the function name from `CompleteSnapshotValidation()` to `MaybeCompleteSnapshotValidation()` in the assumeutxo design documentation.
This change ensures that the documentation accurately reflects the actual function name used in the code
ACKs for top commit:
Empact:
ACK efb70cd645
Tree-SHA512: 68b9be3ba710d91a2a955189e227f86b46ccb6a2a13c345d46f276cec6ff12b77ebf9814c4bcb00db7c17e221510e4a2e71175c78a6faf0e0e3159c761bc9b94
Rather than asserting that the exact fees are the same, check the fee rate rounded to nearest interger. This will account for small differences in fees caused by variability in ECDSA signature lengths.
Updates the weight estimate to be more accurate by removing byte buffers and calculating the length of the count of scriptWitnesses rather than just using the count itself.
f8a06f7a02 doc: remove references to disable-asm option now that it's gone (Cory Fields)
376f0f6d07 build: remove confusing and inconsistent disable-asm option (Cory Fields)
Pull request description:
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.
Additionally, this is one of the last (THE last?) remaining uses of autoconf defines in our crypto code. As such it seems like low-hanging fruit.
ACKs for top commit:
fanquake:
ACK f8a06f7a02
Tree-SHA512: 4a99c2130225acbe9dc7399ed572a04ca155cbfa3eef8178a632ba533017d264691e6482cceb1d8f9c5d768619d99a2466dea4b82b27b18b872bceae91b92fbb
a8c3454ba1 test: speedup bip324_cipher.py unit test (Sebastian Falbesoner)
Pull request description:
Executing the unit tests for the bip324_cipher.py module currently takes quite long (>60 seconds on my older notebook). Most time here is spent in empty plaintext/ciphertext encryption/decryption loops in `test_fschacha20poly1305aead`:
9eeee7caa3/test/functional/test_framework/crypto/bip324_cipher.py (L193-L194)9eeee7caa3/test/functional/test_framework/crypto/bip324_cipher.py (L198-L199)
Their sole purpose is increasing the FSChaCha20Poly1305 packet counter in order to trigger rekeying, i.e. the actual encryption/decryption is not relevant, as the result is thrown away. This commit speeds up the tests by supporting to pass "None" as plaintext/ciphertext, indicating to the routines that no actual encryption/decryption should be done.
The approach here is a bit hacky, a cleaner alternative would probably be to introduce a special `seek`/`skip_packets` method and not touch the encrypt/decrypt routines, but that seemed overkill to me only for speeding up a unit test. Open for suggestions.
master branch:
```
$ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py
..
----------------------------------------------------------------------
Ran 2 tests in 64.658s
```
PR branch:
```
$ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py
..
----------------------------------------------------------------------
Ran 2 tests in 0.822s
```
ACKs for top commit:
delta1:
Concept ACK a8c3454
epiccurious:
Tested ACK a8c3454ba1.
achow101:
ACK a8c3454ba1
marcofleon:
ACK a8c3454ba1. The comments at the top of `bip324_cipher.py` specify that this should only be used for testing, so I think this optimization makes sense in that context.
cbergqvist:
ACK a8c3454!
stratospher:
ACK a8c3454. I think it's worth it because of the significant speedup in the unit test.
Tree-SHA512: 737dd805a850be6e035aa3c6d9e2c5b5b5e89ddc564f84a045c37e0238fef6419912de7c902139b64914abdd647c649fe02a694f1a5e1741d7d4459c041caccc
