From 2dab31f62f3e7aff14afd7897848143456b0c47d Mon Sep 17 00:00:00 2001 From: j-berman Date: Sat, 14 May 2022 20:11:53 -0700 Subject: [PATCH] Don't exclusively drop tor/i2p outgoing cxns in idle loop --- .../cryptonote_protocol_handler.h | 18 +++++- .../cryptonote_protocol_handler.inl | 62 ++++++++++++------- src/p2p/net_node.inl | 10 ++- tests/unit_tests/node_server.cpp | 4 +- 4 files changed, 65 insertions(+), 29 deletions(-) diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.h b/src/cryptonote_protocol/cryptonote_protocol_handler.h index a1e4df563..515b78c94 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.h +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.h @@ -113,12 +113,23 @@ namespace cryptonote const block_queue &get_block_queue() const { return m_block_queue; } void stop(); void on_connection_close(cryptonote_connection_context &context); - void set_max_out_peers(unsigned int max) { m_max_out_peers = max; } + void set_max_out_peers(epee::net_utils::zone zone, unsigned int max) { CRITICAL_REGION_LOCAL(m_max_out_peers_lock); m_max_out_peers[zone] = max; } + unsigned int get_max_out_peers(epee::net_utils::zone zone) const + { + CRITICAL_REGION_LOCAL(m_max_out_peers_lock); + const auto it = m_max_out_peers.find(zone); + if (it == m_max_out_peers.end()) + { + MWARNING(epee::net_utils::zone_to_string(zone) << " max out peers not set, using default"); + return P2P_DEFAULT_CONNECTIONS_COUNT; + } + return it->second; + } bool no_sync() const { return m_no_sync; } void set_no_sync(bool value) { m_no_sync = value; } std::string get_peers_overview() const; std::pair get_next_needed_pruning_stripe() const; - bool needs_new_sync_connections() const; + bool needs_new_sync_connections(epee::net_utils::zone zone) const; bool is_busy_syncing(); private: @@ -171,7 +182,8 @@ namespace cryptonote epee::math_helper::once_a_time_milliseconds<100> m_standby_checker; epee::math_helper::once_a_time_seconds<101> m_sync_search_checker; epee::math_helper::once_a_time_seconds<43> m_bad_peer_checker; - std::atomic m_max_out_peers; + std::unordered_map m_max_out_peers; + mutable epee::critical_section m_max_out_peers_lock; tools::PerformanceTimer m_sync_timer, m_add_timer; uint64_t m_last_add_end_time; uint64_t m_sync_spans_downloaded, m_sync_old_spans_downloaded, m_sync_bad_spans_downloaded; diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl index 891ee109d..af3031263 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl @@ -1776,33 +1776,49 @@ skip: return true; MTRACE("Checking for outgoing syncing peers..."); - unsigned n_syncing = 0, n_synced = 0; - boost::uuids::uuid last_synced_peer_id(boost::uuids::nil_uuid()); + std::unordered_map n_syncing, n_synced; + std::unordered_map last_synced_peer_id; + std::vector zones; m_p2p->for_each_connection([&](cryptonote_connection_context& context, nodetool::peerid_type peer_id, uint32_t support_flags)->bool { if (!peer_id || context.m_is_income) // only consider connected outgoing peers return true; + + const epee::net_utils::zone zone = context.m_remote_address.get_zone(); + if (n_syncing.find(zone) == n_syncing.end()) + { + n_syncing[zone] = 0; + n_synced[zone] = 0; + last_synced_peer_id[zone] = boost::uuids::nil_uuid(); + zones.push_back(zone); + } + if (context.m_state == cryptonote_connection_context::state_synchronizing) - ++n_syncing; + ++n_syncing[zone]; if (context.m_state == cryptonote_connection_context::state_normal) { - ++n_synced; + ++n_synced[zone]; if (!context.m_anchor) - last_synced_peer_id = context.m_connection_id; + last_synced_peer_id[zone] = context.m_connection_id; } return true; }); - MTRACE(n_syncing << " syncing, " << n_synced << " synced"); - // if we're at max out peers, and not enough are syncing - if (n_synced + n_syncing >= m_max_out_peers && n_syncing < P2P_DEFAULT_SYNC_SEARCH_CONNECTIONS_COUNT && last_synced_peer_id != boost::uuids::nil_uuid()) + for (const auto& zone : zones) { - if (!m_p2p->for_connection(last_synced_peer_id, [&](cryptonote_connection_context& ctx, nodetool::peerid_type peer_id, uint32_t f)->bool{ - MINFO(ctx << "dropping synced peer, " << n_syncing << " syncing, " << n_synced << " synced"); - drop_connection(ctx, false, false); - return true; - })) - MDEBUG("Failed to find peer we wanted to drop"); + const unsigned int max_out_peers = get_max_out_peers(zone); + MTRACE("[" << epee::net_utils::zone_to_string(zone) << "] " << n_syncing[zone] << " syncing, " << n_synced[zone] << " synced, " << max_out_peers << " max out peers"); + + // if we're at max out peers, and not enough are syncing, drop the last sync'd non-anchor + if (n_synced[zone] + n_syncing[zone] >= max_out_peers && n_syncing[zone] < P2P_DEFAULT_SYNC_SEARCH_CONNECTIONS_COUNT && last_synced_peer_id[zone] != boost::uuids::nil_uuid()) + { + if (!m_p2p->for_connection(last_synced_peer_id[zone], [&](cryptonote_connection_context& ctx, nodetool::peerid_type peer_id, uint32_t f)->bool{ + MINFO(ctx << "dropping synced peer, " << n_syncing[zone] << " syncing, " << n_synced[zone] << " synced, " << max_out_peers << " max out peers"); + drop_connection(ctx, false, false); + return true; + })) + MDEBUG("Failed to find peer we wanted to drop"); + } } return true; @@ -1987,11 +2003,13 @@ skip: ++n_peers_on_next_stripe; return true; }); + // TODO: investigate tallying by zone and comparing to max out peers by zone + const unsigned int max_out_peers = get_max_out_peers(epee::net_utils::zone::public_); const uint32_t distance = (peer_stripe + (1<= m_max_out_peers && n_peers_on_next_stripe == 0) || (distance > 1 && n_peers_on_next_stripe <= 2) || distance > 2) + if ((n_out_peers >= max_out_peers && n_peers_on_next_stripe == 0) || (distance > 1 && n_peers_on_next_stripe <= 2) || distance > 2) { MDEBUG(context << "we want seed " << next_stripe << ", and either " << n_out_peers << " is at max out peers (" - << m_max_out_peers << ") or distance " << distance << " from " << next_stripe << " to " << peer_stripe << + << max_out_peers << ") or distance " << distance << " from " << next_stripe << " to " << peer_stripe << " is too large and we have only " << n_peers_on_next_stripe << " peers on next seed, dropping connection to make space"); return true; } @@ -2812,11 +2830,13 @@ skip: } return true; }); - const bool use_next = (n_next > m_max_out_peers / 2 && n_subsequent <= 1) || (n_next > 2 && n_subsequent == 0); + // TODO: investigate tallying by zone and comparing to max out peers by zone + const unsigned int max_out_peers = get_max_out_peers(epee::net_utils::zone::public_); + const bool use_next = (n_next > max_out_peers / 2 && n_subsequent <= 1) || (n_next > 2 && n_subsequent == 0); const uint32_t ret_stripe = use_next ? subsequent_pruning_stripe: next_pruning_stripe; MIDEBUG(const std::string po = get_peers_overview(), "get_next_needed_pruning_stripe: want height " << want_height << " (" << want_height_from_blockchain << " from blockchain, " << want_height_from_block_queue << " from block queue), stripe " << - next_pruning_stripe << " (" << n_next << "/" << m_max_out_peers << " on it and " << n_subsequent << " on " << + next_pruning_stripe << " (" << n_next << "/" << max_out_peers << " on it and " << n_subsequent << " on " << subsequent_pruning_stripe << ", " << n_others << " others) -> " << ret_stripe << " (+" << (ret_stripe - next_pruning_stripe + (1 << CRYPTONOTE_PRUNING_LOG_STRIPES)) % (1 << CRYPTONOTE_PRUNING_LOG_STRIPES) << "), current peers " << po); @@ -2824,7 +2844,7 @@ skip: } //------------------------------------------------------------------------------------------------------------------------ template - bool t_cryptonote_protocol_handler::needs_new_sync_connections() const + bool t_cryptonote_protocol_handler::needs_new_sync_connections(epee::net_utils::zone zone) const { const uint64_t target = m_core.get_target_blockchain_height(); const uint64_t height = m_core.get_current_blockchain_height(); @@ -2832,11 +2852,11 @@ skip: return false; size_t n_out_peers = 0; m_p2p->for_each_connection([&](cryptonote_connection_context& ctx, nodetool::peerid_type peer_id, uint32_t support_flags)->bool{ - if (!ctx.m_is_income) + if (!ctx.m_is_income && ctx.m_remote_address.get_zone() == zone) ++n_out_peers; return true; }); - if (n_out_peers >= m_max_out_peers) + if (n_out_peers >= get_max_out_peers(zone)) return false; return true; } diff --git a/src/p2p/net_node.inl b/src/p2p/net_node.inl index a3bc3bf24..872c1a633 100644 --- a/src/p2p/net_node.inl +++ b/src/p2p/net_node.inl @@ -538,7 +538,7 @@ namespace nodetool if ( !set_max_out_peers(public_zone, command_line::get_arg(vm, arg_out_peers) ) ) return false; else - m_payload_handler.set_max_out_peers(public_zone.m_config.m_net_config.max_out_connection_count); + m_payload_handler.set_max_out_peers(epee::net_utils::zone::public_, public_zone.m_config.m_net_config.max_out_connection_count); if ( !set_max_in_peers(public_zone, command_line::get_arg(vm, arg_in_peers) ) ) @@ -575,6 +575,8 @@ namespace nodetool if (!set_max_out_peers(zone, proxy.max_connections)) return false; + else + m_payload_handler.set_max_out_peers(proxy.zone, proxy.max_connections); epee::byte_slice this_noise = nullptr; if (proxy.noise) @@ -2758,7 +2760,7 @@ namespace nodetool public_zone->second.m_config.m_net_config.max_out_connection_count = count; if(current > count) public_zone->second.m_net_server.get_config_object().del_out_connections(current - count); - m_payload_handler.set_max_out_peers(count); + m_payload_handler.set_max_out_peers(epee::net_utils::zone::public_, count); } } @@ -2887,10 +2889,12 @@ namespace nodetool { if (m_offline) return true; if (!m_exclusive_peers.empty()) return true; - if (m_payload_handler.needs_new_sync_connections()) return true; for (auto& zone : m_network_zones) { + if (m_payload_handler.needs_new_sync_connections(zone.first)) + continue; + if (zone.second.m_net_server.is_stop_signal_sent()) return false; diff --git a/tests/unit_tests/node_server.cpp b/tests/unit_tests/node_server.cpp index 134fa6ece..6c8cd9f8d 100644 --- a/tests/unit_tests/node_server.cpp +++ b/tests/unit_tests/node_server.cpp @@ -1026,12 +1026,12 @@ TEST(node_server, race_condition) } void stop() {} void on_connection_close(context_t &context) {} - void set_max_out_peers(unsigned int max) {} + void set_max_out_peers(epee::net_utils::zone zone, unsigned int max) {} bool no_sync() const { return {}; } void set_no_sync(bool value) {} string_t get_peers_overview() const { return {}; } stripes_t get_next_needed_pruning_stripe() const { return {}; } - bool needs_new_sync_connections() const { return {}; } + bool needs_new_sync_connections(epee::net_utils::zone zone) const { return {}; } bool is_busy_syncing() { return {}; } }; using node_server_t = nodetool::node_server;