From fab0bbed66add1230eab8bcea3c5f175ed196abe Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 1 Mar 2024 09:57:48 -0500 Subject: [PATCH 1/6] Use separate `by_best_branch` index definitions. --- libraries/chain/fork_database.cpp | 42 +++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/libraries/chain/fork_database.cpp b/libraries/chain/fork_database.cpp index fc918b545d..465e5f0165 100644 --- a/libraries/chain/fork_database.cpp +++ b/libraries/chain/fork_database.cpp @@ -76,24 +76,40 @@ namespace eosio::chain { using full_branch_t = fork_db_t::full_branch_t; using branch_pair_t = fork_db_t::branch_pair_t; + using by_best_branch_legacy_t = + ordered_unique, + composite_key, + const_mem_fun, + const_mem_fun, + const_mem_fun + >, + composite_key_compare, std::greater, std::greater, sha256_less + >>; + + using by_best_branch_if_t = + ordered_unique, + composite_key, + const_mem_fun, + const_mem_fun, + const_mem_fun, + const_mem_fun + >, + composite_key_compare, std::greater, std::greater, + std::greater, sha256_less> + >; + + using by_best_branch_t = std::conditional_t, + by_best_branch_if_t, + by_best_branch_legacy_t>; + using fork_multi_index_type = multi_index_container< bsp_t, indexed_by< hashed_unique, BOOST_MULTI_INDEX_CONST_MEM_FUN(bs_t, const block_id_type&, id), std::hash>, ordered_non_unique, const_mem_fun>, - ordered_unique, - composite_key, - global_fun, - global_fun, - global_fun, - const_mem_fun - >, - composite_key_compare, - std::greater, std::greater, std::greater, - sha256_less - > - > + by_best_branch_t > >; From 31da03fe224807094985b7767bf20aaa5bd4985f Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Fri, 1 Mar 2024 10:20:55 -0600 Subject: [PATCH 2/6] GH-2125 Remove unneeded bs acccessor methods. Use log_fork_comparison instead of fork_comparison struct. --- libraries/chain/controller.cpp | 2 +- libraries/chain/fork_database.cpp | 54 +++++++++---------- .../chain/include/eosio/chain/block_state.hpp | 3 +- .../include/eosio/chain/fork_database.hpp | 12 +---- 4 files changed, 30 insertions(+), 41 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index df4406e555..f968c838cb 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -3544,7 +3544,7 @@ struct controller_impl { } else if( new_head->id() != head->id() ) { ilog("switching forks from ${current_head_id} (block number ${current_head_num}) ${c} to ${new_head_id} (block number ${new_head_num}) ${n}", ("current_head_id", head->id())("current_head_num", head_block_num())("new_head_id", new_head->id())("new_head_num", new_head->block_num()) - ("c", fork_comparison{*head})("n", fork_comparison{*new_head})); + ("c", log_fork_comparison(*head))("n", log_fork_comparison(*new_head))); // not possible to log transaction specific infor when switching forks if (auto dm_logger = get_deep_mind_logger(false)) { diff --git a/libraries/chain/fork_database.cpp b/libraries/chain/fork_database.cpp index 465e5f0165..50d945c2d8 100644 --- a/libraries/chain/fork_database.cpp +++ b/libraries/chain/fork_database.cpp @@ -23,44 +23,43 @@ namespace eosio::chain { struct block_state_accessor { static bool is_valid(const block_state& bs) { return bs.is_valid(); } static void set_valid(block_state& bs, bool v) { bs.validated = v; } - static uint32_t last_final_block_num(const block_state& bs) { return bs.core.last_final_block_num(); } - static uint32_t lastest_qc_claim_block_num(const block_state& bs) { return bs.core.latest_qc_claim().block_num; } - static uint32_t block_height(const block_state& bs) { return bs.timestamp().slot; } }; struct block_state_legacy_accessor { static bool is_valid(const block_state_legacy& bs) { return bs.is_valid(); } static void set_valid(block_state_legacy& bs, bool v) { bs.validated = v; } - static uint32_t last_final_block_num(const block_state_legacy& bs) { return bs.irreversible_blocknum(); } - static uint32_t lastest_qc_claim_block_num(const block_state_legacy& bs) { return bs.irreversible_blocknum(); } - static uint32_t block_height(const block_state_legacy& bs) { return bs.block_num(); } }; - template - fork_comparison::fork_comparison(const BS& bs) - : valid(BS::fork_db_block_state_accessor_t::is_valid(bs)) - , last_final_block_num(BS::fork_db_block_state_accessor_t::last_final_block_num(bs)) - , lastest_qc_claim_block_num(BS::fork_db_block_state_accessor_t::lastest_qc_claim_block_num(bs)) - , block_height(BS::fork_db_block_state_accessor_t::block_height(bs)) - {} + std::string log_fork_comparison(const block_state& bs) { + std::string r; + r += "[ valid: " + std::to_string(block_state_accessor::is_valid(bs)) + ", "; + r += "last_final_block_num: " + std::to_string(bs.last_final_block_num()) + ", "; + r += "last_qc_block_num: " + std::to_string(bs.last_qc_block_num()) + ", "; + r += "timestamp: " + bs.timestamp().to_time_point().to_iso_string() + " ]"; + return r; + } + + std::string log_fork_comparison(const block_state_legacy& bs) { + std::string r; + r += "[ valid: " + std::to_string(block_state_legacy_accessor::is_valid(bs)) + ", "; + r += "irreversible_blocknum: " + std::to_string(bs.irreversible_blocknum()) + ", "; + r += "block_num: " + std::to_string(bs.block_num()) + ", "; + r += "timestamp: " + bs.timestamp().to_time_point().to_iso_string() + " ]"; + return r; + } struct by_block_id; struct by_best_branch; struct by_prev; - template - void log_bs(const char* desc, fork_database_impl& fork_db, const BS& lhs) { - using BSA = BS::fork_db_block_state_accessor_t; - dlog( "fork_db ${f}, ${d} ${bn}, last_final_block_num ${lfbn}, final_on_strong_qc_block_num ${fsbn}, lastest_qc_claim_block_num ${lbn}, block_height ${bh}, id ${id}", - ("f", (uint64_t)(&fork_db))("d", desc)("bn", lhs.block_num())("lfbn", BSA::last_final_block_num(lhs))("fsbn", BSA::final_on_strong_qc_block_num(lhs))("lbn", BSA::lastest_qc_claim_block_num(lhs))("bh", BSA::block_height(lhs))("id", lhs.id()) ); - } - // match comparison of by_best_branch - template - bool first_preferred( const BS& lhs, const BS& rhs ) { - using BSA = BS::fork_db_block_state_accessor_t; - return std::make_tuple(BSA::last_final_block_num(lhs), BSA::lastest_qc_claim_block_num(lhs), BSA::block_height(lhs)) > - std::make_tuple(BSA::last_final_block_num(rhs), BSA::lastest_qc_claim_block_num(rhs), BSA::block_height(rhs)); + bool first_preferred( const block_state& lhs, const block_state& rhs ) { + return std::make_tuple(lhs.last_final_block_num(), lhs.last_qc_block_num(), lhs.timestamp()) > + std::make_tuple(rhs.last_final_block_num(), rhs.last_qc_block_num(), rhs.timestamp()); + } + bool first_preferred( const block_state_legacy& lhs, const block_state_legacy& rhs ) { + return std::make_tuple(lhs.irreversible_blocknum(), lhs.block_num()) > + std::make_tuple(rhs.irreversible_blocknum(), rhs.block_num()); } template // either [block_state_legacy_ptr, block_state_ptr], same with block_header_state_ptr @@ -91,7 +90,7 @@ namespace eosio::chain { ordered_unique, composite_key, - const_mem_fun, + const_mem_fun, const_mem_fun, const_mem_fun, const_mem_fun @@ -776,9 +775,6 @@ namespace eosio::chain { } // do class instantiations - template struct fork_comparison; - template struct fork_comparison; - template class fork_database_t; template class fork_database_t; diff --git a/libraries/chain/include/eosio/chain/block_state.hpp b/libraries/chain/include/eosio/chain/block_state.hpp index 45e84fc453..16f0150cb5 100644 --- a/libraries/chain/include/eosio/chain/block_state.hpp +++ b/libraries/chain/include/eosio/chain/block_state.hpp @@ -56,7 +56,8 @@ struct block_state : public block_header_state { // block_header_state provi uint32_t block_num() const { return block_header_state::block_num(); } block_timestamp_type timestamp() const { return block_header_state::timestamp(); } const extensions_type& header_extensions() const { return block_header_state::header.header_extensions; } - uint32_t irreversible_blocknum() const { return core.last_final_block_num(); } + uint32_t irreversible_blocknum() const { return core.last_final_block_num(); } // backwards compatibility + uint32_t last_final_block_num() const { return core.last_final_block_num(); } std::optional get_best_qc() const; uint32_t last_qc_block_num() const { return core.latest_qc_claim().block_num; } uint32_t final_on_strong_qc_block_num() const { return core.final_on_strong_qc_block_num; } diff --git a/libraries/chain/include/eosio/chain/fork_database.hpp b/libraries/chain/include/eosio/chain/fork_database.hpp index b17f9e0552..e0983f9153 100644 --- a/libraries/chain/include/eosio/chain/fork_database.hpp +++ b/libraries/chain/include/eosio/chain/fork_database.hpp @@ -12,14 +12,8 @@ namespace eosio::chain { enum class ignore_duplicate_t { no, yes }; // Used for logging of comparison values used for best fork determination - template - struct fork_comparison { - explicit fork_comparison(const BS& bs); - const bool valid; - const uint32_t last_final_block_num{}; - const uint32_t lastest_qc_claim_block_num{}; - const uint32_t block_height{}; - }; + std::string log_fork_comparison(const block_state& bs); + std::string log_fork_comparison(const block_state_legacy& bs); /** * @class fork_database_t @@ -251,5 +245,3 @@ namespace eosio::chain { static constexpr uint32_t max_supported_version = 1; }; } /// eosio::chain - -FC_REFLECT_TEMPLATE((typename BS), eosio::chain::fork_comparison, (valid)(last_final_block_num)(lastest_qc_claim_block_num)(block_height)) From bc827ec4dac884418a6f7ccedfb7be0873899c23 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Fri, 1 Mar 2024 10:57:07 -0600 Subject: [PATCH 3/6] GH-2125 Rename to make more descriptive --- libraries/chain/controller.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index f968c838cb..1839e16913 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -1017,7 +1017,7 @@ struct controller_impl { }); } - signed_block_ptr fork_db_fetch_block_by_num(uint32_t block_num) const { + signed_block_ptr fetch_block_on_head_branch_by_num(uint32_t block_num) const { return fork_db.apply([&](const auto& forkdb) { auto bsp = forkdb.search_on_head_branch(block_num); if (bsp) return bsp->block; @@ -1025,7 +1025,7 @@ struct controller_impl { }); } - std::optional fork_db_fetch_block_id_by_num(uint32_t block_num) const { + std::optional fetch_block_id_on_head_branch_by_num(uint32_t block_num) const { return fork_db.apply>([&](const auto& forkdb) -> std::optional { auto bsp = forkdb.search_on_head_branch(block_num); if (bsp) return bsp->id(); @@ -1034,7 +1034,7 @@ struct controller_impl { } // search on the branch of head - block_state_ptr fork_db_fetch_bsp_by_num(uint32_t block_num) const { + block_state_ptr fetch_bsp_on_head_branch_by_num(uint32_t block_num) const { return fork_db.apply( overloaded{ [](const fork_database_legacy_t&) -> block_state_ptr { return nullptr; }, @@ -1047,7 +1047,7 @@ struct controller_impl { } // search on the branch of given id - block_state_ptr fork_db_fetch_bsp_by_num(const block_id_type& id, uint32_t block_num) const { + block_state_ptr fetch_bsp_on_branch_by_num(const block_id_type& id, uint32_t block_num) const { return fork_db.apply( overloaded{ [](const fork_database_legacy_t&) -> block_state_ptr { return nullptr; }, @@ -3171,7 +3171,7 @@ struct controller_impl { const auto& qc_ext = std::get(block_exts.lower_bound(qc_ext_id)->second); const auto& received_qc = qc_ext.qc.qc; - const auto bsp = fork_db_fetch_bsp_by_num( bsp_in->previous(), qc_ext.qc.block_num ); + const auto bsp = fetch_bsp_on_branch_by_num( bsp_in->previous(), qc_ext.qc.block_num ); if( !bsp ) { return; } @@ -3301,7 +3301,7 @@ struct controller_impl { ("s1", qc_proof.qc.is_strong())("s2", new_qc_claim.is_strong_qc)("b", block_num) ); // find the claimed block's block state on branch of id - auto bsp = fork_db_fetch_bsp_by_num( prev.id(), new_qc_claim.block_num ); + auto bsp = fetch_bsp_on_branch_by_num( prev.id(), new_qc_claim.block_num ); EOS_ASSERT( bsp, invalid_qc_claim, "Block state was not found in forkdb for block_num ${q}. Block number: ${b}", @@ -4510,7 +4510,7 @@ std::optional controller::fetch_block_header_by_id( const b } signed_block_ptr controller::fetch_block_by_number( uint32_t block_num )const { try { - auto b = my->fork_db_fetch_block_by_num( block_num ); + auto b = my->fetch_block_on_head_branch_by_num( block_num ); if (b) return b; @@ -4518,7 +4518,7 @@ signed_block_ptr controller::fetch_block_by_number( uint32_t block_num )const { } FC_CAPTURE_AND_RETHROW( (block_num) ) } std::optional controller::fetch_block_header_by_number( uint32_t block_num )const { try { - auto b = my->fork_db_fetch_block_by_num(block_num); + auto b = my->fetch_block_on_head_branch_by_num(block_num); if (b) return *b; @@ -4532,7 +4532,7 @@ block_id_type controller::get_block_id_for_num( uint32_t block_num )const { try bool find_in_blog = (blog_head && block_num <= blog_head->block_num()); if( !find_in_blog ) { - std::optional id = my->fork_db_fetch_block_id_by_num(block_num); + std::optional id = my->fetch_block_id_on_head_branch_by_num(block_num); if (id) return *id; } From ee5291fcc4aee8e20c922278fa5bda7e0b101ddc Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Fri, 1 Mar 2024 10:57:28 -0600 Subject: [PATCH 4/6] GH-2125 Search on correct branch --- libraries/chain/controller.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index 1839e16913..ca09fbfc39 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -2807,7 +2807,8 @@ struct controller_impl { assert(bsp->header_exts.count(if_ext_id) > 0); // in all instant_finality block headers const auto& if_ext = std::get(bsp->header_exts.lower_bound(if_ext_id)->second); if (if_ext.qc_claim.is_strong_qc) { - auto claimed = forkdb.search_on_head_branch(if_ext.qc_claim.block_num); + // claim has already been verified + auto claimed = forkdb.search_branch(bsp->id(), if_ext.qc_claim.block_num); if (claimed) { set_if_irreversible_block_num(claimed->core.final_on_strong_qc_block_num); } From e3059bf3f389a28727006f98778b865dd444a7b2 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Fri, 1 Mar 2024 11:02:51 -0600 Subject: [PATCH 5/6] GH-2125 Fix compile error and some more type name changes --- libraries/chain/controller.cpp | 2 +- libraries/chain/fork_database.cpp | 4 ++-- libraries/chain/include/eosio/chain/fork_database.hpp | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index ca09fbfc39..dac27d7136 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -2808,7 +2808,7 @@ struct controller_impl { const auto& if_ext = std::get(bsp->header_exts.lower_bound(if_ext_id)->second); if (if_ext.qc_claim.is_strong_qc) { // claim has already been verified - auto claimed = forkdb.search_branch(bsp->id(), if_ext.qc_claim.block_num); + auto claimed = forkdb.search_on_branch(bsp->id(), if_ext.qc_claim.block_num); if (claimed) { set_if_irreversible_block_num(claimed->core.final_on_strong_qc_block_num); } diff --git a/libraries/chain/fork_database.cpp b/libraries/chain/fork_database.cpp index 50d945c2d8..40fdf62dcb 100644 --- a/libraries/chain/fork_database.cpp +++ b/libraries/chain/fork_database.cpp @@ -657,13 +657,13 @@ namespace eosio::chain { } template - void fork_database_t::mark_valid( const BSP& h ) { + void fork_database_t::mark_valid( const bsp_t& h ) { std::lock_guard g( my->mtx ); my->mark_valid_impl( h ); } template - void fork_database_impl::mark_valid_impl( const BSP& h ) { + void fork_database_impl::mark_valid_impl( const bsp_t& h ) { if( bs_accessor_t::is_valid(*h) ) return; auto& by_id_idx = index.template get(); diff --git a/libraries/chain/include/eosio/chain/fork_database.hpp b/libraries/chain/include/eosio/chain/fork_database.hpp index e0983f9153..dfe75e3e33 100644 --- a/libraries/chain/include/eosio/chain/fork_database.hpp +++ b/libraries/chain/include/eosio/chain/fork_database.hpp @@ -4,7 +4,7 @@ namespace eosio::chain { - template + template struct fork_database_impl; using block_branch_t = std::vector; From 2a2a5da58099fb5701d1b746b4f257fec7b21b9e Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Fri, 1 Mar 2024 12:34:21 -0600 Subject: [PATCH 6/6] GH-2125 Simplify search_on_head_branch_impl and add new test cases --- libraries/chain/fork_database.cpp | 7 +------ unittests/fork_db_tests.cpp | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/libraries/chain/fork_database.cpp b/libraries/chain/fork_database.cpp index 40fdf62dcb..9fce0f19fb 100644 --- a/libraries/chain/fork_database.cpp +++ b/libraries/chain/fork_database.cpp @@ -548,12 +548,7 @@ namespace eosio::chain { template BSP fork_database_impl::search_on_head_branch_impl( uint32_t block_num ) const { - for (auto i = index.find(head->id()); i != index.end(); i = index.find((*i)->previous())) { - if ((*i)->block_num() == block_num) - return *i; - } - - return {}; + return search_on_branch_impl(head->id(), block_num); } /** diff --git a/unittests/fork_db_tests.cpp b/unittests/fork_db_tests.cpp index e2d7432030..655b3e9544 100644 --- a/unittests/fork_db_tests.cpp +++ b/unittests/fork_db_tests.cpp @@ -102,6 +102,21 @@ BOOST_AUTO_TEST_CASE(add_remove_test) try { forkdb.add(bsp13b, mark_valid_t::no, ignore_duplicate_t::no); // will throw if already exists forkdb.add(bsp14b, mark_valid_t::no, ignore_duplicate_t::no); // will throw if already exists + // test search + BOOST_TEST(forkdb.search_on_branch( bsp13bb->id(), 11) == bsp11b); + BOOST_TEST(forkdb.search_on_branch( bsp13bb->id(), 9) == block_state_ptr{}); + + // test fetch branch + auto branch = forkdb.fetch_branch( bsp13b->id(), 12); + BOOST_REQUIRE(branch.size() == 2); + BOOST_TEST(branch[0] == bsp12b); + BOOST_TEST(branch[1] == bsp11b); + branch = forkdb.fetch_branch( bsp13bbb->id(), 13); + BOOST_REQUIRE(branch.size() == 3); + BOOST_TEST(branch[0] == bsp13bbb); + BOOST_TEST(branch[1] == bsp12bb); + BOOST_TEST(branch[2] == bsp11b); + } FC_LOG_AND_RETHROW(); BOOST_AUTO_TEST_SUITE_END()