From 79a5d79e8867542a394f3c2b78bf9996509cc397 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Mon, 1 Sep 2025 23:08:02 +0200 Subject: [PATCH 1/4] Activate highest gap optimistic elections first --- nano/core_test/optimistic_scheduler.cpp | 24 ++++-- nano/node/scheduler/optimistic.cpp | 98 +++++++++++++++---------- nano/node/scheduler/optimistic.hpp | 10 ++- 3 files changed, 82 insertions(+), 50 deletions(-) diff --git a/nano/core_test/optimistic_scheduler.cpp b/nano/core_test/optimistic_scheduler.cpp index 7466584b6..a61ac5783 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; @@ -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/node/scheduler/optimistic.cpp b/nano/node/scheduler/optimistic.cpp index caab43c70..cd38ab9ae 100644 --- a/nano/node/scheduler/optimistic.cpp +++ b/nano/node/scheduler/optimistic.cpp @@ -72,34 +72,42 @@ 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); + bool activated = false; + 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 + + activated = inserted; } - return false; // Not activated + + if (activated) + { + condition.notify_all (); + } + + return activated; } bool nano::scheduler::optimistic::predicate () const @@ -115,9 +123,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, network_constants.optimistic_activation_delay); } void nano::scheduler::optimistic::run () @@ -127,27 +136,36 @@ void nano::scheduler::optimistic::run () { stats.inc (nano::stat::type::optimistic_scheduler, nano::stat::detail::loop); - if (predicate ()) - { - auto transaction = ledger.tx_begin_read (); - - while (predicate ()) - { - debug_assert (!candidates.empty ()); - auto candidate = candidates.front (); - candidates.pop_front (); - - lock.unlock (); - - run_one (transaction, candidate); - - lock.lock (); - } - } - condition.wait_for (lock, network_constants.optimistic_activation_delay / 2, [this] () { return stopped || predicate (); }); + + if (stopped) + { + return; + } + + lock.unlock (); + + // Acquire transaction outside of the lock + auto transaction = ledger.tx_begin_read (); + + lock.lock (); + + while (predicate () && !stopped) + { + debug_assert (!candidates.empty ()); + 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 (); + } } } @@ -160,7 +178,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); diff --git a/nano/node/scheduler/optimistic.hpp b/nano/node/scheduler/optimistic.hpp index 24edd1e73..32557263d 100644 --- a/nano/node/scheduler/optimistic.hpp +++ b/nano/node/scheduler/optimistic.hpp @@ -33,10 +33,10 @@ 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{ 32 }; /** Maximum number of candidates stored in memory */ - std::size_t max_size{ 1024 * 64 }; + std::size_t max_size{ 1024 * 4 }; }; class optimistic final @@ -82,17 +82,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 From c71f14fc009718039fbdf399c84e491aa9d57502 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Mon, 1 Sep 2025 23:54:09 +0200 Subject: [PATCH 2/4] Adjust optimistic scheduler config --- nano/lib/constants.hpp | 4 ---- nano/node/scheduler/optimistic.cpp | 6 ++++-- nano/node/scheduler/optimistic.hpp | 5 ++++- 3 files changed, 8 insertions(+), 7 deletions(-) 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 cd38ab9ae..1ba7f764e 100644 --- a/nano/node/scheduler/optimistic.cpp +++ b/nano/node/scheduler/optimistic.cpp @@ -126,7 +126,7 @@ bool nano::scheduler::optimistic::predicate () const auto candidate = candidates.get ().begin (); debug_assert (candidate != candidates.get ().end ()); - return elapsed (candidate->timestamp, network_constants.optimistic_activation_delay); + return elapsed (candidate->timestamp, config.activation_delay); } void nano::scheduler::optimistic::run () @@ -136,7 +136,7 @@ void nano::scheduler::optimistic::run () { stats.inc (nano::stat::type::optimistic_scheduler, nano::stat::detail::loop); - condition.wait_for (lock, network_constants.optimistic_activation_delay / 2, [this] () { + condition.wait_for (lock, config.activation_delay / 2, [this] () { return stopped || predicate (); }); @@ -204,6 +204,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 (); } @@ -213,6 +214,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 32557263d..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 */ - uint64_t gap_threshold{ 32 }; + uint64_t gap_threshold{ 16 }; /** Maximum number of candidates stored in memory */ 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 From 2b0c6648fd85a3ae7eab3472ef70888971b1eb0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Wed, 3 Sep 2025 14:37:24 +0200 Subject: [PATCH 3/4] Add check before thread wake up --- nano/node/scheduler/optimistic.cpp | 54 ++++++++++++++++-------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/nano/node/scheduler/optimistic.cpp b/nano/node/scheduler/optimistic.cpp index 1ba7f764e..51613d209 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 @@ -79,8 +83,6 @@ bool nano::scheduler::optimistic::activate (const nano::account & account, const return false; } - bool activated = false; - if (activate_predicate (account_info, conf_info)) { nano::lock_guard lock{ mutex }; @@ -99,15 +101,12 @@ bool nano::scheduler::optimistic::activate (const nano::account & account, const candidates.pop_front (); } - activated = inserted; + // Not notifying the thread immediately here, since we need to wait for activation_delay to elapse + + return inserted; } - if (activated) - { - condition.notify_all (); - } - - return activated; + return false; // Not activated } bool nano::scheduler::optimistic::predicate () const @@ -134,8 +133,6 @@ 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 (); }); @@ -145,26 +142,31 @@ void nano::scheduler::optimistic::run () return; } - lock.unlock (); - - // Acquire transaction outside of the lock - auto transaction = ledger.tx_begin_read (); - - lock.lock (); - - while (predicate () && !stopped) + if (predicate ()) { - debug_assert (!candidates.empty ()); - auto & height_index = candidates.get (); - auto candidate = *height_index.begin (); - height_index.erase (height_index.begin ()); + stats.inc (nano::stat::type::optimistic_scheduler, nano::stat::detail::loop); lock.unlock (); - transaction.refresh_if_needed (); - run_one (transaction, candidate); + // Acquire transaction outside of the lock + auto transaction = ledger.tx_begin_read (); lock.lock (); + + while (predicate () && !stopped) + { + debug_assert (!candidates.empty ()); + 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 (); + } } } } From 11256f117df5d3100647411b131222a6b35708a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Wed, 3 Sep 2025 22:30:55 +0200 Subject: [PATCH 4/4] Fix test --- nano/core_test/optimistic_scheduler.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nano/core_test/optimistic_scheduler.cpp b/nano/core_test/optimistic_scheduler.cpp index a61ac5783..542c758d0 100644 --- a/nano/core_test/optimistic_scheduler.cpp +++ b/nano/core_test/optimistic_scheduler.cpp @@ -81,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 ());