From 1c5b94079557ba632542458b9ae180b4073f7cab Mon Sep 17 00:00:00 2001 From: Sergey Kroshnin Date: Tue, 5 Jan 2021 22:26:29 +0300 Subject: [PATCH] Storing blocks according to tally in elections with forks (#3026) * Storing blocks according to tally in elections with forks Additionally store tally in inactive votes cache * Fix build error * Update test * Fix build * Apply Wesley review * Apply Wesley review * Replace assert with if condition (Colin review) * Remove hash from active container * Drop removed blocks from inactive votes ilter * Improve election::remove_block () * Replace assert with if condition (Guilherme & Colin review) * Unlock before sorting (Guilherme review) --- nano/core_test/active_transactions.cpp | 144 +++++++++++++++++++++++++ nano/node/active_transactions.cpp | 8 ++ nano/node/active_transactions.hpp | 4 +- nano/node/election.cpp | 90 +++++++++++++++- nano/node/election.hpp | 3 + 5 files changed, 246 insertions(+), 3 deletions(-) diff --git a/nano/core_test/active_transactions.cpp b/nano/core_test/active_transactions.cpp index a856da1e79..5ed593a9d0 100644 --- a/nano/core_test/active_transactions.cpp +++ b/nano/core_test/active_transactions.cpp @@ -850,6 +850,150 @@ TEST (active_transactions, fork_filter_cleanup) ASSERT_EQ (10, election->blocks ().size ()); } +TEST (active_transactions, fork_replacement_tally) +{ + nano::system system; + nano::node_config node_config (nano::get_available_port (), system.logging); + node_config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled; + auto & node1 (*system.add_node (node_config)); + + nano::genesis genesis; + size_t reps_count = 20; + size_t const max_blocks = 10; + std::vector keys (reps_count); + auto latest (genesis.hash ()); + auto balance (nano::genesis_amount); + auto amount (node1.minimum_principal_weight ()); + nano::state_block_builder builder; + + // Create 20 representatives & confirm blocks + for (auto i (0); i < reps_count; i++) + { + balance -= amount + i; + auto send = builder.make_block () + .account (nano::dev_genesis_key.pub) + .previous (latest) + .representative (nano::dev_genesis_key.pub) + .balance (balance) + .link (keys[i].pub) + .sign (nano::dev_genesis_key.prv, nano::dev_genesis_key.pub) + .work (*system.work.generate (latest)) + .build_shared (); + node1.process_active (send); + latest = send->hash (); + auto open = builder.make_block () + .account (keys[i].pub) + .previous (0) + .representative (keys[i].pub) + .balance (amount + i) + .link (send->hash ()) + .sign (keys[i].prv, keys[i].pub) + .work (*system.work.generate (keys[i].pub)) + .build_shared (); + node1.process_active (open); + // Confirmation + auto vote (std::make_shared (nano::dev_genesis_key.pub, nano::dev_genesis_key.prv, 0, std::vector{ send->hash (), open->hash () })); + node1.vote_processor.vote (vote, std::make_shared (node1)); + } + node1.block_processor.flush (); + ASSERT_TIMELY (5s, node1.ledger.cache.cemented_count == 1 + 2 * reps_count); + + nano::keypair key; + auto send_last = builder.make_block () + .account (nano::dev_genesis_key.pub) + .previous (latest) + .representative (nano::dev_genesis_key.pub) + .balance (balance - 2 * nano::Gxrb_ratio) + .link (key.pub) + .sign (nano::dev_genesis_key.prv, nano::dev_genesis_key.pub) + .work (*system.work.generate (latest)) + .build_shared (); + + // Forks without votes + for (auto i (0); i < reps_count; i++) + { + auto fork = builder.make_block () + .account (nano::dev_genesis_key.pub) + .previous (latest) + .representative (nano::dev_genesis_key.pub) + .balance (balance - nano::Gxrb_ratio - i) + .link (key.pub) + .sign (nano::dev_genesis_key.prv, nano::dev_genesis_key.pub) + .work (*system.work.generate (latest)) + .build_shared (); + node1.process_active (fork); + } + node1.block_processor.flush (); + ASSERT_TIMELY (3s, !node1.active.empty ()); + // Check overflow of blocks + auto election (node1.active.election (send_last->qualified_root ())); + ASSERT_NE (nullptr, election); + ASSERT_EQ (max_blocks, election->blocks ().size ()); + + // Generate forks with votes to prevent new block insertion to election + for (auto i (0); i < reps_count; i++) + { + auto fork = builder.make_block () + .account (nano::dev_genesis_key.pub) + .previous (latest) + .representative (nano::dev_genesis_key.pub) + .balance (balance - 1 - i) + .link (key.pub) + .sign (nano::dev_genesis_key.prv, nano::dev_genesis_key.pub) + .work (*system.work.generate (latest)) + .build_shared (); + auto vote (std::make_shared (keys[i].pub, keys[i].prv, 0, std::vector{ fork->hash () })); + node1.vote_processor.vote (vote, std::make_shared (node1)); + node1.vote_processor.flush (); + node1.process_active (fork); + } + node1.block_processor.flush (); + // Check overflow of blocks + ASSERT_EQ (max_blocks, election->blocks ().size ()); + // Check that only max weight blocks remains (and start winner) + auto votes1 (election->votes ()); + ASSERT_EQ (max_blocks, votes1.size ()); + for (auto i (max_blocks + 1); i < reps_count; i++) + { + ASSERT_TRUE (votes1.find (keys[i].pub) != votes1.end ()); + } + + // Process correct block + node_config.peering_port = nano::get_available_port (); + auto & node2 (*system.add_node (node_config)); + node2.network.flood_block (send_last); + ASSERT_TIMELY (3s, node1.stats.count (nano::stat::type::message, nano::stat::detail::publish, nano::stat::dir::in) > 0); + node1.block_processor.flush (); + std::this_thread::sleep_for (50ms); + + // Correct block without votes is ignored + auto blocks1 (election->blocks ()); + ASSERT_EQ (max_blocks, blocks1.size ()); + ASSERT_FALSE (blocks1.find (send_last->hash ()) != blocks1.end ()); + + // Process vote for correct block & replace existing lowest tally block + auto vote (std::make_shared (nano::dev_genesis_key.pub, nano::dev_genesis_key.prv, 0, std::vector{ send_last->hash () })); + node1.vote_processor.vote (vote, std::make_shared (node1)); + node1.vote_processor.flush (); + node2.network.flood_block (send_last); + ASSERT_TIMELY (3s, node1.stats.count (nano::stat::type::message, nano::stat::detail::publish, nano::stat::dir::in) > 1); + node1.block_processor.flush (); + std::this_thread::sleep_for (50ms); + + auto blocks2 (election->blocks ()); + ASSERT_EQ (max_blocks, blocks2.size ()); + ASSERT_TRUE (blocks2.find (send_last->hash ()) != blocks2.end ()); + auto votes2 (election->votes ()); + ASSERT_EQ (max_blocks, votes2.size ()); + for (auto i (max_blocks + 2); i < reps_count; i++) + { + ASSERT_TRUE (votes2.find (keys[i].pub) != votes2.end ()); + } + ASSERT_FALSE (votes2.find (keys[max_blocks].pub) != votes2.end ()); + ASSERT_FALSE (votes2.find (keys[max_blocks + 1].pub) != votes2.end ()); + ASSERT_TRUE (votes2.find (nano::dev_genesis_key.pub) != votes2.end ()); +} + namespace nano { // Blocks that won an election must always be seen as confirming or cemented diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index d0d0769608..713e1ed0f1 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -1239,6 +1239,13 @@ void nano::active_transactions::erase (nano::qualified_root const & root_a) } } +void nano::active_transactions::erase_hash (nano::block_hash const & hash_a) +{ + nano::unique_lock lock (mutex); + [[maybe_unused]] auto erased (blocks.erase (hash_a)); + debug_assert (erased == 1); +} + bool nano::active_transactions::empty () { nano::lock_guard lock (mutex); @@ -1460,6 +1467,7 @@ nano::inactive_cache_status nano::active_transactions::inactive_votes_bootstrap_ { tally += node.ledger.weight (voter); } + status.tally = tally; if (!previously_a.confirmed && tally >= node.online_reps.delta ()) { diff --git a/nano/node/active_transactions.hpp b/nano/node/active_transactions.hpp index bb641f34f1..1474ebc7c0 100644 --- a/nano/node/active_transactions.hpp +++ b/nano/node/active_transactions.hpp @@ -55,10 +55,11 @@ class inactive_cache_status final bool bootstrap_started{ false }; bool election_started{ false }; // Did item reach config threshold to start an impromptu election? bool confirmed{ false }; // Did item reach votes quorum? (minimum config value) + nano::uint128_t tally{ 0 }; // Last votes tally for block bool operator!= (inactive_cache_status const other) const { - return bootstrap_started != other.bootstrap_started || election_started != other.election_started || confirmed != other.confirmed; + return bootstrap_started != other.bootstrap_started || election_started != other.election_started || confirmed != other.confirmed || tally != other.tally; } }; @@ -198,6 +199,7 @@ class active_transactions final uint64_t limited_active_difficulty (nano::work_version const, uint64_t const); double active_multiplier (); void erase (nano::block const &); + void erase_hash (nano::block_hash const &); bool empty (); size_t size (); void stop (); diff --git a/nano/node/election.cpp b/nano/node/election.cpp index ed854e50af..cb716c1686 100644 --- a/nano/node/election.cpp +++ b/nano/node/election.cpp @@ -390,13 +390,14 @@ bool nano::election::publish (std::shared_ptr const & block_a) // Do not insert new blocks if already confirmed auto result (confirmed ()); - if (!result && last_blocks.size () >= 10) + if (!result && last_blocks.size () >= max_blocks && last_blocks.find (block_a->hash ()) == last_blocks.end ()) { - if (last_tally[block_a->hash ()] < node.online_reps.trended () / 10) + if (!replace_by_weight (lock, block_a->hash ())) { result = true; node.network.publish_filter.clear (block_a); } + debug_assert (lock.owns_lock ()); } if (!result) { @@ -415,6 +416,12 @@ bool nano::election::publish (std::shared_ptr const & block_a) } } } + /* + Result is true if: + 1) election is confirmed or expired + 2) given election contains 10 blocks & new block didn't receive enough votes to replace existing blocks + 3) given block in already in election & election contains less than 10 blocks (replacing block content with new) + */ return result; } @@ -524,6 +531,85 @@ void nano::election::remove_votes (nano::block_hash const & hash_a) } } +void nano::election::remove_block (nano::block_hash const & hash_a) +{ + debug_assert (!mutex.try_lock ()); + if (status.winner->hash () != hash_a) + { + if (auto existing = last_blocks.find (hash_a); existing != last_blocks.end ()) + { + for (auto i (last_votes.begin ()); i != last_votes.end ();) + { + if (i->second.hash == hash_a) + { + i = last_votes.erase (i); + } + else + { + ++i; + } + } + node.network.publish_filter.clear (existing->second); + last_blocks.erase (hash_a); + } + } +} + +bool nano::election::replace_by_weight (nano::unique_lock & lock_a, nano::block_hash const & hash_a) +{ + debug_assert (lock_a.owns_lock ()); + nano::block_hash replaced_block (0); + auto winner_hash (status.winner->hash ()); + // Sort existing blocks tally + std::vector> sorted; + sorted.reserve (last_tally.size ()); + std::copy (last_tally.begin (), last_tally.end (), std::back_inserter (sorted)); + lock_a.unlock (); + // Sort in ascending order + std::sort (sorted.begin (), sorted.end (), [](auto const & left, auto const & right) { return left.second < right.second; }); + // Replace if lowest tally is below inactive cache new block weight + auto inactive_existing (node.active.find_inactive_votes_cache (hash_a)); + auto inactive_tally (inactive_existing.status.tally); + if (inactive_tally > 0 && sorted.size () < max_blocks) + { + // If count of tally items is less than 10, remove any block without tally + for (auto const & [hash, block] : blocks ()) + { + if (std::find_if (sorted.begin (), sorted.end (), [& hash = hash](auto const & item_a) { return item_a.first == hash; }) == sorted.end () && hash != winner_hash) + { + replaced_block = hash; + break; + } + } + } + else if (inactive_tally > 0 && inactive_tally > sorted.front ().second) + { + if (sorted.front ().first != winner_hash) + { + replaced_block = sorted.front ().first; + } + else if (inactive_tally > sorted[1].second) + { + // Avoid removing winner + replaced_block = sorted[1].first; + } + } + + bool replaced (false); + if (!replaced_block.is_zero ()) + { + node.active.erase_hash (replaced_block); + lock_a.lock (); + remove_block (replaced_block); + replaced = true; + } + else + { + lock_a.lock (); + } + return replaced; +} + void nano::election::force_confirm (nano::election_status_type type_a) { release_assert (node.network_params.network.is_dev_network ()); diff --git a/nano/node/election.hpp b/nano/node/election.hpp index 22ce77ab25..db938c6196 100644 --- a/nano/node/election.hpp +++ b/nano/node/election.hpp @@ -126,6 +126,8 @@ class election final : public std::enable_shared_from_this // Calculate votes for local representatives void generate_votes () const; void remove_votes (nano::block_hash const &); + void remove_block (nano::block_hash const &); + bool replace_by_weight (nano::unique_lock & lock_a, nano::block_hash const &); nano::election_cleanup_info cleanup_info_impl () const; private: @@ -140,6 +142,7 @@ class election final : public std::enable_shared_from_this mutable std::mutex mutex; static std::chrono::seconds constexpr late_blocks_delay{ 5 }; + static size_t constexpr max_blocks{ 10 }; friend class active_transactions; friend class confirmation_solicitor;