IF: Update new proposer policy algorithm

libraries/chain/block_header_state.cpp

@@ -106,18 +106,23 @@ void finish_next(const block_header_state& prev,
    if (!prev.proposer_policies.empty()) {
       auto it = prev.proposer_policies.begin();
       // +1 since this is called after the block is built, this will be the active schedule for the next block
-      if (it->first.slot <= next_header_state.header.timestamp.slot + 1) {
+      while (it != prev.proposer_policies.end() && it->first.slot <= next_header_state.header.timestamp.slot + 1) {
          next_header_state.active_proposer_policy = it->second;
-         next_header_state.proposer_policies = { ++it, prev.proposer_policies.end() };
-      } else {
+         ++it;
+      }
+      if (it == prev.proposer_policies.begin()) { // none made active
          next_header_state.proposer_policies = prev.proposer_policies;
+      } else if (it != prev.proposer_policies.end()) { // some made active
+         next_header_state.proposer_policies = { it, prev.proposer_policies.end() };
+      } else { // all made active
+         // next_header_state.proposer_policies will be emtpy
       }
    }
 
    if (if_ext.new_proposer_policy) {
       // called when assembling the block
-      next_header_state.proposer_policies[if_ext.new_proposer_policy->active_time] =
-         std::move(if_ext.new_proposer_policy);
+      next_header_state.proposer_policies.emplace_back(if_ext.new_proposer_policy->active_time,
+                                                       std::move(if_ext.new_proposer_policy));
    }
 
    // finality_core

libraries/chain/controller.cpp

@@ -491,38 +491,30 @@ struct building_block {
 
       uint32_t get_block_num() const { return block_num; }
 
-      // returns the next proposer schedule version and true if different
+      // returns the next proposer schedule version and true if producers should be proposed in block
       // 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;
             }
-
             // 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};
+         decltype(lhs.version) v = lhs.version;
+
+         if (!std::ranges::equal(lhs.producers, producers)) {
+            ++v;
+            return {v, true};
          }
 
-         return {lhs.version + 1, true};
+         return {v, false};
       }
 
    };
@@ -5216,25 +5208,39 @@ int64_t controller_impl::set_proposed_producers( vector<producer_authority> prod
 
    assert(pending);
 
-   auto [version, diff] = pending->get_next_proposer_schedule_version(producers);
-   if (!diff)
-      return version;
+   // see if one already proposed in this block
+   auto& gpo = db.get<global_property_object>();
+   if (gpo.proposed_schedule_block_num) {
+      if (std::equal(producers.begin(), producers.end(),
+                     gpo.proposed_schedule.producers.begin(), gpo.proposed_schedule.producers.end())) {
+         return std::numeric_limits<uint32_t>::max(); // the proposed producer schedule does not change
+      }
+      // clear gpo proposed_schedule as we may determine no diff between proposed producers and next proposer schedule
+      db.modify( gpo, [&]( auto& gp ) {
+         gp.proposed_schedule_block_num.reset();
+         gp.proposed_schedule.version = 0;
+         gp.proposed_schedule.producers.clear();
+      });
+   }
+
+   auto [version, should_propose] = pending->get_next_proposer_schedule_version(producers);
+   if (!should_propose)
+      return std::numeric_limits<uint32_t>::max();
 
    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
+   // store schedule in gpo so it will be rolledback if transaction fails
    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;
    });
 
-   return sch.version;
+   return std::numeric_limits<uint32_t>::max();
 }
 
 int64_t controller_impl::set_proposed_producers_legacy( vector<producer_authority> producers ) {

libraries/chain/include/eosio/chain/block_header_state.hpp

@@ -68,8 +68,18 @@ struct block_header_state {
    finalizer_policy_ptr                active_finalizer_policy; // finalizer set + threshold + generation, supports `digest()`
    proposer_policy_ptr                 active_proposer_policy;  // producer authority schedule, supports `digest()`
 
-   // block time when proposer_policy will become active
-   flat_map<block_timestamp_type, proposer_policy_ptr>     proposer_policies;
+   // <block time when proposer_policy will become active, proposer policy>
+   // The active time is the next,next producer round. For example,
+   //   round A [1,2,..12], next_round B [1,2,..12], next_next_round C [1,2,..12], D [1,2,..12]
+   //   If proposed in A1, A2, .. A12 becomes active in C1
+   //   If proposed in B1, B2, .. B12 becomes active in D1
+   // This is a `deque` because the same active block time can contain up to 12 entries (one per block)
+   // for the entry proposed in a block:
+   //   For example, proposer policy proposed: P1 in A1, P2 in A7, P3 in A12, P4 in B2, P5 in B12 then the map contains:
+   //       [C1 -> P1],[C1 -> P2],[C1 -> P3],[D1 -> P4],[D1 -> P5]
+   //   At C1 P1,P2,P3 are applied causing an active policy of P3
+   //   At D1 P4,P5 are appliced causing an acive policy of P5
+   std::deque<std::pair<block_timestamp_type, proposer_policy_ptr>> proposer_policies;
 
    // track in-flight finalizer policies. This is a `multimap` because the same block number
    // can hold a `proposed` and a `pending` finalizer_policy. When that block becomes final, the

libraries/chain/include/eosio/chain/snapshot_detail.hpp

@@ -116,7 +116,7 @@ namespace eosio::chain::snapshot_detail {
       finality_core                                       core;
       finalizer_policy_ptr                                active_finalizer_policy;
       proposer_policy_ptr                                 active_proposer_policy;
-      flat_map<block_timestamp_type, proposer_policy_ptr> proposer_policies;
+      std::deque<std::pair<block_timestamp_type, proposer_policy_ptr>> proposer_policies;
       flat_multimap<block_num_type, finalizer_policy_tracker> finalizer_policies;
       uint32_t                                            finalizer_policy_generation;
 

libraries/chain/include/eosio/chain/webassembly/interface.hpp

@@ -153,7 +153,8 @@ namespace webassembly {
           *
           * @param packed_producer_schedule - vector of producer keys
           *
-          * @return -1 if proposing a new producer schedule was unsuccessful, otherwise returns the version of the new proposed schedule.
+          * @return pre-savanna:  -1 if proposing a new producer schedule was unsuccessful, otherwise returns the version of the new proposed schedule.
+          *         post-savanna: -1 if proposing a new producer schedule was unsuccessful, otherwise returns max uint32_t
          */
          int64_t set_proposed_producers(legacy_span<const char> packed_producer_schedule);
 
@@ -169,7 +170,8 @@ namespace webassembly {
           * @param packed_producer_format - format of the producer data blob.
           * @param packed_producer_schedule - packed data of representing the producer schedule in the format indicated.
           *
-          * @return -1 if proposing a new producer schedule was unsuccessful, otherwise returns the version of the new proposed schedule.
+          * @return pre-savanna:  -1 if proposing a new producer schedule was unsuccessful, otherwise returns the version of the new proposed schedule.
+          *         post-savanna: -1 if proposing a new producer schedule was unsuccessful, otherwise returns max uint32_t
          */
          int64_t set_proposed_producers_ex(uint64_t packed_producer_format, legacy_span<const char> packed_producer_schedule);
 

unittests/producer_schedule_if_tests.cpp

@@ -115,6 +115,19 @@ bool compare_schedules( const vector<producer_authority>& a, const producer_auth
 BOOST_FIXTURE_TEST_CASE( proposer_policy_progression_test, validating_tester ) try {
    create_accounts( {"alice"_n,"bob"_n,"carol"_n} );
 
+   // set_producers in same block, do it the explicit way to use a diff expiration and avoid duplicate trx
+   auto set_producers_force = [&](const std::vector<account_name>& producers) {
+      static int unique = 0; // used to force uniqueness of transaction
+      auto schedule = get_producer_authorities( producers );
+      fc::variants schedule_variant;
+      schedule_variant.reserve(schedule.size());
+      for( const auto& e: schedule ) {
+         schedule_variant.emplace_back(e.get_abi_variant());
+      }
+      push_action( config::system_account_name, "setprods"_n, config::system_account_name,
+                  fc::mutable_variant_object()("schedule", schedule_variant), DEFAULT_EXPIRATION_DELTA + (++unique));
+   };
+
    while (control->head_block_num() < 3) {
       produce_block();
    }
@@ -132,7 +145,7 @@ BOOST_FIXTURE_TEST_CASE( proposer_policy_progression_test, validating_tester ) t
 
    // set a new proposer policy sch1
    set_producers( {"alice"_n} );
-   vector<producer_authority> sch1 = {
+   vector<producer_authority> alice_sch = {
                                  producer_authority{"alice"_n, block_signing_authority_v0{1, {{get_public_key("alice"_n, "active"), 1}}}}
                                };
 
@@ -145,15 +158,15 @@ BOOST_FIXTURE_TEST_CASE( proposer_policy_progression_test, validating_tester ) t
 
    // set another ploicy to have multiple pending different active time policies
    set_producers( {"bob"_n,"carol"_n} );
-   vector<producer_authority> sch2 = {
+   vector<producer_authority> bob_carol_sch = {
                                  producer_authority{"bob"_n,   block_signing_authority_v0{ 1, {{get_public_key("bob"_n,   "active"),1}}}},
                                  producer_authority{"carol"_n, block_signing_authority_v0{ 1, {{get_public_key("carol"_n, "active"),1}}}}
                                };
    produce_block();
 
    // set another ploicy should replace sch2
    set_producers( {"bob"_n,"alice"_n} );
-   vector<producer_authority> sch3 = {
+   vector<producer_authority> bob_alice_sch = {
       producer_authority{"bob"_n,   block_signing_authority_v0{ 1, {{get_public_key("bob"_n,   "active"),1}}}},
       producer_authority{"alice"_n, block_signing_authority_v0{ 1, {{get_public_key("alice"_n, "active"),1}}}}
    };
@@ -163,13 +176,13 @@ BOOST_FIXTURE_TEST_CASE( proposer_policy_progression_test, validating_tester ) t
 
    // sch1  must become active no later than 2 rounds but sch2 cannot become active yet
    BOOST_CHECK_EQUAL( control->active_producers().version, 1u );
-   BOOST_CHECK_EQUAL( true, compare_schedules( sch1, control->active_producers() ) );
+   BOOST_CHECK_EQUAL( true, compare_schedules( alice_sch, control->active_producers() ) );
 
    produce_blocks(config::producer_repetitions);
 
-   // 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() ) );
+   // sch3 becomes active, version should be 3 even though sch2 was replaced by sch3
+   BOOST_CHECK_EQUAL( 3u, control->active_producers().version );
+   BOOST_CHECK_EQUAL( true, compare_schedules( bob_alice_sch, control->active_producers() ) );
 
    // get to next producer round
    auto prod = produce_block()->producer;
@@ -179,19 +192,125 @@ BOOST_FIXTURE_TEST_CASE( proposer_policy_progression_test, validating_tester ) t
    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() ) );
+   BOOST_CHECK_EQUAL( 3u, control->active_producers().version ); // should be 3 as not different so no change
+   BOOST_CHECK_EQUAL( true, compare_schedules( bob_alice_sch, 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() ) );
+      BOOST_CHECK_EQUAL( 3u, control->active_producers().version ); // should be 3 as not taken affect yet
+      BOOST_CHECK_EQUAL( true, compare_schedules( bob_alice_sch, control->active_producers() ) );
       set_producers( {"bob"_n,"carol"_n} );
+      set_producers_force({"bob"_n,"carol"_n} );
+      set_producers_force({"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() ) );
+   BOOST_CHECK_EQUAL( 4u, control->active_producers().version ); // should be 4 now as bob,carol now active
+   BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );
+
+   // get to next producer round
+   prod = produce_block()->producer;
+   for (auto b = produce_block(); b->producer == prod; b = produce_block());
+
+   // test change in same block where there is an existing proposed that is the same
+   set_producers( {"bob"_n,"alice"_n} );
+   produce_block();
+   set_producers( {"bob"_n,"carol"_n} );
+   set_producers_force({"bob"_n,"carol"_n} );
+   produce_blocks(config::producer_repetitions-1);
+   produce_blocks(config::producer_repetitions);
+   BOOST_CHECK_EQUAL( 6u, control->active_producers().version ); // should be 6 now as bob,carol now active
+   BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );
+
+   // test change in same block where there is no change
+   set_producers( {"bob"_n,"alice"_n} );
+   set_producers_force({"bob"_n,"carol"_n} ); // put back, no change expected
+   produce_blocks(config::producer_repetitions);
+   produce_blocks(config::producer_repetitions);
+   BOOST_CHECK_EQUAL( 6u, control->active_producers().version ); // should be 6 now as bob,carol is still active
+   BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );
+
+   // get to next producer round
+   prod = produce_block()->producer;
+   for (auto b = produce_block(); b->producer == prod; b = produce_block());
+
+   // test two in-flight
+   //    round A [1,2,..12], next_round B [1,2,..12], next_next_round C [1,2,..12], D [1,2,..12]
+   //      propose P1 in A2, active in C1
+   //      propose P2 in B2, active in D1
+   //      propose P3 in B3, active in D1, replaces P2
+   produce_block();
+   set_producers({"alice"_n}); // A2, P1
+   produce_blocks(config::producer_repetitions-1); // A12
+   produce_block();
+   set_producers({"bob"_n,"carol"_n}); // B2
+   produce_block();
+   set_producers({"bob"_n, "alice"_n} ); // P3
+   produce_blocks(config::producer_repetitions-2); // B12
+   produce_block(); // C1
+   BOOST_CHECK_EQUAL( 7u, control->active_producers().version );
+   BOOST_CHECK_EQUAL( true, compare_schedules( alice_sch, control->active_producers() ) );
+   produce_blocks(config::producer_repetitions); // D1
+   BOOST_CHECK_EQUAL( 9u, control->active_producers().version );
+   BOOST_CHECK_EQUAL( true, compare_schedules( bob_alice_sch, control->active_producers() ) );
+
+   // get to next producer round
+   prod = produce_block()->producer;
+   for (auto b = produce_block(); b->producer == prod; b = produce_block());
+
+   // test two in-flight, P1 == P3, so no change
+   //    round A [1,2,..12], next_round B [1,2,..12], next_next_round C [1,2,..12], D [1,2,..12]
+   //      propose P1 in A2, active in C1
+   //      propose P2 in B2, active in D1
+   //      propose P3 in B3, active in D1, replaces P2
+   produce_block();
+   set_producers({"bob"_n,"carol"_n}); // A2, P1
+   produce_blocks(config::producer_repetitions-1); // A12
+   produce_block();
+   set_producers({"alice"_n}); // B2
+   produce_block();
+   set_producers({"bob"_n,"carol"_n}); // P3 == P1
+   produce_blocks(config::producer_repetitions-2); // B12
+   produce_block(); // C1
+   BOOST_CHECK_EQUAL( 10u, control->active_producers().version );
+   BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );
+   produce_blocks(config::producer_repetitions); // D1
+   BOOST_CHECK_EQUAL( 12u, control->active_producers().version );
+   BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );
+
+   // get to next producer round
+   prod = produce_block()->producer;
+   for (auto b = produce_block(); b->producer == prod; b = produce_block());
+
+   // test two in-flight, ultimately no change
+   produce_block(); // 1
+   set_producers({"bob"_n,"carol"_n});
+   produce_block(); // 2
+   set_producers({"alice"_n});
+   produce_block(); // 3
+   set_producers({"carol"_n,"alice"_n});
+   produce_block(); // 4
+   set_producers({"carol"_n});
+   produce_block(); // 5
+   set_producers({"alice"_n});
+   produce_block(); // 6
+   set_producers({"bob"_n,"carol"_n});
+   produce_blocks(config::producer_repetitions-6);
+   set_producers({"bob"_n});
+   produce_block(); // 2
+   set_producers({"bob"_n,"carol"_n});
+   produce_block(); // 3
+   set_producers({"carol"_n,"bob"_n});
+   produce_block(); // 4
+   set_producers({"alice"_n} );
+   produce_block(); // 5
+   set_producers({"bob"_n,"carol"_n});
+   produce_blocks(config::producer_repetitions-5); // B12
+   BOOST_CHECK_EQUAL( 17u, control->active_producers().version );
+   BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );
+   produce_blocks(config::producer_repetitions);
+   BOOST_CHECK_EQUAL( 22u, control->active_producers().version );
+   BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );
 
 } FC_LOG_AND_RETHROW()