Page MenuHomePhabricator

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

Authored by deadalnix on Aug 27 2017, 20:13.

Details

Summary

backport of PR10143 from core + a few bugfixes.

Test Plan
../qa/pull-tester/rpc-tests.py disconnect_ban.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Added some usability suggestions.

src/rpc/net.cpp
268 ↗(On Diff #1244)

The above wording implies that:

disconnectnode 1

Should work.. but the code below only supports:

disconnectnode "" 1

Suggest: making the code actually support

disconnectnode 1
295 ↗(On Diff #1244)

Suggest: adding logic here to support trying to interpret request.params[0] as a node id if it is strictly a number and not an address. (See my other comment above)

src/rpc/net.cpp
268 ↗(On Diff #1244)

Seems like a good suggestion.

src/rpc/net.cpp
268 ↗(On Diff #1244)

We can't really do that as there is no way to figure out which is which.

src/rpc/net.cpp
268 ↗(On Diff #1244)

Oh really? So that's not a computable problem at all and is one of those things that baffles computer science? Well then it that's the case sign me up for a nobel prize because I have a solution. :p

Suggest: if it contains non-numeric characters, it's an address. If not, it's a nodeid. Use the C90 standard function

isdigit()

in a loop, if you must.

Granted, that assumes network addresses will never be simply numeric. But since we only support IPv4, IPv6, and tor networks, that's an ok assumption. If some day networks evolve to which addresses are just raw numbers: In that case, users can also do "" nodeid as a 'workaround'.

src/rpc/net.cpp
268 ↗(On Diff #1244)

I don't think it is a good idea to second guess what it given and I don't think it is a good idea to have a different API than core has unless there is a very good reason to.

Yes you can make a best guess as to what the user intended. No this isn't a good idea.

CCulianu added inline comments.
src/rpc/net.cpp
268 ↗(On Diff #1244)

OK well the only argument in the above I agree with is that maintaining API compat. with core is a good thing.

Otherwise the API is a bit silly. Passing "" 123... :/

But if Core's doing it, we should as well. I didn't realize Core also had this silly API -- I thought it was something we added as a "nicety".

I'll approve now.

This revision is now accepted and ready to land.Aug 29 2017, 11:08
src/rpc/net.cpp
268 ↗(On Diff #1244)

Yes i agree that the API is a bit silly. However, the API is very lax in general (we saw that on another diff, where the amount could be a string or a number). Doing second guessing in these condition is dangerous IMO.

295 ↗(On Diff #1244)

As things goes along I do think it make sense to straighten the API instead of doing all kind of second guesses.

IMO the best here should be to take a sting plus a another argument. The string would be "ip" or "nodeid". Obviously, that's breaking the API, so that would need to be considered carefully.

This revision was automatically updated to reflect the committed changes.