IF: Reject blocks with unsupported claims of last QC in block header extension

libraries/chain/block_state.cpp

@@ -126,25 +126,29 @@ void block_state::verify_qc(const valid_quorum_certificate& qc) const {
    std::vector<bls_public_key> pubkeys;
    std::vector<std::vector<uint8_t>> digests;
 
-   // utility to aggregate public keys and digests for verification
-   auto prepare_pubkeys_digests = [&] ( const auto& votes_bitset, const auto& digest ) {
-      auto n = std::min(num_finalizers, votes_bitset.size());
-      pubkeys.reserve(n);
-      digests.reserve(n);
-      for (auto i = 0u; i < n; ++i) {
-         if( votes_bitset[i] ) { // ith finalizer voted the digest
-            pubkeys.emplace_back(finalizers[i].public_key);
-            digests.emplace_back(std::vector<uint8_t>{digest.data(), digest.data() + digest.data_size()});
+   // utility to aggregate public keys for verification
+   auto aggregate_pubkeys = [&](const auto& votes_bitset) -> bls_public_key {
+      const auto n = std::min(num_finalizers, votes_bitset.size());
+      std::vector<bls_public_key> pubkeys_to_aggregate;
+      pubkeys_to_aggregate.reserve(n);
+      for(auto i = 0u; i < n; ++i) {
+         if (votes_bitset[i]) { // ith finalizer voted
+            pubkeys_to_aggregate.emplace_back(finalizers[i].public_key);
          }
       }
+
+      return fc::crypto::blslib::aggregate(pubkeys_to_aggregate);
    };
 
    // aggregate public keys and digests for strong and weak votes
    if( qc._strong_votes ) {
-      prepare_pubkeys_digests(*qc._strong_votes, strong_digest);
+      pubkeys.emplace_back(aggregate_pubkeys(*qc._strong_votes));
+      digests.emplace_back(std::vector<uint8_t>{strong_digest.data(), strong_digest.data() + strong_digest.data_size()});
    }
+
    if( qc._weak_votes ) {
-      prepare_pubkeys_digests(*qc._weak_votes, weak_digest);
+      pubkeys.emplace_back(aggregate_pubkeys(*qc._weak_votes));
+      digests.emplace_back(std::vector<uint8_t>{weak_digest.data(), weak_digest.data() + weak_digest.data_size()});
    }
 
    // validate aggregayed signature

libraries/chain/controller.cpp

@@ -3030,87 +3030,162 @@ struct controller_impl {
    }
 
    void integrate_received_qc_to_block(const block_id_type& id, const signed_block_ptr& b) {
-      // extract QC from block extensions
-      auto block_exts = b->validate_and_extract_extensions();
-      if( block_exts.count(quorum_certificate_extension::extension_id()) > 0 ) {
-         const auto& qc_ext = std::get<quorum_certificate_extension>(block_exts. lower_bound(quorum_certificate_extension::extension_id())->second);
-         auto bsp = fork_db_fetch_bsp_by_num( qc_ext.qc.block_height );
-         // Only save the QC from block extension if the claimed block does not
-         // have valid_qc or the new QC is better than valid_qc.
-         if( bsp && !bsp->valid_qc || (qc_ext.qc.qc.is_strong() && bsp->valid_qc->is_weak()) ) {
-            bsp->valid_qc = qc_ext.qc.qc;
-
-            if( bsp->valid_qc->is_strong() && bsp->core.final_on_strong_qc_block_num ) {
-               // We evaluate a block extension qc and advance lib if strong.
-               // This is done before evaluating the block. It is possible the block
-               // will not be valid or forked out. This is safe because the block is
-               // just acting as a carrier of this info. It doesn't matter if the block
-               // is actually valid as it simply is used as a network message for this data.
-               set_if_irreversible_block_num(*bsp->core.final_on_strong_qc_block_num);
-            }
-         }
+      // extract QC from block extension
+      const auto& block_exts = b->validate_and_extract_extensions();
+      if( block_exts.count(quorum_certificate_extension::extension_id()) == 0 ) {
+         return;
+      }
+      const auto& qc_ext = std::get<quorum_certificate_extension>(block_exts. lower_bound(quorum_certificate_extension::extension_id())->second);
+      const auto& new_qc = qc_ext.qc.qc;
+
+      const auto bsp = fork_db_fetch_bsp_by_num( qc_ext.qc.block_height );
+      if( !bsp ) {
+         return;
+      }
+
+      // Don't save the QC from block extension if the claimed block has a better valid_qc.
+      if (bsp->valid_qc && (bsp->valid_qc->is_strong() || new_qc.is_weak())) {
+         return;
+      }
+
+      // save the QC
+      bsp->valid_qc = new_qc;
+
+      // advance LIB if QC is strong and final_on_strong_qc_block_num has value
+      if( new_qc.is_strong() && bsp->core.final_on_strong_qc_block_num ) {
+         // We evaluate a block extension qc and advance lib if strong.
+         // This is done before evaluating the block. It is possible the block
+         // will not be valid or forked out. This is safe because the block is
+         // just acting as a carrier of this info. It doesn't matter if the block
+         // is actually valid as it simply is used as a network message for this data.
+         set_if_irreversible_block_num(*bsp->core.final_on_strong_qc_block_num);
       }
    }
 
-   // Verify QC claim made by instant_finality_extension in block header extension
+   // Verify QC claim made by instant_finality_extension in header extension
    // and quorum_certificate_extension in block extension are valid.
    // Called from net-threads. It is thread safe as signed_block is never modified
    // after creation.
    void verify_qc_claim( const signed_block_ptr& b, const block_header_state& prev ) {
-      // If block header extension does not have instant_finality_extension,
-      // just return
+      auto block_exts = b->validate_and_extract_extensions();
+
       std::optional<block_header_extension> header_ext = b->extract_header_extension(instant_finality_extension::extension_id());
       if( !header_ext ) {
+         EOS_ASSERT( block_exts.count(quorum_certificate_extension::extension_id()) == 0,
+                     block_validate_exception,
+                     "A block cannot provide QC block extension without QC header extension" );
+
+         // If header extension does not have instant_finality_extension,
+         // do not continue.
          return;
       }
 
-      // If qc_info is not included, just return
       const auto& if_ext = std::get<instant_finality_extension>(*header_ext);
       if( !if_ext.qc_info ) {
+         EOS_ASSERT( block_exts.count(quorum_certificate_extension::extension_id()) == 0,
+                     block_validate_exception,
+                     "A block cannot provide QC block extension without QC claim" );
+
+         // If header extension does not have QC claim,
+         // do not continue.
          return;
       }
 
-      // extract received QC information
-      qc_info_t received_qc_info{ *if_ext.qc_info };
+      // extract QC claim
+      qc_info_t qc_claim{ *if_ext.qc_info };
 
-      // if previous block's header extension has the same claim, just return
-      // (previous block had already validated the claim)
+      // extract previous header extension
       std::optional<block_header_extension> prev_header_ext = prev.header.extract_header_extension(instant_finality_extension::extension_id());
-      if( prev_header_ext ) {
-         auto prev_if_ext = std::get<instant_finality_extension>(*prev_header_ext);
-         if( prev_if_ext.qc_info && prev_if_ext.qc_info->last_qc_block_num == received_qc_info.last_qc_block_num && prev_if_ext.qc_info->is_last_qc_strong == received_qc_info.is_last_qc_strong ) {
-            return;
+
+      // A block should not be able to claim there was a QC on a block that
+      // is prior to the transition to IF.
+      EOS_ASSERT( prev_header_ext,
+                  block_validate_exception,
+                  "Previous header extension must include instant_finality_extension " );
+
+      auto prev_if_ext = std::get<instant_finality_extension>(*prev_header_ext);
+      auto prev_qc_info = prev_if_ext.qc_info;
+
+      // validate QC claim against previous block QC info
+      if( prev_qc_info ) {
+         // new claimed QC block nubmber cannot be smaller than previous block's
+         EOS_ASSERT( qc_claim.last_qc_block_num >= prev_qc_info->last_qc_block_num,
+                     block_validate_exception,
+                     "claimed last_qc_block_num (${n1}) must be equal to or greater than previous block's last_qc_block_num (${n2})",
+                     ("n1", qc_claim.last_qc_block_num)("n2", prev_qc_info->last_qc_block_num) );
+
+         if( qc_claim.last_qc_block_num == prev_qc_info->last_qc_block_num ) {
+            if( qc_claim.is_last_qc_strong == prev_qc_info->is_last_qc_strong ) {
+               // QC block extension is redundant
+               EOS_ASSERT( block_exts.count(quorum_certificate_extension::extension_id()) == 0,
+                           block_validate_exception,
+                           "A block should not provide QC block extension if QC claim is the same as previous block" );
+
+               // if previous block's header extension has the same claim, just return
+               // (previous block already validated the claim)
+               return;
+            }
+
+            // new claimed QC must be stricter than previous if block number is the same
+            EOS_ASSERT( qc_claim.is_last_qc_strong || !prev_qc_info->is_last_qc_strong,
+                        block_validate_exception,
+                        "claimed QC (${s1}) must be stricter than previous block's (${s2}) if block number is the same",
+                       ("s1", qc_claim.is_last_qc_strong)("s2", prev_qc_info->is_last_qc_strong) );
          }
       }
 
-      // extract QC from block extensions
-      auto block_exts = b->validate_and_extract_extensions();
-      EOS_ASSERT( block_exts.count(quorum_certificate_extension::extension_id()) > 0,
-                  block_validate_exception,
-                  "received block has finality header extension but does not have QC block extension" );
-      const auto& qc_ext = std::get<quorum_certificate_extension>(block_exts. lower_bound(quorum_certificate_extension::extension_id())->second);
-      const auto& received_qc = qc_ext.qc;
-      const auto& valid_qc = qc_ext.qc.qc;
+      if( block_exts.count(quorum_certificate_extension::extension_id()) == 0 ) {
+         // If claim is a strong QC and there wasn't already an identical claim
+         // in the previous block (checked earlier), QC proof must be provided
+         EOS_ASSERT( !qc_claim.is_last_qc_strong,
+                     block_validate_exception,
+                     "QC block extension must be provided if the claimed QC block is strong" );
+
+         // Conditions:
+         //    * the claim is that the last QC is a weak QC,
+         //    * it wasn't already satisfied by the claim in the prior block,
+         //    * and there is no block extension
+         // Actions:
+         //    * if it claims a block number lower than that of the current
+         //      last irreversible block, then the new block should be rejected;
+         //    * if it claims a block number greater than that of the current last
+         //      irreversible block, then the new block must have a corresponding
+         //      QC in the extension that must be validated;
+         //    * if it claims a block number exactly equal to that of the current
+         //      last irreversible block number, then the claim of the QC being
+         //      weak can be accepted without a block extension.
+         //
+         EOS_ASSERT( qc_claim.last_qc_block_num == if_irreversible_block_num,
+                     block_validate_exception,
+                     "QC block extension must be included if the claimed QC block is not current irreversible block" );
+
+         return;
+      }
+
+      const auto& qc_ext = std::get<quorum_certificate_extension>(block_exts.lower_bound(quorum_certificate_extension::extension_id())->second);
+      const auto& qc_proof = qc_ext.qc;
 
       // Check QC information in header extension and block extension match
-      EOS_ASSERT( received_qc.block_height == received_qc_info.last_qc_block_num,
+      EOS_ASSERT( qc_proof.block_height == qc_claim.last_qc_block_num,
                   block_validate_exception,
                   "QC block number (${n1}) in block extension does not match last_qc_block_num (${n2}) in header extension",
-                  ("n1", received_qc.block_height)("n2", received_qc_info.last_qc_block_num) );
-      EOS_ASSERT( valid_qc.is_strong() == received_qc_info.is_last_qc_strong,
+                  ("n1", qc_proof.block_height)("n2", qc_claim.last_qc_block_num) );
+
+      // Verify claimed strictness is the same as in proof
+      EOS_ASSERT( qc_proof.qc.is_strong() == qc_claim.is_last_qc_strong,
                   block_validate_exception,
                   "QC is_strong (${s1}) in block extension does not match is_last_qc_strong (${22}) in header extension",
-                  ("s1", valid_qc.is_strong())("s2", received_qc_info.is_last_qc_strong) );
+                  ("s1", qc_proof.qc.is_strong())("s2", qc_claim.is_last_qc_strong) );
 
       // find the claimed block's block state
-      auto claimed_block_bsp = fork_db_fetch_bsp_by_num( received_qc_info.last_qc_block_num );
-      EOS_ASSERT( claimed_block_bsp,
+      auto bsp = fork_db_fetch_bsp_by_num( qc_claim.last_qc_block_num );
+      EOS_ASSERT( bsp,
                   block_validate_exception,
                   "Block state was not found in forkdb for block number ${b}",
-                  ("b", valid_qc.is_strong()) );
+                  ("b", qc_claim.last_qc_block_num) );
 
-      // verify the claims
-      claimed_block_bsp->verify_qc(valid_qc);
+      // verify the QC proof against the claimed block
+      bsp->verify_qc(qc_proof.qc);
    }
 
    // thread safe, expected to be called from thread other than the main thread