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

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.

(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;
 }

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

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

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

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

cr ACK 5a9ee08