Because bans have specific durations I'm not sure if this is a good change. Should a 1 hour ban of foo/16 stop you from adding a 1 year ban of foo/24?

Right now I already have to undo every ban before adding it to make sure it's not rejected and as a result ends up expiring prematurely. But if any wider banned blocked every narrower ban, I'm not sure how I could programmatically handle that. I'd have to have crazy logic interrogated the banlist and broke every larger ban into smaller ones when a smaller one with a different time period was added.

Instead, I think it should just be consistent in that each ban doesn't interact with an different ban. And when I add a ban for an already existing prefix, it should extend the ban if the new one has a later expiration time. It would be fine to canonicalize actually equivalent bans (e.g. 1.2.3.4/24 = 1.2.3.0/24).

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

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26841 (tests: Allow tests to use a loopback address other than 127.0.0.1 for more test runner parallelism by achow101)
• #26822 (p2p, rpc: don't allow past absolute timestamp in setban by brunoerg)
• #26261 (p2p: cleanup LookupIntern, Lookup and LookupHost by brunoerg)
• #26078 (p2p: return CSubNet in LookupSubNet by brunoerg)
• #24097 (Replace RecursiveMutex m_cs_banned with Mutex, and rename it by hebasto)

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.

I'm not sure how I could programmatically handle that. I'd have to have crazy logic interrogated the banlist and broke every larger ban into smaller ones when a smaller one with a different time period was added.

Totally fair. I can't see either how clients will be able to make tooling with sane logic. I am going to think this through and update the PR with a better solution. Thanks for the feedback and a potential solution, @gmaxwell !

Concept ACK on making this consistent. I'll review once you have an updated proposal that addresses gmaxwell's input.

Potential solution

Let (s0, t0) be an existing ban, where s0 is a subnet and t0 is the ban expiration time. The user is now trying to add another ban (s1, t1).

Case s1 ⊂ s0:
If t1 <= t0: return success because the end goal is accomplished - this will minimize client error handling
If t1 > t0: insert (s1, t1) into BanMan

Case s1 ⊇ s0:
If t1 <= t0: insert (s1, t1) into BanMan
If t1 > t0: remove (s0, t0), add (s1, t1) into BanMan

Case no overlap:
insert (s1, t1) into BanMan

Unban:
We will treat unbanning consistent with the semantics of the rpc call: setban ip remove i.e. it is the removal of a ban entry, not the addition of an unban entry. Effectively, unbanning will look for exact matches of ban entries to remove.

Thoughts, @gmaxwell, @jnewbery ?

I think that approach makes sense; it's equivalent to treating every ban as a per-IP thing, and remembering the longest-duration ban for each.

One question I have is how to make clearbanned consistent with that? This is a bit confusing:

• ban 1.2.3.0/24 for 1h
• ban 1.2.0.0/16 for 2h -> overwrites the previous ban
• unban 1.2.3.0/24 -> failure because no such ban exists

And this:

• ban 1.2.3.0/24 for 1h
• ban 1.2.3.4/32 for 2h -> add a longer ban for smaller range
• unban 1.2.3.0/24 -> works, but leaves 1.2.3.4/32 banned

While:

• ban 1.2.3.0/24 for 2h
• ban 1.2.3.4/32 for 1h -> doesn't do anything
• unban 1.2.3.0/24 -> works, and doesn't leave 1.2.3.4/32 banned

I think the most reasonable thing requiring an exact match for some ban record when unbanning, but also make it delete all (longer) subranges?

I don't really like the banning add removal doing fancy logic. This will make it extremely hard to have external software add bans, even with the newly proposed behaviour. It's also unlike access-lists on any networking device I've ever worked with.

What is the downside of the obvious behaviour? There is a list of bans, some of which are shorter-than-maximum-CIDR, add adds to the list, clear removes from it. If you have overlapping bans, ... so? You might expect them to have different durations, or they may come from different sources.

The only limitation I'm aware with that approach is that if you want to remove a ban affecting a specific host or subnet more care might be required to remove all... but that could be addressed with a "removeall" e.g.

setban 1.2/16 add
setban 1.2.3/24 add
setban 1.2.3.5/32 remove #does nothing
setban 1.2.3.5/32 removeall  #removes the 1.2/16 and the 1.2.3/24 ban

Then you can get exact management of the ban list via add/remove, but also get a "just make this host work" removal if that's what you want. This also doesn't make the resulting banlist depend on the order that you did the insertions, which I believe the logic proposed a few messages up would do (e.g. if you insert less specific then more specific you won't get the same result as more than less (as their times won't match, so in one case you'll just get the less specific and the other case you'll get both).

I also have a preference for not returning errors when the action the user wants is accomplished, I've had more than a few people using my ban list complain about the wall of errors when they update... though assuming it didn't return an error when it extended the duration, I think that issue would already be eliminated by the changes we've discussed.

I agree with the idea that we should interpret setban ip/mask add time as "add a ban entry for this address range", and not as "add a ban entry on each single address in the range". This way of thinking allows for overlapping bans.

I also agree that unintended side effects like setban 127.0.0.1/32 remove removing the ban entry for 127.0.0.1/24 is not good as it means client scripts need to maintain banlist state and execute logic prior to, and after, unbanning.

Further agree, on not returning an error when the user intent is accomplished (by a pre-existing ban entry) even if it does not create a new ban entry - I will update the code to reflect that. (Updated the proposal above as well)

removeall is a good idea, perhaps better named force-remove and implemented in a follow-up PR? I am open to adding it in here if we believe we need it for atomic commits.

The one thing I am unsure about is not consolidating ban entries where possible. In my rookie opinion, BanEntry{127.0.0.1/32 200} should be replaced with {BanEntry 127.0.0.1/24 400} instead of just having an additional "dead" entry. Unnecessary ban entries make every call to BanMan::IsBanned a tiny bit more expensive, which makes opening new connections, accepting new connections, and PeerLogicValidation::ProcessMessage/addr more expensive. I am new here and don't yet have a good intuitive sense for the frequency of those invocations but it seems like an expense to avoid?

After a couple attempts, I am not able to see how this proposal will mean the banlist will depend on the order of insertions so long as banUntil times are consistent. I feel sure I'm missing something. Could you please give an example, @gmaxwell ?

The most common usage is a relative time, so the times won't be consistent. :)

Ah, I see. So it's safe to say that the banlist would not depend on the order of insertions.

I think it would with what you're suggesting. You're suggesting that if there is a shorter wider block, and I insert a longer narrower block-- both get added. But if there is a shorter narrower block and I add a longer wider block the shorter block is removed.

If I give both a relative lifetime of 1 day then make the two calls a second apart, then the resulting set of bans depends on the order.

@gmaxwell I still think we are in agreement that equivalent setban instructions will produce identical banlists. We seem to be disagreeing as to whether the following two sets of RPC instructions are equivalent:

Script 1:

setban 127.0.0.0/24 add
sleep 1
setban 127.0.0.0/32 add

Script 2:

setban 127.0.0.0/32 add
sleep 1
setban 127.0.0.0/24 add

Unless I am missing something, per the current RPC semantics, these are not equivalent instructions and the node will behave differently. In order for the node to behave differently and comply with the instructions, the banlists will have to be different.

For the use case you are trying to accomplish (atomic, bulk additions with relative timestamps), I think there are two ways we can be successful:

1. Update the scripts to convert relative times into absolute times
2. Let's scope and work on a new, addall rpc which can provide bulk atomic commits to the banlist. I volunteer to take it on if we have consensus.

I think another interesting thing this conversation highlights is our expectation of where canonical state lives. i.e. Do we expect that listbanned will return a cumulative of past setban instructions (thereby agreeing with state maintained client-side), or whether listbanned is only responsible for painting a canonical picture of the current ban entries. I personally prefer the latter because: (a) Clients should not assume state cannot change as that assumes there are no other clients and (b) Fewer ban entries seems more efficient for the p2p layer.

Current state of this PR

What does the current patch at tip 9cc54bd do?

It tries to make setban behave consistently whether it is provided with an IP address, or a subnet (IP address range).

Let (s0, t0) be an existing ban, where s0 is a subnet(a contiguous range of IP addresses) and t0 is the ban expiration time. The user is now trying to add another ban (s1, t1). If s1 ⊂ s0 && t1 <= t0 the code does nothing because the requested ban is already in place. If s1 ⊇ s0 && t1 > t0, we remove (s0, t0), add (s1, t1) to consolidate the ban entries. In all other cases we add (s1, t1). More details here.

What is the unaddressed concern expressed by reviewers?

A reviewer feels that we should not consolidate the ban entries, instead allow redundant ban entries in BanMan::m_banned. It is their opinion that this will help keep their existing client scripts simpler. The case they make is that the following scripts should generate identical banlists:

Script 1:

setban 127.0.0.0/24 add
sleep 1
setban 127.0.0.0/32 add

Script 2:

setban 127.0.0.0/32 add
sleep 1
setban 127.0.0.0/24 add

Note: Per current setban RPC semantics, setban ip/subnet add means, "ban this ip/subnet for 1 day (86400 seconds)". i.e. subnet 127.0.0.0/24 add == setban 127.0.0.0/24 add 86400

What is the author's response?

The author feels that the aforementioned scripts (say, run at t=0) are not equivalent, and therefore produce different banlists.

Script 1:

t=0 "Ban 127.0.0.0/24 until t=86400"
t=1 "Ban 127.0.0.0/32 until t=86401"

At t=1, the node must remember two ban entries (127.0.0.0/24, 86400), (127.0.0.0/32, 86401).

Script 2:

t=0 "Ban 127.0.0.0/32 until t=86400"
t=1 "Ban 127.0.0.0/24 until t=86401"

At t=1, it suffices for the node to only remember (127.0.0.0/24, 86401) as (127.0.0.0/32 ⊂ 127.0.0.0/24) && (86400 <= 86401).

The author also feels that not consolidating ban entries when possible (and cheap) creates expense in looking up BanMan::IsBanned(subnet) which is done when new peers are added, or when the addr RPC is processed. The author agrees the overhead is seems minimal and infrequent.

How can reviewers help?

The author is new to Bitcoin development and will continue to keep an open mind. Any case that can be made in interest of the node efficiency or the Bitcoin network, or shows that the proposed change violates established RPC semantics/expectations and therefore is backwards incompatible would help make the case to no longer consolidate ban entries.

I agree with @gmaxwell's suggestion here: #19825 (comment) that we shouldn't try to do any clever logic.

I think the behaviour should be:

• for setban add:
  • Convert the IP address/net into the canonical CIDR subnet representation
  • If that subnet is not already banned, add an entry
  • If that subnet is already banned:
    • if the new expiry is after old expiry, update the entry to the new expiry (i.e. setban add can extend an existing ban)
    • if the new expiry is before the old expiry, don't update the entry to the new expiry (i.e. setban add can't shorten an existing ban)
• for setban remove:
  • Convert the IP address/net into the canonical CIDR subet representation
  • If that subnet is already banned, remove the ban

and then add two new commands:

• setban list (only accepts a single IP address, not a subnet): lists all current ban entries that contain that IP address
• setban removeall (only accepts a single IP address, not a subnet): removes all current ban entries that contain that IP address

Rebased. Ready for further review.

Needs rebase to drop already merged hunks in the fuzz tests

Okay, a lot of things here:

1. I like the consolidation because it removes code.
2. I don’t see a big difference between a no-op and throwing an error?
3. I think UnbanAll would be better if it affected everything downwards (all bans for subnets of given net), not upwards. For “unban all above-nets of a specific addr”, I don’t see a use-case (maybe you can point out one?). If you really want to connect to a given addr, use white list?
4. Same for listbanned

Remove BanMan::Ban(CNetAddr&) and use BanMan::Ban(CSubNet&) instead (ips are subnets of one)
RPC change: setban subnet/ip add now allows adding a ban entry for an ip in an already banned subnet. No error will be thrown so long as the user intent is accomplished.
Remove BanMan::Unban(CNetAddr&) and use BanMan::Unban(CSubNet&) instead (ips are subnets of one)
RPC change: Unbanning a non-banned subnet will no longer raise an rpc error as user intent is accomplished.

I don't understand why are these changes required? The errors right now look correct.

RPC change: Add setban ip removeall to remove all ban entries that include an IP address
RPC change: Add listbanned ip to return all ban entries that include an IP address

Concept ACK for setban ip removeall and listbanned ip

Are you still working on this?

@achow101 yes i'd like to, if there's enough interest in the idea. i'm going to work through getting BIP324 PRs up to date with the new mechanisms introduced in the BIP recently and then I can rebase this.

🐙 This pull request conflicts with the target branch and needs rebase.

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

I'm ok with marking this up for grabs or closing it.