Concept ACK

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, refactor: pass uint16 CService::port as uint16 #21328 (net, refactor: pass uint16 CService::port as uint16 by jonatack)
• net: Make DNS lookup mockable, add fuzzing harness #19415 (net: Make DNS lookup mockable, add fuzzing harness by practicalswift)
• fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) #19259 (fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) 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.

Code review ACK ff13f09

@laanwj Thanks for the quick review! ❤️ Pushed an updated version addressing @MarcoFalke's feedback. Should hopefully be ready for final review.

Running the new fuzz test for a few tens of minutes produces no errors.

Reverting the fix for CVE-2017–18350 like this:

--- i/src/netbase.cpp
+++ w/src/netbase.cpp
@@ -533,13 +533,13 @@ bool Socks5(const std::string& strDest, int port, const ProxyCredentials* auth,
         case SOCKS5Atyp::DOMAINNAME:
         {
             recvr = InterruptibleRecv(pchRet3, 1, SOCKS5_RECV_TIMEOUT, sock);
             if (recvr != IntrRecvError::OK) {
                 return error("Error reading from proxy");
             }
-            int nRecv = pchRet3[0];
+            int nRecv = (char)pchRet3[0];
             recvr = InterruptibleRecv(pchRet3, nRecv, SOCKS5_RECV_TIMEOUT, sock);
             break;
         }
         default: return error("Error: malformed proxy response");
     }
     if (recvr != IntrRecvError::OK) {

crashes the test in less than a minute.

The PR description needs an update since it mentions 4 commits, but there are just 2 now.

The PR description needs an update since it mentions 4 commits, but there are just 2 now.

Ah yes I had already updated the PR title for it, but you're right that the description is out of date too.

b62b90f looks good except that the first commit does not compile:

test/fuzz/socks5.cpp:23:5: error: use of undeclared identifier 'default_socks5_recv_timeout'
    default_socks5_recv_timeout = g_socks5_recv_timeout;
    ^
test/fuzz/socks5.cpp:23:35: error: use of undeclared identifier 'g_socks5_recv_timeout'
    default_socks5_recv_timeout = g_socks5_recv_timeout;
                                  ^

(the second commit fixes this)

@vasild

b62b90f looks good except that the first commit does not compile:
…
(the second commit fixes this)

Oh, good catch!

Now addressed :)

Please re-review :)