Merge e44e669378 into c5e44a0435

32d55e28af test: Use the correct node for doubled keypath test (Ava Chow)

Pull request description:

  #29124 had a silent merge conflict with #32350 which resulted in it using the wrong node. Fix the test to use the correct v22 node.

ACKs for top commit:
  maflcko:
    lgtm ACK 32d55e28af
  rkrux:
    ACK 32d55e28af
  BrandonOdiwuor:
    Code Review ACK 32d55e28af

Tree-SHA512: 1e0231985beb382b16e1d608c874750423d0502388db0c8ad450b22d17f9d96f5e16a6b44948ebda5efc750f62b60d0de8dd20131f449427426a36caf374af92

src/net.h

@@ -1002,7 +1002,11 @@ private:
 };
 
 /**
- * Interface for message handling
+ * Interface for message handling.
+ * For a given `CNode`, the methods must be called in the following order:
+ * 1. `InitializeNode()`
+ * 2. Any number of calls to `ProcessMessages()` or `SendMessages()`
+ * 3. `FinalizeNode()`
  */
 class NetEventsInterface
 {
@@ -1010,10 +1014,19 @@ public:
     /** Mutex for anything that is only accessed via the msg processing thread */
     static Mutex g_msgproc_mutex;
 
-    /** Initialize a peer (setup state) */
+    /**
+     * Initialize a peer (setup state). The caller is responsible for eventually
+     * calling `FinalizeNode()` on this node to avoid memory leaks.
+     * @param[in] node Peer to initialize.
+     * @param[in] our_services The services that we have advertised to the peer.
+     */
     virtual void InitializeNode(const CNode& node, ServiceFlags our_services) = 0;
 
-    /** Handle removal of a peer (clear state) */
+    /**
+     * Handle removal of a peer (clear state).
+     * When this is called all socket operations with the node must have completed.
+     * @param[in] node Peer whose state to clear.
+     */
     virtual void FinalizeNode(const CNode& node) = 0;
 
     /**
@@ -1023,23 +1036,26 @@ public:
     virtual bool HasAllDesirableServiceFlags(ServiceFlags services) const = 0;
 
     /**
-    * Process protocol messages received from a given node
-    *
-    * @param[in]   pnode           The node which we have received messages from.
-    * @param[in]   interrupt       Interrupt condition for processing threads
-    * @return                      True if there is more work to be done
-    */
+     * Process protocol messages received from a given node.
+     * The caller must make sure that `pnode` is not destroyed and `FinalizeNode()`
+     * is not called on it during the execution of this function.
+     *
+     * @param[in]   pnode           The node which we have received messages from.
+     * @param[in]   interrupt       Interrupt condition for processing threads
+     * @return                      True if there is more work to be done
+     */
     virtual bool ProcessMessages(CNode* pnode, std::atomic<bool>& interrupt) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
 
     /**
-    * Send queued protocol messages to a given node.
-    *
-    * @param[in]   pnode           The node which we are sending messages to.
-    * @return                      True if there is more work to be done
-    */
+     * Send queued protocol messages to a given node.
+     * The caller must make sure that `pnode` is not destroyed and `FinalizeNode()`
+     * is not called on it during the execution of this function.
+     *
+     * @param[in]   pnode           The node which we are sending messages to.
+     * @return                      True if there is more work to be done
+     */
     virtual bool SendMessages(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
 
-
 protected:
     /**
      * Protected destructor so that instances can only be deleted by derived classes.
@@ -1442,9 +1458,34 @@ private:
     std::vector<AddedNodeParams> m_added_node_params GUARDED_BY(m_added_nodes_mutex);
 
     mutable Mutex m_added_nodes_mutex;
+
+    /**
+     * List of peers we are connected with.
+     * Before destroying objects from this list it must be ensured that
+     * all socket operations have completed. `m_msgproc` must be informed
+     * that the node is about to be destroyed by calling `m_msgproc->FinalizeNode()`.
+     * `m_msgproc->ProcessMessages()` and `m_msgproc->SendMessages()` must not be
+     * running on that node during the destruction.
+     *
+     * CNode objects keep a reference count that is used to avoid a destruction
+     * while the object is used. All users of elements from `m_nodes` must either
+     * do so while holding `m_nodes_mutex` or by incrementing the reference before
+     * use and decrement it after use. To make sure that a new reference is not added
+     * to a no-referenced CNode just before it is destroyed we use the following
+     * mechanism (without a help from the standard library):
+     * - New references are only allowed to be added to objects in `m_nodes`.
+     * - When an object from `m_nodes` is to be destroyed it is moved from
+     *   `m_nodes` to `m_nodes_disconnected`. Adding new references to objects
+     *   in `m_nodes_disconnected` is not allowed.
+     * - Once an object in `m_nodes_disconnected` is not referenced anymore,
+     *   then it is safe to destroy it.
+     *
+     * So CNode destruction is: wait for no-references, `FinalizeNode()`, delete CNode.
+     */
     std::vector<CNode*> m_nodes GUARDED_BY(m_nodes_mutex);
-    std::list<CNode*> m_nodes_disconnected;
     mutable RecursiveMutex m_nodes_mutex;
+
+    std::list<CNode*> m_nodes_disconnected;
     std::atomic<NodeId> nLastNodeId{0};
     unsigned int nPrevNodeCount{0};
 

test/functional/wallet_backwards_compatibility.py

@@ -87,7 +87,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
         # 0.21.x and 22.x would both produce bad derivation paths when topping up an inactive hd chain
         # Make sure that this is being automatically cleaned up by migration
         node_master = self.nodes[1]
-        node_v22 = self.nodes[self.num_nodes - 5]
+        node_v22 = self.nodes[self.num_nodes - 3]
         wallet_name = "bad_deriv_path"
         node_v22.createwallet(wallet_name=wallet_name, descriptors=False)
         bad_deriv_wallet = node_v22.get_wallet_rpc(wallet_name)