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.

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.

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

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.

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

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.

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

@theuni - of course, no objection at all!

 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?

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.

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

Yep, fine.

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

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.

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.

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.

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

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

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

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.

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

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