From 82d9d2d323abf2821032b7d88d21b3369957b814 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 14:17:04 +0200 Subject: [PATCH] Process cemented elections with exclusive lock to avoid races --- nano/node/active_elections.cpp | 42 +++++++++++++++++++++++++--------- nano/node/active_elections.hpp | 12 ++++++---- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/nano/node/active_elections.cpp b/nano/node/active_elections.cpp index 6536b2c4..4d8c953f 100644 --- a/nano/node/active_elections.cpp +++ b/nano/node/active_elections.cpp @@ -31,11 +31,24 @@ nano::active_elections::active_elections (nano::node & node_a, nano::confirming_ // Cementing blocks might implicitly confirm dependent elections confirming_set.batch_cemented.add ([this] (auto const & cemented) { - auto transaction = node.ledger.tx_begin_read (); - for (auto const & [block, confirmation_root, source_election] : cemented) + std::deque results; { - transaction.refresh_if_needed (); - block_cemented (transaction, block, confirmation_root, source_election); + // Process all cemented blocks while holding the lock to avoid races where an election for a block that is already cemented is inserted + nano::lock_guard guard{ mutex }; + for (auto const & [block, confirmation_root, source_election] : cemented) + { + auto result = block_cemented (block, confirmation_root, source_election); + results.push_back (result); + } + } + { + // TODO: This could be offloaded to a separate notification worker, profiling is needed + auto transaction = node.ledger.tx_begin_read (); + for (auto const & [status, votes] : results) + { + transaction.refresh_if_needed (); + notify_observers (transaction, status, votes); + } } }); @@ -83,16 +96,17 @@ void nano::active_elections::stop () clear (); } -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) +auto nano::active_elections::block_cemented (std::shared_ptr const & block, nano::block_hash const & confirmation_root, std::shared_ptr const & source_election) -> block_cemented_result { + debug_assert (!mutex.try_lock ()); debug_assert (node.block_confirmed (block->hash ())); // Dependent elections are implicitly confirmed when their block is cemented - auto dependend_election = election (block->qualified_root ()); + auto dependend_election = election_impl (block->qualified_root ()); if (dependend_election) { node.stats.inc (nano::stat::type::active_elections, nano::stat::detail::confirm_dependent); - dependend_election->try_confirm (block->hash ()); + dependend_election->try_confirm (block->hash ()); // TODO: This should either confirm or cancel the election } nano::election_status status; @@ -126,7 +140,7 @@ void nano::active_elections::block_cemented (nano::secure::transaction const & t nano::log::arg{ "confirmation_root", confirmation_root }, nano::log::arg{ "source_election", source_election }); - notify_observers (transaction, status, votes); + return { status, votes }; } void nano::active_elections::notify_observers (nano::secure::transaction const & transaction, nano::election_status const & status, std::vector const & votes) const @@ -443,11 +457,17 @@ bool nano::active_elections::active (nano::block const & block_a) const return roots.get ().find (block_a.qualified_root ()) != roots.get ().end (); } -std::shared_ptr nano::active_elections::election (nano::qualified_root const & root_a) const +std::shared_ptr nano::active_elections::election (nano::qualified_root const & root) const { - std::shared_ptr result; nano::lock_guard lock{ mutex }; - auto existing = roots.get ().find (root_a); + return election_impl (root); +} + +std::shared_ptr nano::active_elections::election_impl (nano::qualified_root const & root) const +{ + debug_assert (!mutex.try_lock ()); + std::shared_ptr result; + auto existing = roots.get ().find (root); if (existing != roots.get ().end ()) { result = existing->election; diff --git a/nano/node/active_elections.hpp b/nano/node/active_elections.hpp index 049f4142..8b831112 100644 --- a/nano/node/active_elections.hpp +++ b/nano/node/active_elections.hpp @@ -105,7 +105,7 @@ public: bool active (nano::qualified_root const &) const; std::shared_ptr election (nano::qualified_root const &) const; // Returns a list of elections sorted by difficulty - std::vector> list_active (std::size_t = std::numeric_limits::max ()); + std::vector> list_active (std::size_t max_count = std::numeric_limits::max ()); bool erase (nano::block const &); bool erase (nano::qualified_root const &); bool empty () const; @@ -133,11 +133,13 @@ private: void request_confirm (nano::unique_lock &); // Erase all blocks from active and, if not confirmed, clear digests from network filters void cleanup_election (nano::unique_lock & lock_a, std::shared_ptr); - nano::stat::type completion_type (nano::election const & election) const; - // Returns a list of elections sorted by difficulty, mutex must be locked - std::vector> list_active_impl (std::size_t) const; + + using block_cemented_result = std::pair>; + block_cemented_result block_cemented (std::shared_ptr const & block, nano::block_hash const & confirmation_root, std::shared_ptr const & source_election); void notify_observers (nano::secure::transaction const &, nano::election_status const & status, std::vector const & votes) const; - void block_cemented (nano::secure::transaction const &, std::shared_ptr const & block, nano::block_hash const & confirmation_root, std::shared_ptr const & source_election); + + std::shared_ptr election_impl (nano::qualified_root const &) const; + std::vector> list_active_impl (std::size_t max_count) const; private: // Dependencies active_elections_config const & config;