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.

Sign up for GitHub

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

core::crypto: features cleanup in tests #1983

Closed
wants to merge 6 commits into from
Closed

core::crypto: features cleanup in tests #1983

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
df41de0
core::crypto: features cleanup
michalkucharczyk Oct 23, 2023
11708ad
core::crypto tests added to CI
michalkucharczyk Oct 23, 2023
4b0111f
zepter fmt
michalkucharczyk Oct 23, 2023
e3fa4a5
test -> check
michalkucharczyk Oct 23, 2023
aac6793
Merge remote-tracking branch 'origin/master' into mku-keyring-in-runtime
michalkucharczyk Oct 23, 2023
01b0bdd
sp-runtime-interface/disable_target_static_assertions
michalkucharczyk Oct 23, 2023
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
17 changes: 17 additions & 0 deletions .gitlab/pipeline/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,23 @@ test-linux-stable-slow:
# --features=runtime-benchmarks,runtime-metrics,try-runtime --
# --skip upgrade_version_checks_should_work

test-core-crypto-features:
stage: test
extends:
- .docker-env
- .common-refs
- .run-immediately
- .pipeline-stopper-artifacts
variables:
RUST_TOOLCHAIN: stable
# Enable debug assertions since we are running optimized builds for testing
# but still want to have debug assertions.
RUSTFLAGS: "-Cdebug-assertions=y -Dwarnings"
script:
- time cargo test --release -p sp-core --no-default-features --features="full_crypto"
- time cargo test --release -p sp-core --no-default-features --features="full_crypto,serde"
- time cargo test --release -p sp-core --no-default-features --features="serde"
michalkucharczyk marked this conversation as resolved.
Show resolved Hide resolved

test-doc:
stage: test
extends:
Expand Down
1 change: 1 addition & 0 deletions substrate/primitives/core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ serde = [
"scale-info/serde",
"secrecy/alloc",
"sp-core-hashing",
"sp-runtime-interface/disable_target_static_assertions",
Copy link
Contributor Author

@michalkucharczyk michalkucharczyk Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This probably should not be here. However without this compilation fails:

error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
  --> substrate/primitives/runtime-interface/src/impls.rs:45:1
   |
45 | assert_eq_size!(usize, u32);
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: source type: `usize` (64 bits)
   = note: target type: `u32` (32 bits)
   = note: this error originates in the macro `assert_eq_size` (in Nightly builds, run with -Z macro-backtrace for more info)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is wrong and needs to be removed!

Copy link
Member

Choose a reason for hiding this comment

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

Substrate currently assumed that no_std == arch::wasm32. Shitty, I know, but that is currently the assumption.

Copy link
Contributor

@koute koute Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, ideally that assert should be changed to check for wasm32 and not for no_std, and then you won't need to disable it.

...as Basti said, the current assumption everywhere is that no_std equals WASM, which, uh, I guess I'll have to fix soon-ish since I'll be wanting to get the RISC-V based runtimes working. :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall we remove this one too?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah good idea.

"sp-storage/serde",
]

Expand Down
37 changes: 33 additions & 4 deletions substrate/primitives/core/src/crypto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1223,21 +1223,38 @@ impl_from_entropy_base!(u8, u16, u32, u64, u128, i8, i16, i32, i64, i128);
#[cfg(test)]
mod tests {
use super::*;
use crate::DeriveJunction;
#[cfg(feature = "full_crypto")]
use crate::crypto::DeriveJunction;
#[cfg(feature = "full_crypto")]
use sp_std::prelude::ToOwned;
#[cfg(not(feature = "std"))]
use sp_std::vec;

#[derive(Clone, Eq, PartialEq, Debug)]
enum TestPair {
Generated,
#[cfg(feature = "std")]
GeneratedWithPhrase,
GeneratedFromPhrase { phrase: String, password: Option<String> },
Standard { phrase: String, password: Option<String>, path: Vec<DeriveJunction> },
#[cfg(feature = "std")]
GeneratedFromPhrase {
phrase: String,
password: Option<String>,
},
#[cfg(feature = "std")]
Standard {
phrase: String,
password: Option<String>,
path: Vec<DeriveJunction>,
},
#[cfg(feature = "full_crypto")]
Seed(Vec<u8>),
}
impl Default for TestPair {
fn default() -> Self {
TestPair::Generated
}
}
#[cfg(feature = "full_crypto")]
impl CryptoType for TestPair {
type Pair = Self;
}
Expand All @@ -1262,6 +1279,7 @@ mod tests {
}
}
impl CryptoType for TestPublic {
#[cfg(feature = "full_crypto")]
type Pair = TestPair;
}
impl Derive for TestPublic {}
Expand All @@ -1282,19 +1300,23 @@ mod tests {
}
}
impl Public for TestPublic {}
#[cfg(feature = "full_crypto")]
impl Pair for TestPair {
type Public = TestPublic;
type Seed = [u8; 8];
type Signature = [u8; 0];

#[cfg(feature = "std")]
fn generate() -> (Self, <Self as Pair>::Seed) {
(TestPair::Generated, [0u8; 8])
}

#[cfg(feature = "std")]
fn generate_with_phrase(_password: Option<&str>) -> (Self, String, <Self as Pair>::Seed) {
(TestPair::GeneratedWithPhrase, "".into(), [0u8; 8])
}

#[cfg(feature = "std")]
fn from_phrase(
phrase: &str,
password: Option<&str>,
Expand All @@ -1308,18 +1330,21 @@ mod tests {
))
}

#[cfg(feature = "full_crypto")]
fn derive<Iter: Iterator<Item = DeriveJunction>>(
&self,
path_iter: Iter,
_: Option<[u8; 8]>,
) -> Result<(Self, Option<[u8; 8]>), DeriveError> {
Ok((
match self.clone() {
#[cfg(feature = "std")]
TestPair::Standard { phrase, password, path } => TestPair::Standard {
phrase,
password,
path: path.into_iter().chain(path_iter).collect(),
},
#[cfg(feature = "std")]
TestPair::GeneratedFromPhrase { phrase, password } =>
TestPair::Standard { phrase, password, path: path_iter.collect() },
x =>
Expand Down Expand Up @@ -1355,6 +1380,7 @@ mod tests {
}

#[test]
#[cfg(feature = "std")]
fn interpret_std_seed_should_work() {
assert_eq!(
TestPair::from_string("0x0123456789abcdef", None),
Expand All @@ -1363,6 +1389,7 @@ mod tests {
}

#[test]
#[cfg(feature = "std")]
fn password_override_should_work() {
assert_eq!(
TestPair::from_string("hello world///password", None),
Expand All @@ -1375,6 +1402,7 @@ mod tests {
}

#[test]
#[cfg(feature = "std")]
fn interpret_std_secret_string_should_work() {
assert_eq!(
TestPair::from_string("hello world", None),
Expand Down Expand Up @@ -1475,8 +1503,9 @@ mod tests {
}

#[test]
#[cfg(feature = "std")]
fn accountid_32_from_str_works() {
use std::str::FromStr;
use sp_std::str::FromStr;
assert!(AccountId32::from_str("5G9VdMwXvzza9pS8qE8ZHJk3CheHW9uucBn9ngW4C1gmmzpv").is_ok());
assert!(AccountId32::from_str(
"5c55177d67b064bb5d189a3e1ddad9bc6646e02e64d6e308f5acbb1533ac430d"
Expand Down
10 changes: 6 additions & 4 deletions substrate/primitives/core/src/defer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl<F: FnOnce()> Drop for DeferGuard<F> {
/// ```rust
/// use sp_core::defer;
///
/// let message = std::cell::RefCell::new("".to_string());
/// let message = sp_std::cell::RefCell::new("".to_string());
/// {
/// defer!(
/// message.borrow_mut().push_str("world!");
Expand Down Expand Up @@ -82,7 +82,7 @@ mod test {
#[test]
/// `defer` executes the code in reverse order of being called.
fn defer_guard_order_works() {
let called = std::cell::RefCell::new(1);
let called = sp_std::cell::RefCell::new(1);

defer!(
assert_eq!(*called.borrow(), 3);
Expand All @@ -101,7 +101,7 @@ mod test {
#[allow(unused_braces)]
#[allow(clippy::unnecessary_operation)]
fn defer_guard_syntax_works() {
let called = std::cell::RefCell::new(0);
let called = sp_std::cell::RefCell::new(0);
{
defer!(*called.borrow_mut() += 1);
defer!(*called.borrow_mut() += 1;); // With ;
Expand All @@ -113,6 +113,7 @@ mod test {

#[test]
/// `defer` executes the code even in case of a panic.
#[cfg(feature = "std")]
fn defer_guard_panic_unwind_works() {
use std::panic::{catch_unwind, AssertUnwindSafe};
let mut called = false;
Expand All @@ -128,9 +129,10 @@ mod test {

#[test]
/// `defer` executes the code even in case another `defer` panics.
#[cfg(feature = "std")]
fn defer_guard_defer_panics_unwind_works() {
use std::panic::{catch_unwind, AssertUnwindSafe};
let counter = std::cell::RefCell::new(0);
let counter = sp_std::cell::RefCell::new(0);

let should_panic = catch_unwind(AssertUnwindSafe(|| {
defer!(*counter.borrow_mut() += 1);
Expand Down
34 changes: 28 additions & 6 deletions substrate/primitives/core/src/ecdsa.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,13 +535,20 @@ impl CryptoType for Pair {
#[cfg(test)]
mod test {
use super::*;
use crate::crypto::{
set_default_ss58_version, PublicError, Ss58AddressFormat, Ss58AddressFormatRegistry,
DEV_PHRASE,
};

#[cfg(feature = "std")]
use crate::crypto::set_default_ss58_version;
#[cfg(feature = "std")]
use crate::crypto::DEV_PHRASE;
#[cfg(all(feature = "serde", feature = "full_crypto"))]
use crate::crypto::{PublicError, Ss58AddressFormat, Ss58AddressFormatRegistry};
#[cfg(feature = "serde")]
use serde_json;
#[cfg(feature = "full_crypto")]
use sp_std::vec;

#[test]
#[cfg(feature = "std")]
fn default_phrase_should_be_used() {
assert_eq!(
Pair::from_string("//Alice///password", None).unwrap().public(),
Expand All @@ -552,6 +559,7 @@ mod test {
}

#[test]
#[cfg(feature = "full_crypto")]
fn seed_and_derive_should_work() {
let seed = array_bytes::hex2array_unchecked(
"9d61b19deffd5a60ba844af492ec2cc44449c5697b326919703bac031cae7f60",
Expand All @@ -569,6 +577,7 @@ mod test {
}

#[test]
#[cfg(feature = "std")]
Copy link
Member

Choose a reason for hiding this comment

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

As we only run tests in std why do we need all this feature gating?

Copy link
Contributor Author

@michalkucharczyk michalkucharczyk Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. I want e.g. time cargo test --release -p sp-core --no-default-features --features="serde" to be executed correctly.

Copy link
Member

Choose a reason for hiding this comment

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

But you don't run this anymore? And serde is mainly about testing that it compiles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to run it locally. I want to have ability to test the sp-core with the different feature sets it offers. serde was just an example, full-crypto or full-crypto,serde are the other options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have trait methods definition under std, so w/o this paricular gate you will get:

cargo +nightly check --tests --no-default-features --features="full_crypto"

error[E0599]: no function or associated item named `from_full` found for struct `ecdsa::Public` in the current scope
   --> substrate/primitives/core/src/ecdsa.rs:588:12
    |
80  | pub struct Public(pub [u8; PUBLIC_KEY_SERIALIZED_SIZE]);
    | ----------------- function or associated item `from_full` not found for this struct
...
588 |             Public::from_full(
    |                     ^^^^^^^^^ function or associated item not found in `Public`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `sp-core` (lib test) due to previous error

Copy link
Member

Choose a reason for hiding this comment

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

I want to run it locally. I want to have ability to test the sp-core with the different feature sets it offers. serde was just an example, full-crypto or full-crypto,serde are the other options.

These features are not changing anything in the implementation, they only unlock different kind of functions etc. When you run everything using std you test the logic. We also don't have logic in there that is different based on features. So, not sure what you want to test there.

Copy link
Member

Choose a reason for hiding this comment

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

We have trait methods definition under std, so w/o this paricular gate you will get:

cargo +nightly check --tests --no-default-features --features="full_crypto"

Because you are using --tests. Just drop this, these tests only need to work on std.

Copy link
Contributor Author

@michalkucharczyk michalkucharczyk Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

IMO the tests should be runnable for any set of features provided by this module. The point of this PR was to check if tests are fine for particular features (or combinations of them).

If we always want to test with default features, then this PR does not make sense.

Copy link
Member

Choose a reason for hiding this comment

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

I mean checking that it compiles make sense. (without the tests). To ensure that these features compile on their own.

fn test_vector_should_work() {
let pair = Pair::from_seed(&array_bytes::hex2array_unchecked(
"9d61b19deffd5a60ba844af492ec2cc44449c5697b326919703bac031cae7f60",
Expand All @@ -588,6 +597,7 @@ mod test {
}

#[test]
#[cfg(feature = "std")]
fn test_vector_by_string_should_work() {
let pair = Pair::from_string(
"0x9d61b19deffd5a60ba844af492ec2cc44449c5697b326919703bac031cae7f60",
Expand All @@ -609,6 +619,7 @@ mod test {
}

#[test]
#[cfg(feature = "std")]
fn generated_pair_should_work() {
let (pair, _) = Pair::generate();
let public = pair.public();
Expand All @@ -619,6 +630,7 @@ mod test {
}

#[test]
#[cfg(feature = "std")]
fn seeded_pair_should_work() {
let pair = Pair::from_seed(b"12345678901234567890123456789012");
let public = pair.public();
Expand All @@ -630,12 +642,12 @@ mod test {
);
let message = array_bytes::hex2bytes_unchecked("2f8c6129d816cf51c374bc7f08c3e63ed156cf78aefb4a6550d97b87997977ee00000000000000000200d75a980182b10ab7d54bfed3c964073a0ee172f3daa62325af021a68f707511a4500000000000000");
let signature = pair.sign(&message[..]);
println!("Correct signature: {:?}", signature);
assert!(Pair::verify(&signature, &message[..], &public));
assert!(!Pair::verify(&signature, "Other message", &public));
}

#[test]
#[cfg(feature = "std")]
fn generate_with_phrase_recovery_possible() {
let (pair1, phrase, _) = Pair::generate_with_phrase(None);
let (pair2, _) = Pair::from_phrase(&phrase, None).unwrap();
Expand All @@ -644,6 +656,7 @@ mod test {
}

#[test]
#[cfg(feature = "std")]
fn generate_with_password_phrase_recovery_possible() {
let (pair1, phrase, _) = Pair::generate_with_phrase(Some("password"));
let (pair2, _) = Pair::from_phrase(&phrase, Some("password")).unwrap();
Expand All @@ -652,6 +665,7 @@ mod test {
}

#[test]
#[cfg(feature = "std")]
fn password_does_something() {
let (pair1, phrase, _) = Pair::generate_with_phrase(Some("password"));
let (pair2, _) = Pair::from_phrase(&phrase, None).unwrap();
Expand All @@ -660,16 +674,17 @@ mod test {
}

#[test]
#[cfg(all(feature = "full_crypto", feature = "serde"))]
fn ss58check_roundtrip_works() {
let pair = Pair::from_seed(b"12345678901234567890123456789012");
let public = pair.public();
let s = public.to_ss58check();
println!("Correct: {}", s);
let cmp = Public::from_ss58check(&s).unwrap();
assert_eq!(cmp, public);
}

#[test]
#[cfg(all(feature = "full_crypto", feature = "serde"))]
fn ss58check_format_check_works() {
let pair = Pair::from_seed(b"12345678901234567890123456789012");
let public = pair.public();
Expand All @@ -679,6 +694,7 @@ mod test {
}

#[test]
#[cfg(all(feature = "full_crypto", feature = "serde"))]
fn ss58check_full_roundtrip_works() {
let pair = Pair::from_seed(b"12345678901234567890123456789012");
let public = pair.public();
Expand All @@ -696,6 +712,7 @@ mod test {
}

#[test]
#[cfg(feature = "std")]
fn ss58check_custom_format_works() {
// We need to run this test in its own process to not interfere with other tests running in
// parallel and also relying on the ss58 version.
Expand Down Expand Up @@ -730,6 +747,7 @@ mod test {
}

#[test]
#[cfg(all(feature = "full_crypto", feature = "serde"))]
fn signature_serialization_works() {
let pair = Pair::from_seed(b"12345678901234567890123456789012");
let message = b"Something important";
Expand All @@ -742,6 +760,7 @@ mod test {
}

#[test]
#[cfg(feature = "serde")]
fn signature_serialization_doesnt_panic() {
fn deserialize_signature(text: &str) -> Result<Signature, serde_json::error::Error> {
serde_json::from_str(text)
Expand All @@ -753,6 +772,7 @@ mod test {
}

#[test]
#[cfg(feature = "std")]
fn sign_prehashed_works() {
let (pair, _, _) = Pair::generate_with_phrase(Some("password"));

Expand All @@ -777,6 +797,7 @@ mod test {
}

#[test]
#[cfg(feature = "std")]
fn verify_prehashed_works() {
let (pair, _, _) = Pair::generate_with_phrase(Some("password"));

Expand All @@ -791,6 +812,7 @@ mod test {
}

#[test]
#[cfg(feature = "std")]
fn recover_prehashed_works() {
let (pair, _, _) = Pair::generate_with_phrase(Some("password"));

Expand Down
Loading