fuzz: Call SendMessages after ProcessMessage to increase coverage #20674

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2012-fuzzSendMessages changing 2 files +8 −0

MarcoFalke commented at 2:55 PM on December 16, 2020: member

No description provided.
fuzz: Call SendMessages after ProcessMessage to increase coverage fa09f97bea
practicalswift commented at 3:12 PM on December 16, 2020: contributor

Concept ACK: more is more when it comes to coverage :)
practicalswift commented at 3:37 PM on December 16, 2020: contributor
Tested ACK fa09f97beabafaaeb59fca710760578ff1f2e8d7

Before:
```
INITED cov: 15027 ft: 49881 corp: 1258/1045Kb exec/s: 181 rss: 683Mb
```
After:
```
INITED cov: 16201 ft: 52112 corp: 1260/1038Kb exec/s: 93 rss: 669Mb
```
Net result: Increase in coverage: Decrease in execs/second.

In fuzzing we can always add more hardware to the fuzzing farm in order to increase execs/second. Better coverage on the other hand is relatively costly to obtain. Thus we want to optimise for coverage rather than execs/second in the general case.
laanwj added the label Tests on Dec 16, 2020
dhruv commented at 6:56 AM on December 19, 2020: member
tACK fa09f97

I used some additional seeds and ran fuzz tests (took care to remove any further seeds generated between runs):
```
FUZZ=process_message src/test/fuzz/fuzz qa-assets/fuzz_seed_corpus/process_message
master:    [#9987](/bitcoin-bitcoin/9987/)	INITED cov: 23996 ft: 91376 corp: 1999/59Mb exec/s: 59 rss: 863Mb
pr/20674:  [#9987](/bitcoin-bitcoin/9987/)	INITED cov: 25491 ft: 94009 corp: 1976/62Mb exec/s: 27 rss: 827Mb
```
```
FUZZ=process_messages src/test/fuzz/fuzz qa-assets/fuzz_seed_corpus/process_messages
master:   [#14819](/bitcoin-bitcoin/14819/)	INITED cov: 28055 ft: 180329 corp: 3885/119Mb exec/s: 22 rss: 657Mb
pr/20674: [#14675](/bitcoin-bitcoin/14675/)	INITED cov: 30072 ft: 183415 corp: 3584/58Mb exec/s: 35 rss: 703Mb
```
@MarcoFalke Do I understand correctly that:
- Previously, the test tried fuzz inputs to m_node.ProcessMessage(). Since some of those are valid, as coverage increases, m_node accumulates valid responses to send back to p2p_node
- This (very clever) PR takes those accumulated in-memory responses and exercises the code to flush them via SendMessages(p2p_node)
If that's all correct, I think there might be more coverage to gain if we can invoke p2p_node.ProcessMessage() using those in-memory responses(since they are probably valid p2p messages). I am not sure this is possible without a significant refactor. If it is useful, I'd be interested in helping extend this with your guidance.
in src/test/fuzz/process_message.cpp:78 in fa09f97bea
```
  74 | @@ -75,6 +75,10 @@ void fuzz_target(const std::vector<uint8_t>& buffer, const std::string& LIMIT_TO
  75 |                                                  GetTime<std::chrono::microseconds>(), std::atomic<bool>{false});
  76 |      } catch (const std::ios_base::failure&) {
  77 |      }
  78 | +    {
```
dhruv commented at 4:39 PM on December 19, 2020:

Do I understand correctly that we do not need try { ... } catch (const std::ios_base::failure&) {} here because the inputs to SendMessages() have effectively been filtered for "sane inputs" by ProcessMessage()?

sipa commented at 7:00 AM on December 27, 2020:

std::ios_base::failure is thrown by the serialization framework when deserialization fails. SendMessages AFAIK doesn't deserialize anything, it just acts on already parsed data. So I wouldn't say it's just been "filtered" - it has been completely processed already, and the result of that processing may trigger responses in SendMessages.

dhruv commented at 4:06 PM on December 27, 2020:

That makes sense. Thanks, @sipa !
Crypt-iQ commented at 5:23 PM on December 26, 2020: contributor

cr ACK fa09f97beabafaaeb59fca710760578ff1f2e8d7
sipa commented at 7:01 AM on December 27, 2020: member

utACK fa09f97beabafaaeb59fca710760578ff1f2e8d7
fanquake merged this on Dec 27, 2020
fanquake closed this on Dec 27, 2020
sidhujag referenced this in commit 6c610b951a on Dec 27, 2020
MarcoFalke deleted the branch on Dec 29, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels