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

Add SealingState; don't prepare block when not ready. #10529

Merged
merged 1 commit into from
May 24, 2019

Conversation

afck
Copy link
Contributor

@afck afck commented Mar 27, 2019

This replaces Engine::seals_internally with Engine::sealing_state, and returns an enum indicating whether the engine seals internally and whether it is currently ready to do so.

For engines that don't seal internally, Miner::prepare_block is only called when they are ready. This prevents preparing (and subsequently failling to seal) blocks all the time with AuthorityRound.

@parity-cla-bot
Copy link

It looks like @afck hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@afck
Copy link
Contributor Author

afck commented Mar 27, 2019

I tried hard to not miss anything, but I'm not very confident about the change, so let's only merge it if someone who is more familiar with the code is certain that it makes sense.

An enum better conveys the meaning of the value, but of course I'm happy to revert to the Option<bool>, if preferred.

@afck
Copy link
Contributor Author

afck commented Mar 27, 2019

[clabot:check]

@parity-cla-bot
Copy link

It looks like @afck signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@soc1c soc1c added this to the 2.5 milestone Mar 27, 2019
@soc1c soc1c added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Mar 27, 2019
@niklasad1
Copy link
Collaborator

@afck Can you please rebase this branch because a force-push was pushed to master recently?

@afck afck force-pushed the afck-seals-internally-master branch from 58a90df to 8c1fd06 Compare April 1, 2019 16:56
@afck
Copy link
Contributor Author

afck commented Apr 1, 2019

Sorry for the delay! I rebased it.

@varasev
Copy link
Contributor

varasev commented May 3, 2019

@niklasad1 Could you (or someone else from Parity team) please review these changes? We made the same for our fork (poanetwork#103) but not sure the changes are correct. It would be awesome if someone from Parity could have a look at these changes. It would help AuRa nodes not to waste CPU time when they are not validators. Thank you.

Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

LGTM (Need again rebase on master)

@sorpaas sorpaas added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 7, 2019
@afck afck force-pushed the afck-seals-internally-master branch from 8c1fd06 to 7a457bc Compare May 7, 2019 12:03
@afck
Copy link
Contributor Author

afck commented May 9, 2019

Sorry, I don't understand why one of the builds failed: Is that something about the build cache, or is it an actual test failure?

@niklasad1
Copy link
Collaborator

@afck it is build cache error, I don't know why it failed but I restarted and it succeed!

@niklasad1 niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels May 9, 2019
@dvdplm dvdplm merged commit bf24255 into openethereum:master May 24, 2019
@afck afck deleted the afck-seals-internally-master branch May 24, 2019 08:57
ordian added a commit that referenced this pull request May 28, 2019
* master:
  Upgrade to parity-crypto 0.4 (#10650)
  new image (#10673)
  Add SealingState; don't prepare block when not ready. (#10529)
  Fix compiler warning (that will become an error) (#10683)
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.

7 participants