fuzz: a target for the block index database

darosior commented at 10:50 am on August 3, 2023: member

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

DrahtBot commented at 10:50 am on August 3, 2023: contributor

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.

DrahtBot added the label Tests on Aug 3, 2023

fanquake requested review from dergoegge on Aug 3, 2023

in src/chain.h:96 in 8f26ab29a9 outdated

91@@ -92,6 +92,16 @@ class CBlockFileInfo
92         if (nTimeIn > nTimeLast)
93             nTimeLast = nTimeIn;
94     }
95+
96+    bool operator==(const CBlockFileInfo& b) const {

maflcko commented at 11:21 am on August 3, 2023:

not sure about adding test-only code to “real” code. What about moving this to the only fuzz test that needs it?

darosior commented at 11:44 am on August 3, 2023:

Sure

in src/test/fuzz/block_index.cpp:6 in 8f26ab29a9 outdated

0@@ -0,0 +1,116 @@
1+// Copyright (c) 2023 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#include <pow.h>
6+

maflcko commented at 11:21 am on August 3, 2023:

nit: remove newline?

darosior commented at 11:45 am on August 3, 2023:

Oh, i forgot to push my last change, this include shouldn’t be here (that’s why i had separated it).

maflcko approved

maflcko commented at 11:34 am on August 3, 2023: member

Concept ACK, left two nits

darosior force-pushed on Aug 3, 2023

darosior commented at 11:49 am on August 3, 2023: member

Thanks, addressed your comments.

dergoegge commented at 11:57 am on August 3, 2023: member

Concept ACK

brunoerg commented at 4:58 pm on August 3, 2023: contributor

Concept ACK

in src/test/fuzz/block_index.cpp:70 in 8a0cb8b214 outdated

51+    std::vector<std::unique_ptr<CBlockFileInfo>> files;
52+    files.reserve(files_count);
53+    std::vector<std::pair<int, const CBlockFileInfo*>> files_info;
54+    files_info.reserve(files_count);
55+    for (int i = 0; i < files_count; i++) {
56+        if (auto file_info = ConsumeDeserializable<CBlockFileInfo>(fuzzed_data_provider)) {

brunoerg commented at 5:18 pm on August 3, 2023:

In 8a0cb8b2147e852ac80d2030057272bdb59a83f2: Instead of using ConsumeDeserializable, couldn’t we have a function to create a CBlockFileInfo? E.g.:

 0diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
 1index b5b25fcbc7..528c2fae9f 100644
 2--- a/src/test/fuzz/block_index.cpp
 3+++ b/src/test/fuzz/block_index.cpp
 4@@ -37,6 +37,19 @@ void init_block_index()
 5     SelectParams(ChainType::MAIN);
 6 }
 7 
 8+CBlockFileInfo CreateCBlockFileInfo(FuzzedDataProvider& fuzzed_data_provider)
 9+{
10+    CBlockFileInfo block_file_info;
11+    block_file_info.nBlocks = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
12+    block_file_info.nSize = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
13+    block_file_info.nUndoSize = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
14+    block_file_info.nHeightFirst = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
15+    block_file_info.nHeightLast = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
16+    block_file_info.nTimeFirst = fuzzed_data_provider.ConsumeIntegral<uint64_t>();
17+    block_file_info.nTimeLast = fuzzed_data_provider.ConsumeIntegral<uint64_t>();
18+    return block_file_info;
19+}
20+
21 FUZZ_TARGET(block_index, .init = init_block_index)
22 {
23     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
24@@ -53,12 +66,9 @@ FUZZ_TARGET(block_index, .init = init_block_index)
25     std::vector<std::pair<int, const CBlockFileInfo*>> files_info;
26     files_info.reserve(files_count);
27     for (int i = 0; i < files_count; i++) {
28-        if (auto file_info = ConsumeDeserializable<CBlockFileInfo>(fuzzed_data_provider)) {
29-            files.push_back(std::make_unique<CBlockFileInfo>(std::move(*file_info)));
30-            files_info.push_back({i, files.back().get()});
31-        } else {
32-            return;
33-        }
34+        auto file_info{CreateCBlockFileInfo(fuzzed_data_provider)};
35+        files.push_back(std::make_unique<CBlockFileInfo>(std::move(file_info)));
36+        files_info.push_back({i, files.back().get()});
37     }
38 
39     // Generate a number of block headers to be stored in the index.

darosior commented at 9:25 am on August 4, 2023:

Good call, i’ve adopted this approach for both the CBlockFiles and the CBlockHeaders.

maflcko commented at 12:18 pm on August 4, 2023:

What is the benefit? Looks like this is more code, easier to break (when for example a type width changes, or when a new field is added), as well as more wasteful (the early return is now removed and the fuzz engine will do a full run even if the fuzz input buffer is the empty string)?

darosior commented at 12:27 pm on August 4, 2023:

My thinking was that it would actually be more efficient to not rely on ConsumeDeserializable which needs to first guess the length of the byte vector to be consumed: https://github.com/bitcoin/bitcoin/blob/a4ca4975880c4f870c6047065c70610af2529e74/src/test/fuzz/util.h#L95-L107

maflcko commented at 12:30 pm on August 4, 2023:

Ok, that could be. Maybe a benchmark can be done to see if it helps or hurts?

In any case, if you keep it, my preference would be to use decltype() to derive the type of the fields and not hardcode them.

brunoerg commented at 12:44 pm on August 4, 2023:

+1

My thinking was that it would actually be more efficient to not rely on ConsumeDeserializable which needs to first guess the length of the byte vector to be consumed

My initial thought was it facilitates especially for cases that files_count/files_count is closer to their max possible value.

darosior commented at 1:49 pm on August 4, 2023:

Alright, in order to benchmark which approach was most efficient i ran both versions with -runs=100000 on an empty folder 3 times and compared the average coverage and runtime.

ConsumeIntegral-based: runtime of 75 seconds for a coverage of 1565 for all 3 runs.
ConsumeDeserializable-based: average runtime of 12 seconds for a coverage of 1560.

Given the burst in coverage at the start of the run, the clear difference in runtime and the small difference in coverage i figured i’d need better measurement. I ran both on an empty folders with -runs=1000000:

ConsumeIntegral-based: coverage of 2230 after 1M runs (runtime: 2872 seconds).
ConsumeDeserializable-based: coverage of 2167 after 1M runs (runtime: 1614 seconds).

My interpretation of these results is that the ConsumeDeserializable-based target runs faster because of the invalid deserializations, that both targets quickly get to some basic coverage, but that the ConsumeIntegral-based one eventually gets to produce more interesting coverage. In my opinion i should therefore keep the ConsumeIntegral-based version even though its coverage/second is lower.

brunoerg commented at 1:57 pm on August 4, 2023:

Yea, ConsumeDeserializable is faster because it will ‘return’ every time it gets an invalid deserialization. The coverage is similar but I believe that it’s expected because the difference between both approaches will only reflect on the size of blocks and files.

brunoerg commented at 1:59 pm on August 4, 2023:

You could also test both approaches by putting an assert right after the loop, something like: assert(files.size() > 50). I believe that the ConsumeDeserializable-based will take much more time to reach it.

maflcko commented at 2:04 pm on August 4, 2023:

My interpretation of these results is

I don’t think you can use coverage as a metric when comparing two different code bases. The version that has higher coverage is also the version that has more code in the fuzz target, which is also counted toward “coverage”.

darosior commented at 2:15 pm on August 4, 2023:

Trying to maximum the coverage in 100_000 runs for each target i managed to:

achieve 2099 of coverage with the ConsumeDeserializable-based target using -max_len=10000 -len_control=1 -mutate_depth=3 (in 305 seconds).
achieve 2088 of coverage with the ConsumeIntegral-based target using -max_len=8000 -len_control=0 -use_value_profile=1 -mutate_depth=1` (in 252 seconds).

I don’t think you can use coverage as a metric

Oh, right.. I had overlooked this. However the difference should be minimal, there is only 8 more lines for the ConsumeIntegral version. So i think it’s still interesting to compare.

I’m quite surprised by how well the ConsumeDeserializable-based compares though.. I’m starting to lean toward reverting to this version.

Do you have a suggestion of a better metric? I feel like introducing a crash wouldn’t be interesting given the low coverage of the target.

maflcko commented at 2:35 pm on August 4, 2023:

I’m starting to lean toward reverting to this version.

There’s also the possibility to add back the early return to the ConsumeIntegral one. (I haven’t looked at the fuzz target to see if early return makes more or less sense)

I feel like introducing a crash wouldn’t be interesting given the low coverage of the target.

yeah, I guess it is hard to find a meaningful crash. I’d pick a line of code that is usually reached the “last” by coverage or is deeply nested.

darosior commented at 10:49 am on August 15, 2023:

I’ve reverted back to using ConsumeDeserializable. I had the same intuition as @brunoerg but from my testing it’s not clear that the custom way is more efficient (in terms of coverage per unit of time). Absent this, it makes sense to not introduce more code and simply use the existing utilities.

darosior force-pushed on Aug 4, 2023

darosior force-pushed on Aug 15, 2023

DrahtBot added the label CI failed on Aug 15, 2023

darosior force-pushed on Aug 17, 2023

darosior commented at 9:11 am on August 17, 2023: member

Rebased on master to fix the macOS CI.

DrahtBot removed the label CI failed on Aug 17, 2023

DrahtBot added the label CI failed on Sep 5, 2023

DrahtBot commented at 5:58 am on September 6, 2023: contributor

Needs rebase if still relevant

darosior force-pushed on Sep 7, 2023

darosior commented at 10:46 am on September 7, 2023: member

Ok @DrahtBot. Done.

darosior force-pushed on Sep 8, 2023

DrahtBot removed the label CI failed on Sep 10, 2023

DrahtBot added the label CI failed on Sep 22, 2023

DrahtBot removed the label CI failed on Sep 25, 2023

darosior force-pushed on Dec 31, 2023

darosior commented at 3:34 pm on December 31, 2023: member

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

DrahtBot added the label CI failed on Dec 31, 2023

darosior force-pushed on Jan 2, 2024

DrahtBot removed the label CI failed on Jan 2, 2024

dergoegge commented at 3:45 pm on January 3, 2024: member

Coverage for block_index: https://dergoegge.github.io/bitcoin-coverage/pr28209/fuzz.coverage/index.html

in src/test/fuzz/block_index.cpp:120 in 8083aa21a6 outdated

116+    // We should be able to set and read the value of any random flag.
117+    int flag_size = fuzzed_data_provider.ConsumeIntegralInRange(0, 100);
118+    const std::string flag_name = fuzzed_data_provider.ConsumeBytesAsString(flag_size);
119+    bool flag_value;
120+    block_index.WriteFlag(flag_name, true);
121+    block_index.ReadFlag(flag_name, flag_value);

brunoerg commented at 7:17 pm on January 3, 2024:

Couldn’t we fuzz ReadFlag with a name we didn’t previously write?

jamesob commented at 7:55 pm on January 5, 2024:

Good suggestion, but could be done in a follow-up I guess.

TheCharlatan commented at 8:13 am on May 13, 2024:

Adding something to exercise Erase would also be nice.

in src/test/fuzz/block_index.cpp:57 in 8083aa21a6 outdated

52+}
53+
54+FUZZ_TARGET(block_index, .init = init_block_index)
55+{
56+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
57+    auto block_index = kernel::BlockTreeDB(DBParams{

brunoerg commented at 1:38 pm on January 4, 2024:

nit (feel free to ignore):

 0diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
 1index 7885eda747..3f2a55f310 100644
 2--- a/src/test/fuzz/block_index.cpp
 3+++ b/src/test/fuzz/block_index.cpp
 4@@ -54,11 +54,6 @@ void init_block_index()
 5 FUZZ_TARGET(block_index, .init = init_block_index)
 6 {
 7     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
 8-    auto block_index = kernel::BlockTreeDB(DBParams{
 9-        .path = "", // Memory only.
10-        .cache_bytes = 1 << 20, // 1MB.
11-        .memory_only = true,
12-    });
13 
14     // Generate a number of block files to be stored in the index.
15     int files_count = fuzzed_data_provider.ConsumeIntegralInRange(1, 100);
16@@ -75,6 +70,12 @@ FUZZ_TARGET(block_index, .init = init_block_index)
17         }
18     }
19 
20+    auto block_index = kernel::BlockTreeDB(DBParams{
21+        .path = "", // Memory only.
22+        .cache_bytes = 1 << 20, // 1MB.
23+        .memory_only = true,
24+    });
25+
26     // Generate a number of block headers to be stored in the index.
27     int blocks_count = fuzzed_data_provider.ConsumeIntegralInRange(files_count * 10, files_count * 100);
28     std::vector<std::unique_ptr<CBlockIndex>> blocks;

we could initialize the DB after having the files.

darosior commented at 2:31 pm on January 4, 2024:

Heh, no that’s a good point. There is a bunch happening in CDBWrapper’s constructor so i’m sure this would reduce the time we spend on uninteresting inputs.

darosior commented at 3:05 pm on January 4, 2024:

Ok, looks like i’m wrong. It doesn’t make any difference in runtime when running my target over my local corpus. I’ll hold off making this change then.

in src/test/fuzz/block_index.cpp:96 in 8083aa21a6 outdated

91+    // Store these files and blocks in the block index. It should not fail.
92+    assert(block_index.WriteBatchSync(files_info, files_count - 1, blocks_info));
93+
94+    // We should be able to read every block file info we stored. Its value should correspond to
95+    // what we stored above.
96+    CBlockFileInfo info;

brunoerg commented at 1:42 pm on January 4, 2024:

nit (feel free to ignore):

 0diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
 1index 7885eda747..bc073e4930 100644
 2--- a/src/test/fuzz/block_index.cpp
 3+++ b/src/test/fuzz/block_index.cpp
 4@@ -93,8 +93,8 @@ FUZZ_TARGET(block_index, .init = init_block_index)
 5 
 6     // We should be able to read every block file info we stored. Its value should correspond to
 7     // what we stored above.
 8-    CBlockFileInfo info;
 9     for (const auto& [n, file_info]: files_info) {
10+        CBlockFileInfo info;
11         assert(block_index.ReadBlockFileInfo(n, info));
12         assert(info == *file_info);
13     }

darosior commented at 2:10 pm on January 4, 2024:

Why? (EDIT: giving a longer answer: why instantiating a new CBlockFileInfo for each iteration instead of just reusing the same variable?)

brunoerg commented at 2:46 pm on January 4, 2024:

Nevermind, my bad. It’s better to reuse the same variable.

brunoerg approved

brunoerg commented at 1:44 pm on January 4, 2024: contributor

crACK 8083aa21a699cb240a11741be8ba5cfeeb58ee8b

left some nits

DrahtBot requested review from maflcko on Jan 4, 2024

jamesob approved

jamesob commented at 7:58 pm on January 5, 2024: contributor

ACK 8083aa21a699cb240a11741be8ba5cfeeb58ee8b (jamesob/ackr/28209.1.darosior.fuzz_a_target_for_the_bl)

Ran the test locally and read over the change. This is a good basic test to have in place; seems like a low-risk merge.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 8083aa21a699cb240a11741be8ba5cfeeb58ee8b ([`jamesob/ackr/28209.1.darosior.fuzz_a_target_for_the_bl`](https://github.com/jamesob/bitcoin/tree/ackr/28209.1.darosior.fuzz_a_target_for_the_bl))
 4
 5Ran the test locally and read over the change. This is a good basic test to have in place; seems like a low-risk merge.
 6-----BEGIN PGP SIGNATURE-----
 7
 8iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmWYX70ACgkQepNdrbLE
 9TwVgYQ/+LDdpandHPyTi1cLLNY5tCGl/r8dIh5bpYrpz1GNgCdkbvpR3Bmu96NrP
10BbyG4CjtvoZM//F+Ri9Iizy7RjIDXdClnpn5Uw7LQ45Mq0ow9LHRAQB1hTKyHi0v
11SQZ/91I9ZuthzX2EAMwxcCmc1KrRpG8zzM3uDBZ2PCHYjO9/I1e3J6FS0gyJkOFP
12HXDwKo6gPmMXlKOdsdYzWH/M01anDSuK3lK5VYDL6ORgaj75BCc8UOzALKyQXGoG
13jTU27Iu9wGyup5uSZzCEMoTwJUyrcx9jJzmIjdkTPQH7SVerHFOs9yPPeUVv5BPO
14b0yLc3RIMKnFe+1bIHO49KHdd1eXsYeimAMaLygrj7vZrh81RX5Qwf5aKIy2X2HV
15TE5I2yBQ2AX9/e8XNZjSc92pMySdH/deCJOlfADBsWvsYfgjyZQrXY9Q9wQClOUg
16siz/egYV+aqeMjlqC9MLjpY9q+MxJge5r11v9zKqsI99H/VuUdX5Gl0WyvwANxN2
17YcBcXduTeop/PN8R55s3mKFNnts0pagvhYKHXJnDHuSEcIQsxK/F3YWQPo2KL4Ks
18sigccvKG01zezFWiEB/hUsdUl7gz1GkKd56soqVr5CPPwMJobAKJfrcg8pHIgUYe
19BLXPt7sZzgF59Udys1bZ8CjJPUIepx2XVjP6bz8vAyl8EfnctxI=
20=2f3X
21-----END PGP SIGNATURE-----

0Tested on Linux-6.6.8-arch1-1-x86_64-with-glibc2.38
1
2Configured with ./configure 'BDB_LIBS=-L/home/james/src/bitcoin/db4/lib -ldb_cxx-4.8' BDB_CFLAGS=-I/home/james/src/bitcoin/db4/include 'CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare ' --disable-gui-tests --enable-fuzz --with-sanitizers=address,fuzzer,undefined --enable-wallet --enable-debug --with-daemon --enable-natpmp-default
3
4Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++20 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare  -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -fsanitize=address,fuzzer,undefined -msse4.1 -msse4.2 -msse4 -msha  i
5
6Compiler version: clang version 16.0.6

jamesob commented at 4:37 pm on January 8, 2024: contributor

Two ACKs on a test change - RFM?

in src/test/fuzz/block_index.cpp:21 in 8083aa21a6 outdated

16+
17+const BasicTestingSetup* g_setup;
18+
19+// Hardcoded block hash and nBits to make sure the blocks we store pass the pow check.
20+const uint256 g_block_hash{uint256S("000000002c05cc2e78923c34df87fd108b22221ac6076c18f3ade378a4d915e9")};
21+uint32_t g_bits{0x1d00ffff};

dergoegge commented at 1:29 pm on January 9, 2024:

nit: I think constants are not supposed to be prefixed with g_

0const uint256 g_block_hash{uint256S("000000002c05cc2e78923c34df87fd108b22221ac6076c18f3ade378a4d915e9")};
1const uint32_t g_bits{0x1d00ffff};

I think you could also just use the genesis block (Params().GenesisBlock()) for these two values.

darosior commented at 12:48 pm on January 13, 2024:

nit: I think constants are not supposed to be prefixed with g_

https://github.com/bitcoin/bitcoin/blob/3ba8de1b704d590fa4e1975620bd21d830d11666/doc/developer-notes.md?plain=1#L87-L93

I think you could also just use the genesis block (Params().GenesisBlock()) for these two values.

Done.

in src/test/fuzz/block_index.cpp:118 in 8083aa21a6 outdated

113+    block_index.ReadReindexing(reindexing);
114+    assert(!reindexing);
115+
116+    // We should be able to set and read the value of any random flag.
117+    int flag_size = fuzzed_data_provider.ConsumeIntegralInRange(0, 100);
118+    const std::string flag_name = fuzzed_data_provider.ConsumeBytesAsString(flag_size);

dergoegge commented at 1:35 pm on January 9, 2024:

0    const std::string flag_name = fuzzed_data_provider.ConsumeRandomLengthString(100);

ConsumeRandomLengthString: “Designed to be more stable with respect to a fuzzer inserting characters than just picking a random length and then consuming that many bytes with |ConsumeBytes|.”

darosior commented at 12:48 pm on January 13, 2024:

Done.

in src/test/fuzz/block_index.cpp:132 in 8083aa21a6 outdated

128+    // return value we need to make sure all blocks pass the pow check.
129+    const auto params{Params().GetConsensus()};
130+    const auto inserter = [&](const uint256&) {
131+        return blocks.back().get();
132+    };
133+    WITH_LOCK(::cs_main, assert(block_index.LoadBlockIndexGuts(params, inserter, g_setup->m_interrupt)));

dergoegge commented at 1:40 pm on January 9, 2024:

Can we get rid of g_setup if we initialize a new SignalInterrupt here each interation?

darosior commented at 4:01 pm on January 9, 2024:

You need it for the logs iirc.

TheCharlatan commented at 8:40 pm on May 12, 2024:

~~How about:~~ EDIT: Never mind, this wasn’t worth it.

DrahtBot requested review from dergoegge on Jan 9, 2024

DrahtBot added the label CI failed on Jan 12, 2024

darosior force-pushed on Jan 13, 2024

DrahtBot removed the label CI failed on Jan 13, 2024

achow101 requested review from TheCharlatan on Apr 9, 2024

in src/test/fuzz/block_index.cpp:11 in ffee43efe8 outdated

 8+#include <txdb.h>
 9+#include <validation.h>
10+#include <test/fuzz/FuzzedDataProvider.h>
11+#include <test/fuzz/fuzz.h>
12+#include <test/fuzz/util.h>
13+#include <test/util/setup_common.h>

TheCharlatan commented at 8:40 pm on May 12, 2024:

Nit: Can you clean up the includes?

darosior commented at 4:59 pm on May 29, 2024:

Reordered the includes.

TheCharlatan commented at 8:46 pm on May 12, 2024: contributor

Concept ACK

TheCharlatan approved

TheCharlatan commented at 8:19 am on May 13, 2024: contributor

ACK ffee43efe845cbbfbf16d5e61a1d541cb316ef56

DrahtBot requested review from jamesob on May 13, 2024

DrahtBot requested review from brunoerg on May 13, 2024

qa: a fuzz target for the block index database 86b38529d5

darosior force-pushed on May 29, 2024

darosior commented at 5:01 pm on May 29, 2024: member

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

TheCharlatan approved

TheCharlatan commented at 11:29 am on May 30, 2024: contributor

Re-ACK 86b38529d5014612c3e7bb59fdc4dad3bff2aa64

glozow removed review request from maflcko on Aug 7, 2024

glozow removed review request from dergoegge on Aug 7, 2024

glozow removed review request from brunoerg on Aug 7, 2024

glozow requested review from brunoerg on Aug 7, 2024

glozow requested review from maflcko on Aug 7, 2024

glozow requested review from dergoegge on Aug 7, 2024

in src/test/fuzz/block_index.cpp:12 in 86b38529d5

 7+#include <node/blockstorage.h>
 8+#include <test/fuzz/FuzzedDataProvider.h>
 9+#include <test/fuzz/fuzz.h>
10+#include <test/fuzz/util.h>
11+#include <test/util/setup_common.h>
12+#include <txdb.h>

maflcko commented at 12:11 pm on August 7, 2024:

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

Maybe just:

 0test/fuzz/block_index.cpp should add these lines:
 1#include <cassert>                        // for assert
 2#include <functional>                      // for function
 3#include <memory>                          // for unique_ptr, make_unique
 4#include <optional>                        // for optional
 5#include <string>                          // for string
 6#include <utility>                         // for pair, move
 7#include <vector>                          // for vector
 8
 9#include "consensus/params.h"              // for Params
10#include "dbwrapper.h"                     // for DBParams
11#include "primitives/block.h"              // for CBlockHeader, CBlock
12#include "sync.h"                          // for MaybeCheckNotHeld, UniqueLock, WITH_LOCK
13#include "uint256.h"                       // for uint256
14#include "util/fs.h"                       // for path
15
16test/fuzz/block_index.cpp should remove these lines:
17- #include <txdb.h>  // lines 12-12

darosior commented at 3:55 pm on August 7, 2024:

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

in src/test/fuzz/block_index.cpp:1 in 86b38529d5

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

maflcko commented at 12:13 pm on August 7, 2024:

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

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

in src/test/fuzz/block_index.cpp:19 in 86b38529d5

14+
15+namespace {
16+
17+const BasicTestingSetup* g_setup;
18+
19+// Hardcoded block hash and nBits to make sure the blocks we store pass the pow check.

brunoerg commented at 12:13 pm on August 7, 2024:

In 86b38529d5014612c3e7bb59fdc4dad3bff2aa64: nBits is not hardcoded anymore.

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

maflcko commented at 1:06 pm on August 7, 2024:

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

brunoerg commented at 2:16 pm on August 7, 2024:

Yes, but not here as done before.

before:

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

anyway, feel free to ignore it.

DrahtBot requested review from brunoerg on Aug 7, 2024

in src/test/fuzz/block_index.cpp:49 in 86b38529d5

44+
45+} // namespace
46+
47+void init_block_index()
48+{
49+    static const auto testing_setup = MakeNoLogFileContext<>(ChainType::MAIN);

maflcko commented at 12:28 pm on August 7, 2024:

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

darosior commented at 3:51 pm on August 7, 2024:

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, :shrug:.

darosior commented at 3:53 pm on August 7, 2024:

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

maflcko commented at 4:23 pm on August 7, 2024:

i just default to test against mainnet when possible.

I do the opposite :sweat_smile: , 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.

maflcko approved

maflcko commented at 1:04 pm on August 7, 2024: member

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

review ACK 86b38529d5014612c3e7bb59fdc4dad3bff2aa64 🥒

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 86b38529d5014612c3e7bb59fdc4dad3bff2aa64 🥒
3cjVCcUx/BhmdYxcriNMuwwyayOaGH23QYrgOlQYTjyrvNxUXvUDplvgUguxjMPrdahNSoffgh3ITZYRvtcHNCg==

darosior commented at 2:31 pm on August 7, 2024: member

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:

+#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, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

maflcko commented at 3:27 pm on August 7, 2024: member

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 commented at 3:45 pm on August 7, 2024: member

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 approved

brunoerg commented at 4:52 pm on August 7, 2024: contributor

utACK 86b38529d5014612c3e7bb59fdc4dad3bff2aa64

achow101 commented at 7:58 pm on August 12, 2024: member

ACK 86b38529d5014612c3e7bb59fdc4dad3bff2aa64

achow101 merged this on Aug 12, 2024

achow101 closed this on Aug 12, 2024

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

hebasto commented at 7:38 pm on August 16, 2024: member

Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/334.

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

darosior deleted the branch on Nov 8, 2024

fuzz: a target for the block index database #28209

Code Coverage

Reviews