Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix performance issue importing Kovan blocks #9914

Merged
merged 1 commit into from
Nov 14, 2018
Merged

Conversation

ngotchac
Copy link
Contributor

The block passed to self.engine.ancestry_actions didn't have all the Header's data, thus the block.header.hash() was wrong, and the condition finality_checker.subchain_head() != Some(*chain_head.parent_hash()) in AuthorityRound::build_finality would never be false, so we would check all ancestry for every imported blocks, until a finalised one was to be found. This was very costly in same cases, eg. when first syncing the chain: on current master I would sync at 50 blocks/s, with this PR (as on stable) it goes at about 1_000 blocks/s.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Nov 13, 2018
@ngotchac ngotchac requested a review from andresilva November 13, 2018 18:17
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

Nice find! 👍 Since we used the wrong hash when building the subchain the caching wouldn't work properly and we'd have to rebuild the subchain on every block.

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Amazing!

@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 13, 2018
@@ -815,10 +816,11 @@ impl AuthorityRound {
None
}
})
.while_some();
.while_some()
.take_while(|&(h, _)| h != epoch_transition_hash);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So that limits the iterator, can you elaborate a little bit why it's valid to do so?

Copy link
Contributor

Choose a reason for hiding this comment

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

I discussed this with @ngotchac. Aura only used to track finality for safely triggering validator set changes, therefore we were only interested in finalizing blocks within the current epoch, so this take_while was the original behavior. Recently we added finality tracking to the client so we updated Aura to track finality until the last finalized block. On chains where there isn't a majority of validators online the unfinalized subchain just keeps growing and this can take some time to build (although if the subchain is properly cached this cost is avoided).

@ordian
Copy link
Collaborator

ordian commented Nov 14, 2018

#9692 is in beta, so should we backport this PR to beta as well?

@andresilva
Copy link
Contributor

@ordian yes we should 👍

@Tbaut Tbaut mentioned this pull request Nov 14, 2018
15 tasks
@Tbaut Tbaut merged commit ac974a1 into master Nov 14, 2018
@Tbaut Tbaut deleted the ng-aura-finality branch November 14, 2018 12:05
ordian pushed a commit that referenced this pull request Nov 14, 2018
Tbaut added a commit that referenced this pull request Nov 14, 2018
* Bump beta to version 2.2.1

* fix: Intermittent failing CI due to addr in use (#9885)

Allow OS to set port at runtime

* Use Weak reference in PubSubClient (#9886)

* Fix json tracer overflow (#9873)

* Fix json tracer overflow

* Replace trace_executed with a direct trace push

* Remove unused variable

* Add test for 5a51

* Remove duplicate json!

* Fix docker script (#9854)


* Dockerfile: change source path of the newly added check_sync.sh (#9869)

* Allow to seal work on latest block (#9876)

* Allow to seal work on latest block.

* Test from @todr to check sealing conditions.

* gitlab-ci: make android release build succeed (#9743)

* use docker cargo config file for android builds

* make android build succeed

* ethcore: use Machine::verify_transaction on parent block (#9900)

* ethcore: use Machine::verify_transaction on parent block

also fixes off-by-one activation of transaction permission contract

* ethcore: clarify call to verify_transaction

* foundation: #6692865, ropsten: #4417537, kovan: #9363457

* Remove rust-toolchain file (#9906)

* EIP-712 implementation (#9631)

* EIP-712 impl

* added more tests

* removed size parsing unwrap

* corrected TYPE_REGEX to disallow zero sized fixed length arrays, replaced LinkedHashSet with IndexSet, added API spec to docs, fixed Type::Byte encoding branch

* use Option<u64> instead of u64 for Type::Array::Length

* replace `.iter()` with  `.values()`

Co-Authored-By: seunlanlege <[email protected]>

* tabify eip712.rs

* use proper comments for docs

* Cargo.lock: revert unrelated changes

* tabify encode.rs

* EIP 191 (#9701)

* added sign_191 rpc method

* fixed hash_structured_data return type

* added ConfirmationPayload::SignMessage for non-prefixed signatures, added tests for sign191

* renamed WithValidator -> PresignedTransaction

* rename applicationData to data in test

* adds docs for EIP191Version, renamed SignRequest to EIP191SignRequest

* light-fetch: Differentiate between out-of-gas/manual throw and use required gas from response on failure (#9824)

* fix start_gas, handle OOG exceptions & NotEnoughGas

* Change START_GAS: 50_000 -> 60_000
* When the `OutOfGas exception` is received then try to double the gas until it succeeds or block gas limit is reached
* When `NotEnoughBasGas error` is received then use the required gas provided in the response

* fix(light-fetch): ensure block_gas_limit is tried

Try the `block_gas_limit` before regard the execution as an error

* Update rpc/src/v1/helpers/light_fetch.rs

Co-Authored-By: niklasad1 <[email protected]>

* simplify cargo audit

* Use block header for building finality (#9914)

* ci: nuke the gitlab caches (#9855)
@5chdn 5chdn added this to the 2.3 milestone Nov 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants