From cf4ee1f96e4d8a428aaac2f55958cbad25e4611f Mon Sep 17 00:00:00 2001 From: satyakoneru Date: Wed, 28 Aug 2019 11:13:01 +0000 Subject: [PATCH] GRPH-60-Recursion_nesting_guard --- libraries/chain/db_block.cpp | 21 +++ libraries/chain/hardfork.d/1003.hf | 4 + .../chain/include/graphene/chain/database.hpp | 2 + libraries/chain/proposal_evaluator.cpp | 23 ++- libraries/chain/protocol/proposal.cpp | 5 +- tests/tests/authority_tests.cpp | 134 +++++++++++++++++- 6 files changed, 185 insertions(+), 4 deletions(-) create mode 100644 libraries/chain/hardfork.d/1003.hf diff --git a/libraries/chain/db_block.cpp b/libraries/chain/db_block.cpp index 9b2c7f36..631bacf8 100644 --- a/libraries/chain/db_block.cpp +++ b/libraries/chain/db_block.cpp @@ -325,6 +325,24 @@ processed_transaction database::validate_transaction( const signed_transaction& return _apply_transaction( trx ); } +class push_proposal_nesting_guard { +public: + push_proposal_nesting_guard( uint32_t& nesting_counter, const database& db ) + : orig_value(nesting_counter), counter(nesting_counter) + { + FC_ASSERT( counter < db.get_global_properties().active_witnesses.size() * 2, "Max proposal nesting depth exceeded!" ); + counter++; + } + ~push_proposal_nesting_guard() + { + if( --counter != orig_value ) + elog( "Unexpected proposal nesting count value: ${n} != ${o}", ("n",counter)("o",orig_value) ); + } +private: + const uint32_t orig_value; + uint32_t& counter; +}; + processed_transaction database::push_proposal(const proposal_object& proposal) { try { transaction_evaluation_state eval_state(this); @@ -336,6 +354,9 @@ processed_transaction database::push_proposal(const proposal_object& proposal) size_t old_applied_ops_size = _applied_ops.size(); try { + push_proposal_nesting_guard guard( _push_proposal_nesting_depth, *this ); + if( _undo_db.size() >= _undo_db.max_size() ) + _undo_db.set_max_size( _undo_db.size() + 1 ); auto session = _undo_db.start_undo_session(true); for( auto& op : proposal.proposed_transaction.operations ) eval_state.operation_results.emplace_back(apply_operation(eval_state, op)); diff --git a/libraries/chain/hardfork.d/1003.hf b/libraries/chain/hardfork.d/1003.hf new file mode 100644 index 00000000..6da70892 --- /dev/null +++ b/libraries/chain/hardfork.d/1003.hf @@ -0,0 +1,4 @@ +// Approve Proposal Enabling +#ifndef HARDFORK_1003_TIME +#define HARDFORK_1003_TIME (fc::time_point_sec( 1566988401 )) +#endif diff --git a/libraries/chain/include/graphene/chain/database.hpp b/libraries/chain/include/graphene/chain/database.hpp index 0c6dcb0f..5feef35b 100644 --- a/libraries/chain/include/graphene/chain/database.hpp +++ b/libraries/chain/include/graphene/chain/database.hpp @@ -558,6 +558,8 @@ namespace graphene { namespace chain { node_property_object _node_property_object; fc::hash_ctr_rng _random_number_generator; bool _slow_replays = false; + // Counts nested proposal updates + uint32_t _push_proposal_nesting_depth = 0; }; namespace detail diff --git a/libraries/chain/proposal_evaluator.cpp b/libraries/chain/proposal_evaluator.cpp index a6640ea4..84b9f111 100644 --- a/libraries/chain/proposal_evaluator.cpp +++ b/libraries/chain/proposal_evaluator.cpp @@ -142,12 +142,33 @@ struct proposal_operation_hardfork_visitor } }; +struct hardfork_visitor_1003 // non-recursive proposal visitor +{ + typedef void result_type; + + template + void operator()(const T &v) const {} + + void operator()(const proposal_update_operation &v) const { + FC_ASSERT(false, "Not allowed until hardfork 1003"); + } +}; + + void_result proposal_create_evaluator::do_evaluate(const proposal_create_operation& o) { try { const database& d = db(); - proposal_operation_hardfork_visitor vtor( d.head_block_time() ); + // Calling the proposal hardfork visitor + const fc::time_point_sec block_time = d.head_block_time(); + proposal_operation_hardfork_visitor vtor( block_time ); vtor( o ); + if( block_time < HARDFORK_1003_TIME ) + { // cannot be removed after hf, unfortunately + hardfork_visitor_1003 hf1003; + for (const op_wrapper &op : o.proposed_ops) + op.op.visit( hf1003 ); + } const auto& global_parameters = d.get_global_properties().parameters; diff --git a/libraries/chain/protocol/proposal.cpp b/libraries/chain/protocol/proposal.cpp index 069824af..fd0af9b5 100644 --- a/libraries/chain/protocol/proposal.cpp +++ b/libraries/chain/protocol/proposal.cpp @@ -88,8 +88,9 @@ void proposal_update_operation::get_required_authorities( vector& o ) for( const auto& k : key_approvals_to_remove ) auth.key_auths[k] = 1; auth.weight_threshold = auth.key_auths.size(); - - o.emplace_back( std::move(auth) ); + + if( auth.weight_threshold > 0 ) + o.emplace_back( std::move(auth) ); } void proposal_update_operation::get_required_active_authorities( flat_set& a )const diff --git a/tests/tests/authority_tests.cpp b/tests/tests/authority_tests.cpp index f5efbb9d..1180955c 100644 --- a/tests/tests/authority_tests.cpp +++ b/tests/tests/authority_tests.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include @@ -897,7 +898,6 @@ BOOST_FIXTURE_TEST_CASE( max_authority_membership, database_fixture ) }); transaction tx; - processed_transaction ptx; private_key_type committee_key = init_account_priv_key; // Sam is the creator of accounts @@ -1316,4 +1316,136 @@ BOOST_FIXTURE_TEST_CASE( nonminimal_sig_test, database_fixture ) } } +BOOST_AUTO_TEST_CASE( nested_execution ) +{ try { + ACTORS( (alice)(bob) ); + fund( alice ); + + generate_blocks( HARDFORK_1003_TIME + fc::hours(1) ); + set_expiration( db, trx ); + + const auto& gpo = db.get_global_properties(); + + proposal_create_operation pco; + pco.expiration_time = db.head_block_time() + fc::minutes(1); + pco.fee_paying_account = alice_id; + proposal_id_type inner; + { + transfer_operation top; + top.from = alice_id; + top.to = bob_id; + top.amount = asset( 10 ); + pco.proposed_ops.emplace_back( top ); + trx.operations.push_back( pco ); + inner = PUSH_TX( db, trx, ~0 ).operation_results.front().get(); + trx.clear(); + pco.proposed_ops.clear(); + } + + std::vector nested; + nested.push_back( inner ); + for( size_t i = 0; i < gpo.active_witnesses.size() * 2; i++ ) + { + proposal_update_operation pup; + pup.fee_paying_account = alice_id; + pup.proposal = nested.back(); + pup.active_approvals_to_add.insert( alice_id ); + pco.proposed_ops.emplace_back( pup ); + trx.operations.push_back( pco ); + nested.push_back( PUSH_TX( db, trx, ~0 ).operation_results.front().get() ); + trx.clear(); + pco.proposed_ops.clear(); + } + + proposal_update_operation pup; + pup.fee_paying_account = alice_id; + pup.proposal = nested.back(); + pup.active_approvals_to_add.insert( alice_id ); + trx.operations.push_back( pup ); + PUSH_TX( db, trx, ~0 ); + + for( size_t i = 1; i < nested.size(); i++ ) + BOOST_CHECK_THROW( db.get( nested[i] ), fc::assert_exception ); // executed successfully -> object removed + db.get( inner ); // wasn't executed -> object exists, doesn't throw +} FC_LOG_AND_RETHROW() } + + +BOOST_AUTO_TEST_CASE( enable_proposal_approval ) +{ try { + ACTORS( (alice)(bob) ); + fund( alice ); + generate_blocks( HARDFORK_1003_TIME - fc::hours(1) ); + set_expiration( db, trx ); + // Bob proposes that Alice transfer 500 CORE to himself + transfer_operation top; + top.from = alice_id; + top.to = bob_id; + top.amount = asset( 500 ); + proposal_create_operation pop; + pop.proposed_ops.emplace_back(top); + pop.fee_paying_account = bob_id; + pop.expiration_time = db.head_block_time() + fc::days(1); + trx.operations.push_back(pop); + sign( trx, bob_private_key ); + const proposal_id_type pid1 = PUSH_TX( db, trx ).operation_results[0].get(); + trx.clear(); + // Bob wants to propose that Alice confirm the first proposal + proposal_update_operation pup; + pup.fee_paying_account = alice_id; + pup.proposal = pid1; + pup.active_approvals_to_add.insert( alice_id ); + pop.proposed_ops.clear(); + pop.proposed_ops.emplace_back( pup ); + trx.operations.push_back(pop); + sign( trx, bob_private_key ); + // before HF_1003, Bob can't do that + BOOST_REQUIRE_THROW( PUSH_TX( db, trx ), fc::assert_exception ); + trx.signatures.clear(); + + { // Bob can create a proposal nesting the one containing the proposal_update + proposal_create_operation npop; + npop.proposed_ops.emplace_back(pop); + npop.fee_paying_account = bob_id; + npop.expiration_time = db.head_block_time() + fc::days(2); + signed_transaction ntx; + set_expiration( db, ntx ); + ntx.operations.push_back(npop); + sign( ntx, bob_private_key ); + const proposal_id_type pid1a = PUSH_TX( db, ntx ).operation_results[0].get(); + ntx.clear(); + + // But execution after confirming it fails + proposal_update_operation npup; + npup.fee_paying_account = bob_id; + npup.proposal = pid1a; + npup.active_approvals_to_add.insert( bob_id ); + ntx.operations.push_back(npup); + sign( ntx, bob_private_key ); + PUSH_TX( db, ntx ); + ntx.clear(); + + db.get( pid1a ); // still exists + } + + generate_blocks( HARDFORK_1003_TIME + fc::hours(1) ); + set_expiration( db, trx ); + sign( trx, bob_private_key ); + // after the HF it works + // after the HF the previously failed tx works too + const proposal_id_type pid2 = PUSH_TX( db, trx ).operation_results[0].get(); + trx.clear(); + + // For completeness, Alice confirms Bob's second proposal + pup.proposal = pid2; + trx.operations.push_back(pup); + sign( trx, alice_private_key ); + PUSH_TX( db, trx ); + trx.clear(); + // Execution of the second proposal should have confirmed the first, + // which should have been executed by now. + BOOST_CHECK_THROW( db.get(pid1), fc::assert_exception ); + BOOST_CHECK_THROW( db.get(pid2), fc::assert_exception ); + BOOST_CHECK_EQUAL( top.amount.amount.value, get_balance( bob_id, top.amount.asset_id ) ); +} FC_LOG_AND_RETHROW() } + BOOST_AUTO_TEST_SUITE_END()