Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade libsecp256k1 from 0.3.5 ➔ 0.7.0 #967

Merged
merged 22 commits into from
Oct 22, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
[workspace]
resolver = "2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the new cargo feature resolver required for this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, so the issue is that cargo's old resolver does not handle features in dependencies correctly. In our case we have the libsecp256k1 crate as (1) a dependency for non-std and (2) as a dep for the off-chain engines. Both cases require a different feature set, of which some features are not compatible to non-std.

The old feature resolver doesn't handle this properly and merges the features, thus resulting in errors a la error: duplicate lang item in crate std (which secp256k1 depends on): panic_impl.

But since we'll migrate to the 2021 edition soon this will be obsolete anyway (because resolver = "2" is the default there).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be unnecessary since we are using edition 2021 now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shouldn't this be unnecessary since we are using edition 2021 now?

Absolutely my assumption as well, but unfortunately not true: rust-lang/cargo#9956.

If you are using a [virtual workspace], you will still need to explicitly set the [resolver field] in the [workspace] definition if you want to opt-in to the new resolver.

members = [
"crates/metadata",
"crates/allocator",
Expand Down
2 changes: 1 addition & 1 deletion crates/engine/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ sha3 = { version = "0.9" }
blake2 = { version = "0.9" }

# ECDSA for the off-chain environment.
libsecp256k1 = { version = "0.3.5", default-features = false }
secp256k1 = { version = "0.20.3", features = ["recovery"] }

[features]
default = ["std"]
Expand Down
29 changes: 18 additions & 11 deletions crates/engine/src/ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -429,10 +429,12 @@ impl Engine {
output: &mut [u8; 33],
) -> Result {
use secp256k1::{
recover,
recovery::{
RecoverableSignature,
RecoveryId,
},
Message,
RecoveryId,
Signature,
Secp256k1,
};

// In most implementations, the v is just 0 or 1 internally, but 27 was added
Expand All @@ -442,17 +444,22 @@ impl Engine {
} else {
signature[64]
};
let message = Message::parse(message_hash);
let signature = Signature::parse_slice(&signature[0..64])
.unwrap_or_else(|error| panic!("Unable to parse the signature: {}", error));

let recovery_id = RecoveryId::parse(recovery_byte)
let recovery_id = RecoveryId::from_i32(recovery_byte as i32)
.unwrap_or_else(|error| panic!("Unable to parse the recovery id: {}", error));

let pub_key = recover(&message, &signature, &recovery_id);
let message = Message::from_slice(message_hash).unwrap_or_else(|error| {
panic!("Unable to create the message from hash: {}", error)
});
let signature =
RecoverableSignature::from_compact(&signature[0..64], recovery_id)
.unwrap_or_else(|error| {
panic!("Unable to parse the signature: {}", error)
});

let secp = Secp256k1::new();
let pub_key = secp.recover(&message, &signature);
match pub_key {
Ok(pub_key) => {
*output = pub_key.serialize_compressed();
*output = pub_key.serialize();
Ok(())
}
Err(_) => Err(Error::EcdsaRecoverFailed),
Expand Down
82 changes: 82 additions & 0 deletions crates/engine/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ use crate::ext::{
Engine,
Error,
};
use secp256k1::{
recovery::RecoverableSignature,
Message,
PublicKey,
Secp256k1,
SecretKey,
};
use std::convert::TryInto;

/// The public methods of the `contracts` pallet write their result into an
/// `output` buffer instead of returning them. Since we aim to emulate this
Expand Down Expand Up @@ -192,3 +200,77 @@ fn must_panic_when_buffer_too_small() {
// then
unreachable!("`get_storage` must already have panicked");
}

#[test]
fn ecdsa_recovery_test_from_contracts_pallet() {
// given
let mut engine = Engine::new();
#[rustfmt::skip]
let signature: [u8; 65] = [
161, 234, 203, 74, 147, 96, 51, 212, 5, 174, 231, 9, 142, 48, 137, 201,
162, 118, 192, 67, 239, 16, 71, 216, 125, 86, 167, 139, 70, 7, 86, 241,
33, 87, 154, 251, 81, 29, 160, 4, 176, 239, 88, 211, 244, 232, 232, 52,
211, 234, 100, 115, 230, 47, 80, 44, 152, 166, 62, 50, 8, 13, 86, 175,
28,
];
#[rustfmt::skip]
let message_hash: [u8; 32] = [
162, 28, 244, 179, 96, 76, 244, 178, 188, 83, 230, 248, 143, 106, 77, 117,
239, 95, 244, 171, 65, 95, 62, 153, 174, 166, 182, 28, 130, 73, 196, 208
];

// when
let mut output = [0; 33];
engine
.ecdsa_recover(&signature, &message_hash, &mut output)
.expect("must work");

// then
#[rustfmt::skip]
const EXPECTED_COMPRESSED_PUBLIC_KEY: [u8; 33] = [
2, 121, 190, 102, 126, 249, 220, 187, 172, 85, 160, 98, 149, 206, 135, 11,
7, 2, 155, 252, 219, 45, 206, 40, 217, 89, 242, 129, 91, 22, 248, 23,
152,
];
assert_eq!(output, EXPECTED_COMPRESSED_PUBLIC_KEY);
}

#[test]
fn ecdsa_recovery_with_secp256k1_crate() {
// given
let mut engine = Engine::new();
let secp = Secp256k1::new();
let seckey = [
59, 148, 11, 85, 134, 130, 61, 253, 2, 174, 59, 70, 27, 180, 51, 107, 94, 203,
174, 253, 102, 39, 170, 146, 46, 252, 4, 143, 236, 12, 136, 28,
];
let pubkey = PublicKey::from_slice(&[
2, 29, 21, 35, 7, 198, 183, 43, 14, 208, 65, 139, 14, 112, 205, 128, 231, 245,
41, 91, 141, 134, 245, 114, 45, 63, 82, 19, 251, 210, 57, 79, 54,
])
.expect("pubkey creation failed");

let mut msg_hash = [0; 32];
crate::hashing::sha2_256(b"Some message", &mut msg_hash);

let msg = Message::from_slice(&msg_hash).expect("message creation failed");
let seckey = SecretKey::from_slice(&seckey).expect("secret key creation failed");
let recoverable_signature: RecoverableSignature =
secp.sign_recoverable(&msg, &seckey);

let recovery_id = recoverable_signature.serialize_compact().0.to_i32() as u8;
let mut signature = recoverable_signature.serialize_compact().1.to_vec();
signature.push(recovery_id);
let signature_with_recovery_id: [u8; 65] = signature
.try_into()
.expect("unable to create signature with recovery id");

// when
let mut output = [0; 33];
engine
.ecdsa_recover(&signature_with_recovery_id, msg.as_ref(), &mut output)
.expect("ecdsa recovery failed");

// then
assert_eq!(output, pubkey.serialize());
}
12 changes: 7 additions & 5 deletions crates/env/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ categories = ["no-std", "embedded"]
include = ["Cargo.toml", "src/**/*.rs", "README.md", "LICENSE"]

[dependencies]
ink_engine = { version = "3.0.0-rc6", path = "../engine/", default-features = false, optional = true }
ink_metadata = { version = "3.0.0-rc6", path = "../metadata/", default-features = false, features = ["derive"], optional = true }
ink_allocator = { version = "3.0.0-rc6", path = "../allocator/", default-features = false }
ink_primitives = { version = "3.0.0-rc6", path = "../primitives/", default-features = false }
Expand All @@ -30,18 +29,21 @@ arrayref = "0.3"
static_assertions = "1.1"
sp-arithmetic = { version = "3.0", default-features = false }

[target.'cfg(target_arch = "wasm32")'.dependencies]
secp256k1 = { version = "0.20.3", default-features = false }

[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
ink_engine = { version = "3.0.0-rc6", path = "../engine/", default-features = false, optional = true }

# Hashes for the off-chain environment.
sha2 = { version = "0.9", optional = true }
sha3 = { version = "0.9", optional = true }
blake2 = { version = "0.9", optional = true }

# ECDSA for the off-chain environment.
libsecp256k1 = { version = "0.3.5", default-features = false }
secp256k1 = { version = "0.20.3", features = ["recovery"] }

# Only used in the off-chain environment.
#
# Sadly couldn't be marked as dev-dependency.
# Never use this crate outside the off-chain environment!
rand = { version = "0.8", default-features = false, features = ["alloc"], optional = true }
scale-info = { version = "1.0", default-features = false, features = ["derive"], optional = true }

Expand Down
29 changes: 18 additions & 11 deletions crates/env/src/engine/experimental_off_chain/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,10 +256,12 @@ impl EnvBackend for EnvInstance {
output: &mut [u8; 33],
) -> Result<()> {
use secp256k1::{
recover,
recovery::{
RecoverableSignature,
RecoveryId,
},
Message,
RecoveryId,
Signature,
Secp256k1,
};

// In most implementations, the v is just 0 or 1 internally, but 27 was added
Expand All @@ -269,17 +271,22 @@ impl EnvBackend for EnvInstance {
} else {
signature[64]
};
let message = Message::parse(message_hash);
let signature = Signature::parse_slice(&signature[0..64])
.unwrap_or_else(|error| panic!("Unable to parse the signature: {}", error));

let recovery_id = RecoveryId::parse(recovery_byte)
let recovery_id = RecoveryId::from_i32(recovery_byte as i32)
.unwrap_or_else(|error| panic!("Unable to parse the recovery id: {}", error));

let pub_key = recover(&message, &signature, &recovery_id);
let message = Message::from_slice(message_hash).unwrap_or_else(|error| {
panic!("Unable to create the message from hash: {}", error)
});
let signature =
RecoverableSignature::from_compact(&signature[0..64], recovery_id)
.unwrap_or_else(|error| {
panic!("Unable to parse the signature: {}", error)
});

let secp = Secp256k1::new();
let pub_key = secp.recover(&message, &signature);
match pub_key {
Ok(pub_key) => {
*output = pub_key.serialize_compressed();
*output = pub_key.serialize();
Ok(())
}
Err(_) => Err(crate::Error::EcdsaRecoverFailed),
Expand Down
31 changes: 19 additions & 12 deletions crates/env/src/engine/off_chain/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,10 +202,12 @@ impl EnvBackend for EnvInstance {
output: &mut [u8; 33],
) -> Result<()> {
use secp256k1::{
recover,
recovery::{
RecoverableSignature,
RecoveryId,
},
Message,
RecoveryId,
Signature,
Secp256k1,
};

// In most implementations, the v is just 0 or 1 internally, but 27 was added
Expand All @@ -215,20 +217,25 @@ impl EnvBackend for EnvInstance {
} else {
signature[64]
};
let message = Message::parse(message_hash);
let signature = Signature::parse_slice(&signature[0..64])
.unwrap_or_else(|error| panic!("Unable to parse the signature: {}", error));

let recovery_id = RecoveryId::parse(recovery_byte)
let recovery_id = RecoveryId::from_i32(recovery_byte as i32)
.unwrap_or_else(|error| panic!("Unable to parse the recovery id: {}", error));

let pub_key = recover(&message, &signature, &recovery_id);
let message = Message::from_slice(message_hash).unwrap_or_else(|error| {
panic!("Unable to create the message from hash: {}", error)
});
let signature =
RecoverableSignature::from_compact(&signature[0..64], recovery_id)
.unwrap_or_else(|error| {
panic!("Unable to parse the signature: {}", error)
});

let secp = Secp256k1::new();
let pub_key = secp.recover(&message, &signature);
match pub_key {
Ok(pub_key) => {
*output = pub_key.serialize_compressed();
*output = pub_key.serialize();
Ok(())
}
Err(_) => Err(Error::EcdsaRecoverFailed),
Err(_) => Err(crate::Error::EcdsaRecoverFailed),
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/eth_compatibility/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ include = ["Cargo.toml", "src/**/*.rs", "/README.md", "/LICENSE"]

[dependencies]
ink_env = { version = "3.0.0-rc6", path = "../env", default-features = false }
libsecp256k1 = { version = "0.3.5", default-features = false }
secp256k1 = { version = "0.20.3", default-features = false }

[features]
default = ["std"]
Expand Down
5 changes: 3 additions & 2 deletions crates/eth_compatibility/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

#![no_std]

use ink_env::{
DefaultEnvironment,
Environment,
Expand Down Expand Up @@ -95,9 +96,9 @@ impl ECDSAPublicKey {
use secp256k1::PublicKey;

// Transform compressed public key into uncompressed.
let pub_key = PublicKey::parse_compressed(&self.0)
let pub_key = PublicKey::from_slice(&self.0)
.expect("Unable to parse the compressed ECDSA public key");
let uncompressed = pub_key.serialize();
let uncompressed = pub_key.serialize_uncompressed();

// Hash the uncompressed public key by Keccak256 algorithm.
let mut hash = <hash::Keccak256 as hash::HashOutput>::Type::default();
Expand Down
1 change: 1 addition & 0 deletions crates/storage/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ scale-info = { version = "1.0", default-features = false, features = ["derive"],
cfg-if = "1.0"
array-init = { version = "2.0", default-features = false }

[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
# Workaround: we actually just need criterion as a dev-dependency, but
# there is an issue with a doubly included std lib when executing
# `cargo check --no-default-features --target wasm32-unknown-unknown`.
Expand Down
1 change: 1 addition & 0 deletions examples/contract-terminate/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name = "contract_terminate"
version = "3.0.0-rc6"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2018"
resolver = "2"

[dependencies]
ink_primitives = { version = "3.0.0-rc6", path = "../../crates/primitives", default-features = false }
Expand Down
1 change: 1 addition & 0 deletions examples/contract-transfer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name = "contract_transfer"
version = "3.0.0-rc6"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2018"
resolver = "2"

[dependencies]
ink_primitives = { version = "3.0.0-rc6", path = "../../crates/primitives", default-features = false }
Expand Down
1 change: 1 addition & 0 deletions examples/delegator/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name = "delegator"
version = "3.0.0-rc6"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2018"
resolver = "2"

[dependencies]
ink_primitives = { version = "3.0.0-rc6", path = "../../crates/primitives", default-features = false }
Expand Down
1 change: 1 addition & 0 deletions examples/delegator/accumulator/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name = "accumulator"
version = "3.0.0-rc6"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2018"
resolver = "2"

[dependencies]
ink_primitives = { version = "3.0.0-rc6", path = "../../../crates/primitives", default-features = false }
Expand Down
1 change: 1 addition & 0 deletions examples/delegator/adder/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name = "adder"
version = "3.0.0-rc6"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2018"
resolver = "2"

[dependencies]
ink_primitives = { version = "3.0.0-rc6", path = "../../../crates/primitives", default-features = false }
Expand Down
1 change: 1 addition & 0 deletions examples/delegator/subber/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name = "subber"
version = "3.0.0-rc6"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2018"
resolver = "2"

[dependencies]
ink_primitives = { version = "3.0.0-rc6", path = "../../../crates/primitives", default-features = false }
Expand Down
1 change: 1 addition & 0 deletions examples/dns/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name = "dns"
version = "3.0.0-rc6"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2018"
resolver = "2"

[dependencies]
ink_primitives = { version = "3.0.0-rc6", path = "../../crates/primitives", default-features = false }
Expand Down
1 change: 1 addition & 0 deletions examples/erc1155/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name = "erc1155"
version = "3.0.0-rc6"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2018"
resolver = "2"

[dependencies]
ink_primitives = { version = "3.0.0-rc6", path = "../../crates/primitives", default-features = false }
Expand Down
1 change: 1 addition & 0 deletions examples/erc20/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name = "erc20"
version = "3.0.0-rc6"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2018"
resolver = "2"

[dependencies]
ink_primitives = { version = "3.0.0-rc6", path = "../../crates/primitives", default-features = false }
Expand Down
Loading