IF: Add thread safety to chain_pacemaker access of chain state

libraries/chain/controller.cpp

@@ -264,10 +264,8 @@ struct controller_impl {
    map< account_name, map<handler_key, apply_handler> >   apply_handlers;
    unordered_map< builtin_protocol_feature_t, std::function<void(controller_impl&)>, enum_hash<builtin_protocol_feature_t> > protocol_feature_activation_handlers;
 
-   // TODO: This probably wants to be something better;
-   //       Storing when set_finalizers() is called; retrievable via get_finalizers() (called by chain_pacemaker)
-   uint64_t                     fthreshold = 0;
-   vector<finalizer_authority>  finalizers;
+   // TODO: This probably wants to be something better; store in chainbase and/or block_state
+   finalizer_set                current_finalizer_set;
 
    void pop_block() {
       auto prev = fork_db.get_block( head->header.previous );
@@ -1995,13 +1993,9 @@ struct controller_impl {
       emit( self.new_hs_new_block_message, msg );
    }
 
-   void set_finalizers_impl(uint64_t fthreshold, vector<finalizer_authority> finalizers) {
-      this->fthreshold = fthreshold;
-      this->finalizers = std::move(finalizers);
-   }
-
-   std::pair<uint64_t, vector<finalizer_authority>> get_finalizers_impl() const {
-      return { fthreshold, finalizers };
+   void set_finalizers_impl(const finalizer_set& fin_set) {
+      // TODO store in chainbase
+      current_finalizer_set = fin_set;
    }
 
    /**
@@ -3327,12 +3321,12 @@ int64_t controller::set_proposed_producers( vector<producer_authority> producers
    return version;
 }
 
-void controller::set_finalizers( uint64_t fthreshold, vector<finalizer_authority> finalizers ) {
-   my->set_finalizers_impl(fthreshold, std::move(finalizers));
+void controller::set_finalizers( const finalizer_set& fin_set ) {
+   my->set_finalizers_impl(fin_set);
 }
 
-std::pair<uint64_t, vector<finalizer_authority>> controller::get_finalizers() const {
-   return my->get_finalizers_impl();
+const finalizer_set& controller::get_finalizers() const {
+   return my->current_finalizer_set;
 }
 
 const producer_authority_schedule&    controller::active_producers()const {

libraries/chain/include/eosio/chain/controller.hpp

@@ -30,6 +30,7 @@ namespace eosio { namespace chain {
    using hs_new_view_message_ptr = std::shared_ptr<hs_new_view_message>;
    using hs_new_block_message_ptr = std::shared_ptr<hs_new_block_message>;
    struct finalizer_authority;
+   struct finalizer_set;
 
    class authorization_manager;
 
@@ -307,8 +308,8 @@ namespace eosio { namespace chain {
 
          int64_t set_proposed_producers( vector<producer_authority> producers );
 
-         void set_finalizers( uint64_t fthreshold, vector<finalizer_authority> finalizers );
-         std::pair<uint64_t, vector<finalizer_authority>> get_finalizers() const;
+         void set_finalizers( const finalizer_set& fin_set );
+         const finalizer_set& get_finalizers() const;
 
          bool light_validation_allowed() const;
          bool skip_auth_check()const;

libraries/chain/include/eosio/chain/finalizer_set.hpp

@@ -48,7 +48,7 @@ namespace eosio::chain {
    struct finalizer_authority {
 
       std::string  description;
-      uint64_t     fweight; // weight that this finalizer's vote has for meeting fthreshold
+      uint64_t     fweight = 0; // weight that this finalizer's vote has for meeting fthreshold
       fc::crypto::blslib::bls_public_key  public_key;
 
       auto to_shared(chainbase::allocator<char> alloc) const {
@@ -100,7 +100,7 @@ namespace eosio::chain {
       }
 
       uint32_t                                       version = 0; ///< sequentially incrementing version number
-      uint64_t                                       fthreshold;  // vote fweight threshold to finalize blocks
+      uint64_t                                       fthreshold = 0;  // vote fweight threshold to finalize blocks
       vector<finalizer_authority>                    finalizers; // Instant Finality voter set
 
       friend bool operator == ( const finalizer_set& a, const finalizer_set& b )

libraries/chain/webassembly/privileged.cpp

@@ -158,6 +158,7 @@ namespace eosio { namespace chain { namespace webassembly {
       fc::raw::unpack(ds, finset);
       vector<finalizer_authority> & finalizers = finset.finalizers;
 
+      // TODO: check version and increment it or verify correct
       EOS_ASSERT( finalizers.size() <= config::max_finalizers, wasm_execution_error, "Finalizer set exceeds the maximum finalizer count for this chain" );
       EOS_ASSERT( finalizers.size() > 0, wasm_execution_error, "Finalizer set cannot be empty" );
 
@@ -177,7 +178,7 @@ namespace eosio { namespace chain { namespace webassembly {
       EOS_ASSERT( finalizers.size() == unique_finalizer_keys.size(), wasm_execution_error, "Duplicate finalizer bls key in finalizer set" );
       EOS_ASSERT( finset.fthreshold > f_weight_sum / 2, wasm_execution_error, "Finalizer set threshold cannot be met by finalizer weights" );
 
-      context.control.set_finalizers( finset.fthreshold, std::move(finalizers) );
+      context.control.set_finalizers( finset );
    }
 
    uint32_t interface::get_blockchain_parameters_packed( legacy_span<char> packed_blockchain_parameters ) const {

libraries/hotstuff/chain_pacemaker.cpp

@@ -105,18 +105,13 @@ namespace eosio { namespace hotstuff {
         _qc_chain("default"_n, this, std::move(my_producers), logger),
         _logger(logger)
    {
-   }
-
-   // Called internally by the chain_pacemaker to decide whether it should do something or not, based on feature activation.
-   // Only methods called by the outside need to call this; methods called by qc_chain only don't need to check for enable().
-   bool chain_pacemaker::enabled() const {
-      return _chain->is_builtin_activated( builtin_protocol_feature_t::instant_finality );
+      _accepted_block_connection = chain->accepted_block.connect( [this]( const block_state_ptr& blk ) {
+         on_accepted_block( blk );
+      } );
+      _head_block_state = chain->head_block_state();
    }
 
    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) {
@@ -141,12 +136,6 @@ namespace eosio { namespace hotstuff {
       fs = _state_cache;
    }
 
-   name chain_pacemaker::get_proposer() {
-      const block_state_ptr& hbs = _chain->head_block_state();
-      name n = hbs->header.producer;
-      return n;
-   }
-
    name chain_pacemaker::debug_leader_remap(name n) {
 /*
       // FIXME/REMOVE: simple device to test proposer/leader
@@ -212,9 +201,23 @@ namespace eosio { namespace hotstuff {
       return n;
    }
 
+   // called from main thread
+   void chain_pacemaker::on_accepted_block( const block_state_ptr& blk ) {
+      std::scoped_lock g( _chain_state_mutex );
+      _head_block_state = blk;
+      // TODO only update local cache if changed, check version or use !=
+      _finalizer_set = _chain->get_finalizers(); // TODO get from chainbase or from block_state
+   }
+
+   name chain_pacemaker::get_proposer() {
+      std::scoped_lock g( _chain_state_mutex );
+      return _head_block_state->header.producer;
+   }
+
    name chain_pacemaker::get_leader() {
-      const block_state_ptr& hbs = _chain->head_block_state();
-      name n = hbs->header.producer;
+      std::unique_lock g( _chain_state_mutex );
+      name n = _head_block_state->header.producer;
+      g.unlock();
 
       // FIXME/REMOVE: testing leader/proposer separation
       n = debug_leader_remap(n);
@@ -223,9 +226,10 @@ namespace eosio { namespace hotstuff {
    }
 
    name chain_pacemaker::get_next_leader() {
-      const block_state_ptr& hbs = _chain->head_block_state();
-      block_timestamp_type next_block_time = hbs->header.timestamp.next();
-      producer_authority p_auth = hbs->get_scheduled_producer(next_block_time);
+      std::unique_lock g( _chain_state_mutex );
+      block_timestamp_type next_block_time = _head_block_state->header.timestamp.next();
+      producer_authority p_auth = _head_block_state->get_scheduled_producer(next_block_time);
+      g.unlock();
       name n = p_auth.producer_name;
 
       // FIXME/REMOVE: testing leader/proposer separation
@@ -249,10 +253,12 @@ namespace eosio { namespace hotstuff {
       // - list of string finalizer descriptions instead of eosio name now
       // - also return the keys for each finalizer, not just name/description so qc_chain can use them
       //
-      auto [threshold, finalizers] = _chain->get_finalizers();
+      std::unique_lock g( _chain_state_mutex );
+      const auto& fin_set = _chain->get_finalizers(); // TODO use
+      block_state_ptr hbs = _head_block_state;
+      g.unlock();
 
       // Old code: get eosio::name from the producer schedule
-      const block_state_ptr& hbs = _chain->head_block_state();
       const std::vector<producer_authority>& pa_list = hbs->active_schedule.producers;
       std::vector<name> pn_list;
       pn_list.reserve(pa_list.size());
@@ -263,17 +269,16 @@ namespace eosio { namespace hotstuff {
    }
 
    block_id_type chain_pacemaker::get_current_block_id() {
-      return _chain->head_block_id();
+      std::scoped_lock g( _chain_state_mutex );
+      return _head_block_state->id;
    }
 
    uint32_t chain_pacemaker::get_quorum_threshold() {
       return _quorum_threshold;
    }
 
+   // called from the main application thread
    void chain_pacemaker::beat() {
-      if (! enabled())
-         return;
-
       csc prof("beat");
       std::lock_guard g( _hotstuff_global_mutex );
       prof.core_in();
@@ -302,9 +307,6 @@ namespace eosio { namespace hotstuff {
    }
 
    void chain_pacemaker::on_hs_proposal_msg(const hs_proposal_message& msg) {
-      if (! enabled())
-         return;
-
       csc prof("prop");
       std::lock_guard g( _hotstuff_global_mutex );
       prof.core_in();
@@ -313,9 +315,6 @@ namespace eosio { namespace hotstuff {
    }
 
    void chain_pacemaker::on_hs_vote_msg(const hs_vote_message& msg) {
-      if (! enabled())
-         return;
-
       csc prof("vote");
       std::lock_guard g( _hotstuff_global_mutex );
       prof.core_in();
@@ -324,9 +323,6 @@ namespace eosio { namespace hotstuff {
    }
 
    void chain_pacemaker::on_hs_new_block_msg(const hs_new_block_message& msg) {
-      if (! enabled())
-         return;
-
       csc prof("nblk");
       std::lock_guard g( _hotstuff_global_mutex );
       prof.core_in();
@@ -335,9 +331,6 @@ namespace eosio { namespace hotstuff {
    }
 
    void chain_pacemaker::on_hs_new_view_msg(const hs_new_view_message& msg) {
-      if (! enabled())
-         return;
-
       csc prof("view");
       std::lock_guard g( _hotstuff_global_mutex );
       prof.core_in();

libraries/hotstuff/include/eosio/hotstuff/base_pacemaker.hpp

@@ -30,8 +30,6 @@ namespace eosio::hotstuff {
 
       virtual chain::block_id_type get_current_block_id() = 0;
 
-      //hotstuff getters. todo : implement relevant setters as host functions
-#warning hotstuff getters. todo : implement relevant setters as host functions
       virtual chain::name get_proposer() = 0;
       virtual chain::name get_leader() = 0;
       virtual chain::name get_next_leader() = 0;

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

@@ -5,6 +5,8 @@
 
 #include <eosio/chain/finalizer_set.hpp>
 
+#include <boost/signals2/connection.hpp>
+
 #include <shared_mutex>
 
 namespace eosio::chain {
@@ -45,14 +47,14 @@ namespace eosio::hotstuff {
       void send_hs_new_view_msg(const hs_new_view_message& msg, name id);
       void send_hs_new_block_msg(const hs_new_block_message& msg, name id);
 
+   private:
+      void on_accepted_block( const block_state_ptr& blk );
+
    private:
 
       //FIXME/REMOVE: for testing/debugging only
       name debug_leader_remap(name n);
 
-      // Check if consensus upgrade feature is activated
-      bool enabled() const;
-
       // This serializes all messages (high-level requests) to the qc_chain core.
       // For maximum safety, the qc_chain core will only process one request at a time.
       // These requests can come directly from the net threads, or indirectly from a
@@ -66,7 +68,13 @@ namespace eosio::hotstuff {
       mutable finalizer_state        _state_cache;
       mutable std::atomic<uint64_t>  _state_cache_version = 0;
 
-      chain::controller*      _chain = nullptr;
+      chain::controller*                 _chain = nullptr; // TODO will not be needed once this is merged with PR#1559
+
+      mutable std::mutex                 _chain_state_mutex;
+      block_state_ptr                    _head_block_state;
+      finalizer_set                      _finalizer_set;
+
+      boost::signals2::scoped_connection _accepted_block_connection;
 
       qc_chain                _qc_chain;
 

libraries/hotstuff/qc_chain.cpp

@@ -674,6 +674,7 @@ namespace eosio { namespace hotstuff {
    }
 
    // Invoked when we could perhaps make a proposal to the network (or to ourselves, if we are the leader).
+   // Called from the main application thread
    void qc_chain::on_beat(){
 
       // Non-proposing leaders do not care about on_beat(), because leaders react to a block proposal

plugins/chain_plugin/chain_plugin.cpp

@@ -1128,7 +1128,6 @@ void chain_plugin_impl::plugin_startup()
 { try {
    EOS_ASSERT( chain_config->read_mode != db_read_mode::IRREVERSIBLE || !accept_transactions, plugin_config_exception,
                "read-mode = irreversible. transactions should not be enabled by enable_accept_transactions" );
-   EOS_ASSERT( _chain_pacemaker, plugin_config_exception, "chain_pacemaker not initialization" );
    try {
       auto shutdown = [](){ return app().quit(); };
       auto check_shutdown = [](){ return app().is_quiting(); };
@@ -2692,7 +2691,9 @@ void chain_plugin::notify_hs_new_block_message( const hs_new_block_message& msg
 };
 
 void chain_plugin::notify_hs_block_produced() {
-   my->_chain_pacemaker->beat();
+   if (chain().is_builtin_activated( builtin_protocol_feature_t::instant_finality )) {
+      my->_chain_pacemaker->beat();
+   }
 }
 
 fc::variant chain_plugin::get_log_trx_trace(const transaction_trace_ptr& trx_trace ) const {

plugins/producer_plugin/producer_plugin.cpp

@@ -1326,8 +1326,6 @@ void producer_plugin_impl::plugin_initialize(const boost::program_options::varia
 
    _snapshot_scheduler.set_db_path(_snapshots_dir);
    _snapshot_scheduler.set_snapshots_path(_snapshots_dir);
-
-   chain_plug->create_pacemaker(_producers);
 }
 
 void producer_plugin::plugin_initialize(const boost::program_options::variables_map& options) {
@@ -1360,6 +1358,8 @@ void producer_plugin_impl::plugin_startup() {
          EOS_ASSERT(_producers.empty() || chain_plug->accept_transactions(), plugin_config_exception,
                     "node cannot have any producer-name configured because no block production is possible with no [api|p2p]-accepted-transactions");
 
+         chain_plug->create_pacemaker(_producers);
+
          _accepted_block_connection.emplace(chain.accepted_block.connect([this](const auto& bsp) { on_block(bsp); }));
          _accepted_block_header_connection.emplace(chain.accepted_block_header.connect([this](const auto& bsp) { on_block_header(bsp); }));
          _irreversible_block_connection.emplace(