GH-2102 Move valid_qc into pending_qc and make thread safe

libraries/chain/block_state.cpp

@@ -85,7 +85,6 @@ block_state_ptr block_state::create_if_genesis_block(const block_state_legacy& b
    // TODO: https://github.com/AntelopeIO/leap/issues/2057
    // TODO: Do not aggregate votes on blocks created from block_state_legacy. This can be removed when #2057 complete.
    result.pending_qc = pending_quorum_certificate{result.active_finalizer_policy->finalizers.size(), result.active_finalizer_policy->threshold, result.active_finalizer_policy->max_weak_sum_before_weak_final()};
-   result.valid_qc = {}; // best qc received from the network inside block extension, empty until first savanna proper IF block
 
    // Calculate Merkle tree root in Savanna way so that it is stored in Leaf Node when building block_state.
    const auto& digests = *bsp.action_receipt_digests_savanna;
@@ -252,33 +251,6 @@ void block_state::verify_qc(const valid_quorum_certificate& qc) const {
                invalid_qc_claim, "signature validation failed" );
 }
 
-std::optional<quorum_certificate> block_state::get_best_qc() const {
-   // if pending_qc does not have a valid QC, consider valid_qc only
-   if( !pending_qc.is_quorum_met() ) {
-      if( valid_qc ) {
-         return quorum_certificate{ block_num(), *valid_qc };
-      } else {
-         return std::nullopt;
-      }
-   }
-
-   // extract valid QC from pending_qc
-   valid_quorum_certificate valid_qc_from_pending = pending_qc.to_valid_quorum_certificate();
-
-   // if valid_qc does not have value, consider valid_qc_from_pending only
-   if( !valid_qc ) {
-      return quorum_certificate{ block_num(), valid_qc_from_pending };
-   }
-
-   // Both valid_qc and valid_qc_from_pending have value. Compare them and select a better one.
-   // Strong beats weak. Tie break by valid_qc.
-   const auto& best_qc =
-      valid_qc->is_strong() == valid_qc_from_pending.is_strong() ?
-      *valid_qc : // tie broke by valid_qc
-      valid_qc->is_strong() ? *valid_qc : valid_qc_from_pending; // strong beats weak
-   return quorum_certificate{ block_num(), best_qc };
-}
-
 valid_t block_state::new_valid(const block_header_state& next_bhs, const digest_type& action_mroot, const digest_type& strong_digest) const {
    assert(valid);
    assert(next_bhs.core.last_final_block_num() >= core.last_final_block_num());

libraries/chain/controller.cpp

@@ -3753,7 +3753,7 @@ struct controller_impl {
       return fork_db.apply<std::optional<block_handle>>(unlinkable, f);
    }
 
-   // expected to be called from application thread as it modifies bsp->valid_qc
+   // thread safe
    void integrate_received_qc_to_block(const block_state_ptr& bsp_in) {
       // extract QC from block extension
       const auto& block_exts = bsp_in->block->validate_and_extract_extensions();
@@ -3772,17 +3772,17 @@ struct controller_impl {
          return;
       }
 
-      // Don't save the QC from block extension if the claimed block has a better valid_qc.
-      if (claimed->valid_qc && (claimed->valid_qc->is_strong() || received_qc.is_weak())) {
+      // Don't save the QC from block extension if the claimed block has a better or same valid_qc.
+      if (received_qc.is_weak() || claimed->valid_qc_is_strong()) {
          dlog("qc not better, claimed->valid: ${qbn} ${qid}, strong=${s}, received: ${rqc}, for block ${bn} ${id}",
-              ("qbn", claimed->block_num())("qid", claimed->id())("s", claimed->valid_qc->is_strong())
+              ("qbn", claimed->block_num())("qid", claimed->id())("s", !received_qc.is_weak()) // use is_weak() to avoid mutex on valid_qc_is_strong()
               ("rqc", qc_ext.qc.to_qc_claim())("bn", bsp_in->block_num())("id", bsp_in->id()));
          return;
       }
 
       // Save the QC. This is safe as the function is called by push_block & accept_block from application thread.
       dlog("setting valid qc: ${rqc} into claimed block ${bn} ${id}", ("rqc", qc_ext.qc.to_qc_claim())("bn", claimed->block_num())("id", claimed->id()));
-      claimed->valid_qc = received_qc;
+      claimed->set_valid_qc(received_qc);
 
       // advance LIB if QC is strong
       if( received_qc.is_strong() ) {

libraries/chain/hotstuff/finalizer.cpp

@@ -99,7 +99,7 @@ std::optional<vote_message> finalizer::maybe_vote(const bls_public_key& pub_key,
       } else {
          sig =  priv_key.sign({(uint8_t*)digest.data(), (uint8_t*)digest.data() + digest.data_size()});
       }
-      return vote_message{ bsp->id(), decision == vote_decision::strong_vote, pub_key, sig };
+      return std::optional{vote_message{ bsp->id(), decision == vote_decision::strong_vote, pub_key, sig }};
    }
    return {};
 }

libraries/chain/hotstuff/hotstuff.cpp

@@ -163,10 +163,7 @@ vote_status pending_quorum_certificate::add_vote(block_num_type block_num, bool
    return s;
 }
 
-// thread safe
 valid_quorum_certificate pending_quorum_certificate::to_valid_quorum_certificate() const {
-   std::lock_guard g(*_mtx);
-
    valid_quorum_certificate valid_qc;
 
    if( _state == state_t::strong ) {
@@ -183,6 +180,44 @@ valid_quorum_certificate pending_quorum_certificate::to_valid_quorum_certificate
    return valid_qc;
 }
 
+std::optional<quorum_certificate> pending_quorum_certificate::get_best_qc(block_num_type block_num) const {
+   std::lock_guard g(*_mtx);
+   // if pending_qc does not have a valid QC, consider valid_qc only
+   if( !is_quorum_met_no_lock() ) {
+      if( _valid_qc ) {
+         return std::optional{quorum_certificate{ block_num, *_valid_qc }};
+      } else {
+         return std::nullopt;
+      }
+   }
+
+   // extract valid QC from pending_qc
+   valid_quorum_certificate valid_qc_from_pending = to_valid_quorum_certificate();
+
+   // if valid_qc does not have value, consider valid_qc_from_pending only
+   if( !_valid_qc ) {
+      return std::optional{quorum_certificate{ block_num, valid_qc_from_pending }};
+   }
+
+   // Both valid_qc and valid_qc_from_pending have value. Compare them and select a better one.
+   // Strong beats weak. Tie break by valid_qc.
+   const auto& best_qc =
+      _valid_qc->is_strong() == valid_qc_from_pending.is_strong() ?
+      *_valid_qc : // tie broke by valid_qc
+      _valid_qc->is_strong() ? *_valid_qc : valid_qc_from_pending; // strong beats weak
+   return std::optional{quorum_certificate{ block_num, best_qc }};
+}
+
+void pending_quorum_certificate::set_valid_qc(const valid_quorum_certificate& qc) {
+   std::lock_guard g(*_mtx);
+   _valid_qc = qc;
+}
+
+bool pending_quorum_certificate::valid_qc_is_strong() const {
+   std::lock_guard g(*_mtx);
+   return _valid_qc && _valid_qc->is_strong();
+}
+
 bool pending_quorum_certificate::is_quorum_met_no_lock() const {
    return is_quorum_met(_state);
 }

libraries/chain/include/eosio/chain/block_state.hpp

@@ -69,7 +69,6 @@ struct block_state : public block_header_state {     // block_header_state provi
    digest_type                strong_digest;         // finalizer_digest (strong, cached so we can quickly validate votes)
    weak_digest_t              weak_digest;           // finalizer_digest (weak, cached so we can quickly validate votes)
    pending_quorum_certificate pending_qc;            // where we accumulate votes we receive
-   std::optional<valid_quorum_certificate> valid_qc; // best qc received from the network inside block extension
    std::optional<valid_t>     valid;
 
    // ------ updated for votes, used for fork_db ordering ------------------------------
@@ -103,7 +102,9 @@ struct block_state : public block_header_state {     // block_header_state provi
    const extensions_type& header_extensions() const { return block_header_state::header.header_extensions; }
    uint32_t               irreversible_blocknum() const { return core.last_final_block_num(); } // backwards compatibility
    uint32_t               last_final_block_num() const { return core.last_final_block_num(); }
-   std::optional<quorum_certificate> get_best_qc() const;
+   std::optional<quorum_certificate> get_best_qc() const { return pending_qc.get_best_qc(block_num()); } // thread safe
+   bool valid_qc_is_strong() const { return pending_qc.valid_qc_is_strong(); } // thread safe
+   void set_valid_qc(const valid_quorum_certificate& qc) { pending_qc.set_valid_qc(qc); }
 
    protocol_feature_activation_set_ptr get_activated_protocol_features() const { return block_header_state::activated_protocol_features; }
    uint32_t               last_qc_block_num() const { return core.latest_qc_claim().block_num; }
@@ -164,4 +165,4 @@ using block_state_pair      = std::pair<std::shared_ptr<block_state_legacy>, blo
 FC_REFLECT( eosio::chain::valid_t::finality_leaf_node_t, (major_version)(minor_version)(block_num)(finality_digest)(action_mroot) )
 FC_REFLECT( eosio::chain::valid_t, (validation_tree)(validation_mroots))
 FC_REFLECT( eosio::chain::finality_data_t, (major_version)(minor_version)(active_finalizer_policy_generation)(action_mroot)(base_digest))
-FC_REFLECT_DERIVED( eosio::chain::block_state, (eosio::chain::block_header_state), (block)(strong_digest)(weak_digest)(pending_qc)(valid_qc)(valid)(validated) )
+FC_REFLECT_DERIVED( eosio::chain::block_state, (eosio::chain::block_header_state), (block)(strong_digest)(weak_digest)(pending_qc)(valid)(validated) )

libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp

@@ -124,12 +124,15 @@ namespace eosio::chain {
       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;
 
+      std::optional<quorum_certificate> get_best_qc(block_num_type block_num) const;
+      void set_valid_qc(const valid_quorum_certificate& qc);
+      bool valid_qc_is_strong() const;
    private:
       friend struct fc::reflector<pending_quorum_certificate>;
       friend class qc_chain;
       std::unique_ptr<std::mutex> _mtx;
+      std::optional<valid_quorum_certificate> _valid_qc; // best qc received from the network inside block extension
       uint64_t             _quorum {0};
       uint64_t             _max_weak_sum_before_weak_final {0}; // max weak sum before becoming weak_final
       state_t              _state { state_t::unrestricted };
@@ -150,14 +153,15 @@ namespace eosio::chain {
 
       bool is_quorum_met_no_lock() const;
       bool has_voted_no_lock(bool strong, size_t index) const;
+      valid_quorum_certificate to_valid_quorum_certificate() const;
    };
 } //eosio::chain
 
 
 FC_REFLECT(eosio::chain::vote_message, (block_id)(strong)(finalizer_key)(sig));
 FC_REFLECT_ENUM(eosio::chain::vote_status, (success)(duplicate)(unknown_public_key)(invalid_signature)(unknown_block))
 FC_REFLECT(eosio::chain::valid_quorum_certificate, (_strong_votes)(_weak_votes)(_sig));
-FC_REFLECT(eosio::chain::pending_quorum_certificate, (_quorum)(_max_weak_sum_before_weak_final)(_state)(_strong_sum)(_weak_sum)(_weak_votes)(_strong_votes));
+FC_REFLECT(eosio::chain::pending_quorum_certificate, (_valid_qc)(_quorum)(_max_weak_sum_before_weak_final)(_state)(_strong_sum)(_weak_sum)(_weak_votes)(_strong_votes));
 FC_REFLECT_ENUM(eosio::chain::pending_quorum_certificate::state_t, (unrestricted)(restricted)(weak_achieved)(weak_final)(strong));
 FC_REFLECT(eosio::chain::pending_quorum_certificate::votes_t, (_bitset)(_sig));
 FC_REFLECT(eosio::chain::quorum_certificate, (block_num)(qc));