Ref #7: Upgrade AuthorityChecker

I want two things from AuthorityChecker that it wasn't doing yet:
1. I want it to track which of the provided keys were used, so I can
reject transactions which bear more signatures than are necessary
2. To sign a transaction with no unnecessary keys, I want it to support
taking a list of available public keys and an authority, then telling me
which of those keys I should use to fully satisfy the authority, without
having any unnecessary keys

As an added bonus, having both of these operations handled by
AuthorityChecker means that determining the set of keys needed to sign a
transaction, and determining whether a transaction is properly signed,
use the same code. :D

libraries/chain/chain_controller.cpp

@@ -32,6 +32,7 @@
 #include <eos/chain/transaction_object.hpp>
 #include <eos/chain/producer_object.hpp>
 #include <eos/chain/permission_link_object.hpp>
+#include <eos/chain/authority_checker.hpp>
 
 #include <eos/chain/wasm_interface.hpp>
 
@@ -505,6 +506,9 @@ void chain_controller::check_transaction_authorization(const SignedTransaction&
                        ("auth", declaredAuthority));
          }
       }
+
+   EOS_ASSERT(checker.all_keys_used(), tx_irrelevant_sig,
+              "Transaction bears irrelevant signatures from these keys: ${keys}", ("keys", checker.unused_keys()));
 }
 
 ProcessedTransaction chain_controller::apply_transaction(const SignedTransaction& trx, uint32_t skip)

libraries/chain/include/eos/chain/authority.hpp

@@ -30,55 +30,8 @@ struct shared_authority {
    shared_vector<types::KeyPermissionWeight>     keys;
 };
 
-/**
- * @brief This class determines whether a set of signing keys are sufficient to satisfy an authority or not
- *
- * To determine whether an authority is satisfied or not, we first determine which keys have approved of a message, and
- * then determine whether that list of keys is sufficient to satisfy the authority. This class takes a list of keys and
- * provides the @ref satisfied method to determine whether that list of keys satisfies a provided authority.
- *
- * @tparam F A callable which takes a single argument of type @ref AccountPermission and returns the corresponding
- * authority
- */
-template<typename F>
-class AuthorityChecker {
-   F PermissionToAuthority;
-   flat_set<public_key_type> signingKeys;
-
-public:
-   AuthorityChecker(F PermissionToAuthority, const flat_set<public_key_type>& signingKeys)
-      : PermissionToAuthority(PermissionToAuthority), signingKeys(signingKeys) {}
-
-   bool satisfied(const types::AccountPermission& permission) const {
-      return satisfied(PermissionToAuthority(permission));
-   }
-   template<typename AuthorityType>
-   bool satisfied(const AuthorityType& authority) const {
-      UInt32 weight = 0;
-      for (const auto& kpw : authority.keys) {
-         if (signingKeys.count(kpw.key)) {
-            weight += kpw.weight;
-            if (weight >= authority.threshold)
-               return true;
-         }
-      }
-      for (const auto& apw : authority.accounts)
-//#warning TODO: Recursion limit? Yes: implement as producer-configurable parameter 
-         if (satisfied(apw.permission)) {
-            weight += apw.weight;
-            if (weight >= authority.threshold)
-               return true;
-         }
-      return false;
-   }
-};
-
-inline bool operator < ( const types::AccountPermission& a, const types::AccountPermission& b ) {
-   return std::tie( a.account, a.permission ) < std::tie( b.account, b.permission );
-}
-template<typename F>
-AuthorityChecker<F> MakeAuthorityChecker(F&& pta, const flat_set<public_key_type>& signingKeys) {
-   return AuthorityChecker<F>(std::forward<F>(pta), signingKeys);
+inline bool operator< (const types::AccountPermission& a, const types::AccountPermission& b) {
+   return std::tie(a.account, a.permission) < std::tie(b.account, b.permission);
 }
 
 /**

libraries/chain/include/eos/chain/authority_checker.hpp

+#pragma once
+
+#include <eos/chain/types.hpp>
+#include <eos/types/generated.hpp>
+
+#include <eos/utilities/parallel_markers.hpp>
+
+#include <fc/scoped_exit.hpp>
+
+#include <boost/range/algorithm/find.hpp>
+#include <boost/algorithm/cxx11/all_of.hpp>
+
+namespace eos { namespace chain {
+
+namespace detail {
+using MetaPermission = static_variant<types::KeyPermissionWeight, types::AccountPermissionWeight>;
+
+struct GetWeightVisitor {
+   using result_type = UInt32;
+
+   template<typename Permission>
+   UInt32 operator()(const Permission& permission) { return permission.weight; }
+};
+
+// Orders permissions descending by weight, and breaks ties with Key permissions being less than Account permissions
+struct MetaPermissionComparator {
+   bool operator()(const MetaPermission& a, const MetaPermission& b) const {
+      GetWeightVisitor scale;
+      if (a.visit(scale) > b.visit(scale)) return true;
+      return a.contains<types::KeyPermissionWeight>() && !b.contains<types::KeyPermissionWeight>();
+   }
+};
+
+using MetaPermissionSet = boost::container::flat_multiset<MetaPermission, MetaPermissionComparator>;
+}
+
+/**
+ * @brief This class determines whether a set of signing keys are sufficient to satisfy an authority or not
+ *
+ * To determine whether an authority is satisfied or not, we first determine which keys have approved of a message, and
+ * then determine whether that list of keys is sufficient to satisfy the authority. This class takes a list of keys and
+ * provides the @ref satisfied method to determine whether that list of keys satisfies a provided authority.
+ *
+ * @tparam F A callable which takes a single argument of type @ref AccountPermission and returns the corresponding
+ * authority
+ */
+template<typename F>
+class AuthorityChecker {
+   F PermissionToAuthority;
+   vector<public_key_type> signingKeys;
+   vector<bool> usedKeys;
+
+   struct WeightTallyVisitor {
+      using result_type = UInt32;
+
+      AuthorityChecker& checker;
+      UInt32 totalWeight = 0;
+
+      WeightTallyVisitor(AuthorityChecker& checker)
+         : checker(checker) {}
+
+      UInt32 operator()(const types::KeyPermissionWeight& permission) {
+         auto itr = boost::find(checker.signingKeys, permission.key);
+         if (itr != checker.signingKeys.end()) {
+            checker.usedKeys[itr - checker.signingKeys.begin()] = true;
+            totalWeight += permission.weight;
+         }
+         return totalWeight;
+      }
+      UInt32 operator()(const types::AccountPermissionWeight& permission) {
+         //TODO: Recursion limit? Yes: implement as producer-configurable parameter
+         if (checker.satisfied(permission.permission))
+            totalWeight += permission.weight;
+         return totalWeight;
+      }
+   };
+
+public:
+   AuthorityChecker(F PermissionToAuthority, const flat_set<public_key_type>& signingKeys)
+      : PermissionToAuthority(PermissionToAuthority),
+        signingKeys(signingKeys.begin(), signingKeys.end()),
+        usedKeys(signingKeys.size(), false)
+   {}
+
+   bool satisfied(const types::AccountPermission& permission) {
+      return satisfied(PermissionToAuthority(permission));
+   }
+   template<typename AuthorityType>
+   bool satisfied(const AuthorityType& authority) {
+      // Save the current used keys; if we do not satisfy this authority, the newly used keys aren't actually used
+      auto KeyReverter = fc::make_scoped_exit([this, keys = usedKeys] () mutable {
+         usedKeys = keys;
+      });
+
+      // Sort key permissions and account permissions together into a single set of MetaPermissions
+      detail::MetaPermissionSet permissions;
+      permissions.insert(authority.keys.begin(), authority.keys.end());
+      permissions.insert(authority.accounts.begin(), authority.accounts.end());
+
+      // Check all permissions, from highest weight to lowest, seeing if signingKeys satisfies them or not
+      WeightTallyVisitor visitor(*this);
+      for (const auto& permission : permissions)
+         // If we've got enough weight, to satisfy the authority, return!
+         if (permission.visit(visitor) >= authority.threshold) {
+            KeyReverter.cancel();
+            return true;
+         }
+      return false;
+   }
+
+   bool all_keys_used() const { return boost::algorithm::all_of_equal(usedKeys, true); }
+   flat_set<public_key_type> used_keys() const {
+      auto range = utilities::FilterDataByMarker(signingKeys, usedKeys, true);
+      return {range.begin(), range.end()};
+   }
+   flat_set<public_key_type> unused_keys() const {
+      auto range = utilities::FilterDataByMarker(signingKeys, usedKeys, false);
+      return {range.begin(), range.end()};
+   }
+};
+
+template<typename F>
+AuthorityChecker<F> MakeAuthorityChecker(F&& pta, const flat_set<public_key_type>& signingKeys) {
+   return AuthorityChecker<F>(std::forward<F>(pta), signingKeys);
+}
+
+}} // namespace eos::chain

tests/tests/misc_tests.cpp

 #include <eos/chain/BlockchainConfiguration.hpp>
-#include <eos/chain/authority.hpp>
+#include <eos/chain/authority_checker.hpp>
 
 #include <eos/utilities/key_conversion.hpp>
 #include <eos/utilities/rand.hpp>
@@ -88,13 +88,39 @@ BOOST_AUTO_TEST_CASE(authority_checker)
    Make_Key(c);
    auto& c = c_public_key;
 
-   auto GetNullAuthority = [](auto){return Authority();};
+   auto GetNullAuthority = [](auto){abort(); return Authority();};
 
    auto A = Complex_Authority(2, ((a,1))((b,1)),);
-   BOOST_CHECK(MakeAuthorityChecker(GetNullAuthority, {a, b}).satisfied(A));
-   BOOST_CHECK(MakeAuthorityChecker(GetNullAuthority, {a, b, c}).satisfied(A));
-   BOOST_CHECK(!MakeAuthorityChecker(GetNullAuthority, {a, c}).satisfied(A));
-   BOOST_CHECK(!MakeAuthorityChecker(GetNullAuthority, {b, c}).satisfied(A));
+   {
+      auto checker = MakeAuthorityChecker(GetNullAuthority, {a, b});
+      BOOST_CHECK(checker.satisfied(A));
+      BOOST_CHECK(checker.all_keys_used());
+      BOOST_CHECK_EQUAL(checker.used_keys().size(), 2);
+      BOOST_CHECK_EQUAL(checker.unused_keys().size(), 0);
+   }
+   {
+      auto checker = MakeAuthorityChecker(GetNullAuthority, {a, c});
+      BOOST_CHECK(!checker.satisfied(A));
+      BOOST_CHECK(!checker.all_keys_used());
+      BOOST_CHECK_EQUAL(checker.used_keys().size(), 0);
+      BOOST_CHECK_EQUAL(checker.unused_keys().size(), 2);
+   }
+   {
+      auto checker = MakeAuthorityChecker(GetNullAuthority, {a, b, c});
+      BOOST_CHECK(checker.satisfied(A));
+      BOOST_CHECK(!checker.all_keys_used());
+      BOOST_CHECK_EQUAL(checker.used_keys().size(), 2);
+      BOOST_CHECK_EQUAL(checker.used_keys().count(a), 1);
+      BOOST_CHECK_EQUAL(checker.used_keys().count(b), 1);
+      BOOST_CHECK_EQUAL(checker.unused_keys().size(), 1);
+      BOOST_CHECK_EQUAL(checker.unused_keys().count(c), 1);
+   }
+   {
+      auto checker = MakeAuthorityChecker(GetNullAuthority, {b, c});
+      BOOST_CHECK(!checker.satisfied(A));
+      BOOST_CHECK(!checker.all_keys_used());
+      BOOST_CHECK_EQUAL(checker.used_keys().size(), 0);
+   }
 
    A = Complex_Authority(3, ((a,1))((b,1))((c,1)),);
    BOOST_CHECK(MakeAuthorityChecker(GetNullAuthority, {c, b, a}).satisfied(A));
@@ -115,10 +141,44 @@ BOOST_AUTO_TEST_CASE(authority_checker)
    auto GetCAuthority = [c_public_key](auto){return Complex_Authority(1, ((c, 1)),);};
 
    A = Complex_Authority(2, ((a, 2))((b, 1)), (("hello", "world", 1)));
-   BOOST_CHECK(MakeAuthorityChecker(GetCAuthority, {a}).satisfied(A));
-   BOOST_CHECK(!MakeAuthorityChecker(GetCAuthority, {b}).satisfied(A));
-   BOOST_CHECK(!MakeAuthorityChecker(GetCAuthority, {c}).satisfied(A));
-   BOOST_CHECK(MakeAuthorityChecker(GetCAuthority, {b,c}).satisfied(A));
+   {
+      auto checker = MakeAuthorityChecker(GetCAuthority, {a});
+      BOOST_CHECK(checker.satisfied(A));
+      BOOST_CHECK(checker.all_keys_used());
+   }
+   {
+      auto checker = MakeAuthorityChecker(GetCAuthority, {b});
+      BOOST_CHECK(!checker.satisfied(A));
+      BOOST_CHECK_EQUAL(checker.used_keys().size(), 0);
+      BOOST_CHECK_EQUAL(checker.unused_keys().size(), 1);
+      BOOST_CHECK_EQUAL(checker.unused_keys().count(b), 1);
+   }
+   {
+      auto checker = MakeAuthorityChecker(GetCAuthority, {c});
+      BOOST_CHECK(!checker.satisfied(A));
+      BOOST_CHECK_EQUAL(checker.used_keys().size(), 0);
+      BOOST_CHECK_EQUAL(checker.unused_keys().size(), 1);
+      BOOST_CHECK_EQUAL(checker.unused_keys().count(c), 1);
+   }
+   {
+      auto checker = MakeAuthorityChecker(GetCAuthority, {b,c});
+      BOOST_CHECK(checker.satisfied(A));
+      BOOST_CHECK(checker.all_keys_used());
+      BOOST_CHECK_EQUAL(checker.used_keys().size(), 2);
+      BOOST_CHECK_EQUAL(checker.unused_keys().size(), 0);
+      BOOST_CHECK_EQUAL(checker.used_keys().count(b), 1);
+      BOOST_CHECK_EQUAL(checker.used_keys().count(c), 1);
+   }
+   {
+      auto checker = MakeAuthorityChecker(GetCAuthority, {b,c,a});
+      BOOST_CHECK(checker.satisfied(A));
+      BOOST_CHECK(!checker.all_keys_used());
+      BOOST_CHECK_EQUAL(checker.used_keys().size(), 1);
+      BOOST_CHECK_EQUAL(checker.used_keys().count(a), 1);
+      BOOST_CHECK_EQUAL(checker.unused_keys().size(), 2);
+      BOOST_CHECK_EQUAL(checker.unused_keys().count(b), 1);
+      BOOST_CHECK_EQUAL(checker.unused_keys().count(c), 1);
+   }
 
    A = Complex_Authority(2, ((a, 1))((b, 1)), (("hello", "world", 1)));
    BOOST_CHECK(!MakeAuthorityChecker(GetCAuthority, {a}).satisfied(A));
@@ -127,12 +187,60 @@ BOOST_AUTO_TEST_CASE(authority_checker)
    BOOST_CHECK(MakeAuthorityChecker(GetCAuthority, {a,b}).satisfied(A));
    BOOST_CHECK(MakeAuthorityChecker(GetCAuthority, {b,c}).satisfied(A));
    BOOST_CHECK(MakeAuthorityChecker(GetCAuthority, {a,c}).satisfied(A));
+   {
+      auto checker = MakeAuthorityChecker(GetCAuthority, {a,b,c});
+      BOOST_CHECK(checker.satisfied(A));
+      BOOST_CHECK(!checker.all_keys_used());
+      BOOST_CHECK_EQUAL(checker.used_keys().size(), 2);
+      BOOST_CHECK_EQUAL(checker.unused_keys().size(), 1);
+      BOOST_CHECK_EQUAL(checker.unused_keys().count(c), 1);
+   }
 
    A = Complex_Authority(2, ((a, 1))((b, 1)), (("hello", "world", 2)));
    BOOST_CHECK(MakeAuthorityChecker(GetCAuthority, {a,b}).satisfied(A));
    BOOST_CHECK(MakeAuthorityChecker(GetCAuthority, {c}).satisfied(A));
    BOOST_CHECK(!MakeAuthorityChecker(GetCAuthority, {a}).satisfied(A));
    BOOST_CHECK(!MakeAuthorityChecker(GetCAuthority, {b}).satisfied(A));
+   {
+      auto checker = MakeAuthorityChecker(GetCAuthority, {a,b,c});
+      BOOST_CHECK(checker.satisfied(A));
+      BOOST_CHECK(!checker.all_keys_used());
+      BOOST_CHECK_EQUAL(checker.used_keys().size(), 1);
+      BOOST_CHECK_EQUAL(checker.unused_keys().size(), 2);
+      BOOST_CHECK_EQUAL(checker.used_keys().count(c), 1);
+   }
+
+   Make_Key(d);
+   auto& d = d_public_key;
+   Make_Key(e);
+   auto& e = e_public_key;
+
+   auto GetAuthority = [d_public_key, e] (const types::AccountPermission& perm) {
+      if (perm.account == "top")
+         return Complex_Authority(2, ((d, 1)), (("bottom", "bottom", 1)));
+      return Key_Authority(e);
+   };
+
+   A = Complex_Authority(5, ((a, 2))((b, 2))((c, 2)), (("top", "top", 5)));
+   {
+      auto checker = MakeAuthorityChecker(GetAuthority, {a,b,c,d,e});
+      BOOST_CHECK(checker.satisfied(A));
+      BOOST_CHECK(!checker.all_keys_used());
+      BOOST_CHECK_EQUAL(checker.used_keys().size(), 2);
+      BOOST_CHECK_EQUAL(checker.unused_keys().size(), 3);
+      BOOST_CHECK_EQUAL(checker.used_keys().count(d), 1);
+      BOOST_CHECK_EQUAL(checker.used_keys().count(e), 1);
+   }
+   {
+      auto checker = MakeAuthorityChecker(GetAuthority, {a,b,c,e});
+      BOOST_CHECK(checker.satisfied(A));
+      BOOST_CHECK(!checker.all_keys_used());
+      BOOST_CHECK_EQUAL(checker.used_keys().size(), 3);
+      BOOST_CHECK_EQUAL(checker.unused_keys().size(), 1);
+      BOOST_CHECK_EQUAL(checker.used_keys().count(a), 1);
+      BOOST_CHECK_EQUAL(checker.used_keys().count(b), 1);
+      BOOST_CHECK_EQUAL(checker.used_keys().count(c), 1);
+   }
 } FC_LOG_AND_RETHROW() }
 
 /// Test creating the wallet

tests/tests/native_contract_tests.cpp

@@ -5,6 +5,7 @@
 #include <eos/chain/permission_object.hpp>
 #include <eos/chain/permission_link_object.hpp>
 #include <eos/chain/key_value_object.hpp>
+#include <eos/chain/authority_checker.hpp>
 
 #include <eos/native_contract/producer_objects.hpp>