From dfeb6c10bba80dc91245318feb0ad1d879015a99 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Wed, 20 Jan 2021 11:26:43 +0100 Subject: 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. Github-Pull: #21571 Rebased-From: 4d6e246fa46f2309e2998b542e4c104d73d29071 --- src/test/denialofservice_tests.cpp | 65 ++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 28 deletions(-) (limited to 'src') diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index c399da900f..bf981fcbbf 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -22,6 +22,7 @@ #include +#include #include #include @@ -224,43 +225,51 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) auto connman = MakeUnique(0x1337, 0x1337); auto peerLogic = MakeUnique(chainparams, *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); + const std::array addr{CAddress{ip(0xa0b0c001), NODE_NONE}, + CAddress{ip(0xa0b0c002), NODE_NONE}}; + + const CNetAddr other_addr{ip(0xa0b0ff01)}; // Not any of addr[]. + + std::array 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; + 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(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 */ ""); + BOOST_CHECK(banman->IsDiscouraged(addr[0])); + 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; + peerLogic->Misbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1, /* message */ ""); { - 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 + BOOST_CHECK(!banman->IsDiscouraged(addr[1])); // [1] not discouraged yet... + BOOST_CHECK(banman->IsDiscouraged(addr[0])); // ... but [0] still should be + 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(addr1)); // Expect both 1 and 2 - BOOST_CHECK(banman->IsDiscouraged(addr2)); // to be discouraged now + // Expect both [0] and [1] to be discouraged now. + BOOST_CHECK(banman->IsDiscouraged(addr[0])); + BOOST_CHECK(banman->IsDiscouraged(addr[1])); bool dummy; - peerLogic->FinalizeNode(dummyNode1, dummy); - peerLogic->FinalizeNode(dummyNode2, dummy); + for (CNode* node : nodes) { + peerLogic->FinalizeNode(*node, dummy); + delete node; + } } BOOST_AUTO_TEST_CASE(DoS_bantime) -- cgit v1.2.3 From b765f41164663c93d63e5a401d3b23c586a4e4fe Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Wed, 20 Jan 2021 11:40:01 +0100 Subject: 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. Github-Pull: #21571 Rebased-From: 637bb6da368b87711005b909f451f94909400092 --- src/test/denialofservice_tests.cpp | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index bf981fcbbf..22f1ccb2d2 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -222,7 +222,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) { const CChainParams& chainparams = Params(); auto banman = MakeUnique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); - auto connman = MakeUnique(0x1337, 0x1337); + auto connman = MakeUnique(0x1337, 0x1337); auto peerLogic = MakeUnique(chainparams, *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); const std::array addr{CAddress{ip(0xa0b0c001), NODE_NONE}, @@ -237,39 +237,48 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) 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(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(addr[1])); // [1] not discouraged yet... - BOOST_CHECK(banman->IsDiscouraged(addr[0])); // ... but [0] still should be + // [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(nodes[1]->cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(nodes[1])); } - // Expect both [0] and [1] to be discouraged now. + // 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); bool dummy; for (CNode* node : nodes) { peerLogic->FinalizeNode(*node, dummy); - delete node; } + connman->ClearNodes(); } BOOST_AUTO_TEST_CASE(DoS_bantime) -- cgit v1.2.3 From 79cdb4a1984c90a4d9377fbb0dda7bdd61d57031 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Wed, 20 Jan 2021 11:54:17 +0100 Subject: test: make sure non-IP peers get discouraged and disconnected Github-Pull: #21571 Rebased-From: 81747b21719b3fa6b0fdfc3b084c0104d64903f9 --- src/test/denialofservice_tests.cpp | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 22f1ccb2d2..5cffa587e1 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -225,12 +225,18 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) auto connman = MakeUnique(0x1337, 0x1337); auto peerLogic = MakeUnique(chainparams, *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool); - const std::array addr{CAddress{ip(0xa0b0c001), NODE_NONE}, - CAddress{ip(0xa0b0c002), NODE_NONE}}; + CNetAddr tor_netaddr; + BOOST_REQUIRE( + tor_netaddr.SetSpecial("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion")); + const CService tor_service(tor_netaddr, Params().GetDefaultPort()); + + const std::array 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 nodes; + std::array nodes; banman->ClearBanned(); nodes[0] = new CNode{id++, NODE_NETWORK, 0, INVALID_SOCKET, addr[0], 0, 0, CAddress(), "", ConnectionType::INBOUND}; @@ -274,6 +280,26 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) 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(nodes[2]->cs_sendProcessing); + BOOST_CHECK(peerLogic->SendMessages(nodes[2])); + } + 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; for (CNode* node : nodes) { peerLogic->FinalizeNode(*node, dummy); -- cgit v1.2.3 From b8af67eeefc9fc9622f839ec8919b7391d91bf6f Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Sun, 21 Mar 2021 10:45:17 +1000 Subject: fuzz: cleanups for versionbits fuzzer Github-Pull: #21489 Rebased-From: aa7f418fe32b3ec53285693a7731decd99be4528 --- src/test/fuzz/versionbits.cpp | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) (limited to 'src') 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 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& 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::max()); @@ -125,9 +129,9 @@ void test_one_input(const std::vector& 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::max() - MAX_TIME > interval * max_blocks); + assert(std::numeric_limits::max() - MAX_START_TIME > interval * max_blocks); - const int64_t block_start_time = fuzzed_data_provider.ConsumeIntegralInRange(params.GenesisBlock().nTime, MAX_TIME); + const int64_t block_start_time = fuzzed_data_provider.ConsumeIntegralInRange(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(); @@ -171,8 +175,10 @@ void test_one_input(const std::vector& 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& 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& 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); } -- cgit v1.2.3