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

[webmaster128]

During review, I found it hard to follow test code that included instance.set_gas calls. This seems to be an unnatural use of an instance. I could easily rewrite the tests in a way that set the gas limit once and calculate diffs using get_gas.

However, this PR does not work well together with the LRU cache. In

        // pop from lru cache if present
        if let Some(cache) = &mut self.instances {
            let val = cache.pop(&hash);
            if let Some(inst) = val {
                inst.leave_storage(Some(deps.storage));
                return Ok(inst);
            }
        }

it is not possible to reset the gas limit due API I want to remove.

I wonder if you have a good design around this. Re-using an "instance" seems to be counter-intuitive. I was thinking about caching raw wasmer_runtime_core::Instances. 👍/👎/thoughts?

[ethanfrey]

The whole set_gas stuff is a bit of a mess, as there is no way to change the gas limit (which is what I want), so we need to compile it with a huge one, then use up a bunch of gas before calling.

There is an open PR to make this much more intuitive (just set_limit) wasmerio/wasmer#996 but it seems to be hanging without merge for 2 months. I can ask about it.

As to reusing instances, I did this for performance. But actually disable the LRU by default now in wasmd for concerns on stability. I would like to go through with this most recent version and try to benchmark all the steps and see what savings we get from caching instances, rather than just caching the precompile (until wasmer 0.12, singlepass precompiles could not be persisted/cached, making this more important).

We could also cache raw wasmer::Instance. That does make sense. Just the raw vm. However, I would like to wait a bit before changing this code and tackle it when there are a few issues to do at once and then give a deeper inspection and refactoring. (The set_limit is just one issue... I don't like how I handle Storage, but I see no other way)

[webmaster128]

The whole set_gas stuff is a bit of a mess

The good thing is that the mess is nicely encapsulated in backends::set_gas, which works as expected and can remain untauched here.

We could also cache raw wasmer::Instance. That does make sense. Just the raw vm. However, I would like to wait a bit before changing this code and tackle it when there are a few issues to do at once and then give a deeper inspection and refactoring.

Created an issue for that: #137

[ethanfrey]

In general, the approach makes sense, but I think we need the other issue to make this work (wasmer::Instance in cache). Note the gas limit is set incorrectly after reading from cache

[ethanfrey]

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.

[webmaster128]

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

[webmaster128]

Done in 201829c

[ethanfrey]

I guess this is unblocked by #140

Let me know if you finish it up / reimplememt

[webmaster128]

This is ready from my side

[ethanfrey]

Lgtm

[ethanfrey]

Safer for sure

[webmaster128]

Without that, I got overflow crashes

[ethanfrey]

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

[ethanfrey]

Yes. Looks proper