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

Performance opt pt 1 #1359

Merged
merged 5 commits into from
Oct 6, 2018
Merged

Performance opt pt 1 #1359

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
72c0d21
Avoid useless hash calculations
pmconrad Sep 17, 2018
5e1af9e
Skip merkle tree check during replay
pmconrad Sep 28, 2018
b2bafa4
Enable tx dupe check at end of replay
pmconrad Sep 17, 2018
4e6d2d4
Use cheaper comparison for public_key sets + maps
pmconrad Sep 27, 2018
e9f273d
Enable tx dupe check a little earlier
pmconrad Oct 6, 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
6 changes: 3 additions & 3 deletions libraries/chain/account_object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,9 @@ set<account_id_type> account_member_index::get_account_members(const account_obj
result.insert(auth.first);
return result;
}
set<public_key_type> account_member_index::get_key_members(const account_object& a)const
set<public_key_type, account_member_index::key_compare> account_member_index::get_key_members(const account_object& a)const
{
set<public_key_type> result;
set<public_key_type, key_compare> result;
for( auto auth : a.owner.key_auths )
result.insert(auth.first);
for( auto auth : a.active.key_auths )
Expand Down Expand Up @@ -215,7 +215,7 @@ void account_member_index::object_modified(const object& after)


{
set<public_key_type> after_key_members = get_key_members(a);
set<public_key_type, key_compare> after_key_members = get_key_members(a);

vector<public_key_type> removed; removed.reserve(before_key_members.size());
std::set_difference(before_key_members.begin(), before_key_members.end(),
Expand Down
11 changes: 7 additions & 4 deletions libraries/chain/db_block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,9 +626,12 @@ processed_transaction database::_apply_transaction(const signed_transaction& trx

auto& trx_idx = get_mutable_index_type<transaction_index>();
const chain_id_type& chain_id = get_chain_id();
auto trx_id = trx.id();
FC_ASSERT( (skip & skip_transaction_dupe_check) ||
trx_idx.indices().get<by_trx_id>().find(trx_id) == trx_idx.indices().get<by_trx_id>().end() );
transaction_id_type trx_id;
Copy link
Contributor

@jmjatlanta jmjatlanta Oct 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you did this. It is more easily readable IMO. Currently, you're protected in all cases with the ifs. But hopefully someone later will know better to not trust that trx_id is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will address this in #1360 (id is cached inside transaction there)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid using an uninitialized id, perhaps wrap it with optional? May cause some overheads though.

if( !(skip & skip_transaction_dupe_check) )
{
trx_id = trx.id();
FC_ASSERT( trx_idx.indices().get<by_trx_id>().find(trx_id) == trx_idx.indices().get<by_trx_id>().end() );
}
transaction_evaluation_state eval_state(this);
const chain_parameters& chain_parameters = get_global_properties().parameters;
eval_state._trx = &trx;
Expand Down Expand Up @@ -662,7 +665,7 @@ processed_transaction database::_apply_transaction(const signed_transaction& trx
//Insert transaction into unique transactions database.
if( !(skip & skip_transaction_dupe_check) )
{
create<transaction_object>([&](transaction_object& transaction) {
create<transaction_object>([&trx_id,&trx](transaction_object& transaction) {
transaction.trx_id = trx_id;
transaction.trx = trx;
});
Expand Down
4 changes: 4 additions & 0 deletions libraries/chain/db_management.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ void database::reindex( fc::path data_dir )

uint32_t skip = skip_witness_signature |
skip_block_size_check |
skip_merkle_check |
skip_transaction_signatures |
skip_transaction_dupe_check |
skip_tapos_check |
Expand All @@ -84,6 +85,7 @@ void database::reindex( fc::path data_dir )

size_t total_processed_block_size;
size_t total_block_size = _block_id_to_block.total_block_size();
const auto& gpo = get_global_properties();
for( uint32_t i = head_block_num() + 1; i <= last_block_num; ++i )
{
if( i % 10000 == 0 )
Expand All @@ -106,6 +108,8 @@ void database::reindex( fc::path data_dir )
flush();
ilog( "Done" );
}
if( head_block_time() >= last_block->timestamp - gpo.parameters.maximum_time_until_expiration )
skip &= ~skip_transaction_dupe_check;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this is better than the old code to prevent duplicate transactions from being pushed, I think it's not 100% safe due to the use of now(). For example, when maximum_time_until_expiration is 1 day, if last blocks in local database is 1 day before, after finished replay, the dupe-trx db would be empty, so when pushing new blocks received from p2p, dupe check will always pass even if a duplicate transaction is in a block.

Ideally we should use the timestamp of head block after replayed as now, however according to the gap processing below, perhaps it's hard to get head before replay is finished.

Alternatively, we can skip dupe-check during replay, and revisit the pushed blocks to fill the dupe-check db just before enabling undo_db. In order to determine how many blocks to revisit, we can keep track of the greatest maximum_time_until_expiration appeared during replay (assuming it's T) and least block_interval appeared (assuming it's I), then blocks_to_revisit = T/I.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, but I think it doesn't matter. The dupe check is really only relevant when a witness signs a new block, and that happens only close to now().
If replay ends at now - 1 day, then either we receive blocks created in the past 24 hours and have their tx's added to the dupe database, or there's a gap in the blockchain and the next block will be generated at now() which means older tx's have already expired.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is it's not only about block generation but also about block validation. In case when witnesses colluded signing/accepting blocks with duplicate transactions, other nodes remain the ability to recognize the misbehavior and reject invalid blocks.

I agree practically it's not a big deal. We've been running without that check for years, due to restrict timing/conditions required to exploit the potential issue. E.G. it only affects nodes that just finished a replay, but during normal operations most nodes won't be in that state.

IMHO we can leave the logic as proposed in this PR, but better add some comments there explaining why we did so and what scenarios have been considered and why ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After rethinking I switched to last_block->timestamp instead of now. After a chain halt (of course these won't happen again, so it doesn't really matter) it wouldn't require colluding witnesses - any witness could sneak a block with a dupe into the gap.
(Note that this would be noticed during a full sync.)

fc::optional< signed_block > block = _block_id_to_block.fetch_by_number(i);
if( !block.valid() )
{
Expand Down
14 changes: 11 additions & 3 deletions libraries/chain/include/graphene/chain/account_object.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,14 @@ namespace graphene { namespace chain {
*/
class account_member_index : public secondary_index
{
class key_compare {
public:
inline bool operator()( const public_key_type& a, const public_key_type& b )const
{
return a.key_data < b.key_data;
}
};

public:
virtual void object_inserted( const object& obj ) override;
virtual void object_removed( const object& obj ) override;
Expand All @@ -301,18 +309,18 @@ namespace graphene { namespace chain {

/** given an account or key, map it to the set of accounts that reference it in an active or owner authority */
map< account_id_type, set<account_id_type> > account_to_account_memberships;
map< public_key_type, set<account_id_type> > account_to_key_memberships;
map< public_key_type, set<account_id_type>, key_compare > account_to_key_memberships;
/** some accounts use address authorities in the genesis block */
map< address, set<account_id_type> > account_to_address_memberships;


protected:
set<account_id_type> get_account_members( const account_object& a )const;
set<public_key_type> get_key_members( const account_object& a )const;
set<public_key_type, key_compare> get_key_members( const account_object& a )const;
set<address> get_address_members( const account_object& a )const;

set<account_id_type> before_account_members;
set<public_key_type> before_key_members;
set<public_key_type, key_compare> before_key_members;
set<address> before_address_members;
};

Expand Down