Merge #21614: [0.21] test: Backports

b8af67eeefc9fc9622f839ec8919b7391d91bf6f fuzz: cleanups for versionbits fuzzer (Anthony Towns)
79cdb4a1984c90a4d9377fbb0dda7bdd61d57031 test: make sure non-IP peers get discouraged and disconnected (Vasil Dimov)
b765f41164663c93d63e5a401d3b23c586a4e4fe test: also check disconnect in denialofservice_tests/peer_discouragement (Vasil Dimov)
dfeb6c10bba80dc91245318feb0ad1d879015a99 test: use pointers in denialofservice_tests/peer_discouragement (Vasil Dimov)

Pull request description:

  Backport tests

ACKs for top commit:
  vasild:
    ACK b8af67eeefc9fc9622f839ec8919b7391d91bf6f
  jnewbery:
    ACK b8af67eeefc9fc9622f839ec8919b7391d91bf6f
  ajtowns:
    ACK b8af67eeefc9fc9622f839ec8919b7391d91bf6f ; visually compared individual commits to originals, checked original commits are in master

Tree-SHA512: 22f665560f9d452993b12508d93d93ff54e3e91dcf39f731e27aedfb891570168066c185413d455bee4fa082c011b65ea1b0eee51e3633392b07a0db008d51c8

2 files changed, 90 insertions, 40 deletions

diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index c399da900f..5cffa587e1 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -22,6 +22,7 @@
 
 #include <test/util/setup_common.h>
 
+#include <array>
 #include <stdint.h>
 
 #include <boost/test/unit_test.hpp>
@@ -221,46 +222,89 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
 {
     const CChainParams& chainparams = Params();
     auto banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
-    auto connman = MakeUnique<CConnman>(0x1337, 0x1337);
+    auto connman = MakeUnique<CConnmanTest>(0x1337, 0x1337);
     auto peerLogic = MakeUnique<PeerManager>(chainparams, *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool);
 
+    CNetAddr tor_netaddr;
+    BOOST_REQUIRE(
+        tor_netaddr.SetSpecial("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"));
+    const CService tor_service(tor_netaddr, Params().GetDefaultPort());
+
+    const std::array<CAddress, 3> addr{CAddress{ip(0xa0b0c001), NODE_NONE},
+                                       CAddress{ip(0xa0b0c002), NODE_NONE},
+                                       CAddress{tor_service, NODE_NONE}};
+
+    const CNetAddr other_addr{ip(0xa0b0ff01)}; // Not any of addr[].
+
+    std::array<CNode*, 3> nodes;
+
     banman->ClearBanned();
-    CAddress addr1(ip(0xa0b0c001), NODE_NONE);
-    CNode dummyNode1(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::INBOUND);
-    dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
-    peerLogic->InitializeNode(&dummyNode1);
-    dummyNode1.fSuccessfullyConnected = true;
-    peerLogic->Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD, /* message */ ""); // Should be discouraged
+    nodes[0] = new CNode{id++, NODE_NETWORK, 0, INVALID_SOCKET, addr[0], 0, 0, CAddress(), "", ConnectionType::INBOUND};
+    nodes[0]->SetCommonVersion(PROTOCOL_VERSION);
+    peerLogic->InitializeNode(nodes[0]);
+    nodes[0]->fSuccessfullyConnected = true;
+    connman->AddNode(*nodes[0]);
+    peerLogic->Misbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD, /* message */ ""); // Should be discouraged
     {
-        LOCK(dummyNode1.cs_sendProcessing);
-        BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
+        LOCK(nodes[0]->cs_sendProcessing);
+        BOOST_CHECK(peerLogic->SendMessages(nodes[0]));
+    }
+    BOOST_CHECK(banman->IsDiscouraged(addr[0]));
+    BOOST_CHECK(nodes[0]->fDisconnect);
+    BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged
+
+    nodes[1] = new CNode{id++, NODE_NETWORK, 0, INVALID_SOCKET, addr[1], 1, 1, CAddress(), "", ConnectionType::INBOUND};
+    nodes[1]->SetCommonVersion(PROTOCOL_VERSION);
+    peerLogic->InitializeNode(nodes[1]);
+    nodes[1]->fSuccessfullyConnected = true;
+    connman->AddNode(*nodes[1]);
+    peerLogic->Misbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1, /* message */ "");
+    {
+        LOCK(nodes[1]->cs_sendProcessing);
+        BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
     }
-    BOOST_CHECK(banman->IsDiscouraged(addr1));
-    BOOST_CHECK(!banman->IsDiscouraged(ip(0xa0b0c001|0x0000ff00))); // Different IP, not discouraged
-
-    CAddress addr2(ip(0xa0b0c002), NODE_NONE);
-    CNode dummyNode2(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr2, 1, 1, CAddress(), "", ConnectionType::INBOUND);
-    dummyNode2.SetCommonVersion(PROTOCOL_VERSION);
-    peerLogic->InitializeNode(&dummyNode2);
-    dummyNode2.fSuccessfullyConnected = true;
-    peerLogic->Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1, /* message */ "");
+    // [0] is still discouraged/disconnected.
+    BOOST_CHECK(banman->IsDiscouraged(addr[0]));
+    BOOST_CHECK(nodes[0]->fDisconnect);
+    // [1] is not discouraged/disconnected yet.
+    BOOST_CHECK(!banman->IsDiscouraged(addr[1]));
+    BOOST_CHECK(!nodes[1]->fDisconnect);
+    peerLogic->Misbehaving(nodes[1]->GetId(), 1, /* message */ ""); // [1] reaches discouragement threshold
     {
-        LOCK(dummyNode2.cs_sendProcessing);
-        BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
+        LOCK(nodes[1]->cs_sendProcessing);
+        BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
     }
-    BOOST_CHECK(!banman->IsDiscouraged(addr2)); // 2 not discouraged yet...
-    BOOST_CHECK(banman->IsDiscouraged(addr1));  // ... but 1 still should be
-    peerLogic->Misbehaving(dummyNode2.GetId(), 1, /* message */ "");         // 2 reaches discouragement threshold
+    // Expect both [0] and [1] to be discouraged/disconnected now.
+    BOOST_CHECK(banman->IsDiscouraged(addr[0]));
+    BOOST_CHECK(nodes[0]->fDisconnect);
+    BOOST_CHECK(banman->IsDiscouraged(addr[1]));
+    BOOST_CHECK(nodes[1]->fDisconnect);
+
+    // Make sure non-IP peers are discouraged and disconnected properly.
+
+    nodes[2] = new CNode{id++, NODE_NETWORK, 0, INVALID_SOCKET, addr[2], 1, 1, CAddress(), "",
+                         ConnectionType::OUTBOUND_FULL_RELAY};
+    nodes[2]->SetCommonVersion(PROTOCOL_VERSION);
+    peerLogic->InitializeNode(nodes[2]);
+    nodes[2]->fSuccessfullyConnected = true;
+    connman->AddNode(*nodes[2]);
+    peerLogic->Misbehaving(nodes[2]->GetId(), DISCOURAGEMENT_THRESHOLD, /* message */ "");
     {
-        LOCK(dummyNode2.cs_sendProcessing);
-        BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
+        LOCK(nodes[2]->cs_sendProcessing);
+        BOOST_CHECK(peerLogic->SendMessages(nodes[2]));
     }
-    BOOST_CHECK(banman->IsDiscouraged(addr1));  // Expect both 1 and 2
-    BOOST_CHECK(banman->IsDiscouraged(addr2));  // to be discouraged now
+    BOOST_CHECK(banman->IsDiscouraged(addr[0]));
+    BOOST_CHECK(banman->IsDiscouraged(addr[1]));
+    BOOST_CHECK(banman->IsDiscouraged(addr[2]));
+    BOOST_CHECK(nodes[0]->fDisconnect);
+    BOOST_CHECK(nodes[1]->fDisconnect);
+    BOOST_CHECK(nodes[2]->fDisconnect);
 
     bool dummy;
-    peerLogic->FinalizeNode(dummyNode1, dummy);
-    peerLogic->FinalizeNode(dummyNode2, dummy);
+    for (CNode* node : nodes) {
+        peerLogic->FinalizeNode(*node, dummy);
+    }
+    connman->ClearNodes();
 }
 
 BOOST_AUTO_TEST_CASE(DoS_bantime)
diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp
index 645c0d23c0..77a8d0d08f 100644
--- a/src/test/fuzz/versionbits.cpp
+++ b/src/test/fuzz/versionbits.cpp
@@ -52,9 +52,10 @@ public:
     int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, dummy_params, m_cache); }
     BIP9Stats GetStateStatisticsFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateStatisticsFor(pindexPrev, dummy_params); }
 
-    bool Condition(int64_t version) const
+    bool Condition(int32_t version) const
     {
-        return ((version >> m_bit) & 1) != 0 && (version & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;
+        uint32_t mask = ((uint32_t)1) << m_bit;
+        return (((version & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) && (version & mask) != 0);
     }
 
     bool Condition(const CBlockIndex* pindex) const { return Condition(pindex->nVersion); }
@@ -98,17 +99,20 @@ public:
 };
 } // namespace
 
+std::unique_ptr<const CChainParams> g_params;
+
 void initialize()
 {
-    SelectParams(CBaseChainParams::MAIN);
+    // this is actually comparatively slow, so only do it once
+    g_params = CreateChainParams(ArgsManager{}, CBaseChainParams::MAIN);
+    assert(g_params != nullptr);
 }
 
-constexpr uint32_t MAX_TIME = 4102444800; // 2100-01-01
+constexpr uint32_t MAX_START_TIME = 4102444800; // 2100-01-01
 
 void test_one_input(const std::vector<uint8_t>& buffer)
 {
-    const CChainParams& params = Params();
-
+    const CChainParams& params = *g_params;
     const int64_t interval = params.GetConsensus().nPowTargetSpacing;
     assert(interval > 1); // need to be able to halve it
     assert(interval < std::numeric_limits<int32_t>::max());
@@ -125,9 +129,9 @@ void test_one_input(const std::vector<uint8_t>& buffer)
 
     // too many blocks at 10min each might cause uint32_t time to overflow if
     // block_start_time is at the end of the range above
-    assert(std::numeric_limits<uint32_t>::max() - MAX_TIME > interval * max_blocks);
+    assert(std::numeric_limits<uint32_t>::max() - MAX_START_TIME > interval * max_blocks);
 
-    const int64_t block_start_time = fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(params.GenesisBlock().nTime, MAX_TIME);
+    const int64_t block_start_time = fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(params.GenesisBlock().nTime, MAX_START_TIME);
 
     // what values for version will we use to signal / not signal?
     const int32_t ver_signal = fuzzed_data_provider.ConsumeIntegral<int32_t>();
@@ -171,8 +175,10 @@ void test_one_input(const std::vector<uint8_t>& buffer)
     if (checker.Condition(ver_nosignal)) return;
     if (ver_nosignal < 0) return;
 
-    // TOP_BITS should ensure version will be positive
+    // TOP_BITS should ensure version will be positive and meet min
+    // version requirement
     assert(ver_signal > 0);
+    assert(ver_signal >= VERSIONBITS_LAST_OLD_BLOCK_VERSION);
 
     // Now that we have chosen time and versions, setup to mine blocks
     Blocks blocks(block_start_time, interval, ver_signal, ver_nosignal);
@@ -201,7 +207,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
         }
 
         // don't risk exceeding max_blocks or times may wrap around
-        if (blocks.size() + period*2 > max_blocks) break;
+        if (blocks.size() + 2 * period > max_blocks) break;
     }
     // NOTE: fuzzed_data_provider may be fully consumed at this point and should not be used further
 
@@ -321,7 +327,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
         assert(false);
     }
 
-    if (blocks.size() >= max_periods * period) {
+    if (blocks.size() >= period * max_periods) {
         // we chose the timeout (and block times) so that by the time we have this many blocks it's all over
         assert(state == ThresholdState::ACTIVE || state == ThresholdState::FAILED);
     }