Ref bitshares#210: Add hardfork guards

Hardfork guards are complex for this issue, because we can't access
chain time in the operation's get_required_active_authorities() method.

There was no apparent 'good' way to solve this, so I settled for a
fairly nasty hack which seemed the least bad of bad options. Add a
boolean parameter to the verify_authority() functions determining
whether the custom_operation::required_auths field should be ignored or
not, and call these functions setting the boolean appropriately for
whether we have passed the hardfork or not. This works because chain
time is available at the verify_authority() call sites.

libraries/chain/db_block.cpp

@@ -631,7 +631,18 @@ processed_transaction database::_apply_transaction(const signed_transaction& trx
    {
       auto get_active = [&]( account_id_type id ) { return &id(*this).active; };
       auto get_owner  = [&]( account_id_type id ) { return &id(*this).owner;  };
-      trx.verify_authority( chain_id, get_active, get_owner, get_global_properties().parameters.max_authority_depth );
+
+      // See bitshares-core issue #210 for discussion
+      // The custom_operation::get_required_active_authorities() method initially failed to report the authorities
+      // from the custom_operaton::required_auths field. This was a bug. It's a simple fix in that method, but the
+      // fix is a hardfork, and thus we need a hardfork guard. Since that method cannot access chain time, we must
+      // implement the guard here, and skip the call to get_required_active_authorities() prior to the hardfork.
+      // Therefore, if the head_block_time() is prior to the 210 hardfork, we ignore the required auths specified
+      // by the custom_operation.
+      bool ignore_custom_operation_required_auths = (head_block_time() <= HARDFORK_CORE_210_TIME);
+
+      trx.verify_authority(chain_id, get_active, get_owner, ignore_custom_operation_required_auths,
+                           get_global_properties().parameters.max_authority_depth);
    }
 
    //Skip all manner of expiration and TaPoS checking if we're on block 1; It's impossible that the transaction is

libraries/chain/hardfork.d/CORE_210.hf

@@ -0,0 +1,4 @@
+// #210 Check authorities on custom_operation
+#ifndef HARDFORK_CORE_210_TIME
+#define HARDFORK_CORE_210_TIME (fc::time_point_sec(1893456000)) // Jan 1 00:00:00 2030 (Not yet scheduled)
+#endif

libraries/chain/include/graphene/chain/protocol/transaction.hpp

@@ -149,11 +149,11 @@ namespace graphene { namespace chain {
          uint32_t max_recursion = GRAPHENE_MAX_SIG_CHECK_DEPTH
          )const;
 
-      void verify_authority(
-         const chain_id_type& chain_id,
-         const std::function<const authority*(account_id_type)>& get_active,
-         const std::function<const authority*(account_id_type)>& get_owner,
-         uint32_t max_recursion = GRAPHENE_MAX_SIG_CHECK_DEPTH )const;
+      void verify_authority(const chain_id_type& chain_id,
+                            const std::function<const authority*(account_id_type)>& get_active,
+                            const std::function<const authority*(account_id_type)>& get_owner,
+                            bool ignore_custom_operation_required_auths = false,
+                            uint32_t max_recursion = GRAPHENE_MAX_SIG_CHECK_DEPTH)const;
 
       /**
        * This is a slower replacement for get_required_signatures()
@@ -213,13 +213,14 @@ namespace graphene { namespace chain {
       mutable bool _validated = false;
    };
 
-   void verify_authority( const vector<operation>& ops, const flat_set<public_key_type>& sigs,
-                          const std::function<const authority*(account_id_type)>& get_active,
-                          const std::function<const authority*(account_id_type)>& get_owner,
-                          uint32_t max_recursion = GRAPHENE_MAX_SIG_CHECK_DEPTH,
-                          bool allow_committe = false,
-                          const flat_set<account_id_type>& active_aprovals = flat_set<account_id_type>(),
-                          const flat_set<account_id_type>& owner_approvals = flat_set<account_id_type>());
+   void verify_authority(const vector<operation>& ops, const flat_set<public_key_type>& sigs,
+                         const std::function<const authority*(account_id_type)>& get_active,
+                         const std::function<const authority*(account_id_type)>& get_owner,
+                         bool ignore_custom_operation_required_auths = false, ///< Ref issue #210
+                         uint32_t max_recursion = GRAPHENE_MAX_SIG_CHECK_DEPTH,
+                         bool allow_committe = false,
+                         const flat_set<account_id_type>& active_aprovals = flat_set<account_id_type>(),
+                         const flat_set<account_id_type>& owner_approvals = flat_set<account_id_type>());
 
    /**
     *  @brief captures the result of evaluating the operations contained in the transaction

libraries/chain/proposal_object.cpp

@@ -23,22 +23,33 @@
  */
 #include <graphene/chain/database.hpp>
 #include <graphene/chain/proposal_object.hpp>
+#include <graphene/chain/hardfork.hpp>
 
 namespace graphene { namespace chain {
 
 bool proposal_object::is_authorized_to_execute(database& db) const
 {
    transaction_evaluation_state dry_run_eval(&db);
 
+   // See bitshares-core issue #210 for discussion
+   // The custom_operation::get_required_active_authorities() method initially failed to report the authorities
+   // from the custom_operaton::required_auths field. This was a bug. It's a simple fix in that method, but the
+   // fix is a hardfork, and thus we need a hardfork guard. Since that method cannot access chain time, we must
+   // implement the guard here, and skip the call to get_required_active_authorities() prior to the hardfork.
+   // Therefore, if the head_block_time() is prior to the 210 hardfork, we ignore the required auths specified
+   // by the custom_operation.
+   bool ignore_custom_operation_required_auths = (db.head_block_time() <= HARDFORK_CORE_210_TIME);
+
    try {
-      verify_authority( proposed_transaction.operations, 
-                        available_key_approvals,
-                        [&]( account_id_type id ){ return &id(db).active; },
-                        [&]( account_id_type id ){ return &id(db).owner;  },
-                        db.get_global_properties().parameters.max_authority_depth,
-                        true, /* allow committeee */
-                        available_active_approvals,
-                        available_owner_approvals );
+      verify_authority(proposed_transaction.operations,
+                       available_key_approvals,
+                       [&]( account_id_type id ){ return &id(db).active; },
+                       [&]( account_id_type id ){ return &id(db).owner;  },
+                       ignore_custom_operation_required_auths,
+                       db.get_global_properties().parameters.max_authority_depth,
+                       true, /* allow committeee */
+                       available_active_approvals,
+                       available_owner_approvals);
    } 
    catch ( const fc::exception& e )
    {

libraries/chain/protocol/transaction.cpp

@@ -245,20 +245,25 @@ struct sign_state
 };
 
 
-void verify_authority( const vector<operation>& ops, const flat_set<public_key_type>& sigs,
-                       const std::function<const authority*(account_id_type)>& get_active,
-                       const std::function<const authority*(account_id_type)>& get_owner,
-                       uint32_t max_recursion_depth,
-                       bool  allow_committe,
-                       const flat_set<account_id_type>& active_aprovals,
-                       const flat_set<account_id_type>& owner_approvals )
+void verify_authority(const vector<operation>& ops, const flat_set<public_key_type>& sigs,
+                      const std::function<const authority*(account_id_type)>& get_active,
+                      const std::function<const authority*(account_id_type)>& get_owner,
+                      bool ignore_custom_operation_required_auths,
+                      uint32_t max_recursion_depth,
+                      bool  allow_committe,
+                      const flat_set<account_id_type>& active_aprovals,
+                      const flat_set<account_id_type>& owner_approvals)
 { try {
    flat_set<account_id_type> required_active;
    flat_set<account_id_type> required_owner;
    vector<authority> other;
 
-   for( const auto& op : ops )
-      operation_get_required_authorities( op, required_active, required_owner, other );
+   for (const auto& op : ops) {
+      if (op.which() == operation::tag<custom_operation>::value && ignore_custom_operation_required_auths)
+         required_active.insert(op.get<custom_operation>().fee_payer());
+      else
+         operation_get_required_authorities( op, required_active, required_owner, other );
+   }
 
    if( !allow_committe )
       GRAPHENE_ASSERT( required_active.find(GRAPHENE_COMMITTEE_ACCOUNT) == required_active.end(),
@@ -401,13 +406,14 @@ const flat_set<public_key_type>& precomputable_transaction::get_signature_keys(
    return _signees;
 }
 
-void signed_transaction::verify_authority(
-   const chain_id_type& chain_id,
-   const std::function<const authority*(account_id_type)>& get_active,
-   const std::function<const authority*(account_id_type)>& get_owner,
-   uint32_t max_recursion )const
+void signed_transaction::verify_authority(const chain_id_type& chain_id,
+                                          const std::function<const authority*(account_id_type)>& get_active,
+                                          const std::function<const authority*(account_id_type)>& get_owner,
+                                          bool ignore_custom_operation_required_auths,
+                                          uint32_t max_recursion)const
 { try {
-   graphene::chain::verify_authority( operations, get_signature_keys( chain_id ), get_active, get_owner, max_recursion );
+   graphene::chain::verify_authority(operations, get_signature_keys(chain_id), get_active, get_owner,
+                                     ignore_custom_operation_required_auths, max_recursion);
 } FC_CAPTURE_AND_RETHROW( (*this) ) }
 
 } } // graphene::chain

tests/tests/authority_tests.cpp

@@ -1438,12 +1438,43 @@ BOOST_FIXTURE_TEST_CASE( parent_owner_test, database_fixture )
    }
 }
 
-BOOST_AUTO_TEST_CASE( custom_operation_required_auths ) {
+BOOST_AUTO_TEST_CASE( custom_operation_required_auths_before_fork ) {
    try {
       ACTORS((alice)(bob));
       fund(alice);
       enable_fees();
 
+      if (db.head_block_time() >= HARDFORK_CORE_210_TIME) {
+          wlog("Unable to test custom_operation required auths before fork: hardfork already passed");
+          return;
+      }
+
+      signed_transaction trx;
+      custom_operation op;
+      op.payer = alice_id;
+      op.required_auths.insert(bob_id);
+      op.fee = op.calculate_fee(db.current_fee_schedule().get<custom_operation>());
+      trx.operations.emplace_back(op);
+      trx.set_expiration(db.head_block_time() + 30);
+      sign(trx, alice_private_key);
+      // Op requires bob's authorization, but only alice signed. We're before the fork, so this should work anyways.
+      db.push_transaction(trx);
+   } catch (fc::exception& e) {
+      edump((e.to_detail_string()));
+      throw;
+   }
+}
+
+BOOST_AUTO_TEST_CASE( custom_operation_required_auths_after_fork ) {
+   try {
+      ACTORS((alice)(bob));
+      fund(alice);
+
+      if (db.head_block_time() < HARDFORK_CORE_210_TIME)
+         generate_blocks(HARDFORK_CORE_210_TIME + 10);
+
+      enable_fees();
+
       signed_transaction trx;
       custom_operation op;
       op.payer = alice_id;
@@ -1452,8 +1483,10 @@ BOOST_AUTO_TEST_CASE( custom_operation_required_auths ) {
       trx.operations.emplace_back(op);
       trx.set_expiration(db.head_block_time() + 30);
       sign(trx, alice_private_key);
+      // Op require's bob's authorization, but only alice signed. This should throw.
       GRAPHENE_REQUIRE_THROW(db.push_transaction(trx), tx_missing_active_auth);
       sign(trx, bob_private_key);
+      // Now that bob has signed, it should work.
       PUSH_TX(db, trx);
    } catch (fc::exception& e) {
       edump((e.to_detail_string()));