Merge pull request #537 from AntelopeIO/gh_534

Fix issue with fsi handling and `block_ref` creation.

libraries/chain/block_header_state.cpp

@@ -294,11 +294,7 @@ void finish_next(const block_header_state& prev,
 
    // finality_core
    // -------------
-   block_ref parent_block {
-      .block_id        = prev.block_id,
-      .timestamp       = prev.timestamp(),
-      .finality_digest = prev.compute_finality_digest()
-   };
+   block_ref parent_block = prev.make_block_ref();
    next_header_state.core = prev.core.next(parent_block, f_ext.qc_claim);
 
    // finalizer policy

libraries/chain/block_state.cpp

@@ -71,11 +71,7 @@ block_state_ptr block_state::create_if_genesis_block(const block_state_legacy& b
    assert(f_ext.new_finalizer_policy_diff); // required by transition mechanism
    result.active_finalizer_policy = std::make_shared<finalizer_policy>(finalizer_policy{}.apply_diff(std::move(*f_ext.new_finalizer_policy_diff)));
 
-   block_ref genesis_block_ref {
-      .block_id  = bsp.id(),
-      .timestamp = bsp.timestamp()
-   };
-   result.core = finality_core::create_core_for_genesis_block(genesis_block_ref);
+   result.core = finality_core::create_core_for_genesis_block(bsp.id(), bsp.timestamp());
 
    result.last_pending_finalizer_policy_digest = fc::sha256::hash(*result.active_finalizer_policy);
    result.last_pending_finalizer_policy_start_timestamp = bsp.timestamp();

libraries/chain/controller.cpp

@@ -1416,12 +1416,14 @@ struct controller_impl {
          // information for those finalizers that don't already have one. This typically should be done when
          // we create the non-legacy fork_db, as from this point we may need to cast votes to participate
          // to the IF consensus. See https://github.com/AntelopeIO/leap/issues/2070#issuecomment-1941901836
-         auto start_block = chain_head; // doesn't matter this is not updated for IRREVERSIBLE, can be in irreversible mode and be a finalizer
-         auto lib_block   = chain_head;
+         block_ref ref = block_handle_accessor::apply<block_ref>(chain_head,
+            overloaded{[&](const block_state_legacy_ptr& head) { return block_ref{}; },
+                       [&](const block_state_ptr& head) { return head->make_block_ref(); }});
+         // doesn't matter chain_head is not updated for IRREVERSIBLE, cannot be in irreversible mode and be a finalizer
          my_finalizers.set_default_safety_information(
-            finalizer_safety_information{ .last_vote_range_start = block_timestamp_type(0),
-                                          .last_vote = {start_block.id(), start_block.block_time()},
-                                          .lock      = {lib_block.id(),   lib_block.block_time()} });
+            finalizer_safety_information{ .last_vote                             = ref,
+                                          .lock                                  = ref,
+                                          .votes_forked_since_latest_strong_vote = false});
       }
    }
 
@@ -1615,12 +1617,10 @@ struct controller_impl {
                         // information for those finalizers that don't already have one. This typically should be done when
                         // we create the non-legacy fork_db, as from this point we may need to cast votes to participate
                         // to the IF consensus. See https://github.com/AntelopeIO/leap/issues/2070#issuecomment-1941901836
-                        auto start_block = chain_head;
-                        auto lib_block   = chain_head;
                         my_finalizers.set_default_safety_information(
-                           finalizer_safety_information{ .last_vote_range_start = block_timestamp_type(0),
-                                                         .last_vote = {start_block.id(), start_block.block_time()},
-                                                         .lock      = {lib_block.id(),   lib_block.block_time()} });
+                           finalizer_safety_information{.last_vote                             = prev->make_block_ref(),
+                                                        .lock                                  = prev->make_block_ref(),
+                                                        .votes_forked_since_latest_strong_vote = false});
                      }
                   }
                });
@@ -1996,19 +1996,19 @@ struct controller_impl {
             auto set_finalizer_defaults = [&](auto& forkdb) -> void {
                auto lib = forkdb.root();
                my_finalizers.set_default_safety_information(
-                  finalizer_safety_information{ .last_vote_range_start = block_timestamp_type(0),
-                                                .last_vote = {},
-                                                .lock      = {lib->id(), lib->timestamp()} });
+                  finalizer_safety_information{ .last_vote = {},
+                                                .lock      = lib->make_block_ref(),
+                                                .votes_forked_since_latest_strong_vote = false });
             };
             fork_db.apply_s<void>(set_finalizer_defaults);
          } else {
             // we are past the IF transition.
             auto set_finalizer_defaults = [&](auto& forkdb) -> void {
                auto lib = forkdb.root();
                my_finalizers.set_default_safety_information(
-                  finalizer_safety_information{ .last_vote_range_start = block_timestamp_type(0),
-                                                .last_vote = {},
-                                                .lock      = {lib->id(), lib->timestamp()} });
+                  finalizer_safety_information{ .last_vote = {},
+                                                .lock      = lib->make_block_ref(),
+                                                .votes_forked_since_latest_strong_vote = false });
             };
             fork_db.apply_s<void>(set_finalizer_defaults);
          }

libraries/chain/finality/finality_core.cpp

@@ -19,9 +19,9 @@ block_num_type block_ref::block_num() const {
  *  @post returned core has latest_qc_claim() == {.block_num=block_num, .is_strong_qc=false}
  *  @post returned core has last_final_block_num() == block_num
  */
-finality_core finality_core::create_core_for_genesis_block(const block_ref& genesis_block)
+finality_core finality_core::create_core_for_genesis_block(const block_id_type& block_id, block_time_type timestamp)
 {
-   block_num_type block_num = block_header::num_from_id(genesis_block.block_id);
+   block_num_type block_num = block_header::num_from_id(block_id);
    return finality_core {
       .links                        = {
          qc_link{
@@ -31,7 +31,7 @@ finality_core finality_core::create_core_for_genesis_block(const block_ref& gene
          },
       },
       .refs                         = {},
-      .genesis_timestamp            = genesis_block.timestamp
+      .genesis_timestamp            = timestamp
    };
 
    // Invariants 1 to 7 can be easily verified to be satisfied for the returned core.

libraries/chain/finality/finalizer.cpp

@@ -13,16 +13,12 @@ finalizer::vote_result finalizer::decide_vote(const block_state_ptr& bsp) {
    // just activated and we can proceed
 
    if (!res.monotony_check) {
-      if (fsi.last_vote.empty()) {
-         fc_dlog(vote_logger, "monotony check failed, block ${bn} ${p}, cannot vote, fsi.last_vote empty",
-                 ("bn", bsp->block_num())("p", bsp->id()));
-      } else {
-         if (vote_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
-               fc_dlog(vote_logger, "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));
-            }
+      assert(!fsi.last_vote.empty()); // otherwise `res.monotony_check` would be true.
+      if (vote_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
+            fc_dlog(vote_logger, "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;
@@ -68,23 +64,34 @@ finalizer::vote_result finalizer::decide_vote(const block_state_ptr& bsp) {
    // 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(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 = bsp->core.extends(fsi.last_vote.block_id);
+      // if `fsi.last_vote` is not set, it will be initialized with a timestamp slot of 0, so we
+      // don't need to check fsi.last_vote.empty()
+      // ---------------------------------------------------------------------------------------
+      bool voting_strong = fsi.last_vote.timestamp <= bsp->core.latest_qc_block_timestamp();
+
+      if (!voting_strong) {
+         // we can vote strong if the proposal is a descendant of (i.e. extends) our last vote id AND
+         // the latest (weak) vote did not mask a prior (weak) vote for a block not on the same branch.
+         // -------------------------------------------------------------------------------------------
+         voting_strong = !fsi.votes_forked_since_latest_strong_vote && bsp->core.extends(fsi.last_vote.block_id);
       }
 
-      fsi.last_vote             = { bsp->id(), bsp->timestamp() };
-      fsi.last_vote_range_start = p_start;
-
       auto& latest_qc_claim__block_ref = bsp->core.get_block_reference(bsp->core.latest_qc_claim().block_num);
-      if (voting_strong && latest_qc_claim__block_ref.timestamp > fsi.lock.timestamp) {
-         fsi.lock = latest_qc_claim__block_ref;
+      if (voting_strong) {
+         fsi.votes_forked_since_latest_strong_vote = false;             // always reset to false on strong vote
+         if (latest_qc_claim__block_ref.timestamp > fsi.lock.timestamp)
+            fsi.lock = latest_qc_claim__block_ref;
+      } else {
+         // On a weak vote, if `votes_forked_since_latest_strong_vote` was already true, then it should remain true.
+         // The only way `votes_forked_since_latest_strong_vote` can change from false to true is on a weak vote
+         // for a block b where the last_vote references a block that is not an ancestor of b
+         // --------------------------------------------------------------------------------------------------------
+         fsi.votes_forked_since_latest_strong_vote =
+            fsi.votes_forked_since_latest_strong_vote || !bsp->core.extends(fsi.last_vote.block_id);
       }
 
+      fsi.last_vote = bsp->make_block_ref();
+
       res.decision = voting_strong ? vote_decision::strong_vote : vote_decision::weak_vote;
    }
 
@@ -99,9 +106,9 @@ finalizer::vote_result finalizer::decide_vote(const block_state_ptr& bsp) {
 bool finalizer::maybe_update_fsi(const block_state_ptr& bsp) {
    auto& latest_qc_claim__block_ref = bsp->core.get_block_reference(bsp->core.latest_qc_claim().block_num);
    if (latest_qc_claim__block_ref.timestamp > fsi.lock.timestamp && bsp->timestamp() > fsi.last_vote.timestamp) {
-      fsi.lock = latest_qc_claim__block_ref;
-      fsi.last_vote             = { bsp->id(), bsp->timestamp() };
-      fsi.last_vote_range_start = bsp->core.latest_qc_block_timestamp();
+      fsi.lock                                  = latest_qc_claim__block_ref;
+      fsi.last_vote                             = bsp->make_block_ref();
+      fsi.votes_forked_since_latest_strong_vote = false; // always reset to false on strong vote
       return true;
    }
    return false;
@@ -237,20 +244,17 @@ my_finalizers_t::fsi_map my_finalizers_t::load_finalizer_safety_info() {
    try {
       persist_file.seek(0);
 
-      // read magic number. Can be `fsi_t::magic_unversioned` (for files without embedded version number)
-      // or `fsi_t::magic` (for files with a version number right after `magic`).
-      // ------------------------------------------------------------------------------------------------
+      // read magic number. must be `fsi_t::magic`
+      // -----------------------------------------
       uint64_t magic = 0;
       fc::raw::unpack(persist_file, magic);
-      EOS_ASSERT(magic == fsi_t::magic_unversioned || magic == fsi_t::magic, finalizer_safety_exception,
+      EOS_ASSERT(magic == fsi_t::magic, finalizer_safety_exception,
                  "bad magic number in finalizer safety persistence file: ${p}", ("p", persist_file_path));
 
-      // If we expect the file to contain a version number, read it. We can load files with older
-      // versions, but it better not be a file with a version higher that the running nodeos understands.
-      // ------------------------------------------------------------------------------------------------
-      uint64_t file_version = 0; // default version for file with magic == fsi_t::magic_unversioned
-      if (magic != fsi_t::magic_unversioned)
-         fc::raw::unpack(persist_file, file_version);
+      // We can load files with older, but not files with a version higher that the running nodeos understands.
+      // -----------------------------------------------------------------------------------------------------
+      uint64_t file_version = 0; // current file version
+      fc::raw::unpack(persist_file, file_version);
       EOS_ASSERT(file_version <= current_safety_file_version, finalizer_safety_exception,
                  "Incorrect version number in finalizer safety persistence file: ${p}", ("p", persist_file_path));
 

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

@@ -164,6 +164,8 @@ struct block_header_state {
    digest_type compute_base_digest() const;
    digest_type compute_finality_digest() const;
 
+   block_ref make_block_ref() const { return block_ref{block_id, timestamp(), compute_finality_digest() }; }
+
    // Returns true if the block is a Savanna Genesis Block.
    // This method is applicable to any transition block which is re-classified as a Savanna block.
    bool is_savanna_genesis_block() const { return core.is_genesis_block_num(block_num()); }

libraries/chain/include/eosio/chain/block_state.hpp

@@ -127,6 +127,8 @@ struct block_state : public block_header_state {     // block_header_state provi
       return timestamp() > fc::time_point::now() - fc::seconds(30);
    }
 
+   block_ref make_block_ref() const { return block_ref{block_id, timestamp(), strong_digest }; } // use the cached `finality_digest`
+
    protocol_feature_activation_set_ptr get_activated_protocol_features() const { return block_header_state::activated_protocol_features; }
    // build next valid structure from current one with input of next
    valid_t new_valid(const block_header_state& bhs, const digest_type& action_mroot, const digest_type& strong_digest) const;

libraries/chain/include/eosio/chain/finality/finality_core.hpp

@@ -10,6 +10,13 @@ using block_time_type = chain::block_timestamp_type;
 
 struct block_ref
 {
+   block_ref(const block_id_type& block_id, block_time_type timestamp, const digest_type& finality_digest)
+      : block_id(block_id)
+      , timestamp(timestamp)
+      , finality_digest(finality_digest) {}
+
+   block_ref() = default; // creates `empty` representation where `block_id.empty() == true`
+
    block_id_type    block_id;
    block_time_type  timestamp;
    digest_type      finality_digest;  // finality digest associated with the block
@@ -81,7 +88,7 @@ struct finality_core
     *  @post returned core has latest_qc_claim() == {.block_num=block_num, .is_strong_qc=false}
     *  @post returned core has last_final_block_num() == block_num
     */
-   static finality_core create_core_for_genesis_block(const block_ref& genesis_block);
+   static finality_core create_core_for_genesis_block(const block_id_type& block_id, block_time_type timestamp);
 
    bool is_genesis_core() const { return refs.empty(); }
 

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

@@ -32,19 +32,18 @@ namespace eosio::chain {
 
    // ----------------------------------------------------------------------------------------
    struct finalizer_safety_information {
-      block_timestamp_type last_vote_range_start;
       block_ref            last_vote;
       block_ref            lock;
+      bool                 votes_forked_since_latest_strong_vote {false};
 
-      static constexpr uint64_t magic_unversioned = 0x5AFE11115AFE1111ull;
-      static constexpr uint64_t magic             = 0x5AFE11115AFE1112ull;
+      static constexpr uint64_t magic = 0x5AFE11115AFE1111ull;
 
       static finalizer_safety_information unset_fsi() { return {}; }
 
       auto operator==(const finalizer_safety_information& o) const {
-         return last_vote_range_start == o.last_vote_range_start &&
-            last_vote == o.last_vote &&
-            lock == o.lock;
+         return last_vote == o.last_vote &&
+            lock == o.lock &&
+            votes_forked_since_latest_strong_vote == o.votes_forked_since_latest_strong_vote;
       }
    };
 
@@ -71,7 +70,7 @@ namespace eosio::chain {
    // ----------------------------------------------------------------------------------------
    struct my_finalizers_t {
    public:
-      static constexpr uint64_t current_safety_file_version = 1;
+      static constexpr uint64_t current_safety_file_version = 0;
 
       using fsi_t   = finalizer_safety_information;
       using fsi_map = std::map<bls_public_key, fsi_t>;
@@ -174,7 +173,7 @@ namespace eosio::chain {
 
 namespace std {
    inline std::ostream& operator<<(std::ostream& os, const eosio::chain::finalizer_safety_information& fsi) {
-      os << "fsi(" << fsi.last_vote_range_start.slot << ", " << fsi.last_vote << ", " << fsi.lock << ")";
+      os << "fsi(" << fsi.last_vote << ", " << fsi.lock << ", " << fsi.votes_forked_since_latest_strong_vote << ")";
       return os;
    }
 
@@ -192,5 +191,5 @@ namespace std {
    }
 }
 
-FC_REFLECT(eosio::chain::finalizer_safety_information, (last_vote_range_start)(last_vote)(lock))
+FC_REFLECT(eosio::chain::finalizer_safety_information, (last_vote)(lock)(votes_forked_since_latest_strong_vote))
 FC_REFLECT_ENUM(eosio::chain::finalizer::vote_decision, (strong_vote)(weak_vote)(no_vote))