test: make sure non-IP peers get discouraged and disconnected (vasild)

MarcoFalke commented at 12:22 pm on April 2, 2021: member

Split up from #20966, so that it can be backported easier. Merging this ahead of #20966 will also reduce the number of conflicts for that pull.

MarcoFalke added the label Refactoring on Apr 2, 2021

MarcoFalke added the label Tests on Apr 2, 2021

MarcoFalke removed the label Refactoring on Apr 2, 2021

MarcoFalke renamed this:
~~test: make sure non-IP peers get discouraged and disconnected~~
test: make sure non-IP peers get discouraged and disconnected (vasild)
on Apr 2, 2021

DrahtBot commented at 3:10 pm on April 2, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21563 (net: Drop cs_sendProcessing mutex that guards nothing by hebasto)
#21244 (Move GetDataDir to ArgsManager by kiminuo)

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.

in src/test/denialofservice_tests.cpp:220 in 3d5eac6dc3 outdated

216                                        *m_node.scheduler, *m_node.chainman, *m_node.mempool, false);
217 
218+    CNetAddr tor_netaddr;
219+    BOOST_REQUIRE(
220+        tor_netaddr.SetSpecial("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"));
221+    const CService tor_service(tor_netaddr, Params().GetDefaultPort());

jonatack commented at 4:15 pm on April 2, 2021:

3d5eac6d

0    const CService tor_service{tor_netaddr, Params().GetDefaultPort()};

MarcoFalke commented at 4:35 pm on April 2, 2021:

Thanks, done

in src/test/denialofservice_tests.cpp:276 in 3d5eac6dc3 outdated

301+    BOOST_CHECK(banman->IsDiscouraged(addr[0]));
302+    BOOST_CHECK(nodes[0]->fDisconnect);
303+    BOOST_CHECK(banman->IsDiscouraged(addr[1]));
304+    BOOST_CHECK(nodes[1]->fDisconnect);
305+
306+    // Make sure non-IP peers get discouraged and disconnected properly.

jonatack commented at 4:17 pm on April 2, 2021:

3d5eac6dc3f9911a548a76eee3b6532ea3e02bb5

0    // Make sure non-IP peers are discouraged and disconnected properly.

MarcoFalke commented at 4:35 pm on April 2, 2021:

Thanks, done

in src/test/denialofservice_tests.cpp:278 in 3d5eac6dc3 outdated

303+    BOOST_CHECK(banman->IsDiscouraged(addr[1]));
304+    BOOST_CHECK(nodes[1]->fDisconnect);
305+
306+    // Make sure non-IP peers get discouraged and disconnected properly.
307+
308+    nodes[2] = new CNode(id++, NODE_NETWORK, INVALID_SOCKET, addr[2], /* nKeyedNetGroupIn */ 1,

jonatack commented at 4:21 pm on April 2, 2021:

d87815ea

 0@@ -231,9 +231,9 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
 1-    nodes[0] = new CNode(id++, NODE_NETWORK, INVALID_SOCKET, addr[0], /* nKeyedNetGroupIn */ 0,
 2+    nodes[0] = new CNode{id++, NODE_NETWORK, INVALID_SOCKET, addr[0], /* nKeyedNetGroupIn */ 0,
 3                          /* nLocalHostNonceIn */ 0, CAddress(), /* pszDest */ "",
 4-                         ConnectionType::INBOUND, /* inbound_onion */ false);
 5+                         ConnectionType::INBOUND, /* inbound_onion */ false};
 6
 7@@ -247,9 +247,9 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) 
 8-    nodes[1] = new CNode(id++, NODE_NETWORK, INVALID_SOCKET, addr[1], /* nKeyedNetGroupIn */ 1,
 9+    nodes[1] = new CNode{id++, NODE_NETWORK, INVALID_SOCKET, addr[1], /* nKeyedNetGroupIn */ 1,
10                          /* nLocalHostNonceIn */ 1, CAddress(), /* pszDest */ "",
11-                         ConnectionType::INBOUND, /* inbound_onion */ false);
12+                         ConnectionType::INBOUND, /* inbound_onion */ false};

3d5eac6dc3f9911a548a76eee3b6532ea3e02bb5

0@@ -278,11 +278,11 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
1 
2-    nodes[2] = new CNode(id++, NODE_NETWORK, INVALID_SOCKET, addr[2], /* nKeyedNetGroupIn */ 1,
3+    nodes[2] = new CNode{id++, NODE_NETWORK, INVALID_SOCKET, addr[2], /* nKeyedNetGroupIn */ 1,
4                          /* nLocalHostNonceIn */ 1, CAddress(), /* pszDest */ "",
5-                         ConnectionType::OUTBOUND_FULL_RELAY, /* inbound_onion */ false);
6+                         ConnectionType::OUTBOUND_FULL_RELAY, /* inbound_onion */ false};

MarcoFalke commented at 4:35 pm on April 2, 2021:

Thanks, done

jonatack commented at 4:23 pm on April 2, 2021: member

ACK 3d5eac6dc3f9911a548a76eee3b6532ea3e02bb5

A few optional suggestions follow.

test: use pointers in denialofservice_tests/peer_discouragement

This is a non-functional change that replaces the `CNode` on-stack
variables with `CNode` pointers.

The reason for this is that it would allow us to add those `CNode`s
to `CConnman::vNodes[]` which in turn would allow us to check that they
are disconnected properly - a `CNode` object must be in
`CConnman::vNodes[]` in order for its `fDisconnect` flag to be set.

If we store pointers to the on-stack variables in `CConnman` then it
would crash at the end, trying to `delete` them.

4d6e246fa4

test: also check disconnect in denialofservice_tests/peer_discouragement

Use `CConnmanTest` instead of `CConnman` and add the nodes to it
so that their `fDisconnect` flag is set during disconnection.

637bb6da36

test: make sure non-IP peers get discouraged and disconnected 81747b2171

MarcoFalke force-pushed on Apr 2, 2021

jonatack commented at 4:43 pm on April 2, 2021: member

ACK 81747b21719b3fa6b0fdfc3b084c0104d64903f9

vasild approved

vasild commented at 8:22 am on April 6, 2021: member

81747b21719b3fa6b0fdfc3b084c0104d64903f9 looks good

I verified that the 3 commits in this PR are the same as the ones in #20966 (except conflict resolution and minor change in comment).

I think it may be misleading if I ACK this PR, coz im the author of the commitz.

MarcoFalke merged this on Apr 6, 2021

MarcoFalke closed this on Apr 6, 2021

MarcoFalke deleted the branch on Apr 6, 2021

MarcoFalke referenced this in commit f97c957f47 on Apr 6, 2021

MarcoFalke referenced this in commit 6c7e0f699f on Apr 6, 2021

MarcoFalke referenced this in commit c6c1f062ec on Apr 6, 2021

MarcoFalke commented at 9:40 am on April 6, 2021: member

Backported in #21614

sidhujag referenced this in commit 2be4116d32 on Apr 6, 2021

MarcoFalke referenced this in commit dfeb6c10bb on Apr 16, 2021

MarcoFalke referenced this in commit b765f41164 on Apr 16, 2021

MarcoFalke referenced this in commit 79cdb4a198 on Apr 16, 2021

DrahtBot locked this on Aug 18, 2022

test: make sure non-IP peers get discouraged and disconnected (vasild) #21571

Conflicts