Add new 'node' RPC command #341

Roasbeef · 2015-03-05T22:00:35Z

This PR adds new JSON-RPC command: node. node allows a node operator full control over the peers their node is connect to.
A call to the new node RPC can take the following forms:

btcctl node connect <addr> perm (will attempt connect to a peer and mark it was permanent)
btcctl node connect <addr> temp (will attempt to connect to a peer)
btcctl node disconnect <addr or ID> (will attempt to remove an outgoing or incoming peer)
btcctl node remove <addr or ID> (will attempt to remove a permanent peer)

This PR replaces #336.
This aims to fix #79 by adding a new command that can disconnect any (inbound or outbound) non-persistent peer.

davecgh · 2015-03-10T03:19:53Z

@dajohi Does this one address your concerns with the original PR?

dajohi · 2015-03-10T13:16:31Z

Now that getpeerinfo contains ID, disconnect should take an ID.

davecgh · 2015-03-10T15:55:02Z

@dajohi: I'd suggest it should take either address or ID.

The address already works with "addr:port" to be specific enough to identify a given peer. When "addr" with no port is provided, it appends the default port for the network and attempts to remove that node.

So, I think ideally the current logic would stay, but perhaps disconnect/remove should additionally allow the "addr" to be an ID. It would be a simple check in the code I think. First, check if it's a valid IP address (with optional port), and if so, use the current semantics. If not, attempt to parse it as an unsigned integer and, if successful, treat it as an ID. Otherwise, invalid address or ID error.

davecgh · 2015-03-10T15:57:06Z

btcjson/v2/btcjson/btcdextcmds.go

+type NodeCmd struct {
+	SubCmd        NodeSubCmd `jsonrpcusage:"\"connect|remove|disconnect\""`
+	Addr          string
+	ConnectSubCmd *string `jsonrpcusage:"\"perm|non-perm\""`


What's the default?

EDIT: Well I think your intent here is to force the caller to specify perm or non-perm when the connect sub command is provided. That is fine so long as the handler code detects the missing optional parameter and returns an error.

Yeah the intent is to require the caller to specify perm/non-perm. The handler code in rpcserver.go currently detects if the optional parameter is missing, and returns the appropriate error if so.

EDIT: non-perm is now temp

dajohi · 2015-03-10T17:57:18Z

Can we change non-perm to temp?
or maybe non-perm the default.. and the only option after ip:port is perm

dajohi · 2015-03-10T17:58:16Z

@davecgh If i have 2 connections open to ip:port, should disconnect ip:port disconnect the first, or all?

davecgh · 2015-03-10T18:07:42Z

I'd say all of them ideally.

Roasbeef · 2015-03-23T00:39:28Z

Recently pushed a commit changing the following:

node disconnect/remove can now operate on both ip:port or a valid node id.
For node connect, non-perm has been changed to temp, temp is now the default.

@dajohi by "2 connections", do you mean one inbound connection and outbound connection to the same ip:port?

dajohi · 2015-03-23T17:19:39Z

@Roasbeef No, I don't think that is possible. I am talking about > 1 outbound connections to same ip:port.

Roasbeef · 2015-03-24T21:47:18Z

node disconnect <target> now disconnects all matching targets rather than just the first one. I've also updated the command help and RPC documentation to reflect this new behavior.

davecgh · 2015-03-25T02:32:53Z

Thanks for the updates. I'll get the latest changes reviewed.

davecgh · 2015-04-01T22:12:46Z

A couple of things:

Calling node remove existing_id_that_is_non_perm results in -8: peer not found. While I understand that's because the node is not a permanent one and can't be removed, I think the error should be more descriptive there. Something like: "non-permanent peers can't be removed - use disconnect".

Not necessary for this PR, but food for thought:

I think it would made sense to have something like node info (connected|added) (defaults to connected) which returns the same information as getpeerinfo for connected peers, and getaddednodeinfo for added/perm peers. We'll want to keep getpeerinfo and getaddednodeinfo for compatibility, but it feels a little odd to do ./btcctl node connect/disconnect/remove but have to use /btcctl getpeerinfo and /btcctl getaddednodeinfo to see the results. Would like @dajohi feedback on this point.

davecgh · 2015-04-01T22:20:10Z

More testing and I ran into this:

$ ./btcctl node remove 10.1.2.3
-8: invalid address or node ID
$ ./btcctl node remove 10.1.2.3:8333
$

That is to say it didn't remove the node by IP without a port.

Also I noticed that there is no way to see the id for a perm peer that is not connected.

davecgh · 2015-04-01T22:36:42Z

Probably also related to the previous issue:

$ ./btcctl node connect 10.1.2.3 perm
$ ./btcctl getaddednodeinfo true
[
{
"addednode": "10.1.2.3:8333",
"connected": false,

}
]
$ ./btcctl node connect 10.1.2.3 perm
-8: peer already connected
$ ./btcctl node connect 10.1.2.3
$

I suspect that should error out because the node already exists as a perm peer even though it's not connected yet.

Roasbeef · 2015-04-02T00:54:13Z

Pushed some commits fixing the following:

error messages for remove/disconnect has been improved. The user is now pointed to the proper
sub-command if we detect they might have made a mistake discerning a peer as perm or temp.
A bug in parsing nodeId vs IP has been fixed. ip and ip:port should now work correctly for
remove/disconnect.
Trying to connect a peer that's already added as a permanent peer now returns a new error message.

Roasbeef · 2015-04-02T00:57:08Z

Regarding not being able to show the ID of a permanent peer that isn't currently connected.

It seems that this is the current implemented behavior of getpeerinfo, if the peer isn't connected, it's skipped:

    case getPeerInfoMsg:
        syncPeer := s.blockManager.SyncPeer()
        infos := make([]*btcjson.GetPeerInfoResult, 0, state.peers.Len())
        state.forAllPeers(func(p *peer) {
            if !p.Connected() {
                return
            }

davecgh · 2015-04-02T03:17:26Z

Correct, getpeerinfo only shows connected peers. What I was getting at is I think the getaddednodeinfo response needs to include the peer ID. That would of course then transfer over into the node info responses discussed above.

davecgh · 2015-04-14T19:07:41Z

I believe this is ready for merge. There are a few points brought up that can be improved upon, but what is there works properly and reads well.

Please squash and rebase it.

* Gives node operators full control of peer connectivity * RPC adds ability to disconnect all matching non-persistent peers, remove persistent peers, and connect to peers making them either temporary or persistent.

Roasbeef · 2015-04-15T23:28:39Z

Rebased and squashed 👍

tuxcanfly · 2015-04-22T11:14:46Z

server.go

@@ -475,16 +491,20 @@ func (s *server) handleQuery(querymsg interface{}, state *peerState) {
 		msg.reply <- infos

 	case addNodeMsg:
+	case connectNodeMsg:


I think this breaks the existing addNodeMsg, delNodeMsg cases because they are no longer being handled. It would probably make sense to keep these cases as they were for compatibility. If not, we could deprecate addnode RPC command with a note about using node instead.

Yeah you're right. Trying to use the addnode sub-commands, del or add causes the btcctl caller to hang indefinitely waiting for a reply since the case stacking essentially made the commands into a no-op.

I agree that we should mark addnode as deprecated and point users to the new node command instead. But for now, 've created as a temporary PR fix: #398

Roasbeef mentioned this pull request Mar 5, 2015

Add new 'dropnode' RPC command #336

Closed

Roasbeef force-pushed the node-cmd branch from a38bbac to 9aa8821 Compare March 5, 2015 22:02

Roasbeef changed the title ~~Add new 'dropnode' RPC command~~ Add new 'node' RPC command Mar 5, 2015

Roasbeef force-pushed the node-cmd branch from 9aa8821 to f6735a9 Compare March 5, 2015 22:08

davecgh reviewed Mar 10, 2015
View reviewed changes

Roasbeef force-pushed the node-cmd branch from f6735a9 to 3112a0c Compare March 23, 2015 00:11

Roasbeef force-pushed the node-cmd branch from c6ad198 to b8b8477 Compare March 24, 2015 21:46

davecgh mentioned this pull request Apr 13, 2015

need new rpc command .. addnode <ip> remove .. does not remove from dns list as well as persistent list #79

Closed

Fix btcsuite#79 by adding a new node JSON-RPC command

65b044e

* Gives node operators full control of peer connectivity * RPC adds ability to disconnect all matching non-persistent peers, remove persistent peers, and connect to peers making them either temporary or persistent.

Roasbeef force-pushed the node-cmd branch from 50d0569 to 65b044e Compare April 15, 2015 06:09

conformal-deploy merged commit 65b044e into btcsuite:master Apr 16, 2015

tuxcanfly reviewed Apr 22, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new 'node' RPC command #341

Add new 'node' RPC command #341

Roasbeef commented Mar 5, 2015

davecgh commented Mar 10, 2015

dajohi commented Mar 10, 2015

davecgh commented Mar 10, 2015

davecgh Mar 10, 2015

Roasbeef Mar 23, 2015

dajohi commented Mar 10, 2015

dajohi commented Mar 10, 2015

davecgh commented Mar 10, 2015

Roasbeef commented Mar 23, 2015

dajohi commented Mar 23, 2015

Roasbeef commented Mar 24, 2015

davecgh commented Mar 25, 2015

davecgh commented Apr 1, 2015

davecgh commented Apr 1, 2015

davecgh commented Apr 1, 2015

Roasbeef commented Apr 2, 2015

Roasbeef commented Apr 2, 2015

davecgh commented Apr 2, 2015

davecgh commented Apr 14, 2015

Roasbeef commented Apr 15, 2015

tuxcanfly Apr 22, 2015

Roasbeef Apr 22, 2015

Add new 'node' RPC command #341

Add new 'node' RPC command #341

Conversation

Roasbeef commented Mar 5, 2015

davecgh commented Mar 10, 2015

dajohi commented Mar 10, 2015

davecgh commented Mar 10, 2015

davecgh Mar 10, 2015

Choose a reason for hiding this comment

Roasbeef Mar 23, 2015

Choose a reason for hiding this comment

dajohi commented Mar 10, 2015

dajohi commented Mar 10, 2015

davecgh commented Mar 10, 2015

Roasbeef commented Mar 23, 2015

dajohi commented Mar 23, 2015

Roasbeef commented Mar 24, 2015

davecgh commented Mar 25, 2015

davecgh commented Apr 1, 2015

davecgh commented Apr 1, 2015

davecgh commented Apr 1, 2015

Roasbeef commented Apr 2, 2015

Roasbeef commented Apr 2, 2015

davecgh commented Apr 2, 2015

davecgh commented Apr 14, 2015

Roasbeef commented Apr 15, 2015

tuxcanfly Apr 22, 2015

Choose a reason for hiding this comment

Roasbeef Apr 22, 2015

Choose a reason for hiding this comment