Replace automatic bans with discouragement filter

sipa commented at 5:41 am on June 9, 2020: member

This patch improves performance and resource usage around IP addresses that are banned for misbehavior. They’re already not actually banned since #14929, as connections from them are still allowed, but they are preferred for eviction if the inbound connection slots are full.

Stop treating these like manually banned IP ranges, and instead just keep them in a rolling Bloom filter of misbehaving nodes, which isn’t persisted to disk or exposed through the ban framework. The effect remains the same: preferred for eviction, avoided for outgoing connections, and not relayed to other peers.

Also change the name of this mechanism to “discouraged” to better reflect reality.

fanquake added the label P2P on Jun 9, 2020

luke-jr changes_requested

luke-jr commented at 6:08 am on June 9, 2020: member

Concept ACK

I think we need to skip over BanReasonNodeMisbehaving entries when loading existing ban data?

in src/banman.h:80 in 4c4df6bf61 outdated

76@@ -68,6 +77,7 @@ class BanMan
77     CClientUIInterface* m_client_interface = nullptr;
78     CBanDB m_ban_db;
79     const int64_t m_default_ban_time;
80+    CRollingBloomFilter m_discouraged{50000, 0.000001};

jonatack commented at 9:23 am on June 9, 2020:

A comment on the choice of nElements and nFPRate may be good here. These are the same as for filterInventoryKnown.

jnewbery commented at 9:55 pm on June 9, 2020:

Should this have a GUARDED_BY(m_cs_banned) lock annotation?

sipa commented at 11:13 pm on June 9, 2020:

Done.

jonatack commented at 9:39 am on June 9, 2020: member

Concept ACK on improving the performance/resource usage of IsBanned.

Light code review ACK 4c4df6bf61104a4946479fccc7885dc0fe05dbd8. I have not benchmarked the performance deltas of IsBanned (presumably improved for discouraged addrs) and IsBannedLevel (presumably marginally worse for non-banned addrs) and how meaningful they are with this change.

laanwj added this to the milestone 0.20.1 on Jun 9, 2020

fanquake added the label Needs backport (0.20) on Jun 9, 2020

MarcoFalke commented at 1:31 pm on June 9, 2020: member

review ACK 4c4df6bf61104a4946479fccc7885dc0fe05dbd8 🚆

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 4c4df6bf61104a4946479fccc7885dc0fe05dbd8 🚆
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiFHgv/X2KHSRqmN8x9c5zLH7/3ZqkSkcMGsr1wMBS3GXDqAYv14lYl2xAG4iou
 8TAkeccq8MLDpARLeTtjVou1NhjFcEMZXciV55nF4y7GD8DianWdAeH0/yTlGfjdk
 9yuYGZJpQlHipSccFgOY+pf5pTS9Ph1XgHEabfU2mwGsMRHeo5wIGulBGHTH3oGPZ
10r8aTkNv2o0hQ7Gm68g5k8+cYCcnPYTy8CdO/3CTJMjzY7fgg9ig/CuOWWAq3Pka1
11IcxspupCNtwYKui+rFMXpLar78b1ABTLKGRX2iHBLs09bdOMZgO5jDDCooWx7YAV
12ogNVxmb93p7kRiVtZschxRCnGDBnn6TGJKINNxKwfuwu8BPHilHOwWXmiKUF7EKh
13MwVsVQGYrGz1+uSst4FYhwEXewC6o72kC/7t58mKy2hipyFhMhjQC/CW8v0M9Xsu
14K4CIviPL/AphwRDhTff6nk1uot9VJHaC/JXxaJmnaXiYkckdlu9D2VaKe5n5dm58
15gy6VugHp
16=eVeU
17-----END PGP SIGNATURE-----

Timestamp of file with hash efbc9feace2c4d03c3d87e850e69600a2022027bb9fda48388d15e2d99cfe379 -

laanwj commented at 2:08 pm on June 9, 2020: member

Code review ACK 4c4df6bf61104a4946479fccc7885dc0fe05dbd8

sipa commented at 2:52 pm on June 9, 2020: member

@luke-jr I don’t think that’s necessary. They (by default) have a 24 hour limit only, and during that time, they will just be treated as a real ban instead of a discouragement.

in src/banman.cpp:74 in 4c4df6bf61 outdated

87-            level = 1;
88+            return 2;
89         }
90     }
91-    return level;
92+    return m_discouraged.contains(net_addr.GetAddrBytes());

pstratem commented at 6:55 pm on June 9, 2020:

it feels a bit odd to treat what is essentially an enum (0/1/2) as an int like this, suggest replacing with a conditional returning 1/0 explicitly

sipa commented at 11:13 pm on June 9, 2020:

Ok, added an explicit ? 1 : 0.

in src/banman.h:65 in 4c4df6bf61 outdated

45@@ -45,10 +46,18 @@ class BanMan
46     BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time);
47     void Ban(const CNetAddr& net_addr, const BanReason& ban_reason, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
48     void Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
49+    void Discourage(const CNetAddr& net_addr);
50     void ClearBanned();

jasonbcox commented at 9:25 pm on June 9, 2020:

Looks like ClearBanned() should call m_discouraged.reset() otherwise the clearbanned RPC no longer does what users expect.

sipa commented at 9:31 pm on June 9, 2020:

I considered that, but I think it would be counterintuitive. Discouragement isn’t exposed as bans (it’s not visible or accessible in RPCs or UI) as it’s a completely automatic process, so I think that exposing a way to clear it would violate that abstraction.

jasonbcox commented at 9:37 pm on June 9, 2020:

I see. The current implementation may introduce subtle bugs then, especially considering that IsBanned() is now unintuitively named. setban "someIP" "add" may return Error: IP/Subnet already banned for instance.

sipa commented at 11:13 pm on June 9, 2020:

Nice catch. I’ve addressed it by adding a separate IsBannedOrDiscouraged that’s distinct from IsBanned.

sipa force-pushed on Jun 9, 2020

jnewbery commented at 0:08 am on June 10, 2020: member

This seems like an improvement, but it leaves the BanMan with a slightly odd and inconsistent interface:

Ban() has an enum as its second parameter, but that parameter is only ever set to BanReasonManuallyAdded
If IsBanned() is called with an address, it returns whether an address is discouraged or banned. If it’s called with a subnet, it returns if it’s banned, but not discouraged.
Unban() won’t remove a discouraged address from banman, but it’s not possible to add an outbound connection to that address with addnode onetry.
ClearBanned() (which is called by the clearban RPC (“Clear all banned IPs”)) doesn’t clear discouraged addresses.
The listbanned RPC returns a ban_reason, which can only ever be “manually added”. (banReasonToString() is basically dead code now).

It’d be nice to rationalize this so BanMan just had a setter SetBanLevel() function and a getter GetBanLevel() function.

sipa commented at 0:12 am on June 10, 2020: member

@jnewbery Most of those things are no longer true, apart from the BanReason being superfluous now (there is a separate IsBannedOrDiscouraged). I think a follow-up can remove BanReason altogether, but I think it’s preferable to not have a SetBanLevel - discouragement and banning are very different concepts (and it wouldn’t be unreasonable to move discouragement elsewhere entirely).

DrahtBot commented at 1:24 am on June 10, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19316 ([net] Cleanup logic around connection types by amitiuttarwar)
#19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)
#19243 (banman: Limit resources consumed by misbehaving node deprioitisation by luke-jr)
#19222 (tests: Add fuzzing harness for BanMan by practicalswift)
#19191 (net: Extract download permission from noban by MarcoFalke)
#19147 (Document BanMan with regards to malicious exploitation by ariard)
#19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
#19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)
#19098 (test: Remove duplicate NodeContext hacks by ryanofsky)
#18991 (Cache responses to GETADDR to prevent topology leaks by naumenkogs)
#18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

naumenkogs commented at 1:13 pm on June 10, 2020: member

Concept ACK.

pstratem commented at 6:12 pm on June 10, 2020: contributor

@jnewbery agreed that the interface is a little odd now but think that should be dealt with in a separate pr

cculianu commented at 6:56 pm on June 10, 2020: none

You really should add a benchmark for this..

cculianu commented at 7:20 pm on June 10, 2020: none

What happens if someone floods the filter with a big block of 1 million IPv6 addresses?

sipa commented at 7:33 pm on June 10, 2020: member

Rolling Bloom filters only keep the last N elements added; 50000 in this case. Their performance is not affected by how many elements are kept.

So worst case, being flooded with IPs would reduce the effectiveness of the filter in keeping broken peers out.

cculianu commented at 7:43 pm on June 10, 2020: none

Right, so you can flood the filter with even something like 150k inserts, saturate it to always return false positive, and then all peers are “equally discouraged” (meaning you have no ban policy anymore)….

sipa commented at 7:45 pm on June 10, 2020: member

No, the false positive rate never goes above the specified one (0.000001 in this case). The rolling aspect discards old entries.

jnewbery commented at 7:47 pm on June 10, 2020: member

@cculianu - I suggest you take a look at the CRollingBloomFilter implementation. The rolling bloom filter is actually multiple generations of bloom filters, so you can’t saturate it by adding many entries. It’ll just discard the oldest generation when it reaches (max entries * 3/2).

cculianu commented at 8:20 pm on June 10, 2020: none

Indeed, thanks for the explanation.

cculianu commented at 8:39 pm on June 10, 2020: none

Ok, so basically:

this is as if you had a typical hash table implementation clamped to 75k most recent entries
but it has no ability to unban
it only eats about 67k memory
small tiny chance of false positives.
slightly slower insertions and lookups than a hash table, but not monstrously so.

Got it. Thanks.

sipa commented at 8:44 pm on June 10, 2020: member

@cculianu Indeed. Your numbers are a bit off: it only guarantees 50000 entries (it keeps up to 75000, but the last 25000 are removed whenever that number is exceeded). It also uses around 528 KiB (at an fprate of 1/1000000).

cculianu commented at 8:49 pm on June 10, 2020: none

Oh right.. it’s 67k uint64’s -> 528KiB. My bad.

Hmm. For ~800KiB you could have just implemented a regular hash table (std::unordered_map, etc) or a std::unordered_set, clamped it to 50k items, and then had the ability to query it, iterate over it, show it in the UI, and/or remove individual items.

sipa commented at 8:53 pm on June 10, 2020: member

@cculianu Yeah, we do have a limitedmap data structure that implements effectively that. Its memory usage is significantly worse though (because every entry in the map needs several pointers, plus dynamic allocation overhead). If I recall the number correctly it would be 4 MiB at least for 50000 entries.

cculianu commented at 9:33 pm on June 10, 2020: none

I’m aware of that data structure but it’s not a hash table, it’s a red-black tree based map, which also stores a reverse-map as well. It’s not very efficient for this situation…

You can probably get away with just a straight hash table (unordered_map) with some wrapper that periodically sweeps it to clamp it to some max size – probably just remove the oldest entries by creation time. While not ideal, it has some advantages – namely being able to remove from the set… and blazing fast lookups.

I guess it’s your call – you either pay the memory and upkeep costs and preserve the facility of a traditional ban list and save a few cycles on lookups – or you toss that aside and maintain a bloom set and redefine the semantics of banning (which is already happening anyway)…

sipa commented at 9:38 pm on June 10, 2020: member

Hash table or red-black tree doesn’t matter much. It’s (table size + 2 pointers + alloc overhead per entry) or (3 pointers + 1 int + alloc overhead per entry), and in both cases you need a way to iterate efficiently in order of insertion (either through a separate map like limitedmap does, or by doubly-linking the elements together like is supported by boost::multi_index). Even if you don’t do that and just occasionally sweep, you need to keep track of insertion time per item, and the allocation overheads don’t go away.

In either case, the overhead is much larger than a rolling bloom filter, and as you say - changes to the semantics of auto banned items are happening anyway.

pstratem commented at 11:51 pm on June 10, 2020: contributor

ACK 4131caf2431054adf9ad6641386dfbe922af0dc5

sipa force-pushed on Jun 11, 2020

sipa commented at 0:52 am on June 11, 2020: member

I’ve made a few changes suggested by @jnewbery. The PR is now split into two commits:

The first is a minimal change that implements the new behavior (smaller than the previous PR), plus release notes and RPC documentation changes.
The seconds makes the distinction between discouraging and banning much cleaner in the interface, removing BanReason altogether.

I’m happy to split off the second commit. It could also be ignored in backports.

sipa force-pushed on Jun 11, 2020

in doc/release-notes-19219.md:3 in b6a3448435 outdated

0@@ -0,0 +1,20 @@
1+#### Change in automatic banning
2+
3+Automatic banning of peers for bad behavior has been slightly altered:

luke-jr commented at 4:35 am on June 11, 2020:

Since they’re not really bans, let’s not call them bans in docs?

sipa commented at 5:24 am on June 18, 2020:

I think this is improved now with @jnewbery’s changes.

in src/net.cpp:1018 in 0778faa584 outdated

1015+    bool banned = m_banman && m_banman->IsBanned(addr);
1016 
1017     // Don't accept connections from banned peers, but if our inbound slots aren't almost full, accept
1018     // if the only banning reason was an automatic misbehavior ban.
1019-    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && bannedlevel > ((nInbound + 1 < nMaxInbound) ? 1 : 0))
1020+    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && (banned || (discouraged && nInbound + 1 < nMaxInbound)))

pstratem commented at 5:45 am on June 11, 2020:

maybe this should me two checks so the log can indicate ban/discouraged

jonatack commented at 12:41 pm on June 11, 2020:

0778faa5 For determining if the inbounds are nearly full, should the comparison operator be reversed?

0- (banned || (discouraged && nInbound + 1 < nMaxInbound)))
1+ (banned || (nInbound + 1 >= nMaxInbound && discouraged)))

sipa commented at 5:24 am on June 18, 2020:

This has been rewritten following a suggestion by @pstratem below.

pstratem commented at 6:00 am on June 11, 2020: contributor

ACK 0778faa584871010c82db84c9d54d1353a91a37c

in src/banman.h:81 in 0778faa584 outdated

76@@ -68,6 +77,17 @@ class BanMan
77     CClientUIInterface* m_client_interface = nullptr;
78     CBanDB m_ban_db;
79     const int64_t m_default_ban_time;
80+
81+    /** Individual addresses that of peers that misbehave (see Misbehaving() in

jonatack commented at 11:13 am on June 11, 2020:

b6a34484 nit: omit “that”

sipa commented at 5:25 am on June 18, 2020:

This is gone.

in src/banman.h:84 in 0778faa584 outdated

76@@ -68,6 +77,17 @@ class BanMan
77     CClientUIInterface* m_client_interface = nullptr;
78     CBanDB m_ban_db;
79     const int64_t m_default_ban_time;
80+
81+    /** Individual addresses that of peers that misbehave (see Misbehaving() in
82+     *  net_processing) are not banned but discouraged: we still allow incoming
83+     *  connections from them, but prefer them for eviction. Similar to banned
84+     *  IPs, we never make outgoing connections to them, and do not rumour them

jonatack commented at 11:14 am on June 11, 2020:

b6a34484 For my understanding, “rumour” == gossip?

sipa commented at 5:25 am on June 18, 2020:

This is gone.

in src/rpc/net.cpp:674 in 0778faa584 outdated

671@@ -671,7 +672,6 @@ static UniValue listbanned(const JSONRPCRequest& request)
672         rec.pushKV("address", entry.first.ToString());
673         rec.pushKV("banned_until", banEntry.nBanUntil);
674         rec.pushKV("ban_created", banEntry.nCreateTime);
675-        rec.pushKV("ban_reason", banEntry.banReasonToString());

jonatack commented at 11:26 am on June 11, 2020:

b6a34484 Should also remove ban_reason from the help at line 650, I believe, and update the release notes that this has been removed from listbanned.

sipa commented at 5:25 am on June 18, 2020:

Done.

MarcoFalke commented at 8:46 pm on June 18, 2020:

Have the release notes been updated with this breaking RPC change?

sipa commented at 11:11 pm on June 19, 2020:

Fixed.

jonatack commented at 11:30 am on June 11, 2020: member

Reviewing each commit separately.

ACK commit b6a34484 modulo removing {RPCResult::Type::STR, "ban_reason", ""}, from src/rpc/net.cpp::L650

in src/net.cpp:1017 in 0778faa584 outdated

1009@@ -1010,11 +1010,12 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
1010     // on all platforms.  Set it again here just to be sure.
1011     SetSocketNoDelay(hSocket);
1012 
1013-    int bannedlevel = m_banman ? m_banman->IsBannedLevel(addr) : 0;
1014+    bool discouraged = m_banman && m_banman->IsDiscouraged(addr);
1015+    bool banned = m_banman && m_banman->IsBanned(addr);
1016 
1017     // Don't accept connections from banned peers, but if our inbound slots aren't almost full, accept
1018     // if the only banning reason was an automatic misbehavior ban.

jonatack commented at 12:20 pm on June 11, 2020:

0778faa perhaps update the code doc

0-    // Don't accept connections from banned peers, but if our inbound slots aren't almost full, accept
1-    // if the only banning reason was an automatic misbehavior ban.
2+    // Don't accept connections from banned peers, nor from discouraged peers if
3+    // our inbound slots are almost full.

sipa commented at 5:26 am on June 18, 2020:

This has been split up, so I don’t think this applies anymore.

jonatack commented at 1:03 pm on June 11, 2020: member

Nice cleanup in 0778faa584871010c82db84c9d54, a few comments follow.

in doc/release-notes-19219.md:16 in 0778faa584 outdated

11+
12+- automatic bans will no longer be returned by the `listbanned` RPC.
13+
14+- automatic bans can no longer be lifted with the `setban remove` RPC command.
15+  If you need to remove an automatic ban, you can clear all bans (including
16+  manual bans) with the `clearbanned` RPC, or stop-start to clear automatic bans.

luke-jr commented at 8:27 pm on June 11, 2020:

I think it would be useful to have a way to clear only the misbehaving filter, but perhaps that is best left for a followup PR.

sipa commented at 5:26 am on June 18, 2020:

Agree.

jnewbery commented at 9:26 pm on June 11, 2020: member

There are quite a few places where comments/names/logs are now out of date. Commit here: https://github.com/jnewbery/bitcoin/tree/pr19219.2 gets most of them I think.

deadalnix referenced this in commit c5c01c8243 on Jun 11, 2020

sipa force-pushed on Jun 18, 2020

sipa commented at 5:28 am on June 18, 2020: member

@jnewbery I’ve incorporated most of the changes from your branch (split out over both commits).

I dropped the PF_NOBAN -> PF_NODISCOURAGE change, because that capability also protects against bans.

in src/addrdb.h:42 in a46433be54 outdated

35@@ -44,31 +36,17 @@ class CBanEntry
36         nCreateTime = nCreateTimeIn;
37     }
38 
39-    explicit CBanEntry(int64_t n_create_time_in, BanReason ban_reason_in) : CBanEntry(n_create_time_in)
40+    SERIALIZE_METHODS(CBanEntry, obj)
41     {
42-        banReason = ban_reason_in;
43+        uint8_t ban_reason = 2; //! For backward compatibility
44+        READWRITE(obj.nVersion, obj.nCreateTime, obj.nBanUntil, ban_reason);

jnewbery commented at 6:02 pm on June 18, 2020:

Note for reviewers: this means that if any peers are discouraged at the time that the node was last shut down pre-upgrade, they’ll be loaded as banned post-upgrade. They’ll clear automatically after 24 hours, so I think this is ok.

in src/banman.h:62 in a46433be54 outdated

73 public:
74     ~BanMan();
75     BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time);
76-    void Ban(const CNetAddr& net_addr, const BanReason& ban_reason, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
77-    void Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
78+    void Ban(const CNetAddr& net_addr, int64_t ban_time_offset = 0, bool since_unix_epoch = false);

jnewbery commented at 6:07 pm on June 18, 2020:

It should be very straightforward to unify the Ban() public functions and IsBanned() public functions so that the caller always passes in a (possibly /32) subnet. That would actually make the calling code in setban simpler.

This is out of scope for this PR, but could be done in a follow-up.

in src/net.cpp:1014 in a46433be54 outdated

1009@@ -1010,17 +1010,25 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
1010     // on all platforms.  Set it again here just to be sure.
1011     SetSocketNoDelay(hSocket);
1012 
1013-    int bannedlevel = m_banman ? m_banman->IsBannedLevel(addr) : 0;
1014+    bool banned = m_banman->IsBanned(addr);
1015+    bool discouraged = banned || m_banman->IsDiscouraged(addr);

jnewbery commented at 6:10 pm on June 18, 2020:

or’ing with banned is confusing here. After this PR, banning and discouraging are totally separated concepts, but this code signals that discouraging is a superset of banning.

sipa commented at 7:17 pm on June 18, 2020:

I was trying to avoid a call to IsDiscouraged if the peer is already banned. I moved it to accomplish something similar. I think it’s clearer now.

in src/net.cpp:2057 in a46433be54 outdated

2053@@ -2046,7 +2054,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
2054     }
2055     if (!pszDest) {
2056         if (IsLocal(addrConnect) ||
2057-            FindNode(static_cast<CNetAddr>(addrConnect)) || (m_banman && m_banman->IsBanned(addrConnect)) ||
2058+            FindNode(static_cast<CNetAddr>(addrConnect)) || (m_banman && (m_banman->IsDiscouraged(addrConnect) || m_banman->IsBanned(addrConnect))) ||

jnewbery commented at 6:16 pm on June 18, 2020:

This would be easier to read if (m_banman .... ) conditional was on its own line, so it’s easier to see that the || within that conditional is not at the top level.

sipa commented at 7:17 pm on June 18, 2020:

Done.

in src/net_processing.cpp:3584 in a46433be54 outdated

3580+            // Disconnect but don't discourage this local node
3581+            LogPrintf("Warning: disconnecting but not discouraging local peer %s!\n", pnode.addr.ToString());
3582             pnode.fDisconnect = true;
3583         } else {
3584-            // Disconnect and ban all nodes sharing the address
3585+            // Disconnect and discourage all nodes sharing the address

jnewbery commented at 6:20 pm on June 18, 2020:

You added logging here in the first commit and then (accidentally, I think) removed it in the second.

sipa commented at 7:18 pm on June 18, 2020:

Fixed.

in src/banman.h:36 in a46433be54 outdated

45+// it and never create outgoing connections to it. We won't gossip its address
46+// to other peers in addr messages. Banned addresses and subets are stored to
47+// banlist.dat on shutdown and reloaded on startup. Banning can be used to
48+// prevent connections with spy nodes or other griefers.
49+//
50+// 2. Discouragement. If a peer misbehaves enough (see Misbehaving()) in

jnewbery commented at 6:31 pm on June 18, 2020:

nit: the parens are in the wrong place - this should say “(see Misbehaving() in net_processing.cpp)

Sorry, this is my fault!

sipa commented at 7:17 pm on June 18, 2020:

Fixed.

in src/rpc/net.cpp:560 in a46433be54 outdated

555@@ -556,7 +556,8 @@ static UniValue getnetworkinfo(const JSONRPCRequest& request)
556 static UniValue setban(const JSONRPCRequest& request)
557 {
558     const RPCHelpMan help{"setban",
559-                "\nAttempts to add or remove an IP/Subnet from the banned list.\n",
560+                "\nAttempts to add or remove an IP/Subnet from the banned list.\n"
561+                "Peers that are automatically banned cannot be unbanned.\n",

jnewbery commented at 6:32 pm on June 18, 2020:

This line should be removed (sorry - this was from my branch when I was referring to automatic banning)

sipa commented at 7:18 pm on June 18, 2020:

Done.

jnewbery commented at 6:33 pm on June 18, 2020: member

A few small style suggestions, otherwise looks good.

sipa force-pushed on Jun 18, 2020

jnewbery commented at 8:00 pm on June 18, 2020: member

utACK 250e8acc7df63430f73099549d18d707844c08a8

sipa force-pushed on Jun 18, 2020

jnewbery commented at 8:46 pm on June 18, 2020: member

utACK 0ba85217caa333021cf04eb5cff9770f3152c287

in src/init.cpp:431 in 0ba85217ca outdated

426@@ -427,8 +427,8 @@ void SetupServerArgs(NodeContext& node)
427 
428     gArgs.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);
429     gArgs.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);
430-    gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
431-    gArgs.AddArg("-bantime=<n>", strprintf("Number of seconds to keep misbehaving peers from reconnecting (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
432+    gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting and discouraging misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
433+    gArgs.AddArg("-bantime=<n>", strprintf("Default bantime for manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

jonatack commented at 3:30 pm on June 19, 2020:

36db81e Thoughts:

-bantime help would now print

0-bantime=<n>
1       Default bantime for manually configured bans (default: 86400)

“Default”: only the case if this config arg is not passed
It would be helpful to continue providing the units (seconds)

Perhaps "Bantime in seconds for manually configured bans (default: %u)"

sipa commented at 11:12 pm on June 19, 2020:

I don’t understand what your comment on the use of “default” is about. It can be overridden by providing an RPC argument.

I’ve added seconds back.

jonatack commented at 6:51 am on June 20, 2020:

Thanks. The two uses of “default” with different meanings in the same sentence (default for manual bans and default for this setting, e.g. the default for the default) seemed confusing to me. That said, reading it with “(in seconds)” now separating the two seems better.

in src/banman.h:32 in 0ba85217ca outdated

41+// Banman manages two related but distinct concepts:
42+//
43+// 1. Banning. This is configured manually by the user, through the setban RPC.
44+// If an address or subnet is banned, we never accept incoming connections from
45+// it and never create outgoing connections to it. We won't gossip its address
46+// to other peers in addr messages. Banned addresses and subets are stored to

jonatack commented at 3:33 pm on June 19, 2020:

36db81e nit: s/subets/subnets/

sipa commented at 11:12 pm on June 19, 2020:

Fixed.

in doc/release-notes-19219.md:9 in 0ba85217ca outdated

0@@ -0,0 +1,20 @@
1+#### Change in banning behaviour
2+
3+Peers that misbehave (e.g. send us invalid blocks) are now referred to as
4+discouraged nodes in log output, as they're not (and weren't) strictly banned:
5+incoming connections are still allowed from them, but they're preferred for
6+eviction. There are some small changes to our treatment of discouraged
7+addresses:
8+
9+- Discouraging an address does not time out automatically after 24 hours.

jonatack commented at 3:43 pm on June 19, 2020:

perhaps: “after 24 hours or a user’s -bantime setting.”

sipa commented at 11:13 pm on June 19, 2020:

Done.

in doc/release-notes-19219.md:1 in 0ba85217ca outdated

0@@ -0,0 +1,20 @@
1+#### Change in banning behaviour

jonatack commented at 3:45 pm on June 19, 2020:

36db81e The release note looks great.

One thought: we’re talking about discouraging misbehavior rather than banning. I wonder if a more accurate title is possible here, e.g. “Changes regarding misbehaving peers” or “Discouraging misbehaving peers.”

sipa commented at 11:13 pm on June 19, 2020:

Done.

in src/init.cpp:433 in 0ba85217ca outdated

426@@ -427,8 +427,8 @@ void SetupServerArgs(NodeContext& node)
427 
428     gArgs.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);
429     gArgs.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);
430-    gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
431-    gArgs.AddArg("-bantime=<n>", strprintf("Number of seconds to keep misbehaving peers from reconnecting (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
432+    gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting and discouraging misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

jonatack commented at 3:48 pm on June 19, 2020:

36db81e if this change is long-lasting, the -banscore config arg and getpeerinfo field name may need to be gradually renamed to avoid confusion with banning and -bantime (the log output is already being changed in this commit from “BAN” to “DISCOURAGE”)

sipa commented at 11:13 pm on June 19, 2020:

I think banscore should die a swift death (but not in this PR).

amitiuttarwar commented at 1:43 am on June 27, 2020:

whats the process for changing config args? do we go through a deprecation cycle? (and is this documented anywhere? I couldn’t find it) @sipa when you say die a swift death do you mean more than rename?

in src/net.cpp:1024 in 0ba85217ca outdated

1024         return;
1025     }
1026 
1027+    // Only accept connections from discouraged peers if our inbound slots aren't (almost) full.
1028+    bool discouraged = m_banman->IsDiscouraged(addr);
1029+    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && nInbound + 1 >= nMaxInbound && discouraged)

jonatack commented at 6:07 pm on June 19, 2020:

The conditional for determining if the inbounds are nearly full, per previous review #19219 (review), now seems correct.

sipa commented at 11:13 pm on June 19, 2020:

Ok.

jonatack commented at 6:11 pm on June 19, 2020: member

ACK 0ba85217caa333021cf04eb5cff9770f3152c287 code review, verified builds, tests, ran updated fuzzer (src/test/fuzz/addrdb ../qa-assets/fuzz_seed_corpus/). Am running bitcoind on mainnet with this branch and banscore at low thresholds (1, 0), grepping getpeerinfo banscores (bitcoin-cli getpeerinfo | grep 'banscore') and the net debug log (grep -ni "bann\|discour" ~/.bitcoin/debug.log), but the banscores have remained at 0 so far and no discouragement has been logged. As a sanity check did some manual setbans, checked listbanned and the log, watched the bans expire.

Some suggestions if you retouch or as a follow-up.

in src/net.cpp:1054 in 0ba85217ca outdated

1050@@ -1044,7 +1051,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
1051     pnode->m_permissionFlags = permissionFlags;
1052     // If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility)
1053     pnode->m_legacyWhitelisted = legacyWhitelisted;
1054-    pnode->m_prefer_evict = bannedlevel > 0;
1055+    pnode->m_prefer_evict = banned || discouraged;

cculianu commented at 10:52 pm on June 19, 2020:

What if they are PF_NOBAN yet these flags happen to be set.. in that case m_prefer_evict will be set… even though they are PF_NOBAN.. is that acceptable and/or intentional?

sipa commented at 11:14 pm on June 19, 2020:

PF_NOBAN peers are already except from eviction entirely, so the value of the m_prefer_evict field has no meaning for them.

The code here is just maintaining the existing semantics (current master also sets m_prefer_evict for (auto)banned peers with PF_NOBAN).

sipa force-pushed on Jun 19, 2020

jnewbery commented at 1:54 am on June 20, 2020: member

utACK 2605bdd220b118fe3cac6a9cebb673cf651e273c

jonatack commented at 7:10 am on June 20, 2020: member

Code review re-ACK 2605bdd per git diff 0ba8521 2605bdd changes since last review are documentation and help improvements and a code change to clarify that only discouragement affects the bool CNode::m_prefer_eviction value for peers, as we don’t accept connections from banned peers.

in src/banman.cpp:149 in 2605bdd220 outdated

136@@ -151,8 +137,8 @@ void BanMan::Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ba
137     }
138     if (m_client_interface) m_client_interface->BannedListChanged();
139 
140-    //store banlist to disk immediately if user requested ban

narula commented at 10:53 pm on June 22, 2020:

nit: I think this change makes more sense in the previous commit, since that’s where you added the assert.

sipa commented at 8:06 pm on June 26, 2020:

Agreed. Will fix if there are more changes requested, but I don’t think this is worth breaking ACKs for.

sipa commented at 10:48 pm on July 1, 2020:

Done.

in src/banman.cpp:86 in 2605bdd220 outdated

91-    return level;
92+    return false;
93 }
94 
95-bool BanMan::IsBanned(CNetAddr net_addr)
96+bool BanMan::IsDiscouraged(const CNetAddr& net_addr)

amitiuttarwar commented at 9:40 pm on June 25, 2020:

nit: I’d prefer if this was moved up in the file so the two IsBanned functions could be next to each other

sipa commented at 10:48 pm on July 1, 2020:

Done.

in src/net_processing.cpp:3563 in 2605bdd220 outdated

3559@@ -3559,25 +3560,26 @@ bool ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRec
3560     return true;
3561 }
3562 
3563-bool PeerLogicValidation::CheckIfBanned(CNode& pnode)
3564+bool PeerLogicValidation::CheckIfShouldDiscourage(CNode& pnode)

amitiuttarwar commented at 1:23 pm on June 26, 2020:

I find this name misleading since this function doesn’t just check, it takes the critical actions (initiating add to discourage filter, peer disconnect). I don’t have any suggestions I love, but some ideas are ConsiderDiscourageAndDisconnect or MaybeDiscourageAndDisconnect, in line with naming conventions from other net processing functions

sipa commented at 10:48 pm on July 1, 2020:

Changed to MaybeDiscourageAndDisconnect.

in src/init.cpp:431 in 2605bdd220 outdated

426@@ -427,8 +427,8 @@ void SetupServerArgs(NodeContext& node)
427 
428     gArgs.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);
429     gArgs.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);
430-    gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
431-    gArgs.AddArg("-bantime=<n>", strprintf("Number of seconds to keep misbehaving peers from reconnecting (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
432+    gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting and discouraging misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
433+    gArgs.AddArg("-bantime=<n>", strprintf("Default bantime (in seconds) for manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

amitiuttarwar commented at 11:17 pm on June 26, 2020:

hm, I find the previous description of “bantime” (time to disallow reconnect) more helpful than repeating the word

consider: Default time (in seconds) to disallow manually configured bans from reconnecting

sipa commented at 10:48 pm on July 1, 2020:

Done.

in doc/release-notes-19219.md:7 in 2605bdd220 outdated

0@@ -0,0 +1,21 @@
1+#### Changes regarding misbehaving peers
2+
3+Peers that misbehave (e.g. send us invalid blocks) are now referred to as
4+discouraged nodes in log output, as they're not (and weren't) strictly banned:
5+incoming connections are still allowed from them, but they're preferred for
6+eviction. Discouraged addresses are treated slightly different than banned
7+ones:

amitiuttarwar commented at 0:51 am on June 27, 2020:

nice release notes. I have a suggestion for a small change that would help with logical clarity.

when I initially read these release notes, I didn’t understand if the discouraged address behaviors were (A) now different than banned, but functionally the same as before or (B) changes in functionality. After reading the code I understand that the first part is the same (logic for managing connections), and the bullet points are things that changed (managing the discouraged address list).

An attempt to make that clearer-

0eviction.
1
2Managing discouraged addresses is now handled slightly different than banned
3ones:

sipa commented at 10:49 pm on July 1, 2020:

Agree, the text didn’t make that clear. I’ve changed it to “Furthermore, a few additional changes to how discouraged addressed are treated are introduced:”. LMK what you think.

amitiuttarwar commented at 11:39 pm on July 1, 2020:

looks good

in src/banman.h:27 in 2605bdd220 outdated

36-// Dropping a node for sending stuff that is invalid
37-// now but might be valid in a later version is also
38-// dangerous, because it can cause a network split
39-// between nodes running old code and nodes running
40-// new code.
41+// Banman manages two related but distinct concepts:

amitiuttarwar commented at 1:05 am on June 27, 2020:

thanks for this awesome overview!

sipa commented at 10:50 pm on July 1, 2020:

Thank @jnewbery!

in src/net_processing.cpp:1022 in 2605bdd220 outdated

1018@@ -1019,7 +1019,7 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans)
1019 }
1020 
1021 /**
1022- * Mark a misbehaving peer to be banned depending upon the value of `-banscore`.
1023+ * Mark a misbehaving peer to be discouraged depending upon the value of `-banscore`.

amitiuttarwar commented at 1:49 am on June 27, 2020:

0Increment 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.

sipa commented at 10:50 pm on July 1, 2020:

Done.

amitiuttarwar commented at 2:49 am on June 27, 2020: contributor

high level review thoughts

strong concept / approach ACK. splitting out separate bloom filter from the (now manual) ban list makes sense conceptually and ofc is a robustness improvement.

over the last week, I have spent many hours reviewing this PR & learning about the surrounding mechanisms. from what I can tell, the functionality looks solid.

even after spending the week with this code, I still find the discourage language & logical event flow to be kind of confusing. we compare a misbehavior score to a ban score to figure out if the peer should be discouraged, which usually means “add to discouragement filter”, but in this case means “possibly disconnect and possibly add to discouragement filter” 😅

I think part of the problem is the verb “discourage” makes it seem like we are taking an explicit action towards that connection. I think language like “mark as discouraged” or “deprioritize” would better match the meaning of add-to-filter-that-we-later-check. even better would be making it cohesive with the misbehaving mechanisms (since they seem to exclusively apply to discouragement now).

all that said, I don’t want to hold up this PR. the benefit to this new functionality definitely outweighs a wonderful interface. my review comments suggest small changes in the language & documentation in attempt to make the functionality more apparent. if they seem reasonable, they can be incorporated here or can easily be done as followups. the only one I think should be done in this PR (if accepted), is renaming CheckIfShouldDiscourage to something that captures that the function initiates discouragement and disconnection.

curious to hear reactions. I’m happy to help move this PR forward however makes sense. Aside from☝️ I’m ready to leave a review ACK / If a rename made sense to people, I’d be happy to make the updates & offer the changes for you to pick up here / etc..

another review comment

could we rename getpeerinfo->banscore field (link) to misbehaviorscore? It seems to apply exclusively to discouragement. And/or update the description to something more helpful, like The programmatically calculated score for protocol violations. Can lead to the peer being disconnected and/or marked as discouraged.

follow ups

I agree with some of the review comments about possible followups and am collecting here for easy reference

clear misbehaving filter without restarting node (https://github.com/bitcoin/bitcoin/pull/19219#discussion_r439052719)
consolidate Ban() and IsBanned() functions (link)
remove -banscore config arg (1 2)
remove getpeerinfo->banscore field (this comment, link)

sipa force-pushed on Jul 1, 2020

sipa commented at 10:53 pm on July 1, 2020: member

@amitiuttarwar Thanks for the review. I’ve addresses all of your comments, I think. As for your overall suggestion around “discouraging” as a term, do you mean that primarily in RPC/log interface, or in code symbol names/comments? If there are small changes you see that can improve the interface it may be worth including them here, as they may become harder to change later.

I don’t feel like touching the banscore option and RPC field, as I think they should go away. For two reasons:

It’s a configuration option, but there is no advice we know to give (I think?) about when anyone should ever change it. Such configuration options are useless.
The concept of scores for this just complicates reasoning. Either a particular behavior is worthy of discouragement, and it should happen immediately, or it isn’t, and we shouldn’t have scores to track if it happens frequently or not. If the software is broken, it’ll do it repeatedly, and we’ll eventually discourage them anyway. Independently it makes sense to keep track of resource usage we do on behalf of a peer (CPU, memory, network bandwidth) and if resources are constrained, we should disconnect the peers that demand the most of us (possibly with some exceptions like keeping the peers that gave us the latest blocks) - but that feels like a very different aspect of peer management than misbehavior/discouragement currently is.

Besides that, I’d like to get this merged.

sipa commented at 10:57 pm on July 1, 2020: member

Oh, vaguely related: @gmaxwell and I working on designing/implementing a rolling cuckoo filter (see this paper), which would be a drop-in replacement for the rolling bloom filters we have, but around 3x smaller. However, they also support deletion, which may be useful if we’d ever want a set of RPCs to actually manage discouragement (separate from the ban management RPCs).

jnewbery commented at 11:48 pm on July 1, 2020: member

I don’t feel like touching the banscore option and RPC field, as I think they should go away…

Totally agree on these. It seems to me that historically there are two reasons that configuration options get added:

Different users require different functionality from the system, and the configuration controls those different functionalities.
All (or virtually all) users require the same functionality, but the developer has punted (or hedged - pick how charitable you want to be) on the right parameters for the system.

-banscore certainly falls under the second category. As @sipa points out, we have no advice for users on when to change it, and it’s not clear how you’d change it to achieve different desired outcomes.

We should aim to eliminate all of the second category, while being careful that there aren’t genuine use cases for those config options.

amitiuttarwar commented at 2:08 am on July 2, 2020: contributor

As for your overall suggestion around “discouraging” as a term, do you mean that primarily in RPC/log interface, or in code symbol names/comments? If there are small changes you see that can improve the interface it may be worth including them here, as they may become harder to change later.

Was referring to code internals too, so symbols/comments. I proposed my identified small changes to help with clarity (thanks for updates!) and agree moving forward with merge makes more sense than more rounds of changes and review.

Also agree with reasoning & removing banscore config & RPC. Updated my comment aggregating follow ups to reflect.

From my phone 😛 changes look good. I’m off the grid till Monday, but I’ll re-review ack then if this is still open.

in src/init.cpp:434 in 1f7e0cab7a outdated

429@@ -430,8 +430,8 @@ void SetupServerArgs(NodeContext& node)
430 
431     gArgs.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);
432     gArgs.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);
433-    gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
434-    gArgs.AddArg("-bantime=<n>", strprintf("Number of seconds to keep misbehaving peers from reconnecting (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
435+    gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting and discouraging misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
436+    gArgs.AddArg("-bantime=<n>", strprintf("Default time (in seconds) to disallow manually configured bans from reconnecting (default: %lu)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

jnewbery commented at 1:11 am on July 3, 2020:

why change this to %lu? DEFAULT_MISBEHAVING_BANTIME isn’t a long

jonatack commented at 4:27 am on July 3, 2020:

True, though the printed value is unchanged.

Proposed simplification if you retouch:

current

0       Default time (in seconds) to disallow manually configured bans from
1       reconnecting (default: 86400)

proposed

0       Default duration (in seconds) of manually configured bans (default: 86400)

sipa commented at 3:12 am on July 4, 2020:

Fixed. I suspect the %lu was just muscle memory…

jnewbery commented at 1:11 am on July 3, 2020: member

Most of the recent changes look fine. One question about a string format specifier change.

in doc/release-notes-19219.md:8 in 1f7e0cab7a outdated

0@@ -0,0 +1,23 @@
1+#### Changes regarding misbehaving peers
2+
3+Peers that misbehave (e.g. send us invalid blocks) are now referred to as
4+discouraged nodes in log output, as they're not (and weren't) strictly banned:
5+incoming connections are still allowed from them, but they're preferred for
6+eviction.
7+
8+Furthermore, a few additional changes to how discouraged addressed are

jonatack commented at 3:47 am on July 3, 2020:

s/addressed/addresses/

Suggestion:

“Furthermore, a few additional changes are introduced to how discouraged addresses are treated:”

sipa commented at 3:12 am on July 4, 2020:

Done.

jonatack commented at 4:17 am on July 3, 2020: member

re-ACK 1f7e0ca per git range-diff daae8b8 2605bdd 1f7e0ca changes since last review: (a) edits to release note, bantime help and doxygen doc, (b) a function renaming, and (c) passing CNetAddr net_addr to IsBanned() by const reference instead of by value.

Would be good to merge this soon for 0.20 backport in #19224.

sipa force-pushed on Jul 4, 2020

Replace automatic bans with discouragement filter

This patch improves performance and resource usage around IP
addresses that are banned for misbehavior. They're already not
actually banned, as connections from them are still allowed,
but they are preferred for eviction if the inbound connection
slots are full.

Stop treating these like manually banned IP ranges, and instead
just keep them in a rolling Bloom filter of misbehaving nodes,
which isn't persisted to disk or exposed through the ban
framework. The effect remains the same: preferred for eviction,
avoided for outgoing connections, and not relayed to other peers.

Also change the name of this mechanism to better reflect reality;
they're not banned, just discouraged.

Contains release notes and several interface improvements by
John Newbery.

b691f2df5f

Clean up separated ban/discourage interface 2ad58381ff

sipa force-pushed on Jul 4, 2020

jonatack commented at 6:50 am on July 4, 2020: member

ACK 2ad5838 per changes since last review git range-diff 3276c14 1f7e0ca 2ad5838

in src/net.cpp:1014 in 2ad58381ff

1014-
1015-    // Don't accept connections from banned peers, but if our inbound slots aren't almost full, accept
1016-    // if the only banning reason was an automatic misbehavior ban.
1017-    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && bannedlevel > ((nInbound + 1 < nMaxInbound) ? 1 : 0))
1018+    // Don't accept connections from banned peers.
1019+    bool banned = m_banman->IsBanned(addr);

jnewbery commented at 8:32 pm on July 6, 2020:

This appears to be slightly less defensive since it’s not checking that m_banman has been assigned before dereferencing it. It’s fine because m_banman is always set in CConnman::Init() from the value in connOptions.m_banman (which is itself always set in AppInitMain().

sipa commented at 0:59 am on July 7, 2020:

Let’s fix that later.

jnewbery commented at 8:39 pm on July 6, 2020: member

Code review ACK 2ad58381fffb33d611abf900b73d9e6b5a4e35f8

naumenkogs commented at 11:26 am on July 7, 2020: member

utACK 2ad58381fffb33d611abf900b73d9e6b5a4e35f8

amitiuttarwar commented at 5:44 pm on July 7, 2020: contributor

code review ACK 2ad58381ff

sipa merged this on Jul 7, 2020

sipa closed this on Jul 7, 2020

fanquake referenced this in commit 0477348057 on Jul 8, 2020

fanquake referenced this in commit 2b79ac7406 on Jul 8, 2020

fanquake commented at 1:47 am on July 8, 2020: member

I’ve added this for backport in #19224.

fanquake removed the label Needs backport (0.20) on Jul 8, 2020

sidhujag referenced this in commit aec880260b on Jul 8, 2020

fanquake referenced this in commit 01c563708f on Jul 10, 2020

fanquake referenced this in commit a4eb6a51a7 on Jul 10, 2020

sidhujag referenced this in commit 1b2b3e3600 on Jul 10, 2020

MarcoFalke referenced this in commit b93c4244b9 on Jul 14, 2020

sidhujag referenced this in commit b8fc13afe4 on Jul 14, 2020

deadalnix referenced this in commit 3c30539ab6 on Aug 7, 2020

ftrader referenced this in commit a9e47e7970 on Aug 17, 2020

backpacker69 referenced this in commit 7a025fc7af on Sep 8, 2020

backpacker69 referenced this in commit 43d9b5ac88 on Sep 8, 2020

Bushstar referenced this in commit af28aa5b0e on Oct 21, 2020

Bushstar referenced this in commit 35575d2103 on Oct 21, 2020

Platinumwrist referenced this in commit 05e2740d92 on Oct 25, 2020

deadalnix referenced this in commit a40a553f04 on Jan 6, 2021

deadalnix referenced this in commit 91502b3998 on Jan 6, 2021

Fabcien referenced this in commit 6176f87a9d on Jan 6, 2021

PastaPastaPasta referenced this in commit 50cd7855c0 on Jan 16, 2021

UdjinM6 referenced this in commit c1b6fea385 on Jan 18, 2021

PastaPastaPasta referenced this in commit a9a9c3d865 on Jan 18, 2021

gades referenced this in commit 5f1c7e1fe6 on Mar 24, 2022

kittywhiskers referenced this in commit c070c3607b on May 7, 2022

kittywhiskers referenced this in commit 921677f95f on May 7, 2022

kittywhiskers referenced this in commit 7c8d4647c6 on May 7, 2022

kittywhiskers referenced this in commit 2567257861 on May 7, 2022

kittywhiskers referenced this in commit a91d3ee1c9 on May 13, 2022

kittywhiskers referenced this in commit 4831a5ebd4 on May 17, 2022

kittywhiskers referenced this in commit 1253c7e9df on May 17, 2022

kittywhiskers referenced this in commit d0c5b8e1fc on Jun 10, 2022

kittywhiskers referenced this in commit b9d0c73f39 on Jun 10, 2022

kittywhiskers referenced this in commit e8674a5595 on Jun 11, 2022

kittywhiskers referenced this in commit 57575e566e on Jun 11, 2022

kittywhiskers referenced this in commit dda312937d on Jun 12, 2022

kittywhiskers referenced this in commit b675ac0467 on Jun 12, 2022

kittywhiskers referenced this in commit c078a6256e on Jun 12, 2022

kittywhiskers referenced this in commit faf188fbcc on Jun 12, 2022

kittywhiskers referenced this in commit fc2690a855 on Jun 12, 2022

kittywhiskers referenced this in commit 03b8bc887c on Jun 13, 2022

kittywhiskers referenced this in commit b776787890 on Jun 14, 2022

kittywhiskers referenced this in commit b2a0fe9378 on Jun 18, 2022

kittywhiskers referenced this in commit e83a45bf98 on Jun 21, 2022

kittywhiskers referenced this in commit 6bec143af0 on Jun 21, 2022

kittywhiskers referenced this in commit a7ee2c2667 on Jun 21, 2022

PastaPastaPasta referenced this in commit c3f5f81a44 on Jun 23, 2022

PastaPastaPasta referenced this in commit 87b6d1c958 on Jul 3, 2022

DrahtBot locked this on Aug 16, 2022

Replace automatic bans with discouragement filter #19219

Conflicts

high level review thoughts

another review comment

follow ups