Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

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

Merged
merged 8 commits into from
Aug 24, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ethcore/res/wasm-tests
9 changes: 9 additions & 0 deletions ethcore/src/spec/spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ pub struct CommonParams {
pub wasm_activation_transition: BlockNumber,
/// Number of first block where KIP-4 rules begin. Only has effect if Wasm is activated.
pub kip4_transition: BlockNumber,
/// Number of first block where KIP-6 rules begin. Only has effect if Wasm is activated.
pub kip6_transition: BlockNumber,
/// Gas limit bound divisor (how much gas limit can change per block)
pub gas_limit_bound_divisor: U256,
/// Registrar contract address.
Expand Down Expand Up @@ -195,6 +197,9 @@ impl CommonParams {
if block_number >= self.kip4_transition {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be kip6_transition, right?

we've created separate kip for that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it’s approved? 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

it won't be in effect until chainspec changed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

wasm.have_create2 = true;
}
if block_number >= self.kip6_transition {
wasm.have_gasleft = true;
}
schedule.wasm = Some(wasm);
}
}
Expand Down Expand Up @@ -308,6 +313,10 @@ impl From<ethjson::spec::Params> for CommonParams {
BlockNumber::max_value,
Into::into
),
kip6_transition: p.kip6_transition.map_or_else(
BlockNumber::max_value,
Into::into
),
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions ethcore/vm/src/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pub have_gasleft: bool,
}

impl Default for WasmCosts {
Expand All @@ -169,6 +171,7 @@ impl Default for WasmCosts {
opcodes_mul: 3,
opcodes_div: 8,
have_create2: false,
have_gasleft: false,
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions ethcore/wasm/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ pub mod ids {
pub const ORIGIN_FUNC: usize = 200;
pub const ELOG_FUNC: usize = 210;
pub const CREATE2_FUNC: usize = 220;
pub const GASLEFT_FUNC: usize = 230;

pub const PANIC_FUNC: usize = 1000;
pub const DEBUG_FUNC: usize = 1010;
Expand Down Expand Up @@ -157,6 +158,11 @@ pub mod signatures {
None,
);

pub const GASLEFT: StaticSignature = StaticSignature(
&[],
Some(I64),
);

pub const GASLIMIT: StaticSignature = StaticSignature(
&[I32],
None,
Expand Down Expand Up @@ -207,6 +213,7 @@ pub struct ImportResolver {
memory: RefCell<Option<MemoryRef>>,

have_create2: bool,
have_gasleft: bool,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@lexfrl lexfrl Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, i didn't realized

probably not :)

}

impl ImportResolver {
Expand All @@ -217,6 +224,7 @@ impl ImportResolver {
memory: RefCell::new(None),

have_create2: schedule.have_create2,
have_gasleft: schedule.have_gasleft,
}
}

Expand Down Expand Up @@ -274,6 +282,7 @@ impl wasmi::ModuleImportResolver for ImportResolver {
"origin" => host(signatures::ORIGIN, ids::ORIGIN_FUNC),
"elog" => host(signatures::ELOG, ids::ELOG_FUNC),
"create2" if self.have_create2 => host(signatures::CREATE2, ids::CREATE2_FUNC),
"gasleft" if self.have_gasleft => host(signatures::GASLEFT, ids::GASLEFT_FUNC),
_ => {
return Err(wasmi::Error::Instantiation(
format!("Export {} not found", field_name),
Expand Down
10 changes: 10 additions & 0 deletions ethcore/wasm/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,15 @@ impl<'a> Runtime<'a> {
self.return_u256_ptr(args.nth_checked(0)?, difficulty)
}

/// Signature: `fn gasleft() -> i64`
pub fn gasleft(&mut self) -> Result<RuntimeValue> {
Ok(RuntimeValue::from(
self.gas_left()? * self.ext.schedule().wasm().opcodes_mul as u64
/ self.ext.schedule().wasm().opcodes_div as u64
)
)
}

/// Signature: `fn gaslimit(dest: *mut u8)`
pub fn gaslimit(&mut self, args: RuntimeArgs) -> Result<()> {
let gas_limit = self.ext.env_info().gas_limit;
Expand Down Expand Up @@ -782,6 +791,7 @@ mod ext_impl {
ORIGIN_FUNC => void!(self.origin(args)),
ELOG_FUNC => void!(self.elog(args)),
CREATE2_FUNC => some!(self.create2(args)),
GASLEFT_FUNC => some!(self.gasleft()),
_ => panic!("env module doesn't provide function at index {}", index),
}
}
Expand Down
109 changes: 99 additions & 10 deletions ethcore/wasm/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ fn create() {
&FakeCall {
call_type: FakeCallType::Create,
create_scheme: Some(CreateContractAddress::FromSenderAndCodeHash),
gas: U256::from(52_017),
gas: U256::from(49_674),
sender_address: None,
receive_address: None,
value: Some((1_000_000_000 / 2).into()),
Expand All @@ -315,15 +315,15 @@ fn create() {
&FakeCall {
call_type: FakeCallType::Create,
create_scheme: Some(CreateContractAddress::FromSenderSaltAndCodeHash(H256::from([5u8].as_ref()))),
gas: U256::from(10_740),
gas: U256::from(6039),
sender_address: None,
receive_address: None,
value: Some((1_000_000_000 / 2).into()),
data: vec![0u8, 2, 4, 8, 16, 32, 64, 128],
code_address: None,
}
));
assert_eq!(gas_left, U256::from(10_675));
assert_eq!(gas_left, U256::from(5974));
}

#[test]
Expand Down Expand Up @@ -371,6 +371,54 @@ fn call_msg() {
assert_eq!(gas_left, U256::from(91_672));
}

// The same as `call_msg`, but send a `pwasm_ethereum::gasleft`
// value as `gas` argument to the inner pwasm_ethereum::call
#[test]
fn call_msg_gasleft() {
::ethcore_logger::init_log();

let sender: Address = "01030507090b0d0f11131517191b1d1f21232527".parse().unwrap();
let receiver: Address = "0f572e5295c57f15886f9b263e2f6d2d6c7b5ec6".parse().unwrap();
let contract_address: Address = "0d461d4174b4ae35775c4a342f1e5e1e4e6c4db5".parse().unwrap();

let mut params = ActionParams::default();
params.sender = sender.clone();
params.address = receiver.clone();
params.code_address = contract_address.clone();
params.gas = U256::from(100_000);
params.code = Some(Arc::new(load_sample!("call_gasleft.wasm")));
params.data = Some(Vec::new());

let mut ext = FakeExt::new().with_wasm();
ext.schedule.wasm.as_mut().unwrap().have_gasleft = true;
ext.balances.insert(receiver.clone(), U256::from(10000000000u64));

let gas_left = {
let mut interpreter = wasm_interpreter(params);
let result = interpreter.exec(&mut ext).expect("Interpreter to execute without any errors");
match result {
GasLeft::Known(gas_left) => gas_left,
GasLeft::NeedsReturn { .. } => { panic!("Call test should not return payload"); },
}
};

trace!(target: "wasm", "fake_calls: {:?}", &ext.calls);
assert!(ext.calls.contains(
&FakeCall {
call_type: FakeCallType::Call,
create_scheme: None,
gas: U256::from(91_163),
sender_address: Some(receiver),
receive_address: Some(Address::from([99, 88, 77, 66, 55, 44, 33, 22, 11, 0, 11, 22, 33, 44, 55, 66, 77, 88, 99, 0])),
value: Some(1000000000.into()),
data: vec![129u8, 123, 113, 107, 101, 97],
code_address: Some(Address::from([99, 88, 77, 66, 55, 44, 33, 22, 11, 0, 11, 22, 33, 44, 55, 66, 77, 88, 99, 0])),
}
));

assert_eq!(gas_left, U256::from(91_669));
}

#[test]
fn call_code() {
::ethcore_logger::init_log();
Expand Down Expand Up @@ -591,7 +639,7 @@ fn math_add() {
U256::from_dec_str("1888888888888888888888888888887").unwrap(),
(&result[..]).into()
);
assert_eq!(gas_left, U256::from(92_095));
assert_eq!(gas_left, U256::from(92_072));
}

// multiplication
Expand All @@ -613,7 +661,7 @@ fn math_mul() {
U256::from_dec_str("888888888888888888888888888887111111111111111111111111111112").unwrap(),
(&result[..]).into()
);
assert_eq!(gas_left, U256::from(91_423));
assert_eq!(gas_left, U256::from(91_400));
}

// subtraction
Expand All @@ -635,7 +683,7 @@ fn math_sub() {
U256::from_dec_str("111111111111111111111111111111").unwrap(),
(&result[..]).into()
);
assert_eq!(gas_left, U256::from(92_095));
assert_eq!(gas_left, U256::from(92_072));
}

// subtraction with overflow
Expand Down Expand Up @@ -677,7 +725,7 @@ fn math_div() {
U256::from_dec_str("1125000").unwrap(),
(&result[..]).into()
);
assert_eq!(gas_left, U256::from(87_379));
assert_eq!(gas_left, U256::from(85_700));
}

#[test]
Expand Down Expand Up @@ -705,7 +753,7 @@ fn storage_metering() {
};

// 0 -> not 0
assert_eq!(gas_left, U256::from(72_395));
assert_eq!(gas_left, U256::from(72_164));

// #2

Expand All @@ -724,7 +772,7 @@ fn storage_metering() {
};

// not 0 -> not 0
assert_eq!(gas_left, U256::from(87_395));
assert_eq!(gas_left, U256::from(87_164));
}

// This test checks the ability of wasm contract to invoke
Expand Down Expand Up @@ -815,6 +863,47 @@ fn externs() {
assert_eq!(gas_left, U256::from(90_428));
}

// This test checks the ability of wasm contract to invoke gasleft
#[test]
fn gasleft() {
::ethcore_logger::init_log();

let mut params = ActionParams::default();
params.gas = U256::from(100_000);
params.code = Some(Arc::new(load_sample!("gasleft.wasm")));

let mut ext = FakeExt::new().with_wasm();
ext.schedule.wasm.as_mut().unwrap().have_gasleft = true;

let mut interpreter = wasm_interpreter(params);
let result = interpreter.exec(&mut ext).expect("Interpreter to execute without any errors");
match result {
GasLeft::Known(_) => {},
GasLeft::NeedsReturn { gas_left, data, .. } => {
let gas = LittleEndian::read_u64(data.as_ref());
assert_eq!(gas, 93_420);
assert_eq!(gas_left, U256::from(93_343));
},
}
}

// This test should fail because
// ext.schedule.wasm.as_mut().unwrap().have_gasleft = false;
#[test]
fn gasleft_fail() {
::ethcore_logger::init_log();

let mut params = ActionParams::default();
params.gas = U256::from(100_000);
params.code = Some(Arc::new(load_sample!("gasleft.wasm")));
let mut ext = FakeExt::new().with_wasm();
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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't care about data here..

Copy link
Contributor

@pepyakin pepyakin Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, doesn't matter

}
}

#[test]
fn embedded_keccak() {
::ethcore_logger::init_log();
Expand Down Expand Up @@ -873,7 +962,7 @@ fn events() {
assert_eq!(&log_entry.data, b"gnihtemos");

assert_eq!(&result, b"gnihtemos");
assert_eq!(gas_left, U256::from(83_158));
assert_eq!(gas_left, U256::from(83_161));
}

#[test]
Expand Down
3 changes: 3 additions & 0 deletions json/src/spec/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ pub struct Params {
/// KIP4 activiation block height.
#[serde(rename="kip4Transition")]
pub kip4_transition: Option<Uint>,
/// KIP6 activiation block height.
#[serde(rename="kip6Transition")]
pub kip6_transition: Option<Uint>,
}

#[cfg(test)]
Expand Down