Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[GRPH-94] Improve block generation performance #139

Merged
merged 5 commits into from
Sep 24, 2019
Merged

[GRPH-94] Improve block generation performance #139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12105ab
Skip auth check when pushing self-generated blocks
abitmore Aug 13, 2018
56a6f8b
Extract public keys before pushing a transaction
abitmore Aug 22, 2018
c110508
Dereference chain_database shared_ptr
abitmore Aug 23, 2018
2dfb67e
Updated transaction::signees to mutable
abitmore Aug 27, 2018
106824c
No longer extract public keys before pushing a trx
abitmore Aug 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libraries/chain/db_block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ signed_block database::_generate_block(
FC_ASSERT( fc::raw::pack_size(pending_block) <= get_global_properties().parameters.maximum_block_size );
}

push_block( pending_block, skip );
push_block( pending_block, skip | skip_transaction_signatures ); // skip authority check when pushing self-generated blocks

return pending_block;
} FC_CAPTURE_AND_RETHROW( (witness_id) ) }
Expand Down
25 changes: 22 additions & 3 deletions libraries/chain/include/graphene/chain/protocol/transaction.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,12 +165,30 @@ namespace graphene { namespace chain {
uint32_t max_recursion = GRAPHENE_MAX_SIG_CHECK_DEPTH
) const;

flat_set<public_key_type> get_signature_keys( const chain_id_type& chain_id )const;
/**
* @brief Extract public keys from signatures with given chain ID.
* @param chain_id A chain ID
* @return Public keys
* @note If @ref signees is empty, E.G. when it's the first time calling
* this function for the signed transaction, public keys will be
* extracted with given chain ID, and be stored into the mutable
* @ref signees field, then @ref signees will be returned;
* otherwise, the @ref chain_id parameter will be ignored, and
* @ref signees will be returned directly.
*/
const flat_set<public_key_type>& get_signature_keys( const chain_id_type& chain_id )const;

/** Signatures */
vector<signature_type> signatures;

/// Removes all operations and signatures
void clear() { operations.clear(); signatures.clear(); }
/** Public keys extracted from signatures */
mutable flat_set<public_key_type> signees;

/// Removes all operations, signatures and signees
void clear() { operations.clear(); signatures.clear(); signees.clear(); }

/// Removes all signatures and signees
void clear_signatures() { signatures.clear(); signees.clear(); }
};

void verify_authority( const vector<operation>& ops, const flat_set<public_key_type>& sigs,
Expand Down Expand Up @@ -209,5 +227,6 @@ namespace graphene { namespace chain {
} } // graphene::chain

FC_REFLECT( graphene::chain::transaction, (ref_block_num)(ref_block_prefix)(expiration)(operations)(extensions) )
// Note: not reflecting signees field for backward compatibility; in addition, it should not be in p2p messages
FC_REFLECT_DERIVED( graphene::chain::signed_transaction, (graphene::chain::transaction), (signatures) )
FC_REFLECT_DERIVED( graphene::chain::processed_transaction, (graphene::chain::signed_transaction), (operation_results) )
30 changes: 18 additions & 12 deletions libraries/chain/protocol/transaction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const signature_type& graphene::chain::signed_transaction::sign(const private_ke
{
digest_type h = sig_digest( chain_id );
signatures.push_back(key.sign_compact(h));
signees.clear(); // Clear signees since it may be inconsistent after added a new signature
return signatures.back();
}

Expand Down Expand Up @@ -297,22 +298,27 @@ void verify_authority( const vector<operation>& ops, const flat_set<public_key_t
} FC_CAPTURE_AND_RETHROW( (ops)(sigs) ) }


flat_set<public_key_type> signed_transaction::get_signature_keys( const chain_id_type& chain_id )const
const flat_set<public_key_type>& signed_transaction::get_signature_keys( const chain_id_type& chain_id )const
{ try {
auto d = sig_digest( chain_id );
flat_set<public_key_type> result;
for( const auto& sig : signatures )
// Strictly we should check whether the given chain ID is same as the one used to initialize the `signees` field.
// However, we don't pass in another chain ID so far, for better performance, we skip the check.
if( signees.empty() && !signatures.empty() )
{
GRAPHENE_ASSERT(
result.insert( fc::ecc::public_key(sig,d) ).second,
tx_duplicate_sig,
"Duplicate Signature detected" );
auto d = sig_digest( chain_id );
flat_set<public_key_type> result;
for( const auto& sig : signatures )
{
GRAPHENE_ASSERT(
result.insert( fc::ecc::public_key(sig,d) ).second,
tx_duplicate_sig,
"Duplicate Signature detected" );
}
signees = std::move( result );
}
return result;
return signees;
} FC_CAPTURE_AND_RETHROW() }



set<public_key_type> signed_transaction::get_required_signatures(
const chain_id_type& chain_id,
const flat_set<public_key_type>& available_keys,
Expand All @@ -325,8 +331,8 @@ set<public_key_type> signed_transaction::get_required_signatures(
vector<authority> other;
get_required_authorities( required_active, required_owner, other );


sign_state s(get_signature_keys( chain_id ),get_active,available_keys);
const flat_set<public_key_type>& signature_keys = get_signature_keys( chain_id );
sign_state s( signature_keys, get_active, available_keys );
s.max_recursion = max_recursion_depth;

for( const auto& auth : other )
Expand Down
6 changes: 3 additions & 3 deletions libraries/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2196,6 +2196,7 @@ class wallet_api_impl
owned_keys.reserve(pks.size());
std::copy_if(pks.begin(), pks.end(), std::inserter(owned_keys, owned_keys.end()),
[this](const public_key_type &pk) { return _keys.find(pk) != _keys.end(); });
tx.clear_signatures();
set<public_key_type> approving_key_set = _remote_db->get_required_signatures(tx, owned_keys);

auto dyn_props = get_dynamic_global_properties();
Expand All @@ -2213,8 +2214,8 @@ class wallet_api_impl
uint32_t expiration_time_offset = 0;
for (;;)
{
tx.set_expiration(dyn_props.time + fc::seconds(30 + expiration_time_offset));
tx.signatures.clear();
tx.set_expiration( dyn_props.time + fc::seconds(30 + expiration_time_offset) );
tx.clear_signatures();

for (const public_key_type &key : approving_key_set)
tx.sign(get_private_key(key), _chain_id);
Expand Down Expand Up @@ -2295,7 +2296,6 @@ class wallet_api_impl
trx.operations = {op};
set_operation_fees( trx, _remote_db->get_global_properties().parameters.current_fees);
trx.validate();
idump((broadcast));

return sign_transaction(trx, broadcast);
}
Expand Down
Binary file modified programs/build_helpers/cat-parts
View file
Open in desktop
Binary file not shown.
3 changes: 0 additions & 3 deletions tests/common/database_fixture.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,6 @@ const account_object& database_fixture::create_account(
trx.validate();

processed_transaction ptx = db.push_transaction(trx, ~0);
//wdump( (ptx) );
const account_object& result = db.get<account_object>(ptx.operation_results[0].get<object_id_type>());
trx.operations.clear();
return result;
Expand Down Expand Up @@ -769,7 +768,6 @@ const limit_order_object*database_fixture::create_sell_order(account_id_type use

const limit_order_object* database_fixture::create_sell_order( const account_object& user, const asset& amount, const asset& recv )
{
//wdump((amount)(recv));
limit_order_create_operation buy_order;
buy_order.seller = user.id;
buy_order.amount_to_sell = amount;
Expand All @@ -780,7 +778,6 @@ const limit_order_object* database_fixture::create_sell_order( const account_obj
auto processed = db.push_transaction(trx, ~0);
trx.operations.clear();
verify_asset_supplies(db);
//wdump((processed));
return db.find<limit_order_object>( processed.operation_results[0].get<object_id_type>() );
}

Expand Down
58 changes: 25 additions & 33 deletions tests/tests/authority_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,7 @@ BOOST_AUTO_TEST_CASE( any_two_of_three )
trx.operations.push_back(op);
sign(trx, nathan_key1);
PUSH_TX( db, trx, database::skip_transaction_dupe_check );
trx.operations.clear();
trx.signatures.clear();
trx.clear();
} FC_CAPTURE_AND_RETHROW ((nathan.active))

transfer_operation op;
Expand All @@ -99,19 +98,19 @@ BOOST_AUTO_TEST_CASE( any_two_of_three )
PUSH_TX( db, trx, database::skip_transaction_dupe_check );
BOOST_CHECK_EQUAL(get_balance(nathan, core), static_cast<int64_t>(old_balance - 500));

trx.signatures.clear();
trx.clear_signatures();
sign(trx, nathan_key2);
sign(trx, nathan_key3);
PUSH_TX( db, trx, database::skip_transaction_dupe_check );
BOOST_CHECK_EQUAL(get_balance(nathan, core), static_cast<int64_t>(old_balance - 1000));

trx.signatures.clear();
trx.clear_signatures();
sign(trx, nathan_key1);
sign(trx, nathan_key3);
PUSH_TX( db, trx, database::skip_transaction_dupe_check );
BOOST_CHECK_EQUAL(get_balance(nathan, core), static_cast<int64_t>(old_balance - 1500));

trx.signatures.clear();
trx.clear_signatures();
//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);
Expand Down Expand Up @@ -156,7 +155,7 @@ BOOST_AUTO_TEST_CASE( recursive_accounts )
BOOST_TEST_MESSAGE( "Attempting to transfer with parent1 signature, should fail" );
sign(trx,parent1_key);
GRAPHENE_CHECK_THROW(PUSH_TX( db, trx, database::skip_transaction_dupe_check ), fc::exception);
trx.signatures.clear();
trx.clear_signatures();

BOOST_TEST_MESSAGE( "Attempting to transfer with parent2 signature, should fail" );
sign(trx,parent2_key);
Expand All @@ -166,8 +165,7 @@ BOOST_AUTO_TEST_CASE( recursive_accounts )
sign(trx,parent1_key);
PUSH_TX( db, trx, database::skip_transaction_dupe_check );
BOOST_CHECK_EQUAL(get_balance(child, core), static_cast<int64_t>(old_balance - 500));
trx.operations.clear();
trx.signatures.clear();
trx.clear();

BOOST_TEST_MESSAGE( "Adding a key for the child that can override parents" );
fc::ecc::private_key child_key = fc::ecc::private_key::generate();
Expand All @@ -181,8 +179,7 @@ BOOST_AUTO_TEST_CASE( recursive_accounts )
sign(trx,parent2_key);
PUSH_TX( db, trx, database::skip_transaction_dupe_check );
BOOST_REQUIRE_EQUAL(child.active.num_auths(), 3u);
trx.operations.clear();
trx.signatures.clear();
trx.clear();
}

op.from = child.id;
Expand All @@ -195,7 +192,7 @@ BOOST_AUTO_TEST_CASE( recursive_accounts )
BOOST_TEST_MESSAGE( "Attempting transfer just parent1, should fail" );
sign(trx, parent1_key);
GRAPHENE_CHECK_THROW(PUSH_TX( db, trx, database::skip_transaction_dupe_check ), fc::exception);
trx.signatures.clear();
trx.clear_signatures();
BOOST_TEST_MESSAGE( "Attempting transfer just parent2, should fail" );
sign(trx, parent2_key);
GRAPHENE_CHECK_THROW(PUSH_TX( db, trx, database::skip_transaction_dupe_check ), fc::exception);
Expand All @@ -204,14 +201,13 @@ BOOST_AUTO_TEST_CASE( recursive_accounts )
sign(trx, parent1_key);
PUSH_TX( db, trx, database::skip_transaction_dupe_check );
BOOST_CHECK_EQUAL(get_balance(child, core), static_cast<int64_t>(old_balance - 1000));
trx.signatures.clear();
trx.clear_signatures();

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), static_cast<int64_t>(old_balance - 1500));
trx.operations.clear();
trx.signatures.clear();
trx.clear();

BOOST_TEST_MESSAGE( "Creating grandparent account, parent1 now requires authority of grandparent" );
auto grandparent = create_account("grandparent");
Expand All @@ -227,16 +223,15 @@ BOOST_AUTO_TEST_CASE( recursive_accounts )
op.owner = *op.active;
trx.operations.push_back(op);
PUSH_TX( db, trx, ~0 );
trx.operations.clear();
trx.signatures.clear();
trx.clear();
}

BOOST_TEST_MESSAGE( "Attempt to transfer using old parent keys, should fail" );
trx.operations.push_back(op);
sign(trx, parent1_key);
sign(trx, parent2_key);
GRAPHENE_CHECK_THROW(PUSH_TX( db, trx, database::skip_transaction_dupe_check ), fc::exception);
trx.signatures.clear();
trx.clear_signatures();
sign( trx, parent2_key );
sign( trx, grandparent_key );

Expand All @@ -253,8 +248,7 @@ BOOST_AUTO_TEST_CASE( recursive_accounts )
op.owner = *op.active;
trx.operations.push_back(op);
PUSH_TX( db, trx, ~0 );
trx.operations.clear();
trx.signatures.clear();
trx.clear();
}

BOOST_TEST_MESSAGE( "Create recursion depth failure" );
Expand All @@ -265,12 +259,11 @@ BOOST_AUTO_TEST_CASE( recursive_accounts )
//Fails due to recursion depth.
GRAPHENE_CHECK_THROW(PUSH_TX( db, trx, database::skip_transaction_dupe_check ), fc::exception);
BOOST_TEST_MESSAGE( "verify child key can override recursion checks" );
trx.signatures.clear();
trx.clear_signatures();
sign(trx, child_key);
PUSH_TX( db, trx, database::skip_transaction_dupe_check );
BOOST_CHECK_EQUAL(get_balance(child, core), static_cast<int64_t>(old_balance - 2500));
trx.operations.clear();
trx.signatures.clear();
trx.clear();

BOOST_TEST_MESSAGE( "Verify a cycle fails" );
{
Expand All @@ -280,8 +273,7 @@ BOOST_AUTO_TEST_CASE( recursive_accounts )
op.owner = *op.active;
trx.operations.push_back(op);
PUSH_TX( db, trx, ~0 );
trx.operations.clear();
trx.signatures.clear();
trx.clear();
}

trx.operations.push_back(op);
Expand Down Expand Up @@ -372,7 +364,7 @@ BOOST_AUTO_TEST_CASE( proposed_single_account )
//committee has no stake in the transaction.
GRAPHENE_CHECK_THROW(PUSH_TX( db, trx ), fc::exception);

trx.signatures.clear();
trx.clear_signatures();
pup.active_approvals_to_add.clear();
pup.active_approvals_to_add.insert(nathan.id);

Expand Down Expand Up @@ -408,7 +400,7 @@ BOOST_AUTO_TEST_CASE( proposal_failure )
pop.expiration_time = db.head_block_time() + fc::days(1);
pop.fee_paying_account = bob_id;
trx.operations.push_back( pop );
trx.signatures.clear();
trx.clear_signatures();
sign( trx, bob_private_key );
processed_transaction processed = PUSH_TX( db, trx );
proposal_object prop = db.get<proposal_object>(processed.operation_results.front().get<object_id_type>());
Expand Down Expand Up @@ -456,7 +448,7 @@ BOOST_AUTO_TEST_CASE( committee_authority )
sign(trx, committee_key);
GRAPHENE_CHECK_THROW(PUSH_TX( db, trx ), graphene::chain::invalid_committee_approval );

auto _sign = [&] { trx.signatures.clear(); sign( trx, nathan_key ); };
auto _sign = [&] { trx.clear_signatures(); sign( trx, nathan_key ); };

proposal_create_operation pop;
pop.proposed_ops.push_back({trx.operations.front()});
Expand Down Expand Up @@ -490,8 +482,7 @@ BOOST_AUTO_TEST_CASE( committee_authority )

BOOST_TEST_MESSAGE( "Checking that the proposal is not authorized to execute" );
BOOST_REQUIRE(!db.get<proposal_object>(prop.id).is_authorized_to_execute(db));
trx.operations.clear();
trx.signatures.clear();
trx.clear();
proposal_update_operation uop;
uop.fee_paying_account = GRAPHENE_TEMP_ACCOUNT;
uop.proposal = prop.id;
Expand All @@ -512,7 +503,7 @@ BOOST_AUTO_TEST_CASE( committee_authority )
// fails
// BOOST_CHECK(db.get<proposal_object>(prop.id).is_authorized_to_execute(db));

trx.signatures.clear();
trx.clear_signatures();
generate_blocks(*prop.review_period_time);
uop.key_approvals_to_add.clear();
uop.key_approvals_to_add.insert(committee_key.get_public_key()); // was 7
Expand Down Expand Up @@ -1069,16 +1060,17 @@ BOOST_FIXTURE_TEST_CASE( bogus_signature, database_fixture )
PUSH_TX( db, trx, skip );

trx.operations.push_back( xfer_op );
trx.signees.clear(); // signees should be invalidated
BOOST_TEST_MESSAGE( "Invalidating Alices Signature" );
// Alice's signature is now invalid
GRAPHENE_REQUIRE_THROW( PUSH_TX( db, trx, skip ), fc::exception );
// Re-sign, now OK (sig is replaced)
BOOST_TEST_MESSAGE( "Resign with Alice's Signature" );
trx.signatures.clear();
trx.clear_signatures();
sign( trx, alice_key );
PUSH_TX( db, trx, skip );

trx.signatures.clear();
trx.clear_signatures();
trx.operations.pop_back();
sign( trx, alice_key );
sign( trx, charlie_key );
Expand Down Expand Up @@ -1127,7 +1119,7 @@ BOOST_FIXTURE_TEST_CASE( voting_account, database_fixture )
GRAPHENE_CHECK_THROW(PUSH_TX( db, trx ), fc::exception);
op.new_options->num_committee = 3;
trx.operations = {op};
trx.signatures.clear();
trx.clear_signatures();
sign( trx, vikram_private_key );
PUSH_TX( db, trx );
trx.clear();
Expand Down
Loading