PoC: fuzz chainstate and block managers

darosior commented at 10:05 am on December 30, 2023: member

We don’t have a fuzzing harness for our main consensus engine [0]. This PR introduces two new targets which respectively fuzz the BlockManager and ChainstateManager (process headers, blocks, reorgs and assert some invariants in doing so).

There is two main obstacles to achieving this: PoW and io. The blocks and chainstate databases can be stored in memory but blocks still need a valid proof of work and to be stored on disk. Niklas solved the first issue in #28043 by simply introducing a global which makes it possible to mock the PoW check (his commit is cherry-picked here). After considering other approaches, i also used globals to mock disk io.

I’m interested with this PR in getting feedback on the concept and the approach, but also in suggestions of more invariants to be asserting in the chainstate fuzz target.

Regarding other approaches i tried the most potentially promising was to leverage ld’s --wrap option to mock the syscalls without having to modify non-test code. But i didn’t try too hard to make it work: better to have a demo of what can be achieved first with a more trivial way of mocking filesystem calls. If there is interest in these fuzz targets, i can give this approach another look.

Regarding efficiency, the chainstate fuzz target is quite slow at the moment but i’ve at least 2x its performance by rebasing on #28960 and making CheckBlockIndex callable externally even if !ShouldCheckBlockIndex(). Suggestions for performance improvements welcome too.

[0] Well there is utxo_total_supply but it’s very specialized toward exercising a specific past bug.

DrahtBot commented at 10:05 am on December 30, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29158.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	TheCharlatan, jamesob

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30664 (build: Remove Autotools-based build system by hebasto)
#30661 (fuzz: Test headers pre-sync through p2p by marcofleon)

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 CI failed on Dec 30, 2023

in src/test/fuzz/chainstate.cpp:85 in ea36af80be outdated

80+            std::vector<unsigned char> buf(node::MAX_BLOCKFILE_SIZE);
81+            const auto [it2, _]{g_files.insert({file_path, std::move(buf)})};
82+            return std::make_pair(it2->second.data(), it2->second.size());
83+        }()};
84+        if (!data) return (FILE*)nullptr;
85+        return fmemopen(data, size, mode);

dergoegge commented at 3:03 pm on January 2, 2024:

Have you checked if using fmemopen is better/faster than using a ram disk?

darosior commented at 8:33 pm on January 2, 2024:

I haven’t. Can we use a ramdisk at oss-fuzz?

dergoegge commented at 10:35 am on January 3, 2024:

Afaict oss-fuzz already uses a ram disk for its environments: https://github.com/google/clusterfuzz/blob/c461a961d8fb2afe47fb4af5eee3d1434a324a40/docker/base/setup_clusterfuzz.sh#L38 (i.e. /tmp is mounted in ram).

dergoegge commented at 10:12 pm on January 3, 2024:

https://joshua.hu/fuzzing-with-memfd-createfd-fmemopen-syscall-function

The author of this post found that a ram disk is slower🚀

I/O syscalls are avoided with fmempopen and everything simply happens in userland.

darosior commented at 8:21 pm on January 4, 2024:

Interesting. I was aware of open_memstream but not memfd_create. It’s slower but could actually be helpful to get rid of the 128 MiB allocs and make it possible to reindex.

jamesob commented at 4:19 pm on January 10, 2024:

https://github.com/bitcoin/bitcoin/pull/29158/commits/ea36af80beeeee0b9de793e52887ba3e164b803c

fmemopen is Linux-specific; here’s a fixup commit that ifdefs it out and fixes some of the CI errors if you want it: https://github.com/jamesob/bitcoin/commit/26b9c9d48e3ec16e69d550266c7f27f4db9cb9f8

darosior commented at 3:27 pm on July 31, 2024:

Not mocking the fs anymore.

darosior commented at 7:50 am on August 1, 2024:

As discussed below i tried various ways of mocking the filesystem and ended up not mocking it at all: #29158 (comment).

jamesob commented at 3:46 pm on January 2, 2024: contributor

Cool, this is a great thing to investigate. I’ll be giving the approach a look this week.

dergoegge commented at 5:22 pm on January 2, 2024: member

Thanks for working on this!

One alternative that I have considered before (for chainstate fuzzing) is to abstract and further modularize BlockManager, which would allow us to have an InMemoryBlockManager for tests (especially useful for fuzzing but also nice in unit tests).

This would require a bunch of work:

Breaking up the friendship between BlockManager, Chainstate & ChainstateManager
Abstracting BlockManager’s interface away from being file based
Hiding access to BlockManager’s internal fields
Probably more…

This approach would avoid filesystem syscalls entirely, as well as the large block file allocations.

The coinbase maturity also seems relevant because you can’t spend any coins in the test until you’ve mined 100 blocks. Mining 100 blocks every fuzz iteration ends up being pretty slow. Maybe we can use assumeutxo to avoid that? (or snapshot fuzzing)

brunoerg commented at 5:21 pm on January 3, 2024: contributor

Nice one!

TheCharlatan commented at 5:54 pm on January 3, 2024: contributor

Concept ACK

in src/test/fuzz/chainstate.cpp:597 in ea36af80be outdated

546+                        if (extend_tip) {
547+                            return fuzzed_data_provider.ConsumeBool() ? chainman.ActiveTip() : chainman.m_best_header;
548+                        }
549+                        return &PickValue(fuzzed_data_provider, chainman.m_blockman.m_block_index).second;
550+                    }()};
551+                    blocks_in_flight.push_back(CreateValidBlock(fuzzed_data_provider, chainparams.GetConsensus(), prev_block, coins));

jamesob commented at 9:04 pm on January 9, 2024:

When generating a valid block (here and below) do we want to do some kind of assertion that the block was actually considered valid? I could see the test code silently generating only invalid blocks and (afaik) there wouldn’t be an indication of it here.

darosior commented at 3:27 pm on July 31, 2024:

Done.

in src/test/fuzz/chainstate.cpp:513 in ea36af80be outdated

455+FUZZ_TARGET(chainstate, .init = init_chainstate)
456+{
457+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
458+    const auto& chainparams{Params()};
459+    const fs::path datadir{""};
460+    std::unordered_map<COutPoint, CTxOut, SaltedOutpointHasher> utxos;

jamesob commented at 9:05 pm on January 9, 2024:

This type comes up often enough in this file that it might be worth an alias.

jamesob commented at 4:20 pm on January 10, 2024: contributor

Concept ACK; midway through review and trying to resolve some of the CI issues.

jamesob commented at 5:38 pm on January 10, 2024: contributor

Pushed three additional commits to my branch that may make a dent in the CI issues:

make fs::path hashable: https://github.com/jamesob/bitcoin/commit/8f5fdf8ad3d5102e4afaae415e004d4cf6c667cc
avoid use of std::filesystem::path where possible: https://github.com/jamesob/bitcoin/commit/4ef785749ff1c481b5681739653c5b153637e44e
update linter for mockable filesystem ops: https://github.com/jamesob/bitcoin/commit/face876d55711299c2679aad59b24ee04aa892f0

DrahtBot added the label Needs rebase on Mar 12, 2024

darosior force-pushed on Jul 8, 2024

DrahtBot removed the label Needs rebase on Jul 8, 2024

darosior commented at 5:04 pm on July 8, 2024: member

Rebased this, taking advantage of #28960. I’ve also been investigating alternative approaches.

I first tried to move from fmemopen toward the more flexible memfd_create. It avoided the need for some of the filesystem mocks (which were necessary before because you can’t call fileno on a FILE* created with fmemopen). This allowed to drop one commit. Further, this removed the need from creating 128MiB in-memory blk files. This is in turn makes it potentially more reasonable to sometimes reindex in CallOneOf.

With that implemented i tried to “cache” an initial chainstate to re-use on each fuzzing round, to avoid having to connect the 110 blocks initial chain on every single iteration. Unsuccessfully.

Finally, i wanted to compare the performances of mocking the filesystem to simply using a ramdisk. I realized if i were to use a ramdisk i could simply use the filesystem directly to “cache” an initial chainstate: create two datadirs, one for the initial chainstate, one for the fuzzing iteration. At initialization of the target create a fresh chainstate and connect the 110 blocks. Upon each iteration wipe the working datadir and copy over the initial datadir.

So i implemented that, which besides being more efficient also has the advantage of removing the modifications of non-test code and the platform-dependent syscalls. The target is still pretty slow, but at this point it’s just because of the code it calls: we are doing a bunch of block/header connections and reorg upon every iteration, and each of those can take around a hundred milliseconds.

I’ve pushed a WIP commit which implements what i described above for the chainstate target. Do people think this effort is worth pursuing in this form? If so i’ll clean up this PR to remove the filesystem mocking commits and also use a ramdisk for the blockstorage target.

in src/test/fuzz/chainstate.cpp:78 in 1059ca3f33 outdated

73+
74+    void Init() {
75+        init_datadir = fs::temp_directory_path() / "fuzz_chainstate_init_" PACKAGE_NAME;
76+        fs::remove_all(init_datadir);
77+        fs::create_directories(init_datadir / "blocks");
78+        working_datadir = fs::temp_directory_path() / "fuzz_chainstate_" PACKAGE_NAME;

dergoegge commented at 9:16 am on July 9, 2024:

fs::temp_directory_path() points to the top level tmp dir and won’t be unique every time. You’ll want to do something similar to what the testing setups are doing: https://github.com/bitcoin/bitcoin/blob/1f9d30744d32d24ad3128721cf5bd65a3f1543e8/src/test/util/setup_common.cpp#L150-L152

darosior commented at 9:44 am on July 9, 2024:

Done.

in src/test/fuzz/chainstate.cpp:576 in 1059ca3f33 outdated

565+                std::vector<CBlockHeader> headers;
566+
567+                // In 1% of the cases, generate a random list of headers to be processed. Otherwise, create a single
568+                // valid block.
569+                // TODO: make it possible to generate a chain of more than one valid block.
570+                const bool is_random{fuzzed_data_provider.ConsumeIntegralInRange(0, 99) == 99};

dergoegge commented at 9:30 am on July 9, 2024:

Trying to choose probabilities like this won’t really work with modern fuzzing engines (because the inputs given to the harness are not uniformly random) and is probably equivalent to the following:

0                const bool is_random{fuzzed_data_provider.ConsumeBool()};

darosior commented at 3:35 pm on July 9, 2024:

When debating this with myself my weak argument in favour of it was that the target was very slow and this could help discover interesting paths faster. I also wanted your feedback on this, so thanks.

darosior commented at 3:28 pm on July 31, 2024:

Done.

darosior force-pushed on Jul 9, 2024

in src/test/fuzz/chainstate.cpp:88 in 040af0ea45 outdated

83+        fs::create_directories(init_datadir / "blocks");
84+        working_datadir = tmp_dir / "working";
85+    }
86+
87+    ~TestData() {
88+        fs::remove_all(init_datadir);

dergoegge commented at 12:56 pm on July 9, 2024:

This should nuke the whole parent tmp dir that was created for the fuzz test, not just init_datadir. I’m seeing tons of left over directories otherwise.

darosior commented at 3:28 pm on July 31, 2024:

Done.

Allow mocking CheckProofOfWork c94673af1a

darosior commented at 9:15 pm on July 28, 2024: member

Alright so i cleaned up this PR locally to only use a RAM disk. I’m now in a middle of a significant refactoring of the chainstate fuzz target which i hope to be able to push shortly. If you intended to read through this PoC, maybe wait for the next push.

fuzz: add a target for the BlockManager

Exercise (most of) the public interface of the BlockManager and assert
some invariants. Notably, try to mimick block arrival whether its header
was announced first or not.

a724d1db8e

fuzz: add a target for the ChainstateManager e92c9ddb17

darosior force-pushed on Jul 31, 2024

darosior commented at 3:29 pm on July 31, 2024: member

Cleaned up this PR to always use a ramdisk instead of trying to mock the filesystem. Also significantly reworked the chainstate target.

in src/test/fuzz/chainstate.cpp:203 in e92c9ddb17

198+    util::SignalInterrupt m_interrupt;
199+
200+    void Init() {
201+        SeedRandomForTest(SeedRand::SEED);
202+        const auto rand_str{g_insecure_rand_ctx_temp_path.rand256().ToString()};
203+        m_tmp_dir = fs::temp_directory_path() / "fuzz_chainstate_" PACKAGE_NAME / rand_str;

maflcko commented at 8:43 am on August 1, 2024:

Maybe just inline it, if it is only used once? Also, the build system overhead seems not worth it to place PACKAGE_NAME here? (See lint failure)

 0
 1
 2
 3struct TestData {
 4    fs::path m_tmp_dir;
 5    fs::path m_datadir;
 6    fs::path m_init_datadir;
 7    const CChainParams m_chain_params{*CChainParams::RegTest({})};
 8    KernelNotifications m_notifs;
 9    util::SignalInterrupt m_interrupt;
10
11    void Init() {
12        SeedRandomForTest(SeedRand::SEED);
13        const auto rand_str{FastRandomContext{}.rand256().ToString()}; //! To generate a random tmp datadir per process (necessary to fuzz with multiple cores).
14        m_tmp_dir = fs::temp_directory_path() / "fuzz_chainstate" / rand_str;

hebasto added the label Needs CMake port on Aug 16, 2024

maflcko removed the label Needs CMake port on Aug 29, 2024

DrahtBot commented at 10:45 pm on September 2, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Sep 2, 2024

DrahtBot commented at 1:11 am on November 30, 2024: contributor

⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

maflcko commented at 9:19 am on December 2, 2024: member

cc @darosior ?

TheCharlatan commented at 9:39 am on December 2, 2024: contributor

One alternative that I have considered before (for chainstate fuzzing) is to abstract and further modularize BlockManager, which would allow us to have an InMemoryBlockManager for tests (especially useful for fuzzing but also nice in unit tests).

Forgot to post this here at the time, but I was working on an abstract block store for a different context some months ago: https://github.com/TheCharlatan/bitcoin/commit/5f35e506554bae293012ac108d8336801da2204a. I’m not sure how useful this actually is though, since mounting a ramdisk just is very easy, but maybe it could be useful to verify that certain functions, e.g. allocate, are called at the expected moment.

maflcko commented at 9:45 am on December 2, 2024: member

since mounting a ramdisk just is very easy

It is indeed easy, but for some reason I found mixed results when doing it by default in the CI: #31182 . I couldn’t find a real improvement there, except for the utxo_total_supply fuzz target. (See also https://github.com/bitcoin-core/qa-assets/issues/158#issuecomment-2118120742)

So forcing the blockstore into ram for this target (and possibly others) could still be useful.

darosior commented at 3:33 pm on December 2, 2024: member

This has unfortunately gotten lower in my list of priorities. I’ll close this PR for now to clarify its status. I hope to get back to this at some point. Anyone willing to work on this, or parts of this feel free to grab or/and reach out.

darosior closed this on Dec 2, 2024

PoC: fuzz chainstate and block managers #29158

Code Coverage & Benchmarks

Reviews

Conflicts