Merge #6326: backport: bitcoin#20191, #20791, #21345, #21563, #22153, #22376, #22461, bitcoin-core/gui#365, bitcoin-core/gui#375, partial bitcoin#20755

24c01934a2 fix: small fixup for bitcoin#14918 and bitcoin#21679 (Konstantin Akimov)
f358f2bcdd Merge bitcoin-core/gui#375: Emit dataChanged signal to dynamically re-sort Peers table (Hennadii Stepanov)
7aeb0adeb9 Merge bitcoin-core/gui#365: Draw "eye" sign at the beginning of watch-only addresses (Hennadii Stepanov)
c52a582a3f refactor: re-order conditions over flags for m_edge_trig_events - follow-up for bitcoin#21563 (Konstantin Akimov)
6ed62b323c Merge bitcoin/bitcoin#21563: net: Restrict period when cs_vNodes mutex is locked (MarcoFalke)
16052f10ae Merge #20791: p2p: remove unused legacyWhitelisted in AcceptConnection() (MarcoFalke)
42d4f9a9b9 partial Merge #20755: [rpc] Remove deprecated fields from getpeerinfo (MarcoFalke)
cba01aa8f9 Merge bitcoin/bitcoin#20191: wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag (fanquake)
397fe9c0a5 Merge bitcoin/bitcoin#22461: wallet: Change ScriptPubKeyMan::Upgrade default to True (Samuel Dobson)
97f0d91d3e Merge bitcoin/bitcoin#22376: ci: Do not clone `bitcoin-core/qa-assets` git repository if not necessary (MarcoFalke)
0698be3680 Merge bitcoin/bitcoin#22153: test: Fix p2p_leak.py intermittent failure (MarcoFalke)
58b95338eb Merge #21345: test: bring p2p_leak.py up to date (MarcoFalke)

Pull request description:

  ## Issue being fixed or feature implemented
  Regular backports from bitcoin v22

  ## What was done?

  See commits for list of backports.
  bitcoin#20755 is partial because we need `banscore` for functional test `p2p_quorum_data.py`.

  Also several minor fixes for:
   - default args (bitcoin#14918, bitcoin#21679)
   - fixes for CNode::CloseSocketDisconnect (related to bitcoin/bitcoin#21563)
   - minor refactoring of m_edge_trig_events (related to bitcoin/bitcoin#21563)
   - missing executable flags for functional tests

  ## How Has This Been Tested?
  Run unit and functional tests.
  See also a screenshot:
  ![image](https://github.com/user-attachments/assets/a994bb6a-f31e-4083-9d15-56a20c470da8)

  ## Breaking Changes
  ```
  Updated RPCs
  - `getpeerinfo` no longer returns the following fields: `addnode`,
    and `whitelisted`, which were previously deprecated in v21. Instead of
    `addnode`, the `connection_type` field returns manual. Instead of
    `whitelisted`, the `permissions` field indicates if the peer has special
    privileges.
  ```

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  PastaPastaPasta:
    utACK 24c01934a2
  UdjinM6:
    utACK 24c01934a2

Tree-SHA512: d457e7a63bef3edb7bbb82e54deb72e57b021ea74d40f05c5a2fca2253e97919531a3e35936851ac9ca88e9ee94f2f299dae979c53904596b8a489ebf9cd9aa6

ci/test/04_install.sh

@@ -79,12 +79,13 @@ fi
 DOCKER_EXEC echo "Free disk space:"
 DOCKER_EXEC df -h
 
+if [ "$RUN_FUZZ_TESTS" = "true" ] || [ "$RUN_UNIT_TESTS" = "true" ] || [ "$RUN_UNIT_TESTS_SEQUENTIAL" = "true" ]; then
   if [ ! -d ${DIR_QA_ASSETS} ]; then
- if [ "$RUN_FUZZ_TESTS" = "true" ]; then
+    DOCKER_EXEC git clone --depth=1 https://github.com/bitcoin-core/qa-assets ${DIR_QA_ASSETS}
-  DOCKER_EXEC git clone https://github.com/bitcoin-core/qa-assets ${DIR_QA_ASSETS}
- fi
   fi
+
   export DIR_FUZZ_IN=${DIR_QA_ASSETS}/fuzz_seed_corpus/
+fi
 
 DOCKER_EXEC mkdir -p "${BASE_SCRATCH_DIR}/sanitizer-output/"
 

doc/release-notes-20755.md

@@ -0,0 +1,7 @@
+Updated RPCs
+------------
+- `getpeerinfo` no longer returns the following fields: `addnode`,
+  and `whitelisted`, which were previously deprecated in v21. Instead of
+  `addnode`, the `connection_type` field returns manual. Instead of
+  `whitelisted`, the `permissions` field indicates if the peer has special
+  privileges. (#20755)

src/init.cpp

@@ -254,20 +254,7 @@ void PrepareShutdown(NodeContext& node)
     // Because these depend on each-other, we make sure that neither can be
     // using the other before destroying them.
     if (node.peerman) UnregisterValidationInterface(node.peerman.get());
-    // Follow the lock order requirements:
+    if (node.connman) node.connman->Stop();
-    // * CheckForStaleTipAndEvictPeers locks cs_main before indirectly calling GetExtraFullOutboundCount
-    //   which locks cs_vNodes.
-    // * ProcessMessage locks cs_main and g_cs_orphans before indirectly calling ForEachNode which
-    //   locks cs_vNodes.
-    // * CConnman::Stop calls DeleteNode, which calls FinalizeNode, which locks cs_main and calls
-    //   EraseOrphansFor, which locks g_cs_orphans.
-    //
-    // Thus the implicit locking order requirement is: (1) cs_main, (2) g_cs_orphans, (3) cs_vNodes.
-    if (node.connman) {
-        node.connman->StopThreads();
-        LOCK2(::cs_main, ::g_cs_orphans);
-        node.connman->StopNodes();
-    }
 
     StopTorControl();
 

src/net.cpp

@@ -663,8 +663,8 @@ void CNode::CloseSocketDisconnect(CConnman* connman)
         connman->mapSendableNodes.erase(GetId());
     }
 
-    if (connman->m_edge_trig_events && !connman->m_edge_trig_events->UnregisterEvents(m_sock->Get())) {
+    if (connman->m_edge_trig_events) {
-        LogPrint(BCLog::NET, "EdgeTriggeredEvents::UnregisterEvents() failed\n");
+        connman->m_edge_trig_events->UnregisterEvents(m_sock->Get());
     }
 
     LogPrint(BCLog::NET, "disconnecting peer=%d\n", id);
@@ -736,7 +736,6 @@ void CNode::CopyStats(CNodeStats& stats)
         X(cleanSubVer);
     }
     stats.fInbound = IsInboundConn();
-    stats.m_manual_connection = IsManualConn();
     X(m_bip152_highbandwidth_to);
     X(m_bip152_highbandwidth_from);
     {
@@ -752,7 +751,6 @@ void CNode::CopyStats(CNodeStats& stats)
         stats.m_transport_type = info.transport_type;
         if (info.session_id) stats.m_session_id = HexStr(*info.session_id);
     }
-    X(m_legacyWhitelisted);
     X(m_permission_flags);
 
     X(m_last_ping_time);
@@ -1895,14 +1893,12 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
     int nMaxInbound = nMaxConnections - m_max_outbound;
 
     AddWhitelistPermissionFlags(permission_flags, addr);
-    bool legacyWhitelisted = false;
     if (NetPermissions::HasFlag(permission_flags, NetPermissionFlags::Implicit)) {
         NetPermissions::ClearFlag(permission_flags, NetPermissionFlags::Implicit);
         if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(permission_flags, NetPermissionFlags::ForceRelay);
         if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(permission_flags, NetPermissionFlags::Relay);
         NetPermissions::AddFlag(permission_flags, NetPermissionFlags::Mempool);
         NetPermissions::AddFlag(permission_flags, NetPermissionFlags::NoBan);
-        legacyWhitelisted = true;
     }
 
     {
@@ -2010,8 +2006,6 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
                                  .use_v2transport = use_v2transport,
                              });
     pnode->AddRef();
-    // If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility)
-    pnode->m_legacyWhitelisted = legacyWhitelisted;
     m_msgproc->InitializeNode(*pnode, nodeServices);
 
     {
@@ -4307,27 +4301,23 @@ void CConnman::StopNodes()
         }
     }
 
-    {
+    // Delete peer connections.
-        LOCK(m_nodes_mutex);
-
-        // Close sockets
-        for (CNode *pnode : m_nodes)
-            pnode->CloseSocketDisconnect(this);
-    }
-    for (ListenSocket& hListenSocket : vhListenSocket) {
-        if (hListenSocket.sock) {
-            if (m_edge_trig_events && !m_edge_trig_events->RemoveSocket(hListenSocket.sock->Get())) {
-                LogPrintf("EdgeTriggeredEvents::RemoveSocket() failed\n");
-            }
-        }
-    }
-
-    // clean up some globals (to help leak detection)
     std::vector<CNode*> nodes;
     WITH_LOCK(m_nodes_mutex, nodes.swap(m_nodes));
     for (CNode *pnode : nodes) {
+        pnode->CloseSocketDisconnect(this);
         DeleteNode(pnode);
     }
+
+    // Close listening sockets.
+    if (m_edge_trig_events) {
+        for (ListenSocket& hListenSocket : vhListenSocket) {
+            if (hListenSocket.sock) {
+                m_edge_trig_events->RemoveSocket(hListenSocket.sock->Get());
+            }
+        }
+    }
+
     for (CNode* pnode : m_nodes_disconnected) {
         DeleteNode(pnode);
     }

src/net.h

@@ -241,7 +241,6 @@ public:
     int nVersion;
     std::string cleanSubVer;
     bool fInbound;
-    bool m_manual_connection;
     bool m_bip152_highbandwidth_to;
     bool m_bip152_highbandwidth_from;
     int m_starting_height;
@@ -250,7 +249,6 @@ public:
     uint64_t nRecvBytes;
     mapMsgTypeSize mapRecvBytesPerMsgType;
     NetPermissionFlags m_permission_flags;
-    bool m_legacyWhitelisted;
     std::chrono::microseconds m_last_ping_time;
     std::chrono::microseconds m_min_ping_time;
     // Our address, as reported by the peer
@@ -789,8 +787,6 @@ public:
     bool HasPermission(NetPermissionFlags permission) const {
         return NetPermissions::HasFlag(m_permission_flags, permission);
     }
-    // This boolean is unusued in actual processing, only present for backward compatibility at RPC/QT level
-    bool m_legacyWhitelisted{false};
     /** fSuccessfullyConnected is set to true on receiving VERACK from the peer. */
     std::atomic_bool fSuccessfullyConnected{false};
     // Setting fDisconnect to true will cause the node to be disconnected the

src/qt/overviewpage.cpp

@@ -95,17 +95,21 @@ public:
         // Address/Label
         colorForeground = qvariant_cast<QColor>(indexAddress.data(Qt::ForegroundRole));
         QString address = indexAddress.data(Qt::DisplayRole).toString();
-        painter->setPen(colorForeground);
+
-        painter->drawText(rectBottomHalf, Qt::AlignLeft | Qt::AlignVCenter, address, &rectBounding);
-        int address_rect_min_width = rectBounding.width();
         // Optional Watchonly indicator
+        QRect addressRect{rectBottomHalf};
         if (index.data(TransactionTableModel::WatchonlyRole).toBool())
         {
             QIcon iconWatchonly = qvariant_cast<QIcon>(index.data(TransactionTableModel::WatchonlyDecorationRole));
-            QRect rectWatchonly(rectBounding.right() + 5, rectBottomHalf.top(), 16, halfheight);
+            QRect watchonlyRect(rectBottomHalf.left(), rectBottomHalf.top(), rectBottomHalf.height(), halfheight);
-            iconWatchonly.paint(painter, rectWatchonly);
+            iconWatchonly.paint(painter, watchonlyRect);
+            addressRect.setLeft(addressRect.left() + watchonlyRect.width() + 5);
         }
 
+        painter->setPen(colorForeground);
+        painter->drawText(addressRect, Qt::AlignLeft | Qt::AlignVCenter, address, &rectBounding);
+        int address_rect_min_width = rectBounding.width();
+
         const int minimum_width = std::max(address_rect_min_width, amount_bounding_rect.width() /*+ date_bounding_rect.width() */);
         const auto search = m_minimum_width.find(index.row());
         if (search == m_minimum_width.end() || search->second != minimum_width) {

src/qt/peertablemodel.cpp

@@ -182,5 +182,7 @@ void PeerTableModel::refresh()
         m_peers_data.swap(new_peers_data);
     }
 
-    Q_EMIT changed();
+    const auto top_left = index(0, 0);
+    const auto bottom_right = index(rowCount() - 1, columnCount() - 1);
+    Q_EMIT dataChanged(top_left, bottom_right);
 }

src/qt/peertablemodel.h

@@ -74,9 +74,6 @@ public:
 public Q_SLOTS:
     void refresh();
 
-Q_SIGNALS:
-    void changed();
-
 private:
     //! Internal peer data structure.
     QList<CNodeCombinedStats> m_peers_data{};

src/qt/rpcconsole.cpp

@@ -39,6 +39,7 @@
 #endif
 
 #include <QAbstractButton>
+#include <QAbstractItemModel>
 #include <QButtonGroup>
 #include <QDir>
 #include <QFont>
@@ -743,7 +744,7 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
 
         // peer table signal handling - update peer details when selecting new node
         connect(ui->peerWidget->selectionModel(), &QItemSelectionModel::selectionChanged, this, &RPCConsole::updateDetailWidget);
-        connect(model->getPeerTableModel(), &PeerTableModel::changed, this, &RPCConsole::updateDetailWidget);
+        connect(model->getPeerTableModel(), &QAbstractItemModel::dataChanged, [this] { updateDetailWidget(); });
 
         // set up ban table
         ui->banlistWidget->setModel(model->getBanTableModel());

src/rpc/mining.cpp

@@ -571,7 +571,7 @@ static RPCHelpMan getblocktemplate()
                             {"str", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "client side supported feature, 'longpoll', 'coinbasevalue', 'proposal', 'serverlist', 'workid'"},
                         },
                         },
-                    {"rules", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "A list of strings",
+                    {"rules", RPCArg::Type::ARR, RPCArg::Optional::NO, "A list of strings",
                         {
                             {"str", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "client side supported softfork deployment"},
                         },

src/rpc/net.cpp

@@ -137,8 +137,6 @@ static RPCHelpMan getpeerinfo()
                     {RPCResult::Type::BOOL, "inbound", "Inbound (true) or Outbound (false)"},
                     {RPCResult::Type::BOOL, "bip152_hb_to", "Whether we selected peer as (compact blocks) high-bandwidth peer"},
                     {RPCResult::Type::BOOL, "bip152_hb_from", "Whether peer selected us as (compact blocks) high-bandwidth peer"},
-                    {RPCResult::Type::BOOL, "addnode", "Whether connection was due to addnode/-connect or if it was an automatic/inbound connection\n"
-                                                       "(DEPRECATED, returned only if the config option -deprecatedrpc=getpeerinfo_addnode is passed)"},
                     {RPCResult::Type::BOOL, "masternode", "Whether connection was due to masternode connection attempt"},
                     {RPCResult::Type::NUM, "banscore", "The ban score (DEPRECATED, returned only if config option -deprecatedrpc=banscore is passed)"},
                     {RPCResult::Type::NUM, "startingheight", "The starting height (block) of the peer"},
@@ -151,8 +149,6 @@ static RPCHelpMan getpeerinfo()
                     {RPCResult::Type::BOOL, "addr_relay_enabled", "Whether we participate in address relay with this peer"},
                     {RPCResult::Type::NUM, "addr_processed", "The total number of addresses processed, excluding those dropped due to rate limiting"},
                     {RPCResult::Type::NUM, "addr_rate_limited", "The total number of addresses dropped due to rate limiting"},
-                    {RPCResult::Type::BOOL, "whitelisted", /* optional */ true, "Whether the peer is whitelisted with default permissions\n"
-                                                                                "(DEPRECATED, returned only if config option -deprecatedrpc=whitelisted is passed)"},
                     {RPCResult::Type::ARR, "permissions", "Any special permissions that have been granted to this peer",
                     {
                         {RPCResult::Type::STR, "permission_type", Join(NET_PERMISSIONS_DOC, ",\n") + ".\n"},
@@ -241,10 +237,6 @@ static RPCHelpMan getpeerinfo()
         obj.pushKV("inbound", stats.fInbound);
         obj.pushKV("bip152_hb_to", stats.m_bip152_highbandwidth_to);
         obj.pushKV("bip152_hb_from", stats.m_bip152_highbandwidth_from);
-        if (IsDeprecatedRPCEnabled("getpeerinfo_addnode")) {
-            // addnode is deprecated in v21 for removal in v22
-            obj.pushKV("addnode", stats.m_manual_connection);
-        }
         obj.pushKV("masternode", stats.m_masternode_connection);
         if (fStateStats) {
             if (IsDeprecatedRPCEnabled("banscore")) {
@@ -264,10 +256,6 @@ static RPCHelpMan getpeerinfo()
             obj.pushKV("addr_processed", statestats.m_addr_processed);
             obj.pushKV("addr_rate_limited", statestats.m_addr_rate_limited);
         }
-        if (IsDeprecatedRPCEnabled("whitelisted")) {
-            // whitelisted is deprecated in v0.21 for removal in v0.22
-            obj.pushKV("whitelisted", stats.m_legacyWhitelisted);
-        }
         UniValue permissions(UniValue::VARR);
         for (const auto& permission : NetPermissions::ToStrings(stats.m_permission_flags)) {
             permissions.push_back(permission);

src/test/fuzz/process_message.cpp

@@ -103,7 +103,6 @@ void fuzz_target(FuzzBufferType buffer, const std::string& LIMIT_TO_MESSAGE_TYPE
     }
     g_setup->m_node.peerman->SendMessages(&p2p_node);
     SyncWithValidationInterfaceQueue();
-    LOCK2(::cs_main, g_cs_orphans); // See init.cpp for rationale for implicit locking order requirement
     g_setup->m_node.connman->StopNodes();
 }
 

src/test/fuzz/process_messages.cpp

@@ -76,6 +76,5 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages)
         g_setup->m_node.peerman->SendMessages(&random_node);
     }
     SyncWithValidationInterfaceQueue();
-    LOCK2(::cs_main, g_cs_orphans); // See init.cpp for rationale for implicit locking order requirement
     g_setup->m_node.connman->StopNodes();
 }

src/util/edge.h

@@ -17,6 +17,7 @@ enum class SocketEventsMode : int8_t;
  * A manager for abstracting logic surrounding edge-triggered socket events
  * modes like kqueue and epoll.
  */
+// TODO: simplify this class to 2-3 flags; kick out everything else to Sock/~Sock and inherited classes
 class EdgeTriggeredEvents
 {
 public:

src/wallet/scriptpubkeyman.cpp

@@ -1779,12 +1779,10 @@ bool LegacyScriptPubKeyMan::GetHDChain(CHDChain& hdChainRet) const
     return !m_hd_chain.IsNull();
 }
 
-void LegacyScriptPubKeyMan::SetInternal(bool internal) {}
-
 bool DescriptorScriptPubKeyMan::GetNewDestination(CTxDestination& dest, bilingual_str& error)
 {
     // Returns true if this descriptor supports getting new addresses. Conditions where we may be unable to fetch them (e.g. locked) are caught later
-    if (!CanGetAddresses(m_internal)) {
+    if (!CanGetAddresses()) {
         error = _("No addresses available");
         return false;
     }
@@ -2042,7 +2040,7 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const
     }
 }
 
-bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key)
+bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key, bool internal)
 {
     LOCK(cs_desc_man);
     assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
@@ -2060,7 +2058,7 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_
     std::string desc_prefix = strprintf("pkh(%s/44'/%d'", xpub, Params().ExtCoinType());
     std::string desc_suffix = "/*)";
 
-    std::string internal_path = m_internal ? "/1" : "/0";
+    std::string internal_path = internal ? "/1" : "/0";
     std::string desc_str = desc_prefix + "/0'" + internal_path + desc_suffix;
 
     // Make the descriptor
@@ -2115,13 +2113,6 @@ int64_t DescriptorScriptPubKeyMan::GetOldestKeyPoolTime() const
     return 0;
 }
 
-size_t DescriptorScriptPubKeyMan::KeypoolCountExternalKeys() const
-{
-    if (m_internal) {
-        return 0;
-    }
-    return GetKeyPoolSize();
-}
 
 unsigned int DescriptorScriptPubKeyMan::GetKeyPoolSize() const
 {
@@ -2332,11 +2323,6 @@ uint256 DescriptorScriptPubKeyMan::GetID() const
     return m_wallet_descriptor.id;
 }
 
-void DescriptorScriptPubKeyMan::SetInternal(bool internal)
-{
-    this->m_internal = internal;
-}
-
 void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache)
 {
     LOCK(cs_desc_man);

src/wallet/scriptpubkeyman.h

@@ -195,7 +195,6 @@ public:
 
     virtual int64_t GetOldestKeyPoolTime() const { return GetTime(); }
 
-    virtual size_t KeypoolCountExternalKeys() const { return 0; }
     virtual unsigned int GetKeyPoolSize() const { return 0; }
 
     virtual int64_t GetTimeFirstKey() const { return 0; }
@@ -219,8 +218,6 @@ public:
 
     virtual uint256 GetID() const { return uint256(); }
 
-    virtual void SetInternal(bool internal) {}
-
     /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
     template<typename... Params>
     void WalletLogPrintf(std::string fmt, Params... parameters) const {
@@ -345,8 +342,7 @@ public:
     void RewriteDB() override;
 
     int64_t GetOldestKeyPoolTime() const override;
-    size_t KeypoolCountExternalKeys() const override;
+    size_t KeypoolCountExternalKeys() const;
-
     unsigned int GetKeyPoolSize() const override;
 
     int64_t GetTimeFirstKey() const override;
@@ -366,8 +362,6 @@ public:
 
     uint256 GetID() const override;
 
-    void SetInternal(bool internal) override;
-
     // Map from Key ID to key metadata.
     std::map<CKeyID, CKeyMetadata> mapKeyMetadata GUARDED_BY(cs_KeyStore);
 
@@ -520,8 +514,6 @@ private:
     PubKeyMap m_map_pubkeys GUARDED_BY(cs_desc_man);
     int32_t m_max_cached_index = -1;
 
-    bool m_internal = false;
-
     KeyMap m_map_keys GUARDED_BY(cs_desc_man);
     CryptedKeyMap m_map_crypted_keys GUARDED_BY(cs_desc_man);
 
@@ -544,9 +536,8 @@ public:
         :   ScriptPubKeyMan(storage),
             m_wallet_descriptor(descriptor)
         {}
-    DescriptorScriptPubKeyMan(WalletStorage& storage, bool internal)
+    DescriptorScriptPubKeyMan(WalletStorage& storage)
-        :   ScriptPubKeyMan(storage),
+        :   ScriptPubKeyMan(storage)
-            m_internal(internal)
         {}
 
     mutable RecursiveMutex cs_desc_man;
@@ -571,12 +562,11 @@ public:
     bool IsHDEnabled() const override;
 
     //! Setup descriptors based on the given CExtkey
-    bool SetupDescriptorGeneration(const CExtKey& master_key);
+    bool SetupDescriptorGeneration(const CExtKey& master_key, bool internal);
 
     bool HavePrivateKeys() const override;
 
     int64_t GetOldestKeyPoolTime() const override;
-    size_t KeypoolCountExternalKeys() const override;
     unsigned int GetKeyPoolSize() const override;
 
     int64_t GetTimeFirstKey() const override;
@@ -596,8 +586,6 @@ public:
 
     uint256 GetID() const override;
 
-    void SetInternal(bool internal) override;
-
     void SetCache(const DescriptorCache& cache);
 
     bool AddKey(const CKeyID& key_id, const CKey& key);

src/wallet/wallet.cpp

@@ -4145,9 +4145,14 @@ size_t CWallet::KeypoolCountExternalKeys() const
 {
     AssertLockHeld(cs_wallet);
 
+    auto legacy_spk_man = GetLegacyScriptPubKeyMan();
+    if (legacy_spk_man) {
+        return legacy_spk_man->KeypoolCountExternalKeys();
+    }
+
     unsigned int count = 0;
-    for (auto spk_man : GetActiveScriptPubKeyMans()) {
+    if (m_external_spk_managers) {
-        count += spk_man->KeypoolCountExternalKeys();
+        count += m_external_spk_managers->GetKeyPoolSize();
     }
 
     return count;
@@ -5847,7 +5852,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
 
     for (bool internal : {false, true}) {
         { // OUTPUT_TYPE is only one: LEGACY
-            auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, internal));
+            auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this));
             if (IsCrypted()) {
                 if (IsLocked()) {
                     throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors");
@@ -5856,7 +5861,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
                     throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors");
                 }
             }
-            spk_manager->SetupDescriptorGeneration(master_key);
+            spk_manager->SetupDescriptorGeneration(master_key, internal);
             uint256 id = spk_manager->GetID();
             m_spk_managers[id] = std::move(spk_manager);
             AddActiveScriptPubKeyMan(id, internal);
@@ -5883,7 +5888,6 @@ void CWallet::LoadActiveScriptPubKeyMan(uint256 id, bool internal)
     auto& spk_mans = internal ? m_internal_spk_managers : m_external_spk_managers;
     auto& spk_mans_other = internal ? m_external_spk_managers : m_internal_spk_managers;
     auto spk_man = m_spk_managers.at(id).get();
-    spk_man->SetInternal(internal);
     spk_mans = spk_man;
 
     if (spk_mans_other == spk_man) {

test/functional/p2p_blocksonly.py

@@ -51,7 +51,7 @@ class P2PBlocksOnly(BitcoinTestFramework):
             assert_equal(self.nodes[0].getmempoolinfo()['size'], 1)
 
         self.log.info("Restarting node 0 with relay permission and blocksonly")
-        self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly", '-deprecatedrpc=whitelisted'])
+        self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly"])
         assert_equal(self.nodes[0].getrawmempool(), [])
         first_peer = self.nodes[0].add_p2p_connection(P2PInterface())
         second_peer = self.nodes[0].add_p2p_connection(P2PInterface())

test/functional/p2p_leak.py

@@ -4,8 +4,8 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test message sending before handshake completion.
 
-A node should never send anything other than VERSION/VERACK until it's
+Before receiving a VERACK, a node should not send anything but VERSION/VERACK
-received a VERACK.
+and feature negotiation messages ( SENDADDRV2).
 
 This test connects to a node and sends it a few messages, trying to entice it
 into sending us something it shouldn't.
@@ -30,16 +30,19 @@ from test_framework.util import (
     assert_greater_than_or_equal,
 )
 
+PEER_TIMEOUT = 3
+
 
 class LazyPeer(P2PInterface):
     def __init__(self):
         super().__init__()
         self.unexpected_msg = False
         self.ever_connected = False
+        self.got_sendaddrv2 = False
 
     def bad_message(self, message):
         self.unexpected_msg = True
-        self.log.info("should not have received message: %s" % message.msgtype)
+        print("should not have received message: %s" % message.msgtype)
 
     def on_open(self):
         self.ever_connected = True
@@ -64,6 +67,7 @@ class LazyPeer(P2PInterface):
     def on_cmpctblock(self, message): self.bad_message(message)
     def on_getblocktxn(self, message): self.bad_message(message)
     def on_blocktxn(self, message): self.bad_message(message)
+    def on_sendaddrv2(self, message): self.got_sendaddrv2 = True
 
 
 # Peer that sends a version but not a verack.
@@ -95,16 +99,28 @@ class P2PLeakTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
         self.disable_mocktime = True
+        self.extra_args = [[f"-peertimeout={PEER_TIMEOUT}"]]
+
+
+    def create_old_version(self, nversion):
+        old_version_msg = msg_version()
+        old_version_msg.nVersion = nversion
+        old_version_msg.strSubVer = P2P_SUBVERSION
+        old_version_msg.nServices = P2P_SERVICES
+        old_version_msg.relay = P2P_VERSION_RELAY
+        return old_version_msg
+
 
     def run_test(self):
-        # Another peer that never sends a version, nor any other messages. It shouldn't receive anything from the node.
+        self.log.info('Check that the node doesn\'t send unexpected messages before handshake completion')
+        # Peer that never sends a version, nor any other messages. It shouldn't receive anything from the node.
         no_version_idle_peer = self.nodes[0].add_p2p_connection(LazyPeer(), send_version=False, wait_for_verack=False)
 
         # Peer that sends a version but not a verack.
         no_verack_idle_peer = self.nodes[0].add_p2p_connection(NoVerackIdlePeer(), wait_for_verack=False)
 
-        # Wait until we got the verack in response to the version. Though, don't wait for the node to receive the
+        # Wait until the peer gets the verack in response to the version. Though, don't wait for the node to receive the
-        # verack, since we never sent one
+        # verack, since the peer never sent one
         no_verack_idle_peer.wait_for_verack()
 
         no_version_idle_peer.wait_until(lambda: no_version_idle_peer.ever_connected)
@@ -114,13 +130,18 @@ class P2PLeakTest(BitcoinTestFramework):
         self.generate(self.nodes[0], nblocks=1)
 
         # Give the node enough time to possibly leak out a message
-        time.sleep(5)
+        time.sleep(PEER_TIMEOUT + 2)
 
-        self.nodes[0].disconnect_p2ps()
+        # Make sure only expected messages came in
+        assert not no_version_idle_peer.unexpected_msg
+        assert not no_version_idle_peer.got_sendaddrv2
 
-        # Make sure no unexpected messages came in
+        assert not no_verack_idle_peer.unexpected_msg
-        assert no_version_idle_peer.unexpected_msg == False
+        assert no_verack_idle_peer.got_sendaddrv2
-        assert no_verack_idle_peer.unexpected_msg == False
+
+        # Expect peers to be disconnected due to timeout
+        assert not no_version_idle_peer.is_connected
+        assert not no_verack_idle_peer.is_connected
 
         self.log.info('Check that the version message does not leak the local address of the node')
         p2p_version_store = self.nodes[0].add_p2p_connection(P2PVersionStore())
@@ -135,13 +156,8 @@ class P2PLeakTest(BitcoinTestFramework):
 
         self.log.info('Check that old peers are disconnected')
         p2p_old_peer = self.nodes[0].add_p2p_connection(P2PInterface(), send_version=False, wait_for_verack=False)
-        old_version_msg = msg_version()
-        old_version_msg.nVersion = 31799
-        old_version_msg.strSubVer = P2P_SUBVERSION
-        old_version_msg.nServices = P2P_SERVICES
-        old_version_msg.relay = P2P_VERSION_RELAY
         with self.nodes[0].assert_debug_log(['peer=3 using obsolete version 31799; disconnecting']):
-            p2p_old_peer.send_message(old_version_msg)
+            p2p_old_peer.send_message(self.create_old_version(31799))
             p2p_old_peer.wait_for_disconnect()
 
 

test/functional/p2p_permissions.py

@@ -36,35 +36,24 @@ class P2PPermissionsTests(BitcoinTestFramework):
             # default permissions (no specific permissions)
             ["-whitelist=127.0.0.1"],
             # Make sure the default values in the command line documentation match the ones here
-            ["relay", "noban", "mempool", "download"],
+            ["relay", "noban", "mempool", "download"])
-            True)
-
-        self.checkpermission(
-            # check without deprecatedrpc=whitelisted
-            ["-whitelist=127.0.0.1"],
-            # Make sure the default values in the command line documentation match the ones here
-            ["relay", "noban", "mempool", "download"],
-            None)
 
         self.checkpermission(
             # no permission (even with forcerelay)
             ["-whitelist=@127.0.0.1", "-whitelistforcerelay=1"],
-            [],
+            [])
-            False)
 
         self.checkpermission(
             # relay permission removed (no specific permissions)
             ["-whitelist=127.0.0.1", "-whitelistrelay=0"],
-            ["noban", "mempool", "download"],
+            ["noban", "mempool", "download"])
-            True)
 
         self.checkpermission(
             # forcerelay and relay permission added
             # Legacy parameter interaction which set whitelistrelay to true
             # if whitelistforcerelay is true
             ["-whitelist=127.0.0.1", "-whitelistforcerelay"],
-            ["forcerelay", "relay", "noban", "mempool", "download"],
+            ["forcerelay", "relay", "noban", "mempool", "download"])
-            True)
 
         # Let's make sure permissions are merged correctly
         # For this, we need to use whitebind instead of bind
@@ -74,39 +63,28 @@ class P2PPermissionsTests(BitcoinTestFramework):
         self.checkpermission(
             ["-whitelist=noban@127.0.0.1"],
             # Check parameter interaction forcerelay should activate relay
-            ["noban", "bloomfilter", "forcerelay", "relay", "download"],
+            ["noban", "bloomfilter", "forcerelay", "relay", "download"])
-            False)
         self.replaceinconfig(1, "whitebind=bloomfilter,forcerelay@" + ip_port, "bind=127.0.0.1")
 
         self.checkpermission(
             # legacy whitelistrelay should be ignored
             ["-whitelist=noban,mempool@127.0.0.1", "-whitelistrelay"],
-            ["noban", "mempool", "download"],
+            ["noban", "mempool", "download"])
-            False)
-
-        self.checkpermission(
-            # check without deprecatedrpc=whitelisted
-            ["-whitelist=noban,mempool@127.0.0.1", "-whitelistrelay"],
-            ["noban", "mempool", "download"],
-            None)
 
         self.checkpermission(
             # legacy whitelistforcerelay should be ignored
             ["-whitelist=noban,mempool@127.0.0.1", "-whitelistforcerelay"],
-            ["noban", "mempool", "download"],
+            ["noban", "mempool", "download"])
-            False)
 
         self.checkpermission(
             # missing mempool permission to be considered legacy whitelisted
             ["-whitelist=noban@127.0.0.1"],
-            ["noban", "download"],
+            ["noban", "download"])
-            False)
 
         self.checkpermission(
             # all permission added
             ["-whitelist=all@127.0.0.1"],
-            ["forcerelay", "noban", "mempool", "bloomfilter", "relay", "download", "addr"],
+            ["forcerelay", "noban", "mempool", "bloomfilter", "relay", "download", "addr"])
-            False)
 
         self.stop_node(1)
         self.nodes[1].assert_start_raises_init_error(["-whitelist=oopsie@127.0.0.1"], "Invalid P2P permission", match=ErrorMatch.PARTIAL_REGEX)
@@ -170,19 +148,13 @@ class P2PPermissionsTests(BitcoinTestFramework):
             reject_reason='Not relaying non-mempool transaction {} from forcerelay peer=0'.format(txid)
         )
 
-    def checkpermission(self, args, expectedPermissions, whitelisted):
+    def checkpermission(self, args, expectedPermissions):
-        if whitelisted is not None:
-            args = [*args, '-deprecatedrpc=whitelisted']
         self.restart_node(1, args)
         self.connect_nodes(0, 1)
         peerinfo = self.nodes[1].getpeerinfo()[0]
-        if whitelisted is None:
-            assert 'whitelisted' not in peerinfo
-        else:
-            assert_equal(peerinfo['whitelisted'], whitelisted)
         assert_equal(len(expectedPermissions), len(peerinfo['permissions']))
         for p in expectedPermissions:
-            if not p in peerinfo['permissions']:
+            if p not in peerinfo['permissions']:
                 raise AssertionError("Expected permissions %r is not granted." % p)
 
     def replaceinconfig(self, nodeid, old, new):

test/functional/rpc_getpeerinfo_deprecation.py

@@ -14,7 +14,6 @@ class GetpeerinfoDeprecationTest(BitcoinTestFramework):
 
     def run_test(self):
         self.test_banscore_deprecation()
-        self.test_addnode_deprecation()
 
     def test_banscore_deprecation(self):
         self.log.info("Test getpeerinfo by default no longer returns a banscore field")
@@ -23,16 +22,6 @@ class GetpeerinfoDeprecationTest(BitcoinTestFramework):
         self.log.info("Test getpeerinfo returns banscore with -deprecatedrpc=banscore")
         assert "banscore" in self.nodes[1].getpeerinfo()[0].keys()
 
-    def test_addnode_deprecation(self):
-        self.restart_node(1, ["-deprecatedrpc=getpeerinfo_addnode"])
-        self.connect_nodes(0, 1)
-
-        self.log.info("Test getpeerinfo by default no longer returns an addnode field")
-        assert "addnode" not in self.nodes[0].getpeerinfo()[0].keys()
-
-        self.log.info("Test getpeerinfo returns addnode with -deprecatedrpc=addnode")
-        assert "addnode" in self.nodes[1].getpeerinfo()[0].keys()
-
 
 if __name__ == "__main__":
     GetpeerinfoDeprecationTest().main()

test/functional/wallet_upgradewallet.py

@@ -72,10 +72,11 @@ class UpgradeWalletTest(BitcoinTestFramework):
     def test_upgradewallet(self, wallet, previous_version, requested_version=None, expected_version=None):
         unchanged = expected_version == previous_version
         new_version = previous_version if unchanged else expected_version if expected_version else requested_version
-        assert_equal(wallet.getwalletinfo()["walletversion"], previous_version)
+        old_wallet_info = wallet.getwalletinfo()
+        assert_equal(old_wallet_info["walletversion"], previous_version)
         assert_equal(wallet.upgradewallet(requested_version),
             {
-                "wallet_name": "",
+                "wallet_name": old_wallet_info["walletname"],
                 "previous_version": previous_version,
                 "current_version": new_version,
                 "result": "Already at latest version. Wallet version unchanged." if unchanged else "Wallet upgraded successfully from version {} to version {}.".format(previous_version, new_version),
@@ -232,6 +233,11 @@ class UpgradeWalletTest(BitcoinTestFramework):
 
         assert_equal(120200, wallet.getwalletinfo()['walletversion'])
 
+        if self.is_sqlite_compiled():
+            self.log.info("Checking that descriptor wallets do nothing, successfully")
+            self.nodes[0].createwallet(wallet_name="desc_upgrade", descriptors=True)
+            desc_wallet = self.nodes[0].get_wallet_rpc("desc_upgrade")
+            self.test_upgradewallet(desc_wallet, previous_version=120200, expected_version=120200)
 
 if __name__ == '__main__':
     UpgradeWalletTest().main()