Use parity-crypto updated to use upstream rust-secp256k1 #11406

dvdplm · 2020-01-24T14:31:35Z

This PR contains the changes needed to switch away from paritys fork of rust-secp256k1. The PR is very noisy but the most relevant change is the addition of a free function to invert Secrets, used only by secret-storein math.rs. That function uses the libsecp256k1 crate to perform the inversion because the rust-secp256k1 have made clear they do not want to expose that operation in their public API.

This PR requires paritytech/parity-common#258 to be merged and v0.3.5 of libsecp256k1 to be published.

dvdplm · 2020-01-29T13:20:19Z

accounts/ethkey/src/brain_prefix.rs


-	fn generate(&mut self) -> Result<KeyPair, Error> {
+	pub fn generate(&mut self) -> Result<KeyPair, Error> {


Moved this to a free function as it needs to return an error, whereas the Generate trait changed (it can't fail now).

dvdplm · 2020-01-29T13:23:42Z

accounts/ethstore/src/account/safe_account.rs

 		let password = "hello world".into();
-		let message = Message::default();
+		let message = [1u8; 32].into();


In upstream rust-secp256k1, a Message cannot be all zeroes anymore. This is one big reason why this PR is so noisy: we have a lot of code that use the Default trait to initialize values.

dvdplm · 2020-01-29T13:27:10Z

secret-store/src/key_server_cluster/math.rs

@@ -28,6 +28,12 @@ pub struct EncryptedSecret {
 	pub encrypted_point: Public,
 }

+/// Calculate the inversion of a Secret key (in place) using the `libsecp256k1` crate.


This is probably the most contentious change in the PR: in order to use the upstream secp lib we need a way to calculate the inversion. Pulling in a second secp256k1 crate to do that isn't very elegant.

as long as we can make libsecp256k1 optional under secretstore feature it's fine

secret-store/src/key_server_cluster/cluster.rs

…pstream * master: Add POSDAO transition and malice report queue. (#11245) update master/nightly version: v2.8.0 (#11419) ethcore/res: remove morden testnet (#11392) fix: export hardcoded sync format (#11416) update hardcoded headers: mainnet and ropsten (#11414) AuthorityEngine: Minor cleanups. (#11408) Update POA bootnodes (#11411) Add EtherCore support (#11402) verification: fix race same block + misc (#11400) Update ProgPoW to 0.9.3 (#11407) update classic testnet bootnodes (#11398) dependencies: bump `derive_more v0.99` (#11405) engine error: remove faulty/unused `From` (#11404) Switching to stable-track (#11377) ethcore/res: fix ethereum classic chainspec blake2_f activation block num (#11391) Update copyright notice 2020 (#11386)

…pstream * master: Avoid long state queries when serving GetNodeData requests (#11444) Cargo.lock: cargo update -p kvdb-rocksdb (#11447) rlp_derive: cleanup (#11446) chore: remove unused dependencies (#11432) update kvdb-rocksdb to 0.4 (#11442) Rough architecutre diagram. (#11396) ethjson: impl Copy for hash type wrapper (#11423) Remove dead bootnodes, add new geth bootnodes (#11441) goerli: replace foundation bootnode (#11433) Update publish-docker.sh (#11428) Revert "[Trace] Distinguish between `create` and `create2` (#11311)" (#11427)

…pstream * master: upgrade some of the dependencies (#11467) Some more release track changes to README.md (#11465) Update simple one-line installer due to switching to a single stable release track (#11463) update Dockerfile (#11461) Implement EIP-2124 (#11456) [eth classic chainspec]: remove `balance = 1` (#11459) just to make sure we don't screw up this again (#11455) backwards compatible call_type creation_method (#11450) gcc to clang (#11453) Avoid copies (#11451) Cargo.lock: cargo lock translate (#11448)

secret-store/Cargo.toml

secret-store/src/types/error.rs

Use libsecp256k1 0.3.5

dvdplm · 2020-02-08T20:35:47Z

util/panic-hook/Cargo.toml

@@ -7,4 +7,4 @@ version = "0.1.0"
 authors = ["Parity Technologies <admin@parity.io>"]

 [dependencies]
-backtrace = "0.3.2"
+backtrace = "0.3.43"


I had weird problems with the lockfile and the cc crate: rust-secp256k1 has a hard requirement on cc@1.0.41 which caused cascading trouble that is visible here. I can try to revert this change if anyone feels it's risky/irrelevant.

ordian · 2020-02-08T21:01:54Z

Cargo.lock

@@ -4013,6 +4002,25 @@ dependencies = [
 "winapi 0.3.8",
 ]

+[[package]]
+name = "rand"
+version = "0.6.5"


ordian

LGTM!

sorpaas

LGTM!

* master: Use parity-crypto updated to use upstream rust-secp256k1 (#11406) Cleanup some code in Aura (#11466) upgrade some of the dependencies (#11467) Some more release track changes to README.md (#11465) Update simple one-line installer due to switching to a single stable release track (#11463) update Dockerfile (#11461) Implement EIP-2124 (#11456)

After https://github.com/openethereum/openethereum/pull/11406 it is no longer possible to to public key recovery from messages that are all-zero. This create issues when using the `ecrecover` builtin because externally produced signatures may well provide a message (i.e. a preimage) that is all-zeroes. This works around the problem at the cost of cloning the incoming message and create a `ZeroesAllowedMessage` wrapper around it. The `ZeroesAllowedMessage` implements the `ThirtyTwoByteHash` trait from `rust-secp256k1` which circumvents the zero-check. In a follow-up PR we'll likely change the interface of `recover()` to take a `ZeroesAllowedMessage` directly, thus removing the unneeded clone.

* Allow pubkey recovery for all-zero messages After https://github.com/openethereum/openethereum/pull/11406 it is no longer possible to to public key recovery from messages that are all-zero. This create issues when using the `ecrecover` builtin because externally produced signatures may well provide a message (i.e. a preimage) that is all-zeroes. This works around the problem at the cost of cloning the incoming message and create a `ZeroesAllowedMessage` wrapper around it. The `ZeroesAllowedMessage` implements the `ThirtyTwoByteHash` trait from `rust-secp256k1` which circumvents the zero-check. In a follow-up PR we'll likely change the interface of `recover()` to take a `ZeroesAllowedMessage` directly, thus removing the unneeded clone. * Inner doesn't need to be pub * Update parity-crypto/src/publickey/ecdsa_signature.rs Co-Authored-By: Andronik Ordian <write@reusable.software> * Docs and review grumbles * Add `recover_allowing_all_zero_message()` Revert `recover()` to previous behaviour: no zero-messages allowed Docs and cleanup * Obey the fmt Co-authored-by: Andronik Ordian <write@reusable.software>

Use parity-crypto updated to use upstream rust-secp256k1

093d113

dvdplm self-assigned this Jan 24, 2020

dvdplm added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jan 24, 2020

dvdplm added 2 commits January 24, 2020 15:49

Fetch dependency from git

79b921a

Missed a session ID

d817001

dvdplm mentioned this pull request Jan 29, 2020

[parity-crypto] Use upstream secp256k1 paritytech/parity-common#258

Merged

Add free-standing inversion function that uses libsecp256k1

802ff59

dvdplm commented Jan 29, 2020

View reviewed changes

secret-store/src/key_server_cluster/cluster.rs Outdated Show resolved Hide resolved

dvdplm commented Jan 29, 2020

View reviewed changes

secret-store/src/key_server_cluster/cluster.rs Outdated Show resolved Hide resolved

dvdplm requested a review from svyatonik January 29, 2020 13:33

dvdplm added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jan 29, 2020

fixed tests

ea3ee75

dvdplm marked this pull request as ready for review January 29, 2020 15:36

dvdplm added 4 commits January 29, 2020 16:37

Update deps

4f810dc

ordian reviewed Feb 8, 2020

View reviewed changes

secret-store/Cargo.toml Outdated Show resolved Hide resolved

ordian reviewed Feb 8, 2020

View reviewed changes

secret-store/src/types/error.rs Outdated Show resolved Hide resolved

Use parity-crypto 0.5.0

b806a0c

Use libsecp256k1 0.3.5

dvdplm commented Feb 8, 2020

View reviewed changes

Review grumble

1213474

ordian reviewed Feb 8, 2020

View reviewed changes

ordian approved these changes Feb 8, 2020

View reviewed changes

ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 8, 2020

ordian added M5-dependencies 🖇 Dependencies. M4-core ⛓ Core client code / Rust. labels Feb 8, 2020

sorpaas approved these changes Feb 10, 2020

View reviewed changes

sorpaas merged commit 99271db into master Feb 10, 2020

sorpaas deleted the dp/chore/use-latest-attempt-to-switch-to-upstream branch February 10, 2020 17:29

dvdplm mentioned this pull request Apr 9, 2020

Allow pubkey recovery for all-zero messages paritytech/parity-common#369

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use parity-crypto updated to use upstream rust-secp256k1 #11406

Use parity-crypto updated to use upstream rust-secp256k1 #11406

dvdplm commented Jan 24, 2020 •

edited

Loading

dvdplm Jan 29, 2020

dvdplm Jan 29, 2020

dvdplm Jan 29, 2020

ordian Feb 8, 2020

dvdplm Feb 8, 2020

ordian Feb 8, 2020

dvdplm Feb 8, 2020

ordian left a comment

sorpaas left a comment


		fn generate(&mut self) -> Result<KeyPair, Error> {
		pub fn generate(&mut self) -> Result<KeyPair, Error> {

Use parity-crypto updated to use upstream rust-secp256k1 #11406

Use parity-crypto updated to use upstream rust-secp256k1 #11406

Conversation

dvdplm commented Jan 24, 2020 • edited Loading

dvdplm Jan 29, 2020

Choose a reason for hiding this comment

dvdplm Jan 29, 2020

Choose a reason for hiding this comment

dvdplm Jan 29, 2020

Choose a reason for hiding this comment

ordian Feb 8, 2020

Choose a reason for hiding this comment

dvdplm Feb 8, 2020

Choose a reason for hiding this comment

ordian Feb 8, 2020

Choose a reason for hiding this comment

dvdplm Feb 8, 2020

Choose a reason for hiding this comment

ordian left a comment

Choose a reason for hiding this comment

sorpaas left a comment

Choose a reason for hiding this comment

dvdplm commented Jan 24, 2020 •

edited

Loading