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.

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

Conversation

Projects
None yet
3 participants
@pmconrad
Copy link
Contributor

commented Oct 4, 2018

This is some low hanging fruit discovered during my work on #1079 . I'm afraid the fully parallel pre-computation of blocks and transactions will not be finished in time for the upcoming release, therefore submitting this PR first.
The 4th commit in particular help to greatly improve startup time and should also be helpful with get_key_references API call. Related: #1151

@jmjatlanta
Copy link
Contributor

left a comment

This is a manageable-sized bite of performance improvements. I am running a sync on an old db to verify, but the code looks good. I'll update this when the sync finishes.

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;

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Oct 5, 2018

Contributor

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.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Oct 5, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 5, 2018

Member

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

@jmjatlanta
Copy link
Contributor

left a comment

Ran on Ubuntu 18.04 with Boost 1.67, OpenSSL 1.10. Reindexed, replayed, ran without issue. Code looks good.

@abitmore
Copy link
Member

left a comment

Since it's still WIP, perhaps will have answer later. Oops, made in wrong PR.

@@ -106,6 +109,8 @@ void database::reindex( fc::path data_dir )
flush();
ilog( "Done" );
}
if( head_block_time() >= now - gpo.parameters.maximum_time_until_expiration )
skip &= ~skip_transaction_dupe_check;

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 6, 2018

Member

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.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Oct 6, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 6, 2018

Member

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.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Oct 6, 2018

Author Contributor

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.)

@pmconrad

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2018

Is it normal that reviews get dismissed automatically when I push a new commit?

@abitmore

This comment has been minimized.

Copy link
Member

commented Oct 6, 2018

Is it normal that reviews get dismissed automatically when I push a new commit?

Yes. It's configured in repository settings.

@pmconrad pmconrad merged commit fd4a4f2 into develop Oct 6, 2018

3 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@pmconrad pmconrad deleted the performance_opt_1 branch Oct 6, 2018

@pmconrad

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2018

Thanks guys!

@pmconrad pmconrad referenced this pull request Oct 13, 2018

Closed

Sorting of public_keys is inefficient #1371

4 of 17 tasks complete

@pmconrad pmconrad referenced this pull request Feb 1, 2019

Open

Merge improvements from BitShares #26

4 of 24 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.