From 9d54a5ce0bc3f8da33758d8d4b3b641e501f321a Mon Sep 17 00:00:00 2001 From: John Jones Date: Fri, 8 Jun 2018 08:26:30 -0500 Subject: [PATCH 1/4] Add fail_reason to proposal object --- .../graphene/chain/proposal_object.hpp | 4 +- libraries/chain/proposal_evaluator.cpp | 41 ++++++++++++------- libraries/chain/proposal_object.cpp | 9 +++- tests/tests/authority_tests.cpp | 5 +++ 4 files changed, 41 insertions(+), 18 deletions(-) diff --git a/libraries/chain/include/graphene/chain/proposal_object.hpp b/libraries/chain/include/graphene/chain/proposal_object.hpp index 155d00b2a3..8f7b6e3dee 100644 --- a/libraries/chain/include/graphene/chain/proposal_object.hpp +++ b/libraries/chain/include/graphene/chain/proposal_object.hpp @@ -52,8 +52,10 @@ class proposal_object : public abstract_object flat_set available_owner_approvals; flat_set available_key_approvals; account_id_type proposer; + std::string fail_reason; - bool is_authorized_to_execute(database& db)const; + bool is_authorized_to_execute(database& db) const; + bool is_authorized_to_execute(database& db, std::string& fail_reason) const; }; /** diff --git a/libraries/chain/proposal_evaluator.cpp b/libraries/chain/proposal_evaluator.cpp index 60e3b98b58..7a4d29eff2 100644 --- a/libraries/chain/proposal_evaluator.cpp +++ b/libraries/chain/proposal_evaluator.cpp @@ -103,6 +103,11 @@ object_id_type proposal_create_evaluator::do_apply(const proposal_create_operati std::set_difference(required_active.begin(), required_active.end(), proposal.required_owner_approvals.begin(), proposal.required_owner_approvals.end(), std::inserter(proposal.required_active_approvals, proposal.required_active_approvals.begin())); + if ( proposal.is_authorized_to_execute(d, proposal.fail_reason) + && proposal.review_period_time && proposal.fail_reason.empty() ) + { + proposal.fail_reason = "Awaiting expiration of review period"; + } }); return proposal.id; @@ -129,19 +134,7 @@ void_result proposal_update_evaluator::do_evaluate(const proposal_update_operati "", ("id", id)("available", _proposal->available_owner_approvals) ); } - /* All authority checks happen outside of evaluators - if( (d.get_node_properties().skip_flags & database::skip_authority_check) == 0 ) - { - for( const auto& id : o.key_approvals_to_add ) - { - FC_ASSERT( trx_state->signed_by(id) ); - } - for( const auto& id : o.key_approvals_to_remove ) - { - FC_ASSERT( trx_state->signed_by(id) ); - } - } - */ + // NOTE: All authority checks happen outside of evaluators return void_result(); } FC_CAPTURE_AND_RETHROW( (o) ) } @@ -150,10 +143,12 @@ void_result proposal_update_evaluator::do_apply(const proposal_update_operation& { try { database& d = db(); + bool execution_authorized = false; + // Potential optimization: if _executed_proposal is true, we can skip the modify step and make push_proposal skip // signature checks. This isn't done now because I just wrote all the proposals code, and I'm not yet 100% sure the // required approvals are sufficient to authorize the transaction. - d.modify(*_proposal, [&o](proposal_object& p) { + d.modify(*_proposal, [&o, &d, &execution_authorized](proposal_object& p) { p.available_active_approvals.insert(o.active_approvals_to_add.begin(), o.active_approvals_to_add.end()); p.available_owner_approvals.insert(o.owner_approvals_to_add.begin(), o.owner_approvals_to_add.end()); for( account_id_type id : o.active_approvals_to_remove ) @@ -164,6 +159,22 @@ void_result proposal_update_evaluator::do_apply(const proposal_update_operation& p.available_key_approvals.insert(id); for( const auto& id : o.key_approvals_to_remove ) p.available_key_approvals.erase(id); + try + { + // first check authorization + execution_authorized = p.is_authorized_to_execute(d, p.fail_reason); + // now check to see if there is a review period + if (p.review_period_time) + { + execution_authorized = false; + if(p.fail_reason.empty()) + p.fail_reason = "Awaiting expiration of review period"; + } + } + catch (fc::exception& ex) + { + p.fail_reason = ex.to_string(fc::log_level(fc::log_level::all)); + } }); // If the proposal has a review period, don't bother attempting to authorize/execute it. @@ -171,7 +182,7 @@ void_result proposal_update_evaluator::do_apply(const proposal_update_operation& if( _proposal->review_period_time ) return void_result(); - if( _proposal->is_authorized_to_execute(d) ) + if( execution_authorized ) { // All required approvals are satisfied. Execute! _executed_proposal = true; diff --git a/libraries/chain/proposal_object.cpp b/libraries/chain/proposal_object.cpp index 565964a51b..7a095f97a8 100644 --- a/libraries/chain/proposal_object.cpp +++ b/libraries/chain/proposal_object.cpp @@ -28,6 +28,12 @@ namespace graphene { namespace chain { bool proposal_object::is_authorized_to_execute(database& db) const +{ + std::string reason; + return is_authorized_to_execute(db, reason); +} + +bool proposal_object::is_authorized_to_execute( database& db, std::string& fail_reason ) const { transaction_evaluation_state dry_run_eval(&db); @@ -43,8 +49,7 @@ bool proposal_object::is_authorized_to_execute(database& db) const } catch ( const fc::exception& e ) { - //idump((available_active_approvals)); - //wlog((e.to_detail_string())); + fail_reason = e.to_string(fc::log_level(fc::log_level::all)); return false; } return true; diff --git a/tests/tests/authority_tests.cpp b/tests/tests/authority_tests.cpp index 6701ecbdda..5bce227d61 100644 --- a/tests/tests/authority_tests.cpp +++ b/tests/tests/authority_tests.cpp @@ -447,6 +447,7 @@ BOOST_AUTO_TEST_CASE( committee_authority ) BOOST_TEST_MESSAGE( "Checking that the proposal is not authorized to execute" ); BOOST_REQUIRE(!db.get(prop.id).is_authorized_to_execute(db)); + BOOST_CHECK_EQUAL( db.get(prop.id).fail_reason, "missing required active authority: Missing Active Authority 1.2.0"); trx.operations.clear(); trx.signatures.clear(); proposal_update_operation uop; @@ -470,6 +471,8 @@ BOOST_AUTO_TEST_CASE( committee_authority ) trx.signatures.clear(); generate_blocks(*prop.review_period_time); + // check the latest proposal object + BOOST_CHECK_EQUAL(db.get(prop.id).fail_reason, "missing required active authority: Missing Active Authority 1.2.0"); uop.key_approvals_to_add.clear(); uop.key_approvals_to_add.insert(committee_key.get_public_key()); // was 7 trx.operations.back() = uop; @@ -479,6 +482,8 @@ BOOST_AUTO_TEST_CASE( committee_authority ) generate_blocks(prop.expiration_time); BOOST_CHECK_EQUAL(get_balance(nathan, asset_id_type()(db)), 100000); + // proposal deleted + BOOST_CHECK_THROW( db.get(prop.id), fc::exception ); } FC_LOG_AND_RETHROW() } BOOST_FIXTURE_TEST_CASE( fired_committee_members, database_fixture ) From 6df2f4b91b34a96fb6c5104ca69b6132e47149f5 Mon Sep 17 00:00:00 2001 From: John Jones Date: Fri, 8 Jun 2018 08:54:55 -0500 Subject: [PATCH 2/4] Fixed compiler warnings signed/unsigned --- tests/tests/authority_tests.cpp | 64 ++++++++++++++++----------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/tests/tests/authority_tests.cpp b/tests/tests/authority_tests.cpp index 5bce227d61..cc0a4c8f27 100644 --- a/tests/tests/authority_tests.cpp +++ b/tests/tests/authority_tests.cpp @@ -59,7 +59,7 @@ BOOST_AUTO_TEST_CASE( simple_single_signature ) sign(trx, nathan_key); PUSH_TX( db, trx, database::skip_transaction_dupe_check ); - BOOST_CHECK_EQUAL(get_balance(nathan, core), old_balance - 500); + BOOST_CHECK_EQUAL(get_balance(nathan, core), static_cast(old_balance - 500)); } catch (fc::exception& e) { edump((e.to_detail_string())); throw; @@ -97,25 +97,25 @@ BOOST_AUTO_TEST_CASE( any_two_of_three ) GRAPHENE_CHECK_THROW(PUSH_TX( db, trx, database::skip_transaction_dupe_check ), fc::exception); sign(trx, nathan_key2); PUSH_TX( db, trx, database::skip_transaction_dupe_check ); - BOOST_CHECK_EQUAL(get_balance(nathan, core), old_balance - 500); + BOOST_CHECK_EQUAL(get_balance(nathan, core), static_cast(old_balance - 500)); trx.signatures.clear(); sign(trx, nathan_key2); sign(trx, nathan_key3); PUSH_TX( db, trx, database::skip_transaction_dupe_check ); - BOOST_CHECK_EQUAL(get_balance(nathan, core), old_balance - 1000); + BOOST_CHECK_EQUAL(get_balance(nathan, core), static_cast(old_balance - 1000)); trx.signatures.clear(); sign(trx, nathan_key1); sign(trx, nathan_key3); PUSH_TX( db, trx, database::skip_transaction_dupe_check ); - BOOST_CHECK_EQUAL(get_balance(nathan, core), old_balance - 1500); + BOOST_CHECK_EQUAL(get_balance(nathan, core), static_cast(old_balance - 1500)); trx.signatures.clear(); //sign(trx, fc::ecc::private_key::generate()); sign(trx,nathan_key3); GRAPHENE_CHECK_THROW(PUSH_TX( db, trx, database::skip_transaction_dupe_check ), fc::exception); - BOOST_CHECK_EQUAL(get_balance(nathan, core), old_balance - 1500); + BOOST_CHECK_EQUAL(get_balance(nathan, core), static_cast(old_balance - 1500)); } catch (fc::exception& e) { edump((e.to_detail_string())); throw; @@ -165,7 +165,7 @@ BOOST_AUTO_TEST_CASE( recursive_accounts ) BOOST_TEST_MESSAGE( "Attempting to transfer with parent1 and parent2 signature, should succeed" ); sign(trx,parent1_key); PUSH_TX( db, trx, database::skip_transaction_dupe_check ); - BOOST_CHECK_EQUAL(get_balance(child, core), old_balance - 500); + BOOST_CHECK_EQUAL(get_balance(child, core), static_cast(old_balance - 500)); trx.operations.clear(); trx.signatures.clear(); @@ -180,7 +180,7 @@ BOOST_AUTO_TEST_CASE( recursive_accounts ) sign(trx,parent1_key); sign(trx,parent2_key); PUSH_TX( db, trx, database::skip_transaction_dupe_check ); - BOOST_REQUIRE_EQUAL(child.active.num_auths(), 3); + BOOST_REQUIRE_EQUAL(child.active.num_auths(), 3u); trx.operations.clear(); trx.signatures.clear(); } @@ -203,13 +203,13 @@ BOOST_AUTO_TEST_CASE( recursive_accounts ) BOOST_TEST_MESSAGE( "Attempting transfer both parents, should succeed" ); sign(trx, parent1_key); PUSH_TX( db, trx, database::skip_transaction_dupe_check ); - BOOST_CHECK_EQUAL(get_balance(child, core), old_balance - 1000); + BOOST_CHECK_EQUAL(get_balance(child, core), static_cast(old_balance - 1000)); trx.signatures.clear(); BOOST_TEST_MESSAGE( "Attempting transfer with just child key, should succeed" ); sign(trx, child_key); PUSH_TX( db, trx, database::skip_transaction_dupe_check ); - BOOST_CHECK_EQUAL(get_balance(child, core), old_balance - 1500); + BOOST_CHECK_EQUAL(get_balance(child, core), static_cast(old_balance - 1500)); trx.operations.clear(); trx.signatures.clear(); @@ -242,7 +242,7 @@ BOOST_AUTO_TEST_CASE( recursive_accounts ) BOOST_TEST_MESSAGE( "Attempt to transfer using parent2_key and grandparent_key" ); PUSH_TX( db, trx, database::skip_transaction_dupe_check ); - BOOST_CHECK_EQUAL(get_balance(child, core), old_balance - 2000); + BOOST_CHECK_EQUAL(get_balance(child, core), static_cast(old_balance - 2000)); trx.clear(); BOOST_TEST_MESSAGE( "Update grandparent account authority to be committee account" ); @@ -268,7 +268,7 @@ BOOST_AUTO_TEST_CASE( recursive_accounts ) trx.signatures.clear(); sign(trx, child_key); PUSH_TX( db, trx, database::skip_transaction_dupe_check ); - BOOST_CHECK_EQUAL(get_balance(child, core), old_balance - 2500); + BOOST_CHECK_EQUAL(get_balance(child, core), static_cast(old_balance - 2500)); trx.operations.clear(); trx.signatures.clear(); @@ -329,17 +329,17 @@ BOOST_AUTO_TEST_CASE( proposed_single_account ) vector other; flat_set active_set, owner_set; operation_get_required_authorities(op,active_set,owner_set,other); - BOOST_CHECK_EQUAL(active_set.size(), 1); - BOOST_CHECK_EQUAL(owner_set.size(), 0); - BOOST_CHECK_EQUAL(other.size(), 0); + BOOST_CHECK_EQUAL(active_set.size(), 1lu); + BOOST_CHECK_EQUAL(owner_set.size(), 0lu); + BOOST_CHECK_EQUAL(other.size(), 0lu); BOOST_CHECK(*active_set.begin() == moneyman.get_id()); active_set.clear(); other.clear(); operation_get_required_authorities(op.proposed_ops.front().op,active_set,owner_set,other); - BOOST_CHECK_EQUAL(active_set.size(), 1); - BOOST_CHECK_EQUAL(owner_set.size(), 0); - BOOST_CHECK_EQUAL(other.size(), 0); + BOOST_CHECK_EQUAL(active_set.size(), 1lu); + BOOST_CHECK_EQUAL(owner_set.size(), 0lu); + BOOST_CHECK_EQUAL(other.size(), 0lu); BOOST_CHECK(*active_set.begin() == nathan.id); } @@ -349,10 +349,10 @@ BOOST_AUTO_TEST_CASE( proposed_single_account ) sign( trx, init_account_priv_key ); const proposal_object& proposal = db.get(PUSH_TX( db, trx ).operation_results.front().get()); - BOOST_CHECK_EQUAL(proposal.required_active_approvals.size(), 1); - BOOST_CHECK_EQUAL(proposal.available_active_approvals.size(), 0); - BOOST_CHECK_EQUAL(proposal.required_owner_approvals.size(), 0); - BOOST_CHECK_EQUAL(proposal.available_owner_approvals.size(), 0); + BOOST_CHECK_EQUAL(proposal.required_active_approvals.size(), 1lu); + BOOST_CHECK_EQUAL(proposal.available_active_approvals.size(), 0lu); + BOOST_CHECK_EQUAL(proposal.required_owner_approvals.size(), 0lu); + BOOST_CHECK_EQUAL(proposal.available_owner_approvals.size(), 0lu); BOOST_CHECK(*proposal.required_active_approvals.begin() == nathan.id); proposal_update_operation pup; @@ -698,7 +698,7 @@ BOOST_FIXTURE_TEST_CASE( proposal_delete, database_fixture ) PUSH_TX( db, trx ); trx.clear(); BOOST_CHECK(!prop.is_authorized_to_execute(db)); - BOOST_CHECK_EQUAL(prop.available_active_approvals.size(), 1); + BOOST_CHECK_EQUAL(prop.available_active_approvals.size(), 1lu); std::swap(uop.active_approvals_to_add, uop.active_approvals_to_remove); trx.operations.push_back(uop); @@ -706,7 +706,7 @@ BOOST_FIXTURE_TEST_CASE( proposal_delete, database_fixture ) PUSH_TX( db, trx ); trx.clear(); BOOST_CHECK(!prop.is_authorized_to_execute(db)); - BOOST_CHECK_EQUAL(prop.available_active_approvals.size(), 0); + BOOST_CHECK_EQUAL(prop.available_active_approvals.size(), 0lu); } { @@ -760,8 +760,8 @@ BOOST_FIXTURE_TEST_CASE( proposal_owner_authority_delete, database_fixture ) } const proposal_object& prop = *db.get_index_type().indices().begin(); - BOOST_CHECK_EQUAL(prop.required_active_approvals.size(), 1); - BOOST_CHECK_EQUAL(prop.required_owner_approvals.size(), 1); + BOOST_CHECK_EQUAL(prop.required_active_approvals.size(), 1lu); + BOOST_CHECK_EQUAL(prop.required_owner_approvals.size(), 1lu); BOOST_CHECK(!prop.is_authorized_to_execute(db)); { @@ -774,7 +774,7 @@ BOOST_FIXTURE_TEST_CASE( proposal_owner_authority_delete, database_fixture ) PUSH_TX( db, trx ); trx.clear(); BOOST_CHECK(!prop.is_authorized_to_execute(db)); - BOOST_CHECK_EQUAL(prop.available_owner_approvals.size(), 1); + BOOST_CHECK_EQUAL(prop.available_owner_approvals.size(), 1lu); std::swap(uop.owner_approvals_to_add, uop.owner_approvals_to_remove); trx.operations.push_back(uop); @@ -782,7 +782,7 @@ BOOST_FIXTURE_TEST_CASE( proposal_owner_authority_delete, database_fixture ) PUSH_TX( db, trx ); trx.clear(); BOOST_CHECK(!prop.is_authorized_to_execute(db)); - BOOST_CHECK_EQUAL(prop.available_owner_approvals.size(), 0); + BOOST_CHECK_EQUAL(prop.available_owner_approvals.size(), 0lu); } { @@ -837,8 +837,8 @@ BOOST_FIXTURE_TEST_CASE( proposal_owner_authority_complete, database_fixture ) } const proposal_object& prop = *db.get_index_type().indices().begin(); - BOOST_CHECK_EQUAL(prop.required_active_approvals.size(), 1); - BOOST_CHECK_EQUAL(prop.required_owner_approvals.size(), 1); + BOOST_CHECK_EQUAL(prop.required_active_approvals.size(), 1lu); + BOOST_CHECK_EQUAL(prop.required_owner_approvals.size(), 1lu); BOOST_CHECK(!prop.is_authorized_to_execute(db)); { @@ -854,7 +854,7 @@ BOOST_FIXTURE_TEST_CASE( proposal_owner_authority_complete, database_fixture ) PUSH_TX( db, trx ); trx.clear(); BOOST_CHECK(!prop.is_authorized_to_execute(db)); - BOOST_CHECK_EQUAL(prop.available_key_approvals.size(), 1); + BOOST_CHECK_EQUAL(prop.available_key_approvals.size(), 1lu); std::swap(uop.key_approvals_to_add, uop.key_approvals_to_remove); trx.operations.push_back(uop); @@ -864,7 +864,7 @@ BOOST_FIXTURE_TEST_CASE( proposal_owner_authority_complete, database_fixture ) PUSH_TX( db, trx ); trx.clear(); BOOST_CHECK(!prop.is_authorized_to_execute(db)); - BOOST_CHECK_EQUAL(prop.available_key_approvals.size(), 0); + BOOST_CHECK_EQUAL(prop.available_key_approvals.size(), 0lu); std::swap(uop.key_approvals_to_add, uop.key_approvals_to_remove); trx.operations.push_back(uop); @@ -874,7 +874,7 @@ BOOST_FIXTURE_TEST_CASE( proposal_owner_authority_complete, database_fixture ) PUSH_TX( db, trx ); trx.clear(); BOOST_CHECK(!prop.is_authorized_to_execute(db)); - BOOST_CHECK_EQUAL(prop.available_key_approvals.size(), 1); + BOOST_CHECK_EQUAL(prop.available_key_approvals.size(), 1lu); uop.key_approvals_to_add.clear(); uop.owner_approvals_to_add.insert(nathan.get_id()); From b65b518d21c0a7ea921297973dc4f2a8cfbf3f98 Mon Sep 17 00:00:00 2001 From: John Jones Date: Mon, 11 Jun 2018 10:27:56 -0500 Subject: [PATCH 3/4] Re-implemented solution Rolled back changes due to misunderstanding issue, added code to solve issue. Added test to prove solution works. --- .../graphene/chain/proposal_object.hpp | 1 - libraries/chain/proposal_evaluator.cpp | 30 +---- libraries/chain/proposal_object.cpp | 8 -- tests/tests/authority_tests.cpp | 3 - tests/tests/proposal_tests.cpp | 116 ++++++++++++++++++ 5 files changed, 121 insertions(+), 37 deletions(-) create mode 100644 tests/tests/proposal_tests.cpp diff --git a/libraries/chain/include/graphene/chain/proposal_object.hpp b/libraries/chain/include/graphene/chain/proposal_object.hpp index 8f7b6e3dee..12fae3c89e 100644 --- a/libraries/chain/include/graphene/chain/proposal_object.hpp +++ b/libraries/chain/include/graphene/chain/proposal_object.hpp @@ -55,7 +55,6 @@ class proposal_object : public abstract_object std::string fail_reason; bool is_authorized_to_execute(database& db) const; - bool is_authorized_to_execute(database& db, std::string& fail_reason) const; }; /** diff --git a/libraries/chain/proposal_evaluator.cpp b/libraries/chain/proposal_evaluator.cpp index 7a4d29eff2..9f2661c898 100644 --- a/libraries/chain/proposal_evaluator.cpp +++ b/libraries/chain/proposal_evaluator.cpp @@ -103,11 +103,6 @@ object_id_type proposal_create_evaluator::do_apply(const proposal_create_operati std::set_difference(required_active.begin(), required_active.end(), proposal.required_owner_approvals.begin(), proposal.required_owner_approvals.end(), std::inserter(proposal.required_active_approvals, proposal.required_active_approvals.begin())); - if ( proposal.is_authorized_to_execute(d, proposal.fail_reason) - && proposal.review_period_time && proposal.fail_reason.empty() ) - { - proposal.fail_reason = "Awaiting expiration of review period"; - } }); return proposal.id; @@ -143,12 +138,10 @@ void_result proposal_update_evaluator::do_apply(const proposal_update_operation& { try { database& d = db(); - bool execution_authorized = false; - // Potential optimization: if _executed_proposal is true, we can skip the modify step and make push_proposal skip // signature checks. This isn't done now because I just wrote all the proposals code, and I'm not yet 100% sure the // required approvals are sufficient to authorize the transaction. - d.modify(*_proposal, [&o, &d, &execution_authorized](proposal_object& p) { + d.modify(*_proposal, [&o](proposal_object& p) { p.available_active_approvals.insert(o.active_approvals_to_add.begin(), o.active_approvals_to_add.end()); p.available_owner_approvals.insert(o.owner_approvals_to_add.begin(), o.owner_approvals_to_add.end()); for( account_id_type id : o.active_approvals_to_remove ) @@ -159,22 +152,6 @@ void_result proposal_update_evaluator::do_apply(const proposal_update_operation& p.available_key_approvals.insert(id); for( const auto& id : o.key_approvals_to_remove ) p.available_key_approvals.erase(id); - try - { - // first check authorization - execution_authorized = p.is_authorized_to_execute(d, p.fail_reason); - // now check to see if there is a review period - if (p.review_period_time) - { - execution_authorized = false; - if(p.fail_reason.empty()) - p.fail_reason = "Awaiting expiration of review period"; - } - } - catch (fc::exception& ex) - { - p.fail_reason = ex.to_string(fc::log_level(fc::log_level::all)); - } }); // If the proposal has a review period, don't bother attempting to authorize/execute it. @@ -182,13 +159,16 @@ void_result proposal_update_evaluator::do_apply(const proposal_update_operation& if( _proposal->review_period_time ) return void_result(); - if( execution_authorized ) + if( _proposal->is_authorized_to_execute(d) ) { // All required approvals are satisfied. Execute! _executed_proposal = true; try { _processed_transaction = d.push_proposal(*_proposal); } catch(fc::exception& e) { + d.modify(*_proposal, [&e](proposal_object& p) { + p.fail_reason = e.to_string(fc::log_level(fc::log_level::all)); + }); wlog("Proposed transaction ${id} failed to apply once approved with exception:\n----\n${reason}\n----\nWill try again when it expires.", ("id", o.proposal)("reason", e.to_detail_string())); _proposal_failed = true; diff --git a/libraries/chain/proposal_object.cpp b/libraries/chain/proposal_object.cpp index 7a095f97a8..343edce2b3 100644 --- a/libraries/chain/proposal_object.cpp +++ b/libraries/chain/proposal_object.cpp @@ -28,12 +28,6 @@ namespace graphene { namespace chain { bool proposal_object::is_authorized_to_execute(database& db) const -{ - std::string reason; - return is_authorized_to_execute(db, reason); -} - -bool proposal_object::is_authorized_to_execute( database& db, std::string& fail_reason ) const { transaction_evaluation_state dry_run_eval(&db); @@ -49,13 +43,11 @@ bool proposal_object::is_authorized_to_execute( database& db, std::string& fail_ } catch ( const fc::exception& e ) { - fail_reason = e.to_string(fc::log_level(fc::log_level::all)); return false; } return true; } - void required_approval_index::object_inserted( const object& obj ) { assert( dynamic_cast(&obj) ); diff --git a/tests/tests/authority_tests.cpp b/tests/tests/authority_tests.cpp index cc0a4c8f27..53586ce7b8 100644 --- a/tests/tests/authority_tests.cpp +++ b/tests/tests/authority_tests.cpp @@ -447,7 +447,6 @@ BOOST_AUTO_TEST_CASE( committee_authority ) BOOST_TEST_MESSAGE( "Checking that the proposal is not authorized to execute" ); BOOST_REQUIRE(!db.get(prop.id).is_authorized_to_execute(db)); - BOOST_CHECK_EQUAL( db.get(prop.id).fail_reason, "missing required active authority: Missing Active Authority 1.2.0"); trx.operations.clear(); trx.signatures.clear(); proposal_update_operation uop; @@ -471,8 +470,6 @@ BOOST_AUTO_TEST_CASE( committee_authority ) trx.signatures.clear(); generate_blocks(*prop.review_period_time); - // check the latest proposal object - BOOST_CHECK_EQUAL(db.get(prop.id).fail_reason, "missing required active authority: Missing Active Authority 1.2.0"); uop.key_approvals_to_add.clear(); uop.key_approvals_to_add.insert(committee_key.get_public_key()); // was 7 trx.operations.back() = uop; diff --git a/tests/tests/proposal_tests.cpp b/tests/tests/proposal_tests.cpp new file mode 100644 index 0000000000..321beecb3c --- /dev/null +++ b/tests/tests/proposal_tests.cpp @@ -0,0 +1,116 @@ +/* + * Copyright (c) 2018 Bitshares Foundation, and contributors. + * + * The MIT License + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#include + +#include +#include +#include + +#include + +#include "../common/database_fixture.hpp" + +using namespace graphene::chain; +using namespace graphene::chain::test; + +BOOST_FIXTURE_TEST_SUITE( proposal_tests, database_fixture ) + +BOOST_AUTO_TEST_CASE( proposal_failure ) +{ + try + { + // create an account for Bob + fc::ecc::private_key bob_key = fc::ecc::private_key::regenerate(fc::digest("bobkey")); + const account_object& bob = create_account( "bob", bob_key.get_public_key() ); + const account_id_type bob_id = bob.get_id(); + fund( bob, asset(1000000) ); + + // add an account for Alice + fc::ecc::private_key alice_key = fc::ecc::private_key::regenerate(fc::digest("alicekey")); + const account_object& alice = create_account( "alice", alice_key.get_public_key() ); + const account_id_type alice_id = alice.get_id(); + fund(alice, asset(1000000) ); + + set_expiration( db, trx ); + + // create proposal that will eventually fail due to lack of funds + transfer_operation top; + top.to = alice_id; + top.from = bob_id; + top.amount = asset(2000000); + proposal_create_operation pop; + pop.proposed_ops.push_back( { top } ); + pop.expiration_time = db.head_block_time() + fc::days(1); + pop.fee_paying_account = bob_id; + trx.operations.push_back( pop ); + trx.signatures.clear(); + sign( trx, bob_key ); + proposal_object prop; + + try + { + processed_transaction processed = PUSH_TX( db, trx ); + prop = db.get(processed.operation_results.front().get()); + } + catch( fc::exception& ex ) + { + BOOST_FAIL( "Unable to create proposal. Reason: " + ex.to_string(fc::log_level(fc::log_level::all))); + } + + trx.clear(); + generate_block(); + + // make sure proposal is still there + try { + db.get(prop.id); + } + catch (fc::exception& ex) + { + BOOST_FAIL( "proposal object no longer exists after 1 block" ); + } + + // add signature + proposal_update_operation up_op; + up_op.proposal = prop.id; + up_op.fee_paying_account = bob_id; + up_op.active_approvals_to_add.emplace( bob_id ); + trx.operations.push_back( up_op ); + sign( trx, bob_key ); + PUSH_TX( db, trx ); + trx.clear(); + + // check fail reason + const proposal_object& result = db.get(prop.id); + BOOST_CHECK_EQUAL(result.fail_reason, "Assert Exception: insufficient_balance: Insufficient Balance: " + "10 BTS, unable to transfer '20 BTS' from account 'bob' to 'alice' Unable to transfer 20 BTS " + "from bob to alice"); + } + catch (fc::exception& ex) + { + BOOST_FAIL(ex.to_string(fc::log_level(fc::log_level::all))); + } +} + +BOOST_AUTO_TEST_SUITE_END() From ef8cfbafdd1085aaede19470199c429ba87f6aff Mon Sep 17 00:00:00 2001 From: John Jones Date: Mon, 11 Jun 2018 16:27:07 -0500 Subject: [PATCH 4/4] Test cleanup --- libraries/chain/proposal_evaluator.cpp | 2 - tests/tests/authority_tests.cpp | 43 +++++++++ tests/tests/proposal_tests.cpp | 116 ------------------------- 3 files changed, 43 insertions(+), 118 deletions(-) delete mode 100644 tests/tests/proposal_tests.cpp diff --git a/libraries/chain/proposal_evaluator.cpp b/libraries/chain/proposal_evaluator.cpp index 9f2661c898..4b9d159855 100644 --- a/libraries/chain/proposal_evaluator.cpp +++ b/libraries/chain/proposal_evaluator.cpp @@ -129,8 +129,6 @@ void_result proposal_update_evaluator::do_evaluate(const proposal_update_operati "", ("id", id)("available", _proposal->available_owner_approvals) ); } - // NOTE: All authority checks happen outside of evaluators - return void_result(); } FC_CAPTURE_AND_RETHROW( (o) ) } diff --git a/tests/tests/authority_tests.cpp b/tests/tests/authority_tests.cpp index 53586ce7b8..e6f0ca30ca 100644 --- a/tests/tests/authority_tests.cpp +++ b/tests/tests/authority_tests.cpp @@ -389,6 +389,49 @@ BOOST_AUTO_TEST_CASE( proposed_single_account ) } } +BOOST_AUTO_TEST_CASE( proposal_failure ) +{ + try + { + ACTORS( (bob) (alice) ); + + fund( bob, asset(1000000) ); + fund( alice, asset(1000000) ); + + // create proposal that will eventually fail due to lack of funds + transfer_operation top; + top.to = alice_id; + top.from = bob_id; + top.amount = asset(2000000); + proposal_create_operation pop; + pop.proposed_ops.push_back( { top } ); + pop.expiration_time = db.head_block_time() + fc::days(1); + pop.fee_paying_account = bob_id; + trx.operations.push_back( pop ); + trx.signatures.clear(); + sign( trx, bob_private_key ); + processed_transaction processed = PUSH_TX( db, trx ); + proposal_object prop = db.get(processed.operation_results.front().get()); + trx.clear(); + generate_block(); + // add signature + proposal_update_operation up_op; + up_op.proposal = prop.id; + up_op.fee_paying_account = bob_id; + up_op.active_approvals_to_add.emplace( bob_id ); + trx.operations.push_back( up_op ); + sign( trx, bob_private_key ); + PUSH_TX( db, trx ); + trx.clear(); + + // check fail reason + const proposal_object& result = db.get(prop.id); + BOOST_CHECK(!result.fail_reason.empty()); + BOOST_CHECK_EQUAL( result.fail_reason.substr(0, 16), "Assert Exception"); + } + FC_LOG_AND_RETHROW() +} + /// Verify that committee authority cannot be invoked in a normal transaction BOOST_AUTO_TEST_CASE( committee_authority ) { try { diff --git a/tests/tests/proposal_tests.cpp b/tests/tests/proposal_tests.cpp deleted file mode 100644 index 321beecb3c..0000000000 --- a/tests/tests/proposal_tests.cpp +++ /dev/null @@ -1,116 +0,0 @@ -/* - * Copyright (c) 2018 Bitshares Foundation, and contributors. - * - * The MIT License - * - * Permission is hereby granted, free of charge, to any person obtaining a copy - * of this software and associated documentation files (the "Software"), to deal - * in the Software without restriction, including without limitation the rights - * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell - * copies of the Software, and to permit persons to whom the Software is - * furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in - * all copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, - * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN - * THE SOFTWARE. - */ - -#include - -#include -#include -#include - -#include - -#include "../common/database_fixture.hpp" - -using namespace graphene::chain; -using namespace graphene::chain::test; - -BOOST_FIXTURE_TEST_SUITE( proposal_tests, database_fixture ) - -BOOST_AUTO_TEST_CASE( proposal_failure ) -{ - try - { - // create an account for Bob - fc::ecc::private_key bob_key = fc::ecc::private_key::regenerate(fc::digest("bobkey")); - const account_object& bob = create_account( "bob", bob_key.get_public_key() ); - const account_id_type bob_id = bob.get_id(); - fund( bob, asset(1000000) ); - - // add an account for Alice - fc::ecc::private_key alice_key = fc::ecc::private_key::regenerate(fc::digest("alicekey")); - const account_object& alice = create_account( "alice", alice_key.get_public_key() ); - const account_id_type alice_id = alice.get_id(); - fund(alice, asset(1000000) ); - - set_expiration( db, trx ); - - // create proposal that will eventually fail due to lack of funds - transfer_operation top; - top.to = alice_id; - top.from = bob_id; - top.amount = asset(2000000); - proposal_create_operation pop; - pop.proposed_ops.push_back( { top } ); - pop.expiration_time = db.head_block_time() + fc::days(1); - pop.fee_paying_account = bob_id; - trx.operations.push_back( pop ); - trx.signatures.clear(); - sign( trx, bob_key ); - proposal_object prop; - - try - { - processed_transaction processed = PUSH_TX( db, trx ); - prop = db.get(processed.operation_results.front().get()); - } - catch( fc::exception& ex ) - { - BOOST_FAIL( "Unable to create proposal. Reason: " + ex.to_string(fc::log_level(fc::log_level::all))); - } - - trx.clear(); - generate_block(); - - // make sure proposal is still there - try { - db.get(prop.id); - } - catch (fc::exception& ex) - { - BOOST_FAIL( "proposal object no longer exists after 1 block" ); - } - - // add signature - proposal_update_operation up_op; - up_op.proposal = prop.id; - up_op.fee_paying_account = bob_id; - up_op.active_approvals_to_add.emplace( bob_id ); - trx.operations.push_back( up_op ); - sign( trx, bob_key ); - PUSH_TX( db, trx ); - trx.clear(); - - // check fail reason - const proposal_object& result = db.get(prop.id); - BOOST_CHECK_EQUAL(result.fail_reason, "Assert Exception: insufficient_balance: Insufficient Balance: " - "10 BTS, unable to transfer '20 BTS' from account 'bob' to 'alice' Unable to transfer 20 BTS " - "from bob to alice"); - } - catch (fc::exception& ex) - { - BOOST_FAIL(ex.to_string(fc::log_level(fc::log_level::all))); - } -} - -BOOST_AUTO_TEST_SUITE_END()