From 669b2a529ca08baa683aa661793c2b9e2834263c Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Thu, 22 Aug 2024 17:00:04 -0400 Subject: [PATCH 1/8] Start implementing `weak_masking_issue` testcase. --- unittests/savanna_cluster.cpp | 9 ++++- unittests/savanna_cluster.hpp | 61 ++++++++++++++++++++++++++++-- unittests/savanna_misc_tests.cpp | 64 ++++++++++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 6 deletions(-) diff --git a/unittests/savanna_cluster.cpp b/unittests/savanna_cluster.cpp index 428f9a063a..279ba7f585 100644 --- a/unittests/savanna_cluster.cpp +++ b/unittests/savanna_cluster.cpp @@ -14,8 +14,13 @@ node_t::node_t(size_t node_idx, cluster_t& cluster, setup_policy policy /* = set // to vote (and emit the `voted_block` signal) synchronously. // -------------------------------------------------------------------------------------- vote_result_t status = std::get<1>(v); - if (status == vote_result_t::success) - cluster.dispatch_vote_to_peers(node_idx, skip_self_t::yes, std::get<2>(v)); + + if (status == vote_result_t::success) { + vote_message_ptr vote_msg = std::get<2>(v); + last_vote = vote_t(vote_msg); + if (propagate_votes) + cluster.dispatch_vote_to_peers(node_idx, skip_self_t::yes, std::get<2>(v)); + } }; // called on `commit_block`, for both blocks received from `push_block` and produced blocks diff --git a/unittests/savanna_cluster.hpp b/unittests/savanna_cluster.hpp index b135a1ca7e..591f255085 100644 --- a/unittests/savanna_cluster.hpp +++ b/unittests/savanna_cluster.hpp @@ -10,6 +10,43 @@ #include "snapshot_suites.hpp" +// --------------------------------------------------------------------------- +// An object which assigns a value to a variable in its constructor, and resets +// to its previous value in its destructor +// --------------------------------------------------------------------------- +template +class scoped_set_value { +public: + template + [[nodiscard]] scoped_set_value(T& var, V&& val, + bool do_it = true) noexcept(std::is_nothrow_copy_constructible_v && + std::is_nothrow_move_assignable_v) + : v_(var) + , do_it_(do_it) { + if (do_it_) { + old_value_ = std::move(v_); + v_ = std::forward(val); + } + } + + ~scoped_set_value() { + if (do_it_) + v_ = std::move(old_value_); + } + + void dismiss() noexcept { do_it_ = false; } + + scoped_set_value(const scoped_set_value&) = delete; + scoped_set_value& operator=(const scoped_set_value&) = delete; + scoped_set_value(scoped_set_value&&) = delete; + scoped_set_value& operator=(scoped_set_value&&) = delete; + void* operator new(std::size_t) = delete; + + T& v_; + T old_value_; + bool do_it_; +}; + namespace savanna_cluster { namespace ranges = std::ranges; @@ -54,13 +91,30 @@ namespace savanna_cluster { // ---------------------------------------------------------------------------- class node_t : public tester { private: - size_t node_idx; - bool pushing_a_block {false }; + size_t node_idx; + bool pushing_a_block{false}; std::function accepted_block_cb; std::function voted_block_cb; public: + struct vote_t { + vote_t() = default; + vote_t(const vote_message_ptr& p) : id(p->block_id), strong(p->strong) {} + vote_t(const signed_block_ptr& p, bool strong) : id(p->calculate_id()), strong(strong) {} + + friend std::ostream& operator<<(std::ostream& s, const vote_t& v) { + s << "vote_t(" << v.id.str().substr(8, 16) << ", " << (v.strong ? "strong" : "weak") << ")"; + return s; + } + bool operator==(const vote_t&) const = default; + + block_id_type id; + bool strong; + }; + + bool propagate_votes{true}; + vote_t last_vote; std::vector node_finalizers; public: @@ -151,8 +205,7 @@ namespace savanna_cluster { void push_block(const signed_block_ptr& b) { if (is_open() && !fetch_block_by_id(b->calculate_id())) { assert(!pushing_a_block); - pushing_a_block = true; - auto reset_pending_on_exit = fc::make_scoped_exit([this]{ pushing_a_block = false; }); + scoped_set_value set_pushing_a_block(pushing_a_block, true); tester::push_block(b); } } diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index bcc48467d0..4bc8323563 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -5,6 +5,7 @@ using namespace eosio::testing; BOOST_AUTO_TEST_SUITE(savanna_misc_tests) +// ------------------------------------------------------------------------------------ // Verify that we can restart a node from a snapshot without state or blocks (reversible // or not) // ------------------------------------------------------------------------------------ @@ -21,6 +22,7 @@ BOOST_FIXTURE_TEST_CASE(snapshot_startup_without_forkdb, savanna_cluster::cluste } FC_LOG_AND_RETHROW() +// ------------------------------------------------------------------------------------ // Verify that we cannot restart a node from a snapshot without state and blocks log, // but with a fork database // ------------------------------------------------------------------------------------ @@ -39,5 +41,67 @@ BOOST_FIXTURE_TEST_CASE(snapshot_startup_with_forkdb, savanna_cluster::cluster_t } FC_LOG_AND_RETHROW() +// ----------------------------------------------------------------------------------------------------- +// Test case demonstrating the weak masking issue (see https://github.com/AntelopeIO/spring/issues/534) +// Because the issue is fixed in spring https://github.com/AntelopeIO/spring/pull/537, test must pass +// on all versions past that commit. +// ----------------------------------------------------------------------------------------------------- +BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { + auto& A=_nodes[0]; auto& B=_nodes[1]; auto& C=_nodes[2]; auto& D=_nodes[3]; + using vote_t = savanna_cluster::node_t::vote_t; + + auto b0 = A.produce_blocks(2); // receive strong votes from all finalizers + + // partition D out. D will be used to produce blocks on an alternative fork. + // We will have 3 finalizers voting which is enough to reach QCs + // ------------------------------------------------------------------------- + const std::vector partition {3}; + set_partition(partition); + + auto b1 = A.produce_block(); // receives strong votes from 3 finalizers + + auto b2 = D.produce_block(_block_interval_us * 2); // produce a `later` block on D + push_block(0, b2); // push block to other partition, should receive weak votes + BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b2, false)); + BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b2, false)); + + { + scoped_set_value tmp(B.propagate_votes, false); // temporarily prevent B from broadcasting its votes) + // so A won't receive them and form a QC on b3 + + auto b3 = A.produce_block(_block_interval_us * 3); // b3 should receive 2 weak votes from A and C (not a quorum) + // because B doesn't propagate and D is partitioned away + BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b3, false)); + BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b3, false)); + } + + std::cout << "lib=" << B.lib_number << '\n'; + + // Now B broadcasts its votes again, so + auto b4 = A.produce_block(); // b4 should receive 3 weak votes from A, B and C + // and should include a strong QC claim on b1 (repeated) + // since we don't have enough votes to form a QC on b3 + BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b4, false)); + BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b4, false)); + + + std::cout << "lib after b4=" << B.lib_number << '\n'; + auto b5 = A.produce_block(); // a weak QC was formed on b4 and is be included in b5 + // b5 should receive 3 strong votes (because it has a + // weak QC on b4, which itself had a strong QC on b1. + // Upon receiving a strong QC on b5, b1 will be final + BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b5, true)); + BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b5, true)); + + + std::cout << "lib after b5=" << B.lib_number << '\n'; + auto b6 = A.produce_block(); // should include strong QC on b5, b1 should be final + + BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b6, true)); + BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b6, true)); + + BOOST_REQUIRE_EQUAL(B.lib_number, b1->block_num()); + +} FC_LOG_AND_RETHROW() BOOST_AUTO_TEST_SUITE_END() From f6cd0d8f5a39396442fd80102a3bb31e0861706c Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 23 Aug 2024 10:46:09 -0400 Subject: [PATCH 2/8] Progress on `weak_masking_issue` testcase --- unittests/savanna_cluster.hpp | 17 +++++++++++ unittests/savanna_misc_tests.cpp | 51 +++++++++++++++++++++++--------- 2 files changed, 54 insertions(+), 14 deletions(-) diff --git a/unittests/savanna_cluster.hpp b/unittests/savanna_cluster.hpp index 591f255085..14682c4cb9 100644 --- a/unittests/savanna_cluster.hpp +++ b/unittests/savanna_cluster.hpp @@ -508,6 +508,23 @@ namespace savanna_cluster { size_t num_nodes() const { return _num_nodes; } + qc_claim_t qc_claim(const signed_block_ptr& b) { + return b->extract_header_extension().qc_claim; + } + + std::optional qc(const signed_block_ptr& b) { + if (b->contains_extension(quorum_certificate_extension::extension_id())) + return b->extract_extension().qc; + return {}; + } + + // debugging utilities + // ------------------- + static void print(const char* name, const signed_block_ptr& b) { + std::cout << name << " ts = " << b->timestamp.slot << ", id = " << b->calculate_id().str().substr(8, 16) + << ", previous = " << b->previous.str().substr(8, 16) << '\n'; + } + public: std::vector _nodes; fin_keys_t _fin_keys; diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index 4bc8323563..68fabd7658 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -51,6 +51,7 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { using vote_t = savanna_cluster::node_t::vote_t; auto b0 = A.produce_blocks(2); // receive strong votes from all finalizers + print("b0", b0); // partition D out. D will be used to produce blocks on an alternative fork. // We will have 3 finalizers voting which is enough to reach QCs @@ -59,48 +60,70 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { set_partition(partition); auto b1 = A.produce_block(); // receives strong votes from 3 finalizers + print("b1", b1); auto b2 = D.produce_block(_block_interval_us * 2); // produce a `later` block on D - push_block(0, b2); // push block to other partition, should receive weak votes - BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b2, false)); + print("b2", b2); + + BOOST_REQUIRE_GT(b2->timestamp.slot, b1->timestamp.slot); + + const std::vector tmp_partition {0}; // we temporarily separate A + set_partitions({tmp_partition, partition}); // because we don't want A to see the block produced by D (b2) + // otherwise it will switch forks and build its next block + // on top of it + + push_block(1, b2); // push block to B and C, should receive weak votes BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b2, false)); + BOOST_REQUIRE_EQUAL(C.last_vote, vote_t(b2, false)); + BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b1, true));// A should not have seen b2, and therefore not voted on it - { - scoped_set_value tmp(B.propagate_votes, false); // temporarily prevent B from broadcasting its votes) - // so A won't receive them and form a QC on b3 + set_partition(partition); // restore our original partition {A, B, C} and {D} - auto b3 = A.produce_block(_block_interval_us * 3); // b3 should receive 2 weak votes from A and C (not a quorum) - // because B doesn't propagate and D is partitioned away - BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b3, false)); - BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b3, false)); + signed_block_ptr b3; + { + scoped_set_value tmp(B.propagate_votes, false); // temporarily prevent B from broadcasting its votes) + // so A won't receive them and form a QC on b3 + + b3 = A.produce_block(_block_interval_us * 2); // A will see its own strong vote on b3, and C's weak vote + // (not a quorum) + // because B doesn't propagate and D is partitioned away + print("b3", b3); + BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b3, true)); // A didn't vote on b2 so it can vote strong + BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b3, false)); // but B and C have to vote weak. + BOOST_REQUIRE_EQUAL(C.last_vote, vote_t(b3, false)); // C did vote, but we turned vote propagation off so + // A will never see it } - std::cout << "lib=" << B.lib_number << '\n'; + BOOST_REQUIRE_EQUAL(A.lib_number, b0->block_num()); // Now B broadcasts its votes again, so auto b4 = A.produce_block(); // b4 should receive 3 weak votes from A, B and C // and should include a strong QC claim on b1 (repeated) // since we don't have enough votes to form a QC on b3 - BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b4, false)); + print("b4", b4); + BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b4, true)); BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b4, false)); - + BOOST_REQUIRE_EQUAL(C.last_vote, vote_t(b4, false)); + BOOST_REQUIRE_EQUAL(qc_claim(b3), qc_claim(b4)); // A didn't form a QC on b3, so b4 should repeat b3's claim std::cout << "lib after b4=" << B.lib_number << '\n'; auto b5 = A.produce_block(); // a weak QC was formed on b4 and is be included in b5 // b5 should receive 3 strong votes (because it has a // weak QC on b4, which itself had a strong QC on b1. // Upon receiving a strong QC on b5, b1 will be final + print("b5", b5); BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b5, true)); BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b5, true)); + BOOST_REQUIRE_EQUAL(A.lib_number, b0->block_num()); - std::cout << "lib after b5=" << B.lib_number << '\n'; auto b6 = A.produce_block(); // should include strong QC on b5, b1 should be final + print("b6", b6); BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b6, true)); BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b6, true)); - BOOST_REQUIRE_EQUAL(B.lib_number, b1->block_num()); + BOOST_REQUIRE_EQUAL(A.lib_number, b1->block_num()); } FC_LOG_AND_RETHROW() From e483160a0d872a60361705e35ef973ca229beba3 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 23 Aug 2024 11:30:07 -0400 Subject: [PATCH 3/8] Complete `weak_masking_issue` testcase. --- unittests/savanna_cluster.hpp | 32 +++++++++++++++++++++++++------- unittests/savanna_misc_tests.cpp | 15 ++++++++++++--- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/unittests/savanna_cluster.hpp b/unittests/savanna_cluster.hpp index 14682c4cb9..0804fc39dc 100644 --- a/unittests/savanna_cluster.hpp +++ b/unittests/savanna_cluster.hpp @@ -110,7 +110,7 @@ namespace savanna_cluster { bool operator==(const vote_t&) const = default; block_id_type id; - bool strong; + bool strong; }; bool propagate_votes{true}; @@ -456,7 +456,7 @@ namespace savanna_cluster { // returns the number of nodes where `lib` has advanced after executing `f` template - size_t num_lib_advancing(F&& f) { + size_t num_lib_advancing(F&& f) const { std::vector libs(_nodes.size()); for (size_t i=0; i<_nodes.size(); ++i) libs[i] = _nodes[i].lib_num(); @@ -508,11 +508,27 @@ namespace savanna_cluster { size_t num_nodes() const { return _num_nodes; } - qc_claim_t qc_claim(const signed_block_ptr& b) { + // Class for comparisons in BOOST_REQUIRE_EQUAL + // -------------------------------------------- + struct qc_s { + qc_s(const signed_block_ptr& p, bool strong) : block_num(p->block_num()), strong(strong) {} + qc_s(const std::optional& qc) : block_num(qc->block_num), strong(qc->is_strong()) {} + + friend std::ostream& operator<<(std::ostream& s, const qc_s& v) { + s << "qc_s(" << v.block_num << ", " << (v.strong ? "strong" : "weak") << ")"; + return s; + } + bool operator==(const qc_s&) const = default; + + uint32_t block_num; // claimed block + bool strong; + }; + + static qc_claim_t qc_claim(const signed_block_ptr& b) { return b->extract_header_extension().qc_claim; } - std::optional qc(const signed_block_ptr& b) { + static std::optional qc(const signed_block_ptr& b) { if (b->contains_extension(quorum_certificate_extension::extension_id())) return b->extract_extension().qc; return {}; @@ -520,14 +536,16 @@ namespace savanna_cluster { // debugging utilities // ------------------- - static void print(const char* name, const signed_block_ptr& b) { - std::cout << name << " ts = " << b->timestamp.slot << ", id = " << b->calculate_id().str().substr(8, 16) - << ", previous = " << b->previous.str().substr(8, 16) << '\n'; + void print(const char* name, const signed_block_ptr& b) const { + if (_debug_mode) + std::cout << name << " ts = " << b->timestamp.slot << ", id = " << b->calculate_id().str().substr(8, 16) + << ", previous = " << b->previous.str().substr(8, 16) << '\n'; } public: std::vector _nodes; fin_keys_t _fin_keys; + bool _debug_mode{false}; static constexpr fc::microseconds _block_interval_us = fc::milliseconds(eosio::chain::config::block_interval_ms); diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index 68fabd7658..e15e1c242c 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -49,6 +49,7 @@ BOOST_FIXTURE_TEST_CASE(snapshot_startup_with_forkdb, savanna_cluster::cluster_t BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { auto& A=_nodes[0]; auto& B=_nodes[1]; auto& C=_nodes[2]; auto& D=_nodes[3]; using vote_t = savanna_cluster::node_t::vote_t; + //_debug_mode = true; auto b0 = A.produce_blocks(2); // receive strong votes from all finalizers print("b0", b0); @@ -77,6 +78,9 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { BOOST_REQUIRE_EQUAL(C.last_vote, vote_t(b2, false)); BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b1, true));// A should not have seen b2, and therefore not voted on it + BOOST_REQUIRE_EQUAL(qc_s(qc(b2)), qc_s(b0, true)); // b2 should include a strong qc on b0 + + set_partition(partition); // restore our original partition {A, B, C} and {D} signed_block_ptr b3; @@ -92,6 +96,7 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b3, false)); // but B and C have to vote weak. BOOST_REQUIRE_EQUAL(C.last_vote, vote_t(b3, false)); // C did vote, but we turned vote propagation off so // A will never see it + BOOST_REQUIRE_EQUAL(qc_s(qc(b3)), qc_s(b1, true)); // b3 should include a strong qc on b1 } BOOST_REQUIRE_EQUAL(A.lib_number, b0->block_num()); @@ -105,8 +110,10 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b4, false)); BOOST_REQUIRE_EQUAL(C.last_vote, vote_t(b4, false)); BOOST_REQUIRE_EQUAL(qc_claim(b3), qc_claim(b4)); // A didn't form a QC on b3, so b4 should repeat b3's claim + BOOST_REQUIRE(!qc(b4)); // b4 should not have a QC extension + + BOOST_REQUIRE_EQUAL(A.lib_number, b0->block_num()); - std::cout << "lib after b4=" << B.lib_number << '\n'; auto b5 = A.produce_block(); // a weak QC was formed on b4 and is be included in b5 // b5 should receive 3 strong votes (because it has a // weak QC on b4, which itself had a strong QC on b1. @@ -114,16 +121,18 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { print("b5", b5); BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b5, true)); BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b5, true)); + BOOST_REQUIRE_EQUAL(qc_s(qc(b5)), qc_s(b4, false)); // b5 should include a weak qc on b4 BOOST_REQUIRE_EQUAL(A.lib_number, b0->block_num()); - auto b6 = A.produce_block(); // should include strong QC on b5, b1 should be final + auto b6 = A.produce_block(); // should include a strong QC on b5, b1 should be final print("b6", b6); + BOOST_REQUIRE_EQUAL(qc_s(qc(b6)), qc_s(b5, true)); // b6 should include a strong qc on b5 BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b6, true)); BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b6, true)); - BOOST_REQUIRE_EQUAL(A.lib_number, b1->block_num()); + BOOST_REQUIRE_EQUAL(A.lib_number, b4->block_num()); } FC_LOG_AND_RETHROW() From 49c410033cc80b184d7364cec00f6f2b2a40229d Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 23 Aug 2024 12:38:16 -0400 Subject: [PATCH 4/8] Rename member variables. --- unittests/savanna_cluster.hpp | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/unittests/savanna_cluster.hpp b/unittests/savanna_cluster.hpp index 0804fc39dc..a43efe0139 100644 --- a/unittests/savanna_cluster.hpp +++ b/unittests/savanna_cluster.hpp @@ -21,20 +21,20 @@ class scoped_set_value { [[nodiscard]] scoped_set_value(T& var, V&& val, bool do_it = true) noexcept(std::is_nothrow_copy_constructible_v && std::is_nothrow_move_assignable_v) - : v_(var) - , do_it_(do_it) { - if (do_it_) { - old_value_ = std::move(v_); - v_ = std::forward(val); + : _v(var) + , _do_it(do_it) { + if (_do_it) { + _old_value = std::move(_v); + _v = std::forward(val); } } ~scoped_set_value() { - if (do_it_) - v_ = std::move(old_value_); + if (_do_it) + _v = std::move(_old_value); } - void dismiss() noexcept { do_it_ = false; } + void dismiss() noexcept { _do_it = false; } scoped_set_value(const scoped_set_value&) = delete; scoped_set_value& operator=(const scoped_set_value&) = delete; @@ -42,9 +42,9 @@ class scoped_set_value { scoped_set_value& operator=(scoped_set_value&&) = delete; void* operator new(std::size_t) = delete; - T& v_; - T old_value_; - bool do_it_; + T& _v; + T _old_value; + bool _do_it; }; namespace savanna_cluster { From e147d542faddf13ba10e4773cf55250db89c9ed7 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 23 Aug 2024 13:18:54 -0400 Subject: [PATCH 5/8] Fix some typos in comments. --- unittests/savanna_misc_tests.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index e15e1c242c..d689565920 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -51,7 +51,7 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { using vote_t = savanna_cluster::node_t::vote_t; //_debug_mode = true; - auto b0 = A.produce_blocks(2); // receive strong votes from all finalizers + auto b0 = A.produce_blocks(2); // receives strong votes from all finalizers print("b0", b0); // partition D out. D will be used to produce blocks on an alternative fork. @@ -60,7 +60,7 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { const std::vector partition {3}; set_partition(partition); - auto b1 = A.produce_block(); // receives strong votes from 3 finalizers + auto b1 = A.produce_block(); // receives strong votes from 3 finalizers (D partitioned out) print("b1", b1); auto b2 = D.produce_block(_block_interval_us * 2); // produce a `later` block on D @@ -68,9 +68,9 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { BOOST_REQUIRE_GT(b2->timestamp.slot, b1->timestamp.slot); - const std::vector tmp_partition {0}; // we temporarily separate A + const std::vector tmp_partition {0}; // we temporarily separate A (before pushing b2) set_partitions({tmp_partition, partition}); // because we don't want A to see the block produced by D (b2) - // otherwise it will switch forks and build its next block + // otherwise it will switch forks and build its next block (b3) // on top of it push_block(1, b2); // push block to B and C, should receive weak votes @@ -95,7 +95,7 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b3, true)); // A didn't vote on b2 so it can vote strong BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b3, false)); // but B and C have to vote weak. BOOST_REQUIRE_EQUAL(C.last_vote, vote_t(b3, false)); // C did vote, but we turned vote propagation off so - // A will never see it + // A will never see C's vote BOOST_REQUIRE_EQUAL(qc_s(qc(b3)), qc_s(b1, true)); // b3 should include a strong qc on b1 } @@ -110,14 +110,14 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b4, false)); BOOST_REQUIRE_EQUAL(C.last_vote, vote_t(b4, false)); BOOST_REQUIRE_EQUAL(qc_claim(b3), qc_claim(b4)); // A didn't form a QC on b3, so b4 should repeat b3's claim - BOOST_REQUIRE(!qc(b4)); // b4 should not have a QC extension + BOOST_REQUIRE(!qc(b4)); // b4 should not have a QC extension (no new QC formed on b3) BOOST_REQUIRE_EQUAL(A.lib_number, b0->block_num()); - auto b5 = A.produce_block(); // a weak QC was formed on b4 and is be included in b5 + auto b5 = A.produce_block(); // a weak QC was formed on b4 and is included in b5 // b5 should receive 3 strong votes (because it has a // weak QC on b4, which itself had a strong QC on b1. - // Upon receiving a strong QC on b5, b1 will be final + // Upon receiving a strong QC on b5, b4 will be final print("b5", b5); BOOST_REQUIRE_EQUAL(A.last_vote, vote_t(b5, true)); BOOST_REQUIRE_EQUAL(B.last_vote, vote_t(b5, true)); From a65d093a84082d25fa7ae2d34d0d174dec76fdee Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 23 Aug 2024 13:38:39 -0400 Subject: [PATCH 6/8] Add block comment --- unittests/savanna_misc_tests.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index d689565920..2d60fa2112 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -46,6 +46,19 @@ BOOST_FIXTURE_TEST_CASE(snapshot_startup_with_forkdb, savanna_cluster::cluster_t // Because the issue is fixed in spring https://github.com/AntelopeIO/spring/pull/537, test must pass // on all versions past that commit. // ----------------------------------------------------------------------------------------------------- +/* + S + +------------------------------+ + V | + +-----+ S +-----+ S +-----+ no +-----+ W +-----+ S +-----+ +A produces <----| b0 |<-----| b1 |<-----------| b3 |<-------+ b4 |<-----| b5 |<----| b6 |<------- + +-----+ +-----+ +-----+ claim +-----+ +-----+ +-----+ + ^ + | +-----+ +D produces +------| b2 | + S +-----+ + +*/ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { auto& A=_nodes[0]; auto& B=_nodes[1]; auto& C=_nodes[2]; auto& D=_nodes[3]; using vote_t = savanna_cluster::node_t::vote_t; From 20eae6ddc92be287d270ac3827d648512c81147f Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 23 Aug 2024 17:03:20 -0400 Subject: [PATCH 7/8] Fix error in block diagram --- unittests/savanna_misc_tests.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index 2d60fa2112..88facd93ab 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -53,9 +53,9 @@ BOOST_FIXTURE_TEST_CASE(snapshot_startup_with_forkdb, savanna_cluster::cluster_t +-----+ S +-----+ S +-----+ no +-----+ W +-----+ S +-----+ A produces <----| b0 |<-----| b1 |<-----------| b3 |<-------+ b4 |<-----| b5 |<----| b6 |<------- +-----+ +-----+ +-----+ claim +-----+ +-----+ +-----+ - ^ - | +-----+ -D produces +------| b2 | + ^ + | +-----+ +D produces +--------------------| b2 | S +-----+ */ From ee51e5f84f9cf8b90c4dbd133ccb4108fa68baf5 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Sat, 24 Aug 2024 18:05:29 -0400 Subject: [PATCH 8/8] Address PR comments. --- libraries/libfc/include/fc/scoped_exit.hpp | 37 ++++++++++++++++ unittests/savanna_cluster.hpp | 49 +++------------------- unittests/savanna_misc_tests.cpp | 2 +- 3 files changed, 44 insertions(+), 44 deletions(-) diff --git a/libraries/libfc/include/fc/scoped_exit.hpp b/libraries/libfc/include/fc/scoped_exit.hpp index 657bfe6a08..d21b24a2a2 100644 --- a/libraries/libfc/include/fc/scoped_exit.hpp +++ b/libraries/libfc/include/fc/scoped_exit.hpp @@ -36,4 +36,41 @@ namespace fc { return scoped_exit( std::forward(c) ); } + // --------------------------------------------------------------------------- + // An object which assigns a value to a variable in its constructor, and resets + // to its previous value in its destructor + // --------------------------------------------------------------------------- + template + class scoped_set_value { + public: + template + [[nodiscard]] scoped_set_value(T& var, V&& val, + bool do_it = true) noexcept(std::is_nothrow_copy_constructible_v && + std::is_nothrow_move_assignable_v) + : _v(var) + , _do_it(do_it) { + if (_do_it) { + _old_value = std::move(_v); + _v = std::forward(val); + } + } + + ~scoped_set_value() { + if (_do_it) + _v = std::move(_old_value); + } + + void dismiss() noexcept { _do_it = false; } + + scoped_set_value(const scoped_set_value&) = delete; + scoped_set_value& operator=(const scoped_set_value&) = delete; + scoped_set_value(scoped_set_value&&) = delete; + scoped_set_value& operator=(scoped_set_value&&) = delete; + void* operator new(std::size_t) = delete; + + private: + T& _v; + T _old_value; + bool _do_it; + }; } diff --git a/unittests/savanna_cluster.hpp b/unittests/savanna_cluster.hpp index a43efe0139..7c5ac49676 100644 --- a/unittests/savanna_cluster.hpp +++ b/unittests/savanna_cluster.hpp @@ -10,43 +10,6 @@ #include "snapshot_suites.hpp" -// --------------------------------------------------------------------------- -// An object which assigns a value to a variable in its constructor, and resets -// to its previous value in its destructor -// --------------------------------------------------------------------------- -template -class scoped_set_value { -public: - template - [[nodiscard]] scoped_set_value(T& var, V&& val, - bool do_it = true) noexcept(std::is_nothrow_copy_constructible_v && - std::is_nothrow_move_assignable_v) - : _v(var) - , _do_it(do_it) { - if (_do_it) { - _old_value = std::move(_v); - _v = std::forward(val); - } - } - - ~scoped_set_value() { - if (_do_it) - _v = std::move(_old_value); - } - - void dismiss() noexcept { _do_it = false; } - - scoped_set_value(const scoped_set_value&) = delete; - scoped_set_value& operator=(const scoped_set_value&) = delete; - scoped_set_value(scoped_set_value&&) = delete; - scoped_set_value& operator=(scoped_set_value&&) = delete; - void* operator new(std::size_t) = delete; - - T& _v; - T _old_value; - bool _do_it; -}; - namespace savanna_cluster { namespace ranges = std::ranges; @@ -99,9 +62,9 @@ namespace savanna_cluster { public: struct vote_t { - vote_t() = default; - vote_t(const vote_message_ptr& p) : id(p->block_id), strong(p->strong) {} - vote_t(const signed_block_ptr& p, bool strong) : id(p->calculate_id()), strong(strong) {} + vote_t() : strong(false) {} + explicit vote_t(const vote_message_ptr& p) : id(p->block_id), strong(p->strong) {} + explicit vote_t(const signed_block_ptr& p, bool strong) : id(p->calculate_id()), strong(strong) {} friend std::ostream& operator<<(std::ostream& s, const vote_t& v) { s << "vote_t(" << v.id.str().substr(8, 16) << ", " << (v.strong ? "strong" : "weak") << ")"; @@ -205,7 +168,7 @@ namespace savanna_cluster { void push_block(const signed_block_ptr& b) { if (is_open() && !fetch_block_by_id(b->calculate_id())) { assert(!pushing_a_block); - scoped_set_value set_pushing_a_block(pushing_a_block, true); + fc::scoped_set_value set_pushing_a_block(pushing_a_block, true); tester::push_block(b); } } @@ -511,8 +474,8 @@ namespace savanna_cluster { // Class for comparisons in BOOST_REQUIRE_EQUAL // -------------------------------------------- struct qc_s { - qc_s(const signed_block_ptr& p, bool strong) : block_num(p->block_num()), strong(strong) {} - qc_s(const std::optional& qc) : block_num(qc->block_num), strong(qc->is_strong()) {} + explicit qc_s(const signed_block_ptr& p, bool strong) : block_num(p->block_num()), strong(strong) {} + explicit qc_s(const std::optional& qc) : block_num(qc->block_num), strong(qc->is_strong()) {} friend std::ostream& operator<<(std::ostream& s, const qc_s& v) { s << "qc_s(" << v.block_num << ", " << (v.strong ? "strong" : "weak") << ")"; diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index 88facd93ab..11285975bb 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -98,7 +98,7 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { signed_block_ptr b3; { - scoped_set_value tmp(B.propagate_votes, false); // temporarily prevent B from broadcasting its votes) + fc::scoped_set_value tmp(B.propagate_votes, false); // temporarily prevent B from broadcasting its votes) // so A won't receive them and form a QC on b3 b3 = A.produce_block(_block_interval_us * 2); // A will see its own strong vote on b3, and C's weak vote