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

RPC method for clearing the engine signer #10920

Merged

Conversation

vkomenda
Copy link
Contributor

This PR adds a new RPC method for clearing the engine signer value.

Original PR: poanetwork#114

See also gnosischain/posdao-test-setup#41 (comment)

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@jam10o-new jam10o-new added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Jul 25, 2019
@vkomenda vkomenda force-pushed the poanetwork-clear-engine-signer branch from 443825f to 64d6cb5 Compare August 27, 2019 10:02
@vkomenda
Copy link
Contributor Author

Please review.

ethcore/engine/src/engine.rs Outdated Show resolved Hide resolved
@afck afck force-pushed the poanetwork-clear-engine-signer branch from 2851b0d to 74fcb11 Compare October 1, 2019 14:05
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Apologies for the late re-review. I think we're real close!

@@ -133,6 +133,9 @@ pub trait MinerService : Send + Sync {
/// On chains where sealing is done externally (e.g. PoW) we provide only reward beneficiary.
fn set_author(&self, author: Author);

/// Clears the engine signer and stops signing.
fn clear_author(&self);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think the same "trick" would work here too? I.e. avoid adding a new trait method and instead change set_author to take an Option<Author> instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.
I made it a T: Into<Option<Author>>, so that you can pass in just an Author, and we don't have to update all the call sites.
Also, I added *self.signer.write() = None to MinerService::set_author, which seemed like an omission. Or am I misreading this?

Copy link
Contributor

Choose a reason for hiding this comment

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

(And: Greetings from Solna! 🙂 🇸🇪)

@dvdplm dvdplm requested a review from niklasad1 October 4, 2019 17:27
@afck afck force-pushed the poanetwork-clear-engine-signer branch from 74fcb11 to 58ddaee Compare October 8, 2019 10:48
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm

@dvdplm
Copy link
Collaborator

dvdplm commented Oct 8, 2019

@niklasad1 can you take a look at this as well please?

Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM, except the removed log in clique::set_signer

@afck afck force-pushed the poanetwork-clear-engine-signer branch from 58ddaee to 869c3b3 Compare October 8, 2019 14:38
@afck
Copy link
Contributor

afck commented Oct 8, 2019

Another test failure I can't reproduce locally.

@niklasad1
Copy link
Collaborator

@afck Yeah, it's a CI issue. Restarted and it succeeded.

@niklasad1 niklasad1 added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Oct 8, 2019
@niklasad1 niklasad1 added the A8-looksgood 🦄 Pull request is reviewed well. label Oct 8, 2019
@afck afck force-pushed the poanetwork-clear-engine-signer branch from e6715b1 to 2249974 Compare October 8, 2019 18:51
@dvdplm dvdplm merged commit 93fbbb9 into openethereum:master Oct 9, 2019
@niklasad1 niklasad1 added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Oct 9, 2019
@afck afck deleted the poanetwork-clear-engine-signer branch October 9, 2019 14:58
dvdplm added a commit that referenced this pull request Oct 10, 2019
* master:
  Secret store: fix Instant::now() related race in net_keep_alive (#11155)
  RPC method for clearing the engine signer (#10920)
  Use TryFrom instead of From+panic for Builtin (#11140)
afck pushed a commit to poanetwork/parity-ethereum that referenced this pull request Oct 22, 2019
Original commit messages:

RPC method parity_clearEngineSigner

Add RPC method parity_clearEngineSigner
Fixes #113

corrected the return type of clear_author

review comment responses and a rebase fix

removed a spurrious warning

moved clear_signer functionality to set_signer

Merge clear_author into MinerService::set_author.

Add trace logs to Clique::set_signer.

Clique: Don't lock signer multiple times.
varasev pushed a commit to poanetwork/parity-ethereum that referenced this pull request Oct 23, 2019
Original commit messages:

RPC method parity_clearEngineSigner

Add RPC method parity_clearEngineSigner
Fixes #113

corrected the return type of clear_author

review comment responses and a rebase fix

removed a spurrious warning

moved clear_signer functionality to set_signer

Merge clear_author into MinerService::set_author.

Add trace logs to Clique::set_signer.

Clique: Don't lock signer multiple times.
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. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants