fuzz: Consolidate fuzzing TestingSetup initialization

[dongcarl]

Previously, the {Basic,}TestingSetup for fuzzers were set up in many ways:

1. Calling InitializeFuzzingContext, which implicitly constructs a static
   const BasicTestingSetup
2. Directly constructing a static const BasicTestingSetup in the initialize_*
   function
3. Directly constructing a static TestingSetup and reproducing the
   initialization arguments (I'm assuming because
   InitializeFuzzingContext only initializes a BasicTestingSetup)

The new, relatively-simple MakeFuzzingContext function allows us to
consolidate these methods of initialization by being flexible enough to
be used in all situations. It:

1. Is templated so that we can choose to initialize any of
   the *TestingSetup classes
2. Has sane defaults which are often used in fuzzers but are also
   easily overridable
3. Returns a unique_ptr, explicitly transferring ownership to the caller
   to deal with according to its situation

Question for fuzzing people: was it intentional that src/test/fuzz/net.cpp would directly instantiate the BasicTestingSetup and thus omit the "-nodebuglogfile" flag? Answered

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• fuzz: Fail if message type is not fuzzed #20915 (fuzz: Fail if message type is not fuzzed by MarcoFalke)
• [Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749 ([Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex by dongcarl)

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.

[practicalswift]

Concept ACK

Thanks for improving the fuzzing code! Very happy to the number of active contributors to src/test/fuzz/ grow :)

Question for fuzzing people: was it intentional that src/test/fuzz/net.cpp would directly instantiate the BasicTestingSetup and thus omit the "-nodebuglogfile" flag?

I don't think that was intentional on my part. We want -nodebuglogfile :)

[dongcarl]

Can someone help me out with this fuzzer message? Is this a real error that we caught or something benign?

https://cirrus-ci.com/task/5473797673320448?command=ci#L3550

[maflcko]

Looks like an issue with your code. Could it mean that you destroyed some object twice?

[maflcko]

ACK b4a5ccc

[maflcko]

style nit: templates are already inline, no need to specify twice

[maflcko]

Also, it looks like returning T is just fine here and should be sufficient to show that the caller owns the object that is returned:

diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
index e9c5cb5954..19523d6951 100644
--- a/src/test/fuzz/process_message.cpp
+++ b/src/test/fuzz/process_message.cpp
@@ -39,7 +39,7 @@ const TestingSetup* g_setup;
 void initialize_process_message()
 {
     static const auto testing_setup = MakeFuzzingContext<const TestingSetup>();
-    g_setup = testing_setup.get();
+    g_setup = &testing_setup;
     for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
         MineBlock(g_setup->m_node, CScript() << OP_TRUE);
     }
diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp
index 291d5df191..d8fa1028e5 100644
--- a/src/test/fuzz/process_messages.cpp
+++ b/src/test/fuzz/process_messages.cpp
@@ -24,7 +24,7 @@ const TestingSetup* g_setup;
 void initialize_process_messages()
 {
     static const auto testing_setup = MakeFuzzingContext<const TestingSetup>();
-    g_setup = testing_setup.get();
+    g_setup = &testing_setup;
     for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
         MineBlock(g_setup->m_node, CScript() << OP_TRUE);
     }
diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
index 9f1dfa44a1..68c6838a36 100644
--- a/src/test/fuzz/util.h
+++ b/src/test/fuzz/util.h
@@ -339,8 +339,8 @@ inline void FillNode(FuzzedDataProvider& fuzzed_data_provider, CNode& node, cons
     }
 }
 
-template <class T = const BasicTestingSetup>
-inline std::unique_ptr<T> MakeFuzzingContext(const std::string& chain_name = CBaseChainParams::REGTEST, const std::vector<const char*>& extra_args = {})
+template <class T = BasicTestingSetup>
+T MakeFuzzingContext(const std::string& chain_name = CBaseChainParams::REGTEST, const std::vector<const char*>& extra_args = {})
 {
     // Prepend default arguments for fuzzing
     const std::vector<const char*> arguments = Cat(
@@ -349,7 +349,7 @@ inline std::unique_ptr<T> MakeFuzzingContext(const std::string& chain_name = CBa
         },
         extra_args);
 
-    return MakeUnique<T>(chain_name, arguments);
+    return T{chain_name, arguments};
 }
 
 class FuzzedFileProvider

[dongcarl]

w/re returning T, I think I'm going to keep the unique_ptr just because in the future we might do something in MakeFuzzingContext that doesn't trigger RVO and I'm not sure about the copy semantics of our *TestingSetup classes...

[maflcko]

As you are using auto, changing from/to unique_ptr is a small diff, so can be done later easily, if needed

[maflcko]

ACK abb6fa7

[maflcko]

nit: new code can use

Suggested change

	return MakeUnique<T>(chain_name, arguments);
	return std::make_unique<T>(chain_name, arguments);

[maflcko]

Fixed in #21003