fuzz: Consolidate fuzzing TestingSetup initialization #20946

pull dongcarl wants to merge 2 commits into bitcoin:master from dongcarl:2021-01-make-fuzzing-ctx changing 12 files +26 −29
  1. dongcarl commented at 9:03 pm on January 15, 2021: member
     0Previously, the {Basic,}TestingSetup for fuzzers were set up in many ways:
     1
     21. Calling InitializeFuzzingContext, which implicitly constructs a static
     3   const BasicTestingSetup
     42. Directly constructing a static const BasicTestingSetup in the initialize_*
     5   function
     63. Directly constructing a static TestingSetup and reproducing the
     7   initialization arguments (I'm assuming because
     8   InitializeFuzzingContext only initializes a BasicTestingSetup)
     9
    10The new, relatively-simple MakeFuzzingContext function allows us to
    11consolidate these methods of initialization by being flexible enough to
    12be used in all situations. It:
    13
    141. Is templated so that we can choose to initialize any of
    15   the *TestingSetup classes
    162. Has sane defaults which are often used in fuzzers but are also
    17   easily overridable
    183. Returns a unique_ptr, explicitly transferring ownership to the caller
    19   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

  2. DrahtBot added the label Tests on Jan 15, 2021
  3. DrahtBot commented at 2:53 am on January 16, 2021: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #20915 (fuzz: Fail if message type is not fuzzed by MarcoFalke)
    • #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.

  4. practicalswift commented at 9:59 am on January 16, 2021: contributor

    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 :)

  5. dongcarl commented at 0:03 am on January 18, 2021: member

    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

  6. MarcoFalke commented at 6:31 am on January 18, 2021: member
    Looks like an issue with your code. Could it mean that you destroyed some object twice?
  7. in src/test/fuzz/coins_view.cpp:39 in 827b5df366 outdated
    35@@ -36,9 +36,7 @@ bool operator==(const Coin& a, const Coin& b)
    36 
    37 void initialize_coins_view()
    38 {
    39-    static const ECCVerifyHandle ecc_verify_handle;
    40-    ECC_Start();
    41-    SelectParams(CBaseChainParams::REGTEST);
    42+    static const auto testing_setup = MakeFuzzingContext<const TestingSetup>();
    


    MarcoFalke commented at 6:43 am on January 18, 2021:
    why is it needed to spin up more than just ecc and params?

    dongcarl commented at 11:15 pm on January 20, 2021:

    Happy to delay this change, but GetUTXOStats calls LookupBlockIndex which asks ChainstateManager for its BlockManager. After g_chainman is de-globalized, we will no longer have an “implicitly-constructed-by-virtue-of-being-member-of-a-global” m_blockman, so we will need to run our initialization codepath at least until we have populated NodeContext::chainman.

    However, it is not absolutely necessary right now, so I’d be happy to delay this change if you want to!


    MarcoFalke commented at 8:57 am on January 21, 2021:
    Thanks, makes sense.
  8. dongcarl force-pushed on Jan 20, 2021
  9. in src/test/fuzz/util.h:343 in ed7f845000 outdated
    338@@ -338,9 +339,17 @@ inline void FillNode(FuzzedDataProvider& fuzzed_data_provider, CNode& node, cons
    339     }
    340 }
    341 
    342-inline void InitializeFuzzingContext(const std::string& chain_name = CBaseChainParams::REGTEST)
    343+template <class T = const BasicTestingSetup>
    344+inline std::unique_ptr<T> MakeFuzzingContext(const std::string& chain_name = CBaseChainParams::REGTEST, const std::vector<const char*>& extra_args = {})
    


    MarcoFalke commented at 8:30 am on January 21, 2021:
    style nit: templates are already inline, no need to specify twice

    MarcoFalke commented at 8:53 am on January 21, 2021:

    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:

     0diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
     1index e9c5cb5954..19523d6951 100644
     2--- a/src/test/fuzz/process_message.cpp
     3+++ b/src/test/fuzz/process_message.cpp
     4@@ -39,7 +39,7 @@ const TestingSetup* g_setup;
     5 void initialize_process_message()
     6 {
     7     static const auto testing_setup = MakeFuzzingContext<const TestingSetup>();
     8-    g_setup = testing_setup.get();
     9+    g_setup = &testing_setup;
    10     for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
    11         MineBlock(g_setup->m_node, CScript() << OP_TRUE);
    12     }
    13diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp
    14index 291d5df191..d8fa1028e5 100644
    15--- a/src/test/fuzz/process_messages.cpp
    16+++ b/src/test/fuzz/process_messages.cpp
    17@@ -24,7 +24,7 @@ const TestingSetup* g_setup;
    18 void initialize_process_messages()
    19 {
    20     static const auto testing_setup = MakeFuzzingContext<const TestingSetup>();
    21-    g_setup = testing_setup.get();
    22+    g_setup = &testing_setup;
    23     for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
    24         MineBlock(g_setup->m_node, CScript() << OP_TRUE);
    25     }
    26diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
    27index 9f1dfa44a1..68c6838a36 100644
    28--- a/src/test/fuzz/util.h
    29+++ b/src/test/fuzz/util.h
    30@@ -339,8 +339,8 @@ inline void FillNode(FuzzedDataProvider& fuzzed_data_provider, CNode& node, cons
    31     }
    32 }
    33 
    34-template <class T = const BasicTestingSetup>
    35-inline std::unique_ptr<T> MakeFuzzingContext(const std::string& chain_name = CBaseChainParams::REGTEST, const std::vector<const char*>& extra_args = {})
    36+template <class T = BasicTestingSetup>
    37+T MakeFuzzingContext(const std::string& chain_name = CBaseChainParams::REGTEST, const std::vector<const char*>& extra_args = {})
    38 {
    39     // Prepend default arguments for fuzzing
    40     const std::vector<const char*> arguments = Cat(
    41@@ -349,7 +349,7 @@ inline std::unique_ptr<T> MakeFuzzingContext(const std::string& chain_name = CBa
    42         },
    43         extra_args);
    44 
    45-    return MakeUnique<T>(chain_name, arguments);
    46+    return T{chain_name, arguments};
    47 }
    48 
    49 class FuzzedFileProvider
    

    dongcarl commented at 2:30 pm on January 21, 2021:
    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…

    MarcoFalke commented at 3:00 pm on January 21, 2021:
    As you are using auto, changing from/to unique_ptr is a small diff, so can be done later easily, if needed
  10. MarcoFalke approved
  11. MarcoFalke commented at 8:56 am on January 21, 2021: member
    ACK b4a5ccc6e94f16c31e267c91c5de3dfed7512c67
  12. fuzz: Consolidate fuzzing TestingSetup initialization
    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
    713314abfa
  13. fuzz: Initialize a full TestingSetup where appropriate
    A full TestingSetup is required for both coins_view and
    load_external_block_file as they interact with the active chainstate.
    abb6fa7285
  14. dongcarl force-pushed on Jan 21, 2021
  15. MarcoFalke commented at 3:00 pm on January 21, 2021: member
    ACK abb6fa728598c4cc8874eae1c3c5e587e36424cd
  16. in src/test/fuzz/util.h:352 in abb6fa7285
    349+        {
    350+            "-nodebuglogfile",
    351+        },
    352+        extra_args);
    353+
    354+    return MakeUnique<T>(chain_name, arguments);
    


    MarcoFalke commented at 3:01 pm on January 21, 2021:

    nit: new code can use

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

    MarcoFalke commented at 10:44 am on January 25, 2021:
    Fixed in #21003
  17. MarcoFalke merged this on Jan 21, 2021
  18. MarcoFalke closed this on Jan 21, 2021

  19. sidhujag referenced this in commit 4c028b5407 on Jan 21, 2021
  20. fanquake referenced this in commit 83bdbbd300 on Mar 4, 2021
  21. sidhujag referenced this in commit 0ac2c0e46a on Mar 4, 2021
  22. MarcoFalke referenced this in commit e0bc27a14c on Mar 12, 2021
  23. Fabcien referenced this in commit c5004dcda2 on Jun 29, 2022
  24. Fabcien referenced this in commit 4f0ff1e2f9 on Jun 29, 2022
  25. DrahtBot locked this on Aug 16, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me