BSIP 40: Changes from code review

Author: nathanielhourt

libraries/chain/custom_authority_evaluator.cpp

@@ -48,15 +48,21 @@ void_result custom_authority_create_evaluator::do_evaluate(const custom_authorit
    bool operation_forked_in = hardfork_visitor(now).visit((operation::tag_type)op.operation_type.value);
    FC_ASSERT(operation_forked_in, "Cannot create custom authority for operation which is not valid yet");
 
+   auto restriction_count = std::for_each(op.restrictions.begin(), op.restrictions.end(), restriction::adder()).sum;
+   FC_ASSERT(restriction_count <= config->max_custom_authority_restrictions,
+             "Custom authority has more than the maximum number of restrictions");
+
    for (const auto& account_weight_pair : op.auth.account_auths)
       account_weight_pair.first(d);
 
    const auto& index = d.get_index_type<custom_authority_index>().indices().get<by_account_custom>();
    auto range = index.equal_range(op.account);
    FC_ASSERT(std::distance(range.first, range.second) < config->max_custom_authorities_per_account,
-             "Cannot create custom authority for account: account already has maximum number");
+             "Cannot create custom authority: account already has maximum number");
+   range = index.equal_range(boost::make_tuple(op.account, op.operation_type));
+   FC_ASSERT(std::distance(range.first, range.second) < config->max_custom_authorities_per_account_op,
+             "Cannot create custom authority: account already has maximum number for this operation type");
 
-   get_restriction_predicate(op.restrictions, op.operation_type);
    return void_result();
 } FC_CAPTURE_AND_RETHROW((op)) }
 
@@ -74,21 +80,16 @@ object_id_type custom_authority_create_evaluator::do_apply(const custom_authorit
       std::for_each(op.restrictions.begin(), op.restrictions.end(), [&obj](const restriction& r) mutable {
          obj.restrictions.insert(std::make_pair(obj.restriction_counter++, r));
       });
-
-      // Update the predicate cache
-      obj.update_predicate_cache();
    }).id;
 } FC_CAPTURE_AND_RETHROW((op)) }
 
 void_result custom_authority_update_evaluator::do_evaluate(const custom_authority_update_operation& op)
 { try {
    const database& d = db();
    auto now = d.head_block_time();
-   FC_ASSERT(HARDFORK_BSIP_40_PASSED(now), "Custom active authorities are not yet enabled");
    old_object = &op.authority_to_update(d);
    FC_ASSERT(old_object->account == op.account, "Cannot update a different account's custom authority");
 
-   op.account(d);
    if (op.new_enabled)
       FC_ASSERT(*op.new_enabled != old_object->enabled,
                 "Custom authority update specifies an enabled flag, but flag is not changed");
@@ -131,6 +132,17 @@ void_result custom_authority_update_evaluator::do_evaluate(const custom_authorit
                 "Unable to add restrictions: causes wraparound of restriction IDs");
    }
 
+   // Add up the restriction counts for all old restrictions not being removed, and all new ones
+   size_t restriction_count = 0;
+   for (const auto& restriction_pair : old_object->restrictions)
+      if (op.restrictions_to_remove.count(restriction_pair.first) == 0)
+         restriction_count += restriction_pair.second.restriction_count();
+   restriction_count += std::for_each(op.restrictions_to_add.begin(), op.restrictions_to_add.end(),
+                                      restriction::adder()).sum;
+   // Check restriction count against limit
+   FC_ASSERT(restriction_count <= config->max_custom_authority_restrictions,
+             "Cannot update custom authority: updated authority would exceed the maximum number of restrictions");
+
    get_restriction_predicate(op.restrictions_to_add, old_object->operation_type);
    return void_result();
 } FC_CAPTURE_AND_RETHROW((op)) }
@@ -152,8 +164,8 @@ void_result custom_authority_update_evaluator::do_apply(const custom_authority_u
          obj.restrictions.insert(std::make_pair(obj.restriction_counter++, r));
       });
 
-      // Update the predicate cache
-      obj.update_predicate_cache();
+      // Clear the predicate cache
+      obj.clear_predicate_cache();
    });
 
    return void_result();
@@ -162,9 +174,7 @@ void_result custom_authority_update_evaluator::do_apply(const custom_authority_u
 void_result custom_authority_delete_evaluator::do_evaluate(const custom_authority_delete_operation& op)
 { try {
    const database& d = db();
-   FC_ASSERT(HARDFORK_BSIP_40_PASSED(d.head_block_time()), "Custom active authorities are not yet enabled");
 
-   op.account(d);
    old_object = &op.authority_to_delete(d);
    FC_ASSERT(old_object->account == op.account, "Cannot delete a different account's custom authority");
 

libraries/chain/include/graphene/chain/custom_authority_object.hpp

@@ -39,7 +39,7 @@ namespace graphene { namespace chain {
    class custom_authority_object : public abstract_object<custom_authority_object> {
       /// Unreflected field to store a cache of the predicate function
       /// Note that this cache can be modified when the object is const!
-      optional<restriction_predicate_function> predicate_cache;
+      mutable optional<restriction_predicate_function> predicate_cache;
 
    public:
       static const uint8_t space_id = protocol_ids;
@@ -66,16 +66,17 @@ namespace graphene { namespace chain {
       }
       /// Get predicate, from cache if possible, and update cache if not (modifies const object!)
       restriction_predicate_function get_predicate() const {
-         if (predicate_cache.valid())
-            return *predicate_cache;
+         if (!predicate_cache.valid())
+            update_predicate_cache();
 
-         const_cast<custom_authority_object*>(this)->update_predicate_cache();
          return *predicate_cache;
       }
       /// Regenerate predicate function and update predicate cache
-      void update_predicate_cache() {
+      void update_predicate_cache() const {
          predicate_cache = get_restriction_predicate(get_restrictions(), operation_type);
       }
+      /// Clear the cache of the predicate function
+      void clear_predicate_cache() { predicate_cache.reset(); }
    };
 
    struct by_account_custom;

libraries/chain/proposal_object.cpp

@@ -41,7 +41,7 @@ bool proposal_object::is_authorized_to_execute( database& db ) const
                         available_key_approvals,
                         [&db]( account_id_type id ){ return &id( db ).active; },
                         [&db]( account_id_type id ){ return &id( db ).owner;  },
-                        [&]( account_id_type id, const operation& op, rejected_predicate_map* rejects ){
+                        [&db]( account_id_type id, const operation& op, rejected_predicate_map* rejects ){
                            return db.get_viable_custom_authorities(id, op, rejects); },
                         allow_non_immediate_owner,
                         MUST_IGNORE_CUSTOM_OP_REQD_AUTHS( db.head_block_time() ),

libraries/protocol/custom_authorities/restriction_predicate.hxx

@@ -283,13 +283,12 @@ template<typename FieldContainer, typename ArgumentElement>
 struct predicate_has_all<FieldContainer, flat_set<ArgumentElement>,
                          std::enable_if_t<is_container<FieldContainer> && !is_flat_set<FieldContainer> &&
                                           comparable_types<typename FieldContainer::value_type, ArgumentElement>>> {
-   predicate_has_all<flat_set<typename FieldContainer::value_type>, flat_set<ArgumentElement>> inner;
    // Field is other container; convert to flat_set
    constexpr static bool valid = true;
    bool operator()(const FieldContainer& f, const flat_set<ArgumentElement>& a) const {
       if (f.size() < a.size()) return false;
-      flat_set<typename FieldContainer::value_type> fs(f.begin(), f.end());
-      return inner(fs, a);
+      std::set<typename FieldContainer::value_type> fs(f.begin(), f.end());
+      return std::includes(fs.begin(), fs.end(), a.begin(), a.end());
    }
 };
 template<typename OptionalType, typename Argument>
@@ -319,12 +318,10 @@ template<typename FieldContainer, typename ArgumentElement>
 struct predicate_has_none<FieldContainer, flat_set<ArgumentElement>,
                           std::enable_if_t<is_container<FieldContainer> && !is_flat_set<FieldContainer> &&
                                            comparable_types<typename FieldContainer::value_type, ArgumentElement>>> {
-   predicate_has_none<flat_set<typename FieldContainer::value_type>, flat_set<ArgumentElement>> inner;
-   // Field is other container; convert to flat_set
+   // Field is other container
    constexpr static bool valid = true;
    bool operator()(const FieldContainer& f, const flat_set<ArgumentElement>& a) const {
-      flat_set<typename FieldContainer::value_type> fs(f.begin(), f.end());
-      return inner(fs, a);
+      return !std::any_of(f.begin(), f.end(), [&a](const auto& fe) { return a.count(fe) > 0; });
    }
 };
 template<typename OptionalType, typename Argument>

libraries/protocol/include/graphene/protocol/chain_parameters.hpp

@@ -39,6 +39,8 @@ namespace graphene { namespace protocol {
    {
       uint32_t max_custom_authority_lifetime_seconds = GRAPHENE_DEFAULT_MAX_CUSTOM_AUTHORITY_LIFETIME_SECONDS;
       uint32_t max_custom_authorities_per_account = GRAPHENE_DEFAULT_MAX_CUSTOM_AUTHORITIES_PER_ACCOUNT;
+      uint32_t max_custom_authorities_per_account_op = GRAPHENE_DEFAULT_MAX_CUSTOM_AUTHORITIES_PER_ACCOUNT_OP;
+      uint32_t max_custom_authority_restrictions = GRAPHENE_DEFAULT_MAX_CUSTOM_AUTHORITY_RESTRICTIONS;
    };
 
    struct chain_parameters
@@ -108,6 +110,8 @@ FC_REFLECT( graphene::protocol::htlc_options,
 FC_REFLECT( graphene::protocol::custom_authority_options_type,
       (max_custom_authority_lifetime_seconds)
       (max_custom_authorities_per_account)
+      (max_custom_authorities_per_account_op)
+      (max_custom_authority_restrictions)
 )
 
 FC_REFLECT( graphene::protocol::chain_parameters::ext,

libraries/protocol/include/graphene/protocol/config.hpp

@@ -139,7 +139,11 @@
 
 #define GRAPHENE_FBA_STEALTH_DESIGNATED_ASSET (asset_id_type(743))
 
-/// Maximum duration before a custom authority can expire
-#define GRAPHENE_DEFAULT_MAX_CUSTOM_AUTHORITY_LIFETIME_SECONDS (60*60*24*365)
+/// Maximum duration before a custom authority can expire (1 month)
+#define GRAPHENE_DEFAULT_MAX_CUSTOM_AUTHORITY_LIFETIME_SECONDS (60*60*24*30)
 /// Maximum number of custom authorities a particular account can set
 #define GRAPHENE_DEFAULT_MAX_CUSTOM_AUTHORITIES_PER_ACCOUNT 10
+/// Maximum number of custom authorities a particular account can set for a particular operation
+#define GRAPHENE_DEFAULT_MAX_CUSTOM_AUTHORITIES_PER_ACCOUNT_OP 3
+/// Maximum number of restrictions a custom authority can contain
+#define GRAPHENE_DEFAULT_MAX_CUSTOM_AUTHORITY_RESTRICTIONS 10

libraries/protocol/include/graphene/protocol/restriction.hpp

@@ -107,6 +107,26 @@ struct restriction {
    restriction() = default;
    restriction(const unsigned_int& member_index, function_type type, const argument_type& argument)
       : member_index(member_index), restriction_type(type), argument(argument) {}
+
+   struct adder {
+      size_t sum = 0;
+      void operator()(const restriction& r) { sum += r.restriction_count(); }
+      void operator()(const vector<restriction>& r) { sum += std::for_each(r.begin(), r.end(), adder()).sum; }
+   };
+
+   size_t restriction_count() const {
+      if (argument.is_type<vector<restriction>>()) {
+         const vector<restriction>& rs = argument.get<vector<restriction>>();
+         return 1 + std::for_each(rs.begin(), rs.end(), adder()).sum;
+      } else if (argument.is_type<vector<vector<restriction>>>()) {
+         const vector<vector<restriction>>& rs = argument.get<vector<vector<restriction>>>();
+         return 1 + std::for_each(rs.begin(), rs.end(), adder()).sum;
+      } else if (argument.is_type<variant_assert_argument_type>()) {
+         const variant_assert_argument_type& arg = argument.get<variant_assert_argument_type>();
+         return 1 + std::for_each(arg.second.begin(), arg.second.end(), adder()).sum;
+      }
+      return 1;
+   }
 };
 
 } } // graphene::protocol

tests/tests/custom_authority_tests.cpp

@@ -52,6 +52,9 @@ BOOST_FIXTURE_TEST_SUITE(custom_authority_tests, database_fixture)
 
 #define FUNC(TYPE) BOOST_PP_CAT(restriction::func_, TYPE)
 
+size_t restriction_count(const vector<restriction>& rs) {
+   return std::for_each(rs.begin(), rs.end(), restriction::adder()).sum;
+}
 template<typename Object>
 unsigned_int member_index(string name) {
    unsigned_int index;
@@ -106,6 +109,7 @@ BOOST_AUTO_TEST_CASE(restriction_predicate_tests) { try {
    //    "extensions": []
    //  }
    //]
+   BOOST_CHECK_EQUAL(restriction_count(restrictions), 1);
    BOOST_CHECK(get_restriction_predicate(restrictions, operation::tag<transfer_operation>::value)(transfer)
                .rejection_path.size() == 2);
    // Index 0 (the outer-most) rejection path refers to the first and only restriction
@@ -222,6 +226,7 @@ BOOST_AUTO_TEST_CASE(restriction_predicate_tests) { try {
    //  }
    //]
 
+   BOOST_CHECK_EQUAL(restriction_count(restrictions), 2);
    //////
    // Check the transfer operation that pays the fee with Asset ID 0 against the restriction.
    // This should violate the restriction.
@@ -275,6 +280,7 @@ BOOST_AUTO_TEST_CASE(restriction_predicate_tests) { try {
    //    "extensions": []
    //  }
    //]
+   BOOST_CHECK_EQUAL(restriction_count(restrictions), 3);
 
    //////
    // Create a transfer operation that authorizes transfer to Account ID 12
@@ -332,6 +338,7 @@ BOOST_AUTO_TEST_CASE(restriction_predicate_tests) { try {
    //    "extensions": []
    //  }
    //]
+   BOOST_CHECK_EQUAL(restriction_count(restrictions), 2);
    auto predicate = get_restriction_predicate(restrictions, operation::tag<account_update_operation>::value);
 
    //////
@@ -446,6 +453,7 @@ BOOST_AUTO_TEST_CASE(restriction_predicate_tests) { try {
          //    "extensions": []
          //  }
          //]
+         BOOST_CHECK_EQUAL(restriction_count(or_restrictions), 3);
          auto predicate = get_restriction_predicate(or_restrictions, operation::tag<transfer_operation>::value);
 
          //////
@@ -562,6 +570,7 @@ BOOST_AUTO_TEST_CASE(custom_auths) { try {
    //    "extensions": []
    //  }
    //]
+   BOOST_CHECK_EQUAL(restriction_count(op.restrictions), 3);
 
 
    //////
@@ -1634,6 +1643,7 @@ BOOST_AUTO_TEST_CASE(custom_auths) { try {
          //    "extensions": []
          //  }
          //]
+         BOOST_CHECK_EQUAL(restriction_count(op.restrictions), 3);
 
          // Publish the new custom authority
          trx.clear();