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

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:2104-testPointers changing 1 files +79 −30
  1. 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.

  2. MarcoFalke added the label Refactoring on Apr 2, 2021
  3. MarcoFalke added the label Tests on Apr 2, 2021
  4. MarcoFalke removed the label Refactoring on Apr 2, 2021
  5. 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
  6. DrahtBot commented at 3:10 PM on April 2, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  7. DrahtBot cross-referenced this on Apr 2, 2021 from issue net: Restrict period when cs_vNodes mutex is locked by hebasto
  8. 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

        const CService tor_service{tor_netaddr, Params().GetDefaultPort()};
    MarcoFalke commented at 4:35 PM on April 2, 2021:

    Thanks, done

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

        // Make sure non-IP peers are discouraged and disconnected properly.
    MarcoFalke commented at 4:35 PM on April 2, 2021:

    Thanks, done

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

    @@ -231,9 +231,9 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
-    nodes[0] = new CNode(id++, NODE_NETWORK, INVALID_SOCKET, addr[0], /* nKeyedNetGroupIn */ 0,
+    nodes[0] = new CNode{id++, NODE_NETWORK, INVALID_SOCKET, addr[0], /* nKeyedNetGroupIn */ 0,
                          /* nLocalHostNonceIn */ 0, CAddress(), /* pszDest */ "",
-                         ConnectionType::INBOUND, /* inbound_onion */ false);
+                         ConnectionType::INBOUND, /* inbound_onion */ false};

@@ -247,9 +247,9 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) 
-    nodes[1] = new CNode(id++, NODE_NETWORK, INVALID_SOCKET, addr[1], /* nKeyedNetGroupIn */ 1,
+    nodes[1] = new CNode{id++, NODE_NETWORK, INVALID_SOCKET, addr[1], /* nKeyedNetGroupIn */ 1,
                          /* nLocalHostNonceIn */ 1, CAddress(), /* pszDest */ "",
-                         ConnectionType::INBOUND, /* inbound_onion */ false);
+                         ConnectionType::INBOUND, /* inbound_onion */ false};

    3d5eac6dc3f9911a548a76eee3b6532ea3e02bb5

    @@ -278,11 +278,11 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
 
-    nodes[2] = new CNode(id++, NODE_NETWORK, INVALID_SOCKET, addr[2], /* nKeyedNetGroupIn */ 1,
+    nodes[2] = new CNode{id++, NODE_NETWORK, INVALID_SOCKET, addr[2], /* nKeyedNetGroupIn */ 1,
                          /* nLocalHostNonceIn */ 1, CAddress(), /* pszDest */ "",
-                         ConnectionType::OUTBOUND_FULL_RELAY, /* inbound_onion */ false);
+                         ConnectionType::OUTBOUND_FULL_RELAY, /* inbound_onion */ false};
    MarcoFalke commented at 4:35 PM on April 2, 2021:

    Thanks, done

  11. jonatack commented at 4:23 PM on April 2, 2021: contributor

    ACK 3d5eac6dc3f9911a548a76eee3b6532ea3e02bb5

    A few optional suggestions follow.

  12. 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
  13. 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
  14. test: make sure non-IP peers get discouraged and disconnected 81747b2171
  15. MarcoFalke force-pushed on Apr 2, 2021
  16. jonatack commented at 4:43 PM on April 2, 2021: contributor

    ACK 81747b21719b3fa6b0fdfc3b084c0104d64903f9

  17. DrahtBot cross-referenced this on Apr 2, 2021 from issue Move GetDataDir to ArgsManager by kiminuo
  18. MarcoFalke cross-referenced this on Apr 2, 2021 from issue banman: save the banlist in a JSON format on disk by vasild
  19. vasild approved
  20. vasild commented at 8:22 AM on April 6, 2021: contributor

    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.

  21. MarcoFalke merged this on Apr 6, 2021
  22. MarcoFalke closed this on Apr 6, 2021
  23. MarcoFalke deleted the branch on Apr 6, 2021
  24. MarcoFalke referenced this in commit f97c957f47 on Apr 6, 2021
  25. MarcoFalke referenced this in commit 6c7e0f699f on Apr 6, 2021
  26. MarcoFalke referenced this in commit c6c1f062ec on Apr 6, 2021
  27. MarcoFalke commented at 9:40 AM on April 6, 2021: member

    Backported in #21614

  28. sidhujag referenced this in commit 2be4116d32 on Apr 6, 2021
  29. MarcoFalke referenced this in commit dfeb6c10bb on Apr 16, 2021
  30. MarcoFalke referenced this in commit b765f41164 on Apr 16, 2021
  31. MarcoFalke referenced this in commit 79cdb4a198 on Apr 16, 2021
  32. bitcoin locked this on Aug 18, 2022
Contributors
MarcoFalkeDrahtBotjonatackvasild
Labels
Tests
Linked (view graph)
#20966 banman: save the banlist in a JSON format on disk#21614 [0.21] test: Backports
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC