net: disconnect peers by address without using a subnet by vasild · Pull Request #20849 · bitcoin/bitcoin

vasild · 2021-01-04T16:25:13Z

Previously, when disconnecting a peer with a given address we would
create a dummy subnet that contains just 1 address (/32 for IPv4 and
/128 for IPv6) and would disconnect all peers that belong to this
subnet (there may be more than one connection, to a different port).

The problem is that for any non-IPv4 and non-IPv6 address we would
create an invalid subnet which would later not match any addresses.

Thus, don't use a one-host-subnet, but compare the addresses directly.
This works for any type of addresses.

Previously, when disconnecting a peer with a given address we would create a dummy subnet that contains just 1 address (/32 for IPv4 and /128 for IPv6) and would disconnect all peers that belong to this subnet (there may be more than one connection, to a different port). The problem is that for any non-IPv4 and non-IPv6 address we would create an invalid subnet which would later not match any addresses. Thus, don't use a one-host-subnet, but compare the addresses directly. This works for any type of addresses.

vasild · 2021-01-04T16:27:50Z

The problem was discovered by @MarcoFalke.

maflcko · 2021-01-04T16:35:58Z

tested-only ACK 5fd2cee did not review the code 🔮

Show signature and timestamp

Signature:

Timestamp of file with hash ffb08779a664f7e34db6952de6f0948d1fb8eeb7089b81525f9ffde67a8ac202 -

jonatack

Code review ACK 5fd2cee

It would be good to have a failing test case that this patch fixes, either unit tests or in the functional rpc_setban or p2p_disconnect_ban ones.

vasild · 2021-01-04T17:57:25Z

#20852 contains a fix for this issue + it also fixes a similar issue in BanMan. If it is merged, then this can be closed without merge. They do not conflict in any way. Both can go in.

jonatack · 2021-01-05T12:20:36Z

Any tips how to test this manually? Disconnecting outbound onion peers with disconnectnode works for me on both master and this PR and in #20852.

vasild · 2021-01-05T16:21:32Z

@jonatack yes, the disconnectnode RPC uses either DisconnectNode(std::string) or DisconnectNode(NodeId) and the problem is in DisconnectNode(CNetAddr).

bitcoin-cli setban foo.onion add will do the job.

sipa · 2021-01-05T18:06:10Z

utACK 5fd2cee

Let's get this merged so the path forward for 0.21 is clear (we can still do #20852, but it's no longer critical then).

jonatack · 2021-01-06T12:06:04Z

Adding logging to the code path and verified that setban <onion> add disconnects peers with this patch and fails to do so on master @ 68196a8, with both tor v2 and v3 peers.

Logging added to master and to this patch:

@@ -2805,6 +2805,7 @@ bool CConnman::DisconnectNode(const CSubNet& subnet)
     LOCK(cs_vNodes);
     for (CNode* pnode : vNodes) {
         if (subnet.Match(pnode->addr)) {
+            LogPrintf("Disconnecting %s via subnet peer=%d address=%s network=%s\n",
+                      pnode->ConnectionTypeAsString(), pnode->GetId(),
+                      pnode->addr.ToString(), GetNetworkName(pnode->ConnectedThroughNetwork()));
             pnode->fDisconnect = true;
             disconnected = true;
         }

Logging also added to this patch:

@@ -2818,6 +2819,7 @@ bool CConnman::DisconnectNode(const CNetAddr& addr)
     LOCK(cs_vNodes);
     for (CNode* pnode : vNodes) {
         if (addr == pnode->addr) {
+            LogPrintf("Disconnecting %s via addr peer=%d address=%s network=%s\n",
+                      pnode->ConnectionTypeAsString(), pnode->GetId(),
+                      pnode->addr.ToString(), GetNetworkName(pnode->ConnectedThroughNetwork()));
             pnode->fDisconnect = true;
             disconnected = true;
         }

On master, disconnection fails and nothing is logged. With this patch, the peer is immediately removed from the -netinfo 4 peers dashboard and the log shows:

2021-01-06T11:47:36Z Disconnecting manual via addr peer=5 address=TOR_V3_PEER network=onion
2021-01-06T11:54:30Z Disconnecting manual via addr peer=25 address=TOR_V2_PEER network=onion
2021-01-06T11:54:49Z Disconnecting outbound-full-relay via addr peer=16 address=TOR_V2_PEER:8333 network=onion

Upgrading to tested ACK 5fd2cee

Change the `peer_discouragement` test to use `CNode` pointers so that the nodes it uses can be added to `CConnman::vNodes` and cleaned up properly. Make it use `CConnmanTest` instead of `CConnman`. This is needed because we want to check `CNode::fDisconnect` and for this flag to be flipped by `CConnman::DisconnectNode()` the node must be in `CConnman::vNodes`. Extend the test with one torv3 peer and check that it is discouraged and disconnected as expected.

vasild · 2021-01-06T14:55:17Z

Appended an extra commit with unit test that fails on master and passes on this PR.

If it does not get re-ACKs or if it turns out to be problematic I will remove it, restoring the already ACK'ed commit id.

jonatack · 2021-01-06T15:33:21Z

Nice, will review asap.

DrahtBot · 2021-01-06T19:03:27Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: move net_processing implementation details out of header #20811 (refactor: move net_processing implementation details out of header by ajtowns)
addrman: Make addrman a top-level component #20228 (addrman: Make addrman a top-level component by jnewbery)

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.

jonatack

ACK 98140bc code review, manual testing with setban OUTBOUND_ONION add, and verified the new test fails on master on the non-IP Tor v3 peer disconnection assertion.

jonatack · 2021-01-06T19:58:35Z

src/test/denialofservice_tests.cpp

+    BOOST_CHECK(banman->IsDiscouraged(addr2));
+    BOOST_CHECK(banman->IsDiscouraged(addr3));
+    for (CNode* node : nodes) {
+        BOOST_CHECK(node->fDisconnect);


The new test code fails at the right place on master

test/denialofservice_tests.cpp(292): info: check banman->IsDiscouraged(addr1) has passed test/denialofservice_tests.cpp(293): info: check banman->IsDiscouraged(addr2) has passed test/denialofservice_tests.cpp(294): info: check banman->IsDiscouraged(addr3) has passed test/denialofservice_tests.cpp(296): info: check node->fDisconnect has passed test/denialofservice_tests.cpp(296): info: check node->fDisconnect has passed test/denialofservice_tests.cpp(296): error: in "denialofservice_tests/peer_discouragement": check node->fDisconnect has failed test/denialofservice_tests.cpp(222): Leaving test case "peer_discouragement"; testing time: 32905us

nit, maybe not worth it for tests, could use nodes.at() notation for bounds checking instead of nodes[]

laanwj · 2021-01-07T14:49:46Z

src/net.cpp

 bool CConnman::DisconnectNode(const CNetAddr& addr)
 {
-    return DisconnectNode(CSubNet(addr));
+    bool disconnected = false;


Is this change still needed with #20852? It seems to me that makes the old, much more compact, construction work?

No, if #20852 is merged, then this PR is not needed, see #20849 (comment).

I first opened this PR to fix DisconnectNode() and then realized that the same problem exists in BanMan, thus opened #20852 which fixes both by modifying CSubNet.

Maybe this PR could be preferred as less risky compared to #20852. It is ok if this is merged first and later #20852 is also merged - they do not conflict and both go in master.

vasild · 2021-01-08T10:05:18Z

Just to elaborate on the relationship between #20849 and #20852:

The problem in master is that CSubNet(foo.onion) creates an invalid subnet which later does not match any address, not even foo.onion. In two places in the code we expect that once we create such subnet later it will match foo.onion: in CConnman::DisconnectNode() and in BanMan.

#20849 fixes DisconnectNode() by changing it to not use subnets at all (small isolated change, easy to verify it is correct).

#20852 changes CSubNet to follow the above expected behavior, thus fixing both DisconnectNode() and BanMan (bigger change, harder to verify it is correct, caused unexpected CI failure (fixed)).

About the release, maybe the following makes sense:

Include #20849 in the next rc and in the 0.21.0 release.

Later, if #20852 is merged (ever) there is no need to revert #20849.

maflcko · 2021-01-11T10:15:39Z

It might be better to wait for #20852 (0.21.1), after which this is not needed.

maflcko

Concept ACK on adding the test. Mind to open a new pull for this?

src/test/denialofservice_tests.cpp

vasild · 2021-01-11T16:39:20Z

The test is included in #20904.

maflcko added this to the 0.21.0 milestone Jan 4, 2021

maflcko added Needs backport (0.21) P2P labels Jan 4, 2021

jonatack reviewed Jan 4, 2021

View reviewed changes

vasild mentioned this pull request Jan 4, 2021

net: allow CSubNet of non-IP networks #20852

Merged

This comment has been minimized.

Sign in to view

DrahtBot mentioned this pull request Jan 6, 2021

refactor: move net_processing implementation details out of header #20811

Merged

jonatack reviewed Jan 6, 2021

View reviewed changes

DrahtBot mentioned this pull request Jan 6, 2021

addrman: Make addrman a top-level component #20228

Merged

laanwj reviewed Jan 7, 2021

View reviewed changes

DrahtBot mentioned this pull request Jan 9, 2021

net-processing refactoring -- lose globals, move implementation details from .h to .cpp #20758

Closed

fanquake closed this Jan 11, 2021

maflcko reviewed Jan 11, 2021

View reviewed changes

src/test/denialofservice_tests.cpp Show resolved Hide resolved

vasild deleted the disconnect_by_subnet_fix branch January 11, 2021 12:52

maflcko removed the Needs backport (0.21) label Mar 13, 2021

bitcoin deleted a comment from taurus228 Aug 15, 2021

bitcoin locked and limited conversation to collaborators Aug 15, 2021

Conversation

vasild commented Jan 4, 2021

Uh oh!

vasild commented Jan 4, 2021

Uh oh!

maflcko commented Jan 4, 2021

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild commented Jan 4, 2021

Uh oh!

jonatack commented Jan 5, 2021

Uh oh!

vasild commented Jan 5, 2021

Uh oh!

This comment has been minimized.

sipa commented Jan 5, 2021

Uh oh!

jonatack commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Jan 6, 2021

Uh oh!

jonatack commented Jan 6, 2021

Uh oh!

DrahtBot commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

vasild commented Jan 8, 2021

Uh oh!

maflcko commented Jan 11, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Jan 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jonatack left a comment •

edited

Loading

jonatack commented Jan 6, 2021 •

edited

Loading

DrahtBot commented Jan 6, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack Jan 6, 2021 •

edited

Loading

laanwj Jan 7, 2021 •

edited

Loading