New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add setban/listbanned RPC commands #6158

Merged
merged 10 commits into from Jun 18, 2015

Conversation

Projects
None yet
7 participants
@jonasschnelli
Member

jonasschnelli commented May 19, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 19, 2015

Member

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

Member

jonasschnelli commented May 19, 2015

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

Show outdated Hide outdated src/net.cpp
@@ -458,13 +458,30 @@ bool CNode::IsBanned(CNetAddr ip)
return fResult;
}
bool CNode::Ban(const CNetAddr &addr) {
bool CNode::Ban(const CNetAddr &addr, int64_t bantimeoffset) {

This comment has been minimized.

@Diapolo

Diapolo May 19, 2015

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

@Diapolo

Diapolo May 19, 2015

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

This comment has been minimized.

@jonasschnelli

jonasschnelli May 19, 2015

Member

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.

@jonasschnelli

jonasschnelli May 19, 2015

Member

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.

This comment has been minimized.

@Diapolo

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

@Diapolo

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

This comment has been minimized.

@jonasschnelli

jonasschnelli May 19, 2015

Member

Agreed. Added a commit on top.

@jonasschnelli

jonasschnelli May 19, 2015

Member

Agreed. Added a commit on top.

Show outdated Hide outdated src/net.cpp
return false;
}
bool CNode::GetBanned(std::map<CNetAddr, int64_t> &banMap)

This comment has been minimized.

@Diapolo

Diapolo May 19, 2015

Same question for this function...

@Diapolo

Diapolo May 19, 2015

Same question for this function...

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

This comment has been minimized.

@Diapolo

Diapolo May 19, 2015

listbanned is not added here?

@Diapolo

Diapolo May 19, 2015

listbanned is not added here?

This comment has been minimized.

@jonasschnelli

jonasschnelli May 19, 2015

Member

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

@jonasschnelli

jonasschnelli May 19, 2015

Member

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

Show outdated Hide outdated src/rpcnet.cpp
"\nArguments:\n"
"1. \"ip\" (string, required) The IP (see getpeerinfo for nodes ip)\n"
"2. \"command\" (string, required) 'add' to add a IP to the list, 'remove' to remove a IP from the list\n"
"1. \"bantime\" (numeric, optional) time in seconds how long the ip is banned\n"

This comment has been minimized.

@Diapolo

Diapolo May 19, 2015

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

@Diapolo

Diapolo May 19, 2015

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

This comment has been minimized.

@jonasschnelli

jonasschnelli May 19, 2015

Member

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

@jonasschnelli

jonasschnelli May 19, 2015

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli May 19, 2015

Member

Just added information about the default bantime.

@jonasschnelli

jonasschnelli May 19, 2015

Member

Just added information about the default bantime.

Show outdated Hide outdated src/rpcnet.cpp
+ HelpExampleRpc("setban", "\"192.168.0.6\", \"add\" 86400")
);
string strNode = params[0].get_str();

This comment has been minimized.

@Diapolo

Diapolo May 19, 2015

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

@Diapolo

Diapolo May 19, 2015

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

This comment has been minimized.

@jonasschnelli

jonasschnelli May 19, 2015

Member

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

@jonasschnelli

jonasschnelli May 19, 2015

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli May 19, 2015

Member

Fixed @Diapolo's nit

@jonasschnelli
Show outdated Hide outdated src/rpcnet.cpp
while(CNode *bannedNode = FindNode(netAddr))
bannedNode->CloseSocketDisconnect();
}
else if(strCommand == "remove")

This comment has been minimized.

@Diapolo

Diapolo May 19, 2015

Suggestion: Perhaps also add a removeall option?

@Diapolo

Diapolo May 19, 2015

Suggestion: Perhaps also add a removeall option?

This comment has been minimized.

@LeMiner

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

@LeMiner

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

This comment has been minimized.

@jonasschnelli

jonasschnelli May 19, 2015

Member

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?

@jonasschnelli

jonasschnelli May 19, 2015

Member

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?

This comment has been minimized.

@Diapolo

Diapolo May 19, 2015

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

@Diapolo

Diapolo May 19, 2015

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 19, 2015

Member

Added a clearbanned command (also included in tests).

Member

jonasschnelli commented May 19, 2015

Added a clearbanned command (also included in tests).

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 21, 2015

Member

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

Member

laanwj commented May 21, 2015

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

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 22, 2015

Member

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)

Member

gmaxwell commented May 22, 2015

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

This comment has been minimized.

Show comment
Hide comment
@LeMiner

LeMiner May 22, 2015

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.

LeMiner commented May 22, 2015

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

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

Member

laanwj commented May 22, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 22, 2015

Member

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

Member

jonasschnelli commented May 22, 2015

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

@LeMiner

This comment has been minimized.

Show comment
Hide comment
@LeMiner

LeMiner May 22, 2015

Yep, agreed as well.

LeMiner commented May 22, 2015

Yep, agreed as well.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

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

Member

laanwj commented May 22, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 26, 2015

Member

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

Member

jonasschnelli commented May 26, 2015

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 27, 2015

Member

Rebased to make use of the fixed CSubNet issue (#6186).

Member

jonasschnelli commented May 27, 2015

Rebased to make use of the fixed CSubNet issue (#6186).

Show outdated Hide outdated src/net.cpp
CSubNet subNet = (*it).first;
int64_t t = (*it).second;
std::string a = subNet.ToString();

This comment has been minimized.

@Diapolo

Diapolo Jun 1, 2015

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

@Diapolo

Diapolo Jun 1, 2015

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 1, 2015

Member

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

@jonasschnelli

jonasschnelli Jun 1, 2015

Member

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

Show outdated Hide outdated src/net.cpp
Ban(subNet, bantimeoffset);
}
void CNode::Ban(const CSubNet &subNet, int64_t bantimeoffset) {

This comment has been minimized.

@Diapolo

Diapolo Jun 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.

@Diapolo

Diapolo Jun 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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 1, 2015

Member

Agreed. Fixed.

@jonasschnelli

jonasschnelli Jun 1, 2015

Member

Agreed. Fixed.

def run_test(self):
def run_test(self):

This comment has been minimized.

@luke-jr

luke-jr Jun 2, 2015

Member

What's with all the whitespace changes? :(

@luke-jr

luke-jr Jun 2, 2015

Member

What's with all the whitespace changes? :(

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 2, 2015

Member

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

@jonasschnelli

jonasschnelli Jun 2, 2015

Member

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

Show outdated Hide outdated src/rpcnet.cpp
{
Object rec;
rec.push_back(Pair("address", (*it).first.ToString()));
rec.push_back(Pair("bannedtill", (*it).second));

This comment has been minimized.

@luke-jr

luke-jr Jun 2, 2015

Member

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

@luke-jr

luke-jr Jun 2, 2015

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 2, 2015

Member

Agreed. Will change this.

@jonasschnelli

jonasschnelli Jun 2, 2015

Member

Agreed. Will change this.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 12, 2015

Member

Renamed "bannedtill" to "banned_until".

@jonasschnelli

jonasschnelli Jun 12, 2015

Member

Renamed "bannedtill" to "banned_until".

Show outdated Hide outdated src/rpcnet.cpp
"\nArguments:\n"
"1. \"ip(/netmask)\" (string, required) The IP/Subnet (see getpeerinfo for nodes ip) with a optional netmask (default is /32 = single ip)\n"
"2. \"command\" (string, required) 'add' to add a IP/Subnet to the list, 'remove' to remove a IP/Subnet from the list\n"
"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"

This comment has been minimized.

@luke-jr

luke-jr Jun 2, 2015

Member

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

@luke-jr

luke-jr Jun 2, 2015

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 2, 2015

Member

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.

@jonasschnelli

jonasschnelli Jun 2, 2015

Member

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.

This comment has been minimized.

@luke-jr

luke-jr Jun 2, 2015

Member

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

@luke-jr

luke-jr Jun 2, 2015

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 2, 2015

Member

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

@jonasschnelli

jonasschnelli Jun 2, 2015

Member

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

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 11, 2015

Contributor

ut ACK

Contributor

jgarzik commented Jun 11, 2015

ut ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 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.

Member

laanwj commented Jun 12, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 12, 2015

Member

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

Member

jonasschnelli commented Jun 12, 2015

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

@laanwj

View changes

Show outdated Hide outdated src/netbase.cpp
@@ -1330,6 +1330,11 @@ bool operator!=(const CSubNet& a, const CSubNet& b)
return !(a==b);
}
bool operator<(const CSubNet& a, const CSubNet& b)
{
return (a.network < b.network || (a.network == b.network && memcmp(a.netmask, b.netmask, 16)));

This comment has been minimized.

@laanwj

laanwj Jun 12, 2015

Member

I think this should be:

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

laanwj Jun 12, 2015

Member

I think this should be:

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 12, 2015

Member

Good catch! Thanks for the review.
Fixed.

@jonasschnelli

jonasschnelli Jun 12, 2015

Member

Good catch! Thanks for the review.
Fixed.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 16, 2015

Member

Rebased and added also tests for the new disconnectnode command

Member

jonasschnelli commented Jun 16, 2015

Rebased and added also tests for the new disconnectnode command

@laanwj laanwj merged commit 9d79afe into bitcoin:master Jun 18, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 18, 2015

Merge pull request #6158
9d79afe add RPC tests for setban & disconnectnode (Jonas Schnelli)
1f02b80 setban: add RPCErrorCode (Jonas Schnelli)
d624167 fix CSubNet comparison operator (Jonas Schnelli)
4e36e9b setban: rewrite to UniValue, allow absolute bantime (Jonas Schnelli)
3de24d7 rename json field "bannedtill" to "banned_until" (Jonas Schnelli)
433fb1a [RPC] extend setban to allow subnets (Jonas Schnelli)
e8b9347 [net] remove unused return type bool from CNode::Ban() (Jonas Schnelli)
1086ffb [QA] add setban/listbanned/clearbanned tests (Jonas Schnelli)
d930b26 [RPC] add setban/listbanned/clearbanned RPC commands (Jonas Schnelli)
2252fb9 [net] extend core functionallity for ban/unban/listban (Jonas Schnelli)
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 18, 2015

Member

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

Member

jonasschnelli commented Jun 18, 2015

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

@str4d str4d referenced this pull request Feb 14, 2017

Merged

Bitcoin 0.12 RPC PRs 1 #2100

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment