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

[jnewbery]

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]

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]

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]

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

[theuni]

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.

[theuni]

See theuni/bitcoin@57ea0cc 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.

[laanwj]

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]

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]

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]

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]

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

[jnewbery]

@theuni - of course, no objection at all!

[sipa]

 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]

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]

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

Yep, fine.

[jnewbery]

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: jnewbery/bitcoin@5bbe24f . 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]

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]

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]

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]

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: theuni/bitcoin@57ea0cc.

[luke-jr]

Second argument is missing here

[jnewbery]

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

[luke-jr]

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]

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

(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]

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

[MarcoFalke]

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

JSONRPCException: Unknown named parameter address (-8)

[jnewbery]

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

Rebased and squashed, with a couple of code cleanups.

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

[MarcoFalke]

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

rebased

[laanwj]

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]

fixed @laanwj's review comment.

[sdaftuar]

ACK 1871884

[luke-jr]

What about disconnectnode("address", null)?

[laanwj]

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

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

[laanwj]

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]

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

[luke-jr]

Use address here?

[luke-jr]

get_int64

[luke-jr]

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

[jnewbery]

Rewritten using @laanwj's suggested structure.

[luke-jr]

Looking better, just a few nits.

[luke-jr]

This doesn't convey that address is also optional.

[luke-jr]

s/int/number/

JSON doesn't have ints.

[jnewbery]

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.

[laanwj]

utACK d54297f

[jnewbery]

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

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