From ad4369fd833ff5396453b34aca15d03b64cac5d8 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 20 Jan 2021 11:26:43 +0100 Subject: [PATCH 01/14] merge bitcoin#21571: make sure non-IP peers get discouraged and disconnected --- src/test/denialofservice_tests.cpp | 143 +++++++++++++++++++---------- 1 file changed, 97 insertions(+), 46 deletions(-) diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index e3f9d71039..457106d0f5 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -230,66 +230,117 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) { const CChainParams& chainparams = Params(); auto banman = std::make_unique(m_args.GetDataDirPath() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); - auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync, *m_node.govman, *m_node.sporkman, /* mn_activeman = */ nullptr, m_node.dmnman, m_node.cj_ctx, m_node.llmq_ctx, /* ignore_incoming_txs = */ false); + 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; + banman->ClearBanned(); - CAddress addr1(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode1{id++, - NODE_NETWORK, - /*sock=*/nullptr, - addr1, - /*nKeyedNetGroupIn=*/0, - /*nLocalHostNonceIn=*/0, - CAddress(), - /*addrNameIn=*/"", - ConnectionType::INBOUND, - /*inbound_onion=*/false}; - dummyNode1.SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(&dummyNode1); - dummyNode1.fSuccessfullyConnected = true; - peerLogic->Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged + nodes[0] = new CNode{id++, + NODE_NETWORK, + /*sock=*/nullptr, + addr[0], + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + CAddress(), + /*addrNameIn=*/"", + ConnectionType::INBOUND, + /*inbound_onion=*/false}; + nodes[0]->SetCommonVersion(PROTOCOL_VERSION); + peerLogic->InitializeNode(nodes[0]); + nodes[0]->fSuccessfullyConnected = true; + connman->AddTestNode(*nodes[0]); + peerLogic->Misbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // 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 + BOOST_CHECK(banman->IsDiscouraged(addr[0])); + BOOST_CHECK(nodes[0]->fDisconnect); + BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged - CAddress addr2(ip(0xa0b0c002), NODE_NONE); - CNode dummyNode2{id++, - NODE_NETWORK, - /*sock=*/nullptr, - addr2, - /*nKeyedNetGroupIn=*/1, - /*nLocalHostNonceIn=*/1, - CAddress(), - /*pszDest=*/"", - ConnectionType::INBOUND, - /*inbound_onion=*/false}; - dummyNode2.SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(&dummyNode2); - dummyNode2.fSuccessfullyConnected = true; - peerLogic->Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1); + nodes[1] = new CNode{id++, + NODE_NETWORK, + /*sock=*/nullptr, + addr[1], + /*nKeyedNetGroupIn=*/1, + /*nLocalHostNonceIn=*/1, + CAddress(), + /*pszDest=*/"", + ConnectionType::INBOUND, + /*inbound_onion=*/false}; + nodes[1]->SetCommonVersion(PROTOCOL_VERSION); + peerLogic->InitializeNode(nodes[1]); + nodes[1]->fSuccessfullyConnected = true; + connman->AddTestNode(*nodes[1]); + peerLogic->Misbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1); { - 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); // 2 reaches discouragement threshold + // [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); // [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/disconnected now. + BOOST_CHECK(banman->IsDiscouraged(addr[0])); + BOOST_CHECK(nodes[0]->fDisconnect); + BOOST_CHECK(banman->IsDiscouraged(addr[1])); + BOOST_CHECK(nodes[1]->fDisconnect); - peerLogic->FinalizeNode(dummyNode1); - peerLogic->FinalizeNode(dummyNode2); + // Make sure non-IP peers are discouraged and disconnected properly. + + nodes[2] = new CNode{id++, + NODE_NETWORK, + /*sock=*/nullptr, + addr[2], + /*nKeyedNetGroupIn=*/1, + /*nLocalHostNonceIn=*/1, + CAddress(), + /*pszDest=*/"", + ConnectionType::OUTBOUND_FULL_RELAY, + /*inbound_onion=*/false}; + nodes[2]->SetCommonVersion(PROTOCOL_VERSION); + peerLogic->InitializeNode(nodes[2]); + nodes[2]->fSuccessfullyConnected = true; + connman->AddTestNode(*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); + + for (CNode* node : nodes) { + peerLogic->FinalizeNode(*node); + } + connman->ClearTestNodes(); } BOOST_AUTO_TEST_CASE(DoS_bantime) From 30ac41e068d1a6cfbc92e44529462f363bac12e9 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 12 Jun 2024 14:27:51 +0000 Subject: [PATCH 02/14] merge bitcoin#22284: performance improvements to ProtectEvictionCandidatesByRatio() --- src/Makefile.bench.include | 1 + src/bench/peer_eviction.cpp | 156 +++++++++++++++++++++++++++ src/net.cpp | 15 ++- src/test/net_peer_eviction_tests.cpp | 22 ---- src/test/util/net.cpp | 25 +++++ src/test/util/net.h | 2 + 6 files changed, 194 insertions(+), 27 deletions(-) create mode 100644 src/bench/peer_eviction.cpp diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index 8efcee2868..11d8c1460f 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -40,6 +40,7 @@ bench_bench_dash_SOURCES = \ bench/mempool_stress.cpp \ bench/nanobench.h \ bench/nanobench.cpp \ + bench/peer_eviction.cpp \ bench/rpc_blockchain.cpp \ bench/rpc_mempool.cpp \ bench/util_time.cpp \ diff --git a/src/bench/peer_eviction.cpp b/src/bench/peer_eviction.cpp new file mode 100644 index 0000000000..0469f0cb4c --- /dev/null +++ b/src/bench/peer_eviction.cpp @@ -0,0 +1,156 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +static void EvictionProtectionCommon( + benchmark::Bench& bench, + int num_candidates, + std::function candidate_setup_fn) +{ + using Candidates = std::vector; + FastRandomContext random_context{true}; + bench.warmup(100).epochIterations(1100); + + Candidates candidates{GetRandomNodeEvictionCandidates(num_candidates, random_context)}; + for (auto& c : candidates) { + candidate_setup_fn(c); + } + + std::vector copies{bench.epochs() * bench.epochIterations(), candidates}; + size_t i{0}; + bench.run([&] { + ProtectEvictionCandidatesByRatio(copies.at(i)); + ++i; + }); +} + +/* Benchmarks */ + +static void EvictionProtection0Networks250Candidates(benchmark::Bench& bench) +{ + EvictionProtectionCommon( + bench, + 250 /* num_candidates */, + [](NodeEvictionCandidate& c) { + c.nTimeConnected = c.id; + c.m_network = NET_IPV4; + }); +} + +static void EvictionProtection1Networks250Candidates(benchmark::Bench& bench) +{ + EvictionProtectionCommon( + bench, + 250 /* num_candidates */, + [](NodeEvictionCandidate& c) { + c.nTimeConnected = c.id; + c.m_is_local = false; + if (c.id >= 130 && c.id < 240) { // 110 Tor + c.m_network = NET_ONION; + } else { + c.m_network = NET_IPV4; + } + }); +} + +static void EvictionProtection2Networks250Candidates(benchmark::Bench& bench) +{ + EvictionProtectionCommon( + bench, + 250 /* num_candidates */, + [](NodeEvictionCandidate& c) { + c.nTimeConnected = c.id; + c.m_is_local = false; + if (c.id >= 90 && c.id < 160) { // 70 Tor + c.m_network = NET_ONION; + } else if (c.id >= 170 && c.id < 250) { // 80 I2P + c.m_network = NET_I2P; + } else { + c.m_network = NET_IPV4; + } + }); +} + +static void EvictionProtection3Networks050Candidates(benchmark::Bench& bench) +{ + EvictionProtectionCommon( + bench, + 50 /* num_candidates */, + [](NodeEvictionCandidate& c) { + c.nTimeConnected = c.id; + c.m_is_local = (c.id == 28 || c.id == 47); // 2 localhost + if (c.id >= 30 && c.id < 47) { // 17 I2P + c.m_network = NET_I2P; + } else if (c.id >= 24 && c.id < 28) { // 4 Tor + c.m_network = NET_ONION; + } else { + c.m_network = NET_IPV4; + } + }); +} + +static void EvictionProtection3Networks100Candidates(benchmark::Bench& bench) +{ + EvictionProtectionCommon( + bench, + 100 /* num_candidates */, + [](NodeEvictionCandidate& c) { + c.nTimeConnected = c.id; + c.m_is_local = (c.id >= 55 && c.id < 60); // 5 localhost + if (c.id >= 70 && c.id < 80) { // 10 I2P + c.m_network = NET_I2P; + } else if (c.id >= 80 && c.id < 96) { // 16 Tor + c.m_network = NET_ONION; + } else { + c.m_network = NET_IPV4; + } + }); +} + +static void EvictionProtection3Networks250Candidates(benchmark::Bench& bench) +{ + EvictionProtectionCommon( + bench, + 250 /* num_candidates */, + [](NodeEvictionCandidate& c) { + c.nTimeConnected = c.id; + c.m_is_local = (c.id >= 140 && c.id < 160); // 20 localhost + if (c.id >= 170 && c.id < 180) { // 10 I2P + c.m_network = NET_I2P; + } else if (c.id >= 190 && c.id < 240) { // 50 Tor + c.m_network = NET_ONION; + } else { + c.m_network = NET_IPV4; + } + }); +} + +// Candidate numbers used for the benchmarks: +// - 50 candidates simulates a possible use of -maxconnections +// - 100 candidates approximates an average node with default settings +// - 250 candidates is the number of peers reported by operators of busy nodes + +// No disadvantaged networks, with 250 eviction candidates. +BENCHMARK(EvictionProtection0Networks250Candidates); + +// 1 disadvantaged network (Tor) with 250 eviction candidates. +BENCHMARK(EvictionProtection1Networks250Candidates); + +// 2 disadvantaged networks (I2P, Tor) with 250 eviction candidates. +BENCHMARK(EvictionProtection2Networks250Candidates); + +// 3 disadvantaged networks (I2P/localhost/Tor) with 50/100/250 eviction candidates. +BENCHMARK(EvictionProtection3Networks050Candidates); +BENCHMARK(EvictionProtection3Networks100Candidates); +BENCHMARK(EvictionProtection3Networks250Candidates); diff --git a/src/net.cpp b/src/net.cpp index 255cb969c0..6679b26a8b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1068,14 +1068,17 @@ void ProtectEvictionCandidatesByRatio(std::vector& evicti size_t num_protected{0}; while (num_protected < max_protect_by_network) { + // Count the number of disadvantaged networks from which we have peers to protect. + auto num_networks = std::count_if(networks.begin(), networks.end(), [](const Net& n) { return n.count; }); + if (num_networks == 0) { + break; + } const size_t disadvantaged_to_protect{max_protect_by_network - num_protected}; - const size_t protect_per_network{ - std::max(disadvantaged_to_protect / networks.size(), static_cast(1))}; - + const size_t protect_per_network{std::max(disadvantaged_to_protect / num_networks, static_cast(1))}; // Early exit flag if there are no remaining candidates by disadvantaged network. bool protected_at_least_one{false}; - for (const Net& n : networks) { + for (Net& n : networks) { if (n.count == 0) continue; const size_t before = eviction_candidates.size(); EraseLastKElements(eviction_candidates, CompareNodeNetworkTime(n.is_local, n.id), @@ -1085,10 +1088,12 @@ void ProtectEvictionCandidatesByRatio(std::vector& evicti const size_t after = eviction_candidates.size(); if (before > after) { protected_at_least_one = true; - num_protected += before - after; + const size_t delta{before - after}; + num_protected += delta; if (num_protected >= max_protect_by_network) { break; } + n.count -= delta; } } if (!protected_at_least_one) { diff --git a/src/test/net_peer_eviction_tests.cpp b/src/test/net_peer_eviction_tests.cpp index 4957969d23..ca24a4070c 100644 --- a/src/test/net_peer_eviction_tests.cpp +++ b/src/test/net_peer_eviction_tests.cpp @@ -17,28 +17,6 @@ BOOST_FIXTURE_TEST_SUITE(net_peer_eviction_tests, BasicTestingSetup) -std::vector GetRandomNodeEvictionCandidates(const int n_candidates, FastRandomContext& random_context) -{ - std::vector candidates; - for (int id = 0; id < n_candidates; ++id) { - candidates.push_back({ - /* id */ id, - /* nTimeConnected */ static_cast(random_context.randrange(100)), - /* m_min_ping_time */ std::chrono::microseconds{random_context.randrange(100)}, - /* nLastBlockTime */ static_cast(random_context.randrange(100)), - /* nLastTXTime */ static_cast(random_context.randrange(100)), - /* fRelevantServices */ random_context.randbool(), - /* m_relay_txs */ random_context.randbool(), - /* fBloomFilter */ random_context.randbool(), - /* nKeyedNetGroup */ random_context.randrange(100), - /* prefer_evict */ random_context.randbool(), - /* m_is_local */ random_context.randbool(), - /* m_network */ ALL_NETWORKS[random_context.randrange(ALL_NETWORKS.size())], - }); - } - return candidates; -} - // Create `num_peers` random nodes, apply setup function `candidate_setup_fn`, // call ProtectEvictionCandidatesByRatio() to apply protection logic, and then // return true if all of `protected_peer_ids` and none of `unprotected_peer_ids` diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 847a490e03..7d44f783fa 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -6,6 +6,9 @@ #include #include +#include + +#include void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const { @@ -37,3 +40,25 @@ bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) con NodeReceiveMsgBytes(node, ser_msg.data, complete); return complete; } + +std::vector GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context) +{ + std::vector candidates; + for (int id = 0; id < n_candidates; ++id) { + candidates.push_back({ + /* id */ id, + /* nTimeConnected */ static_cast(random_context.randrange(100)), + /* m_min_ping_time */ std::chrono::microseconds{random_context.randrange(100)}, + /* nLastBlockTime */ static_cast(random_context.randrange(100)), + /* nLastTXTime */ static_cast(random_context.randrange(100)), + /* fRelevantServices */ random_context.randbool(), + /* m_relay_txs */ random_context.randbool(), + /* fBloomFilter */ random_context.randbool(), + /* nKeyedNetGroup */ random_context.randrange(100), + /* prefer_evict */ random_context.randbool(), + /* m_is_local */ random_context.randbool(), + /* m_network */ ALL_NETWORKS[random_context.randrange(ALL_NETWORKS.size())], + }); + } + return candidates; +} diff --git a/src/test/util/net.h b/src/test/util/net.h index cfb465c5fd..6516577f44 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -177,4 +177,6 @@ private: mutable size_t m_consumed; }; +std::vector GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context); + #endif // BITCOIN_TEST_UTIL_NET_H From 224fb687c86d344dd55ee8cacec598efc0141ce9 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 1 Dec 2020 11:00:26 -0800 Subject: [PATCH 03/14] merge bitcoin#20541: Move special CAddress-without-nTime logic to net_processing --- src/net_processing.cpp | 37 +++++++++++++++++++++---------------- src/protocol.h | 11 +---------- 2 files changed, 22 insertions(+), 26 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 14ed0adde9..18e1d380ef 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1254,17 +1254,15 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer) // Note that pnode->GetLocalServices() is a reflection of the local // services we were offering when the CNode object was created for this // peer. - ServiceFlags nLocalNodeServices = pnode.GetLocalServices(); + uint64_t my_services{pnode.GetLocalServices()}; const int64_t nTime{count_seconds(GetTime())}; uint64_t nonce = pnode.GetLocalNonce(); const int nNodeStartingHeight{m_best_height}; NodeId nodeid = pnode.GetId(); CAddress addr = pnode.addr; - CAddress addrYou = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? - addr : - CAddress(CService(), addr.nServices); - CAddress addrMe = CAddress(CService(), nLocalNodeServices); + CService addr_you = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? addr : CService(); + uint64_t your_services{addr.nServices}; uint256 mnauthChallenge; GetRandBytes({mnauthChallenge.begin(), mnauthChallenge.size()}); @@ -1276,13 +1274,15 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer) } const bool tx_relay = !m_ignore_incoming_txs && !pnode.IsBlockOnlyConn(); - m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, nProtocolVersion, (uint64_t)nLocalNodeServices, nTime, addrYou, addrMe, - nonce, strSubVersion, nNodeStartingHeight, tx_relay, mnauthChallenge, pnode.m_masternode_connection.load())); + m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, nProtocolVersion, my_services, nTime, + your_services, addr_you, // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime) + my_services, CService(), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime) + nonce, strSubVersion, nNodeStartingHeight, tx_relay, mnauthChallenge, pnode.m_masternode_connection.load())); if (fLogIPs) { - LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, us=%s, them=%s, txrelay=%d, peer=%d\n", nProtocolVersion, nNodeStartingHeight, addrMe.ToString(), addrYou.ToString(), tx_relay, nodeid); + LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, them=%s, txrelay=%d, peer=%d\n", nProtocolVersion, nNodeStartingHeight, addr_you.ToString(), tx_relay, nodeid); } else { - LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, us=%s, txrelay=%d, peer=%d\n", nProtocolVersion, nNodeStartingHeight, addrMe.ToString(), tx_relay, nodeid); + LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, txrelay=%d, peer=%d\n", nProtocolVersion, nNodeStartingHeight, tx_relay, nodeid); } } @@ -3276,21 +3276,20 @@ void PeerManagerImpl::ProcessMessage( } int64_t nTime; - CAddress addrMe; - CAddress addrFrom; + CService addrMe; uint64_t nNonce = 1; - uint64_t nServiceInt; ServiceFlags nServices; int nVersion; std::string cleanSubVer; int starting_height = -1; bool fRelay = true; - vRecv >> nVersion >> nServiceInt >> nTime >> addrMe; + vRecv >> nVersion >> Using>(nServices) >> nTime; if (nTime < 0) { nTime = 0; } - nServices = ServiceFlags(nServiceInt); + vRecv.ignore(8); // Ignore the addrMe service bits sent by the peer + vRecv >> addrMe; if (!pfrom.IsInboundConn()) { m_addrman.SetServices(pfrom.addr, nServices); @@ -3309,8 +3308,14 @@ void PeerManagerImpl::ProcessMessage( return; } - if (!vRecv.empty()) - vRecv >> addrFrom >> nNonce; + if (!vRecv.empty()) { + // The version message includes information about the sending node which we don't use: + // - 8 bytes (service bits) + // - 16 bytes (ipv6 address) + // - 2 bytes (port) + vRecv.ignore(26); + vRecv >> nNonce; + } if (!vRecv.empty()) { std::string strSubVer; vRecv >> LIMITED_STRING(strSubVer, MAX_SUBVERSION_LENGTH); diff --git a/src/protocol.h b/src/protocol.h index eba54aef77..f3c354626f 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -431,7 +431,6 @@ public: // ambiguous what that would mean. Make sure no code relying on that is introduced: assert(!(s.GetType() & SER_GETHASH)); bool use_v2; - bool store_time; if (s.GetType() & SER_DISK) { // In the disk serialization format, the encoding (v1 or v2) is determined by a flag version // that's part of the serialization itself. ADDRV2_FORMAT in the stream version only determines @@ -448,24 +447,16 @@ public: } else { throw std::ios_base::failure("Unsupported CAddress disk format version"); } - store_time = true; } else { // In the network serialization format, the encoding (v1 or v2) is determined directly by // the value of ADDRV2_FORMAT in the stream version, as no explicitly encoded version // exists in the stream. assert(s.GetType() & SER_NETWORK); use_v2 = s.GetVersion() & ADDRV2_FORMAT; - // The only time we serialize a CAddress object without nTime is in - // the initial VERSION messages which contain two CAddress records. - // At that point, the serialization version is INIT_PROTO_VERSION. - // After the version handshake, serialization version is >= - // MIN_PEER_PROTO_VERSION and all ADDR messages are serialized with - // nTime. - store_time = s.GetVersion() != INIT_PROTO_VERSION; } SER_READ(obj, obj.nTime = TIME_INIT); - if (store_time) READWRITE(obj.nTime); + READWRITE(obj.nTime); // nServices is serialized as CompactSize in V2; as uint64_t in V1. if (use_v2) { uint64_t services_tmp; From cf8f17e423a1346f968abec4061948b925c274cc Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 5 Nov 2020 05:05:32 -0500 Subject: [PATCH 04/14] merge bitcoin#22735: Don't return an optional from TransportDeserializer::GetMessage() --- src/net.cpp | 49 ++++++++++--------- src/net.h | 4 +- src/test/fuzz/p2p_transport_serialization.cpp | 20 ++++---- 3 files changed, 36 insertions(+), 37 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 6679b26a8b..573ab5adcc 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -766,26 +766,27 @@ bool CNode::ReceiveMsgBytes(Span msg_bytes, bool& complete) if (m_deserializer->Complete()) { // decompose a transport agnostic CNetMessage from the deserializer - uint32_t out_err_raw_size{0}; - std::optional result{m_deserializer->GetMessage(time, out_err_raw_size)}; - if (!result) { + bool reject_message{false}; + CNetMessage msg = m_deserializer->GetMessage(time, reject_message); + if (reject_message) { // Message deserialization failed. Drop the message but don't disconnect the peer. // store the size of the corrupt message - mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER)->second += out_err_raw_size; + mapRecvBytesPerMsgCmd.at(NET_MESSAGE_COMMAND_OTHER) += msg.m_raw_message_size; continue; } - //store received bytes per message command - //to prevent a memory DOS, only allow valid commands - mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(result->m_command); - if (i == mapRecvBytesPerMsgCmd.end()) + // Store received bytes per message command + // to prevent a memory DOS, only allow valid commands + auto i = mapRecvBytesPerMsgCmd.find(msg.m_command); + if (i == mapRecvBytesPerMsgCmd.end()) { i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER); + } assert(i != mapRecvBytesPerMsgCmd.end()); - i->second += result->m_raw_message_size; - statsClient.count("bandwidth.message." + std::string(result->m_command) + ".bytesReceived", result->m_raw_message_size, 1.0f); + i->second += msg.m_raw_message_size; + statsClient.count("bandwidth.message." + std::string(msg.m_command) + ".bytesReceived", msg.m_raw_message_size, 1.0f); // push the message to the process queue, - vRecvMsg.push_back(std::move(*result)); + vRecvMsg.push_back(std::move(msg)); complete = true; } @@ -859,16 +860,18 @@ const uint256& V1TransportDeserializer::GetMessageHash() const return data_hash; } -std::optional V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, uint32_t& out_err_raw_size) +CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, bool& reject_message) { + // Initialize out parameter + reject_message = false; // decompose a single CNetMessage from the TransportDeserializer - std::optional msg(std::move(vRecv)); + CNetMessage msg(std::move(vRecv)); // store command string, time, and sizes - msg->m_command = hdr.GetCommand(); - msg->m_time = time; - msg->m_message_size = hdr.nMessageSize; - msg->m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE; + msg.m_command = hdr.GetCommand(); + msg.m_time = time; + msg.m_message_size = hdr.nMessageSize; + msg.m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE; uint256 hash = GetMessageHash(); @@ -878,17 +881,15 @@ std::optional V1TransportDeserializer::GetMessage(const std::chrono // Check checksum and header command string if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) { LogPrint(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n", - SanitizeString(msg->m_command), msg->m_message_size, + SanitizeString(msg.m_command), msg.m_message_size, HexStr(Span{hash}.first(CMessageHeader::CHECKSUM_SIZE)), HexStr(hdr.pchChecksum), m_node_id); - out_err_raw_size = msg->m_raw_message_size; - msg.reset(); + reject_message = true; } else if (!hdr.IsCommandValid()) { LogPrint(BCLog::NET, "Header error: Invalid message type (%s, %u bytes), peer=%d\n", - SanitizeString(hdr.GetCommand()), msg->m_message_size, m_node_id); - out_err_raw_size = msg->m_raw_message_size; - msg.reset(); + SanitizeString(hdr.GetCommand()), msg.m_message_size, m_node_id); + reject_message = true; } // Always reset the network deserializer (prepare for the next message) @@ -4115,7 +4116,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr s LogPrint(BCLog::NET, "Added connection peer=%d\n", id); } - m_deserializer = std::make_unique(V1TransportDeserializer(Params(), GetId(), SER_NETWORK, INIT_PROTO_VERSION)); + m_deserializer = std::make_unique(V1TransportDeserializer(Params(), id, SER_NETWORK, INIT_PROTO_VERSION)); m_serializer = std::make_unique(V1TransportSerializer()); } diff --git a/src/net.h b/src/net.h index 22edb30c14..e6691d156a 100644 --- a/src/net.h +++ b/src/net.h @@ -347,7 +347,7 @@ public: /** read and deserialize data, advances msg_bytes data pointer */ virtual int Read(Span& msg_bytes) = 0; // decomposes a message from the context - virtual std::optional GetMessage(std::chrono::microseconds time, uint32_t& out_err) = 0; + virtual CNetMessage GetMessage(std::chrono::microseconds time, bool& reject_message) = 0; virtual ~TransportDeserializer() {} }; @@ -411,7 +411,7 @@ public: } return ret; } - std::optional GetMessage(std::chrono::microseconds time, uint32_t& out_err_raw_size) override; + CNetMessage GetMessage(std::chrono::microseconds time, bool& reject_message) override; }; /** The TransportSerializer prepares messages for the network transport diff --git a/src/test/fuzz/p2p_transport_serialization.cpp b/src/test/fuzz/p2p_transport_serialization.cpp index 543bc160a5..00b4f1b1db 100644 --- a/src/test/fuzz/p2p_transport_serialization.cpp +++ b/src/test/fuzz/p2p_transport_serialization.cpp @@ -68,18 +68,16 @@ FUZZ_TARGET_INIT(p2p_transport_serialization, initialize_p2p_transport_serializa } if (deserializer.Complete()) { const std::chrono::microseconds m_time{std::numeric_limits::max()}; - uint32_t out_err_raw_size{0}; - std::optional result{deserializer.GetMessage(m_time, out_err_raw_size)}; - if (result) { - assert(result->m_command.size() <= CMessageHeader::COMMAND_SIZE); - assert(result->m_raw_message_size <= mutable_msg_bytes.size()); - assert(result->m_raw_message_size == CMessageHeader::HEADER_SIZE + result->m_message_size); - assert(result->m_time == m_time); + bool reject_message{false}; + CNetMessage msg = deserializer.GetMessage(m_time, reject_message); + assert(msg.m_command.size() <= CMessageHeader::COMMAND_SIZE); + assert(msg.m_raw_message_size <= mutable_msg_bytes.size()); + assert(msg.m_raw_message_size == CMessageHeader::HEADER_SIZE + msg.m_message_size); + assert(msg.m_time == m_time); - std::vector header; - auto msg = CNetMsgMaker{result->m_recv.GetVersion()}.Make(result->m_command, MakeUCharSpan(result->m_recv)); - serializer.prepareForTransport(msg, header); - } + std::vector header; + auto msg2 = CNetMsgMaker{msg.m_recv.GetVersion()}.Make(msg.m_command, MakeUCharSpan(msg.m_recv)); + serializer.prepareForTransport(msg2, header); } } } From 7beeae77b9a835a038267b332e37a89b6af38213 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 11 Jun 2024 20:10:00 +0000 Subject: [PATCH 05/14] merge bitcoin#23614: add unit test for block-relay-only eviction --- src/test/denialofservice_tests.cpp | 72 ++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 4 deletions(-) diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 457106d0f5..961e5bb459 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -130,7 +130,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) peerLogic->FinalizeNode(dummyNode1); } -static void AddRandomOutboundPeer(std::vector& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman) +static void AddRandomOutboundPeer(std::vector& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType) { CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE); vNodes.emplace_back(new CNode{id++, @@ -141,7 +141,7 @@ static void AddRandomOutboundPeer(std::vector& vNodes, PeerManager& peer /*nLocalHostNonceIn=*/0, CAddress(), /*addrNameIn=*/"", - ConnectionType::OUTBOUND_FULL_RELAY, + connType, /*inbound_onion=*/false}); CNode &node = *vNodes.back(); node.SetCommonVersion(PROTOCOL_VERSION); @@ -172,7 +172,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) // Mock some outbound peers for (int i = 0; i < max_outbound_full_relay; ++i) { - AddRandomOutboundPeer(vNodes, *peerLogic, *connman); + AddRandomOutboundPeer(vNodes, *peerLogic, *connman, ConnectionType::OUTBOUND_FULL_RELAY); } peerLogic->CheckForStaleTipAndEvictPeers(); @@ -197,7 +197,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) // If we add one more peer, something should get marked for eviction // on the next check (since we're mocking the time to be in the future, the // required time connected check should be satisfied). - AddRandomOutboundPeer(vNodes, *peerLogic, *connman); + AddRandomOutboundPeer(vNodes, *peerLogic, *connman, ConnectionType::OUTBOUND_FULL_RELAY); peerLogic->CheckForStaleTipAndEvictPeers(); for (int i = 0; i < max_outbound_full_relay; ++i) { @@ -226,6 +226,70 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) connman->ClearTestNodes(); } +BOOST_AUTO_TEST_CASE(block_relay_only_eviction) +{ + const CChainParams& chainparams = Params(); + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); + auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr, *m_node.scheduler, + *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync, + *m_node.govman, *m_node.sporkman, /* mn_activeman = */ nullptr, m_node.dmnman, + m_node.cj_ctx, m_node.llmq_ctx, /* ignore_incoming_txs = */ false); + + constexpr int max_outbound_block_relay{MAX_BLOCK_RELAY_ONLY_CONNECTIONS}; + constexpr int64_t MINIMUM_CONNECT_TIME{30}; + CConnman::Options options; + options.nMaxConnections = DEFAULT_MAX_PEER_CONNECTIONS; + options.m_max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS; + options.m_max_outbound_block_relay = max_outbound_block_relay; + + connman->Init(options); + std::vector vNodes; + + // Add block-relay-only peers up to the limit + for (int i = 0; i < max_outbound_block_relay; ++i) { + AddRandomOutboundPeer(vNodes, *peerLogic, *connman, ConnectionType::BLOCK_RELAY); + } + peerLogic->CheckForStaleTipAndEvictPeers(); + + for (int i = 0; i < max_outbound_block_relay; ++i) { + BOOST_CHECK(vNodes[i]->fDisconnect == false); + } + + // Add an extra block-relay-only peer breaking the limit (mocks logic in ThreadOpenConnections) + AddRandomOutboundPeer(vNodes, *peerLogic, *connman, ConnectionType::BLOCK_RELAY); + peerLogic->CheckForStaleTipAndEvictPeers(); + + // The extra peer should only get marked for eviction after MINIMUM_CONNECT_TIME + for (int i = 0; i < max_outbound_block_relay; ++i) { + BOOST_CHECK(vNodes[i]->fDisconnect == false); + } + BOOST_CHECK(vNodes.back()->fDisconnect == false); + + SetMockTime(GetTime() + MINIMUM_CONNECT_TIME + 1); + peerLogic->CheckForStaleTipAndEvictPeers(); + for (int i = 0; i < max_outbound_block_relay; ++i) { + BOOST_CHECK(vNodes[i]->fDisconnect == false); + } + BOOST_CHECK(vNodes.back()->fDisconnect == true); + + // Update the last block time for the extra peer, + // and check that the next youngest peer gets evicted. + vNodes.back()->fDisconnect = false; + vNodes.back()->nLastBlockTime = GetTime(); + + peerLogic->CheckForStaleTipAndEvictPeers(); + for (int i = 0; i < max_outbound_block_relay - 1; ++i) { + BOOST_CHECK(vNodes[i]->fDisconnect == false); + } + BOOST_CHECK(vNodes[max_outbound_block_relay - 1]->fDisconnect == true); + BOOST_CHECK(vNodes.back()->fDisconnect == false); + + for (const CNode* node : vNodes) { + peerLogic->FinalizeNode(*node); + } + connman->ClearTestNodes(); +} + BOOST_AUTO_TEST_CASE(peer_discouragement) { const CChainParams& chainparams = Params(); From 6e6c9442fa33e61e969529212866db55a88564e1 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 11 Jun 2024 20:10:36 +0000 Subject: [PATCH 06/14] merge bitcoin#23758: Use type-safe mockable time for peer connection time --- src/bench/peer_eviction.cpp | 12 ++--- src/masternode/utils.cpp | 4 +- src/net.cpp | 36 ++++++------- src/net.h | 28 +++++----- src/net_processing.cpp | 32 ++++++------ src/qt/rpcconsole.cpp | 6 +-- src/rpc/net.cpp | 6 +-- src/test/denialofservice_tests.cpp | 9 +++- src/test/fuzz/node_eviction.cpp | 6 +-- src/test/net_peer_eviction_tests.cpp | 78 ++++++++++++++-------------- src/test/util/net.cpp | 6 +-- 11 files changed, 115 insertions(+), 108 deletions(-) diff --git a/src/bench/peer_eviction.cpp b/src/bench/peer_eviction.cpp index 0469f0cb4c..f3f21e0472 100644 --- a/src/bench/peer_eviction.cpp +++ b/src/bench/peer_eviction.cpp @@ -43,7 +43,7 @@ static void EvictionProtection0Networks250Candidates(benchmark::Bench& bench) bench, 250 /* num_candidates */, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_network = NET_IPV4; }); } @@ -54,7 +54,7 @@ static void EvictionProtection1Networks250Candidates(benchmark::Bench& bench) bench, 250 /* num_candidates */, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = false; if (c.id >= 130 && c.id < 240) { // 110 Tor c.m_network = NET_ONION; @@ -70,7 +70,7 @@ static void EvictionProtection2Networks250Candidates(benchmark::Bench& bench) bench, 250 /* num_candidates */, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = false; if (c.id >= 90 && c.id < 160) { // 70 Tor c.m_network = NET_ONION; @@ -88,7 +88,7 @@ static void EvictionProtection3Networks050Candidates(benchmark::Bench& bench) bench, 50 /* num_candidates */, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 28 || c.id == 47); // 2 localhost if (c.id >= 30 && c.id < 47) { // 17 I2P c.m_network = NET_I2P; @@ -106,7 +106,7 @@ static void EvictionProtection3Networks100Candidates(benchmark::Bench& bench) bench, 100 /* num_candidates */, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id >= 55 && c.id < 60); // 5 localhost if (c.id >= 70 && c.id < 80) { // 10 I2P c.m_network = NET_I2P; @@ -124,7 +124,7 @@ static void EvictionProtection3Networks250Candidates(benchmark::Bench& bench) bench, 250 /* num_candidates */, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id >= 140 && c.id < 160); // 20 localhost if (c.id >= 170 && c.id < 180) { // 10 I2P c.m_network = NET_I2P; diff --git a/src/masternode/utils.cpp b/src/masternode/utils.cpp index 3d20a7b4b7..48a3d19b8e 100644 --- a/src/masternode/utils.cpp +++ b/src/masternode/utils.cpp @@ -49,7 +49,7 @@ void CMasternodeUtils::DoMaintenance(CConnman& connman, CDeterministicMNManager& connman.ForEachNode(CConnman::AllNodes, [&](CNode* pnode) { if (pnode->m_masternode_probe_connection) { // we're not disconnecting masternode probes for at least PROBE_WAIT_INTERVAL seconds - if (GetTimeSeconds() - pnode->nTimeConnected < PROBE_WAIT_INTERVAL) return; + if (GetTime() - pnode->m_connected < PROBE_WAIT_INTERVAL) return; } else { // we're only disconnecting m_masternode_connection connections if (!pnode->m_masternode_connection) return; @@ -67,7 +67,7 @@ void CMasternodeUtils::DoMaintenance(CConnman& connman, CDeterministicMNManager& if (pnode->IsInboundConn()) { return; } - } else if (GetTimeSeconds() - pnode->nTimeConnected < 5) { + } else if (GetTime() - pnode->m_connected < 5s) { // non-verified, give it some time to verify itself return; } else if (pnode->qwatch) { diff --git a/src/net.cpp b/src/net.cpp index 573ab5adcc..f6f3e429b7 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -704,9 +704,9 @@ void CNode::CopyStats(CNodeStats& stats) stats.m_network = ConnectedThroughNetwork(); X(m_last_send); X(m_last_recv); - X(nLastTXTime); - X(nLastBlockTime); - X(nTimeConnected); + X(m_last_tx_time); + X(m_last_block_time); + X(m_connected); X(nTimeOffset); X(m_addr_name); X(nVersion); @@ -972,7 +972,7 @@ static bool ReverseCompareNodeMinPingTime(const NodeEvictionCandidate& a, const static bool ReverseCompareNodeTimeConnected(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b) { - return a.nTimeConnected > b.nTimeConnected; + return a.m_connected > b.m_connected; } static bool CompareNetGroupKeyed(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) { @@ -982,27 +982,27 @@ static bool CompareNetGroupKeyed(const NodeEvictionCandidate &a, const NodeEvict static bool CompareNodeBlockTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) { // There is a fall-through here because it is common for a node to have many peers which have not yet relayed a block. - if (a.nLastBlockTime != b.nLastBlockTime) return a.nLastBlockTime < b.nLastBlockTime; + if (a.m_last_block_time != b.m_last_block_time) return a.m_last_block_time < b.m_last_block_time; if (a.fRelevantServices != b.fRelevantServices) return b.fRelevantServices; - return a.nTimeConnected > b.nTimeConnected; + return a.m_connected > b.m_connected; } static bool CompareNodeTXTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) { // There is a fall-through here because it is common for a node to have more than a few peers that have not yet relayed txn. - if (a.nLastTXTime != b.nLastTXTime) return a.nLastTXTime < b.nLastTXTime; + if (a.m_last_tx_time != b.m_last_tx_time) return a.m_last_tx_time < b.m_last_tx_time; if (a.m_relay_txs != b.m_relay_txs) return b.m_relay_txs; if (a.fBloomFilter != b.fBloomFilter) return a.fBloomFilter; - return a.nTimeConnected > b.nTimeConnected; + return a.m_connected > b.m_connected; } // Pick out the potential block-relay only peers, and sort them by last block time. static bool CompareNodeBlockRelayOnlyTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) { if (a.m_relay_txs != b.m_relay_txs) return a.m_relay_txs; - if (a.nLastBlockTime != b.nLastBlockTime) return a.nLastBlockTime < b.nLastBlockTime; + if (a.m_last_block_time != b.m_last_block_time) return a.m_last_block_time < b.m_last_block_time; if (a.fRelevantServices != b.fRelevantServices) return b.fRelevantServices; - return a.nTimeConnected > b.nTimeConnected; + return a.m_connected > b.m_connected; } /** @@ -1021,7 +1021,7 @@ struct CompareNodeNetworkTime { { if (m_is_local && a.m_is_local != b.m_is_local) return b.m_is_local; if ((a.m_network == m_network) != (b.m_network == m_network)) return b.m_network == m_network; - return a.nTimeConnected > b.nTimeConnected; + return a.m_connected > b.m_connected; }; }; @@ -1148,12 +1148,12 @@ void ProtectEvictionCandidatesByRatio(std::vector& evicti // (vEvictionCandidates is already sorted by reverse connect time) uint64_t naMostConnections; unsigned int nMostConnections = 0; - int64_t nMostConnectionsTime = 0; + std::chrono::seconds nMostConnectionsTime{0}; std::map > mapNetGroupNodes; for (const NodeEvictionCandidate &node : vEvictionCandidates) { std::vector &group = mapNetGroupNodes[node.nKeyedNetGroup]; group.push_back(node); - const int64_t grouptime = group[0].nTimeConnected; + const auto grouptime{group[0].m_connected}; if (group.size() > nMostConnections || (group.size() == nMostConnections && grouptime > nMostConnectionsTime)) { nMostConnections = group.size(); @@ -1196,7 +1196,7 @@ bool CConnman::AttemptToEvictConnection() // was accepted. This short time is meant for the VERSION/VERACK exchange and the possible MNAUTH that might // follow when the incoming connection is from another masternode. When a message other than MNAUTH // is received after VERSION/VERACK, the protection is lifted immediately. - bool isProtected = GetTimeSeconds() - node->nTimeConnected < INBOUND_EVICTION_PROTECTION_TIME; + bool isProtected = GetTime() - node->m_connected < INBOUND_EVICTION_PROTECTION_TIME; if (node->nTimeFirstMessageReceived != 0 && !node->fFirstMessageIsMNAUTH) { isProtected = false; } @@ -1210,8 +1210,8 @@ bool CConnman::AttemptToEvictConnection() } } - NodeEvictionCandidate candidate = {node->GetId(), node->nTimeConnected, node->m_min_ping_time, - node->nLastBlockTime, node->nLastTXTime, + NodeEvictionCandidate candidate = {node->GetId(), node->m_connected, node->m_min_ping_time, + node->m_last_block_time, node->m_last_tx_time, HasAllDesirableServiceFlags(node->nServices), node->m_relays_txs.load(), node->m_bloom_filter_loaded.load(), node->nKeyedNetGroup, node->m_prefer_evict, node->addr.IsLocal(), @@ -1644,7 +1644,7 @@ void CConnman::CalculateNumConnectionsChangedStats() bool CConnman::ShouldRunInactivityChecks(const CNode& node, std::chrono::seconds now) const { - return std::chrono::seconds{node.nTimeConnected} + m_peer_connect_timeout < now; + return node.m_connected + m_peer_connect_timeout < now; } bool CConnman::InactivityCheck(const CNode& node) const @@ -4092,7 +4092,7 @@ unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, std::unique_ptr&& i2p_sam_session) : m_sock{sock}, - nTimeConnected{GetTimeSeconds()}, + m_connected{GetTime()}, addr{addrIn}, addrBind{addrBindIn}, m_addr_name{addrNameIn.empty() ? addr.ToStringIPPort() : addrNameIn}, diff --git a/src/net.h b/src/net.h index e6691d156a..9e9cde3126 100644 --- a/src/net.h +++ b/src/net.h @@ -63,8 +63,8 @@ static const bool DEFAULT_WHITELISTFORCERELAY = false; /** Time after which to disconnect, after waiting for a ping response (or inactivity). */ static constexpr std::chrono::minutes TIMEOUT_INTERVAL{20}; -/** Time to wait since nTimeConnected before disconnecting a probe node. **/ -static const int PROBE_WAIT_INTERVAL = 5; +/** Time to wait since m_connected before disconnecting a probe node. */ +static const auto PROBE_WAIT_INTERVAL{5s}; /** Minimum time between warnings printed to log. */ static const int WARNING_INTERVAL = 10 * 60; /** Run the feeler connection loop once every 2 minutes. **/ @@ -82,7 +82,7 @@ static const int MAX_OUTBOUND_FULL_RELAY_CONNECTIONS = 8; /** Maximum number of addnode outgoing nodes */ static const int MAX_ADDNODE_CONNECTIONS = 8; /** Eviction protection time for incoming connections */ -static const int INBOUND_EVICTION_PROTECTION_TIME = 1; +static const auto INBOUND_EVICTION_PROTECTION_TIME{1s}; /** Maximum number of block-relay-only outgoing connections */ static const int MAX_BLOCK_RELAY_ONLY_CONNECTIONS = 2; /** Maximum number of feeler connections */ @@ -276,9 +276,9 @@ public: ServiceFlags nServices; std::chrono::seconds m_last_send; std::chrono::seconds m_last_recv; - int64_t nLastTXTime; - int64_t nLastBlockTime; - int64_t nTimeConnected; + std::chrono::seconds m_last_tx_time; + std::chrono::seconds m_last_block_time; + std::chrono::seconds m_connected; int64_t nTimeOffset; std::string m_addr_name; int nVersion; @@ -472,8 +472,8 @@ public: std::atomic m_last_send{0s}; std::atomic m_last_recv{0s}; - //! Unix epoch time at peer connection, in seconds. - const int64_t nTimeConnected; + //! Unix epoch time at peer connection + const std::chrono::seconds m_connected; std::atomic nTimeOffset{0}; std::atomic nLastWarningTime{0}; std::atomic nTimeFirstMessageReceived{0}; @@ -513,7 +513,7 @@ public: std::atomic m_masternode_connection{false}; /** * If 'true' this node will be disconnected after MNAUTH (outbound only) or - * after PROBE_WAIT_INTERVAL seconds since nTimeConnected + * after PROBE_WAIT_INTERVAL seconds since m_connected */ std::atomic m_masternode_probe_connection{false}; // If 'true', we identified it as an intra-quorum relay connection @@ -615,13 +615,13 @@ public: * preliminary validity checks and was saved to disk, even if we don't * connect the block or it eventually fails connection. Used as an inbound * peer eviction criterium in CConnman::AttemptToEvictConnection. */ - std::atomic nLastBlockTime{0}; + std::atomic m_last_block_time{0s}; /** UNIX epoch time of the last transaction received from this peer that we * had not yet seen (e.g. not already received from another peer) and that * was accepted into our mempool. Used as an inbound peer eviction criterium * in CConnman::AttemptToEvictConnection. */ - std::atomic nLastTXTime{0}; + std::atomic m_last_tx_time{0s}; /** Last measured round-trip time. Used only for RPC/GUI stats/debugging.*/ std::atomic m_last_ping_time{0us}; @@ -1628,10 +1628,10 @@ extern std::function(); } else { LOCK(cs_main); mapBlockSource.erase(pblock->GetHash()); @@ -4136,7 +4136,7 @@ void PeerManagerImpl::ProcessMessage( } } - pfrom.nLastTXTime = GetTime(); + pfrom.m_last_tx_time = GetTime(); LogPrint(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (poolsz %u txn, %u kB)\n", pfrom.GetId(), @@ -5087,7 +5087,7 @@ void PeerManagerImpl::ConsiderEviction(CNode& pto, int64_t time_in_seconds) } } -void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds) +void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now) { // If we have any extra block-relay-only peers, disconnect the youngest unless // it's given us a block -- in which case, compare with the second-youngest, and @@ -5096,14 +5096,14 @@ void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds) // to temporarily in order to sync our tip; see net.cpp. // Note that we use higher nodeid as a measure for most recent connection. if (m_connman.GetExtraBlockRelayCount() > 0) { - std::pair youngest_peer{-1, 0}, next_youngest_peer{-1, 0}; + std::pair youngest_peer{-1, 0}, next_youngest_peer{-1, 0}; m_connman.ForEachNode([&](CNode* pnode) { if (!pnode->IsBlockOnlyConn() || pnode->fDisconnect) return; if (pnode->GetId() > youngest_peer.first) { next_youngest_peer = youngest_peer; youngest_peer.first = pnode->GetId(); - youngest_peer.second = pnode->nLastBlockTime; + youngest_peer.second = pnode->m_last_block_time; } }); NodeId to_disconnect = youngest_peer.first; @@ -5121,13 +5121,14 @@ void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds) // valid headers chain with at least as much work as our tip. CNodeState *node_state = State(pnode->GetId()); if (node_state == nullptr || - (time_in_seconds - pnode->nTimeConnected >= MINIMUM_CONNECT_TIME && node_state->nBlocksInFlight == 0)) { + (now - pnode->m_connected >= MINIMUM_CONNECT_TIME && node_state->nBlocksInFlight == 0)) { pnode->fDisconnect = true; - LogPrint(BCLog::NET, "disconnecting extra block-relay-only peer=%d (last block received at time %d)\n", pnode->GetId(), pnode->nLastBlockTime); + LogPrint(BCLog::NET, "disconnecting extra block-relay-only peer=%d (last block received at time %d)\n", + pnode->GetId(), count_seconds(pnode->m_last_block_time)); return true; } else { LogPrint(BCLog::NET, "keeping block-relay-only peer=%d chosen for eviction (connect time: %d, blocks_in_flight: %d)\n", - pnode->GetId(), pnode->nTimeConnected, node_state->nBlocksInFlight); + pnode->GetId(), count_seconds(pnode->m_connected), node_state->nBlocksInFlight); } return false; }); @@ -5169,12 +5170,13 @@ void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds) // Also don't disconnect any peer we're trying to download a // block from. CNodeState &state = *State(pnode->GetId()); - if (time_in_seconds - pnode->nTimeConnected > MINIMUM_CONNECT_TIME && state.nBlocksInFlight == 0) { + if (now - pnode->m_connected > MINIMUM_CONNECT_TIME && state.nBlocksInFlight == 0) { LogPrint(BCLog::NET, "disconnecting extra outbound peer=%d (last block announcement received at time %d)\n", pnode->GetId(), oldest_block_announcement); pnode->fDisconnect = true; return true; } else { - LogPrint(BCLog::NET, "keeping outbound peer=%d chosen for eviction (connect time: %d, blocks_in_flight: %d)\n", pnode->GetId(), pnode->nTimeConnected, state.nBlocksInFlight); + LogPrint(BCLog::NET, "keeping outbound peer=%d chosen for eviction (connect time: %d, blocks_in_flight: %d)\n", + pnode->GetId(), count_seconds(pnode->m_connected), state.nBlocksInFlight); return false; } }); @@ -5196,7 +5198,7 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers() int64_t time_in_seconds = GetTime(); - EvictExtraOutboundPeers(time_in_seconds); + EvictExtraOutboundPeers(std::chrono::seconds{time_in_seconds}); if (time_in_seconds > m_stale_tip_check_time) { // Check whether our tip is stale, and if so, allow using an extra @@ -5379,7 +5381,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) const auto current_time = GetTime(); - if (pto->IsAddrFetchConn() && current_time - std::chrono::seconds(pto->nTimeConnected) > 10 * AVG_ADDRESS_BROADCAST_INTERVAL) { + if (pto->IsAddrFetchConn() && current_time - pto->m_connected > 10 * AVG_ADDRESS_BROADCAST_INTERVAL) { LogPrint(BCLog::NET_NETCONN, "addrfetch connection timeout; disconnecting peer=%d\n", pto->GetId()); pto->fDisconnect = true; return true; diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index 26bfa85751..478477597c 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1285,9 +1285,9 @@ void RPCConsole::updateDetailWidget() ui->peerHeading->setText(peerAddrDetails); ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStats.nServices)); const auto time_now{GetTime()}; - ui->peerConnTime->setText(GUIUtil::formatDurationStr(time_now - std::chrono::seconds{stats->nodeStats.nTimeConnected})); - ui->peerLastBlock->setText(TimeDurationField(time_now, std::chrono::seconds{stats->nodeStats.nLastBlockTime})); - ui->peerLastTx->setText(TimeDurationField(time_now, std::chrono::seconds{stats->nodeStats.nLastTXTime})); + ui->peerConnTime->setText(GUIUtil::formatDurationStr(time_now - stats->nodeStats.m_connected)); + ui->peerLastBlock->setText(TimeDurationField(time_now, stats->nodeStats.m_last_block_time)); + ui->peerLastTx->setText(TimeDurationField(time_now, stats->nodeStats.m_last_tx_time)); QString bip152_hb_settings; if (stats->nodeStats.m_bip152_highbandwidth_to) bip152_hb_settings += "To"; if (stats->nodeStats.m_bip152_highbandwidth_from) bip152_hb_settings += (bip152_hb_settings == "" ? "From" : "/From"); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 9f25c8dfd5..290a5de426 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -225,11 +225,11 @@ static RPCHelpMan getpeerinfo() obj.pushKV("servicesnames", GetServicesNames(stats.nServices)); obj.pushKV("lastsend", count_seconds(stats.m_last_send)); obj.pushKV("lastrecv", count_seconds(stats.m_last_recv)); - obj.pushKV("last_transaction", stats.nLastTXTime); - obj.pushKV("last_block", stats.nLastBlockTime); + obj.pushKV("last_transaction", count_seconds(stats.m_last_tx_time)); + obj.pushKV("last_block", count_seconds(stats.m_last_block_time)); obj.pushKV("bytessent", stats.nSendBytes); obj.pushKV("bytesrecv", stats.nRecvBytes); - obj.pushKV("conntime", stats.nTimeConnected); + obj.pushKV("conntime", count_seconds(stats.m_connected)); obj.pushKV("timeoffset", stats.nTimeOffset); if (stats.m_last_ping_time > 0us) { obj.pushKV("pingtime", CountSecondsDouble(stats.m_last_ping_time)); diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 961e5bb459..72dd09af01 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -167,6 +167,9 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) options.m_max_outbound_full_relay = max_outbound_full_relay; options.nMaxFeeler = MAX_FEELER_CONNECTIONS; + const auto time_init{GetTime()}; + SetMockTime(time_init); + const auto time_later{time_init + 3 * std::chrono::seconds{chainparams.GetConsensus().nPowTargetSpacing} + 1s}; connman->Init(options); std::vector vNodes; @@ -182,7 +185,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) BOOST_CHECK(node->fDisconnect == false); } - SetMockTime(GetTime() + 3 * chainparams.GetConsensus().nPowTargetSpacing + 1); + SetMockTime(time_later); // Now tip should definitely be stale, and we should look for an extra // outbound peer @@ -197,7 +200,9 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) // If we add one more peer, something should get marked for eviction // on the next check (since we're mocking the time to be in the future, the // required time connected check should be satisfied). + SetMockTime(time_init); AddRandomOutboundPeer(vNodes, *peerLogic, *connman, ConnectionType::OUTBOUND_FULL_RELAY); + SetMockTime(time_later); peerLogic->CheckForStaleTipAndEvictPeers(); for (int i = 0; i < max_outbound_full_relay; ++i) { @@ -275,7 +280,7 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction) // Update the last block time for the extra peer, // and check that the next youngest peer gets evicted. vNodes.back()->fDisconnect = false; - vNodes.back()->nLastBlockTime = GetTime(); + vNodes.back()->m_last_block_time = GetTime(); peerLogic->CheckForStaleTipAndEvictPeers(); for (int i = 0; i < max_outbound_block_relay - 1; ++i) { diff --git a/src/test/fuzz/node_eviction.cpp b/src/test/fuzz/node_eviction.cpp index 161dfd64fa..0835b399e0 100644 --- a/src/test/fuzz/node_eviction.cpp +++ b/src/test/fuzz/node_eviction.cpp @@ -21,10 +21,10 @@ FUZZ_TARGET(node_eviction) while (fuzzed_data_provider.ConsumeBool()) { eviction_candidates.push_back({ /* id */ fuzzed_data_provider.ConsumeIntegral(), - /* nTimeConnected */ fuzzed_data_provider.ConsumeIntegral(), + /* m_connected */ std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral()}, /* m_min_ping_time */ std::chrono::microseconds{fuzzed_data_provider.ConsumeIntegral()}, - /* nLastBlockTime */ fuzzed_data_provider.ConsumeIntegral(), - /* nLastTXTime */ fuzzed_data_provider.ConsumeIntegral(), + /* m_last_block_time */ std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral()}, + /* m_last_tx_time */ std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral()}, /* fRelevantServices */ fuzzed_data_provider.ConsumeBool(), /* m_relay_txs */ fuzzed_data_provider.ConsumeBool(), /* fBloomFilter */ fuzzed_data_provider.ConsumeBool(), diff --git a/src/test/net_peer_eviction_tests.cpp b/src/test/net_peer_eviction_tests.cpp index ca24a4070c..6cca764127 100644 --- a/src/test/net_peer_eviction_tests.cpp +++ b/src/test/net_peer_eviction_tests.cpp @@ -64,11 +64,11 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) FastRandomContext random_context{true}; int num_peers{12}; - // Expect half of the peers with greatest uptime (the lowest nTimeConnected) + // Expect half of the peers with greatest uptime (the lowest m_connected) // to be protected from eviction. BOOST_CHECK(IsProtected( num_peers, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = false; c.m_network = NET_IPV4; }, @@ -79,7 +79,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // Verify in the opposite direction. BOOST_CHECK(IsProtected( num_peers, [num_peers](NodeEvictionCandidate& c) { - c.nTimeConnected = num_peers - c.id; + c.m_connected = std::chrono::seconds{num_peers - c.id}; c.m_is_local = false; c.m_network = NET_IPV6; }, @@ -101,10 +101,10 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) random_context)); // Expect 1/4 onion peers and 1/4 of the other peers to be protected, - // sorted by longest uptime (lowest nTimeConnected), if no localhost, I2P or CJDNS peers. + // sorted by longest uptime (lowest m_connected), if no localhost, I2P or CJDNS peers. BOOST_CHECK(IsProtected( num_peers, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = false; c.m_network = (c.id == 3 || c.id > 7) ? NET_ONION : NET_IPV6; }, @@ -124,10 +124,10 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) random_context)); // Expect 1/4 localhost peers and 1/4 of the other peers to be protected, - // sorted by longest uptime (lowest nTimeConnected), if no onion, I2P, or CJDNS peers. + // sorted by longest uptime (lowest m_connected), if no onion, I2P, or CJDNS peers. BOOST_CHECK(IsProtected( num_peers, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id > 6); c.m_network = NET_IPV6; }, @@ -147,10 +147,10 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) random_context)); // Expect 1/4 I2P peers and 1/4 of the other peers to be protected, sorted - // by longest uptime (lowest nTimeConnected), if no onion, localhost, or CJDNS peers. + // by longest uptime (lowest m_connected), if no onion, localhost, or CJDNS peers. BOOST_CHECK(IsProtected( num_peers, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = false; c.m_network = (c.id == 4 || c.id > 8) ? NET_I2P : NET_IPV6; }, @@ -170,10 +170,10 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) random_context)); // Expect 1/4 CJDNS peers and 1/4 of the other peers to be protected, sorted - // by longest uptime (lowest nTimeConnected), if no onion, localhost, or I2P peers. + // by longest uptime (lowest m_connected), if no onion, localhost, or I2P peers. BOOST_CHECK(IsProtected( num_peers, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = false; c.m_network = (c.id == 4 || c.id > 8) ? NET_CJDNS : NET_IPV6; }, @@ -188,7 +188,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // stable sort breaks tie with array order of localhost first. BOOST_CHECK(IsProtected( 4, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 4); c.m_network = (c.id == 3) ? NET_ONION : NET_IPV4; }, @@ -201,7 +201,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // uptime; stable sort breaks tie with array order of localhost first. BOOST_CHECK(IsProtected( 7, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 6); c.m_network = (c.id == 5) ? NET_ONION : NET_IPV4; }, @@ -214,7 +214,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // by uptime; stable sort breaks tie with array order of localhost first. BOOST_CHECK(IsProtected( 8, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 6); c.m_network = (c.id == 5) ? NET_ONION : NET_IPV4; }, @@ -227,7 +227,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // uptime; stable sort breaks ties with the array order of localhost first. BOOST_CHECK(IsProtected( num_peers, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 6 || c.id == 9 || c.id == 11); c.m_network = (c.id == 7 || c.id == 8 || c.id == 10) ? NET_ONION : NET_IPV6; }, @@ -239,7 +239,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // protect 2 localhost and 1 onion, plus 3 other peers, sorted by longest uptime. BOOST_CHECK(IsProtected( num_peers, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id > 4 && c.id < 9); c.m_network = (c.id == 10) ? NET_ONION : NET_IPV4; }, @@ -251,7 +251,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // protect 2 localhost and 2 onions, plus 4 other peers, sorted by longest uptime. BOOST_CHECK(IsProtected( 16, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 6 || c.id == 9 || c.id == 11 || c.id == 12); c.m_network = (c.id == 8 || c.id == 10) ? NET_ONION : NET_IPV6; }, @@ -264,7 +264,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // others, sorted by longest uptime. BOOST_CHECK(IsProtected( 16, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id > 10); c.m_network = (c.id == 10) ? NET_ONION : NET_IPV4; }, @@ -277,7 +277,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // plus 4 others, sorted by longest uptime. BOOST_CHECK(IsProtected( 16, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 15); c.m_network = (c.id > 6 && c.id < 11) ? NET_ONION : NET_IPV6; }, @@ -290,7 +290,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // others, sorted by longest uptime. BOOST_CHECK(IsProtected( num_peers, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = false; if (c.id == 8 || c.id == 10) { c.m_network = NET_ONION; @@ -311,7 +311,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // by longest uptime; stable sort breaks tie with array order of I2P first. BOOST_CHECK(IsProtected( 4, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 2); if (c.id == 3) { c.m_network = NET_I2P; @@ -330,7 +330,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // by longest uptime; stable sort breaks tie with array order of I2P first. BOOST_CHECK(IsProtected( 7, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 4); if (c.id == 6) { c.m_network = NET_I2P; @@ -349,7 +349,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // by uptime; stable sort breaks tie with array order of I2P then localhost. BOOST_CHECK(IsProtected( 8, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 6); if (c.id == 5) { c.m_network = NET_I2P; @@ -368,7 +368,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // for 8 total, sorted by longest uptime. BOOST_CHECK(IsProtected( 16, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 6 || c.id > 11); if (c.id == 7 || c.id == 11) { c.m_network = NET_I2P; @@ -387,7 +387,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // sorted by longest uptime. BOOST_CHECK(IsProtected( 24, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 12); if (c.id > 14 && c.id < 23) { // 4 protected instead of usual 2 c.m_network = NET_I2P; @@ -406,7 +406,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // unused localhost slot), plus 6 others for 12/24 total, sorted by longest uptime. BOOST_CHECK(IsProtected( 24, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 15); if (c.id == 12 || c.id == 14 || c.id == 17) { c.m_network = NET_I2P; @@ -425,7 +425,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // for 12/24 total, sorted by longest uptime. BOOST_CHECK(IsProtected( 24, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 13); if (c.id > 16) { c.m_network = NET_I2P; @@ -444,7 +444,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // sorted by longest uptime. BOOST_CHECK(IsProtected( 24, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id > 15); if (c.id > 10 && c.id < 15) { c.m_network = NET_CJDNS; @@ -466,7 +466,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // order of CJDNS first. BOOST_CHECK(IsProtected( 5, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 3); if (c.id == 4) { c.m_network = NET_CJDNS; @@ -488,7 +488,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // array order of CJDNS first. BOOST_CHECK(IsProtected( 7, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 4); if (c.id == 6) { c.m_network = NET_CJDNS; @@ -510,7 +510,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // array order of CJDNS first. BOOST_CHECK(IsProtected( 8, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 3); if (c.id == 5) { c.m_network = NET_CJDNS; @@ -531,7 +531,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // total), plus 4 others for 8 total, sorted by longest uptime. BOOST_CHECK(IsProtected( 16, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id > 5); if (c.id == 11 || c.id == 15) { c.m_network = NET_CJDNS; @@ -552,7 +552,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test) // total), plus 6 others for 12/24 total, sorted by longest uptime. BOOST_CHECK(IsProtected( 24, [](NodeEvictionCandidate& c) { - c.nTimeConnected = c.id; + c.m_connected = std::chrono::seconds{c.id}; c.m_is_local = (c.id == 13); if (c.id > 17) { c.m_network = NET_CJDNS; @@ -617,7 +617,7 @@ BOOST_AUTO_TEST_CASE(peer_eviction_test) // into our mempool should be protected from eviction. BOOST_CHECK(!IsEvicted( number_of_nodes, [number_of_nodes](NodeEvictionCandidate& candidate) { - candidate.nLastTXTime = number_of_nodes - candidate.id; + candidate.m_last_tx_time = std::chrono::seconds{number_of_nodes - candidate.id}; }, {0, 1, 2, 3}, random_context)); @@ -625,7 +625,7 @@ BOOST_AUTO_TEST_CASE(peer_eviction_test) // blocks should be protected from eviction. BOOST_CHECK(!IsEvicted( number_of_nodes, [number_of_nodes](NodeEvictionCandidate& candidate) { - candidate.nLastBlockTime = number_of_nodes - candidate.id; + candidate.m_last_block_time = std::chrono::seconds{number_of_nodes - candidate.id}; if (candidate.id <= 7) { candidate.m_relay_txs = false; candidate.fRelevantServices = true; @@ -637,14 +637,14 @@ BOOST_AUTO_TEST_CASE(peer_eviction_test) // protected from eviction. BOOST_CHECK(!IsEvicted( number_of_nodes, [number_of_nodes](NodeEvictionCandidate& candidate) { - candidate.nLastBlockTime = number_of_nodes - candidate.id; + candidate.m_last_block_time = std::chrono::seconds{number_of_nodes - candidate.id}; }, {0, 1, 2, 3}, random_context)); // Combination of the previous two tests. BOOST_CHECK(!IsEvicted( number_of_nodes, [number_of_nodes](NodeEvictionCandidate& candidate) { - candidate.nLastBlockTime = number_of_nodes - candidate.id; + candidate.m_last_block_time = std::chrono::seconds{number_of_nodes - candidate.id}; if (candidate.id <= 7) { candidate.m_relay_txs = false; candidate.fRelevantServices = true; @@ -657,8 +657,8 @@ BOOST_AUTO_TEST_CASE(peer_eviction_test) number_of_nodes, [number_of_nodes](NodeEvictionCandidate& candidate) { candidate.nKeyedNetGroup = number_of_nodes - candidate.id; // 4 protected candidate.m_min_ping_time = std::chrono::microseconds{candidate.id}; // 8 protected - candidate.nLastTXTime = number_of_nodes - candidate.id; // 4 protected - candidate.nLastBlockTime = number_of_nodes - candidate.id; // 4 protected + candidate.m_last_tx_time = std::chrono::seconds{number_of_nodes - candidate.id}; // 4 protected + candidate.m_last_block_time = std::chrono::seconds{number_of_nodes - candidate.id}; // 4 protected }, {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19}, random_context)); diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 7d44f783fa..c71377f821 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -47,10 +47,10 @@ std::vector GetRandomNodeEvictionCandidates(int n_candida for (int id = 0; id < n_candidates; ++id) { candidates.push_back({ /* id */ id, - /* nTimeConnected */ static_cast(random_context.randrange(100)), + /* m_connected */ std::chrono::seconds{random_context.randrange(100)}, /* m_min_ping_time */ std::chrono::microseconds{random_context.randrange(100)}, - /* nLastBlockTime */ static_cast(random_context.randrange(100)), - /* nLastTXTime */ static_cast(random_context.randrange(100)), + /* m_last_block_time */ std::chrono::seconds{random_context.randrange(100)}, + /* m_last_tx_time */ std::chrono::seconds{random_context.randrange(100)}, /* fRelevantServices */ random_context.randbool(), /* m_relay_txs */ random_context.randbool(), /* fBloomFilter */ random_context.randbool(), From 182e31d04c243d72cb5306f1a9bbd21970b42577 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 12 Jun 2024 16:15:22 +0000 Subject: [PATCH 07/14] merge bitcoin#23801: Change time variable type from int64_t to std::chrono::seconds in net_processing.cpp --- src/net_processing.cpp | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b96c6ed6f1..8180d48fe9 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -34,6 +34,8 @@ #include #include +#include +#include #include #include #include @@ -101,10 +103,10 @@ static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1ms; static constexpr int32_t MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT = 4; /** Timeout for (unprotected) outbound peers to sync to our chainwork, in seconds */ static constexpr int64_t CHAIN_SYNC_TIMEOUT = 20 * 60; // 20 minutes -/** How frequently to check for stale tips, in seconds */ -static constexpr int64_t STALE_CHECK_INTERVAL = 2.5 * 60; // 2.5 minutes (~block interval) -/** How frequently to check for extra outbound peers and disconnect, in seconds */ -static constexpr int64_t EXTRA_PEER_CHECK_INTERVAL = 45; +/** How frequently to check for stale tips */ +static constexpr auto STALE_CHECK_INTERVAL{150s}; // 2.5 minutes (~block interval) +/** How frequently to check for extra outbound peers and disconnect */ +static constexpr auto EXTRA_PEER_CHECK_INTERVAL{45s}; /** Minimum time an outbound-peer-eviction candidate must be connected for, in order to evict */ static constexpr std::chrono::seconds MINIMUM_CONNECT_TIME{30}; /** SHA256("main address relay")[0:8] */ @@ -517,7 +519,7 @@ private: std::atomic m_best_height{-1}; /** Next time to check for stale tip */ - int64_t m_stale_tip_check_time{0}; + std::chrono::seconds m_stale_tip_check_time{0s}; /** Whether this node is running in blocks only mode */ const bool m_ignore_incoming_txs; @@ -685,7 +687,7 @@ private: std::map::iterator> > mapBlocksInFlight GUARDED_BY(cs_main); /** When our tip was last updated. */ - std::atomic m_last_tip_update{0}; + std::atomic m_last_tip_update{0s}; /** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */ CTransactionRef FindTxForGetData(const CNode* peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main); @@ -1108,10 +1110,10 @@ bool PeerManagerImpl::TipMayBeStale() { AssertLockHeld(cs_main); const Consensus::Params& consensusParams = m_chainparams.GetConsensus(); - if (m_last_tip_update == 0) { - m_last_tip_update = GetTime(); + if (count_seconds(m_last_tip_update) == 0) { + m_last_tip_update = GetTime(); } - return m_last_tip_update < GetTime() - consensusParams.nPowTargetSpacing * 3 && mapBlocksInFlight.empty(); + return count_seconds(m_last_tip_update) < GetTime() - consensusParams.nPowTargetSpacing * 3 && mapBlocksInFlight.empty(); } bool PeerManagerImpl::CanDirectFetch() @@ -1956,7 +1958,7 @@ void PeerManagerImpl::BlockConnected(const std::shared_ptr& pblock ProcessOrphanTx(orphanWorkSet); } - m_last_tip_update = GetTime(); + m_last_tip_update = GetTime(); } { LOCK(m_recent_confirmed_transactions_mutex); @@ -5196,20 +5198,20 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers() { LOCK(cs_main); - int64_t time_in_seconds = GetTime(); + auto now{GetTime()}; - EvictExtraOutboundPeers(std::chrono::seconds{time_in_seconds}); + EvictExtraOutboundPeers(now); - if (time_in_seconds > m_stale_tip_check_time) { + if (now > m_stale_tip_check_time) { // Check whether our tip is stale, and if so, allow using an extra // outbound peer if (!fImporting && !fReindex && m_connman.GetNetworkActive() && m_connman.GetUseAddrmanOutgoing() && TipMayBeStale()) { - LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", time_in_seconds - m_last_tip_update); + LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", count_seconds(now) - count_seconds(m_last_tip_update)); m_connman.SetTryNewOutboundPeer(true); } else if (m_connman.GetTryNewOutboundPeer()) { m_connman.SetTryNewOutboundPeer(false); } - m_stale_tip_check_time = time_in_seconds + STALE_CHECK_INTERVAL; + m_stale_tip_check_time = now + STALE_CHECK_INTERVAL; } if (!m_initial_sync_finished && CanDirectFetch()) { From c443cf48257b93b313aa97016980b2f1a494e005 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 12 Jun 2024 16:15:36 +0000 Subject: [PATCH 08/14] merge bitcoin#24078: Rename CNetMessage::m_command with CNetMessage::m_type --- src/net.cpp | 8 ++++---- src/net.h | 4 ++-- src/net_processing.cpp | 12 ++++-------- src/test/fuzz/p2p_transport_serialization.cpp | 4 ++-- 4 files changed, 12 insertions(+), 16 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index f6f3e429b7..501fef5a84 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -777,13 +777,13 @@ bool CNode::ReceiveMsgBytes(Span msg_bytes, bool& complete) // Store received bytes per message command // to prevent a memory DOS, only allow valid commands - auto i = mapRecvBytesPerMsgCmd.find(msg.m_command); + auto i = mapRecvBytesPerMsgCmd.find(msg.m_type); if (i == mapRecvBytesPerMsgCmd.end()) { i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER); } assert(i != mapRecvBytesPerMsgCmd.end()); i->second += msg.m_raw_message_size; - statsClient.count("bandwidth.message." + std::string(msg.m_command) + ".bytesReceived", msg.m_raw_message_size, 1.0f); + statsClient.count("bandwidth.message." + std::string(msg.m_type) + ".bytesReceived", msg.m_raw_message_size, 1.0f); // push the message to the process queue, vRecvMsg.push_back(std::move(msg)); @@ -868,7 +868,7 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds CNetMessage msg(std::move(vRecv)); // store command string, time, and sizes - msg.m_command = hdr.GetCommand(); + msg.m_type = hdr.GetCommand(); msg.m_time = time; msg.m_message_size = hdr.nMessageSize; msg.m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE; @@ -881,7 +881,7 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds // Check checksum and header command string if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) { LogPrint(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n", - SanitizeString(msg.m_command), msg.m_message_size, + SanitizeString(msg.m_type), msg.m_message_size, HexStr(Span{hash}.first(CMessageHeader::CHECKSUM_SIZE)), HexStr(hdr.pchChecksum), m_node_id); diff --git a/src/net.h b/src/net.h index 9e9cde3126..1adf20b296 100644 --- a/src/net.h +++ b/src/net.h @@ -316,7 +316,7 @@ public: /** Transport protocol agnostic message container. * Ideally it should only contain receive time, payload, - * command and size. + * type and size. */ class CNetMessage { public: @@ -324,7 +324,7 @@ public: std::chrono::microseconds m_time{0}; //!< time of message receipt uint32_t m_message_size{0}; //!< size of the payload uint32_t m_raw_message_size{0}; //!< used wire size of the message (including header/checksum) - std::string m_command; + std::string m_type; CNetMessage(CDataStream&& recv_in) : m_recv(std::move(recv_in)) {} diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8180d48fe9..77dbcfaeeb 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5006,26 +5006,22 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt CNetMessage& msg(msgs.front()); if (gArgs.GetBoolArg("-capturemessages", false)) { - CaptureMessage(pfrom->addr, msg.m_command, MakeUCharSpan(msg.m_recv), /* incoming */ true); + CaptureMessage(pfrom->addr, msg.m_type, MakeUCharSpan(msg.m_recv), /* incoming */ true); } msg.SetVersion(pfrom->GetCommonVersion()); - const std::string& msg_type = msg.m_command; - - // Message size - unsigned int nMessageSize = msg.m_message_size; try { - ProcessMessage(*pfrom, msg_type, msg.m_recv, msg.m_time, interruptMsgProc); + ProcessMessage(*pfrom, msg.m_type, msg.m_recv, msg.m_time, interruptMsgProc); if (interruptMsgProc) return false; { LOCK(peer->m_getdata_requests_mutex); if (!peer->m_getdata_requests.empty()) fMoreWork = true; } } catch (const std::exception& e) { - LogPrint(BCLog::NET, "%s(%s, %u bytes): Exception '%s' (%s) caught\n", __func__, SanitizeString(msg_type), nMessageSize, e.what(), typeid(e).name()); + LogPrint(BCLog::NET, "%s(%s, %u bytes): Exception '%s' (%s) caught\n", __func__, SanitizeString(msg.m_type), msg.m_message_size, e.what(), typeid(e).name()); } catch (...) { - LogPrint(BCLog::NET, "%s(%s, %u bytes): Unknown exception caught\n", __func__, SanitizeString(msg_type), nMessageSize); + LogPrint(BCLog::NET, "%s(%s, %u bytes): Unknown exception caught\n", __func__, SanitizeString(msg.m_type), msg.m_message_size); } return fMoreWork; diff --git a/src/test/fuzz/p2p_transport_serialization.cpp b/src/test/fuzz/p2p_transport_serialization.cpp index 00b4f1b1db..8247bbabc4 100644 --- a/src/test/fuzz/p2p_transport_serialization.cpp +++ b/src/test/fuzz/p2p_transport_serialization.cpp @@ -70,13 +70,13 @@ FUZZ_TARGET_INIT(p2p_transport_serialization, initialize_p2p_transport_serializa const std::chrono::microseconds m_time{std::numeric_limits::max()}; bool reject_message{false}; CNetMessage msg = deserializer.GetMessage(m_time, reject_message); - assert(msg.m_command.size() <= CMessageHeader::COMMAND_SIZE); + assert(msg.m_type.size() <= CMessageHeader::COMMAND_SIZE); assert(msg.m_raw_message_size <= mutable_msg_bytes.size()); assert(msg.m_raw_message_size == CMessageHeader::HEADER_SIZE + msg.m_message_size); assert(msg.m_time == m_time); std::vector header; - auto msg2 = CNetMsgMaker{msg.m_recv.GetVersion()}.Make(msg.m_command, MakeUCharSpan(msg.m_recv)); + auto msg2 = CNetMsgMaker{msg.m_recv.GetVersion()}.Make(msg.m_type, MakeUCharSpan(msg.m_recv)); serializer.prepareForTransport(msg2, header); } } From a6aa3735be49eb34b46f6eed6ebd52361adf8256 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 12 Jun 2024 16:22:21 +0000 Subject: [PATCH 09/14] merge bitcoin#20196: fix GetListenPort() to derive the proper port continuation of 24205d94fe51093067cf9dd64c29d78147e4619c from dash#5982 includes: - 0cfc0cd32239d3c08d2121e028b297022450b320 - 7d64ea4a01920bb55bc6de0de6766712ec792a11 --- src/init.cpp | 15 +- src/net.cpp | 37 +++- src/net.h | 8 + src/net_processing.cpp | 4 + src/test/net_tests.cpp | 191 +++++++++++++++++- test/functional/feature_bind_port_discover.py | 78 +++++++ .../feature_bind_port_externalip.py | 75 +++++++ test/functional/test_runner.py | 2 + 8 files changed, 400 insertions(+), 10 deletions(-) create mode 100755 test/functional/feature_bind_port_discover.py create mode 100755 test/functional/feature_bind_port_externalip.py diff --git a/src/init.cpp b/src/init.cpp index 04442f2db2..3fc7eef7c8 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -2477,8 +2477,6 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc LogPrintf("::ChainActive().Height() = %d\n", chain_active_height); if (node.peerman) node.peerman->SetBestHeight(chain_active_height); - Discover(); - // Map ports with UPnP or NAT-PMP. StartMapPort(args.GetBoolArg("-upnp", DEFAULT_UPNP), args.GetBoolArg("-natpmp", DEFAULT_NATPMP)); @@ -2495,15 +2493,18 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc connOptions.nSendBufferMaxSize = 1000 * args.GetArg("-maxsendbuffer", DEFAULT_MAXSENDBUFFER); connOptions.nReceiveFloodSize = 1000 * args.GetArg("-maxreceivebuffer", DEFAULT_MAXRECEIVEBUFFER); connOptions.m_added_nodes = args.GetArgs("-addnode"); - connOptions.nMaxOutboundLimit = 1024 * 1024 * args.GetArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET); connOptions.m_peer_connect_timeout = peer_connect_timeout; + // Port to bind to if `-bind=addr` is provided without a `:port` suffix. + const uint16_t default_bind_port = + static_cast(args.GetArg("-port", Params().GetDefaultPort())); + for (const std::string& bind_arg : args.GetArgs("-bind")) { CService bind_addr; const size_t index = bind_arg.rfind('='); if (index == std::string::npos) { - if (Lookup(bind_arg, bind_addr, GetListenPort(), false)) { + if (Lookup(bind_arg, bind_addr, default_bind_port, /*fAllowLookup=*/false)) { connOptions.vBinds.push_back(bind_addr); continue; } @@ -2548,6 +2549,12 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc StartTorControl(onion_service_target); } + if (connOptions.bind_on_any) { + // Only add all IP addresses of the machine if we would be listening on + // any address - 0.0.0.0 (IPv4) and :: (IPv6). + Discover(); + } + for (const auto& net : args.GetArgs("-whitelist")) { NetWhitelistPermissions subnet; bilingual_str error; diff --git a/src/net.cpp b/src/net.cpp index 501fef5a84..8824b606f5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -151,6 +151,31 @@ void CConnman::AddAddrFetch(const std::string& strDest) uint16_t GetListenPort() { + // If -bind= is provided with ":port" part, use that (first one if multiple are provided). + for (const std::string& bind_arg : gArgs.GetArgs("-bind")) { + CService bind_addr; + constexpr uint16_t dummy_port = 0; + + if (Lookup(bind_arg, bind_addr, dummy_port, /*fAllowLookup=*/false)) { + if (bind_addr.GetPort() != dummy_port) { + return bind_addr.GetPort(); + } + } + } + + // Otherwise, if -whitebind= without NetPermissionFlags::NoBan is provided, use that + // (-whitebind= is required to have ":port"). + for (const std::string& whitebind_arg : gArgs.GetArgs("-whitebind")) { + NetWhitebindPermissions whitebind; + bilingual_str error; + if (NetWhitebindPermissions::TryParse(whitebind_arg, whitebind, error)) { + if (!NetPermissions::HasFlag(whitebind.m_flags, NetPermissionFlags::NoBan)) { + return whitebind.m_service.GetPort(); + } + } + } + + // Otherwise, if -port= is provided, use that. Otherwise use the default port. return static_cast(gArgs.GetArg("-port", Params().GetDefaultPort())); } @@ -246,7 +271,17 @@ std::optional GetLocalAddrForPeer(CNode *pnode) if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() || rng.randbits((GetnScore(addrLocal) > LOCAL_MANUAL) ? 3 : 1) == 0)) { - addrLocal.SetIP(pnode->GetAddrLocal()); + if (pnode->IsInboundConn()) { + // For inbound connections, assume both the address and the port + // as seen from the peer. + addrLocal = CAddress{pnode->GetAddrLocal(), addrLocal.nServices}; + } else { + // For outbound connections, assume just the address as seen from + // the peer and leave the port in `addrLocal` as returned by + // `GetLocalAddress()` above. The peer has no way to observe our + // listening port when we have initiated the connection. + addrLocal.SetIP(pnode->GetAddrLocal()); + } } if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false)) { diff --git a/src/net.h b/src/net.h index 1adf20b296..9ec6199638 100644 --- a/src/net.h +++ b/src/net.h @@ -215,7 +215,15 @@ enum class ConnectionType { /** Convert ConnectionType enum to a string value */ std::string ConnectionTypeAsString(ConnectionType conn_type); + +/** + * Look up IP addresses from all interfaces on the machine and add them to the + * list of local addresses to self-advertise. + * The loopback interface is skipped and only the first address from each + * interface is used. + */ void Discover(); + uint16_t GetListenPort(); enum diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 77dbcfaeeb..a8c4f52de1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3444,6 +3444,10 @@ void PeerManagerImpl::ProcessMessage( LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString()); PushAddress(*peer, addr, insecure_rand); } else if (IsPeerAddrLocalGood(&pfrom)) { + // Override just the address with whatever the peer sees us as. + // Leave the port in addr as it was returned by GetLocalAddress() + // above, as this is an outbound connection and the peer cannot + // observe our listening port. addr.SetIP(addrMe); LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString()); PushAddress(*peer, addr, insecure_rand); diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 06f37a1468..050cb3e4b4 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -6,15 +6,21 @@ #include #include +#include #include +#include #include #include +#include #include #include #include +#include +#include #include #include #include +#include #include #include @@ -28,7 +34,7 @@ using namespace std::literals; -BOOST_FIXTURE_TEST_SUITE(net_tests, BasicTestingSetup) +BOOST_FIXTURE_TEST_SUITE(net_tests, RegTestingSetup) BOOST_AUTO_TEST_CASE(cnode_listen_port) { @@ -608,15 +614,15 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test) // set up local addresses; all that's necessary to reproduce the bug is // that a normal IPv4 address is among the entries, but if this address is // !IsRoutable the undefined behavior is easier to trigger deterministically + in_addr raw_addr; + raw_addr.s_addr = htonl(0x7f000001); + const CNetAddr mapLocalHost_entry = CNetAddr(raw_addr); { LOCK(g_maplocalhost_mutex); - in_addr ipv4AddrLocal; - ipv4AddrLocal.s_addr = 0x0100007f; - CNetAddr addr = CNetAddr(ipv4AddrLocal); LocalServiceInfo lsi; lsi.nScore = 23; lsi.nPort = 42; - mapLocalHost[addr] = lsi; + mapLocalHost[mapLocalHost_entry] = lsi; } // create a peer with an IPv4 address @@ -647,8 +653,79 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test) // suppress no-checks-run warning; if this test fails, it's by triggering a sanitizer BOOST_CHECK(1); + + // Cleanup, so that we don't confuse other tests. + { + LOCK(g_maplocalhost_mutex); + mapLocalHost.erase(mapLocalHost_entry); + } } +BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port) +{ + // Test that GetLocalAddrForPeer() properly selects the address to self-advertise: + // + // 1. GetLocalAddrForPeer() calls GetLocalAddress() which returns an address that is + // not routable. + // 2. GetLocalAddrForPeer() overrides the address with whatever the peer has told us + // he sees us as. + // 2.1. For inbound connections we must override both the address and the port. + // 2.2. For outbound connections we must override only the address. + + // Pretend that we bound to this port. + const uint16_t bind_port = 20001; + m_node.args->ForceSetArg("-bind", strprintf("3.4.5.6:%u", bind_port)); + + // Our address:port as seen from the peer, completely different from the above. + in_addr peer_us_addr; + peer_us_addr.s_addr = htonl(0x02030405); + const CAddress peer_us{CService{peer_us_addr, 20002}, NODE_NETWORK}; + + // Create a peer with a routable IPv4 address (outbound). + in_addr peer_out_in_addr; + peer_out_in_addr.s_addr = htonl(0x01020304); + CNode peer_out{/*id=*/0, + /*nLocalServicesIn=*/NODE_NETWORK, + /*sock=*/nullptr, + /*addrIn=*/CAddress{CService{peer_out_in_addr, 8333}, NODE_NETWORK}, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + /*addrBindIn=*/CAddress{}, + /*addrNameIn=*/std::string{}, + /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY, + /*inbound_onion=*/false}; + peer_out.fSuccessfullyConnected = true; + peer_out.SetAddrLocal(peer_us); + + // Without the fix peer_us:8333 is chosen instead of the proper peer_us:bind_port. + auto chosen_local_addr = GetLocalAddrForPeer(&peer_out); + BOOST_REQUIRE(chosen_local_addr); + const CService expected{peer_us_addr, bind_port}; + BOOST_CHECK(*chosen_local_addr == expected); + + // Create a peer with a routable IPv4 address (inbound). + in_addr peer_in_in_addr; + peer_in_in_addr.s_addr = htonl(0x05060708); + CNode peer_in{/*id=*/0, + /*nLocalServicesIn=*/NODE_NETWORK, + /*sock=*/nullptr, + /*addrIn=*/CAddress{CService{peer_in_in_addr, 8333}, NODE_NETWORK}, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + /*addrBindIn=*/CAddress{}, + /*addrNameIn=*/std::string{}, + /*conn_type_in=*/ConnectionType::INBOUND, + /*inbound_onion=*/false}; + peer_in.fSuccessfullyConnected = true; + peer_in.SetAddrLocal(peer_us); + + // Without the fix peer_us:8333 is chosen instead of the proper peer_us:peer_us.GetPort(). + chosen_local_addr = GetLocalAddrForPeer(&peer_in); + BOOST_REQUIRE(chosen_local_addr); + BOOST_CHECK(*chosen_local_addr == peer_us); + + m_node.args->ForceSetArg("-bind", ""); +} BOOST_AUTO_TEST_CASE(LimitedAndReachable_Network) { @@ -734,4 +811,108 @@ BOOST_AUTO_TEST_CASE(LocalAddress_BasicLifecycle) BOOST_CHECK(!IsLocal(addr)); } +BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) +{ + // Tests the following scenario: + // * -bind=3.4.5.6:20001 is specified + // * we make an outbound connection to a peer + // * the peer reports he sees us as 2.3.4.5:20002 in the version message + // (20002 is a random port assigned by our OS for the outgoing TCP connection, + // we cannot accept connections to it) + // * we should self-advertise to that peer as 2.3.4.5:20001 + + // Pretend that we bound to this port. + const uint16_t bind_port = 20001; + m_node.args->ForceSetArg("-bind", strprintf("3.4.5.6:%u", bind_port)); + m_node.args->ForceSetArg("-capturemessages", "1"); + + // Our address:port as seen from the peer - 2.3.4.5:20002 (different from the above). + in_addr peer_us_addr; + peer_us_addr.s_addr = htonl(0x02030405); + const CService peer_us{peer_us_addr, 20002}; + + // Create a peer with a routable IPv4 address. + in_addr peer_in_addr; + peer_in_addr.s_addr = htonl(0x01020304); + CNode peer{/*id=*/0, + /*nLocalServicesIn=*/NODE_NETWORK, + /*sock=*/nullptr, + /*addrIn=*/CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK}, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + /*addrBindIn=*/CAddress{}, + /*addrNameIn=*/std::string{}, + /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY, + /*inbound_onion=*/false}; + + const uint64_t services{NODE_NETWORK}; + const int64_t time{0}; + const CNetMsgMaker msg_maker{PROTOCOL_VERSION}; + + // Force CChainState::IsInitialBlockDownload() to return false. + // Otherwise PushAddress() isn't called by PeerManager::ProcessMessage(). + TestChainState& chainstate = + *static_cast(&m_node.chainman->ActiveChainstate()); + chainstate.JumpOutOfIbd(); + + m_node.peerman->InitializeNode(&peer); + + std::atomic interrupt_dummy{false}; + std::chrono::microseconds time_received_dummy{0}; + + const auto msg_version = + msg_maker.Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, peer_us); + CDataStream msg_version_stream{msg_version.data, SER_NETWORK, PROTOCOL_VERSION}; + + m_node.peerman->ProcessMessage( + peer, NetMsgType::VERSION, msg_version_stream, time_received_dummy, interrupt_dummy); + + const auto msg_verack = msg_maker.Make(NetMsgType::VERACK); + CDataStream msg_verack_stream{msg_verack.data, SER_NETWORK, PROTOCOL_VERSION}; + + // Will set peer.fSuccessfullyConnected to true (necessary in SendMessages()). + m_node.peerman->ProcessMessage( + peer, NetMsgType::VERACK, msg_verack_stream, time_received_dummy, interrupt_dummy); + + // Ensure that peer_us_addr:bind_port is sent to the peer. + const CService expected{peer_us_addr, bind_port}; + bool sent{false}; + + const auto CaptureMessageOrig = CaptureMessage; + CaptureMessage = [&sent, &expected](const CAddress& addr, + const std::string& msg_type, + Span data, + bool is_incoming) -> void { + if (!is_incoming && msg_type == "addr") { + CDataStream s(data, SER_NETWORK, PROTOCOL_VERSION); + std::vector addresses; + + s >> addresses; + + for (const auto& addr : addresses) { + if (addr == expected) { + sent = true; + return; + } + } + } + }; + + { + LOCK(peer.cs_sendProcessing); + m_node.peerman->SendMessages(&peer); + } + + BOOST_CHECK(sent); + + CaptureMessage = CaptureMessageOrig; + chainstate.ResetIbd(); + m_node.args->ForceSetArg("-capturemessages", "0"); + m_node.args->ForceSetArg("-bind", ""); + // PeerManager::ProcessMessage() calls AddTimeData() which changes the internal state + // in timedata.cpp and later confuses the test "timedata_tests/addtimedata". Thus reset + // that state as it was before our test was run. + TestOnlyResetTimeData(); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/test/functional/feature_bind_port_discover.py b/test/functional/feature_bind_port_discover.py new file mode 100755 index 0000000000..6e07f2f16c --- /dev/null +++ b/test/functional/feature_bind_port_discover.py @@ -0,0 +1,78 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020-2021 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +""" +Test that -discover does not add all interfaces' addresses if we listen on only some of them +""" + +from test_framework.test_framework import BitcoinTestFramework, SkipTest +from test_framework.util import assert_equal + +# We need to bind to a routable address for this test to exercise the relevant code +# and also must have another routable address on another interface which must not +# be named "lo" or "lo0". +# To set these routable addresses on the machine, use: +# Linux: +# ifconfig lo:0 1.1.1.1/32 up && ifconfig lo:1 2.2.2.2/32 up # to set up +# ifconfig lo:0 down && ifconfig lo:1 down # to remove it, after the test +# FreeBSD: +# ifconfig em0 1.1.1.1/32 alias && ifconfig wlan0 2.2.2.2/32 alias # to set up +# ifconfig em0 1.1.1.1 -alias && ifconfig wlan0 2.2.2.2 -alias # to remove it, after the test +ADDR1 = '1.1.1.1' +ADDR2 = '2.2.2.2' + +BIND_PORT = 31001 + +class BindPortDiscoverTest(BitcoinTestFramework): + def set_test_params(self): + # Avoid any -bind= on the command line. Force the framework to avoid adding -bind=127.0.0.1. + self.setup_clean_chain = True + self.bind_to_localhost_only = False + self.extra_args = [ + ['-discover', f'-port={BIND_PORT}'], # bind on any + ['-discover', f'-bind={ADDR1}:{BIND_PORT}'], + ] + self.num_nodes = len(self.extra_args) + + def add_options(self, parser): + parser.add_argument( + "--ihave1111and2222", action='store_true', dest="ihave1111and2222", + help=f"Run the test, assuming {ADDR1} and {ADDR2} are configured on the machine", + default=False) + + def skip_test_if_missing_module(self): + if not self.options.ihave1111and2222: + raise SkipTest( + f"To run this test make sure that {ADDR1} and {ADDR2} (routable addresses) are " + "assigned to the interfaces on this machine and rerun with --ihave1111and2222") + + def run_test(self): + self.log.info( + "Test that if -bind= is not passed then all addresses are " + "added to localaddresses") + found_addr1 = False + found_addr2 = False + for local in self.nodes[0].getnetworkinfo()['localaddresses']: + if local['address'] == ADDR1: + found_addr1 = True + assert_equal(local['port'], BIND_PORT) + if local['address'] == ADDR2: + found_addr2 = True + assert_equal(local['port'], BIND_PORT) + assert found_addr1 + assert found_addr2 + + self.log.info( + "Test that if -bind= is passed then only that address is " + "added to localaddresses") + found_addr1 = False + for local in self.nodes[1].getnetworkinfo()['localaddresses']: + if local['address'] == ADDR1: + found_addr1 = True + assert_equal(local['port'], BIND_PORT) + assert local['address'] != ADDR2 + assert found_addr1 + +if __name__ == '__main__': + BindPortDiscoverTest().main() diff --git a/test/functional/feature_bind_port_externalip.py b/test/functional/feature_bind_port_externalip.py new file mode 100755 index 0000000000..6a74ce5738 --- /dev/null +++ b/test/functional/feature_bind_port_externalip.py @@ -0,0 +1,75 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020-2021 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +""" +Test that the proper port is used for -externalip= +""" + +from test_framework.test_framework import BitcoinTestFramework, SkipTest +from test_framework.util import assert_equal, p2p_port + +# We need to bind to a routable address for this test to exercise the relevant code. +# To set a routable address on the machine use: +# Linux: +# ifconfig lo:0 1.1.1.1/32 up # to set up +# ifconfig lo:0 down # to remove it, after the test +# FreeBSD: +# ifconfig lo0 1.1.1.1/32 alias # to set up +# ifconfig lo0 1.1.1.1 -alias # to remove it, after the test +ADDR = '1.1.1.1' + +# array of tuples [arguments, expected port in localaddresses] +EXPECTED = [ + [['-externalip=2.2.2.2', '-port=30001'], 30001], + [['-externalip=2.2.2.2', '-port=30002', f'-bind={ADDR}'], 30002], + [['-externalip=2.2.2.2', f'-bind={ADDR}'], 'default_p2p_port'], + [['-externalip=2.2.2.2', '-port=30003', f'-bind={ADDR}:30004'], 30004], + [['-externalip=2.2.2.2', f'-bind={ADDR}:30005'], 30005], + [['-externalip=2.2.2.2:30006', '-port=30007'], 30006], + [['-externalip=2.2.2.2:30008', '-port=30009', f'-bind={ADDR}'], 30008], + [['-externalip=2.2.2.2:30010', f'-bind={ADDR}'], 30010], + [['-externalip=2.2.2.2:30011', '-port=30012', f'-bind={ADDR}:30013'], 30011], + [['-externalip=2.2.2.2:30014', f'-bind={ADDR}:30015'], 30014], + [['-externalip=2.2.2.2', '-port=30016', f'-bind={ADDR}:30017', + f'-whitebind={ADDR}:30018'], 30017], + [['-externalip=2.2.2.2', '-port=30019', + f'-whitebind={ADDR}:30020'], 30020], +] + +class BindPortExternalIPTest(BitcoinTestFramework): + def set_test_params(self): + # Avoid any -bind= on the command line. Force the framework to avoid adding -bind=127.0.0.1. + self.setup_clean_chain = True + self.bind_to_localhost_only = False + self.num_nodes = len(EXPECTED) + self.extra_args = list(map(lambda e: e[0], EXPECTED)) + + def add_options(self, parser): + parser.add_argument( + "--ihave1111", action='store_true', dest="ihave1111", + help=f"Run the test, assuming {ADDR} is configured on the machine", + default=False) + + def skip_test_if_missing_module(self): + if not self.options.ihave1111: + raise SkipTest( + f"To run this test make sure that {ADDR} (a routable address) is assigned " + "to one of the interfaces on this machine and rerun with --ihave1111") + + def run_test(self): + self.log.info("Test the proper port is used for -externalip=") + for i in range(len(EXPECTED)): + expected_port = EXPECTED[i][1] + if expected_port == 'default_p2p_port': + expected_port = p2p_port(i) + found = False + for local in self.nodes[i].getnetworkinfo()['localaddresses']: + if local['address'] == '2.2.2.2': + assert_equal(local['port'], expected_port) + found = True + break + assert found + +if __name__ == '__main__': + BindPortExternalIPTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 193dee7c18..bcf26c453e 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -277,6 +277,7 @@ BASE_SCRIPTS = [ 'p2p_connect_to_devnet.py', 'feature_sporks.py', 'rpc_getblockstats.py', + 'feature_bind_port_externalip.py', 'wallet_encryption.py --legacy-wallet', 'wallet_encryption.py --descriptors', 'wallet_upgradetohd.py --legacy-wallet', @@ -316,6 +317,7 @@ BASE_SCRIPTS = [ 'feature_loadblock.py', 'p2p_dos_header_tree.py', 'p2p_add_connections.py', + 'feature_bind_port_discover.py', 'p2p_blockfilters.py', 'p2p_message_capture.py', 'feature_addrman.py', From b9b13bd8ec46807702c2d1e78a2a3a182c21dc41 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 7 Apr 2022 17:13:52 +0530 Subject: [PATCH 10/14] merge bitcoin#24141: Rename message_command variables in src/net* and src/rpc/net.cpp --- src/net.cpp | 44 ++++++++++++++++++++++---------------------- src/net.h | 14 +++++++------- src/rpc/net.cpp | 18 +++++++++--------- 3 files changed, 38 insertions(+), 38 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 8824b606f5..a31b2a5efc 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -125,7 +125,7 @@ static const uint64_t SELECT_TIMEOUT_MILLISECONDS = 50; static const uint64_t SELECT_TIMEOUT_MILLISECONDS = 500; #endif /* USE_WAKEUP_PIPE */ -const std::string NET_MESSAGE_COMMAND_OTHER = "*other*"; +const std::string NET_MESSAGE_TYPE_OTHER = "*other*"; constexpr const CConnman::CFullyConnectedOnly CConnman::FullyConnectedOnly; constexpr const CConnman::CAllNodes CConnman::AllNodes; @@ -755,12 +755,12 @@ void CNode::CopyStats(CNodeStats& stats) X(m_bip152_highbandwidth_from); { LOCK(cs_vSend); - X(mapSendBytesPerMsgCmd); + X(mapSendBytesPerMsgType); X(nSendBytes); } { LOCK(cs_vRecv); - X(mapRecvBytesPerMsgCmd); + X(mapRecvBytesPerMsgType); X(nRecvBytes); } X(m_legacyWhitelisted); @@ -804,19 +804,19 @@ bool CNode::ReceiveMsgBytes(Span msg_bytes, bool& complete) bool reject_message{false}; CNetMessage msg = m_deserializer->GetMessage(time, reject_message); if (reject_message) { - // Message deserialization failed. Drop the message but don't disconnect the peer. + // Message deserialization failed. Drop the message but don't disconnect the peer. // store the size of the corrupt message - mapRecvBytesPerMsgCmd.at(NET_MESSAGE_COMMAND_OTHER) += msg.m_raw_message_size; + mapRecvBytesPerMsgType.at(NET_MESSAGE_TYPE_OTHER) += msg.m_raw_message_size; continue; } - // Store received bytes per message command - // to prevent a memory DOS, only allow valid commands - auto i = mapRecvBytesPerMsgCmd.find(msg.m_type); - if (i == mapRecvBytesPerMsgCmd.end()) { - i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER); + // Store received bytes per message type. + // To prevent a memory DOS, only allow known message types. + auto i = mapRecvBytesPerMsgType.find(msg.m_type); + if (i == mapRecvBytesPerMsgType.end()) { + i = mapRecvBytesPerMsgType.find(NET_MESSAGE_TYPE_OTHER); } - assert(i != mapRecvBytesPerMsgCmd.end()); + assert(i != mapRecvBytesPerMsgType.end()); i->second += msg.m_raw_message_size; statsClient.count("bandwidth.message." + std::string(msg.m_type) + ".bytesReceived", msg.m_raw_message_size, 1.0f); @@ -902,7 +902,7 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds // decompose a single CNetMessage from the TransportDeserializer CNetMessage msg(std::move(vRecv)); - // store command string, time, and sizes + // store message type string, time, and sizes msg.m_type = hdr.GetCommand(); msg.m_time = time; msg.m_message_size = hdr.nMessageSize; @@ -913,7 +913,7 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds // We just received a message off the wire, harvest entropy from the time (and the message checksum) RandAddEvent(ReadLE32(hash.begin())); - // Check checksum and header command string + // Check checksum and header message type string if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) { LogPrint(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n", SanitizeString(msg.m_type), msg.m_message_size, @@ -1625,24 +1625,24 @@ void CConnman::CalculateNumConnectionsChangedStats() int ipv4Nodes = 0; int ipv6Nodes = 0; int torNodes = 0; - mapMsgCmdSize mapRecvBytesMsgStats; - mapMsgCmdSize mapSentBytesMsgStats; + mapMsgTypeSize mapRecvBytesMsgStats; + mapMsgTypeSize mapSentBytesMsgStats; for (const std::string &msg : getAllNetMessageTypes()) { mapRecvBytesMsgStats[msg] = 0; mapSentBytesMsgStats[msg] = 0; } - mapRecvBytesMsgStats[NET_MESSAGE_COMMAND_OTHER] = 0; - mapSentBytesMsgStats[NET_MESSAGE_COMMAND_OTHER] = 0; + mapRecvBytesMsgStats[NET_MESSAGE_TYPE_OTHER] = 0; + mapSentBytesMsgStats[NET_MESSAGE_TYPE_OTHER] = 0; const NodesSnapshot snap{*this, /* filter = */ CConnman::FullyConnectedOnly}; for (auto pnode : snap.Nodes()) { { LOCK(pnode->cs_vRecv); - for (const mapMsgCmdSize::value_type &i : pnode->mapRecvBytesPerMsgCmd) + for (const mapMsgTypeSize::value_type &i : pnode->mapRecvBytesPerMsgType) mapRecvBytesMsgStats[i.first] += i.second; } { LOCK(pnode->cs_vSend); - for (const mapMsgCmdSize::value_type &i : pnode->mapSendBytesPerMsgCmd) + for (const mapMsgTypeSize::value_type &i : pnode->mapSendBytesPerMsgType) mapSentBytesMsgStats[i.first] += i.second; } if(pnode->fClient) @@ -4142,8 +4142,8 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr s if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND); for (const std::string &msg : getAllNetMessageTypes()) - mapRecvBytesPerMsgCmd[msg] = 0; - mapRecvBytesPerMsgCmd[NET_MESSAGE_COMMAND_OTHER] = 0; + mapRecvBytesPerMsgType[msg] = 0; + mapRecvBytesPerMsgType[NET_MESSAGE_TYPE_OTHER] = 0; if (fLogIPs) { LogPrint(BCLog::NET, "Added connection to %s peer=%d\n", m_addr_name, id); @@ -4182,7 +4182,7 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg) bool hasPendingData = !pnode->vSendMsg.empty(); //log total amount of bytes per message type - pnode->mapSendBytesPerMsgCmd[msg.m_type] += nTotalSize; + pnode->mapSendBytesPerMsgType[msg.m_type] += nTotalSize; pnode->nSendSize += nTotalSize; if (pnode->nSendSize > nSendBufferMaxSize) pnode->fPauseSend = true; diff --git a/src/net.h b/src/net.h index 9ec6199638..e2d258e703 100644 --- a/src/net.h +++ b/src/net.h @@ -274,8 +274,8 @@ struct LocalServiceInfo { extern Mutex g_maplocalhost_mutex; extern std::map mapLocalHost GUARDED_BY(g_maplocalhost_mutex); -extern const std::string NET_MESSAGE_COMMAND_OTHER; -typedef std::map mapMsgCmdSize; //command, total bytes +extern const std::string NET_MESSAGE_TYPE_OTHER; +using mapMsgTypeSize = std::map; class CNodeStats { @@ -297,9 +297,9 @@ public: bool m_bip152_highbandwidth_from; int m_starting_height; uint64_t nSendBytes; - mapMsgCmdSize mapSendBytesPerMsgCmd; + mapMsgTypeSize mapSendBytesPerMsgType; uint64_t nRecvBytes; - mapMsgCmdSize mapRecvBytesPerMsgCmd; + mapMsgTypeSize mapRecvBytesPerMsgType; NetPermissionFlags m_permissionFlags; bool m_legacyWhitelisted; std::chrono::microseconds m_last_ping_time; @@ -344,7 +344,7 @@ public: /** The TransportDeserializer takes care of holding and deserializing the * network receive buffer. It can deserialize the network buffer into a - * transport protocol agnostic CNetMessage (command & payload) + * transport protocol agnostic CNetMessage (message type & payload) */ class TransportDeserializer { public: @@ -799,8 +799,8 @@ private: uint256 verifiedProRegTxHash GUARDED_BY(cs_mnauth); uint256 verifiedPubKeyHash GUARDED_BY(cs_mnauth); - mapMsgCmdSize mapSendBytesPerMsgCmd GUARDED_BY(cs_vSend); - mapMsgCmdSize mapRecvBytesPerMsgCmd GUARDED_BY(cs_vRecv); + mapMsgTypeSize mapSendBytesPerMsgType GUARDED_BY(cs_vSend); + mapMsgTypeSize mapRecvBytesPerMsgType GUARDED_BY(cs_vRecv); /** * If an I2P session is created per connection (for outbound transient I2P diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 290a5de426..ffc3f7038f 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -177,7 +177,7 @@ static RPCHelpMan getpeerinfo() { {RPCResult::Type::NUM, "msg", "The total bytes received aggregated by message type\n" "When a message type is not listed in this json object, the bytes received are 0.\n" - "Only known message types can appear as keys in the object and all bytes received of unknown message types are listed under '"+NET_MESSAGE_COMMAND_OTHER+"'."} + "Only known message types can appear as keys in the object and all bytes received of unknown message types are listed under '"+NET_MESSAGE_TYPE_OTHER+"'."} }}, {RPCResult::Type::STR, "connection_type", "Type of connection: \n" + Join(CONNECTION_TYPE_DOC, ",\n") + ".\n" "Please note this output is unlikely to be stable in upcoming releases as we iterate to\n" @@ -281,19 +281,19 @@ static RPCHelpMan getpeerinfo() } obj.pushKV("permissions", permissions); - UniValue sendPerMsgCmd(UniValue::VOBJ); - for (const auto& i : stats.mapSendBytesPerMsgCmd) { + UniValue sendPerMsgType(UniValue::VOBJ); + for (const auto& i : stats.mapSendBytesPerMsgType) { if (i.second > 0) - sendPerMsgCmd.pushKV(i.first, i.second); + sendPerMsgType.pushKV(i.first, i.second); } - obj.pushKV("bytessent_per_msg", sendPerMsgCmd); + obj.pushKV("bytessent_per_msg", sendPerMsgType); - UniValue recvPerMsgCmd(UniValue::VOBJ); - for (const auto& i : stats.mapRecvBytesPerMsgCmd) { + UniValue recvPerMsgType(UniValue::VOBJ); + for (const auto& i : stats.mapRecvBytesPerMsgType) { if (i.second > 0) - recvPerMsgCmd.pushKV(i.first, i.second); + recvPerMsgType.pushKV(i.first, i.second); } - obj.pushKV("bytesrecv_per_msg", recvPerMsgCmd); + obj.pushKV("bytesrecv_per_msg", recvPerMsgType); obj.pushKV("connection_type", ConnectionTypeAsString(stats.m_conn_type)); ret.push_back(obj); From dba4cf056b1abd2feb23f8df7a1cf0e15369cc97 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 11 Jul 2022 18:14:36 +0200 Subject: [PATCH 11/14] merge bitcoin#25591: Version handshake to libtest_util --- src/test/fuzz/process_message.cpp | 3 +- src/test/fuzz/process_messages.cpp | 3 +- src/test/fuzz/util.cpp | 57 ++++------------------------- src/test/fuzz/util.h | 2 +- src/test/util/net.cpp | 58 ++++++++++++++++++++++++++++++ src/test/util/net.h | 7 ++++ 6 files changed, 75 insertions(+), 55 deletions(-) diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index de44e65fa1..5b921696f9 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -87,8 +87,7 @@ void fuzz_target(FuzzBufferType buffer, const std::string& LIMIT_TO_MESSAGE_TYPE CNode& p2p_node = *ConsumeNodeAsUniquePtr(fuzzed_data_provider).release(); connman.AddTestNode(p2p_node); - g_setup->m_node.peerman->InitializeNode(&p2p_node); - FillNode(fuzzed_data_provider, connman, *g_setup->m_node.peerman, p2p_node); + FillNode(fuzzed_data_provider, connman, p2p_node); const auto mock_time = ConsumeTime(fuzzed_data_provider); SetMockTime(mock_time); diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 19a7371416..744503bf55 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -45,8 +45,7 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages) peers.push_back(ConsumeNodeAsUniquePtr(fuzzed_data_provider, i).release()); CNode& p2p_node = *peers.back(); - g_setup->m_node.peerman->InitializeNode(&p2p_node); - FillNode(fuzzed_data_provider, connman, *g_setup->m_node.peerman, p2p_node); + FillNode(fuzzed_data_provider, connman, p2p_node); connman.AddTestNode(p2p_node); } diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index cb88167ee1..d3674790eb 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -282,57 +282,14 @@ bool FuzzedSock::IsConnected(std::string& errmsg) const return false; } -void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, PeerManager& peerman, CNode& node) noexcept +void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, CNode& node) noexcept { - const bool successfully_connected{fuzzed_data_provider.ConsumeBool()}; - const ServiceFlags remote_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS); - const NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS); - const int32_t version = fuzzed_data_provider.ConsumeIntegralInRange(MIN_PEER_PROTO_VERSION, std::numeric_limits::max()); - const bool relay_txs{fuzzed_data_provider.ConsumeBool()}; - - const CNetMsgMaker mm{0}; - - CSerializedNetMsg msg_version{ - mm.Make(NetMsgType::VERSION, - version, // - Using>(remote_services), // - int64_t{}, // dummy time - int64_t{}, // ignored service bits - CService{}, // dummy - int64_t{}, // ignored service bits - CService{}, // ignored - uint64_t{1}, // dummy nonce - std::string{}, // dummy subver - int32_t{}, // dummy starting_height - relay_txs), - }; - - (void)connman.ReceiveMsgFrom(node, msg_version); - node.fPauseSend = false; - connman.ProcessMessagesOnce(node); - { - LOCK(node.cs_sendProcessing); - peerman.SendMessages(&node); - } - if (node.fDisconnect) return; - assert(node.nVersion == version); - assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION)); - assert(node.nServices == remote_services); - CNodeStateStats statestats; - assert(peerman.GetNodeStateStats(node.GetId(), statestats)); - assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn())); - node.m_permissionFlags = permission_flags; - if (successfully_connected) { - CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)}; - (void)connman.ReceiveMsgFrom(node, msg_verack); - node.fPauseSend = false; - connman.ProcessMessagesOnce(node); - { - LOCK(node.cs_sendProcessing); - peerman.SendMessages(&node); - } - assert(node.fSuccessfullyConnected == true); - } + connman.Handshake(node, + /*successfully_connected=*/fuzzed_data_provider.ConsumeBool(), + /*remote_services=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS), + /*permission_flags=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS), + /*version=*/fuzzed_data_provider.ConsumeIntegralInRange(MIN_PEER_PROTO_VERSION, std::numeric_limits::max()), + /*relay_txs=*/fuzzed_data_provider.ConsumeBool()); } int64_t ConsumeTime(FuzzedDataProvider& fuzzed_data_provider, const std::optional& min, const std::optional& max) noexcept diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index f3a18a1efb..f29a260cbb 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -396,7 +396,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional ConsumeNodeAsUniquePtr(FuzzedDataProvider& fdp, const std::optional& node_id_in = std::nullopt) { return ConsumeNode(fdp, node_id_in); } -void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, PeerManager& peerman, CNode& node) noexcept; +void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, CNode& node) noexcept; class FuzzedFileProvider { diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index c71377f821..380a9350b6 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -6,10 +6,68 @@ #include #include +#include +#include #include #include +void ConnmanTestMsg::Handshake(CNode& node, + bool successfully_connected, + ServiceFlags remote_services, + NetPermissionFlags permission_flags, + int32_t version, + bool relay_txs) +{ + auto& peerman{static_cast(*m_msgproc)}; + auto& connman{*this}; + const CNetMsgMaker mm{0}; + + peerman.InitializeNode(&node); + + CSerializedNetMsg msg_version{ + mm.Make(NetMsgType::VERSION, + version, // + Using>(remote_services), // + int64_t{}, // dummy time + int64_t{}, // ignored service bits + CService{}, // dummy + int64_t{}, // ignored service bits + CService{}, // ignored + uint64_t{1}, // dummy nonce + std::string{}, // dummy subver + int32_t{}, // dummy starting_height + relay_txs), + }; + + (void)connman.ReceiveMsgFrom(node, msg_version); + node.fPauseSend = false; + connman.ProcessMessagesOnce(node); + { + LOCK(node.cs_sendProcessing); + peerman.SendMessages(&node); + } + if (node.fDisconnect) return; + assert(node.nVersion == version); + assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION)); + assert(node.nServices == remote_services); + CNodeStateStats statestats; + assert(peerman.GetNodeStateStats(node.GetId(), statestats)); + assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn())); + node.m_permissionFlags = permission_flags; + if (successfully_connected) { + CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)}; + (void)connman.ReceiveMsgFrom(node, msg_verack); + node.fPauseSend = false; + connman.ProcessMessagesOnce(node); + { + LOCK(node.cs_sendProcessing); + peerman.SendMessages(&node); + } + assert(node.fSuccessfullyConnected == true); + } +} + void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const { assert(node.ReceiveMsgBytes(msg_bytes, complete)); diff --git a/src/test/util/net.h b/src/test/util/net.h index 6516577f44..8e7992c2c2 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -38,6 +38,13 @@ struct ConnmanTestMsg : public CConnman { m_nodes.clear(); } + void Handshake(CNode& node, + bool successfully_connected, + ServiceFlags remote_services, + NetPermissionFlags permission_flags, + int32_t version, + bool relay_txs); + void ProcessMessagesOnce(CNode& node) { m_msgproc->ProcessMessages(&node, flagInterruptMsgProc); } void NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const; From 4847f6e96fc52ca636a6484edcc31457d22efe38 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 11 Jun 2024 20:46:07 +0000 Subject: [PATCH 12/14] refactor: move `m_initial_sync_finished` out of header The PeerManager implementation was moved into the source file in bitcoin#20811 (dash#5352) but the PR that introduced `m_initial_sync_finished` bitcoin#19858 (dash#5869) was merged later and didn't account for the out-of-order backport. We need to correct for that manually. --- src/net_processing.cpp | 4 ++++ src/net_processing.h | 5 ----- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a8c4f52de1..c7cb7cb82a 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -524,6 +524,10 @@ private: /** Whether this node is running in blocks only mode */ const bool m_ignore_incoming_txs; + /** Whether we've completed initial sync yet, for determining when to turn + * on extra block-relay-only peers. */ + bool m_initial_sync_finished{false}; + /** Protects m_peer_map. This mutex must not be locked while holding a lock * on any of the mutexes inside a Peer object. */ mutable Mutex m_peer_mutex; diff --git a/src/net_processing.h b/src/net_processing.h index cc7c846b68..8c575a3e64 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -116,11 +116,6 @@ public: const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) = 0; virtual bool IsBanned(NodeId pnode) = 0; - - /** Whether we've completed initial sync yet, for determining when to turn - * on extra block-relay-only peers. */ - bool m_initial_sync_finished{false}; - }; #endif // BITCOIN_NET_PROCESSING_H From f8d1e5b3ec5be7f9860c954c5df487dae1c718e1 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 11 Jun 2024 20:40:23 +0000 Subject: [PATCH 13/14] merge bitcoin#25174: Add thread safety related annotations for CNode and Peer --- src/net.cpp | 10 +++++----- src/net.h | 16 ++++++++-------- src/net_processing.cpp | 4 ++-- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index a31b2a5efc..8e596cb6b3 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -932,7 +932,8 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds return msg; } -void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector& header) { +void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector& header) const +{ // create dbl-sha256 checksum uint256 hash = Hash(msg.data); @@ -4126,7 +4127,9 @@ ServiceFlags CConnman::GetLocalServices() const unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, std::unique_ptr&& i2p_sam_session) - : m_sock{sock}, + : m_deserializer{std::make_unique(V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION))}, + m_serializer{std::make_unique(V1TransportSerializer())}, + m_sock{sock}, m_connected{GetTime()}, addr{addrIn}, addrBind{addrBindIn}, @@ -4150,9 +4153,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr s } else { LogPrint(BCLog::NET, "Added connection peer=%d\n", id); } - - m_deserializer = std::make_unique(V1TransportDeserializer(Params(), id, SER_NETWORK, INIT_PROTO_VERSION)); - m_serializer = std::make_unique(V1TransportSerializer()); } bool CConnman::NodeFullyConnected(const CNode* pnode) diff --git a/src/net.h b/src/net.h index e2d258e703..def5098b38 100644 --- a/src/net.h +++ b/src/net.h @@ -427,13 +427,13 @@ public: class TransportSerializer { public: // prepare message for transport (header construction, error-correction computation, payload encryption, etc.) - virtual void prepareForTransport(CSerializedNetMsg& msg, std::vector& header) = 0; + virtual void prepareForTransport(CSerializedNetMsg& msg, std::vector& header) const = 0; virtual ~TransportSerializer() {} }; -class V1TransportSerializer : public TransportSerializer { +class V1TransportSerializer : public TransportSerializer { public: - void prepareForTransport(CSerializedNetMsg& msg, std::vector& header) override; + void prepareForTransport(CSerializedNetMsg& msg, std::vector& header) const override; }; /** Information about a peer */ @@ -443,10 +443,10 @@ class CNode friend struct ConnmanTestMsg; public: - std::unique_ptr m_deserializer; - std::unique_ptr m_serializer; + const std::unique_ptr m_deserializer; // Used only by SocketHandler thread + const std::unique_ptr m_serializer; - NetPermissionFlags m_permissionFlags{ NetPermissionFlags::None }; + NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester std::atomic nServices{NODE_NONE}; /** @@ -472,7 +472,7 @@ public: RecursiveMutex cs_vProcessMsg; std::list vProcessMsg GUARDED_BY(cs_vProcessMsg); - size_t nProcessQueueSize{0}; + size_t nProcessQueueSize GUARDED_BY(cs_vProcessMsg){0}; RecursiveMutex cs_sendProcessing; @@ -501,7 +501,7 @@ public: * from the wire. This cleaned string can safely be logged or displayed. */ std::string cleanSubVer GUARDED_BY(m_subver_mutex){}; - bool m_prefer_evict{false}; // This peer is preferred for eviction. + bool m_prefer_evict{false}; // This peer is preferred for eviction. (treated as const) bool HasPermission(NetPermissionFlags permission) const { return NetPermissions::HasFlag(m_permissionFlags, permission); } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c7cb7cb82a..77ed5bb9b2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -519,14 +519,14 @@ private: std::atomic m_best_height{-1}; /** Next time to check for stale tip */ - std::chrono::seconds m_stale_tip_check_time{0s}; + std::chrono::seconds m_stale_tip_check_time GUARDED_BY(cs_main){0s}; /** Whether this node is running in blocks only mode */ const bool m_ignore_incoming_txs; /** Whether we've completed initial sync yet, for determining when to turn * on extra block-relay-only peers. */ - bool m_initial_sync_finished{false}; + bool m_initial_sync_finished GUARDED_BY(cs_main){false}; /** Protects m_peer_map. This mutex must not be locked while holding a lock * on any of the mutexes inside a Peer object. */ From 71d14528caf7e6a155e7b102da81532ba5d606d0 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 12 Jun 2024 16:20:39 +0000 Subject: [PATCH 14/14] refactor: enumerate each CNode argument on a separate line This change in styling was introduced in a1580a04 (bitcoin#25355) but not backported in 9bf38295 (dash#6035). This remedies that. --- src/net.cpp | 12 +++++++++++- src/net.h | 12 +++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 8e596cb6b3..8895f28bf8 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -4126,7 +4126,17 @@ ServiceFlags CConnman::GetLocalServices() const unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } -CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, std::unique_ptr&& i2p_sam_session) +CNode::CNode(NodeId idIn, + ServiceFlags nLocalServicesIn, + std::shared_ptr sock, + const CAddress& addrIn, + uint64_t nKeyedNetGroupIn, + uint64_t nLocalHostNonceIn, + const CAddress& addrBindIn, + const std::string& addrNameIn, + ConnectionType conn_type_in, + bool inbound_onion, + std::unique_ptr&& i2p_sam_session) : m_deserializer{std::make_unique(V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION))}, m_serializer{std::make_unique(V1TransportSerializer())}, m_sock{sock}, diff --git a/src/net.h b/src/net.h index def5098b38..a0bb7bbba8 100644 --- a/src/net.h +++ b/src/net.h @@ -648,7 +648,17 @@ public: bool IsBlockRelayOnly() const; - CNode(NodeId id, ServiceFlags nLocalServicesIn, std::shared_ptr sock, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn, ConnectionType conn_type_in, bool inbound_onion, std::unique_ptr&& i2p_sam_session = nullptr); + CNode(NodeId id, + ServiceFlags nLocalServicesIn, + std::shared_ptr sock, + const CAddress &addrIn, + uint64_t nKeyedNetGroupIn, + uint64_t nLocalHostNonceIn, + const CAddress &addrBindIn, + const std::string &addrNameIn, + ConnectionType conn_type_in, + bool inbound_onion, + std::unique_ptr&& i2p_sam_session = nullptr); CNode(const CNode&) = delete; CNode& operator=(const CNode&) = delete;