diff --git a/nano/core_test/confirmation_solicitor.cpp b/nano/core_test/confirmation_solicitor.cpp index bd963ab6..260b39b8 100644 --- a/nano/core_test/confirmation_solicitor.cpp +++ b/nano/core_test/confirmation_solicitor.cpp @@ -87,3 +87,57 @@ TEST (confirmation_solicitor, different_hash) solicitor.flush (); ASSERT_EQ (1, node2.stats.count (nano::stat::type::message, nano::stat::detail::confirm_req, nano::stat::dir::out)); } + +TEST (confirmation_solicitor, bypass_max_requests_cap) +{ + nano::system system; + nano::node_flags node_flags; + node_flags.disable_request_loop = true; + node_flags.disable_rep_crawler = true; + node_flags.disable_udp = false; + auto & node1 = *system.add_node (node_flags); + auto & node2 = *system.add_node (node_flags); + nano::confirmation_solicitor solicitor (node2.network, node2.network_params.network); + std::vector representatives; + auto max_representatives = std::max (solicitor.max_election_requests, solicitor.max_election_broadcasts); + representatives.reserve (max_representatives + 1); + for (auto i (0); i < max_representatives + 1; ++i) + { + auto channel (node2.network.udp_channels.create (node1.network.endpoint ())); + nano::representative representative (nano::account (i), i, channel); + representatives.push_back (representative); + } + ASSERT_EQ (max_representatives + 1, representatives.size ()); + solicitor.prepare (representatives); + ASSERT_TIMELY (3s, node2.network.size () == 1); + auto send (std::make_shared (nano::genesis_hash, nano::keypair ().pub, nano::genesis_amount - 100, nano::dev_genesis_key.prv, nano::dev_genesis_key.pub, *system.work.generate (nano::genesis_hash))); + send->sideband_set ({}); + { + nano::lock_guard guard (node2.active.mutex); + auto election (std::make_shared (node2, send, nullptr, false)); + // Add a vote for something else, not the winner + for (auto const & rep : representatives) + { + election->last_votes[rep.account] = { std::chrono::steady_clock::now (), 1, 1 }; + } + ASSERT_FALSE (solicitor.add (*election)); + ASSERT_FALSE (solicitor.broadcast (*election)); + } + solicitor.flush (); + // All requests went through, the last one would normally not go through due to the cap but a vote for a different hash does not count towards the cap + ASSERT_EQ (max_representatives + 1, node2.stats.count (nano::stat::type::message, nano::stat::detail::confirm_req, nano::stat::dir::out)); + + solicitor.prepare (representatives); + { + nano::lock_guard guard (node2.active.mutex); + auto election (std::make_shared (node2, send, nullptr, false)); + // Erase all votes + election->last_votes.clear (); + ASSERT_FALSE (solicitor.add (*election)); + ASSERT_FALSE (solicitor.broadcast (*election)); + } + + solicitor.flush (); + // All requests but one went through, due to the cap + ASSERT_EQ (2 * max_representatives + 1, node2.stats.count (nano::stat::type::message, nano::stat::detail::confirm_req, nano::stat::dir::out)); +} diff --git a/nano/node/confirmation_solicitor.cpp b/nano/node/confirmation_solicitor.cpp index fcfc1189..0dc0bd64 100644 --- a/nano/node/confirmation_solicitor.cpp +++ b/nano/node/confirmation_solicitor.cpp @@ -6,7 +6,7 @@ using namespace std::chrono_literals; nano::confirmation_solicitor::confirmation_solicitor (nano::network & network_a, nano::network_constants const & params_a) : max_confirm_req_batches (params_a.is_dev_network () ? 1 : 20), max_block_broadcasts (params_a.is_dev_network () ? 4 : 30), -max_election_requests (30), +max_election_requests (50), max_election_broadcasts (std::max (network_a.fanout () / 2, 1)), network (network_a) { @@ -36,10 +36,12 @@ bool nano::confirmation_solicitor::broadcast (nano::election const & election_a) for (auto i (representatives_broadcasts.begin ()), n (representatives_broadcasts.end ()); i != n && count < max_election_broadcasts; ++i) { auto existing (election_a.last_votes.find (i->account)); - if (existing == election_a.last_votes.end () || existing->second.hash != hash) + bool const exists (existing != election_a.last_votes.end ()); + bool const different (exists && existing->second.hash != hash); + if (!exists || different) { i->channel->send (winner); - ++count; + count += different ? 0 : 1; } } // Random flood for block propagation @@ -52,21 +54,25 @@ bool nano::confirmation_solicitor::broadcast (nano::election const & election_a) bool nano::confirmation_solicitor::add (nano::election const & election_a) { debug_assert (prepared); - auto const max_channel_requests (max_confirm_req_batches * nano::network::confirm_req_hashes_max); + bool error (true); unsigned count = 0; + auto const max_channel_requests (max_confirm_req_batches * nano::network::confirm_req_hashes_max); auto const & hash (election_a.status.winner->hash ()); for (auto i (representatives_requests.begin ()); i != representatives_requests.end () && count < max_election_requests;) { bool full_queue (false); auto rep (*i); auto existing (election_a.last_votes.find (rep.account)); - if (existing == election_a.last_votes.end () || existing->second.hash != hash) + bool const exists (existing != election_a.last_votes.end ()); + bool const different (exists && existing->second.hash != hash); + if (!exists || different) { auto & request_queue (requests[rep.channel]); if (request_queue.size () < max_channel_requests) { request_queue.emplace_back (election_a.status.winner->hash (), election_a.status.winner->root ()); - ++count; + count += different ? 0 : 1; + error = false; } else { @@ -75,7 +81,7 @@ bool nano::confirmation_solicitor::add (nano::election const & election_a) } i = !full_queue ? i + 1 : representatives_requests.erase (i); } - return count == 0; + return error; } void nano::confirmation_solicitor::flush () diff --git a/nano/node/confirmation_solicitor.hpp b/nano/node/confirmation_solicitor.hpp index bce99b32..ef04cca6 100644 --- a/nano/node/confirmation_solicitor.hpp +++ b/nano/node/confirmation_solicitor.hpp @@ -26,7 +26,7 @@ public: size_t const max_confirm_req_batches; /** Global maximum amount of block broadcasts */ size_t const max_block_broadcasts; - /** Maximum amount of requests to be sent per election */ + /** Maximum amount of requests to be sent per election, bypassed if an existing vote is for a different hash*/ size_t const max_election_requests; /** Maximum amount of directed broadcasts to be sent per election */ size_t const max_election_broadcasts;