From c1a311f0e730ef92f0c57c996fc178b66765e377 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Fri, 23 Aug 2024 15:58:29 -0500 Subject: [PATCH 1/4] GH-621 Use a other_branch_latest_time block_timestamp instead of a bool votes_forked_since_latest_strong_vote for vote decision --- libraries/chain/controller.cpp | 24 +++--- libraries/chain/finality/finalizer.cpp | 75 ++++++++++++------- .../eosio/chain/finality/finalizer.hpp | 12 +-- unittests/finalizer_tests.cpp | 6 +- unittests/finalizer_vote_tests.cpp | 2 +- 5 files changed, 68 insertions(+), 51 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index e616b55f33..8ad18281b4 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -1425,9 +1425,9 @@ struct controller_impl { [&](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 = ref, - .lock = ref, - .votes_forked_since_latest_strong_vote = false}); + finalizer_safety_information{ .last_vote = ref, + .lock = ref, + .other_branch_latest_time = {} }); } } @@ -1636,9 +1636,9 @@ struct controller_impl { // 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 my_finalizers.set_default_safety_information( - finalizer_safety_information{.last_vote = prev->make_block_ref(), - .lock = prev->make_block_ref(), - .votes_forked_since_latest_strong_vote = false}); + finalizer_safety_information{.last_vote = prev->make_block_ref(), + .lock = prev->make_block_ref(), + .other_branch_latest_time = {} }); } } }); @@ -2040,9 +2040,9 @@ 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 = {}, - .lock = lib->make_block_ref(), - .votes_forked_since_latest_strong_vote = false }); + finalizer_safety_information{ .last_vote = {}, + .lock = lib->make_block_ref(), + .other_branch_latest_time = {} }); }; fork_db.apply_s(set_finalizer_defaults); } else { @@ -2050,9 +2050,9 @@ 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 = {}, - .lock = lib->make_block_ref(), - .votes_forked_since_latest_strong_vote = false }); + finalizer_safety_information{.last_vote = {}, + .lock = lib->make_block_ref(), + .other_branch_latest_time = {} }); }; fork_db.apply_s(set_finalizer_defaults); } diff --git a/libraries/chain/finality/finalizer.cpp b/libraries/chain/finality/finalizer.cpp index 5aeaf95cb5..c0ccb8ac76 100644 --- a/libraries/chain/finality/finalizer.cpp +++ b/libraries/chain/finality/finalizer.cpp @@ -61,35 +61,37 @@ 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) { - // 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); + const auto latest_qc_block_timestamp = bsp->core.latest_qc_block_timestamp(); + + // If `fsi.last_vote` is not set, it will be initialized with a timestamp slot of 0, + // which means `fsi.last_vote.timestamp` would always be less than or equal + // to `latest_qc_block_timestamp`. + // So, we don't need to separately check for the case where `fsi.last_vote.empty()` is true. + if (fsi.last_vote.timestamp <= latest_qc_block_timestamp) { + res.decision = vote_decision::strong_vote; + } else if (bsp->core.extends(fsi.last_vote.block_id)) { + // If `fsi.other_branch_latest_time` is not present, it will have a timestamp slot of + // 0, which means it will always be less than or equal to `latest_qc_block_timestamp`. + // So, we don't need to separately check for the case where + // `fsi.other_branch_latest_time` is not present. + if (fsi.other_branch_latest_time <= latest_qc_block_timestamp) { + res.decision = vote_decision::strong_vote; + } else { + res.decision = vote_decision::weak_vote; + } + } else { + res.decision = vote_decision::weak_vote; + fsi.other_branch_latest_time = fsi.last_vote.timestamp; } - auto& latest_qc_claim__block_ref = bsp->core.get_block_reference(bsp->core.latest_qc_claim().block_num); - 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); + if (res.decision == vote_decision::strong_vote) { + fsi.other_branch_latest_time = {}; + if (latest_qc_block_timestamp > fsi.lock.timestamp) { + fsi.lock = bsp->core.get_block_reference(bsp->core.latest_qc_claim().block_num); + } } fsi.last_vote = bsp->make_block_ref(); - - res.decision = voting_strong ? vote_decision::strong_vote : vote_decision::weak_vote; } fc_dlog(vote_logger, "block=${bn} ${id}, liveness_check=${l}, safety_check=${s}, monotony_check=${m}, can vote=${can_vote}, voting=${v}, locked=${lbn} ${lid}", @@ -103,9 +105,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->make_block_ref(); - fsi.votes_forked_since_latest_strong_vote = false; // always reset to false on strong vote + fsi.lock = latest_qc_claim__block_ref; + fsi.last_vote = bsp->make_block_ref(); + fsi.other_branch_latest_time = {}; // always reset on strong vote return true; } return false; @@ -219,14 +221,27 @@ void my_finalizers_t::save_finalizer_safety_info() const { // ---------------------------------------------------------------------------------------- +// Corresponds to safety_file_version_0 +struct finalizer_safety_information_v0 { + block_ref last_vote; + block_ref lock; + bool votes_forked_since_latest_strong_vote {false}; +}; + void my_finalizers_t::load_finalizer_safety_info_v0(fsi_map& res) { uint64_t num_finalizers {0}; fc::raw::unpack(persist_file, num_finalizers); for (size_t i=0; i create_random_fsi(size_t count) { .lock = block_ref{sha256::hash("lock"s + std::to_string(i)), tstamp(i * 100), sha256::hash("lock_digest"s + std::to_string(i))}, - .votes_forked_since_latest_strong_vote = false + .other_branch_latest_time = {} }); if (i) assert(res.back() != res[0]); @@ -101,7 +101,7 @@ BOOST_AUTO_TEST_CASE( basic_finalizer_safety_file_io ) try { fsi_t fsi { .last_vote = proposals[6], .lock = proposals[2], - .votes_forked_since_latest_strong_vote = false }; + .other_branch_latest_time = {} }; bls_keys_t k("alice"_n); bls_pub_priv_key_map_t local_finalizers = { { k.pubkey_str, k.privkey_str } }; @@ -134,7 +134,7 @@ BOOST_AUTO_TEST_CASE( corrupt_finalizer_safety_file ) try { fsi_t fsi { .last_vote = proposals[6], .lock = proposals[2], - .votes_forked_since_latest_strong_vote = false }; + .other_branch_latest_time = {} }; bls_keys_t k("alice"_n); bls_pub_priv_key_map_t local_finalizers = { { k.pubkey_str, k.privkey_str } }; diff --git a/unittests/finalizer_vote_tests.cpp b/unittests/finalizer_vote_tests.cpp index 7499a16f90..a9fe942b02 100644 --- a/unittests/finalizer_vote_tests.cpp +++ b/unittests/finalizer_vote_tests.cpp @@ -143,7 +143,7 @@ struct simulator_t { forkdb.reset_root(genesis); block_ref genesis_ref(genesis->id(), genesis->timestamp(), genesis->id()); - my_finalizer.fsi = fsi_t{genesis_ref, genesis_ref, false}; + my_finalizer.fsi = fsi_t{genesis_ref, genesis_ref, {}}; } vote_result vote(const bsp& p) { From a7a413c2bb6b51a083f8fc93f376a13bc89062ca Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Fri, 23 Aug 2024 16:20:54 -0500 Subject: [PATCH 2/4] GH-621 GCC wants explicit type used --- libraries/chain/controller.cpp | 8 ++++---- libraries/chain/finality/finalizer.cpp | 4 ++-- unittests/finalizer_tests.cpp | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index 8ad18281b4..70a373efe2 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -1427,7 +1427,7 @@ struct controller_impl { my_finalizers.set_default_safety_information( finalizer_safety_information{ .last_vote = ref, .lock = ref, - .other_branch_latest_time = {} }); + .other_branch_latest_time = block_timestamp_type{} }); } } @@ -1638,7 +1638,7 @@ struct controller_impl { my_finalizers.set_default_safety_information( finalizer_safety_information{.last_vote = prev->make_block_ref(), .lock = prev->make_block_ref(), - .other_branch_latest_time = {} }); + .other_branch_latest_time = block_timestamp_type{} }); } } }); @@ -2042,7 +2042,7 @@ struct controller_impl { my_finalizers.set_default_safety_information( finalizer_safety_information{ .last_vote = {}, .lock = lib->make_block_ref(), - .other_branch_latest_time = {} }); + .other_branch_latest_time = block_timestamp_type{} }); }; fork_db.apply_s(set_finalizer_defaults); } else { @@ -2052,7 +2052,7 @@ struct controller_impl { my_finalizers.set_default_safety_information( finalizer_safety_information{.last_vote = {}, .lock = lib->make_block_ref(), - .other_branch_latest_time = {} }); + .other_branch_latest_time = block_timestamp_type{} }); }; fork_db.apply_s(set_finalizer_defaults); } diff --git a/libraries/chain/finality/finalizer.cpp b/libraries/chain/finality/finalizer.cpp index c0ccb8ac76..9e39fc82d1 100644 --- a/libraries/chain/finality/finalizer.cpp +++ b/libraries/chain/finality/finalizer.cpp @@ -85,7 +85,7 @@ finalizer::vote_result finalizer::decide_vote(const block_state_ptr& bsp) { } if (res.decision == vote_decision::strong_vote) { - fsi.other_branch_latest_time = {}; + fsi.other_branch_latest_time = block_timestamp_type{}; if (latest_qc_block_timestamp > fsi.lock.timestamp) { fsi.lock = bsp->core.get_block_reference(bsp->core.latest_qc_claim().block_num); } @@ -107,7 +107,7 @@ bool finalizer::maybe_update_fsi(const block_state_ptr& bsp) { 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->make_block_ref(); - fsi.other_branch_latest_time = {}; // always reset on strong vote + fsi.other_branch_latest_time = block_timestamp_type{}; // always reset on strong vote return true; } return false; diff --git a/unittests/finalizer_tests.cpp b/unittests/finalizer_tests.cpp index 249a10660a..e78c66a81f 100644 --- a/unittests/finalizer_tests.cpp +++ b/unittests/finalizer_tests.cpp @@ -47,7 +47,7 @@ std::vector create_random_fsi(size_t count) { .lock = block_ref{sha256::hash("lock"s + std::to_string(i)), tstamp(i * 100), sha256::hash("lock_digest"s + std::to_string(i))}, - .other_branch_latest_time = {} + .other_branch_latest_time = block_timestamp_type{} }); if (i) assert(res.back() != res[0]); @@ -101,7 +101,7 @@ BOOST_AUTO_TEST_CASE( basic_finalizer_safety_file_io ) try { fsi_t fsi { .last_vote = proposals[6], .lock = proposals[2], - .other_branch_latest_time = {} }; + .other_branch_latest_time = block_timestamp_type{} }; bls_keys_t k("alice"_n); bls_pub_priv_key_map_t local_finalizers = { { k.pubkey_str, k.privkey_str } }; @@ -134,7 +134,7 @@ BOOST_AUTO_TEST_CASE( corrupt_finalizer_safety_file ) try { fsi_t fsi { .last_vote = proposals[6], .lock = proposals[2], - .other_branch_latest_time = {} }; + .other_branch_latest_time = block_timestamp_type{} }; bls_keys_t k("alice"_n); bls_pub_priv_key_map_t local_finalizers = { { k.pubkey_str, k.privkey_str } }; From 03777c652715666275110230d1c7d0df22226efc Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Fri, 23 Aug 2024 18:58:27 -0500 Subject: [PATCH 3/4] Add assert for not passing 0 --- libraries/libfc/include/fc/io/datastream_crc.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/libraries/libfc/include/fc/io/datastream_crc.hpp b/libraries/libfc/include/fc/io/datastream_crc.hpp index d447cc6379..42042e7dae 100644 --- a/libraries/libfc/include/fc/io/datastream_crc.hpp +++ b/libraries/libfc/include/fc/io/datastream_crc.hpp @@ -71,6 +71,7 @@ class datastream_crc { // only use with p==0, otherwise use seekp() below bool seekp(size_t p) { + assert(p == 0); if (p == 0) { crc_.reset(); return ds_.seekp(0); From e8ae0aa3c454837db4bc31fcd397cdad480c4d23 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 27 Aug 2024 05:36:17 -0500 Subject: [PATCH 4/4] GH-621 Update comment --- libraries/chain/include/eosio/chain/finality/finalizer.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libraries/chain/include/eosio/chain/finality/finalizer.hpp b/libraries/chain/include/eosio/chain/finality/finalizer.hpp index cbe51ec0fa..1ca4c90a2d 100644 --- a/libraries/chain/include/eosio/chain/finality/finalizer.hpp +++ b/libraries/chain/include/eosio/chain/finality/finalizer.hpp @@ -75,7 +75,9 @@ namespace eosio::chain { /// Version 0: Spring 1.0.0 RC2 - File has fixed packed sizes with inactive safety info written to the end /// of the file. Consists of [finalizer public_key, FSI].. /// Version 1: Spring 1.0.0 RC3 - File has inactive FSIs written at the beginning of the file. Uses crc32 - /// checksum to verify data on read. + /// checksum to verify data on read. Removes FSI + /// votes_forked_since_latest_strong_vote from the version 0 FSI and replaces it + /// with other_branch_latest_time. /// static constexpr uint64_t safety_file_version_0 = 0; static constexpr uint64_t safety_file_version_1 = 1;