Adds a fuzz harness for testing compact blocks, similar to process_message(s). It can make (in)valid blocks, reconstruct blocks with in-mempool txns, mark peers as HB, and has high stability in AFL++ (~98-99%).
Coverage is here (look in src/blockencodings.cpp, relevant compact block bits in src/net_processing.cpp).
<!--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/33300.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | instagibbs |
| Concept ACK | frankomosh |
| Approach ACK | l0rinc |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
There does not seem to be a way without signal handlers to delete the static, cached datadir when fuzzing is complete. Another issue is that multi-core fuzzing via AFL++ does not work because each worker will try to wipe the cached datadir (created via
-testdatadirwhich wipes if the directory exists) which will cause other workers to crash if they are trying to copy it. I could not figure out a way for each worker to have their own cached datadir.
Is my understanding correct that AFL will run init (create the static dir), then fork into several fuzz processes, which run for N iterations and then shut down and delete the static dir? (On the next re-fork, it will fail to find the static dir)?
If yes, a solution could be to lazy-init the static dir after init, after the fork. Also, you may have to use the pid (or os-rand) to name the static folder to avoid collisions.
As for deleting the dirs, maybe it is possible to spin up several testing setups at the same time:
(Not sure if this is possible, though)
Is my understanding correct that AFL will run init (create the static dir), then fork into several fuzz processes, which run for N iterations and then shut down and delete the static dir? (On the next re-fork, it will fail to find the static dir)?
We removed the __AFL_INIT call, so it will actually fork prior to init, so I think all that's needed is to randomize the static dir name as you suggested as well.
Is my understanding correct that AFL will run init (create the static dir), then fork into several fuzz processes, which run for N iterations and then shut down and delete the static dir? (On the next re-fork, it will fail to find the static dir)?
My description was a bit vague. Since the harness uses -testdatadir in init to create the static datadir, it does not get deleted in the TestingSetup destructor (by design of -testdatadir). Any time init is called, it will wipe the static datadir if it already exists. This can happen after N iterations (100,000 currently) or with a newly spawned worker after forking since the fork point is prior to init like @dergoegge mentioned. If the datadir is deleted while a fuzz iteration tries to copy the non-existent directory, it will crash.
a solution could be to lazy-init the static dir after init, after the fork
I considered this, but I think this introduces some (slight) non-determinism as the directory may/may not exist?
Also, you may have to use the pid (or os-rand) to name the static folder to avoid collisions.
Yup, I agree. I think this randomization can only be done if the static datadir can be deleted at the end of fuzzing, otherwise there will be lots of these lying around?
As for deleting the dirs, maybe it is possible to spin up several testing setups at the same time:
Why is the dummy testing setup needed in this example? Could it instead just be two testing setups (one for the static datadir, one for the copied datadir)? I considered something similar to this as well where there is a static TestingSetup that gets destructed at the end of fuzzing (and wipes the static datadir) and one for each fuzz iteration (that gets destructed at the end of the iteration), but I was worried that the static TestingSetup would introduce more non-determinism? For example, what if this static TestingSetup has scheduling going on or is using the static datadir while we try to copy from it?
I was able to get AFL++ multi-core fuzzing to work by adding a static FuzzedDirectoryWrapper instance that deletes the passed path in its destructor as well as using a random element to each statically named datadir. Each instance is in the high 98-99% stability and does not crash after hitting 100,000 iterations (yay). I was not able to use multiple TestingSetups at the same time without causing several panics due to assertions about globals. I will fix the lint, tidy jobs and also see if the deterministic-fuzz-coverage issues still pop up.
There is non-determinism here because m_dirty_blockindex is std::set<CBlockIndex*> and sorts based on the pointer. This can be fixed by adding a function object that compares the block hashes. However, I did not know if this should be added just for fuzz code and I have not run benchmarks.
After patching the above non-determinism locally, there is also non-determinism in the scheduler because this fuzz test uses the scheduler. I am guessing this is due to the RegisterValidationInterface call in the fuzz test notifying when transactions are being added or removed, and blocks being mined.
35 | +using namespace util::hex_literals; 36 | + 37 | +namespace { 38 | + 39 | +//! Fee each created tx will pay. 40 | +const CAmount amount_fee = 1000;
nit
const CAmount AMOUNT_FEE{1000};
Thanks, done.
354 | + return; 355 | + } 356 | + 357 | + // Choose an existing block and send a HEADERS message for it. 358 | + size_t index = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, num_blocks - 1); 359 | + CBlock block = *info[index].block;
CBlock block = *info[index].block;
block.vtx.clear();
The headers message is a vector of blocks without transactions, so I think you should clear the copy of the block here? Although, technically this doesn't matter because there's only one header in the vector being sent, so the following txs are just ignored.
Ah nice catch, I completely missed this. Done.
315 | + // Remove from shorttxids since we've prefilled. Subtract however many txs have been prefilled. 316 | + cmpctBlock.EraseShortTxIDs(i - 1 - num_erased); 317 | + ++num_erased; 318 | + } 319 | + 320 | + CBlockHeaderAndShortTxIDs baseCmpctBlock = cmpctBlock;
CBlockHeaderAndShortTxIDs baseCmpctBlock = cmpctBlock;
nit: there are few instances of camel case usage here. Our convention is to use snake case (see dev notes).
Thanks, I believe I've fixed all of the camel case.
Did a first pass, overall approach looks good to me
The latest push adds two commits:
TestingSetup inside FUZZ_TARGET and scheduling a promise can be non-deterministic as the scheduler's serviceQueue may start with a non-empty taskQueue. This is not an issue in the other fuzz tests because their TestingSetup is created in .init and ResetCoverageCounters is called after.m_dirty_blockindex, whereas sorting by block hash won't). I added it to show the change needed to make this harness fully non-deterministic. It is also possible to add an #ifdef so that it doesn't negatively affect production code, but I also don't want to set a precedent and litter the codebase with fuzz-specific macros. The sorting is needed as otherwise leveldb's MemTable will be non-deterministic across runs. I tried some alternatives, but this is the best I could come up with that made the determinstic-fuzz-coverage script happy.256 | + } 257 | + } 258 | + 259 | + CBlockIndex* pindexPrev{WITH_LOCK(::cs_main, return setup->m_node.chainman->m_blockman.LookupBlockIndex(prev))}; 260 | + setup->m_node.chainman->GenerateCoinbaseCommitment(*block, pindexPrev); 261 | + FinalizeHeader(header, *setup->m_node.chainman);
Shouldn't *block be passed in here instead of header? Right now, I think the block just ends up with the random nonce from when the header was created. Although this still works half the time with the simplifed PoW check. Also, calling GetHash() on the header includes the merkle root, so finalizing the header should be after we calculate the merkle root below.
Ah yes, nice catch. I was wondering why the PoW check was still failing so many times in the coverage.
Fixed in ba89cdd93ee39e884c924ae12737dd06639cee8f, verified that the CheckBlockHeader call in validation.cpp no longer fails.
e2f921458913bcbbe74115cdb2174b0ab31784f2 -> ed813c48f826d083becf93c741b483774c850c86:
211 | + 212 | + m_path_root = rand_path(); 213 | + TryCreateDirectories(m_path_root); 214 | + 215 | + // Copy the cached directory into the newly created temporary directory. 216 | + fs::copy(cached_dir, m_path_root, fs::copy_options::recursive);
When fuzzing on macOS (Apple Silicon), I ran into lots of crashes when copying the cached dir into the destination:
libc++abi: terminating due to uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in copy_file: File exists ["/var/folders/wc/3l6l_0b92zzg54v6rmvqqn300000gn/T/test_common bitcoin/cmpctblock/6c5e38c6dcfbc2607321/regtest/blocks/xor.dat"] ["/var/folders/wc/3l6l_0b92zzg54v6rmvqqn300000gn/T/cmpctblock_cachedca12974312651650d6b0/datadir/regtest/blocks/xor.dat"]
The issue is that the destination dir may already exist before TryCreateDirectories(), and it happens when the AFL++ fork server is enabled (it does not happen when fuzzing with AFL_NO_FORKSRV=1). It appears that forked processes are running into collisions when generating the random path (rand_path()) for creating the destination dir, and when we switch to strong randomness (like how the static dir name is generated), the issue seems to be resolved:
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index e9768d4c5c..ac477caa62 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -164,8 +164,9 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
// tests, such as the fuzz tests to run in several processes at the
// same time, add a random element to the path. Keep it small enough to
// avoid a MAX_PATH violation on Windows.
- const auto rand{HexStr(g_rng_temp_path.randbytes(10))};
- return fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / rand;
+ std::vector<unsigned char> random_path_suffix(10);
+ GetStrongRandBytes(random_path_suffix);
+ return fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / HexStr(random_path_suffix);
};
if (m_node.args->IsArgSet("-testdatadir")) {
Are you fuzzing with persistent mode?
Yes. I am compiling using afl-clang-fast++ (it seems that other options are either unavailable or obsolete on macOS). However, I just added a #undef __AFL_LOOP at the top of fuzz.cpp to disable persistent mode (I made sure I no longer see the [+] Persistent mode binary detected log when running) and reverted the above change to rand_path(), but the same crash still occurs.
Think this is because the child processes have the same g_rng_temp_path so they create the same directory. Will need to see if GetStrongRandBytes introduces non-determinism. I'm confused why fuzzing on debian didn't bring this up.
Thanks for pointing this out, GetStrongRandBytes does not introduce non-determinism. I'll change the next time I push up by making rand_path accept a bool strong so that the original behavior when -testdatadir is set is unchanged.
Since the crash I explained wasn't observed by others, I doubted whether it was actually coming from multiple child processes running simultaneously or from missed data dir cleanup. So I added a bunch of logs (including pid and parent pid in each) to investigate this further and found this after reading the logs for multiple runs:
I observed that AFL++ ran one worker process at a time, not multiple workers in parallel. Each worker created a static cached directory once, then ran multiple fuzz iterations sequentially; each iteration creating and destroying a temporary data directory. This worked fine until a worker died unexpectedly (not yet sure why), leaving behind both its last temp data directory and its static cached directory without cleanup.
Each new worker had the same g_rng_temp_path as the previous worker, so it generated the same temp directory paths. When the next worker started, it faced the copy crash at exactly the same iteration where the previous worker died. For example, if process A died before completing iteration N, it left behind the temp data directory at that iteration. Process B then faced the fs::copy() crash at exactly iteration N, since the generated sequence of temp data directory paths was identical. This worker also died, leaving behind another orphaned static cached directory. This flow would repeat, eventually filling the disk with orphaned directories.
I'm confused why fuzzing on debian didn't bring this up.
Yeah, I am also confused by this. While my case wasn't exactly a multi-process execution issue, it seems a true multi-process execution should observe the same crash because of the same g_rng_temp_path across processes.
Some signals are not specifically handled by the fuzzer, so if it creates a datadir (i.e. process_messages) and you Ctrl-C, it will leave it around instead of deleting it in ~BasicTestingSetup. In any case, not using g_rng_temp_path seems like the right choice.
142 | + 143 | + LOCK(NetEventsInterface::g_msgproc_mutex); 144 | + 145 | + std::vector<CNode*> peers; 146 | + auto& connman = *static_cast<ConnmanTestMsg*>(setup->m_node.connman.get()); 147 | + for (int i = 0; i < 3; ++i) {
As we discussed a bit offline, increasing the number of peers here to greater than 3 should be enough to hit some of the missing "eviction" logic in MaybeSetPeerAsAnnouncingHeaderAndIDs. Maybe increasing to 8 peers would be fine? Although not sure if that would test anything more (somewhere else) than if it were 4.
Will compare the coverage from increasing to 4 vs increasing to 8.
Neither 4 nor 8 helped hit the case, will look into it more.
Non-blocking I'd say, so could be a followup once it's figured out.
Strange though, now I'm curious.
I think outbound connections are getting disconnected for some reason, latest coverage is here. On a local branch, I modified the harness to return early if any of the test nodes did not complete the version-verack handshake, but coverage is still missing.
This is proving to be a difficult check to hit. Part of the issue is that outbound connections are unlikely to be kept around after the initial handshake because they may not have the desired service flags (this is also an issue in the existing process_message(s) harnesses): https://github.com/bitcoin/bitcoin/blob/d3056bc149f605225f22b1cc83b1a2d1cea64258/src/net_processing.cpp#L3613-L3620
The fuzz code can either consume a single service flag or randomly pick a uint64 that can contain the service flag (the probability for two bits being set i.e. NODE_NETWORK | NODE_WITNESS is 25%): https://github.com/bitcoin/bitcoin/blob/d3056bc149f605225f22b1cc83b1a2d1cea64258/src/test/fuzz/util/net.cpp#L460
I made a small patch that:
NODE_NETWORK | NODE_WITNESSWith this, I see a lot more hits in MaybeSetPeerAsAnnouncingHeaderAndIDs in less time than earlier campaigns, but the eviction logic is not hit because lNodesAnnouncingHeaderAndIDs is never 3. I think the fuzzer is just getting lost in many of the other branches.
Ok, Claude reverse-engineered FuzzedDataProvider with a python script and I was able to produce an input that hit the eviction case on 3c515102f4e83b4117f52d7d0fd41aac59740b16:
echo "CgAAAX8AAAFcAAoAAAJ/AAABXAAKAAADfwAAAVwACgAABH8AAAFcAAABAQEBAQEBAwEAAAAAAAAAAAAABAAAgAAAAAABAAABAQEBAQECAQAAAAAAAAAAAAAEAACAAAAAAAEAAAEBAQEBAQEBAAAAAAAAAAAAAAQAAIAAAAAAAQAAAQEBAAEAAAAAAAAAAAAABAAAgAAAAAABAAABAQEBAAEAAAAAAAAAAAAABAAAgAAAAAABAAABAQEDAAMBAQECAAMBAQEBAAMBAAADAQAGAQEBAQEBAQEBAQEBAUiVAAABAQkAAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAY0gAAEBAwAAAAAAAAABAQEBAQEBAQEBAQFIlQAAAQEJAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQGNIAABAQIAAAAAAAAAAQEBAQEBAQEBAQEBSJUAAAEBCQAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEBjSAAAQEBAAAAAAAAAAEBAQEBAQEBAQEBAQEBAUiVAAABAQkAAAAAAAAAAAEAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEBjSAAAQEAAAAAAAAAAA==" | base64 -d -o special-input
My hunch is that the reason this was so hard to hit, in addition to the things mentioned in the previous comment, is that FuzzedDataProvider consumes from the front and back of the input and mutations easily mess up the structure of an existing input.
Nice. Sometimes just gotta hand-craft a golden input.
Still sort of weird to me that the fuzzer couldn't do it on its own. Even with your patch, there were just never 4 different peers that sent SENDCMPCT and delivered a valid block? I'm assuming MaybeSetPeerAsAnnouncingHeaderAndIDs was being called for other peers too, but they would just hit the early return.
Also, I briefly looked at FuzzedDataProvider.h and I'm not sure if consuming from the front or back of the input affects the structure of the other end. The back always reads from the end of the original buffer (ConsumeIntegralInRange) regardless of how much the front has consumed (Advance). I also might not quite understand what you mean exactly. Either way, a lot has to be right for this branch to be hit and mutations extending from 2 to 3 to 4 passing peers seems to break previous input structure somehow.
Even with your patch, there were just never 4 different peers that sent SENDCMPCT and delivered a valid block?
Yup, you can see the coverage that shows this in the OP. MaybeSetPeerAsAnnouncingHeaderAndIDs is called 8.32K times and then it only adds to lNodesAnnouncingHeaderAndIDs 1.98K times and never hits the >= 3 logic. Results roughly the same across multiple multi-week campaigns, coverage generated using anywhere from 50K - 200K inputs.
The back always reads from the end of the original buffer (ConsumeIntegralInRange) regardless of how much the front has consumed (Advance)
I'm wrong here oops. I was thinking that because it's hard for me to reason about what bytes correspond to different actions (like successfully making an outbound peer) that the fuzzer would also have a hard time figuring it out, but that's not the case. I think if the fuzzer mutates bytes uniformly at random then it does not matter how the input is consumed (front + back), but if it's biased then it would matter how the input is consumed; I think the mutations are uniformly at random in AFL++. If the input was linear and always consumed from the front, it would be easier (for me) to construct the input.
491 | @@ -487,7 +492,7 @@ bool BlockManager::WriteBlockIndexDB() 492 | } 493 | std::vector<const CBlockIndex*> vBlocks; 494 | vBlocks.reserve(m_dirty_blockindex.size()); 495 | - for (std::set<CBlockIndex*>::iterator it = m_dirty_blockindex.begin(); it != m_dirty_blockindex.end();) { 496 | + for (std::set<CBlockIndex*, CBlockIndexBlockHashComparator>::iterator it = m_dirty_blockindex.begin(); it != m_dirty_blockindex.end();) {
I feel like I might be overlooking something here, but could we keep m_dirty_blockindex the same and then if EnableFuzzDeterminism() just sort vBlocks by hash before calling WriteBatchSync()?
I guess I'm asking is there a reason why m_dirty_blockindex needs to be sorted by hash from the beginning vs right before the write.
Answered in the review club, also posting here:
Without this commit, leveldb's MemTable is non-deterministic since it depends on insert order. If we sort vBlocks right before calling WriteBatchSync, then the leveldb non-determinism is solved, but the number of times we call the comparison function varies since the initial ordering before sorting varies across runs of the same input. That means we need to sort when we insert into m_dirty_blockindex as the order of CBlockIndex* we insert here is deterministic (i.e. we'll always insert A then B, even if it may sort as {A, B} or {B, A}). This is definitely confusing, I used pencil and paper to work this out.
I did a simple benchmark and it showed ~O(n log n) slowdown. I was not sure how to bench this in a production scenario or what a realistic load here would look like. I guess it's possible to bench two different IBDs, but there may be other non-IBD scenarios?
Is my understanding correct that we want to remove useless jitter that doesn't actually increase code coverage but confuses the fuzzer, such as the internal Random in leveldb's skiplist?
It seems to me that should be deterministic though, as long as we're actually giving it the same work on the same thread: https://github.com/bitcoin/bitcoin/blob/a7e80676104b5c90c5b5e3bfab815d55a9061052/src/leveldb/db/skiplist.h#L330
And (as I think you also mentioned) the write and iteration order should be deterministic, regardless of the inputs:
diff --git a/src/test/dbwrapper_tests.cpp b/src/test/dbwrapper_tests.cpp
--- a/src/test/dbwrapper_tests.cpp (revision c44048159359f8a7335a00b768548b351b4181a5)
+++ b/src/test/dbwrapper_tests.cpp (date 1762874342278)
@@ -422,5 +422,29 @@
BOOST_CHECK(fs::exists(lockPath));
}
+BOOST_AUTO_TEST_CASE(leveldb_memtable_rand256_sorted)
+{
+ const fs::path path{m_args.GetDataDirBase() / "dbwrapper_memtable_sorted"};
+ CDBWrapper dbw{{.path = path, .cache_bytes = 1_MiB, .memory_only = true, .wipe_data = true}};
+
+ // Write a batch of random keys.
+ CDBBatch batch{dbw};
+ for (int i{0}; i < 10; ++i) {
+ auto key = m_rng.rand256().ToString().substr(0, 10);
+ BOOST_TEST_MESSAGE("Adding: " + key);
+ batch.Write(key, /*value=*/0);
+ }
+ dbw.WriteBatch(batch);
+
+ // Get iterator and collect all keys as they are stored.
+ std::unique_ptr<CDBIterator> it{dbw.NewIterator()};
+ for (std::string prev{""}; it->Valid(); it->Next()) {
+ std::string key;
+ BOOST_REQUIRE(it->GetKey(key));
+ BOOST_TEST_MESSAGE("Got: " + key);
+ BOOST_CHECK_LE(prev, key);
+ prev = key;
+ }
+}
BOOST_AUTO_TEST_SUITE_END()
These identity sets would indeed cause cross-run nondeterminism. We've probably done it since we don't expect to have duplicate objects, we just care about pointer-equality for otherwise volatile instances and we don't really care about order, just set properties. But having sorted sets of pointers does introduce needless jitter that I agree we should stabilize - and we seem to have quite a few:
git grep -E 'std::set<[^>]+\*>'
src/node/blockstorage.cpp: for (std::set<CBlockIndex*>::iterator it = m_dirty_blockindex.begin(); it != m_dirty_blockindex.end();) {
src/node/blockstorage.h: std::set<CBlockIndex*> m_dirty_blockindex;
src/rpc/blockchain.cpp: std::set<const CBlockIndex*> setOrphans;
src/rpc/blockchain.cpp: std::set<const CBlockIndex*> setPrevs;
src/rpc/blockchain.cpp: for (std::set<const CBlockIndex*>::iterator it = setOrphans.begin(); it != setOrphans.end(); ++it) {
src/test/fuzz/txgraph.cpp: std::set<std::vector<TxGraph::Ref*>> clusters;
src/txgraph.cpp: std::set<const Cluster*> expected_clusters[MAX_LEVELS];
src/txgraph.cpp: std::set<const Cluster*> actual_clusters;
src/wallet/rpc/wallet.cpp: std::set<ScriptPubKeyMan*> spkms;
src/wallet/wallet.cpp:std::set<ScriptPubKeyMan*> CWallet::GetActiveScriptPubKeyMans() const
src/wallet/wallet.cpp: std::set<ScriptPubKeyMan*> spk_mans;
src/wallet/wallet.cpp:std::set<ScriptPubKeyMan*> CWallet::GetAllScriptPubKeyMans() const
src/wallet/wallet.cpp: std::set<ScriptPubKeyMan*> spk_mans;
src/wallet/wallet.cpp:std::set<ScriptPubKeyMan*> CWallet::GetScriptPubKeyMans(const CScript& script) const
src/wallet/wallet.cpp: std::set<ScriptPubKeyMan*> spk_mans;
src/wallet/wallet.h: std::set<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const;
src/wallet/wallet.h: std::set<ScriptPubKeyMan*> GetAllScriptPubKeyMans() const;
src/wallet/wallet.h: std::set<ScriptPubKeyMan*> GetScriptPubKeyMans(const CScript& script) const;
I think we should tackle these in a separate PR. In other cases we either have dedicated sorting methods for the red-black trees (e.g. #33637) or hash them and use a hashset (e.g. #30442). In this particular case sorting could help in seeding the MemTable since we'd be sorting already sorted content - as long as iteration is not a lot slower we should keep an std::set, but I would definitely add a dedicated sorter to avoid jitter (but I would prefer doing it in a separate PR and for all other cases as well. I will try something like that)
Is my understanding correct that we want to remove useless jitter that doesn't actually increase code coverage but confuses the fuzzer, such as the internal Random in leveldb's skiplist?
It depends on the specific fuzz engine as far as exactly what happens (e.g. how it prioritizes or drops executed inputs). Speaking generally, if the fuzzer sees increased coverage (due to non-determinism) it could prioritize a no-coverage-gain input and if it sees decreased coverage it could de-prioritize an input where it shouldn't.
To clarify if I was unclear above, this non-determinism doesn't show up in what hits the disk (as it's a k-v store), only in the MemTable which depends on the order that we write to it.
I think we should tackle these in a separate PR.
This sounds fine to me and I can drop the commit here. #33469 is another case where a set of pointers caused non-determinism during fuzzing.
<details> <summary> contrib/devtools/deterministic-fuzz-coverage output without this commit </summary>
diff --git a/Users/eugenesiegel/btc/bitcoin/build_fuzzcov/fuzz_det_cov.show.t1.a.txt b/Users/eugenesiegel/btc/bitcoin/build_fuzzcov/fuzz_det_cov.show.t1.b.txt
index 8389ec5527..68708cc9fc 100644
--- a/Users/eugenesiegel/btc/bitcoin/build_fuzzcov/fuzz_det_cov.show.t1.a.txt
+++ b/Users/eugenesiegel/btc/bitcoin/build_fuzzcov/fuzz_det_cov.show.t1.b.txt
@@ -61402,15 +61402,15 @@
45| 0| return "leveldb.InternalKeyComparator";
46| 0|}
47| |
- 48| 6.11k|int InternalKeyComparator::Compare(const Slice& akey, const Slice& bkey) const {
+ 48| 6.01k|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| 6.11k| int r = user_comparator_->Compare(ExtractUserKey(akey), ExtractUserKey(bkey));
- 54| 6.11k| if (r == 0) {
+ 53| 6.01k| int r = user_comparator_->Compare(ExtractUserKey(akey), ExtractUserKey(bkey));
+ 54| 6.01k| if (r == 0) {
------------------
- | Branch (54:7): [True: 61, False: 6.05k]
+ | Branch (54:7): [True: 61, False: 5.95k]
------------------
55| 61| const uint64_t anum = DecodeFixed64(akey.data() + akey.size() - 8);
56| 61| const uint64_t bnum = DecodeFixed64(bkey.data() + bkey.size() - 8);
@@ -61426,8 +61426,8 @@
60| 51| r = +1;
61| 51| }
62| 61| }
- 63| 6.11k| return r;
- 64| 6.11k|}
+ 63| 6.01k| return r;
+ 64| 6.01k|}
65| |
66| |void InternalKeyComparator::FindShortestSeparator(std::string* start,
67| 10| const Slice& limit) const {
@@ -61620,10 +61620,10 @@
92| |bool ParseInternalKey(const Slice& internal_key, ParsedInternalKey* result);
93| |
94| |// Returns the user key portion of an internal key.
- 95| 13.1k|inline Slice ExtractUserKey(const Slice& internal_key) {
- 96| 13.1k| assert(internal_key.size() >= 8);
- 97| 13.1k| return Slice(internal_key.data(), internal_key.size() - 8);
- 98| 13.1k|}
+ 95| 12.9k|inline Slice ExtractUserKey(const Slice& internal_key) {
+ 96| 12.9k| assert(internal_key.size() >= 8);
+ 97| 12.9k| return Slice(internal_key.data(), internal_key.size() - 8);
+ 98| 12.9k|}
99| |
100| |// A comparator for internal keys that uses a specified comparator for
101| |// the user key portion and breaks ties by decreasing sequence number.
@@ -62498,12 +62498,12 @@
11| |
12| |namespace leveldb {
13| |
- 14| 11.1k|static Slice GetLengthPrefixedSlice(const char* data) {
- 15| 11.1k| uint32_t len;
- 16| 11.1k| const char* p = data;
- 17| 11.1k| p = GetVarint32Ptr(p, p + 5, &len); // +5: we assume "p" is not corrupted
- 18| 11.1k| return Slice(p, len);
- 19| 11.1k|}
+ 14| 10.9k|static Slice GetLengthPrefixedSlice(const char* data) {
+ 15| 10.9k| uint32_t len;
+ 16| 10.9k| const char* p = data;
+ 17| 10.9k| p = GetVarint32Ptr(p, p + 5, &len); // +5: we assume "p" is not corrupted
+ 18| 10.9k| return Slice(p, len);
+ 19| 10.9k|}
20| |
21| |MemTable::MemTable(const InternalKeyComparator& comparator)
22| 4| : comparator_(comparator), refs_(0), table_(comparator_, &arena_) {}
@@ -62513,12 +62513,12 @@
26| 5|size_t MemTable::ApproximateMemoryUsage() { return arena_.MemoryUsage(); }
27| |
28| |int MemTable::KeyComparator::operator()(const char* aptr,
- 29| 4.93k| const char* bptr) const {
+ 29| 4.83k| const char* bptr) const {
30| | // Internal keys are encoded as length-prefixed strings.
- 31| 4.93k| Slice a = GetLengthPrefixedSlice(aptr);
- 32| 4.93k| Slice b = GetLengthPrefixedSlice(bptr);
- 33| 4.93k| return comparator.Compare(a, b);
- 34| 4.93k|}
+ 31| 4.83k| Slice a = GetLengthPrefixedSlice(aptr);
+ 32| 4.83k| Slice b = GetLengthPrefixedSlice(bptr);
+ 33| 4.83k| return comparator.Compare(a, b);
+ 34| 4.83k|}
35| |
36| |// Encode a suitable internal key target for "target" and return it.
37| |// Uses *scratch as scratch space, and the returned pointer will point
@@ -62883,12 +62883,12 @@
150| |
151| | // Accessors/mutators for links. Wrapped in methods so we can
152| | // add the appropriate barriers as necessary.
- 153| 5.55k| Node* Next(int n) {
- 154| 5.55k| assert(n >= 0);
+ 153| 5.24k| Node* Next(int n) {
+ 154| 5.24k| assert(n >= 0);
155| | // Use an 'acquire load' so that we observe a fully initialized
156| | // version of the returned Node.
- 157| 5.55k| return next_[n].load(std::memory_order_acquire);
- 158| 5.55k| }
+ 157| 5.24k| return next_[n].load(std::memory_order_acquire);
+ 158| 5.24k| }
159| 587| void SetNext(int n, Node* x) {
160| 587| assert(n >= 0);
161| | // Use a 'release store' so that anybody who reads through this
@@ -62995,15 +62995,15 @@
252| 414|}
253| |
254| |template <typename Key, class Comparator>
- 255| 5.13k|bool SkipList<Key, Comparator>::KeyIsAfterNode(const Key& key, Node* n) const {
+ 255| 4.82k|bool SkipList<Key, Comparator>::KeyIsAfterNode(const Key& key, Node* n) const {
256| | // null n is considered infinite
- 257| 5.13k| return (n != nullptr) && (compare_(n->key, key) < 0);
- ^4.52k
+ 257| 4.82k| return (n != nullptr) && (compare_(n->key, key) < 0);
+ ^4.42k
------------------
- | Branch (257:10): [True: 4.52k, False: 613]
- | Branch (257:28): [True: 3.25k, False: 1.26k]
+ | Branch (257:10): [True: 4.42k, False: 403]
+ | Branch (257:28): [True: 2.94k, False: 1.47k]
------------------
- 258| 5.13k|}
+ 258| 4.82k|}
259| |
260| |template <typename Key, class Comparator>
261| |typename SkipList<Key, Comparator>::Node*
@@ -63011,18 +63011,18 @@
263| 443| Node** prev) const {
264| 443| Node* x = head_;
265| 443| int level = GetMaxHeight() - 1;
- 266| 5.13k| while (true) {
+ 266| 4.82k| while (true) {
------------------
| Branch (266:10): [Folded - Ignored]
------------------
- 267| 5.13k| Node* next = x->Next(level);
- 268| 5.13k| if (KeyIsAfterNode(key, next)) {
+ 267| 4.82k| Node* next = x->Next(level);
+ 268| 4.82k| if (KeyIsAfterNode(key, next)) {
------------------
- | Branch (268:9): [True: 3.25k, False: 1.87k]
+ | Branch (268:9): [True: 2.94k, False: 1.87k]
------------------
269| | // Keep searching in this list
- 270| 3.25k| x = next;
- 271| 3.25k| } else {
+ 270| 2.94k| x = next;
+ 271| 2.94k| } else {
272| 1.87k| if (prev != nullptr) prev[level] = x;
^1.85k
------------------
@@ -63038,7 +63038,7 @@
277| 1.43k| level--;
278| 1.43k| }
279| 1.87k| }
- 280| 5.13k| }
+ 280| 4.82k| }
281| 443|}
282| |
283| |template <typename Key, class Comparator>
@@ -68240,7 +68240,7 @@
31| 945| Slice() : data_(""), size_(0) {}
32| |
33| | // Create a slice that refers to d[0,n-1].
- 34| 27.6k| Slice(const char* d, size_t n) : data_(d), size_(n) {}
+ 34| 27.2k| Slice(const char* d, size_t n) : data_(d), size_(n) {}
35| |
36| | // Create a slice that refers to the contents of "s"
37| 2.16k| Slice(const std::string& s) : data_(s.data()), size_(s.size()) {}
@@ -68253,10 +68253,10 @@
44| | Slice& operator=(const Slice&) = default;
45| |
46| | // Return a pointer to the beginning of the referenced data
- 47| 22.1k| const char* data() const { return data_; }
+ 47| 21.9k| const char* data() const { return data_; }
48| |
49| | // Return the length (in bytes) of the referenced data
- 50| 40.5k| size_t size() const { return size_; }
+ 50| 40.1k| size_t size() const { return size_; }
51| |
52| | // Return true iff the length of the referenced data is zero
53| 433| bool empty() const { return size_ == 0; }
@@ -68322,16 +68322,16 @@
101| |
102| 26|inline bool operator!=(const Slice& x, const Slice& y) { return !(x == y); }
103| |
- 104| 6.41k|inline int Slice::compare(const Slice& b) const {
- 105| 6.41k| const size_t min_len = (size_ < b.size_) ? size_ : b.size_;
- ^227 ^6.19k
+ 104| 6.31k|inline int Slice::compare(const Slice& b) const {
+ 105| 6.31k| const size_t min_len = (size_ < b.size_) ? size_ : b.size_;
+ ^227 ^6.09k
------------------
- | Branch (105:26): [True: 227, False: 6.19k]
+ | Branch (105:26): [True: 227, False: 6.09k]
------------------
- 106| 6.41k| int r = memcmp(data_, b.data_, min_len);
- 107| 6.41k| if (r == 0) {
+ 106| 6.31k| int r = memcmp(data_, b.data_, min_len);
+ 107| 6.31k| if (r == 0) {
------------------
- | Branch (107:7): [True: 89, False: 6.32k]
+ | Branch (107:7): [True: 89, False: 6.22k]
------------------
108| 89| if (size_ < b.size_)
------------------
@@ -68344,8 +68344,8 @@
------------------
111| 0| r = +1;
112| 89| }
- 113| 6.41k| return r;
- 114| 6.41k|}
+ 113| 6.31k| return r;
+ 114| 6.31k|}
115| |
116| |} // namespace leveldb
117| |
@@ -72351,22 +72351,22 @@
106| |const char* GetVarint32PtrFallback(const char* p, const char* limit,
107| | uint32_t* value);
108| |inline const char* GetVarint32Ptr(const char* p, const char* limit,
- 109| 11.9k| uint32_t* value) {
- 110| 11.9k| if (p < limit) {
+ 109| 11.7k| uint32_t* value) {
+ 110| 11.7k| if (p < limit) {
------------------
- | Branch (110:7): [True: 11.9k, False: 4]
+ | Branch (110:7): [True: 11.7k, False: 4]
------------------
- 111| 11.9k| uint32_t result = *(reinterpret_cast<const uint8_t*>(p));
- 112| 11.9k| if ((result & 128) == 0) {
+ 111| 11.7k| uint32_t result = *(reinterpret_cast<const uint8_t*>(p));
diff --git a/Users/eugenesiegel/btc/bitcoin/build_fuzzcov/fuzz_det_cov.show.t0.a.txt b/Users/eugenesiegel/btc/bitcoin/build_fuzzcov/fuzz_det_cov.show.t0.b.txt
index 209490c47e..67c0258d57 100644
+ 112| 11.7k| if ((result & 128) == 0) {
------------------
- | Branch (112:9): [True: 11.9k, False: 0]
--- a/Users/eugenesiegel/btc/bitcoin/build_fuzzcov/fuzz_det_cov.show.t0.a.txt
+ | Branch (112:9): [True: 11.7k, False: 0]
+++ b/Users/eugenesiegel/btc/bitcoin/build_fuzzcov/fuzz_det_cov.show.t0.b.txt
------------------
- 113| 11.9k| *value = result;
- 114| 11.9k| return p + 1;
- 115| 11.9k| }
- 116| 11.9k| }
@@ -61393,15 +61393,15 @@
+ 113| 11.7k| *value = result;
+ 114| 11.7k| return p + 1;
45| 0| return "leveldb.InternalKeyComparator";
+ 115| 11.7k| }
46| 0|}
+ 116| 11.7k| }
47| |
117| 4| return GetVarint32PtrFallback(p, limit, value);
- 118| 11.9k|}
+ 118| 11.7k|}
- 48| 6.06k|int InternalKeyComparator::Compare(const Slice& akey, const Slice& bkey) const {
119| |
120| |} // namespace leveldb
121| |
+ 48| 6.01k|int InternalKeyComparator::Compare(const Slice& akey, const Slice& bkey) const {
@@ -72399,9 +72399,9 @@
24| |
25| 4| const char* Name() const override { return "leveldb.BytewiseComparator"; }
26| |
49| | // Order by:
- 27| 6.41k| int Compare(const Slice& a, const Slice& b) const override {
50| | // increasing user key (according to user-supplied comparator)
- 28| 6.41k| return a.compare(b);
51| | // decreasing sequence number
- 29| 6.41k| }
52| | // decreasing type (though sequence# should be enough to disambiguate)
+ 27| 6.31k| int Compare(const Slice& a, const Slice& b) const override {
- 53| 6.06k| int r = user_comparator_->Compare(ExtractUserKey(akey), ExtractUserKey(bkey));
+ 28| 6.31k| return a.compare(b);
- 54| 6.06k| if (r == 0) {
+ 29| 6.31k| }
+ 53| 6.01k| int r = user_comparator_->Compare(ExtractUserKey(akey), ExtractUserKey(bkey));
30| |
+ 54| 6.01k| if (r == 0) {
31| | void FindShortestSeparator(std::string* start,
------------------
32| 10| const Slice& limit) const override {
- | Branch (54:7): [True: 66, False: 5.99k]
+ | Branch (54:7): [True: 66, False: 5.94k]
------------------
55| 66| const uint64_t anum = DecodeFixed64(akey.data() + akey.size() - 8);
56| 66| const uint64_t bnum = DecodeFixed64(bkey.data() + bkey.size() - 8);
@@ -61417,8 +61417,8 @@
60| 56| r = +1;
61| 56| }
62| 66| }
- 63| 6.06k| return r;
- 64| 6.06k|}
+ 63| 6.01k| return r;
+ 64| 6.01k|}
65| |
66| |void InternalKeyComparator::FindShortestSeparator(std::string* start,
67| 10| const Slice& limit) const {
@@ -61611,10 +61611,10 @@
92| |bool ParseInternalKey(const Slice& internal_key, ParsedInternalKey* result);
93| |
94| |// Returns the user key portion of an internal key.
- 95| 13.0k|inline Slice ExtractUserKey(const Slice& internal_key) {
- 96| 13.0k| assert(internal_key.size() >= 8);
- 97| 13.0k| return Slice(internal_key.data(), internal_key.size() - 8);
- 98| 13.0k|}
+ 95| 12.9k|inline Slice ExtractUserKey(const Slice& internal_key) {
+ 96| 12.9k| assert(internal_key.size() >= 8);
+ 97| 12.9k| return Slice(internal_key.data(), internal_key.size() - 8);
+ 98| 12.9k|}
99| |
100| |// A comparator for internal keys that uses a specified comparator for
101| |// the user key portion and breaks ties by decreasing sequence number.
@@ -62489,12 +62489,12 @@
11| |
12| |namespace leveldb {
13| |
- 14| 10.9k|static Slice GetLengthPrefixedSlice(const char* data) {
- 15| 10.9k| uint32_t len;
- 16| 10.9k| const char* p = data;
- 17| 10.9k| p = GetVarint32Ptr(p, p + 5, &len); // +5: we assume "p" is not corrupted
- 18| 10.9k| return Slice(p, len);
- 19| 10.9k|}
+ 14| 10.8k|static Slice GetLengthPrefixedSlice(const char* data) {
+ 15| 10.8k| uint32_t len;
+ 16| 10.8k| const char* p = data;
+ 17| 10.8k| p = GetVarint32Ptr(p, p + 5, &len); // +5: we assume "p" is not corrupted
+ 18| 10.8k| return Slice(p, len);
+ 19| 10.8k|}
20| |
21| |MemTable::MemTable(const InternalKeyComparator& comparator)
22| 4| : comparator_(comparator), refs_(0), table_(comparator_, &arena_) {}
@@ -62504,12 +62504,12 @@
26| 5|size_t MemTable::ApproximateMemoryUsage() { return arena_.MemoryUsage(); }
27| |
28| |int MemTable::KeyComparator::operator()(const char* aptr,
- 29| 4.83k| const char* bptr) const {
+ 29| 4.78k| const char* bptr) const {
30| | // Internal keys are encoded as length-prefixed strings.
- 31| 4.83k| Slice a = GetLengthPrefixedSlice(aptr);
- 32| 4.83k| Slice b = GetLengthPrefixedSlice(bptr);
- 33| 4.83k| return comparator.Compare(a, b);
- 34| 4.83k|}
+ 31| 4.78k| Slice a = GetLengthPrefixedSlice(aptr);
+ 32| 4.78k| Slice b = GetLengthPrefixedSlice(bptr);
+ 33| 4.78k| return comparator.Compare(a, b);
+ 34| 4.78k|}
35| |
36| |// Encode a suitable internal key target for "target" and return it.
37| |// Uses *scratch as scratch space, and the returned pointer will point
@@ -62874,12 +62874,12 @@
150| |
151| | // Accessors/mutators for links. Wrapped in methods so we can
152| | // add the appropriate barriers as necessary.
- 153| 5.37k| Node* Next(int n) {
- 154| 5.37k| assert(n >= 0);
+ 153| 5.14k| Node* Next(int n) {
+ 154| 5.14k| assert(n >= 0);
155| | // Use an 'acquire load' so that we observe a fully initialized
156| | // version of the returned Node.
- 157| 5.37k| return next_[n].load(std::memory_order_acquire);
- 158| 5.37k| }
+ 157| 5.14k| return next_[n].load(std::memory_order_acquire);
+ 158| 5.14k| }
159| 587| void SetNext(int n, Node* x) {
160| 587| assert(n >= 0);
161| | // Use a 'release store' so that anybody who reads through this
@@ -62986,15 +62986,15 @@
252| 414|}
253| |
254| |template <typename Key, class Comparator>
- 255| 4.95k|bool SkipList<Key, Comparator>::KeyIsAfterNode(const Key& key, Node* n) const {
+ 255| 4.73k|bool SkipList<Key, Comparator>::KeyIsAfterNode(const Key& key, Node* n) const {
256| | // null n is considered infinite
- 257| 4.95k| return (n != nullptr) && (compare_(n->key, key) < 0);
- ^4.42k
+ 257| 4.73k| return (n != nullptr) && (compare_(n->key, key) < 0);
+ ^4.37k
------------------
- | Branch (257:10): [True: 4.42k, False: 534]
- | Branch (257:28): [True: 3.07k, False: 1.35k]
+ | Branch (257:10): [True: 4.37k, False: 356]
+ | Branch (257:28): [True: 2.84k, False: 1.52k]
------------------
- 258| 4.95k|}
+ 258| 4.73k|}
259| |
260| |template <typename Key, class Comparator>
261| |typename SkipList<Key, Comparator>::Node*
@@ -63002,18 +63002,18 @@
263| 448| Node** prev) const {
264| 448| Node* x = head_;
265| 448| int level = GetMaxHeight() - 1;
- 266| 4.95k| while (true) {
+ 266| 4.73k| while (true) {
------------------
| Branch (266:10): [Folded - Ignored]
------------------
- 267| 4.95k| Node* next = x->Next(level);
- 268| 4.95k| if (KeyIsAfterNode(key, next)) {
+ 267| 4.73k| Node* next = x->Next(level);
+ 268| 4.73k| if (KeyIsAfterNode(key, next)) {
------------------
- | Branch (268:9): [True: 3.07k, False: 1.88k]
+ | Branch (268:9): [True: 2.84k, False: 1.88k]
------------------
269| | // Keep searching in this list
- 270| 3.07k| x = next;
- 271| 3.07k| } else {
+ 270| 2.84k| x = next;
+ 271| 2.84k| } else {
272| 1.88k| if (prev != nullptr) prev[level] = x;
^1.85k
------------------
@@ -63029,7 +63029,7 @@
277| 1.43k| level--;
278| 1.43k| }
279| 1.88k| }
- 280| 4.95k| }
+ 280| 4.73k| }
281| 448|}
282| |
283| |template <typename Key, class Comparator>
@@ -68231,7 +68231,7 @@
31| 985| Slice() : data_(""), size_(0) {}
32| |
33| | // Create a slice that refers to d[0,n-1].
- 34| 27.4k| Slice(const char* d, size_t n) : data_(d), size_(n) {}
+ 34| 27.2k| Slice(const char* d, size_t n) : data_(d), size_(n) {}
35| |
36| | // Create a slice that refers to the contents of "s"
37| 2.20k| Slice(const std::string& s) : data_(s.data()), size_(s.size()) {}
@@ -68244,10 +68244,10 @@
44| | Slice& operator=(const Slice&) = default;
45| |
46| | // Return a pointer to the beginning of the referenced data
- 47| 22.1k| const char* data() const { return data_; }
+ 47| 22.0k| const char* data() const { return data_; }
48| |
49| | // Return the length (in bytes) of the referenced data
- 50| 40.4k| size_t size() const { return size_; }
+ 50| 40.2k| size_t size() const { return size_; }
51| |
52| | // Return true iff the length of the referenced data is zero
53| 433| bool empty() const { return size_ == 0; }
@@ -68313,16 +68313,16 @@
101| |
102| 31|inline bool operator!=(const Slice& x, const Slice& y) { return !(x == y); }
103| |
- 104| 6.37k|inline int Slice::compare(const Slice& b) const {
- 105| 6.37k| const size_t min_len = (size_ < b.size_) ? size_ : b.size_;
- ^250 ^6.12k
+ 104| 6.32k|inline int Slice::compare(const Slice& b) const {
+ 105| 6.32k| const size_t min_len = (size_ < b.size_) ? size_ : b.size_;
+ ^239 ^6.08k
------------------
- | Branch (105:26): [True: 250, False: 6.12k]
+ | Branch (105:26): [True: 239, False: 6.08k]
------------------
- 106| 6.37k| int r = memcmp(data_, b.data_, min_len);
- 107| 6.37k| if (r == 0) {
+ 106| 6.32k| int r = memcmp(data_, b.data_, min_len);
+ 107| 6.32k| if (r == 0) {
------------------
- | Branch (107:7): [True: 99, False: 6.27k]
+ | Branch (107:7): [True: 99, False: 6.22k]
------------------
108| 99| if (size_ < b.size_)
------------------
@@ -68335,8 +68335,8 @@
------------------
111| 0| r = +1;
112| 99| }
- 113| 6.37k| return r;
- 114| 6.37k|}
+ 113| 6.32k| return r;
+ 114| 6.32k|}
115| |
116| |} // namespace leveldb
117| |
@@ -71306,15 +71306,15 @@
45| 418| char* result;
46| 418| if (needed <= alloc_bytes_remaining_) {
------------------
- | Branch (46:7): [True: 411, False: 7]
+ | Branch (46:7): [True: 412, False: 6]
------------------
- 47| 411| result = alloc_ptr_ + slop;
- 48| 411| alloc_ptr_ += needed;
- 49| 411| alloc_bytes_remaining_ -= needed;
- 50| 411| } else {
+ 47| 412| result = alloc_ptr_ + slop;
+ 48| 412| alloc_ptr_ += needed;
+ 49| 412| alloc_bytes_remaining_ -= needed;
+ 50| 412| } else {
51| | // AllocateFallback always returned aligned memory
- 52| 7| result = AllocateFallback(bytes);
- 53| 7| }
+ 52| 6| result = AllocateFallback(bytes);
+ 53| 6| }
54| 418| assert((reinterpret_cast<uintptr_t>(result) & (align - 1)) == 0);
55| 418| return result;
56| 418|}
@@ -71391,14 +71391,14 @@
59| 414| assert(bytes > 0);
60| 414| if (bytes <= alloc_bytes_remaining_) {
------------------
- | Branch (60:7): [True: 405, False: 9]
+ | Branch (60:7): [True: 404, False: 10]
------------------
- 61| 405| char* result = alloc_ptr_;
- 62| 405| alloc_ptr_ += bytes;
- 63| 405| alloc_bytes_remaining_ -= bytes;
- 64| 405| return result;
- 65| 405| }
- 66| 9| return AllocateFallback(bytes);
+ 61| 404| char* result = alloc_ptr_;
+ 62| 404| alloc_ptr_ += bytes;
+ 63| 404| alloc_bytes_remaining_ -= bytes;
+ 64| 404| return result;
+ 65| 404| }
+ 66| 10| return AllocateFallback(bytes);
67| 414|}
68| |
69| |} // namespace leveldb
@@ -72342,22 +72342,22 @@
106| |const char* GetVarint32PtrFallback(const char* p, const char* limit,
107| | uint32_t* value);
108| |inline const char* GetVarint32Ptr(const char* p, const char* limit,
- 109| 11.7k| uint32_t* value) {
- 110| 11.7k| if (p < limit) {
+ 109| 11.6k| uint32_t* value) {
+ 110| 11.6k| if (p < limit) {
------------------
- | Branch (110:7): [True: 11.7k, False: 4]
+ | Branch (110:7): [True: 11.6k, False: 4]
------------------
- 111| 11.7k| uint32_t result = *(reinterpret_cast<const uint8_t*>(p));
- 112| 11.7k| if ((result & 128) == 0) {
+ 111| 11.6k| uint32_t result = *(reinterpret_cast<const uint8_t*>(p));
+ 112| 11.6k| if ((result & 128) == 0) {
------------------
- | Branch (112:9): [True: 11.7k, False: 0]
+ | Branch (112:9): [True: 11.6k, False: 0]
------------------
- 113| 11.7k| *value = result;
- 114| 11.7k| return p + 1;
- 115| 11.7k| }
- 116| 11.7k| }
+ 113| 11.6k| *value = result;
+ 114| 11.6k| return p + 1;
+ 115| 11.6k| }
+ 116| 11.6k| }
117| 4| return GetVarint32PtrFallback(p, limit, value);
- 118| 11.7k|}
+ 118| 11.6k|}
119| |
120| |} // namespace leveldb
121| |
@@ -72390,9 +72390,9 @@
24| |
25| 4| const char* Name() const override { return "leveldb.BytewiseComparator"; }
26| |
- 27| 6.36k| int Compare(const Slice& a, const Slice& b) const override {
- 28| 6.36k| return a.compare(b);
- 29| 6.36k| }
+ 27| 6.32k| int Compare(const Slice& a, const Slice& b) const override {
+ 28| 6.32k| return a.compare(b);
+ 29| 6.32k| }
30| |
31| | void FindShortestSeparator(std::string* start,
32| 10| const Slice& limit) const override {
⚠️
The coverage was not deterministic between runs.
The fuzz target input was /Users/eugenesiegel/btc/bitcoin/cmpctblock/cmpctblock/00095f04def2d4e203a2844032ee4bb88e33681d
</details>
Resolving as this commit has been removed.
Some possible discussion about ed813c48f826d083becf93c741b483774c850c86 for review club.
Latest push ed813c48f826d083becf93c741b483774c850c86 -> f2ce362:
GetStrongRandBytes per commentcreate_tx to choose a mempool UTXO only sometimes instead of most of the timecreate_block to generate more than 2 non-coinbase transactions per feedback from review club142 | @@ -143,13 +143,6 @@ CBlock ConsumeBlock(FuzzedDataProvider& fuzzed_data_provider, const uint256& pre 143 | return block; 144 | } 145 | 146 | -void FinalizeHeader(CBlockHeader& header, const ChainstateManager& chainman)
do we still need #include <pow.h> here after the move?
Nope, thanks.
135 | @@ -136,6 +136,13 @@ static inline bool copy_file(const path& from, const path& to, copy_options opti 136 | return std::filesystem::copy_file(from, to, options); 137 | } 138 | 139 | +// Disallow implicit std::string conversion for copy to avoid locale-dependent 140 | +// encoding on Windows. This is currently only used in fuzzing. 141 | +static inline void copy(const path& from, const path& to, copy_options options)
8515594b3b716d05a89b9fef36dfe7e8032d19c7
Do we still need this after #33550?
This is currently only used in fuzzing.
Are we adding dead code in this commit? It's hard to judge if a solution is accurate if we see it before the problem is presented - can we make the question obvious before we provide the answer?
And more concretely: my IDE is confused about the usage of this even at the tip - would it be possible to invalidate the conversions you don't want instead, maybe something like:
static void copy(const std::string&, const path&, copy_options) = delete;
static void copy(const path&, const std::string&, copy_options) = delete;
static void copy(const std::string&, const std::string&, copy_options) = delete;
nit: inline is implicit
Do we still need this after #33550?
I think it's still needed, I added it to get rid of linter warnings if std::filesystem::copy is instead called directly from the test code.
Are we adding dead code in this commit? It's hard to judge if a solution is accurate if we see it before the problem is presented - can we make the question obvious before we provide the answer?
Fair point, I'll rework the commits so it's a bit easier to tell where things are used.
would it be possible to invalidate the conversions you don't want instead, maybe something like
Yes, that also works.
would it be possible to invalidate the conversions you don't want instead, maybe something like
Currently implementing this and wondering if this is necessary; can you share what your IDE is confused about? The constructor of path that takes a std::string is deleted.
I don't have Windows, it's why I asked if instead of adding a new method we can just delete the conversions, somewhat similarly to the mentioned https://github.com/bitcoin/bitcoin/pull/33550/files#diff-69423eb01bf14b3bd0d930c0b3e1fd6f4f061ffefacab579053eaa734fc22f38R65 (since the error seemed superficially similar to me).
Deleting the conversions instead of adding a new method does not work because overload resolution considers these functions even though they are deleted and then the compiler errors that copy is ambiguous:
/Users/eugenesiegel/btc/bitcoin-clone/src/test/util/setup_common.cpp:210:9: error: call to 'copy' is ambiguous
210 | fs::copy(cached_dir, m_path_root, fs::copy_options::recursive);
| ^~~~~~~~
/Users/eugenesiegel/btc/bitcoin-clone/src/util/fs.h:142:13: note: candidate function has been explicitly deleted
142 | static void copy(const path&, const std::string&, copy_options) = delete;
| ^
/Users/eugenesiegel/btc/bitcoin-clone/src/util/fs.h:141:13: note: candidate function has been explicitly deleted
141 | static void copy(const std::string&, const path&, copy_options) = delete;
nit: inline is implicit
clang warns about unused functions if I remove the inline
Since this helper appears to be used only by fuzz test code, is there any downside to keeping it in a test only location(i.e under src/test/util) instead of src/util/fs.h? Maybe that could help keep util’s interface focused on production use, but happy to defer if there’s a reason it needs to live here specifically.
This needs to be here because of the linter which checks that std::filesystem isn't used outside of this fs namespace.
Hmm, I guess the linter could be modified. I'll need to think a bit more on this. Ideally this wouldn't live in production code if it's not used in production.
286 | @@ -287,6 +287,8 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) 287 | m_node.scheduler->scheduleFromNow([&promise] { promise.set_value(); }, 0ms); 288 | promise.get_future().wait(); 289 | } 290 | + } else if (opts.setup_validation_interface_no_scheduler) { 291 | + m_node.validation_signals = std::make_unique<ValidationSignals>(std::make_unique<util::ImmediateTaskRunner>());
I have the same problem in 71719d172e3a435dd983293fc9da6a08974f8b01 - we're introducing dead code, I don't have a way to judge if this is indeed the correct solution, since up to this point there wasn't any pain that this alleviates.
Looking at the commit I see that setup_validation_interface_no_scheduler is always false in this commit therefore we can delete the code - and I have to check the next commits and come back here to reevaluate that.
Can we add a simpler fuzz test which needs this in the same commit that introduces it - and extend the test with other functionality separately.
Can we add a simpler fuzz test which needs this in the same commit that introduces it - and extend the test with other functionality separately.
Yes, I'll do some variation of this. I think it might be cleaner to put it as one of the last commits.
Done in 0190ac6622ff017aeead385289918d388a85218a
417 | + random_node.fPauseSend = false; 418 | + 419 | + try { 420 | + more_work = connman.ProcessMessagesOnce(random_node); 421 | + } catch (const std::ios_base::failure&) { 422 | + }
// Expected for truncated/invalid fuzzed messages; swallow to continue exercising code paths.
// TODO validate that the error is something expected, otherwise anything can happen here
}
This is copied from the process_message(s) harness. Unlike those, this fuzz test should always construct correctly serialized messages (even if the messages themselves are deemed "protocol invalid"), so I think the catch case can just be assert(false).
Isn't it cleaner to just drop the try/catch entirely in that case?
Ah yes, that is cleaner.
Done in 62f2b9e4601ba34e504171f87577f315af51f3c9
194 | + in.nSequence = sequence; 195 | + in.scriptSig = script_sig; 196 | + in.scriptWitness.stack = script_wit_stack; 197 | + tx_mut.vin.push_back(in); 198 | + 199 | + const CAmount amount_in = GetAmount(outpoint);
nit: seems excessive to add a helper for a single usage here:
const CAmount amount_in = Assert(amount_view.GetCoin(outpoint))->out.nValue;
Will do, I think this was used in multiple places in an earlier version of this code.
Done in 04d1dc78599362623bdd4b55c7be0c7b0f9f2174
5a0f0c3fac47053f7d0245ac0b005d4b26cacd48 nit:
- Since the caller is BasicTestingSetup() is not aware of the random path element
+ Since the caller of BasicTestingSetup() is not aware of the random path element
Resolving since commit message changed so no longer relevant.
327 | + } 328 | + 329 | + CBlockHeaderAndShortTxIDs base_cmpctblock = cmpctblock; 330 | + net_msg = NetMsg::Make(NetMsgType::CMPCTBLOCK, base_cmpctblock); 331 | + }, 332 | + [&]() {
This commit is huge, it's hard to review it meaningfully. Can we separate some of these cases to independent commits that can provide more context?
I'll incrementally add the cases so it's a bit easier to digest.
Done in the latest push to 039c3aab3b79e61e8ffe193bffee36b826ca2984
101 | @@ -101,6 +102,7 @@ void SetupCommonTestArgs(ArgsManager& argsman) 102 | { 103 | argsman.AddArg("-testdatadir", strprintf("Custom data directory (default: %s<random_string>)", fs::PathToString(fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / "")), 104 | ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); 105 | + argsman.AddArg("-fuzzcopydatadir", "Copies the passed data directory to a temporary directory to use during a single fuzz iteration", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
5a0f0c3fac47053f7d0245ac0b005d4b26cacd48 commit is also quite big, can we split by functionality instead? I would prefer the commits telling a story, to each make be self-contained as much as possible, to make sense on their own as far as it's reasonable. Can we split this one by features, e.g. adding rand_path to be able to specify strong random, maybe fuzzcopydatadir param and usage next, EnableFuzzDeterminism last - or something similar
Done in f07ef4149b89d2c1d71af2cafd1bf29f6661b35a which introduces rand_path using strong randomness, and -fuzzcopydatadir and uses both in the same commit. The EnableFuzzDeterminism change is done in 039c3aab3b79e61e8ffe193bffee36b826ca2984.
179 | @@ -180,6 +180,11 @@ bool CBlockIndexHeightOnlyComparator::operator()(const CBlockIndex* pa, const CB 180 | return pa->nHeight < pb->nHeight; 181 | } 182 | 183 | +bool CBlockIndexBlockHashComparator::operator()(const CBlockIndex* pa, const CBlockIndex* pb) const 184 | +{ 185 | + return UintToArith256(pa->GetBlockHash()) < UintToArith256(pb->GetBlockHash());
What was your reason for instantiating new objects just for comparison, is it just to make sure you're sorting backwards?
return pa->GetBlockHash().Compare(pb->GetBlockHash());
or
return pa->GetBlockHash() < pb->GetBlockHash();
I don't think that's necessary, the above should likely be safe as well.
But since it's a lot faster to compare pointers for equality (which seems like an important feature, it's why most of these are a set in the first place I assume) and since that's stable across runs as well, we can likely optimize further to:
return pa != pb && pa->GetBlockHash() < pb->GetBlockHash();
And if we inline this to the header it should be just as fast as pointers.
IIRC I tried comparing the uint256 block hashes similar to your second suggestion, and while it fixed the leveldb non-determinism, it introduced its own non-determinism.
The suggested comparator is a pure function, I don't see how it could be non-deterministic - unless it's broken, in which case we should definitely dig deeper. Can you try it again an report what you see?
My bad, I must have tested a slightly different comparator. The suggested ones are deterministic.
Resolving as the commit has been removed.
94 | @@ -95,6 +95,10 @@ struct CBlockIndexHeightOnlyComparator { 95 | bool operator()(const CBlockIndex* pa, const CBlockIndex* pb) const; 96 | }; 97 | 98 | +struct CBlockIndexBlockHashComparator { 99 | + bool operator()(const CBlockIndex* pa, const CBlockIndex* pb) const;
Resolving as the commit has been removed.
Concept and approach ACK, thanks for tackling this.
I like how you've extracted the first few commits that I could quickly get out of the way to continue to the more difficult ones - which are a bit too difficult though, it would help me personally if we could split it into smaller functional chunks, so that the reviewers are guided through the change. The commit messages could give extra context where needed. I don't think we should split by functions, but rather functionalities so that each commit could theoretically be merged if needed - we shouldn't depend on a future commit to understand a previous one.
I have provided some hints and some further context for the LevelDB related changes. I think we should do the set stabilization in a dedicated PR, but you can also adjust it here so that it likely performs similarly to the original.
Latest push f2ce3626a6a40b8688d711da1924db156dc2f02c -> 039c3aa breaks up the harness into more reviewable chunks. I forgot to add @stringintech as co-author on a commit during rebase, will address the next time I push up.
160 | + COutPoint outpoint; 161 | + unsigned long mempool_size = setup->m_node.mempool->size(); 162 | + if (fuzzed_data_provider.ConsumeBool() && mempool_size != 0) { 163 | + size_t random_idx = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, mempool_size - 1); 164 | + LOCK(setup->m_node.mempool->cs); 165 | + outpoint = COutPoint(setup->m_node.mempool->txns_randomized[random_idx].second->GetSharedTx()->GetHash(), 0);
Seems like txns_randomized reorders between runs, could this cause the same fuzz seed to pick different transactions? If so, would it make sense to snapshot the vector first for stable indexing?
I don't think txns_randomized gets reordered between runs, its randomness depends on the order that inserts/deletes happen in the mempool which is deterministic. I've also checked that this harness is fully deterministic with the deterministic-fuzz-coverage script.
Actually, the harness is not fully deterministic because I dropped a commit that will be done in a follow-up which causes some LevelDB non-determinism. But it is deterministic as far as the usage of txns_randomized goes.
Concept ACK. fuzzing direction look reasonable.
I plan to update this branch to get rid of the directory copying to instead reset the state similar to how the process_message(s) harnesses do it. I still need to hunt down some stability issues with the new approach in AFL++, but then I can push up. It gives a noticeable speedup since each iteration isn't creating a TestingSetup and also lends nicely to eventually being able to mock the underlying file store (which itself gives another ~2-3x speedup).
Latest push to 3c515102f4e83b4117f52d7d0fd41aac59740b16 gets rid of all the fs and TestingSetup changes. Now if the mempool changed at all (if sequence numbers changed) or the block index changed, it resets the rng (calls MakeRandDeterministicDANGEROUS, its extern which I think is acceptable), resets the mempool, the chainman, and mines blocks again to make it deterministic. This is faster than the old version (which is surprising to me) and the changes are nicely contained to the fuzz code. Will post coverage later (though nothing should have changed).
Also cc @instagibbs
116 | + } 117 | + 118 | LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 1000) 119 | { 120 | + CSerializedNetMsg net_msg; 121 | + bool set_net_msg = true;
09f547384aff90359b8a9e9a6f469b14683ffbab
sent_net_msg?
Changed in 6b1031020574bba672233296429791cc7c881a06
123 | CallOneOf( 124 | fuzzed_data_provider, 125 | + [&]() { 126 | + // Send a sendcmpct message, optionally setting hb mode. 127 | + bool hb = fuzzed_data_provider.ConsumeBool(); 128 | + net_msg = NetMsg::Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/hb, /*version=*/CMPCTBLOCKS_VERSION);
09f547384aff90359b8a9e9a6f469b14683ffbab
not a ton of value, but we should also not crash/etc if the wrong version is sent?
I can change this to send the correct one most of the time.
Changed in 6b1031020574bba672233296429791cc7c881a06. Added an assert that if the peer is still connected, the node updated m_bip152_highbandwidth_from for the peer to hb.
140 | + 141 | + const uint64_t initial_sequence{WITH_LOCK(mempool.cs, return mempool.GetSequence())}; 142 | + 143 | + const CCoinsViewMemPool amount_view{WITH_LOCK(::cs_main, return &chainman.ActiveChainstate().CoinsTip()), mempool}; 144 | + 145 | + auto create_tx = [&]() -> CTransactionRef {
49a3bdce613bb97d016c4120fa11655034262a15
Any justification why some fields are chosen by fuzzer and some not?
Currently I think only the version is hard-coded. An earlier version of this PR added a lot more invalidity, but I noticed that coverage suffered because the fuzzer would get lost trying to hit different invalidity checks that had little to do with compact blocks.
148 | + tx_mut.nLockTime = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<uint32_t>(); 149 | + 150 | + // If the mempool is non-empty, choose a mempool outpoint. Otherwise, choose a coinbase. 151 | + COutPoint outpoint; 152 | + unsigned long mempool_size = mempool.size(); 153 | + if (fuzzed_data_provider.ConsumeBool() && mempool_size != 0) {
49a3bdce613bb97d016c4120fa11655034262a15
noting asymmetry: we're intentionally not double-spending coinbase outpoints but we can double spend mempool outpoints
Yes good point, I can change this. I wanted to limit invalidity here, but there's no good reason that we can't double-spend the coinbase outpoints.
Changed in c93df0e9d93dde68554721463a96c752457fef58. The following commit a165c3f3ff63faf3a2420444f1dd6a3032154d89 also adds a case to spend an outpoint from a created block.
159 | + std::advance(pop, fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, mature_coinbase.size() - 1)); 160 | + outpoint = *pop; 161 | + mature_coinbase.erase(pop); 162 | + } else { 163 | + // We have no utxos available to make a transaction. 164 | + return nullptr;
49a3bdce613bb97d016c4120fa11655034262a15
This happens when all coinbase outpoints are gone, and no outpoints are in the mempool? I think this only happens if all txns failed to enter the mempool?
Yup, I hit a null-pointer-dereference without this.
Removed now that coinbase outpoints can be double-spent in c93df0e9d93dde68554721463a96c752457fef58.
290 | + // Choose an existing block and send a HEADERS message for it. 291 | + size_t index = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, num_blocks - 1); 292 | + CBlock block = *info[index].block; 293 | + block.vtx.clear(); // No tx in HEADERS. 294 | + std::vector<CBlock> headers; 295 | + headers.emplace_back(block);
579f663f1897c9d584b04fbd0df8d09bb06f9440
any value in sending more than one once in a while?
I don't think so since the next loop iteration could hit this case again, but it doesn't hurt to add.
Did not do this because I would need to introduce a data structure that let me query info for the previous block since the headers need to connect.
334 | + 335 | + // Remove from shorttxids since we've prefilled. Subtract however many txs have been prefilled. 336 | + cmpctblock.EraseShortTxIDs(i - 1 - num_erased); 337 | + ++num_erased; 338 | + } 339 | +
558a97487f85be731dc663e264cad49b4e24183b
Cheap sanity check could be:
assert(cmpctblock.PrefilledTxCount() + cmpctblock.ShortTxIdCount() == num_txs);
Changed in f8438ff2ce38face5fb9994c0d9505e2f3cd726b.
The count assert is a good cheap check. Just wondering if it could be complemented with another one. I think the totals would still add up correctly even if AddPrefilledTx silently does nothing or EraseShortTxIDs is never called, so those cases would pass undetected. Could maybe also track selected_for_prefill indices during the loop and assert they appear in prefilledtxn afterwards?
How would 1) AddPrefilledTx do nothing, and 2) how would EraseShortTxIDs never get called? Is there a bug in the current code that you can see? I don't think the existing assert is even necessary, I've looked at the coverage.
@frankomosh The way your comment is worded and Eugene's response make me think your comment was LLM generated. I appreciate that you try to contribute constructively, but just posting LLM output is not useful during review and wastes time.
I had ran mutation-core against the prefill logic and found that mutants removing the AddPrefilledTx call or skipping EraseShortTxIDs both survived the existing assert, meaning the count check doesn't distinguish correct encoding from those broken states. That's the scenario I had in mind, not a bug in the current code. If the assert isn't needed at all as you say, that's also useful to know. Was simply giving a suggestion and not objecting to anything.
Maybe should have mentioned that first hand
Could you please provide the mutant "diff" @frankomosh? When I remove the AddPrefilledTx call, I get a crash on the assertion and I want to know if I'm overlooking something.
Sure. Here are the mutant diffs:
Removes the AddPrefilledTx call:
- cmpctblock.AddPrefilledTx(std::move(prefilledtx));
+
Skips EraseShortTxIDs:
- cmpctblock.EraseShortTxIDs(i - num_erased);
+
Wait... I might have made a mistake because I was using build_fuzz_nosan, (which I think compiles out asserts?)
With --preset=libfuzzer-nosan I hit the assertion with both mutants
354 | + BlockTransactions block_txn; 355 | + block_txn.blockhash = block_info.hash; 356 | + std::shared_ptr<CBlock> cblock = block_info.block; 357 | + 358 | + size_t num_txs = cblock->vtx.size(); 359 | + if (num_txs > 1) {
3c515102f4e83b4117f52d7d0fd41aac59740b16
Sending a coinbase is fair play for the target, no? Why gate?
Gated because the coinbase is prefilled when sending the compact block. I can change it so that the coinbase is not always prefilled and can be sent in blocktxn also.
This case is not ideal because it's just guessing what the peer needs so it will only sometimes fill the block. What we do in fuzzamoto (which we could do here) is read the getblocktxn and then insert what was missing (and randomly give garbage some of the time). I think this could be done in ReceiveMsgFrom after receiving the node's message and filtering for getblocktxn and we could probably do this to assert that the node sends well-formed responses. What do you think?
Change done in f8438ff2ce38face5fb9994c0d9505e2f3cd726b and the following commit 14e153aab5731e151d0a408432366db84d922b4a. The logic change is a little hairy because of how the encoding for the first prefill works if it's not the coinbase. I checked it multiple times, so hopefully there's no mistake.
strong concept ACK on the target, I do wonder what issues we would like to shoot for.
For instance, do we want to cover things like protocol-invalid messages, malformed compact blocks, invalid PoW, etc.
Seems clearly better than what we have now at least.
For instance, do we want to cover things like protocol-invalid messages, malformed compact blocks, invalid PoW, etc.
I was having some trouble figuring out what to assert on. I added protocol-invalid messages and the fuzzer tended to get lost and the coverage was pretty bad. That just means it takes more CPUs / time to hit the cases we're interested in, which is not a huge deal but something to consider. It is possible to query the node (like the mempool, blockindex) or check the messages it sends out to assert, I just don't know what the asserts could be.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/23506646746/job/68419351392</sub>
<sub>LLM reason (✨ experimental): CI failed due to a build error compiling the fuzz test source (src/test/fuzz/.../cmpctblock.cpp.o).</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>
Push to 14e153aab5731e151d0a408432366db84d922b4a adds, among other things, an assertion that we may only have 3 connected HB peers and that they must be of a certain connection type (note the special-case for addrfetch). There is probably more room for assertions using the GetNodeStats call, open to suggestions. Fuzzing it again since the old corpus is invalidated.
144 | + auto create_tx = [&]() -> CTransactionRef { 145 | + CMutableTransaction tx_mut; 146 | + tx_mut.version = fuzzed_data_provider.ConsumeBool() ? CTransaction::CURRENT_VERSION : TRUC_VERSION; 147 | + tx_mut.nLockTime = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<uint32_t>(); 148 | + 149 | + // If the mempool is non-empty, choose a mempool outpoint. Otherwise, choose a coinbase.
c93df0e9d93dde68554721463a96c752457fef58
nit: comment makes it sound unconditional
Changed to "If the mempool is non-empty, sometimes choose a mempool outpoint. Otherwise, choose a coinbase." in 651622432d2d43d6503412094ad8c7bf16487b62.
479 | + assert(hb_peer->IsInboundConn() || hb_peer->IsOutboundOrBlockRelayConn() || hb_peer->IsManualConn() || hb_peer->IsAddrFetchConn()); 480 | + } 481 | + } 482 | + assert(num_hb <= 3); 483 | + 484 | + if (sent_sendcmpct && random_node.fSuccessfullyConnected && !random_node.fDisconnect) {
In net_processing I'm not seeing an fSuccessfullyConnected check for the SENDCMPCT message. There is a check further down, after SENDCMPCT. Do we need to gate on it in this harness? Dropping it would check the assertion for every peer regardless of a completed handshake, which I think is the correct behavior unless I'm missing something.
I don't think it's needed, will run without and see if it crashes.
Changed in 8c9a3fd0e87f5855e398a46b482ade4644dc3e92. It didn't crash after a few hours -- I think that's enough time to test as simply removing the (needed) fDisconnect check crashes within a minute.
169 | @@ -155,6 +170,14 @@ FUZZ_TARGET(cmpctblock, .init = initialize_cmpctblock) 170 | CTransactionRef tx = WITH_LOCK(mempool.cs, return mempool.txns_randomized[random_idx].second->GetSharedTx();); 171 | outpoint = COutPoint(tx->GetHash(), 0); 172 | amount_in = tx->vout[0].nValue; 173 | + } else if (fuzzed_data_provider.ConsumeBool() && info.size() != 0) { 174 | + // These blocks (and txs) may be invalid or not in the main chain.
a165c3f3ff63faf3a2420444f1dd6a3032154d89
nit: or the output already spent
Changed to "These blocks (and txs) may be invalid, use a spent output, or not be in the main chain." in 3c58efe2acf0e7fbf1daaadd8bff2e0cfd3761e1.
202 | @@ -180,6 +203,78 @@ FUZZ_TARGET(cmpctblock, .init = initialize_cmpctblock) 203 | return tx; 204 | }; 205 | 206 | + auto create_block = [&]() { 207 | + uint256 prev; 208 | + uint32_t height; 209 | + 210 | + if (fuzzed_data_provider.ConsumeBool() || info.size() == 0) {
a165c3f3ff63faf3a2420444f1dd6a3032154d89
swap the checks to reduce unnecessary reading of fuzz input?
Changed in 3c58efe2acf0e7fbf1daaadd8bff2e0cfd3761e1. Also changed order of checks in create_tx in both 3c58efe2acf0e7fbf1daaadd8bff2e0cfd3761e1 and 651622432d2d43d6503412094ad8c7bf16487b62.
Confirmed that blockchain is moving forward even with non-trivial compact blocks (not just cb)
Can't seem to generate a mempool tx in my short run that actually enters the mempool. Might want to check coverage in depth to validate the state you think we should be hitting.
ok found mempool entries, just took a few more minutes to hit
Ran with AFL++ for a couple days and stability ended up at 98.53% (nice) and exec/s was ~100, which is reasonable for a harness that does as much as this one does. Maybe lowering the LIMITED_WHILE max would help with that, if we care.
Left a question.
105 | + } 106 | +}; 107 | + 108 | +void ResetChainmanAndMempool(TestingSetup& setup) 109 | +{ 110 | + SetMockTime(Params().GenesisBlock().Time());
nit: could we use NodeClockContext instead?..not sure if that pattern works well here, but I just noticed that most recent tests are moving away from SetMockTime
I will copy the pattern used in process_message(s), which keeps SetMockTime usage in the reset function and uses the context at the top of the loop. It should be fine to also use another context here, but I'd rather keep the style the same.
Changed in 6cd480f62f69f170e4eb24ea0ccca97e996e5333.
Adds a fuzz harness cmpctblock to test BIP152 compact block relay.
Currently just sets mock time in a loop.
The cmpctblock harness now adds peers and processes the sendcmpct message.
If the mempool is modified at all (determined by a change in the sequence
counter), reset the rng, mempool, and the chainman for the next iteration.
The blocks may include mempool and non-mempool transactions. If the
block index was added to, reset the rng, mempool, and chainman. Also
move FinalizeHeader from p2p_headers_presync.cpp to util.h so that
the mining function can use it to create valid headers.
Using an existing block or a newly created block, send a compact
block message to the target node. Some of the time, the harness
will prefill multiple transactions from the block. Adds a
FuzzedCBlockHeaderAndShortTxIDs class that's used to populate the
protected prefilledtxn and shorttxids fields.
Sometimes, blindly take an existing block and choose random
transactions to request in a blocktxn message.
I pushed twice because I forgot to address one comment. Can see the diff here. Coverage from last month is here -- the corpus was invalidated so I will run again.
Edit:
Maybe lowering the LIMITED_WHILE max would help with that
This might help for large inputs, I can compare 1000 to 100 and see if it makes a difference.
ACK c8d688f41ccb218f301adde4120f2094d13b739a
Reasonable to have this land and can be improved more in the future
This might help for large inputs, I can compare 1000 to 100 and see if it makes a difference.
Did you check this?
Did you check this?
Currently running, will report back.
I ran for 3 hours with 1000 vs 100 in the loop condition and the numbers were basically the same at around 100 execs/s. Not a perfect benchmark since I didn't run many trials, but I don't think the bottleneck is in the loop condition. I did profile a while back and I did notice a speedup when I disabled CheckBlockIndex.