Merge pull request #2359 from AntelopeIO/verify-bls-sig-unlocked

IF: Verify bls signature of vote while not holding a mutex

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;

libraries/chain/controller.cpp

@@ -3493,6 +3493,23 @@ 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) {
+            return std::ranges::all_of(my_finalizers.finalizers, [&bsp](auto& f) {
+               return bsp->has_voted(f.first);
+            });
+         }
+         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;
@@ -5079,6 +5096,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();
 }

libraries/chain/hotstuff/hotstuff.cpp

@@ -20,17 +20,24 @@ 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::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];
    }
-   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 +66,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 +96,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 +128,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 (has_voted_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;
 }
 

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;

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;

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);
       };
@@ -110,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;
 
@@ -126,20 +128,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 has_voted_no_lock(bool strong, size_t index) const;
    };
 } //eosio::chain
 

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;
       }
 

libraries/testing/tester.cpp

@@ -485,9 +485,22 @@ namespace eosio { namespace testing {
       control->commit_block();
       last_produced_block[producer_name] = control->head_block_id();
 
+      _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 (!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");
+      }
+   }
+
    signed_block_ptr base_tester::produce_block( std::vector<transaction_trace_ptr>& traces ) {
       return _produce_block( fc::milliseconds(config::block_interval_ms), false, true, traces );
    }