Nice feature. RE -storebanlist, the system should default to storing. Users may disable.

Or don't create an option at all -- same as CAddrDB, user may delete file to clear the list.

I'd also vote for treating this the same as peers.dat and don't add an option for it.

@jgarzik: RE -storebanlist, the system should default to storing. Users may disable.

This seems risky in my opinion, because not all IPs are static, and they don't necessarily always belong to the same node.

@jgarziks way of resetting the banlist (by removing banlist.dat) is very straightforward and doesn't need any explanation. I'd like to avoid another cmd arg option to not end up in having thousands of tiny options.

Node misbehaving bans are by default 24h. All other bans needs conscious actions from the users.
The chance that your node accidentally get an IP out of a dynamic range, where the previous owner managed to add his bitcoin-core on a ban-list, is very rar. And then, the chance that all nodes did ban this IP or that you connect to a node which conscious added your new IP to the banlist is even rarer
We have 3,706,452,992 assignable public IPv4 addresses.

Concept ACK, also agree that defaulting to store bans is perfectly fine (so is not offering an option).

As any kind of persistence commits to a format it is good to think forward a bit what should be included: would it make sense to add e.g. a "ban source" enum field, that specifies whether the ban is automatic or manual?

I like the idea of storing the "ban source". Because we now keep/dump a serialized std::map<CSubNet, int64_t> everything beyond that would be a bigger change. We should probably introduce a simple class for the banlist entry like class CBanEntry { int64_t bantime, enum banSource }. This might also be capable of allowing ban distinction between multiple bans on the same IP but with different ports (but would mean moving from std::map to a vector).

We should probably introduce a simple class for the banlist entry like class CBanEntry { int64_t bantime, enum banSource }

Right. That would also allow for versioning in serialization.

This might also be capable of allowing ban distinction between multiple bans on the same IP but with different ports

This might be overdoing it; at least I don't see much added value in per-port bans, at the least they're easily circumvented (for incoming connections: just reconnect and you get a different source port, for outgoing connections simply rebind to a different port).

@laanwj Think about per port bans with incoming Tor/proxy connections, they're all 127.0.0.1 and when I currently ban a single 127.0.0.1 with a specific port all further connection attempts are blocked until bantime is over. Not sure if this is to solve in any way.

@Diapolo That simply won't work. As I try to explain above, source ports aren't unique nor identifying.

@laanwj Indeed, my fault... but should be warned when banning 127.0.0.1 or do you assume people know what they are banning?

Followed and implemented @laanwj s proposal.
Added CBanEntry as ban metadata container which uses enum BanReason (BanReasonNodeMisbehaving, BanReasonManuallyAdded). Also added nCreateTime within the new metadata class, to allow to backtrack when a ban was added/created.
Using now everywhere the typedef std::map<CSubNet, CBanEntry> banmap_t.

Now the banlist.dat file is extendable over the possibility to detect different versions (CBanEntry->nVersion) of entries and allow possible migrations during deserialization.

utACK

I have an unsubmitted patch that also adds "remembering" of ban scores associated by IP address across disconnects over a time window (24h I think). That is, it prevents malicious nodes from carrying out sawtooth attacks where they can reset their ban score by disconnecting and reconnecting before being banned. There are pros (prevents malicious behavior) and cons (tightens up the rules and increases the likelihood of NAT exit hosts getting banned due to lots of small accumulations of ban points over time.) Is there any interest in this? I can open a new pull if so...

@ajweiss Not sure about that. At least make sure that the associated structure is size-limited. Especially with IPv6 it will be extremely easy to fill memory that way. But this needs to be discussed somewhere else as it is not related to this pull.