-
Notifications
You must be signed in to change notification settings - Fork 1.1k
sc-executor-polkavm: Migrate into PolkaVM 0.18.0 #6533
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
Changes from all commits
f91a828
618dc43
86179c2
1bae57b
1f888ed
cfa00ce
5b37507
0582e94
f173aae
2590aab
def26a5
6edfed7
f69b82d
0eedd5e
9b8bc81
e001c39
f7aa8a1
9f49e12
284d1f3
bdaa448
ae00e1c
a4190e3
e259528
8e3bb67
52289aa
6182ec8
5fa39bb
7a30409
8491e6e
bb9de25
c643934
c014bd8
68f7263
b3725df
07863f2
7defa51
73ed0a6
3bcbc83
ba53f46
a0a85e0
5e04285
6afa0e3
5437217
028b52f
b9f7879
af7580e
d78c3c7
b19f044
979e718
3abc94a
c95f16c
de8fb52
cc7a05d
2d39944
f2e9983
c023ca2
98d909c
10b6490
a2a24d3
d680f3d
44e1dab
0c162bd
f9ab11d
72ec780
db0fed9
2aff575
e132b7c
8b0fcd6
82ab893
f91314b
14374d0
93dc0ba
93fbd0d
524cf22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| 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 | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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, | ||||||
|
|
@@ -29,7 +30,7 @@ pub struct RuntimeBlob(BlobKind); | |||||
| #[derive(Clone)] | ||||||
| enum BlobKind { | ||||||
| WebAssembly(Module), | ||||||
| PolkaVM(polkavm::ProgramBlob<'static>), | ||||||
| PolkaVM((polkavm::ProgramBlob, ArcBytes)), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK good (the links above link to the same comment). |
||||||
| } | ||||||
|
|
||||||
| impl RuntimeBlob { | ||||||
|
|
@@ -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())?; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The parsing could have been done lazily in |
||||||
| 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())); | ||||||
| } | ||||||
|
|
@@ -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(), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -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), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Let's make it clear, at least at the variable name level, for now. |
||||||
| } | ||||||
| } | ||||||
| } | ||||||
Uh oh!
There was an error while loading. Please reload this page.