Expand `no_std` availability of primitives and add `serde` feature flag

[haerdib]

Adds serde feature flag to the primitives. This way, serde derivations can also be used in no_std environments. The following crates now implement the serde flag:

• sp-application-crypto
• sp-arithmetic
• sp-beefy
• sp-consensus-babe
• sp-consensus-slots
• sp-core
• sp-finality-grandpa
• sp-keystore
• sp-mmr-primitives
• sp-npos-elections
• sp-runtime
• sp-storage
• sp-test-primitives
• sp-version
• sp-weights

The following crates are now (partially) no_std compatible:

• sp-keystore
• sp-rpc
• sp-serializer

The following crates now implement the full_crypto feature:

• sp-runtime
• sp-finality-grandpa

Note: Neither sp-rpc nor sp-serializer implement the serde flag. Reasoning: both crates will most likely only be used if serde derivations are needed.

closes https://github.com/paritytech/substrate/issues/12994

[cla-bot-2021[bot]]

User @haerdib, please sign the CLA here.

[haerdib]

@bkchr Any news on this? I'll happily split the PR, keep it up to date.. if there's a chance, this can go through.

[stale[bot]]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

[michalkucharczyk]

LGTM. I left some minor comments.

@haerdib Would you be willing to continue work on this (rebase)? I already did some work here: https://github.com/michalkucharczyk/substrate/commit/17a7981af31b680a231465a610f11ea93b7301ae, https://github.com/michalkucharczyk/substrate/commit/218f1e8fb35addf0df68322151512ab316686877. But I am not sure how I can share it.

[michalkucharczyk]

So does this PR still have a chance of being merged eventually?

I believe so :) This is a foundation for paritytech/polkadot-sdk#25

[haerdib]

I believe so :) This is a foundation for https://github.com/paritytech/polkadot-sdk/issues/25

Awesome! I'll do the rebase by the end of this week then. I'll ping you once finished.

[michalkucharczyk]

Thank you for rebasing. Despite my Derive functionality doubts, the PR looks good.

Note: you can locally git merge origin/master, resolve conflicts and push the merge commit to the PR. This avoids forced pushes, and is preferred way of rebasing.

[michalkucharczyk]

If now the serde crate is enabled with no-std friendly features (alloc and derive) why introduce an extra feature to unlock it? That is. Why hide it behind a feature?

We probably should move this discussion into original issue, not this PR: https://github.com/paritytech/substrate/issues/12994

Assuming that my measurements are correct, the size impact of enabling serde (and using serialization in GenesisConfig) in substrate-test-runtime is as follows:

928K May  9 12:29 ./substrate-test-runtime-with-genesis-config-builder/substrate_test_runtime.compact.wasm
779K May  9 12:33 ./substrate-test-runtime-without-genesis-config-builder/substrate_test_runtime.compact.wasm

Number of funcs: 1178 vs 986.

I don't know if the size of the binary impacts the performance of wasm runtime. I would say not significantly (@koute any comments?)

(note: The check that serde is not enabled in wasm was: wasm-objdump -x file.wasm | grep serde)

edit: one more thought - theoretically linker should remove unused code. if serde functionality is not used, its code (also genereted) should not be included.

[davxy]

edit: one more thought - theoretically linker should remove unused code. if serde functionality is not used, its code (also genereted) should not be included.

This was my main point. If this is an artifact size motivation then if one doesn't call any serde related thing than the binary should not contain it. Also if I call function A then an optimized binary should contain only A (and what it uses internally) and nothing else (at least this is what I expect from C Rust etc. Not sure about wasm... but I bet the same applies).

Is possible that the size difference you have pointed out is due to some code that is in our primitive crates that is included when we enable serde feature and that also has a fallback "dummy" implementation when serde is not enabled. (because should be code that is always referenced by us)

[koute]

Assuming that my measurements are correct, the size impact of enabling serde (and using serialization in GenesisConfig) in substrate-test-runtime is as follows:

Did you use the production profile? As that's what we use for the production artifacts, and on the release profile LTO isn't as effective so the binary might be needlessly bigger.

It should be also possible to diff the two binaries to see why exactly they got bigger.

I don't know if the size of the binary impacts the performance of wasm runtime. I would say not significantly (@koute any comments?)

In theory it could (e.g. initial JIT compilation can take longer, etc.), but unless the increase is really substantial I wouldn't worry about it.

[michalkucharczyk]

It should be also possible to diff the two binaries to see why exactly they got bigger.

Binary got bigger because of serde functions pulled in which are used by GenesisConfig (on which I am working on my branch).

What I did:

• in Cargo.toml serde is enabled for both builds,
• the bigger binary has suport for GenesisConfig serialization
• the smaller binary does not have support for GenesisConfig thus serde staff wasn't linked in.

So I would say that the sizes are as expected, no surprise here :)

I used debug profile.

[koute]

I used debug profile.

In that case, comparing the size of the binary under the debug profile is pretty much pointless. (: Always use the production profile for comparisons.

[bkchr]

In that case, comparing the size of the binary under the debug profile is pretty much pointless. (: Always use the production profile for comparisons.

WASM_BUILDER builds in release mode any way by default, if you not force it to use debug ;P

If this is the case, well maybe the user has access to more expensive stuff anyway and nothing prevents him to indirectly allocate using 3rd party crates which are not so "explicit" about warning the user.

Can't we just remove the serde feature and keep it simple? :-)

This is a valid and understandable point. This pr was opened quite some time ago and procrastinated by me :see_no_evil: The pr was opened without the direction we may want to use it for in mind. As long as @michalkucharczyk isn't finished and we are 100% sure on the direction I would say we keep the serde feature. Later, we can revisit when @michalkucharczyk lands the pr he is working on. It will not be that hard to remove the feature afterwards.

[koute]

WASM_BUILDER builds in release mode any way by default, if you not force it to use debug ;P

Ah right, good point! I completely forgot about that. (:

[bkchr]

In general we have done a lot like not using the default Debug implementation without doing any real scientific approach of checking on the size impact and that the linker removes all the things as we would expect it. I think this is something for some greater discussion and some issue to work on over time to check all these things etc. However, you already see today that downstream isn't using features like disable-logging to remove all logging functionality to bring down the binary size. The next question would also be, what is the upper binary size limit we would like to support and why. A lot of questions :see_no_evil:

[michalkucharczyk]

We may need to merge paritytech/master into haerdib:add-serde-feature-to-primitives to make CI green. Normally bot rebase should do the trick. Not sure if this works for non-paritytech branches.

[haerdib]

We may need to merge paritytech/master into haerdib:add-serde-feature-to-primitives to make CI green. Normally bot rebase should do the trick. Not sure if this works for non-paritytech branches.

Merged

[michalkucharczyk]

@haerdib we are almost there :)

In order to merge we need to make CI green, some small problems may still appear there. Would you please merge master again, and update Cargo.lock to enable CI flows?

[haerdib]

Any idea what this errors means? https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2842595

error: failed to run custom build command for `substrate-test-runtime v2.0.0 (/builds/parity/mirrors/substrate/test-utils/runtime)`
[2023-05-16 09:02:26] note: To improve backtraces for build dependencies, set the CARGO_PROFILE_DEV_BUILD_OVERRIDE_DEBUG=true environment variable to enable debug information generation.
[2023-05-16 09:02:26] 
[2023-05-16 09:02:26] Caused by:
[2023-05-16 09:02:26]   process didn't exit successfully: `/cargo_target_dir/debug/build/substrate-test-runtime-10d4078345484e7d/build-script-build` (exit status: 101)
[2023-05-16 09:02:26]   --- stderr
[2023-05-16 09:02:26]   thread 'main' panicked at '`cargo metadata` can not fail on project `Cargo.toml`; qed: CargoMetadata { stderr: " Downloading crates ...\n  Downloaded anstyle-wincon v1.0.1\n  Downloaded ark-serialize v0.4.2\n  Downloaded core-foundation v0.9.3\n  Downloaded derivative v2.2.0\n  Downloaded mach v0.3.2\n  Downloaded system-configuration v0.5.
[...]
 Downloaded windows_i686_gnu v0.34.0\nwarning: spurious network error (2 tries remaining): failed to get successful HTTP response from `https://crates.io/api/v1/crates/ark-bls12-381/0.4.0/download`, got 502\nbody:\n<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.01 Transitional//EN\" \"http://www.w3.org/TR/html4/loose.dtd\">\n<HTML><HEAD><META HTTP-EQUIV=\"Content-Type\" CONTENT=\"text/html; charset=iso-8859-1\">\n<TITLE>ERROR: The request could not be satisfied</TITLE>\n</HEAD><BODY>\n<H1>502 ERROR</H1>\n<H2>The request could not be satisfied.
[...]

[michalkucharczyk]

Any idea what this errors means? https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2842595

error: failed to run custom build command for `substrate-test-runtime v2.0.0 (/builds/parity/mirrors/substrate/test-utils/runtime)`
[2023-05-16 09:02:26] note: To improve backtraces for build dependencies, set the CARGO_PROFILE_DEV_BUILD_OVERRIDE_DEBUG=true environment variable to enable debug information generation.
[2023-05-16 09:02:26] 
[2023-05-16 09:02:26] Caused by:
[2023-05-16 09:02:26]   process didn't exit successfully: `/cargo_target_dir/debug/build/substrate-test-runtime-10d4078345484e7d/build-script-build` (exit status: 101)
[2023-05-16 09:02:26]   --- stderr
[2023-05-16 09:02:26]   thread 'main' panicked at '`cargo metadata` can not fail on project `Cargo.toml`; qed: CargoMetadata { stderr: " Downloading crates ...\n  Downloaded anstyle-wincon v1.0.1\n  Downloaded ark-serialize v0.4.2\n  Downloaded core-foundation v0.9.3\n  Downloaded derivative v2.2.0\n  Downloaded mach v0.3.2\n  Downloaded system-configuration v0.5.
[...]
 Downloaded windows_i686_gnu v0.34.0\nwarning: spurious network error (2 tries remaining): failed to get successful HTTP response from `https://crates.io/api/v1/crates/ark-bls12-381/0.4.0/download`, got 502\nbody:\n<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.01 Transitional//EN\" \"http://www.w3.org/TR/html4/loose.dtd\">\n<HTML><HEAD><META HTTP-EQUIV=\"Content-Type\" CONTENT=\"text/html; charset=iso-8859-1\">\n<TITLE>ERROR: The request could not be satisfied</TITLE>\n</HEAD><BODY>\n<H1>502 ERROR</H1>\n<H2>The request could not be satisfied.
[...]

This was some internal problem. I retriggered it and it is green: https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines/278469

The last problematic one is: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2842577

[michalkucharczyk]

(sorry I accidentally close)

[michalkucharczyk]

@haerdib thanks for moving this forward. Seems that we need one more origin/master merge due to little bug that was fixed in github flows: https://github.com/paritytech/substrate/pull/14161

[haerdib]

Seems that we need one more origin/master merge due to little bug that was fixed in github flows: https://github.com/paritytech/substrate/pull/14161

Finally passed 👍

[bkchr]

/tip medium

[substrate-tip-bot[bot]]

@bkchr Contributor did not properly post their account address.

Make sure the pull request description has: "{network} address: {address}".

[bkchr]

@bkchr Contributor did not properly post their account address.

Make sure the pull request description has: "{network} address: {address}".

@haerdib pleae do what the bot says and ping me when you have done it :D

[haerdib]

@bkchr thanks a lot for the tip! Honestly, didn't expect that 🥳

[bkchr]

/tip medium

[substrate-tip-bot[bot]]

@bkchr A medium tip was successfully submitted for haerdib (FsnxqJnqWVNMZZgxaQdhaCk9c5sL3WSggRCRqp1qEzk1L2i on kusama).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama-rpc.polkadot.io#/referenda

[michalkucharczyk]

bot merge