diff --git a/doc/release-notes-19464.md b/doc/release-notes-19464.md new file mode 100644 index 0000000000..b1161c2859 --- /dev/null +++ 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 new file mode 100644 index 0000000000..0fb5a2f52b --- /dev/null +++ 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 index ea49bb0aa6..c60eb0e286 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -572,7 +572,6 @@ void SetupServerArgs(NodeContext& node) argsman.AddArg("-asmap=", 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=", "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=", strprintf("Threshold for disconnecting and discouraging misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-bantime=", strprintf("Default duration (in seconds) of manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-bind=[:][=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=", "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 index a8f2413ef9..91bf703262 100644 --- a/src/net.h +++ b/src/net.h @@ -440,9 +440,9 @@ public: uint64_t nSendBytes GUARDED_BY(cs_vSend){0}; std::list> vSendMsg GUARDED_BY(cs_vSend); std::atomic 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 vProcessMsg GUARDED_BY(cs_vProcessMsg); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3ec719e7c8..fb8c1fc66d 100644 --- 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 index 0d422b6288..b4f47419e9 100644 --- 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 @@ public: 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 index 2f20c49092..96df636296 100644 --- a/src/qt/forms/debugwindow.ui +++ b/src/qt/forms/debugwindow.ui @@ -1190,36 +1190,13 @@ - - - Ban Score - - - - - - - IBeamCursor - - - N/A - - - Qt::PlainText - - - Qt::LinksAccessibleByMouse|Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse - - - - Connection Time - + IBeamCursor @@ -1235,14 +1212,14 @@ - + Last Send - + IBeamCursor @@ -1258,14 +1235,14 @@ - + Last Receive - + IBeamCursor @@ -1281,14 +1258,14 @@ - + Sent - + IBeamCursor @@ -1304,14 +1281,14 @@ - + Received - + IBeamCursor @@ -1327,14 +1304,14 @@ - + Ping Time - + IBeamCursor @@ -1350,7 +1327,7 @@ - + The duration of a currently outstanding ping. @@ -1360,7 +1337,7 @@ - + IBeamCursor @@ -1376,14 +1353,14 @@ - + Min Ping - + IBeamCursor @@ -1399,14 +1376,14 @@ - + Time Offset - + IBeamCursor @@ -1422,7 +1399,7 @@ - + The mapped Autonomous System used for diversifying peer selection. @@ -1432,7 +1409,7 @@ - + IBeamCursor @@ -1448,7 +1425,7 @@ - + Qt::Vertical diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index d3a99707a9..27944efde7 100644 --- 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 index 6de82e6a7c..96840bd566 100644 --- 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); diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 392b9707b4..0ae1ad5c31 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -204,7 +204,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) connman->ClearTestNodes(); } -BOOST_AUTO_TEST_CASE(DoS_banning) +BOOST_AUTO_TEST_CASE(peer_discouragement) { const CChainParams& chainparams = Params(); auto banman = std::make_unique(GetDataDir() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); @@ -219,82 +219,38 @@ BOOST_AUTO_TEST_CASE(DoS_banning) dummyNode1.SetCommonVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode1); dummyNode1.fSuccessfullyConnected = true; - peerLogic->Misbehaving(dummyNode1.GetId(), 100); // Should get banned + peerLogic->Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged { LOCK(dummyNode1.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); } BOOST_CHECK(banman->IsDiscouraged(addr1)); - BOOST_CHECK(!banman->IsDiscouraged(ip(0xa0b0c001|0x0000ff00))); // Different IP, not banned + BOOST_CHECK(!banman->IsDiscouraged(ip(0xa0b0c001|0x0000ff00))); // Different IP, not discouraged CAddress addr2(ip(0xa0b0c002), NODE_NONE); CNode dummyNode2(id++, NODE_NETWORK, INVALID_SOCKET, addr2, 1, 1, CAddress(), "", ConnectionType::INBOUND); dummyNode2.SetCommonVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode2); dummyNode2.fSuccessfullyConnected = true; - peerLogic->Misbehaving(dummyNode2.GetId(), 50); + peerLogic->Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1); { LOCK(dummyNode2.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode2)); } - BOOST_CHECK(!banman->IsDiscouraged(addr2)); // 2 not banned yet... + BOOST_CHECK(!banman->IsDiscouraged(addr2)); // 2 not discouraged yet... BOOST_CHECK(banman->IsDiscouraged(addr1)); // ... but 1 still should be - peerLogic->Misbehaving(dummyNode2.GetId(), 50); + peerLogic->Misbehaving(dummyNode2.GetId(), 1); // 2 reaches discouragement threshold { LOCK(dummyNode2.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode2)); } - BOOST_CHECK(banman->IsDiscouraged(addr2)); + BOOST_CHECK(banman->IsDiscouraged(addr1)); // Expect both 1 and 2 + BOOST_CHECK(banman->IsDiscouraged(addr2)); // to be discouraged now peerLogic->FinalizeNode(dummyNode1); peerLogic->FinalizeNode(dummyNode2); } -BOOST_AUTO_TEST_CASE(DoS_banscore) -{ - const CChainParams& chainparams = Params(); - auto banman = std::make_unique(GetDataDir() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); - auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); - auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(), *m_node.scheduler, - *m_node.chainman, *m_node.mempool, *m_node.govman, *m_node.sporkman, - ::deterministicMNManager, m_node.cj_ctx, m_node.llmq_ctx, false); - - banman->ClearBanned(); - gArgs.ForceSetArg("-banscore", "111"); // because 11 is my favorite number - CAddress addr1(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode1(id++, NODE_NETWORK, INVALID_SOCKET, addr1, 3, 1, CAddress(), "", ConnectionType::INBOUND); - dummyNode1.SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(&dummyNode1); - dummyNode1.fSuccessfullyConnected = true; - { - peerLogic->Misbehaving(dummyNode1.GetId(), 100); - } - { - LOCK(dummyNode1.cs_sendProcessing); - BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); - } - BOOST_CHECK(!banman->IsDiscouraged(addr1)); - { - peerLogic->Misbehaving(dummyNode1.GetId(), 10); - } - { - LOCK(dummyNode1.cs_sendProcessing); - BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); - } - BOOST_CHECK(!banman->IsDiscouraged(addr1)); - { - peerLogic->Misbehaving(dummyNode1.GetId(), 1); - } - { - LOCK(dummyNode1.cs_sendProcessing); - BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); - } - BOOST_CHECK(banman->IsDiscouraged(addr1)); - gArgs.ForceSetArg("-banscore", ToString(DEFAULT_BANSCORE_THRESHOLD)); - - peerLogic->FinalizeNode(dummyNode1); -} - BOOST_AUTO_TEST_CASE(DoS_bantime) { const CChainParams& chainparams = Params(); @@ -314,7 +270,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) peerLogic->InitializeNode(&dummyNode); dummyNode.fSuccessfullyConnected = true; - peerLogic->Misbehaving(dummyNode.GetId(), 100); + peerLogic->Misbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD); { LOCK(dummyNode.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode)); diff --git a/src/validation.h b/src/validation.h index 7a4041d5f7..3bfe6b872d 100644 --- a/src/validation.h +++ b/src/validation.h @@ -107,7 +107,6 @@ static const bool DEFAULT_ADDRESSINDEX = false; static const bool DEFAULT_TIMESTAMPINDEX = false; static const bool DEFAULT_SPENTINDEX = false; static const char* const DEFAULT_BLOCKFILTERINDEX = "0"; -static const unsigned int DEFAULT_BANSCORE_THRESHOLD = 100; /** Default for -persistmempool */ static const bool DEFAULT_PERSIST_MEMPOOL = true; /** Default for -syncmempool */ diff --git a/test/functional/p2p_leak.py b/test/functional/p2p_leak.py index 598e3f06e8..de4188174c 100755 --- a/test/functional/p2p_leak.py +++ b/test/functional/p2p_leak.py @@ -25,7 +25,7 @@ from test_framework.util import ( assert_greater_than_or_equal, ) -banscore = 10 +DISCOURAGEMENT_THRESHOLD = 100 class LazyPeer(P2PInterface): @@ -91,7 +91,6 @@ class P2PVersionStore(P2PInterface): class P2PLeakTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 - self.extra_args = [['-banscore=' + str(banscore)]] def setup_network(self): self.disable_mocktime() @@ -100,7 +99,7 @@ class P2PLeakTest(BitcoinTestFramework): def run_test(self): # Peer that never sends a version. We will send a bunch of messages # from this peer anyway and verify eventual disconnection. - no_version_ban_peer = self.nodes[0].add_p2p_connection( + no_version_disconnect_peer = self.nodes[0].add_p2p_connection( LazyPeer(), send_version=False, wait_for_verack=False) # Another peer that never sends a version, nor any other messages. It shouldn't receive anything from the node. @@ -111,14 +110,14 @@ class P2PLeakTest(BitcoinTestFramework): # Send enough ping messages (any non-version message will do) prior to sending # version to reach the peer discouragement threshold. This should get us disconnected. - for _ in range(banscore): - no_version_ban_peer.send_message(msg_ping()) + for _ in range(DISCOURAGEMENT_THRESHOLD): + no_version_disconnect_peer.send_message(msg_ping()) # Wait until we got the verack in response to the version. Though, don't wait for the node to receive the # verack, since we never sent one no_verack_idle_peer.wait_for_verack() - no_version_ban_peer.wait_until(lambda: no_version_ban_peer.ever_connected, check_connected=False) + no_version_disconnect_peer.wait_until(lambda: no_version_disconnect_peer.ever_connected, check_connected=False) no_version_idle_peer.wait_until(lambda: no_version_idle_peer.ever_connected) no_verack_idle_peer.wait_until(lambda: no_verack_idle_peer.version_received) @@ -129,12 +128,12 @@ class P2PLeakTest(BitcoinTestFramework): time.sleep(5) #This peer should have been banned - assert not no_version_ban_peer.is_connected + assert not no_version_disconnect_peer.is_connected self.nodes[0].disconnect_p2ps() # Make sure no unexpected messages came in - assert no_version_ban_peer.unexpected_msg == False + assert no_version_disconnect_peer.unexpected_msg == False assert no_version_idle_peer.unexpected_msg == False assert no_verack_idle_peer.unexpected_msg == False diff --git a/test/functional/p2p_quorum_data.py b/test/functional/p2p_quorum_data.py index 08765bc8f3..d14ce4874d 100755 --- a/test/functional/p2p_quorum_data.py +++ b/test/functional/p2p_quorum_data.py @@ -117,7 +117,7 @@ class QuorumDataInterface(P2PInterface): class QuorumDataMessagesTest(DashTestFramework): def set_test_params(self): - extra_args = [["-llmq-data-recovery=0"]] * 4 + extra_args = [["-llmq-data-recovery=0", "-deprecatedrpc=banscore"]] * 4 self.set_dash_test_params(4, 3, fast_dip3_enforcement=True, extra_args=extra_args) def restart_mn(self, mn, reindex=False): diff --git a/test/functional/rpc_getpeerinfo_banscore_deprecation.py b/test/functional/rpc_getpeerinfo_banscore_deprecation.py new file mode 100755 index 0000000000..b830248e1e --- /dev/null +++ b/test/functional/rpc_getpeerinfo_banscore_deprecation.py @@ -0,0 +1,24 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test deprecation of getpeerinfo RPC banscore field.""" + +from test_framework.test_framework import BitcoinTestFramework + + +class GetpeerinfoBanscoreDeprecationTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 2 + self.extra_args = [[], ["-deprecatedrpc=banscore"]] + + def run_test(self): + self.log.info("Test getpeerinfo by default no longer returns a banscore field") + assert "banscore" not in self.nodes[0].getpeerinfo()[0].keys() + + self.log.info("Test getpeerinfo returns banscore with -deprecatedrpc=banscore") + assert "banscore" in self.nodes[1].getpeerinfo()[0].keys() + + +if __name__ == "__main__": + GetpeerinfoBanscoreDeprecationTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index dc75bd0eb0..5c846488eb 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -326,6 +326,7 @@ BASE_SCRIPTS = [ 'feature_config_args.py', 'feature_settings.py', 'rpc_getdescriptorinfo.py', + 'rpc_getpeerinfo_banscore_deprecation.py', 'rpc_help.py', 'feature_help.py', 'feature_blockfilterindex_prune.py'