[net] Allow disconnectnode RPC to be called with node id

jnewbery commented at 4:45 pm on April 3, 2017: member

The disconnectnode RPC can currently only be called with the IP address/port of the node the user wishes to connect. This commit allows the node to be disconnected using the nodeid returned by getpeerinfo().

theuni commented at 8:27 pm on April 3, 2017: member

Concept ACK. Though if we’re going to be exposing this value to our apis, we need to define it sanely first.

ATM it’s just an int, so it’s tricky encode/decode it safely. I’d prefer change it to a uint32_t first, so that we can use ParseUInt32 here.

gmaxwell commented at 9:23 pm on April 3, 2017: contributor

Concept ACK, I’ve wanted this many times before… but I’m concerned about the nodeid— right now if it wraps bad things happen, and it will be harder to fix if we’ve made it a part of the API.

theuni commented at 9:52 pm on April 3, 2017: member

edit: Decided I didn’t like this thought and bailed. Meant to cancel, closed instead. Sorry!

theuni closed this on Apr 3, 2017

theuni reopened this on Apr 3, 2017

theuni commented at 11:27 pm on April 3, 2017: member

I suppose it would be prudent to

determine where we assume nodeids are increasing in value, as opposed to just being unique. I should hope there are none of these, other than generally being able to assume that higher node == earlier connection
determine when we assume a nodeid to be globally unique. For example, if there’s a global map<nodeid,int> that tallies sent bytes, that would be broken by duplicated values when ids wrapped.
determine when we assume that a nodeid is a unique identifier among current nodes.

The last one is what happens all over the place, and is fine as long as we ensure that the values really are unique. To do so, we can either:

maintain a set of currently used nodeids, and make sure that new values aren’t in use before assigning a new one
disconnect all nodes when we hit max, forcing new connections starting from 0.

Obviously the first is cleaner, and would probably end up being simpler too.

fanquake added the label RPC/REST/ZMQ on Apr 3, 2017

theuni commented at 3:05 am on April 4, 2017: member

See https://github.com/theuni/bitcoin/commit/57ea0cc55e0368466c32f3fd272e48504ab2871d for the wrapping/defining changes. Still need to verify the first two points above, though. @jnewbery If you don’t object, I’ll go ahead and PR that as a prerequisite for this one.

in src/rpc/net.cpp:253 in 54695963da outdated

247@@ -248,7 +248,16 @@ UniValue disconnectnode(const JSONRPCRequest& request)
248     if(!g_connman)
249         throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
250 
251-    bool ret = g_connman->DisconnectNode(request.params[0].get_str());
252+    std::string node = (request.params[0].get_str());
253+    bool ret;
254+    if (std::all_of(node.begin(), node.end(), ::isdigit)) {

laanwj commented at 6:48 am on April 4, 2017:

Concept ACK, but please make this API explicit.

APIs that switch based on magic heuristics on how a value ’looks’. Initially this seems user friendly, but it will quickly grow to maintainable, insecure monsters of obscure rules. It should be made completely explicit.
Also: do not encode integers as strings and use functions like std::stoi. Just use .get_int() if you need an int value and leave the encoding of values up to the RPC mechanism.

The most straightforward way would be to just add a function disconnectnodebyid.

jnewbery commented at 9:35 pm on April 4, 2017:

Yes, I agree that APIs shouldn’t be magic. I thought it wouldn’t be too problematic here because there can’t be any ambiguity at all between nodeIds and IP addresses (and disconnectnode() is already a bit magic - it can take either IPv4 or IPv6 addresses).

I don’t like the proliferation of additional RPCs that perform a very small function if we can avoid it.

How about we add a new argument to this RPC called “nodeId”? The RPC can be called with strictly one of the arguments. There’s no magic or ambiguity, and the node disconnection logic is contained within one RPC.

laanwj commented at 9:21 am on April 5, 2017:

How about we add a new argument to this RPC called “nodeId”? The RPC can be called with strictly one of the arguments. There’s no magic or ambiguity, and the node disconnection logic is contained within one RPC.

Sounds reasonable to me. To be clear: anything that doesn’t encode the integer as a string is more reasonable to me than this (another reason: because it allows passing the ‘id’ value literally without having to string-encode it in the client). I don’t have a strong opinion whether it should be one or multiple RPCs.

laanwj commented at 6:54 am on April 4, 2017: member

Concept ACK. Though if we’re going to be exposing this value to our apis, we need to define it sanely first.

This value is already exposed in APIs since getpeerinfo added {"id": 14669}. Handling wrapping sanely is important, but I don’t think this is a blocker for more usage on the RPC API. Client applications should treat these numbers as opaque identifiers and the only thing they can rely on is that that an ID won’t be reused for a different node within a very short timespan.

TheBlueMatt commented at 9:30 am on April 4, 2017: member

Found myself wanting this for disconnect after reading debug.log many times, annoying to have to getpeerinfo for it.

jnewbery commented at 9:38 pm on April 4, 2017: member

@theuni - of course, no objection at all!

sipa commented at 10:29 pm on April 4, 2017: member

there can’t be any ambiguity at all between nodeIds and IP addresses

Well you can write an IPv4 address as a decimal integer… I guess we’re excluding that (very uncommon) usage?

laanwj commented at 9:23 am on April 5, 2017: member

Well you can write an IPv4 address as a decimal integer… I guess we’re excluding that (very uncommon) usage?

+1. Exactly. This is exactly why these things get ugly. There’s always overlaps in representations that you may not be thinking about in the initial design, then get realized later. Let’s just take the sane route here and make the ambiguity impossible.

jnewbery commented at 2:09 pm on April 5, 2017: member

Let’s just take the sane route here and make the ambiguity impossible.

Yep, fine.

jnewbery commented at 2:15 pm on April 5, 2017: member

ok, another idea. I think it’s reasonable that people might want to update node by id in other ways (ie switching whitelisting on and off, changing ban score, etc). How would you feel about an updatenode RPC to do those things? To begin with it will allow disconnecting and turning whitelisting on/off, and it can be added to later if people think further functionality is useful.

I already have a branch with an updatenode RPC here: https://github.com/jnewbery/bitcoin/commit/5bbe24fd7597237388b73017036f5fe18c8470a6 . Adding a ‘disconnect’ argument to it is trivial.

We should keep this RPC hidden in order to not commit ourselves to a public API that we might want to change later (eg if we break out whitelisting into more granular behaviour at some point).

Thoughts?

jnewbery commented at 9:52 pm on April 5, 2017: member

I’ve added an updatepeer RPC here: #10160 which I think is better and more extensible, but I’d be interested to hear others’ thoughts.

laanwj commented at 3:02 pm on April 7, 2017: member

I don’t think ‘updatepeer X disconnect’ is clearer than ‘disconnectnode’. Having the RPC call as the verb is easier to use and remember.

We have an example of a very bad multiplexed call in the API: addnode X remove. Your proposal isn’t half as crazy as that :) And the idea of having a updatenode makes sense for changing other per-node variables, certainly for testing/debugging, but I don’t think it’s a good place to place disconnect.

jnewbery commented at 4:13 pm on April 7, 2017: member

Yes, addnode X remove is nuts, and I agree that having the RPC call as the verb is sensible.

I’ll reimplement this as an additional argument to disconnectnode.

Longer term, my thinking around updatepeer is that it’d be handy as a swiss army knife for peer interop. We may wish to disconnect/ban/whitelist/change other attributes/etc for peers, and having a single RPC that does all of that would be a sensible API, and certainly better than a different RPC for each attribute. On the other hand, I do agree that ‘disconnect’ is a verb, so naturally updatenode disconnect is a bit weird.

jnewbery force-pushed on Apr 7, 2017

jnewbery commented at 8:55 pm on April 7, 2017: member

I’ve pushed a version where disconnectnode now takes two arguments: address and nodeid. Strictly one argument must be given. (note: with named arguments, you can just supply a single argument, with positional arguments you need to set ‘address’ to the empty string if you want to use ’nodeid’).

I changed the name of the existing argument from ’node’ to ‘address’, since it only takes address. I think that’s the right thing to do since the current name is misleading, but we’ll need release notes to document the API change.

This should only be merged after @theuni’s branch here: https://github.com/theuni/bitcoin/commit/57ea0cc55e0368466c32f3fd272e48504ab2871d.

in src/rpc/net.cpp:239 in 3ed1bef2b9 outdated

233@@ -234,12 +234,15 @@ UniValue addnode(const JSONRPCRequest& request)
234 
235 UniValue disconnectnode(const JSONRPCRequest& request)
236 {
237-    if (request.fHelp || request.params.size() != 1)
238+    if (request.fHelp || request.params.size() >= 3)
239         throw std::runtime_error(
240             "disconnectnode \"node\" \n"

luke-jr commented at 2:05 pm on April 8, 2017:

Second argument is missing here

jnewbery commented at 1:17 pm on April 10, 2017:

Thanks. Help text now fixed. I’ll squash the commit when this is ready for merge.

luke-jr commented at 2:07 pm on April 8, 2017: member

While you can write an IP as a decimal number, you can’t specify an IP plus port in such a manner. Makes perfect sense to say String = IP and Number = nodeid IMO. Don’t care too strongly if the current (two arguments) approach is used, but I wouldn’t call it sane. :p

jnewbery commented at 1:14 pm on April 10, 2017: member

@luke-jr thanks for the review. I agree that the two arguments approach looks a bit odd when used as positional arguments, but it makes perfect sense when using named arguments: call the RPC with address= if you want to disconnect by address, or call with nodeid= if you want to disconnect by nodeid. The fact that it works at all with positional arguments is more a historical quirk of bitcoin’s RPC rather than a feature.

I think any of the three approaches I’ve implemented are fine (overloading one argument, having a second argument or having a separate ‘update peer’ RPC). I really just want the functionality somewhere, and I’m happy to go along with the consensus view for where the best place to put it is.

gmaxwell commented at 8:53 am on April 11, 2017: contributor

(aside, the addnode thing isn’t that nuts if you thing of it as a shortening of “addednodelist ”, which is what it actually is :) )

jnewbery commented at 1:07 pm on April 11, 2017: member

aside, the addnode thing isn’t that nuts if …

Yes, it makes sense if you’re a scholar of Bitcoin Core code archaeology :)

It’d be nice if addnode could be renamed to trusted peer or persistent peer, but that’s a lot of work (and a change to public APIs).

in src/rpc/net.cpp:630 in c070d5db5a outdated

628@@ -607,7 +629,7 @@ static const CRPCCommand commands[] =
629     { "network",            "ping",                   &ping,                   true,  {} },
630     { "network",            "getpeerinfo",            &getpeerinfo,            true,  {} },
631     { "network",            "addnode",                &addnode,                true,  {"node","command"} },
632-    { "network",            "disconnectnode",         &disconnectnode,         true,  {"node"} },
633+    { "network",            "disconnectnode",         &disconnectnode,         true,  {"address", "nodeid"} },

MarcoFalke commented at 8:39 am on April 13, 2017:

This is yet another breaking change. Needs rationale and mention in the release notes. Maybe backport.

0JSONRPCException: Unknown named parameter address (-8)

jnewbery commented at 12:48 pm on April 13, 2017: member

@MarcoFalke indeed. See my earlier comment:

I changed the name of the existing argument from ’node’ to ‘address’, since it only takes address. I think that’s the right thing to do since the current name is misleading, but we’ll need release notes to document the API change.

The fact that this argument is currently called ’node’ and the help text say (string, required) The node (see getpeerinfo for nodes) is misleading at best. It currently only accepts a string which must match on the CNode::addrName, which is the IP address/port.

jnewbery force-pushed on Apr 13, 2017

jnewbery commented at 3:36 pm on April 13, 2017: member

Rebased and squashed, with a couple of code cleanups.

#10176 is merged so this is now ready for review.

MarcoFalke commented at 7:13 pm on April 13, 2017: member

@jnewbery Yeah sorry for missing that. During my review flow I look at the code first, then read the comments before sending the ACK.

Generally I think it makes sense to keep breaking changes at a minimum or at least don’t splatter them across consecutive releases. As we already have such a breaking change in 0.14.1 it makes sense to bundle this one in as well.

Would you mind to create a minimal fix (similar to #10084) and tag that for backport? Not sure if it can into 0.14.1 at this stage, but having the pull open can not hurt.

jnewbery force-pushed on Apr 17, 2017

jnewbery commented at 1:39 pm on April 17, 2017: member

rebased

in src/rpc/net.cpp:243 in 569e5bb5a0 outdated

240-            "disconnectnode \"address\" \n"
241+            "disconnectnode \"address\" [nodeid]\n"
242             "\nImmediately disconnects from the specified node.\n"
243             "\nArguments:\n"
244             "1. \"address\"     (string, required) The IP address/port of the node\n"
245+            "2. \"nodeid\"   (string, required) The node ID(see getpeerinfo for nodes)\n"

laanwj commented at 2:11 pm on April 18, 2017:

Please mention in the documentation that only one of either can be provided, and the other one needs to be null (or missing, in case of the second argument, I guess). Also I’d prefer a space after ID before ( otherwise it looks like a parametrized something

jnewbery force-pushed on Apr 18, 2017

jnewbery commented at 3:56 pm on April 18, 2017: member

fixed @laanwj’s review comment.

sdaftuar commented at 4:26 pm on April 18, 2017: member

ACK 18718846c9ab7b7b4392912a14e1a64e9136e970

in src/rpc/net.cpp:264 in 18718846c9 outdated

265+    if (request.params[0].isStr()) {
266+        address = request.params[0].get_str();
267+    }
268+
269+    if (request.params.size() == 1) {
270+        ret = g_connman->DisconnectNode(request.params[0].get_str());

luke-jr commented at 1:40 am on April 19, 2017:

Use address here?

in src/rpc/net.cpp:263 in 18718846c9 outdated

264+
265+    if (request.params[0].isStr()) {
266+        address = request.params[0].get_str();
267+    }
268+
269+    if (request.params.size() == 1) {

luke-jr commented at 1:41 am on April 19, 2017:

What about disconnectnode("address", null)?

laanwj commented at 8:12 am on April 19, 2017:

Right, I also wondered about that. My recommendation, also for readability, would be to structure this symmetrically e.g.

 0const UniValue &address_arg = request.params[0];
 1const UniValue &id_arg = request.params.size() < 2 ? NullUniValue : request.params[1];
 2...
 3if (!address_arg.IsNull() && id_arg.IsNull()) {
 4    /* handle kick-by-address */
 5    std::string address = address_arg.get_str();
 6} else if (!id_arg.IsNull() && address_arg.IsNull()) {
 7    /* handle kick-by-id */
 8    NodeId nodeid = (NodeId) id_arg.get_int64();
 9} else {
10    throw JSONRPCError(RPC_INVALID_PARAMS, "Only one of adress and nodeid should be provided.");
11}

laanwj commented at 8:19 am on April 19, 2017:

General advice when designing new RPC APIs: please try to treat IsNull arguments the same as missing arguments, both in the middle as at the end. I know a lot of the current RPCs don’t heed that advice, but that’s something that needs to be improved to prevent unexpected behavior when switching to using named arguments.

jnewbery commented at 2:50 pm on April 19, 2017:

Thanks @laanwj. That’s good advice. I’ve rewritten this function based on your suggested structure.

in src/rpc/net.cpp:270 in 18718846c9 outdated

271+    } else {
272+        if (!address.empty()) {
273+            throw JSONRPCError(RPC_INVALID_PARAMS, "Only one of adress and nodeid should be provided.");
274+        }
275+
276+        NodeId nodeid = (NodeId) request.params[1].get_int();

luke-jr commented at 1:42 am on April 19, 2017:

get_int64

luke-jr changes_requested

luke-jr commented at 1:45 am on April 19, 2017: member

@jnewbery I don’t consider positional arguments to be merely historical.

jnewbery force-pushed on Apr 19, 2017

jnewbery commented at 2:51 pm on April 19, 2017: member

Rewritten using @laanwj’s suggested structure.

in src/rpc/net.cpp:239 in 6af6bdf957 outdated

233@@ -234,23 +234,43 @@ UniValue addnode(const JSONRPCRequest& request)
234 
235 UniValue disconnectnode(const JSONRPCRequest& request)
236 {
237-    if (request.fHelp || request.params.size() != 1)
238+    if (request.fHelp || request.params.size() == 0 || request.params.size() >= 3)
239         throw std::runtime_error(
240-            "disconnectnode \"address\" \n"
241-            "\nImmediately disconnects from the specified node.\n"
242+            "disconnectnode \"address\" [nodeid]\n"

luke-jr commented at 3:42 pm on April 19, 2017:

This doesn’t convey that address is also optional.

in src/rpc/net.cpp:245 in 6af6bdf957 outdated

244+            "\nStrictly one out of 'address' and 'nodeid' can be provided to identify the node.\n"
245+            "\nTo disconnect by nodeid, either set 'address' to the empty string, or call using the named 'nodeid' argument only.\n"
246             "\nArguments:\n"
247-            "1. \"address\"     (string, required) The IP address/port of the node\n"
248+            "1. \"address\"     (string, optional) The IP address/port of the node\n"
249+            "2. \"nodeid\"      (int, optional) The node ID (see getpeerinfo for node IDs)\n"

luke-jr commented at 3:43 pm on April 19, 2017:

s/int/number/

JSON doesn’t have ints.

luke-jr approved

luke-jr commented at 3:46 pm on April 19, 2017: member

Looking better, just a few nits.

Allow disconnectnode() to be called with node id

disconnectnode() can currently only be called with the IP address/port
of the node the user wishes to connect. This commit allows the node to
be disconnected using the nodeid returned by getpeerinfo().

23e6e64a24

[tests] fix nodehandling.py flake8 warnings d6564a26f4

[tests] rename nodehandling to disconnectban e367ad5b44

[tests] disconnectban test - only use two nodes 395561becf

[tests] disconnect_ban: add logging 2077fdacd3

[tests] disconnect_ban: use wait_until instead of sleep 12de2f252c

[tests] disconnect_ban: remove dependency on urllib 5cc3ee24d2

[tests] disconnect_ban: add tests for disconnect-by-nodeid d54297f1a8

jnewbery force-pushed on Apr 19, 2017

jnewbery commented at 5:50 pm on April 19, 2017: member

Thanks @luke-jr . Nits addressed.

I’ve added tests for the new functionality in the nodehandling.py test script (and renamed it to disconnect_ban.py). Whilst I was there I tidied up the test script, reduced the dependencies on standard libraries, and reduced the runtime from 20s to 9s on my pc.

Happy to split the test script changes out into a new PR if that’s preferable for people.

luke-jr approved

laanwj commented at 9:47 am on April 20, 2017: member

utACK d54297f

laanwj merged this on Apr 20, 2017

laanwj closed this on Apr 20, 2017

laanwj referenced this in commit a987def4f6 on Apr 20, 2017

jnewbery commented at 1:26 pm on April 20, 2017: member

@laanwj thanks for merging (and thanks for the suggestion of structuring the disconnectnode code differently). In future, feel free to squash my test commits before merging, or ask me to squash. I feel like micro commits are helpful for reviewers, but don’t necessarily need to be in the history.

laanwj commented at 6:23 pm on April 20, 2017: member

@jnewbery Yes, that would have made sense. I try to keep some pace in merging things once they’re ready to merge, as there are so many PRs held up on one thing or another.

luke-jr referenced this in commit c4bd214940 on Apr 21, 2017

luke-jr referenced this in commit 2fc4ea758b on Apr 21, 2017

luke-jr referenced this in commit 7465247e38 on Apr 21, 2017

luke-jr referenced this in commit 416268d69c on Apr 21, 2017

luke-jr referenced this in commit 5765843fdb on Apr 21, 2017

luke-jr referenced this in commit 2e650e0488 on Apr 21, 2017

jnewbery deleted the branch on Apr 25, 2017

luke-jr referenced this in commit 669772210d on Jun 3, 2017

luke-jr referenced this in commit 4c81b641f6 on Jun 3, 2017

luke-jr referenced this in commit c82de33fa0 on Jun 3, 2017

luke-jr referenced this in commit 13f9a4403f on Jun 3, 2017

luke-jr referenced this in commit 35c1055115 on Jun 3, 2017

luke-jr referenced this in commit 76830db3e8 on Jun 3, 2017

luke-jr referenced this in commit 96666af9ec on Jun 3, 2017

luke-jr referenced this in commit 2894dbb9dd on Jun 3, 2017

luke-jr referenced this in commit 014ff584c0 on Jun 3, 2017

luke-jr referenced this in commit ba167c669b on Jun 3, 2017

luke-jr referenced this in commit c132d44ccb on Jun 5, 2017

luke-jr referenced this in commit 104f9593b8 on Jun 5, 2017

luke-jr referenced this in commit 3eec7d4ee5 on Jun 5, 2017

luke-jr referenced this in commit 641649f7c9 on Jun 5, 2017

luke-jr referenced this in commit 54469cbdbf on Jun 5, 2017

luke-jr referenced this in commit 5bc75bb8ee on Jun 5, 2017

luke-jr referenced this in commit bfd1cf6713 on Jun 5, 2017

luke-jr referenced this in commit e9dabd0a71 on Jun 5, 2017

luke-jr referenced this in commit cc1e56e9cb on Jun 5, 2017

luke-jr referenced this in commit f8f08c46fd on Jun 5, 2017

luke-jr referenced this in commit 28734848dd on Jun 5, 2017

luke-jr referenced this in commit 98bd0c338b on Jun 5, 2017

luke-jr referenced this in commit 04226938a3 on Jun 5, 2017

luke-jr referenced this in commit e05799a381 on Jun 5, 2017

luke-jr referenced this in commit 7317c1b087 on Jun 15, 2017

luke-jr referenced this in commit 96841d9f1c on Jun 15, 2017

nomnombtc referenced this in commit 2175410a31 on Jul 17, 2017

nomnombtc referenced this in commit bb1127061c on Jul 17, 2017

nomnombtc referenced this in commit 094834b427 on Jul 17, 2017

nomnombtc referenced this in commit ea7c707979 on Jul 17, 2017

PastaPastaPasta referenced this in commit 3ec5a9966a on May 21, 2019

PastaPastaPasta referenced this in commit 08e58be440 on May 21, 2019

PastaPastaPasta referenced this in commit cb554d0d2c on May 22, 2019

PastaPastaPasta referenced this in commit c984626e34 on May 22, 2019

PastaPastaPasta referenced this in commit b4a49628a3 on May 22, 2019

PastaPastaPasta referenced this in commit 2a43f92284 on May 22, 2019

PastaPastaPasta referenced this in commit 6f558270d4 on May 23, 2019

UdjinM6 referenced this in commit f170a3ff26 on May 28, 2019

barrystyle referenced this in commit 3d42499bf7 on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021

[net] Allow disconnectnode RPC to be called with node id #10143