From 3cd7dfb25b579a5affcc6ed4bdee5e9dfffa65b1 Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Fri, 3 Jan 2020 22:09:56 +0000 Subject: [PATCH] Avoid replying to confirm_req with repeated votes (#2451) * Avoid replying to confirm_req with repeated votes * Use vector + find due to the low number of items (up to 15) --- nano/core_test/node.cpp | 40 ++++++++++++++++++++++++++++++++++++++++ nano/node/network.cpp | 16 ++++++++++++++-- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 164e58a4..4d156d4d 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -2400,6 +2400,46 @@ TEST (node, local_votes_cache) ASSERT_FALSE (node.votes_cache.find (send3->hash ()).empty ()); } +TEST (node, local_votes_cache_batch) +{ + nano::system system; + nano::node_config node_config (nano::get_available_port (), system.logging); + node_config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled; + auto & node (*system.add_node (node_config)); + ASSERT_GE (node.network_params.voting.max_cache, 2); + nano::genesis genesis; + system.wallet (0)->insert_adhoc (nano::test_genesis_key.prv); + auto send1 (std::make_shared (nano::test_genesis_key.pub, genesis.hash (), nano::test_genesis_key.pub, nano::genesis_amount - nano::Gxrb_ratio, nano::test_genesis_key.pub, nano::test_genesis_key.prv, nano::test_genesis_key.pub, *node.work_generate_blocking (genesis.hash ()))); + auto send2 (std::make_shared (nano::test_genesis_key.pub, send1->hash (), nano::test_genesis_key.pub, nano::genesis_amount - 2 * nano::Gxrb_ratio, nano::test_genesis_key.pub, nano::test_genesis_key.prv, nano::test_genesis_key.pub, *node.work_generate_blocking (send1->hash ()))); + std::vector> blocks{ send1, send2 }; + std::vector> batch{ { send1->hash (), send1->root () }, { send2->hash (), send2->root () } }; + { + auto transaction (node.store.tx_begin_write ()); + ASSERT_EQ (nano::process_result::progress, node.ledger.process (transaction, *send1).code); + ASSERT_EQ (nano::process_result::progress, node.ledger.process (transaction, *send2).code); + } + nano::confirm_req message (batch); + auto channel (node.network.udp_channels.create (node.network.endpoint ())); + // Generates and sends one vote for both hashes which is then cached + node.network.process_message (message, channel); + ASSERT_EQ (1, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); + ASSERT_FALSE (node.votes_cache.find (send1->hash ()).empty ()); + ASSERT_FALSE (node.votes_cache.find (send2->hash ()).empty ()); + // Only one confirm_ack should be sent if all hashes are part of the same vote + node.network.process_message (message, channel); + ASSERT_EQ (2, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); + // Test when votes are different + node.votes_cache.remove (send1->hash ()); + node.votes_cache.remove (send2->hash ()); + node.network.process_message (nano::confirm_req (send1->hash (), send1->root ()), channel); + ASSERT_EQ (3, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); + node.network.process_message (nano::confirm_req (send2->hash (), send2->root ()), channel); + ASSERT_EQ (4, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); + // There are two different votes, so both should be sent in response + node.network.process_message (message, channel); + ASSERT_EQ (6, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); +} + TEST (node, local_votes_cache_generate_new_vote) { nano::system system; diff --git a/nano/node/network.cpp b/nano/node/network.cpp index 6a27b9b2..ab19c389 100644 --- a/nano/node/network.cpp +++ b/nano/node/network.cpp @@ -487,7 +487,13 @@ public: if (!find_votes.empty ()) { ++cached_count; - cached_votes.insert (cached_votes.end (), find_votes.begin (), find_votes.end ()); + for (auto const & vote : find_votes) + { + if (std::find (cached_votes.begin (), cached_votes.end (), vote) == cached_votes.end ()) + { + cached_votes.push_back (vote); + } + } } if (!find_votes.empty () || (!root_hash.first.is_zero () && node.store.block_exists (transaction, root_hash.first))) { @@ -514,7 +520,13 @@ public: if (!find_successor_votes.empty ()) { ++cached_count; - cached_votes.insert (cached_votes.end (), find_successor_votes.begin (), find_successor_votes.end ()); + for (auto const & vote : find_successor_votes) + { + if (std::find (cached_votes.begin (), cached_votes.end (), vote) == cached_votes.end ()) + { + cached_votes.push_back (vote); + } + } } blocks_bundle.push_back (successor); auto successor_block (node.store.block_get (transaction, successor));