From 97e164f10fac3b4f7b0def2e12b39ff3835fc790 Mon Sep 17 00:00:00 2001 From: clemahieu Date: Wed, 8 Jan 2020 16:06:38 -0300 Subject: [PATCH] Adding forked votes to inactive vote cache. (#2463) There's no guarantee that the first block we observe is the winning fork. This change processes all votes for a particular election instead of just the ones for thee first block observed. --- nano/core_test/active_transactions.cpp | 33 +++++++++++++++++++- nano/node/active_transactions.cpp | 2 +- nano/node/election.cpp | 8 ++--- nano/node/election.hpp | 2 +- nano/node/node.cpp | 42 +++++++++++--------------- 5 files changed, 56 insertions(+), 31 deletions(-) diff --git a/nano/core_test/active_transactions.cpp b/nano/core_test/active_transactions.cpp index c2f641fa..6a94a256 100644 --- a/nano/core_test/active_transactions.cpp +++ b/nano/core_test/active_transactions.cpp @@ -411,6 +411,37 @@ TEST (active_transactions, inactive_votes_cache) ASSERT_EQ (1, system.nodes[0]->stats.count (nano::stat::type::election, nano::stat::detail::vote_cached)); } +TEST (active_transactions, inactive_votes_cache_fork) +{ + nano::system system (1); + nano::block_hash latest (system.nodes[0]->latest (nano::test_genesis_key.pub)); + nano::keypair key; + auto send1 (std::make_shared (latest, key.pub, nano::genesis_amount - 100, nano::test_genesis_key.prv, nano::test_genesis_key.pub, *system.work.generate (latest))); + auto send2 (std::make_shared (latest, key.pub, nano::genesis_amount - 200, nano::test_genesis_key.prv, nano::test_genesis_key.pub, *system.work.generate (latest))); + auto vote (std::make_shared (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 0, std::vector (1, send1->hash ()))); + system.nodes[0]->vote_processor.vote (vote, std::make_shared (system.nodes[0]->network.udp_channels, system.nodes[0]->network.endpoint (), system.nodes[0]->network_params.protocol.protocol_version)); + auto channel1 (system.nodes [0]->network.udp_channels.create (system.nodes [0]->network.endpoint ())); + system.deadline_set (5s); + while (system.nodes[0]->active.inactive_votes_cache_size () != 1) + { + ASSERT_NO_ERROR (system.poll ()); + } + system.nodes[0]->network.process_message (nano::publish (send2), channel1); + system.nodes[0]->block_processor.flush (); + ASSERT_NE (nullptr, system.nodes[0]->block (send2->hash ())); + system.nodes[0]->network.process_message (nano::publish (send1), channel1); + system.nodes[0]->block_processor.flush (); + bool confirmed (false); + system.deadline_set (5s); + while (!confirmed) + { + auto transaction (system.nodes[0]->store.tx_begin_read ()); + confirmed = system.nodes[0]->block (send1->hash ()) != nullptr && system.nodes[0]->ledger.block_confirmed (transaction, send1->hash ()); + ASSERT_NO_ERROR (system.poll ()); + } + ASSERT_EQ (1, system.nodes[0]->stats.count (nano::stat::type::election, nano::stat::detail::vote_cached)); +} + TEST (active_transactions, inactive_votes_cache_existing_vote) { nano::system system; @@ -457,7 +488,7 @@ TEST (active_transactions, inactive_votes_cache_existing_vote) // Attempt to change vote with inactive_votes_cache node->active.add_inactive_votes_cache (send->hash (), key.pub); ASSERT_EQ (1, node->active.find_inactive_votes_cache (send->hash ()).voters.size ()); - election->insert_inactive_votes_cache (); + election->insert_inactive_votes_cache (send->hash ()); // Check that election data is not changed ASSERT_EQ (2, election->last_votes.size ()); auto last_vote2 (election->last_votes[key.pub]); diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index 61a4863a..ee9f5b36 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -523,7 +523,7 @@ bool nano::active_transactions::add (std::shared_ptr block_a, bool roots.get ().emplace (nano::conflict_info{ root, difficulty, difficulty, election }); blocks.insert (std::make_pair (hash, election)); adjust_difficulty (hash); - election->insert_inactive_votes_cache (); + election->insert_inactive_votes_cache (hash); } } return error; diff --git a/nano/node/election.cpp b/nano/node/election.cpp index 333ba61a..a1cc968d 100644 --- a/nano/node/election.cpp +++ b/nano/node/election.cpp @@ -218,6 +218,7 @@ bool nano::election::publish (std::shared_ptr block_a) if (blocks.find (block_a->hash ()) == blocks.end ()) { blocks.insert (std::make_pair (block_a->hash (), block_a)); + insert_inactive_votes_cache (block_a->hash ()); confirm_if_quorum (); node.network.flood_block (block_a, false); } @@ -296,13 +297,12 @@ void nano::election::clear_blocks () } } -void nano::election::insert_inactive_votes_cache () +void nano::election::insert_inactive_votes_cache (nano::block_hash const & hash_a) { - auto winner_hash (status.winner->hash ()); - auto cache (node.active.find_inactive_votes_cache (winner_hash)); + auto cache (node.active.find_inactive_votes_cache (hash_a)); for (auto & rep : cache.voters) { - auto inserted (last_votes.emplace (rep, nano::vote_info{ std::chrono::steady_clock::time_point::min (), 0, winner_hash })); + auto inserted (last_votes.emplace (rep, nano::vote_info{ std::chrono::steady_clock::time_point::min (), 0, hash_a })); if (inserted.second) { node.stats.inc (nano::stat::type::election, nano::stat::detail::vote_cached); diff --git a/nano/node/election.hpp b/nano/node/election.hpp index ee097bcd..8aec3179 100644 --- a/nano/node/election.hpp +++ b/nano/node/election.hpp @@ -67,7 +67,7 @@ public: void update_dependent (); void clear_dependent (); void clear_blocks (); - void insert_inactive_votes_cache (); + void insert_inactive_votes_cache (nano::block_hash const &); void stop (); nano::node & node; std::unordered_map last_votes; diff --git a/nano/node/node.cpp b/nano/node/node.cpp index feae983d..12d0f027 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -551,32 +551,26 @@ void nano::node::process_fork (nano::transaction const & transaction_a, std::sha std::shared_ptr ledger_block (ledger.forked_block (transaction_a, *block_a)); if (ledger_block && !block_confirmed_or_being_confirmed (transaction_a, ledger_block->hash ())) { - // Clear inactive votes cache for forks - { - nano::lock_guard lock (active.mutex); - active.erase_inactive_votes_cache (ledger_block->hash ()); - active.erase_inactive_votes_cache (block_a->hash ()); - } std::weak_ptr this_w (shared_from_this ()); if (!active.start (ledger_block, false, [this_w, root](std::shared_ptr) { - if (auto this_l = this_w.lock ()) - { - auto attempt (this_l->bootstrap_initiator.current_attempt ()); - if (attempt && attempt->mode == nano::bootstrap_mode::legacy) - { - auto transaction (this_l->store.tx_begin_read ()); - auto account (this_l->ledger.store.frontier_get (transaction, root)); - if (!account.is_zero ()) - { - attempt->requeue_pull (nano::pull_info (account, root, root)); - } - else if (this_l->ledger.store.account_exists (transaction, root)) - { - attempt->requeue_pull (nano::pull_info (root, nano::block_hash (0), nano::block_hash (0))); - } - } - } - })) + if (auto this_l = this_w.lock ()) + { + auto attempt (this_l->bootstrap_initiator.current_attempt ()); + if (attempt && attempt->mode == nano::bootstrap_mode::legacy) + { + auto transaction (this_l->store.tx_begin_read ()); + auto account (this_l->ledger.store.frontier_get (transaction, root)); + if (!account.is_zero ()) + { + attempt->requeue_pull (nano::pull_info (account, root, root)); + } + else if (this_l->ledger.store.account_exists (transaction, root)) + { + attempt->requeue_pull (nano::pull_info (root, nano::block_hash (0), nano::block_hash (0))); + } + } + } + })) { logger.always_log (boost::str (boost::format ("Resolving fork between our block: %1% and block %2% both with root %3%") % ledger_block->hash ().to_string () % block_a->hash ().to_string () % block_a->root ().to_string ())); network.broadcast_confirm_req (ledger_block);