diff --git a/nano/core_test/optimistic_scheduler.cpp b/nano/core_test/optimistic_scheduler.cpp index 7466584b6..542c758d0 100644 --- a/nano/core_test/optimistic_scheduler.cpp +++ b/nano/core_test/optimistic_scheduler.cpp @@ -17,8 +17,11 @@ using namespace std::chrono_literals; */ TEST (optimistic_scheduler, activate_one) { - nano::test::system system{}; - auto & node = *system.add_node (); + nano::test::system system; + + nano::node_config config; + config.priority_scheduler.enable = false; // Disable priority scheduler to avoid interference + auto & node = *system.add_node (config); // Needs to be greater than optimistic scheduler `gap_threshold` const int howmany_blocks = 64; @@ -41,8 +44,11 @@ TEST (optimistic_scheduler, activate_one) */ TEST (optimistic_scheduler, activate_one_zero_conf) { - nano::test::system system{}; - auto & node = *system.add_node (); + nano::test::system system; + + nano::node_config config; + config.priority_scheduler.enable = false; // Disable priority scheduler to avoid interference + auto & node = *system.add_node (config); // Can be smaller than optimistic scheduler `gap_threshold` // This is meant to activate short account chains (eg. binary tree spam leaf accounts) @@ -63,8 +69,11 @@ TEST (optimistic_scheduler, activate_one_zero_conf) */ TEST (optimistic_scheduler, activate_many) { - nano::test::system system{}; - auto & node = *system.add_node (); + nano::test::system system; + + nano::node_config config; + config.priority_scheduler.enable = false; // Disable priority scheduler to avoid interference + auto & node = *system.add_node (config); // Needs to be greater than optimistic scheduler `gap_threshold` const int howmany_blocks = 64; @@ -72,8 +81,8 @@ TEST (optimistic_scheduler, activate_many) auto chains = nano::test::setup_chains (system, node, howmany_chains, howmany_blocks, nano::dev::genesis_key, /* do not confirm */ false); - // Ensure all unconfirmed accounts head block gets activated - ASSERT_TIMELY (5s, std::all_of (chains.begin (), chains.end (), [&] (auto const & entry) { + // Ensure all unconfirmed account head blocks get activated + ASSERT_TIMELY (15s, std::all_of (chains.begin (), chains.end (), [&] (auto const & entry) { auto const & [account, blocks] = entry; auto const & block = blocks.back (); auto election = node.active.election (block->qualified_root ()); @@ -86,7 +95,8 @@ TEST (optimistic_scheduler, activate_many) */ TEST (optimistic_scheduler, under_gap_threshold) { - nano::test::system system{}; + nano::test::system system; + nano::node_config config = system.default_config (); config.backlog_scan.enable = false; auto & node = *system.add_node (config); diff --git a/nano/lib/constants.hpp b/nano/lib/constants.hpp index aeef2474e..4eaffde86 100644 --- a/nano/lib/constants.hpp +++ b/nano/lib/constants.hpp @@ -132,7 +132,6 @@ public: telemetry_cache_cutoff = 2000ms; telemetry_request_interval = 500ms; telemetry_broadcast_interval = 500ms; - optimistic_activation_delay = 2s; rep_crawler_normal_interval = 500ms; rep_crawler_warmup_interval = 500ms; } @@ -183,9 +182,6 @@ public: /** Telemetry data older than this value is considered stale */ std::chrono::milliseconds telemetry_cache_cutoff{ 1000 * 130 }; // 2 * `telemetry_broadcast_interval` + some margin - /** How much to delay activation of optimistic elections to avoid interfering with election scheduler */ - std::chrono::seconds optimistic_activation_delay{ 30 }; - std::chrono::milliseconds rep_crawler_normal_interval{ 1000 * 7 }; std::chrono::milliseconds rep_crawler_warmup_interval{ 1000 * 3 }; diff --git a/nano/node/scheduler/optimistic.cpp b/nano/node/scheduler/optimistic.cpp index 818421ada..a34869e80 100644 --- a/nano/node/scheduler/optimistic.cpp +++ b/nano/node/scheduler/optimistic.cpp @@ -52,7 +52,11 @@ void nano::scheduler::optimistic::stop () void nano::scheduler::optimistic::notify () { - condition.notify_all (); + // Only wake up the thread if there is space inside AEC for optimistic elections + if (active.vacancy (nano::election_behavior::optimistic) > 0) + { + condition.notify_all (); + } } bool nano::scheduler::optimistic::activate_predicate (const nano::account_info & account_info, const nano::confirmation_height_info & conf_info) const @@ -72,33 +76,36 @@ bool nano::scheduler::optimistic::activate_predicate (const nano::account_info & bool nano::scheduler::optimistic::activate (const nano::account & account, const nano::account_info & account_info, const nano::confirmation_height_info & conf_info) { + debug_assert (account_info.block_count >= conf_info.height); + if (!config.enable) { return false; } - debug_assert (account_info.block_count >= conf_info.height); if (activate_predicate (account_info, conf_info)) { + nano::lock_guard lock{ mutex }; + + // Assume gap_threshold for accounts with nothing confirmed + auto const unconfirmed_height = std::max (account_info.block_count - conf_info.height, config.gap_threshold); + + auto [it, inserted] = candidates.push_back ({ account, nano::clock::now (), unconfirmed_height }); + + stats.inc (nano::stat::type::optimistic_scheduler, inserted ? nano::stat::detail::activated : nano::stat::detail::duplicate); + + // Limit candidates container size + if (candidates.size () > config.max_size) { - nano::lock_guard lock{ mutex }; - - // Prevent duplicate candidate accounts - if (candidates.get ().contains (account)) - { - return false; // Not activated - } - // Limit candidates container size - if (candidates.size () >= config.max_size) - { - return false; // Not activated - } - - stats.inc (nano::stat::type::optimistic_scheduler, nano::stat::detail::activated); - candidates.push_back ({ account, nano::clock::now () }); + // Remove oldest candidate + candidates.pop_front (); } - return true; // Activated + + // Not notifying the thread immediately here, since we need to wait for activation_delay to elapse + + return inserted; } + return false; // Not activated } @@ -115,9 +122,10 @@ bool nano::scheduler::optimistic::predicate () const return false; } - auto candidate = candidates.front (); - bool result = nano::elapsed (candidate.timestamp, network_constants.optimistic_activation_delay); - return result; + auto candidate = candidates.get ().begin (); + debug_assert (candidate != candidates.get ().end ()); + + return elapsed (candidate->timestamp, config.activation_delay); } void nano::scheduler::optimistic::run () @@ -125,29 +133,41 @@ void nano::scheduler::optimistic::run () nano::unique_lock lock{ mutex }; while (!stopped) { - stats.inc (nano::stat::type::optimistic_scheduler, nano::stat::detail::loop); + condition.wait_for (lock, config.activation_delay / 2, [this] () { + return stopped || predicate (); + }); + + if (stopped) + { + return; + } if (predicate ()) { + stats.inc (nano::stat::type::optimistic_scheduler, nano::stat::detail::loop); + + lock.unlock (); + + // Acquire transaction outside of the lock auto transaction = ledger.tx_begin_read (); - while (predicate ()) + lock.lock (); + + while (predicate () && !stopped) { debug_assert (!candidates.empty ()); - auto candidate = candidates.front (); - candidates.pop_front (); + auto & height_index = candidates.get (); + auto candidate = *height_index.begin (); + height_index.erase (height_index.begin ()); lock.unlock (); + transaction.refresh_if_needed (); run_one (transaction, candidate); lock.lock (); } } - - condition.wait_for (lock, network_constants.optimistic_activation_delay / 2, [this] () { - return stopped || predicate (); - }); } } @@ -160,7 +180,7 @@ void nano::scheduler::optimistic::run_one (secure::transaction const & transacti if (!node.block_confirmed_or_being_confirmed (transaction, block->hash ())) { // Try to insert it into AEC - // We check for AEC vacancy inside our predicate + // We check for AEC vacancy inside the predicate auto result = node.active.insert (block, nano::election_behavior::optimistic); stats.inc (nano::stat::type::optimistic_scheduler, result.inserted ? nano::stat::detail::insert : nano::stat::detail::insert_failed); @@ -186,6 +206,7 @@ nano::error nano::scheduler::optimistic_config::deserialize (nano::tomlconfig & toml.get ("enable", enable); toml.get ("gap_threshold", gap_threshold); toml.get ("max_size", max_size); + toml.get_duration ("activation_delay", activation_delay); return toml.get_error (); } @@ -195,6 +216,7 @@ nano::error nano::scheduler::optimistic_config::serialize (nano::tomlconfig & to toml.put ("enable", enable, "Enable or disable optimistic elections\ntype:bool"); toml.put ("gap_threshold", gap_threshold, "Minimum difference between confirmation frontier and account frontier to become a candidate for optimistic confirmation\ntype:uint64"); toml.put ("max_size", max_size, "Maximum number of candidates stored in memory\ntype:uint64"); + toml.put ("activation_delay", activation_delay.count (), "How much to delay activation of optimistic elections to avoid interfering with election scheduler\ntype:milliseconds"); return toml.get_error (); } diff --git a/nano/node/scheduler/optimistic.hpp b/nano/node/scheduler/optimistic.hpp index 24edd1e73..6977c0ada 100644 --- a/nano/node/scheduler/optimistic.hpp +++ b/nano/node/scheduler/optimistic.hpp @@ -33,10 +33,13 @@ public: bool enable{ true }; /** Minimum difference between confirmation frontier and account frontier to become a candidate for optimistic confirmation */ - std::size_t gap_threshold{ 32 }; + uint64_t gap_threshold{ 16 }; /** Maximum number of candidates stored in memory */ - std::size_t max_size{ 1024 * 64 }; + std::size_t max_size{ 1024 * 4 }; + + /** How much to delay activation of optimistic elections to avoid interfering with election scheduler */ + std::chrono::milliseconds activation_delay{ 1s }; }; class optimistic final @@ -82,17 +85,21 @@ private: { nano::account account; nano::clock::time_point timestamp; + uint64_t unconfirmed_height; }; // clang-format off class tag_sequenced {}; class tag_account {}; + class tag_unconfirmed_height {}; using ordered_candidates = boost::multi_index_container>, mi::hashed_unique, - mi::member> + mi::member>, + mi::ordered_non_unique, + mi::member, std::greater> // Descending >>; // clang-format on