`gasleft` extern implemented for WASM runtime (kip-6) #9357

lexfrl · 2018-08-15T15:49:20Z

wasmGasleftActivationTransition chainspec parameter added
gasleft extern implemented
Wasm tests updated

NikVolf · 2018-08-16T08:36:13Z

json/src/spec/params.rs

@@ -152,6 +152,9 @@ pub struct Params {
 	/// KIP4 activiation block height.
 	#[serde(rename="kip4Transition")]
 	pub kip4_transition: Option<Uint>,
+	/// Wasm `gasleft` extern (aka opcode) activation block height.
+	#[serde(rename="wasmGasleftActivationTransition")]
+	pub wasm_gasleft_activation_transition: Option<Uint>,


i wonder if we can bind it to kip4_transition

how about extending kip4, since it is not activated so far

@debris @sorpaas ?

Agree that we can use 1 switch for that. However, I think we should update the KIP4 proposal itself. Otherwise, it might be very confusing

UPD: Either that, or rename the switch name.

Yeah, I will add another kip then

so now it is 2 switches, but they can be activated at once (with the same block number)

please review @pepyakin @sorpaas

NikVolf · 2018-08-20T07:33:17Z

ethcore/src/spec/spec.rs

@@ -194,6 +194,7 @@ impl CommonParams {
 			let mut wasm = ::vm::WasmCosts::default();
 			if block_number >= self.kip4_transition {


should be kip6_transition, right?

we've created separate kip for that

it’s approved? 🙂

if it will get updated, we'll update the code

it won't be in effect until chainspec changed

I see no problem to update it when KIP will be considered as final.

pepyakin · 2018-08-21T11:14:18Z

I have a little concern about gasleft extern in parity-ethereum: it doesn't have a feature gate.

lexfrl · 2018-08-21T11:16:34Z

I have a little concern about gasleft extern in parity-ethereum: it doesn't have a feature gate.

it's strictly necessary to have you think?

pepyakin · 2018-08-21T11:20:12Z

ethcore/vm/src/schedule.rs

@@ -151,6 +151,8 @@ pub struct WasmCosts {
 	pub opcodes_div: u32,
 	/// Whether create2 extern function is activated.
 	pub have_create2: bool,
+	/// Does it have a GASLEFT instruction


I'm not sure why it is referred as a "GASLEFT instruction". I believe it may be quite confusing: GASLEFT is a real concept yet which has nothing to do with wasm which instruction set isn't affect by the KIP6.

Better to borrow wording from the field above.

GASLEFT is indeed not an opcode and should not be referred as such

pepyakin · 2018-08-21T11:22:06Z

ethcore/wasm/src/tests.rs

+// This test should fail because
+// ext.schedule.wasm.as_mut().unwrap().have_gasleft = false;
+#[test]
+fn gasleft_panic() {


Why is this has a _panic postfix? It doesn't expect to actually panic afaict

it was panicking initially. So the idea is to test that it should fail in case have_gasleft is not enabled.

Yes, I can see what this test tests, but still can't see what it has to do with panics. I would rather go with _fail at least.

pepyakin · 2018-08-21T11:23:49Z

ethcore/wasm/src/tests.rs

+	let mut interpreter = wasm_interpreter(params);
+	match interpreter.exec(&mut ext) {
+		Err(..) => {},
+		Ok(..) => panic!("interpreter.exec should return Err if ext.schedule.wasm.have_gasleft = false")


I'm not sure about our house style, but this file certainly uses underscore for ignored fields while matching against enums

we don't care about data here..

Yeah, and I'm saying that when we don't care about data we use underscore.

so, I guess, the difference here is that in this specific code we also want to not care about argument arity as well

should be Ok(_) / Err(_) indeed, like in the rest of the codebase

it's not a "rest of the codebase". It's a concrete code, within its concrete context, which I explained above #9357 (comment)

oh, doesn't matter

pepyakin

Looks good to me, except a few minor nits!

pepyakin · 2018-08-21T11:30:36Z

it's strictly necessary to have you think?

Now, after KIP4 set a precedent, I think it's quite desirable.

lexfrl · 2018-08-21T11:35:41Z

Now, after KIP4 set a precedent, I think it's quite desirable.

makes sense

NikVolf · 2018-08-21T12:39:01Z

ethcore/wasm/src/env.rs

@@ -207,6 +213,7 @@ pub struct ImportResolver {
 	memory: RefCell<Option<MemoryRef>>,

 	have_create2: bool,
+	have_gasleft: bool,


better to rename this to has_gasleft and the one above to has_create2 for consistency

shall we rename everything else in the stack and is it in the scope of the PR?
https://github.com/paritytech/parity-ethereum/blob/108590d924bcc00e62b894de02516ffb2378be11/ethcore/src/spec/spec.rs#L177

schedule.have_create2 = block_number >= self.eip86_transition; schedule.have_revert = block_number >= self.eip140_transition; schedule.have_static_call = block_number >= self.eip214_transition; schedule.have_return_data = block_number >= self.eip211_transition; schedule.have_bitwise_shifting = block_number >= self.eip145_transition; schedule.have_extcodehash = block_number >= self.eip1052_transition;

oh, i didn't realized

probably not :)

NikVolf · 2018-08-21T12:41:44Z

Also there should be a test that ensures that you can make a call passing exactly gasleft gas into it

…ethereum/wasm-tests#52

* master: evmbin: Fix gas_used issue in state root mismatch and handle output better (#9418) Update hardcoded sync (#9421) Add block reward contract config to ethash and allow off-chain contracts (#9312) Private packets verification and queue refactoring (#8715) Update tobalaba.json (#9419) docs: add parity ethereum logo to readme (#9415) build: update rocksdb crate (#9414) Updating the CI system (#8765) Better support for eth_getLogs in light mode (#9186) Add update docs script to CI (#9219) `gasleft` extern implemented for WASM runtime (kip-6) (#9357) block view! removal in progress (#9397) Prevent sync restart if import queue full (#9381) nonroot CentOS Docker image (#9280) ethcore: kovan: delay activation of strict score validation (#9406)

* Wasm gasleft extern added * wasm_gasleft_activation_transition -> kip4_transition * use kip-6 switch * gasleft_panic -> gasleft_fail rename * call_msg_gasleft test added and gas_left agustments because this openethereum/wasm-tests#52 * change .. to _ * fix comment for the have_gasleft param * update tests (openethereum/wasm-tests@0edbf86)

* ethash: implement EIP-1234 (#9187) * Implement EIP-1052 (EXTCODEHASH) and fix several issues in state account cache (#9234) * Implement EIP-1052 and fix several issues related to account cache * Fix jsontests * Merge two matches together * Avoid making unnecessary Arc<Vec> * Address grumbles * Comply EIP-86 with the new definition (#9140) * Comply EIP-86 with the new CREATE2 opcode * Fix rpc compile * Fix interpreter CREATE/CREATE2 stack pop difference * Add unreachable! to fix compile * Fix instruction_info * Fix gas check due to new stack item * Add new tests in executive * Fix have_create2 comment * Remove all unused references of eip86_transition and block_number * Implement KIP4: create2 for wasm (#9277) * Basic implementation for kip4 * Add KIP-4 config flags * typo: docs fix * Fix args offset * Add tests for create2 * tests: evm * Update wasm-tests and fix all gas costs * Update wasm-tests * Update wasm-tests and fix gas costs * `gasleft` extern implemented for WASM runtime (kip-6) (#9357) * Wasm gasleft extern added * wasm_gasleft_activation_transition -> kip4_transition * use kip-6 switch * gasleft_panic -> gasleft_fail rename * call_msg_gasleft test added and gas_left agustments because this openethereum/wasm-tests#52 * change .. to _ * fix comment for the have_gasleft param * update tests (openethereum/wasm-tests@0edbf86) * Add EIP-1014 transition config flag (#9268) * Add EIP-1014 transition config flag * Remove EIP-86 configs * Change CREATE2 opcode index to 0xf5 * Move salt to the last item in the stack * Change sendersaltandaddress scheme to comply with current EIP-1014 * Fix json configs * Fix create2 test * Fix deprecated comments * EIP 1283: Net gas metering for SSTORE without dirty maps (#9319) * Implement last_checkpoint_storage_at * Add reverted_storage_at for externalities * sstore_clears_count -> sstore_clears_refund * Implement eip1283 for evm * Add eip1283Transition params * evm: fix tests * jsontests: fix test * Return checkpoint index when creating * Comply with spec Version II * Fix docs * Fix jsontests feature compile * Address grumbles * Fix no-checkpoint-entry case * Remove unnecessary expect * Add test for State::checkpoint_storage_at * Add executive level test for eip1283 * Hard-code transaction_checkpoint_index to 0 * Fix jsontests * Add tests for checkpoint discard/revert * Require checkpoint to be empty for kill_account and commit * Get code coverage * Use saturating_add/saturating_sub * Fix issues in insert_cache * Clear the state again * Fix original_storage_at * Early return for empty RLP trie storage * Update comments * Fix borrow_mut issue * Simplify checkpoint_storage_at if branches * Better commenting for gas handling code * Address naming grumbles * More tests * Fix an issue in overwrite_with * Add another test * Fix comment * Remove unnecessary bracket * Move orig to inner if * Remove test coverage for this PR * Add tests for executive original value * Add warn! for an unreachable cause * Update state tests execution model (#9440) * Update & fix JSON state tests. * Update tests to be able to run ethtest at 021fe3d410773024cd5f0387e62db6e6ec800f32. - Touch user in state - Adjust transaction tests to new json format * Switch to same commit for submodule ethereum/test as geth (next includes constantinople changes). Added test `json_tests::trie::generic::TrieTests_trieanyorder` and a few difficulty tests. * Remove trietestnextprev as it would require to parse differently and implement it. * Support new (shitty) format of transaction tests. * Ignore junk in ethereum/tests repo. * Ignore incorrect test. * Update to a later commit * Move block number to a constant. * Fix ZK2 test - touched account should also be cleared. * Fix conflict resolution * Fix checkpointing when creating contract failed (#9514) * In create memory calculation is the same for create2 because the additional parameter was popped before. (#9522) * Enable all Constantinople hard fork changes in constantinople_test.json (#9505) * Enable all Constantinople hard fork changes in constantinople_test.json * Address grumbles * Remove EIP-210 activation * 8m -> 5m * Temporarily add back eip210 transition so we can get test passed * Add eip210_test and remove eip210 transition from const_test * Add constantinople conf to EvmTestClient. (#9570) * Add constantinople conf to EvmTestClient. * Skip some test to update submodule etheureum/tests submodule to latest. * Put skipping 'under issue' test behind a feature. * Change blockReward for const-test to pass ethereum/tests * Update tests to new constantinple definition (change of reward at block 5). Switch 'reference' to string, that way we can include issues from others repo (more flexible)Update tests to new constantinple definition (change of reward at block 5). Switch 'reference' to string, that way we can include issues from others repo (more flexible). * Fix modexp and bn128_mul gas prices in chain config * Changes `run_test_path` method to append its directory results (without that it stop testing at the first file failure). Add some missing tests. Add skip for those (block create2 is one hundred percent false but on hive we can see that geth and aleth got similar issue for this item). * retab current.json * Update reference to parity issue for failing tests. * Hardfork the testnets (#9562) * ethcore: propose hardfork block number 4230000 for ropsten * ethcore: propose hardfork block number 9000000 for kovan * ethcore: enable kip-4 and kip-6 on kovan * etcore: bump kovan hardfork to block 9.2M * ethcore: fix ropsten constantinople block number to 4.2M * ethcore: disable difficulty_test_ropsten until ethereum/tests are updated upstream * Don't hash the init_code of CREATE. (#9688) * Implement CREATE2 gas changes and fix some potential overflowing (#9694) * Implement CREATE2 gas changes and fix some potential overflowing * Ignore create2 state tests * Split CREATE and CREATE2 in gasometer * Generalize rounding (x + 31) / 32 to to_word_size * ethcore: delay ropsten hardfork (#9704) * Add hardcoded headers (#9730) * add foundation hardcoded header #6486017 * add ropsten hardcoded headers #4202497 * add kovan hardcoded headers #9023489 * gitlab ci: releasable_branches: change variables condition to schedule (#9729) * HF in POA Core (2018-10-22) (#9724) poanetwork/poa-chain-spec#87

lexfrl requested review from pepyakin and NikVolf August 15, 2018 15:49

This was referenced Aug 15, 2018

ext::gas_left check added openethereum/wasm-tests#53

Merged

gasleft extern added openethereum/pwasm-ethereum#14

Merged

NikVolf reviewed Aug 16, 2018

View reviewed changes

lexfrl force-pushed the wasm-gasleft-extern branch 2 times, most recently from 98ff708 to f56608d Compare August 16, 2018 12:21

NikVolf reviewed Aug 20, 2018

View reviewed changes

lexfrl mentioned this pull request Aug 20, 2018

Resolve default gas passed to call openethereum/pwasm-abi#34

Open

NikVolf changed the title ~~[Wasm] gasleft extern implemented in Wasm runtime~~ gasleft extern implemented for WASM runtime (kip-6) Aug 21, 2018

pepyakin reviewed Aug 21, 2018

View reviewed changes

NikVolf reviewed Aug 21, 2018

View reviewed changes

lexfrl mentioned this pull request Aug 21, 2018

Add the kip6 feature gate openethereum/pwasm-ethereum#15

Merged

lexfrl and others added 4 commits August 21, 2018 17:54

Wasm gasleft extern added

f57fad6

wasm_gasleft_activation_transition -> kip4_transition

1dd8a01

use kip-6 switch

21eaf77

gasleft_panic -> gasleft_fail rename

2ea832b

lexfrl force-pushed the wasm-gasleft-extern branch from 04417f8 to 2ea832b Compare August 21, 2018 13:54

lexfrl mentioned this pull request Aug 21, 2018

call_gasleft test added openethereum/wasm-tests#54

Merged

call_msg_gasleft test added and gas_left agustments because this open…

d342189

…ethereum/wasm-tests#52

NikVolf approved these changes Aug 22, 2018

View reviewed changes

lexfrl added 3 commits August 23, 2018 11:11

change .. to _

f1d1ede

fix comment for the have_gasleft param

7b136ff

update tests (openethereum/wasm-tests@0edbf86)

fa53a4a

5chdn added this to the 2.1 milestone Aug 23, 2018

5chdn added B0-patchthis and removed A8-backport 🕸 Pull request is already reviewed well in another branch. labels Aug 23, 2018

5chdn mentioned this pull request Aug 23, 2018

beta backports for 2.0.3 #9229

Merged

28 tasks

pepyakin approved these changes Aug 24, 2018

View reviewed changes

5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 24, 2018

5chdn merged commit 5ed2527 into master Aug 24, 2018

5chdn deleted the wasm-gasleft-extern branch August 24, 2018 16:03

5chdn mentioned this pull request Aug 31, 2018

Beta backports to 2.0.4 #9452

Merged

8 tasks

andresilva mentioned this pull request Oct 9, 2018

[stable] Backport constantinople changes #9723

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`gasleft` extern implemented for WASM runtime (kip-6) #9357

`gasleft` extern implemented for WASM runtime (kip-6) #9357

lexfrl commented Aug 15, 2018 •

edited

Loading

NikVolf Aug 16, 2018

pepyakin Aug 16, 2018 •

edited

Loading

NikVolf Aug 16, 2018

NikVolf Aug 21, 2018

NikVolf Aug 20, 2018

lexfrl Aug 20, 2018

NikVolf Aug 20, 2018

lexfrl Aug 21, 2018

pepyakin commented Aug 21, 2018

lexfrl commented Aug 21, 2018

pepyakin Aug 21, 2018

NikVolf Aug 22, 2018

pepyakin Aug 21, 2018 •

edited

Loading

lexfrl Aug 21, 2018

pepyakin Aug 21, 2018

pepyakin Aug 21, 2018

lexfrl Aug 21, 2018

pepyakin Aug 21, 2018 •

edited

Loading

lexfrl Aug 21, 2018

NikVolf Aug 22, 2018

lexfrl Aug 23, 2018

lexfrl Aug 23, 2018

pepyakin left a comment

pepyakin commented Aug 21, 2018

lexfrl commented Aug 21, 2018

NikVolf Aug 21, 2018

lexfrl Aug 21, 2018 •

edited

Loading

NikVolf Aug 21, 2018

NikVolf commented Aug 21, 2018

		@@ -194,6 +194,7 @@ impl CommonParams {
		let mut wasm = ::vm::WasmCosts::default();
		if block_number >= self.kip4_transition {

gasleft extern implemented for WASM runtime (kip-6) #9357

gasleft extern implemented for WASM runtime (kip-6) #9357

Conversation

lexfrl commented Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

pepyakin Aug 16, 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

pepyakin commented Aug 21, 2018

lexfrl commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepyakin Aug 21, 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

pepyakin Aug 21, 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

pepyakin left a comment

Choose a reason for hiding this comment

pepyakin commented Aug 21, 2018

lexfrl commented Aug 21, 2018

Choose a reason for hiding this comment

lexfrl Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikVolf commented Aug 21, 2018

`gasleft` extern implemented for WASM runtime (kip-6) #9357

`gasleft` extern implemented for WASM runtime (kip-6) #9357

lexfrl commented Aug 15, 2018 •

edited

Loading

pepyakin Aug 16, 2018 •

edited

Loading

pepyakin Aug 21, 2018 •

edited

Loading

pepyakin Aug 21, 2018 •

edited

Loading

lexfrl Aug 21, 2018 •

edited

Loading