Similarly to #28209, this introduces a fuzz target for CCoinsViewDb by using an in-memory LevelDB. We reuse the body of the existing fuzz target for coins_view.
fuzz: a new target for the coins database #28216
pull darosior wants to merge 3 commits into bitcoin:master from darosior:fuzz_coins_db changing 1 files +36 −6-
darosior commented at 12:31 PM on August 4, 2023: member
-
DrahtBot commented at 12:31 PM on August 4, 2023: contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
Code Coverage & Benchmarks
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28216.
<!--021abf342d371248e50ceaed478a90ca-->
Reviews
See the guideline for information on the review process.
Type Reviewers Concept ACK brunoerg, l0rinc Stale ACK TheCharlatan If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #31703 (Use number of dirty cache entries in flush warnings/logs by sipa)
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 Tests on Aug 4, 2023
- darosior force-pushed on Aug 4, 2023
- DrahtBot added the label CI failed on Aug 4, 2023
- DrahtBot removed the label CI failed on Aug 6, 2023
- Allah1king approved
- Allah1king approved
- fanquake referenced this in commit e38c225261 on Aug 15, 2023
-
- darosior force-pushed on Aug 15, 2023
-
darosior commented at 10:34 AM on August 15, 2023: member
Rebased.
- DrahtBot added the label CI failed on Aug 15, 2023
- sidhujag referenced this in commit b3230931a8 on Aug 17, 2023
- darosior force-pushed on Aug 17, 2023
-
darosior commented at 9:13 AM on August 17, 2023: member
Rebased on master to fix the macOS CI.
- DrahtBot removed the label CI failed on Aug 17, 2023
- DrahtBot added the label CI failed on Sep 4, 2023
- DrahtBot removed the label CI failed on Sep 5, 2023
- DrahtBot added the label CI failed on Oct 19, 2023
- DrahtBot removed the label CI failed on Oct 20, 2023
- DrahtBot added the label CI failed on Oct 23, 2023
- DrahtBot added the label Needs rebase on Nov 8, 2023
-
darosior commented at 4:45 PM on December 31, 2023: member
While rebasing this i found another crash in the
coins_viewtarget. Catching the exception onAddCoin()and continuing will causecachedCoinsUsageto underflow: https://github.com/bitcoin/bitcoin/blob/4b1196a9855dcd188a24f393aa2fa21e2d61f061/src/test/fuzz/coins_view.cpp#L67-L75 https://github.com/bitcoin/bitcoin/blob/4b1196a9855dcd188a24f393aa2fa21e2d61f061/src/coins.cpp#L76-L100 - darosior force-pushed on Jan 2, 2024
-
- DrahtBot removed the label Needs rebase on Jan 2, 2024
- darosior force-pushed on Jan 2, 2024
-
darosior commented at 12:29 PM on January 2, 2024: member
Removed the use of a proper fix to simply abuse
Flush()when the exception is hit to avoid the crash. See the first commit. -
brunoerg commented at 1:13 PM on January 2, 2024: contributor
Concept ACK
- DrahtBot removed the label CI failed on Jan 2, 2024
-
dergoegge commented at 3:44 PM on January 3, 2024: member
Coverage for
coins_db: https://dergoegge.github.io/bitcoin-coverage/pr28216/fuzz.coverage/index.html -
in src/test/fuzz/coins_view.cpp:77 in 4a0c47fd8f outdated
70 | @@ -71,6 +71,12 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) 71 | if (e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}) { 72 | assert(!possible_overwrite); 73 | expected_code_path = true; 74 | + // AddCoin() decreases cachedCoinsUsage by the memory usage of the old coin at the beginning and 75 | + // increase it by the value of the new coin at the end. If it throws in the process, the value 76 | + // of cachedCoinsUsage would have been incorrectly decreased, leading to an underflow later on. 77 | + // To avoid this, use Flush() to reset the value of cachedCoinsUsage in sync with the cacheCoins 78 | + // mapping. 79 | + (void)coins_view_cache.Flush();
jamesob commented at 8:22 PM on January 5, 2024:This doesn't merit any updating in non-test code because we never call
AddCoin()within atryoutside of tests, right?
darosior commented at 3:09 PM on January 9, 2024:Yes, thankfully. :) Attempting to overwrite an unspent coin when
possible_overwriteisfalseis always considered a fatal error elsewhere AFAICT.in src/test/fuzz/coins_view.cpp:91 in d4f905d2dd outdated
87 | @@ -87,7 +88,9 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend 88 | (void)coins_view_cache.Sync(); 89 | }, 90 | [&] { 91 | - coins_view_cache.SetBestBlock(ConsumeUInt256(fuzzed_data_provider)); 92 | + uint256 best_block{coins_view_cache.GetBestBlock()}; 93 | + if (is_db && best_block.IsNull()) best_block = uint256::ONE; 94 | + coins_view_cache.SetBestBlock(best_block);
jamesob commented at 4:57 PM on January 8, 2024:To confirm: the removal of
ConsumeUint256()here is okay (and doesn't cause this test to never rotate the best block) because of theBatchWrite()call below, right?For
!is_dbcases this is now a no-op, whereas before it wasn't. Is that intended?
darosior commented at 3:10 PM on January 9, 2024:It's been a while since i wrote this code and to be honest i don't remember what i was thinking. (Note-to-self: comments are good actually.)
As you point out it does change the behaviour of the existing target. Since i don't have a rationale i've updated it to simply keep the existing behaviour.
darosior force-pushed on Jan 9, 2024DrahtBot added the label CI failed on Jan 12, 2024achow101 requested review from TheCharlatan on Apr 9, 2024DrahtBot commented at 6:15 PM on April 17, 2024: contributorCould rebase for fresh CI?
in src/test/fuzz/coins_view.cpp:44 in 8daf327ad2 outdated
43 | @@ -43,13 +44,12 @@ void initialize_coins_view() 44 | g_setup = testing_setup.get();
TheCharlatan commented at 9:22 AM on May 13, 2024:Unrelated to this PR, but is there a reason this is using the full testing setup?
diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 8f3e357a84..c7563ae9f4 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -30 +30 @@ namespace { -const TestingSetup* g_setup; +const BasicTestingSetup* g_setup; @@ -42 +42 @@ void initialize_coins_view() - static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(); + static const auto testing_setup = MakeNoLogFileContext<>(ChainType::MAIN);
darosior commented at 7:24 AM on May 30, 2024:It could use the basic testing setup indeed.
TheCharlatan commented at 9:54 AM on May 13, 2024: contributorlgtm, but I think it would be good to rebase this so the CI can run through it again.
darosior force-pushed on May 30, 2024DrahtBot removed the label CI failed on May 30, 2024TheCharlatan approvedTheCharlatan commented at 11:32 AM on May 30, 2024: contributorACK e79784e517653cd8d29670861528f7adbf4c976a
DrahtBot requested review from brunoerg on May 30, 2024darosior commented at 3:46 PM on June 27, 2024: memberStatus here is i'm looking into some determinism issues pointed out by Niklas: https://gnusha.org/bitcoin-core-dev/2024-06-03.log
darosior commented at 4:12 PM on June 27, 2024: memberAlright, i can't reproduce the non-determinism he observed.
darosior commented at 12:25 PM on July 28, 2024: member@dergoegge could you have a look here when you have a sec?
dergoegge commented at 1:44 PM on July 30, 2024: memberAlright, i can't reproduce the non-determinism he observed.
How did you try to reproduce it? Because I'm still seeing only ~70% stability when running with afl++. @marcofleon this could also be a target to test your "stability debugging" script on.
DrahtBot added the label Needs rebase on Aug 8, 2024PastaPastaPasta referenced this in commit f0951a41d5 on Oct 24, 2024PastaPastaPasta referenced this in commit 580d5956f7 on Oct 24, 2024PastaPastaPasta referenced this in commit 8b8ff1c7d5 on Oct 24, 2024PastaPastaPasta referenced this in commit e364aaf4f0 on Oct 24, 2024maflcko commented at 11:39 AM on November 8, 2024: memberI think fixing stability would be nice, but not a blocker
maflcko commented at 4:14 PM on January 22, 2025: memberAlso, if you rebase, you get a free check from the newly added "fuzz stability checkers"
fuzz: avoid underflow in coins_view target 16aaad3c1d85b9b8dfd0fuzz: move the coins_view target's body into a standalone function
We'll reuse it for a target where the coins view is a DB.
darosior force-pushed on Feb 18, 2025DrahtBot commented at 7:15 PM on February 18, 2025: contributor<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/37422309131</sub>
<details><summary>Hints</summary>
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.
</details>
DrahtBot added the label CI failed on Feb 18, 2025ea0bcbc921fuzz: add a target for the coins database
It reuses the logic from the `coins_view` target, except it uses an in-memory CCoinsViewDB as the backend. Note `CCoinsViewDB` will assert the best block hash is never null, so we slightly modify the coins_view fuzz logic to take care of this.
darosior force-pushed on Feb 18, 2025DrahtBot removed the label Needs rebase on Feb 18, 2025DrahtBot removed the label CI failed on Feb 18, 2025in src/test/fuzz/coins_view.cpp:216 in ea0bcbc921
212 | @@ -207,7 +213,7 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend 213 | 214 | { 215 | std::unique_ptr<CCoinsViewCursor> coins_view_cursor = backend_coins_view.Cursor(); 216 | - assert(!coins_view_cursor); 217 | + assert(is_db ? !!coins_view_cursor : !coins_view_cursor);
davidgumberg commented at 4:59 AM on February 19, 2025:Why explicit conversion with
!!? Doesn't assert cause contextual conversion to bool of the expression, or is this implementation specific?
maflcko commented at 7:32 AM on February 19, 2025:Wouldn't it be shorter and clearer to write
assert(!!coins_view_cursor == is_db);?
l0rinc commented at 1:55 PM on February 19, 2025:I thought I commented on this already, but seems I forgot to submit: 👍 for
assert(is_db == !!coins_view_cursor);or maybe even
assert(is_db != !coins_view_cursor);davidgumberg commented at 6:52 AM on February 19, 2025: contributorReviewed the added fuzz test and it looks good.
I let the fuzz binary run with
FUZZ=coins_dbfor about an hour and nothing crashed.And, I generated some corpora (https://github.com/davidgumberg/qa-assets/tree/coins_db_corpora) for ~20 minutes by doing
$ ./build_fuzz/test/fuzz/test_runner.py -g qa-assets/fuzz_corpora/ coins_dband used the deterministic fuzz coverage tool from #31836 but it found instability:
$ RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_fuzz $PWD/qa-assets/fuzz_corpora/ coins_db<details>
<summary>deterministic-fuzz-coverage output</summary>
--- /bitcoin/build_det/fuzz_det_cov.show.0.txt 2025-02-18 21:44:37.658467363 -0800 +++ /bitcoin/build_det/fuzz_det_cov.show.1.txt 2025-02-18 21:44:38.217482311 -0800 @@ -58994,32 +58994,32 @@ 45| 0| return "leveldb.InternalKeyComparator"; 46| 0|} 47| | - 48| 81.7k|int InternalKeyComparator::Compare(const Slice& akey, const Slice& bkey) const { + 48| 81.8k|int InternalKeyComparator::Compare(const Slice& akey, const Slice& bkey) const { 49| | // Order by: 50| | // increasing user key (according to user-supplied comparator) 51| | // decreasing sequence number 52| | // decreasing type (though sequence# should be enough to disambiguate) - 53| 81.7k| int r = user_comparator_->Compare(ExtractUserKey(akey), ExtractUserKey(bkey)); - 54| 81.7k| if (r == 0) { + 53| 81.8k| int r = user_comparator_->Compare(ExtractUserKey(akey), ExtractUserKey(bkey)); + 54| 81.8k| if (r == 0) { ------------------ - | Branch (54:7): [True: 49.6k, False: 32.0k] + | Branch (54:7): [True: 49.8k, False: 32.0k] ------------------ - 55| 49.6k| const uint64_t anum = DecodeFixed64(akey.data() + akey.size() - 8); - 56| 49.6k| const uint64_t bnum = DecodeFixed64(bkey.data() + bkey.size() - 8); - 57| 49.6k| if (anum > bnum) { + 55| 49.8k| const uint64_t anum = DecodeFixed64(akey.data() + akey.size() - 8); + 56| 49.8k| const uint64_t bnum = DecodeFixed64(bkey.data() + bkey.size() - 8); + 57| 49.8k| if (anum > bnum) { ------------------ - | Branch (57:9): [True: 4.60k, False: 45.0k] + | Branch (57:9): [True: 4.60k, False: 45.2k] ------------------ 58| 4.60k| r = -1; - 59| 45.0k| } else if (anum < bnum) { + 59| 45.2k| } else if (anum < bnum) { ------------------ - | Branch (59:16): [True: 43.6k, False: 1.46k] + | Branch (59:16): [True: 43.6k, False: 1.50k] ------------------ 60| 43.6k| r = +1; 61| 43.6k| } - 62| 49.6k| } - 63| 81.7k| return r; - 64| 81.7k|} + 62| 49.8k| } + 63| 81.8k| return r; + 64| 81.8k|} 65| | 66| |void InternalKeyComparator::FindShortestSeparator(std::string* start, 67| 37| const Slice& limit) const { @@ -60131,17 +60131,17 @@ 52| | 53| 3| ~MemTableIterator() override = default; 54| | - 55| 6.60k| bool Valid() const override { return iter_.Valid(); } + 55| 6.59k| bool Valid() const override { return iter_.Valid(); } 56| 2| void Seek(const Slice& k) override { iter_.Seek(EncodeKey(&tmp_, k)); } 57| 1| void SeekToFirst() override { iter_.SeekToFirst(); } 58| 0| void SeekToLast() override { iter_.SeekToLast(); } 59| 6.62k| void Next() override { iter_.Next(); } 60| 0| void Prev() override { iter_.Prev(); } 61| 6.59k| Slice key() const override { return GetLengthPrefixedSlice(iter_.key()); } - 62| 6.58k| Slice value() const override { - 63| 6.58k| Slice key_slice = GetLengthPrefixedSlice(iter_.key()); - 64| 6.58k| return GetLengthPrefixedSlice(key_slice.data() + key_slice.size()); - 65| 6.58k| } + 62| 6.59k| Slice value() const override { + 63| 6.59k| Slice key_slice = GetLengthPrefixedSlice(iter_.key()); + 64| 6.59k| return GetLengthPrefixedSlice(key_slice.data() + key_slice.size()); + 65| 6.59k| } 66| | 67| 1| Status status() const override { return Status::OK(); } 68| | @@ -60475,12 +60475,12 @@ 150| | 151| | // Accessors/mutators for links. Wrapped in methods so we can 152| | // add the appropriate barriers as necessary. - 153| 78.3k| Node* Next(int n) { - 154| 78.3k| assert(n >= 0); + 153| 78.4k| Node* Next(int n) { + 154| 78.4k| assert(n >= 0); 155| | // Use an 'acquire load' so that we observe a fully initialized 156| | // version of the returned Node. - 157| 78.3k| return next_[n].load(std::memory_order_acquire); - 158| 78.3k| } + 157| 78.4k| return next_[n].load(std::memory_order_acquire); + 158| 78.4k| } 159| 6.13k| void SetNext(int n, Node* x) { 160| 6.13k| assert(n >= 0); 161| | // Use a 'release store' so that anybody who reads through this @@ -60518,9 +60518,9 @@ 193| 1.17k|} 194| | 195| |template <typename Key, class Comparator> - 196| 28.1k|inline bool SkipList<Key, Comparator>::Iterator::Valid() const { - 197| 28.1k| return node_ != nullptr; - 198| 28.1k|} + 196| 28.2k|inline bool SkipList<Key, Comparator>::Iterator::Valid() const { + 197| 28.2k| return node_ != nullptr; + 198| 28.2k|} 199| | 200| |template <typename Key, class Comparator> 201| 14.1k|inline const Key& SkipList<Key, Comparator>::Iterator::key() const { @@ -60531,8 +60531,8 @@ 206| |template <typename Key, class Comparator> 207| 6.62k|inline void SkipList<Key, Comparator>::Iterator::Next() { 208| 6.62k| assert(Valid()); - 209| 6.62k| node_ = node_->Next(0); - 210| 6.62k|} + 209| 6.63k| node_ = node_->Next(0); + 210| 6.63k|} 211| | 212| |template <typename Key, class Comparator> 213| 0|inline void SkipList<Key, Comparator>::Iterator::Prev() { @@ -65829,10 +65829,10 @@ 31| 11.3k| Slice() : data_(""), size_(0) {} 32| | 33| | // Create a slice that refers to d[0,n-1]. - 34| 364k| Slice(const char* d, size_t n) : data_(d), size_(n) {} + 34| 365k| Slice(const char* d, size_t n) : data_(d), size_(n) {} 35| | 36| | // Create a slice that refers to the contents of "s" - 37| 16.6k| Slice(const std::string& s) : data_(s.data()), size_(s.size()) {} + 37| 16.5k| Slice(const std::string& s) : data_(s.data()), size_(s.size()) {} 38| | 39| | // Create a slice that refers to s[0,strlen(s)-1] 40| 26| Slice(const char* s) : data_(s), size_(strlen(s)) {} @@ -65842,10 +65842,10 @@ 44| | Slice& operator=(const Slice&) = default; 45| | 46| | // Return a pointer to the beginning of the referenced data - 47| 345k| const char* data() const { return data_; } + 47| 347k| const char* data() const { return data_; } 48| | 49| | // Return the length (in bytes) of the referenced data - 50| 576k| size_t size() const { return size_; } + 50| 579k| size_t size() const { return size_; } 51| | 52| | // Return true iff the length of the referenced data is zero 53| 5.76k| bool empty() const { return size_ == 0; } @@ -65910,30 +65910,30 @@ 101| | 102| 0|inline bool operator!=(const Slice& x, const Slice& y) { return !(x == y); } 103| | - 104| 83.5k|inline int Slice::compare(const Slice& b) const { - 105| 83.5k| const size_t min_len = (size_ < b.size_) ? size_ : b.size_; - ^201 ^83.3k + 104| 83.7k|inline int Slice::compare(const Slice& b) const { + 105| 83.7k| const size_t min_len = (size_ < b.size_) ? size_ : b.size_; + ^201 ^83.5k ------------------ - | Branch (105:26): [True: 201, False: 83.3k] + | Branch (105:26): [True: 201, False: 83.5k] ------------------ - 106| 83.5k| int r = memcmp(data_, b.data_, min_len); - 107| 83.5k| if (r == 0) { + 106| 83.7k| int r = memcmp(data_, b.data_, min_len); + 107| 83.7k| if (r == 0) { ------------------ - | Branch (107:7): [True: 51.7k, False: 31.8k] + | Branch (107:7): [True: 51.8k, False: 31.9k] ------------------ - 108| 51.7k| if (size_ < b.size_) + 108| 51.8k| if (size_ < b.size_) ------------------ - | Branch (108:9): [True: 0, False: 51.7k] + | Branch (108:9): [True: 0, False: 51.8k] ------------------ 109| 0| r = -1; - 110| 51.7k| else if (size_ > b.size_) + 110| 51.8k| else if (size_ > b.size_) ------------------ - | Branch (110:14): [True: 0, False: 51.7k] + | Branch (110:14): [True: 0, False: 51.8k] ------------------ 111| 0| r = +1; - 112| 51.7k| } - 113| 83.5k| return r; - 114| 83.5k|} + 112| 51.8k| } + 113| 83.7k| return r; + 114| 83.7k|} 115| | 116| |} // namespace leveldb 117| | @@ -69664,11 +69664,11 @@ 46| 30.3k| return reinterpret_cast<char*>(ptr); 47| 30.3k|} 48| | - 49| 20.0k|void PutVarint32(std::string* dst, uint32_t v) { - 50| 20.0k| char buf[5]; - 51| 20.0k| char* ptr = EncodeVarint32(buf, v); - 52| 20.0k| dst->append(buf, ptr - buf); - 53| 20.0k|} + 49| 20.1k|void PutVarint32(std::string* dst, uint32_t v) { + 50| 20.1k| char buf[5]; + 51| 20.1k| char* ptr = EncodeVarint32(buf, v); + 52| 20.1k| dst->append(buf, ptr - buf); + 53| 20.1k|} 54| | 55| 89|char* EncodeVarint64(char* dst, uint64_t v) { 56| 89| static const int B = 128; @@ -69979,9 +69979,9 @@ 24| | 25| 3| const char* Name() const override { return "leveldb.BytewiseComparator"; } 26| | - 27| 83.5k| int Compare(const Slice& a, const Slice& b) const override { - 28| 83.5k| return a.compare(b); - 29| 83.5k| } + 27| 83.7k| int Compare(const Slice& a, const Slice& b) const override { + 28| 83.7k| return a.compare(b); + 29| 83.7k| } 30| | 31| | void FindShortestSeparator(std::string* start, 32| 37| const Slice& limit) const override { The coverage was not determinstic between runs. The fuzz target input was /bitcoin/qa-assets/fuzz_corpora/coins_db/0d249c3962392259953e8ce2148addd7ed92cda5.</details>
in src/test/fuzz/coins_view.cpp:319 in ea0bcbc921
314 | + .path = "", // Memory only. 315 | + .cache_bytes = 1 << 20, // 1MB. 316 | + .memory_only = true, 317 | + }; 318 | + CCoinsViewDB coins_db{std::move(db_params), CoinsViewOptions{}}; 319 | + TestCoinsView(fuzzed_data_provider, coins_db, /* is_db = */ true);
l0rinc commented at 2:04 PM on February 19, 2025:The boolean parameter basically decides if the second parameter is a database view or not. We could reduce this duplication by deducing it inside the
TestCoinsViewmethod instead:void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend_coins_view) { const bool is_db{!!dynamic_cast<CCoinsViewDB*>(&backend_coins_view)};or if you prefer:
void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend_coins_view) { const bool is_db{typeid(backend_coins_view) == typeid(CCoinsViewDB)};in src/test/fuzz/coins_view.cpp:315 in ea0bcbc921
310 | +FUZZ_TARGET(coins_db, .init = initialize_coins_view) 311 | +{ 312 | + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; 313 | + auto db_params = DBParams{ 314 | + .path = "", // Memory only. 315 | + .cache_bytes = 1 << 20, // 1MB.
l0rinc commented at 2:08 PM on February 19, 2025:nit: I know this was just copied over, but technically this is
.cache_bytes = 1 << 20, // 1MiB.in src/test/fuzz/coins_view.cpp:310 in ea0bcbc921
305 | + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; 306 | + CCoinsView backend_coins_view; 307 | + TestCoinsView(fuzzed_data_provider, backend_coins_view); 308 | +} 309 | + 310 | +FUZZ_TARGET(coins_db, .init = initialize_coins_view)
l0rinc commented at 3:09 PM on February 19, 2025:The file name previously coincided with the only fuzz target - now a
coins_viewcontains acoins_dbtarget as well. Given that the type isCCoinsViewDB, we could rename this to:FUZZ_TARGET(coins_view_db, .init = initialize_coins_view)in src/test/fuzz/coins_view.cpp:314 in ea0bcbc921
309 | + 310 | +FUZZ_TARGET(coins_db, .init = initialize_coins_view) 311 | +{ 312 | + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; 313 | + auto db_params = DBParams{ 314 | + .path = "", // Memory only.
l0rinc commented at 3:10 PM on February 19, 2025:This is already obvious from two lines below:
.path = "",in src/test/fuzz/coins_view.cpp:77 in ea0bcbc921
68 | @@ -69,6 +69,12 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) 69 | if (e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}) { 70 | assert(!possible_overwrite); 71 | expected_code_path = true; 72 | + // AddCoin() decreases cachedCoinsUsage by the memory usage of the old coin at the beginning and 73 | + // increase it by the value of the new coin at the end. If it throws in the process, the value 74 | + // of cachedCoinsUsage would have been incorrectly decreased, leading to an underflow later on. 75 | + // To avoid this, use Flush() to reset the value of cachedCoinsUsage in sync with the cacheCoins 76 | + // mapping. 77 | + (void)coins_view_cache.Flush();
l0rinc commented at 3:14 PM on February 19, 2025:What's the purpose of the comment? Why not add that to the commit message only?
davidgumberg commented at 4:39 PM on February 19, 2025:This comment seems to me to document relevant and lasting context for what is otherwise a non-sequitur, anyone touching this line in the future should know what motivated putting it here.
l0rinc commented at 3:15 PM on February 19, 2025: contributorI still can't run any fuzzing on my Mac (for the past ~5 months), so I only added code review comments based on what I found.
Concept ACK
marcofleon commented at 3:47 PM on March 26, 2025: contributorI explored the instablilty of the
coins_dbtarget some more and it seems to be in LevelDB, so I think we can ignore it as an issue for this fuzz test.I generated two coverage reports: one with a corpus of inputs that were all stable (afl++ showing 99.3%) and then one with that same corpus but just a single added unstable input (afl++ showing ~75%). You can see that basically all of the differences in coverage are in LevelDB. If you look at the coverage for
db_impl.ccyou'll see that the one unstable input reaches the compaction code (probably seen clearest inMaybeScheduleCompaction()).Anyway, all this to say that I don't think the instability should be a blocker for this harness. Unless we want to start changing LevelDB...
darosior commented at 8:18 PM on March 27, 2025: memberClosing my old open PRs. I have intended to come back to it for a while but as a matter of fact i did not. Anyone interested feel free to pick it up.
darosior closed this on Mar 27, 2025
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-15 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me
