Split CConnman #30988

1bfc1ca9b6b68c6356ffc23ecf01e417152ade95

Why do CNode .addr and .addrBind still need to be CAddress? maybe it makes some sense for .addr if that’s where we also store the service flags for easier gossiping, but why would the local bind address need anything besides an IP and port?

CNode::addr needs to be CAddress because at least PeerManagerImpl::PushNodeVersion() is using addr.nServices.

CNode::addrBind indeed does not need to be CAddress. I changed it to CService.

41c87ddb3d7a18d2c0fa7eccfbde57e9d6e898c2

Maybe these are expected to be set by callers in future commits, so I’m just leaving myself a note to look for SO_KEEPALIVE and TCP_NODELAY

Ok I see NODELAY set by ConnMan in EventNewConnectionAccepted in b94f9d338fd0d3b47a4ea6165ce9cde48f3b19bc

I’m guessing we dont use KEEPALIVE for bitcoin p2p because we have out own ping/pong protocol

5d4920f630417ceeee1c79304faac6803795cd64

I don’t really have any better suggestion for this but the term “node” is now a higher-level ConnMan context, for HTTP and StratumV2 I’d probably refer to them as “client”.

1b05e1d4ba55a42ba74026b68fa4e616b973e06d: maybe ConnectionId?

And then in net.h use typedef ConnectionId NodeId;

https://github.com/Sjors/bitcoin/commit/f33049b45b7013022ed4c75c0bc52d878fe15fd5

Most of the churn would be in sockman.{h,cpp}, so that seems acceptable.

That said, I don’t think it’s very important.

Changed. This is now SockMan::Id and in net.h: using NodeId = SockMan::Id;. In SockMan comments change the word “node” to “connection”. This caused a lot of mechanical renames within this PR, but it looks better now, thanks!

SockMan::GetNewNodeId() renamed to SockMan::GetNewId()

SockMan::EventIOLoopCompletedForNode() renamed to SockMan::EventIOLoopCompletedForOne()

SockMan::EventIOLoopCompletedForAllPeers() renamed to SockMan::EventIOLoopCompletedForAll()

SockMan::ConnectAndMakeNodeId() renamed to SockMan::ConnectAndMakeId().

Will be in next push.

b94f9d338fd0d3b47a4ea6165ce9cde48f3b19bc

Reminding myself to check for a test that covers a child class with i2p true but no Event listener

b94f9d338fd0d3b47a4ea6165ce9cde48f3b19bc

Seems weird to me to move the unique pointer to a connected socket out of sockman, implying that sockman itself doesn’t own the connections. Peeking ahead at future commits though I think this gets resolved.

b96beb27d2b3d06a45644c537c114a08f0ccd285

Love this, great optimization and makes a lot of sense.

b96beb27d2b3d06a45644c537c114a08f0ccd285

Why not use GetNewNodeId() here? (and below) Don’t you have a Connman with its own m_next_node_id?

bb5b91d430026c4826a2107c8c1a311518cc97ce

nit s/temporary/temporarily

and above

50cb52470ea6cc4de5a5e5260b8f308353942ec0

This is great, can be used for timed actions (like locking the wallet) we used to depend on libevent for

50cb52470ea6cc4de5a5e5260b8f308353942ec0

Interesting that the parent class handles accepting connections but the child is expected to handle disconnections, and inside the utility event loop function. Of course only the derived class would know when disconnection is appropriate, I wonder if anything it makes sense just to add a comment in sockman.h that EventIOLoopCompletedForAllPeers() is expected to handle that, erasing items from m_nodes

Good observation. I guess this is the nature of things: accepting new connections is driven (initiated) by low level socket event when a new connection arrives, while disconnecting can be low level driven or high level driven - e.g. misbehaving peer at the application layer.

I extended the comment of EventIOLoopCompletedForAllPeers() a little bit:

0    -+     * Can be used to execute periodic tasks for all nodes.                   
1    ++     * Can be used to execute periodic tasks for all nodes, like disconnecting
2    ++     * nodes due to higher level logic.           

3bb0145514978092ae966e30693cf619e4034837

the else {error} looked backwards to me here at first, might be more clear to use == 0 instead of ! but i dunno what the official style is for this.

I don’t think we have an official recommendation. My personal preference is to not treat integers as booleans: use if (x == 0) instead of if (!x) when x is an integer. But I am leaving this as it is because it is like that in master and this PR only moves it from net.cpp to sockman.cpp verbatim. I hope it is easier to reviewers to just check that a blob of code is moved without having to validate its correctness since that same code is already in master. This helps with such moves:

~/.gitconfig:

0[diff]  
1        colorMoved = dimmed-zebra
2        colorMovedWS = allow-indentation-change

3bb0145514978092ae966e30693cf619e4034837

Would this make more sense as a direct method of Sock ? Especially now that it returns the simpler CService instead of CAddress

3bb0145514978092ae966e30693cf619e4034837

This is great. Won’t be needed immediately for HTTP or StratumV2 but means we can more easily remove libevent from bitcoin-cli !

c133a634d10e172652ebbf05ca114037c8e551b9

I know this is for testing only, but what happens if there is a NodeId collision (An existing node is inserted with a duplicate ID, replacing a connected node in sockman)?

c133a634d10e172652ebbf05ca114037c8e551b9

You might want to add another comment here - when I was trying to handle socks myself I found this a bit weird and wrote:

0    // WaitMany() mine as well be a static function, the context
1    // of the first Sock in the vector is not relevant.

Must have been just inertia. The caller of the newly added SendBytes() has an argument that is already Span from elsewhere in the code, coming from here:

https://github.com/bitcoin/bitcoin/blob/c1252b14d714448295200a595086dd3e78b46c8f/src/net.h#L307-L311

Now I changed the argument of the newly added SendBytes() to std::span, looks better and the Span variable is accepted. Thanks!

wen s/Span/std::span/ all over the place?

wen s/Span/std::span/ all over the place?

now: https://github.com/bitcoin/bitcoin/pull/31519

4c23aacc2b82083f96255963f543dc35fff28334: I’m inclined to say “listening on %s”

As in: we bind to a port in order to listen on it.

The linux manual for listen also uses “on”, as in “listen for connections on a socket” https://man7.org/linux/man-pages/man2/listen.2.html

Yes, good observation! Indeed the flip will do nothing on the invalid 0.0.0.0 addr_accepted.

The code in master is also a bit odd that it continues if addr.SetSockAddr() fails in CConnman::AcceptConnection() and it will pass the default constructed, invalid 0.0.0.0 address to CreateNodeFromAcceptedSocket(). I didn’t change that in this PR as it would be a functional change that is not needed for this PR.

14fcef6b0d1d1fa9395f9af2bafbf3de63d14ac2: I find this confusing.

I think the problem starts with the strange naming of ThreadI2PAccept, instead of ThreadI2PHandler akin to ThreadSocketHandler.

Anyway, what that thread seems to do is, in a loop, try to Listen on the socket and once listening, Accept( new connections.

Whenever listening fails or succeeds it calls EventI2PListen which then calls AddLocal / RemoveLocal to keep the announced public addresses up to date.

With that in mind, I would call this new method EventI2PConnectivity and rename success to either connected or listening. Then the rest makes sense.

ThreadI2PAccept vs ThreadI2PHandler. The thing is that this thread is only accepting connections. It does not do anything about I2P connections that are already established. ThreadSocketHandler does both accepting new connections and doing send/recv on existent connections.

Yes, your observation is correct. Listening can fail temporary, e.g. if the I2P router daemon is restarted. This is unlike the TCP listening where once bind() and listen() succeed, then we are listening until we decide to close the socket. Tor is like I2P - it can fail to listen for a while if the Tor daemon is restarted. Our Tor code has the deficiency that it does not handle that, so it would act as if we are listening (e.g. advertising our Tor address) even if we are not listening.

Current EventI2PListen vs EventI2PConnectivity - I think “Connectivity” would be too broad because this method is only called in the event where we start or stop listening for incoming I2P connections. It is not related to whether we have existent I2P connections or to our ability to make new outgoing I2P connections which “Connectivity” would imply.

21c5e05619c8a6eb736bb1c61725f4b5f669ffb4: maybe use [_, node] in places where you don’t need the id.

It’s a bit unfortunate that you can’t (?) directly loop over all T in std::unordered_map<Key, T>. Since most of the time we don’t need Key (id) here.

Hmm, you probably mean ShouldTryToSend is false, right? And then the same applies to receiving. Amended this:

0-                recvSet = it->second.occurred & Sock::RECV;
1-                sendSet = it->second.occurred & Sock::SEND;
2+                recvSet = it->second.occurred & Sock::RECV; // Sock::RECV could only be set if ShouldTryToRecv() has returned true in GenerateWaitSockets().
3+                sendSet = it->second.occurred & Sock::SEND; // Sock::SEND could only be set if ShouldTryToSend() has returned true in GenerateWaitSockets().

I might be misunderstanding cancel_recv, in my HTTP server I set it to true when I’m planning on disconnecting a node like if there was an error sending data or if "Connection: close" is set. However this causes weird behavior where new connections get hung up forever. In particular, interface_rpc.py stalls on the ultimate node shutdown and eventually times out after 120 seconds of rpc stop not working. When I set cancel_recv=false everywhere, i don’t have this issue. (Still a lot of other issues to clean up in this branch!)

https://github.com/pinheadmz/bitcoin/commit/df1965a4e97ce7f3df067df9b3dcc2bbd8e6e15b

@param[out] cancel_recv Should always be set upon return

I think the problem was that HTTPServer::EventReadyToSend() could end up not setting cancel_recv in which case it remains uninitialized, containing a “random” value from the stack at the caller (which was probably true, so it canceled all receives from the client, never receiving anything causing an infinite loop):

0              bool cancel_recv;
1  
2              EventReadyToSend(node_id, cancel_recv);
3  
4              if (cancel_recv) {
5                  recv_ready = false;
6              }

Valgrind or memory sanitizer should complain about this code reading uninitialized value in if (cancel_recv).

The below fixes it, remove the last commit from your branch and move the setting of cancel_recv from the bottom to the top of the method. So that cancel_recv is set in all code paths inside HTTPServer::EventReadyToSend().

 0--- i/src/httpserver.cpp
 1+++ w/src/httpserver.cpp
 2@@ -1123,12 +1123,15 @@ bool HTTPServer::EventNewConnectionAccepted(NodeId node_id,
 3     m_connected_clients.emplace(client->m_node_id, std::move(client));
 4     return true;
 5 }
 6 
 7 void HTTPServer::EventReadyToSend(NodeId node_id, bool& cancel_recv)
 8 {
 9+    // Next attempt to receive data from this node is permitted
10+    cancel_recv = false;
11+
12     // Get the HTTPClient
13     auto client{WITH_LOCK(m_clients_mutex, return GetClientById(node_id);)};
14     if (client == nullptr) {
15         return;
16     }
17 
18@@ -1149,26 +1152,26 @@ void HTTPServer::EventReadyToSend(NodeId node_id, bool& cancel_recv)
19             LogDebug(
20                 BCLog::HTTP,
21                 "Error sending HTTP response headers to client %s (id=%lld): %s\n",
22                 client->m_origin,
23                 client->m_node_id,
24                 err);
25-            cancel_recv = false;
26+            cancel_recv = true;
27             client->m_disconnect = true;
28             return;
29         }
30 
31         ssize_t body_bytes_sent = SendBytes(node_id, MakeUCharSpan(res.m_body), /*will_send_more=*/true, err);
32         if (body_bytes_sent < 0) {
33             LogDebug(
34                 BCLog::HTTP,
35                 "Error sending HTTP response body to client %s (id=%lld): %s\n",
36                 client->m_origin,
37                 client->m_node_id,
38                 err);
39-            cancel_recv = false;
40+            cancel_recv = true;
41             client->m_disconnect = true;
42             return;
43         }
44 
45         LogDebug(
46             BCLog::HTTP,
47@@ -1176,20 +1179,17 @@ void HTTPServer::EventReadyToSend(NodeId node_id, bool& cancel_recv)
48             hdr_bytes_sent + body_bytes_sent,
49             client->m_origin,
50             client->m_node_id);
51 
52         // Our work is done here
53         if (!res.m_keep_alive) {
54-            cancel_recv = false;
55+            cancel_recv = true;
56             client->m_disconnect = true;
57             return;
58         }
59     }
60-
61-    // Next attempt to receive data from this node is permitted
62-    cancel_recv = false;
63 }
64 
65 void HTTPServer::EventGotData(NodeId node_id, const uint8_t* data, size_t n)
66 {
67     // Get the HTTPClient
68     auto client{WITH_LOCK(m_clients_mutex, return GetClientById(node_id);)};

strError only appears in the comment, and “error message” should probably be snake_case:

0     * [@param](/bitcoin-bitcoin/contributor/param/)[out] err_msg Error string if an error occurs.
1     * [@retval](/bitcoin-bitcoin/contributor/retval/) true Success.
2     * [@retval](/bitcoin-bitcoin/contributor/retval/) false Failure, `err_msg` will be set.
3     */
4    bool BindAndStartListening(const CService& to, bilingual_str& err_msg);

Do we ever want to remove individual entries from m_listen_permissions upon disconnect of single peers, long before .clear()?

Same goes for m_listen. I’m not claiming vhListenSocket was any better before this PR.

Seems like .emplace() will not replace existing entries. In the off chance that someone takes a peer offline and then restarts it with other permissions, would it not be better to do this?

0    m_listen_permissions[addr] = permissions;

addr is our listening address. We can’t listen on the same address two times:

If I put this in my bitcoind.conf:

0bind=127.0.0.1:20001
1bind=127.0.0.1:20002
2bind=127.0.0.1:20003
3bind=127.0.0.1:20003

I get:

02025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20001
12025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20002
22025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20003
32025-02-07T05:39:56Z [net:error] Cannot bind to 127.0.0.1:20003: Address already in use (48) (Bitcoin Core already running?)

and startup is aborted.

I could add Assume() or return an error from here if insertion fails due to an already existent entry?

In fd81820214e695ba228a954506397c3d781fe3fe: nit: Possible less mutating simplification (assumes NetPermissionFlags::None == 0):

0-     NetPermissionFlags permission_flags = NetPermissionFlags::None;
1-     auto it{m_listen_permissions.find(addr_bind)};
2-     if (it != m_listen_permissions.end()) {
3-         NetPermissions::AddFlag(permission_flags, it->second);
4-     }
5+     auto it{m_listen_permissions.find(addr_bind)};
6+     NetPermissionFlags permission_flags = it != m_listen_permissions.end() ?
7+         it->second : NetPermissionFlags::None;

nit: The comment adds no information.

0    std::vector<std::shared_ptr<Sock>> m_listen;

Same for m_unused_i2p_sessions_mutex.

nit: Might have a more precise name since we are releasing references to shared_ptrs?

0void SockMan::ReleaseSockets()

nit: Sufficiently critical for a generic error?

0        LogError("%s", strError.original);

(Can also omit trailing newline here and in other added/modified log lines).

According to developer notes, LogError is for:

severe problems that require the node (or a subsystem) to shut down entirely

And IIUC we indeed shut down here.

Problem: " (<CLIENT_NAME> already running?)"-string is untranslated.

Would suggest bringing back old strings to reduce translation churn, or ensuring the new version is fully translated. Old strings:

0        if (err == WSAEADDRINUSE) {
1            err_msg = strprintf(_("Unable to bind to %s on this computer. %s is probably already running."), to.ToStringAddrPort(), CLIENT_NAME);
2        } else {
3            err_msg = strprintf(_("Unable to bind to %s on this computer (bind returned error %s)"), to.ToStringAddrPort(), NetworkErrorString(nErr));
4        }

nit: Curious why you changed these to be categorized (used to be LogPrintf() without category) - worth noting in commit message?

0        LogInfo("Cannot set SO_REUSEADDR on %s listen socket: %s, continuing anyway",
1                to.ToStringAddrPort(),
2                NetworkErrorString(WSAGetLastError()));

nit: listen to -> listen on?

0        errmsg = strprintf(_("Cannot listen on %s: %s"), to.ToStringAddrPort(), NetworkErrorString(WSAGetLastError()));

nit: Leftover struct for instance and C-style casts post-modernization. Also in BindAndStartListening.

0    sockaddr_storage sockaddr_bind;
1    socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
2    if (!sock.GetSockName(reinterpret_cast<sockaddr*>(&sockaddr_bind), &sockaddr_bind_len)) {
3        addr_bind.SetSockAddr(reinterpret_cast<const sockaddr*>(&sockaddr_bind));

Removed struct from BindAndStartListening() in commit style: modernize the style of SockMan::BindListenPort().

GetBindAddress() was just moved verbatim without any mods from net.cpp to sockman.cpp in another commit. This makes it easier to review with

0[diff]  
1        colorMoved = dimmed-zebra
2        colorMovedWS = allow-indentation-change

in ~/.gitconfig. Will leave it as it is. Don’t want to bloat this PR further with one more “style changing” commit.

Since it’s only inherited privately, destructor could be non-virtual and protected unless we want to destroy by SockMan-reference?

Happy to see you made the virtual methods only called by SockMan private. :)

In general, less indentation makes the code more readable, so I prefer to have return; and then the subsequent code to be with less indentation.

Anyway this code was changed and now uses switch (see CConnman::EventI2PStatus() after I push).

nit: One plural word is enough?

0     * Start the necessary threads for socket IO.
1     */
2    void StartSocketThreads(const Options& options);
3
4    /**
5     * Join (wait for) the threads started by `StartSocketThreads()` to exit.
6     */
7    void JoinSocketThreads();

nit: Could go with something closer to original?

0    if (auto it{m_nodes.find(id)}; it != m_nodes.end()) {
1        auto node{it->second};
2        LogDebug(BCLog::NET, "disconnect by id peer=%d; disconnecting\n", id);
3        node->fDisconnect = true;
4        return true;
5    } else {
6        return false;
7    }

Same pattern of auto it{m_nodes.find( appearing on it’s own line when it could be hoisted into the if to reduce scope is repeated elsewhere too.

Less indentation makes the code more readable in general. So I prefer:

0if (A) {
1    return;
2}
3CODE;

over:

0if (!A) {
1    CODE;
2} else {
3    return;
4}

Reduced scope is nice, but I find if (auto it{m_nodes.find(id)}; it != m_nodes.end()) { hard to read. Sometimes I confuse it for a for-loop: for (foo; bar; baz) {. Also in this particular case the function ends right after this, so the scope of it is the same.

Will leave it as it is.

There should be no change in behavior. Note that the original code in master was very subtle - it required that ForEachNode() would iterate the nodes in increasing order of id:

0std::pair<NodeId, std::chrono::seconds> youngest_peer{-1, 0}, next_youngest_peer{-1, 0};
1
2m_connman.ForEachNode([&](CNode* pnode) {
3    if (!pnode->IsBlockOnlyConn() || pnode->fDisconnect) return;
4    if (pnode->GetId() > youngest_peer.first) {
5        next_youngest_peer = youngest_peer;
6        youngest_peer.first = pnode->GetId();
7        youngest_peer.second = pnode->m_last_block_time;
8    }
9});

The condition pnode->GetId() > youngest_peer.first would be true every time. Because of that, this code is equivalent to:

0std::pair<NodeId, std::chrono::seconds> youngest_peer{-1, 0}, next_youngest_peer{-1, 0};
1
2m_connman.ForEachNode([&](CNode* pnode) {
3    if (!pnode->IsBlockOnlyConn() || pnode->fDisconnect) return;
4    next_youngest_peer = youngest_peer;
5    youngest_peer.first = pnode->GetId();
6    youngest_peer.second = pnode->m_last_block_time;
7});

The new code will again select the two youngest peers (the two with the bigger ids), but does not require that the iterating is in increasing order of id:

 0std::pair<NodeId, std::chrono::seconds> youngest_peer{-1, 0}, next_youngest_peer{-1, 0};
 1
 2m_connman.ForEachNode([&](CNode* pnode) {
 3    if (!pnode->IsBlockOnlyConn() || pnode->fDisconnect) return;
 4    if (pnode->GetId() > next_youngest_peer.first) {
 5        if (pnode->GetId() > youngest_peer.first) {
 6            next_youngest_peer = youngest_peer;
 7            youngest_peer.first = pnode->GetId();
 8            youngest_peer.second = pnode->m_last_block_time;
 9        } else {  
10            next_youngest_peer.first = pnode->GetId();
11            next_youngest_peer.second = pnode->m_last_block_time;
12        }
13    }
14});

Ah, that was subtle, thanks for laying it out so clearly.

Maybe amend the commit message in “net: index nodes in CConnman by id” with something like:

Change PeerManagerImpl::EvictExtraOutboundPeers to account for nodes no longer always being in order. The old code would have failed to update next_youngest_peer correctly if CConnman::m_nodes hadn’t always had node ids in ascending order.

nit:

0    for (const auto& [_, node] : connman->TestNodes()) {

Might want to assert in case a test assigns ids randomly and the birthday paradox occurs on annoyingly rare test-runs.

0        auto [_, success] = m_nodes.emplace(node.GetId(), &node);
1        Assert(success);

It is for every function, not just “thread” ones - doc/developer-notes.md says:

• Combine annotations in function declarations with run-time asserts in function definitions …

This means:

0void f() EXCLUSIVE_LOCKS_REQUIRED(!m);
1
2...
3
4void f()
5{
6    AssertLockNotHeld(m);
7    ...
8}

The reasoning is that the annotation EXCLUSIVE_LOCKS_REQUIRED can be ignored. It is nice that the check is done at compile time, but if the compiler is not clang then it will be ignored, or if the compiler is clang but is running without -Werror, then it will only result in a compile warning which is likely to be missed in the build output. In other words, it is possible to create a violating code and compile and run it with both gcc and clang. In that case AssertLockNotHeld() will stop the program at runtime.

An issue here is that we return a CNode*, unlock m_nodes_mutex, then another thread comes along and deletes the node - leaving the caller with a dangling pointer.

Maybe there are implementation details that make that impossible right now, but it would feel safer to instead have this function require the caller locks m_nodes_mutex, or do an AddRef() internally and require caller to do Release(). (Switching to shared_ptr instead of custom ref-counting might be more straight forward but maybe too disruptive).

Maybe it is worth doing this: #30988 (comment). It is a super good change and normally I would do it, but the reason I didn’t is that it will increase the size of this PR which, I am afraid, would turn reviewers away.

I will proceed to other suggestions and give this some thought…

After thinking about this a bit more and doing some changes to the code, I decided to:

1. Open a separate PR to remove the manual ref counting on CNode and replace it with shared_ptr and change CConnman::m_nodes from vector<CNode*> to unordered_set<shared_ptr<CNode>>. That PR would be independent from this one.

2. In this PR, change the communication between SockMan and CConman to be based on pointer to CNode instead of node id. Similarly to #30988 (comment). I will start with a raw pointer and the responsibility of destruction will be in CConnman, like it is now. If 1. is merged in the mean time before this PR, then I will just change this PR to use shared_ptr instead of a raw pointer.

nit: Could use span, and maybe even byte?

0    virtual void EventGotData(NodeId node_id, const std::span<std::byte> data) = 0;

conversion is not possible.

It should be possible to convert std::byte* to uint8_t* (and vice-versa). The two are the almost the same anyway (https://en.cppreference.com/w/cpp/types/byte). And a span is just a pointer+size, so conversion between the two span types should also be possible.

By default trying to pass std::span<std::byte> to a function that takes std::span<uint8_t> gives:

0no known conversion from 'span<std::byte>' to 'span<uint8_t>'

If I really insist then I can convert it like:

0void f(std::span<uint8_t> s);
1...
2std::span<std::byte> a;
3f(std::span<uint8_t>{reinterpret_cast<uint8_t*>(a.data()), a.size()});

I find it cleaner to use uint8_t because it is already used in the existent code (ReceiveMsgBytes()) and avoid such forced coversion.

Certainly won’t insist, but another possibility is to use bit_cast for trivially constructible types:

0void f(std::span<uint8_t> s);
1...
2std::span<std::byte> a;
3f(std::bit_cast<std::span<uint8_t>>(a));

bit_cast

While this will likely work in practise, I don’t think there is any inherent guarantee that the layout of span is identical for all underlying types. So my recommendation would be to just use std::as_bytes instead.

(Same for the reinterpret_cast: While it works, it is a bit verbose and std::as_bytes is the existing alias in the std lib, which is already used in the code today.)

Obviously, anything is fine. I just left a comment to say it is possible to convert :sweat_smile:

248ec2d2687fae47b63688e00b9ef18d4c0c9676 nit: when I run the linter (along with all other tests) on this commit it complains about trailing whitespace.

 0diff --git a/src/net.cpp b/src/net.cpp
 1index 8d0dd84d91..0228ddce57 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -2068,7 +2068,7 @@ void CConnman::EventIOLoopCompletedForAllPeers()
 5     DisconnectNodes();
 6     NotifyNumConnectionsChanged();
 7 }
 8- 
 9+
10 Sock::EventsPerSock CConnman::GenerateWaitSockets(Span<CNode* const> nodes)
11 {
12     AssertLockNotHeld(m_nodes_mutex);
13diff --git a/src/net.h b/src/net.h
14index 8778bcb88a..6c2c202fbb 100644
15--- a/src/net.h
16+++ b/src/net.h
17@@ -1317,7 +1317,7 @@ private:
18 
19     virtual void EventIOLoopCompletedForAllPeers() override
20         EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_reconnections_mutex);
21- 
22+
23     /**
24      * Generate a collection of sockets to check for IO readiness.
25      * [@param](/bitcoin-bitcoin/contributor/param/)[in] nodes Select from these nodes' sockets.