Use shared_ptr for CNode inside CConnman

willcl-ark commented at 9:45 am on August 5, 2023: member

Switch to using smart pointers to CNodes inside of CConnman.

Currently we are manually refcounting CNodes which is potentially error-prone and makes operations such as deleting them from multiple threads difficult without introducing new locks or other synchronisation operations (see #27912).

Switch to using std::shared_ptr references to CNodes inside of m_nodes and m_nodes_disconnected to give us better memory safety today, and in the future allow AttemptToEvictConnection (and optionally other sites) to safely synchronously disconnect nodes when needed.

Opening as draft for now as I want to both gauge feedback on the approach, and see which PRs this may conflict with (#27213?) before moving it forwards.

CC @vasild

DrahtBot commented at 9:45 am on August 5, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	dergoegge, vasild

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:

#28248 (p2p: bugfixes, logic and logging improvements by jonatack)
#28196 (BIP324 connection support by sipa)
#28165 (net: transport abstraction by sipa)
#28155 (Improves addnode / m_added_nodes logic by sr-gi)
#28016 (p2p: gives seednode priority over dnsseed if both are provided by sr-gi)
#27981 (Fix potential network stalling bug by sipa)
#27600 (net: Add new permission forceinbound to evict a random unprotected connection if all slots are otherwise full by pinheadmz)
#27114 (p2p: Allow whitelisting outgoing connections by brunoerg)
#27071 (Handle CJDNS from LookupSubNet() by vasild)
#26938 ([WIP] p2p: asmap, avoid inbound connections from a specific AS by brunoerg)
#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
#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.

DrahtBot added the label CI failed on Aug 5, 2023

dergoegge commented at 1:35 pm on August 5, 2023: member

Also see #10738

willcl-ark commented at 2:14 pm on August 5, 2023: member

Also see #10738

Thanks, I hadn’t seen this one before. Will take a look tonight

Edit: Really like the last commit. Will see if I can also drop a few locks too..

willcl-ark force-pushed on Aug 5, 2023

DrahtBot removed the label CI failed on Aug 5, 2023

DrahtBot added the label Needs rebase on Aug 6, 2023

vasild commented at 6:42 am on August 7, 2023: contributor

Also see #10738

:+1:

I guess it must be possible to do that without “reinventing the wheel aka introducing custom smart pointer types”, like mentioned in the last comment.

in src/net_processing.cpp:505 in 499f267050 outdated

501@@ -502,7 +502,7 @@ class PeerManagerImpl final : public PeerManager
502 
503     /** Implement NetEventsInterface */
504     void InitializeNode(CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
505-    void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex);
506+    void FinalizeNode(const CNodeRef node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex);

dergoegge commented at 10:24 am on August 7, 2023:

Is there a reason why we need to use the shared_ptr type in net processing?

in src/net.h:636 in 499f267050 outdated

632@@ -651,6 +633,8 @@ class CNode
633     std::unique_ptr<i2p::sam::Session> m_i2p_sam_session GUARDED_BY(m_sock_mutex);
634 };
635 
636+typedef std::shared_ptr<CNode> CNodeRef;

dergoegge commented at 10:25 am on August 7, 2023:

(I know this will get push back but)

nit: Could we call this ConnectionRef? After all the connection manager is managing… connections.

dergoegge commented at 10:27 am on August 7, 2023:

It would also seem more modern to use using here

in src/net.cpp:1148 in 499f267050 outdated

1152-                m_nodes_disconnected.remove(pnode);
1153-                DeleteNode(pnode);
1154+        std::vector<CNodeRef> disconnected_still_in_use;
1155+        for (auto& pnode : m_nodes_disconnected) {
1156+            // Finalize when we have the final reference inside this block
1157+            if (pnode.use_count() == 1) {

dergoegge commented at 10:33 am on August 7, 2023:

In a way this is worse than the custom ref counting because it is harder to assert that we actually don’t take new references after this check.

dergoegge commented at 12:00 pm on August 7, 2023: member

Concept ACK on getting rid of our custom ref counting.

I actually implemented something similar a while ago (https://github.com/dergoegge/bitcoin/commit/b3913b4c052b37790b3737e5ddb9c2d029502477) but didn’t pursue because it doesn’t retain the behavior we currently have w.r.t. which thread actually deletes the CNode. That is also the reason why #10738 is a bit more complex and introduces new pointer types.

which thread actually deletes the CNode.

The reason why that is important is because we only want to call FinalizeNode once and we want there to be no other references to that CNode at that time.

The changes in this PR and the current ref-counting don’t guarantee that, they just aim to work that way but it always requires manual review to avoid bugs.

I think I’d prefer an approach that guarantees (at least conceptually) correct behavior (like e.g. the pointer types in #10738).

dergoegge commented at 12:01 pm on August 7, 2023: member

cc @theuni (just in case you are interested in chiming in)

net: Add CNodeRef typedef

To be used in m_nodes and m_nodes_disconnected.

6475adae36

refactor: use CNodeRef in CConnman vectors

Switch to using CNodeRef in m_nodes and m_nodes_disconnected.

This gives us automatic refcounting and will permit safer deletion of
nodes from multiple threads in the future.

Some methods continue to use the stored pointer for now as they require
larger LOC changes, and they operate synchronously under the shared_ptr
reference counting already:

In ThreadMessageHandler SendMessages() and ProcessMessages() are called
using a ref-counted NodeSnapshot.

PushMessage() is called from inside SendMessages() or ForNode().
ProcessMessage() from inside ProcessMessages().

1f612cded8

net: remove manual CNode refcounting

Use the shared_ptr for refcounting.

2edb21812e

willcl-ark force-pushed on Aug 9, 2023

willcl-ark commented at 10:49 am on August 9, 2023: member

Thanks for the conceptual review @dergoegge.

I’ve forced push a relatively different approach here, trying to take into consideration your review points, and #10738, but trying not to require introducing the decaying pointers. This has resulted in the following conceptual changes:

Taking the idea of @theuni’s GetnodesCopy() in m_nodes iterations by re-purposing and upgrading NodesSnapshot() a bit, allowing removal of many instances of m_nodes_mutex in the code, as he achieved.
Made CNode a friend of CConnman. This allows us to call back to DeleteNode() automagically 🪄 on CNode destruction when the final reference is dropped. Then we can just worry about dropping all references, which seems easier to reason about for me. I think it might be possible to modify DeleteNode() further to assert no shared_ptrs remain for it –ensuring that nobody can accidently re-introduce unsafe deletion in the codebase in the future –, but I haven’t looked at this yet.

In a way this is worse than the custom ref counting because it is harder to assert that we actually don’t take new references after this check.

I re-worked this part of DisconnectNodes() in general, but IMO this is safe-enough to drop from m_nodes_disconnected outright, as the only users of m_nodes_disconnected are StopNodes() and DisconnectNodes(). To belt-and-suspenders this anyway, I added a final optional commit which adds an m_nodes_disconnected_mutex to ensure no new references can be created while we are dropping from m_nodes_disconnected.

The commits need tidying up, rewording etc. still, but otherwise would be curious to know if this new approach looked better to you?

DrahtBot removed the label Needs rebase on Aug 9, 2023

in src/net.h:653 in 82dc400170 outdated

649@@ -665,8 +650,11 @@ class CNode
650      * Otherwise this unique_ptr is empty.
651      */
652     std::unique_ptr<i2p::sam::Session> m_i2p_sam_session GUARDED_BY(m_sock_mutex);
653+    CConnman* m_connman;

dergoegge commented at 12:23 pm on August 9, 2023:

Having the managed objects have a reference to the manager leads to absolute spaghetti code, so leaning towards NACK on that.

willcl-ark force-pushed on Aug 9, 2023

DrahtBot added the label CI failed on Aug 9, 2023

in src/net.h:1214 in 36c6fe59b2 outdated

1212      */
1213     class NodesSnapshot
1214     {
1215     public:
1216-        explicit NodesSnapshot(const CConnman& connman, bool shuffle)
1217+        explicit NodesSnapshot(const CConnman& connman, bool shuffle = false)

dergoegge commented at 1:57 pm on August 9, 2023:

You can just get rid of NodesSnapshot entirely now, it was only needed as a RAII helper for the manual ref counting.

theuni commented at 2:04 pm on August 9, 2023: member

@dergoegge Thanks for the ping.

makes operations such as deleting them from multiple threads difficult

Why is this a goal?

I’m quite nervous about these changes. I’ve spoken with @dergoegge about this a few times now, but I’m afraid my information and opinions on the code involved are quite dated at this point (2017 to be exact :), so I haven’t had much to contribute.

I’ll say though, any changes or refactors for future changes to the threading model should be very well justified upfront imo. I remember even being uneasy about #10738 at the time, in fact I might have even talked myself out of rebasing it for merge.

net: switch to using NodeSnapshot in loops dd03b92984

net: automatically destruct CNodes on disconnect af78c48ff8

net: optional m_nodes_disconnected mutex commit

Ensure we don't create new references while trying to drop a node

0954822045

willcl-ark force-pushed on Aug 9, 2023

dergoegge changes_requested

dergoegge commented at 3:05 pm on August 9, 2023: member

makes operations such as deleting them from multiple threads difficult

Why is this a goal?

I missed this in the OP and think that this should not be a goal or should be well justified.

I like getting rid of the custom ref counting for other reasons. Mainly, because it’s annoying and bug prone.

Something I have been working on is hiding CNode from the public net interface and turning the connection manager into a NodeId based manager, e.g. CConnman::PushMessage(CNode*, CSerializedNetMsg&&) becomes CConnman::PushMessage(NodeId, CSerializedNetMsg&&). This leads to situations where the CConnman needs to hold m_nodes_mutex while acquiring the right CNode to call AddRef and then later Release (once finished with the CNode). While that avoids holding m_nodes_mutex the entire time, it is pretty messy/bug-prone while we are using the custom ref-counting and smart pointers would be a good improvement there.

willcl-ark commented at 3:09 pm on August 9, 2023: member

@dergoegge Thanks for the ping.

makes operations such as deleting them from multiple threads difficult

Why is this a goal?

I’m quite nervous about these changes. I’ve spoken with @dergoegge about this a few times now, but I’m afraid my information and opinions on the code involved are quite dated at this point (2017 to be exact :), so I haven’t had much to contribute.

I’ll say though, any changes or refactors for future changes to the threading model should be very well justified upfront imo. I remember even being uneasy about #10738 at the time, in fact I might have even talked myself out of rebasing it for merge.

Thanks, and I agree that after working on this I’m not sure this will ever extend to being threadsafe (todo: update PR description here). For context #27912 is how I came to be looking at this.

I feel like the model as (intended to be) implemented here is easier to reason about as refcounting is taken care of for us via the smart pointers and node cleanup only happens (automatically) once we have a single final reference in m_nodes_disconnected and we drop it: once moved from m_nodes the node won’t be included in any future iterations of that container and it waits to be gracefully dropped from m_nodes_disconnected and cleaned up when appropriate. @dergoegge also left some comments on the current implementation of the approach while I was replying here, which I will consider and reply to seperately :)

jonatack commented at 11:09 pm on August 9, 2023: contributor

Seeing these failures locally:

 0$ ./src/test/test_bitcoin -t denialofservice_tests
 1Running 5 test cases...
 2Assertion failed: lock m_nodes_mutex not held in net.cpp:1598; locks held:
 3'cs_main' in net_processing.cpp:5203 (in thread 'test')
 4unknown location:0: fatal error: in "denialofservice_tests/stale_tip_peer_management": signal: SIGABRT (application abort requested)
 5test/denialofservice_tests.cpp:182: last checkpoint
 6Assertion failed: (ret.second), function AddArg, file args.cpp, line 577.
 7unknown location:0: fatal error: in "denialofservice_tests/block_relay_only_eviction": signal: SIGABRT (application abort requested)
 8test/denialofservice_tests.cpp:243: last checkpoint: "block_relay_only_eviction" fixture ctor
 9Assertion failed: (ret.second), function AddArg, file args.cpp, line 577.
10unknown location:0: fatal error: in "denialofservice_tests/peer_discouragement": signal: SIGABRT (application abort requested)
11test/denialofservice_tests.cpp:304: last checkpoint: "peer_discouragement" fixture ctor
12Assertion failed: (ret.second), function AddArg, file args.cpp, line 577.
13unknown location:0: fatal error: in "denialofservice_tests/DoS_bantime": signal: SIGABRT (application abort requested)
14test/denialofservice_tests.cpp:409: last checkpoint: "DoS_bantime" fixture ctor
15
16*** 4 failures are detected in the test module "Bitcoin Core Test Suite"
17Assertion failed: (m_worker_threads.empty()), function ~CCheckQueue, file checkqueue.h, line 198.
18zsh: abort      ./src/test/test_bitcoin -t denialofservice_tests```

0$ FUZZ=process_message ./src/test/fuzz/fuzz
1
2...a few seconds later...
3
4in Peer::SetTxRelay() net_processing.cpp:305
5net_processing.cpp:307 SetTxRelay: Assertion `!m_tx_relay' failed.

vasild commented at 5:05 pm on August 14, 2023: contributor

What about calling FinalizeNode() from the destructor of CNode? This ensures it will be called exactly once by some thread after no other threads are referencing it. Is this not what we want?

   0diff --git i/src/net.h w/src/net.h
   1index 1ea0ad868a..587d907c3a 100644
   2--- i/src/net.h
   3+++ w/src/net.h
   4@@ -412,13 +412,12 @@ public:
   5     /** fSuccessfullyConnected is set to true on receiving VERACK from the peer. */
   6     std::atomic_bool fSuccessfullyConnected{false};
   7     // Setting fDisconnect to true will cause the node to be disconnected the
   8     // next time DisconnectNodes() runs
   9     std::atomic_bool fDisconnect{false};
  10     CSemaphoreGrant grantOutbound;
  11-    std::atomic<int> nRefCount{0};
  12 
  13     const uint64_t nKeyedNetGroup;
  14     std::atomic_bool fPauseRecv{false};
  15     std::atomic_bool fPauseSend{false};
  16 
  17     const ConnectionType m_conn_type;
  18@@ -566,30 +565,27 @@ public:
  19           uint64_t nKeyedNetGroupIn,
  20           uint64_t nLocalHostNonceIn,
  21           const CAddress& addrBindIn,
  22           const std::string& addrNameIn,
  23           ConnectionType conn_type_in,
  24           bool inbound_onion,
  25+          std::function<void(CNode&)> destruct_cb = {},
  26           CNodeOptions&& node_opts = {});
  27     CNode(const CNode&) = delete;
  28     CNode& operator=(const CNode&) = delete;
  29 
  30+    ~CNode();
  31+
  32     NodeId GetId() const {
  33         return id;
  34     }
  35 
  36     uint64_t GetLocalNonce() const {
  37         return nLocalHostNonce;
  38     }
  39 
  40-    int GetRefCount() const
  41-    {
  42-        assert(nRefCount >= 0);
  43-        return nRefCount;
  44-    }
  45-
  46     /**
  47      * Receive bytes from the buffer and deserialize them into messages.
  48      *
  49      * [@param](/bitcoin-bitcoin/contributor/param/)[in]   msg_bytes   The raw data
  50      * [@param](/bitcoin-bitcoin/contributor/param/)[out]  complete    Set True if at least one message has been
  51      *                          deserialized and is ready to be processed
  52@@ -609,23 +605,12 @@ public:
  53     }
  54 
  55     CService GetAddrLocal() const EXCLUSIVE_LOCKS_REQUIRED(!m_addr_local_mutex);
  56     //! May not be called more than once
  57     void SetAddrLocal(const CService& addrLocalIn) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_local_mutex);
  58 
  59-    CNode* AddRef()
  60-    {
  61-        nRefCount++;
  62-        return this;
  63-    }
  64-
  65-    void Release()
  66-    {
  67-        nRefCount--;
  68-    }
  69-
  70     void CloseSocketDisconnect() EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex);
  71 
  72     void CopyStats(CNodeStats& stats) EXCLUSIVE_LOCKS_REQUIRED(!m_subver_mutex, !m_addr_local_mutex, !cs_vSend, !cs_vRecv);
  73 
  74     std::string ConnectionTypeAsString() const { return ::ConnectionTypeAsString(m_conn_type); }
  75 
  76@@ -662,12 +647,17 @@ private:
  77      * the data socket is closed, the control socket is not going to be used anymore
  78      * and is just taking up resources. So better close it as soon as `m_sock` is
  79      * closed.
  80      * Otherwise this unique_ptr is empty.
  81      */
  82     std::unique_ptr<i2p::sam::Session> m_i2p_sam_session GUARDED_BY(m_sock_mutex);
  83+
  84+    /**
  85+     * A function to be called just before this object is destroyed.
  86+     */
  87+    std::function<void(CNode&)> m_destruct_cb;
  88 };
  89 
  90 /**
  91  * Interface for message handling
  92  */
  93 class NetEventsInterface
  94@@ -802,23 +792,23 @@ public:
  95 
  96     using NodeFn = std::function<void(CNode*)>;
  97     void ForEachNode(const NodeFn& func)
  98     {
  99         LOCK(m_nodes_mutex);
 100         for (auto&& node : m_nodes) {
 101-            if (NodeFullyConnected(node))
 102-                func(node);
 103+            if (NodeFullyConnected(*node))
 104+                func(node.get());
 105         }
 106     };
 107 
 108     void ForEachNode(const NodeFn& func) const
 109     {
 110         LOCK(m_nodes_mutex);
 111         for (auto&& node : m_nodes) {
 112-            if (NodeFullyConnected(node))
 113-                func(node);
 114+            if (NodeFullyConnected(*node))
 115+                func(node.get());
 116         }
 117     };
 118 
 119     // Addrman functions
 120     /**
 121      * Return all or many randomly selected addresses, optionally by network.
 122@@ -964,25 +954,25 @@ private:
 123 
 124     /**
 125      * Generate a collection of sockets to check for IO readiness.
 126      * [@param](/bitcoin-bitcoin/contributor/param/)[in] nodes Select from these nodes' sockets.
 127      * [@return](/bitcoin-bitcoin/contributor/return/) sockets to check for readiness
 128      */
 129-    Sock::EventsPerSock GenerateWaitSockets(Span<CNode* const> nodes);
 130+    Sock::EventsPerSock GenerateWaitSockets(const std::vector<std::shared_ptr<CNode>>& nodes);
 131 
 132     /**
 133      * Check connected and listening sockets for IO readiness and process them accordingly.
 134      */
 135     void SocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc);
 136 
 137     /**
 138      * Do the read/write for connected sockets that are ready for IO.
 139      * [@param](/bitcoin-bitcoin/contributor/param/)[in] nodes Nodes to process. The socket of each node is checked against `what`.
 140      * [@param](/bitcoin-bitcoin/contributor/param/)[in] events_per_sock Sockets that are ready for IO.
 141      */
 142-    void SocketHandlerConnected(const std::vector<CNode*>& nodes,
 143+    void SocketHandlerConnected(const std::vector<std::shared_ptr<CNode>>& nodes,
 144                                 const Sock::EventsPerSock& events_per_sock)
 145         EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc);
 146 
 147     /**
 148      * Accept incoming connections, one from each read-ready listening socket.
 149      * [@param](/bitcoin-bitcoin/contributor/param/)[in] events_per_sock Sockets that are ready for IO.
 150@@ -991,25 +981,25 @@ private:
 151 
 152     void ThreadSocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc);
 153     void ThreadDNSAddressSeed() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_nodes_mutex);
 154 
 155     uint64_t CalculateKeyedNetGroup(const CAddress& ad) const;
 156 
 157-    CNode* FindNode(const CNetAddr& ip);
 158-    CNode* FindNode(const CSubNet& subNet);
 159-    CNode* FindNode(const std::string& addrName);
 160-    CNode* FindNode(const CService& addr);
 161+    std::shared_ptr<CNode> FindNode(const CNetAddr& ip);
 162+    std::shared_ptr<CNode> FindNode(const CSubNet& subNet);
 163+    std::shared_ptr<CNode> FindNode(const std::string& addrName);
 164+    std::shared_ptr<CNode> FindNode(const CService& addr);
 165 
 166     /**
 167      * Determine whether we're already connected to a given address, in order to
 168      * avoid initiating duplicate connections.
 169      */
 170     bool AlreadyConnectedToAddress(const CAddress& addr);
 171 
 172     bool AttemptToEvictConnection();
 173-    CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex);
 174+    std::shared_ptr<CNode> ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex);
 175     void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const;
 176 
 177     void DeleteNode(CNode* pnode);
 178 
 179     NodeId GetNewNodeId();
 180 
 181@@ -1041,13 +1031,13 @@ private:
 182      *
 183      * [@return](/bitcoin-bitcoin/contributor/return/)           bool        Whether a preferred network was found.
 184      */
 185     bool MaybePickPreferredNetwork(std::optional<Network>& network);
 186 
 187     // Whether the node should be passed out in ForEach* callbacks
 188-    static bool NodeFullyConnected(const CNode* pnode);
 189+    static bool NodeFullyConnected(const CNode& node);
 190 
 191     // Network usage totals
 192     mutable Mutex m_total_bytes_sent_mutex;
 193     std::atomic<uint64_t> nTotalBytesRecv{0};
 194     uint64_t nTotalBytesSent GUARDED_BY(m_total_bytes_sent_mutex) {0};
 195 
 196@@ -1072,14 +1062,13 @@ private:
 197     AddrMan& addrman;
 198     const NetGroupManager& m_netgroupman;
 199     std::deque<std::string> m_addr_fetches GUARDED_BY(m_addr_fetches_mutex);
 200     Mutex m_addr_fetches_mutex;
 201     std::vector<std::string> m_added_nodes GUARDED_BY(m_added_nodes_mutex);
 202     mutable Mutex m_added_nodes_mutex;
 203-    std::vector<CNode*> m_nodes GUARDED_BY(m_nodes_mutex);
 204-    std::list<CNode*> m_nodes_disconnected;
 205+    std::vector<std::shared_ptr<CNode>> m_nodes GUARDED_BY(m_nodes_mutex);
 206     mutable RecursiveMutex m_nodes_mutex;
 207     std::atomic<NodeId> nLastNodeId{0};
 208     unsigned int nPrevNodeCount{0};
 209 
 210     // Stores number of full-tx connections (outbound and manual) per network
 211     std::array<unsigned int, Network::NET_MAX> m_network_conn_counts GUARDED_BY(m_nodes_mutex) = {};
 212@@ -1227,35 +1216,25 @@ private:
 213     public:
 214         explicit NodesSnapshot(const CConnman& connman, bool shuffle)
 215         {
 216             {
 217                 LOCK(connman.m_nodes_mutex);
 218                 m_nodes_copy = connman.m_nodes;
 219-                for (auto& node : m_nodes_copy) {
 220-                    node->AddRef();
 221-                }
 222             }
 223             if (shuffle) {
 224                 Shuffle(m_nodes_copy.begin(), m_nodes_copy.end(), FastRandomContext{});
 225             }
 226         }
 227 
 228-        ~NodesSnapshot()
 229-        {
 230-            for (auto& node : m_nodes_copy) {
 231-                node->Release();
 232-            }
 233-        }
 234-
 235-        const std::vector<CNode*>& Nodes() const
 236+        const std::vector<std::shared_ptr<CNode>>& Nodes() const
 237         {
 238             return m_nodes_copy;
 239         }
 240 
 241     private:
 242-        std::vector<CNode*> m_nodes_copy;
 243+        std::vector<std::shared_ptr<CNode>> m_nodes_copy;
 244     };
 245 
 246     friend struct ConnmanTestMsg;
 247 };
 248 
 249 /** Dump binary message to file, with timestamp */
 250diff --git i/src/net.cpp w/src/net.cpp
 251index b51043ba27..c1ad1166f3 100644
 252--- i/src/net.cpp
 253+++ w/src/net.cpp
 254@@ -359,49 +359,49 @@ bool SeenLocal(const CService& addr)
 255 bool IsLocal(const CService& addr)
 256 {
 257     LOCK(g_maplocalhost_mutex);
 258     return mapLocalHost.count(addr) > 0;
 259 }
 260 
 261-CNode* CConnman::FindNode(const CNetAddr& ip)
 262+std::shared_ptr<CNode> CConnman::FindNode(const CNetAddr& ip)
 263 {
 264     LOCK(m_nodes_mutex);
 265-    for (CNode* pnode : m_nodes) {
 266+    for (auto& pnode : m_nodes) {
 267       if (static_cast<CNetAddr>(pnode->addr) == ip) {
 268             return pnode;
 269         }
 270     }
 271     return nullptr;
 272 }
 273 
 274-CNode* CConnman::FindNode(const CSubNet& subNet)
 275+std::shared_ptr<CNode> CConnman::FindNode(const CSubNet& subNet)
 276 {
 277     LOCK(m_nodes_mutex);
 278-    for (CNode* pnode : m_nodes) {
 279+    for (auto& pnode : m_nodes) {
 280         if (subNet.Match(static_cast<CNetAddr>(pnode->addr))) {
 281             return pnode;
 282         }
 283     }
 284     return nullptr;
 285 }
 286 
 287-CNode* CConnman::FindNode(const std::string& addrName)
 288+std::shared_ptr<CNode> CConnman::FindNode(const std::string& addrName)
 289 {
 290     LOCK(m_nodes_mutex);
 291-    for (CNode* pnode : m_nodes) {
 292+    for (auto& pnode : m_nodes) {
 293         if (pnode->m_addr_name == addrName) {
 294             return pnode;
 295         }
 296     }
 297     return nullptr;
 298 }
 299 
 300-CNode* CConnman::FindNode(const CService& addr)
 301+std::shared_ptr<CNode> CConnman::FindNode(const CService& addr)
 302 {
 303     LOCK(m_nodes_mutex);
 304-    for (CNode* pnode : m_nodes) {
 305+    for (auto& pnode : m_nodes) {
 306         if (static_cast<CService>(pnode->addr) == addr) {
 307             return pnode;
 308         }
 309     }
 310     return nullptr;
 311 }
 312@@ -411,13 +411,13 @@ bool CConnman::AlreadyConnectedToAddress(const CAddress& addr)
 313     return FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort());
 314 }
 315 
 316 bool CConnman::CheckIncomingNonce(uint64_t nonce)
 317 {
 318     LOCK(m_nodes_mutex);
 319-    for (const CNode* pnode : m_nodes) {
 320+    for (const auto& pnode : m_nodes) {
 321         if (!pnode->fSuccessfullyConnected && !pnode->IsInboundConn() && pnode->GetLocalNonce() == nonce)
 322             return false;
 323     }
 324     return true;
 325 }
 326 
 327@@ -434,25 +434,23 @@ static CAddress GetBindAddress(const Sock& sock)
 328             LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
 329         }
 330     }
 331     return addr_bind;
 332 }
 333 
 334-CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type)
 335+std::shared_ptr<CNode> CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type)
 336 {
 337     AssertLockNotHeld(m_unused_i2p_sessions_mutex);
 338     assert(conn_type != ConnectionType::INBOUND);
 339 
 340     if (pszDest == nullptr) {
 341         if (IsLocal(addrConnect))
 342             return nullptr;
 343 
 344         // Look for an existing connection
 345-        CNode* pnode = FindNode(static_cast<CService>(addrConnect));
 346-        if (pnode)
 347-        {
 348+        if (FindNode(static_cast<CService>(addrConnect))) {
 349             LogPrintf("Failed to open new connection, already connected\n");
 350             return nullptr;
 351         }
 352     }
 353 
 354     LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "trying connection %s lastseen=%.1fhrs\n",
 355@@ -471,14 +469,13 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 356                 LogPrint(BCLog::NET, "Resolver returned invalid address %s for %s\n", addrConnect.ToStringAddrPort(), pszDest);
 357                 return nullptr;
 358             }
 359             // It is possible that we already have a connection to the IP/port pszDest resolved to.
 360             // In that case, drop the connection that was just created.
 361             LOCK(m_nodes_mutex);
 362-            CNode* pnode = FindNode(static_cast<CService>(addrConnect));
 363-            if (pnode) {
 364+            if (FindNode(static_cast<CService>(addrConnect))) {
 365                 LogPrintf("Failed to open new connection, already connected\n");
 366                 return nullptr;
 367             }
 368         }
 369     }
 370 
 371@@ -563,26 +560,26 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 372     // Add node
 373     NodeId id = GetNewNodeId();
 374     uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize();
 375     if (!addr_bind.IsValid()) {
 376         addr_bind = GetBindAddress(*sock);
 377     }
 378-    CNode* pnode = new CNode(id,
 379+    auto pnode = std::make_shared<CNode>(id,
 380                              std::move(sock),
 381                              addrConnect,
 382                              CalculateKeyedNetGroup(addrConnect),
 383                              nonce,
 384                              addr_bind,
 385                              pszDest ? pszDest : "",
 386                              conn_type,
 387                              /*inbound_onion=*/false,
 388+                             [this](CNode& node) {m_msgproc->FinalizeNode(node);},
 389                              CNodeOptions{
 390                                  .i2p_sam_session = std::move(i2p_transient_session),
 391                                  .recv_flood_size = nReceiveFloodSize,
 392                              });
 393-    pnode->AddRef();
 394 
 395     // We're making a new connection, harvest entropy from the time (and our peer count)
 396     RandAddEvent((uint32_t)id);
 397 
 398     return pnode;
 399 }
 400@@ -902,13 +899,13 @@ size_t CConnman::SocketSendData(CNode& node) const
 401 bool CConnman::AttemptToEvictConnection()
 402 {
 403     std::vector<NodeEvictionCandidate> vEvictionCandidates;
 404     {
 405 
 406         LOCK(m_nodes_mutex);
 407-        for (const CNode* node : m_nodes) {
 408+        for (const auto& node : m_nodes) {
 409             if (node->fDisconnect)
 410                 continue;
 411             NodeEvictionCandidate candidate{
 412                 .id = node->GetId(),
 413                 .m_connected = node->m_connected,
 414                 .m_min_ping_time = node->m_min_ping_time,
 415@@ -929,13 +926,13 @@ bool CConnman::AttemptToEvictConnection()
 416     }
 417     const std::optional<NodeId> node_id_to_evict = SelectNodeToEvict(std::move(vEvictionCandidates));
 418     if (!node_id_to_evict) {
 419         return false;
 420     }
 421     LOCK(m_nodes_mutex);
 422-    for (CNode* pnode : m_nodes) {
 423+    for (const auto& pnode : m_nodes) {
 424         if (pnode->GetId() == *node_id_to_evict) {
 425             LogPrint(BCLog::NET, "selected %s connection for eviction peer=%d; disconnecting\n", pnode->ConnectionTypeAsString(), pnode->GetId());
 426             pnode->fDisconnect = true;
 427             return true;
 428         }
 429     }
 430@@ -986,13 +983,13 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
 431         NetPermissions::AddFlag(permission_flags, NetPermissionFlags::Mempool);
 432         NetPermissions::AddFlag(permission_flags, NetPermissionFlags::NoBan);
 433     }
 434 
 435     {
 436         LOCK(m_nodes_mutex);
 437-        for (const CNode* pnode : m_nodes) {
 438+        for (const auto& pnode : m_nodes) {
 439             if (pnode->IsInboundConn()) nInbound++;
 440         }
 441     }
 442 
 443     if (!fNetworkActive) {
 444         LogPrint(BCLog::NET, "connection from %s dropped: not accepting new connections\n", addr.ToStringAddrPort());
 445@@ -1043,27 +1040,27 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
 446     ServiceFlags nodeServices = nLocalServices;
 447     if (NetPermissions::HasFlag(permission_flags, NetPermissionFlags::BloomFilter)) {
 448         nodeServices = static_cast<ServiceFlags>(nodeServices | NODE_BLOOM);
 449     }
 450 
 451     const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end();
 452-    CNode* pnode = new CNode(id,
 453+    auto pnode = std::make_shared<CNode>(id,
 454                              std::move(sock),
 455                              addr,
 456                              CalculateKeyedNetGroup(addr),
 457                              nonce,
 458                              addr_bind,
 459                              /*addrNameIn=*/"",
 460                              ConnectionType::INBOUND,
 461                              inbound_onion,
 462+                             [this](CNode& node) {m_msgproc->FinalizeNode(node);},
 463                              CNodeOptions{
 464                                  .permission_flags = permission_flags,
 465                                  .prefer_evict = discouraged,
 466                                  .recv_flood_size = nReceiveFloodSize,
 467                              });
 468-    pnode->AddRef();
 469     m_msgproc->InitializeNode(*pnode, nodeServices);
 470 
 471     LogPrint(BCLog::NET, "connection from %s accepted\n", addr.ToStringAddrPort());
 472 
 473     {
 474         LOCK(m_nodes_mutex);
 475@@ -1095,13 +1092,13 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ
 476     case ConnectionType::FEELER:
 477         break;
 478     } // no default case, so the compiler can warn about missing cases
 479 
 480     // Count existing connections
 481     int existing_connections = WITH_LOCK(m_nodes_mutex,
 482-                                         return std::count_if(m_nodes.begin(), m_nodes.end(), [conn_type](CNode* node) { return node->m_conn_type == conn_type; }););
 483+                                         return std::count_if(m_nodes.begin(), m_nodes.end(), [conn_type](std::shared_ptr<CNode> node) { return node->m_conn_type == conn_type; }););
 484 
 485     // Max connections of specified type already exist
 486     if (max_connections != std::nullopt && existing_connections >= max_connections) return false;
 487 
 488     // Max total outbound connections already exist
 489     CSemaphoreGrant grant(*semOutbound, true);
 490@@ -1110,58 +1107,50 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ
 491     OpenNetworkConnection(CAddress(), false, &grant, address.c_str(), conn_type);
 492     return true;
 493 }
 494 
 495 void CConnman::DisconnectNodes()
 496 {
 497+    std::vector<std::shared_ptr<CNode>> disconnected_nodes;
 498+
 499     {
 500         LOCK(m_nodes_mutex);
 501 
 502         if (!fNetworkActive) {
 503             // Disconnect any connected nodes
 504-            for (CNode* pnode : m_nodes) {
 505+            for (const auto& pnode : m_nodes) {
 506                 if (!pnode->fDisconnect) {
 507                     LogPrint(BCLog::NET, "Network not active, dropping peer=%d\n", pnode->GetId());
 508                     pnode->fDisconnect = true;
 509                 }
 510             }
 511         }
 512 
 513         // Disconnect unused nodes
 514-        std::vector<CNode*> nodes_copy = m_nodes;
 515-        for (CNode* pnode : nodes_copy)
 516-        {
 517-            if (pnode->fDisconnect)
 518-            {
 519+        for (auto it = m_nodes.begin(); it != m_nodes.end();) {
 520+            auto node = *it;
 521+            if (node->fDisconnect) {
 522                 // remove from m_nodes
 523-                m_nodes.erase(remove(m_nodes.begin(), m_nodes.end(), pnode), m_nodes.end());
 524+                it = m_nodes.erase(it);
 525+
 526+                // Keep a reference to this CNode object to delay its destruction until
 527+                // after m_nodes_mutex has been released. Destructing a node involves
 528+                // calling m_msgproc->FinalizeNode() which acquires cs_main. Lock order
 529+                // should be cs_main, m_nodes_mutex.
 530+                disconnected_nodes.push_back(node);
 531 
 532                 // release outbound grant (if any)
 533-                pnode->grantOutbound.Release();
 534+                node->grantOutbound.Release();
 535 
 536                 // close socket and cleanup
 537-                pnode->CloseSocketDisconnect();
 538+                node->CloseSocketDisconnect();
 539 
 540                 // update connection count by network
 541-                if (pnode->IsManualOrFullOutboundConn()) --m_network_conn_counts[pnode->addr.GetNetwork()];
 542-
 543-                // hold in disconnected pool until all refs are released
 544-                pnode->Release();
 545-                m_nodes_disconnected.push_back(pnode);
 546-            }
 547-        }
 548-    }
 549-    {
 550-        // Delete disconnected nodes
 551-        std::list<CNode*> nodes_disconnected_copy = m_nodes_disconnected;
 552-        for (CNode* pnode : nodes_disconnected_copy)
 553-        {
 554-            // Destroy the object only after other threads have stopped using it.
 555-            if (pnode->GetRefCount() <= 0) {
 556-                m_nodes_disconnected.remove(pnode);
 557-                DeleteNode(pnode);
 558+                if (node->IsManualOrFullOutboundConn()) --m_network_conn_counts[node->addr.GetNetwork()];
 559+            } else {
 560+                ++it;
 561             }
 562         }
 563     }
 564 }
 565 
 566 void CConnman::NotifyNumConnectionsChanged()
 567@@ -1214,21 +1203,21 @@ bool CConnman::InactivityCheck(const CNode& node) const
 568         return true;
 569     }
 570 
 571     return false;
 572 }
 573 
 574-Sock::EventsPerSock CConnman::GenerateWaitSockets(Span<CNode* const> nodes)
 575+Sock::EventsPerSock CConnman::GenerateWaitSockets(const std::vector<std::shared_ptr<CNode>>& nodes)
 576 {
 577     Sock::EventsPerSock events_per_sock;
 578 
 579     for (const ListenSocket& hListenSocket : vhListenSocket) {
 580         events_per_sock.emplace(hListenSocket.sock, Sock::Events{Sock::RECV});
 581     }
 582 
 583-    for (CNode* pnode : nodes) {
 584+    for (auto& pnode : nodes) {
 585         // Implement the following logic:
 586         // * If there is data to send, select() for sending data. As this only
 587         //   happens when optimistic write failed, we choose to first drain the
 588         //   write buffer in this case before receiving more. This avoids
 589         //   needlessly queueing received data, if the remote peer is not themselves
 590         //   receiving data. This means properly utilizing TCP flow control signalling.
 591@@ -1287,18 +1276,18 @@ void CConnman::SocketHandler()
 592     }
 593 
 594     // Accept new connections from listening sockets.
 595     SocketHandlerListening(events_per_sock);
 596 }
 597 
 598-void CConnman::SocketHandlerConnected(const std::vector<CNode*>& nodes,
 599+void CConnman::SocketHandlerConnected(const std::vector<std::shared_ptr<CNode>>& nodes,
 600                                       const Sock::EventsPerSock& events_per_sock)
 601 {
 602     AssertLockNotHeld(m_total_bytes_sent_mutex);
 603 
 604-    for (CNode* pnode : nodes) {
 605+    for (auto& pnode : nodes) {
 606         if (interruptNet)
 607             return;
 608 
 609         //
 610         // Receive
 611         //
 612@@ -1454,13 +1443,13 @@ void CConnman::ThreadDNSAddressSeed()
 613                     if (!interruptNet.sleep_for(w)) return;
 614                     to_wait -= w;
 615 
 616                     int nRelevant = 0;
 617                     {
 618                         LOCK(m_nodes_mutex);
 619-                        for (const CNode* pnode : m_nodes) {
 620+                        for (const auto& pnode : m_nodes) {
 621                             if (pnode->fSuccessfullyConnected && pnode->IsFullOutboundConn()) ++nRelevant;
 622                         }
 623                     }
 624                     if (nRelevant >= 2) {
 625                         if (found > 0) {
 626                             LogPrintf("%d addresses found from DNS seeds\n", found);
 627@@ -1572,13 +1561,13 @@ void CConnman::StartExtraBlockRelayPeers()
 628 // evict some peer that has finished the handshake)
 629 int CConnman::GetExtraFullOutboundCount() const
 630 {
 631     int full_outbound_peers = 0;
 632     {
 633         LOCK(m_nodes_mutex);
 634-        for (const CNode* pnode : m_nodes) {
 635+        for (const auto& pnode : m_nodes) {
 636             if (pnode->fSuccessfullyConnected && !pnode->fDisconnect && pnode->IsFullOutboundConn()) {
 637                 ++full_outbound_peers;
 638             }
 639         }
 640     }
 641     return std::max(full_outbound_peers - m_max_outbound_full_relay, 0);
 642@@ -1586,13 +1575,13 @@ int CConnman::GetExtraFullOutboundCount() const
 643 
 644 int CConnman::GetExtraBlockRelayCount() const
 645 {
 646     int block_relay_peers = 0;
 647     {
 648         LOCK(m_nodes_mutex);
 649-        for (const CNode* pnode : m_nodes) {
 650+        for (const auto& pnode : m_nodes) {
 651             if (pnode->fSuccessfullyConnected && !pnode->fDisconnect && pnode->IsBlockOnlyConn()) {
 652                 ++block_relay_peers;
 653             }
 654         }
 655     }
 656     return std::max(block_relay_peers - m_max_outbound_block_relay, 0);
 657@@ -1734,13 +1723,13 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
 658         int nOutboundBlockRelay = 0;
 659         int outbound_privacy_network_peers = 0;
 660         std::set<std::vector<unsigned char>> outbound_ipv46_peer_netgroups;
 661 
 662         {
 663             LOCK(m_nodes_mutex);
 664-            for (const CNode* pnode : m_nodes) {
 665+            for (const auto& pnode : m_nodes) {
 666                 if (pnode->IsFullOutboundConn()) nOutboundFullRelay++;
 667                 if (pnode->IsBlockOnlyConn()) nOutboundBlockRelay++;
 668 
 669                 // Make sure our persistent outbound slots to ipv4/ipv6 peers belong to different netgroups.
 670                 switch (pnode->m_conn_type) {
 671                     // We currently don't take inbound connections into account. Since they are
 672@@ -1954,13 +1943,13 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
 673 }
 674 
 675 std::vector<CAddress> CConnman::GetCurrentBlockRelayOnlyConns() const
 676 {
 677     std::vector<CAddress> ret;
 678     LOCK(m_nodes_mutex);
 679-    for (const CNode* pnode : m_nodes) {
 680+    for (const auto& pnode : m_nodes) {
 681         if (pnode->IsBlockOnlyConn()) {
 682             ret.push_back(pnode->addr);
 683         }
 684     }
 685 
 686     return ret;
 687@@ -1980,13 +1969,13 @@ std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
 688 
 689     // Build a map of all already connected addresses (by IP:port and by name) to inbound/outbound and resolved CService
 690     std::map<CService, bool> mapConnected;
 691     std::map<std::string, std::pair<bool, CService>> mapConnectedByName;
 692     {
 693         LOCK(m_nodes_mutex);
 694-        for (const CNode* pnode : m_nodes) {
 695+        for (const auto& pnode : m_nodes) {
 696             if (pnode->addr.IsValid()) {
 697                 mapConnected[pnode->addr] = pnode->IsInboundConn();
 698             }
 699             std::string addrName{pnode->m_addr_name};
 700             if (!addrName.empty()) {
 701                 mapConnectedByName[std::move(addrName)] = std::make_pair(pnode->IsInboundConn(), static_cast<const CService&>(pnode->addr));
 702@@ -2068,13 +2057,13 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
 703         if (IsLocal(addrConnect) || banned_or_discouraged || AlreadyConnectedToAddress(addrConnect)) {
 704             return;
 705         }
 706     } else if (FindNode(std::string(pszDest)))
 707         return;
 708 
 709-    CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure, conn_type);
 710+    auto pnode = ConnectNode(addrConnect, pszDest, fCountFailure, conn_type);
 711 
 712     if (!pnode)
 713         return;
 714     if (grantOutbound)
 715         grantOutbound->MoveTo(pnode->grantOutbound);
 716 
 717@@ -2101,23 +2090,23 @@ void CConnman::ThreadMessageHandler()
 718         {
 719             // Randomize the order in which we process messages from/to our peers.
 720             // This prevents attacks in which an attacker exploits having multiple
 721             // consecutive connections in the m_nodes list.
 722             const NodesSnapshot snap{*this, /*shuffle=*/true};
 723 
 724-            for (CNode* pnode : snap.Nodes()) {
 725+            for (auto& pnode : snap.Nodes()) {
 726                 if (pnode->fDisconnect)
 727                     continue;
 728 
 729                 // Receive messages
 730-                bool fMoreNodeWork = m_msgproc->ProcessMessages(pnode, flagInterruptMsgProc);
 731+                bool fMoreNodeWork = m_msgproc->ProcessMessages(pnode.get(), flagInterruptMsgProc);
 732                 fMoreWork |= (fMoreNodeWork && !pnode->fPauseSend);
 733                 if (flagInterruptMsgProc)
 734                     return;
 735                 // Send messages
 736-                m_msgproc->SendMessages(pnode);
 737+                m_msgproc->SendMessages(pnode.get());
 738 
 739                 if (flagInterruptMsgProc)
 740                     return;
 741             }
 742         }
 743 
 744@@ -2531,35 +2520,24 @@ void CConnman::StopNodes()
 745             }
 746             DumpAnchors(gArgs.GetDataDirNet() / ANCHORS_DATABASE_FILENAME, anchors_to_dump);
 747         }
 748     }
 749 
 750     // Delete peer connections.
 751-    std::vector<CNode*> nodes;
 752+    std::vector<std::shared_ptr<CNode>> nodes;
 753     WITH_LOCK(m_nodes_mutex, nodes.swap(m_nodes));
 754-    for (CNode* pnode : nodes) {
 755-        pnode->CloseSocketDisconnect();
 756-        DeleteNode(pnode);
 757+    for (auto& node : nodes) {
 758+        node->CloseSocketDisconnect();
 759     }
 760+    nodes.clear();
 761 
 762-    for (CNode* pnode : m_nodes_disconnected) {
 763-        DeleteNode(pnode);
 764-    }
 765-    m_nodes_disconnected.clear();
 766     vhListenSocket.clear();
 767     semOutbound.reset();
 768     semAddnode.reset();
 769 }
 770 
 771-void CConnman::DeleteNode(CNode* pnode)
 772-{
 773-    assert(pnode);
 774-    m_msgproc->FinalizeNode(*pnode);
 775-    delete pnode;
 776-}
 777-
 778 CConnman::~CConnman()
 779 {
 780     Interrupt();
 781     Stop();
 782 }
 783 
 784@@ -2664,35 +2642,35 @@ uint32_t CConnman::GetMappedAS(const CNetAddr& addr) const
 785 
 786 void CConnman::GetNodeStats(std::vector<CNodeStats>& vstats) const
 787 {
 788     vstats.clear();
 789     LOCK(m_nodes_mutex);
 790     vstats.reserve(m_nodes.size());
 791-    for (CNode* pnode : m_nodes) {
 792+    for (const auto& pnode : m_nodes) {
 793         vstats.emplace_back();
 794         pnode->CopyStats(vstats.back());
 795         vstats.back().m_mapped_as = GetMappedAS(pnode->addr);
 796     }
 797 }
 798 
 799 bool CConnman::DisconnectNode(const std::string& strNode)
 800 {
 801     LOCK(m_nodes_mutex);
 802-    if (CNode* pnode = FindNode(strNode)) {
 803+    if (auto pnode = FindNode(strNode)) {
 804         LogPrint(BCLog::NET, "disconnect by address%s matched peer=%d; disconnecting\n", (fLogIPs ? strprintf("=%s", strNode) : ""), pnode->GetId());
 805         pnode->fDisconnect = true;
 806         return true;
 807     }
 808     return false;
 809 }
 810 
 811 bool CConnman::DisconnectNode(const CSubNet& subnet)
 812 {
 813     bool disconnected = false;
 814     LOCK(m_nodes_mutex);
 815-    for (CNode* pnode : m_nodes) {
 816+    for (auto& pnode : m_nodes) {
 817         if (subnet.Match(pnode->addr)) {
 818             LogPrint(BCLog::NET, "disconnect by subnet%s matched peer=%d; disconnecting\n", (fLogIPs ? strprintf("=%s", subnet.ToString()) : ""), pnode->GetId());
 819             pnode->fDisconnect = true;
 820             disconnected = true;
 821         }
 822     }
 823@@ -2704,13 +2682,13 @@ bool CConnman::DisconnectNode(const CNetAddr& addr)
 824     return DisconnectNode(CSubNet(addr));
 825 }
 826 
 827 bool CConnman::DisconnectNode(NodeId id)
 828 {
 829     LOCK(m_nodes_mutex);
 830-    for(CNode* pnode : m_nodes) {
 831+    for(auto& pnode : m_nodes) {
 832         if (id == pnode->GetId()) {
 833             LogPrint(BCLog::NET, "disconnect by id peer=%d; disconnecting\n", pnode->GetId());
 834             pnode->fDisconnect = true;
 835             return true;
 836         }
 837     }
 838@@ -2828,12 +2806,13 @@ CNode::CNode(NodeId idIn,
 839              uint64_t nKeyedNetGroupIn,
 840              uint64_t nLocalHostNonceIn,
 841              const CAddress& addrBindIn,
 842              const std::string& addrNameIn,
 843              ConnectionType conn_type_in,
 844              bool inbound_onion,
 845+             std::function<void(CNode&)> destruct_cb,
 846              CNodeOptions&& node_opts)
 847     : m_deserializer{std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION))},
 848       m_serializer{std::make_unique<V1TransportSerializer>(V1TransportSerializer())},
 849       m_permission_flags{node_opts.permission_flags},
 850       m_sock{sock},
 851       m_connected{GetTime<std::chrono::seconds>()},
 852@@ -2844,13 +2823,14 @@ CNode::CNode(NodeId idIn,
 853       m_prefer_evict{node_opts.prefer_evict},
 854       nKeyedNetGroup{nKeyedNetGroupIn},
 855       m_conn_type{conn_type_in},
 856       id{idIn},
 857       nLocalHostNonce{nLocalHostNonceIn},
 858       m_recv_flood_size{node_opts.recv_flood_size},
 859-      m_i2p_sam_session{std::move(node_opts.i2p_sam_session)}
 860+      m_i2p_sam_session{std::move(node_opts.i2p_sam_session)},
 861+      m_destruct_cb{destruct_cb}
 862 {
 863     if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND);
 864 
 865     for (const std::string &msg : getAllNetMessageTypes())
 866         mapRecvBytesPerMsgType[msg] = 0;
 867     mapRecvBytesPerMsgType[NET_MESSAGE_TYPE_OTHER] = 0;
 868@@ -2859,12 +2839,19 @@ CNode::CNode(NodeId idIn,
 869         LogPrint(BCLog::NET, "Added connection to %s peer=%d\n", m_addr_name, id);
 870     } else {
 871         LogPrint(BCLog::NET, "Added connection peer=%d\n", id);
 872     }
 873 }
 874 
 875+CNode::~CNode()
 876+{
 877+    if (m_destruct_cb) {
 878+        m_destruct_cb(*this);
 879+    }
 880+}
 881+
 882 void CNode::MarkReceivedMsgsForProcessing()
 883 {
 884     AssertLockNotHeld(m_msg_process_queue_mutex);
 885 
 886     size_t nSizeAdded = 0;
 887     for (const auto& msg : vRecvMsg) {
 888@@ -2890,15 +2877,15 @@ std::optional<std::pair<CNetMessage, bool>> CNode::PollMessage()
 889     m_msg_process_queue_size -= msgs.front().m_raw_message_size;
 890     fPauseRecv = m_msg_process_queue_size > m_recv_flood_size;
 891 
 892     return std::make_pair(std::move(msgs.front()), !m_msg_process_queue.empty());
 893 }
 894 
 895-bool CConnman::NodeFullyConnected(const CNode* pnode)
 896+bool CConnman::NodeFullyConnected(const CNode& node)
 897 {
 898-    return pnode && pnode->fSuccessfullyConnected && !pnode->fDisconnect;
 899+    return node.fSuccessfullyConnected && !node.fDisconnect;
 900 }
 901 
 902 void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
 903 {
 904     AssertLockNotHeld(m_total_bytes_sent_mutex);
 905     size_t nMessageSize = msg.data.size();
 906@@ -2939,21 +2926,19 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
 907     }
 908     if (nBytesSent) RecordBytesSent(nBytesSent);
 909 }
 910 
 911 bool CConnman::ForNode(NodeId id, std::function<bool(CNode* pnode)> func)
 912 {
 913-    CNode* found = nullptr;
 914     LOCK(m_nodes_mutex);
 915     for (auto&& pnode : m_nodes) {
 916         if(pnode->GetId() == id) {
 917-            found = pnode;
 918-            break;
 919+            return NodeFullyConnected(*pnode) && func(pnode.get());
 920         }
 921     }
 922-    return found != nullptr && NodeFullyConnected(found) && func(found);
 923+    return false;
 924 }
 925 
 926 CSipHasher CConnman::GetDeterministicRandomizer(uint64_t id) const
 927 {
 928     return CSipHasher(nSeed0, nSeed1).Write(id);
 929 }
 930diff --git i/src/test/fuzz/net.cpp w/src/test/fuzz/net.cpp
 931index ddf919f2e6..19db2c7c50 100644
 932--- i/src/test/fuzz/net.cpp
 933+++ w/src/test/fuzz/net.cpp
 934@@ -40,21 +40,12 @@ FUZZ_TARGET(net, .init = initialize_net)
 935                 node.CloseSocketDisconnect();
 936             },
 937             [&] {
 938                 CNodeStats stats;
 939                 node.CopyStats(stats);
 940             },
 941-            [&] {
 942-                const CNode* add_ref_node = node.AddRef();
 943-                assert(add_ref_node == &node);
 944-            },
 945-            [&] {
 946-                if (node.GetRefCount() > 0) {
 947-                    node.Release();
 948-                }
 949-            },
 950             [&] {
 951                 const std::optional<CService> service_opt = ConsumeDeserializable<CService>(fuzzed_data_provider);
 952                 if (!service_opt) {
 953                     return;
 954                 }
 955                 node.SetAddrLocal(*service_opt);
 956@@ -66,14 +57,12 @@ FUZZ_TARGET(net, .init = initialize_net)
 957             });
 958     }
 959 
 960     (void)node.GetAddrLocal();
 961     (void)node.GetId();
 962     (void)node.GetLocalNonce();
 963-    const int ref_count = node.GetRefCount();
 964-    assert(ref_count >= 0);
 965     (void)node.GetCommonVersion();
 966 
 967     const NetPermissionFlags net_permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
 968     (void)node.HasPermission(net_permission_flags);
 969     (void)node.ConnectedThroughNetwork();
 970 }
 971diff --git i/src/test/fuzz/util/net.h w/src/test/fuzz/util/net.h
 972index 47e4a2fac0..bc68018cdb 100644
 973--- i/src/test/fuzz/util/net.h
 974+++ w/src/test/fuzz/util/net.h
 975@@ -117,23 +117,25 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
 976                                        keyed_net_group,
 977                                        local_host_nonce,
 978                                        addr_bind,
 979                                        addr_name,
 980                                        conn_type,
 981                                        inbound_onion,
 982+                                       [](CNode&){},
 983                                        CNodeOptions{ .permission_flags = permission_flags });
 984     } else {
 985         return CNode{node_id,
 986                      sock,
 987                      address,
 988                      keyed_net_group,
 989                      local_host_nonce,
 990                      addr_bind,
 991                      addr_name,
 992                      conn_type,
 993                      inbound_onion,
 994+                     {},
 995                      CNodeOptions{ .permission_flags = permission_flags }};
 996     }
 997 }
 998 inline std::unique_ptr<CNode> ConsumeNodeAsUniquePtr(FuzzedDataProvider& fdp, const std::optional<NodeId>& node_id_in = std::nullopt) { return ConsumeNode<true>(fdp, node_id_in); }
 999 
1000 void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, CNode& node) noexcept EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
1001diff --git i/src/test/util/net.h w/src/test/util/net.h
1002index b2f6ebb163..e90e3d8401 100644
1003--- i/src/test/util/net.h
1004+++ w/src/test/util/net.h
1005@@ -25,23 +25,20 @@ struct ConnmanTestMsg : public CConnman {
1006         m_peer_connect_timeout = timeout;
1007     }
1008 
1009     void AddTestNode(CNode& node)
1010     {
1011         LOCK(m_nodes_mutex);
1012-        m_nodes.push_back(&node);
1013+        m_nodes.push_back(std::shared_ptr<CNode>(&node));
1014 
1015         if (node.IsManualOrFullOutboundConn()) ++m_network_conn_counts[node.addr.GetNetwork()];
1016     }
1017 
1018     void ClearTestNodes()
1019     {
1020         LOCK(m_nodes_mutex);
1021-        for (CNode* node : m_nodes) {
1022-            delete node;
1023-        }
1024         m_nodes.clear();
1025     }
1026 
1027     void Handshake(CNode& node,
1028                    bool successfully_connected,
1029                    ServiceFlags remote_services,

DrahtBot added the label Needs rebase on Aug 17, 2023

DrahtBot commented at 1:49 pm on August 17, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

in src/net.h:801 in 1f612cded8 outdated

797@@ -798,11 +798,11 @@ class CConnman
798     // alias for thread safety annotations only, not defined
799     RecursiveMutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex);
800 
801-    bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);
802+    bool ForNode(NodeId id, std::function<bool(CNodeRef pnode)> func);

ajtowns commented at 2:01 pm on September 3, 2023:

I don’t think this makes sense in general? If you have a shared_ptr, you should normally just pass the reference to the object (CNode&) if you’ve checked it’s not null, or the pointer itself (CNode*).

cf http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-smart

DrahtBot commented at 4:09 pm on November 13, 2023: contributor

Are you still working on this?

DrahtBot commented at 1:20 am on February 11, 2024: contributor

⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 9:19 am on May 13, 2024: contributor

⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 9:19 am on May 13, 2024: contributor

🤔 There hasn’t been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

willcl-ark commented at 1:56 pm on May 15, 2024: member

Closing for now

willcl-ark closed this on May 15, 2024

Use shared_ptr for CNode inside CConnman #28222

Reviews

Conflicts