rpc, p2p: allow `disconnectnode` with subnet #26576

Since disconnectnode function allows to be used with a subnet, e.g.: https://github.com/bitcoin/bitcoin/blob/38d06e1561013f4ca845fd5ba6ffcc64de67f9c0/src/net.h#L826-L829 https://github.com/bitcoin/bitcoin/blob/38d06e1561013f4ca845fd5ba6ffcc64de67f9c0/src/net.cpp#L2589-L2601

This PR adds support for subnet in address parameter in disconnectnode RPC. When passing a string with / in address, it is gonna recognize it as a subnet.. It allows us to disconnect multiple nodes from all ports on the same IP. The only way to do it before was banning them for 1sec which seems a bad UX.

Also, when there are multiples nodes connected to a node in a functional test and we want to disconnect them all, we could call disconnectnode with a subnet instead of calling disconnect_nodes multiple times. A simple example:

-        for i in range(1, 6):
-            self.disconnect_nodes(i, 0)
-
+        self.nodes[0].disconnectnode(address="127.0.0.1/24")
+        self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 0, timeout=10)

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26863 (test: merge banning test from p2p_disconnect_ban to rpc_setban by brunoerg)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This changes the behavior of disconnectnode to potentially disconnect multiple nodes. It's unclear to me what the use case is for this change. When would we want to disconnect nodes by subnet instead of passing in address/port?

This changes the behavior of disconnectnode to potentially disconnect multiple nodes. It's unclear to me what the use case is for this change. When would we want to disconnect nodes by subnet instead of passing in address/port?

I think this is the same logic for setban, banning or disconnecing by subnet is a good way to ban multiple nodes from a range of addresses, or when we want to disconnect from nodes running at the same addr but different ports (obs: passing 127.0.0.1 to address in disconnectnodes don't disconnect the node 127.0.0.1:8333, you must specify the port, that's one of the reasons subnet can be useful.

E.g.:

diff --git a/test/functional/p2p_disconnect_ban.py b/test/functional/p2p_disconnect_ban.py
index 0acb5abac..f90a14cdb 100755
--- a/test/functional/p2p_disconnect_ban.py
+++ b/test/functional/p2p_disconnect_ban.py
@@ -13,7 +13,7 @@ from test_framework.util import (
 
 class DisconnectBanTest(BitcoinTestFramework):
     def set_test_params(self):
-        self.num_nodes = 2
+        self.num_nodes = 3
         self.supports_cli = False
 
     def run_test(self):
@@ -27,7 +27,7 @@ class DisconnectBanTest(BitcoinTestFramework):
         self.log.info("Test setban and listbanned RPCs")
 
         self.log.info("setban: successfully ban single IP address")
-        assert_equal(len(self.nodes[1].getpeerinfo()), 2)  # node1 should have 2 connections to node0 at this point
+        assert_equal(len(self.nodes[1].getpeerinfo()), 3)  # node1 should have 2 connections to node0 at this point
         self.nodes[1].setban(subnet="127.0.0.1", command="add")
         self.wait_until(lambda: len(self.nodes[1].getpeerinfo()) == 0, timeout=10)
         assert_equal(len(self.nodes[1].getpeerinfo()), 0)  # all nodes must be disconnected at this point
@@ -123,6 +123,7 @@ class DisconnectBanTest(BitcoinTestFramework):
         assert not [node for node in self.nodes[0].getpeerinfo() if node['id'] == id1]
 
         self.log.info("disconnectnode: successfully disconnect node by subnet")
+        self.connect_nodes(0, 2)
         self.nodes[0].disconnectnode(address='127.0.0.1/24')
         self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 0, timeout=10)

~0 I don't really see the use case here. What would be the reason for disconnecting by subnet besides banning spammy peers? In that case setban seems more appropriate.

This also seems out of scope for the disconnectnode rpc as it is meant (judging by the name and api right now) to disconnect a single node.

Concept ACK

What would be the reason for disconnecting by subnet besides banning spammy peers? In that case setban seems more appropriate.

Its about providing options. Alice could disconnect a subnet and Bob could could ban same subnet for 1 hour.

Alice could disconnect a subnet and Bob could could ban same subnet for 1 hour.

Alice could just ban for a couple seconds to achieve the same, no?

Alice could disconnect a subnet and Bob could could ban same subnet for 1 hour.

Alice could just ban for a couple seconds to achieve the same, no?

Yes. Although I am not aware if its possible using bitcoin-qt (GUI)

What would be the reason for disconnecting by subnet besides banning spammy peers? In that case setban seems more appropriate.

I use addnode a lot and I usually have lots of manual connections, so disconnecting by subnet could help in the case I am connected with nodes from the same ip but different ports and wanna disconnect them all (just a simple example, there are other cases).

@brunoerg you can already achieve that by using setban.

e.g.

bitcoin-cli setban <subnet> add 1

disconnects you from a subnet by banning it for 1 second.

disconnects you from a subnet by banning it for 1 second.

It seems a bad UX for me, if I want to disconnect, what comes to my mind is the disconnectnode command, seems weird to me to ban them for 1 sec when I just want to disconnect them.

Concept ACK

I would recommend updating the description with a more clear use case, as I also wasn't convinced this was a good change until I read through the discussion and saw your comments @brunoerg

This also seems out of scope for the disconnectnode rpc as it is meant (judging by the name and api right now) to disconnect a single node.

Counter argument: as mentioned in the description, DisconnectNode supports subnet as an argument:

https://github.com/bitcoin/bitcoin/blob/38d06e1561013f4ca845fd5ba6ffcc64de67f9c0/src/net.h#L826-L829

so why not make the RPC expose the full functionality? I think I'd be more annoyed if I saw that DisconnectNode supported subnet in the code, but the RPC wouldn't let me use it.

I think disconnecting from all ports on the same IP seems like a compelling enough use case for a very small change.

EDIT:

(just a simple example, there are other cases).

It would be great to see some of these listed in the description as well

@josibake thank you, I just updated the description.

so why not make the RPC expose the full functionality? I think I'd be more annoyed if I saw that DisconnectNode supported subnet in the code, but the RPC wouldn't let me use it.

I think disconnecting from all ports on the same IP seems like a compelling enough use case for a very small change.

Yea, I only opened this one because the change would be really small, and since DisconnectNode supports subnet, it might worth.

crACK https://github.com/bitcoin/bitcoin/pull/26576/commits/23f4c2cb452d25f61dada898d5cc4c74f72e0145

This PR does not seem to have conceptual support. Please leave a comment if you would like this to be reopened.

disconnects you from a subnet by banning it for 1 second.

It seems a bad UX for me, if I want to disconnect, what comes to my mind is the disconnectnode command, seems weird to me to ban them for 1 sec when I just want to disconnect them.

Maybe update the doc to mention the alternative command for this use case?

Maybe update the doc to mention the alternative command for this use case?

SGTM