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
Merged
Show file tree
Hide file tree
Changes from 62 commits
Commits
Show all changes
74 commits
Select commit Hold shift + click to select a range
f91a828
Bump polkavm version to 0.16.0
jarkkojs Nov 20, 2024
618dc43
Remove unused import
jarkkojs Nov 20, 2024
86179c2
Update Cargo.toml
jarkkojs Nov 20, 2024
1bae57b
Drop polkavm-common
jarkkojs Nov 20, 2024
1f888ed
sc-executor-common: Use polkavm::ArcBytes to store the raw blob
jarkkojs Nov 21, 2024
cfa00ce
sc-executor-polkavm: remove unnecessary unwrap
jarkkojs Nov 21, 2024
5b37507
sc-executor-polkavm: address unwraps and restore code
jarkkojs Nov 21, 2024
0582e94
sc-executor-polkavm: fix exception handling in call_with_allocation_s…
jarkkojs Nov 21, 2024
f173aae
sc-executor-polkavm: Restore allocate_memory()
jarkkojs Nov 21, 2024
2590aab
sc-executor-polkavm: Fix allocate_memory() error handling
jarkkojs Nov 21, 2024
def26a5
sc-executor-polkavm: Fix 64-bit reg handling in call_host_function()
jarkkojs Nov 21, 2024
6edfed7
sc-executor-polkavm: method_index -> _
jarkkojs Nov 21, 2024
f69b82d
Merge branch 'master' into bump-polkavm-version
jarkkojs Nov 21, 2024
0eedd5e
Merge branch 'master' into bump-polkavm-version
jarkkojs Nov 22, 2024
9b8bc81
ci: PR fixups
jarkkojs Nov 22, 2024
e001c39
polkadot-primitives: Revert compilation bug fix
jarkkojs Nov 22, 2024
f7aa8a1
Merge branch 'master' into bump-polkavm-version
jarkkojs Nov 22, 2024
9f49e12
polkadot-primitives: Revert compilation bug fix
jarkkojs Nov 22, 2024
284d1f3
Update substrate/client/executor/common/src/runtime_blob/runtime_blob.rs
jarkkojs Nov 22, 2024
bdaa448
sc-executor-polkavm: increase nth_reg
jarkkojs Nov 22, 2024
ae00e1c
sc-executor-polkavm: Add 64-bit set_reg calls
jarkkojs Nov 22, 2024
a4190e3
sc-executor-common: cargo fmt fix
jarkkojs Nov 22, 2024
e259528
sc-executor-polkavm: remove exports lookup from call_with_allocation_…
jarkkojs Nov 22, 2024
8e3bb67
Merge branch 'master' into bump-polkavm-version
jarkkojs Nov 25, 2024
52289aa
sc-executor-polkavm: Fixups
jarkkojs Nov 30, 2024
6182ec8
Merge remote-tracking branch 'upstream/master' into bump-polkavm-version
jarkkojs Nov 30, 2024
5fa39bb
sc-executor-polkavm: Fix cargo fmt errors
jarkkojs Nov 30, 2024
7a30409
Revert "sc-executor-polkavm: remove exports lookup from call_with_all…
jarkkojs Dec 2, 2024
8491e6e
sc-executor-polkavm: Fixup call pattern
jarkkojs Dec 3, 2024
bb9de25
Merge remote-tracking branch 'upstream/master' into bump-polkavm-version
jarkkojs Dec 3, 2024
c643934
Add Cargo.lock back
jarkkojs Dec 3, 2024
c014bd8
Merge remote-tracking branch 'upstream/master' into bump-polkavm-version
jarkkojs Dec 7, 2024
68f7263
Fix compilation errors
jarkkojs Dec 7, 2024
b3725df
Bump versions
jarkkojs Dec 7, 2024
07863f2
Update Cargo.lock
jarkkojs Dec 7, 2024
7defa51
Print errors
jarkkojs Dec 7, 2024
73ed0a6
Fix CallError handling
jarkkojs Dec 7, 2024
3bcbc83
Adjust errors
jarkkojs Dec 7, 2024
ba53f46
Adjust prdoc/pr_6533.prdoc
jarkkojs Dec 7, 2024
a0a85e0
Adjust prdoc/pr_6533.prdoc
jarkkojs Dec 7, 2024
5e04285
Do TRYBUILD=overwrite cargo t -p frame-support-test --test benchmark_ui
jarkkojs Dec 7, 2024
6afa0e3
Fixup versions
jarkkojs Dec 7, 2024
5437217
Adjust prdoc/pr_6533.prdoc
jarkkojs Dec 7, 2024
028b52f
Revert "Fixup versions"
jarkkojs Dec 7, 2024
b9f7879
Fix corrupted Cargo.lock
jarkkojs Dec 8, 2024
af7580e
Revert "Do TRYBUILD=overwrite cargo t -p frame-support-test --test be…
jarkkojs Dec 8, 2024
d78c3c7
Adjust prdoc/pr_6533.prdoc
jarkkojs Dec 8, 2024
b19f044
Adjust prdoc/pr_6533.prdoc
jarkkojs Dec 9, 2024
979e718
Merge branch 'master' into bump-polkavm-version
jarkkojs Dec 9, 2024
3abc94a
Adjust prdoc/pr_6533.prdoc
jarkkojs Dec 9, 2024
c95f16c
Update substrate/client/executor/polkavm/src/lib.rs
jarkkojs Dec 9, 2024
de8fb52
Adjust prdoc/pr_6533.prdoc
jarkkojs Dec 9, 2024
cc7a05d
Adjust prdoc/pr_6533.prdoc
jarkkojs Dec 9, 2024
2d39944
Adjust prdoc/pr_6533.prdoc
jarkkojs Dec 9, 2024
f2e9983
Fix typos prdoc/pr_6533.prdoc
jarkkojs Dec 9, 2024
c023ca2
Revert "Update substrate/client/executor/polkavm/src/lib.rs"
jarkkojs Dec 9, 2024
98d909c
Merge branch 'master' into bump-polkavm-version
jarkkojs Dec 9, 2024
10b6490
Merge branch 'master' into bump-polkavm-version
jarkkojs Dec 9, 2024
a2a24d3
Merge branch 'master' into bump-polkavm-version
jarkkojs Dec 9, 2024
d680f3d
Merge branch 'master' into bump-polkavm-version
jarkkojs Dec 10, 2024
44e1dab
rococo-runtime: Double min_stack_size
jarkkojs Dec 14, 2024
0c162bd
Revert "rococo-runtime: Double min_stack_size"
jarkkojs Dec 14, 2024
f9ab11d
Add min_stack_size!()
jarkkojs Dec 14, 2024
72ec780
Adjust min_stack_size
jarkkojs Dec 14, 2024
db0fed9
Adjust min_stack_size to 2MB
jarkkojs Dec 14, 2024
2aff575
asm_const
jarkkojs Dec 14, 2024
e132b7c
Adjust min_stack_size definition
jarkkojs Dec 14, 2024
8b0fcd6
Adjust min_stack_size definition
jarkkojs Dec 14, 2024
82ab893
add SUBSTRATE_ENABLE_POLKAVM=1 to .github/workflows/build-misc.yml
jarkkojs Dec 14, 2024
f91314b
Revert "add SUBSTRATE_ENABLE_POLKAVM=1 to .github/workflows/build-mis…
jarkkojs Dec 14, 2024
14374d0
Match rust version with polkavm
jarkkojs Dec 14, 2024
93dc0ba
Open code polkavm_derive::min_stack_size!()
jarkkojs Dec 14, 2024
93fbd0d
Remove extra changes
jarkkojs Dec 14, 2024
524cf22
Revert "Match rust version with polkavm"
jarkkojs Dec 14, 2024
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
95 changes: 90 additions & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -1089,9 +1089,9 @@ polkadot-subsystem-bench = { path = "polkadot/node/subsystem-bench" }
polkadot-test-client = { path = "polkadot/node/test/client" }
polkadot-test-runtime = { path = "polkadot/runtime/test-runtime" }
polkadot-test-service = { path = "polkadot/node/test/service" }
polkavm = { version = "0.9.3", default-features = false }
polkavm-derive = "0.17.0"
polkavm-linker = "0.17.1"
polkavm = { version = "0.18.0", default-features = false }
polkavm-derive = "0.18.0"
polkavm-linker = "0.18.0"
portpicker = { version = "0.1.1" }
pretty_assertions = { version = "1.3.0" }
primitive-types = { version = "0.13.1", default-features = false, features = [
Expand Down
20 changes: 20 additions & 0 deletions prdoc/pr_6533.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
title: "Migrate executor into PolkaVM 0.18.0"
doc:
- audience: Runtime Dev
description: |
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.

crates:
- name: sc-executor-common
bump: major
- name: sc-executor-polkavm
bump: minor
- name: substrate-wasm-builder
bump: minor
jarkkojs marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions substrate/client/executor/common/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,8 @@ pub enum WasmError {
Other(String),
}

impl From<polkavm::ProgramParseError> for WasmError {
fn from(error: polkavm::ProgramParseError) -> Self {
impl From<polkavm::program::ProgramParseError> for WasmError {
fn from(error: polkavm::program::ProgramParseError) -> Self {
WasmError::Other(error.to_string())
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use crate::{error::WasmError, wasm_runtime::HeapAllocStrategy};
use polkavm::ArcBytes;
use wasm_instrument::parity_wasm::elements::{
deserialize_buffer, serialize, ExportEntry, External, Internal, MemorySection, MemoryType,
Module, Section,
Expand All @@ -29,7 +30,7 @@ pub struct RuntimeBlob(BlobKind);
#[derive(Clone)]
enum BlobKind {
WebAssembly(Module),
PolkaVM(polkavm::ProgramBlob<'static>),
PolkaVM((polkavm::ProgramBlob, ArcBytes)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for the record, I'm not a fan of this solution. In this form, the parsed blob and the raw blob are not guaranteed to correspond to the same code. I mean, ideally, those guarantees should be provided by PolkaVM itself, not by a dependent package.

It's not a show-stopper, but I think it should be improved in the future.

Copy link
Contributor Author

@jarkkojs jarkkojs Dec 11, 2024

Choose a reason for hiding this comment

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

The reason for this change was that as_bytes() no longer exists. Please provide an alternative solution.

How it right now was based on a comment from @koute.

Copy link
Contributor

Choose a reason for hiding this comment

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

@s0me0ne-unkn0wn See my comment here regarding this. We will remove this field altogether in the future; it's just here to not add more churn than necessary, considering the PolkaVM-based executor is still experimental anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK good (the links above link to the same comment).

}

impl RuntimeBlob {
Expand All @@ -52,9 +53,9 @@ impl RuntimeBlob {
pub fn new(raw_blob: &[u8]) -> Result<Self, WasmError> {
if raw_blob.starts_with(b"PVM\0") {
if crate::is_polkavm_enabled() {
return Ok(Self(BlobKind::PolkaVM(
polkavm::ProgramBlob::parse(raw_blob)?.into_owned(),
)));
let raw = ArcBytes::from(raw_blob);
let blob = polkavm::ProgramBlob::parse(raw.clone())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

The parsing could have been done lazily in .as_polkavm_blob() as well.

return Ok(Self(BlobKind::PolkaVM((blob, raw))));
} else {
return Err(WasmError::Other("expected a WASM runtime blob, found a PolkaVM runtime blob; set the 'SUBSTRATE_ENABLE_POLKAVM' environment variable to enable the experimental PolkaVM-based executor".to_string()));
}
Expand Down Expand Up @@ -192,7 +193,7 @@ impl RuntimeBlob {
match self.0 {
BlobKind::WebAssembly(raw_module) =>
serialize(raw_module).expect("serializing into a vec should succeed; qed"),
BlobKind::PolkaVM(ref blob) => blob.as_bytes().to_vec(),
BlobKind::PolkaVM(ref blob) => blob.1.to_vec(),
Copy link
Contributor

Choose a reason for hiding this comment

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

And here's a good example of what I meant in a comment above. In this occurrence, it's PolkaVM(ref blob), and in the next one, it's PolkaVM((ref blob, _)). That is, we've already started to call two different things a "blob". That's error-prone and causes confusion, and overall, there's a lot of room for design improvement here.

}
}

Expand Down Expand Up @@ -227,7 +228,7 @@ impl RuntimeBlob {
pub fn as_polkavm_blob(&self) -> Option<&polkavm::ProgramBlob> {
match self.0 {
BlobKind::WebAssembly(..) => None,
BlobKind::PolkaVM(ref blob) => Some(blob),
BlobKind::PolkaVM((ref blob, _)) => Some(blob),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
BlobKind::PolkaVM((ref blob, _)) => Some(blob),
BlobKind::PolkaVM((ref program_blob, _)) => Some(program_blob),

Let's make it clear, at least at the variable name level, for now.

}
}
}
Loading
Loading