From 1ced859e5429ee2a85cc2dea1a66c36346c38ad3 Mon Sep 17 00:00:00 2001
From: Kevin Heifner <heifnerk@objectcomputing.com>
Date: Fri, 29 Mar 2024 12:36:19 -0500
Subject: [PATCH 1/5] Verify bls signature of vote while not holding a mutex

---
 libraries/chain/hotstuff/hotstuff.cpp         | 59 ++++++++++++-------
 .../include/eosio/chain/hotstuff/hotstuff.hpp | 12 ++--
 2 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/libraries/chain/hotstuff/hotstuff.cpp b/libraries/chain/hotstuff/hotstuff.cpp
index c4b4c454a3..c3cf06656c 100644
--- a/libraries/chain/hotstuff/hotstuff.cpp
+++ b/libraries/chain/hotstuff/hotstuff.cpp
@@ -20,17 +20,19 @@ inline std::vector<uint32_t> bitset_to_vector(const hs_bitset& bs) {
    return r;
 }
 
-vote_status pending_quorum_certificate::votes_t::add_vote(std::span<const uint8_t> proposal_digest, size_t index,
-                                                          const bls_public_key& pubkey, const bls_signature& new_sig) {
-   if (_bitset[index]) {
-      return vote_status::duplicate; // shouldn't be already present
+bool pending_quorum_certificate::is_duplicate_no_lock(bool strong, size_t index) const {
+   if (strong) {
+      return _strong_votes._bitset[index];
    }
-   if (!fc::crypto::blslib::verify(pubkey, proposal_digest, new_sig)) {
-      wlog( "signature from finalizer ${i} cannot be verified", ("i", index) );
-      return vote_status::invalid_signature;
+   return _weak_votes._bitset[index];
+}
+
+vote_status pending_quorum_certificate::votes_t::add_vote(size_t index, const bls_signature& sig) {
+   if (_bitset[index]) { // check here as could have come in while unlocked
+      return vote_status::duplicate; // shouldn't be already present
    }
    _bitset.set(index);
-   _sig.aggregate(new_sig); // works even if _sig is default initialized (fp2::zero())
+   _sig.aggregate(sig); // works even if _sig is default initialized (fp2::zero())
    return vote_status::success;
 }
 
@@ -59,10 +61,8 @@ bool pending_quorum_certificate::is_quorum_met() const {
 }
 
 // called by add_vote, already protected by mutex
-vote_status pending_quorum_certificate::add_strong_vote(std::span<const uint8_t> proposal_digest, size_t index,
-                                                        const bls_public_key& pubkey, const bls_signature& sig,
-                                                        uint64_t weight) {
-   if (auto s = _strong_votes.add_vote(proposal_digest, index, pubkey, sig); s != vote_status::success) {
+vote_status pending_quorum_certificate::add_strong_vote(size_t index, const bls_signature& sig, uint64_t weight) {
+   if (auto s = _strong_votes.add_vote(index, sig); s != vote_status::success) {
       return s;
    }
    _strong_sum += weight;
@@ -91,10 +91,8 @@ vote_status pending_quorum_certificate::add_strong_vote(std::span<const uint8_t>
 }
 
 // called by add_vote, already protected by mutex
-vote_status pending_quorum_certificate::add_weak_vote(std::span<const uint8_t> proposal_digest, size_t index,
-                                                      const bls_public_key& pubkey, const bls_signature& sig,
-                                                      uint64_t weight) {
-   if (auto s = _weak_votes.add_vote(proposal_digest, index, pubkey, sig); s != vote_status::success)
+vote_status pending_quorum_certificate::add_weak_vote(size_t index, const bls_signature& sig, uint64_t weight) {
+   if (auto s = _weak_votes.add_vote(index, sig); s != vote_status::success)
       return s;
    _weak_sum += weight;
 
@@ -125,15 +123,32 @@ vote_status pending_quorum_certificate::add_weak_vote(std::span<const uint8_t> p
    return vote_status::success;
 }
 
-// thread safe, status, pre state                          , post state
+// thread safe
 vote_status pending_quorum_certificate::add_vote(block_num_type block_num, bool strong, std::span<const uint8_t> proposal_digest, size_t index,
                                                  const bls_public_key& pubkey, const bls_signature& sig, uint64_t weight) {
-   std::lock_guard g(*_mtx);
-   auto pre_state = _state;
-   vote_status s = strong ? add_strong_vote(proposal_digest, index, pubkey, sig, weight)
-                          : add_weak_vote(proposal_digest, index, pubkey, sig, weight);
+   vote_status s = vote_status::success;
+
+   std::unique_lock g(*_mtx);
+   state_t pre_state = _state;
+   state_t post_state = pre_state;
+   if (is_duplicate_no_lock(strong, index)) {
+      s = vote_status::duplicate;
+   } else {
+      g.unlock();
+      if (!fc::crypto::blslib::verify(pubkey, proposal_digest, sig)) {
+         wlog( "signature from finalizer ${i} cannot be verified", ("i", index) );
+         s = vote_status::invalid_signature;
+      } else {
+         g.lock();
+         s = strong ? add_strong_vote(index, sig, weight)
+                    : add_weak_vote(index, sig, weight);
+         post_state = _state;
+         g.unlock();
+      }
+   }
+
    dlog("block_num: ${bn}, vote strong: ${sv}, status: ${s}, pre-state: ${pre}, post-state: ${state}, quorum_met: ${q}",
-        ("bn", block_num)("sv", strong)("s", s)("pre", pre_state)("state", _state)("q", is_quorum_met_no_lock()));
+        ("bn", block_num)("sv", strong)("s", s)("pre", pre_state)("state", post_state)("q", is_quorum_met(post_state)));
    return s;
 }
 
diff --git a/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp b/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp
index 72c6cf2404..f371c7e5a0 100644
--- a/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp
+++ b/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp
@@ -85,8 +85,7 @@ namespace eosio::chain {
          void resize(size_t num_finalizers) { _bitset.resize(num_finalizers); }
          size_t count() const { return _bitset.count(); }
 
-         vote_status add_vote(std::span<const uint8_t> proposal_digest, size_t index, const bls_public_key& pubkey,
-                              const bls_signature& new_sig);
+         vote_status add_vote(size_t index, const bls_signature& sig);
 
          void reset(size_t num_finalizers);
       };
@@ -126,20 +125,17 @@ namespace eosio::chain {
       votes_t              _strong_votes;
 
       // called by add_vote, already protected by mutex
-      vote_status add_strong_vote(std::span<const uint8_t> proposal_digest,
-                                  size_t index,
-                                  const bls_public_key& pubkey,
+      vote_status add_strong_vote(size_t index,
                                   const bls_signature& sig,
                                   uint64_t weight);
 
       // called by add_vote, already protected by mutex
-      vote_status add_weak_vote(std::span<const uint8_t> proposal_digest,
-                                size_t index,
-                                const bls_public_key& pubkey,
+      vote_status add_weak_vote(size_t index,
                                 const bls_signature& sig,
                                 uint64_t weight);
 
       bool is_quorum_met_no_lock() const;
+      bool is_duplicate_no_lock(bool strong, size_t index) const;
    };
 } //eosio::chain
 

From ceec40b4d5594dff7c33da5561f84b180abb4c1f Mon Sep 17 00:00:00 2001
From: Kevin Heifner <heifnerk@objectcomputing.com>
Date: Fri, 29 Mar 2024 14:53:47 -0500
Subject: [PATCH 2/5] Add has_voted() method

---
 libraries/chain/block_state.cpp                     | 13 +++++++++++++
 libraries/chain/hotstuff/hotstuff.cpp               |  9 +++++++--
 libraries/chain/include/eosio/chain/block_state.hpp |  1 +
 .../chain/include/eosio/chain/hotstuff/hotstuff.hpp |  5 ++++-
 4 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/libraries/chain/block_state.cpp b/libraries/chain/block_state.cpp
index e6ee9ce2ef..649fa62df8 100644
--- a/libraries/chain/block_state.cpp
+++ b/libraries/chain/block_state.cpp
@@ -158,6 +158,19 @@ vote_status block_state::aggregate_vote(const vote_message& vote) {
    }
 }
 
+bool block_state::has_voted(const bls_public_key& key) const {
+   const auto& finalizers = active_finalizer_policy->finalizers;
+   auto it = std::find_if(finalizers.begin(),
+                          finalizers.end(),
+                          [&](const auto& finalizer) { return finalizer.public_key == key; });
+
+   if (it != finalizers.end()) {
+      auto index = std::distance(finalizers.begin(), it);
+      return pending_qc.has_voted(index);
+   }
+   return false;
+}
+
 // Called from net threads
 void block_state::verify_qc(const valid_quorum_certificate& qc) const {
    const auto& finalizers = active_finalizer_policy->finalizers;
diff --git a/libraries/chain/hotstuff/hotstuff.cpp b/libraries/chain/hotstuff/hotstuff.cpp
index c3cf06656c..a0839affb8 100644
--- a/libraries/chain/hotstuff/hotstuff.cpp
+++ b/libraries/chain/hotstuff/hotstuff.cpp
@@ -20,7 +20,12 @@ inline std::vector<uint32_t> bitset_to_vector(const hs_bitset& bs) {
    return r;
 }
 
-bool pending_quorum_certificate::is_duplicate_no_lock(bool strong, size_t index) const {
+bool pending_quorum_certificate::has_voted(size_t index) const {
+   std::lock_guard g(*_mtx);
+   return _strong_votes._bitset.at(index) || _weak_votes._bitset.at(index);
+}
+
+bool pending_quorum_certificate::has_voted_no_lock(bool strong, size_t index) const {
    if (strong) {
       return _strong_votes._bitset[index];
    }
@@ -131,7 +136,7 @@ vote_status pending_quorum_certificate::add_vote(block_num_type block_num, bool
    std::unique_lock g(*_mtx);
    state_t pre_state = _state;
    state_t post_state = pre_state;
-   if (is_duplicate_no_lock(strong, index)) {
+   if (has_voted_no_lock(strong, index)) {
       s = vote_status::duplicate;
    } else {
       g.unlock();
diff --git a/libraries/chain/include/eosio/chain/block_state.hpp b/libraries/chain/include/eosio/chain/block_state.hpp
index 05438c56fc..6b8826dd6d 100644
--- a/libraries/chain/include/eosio/chain/block_state.hpp
+++ b/libraries/chain/include/eosio/chain/block_state.hpp
@@ -124,6 +124,7 @@ struct block_state : public block_header_state {     // block_header_state provi
 
    // vote_status
    vote_status aggregate_vote(const vote_message& vote); // aggregate vote into pending_qc
+   bool has_voted(const bls_public_key& key) const;
    void verify_qc(const valid_quorum_certificate& qc) const; // verify given qc is valid with respect block_state
 
    using bhs_t  = block_header_state;
diff --git a/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp b/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp
index f371c7e5a0..a98cbd023a 100644
--- a/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp
+++ b/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp
@@ -109,6 +109,9 @@ namespace eosio::chain {
                            const bls_signature& sig,
                            uint64_t weight);
 
+      // thread safe
+      bool has_voted(size_t index) const;
+
       state_t state() const { std::lock_guard g(*_mtx); return _state; };
       valid_quorum_certificate to_valid_quorum_certificate() const;
 
@@ -135,7 +138,7 @@ namespace eosio::chain {
                                 uint64_t weight);
 
       bool is_quorum_met_no_lock() const;
-      bool is_duplicate_no_lock(bool strong, size_t index) const;
+      bool has_voted_no_lock(bool strong, size_t index) const;
    };
 } //eosio::chain
 

From be4c877bed04b73c6aefb0e002940cee1b1b4fc3 Mon Sep 17 00:00:00 2001
From: Kevin Heifner <heifnerk@objectcomputing.com>
Date: Fri, 29 Mar 2024 14:54:12 -0500
Subject: [PATCH 3/5] Wait for controller to vote if it should

---
 libraries/chain/controller.cpp                | 22 +++++++++++++++++++
 .../chain/include/eosio/chain/controller.hpp  |  2 ++
 libraries/testing/tester.cpp                  |  9 ++++++++
 3 files changed, 33 insertions(+)

diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp
index df882f3f68..32d711aeb8 100644
--- a/libraries/chain/controller.cpp
+++ b/libraries/chain/controller.cpp
@@ -3491,6 +3491,24 @@ struct controller_impl {
       return fork_db.apply<vote_status>(aggregate_vote_legacy, aggregate_vote);
    }
 
+   bool node_has_voted_if_finalizer(const block_id_type& id) const {
+      if (my_finalizers.finalizers.empty())
+         return true;
+
+      std::optional<bool> voted = fork_db.apply_s<std::optional<bool>>([&](auto& forkdb) -> std::optional<bool> {
+         auto bsp = forkdb.get_block(id);
+         if (bsp) {
+            for (auto& f : my_finalizers.finalizers) {
+               if (bsp->has_voted(f.first))
+                  return {true};
+            }
+         }
+         return {false};
+      });
+      // empty optional means legacy forkdb
+      return !voted || *voted;
+   }
+
    void create_and_send_vote_msg(const block_state_ptr& bsp) {
       if (!bsp->block->is_proper_svnn_block())
          return;
@@ -5068,6 +5086,10 @@ vote_status controller::process_vote_message( const vote_message& vote ) {
    return my->process_vote_message( vote );
 };
 
+bool controller::node_has_voted_if_finalizer(const block_id_type& id) const {
+   return my->node_has_voted_if_finalizer(id);
+}
+
 const producer_authority_schedule& controller::active_producers()const {
    return my->active_producers();
 }
diff --git a/libraries/chain/include/eosio/chain/controller.hpp b/libraries/chain/include/eosio/chain/controller.hpp
index c1bc7a8d1b..0eeed40015 100644
--- a/libraries/chain/include/eosio/chain/controller.hpp
+++ b/libraries/chain/include/eosio/chain/controller.hpp
@@ -326,6 +326,8 @@ namespace eosio::chain {
          void set_proposed_finalizers( const finalizer_policy& fin_set );
          // called from net threads
          vote_status process_vote_message( const vote_message& msg );
+         // thread safe, for testing
+         bool node_has_voted_if_finalizer(const block_id_type& id) const;
 
          bool light_validation_allowed() const;
          bool skip_auth_check()const;
diff --git a/libraries/testing/tester.cpp b/libraries/testing/tester.cpp
index c4a3771bdf..7f3a024739 100644
--- a/libraries/testing/tester.cpp
+++ b/libraries/testing/tester.cpp
@@ -485,6 +485,15 @@ namespace eosio { namespace testing {
       control->commit_block();
       last_produced_block[producer_name] = control->head_block_id();
 
+      if (control->head_block()->is_proper_svnn_block()) {
+         // wait for this node's vote to be processed
+         size_t retrys = 200;
+         while (!control->node_has_voted_if_finalizer(control->head_block_id()) && --retrys) {
+            std::this_thread::sleep_for(std::chrono::milliseconds(1));
+         }
+         FC_ASSERT(retrys, "Never saw this nodes vote processed before timeout");
+      }
+
       return control->head_block();
    }
 

From 67c4f1db6c4d0a7954b213363075fe5ae8648a5d Mon Sep 17 00:00:00 2001
From: Kevin Heifner <heifnerk@objectcomputing.com>
Date: Fri, 29 Mar 2024 15:39:06 -0500
Subject: [PATCH 4/5] Verify all have voted

---
 libraries/chain/controller.cpp | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp
index 32d711aeb8..b5b3361dec 100644
--- a/libraries/chain/controller.cpp
+++ b/libraries/chain/controller.cpp
@@ -3498,12 +3498,11 @@ struct controller_impl {
       std::optional<bool> voted = fork_db.apply_s<std::optional<bool>>([&](auto& forkdb) -> std::optional<bool> {
          auto bsp = forkdb.get_block(id);
          if (bsp) {
-            for (auto& f : my_finalizers.finalizers) {
-               if (bsp->has_voted(f.first))
-                  return {true};
-            }
+            return std::ranges::all_of(my_finalizers.finalizers, [&bsp](auto& f) {
+               return bsp->has_voted(f.first);
+            });
          }
-         return {false};
+         return false;
       });
       // empty optional means legacy forkdb
       return !voted || *voted;

From a241785d7413de2792062e4e651ea034cc36d89c Mon Sep 17 00:00:00 2001
From: Kevin Heifner <heifnerk@objectcomputing.com>
Date: Fri, 29 Mar 2024 15:39:30 -0500
Subject: [PATCH 5/5] Make sure validating node has voted

---
 libraries/testing/include/eosio/testing/tester.hpp | 12 ++++--------
 libraries/testing/tester.cpp                       | 12 ++++++++----
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/libraries/testing/include/eosio/testing/tester.hpp b/libraries/testing/include/eosio/testing/tester.hpp
index 22bf1666ce..ee2862d405 100644
--- a/libraries/testing/include/eosio/testing/tester.hpp
+++ b/libraries/testing/include/eosio/testing/tester.hpp
@@ -458,6 +458,7 @@ namespace eosio { namespace testing {
 
          void             _start_block(fc::time_point block_time);
          signed_block_ptr _finish_block();
+         void             _wait_for_vote_if_needed(controller& c);
 
       // Fields:
       protected:
@@ -626,10 +627,7 @@ namespace eosio { namespace testing {
 
       signed_block_ptr produce_block( fc::microseconds skip_time = fc::milliseconds(config::block_interval_ms) )override {
          auto sb = _produce_block(skip_time, false);
-         auto btf = validating_node->create_block_handle_future( sb->calculate_id(), sb );
-         controller::block_report br;
-         validating_node->push_block( br, btf.get(), {}, trx_meta_cache_lookup{} );
-
+         validate_push_block(sb);
          return sb;
       }
 
@@ -641,15 +639,13 @@ namespace eosio { namespace testing {
          auto btf = validating_node->create_block_handle_future( sb->calculate_id(), sb );
          controller::block_report br;
          validating_node->push_block( br, btf.get(), {}, trx_meta_cache_lookup{} );
+         _wait_for_vote_if_needed(*validating_node);
       }
 
       signed_block_ptr produce_empty_block( fc::microseconds skip_time = fc::milliseconds(config::block_interval_ms) )override {
          unapplied_transactions.add_aborted( control->abort_block() );
          auto sb = _produce_block(skip_time, true);
-         auto btf = validating_node->create_block_handle_future( sb->calculate_id(), sb );
-         controller::block_report br;
-         validating_node->push_block( br, btf.get(), {}, trx_meta_cache_lookup{} );
-
+         validate_push_block(sb);
          return sb;
       }
 
diff --git a/libraries/testing/tester.cpp b/libraries/testing/tester.cpp
index 7f3a024739..7e1efb0177 100644
--- a/libraries/testing/tester.cpp
+++ b/libraries/testing/tester.cpp
@@ -485,16 +485,20 @@ namespace eosio { namespace testing {
       control->commit_block();
       last_produced_block[producer_name] = control->head_block_id();
 
-      if (control->head_block()->is_proper_svnn_block()) {
+      _wait_for_vote_if_needed(*control);
+
+      return control->head_block();
+   }
+
+   void base_tester::_wait_for_vote_if_needed(controller& c) {
+      if (c.head_block()->is_proper_svnn_block()) {
          // wait for this node's vote to be processed
          size_t retrys = 200;
-         while (!control->node_has_voted_if_finalizer(control->head_block_id()) && --retrys) {
+         while (!c.node_has_voted_if_finalizer(c.head_block_id()) && --retrys) {
             std::this_thread::sleep_for(std::chrono::milliseconds(1));
          }
          FC_ASSERT(retrys, "Never saw this nodes vote processed before timeout");
       }
-
-      return control->head_block();
    }
 
    signed_block_ptr base_tester::produce_block( std::vector<transaction_trace_ptr>& traces ) {