diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 5fcec4db30..616cfa0f0e 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -2003,7 +2003,7 @@ TEST (node, vote_by_hash_bundle) for (auto const & block : blocks) { - system.nodes[0]->generator.add (block->root (), block->hash ()); + system.nodes[0]->generator.add (*block); } // Verify that bundling occurs. While reaching 12 should be common on most hardware in release mode, diff --git a/nano/core_test/voting.cpp b/nano/core_test/voting.cpp index dd0487ddfc..ec0a4f099d 100644 --- a/nano/core_test/voting.cpp +++ b/nano/core_test/voting.cpp @@ -60,13 +60,56 @@ TEST (local_vote_history, basic) } } +/* + * vote_spacing + */ + +TEST (vote_spacing, basic) +{ + nano::vote_spacing spacing{ std::chrono::milliseconds{ 100 } }; + nano::qualified_root root1{ 1 }; + nano::qualified_root root2{ 2 }; + nano::block_hash hash3{ 3 }; + nano::block_hash hash4{ 4 }; + nano::block_hash hash5{ 5 }; + ASSERT_EQ (0, spacing.size ()); + ASSERT_TRUE (spacing.votable (root1, hash3)); + spacing.flag (root1, hash3); + ASSERT_EQ (1, spacing.size ()); + ASSERT_FALSE (spacing.votable (root1, hash3)); + ASSERT_TRUE (spacing.votable (root1, hash4)); + spacing.flag (root2, hash5); + ASSERT_EQ (2, spacing.size ()); +} + +TEST (vote_spacing, prune) +{ + auto now = std::chrono::steady_clock::now (); + + auto length = std::chrono::milliseconds{ 100 }; + nano::vote_spacing spacing{ length }; + nano::qualified_root root1{ 1 }; + nano::qualified_root root2{ 2 }; + nano::block_hash hash3{ 3 }; + nano::block_hash hash4{ 4 }; + spacing.flag (root1, hash3, now); + ASSERT_EQ (1, spacing.size ()); + // Wait for the spacing to expire + spacing.flag (root2, hash4, now + length); + ASSERT_EQ (1, spacing.size ()); +} + +/* + * vote_generator + */ + TEST (vote_generator, cache) { nano::test::system system (1); auto & node (*system.nodes[0]); auto epoch1 = system.upgrade_genesis_epoch (node, nano::epoch::epoch_1); system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv); - node.generator.add (epoch1->root (), epoch1->hash ()); + node.generator.add (*epoch1); ASSERT_TIMELY (1s, !node.history.votes (epoch1->root (), epoch1->hash ()).empty ()); auto votes (node.history.votes (epoch1->root (), epoch1->hash ())); ASSERT_FALSE (votes.empty ()); @@ -110,40 +153,7 @@ TEST (vote_generator, multiple_representatives) } } -TEST (vote_spacing, basic) -{ - nano::vote_spacing spacing{ std::chrono::milliseconds{ 100 } }; - nano::root root1{ 1 }; - nano::root root2{ 2 }; - nano::block_hash hash3{ 3 }; - nano::block_hash hash4{ 4 }; - nano::block_hash hash5{ 5 }; - ASSERT_EQ (0, spacing.size ()); - ASSERT_TRUE (spacing.votable (root1, hash3)); - spacing.flag (root1, hash3); - ASSERT_EQ (1, spacing.size ()); - ASSERT_TRUE (spacing.votable (root1, hash3)); - ASSERT_FALSE (spacing.votable (root1, hash4)); - spacing.flag (root2, hash5); - ASSERT_EQ (2, spacing.size ()); -} - -TEST (vote_spacing, prune) -{ - auto length = std::chrono::milliseconds{ 100 }; - nano::vote_spacing spacing{ length }; - nano::root root1{ 1 }; - nano::root root2{ 2 }; - nano::block_hash hash3{ 3 }; - nano::block_hash hash4{ 4 }; - spacing.flag (root1, hash3); - ASSERT_EQ (1, spacing.size ()); - std::this_thread::sleep_for (length); - spacing.flag (root2, hash4); - ASSERT_EQ (1, spacing.size ()); -} - -TEST (vote_spacing, vote_generator) +TEST (vote_generator, vote_spacing) { nano::node_config config; config.backlog_scan.enable = false; @@ -175,19 +185,20 @@ TEST (vote_spacing, vote_generator) .build (); ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send1)); ASSERT_EQ (0, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts)); - node.generator.add (nano::dev::genesis->hash (), send1->hash ()); - ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts), 1); + node.generator.add (*send1); + ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts)); + ASSERT_FALSE (node.ledger.rollback (node.ledger.tx_begin_write (), send1->hash ())); ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send2)); - node.generator.add (nano::dev::genesis->hash (), send2->hash ()); - ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing), 1); - ASSERT_EQ (1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts)); - std::this_thread::sleep_for (config.network_params.voting.delay); - node.generator.add (nano::dev::genesis->hash (), send2->hash ()); - ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts), 2); + node.generator.add (*send2); + ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts)); + // vote on send2 second time (rapid succession) - expect spacing + node.generator.add (*send2); + ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing)); + ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts)); } -TEST (vote_spacing, rapid) +TEST (vote_generator, vote_spacing_rapid) { nano::node_config config; config.backlog_scan.enable = false; @@ -218,14 +229,22 @@ TEST (vote_spacing, rapid) .work (*system.work.generate (nano::dev::genesis->hash ())) .build (); ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send1)); - node.generator.add (nano::dev::genesis->hash (), send1->hash ()); - ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts), 1); + node.generator.add (*send1); + ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts)); ASSERT_FALSE (node.ledger.rollback (node.ledger.tx_begin_write (), send1->hash ())); ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send2)); - node.generator.add (nano::dev::genesis->hash (), send2->hash ()); - ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing), 1); - ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts)); + // vote on send2 first time - expect : new broadcast & no spacing + node.generator.add (*send2); + ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts)); + ASSERT_TIMELY_EQ (3s, 0, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing)); + + // vote on send2 again rapidly - expect : no broadcast & new spacing + node.generator.add (*send2); + ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts)); + ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing)); std::this_thread::sleep_for (config.network_params.voting.delay); - node.generator.add (nano::dev::genesis->hash (), send2->hash ()); - ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts), 2); + // vote on send2 again after the spacing delay - expect : new broadcast & no spacing + node.generator.add (*send2); + ASSERT_TIMELY_EQ (3s, 3, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts)); + ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing)); } diff --git a/nano/node/election.cpp b/nano/node/election.cpp index 3ee56c1f0b..d89aa720c1 100644 --- a/nano/node/election.cpp +++ b/nano/node/election.cpp @@ -463,7 +463,7 @@ void nano::election::confirm_if_quorum (nano::unique_lock & lock_a) if (!is_quorum.exchange (true) && node.config.enable_voting && node.wallets.reps ().voting > 0) { ++vote_broadcast_count; - node.final_generator.add (root, status.winner->hash ()); + node.final_generator.add (*status.winner); } if (final_weight >= node.online_reps.delta ()) { @@ -668,7 +668,7 @@ void nano::election::broadcast_vote_locked (nano::unique_lock & loc nano::log::arg{ "winner", status.winner }, nano::log::arg{ "type", "final" }); - node.final_generator.add (root, status.winner->hash ()); // Broadcasts vote to the network + node.final_generator.add (*status.winner); // Broadcasts vote to the network } else { @@ -679,7 +679,7 @@ void nano::election::broadcast_vote_locked (nano::unique_lock & loc nano::log::arg{ "winner", status.winner }, nano::log::arg{ "type", "normal" }); - node.generator.add (root, status.winner->hash ()); // Broadcasts vote to the network + node.generator.add (*status.winner); // Broadcasts vote to the network } } } diff --git a/nano/node/vote_generator.cpp b/nano/node/vote_generator.cpp index 9e10e4c393..45b1ef1f5c 100644 --- a/nano/node/vote_generator.cpp +++ b/nano/node/vote_generator.cpp @@ -43,7 +43,7 @@ nano::vote_generator::~vote_generator () debug_assert (!thread.joinable ()); } -bool nano::vote_generator::should_vote (transaction_variant_t const & transaction_variant, nano::root const & root_a, nano::block_hash const & hash_a) const +bool nano::vote_generator::should_vote (transaction_variant_t const & transaction_variant, nano::qualified_root const & root_a, nano::block_hash const & hash_a) const { bool should_vote = false; std::shared_ptr block; @@ -54,7 +54,7 @@ bool nano::vote_generator::should_vote (transaction_variant_t const & transactio block = ledger.any.block_get (transaction, hash_a); should_vote = block != nullptr && ledger.dependents_confirmed (transaction, *block) && ledger.store.final_vote.put (transaction, block->qualified_root (), hash_a); - debug_assert (block == nullptr || root_a == block->root ()); + debug_assert (block == nullptr || root_a == block->qualified_root ()); } else { @@ -98,9 +98,9 @@ void nano::vote_generator::stop () } } -void nano::vote_generator::add (const root & root, const block_hash & hash) +void nano::vote_generator::add (nano::block const & block) { - vote_generation_queue.add (std::make_pair (root, hash)); + vote_generation_queue.add (std::make_pair (block.qualified_root (), block.hash ())); } void nano::vote_generator::process_batch (std::deque & batch) @@ -157,7 +157,7 @@ std::size_t nano::vote_generator::generate (std::vectorledger.dependents_confirmed (transaction, *block_a); }; auto as_candidate = [] (auto const & block_a) { - return candidate_t{ block_a->root (), block_a->hash () }; + return candidate_t{ block_a->qualified_root (), block_a->hash () }; }; nano::transform_if (blocks_a.begin (), blocks_a.end (), std::back_inserter (req_candidates), dependents_confirmed, as_candidate); } @@ -178,7 +178,7 @@ void nano::vote_generator::broadcast (nano::unique_lock & lock_a) debug_assert (lock_a.owns_lock ()); std::vector hashes; - std::vector roots; + std::vector roots; hashes.reserve (nano::network::confirm_ack_hashes_max); roots.reserve (nano::network::confirm_ack_hashes_max); while (!candidates.empty () && hashes.size () < nano::network::confirm_ack_hashes_max) @@ -222,7 +222,7 @@ void nano::vote_generator::reply (nano::unique_lock & lock_a, reque while (i != n && !stopped) { std::vector hashes; - std::vector roots; + std::vector roots; hashes.reserve (nano::network::confirm_ack_hashes_max); roots.reserve (nano::network::confirm_ack_hashes_max); for (; i != n && hashes.size () < nano::network::confirm_ack_hashes_max; ++i) @@ -230,15 +230,9 @@ void nano::vote_generator::reply (nano::unique_lock & lock_a, reque auto const & [root, hash] = *i; if (std::find (roots.begin (), roots.end (), root) == roots.end ()) { - if (spacing.votable (root, hash)) - { - roots.push_back (root); - hashes.push_back (hash); - } - else - { - stats.inc (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing); - } + // Vote spacing skipped for replies - allows all peers to request same vote + roots.push_back (root); + hashes.push_back (hash); } } if (!hashes.empty ()) @@ -256,7 +250,7 @@ void nano::vote_generator::reply (nano::unique_lock & lock_a, reque lock_a.lock (); } -void nano::vote_generator::vote (std::vector const & hashes_a, std::vector const & roots_a, std::function const &)> const & action_a) +void nano::vote_generator::vote (std::vector const & hashes_a, std::vector const & roots_a, std::function const &)> const & action_a) { debug_assert (hashes_a.size () == roots_a.size ()); std::vector> votes_l; @@ -269,7 +263,7 @@ void nano::vote_generator::vote (std::vector const & hashes_a, { for (std::size_t i (0), n (hashes_a.size ()); i != n; ++i) { - history.add (roots_a[i], hashes_a[i], vote_l); + history.add (roots_a[i].root (), hashes_a[i], vote_l); spacing.flag (roots_a[i], hashes_a[i]); } action_a (vote_l); diff --git a/nano/node/vote_generator.hpp b/nano/node/vote_generator.hpp index 4f27dc44c0..af3502d14c 100644 --- a/nano/node/vote_generator.hpp +++ b/nano/node/vote_generator.hpp @@ -27,9 +27,9 @@ namespace nano class vote_generator final { private: - using candidate_t = std::pair; + using candidate_t = std::pair; using request_t = std::pair, std::shared_ptr>; - using queue_entry_t = std::pair; + using queue_entry_t = std::pair; std::chrono::steady_clock::time_point next_broadcast = { std::chrono::steady_clock::now () }; public: @@ -37,9 +37,9 @@ class vote_generator final ~vote_generator (); /** Queue items for vote generation, or broadcast votes already in cache */ - void add (nano::root const &, nano::block_hash const &); + void add (nano::block const & block); /** Queue blocks for vote generation, returning the number of successful candidates.*/ - std::size_t generate (std::vector> const & blocks_a, std::shared_ptr const & channel_a); + std::size_t generate (std::vector> const & blocks, std::shared_ptr const & channel); void start (); void stop (); @@ -52,10 +52,10 @@ class vote_generator final void run (); void broadcast (nano::unique_lock &); void reply (nano::unique_lock &, request_t &&); - void vote (std::vector const &, std::vector const &, std::function const &)> const &); + void vote (std::vector const &, std::vector const &, std::function const &)> const &); void broadcast_action (std::shared_ptr const &) const; void process_batch (std::deque & batch); - bool should_vote (transaction_variant_t const &, nano::root const &, nano::block_hash const &) const; + bool should_vote (transaction_variant_t const &, nano::qualified_root const &, nano::block_hash const &) const; bool broadcast_predicate () const; private: // Dependencies diff --git a/nano/node/vote_spacing.cpp b/nano/node/vote_spacing.cpp index 201a9d7b57..066e3c6e0e 100644 --- a/nano/node/vote_spacing.cpp +++ b/nano/node/vote_spacing.cpp @@ -1,35 +1,39 @@ #include -void nano::vote_spacing::trim () +void nano::vote_spacing::trim (std::chrono::steady_clock::time_point now) { - recent.get ().erase (recent.get ().begin (), recent.get ().upper_bound (std::chrono::steady_clock::now () - delay)); + recent.get ().erase (recent.get ().begin (), recent.get ().upper_bound (now - delay)); } -bool nano::vote_spacing::votable (nano::root const & root_a, nano::block_hash const & hash_a) const +bool nano::vote_spacing::votable (nano::qualified_root const & root, nano::block_hash const & hash, std::chrono::steady_clock::time_point now) const { - bool result = true; - for (auto range = recent.get ().equal_range (root_a); result && range.first != range.second; ++range.first) + if (auto it = recent.get ().find (root); it != recent.end ()) { - auto & item = *range.first; - result = hash_a == item.hash || item.time < std::chrono::steady_clock::now () - delay; + if (hash == it->hash && it->time >= now - delay) + { + return false; // Same hash and not expired -> not votable + } } - return result; + return true; // Either different hash or expired -> votable } -void nano::vote_spacing::flag (nano::root const & root_a, nano::block_hash const & hash_a) +void nano::vote_spacing::flag (nano::qualified_root const & root, nano::block_hash const & hash, std::chrono::steady_clock::time_point now) { - trim (); - auto now = std::chrono::steady_clock::now (); - auto existing = recent.get ().find (root_a); - if (existing != recent.end ()) + trim (now); + + if (auto it = recent.get ().find (root); it != recent.end ()) { - recent.get ().modify (existing, [now] (entry & entry) { + // We update both timestamp and hash because we want to track which fork + // we most recently voted for. This ensures proper spacing between votes + // for the same block while allowing immediate votes for competing forks. + recent.get ().modify (it, [now, hash] (entry & entry) { + entry.hash = hash; entry.time = now; }); } else { - recent.insert ({ root_a, now, hash_a }); + recent.insert ({ root, hash, now }); } } diff --git a/nano/node/vote_spacing.hpp b/nano/node/vote_spacing.hpp index c5ee41827f..ef62e9d913 100644 --- a/nano/node/vote_spacing.hpp +++ b/nano/node/vote_spacing.hpp @@ -16,31 +16,42 @@ namespace nano { class vote_spacing final { - class entry +public: + explicit vote_spacing (std::chrono::milliseconds const & delay) : + delay{ delay } { - public: - nano::root root; - std::chrono::steady_clock::time_point time; + } + + bool votable (nano::qualified_root const & root, nano::block_hash const & hash, std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now ()) const; + void flag (nano::qualified_root const & root, nano::block_hash const & hash, std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now ()); + std::size_t size () const; + +private: + void trim (std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now ()); + +private: + std::chrono::milliseconds const delay; + + struct entry + { + nano::qualified_root root; nano::block_hash hash; + std::chrono::steady_clock::time_point time; }; - boost::multi_index_container, - mi::member>, - mi::ordered_non_unique, - mi::member>>> - recent; - std::chrono::milliseconds const delay; - void trim (); + mi::hashed_unique, + mi::member>, + mi::ordered_non_unique, + mi::member> + >>; + // clang-format on -public: - vote_spacing (std::chrono::milliseconds const & delay) : - delay{ delay } - { - } - bool votable (nano::root const & root_a, nano::block_hash const & hash_a) const; - void flag (nano::root const & root_a, nano::block_hash const & hash_a); - std::size_t size () const; + ordered_votes recent; }; -} +} \ No newline at end of file