refactor: reenable `implicit-integer-sign-change` check for `serialize.h`

l0rinc commented at 1:15 pm on April 17, 2025: contributor

Inspired by #32154, the main goal of this PR is to reenable sanitizer checks for serialize.h (last commit)

DrahtBot commented at 1:15 pm on April 17, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32296.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32306 (ci: Temporarily disable WalletMigration benchmark by hebasto)
#32043 ([IBD] Tracking PR for speeding up Initial Block Download by l0rinc)
#31868 ([IBD] specialize block serialization by l0rinc)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Refactoring on Apr 17, 2025

in test/sanitizer_suppressions/ubsan:18 in 29cc92a9ff outdated

19 implicit-integer-sign-change:*/new_allocator.h
20 implicit-integer-sign-change:*/qarraydata.h
21 implicit-integer-sign-change:crc32c/
22 implicit-integer-sign-change:minisketch/
23 implicit-integer-sign-change:secp256k1/
24+implicit-signed-integer-truncation,implicit-integer-sign-change:secp256k1_modinv64_posdivsteps_62_var

maflcko commented at 1:48 pm on April 17, 2025:

It seems a bit odd to first sort stuff, and then remove it again later on. Just seems like unnecessary churn to touch the same lines several times. Also, sorting with lower case and upper case is “controversial”, so my recommendation would be to just drop this commit and focus on the more meaningful changes.

l0rinc commented at 2:04 pm on April 17, 2025:

Done, thanks!

l0rinc force-pushed on Apr 17, 2025

maflcko commented at 3:31 pm on April 17, 2025: member

CI fails. Did this pass locally?

fanquake commented at 3:33 pm on April 17, 2025: member

For reference (https://github.com/bitcoin/bitcoin/actions/runs/14517522442/job/40730094523?pr=32296#step:6:4644):

 0/home/runner/work/_temp/src/secp256k1/src/modinv64_impl.h:360:17: runtime error: implicit conversion from type 'uint64_t' (aka 'unsigned long') of value 7287561039273463316 (64-bit, unsigned) to type 'int' changed the value to 1893666324 (32-bit, signed)
 1    [#0](/bitcoin-bitcoin/0/) 0x555bd4509ce3 in secp256k1_modinv64_posdivsteps_62_var /home/runner/work/_temp/src/secp256k1/src/modinv64_impl.h:360:17
 2    [#1](/bitcoin-bitcoin/1/) 0x555bd4509ce3 in secp256k1_jacobi64_maybe_var /home/runner/work/_temp/src/secp256k1/src/modinv64_impl.h:742:15
 3    [#2](/bitcoin-bitcoin/2/) 0x555bd4509ce3 in secp256k1_fe_impl_is_square_var /home/runner/work/_temp/src/secp256k1/src/field_5x52_impl.h:509:11
 4    [#3](/bitcoin-bitcoin/3/) 0x555bd44d014d in secp256k1_ge_x_frac_on_curve_var /home/runner/work/_temp/src/secp256k1/src/group_impl.h:934:13
 5    [#4](/bitcoin-bitcoin/4/) 0x555bd44d014d in secp256k1_ellswift_xswiftec_frac_var /home/runner/work/_temp/src/secp256k1/src/modules/ellswift/main_impl.h:108:9
 6    [#5](/bitcoin-bitcoin/5/) 0x555bd44cf063 in secp256k1_ellswift_xdh /home/runner/work/_temp/src/secp256k1/src/modules/ellswift/main_impl.h:570:5
 7    [#6](/bitcoin-bitcoin/6/) 0x555bd40c4246 in CKey::ComputeBIP324ECDHSecret(EllSwiftPubKey const&, EllSwiftPubKey const&, bool) const /home/runner/work/_temp/build-asan/src/./key.cpp:334:20
 8    [#7](/bitcoin-bitcoin/7/) 0x555bd2dcc08a in BIP324_ECDH(ankerl::nanobench::Bench&)::$_0::operator()() const /home/runner/work/_temp/build-asan/src/bench/./bench/bip324_ecdh.cpp:34:24
 9    [#8](/bitcoin-bitcoin/8/) 0x555bd2dcc08a in ankerl::nanobench::Bench& ankerl::nanobench::Bench::run<BIP324_ECDH(ankerl::nanobench::Bench&)::$_0>(BIP324_ECDH(ankerl::nanobench::Bench&)::$_0&&) /home/runner/work/_temp/build-asan/src/bench/./bench/nanobench.h:1221:13
10    [#9](/bitcoin-bitcoin/9/) 0x555bd2dcbbb4 in BIP324_ECDH(ankerl::nanobench::Bench&) /home/runner/work/_temp/build-asan/src/bench/./bench/bip324_ecdh.cpp:28:33
11    [#10](/bitcoin-bitcoin/10/) 0x555bd2d24806 in std::function<void (ankerl::nanobench::Bench&)>::operator()(ankerl::nanobench::Bench&) const /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
12    [#11](/bitcoin-bitcoin/11/) 0x555bd2d24806 in benchmark::BenchRunner::RunAll(benchmark::Args const&) /home/runner/work/_temp/build-asan/src/bench/./bench/bench.cpp:153:13
13    [#12](/bitcoin-bitcoin/12/) 0x555bd2d10c95 in main /home/runner/work/_temp/build-asan/src/bench/./bench/bench_bitcoin.cpp:150:9
14    [#13](/bitcoin-bitcoin/13/) 0x7f6dbdcf41c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 42c84c92e6f98126b3e2230ebfdead22c235b667)
15    [#14](/bitcoin-bitcoin/14/) 0x7f6dbdcf428a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 42c84c92e6f98126b3e2230ebfdead22c235b667)
16    [#15](/bitcoin-bitcoin/15/) 0x555bd2c237c4 in _start (/home/runner/work/_temp/build-asan/bin/bench_bitcoin+0xbdc7c4) (BuildId: 9803a2db4f1b48ae5c51c4c10d78e993d6226047)
17
18SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation-or-sign-change /home/runner/work/_temp/src/secp256k1/src/modinv64_impl.h:360:17

l0rinc force-pushed on Apr 17, 2025

l0rinc commented at 5:14 pm on April 17, 2025: contributor

Since I can’t often reproduce the exact CI behavior locally, I have verified it by pushing to my own remote instead - which failed as expected when I have removed something that still needed the suppression: https://github.com/l0rinc/bitcoin/pull/8

I’ve simplified the commit structure, should be fine now - except for the benchmark failures on master which will likely fail.

l0rinc marked this as ready for review on Apr 17, 2025

DrahtBot added the label CI failed on Apr 17, 2025

DrahtBot commented at 9:34 pm on April 17, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40741925670

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

l0rinc commented at 10:01 pm on April 17, 2025: contributor

The fuzzer still has some problems with CCoinsViewCache:

runtime error: unsigned integer overflow: 0 - 48 cannot be represented in type ‘size_t’ (aka ‘unsigned long’) [15:43:31.228] #0 0x558e624b3787 in CCoinsViewCache::SpendCoin(COutPoint const&, Coin*) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./coins.cpp:133:22 [15:43:31.228]

I’m not yet sure how that’s possible, FetchCoin should populate the cachedCoinsUsage field if it’s missing - will try to reproduce it locally, drafting until then.

l0rinc marked this as a draft on Apr 17, 2025

ci: Temporarily revert "Drop bench -priority-level in win cross CI"

This reverts commit 27f11217ca63e0f8f78f14db139150052dcd9962.

e525cf3aa1

refactor: reenable `prevector` sanitizer checks

Original suppression was added in 2018

0f320dc43a

refactor: reenable implicit-integer-sign-change check for serialize.h

Serialization casts back and forth between signed and unsigned - let's make it explicit and reenable the sanitizers

b885f0d584

l0rinc force-pushed on Apr 18, 2025

l0rinc commented at 9:20 am on April 18, 2025: contributor

Rebased, cherry picked #32302 to fix unrelated CI failure and added back the unsigned-integer-overflow:CCoinsViewCache suppressions since it seems like a genuine error that I will have to tackle in a separate PR instead.

DrahtBot removed the label CI failed on Apr 18, 2025

l0rinc marked this as ready for review on Apr 18, 2025

refactor: reenable implicit-integer-sign-change check for serialize.h #32296

Code Coverage & Benchmarks

Reviews

Conflicts

refactor: reenable `implicit-integer-sign-change` check for `serialize.h` #32296