Skip to content

Conversation

@marcofleon
Copy link
Contributor

This PR expands the CheckGlobals utility that was introduced in #31486 and should help with fuzz stability (#29018).

System time shouldn't be used when running a fuzz test, as it is likely to introduce instability (non-determinism). This PR identifies and fixes the targets that were calling system time without setting mock time at the start of an iteration.

RemovingSetMockTime() from any one of these targets should result in a crash and a message describing the issue.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 20, 2024

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/31549.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK dergoegge, brunoerg, achow101
Concept ACK Prabhat1308
Stale ACK i-am-yuvi, maflcko

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:

  • #31022 (test: Add mockable steady clock, tests for PCP and NATPMP implementations by laanwj)

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 DrahtBot added the Tests label Dec 20, 2024
@marcofleon marcofleon force-pushed the 2024/12/fuzz-set-mocktime branch from f0fc536 to d482964 Compare December 20, 2024 18:35
@DrahtBot
Copy link
Contributor

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

Hints

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.

@marcofleon marcofleon force-pushed the 2024/12/fuzz-set-mocktime branch from d482964 to 8cf5c4e Compare December 20, 2024 18:49
@brunoerg
Copy link
Contributor

Concept ACK

Copy link
Contributor

@Prabhat1308 Prabhat1308 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK .

@yuvicc
Copy link
Contributor

yuvicc commented Jan 2, 2025

I am unable to crash the fuzz test when setmocktime() is removed from one of the targets.

clang version:

Homebrew clang version 18.1.7
Target: arm64-apple-darwin24.2.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin

clang++ version:

Homebrew clang version 18.1.7
Target: arm64-apple-darwin24.2.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin

and I've used this command mentioned in docs for macos:

$ cmake --preset=libfuzzer \
   -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
   -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
   -DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries

Is there anything I am doing wrong??

@marcofleon
Copy link
Contributor Author

marcofleon commented Jan 2, 2025

I am unable to crash the fuzz test when setmocktime() is removed from one of the targets.

@i-am-yuvi which target was it?

It could be that running the test from scratch doesn't quickly reach code where now() is called. Try running the target on a corpus of inputs:

FUZZ=load_external_block_file ./mocktimefuzzbuild/src/test/fuzz/fuzz ../qa-assets/fuzz_corpora/load_external_block_file -runs=1

You can find the corpora here: https://github.com/bitcoin-core/qa-assets

I think it's also possible that it's machine specifc. The command above for load_external_block_file crashes on my Linux machine but not on my mac, for example.

@marcofleon marcofleon force-pushed the 2024/12/fuzz-set-mocktime branch from 8cf5c4e to 28e374f Compare January 2, 2025 20:04
@yuvicc
Copy link
Contributor

yuvicc commented Jan 4, 2025

I am unable to crash the fuzz test when setmocktime() is removed from one of the targets.

@i-am-yuvi which target was it?

It could be that running the test from scratch doesn't quickly reach code where now() is called. Try running the target on a corpus of inputs:

FUZZ=load_external_block_file ./mocktimefuzzbuild/src/test/fuzz/fuzz ../qa-assets/fuzz_corpora/load_external_block_file -runs=1

You can find the corpora here: https://github.com/bitcoin-core/qa-assets

I think it's also possible that it's machine specifc. The command above for load_external_block_file crashes on my Linux machine but not on my mac, for example.

Thanks, worked(crashed) on Linux with Corpus. I haven't tested on Mac due to storage issue, it would be interesting to see if that would crash on macos!!

Copy link
Contributor

@yuvicc yuvicc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK fdc5224d205c69ebcb2d2b7bcd95386ef89181ca on x86_64

  • removing setmocktime () from any one of the targets would cause a crash.
  • Not tested on macos

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to set it in the init function? Otherwise, the global state may be non-deterministic:

index e4e4723c74..3f3bfbeeaa 100644
--- a/src/test/fuzz/fuzz.cpp
+++ b/src/test/fuzz/fuzz.cpp
@@ -115,6 +115,7 @@ void initialize()
     // - GetStrongRandBytes(), which is used for the creation of private key material.
     // - Creating a BasicTestingSetup or derived class will switch to a random seed.
     SeedRandomStateForTest(SeedRand::ZEROS);
+    //here
 
     // Terminate immediately if a fuzzing harness ever tries to create a socket.
     // Individual tests can override this by pointing CreateSock to a mocked alternative.

@maflcko
Copy link
Member

maflcko commented Jan 6, 2025

Tested that no superfluous ones were added via:

diff
diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp
index e4e4723c74..eda5ff4cc6 100644
--- a/src/test/fuzz/fuzz.cpp
+++ b/src/test/fuzz/fuzz.cpp
@@ -78,11 +78,13 @@ void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target,
 
 static std::string_view g_fuzz_target;
 static const TypeTestOneInput* g_test_one_input{nullptr};
+bool g_ever_used_g_mocktime{false};
 
 inline void test_one_input(FuzzBufferType buffer)
 {
     CheckGlobals check{};
     (*Assert(g_test_one_input))(buffer);
+    g_ever_used_g_mocktime|= g_used_mocktime;
 }
 
 const std::function<std::string()> G_TEST_GET_FULL_NAME{[]{
@@ -274,6 +276,9 @@ int main(int argc, char** argv)
     }
     const auto end_time{Now<SteadySeconds>()};
     std::cout << g_fuzz_target << ": succeeded against " << tested << " files in " << count_seconds(end_time - start_time) << "s." << std::endl;
+    if (tested && !g_ever_used_g_mocktime && g_set_mocktime) {
+        Assert(false); // remove unused SetMockTime()?
+    }
 #endif
     return 0;
 }
diff --git a/src/test/fuzz/util/check_globals.cpp b/src/test/fuzz/util/check_globals.cpp
index f91a965afc..76d62fa3ca 100644
--- a/src/test/fuzz/util/check_globals.cpp
+++ b/src/test/fuzz/util/check_globals.cpp
@@ -17,6 +17,8 @@ struct CheckGlobalsImpl {
     {
         g_used_g_prng = false;
         g_seeded_g_prng_zero = false;
+        g_set_mocktime= false;
+        g_used_mocktime=false;
         g_used_system_time = false;
         SetMockTime(0s);
     }
diff --git a/src/test/fuzz/util/check_globals.h b/src/test/fuzz/util/check_globals.h
index 12d39c2daf..798a9ba506 100644
--- a/src/test/fuzz/util/check_globals.h
+++ b/src/test/fuzz/util/check_globals.h
@@ -10,6 +10,8 @@
 #include <optional>
 #include <string>
 
+extern std::atomic<bool> g_set_mocktime;
+extern std::atomic<bool> g_used_mocktime;
 extern std::atomic<bool> g_used_system_time;
 
 struct CheckGlobalsImpl;
diff --git a/src/util/time.cpp b/src/util/time.cpp
index 00f0f47392..f8f1eb5504 100644
--- a/src/util/time.cpp
+++ b/src/util/time.cpp
@@ -20,6 +20,8 @@
 void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); }
 
 static std::atomic<std::chrono::seconds> g_mock_time{}; //!< For testing
+std::atomic<bool> g_set_mocktime{false};
+std::atomic<bool> g_used_mocktime{false};
 std::atomic<bool> g_used_system_time{false};
 
 NodeClock::time_point NodeClock::now() noexcept
@@ -27,6 +29,8 @@ NodeClock::time_point NodeClock::now() noexcept
     const auto mocktime{g_mock_time.load(std::memory_order_relaxed)};
     if (!mocktime.count()) {
         g_used_system_time = true;
+    }else{
+    g_used_mocktime=true;
     }
     const auto ret{
         mocktime.count() ?
@@ -40,6 +44,7 @@ void SetMockTime(int64_t nMockTimeIn) { SetMockTime(std::chrono::seconds{nMockTi
 void SetMockTime(std::chrono::seconds mock_time_in)
 {
     Assert(mock_time_in >= 0s);
+    g_set_mocktime=true;
     g_mock_time.store(mock_time_in, std::memory_order_relaxed);
 }
 

@maflcko
Copy link
Member

maflcko commented Jan 6, 2025

ACK 28e374f0a7dc1bda1c71e14eb896b2d9d2f23b48 🍭

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 28e374f0a7dc1bda1c71e14eb896b2d9d2f23b48 🍭
k8AjcAoUwz8rU6OdmDfzs1fRRDn4rJcQL6cG4GfGNM5b8gdMKm2r8/okjdf72tEmzwVbTt5qEHM12yxpjaAPCw==

@marcofleon marcofleon force-pushed the 2024/12/fuzz-set-mocktime branch from 28e374f to a96b84c Compare January 6, 2025 15:44
@marcofleon
Copy link
Contributor Author

Thanks for the review @i-am-yuvi!

I haven't tested on Mac due to storage issue, it would be interesting to see if that would crash on macos!!

So I learned that it's likely this

#if defined _GNU_SOURCE && (defined(__linux__) || defined(__FreeBSD__))
const cookie_io_functions_t io_hooks = {
FuzzedFileProvider::read,
FuzzedFileProvider::write,
FuzzedFileProvider::seek,
FuzzedFileProvider::close,
};
return fopencookie(this, mode.c_str(), io_hooks);
#else
(void)mode;
return nullptr;
#endif

that is preventing the load_external_block_file target from crashing on a mac.

@marcofleon
Copy link
Contributor Author

I think you forgot to set it in the init function? Otherwise, the global state may be non-deterministic

Good catch, thank you. Added in ff21870

Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK a96b84c

@DrahtBot DrahtBot requested a review from maflcko January 7, 2025 10:47
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK a96b84c

@achow101
Copy link
Member

ACK a96b84c

@achow101 achow101 merged commit 37af8bf into bitcoin:master Jan 10, 2025
18 checks passed
@maflcko
Copy link
Member

maflcko commented Jan 10, 2025

post-merge reACK a96b84c (only change is adding missing mocktime in init)

sedited added a commit to sedited/rust-bitcoinkernel that referenced this pull request Jan 17, 2025