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 since it’s modified in a few other PRs.

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.

Type	Reviewers
ACK	maflcko, stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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

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

DrahtBot added the label Needs rebase on Apr 19, 2025

l0rinc force-pushed on Apr 19, 2025

DrahtBot removed the label Needs rebase on Apr 19, 2025

in src/serialize.h:256 in 7b6ab350fa outdated

252@@ -253,14 +253,14 @@ template<class T>
253 concept CharNotInt8 = std::same_as<T, char> && !std::same_as<T, int8_t>;
254 
255 template <typename Stream, CharNotInt8 V> void Serialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t
256-template <typename Stream> void Serialize(Stream& s, std::byte a) { ser_writedata8(s, uint8_t(a)); }
257-template<typename Stream> inline void Serialize(Stream& s, int8_t a  ) { ser_writedata8(s, a); }
258+template <typename Stream> void Serialize(Stream& s, std::byte a) { ser_writedata8(s, static_cast<uint8_t>(a)); }

maflcko commented at 8:07 am on May 14, 2025:

style nit: Could adjust the indent manually, while touching this, so that the ser_writedata* are aligned? Also, personally I prefer to use uint8_t(a) over the verbose static cast for integral types. See https://github.com/bitcoin/bitcoin/blob/f9d8910539a2f7c4542457e08cd2bdd2dcb9cd08/doc/developer-notes.md#L111

But just style nits, up to you.

l0rinc commented at 12:25 pm on May 14, 2025:

Nice, I like that a lot more - addressed the reformatting and unification in separate commits, let me know if you think this is better. Pushed a rebase separately to simplify review from GitHub as well (I know you do range-diff), and to make sure the latest CI is run against it.

maflcko approved

maflcko commented at 8:07 am on May 14, 2025: member

nice, lgtm

l0rinc force-pushed on May 14, 2025

in src/serialize.h:257 in 427b82ec84 outdated

253@@ -254,28 +254,28 @@ concept CharNotInt8 = std::same_as<T, char> && !std::same_as<T, int8_t>;
254 
255 template <typename Stream, CharNotInt8 V> void Serialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t
256 template <typename Stream> void Serialize(Stream& s, std::byte a) { ser_writedata8(s, uint8_t(a)); }
257-template<typename Stream> inline void Serialize(Stream& s, int8_t a  ) { ser_writedata8(s, a); }
258+template<typename Stream> inline void Serialize(Stream& s, int8_t a  ) { ser_writedata8(s, uint8_t(a)); }

maflcko commented at 12:36 pm on May 14, 2025:

nit in the second commit: Would be good to just set this to the final version template<typename Stream> void Serialize(Stream& s, int8_t a ) { ser_writedata8(s, uint8_t(a)); } from the beginning. Otherwise there will be three style-fixup commits on top of each other, making review harder and the git log and git blame depth tedious to follow.

l0rinc commented at 1:06 pm on May 14, 2025:

we could, but I would be mixing changes in the same commit - the commit message wouldn’t represent the behavior. Or you mean I should just squash the last few changes as “(un)serialization consistency”?

maflcko commented at 1:12 pm on May 14, 2025:

You can just append a section to the commit message:

0Also, fixup whitespace while touching...

I am just mentioning it, because I often use git log -S 'string' and it is tedious to walk back the history if string is modified three times just to fixup some whitespace.

l0rinc commented at 1:44 pm on May 14, 2025:

Split into 2 commits now, minimizing the same line being changed multiple times. Also dropped the prevector related change which you’ve fixed properly in https://github.com/bitcoin/bitcoin/pull/32490

l0rinc force-pushed on May 14, 2025

refactor: use consistent size type for serialization template parameters 5827e93507

refactor: re-enable UBSan implicit-sign-change in serialize.h

Made every signed/unsigned conversion in the serialization helpers explicit so the UBSan `implicit-sign-change` check passes and the `serialize.h` suppression can be dropped.

For consistency, a few other simple changes were also applied to the serialization helpers:
* remove redundant `inline` on function templates;
* unify formatting to make the differences between similar methods obvious.

516f0689b5

l0rinc force-pushed on May 14, 2025

maflcko approved

maflcko commented at 2:34 pm on May 14, 2025: member

Makes sense to drop the file-wide (!) suppression in favour of just documenting the intentional casts explicitly in the code.

review ACK 516f0689b511c09153e4b6b4a58dfedd61c6cda7 🎈

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 516f0689b511c09153e4b6b4a58dfedd61c6cda7 🎈
3IkxXYBj5Lj+9ve3PmFYt0hLsok5w9mIRQN41g/T8vHnCXL2GR+xNqCSXAs/wpLXyc+Pp3qziT+sZZo4Hs7tmDg==

fanquake requested review from stickies-v on May 14, 2025

in src/serialize.h:257 in 516f0689b5

265-template<typename Stream> inline void Serialize(Stream& s, uint64_t a) { ser_writedata64(s, a); }
266-template <typename Stream, BasicByte B, int N> void Serialize(Stream& s, const B (&a)[N]) { s.write(MakeByteSpan(a)); }
267-template <typename Stream, BasicByte B, std::size_t N> void Serialize(Stream& s, const std::array<B, N>& a) { s.write(MakeByteSpan(a)); }
268-template <typename Stream, BasicByte B, std::size_t N> void Serialize(Stream& s, std::span<B, N> span) { s.write(std::as_bytes(span)); }
269-template <typename Stream, BasicByte B> void Serialize(Stream& s, std::span<B> span) { s.write(std::as_bytes(span)); }
270+template <typename Stream> void Serialize(Stream& s, int8_t a)    { ser_writedata8(s, uint8_t(a)); }

stickies-v commented at 2:54 pm on May 15, 2025:

Doesn’t look like a regression in this PR because the conversion was happening implicitly anyway, but, isn’t this a rather dangerous thing to do? How do we know a is never going to be negative?

maflcko commented at 3:06 pm on May 15, 2025:

Doesn’t look like a regression in this PR because the conversion was happening implicitly anyway, but, isn’t this a rather dangerous thing to do? How do we know a is never going to be negative?

It will be negative and is known to be, otherwise there would be no suppression needed in master right now.

Also, it is perfectly fine and intended to serialize any int8_t value (even a negative one) as uint8_t, because the two int types are the same width. The only requirement is that the unserialize does not forget to cast to the right type as well.

stickies-v commented at 4:18 pm on May 15, 2025:

Oh right, they have the same bit representation, so underflowing here doesn’t mean anything for serialization (and vice versa). Thanks - can be marked as resolved!

stickies-v commented at 2:55 pm on May 15, 2025: contributor

Concept ACK for reducing suppressions.

stickies-v approved

stickies-v commented at 5:12 pm on May 15, 2025: contributor

ACK 516f0689b511c09153e4b6b4a58dfedd61c6cda7, nice cleanup!

fanquake merged this on May 16, 2025

fanquake closed this on May 16, 2025

l0rinc deleted the branch on May 16, 2025

l0rinc commented at 3:11 pm on May 16, 2025: contributor

Thanks for the reviews and for splitting out prevector! A follow-up that reenables the coins cache sanitizers by fixing the used cache accounting is done in https://github.com/bitcoin/bitcoin/pull/32313

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