properly handle producer_authority field in producers table - v1.9.x

contracts/eosio.system/include/eosio.system/eosio.system.hpp

@@ -179,6 +179,10 @@ namespace eosiosystem {
       EOSLIB_SERIALIZE( eosio_global_state4, (continuous_rate)(inflation_pay_factor)(votepay_factor) )
    };
 
+   inline eosio::block_signing_authority convert_to_block_signing_authority( const eosio::public_key& producer_key ) {
+      return eosio::block_signing_authority_v0{ .threshold = 1, .keys = {{producer_key, 1}} };
+   }
+
    // Defines `producer_info` structure to be stored in `producer_info` table, added after version 1.0
    struct [[eosio::table, eosio::contract("eosio.system")]] producer_info {
       name                                                     owner;
@@ -196,9 +200,56 @@ namespace eosiosystem {
       bool     active()const      { return is_active;                               }
       void     deactivate()       { producer_key = public_key(); producer_authority.reset(); is_active = false; }
 
-      // explicit serialization macro is not necessary, used here only to improve compilation time
-      EOSLIB_SERIALIZE( producer_info, (owner)(total_votes)(producer_key)(is_active)(url)
-                        (unpaid_blocks)(last_claim_time)(location)(producer_authority) )
+      eosio::block_signing_authority get_producer_authority()const {
+         if( producer_authority.has_value() ) {
+            bool zero_threshold = std::visit( [](auto&& auth ) -> bool {
+               return (auth.threshold == 0);
+            }, *producer_authority );
+            // zero_threshold could be true despite the validation done in regproducer2 because the v1.9.0 eosio.system
+            // contract has a bug which may have modified the producer table such that the producer_authority field
+            // contains a default constructed eosio::block_signing_authority (which has a 0 threshold and so is invalid).
+            if( !zero_threshold ) return *producer_authority;
+         }
+         return convert_to_block_signing_authority( producer_key );
+      }
+
+      // The unregprod and claimrewards actions modify unrelated fields of the producers table and under the default
+      // serialization behavior they would increase the size of the serialized table if the producer_authority field
+      // was not already present. This is acceptable (though not necessarily desired) because those two actions require
+      // the authority of the producer who pays for the table rows.
+      // However, the rmvproducer action and the onblock transaction would also modify the producer table in a similar
+      // way and increasing its serialized size is not acceptable in that context.
+      // So, a custom serialization is defined to handle the binary_extension producer_authority
+      // field in the desired way. (Note: v1.9.0 did not have this custom serialization behavior.)
+
+      template<typename DataStream>
+      friend DataStream& operator << ( DataStream& ds, const producer_info& t ) {
+         ds << t.owner
+            << t.total_votes
+            << t.producer_key
+            << t.is_active
+            << t.url
+            << t.unpaid_blocks
+            << t.last_claim_time
+            << t.location;
+
+         if( !t.producer_authority.has_value() ) return ds;
+
+         return ds << t.producer_authority;
+      }
+
+      template<typename DataStream>
+      friend DataStream& operator >> ( DataStream& ds, producer_info& t ) {
+         return ds >> t.owner
+                   >> t.total_votes
+                   >> t.producer_key
+                   >> t.is_active
+                   >> t.url
+                   >> t.unpaid_blocks
+                   >> t.last_claim_time
+                   >> t.location
+                   >> t.producer_authority;
+      }
    };
 
    // Defines new producer info structure to be stored in new producer info table, added after version 1.3.0
@@ -343,8 +394,8 @@ namespace eosiosystem {
    // - `version` defaulted to zero,
    // - `last_dist_time` the last time proceeds from renting, ram fees, and name bids were added to the rex pool,
    // - `pending_bucket_time` timestamp of the pending 12-hour return bucket,
-   // - `oldest_bucket_time` cached timestamp of the oldest 12-hour return bucket, 
-   // - `pending_bucket_proceeds` proceeds in the pending 12-hour return bucket, 
+   // - `oldest_bucket_time` cached timestamp of the oldest 12-hour return bucket,
+   // - `pending_bucket_proceeds` proceeds in the pending 12-hour return bucket,
    // - `current_rate_of_increase` the current rate per dist_interval at which proceeds are added to the rex pool,
    // - `proceeds` the maximum amount of proceeds that can be added to the rex pool at any given time
    struct [[eosio::table,eosio::contract("eosio.system")]] rex_return_pool {
@@ -368,7 +419,7 @@ namespace eosiosystem {
 
    // `rex_return_buckets` structure underlying the rex return buckets table. A rex return buckets table is defined by:
    // - `version` defaulted to zero,
-   // - `return_buckets` buckets of proceeds accumulated in 12-hour intervals 
+   // - `return_buckets` buckets of proceeds accumulated in 12-hour intervals
    struct [[eosio::table,eosio::contract("eosio.system")]] rex_return_buckets {
       uint8_t                           version = 0;
       std::map<time_point_sec, int64_t> return_buckets;

contracts/eosio.system/src/voting.cpp

@@ -24,10 +24,6 @@ namespace eosiosystem {
    using eosio::microseconds;
    using eosio::singleton;
 
-   eosio::block_signing_authority convert_to_block_signing_authority( const eosio::public_key& producer_key ) {
-      return eosio::block_signing_authority_v0{ .threshold = 1, .keys = {{producer_key, 1}} };
-   }
-
    void system_contract::register_producer( const name& producer, const eosio::block_signing_authority& producer_authority, const std::string& url, uint16_t location ) {
       auto prod = _producers.find( producer.value );
       const auto ct = current_time_point();
@@ -120,8 +116,7 @@ namespace eosiosystem {
          top_producers.emplace_back(
             eosio::producer_authority{
                .producer_name = it->owner,
-               .authority     = it->producer_authority.has_value() ? *it->producer_authority
-                                                                   : convert_to_block_signing_authority( it->producer_key )
+               .authority     = it->get_producer_authority()
             },
             it->location
          );

tests/contracts.hpp.in

@@ -18,10 +18,12 @@ struct contracts {
    struct util {
       static std::vector<uint8_t> reject_all_wasm() { return read_wasm("${CMAKE_SOURCE_DIR}/test_contracts/reject_all.wasm"); }
       static std::vector<uint8_t> exchange_wasm() { return read_wasm("${CMAKE_SOURCE_DIR}/test_contracts/exchange.wasm"); }
-      static std::vector<uint8_t> system_wasm_old() { return read_wasm("${CMAKE_SOURCE_DIR}/test_contracts/eosio.system.old/eosio.system.wasm"); }
-      static std::vector<char>    system_abi_old() { return read_abi("${CMAKE_SOURCE_DIR}/test_contracts/eosio.system.old/eosio.system.abi"); }
-      static std::vector<uint8_t> msig_wasm_old() { return read_wasm("${CMAKE_SOURCE_DIR}/test_contracts/eosio.msig.old/eosio.msig.wasm"); }
-      static std::vector<char>    msig_abi_old() { return read_abi("${CMAKE_SOURCE_DIR}/test_contracts/eosio.msig.old/eosio.msig.abi"); }
+      static std::vector<uint8_t> system_wasm_v1_8() { return read_wasm("${CMAKE_SOURCE_DIR}/test_contracts/old_versions/v1.8.3/eosio.system/eosio.system.wasm"); }
+      static std::vector<char>    system_abi_v1_8() { return read_abi("${CMAKE_SOURCE_DIR}/test_contracts/old_versions/v1.8.3/eosio.system/eosio.system.abi"); }
+      static std::vector<uint8_t> system_wasm_old() { return read_wasm("${CMAKE_SOURCE_DIR}/test_contracts/old_versions/v1.2.1/eosio.system/eosio.system.wasm"); }
+      static std::vector<char>    system_abi_old() { return read_abi("${CMAKE_SOURCE_DIR}/test_contracts/old_versions/v1.2.1/eosio.system/eosio.system.abi"); }
+      static std::vector<uint8_t> msig_wasm_old() { return read_wasm("${CMAKE_SOURCE_DIR}/test_contracts/old_versions/v1.2.1/eosio.msig/eosio.msig.wasm"); }
+      static std::vector<char>    msig_abi_old() { return read_abi("${CMAKE_SOURCE_DIR}/test_contracts/old_versions/v1.2.1/eosio.msig/eosio.msig.abi"); }
    };
 };
 }} //ns eosio::testing

tests/eosio.system_tests.cpp

@@ -899,6 +899,103 @@ BOOST_FIXTURE_TEST_CASE( producer_wtmsig, eosio_system_tester ) try {
 
 } FC_LOG_AND_RETHROW()
 
+BOOST_FIXTURE_TEST_CASE( producer_wtmsig_transition, eosio_system_tester ) try {
+   cross_15_percent_threshold();
+
+   BOOST_REQUIRE_EQUAL( control->active_producers().version, 0u );
+
+   set_code( config::system_account_name, contracts::util::system_wasm_v1_8() );
+   set_abi(  config::system_account_name, contracts::util::system_abi_v1_8().data() );
+
+   issue_and_transfer( N(alice1111111), core_sym::from_string("200000000.0000"),  config::system_account_name );
+   BOOST_REQUIRE_EQUAL( success(), push_action( N(alice1111111), N(regproducer), mvo()
+                                               ("producer",  "alice1111111")
+                                               ("producer_key", get_public_key( N(alice1111111), "active") )
+                                               ("url","")
+                                               ("location", 0)
+                        )
+   );
+   BOOST_REQUIRE_EQUAL( success(), stake( N(alice1111111), core_sym::from_string("100000000.0000"), core_sym::from_string("100000000.0000") ) );
+   BOOST_REQUIRE_EQUAL( success(), vote( N(alice1111111), { N(alice1111111) } ) );
+
+   //auto alice_prod_info1 = get_producer_info( N(alice1111111) );
+   //wdump((alice_prod_info1));
+
+   produce_block();
+   produce_block( fc::minutes(2) );
+   produce_blocks(2);
+   BOOST_REQUIRE_EQUAL( control->active_producers().version, 1u );
+
+   const auto schedule_update1 = get_global_state()["last_producer_schedule_update"];
+
+   const auto& rlm = control->get_resource_limits_manager();
+
+   auto alice_initial_ram_usage = rlm.get_account_ram_usage(N(alice1111111));
+
+   set_code( config::system_account_name, contracts::system_wasm() );
+   set_abi(  config::system_account_name, contracts::system_abi().data() );
+   produce_block();
+   BOOST_REQUIRE_EQUAL( control->pending_block_producer(), N(alice1111111) );
+
+   auto alice_prod_info2 = get_producer_info( N(alice1111111) );
+   BOOST_REQUIRE_EQUAL( alice_prod_info2["is_active"], true );
+
+   produce_block( fc::minutes(2) );
+   const auto schedule_update2 = get_global_state()["last_producer_schedule_update"];
+   BOOST_REQUIRE( schedule_update1 < schedule_update2 ); // Ensure last_producer_schedule_update is increasing.
+
+   // Producing the above block would trigger the bug in v1.9.0 that sets the default block_signing_authority
+   // in the producer object of the currently active producer alice1111111.
+   // However, starting in v1.9.1, the producer object does not have a default block_signing_authority added to the
+   // serialization of the producer object if it was not already present in the binary extension field
+   // producer_authority to begin with. This is verified below by ensuring the RAM usage of alice (who pays for the
+   // producer object) does not increase.
+
+   auto alice_ram_usage = rlm.get_account_ram_usage(N(alice1111111));
+   BOOST_CHECK_EQUAL( alice_initial_ram_usage, alice_ram_usage );
+
+   auto alice_prod_info3 = get_producer_info( N(alice1111111) );
+   if( alice_prod_info3.get_object().contains("producer_authority") ) {
+      BOOST_CHECK_EQUAL( alice_prod_info3["producer_authority"][1]["threshold"], 0 );
+   }
+
+   produce_block( fc::minutes(2) );
+   const auto schedule_update3 = get_global_state()["last_producer_schedule_update"];
+
+   // The bug in v1.9.0 would cause alice to have an invalid producer authority (the default block_signing_authority).
+   // The v1.9.0 system contract would have attempted to set a proposed producer schedule including this invalid
+   // authority which would be rejected by the EOSIO native system and cause the onblock transaction to continue to fail.
+   // This could be observed by noticing that last_producer_schedule_update was not being updated even though it should.
+   // However, starting in v1.9.1, update_elected_producers is smarter about the producer schedule it constructs to
+   // propose to the system. It will recognize the default constructed authority (which shouldn't be created by the
+   // v1.9.1 system contract but may still exist in the tables if it was constructed by the buggy v1.9.0 system contract)
+   // and instead resort to constructing the block signing authority from the single producer key in the table.
+   // So newer system contracts should see onblock continue to function, which is verified by the check below.
+
+   BOOST_CHECK( schedule_update2 < schedule_update3 ); // Ensure last_producer_schedule_update is increasing.
+
+   // But even if the buggy v1.9.0 system contract was running, it should always still be possible to recover
+   // by having the producer with the invalid authority simply call regproducer or regproducer2 to correct their
+   // block signing authority.
+
+   BOOST_REQUIRE_EQUAL( success(), push_action( N(alice1111111), N(regproducer), mvo()
+                                               ("producer",  "alice1111111")
+                                               ("producer_key", get_public_key( N(alice1111111), "active") )
+                                               ("url","")
+                                               ("location", 0)
+                        )
+   );
+
+   produce_block();
+   produce_block( fc::minutes(2) );
+
+   auto alice_prod_info4 = get_producer_info( N(alice1111111) );
+   BOOST_REQUIRE_EQUAL( alice_prod_info4["is_active"], true );
+   const auto schedule_update4 = get_global_state()["last_producer_schedule_update"];
+   BOOST_REQUIRE( schedule_update2 < schedule_update4 );
+
+} FC_LOG_AND_RETHROW()
+
 BOOST_FIXTURE_TEST_CASE( vote_for_producer, eosio_system_tester, * boost::unit_test::tolerance(1e+5) ) try {
    cross_15_percent_threshold();
 
@@ -5408,7 +5505,7 @@ BOOST_FIXTURE_TEST_CASE( rex_return, eosio_system_tester ) try {
       BOOST_REQUIRE_EQUAL( success(),        rexexec( bob, 1 ) );
       BOOST_REQUIRE_EQUAL( 0,                get_rex_return_buckets()["return_buckets"].get_array().size() );
    }
-   
+
 } FC_LOG_AND_RETHROW()
 
 

tests/test_contracts/old_versions/v1.2.1/eosio.system/Readme.txt

@@ -0,0 +1,3 @@
+Compiled with
+eosio.contracts: bf28f8bbf9ee753966c95c586906e82d72f9dfac (v1.2.1)
+eosio.wasmsdk:   2c106a018f2e69d34325ef2376cf085426068e93, CORE_SYMBOL_NAME = "SYS"

tests/test_contracts/old_versions/v1.8.3/eosio.system/README.txt

@@ -0,0 +1,3 @@
+Compiled with
+eosio.contracts: 7109f002fa9afff18edcafce5c32b224a21eef98 (v1.8.3)
+eosio.cdt:       263e41cbb3e58dca71117401f43be104f1e58ae4 (v1.6.3)