diff --git a/nano/core_test/bootstrap.cpp b/nano/core_test/bootstrap.cpp index ec30708ebf..ccebd97cd4 100644 --- a/nano/core_test/bootstrap.cpp +++ b/nano/core_test/bootstrap.cpp @@ -122,7 +122,7 @@ TEST (account_sets, priority_up_down) sets.priority_up (account); ASSERT_EQ (sets.priority (account), nano::bootstrap::account_sets::priority_initial); sets.priority_down (account); - ASSERT_EQ (sets.priority (account), nano::bootstrap::account_sets::priority_initial); + ASSERT_EQ (sets.priority (account), nano::bootstrap::account_sets::priority_initial / nano::bootstrap::account_sets::priority_divide); } TEST (account_sets, priority_down_empty) diff --git a/nano/lib/stats_enums.hpp b/nano/lib/stats_enums.hpp index c2fec6a159..23c7e819d2 100644 --- a/nano/lib/stats_enums.hpp +++ b/nano/lib/stats_enums.hpp @@ -127,6 +127,7 @@ enum class detail erased, request, request_failed, + request_success, broadcast, cleanup, top, diff --git a/nano/node/bootstrap/account_sets.cpp b/nano/node/bootstrap/account_sets.cpp index 98d705dbdd..421ffddb46 100644 --- a/nano/node/bootstrap/account_sets.cpp +++ b/nano/node/bootstrap/account_sets.cpp @@ -61,15 +61,18 @@ void nano::bootstrap::account_sets::priority_down (nano::account const & account { stats.inc (nano::stat::type::bootstrap_account_sets, nano::stat::detail::deprioritize); - if (it->fails >= account_sets::max_fails || it->fails >= it->priority) + auto priority = it->priority / account_sets::priority_divide; + + if (it->fails >= account_sets::max_fails || it->fails >= it->priority || priority <= account_sets::priority_cutoff) { stats.inc (nano::stat::type::bootstrap_account_sets, nano::stat::detail::erase_by_threshold); priorities.get ().erase (it); } else { - priorities.get ().modify (it, [] (auto & val) { + priorities.get ().modify (it, [priority] (auto & val) { val.fails += 1; + val.priority = priority; }); } } @@ -217,7 +220,7 @@ void nano::bootstrap::account_sets::trim_overflow () } } -nano::account nano::bootstrap::account_sets::next_priority (std::function const & filter) +auto nano::bootstrap::account_sets::next_priority (std::function const & filter) -> priority_result { if (priorities.empty ()) { @@ -236,10 +239,14 @@ nano::account nano::bootstrap::account_sets::next_priority (std::function const & filter) diff --git a/nano/node/bootstrap/account_sets.hpp b/nano/node/bootstrap/account_sets.hpp index 8456efa3d8..59bdbf95af 100644 --- a/nano/node/bootstrap/account_sets.hpp +++ b/nano/node/bootstrap/account_sets.hpp @@ -63,10 +63,17 @@ namespace bootstrap */ void sync_dependencies (); + struct priority_result + { + nano::account account; + double priority; + unsigned fails; + }; + /** * Sampling */ - nano::account next_priority (std::function const & filter); + priority_result next_priority (std::function const & filter); nano::block_hash next_blocking (std::function const & filter); bool blocked (nano::account const & account) const; diff --git a/nano/node/bootstrap/bootstrap_service.cpp b/nano/node/bootstrap/bootstrap_service.cpp index e6a2295cb8..c2d8188249 100644 --- a/nano/node/bootstrap/bootstrap_service.cpp +++ b/nano/node/bootstrap/bootstrap_service.cpp @@ -153,6 +153,8 @@ bool nano::bootstrap_service::send (std::shared_ptr co { nano::lock_guard lock{ mutex }; debug_assert (tags.get ().count (tag.id) == 0); + // Give extra time for the request to be processed by the channel + tag.cutoff = std::chrono::steady_clock::now () + config.request_timeout * 4; tags.get ().insert (tag); } @@ -202,10 +204,25 @@ bool nano::bootstrap_service::send (std::shared_ptr co stats.inc (nano::stat::type::bootstrap, nano::stat::detail::request, nano::stat::dir::out); stats.inc (nano::stat::type::bootstrap_request, to_stat_detail (tag.type)); - // TODO: There is no feedback mechanism if bandwidth limiter starts dropping our requests channel->send ( - request, nullptr, - nano::transport::buffer_drop_policy::limiter, nano::transport::traffic_type::bootstrap); + request, [this, id = tag.id] (auto const & ec, auto size) { + nano::lock_guard lock{ mutex }; + if (auto it = tags.get ().find (id); it != tags.get ().end ()) + { + if (!ec) + { + stats.inc (nano::stat::type::bootstrap, nano::stat::detail::request_success, nano::stat::dir::out); + tags.get ().modify (it, [&] (auto & tag) { + // After the request has been sent, the peer has a limited time to respond + tag.cutoff = std::chrono::steady_clock::now () + config.request_timeout; + }); + } + else + { + stats.inc (nano::stat::type::bootstrap, nano::stat::detail::request_failed, nano::stat::dir::out); + tags.get ().erase (it); + } + } }, nano::transport::buffer_drop_policy::limiter, nano::transport::traffic_type::bootstrap); return true; // TODO: Return channel send result } @@ -355,30 +372,28 @@ size_t nano::bootstrap_service::count_tags (nano::block_hash const & hash, query return std::count_if (begin, end, [source] (auto const & tag) { return tag.source == source; }); } -std::pair nano::bootstrap_service::next_priority () +nano::bootstrap::account_sets::priority_result nano::bootstrap_service::next_priority () { debug_assert (!mutex.try_lock ()); - auto account = accounts.next_priority ([this] (nano::account const & account) { + auto next = accounts.next_priority ([this] (nano::account const & account) { return count_tags (account, query_source::priority) < 4; }); - if (account.is_zero ()) + if (next.account.is_zero ()) { return {}; } stats.inc (nano::stat::type::bootstrap_next, nano::stat::detail::next_priority); - accounts.timestamp_set (account); - // TODO: Priority could be returned by the accounts.next_priority() call - return { account, accounts.priority (account) }; + return next; } -std::pair nano::bootstrap_service::wait_priority () +nano::bootstrap::account_sets::priority_result nano::bootstrap_service::wait_priority () { - std::pair result{ 0, 0 }; + nano::bootstrap::account_sets::priority_result result{}; wait ([this, &result] () { debug_assert (!mutex.try_lock ()); result = next_priority (); - if (!result.first.is_zero ()) + if (!result.account.is_zero ()) { return true; } @@ -548,14 +563,26 @@ void nano::bootstrap_service::run_one_priority () { return; } - auto [account, priority] = wait_priority (); + auto [account, priority, fails] = wait_priority (); if (account.is_zero ()) { return; } + + // Decide how many blocks to request size_t const min_pull_count = 2; - auto count = std::clamp (static_cast (priority), min_pull_count, nano::bootstrap_server::max_blocks); - request (account, count, channel, query_source::priority); + auto pull_count = std::clamp (static_cast (priority), min_pull_count, nano::bootstrap_server::max_blocks); + + bool sent = request (account, pull_count, channel, query_source::priority); + + // Only cooldown accounts that are likely to have more blocks + // This is to avoid requesting blocks from the same frontier multiple times, before the block processor had a chance to process them + // Not throttling accounts that are probably up-to-date allows us to evict them from the priority set faster + if (sent && fails == 0) + { + nano::lock_guard lock{ mutex }; + accounts.timestamp_set (account); + } } void nano::bootstrap_service::run_priorities () @@ -674,9 +701,9 @@ void nano::bootstrap_service::cleanup_and_sync () throttle.resize (compute_throttle_size ()); - auto const cutoff = std::chrono::steady_clock::now () - config.request_timeout; - auto should_timeout = [cutoff] (async_tag const & tag) { - return tag.timestamp < cutoff; + auto const now = std::chrono::steady_clock::now (); + auto should_timeout = [&] (async_tag const & tag) { + return tag.cutoff < now; }; auto & tags_by_order = tags.get (); @@ -828,13 +855,18 @@ bool nano::bootstrap_service::process (const nano::asc_pull_ack::blocks_payload case verify_result::nothing_new: { stats.inc (nano::stat::type::bootstrap_verify_blocks, nano::stat::detail::nothing_new); - - nano::lock_guard lock{ mutex }; - accounts.priority_down (tag.account); - if (tag.source == query_source::database) { - throttle.add (false); + nano::lock_guard lock{ mutex }; + + accounts.priority_down (tag.account); + accounts.timestamp_reset (tag.account); + + if (tag.source == query_source::database) + { + throttle.add (false); + } } + condition.notify_all (); } break; case verify_result::invalid: diff --git a/nano/node/bootstrap/bootstrap_service.hpp b/nano/node/bootstrap/bootstrap_service.hpp index 1959df41e1..7e881e5c9d 100644 --- a/nano/node/bootstrap/bootstrap_service.hpp +++ b/nano/node/bootstrap/bootstrap_service.hpp @@ -90,9 +90,9 @@ class bootstrap_service nano::account account{ 0 }; nano::block_hash hash{ 0 }; size_t count{ 0 }; - - id_t id{ nano::bootstrap::generate_id () }; + std::chrono::steady_clock::time_point cutoff{}; std::chrono::steady_clock::time_point timestamp{ std::chrono::steady_clock::now () }; + id_t id{ nano::bootstrap::generate_id () }; }; private: @@ -117,9 +117,10 @@ class bootstrap_service void wait_blockprocessor () const; /* Waits for a channel that is not full */ std::shared_ptr wait_channel (); - /* Waits until a suitable account outside of cool down period is available */ - std::pair next_priority (); - std::pair wait_priority (); + /* Waits until a suitable account outside of cooldown period is available */ + using priority_result = nano::bootstrap::account_sets::priority_result; + priority_result next_priority (); + priority_result wait_priority (); /* Gets the next account from the database */ nano::account next_database (bool should_throttle); nano::account wait_database (bool should_throttle);