Merge pull request #1550 from AntelopeIO/hotstuff_1541_getstate_cache

IF: Fix threading 1/2 (#1541) & qc_chain private:

#1541 should not be closed by this. The `_chain` pointer net vs main thread controller access will be dealt with by another PR.

Using the Github "Merge pull request" button, which should do the right thing.

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

@@ -68,6 +68,13 @@ namespace eosio::chain {
       eosio::chain::quorum_certificate current_qc;
       eosio::chain::extended_schedule schedule;
       map<fc::sha256, hs_proposal_message> proposals;
+
+      const hs_proposal_message* get_proposal(const fc::sha256& id) const {
+         auto it = proposals.find(id);
+         if (it == proposals.end())
+            return nullptr;
+         return & it->second;
+      }
    };
 
    using hs_proposal_message_ptr = std::shared_ptr<hs_proposal_message>;

libraries/hotstuff/chain_pacemaker.cpp

@@ -112,9 +112,32 @@ namespace eosio { namespace hotstuff {
       return _chain->is_builtin_activated( builtin_protocol_feature_t::instant_finality );
    }
 
-   void chain_pacemaker::get_state( finalizer_state& fs ) const {
-      if (enabled())
-         _qc_chain.get_state( fs ); // get_state() takes scare of finer-grained synchronization internally
+   void chain_pacemaker::get_state(finalizer_state& fs) const {
+      if (! enabled())
+         return;
+
+      // lock-free state version check
+      uint64_t current_state_version = _qc_chain.get_state_version();
+      if (_state_cache_version != current_state_version) {
+         finalizer_state current_state;
+         {
+            csc prof("stat");
+            std::lock_guard g( _hotstuff_global_mutex ); // lock IF engine to read state
+            prof.core_in();
+            current_state_version = _qc_chain.get_state_version(); // get potentially fresher version
+            if (_state_cache_version != current_state_version) 
+               _qc_chain.get_state(current_state);
+            prof.core_out();
+         }
+         if (_state_cache_version != current_state_version) {
+            std::unique_lock ul(_state_cache_mutex); // lock cache for writing
+            _state_cache = current_state;
+            _state_cache_version = current_state_version;
+         }
+      }
+
+      std::shared_lock sl(_state_cache_mutex); // lock cache for reading
+      fs = _state_cache;
    }
 
    name chain_pacemaker::get_proposer() {

libraries/hotstuff/include/eosio/hotstuff/chain_pacemaker.hpp

@@ -3,6 +3,8 @@
 #include <eosio/hotstuff/base_pacemaker.hpp>
 #include <eosio/hotstuff/qc_chain.hpp>
 
+#include <shared_mutex>
+
 namespace eosio::chain {
    class controller;
 }
@@ -23,7 +25,7 @@ namespace eosio::hotstuff {
       void on_hs_new_view_msg(const hs_new_view_message& msg); //new view msg event handler
       void on_hs_new_block_msg(const hs_new_block_message& msg); //new block msg event handler
 
-      void get_state( finalizer_state& fs ) const;
+      void get_state(finalizer_state& fs) const;
 
       //base_pacemaker interface functions
 
@@ -54,7 +56,13 @@ namespace eosio::hotstuff {
       // These requests can come directly from the net threads, or indirectly from a
       //   dedicated finalizer thread (TODO: discuss).
 #warning discuss
-      std::mutex              _hotstuff_global_mutex;
+      mutable std::mutex      _hotstuff_global_mutex;
+
+      // _state_cache_mutex provides a R/W lock over _state_cache and _state_cache_version,
+      //   which implement a cache of the finalizer_state (_qc_chain::get_state()).
+      mutable std::shared_mutex      _state_cache_mutex;
+      mutable finalizer_state        _state_cache;
+      mutable std::atomic<uint64_t>  _state_cache_version = 0;
 
       chain::controller*      _chain = nullptr;
 

libraries/hotstuff/include/eosio/hotstuff/qc_chain.hpp

@@ -21,7 +21,6 @@
 #include <exception>
 #include <stdexcept>
 
-
 // Enable this to swap the multi-index proposal store with std::map
 //#define QC_CHAIN_SIMPLE_PROPOSAL_STORE
 
@@ -31,65 +30,37 @@ namespace eosio::hotstuff {
    using namespace boost::multi_index;
    using namespace eosio::chain;
 
+   // Concurrency note: qc_chain is a single-threaded and lock-free decision engine.
+   //                   All thread synchronization, if any, is external.
    class qc_chain {
    public:
 
       qc_chain() = delete;
 
       qc_chain(name id, base_pacemaker* pacemaker, std::set<name> my_producers, bool info_logging, bool error_logging);
 
-#warning remove. bls12-381 key used for testing purposes
-      //todo : remove. bls12-381 key used for testing purposes
-      std::vector<uint8_t> _seed =
-         {  0,  50, 6,  244, 24,  199, 1,  25,  52,  88,  192,
-            19, 18, 12, 89,  6,   220, 18, 102, 58,  209, 82,
-            12, 62, 89, 110, 182, 9,   44, 20,  254, 22 };
-
-      fc::crypto::blslib::bls_private_key _private_key = fc::crypto::blslib::bls_private_key(_seed);
-
-      enum msg_type {
-         new_view = 1,
-         new_block = 2,
-         qc = 3,
-         vote = 4
-      };
-
-      bool _chained_mode = false;
-
-      fc::sha256 _b_leaf = NULL_PROPOSAL_ID;
-      fc::sha256 _b_lock = NULL_PROPOSAL_ID;
-      fc::sha256 _b_exec = NULL_PROPOSAL_ID;
-
-      fc::sha256 _b_finality_violation = NULL_PROPOSAL_ID;
+      uint64_t get_state_version() const { return _state_version; } // calling this w/ thread sync is optional
 
-      block_id_type _block_exec = NULL_BLOCK_ID;
+      name get_id_i() const { return _id; } // so far, only ever relevant in a test environment (no sync)
 
-      block_id_type _pending_proposal_block = NULL_BLOCK_ID;
+      // Calls to the following methods should be thread-synchronized externally:
 
-      uint32_t _v_height = 0;
-
-      eosio::chain::quorum_certificate _high_qc;
-      eosio::chain::quorum_certificate _current_qc;
-
-      eosio::chain::extended_schedule _schedule;
+      void get_state(finalizer_state& fs) const;
 
-      name _id;
-
-      base_pacemaker* _pacemaker = nullptr;
+      void on_beat(); //handler for pacemaker beat()
 
-      std::set<name> _my_producers;
+      void on_hs_vote_msg(const hs_vote_message& msg); //vote msg event handler
+      void on_hs_proposal_msg(const hs_proposal_message& msg); //proposal msg event handler
+      void on_hs_new_view_msg(const hs_new_view_message& msg); //new view msg event handler
+      void on_hs_new_block_msg(const hs_new_block_message& msg); //new block msg event handler
 
-      bool _log = true;
-      bool _errors = true;
+   private:
 
-      // returns nullptr if not found
-      const hs_proposal_message* get_proposal(const fc::sha256& proposal_id);
+      const hs_proposal_message* get_proposal(const fc::sha256& proposal_id); // returns nullptr if not found
 
       // returns false if proposal with that same ID already exists at the store of its height
       bool insert_proposal(const hs_proposal_message& proposal);
 
-      void get_state( finalizer_state& fs ) const;
-
       uint32_t positive_bits_count(fc::unsigned_int value);
 
       fc::unsigned_int update_bitset(fc::unsigned_int value, name finalizer);
@@ -119,9 +90,7 @@ namespace eosio::hotstuff {
 
       bool extends(const fc::sha256& descendant, const fc::sha256& ancestor); //verify that a proposal descends from another
 
-      void on_beat(); //handler for pacemaker beat()
-
-      void update_high_qc(const eosio::chain::quorum_certificate& high_qc); //check if update to our high qc is required
+      bool update_high_qc(const eosio::chain::quorum_certificate& high_qc); //check if update to our high qc is required
 
       void leader_rotation_check(); //check if leader rotation is required
 
@@ -134,31 +103,45 @@ namespace eosio::hotstuff {
       void send_hs_new_view_msg(const hs_new_view_message& msg); //send new view msg
       void send_hs_new_block_msg(const hs_new_block_message& msg); //send new block msg
 
-      void on_hs_vote_msg(const hs_vote_message& msg); //vote msg event handler
-      void on_hs_proposal_msg(const hs_proposal_message& msg); //proposal msg event handler
-      void on_hs_new_view_msg(const hs_new_view_message& msg); //new view msg event handler
-      void on_hs_new_block_msg(const hs_new_block_message& msg); //new block msg event handler
-
       void update(const hs_proposal_message& proposal); //update internal state
       void commit(const hs_proposal_message& proposal); //commit proposal (finality)
 
       void gc_proposals(uint64_t cutoff); //garbage collection of old proposals
 
-private:
-
-      // This mutex synchronizes all writes to the data members of this qc_chain against
-      //   get_state() calls (which ultimately come from e.g. the HTTP plugin).
-      // This could result in a HTTP query that gets the state of the core while it is
-      //   in the middle of processing a given request, since this is not serializing
-      //   against high-level message or request processing borders.
-      // If that behavior is not desired, we can instead synchronize this against a
-      //   consistent past snapshot of the qc_chain's state for e.g. the HTTP plugin,
-      //   which would be updated at the end of processing every request to the core
-      //   that does alter the qc_chain (hotstuff protocol state).
-      // And if the chain_pacemaker::_hotstuff_global_mutex locking strategy is ever
-      //   changed, then this probably needs to be reviewed as well.
-      //
-      mutable std::mutex     _state_mutex;
+#warning remove. bls12-381 key used for testing purposes
+      //todo : remove. bls12-381 key used for testing purposes
+      std::vector<uint8_t> _seed =
+         {  0,  50, 6,  244, 24,  199, 1,  25,  52,  88,  192,
+            19, 18, 12, 89,  6,   220, 18, 102, 58,  209, 82,
+            12, 62, 89, 110, 182, 9,   44, 20,  254, 22 };
+
+      fc::crypto::blslib::bls_private_key _private_key = fc::crypto::blslib::bls_private_key(_seed);
+
+      enum msg_type {
+         new_view = 1,
+         new_block = 2,
+         qc = 3,
+         vote = 4
+      };
+
+      bool _chained_mode = false;
+      block_id_type _block_exec = NULL_BLOCK_ID;
+      block_id_type _pending_proposal_block = NULL_BLOCK_ID;
+      fc::sha256 _b_leaf = NULL_PROPOSAL_ID;
+      fc::sha256 _b_lock = NULL_PROPOSAL_ID;
+      fc::sha256 _b_exec = NULL_PROPOSAL_ID;
+      fc::sha256 _b_finality_violation = NULL_PROPOSAL_ID;
+      eosio::chain::quorum_certificate _high_qc;
+      eosio::chain::quorum_certificate _current_qc;
+      uint32_t _v_height = 0;
+      eosio::chain::extended_schedule _schedule;
+      base_pacemaker* _pacemaker = nullptr;
+      std::set<name> _my_producers;
+      bool _log = true;
+      bool _errors = true;
+      name _id;
+
+      mutable std::atomic<uint64_t> _state_version = 1;
 
 #ifdef QC_CHAIN_SIMPLE_PROPOSAL_STORE
       // keep one proposal store (id -> proposal) by each height (height -> proposal store)