From bdf948a0398ecca86aadc6273622952a62673751 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Wed, 31 Aug 2022 00:40:48 +0100 Subject: [PATCH 1/2] Fix for test election.quorum_minimum_update_weight_before_quorum_checks The problem was that there was a race condition between the rep crawler background thread and rep_crawler::response(). For be response to be processed as valid, a request must be marked in rep_crawler::active container. --- nano/core_test/election.cpp | 1 + nano/node/repcrawler.cpp | 7 +++++++ nano/node/repcrawler.hpp | 3 +++ 3 files changed, 11 insertions(+) diff --git a/nano/core_test/election.cpp b/nano/core_test/election.cpp index 49c8f5722f..682bff1d36 100644 --- a/nano/core_test/election.cpp +++ b/nano/core_test/election.cpp @@ -243,6 +243,7 @@ TEST (election, quorum_minimum_update_weight_before_quorum_checks) ASSERT_NE (channel, nullptr); auto const vote2 = std::make_shared (key1.pub, key1.prv, nano::vote::timestamp_max, nano::vote::duration_max, std::vector{ send1->hash () }); + node1.rep_crawler.add_to_active (send1->hash ()); ASSERT_TIMELY (10s, !node1.rep_crawler.response (channel, vote2)); ASSERT_FALSE (election->confirmed ()); diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index 8c7e8079bd..288bf02bba 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -246,6 +246,13 @@ bool nano::rep_crawler::response (std::shared_ptr cons return error; } +bool nano::rep_crawler::add_to_active (const nano::block_hash & hash_a) +{ + nano::lock_guard lock (active_mutex); + auto result = active.insert (hash_a); + return result.second; +} + nano::uint128_t nano::rep_crawler::total_weight () const { nano::lock_guard lock (probable_reps_mutex); diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index d6ed45f1d8..49913c6384 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -102,6 +102,9 @@ class rep_crawler final */ bool response (std::shared_ptr const &, std::shared_ptr const &); + /** add block hash to the active list of hashes, this should be used only for testing purposes for injecting responses into rep crawler */ + bool add_to_active (const nano::block_hash &); + /** Get total available weight from representatives */ nano::uint128_t total_weight () const; From 248eb871f2eb16bec3d746da8cd23792439eebac Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Wed, 31 Aug 2022 15:10:25 +0100 Subject: [PATCH 2/2] remove add_to_active --- nano/core_test/election.cpp | 4 ++-- nano/node/repcrawler.cpp | 11 ++--------- nano/node/repcrawler.hpp | 8 +++----- 3 files changed, 7 insertions(+), 16 deletions(-) diff --git a/nano/core_test/election.cpp b/nano/core_test/election.cpp index 682bff1d36..0c221f49da 100644 --- a/nano/core_test/election.cpp +++ b/nano/core_test/election.cpp @@ -188,6 +188,7 @@ TEST (election, quorum_minimum_confirm_fail) namespace nano { +// FIXME: this test fails on rare occasions. It needs a review. TEST (election, quorum_minimum_update_weight_before_quorum_checks) { nano::test::system system{}; @@ -243,8 +244,7 @@ TEST (election, quorum_minimum_update_weight_before_quorum_checks) ASSERT_NE (channel, nullptr); auto const vote2 = std::make_shared (key1.pub, key1.prv, nano::vote::timestamp_max, nano::vote::duration_max, std::vector{ send1->hash () }); - node1.rep_crawler.add_to_active (send1->hash ()); - ASSERT_TIMELY (10s, !node1.rep_crawler.response (channel, vote2)); + ASSERT_FALSE (node1.rep_crawler.response (channel, vote2, true)); ASSERT_FALSE (election->confirmed ()); { diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index 288bf02bba..daf7754b92 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -230,13 +230,13 @@ bool nano::rep_crawler::is_pr (nano::transport::channel const & channel_a) const return result; } -bool nano::rep_crawler::response (std::shared_ptr const & channel_a, std::shared_ptr const & vote_a) +bool nano::rep_crawler::response (std::shared_ptr const & channel_a, std::shared_ptr const & vote_a, bool force) { bool error = true; nano::lock_guard lock (active_mutex); for (auto i = vote_a->hashes.begin (), n = vote_a->hashes.end (); i != n; ++i) { - if (active.count (*i) != 0) + if (force || active.count (*i) != 0) { responses.emplace_back (channel_a, vote_a); error = false; @@ -246,13 +246,6 @@ bool nano::rep_crawler::response (std::shared_ptr cons return error; } -bool nano::rep_crawler::add_to_active (const nano::block_hash & hash_a) -{ - nano::lock_guard lock (active_mutex); - auto result = active.insert (hash_a); - return result.second; -} - nano::uint128_t nano::rep_crawler::total_weight () const { nano::lock_guard lock (probable_reps_mutex); diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index 49913c6384..7186c944e9 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -98,12 +98,10 @@ class rep_crawler final /** * Called when a non-replay vote on a block previously sent by query() is received. This indicates * with high probability that the endpoint is a representative node. - * @return false if the vote corresponded to any active hash. + * The force flag can be set to skip the active check in unit testing when we want to force a vote in the rep crawler. + * @return false if any vote passed the checks and was added to the response queue of the rep crawler */ - bool response (std::shared_ptr const &, std::shared_ptr const &); - - /** add block hash to the active list of hashes, this should be used only for testing purposes for injecting responses into rep crawler */ - bool add_to_active (const nano::block_hash &); + bool response (std::shared_ptr const &, std::shared_ptr const &, bool force = false); /** Get total available weight from representatives */ nano::uint128_t total_weight () const;