Add setban/listbanned RPC commands #6158

pull jonasschnelli wants to merge 10 commits into bitcoin:master from jonasschnelli:2015/05/rpc_ban changing 13 files +331 −28
  1. 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.

  2. jonasschnelli force-pushed on May 19, 2015
  3. jonasschnelli force-pushed on May 19, 2015
  4. 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).
  5. 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.
  6. 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…
  7. 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.
  8. 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.
  9. 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
  10. 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 ;)?
  11. jonasschnelli force-pushed on May 19, 2015
  12. jonasschnelli force-pushed on May 19, 2015
  13. jonasschnelli force-pushed on May 19, 2015
  14. jonasschnelli commented at 3:09 pm on May 19, 2015: contributor
    Added a clearbanned command (also included in tests).
  15. jonasschnelli force-pushed on May 19, 2015
  16. jonasschnelli force-pushed on May 19, 2015
  17. jonasschnelli force-pushed on May 19, 2015
  18. jonasschnelli force-pushed on May 19, 2015
  19. laanwj added the label Feature on May 20, 2015
  20. laanwj added the label P2P on May 20, 2015
  21. 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
  22. 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)
  23. 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.
  24. 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.
  25. 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
  26. LeMiner commented at 11:39 am on May 22, 2015: none
    Yep, agreed as well.
  27. 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.
  28. jonasschnelli force-pushed on May 26, 2015
  29. 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.
  30. jonasschnelli force-pushed on May 26, 2015
  31. jonasschnelli force-pushed on May 27, 2015
  32. 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).
  33. 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).
  34. 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.
  35. jonasschnelli force-pushed on Jun 1, 2015
  36. jonasschnelli force-pushed on Jun 1, 2015
  37. 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. :-)
  38. 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”.
  39. 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.
  40. jgarzik commented at 7:45 pm on June 11, 2015: contributor
    ut ACK
  41. 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.
  42. jonasschnelli force-pushed on Jun 12, 2015
  43. jonasschnelli force-pushed on Jun 12, 2015
  44. 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.
  45. 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.
  46. jonasschnelli force-pushed on Jun 12, 2015
  47. jonasschnelli force-pushed on Jun 16, 2015
  48. jonasschnelli force-pushed on Jun 16, 2015
  49. jonasschnelli commented at 4:22 pm on June 16, 2015: contributor
    Rebased and added also tests for the new disconnectnode command
  50. [net] extend core functionallity for ban/unban/listban 2252fb91cd
  51. [RPC] add setban/listbanned/clearbanned RPC commands d930b26a26
  52. [QA] add setban/listbanned/clearbanned tests 1086ffba26
  53. [net] remove unused return type bool from CNode::Ban() e8b93473f1
  54. [RPC] extend setban to allow subnets 433fb1a95d
  55. rename json field "bannedtill" to "banned_until" 3de24d7647
  56. setban: rewrite to UniValue, allow absolute bantime 4e36e9bcc7
  57. fix CSubNet comparison operator d624167387
  58. setban: add RPCErrorCode 1f02b80253
  59. add RPC tests for setban & disconnectnode 9d79afe9a9
  60. jonasschnelli force-pushed on Jun 17, 2015
  61. laanwj merged this on Jun 18, 2015
  62. laanwj closed this on Jun 18, 2015
  63. laanwj referenced this in commit 0abfa8a22f on Jun 18, 2015
  64. 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.
  65. zkbot referenced this in commit 9af55822fb on Feb 15, 2017
  66. zkbot referenced this in commit a7cf698873 on Mar 4, 2017
  67. MarcoFalke locked this on Sep 8, 2021


jonasschnelli Diapolo LeMiner laanwj gmaxwell luke-jr jgarzik

Labels
Feature P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-16 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me