-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[EngineSigner]: don't sign message with only zeroes #11524
Conversation
Fixes #11521, caused by switching to `upstream rust-secp256k1`
debug!(target: "account", "Signing test of `EngineSigner ({})` with password index: {} failed because of: {:?}", engine_signer, idx, e); | ||
invalid_reasons.insert(e.to_string()); | ||
} else { | ||
return Ok(Some(ethcore::miner::Author::Sealer(Box::new(signer)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once we have found a valid password then return it directly, no need to iterate over the rest.
|
||
Ok(author) | ||
Err(format!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will look like:
No valid password found for EngineSigner 0x5100…cae4, the following errors were found during testing: {"custom crypto error: invalid AES message", "custom crypto error: Invalid password"}. Make sure valid password is present in files passed using `--password` or in the configuration file.
Err(_) => Err(Error::InvalidSecretKey), | ||
} | ||
self.accounts.sign(self.address, Some(self.password.clone()), message).map_err(|e| { | ||
Error::Custom(e.to_string()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
propagate real error cause instead of making assumptions.
} | ||
|
||
fn decrypt(&self, auth_data: &[u8], cipher: &[u8]) -> Result<Vec<u8>, Error> { | ||
self.accounts.decrypt(self.address, None, auth_data, cipher).map_err(|e| { | ||
warn!("Unable to decrypt message: {:?}", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this, because it is better that the caller handles this error accordingly. Thoughts?
author = Some(::ethcore::miner::Author::Sealer(Box::new(signer))); | ||
if let Err(e) = signer.sign(SECP_TEST_MESSAGE) { | ||
debug!(target: "account", "Signing test of `EngineSigner ({})` with password index: {} failed because of: {:?}", engine_signer, idx, e); | ||
invalid_reasons.insert(e.to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parity_crypto::public::Error
doesn't impl Eq
, that's why it is converted to a String
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should? :)
* master: (27 commits) Faster kill_garbage (#11514) [EngineSigner]: don't sign message with only zeroes (#11524) fix compilation warnings (#11522) [ethcore cleanup]: various unrelated fixes from `#11493` (#11507) Add benchmark for transaction execution (#11509) Add Smart Contract License v1.0 Misc fixes (#11510) [dependencies]: unify `rustc-hex` (#11506) Activate on-chain randomness in POA Sokol (#11505) Grab bag of cleanup (#11504) Implement eth/64 (EIP-2364) and drop support for eth/62 (#11472) [dependencies]: remove `util/macros` (#11501) OpenEthereum bootnodes are added (#11499) [ci benches]: use `all-features` (#11496) [verification]: make test-build compile standalone (#11495) complete null-signatures removal (#11491) Include the seal when populating the header for a new block (#11475) fix compilation warnings (#11492) cargo update -p cmake (#11490) update to published rlp-derive (#11489) ...
* master: Code cleanup in the sync module (#11552) initial cleanup (#11542) Warn if genesis constructor revert (#11550) ethcore: cleanup after #11531 (#11546) license update (#11543) Less cloning when importing blocks (#11531) Github Actions (#11528) Fix Alpine Dockerfile (#11538) Remove AuxiliaryData/AuxiliaryRequest (#11533) [journaldb]: cleanup (#11534) Remove references to parity-ethereum (#11525) Drop IPFS support (#11532) chain-supplier: fix warning reporting for GetNodeData request (#11530) Faster kill_garbage (#11514) [EngineSigner]: don't sign message with only zeroes (#11524)
Fixes #11521
At startup the
engine_signer(s)
are tested that they can signdummy data
, previously only zeroes were signed to check this. After switching toupstream rust-secp256k1
signing only zeroes doesn't work which this PR fixes.