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

New contract PoA sync fixes #5991

Merged
merged 15 commits into from
Jul 13, 2017
Merged

New contract PoA sync fixes #5991

merged 15 commits into from
Jul 13, 2017

Conversation

keorn
Copy link

@keorn keorn commented Jul 4, 2017

Generate proof on uncommitted state and dont report skipped proposer if not signer.
Also prevents re-sealing on same step when validator set changes.

@keorn keorn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 4, 2017
@rphmeier rphmeier added the A4-awaitingci 🤖 Pull request is waiting for changes on the CI to complete tests before review/merge can begin. label Jul 4, 2017
@keorn keorn removed the A4-awaitingci 🤖 Pull request is waiting for changes on the CI to complete tests before review/merge can begin. label Jul 5, 2017
@keorn keorn changed the title Dont report skipped proposer if not signer New contract PoA sync fixes Jul 6, 2017
@gavofyork gavofyork requested a review from rphmeier July 10, 2017 17:28
@rphmeier
Copy link
Contributor

@gavofyork I'm probably not the best person to review this impartially as Peter and I made these fixes together

@gavofyork gavofyork requested review from arkpar and removed request for rphmeier July 12, 2017 08:29
@@ -107,14 +107,14 @@ impl ValidatorSet for ValidatorContract {
fn report_malicious(&self, address: &Address, _set_block: BlockNumber, block: BlockNumber, proof: Bytes) {
match self.provider.report_malicious(&*self.transact(), *address, block.into(), proof).wait() {
Ok(_) => warn!(target: "engine", "Reported malicious validator {}", address),
Err(s) => warn!(target: "engine", "Validator {} could not be reported {}", address, s),
Err(_) => {} // warn!(target: "engine", "Validator {} could not be reported {}", address, s),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reason for warning removal?

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch. it was removed before because the checks weren't in place and it made testing very spammy

@arkpar arkpar added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 12, 2017
@gavofyork
Copy link
Contributor

@rphmeier doesn't build

@rphmeier rphmeier force-pushed the engine-signer-option branch from ec4bf60 to ad7507c Compare July 12, 2017 13:17
@rphmeier rphmeier added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jul 12, 2017
@rphmeier
Copy link
Contributor

rphmeier commented Jul 12, 2017

Added a few more commits to address two issues:

  1. ensure that transition proofs are always flushed, since finding the correct epoch transition proof can make use of a database iterator, which is only available on flushed data
  2. ensure that the amount of transition proof lookups are minimized by caching them between blocks. This actually also fixes the symptom that lead me to find the first issue, so that commit may not be necessary.

I would suggest keeping the additional flush for now since it's very rare, but then we can adjust kvdb::Database to support ordered iteration over the backing and buffered storage. See #6048

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jul 12, 2017
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 12, 2017
@gavofyork gavofyork merged commit 22261bc into master Jul 13, 2017
@gavofyork gavofyork deleted the engine-signer-option branch July 13, 2017 07:48
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.

4 participants