Add setban/listbanned RPC commands

jonasschnelli commented at 8:30 am on May 19, 2015: contributor

Groundwork for #5866. If this makes it into master i’d like to add a GUI context menu for the peers table. A simple disconnect (without banning) would be possible with setban <ip> add 1 (where 1 is the bantime).

At the moment the banned set does not survive a restart (should be added once). Also currently banning is per IP and not per Node which results in disconnecting all nodes of a given IP (if the nodes uses the same ip but different ports).

Also includes some whitespace fixes for httpbasics.py test.

jonasschnelli force-pushed on May 19, 2015

jonasschnelli commented at 9:01 am on May 19, 2015: contributor

I’m open to alternatives (naming) for setban and listbanned. I thought adding banning options over the addnode command is a misuse. I’m also not sure about banning whole IPs instead of only IP:port (node).

in src/net.cpp: in ab10e9d0ce outdated

457@@ -458,13 +458,30 @@ bool CNode::IsBanned(CNetAddr ip)
458     return fResult;
459 }
460 
461-bool CNode::Ban(const CNetAddr &addr) {
462+bool CNode::Ban(const CNetAddr &addr, int64_t bantimeoffset) {

Diapolo commented at 12:32 pm on May 19, 2015:

Why was/is this returning a bool, if it seems to be only true?

jonasschnelli commented at 12:39 pm on May 19, 2015:

Right. I didn’t want to change this in this PR and i kept it for legacy reasons and to lower the risk of breaking something.

Diapolo commented at 3:05 pm on May 19, 2015:

Understood, but IMHO a function that doesn’t need it could just be void. Perhaps you can just add a commit for that at the end? At least it could be changed for GetBanned as you just added it :).

jonasschnelli commented at 3:16 pm on May 19, 2015:

Agreed. Added a commit on top.

in src/net.cpp: in ab10e9d0ce outdated

481+    if (setBanned.erase(addr))
482+        return true;
483+    return false;
484+}
485+
486+bool CNode::GetBanned(std::map<CNetAddr, int64_t> &banMap)

Diapolo commented at 12:33 pm on May 19, 2015:

Same question for this function…

in src/rpcclient.cpp: in ab10e9d0ce outdated

90@@ -91,6 +91,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
91     { "estimatepriority", 0 },
92     { "prioritisetransaction", 1 },
93     { "prioritisetransaction", 2 },
94+    { "setban", 2 },

Diapolo commented at 12:35 pm on May 19, 2015:

listbanned is not added here?

jonasschnelli commented at 12:40 pm on May 19, 2015:

This is only for the rpc client (bitcoin-cli) string to json conversion which is not required for listbanned.

in src/rpcnet.cpp: in ab10e9d0ce outdated

455+                            "setban \"node\" \"add|remove\" (bantime)\n"
456+                            "\nAttempts add or remove a IP from the banned list.\n"
457+                            "\nArguments:\n"
458+                            "1. \"ip\"       (string, required) The IP (see getpeerinfo for nodes ip)\n"
459+                            "2. \"command\"  (string, required) 'add' to add a IP to the list, 'remove' to remove a IP from the list\n"
460+                            "1. \"bantime\"  (numeric, optional) time in seconds how long the ip is banned\n"

Diapolo commented at 12:36 pm on May 19, 2015:

What is the default value, if no bantime is supplied? Perhaps add this information in here?

jonasschnelli commented at 12:41 pm on May 19, 2015:

Right. This is a good point. It’s actually the cmd arg -bantime. Will add this information.

jonasschnelli commented at 3:01 pm on May 19, 2015:

Just added information about the default bantime.

in src/rpcnet.cpp: in ab10e9d0ce outdated

461+                            "\nExamples:\n"
462+                            + HelpExampleCli("setban", "\"192.168.0.6\" \"add\" 86400")
463+                            + HelpExampleRpc("setban", "\"192.168.0.6\", \"add\" 86400")
464+                            );
465+
466+    string strNode = params[0].get_str();

Diapolo commented at 12:36 pm on May 19, 2015:

Why make a copy here, you seem to only use it once anyway?

jonasschnelli commented at 12:41 pm on May 19, 2015:

Indeed. This is a relict from copy/paste some code from addnode. Will change.

jonasschnelli commented at 3:01 pm on May 19, 2015:

Fixed @Diapolo’s nit

in src/rpcnet.cpp: in ab10e9d0ce outdated

481+
482+        //disconnect possible nodes
483+        while(CNode *bannedNode = FindNode(netAddr))
484+            bannedNode->CloseSocketDisconnect();
485+    }
486+    else if(strCommand == "remove")

Diapolo commented at 12:37 pm on May 19, 2015:

Suggestion: Perhaps also add a removeall option?

LeMiner commented at 2:29 pm on May 19, 2015:

+1 for removeall option to clear the list of banned peers. And if possible, adding a right click -> kick/ban option for GUI, which could also be implemented like this: http://i.imgur.com/K5jifJx.png (but better looking obv.)

jonasschnelli commented at 2:45 pm on May 19, 2015:

Agreed on the remove all feature. But it would be ugly to have a command like setban <IP> removeall ( would then be dummy). And supporting setban removeall will make the help message unreadable (because of parameter mixups). IMO adding a clearbanned command could make sense. Any other suggestions?

Diapolo commented at 3:08 pm on May 19, 2015:

clearbanned is fine with me, or perhaps setban * remove ;)?

jonasschnelli force-pushed on May 19, 2015

jonasschnelli commented at 3:09 pm on May 19, 2015: contributor

Added a clearbanned command (also included in tests).

jonasschnelli force-pushed on May 19, 2015

laanwj added the label Feature on May 20, 2015

laanwj added the label P2P on May 20, 2015

laanwj commented at 10:52 am on May 21, 2015: member

Concept ACK, did not test or review code yet, this will be after 0.11 release

gmaxwell commented at 6:46 am on May 22, 2015: contributor

Great; I had an old bitrotted version of this and had implemented almost exactly the same interface. One thing this can’t accomplish though is banning netgroups. (thats actually what held up my code: I found issues with out netgroup parser that broke my tests)

LeMiner commented at 10:00 am on May 22, 2015: none

In light of what gmaxwell said, perhaps allowing for – setban 12. * .12.12 or setban 50.50.50. * would make sense as well. To allow for banning of entire octets.

laanwj commented at 10:08 am on May 22, 2015: member

@leminer Good idea, but I’d say the interface should use /n or /x.y.z.w CIDR syntax (as parsed by CSubNet) instead of bringing back 0.9-era wildcards.

jonasschnelli commented at 11:32 am on May 22, 2015: contributor

Agreed with @laanwj. I don’t think there is a use case for 1.x.2.3

LeMiner commented at 11:39 am on May 22, 2015: none

Yep, agreed as well.

laanwj commented at 12:26 pm on May 22, 2015: member

1.x.2.3 in CIDR would be 1.0.2.3/255.0.255.255. But no, I don’t see a use-case either.

jonasschnelli force-pushed on May 26, 2015

jonasschnelli commented at 7:55 am on May 26, 2015: contributor

Extended this PR to allow subnet banning/unbanning. This needs testing because it changes the internal ban set from CNetAddr to CSubNet.

jonasschnelli force-pushed on May 26, 2015

jonasschnelli force-pushed on May 27, 2015

jonasschnelli commented at 7:15 am on May 27, 2015: contributor

Rebased to make use of the fixed CSubNet issue (https://github.com/bitcoin/bitcoin/pull/6186).

in src/net.cpp: in 2eff5b874f outdated

460+        for (std::map<CSubNet, int64_t>::iterator it = setBanned.begin(); it != setBanned.end(); it++)
461+        {
462+            CSubNet subNet = (*it).first;
463+            int64_t t = (*it).second;
464+
465+            std::string a = subNet.ToString();

Diapolo commented at 12:09 pm on June 1, 2015:

Perhaps I’m not seeing it, but where is a or b used at all?

jonasschnelli commented at 12:15 pm on June 1, 2015:

Oh. Right. Fixed (did use those lines for debugging).

in src/net.cpp: in 2eff5b874f outdated

490+void CNode::Ban(const CNetAddr &addr, int64_t bantimeoffset) {
491+    CSubNet subNet(addr.ToString()+(addr.IsIPv4() ? "/32" : "/128"));
492+    Ban(subNet, bantimeoffset);
493+}
494+
495+void CNode::Ban(const CSubNet &subNet, int64_t bantimeoffset) {

Diapolo commented at 12:10 pm on June 1, 2015:

Nit: The & should be left CSubNet&. This is also true for the new functions below… will make the clang-cleanup smaller in the future.

jonasschnelli commented at 12:19 pm on June 1, 2015:

Agreed. Fixed.

jonasschnelli force-pushed on Jun 1, 2015

in qa/rpc-tests/httpbasics.py: in 11b6c2ffa1 outdated

26         return start_nodes(4, self.options.tmpdir, extra_args=[['-rpckeepalive=1'], ['-rpckeepalive=0'], [], []])
27 
28-    def run_test(self):        
29-        
30+    def run_test(self):
31+

luke-jr commented at 4:01 am on June 2, 2015:

What’s with all the whitespace changes? :(

jonasschnelli commented at 4:12 am on June 2, 2015:

A relict from one of my previous pulls which i try to clean out without big noise. :-)

in src/rpcnet.cpp: in 11b6c2ffa1 outdated

520+    Array bannedAddresses;
521+    for (std::map<CSubNet, int64_t>::iterator it = banMap.begin(); it != banMap.end(); it++)
522+    {
523+        Object rec;
524+        rec.push_back(Pair("address", (*it).first.ToString()));
525+        rec.push_back(Pair("bannedtill", (*it).second));

luke-jr commented at 4:06 am on June 2, 2015:

Full words? (s/till/until/) Maybe add underscore?

jonasschnelli commented at 4:14 am on June 2, 2015:

Agreed. Will change this.

jonasschnelli commented at 4:35 pm on June 12, 2015:

Renamed “bannedtill” to “banned_until”.

in src/rpcnet.cpp: in 11b6c2ffa1 outdated

455+                            "setban \"ip(/netmask)\" \"add|remove\" (bantime)\n"
456+                            "\nAttempts add or remove a IP/Subnet from the banned list.\n"
457+                            "\nArguments:\n"
458+                            "1. \"ip(/netmask)\" (string, required) The IP/Subnet (see getpeerinfo for nodes ip) with a optional netmask (default is /32 = single ip)\n"
459+                            "2. \"command\"      (string, required) 'add' to add a IP/Subnet to the list, 'remove' to remove a IP/Subnet from the list\n"
460+                            "1. \"bantime\"      (numeric, optional) time in seconds how long the ip is banned (0 or empty means using the default time of 24h which can also be overwritten by the -bantime startup argument)\n"

luke-jr commented at 4:06 am on June 2, 2015:

Should have some way to set an absolute time here, so banlists can be easily saved/restored across restarts.

jonasschnelli commented at 4:14 am on June 2, 2015:

The stored time is absolute (int64 Unix timestamp). The offset is more a input thing and I think it’s suitable when banning a node.

luke-jr commented at 5:18 am on June 2, 2015:

Unless you can input an absolute time, restoring a saved list of bans is annoying.

jonasschnelli commented at 6:45 pm on June 2, 2015:

Right. An additional method to set the bantime over an absolute value would make sense.

jgarzik commented at 7:45 pm on June 11, 2015: contributor

ut ACK

laanwj commented at 2:53 pm on June 12, 2015: member

Needs rebase. I agree with @luke-jr that it should be somehow possible to specify an absolute ban time. Relative times are useful for end-users, but not so much for programmatic use.

jonasschnelli force-pushed on Jun 12, 2015

jonasschnelli commented at 4:34 pm on June 12, 2015: contributor

Rebased and added the possibility of setting an absolute bantime with setban add <ip> <unixtimestamp> true.

in src/netbase.cpp: in 83b0cc61f6 outdated

1329@@ -1330,6 +1330,11 @@ bool operator!=(const CSubNet& a, const CSubNet& b)
1330     return !(a==b);
1331 }
1332 
1333+bool operator<(const CSubNet& a, const CSubNet& b)
1334+{
1335+    return (a.network < b.network || (a.network == b.network && memcmp(a.netmask, b.netmask, 16)));

laanwj commented at 4:49 pm on June 12, 2015:

I think this should be:

0return (a.network < b.network || (a.network == b.network && memcmp(a.netmask, b.netmask, 16) < 0));

jonasschnelli commented at 5:52 pm on June 12, 2015:

Good catch! Thanks for the review. Fixed.

jonasschnelli force-pushed on Jun 12, 2015

jonasschnelli force-pushed on Jun 16, 2015

jonasschnelli commented at 4:22 pm on June 16, 2015: contributor

Rebased and added also tests for the new disconnectnode command

[net] extend core functionallity for ban/unban/listban 2252fb91cd

[RPC] add setban/listbanned/clearbanned RPC commands d930b26a26

[QA] add setban/listbanned/clearbanned tests 1086ffba26

[net] remove unused return type bool from CNode::Ban() e8b93473f1

[RPC] extend setban to allow subnets 433fb1a95d

rename json field "bannedtill" to "banned_until" 3de24d7647

setban: rewrite to UniValue, allow absolute bantime 4e36e9bcc7

fix CSubNet comparison operator d624167387

setban: add RPCErrorCode 1f02b80253

add RPC tests for setban & disconnectnode 9d79afe9a9

jonasschnelli force-pushed on Jun 17, 2015

laanwj merged this on Jun 18, 2015

laanwj closed this on Jun 18, 2015

laanwj referenced this in commit 0abfa8a22f on Jun 18, 2015

jonasschnelli commented at 2:51 pm on June 18, 2015: contributor

Thanks for the merge. I now try to extend this to the UI peers window.

zkbot referenced this in commit 9af55822fb on Feb 15, 2017

zkbot referenced this in commit a7cf698873 on Mar 4, 2017

MarcoFalke locked this on Sep 8, 2021

Add setban/listbanned RPC commands #6158