Merge pull request #68 from AntelopeIO/GH-6-proposer-policy

IF: Do not set new proposer policy unless different

Author: heifner

libraries/chain/controller.cpp

@@ -491,16 +491,38 @@ 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)
+      std::tuple<uint32_t, bool> get_next_proposer_schedule_version(const vector<producer_authority>& 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()) {
+                  // 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;
+               }
+               // will not replace any proposed policies, use next to become active
+               return parent.proposer_policies.begin()->second->proposer_schedule;
             }
-            return (--parent.proposer_policies.end())->second->proposer_schedule.version + 1;
+
+            // none currently in-flight, use active
+            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 +613,13 @@ struct building_block {
                         v);
    }
 
-   int64_t get_next_proposer_schedule_version() const {
+   std::tuple<uint32_t, bool> get_next_proposer_schedule_version(const vector<producer_authority>& 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<uint32_t, bool> { return {-1, false}; },
+                    [&](const building_block_if& bb) -> std::tuple<uint32_t, bool> {
+                       return bb.get_next_proposer_schedule_version(producers);
+                    }
+         },
          v);
    }
 
@@ -887,11 +911,13 @@ struct pending_state {
          _block_stage);
    }
 
-   int64_t get_next_proposer_schedule_version() const {
+   std::tuple<uint32_t, bool> get_next_proposer_schedule_version(const vector<producer_authority>& 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<uint32_t, bool> {
+                              return stage.get_next_proposer_schedule_version(producers);
+                           },
+                           [](const assembled_block&) -> std::tuple<uint32_t, bool> { assert(false); return {-1, false}; },
+                           [](const completed_block&) -> std::tuple<uint32_t, bool> { assert(false); return {-1, false}; }
                         },
                         _block_stage);
    }
@@ -5189,17 +5215,20 @@ int64_t controller_impl::set_proposed_producers( vector<producer_authority> prod
       return -1; // INSTANT_FINALITY depends on DISALLOW_EMPTY_PRODUCER_SCHEDULE
 
    assert(pending);
-   const auto& gpo = db.get<global_property_object>();
-   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<global_property_object>();
    db.modify( gpo, [&]( auto& gp ) {
       gp.proposed_schedule_block_num = cur_block_num;
       gp.proposed_schedule = sch;

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()