Skip to content

Make gas limit immutable in cosmwasm_vm::instance::Instance #136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 1, 2020
Merged

Make gas limit immutable in cosmwasm_vm::instance::Instance #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
40394cc
Make gas limit immutable in `cosmwasm_vm::instance::Instance`
webmaster128 Jan 31, 2020
236cc05
Add test: resets_gas_when_reusing_instance()
webmaster128 Jan 31, 2020
201829c
Test recovers_from_out_of_gas
webmaster128 Jan 31, 2020
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: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
- Remove export `check_api_compatibility`. The VM will take care of calling it.
- Let `check_api_compatibility` check imports by fully qualified identifier
`<module>.<name>`.
- Make gas limit immutable in `cosmwasm_vm::instance::Instance`. It is passed
once at construction time and cannot publicly be manipulated anymore.

## 0.6

Expand Down
7 changes: 6 additions & 1 deletion lib/vm/src/backends/singlepass.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,10 @@ pub fn set_gas(instance: &mut Instance, limit: u64) {

pub fn get_gas(instance: &Instance) -> u64 {
let used = metering::get_points_used(instance);
GAS_LIMIT - used
// when running out of gas, get_points_used can exceed GAS_LIMIT
if used < GAS_LIMIT {
Copy link
Member

Choose a reason for hiding this comment

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

Safer for sure

Copy link
Member Author

Choose a reason for hiding this comment

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

Without that, I got overflow crashes

Copy link
Member

Choose a reason for hiding this comment

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

I did also months ago with huge values, but then just used smaller numbers and forgot about it. Good fix

GAS_LIMIT - used
} else {
0
}
}
106 changes: 92 additions & 14 deletions lib/vm/src/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,28 +88,33 @@ where
}

/// get instance returns a wasmer Instance tied to a previously saved wasm
pub fn get_instance(&mut self, id: &[u8], deps: Extern<S, A>) -> Result<Instance<S, A>, Error> {
pub fn get_instance(
&mut self,
id: &[u8],
deps: Extern<S, A>,
gas_limit: u64,
) -> Result<Instance<S, A>, Error> {
let hash = WasmHash::generate(&id);

// pop from lru cache if present
Copy link
Member

Choose a reason for hiding this comment

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

The issue with this PR, is that if I pull an Instance off the lru cache, it will have the same gas limit as the first time.

Try this, by running some code (with lru), get_instance the first time with a too low gas, and it fails. Then put it back on cache. Then try get_instance with more than enough gas. I believe it will fail again as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am working on a dedicated test for the recovering after out of gas now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 201829c

if let Some(cache) = &mut self.instances {
if let Some(cached_instance) = cache.pop(&hash) {
self.stats.hits_instance += 1;
return Ok(Instance::from_wasmer(cached_instance, deps));
return Ok(Instance::from_wasmer(cached_instance, deps, gas_limit));
}
}

// try from the module cache
let res = self.modules.load_with_backend(hash, backend());
if let Ok(module) = res {
self.stats.hits_module += 1;
return Instance::from_module(&module, deps);
return Instance::from_module(&module, deps, gas_limit);
}

// fall back to wasm cache (and re-compiling) - this is for backends that don't support serialization
let wasm = self.load_wasm(id)?;
self.stats.misses += 1;
Instance::from_code(&wasm, deps)
Instance::from_code(&wasm, deps, gas_limit)
}

pub fn store_instance(&mut self, id: &[u8], instance: Instance<S, A>) -> Option<Extern<S, A>> {
Expand All @@ -135,6 +140,7 @@ mod test {
use cosmwasm::mock::{dependencies, mock_env, MockApi, MockStorage};
use cosmwasm::types::coin;

static TESTING_GAS_LIMIT: u64 = 400_000;
static CONTRACT_0_7: &[u8] = include_bytes!("../testdata/contract_0.7.wasm");

#[test]
Expand Down Expand Up @@ -170,7 +176,7 @@ mod test {
let mut cache = unsafe { CosmCache::new(tmp_dir.path(), 10).unwrap() };
let id = cache.save_wasm(CONTRACT_0_7).unwrap();
let deps = dependencies(20);
let _instance = cache.get_instance(&id, deps).unwrap();
let _instance = cache.get_instance(&id, deps, TESTING_GAS_LIMIT).unwrap();
assert_eq!(cache.stats.hits_instance, 0);
assert_eq!(cache.stats.hits_module, 1);
assert_eq!(cache.stats.misses, 0);
Expand All @@ -184,11 +190,11 @@ mod test {
let deps1 = dependencies(20);
let deps2 = dependencies(20);
let deps3 = dependencies(20);
let instance1 = cache.get_instance(&id, deps1).unwrap();
let instance1 = cache.get_instance(&id, deps1, TESTING_GAS_LIMIT).unwrap();
cache.store_instance(&id, instance1);
let instance2 = cache.get_instance(&id, deps2).unwrap();
let instance2 = cache.get_instance(&id, deps2, TESTING_GAS_LIMIT).unwrap();
cache.store_instance(&id, instance2);
let instance3 = cache.get_instance(&id, deps3).unwrap();
let instance3 = cache.get_instance(&id, deps3, TESTING_GAS_LIMIT).unwrap();
cache.store_instance(&id, instance3);
assert_eq!(cache.stats.hits_instance, 2);
assert_eq!(cache.stats.hits_module, 1);
Expand All @@ -201,7 +207,7 @@ mod test {
let mut cache = unsafe { CosmCache::new(tmp_dir.path(), 10).unwrap() };
let id = cache.save_wasm(CONTRACT_0_7).unwrap();
let deps = dependencies(20);
let mut instance = cache.get_instance(&id, deps).unwrap();
let mut instance = cache.get_instance(&id, deps, TESTING_GAS_LIMIT).unwrap();

// run contract
let env = mock_env(&instance.api, "creator", &coin("1000", "earth"), &[]);
Expand All @@ -219,7 +225,7 @@ mod test {
let mut cache = unsafe { CosmCache::new(tmp_dir.path(), 10).unwrap() };
let id = cache.save_wasm(CONTRACT_0_7).unwrap();
let deps = dependencies(20);
let mut instance = cache.get_instance(&id, deps).unwrap();
let mut instance = cache.get_instance(&id, deps, TESTING_GAS_LIMIT).unwrap();

// init contract
let env = mock_env(&instance.api, "creator", &coin("1000", "earth"), &[]);
Expand Down Expand Up @@ -252,7 +258,7 @@ mod test {
let deps2 = dependencies(20);

// init instance 1
let mut instance = cache.get_instance(&id, deps1).unwrap();
let mut instance = cache.get_instance(&id, deps1, TESTING_GAS_LIMIT).unwrap();
let env = mock_env(&instance.api, "owner1", &coin("1000", "earth"), &[]);
let msg = r#"{"verifier": "sue", "beneficiary": "mary"}"#.as_bytes();
let res = call_init(&mut instance, &env, msg).unwrap();
Expand All @@ -261,7 +267,7 @@ mod test {
let deps1 = cache.store_instance(&id, instance).unwrap();

// init instance 2
let mut instance = cache.get_instance(&id, deps2).unwrap();
let mut instance = cache.get_instance(&id, deps2, TESTING_GAS_LIMIT).unwrap();
let env = mock_env(&instance.api, "owner2", &coin("500", "earth"), &[]);
let msg = r#"{"verifier": "bob", "beneficiary": "john"}"#.as_bytes();
let res = call_init(&mut instance, &env, msg).unwrap();
Expand All @@ -270,7 +276,7 @@ mod test {
let deps2 = cache.store_instance(&id, instance).unwrap();

// run contract 2 - just sanity check - results validate in contract unit tests
let mut instance = cache.get_instance(&id, deps2).unwrap();
let mut instance = cache.get_instance(&id, deps2, TESTING_GAS_LIMIT).unwrap();
let env = mock_env(
&instance.api,
"bob",
Expand All @@ -284,7 +290,7 @@ mod test {
let _ = cache.store_instance(&id, instance).unwrap();

// run contract 1 - just sanity check - results validate in contract unit tests
let mut instance = cache.get_instance(&id, deps1).unwrap();
let mut instance = cache.get_instance(&id, deps1, TESTING_GAS_LIMIT).unwrap();
let env = mock_env(
&instance.api,
"sue",
Expand All @@ -297,4 +303,76 @@ mod test {
assert_eq!(1, msgs.len());
let _ = cache.store_instance(&id, instance);
}

#[test]
#[cfg(feature = "default-singlepass")]
fn resets_gas_when_reusing_instance() {
let tmp_dir = TempDir::new().unwrap();
let mut cache = unsafe { CosmCache::new(tmp_dir.path(), 10).unwrap() };
let id = cache.save_wasm(CONTRACT_0_7).unwrap();

let deps1 = dependencies(20);
let deps2 = dependencies(20);

// Init from module cache
let mut instance1 = cache.get_instance(&id, deps1, TESTING_GAS_LIMIT).unwrap();
assert_eq!(cache.stats.hits_module, 1);
assert_eq!(cache.stats.hits_instance, 0);
assert_eq!(cache.stats.misses, 0);
let original_gas = instance1.get_gas();

// Consume some gas
let env = mock_env(&instance1.api, "owner1", &coin("1000", "earth"), &[]);
let msg = r#"{"verifier": "sue", "beneficiary": "mary"}"#.as_bytes();
call_init(&mut instance1, &env, msg).unwrap();
assert!(instance1.get_gas() < original_gas);
cache.store_instance(&id, instance1).unwrap();

// Init from instance cache
let instance2 = cache.get_instance(&id, deps2, TESTING_GAS_LIMIT).unwrap();
assert_eq!(cache.stats.hits_module, 1);
assert_eq!(cache.stats.hits_instance, 1);
assert_eq!(cache.stats.misses, 0);
assert_eq!(instance2.get_gas(), TESTING_GAS_LIMIT);
}

#[test]
#[cfg(feature = "default-singlepass")]
fn recovers_from_out_of_gas() {
let tmp_dir = TempDir::new().unwrap();
let mut cache = unsafe { CosmCache::new(tmp_dir.path(), 10).unwrap() };
let id = cache.save_wasm(CONTRACT_0_7).unwrap();

let deps1 = dependencies(20);
let deps2 = dependencies(20);

// Init from module cache
let mut instance1 = cache.get_instance(&id, deps1, 10).unwrap();
assert_eq!(cache.stats.hits_module, 1);
assert_eq!(cache.stats.hits_instance, 0);
assert_eq!(cache.stats.misses, 0);

// Consume some gas. This fails
let env1 = mock_env(&instance1.api, "owner1", &coin("1000", "earth"), &[]);
let msg1 = r#"{"verifier": "sue", "beneficiary": "mary"}"#.as_bytes();
match call_init(&mut instance1, &env1, msg1) {
Err(Error::RuntimeErr { .. }) => (), // all good, continue
Err(e) => panic!("unexpected error, {:?}", e),
Ok(_) => panic!("call_init must run out of gas"),
}
assert_eq!(instance1.get_gas(), 0);
cache.store_instance(&id, instance1).unwrap();

// Init from instance cache
let mut instance2 = cache.get_instance(&id, deps2, TESTING_GAS_LIMIT).unwrap();
assert_eq!(cache.stats.hits_module, 1);
assert_eq!(cache.stats.hits_instance, 1);
assert_eq!(cache.stats.misses, 0);
assert_eq!(instance2.get_gas(), TESTING_GAS_LIMIT);

// Now it works
let env2 = mock_env(&instance2.api, "owner2", &coin("500", "earth"), &[]);
let msg2 = r#"{"verifier": "bob", "beneficiary": "john"}"#.as_bytes();
call_init(&mut instance2, &env2, msg2).unwrap();
}
}
56 changes: 23 additions & 33 deletions lib/vm/src/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ where
S: Storage + 'static,
A: Api + 'static,
{
pub fn from_code(code: &[u8], deps: Extern<S, A>) -> Result<Self> {
pub fn from_code(code: &[u8], deps: Extern<S, A>, gas_limit: u64) -> Result<Self> {
let module = compile(code)?;
Instance::from_module(&module, deps)
Instance::from_module(&module, deps, gas_limit)
}

pub fn from_module(module: &Module, deps: Extern<S, A>) -> Result<Self> {
pub fn from_module(module: &Module, deps: Extern<S, A>, gas_limit: u64) -> Result<Self> {
// copy this so it can be moved into the closures, without pulling in deps
let api = deps.api;
let import_obj = imports! {
Expand Down Expand Up @@ -72,10 +72,15 @@ where
},
};
let wasmer_instance = module.instantiate(&import_obj).context(WasmerErr {})?;
Ok(Instance::from_wasmer(wasmer_instance, deps))
Ok(Instance::from_wasmer(wasmer_instance, deps, gas_limit))
}

pub fn from_wasmer(wasmer_instance: wasmer_runtime_core::Instance, deps: Extern<S, A>) -> Self {
pub fn from_wasmer(
mut wasmer_instance: wasmer_runtime_core::Instance,
deps: Extern<S, A>,
gas_limit: u64,
) -> Self {
set_gas(&mut wasmer_instance, gas_limit);
let res = Instance {
Copy link
Member

Choose a reason for hiding this comment

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

Yes. Looks proper

wasmer_instance: wasmer_instance,
api: deps.api,
Expand All @@ -95,10 +100,6 @@ where
get_gas(&self.wasmer_instance)
}

pub fn set_gas(&mut self, gas: u64) {
set_gas(&mut self.wasmer_instance, gas)
}

pub fn with_storage<F: FnMut(&mut S)>(&self, func: F) {
with_storage_from_context(self.wasmer_instance.context(), func)
}
Expand Down Expand Up @@ -145,32 +146,26 @@ where
#[cfg(test)]
mod test {
use crate::calls::{call_handle, call_init, call_query};
use crate::testing::mock_instance;
use crate::testing::{mock_instance, mock_instance_with_gas_limit};
use cosmwasm::mock::mock_env;
use cosmwasm::types::coin;

static CONTRACT_0_7: &[u8] = include_bytes!("../testdata/contract_0.7.wasm");

#[test]
#[cfg(feature = "default-cranelift")]
fn get_and_set_gas_cranelift_noop() {
let mut instance = mock_instance(&CONTRACT_0_7);
fn set_get_and_gas_cranelift_noop() {
let instance = mock_instance_with_gas_limit(&CONTRACT_0_7, 123321);
let orig_gas = instance.get_gas();
assert!(orig_gas > 1000);
// this is a no-op
instance.set_gas(123456);
assert_eq!(orig_gas, instance.get_gas());
assert_eq!(orig_gas, 1_000_000);
}

#[test]
#[cfg(feature = "default-singlepass")]
fn get_and_set_gas_singlepass_works() {
let mut instance = mock_instance(&CONTRACT_0_7);
fn set_get_and_gas_singlepass_works() {
let instance = mock_instance_with_gas_limit(&CONTRACT_0_7, 123321);
let orig_gas = instance.get_gas();
assert!(orig_gas > 1000000);
// it is updated to whatever we set it with
instance.set_gas(123456);
assert_eq!(123456, instance.get_gas());
assert_eq!(orig_gas, 123321);
}

#[test]
Expand All @@ -185,8 +180,7 @@ mod test {
#[cfg(feature = "default-singlepass")]
fn contract_deducts_gas() {
let mut instance = mock_instance(&CONTRACT_0_7);
let orig_gas = 200_000;
instance.set_gas(orig_gas);
let orig_gas = instance.get_gas();

// init contract
let env = mock_env(&instance.api, "creator", &coin("1000", "earth"), &[]);
Expand All @@ -200,7 +194,7 @@ mod test {
assert_eq!(init_used, 52_468);

// run contract - just sanity check - results validate in contract unit tests
instance.set_gas(orig_gas);
let gas_before_handle = instance.get_gas();
let env = mock_env(
&instance.api,
"verifies",
Expand All @@ -212,17 +206,15 @@ mod test {
let msgs = res.unwrap().messages;
assert_eq!(1, msgs.len());

let handle_used = orig_gas - instance.get_gas();
let handle_used = gas_before_handle - instance.get_gas();
println!("handle used: {}", handle_used);
assert_eq!(handle_used, 86_749);
}

#[test]
#[cfg(feature = "default-singlepass")]
fn contract_enforces_gas_limit() {
let mut instance = mock_instance(&CONTRACT_0_7);
let orig_gas = 20_000;
instance.set_gas(orig_gas);
let mut instance = mock_instance_with_gas_limit(&CONTRACT_0_7, 20_000);

// init contract
let env = mock_env(&instance.api, "creator", &coin("1000", "earth"), &[]);
Expand All @@ -235,23 +227,21 @@ mod test {
#[cfg(feature = "default-singlepass")]
fn query_works_with_metering() {
let mut instance = mock_instance(&CONTRACT_0_7);
let orig_gas = 200_000;
instance.set_gas(orig_gas);

// init contract
let env = mock_env(&instance.api, "creator", &coin("1000", "earth"), &[]);
let msg = r#"{"verifier": "verifies", "beneficiary": "benefits"}"#.as_bytes();
let _res = call_init(&mut instance, &env, msg).unwrap().unwrap();

// run contract - just sanity check - results validate in contract unit tests
instance.set_gas(orig_gas);
let gas_before_query = instance.get_gas();
// we need to encode the key in base64
let msg = r#"{"verifier":{}}"#.as_bytes();
let res = call_query(&mut instance, msg).unwrap();
let answer = res.unwrap();
assert_eq!(answer.as_slice(), "verifies".as_bytes());

let query_used = orig_gas - instance.get_gas();
let query_used = gas_before_query - instance.get_gas();
println!("query used: {}", query_used);
assert_eq!(query_used, 44_919);
}
Expand Down
9 changes: 8 additions & 1 deletion lib/vm/src/testing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,17 @@ use crate::calls::{call_handle, call_init, call_query};
use crate::compatability::check_api_compatibility;
use crate::instance::Instance;

/// Gas limit for testing
static DEFAULT_GAS_LIMIT: u64 = 500_000;

pub fn mock_instance(wasm: &[u8]) -> Instance<MockStorage, MockApi> {
mock_instance_with_gas_limit(wasm, DEFAULT_GAS_LIMIT)
}

pub fn mock_instance_with_gas_limit(wasm: &[u8], gas_limit: u64) -> Instance<MockStorage, MockApi> {
check_api_compatibility(wasm).unwrap();
let deps = dependencies(20);
Instance::from_code(wasm, deps).unwrap()
Instance::from_code(wasm, deps, gas_limit).unwrap()
}

// init mimicks the call signature of the smart contracts.
Expand Down