Merge pull request #1966 from EOSIO/1671-QA

#1671 QA - Producer schedule version in the block header is not updated properly

contracts/eosio.bios/eosio.bios.abi

@@ -27,7 +27,7 @@
       "base": "",
       "fields": [
         {"name":"producer_name",      "type":"account_name"},
-        {"name":"signing_key",  "type":"public_key"}
+        {"name":"block_signing_key",  "type":"public_key"}
       ]
     },{
       "name": "set_producers",

libraries/chain/include/eosio/chain/producer_schedule.hpp

@@ -42,6 +42,7 @@ namespace eosio { namespace chain {
 
       operator producer_schedule_type()const {
          producer_schedule_type result;
+         result.version = version;
          result.producers.reserve(producers.size());
          for( const auto& p : producers )
             result.producers.push_back(p);

libraries/testing/include/eosio/testing/tester.hpp

@@ -181,8 +181,8 @@ namespace eosio { namespace testing {
       tester(chain_controller::runtime_limits limits = chain_controller::runtime_limits());
       tester(chain_controller::controller_config config);
 
-      void              push_genesis_block();
-      void              set_producers(const vector<account_name>& producer_names);
+      void                    push_genesis_block();
+      producer_schedule_type  set_producers(const vector<account_name>& producer_names, const uint32_t version = 0);
    };
 
    /**

libraries/testing/tester.cpp

@@ -554,24 +554,19 @@ namespace eosio { namespace testing {
       set_abi(config::system_account_name, eosio_bios_abi);
    }
 
-   void tester::set_producers(const vector<account_name>& producer_names) {
-      // Create producer accounts, if it does not exist yet
+   producer_schedule_type tester::set_producers(const vector<account_name>& producer_names, const uint32_t version) {
+      // Create producer schedule
+      producer_schedule_type schedule;
+      schedule.version = version;
       for (auto& producer_name: producer_names) {
-        create_account(producer_name);
+         producer_key pk = { producer_name, get_public_key( producer_name, "active" )};
+         schedule.producers.emplace_back(pk);
       }
 
-      // Construct the param for setprods action
-      vector<fc::mutable_variant_object> producer_keys;
-      for (auto& producer_name: producer_names) {
-         producer_keys.emplace_back( fc::mutable_variant_object()
-                                             ("producer_name", producer_name)
-                                             ("signing_key", get_public_key( producer_name, "active" )));
-      }
-      // Send setprods action
-      static uint32_t version = 1;
-      push_action(N(eosio), N(setprods), N(eosio), fc::mutable_variant_object()
-                                                        ("version", version++)
-                                                        ("producers", producer_keys));
+      push_action(N(eosio), N(setprods), N(eosio),
+                  fc::mutable_variant_object()("version", schedule.version)("producers", schedule.producers));
+
+      return schedule;
    }
 
 } }  /// eosio::test

tests/chain_tests/producer_schedule_tests.cpp

 #include <boost/test/unit_test.hpp>
 #include <eosio/testing/tester.hpp>
-#include <iostream>
-#include <string>
-#include <tuple>
+#include <boost/range/algorithm.hpp>
+
 using namespace eosio::testing;
 using namespace eosio::chain;
 using mvo = fc::mutable_variant_object;
 
 BOOST_AUTO_TEST_SUITE(producer_schedule_tests)
 
-   BOOST_FIXTURE_TEST_CASE( verify_producer_schedule, tester ) try {
-
-      // --- Define some utility functions -----
-
-      using block_num_and_schd_tuple = std::tuple<block_num_type, producer_schedule_type>;
-
-      // Utility function to send setprods action
-      uint32_t version = 1;
-      const auto& set_producers = [&](const vector<account_name>& producers) -> block_num_and_schd_tuple {
-         // Construct producer schedule object
-         producer_schedule_type schedule;
-         schedule.version = version++;
-         for (auto& producer_name: producers) {
-            producer_key pk = { producer_name, get_public_key( producer_name, "active" )};
-            schedule.producers.emplace_back(pk);
-         }
-
-         // Send setprods action
-         vector<mvo> producer_keys;
-         for (auto& producer: schedule.producers) {
-            producer_keys.emplace_back( mvo()("producer_name", producer.producer_name)("signing_key", producer.block_signing_key));
-         }
-         push_action(N(eosio), N(setprods), N(eosio), mvo()("version", schedule.version)("producers", producer_keys));
-
-         // Calculate effective block num, which is the start of the next round
-         auto eff_block_num = control->get_dynamic_global_properties().head_block_number + 1;
-         const auto blocks_per_round = control->get_global_properties().active_producers.producers.size() * config::producer_repetitions;
-         while((eff_block_num % blocks_per_round) != 0) {
-            eff_block_num++;
-         }
-
-         // Return the effective block num and new schedule
-         return block_num_and_schd_tuple{ eff_block_num, schedule };
-      };
-
-      // Utility function to calculate expected producer
-      const auto& get_expected_producer = [&](const producer_schedule_type& schedule, const uint64_t slot) -> account_name {
-         const auto& index = (slot % (schedule.producers.size() * config::producer_repetitions)) / config::producer_repetitions;
-         return schedule.producers.at(index).producer_name;
-      };
+   // Calculate expected producer given the schedule and slot number
+   account_name get_expected_producer(const producer_schedule_type& schedule, const uint64_t slot) {
+      const auto& index = (slot % (schedule.producers.size() * config::producer_repetitions)) / config::producer_repetitions;
+      return schedule.producers.at(index).producer_name;
+   };
+
+   // Check if two schedule is equal
+   bool is_schedule_equal(const producer_schedule_type& first, const producer_schedule_type& second) {
+      bool is_equal = first.producers.size() == second.producers.size();
+      for (uint32_t i = 0; i < first.producers.size(); i++) {
+         is_equal = is_equal && first.producers.at(i) == second.producers.at(i);
+      }
+      return is_equal;
+   };
+
+   // Calculate the block num of the next round first block
+   // The new producer schedule will become effective when it's in the block of the next round first block
+   // However, it won't be applied until the effective block num is deemed irreversible
+   uint64_t calc_block_num_of_next_round_first_block(const chain_controller& control){
+      auto res = control.get_dynamic_global_properties().head_block_number + 1;
+      const auto blocks_per_round = control.get_global_properties().active_producers.producers.size() * config::producer_repetitions;
+      while((res % blocks_per_round) != 0) {
+         res++;
+      }
+      return res;
+   };
 
-      // Utility function to check if two schedule is equal
-      const auto& is_schedule_equal = [&](const producer_schedule_type& first, const producer_schedule_type& second) -> bool {
-         bool is_equal = first.producers.size() == second.producers.size();
-         for (uint32_t i = 0; i < first.producers.size(); i++) {
-            is_equal = is_equal && first.producers.at(i) == second.producers.at(i);
-         }
-         return is_equal;
-      };
+   BOOST_FIXTURE_TEST_CASE( verify_producer_schedule, tester ) try {
 
       // Utility function to ensure that producer schedule work as expected
-      const auto& confirm_schedule_correctness = [&](const block_num_and_schd_tuple& new_eff_block_num_and_schd_tuple)  {
+      const auto& confirm_schedule_correctness = [&](const producer_schedule_type& new_prod_schd, const uint64_t eff_new_prod_schd_block_num)  {
          const uint32_t check_duration = 1000; // number of blocks
          for (uint32_t i = 0; i < check_duration; ++i) {
             const producer_schedule_type current_schedule = control->get_global_properties().active_producers;
@@ -68,14 +46,14 @@ BOOST_AUTO_TEST_SUITE(producer_schedule_tests)
             // Determine expected producer
             const auto& expected_producer = get_expected_producer(current_schedule, current_absolute_slot + 1);
 
+            // The new schedule will only be applied once the effective block num is deemed irreversible
+            const bool is_new_schedule_applied = control->last_irreversible_block_num() > eff_new_prod_schd_block_num;
+
             // Ensure that we have the correct schedule at the right time
-            // The new schedule will only be used once the effective block num is deemed irreversible
-            const auto& new_schd_eff_block_num = std::get<0>(new_eff_block_num_and_schd_tuple);
-            const auto& new_schedule = std::get<1>(new_eff_block_num_and_schd_tuple);
-            if (control->last_irreversible_block_num() > new_schd_eff_block_num) {
-               BOOST_TEST(is_schedule_equal(new_schedule, current_schedule));
+            if (is_new_schedule_applied) {
+               BOOST_TEST(is_schedule_equal(new_prod_schd, current_schedule));
             } else {
-               BOOST_TEST(!is_schedule_equal(new_schedule, current_schedule));
+               BOOST_TEST(!is_schedule_equal(new_prod_schd, current_schedule));
             }
 
             // Produce block
@@ -86,8 +64,6 @@ BOOST_AUTO_TEST_SUITE(producer_schedule_tests)
          }
       };
 
-      // ---- End of utility function definitions ----
-
       // Create producer accounts
       vector<account_name> producers = {
               "inita", "initb", "initc", "initd", "inite", "initf", "initg",
@@ -97,41 +73,90 @@ BOOST_AUTO_TEST_SUITE(producer_schedule_tests)
       create_accounts(producers);
 
       // ---- Test first set of producers ----
-      vector<account_name> first_set_of_producer = {
-              producers[0], producers[1], producers[2], producers[3], producers[4], producers[5], producers[6],
-              producers[7], producers[8], producers[9], producers[10], producers[11], producers[12], producers[13],
-              producers[14], producers[15], producers[16], producers[17], producers[18], producers[19], producers[20]
-      };
       // Send set prods action and confirm schedule correctness
-      const auto& first_eff_block_num_and_schd_tuple = set_producers(first_set_of_producer);
-      confirm_schedule_correctness(first_eff_block_num_and_schd_tuple);
+      const auto& first_prod_schd = set_producers(producers, 1);
+      const auto& eff_first_prod_schd_block_num = calc_block_num_of_next_round_first_block(*control);
+      confirm_schedule_correctness(first_prod_schd, eff_first_prod_schd_block_num);
 
       // ---- Test second set of producers ----
-      vector<account_name> second_set_producer_order = {
+      vector<account_name> second_set_of_producer = {
               producers[3], producers[6], producers[9], producers[12], producers[15], producers[18], producers[20]
       };
       // Send set prods action and confirm schedule correctness
-      const auto& second_eff_block_num_and_schd_tuple = set_producers(second_set_producer_order);
-      confirm_schedule_correctness(second_eff_block_num_and_schd_tuple);
-
+      const auto& second_prod_schd = set_producers(second_set_of_producer, 2);
+      const auto& eff_second_prod_schd_block_num = calc_block_num_of_next_round_first_block(*control);
+      confirm_schedule_correctness(second_prod_schd, eff_second_prod_schd_block_num);
 
       // ---- Test deliberately miss some blocks ----
       const int64_t num_of_missed_blocks = 5000;
       produce_block(fc::microseconds(500 * 1000 * num_of_missed_blocks));
       // Ensure schedule is still correct
-      confirm_schedule_correctness(second_eff_block_num_and_schd_tuple);
+      confirm_schedule_correctness(second_prod_schd, eff_second_prod_schd_block_num);
       produce_block();
 
-
       // ---- Test third set of producers ----
-      vector<account_name> third_set_producer_order = {
+      vector<account_name> third_set_of_producer = {
               producers[2], producers[5], producers[8], producers[11], producers[14], producers[17], producers[20],
               producers[0], producers[3], producers[6], producers[9], producers[12], producers[15], producers[18],
               producers[1], producers[4], producers[7], producers[10], producers[13], producers[16], producers[19]
       };
       // Send set prods action and confirm schedule correctness
-      const auto& third_eff_block_num_and_schd_tuple = set_producers(third_set_producer_order);
-      confirm_schedule_correctness(third_eff_block_num_and_schd_tuple);
+      const auto& third_prod_schd = set_producers(third_set_of_producer, 3);
+      const auto& eff_third_prod_schd_block_num = calc_block_num_of_next_round_first_block(*control);
+      confirm_schedule_correctness(third_prod_schd, eff_third_prod_schd_block_num);
+
+   } FC_LOG_AND_RETHROW()
+
+
+   BOOST_FIXTURE_TEST_CASE( verify_header_schedule_version, tester ) try {
+
+      // Utility function to ensure that producer schedule version in the header is correct
+      const auto& confirm_header_schd_ver_correctness = [&](const uint64_t expected_version, const uint64_t eff_new_prod_schd_block_num)  {
+         const uint32_t check_duration = 1000; // number of blocks
+         for (uint32_t i = 0; i < check_duration; ++i) {
+            // The new schedule will only be applied once the effective block num is deemed irreversible
+            const bool is_new_schedule_applied = control->last_irreversible_block_num() > eff_new_prod_schd_block_num;
+
+            // Produce block
+            produce_block();
+
+            // Ensure that the head block header is updated at the right time
+            const auto current_schd_ver = control->head_block_header().schedule_version;
+            if (is_new_schedule_applied) {
+               BOOST_TEST(current_schd_ver == expected_version);
+            } else {
+               BOOST_TEST(current_schd_ver != expected_version);
+            }
+         }
+      };
+
+      // Create producer accounts
+      vector<account_name> producers = {
+              "inita", "initb", "initc", "initd", "inite", "initf", "initg",
+              "inith", "initi", "initj", "initk", "initl", "initm", "initn",
+              "inito", "initp", "initq", "initr", "inits", "initt", "initu"
+      };
+      create_accounts(producers);
+
+      // Send set prods action and confirm schedule correctness
+      set_producers(producers, 1);
+      const auto& eff_first_prod_schd_block_num = calc_block_num_of_next_round_first_block(*control);
+      // Confirm the version is correct
+      confirm_header_schd_ver_correctness(1, eff_first_prod_schd_block_num);
+
+      // Shuffle the producers and set the new one with smaller version
+      boost::range::random_shuffle(producers);
+      set_producers(producers, 0);
+      const auto& eff_second_prod_schd_block_num = calc_block_num_of_next_round_first_block(*control);
+      // Even though we set it with smaller version number, the version should be incremented instead
+      confirm_header_schd_ver_correctness(2, eff_second_prod_schd_block_num);
+
+      // Shuffle the producers and set the new one with much larger version number
+      boost::range::random_shuffle(producers);
+      set_producers(producers, 1000);
+      const auto& eff_third_prod_schd_block_num = calc_block_num_of_next_round_first_block(*control);
+      // Even though we set it with much large version number, the version should be incremented instead
+      confirm_header_schd_ver_correctness(3, eff_third_prod_schd_block_num);
 
    } FC_LOG_AND_RETHROW()