Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Compile the wasm runtime with a stable toolchain #1252

Closed
4 tasks done
pepyakin opened this issue Dec 11, 2018 · 37 comments · Fixed by #13580
Closed
4 tasks done

Compile the wasm runtime with a stable toolchain #1252

pepyakin opened this issue Dec 11, 2018 · 37 comments · Fixed by #13580
Labels
I7-refactor Code needs refactoring. U4-some_day_maybe Issue might be worth doing eventually.
Milestone

Comments

@pepyakin
Copy link
Contributor

pepyakin commented Dec 11, 2018

We require nightly compiler for building the wasm runtime. The main reason for that is several important features was available only on the nightly compilers.

However, as the time progresses more and more features are promoted to stable.

@pepyakin pepyakin added this to the As-and-when milestone Dec 11, 2018
@gavofyork gavofyork added the I7-refactor Code needs refactoring. label Dec 18, 2018
@pepyakin
Copy link
Contributor Author

The alloc crate is on the course of stabilization: PR

@gui1117
Copy link
Contributor

gui1117 commented Jun 14, 2019

alloc crate is stabilized by the way

@tomaka
Copy link
Contributor

tomaka commented Jul 24, 2019

The biggest offenders for this are actually not in Substrate, but dependencies such as elastic-array or hash-db.
There seems to be way more than just std::arch::wasm32::unreachable missing.

@pepyakin
Copy link
Contributor Author

pepyakin commented Feb 4, 2020

  • One dependency that still uses features is safe_mix. It does so needlessly and actually is already fixed on master (not published yet though).
  • core::arch::wasm32::unreachable was landed and it allowed us to get rid of core_intrinsics, see Get rid of in-substrate usages of core_intrinsics feature #4823.
  • alloc_handler_error is a real problem. We cannot work it around since liballoc requires its presence if used. There are some activity in discussion with some unhappy splats coming out of it, so 🤷‍♂

@pepyakin pepyakin changed the title Compile the runtime with a stable toolchain Compile the wasm runtime with a stable toolchain Feb 4, 2020
@pepyakin pepyakin modified the milestones: Ideas, The Distant Future Feb 4, 2020
@gnunicorn gnunicorn modified the milestones: The Distant Future, Ideas Mar 4, 2020
lamafab pushed a commit to lamafab/substrate that referenced this issue Jun 16, 2020
* Bump versions, tweak deposit costs.

* Version

* Lock

* Make test work ok when numbers are not round.

* Bump Substrate

* Lock
@athei
Copy link
Member

athei commented Oct 20, 2020

What about dropping #[no_std]? wasm32-unknown-unknown has a std target. It sounds weird that an unknown has std support but memory allocation is standardized in wasm so it can support it even without an explicit operating system. Any file system operation won't work of course (i.e panic). But that will make the runtime compile with stable.

Given that using nightly creates a a lot of friction and support overhead that might worth a shot.

@xlc
Copy link
Contributor

xlc commented Oct 20, 2020

@athei #4043 (comment)

@athei
Copy link
Member

athei commented Oct 20, 2020

Thanks. Question is if those points are still valid a year later. I am just wondering how using a nightly only feature (whose definition is to be unstable) is considered more stable than using a stable compiler.

I see that this nasty std = native assumption would making the switch very painful, though.

@pepyakin
Copy link
Contributor Author

I raised this question recently on a retreat, although with different reasons. The main concern in our discussion was the resulting binary size, and then the discussion spiraled into other realms. I was of opinion that there should be a minor increase in binary size which should be fine. I never checked though.

An argument against I heard back is that we are still changing nightlies every now and then because something breaks, or something. But I am not sure if I buy this argument. I am pretty sure that all our stuff works on many previous stable compiler versions.

As to Wei's list,

  • I don't think that no_std=wasm assumption is very painful. I think we might get away by introduction of another feature. In fact, there is a PR use runtime-wasm feature instead of assuming no_std to be wasm #7275 (issue Don't assume wasm in no_std env in runtime-interface #5547). Long term, this is related to Deprecation and Removal of Substrate Native Runtime Optimization #7288
  • I don't buy the determinism argument. I would argue that no_std/std doesn't change anything with that regard. In this context, I heard sometimes that allegedly HashMap would be non-deterministic, but it doesn't change that the wasm binary has to obtain entropy from somewhere and the host doesn't provide it anyway.
  • The stability is indeed tricky. In libstd/sys one can find this stability warning. IMO this doesn't matter though. It's not like we would switch to allow use the std stuff: of course no. mutexes, entropy, filesystems, networking will still be unusable, and we don't care if that changes. Then I don't see how these things can change because the wasm32-unknown-unknown cannot import anything from the environment. The other stuff I either see not changing (panics) or we override that anyway (allocations). As a data point, this part stays the same for three years already.

The biggest problem from the list is that the libstd is available and will panic on unsupported operations. Although I think it's unlikely, but still not impossible, e.g. that some crate dependency tries to spawn a thread. I don't see a plausible way to check that without going to inspect MIR or something alike.

On the other hand, I think one would need to put a lot of effort in order to accidentally get such an issue even on a testnet: that implies no inspection and no testing.

That said, I think it is very unlikely that we get alloc_error_handler stabilized soon and compiling runtime on stable does sound interesting.

@athei
Copy link
Member

athei commented Oct 20, 2020

The main concern in our discussion was the resulting binary size, and then the discussion spiraled into other realms. I was of opinion that there should be a minor increase in binary size which should be fine. I never checked though.

With LTO enabled unused functionality should be removed effectively. I also don't expect a huge impact. It mainly gets huge when using the included memory allocator which we will not.

I don't think that no_std=wasm assumption is very painful. I think we might get away by introduction of another feature. In fact, there is a PR #7275 (issue #5547). Long term, this is related to #7288

I agree. Using a proper feature was always the right thing to do that. It is just that this fact will add a lot of code churn to the transition. But still more of a mechanical change than something difficult.

I don't buy the determinism argument. I would argue that no_std/std doesn't change anything with that regard. In this context, I heard sometimes that allegedly HashMap would be non-deterministic, but it doesn't change that the wasm binary has to obtain entropy from somewhere and the host doesn't provide it anyway.

We could still avoid HashMap. Not everything that would be able to be used is to be used. Maybe we can create (or there is even something like it already) that works like cargo-deny just for imports to make sure only whitelisted stuff from std can be imported.

@pepyakin
Copy link
Contributor Author

(My point is that I don't see a big reason to avoid HashMap)

@athei
Copy link
Member

athei commented Oct 20, 2020

I get that. However, buying into your point is not necessary for using std. We can still continue to not use it.

@pepyakin
Copy link
Contributor Author

With LTO enabled unused functionality should be removed effectively. I also don't expect a huge impact. It mainly gets huge when using the included memory allocator which we will not.

Yeah, that takes into account effects of LTO. The problem is that the libstd builds use some additional machinery (IIRC, it was around panics).

Also, perhaps related. There were discussions about introducing a custom target. #4225

@shawntabrizi
Copy link
Member

@pepyakin should this be closed or moved?

@pepyakin
Copy link
Contributor Author

Nope, still relevant.

@stale
Copy link

stale bot commented Jul 7, 2021

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.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 7, 2021
@MrishoLukamba
Copy link
Contributor

So Should we use stable toolchain for compilation or still nightly?

@athei
Copy link
Member

athei commented Jun 6, 2022

It is still nightly. There is no stable way to use alloc in a no_std binary.

@chevdor
Copy link
Contributor

chevdor commented Jun 22, 2022

This is not more than a workaround but note that you may use stable if you really need/want to, as long as you also use RUSTC_BOOTSTRAP=1. srtool is building runtimes like this for a while now.

@athei
Copy link
Member

athei commented Dec 17, 2022

rust-lang/rust#66741 was just closed. We may have stable runtime builds soon.

@nuke-web3
Copy link
Contributor

1.67 this should be included then? What actions are needed on ops and tooling to move to stable once once it is?

  • docs updated,
  • ci/cd images and scripts remove nightly ( if possible, might need it for fmt still? Perhaps a reason to update rustfmt to be stable only)
  • tooling removes nightly (zombienet et.al.)
  • ...what else?

@athei
Copy link
Member

athei commented Dec 22, 2022

wasmbuilder needs to be changed to use a stable toolchain instead of defaulting to the latest installed nightly compiler.

We still use nightly for rustfmt. This is also executed inside the CI. So purging nightly from all our CI images is not possibly unless we are willing to move to stable rustfmt (and losing some formatting rules).

@gilescope
Copy link
Contributor

Loosing the odd formatting rule seems a small price to pay tbh. I don't think that should hold us back.

@athei
Copy link
Member

athei commented Jan 28, 2023

Rust 1.67 landed. We can compile the runtime with stable now if we use the default alloc_error_handler. However, since the opening of this issue stuff was put into this error handler to report the oom error to the client.

@koute Can we remove this error from the alloc error handler for the sake of compiling with stable? Maybe we can put the logging into the client side allocator? It can write a log when it can't fulfil a memory request which should lead to an panic.

@bkchr
Copy link
Member

bkchr commented Jan 28, 2023

@koute Can we remove this error from the alloc error handler for the sake of compiling with stable? Maybe we can put the logging into the client side allocator? It can write a log when it can't fulfil a memory request which should lead to an panic.

We can remove it. The feature isn't currently active and oom should be detected by our allocator anyway on the host side. Not sure if this is even triggered somehow.

@athei
Copy link
Member

athei commented Jan 29, 2023

Apparently it is not in 1.67. I thought since it was merged mid December it should be in this release.

@koute
Copy link
Contributor

koute commented Jan 30, 2023

@koute Can we remove this error from the alloc error handler for the sake of compiling with stable? Maybe we can put the logging into the client side allocator? It can write a log when it can't fulfil a memory request which should lead to an panic.

Yeah that's fine.

As Basti said, I'm not entirely sure if it's actually possible to trigger it. That callback is called by the alloc crate when memory allocation fails, but our allocator never returns NULL and just panics inside of the host function if there's an allocation failure so it never actually returns NULL.

@athei
Copy link
Member

athei commented Jan 30, 2023

Great. Thanks for the explanation. In that case the handler should never called indeed. Then I do this a as soon as a stable version with this is released.

@chevdor
Copy link
Contributor

chevdor commented Jan 30, 2023

I prepared a new srtool image based on Rust 1.67.0 and not using the dirty trick mentioned above.

A build can be tested from this workflow with a manual start (workflow dispatch) and some input similar to what is shown below:

image

The most import being to use the proper Docker image: chevdor/srtool:1.67.0-0.10.0-d498197-alpha

@athei
Copy link
Member

athei commented Jan 30, 2023

Didn't we just discover that it doesn't work with 1.67?

@chevdor
Copy link
Contributor

chevdor commented Jan 31, 2023

I missed that part @athei. I can update and make a new image with 1.68 or whenever it lands so we can test and confirm.

@Dengjianping
Copy link
Contributor

Now, rust 1.68 has been released.

@athei
Copy link
Member

athei commented Mar 10, 2023

Nice. I will make a PR. @chevdor can you prepare an image with the new stable version?

@bkchr
Copy link
Member

bkchr commented Mar 10, 2023

I have created another version (hadn't seen your pr, but also started earlier) that is fully backwards compatible: #13580

@chevdor
Copy link
Contributor

chevdor commented Mar 10, 2023

@athei yep I started already and found an issue in srtool, which is now fixed and merged.
So I will rebuild an image and we can test when those PRs above are merged.

@athei
Copy link
Member

athei commented Mar 13, 2023

I have created another version (hadn't seen your pr, but also started earlier) that is fully backwards compatible: #13580

Closed mine. I think your non breaking version makes more sense for the transition.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I7-refactor Code needs refactoring. U4-some_day_maybe Issue might be worth doing eventually.
Projects
Status: Done