Don’t check if the listening socket is valid #23601

1. 
   vasild commented at 11:06 am on November 26, 2021: member

   This is a piece of #21878, chopped off to ease review.

   Listening sockets in CConnman::vhListenSocket are always valid (underlying file descriptor is not INVALID_SOCKET).

2. net: don't check if the listening socket is valid

   Listening sockets in `CConnman::vhListenSocket` are always valid
   (underlying file descriptor is not `INVALID_SOCKET`).

   6c9ee92ffe
3. fanquake added the label P2P on Nov 26, 2021
4. vasild renamed this:
   net: don't check if the listening socket is valid
   Don't check if the listening socket is valid
   on Nov 26, 2021
5. theStack approved
6. 
   theStack commented at 7:14 pm on November 26, 2021: member

   Code-review ACK 6c9ee92ffe390064881ef5bf47e38e345d78a2a1 🔌

   Verified that it’s currently not possible that CConnman::vhListenSocket ever contains invalid sockets. The only place where an element gets added to the vector is in the method CConnman::BindListenPort:

   https://github.com/bitcoin/bitcoin/blob/111c3e06b35b7867f2e0f740e988f648ac6c325d/src/net.cpp#L2394

   which would already return earlier (directly after CreateSock(...)) if the socket contained in the sock wrapper was not valid. Also built and ran unit tests locally with success.

7. 
   DrahtBot commented at 5:34 am on November 27, 2021: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #21879 (Wrap accept() and extend usage of Sock by vasild)
   • #21878 (Make all networking code mockable by vasild)

   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.

8. luke-jr approved
9. 
   luke-jr commented at 10:41 pm on November 30, 2021: member

   utACK.

   If INVALID_SOCKET can get into recv_set, we have UB earlier anyway. Therefore, .count should always be 0 even if somehow listen_socket.socket were INVALID_SOCKET.

10. MarcoFalke added the label Refactoring on Dec 1, 2021
11. MarcoFalke merged this on Dec 1, 2021
12. MarcoFalke closed this on Dec 1, 2021

    ---

13. 
    MarcoFalke commented at 8:40 am on December 1, 2021: member

    If this is safe, is there a reason why it can’t be removed here as well?

    0src/net.cpp=void CConnman::StopNodes()
    1src/net.cpp-{
    2...
    3src/net.cpp-
    4src/net.cpp-    // Close listening sockets.
    5src/net.cpp:    for (ListenSocket& hListenSocket : vhListenSocket) {
    6src/net.cpp-        if (hListenSocket.socket != INVALID_SOCKET) {
    

14. vasild deleted the branch on Dec 1, 2021
15. 
    vasild commented at 12:32 pm on December 1, 2021: member

    @MarcoFalke, that’s right - can be removed from here too and should have been part of this PR, I missed it.

    I missed it because in #21878 that “close listening sockets” code becomes unnecessary and is removed:

    0-    // Close listening sockets.
    1-    for (ListenSocket& hListenSocket : vhListenSocket) {
    2-        if (hListenSocket.socket != INVALID_SOCKET) {
    3-            if (!CloseSocket(hListenSocket.socket)) {
    4-                LogPrintf("CloseSocket(hListenSocket) failed with error %s\n", NetworkErrorString(WSAG
    5-            }
    6-        }
    7-    }
    

    This removal is also in #21879 which is chopped off from the big #21878 to ease review.

    I will leave it as is to avoid further conflicts, hoping that #21879 will eventually make it in.

16. sidhujag referenced this in commit 7a5e16a331 on Dec 1, 2021
17. RandyMcMillan referenced this in commit 509fa3a93f on Dec 23, 2021
18. DrahtBot locked this on Dec 1, 2022

Contributors
vasild theStack DrahtBot luke-jr MarcoFalke

Labels
Refactoring P2P