rpc: simpler setban and new ban manipulation commands

dhruv commented at 11:50 pm on August 27, 2020: contributor

Initially motivated by: https://github.com/bitcoin/bitcoin/pull/19219/files#r442410269

Objectives:

Consolidate BanMan functions
Throw fewer RPC errors. If user intent is accomplished, the command is successful even if no change was made.

Changes:

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.
Remove unused BanMan::IsBanned(CSubNet&)
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

dhruv force-pushed on Aug 27, 2020

dhruv renamed this:
~~rpc: simplify setban and consolidate BanMan functions~~
[WIP] rpc: simplify setban and consolidate BanMan functions
on Aug 28, 2020

dhruv force-pushed on Aug 28, 2020

gmaxwell commented at 0:36 am on August 28, 2020: contributor

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

DrahtBot added the label Docs on Aug 28, 2020

DrahtBot added the label GUI on Aug 28, 2020

DrahtBot added the label P2P on Aug 28, 2020

DrahtBot added the label RPC/REST/ZMQ on Aug 28, 2020

DrahtBot commented at 3:23 am on August 28, 2020: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	jnewbery, glozow, fjahr, jonatack, 1440000bytes

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.

dhruv force-pushed on Aug 28, 2020

dhruv commented at 6:59 am on August 28, 2020: contributor

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 !

jnewbery commented at 7:06 am on August 28, 2020: contributor

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

dhruv commented at 11:24 pm on August 28, 2020: contributor

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 ?

sipa commented at 6:36 pm on August 29, 2020: member

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?

gmaxwell commented at 3:49 am on August 30, 2020: contributor

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.

0setban 1.2/16 add
1setban 1.2.3/24 add
2setban 1.2.3.5/32 remove #does nothing
3setban 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.

dhruv commented at 5:19 am on August 30, 2020: contributor

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 ?

gmaxwell commented at 5:55 am on August 30, 2020: contributor

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

dhruv commented at 6:46 am on August 30, 2020: contributor

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

gmaxwell commented at 9:13 am on August 30, 2020: contributor

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.

dhruv force-pushed on Aug 30, 2020

dhruv renamed this:
~~[WIP] rpc: simplify setban and consolidate BanMan functions~~
rpc: simplify setban and consolidate BanMan functions
on Aug 30, 2020

dhruv commented at 5:48 pm on August 30, 2020: contributor

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

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

Script 2:

0setban 127.0.0.0/32 add
1sleep 1
2setban 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:

Update the scripts to convert relative times into absolute times
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.

dhruv force-pushed on Aug 30, 2020

dhruv force-pushed on Sep 2, 2020

dhruv commented at 7:38 pm on September 6, 2020: contributor

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:

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

Script 2:

0setban 127.0.0.0/32 add
1sleep 1
2setban 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:

0t=0 "Ban 127.0.0.0/24 until t=86400"
1t=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:

0t=0 "Ban 127.0.0.0/32 until t=86400"
1t=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.

jnewbery commented at 9:29 am on September 28, 2020: contributor

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

maflcko removed the label Docs on Sep 28, 2020

maflcko removed the label GUI on Sep 28, 2020

dhruv commented at 3:30 pm on September 28, 2020: contributor

Thank you for the kind review, @jnewbery. I’ve heard the same opinion from another casual in-person conversation. It seems like @practicalswift agrees as well. Sounds like there is consensus that simplicity of logic wins over performance in this case, especially given the minimal overhead. I will re-work the PR this week.

fanquake added the label Waiting for author on Sep 29, 2020

fanquake marked this as a draft on Sep 29, 2020

dhruv force-pushed on Dec 14, 2020

dhruv marked this as ready for review on Dec 14, 2020

dhruv commented at 7:30 am on December 14, 2020: contributor

@jnewbery @gmaxwell @practicalswift I’ve changed the code to reflect what we agreed on.

Objectives:

Consolidate BanMan functions
Throw fewer RPC errors. If user intent is accomplished, the command is successful even if no change was made.

Changes:

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.
Remove unused BanMan::IsBanned(CSubNet&)
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

dhruv force-pushed on Dec 14, 2020

dhruv commented at 7:44 am on December 14, 2020: contributor

Force pushed again to alter commit messages. Ready for review.

fanquake removed the label Waiting for author on Dec 14, 2020

dhruv renamed this:
~~rpc: simplify setban and consolidate BanMan functions~~
rpc: simplify setban, consolidate `BanMan` functions and add `listbanned ip` and `setban ip removeall`
on Dec 14, 2020

dhruv renamed this:
~~rpc: simplify setban, consolidate `BanMan` functions and add `listbanned ip` and `setban ip removeall`~~
rpc: simpler setban and new ban manipulation commands
on Dec 14, 2020

in src/test/fuzz/banman.cpp:40 in a39cca4266 outdated

37@@ -38,11 +38,6 @@ void test_one_input(const std::vector<uint8_t>& buffer)
38         BanMan ban_man{banlist_file, nullptr, ConsumeBanTimeOffset(fuzzed_data_provider)};
39         while (fuzzed_data_provider.ConsumeBool()) {
40             switch (fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 11)) {

glozow commented at 10:10 pm on January 1, 2021:

Should this be switching on a range of cases 0 to 8 now?

dhruv commented at 7:16 am on January 4, 2021:

Yup - could increase test coverage. Done.

in test/functional/p2p_disconnect_ban.py:12 in a39cca4266 outdated

 8@@ -9,6 +9,7 @@
 9 from test_framework.util import (
10     assert_equal,
11     assert_raises_rpc_error,
12+    assert_approx,

glozow commented at 10:24 pm on January 1, 2021:

nit: sort alphabetically

in src/banman.cpp:150 in a39cca4266 outdated

147+        }
148+
149+        if (!entries_to_delete.empty()) m_is_dirty = true;
150+        for (const auto&it : entries_to_delete) {
151+            m_banned.erase(it);
152+        }

glozow commented at 11:37 pm on January 1, 2021:

Why not erase as you go and use m_banned.size() to infer m_is_dirty, instead of constructing a whole new vector?

jnewbery commented at 2:06 pm on January 2, 2021:

Using erase-remove_if is also more efficient - you’ll only need to pass over m_banned once instead of up to entries_to_delete.size() times.

glozow commented at 7:48 pm on January 2, 2021:

yeah erase_if would be the best but it’s C++20 😭

dhruv commented at 7:26 am on January 4, 2021:

Since map.erase(it) invalidates it I was doing a double-pass to avoid incrementing it. However, it turns out erase will return the Iterator following the last removed element. So I’ve updated the code to use that. @jnewbery I am confused about this:

you’ll only need to pass over m_banned once instead of up to entries_to_delete.size() times.

I think the code is only passing over m_banned once. Am I missing something? I also tried erase-remove_if but it does not seem to work for associative containers. Is there an example I can learn from?

jnewbery commented at 10:31 am on January 14, 2021:

Sorry. I mistakenly thought that m_banned was a vector.

in src/banman.cpp:163 in a39cca4266 outdated

161     LOCK(m_cs_banned);
162     // Sweep the banlist so expired bans are not returned
163     SweepBanned();
164     banmap = m_banned; //create a thread safe copy
165+
166+    if (filterForIp) {

glozow commented at 11:38 pm on January 1, 2021:

same, I wonder if this could just be a std::remove_if

dhruv commented at 7:26 am on January 4, 2021:

Done.

in src/rpc/net.cpp:690 in a39cca4266 outdated

685@@ -702,7 +686,9 @@ static RPCHelpMan listbanned()
686 {
687     return RPCHelpMan{"listbanned",
688                 "\nList all manually banned IPs/Subnets.\n",
689-                {},
690+                {
691+                    {"ip", RPCArg::Type::STR, "all ips", "If IP is provided, only bans which include the IP will be returned."},

glozow commented at 11:43 pm on January 1, 2021:

RPCArg::Optional::OMITTED_NAMED_ARG?

dhruv commented at 7:26 am on January 4, 2021:

Thanks for showing me that. Much better.

glozow commented at 11:49 pm on January 1, 2021: member

Concept ACK thinking of this as a cleanup based on your description, Ieft some light code review for now.

dhruv force-pushed on Jan 4, 2021

in src/banman.h:80 in e650bba6fd outdated

76@@ -77,7 +77,7 @@ class BanMan
77 
78     // Removes all ban entries that include net_addr
79     void UnbanAll(const CNetAddr& net_addr);
80-    void GetBanned(banmap_t& banmap);
81+    void GetBanned(banmap_t& banmap, std::optional<const CNetAddr> filterForIp = {});

dhruv commented at 7:57 am on January 4, 2021:

#20671 was merged, so I’m now using std::optional instead of boost::optional.

std::optional<T> is considered ill-formed if is_reference_v<T> and also if !is_destructible_v<T>. So we can’t use std::optional<const CNetAddr&> and can’t forward declare CNetAddr and CSubNet in this file (incomplete types are not destructible).

jnewbery commented at 11:46 am on January 8, 2021:

I think better than adding this std::optional<CNetAddr> would be to just overload the GetBanned() function, since most of the logic in the function is now specific to the one case where you pass a filterForIp. Those feel like almost two different functional interfaces.

I think better yet would be to do the filtering logic inside the RPC listbanned() function. It’s not required by any other callers, so I don’t think there’s a need to add it to banman. If other callers need the same filtering logic at some point in the future, the logic can be moved into banman at that point.

dhruv commented at 7:11 am on January 13, 2021:

That sounds good. Updated code accordingly.

dhruv commented at 8:19 am on January 4, 2021: contributor

Thank you for the reviews, @glozow @jnewbery. Comments addressed. Rebased. Ready for further review.

DrahtBot added the label Needs rebase on Jan 11, 2021

dhruv force-pushed on Jan 13, 2021

DrahtBot removed the label Needs rebase on Jan 13, 2021

dhruv commented at 7:13 am on January 13, 2021: contributor

Rebased and addressed #19825 (review). Ready for further review.

DrahtBot added the label Needs rebase on Jan 14, 2021

in src/test/fuzz/banman.cpp:47 in 38abc322d3 outdated

43             case 0: {
44-                ban_man.Ban(ConsumeNetAddr(fuzzed_data_provider),
45-                    ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
46-                break;
47-            }
48-            case 1: {

maflcko commented at 10:32 am on January 14, 2021:

After you rebase, you may want to apply this patch: #20837 (comment)

dhruv commented at 0:31 am on January 27, 2021:

Done in 0dbe88ad3894cd76d52ed196fd2eed39de1296d2

fjahr commented at 10:42 pm on January 23, 2021: contributor

Concept ACK

in src/banman.cpp:148 in 38abc322d3 outdated

143+            CSubNet sub_net = it->first;
144+            if (sub_net.Match(net_addr)) {
145+                it = m_banned.erase(it);
146+                m_is_dirty = true;
147+            } else {
148+                it++;

jonatack commented at 1:22 pm on January 25, 2021:

579eaaa prefer prefix iterator over postfix one (see developer-notes.md)

dhruv commented at 0:32 am on January 27, 2021:

Done

in src/rpc/net.cpp:666 in 38abc322d3 outdated

662@@ -663,7 +663,7 @@ static RPCHelpMan setban()
663                 "\nAttempts to add or remove an IP/Subnet from the banned list.\n",
664                 {
665                     {"subnet", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP/Subnet (see getpeerinfo for nodes IP) with an optional netmask (default is /32 = single IP)"},
666-                    {"command", RPCArg::Type::STR, RPCArg::Optional::NO, "'add' to add an IP/Subnet to the list, 'remove' to remove an IP/Subnet from the list"},
667+                    {"command", RPCArg::Type::STR, RPCArg::Optional::NO, "'add' to add an IP/Subnet to the list, 'remove' to remove an IP/Subnet from the list, 'removeall' to remove all bans that include the single ip address"},

jonatack commented at 1:23 pm on January 25, 2021:

579eaaa

0                    {"command", RPCArg::Type::STR, RPCArg::Optional::NO, "'add' to add an IP/Subnet to the list, 'remove' to remove an IP/Subnet from the list, 'removeall' to remove all bans that include the single IP address"},

dhruv commented at 0:32 am on January 27, 2021:

Done

in src/rpc/net.cpp:691 in 38abc322d3 outdated

687         throw JSONRPCError(RPC_DATABASE_ERROR, "Error: Ban database not loaded");
688     }
689 
690+    bool isSubnet = (request.params[0].get_str().find('/') != std::string::npos);
691+    if (strCommand == "removeall" && isSubnet) {
692+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: setban removeall requires single ip address");

jonatack commented at 1:24 pm on January 25, 2021:

579eaaa

0        throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: setban removeall requires a single IP address");

dhruv commented at 0:32 am on January 27, 2021:

Done

in src/rpc/net.cpp:718 in 38abc322d3 outdated

729-        if (!( isSubnet ? node.banman->Unban(subNet) : node.banman->Unban(netAddr) )) {
730-            throw JSONRPCError(RPC_CLIENT_INVALID_IP_OR_SUBNET, "Error: Unban failed. Requested address/subnet was not previously manually banned.");
731+        if (node.banman->Ban(subNet, banTime, absolute) && node.connman) {
732+            node.connman->DisconnectNode(subNet);
733         }
734+    } else if(strCommand == "remove") {

jonatack commented at 1:24 pm on January 25, 2021:

579eaaa

0    } else if (strCommand == "remove") {

dhruv commented at 0:32 am on January 27, 2021:

Done

in src/rpc/net.cpp:761 in 38abc322d3 outdated

753@@ -768,7 +754,21 @@ static RPCHelpMan listbanned()
754 
755     banmap_t banMap;
756     node.banman->GetBanned(banMap);
757-
758+    if (!request.params[0].isNull()) {
759+        CNetAddr netAddr;
760+        LookupHost(request.params[0].get_str(), netAddr, false);
761+        if (!netAddr.IsValid()) {
762+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: Invalid ip address");

jonatack commented at 1:26 pm on January 25, 2021:

38abc322

0            throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: Invalid IP address");

dhruv commented at 0:32 am on January 27, 2021:

Done

in src/rpc/net.cpp:768 in 38abc322d3 outdated

764+        for (auto it = banMap.begin(); it != banMap.end();) {
765+            CSubNet ban_sub_net = it->first;
766+            if (!ban_sub_net.Match(netAddr)) {
767+                it = banMap.erase(it);
768+            } else {
769+                it++;

jonatack commented at 1:26 pm on January 25, 2021:

38abc322 prefer prefix iterator

0                ++it;

dhruv commented at 0:32 am on January 27, 2021:

Done

in src/banman.cpp:117 in 38abc322d3 outdated

113         if (m_banned[sub_net].nBanUntil < ban_entry.nBanUntil) {
114             m_banned[sub_net] = ban_entry;
115             m_is_dirty = true;
116         } else
117-            return;
118+            return false; // nothing to do, longer ban already exists

jonatack commented at 1:28 pm on January 25, 2021:

877b14aaeb ISTM this behavior ought to be documented/clarified in the setban rpc help

dhruv commented at 0:33 am on January 27, 2021:

Done.

jonatack commented at 1:29 pm on January 25, 2021: contributor

Concept ACK, need rebase, a few minor comments on cursory first look

dhruv force-pushed on Jan 27, 2021

dhruv commented at 0:33 am on January 27, 2021: contributor

Rebased and comments addressed. Ready for review.

dhruv force-pushed on Jan 27, 2021

DrahtBot removed the label Needs rebase on Jan 27, 2021

dhruv commented at 6:23 am on January 27, 2021: contributor

Failing test fixed. Ready for review.

in src/rpc/net.cpp:955 in 8ad64d2879 outdated

951@@ -952,7 +952,7 @@ static const CRPCCommand commands[] =
952     { "network",            "getnettotals",           &getnettotals,           {} },
953     { "network",            "getnetworkinfo",         &getnetworkinfo,         {} },
954     { "network",            "setban",                 &setban,                 {"subnet", "command", "bantime", "absolute"} },
955-    { "network",            "listbanned",             &listbanned,             {} },
956+    { "network",            "listbanned",             &listbanned,             {"ip"} },

maflcko commented at 6:40 pm on January 28, 2021:

Can remove this diff, and then rebase

dhruv commented at 7:25 pm on January 28, 2021:

Thanks for making that easier :)

DrahtBot added the label Needs rebase on Jan 28, 2021

dhruv force-pushed on Jan 28, 2021

dhruv commented at 7:25 pm on January 28, 2021: contributor

Rebased. Ready for further review.

DrahtBot removed the label Needs rebase on Jan 28, 2021

DrahtBot added the label Needs rebase on Apr 11, 2021

[rpc, refactor] Simplify setban logic and consolidate BanMan::Ban() functions

- consolidate Ban() functions
- setban logic is simpler: adding a subset ban will not throw an error

80044bd2ed

[rpc, refactor] Consolidate BanMan::Unban() functions

Unbanning a non-banned subnet should not raise an rpc error

b2272c6816

[refactor] Remove unused BanMan::IsBanned(CSubNet) 66457146be

[rpc] Add `setban ip removeall` command bfdb8a1b1c

[rpc] Add `listbanned ip` command b84ee4730c

[fuzz] removing unused fuzz test branches (invalidates banman seeds) a2a36f93ff

dhruv force-pushed on Apr 23, 2021

dhruv commented at 9:09 pm on April 23, 2021: contributor

Rebased. Ready for further review.

DrahtBot removed the label Needs rebase on Apr 23, 2021

maflcko commented at 12:08 pm on July 30, 2021: member

Needs rebase to drop already merged hunks in the fuzz tests

in src/netaddress.cpp:1119 in 80044bd2ed outdated

1115@@ -1116,7 +1116,7 @@ CSubNet::CSubNet(const CNetAddr& addr) : CSubNet()
1116     switch (addr.m_net) {
1117     case NET_IPV4:
1118     case NET_IPV6:
1119-        valid = true;
1120+        valid = addr.IsValid();

naumenkogs commented at 10:10 am on September 28, 2021:

what is this change exactly about? Why is it relevant to the given PR?

naumenkogs commented at 10:10 am on September 28, 2021:

I see it’s kinda used below in the same commit 80044bd2ed96c391e9e87d77cc876e653b512657, but splitting this change out (e.g. do it first) would simplify the review.

naumenkogs commented at 10:18 am on September 28, 2021: member

Okay, a lot of things here:

I like the consolidation because it removes code.
I don’t see a big difference between a no-op and throwing an error?
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?
Same for listbanned

ghost commented at 1:42 am on August 9, 2022: none

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

achow101 commented at 6:14 pm on October 12, 2022: member

Are you still working on this?

dhruv commented at 11:22 pm on October 12, 2022: contributor

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

DrahtBot commented at 1:11 pm on January 9, 2023: contributor

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

DrahtBot added the label Needs rebase on Jan 9, 2023

DrahtBot commented at 1:36 am on April 9, 2023: contributor

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.

dhruv commented at 3:05 am on April 9, 2023: contributor

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

dhruv closed this on Apr 9, 2023

bitcoin locked this on Apr 8, 2024

rpc: simpler setban and new ban manipulation commands #19825

Objectives:

Changes:

Reviews

Conflicts

Potential solution

Current state of this PR

What does the current patch at tip 9cc54bd do?

What is the unaddressed concern expressed by reviewers?

What is the author’s response?

How can reviewers help?

Objectives:

Changes:

What does the current patch at tip `9cc54bd` do?