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

brunoerg · 2022-11-25T16:07:47Z

Since disconnectnode function allows to be used with a subnet, e.g.:

Lines 826 to 829 in 38d06e1

    
           bool DisconnectNode(const std::string& node); 
        
           bool DisconnectNode(const CSubNet& subnet); 
        
           bool DisconnectNode(const CNetAddr& addr); 
        
           bool DisconnectNode(NodeId id);

bitcoin/src/net.cpp

Lines 2589 to 2601 in 38d06e1

    
           bool CConnman::DisconnectNode(const CSubNet& subnet) 
        
           { 
        
               bool disconnected = false; 
        
               LOCK(m_nodes_mutex); 
        
               for (CNode* pnode : m_nodes) { 
        
                   if (subnet.Match(pnode->addr)) { 
        
                       LogPrint(BCLog::NET, "disconnect by subnet%s matched peer=%d; disconnecting\n", (fLogIPs ? strprintf("=%s", subnet.ToString()) : ""), pnode->GetId()); 
        
                       pnode->fDisconnect = true; 
        
                       disconnected = true; 
        
                   } 
        
               } 
        
               return disconnected; 
        
           }

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)

DrahtBot · 2022-11-25T16:07:50Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	josibake
Concept ACK	ghost

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

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.

andrewtoth · 2022-11-25T16:45:46Z

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?

brunoerg · 2022-11-25T18:35:52Z

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)

dergoegge

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

doc/release-notes-26576.md

ghost · 2022-11-26T16:56:57Z

Concept ACK

ghost · 2022-11-26T16:59:19Z

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.

dergoegge · 2022-11-26T17:03:04Z

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?

ghost · 2022-11-26T17:05:59Z

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)

brunoerg · 2022-11-29T14:05:57Z

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

dergoegge · 2022-11-29T14:15:33Z

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

brunoerg · 2022-11-29T15:42:19Z

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.

josibake · 2023-05-03T10:58:24Z

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:

bitcoin/src/net.h

Lines 826 to 829 in 38d06e1

    
           bool DisconnectNode(const std::string& node); 
        
           bool DisconnectNode(const CSubNet& subnet); 
        
           bool DisconnectNode(const CNetAddr& addr); 
        
           bool DisconnectNode(NodeId id);

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

brunoerg · 2023-05-03T19:20:05Z

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

josibake

crACK 23f4c2c

josibake · 2023-05-04T07:54:50Z

src/rpc/net.cpp

                "\nTo disconnect by nodeid, either set 'address' to the empty string, or call using the named 'nodeid' argument only.\n",
                {
-                    {"address", RPCArg::Type::STR, RPCArg::DefaultHint{"fallback to nodeid"}, "The IP address/port of the node"},
+                    {"address", RPCArg::Type::STR, RPCArg::DefaultHint{"fallback to nodeid"}, "The IP address/port of the node or subnet"},


it would be great to add an example for subnets in the help section below.

josibake · 2023-05-04T07:58:15Z

doc/release-notes-26576.md

+Updated RPCs
+----------
+
+- RPC `disconnectnode` now accepts a subnet into `address`


The release notes could be a bit more detailed, maybe mentioning that this aligns DisconnectNode and the disconnectnode rpc. It's nice to have at least a summary of the motivation in the release notes.

maybe mentioning that this aligns DisconnectNode and the disconnectnode rpc.

Internal APIs really don't seem like they belong here

I don't think it's worth to mention this in a release note, seems like a specific code nuance to be exposed. Isn't it?

luke-jr · 2023-07-27T02:54:18Z

src/rpc/net.cpp

+        } else {
+            success = connman.DisconnectNode(address_arg.get_str());
+        }


Why not drop this entirely? Seems like a bug specifying an IP won't disconnect all nodes connecting from that IP...

In fact, maybe this should be split into a bugfix commit before adding subnet support.

I don't think it's really a bug. It seems like this was the expected behavior (due to examples and docs - e.g. having to specify port). However, I also think it should be able to disconnect all nodes from an IP. I'm gonna address it.

achow101 · 2023-09-20T17:15:40Z

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

maflcko · 2023-09-20T17:16:19Z

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?

brunoerg · 2023-09-21T09:01:09Z

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

SGTM

Github-Pull: bitcoin#26576 Rebased-From: b2e3edc

Github-Pull: bitcoin#26576 Rebased-From: ad39ede

brunoerg added 2 commits November 25, 2022 11:20

rpc, p2p: allow disconnectnode with subnet

b2e3edc

test: add coverage for subnet in disconnectnode RPC

ad39ede

dergoegge reviewed Nov 26, 2022

View reviewed changes

doc/release-notes-26576.md Outdated Show resolved Hide resolved

doc: add release note for 26576

23f4c2c

brunoerg force-pushed the 2022-11-disconnectnode-subnet branch from fcd014a to 23f4c2c Compare November 28, 2022 12:06

brunoerg marked this pull request as ready for review November 29, 2022 15:42

DrahtBot mentioned this pull request Jan 10, 2023

test: merge banning test from p2p_disconnect_ban to rpc_setban #26863

Closed

achow101 requested review from josibake and maflcko April 25, 2023 15:34

maflcko removed their request for review April 27, 2023 08:30

josibake reviewed May 4, 2023

View reviewed changes

luke-jr suggested changes Jul 27, 2023

View reviewed changes

achow101 closed this Sep 20, 2023

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2024

rpc, p2p: allow disconnectnode with subnet

6d2bc57

Github-Pull: bitcoin#26576 Rebased-From: b2e3edc

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2024

test: add coverage for subnet in disconnectnode RPC

dc36f2b

Github-Pull: bitcoin#26576 Rebased-From: ad39ede

bitcoin locked and limited conversation to collaborators Sep 20, 2024

	bool DisconnectNode(const std::string& node);
	bool DisconnectNode(const CSubNet& subnet);
	bool DisconnectNode(const CNetAddr& addr);
	bool DisconnectNode(NodeId id);

	bool CConnman::DisconnectNode(const CSubNet& subnet)
	{
	bool disconnected = false;
	LOCK(m_nodes_mutex);
	for (CNode* pnode : m_nodes) {
	if (subnet.Match(pnode->addr)) {
	LogPrint(BCLog::NET, "disconnect by subnet%s matched peer=%d; disconnecting\n", (fLogIPs ? strprintf("=%s", subnet.ToString()) : ""), pnode->GetId());
	pnode->fDisconnect = true;
	disconnected = true;
	}
	}
	return disconnected;
	}

rpc, p2p: allow disconnectnode with subnet #26576

rpc, p2p: allow disconnectnode with subnet #26576

Uh oh!

Conversation

brunoerg commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

andrewtoth commented Nov 25, 2022

Uh oh!

brunoerg commented Nov 25, 2022

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Nov 26, 2022

Uh oh!

ghost commented Nov 26, 2022

Uh oh!

dergoegge commented Nov 26, 2022

Uh oh!

ghost commented Nov 26, 2022

Uh oh!

brunoerg commented Nov 29, 2022

Uh oh!

dergoegge commented Nov 29, 2022

Uh oh!

brunoerg commented Nov 29, 2022

Uh oh!

josibake commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brunoerg commented May 3, 2023

Uh oh!

josibake left a comment

Choose a reason for hiding this comment

Uh oh!

josibake May 4, 2023

Choose a reason for hiding this comment

Uh oh!

josibake May 4, 2023

Choose a reason for hiding this comment

Uh oh!

luke-jr Jul 27, 2023

Choose a reason for hiding this comment

Uh oh!

brunoerg Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

luke-jr Jul 27, 2023

Choose a reason for hiding this comment

Uh oh!

luke-jr Jul 27, 2023

Choose a reason for hiding this comment

Uh oh!

brunoerg Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 commented Sep 20, 2023

Uh oh!

maflcko commented Sep 20, 2023

Uh oh!

brunoerg commented Sep 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

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

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

brunoerg commented Nov 25, 2022 •

edited

Loading

DrahtBot commented Nov 25, 2022 •

edited

Loading

josibake commented May 3, 2023 •

edited

Loading