Disconnect node on addnode remove #6259

pull Alex-van-der-Peet wants to merge 2 commits into bitcoin:master from Alex-van-der-Peet:AddNodeRemoveDisconnect changing 1 files +5 −0

Alex-van-der-Peet commented at 7:01 AM on June 9, 2015: contributor

On calling addnode <ip> remove, now disconnects from said node immediately per issue #2729. Requires the port to be specified in the IP parameter for it to be found and disconnected however.
Disconnect node on addnode remove b5974eb729
Alex-van-der-Peet cross-referenced this on Jun 9, 2015 from issue "bitcoind addnode IP remove" should disconnect immediately from the given node. by randy-waterhouse
jonasschnelli commented at 9:20 AM on June 9, 2015: contributor
Not against this.

But a pure disconnect won't prevent from a reconnect of the just kicked node.

I think the setban rpc command (#6158) would suit better for a node kick (kick and ban for 1h).

For this PR two things would be cool:
- ban disconnected nodes for 1h (or at least 10mins)
- allow removing nodes by IP without port (kick all nodes from the given IP)
in src/rpcnet.cpp:None in b5974eb729 outdated
```
 205 | @@ -206,6 +206,13 @@ UniValue addnode(const UniValue& params, bool fHelp)
 206 |      }
 207 |      else if(strCommand == "remove")
 208 |      {
 209 | +        CNode* pNode = FindNode(strNode.c_str());
 210 | +
 211 | +        if (pNode != NULL)
```
jonasschnelli commented at 11:57 AM on June 9, 2015:

F.I.Y.: most one-line-ifs in the source codes are without brackets.

Alex-van-der-Peet commented at 12:50 PM on June 9, 2015:

You're absolutely right, missed some clean up there, my bad.
laanwj added the label RPC on Jun 9, 2015
Bracket cleanup 8e7f85128d
luke-jr commented at 6:39 PM on June 9, 2015: member

I don't think it makes sense to disconnect from a peer merely because you remove it from the "persistent" addnode list...
laanwj commented at 8:57 AM on June 10, 2015: member

Thanks for taking a stab at implementing this, concept ACK. @luke-jr Agreed. This is useful functionality (now available in the GUI but not on RPC), however this is the wrong place. Let's just add a disconnectnode RPC.
jgarzik commented at 1:06 PM on June 10, 2015: contributor

+1 @laanwj RE "disconnectnode"

It is also odd that an "addnode" RPC actually performs the opposite of an "add"
Alex-van-der-Peet commented at 1:58 PM on June 10, 2015: contributor

@jgarzik Yeh that was my comment on the original issue too, but I thought what the hey, here's an issue I can actually take care of, I'll give it a shot.

Will take a look at adding disconnectnode later this week, leave it with me.
franko-org commented at 4:43 PM on June 10, 2015: none

disconnectnode imho makes more sense to me. Maybe even just a "node" call that takes specific commands. IE node ban, node add, node disconnect.
laanwj commented at 8:42 AM on June 11, 2015: member

@franko-org That would be possible, but e.g. with help it's handier to see what is available in one glance. I don't see an advantage to be particlularly thrifty with adding RPC calls. This gave us peculiar constructs like addnode remove in the first place :)

In retrospect, the RPC mechanism would have benefitted from namespacing (node.X, wallet.X etc) but doing that for one call is inconsistent.
Alex-van-der-Peet cross-referenced this on Jun 12, 2015 from issue New RPC command disconnectnode by Alex-van-der-Peet
laanwj commented at 2:40 PM on June 12, 2015: member

Closing in favor of #6271
laanwj closed this on Jun 12, 2015
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#2729 "bitcoind addnode IP remove" should disconnect immediately from the given node.#6158 Add setban/listbanned RPC commands #6271 New RPC command disconnectnode