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

Validator/authority contract #3937

Merged
merged 48 commits into from
Jan 10, 2017
Merged

Validator/authority contract #3937

merged 48 commits into from
Jan 10, 2017

Conversation

keorn
Copy link

@keorn keorn commented Dec 21, 2016

Ability to use a contract at a specified address to determine the current list of validators (renamed from authorities) as an alternative to fixed spec list. The test contract is here.

Another change is that Engine now holds a Weak<Client> rather than IoChannel.

Do not merge for 1.5, goes after #3932.<\s>

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 86.689% when pulling b7996fa on validator-lookup into bce69b2 on master.

@rphmeier
Copy link
Contributor

Another change is that Engine now holds a Weak rather than IoChannel.

I really don't agree with this -- Engine is supposed to be a relatively agnostic consensus mechanism which the client and other related services can leverage. Any communication with other parts of the codebase should be done through a more general mechanism. I'm not sure IoChannel is quite right, but requiring a dependency on Client in the engine is overly restrictive.

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

gavofyork commented Dec 24, 2016

so Engine is indeed meant to be a resource, not a coordinator.

however, Engine and Client are very naturally tightly coupled and this becomes especially evident when aspects of the consensus algorithm (Engine) depend on aspects of the present state (Client) rather than just the other way around, as with the Ethash system. there could be a much tighter trait made from Client (EngineClient?) which only exposed a specific subset that could be useful to Engine (and that might be worth considering for a separate refactoring PR), but right now i fear this is a best we can manage.

@gavofyork gavofyork added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Dec 24, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 38ea976 on validator-lookup into ** on master**.

@keorn
Copy link
Author

keorn commented Jan 5, 2017

Now the two Engines hold EngineClient.
Client is passed since when necessary (validator set is in the state) an Engine will add_notify which inspects the validators using a closure, similar to the updater.

@keorn keorn removed the A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. label Jan 5, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 86.737% when pulling fa59451 on validator-lookup into 4c94878 on master.

@keorn keorn added A0-pleasereview 🤓 Pull request needs code review. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Jan 10, 2017
let mut sealing_work = self.sealing_work.lock();
sealing_work.enabled = self.engine.is_sealer(&address).unwrap_or(false);
*self.author.write() = address;
{
Copy link
Contributor

Choose a reason for hiding this comment

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

good to have a comment here to ensure any newcomer understands what the parens are for.

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

minor grumble. ltgm otherwise.

@keorn keorn added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 10, 2017
@keorn keorn mentioned this pull request Jan 10, 2017
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 10, 2017
@arkpar arkpar merged commit be30c44 into master Jan 10, 2017
@arkpar arkpar deleted the validator-lookup branch January 10, 2017 11:24
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. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants