IF: Consider voting immediately if final on strong qc is validated

libraries/chain/block_state.cpp

@@ -39,6 +39,7 @@ block_state::block_state(const block_header_state& bhs, deque<transaction_metada
    block->transactions = std::move(trx_receipts);
 
    if( qc ) {
+      dlog("integrate qc ${qc} into block ${bn} ${id}", ("qc", qc->to_qc_claim())("bn", block_num())("id", id()));
       emplace_extension(block->block_extensions, quorum_certificate_extension::extension_id(), fc::raw::pack( *qc ));
    }
 

libraries/chain/controller.cpp

@@ -690,27 +690,25 @@ struct building_block {
                } else {
                   fork_db.apply_s<void>([&](const auto& forkdb) {
                      auto branch = forkdb.fetch_branch(parent_id());
-                     std::optional<quorum_certificate> qc;
                      for( auto it = branch.begin(); it != branch.end(); ++it ) {
-                        qc = (*it)->get_best_qc();
-                        if( qc ) {
+                        if( auto qc = (*it)->get_best_qc(); qc ) {
                            EOS_ASSERT( qc->block_num <= block_header::num_from_id(parent_id()), block_validate_exception,
                                        "most recent ancestor QC block number (${a}) cannot be greater than parent's block number (${p})",
                                        ("a", qc->block_num)("p", block_header::num_from_id(parent_id())) );
-                           auto qc_claim = qc_claim_t { qc->block_num, qc->qc.is_strong() };
-                           if( bb.parent.is_needed(*qc) ) {
+                           auto qc_claim = qc->to_qc_claim();
+                           if( bb.parent.is_needed(qc_claim) ) {
                               qc_data = qc_data_t{ *qc, qc_claim };
                            } else {
-                              qc_data = qc_data_t{ {},  qc_claim };
+                              // no new qc info, repeat existing
+                              qc_data = qc_data_t{ {},  bb.parent.core.latest_qc_claim() };
                            }
                            break;
                         }
                      }
 
-                     if (!qc) {
-                        // This only happens when parent block is the IF genesis block.
-                        // There is no ancestor block which has a QC.
-                        // Construct a default QC claim.
+                     if (!qc_data) {
+                        // This only happens when parent block is the IF genesis block or starting from snapshot.
+                        // There is no ancestor block which has a QC. Construct a default QC claim.
                         qc_data = qc_data_t{ {}, bb.parent.core.latest_qc_claim() };
                      }
                   });
@@ -1062,9 +1060,8 @@ struct controller_impl {
       return fork_db.apply<block_state_ptr>(
          overloaded{
             [](const fork_database_legacy_t&) -> block_state_ptr { return nullptr; },
-            [&](const fork_database_if_t&forkdb) -> block_state_ptr {
-               auto bsp = forkdb.search_on_head_branch(block_num, include_root_t::yes);
-               return bsp;
+            [&](const fork_database_if_t& forkdb) -> block_state_ptr {
+               return forkdb.search_on_head_branch(block_num, include_root_t::yes);
             }
          }
       );
@@ -1075,9 +1072,19 @@ struct controller_impl {
       return fork_db.apply<block_state_ptr>(
          overloaded{
             [](const fork_database_legacy_t&) -> block_state_ptr { return nullptr; },
-            [&](const fork_database_if_t&forkdb) -> block_state_ptr {
-               auto bsp = forkdb.search_on_branch(id, block_num, include_root_t::yes);
-               return bsp;
+            [&](const fork_database_if_t& forkdb) -> block_state_ptr {
+               return forkdb.search_on_branch(id, block_num, include_root_t::yes);
+            }
+         }
+      );
+   }
+
+   block_state_ptr fetch_bsp(const block_id_type& id) const {
+      return fork_db.apply<block_state_ptr>(
+         overloaded{
+            [](const fork_database_legacy_t&) -> block_state_ptr { return nullptr; },
+            [&](const fork_database_if_t& forkdb) -> block_state_ptr {
+               return forkdb.get_block(id, include_root_t::yes);
             }
          }
       );
@@ -1664,7 +1671,7 @@ struct controller_impl {
                my_finalizers.set_default_safety_information(
                   finalizer_safety_information{ .last_vote_range_start = block_timestamp_type(0),
                                                 .last_vote = {},
-                                                .lock      = proposal_ref(lib->id(), lib->timestamp()) });
+                                                .lock      = {lib->id(), lib->timestamp()} });
             };
             fork_db.apply_s<void>(set_finalizer_defaults);
          } else {
@@ -1674,7 +1681,7 @@ struct controller_impl {
                my_finalizers.set_default_safety_information(
                   finalizer_safety_information{ .last_vote_range_start = block_timestamp_type(0),
                                                 .last_vote = {},
-                                                .lock      = proposal_ref(lib->id(), lib->timestamp()) });
+                                                .lock      = {lib->id(), lib->timestamp()} });
             };
             fork_db.apply_s<void>(set_finalizer_defaults);
          }
@@ -2949,8 +2956,8 @@ struct controller_impl {
                      auto lib_block   = head;
                      my_finalizers.set_default_safety_information(
                         finalizer_safety_information{ .last_vote_range_start = block_timestamp_type(0),
-                                                      .last_vote = proposal_ref(start_block->id(), start_block->timestamp()),
-                                                      .lock      = proposal_ref(lib_block->id(),   lib_block->timestamp()) });
+                                                      .last_vote = {start_block->id(), start_block->timestamp()},
+                                                      .lock      = {lib_block->id(),   lib_block->timestamp()} });
                   }
 
                   if ( (s != controller::block_status::irreversible && read_mode != db_read_mode::IRREVERSIBLE) && s != controller::block_status::ephemeral)
@@ -3232,7 +3239,7 @@ struct controller_impl {
    // called from net threads and controller's thread pool
    vote_status process_vote_message( const vote_message& vote ) {
       auto aggregate_vote = [&vote](auto& forkdb) -> vote_status {
-          auto bsp = forkdb.get_block(vote.proposal_id);
+          auto bsp = forkdb.get_block(vote.block_id);
           if (bsp) {
              return bsp->aggregate_vote(vote);
           }
@@ -3469,15 +3476,21 @@ struct controller_impl {
 
       const auto claimed = fetch_bsp_on_branch_by_num( bsp_in->previous(), qc_ext.qc.block_num );
       if( !claimed ) {
+         dlog("qc not found in forkdb, qc: ${qc} for block ${bn} ${id}, previous ${p}",
+              ("qc", qc_ext.qc.to_qc_claim())("bn", bsp_in->block_num())("id", bsp_in->id())("p", bsp_in->previous()));
          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())) {
+         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())
+              ("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;
 
       // advance LIB if QC is strong
@@ -3495,14 +3508,13 @@ struct controller_impl {
    void consider_voting(const block_state_ptr& bsp) {
       // 1. Get the `core.final_on_strong_qc_block_num` for the block you are considering to vote on and use that to find the actual block ID
       //    of the ancestor block that has that block number.
-      // 2. If that block ID is not an ancestor of the current head block, then do not vote for that block.
+      // 2. If that block ID is for a non validated block, then do not vote for that block.
       // 3. Otherwise, consider voting for that block according to the decide_vote rules.
 
       if (bsp->core.final_on_strong_qc_block_num > 0) {
          const auto& final_on_strong_qc_block_ref = bsp->core.get_block_reference(bsp->core.final_on_strong_qc_block_num);
-         auto final = fetch_bsp_on_head_branch_by_num(final_on_strong_qc_block_ref.block_num());
-         if (final) {
-            assert(final->is_valid()); // if found on head branch then it must be validated
+         auto final = fetch_bsp(final_on_strong_qc_block_ref.block_id);
+         if (final && final->is_valid()) {
             create_and_send_vote_msg(bsp);
          }
       }
@@ -3643,10 +3655,11 @@ struct controller_impl {
    void maybe_switch_forks(const forked_callback_t& cb, const trx_meta_cache_lookup& trx_lookup) {
       auto maybe_switch = [&](auto& forkdb) {
          if (read_mode != db_read_mode::IRREVERSIBLE) {
-            auto fork_head = forkdb.head();
-            if (chain_head.id() != fork_head->id()) {
+            auto pending_head = forkdb.pending_head();
+            if (chain_head.id() != pending_head->id() && pending_head->id() != forkdb.head()->id()) {
+               dlog("switching forks on controller->maybe_switch_forks call");
                controller::block_report br;
-               maybe_switch_forks(br, fork_head, fork_head->is_valid() ? controller::block_status::validated : controller::block_status::complete,
+               maybe_switch_forks(br, pending_head, pending_head->is_valid() ? controller::block_status::validated : controller::block_status::complete,
                                   cb, trx_lookup);
             }
          }

libraries/chain/hotstuff/finalizer.cpp

@@ -5,16 +5,24 @@
 namespace eosio::chain {
 
 // ----------------------------------------------------------------------------------------
-finalizer::vote_result finalizer::decide_vote(const finality_core& core, const block_id_type& proposal_id,
-                                               const block_timestamp_type proposal_timestamp) {
+finalizer::vote_result finalizer::decide_vote(const block_state_ptr& bsp) {
    vote_result res;
 
-   res.monotony_check = fsi.last_vote.empty() || proposal_timestamp > fsi.last_vote.timestamp;
+   res.monotony_check = fsi.last_vote.empty() || bsp->timestamp() > fsi.last_vote.timestamp;
    // fsi.last_vote.empty() means we have never voted on a proposal, so the protocol feature
    // just activated and we can proceed
 
    if (!res.monotony_check) {
-      dlog("monotony check failed for proposal ${bn} ${p}, cannot vote", ("bn", block_header::num_from_id(proposal_id))("p", proposal_id));
+      if (fsi.last_vote.empty()) {
+         dlog("monotony check failed, block ${bn} ${p}, cannot vote, fsi.last_vote empty", ("bn", bsp->block_num())("p", bsp->id()));
+      } else {
+         if (fc::logger::get(DEFAULT_LOGGER).is_enabled(fc::log_level::debug)) {
+            if (bsp->id() != fsi.last_vote.block_id) { // we may have already voted when we received the block
+               dlog("monotony check failed, block ${bn} ${p}, cannot vote, ${t} <= ${lt}, fsi.last_vote ${lbn} ${lid}",
+                    ("bn", bsp->block_num())("p", bsp->id())("t", bsp->timestamp())("lt", fsi.last_vote.timestamp)("lbn", fsi.last_vote.block_num())("lid", fsi.last_vote.block_id));
+            }
+         }
+      }
       return res;
    }
 
@@ -23,16 +31,24 @@ finalizer::vote_result finalizer::decide_vote(const finality_core& core, const b
       // than the height of the proposal I'm locked on.
       // This allows restoration of liveness if a replica is locked on a stale proposal
       // -------------------------------------------------------------------------------
-      res.liveness_check = core.latest_qc_block_timestamp() > fsi.lock.timestamp;
+      res.liveness_check = bsp->core.latest_qc_block_timestamp() > fsi.lock.timestamp;
 
       if (!res.liveness_check) {
+         dlog("liveness check failed, block ${bn} ${id}: ${c} <= ${l}, fsi.lock ${lbn} ${lid}, latest_qc_claim: ${qc}",
+              ("bn", bsp->block_num())("id", bsp->id())("c", bsp->core.latest_qc_block_timestamp())("l", fsi.lock.timestamp)
+              ("lbn", fsi.lock.block_num())("lid", fsi.lock.block_id)("qc", bsp->core.latest_qc_claim()));
          // Safety check : check if this proposal extends the proposal we're locked on
-         res.safety_check = core.extends(fsi.lock.block_id);
+         res.safety_check = bsp->core.extends(fsi.lock.block_id);
+         if (!res.safety_check) {
+            dlog("safety  check  failed, block ${bn} ${id} did not extend fsi.lock ${lbn} ${lid}",
+                 ("bn", bsp->block_num())("id", bsp->id())("lbn", fsi.lock.block_num())("lid", fsi.lock.block_id));
+         }
       }
    } else {
       // Safety and Liveness both fail if `fsi.lock` is empty. It should not happen.
       // `fsi.lock` is initially set to `lib` when switching to IF or starting from a snapshot.
       // -------------------------------------------------------------------------------------
+      wlog("liveness check & safety check failed, block ${bn} ${id}, fsi.lock is empty", ("bn", bsp->block_num())("id", bsp->id()));
       res.liveness_check = false;
       res.safety_check   = false;
    }
@@ -43,36 +59,37 @@ finalizer::vote_result finalizer::decide_vote(const finality_core& core, const b
    // If we vote, update `fsi.last_vote` and also `fsi.lock` if we have a newer commit qc
    // -----------------------------------------------------------------------------------
    if (can_vote) {
-      auto [p_start, p_end] = std::make_pair(core.latest_qc_block_timestamp(), proposal_timestamp);
+      auto [p_start, p_end] = std::make_pair(bsp->core.latest_qc_block_timestamp(), bsp->timestamp());
 
       bool time_range_disjoint  = fsi.last_vote_range_start >= p_end || fsi.last_vote.timestamp <= p_start;
       bool voting_strong        = time_range_disjoint;
       if (!voting_strong && !fsi.last_vote.empty()) {
          // we can vote strong if the proposal is a descendant of (i.e. extends) our last vote id
-         voting_strong = core.extends(fsi.last_vote.block_id);
+         voting_strong = bsp->core.extends(fsi.last_vote.block_id);
       }
 
-      fsi.last_vote             = proposal_ref(proposal_id, proposal_timestamp);
+      fsi.last_vote             = { bsp->id(), bsp->timestamp() };
       fsi.last_vote_range_start = p_start;
 
-      auto& final_on_strong_qc_block_ref = core.get_block_reference(core.final_on_strong_qc_block_num);
-      if (voting_strong && final_on_strong_qc_block_ref.timestamp > fsi.lock.timestamp)
-         fsi.lock = proposal_ref(final_on_strong_qc_block_ref.block_id, final_on_strong_qc_block_ref.timestamp);
+      auto& final_on_strong_qc_block_ref = bsp->core.get_block_reference(bsp->core.final_on_strong_qc_block_num);
+      if (voting_strong && final_on_strong_qc_block_ref.timestamp > fsi.lock.timestamp) {
+         fsi.lock = { final_on_strong_qc_block_ref.block_id, final_on_strong_qc_block_ref.timestamp };
+      }
 
       res.decision = voting_strong ? vote_decision::strong_vote : vote_decision::weak_vote;
    }
 
-   dlog("block=${bn}, liveness_check=${l}, safety_check=${s}, monotony_check=${m}, can vote=${can_vote}, voting=${v}",
-        ("bn", block_header::num_from_id(proposal_id))("l",res.liveness_check)("s",res.safety_check)("m",res.monotony_check)
-        ("can_vote",can_vote)("v", res.decision));
+   dlog("block=${bn} ${id}, liveness_check=${l}, safety_check=${s}, monotony_check=${m}, can vote=${can_vote}, voting=${v}, locked=${lbn} ${lid}",
+        ("bn", bsp->block_num())("id", bsp->id())("l",res.liveness_check)("s",res.safety_check)("m",res.monotony_check)
+        ("can_vote",can_vote)("v", res.decision)("lbn", fsi.lock.block_num())("lid", fsi.lock.block_id));
    return res;
 }
 
 // ----------------------------------------------------------------------------------------
 std::optional<vote_message> finalizer::maybe_vote(const bls_public_key& pub_key,
-                                                  const block_header_state_ptr& p,
+                                                  const block_state_ptr& bsp,
                                                   const digest_type& digest) {
-   finalizer::vote_decision decision = decide_vote(p->core, p->id(), p->timestamp()).decision;
+   finalizer::vote_decision decision = decide_vote(bsp).decision;
    if (decision == vote_decision::strong_vote || decision == vote_decision::weak_vote) {
       bls_signature sig;
       if (decision == vote_decision::weak_vote) {
@@ -82,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{ p->id(), decision == vote_decision::strong_vote, pub_key, sig };
+      return vote_message{ bsp->id(), decision == vote_decision::strong_vote, pub_key, sig };
    }
    return {};
 }

libraries/chain/include/eosio/chain/block_header_state.hpp

@@ -76,8 +76,8 @@ struct block_header_state {
    block_header_state next(const signed_block_header& h, validator_t& validator) const;
 
    // block descending from this need the provided qc in the block extension
-   bool is_needed(const quorum_certificate& qc) const {
-      return qc.block_num > core.latest_qc_claim().block_num;
+   bool is_needed(const qc_claim_t& qc_claim) const {
+      return qc_claim > core.latest_qc_claim();
    }
 
    const vector<digest_type>& get_new_protocol_feature_activations() const;

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

@@ -26,14 +26,12 @@
 // -------------------------------------------------------------------------------------------
 
 namespace eosio::chain {
-   // ----------------------------------------------------------------------------------------
-   using proposal_ref = block_ref;
 
    // ----------------------------------------------------------------------------------------
    struct finalizer_safety_information {
       block_timestamp_type last_vote_range_start;
-      proposal_ref         last_vote;
-      proposal_ref         lock;
+      block_ref            last_vote;
+      block_ref            lock;
 
       static constexpr uint64_t magic = 0x5AFE11115AFE1111ull;
 
@@ -59,10 +57,9 @@ namespace eosio::chain {
       bls_private_key               priv_key;
       finalizer_safety_information  fsi;
 
-      vote_result  decide_vote(const finality_core& core, const block_id_type &id,
-                               const block_timestamp_type timestamp);
+      vote_result  decide_vote(const block_state_ptr& bsp);
 
-      std::optional<vote_message> maybe_vote(const bls_public_key& pub_key, const block_header_state_ptr& bhsp,
+      std::optional<vote_message> maybe_vote(const bls_public_key& pub_key, const block_state_ptr& bsp,
                                              const digest_type& digest);
    };
 
@@ -81,7 +78,7 @@ namespace eosio::chain {
 
       template<class F>
       void maybe_vote(const finalizer_policy& fin_pol,
-                      const block_header_state_ptr& bhsp,
+                      const block_state_ptr& bsp,
                       const digest_type& digest,
                       F&& process_vote) {
          std::vector<vote_message> votes;
@@ -90,7 +87,7 @@ namespace eosio::chain {
          // first accumulate all the votes
          for (const auto& f : fin_pol.finalizers) {
             if (auto it = finalizers.find(f.public_key); it != finalizers.end()) {
-               std::optional<vote_message> vote_msg = it->second.maybe_vote(it->first, bhsp, digest);
+               std::optional<vote_message> vote_msg = it->second.maybe_vote(it->first, bsp, digest);
                if (vote_msg)
                   votes.push_back(std::move(*vote_msg));
             }