Updated WASM Runtime & new interpreter (wasmi) #7796

NikVolf · 2018-02-02T23:08:40Z

mem* removed from the runtime, we'll add it back if optimization needed
wasmi replaces deprecated parity-wasm interpreter
wasm32-unknown-unknown import/export name conventions
dropped emscripten runtime glue completely (STACKTOP, llvmtrap, memory base, etc..)
added wasm opcodes scaling factor of 3/8

pepyakin · 2018-02-03T11:36:43Z

ethcore/wasm/src/tests.rs

@@ -516,128 +519,10 @@ fn keccak() {
 	};

 	assert_eq!(H256::from_slice(&result), H256::from("68371d7e884c168ae2022c82bd837d51837718a7f7dfb7aa3f753074a35e1d87"));
-	assert_eq!(gas_left, U256::from(81_067));
+	assert_eq!(gas_left, U256::from(70_217));
 }

 // memcmp test.


Could you delete this line too?

pepyakin · 2018-02-03T11:38:46Z

ethcore/wasm/src/runtime.rs

 	}

-	pub fn call(&mut self, context: InterpreterCallerContext)
-		-> Result<Option<interpreter::RuntimeValue>, InterpreterError>
+	/// Read from the storage to wasm memory


Isn't this should be the other way around: from wasm memory to the storage?

pepyakin · 2018-02-03T11:41:22Z

ethcore/wasm/src/lib.rs


 		if params.gas > ::std::u64::MAX.into() {
 			return Err(vm::Error::Wasm("Wasm interpreter cannot run contracts with gas >= 2^64".to_owned()));
 		}

+		let initial_memory = instantiation_resolover.memory_size().map_err(Error)?
+			.saturating_sub(MEMORY_STIPEND);


This is quite obscure IMO, could you please add comment clarifying why this is here?

I moved to the schedule

pepyakin · 2018-02-03T12:00:16Z

ethcore/wasm/src/runtime.rs

+		&self.result
+	}
+
+	pub fn dissolve(self) -> Vec<u8> {


How about into_result?

NikVolf · 2018-02-04T17:03:28Z

@pepyakin addressed

pepyakin · 2018-02-05T16:12:08Z

ethcore/wasm/src/env.rs

+	pub fn with_limit(max_memory: u32) -> ImportResolver {
+		ImportResolver {
+			max_memory: max_memory,
+			.. Default::default()


Is this supposed to be memory (0, 0)?

Ah, I see.
Maybe it would be better to explicitly initialize memory
like RefCell::new(None) ?

pepyakin · 2018-02-05T16:17:15Z

ethcore/wasm/src/env.rs

+	pub fn memory_ref(&self) -> Result<MemoryRef, Error> {
+		{
+			let mut mem_ref = self.memory.borrow_mut();
+			if mem_ref.is_none() { *mem_ref = Some(MemoryInstance::alloc(0, Some(0))?); }


This invocation actually can't return error since params (0, Some(0)) are perfectly valid

But i don't want to depend on this invariant from other lib

But otherwise this code embeds this invariant into the consensus

pepyakin · 2018-02-05T16:20:55Z

ethcore/wasm/src/env.rs

 	}
 }
+
+impl wasmi::ModuleImportResolver for ImportResolver {
+	// todo: check against `_signature` ?


stray TODO

IMO we should check signatures early and don't rely that execution engine will check them for us.

pepyakin · 2018-02-05T16:22:37Z

ethcore/wasm/src/env.rs

+
+impl ImportResolver {
+	/// New import resolver with specifed maximum amount of inital memory.
+	pub fn with_limit(max_memory: u32) -> ImportResolver {


I think it is not clear that max_memory is denoted in pages. Maybe it would be better if we leave a comments on this? Or maybe change the name to something like max_memory_pages

pepyakin · 2018-02-05T16:31:06Z

ethcore/wasm/src/env.rs

+	) -> Result<MemoryRef, Error> {
+		if field_name == "memory" {
+			if descriptor.initial() >= self.max_memory ||
+				(descriptor.maximum().map_or(false, |m| m < descriptor.initial() || m > self.max_memory))


This is an interesting check.

If I understand the intent correctly, it checks whether initial or maximum number of memory pages requested by the module is greater than some limit. In case if maximum isn't provided then this check is passed and runtime proceeds to execution.

If that's correct, then I don't see the point. What stops the contract writer to not set maximum limit on memory and then allocate amount of memory larger than the limit (don't we have the control on grow_memory op, do we?).

And as I remember, don't we agreed that we don't want to restrict the upper memory limit by any means except the gas price?

pepyakin · 2018-02-05T16:44:33Z

ethcore/wasm/src/env.rs

+/// Internal ids all functions runtime supports. This is just a glue for wasmi interpreter
+/// that lacks high-level api and later will be factored out
+pub mod ids {
+	pub const STORAGE_WRITE_FUNC: usize = 0;


I don't see the point of this 10-step numbering.
Also, maybe we can use some macro which will generate C-like enum, so we don't have to number this constants by hand?

It's implementation detail, will refactored once more high-level api for wasmi is available

Ok, I think I'm fine with it.

pepyakin · 2018-02-05T16:46:35Z

ethcore/wasm/src/runtime.rs

 		} else {
 			Ok(())
 		}
 	}

-	pub fn overflow_charge<F>(&mut self, f: F) -> Result<(), InterpreterError>
+	/// Adjusted charge applies wasm divisor coeff as


Im not sure what you meant here...

pepyakin · 2018-02-05T16:59:52Z

ethcore/wasm/src/runtime.rs

+	}
+
+	/// Return. Syscall takes 2 arguments - pointer in sandboxed memory where result is and
+	/// the lendth of the result.


typo: lendth → length

pepyakin · 2018-02-05T17:07:19Z

ethcore/wasm/src/runtime.rs

-		let result_alloc_len = context.value_stack.pop_as::<i32>()? as u32;
-		trace!(target: "wasm", "    result_len: {:?}", result_alloc_len);
+		let vofs = if use_val { 1 } else { 0 };
+		let val = if use_val { Some(self.u256_at(args.nth(2)?)?) } else { None };


I think such sorta dynamic signatures might lead to a disaster.
Can we fix this somehow? For example, change pwasm-std in a such way that we always pass some dummy val parameter?
I think even duplicating parameter decoding in methods *call below would be a better solution.

It won't lead to disaster if there is test that shows that all arguments are mapped correctly, which is the case here

Can we do this multiplexing on the pwasm-std side? This way we'll get rid of all this dcall/scall and etc exports.

I don't think so, these are mostly different operations, and might be handled completely differently lately, so it might be good idea to differentiate them at link time

also, EVM has different opcodes for those, so it's good to have 1-1 mapping opcode<->wasm external

pepyakin · 2018-02-05T17:11:58Z

ethcore/wasm/src/runtime.rs

+		let code_len: u32 = args.nth(2)?;
+		trace!(target: "wasm", "  code_len: {:?}", code_len);
+		let result_ptr: u32 = args.nth(3)?;
+		trace!(target: "wasm", "result_ptr: {:?}", result_ptr);


I think this code will more legible if we won't interleave argument decoding code with displaying it, as we do in do_call.
At least, I'm wondering, should we synchronize them?

pepyakin · 2018-02-05T17:24:30Z

ethcore/wasm/src/lib.rs

-					runtime.write_descriptor(
-						if data_position < code.len() { &code[data_position..] } else { &[] }
-					).map_err(Error)?
+		// We count all memory except free (MEMORY_STIPEND)


What's the MEMORY_STIPEND?

NikVolf · 2018-02-05T17:30:01Z

@pepyakin all addressed

* update back-references more aggressively after answering from cache (#7578) * Add new EF ropstens nodes. (#7824) * Add new EF ropstens nodes. * Fix tests * Add a timeout for light client sync requests (#7848) * Add a timeout for light client sync requests * Adjusting timeout to number of headers * Flush keyfiles. Resolves #7632 (#7868) * Fix wallet import (#7873) * rpc: generate new account id for imported wallets * ethstore: handle duplicate wallet filenames * ethstore: simplify deduplication of wallet file names * ethstore: do not dedup wallet filenames on update * ethstore: fix minor grumbles * [WASM] mem_cmp added to the Wasm runtime (#7539) * mem_cmp added to the Wasm runtime * schedule.wasm.mem_copy to schedule.wasm.mem_cmp for mem_cmp * [Wasm] memcmp fix and test added (#7590) * [Wasm] memcmp fix and test added * [Wasm] use reqrep_test! macro for memcmp test * wasmi interpreter (#7796) * adjust storage update evm-style (#7812) * disable internal memory (#7842)

NikVolf added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 2, 2018

pepyakin reviewed Feb 3, 2018

View reviewed changes

NikVolf force-pushed the wasmi branch 3 times, most recently from 25afff3 to f85433b Compare February 4, 2018 17:02

NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 4, 2018

5chdn added this to the 1.10 milestone Feb 5, 2018

NikVolf force-pushed the wasmi branch 3 times, most recently from 7db1f31 to 1332adf Compare February 5, 2018 16:08