fuzz: [refactor] Remove unused g

maflcko commented at 11:15 am on March 25, 2026: member

This is unused and avoids a clang warning.

Can be tested via: git grep --count '\<g_setup' | grep ':2'

Before: Prints files names. After: No output.

DrahtBot added the label Fuzzing on Mar 25, 2026

DrahtBot commented at 11:16 am on March 25, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, hodlinator
Stale ACK	sedited

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

maflcko commented at 11:16 am on March 25, 2026: member

Ref: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/23524974067/job/68476342922#step:6:666

cc @hebasto

sedited approved

sedited commented at 11:23 am on March 25, 2026: contributor

ACK fa4bbd6378fe133f4d125357fbe7bb0bfe876eb6

hebasto commented at 11:24 am on March 25, 2026: member

Running https://github.com/hebasto/bitcoin-core-nightly/pull/221.

in src/test/fuzz/deserialize.cpp:1 in fa4bbd6378 outdated

hodlinator commented at 11:32 am on March 25, 2026:

(PR description could mention fac81affb527132945773a5315bd27fec61ec52f removing use of g_setup).

maflcko commented at 12:13 pm on March 25, 2026:

I don’t think it is relevant where and when this was introduced. For reference, the other one was commit dec138d1ddc79cc3a06e53ed255f0931ce46e684

in src/test/fuzz/deserialize.cpp:43 in fa4bbd6378 outdated

42-const BasicTestingSetup* g_setup;
43-} // namespace
44-
45 void initialize_deserialize()
46 {
47     static const auto testing_setup = MakeNoLogFileContext<>();

hodlinator commented at 11:51 am on March 25, 2026:

Only the snapshotmetadata_deserialize-target needs this. Moreover, it only needs SelectParams(ChainType::MAIN); to be called.

 0diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp
 1index 1329f471c7..23e5e0dfbc 100644
 2--- a/src/test/fuzz/deserialize.cpp
 3+++ b/src/test/fuzz/deserialize.cpp
 4@@ -8,6 +8,7 @@
 5 #include <blockencodings.h>
 6 #include <blockfilter.h>
 7 #include <chain.h>
 8+#include <chainparams.h>
 9 #include <coins.h>
10 #include <common/args.h>
11 #include <compressor.h>
12@@ -27,7 +28,6 @@
13 #include <streams.h>
14 #include <test/fuzz/fuzz.h>
15 #include <test/fuzz/util.h>
16-#include <test/util/setup_common.h>
17 #include <undo.h>
18 
19 #include <cstdint>
20@@ -38,13 +38,13 @@
21 using kernel::CBlockFileInfo;
22 using node::SnapshotMetadata;
23 
24-void initialize_deserialize()
25+static void InitializeParams()
26 {
27-    static const auto testing_setup = MakeNoLogFileContext<>();
28+    SelectParams(ChainType::MAIN);
29 }
30 
31 #define FUZZ_TARGET_DESERIALIZE(name, code)                \
32-    FUZZ_TARGET(name, .init = initialize_deserialize)      \
33+    FUZZ_TARGET(name)                                      \
34     {                                                      \
35         try {                                              \
36             code                                           \
37@@ -128,7 +128,7 @@ FUZZ_TARGET_DESERIALIZE(block_filter_deserialize, {
38     BlockFilter block_filter;
39     DeserializeFromFuzzingInput(buffer, block_filter);
40 })
41-FUZZ_TARGET(addr_info_deserialize, .init = initialize_deserialize)
42+FUZZ_TARGET(addr_info_deserialize)
43 {
44     FuzzedDataProvider fdp{buffer.data(), buffer.size()};
45     (void)ConsumeDeserializable<AddrInfo>(fdp, ConsumeDeserializationParams<CAddress::SerParams>(fdp));
46@@ -229,7 +229,7 @@ FUZZ_TARGET_DESERIALIZE(coins_deserialize, {
47     Coin coin;
48     DeserializeFromFuzzingInput(buffer, coin);
49 })
50-FUZZ_TARGET(netaddr_deserialize, .init = initialize_deserialize)
51+FUZZ_TARGET(netaddr_deserialize)
52 {
53     FuzzedDataProvider fdp{buffer.data(), buffer.size()};
54     const auto maybe_na{ConsumeDeserializable<CNetAddr>(fdp, ConsumeDeserializationParams<CNetAddr::SerParams>(fdp))};
55@@ -240,7 +240,7 @@ FUZZ_TARGET(netaddr_deserialize, .init = initialize_deserialize)
56     }
57     AssertEqualAfterSerializeDeserialize(na, CNetAddr::V2);
58 }
59-FUZZ_TARGET(service_deserialize, .init = initialize_deserialize)
60+FUZZ_TARGET(service_deserialize)
61 {
62     FuzzedDataProvider fdp{buffer.data(), buffer.size()};
63     const auto ser_params{ConsumeDeserializationParams<CNetAddr::SerParams>(fdp)};
64@@ -260,7 +260,7 @@ FUZZ_TARGET_DESERIALIZE(messageheader_deserialize, {
65     DeserializeFromFuzzingInput(buffer, mh);
66     (void)mh.IsMessageTypeValid();
67 })
68-FUZZ_TARGET(address_deserialize, .init = initialize_deserialize)
69+FUZZ_TARGET(address_deserialize)
70 {
71     FuzzedDataProvider fdp{buffer.data(), buffer.size()};
72     const auto ser_enc{ConsumeDeserializationParams<CAddress::SerParams>(fdp)};
73@@ -310,11 +310,15 @@ FUZZ_TARGET_DESERIALIZE(blocktransactionsrequest_deserialize, {
74     BlockTransactionsRequest btr;
75     DeserializeFromFuzzingInput(buffer, btr);
76 })
77-FUZZ_TARGET_DESERIALIZE(snapshotmetadata_deserialize, {
78+FUZZ_TARGET(snapshotmetadata_deserialize, .init = InitializeParams)
79+{
80     auto msg_start = Params().MessageStart();
81     SnapshotMetadata snapshot_metadata{msg_start};
82-    DeserializeFromFuzzingInput(buffer, snapshot_metadata);
83-})
84+    try {
85+        DeserializeFromFuzzingInput(buffer, snapshot_metadata);
86+    } catch (const invalid_fuzzing_input_exception&) {
87+    }
88+}
89 FUZZ_TARGET_DESERIALIZE(uint160_deserialize, {
90     uint160 u160;
91     DeserializeFromFuzzingInput(buffer, u160);

maflcko commented at 12:13 pm on March 25, 2026:

Not sure. The changes in this pull are trivially correct. Whereas this suggested diff is hard to review. See #34562 (review) for a similar diff.

hodlinator commented at 12:24 pm on March 25, 2026:

this suggested diff is hard to review.

It’s not a one-liner, but I wouldn’t characterize the diff as hard to review?

The targets barely contain any branches. Running ./build_fuzz/test/fuzz/test_runner.py deserialize helps give some assurance of correctness.

See #34562 (comment) for a similar diff.

I’ve been working on an isolated PR for the change discussed in that thread for the last week. Hope to get it up for review tomorrow. That’s part of why I felt comfortable about suggesting the diff I guess, and also motivated to minimize dependencies on BasicTestingSetup.

maflcko commented at 1:40 pm on March 25, 2026:

That’s part of why I felt comfortable about suggesting the diff I guess

Ok, I see. Though, my preference would be to just wait until tomorrow and then review the unrelated changes separately, so that all reviewers can be equally comfortable about them.

The goal here is just to remove a clearly unused pointer. Restructuring the logging, or the testing context should ideally be done in a separate pull.

hodlinator commented at 2:36 pm on March 25, 2026:

The goal here is just to remove a clearly unused pointer. Restructuring the logging, or the testing context should ideally be done in a separate pull.

These fuzz targets we’re already running under MakeNoLogFileContext on master, so my diff doesn’t materially change that aspect.

I guess I can tack on a version of my diff above to my upcoming PR… but it would feel more tangential there, while here it’s more off a thorough change instead of doing the absolute minimal to silence the compiler.

Is there another semi-urgent PR depending on this one?

maflcko commented at 3:43 pm on March 25, 2026:

The goal here is just to remove a clearly unused pointer. Restructuring the logging, or the testing context should ideally be done in a separate pull.

These fuzz targets we’re already running under MakeNoLogFileContext on master, so my diff doesn’t materially change that aspect.

It literally changes that by removing (-) it. At least I see in the diff:

0-    static const auto testing_setup = MakeNoLogFileContext<>();

Is there another semi-urgent PR depending on this one?

Nothing urgent. Happy to close this one, if you think it is too controversial, no strong opinion.

hodlinator commented at 4:12 pm on March 25, 2026:

Yes, on master MakeNoLogFileContext prevents logging to a file, and in my diff the removal of the testing_setup prevents logging to a file.

I don’t think the PR is controversial in it’s current state, just half-baked, leaving detritus behind.

Is logging to the console important for these fuzz tests? Or is there some other context I’m missing?

maflcko commented at 4:49 pm on March 25, 2026:

I don’t think the PR is controversial in it’s current state, just half-baked, leaving detritus behind.

Is logging to the console important for these fuzz tests? Or is there some other context I’m missing?

I don’t like mixing behavior changes with refactors. If you want to remove the logging feature, it may be fine, but this is unrelated to this pull request, whose aim is to not change any behavior.

This is just what i’ve already said previously in #34562 (review):

Seems a bit odd to hide those changes without any mention in a commit called “test: move and simplify BOOST_CHECK ostream helpers”.

This will break DEBUG_LOG_OUT for all of those tests, which makes failures harder to debug.

I think this should be reverted, or at a minimum put into a different commit, to explain that this is a breaking change.

hodlinator commented at 6:24 pm on March 25, 2026:

I don’t like mixing behavior changes with refactors. If you want to remove the logging feature, it may be fine, but this is unrelated to this pull request, whose aim is to not change any behavior.

Okay, it boils down to wanting to make it a strict refactoring. I can respect that.

Seems a bit odd to hide those changes without any mention in a commit called “test: move and simplify BOOST_CHECK ostream helpers”.

It was good you called it out, it was a combination of that PR authors change and a patch I had posted earlier. At that time I didn’t see the integration of our logging system into the unit test execution to be an important feature, but you changed my mind.

This will break DEBUG_LOG_OUT for all of those tests, which makes failures harder to debug.

As I said in that thread, I hadn’t been aware of DEBUG_LOG_OUT.

My upcoming PR takes care to preserve DEBUG_LOG_OUT support for all unit tests using a *TestingSetup.

hodlinator commented at 8:42 pm on March 25, 2026:

My upcoming PR

#34922

maflcko commented at 8:47 am on March 26, 2026:

For reference, for the fuzz tests, it is not called DEBUG_LOG_OUT, but using the bitcoind option: FUZZ=block_deserialize ./bld-cmake/bin/fuzz -- --printtoconsole=1

hodlinator commented at 9:36 am on March 26, 2026:

Could we switch unit tests to use -printtoconsole=1 and drop DEBUG_LOG_OUT?

maflcko commented at 10:17 am on March 26, 2026:

I think that requires a NodeContext::ArgsManager, whereas DEBUG_LOG_OUT works on the global logger context.

I guess it is a bit unclear which direction to go:

Require an argsmanager in all tests (to support this -printtoconsole=1)
Add support for DEBUG_LOG_OUT to the fuzz tests

I minimally tend toward the second option, but no strong opinion.

hodlinator commented at 10:47 am on March 26, 2026:

DEBUG_LOG_OUT is implemented within G_TEST_LOG_FUN which is only added as a logging callback inside of BasicTestingSetup which also calls InitLogging() which calls SetLoggingOptions() which reads -printtoconsole=1.

During destruction of BasicTestingSetup it calls LogInstance().DisconnectTestLogger() which removes the G_TEST_LOG_FUN callback. DisableLogging() is not called, so Logger::m_print_to_console would be left on.

So it doesn’t seem like DEBUG_LOG_OUT leads to increased logging over -printtoconsole=1.

maflcko commented at 1:08 pm on March 26, 2026:

Ah, I didn’t read the code, and just went from memory. Thanks for correcting me.

Though, I guess I still don’t know which direction to go:

Require an argsmanager in all tests (to support this -printtoconsole=1)
Make DEBUG_LOG_OUT independent of the test setup context and Add support for DEBUG_LOG_OUT to the fuzz tests

Either change seems large-ish, so should come with a strong rationale.

maflcko commented at 1:10 pm on March 26, 2026:

Maybe just removing DEBUG_LOG_OUT and replacing it with the equivalent -- --printtoconsole=1 seems fine for now. If there is need for it in the future, it can trivially be added back.

hodlinator commented at 1:48 pm on March 26, 2026:

Maybe just removing DEBUG_LOG_OUT and replacing it with the equivalent -- --printtoconsole=1 seems fine for now. If there is need for it in the future, it can trivially be added back.

Done now in #34926.

hodlinator commented at 11:53 am on March 25, 2026: contributor

Reviewed fa4bbd6378fe133f4d125357fbe7bb0bfe876eb6

hebasto commented at 12:06 pm on March 25, 2026: member

Running hebasto/bitcoin-core-nightly#221.

There is another instance:

0/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/test/fuzz/mini_miner.cpp:30:21: error: variable 'g_setup' set but not used [-Werror,-Wunused-but-set-variable]
1   30 | const TestingSetup* g_setup;
2      |                     ^
31 error generated.

maflcko force-pushed on Mar 25, 2026

maflcko renamed this:
~~fuzz: Remove unused g_setup pointer in deserialize.cpp~~
fuzz: Remove unused g_setup pointers
on Mar 25, 2026

maflcko commented at 12:14 pm on March 25, 2026: member

There is another instance:

Thx, updated pull description with git grep command to find all.

hebasto commented at 12:45 pm on March 25, 2026: member

Tested fa55562d841f593f87fd2a04fe188c77184d6b36.

https://github.com/hebasto/bitcoin-core-nightly/actions/runs/23540222699:

0/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/wallet/test/fuzz/crypter.cpp:14:21: error: variable 'g_setup' set but not used [-Werror,-Wunused-but-set-variable]
1   14 | const TestingSetup* g_setup;
2      |                     ^
31 error generated.

fuzz: Remove unused g_setup pointers

These are unused and removing them avoids clang warnings like:

src/test/fuzz/deserialize.cpp:42:26: error: variable g_setup set but not used [-Werror,-Wunused-but-set-variable]

fabbfec3b0

maflcko force-pushed on Mar 25, 2026

hebasto approved

hebasto commented at 1:37 pm on March 25, 2026: member

ACK fabbfec3b00c138a28034a4f5594305d2220b9bb.

DrahtBot requested review from sedited on Mar 25, 2026

kevkevinpal commented at 3:38 pm on March 25, 2026: contributor

In your description, you mention that the grep command will output two file names, but I get three.

On master branch 2fe76ed8324af44c985b96455a05c3e8bec0a03e

0$ git grep --count  '\<g_setup' | grep ':2'
1src/test/fuzz/deserialize.cpp:2
2src/test/fuzz/mini_miner.cpp:2
3src/wallet/test/fuzz/crypter.cpp:2

I do in fact get none after fabbfec

maflcko commented at 3:44 pm on March 25, 2026: member

In your description, you mention that the grep command will output two file names, but I get three.

thx, removed outdated “two”

maflcko renamed this:
~~fuzz: Remove unused g_setup pointers~~
fuzz: [refactor] Remove unused g_setup pointers
on Mar 25, 2026

maflcko added the label Refactoring on Mar 25, 2026

hodlinator approved

hodlinator commented at 6:29 pm on March 25, 2026: contributor

ACK fabbfec3b00c138a28034a4f5594305d2220b9bb

Could clean more stuff away (https://github.com/bitcoin/bitcoin/pull/34918#discussion_r2987687080), but it’s okay to keep it a strict refactor.

fanquake merged this on Mar 26, 2026

fanquake closed this on Mar 26, 2026

maflcko deleted the branch on Mar 26, 2026

fuzz: [refactor] Remove unused g_setup pointers #34918

Reviews