fuzz: Add fuzzing harness for node eviction logic #19972

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:fuzzers-eviction-logic changing 2 files +45 −0
  1. practicalswift commented at 3:04 PM on September 18, 2020: contributor

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

  2. practicalswift renamed this:
    tests: Add fuzzing harness for node eviction logic
    tests/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.
    on Sep 18, 2020
  3. amitiuttarwar cross-referenced this on Sep 18, 2020 from issue Add unit tests for eviction logic by practicalswift
  4. DrahtBot added the label Build system on Sep 18, 2020
  5. DrahtBot added the label P2P on Sep 18, 2020
  6. jonatack commented at 5:59 PM on September 18, 2020: contributor

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

  7. practicalswift force-pushed on Sep 19, 2020
  8. DrahtBot commented at 1:23 PM on September 19, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #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.

  9. DrahtBot cross-referenced this on Sep 19, 2020 from issue Per-Peer Message Capture by troygiorshev
  10. DrahtBot cross-referenced this on Sep 19, 2020 from issue net: Make DNS lookup mockable, add fuzzing harness by practicalswift
  11. jonatack commented at 4:53 PM on September 20, 2020: contributor

    ACK c57cabdea44cab2a9dd2f0443f7a6b28cb41e7b4 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](/github-metadata-backup-bitcoin-bitcoin/10750687/)	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 631Mb L: 2188/4091 MS: 1 EraseBytes-
[#10761148](/github-metadata-backup-bitcoin-bitcoin/10761148/)	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 642Mb L: 1430/4091 MS: 1 EraseBytes-
[#10761655](/github-metadata-backup-bitcoin-bitcoin/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](/github-metadata-backup-bitcoin-bitcoin/10770237/)	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 642Mb L: 2961/4091 MS: 2 ChangeBinInt-EraseBytes-
[#10778939](/github-metadata-backup-bitcoin-bitcoin/10778939/)	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 643Mb L: 3266/4091 MS: 2 ChangeByte-EraseBytes-
  12. jonatack commented at 5:44 PM on September 20, 2020: contributor

    (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;
 }
  13. DrahtBot cross-referenced this on Sep 30, 2020 from issue Make all of net_processing (and some of net) use std::chrono types by sipa
  14. practicalswift cross-referenced this on Oct 6, 2020 from issue fuzz: Add fuzzing harness for TorController by practicalswift
  15. DrahtBot cross-referenced this on Oct 20, 2020 from issue p2p: improve onion detection in AttemptToEvictConnection by jonatack
  16. jonatack commented at 7:49 AM on October 20, 2020: contributor

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

  17. MarcoFalke removed the label Build system on Oct 20, 2020
  18. DrahtBot cross-referenced this on Oct 20, 2020 from issue p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage by jonatack
  19. DrahtBot cross-referenced this on Nov 4, 2020 from issue net: Make it easier to reason about node eviction by removing unused NodeEvictionCandidate::addr (CAddress) by practicalswift
  20. in src/net.cpp:879 in 818dd2a04d outdated 
     911 | -                                               node->m_prefer_evict, node->addr.IsLocal()};
 912 | -            vEvictionCandidates.push_back(candidate);
 913 | -        }
 914 | -    }
 915 | -
 916 | +NODISCARD Optional<NodeEvictionCandidate> SelectNodeToEvict(std::vector<NodeEvictionCandidate> vEvictionCandidates) {
    sipa commented at 1:44 AM on November 5, 2020:

    Nit: brace on the next line.

    sipa commented at 1:47 AM on November 5, 2020:

    This will cause an unnecessary copy of the vEvictionCandidates vector when invoking.

    As the caller doesn't need its version anymore after the call, it's possible to change this parameter to std::vector<NodeEvictionCandidate>&&, and call it with SelectNodeToEvict(std::move(vEvictionCandidates)).

    practicalswift commented at 3:26 PM on November 5, 2020:

    Feedback addressed.

    Also reduced the scope of std::vector<NodeEvictionCandidate> vEvictionCandidates to make reasoning about the life-time easier.

  21. in src/test/fuzz/node_eviction.cpp:18 in c57cabdea4 outdated 
      13 | +#include <cassert>
  14 | +#include <cstdint>
  15 | +#include <optional>
  16 | +#include <vector>
  17 | +
  18 | +NODISCARD Optional<NodeEvictionCandidate> SelectNodeToEvict(std::vector<NodeEvictionCandidate> vEvictionCandidates);
    sipa commented at 1:51 AM on November 5, 2020:

    Since this file is already including net.h, I'd prefer seeing this declaration moved there, rather than forward declaring it (as it risks going out of sync with the implementation here).

    practicalswift commented at 3:19 PM on November 5, 2020:

    Makes sense. Feedback addressed. Thanks!

  22. in src/test/fuzz/node_eviction.cpp:37 in c57cabdea4 outdated 
      32 | +    if (node_to_evict) {
  33 | +        assert(std::any_of(eviction_candidates.begin(), eviction_candidates.end(), [&node_to_evict](const NodeEvictionCandidate& eviction_candidate) { return node_to_evict->id == eviction_candidate.id; }));
  34 | +    }
  35 | +    // eviction_candidates.empty() implies !node_to_evict, but !node_to_evict
  36 | +    // does not imply eviction_candidates.empty()).
  37 | +    if (eviction_candidates.empty()) {
    sipa commented at 1:53 AM on November 5, 2020:

    I believe this test is redundant; if eviction_candidates is empty but node_to_evict isn't, the std::any_of call above must fail.

    practicalswift commented at 3:20 PM on November 5, 2020:

    Good catch. Feedback addressed. Thanks!

  23. sipa commented at 1:53 AM on November 5, 2020: member

    Concept ACK

  24. DrahtBot added the label Needs rebase on Nov 5, 2020
  25. practicalswift force-pushed on Nov 5, 2020
  26. practicalswift force-pushed on Nov 5, 2020
  27. practicalswift force-pushed on Nov 5, 2020
  28. DrahtBot removed the label Needs rebase on Nov 5, 2020
  29. DrahtBot cross-referenced this on Nov 5, 2020 from issue net: Assume that SetCommonVersion is called at most once per peer by MarcoFalke
  30. practicalswift cross-referenced this on Nov 10, 2020 from issue test: Mock IBD in net_processing fuzzers by MarcoFalke
  31. practicalswift renamed this:
    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.
    on Nov 24, 2020
  32. practicalswift renamed this:
    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.
    on Nov 24, 2020
  33. DrahtBot cross-referenced this on Nov 24, 2020 from issue net: Add unit testing of node eviction logic by practicalswift
  34. DrahtBot cross-referenced this on Dec 4, 2020 from issue fuzz: Link all targets once by MarcoFalke
  35. practicalswift renamed this:
    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.
    on Dec 6, 2020
  36. practicalswift commented at 9:33 PM on December 6, 2020: contributor

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

  37. DrahtBot added the label Needs rebase on Dec 7, 2020
  38. practicalswift force-pushed on Dec 7, 2020
  39. DrahtBot removed the label Needs rebase on Dec 7, 2020
  40. DrahtBot added the label Needs rebase on Dec 15, 2020
  41. practicalswift force-pushed on Dec 16, 2020
  42. practicalswift force-pushed on Dec 16, 2020
  43. DrahtBot removed the label Needs rebase on Dec 16, 2020
  44. practicalswift force-pushed on Dec 16, 2020
  45. practicalswift renamed this:
    [draft] fuzz/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.
    fuzz: Add fuzzing harness for node eviction logic
    on Dec 16, 2020
  46. practicalswift commented at 12:49 PM on December 16, 2020: contributor

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

  47. in src/test/fuzz/node_eviction.cpp:23 in 42321f469f outdated 
      18 | +FUZZ_TARGET(node_eviction)
  19 | +{
  20 | +    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
  21 | +    std::vector<NodeEvictionCandidate> eviction_candidates;
  22 | +    while (fuzzed_data_provider.ConsumeBool()) {
  23 | +        eviction_candidates.push_back({fuzzed_data_provider.ConsumeIntegral<NodeId>(), fuzzed_data_provider.ConsumeIntegral<int64_t>(), fuzzed_data_provider.ConsumeIntegral<int64_t>(), fuzzed_data_provider.ConsumeIntegral<int64_t>(), fuzzed_data_provider.ConsumeIntegral<int64_t>(), fuzzed_data_provider.ConsumeBool(), fuzzed_data_provider.ConsumeBool(), fuzzed_data_provider.ConsumeBool(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeBool(), fuzzed_data_provider.ConsumeBool()});
    MarcoFalke commented at 12:55 PM on December 16, 2020:

    For the sanity of future devs, please add a trailing comma

    practicalswift commented at 1:01 PM on December 16, 2020:

    Thanks! Fixed!

  48. MarcoFalke commented at 12:57 PM on December 16, 2020: member

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

  49. tests: Add fuzzing harness for node eviction logic 5a9ee0869b
  50. practicalswift force-pushed on Dec 16, 2020
  51. practicalswift commented at 1:17 PM on December 16, 2020: contributor

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

  52. MarcoFalke commented at 1:20 PM on December 16, 2020: member

    cr ACK 5a9ee0869b0b722ebfcdaabaefba6376522b2eeb

  53. MarcoFalke merged this on Dec 25, 2020
  54. MarcoFalke closed this on Dec 25, 2020
  55. sidhujag referenced this in commit 92e5b1d1c6 on Dec 25, 2020
  56. in src/test/fuzz/node_eviction.cpp:34 in 5a9ee0869b 
      29 | +            fuzzed_data_provider.ConsumeBool(),
  30 | +            fuzzed_data_provider.ConsumeBool(),
  31 | +            fuzzed_data_provider.ConsumeBool(),
  32 | +            fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
  33 | +            fuzzed_data_provider.ConsumeBool(),
  34 | +            fuzzed_data_provider.ConsumeBool(),
    jonatack commented at 11:48 PM on December 25, 2020:

    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.

  57. jonatack commented at 11:50 PM on December 25, 2020: contributor

    ACK 5a9ee0869b0b722ebfcdaabaefba6376522b2eeb

  58. practicalswift deleted the branch on Apr 10, 2021
  59. bitcoin locked this on Aug 16, 2022
Contributors
practicalswiftjonatackDrahtBotsipaMarcoFalke
Labels
P2P
Linked (view graph)
#19288 fuzz: Add fuzzing harness for TorController#19966 Add unit tests for eviction logic#20332 test: Mock IBD in net_processing fuzzers
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: 2026-05-20 06:54 UTC