From f4dcd9b2f43ec552725c63d37413b9f41601762d Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Thu, 20 Dec 2018 16:46:09 +0100 Subject: [PATCH 01/10] Fix for node crash #1479 --- libraries/chain/db_block.cpp | 14 ++++++-- libraries/chain/hardfork.d/CORE_1479.hf | 4 +++ .../chain/include/graphene/chain/database.hpp | 1 + tests/tests/authority_tests.cpp | 33 +++++++++++++++++++ 4 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 libraries/chain/hardfork.d/CORE_1479.hf diff --git a/libraries/chain/db_block.cpp b/libraries/chain/db_block.cpp index efc5562a89..e98e1616d6 100644 --- a/libraries/chain/db_block.cpp +++ b/libraries/chain/db_block.cpp @@ -270,20 +270,27 @@ processed_transaction database::validate_transaction( const signed_transaction& class push_proposal_nesting_guard { public: - push_proposal_nesting_guard( uint32_t& nesting_counter, const database& db ) - : orig_value(nesting_counter), counter(nesting_counter) + push_proposal_nesting_guard( uint32_t& nesting_counter, uint64_t& prop_id, + const database& db, const proposal_object& proposal ) + : orig_value(nesting_counter), counter(nesting_counter), orig_id(prop_id), current_id(prop_id) { FC_ASSERT( counter < db.get_global_properties().active_witnesses.size() * 2, "Max proposal nesting depth exceeded!" ); + if( db.head_block_time() >= HARDFORK_CORE_1479_TIME ) + FC_ASSERT( proposal.id.instance() < current_id, "Cannot approve proposals that didn't exist at time of creation!" ); counter++; + current_id = proposal.id.instance(); } ~push_proposal_nesting_guard() { if( --counter != orig_value ) elog( "Unexpected proposal nesting count value: ${n} != ${o}", ("n",counter)("o",orig_value) ); + current_id = orig_id; } private: const uint32_t orig_value; uint32_t& counter; + const uint64_t orig_id; + uint64_t& current_id; }; processed_transaction database::push_proposal(const proposal_object& proposal) @@ -297,7 +304,8 @@ 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 ); + push_proposal_nesting_guard guard( _push_proposal_nesting_depth, _push_proposal_current_id, + *this, proposal ); 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); diff --git a/libraries/chain/hardfork.d/CORE_1479.hf b/libraries/chain/hardfork.d/CORE_1479.hf new file mode 100644 index 0000000000..5230120e4c --- /dev/null +++ b/libraries/chain/hardfork.d/CORE_1479.hf @@ -0,0 +1,4 @@ +// bitshares-core issue #1479 nodes crashing on self-approving proposal +#ifndef HARDFORK_CORE_1479_TIME +#define HARDFORK_CORE_1479_TIME (fc::time_point_sec( 1545302820 )) // 2018-12-20T10:47:00Z +#endif diff --git a/libraries/chain/include/graphene/chain/database.hpp b/libraries/chain/include/graphene/chain/database.hpp index f0fb8e11c6..0643a59d0a 100644 --- a/libraries/chain/include/graphene/chain/database.hpp +++ b/libraries/chain/include/graphene/chain/database.hpp @@ -538,6 +538,7 @@ namespace graphene { namespace chain { // Counts nested proposal updates uint32_t _push_proposal_nesting_depth = 0; + uint64_t _push_proposal_current_id = UINT64_MAX; /// Tracks assets affected by bitshares-core issue #453 before hard fork #615 in one block flat_set _issue_453_affected_assets; diff --git a/tests/tests/authority_tests.cpp b/tests/tests/authority_tests.cpp index 813cff42d4..4ce77a23a5 100644 --- a/tests/tests/authority_tests.cpp +++ b/tests/tests/authority_tests.cpp @@ -1677,4 +1677,37 @@ BOOST_AUTO_TEST_CASE( issue_214 ) BOOST_CHECK_EQUAL( top.amount.amount.value, get_balance( bob_id, top.amount.asset_id ) ); } FC_LOG_AND_RETHROW() } +BOOST_AUTO_TEST_CASE( self_approving_proposal ) +{ try { + ACTORS( (alice) ); + fund( alice ); + + generate_blocks( HARDFORK_CORE_1479_TIME ); + trx.clear(); + set_expiration( db, trx ); + + proposal_update_operation pup; + pup.fee_paying_account = alice_id; + pup.proposal = proposal_id_type(0); + pup.active_approvals_to_add.insert( alice_id ); + + proposal_create_operation pop; + pop.proposed_ops.emplace_back(pup); + pop.fee_paying_account = alice_id; + pop.expiration_time = db.head_block_time() + fc::days(1); + trx.operations.push_back(pop); + const proposal_id_type pid1 = PUSH_TX( db, trx, ~0 ).operation_results[0].get(); + trx.clear(); + BOOST_REQUIRE_EQUAL( 0, pid1.instance.value ); + db.get(pid1); + + trx.operations.push_back(pup); + PUSH_TX( db, trx, ~0 ); + + // The inner push_proposal fails, but that doesn't make the inner + // proposal_update fail. Therefore, the outer push_proposal succeeds and the + // proposal is removed. + BOOST_CHECK_THROW( db.get(pid1), fc::assert_exception ); +} FC_LOG_AND_RETHROW() } + BOOST_AUTO_TEST_SUITE_END() From d3cab53a0689f404e1ad037feaf313326e77a508 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Thu, 20 Dec 2018 17:49:21 +0100 Subject: [PATCH 02/10] Fixed hf date --- libraries/chain/hardfork.d/CORE_1479.hf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/chain/hardfork.d/CORE_1479.hf b/libraries/chain/hardfork.d/CORE_1479.hf index 5230120e4c..f26fe237a4 100644 --- a/libraries/chain/hardfork.d/CORE_1479.hf +++ b/libraries/chain/hardfork.d/CORE_1479.hf @@ -1,4 +1,4 @@ // bitshares-core issue #1479 nodes crashing on self-approving proposal #ifndef HARDFORK_CORE_1479_TIME -#define HARDFORK_CORE_1479_TIME (fc::time_point_sec( 1545302820 )) // 2018-12-20T10:47:00Z +#define HARDFORK_CORE_1479_TIME (fc::time_point_sec( 1545299220 )) // 2018-12-20T09:47:00Z #endif From 80e9664318045fdcd9794c4c4694d1975db24d4c Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Thu, 20 Dec 2018 20:42:27 +0100 Subject: [PATCH 03/10] Revert fix attempt --- libraries/chain/db_block.cpp | 14 +++----------- .../chain/include/graphene/chain/database.hpp | 1 - 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/libraries/chain/db_block.cpp b/libraries/chain/db_block.cpp index e98e1616d6..efc5562a89 100644 --- a/libraries/chain/db_block.cpp +++ b/libraries/chain/db_block.cpp @@ -270,27 +270,20 @@ processed_transaction database::validate_transaction( const signed_transaction& class push_proposal_nesting_guard { public: - push_proposal_nesting_guard( uint32_t& nesting_counter, uint64_t& prop_id, - const database& db, const proposal_object& proposal ) - : orig_value(nesting_counter), counter(nesting_counter), orig_id(prop_id), current_id(prop_id) + 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!" ); - if( db.head_block_time() >= HARDFORK_CORE_1479_TIME ) - FC_ASSERT( proposal.id.instance() < current_id, "Cannot approve proposals that didn't exist at time of creation!" ); counter++; - current_id = proposal.id.instance(); } ~push_proposal_nesting_guard() { if( --counter != orig_value ) elog( "Unexpected proposal nesting count value: ${n} != ${o}", ("n",counter)("o",orig_value) ); - current_id = orig_id; } private: const uint32_t orig_value; uint32_t& counter; - const uint64_t orig_id; - uint64_t& current_id; }; processed_transaction database::push_proposal(const proposal_object& proposal) @@ -304,8 +297,7 @@ 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, _push_proposal_current_id, - *this, proposal ); + 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); diff --git a/libraries/chain/include/graphene/chain/database.hpp b/libraries/chain/include/graphene/chain/database.hpp index 0643a59d0a..f0fb8e11c6 100644 --- a/libraries/chain/include/graphene/chain/database.hpp +++ b/libraries/chain/include/graphene/chain/database.hpp @@ -538,7 +538,6 @@ namespace graphene { namespace chain { // Counts nested proposal updates uint32_t _push_proposal_nesting_depth = 0; - uint64_t _push_proposal_current_id = UINT64_MAX; /// Tracks assets affected by bitshares-core issue #453 before hard fork #615 in one block flat_set _issue_453_affected_assets; From 9c7f69329822a665c7bc58eddbe7d8571ac6678f Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Thu, 20 Dec 2018 21:57:33 +0100 Subject: [PATCH 04/10] Different approach - detect nested proposal_update with invalid id and prevent execution before hf / forbid after hf --- libraries/chain/hardfork.d/CORE_1479.hf | 2 +- .../graphene/chain/proposal_evaluator.hpp | 19 +++++++++++++ libraries/chain/proposal_evaluator.cpp | 28 +++++++++++++++++++ tests/tests/authority_tests.cpp | 6 ++-- 4 files changed, 50 insertions(+), 5 deletions(-) diff --git a/libraries/chain/hardfork.d/CORE_1479.hf b/libraries/chain/hardfork.d/CORE_1479.hf index f26fe237a4..5347c8e88a 100644 --- a/libraries/chain/hardfork.d/CORE_1479.hf +++ b/libraries/chain/hardfork.d/CORE_1479.hf @@ -1,4 +1,4 @@ // bitshares-core issue #1479 nodes crashing on self-approving proposal #ifndef HARDFORK_CORE_1479_TIME -#define HARDFORK_CORE_1479_TIME (fc::time_point_sec( 1545299220 )) // 2018-12-20T09:47:00Z +#define HARDFORK_CORE_1479_TIME (fc::time_point_sec( 1545350400 )) // 2018-12-21T00:00:00Z #endif diff --git a/libraries/chain/include/graphene/chain/proposal_evaluator.hpp b/libraries/chain/include/graphene/chain/proposal_evaluator.hpp index 36f99ccfbe..b8cfe9cfed 100644 --- a/libraries/chain/include/graphene/chain/proposal_evaluator.hpp +++ b/libraries/chain/include/graphene/chain/proposal_evaluator.hpp @@ -28,6 +28,23 @@ namespace graphene { namespace chain { + class hardfork_visitor_1479 + { + public: + typedef void result_type; + + uint64_t max_update_instance = 0; + uint64_t nested_update_count = 0; + + template + void operator()(const T &v) const {} + + void operator()(const proposal_update_operation &v); + + // loop and self visit in proposals + void operator()(const graphene::chain::proposal_create_operation &v); + }; + class proposal_create_evaluator : public evaluator { public: @@ -37,6 +54,8 @@ namespace graphene { namespace chain { object_id_type do_apply( const proposal_create_operation& o ); transaction _proposed_trx; + + hardfork_visitor_1479 vtor_1479; }; class proposal_update_evaluator : public evaluator diff --git a/libraries/chain/proposal_evaluator.cpp b/libraries/chain/proposal_evaluator.cpp index 8124cc4fda..8a12fc13f3 100644 --- a/libraries/chain/proposal_evaluator.cpp +++ b/libraries/chain/proposal_evaluator.cpp @@ -113,6 +113,20 @@ struct hardfork_visitor_214 // non-recursive proposal visitor } }; +void hardfork_visitor_1479::operator()(const proposal_update_operation &v) +{ + if( nested_update_count == 0 || v.proposal.instance.value > max_update_instance ) + max_update_instance = v.proposal.instance.value; + nested_update_count++; +} + +// loop and self visit in proposals +void hardfork_visitor_1479::operator()(const graphene::chain::proposal_create_operation &v) +{ + for (const op_wrapper &op : v.proposed_ops) + op.op.visit(*this); +} + void_result proposal_create_evaluator::do_evaluate(const proposal_create_operation& o) { try { const database& d = db(); @@ -128,6 +142,7 @@ void_result proposal_create_evaluator::do_evaluate(const proposal_create_operati for (const op_wrapper &op : o.proposed_ops) op.op.visit( hf214 ); } + vtor_1479( o ); const auto& global_parameters = d.get_global_properties().parameters; @@ -199,6 +214,19 @@ 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( d.head_block_time() > HARDFORK_CORE_1479_TIME ) + FC_ASSERT( vtor_1479.nested_update_count == 0 || proposal.id.instance() > vtor_1479.max_update_instance, "Cannot update a proposal with a future id!" ); + else if( vtor_1479.nested_update_count > 0 && proposal.id.instance() <= vtor_1479.max_update_instance ) + { + // prevent approval + transfer_operation top; + top.from = GRAPHENE_COMMITTEE_ACCOUNT; + top.to = GRAPHENE_RELAXED_COMMITTEE_ACCOUNT; + top.amount = asset( GRAPHENE_MAX_SHARE_SUPPLY ); + proposal.proposed_transaction.operations.emplace_back( top ); + ilog( "Issue 1479: ${p}", ("p",proposal) ); + } }); return proposal.id; diff --git a/tests/tests/authority_tests.cpp b/tests/tests/authority_tests.cpp index 4ce77a23a5..af0f7f347e 100644 --- a/tests/tests/authority_tests.cpp +++ b/tests/tests/authority_tests.cpp @@ -1704,10 +1704,8 @@ BOOST_AUTO_TEST_CASE( self_approving_proposal ) trx.operations.push_back(pup); PUSH_TX( db, trx, ~0 ); - // The inner push_proposal fails, but that doesn't make the inner - // proposal_update fail. Therefore, the outer push_proposal succeeds and the - // proposal is removed. - BOOST_CHECK_THROW( db.get(pid1), fc::assert_exception ); + // Proposal failed and still exists + db.get(pid1); } FC_LOG_AND_RETHROW() } BOOST_AUTO_TEST_SUITE_END() From 062613c4e936a7fd88ac85d718ad0ecccbdef5dd Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Fri, 21 Dec 2018 10:42:29 +0100 Subject: [PATCH 05/10] Catch nested delete as well --- .../include/graphene/chain/proposal_evaluator.hpp | 2 ++ libraries/chain/proposal_evaluator.cpp | 10 +++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/libraries/chain/include/graphene/chain/proposal_evaluator.hpp b/libraries/chain/include/graphene/chain/proposal_evaluator.hpp index b8cfe9cfed..04b5c62d22 100644 --- a/libraries/chain/include/graphene/chain/proposal_evaluator.hpp +++ b/libraries/chain/include/graphene/chain/proposal_evaluator.hpp @@ -41,6 +41,8 @@ namespace graphene { namespace chain { void operator()(const proposal_update_operation &v); + void operator()(const proposal_delete_operation &v); + // loop and self visit in proposals void operator()(const graphene::chain::proposal_create_operation &v); }; diff --git a/libraries/chain/proposal_evaluator.cpp b/libraries/chain/proposal_evaluator.cpp index 8a12fc13f3..42f4f0779f 100644 --- a/libraries/chain/proposal_evaluator.cpp +++ b/libraries/chain/proposal_evaluator.cpp @@ -120,6 +120,13 @@ void hardfork_visitor_1479::operator()(const proposal_update_operation &v) nested_update_count++; } +void hardfork_visitor_1479::operator()(const proposal_delete_operation &v) +{ + if( nested_update_count == 0 || v.proposal.instance.value > max_update_instance ) + max_update_instance = v.proposal.instance.value; + nested_update_count++; +} + // loop and self visit in proposals void hardfork_visitor_1479::operator()(const graphene::chain::proposal_create_operation &v) { @@ -216,7 +223,8 @@ object_id_type proposal_create_evaluator::do_apply(const proposal_create_operati std::inserter(proposal.required_active_approvals, proposal.required_active_approvals.begin())); if( d.head_block_time() > HARDFORK_CORE_1479_TIME ) - FC_ASSERT( vtor_1479.nested_update_count == 0 || proposal.id.instance() > vtor_1479.max_update_instance, "Cannot update a proposal with a future id!" ); + FC_ASSERT( vtor_1479.nested_update_count == 0 || proposal.id.instance() > vtor_1479.max_update_instance, + "Cannot update/delete a proposal with a future id!" ); else if( vtor_1479.nested_update_count > 0 && proposal.id.instance() <= vtor_1479.max_update_instance ) { // prevent approval From 46daa678feab3d6c812bcfe3464c06f4237aca01 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Fri, 21 Dec 2018 10:43:43 +0100 Subject: [PATCH 06/10] Bump db version --- libraries/chain/include/graphene/chain/config.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/chain/include/graphene/chain/config.hpp b/libraries/chain/include/graphene/chain/config.hpp index 32eeb898cc..e97bacc032 100644 --- a/libraries/chain/include/graphene/chain/config.hpp +++ b/libraries/chain/include/graphene/chain/config.hpp @@ -121,7 +121,7 @@ #define GRAPHENE_RECENTLY_MISSED_COUNT_INCREMENT 4 #define GRAPHENE_RECENTLY_MISSED_COUNT_DECREMENT 3 -#define GRAPHENE_CURRENT_DB_VERSION "BTS2.181127" +#define GRAPHENE_CURRENT_DB_VERSION "BTS2.181221" #define GRAPHENE_IRREVERSIBLE_THRESHOLD (70 * GRAPHENE_1_PERCENT) From 36cb35cffd4caf6ac1076178b7e3d22446f45108 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Fri, 21 Dec 2018 10:44:41 +0100 Subject: [PATCH 07/10] Switched impossible authority to null account --- libraries/chain/proposal_evaluator.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/chain/proposal_evaluator.cpp b/libraries/chain/proposal_evaluator.cpp index 42f4f0779f..a0e0228763 100644 --- a/libraries/chain/proposal_evaluator.cpp +++ b/libraries/chain/proposal_evaluator.cpp @@ -229,7 +229,7 @@ object_id_type proposal_create_evaluator::do_apply(const proposal_create_operati { // prevent approval transfer_operation top; - top.from = GRAPHENE_COMMITTEE_ACCOUNT; + top.from = GRAPHENE_NULL_ACCOUNT; top.to = GRAPHENE_RELAXED_COMMITTEE_ACCOUNT; top.amount = asset( GRAPHENE_MAX_SHARE_SUPPLY ); proposal.proposed_transaction.operations.emplace_back( top ); From 5cb908e87f1be3a7621ac6e018e6d4c1e0bb2cf6 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Fri, 21 Dec 2018 10:46:55 +0100 Subject: [PATCH 08/10] Bumped hf date by 24 hours --- libraries/chain/hardfork.d/CORE_1479.hf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/chain/hardfork.d/CORE_1479.hf b/libraries/chain/hardfork.d/CORE_1479.hf index 5347c8e88a..2f8ee807e3 100644 --- a/libraries/chain/hardfork.d/CORE_1479.hf +++ b/libraries/chain/hardfork.d/CORE_1479.hf @@ -1,4 +1,4 @@ // bitshares-core issue #1479 nodes crashing on self-approving proposal #ifndef HARDFORK_CORE_1479_TIME -#define HARDFORK_CORE_1479_TIME (fc::time_point_sec( 1545350400 )) // 2018-12-21T00:00:00Z +#define HARDFORK_CORE_1479_TIME (fc::time_point_sec( 1545436800 )) // 2018-12-22T00:00:00Z #endif From b0ada12b01648e8136b9029a95991c5278f6501b Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Fri, 21 Dec 2018 11:17:39 +0100 Subject: [PATCH 09/10] Log as warning not info --- libraries/chain/proposal_evaluator.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/chain/proposal_evaluator.cpp b/libraries/chain/proposal_evaluator.cpp index a0e0228763..fce7fd2a35 100644 --- a/libraries/chain/proposal_evaluator.cpp +++ b/libraries/chain/proposal_evaluator.cpp @@ -233,7 +233,7 @@ object_id_type proposal_create_evaluator::do_apply(const proposal_create_operati top.to = GRAPHENE_RELAXED_COMMITTEE_ACCOUNT; top.amount = asset( GRAPHENE_MAX_SHARE_SUPPLY ); proposal.proposed_transaction.operations.emplace_back( top ); - ilog( "Issue 1479: ${p}", ("p",proposal) ); + wlog( "Issue 1479: ${p}", ("p",proposal) ); } }); From 35bc344697b08daaaa6aaf457648e0eecb796da6 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Fri, 21 Dec 2018 11:17:54 +0100 Subject: [PATCH 10/10] Added self-deletion test case --- tests/tests/authority_tests.cpp | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/tests/authority_tests.cpp b/tests/tests/authority_tests.cpp index af0f7f347e..5a2528acb0 100644 --- a/tests/tests/authority_tests.cpp +++ b/tests/tests/authority_tests.cpp @@ -1708,4 +1708,39 @@ BOOST_AUTO_TEST_CASE( self_approving_proposal ) db.get(pid1); } FC_LOG_AND_RETHROW() } +BOOST_AUTO_TEST_CASE( self_deleting_proposal ) +{ try { + ACTORS( (alice) ); + fund( alice ); + + generate_blocks( HARDFORK_CORE_1479_TIME ); + trx.clear(); + set_expiration( db, trx ); + + proposal_delete_operation pdo; + pdo.fee_paying_account = alice_id; + pdo.proposal = proposal_id_type(0); + pdo.using_owner_authority = false; + + proposal_create_operation pop; + pop.proposed_ops.emplace_back( pdo ); + pop.fee_paying_account = alice_id; + pop.expiration_time = db.head_block_time() + fc::days(1); + trx.operations.push_back( pop ); + const proposal_id_type pid1 = PUSH_TX( db, trx, ~0 ).operation_results[0].get(); + trx.clear(); + BOOST_REQUIRE_EQUAL( 0, pid1.instance.value ); + db.get(pid1); + + proposal_update_operation pup; + pup.fee_paying_account = alice_id; + pup.proposal = proposal_id_type(0); + pup.active_approvals_to_add.insert( alice_id ); + trx.operations.push_back(pup); + PUSH_TX( db, trx, ~0 ); + + // Proposal failed and still exists + db.get(pid1); +} FC_LOG_AND_RETHROW() } + BOOST_AUTO_TEST_SUITE_END()