Replace automatic bans with discouragement filter

[sipa]

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.

[luke-jr]

Concept ACK

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

[jonatack]

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

Light code review ACK 4c4df6b. 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.

[jonatack]

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

[maflcko]

review ACK 4c4df6b 🚆

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 4c4df6bf61104a4946479fccc7885dc0fe05dbd8 🚆
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiFHgv/X2KHSRqmN8x9c5zLH7/3ZqkSkcMGsr1wMBS3GXDqAYv14lYl2xAG4iou
TAkeccq8MLDpARLeTtjVou1NhjFcEMZXciV55nF4y7GD8DianWdAeH0/yTlGfjdk
yuYGZJpQlHipSccFgOY+pf5pTS9Ph1XgHEabfU2mwGsMRHeo5wIGulBGHTH3oGPZ
r8aTkNv2o0hQ7Gm68g5k8+cYCcnPYTy8CdO/3CTJMjzY7fgg9ig/CuOWWAq3Pka1
IcxspupCNtwYKui+rFMXpLar78b1ABTLKGRX2iHBLs09bdOMZgO5jDDCooWx7YAV
ogNVxmb93p7kRiVtZschxRCnGDBnn6TGJKINNxKwfuwu8BPHilHOwWXmiKUF7EKh
MwVsVQGYrGz1+uSst4FYhwEXewC6o72kC/7t58mKy2hipyFhMhjQC/CW8v0M9Xsu
K4CIviPL/AphwRDhTff6nk1uot9VJHaC/JXxaJmnaXiYkckdlu9D2VaKe5n5dm58
gy6VugHp
=eVeU
-----END PGP SIGNATURE-----

Timestamp of file with hash efbc9feace2c4d03c3d87e850e69600a2022027bb9fda48388d15e2d99cfe379 -

[laanwj]

Code review ACK 4c4df6b

[sipa]

@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.

[jnewbery]

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]

@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]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

Concept ACK.

[pstratem]

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

[cculianu]

You really should add a benchmark for this..

[cculianu]

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

[sipa]

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]

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]

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

[jnewbery]

@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]

Indeed, thanks for the explanation.

[cculianu]

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]

@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]

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]

@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.