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

sc-executor-polkavm: Migrate into PolkaVM 0.18.0 #6533

Merged
merged 74 commits into from
Dec 14, 2024

Conversation

jarkkojs
Copy link
Contributor

@jarkkojs jarkkojs commented Nov 18, 2024

Bump polkavm to 0.18.0, and update sc-polkavm-executor to be compatible with the API changes. In addition, bump also polkavm-derive and polkavm-linker in order to make sure that the all parts of the Polkadot SDK use the exact same ABI for .polkavm binaries.

Purely relying on RV32E/RV64E ABI is not possible, as PolkaVM uses a RISCV-V alike ISA, which is derived from RV32E/RV64E but it is still its own microarchitecture, i.e. not fully binary compatible.

@jarkkojs jarkkojs self-assigned this Nov 18, 2024
@jarkkojs jarkkojs force-pushed the bump-polkavm-version branch from cbe0284 to f8ddde9 Compare November 18, 2024 23:27
@jarkkojs jarkkojs added the T0-node This PR/Issue is related to the topic “node”. label Nov 18, 2024
@jarkkojs jarkkojs force-pushed the bump-polkavm-version branch 7 times, most recently from 3e5bb53 to 626ada2 Compare November 19, 2024 09:49
@jarkkojs jarkkojs force-pushed the bump-polkavm-version branch 11 times, most recently from 80a60ee to 39c80c9 Compare November 19, 2024 23:21
Signed-off-by: Jarkko Sakkinen <[email protected]>
@jarkkojs jarkkojs force-pushed the bump-polkavm-version branch from 39c80c9 to f91a828 Compare November 20, 2024 03:39
@jarkkojs jarkkojs marked this pull request as ready for review November 20, 2024 03:58
@jarkkojs jarkkojs requested a review from koute as a code owner November 20, 2024 03:58
@jarkkojs jarkkojs requested a review from athei November 20, 2024 04:00
Signed-off-by: Jarkko Sakkinen <[email protected]>
@jarkkojs
Copy link
Contributor Author

BTW, please help me to interpret the CI errors...

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
polkadot/primitives/src/v8/mod.rs Outdated Show resolved Hide resolved
substrate/client/executor/common/src/error.rs Outdated Show resolved Hide resolved
substrate/client/executor/polkavm/src/lib.rs Outdated Show resolved Hide resolved
substrate/client/executor/polkavm/src/lib.rs Outdated Show resolved Hide resolved
substrate/client/executor/polkavm/src/lib.rs Outdated Show resolved Hide resolved
substrate/client/executor/polkavm/src/lib.rs Outdated Show resolved Hide resolved
substrate/client/executor/polkavm/src/lib.rs Outdated Show resolved Hide resolved
@jarkkojs
Copy link
Contributor Author

jarkkojs commented Dec 14, 2024

I get:

error[E0433]: failed to resolve: could not find `min_stack_size` in `polkavm_derive`
  --> polkadot/runtime/rococo/src/lib.rs:22:17
   |
22 | polkavm_derive::min_stack_size!(16384);
   |                 ^^^^^^^^^^^^^^ could not find `min_stack_size` in `polkavm_derive`
   |
note: found an item that was configured out
  --> /home/jarkko/.cargo/registry/src/index.crates.io-6f17d22bba15001f/polkavm-derive-0.18.0/src/lib.rs:57:14
   |
57 | macro_rules! min_stack_size {
   |              ^^^^^^^^^^^^^^
note: the item is gated here
  --> /home/jarkko/.cargo/registry/src/index.crates.io-6f17d22bba15001f/polkavm-derive-0.18.0/src/lib.rs:55:1
   |
55 | #[cfg(any(all(any(target_arch = "riscv32", target_arch = "riscv64"), target_feature = "e"), doc))]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0433`.
warning: `rococo-runtime` (lib) generated 1 warning
error: could not compile `rococo-runtime` (lib) due to 1 previous error; 1 warning emitted

How fix this?

Change was:

commit 6807f2fd81d2b06d0718c6a7ac6f427505f0b9d6
Author: Jarkko Sakkinen <[email protected]>
Date:   Sat Dec 14 12:17:05 2024 +0200

    Add min_stack_size!()
    
    Signed-off-by: Jarkko Sakkinen <[email protected]>

diff --git a/Cargo.lock b/Cargo.lock
index 8658056fee..05a634cb62 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -21511,6 +21511,7 @@ dependencies = [
  "polkadot-primitives 7.0.0",
  "polkadot-runtime-common 7.0.0",
  "polkadot-runtime-parachains 7.0.0",
+ "polkavm-derive 0.18.0",
  "rococo-runtime-constants 7.0.0",
  "scale-info",
  "separator",
diff --git a/polkadot/runtime/rococo/Cargo.toml b/polkadot/runtime/rococo/Cargo.toml
index 764c53abbf..bda7bf8fca 100644
--- a/polkadot/runtime/rococo/Cargo.toml
+++ b/polkadot/runtime/rococo/Cargo.toml
@@ -103,6 +103,7 @@ frame-try-runtime = { optional = true, workspace = true }
 frame-system-benchmarking = { optional = true, workspace = true }
 hex-literal = { workspace = true, default-features = true }
 
+polkavm-derive = { workspace = true }
 polkadot-runtime-common = { workspace = true }
 polkadot-runtime-parachains = { workspace = true }
 polkadot-primitives = { workspace = true }
diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs
index c832ace91c..204ba0bcdb 100644
--- a/polkadot/runtime/rococo/src/lib.rs
+++ b/polkadot/runtime/rococo/src/lib.rs
@@ -19,6 +19,7 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 // `construct_runtime!` does a lot of recursion and requires us to increase the limit.
 #![recursion_limit = "512"]
+polkavm_derive::min_stack_size!(16384);
 
 extern crate alloc;

@s0me0ne-unkn0wn
Copy link
Contributor

I guess it's produced during the std (native) runtime build, so just putting the min_stack_size! call under the same #[cfg] conditions should be enough. Btw, it would be good to have succinct macros to generate those cfgs, copy-pasting them over the code does not add to readability.

Signed-off-by: Jarkko Sakkinen <[email protected]>
@jarkkojs
Copy link
Contributor Author

I guess it's produced during the std (native) runtime build, so just putting the min_stack_size! call under the same #[cfg] conditions should be enough. Btw, it would be good to have succinct macros to generate those cfgs, copy-pasting them over the code does not add to readability.

Yeah should be good enough for the moment. Otherwise this becomes two-crate problem and that weights worse for the time being...

Signed-off-by: Jarkko Sakkinen <[email protected]>
@jarkkojs
Copy link
Contributor Author

OK great. After expanding to 32 kB stack does not overrun. I have now node running.

@jarkkojs jarkkojs enabled auto-merge December 14, 2024 11:56
@s0me0ne-unkn0wn
Copy link
Contributor

Running and not doing much useful work, right? I bet if you put it under some load, it'll start overflowing again. I'd use some conservative stack limit like 2 Mb. Given that we currently use a 256 Mb stack limit for Wasm PVFs, that doesn't sound like too much. Curious to hear @koute's opinion on that. And definitely needs a burn-in before going any closer to production ;)

@jarkkojs jarkkojs disabled auto-merge December 14, 2024 12:00
Signed-off-by: Jarkko Sakkinen <[email protected]>
@jarkkojs
Copy link
Contributor Author

Running and not doing much useful work, right? I bet if you put it under some load, it'll start overflowing again. I'd use some conservative stack limit like 2 Mb. Given that we currently use a 256 Mb stack limit for Wasm PVFs, that doesn't sound like too much. Curious to hear @koute's opinion on that. And definitely needs a burn-in before going any closer to production ;)

Increased to 2 MiB

@jarkkojs
Copy link
Contributor Author

Running and not doing much useful work, right? I bet if you put it under some load, it'll start overflowing again. I'd use some conservative stack limit like 2 Mb. Given that we currently use a 256 Mb stack limit for Wasm PVFs, that doesn't sound like too much. Curious to hear @koute's opinion on that. And definitely needs a burn-in before going any closer to production ;)

Increased to 2 MiB

It's not the only thing that needs to be possibly tuned after this PR. Node starting is good enough DoD.

@jarkkojs jarkkojs enabled auto-merge December 14, 2024 13:36
@jarkkojs jarkkojs requested review from a team as code owners December 14, 2024 14:06
@jarkkojs jarkkojs added this pull request to the merge queue Dec 14, 2024
Merged via the queue into paritytech:master with commit bd2c35f Dec 14, 2024
196 of 199 checks passed
@jarkkojs jarkkojs deleted the bump-polkavm-version branch December 14, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants