Add cmpctblock inputs#276
Conversation
|
ACK 8dfbac8 waiting for CI |
|
This is because of Not really sure what to do here because I don't think I can modify the order of the locks here (and wouldn't if I could just for fuzz code). |
|
Hmm, For this fuzz test: Can it be fixed in a hacky way by turning class ImmediateBackgroundTaskRunner : public TaskRunnerInterface
{
public:
void insert(std::function<void()> func) override { std::thread(std::move(func)).join(); }
void flush() override {}
size_t size() override { return 0; }
}; |
I am not familiar with the kernel code, it looks like the The suggested patch does fix the issue, it should fix any current false positive deadlocks and also can't have false negative deadlocks. If we previously locked A->B->C and now split off B->C into a separate thread, we have a subset of the original lock orders. However double locking could technically go undetected since some callbacks are launched in the same thread ( |
One thing to consider is how much overhead it has to launch a std::thread for each function. If it is too much, one can consider adding a "pool" with one thread ( The submit funciton will then probably be: void insert(std::function<void()> func) override { Assert(g_pool.Submit(std::move(func)))->wait(); } |
Am currently benching this, probably should have started with profiling first though to see if it's indeed a bottleneck. The diff is a bit confusing since diffdiff --git a/src/test/fuzz/cmpctblock.cpp b/src/test/fuzz/cmpctblock.cpp
index 3e4268cb65..2ca333c15b 100644
--- a/src/test/fuzz/cmpctblock.cpp
+++ b/src/test/fuzz/cmpctblock.cpp
@@ -36,6 +36,7 @@
#include <txmempool.h>
#include <uint256.h>
#include <util/check.h>
+#include <util/threadpool.h>
#include <util/time.h>
#include <util/translation.h>
#include <validation.h>
@@ -139,16 +140,34 @@ void ResetChainmanAndMempool(TestingSetup& setup)
extern void MakeRandDeterministicDANGEROUS(const uint256& seed) noexcept;
+ThreadPool g_pool{"fuzz"};
+Mutex g_pool_mutex;
+size_t g_num_workers = 1;
+
+static void StartPoolIfNeeded() EXCLUSIVE_LOCKS_REQUIRED(!g_pool_mutex)
+{
+ LOCK(g_pool_mutex);
+ if (g_pool.WorkersCount() == g_num_workers) return;
+ g_pool.Start(g_num_workers);
+}
+
void initialize_cmpctblock()
{
- static const auto testing_setup = MakeNoLogFileContext<TestingSetup>();
+ StartPoolIfNeeded();
+ static const auto testing_setup = MakeNoLogFileContext<TestingSetup>(ChainType::REGTEST, {.pool = &g_pool});
g_setup = testing_setup.get();
g_nBits = Params().GenesisBlock().nBits;
ResetChainmanAndMempool(*g_setup);
+ // Stop the threads before the fuzzing engine forks.
+ LOCK(g_pool_mutex);
+ g_pool.Stop();
+ assert(g_pool.WorkersCount() == 0);
}
-FUZZ_TARGET(cmpctblock, .init = initialize_cmpctblock)
+FUZZ_TARGET(cmpctblock, .init = initialize_cmpctblock) EXCLUSIVE_LOCKS_REQUIRED(!g_pool_mutex)
{
+ StartPoolIfNeeded();
+
SeedRandomStateForTest(SeedRand::ZEROS);
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 39c691c336..4866ce5254 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -250,7 +250,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts)
m_node.validation_signals =
// Use synchronous task runner while fuzzing to avoid non-determinism
EnableFuzzDeterminism() ?
- std::make_unique<ValidationSignals>(std::make_unique<util::ImmediateTaskRunner>()) :
+ std::make_unique<ValidationSignals>(std::make_unique<util::ImmediateBackgroundTaskRunner>(opts.pool)) :
std::make_unique<ValidationSignals>(std::make_unique<SerialTaskRunner>(*m_node.scheduler));
{
// Ensure deterministic coverage by waiting for m_service_thread to be running
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 8b1b063218..916b25bd3a 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -22,6 +22,7 @@
#include <util/fs.h>
#include <util/signalinterrupt.h>
#include <util/string.h>
+#include <util/threadpool.h>
#include <util/vector.h>
#include <functional>
@@ -53,6 +54,7 @@ struct TestOpts {
bool setup_net{true};
bool setup_validation_interface{true};
bool min_validation_cache{false}; // Equivalent of -maxsigcachebytes=0
+ ThreadPool* pool;
};
/** Basic testing setup.
diff --git a/src/util/task_runner.h b/src/util/task_runner.h
index 951381823b..1f8ce156e9 100644
--- a/src/util/task_runner.h
+++ b/src/util/task_runner.h
@@ -7,6 +7,7 @@
#include <cstddef>
#include <functional>
+#include <util/threadpool.h>
namespace util {
@@ -47,6 +48,18 @@ public:
size_t size() override { return 0; }
};
+class ImmediateBackgroundTaskRunner : public TaskRunnerInterface
+{
+public:
+ explicit ImmediateBackgroundTaskRunner(ThreadPool* pool) : pool{pool} {};
+ void insert(std::function<void()> func) override { Assert(pool->Submit(std::move(func)))->wait(); }
+ void flush() override {}
+ size_t size() override { return 0; }
+
+private:
+ ThreadPool* pool;
+};
+
} // namespace util
#endif // BITCOIN_UTIL_TASK_RUNNER_H |
|
Ah, I didn't mean to globally replace it. Just in this single fuzz test that needs it call This should allow to drop the diff in setup common. Also, if you move |
|
I guess this is now rfm, if CI passes, right? (After bitcoin/bitcoin#35284) |
Yup the format didn't change so these are still valid |
3 participants
Ran my corpus with
set_cover_merge=1anduse_value_profile=0. Coverage is here, it does not hit some cases because it has only run for a few days, but I can PR those inputs when it does.