fuzz: a target for the block index database #28209

darosior · 2023-08-03T10:50:36Z

This introduces a small fuzz target for CBlockTreeDB which asserts a few invariants by using an in-memory LevelDb.

DrahtBot · 2023-08-03T10:50:39Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, maflcko, brunoerg, achow101
Concept ACK	dergoegge
Stale ACK	jamesob

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

maflcko

Concept ACK, left two nits

src/chain.h

src/test/fuzz/block_index.cpp

darosior · 2023-08-03T11:49:42Z

Thanks, addressed your comments.

dergoegge · 2023-08-03T11:57:14Z

Concept ACK

brunoerg · 2023-08-03T16:58:57Z

Concept ACK

src/test/fuzz/block_index.cpp

darosior · 2023-08-17T09:11:51Z

Rebased on master to fix the macOS CI.

DrahtBot · 2023-09-06T05:58:02Z

Needs rebase if still relevant

darosior · 2023-09-07T10:46:33Z

Ok @DrahtBot. Done.

darosior · 2023-12-31T15:34:15Z

Added a MakeNoLogFileContext at init to avoid the ever-increasing memory usage due to log messages.

sedited

ACK ffee43efe845cbbfbf16d5e61a1d541cb316ef56

sedited · 2024-05-13T08:13:34Z

src/test/fuzz/block_index.cpp

+    const std::string flag_name = fuzzed_data_provider.ConsumeBytesAsString(flag_size);
+    bool flag_value;
+    block_index.WriteFlag(flag_name, true);
+    block_index.ReadFlag(flag_name, flag_value);


Adding something to exercise Erase would also be nice.

darosior · 2024-05-29T17:01:37Z

Thanks for the review, i've also rebased on top of master for fresh CI.

sedited

Re-ACK 86b3852

brunoerg · 2024-08-07T12:13:37Z

src/test/fuzz/block_index.cpp

+
+const BasicTestingSetup* g_setup;
+
+// Hardcoded block hash and nBits to make sure the blocks we store pass the pow check.


In 86b3852: nBits is not hardcoded anymore.

Suggested change

// Hardcoded block hash and nBits to make sure the blocks we store pass the pow check.

// Hardcoded block hash to make sure the blocks we store pass the pow check.

No, it is. (At least the code I reviewed locally)

Yes, but not here as done before.

before:

// Hardcoded block hash and nBits to make sure the blocks we store pass the pow check. const uint256 g_block_hash{uint256S("000000002c05cc2e78923c34df87fd108b22221ac6076c18f3ade378a4d915e9")}; uint32_t g_bits{0x1d00ffff};

anyway, feel free to ignore it.

maflcko

left some style-nits, feel free to ignore. lgtm

review ACK 86b3852 🥒

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 86b38529d5014612c3e7bb59fdc4dad3bff2aa64 🥒
cjVCcUx/BhmdYxcriNMuwwyayOaGH23QYrgOlQYTjyrvNxUXvUDplvgUguxjMPrdahNSoffgh3ITZYRvtcHNCg==

maflcko · 2024-08-07T12:11:43Z

src/test/fuzz/block_index.cpp

+#include <test/fuzz/fuzz.h>
+#include <test/fuzz/util.h>
+#include <test/util/setup_common.h>
+#include <txdb.h>


style-nit: txdb.h is CCoinsViewDB (chainstate/), but you are fuzzing BlockTreeDB (blocks/index/).

Maybe just:

test/fuzz/block_index.cpp should add these lines: #include <cassert> // for assert #include <functional> // for function #include <memory> // for unique_ptr, make_unique #include <optional> // for optional #include <string> // for string #include <utility> // for pair, move #include <vector> // for vector #include "consensus/params.h" // for Params #include "dbwrapper.h" // for DBParams #include "primitives/block.h" // for CBlockHeader, CBlock #include "sync.h" // for MaybeCheckNotHeld, UniqueLock, WITH_LOCK #include "uint256.h" // for uint256 #include "util/fs.h" // for path test/fuzz/block_index.cpp should remove these lines: - #include <txdb.h> // lines 12-12

Not sure why i have this include here, i'll just copy what you suggest.

maflcko · 2024-08-07T12:13:36Z

src/test/fuzz/block_index.cpp

@@ -0,0 +1,133 @@
+// Copyright (c) 2023 The Bitcoin Core developers


style-nit, to pre-empt touching this file on (or after) future changes.

Suggested change

// Copyright (c) 2023 The Bitcoin Core developers

// Copyright (c) 2023-present The Bitcoin Core developers

maflcko · 2024-08-07T12:28:47Z

src/test/fuzz/block_index.cpp

+
+void init_block_index()
+{
+    static const auto testing_setup = MakeNoLogFileContext<>(ChainType::MAIN);


q/nit: Any reason to use MAIN here, instead of the default, or a test-net?

I don't think there is any difference, i just default to test against mainnet when possible. On the other hand it sometimes makes sense to default to regtest instead (for instance in the context of a fuzz target touching validation it would enable the codepaths for all soft forks). So, yeah, 🤷.

Actually i think i can get rid of this now, and just disable logging like i do like in #29158.

i just default to test against mainnet when possible.

I do the opposite 😅 , so that a corrupt test is less likely to corrupt a developers (default) main datadir for their testing by accident, but no strong opinion. Just a style-nit in any case.

(Even if you disable the logging manually, I presume you'll have to select a network) But if it works without, then it is fine as well.

darosior · 2024-08-07T14:31:38Z

Thanks for the review everyone. Maintainers: there is still a determinism issue here as per Niklas, which i have yet to re-reproduce.

…

-------- Original Message --------

On 8/7/24 4:16 PM, Bruno Garcia wrote: @brunoerg commented on this pull request. --------------------------------------------------------------- In [src/test/fuzz/block_index.cpp](#28209 (comment)): > + +#include <chain.h> +#include <chainparams.h> +#include <node/blockstorage.h> +#include <test/fuzz/FuzzedDataProvider.h> +#include <test/fuzz/fuzz.h> +#include <test/fuzz/util.h> +#include <test/util/setup_common.h> +#include <txdb.h> +#include <validation.h> + +namespace { + +const BasicTestingSetup* g_setup; + +// Hardcoded block hash and nBits to make sure the blocks we store pass the pow check. Yes, but not here as done before. before: // Hardcoded block hash and nBits to make sure the blocks we store pass the pow check. const uint256 g_block_hash{ uint256S ( " 000000002c05cc2e78923c34df87fd108b22221ac6076c18f3ade378a4d915e9 " )}; uint32_t g_bits{ 0x1d00ffff }; anyway, feel free to ignore it. — Reply to this email directly, [view it on GitHub](#28209 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F74IQUKD7UY52RWYFTZQIT5VAVCNFSM6AAAAAA3CSR3DWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMRVGMZTOMJYGU). You are receiving this because you authored the thread.Message ID: ***@***.***>

maflcko · 2024-08-07T15:27:26Z

Thanks for the review everyone. Maintainers: there is still a determinism issue here as per Niklas, which i have yet to re-reproduce.

Are you sure? I think may be confused with #28216 (comment) ?

Even if not, I wonder if stability is a hard blocker for a fuzz target. Obviously, it should be fixed, but this can be done in a follow-up, if the fuzz target otherwise makes sense to merge.

dergoegge · 2024-08-07T15:45:49Z

Are you sure? I think may be confused with #28216 (comment)?

Iirc, I pinged Antoine for both PRs on irc.

I also don't think the stability needs to be a blocker.

brunoerg

utACK 86b3852

achow101 · 2024-08-12T19:58:19Z

ACK 86b3852

hebasto · 2024-08-16T19:38:26Z

Ported to the CMake-based build system in hebasto#334.

86b3852 qa: a fuzz target for the block index database (Antoine Poinsot) Pull request description: This introduces a small fuzz target for `CBlockTreeDB` which asserts a few invariants by using an in-memory LevelDb. ACKs for top commit: achow101: ACK 86b3852 TheCharlatan: Re-ACK 86b3852 maflcko: review ACK 86b3852 🥒 brunoerg: utACK 86b3852 Tree-SHA512: ab75b4ae1c7e0a4b15f8a6ceffdf509fbc79833e6ea073ecef68558d53b83663d1b30362aaa2d77c22b8890a572f5b1d4b1c5abbca483c8c8f9b1fb5b276a59a

DrahtBot added the Tests label Aug 3, 2023

fanquake requested a review from dergoegge August 3, 2023 10:51

maflcko approved these changes Aug 3, 2023

View reviewed changes

src/chain.h Outdated Show resolved Hide resolved

src/test/fuzz/block_index.cpp Outdated Show resolved Hide resolved

darosior force-pushed the fuzz_block_index branch from 8f26ab2 to 8a0cb8b Compare August 3, 2023 11:49

brunoerg reviewed Aug 3, 2023

View reviewed changes

src/test/fuzz/block_index.cpp Outdated Show resolved Hide resolved

darosior force-pushed the fuzz_block_index branch 2 times, most recently from 65a107e to c3f6289 Compare August 4, 2023 09:40

darosior mentioned this pull request Aug 4, 2023

fuzz: a new target for the coins database #28216

Closed

darosior force-pushed the fuzz_block_index branch from c3f6289 to 7f6ff2f Compare August 15, 2023 10:43

DrahtBot added the CI failed label Aug 15, 2023

darosior force-pushed the fuzz_block_index branch from 7f6ff2f to e626f35 Compare August 17, 2023 09:11

DrahtBot removed the CI failed label Aug 17, 2023

DrahtBot added the CI failed label Sep 5, 2023

darosior force-pushed the fuzz_block_index branch from e626f35 to f0ca012 Compare September 7, 2023 10:46

darosior force-pushed the fuzz_block_index branch from f0ca012 to 1847309 Compare September 8, 2023 09:10

DrahtBot removed the CI failed label Sep 10, 2023

DrahtBot added CI failed and removed CI failed labels Sep 22, 2023

darosior force-pushed the fuzz_block_index branch from 1847309 to 1c95882 Compare December 31, 2023 15:34

DrahtBot added the CI failed label Dec 31, 2023

darosior force-pushed the fuzz_block_index branch from 1c95882 to 8083aa2 Compare January 2, 2024 11:10

DrahtBot removed the CI failed label Jan 2, 2024

sedited approved these changes May 13, 2024

View reviewed changes

DrahtBot requested review from brunoerg and jamesob May 13, 2024 08:19

qa: a fuzz target for the block index database

86b3852

darosior force-pushed the fuzz_block_index branch from ffee43e to 86b3852 Compare May 29, 2024 16:55

sedited approved these changes May 30, 2024

View reviewed changes

glozow requested review from brunoerg, dergoegge and maflcko and removed request for brunoerg, dergoegge and maflcko August 7, 2024 11:57

brunoerg reviewed Aug 7, 2024

View reviewed changes

DrahtBot requested a review from brunoerg August 7, 2024 12:13

maflcko approved these changes Aug 7, 2024

View reviewed changes

brunoerg approved these changes Aug 7, 2024

View reviewed changes

achow101 merged commit ee57737 into bitcoin:master Aug 12, 2024

hebasto added the Needs CMake port label Aug 15, 2024

hebasto mentioned this pull request Aug 16, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#334

Closed

hebasto removed the Needs CMake port label Aug 16, 2024

darosior deleted the fuzz_block_index branch November 8, 2024 21:16

bitcoin locked and limited conversation to collaborators Nov 8, 2025


		const BasicTestingSetup* g_setup;

		// Hardcoded block hash and nBits to make sure the blocks we store pass the pow check.

	// Hardcoded block hash and nBits to make sure the blocks we store pass the pow check.
	// Hardcoded block hash to make sure the blocks we store pass the pow check.

		@@ -0,0 +1,133 @@
		// Copyright (c) 2023 The Bitcoin Core developers

	// Copyright (c) 2023 The Bitcoin Core developers
	// Copyright (c) 2023-present The Bitcoin Core developers

fuzz: a target for the block index database #28209

fuzz: a target for the block index database #28209

Uh oh!

Conversation

darosior commented Aug 3, 2023

Uh oh!

DrahtBot commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

darosior commented Aug 3, 2023

Uh oh!

dergoegge commented Aug 3, 2023

Uh oh!

brunoerg commented Aug 3, 2023

Uh oh!

Uh oh!

darosior commented Aug 17, 2023

Uh oh!

DrahtBot commented Sep 6, 2023

Uh oh!

darosior commented Sep 7, 2023

Uh oh!

darosior commented Dec 31, 2023

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darosior commented May 29, 2024

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darosior commented Aug 7, 2024 via email

Uh oh!

maflcko commented Aug 7, 2024

Uh oh!

dergoegge commented Aug 7, 2024

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 12, 2024

Uh oh!

hebasto commented Aug 16, 2024

Uh oh!

Reviewers

DrahtBot commented Aug 3, 2023 •

edited

Loading

maflcko Aug 7, 2024 •

edited

Loading