From 8c6e0b9e55cd59e500f58ba7289cebb0d70fc6a8 Mon Sep 17 00:00:00 2001 From: Daniel Larimer Date: Fri, 17 Jul 2015 00:41:43 -0400 Subject: [PATCH] Refactor Authority Checking transaction_evaluation_state no longer tracks authority validation. Authority validation is now compeltely independent of the database. --- libraries/chain/CMakeLists.txt | 1 - libraries/chain/db_block.cpp | 38 ------------------- .../include/graphene/chain/exceptions.hpp | 4 +- .../graphene/chain/protocol/transaction.hpp | 8 +++- .../chain/transaction_evaluation_state.hpp | 24 ------------ libraries/chain/proposal_object.cpp | 7 +++- libraries/chain/protocol/transaction.cpp | 21 +++++++--- tests/tests/authority_tests.cpp | 6 +-- 8 files changed, 34 insertions(+), 75 deletions(-) diff --git a/libraries/chain/CMakeLists.txt b/libraries/chain/CMakeLists.txt index 698b5089..3ad4183a 100644 --- a/libraries/chain/CMakeLists.txt +++ b/libraries/chain/CMakeLists.txt @@ -43,7 +43,6 @@ add_library( graphene_chain proposal_object.cpp vesting_balance_object.cpp - transaction_evaluation_state.cpp fork_database.cpp block_database.cpp diff --git a/libraries/chain/db_block.cpp b/libraries/chain/db_block.cpp index 68c0fa03..489897d3 100644 --- a/libraries/chain/db_block.cpp +++ b/libraries/chain/db_block.cpp @@ -215,22 +215,6 @@ processed_transaction database::push_proposal(const proposal_object& proposal) transaction_evaluation_state eval_state(this); eval_state._is_proposed_trx = true; - //Inject the approving authorities into the transaction eval state - /* - std::transform(proposal.required_active_approvals.begin(), - proposal.required_active_approvals.end(), - std::inserter(eval_state.approved_by, eval_state.approved_by.begin()), - []( account_id_type id ) { - return std::make_pair(id, authority::active); - }); - std::transform(proposal.required_owner_approvals.begin(), - proposal.required_owner_approvals.end(), - std::inserter(eval_state.approved_by, eval_state.approved_by.begin()), - []( account_id_type id ) { - return std::make_pair(id, authority::owner); - }); - */ - eval_state.operation_results.reserve(proposal.proposed_transaction.operations.size()); processed_transaction ptrx(proposal.proposed_transaction); eval_state._trx = &ptrx; @@ -489,17 +473,6 @@ 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( get_active, get_owner ); - - /* - eval_state._sigs.reserve(trx.signatures.size()); - - for( const auto& sig : trx.signatures ) - { - FC_ASSERT( eval_state._sigs.insert(std::make_pair(public_key_type(fc::ecc::public_key(sig, trx.digest())), - false)).second, - "Multiple signatures by same key detected" ); - } - */ } //Skip all manner of expiration and TaPoS checking if we're on block 1; It's impossible that the transaction is @@ -545,17 +518,6 @@ processed_transaction database::_apply_transaction(const signed_transaction& trx auto range = index.equal_range(GRAPHENE_TEMP_ACCOUNT); std::for_each(range.first, range.second, [](const account_balance_object& b) { FC_ASSERT(b.balance == 0); }); - //Make sure all signatures were needed to validate the transaction - /* - if( !(skip & (skip_transaction_signatures|skip_authority_check)) ) - { - for( const auto& item : eval_state._sigs ) - { - FC_ASSERT( item.second, "All signatures must be used", ("item",item) ); - } - } - */ - return ptrx; } FC_CAPTURE_AND_RETHROW( (trx) ) } diff --git a/libraries/chain/include/graphene/chain/exceptions.hpp b/libraries/chain/include/graphene/chain/exceptions.hpp index a75e3bba..05bde928 100644 --- a/libraries/chain/include/graphene/chain/exceptions.hpp +++ b/libraries/chain/include/graphene/chain/exceptions.hpp @@ -73,7 +73,9 @@ namespace graphene { namespace chain { FC_DECLARE_DERIVED_EXCEPTION( tx_missing_active_auth, graphene::chain::transaction_exception, 3030001, "missing required active authority" ) FC_DECLARE_DERIVED_EXCEPTION( tx_missing_owner_auth, graphene::chain::transaction_exception, 3030002, "missing required owner authority" ) FC_DECLARE_DERIVED_EXCEPTION( tx_missing_other_auth, graphene::chain::transaction_exception, 3030003, "missing required other authority" ) - //FC_DECLARE_DERIVED_EXCEPTION( tx_irrelevant_authority, graphene::chain::transaction_exception, 3030004, "irrelevant authority" ) + FC_DECLARE_DERIVED_EXCEPTION( tx_irrelevant_authority, graphene::chain::transaction_exception, 3030004, "irrelevant authority" ) + FC_DECLARE_DERIVED_EXCEPTION( invalid_committee_approval, graphene::chain::transaction_exception, 3030005, + "committee account cannot directly approve transaction" ) FC_DECLARE_DERIVED_EXCEPTION( invalid_pts_address, graphene::chain::utility_exception, 3060001, "invalid pts address" ) FC_DECLARE_DERIVED_EXCEPTION( insufficient_feeds, graphene::chain::chain_exception, 37006, "insufficient feeds" ) diff --git a/libraries/chain/include/graphene/chain/protocol/transaction.hpp b/libraries/chain/include/graphene/chain/protocol/transaction.hpp index 30e373d1..4ada2b34 100644 --- a/libraries/chain/include/graphene/chain/protocol/transaction.hpp +++ b/libraries/chain/include/graphene/chain/protocol/transaction.hpp @@ -122,11 +122,13 @@ namespace graphene { namespace chain { */ set get_required_signatures( const flat_set& available_keys, const std::function& get_active, - const std::function& get_owner + const std::function& get_owner, + uint32_t max_recursion = GRAPHENE_MAX_SIG_CHECK_DEPTH )const; void verify_authority( const std::function& get_active, - const std::function& get_owner )const; + const std::function& get_owner, + uint32_t max_recursion = GRAPHENE_MAX_SIG_CHECK_DEPTH )const; flat_set get_signature_keys()const; @@ -139,6 +141,8 @@ namespace graphene { namespace chain { void verify_authority( const vector& ops, const flat_set& sigs, const std::function& get_active, const std::function& get_owner, + uint32_t max_recursion = GRAPHENE_MAX_SIG_CHECK_DEPTH, + bool allow_committe = false, const flat_set& active_aprovals = flat_set(), const flat_set& owner_approvals = flat_set()); diff --git a/libraries/chain/include/graphene/chain/transaction_evaluation_state.hpp b/libraries/chain/include/graphene/chain/transaction_evaluation_state.hpp index 027beb9a..49a69a98 100644 --- a/libraries/chain/include/graphene/chain/transaction_evaluation_state.hpp +++ b/libraries/chain/include/graphene/chain/transaction_evaluation_state.hpp @@ -32,34 +32,10 @@ namespace graphene { namespace chain { transaction_evaluation_state( database* db = nullptr ) :_db(db){} - /* - bool check_authority(const account_object&, - authority::classification auth_class = authority::active, - int depth = 0); - - bool check_authority(const authority&, - authority::classification auth_class = authority::active, - int depth = 0); - */ database& db()const { assert( _db ); return *_db; } - - /* - bool signed_by(const public_key_type& k); - bool signed_by(const address& k); - - /// cached approval (accounts and keys) - flat_set> approved_by; - */ - - /// Used to look up new objects using transaction relative IDs vector operation_results; - /** - * When an address is referenced via check authority it is flagged as being used, all addresses must be - * flagged as being used or the transaction will fail. - */ - // flat_map _sigs; const signed_transaction* _trx = nullptr; database* _db = nullptr; bool _is_proposed_trx = false; diff --git a/libraries/chain/proposal_object.cpp b/libraries/chain/proposal_object.cpp index d67b4f8a..f40d22e6 100644 --- a/libraries/chain/proposal_object.cpp +++ b/libraries/chain/proposal_object.cpp @@ -30,10 +30,15 @@ bool proposal_object::is_authorized_to_execute(database& db) const available_key_approvals, [&]( account_id_type id ){ return &id(db).active; }, [&]( account_id_type id ){ return &id(db).owner; }, + GRAPHENE_MAX_SIG_CHECK_DEPTH, // TODO make chain param + true, /* allow committeee */ available_active_approvals, available_owner_approvals ); - } catch ( const fc::exception& e ) + } + catch ( const fc::exception& e ) { + //idump((available_active_approvals)); + //wlog((e.to_detail_string())); return false; } return true; diff --git a/libraries/chain/protocol/transaction.cpp b/libraries/chain/protocol/transaction.cpp index 253cbee6..d3e662db 100644 --- a/libraries/chain/protocol/transaction.cpp +++ b/libraries/chain/protocol/transaction.cpp @@ -103,7 +103,7 @@ struct sign_state bool check_authority( account_id_type id ) { - if( id == GRAPHENE_TEMP_ACCOUNT ) return true; + if( approved_by.find(id) != approved_by.end() ) return true; return check_authority( get_active(id) ); } @@ -129,7 +129,7 @@ struct sign_state { if( approved_by.find(a.first) == approved_by.end() ) { - if( depth == GRAPHENE_MAX_SIG_CHECK_DEPTH ) + if( depth == max_recursion ) return false; if( check_authority( get_active( a.first ), depth+1 ) ) { @@ -176,12 +176,15 @@ struct sign_state flat_map provided_signatures; flat_set approved_by; + uint32_t max_recursion = GRAPHENE_MAX_SIG_CHECK_DEPTH; }; void verify_authority( const vector& ops, const flat_set& sigs, const std::function& get_active, const std::function& get_owner, + uint32_t max_recursion_depth, + bool allow_committe, const flat_set& active_aprovals, const flat_set& owner_approvals ) { try { @@ -192,7 +195,12 @@ void verify_authority( const vector& ops, const flat_set signed_transaction::get_signature_keys()const set signed_transaction::get_required_signatures( const flat_set& available_keys, const std::function& get_active, - const std::function& get_owner )const + const std::function& get_owner, + uint32_t max_recursion_depth )const { flat_set required_active; flat_set required_owner; @@ -238,6 +247,7 @@ set signed_transaction::get_required_signatures( const flat_set sign_state s(get_signature_keys(),get_active,available_keys); + s.max_recursion = max_recursion_depth; for( const auto& auth : other ) s.check_authority(&auth); @@ -259,9 +269,10 @@ set signed_transaction::get_required_signatures( const flat_set void signed_transaction::verify_authority( const std::function& get_active, - const std::function& get_owner )const + const std::function& get_owner, + uint32_t max_recursion )const { try { - graphene::chain::verify_authority( operations, get_signature_keys(), get_active, get_owner ); + graphene::chain::verify_authority( operations, get_signature_keys(), get_active, get_owner, max_recursion ); } FC_CAPTURE_AND_RETHROW( (*this) ) } } } // graphene::chain diff --git a/tests/tests/authority_tests.cpp b/tests/tests/authority_tests.cpp index 8b0c6cbf..dfabf40a 100644 --- a/tests/tests/authority_tests.cpp +++ b/tests/tests/authority_tests.cpp @@ -340,7 +340,6 @@ BOOST_AUTO_TEST_CASE( proposed_single_account ) trx.set_expiration( db.head_block_time() + fc::seconds( 3 * db.get_global_properties().parameters.block_interval )); trx.set_reference_block( db.head_block_id() ); - //idump((moneyman)); trx.sign( init_account_priv_key ); const proposal_object& proposal = db.get(PUSH_TX( db, trx ).operation_results.front().get()); @@ -353,6 +352,7 @@ BOOST_AUTO_TEST_CASE( proposed_single_account ) proposal_update_operation pup; pup.proposal = proposal.id; pup.fee_paying_account = nathan.id; + BOOST_TEST_MESSAGE( "Updating the proposal to have nathan's authority" ); pup.active_approvals_to_add.insert(nathan.id); trx.operations = {pup}; @@ -399,13 +399,13 @@ BOOST_AUTO_TEST_CASE( committee_authority ) p.parameters.committee_proposal_review_period = fc::days(1).to_seconds(); }); - BOOST_TEST_MESSAGE( "transfering 100000 CORE to nathan, signing with committee key" ); + BOOST_TEST_MESSAGE( "transfering 100000 CORE to nathan, signing with committee key should fail because this requires it to be part of a proposal" ); transfer_operation top; top.to = nathan.id; top.amount = asset(100000); trx.operations.push_back(top); sign(trx, committee_key); - GRAPHENE_CHECK_THROW(PUSH_TX( db, trx ), fc::exception); + GRAPHENE_CHECK_THROW(PUSH_TX( db, trx ), graphene::chain::invalid_committee_approval ); auto sign = [&] { trx.signatures.clear(); trx.sign(nathan_key); };