Merge #5511: backport!: Merge bitcoin#19915, 19469, 19464, 19512, 22495

a266abe Merge bitcoin#22495: p2p: refactor: tidy up `PeerManagerImpl::Misbehaving(...)` (MarcoFalke) d7985aa Merge bitcoin#19512: p2p: banscore updates to gui, tests, release notes (Wladimir J. van der Laan) b9799de Merge bitcoin#19464: net: remove -banscore configuration option (MarcoFalke) c0c5d05 Merge bitcoin#19469: rpc: deprecate banscore field in getpeerinfo (fanquake) 02faef7 Merge bitcoin#19915: p2p, refactor: Use Mutex type for some mutexes in CNode class (MarcoFalke) Pull request description: bitcoin backports Top commit has no ACKs. Tree-SHA512: 7537fd2203e10f3de03e3e2df7f9c7b4fcf05387325635c352a4e9898338e979bfd46626ceee8d267ea48aded4f342bca56ff8020e71dab79f37f4b1bb7cf27c
dashpay · Mar 22, 2024 · 2397209 · 2397209
2 parents b7c5a92 + a266abe
commit 2397209
Show file tree

Hide file tree

Showing 15 changed files with 96 additions and 121 deletions.
diff --git a/doc/release-notes-19464.md b/doc/release-notes-19464.md
@@ -0,0 +1,7 @@
+Updated settings
+----------------
+
+- The `-banscore` configuration option, which modified the default threshold for
+  disconnecting and discouraging misbehaving peers, has been removed as part of
+  changes in this release to the handling of misbehaving peers. Refer to the
+  section, "Changes regarding misbehaving peers", for details. (dash#5511)
diff --git a/doc/release-notes-19469.md b/doc/release-notes-19469.md
@@ -0,0 +1,6 @@
+Updated RPCs
+------------
+
+- `getpeerinfo` no longer returns the `banscore` field unless the configuration
+  option `-deprecatedrpc=banscore` is used. The `banscore` field will be fully
+  removed in the next major release. (dash#5511)
diff --git a/src/init.cpp b/src/init.cpp
@@ -572,7 +572,6 @@ void SetupServerArgs(NodeContext& node)
     argsman.AddArg("-asmap=<file>", strprintf("Specify asn mapping used for bucketing of the peers (default: %s). Relative paths will be prefixed by the net-specific datadir location.", DEFAULT_ASMAP_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-addnode=<ip>", "Add a node to connect to and attempt to keep the connection open (see the `addnode` RPC command help for more info). This option can be specified multiple times to add multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     argsman.AddArg("-allowprivatenet", strprintf("Allow RFC1918 addresses to be relayed and connected to (default: %u)", DEFAULT_ALLOWPRIVATENET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
-    argsman.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting and discouraging misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-bantime=<n>", strprintf("Default duration (in seconds) of manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-bind=<addr>[:<port>][=onion]", strprintf("Bind to given address and always listen on it (default: 0.0.0.0). Use [host]:port notation for IPv6. Append =onion to tag any incoming connections to that address and port as incoming Tor connections (default: 127.0.0.1:%u=onion, testnet: 127.0.0.1:%u=onion, regtest: 127.0.0.1:%u=onion)", defaultBaseParams->OnionServiceTargetPort(), testnetBaseParams->OnionServiceTargetPort(), regtestBaseParams->OnionServiceTargetPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     argsman.AddArg("-connect=<ip>", "Connect only to the specified node; -noconnect disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);

diff --git a/src/net.h b/src/net.h
@@ -440,9 +440,9 @@ class CNode
     uint64_t nSendBytes GUARDED_BY(cs_vSend){0};
     std::list<std::vector<unsigned char>> vSendMsg GUARDED_BY(cs_vSend);
     std::atomic<size_t> nSendMsgSize{0};
-    RecursiveMutex cs_vSend;
-    RecursiveMutex cs_hSocket;
-    RecursiveMutex cs_vRecv;
+    Mutex cs_vSend;
+    Mutex cs_hSocket;
+    Mutex cs_vRecv;
 
     RecursiveMutex cs_vProcessMsg;
     std::list<CNetMessage> vProcessMsg GUARDED_BY(cs_vProcessMsg);

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -1475,18 +1475,23 @@ void PeerManagerImpl::Misbehaving(const NodeId pnode, const int howmuch, const s
     if (peer == nullptr) return;
 
     LOCK(peer->m_misbehavior_mutex);
+    const int score_before{peer->m_misbehavior_score};
     peer->m_misbehavior_score += howmuch;
-    const int banscore = gArgs.GetArg("-banscore", DEFAULT_BANSCORE_THRESHOLD);
+    const int score_now{peer->m_misbehavior_score};
+
     const std::string message_prefixed = message.empty() ? "" : (": " + message);
-    if (peer->m_misbehavior_score >= banscore && peer->m_misbehavior_score - howmuch < banscore)
-    {
-        LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d) DISCOURAGE THRESHOLD EXCEEDED%s\n", pnode, peer->m_misbehavior_score - howmuch, peer->m_misbehavior_score, message_prefixed);
+    std::string warning;
+
+    if (score_now >= DISCOURAGEMENT_THRESHOLD && score_before < DISCOURAGEMENT_THRESHOLD) {
+        warning = " DISCOURAGE THRESHOLD EXCEEDED";
         peer->m_should_discourage = true;
         statsClient.inc("misbehavior.banned", 1.0f);
     } else {
-        LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d)%s\n", pnode, peer->m_misbehavior_score - howmuch, peer->m_misbehavior_score, message_prefixed);
         statsClient.count("misbehavior.amount", howmuch, 1.0);
     }
+
+    LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d)%s%s\n",
+             pnode, score_before, score_now, warning, message_prefixed);
 }
 
 bool PeerManagerImpl::IsBanned(NodeId pnode)

diff --git a/src/net_processing.h b/src/net_processing.h
@@ -31,6 +31,8 @@ static const unsigned int DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE = 10; // this all
 static const unsigned int DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN = 100;
 static const bool DEFAULT_PEERBLOOMFILTERS = true;
 static const bool DEFAULT_PEERBLOCKFILTERS = false;
+/** Threshold for marking a node to be discouraged, e.g. disconnected and added to the discouragement filter. */
+static const int DISCOURAGEMENT_THRESHOLD{100};
 
 struct CNodeStateStats {
     int m_misbehavior_score = 0;
@@ -64,7 +66,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
     virtual void SetBestHeight(int height) = 0;
 
     /**
-     * Increment peer's misbehavior score. If the new value surpasses banscore (specified on startup or by default), mark node to be discouraged, meaning the peer might be disconnected & added to the discouragement filter.
+     * Increment peer's misbehavior score. If the new value surpasses DISCOURAGEMENT_THRESHOLD (specified on startup or by default), mark node to be discouraged, meaning the peer might be disconnected & added to the discouragement filter.
      */
     virtual void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") = 0;
 

diff --git a/src/qt/forms/debugwindow.ui b/src/qt/forms/debugwindow.ui
@@ -1190,36 +1190,13 @@
                 </widget>
                </item>
                <item row="11" column="0">
-                <widget class="QLabel" name="label_24">
-                 <property name="text">
-                  <string>Ban Score</string>
-                 </property>
-                </widget>
-               </item>
-               <item row="11" column="2">
-                <widget class="QLabel" name="peerBanScore">
-                 <property name="cursor">
-                  <cursorShape>IBeamCursor</cursorShape>
-                 </property>
-                 <property name="text">
-                  <string>N/A</string>
-                 </property>
-                 <property name="textFormat">
-                  <enum>Qt::PlainText</enum>
-                 </property>
-                 <property name="textInteractionFlags">
-                  <set>Qt::LinksAccessibleByMouse|Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse</set>
-                 </property>
-                </widget>
-               </item>
-               <item row="12" column="0">
                 <widget class="QLabel" name="label_22">
                  <property name="text">
                   <string>Connection Time</string>
                  </property>
                 </widget>
                </item>
-               <item row="12" column="2">
+               <item row="11" column="2">
                 <widget class="QLabel" name="peerConnTime">
                  <property name="cursor">
                   <cursorShape>IBeamCursor</cursorShape>
@@ -1235,14 +1212,14 @@
                  </property>
                 </widget>
                </item>
-               <item row="13" column="0">
+               <item row="12" column="0">
                 <widget class="QLabel" name="label_15">
                  <property name="text">
                   <string>Last Send</string>
                  </property>
                 </widget>
                </item>
-               <item row="13" column="2">
+               <item row="12" column="2">
                 <widget class="QLabel" name="peerLastSend">
                  <property name="cursor">
                   <cursorShape>IBeamCursor</cursorShape>
@@ -1258,14 +1235,14 @@
                  </property>
                 </widget>
                </item>
-               <item row="14" column="0">
+               <item row="13" column="0">
                 <widget class="QLabel" name="label_19">
                  <property name="text">
                   <string>Last Receive</string>
                  </property>
                 </widget>
                </item>
-               <item row="14" column="2">
+               <item row="13" column="2">
                 <widget class="QLabel" name="peerLastRecv">
                  <property name="cursor">
                   <cursorShape>IBeamCursor</cursorShape>
@@ -1281,14 +1258,14 @@
                  </property>
                 </widget>
                </item>
-               <item row="15" column="0">
+               <item row="14" column="0">
                 <widget class="QLabel" name="label_18">
                  <property name="text">
                   <string>Sent</string>
                  </property>
                 </widget>
                </item>
-               <item row="15" column="2">
+               <item row="14" column="2">
                 <widget class="QLabel" name="peerBytesSent">
                  <property name="cursor">
                   <cursorShape>IBeamCursor</cursorShape>
@@ -1304,14 +1281,14 @@
                  </property>
                 </widget>
                </item>
-               <item row="16" column="0">
+               <item row="15" column="0">
                 <widget class="QLabel" name="label_20">
                  <property name="text">
                   <string>Received</string>
                  </property>
                 </widget>
                </item>
-               <item row="16" column="2">
+               <item row="15" column="2">
                 <widget class="QLabel" name="peerBytesRecv">
                  <property name="cursor">
                   <cursorShape>IBeamCursor</cursorShape>
@@ -1327,14 +1304,14 @@
                  </property>
                 </widget>
                </item>
-               <item row="17" column="0">
+               <item row="16" column="0">
                 <widget class="QLabel" name="label_26">
                  <property name="text">
                   <string>Ping Time</string>
                  </property>
                 </widget>
                </item>
-               <item row="17" column="2">
+               <item row="16" column="2">
                 <widget class="QLabel" name="peerPingTime">
                  <property name="cursor">
                   <cursorShape>IBeamCursor</cursorShape>
@@ -1350,7 +1327,7 @@
                  </property>
                 </widget>
                </item>
-               <item row="18" column="0">
+               <item row="17" column="0">
                 <widget class="QLabel" name="peerPingWaitLabel">
                  <property name="toolTip">
                   <string>The duration of a currently outstanding ping.</string>
@@ -1360,7 +1337,7 @@
                  </property>
                 </widget>
                </item>
-               <item row="18" column="2">
+               <item row="17" column="2">
                 <widget class="QLabel" name="peerPingWait">
                  <property name="cursor">
                   <cursorShape>IBeamCursor</cursorShape>
@@ -1376,14 +1353,14 @@
                  </property>
                 </widget>
                </item>
-               <item row="19" column="0">
+               <item row="18" column="0">
                 <widget class="QLabel" name="peerMinPingLabel">
                  <property name="text">
                   <string>Min Ping</string>
                  </property>
                 </widget>
                </item>
-               <item row="19" column="2">
+               <item row="18" column="2">
                 <widget class="QLabel" name="peerMinPing">
                  <property name="cursor">
                   <cursorShape>IBeamCursor</cursorShape>
@@ -1399,14 +1376,14 @@
                  </property>
                 </widget>
                </item>
-               <item row="20" column="0">
+               <item row="19" column="0">
                 <widget class="QLabel" name="label_timeoffset">
                  <property name="text">
                   <string>Time Offset</string>
                  </property>
                 </widget>
                </item>
-               <item row="20" column="2">
+               <item row="19" column="2">
                 <widget class="QLabel" name="timeoffset">
                  <property name="cursor">
                   <cursorShape>IBeamCursor</cursorShape>
@@ -1422,7 +1399,7 @@
                  </property>
                 </widget>
                </item>
-               <item row="21" column="0">
+               <item row="20" column="0">
                 <widget class="QLabel" name="peerMappedASLabel">
                  <property name="toolTip">
                   <string>The mapped Autonomous System used for diversifying peer selection.</string>
@@ -1432,7 +1409,7 @@
                  </property>
                 </widget>
                </item>
-               <item row="21" column="2">
+               <item row="20" column="2">
                 <widget class="QLabel" name="peerMappedAS">
                  <property name="cursor">
                   <cursorShape>IBeamCursor</cursorShape>
@@ -1448,7 +1425,7 @@
                  </property>
                 </widget>
                </item>
-               <item row="22" column="0">
+               <item row="21" column="0">
                 <spacer name="verticalSpacer_3">
                  <property name="orientation">
                   <enum>Qt::Vertical</enum>

diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
@@ -1287,9 +1287,6 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats)
     // This check fails for example if the lock was busy and
     // nodeStateStats couldn't be fetched.
     if (stats->fNodeStateStatsAvailable) {
-        // Ban score is init to 0
-        ui->peerBanScore->setText(QString("%1").arg(stats->nodeStateStats.m_misbehavior_score));
-
         // Sync height is init to -1
         if (stats->nodeStateStats.nSyncHeight > -1)
             ui->peerSyncHeight->setText(QString("%1").arg(stats->nodeStateStats.nSyncHeight));

diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
@@ -138,7 +138,7 @@ static RPCHelpMan getpeerinfo()
                                                               "best capture connection behaviors."},
                     {RPCResult::Type::BOOL, "masternode", "Whether connection was due to masternode connection attempt"},
                     {RPCResult::Type::NUM, "startingheight", "The starting height (block) of the peer"},
-                    {RPCResult::Type::NUM, "banscore", "The ban score"},
+                    {RPCResult::Type::NUM, "banscore", "The ban score (DEPRECATED, returned only if config option -deprecatedrpc=banscore is passed)"},
                     {RPCResult::Type::NUM, "synced_headers", "The last header we have in common with this peer"},
                     {RPCResult::Type::NUM, "synced_blocks", "The last block we have in common with this peer"},
                     {RPCResult::Type::ARR, "inflight", "",
@@ -228,7 +228,10 @@ static RPCHelpMan getpeerinfo()
         obj.pushKV("masternode", stats.m_masternode_connection);
         obj.pushKV("startingheight", stats.nStartingHeight);
         if (fStateStats) {
-            obj.pushKV("banscore", statestats.m_misbehavior_score);
+            if (IsDeprecatedRPCEnabled("banscore")) {
+                // banscore is deprecated in v21 for removal in v22
+                obj.pushKV("banscore", statestats.m_misbehavior_score);
+            }
             obj.pushKV("synced_headers", statestats.nSyncHeight);
             obj.pushKV("synced_blocks", statestats.nCommonHeight);
             UniValue heights(UniValue::VARR);