Progress on #7, upgrades to testing framework #151
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| #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 { | ||
|
There was a problem hiding this comment. Minor style note: I lean towards stateless visitors when I can in order to improve the colocation of code and intent. Below in I am not going to ding a review for it but, I would consider making the visitor simply produce the weight and then putting the aggregation of weights in the There was a problem hiding this comment. Hrm, on second blush there are a few hidden side effects in this visitor, like There was a problem hiding this comment. Yes, the whole AuthorityChecker class got pretty complex, but what it's doing is also pretty bloody complex... I didn't come up with a simpler approach, but perhaps one exists. I'm open to suggestions. In the meantime, I designed it to hide all the details inside. There was a problem hiding this comment.
Not sure I understand what you mean here. Do you mean not taking the early out you mentioned below? There was a problem hiding this comment. Ohh, you mean add up the weights in |
||
| 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; | ||
|
There was a problem hiding this comment. are we concerned that this early out may lead to problems when permissions are over-authorized? There was a problem hiding this comment. I don't see what affect breaking early has on that... but we definitely want to break early, some of these authorities can be quite expensive to check (many recursions down an account multisig tree, for instance) and we don't want to do that if we already know the answer. As to the case where two messages might let the user slip some extra sigs in... Can they, though? Authorities should be sorted, so we should always check the keys within them in the same order... That should prevent the situation you described, yes? There was a problem hiding this comment. the sorting certainly means that for any transaction this always goes the same way. My concern is the burden on wallets to match this algorithm in their signature collection and pruning. There should be a usable set of rules for this pruning but it seems excessive. A wallet must prune any signatures made irrelevant by this evaluation algorithm or it risks being rejected. Likely this means it will collect signatures and re-run this algorithm with the early out to determine which of the obtained signatures will be marked as "unused" in the case of over authorization. I understand that over-authorization has costs but the rules for a properly formed transaction are made more complex by this early out. Without it, we may exceed the authority threshold but we will mark all signatures that could be part of a message authorization as used. Relieving the burden of wallets but putting potentially more stress on the chain/processing. There was a problem hiding this comment. I guess the ordering requirement only means that we have consistent behavior across messages, not that we would detect that oversigning issue you described. I suppose to address that, I can make I'll write up a test case to exercise this issue, then fix it. There was a problem hiding this comment. I think we can make the over-authorization a "soft" consensus rather than a "hard" consensus. In other words, block producers may choose to include over-authorized transactions, but in practice will reject them. This will allow us to update / refine this without hard forking the chain. There was a problem hiding this comment. Then again, I can't figure out a way to make |
||
| } | ||
| 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| #pragma once | ||
|
|
||
| #include <boost/range/combine.hpp> | ||
| #include <boost/range/adaptor/filtered.hpp> | ||
| #include <boost/range/adaptor/transformed.hpp> | ||
|
|
||
| namespace eos { namespace utilities { | ||
|
|
||
| /** | ||
| * @brief Return values in DataRange corresponding to matching Markers | ||
| * | ||
| * Takes two parallel ranges, a Data range containing data values, and a Marker range containing markers on the | ||
| * corresponding data values. Returns a new Data range containing only the values corresponding to markers which match | ||
| * markerValue | ||
| * | ||
| * For example: | ||
| * @code{.cpp} | ||
| * vector<char> data = {'A', 'B', 'C'}; | ||
| * vector<bool> markers = {true, false, true}; | ||
| * auto markedData = FilterDataByMarker(data, markers, true); | ||
| * // markedData contains {'A', 'C'} | ||
| * @endcode | ||
| */ | ||
| template<typename DataRange, typename MarkerRange, typename Marker> | ||
| DataRange FilterDataByMarker(DataRange data, MarkerRange markers, const Marker& markerValue) { | ||
| auto RemoveMismatchedMarkers = boost::adaptors::filtered([&markerValue](const auto& tuple) { | ||
| return boost::get<0>(tuple) == markerValue; | ||
| }); | ||
| auto ToData = boost::adaptors::transformed([](const auto& tuple) { | ||
| return boost::get<1>(tuple); | ||
| }); | ||
|
|
||
| // Zip the ranges together, filter out data with markers that don't match, and return the data without the markers | ||
| auto range = boost::combine(markers, data) | RemoveMismatchedMarkers | ToData; | ||
| return {range.begin(), range.end()}; | ||
| } | ||
|
|
||
| }} // namespace eos::utilities | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From now on all such TODO's need to reference a specific github issue number and should be tagged as "bug" and assigned to a final release milestone (June 2018) or earlier. We should also not flood our builds with warnings like this, especially in header files.