From f29eaa92e2207dc702bb7f014688146f1ba96879 Mon Sep 17 00:00:00 2001 From: Nathan Hourt Date: Wed, 1 Jul 2015 13:53:04 -0400 Subject: [PATCH] Fix all outstanding unexpected test failures --- libraries/chain/db_block.cpp | 39 ++++++++++++++++++-------------- libraries/chain/transaction.cpp | 9 ++++---- tests/tests/block_tests.cpp | 20 ++++++++-------- tests/tests/operation_tests2.cpp | 33 +++++++++++++++------------ 4 files changed, 56 insertions(+), 45 deletions(-) diff --git a/libraries/chain/db_block.cpp b/libraries/chain/db_block.cpp index 9d9e3bdb..1d8708a6 100644 --- a/libraries/chain/db_block.cpp +++ b/libraries/chain/db_block.cpp @@ -425,17 +425,17 @@ void database::notify_changed_objects() changed_objects(changed_ids); } -processed_transaction database::apply_transaction( const signed_transaction& trx, uint32_t skip ) +processed_transaction database::apply_transaction(const signed_transaction& trx, uint32_t skip) { processed_transaction result; - with_skip_flags( skip, [&]() + with_skip_flags(skip, [&]() { - result = _apply_transaction( trx ); - } ); + result = _apply_transaction(trx); + }); return result; } -processed_transaction database::_apply_transaction( const signed_transaction& trx ) +processed_transaction database::_apply_transaction(const signed_transaction& trx) { try { uint32_t skip = get_node_properties().skip_flags; trx.validate(); @@ -450,13 +450,14 @@ processed_transaction database::_apply_transaction( const signed_transaction& tr //This check is used only if this transaction has an absolute expiration time. if( !(skip & skip_transaction_signatures) && trx.relative_expiration == 0 ) { - eval_state._sigs.reserve( trx.signatures.size() ); + 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" ) ; + 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" ); } - } //If we're skipping tapos check, but not dupe check, assume all transactions have maximum expiration time. @@ -522,8 +523,8 @@ processed_transaction database::_apply_transaction( const signed_transaction& tr ref_block_height = uint32_t( x0 ); } - const block_summary_object& tapos_block_summary - = static_cast( + const block_summary_object& tapos_block_summary = + static_cast( get_index() .get(block_summary_id_type(ref_block_height)) ); @@ -531,13 +532,16 @@ processed_transaction database::_apply_transaction( const signed_transaction& tr //This is the signature check for transactions with relative expiration. if( !(skip & skip_transaction_signatures) ) { - eval_state._sigs.reserve( trx.signatures.size() ); + 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(tapos_block_summary.block_id) )), - false)).second, "Multiple signatures by same key detected"); + FC_ASSERT(eval_state._sigs.insert(std::make_pair( + public_key_type( + fc::ecc::public_key(sig, + trx.digest(tapos_block_summary.block_id))), + false)).second, + "Multiple signatures by same key detected"); } } @@ -564,7 +568,7 @@ processed_transaction database::_apply_transaction( const signed_transaction& tr }); } - eval_state.operation_results.reserve( trx.operations.size() ); + eval_state.operation_results.reserve(trx.operations.size()); //Finally process the operations processed_transaction ptrx(trx); @@ -574,14 +578,15 @@ processed_transaction database::_apply_transaction( const signed_transaction& tr eval_state.operation_results.emplace_back(apply_operation(eval_state, op)); ++_current_op_in_trx; } - ptrx.operation_results = std::move( eval_state.operation_results ); + ptrx.operation_results = std::move(eval_state.operation_results); //Make sure the temp account has no non-zero balances const auto& index = get_index_type().indices().get(); 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); }); - if( !(skip & (skip_transaction_signatures|skip_authority_check)) ) + //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 ) { diff --git a/libraries/chain/transaction.cpp b/libraries/chain/transaction.cpp index 6ac810f1..ee1b3c10 100644 --- a/libraries/chain/transaction.cpp +++ b/libraries/chain/transaction.cpp @@ -36,7 +36,6 @@ digest_type processed_transaction::merkle_digest()const digest_type transaction::digest()const { //Only use this digest() for transactions with absolute expiration times. - if( relative_expiration != 0 ) edump((*this)); assert(relative_expiration == 0); digest_type::encoder enc; fc::raw::pack( enc, *this ); @@ -60,15 +59,15 @@ graphene::chain::transaction_id_type graphene::chain::transaction::id() const memcpy(result._hash, hash._hash, std::min(sizeof(result), sizeof(hash))); return result; } -void graphene::chain::signed_transaction::sign( const private_key_type& key ) +void graphene::chain::signed_transaction::sign(const private_key_type& key) { if( relative_expiration != 0 ) { - if( !block_id_cache.valid() ) edump((*this)); + // Relative expiration is set, meaning we must include the block ID in the signature assert(block_id_cache.valid()); - signatures.push_back( key.sign_compact( digest(*block_id_cache) ) ); + signatures.push_back(key.sign_compact(digest(*block_id_cache))); } else { - signatures.push_back( key.sign_compact( digest() ) ); + signatures.push_back(key.sign_compact(digest())); } } diff --git a/tests/tests/block_tests.cpp b/tests/tests/block_tests.cpp index b0b9e613..037b8c4d 100644 --- a/tests/tests/block_tests.cpp +++ b/tests/tests/block_tests.cpp @@ -587,31 +587,33 @@ BOOST_FIXTURE_TEST_CASE( double_sign_check, database_fixture ) trx.set_expiration(db.head_block_time() + fc::minutes(1)); trx.operations.push_back(transfer_operation({ asset(), from.id, to.id, amount, memo_data() })); - for( auto& op : trx.operations ) op.visit( operation_set_fee( db.current_fee_schedule() ) ); + for( auto& op : trx.operations ) op.visit(operation_set_fee(db.current_fee_schedule())); trx.validate(); db.push_transaction(trx, ~0); trx.operations.clear(); trx.operations.push_back(transfer_operation({ asset(), to.id, from.id, amount, memo_data() })); - for( auto& op : trx.operations ) op.visit( operation_set_fee( db.current_fee_schedule() ) ); + for( auto& op : trx.operations ) op.visit(operation_set_fee(db.current_fee_schedule())); trx.validate(); BOOST_TEST_MESSAGE( "Verify that not-signing causes an exception" ); - BOOST_REQUIRE_THROW( db.push_transaction(trx, 0 ), fc::exception ); - trx.sign( to_private_key ); + BOOST_REQUIRE_THROW( db.push_transaction(trx, 0), fc::exception ); BOOST_TEST_MESSAGE( "Verify that double-signing causes an exception" ); - BOOST_REQUIRE_THROW( db.push_transaction(trx, 0 ), fc::exception ); + trx.sign(to_private_key); + trx.sign(to_private_key); + BOOST_REQUIRE_THROW( db.push_transaction(trx, 0), fc::exception ); BOOST_TEST_MESSAGE( "Verify that signing with an extra, unused key fails" ); trx.signatures.pop_back(); - trx.sign( generate_private_key("bogus") ); - BOOST_REQUIRE_THROW( db.push_transaction(trx, 0 ), fc::exception ); + trx.sign(generate_private_key("bogus")); + BOOST_REQUIRE_THROW( db.push_transaction(trx, 0), fc::exception ); BOOST_TEST_MESSAGE( "Verify that signing once with the proper key passes" ); - db.push_transaction(trx, 0 ); - trx.sign( to_private_key ); + trx.signatures.pop_back(); + db.push_transaction(trx, 0); + trx.sign(to_private_key); } FC_LOG_AND_RETHROW() } diff --git a/tests/tests/operation_tests2.cpp b/tests/tests/operation_tests2.cpp index c479356c..7544e909 100644 --- a/tests/tests/operation_tests2.cpp +++ b/tests/tests/operation_tests2.cpp @@ -124,16 +124,19 @@ BOOST_AUTO_TEST_CASE( withdraw_permission_test ) REQUIRE_THROW_WITH_VALUE(op, amount_to_withdraw, asset(6)); trx.clear(); trx.operations.push_back(op); - trx.sign(dan_key_id, dan_private_key); + trx.sign(dan_private_key); PUSH_TX( db, trx ); // would be legal on its own, but doesn't work because trx already withdrew REQUIRE_THROW_WITH_VALUE(op, amount_to_withdraw, asset(5)); // Make sure we can withdraw again this period, as long as we're not exceeding the periodic limit - trx.operations.back() = op; // withdraw 1 - trx.ref_block_prefix++; // make it different from previous trx so it's non-duplicate - trx.sign(dan_key_id, dan_private_key); + trx.clear(); + // withdraw 1 + trx.operations = {op}; + // make it different from previous trx so it's non-duplicate + trx.ref_block_prefix++; + trx.sign(dan_private_key); PUSH_TX( db, trx ); trx.clear(); } @@ -149,10 +152,10 @@ BOOST_AUTO_TEST_CASE( withdraw_permission_test ) BOOST_CHECK(permit_object.withdrawal_limit == asset(5)); BOOST_CHECK(permit_object.withdrawal_period_sec == fc::hours(1).to_seconds()); BOOST_CHECK_EQUAL(permit_object.claimed_this_period.value, 2 ); - BOOST_CHECK(permit_object.expiration == first_start_time + 5*permit_object.withdrawal_period_sec ); + BOOST_CHECK(permit_object.expiration == first_start_time + 5*permit_object.withdrawal_period_sec); generate_blocks(first_start_time + permit_object.withdrawal_period_sec); // lazy update: verify period_start_time isn't updated until new trx occurs - BOOST_CHECK(permit_object.period_start_time == first_start_time ); + BOOST_CHECK(permit_object.period_start_time == first_start_time); } { @@ -163,11 +166,12 @@ BOOST_AUTO_TEST_CASE( withdraw_permission_test ) op.withdraw_to_account = dan_id; op.amount_to_withdraw = asset(5); trx.operations.push_back(op); - trx.sign(dan_key_id, dan_private_key); + trx.sign(dan_private_key); //Throws because nathan doesn't have the money BOOST_CHECK_THROW(PUSH_TX( db, trx ), fc::exception); op.amount_to_withdraw = asset(1); - trx.operations.back() = op; + trx.clear(); + trx.operations = {op}; trx.sign(dan_key_id, dan_private_key); PUSH_TX( db, trx ); } @@ -181,14 +185,14 @@ BOOST_AUTO_TEST_CASE( withdraw_permission_test ) const withdraw_permission_object& permit_object = permit(db); BOOST_CHECK(permit_object.authorized_account == dan_id); BOOST_CHECK(permit_object.withdraw_from_account == nathan_id); - BOOST_CHECK(permit_object.period_start_time == first_start_time + permit_object.withdrawal_period_sec ); - BOOST_CHECK(permit_object.expiration == first_start_time + 5*permit_object.withdrawal_period_sec ); + BOOST_CHECK(permit_object.period_start_time == first_start_time + permit_object.withdrawal_period_sec); + BOOST_CHECK(permit_object.expiration == first_start_time + 5*permit_object.withdrawal_period_sec); BOOST_CHECK(permit_object.withdrawal_limit == asset(5)); BOOST_CHECK(permit_object.withdrawal_period_sec == fc::hours(1).to_seconds()); generate_blocks(permit_object.expiration); } // Ensure the permit object has been garbage collected - BOOST_CHECK(db.find_object( permit ) == nullptr ); + BOOST_CHECK(db.find_object(permit) == nullptr); { withdraw_permission_claim_operation op; @@ -197,7 +201,7 @@ BOOST_AUTO_TEST_CASE( withdraw_permission_test ) op.withdraw_to_account = dan_id; op.amount_to_withdraw = asset(5); trx.operations.push_back(op); - trx.sign(dan_key_id, dan_private_key); + trx.sign(dan_private_key); //Throws because the permission has expired BOOST_CHECK_THROW(PUSH_TX( db, trx ), fc::exception); } @@ -388,7 +392,7 @@ BOOST_AUTO_TEST_CASE( feed_limit_test ) op.asset_to_update = bit_usd.get_id(); op.issuer = bit_usd.issuer; trx.operations = {op}; - trx.sign(nathan_key_id, nathan_private_key); + trx.sign(nathan_private_key); db.push_transaction(trx); BOOST_TEST_MESSAGE("Checking current_feed is null"); @@ -396,8 +400,9 @@ BOOST_AUTO_TEST_CASE( feed_limit_test ) BOOST_TEST_MESSAGE("Setting minimum feeds to 3"); op.new_options.minimum_feeds = 3; + trx.clear(); trx.operations = {op}; - trx.sign(nathan_key_id, nathan_private_key); + trx.sign(nathan_private_key); db.push_transaction(trx); BOOST_TEST_MESSAGE("Checking current_feed is not null");