The recently merged fuzz harness targeting concurrent reads suffers from poor performance and memory leaks (https://github.com/bitcoin/bitcoin/pull/34866#issuecomment-4614323925).
Fix this by
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35455.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | marcofleon, l0rinc, sedited |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
174 | +Mutex g_read_pool_mutex; 175 | + 176 | +void StartReadPoolIfNeeded() EXCLUSIVE_LOCKS_REQUIRED(!g_read_pool_mutex) 177 | +{ 178 | + LOCK(g_read_pool_mutex); 179 | + if (g_read_pool.WorkersCount() != MAX_READ_WORKERS) g_read_pool.Start(MAX_READ_WORKERS);
Please see #35295 (review)
393 | @@ -414,7 +394,8 @@ FUZZ_TARGET(dbwrapper_concurrent_reads, .init = [] { static auto setup{MakeNoLog 394 | 395 | // Build query list from seeded and random keys. 396 | const size_t num_queries{provider.ConsumeIntegralInRange<size_t>(1, 2'000)}; 397 | - std::vector<ReadQuery> queries; 398 | + enum class ReadOp { Read, Exists, IteratorSeek }; 399 | + std::vector<std::tuple<ReadOp, uint16_t>> queries; 400 | queries.reserve(num_queries); 401 | for (size_t i{0}; i < num_queries; ++i) { 402 | const auto op{static_cast<ReadOp>(provider.ConsumeIntegralInRange<int>(0, 2))};
Related to #34866 (review):
const auto op{provider.PickValueInArray({ReadOp::Read, ReadOp::Exists, ReadOp::IteratorSeek})};
or
enum class ReadOp { Read, Exists, IteratorSeek, kMaxValue = IteratorSeek };
...
const auto op{provider.ConsumeEnum<ReadOp>()};
or if we want to give the fuzzer more control:
const auto consume_key{[&] {
return provider.ConsumeBool()
? keys[provider.ConsumeIntegralInRange<size_t>(0, keys.size() - 1)]
: ConsumeKey(provider);
}};
for (size_t i{0}; i < num_queries; ++i) {
CallOneOf(provider,
[&] { queries.emplace_back(ReadOp::Read, consume_key()); },
[&] { queries.emplace_back(ReadOp::Exists, consume_key()); },
[&] { queries.emplace_back(ReadOp::IteratorSeek, consume_key()); });
}
373 | @@ -397,14 +374,17 @@ FUZZ_TARGET(dbwrapper_concurrent_reads, .init = [] { static auto setup{MakeNoLog 374 | const size_t num_entries{provider.ConsumeIntegralInRange<size_t>(100, 5'000)}; 375 | std::vector<uint16_t> keys; 376 | keys.reserve(num_entries); 377 | + Oracle oracle;
Related to #34866 (review)
425 | + std::iota(order.begin(), order.end(), size_t{0}); 426 | + std::shuffle(order.begin(), order.end(), thread_rng); 427 | + std::vector<uint8_t> v; 428 | + std::string key_str; 429 | + const std::unique_ptr<CDBIterator> it{db.NewIterator()}; 430 | start_latch.arrive_and_wait();
Could we make sure the iterator creation also races the rest and put it after the wait?
case ReadOp::IteratorSeek: {
const std::unique_ptr<CDBIterator> it{db.NewIterator()};
it->Seek(key);
Moved it below latch but still created above the loop for performance.
447 | + it->Seek(key); 448 | + // Skip the obfuscation metadata entry (a non-uint16_t key) if we land 449 | + // on it, so the result matches the oracle, which only tracks user keys. 450 | + if (it->Valid() && it->GetKey(key_str) && key_str == OBFUSCATION_KEY) it->Next(); 451 | + if (const auto oit{oracle.lower_bound(key)}; oit != oracle.end()) { 452 | + assert(it->Valid() && it->GetValue(v) && v == MakeValue(oit->first, oit->second));
can we assert the key as well?
uint16_t actual_key;
assert(it->Valid() && it->GetKey(actual_key) && actual_key == oit->first);
assert(it->GetValue(v) && v == MakeValue(actual_key, oit->second));
421 | - return [&, seed = rng.rand256()]() -> Results { 422 | + return [&, seed = rng.rand256()] { 423 | FastRandomContext thread_rng{seed}; 424 | + std::vector<size_t> order(queries.size()); 425 | + std::iota(order.begin(), order.end(), size_t{0}); 426 | + std::shuffle(order.begin(), order.end(), thread_rng);
nit: could use std::ranges here
std::ranges::shuffle(order, thread_rng);
415 | // Workers + main thread synchronize on the latch so all reads start together.
416 | std::latch start_latch{static_cast<ptrdiff_t>(MAX_READ_WORKERS + 1)};
417 | - std::vector<std::function<Results()>> tasks(MAX_READ_WORKERS);
418 | + std::vector<std::function<void()>> tasks(MAX_READ_WORKERS);
419 | + FastRandomContext rng{ConsumeUInt256(provider)};
420 | std::generate(tasks.begin(), tasks.end(), [&] {
nit: could use std::ranges here:
std::ranges::generate(tasks, [&] {
Thanks @l0rinc. I've applied all your suggestions.
code review ACK 26cac4b68180446dcef2ea5293de23e8cec551d4
tested ACK 26cac4b68180446dcef2ea5293de23e8cec551d4
Reduced num_entries from 5k -> 3k. This should still provide plenty of room to trigger L1->L2 compactions since each value could be expanded up to 32kb * ~320 would exceed 10MB.
Requested by @marcofleon (thanks!).
git diff 26cac4b68180446dcef2ea5293de23e8cec551d4..1ce9e262395bb17e03a7203efab0ef5ae168459b
reACK 1ce9e262395bb17e03a7203efab0ef5ae168459b
ACK 1ce9e262395bb17e03a7203efab0ef5ae168459b
ACK 1ce9e262395bb17e03a7203efab0ef5ae168459b
This might need some further parameter tweaking, but I think we are within our typical timeout parameters now.
166 | @@ -167,7 +167,16 @@ void VerifyIterator(CDBWrapper& dbw, const Oracle& oracle, 167 | } 168 | 169 | /** Maximum number of concurrent reader threads in dbwrapper_concurrent_reads. */ 170 | -constexpr size_t MAX_READ_WORKERS{16}; 171 | +constexpr size_t MAX_READ_WORKERS{8}; 172 | + 173 | +ThreadPool g_read_pool{"dbfuzz"}; 174 | +Mutex g_read_pool_mutex;
unrelated nit: I know you just copy-pasted this from the other fuzz test, but is the Mutex here actually needed? IIUC no fuzz engine supports running the same fuzz target in parallel in the same thread. They'll all either run the fuzz target sequentially in a single thread/process (possibly in several processes), or run them in a loop+occasional fork.
So I don't think the Mutex here is needed. Also, if it was needed, g_read_pool would have to be annotated with clang TSA?
Just a style nit, but I think it would be better to remove it for clarity, or properly document/annotate why it is needed.