fuzz: Add fuzzing harness for node eviction logic #19972

practicalswift · 2020-09-18T15:04:44Z

Add fuzzing harness for node eviction logic.

See doc/fuzzing.md for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the Bitcoin Core fuzzing corpus repo.

Happy fuzzing :)

jonatack · 2020-09-18T17:59:33Z

Concept ACK, thanks for working on this. Will review.

DrahtBot · 2020-09-19T13:23:33Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net: Make DNS lookup mockable, add fuzzing harness #19415 (net: Make DNS lookup mockable, add fuzzing harness by practicalswift)

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.

jonatack

ACK c57cabd code review, ran the fuzzer up to 10M execs, ran bitcoind grepping the net logging and observing the peer conns. Consider maybe running clang-format on the changes.

#10750687	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 631Mb L: 2188/4091 MS: 1 EraseBytes-
#10761148	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 642Mb L: 1430/4091 MS: 1 EraseBytes-
#10761655	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 642Mb L: 3142/4091 MS: 2 PersAutoDict-EraseBytes- DE: "_\xb0\x00\x8d\x8d\x8d\x8d\x8d\x8d\x8d\x8d\x8d"-
#10770237	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 642Mb L: 2961/4091 MS: 2 ChangeBinInt-EraseBytes-
#10778939	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 643Mb L: 3266/4091 MS: 2 ChangeByte-EraseBytes-

jonatack · 2020-09-20T17:44:53Z

(am running the node with this logging)

+++ b/src/net.cpp
@@ -972,6 +972,7 @@ bool CConnman::AttemptToEvictConnection()
 
     const Optional<NodeEvictionCandidate> node_to_evict = SelectNodeToEvict(vEvictionCandidates);
     if (!node_to_evict) {
+        LogPrintf("ATEC: no node to evict returned by SelectNodeToEvict()\n");
         return false;
     }
     const NodeId evicted = node_to_evict->id;
@@ -979,9 +980,12 @@ bool CConnman::AttemptToEvictConnection()
     for (CNode* pnode : vNodes) {
         if (pnode->GetId() == evicted) {
             pnode->fDisconnect = true;
+            LogPrintf("ATEC: node to evict FOUND in vEvictioncandidates and disconnected: %s\n", evicted);
             return true;
         }
     }
+    LogPrintf("ATEC: node to evict selected but not found in vEvictioncandidates: %s\n", evicted);
+
     return false;
 }

jonatack · 2020-10-20T07:49:14Z

A month has passed since reviewing, so a gentle bump for reviewers.

sipa

Concept ACK

src/net.cpp

src/test/fuzz/node_eviction.cpp

practicalswift · 2020-12-06T21:33:49Z

Marking this as draft since it depends on the changes in #20477 ("test/net: Add unit testing of node eviction logic") :)

practicalswift · 2020-12-16T12:49:23Z

Rebased on top of master now that the prerequisite #20477 has been merged.

This PR is now touches only the fuzz tests and should hopefully be easy to review :)

maflcko

Aren't the code paths already exercised through the unit test?

src/test/fuzz/node_eviction.cpp

practicalswift · 2020-12-16T13:17:15Z

Aren't the code paths already exercised through the unit test?

There are differences: the unit test clamp the eviction candidate parameters (like static_cast<int64_t>(random_context.randrange(100)) ) in order to trigger duplicate values, whereas this fuzzing harness can span the entire input space (like fuzzed_data_provider.ConsumeIntegral<int64_t>()). The latter is good for catching integer overflows and similar stuff resulting from "extreme" inputs.

Also the unit testing performs a couple of thousand test eviction rounds whereas the fuzzing test runs indefinitely :)

I think it makes sense to both unit test and fuzz test SelectNodeToEvict since that function is of extreme importance :)

maflcko · 2020-12-16T13:20:50Z

cr ACK 5a9ee08

5a9ee08 tests: Add fuzzing harness for node eviction logic (practicalswift) Pull request description: Add fuzzing harness for node eviction logic. See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets). Happy fuzzing :) ACKs for top commit: MarcoFalke: cr ACK 5a9ee08 Tree-SHA512: c2401d22134867e23dab1ba94ae7ef36fdf52aa0588fdc4705d9cb765ddf979fd775fdf153ce2359f1bc1787cf60bf0ebcd47c7aa29c672e6a253fa58cac292d

jonatack

ACK 5a9ee08

jonatack · 2020-12-25T23:48:48Z

src/test/fuzz/node_eviction.cpp

+            fuzzed_data_provider.ConsumeBool(),
+            fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
+            fuzzed_data_provider.ConsumeBool(),
+            fuzzed_data_provider.ConsumeBool(),


Following this merge I needed to update this fuzzer for #20197 and found the documentation for each data member previously added in net_tests.cpp both useful and missing here. Added it in #20197.

practicalswift changed the title ~~tests: Add fuzzing harness for node eviction logic~~ tests/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable. Sep 18, 2020

amitiuttarwar mentioned this pull request Sep 18, 2020

Add unit tests for eviction logic #19966

Closed

DrahtBot added Build system P2P labels Sep 18, 2020

practicalswift force-pushed the fuzzers-eviction-logic branch from be31739 to c57cabd Compare September 19, 2020 08:25

This was referenced Sep 19, 2020

Per-Peer Message Capture #19509

Merged

net: Make DNS lookup mockable, add fuzzing harness #19415

Merged

jonatack reviewed Sep 20, 2020

View reviewed changes

DrahtBot mentioned this pull request Sep 30, 2020

Make all of net_processing (and some of net) use std::chrono types #20044

Closed

practicalswift mentioned this pull request Oct 6, 2020

fuzz: Add fuzzing harness for TorController #19288

Merged

DrahtBot mentioned this pull request Oct 20, 2020

p2p: improve onion detection in AttemptToEvictConnection #20193

Closed

maflcko removed the Build system label Oct 20, 2020

DrahtBot mentioned this pull request Oct 20, 2020

p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197

Merged

DrahtBot mentioned this pull request Nov 4, 2020

net: Make it easier to reason about node eviction by removing unused NodeEvictionCandidate::addr (CAddress) #20302

Merged

sipa reviewed Nov 5, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/test/fuzz/node_eviction.cpp Outdated Show resolved Hide resolved

src/test/fuzz/node_eviction.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Nov 5, 2020

practicalswift force-pushed the fuzzers-eviction-logic branch 3 times, most recently from 1533813 to 0b236f9 Compare November 5, 2020 15:33

DrahtBot removed the Needs rebase label Nov 5, 2020

DrahtBot mentioned this pull request Nov 5, 2020

net: Assume that SetCommonVersion is called at most once per peer #20138

Merged

practicalswift mentioned this pull request Nov 10, 2020

test: Mock IBD in net_processing fuzzers #20332

Merged

practicalswift changed the title ~~tests/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.~~ test/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable. Nov 24, 2020

practicalswift changed the title ~~test/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.~~ fuzz/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable. Nov 24, 2020

DrahtBot mentioned this pull request Nov 24, 2020

net: Add unit testing of node eviction logic #20477

Merged

DrahtBot mentioned this pull request Dec 4, 2020

fuzz: Link all targets once #20560

Merged

practicalswift changed the title ~~fuzz/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.~~ [draft] fuzz/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable. Dec 6, 2020

DrahtBot added the Needs rebase label Dec 7, 2020

practicalswift force-pushed the fuzzers-eviction-logic branch from 0b236f9 to d041f2f Compare December 7, 2020 13:31

DrahtBot removed the Needs rebase label Dec 7, 2020

DrahtBot added the Needs rebase label Dec 15, 2020

practicalswift force-pushed the fuzzers-eviction-logic branch 2 times, most recently from c6f0bc1 to 1c03aa6 Compare December 16, 2020 11:29

DrahtBot removed the Needs rebase label Dec 16, 2020

practicalswift force-pushed the fuzzers-eviction-logic branch from 1c03aa6 to 42321f4 Compare December 16, 2020 12:47

practicalswift changed the title ~~[draft] fuzz/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.~~ fuzz: Add fuzzing harness for node eviction logic Dec 16, 2020

maflcko reviewed Dec 16, 2020

View reviewed changes

src/test/fuzz/node_eviction.cpp Outdated Show resolved Hide resolved

tests: Add fuzzing harness for node eviction logic

5a9ee08

practicalswift force-pushed the fuzzers-eviction-logic branch from 42321f4 to 5a9ee08 Compare December 16, 2020 13:01

maflcko merged commit 43fc7a5 into bitcoin:master Dec 25, 2020

jonatack reviewed Dec 25, 2020

View reviewed changes

practicalswift deleted the fuzzers-eviction-logic branch April 10, 2021 19:43

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fuzz: Add fuzzing harness for node eviction logic #19972

fuzz: Add fuzzing harness for node eviction logic #19972

Uh oh!

practicalswift commented Sep 18, 2020 •

edited

Loading

Uh oh!

jonatack commented Sep 18, 2020

Uh oh!

DrahtBot commented Sep 19, 2020 •

edited

Loading

Uh oh!

jonatack left a comment

Uh oh!

jonatack commented Sep 20, 2020

Uh oh!

jonatack commented Oct 20, 2020

Uh oh!

sipa left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Dec 6, 2020

Uh oh!

practicalswift commented Dec 16, 2020 •

edited

Loading

Uh oh!

maflcko left a comment

Uh oh!

Uh oh!

practicalswift commented Dec 16, 2020

Uh oh!

maflcko commented Dec 16, 2020

Uh oh!

jonatack left a comment

Uh oh!

jonatack Dec 25, 2020

Uh oh!

Uh oh!

fuzz: Add fuzzing harness for node eviction logic #19972

fuzz: Add fuzzing harness for node eviction logic #19972

Uh oh!

Conversation

practicalswift commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Sep 18, 2020

Uh oh!

DrahtBot commented Sep 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Sep 20, 2020

Uh oh!

jonatack commented Oct 20, 2020

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Dec 6, 2020

Uh oh!

practicalswift commented Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Dec 16, 2020

Uh oh!

maflcko commented Dec 16, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Dec 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Sep 18, 2020 •

edited

Loading

DrahtBot commented Sep 19, 2020 •

edited

Loading

practicalswift commented Dec 16, 2020 •

edited

Loading