Code review ACK 4c4df6b

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

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.

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

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.

Concept ACK.

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

You really should add a benchmark for this..

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

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.

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

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

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

Indeed, thanks for the explanation.

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.

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

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.

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