`core::crypto`: features cleanup in tests #1983

michalkucharczyk · 2023-10-23T10:24:30Z

There are three features that affect how crypto module is compiled: std, full_crypto and serde.

Following invocations are now failing:

cargo test --no-default-features --features="full_crypto"
cargo test --no-default-features --features="full_crypto,serde"
cargo test --no-default-features --features="serde"
cargo test --no-default-features

This PR adds the features to the test, so these commands are working properly.

Some groundwork for: #1984

michalkucharczyk · 2023-10-23T10:45:56Z

substrate/primitives/core/Cargo.toml

@@ -132,6 +132,7 @@ serde = [
 	"secrecy/alloc",
 	"sp-core-hashing",
 	"sp-storage/serde",
+	"sp-runtime-interface/disable_target_static_assertions",


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)

Yeah this is wrong and needs to be removed!

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

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

Shall we remove this one too?

Yeah good idea.

burdges · 2023-10-23T12:17:25Z

Is serde still relevant?

michalkucharczyk · 2023-10-23T12:53:06Z

Is serde still relevant?

At the moment: yes.

We had some discussion on removing this feature and inclusion of serde dependency unconditionally, but we didn't reach a conclusion.

.gitlab/pipeline/test.yml

bkchr · 2023-10-23T19:58:03Z

substrate/primitives/core/src/ecdsa.rs

@@ -569,6 +577,7 @@ mod test {
 	}

 	#[test]
+	#[cfg(feature = "std")]


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

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

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

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.

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

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.

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.

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.

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

* removed FromBridgedChainMessageDispatch in favor of XcmBlobMessageDispatch * use HaulBlobExporter/HaulBlobExporterAdapter instead of XcmBridge/XcmBridgeAdapter * tests for sending/dispatching messages * use new schema in testnet bridges + some cleanup * clippy * spelling + added TODO * cleanup some checks * benchmarks compilation * all is XCM * updated README.md * ref issue from TODO

core::crypto: features cleanup

df41de0

michalkucharczyk changed the title ~~core::crypto: features cleanup in tests~~ core::crypto: features cleanup in tests Oct 23, 2023

michalkucharczyk requested review from davxy and a team October 23, 2023 10:24

michalkucharczyk added the T0-node This PR/Issue is related to the topic “node”. label Oct 23, 2023

michalkucharczyk commented Oct 23, 2023

View reviewed changes

michalkucharczyk added 2 commits October 23, 2023 13:01

core::crypto tests added to CI

11708ad

zepter fmt

4b0111f

michalkucharczyk requested a review from a team as a code owner October 23, 2023 11:01

paritytech-ci requested a review from a team October 23, 2023 11:03

bkchr reviewed Oct 23, 2023

View reviewed changes

.gitlab/pipeline/test.yml Outdated Show resolved Hide resolved

michalkucharczyk added 3 commits October 23, 2023 15:09

test -> check

e3fa4a5

Merge remote-tracking branch 'origin/master' into mku-keyring-in-runtime

aac6793

sp-runtime-interface/disable_target_static_assertions

01b0bdd

bkchr reviewed Oct 23, 2023

View reviewed changes

michalkucharczyk closed this Oct 24, 2023

This was referenced Jun 5, 2024

Update polkadot-sdk from v1.7.0 to v1.11.0 moondance-labs/tanssi#573

Closed

Update polkadot-sdk from v1.10.0 to v1.11.0 moondance-labs/tanssi#577

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`core::crypto`: features cleanup in tests #1983

`core::crypto`: features cleanup in tests #1983

michalkucharczyk commented Oct 23, 2023 •

edited

Loading

michalkucharczyk Oct 23, 2023 •

edited

Loading

bkchr Oct 23, 2023

bkchr Oct 23, 2023

koute Oct 23, 2023 •

edited

Loading

michalkucharczyk Oct 23, 2023

bkchr Oct 23, 2023

burdges commented Oct 23, 2023

michalkucharczyk commented Oct 23, 2023 •

edited

Loading

bkchr Oct 23, 2023

michalkucharczyk Oct 23, 2023 •

edited

Loading

bkchr Oct 23, 2023

michalkucharczyk Oct 23, 2023

michalkucharczyk Oct 23, 2023

bkchr Oct 24, 2023

bkchr Oct 24, 2023

michalkucharczyk Oct 24, 2023 •

edited

Loading

bkchr Oct 24, 2023

core::crypto: features cleanup in tests #1983

core::crypto: features cleanup in tests #1983

Conversation

michalkucharczyk commented Oct 23, 2023 • edited Loading

michalkucharczyk Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koute Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burdges commented Oct 23, 2023

michalkucharczyk commented Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

michalkucharczyk Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkucharczyk Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`core::crypto`: features cleanup in tests #1983

`core::crypto`: features cleanup in tests #1983

michalkucharczyk commented Oct 23, 2023 •

edited

Loading

michalkucharczyk Oct 23, 2023 •

edited

Loading

koute Oct 23, 2023 •

edited

Loading

michalkucharczyk commented Oct 23, 2023 •

edited

Loading

michalkucharczyk Oct 23, 2023 •

edited

Loading

michalkucharczyk Oct 24, 2023 •

edited

Loading