diff --git a/src/addrdb.h b/src/addrdb.h --- a/src/addrdb.h +++ b/src/addrdb.h @@ -17,19 +17,12 @@ class CDataStream; class CChainParams; -typedef enum BanReason { - BanReasonUnknown = 0, - BanReasonNodeMisbehaving = 1, - BanReasonManuallyAdded = 2 -} BanReason; - class CBanEntry { public: static const int CURRENT_VERSION = 1; int nVersion; int64_t nCreateTime; int64_t nBanUntil; - uint8_t banReason; CBanEntry() { SetNull(); } @@ -38,37 +31,22 @@ nCreateTime = nCreateTimeIn; } - explicit CBanEntry(int64_t n_create_time_in, BanReason ban_reason_in) - : CBanEntry(n_create_time_in) { - banReason = ban_reason_in; - } - ADD_SERIALIZE_METHODS; template inline void SerializationOp(Stream &s, Operation ser_action) { + //! For backward compatibility + uint8_t ban_reason = 2; READWRITE(this->nVersion); READWRITE(nCreateTime); READWRITE(nBanUntil); - READWRITE(banReason); + READWRITE(ban_reason); } void SetNull() { nVersion = CBanEntry::CURRENT_VERSION; nCreateTime = 0; nBanUntil = 0; - banReason = BanReasonUnknown; - } - - std::string banReasonToString() const { - switch (banReason) { - case BanReasonNodeMisbehaving: - return "node misbehaving"; - case BanReasonManuallyAdded: - return "manually added"; - default: - return "unknown"; - } } }; diff --git a/src/banman.h b/src/banman.h --- a/src/banman.h +++ b/src/banman.h @@ -59,14 +59,22 @@ ~BanMan(); BanMan(fs::path ban_file, const CChainParams &chainparams, CClientUIInterface *client_interface, int64_t default_ban_time); - void Ban(const CNetAddr &net_addr, const BanReason &ban_reason, - int64_t ban_time_offset = 0, bool since_unix_epoch = false); - void Ban(const CSubNet &sub_net, const BanReason &ban_reason, - int64_t ban_time_offset = 0, bool since_unix_epoch = false); + void Ban(const CNetAddr &net_addr, int64_t ban_time_offset = 0, + bool since_unix_epoch = false); + void Ban(const CSubNet &sub_net, int64_t ban_time_offset = 0, + bool since_unix_epoch = false); + void Discourage(const CNetAddr &net_addr); void ClearBanned(); - int IsBannedLevel(CNetAddr net_addr); - bool IsBanned(CNetAddr net_addr); - bool IsBanned(CSubNet sub_net); + + //! Return whether net_addr is banned + bool IsBanned(const CNetAddr &net_addr); + + //! Return whether sub_net is exactly banned + bool IsBanned(const CSubNet &sub_net); + + //! Return whether net_addr is discouraged. + bool IsDiscouraged(const CNetAddr &net_addr); + bool Unban(const CNetAddr &net_addr); bool Unban(const CSubNet &sub_net); void GetBanned(banmap_t &banmap); diff --git a/src/banman.cpp b/src/banman.cpp --- a/src/banman.cpp +++ b/src/banman.cpp @@ -81,30 +81,14 @@ } } -int BanMan::IsBannedLevel(CNetAddr net_addr) { - // Returns the most severe level of banning that applies to this address. - // 0 - Not banned - // 1 - Automatic misbehavior ban - // 2 - Any other ban - auto current_time = GetTime(); +bool BanMan::IsDiscouraged(const CNetAddr &net_addr) { LOCK(m_cs_banned); - for (const auto &it : m_banned) { - CSubNet sub_net = it.first; - CBanEntry ban_entry = it.second; - - if (current_time < ban_entry.nBanUntil && sub_net.Match(net_addr)) { - return 2; - } - } - return m_discouraged.contains(net_addr.GetAddrBytes()) ? 1 : 0; + return m_discouraged.contains(net_addr.GetAddrBytes()); } -bool BanMan::IsBanned(CNetAddr net_addr) { +bool BanMan::IsBanned(const CNetAddr &net_addr) { auto current_time = GetTime(); LOCK(m_cs_banned); - if (m_discouraged.contains(net_addr.GetAddrBytes())) { - return true; - } for (const auto &it : m_banned) { CSubNet sub_net = it.first; CBanEntry ban_entry = it.second; @@ -116,7 +100,7 @@ return false; } -bool BanMan::IsBanned(CSubNet sub_net) { +bool BanMan::IsBanned(const CSubNet &sub_net) { auto current_time = GetTime(); LOCK(m_cs_banned); banmap_t::iterator i = m_banned.find(sub_net); @@ -129,21 +113,20 @@ return false; } -void BanMan::Ban(const CNetAddr &net_addr, const BanReason &ban_reason, - int64_t ban_time_offset, bool since_unix_epoch) { - if (ban_reason == BanReasonNodeMisbehaving) { - LOCK(m_cs_banned); - m_discouraged.insert(net_addr.GetAddrBytes()); - return; - } +void BanMan::Ban(const CNetAddr &net_addr, int64_t ban_time_offset, + bool since_unix_epoch) { CSubNet sub_net(net_addr); - Ban(sub_net, ban_reason, ban_time_offset, since_unix_epoch); + Ban(sub_net, ban_time_offset, since_unix_epoch); +} + +void BanMan::Discourage(const CNetAddr &net_addr) { + LOCK(m_cs_banned); + m_discouraged.insert(net_addr.GetAddrBytes()); } -void BanMan::Ban(const CSubNet &sub_net, const BanReason &ban_reason, - int64_t ban_time_offset, bool since_unix_epoch) { - assert(ban_reason == BanReasonManuallyAdded); - CBanEntry ban_entry(GetTime(), ban_reason); +void BanMan::Ban(const CSubNet &sub_net, int64_t ban_time_offset, + bool since_unix_epoch) { + CBanEntry ban_entry(GetTime()); int64_t normalized_ban_time_offset = ban_time_offset; bool normalized_since_unix_epoch = since_unix_epoch; @@ -167,10 +150,8 @@ m_client_interface->BannedListChanged(); } - // store banlist to disk immediately if user requested ban - if (ban_reason == BanReasonManuallyAdded) { - DumpBanlist(); - } + // store banlist to disk immediately + DumpBanlist(); } bool BanMan::Unban(const CNetAddr &net_addr) { diff --git a/src/interfaces/node.h b/src/interfaces/node.h --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -119,8 +119,7 @@ virtual bool getBanned(banmap_t &banmap) = 0; //! Ban node. - virtual bool ban(const CNetAddr &net_addr, BanReason reason, - int64_t ban_time_offset) = 0; + virtual bool ban(const CNetAddr &net_addr, int64_t ban_time_offset) = 0; //! Unban node. virtual bool unban(const CSubNet &ip) = 0; diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp --- a/src/interfaces/node.cpp +++ b/src/interfaces/node.cpp @@ -160,10 +160,9 @@ } return false; } - bool ban(const CNetAddr &net_addr, BanReason reason, - int64_t ban_time_offset) override { + bool ban(const CNetAddr &net_addr, int64_t ban_time_offset) override { if (m_context.banman) { - m_context.banman->Ban(net_addr, reason, ban_time_offset); + m_context.banman->Ban(net_addr, ban_time_offset); return true; } return false; diff --git a/src/net.cpp b/src/net.cpp --- a/src/net.cpp +++ b/src/net.cpp @@ -1057,12 +1057,11 @@ // sockets on all platforms. Set it again here just to be sure. SetSocketNoDelay(hSocket); - int bannedlevel = m_banman ? m_banman->IsBannedLevel(addr) : 0; - // Don't accept connections from banned peers. + bool banned = m_banman->IsBanned(addr); if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && - bannedlevel == 2) { + banned) { LogPrint(BCLog::NET, "connection from %s dropped (banned)\n", addr.ToString()); CloseSocket(hSocket); @@ -1071,9 +1070,10 @@ // Only accept connections from discouraged peers if our inbound slots // aren't (almost) full. + bool discouraged = m_banman->IsDiscouraged(addr); if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && - nInbound + 1 >= nMaxInbound && bannedlevel >= 1) { + nInbound + 1 >= nMaxInbound && discouraged) { LogPrint(BCLog::NET, "connection from %s dropped (discouraged)\n", addr.ToString()); CloseSocket(hSocket); @@ -1108,7 +1108,7 @@ // If this flag is present, the user probably expect that RPC and QT report // it as whitelisted (backward compatibility) pnode->m_legacyWhitelisted = legacyWhitelisted; - pnode->m_prefer_evict = bannedlevel > 0; + pnode->m_prefer_evict = discouraged; m_msgproc->InitializeNode(*config, pnode); LogPrint(BCLog::NET, "connection from %s accepted\n", addr.ToString()); @@ -2124,10 +2124,12 @@ return; } if (!pszDest) { + bool banned_or_discouraged = + m_banman && (m_banman->IsDiscouraged(addrConnect) || + m_banman->IsBanned(addrConnect)); if (IsLocal(addrConnect) || FindNode(static_cast(addrConnect)) || - (m_banman && m_banman->IsBanned(addrConnect)) || - FindNode(addrConnect.ToStringIPPort())) { + banned_or_discouraged || FindNode(addrConnect.ToStringIPPort())) { return; } } else if (FindNode(std::string(pszDest))) { diff --git a/src/net_permissions.h b/src/net_permissions.h --- a/src/net_permissions.h +++ b/src/net_permissions.h @@ -19,7 +19,7 @@ // Always relay transactions from this peer, even if already in mempool or // rejected from policy Keep parameter interaction: forcerelay implies relay PF_FORCERELAY = (1U << 2) | PF_RELAY, - // Can't be banned for misbehavior + // Can't be banned/disconnected/discouraged for misbehavior PF_NOBAN = (1U << 4), // Can query the mempool PF_MEMPOOL = (1U << 5), diff --git a/src/net_processing.cpp b/src/net_processing.cpp --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -275,9 +275,9 @@ bool fCurrentlyConnected; //! Accumulated misbehaviour score for this peer. int nMisbehavior; - //! Whether this peer should be disconnected and banned (unless - //! whitelisted). - bool fShouldBan; + //! Whether this peer should be disconnected and marked as discouraged + //! (unless whitelisted with noban). + bool m_should_discourage; //! String name of this peer (debugging/logging purposes). const std::string name; //! List of asynchronously-determined block rejections to notify this peer @@ -442,7 +442,7 @@ m_is_inbound(is_inbound), m_is_manual_connection(is_manual) { fCurrentlyConnected = false; nMisbehavior = 0; - fShouldBan = false; + m_should_discourage = false; pindexBestKnownBlock = nullptr; hashLastUnknownBlock = BlockHash(); pindexLastCommonBlock = nullptr; @@ -1182,7 +1182,7 @@ "%s: %s peer=%d (%d -> %d) reason: %s BAN THRESHOLD EXCEEDED\n", __func__, state->name, pnode, state->nMisbehavior - howmuch, state->nMisbehavior, reason); - state->fShouldBan = true; + state->m_should_discourage = true; } else { LogPrintf("%s: %s peer=%d (%d -> %d) reason: %s\n", __func__, state->name, pnode, state->nMisbehavior - howmuch, @@ -2598,9 +2598,12 @@ addr.nTime = nNow - 5 * 24 * 60 * 60; } pfrom->AddAddressKnown(addr); + // Do not process banned/discouraged addresses beyond remembering we + // received them + if (banman->IsDiscouraged(addr)) { + continue; + } if (banman->IsBanned(addr)) { - // Do not process banned addresses beyond remembering - // we received them continue; } bool fReachable = IsReachable(addr); @@ -3735,7 +3738,7 @@ std::vector vAddr = connman->GetAddresses(); FastRandomContext insecure_rand; for (const CAddress &addr : vAddr) { - if (!banman->IsBanned(addr)) { + if (!banman->IsDiscouraged(addr) && !banman->IsBanned(addr)) { pfrom->PushAddress(addr, insecure_rand); } } @@ -3979,8 +3982,8 @@ } state.rejects.clear(); - if (state.fShouldBan) { - state.fShouldBan = false; + if (state.m_should_discourage) { + state.m_should_discourage = false; if (pnode->HasPermission(PF_NOBAN)) { LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode->addr.ToString()); @@ -3988,17 +3991,17 @@ LogPrintf("Warning: not punishing manually-connected peer %s!\n", pnode->addr.ToString()); } else if (pnode->addr.IsLocal()) { - // Disconnect but don't ban _this_ local node + // Disconnect but don't discourage this local node LogPrintf( "Warning: disconnecting but not discouraging local peer %s!\n", pnode->addr.ToString()); pnode->fDisconnect = true; } else { - // Disconnect and ban all nodes sharing the address + // Disconnect and discourage all nodes sharing the address LogPrintf("Disconnecting and discouraging peer %s!\n", pnode->addr.ToString()); if (m_banman) { - m_banman->Ban(pnode->addr, BanReasonNodeMisbehaving); + m_banman->Discourage(pnode->addr); } connman->DisconnectNode(pnode->addr); } @@ -4074,9 +4077,9 @@ "PROCESSMESSAGE: INVALID MESSAGESTART %s peer=%d\n", SanitizeString(msg.hdr.GetCommand()), pfrom->GetId()); - // Make sure we ban where that come from for some time. + // Make sure we discourage where that come from for some time. if (m_banman) { - m_banman->Ban(pfrom->addr, BanReasonNodeMisbehaving); + m_banman->Discourage(pfrom->addr); } connman->DisconnectNode(pfrom->addr); @@ -4111,7 +4114,7 @@ hdr.pchChecksum + CMessageHeader::CHECKSUM_SIZE), pfrom->GetId()); if (m_banman) { - m_banman->Ban(pfrom->addr, BanReasonNodeMisbehaving); + m_banman->Discourage(pfrom->addr); } connman->DisconnectNode(pfrom->addr); return fMoreWork; diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1420,7 +1420,7 @@ const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(detailNodeRow); if (stats) { - m_node.ban(stats->nodeStats.addr, BanReasonManuallyAdded, bantime); + m_node.ban(stats->nodeStats.addr, bantime); m_node.disconnect(stats->nodeStats.addr); } } diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -701,9 +701,8 @@ } if (strCommand == "add") { - if ((isSubnet && g_rpc_node->banman->IsBanned(subNet)) || - (!isSubnet && g_rpc_node->banman->IsBannedLevel(netAddr) == - BanReasonManuallyAdded)) { + if (isSubnet ? g_rpc_node->banman->IsBanned(subNet) + : g_rpc_node->banman->IsBanned(netAddr)) { throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED, "Error: IP/Subnet already banned"); } @@ -720,14 +719,12 @@ } if (isSubnet) { - g_rpc_node->banman->Ban(subNet, BanReasonManuallyAdded, banTime, - absolute); + g_rpc_node->banman->Ban(subNet, banTime, absolute); if (g_rpc_node->connman) { g_rpc_node->connman->DisconnectNode(subNet); } } else { - g_rpc_node->banman->Ban(netAddr, BanReasonManuallyAdded, banTime, - absolute); + g_rpc_node->banman->Ban(netAddr, banTime, absolute); if (g_rpc_node->connman) { g_rpc_node->connman->DisconnectNode(netAddr); } diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -263,9 +263,9 @@ BOOST_CHECK( peerLogic->SendMessages(config, &dummyNode1, interruptDummy)); } - BOOST_CHECK(banman->IsBanned(addr1)); - // Different IP, not banned. - BOOST_CHECK(!banman->IsBanned(ip(0xa0b0c001 | 0x0000ff00))); + BOOST_CHECK(banman->IsDiscouraged(addr1)); + // Different IP, not banned + BOOST_CHECK(!banman->IsDiscouraged(ip(0xa0b0c001 | 0x0000ff00))); CAddress addr2(ip(0xa0b0c002), NODE_NONE); CNode dummyNode2(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr2, 1, 1, @@ -284,9 +284,9 @@ peerLogic->SendMessages(config, &dummyNode2, interruptDummy)); } // 2 not banned yet... - BOOST_CHECK(!banman->IsBanned(addr2)); - // ... but 1 still should be. - BOOST_CHECK(banman->IsBanned(addr1)); + BOOST_CHECK(!banman->IsDiscouraged(addr2)); + // ... but 1 still should be + BOOST_CHECK(banman->IsDiscouraged(addr1)); { LOCK(cs_main); Misbehaving(dummyNode2.GetId(), 50, ""); @@ -296,7 +296,7 @@ BOOST_CHECK( peerLogic->SendMessages(config, &dummyNode2, interruptDummy)); } - BOOST_CHECK(banman->IsBanned(addr2)); + BOOST_CHECK(banman->IsDiscouraged(addr2)); bool dummy; peerLogic->FinalizeNode(config, dummyNode1.GetId(), dummy); @@ -333,7 +333,7 @@ BOOST_CHECK( peerLogic->SendMessages(config, &dummyNode1, interruptDummy)); } - BOOST_CHECK(!banman->IsBanned(addr1)); + BOOST_CHECK(!banman->IsDiscouraged(addr1)); { LOCK(cs_main); Misbehaving(dummyNode1.GetId(), 10, ""); @@ -343,7 +343,7 @@ BOOST_CHECK( peerLogic->SendMessages(config, &dummyNode1, interruptDummy)); } - BOOST_CHECK(!banman->IsBanned(addr1)); + BOOST_CHECK(!banman->IsDiscouraged(addr1)); { LOCK(cs_main); Misbehaving(dummyNode1.GetId(), 1, ""); @@ -353,7 +353,7 @@ BOOST_CHECK( peerLogic->SendMessages(config, &dummyNode1, interruptDummy)); } - BOOST_CHECK(banman->IsBanned(addr1)); + BOOST_CHECK(banman->IsDiscouraged(addr1)); gArgs.ForceSetArg("-banscore", std::to_string(DEFAULT_BANSCORE_THRESHOLD)); bool dummy; @@ -393,7 +393,7 @@ BOOST_CHECK( peerLogic->SendMessages(config, &dummyNode, interruptDummy)); } - BOOST_CHECK(banman->IsBanned(addr)); + BOOST_CHECK(banman->IsDiscouraged(addr)); bool dummy; peerLogic->FinalizeNode(config, dummyNode.GetId(), dummy); diff --git a/src/test/fuzz/addrdb.cpp b/src/test/fuzz/addrdb.cpp --- a/src/test/fuzz/addrdb.cpp +++ b/src/test/fuzz/addrdb.cpp @@ -16,22 +16,13 @@ void test_one_input(const std::vector &buffer) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); - const CBanEntry ban_entry = [&] { - switch (fuzzed_data_provider.ConsumeIntegralInRange(0, 3)) { + [&] { + switch (fuzzed_data_provider.ConsumeIntegralInRange(0, 2)) { case 0: return CBanEntry{ fuzzed_data_provider.ConsumeIntegral()}; break; - case 1: - return CBanEntry{ - fuzzed_data_provider.ConsumeIntegral(), - fuzzed_data_provider.PickValueInArray({ - BanReason::BanReasonUnknown, - BanReason::BanReasonNodeMisbehaving, - BanReason::BanReasonManuallyAdded, - })}; - break; - case 2: { + case 1: { const Optional ban_entry = ConsumeDeserializable(fuzzed_data_provider); if (ban_entry) { @@ -42,5 +33,4 @@ } return CBanEntry{}; }(); - assert(!ban_entry.banReasonToString().empty()); }