From efffdb2276ece22577334e3d1fc02ffafbf5806f Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Fri, 23 Aug 2024 12:17:18 -0500 Subject: [PATCH 01/14] Add new checksum datastream --- libraries/libfc/include/fc/io/cfile.hpp | 10 ++ .../libfc/include/fc/io/datastream_crc.hpp | 109 ++++++++++++++++++ 2 files changed, 119 insertions(+) create mode 100644 libraries/libfc/include/fc/io/datastream_crc.hpp diff --git a/libraries/libfc/include/fc/io/cfile.hpp b/libraries/libfc/include/fc/io/cfile.hpp index f3cd05ded1..5ca5c90461 100644 --- a/libraries/libfc/include/fc/io/cfile.hpp +++ b/libraries/libfc/include/fc/io/cfile.hpp @@ -329,6 +329,16 @@ class datastream : public fc::cfile { return true; } + bool read( char* d, size_t s ) { + cfile::read( d, s ); + return true; + } + + bool write(const char* d, size_t s) { + cfile::write(d, s); + return true; + } + fc::cfile& storage() { return *this; } const fc::cfile& storage() const { return *this; } diff --git a/libraries/libfc/include/fc/io/datastream_crc.hpp b/libraries/libfc/include/fc/io/datastream_crc.hpp new file mode 100644 index 0000000000..d447cc6379 --- /dev/null +++ b/libraries/libfc/include/fc/io/datastream_crc.hpp @@ -0,0 +1,109 @@ +#pragma once +#include +#include + +namespace fc { + +/** +* Provides a datasteam wrapper around another datastream for calculation of checksum. +* Example use: +* fc::datastream persist_cfile; +* fc::datastream_crc> persist_stream{persist_cfile}; +* +* persist_stream.seekp(0); +* fc::raw::pack(persist_stream, 'a'); +* uint32_t cs = persist_stream.check_sum(); +* fc::raw::pack(persist_stream, cs); // write checksum to file +* // ... +* persist_stream.seekp(0); +* char c; +* fc::raw::unpack(persist_stream, c); +* uint32_t calc_cs = persist_stream.check_sum(); +* uint32_t cs; +* fc::raw::unpack(persist_stream, cs); +* FC_ASSERT(calc_cs == cs, "checksum not equal"); +*/ +template +class datastream_crc { +public: + // ds must outlive datasteam_crc + explicit datastream_crc( DS& ds ) : ds_(ds) {} + + void skip(size_t s) { + ds_.skip(s); + } + + bool read(char* d, size_t s) { + bool r = ds_.read(d, s); + crc_.process_bytes(d, s); + return r; + } + + bool write(const char* d, size_t s) { + crc_.process_bytes(d, s); + return ds_.write(d, s); + } + + bool put(char c) { + crc_.process_byte(c); + return ds_.put(c); + } + + bool get(unsigned char& c) { + bool r = ds_.get(c); + crc_.process_byte(c); + return r; + } + + bool get(char& c) { + bool r = ds_.get(c); + crc_.process_byte(c); + return r; + } + + auto pos() const { + return ds_.pos(); + } + + bool valid() const { + return ds_.valid(); + } + + // only use with p==0, otherwise use seekp() below + bool seekp(size_t p) { + if (p == 0) { + crc_.reset(); + return ds_.seekp(0); + } + return false; + } + + size_t tellp() const { + return ds_.tellp(); + } + size_t remaining() const { + return ds_.remaining(); + } + + // extension to datastream + + bool seekp(size_t p, const CRC& crc) { + crc_ = crc; + return ds_.seekp(p); + } + + uint32_t checksum() const { + return crc_.checksum(); + } + + CRC crc() const { + return crc_; + } + +private: + DS& ds_; + CRC crc_; +}; + + +} // namespace fc From 7f8bd4e9fa1df194cc4618d3892b1ddd04ff5769 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Fri, 23 Aug 2024 12:32:37 -0500 Subject: [PATCH 02/14] GH-617 Add version 1 of safety file. Uses checksum to verify contents. --- libraries/chain/finality/finalizer.cpp | 108 +++++++++++++----- .../eosio/chain/finality/finalizer.hpp | 27 ++++- unittests/test-data/fsi/safety_v1.dat | Bin 0 -> 730 bytes 3 files changed, 100 insertions(+), 35 deletions(-) create mode 100644 unittests/test-data/fsi/safety_v1.dat diff --git a/libraries/chain/finality/finalizer.cpp b/libraries/chain/finality/finalizer.cpp index c88153accf..5aeaf95cb5 100644 --- a/libraries/chain/finality/finalizer.cpp +++ b/libraries/chain/finality/finalizer.cpp @@ -175,47 +175,80 @@ void my_finalizers_t::maybe_update_fsi(const block_state_ptr& bsp, const qc_t& r void my_finalizers_t::save_finalizer_safety_info() const { - if (!persist_file.is_open()) { + if (!cfile_ds.is_open()) { EOS_ASSERT(!persist_file_path.empty(), finalizer_safety_exception, "path for storing finalizer safety information file not specified"); if (!std::filesystem::exists(persist_file_path.parent_path())) std::filesystem::create_directories(persist_file_path.parent_path()); - persist_file.set_file_path(persist_file_path); - persist_file.open(fc::cfile::truncate_rw_mode); + cfile_ds.set_file_path(persist_file_path); + cfile_ds.open(fc::cfile::truncate_rw_mode); } try { - static_assert(sizeof(finalizer_safety_information) == 152, "If size changes then need to handle loading old files"); - static_assert(sizeof(decltype(bls_public_key{}.affine_non_montgomery_le())) == 96, - "If size changes then need to handle loading old files, fc::pack uses affine_non_montgomery_le()"); - - persist_file.seek(0); - fc::raw::pack(persist_file, fsi_t::magic); - fc::raw::pack(persist_file, current_safety_file_version); - fc::raw::pack(persist_file, (uint64_t)(finalizers.size() + inactive_safety_info.size())); - for (const auto& [pub_key, f] : finalizers) { - fc::raw::pack(persist_file, pub_key); - fc::raw::pack(persist_file, f.fsi); - } - if (!inactive_safety_info_written) { + // optimize by only calculating crc for inactive once + if (inactive_safety_info_written_pos == 0) { + persist_file.seekp(0); + fc::raw::pack(persist_file, fsi_t::magic); + fc::raw::pack(persist_file, current_safety_file_version); + uint64_t size = finalizers.size() + inactive_safety_info.size(); + fc::raw::pack(persist_file, size); + // save also the fsi that was originally present in the file, but which applied to // finalizers not configured anymore. for (const auto& [pub_key, fsi] : inactive_safety_info) { fc::raw::pack(persist_file, pub_key); fc::raw::pack(persist_file, fsi); } - inactive_safety_info_written = true; + inactive_safety_info_written_pos = persist_file.tellp(); + inactive_crc32 = persist_file.crc(); + } else { + persist_file.seekp(inactive_safety_info_written_pos, inactive_crc32); + } + + // active finalizers + for (const auto& [pub_key, f] : finalizers) { + fc::raw::pack(persist_file, pub_key); + fc::raw::pack(persist_file, f.fsi); } - persist_file.flush(); + + uint32_t cs = persist_file.checksum(); + fc::raw::pack(persist_file, cs); + + cfile_ds.flush(); } FC_LOG_AND_RETHROW() } // ---------------------------------------------------------------------------------------- + +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 #include #include +#include #include #include #include @@ -70,21 +71,34 @@ namespace eosio::chain { // ---------------------------------------------------------------------------------------- struct my_finalizers_t { public: - static constexpr uint64_t current_safety_file_version = 0; + /// + /// 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 - Variable length FSI with inactive safety info written at the beginning of + /// the file. Uses crc32 checksum to verify data on read. + /// + static constexpr uint64_t safety_file_version_0 = 0; + static constexpr uint64_t safety_file_version_1 = 1; + static constexpr uint64_t current_safety_file_version = safety_file_version_1; using fsi_t = finalizer_safety_information; using fsi_map = std::map; using vote_t = std::tuple; private: + using persist_file_t = fc::datastream_crc>; + using finalizer_map_t = std::map; + const std::filesystem::path persist_file_path; // where we save the safety data std::atomic has_voted{false}; // true if this node has voted and updated safety info mutable std::mutex mtx; - mutable fc::datastream persist_file; // we want to keep the file open for speed - std::map finalizers; // the active finalizers for this node, loaded at startup, not mutated afterwards + mutable fc::datastream cfile_ds; // we want to keep the file open for speed + mutable persist_file_t persist_file{cfile_ds};// we want to calculate checksum + finalizer_map_t finalizers; // the active finalizers for this node, loaded at startup, not mutated afterwards fsi_map inactive_safety_info; // loaded at startup, not mutated afterwards fsi_t default_fsi = fsi_t::unset_fsi(); // default provided at spring startup - mutable bool inactive_safety_info_written{false}; + mutable long inactive_safety_info_written_pos{0}; + mutable boost::crc_32_type inactive_crc32; // cached value public: explicit my_finalizers_t(const std::filesystem::path& persist_file_path) @@ -167,6 +181,11 @@ namespace eosio::chain { // for testing purposes only, not thread safe const fsi_t& get_fsi(const bls_public_key& k) { return finalizers[k].fsi; } void set_fsi(const bls_public_key& k, const fsi_t& fsi) { finalizers[k].fsi = fsi; } + + private: + void load_finalizer_safety_info_v0(fsi_map& res); + void load_finalizer_safety_info_v1(fsi_map& res); + }; } diff --git a/unittests/test-data/fsi/safety_v1.dat b/unittests/test-data/fsi/safety_v1.dat new file mode 100644 index 0000000000000000000000000000000000000000..58c22860ca01910b4e2c6c0ff93e9f5fcfcac52d GIT binary patch literal 730 zcmV<00ww(s5&l{c5&l{M0000000003000000001BK*&7%09Avcbu^tsiuL%v3=VL< z21S2S8<(4L`i*-wxREd8jE2wRVwv{pLf*;|&mo?cG*2iW9s0lARXNOqWE@@f@R|ga z09pd!afyB%i%W7K1PqkD`~#Vy&EhH)$jSffwk@eD*inS5`3Zp!(<4EL$*6_*uMN|d z&D~l1%K!iX>R7=MV06WC!8xGPA-$MGZ3=Ja+e-4(hbnu8kRM|kys}18S>m4&quK>T z7%i$rQ!HjdcllxyJ>7Vbj*DU-$N&HUHw5nq%4(%e+eQL(CdF)I9FpIDy&ZL8VE71o zGoct?0AP|#F2yNb0s>tVg>DY$$EccbuBK~5dPQUJCYSLZ6H0I1u4;RL- zcLM(A;+Q|FfpITJb`G4vVACdLm`xTLdA}CT-SiJmZ08WL(}uq$4X3x?wCbK>d=7PG z=rJ8*!>fDv<5K=&I4sY5q?sTuELo8<)K2SH?-2t4000b(?^*zBw(fK)!NggT_Py)e zNgI4px;y}kR>a5J0e~WFWgw`rPO`hKmXVppF26Z>PGuQCODVAc6Lnwe&ANEUx&AgRcAyP%J M(hlVH03dkuwU{$fDgXcg literal 0 HcmV?d00001 From 5315ce679a5f3519552582a688c02c1f62d434c8 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Fri, 23 Aug 2024 13:03:45 -0500 Subject: [PATCH 03/14] Add Boost::crc --- libraries/libfc/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/libfc/CMakeLists.txt b/libraries/libfc/CMakeLists.txt index 18b1b2e7a2..c9c88f83a8 100644 --- a/libraries/libfc/CMakeLists.txt +++ b/libraries/libfc/CMakeLists.txt @@ -103,7 +103,7 @@ if(APPLE) find_library(corefoundation_framework CoreFoundation) endif() target_link_libraries( fc PUBLIC Boost::date_time Boost::chrono Boost::iostreams Boost::interprocess Boost::multi_index Boost::dll - Boost::multiprecision Boost::beast Boost::asio Boost::thread Threads::Threads + Boost::multiprecision Boost::beast Boost::asio Boost::thread Boost::crc Threads::Threads boringssl ZLIB::ZLIB ${PLATFORM_SPECIFIC_LIBS} ${CMAKE_DL_LIBS} secp256k1 bls12-381 ${security_framework} ${corefoundation_framework}) add_subdirectory( test ) From c1a311f0e730ef92f0c57c996fc178b66765e377 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Fri, 23 Aug 2024 15:58:29 -0500 Subject: [PATCH 04/14] 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 05/14] 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 06/14] 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 e1c7e9c6566ca56ad5a47d643d75053342f55074 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Mon, 26 Aug 2024 12:37:39 -0500 Subject: [PATCH 07/14] GH-635 Prevent node from syncing ahead do to irreversible block notice message --- plugins/net_plugin/net_plugin.cpp | 75 ++++++++++++++++++------------- 1 file changed, 43 insertions(+), 32 deletions(-) diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index fdbe8726f7..6733757631 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -245,14 +245,15 @@ namespace eosio { private: constexpr static auto stage_str( stages s ); - bool set_state( stages newstate ); - bool is_sync_required( uint32_t fork_head_block_num ); // call with locked mutex - void request_next_chunk( const connection_ptr& conn = connection_ptr() ) REQUIRES(sync_mtx); - connection_ptr find_next_sync_node(); // call with locked mutex - void start_sync( const connection_ptr& c, uint32_t target ); // locks mutex - bool sync_recently_active() const; - bool verify_catchup( const connection_ptr& c, uint32_t num, const block_id_type& id ); // locks mutex - uint32_t active_sync_fetch_span() const; + bool set_state( stages newstate ); + bool is_sync_required( uint32_t fork_head_block_num ) const REQUIRES(sync_mtx); + bool is_sync_request_ahead_allowed() const REQUIRES(sync_mtx); + void request_next_chunk( const connection_ptr& conn = connection_ptr() ) REQUIRES(sync_mtx); + connection_ptr find_next_sync_node(); // call with locked mutex + void start_sync( const connection_ptr& c, uint32_t target ); // locks mutex + bool sync_recently_active() const; + bool verify_catchup( const connection_ptr& c, uint32_t num, const block_id_type& id ); // locks mutex + uint32_t active_sync_fetch_span(bool log) const; public: enum class closing_mode { immediately, // closing connection immediately @@ -2000,22 +2001,26 @@ namespace eosio { { } - uint32_t sync_manager::active_sync_fetch_span() const { + uint32_t sync_manager::active_sync_fetch_span(bool log) const { const uint32_t constrained_reversible_remaining = [&]() -> uint32_t { const int32_t reversible_remaining = my_impl->chain_plug->chain().max_reversible_blocks_allowed(); if (reversible_remaining <= 0) { - auto fork_db_size = my_impl->chain_plug->chain().fork_db_size(); - fc_wlog(logger, "max-reversible-blocks exceeded by ${ex}, fork_db_size ${fs}", - ("ex", -reversible_remaining)("fs", fork_db_size)); + if (log) { + auto fork_db_size = my_impl->chain_plug->chain().fork_db_size(); + fc_wlog(logger, "max-reversible-blocks exceeded by ${ex}, fork_db_size ${fs}", + ("ex", -reversible_remaining)("fs", fork_db_size)); + } return 0; } return reversible_remaining; }(); if (constrained_reversible_remaining < sync_fetch_span) { - auto fork_db_size = my_impl->chain_plug->chain().fork_db_size(); - fc_wlog(logger, "sync-fetch-span ${sfs} restricted to ${r} by max-reversible-blocks, fork_db_size ${fs}", - ("sfs", sync_fetch_span)("r", constrained_reversible_remaining)("fs", fork_db_size)); + if (log) { + auto fork_db_size = my_impl->chain_plug->chain().fork_db_size(); + fc_wlog(logger, "sync-fetch-span ${sfs} restricted to ${r} by max-reversible-blocks, fork_db_size ${fs}", + ("sfs", sync_fetch_span)("r", constrained_reversible_remaining)("fs", fork_db_size)); + } return constrained_reversible_remaining; } return sync_fetch_span; @@ -2164,7 +2169,7 @@ namespace eosio { bool request_sent = false; if( sync_last_requested_num != sync_known_lib_num ) { uint32_t start = sync_next_expected_num; - auto fetch_span = active_sync_fetch_span(); + auto fetch_span = active_sync_fetch_span(true); uint32_t end = start + fetch_span - 1; if( end > sync_known_lib_num ) end = sync_known_lib_num; @@ -2194,7 +2199,7 @@ namespace eosio { } ); } - bool sync_manager::is_sync_required( uint32_t fork_head_block_num ) REQUIRES(sync_mtx) { + bool sync_manager::is_sync_required( uint32_t fork_head_block_num ) const REQUIRES(sync_mtx) { fc_dlog( logger, "last req = ${req}, last recv = ${recv} known = ${known} our fhead = ${h}", ("req", sync_last_requested_num)( "recv", sync_next_expected_num )( "known", sync_known_lib_num ) ("h", fork_head_block_num ) ); @@ -2203,6 +2208,18 @@ namespace eosio { sync_next_expected_num < sync_last_requested_num ); } + bool sync_manager::is_sync_request_ahead_allowed() const REQUIRES(sync_mtx) { + if (sync_next_expected_num >= sync_last_requested_num) { + // do not allow to get too far ahead (sync_fetch_span) of chain head + auto fetch_span = active_sync_fetch_span(false); + // use chain head instead of fork head so we do not get too far ahead of applied blocks + uint32_t head = my_impl->get_chain_head_num(); + if (sync_next_expected_num < head + fetch_span) + return true; + } + return false; + } + // called from c's connection strand void sync_manager::start_sync(const connection_ptr& c, uint32_t target) { fc::unique_lock g_sync( sync_mtx ); @@ -2222,7 +2239,7 @@ namespace eosio { set_state( lib_catchup ); sync_last_requested_num = 0; sync_next_expected_num = chain_info.lib_num + 1; - } else if (sync_next_expected_num >= sync_last_requested_num) { + } else if (is_sync_request_ahead_allowed()) { // break } else { peer_dlog(c, "already syncing, start sync ignored"); @@ -2571,20 +2588,14 @@ namespace eosio { ("bn", blk_num)("kn", sync_known_lib_num)); send_handshakes_when_synced = true; } else { - if (blk_num >= sync_last_requested_num) { - // do not allow to get too far ahead (sync_fetch_span) of chain head - auto fetch_span = active_sync_fetch_span(); - // use chain head instead of fork head so we do not get too far ahead of applied blocks - uint32_t head = my_impl->get_chain_head_num(); - if (blk_num < head + fetch_span) { - // block was not applied, possibly because we already have the block - fc_dlog(logger, "Requesting ${fs} blocks ahead, head: ${h} fhead ${fh} blk_num: ${bn} sync_next_expected_num ${nen} " - "sync_last_requested_num: ${lrn}, sync_last_requested_block: ${lrb}", - ("fs", fetch_span)("h", head)("fh", my_impl->get_fork_head_num()) - ("bn", blk_num)("nen", sync_next_expected_num) - ("lrn", sync_last_requested_num)("lrb", c->sync_last_requested_block)); - request_next_chunk(); - } + if (is_sync_request_ahead_allowed()) { + // block was not applied, possibly because we already have the block + fc_dlog(logger, "Requesting ${fs} blocks ahead, head: ${h} fhead ${fh} blk_num: ${bn} sync_next_expected_num ${nen} " + "sync_last_requested_num: ${lrn}, sync_last_requested_block: ${lrb}", + ("fs", active_sync_fetch_span(false))("h", my_impl->get_chain_head_num())("fh", my_impl->get_fork_head_num()) + ("bn", blk_num)("nen", sync_next_expected_num) + ("lrn", sync_last_requested_num)("lrb", c->sync_last_requested_block)); + request_next_chunk(); } } } else { // blk_applied From f86652104441e0cb02cc01fec6a33c4c0d71f8a6 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Mon, 26 Aug 2024 12:40:47 -0500 Subject: [PATCH 08/14] GH-635 Fix accidental code formating --- plugins/net_plugin/net_plugin.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index 6733757631..db6d5186be 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -245,15 +245,15 @@ namespace eosio { private: constexpr static auto stage_str( stages s ); - bool set_state( stages newstate ); - bool is_sync_required( uint32_t fork_head_block_num ) const REQUIRES(sync_mtx); - bool is_sync_request_ahead_allowed() const REQUIRES(sync_mtx); - void request_next_chunk( const connection_ptr& conn = connection_ptr() ) REQUIRES(sync_mtx); - connection_ptr find_next_sync_node(); // call with locked mutex - void start_sync( const connection_ptr& c, uint32_t target ); // locks mutex - bool sync_recently_active() const; - bool verify_catchup( const connection_ptr& c, uint32_t num, const block_id_type& id ); // locks mutex - uint32_t active_sync_fetch_span(bool log) const; + bool set_state( stages newstate ); + bool is_sync_required( uint32_t fork_head_block_num ) const REQUIRES(sync_mtx); + bool is_sync_request_ahead_allowed() const REQUIRES(sync_mtx); + void request_next_chunk( const connection_ptr& conn = connection_ptr() ) REQUIRES(sync_mtx); + connection_ptr find_next_sync_node(); // call with locked mutex + void start_sync( const connection_ptr& c, uint32_t target ); // locks mutex + bool sync_recently_active() const; + bool verify_catchup( const connection_ptr& c, uint32_t num, const block_id_type& id ); // locks mutex + uint32_t active_sync_fetch_span(bool log) const; public: enum class closing_mode { immediately, // closing connection immediately From 33fcb662ea5a7313a7b9f8245f084e1471257bbe Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Mon, 26 Aug 2024 19:40:03 -0500 Subject: [PATCH 09/14] GH-635 Pass block num to is_sync_request_ahead_allowed() --- plugins/net_plugin/net_plugin.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index db6d5186be..0e6209520f 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -247,7 +247,7 @@ namespace eosio { constexpr static auto stage_str( stages s ); bool set_state( stages newstate ); bool is_sync_required( uint32_t fork_head_block_num ) const REQUIRES(sync_mtx); - bool is_sync_request_ahead_allowed() const REQUIRES(sync_mtx); + bool is_sync_request_ahead_allowed(block_num_type blk_num) const REQUIRES(sync_mtx); void request_next_chunk( const connection_ptr& conn = connection_ptr() ) REQUIRES(sync_mtx); connection_ptr find_next_sync_node(); // call with locked mutex void start_sync( const connection_ptr& c, uint32_t target ); // locks mutex @@ -2208,13 +2208,13 @@ namespace eosio { sync_next_expected_num < sync_last_requested_num ); } - bool sync_manager::is_sync_request_ahead_allowed() const REQUIRES(sync_mtx) { - if (sync_next_expected_num >= sync_last_requested_num) { + bool sync_manager::is_sync_request_ahead_allowed(block_num_type blk_num) const REQUIRES(sync_mtx) { + if (blk_num >= sync_last_requested_num) { // do not allow to get too far ahead (sync_fetch_span) of chain head auto fetch_span = active_sync_fetch_span(false); // use chain head instead of fork head so we do not get too far ahead of applied blocks uint32_t head = my_impl->get_chain_head_num(); - if (sync_next_expected_num < head + fetch_span) + if (blk_num < head + fetch_span) return true; } return false; @@ -2239,7 +2239,7 @@ namespace eosio { set_state( lib_catchup ); sync_last_requested_num = 0; sync_next_expected_num = chain_info.lib_num + 1; - } else if (is_sync_request_ahead_allowed()) { + } else if (is_sync_request_ahead_allowed(sync_next_expected_num)) { // break } else { peer_dlog(c, "already syncing, start sync ignored"); @@ -2588,7 +2588,7 @@ namespace eosio { ("bn", blk_num)("kn", sync_known_lib_num)); send_handshakes_when_synced = true; } else { - if (is_sync_request_ahead_allowed()) { + if (is_sync_request_ahead_allowed(blk_num)) { // block was not applied, possibly because we already have the block fc_dlog(logger, "Requesting ${fs} blocks ahead, head: ${h} fhead ${fh} blk_num: ${bn} sync_next_expected_num ${nen} " "sync_last_requested_num: ${lrn}, sync_last_requested_block: ${lrb}", From e8ae0aa3c454837db4bc31fcd397cdad480c4d23 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 27 Aug 2024 05:36:17 -0500 Subject: [PATCH 10/14] 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; From e6792659b576575b1712329cfd626a624d2e6141 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 27 Aug 2024 07:28:03 -0500 Subject: [PATCH 11/14] GH-641 Remove max-reversible-blocks shutdown option. With this commit, max-reversible-blocks does nothing. --- libraries/chain/controller.cpp | 22 ------------ .../chain/include/eosio/chain/controller.hpp | 7 +--- plugins/chain_plugin/chain_plugin.cpp | 10 +++--- plugins/net_plugin/net_plugin.cpp | 34 ++----------------- tests/CMakeLists.txt | 4 --- tests/nodeos_startup_catchup.py | 2 -- tests/terminate-scenarios-test.py | 19 +---------- 7 files changed, 9 insertions(+), 89 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index 70a373efe2..a0f4c18293 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -4833,19 +4833,6 @@ struct controller_impl { return conf.block_validation_mode == validation_mode::LIGHT || conf.trusted_producers.count(producer); } - int32_t max_reversible_blocks_allowed() const { - if (conf.max_reversible_blocks == 0) - return std::numeric_limits::max(); - - return fork_db.apply( - [&](const fork_database_legacy_t& forkdb) { - return std::numeric_limits::max(); - }, - [&](const fork_database_if_t& forkdb) { - return conf.max_reversible_blocks - forkdb.size(); - }); - } - bool should_terminate(block_num_type reversible_block_num) const { assert(reversible_block_num > 0); if (conf.terminate_at_block > 0 && conf.terminate_at_block <= reversible_block_num) { @@ -4853,11 +4840,6 @@ struct controller_impl { ("n", reversible_block_num)("num", conf.terminate_at_block) ); return true; } - if (max_reversible_blocks_allowed() <= 0) { - elog("Exceeded max reversible blocks allowed, fork db size ${s} >= max-reversible-blocks ${m}", - ("s", fork_db_size())("m", conf.max_reversible_blocks)); - return true; - } return false; } @@ -5666,10 +5648,6 @@ bool controller::should_terminate() const { return my->should_terminate(); } -int32_t controller:: max_reversible_blocks_allowed() const { - return my->max_reversible_blocks_allowed(); -} - const apply_handler* controller::find_apply_handler( account_name receiver, account_name scope, action_name act ) const { auto native_handler_scope = my->apply_handlers.find( receiver ); diff --git a/libraries/chain/include/eosio/chain/controller.hpp b/libraries/chain/include/eosio/chain/controller.hpp index 6e4ce2b061..7514ccb102 100644 --- a/libraries/chain/include/eosio/chain/controller.hpp +++ b/libraries/chain/include/eosio/chain/controller.hpp @@ -392,15 +392,10 @@ namespace eosio::chain { db_read_mode get_read_mode()const; validation_mode get_validation_mode()const; - /// @return true if terminate-at-block reached, or max-reversible-blocks reached + /// @return true if terminate-at-block reached /// not-thread-safe bool should_terminate() const; - /// Difference between max-reversible-blocks and fork database size. - /// Can return MAX_INT32 if disabled or pre-Savanna - /// @return the number of reversible blocks still allowed - int32_t max_reversible_blocks_allowed() const; - void set_subjective_cpu_leeway(fc::microseconds leeway); std::optional get_subjective_cpu_leeway() const; void set_greylist_limit( uint32_t limit ); diff --git a/plugins/chain_plugin/chain_plugin.cpp b/plugins/chain_plugin/chain_plugin.cpp index 4448e69519..e342374a5d 100644 --- a/plugins/chain_plugin/chain_plugin.cpp +++ b/plugins/chain_plugin/chain_plugin.cpp @@ -361,8 +361,8 @@ void chain_plugin::set_program_options(options_description& cli, options_descrip "'none' - EOS VM OC tier-up is completely disabled.\n") #endif ("enable-account-queries", bpo::value()->default_value(false), "enable queries to find accounts by various metadata.") - ("max-reversible-blocks", bpo::value()->default_value(config::default_max_reversible_blocks), - "Approximate maximum allowed reversible blocks before shutdown. Will shut down if limit reached. Specify 0 to disable.") + ("max-reversible-blocks", bpo::value()->default_value(config::default_max_reversible_blocks), + "Maximum allowed reversible blocks beyond irreversible before block production is paused. Specify 0 to disable.") ("transaction-retry-max-storage-size-gb", bpo::value(), "Maximum size (in GiB) allowed to be allocated for the Transaction Retry feature. Setting above 0 enables this feature.") ("transaction-retry-interval-sec", bpo::value()->default_value(20), @@ -954,11 +954,9 @@ void chain_plugin_impl::plugin_initialize(const variables_map& options) { account_queries_enabled = options.at("enable-account-queries").as(); - chain_config->max_reversible_blocks = options.at("max-reversible-blocks").as(); - if (chain_config->max_reversible_blocks == -1) // allow -1 or 0 for disable - chain_config->max_reversible_blocks = 0; + chain_config->max_reversible_blocks = options.at("max-reversible-blocks").as(); EOS_ASSERT(chain_config->max_reversible_blocks >= 0, plugin_config_exception, - "max-reversible-blocks ${m} must be > 0", ("m", chain_config->max_reversible_blocks)); + "max-reversible-blocks ${m} must be >= 0", ("m", chain_config->max_reversible_blocks)); chain_config->integrity_hash_on_start = options.at("integrity-hash-on-start").as(); chain_config->integrity_hash_on_stop = options.at("integrity-hash-on-stop").as(); diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index 0e6209520f..09787977ae 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -253,7 +253,6 @@ namespace eosio { void start_sync( const connection_ptr& c, uint32_t target ); // locks mutex bool sync_recently_active() const; bool verify_catchup( const connection_ptr& c, uint32_t num, const block_id_type& id ); // locks mutex - uint32_t active_sync_fetch_span(bool log) const; public: enum class closing_mode { immediately, // closing connection immediately @@ -2001,31 +2000,6 @@ namespace eosio { { } - uint32_t sync_manager::active_sync_fetch_span(bool log) const { - const uint32_t constrained_reversible_remaining = [&]() -> uint32_t { - const int32_t reversible_remaining = my_impl->chain_plug->chain().max_reversible_blocks_allowed(); - if (reversible_remaining <= 0) { - if (log) { - auto fork_db_size = my_impl->chain_plug->chain().fork_db_size(); - fc_wlog(logger, "max-reversible-blocks exceeded by ${ex}, fork_db_size ${fs}", - ("ex", -reversible_remaining)("fs", fork_db_size)); - } - return 0; - } - return reversible_remaining; - }(); - - if (constrained_reversible_remaining < sync_fetch_span) { - if (log) { - auto fork_db_size = my_impl->chain_plug->chain().fork_db_size(); - fc_wlog(logger, "sync-fetch-span ${sfs} restricted to ${r} by max-reversible-blocks, fork_db_size ${fs}", - ("sfs", sync_fetch_span)("r", constrained_reversible_remaining)("fs", fork_db_size)); - } - return constrained_reversible_remaining; - } - return sync_fetch_span; - } - constexpr auto sync_manager::stage_str(stages s) { switch (s) { case in_sync : return "in sync"; @@ -2169,8 +2143,7 @@ namespace eosio { bool request_sent = false; if( sync_last_requested_num != sync_known_lib_num ) { uint32_t start = sync_next_expected_num; - auto fetch_span = active_sync_fetch_span(true); - uint32_t end = start + fetch_span - 1; + uint32_t end = start + sync_fetch_span - 1; if( end > sync_known_lib_num ) end = sync_known_lib_num; if( end > 0 && end >= start ) { @@ -2211,10 +2184,9 @@ namespace eosio { bool sync_manager::is_sync_request_ahead_allowed(block_num_type blk_num) const REQUIRES(sync_mtx) { if (blk_num >= sync_last_requested_num) { // do not allow to get too far ahead (sync_fetch_span) of chain head - auto fetch_span = active_sync_fetch_span(false); // use chain head instead of fork head so we do not get too far ahead of applied blocks uint32_t head = my_impl->get_chain_head_num(); - if (blk_num < head + fetch_span) + if (blk_num < head + sync_fetch_span) return true; } return false; @@ -2592,7 +2564,7 @@ namespace eosio { // block was not applied, possibly because we already have the block fc_dlog(logger, "Requesting ${fs} blocks ahead, head: ${h} fhead ${fh} blk_num: ${bn} sync_next_expected_num ${nen} " "sync_last_requested_num: ${lrn}, sync_last_requested_block: ${lrb}", - ("fs", active_sync_fetch_span(false))("h", my_impl->get_chain_head_num())("fh", my_impl->get_fork_head_num()) + ("fs", sync_fetch_span)("h", my_impl->get_chain_head_num())("fh", my_impl->get_fork_head_num()) ("bn", blk_num)("nen", sync_next_expected_num) ("lrn", sync_last_requested_num)("lrb", c->sync_last_requested_block)); request_next_chunk(); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index f7bb1faf06..3453f4ac2e 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -244,10 +244,6 @@ add_test(NAME terminate-scenarios-if-test-replay-pass-transition COMMAND tests/t set_property(TEST terminate-scenarios-if-test-replay-pass-transition PROPERTY LABELS nonparallelizable_tests) add_test(NAME terminate-scenarios-if-test-hard_replay-pass-transition COMMAND tests/terminate-scenarios-test.py -c hardReplay --terminate-at-block 150 --kill-sig term --activate-if ${UNSHARE} WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) set_property(TEST terminate-scenarios-if-test-hard_replay-pass-transition PROPERTY LABELS nonparallelizable_tests) -add_test(NAME terminate-scenarios-if-test-max-reversible-blocks-sync COMMAND tests/terminate-scenarios-test.py -c resync --max-reversible-blocks 25 --kill-sig term --activate-if ${UNSHARE} WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) -set_property(TEST terminate-scenarios-if-test-max-reversible-blocks-sync PROPERTY LABELS nonparallelizable_tests) -add_test(NAME terminate-scenarios-if-test-max-reversible-blocks-replay COMMAND tests/terminate-scenarios-test.py -c replay --max-reversible-blocks 25 --kill-sig term --activate-if ${UNSHARE} WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) -set_property(TEST terminate-scenarios-if-test-max-reversible-blocks-replay PROPERTY LABELS nonparallelizable_tests) add_test(NAME validate_dirty_db_test COMMAND tests/validate-dirty-db.py -v ${UNSHARE} WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) set_property(TEST validate_dirty_db_test PROPERTY LABELS nonparallelizable_tests) add_test(NAME keosd_auto_launch_test COMMAND tests/keosd_auto_launch_test.py WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) diff --git a/tests/nodeos_startup_catchup.py b/tests/nodeos_startup_catchup.py index 625535799c..dabedb2120 100755 --- a/tests/nodeos_startup_catchup.py +++ b/tests/nodeos_startup_catchup.py @@ -67,8 +67,6 @@ specificExtraNodeosArgs[pnodes+7] = f' --sync-fetch-span 1597 ' specificExtraNodeosArgs[pnodes+8] = f' --sync-fetch-span 6765 ' specificExtraNodeosArgs[pnodes+9] = f' --sync-fetch-span 28657 ' - if not activateIF: - specificExtraNodeosArgs[pnodes+9] += " --max-reversible-blocks 2 " # should be ignored for pre-savanna blocks specificExtraNodeosArgs[pnodes+10] = f' --sync-fetch-span 89 --read-mode irreversible ' specificExtraNodeosArgs[pnodes+11] = f' --sync-fetch-span 377 --read-mode irreversible ' if cluster.launch(prodCount=prodCount, specificExtraNodeosArgs=specificExtraNodeosArgs, activateIF=activateIF, onlyBios=False, diff --git a/tests/terminate-scenarios-test.py b/tests/terminate-scenarios-test.py index af6b3a1671..2ee9b77ab1 100755 --- a/tests/terminate-scenarios-test.py +++ b/tests/terminate-scenarios-test.py @@ -13,8 +13,6 @@ # (--delete-all-blocks), "hardReplay"(--hard-replay-blockchain), and "none" to indicate what kind of restart flag should # be used. This is one of the only test that actually verify that nodeos terminates with a good exit status. # -# Also used to test max-reversible-blocks in savanna. -# ############################################################### @@ -22,8 +20,6 @@ errorExit=Utils.errorExit appArgs=AppArgs() -appArgs.add(flag="--max-reversible-blocks", type=int, help="pass max-reversible-blocks to nodeos", default=0) - args=TestHelper.parse_args({"-d","-c","--kill-sig","--keep-logs" ,"--activate-if","--dump-error-details","-v","--leave-running" ,"--terminate-at-block","--unshared"}, applicationSpecificArgs=appArgs) @@ -37,7 +33,6 @@ activateIF=args.activate_if dumpErrorDetails=args.dump_error_details terminate=args.terminate_at_block -maxReversibleBlocks=args.max_reversible_blocks seed=1 Utils.Debug=debug @@ -49,11 +44,6 @@ try: TestHelper.printSystemInfo("BEGIN") - if maxReversibleBlocks > 0 and not activateIF: - errorExit("--max-reversible-blocks requires --activate-if") - if maxReversibleBlocks > 0 and terminate > 0: - errorExit("Test supports only one of --max-reversible-blocks requires --terminate-at-block") - cluster.setWalletMgr(walletMgr) cluster.setChainStrategy(chainSyncStrategyStr) @@ -64,11 +54,6 @@ if cluster.launch(pnodes=pnodes, totalNodes=total_nodes, totalProducers=pnodes, topo=topo, delay=delay, activateIF=activateIF) is False: errorExit("Failed to stand up eos cluster.") - # killing bios node means LIB will not advance as it hosts the only finalizer - if maxReversibleBlocks > 0: - Print ("Kill bios node") - cluster.biosNode.kill(signal.SIGTERM) - Print ("Wait for Cluster stabilization") # wait for cluster to start producing blocks if not cluster.waitOnClusterBlockNumSync(3): @@ -88,13 +73,11 @@ Print ("Relaunch dead cluster node instance.") nodeArg = "--terminate-at-block %d" % terminate if terminate > 0 else "" - if nodeArg == "": - nodeArg = "--max-reversible-blocks %d --production-pause-vote-timeout-ms 0" % maxReversibleBlocks if maxReversibleBlocks > 0 else "" if nodeArg != "": if chainSyncStrategyStr == "hardReplay": nodeArg += " --truncate-at-block %d" % terminate nodeArg += " --enable-stale-production " - if cluster.relaunchEosInstances(nodeArgs=nodeArg, waitForTerm=(terminate > 0 or maxReversibleBlocks > 0)) is False: + if cluster.relaunchEosInstances(nodeArgs=nodeArg, waitForTerm=(terminate > 0)) is False: errorExit("Failed to relaunch Eos instance") Print("nodeos instance relaunched.") From 45162f2117355427325fc40a5b9cd683f10d49c8 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 27 Aug 2024 07:58:11 -0500 Subject: [PATCH 12/14] GH-641 Move max-reversible-blocks to producer_plugin since it only affects block production now. --- libraries/chain/include/eosio/chain/controller.hpp | 1 - plugins/chain_plugin/chain_plugin.cpp | 6 ------ plugins/producer_plugin/producer_plugin.cpp | 5 +++++ 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/libraries/chain/include/eosio/chain/controller.hpp b/libraries/chain/include/eosio/chain/controller.hpp index 7514ccb102..94de0b7f83 100644 --- a/libraries/chain/include/eosio/chain/controller.hpp +++ b/libraries/chain/include/eosio/chain/controller.hpp @@ -95,7 +95,6 @@ namespace eosio::chain { uint32_t sig_cpu_bill_pct = chain::config::default_sig_cpu_bill_pct; uint16_t chain_thread_pool_size = chain::config::default_controller_thread_pool_size; uint16_t vote_thread_pool_size = 0; - int32_t max_reversible_blocks = chain::config::default_max_reversible_blocks; bool read_only = false; bool force_all_checks = false; bool disable_replay_opts = false; diff --git a/plugins/chain_plugin/chain_plugin.cpp b/plugins/chain_plugin/chain_plugin.cpp index e342374a5d..3a33b11e85 100644 --- a/plugins/chain_plugin/chain_plugin.cpp +++ b/plugins/chain_plugin/chain_plugin.cpp @@ -361,8 +361,6 @@ void chain_plugin::set_program_options(options_description& cli, options_descrip "'none' - EOS VM OC tier-up is completely disabled.\n") #endif ("enable-account-queries", bpo::value()->default_value(false), "enable queries to find accounts by various metadata.") - ("max-reversible-blocks", bpo::value()->default_value(config::default_max_reversible_blocks), - "Maximum allowed reversible blocks beyond irreversible before block production is paused. Specify 0 to disable.") ("transaction-retry-max-storage-size-gb", bpo::value(), "Maximum size (in GiB) allowed to be allocated for the Transaction Retry feature. Setting above 0 enables this feature.") ("transaction-retry-interval-sec", bpo::value()->default_value(20), @@ -954,10 +952,6 @@ void chain_plugin_impl::plugin_initialize(const variables_map& options) { account_queries_enabled = options.at("enable-account-queries").as(); - chain_config->max_reversible_blocks = options.at("max-reversible-blocks").as(); - EOS_ASSERT(chain_config->max_reversible_blocks >= 0, plugin_config_exception, - "max-reversible-blocks ${m} must be >= 0", ("m", chain_config->max_reversible_blocks)); - chain_config->integrity_hash_on_start = options.at("integrity-hash-on-start").as(); chain_config->integrity_hash_on_stop = options.at("integrity-hash-on-stop").as(); diff --git a/plugins/producer_plugin/producer_plugin.cpp b/plugins/producer_plugin/producer_plugin.cpp index 8f5a42960a..9f8a869d20 100644 --- a/plugins/producer_plugin/producer_plugin.cpp +++ b/plugins/producer_plugin/producer_plugin.cpp @@ -508,6 +508,7 @@ class producer_plugin_impl : public std::enable_shared_from_this _last_producer_vote_received; fc::microseconds _production_pause_vote_timeout; fc::microseconds _max_irreversible_block_age_us; + block_num_type _max_reversible_blocks{0}; // produce-block-offset is in terms of the complete round, internally use calculated value for each block of round fc::microseconds _produce_block_cpu_effort; fc::time_point _pending_block_deadline; @@ -1169,6 +1170,8 @@ void producer_plugin::set_program_options( "Setting this value (in milliseconds) will restrict the allowed transaction execution time to a value potentially lower than the on-chain consensus max_transaction_cpu_usage value.") ("max-irreversible-block-age", bpo::value()->default_value( -1 ), "Limits the maximum age (in seconds) of the DPOS Irreversible Block for a chain this node will produce blocks on (use negative value to indicate unlimited)") + ("max-reversible-blocks", bpo::value()->default_value(config::default_max_reversible_blocks), + "Maximum allowed reversible blocks beyond irreversible before block production is paused. Specify 0 to disable.") ("producer-name,p", boost::program_options::value>()->composing()->multitoken(), "ID of producer controlled by this node (e.g. inita; may specify multiple times)") ("signature-provider", boost::program_options::value>()->composing()->multitoken()->default_value( @@ -1308,6 +1311,8 @@ void producer_plugin_impl::plugin_initialize(const boost::program_options::varia _max_irreversible_block_age_us = fc::seconds(options.at("max-irreversible-block-age").as()); + _max_reversible_blocks = options.at("max-reversible-blocks").as(); + auto max_incoming_transaction_queue_size = options.at("incoming-transaction-queue-size-mb").as() * 1024 * 1024; EOS_ASSERT(max_incoming_transaction_queue_size > 0, plugin_config_exception, From 15ae10f613c1c1d6883348b18bfd6a1b21c42c0a Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 27 Aug 2024 07:59:01 -0500 Subject: [PATCH 13/14] GH-641 Pause production when max-reversible-blocks is reached. --- libraries/chain/include/eosio/chain/block_handle.hpp | 3 ++- plugins/producer_plugin/producer_plugin.cpp | 8 +++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/libraries/chain/include/eosio/chain/block_handle.hpp b/libraries/chain/include/eosio/chain/block_handle.hpp index fc2fc31afd..48a53d17ef 100644 --- a/libraries/chain/include/eosio/chain/block_handle.hpp +++ b/libraries/chain/include/eosio/chain/block_handle.hpp @@ -26,7 +26,8 @@ struct block_handle { bool is_valid() const { return !_bsp.valueless_by_exception() && std::visit([](const auto& bsp) { return !!bsp; }, _bsp); } - uint32_t block_num() const { return std::visit([](const auto& bsp) { return bsp->block_num(); }, _bsp); } + block_num_type block_num() const { return std::visit([](const auto& bsp) { return bsp->block_num(); }, _bsp); } + block_num_type irreversible_blocknum() const { return std::visit([](const auto& bsp) { return bsp->irreversible_blocknum(); }, _bsp); } block_timestamp_type timestamp() const { return std::visit([](const auto& bsp) { return bsp->timestamp(); }, _bsp); }; time_point block_time() const { return std::visit([](const auto& bsp) { return time_point{bsp->timestamp()}; }, _bsp); }; const block_id_type& id() const { return std::visit([](const auto& bsp) -> const block_id_type& { return bsp->id(); }, _bsp); } diff --git a/plugins/producer_plugin/producer_plugin.cpp b/plugins/producer_plugin/producer_plugin.cpp index 9f8a869d20..b605f2307a 100644 --- a/plugins/producer_plugin/producer_plugin.cpp +++ b/plugins/producer_plugin/producer_plugin.cpp @@ -1935,7 +1935,8 @@ producer_plugin_impl::start_block_result producer_plugin_impl::start_block() { return start_block_result::failed; } - block_num_type head_block_num = chain.head().block_num(); + block_handle head = chain.head(); + block_num_type head_block_num = head.block_num(); const fc::time_point now = fc::time_point::now(); const block_timestamp_type block_time = calculate_pending_block_time(); const uint32_t pending_block_num = head_block_num + 1; @@ -1981,6 +1982,11 @@ producer_plugin_impl::start_block_result producer_plugin_impl::start_block() { ("ov", _last_other_vote_received.load(std::memory_order_relaxed))("bt", _accepted_block_time)); _pending_block_mode = pending_block_mode::speculating; not_producing_when_time = true; + } else if (head_block_num - head.irreversible_blocknum() >= _max_reversible_blocks) { + fc_elog(_log, "Not producing block because max-reversible-blocks ${m} reached, head ${h}, lib ${l}.", + ("m", _max_reversible_blocks)("h", head_block_num)("l", head.irreversible_blocknum())); + _pending_block_mode = pending_block_mode::speculating; + not_producing_when_time = true; } // !not_producing_when_time to avoid tight spin because of error or paused production From 03c79ec506803244a42290bfeb6f92654c01fb44 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 27 Aug 2024 09:49:38 -0500 Subject: [PATCH 14/14] GH-641 Check max-reversible-blocks is not zero --- plugins/producer_plugin/producer_plugin.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/producer_plugin/producer_plugin.cpp b/plugins/producer_plugin/producer_plugin.cpp index b605f2307a..a69f72532c 100644 --- a/plugins/producer_plugin/producer_plugin.cpp +++ b/plugins/producer_plugin/producer_plugin.cpp @@ -1982,7 +1982,7 @@ producer_plugin_impl::start_block_result producer_plugin_impl::start_block() { ("ov", _last_other_vote_received.load(std::memory_order_relaxed))("bt", _accepted_block_time)); _pending_block_mode = pending_block_mode::speculating; not_producing_when_time = true; - } else if (head_block_num - head.irreversible_blocknum() >= _max_reversible_blocks) { + } else if (_max_reversible_blocks > 0 && head_block_num - head.irreversible_blocknum() > _max_reversible_blocks) { fc_elog(_log, "Not producing block because max-reversible-blocks ${m} reached, head ${h}, lib ${l}.", ("m", _max_reversible_blocks)("h", head_block_num)("l", head.irreversible_blocknum())); _pending_block_mode = pending_block_mode::speculating;