WASM contracts MVP #5679

NikVolf · 2017-05-22T15:15:00Z

If wasm contracts are enabled by the engine, evm executor will choose wasm interpreter to run the code if also the contract code starts with wasm program header.
Wasm contracts can be called, but cannot call other contracts in the MVP
Wasm contracts can suicide
Wasm contracts can read and update storage, allocate memory and use most of the rust std lib (the only implication of it is the resulting size of the contract)
Build process for the contracts and use of the state api both can be found in wasm-tests repo:
https://github.com/paritytech/wasm-tests (the parity also runs tests against them, compiled). Rust wasm support is not very mature still, requires nightly and some linker args magic to produce result we need (contributions and suggestions to improve the above are welcome)
parity-wasm repo is the general purpose crate to serialize/deserialize wasm and also hosts (probably temporary) the interpreter
wasm-utils repo contains utilities to manipulate wasm for parity-specific targets (optimize symbols, substitute allocators, inject gas counter)

tomusdrw · 2017-05-24T07:53:48Z

ethcore/Cargo.toml

@@ -51,6 +51,8 @@ semver = "0.6"
 stats = { path = "../util/stats" }
 time = "0.1"
 transient-hashmap = "0.4"
+parity-wasm = { git = "http://github.com/nikvolf/parity-wasm" }


Should use https for that.

rphmeier · 2017-06-14T13:25:06Z

ethcore/src/evm/wasm/mod.rs

 	interpreter::UserFunctions {
 		executor: runtime,
 		functions: vec![
 			interpreter::UserFunction {
 				name: "_storage_read".to_owned(),
-				params: vec![elements::ValueType::I32, elements::ValueType::I32],
-				result: Some(elements::ValueType::I32),
+				params: vec![I32, I32],


since these all seem to be statically known, why not have it take a &'static [ValueType] instead of requiring allocation on each one?

Yeah, this optimisation can be made right away

eira-fransham · 2017-07-10T11:31:32Z

ethcore/src/evm/evm.rs

@@ -66,6 +67,8 @@ pub enum Error {
 	MutableCallInStaticContext,
 	/// Likely to cause consensus issues.
 	Internal(String),
+	/// Wasm runtime error
+	Wasm(String),


Is there something better we can give here than String? Maybe a WasmError enum with some number of "proper" error variants and one Unknown(String) variant.

I would avoid to wrap parity_wasm::interpreter::Error, because it cannot be later meaningfully encoded as trace error and put in trace db, that's why it's encoded as string

Also I cannot imagine any code that would rely on handling specific wasm interpreter errors

As long as it's strictly for human-readable messages then that's fine. If we ever finding ourself matching on it we can refactor to make it an enum.

eira-fransham · 2017-07-10T11:44:37Z

ethcore/src/evm/wasm/mod.rs

+	}
+}
+
+fn native_bindings<'a>(runtime: &'a mut Runtime) -> interpreter::UserFunctions<'a> {


Unnecessary lifetime parameters, you can just write

fn native_bindings(runtime: &mut Runtime) -> interpreter::UserFunctions {

not working, btw, since interpreter::UserFunctions has an explicit lifetime parameter
see https://gitlab.parity.io/parity/parity/-/jobs/59538

It's not because UserFunctions has a lifetime parameter, it's because Runtime has a lifetime parameter. &mut Runtime has two elided lifetime parameters, so when you had the lifetime parameter to the function it elided one of them and not the other. You do need the parameters in this case, sorry. I didn't know Runtime took a lifetime parameter.

Basically, it desugars to:

fn native_bindings<'a, 'b>(runtime: &'a mut Runtime<'b>) -> interpreter::UserFunctions<'???>

Since there are two parameters to choose from, it doesn't know which to pass to UserFunctions.

Also, I think that the b"\0asm" change will fail, since b"\0asm" has type &[u8; 4]. You need to do *b"\0asm" It's easy to miss, I should have explicitly pointed it out in my original comment.

eira-fransham · 2017-07-10T11:50:17Z

ethcore/src/evm/wasm/runtime.rs

+		}		
+	}
+
+	fn h256_at(&self, ptr: WasmPtr) -> Result<H256, interpreter::Error> {


Can this lead to type confusion bugs? Can we have a WasmPtr carry a PhantomData to ensure that we deserialize to the correct type? If not, can we mark these functions as unsafe?

It's perfectly safe, because it works on bound-checked memory api

Bound-checked, sure, but that doesn't mean that it won't cause type confusion. We're for all intents and purposes doing unchecked mem::transmute to synthesise these values. It's not going to access uninitialized memory but it may cause us to interpret data as the wrong type.

eira-fransham · 2017-07-10T11:53:11Z

ethcore/src/evm/wasm/runtime.rs

+		self.memory.set(d_ptr, &d_buf)?;
+
+		// write call args to memory
+		self.memory.set(args_ptr, &call_args.address)?;


Hard-coded byte offsets here - can we use mem::size_of_value or abstract this some other way?

No, it's crossplatform offset, we cannot rely on Rust memory layout, despite it may happen to have same value.

Ok, can we at least extract it to be more self-documenting, or use debug_assert_eq!(mem::size_of_value(something), 20) to make sure that we're not truncating data? Also, we can use non-target-dependent datatypes in a #[repr(C)] struct, which also has the benefit of allowing us to blit it to memory in a single call (even if it requires doing mem::transmute::<Foo, [u8; 100]>(my_struct) or whatever).

ok, let's chat about this and WasmPtr later in more detail
I will update in following prs if the chat will produce some useful outcome

Sounds like a great idea. Don't get me wrong, the code looks good, I'm just pointing to some areas that might deserve a second look. There's nothing that I would argue against merging because of.

eira-fransham · 2017-07-10T11:59:44Z

ethcore/src/evm/wasm/runtime.rs

+			"_debug" => {
+				self.debug_log(context)
+			},
+			"gas" => {


Should this have an underscore like the rest? (I don't know, but it just sticks out so I thought I'd mention it).

I think we'd better keep it this way for now
to distinguish injected functions from runtime
"gas" function is unique in a way it cannot be called ever directly

What happens if you compile a C file with a function called gas to wasm and run it as a contract? If we want it to be impossible (for some value of "impossible") to accidentally link a file with a conflicting name we can use non-ASCII or unprintable characters, like \0 or 💩.

Obviously if defining gas and compiling to wasm does nothing then you can just give it whatever name you like, I don't actually know the semantics here I'm just pointing out something that might cause a problem.

You cannot have named functions in webassembly, only imports or exports are named
And if C program will import gas function explicitly, it will be invalid as a contract and will fail instantly, before even running (as wasm module validation fails when there are duplicated imports, there should be rule about it in webassembly official testsuite)

Sweet, LGTM then.

Still worth adding a explicit test for this case, thanks

eira-fransham · 2017-07-10T12:04:28Z

ethcore/src/executive.rs

@@ -34,6 +34,8 @@ pub use types::executed::{Executed, ExecutionResult};
 /// Maybe something like here: `https://github.com/ethereum/libethereum/blob/4db169b8504f2b87f7d5a481819cfb959fc65f6c/libethereum/ExtVM.cpp`
 const STACK_SIZE_PER_DEPTH: usize = 24*1024;

+const WASM_MAGIC_NUMBER: [u8; 4] = [0x00, 0x61, 0x73, 0x6d];


This is just personal taste but if a byte array is supposed to be ASCII I'd rather write this as *b"\0asm" instead. It doesn't matter, but it might be something to keep in mind going forward.

eira-fransham · 2017-07-10T14:19:30Z

ethcore/src/executive.rs

@@ -34,7 +34,7 @@ pub use types::executed::{Executed, ExecutionResult};
 /// Maybe something like here: `https://github.com/ethereum/libethereum/blob/4db169b8504f2b87f7d5a481819cfb959fc65f6c/libethereum/ExtVM.cpp`
 const STACK_SIZE_PER_DEPTH: usize = 24*1024;

-const WASM_MAGIC_NUMBER: [u8; 4] = b"\0asm";
+const WASM_MAGIC_NUMBER: &'static [u8; 4] = b"\0asm";


Alternatively (it doesn't matter in this case, but it might in the future):

const WASM_MAGIC_NUMBER: [u8; 4] = *b"\0asm";

Note the deref operator in the RHS. Also, Rust will automatically infer 'static in const and static items (so you can write &[u8; 4] for the type).

NikVolf added 13 commits May 17, 2017 13:57

lifetime issues

d8cc649

Merge remote-tracking branch 'origin/master' into wasm-mvp

36f51d6

refactor to new 'native env'

1632387

Merge remote-tracking branch 'origin/master' into wasm-mvp

d3c0e85

descriptors and such

af5a4b4

wasm mvp continued

9a8c3c3

Merge remote-tracking branch 'origin/master' into wasm-mvp

7dced88

finalized env/ext bindings

ee07e77

descriptor -> call_args

fcbd5bc

inject gas counter

d9c026c

Merge remote-tracking branch 'origin/master' into wasm-mvp

a952b56

result processing and engine activation

b073836

tabify some source files

7c7c5b1

NikVolf added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels May 22, 2017

NikVolf added 3 commits May 23, 2017 13:59

Merge branch 'master' into wasm-mvp

5b2aac6

Merge remote-tracking branch 'origin/master' into wasm-mvp

0546281

Merge remote-tracking branch 'origin/master' into wasm-mvp

fe18e14

tomusdrw reviewed May 24, 2017

View reviewed changes

NikVolf added 11 commits May 24, 2017 12:18

Merge remote-tracking branch 'origin/master' into wasm-mvp

340b2fa

needs return new

b39552d

Merge remote-tracking branch 'origin/master' into wasm-mvp

0b8d0b4

wasm tests initial

34ad213

erradicate warnings

290347e

origin in the descriptor

f7a8959

Merge branch 'master' of github.com:paritytech/parity into wasm-mvp

74cbc2f

update test repo

5f9056d

payload verification tests

e80a75b

identity return payload test

5edcaf3

some test description

a7c3e21

NikVolf added 6 commits June 6, 2017 16:55

update wasm-utils also

e6a1fd7

merge and update

201a115

few traces and result handle change

9222708

Merge remote-tracking branch 'origin/master' into wasm-mvp

f514444

alter trace content

d873433

fix issues with optimizer, update to latest parity with validator, etc

6ca80c4

rphmeier reviewed Jun 14, 2017

View reviewed changes

NikVolf added 6 commits June 14, 2017 19:11

static initialization

b45c451

license preamble

7811663

Merge remote-tracking branch 'origin/master' into wasm-mvp

540bd6a

Merge remote-tracking branch 'origin/master' into wasm-mvp

ce97dcf

update wasm crates and gas costs

3b96f08

Merge remote-tracking branch 'origin/master' into wasm-mvp

59c5da4

NikVolf force-pushed the wasm-mvp branch from 8614f73 to 59c5da4 Compare July 3, 2017 09:32

NikVolf closed this Jul 3, 2017

NikVolf reopened this Jul 3, 2017

eira-fransham reviewed Jul 10, 2017

View reviewed changes

arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 10, 2017

NikVolf added 3 commits July 10, 2017 16:15

fix grumbles

397fb7b

bring back lifetime

1731787

fix compilation

7dadc9f

eira-fransham reviewed Jul 10, 2017

View reviewed changes

gavofyork merged commit 62210fb into master Jul 10, 2017

gavofyork deleted the wasm-mvp branch July 10, 2017 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WASM contracts MVP #5679

WASM contracts MVP #5679

NikVolf commented May 22, 2017 •

edited

Loading

tomusdrw May 24, 2017

rphmeier Jun 14, 2017

NikVolf Jun 14, 2017

eira-fransham Jul 10, 2017

NikVolf Jul 10, 2017

eira-fransham Jul 10, 2017 •

edited

Loading

eira-fransham Jul 10, 2017 •

edited

Loading

NikVolf Jul 10, 2017 •

edited

Loading

eira-fransham Jul 10, 2017 •

edited

Loading

eira-fransham Jul 10, 2017

NikVolf Jul 10, 2017

eira-fransham Jul 10, 2017

eira-fransham Jul 10, 2017

NikVolf Jul 10, 2017

eira-fransham Jul 10, 2017

NikVolf Jul 10, 2017

eira-fransham Jul 10, 2017

eira-fransham Jul 10, 2017

NikVolf Jul 10, 2017

eira-fransham Jul 10, 2017

NikVolf Jul 10, 2017

eira-fransham Jul 10, 2017

NikVolf Jul 10, 2017

eira-fransham Jul 10, 2017

eira-fransham Jul 10, 2017 •

edited

Loading

WASM contracts MVP #5679

WASM contracts MVP #5679

Conversation

NikVolf commented May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eira-fransham Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

eira-fransham Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

NikVolf Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

eira-fransham Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eira-fransham Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

NikVolf commented May 22, 2017 •

edited

Loading

eira-fransham Jul 10, 2017 •

edited

Loading

eira-fransham Jul 10, 2017 •

edited

Loading

NikVolf Jul 10, 2017 •

edited

Loading

eira-fransham Jul 10, 2017 •

edited

Loading

eira-fransham Jul 10, 2017 •

edited

Loading