Gas consumed

[xiangjianmeng]

Suppose gas limit passed in wasm is 100, then amount of gas used for instructions is 90 when call function in contract, and reading or writing storage consumed 30 gas which was deducted from the context gas limit in the call for cotract. The call can be excuted finally because the gas limit(100) is greater than which needed(90). But now the context gas limit is 70. So it will failed when call consumeGas(ctx, res.GasUsed). I think it maybe not be a bug, but it will increase the failure rate of calling contract.

[ethanfrey]

The contract should definitely fail in that case.

You are right that there is a bit of a space for abuse, that if gas limit was 100, I could spend 99 gas running wasm, and 99 gas accessing storage and thus spend 198 gas before triggering an out-of-gas panic. So, failure is right, but it should fail earlier.

There is an open pull request in wasmer that will make this much easier. The use of gas in the sdk storage can reduce the limit left on the contract, while the contract keeps track of gas_used. Unless there is a strong reason otherwise, I would leave the current behavior (with mild DoS escalation potential) and fix it properly when the "dynamic limit" PR lands.

There is also another open PR to make gas limits stricter in wasmer, so it would be nice to tackle them both together to really tighten up any leaks before putting this in production. (Not that either can do anything horrible or halt the chain, but they can be used as a multiplier to slow down the system beyond the gas paid).

[reuvenpo]

To fully solve this issue several components are required:

• When a panic occurs in a Go callback that reads from storage (cGet/cSet) it needs to be checked for the types thrown in the case of out-of-gas events from the SDK, and inform the Rust code that such an error occurred across the FFI boundary. Commit 47ef4bc in PR Unlimited buffers in ffi #67 helps expose this information to Rust
• Rust must then handle this condition and correctly stop everything it is doing, and propagate the error back up to the Go code that called it (convert those errors somewhere around go-cosmwasm/src/lib.rs calls to call_handle_raw, call_init_raw, and call_query_raw). That Go code must then rethrow a similar panic.
• When wasmer fails due to an OutOfGas event (this match statement implies that this is represented as a WasmerRuntimeErr of some sort, but that's as accurate as it gets from what i can tell), that error needs to be caught after contract execution and converted to an appropriate error type as in the previous point. That will allow both cases - SDK out of gas, and contract out of gas - to be represented the same way

From what i can tell, today the code does not correctly represent contract-out-of-gas scenarios at all. It also invokes UB (because of cross-language stack unwinding) on sdk-out-of-gas events. PR #67 currently fixes the UB, but makes the out-of-gas event more opaque than it was, until the above changes are implemented.

[ethanfrey]

That does make the panics more secure, but to solve this info we must somehow update (reduce) the remaining gas allowed for the wasmer instance after each call out to the go storage code.

Currently, the go panic unwrapping over rust then back to go does seem to work. In fact, we do test this case: https://github.com/CosmWasm/wasmd/blob/master/x/wasm/internal/keeper/keeper_test.go#L420-L436

Currently this just "magically" works, and yes, it is relying on UB. The above suggestion would correctly implement this in a more stable way, but not change the functionality. In fact, That will allow both cases - SDK out of gas, and contract out of gas - to be represented the same way is already the case.

I am happy for the above PR, but really it is a different issue. This issue requires updating gas_used in wasmer with each call out to the sdk, so it is aware of combined sdk and wasmer gas usage - which will actually make it much less likely that the panic ever happens in go, as wasmer will usually enforce the limit before it hits a go panic.

[reuvenpo]

This isn't finished yet, we just decided to merge the progress in the PR mentioned here.

[ethanfrey]

Actually, I think it would be interesting to change the API interfaces to return something like:

FfiError<(i32, u32, u32)> - where the result is (original result flag, gas used to be charged to vm, gas used to be reduced from limit). I am not sure if we can differentiate between the two types of gas, but this is my ideas:

• We start with eg. 20k sdk gas in the beginning and grant 2 million wasm gas
• We run some wasm code and it marks 100k used (of 2 million limit)
• We call out to human_address and it returns 10k gas used, which is NOT recorded on the Go side, but IS recorded in the wasmer vm (not sent to the contract). In this case, we now have used 110k of 2 million limit.
• We now call out to a storage API, which uses 6k SDK gas = 600k wasm gas, which is deducted from the context there. When getting the return value, we cannot mark this as used gas, as that would double-charge, but we should reduce the limit - so we no have used 110k wasm gas with a limit of 1.4 million.

We no longer need to worry about OutOfGas error as long as we return proper gas prices, the wasm contract should hit the out of gas state before (and simultaneous to) the runtime. And we never get double charges, cuz if we used the gas in the sdk storage, we will never spend the same gas in the contract (and this also tightens up abuse of the api calls).

[ethanfrey]

The limit vs used gas measurement should be in the API as it is correct, but it not yet supported in wasmer. Here is a 6 month old PR that seems to have stagnated, but I will try to revive it: wasmerio/wasmer#996

[reuvenpo]

Three notes:

1. You still could have a case where gas runs out during access to storage, but that's ok.
2. Is there a way to check how much gas was used in the SDK before returning from Go to Rust?
3. Until that issue in wasmer is solved we can't really do this, unless we can check how much gas was used in the contract before returning from an import. If we can, then we can detect that we ran out of gas manually, and Trap with a VmError::GasDepletion in the import, and catch that outside.

[ethanfrey]

1. You still could have a case where gas runs out during access to storage, but that's ok.

Yes, we can handle out of gas as you plan to, this adds finer-grain measurements

2. Is there a way to check how much gas was used in the SDK before returning from Go to Rust?

I am not sure for the storage usage. I think it would require more arguments in the go_cosmwasm::DB type but at least we can start doing this for the API functions, which just define their gas price, and investigate for the querier functions.

Simply deducting gas (incrementing gas_used) on the Context is rather easy and would then possibly trigger out of gas error inside wasmer middleware after returning (this trap already happens, no need to write that ourselves).

I agree not everything is in place, but adding a gas price to each api call and then deducting it should allow us to block one abuse potential and test out this gas metering in the easiest location (no deep integration with cosmos sdk gas metering)

[reuvenpo]

I don't know the Cosmos APIs that well, but if there's a way to query the gas-used-so-far in the SDK, then we can measure it before and after the storage access to deduce the amount of gas it consumed.

[ethanfrey]

If we have access to sdk.Context we can get the amount of gas (yes, before and after).

We pass in a KVStore, which somehow has bonded the sdk.Context (and it's gas meter) internally and updates that. I don't think there is a way to get this info from the KVStore itself, so we would have to pass in the sdk.Context to the DB FFI object as well, so we can reconstruct it on the Go side to do the proper gas measurements.

Thus, trying with api first (which doesn't require such hoops, just a return value), would be a nice step to see how to do it, then we can do it fully.

[reuvenpo]

This will be fixed by these PRs together:

• added interpretation and unwrapping of wasmer_runtime_core::error::RuntimeError cosmwasm#359
• added correct handling of out-of-gas events in cosmwasm-vm #90
• adjusted gas consumption logic in case of errors in go-cosmwasm wasmd#115

[ethanfrey]

Those PRs were merged and this is now done!

Thanks @reuvenpo