From 968a4ab6662d05083b42fe39f19ca7d8a92220f0 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Wed, 24 Apr 2024 17:27:18 -0500 Subject: [PATCH 1/2] GH-6 Avoid setting new proposer policy if there is no change --- libraries/chain/controller.cpp | 66 +++++++++++++++++------- unittests/producer_schedule_if_tests.cpp | 23 +++++++++ 2 files changed, 69 insertions(+), 20 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index 657cf2eb7a..dd2ea0282a 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -491,16 +491,35 @@ struct building_block { uint32_t get_block_num() const { return block_num; } - uint32_t get_next_proposer_schedule_version() const { - if (!parent.proposer_policies.empty()) { - block_timestamp_type active_time = detail::get_next_next_round_block_time(timestamp); - if (auto itr = parent.proposer_policies.find(active_time); itr != parent.proposer_policies.cend()) { - return itr->second->proposer_schedule.version; // will replace so return same version + // returns the next proposer schedule version and true if different + // if producers is not different then returns the current schedule version (or next schedule version) + // uses current building_block timestamp + std::tuple get_next_proposer_schedule_version(const vector& producers) const { + assert(active_proposer_policy); + + auto get_next_sched = [&]() -> const producer_authority_schedule& { + if (!parent.proposer_policies.empty()) { + block_timestamp_type active_time = detail::get_next_next_round_block_time(timestamp); + if (auto itr = parent.proposer_policies.find(active_time); itr != parent.proposer_policies.cend()) { + // would replace so compare to prev + if (itr != parent.proposer_policies.begin()) { + return (--itr)->second->proposer_schedule; + } + return active_proposer_policy->proposer_schedule; + } + return (--parent.proposer_policies.end())->second->proposer_schedule; } - return (--parent.proposer_policies.end())->second->proposer_schedule.version + 1; + + return active_proposer_policy->proposer_schedule; + }; + + const producer_authority_schedule& lhs = get_next_sched(); + + if (std::ranges::equal(lhs.producers, producers)) { + return {lhs.version, false}; } - assert(active_proposer_policy); - return active_proposer_policy->proposer_schedule.version + 1; + + return {lhs.version + 1, true}; } }; @@ -591,11 +610,13 @@ struct building_block { v); } - int64_t get_next_proposer_schedule_version() const { + std::tuple get_next_proposer_schedule_version(const vector& producers) const { return std::visit( - overloaded{[](const building_block_legacy&) -> int64_t { return -1; }, - [&](const building_block_if& bb) -> int64_t { return bb.get_next_proposer_schedule_version(); } - }, + overloaded{[](const building_block_legacy&) -> std::tuple { return {-1, false}; }, + [&](const building_block_if& bb) -> std::tuple { + return bb.get_next_proposer_schedule_version(producers); + } + }, v); } @@ -887,11 +908,13 @@ struct pending_state { _block_stage); } - int64_t get_next_proposer_schedule_version() const { + std::tuple get_next_proposer_schedule_version(const vector& producers) const { return std::visit(overloaded{ - [](const building_block& stage) -> int64_t { return stage.get_next_proposer_schedule_version(); }, - [](const assembled_block&) -> int64_t { assert(false); return -1; }, - [](const completed_block&) -> int64_t { assert(false); return -1; } + [&](const building_block& stage) -> std::tuple { + return stage.get_next_proposer_schedule_version(producers); + }, + [](const assembled_block&) -> std::tuple { assert(false); return {-1, false}; }, + [](const completed_block&) -> std::tuple { assert(false); return {-1, false}; } }, _block_stage); } @@ -5189,17 +5212,20 @@ int64_t controller_impl::set_proposed_producers( vector prod return -1; // INSTANT_FINALITY depends on DISALLOW_EMPTY_PRODUCER_SCHEDULE assert(pending); - const auto& gpo = db.get(); - auto cur_block_num = chain_head.block_num() + 1; - producer_authority_schedule sch; + auto [version, diff] = pending->get_next_proposer_schedule_version(producers); + if (!diff) + return version; - sch.version = pending->get_next_proposer_schedule_version(); + producer_authority_schedule sch; + sch.version = version; sch.producers = std::move(producers); ilog( "proposed producer schedule with version ${v}", ("v", sch.version) ); // overwrite any existing proposed_schedule set earlier in this block + auto cur_block_num = chain_head.block_num() + 1; + auto& gpo = db.get(); db.modify( gpo, [&]( auto& gp ) { gp.proposed_schedule_block_num = cur_block_num; gp.proposed_schedule = sch; diff --git a/unittests/producer_schedule_if_tests.cpp b/unittests/producer_schedule_if_tests.cpp index 60833bb359..ff1afe3b6a 100644 --- a/unittests/producer_schedule_if_tests.cpp +++ b/unittests/producer_schedule_if_tests.cpp @@ -170,6 +170,29 @@ BOOST_FIXTURE_TEST_CASE( proposer_policy_progression_test, validating_tester ) t // sch3 becomes active BOOST_CHECK_EQUAL( 2u, control->active_producers().version ); // should be 2 as sch2 was replaced by sch3 BOOST_CHECK_EQUAL( true, compare_schedules( sch3, control->active_producers() ) ); + + // get to next producer round + auto prod = produce_block()->producer; + for (auto b = produce_block(); b->producer == prod; b = produce_block()); + + // test no change to active schedule + set_producers( {"bob"_n,"alice"_n} ); // same as before, so no change + produce_blocks(config::producer_repetitions); + produce_blocks(config::producer_repetitions); + BOOST_CHECK_EQUAL( 2u, control->active_producers().version ); // should be 2 as not different so no change + BOOST_CHECK_EQUAL( true, compare_schedules( sch3, control->active_producers() ) ); + + // test no change to proposed schedule, only the first one will take affect + for (size_t i = 0; i < config::producer_repetitions*2-1; ++i) { + BOOST_CHECK_EQUAL( 2u, control->active_producers().version ); // should be 2 as not taken affect yet + BOOST_CHECK_EQUAL( true, compare_schedules( sch3, control->active_producers() ) ); + set_producers( {"bob"_n,"carol"_n} ); + produce_block(); + } + produce_block(); + BOOST_CHECK_EQUAL( 3u, control->active_producers().version ); // should be 3 now as bob,carol now active + BOOST_CHECK_EQUAL( true, compare_schedules( sch2, control->active_producers() ) ); + } FC_LOG_AND_RETHROW() From 9df701c75ae0bebef2015a810acce104b6800e09 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Thu, 25 Apr 2024 09:54:18 -0500 Subject: [PATCH 2/2] GH-6 Add additional comments, cleanup up logic --- libraries/chain/controller.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index 0c8c269765..750015fee5 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -492,24 +492,27 @@ struct building_block { uint32_t get_block_num() const { return block_num; } // returns the next proposer schedule version and true if different - // if producers is not different then returns the current schedule version (or next schedule version) - // uses current building_block timestamp + // if producers is not different then returns the current schedule version (or next schedule version) std::tuple get_next_proposer_schedule_version(const vector& producers) const { assert(active_proposer_policy); auto get_next_sched = [&]() -> const producer_authority_schedule& { + // if there are any policies already proposed but not active yet then they are what needs to be compared if (!parent.proposer_policies.empty()) { block_timestamp_type active_time = detail::get_next_next_round_block_time(timestamp); if (auto itr = parent.proposer_policies.find(active_time); itr != parent.proposer_policies.cend()) { - // would replace so compare to prev + // Same active time, a new proposer schedule will replace this entry, `next` therefore is the previous if (itr != parent.proposer_policies.begin()) { return (--itr)->second->proposer_schedule; } + // no previous to what will be replaced, use active return active_proposer_policy->proposer_schedule; } - return (--parent.proposer_policies.end())->second->proposer_schedule; + // will not replace any proposed policies, use next to become active + return parent.proposer_policies.begin()->second->proposer_schedule; } + // none currently in-flight, use active return active_proposer_policy->proposer_schedule; };