From 9807e86b639635e5d2572332048d0bc2e78c3e09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Tue, 15 Oct 2024 13:22:21 +0200 Subject: [PATCH 01/11] Track election confirm stats --- nano/lib/stats_enums.hpp | 2 ++ nano/node/election.cpp | 2 ++ nano/node/node.cpp | 2 ++ 3 files changed, 6 insertions(+) diff --git a/nano/lib/stats_enums.hpp b/nano/lib/stats_enums.hpp index ee9d8799dc..d855b02950 100644 --- a/nano/lib/stats_enums.hpp +++ b/nano/lib/stats_enums.hpp @@ -249,6 +249,8 @@ enum class detail generate_vote_final, broadcast_block_initial, broadcast_block_repeat, + confirm_once, + confirm_once_failed, // election types manual, diff --git a/nano/node/election.cpp b/nano/node/election.cpp index 98fa9ba491..48b856a608 100644 --- a/nano/node/election.cpp +++ b/nano/node/election.cpp @@ -56,6 +56,7 @@ void nano::election::confirm_once (nano::unique_lock & lock_a) node.active.recently_confirmed.put (qualified_root, status_l.winner->hash ()); + node.stats.inc (nano::stat::type::election, nano::stat::detail::confirm_once); node.logger.trace (nano::log::type::election, nano::log::detail::election_confirmed, nano::log::arg{ "id", id }, nano::log::arg{ "qualified_root", qualified_root }, @@ -74,6 +75,7 @@ void nano::election::confirm_once (nano::unique_lock & lock_a) } else { + node.stats.inc (nano::stat::type::election, nano::stat::detail::confirm_once_failed); lock_a.unlock (); } } diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 3cda7ce369..438298239d 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -1143,6 +1143,8 @@ void nano::node::ongoing_online_weight_calculation () void nano::node::process_confirmed (nano::election_status const & status_a, uint64_t iteration_a) { + stats.inc (nano::stat::type::process_confirmed, nano::stat::detail::initiate); + auto hash (status_a.winner->hash ()); decltype (iteration_a) const num_iters = (config.block_processor_batch_max_time / network_params.node.process_confirmed_interval) * 4; if (auto block_l = ledger.any.block_get (ledger.tx_begin_read (), hash)) From 2badf066470bb1d082c6ade98d65ea528569d0de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 12 Oct 2024 13:30:12 +0200 Subject: [PATCH 02/11] Stats for `node::process_confirmed ()` --- nano/lib/stats_enums.hpp | 3 +++ nano/node/node.cpp | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/nano/lib/stats_enums.hpp b/nano/lib/stats_enums.hpp index d855b02950..ee5629ad4b 100644 --- a/nano/lib/stats_enums.hpp +++ b/nano/lib/stats_enums.hpp @@ -93,6 +93,7 @@ enum class type message_processor, message_processor_overfill, message_processor_type, + process_confirmed, _last // Must be the last enum }; @@ -134,6 +135,8 @@ enum class detail cemented, cooldown, empty, + done, + retry, // processing queue queue, diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 438298239d..346c20ae10 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -1149,12 +1149,15 @@ void nano::node::process_confirmed (nano::election_status const & status_a, uint decltype (iteration_a) const num_iters = (config.block_processor_batch_max_time / network_params.node.process_confirmed_interval) * 4; if (auto block_l = ledger.any.block_get (ledger.tx_begin_read (), hash)) { + stats.inc (nano::stat::type::process_confirmed, nano::stat::detail::done); logger.trace (nano::log::type::node, nano::log::detail::process_confirmed, nano::log::arg{ "block", block_l }); confirming_set.add (block_l->hash ()); } else if (iteration_a < num_iters) { + stats.inc (nano::stat::type::process_confirmed, nano::stat::detail::retry); + iteration_a++; std::weak_ptr node_w (shared ()); election_workers.add_timed_task (std::chrono::steady_clock::now () + network_params.node.process_confirmed_interval, [node_w, status_a, iteration_a] () { @@ -1166,6 +1169,8 @@ void nano::node::process_confirmed (nano::election_status const & status_a, uint } else { + stats.inc (nano::stat::type::process_confirmed, nano::stat::detail::timeout); + // Do some cleanup due to this block never being processed by confirmation height processor active.remove_election_winner_details (hash); } From 0747d4afe46878b357fa08f43376c56f505e30c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 12 Oct 2024 11:59:17 +0200 Subject: [PATCH 03/11] Handle rollbacks when cementing --- nano/node/confirming_set.cpp | 8 ++++++++ nano/secure/ledger.cpp | 5 ++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/nano/node/confirming_set.cpp b/nano/node/confirming_set.cpp index c499e35057..da342a4e52 100644 --- a/nano/node/confirming_set.cpp +++ b/nano/node/confirming_set.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -180,6 +181,13 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) stats.inc (nano::stat::type::confirming_set, nano::stat::detail::cementing); + // The block might be rolled back before it's fully cemented + if (!ledger.any.block_exists (transaction, hash)) + { + stats.inc (nano::stat::type::confirming_set, nano::stat::detail::missing_block); + break; + } + auto added = ledger.confirm (transaction, hash, config.max_blocks); if (!added.empty ()) { diff --git a/nano/secure/ledger.cpp b/nano/secure/ledger.cpp index efe7c5ad86..cba173a6c8 100644 --- a/nano/secure/ledger.cpp +++ b/nano/secure/ledger.cpp @@ -865,7 +865,10 @@ std::deque> nano::ledger::confirm (secure::write_tr bool refreshed = transaction.refresh_if_needed (); if (refreshed) { - release_assert (any.block_exists (transaction, target_hash), "block was rolled back during cementing"); + if (!any.block_exists (transaction, target_hash)) + { + break; // Block was rolled back during cementing + } } // Early return might leave parts of the dependency tree unconfirmed From b7ba1eb08a4b2c54dd87a204f8df4afb47f63e17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Tue, 15 Oct 2024 15:27:12 +0200 Subject: [PATCH 04/11] Rework cemented notification --- nano/node/active_elections.cpp | 18 +++++++++--------- nano/node/confirming_set.cpp | 22 +++++++++++----------- nano/node/confirming_set.hpp | 12 ++++-------- 3 files changed, 24 insertions(+), 28 deletions(-) diff --git a/nano/node/active_elections.cpp b/nano/node/active_elections.cpp index 134c649dfa..2f04243f78 100644 --- a/nano/node/active_elections.cpp +++ b/nano/node/active_elections.cpp @@ -30,17 +30,17 @@ nano::active_elections::active_elections (nano::node & node_a, nano::confirming_ { count_by_behavior.fill (0); // Zero initialize array - confirming_set.batch_cemented.add ([this] (nano::confirming_set::cemented_notification const & notification) { + confirming_set.batch_cemented.add ([this] (auto const & cemented) { + auto transaction = node.ledger.tx_begin_read (); + for (auto const & [block, confirmation_root] : cemented) { - auto transaction = node.ledger.tx_begin_read (); - for (auto const & [block, confirmation_root] : notification.cemented) - { - transaction.refresh_if_needed (); - - block_cemented_callback (transaction, block, confirmation_root); - } + transaction.refresh_if_needed (); + block_cemented_callback (transaction, block, confirmation_root); } - for (auto const & hash : notification.already_cemented) + }); + + confirming_set.already_cemented.add ([this] (auto const & already_cemented) { + for (auto const & hash : already_cemented) { block_already_cemented_callback (hash); } diff --git a/nano/node/confirming_set.cpp b/nano/node/confirming_set.cpp index da342a4e52..1d2aa2facc 100644 --- a/nano/node/confirming_set.cpp +++ b/nano/node/confirming_set.cpp @@ -12,8 +12,8 @@ nano::confirming_set::confirming_set (confirming_set_config const & config_a, na stats{ stats_a }, notification_workers{ 1, nano::thread_role::name::confirmation_height_notifications } { - batch_cemented.add ([this] (auto const & notification) { - for (auto const & [block, confirmation_root] : notification.cemented) + batch_cemented.add ([this] (auto const & cemented) { + for (auto const & [block, confirmation_root] : cemented) { cemented_observers.notify (block); } @@ -124,17 +124,17 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) std::deque cemented; std::deque already; - auto batch = next_batch (256); + auto batch = next_batch (batch_size); lock.unlock (); - auto notify = [this, &cemented, &already] () { - cemented_notification notification{}; - notification.cemented.swap (cemented); - notification.already_cemented.swap (already); + auto notify = [this, &cemented] () { + std::deque batch; + batch.swap (cemented); std::unique_lock lock{ mutex }; + // It's possible that ledger cementing happens faster than the notifications can be processed by other components, cooldown here while (notification_workers.num_queued_tasks () >= config.max_queued_notifications) { stats.inc (nano::stat::type::confirming_set, nano::stat::detail::cooldown); @@ -145,9 +145,9 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) } } - notification_workers.push_task ([this, notification = std::move (notification)] () { + notification_workers.push_task ([this, batch = std::move (batch)] () { stats.inc (nano::stat::type::confirming_set, nano::stat::detail::notify); - batch_cemented.notify (notification); + batch_cemented.notify (batch); }); }; @@ -211,9 +211,9 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) } notify (); - release_assert (cemented.empty ()); - release_assert (already.empty ()); + + already_cemented.notify (already); } nano::container_info nano::confirming_set::container_info () const diff --git a/nano/node/confirming_set.hpp b/nano/node/confirming_set.hpp index e5ef4857f1..b9e568db4b 100644 --- a/nano/node/confirming_set.hpp +++ b/nano/node/confirming_set.hpp @@ -48,16 +48,10 @@ class confirming_set final nano::container_info container_info () const; public: // Events - // Observers will be called once ledger has blocks marked as confirmed using cemented_t = std::pair, nano::block_hash>; // + nano::observer_set const &> batch_cemented; + nano::observer_set const &> already_cemented; - struct cemented_notification - { - std::deque cemented; - std::deque already_cemented; - }; - - nano::observer_set batch_cemented; nano::observer_set> cemented_observers; private: // Dependencies @@ -79,5 +73,7 @@ class confirming_set final mutable std::mutex mutex; std::condition_variable condition; std::thread thread; + + static size_t constexpr batch_size = 256; }; } From 155cb1c2526036defc14d65e9c043871d65632dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Tue, 15 Oct 2024 16:18:53 +0200 Subject: [PATCH 05/11] Keep election in confirming set --- nano/node/active_elections.cpp | 2 +- nano/node/confirming_set.cpp | 27 ++++++++++----------- nano/node/confirming_set.hpp | 43 ++++++++++++++++++++++++++++++---- 3 files changed, 52 insertions(+), 20 deletions(-) diff --git a/nano/node/active_elections.cpp b/nano/node/active_elections.cpp index 2f04243f78..a8fb6469bc 100644 --- a/nano/node/active_elections.cpp +++ b/nano/node/active_elections.cpp @@ -32,7 +32,7 @@ nano::active_elections::active_elections (nano::node & node_a, nano::confirming_ confirming_set.batch_cemented.add ([this] (auto const & cemented) { auto transaction = node.ledger.tx_begin_read (); - for (auto const & [block, confirmation_root] : cemented) + for (auto const & [block, confirmation_root, election] : cemented) { transaction.refresh_if_needed (); block_cemented_callback (transaction, block, confirmation_root); diff --git a/nano/node/confirming_set.cpp b/nano/node/confirming_set.cpp index 1d2aa2facc..f7a0125340 100644 --- a/nano/node/confirming_set.cpp +++ b/nano/node/confirming_set.cpp @@ -13,9 +13,9 @@ nano::confirming_set::confirming_set (confirming_set_config const & config_a, na notification_workers{ 1, nano::thread_role::name::confirmation_height_notifications } { batch_cemented.add ([this] (auto const & cemented) { - for (auto const & [block, confirmation_root] : cemented) + for (auto const & context : cemented) { - cemented_observers.notify (block); + cemented_observers.notify (context.block); } }); } @@ -25,12 +25,12 @@ nano::confirming_set::~confirming_set () debug_assert (!thread.joinable ()); } -void nano::confirming_set::add (nano::block_hash const & hash) +void nano::confirming_set::add (nano::block_hash const & hash, std::shared_ptr const & election) { bool added = false; { std::lock_guard lock{ mutex }; - auto [it, inserted] = set.insert (hash); + auto [it, inserted] = set.push_back ({ hash, election }); added = inserted; } if (added) @@ -71,7 +71,7 @@ void nano::confirming_set::stop () bool nano::confirming_set::exists (nano::block_hash const & hash) const { std::lock_guard lock{ mutex }; - return set.count (hash) != 0; + return set.get ().contains (hash); } std::size_t nano::confirming_set::size () const @@ -100,17 +100,16 @@ void nano::confirming_set::run () } } -std::deque nano::confirming_set::next_batch (size_t max_count) +auto nano::confirming_set::next_batch (size_t max_count) -> std::deque { debug_assert (!mutex.try_lock ()); debug_assert (!set.empty ()); - std::deque results; + std::deque results; while (!set.empty () && results.size () < max_count) { - auto it = set.begin (); - results.push_back (*it); - set.erase (it); + results.push_back (set.front ()); + set.pop_front (); } return results; } @@ -121,7 +120,7 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) debug_assert (!mutex.try_lock ()); debug_assert (!set.empty ()); - std::deque cemented; + std::deque cemented; std::deque already; auto batch = next_batch (batch_size); @@ -129,7 +128,7 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) lock.unlock (); auto notify = [this, &cemented] () { - std::deque batch; + std::deque batch; batch.swap (cemented); std::unique_lock lock{ mutex }; @@ -164,7 +163,7 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) { auto transaction = ledger.tx_begin_write (nano::store::writer::confirmation_height); - for (auto const & hash : batch) + for (auto const & [hash, election] : batch) { do { @@ -195,7 +194,7 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) stats.add (nano::stat::type::confirming_set, nano::stat::detail::cemented, added.size ()); for (auto & block : added) { - cemented.emplace_back (block, hash); + cemented.push_back ({ block, hash, election }); } } else diff --git a/nano/node/confirming_set.hpp b/nano/node/confirming_set.hpp index b9e568db4b..5eb429672e 100644 --- a/nano/node/confirming_set.hpp +++ b/nano/node/confirming_set.hpp @@ -4,6 +4,13 @@ #include #include #include +#include + +#include +#include +#include +#include +#include #include #include @@ -11,6 +18,8 @@ #include #include +namespace mi = boost::multi_index; + namespace nano { class confirming_set_config final @@ -40,7 +49,7 @@ class confirming_set final void stop (); // Adds a block to the set of blocks to be confirmed - void add (nano::block_hash const & hash); + void add (nano::block_hash const & hash, std::shared_ptr const & election = nullptr); // Added blocks will remain in this set until after ledger has them marked as confirmed. bool exists (nano::block_hash const & hash) const; std::size_t size () const; @@ -48,8 +57,14 @@ class confirming_set final nano::container_info container_info () const; public: // Events - using cemented_t = std::pair, nano::block_hash>; // - nano::observer_set const &> batch_cemented; + struct context + { + std::shared_ptr block; + nano::block_hash confirmation_root; + std::shared_ptr election; + }; + + nano::observer_set const &> batch_cemented; nano::observer_set const &> already_cemented; nano::observer_set> cemented_observers; @@ -60,12 +75,30 @@ class confirming_set final nano::stats & stats; private: + struct entry + { + nano::block_hash hash; + std::shared_ptr election; + }; + void run (); void run_batch (std::unique_lock &); - std::deque next_batch (size_t max_count); + std::deque next_batch (size_t max_count); private: - std::unordered_set set; + // clang-format off + class tag_hash {}; + class tag_sequenced {}; + + using ordered_entries = boost::multi_index_container>, + mi::hashed_unique, + mi::member> + >>; + // clang-format on + + ordered_entries set; nano::thread_pool notification_workers; From b44040347b58007acf4a8bf1a476895a4d2a9afc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Tue, 15 Oct 2024 17:19:01 +0200 Subject: [PATCH 06/11] Rename `exists ()` to `contains ()` --- nano/core_test/active_elections.cpp | 2 +- nano/core_test/confirming_set.cpp | 4 ++-- nano/node/confirming_set.cpp | 2 +- nano/node/confirming_set.hpp | 2 +- nano/node/json_handler.cpp | 2 +- nano/node/node.cpp | 8 ++++---- nano/node/wallet.cpp | 2 +- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/nano/core_test/active_elections.cpp b/nano/core_test/active_elections.cpp index 6c13d4a02c..05c243aa3e 100644 --- a/nano/core_test/active_elections.cpp +++ b/nano/core_test/active_elections.cpp @@ -1224,7 +1224,7 @@ TEST (active_elections, activate_inactive) ASSERT_NE (nullptr, election); election->force_confirm (); - ASSERT_TIMELY (5s, !node.confirming_set.exists (send2->hash ())); + ASSERT_TIMELY (5s, !node.confirming_set.contains (send2->hash ())); ASSERT_TIMELY (5s, node.block_confirmed (send2->hash ())); ASSERT_TIMELY (5s, node.block_confirmed (send->hash ())); diff --git a/nano/core_test/confirming_set.cpp b/nano/core_test/confirming_set.cpp index 7d25017874..46e7b914a1 100644 --- a/nano/core_test/confirming_set.cpp +++ b/nano/core_test/confirming_set.cpp @@ -30,7 +30,7 @@ TEST (confirming_set, add_exists) nano::confirming_set confirming_set{ config, ctx.ledger (), ctx.stats () }; auto send = ctx.blocks ()[0]; confirming_set.add (send->hash ()); - ASSERT_TRUE (confirming_set.exists (send->hash ())); + ASSERT_TRUE (confirming_set.contains (send->hash ())); } TEST (confirming_set, process_one) @@ -242,7 +242,7 @@ TEST (confirmation_callback, dependent_election) // Wait for blocks to be confirmed in ledger, callbacks will happen after ASSERT_TIMELY_EQ (5s, 3, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); // Once the item added to the confirming set no longer exists, callbacks have completed - ASSERT_TIMELY (5s, !node->confirming_set.exists (send2->hash ())); + ASSERT_TIMELY (5s, !node->confirming_set.contains (send2->hash ())); ASSERT_TIMELY_EQ (5s, 1, node->stats.count (nano::stat::type::confirmation_observer, nano::stat::detail::active_quorum, nano::stat::dir::out)); ASSERT_TIMELY_EQ (5s, 1, node->stats.count (nano::stat::type::confirmation_observer, nano::stat::detail::active_conf_height, nano::stat::dir::out)); diff --git a/nano/node/confirming_set.cpp b/nano/node/confirming_set.cpp index f7a0125340..de92e853a9 100644 --- a/nano/node/confirming_set.cpp +++ b/nano/node/confirming_set.cpp @@ -68,7 +68,7 @@ void nano::confirming_set::stop () notification_workers.stop (); } -bool nano::confirming_set::exists (nano::block_hash const & hash) const +bool nano::confirming_set::contains (nano::block_hash const & hash) const { std::lock_guard lock{ mutex }; return set.get ().contains (hash); diff --git a/nano/node/confirming_set.hpp b/nano/node/confirming_set.hpp index 5eb429672e..00329fea83 100644 --- a/nano/node/confirming_set.hpp +++ b/nano/node/confirming_set.hpp @@ -51,7 +51,7 @@ class confirming_set final // Adds a block to the set of blocks to be confirmed void add (nano::block_hash const & hash, std::shared_ptr const & election = nullptr); // Added blocks will remain in this set until after ledger has them marked as confirmed. - bool exists (nano::block_hash const & hash) const; + bool contains (nano::block_hash const & hash) const; std::size_t size () const; nano::container_info container_info () const; diff --git a/nano/node/json_handler.cpp b/nano/node/json_handler.cpp index 198bdeac13..8b75b9d0f7 100644 --- a/nano/node/json_handler.cpp +++ b/nano/node/json_handler.cpp @@ -1187,7 +1187,7 @@ void nano::json_handler::block_confirm () if (!node.ledger.confirmed.block_exists_or_pruned (transaction, hash)) { // Start new confirmation for unconfirmed (or not being confirmed) block - if (!node.confirming_set.exists (hash)) + if (!node.confirming_set.contains (hash)) { node.start_election (std::move (block_l)); } diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 346c20ae10..5871f2cff8 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -1104,14 +1104,14 @@ void nano::node::start_election (std::shared_ptr const & block) scheduler.manual.push (block); } -bool nano::node::block_confirmed (nano::block_hash const & hash_a) +bool nano::node::block_confirmed (nano::block_hash const & hash) { - return ledger.confirmed.block_exists_or_pruned (ledger.tx_begin_read (), hash_a); + return ledger.confirmed.block_exists_or_pruned (ledger.tx_begin_read (), hash); } -bool nano::node::block_confirmed_or_being_confirmed (nano::secure::transaction const & transaction, nano::block_hash const & hash_a) +bool nano::node::block_confirmed_or_being_confirmed (nano::secure::transaction const & transaction, nano::block_hash const & hash) { - return confirming_set.exists (hash_a) || ledger.confirmed.block_exists_or_pruned (transaction, hash_a); + return confirming_set.contains (hash) || ledger.confirmed.block_exists_or_pruned (transaction, hash); } bool nano::node::block_confirmed_or_being_confirmed (nano::block_hash const & hash_a) diff --git a/nano/node/wallet.cpp b/nano/node/wallet.cpp index 837a2ca417..716f78ee2f 100644 --- a/nano/node/wallet.cpp +++ b/nano/node/wallet.cpp @@ -1201,7 +1201,7 @@ bool nano::wallet::search_receivable (store::transaction const & wallet_transact // Receive confirmed block receive_async (hash, representative, amount, account, [] (std::shared_ptr const &) {}); } - else if (!wallets.node.confirming_set.exists (hash)) + else if (!wallets.node.confirming_set.contains (hash)) { auto block = wallets.node.ledger.any.block_get (block_transaction, hash); if (block) From e456b2282b14686687a659989bc81f96d60ffb2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Tue, 15 Oct 2024 17:30:57 +0200 Subject: [PATCH 07/11] Remove `election_winner_details` --- nano/core_test/confirming_set.cpp | 27 ---------- nano/core_test/ledger_confirm.cpp | 23 -------- nano/lib/stats_enums.hpp | 1 + nano/node/active_elections.cpp | 87 ++++++++----------------------- nano/node/active_elections.hpp | 13 +---- nano/node/election.cpp | 24 ++++----- nano/node/node.cpp | 1 - nano/slow_test/node.cpp | 9 ---- 8 files changed, 37 insertions(+), 148 deletions(-) diff --git a/nano/core_test/confirming_set.cpp b/nano/core_test/confirming_set.cpp index 46e7b914a1..a2b3d60803 100644 --- a/nano/core_test/confirming_set.cpp +++ b/nano/core_test/confirming_set.cpp @@ -111,7 +111,6 @@ TEST (confirmation_callback, observer_callbacks) ASSERT_EQ (2, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (3, node->ledger.cemented_count ()); - ASSERT_EQ (0, node->active.election_winner_details_size ()); } // The callback and confirmation history should only be updated after confirmation height is set (and not just after voting) @@ -186,7 +185,6 @@ TEST (confirmation_callback, confirmed_history) ASSERT_TIMELY_EQ (5s, 1, node->stats.count (nano::stat::type::confirmation_observer, nano::stat::detail::inactive_conf_height, nano::stat::dir::out)); ASSERT_TIMELY_EQ (5s, 2, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (3, node->ledger.cemented_count ()); - ASSERT_EQ (0, node->active.election_winner_details_size ()); } TEST (confirmation_callback, dependent_election) @@ -248,29 +246,4 @@ TEST (confirmation_callback, dependent_election) ASSERT_TIMELY_EQ (5s, 1, node->stats.count (nano::stat::type::confirmation_observer, nano::stat::detail::active_conf_height, nano::stat::dir::out)); ASSERT_TIMELY_EQ (5s, 1, node->stats.count (nano::stat::type::confirmation_observer, nano::stat::detail::inactive_conf_height, nano::stat::dir::out)); ASSERT_EQ (4, node->ledger.cemented_count ()); - - ASSERT_EQ (0, node->active.election_winner_details_size ()); -} - -TEST (confirmation_callback, election_winner_details_clearing_node_process_confirmed) -{ - // Make sure election_winner_details is also cleared if the block never enters the confirmation height processor from node::process_confirmed - nano::test::system system (1); - auto node = system.nodes.front (); - - nano::block_builder builder; - auto send = builder - .send () - .previous (nano::dev::genesis->hash ()) - .destination (nano::dev::genesis_key.pub) - .balance (nano::dev::constants.genesis_amount - nano::Knano_ratio) - .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) - .work (*system.work.generate (nano::dev::genesis->hash ())) - .build (); - // Add to election_winner_details. Use an unrealistic iteration so that it should fall into the else case and do a cleanup - node->active.add_election_winner_details (send->hash (), nullptr); - nano::election_status election; - election.winner = send; - node->process_confirmed (election, 1000000); - ASSERT_EQ (0, node->active.election_winner_details_size ()); } diff --git a/nano/core_test/ledger_confirm.cpp b/nano/core_test/ledger_confirm.cpp index 1301c9ff95..f9bfeff998 100644 --- a/nano/core_test/ledger_confirm.cpp +++ b/nano/core_test/ledger_confirm.cpp @@ -752,29 +752,6 @@ TEST (ledger_confirm, observers) ASSERT_EQ (2, node1->ledger.cemented_count ()); } -TEST (ledger_confirm, election_winner_details_clearing_node_process_confirmed) -{ - // Make sure election_winner_details is also cleared if the block never enters the confirmation height processor from node::process_confirmed - nano::test::system system (1); - auto node = system.nodes.front (); - - nano::block_builder builder; - auto send = builder - .send () - .previous (nano::dev::genesis->hash ()) - .destination (nano::dev::genesis_key.pub) - .balance (nano::dev::constants.genesis_amount - nano::Knano_ratio) - .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) - .work (*system.work.generate (nano::dev::genesis->hash ())) - .build (); - // Add to election_winner_details. Use an unrealistic iteration so that it should fall into the else case and do a cleanup - node->active.add_election_winner_details (send->hash (), nullptr); - nano::election_status election; - election.winner = send; - node->process_confirmed (election, 1000000); - ASSERT_EQ (0, node->active.election_winner_details_size ()); -} - TEST (ledger_confirm, pruned_source) { nano::test::system system; diff --git a/nano/lib/stats_enums.hpp b/nano/lib/stats_enums.hpp index ee5629ad4b..ea8c07780f 100644 --- a/nano/lib/stats_enums.hpp +++ b/nano/lib/stats_enums.hpp @@ -412,6 +412,7 @@ enum class detail // active_elections started, stopped, + confirm_dependent, // unchecked put, diff --git a/nano/node/active_elections.cpp b/nano/node/active_elections.cpp index a8fb6469bc..feafab86bf 100644 --- a/nano/node/active_elections.cpp +++ b/nano/node/active_elections.cpp @@ -25,24 +25,16 @@ nano::active_elections::active_elections (nano::node & node_a, nano::confirming_ confirming_set{ confirming_set_a }, block_processor{ block_processor_a }, recently_confirmed{ config.confirmation_cache }, - recently_cemented{ config.confirmation_history_size }, - election_time_to_live{ node_a.network_params.network.is_dev_network () ? 0s : 2s } + recently_cemented{ config.confirmation_history_size } { count_by_behavior.fill (0); // Zero initialize array confirming_set.batch_cemented.add ([this] (auto const & cemented) { auto transaction = node.ledger.tx_begin_read (); - for (auto const & [block, confirmation_root, election] : cemented) + for (auto const & [block, confirmation_root, source_election] : cemented) { transaction.refresh_if_needed (); - block_cemented_callback (transaction, block, confirmation_root); - } - }); - - confirming_set.already_cemented.add ([this] (auto const & already_cemented) { - for (auto const & hash : already_cemented) - { - block_already_cemented_callback (hash); + block_cemented (transaction, block, confirmation_root, source_election); } }); @@ -91,28 +83,31 @@ void nano::active_elections::stop () clear (); } -void nano::active_elections::block_cemented_callback (nano::secure::transaction const & transaction, std::shared_ptr const & block, nano::block_hash const & confirmation_root) +void nano::active_elections::block_cemented (nano::secure::transaction const & transaction, std::shared_ptr const & block, nano::block_hash const & confirmation_root, std::shared_ptr const & source_election) { debug_assert (node.block_confirmed (block->hash ())); - if (auto election_l = election (block->qualified_root ())) + // Dependent elections are implicitly confirmed when their block is cemented + auto dependend_election = election (block->qualified_root ()); + if (dependend_election) { - election_l->try_confirm (block->hash ()); + node.stats.inc (nano::stat::type::active_elections, nano::stat::detail::confirm_dependent); + dependend_election->try_confirm (block->hash ()); } - auto election = remove_election_winner_details (block->hash ()); + nano::election_status status; std::vector votes; status.winner = block; - if (election) - { - status = election->get_status (); - votes = election->votes_with_weight (); - } - if (block->hash () == confirmation_root) + + // Check if the currently cemented block was part of an election that triggered the confirmation + if (source_election && source_election->qualified_root == block->qualified_root ()) { + status = source_election->get_status (); + debug_assert (status.winner->hash () == block->hash ()); + votes = source_election->votes_with_weight (); status.type = nano::election_status_type::active_confirmed_quorum; } - else if (election) + else if (dependend_election) { status.type = nano::election_status_type::active_confirmation_height; } @@ -120,12 +115,16 @@ void nano::active_elections::block_cemented_callback (nano::secure::transaction { status.type = nano::election_status_type::inactive_confirmation_height; } + recently_cemented.put (status); node.stats.inc (nano::stat::type::active_elections, nano::stat::detail::cemented); node.stats.inc (nano::stat::type::active_elections_cemented, to_stat_detail (status.type)); - node.logger.trace (nano::log::type::active_elections, nano::log::detail::active_cemented, nano::log::arg{ "election", election }); + node.logger.trace (nano::log::type::active_elections, nano::log::detail::active_cemented, + nano::log::arg{ "block", block }, + nano::log::arg{ "confirmation_root", confirmation_root }, + nano::log::arg{ "source_election", source_election }); notify_observers (transaction, status, votes); @@ -185,39 +184,6 @@ void nano::active_elections::activate_successors (nano::secure::transaction cons } } -void nano::active_elections::add_election_winner_details (nano::block_hash const & hash_a, std::shared_ptr const & election_a) -{ - nano::lock_guard guard{ election_winner_details_mutex }; - election_winner_details.emplace (hash_a, election_a); -} - -std::shared_ptr nano::active_elections::remove_election_winner_details (nano::block_hash const & hash_a) -{ - std::shared_ptr result; - { - nano::lock_guard guard{ election_winner_details_mutex }; - auto existing = election_winner_details.find (hash_a); - if (existing != election_winner_details.end ()) - { - result = existing->second; - election_winner_details.erase (existing); - } - } - - vacancy_update (); - - return result; -} - -void nano::active_elections::block_already_cemented_callback (nano::block_hash const & hash_a) -{ - // Depending on timing there is a situation where the election_winner_details is not reset. - // This can happen when a block wins an election, and the block is confirmed + observer - // called before the block hash gets added to election_winner_details. If the block is confirmed - // callbacks have already been done, so we can safely just remove it. - remove_election_winner_details (hash_a); -} - int64_t nano::active_elections::limit (nano::election_behavior behavior) const { switch (behavior) @@ -265,7 +231,7 @@ int64_t nano::active_elections::vacancy (nano::election_behavior behavior) const }; auto election_winners_vacancy = [this] () -> int64_t { - return static_cast (config.max_election_winners) - static_cast (election_winner_details_size ()); + return static_cast (config.max_election_winners) - static_cast (confirming_set.size ()); }; return std::min (election_vacancy (behavior), election_winners_vacancy ()); @@ -572,12 +538,6 @@ bool nano::active_elections::publish (std::shared_ptr const & block return result; } -std::size_t nano::active_elections::election_winner_details_size () const -{ - nano::lock_guard guard{ election_winner_details_mutex }; - return election_winner_details.size (); -} - void nano::active_elections::clear () { // TODO: Call erased_callback for each election @@ -595,7 +555,6 @@ nano::container_info nano::active_elections::container_info () const nano::container_info info; info.put ("roots", roots.size ()); - info.put ("election_winner_details", election_winner_details_size ()); info.put ("normal", static_cast (count_by_behavior[nano::election_behavior::priority])); info.put ("hinted", static_cast (count_by_behavior[nano::election_behavior::hinted])); info.put ("optimistic", static_cast (count_by_behavior[nano::election_behavior::optimistic])); diff --git a/nano/node/active_elections.hpp b/nano/node/active_elections.hpp index 222d5d278d..1a76cf0448 100644 --- a/nano/node/active_elections.hpp +++ b/nano/node/active_elections.hpp @@ -123,10 +123,6 @@ class active_elections final int64_t vacancy (nano::election_behavior behavior) const; std::function vacancy_update{ [] () {} }; - std::size_t election_winner_details_size () const; - void add_election_winner_details (nano::block_hash const &, std::shared_ptr const &); - std::shared_ptr remove_election_winner_details (nano::block_hash const &); - nano::container_info container_info () const; private: @@ -139,8 +135,7 @@ class active_elections final std::vector> list_active_impl (std::size_t) const; void activate_successors (nano::secure::transaction const &, std::shared_ptr const & block); void notify_observers (nano::secure::transaction const &, nano::election_status const & status, std::vector const & votes) const; - void block_cemented_callback (nano::secure::transaction const &, std::shared_ptr const & block, nano::block_hash const & confirmation_root); - void block_already_cemented_callback (nano::block_hash const & hash); + void block_cemented (nano::secure::transaction const &, std::shared_ptr const & block, nano::block_hash const & confirmation_root, std::shared_ptr const & source_election); private: // Dependencies active_elections_config const & config; @@ -157,12 +152,6 @@ class active_elections final mutable nano::mutex mutex{ mutex_identifier (mutexes::active) }; private: - mutable nano::mutex election_winner_details_mutex{ mutex_identifier (mutexes::election_winner_details) }; - std::unordered_map> election_winner_details; - - // Maximum time an election can be kept active if it is extending the container - std::chrono::seconds const election_time_to_live; - /** Keeps track of number of elections by election behavior (normal, hinted, optimistic) */ nano::enum_array count_by_behavior{}; diff --git a/nano/node/election.cpp b/nano/node/election.cpp index 48b856a608..a8a392dbe5 100644 --- a/nano/node/election.cpp +++ b/nano/node/election.cpp @@ -35,18 +35,16 @@ nano::election::election (nano::node & node_a, std::shared_ptr cons last_blocks.emplace (block_a->hash (), block_a); } -void nano::election::confirm_once (nano::unique_lock & lock_a) +void nano::election::confirm_once (nano::unique_lock & lock) { - debug_assert (lock_a.owns_lock ()); + debug_assert (lock.owns_lock ()); + debug_assert (!mutex.try_lock ()); - // This must be kept above the setting of election state, as dependent confirmed elections require up to date changes to election_winner_details - nano::unique_lock election_winners_lk{ node.active.election_winner_details_mutex }; - auto just_confirmed = state_m != nano::election_state::confirmed; + bool just_confirmed = state_m != nano::election_state::confirmed; state_m = nano::election_state::confirmed; - if (just_confirmed && (node.active.election_winner_details.count (status.winner->hash ()) == 0)) + + if (just_confirmed) { - node.active.election_winner_details.emplace (status.winner->hash (), shared_from_this ()); - election_winners_lk.unlock (); status.election_end = std::chrono::duration_cast (std::chrono::system_clock::now ().time_since_epoch ()); status.election_duration = std::chrono::duration_cast (std::chrono::steady_clock::now () - election_start); status.confirmation_request_count = confirmation_request_count; @@ -62,11 +60,11 @@ void nano::election::confirm_once (nano::unique_lock & lock_a) nano::log::arg{ "qualified_root", qualified_root }, nano::log::arg{ "status", current_status_locked () }); - lock_a.unlock (); + node.confirming_set.add (status_l.winner->hash (), shared_from_this ()); - node.election_workers.push_task ([node_l = node.shared (), status_l, confirmation_action_l = confirmation_action] () { - node_l->process_confirmed (status_l); + lock.unlock (); + node.election_workers.push_task ([status_l, confirmation_action_l = confirmation_action] () { if (confirmation_action_l) { confirmation_action_l (status_l.winner); @@ -76,7 +74,7 @@ void nano::election::confirm_once (nano::unique_lock & lock_a) else { node.stats.inc (nano::stat::type::election, nano::stat::detail::confirm_once_failed); - lock_a.unlock (); + lock.unlock (); } } @@ -416,6 +414,7 @@ void nano::election::confirm_if_quorum (nano::unique_lock & lock_a) if (final_weight >= node.online_reps.delta ()) { confirm_once (lock_a); + debug_assert (!lock_a.owns_lock ()); } } } @@ -429,6 +428,7 @@ void nano::election::try_confirm (nano::block_hash const & hash) if (!confirmed_locked ()) { confirm_once (election_lock); + debug_assert (!election_lock.owns_lock ()); } } } diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 5871f2cff8..b84f7539dd 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -1172,7 +1172,6 @@ void nano::node::process_confirmed (nano::election_status const & status_a, uint stats.inc (nano::stat::type::process_confirmed, nano::stat::detail::timeout); // Do some cleanup due to this block never being processed by confirmation height processor - active.remove_election_winner_details (hash); } } diff --git a/nano/slow_test/node.cpp b/nano/slow_test/node.cpp index 4e4781d412..a51881b6d4 100644 --- a/nano/slow_test/node.cpp +++ b/nano/slow_test/node.cpp @@ -717,7 +717,6 @@ TEST (confirmation_height, many_accounts_single_confirmation) ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed_unbounded, nano::stat::dir::in), 0); ASSERT_TIMELY_EQ (40s, (node->ledger.cemented_count () - 1), node->stats.count (nano::stat::type::confirmation_observer, nano::stat::dir::out)); - ASSERT_TIMELY_EQ (10s, node->active.election_winner_details_size (), 0); } TEST (confirmation_height, many_accounts_many_confirmations) @@ -792,8 +791,6 @@ TEST (confirmation_height, many_accounts_many_confirmations) ASSERT_EQ (cemented_count, node->ledger.cemented_count ()); ASSERT_TIMELY_EQ (20s, (node->ledger.cemented_count () - 1), node->stats.count (nano::stat::type::confirmation_observer, nano::stat::dir::out)); - - ASSERT_TIMELY_EQ (10s, node->active.election_winner_details_size (), 0); } TEST (confirmation_height, long_chains) @@ -939,7 +936,6 @@ TEST (confirmation_height, long_chains) ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed_unbounded, nano::stat::dir::in), 0); ASSERT_TIMELY_EQ (40s, (node->ledger.cemented_count () - 1), node->stats.count (nano::stat::type::confirmation_observer, nano::stat::dir::out)); - ASSERT_TIMELY_EQ (10s, node->active.election_winner_details_size (), 0); } TEST (confirmation_height, dynamic_algorithm) @@ -987,7 +983,6 @@ TEST (confirmation_height, dynamic_algorithm) ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in), num_blocks); ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed_bounded, nano::stat::dir::in), 1); ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed_unbounded, nano::stat::dir::in), num_blocks - 1); - ASSERT_TIMELY_EQ (10s, node->active.election_winner_details_size (), 0); } TEST (confirmation_height, many_accounts_send_receive_self) @@ -1118,10 +1113,6 @@ TEST (confirmation_height, many_accounts_send_receive_self) } system.deadline_set (60s); - while (node->active.election_winner_details_size () > 0) - { - ASSERT_NO_ERROR (system.poll ()); - } } // Same as the many_accounts_send_receive_self test, except works on the confirmation height processor directly From b9f02254d089cff922fb7f98c8bdb350909065ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Wed, 16 Oct 2024 13:01:00 +0200 Subject: [PATCH 08/11] Fix test --- nano/core_test/active_elections.cpp | 18 +++++------------- nano/node/active_elections.cpp | 8 ++------ nano/node/confirming_set.cpp | 7 ++++++- nano/node/confirming_set.hpp | 5 +++-- 4 files changed, 16 insertions(+), 22 deletions(-) diff --git a/nano/core_test/active_elections.cpp b/nano/core_test/active_elections.cpp index 05c243aa3e..9bd1d6c204 100644 --- a/nano/core_test/active_elections.cpp +++ b/nano/core_test/active_elections.cpp @@ -1398,6 +1398,8 @@ TEST (active_elections, bound_election_winners) nano::node_config config = system.default_config (); // Set election winner limit to a low value config.active_elections.max_election_winners = 5; + // Large batch size would complicate this testcase + config.confirming_set.batch_size = 1; auto & node = *system.add_node (config); // Start elections for a couple of blocks, number of elections is larger than the election winner set limit @@ -1411,22 +1413,12 @@ TEST (active_elections, bound_election_winners) auto guard = node.ledger.tx_begin_write (nano::store::writer::testing); // Ensure that when the number of election winners reaches the limit, AEC vacancy reflects that + // Confirming more elections should make the vacancy negative ASSERT_TRUE (node.active.vacancy (nano::election_behavior::priority) > 0); - int index = 0; - for (; index < config.active_elections.max_election_winners; ++index) - { - auto election = node.vote_router.election (blocks[index]->hash ()); - ASSERT_TRUE (election); - election->force_confirm (); - } - - ASSERT_TIMELY_EQ (5s, node.active.vacancy (nano::election_behavior::priority), 0); - - // Confirming more elections should make the vacancy negative - for (; index < blocks.size (); ++index) + for (auto const & block : blocks) { - auto election = node.vote_router.election (blocks[index]->hash ()); + auto election = node.vote_router.election (block->hash ()); ASSERT_TRUE (election); election->force_confirm (); } diff --git a/nano/node/active_elections.cpp b/nano/node/active_elections.cpp index feafab86bf..0cc4953f0e 100644 --- a/nano/node/active_elections.cpp +++ b/nano/node/active_elections.cpp @@ -40,13 +40,9 @@ nano::active_elections::active_elections (nano::node & node_a, nano::confirming_ // Notify elections about alternative (forked) blocks block_processor.block_processed.add ([this] (auto const & result, auto const & context) { - switch (result) + if (result == nano::block_status::fork) { - case nano::block_status::fork: - publish (context.block); - break; - default: - break; + publish (context.block); } }); } diff --git a/nano/node/confirming_set.cpp b/nano/node/confirming_set.cpp index de92e853a9..d669153bbb 100644 --- a/nano/node/confirming_set.cpp +++ b/nano/node/confirming_set.cpp @@ -48,6 +48,11 @@ void nano::confirming_set::start () { debug_assert (!thread.joinable ()); + if (!config.enable) + { + return; + } + thread = std::thread{ [this] () { nano::thread_role::set (nano::thread_role::name::confirmation_height); run (); @@ -123,7 +128,7 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) std::deque cemented; std::deque already; - auto batch = next_batch (batch_size); + auto batch = next_batch (config.batch_size); lock.unlock (); diff --git a/nano/node/confirming_set.hpp b/nano/node/confirming_set.hpp index 00329fea83..7085b5baa3 100644 --- a/nano/node/confirming_set.hpp +++ b/nano/node/confirming_set.hpp @@ -28,6 +28,9 @@ class confirming_set_config final // TODO: Serialization & deserialization public: + bool enable{ true }; + size_t batch_size{ 256 }; + /** Maximum number of dependent blocks to be stored in memory during processing */ size_t max_blocks{ 128 * 1024 }; size_t max_queued_notifications{ 8 }; @@ -106,7 +109,5 @@ class confirming_set final mutable std::mutex mutex; std::condition_variable condition; std::thread thread; - - static size_t constexpr batch_size = 256; }; } From 6542a7851f57edcbec2925d0fe812fb404f437ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Thu, 17 Oct 2024 17:30:05 +0200 Subject: [PATCH 09/11] Fix confirming set contains check --- nano/node/confirming_set.cpp | 13 ++++++++++++- nano/node/confirming_set.hpp | 1 + 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/nano/node/confirming_set.cpp b/nano/node/confirming_set.cpp index d669153bbb..40216e447f 100644 --- a/nano/node/confirming_set.cpp +++ b/nano/node/confirming_set.cpp @@ -76,7 +76,7 @@ void nano::confirming_set::stop () bool nano::confirming_set::contains (nano::block_hash const & hash) const { std::lock_guard lock{ mutex }; - return set.get ().contains (hash); + return set.get ().contains (hash) || current.contains (hash); } std::size_t nano::confirming_set::size () const @@ -130,6 +130,13 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) auto batch = next_batch (config.batch_size); + // Keep track of the blocks we're currently cementing, so that the .contains (...) check is accurate + debug_assert (current.empty ()); + for (auto const & [hash, election] : batch) + { + current.insert (hash); + } + lock.unlock (); auto notify = [this, &cemented] () { @@ -218,6 +225,10 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) release_assert (cemented.empty ()); already_cemented.notify (already); + + lock.lock (); + current.clear (); + lock.unlock (); } nano::container_info nano::confirming_set::container_info () const diff --git a/nano/node/confirming_set.hpp b/nano/node/confirming_set.hpp index 7085b5baa3..71214e56dd 100644 --- a/nano/node/confirming_set.hpp +++ b/nano/node/confirming_set.hpp @@ -102,6 +102,7 @@ class confirming_set final // clang-format on ordered_entries set; + std::unordered_set current; nano::thread_pool notification_workers; From 46696f7646a3708c7a607f3850d1d99719b84da8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Thu, 17 Oct 2024 23:19:17 +0200 Subject: [PATCH 10/11] Debug log cemented blocks --- nano/core_test/confirming_set.cpp | 8 ++++---- nano/lib/logging_enums.hpp | 1 + nano/lib/stats_enums.hpp | 1 + nano/node/confirming_set.cpp | 22 +++++++++++++++++++--- nano/node/confirming_set.hpp | 3 ++- nano/node/node.cpp | 2 +- nano/test_common/ledger_context.cpp | 9 +++++++-- nano/test_common/ledger_context.hpp | 5 +++-- 8 files changed, 38 insertions(+), 13 deletions(-) diff --git a/nano/core_test/confirming_set.cpp b/nano/core_test/confirming_set.cpp index a2b3d60803..ee7302eba6 100644 --- a/nano/core_test/confirming_set.cpp +++ b/nano/core_test/confirming_set.cpp @@ -20,14 +20,14 @@ TEST (confirming_set, construction) { auto ctx = nano::test::ledger_empty (); nano::confirming_set_config config{}; - nano::confirming_set confirming_set{ config, ctx.ledger (), ctx.stats () }; + nano::confirming_set confirming_set{ config, ctx.ledger (), ctx.stats (), ctx.logger () }; } TEST (confirming_set, add_exists) { auto ctx = nano::test::ledger_send_receive (); nano::confirming_set_config config{}; - nano::confirming_set confirming_set{ config, ctx.ledger (), ctx.stats () }; + nano::confirming_set confirming_set{ config, ctx.ledger (), ctx.stats (), ctx.logger () }; auto send = ctx.blocks ()[0]; confirming_set.add (send->hash ()); ASSERT_TRUE (confirming_set.contains (send->hash ())); @@ -37,7 +37,7 @@ TEST (confirming_set, process_one) { auto ctx = nano::test::ledger_send_receive (); nano::confirming_set_config config{}; - nano::confirming_set confirming_set{ config, ctx.ledger (), ctx.stats () }; + nano::confirming_set confirming_set{ config, ctx.ledger (), ctx.stats (), ctx.logger () }; std::atomic count = 0; std::mutex mutex; std::condition_variable condition; @@ -54,7 +54,7 @@ TEST (confirming_set, process_multiple) { auto ctx = nano::test::ledger_send_receive (); nano::confirming_set_config config{}; - nano::confirming_set confirming_set{ config, ctx.ledger (), ctx.stats () }; + nano::confirming_set confirming_set{ config, ctx.ledger (), ctx.stats (), ctx.logger () }; std::atomic count = 0; std::mutex mutex; std::condition_variable condition; diff --git a/nano/lib/logging_enums.hpp b/nano/lib/logging_enums.hpp index 922c62baa1..85136b98b7 100644 --- a/nano/lib/logging_enums.hpp +++ b/nano/lib/logging_enums.hpp @@ -82,6 +82,7 @@ enum class type message_processor, local_block_broadcaster, monitor, + confirming_set, // bootstrap bulk_pull_client, diff --git a/nano/lib/stats_enums.hpp b/nano/lib/stats_enums.hpp index ea8c07780f..1e3889d180 100644 --- a/nano/lib/stats_enums.hpp +++ b/nano/lib/stats_enums.hpp @@ -505,6 +505,7 @@ enum class detail already_cemented, cementing, cemented_hash, + cementing_failed, // election_state passive, diff --git a/nano/node/confirming_set.cpp b/nano/node/confirming_set.cpp index 40216e447f..4d8d145084 100644 --- a/nano/node/confirming_set.cpp +++ b/nano/node/confirming_set.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -6,10 +7,11 @@ #include #include -nano::confirming_set::confirming_set (confirming_set_config const & config_a, nano::ledger & ledger_a, nano::stats & stats_a) : +nano::confirming_set::confirming_set (confirming_set_config const & config_a, nano::ledger & ledger_a, nano::stats & stats_a, nano::logger & logger_a) : config{ config_a }, ledger{ ledger_a }, stats{ stats_a }, + logger{ logger_a }, notification_workers{ 1, nano::thread_role::name::confirmation_height_notifications } { batch_cemented.add ([this] (auto const & cemented) { @@ -177,6 +179,8 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) auto transaction = ledger.tx_begin_write (nano::store::writer::confirmation_height); for (auto const & [hash, election] : batch) { + size_t cemented_count = 0; + bool success = false; do { transaction.refresh_if_needed (); @@ -208,6 +212,7 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) { cemented.push_back ({ block, hash, election }); } + cemented_count += added.size (); } else { @@ -215,9 +220,20 @@ void nano::confirming_set::run_batch (std::unique_lock & lock) already.push_back (hash); debug_assert (ledger.confirmed.block_exists (transaction, hash)); } - } while (!ledger.confirmed.block_exists (transaction, hash)); - stats.inc (nano::stat::type::confirming_set, nano::stat::detail::cemented_hash); + success = ledger.confirmed.block_exists (transaction, hash); + } while (!success); + + if (success) + { + stats.inc (nano::stat::type::confirming_set, nano::stat::detail::cemented_hash); + logger.debug (nano::log::type::confirming_set, "Cemented block: {} (total cemented: {})", hash.to_string (), cemented_count); + } + else + { + stats.inc (nano::stat::type::confirming_set, nano::stat::detail::cementing_failed); + logger.debug (nano::log::type::confirming_set, "Failed to cement block: {}", hash.to_string ()); + } } } diff --git a/nano/node/confirming_set.hpp b/nano/node/confirming_set.hpp index 71214e56dd..99569ce1e6 100644 --- a/nano/node/confirming_set.hpp +++ b/nano/node/confirming_set.hpp @@ -45,7 +45,7 @@ class confirming_set final friend class confirmation_height_pruned_source_Test; public: - confirming_set (confirming_set_config const &, nano::ledger &, nano::stats &); + confirming_set (confirming_set_config const &, nano::ledger &, nano::stats &, nano::logger &); ~confirming_set (); void start (); @@ -76,6 +76,7 @@ class confirming_set final confirming_set_config const & config; nano::ledger & ledger; nano::stats & stats; + nano::logger & logger; private: struct entry diff --git a/nano/node/node.cpp b/nano/node/node.cpp index b84f7539dd..da9b9c5dfd 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -115,7 +115,7 @@ nano::node::node (std::shared_ptr io_ctx_a, std::filesy port_mapping_impl{ std::make_unique (*this) }, port_mapping{ *port_mapping_impl }, block_processor (*this), - confirming_set_impl{ std::make_unique (config.confirming_set, ledger, stats) }, + confirming_set_impl{ std::make_unique (config.confirming_set, ledger, stats, logger) }, confirming_set{ *confirming_set_impl }, active_impl{ std::make_unique (*this, confirming_set, block_processor) }, active{ *active_impl }, diff --git a/nano/test_common/ledger_context.cpp b/nano/test_common/ledger_context.cpp index 578aaa6342..e9a6b33c49 100644 --- a/nano/test_common/ledger_context.cpp +++ b/nano/test_common/ledger_context.cpp @@ -4,8 +4,8 @@ #include nano::test::ledger_context::ledger_context (std::deque> && blocks) : - store_m{ nano::make_store (logger, nano::unique_path (), nano::dev::constants) }, - stats_m{ logger }, + store_m{ nano::make_store (logger_m, nano::unique_path (), nano::dev::constants) }, + stats_m{ logger_m }, ledger_m{ *store_m, stats_m, nano::dev::constants }, blocks_m{ blocks }, pool_m{ nano::dev::network_params.network, 1 } @@ -35,6 +35,11 @@ nano::stats & nano::test::ledger_context::stats () return stats_m; } +nano::logger & nano::test::ledger_context::logger () +{ + return logger_m; +} + std::deque> const & nano::test::ledger_context::blocks () const { return blocks_m; diff --git a/nano/test_common/ledger_context.hpp b/nano/test_common/ledger_context.hpp index 8edca08508..62b376a4fd 100644 --- a/nano/test_common/ledger_context.hpp +++ b/nano/test_common/ledger_context.hpp @@ -16,12 +16,13 @@ class ledger_context ledger_context (std::deque> && blocks = std::deque>{}); nano::ledger & ledger (); nano::store::component & store (); - nano::stats & stats (); std::deque> const & blocks () const; nano::work_pool & pool (); + nano::stats & stats (); + nano::logger & logger (); private: - nano::logger logger; + nano::logger logger_m; std::unique_ptr store_m; nano::stats stats_m; nano::ledger ledger_m; From 4b9a64211f48d6c44637490c341de240cdde9a68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Fri, 18 Oct 2024 00:09:37 +0200 Subject: [PATCH 11/11] Use `node::process_confirmed ()` --- nano/core_test/confirming_set.cpp | 3 +-- nano/core_test/election_scheduler.cpp | 4 ++-- nano/core_test/node.cpp | 12 ++++++------ nano/node/election.cpp | 8 +++++--- nano/node/node.cpp | 27 +++++++++++++-------------- nano/node/node.hpp | 2 +- 6 files changed, 28 insertions(+), 28 deletions(-) diff --git a/nano/core_test/confirming_set.cpp b/nano/core_test/confirming_set.cpp index ee7302eba6..596f65444c 100644 --- a/nano/core_test/confirming_set.cpp +++ b/nano/core_test/confirming_set.cpp @@ -170,8 +170,7 @@ TEST (confirmation_callback, confirmed_history) ASSERT_TIMELY (10s, !node->store.write_queue.contains (nano::store::writer::confirmation_height)); - auto transaction = node->ledger.tx_begin_read (); - ASSERT_TRUE (node->ledger.confirmed.block_exists (transaction, send->hash ())); + ASSERT_TIMELY (5s, node->ledger.confirmed.block_exists (node->ledger.tx_begin_read (), send->hash ())); ASSERT_TIMELY_EQ (10s, node->active.size (), 0); ASSERT_TIMELY_EQ (10s, node->stats.count (nano::stat::type::confirmation_observer, nano::stat::detail::active_quorum, nano::stat::dir::out), 1); diff --git a/nano/core_test/election_scheduler.cpp b/nano/core_test/election_scheduler.cpp index 56c1a5c7b8..14f840d4fa 100644 --- a/nano/core_test/election_scheduler.cpp +++ b/nano/core_test/election_scheduler.cpp @@ -195,7 +195,7 @@ TEST (election_scheduler, no_vacancy) .work (*system.work.generate (nano::dev::genesis->hash ())) .build (); ASSERT_EQ (nano::block_status::progress, node.process (send)); - node.process_confirmed (nano::election_status{ send }); + node.process_confirmed (send->hash ()); auto receive = builder.make_block () .account (key.pub) @@ -207,7 +207,7 @@ TEST (election_scheduler, no_vacancy) .work (*system.work.generate (key.pub)) .build (); ASSERT_EQ (nano::block_status::progress, node.process (receive)); - node.process_confirmed (nano::election_status{ receive }); + node.process_confirmed (receive->hash ()); ASSERT_TIMELY (5s, nano::test::confirmed (node, { send, receive })); diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 54d1ef29f6..218ceb23d8 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -3564,9 +3564,9 @@ TEST (node, pruning_automatic) ASSERT_TIMELY (5s, node1.block (send2->hash ()) != nullptr); // Force-confirm both blocks - node1.process_confirmed (nano::election_status{ send1 }); + node1.process_confirmed (send1->hash ()); ASSERT_TIMELY (5s, node1.block_confirmed (send1->hash ())); - node1.process_confirmed (nano::election_status{ send2 }); + node1.process_confirmed (send2->hash ()); ASSERT_TIMELY (5s, node1.block_confirmed (send2->hash ())); // Check pruning result @@ -3615,9 +3615,9 @@ TEST (node, DISABLED_pruning_age) node1.process_active (send2); // Force-confirm both blocks - node1.process_confirmed (nano::election_status{ send1 }); + node1.process_confirmed (send1->hash ()); ASSERT_TIMELY (5s, node1.block_confirmed (send1->hash ())); - node1.process_confirmed (nano::election_status{ send2 }); + node1.process_confirmed (send2->hash ()); ASSERT_TIMELY (5s, node1.block_confirmed (send2->hash ())); // Three blocks in total, nothing pruned yet @@ -3676,9 +3676,9 @@ TEST (node, DISABLED_pruning_depth) node1.process_active (send2); // Force-confirm both blocks - node1.process_confirmed (nano::election_status{ send1 }); + node1.process_confirmed (send1->hash ()); ASSERT_TIMELY (5s, node1.block_confirmed (send1->hash ())); - node1.process_confirmed (nano::election_status{ send2 }); + node1.process_confirmed (send2->hash ()); ASSERT_TIMELY (5s, node1.block_confirmed (send2->hash ())); // Three blocks in total, nothing pruned yet diff --git a/nano/node/election.cpp b/nano/node/election.cpp index a8a392dbe5..93b9e8f7dc 100644 --- a/nano/node/election.cpp +++ b/nano/node/election.cpp @@ -60,11 +60,13 @@ void nano::election::confirm_once (nano::unique_lock & lock) nano::log::arg{ "qualified_root", qualified_root }, nano::log::arg{ "status", current_status_locked () }); - node.confirming_set.add (status_l.winner->hash (), shared_from_this ()); - lock.unlock (); - node.election_workers.push_task ([status_l, confirmation_action_l = confirmation_action] () { + node.election_workers.push_task ([this_l = shared_from_this (), status_l, confirmation_action_l = confirmation_action] () { + // This is necessary if the winner of the election is one of the forks. + // In that case the winning block is not yet in the ledger and cementing needs to wait for rollbacks to complete. + this_l->node.process_confirmed (status_l.winner->hash (), this_l); + if (confirmation_action_l) { confirmation_action_l (status_l.winner); diff --git a/nano/node/node.cpp b/nano/node/node.cpp index da9b9c5dfd..c6f550c98f 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -1141,30 +1141,28 @@ void nano::node::ongoing_online_weight_calculation () ongoing_online_weight_calculation_queue (); } -void nano::node::process_confirmed (nano::election_status const & status_a, uint64_t iteration_a) +// TODO: Replace this with a queue of some sort. Blocks submitted here could be in a limbo for a while: neither part of an active election nor cemented +void nano::node::process_confirmed (nano::block_hash hash, std::shared_ptr election, uint64_t iteration) { stats.inc (nano::stat::type::process_confirmed, nano::stat::detail::initiate); - auto hash (status_a.winner->hash ()); - decltype (iteration_a) const num_iters = (config.block_processor_batch_max_time / network_params.node.process_confirmed_interval) * 4; - if (auto block_l = ledger.any.block_get (ledger.tx_begin_read (), hash)) + // Limit the maximum number of iterations to avoid getting stuck + uint64_t const max_iterations = (config.block_processor_batch_max_time / network_params.node.process_confirmed_interval) * 4; + + if (auto block = ledger.any.block_get (ledger.tx_begin_read (), hash)) { stats.inc (nano::stat::type::process_confirmed, nano::stat::detail::done); - logger.trace (nano::log::type::node, nano::log::detail::process_confirmed, nano::log::arg{ "block", block_l }); + logger.trace (nano::log::type::node, nano::log::detail::process_confirmed, nano::log::arg{ "block", block }); - confirming_set.add (block_l->hash ()); + confirming_set.add (block->hash (), election); } - else if (iteration_a < num_iters) + else if (iteration < max_iterations) { stats.inc (nano::stat::type::process_confirmed, nano::stat::detail::retry); - iteration_a++; - std::weak_ptr node_w (shared ()); - election_workers.add_timed_task (std::chrono::steady_clock::now () + network_params.node.process_confirmed_interval, [node_w, status_a, iteration_a] () { - if (auto node_l = node_w.lock ()) - { - node_l->process_confirmed (status_a, iteration_a); - } + // Try again later + election_workers.add_timed_task (std::chrono::steady_clock::now () + network_params.node.process_confirmed_interval, [this, hash, election, iteration] () { + process_confirmed (hash, election, iteration + 1); }); } else @@ -1172,6 +1170,7 @@ void nano::node::process_confirmed (nano::election_status const & status_a, uint stats.inc (nano::stat::type::process_confirmed, nano::stat::detail::timeout); // Do some cleanup due to this block never being processed by confirmation height processor + active.recently_confirmed.erase (hash); } } diff --git a/nano/node/node.hpp b/nano/node/node.hpp index c411b6d9a5..9a79d5bdd0 100644 --- a/nano/node/node.hpp +++ b/nano/node/node.hpp @@ -94,7 +94,7 @@ class node final : public std::enable_shared_from_this bool copy_with_compaction (std::filesystem::path const &); void keepalive (std::string const &, uint16_t); int store_version (); - void process_confirmed (nano::election_status const &, uint64_t = 0); + void process_confirmed (nano::block_hash, std::shared_ptr = nullptr, uint64_t iteration = 0); void process_active (std::shared_ptr const &); std::optional process_local (std::shared_ptr const &); void process_local_async (std::shared_ptr const &);