From 09fd2b1d450906850198bf11ebf07cf6379e4e0d Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Wed, 18 Mar 2020 15:59:10 +0000 Subject: [PATCH] Check if a vote is for a recently confirmed block (#2663) * Check if a vote is for a recently confirmed block * Formatting * Use count() as the iterator is not necessary (Wes comment) --- nano/core_test/active_transactions.cpp | 38 ++++++++++++++------------ nano/node/active_transactions.cpp | 30 +++++++++++++++----- nano/node/active_transactions.hpp | 11 ++++++-- nano/node/election.cpp | 2 +- 4 files changed, 52 insertions(+), 29 deletions(-) diff --git a/nano/core_test/active_transactions.cpp b/nano/core_test/active_transactions.cpp index 60dcdcb3..47a7887e 100644 --- a/nano/core_test/active_transactions.cpp +++ b/nano/core_test/active_transactions.cpp @@ -636,6 +636,8 @@ TEST (active_transactions, update_difficulty) } } +namespace nano +{ TEST (active_transactions, vote_replays) { nano::system system; @@ -659,24 +661,16 @@ TEST (active_transactions, vote_replays) ASSERT_EQ (nano::vote_code::vote, node.active.vote (vote_send1)); ASSERT_EQ (2, node.active.size ()); ASSERT_EQ (nano::vote_code::replay, node.active.vote (vote_send1)); - // Wait until the election is removed, at which point the vote should be indeterminate - system.deadline_set (3s); - while (node.active.size () != 1) - { - ASSERT_NO_ERROR (system.poll ()); - } - ASSERT_EQ (nano::vote_code::indeterminate, node.active.vote (vote_send1)); + // Wait until the election is removed, at which point the vote is still a replay since it's been recently confirmed + ASSERT_TIMELY (3s, node.active.size () == 1); + ASSERT_EQ (nano::vote_code::replay, node.active.vote (vote_send1)); // Open new account auto vote_open1 (std::make_shared (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 0, open1)); ASSERT_EQ (nano::vote_code::vote, node.active.vote (vote_open1)); ASSERT_EQ (1, node.active.size ()); ASSERT_EQ (nano::vote_code::replay, node.active.vote (vote_open1)); - system.deadline_set (3s); - while (!node.active.empty ()) - { - ASSERT_NO_ERROR (system.poll ()); - } - ASSERT_EQ (nano::vote_code::indeterminate, node.active.vote (vote_open1)); + ASSERT_TIMELY (3s, node.active.empty ()); + ASSERT_EQ (nano::vote_code::replay, node.active.vote (vote_open1)); ASSERT_EQ (nano::Gxrb_ratio, node.ledger.weight (key.pub)); auto send2 (std::make_shared (key.pub, open1->hash (), key.pub, nano::Gxrb_ratio - 1, key.pub, key.prv, key.pub, *system.work.generate (open1->hash ()))); @@ -693,14 +687,22 @@ TEST (active_transactions, vote_replays) ASSERT_EQ (nano::vote_code::vote, node.active.vote (vote1_send2)); ASSERT_EQ (1, node.active.size ()); ASSERT_EQ (nano::vote_code::replay, node.active.vote (vote1_send2)); - while (!node.active.empty ()) - { - ASSERT_NO_ERROR (system.poll ()); - } + ASSERT_TIMELY (3s, node.active.empty ()); ASSERT_EQ (0, node.active.size ()); + ASSERT_EQ (nano::vote_code::replay, node.active.vote (vote1_send2)); + ASSERT_EQ (nano::vote_code::replay, node.active.vote (vote2_send2)); + + // Removing blocks as recently confirmed makes every vote indeterminate + { + nano::lock_guard guard (node.active.mutex); + node.active.recently_confirmed.clear (); + } + ASSERT_EQ (nano::vote_code::indeterminate, node.active.vote (vote_send1)); + ASSERT_EQ (nano::vote_code::indeterminate, node.active.vote (vote_open1)); ASSERT_EQ (nano::vote_code::indeterminate, node.active.vote (vote1_send2)); ASSERT_EQ (nano::vote_code::indeterminate, node.active.vote (vote2_send2)); } +} TEST (active_transactions, activate_dependencies) { @@ -846,7 +848,7 @@ TEST (active_transactions, confirmation_consistency) } nano::lock_guard guard (node.active.mutex); ASSERT_EQ (i + 1, node.active.recently_confirmed.size ()); - ASSERT_EQ (block->qualified_root (), node.active.recently_confirmed.back ()); + ASSERT_EQ (block->qualified_root (), node.active.recently_confirmed.back ().first); ASSERT_EQ (i + 1, node.active.recently_cemented.size ()); } } diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index c20049f2..993c04e3 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -517,9 +517,10 @@ std::pair, bool> nano::active_transactions::inse // Validate a vote and apply it to the current election if one exists nano::vote_code nano::active_transactions::vote (std::shared_ptr vote_a) { - // If none of the hashes are active, it is unknown whether it's a replay - // In this case, votes are also not republished + // If none of the hashes are active, votes are not republished bool at_least_one (false); + // If all hashes were recently confirmed then it is a replay + unsigned recently_confirmed_counter (0); bool replay (false); bool processed (false); { @@ -527,6 +528,7 @@ nano::vote_code nano::active_transactions::vote (std::shared_ptr vot for (auto vote_block : vote_a->blocks) { nano::election_vote_result result; + auto & recently_confirmed_by_hash (recently_confirmed.get ()); if (vote_block.which ()) { auto block_hash (boost::get (vote_block)); @@ -536,10 +538,14 @@ nano::vote_code nano::active_transactions::vote (std::shared_ptr vot at_least_one = true; result = existing->second->vote (vote_a->account, vote_a->sequence, block_hash); } - else // possibly a vote for a recently confirmed election + else if (recently_confirmed_by_hash.count (block_hash) == 0) { add_inactive_votes_cache (block_hash, vote_a->account); } + else + { + ++recently_confirmed_counter; + } } else { @@ -550,23 +556,33 @@ nano::vote_code nano::active_transactions::vote (std::shared_ptr vot at_least_one = true; result = existing->election->vote (vote_a->account, vote_a->sequence, block->hash ()); } - else + else if (recently_confirmed_by_hash.count (block->hash ()) == 0) { add_inactive_votes_cache (block->hash (), vote_a->account); } + else + { + ++recently_confirmed_counter; + } } processed = processed || result.processed; replay = replay || result.replay; } } + if (at_least_one) { + // Republish vote if it is new and the node does not host a principal representative (or close to) if (processed && !node.wallets.rep_counts ().have_half_rep ()) { node.network.flood_vote (vote_a, 0.5f); } return replay ? nano::vote_code::replay : nano::vote_code::vote; } + else if (recently_confirmed_counter == vote_a->blocks.size ()) + { + return nano::vote_code::replay; + } else { return nano::vote_code::indeterminate; @@ -792,10 +808,10 @@ void nano::active_transactions::add_recently_cemented (nano::election_status con } } -void nano::active_transactions::add_recently_confirmed (nano::qualified_root const & root_a) +void nano::active_transactions::add_recently_confirmed (nano::qualified_root const & root_a, nano::block_hash const & hash_a) { - recently_confirmed.get ().push_back (root_a); - if (recently_confirmed.size () > node.config.confirmation_history_size) + recently_confirmed.get ().emplace_back (root_a, hash_a); + if (recently_confirmed.size () > recently_confirmed_size) { recently_confirmed.get ().pop_front (); } diff --git a/nano/node/active_transactions.hpp b/nano/node/active_transactions.hpp index 59e7c930..07361499 100644 --- a/nano/node/active_transactions.hpp +++ b/nano/node/active_transactions.hpp @@ -124,7 +124,7 @@ public: std::deque recently_cemented; void add_recently_cemented (nano::election_status const &); - void add_recently_confirmed (nano::qualified_root const &); + void add_recently_confirmed (nano::qualified_root const &, nano::block_hash const &); void add_inactive_votes_cache (nano::block_hash const &, nano::account const &); nano::inactive_cache_information find_inactive_votes_cache (nano::block_hash const &); void erase_inactive_votes_cache (nano::block_hash const &); @@ -164,12 +164,16 @@ private: // Maximum time an election can be kept active if it is extending the container std::chrono::seconds const election_time_to_live; + static size_t constexpr recently_confirmed_size{ 65536 }; + using recent_confirmation = std::pair; // clang-format off - boost::multi_index_container>, mi::hashed_unique, - mi::identity>>> + mi::member>, + mi::hashed_unique, + mi::member>>> recently_confirmed; using prioritize_num_uncemented = boost::multi_index_containerhash (), this_l); } - node.active.add_recently_confirmed (status_l.winner->qualified_root ()); + node.active.add_recently_confirmed (status_l.winner->qualified_root (), status_l.winner->hash ()); node.background ([node_l, status_l, confirmation_action_l, this_l]() { node_l->process_confirmed (status_l, this_l); confirmation_action_l (status_l.winner);