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

Fix aura difficulty race #7198

Merged
merged 5 commits into from
Dec 7, 2017
Merged

Fix aura difficulty race #7198

merged 5 commits into from
Dec 7, 2017

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Dec 4, 2017

Previously this sequence of events was possible:

  • update_sealing called at step S when best block (from step S) imported
  • populate_from_parent creates a header with invalid difficulty (2^128 + S - S)
  • step changes to step S + 1
  • generate_seal seals the block with invalid difficulty at step S + 1
  • block is shortcutted into local chain while rejected by network
  • potentially fork

This might cause #6761 but will need further investigation.

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Dec 4, 2017
@rphmeier rphmeier requested a review from keorn December 4, 2017 18:16
@rphmeier rphmeier added the A8-backport 🕸 Pull request is already reviewed well in another branch. label Dec 4, 2017
@rphmeier rphmeier added B0-patch and removed A8-backport 🕸 Pull request is already reviewed well in another branch. labels Dec 4, 2017
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.

LGTM, tiny grumble.

let step = self.step.load();
let expected_diff = U256::from(U128::max_value()) + parent_step - step.into();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the difficulty calcution is already copy pasted in couple of places, maybe we should extract it into a helper function?

@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 5, 2017
let expected_diff = U256::from(U128::max_value()) + parent_step - step.into();

if header.difficulty() != &expected_diff {
return Seal::None;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to seal and release a block with a previous step instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case can also only happen when update_sealing is called upon import of new best block from the prior step, but update_sealing also gets called when the step changes. So triggering sealing again for the correct step is unnecessary.

@arkpar arkpar added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Dec 5, 2017
@5chdn 5chdn mentioned this pull request Dec 5, 2017
25 tasks
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Dec 6, 2017
@5chdn 5chdn added this to the 1.9 milestone Dec 6, 2017
@tomusdrw tomusdrw merged commit 3cb4d81 into master Dec 7, 2017
@tomusdrw tomusdrw deleted the fix-aura-difficulty-race branch December 7, 2017 11:17
tomusdrw pushed a commit that referenced this pull request Dec 7, 2017
* Fix Aura difficulty race

* fix test key

* extract out score calculation

* fix build
arkpar pushed a commit that referenced this pull request Dec 8, 2017
* Kovan HF.

* Bump version.

* Fix aura difficulty race (#7198)

* Fix Aura difficulty race

* fix test key

* extract out score calculation

* fix build

* Update kovan HF block number.

* Add missing byzantium builtins.

* Bump installers versions.

* Increase allowed time drift to 10s. (#7238)
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