fuzz: Change LIMIT_TO_MESSAGE_TYPE from a compile-time to a run-time setting #27766

1. 
   maflcko commented at 10:20 am on May 26, 2023: member

   The process_message_${msg_type} fuzz targets have many issues:

   • In a context where each fuzz target must be a separate binary, this bloats the storage requirements by the number of message types.
   • The qa-assets repo for fuzz inputs also bloats, because each input in the type specific folder (./process_message_${msg_type}) is accompanied by a similar input in the general folder (./process_message) or a in another specific folder. The size seems to be ~3GB for the sum of all folders vs 0.3GB for the general folder.
   • Handling of different folders for each message type and one general folder for all message types (and unknown message types) is undocumented and unclear. Cross-pollination is encouraged, I guess, but who does it?
   • It is unclear if the fuzz target has any value at all, given that any bug that is found here should also be found by the process_messages fuzz target, and historically always has been? So maybe it can even be removed completely in the future?
   • (minor nit): When adding a new message type, the message type has to be added to this fuzz target as well.

   Fix all issues by turning the compile-time setting into a run-time setting, thus removing the extra executables and fuzz folders. The same approach is also taken by the rpc fuzz target.

   If someone wants to limit their fuzzing to a specific message type, they can still do it. For example,

   0LIMIT_TO_MESSAGE_TYPE=inv FUZZ=process_message ./src/test/fuzz/fuzz
   

2. 
   DrahtBot commented at 10:20 am on May 26, 2023: 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	dergoegge
   Concept ACK	brunoerg

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

3. DrahtBot added the label Tests on May 26, 2023
4. fanquake requested review from dergoegge on May 26, 2023
5. in src/test/fuzz/process_message.cpp:55 in fa0a0b8503 outdated

   59-    }
   60 
   61 void initialize_process_message()
   62 {
   63-    Assert(GetNumMsgTypes() == getAllNetMessageTypes().size()); // If this fails, add or remove the message type below
   64+    Assert(MsgTypes().size() == getAllNetMessageTypes().size()); // If this fails, add or remove the message type below (FUZZ_TARGET_MSG)
   

   ---

   ---

   

   dergoegge commented at 1:03 pm on May 26, 2023:

   I think we can just get rid of the FUZZ_TARGET_MSG macro including MsgTypes() and use getAllNetMessageTypes directly?

   ---

   

   maflcko commented at 3:18 pm on May 26, 2023:

   thx, done
6. 
   dergoegge commented at 1:03 pm on May 26, 2023: member

   Concept ACK
7. fuzz: Change LIMIT_TO_MESSAGE_TYPE from a compile-time to a run-time setting 1111c9ac97
8. maflcko force-pushed on May 26, 2023
9. dergoegge approved
10. 
    dergoegge commented at 3:29 pm on May 26, 2023: member

    ACK 1111c9ac97ca0f0afeb5df45bc0970b761c3c9ff
11. 
    brunoerg commented at 4:37 pm on May 26, 2023: contributor

    Concept ACK
12. 
    fanquake commented at 9:17 am on May 27, 2023: member

    Nice. Should help with oss-fuzz disk issues. Related to https://github.com/bitcoin-core/qa-assets/pull/131
13. fanquake merged this on May 27, 2023
14. fanquake closed this on May 27, 2023

    ---

15. 
    brunoerg commented at 12:16 pm on May 27, 2023: contributor

    post-merge ACK 1111c9ac97ca0f0afeb5df45bc0970b761c3c9ff
16. maflcko deleted the branch on May 27, 2023
17. sidhujag referenced this in commit c63ab52eb9 on May 29, 2023
18. bitcoin locked this on May 26, 2024

Contributors
maflcko DrahtBot dergoegge brunoerg fanquake

Labels
Tests