Correctly support when finality advances multiple blocks at a time.

libraries/chain/block_header_state.cpp

@@ -158,6 +158,8 @@ void finish_next(const block_header_state& prev,
          // ---------------------------------------------------------------------------
          next_header_state.finalizer_policies = prev.finalizer_policies;
       } else {
+         auto next_block_num = next_header_state.block_num();
+
          while (it != prev.finalizer_policies.end() && it->first <= lib) {
             const finalizer_policy_tracker& tracker = it->second;
             if (tracker.state == finalizer_policy_tracker::state_t::pending) {
@@ -166,11 +168,33 @@ void finish_next(const block_header_state& prev,
                next_header_state.active_finalizer_policy.reset(new finalizer_policy(*tracker.policy));
             } else {
                assert(tracker.state == finalizer_policy_tracker::state_t::proposed);
-               // block where finalizer_policy was proposed became final. The finalizer policy will
-               // become active when next block becomes final.
+
+               // The block where finalizer_policy was proposed has became final. The finalizer
+               // policy will become active when `next_block_num` becomes final.
+               //
+               // So `tracker.policy` should become `pending` at `next_block_num`.
+               //
+               // Either insert a new `finalizer_policy_tracker` value, or update the `pending`
+               // policy if there is already one  at `next_block_num` (which can happen when
+               // finality advances multiple block at a time, and more than one policy move from
+               // proposed to pending.
+               //
+               // Since we iterate finalizer_policies which is a multimap sorted by block number,
+               // the last one we add will be for the highest block number, which is what we want.
                // ---------------------------------------------------------------------------------
-               finalizer_policy_tracker t { finalizer_policy_tracker::state_t::pending, tracker.policy };
-               next_header_state.finalizer_policies.emplace(next_header_state.block_num(), std::move(t));
+               auto range = next_header_state.finalizer_policies.equal_range(next_block_num);
+               auto itr = range.first;
+               for (; itr != range.second; ++itr) {
+                  if (itr->second.state == finalizer_policy_tracker::state_t::pending) {
+                     itr->second.policy = tracker.policy;
+                     break;
+                  }
+               }
+               if (itr == range.second) {
+                  // there wasn't already a pending one for `next_block_num`, add a new tracker
+                  finalizer_policy_tracker t { finalizer_policy_tracker::state_t::pending, tracker.policy };
+                  next_header_state.finalizer_policies.emplace(next_block_num, std::move(t));
+               }
             }
             ++it;
          }
@@ -321,5 +345,37 @@ block_header_state block_header_state::next(const signed_block_header& h, valida
    return next_header_state;
 }
 
+// -------------------------------------------------------------------------------
+// do some sanity checks on block_header_state
+// -------------------------------------------------------------------------------
+bool block_header_state::sanity_check() const {
+   // check that we have at most *one* proposed and *one* pending `finalizer_policy`
+   // for any block number
+   // -----------------------------------------------------------------------------
+   block_num_type block_num(0);
+   bool pending{false}, proposed{false};
+
+   for (auto it = finalizer_policies.begin(); it != finalizer_policies.end(); ++it) {
+      if (block_num != it->first) {
+         pending = proposed = false;
+         block_num = it->first;
+      }
+      const auto& tracker = it->second;
+      if (tracker.state == finalizer_policy_tracker::state_t::proposed) {
+         if (proposed)
+            return false;
+         else
+            proposed = true;
+      }
+      if (tracker.state == finalizer_policy_tracker::state_t::pending) {
+         if (pending)
+            return false;
+         else
+            pending = true;
+      }
+   }
+   return true;
+}
+
 } // namespace eosio::chain
 

libraries/chain/controller.cpp

@@ -5111,6 +5111,10 @@ block_state_legacy_ptr controller::head_block_state_legacy()const {
    });
 }
 
+bool controller::head_sanity_check()const {
+   return apply<bool>(my->chain_head, [](const auto& head) { return head->sanity_check(); });
+}
+
 const signed_block_ptr& controller::head_block()const {
    return my->chain_head.block();
 }

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

@@ -131,6 +131,8 @@ struct block_header_state {
       return qc_claim > core.latest_qc_claim();
    }
 
+   bool sanity_check() const; // does sanity check of block_header_state, returns true if successful
+
    const vector<digest_type>& get_new_protocol_feature_activations() const;
    const producer_authority& get_scheduled_producer(block_timestamp_type t) const;
 

libraries/chain/include/eosio/chain/block_header_state_legacy.hpp

@@ -148,6 +148,8 @@ struct block_header_state_legacy : public detail::block_header_state_legacy_comm
    void                   sign( const signer_callback_type& signer );
    void                   verify_signee()const;
 
+   bool                   sanity_check() const { return true; }
+
    const vector<digest_type>& get_new_protocol_feature_activations()const;
 };
 

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

@@ -240,6 +240,7 @@ namespace eosio::chain {
          account_name         head_block_producer()const;
          const block_header&  head_block_header()const;
          const signed_block_ptr& head_block()const;
+         bool                 head_sanity_check()const;
          // returns nullptr after instant finality enabled
          block_state_legacy_ptr head_block_state_legacy()const;
          // returns finality_data associated with chain head for SHiP when in Savanna,

libraries/testing/include/eosio/testing/tester.hpp

@@ -501,6 +501,7 @@ namespace eosio::testing {
          // -----------------------------------------------------------------
          void check_head_finalizer_policy(uint32_t generation,
                                           std::span<const bls_public_key> keys_span) {
+            BOOST_REQUIRE_EQUAL(control->head_sanity_check(), true);
             auto finpol = active_finalizer_policy(control->head_block_header().calculate_id());
             BOOST_REQUIRE(!!finpol);
             BOOST_REQUIRE_EQUAL(finpol->generation, generation);

unittests/finality_tests.cpp

@@ -522,4 +522,78 @@ BOOST_FIXTURE_TEST_CASE(second_set_finalizers, finality_test_cluster<4>) { try {
 
 } FC_LOG_AND_RETHROW() }
 
+// verify issue https://github.com/AntelopeIO/spring/issues/130 is fixed
+// ---------------------------------------------------------------------
+BOOST_FIXTURE_TEST_CASE(finality_skip, finality_test_cluster<4>) { try {
+   produce_and_push_block();
+   process_votes(1, num_needed_for_quorum);
+   produce_and_push_block();
+
+   // when a quorum of nodes vote, LIB should advance
+   BOOST_REQUIRE_EQUAL(num_lib_advancing(), num_nodes);
+   BOOST_REQUIRE(produce_blocks_and_verify_lib_advancing());
+
+   auto add_set_finalizers = [&](size_t start_idx) {
+      assert(fin_policy_0);                 // current finalizer policy from transition to Savanna
+      auto indices = fin_policy_indices_0;  // start from original set of indices
+      assert(indices[0] == 0u);             // we used index 0 for node0 in original policy
+      indices[0] = start_idx;               // update key used for node0 in policy
+      auto pubkeys = node0.finkeys.set_finalizer_policy(indices).pubkeys;
+      produce_and_push_block();
+      return pubkeys;
+   };
+
+   clear_votes_and_reset_lib();
+
+   // produce 2 blocks that will be made final after the three `add_set_finalizers` below
+   // ------------------------------------------------------------------------------------
+   for (size_t i=0; i<4; ++i) {
+      produce_and_push_block();
+      process_votes(1, num_nodes - 1);
+   }
+
+   // run three set_finalizers in 3 blocks without voting
+   // they will be in `proposed` state with different block numbers.
+   // -------------------------------------------------------------
+   auto pubkeys1 = add_set_finalizers(1); // will be generation == 2
+   auto pubkeys2 = add_set_finalizers(2); // will be generation == 3
+   auto pubkeys3 = add_set_finalizers(3); // will be generation == 4
+
+   // produce_and_push 3 blocks. The last one will make finality skip over the three
+   // `add_set_finalizers` blocks above, so that they all become `pending` on the same block.
+   // ---------------------------------------------------------------------------------------
+   for (size_t i=0; i<3; ++i) {
+      produce_and_push_block();
+      process_votes(1, num_nodes - 1);
+
+      // make sure we don't have duplicate finalizer policies for the same block number
+      // in either `proposed` or `pending` state
+      // ------------------------------------------------------------------------------
+      node0.check_head_finalizer_policy(1u, fin_policy_pubkeys_0);
+   }
+
+   // now *only* the third `set_finalizers` should be `pending`, the one with
+   // `generation == 4`. The other policies must have been overwritten since they all
+   // became `pending` at the same block.
+   //
+   // we need another 3-chain to make that block final.
+   // -------------------------------------------------------------------------------
+   for (size_t i=0; i<3; ++i) {
+      produce_and_push_block();
+      process_votes(1, num_nodes - 1);
+      node0.check_head_finalizer_policy(1u, fin_policy_pubkeys_0);
+   }
+
+   // when we receive the votes of that last block finishing the 3-chain, the active
+   // `finalizer_policy` finally changes.
+   // ------------------------------------------------------------------------------
+   produce_and_push_block();
+   process_votes(1, num_nodes - 1);
+   node0.check_head_finalizer_policy(4u, pubkeys3);
+
+} FC_LOG_AND_RETHROW() }
+
+
+
+
 BOOST_AUTO_TEST_SUITE_END()