[1.0] Add test case to demonstrate the weak masking issue

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

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 T>
+class scoped_set_value {
+public:
+   template <class V>
+   [[nodiscard]] scoped_set_value(T& var, V&& val,
+                                  bool do_it = true) noexcept(std::is_nothrow_copy_constructible_v<T> &&
+                                                              std::is_nothrow_move_assignable_v<T>)
+      : _v(var)
+      , _do_it(do_it) {
+      if (_do_it) {
+         _old_value = std::move(_v);
+         _v         = std::forward<V>(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<void(const block_signal_params&)> accepted_block_cb;
       std::function<void(const vote_signal_params&)>  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<account_name> 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);
          }
       }
@@ -403,7 +456,7 @@ namespace savanna_cluster {
 
       // returns the number of nodes where `lib` has advanced after executing `f`
       template<class F>
-      size_t num_lib_advancing(F&& f) {
+      size_t num_lib_advancing(F&& f) const {
          std::vector<uint32_t> libs(_nodes.size());
          for (size_t i=0; i<_nodes.size(); ++i)
             libs[i] = _nodes[i].lib_num();
@@ -455,9 +508,44 @@ namespace savanna_cluster {
 
       size_t num_nodes() const { return _num_nodes; }
 
+      // 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_t>& 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<finality_extension>().qc_claim;
+      }
+
+      static std::optional<qc_t> qc(const signed_block_ptr& b) {
+         if (b->contains_extension(quorum_certificate_extension::extension_id()))
+             return b->extract_extension<quorum_certificate_extension>().qc;
+         return {};
+      }
+
+      // debugging utilities
+      // -------------------
+      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<node_t>  _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);

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,112 @@ 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.
+// -----------------------------------------------------------------------------------------------------
+/*
+                                               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;
+   //_debug_mode = true;
+
+   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.
+   // We will have 3 finalizers voting which is enough to reach QCs
+   // -------------------------------------------------------------------------
+   const std::vector<size_t> partition {3};
+   set_partition(partition);
+
+   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
+   print("b2", b2);
+
+   BOOST_REQUIRE_GT(b2->timestamp.slot, b1->timestamp.slot);
+
+   const std::vector<size_t> 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 (b3)
+                                                      // 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
+
+   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;
+   {
+      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 C's vote
+      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());
+
+                                                      // 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
+   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
+   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 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, 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));
+   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 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, b4->block_num());
+
+} FC_LOG_AND_RETHROW()
 
 BOOST_AUTO_TEST_SUITE_END()