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

Remove excessive warning #10831

Merged
merged 1 commit into from
Jul 2, 2019
Merged

Remove excessive warning #10831

merged 1 commit into from
Jul 2, 2019

Conversation

grbIzl
Copy link
Collaborator

@grbIzl grbIzl commented Jul 2, 2019

This warning is a excessive, because calling sign method of EngineSigner with incorrect password is legit situation. For example, it's being done on the start in order to define proper engine-signer account (all passwords are looped through and tried for signing test message, as result warnings about all incorrect passwords are shown in the console).
Also unifies the logic of EngineSigner because all other components in this layer (rpc/v1/helpers) silently return the result or error.

Relates to #10695

@grbIzl grbIzl added M4-core ⛓ Core client code / Rust. A0-pleasereview 🤓 Pull request needs code review. labels Jul 2, 2019
@dvdplm
Copy link
Collaborator

dvdplm commented Jul 2, 2019

Did you git blame to see why it was added in the first place? Also: would s/warn!/trace!/ improve things?

@grbIzl
Copy link
Collaborator Author

grbIzl commented Jul 2, 2019

Did you git blame to see why it was added in the first place?

Yes, indeed. This warning was introduced by @tomusdrw in his #10213 Before it we tried to sign test message using direct sign call from AccountProvider:

in run.rs:
...
if !passwords.iter().any(|p| miner.set_author(engine_signer, Some(p.to_owned())).is_ok()) {
return Err(format!("No valid password for the consensus signer {}. {}", engine_signer,
VERIFY_PASSWORD_HINT));
...
in miner.rs:
...
fn set_author(&self, address: Address, password: Option) -> Result<(), AccountError> {
self.params.write().author = address;

if self.engine.seals_internally().is_some() && password.is_some() {
	if let Some(ref ap) = self.accounts {
		let password = password.unwrap_or_else(|| Password::from(String::new()));
		// Sign test message
		ap.sign(address.clone(), Some(password.clone()), Default::default())?;

...

@tomusdrw created EngineSigner in RPC in order to communicate with accounts through it and used its sign. I don't know, why he added a global warning in sign method. All account provider methods and RPC helpers just return error and let the caller log it if needed.

Also: would s/warn!/trace!/ improve things?

Obviously it would, but IMO it doesn't make sense for such component to log anything. See before, IMO the user of this component should take care about logging the error. And we have such approach for all other components.

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.

Looks good, but can we make sure we have a warning when we actually try to use EngineSigner from within Engine when new blocks are being sealed?

@grbIzl
Copy link
Collaborator Author

grbIzl commented Jul 2, 2019

Looks good, but can we make sure we have a warning when we actually try to use EngineSigner from within Engine when new blocks are being sealed?

You mean this warning: https://github.com/paritytech/parity-ethereum/blob/master/ethcore/src/engines/authority_round/mod.rs#L1196 ?

@dvdplm dvdplm merged commit 5f064a9 into master Jul 2, 2019
@dvdplm dvdplm deleted the RemoveWarnMessage branch July 2, 2019 15:52
@ordian ordian added this to the 2.6 milestone Jul 2, 2019
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 2, 2019
ordian added a commit that referenced this pull request Jul 3, 2019
* master:
  idiomatic changes to PodState (#10834)
  Allow --nat extip:your.host.here.org (#10830)
  When updating the client or when called from RPC, sleep should mean sleep (#10814)
  Remove excessive warning (#10831)
  Fix typo in README.md (#10828)
  ethcore does not use byteorder (#10829)
dvdplm added a commit that referenced this pull request Jul 4, 2019
…me-parent

* master:
  refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812)
  Break circular dependency between Client and Engine (part 1) (#10833)
  tests: Relates to #10655: Test instructions for Readme (#10835)
  refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)
  idiomatic changes to PodState (#10834)
  Allow --nat extip:your.host.here.org (#10830)
  When updating the client or when called from RPC, sleep should mean sleep (#10814)
  Remove excessive warning (#10831)
  Fix typo in README.md (#10828)
  ethcore does not use byteorder (#10829)
  Better logging when backfilling ancient blocks fail (#10796)
  depends: Update wordlist to v1.3 (#10823)
dvdplm added a commit that referenced this pull request Jul 4, 2019
* master:
  refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812)
  Break circular dependency between Client and Engine (part 1) (#10833)
  tests: Relates to #10655: Test instructions for Readme (#10835)
  refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)
  idiomatic changes to PodState (#10834)
  Allow --nat extip:your.host.here.org (#10830)
  When updating the client or when called from RPC, sleep should mean sleep (#10814)
  Remove excessive warning (#10831)
  Fix typo in README.md (#10828)
  ethcore does not use byteorder (#10829)
  Better logging when backfilling ancient blocks fail (#10796)
  depends: Update wordlist to v1.3 (#10823)
  cargo update -p smallvec (#10822)
  replace memzero with zeroize crate (#10816)
  Don't repeat the logic from Default impl (#10813)
  removed additional_params method (#10818)
  Add Constantinople eips to the dev (instant_seal) config (#10809)
dvdplm added a commit that referenced this pull request Jul 4, 2019
* master: (21 commits)
  refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812)
  Break circular dependency between Client and Engine (part 1) (#10833)
  tests: Relates to #10655: Test instructions for Readme (#10835)
  refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)
  idiomatic changes to PodState (#10834)
  Allow --nat extip:your.host.here.org (#10830)
  When updating the client or when called from RPC, sleep should mean sleep (#10814)
  Remove excessive warning (#10831)
  Fix typo in README.md (#10828)
  ethcore does not use byteorder (#10829)
  Better logging when backfilling ancient blocks fail (#10796)
  depends: Update wordlist to v1.3 (#10823)
  cargo update -p smallvec (#10822)
  replace memzero with zeroize crate (#10816)
  Don't repeat the logic from Default impl (#10813)
  removed additional_params method (#10818)
  Add Constantinople eips to the dev (instant_seal) config (#10809)
  removed redundant fmt::Display implementations (#10806)
  revert changes to .gitlab-ci.yml (#10807)
  Add filtering capability to `parity_pendingTransactions` (issue 8269) (#10506)
  ...
@grbIzl grbIzl mentioned this pull request Jul 8, 2019
@jam10o-new jam10o-new added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Aug 2, 2019
@jam10o-new jam10o-new modified the milestones: 2.6, 2.7 Aug 2, 2019
@jam10o-new
Copy link
Contributor

Added patch labels and bumping milestone because this missed being backported

s3krit pushed a commit that referenced this pull request Aug 12, 2019
@s3krit s3krit mentioned this pull request Aug 12, 2019
s3krit added a commit that referenced this pull request Aug 12, 2019
  -  Fix cargo audit (#10921)
  - Add support for Energy Web Foundation's new chains (#10957)
  - Kaspersky AV whitelisting (#10919)
  - Avast whitelist script (#10900)
  - Docker images renaming (#10863)
  - Remove excessive warning (#10831)
  - Allow --nat extip:your.host.here.org (#10830)
  - When updating the client or when called from RPC, sleep should mean sleep (#10814)
  - added new ropsten-bootnode and removed old one (#10794)
  - ethkey no longer uses byteorder (#10786)
  - Do not drop the peer with None difficulty (#10772)
  - docs: Update Readme with TOC, Contributor Guideline. Update Cargo package descriptions (#10652)
@s3krit s3krit mentioned this pull request Aug 14, 2019
s3krit added a commit that referenced this pull request Aug 14, 2019
  * Add support for Energy Web Foundation's new chains (#10957)                                                                                                                                  
  * Kaspersky AV whitelisting (#10919)                                                                                                                                                           
  * Avast whitelist script (#10900)                                                                                                                                                              
  * Docker images renaming (#10863)                                                                                                                                                              
  * Remove excessive warning (#10831)                                                                                                                                                            
  * Allow --nat extip:your.host.here.org (#10830)                                                                                                                                                
  * When updating the client or when called from RPC, sleep should mean sleep (#10814)                                                                                                           
  * added new ropsten-bootnode and removed old one (#10794)                                                      
  * ethkey no longer uses byteorder (#10786)                                                            
  * docs: Update Readme with TOC, Contributor Guideline. Update Cargo package descriptions (#10652)
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. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants