Introduce `g_fuzzing` global for fuzzing checks

dergoegge commented at 11:32 am on October 15, 2024: member

This PR introduces a global g_fuzzing that indicates if we are fuzzing.

If g_fuzzing is true then:

Assume checks are enabled
Special fuzzing paths are taken (e.g. pow check is reduced to one bit)

Closes #30950 #31057

DrahtBot commented at 11:32 am on October 15, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	brunoerg, marcofleon, maflcko

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

dergoegge commented at 11:32 am on October 15, 2024: member

cc @maflcko @ryanofsky

DrahtBot added the label CI failed on Oct 15, 2024

DrahtBot commented at 12:19 pm on October 15, 2024: contributor

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

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.

dergoegge force-pushed on Oct 15, 2024

maflcko commented at 12:21 pm on October 15, 2024: member

Looks like this fails, because it can’t be used via Assume in a consteval context? However, I had difficulties anyway using Assert/Assume in those contexts, so I wonder if they shouldn’t just use assert directly?

in src/util/check.h:56 in 80ee9f092d outdated

52@@ -51,6 +53,7 @@ constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] con
53             assertion_fail(file, line, func, assertion);
54         }
55     }
56+    if (g_fuzzing && !val) abort();

maflcko commented at 12:24 pm on October 15, 2024:

Any reason to use a different branch here? Removing the constexpr above and adding ||g_fuzzing to the if should achieve the same? There shouldn’t be a downside, because the g_fuzzing will always be checked anyway?

dergoegge commented at 1:13 pm on October 15, 2024:

Will change on next push

maflcko commented at 3:27 pm on October 23, 2024:

Are you still working on this?

dergoegge commented at 10:00 am on October 24, 2024:

Done!

maflcko commented at 10:16 am on October 24, 2024:

I don’t see it. I still see else if (g_fuzzing && !val) abort();. Thus, I don’t see any ||g_fuzzing. Also, the std::abort wasn’t removed nor the “duplicate” val check.

maflcko commented at 10:17 am on October 24, 2024:

If you want to keep them for some reason, it would be good to explain, and then also add the missing includes.

dergoegge commented at 10:31 am on October 24, 2024:

I misunderstood what you meant, slow start today😴 should be fixed now!

dergoegge force-pushed on Oct 15, 2024

dergoegge commented at 10:04 am on October 16, 2024: member

The CI failure is a little puzzling:

 0[13:26:14.486] Run bitset with args ['/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_corpora/bitset')]INFO: Running with entropic power schedule (0xFF, 100).
 1[13:26:14.486] INFO: Seed: 1575764882
 2[13:26:14.486] INFO: Loaded 1 modules   (622699 inline 8-bit counters): 622699 [0x557b75e3b468, 0x557b75ed34d3), 
 3[13:26:14.486] INFO: Loaded 1 PC tables (622699 PCs): 622699 [0x557b75ed34d8,0x557b76853b88), 
 4[13:26:14.486] INFO:     1372 files found in /ci_container_base/ci/scratch/qa-assets/fuzz_corpora/bitset
 5[13:26:14.486] INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
 6[13:26:14.486] INFO: seed corpus: files: 1372 min: 1b max: 4194338b total: 10778447b rss: 110Mb
 7[13:26:14.486] /ci_container_base/src/util/bitset.h:369:34: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'value_type' (aka 'unsigned short') changed the value to 65535 (16-bit, unsigned)
 8[13:26:14.486]     [#0](/bitcoin-bitcoin/0/) 0x557b72af9a65 in bitset_detail::MultiIntBitSet<unsigned short, 2u>::Fill(unsigned int) ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/./src/util/bitset.h:369:34
 9[13:26:14.486]     [#1](/bitcoin-bitcoin/1/) 0x557b72a604a3 in void (anonymous namespace)::TestType<bitset_detail::MultiIntBitSet<unsigned short, 2u>>(std::span<unsigned char const, 18446744073709551615ul>) ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/./src/test/fuzz/bitset.cpp:182:30
10[13:26:14.486]     [#2](/bitcoin-bitcoin/2/) 0x557b72a4763c in bitset_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/./src/test/fuzz/bitset.cpp:291:9
11[13:26:14.486]     [#3](/bitcoin-bitcoin/3/) 0x557b731e929d in std::function<void (std::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::span<unsigned char const, 18446744073709551615ul>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
12[13:26:14.486]     [#4](/bitcoin-bitcoin/4/) 0x557b731e929d in LLVMFuzzerTestOneInput ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/util/./src/test/fuzz/fuzz.cpp:211:5
13[13:26:14.486]     [#5](/bitcoin-bitcoin/5/) 0x557b728a24f4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c494f4) (BuildId: 1cb0f5769fd52f5e14ffeabc7e79e3bb0eee7ba7)
14[13:26:14.486]     [#6](/bitcoin-bitcoin/6/) 0x557b728a1be9 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c48be9) (BuildId: 1cb0f5769fd52f5e14ffeabc7e79e3bb0eee7ba7)
15[13:26:14.486]     [#7](/bitcoin-bitcoin/7/) 0x557b728a3806 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c4a806) (BuildId: 1cb0f5769fd52f5e14ffeabc7e79e3bb0eee7ba7)
16[13:26:14.486]     [#8](/bitcoin-bitcoin/8/) 0x557b728a3d17 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c4ad17) (BuildId: 1cb0f5769fd52f5e14ffeabc7e79e3bb0eee7ba7)
17[13:26:14.486]     [#9](/bitcoin-bitcoin/9/) 0x557b7289120f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c3820f) (BuildId: 1cb0f5769fd52f5e14ffeabc7e79e3bb0eee7ba7)
18[13:26:14.486]     [#10](/bitcoin-bitcoin/10/) 0x557b728bb896 in main (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c62896) (BuildId: 1cb0f5769fd52f5e14ffeabc7e79e3bb0eee7ba7)
19[13:26:14.486]     [#11](/bitcoin-bitcoin/11/) 0x7ff169e111c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
20[13:26:14.486]     [#12](/bitcoin-bitcoin/12/) 0x7ff169e1128a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
21[13:26:14.486]     [#13](/bitcoin-bitcoin/13/) 0x557b728861f4 in _start (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c2d1f4) (BuildId: 1cb0f5769fd52f5e14ffeabc7e79e3bb0eee7ba7)
22[13:26:14.486] 
23[13:26:14.486] SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation /ci_container_base/src/util/bitset.h:369:34 
24[13:26:14.486] MS: 0 ; base unit: 0000000000000000000000000000000000000000
25[13:26:14.486] 0x9,0x25,0x3a,0x9b,0xff,
26[13:26:14.486] \011%:\233\377
27[13:26:14.486] artifact_prefix='./'; Test unit written to ./crash-29d7e95e22f18f0e6312373978433d7d97b91a95
28[13:26:14.486] Base64: CSU6m/8=

in src/pow.cpp:142 in e312d7596c outdated

138@@ -138,11 +139,8 @@ bool PermittedDifficultyTransition(const Consensus::Params& params, int64_t heig
139 // the most signficant bit of the last byte of the hash is set.
140 bool CheckProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params& params)
141 {
142-#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
143-    return (hash.data()[31] & 0x80) == 0;
144-#else
145+    if(g_fuzzing) return (hash.data()[31] & 0x80) == 0;

maflcko commented at 10:57 am on October 16, 2024:

clang-format for new code :sweat_smile:

dergoegge force-pushed on Oct 16, 2024

dergoegge force-pushed on Oct 17, 2024

DrahtBot removed the label CI failed on Oct 17, 2024

laanwj added the label Tests on Oct 20, 2024

brunoerg commented at 10:44 am on October 23, 2024: contributor

Concept ACK

dergoegge force-pushed on Oct 24, 2024

dergoegge commented at 1:46 pm on October 24, 2024: member

Looks like switching from Assume to assert in BitSet has a noticeable impact on the Linearize* bench tests: https://corecheck.dev/bitcoin/bitcoin/pulls/31093

maflcko commented at 2:33 pm on October 24, 2024: member

You can just turn the Assume to assert at compile-time, eagerly, if possible (decided by the compiler) at no downside, at the benefit that it will be evaluated and checked at compile-time:

  0diff --git a/src/util/bitset.h b/src/util/bitset.h
  1index 7db6f2424f..6f9e808c37 100644
  2--- a/src/util/bitset.h
  3+++ b/src/util/bitset.h
  4@@ -101,7 +101,7 @@ class IntBitSet
  5         /** Progress to the next 1 bit (only if != IteratorEnd). */
  6         constexpr Iterator& operator++() noexcept
  7         {
  8-            assert(m_val != 0);
  9+            Assume(m_val != 0);
 10             m_val &= m_val - I{1U};
 11             if (m_val != 0) m_pos = std::countr_zero(m_val);
 12             return *this;
 13@@ -109,7 +109,7 @@ class IntBitSet
 14         /** Get the current bit position (only if != IteratorEnd). */
 15         constexpr unsigned operator*() const noexcept
 16         {
 17-            assert(m_val != 0);
 18+            Assume(m_val != 0);
 19             return m_pos;
 20         }
 21     };
 22@@ -136,39 +136,39 @@ public:
 23     /** Construct a bitset with the singleton i. */
 24     static constexpr IntBitSet Singleton(unsigned i) noexcept
 25     {
 26-        assert(i < MAX_SIZE);
 27+        Assume(i < MAX_SIZE);
 28         return IntBitSet(I(1U) << i);
 29     }
 30     /** Construct a bitset with bits 0..count-1 (inclusive) set to 1. */
 31     static constexpr IntBitSet Fill(unsigned count) noexcept
 32     {
 33         IntBitSet ret;
 34-        assert(count <= MAX_SIZE);
 35+        Assume(count <= MAX_SIZE);
 36         if (count) ret.m_val = I(~I{0}) >> (MAX_SIZE - count);
 37         return ret;
 38     }
 39     /** Set a bit to 1. */
 40     constexpr void Set(unsigned pos) noexcept
 41     {
 42-        assert(pos < MAX_SIZE);
 43+        Assume(pos < MAX_SIZE);
 44         m_val |= I{1U} << pos;
 45     }
 46     /** Set a bit to the specified value. */
 47     constexpr void Set(unsigned pos, bool val) noexcept
 48     {
 49-        assert(pos < MAX_SIZE);
 50+        Assume(pos < MAX_SIZE);
 51         m_val = (m_val & ~I(I{1U} << pos)) | (I(val) << pos);
 52     }
 53     /** Set a bit to 0. */
 54     constexpr void Reset(unsigned pos) noexcept
 55     {
 56-        assert(pos < MAX_SIZE);
 57+        Assume(pos < MAX_SIZE);
 58         m_val &= ~I(I{1U} << pos);
 59     }
 60     /** Retrieve a bit at the given position. */
 61     constexpr bool operator[](unsigned pos) const noexcept
 62     {
 63-        assert(pos < MAX_SIZE);
 64+        Assume(pos < MAX_SIZE);
 65         return (m_val >> pos) & 1U;
 66     }
 67     /** Compute the number of 1 bits in the bitset. */
 68@@ -186,13 +186,13 @@ public:
 69     /** Find the first element (requires Any()). */
 70     constexpr unsigned First() const noexcept
 71     {
 72-        assert(m_val != 0);
 73+        Assume(m_val != 0);
 74         return std::countr_zero(m_val);
 75     }
 76     /** Find the last element (requires Any()). */
 77     constexpr unsigned Last() const noexcept
 78     {
 79-        assert(m_val != 0);
 80+        Assume(m_val != 0);
 81         return std::bit_width(m_val) - 1;
 82     }
 83     /** Set this object's bits to be the binary AND between respective bits from this and a. */
 84@@ -281,7 +281,7 @@ class MultiIntBitSet
 85         /** Progress to the next 1 bit (only if != IteratorEnd). */
 86         constexpr Iterator& operator++() noexcept
 87         {
 88-            assert(m_idx < N);
 89+            Assume(m_idx < N);
 90             m_val &= m_val - I{1U};
 91             if (m_val == 0) {
 92                 while (true) {
 93@@ -301,7 +301,7 @@ class MultiIntBitSet
 94         /** Get the current bit position (only if != IteratorEnd). */
 95         constexpr unsigned operator*() const noexcept
 96         {
 97-            assert(m_idx < N);
 98+            Assume(m_idx < N);
 99             return m_pos;
100         }
101     };
102@@ -316,13 +316,13 @@ public:
103     /** Set a bit to 1. */
104     void constexpr Set(unsigned pos) noexcept
105     {
106-        assert(pos < MAX_SIZE);
107+        Assume(pos < MAX_SIZE);
108         m_val[pos / LIMB_BITS] |= I{1U} << (pos % LIMB_BITS);
109     }
110     /** Set a bit to the specified value. */
111     void constexpr Set(unsigned pos, bool val) noexcept
112     {
113-        assert(pos < MAX_SIZE);
114+        Assume(pos < MAX_SIZE);
115         m_val[pos / LIMB_BITS] = (m_val[pos / LIMB_BITS] & ~I(I{1U} << (pos % LIMB_BITS))) |
116                                  (I{val} << (pos % LIMB_BITS));
117     }
118@@ -341,19 +341,19 @@ public:
119     /** Set a bit to 0. */
120     void constexpr Reset(unsigned pos) noexcept
121     {
122-        assert(pos < MAX_SIZE);
123+        Assume(pos < MAX_SIZE);
124         m_val[pos / LIMB_BITS] &= ~I(I{1U} << (pos % LIMB_BITS));
125     }
126     /** Retrieve a bit at the given position. */
127     bool constexpr operator[](unsigned pos) const noexcept
128     {
129-        assert(pos < MAX_SIZE);
130+        Assume(pos < MAX_SIZE);
131         return (m_val[pos / LIMB_BITS] >> (pos % LIMB_BITS)) & 1U;
132     }
133     /** Construct a bitset with the singleton pos. */
134     static constexpr MultiIntBitSet Singleton(unsigned pos) noexcept
135     {
136-        assert(pos < MAX_SIZE);
137+        Assume(pos < MAX_SIZE);
138         MultiIntBitSet ret;
139         ret.m_val[pos / LIMB_BITS] = I{1U} << (pos % LIMB_BITS);
140         return ret;
141@@ -361,12 +361,12 @@ public:
142     /** Construct a bitset with bits 0..count-1 (inclusive) set to 1. */
143     static constexpr MultiIntBitSet Fill(unsigned count) noexcept
144     {
145-        assert(count <= MAX_SIZE);
146+        Assume(count <= MAX_SIZE);
147         MultiIntBitSet ret;
148         if (count) {
149             unsigned i = 0;
150             while (count > LIMB_BITS) {
151-                ret.m_val[i++] = I(~I{0});
152+                ret.m_val[i++] = ~I{0};
153                 count -= LIMB_BITS;
154             }
155             ret.m_val[i] = I(~I{0}) >> (LIMB_BITS - count);
156@@ -402,7 +402,7 @@ public:
157         unsigned p = 0;
158         while (m_val[p] == 0) {
159             ++p;
160-            assert(p < N);
161+            Assume(p < N);
162         }
163         return std::countr_zero(m_val[p]) + p * LIMB_BITS;
164     }
165@@ -411,7 +411,7 @@ public:
166     {
167         unsigned p = N - 1;
168         while (m_val[p] == 0) {
169-            assert(p > 0);
170+            Assume(p > 0);
171             --p;
172         }
173         return std::bit_width(m_val[p]) - 1 + p * LIMB_BITS;
174diff --git a/src/util/check.h b/src/util/check.h
175index 8d3c6ca64d..121038df4b 100644
176--- a/src/util/check.h
177+++ b/src/util/check.h
178@@ -42,9 +42,9 @@ void assertion_fail(std::string_view file, int line, std::string_view func, std:
179 
180 /** Helper for Assert()/Assume() */
181 template <bool IS_ASSERT, typename T>
182-T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* file, [[maybe_unused]] int line, [[maybe_unused]] const char* func, [[maybe_unused]] const char* assertion)
183+constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* file, [[maybe_unused]] int line, [[maybe_unused]] const char* func, [[maybe_unused]] const char* assertion)
184 {
185-    if (IS_ASSERT || g_fuzzing
186+    if (IS_ASSERT || std::is_constant_evaluated()||g_fuzzing
187 #ifdef ABORT_ON_FAILED_ASSUME
188                   || true
189 #endif

dergoegge commented at 2:36 pm on October 24, 2024: member

I’ll try reverting the bitset changes, I remember that it failed to compile without but let’s see…

maflcko commented at 2:39 pm on October 24, 2024: member

You can push the diff as a dummy commit for now, if you want. I’ll create a separate pull for it, because the Assume change at compile time possibly makes sense on its own. Then you can rebase on that and squash everything down to one commit again.

dergoegge marked this as a draft on Oct 24, 2024

dergoegge force-pushed on Oct 24, 2024

DrahtBot added the label CI failed on Oct 24, 2024

DrahtBot commented at 3:29 pm on October 24, 2024: contributor

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

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.

DrahtBot removed the label CI failed on Oct 24, 2024

maflcko approved

maflcko commented at 6:19 pm on October 24, 2024: member

lgtm, after squash

dergoegge commented at 9:42 am on October 25, 2024: member

Waiting for #31150 to go in

fanquake referenced this in commit b95adf057a on Oct 25, 2024

Introduce `g_fuzzing` global for fuzzing checks 9f243cd7fa

dergoegge force-pushed on Oct 25, 2024

dergoegge marked this as ready for review on Oct 25, 2024

brunoerg approved

brunoerg commented at 1:14 pm on October 25, 2024: contributor

crACK 9f243cd7fa6654e3b71ba6bff82cceed547c5d53

dergoegge requested review from maflcko on Oct 25, 2024

marcofleon commented at 2:48 pm on October 25, 2024: contributor

Tested ACK 9f243cd7fa6654e3b71ba6bff82cceed547c5d53

Reviewed the code, which changes the fuzzing check from compile time to run time. This should work for our purposes and is a reasonable solution to #30950. It’s better than #31028, which I’ll close if this gets merged.

Built with BUILD_FUZZ_BINARY (without BUILD_FOR_FUZZING) and ran the p2p_headers_presync target on a corpus that I have. There was no timeout.

0FUZZ=p2p_headers_presync ./fuzzbuildgfuzz/src/test/fuzz/fuzz ../fuzzcorpus/p2p_headers_presync/
1p2p_headers_presync: succeeded against 781 files in 1s.

maflcko commented at 9:08 am on October 28, 2024: member

review ACK 9f243cd7fa6654e3b71ba6bff82cceed547c5d53 🗜

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 9f243cd7fa6654e3b71ba6bff82cceed547c5d53 🗜
308lGpbkgVhcFKipondTngBBc44P3JvdyCzjDwpY6jkQE9U7u7UDhHjPBiQ9v07Mi8l28j7TUMxPWsMJWT+ElCw==

fanquake merged this on Oct 28, 2024

fanquake closed this on Oct 28, 2024

sipa commented at 2:00 am on October 31, 2024: member

This means that no Assume can be compiled out in production builds anymore, as they all involve a g_fuzzing check?

See #31178.

Introduce g_fuzzing global for fuzzing checks #31093

Code Coverage

Reviews

Introduce `g_fuzzing` global for fuzzing checks #31093