Split CConnman #30988

pull vasild wants to merge 16 commits into bitcoin:master from vasild:sockman changing 18 files +1343 −714
  1. vasild commented at 7:57 am on September 27, 2024: contributor

    Currently CConnman is a mixture of:

    • low level socket handling, e.g. send, recv, poll, bind, listen, connect, and
    • higher level logic that is specific to the Bitcoin P2P protocol, e.g. V1/V2 transport, choosing which address to connect to, if we manage to connect mark the address good in AddrMan, maintaining the number of inbound and outbound connections, banning of peers, interacting with PeerManager.

    This PR splits the socket handling into a new class which makes the code more modular and reusable. The aim is to (re)use that for Stratum V2 and for libevent-less RPC/HTTP server.

    The socket operations are driven by the new class SockMan which informs the higher level via provided methods when e.g. new data arrives on the socket or a new connection is accepted. For this, SockMan provides some non-virtual methods to start it rolling and then it calls pure virtual methods which are implemented by the higher level (e.g. CConnman) on certain events, for example “got this new data on this node’s socket”.

    The public interface of SockMan is:

      0/**
      1 * A socket manager class which handles socket operations.
      2 * To use this class, inherit from it and implement the pure virtual methods.
      3 * Handled operations:
      4 * - binding and listening on sockets
      5 * - starting of necessary threads to process socket operations
      6 * - accepting incoming connections
      7 * - making outbound connections
      8 * - closing connections
      9 * - waiting for IO readiness on sockets and doing send/recv accordingly
     10 */
     11class SockMan
     12{
     13public:
     14
     15    //
     16    // Non-virtual functions, to be reused by children classes.
     17    //
     18
     19    /**
     20     * Bind to a new address:port, start listening and add the listen socket to `m_listen`.
     21     * Should be called before `StartSocketsThreads()`.
     22     * [@param](/bitcoin-bitcoin/contributor/param/)[in] to Where to bind.
     23     * [@param](/bitcoin-bitcoin/contributor/param/)[out] errmsg Error string if an error occurs.
     24     * [@retval](/bitcoin-bitcoin/contributor/retval/) true Success.
     25     * [@retval](/bitcoin-bitcoin/contributor/retval/) false Failure, `strError` will be set.
     26     */
     27    bool BindAndStartListening(const CService& to, bilingual_str& errmsg);
     28
     29    /**
     30     * Start the necessary threads for sockets IO.
     31     */
     32    void StartSocketsThreads(const Options& options);
     33
     34    /**
     35     * Join (wait for) the threads started by `StartSocketsThreads()` to exit.
     36     */
     37    void JoinSocketsThreads();
     38
     39    /**
     40     * Make an outbound connection, save the socket internally and return a newly generated node id.
     41     * [@param](/bitcoin-bitcoin/contributor/param/)[in] to The address to connect to, either as CService or a host as string and port as
     42     * an integer, if the later is used, then `proxy` must be valid.
     43     * [@param](/bitcoin-bitcoin/contributor/param/)[in] is_important If true, then log failures with higher severity.
     44     * [@param](/bitcoin-bitcoin/contributor/param/)[in] proxy Proxy to connect through if `proxy.IsValid()` is true.
     45     * [@param](/bitcoin-bitcoin/contributor/param/)[out] proxy_failed If `proxy` is valid and the connection failed because of the
     46     * proxy, then it will be set to true.
     47     * [@param](/bitcoin-bitcoin/contributor/param/)[out] me If the connection was successful then this is set to the address on the
     48     * local side of the socket.
     49     * [@return](/bitcoin-bitcoin/contributor/return/) Newly generated node id, or std::nullopt if the operation fails.
     50     */
     51    std::optional<NodeId> ConnectAndMakeNodeId(const std::variant<CService, StringHostIntPort>& to,
     52                                               bool is_important,
     53                                               const Proxy& proxy,
     54                                               bool& proxy_failed,
     55                                               CService& me);
     56
     57    /**
     58     * Disconnect a given peer by closing its socket and release resources occupied by it.
     59     * [@return](/bitcoin-bitcoin/contributor/return/) Whether the peer existed and its socket was closed by this call.
     60     */
     61    bool CloseConnection(NodeId node_id);
     62
     63    /**
     64     * Try to send some data to the given node.
     65     * [@param](/bitcoin-bitcoin/contributor/param/)[in] node_id Identifier of the node to send to.
     66     * [@param](/bitcoin-bitcoin/contributor/param/)[in] data The data to send, it might happen that only a prefix of this is sent.
     67     * [@param](/bitcoin-bitcoin/contributor/param/)[in] will_send_more Used as an optimization if the caller knows that they will
     68     * be sending more data soon after this call.
     69     * [@param](/bitcoin-bitcoin/contributor/param/)[out] errmsg If <0 is returned then this will contain a human readable message
     70     * explaining the error.
     71     * [@retval](/bitcoin-bitcoin/contributor/retval/) >=0 The number of bytes actually sent.
     72     * [@retval](/bitcoin-bitcoin/contributor/retval/) <0 A permanent error has occurred.
     73     */
     74    ssize_t SendBytes(NodeId node_id,
     75                      Span<const unsigned char> data,
     76                      bool will_send_more,
     77                      std::string& errmsg) const;
     78
     79    /**
     80     * Close all sockets.
     81     */
     82    void CloseSockets();
     83
     84    //
     85    // Pure virtual functions must be implemented by children classes.
     86    //
     87
     88    /**
     89     * Be notified when a new connection has been accepted.
     90     * [@param](/bitcoin-bitcoin/contributor/param/)[in] node_id Id of the newly accepted connection.
     91     * [@param](/bitcoin-bitcoin/contributor/param/)[in] me The address and port at our side of the connection.
     92     * [@param](/bitcoin-bitcoin/contributor/param/)[in] them The address and port at the peer's side of the connection.
     93     * [@retval](/bitcoin-bitcoin/contributor/retval/) true The new connection was accepted at the higher level.
     94     * [@retval](/bitcoin-bitcoin/contributor/retval/) false The connection was refused at the higher level, so the
     95     * associated socket and node_id should be discarded by `SockMan`.
     96     */
     97    virtual bool EventNewConnectionAccepted(NodeId node_id,
     98                                            const CService& me,
     99                                            const CService& them) = 0;
    100
    101    /**
    102     * Called when the socket is ready to send data and `ShouldTryToSend()` has
    103     * returned true. This is where the higher level code serializes its messages
    104     * and calls `SockMan::SendBytes()`.
    105     * [@param](/bitcoin-bitcoin/contributor/param/)[in] node_id Id of the node whose socket is ready to send.
    106     * [@param](/bitcoin-bitcoin/contributor/param/)[out] cancel_recv Should always be set upon return and if it is true,
    107     * then the next attempt to receive data from that node will be omitted.
    108     */
    109    virtual void EventReadyToSend(NodeId node_id, bool& cancel_recv) = 0;
    110
    111    /**
    112     * Called when new data has been received.
    113     * [@param](/bitcoin-bitcoin/contributor/param/)[in] node_id Node for which the data arrived.
    114     * [@param](/bitcoin-bitcoin/contributor/param/)[in] data Data buffer.
    115     * [@param](/bitcoin-bitcoin/contributor/param/)[in] n Number of bytes in `data`.
    116     */
    117    virtual void EventGotData(NodeId node_id, const uint8_t* data, size_t n) = 0;
    118
    119    /**
    120     * Called when the remote peer has sent an EOF on the socket. This is a graceful
    121     * close of their writing side, we can still send and they will receive, if it
    122     * makes sense at the application level.
    123     * [@param](/bitcoin-bitcoin/contributor/param/)[in] node_id Node whose socket got EOF.
    124     */
    125    virtual void EventGotEOF(NodeId node_id) = 0;
    126
    127    /**
    128     * Called when we get an irrecoverable error trying to read from a socket.
    129     * [@param](/bitcoin-bitcoin/contributor/param/)[in] node_id Node whose socket got an error.
    130     * [@param](/bitcoin-bitcoin/contributor/param/)[in] errmsg Message describing the error.
    131     */
    132    virtual void EventGotPermanentReadError(NodeId node_id, const std::string& errmsg) = 0;
    133
    134    //
    135    // Non-pure virtual functions can be overridden by children classes or left
    136    // alone to use the default implementation from SockMan.
    137    //
    138
    139    /**
    140     * SockMan would only call EventReadyToSend() if this returns true.
    141     * Can be used to temporary pause sends for a node.
    142     * The implementation in SockMan always returns true.
    143     * [@param](/bitcoin-bitcoin/contributor/param/)[in] node_id Node for which to confirm or cancel a call to EventReadyToSend().
    144     */
    145    virtual bool ShouldTryToSend(NodeId node_id) const;
    146
    147    /**
    148     * SockMan would only call Recv() on a node's socket if this returns true.
    149     * Can be used to temporary pause receives for a node.
    150     * The implementation in SockMan always returns true.
    151     * [@param](/bitcoin-bitcoin/contributor/param/)[in] node_id Node for which to confirm or cancel a receive.
    152     */
    153    virtual bool ShouldTryToRecv(NodeId node_id) const;
    154
    155    /**
    156     * SockMan has completed the current send+recv iteration for a node.
    157     * It will do another send+recv for this node after processing all other nodes.
    158     * Can be used to execute periodic tasks for a given node.
    159     * The implementation in SockMan does nothing.
    160     * [@param](/bitcoin-bitcoin/contributor/param/)[in] node_id Node for which send+recv has been done.
    161     */
    162    virtual void EventIOLoopCompletedForNode(NodeId node_id);
    163
    164    /**
    165     * SockMan has completed send+recv for all nodes.
    166     * Can be used to execute periodic tasks for all nodes.
    167     * The implementation in SockMan does nothing.
    168     */
    169    virtual void EventIOLoopCompletedForAllPeers();
    170
    171    /**
    172     * Be notified of a change in the state of listening for incoming I2P connections.
    173     * The default behavior, implemented by `SockMan`, is to ignore this event.
    174     * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr Our listening address.
    175     * [@param](/bitcoin-bitcoin/contributor/param/)[in] success If true then the listen succeeded and we are now
    176     * listening for incoming I2P connections at `addr`. If false then the
    177     * call failed and now we are not listening (even if this was invoked
    178     * before with `true`).
    179     */
    180    virtual void EventI2PListen(const CService& addr, bool success);
    181};
    

    Resolves: #30694


    Review hint: this PR moves some code around, so reviewers may find this helpful: git show --color-moved --color-moved-ws=allow-indentation-change.

  2. net: reduce CAddress usage to CService or CNetAddr
    * `CConnman::CalculateKeyedNetGroup()` needs `CNetAddr`, not `CAddress`,
      thus change its argument.
    
    * Both callers of `CConnman::CreateNodeFromAcceptedSocket()` create a
      dummy `CAddress` from `CService`, so use `CService` instead.
    1bfc1ca9b6
  3. DrahtBot commented at 7:57 am on September 27, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK tdb3, pinheadmz

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31022 (test: Add mockable steady clock, tests for PCP and NATPMP implementations by laanwj)
    • #31014 (net: Use GetAdaptersAddresses to get local addresses on Windows by laanwj)
    • #30951 (net: option to disallow v1 connection on ipv4 and ipv6 peers by stratospher)
    • #30381 ([WIP] net: return result from addnode RPC by willcl-ark)
    • #30315 (Stratum v2 Transport by Sjors)
    • #29418 (rpc: provide per message stats for global traffic via new RPC ‘getnetmsgstats’ by vasild)
    • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
    • #28584 (Fuzz: extend CConnman tests by vasild)
    • #28521 (net, net_processing: additional and consistent disconnect logging by Sjors)
    • #28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)
    • #25832 (tracing: network connection tracepoints by 0xB10C)

    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.

  4. in src/rpc/net.cpp:210 in 03f6cc2b4a outdated
    202@@ -203,6 +203,15 @@ static RPCHelpMan getpeerinfo()
    203     std::vector<CNodeStats> vstats;
    204     connman.GetNodeStats(vstats);
    205 
    206+    // An undocumented side effect of how CConnman previously stored nodes was
    207+    // that they were returned ordered by id. At least some functional tests
    208+    // rely on that, so keep it that way. An alternative is to remove this sort
    209+    // and fix the tests and take the risk of breaking other users of the
    210+    // "getpeerinfo" RPC.
    


    maflcko commented at 8:41 am on September 27, 2024:
    nit: Arrays are ordered data structures and I don’t think it is user-friendly to shuffle them by ID. I think this comment can just be removed.

    vasild commented at 3:05 pm on September 27, 2024:
    Removed, thanks!
  5. DrahtBot added the label CI failed on Sep 27, 2024
  6. Sjors commented at 11:41 am on September 27, 2024: member

    Nice! I’ll try to use this for Sv2Connman in https://github.com/Sjors/bitcoin/pull/50 and will let you know if anything is missing.

    Can you put sockman.h in libbitcoin_common instead of libbitcoin_node? For the Template Provider I’m trying to prevent a circular dependency on the node. This should do the trick: https://github.com/bitcoin/bitcoin/commit/4dd51b2924860bf10466da080ea4ff7bed1a3e3f

  7. net: split CConnman::BindListenPort() off CConnman
    Introduce a new low-level socket managing class `SockMan`
    and move the `CConnman::BindListenPort()` method to it.
    
    Also, separate the listening socket from the permissions -
    they were coupled in `struct ListenSocket`, but the socket
    is protocol agnostic, whereas the permissions are specific
    to the application of the Bitcoin P2P protocol.
    41c87ddb3d
  8. style: modernize the style of SockMan::BindListenPort()
    It was copied verbatim from `CConnman::BindListenPort()` in the previous
    commit. Modernize its variables and style and log the error messages
    from the caller.
    b5362b73ec
  9. net: split CConnman::AcceptConnection() off CConnman
    Move the `CConnman::AcceptConnection()` method to `SockMan` and split
    parts of it:
    * the flip-to-CJDNS part: to just after the `AcceptConnection()` call
    * the permissions part: at the start of `CreateNodeFromAcceptedSocket()`
    708398cbe8
  10. style: modernize the style of SockMan::AcceptConnection() 0398a8a017
  11. net: move the generation of ids for new nodes from CConnman to SockMan 5d4920f630
  12. net: move CConnman-specific parts away from ThreadI2PAcceptIncoming()
    CConnman-specific or in other words, Bitcoin P2P specific. Now
    the `ThreadI2PAcceptIncoming()` method is protocol agnostic and
    can be moved to `SockMan`.
    189827785b
  13. net: move I2P-accept-incoming code from CConnman to SockMan b94f9d338f
  14. net: index nodes in CConnman by id
    Change `CConnman::m_nodes` from `std::vector<CNode*>` to
    `std::unordered_map<NodeId, CNode*>` because interaction
    between `CConnman` and `SockMan` is going to be based on
    `NodeId` and finding a node by its id would better be fast.
    
    As a nice side effect the existent search-by-id operations in
    `CConnman::AttemptToEvictConnection()`,
    `CConnman::DisconnectNode()` and
    `CConnman::ForNode()` now become `O(1)` (were `O(number of nodes)`),
    as well as the erase in `CConnman::DisconnectNodes()`.
    b96beb27d2
  15. net: isolate P2P specifics from GenerateWaitSockets()
    Move the parts of `CConnman::GenerateWaitSockets()` that are specific to
    the Bitcoin-P2P protocol to dedicated methods:
    `ShouldTryToSend()` and `ShouldTryToRecv()`.
    
    This brings us one step closer to moving `GenerateWaitSockets()` to the
    protocol agnostic `SockMan` (which would call `ShouldTry...()` from
    `CConnman`).
    bb5b91d430
  16. net: isolate P2P specifics from SocketHandlerConnected() and ThreadSocketHandler()
    Move some parts of `CConnman::SocketHandlerConnected()` and
    `CConnman::ThreadSocketHandler()` that are specific to the Bitcoin-P2P
    protocol to dedicated methods:
    `EventIOLoopCompletedForNode()` and `EventIOLoopCompletedForAllPeers()`.
    
    This brings us one step closer to moving `SocketHandlerConnected()` and
    `ThreadSocketHandler()` to the protocol agnostic `SockMan` (which would
    call `EventIOLoopCompleted...()` from `CConnman`).
    50cb52470e
  17. net: isolate all remaining P2P specifics from SocketHandlerConnected()
    Introduce 4 new methods for the interaction between `CConnman` and
    `SockMan`:
    
    * `EventReadyToSend()`:
      called when there is readiness to send and do the actual sending of data.
    
    * `EventGotData()`, `EventGotEOF()`, `EventGotPermanentReadError()`:
      called when the corresponing recv events occur.
    
    These methods contain logic that is specific to the Bitcoin-P2P protocol
    and move it away from `CConnman::SocketHandlerConnected()` which will
    become a protocol agnostic method of `SockMan`.
    
    Also, move the counting of sent bytes to `CConnman::SocketSendData()` -
    both callers of that method called `RecordBytesSent()` just after the
    call, so move it from the callers to inside
    `CConnman::SocketSendData()`.
    96974ffb3b
  18. net: split CConnman::ConnectNode()
    Move the protocol agnostic parts of `CConnman::ConnectNode()` into
    `SockMan::ConnectAndMakeNodeId()` and leave the Bitcoin-P2P specific
    stuff in `CConnman::ConnectNode()`.
    
    Move the protocol agnostic `CConnman::m_unused_i2p_sessions`, its mutex
    and `MAX_UNUSED_I2P_SESSIONS_SIZE` to `SockMan`.
    
    Move `GetBindAddress()` from `net.cpp` to `sockman.cpp`.
    3bb0145514
  19. net: tweak EventNewConnectionAccepted()
    Move `MaybeFlipIPv6toCJDNS()`, which is Bitcoin P2P specific from the
    callers of `CConnman::EventNewConnectionAccepted()` to inside that
    method.
    
    Move the IsSelectable check, the `TCP_NODELAY` option set and the
    generation of new node id out of `CConnman::EventNewConnectionAccepted()`
    because those are protocol agnostic. Move those to a new method
    `SockMan::NewSockAccepted()` which is called instead of
    `CConnman::EventNewConnectionAccepted()`.
    9d1b352a4d
  20. net: move sockets from CNode to SockMan
    Move `CNode::m_sock` and `CNode::m_i2p_sam_session` to `SockMan::m_connected`.
    Also move all the code that handles sockets to `SockMan`.
    
    `CNode::CloseSocketDisconnect()` becomes
    `CConnman::MarkAsDisconnectAndCloseConnection()`.
    
    `CConnman::SocketSendData()` is renamed to
    `CConnman::SendMessagesAsBytes()` and its sockets-touching bits are moved to
    `SockMan::SendBytes()`.
    
    `CConnman::GenerateWaitSockets()` goes to
    `SockMan::GenerateWaitSockets()`.
    
    `CConnman::ThreadSocketHandler()` and
    `CConnman::SocketHandler()` are combined into
    `SockMan::ThreadSocketHandler()`.
    
    `CConnman::SocketHandlerConnected()` goes to
    `SockMan::SocketHandlerConnected()`.
    
    `CConnman::SocketHandlerListening()` goes to
    `SockMan::SocketHandlerListening()`.
    c133a634d1
  21. net: move-only: improve encapsulation of SockMan
    `SockMan` members
    
    `AcceptConnection()`
    `NewSockAccepted()`
    `GetNewNodeId()`
    `m_i2p_sam_session`
    `m_listen private`
    
    are now used only by `SockMan`, thus make them private.
    70c2f13f83
  22. vasild force-pushed on Sep 27, 2024
  23. vasild commented at 3:07 pm on September 27, 2024: contributor

    03f6cc2b4a...70c2f13f83: fix CI failure, and address suggestions

    Can you put sockman.h in libbitcoin_common

    Done.

  24. DrahtBot removed the label CI failed on Sep 27, 2024
  25. Sjors commented at 4:51 pm on September 27, 2024: member

    Here’s an initial sketch of making Sv2Connman a subclass of SockMan. The test gets through the handshake but fails later on, so I’ll need to study it a bit more closely.

    https://github.com/Sjors/bitcoin/pull/64

  26. DrahtBot added the label CI failed on Sep 29, 2024
  27. DrahtBot removed the label CI failed on Sep 29, 2024
  28. tdb3 commented at 10:05 pm on September 29, 2024: contributor
    Concept ACK
  29. pinheadmz commented at 1:38 pm on September 30, 2024: member

    Concept ACK

    This looks great and the API in the header looks easy, thanks.

    I’m in the process of cleaning up my HTTP branch for a pull request and then I can start reviewing this and rebasing on top.

    One element of libevent I’m not immediately seeing here is timed events. Really the only thing HTTP needs it for is walletpassphrase which calls RPCRunLater() which interacts with HTTPRPCTimerInterface(). I don’t think Conman has a specific mechanism for this because timed things are attached directly to nodes like m_last_getheaders_timestamp etc. The current HTTPRPCTimerInterface uses libevent event_new() and evtimer_add(), I accomplish this with a map of timestamps and callback functions in my event loop: https://github.com/pinheadmz/bitcoin/commit/42b7240378ffc4890ab4ba1453623e5986ea2a71

  30. maflcko commented at 2:07 pm on September 30, 2024: member

    I accomplish this with a map of timestamps and callback functions in my event loop

    I wonder why the existing scheduler can’t be used for re-locking the wallet? I know there is #18488 and #14289, but the thread is already filled with random stuff such as BerkeleyDatabase::PeriodicFlush(), and relocking the wallet seems(?) fast (I haven’t benchmarked), so should be fine to put in there as well, at least from that perspective?

  31. vasild commented at 10:07 am on October 3, 2024: contributor

    @pinheadmz, I think that the functionality of “execute this code after some time”, is not much related to the sockets handling and better be implemented at some higher level, not inside SockMan. Maybe the scheduler, like @maflcko suggested, or in the EventIOLoopCompletedForAllPeers() method which will be called periodically by SockMan:

    0    /**
    1     * SockMan has completed send+recv for all nodes.
    2     * Can be used to execute periodic tasks for all nodes.
    3     * The implementation in SockMan does nothing.
    4     */
    5    virtual void EventIOLoopCompletedForAllPeers();
    

    Edit: I guess TriggerEvents() from https://github.com/pinheadmz/bitcoin/commit/42b7240378ffc4890ab4ba1453623e5986ea2a71 can be called from EventIOLoopCompletedForAllPeers() or from the scheduler.

  32. Sjors commented at 1:54 pm on October 4, 2024: member
    @vasild if you rebase past #31011, tidy might point out that sockman.cpp.o depends on i2p.cpp. So you probably need to either move i2p.cpp to common as well, or remove the dependency.
  33. Jacksonearl2468 approved
  34. DrahtBot added the label CI failed on Oct 7, 2024

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: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me