Kovan WASM fork code #7849

NikVolf · 2018-02-09T16:56:57Z

in chainspec json files, there is a breaking change with wasm flag, one should replace

wasm: true with wasmActivationTransition: "0x0"

debris · 2018-02-12T11:06:09Z

ethcore/src/executive.rs

+	params: &ActionParams,
+	blocknumber: BlockNumber,
+) -> Box<vm::Vm> {
+	if machine.supports_wasm(blocknumber) && params.code.as_ref().map_or(false, |code| code.len() > 4 && &code[0..4] == WASM_MAGIC_NUMBER) {


I don't like this change at all. The if statement that we have right now is terrible and this change makes it even worse. This function/interface needs to be completely reworked

Yeah, perhaps it should just go to vm_factory? This function seems to be another factory that delegates to vm_factory in case it's not wasm.

vm_factory.create(params.gas, params.code, blocknumber)

I wanted to go this way, but last time I checked it was just not possible for some reason I can't recall. perhaps I should try again and find this reason again

debris · 2018-02-12T11:08:14Z

ethcore/Cargo.toml

@@ -70,6 +70,9 @@ unexpected = { path = "../util/unexpected" }
 journaldb = { path = "../util/journaldb" }
 tempdir = "0.3"

+[dev-dependencies]
+wabt = "0.1"


I refuse pulling a huge c++ dependency to ethcore as a mandatory dependency. It needs to be do differently.

it's not mandatory, it's only for running tests

It's mandatory for everyone working on daily basis on the project. It adds cmake with all of it's burden to our toolchain.

np, but it will be just bytecode dump as a test

we already have c++ build tools prerequisites by the way (rocksdb will be first notable example), they just don't use cmake crate directly and instead use some obscure build scripts, so this cmake reference does not add anything to the list instead of one-time compilation of wabt

tomusdrw · 2018-02-12T11:21:20Z

ethcore/res/ethereum/kovan_wasm_test.json

+		"eip211Transition": 5067000,
+		"eip214Transition": 5067000,
+		"eip658Transition": 5067000,
+		"wasmActivation": 10


The convention seems to be *Transition, would keep it.

"wasmTransition" might imply that evm no longer works, i think it's not the best convention in this case

Maybe enableWasmTransition or supportWasmTransition then?

both enableWasmTransition and supportWasmTransition imply the possibilty of such transition at all, not some blocknumber where it is activated

it is not really about transition as eip-s

supportWasmTransition and enableWasmTransition implies for me that wasm is supported or wasm is enabled starting at some block. Similar to how eip658Transition implies that eip658 is active at some point (it doesn't disable any previous eips).
That said my main reason is to complain is to keep the naming consistent here, if in the future we support a fork to wasm-only network we can either have: wasmOnlyTransition or disableEvmTransition there. Introducing another convention will make the file even messier imho.

tomusdrw · 2018-02-12T11:25:36Z

ethcore/src/machine.rs

@@ -377,8 +377,8 @@ impl EthereumMachine {
 	}

 	/// If this machine supports wasm.
-	pub fn supports_wasm(&self) -> bool {
-		self.params().wasm
+	pub fn supports_wasm(&self, blocknumber: BlockNumber) -> bool {


Do we need a separate public method for that? perhaps use Schedule directly on the call site.

tomusdrw · 2018-02-12T11:26:07Z

ethcore/src/spec/spec.rs

@@ -111,6 +111,8 @@ pub struct CommonParams {
 	pub remove_dust_contracts: bool,
 	/// Wasm support
 	pub wasm: bool,


Why is that needed? Seems that it's just wasmTransition: 0. We already did breaking changes to spec files in the past, we should just highlight that in the release notes.

tomusdrw · 2018-02-12T11:28:15Z

ethcore/vm/src/action_params.rs

@@ -112,6 +112,14 @@ impl Default for ActionParams {
 	}
 }

+impl ActionParams {
+	pub fn from_origin(mut self, address: Address) -> Self {


Seems to be used only for tests, might be misleading to include it everwhere in the code.

tomusdrw · 2018-02-12T11:28:54Z

ethcore/vm/src/schedule.rs

@@ -113,6 +113,8 @@ pub struct Schedule {
 	pub kill_dust: CleanDustMode,
 	/// Enable EIP-86 rules
 	pub eip86: bool,
+	/// Is wasm active
+	pub wasm_activated: bool,


Maybe wasm: Option<WasmCosts> will be more future proof?

how do you imagine this line then:

if self.wasm || block_number >= self.wasm_activation { schedule.wasm_activated = true; }

in CommonParams::update_schedule

schedule.wasm = Some(...<WHAT?>) ?

where the prices will be held until they are suddenly active?

On the second thought, no need to keep them, they can be just default :)

Yeah, they can be default for now, but later probably come from spec (that's why I suggested merging them with the flag).

tomusdrw · 2018-02-12T11:30:10Z

ethcore/src/executive.rs

+	params: &ActionParams,
+	blocknumber: BlockNumber,
+) -> Box<vm::Vm> {
+	if machine.supports_wasm(blocknumber) && params.code.as_ref().map_or(false, |code| code.len() > 4 && &code[0..4] == WASM_MAGIC_NUMBER) {


Yeah, perhaps it should just go to vm_factory? This function seems to be another factory that delegates to vm_factory in case it's not wasm.

vm_factory.create(params.gas, params.code, blocknumber)

NikVolf · 2018-02-12T16:46:19Z

@debris , @tomusdrw

all addressed!

tomusdrw

Two more minor grumbles. Looks good otherwise.

tomusdrw · 2018-02-12T16:59:31Z

ethcore/wasm/src/lib.rs

@@ -79,8 +79,8 @@ impl vm::Vm for WasmInterpreter {
 			&wasmi::ImportsBuilder::new().with_resolver("env", &instantiation_resolover)
 		).map_err(Error)?;

-		let adjusted_gas = params.gas * U256::from(ext.schedule().wasm.opcodes_div) /
-			U256::from(ext.schedule().wasm.opcodes_mul);
+		let adjusted_gas = params.gas * U256::from(ext.schedule().wasm().opcodes_div) /


Each call to ext.schedule() is virtual, can we cache it in a variable somewhere at the top?

I don't think so, it's mutably borrows self all the way down the end of the function few times

I think it will be also optimized / devirtualized anyway

tomusdrw · 2018-02-12T17:02:54Z

ethcore/wasm/src/parser.rs

-		vals.insert(rules::InstructionType::Store, schedule.wasm.mem as u32);
-		vals.insert(rules::InstructionType::Div, schedule.wasm.div as u32);
-		vals.insert(rules::InstructionType::Mul, schedule.wasm.mul as u32);
+		vals.insert(rules::InstructionType::Load, schedule.wasm().mem as u32);


Just wondering, maybe all these methods could just do with WasmCost instead of entire schedule?

rphmeier · 2018-02-12T17:47:22Z

Prefer to put this in after #7605 -- should be easier to merge this on top (if it even has conflicts, which I'm not sure of actually)

rphmeier · 2018-02-12T17:48:09Z

ethcore/res/ethereum/kovan_wasm_test.json

@@ -0,0 +1,74 @@
+{
+	"name": "Kovan",
+	"dataDir": "kovan",


these should reflect the test-ness of the spec

rphmeier · 2018-02-12T17:48:21Z

ethcore/res/ethereum/kovan_wasm_test.json

+	},
+	"nodes": [
+		"enode://56abaf065581a5985b8c5f4f88bd202526482761ba10be9bfdcd14846dd01f652ec33fde0f8c0fd1db19b59a4c04465681fcef50e11380ca88d25996191c52de@40.71.221.215:30303",
+		"enode://d07827483dc47b368eaf88454fb04b41b7452cf454e194e2bd4c14f98a3278fed5d819dbecd0d010407fc7688d941ee1e58d4f9c6354d3da3be92f55c17d7ce3@52.166.117.77:30303",


what are these bootnodes?

it's just copy from kovan.json

rphmeier · 2018-02-12T18:02:08Z

ethcore/vm/src/tests.rs

@@ -85,6 +85,11 @@ impl FakeExt {
 		ext.schedule = Schedule::new_byzantium();
 		ext
 	}
+
+	pub fn with_wasm(mut self) -> Self {


NikVolf · 2018-02-12T20:04:21Z

@rphmeier it's easy to check (just merge 2 branches in 1), there is no conflicts with #7605 for now

stale review

tomusdrw

lgtm

tomusdrw · 2018-02-19T09:46:42Z

json/src/spec/params.rs

@@ -117,6 +115,11 @@ pub struct Params {
 	/// Transaction permission contract address.
 	#[serde(rename="transactionPermissionContract")]
 	pub transaction_permission_contract: Option<Address>,
+	/// Wasm support flag
+	pub wasm: Option<bool>,


Is it still needed? What if it's set to wasm: true && wasm_activiation_transition: 500 is it activated right away or at block 500?

IMHO we should keep spec files clean (even if it requires a migration of existing spec files), especially given wasm was always experimental to date.

no, not needed, probably should mention that it requires to change in json spec to replace

wasm: true with wasmActivationTransition: "0x0"

tomusdrw · 2018-02-19T09:47:38Z

ethcore/src/factory.rs

+
+impl VmFactory {
+	pub fn create(&self, params: &ActionParams, schedule: &Schedule) -> Box<Vm> {
+		if schedule.wasm.is_some() && params.code.as_ref().map_or(false, |code| code.len() > 4 && &code[0..4] == WASM_MAGIC_NUMBER) {


Does WASM work with gas > u64::max_value()? I think I saw couple of low_u64 calls below.

It will fail later in the vm itself:

https://github.com/paritytech/parity/blob/master/ethcore/wasm/src/lib.rs#L85

* kovan fork code * introduce ethcore level vm_factory and let it fail * fix json tests * wasmcosts as option * review changes * wasm costs in parser * fix evm tests * review fixes * fix test * remove redundant json field

* ECIP 1041 - Remove Difficulty Bomb (#7905) Enable difficulty bomb defusion at block: - 5900000 on Ethereum Classic mainnet, - 2300000 on morden testnet. Reference: https://github.com/ethereumproject/ECIPs/blob/master/ECIPs/ECIP-1041.md * spec: Validate required divisor fields are not 0 (#7933) * Add validate_non_zero function It's used to validate that a Spec's uint field used as a divisor is not zero. * Add deserialize_with to gas_limit_bound_divisor Prevents panics due to divide-by-zero on the gas_limit_bound_divisor field. * Add deserialize_with to difficulty_bound_divisor Prevents panics due to divide-by-zero on the difficulty_bound_divisor field. * Add validate_optional_non_zero function Used to validate Option<Uint> divisor fields. * Use deserialize_with on optional divisor fields. * Add #[serde(default)] attribute to divisor fields When using `#[serde(deserialize_with)]`, `#[serde(default)]` must be specified so that missing fields can be deserialized with the deserializer for `None`. * Kovan WASM fork code (#7849) * kovan fork code * introduce ethcore level vm_factory and let it fail * fix json tests * wasmcosts as option * review changes * wasm costs in parser * fix evm tests * review fixes * fix test * remove redundant json field

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

NikVolf force-pushed the kovan-fork branch 2 times, most recently from e28fbb5 to 775c967 Compare February 9, 2018 21:32

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 9, 2018

kovan fork code

8918d58

NikVolf force-pushed the kovan-fork branch from 775c967 to 8918d58 Compare February 9, 2018 22:23

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

debris previously requested changes Feb 12, 2018

View reviewed changes

debris added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 12, 2018

tomusdrw reviewed Feb 12, 2018

View reviewed changes

NikVolf added 4 commits February 12, 2018 17:55

introduce ethcore level vm_factory and let it fail

e81ee4a

fix json tests

8683ff6

wasmcosts as option

c98c970

review changes

9a50983

NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 12, 2018

tomusdrw reviewed Feb 12, 2018

View reviewed changes

tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 12, 2018

NikVolf added 2 commits February 12, 2018 20:13

wasm costs in parser

89dc7e3

fix evm tests

232594f

rphmeier reviewed Feb 12, 2018

View reviewed changes

review fixes

70acef5

NikVolf force-pushed the kovan-fork branch from 9069986 to 70acef5 Compare February 12, 2018 19:58

Merge remote-tracking branch 'origin/master' into kovan-fork

eea97f5

fix test

b341bd6

5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Feb 13, 2018

NikVolf added the B0-patch label Feb 15, 2018

tomusdrw approved these changes Feb 19, 2018

View reviewed changes

tomusdrw reviewed Feb 19, 2018

View reviewed changes

tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 19, 2018

remove redundant json field

bf40801

tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 19, 2018

tomusdrw approved these changes Feb 19, 2018

View reviewed changes

5chdn merged commit 684322c into master Feb 19, 2018

5chdn deleted the kovan-fork branch February 19, 2018 11:27

andresilva mentioned this pull request Feb 19, 2018

[Beta] Backports #7945

Merged

Kovan WASM fork code #7849

Kovan WASM fork code #7849

Conversation

NikVolf commented Feb 9, 2018 • 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

NikVolf Feb 12, 2018 • 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

NikVolf Feb 12, 2018 • 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

NikVolf commented Feb 12, 2018

tomusdrw left a 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

rphmeier commented Feb 12, 2018 • 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

NikVolf commented Feb 12, 2018

tomusdrw left a 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

NikVolf commented Feb 9, 2018 •

edited

Loading

NikVolf Feb 12, 2018 •

edited

Loading

NikVolf Feb 12, 2018 •

edited

Loading

rphmeier commented Feb 12, 2018 •

edited

Loading