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 all 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
2 changes: 1 addition & 1 deletion .rustfmt.toml
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ trailing_comma = "Vertical"
match_block_trailing_comma = false
blank_lines_upper_bound = 1
blank_lines_lower_bound = 0
edition = "2018" # changed
edition = "2021" # changed
Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch!

version = "One"
merge_derives = true
use_try_shorthand = true # changed
Expand Down
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 }
libsecp256k1 = { version = "0.7.0" }

[features]
default = ["std"]
Expand Down
4 changes: 2 additions & 2 deletions crates/engine/src/ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ impl Engine {
message_hash: &[u8; 32],
output: &mut [u8; 33],
) -> Result {
use secp256k1::{
use libsecp256k1::{
recover,
Message,
RecoveryId,
Expand All @@ -443,7 +443,7 @@ impl Engine {
signature[64]
};
let message = Message::parse(message_hash);
let signature = Signature::parse_slice(&signature[0..64])
let signature = Signature::parse_standard_slice(&signature[0..64])
.unwrap_or_else(|error| panic!("Unable to parse the signature: {}", error));

let recovery_id = RecoveryId::parse(recovery_byte)
Expand Down
76 changes: 76 additions & 0 deletions crates/engine/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ use crate::ext::{
Engine,
Error,
};
use libsecp256k1::{
Message,
PublicKey,
SecretKey,
};

/// 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 +197,74 @@ 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 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::parse_compressed(&[
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::parse(&msg_hash);
let seckey = SecretKey::parse(&seckey).expect("secret key creation failed");
let (signature, recovery_id) = libsecp256k1::sign(&msg, &seckey);

let mut signature = signature.serialize().to_vec();
signature.push(recovery_id.serialize());
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.serialize(), &mut output)
.expect("ecdsa recovery failed");

// then
assert_eq!(output, pubkey.serialize_compressed());
}
9 changes: 7 additions & 2 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,13 +29,19 @@ arrayref = "0.3"
static_assertions = "1.1"
sp-arithmetic = { version = "3.0", default-features = false }

[target.'cfg(target_arch = "wasm32")'.dependencies]
libsecp256k1 = { version = "0.7.0", 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 }
libsecp256k1 = { version = "0.7.0" }

# Only used in the off-chain environment.
#
Expand Down
7 changes: 4 additions & 3 deletions crates/env/src/engine/experimental_off_chain/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use crate::{
Clear,
EnvBackend,
Environment,
Error,
RentParams,
RentStatus,
Result,
Expand Down Expand Up @@ -252,7 +253,7 @@ impl EnvBackend for EnvInstance {
message_hash: &[u8; 32],
output: &mut [u8; 33],
) -> Result<()> {
use secp256k1::{
use libsecp256k1::{
recover,
Message,
RecoveryId,
Expand All @@ -267,7 +268,7 @@ impl EnvBackend for EnvInstance {
signature[64]
};
let message = Message::parse(message_hash);
let signature = Signature::parse_slice(&signature[0..64])
let signature = Signature::parse_standard_slice(&signature[0..64])
.unwrap_or_else(|error| panic!("Unable to parse the signature: {}", error));

let recovery_id = RecoveryId::parse(recovery_byte)
Expand All @@ -279,7 +280,7 @@ impl EnvBackend for EnvInstance {
*output = pub_key.serialize_compressed();
Ok(())
}
Err(_) => Err(crate::Error::EcdsaRecoverFailed),
Err(_) => Err(Error::EcdsaRecoverFailed),
}
}

Expand Down
4 changes: 2 additions & 2 deletions crates/env/src/engine/off_chain/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ impl EnvBackend for EnvInstance {
message_hash: &[u8; 32],
output: &mut [u8; 33],
) -> Result<()> {
use secp256k1::{
use libsecp256k1::{
recover,
Message,
RecoveryId,
Expand All @@ -216,7 +216,7 @@ impl EnvBackend for EnvInstance {
signature[64]
};
let message = Message::parse(message_hash);
let signature = Signature::parse_slice(&signature[0..64])
let signature = Signature::parse_standard_slice(&signature[0..64])
.unwrap_or_else(|error| panic!("Unable to parse the signature: {}", error));

let recovery_id = RecoveryId::parse(recovery_byte)
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 }
libsecp256k1 = { version = "0.7.0", default-features = false }

[features]
default = ["std"]
Expand Down
3 changes: 2 additions & 1 deletion 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 @@ -92,7 +93,7 @@ impl ECDSAPublicKey {
/// ```
pub fn to_eth_address(&self) -> EthereumAddress {
use ink_env::hash;
use secp256k1::PublicKey;
use libsecp256k1::PublicKey;

// Transform compressed public key into uncompressed.
let pub_key = PublicKey::parse_compressed(&self.0)
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