diff --git a/nano/core_test/election.cpp b/nano/core_test/election.cpp index 4c9962e9..3879e510 100644 --- a/nano/core_test/election.cpp +++ b/nano/core_test/election.cpp @@ -11,11 +11,7 @@ TEST (election, construction) auto & node = *system.nodes[0]; genesis.open->sideband_set (nano::block_sideband (nano::genesis_account, 0, nano::genesis_amount, 1, nano::seconds_since_epoch (), nano::epoch::epoch_0, false, false, false, nano::epoch::epoch_0)); auto election = node.active.insert (genesis.open).election; - ASSERT_TRUE (election->idle ()); election->transition_active (); - ASSERT_FALSE (election->idle ()); - election->transition_passive (); - ASSERT_FALSE (election->idle ()); } namespace nano diff --git a/nano/core_test/request_aggregator.cpp b/nano/core_test/request_aggregator.cpp index dc1b39f7..e5ea6480 100644 --- a/nano/core_test/request_aggregator.cpp +++ b/nano/core_test/request_aggregator.cpp @@ -299,12 +299,16 @@ TEST (request_aggregator, unique) ASSERT_TIMELY (3s, 1 == node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes)); } +namespace nano +{ TEST (request_aggregator, cannot_vote) { nano::system system; nano::node_flags flags; flags.disable_request_loop = true; auto & node (*system.add_node (flags)); + // This prevents activation of blocks which are cemented + node.confirmation_height_processor.cemented_observers.clear (); nano::genesis genesis; nano::state_block_builder builder; auto send1 = builder.make_block () @@ -378,3 +382,4 @@ TEST (request_aggregator, cannot_vote) ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown)); ASSERT_TIMELY (3s, 1 == node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); } +} diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index 22f2b2b0..080193a5 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -95,12 +95,17 @@ void nano::active_transactions::confirm_prioritized_frontiers (nano::transaction { auto block (this->node.store.block_get (transaction_a, info.head)); auto previous_balance (this->node.ledger.balance (transaction_a, block->previous ())); - auto insert_result = this->insert (block, previous_balance); - if (insert_result.inserted) + lk.lock (); + if (roots.get ().find (block->qualified_root ()) == roots.get ().end ()) { - insert_result.election->transition_active (); - ++elections_count; + auto insert_result = this->insert_impl (block, previous_balance); + if (insert_result.inserted) + { + insert_result.election->transition_active (); + ++elections_count; + } } + lk.unlock (); } } } @@ -191,7 +196,7 @@ void nano::active_transactions::block_cemented_callback (std::shared_ptrelection; } + + // Votes are generated for inserted or ongoing elections if they're prioritized + // Non-priority elections generate votes when they gain priority in the future + if (result.election && result.election->prioritized ()) + { + result.election->generate_votes (); + } } return result; } @@ -791,17 +803,9 @@ nano::election_insertion_result nano::active_transactions::activate (nano::accou if (node.ledger.can_vote (transaction, *block)) { result = insert (block); - if (result.election) + if (result.inserted) { - if (result.inserted) - { - result.election->transition_active (); - } - else if (result.election->prioritized ()) - { - // Generate vote for ongoing election - result.election->generate_votes (block->hash ()); - } + result.election->transition_active (); } } } diff --git a/nano/node/blockprocessor.cpp b/nano/node/blockprocessor.cpp index fca06ac9..4fc073f2 100644 --- a/nano/node/blockprocessor.cpp +++ b/nano/node/blockprocessor.cpp @@ -297,15 +297,7 @@ void nano::block_processor::process_live (nano::block_hash const & hash_a, std:: // Start collecting quorum on block if (watch_work_a || node.ledger.can_vote (node.store.tx_begin_read (), *block_a)) { - auto election = node.active.insert (block_a, process_return_a.previous_balance.number ()); - if (election.inserted) - { - election.election->transition_passive (); - } - else if (election.election) - { - election.election->try_generate_votes (block_a->hash ()); - } + node.active.insert (block_a, process_return_a.previous_balance.number ()); } // Announce block contents to the network diff --git a/nano/node/confirmation_height_processor.hpp b/nano/node/confirmation_height_processor.hpp index 0146cc32..8761bf44 100644 --- a/nano/node/confirmation_height_processor.hpp +++ b/nano/node/confirmation_height_processor.hpp @@ -93,6 +93,7 @@ private: friend class confirmation_height_many_accounts_many_confirmations_Test; friend class confirmation_height_long_chains_Test; friend class confirmation_height_many_accounts_single_confirmation_Test; + friend class request_aggregator_cannot_vote_Test; }; std::unique_ptr collect_container_info (confirmation_height_processor &, const std::string &); diff --git a/nano/node/election.cpp b/nano/node/election.cpp index 47616905..ef175307 100644 --- a/nano/node/election.cpp +++ b/nano/node/election.cpp @@ -34,10 +34,6 @@ root (block_a->root ()) last_votes.emplace (node.network_params.random.not_an_account, nano::vote_info{ std::chrono::steady_clock::now (), 0, block_a->hash () }); blocks.emplace (block_a->hash (), block_a); update_dependent (); - if (prioritized_a) - { - generate_votes (block_a->hash ()); - } } void nano::election::confirm_once (nano::election_status_type type_a) @@ -71,21 +67,9 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele bool result = false; switch (expected_a) { - case nano::election::state_t::idle: - switch (desired_a) - { - case nano::election::state_t::passive: - case nano::election::state_t::active: - result = true; - break; - default: - break; - } - break; case nano::election::state_t::passive: switch (desired_a) { - case nano::election::state_t::idle: case nano::election::state_t::active: case nano::election::state_t::confirmed: case nano::election::state_t::expired_unconfirmed: @@ -98,7 +82,6 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele case nano::election::state_t::active: switch (desired_a) { - case nano::election::state_t::idle: case nano::election::state_t::broadcasting: case nano::election::state_t::confirmed: case nano::election::state_t::expired_unconfirmed: @@ -107,10 +90,10 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele default: break; } + break; case nano::election::state_t::broadcasting: switch (desired_a) { - case nano::election::state_t::idle: case nano::election::state_t::backtracking: case nano::election::state_t::confirmed: case nano::election::state_t::expired_unconfirmed: @@ -119,10 +102,10 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele default: break; } + break; case nano::election::state_t::backtracking: switch (desired_a) { - case nano::election::state_t::idle: case nano::election::state_t::confirmed: case nano::election::state_t::expired_unconfirmed: result = true; @@ -130,6 +113,7 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele default: break; } + break; case nano::election::state_t::confirmed: switch (desired_a) { @@ -139,8 +123,8 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele default: break; } - case nano::election::state_t::expired_unconfirmed: break; + case nano::election::state_t::expired_unconfirmed: case nano::election::state_t::expired_confirmed: break; } @@ -178,17 +162,6 @@ void nano::election::send_confirm_req (nano::confirmation_solicitor & solicitor_ } } -void nano::election::transition_passive () -{ - nano::lock_guard guard (timepoints_mutex); - transition_passive_impl (); -} - -void nano::election::transition_passive_impl () -{ - state_change (nano::election::state_t::idle, nano::election::state_t::passive); -} - void nano::election::transition_active () { nano::lock_guard guard (timepoints_mutex); @@ -197,12 +170,7 @@ void nano::election::transition_active () void nano::election::transition_active_impl () { - state_change (nano::election::state_t::idle, nano::election::state_t::active); -} - -bool nano::election::idle () const -{ - return state_m == nano::election::state_t::idle; + state_change (nano::election::state_t::passive, nano::election::state_t::active); } bool nano::election::confirmed () const @@ -234,16 +202,12 @@ bool nano::election::transition_time (nano::confirmation_solicitor & solicitor_a bool result = false; switch (state_m) { - case nano::election::state_t::idle: - break; case nano::election::state_t::passive: - { if (base_latency () * passive_duration_factor < std::chrono::steady_clock::now () - state_start) { state_change (nano::election::state_t::passive, nano::election::state_t::active); } break; - } case nano::election::state_t::active: send_confirm_req (solicitor_a); if (confirmation_request_count > active_request_count_min) @@ -579,21 +543,12 @@ void nano::election::prioritize_election (nano::vote_generator_session & generat generator_session_a.add (root, status.winner->hash ()); } -void nano::election::try_generate_votes (nano::block_hash const & hash_a) -{ - nano::unique_lock lock (node.active.mutex); - if (status.winner->hash () == hash_a) - { - lock.unlock (); - generate_votes (hash_a); - } -} - -void nano::election::generate_votes (nano::block_hash const & hash_a) +void nano::election::generate_votes () { + debug_assert (!node.active.mutex.try_lock ()); if (node.config.enable_voting && node.wallets.reps ().voting > 0) { - node.active.generator.add (root, hash_a); + node.active.generator.add (root, status.winner->hash ()); } } diff --git a/nano/node/election.hpp b/nano/node/election.hpp index 52b13220..76824c86 100644 --- a/nano/node/election.hpp +++ b/nano/node/election.hpp @@ -39,7 +39,6 @@ class election final : public std::enable_shared_from_this private: // State management enum class state_t { - idle, passive, // only listening for incoming votes active, // actively request confirmations broadcasting, // request confirmations and broadcast the winner @@ -52,7 +51,7 @@ private: // State management static int constexpr active_request_count_min = 2; static int constexpr active_broadcasting_duration_factor = 30; static int constexpr confirmed_duration_factor = 5; - std::atomic state_m = { state_t::idle }; + std::atomic state_m = { state_t::passive }; // These time points must be protected by this mutex std::mutex timepoints_mutex; @@ -66,7 +65,7 @@ private: // State management void send_confirm_req (nano::confirmation_solicitor &); void activate_dependencies (); // Calculate votes for local representatives - void generate_votes (nano::block_hash const &); + void generate_votes (); void remove_votes (nano::block_hash const &); std::atomic prioritized_m = { false }; @@ -87,22 +86,17 @@ public: size_t insert_inactive_votes_cache (nano::block_hash const &); bool prioritized () const; void prioritize_election (nano::vote_generator_session &); - // Calculate votes if the current winner matches \p hash_a - void try_generate_votes (nano::block_hash const & hash_a); // Erase all blocks from active and, if not confirmed, clear digests from network filters void cleanup (); public: // State transitions bool transition_time (nano::confirmation_solicitor &); - void transition_passive (); void transition_active (); private: - void transition_passive_impl (); void transition_active_impl (); public: - bool idle () const; bool confirmed () const; nano::node & node; std::unordered_map last_votes;