Out of gas handling #331

reuvenpo · 2020-05-13T17:31:44Z

This PR aims to fix CosmWasm/wasmvm#29 .
It adds all the support required in the cosmwasm-vm crate (which is a lot)

…storage.rs

…sm#331

…er VM instance

…sm#331

reuvenpo

Just went over my diff again and added a few notes. Most of these are for myself but one is for the other reviewers.

packages/vm/src/context.rs

packages/vm/src/imports.rs

webmaster128

Very nice work, great points.

I guess it will be hard to update. Maybe we can split this PR a little bit into smaller chunks to get them in?

contracts/hackatom/tests/integration.rs

packages/std/src/errors/system_error.rs

packages/vm/src/context.rs

packages/vm/src/imports.rs

packages/vm/src/instance.rs

packages/vm/src/mock/storage.rs

reuvenpo · 2020-05-15T11:10:26Z

Very nice work, great points.

Thanks! :D It took a lot of time to figure out a lot of the subtleties here, as you've seen so far.

I guess it will be hard to update. Maybe we can split this PR a little bit into smaller chunks to get them in?

My plan for Sunday is to churn through all the conflicts i'll get after running git merge origin/master. Can't really see an easier way to move forward since i know how much more i need to do here anyways.

webmaster128 · 2020-05-15T11:35:07Z

Okay, let's try to get this in first. After it is merged there are a few action items (not necessarily for you) good to get done on Monday (for 0.8.0):

in cosmwasm-vm, rename MemoryStorage to MockStorage since it is not reused anymore
in cosmwasm-vm, export everything that is related to testing (such as the mock stuff) in the ::testing module, like we do in cosmwasm-std
move MemoryStorage from cosmwasm-std to cosmwasm-storage
update Use FnOnce in with_*_from_context #333

reuvenpo · 2020-05-15T11:50:11Z

Cool 👍
Please keep in mind that i also have more items to work on in this PR before i would consider the gas issue fully fixed. I don't think that i'd be able to act on all the comments so far, merge master into this PR, and finish the gas handling all in one day.

webmaster128 · 2020-05-15T11:52:08Z

@reuvenpo no worries. Amazing work has been done already and my focus is to reduce the diff, allowing us to progress in parallel from there.

reuvenpo · 2020-05-17T20:57:24Z

I (believe that I) fixed everything we discussed in this PR on Friday
I merged latest master into this branch and fixed all the tests. Clippy is complaining because of an unused function: it's a function you defined so i want your feedback on whether i can safely remove it.
Please comment on the few issue that are still left open above 🙏

packages/vm/src/errors.rs

packages/std/src/errors/system_error.rs

packages/vm/src/imports.rs

webmaster128 · 2020-05-18T07:39:00Z

Clippy is complaining because of an unused function: it's a function you defined so i want your feedback on whether i can safely remove it.

make_backend_err becoming unused is a good sign. It means FfiError covers all cases where an error occurs when the VM calls into the backend (chain). Could you remove make_backend_err and VmError::BackendErr?

reuvenpo · 2020-05-18T09:13:28Z

Could you remove make_backend_err and VmError::BackendErr?

Done :)

ethanfrey

Nice work on the major overhaul. There seem to be multiple different concerns in this PR, but good stuff in general. Added some comments on structure.

packages/std/src/errors/system_error.rs

packages/vm/src/context.rs

ethanfrey · 2020-05-18T09:03:14Z

packages/vm/src/errors.rs

+    },
+}
+
+impl FfiError {


What is this for? I generally don't like the idea of modifying errors.
Maybe just a simpler constructor ffi_error(message: S) -> FfiError that returns an FfiError::Other variant?

Oh, this isn't a constructor. you can see how i use it, it's kind of like the context method from snafu::ResultExt. it basically says "If the value is a specific variant of the type, please give it this additional data". In this case i say that if it can, please add this message with context information. It's used to give a specific error message when go has an unexpected error (not panic) and we have something useful to say.

Can this be implemented as a constructor, i.e. keep the error immutable? Or do we get the error from somewhere and need to add the information later on?

Uhh, i suppose it could be? Let me play around with it. If you look at the places this is used, we first convert a GoResult to Result<(), FfiError>, and then if we got Other we add an error message with this method. I can wrap that up into an associated function on GoResult, perhaps.

Not too important in my opinion. Wrote it down here: #336

packages/vm/src/instance.rs

ethanfrey · 2020-05-18T09:10:22Z

packages/vm/src/mock/storage.rs

+        start: Option<&[u8]>,
+        end: Option<&[u8]>,
+        order: Order,
+    ) -> FfiResult<Box<dyn Iterator<Item = FfiResult<KV>> + 'a>> {


Okay, so copying all this code is to give us the proper Result/Error type for the iterator (and other ops). It seems necessary to match the different function signatures, but it is also missing any code that would let use test the error cases. I would propose replacing this with the following:

A wrapper that takes cosmwasm_std::Storage and converts to cosmwasm_vm::Storage (different return types). This would convert any StdError to FfiError::Other by stringifying it. It lets us easily reuse code for the success-case.

An implementation that always returns an error, where you can set an FfiError to in the struct, and all calls will return this error (cloned). I think it is critical to test this error handling - which is a large part of the refactor

Another case for this is the fact that staking contract is failing.

https://app.circleci.com/pipelines/github/CosmWasm/cosmwasm/1049/workflows/6581f6b9-4460-42d5-83b6-3467606c2680/jobs/7649/steps

My integration test customizes the Querier (staking module), but that uses the cosmwasm_std::Extern. Having no (test-safe) translation to cosmwasm_vm::Extern makes life hard here (and a whole lot more code copying if I want to support this). I think some transforms allow the two to diverge, but let us easily re-use test helpers. Such transforms can be in the testing/mock modules to signify they should only be used for test code (like MockStorage, MockQuerier, etc)

Uhm, i just fixed the staking contract integration tests by changing an import and tweaking a function signature to reflect a change that was made after i forked. i just pushed it but everything's passing on my machine, so it should pass here in a few minutes as well.
As for the suggestion in your first comment on this thread, that sounds good, but i don't think there's a strong need to do this right now. there were already tests that checked for errors in a lot of the code i touched, and i had to modify those tests to reflect my changes. Most of what i did is already backed by tests. The only thing that i feel that i should write a test for, is the case where things run out of gas, but i don't want to do it before i've completed the full error flow for that scenario.

Yes, these were ideas for future enhancements rather than in this pr.

packages/vm/src/traits.rs

packages/vm/src/imports.rs

packages/vm/src/context.rs

webmaster128

Solid and strictly better than current master. Ready to merge from my point of view

…sm#331

copied mock.rs and storage.rs from std to vm as mock/mod.rs and mock/…

a89e9f8

…storage.rs

reuvenpo added a commit to CosmWasm/wasmvm that referenced this pull request May 13, 2020

modified trait implementation according to changes in CosmWasm/cosmwa…

63ad440

…sm#331

reuvenpo mentioned this pull request May 13, 2020

Out of gas handling CosmWasm/wasmvm#82

Merged

implemented propagation of out-of-gas events from the FFI to the wasm…

44ca77e

…er VM instance

reuvenpo force-pushed the out-of-gas-handling branch from c6f578a to 44ca77e Compare May 13, 2020 17:56

reuvenpo added a commit to CosmWasm/wasmvm that referenced this pull request May 13, 2020

modified trait implementation according to changes in CosmWasm/cosmwa…

107ca11

…sm#331

reuvenpo commented May 14, 2020

View reviewed changes

packages/vm/src/context.rs Outdated Show resolved Hide resolved

packages/vm/src/context.rs Outdated Show resolved Hide resolved

packages/vm/src/imports.rs Show resolved Hide resolved

packages/vm/src/imports.rs Outdated Show resolved Hide resolved

webmaster128 reviewed May 15, 2020

View reviewed changes

webmaster128 mentioned this pull request May 15, 2020

Use FnOnce in with_*_from_context #333

Closed

reuvenpo added 5 commits May 17, 2020 20:06

PR code review fixes

41be7ba

Merge remote-tracking branch 'origin/master' into out-of-gas-handling

9de34ae

fixed error when using the iterator feature

95ea5f1

fixed tests

87adf87

integrated all the changes from CosmWasm#333

235994f

webmaster128 reviewed May 18, 2020

View reviewed changes

reuvenpo added 2 commits May 18, 2020 12:02

removed commented code fragment from last merge

77f7b77

removed VmError::BackendErr as it is now deprecated

a9ee6ed

ethanfrey reviewed May 18, 2020

View reviewed changes

changed SystemError fields to contain Binary instead of Vec<u8>

52b8d7f

webmaster128 reviewed May 18, 2020

View reviewed changes

packages/vm/src/context.rs Show resolved Hide resolved

reuvenpo mentioned this pull request May 18, 2020

Fix out-of-gas scenario handling scrtlabs/SecretNetwork#151

Closed

2 tasks

reuvenpo added 2 commits May 18, 2020 15:51

added {} to the definition of FfiError::OutOfGas

bb24665

added {} to the definition of SystemError::Unknown

daf71dc

fixed staking/tests/integration.rs

fe40d41

This was referenced May 18, 2020

Fix cyclical reference in get_context_data #335

Closed

Make FfiError immutable #336

Closed

webmaster128 approved these changes May 18, 2020

View reviewed changes

webmaster128 merged commit 701e10e into CosmWasm:master May 18, 2020

webmaster128 mentioned this pull request May 18, 2020

Use cosmwasm_vm::testing module #339

Merged

Expotential108 added a commit to Expotential108/Binding-cosmwasm that referenced this pull request Mar 12, 2023

modified trait implementation according to changes in CosmWasm/cosmwa…

c78e09f

…sm#331

delneg pushed a commit to SigmaGmbH/swisstronik-librustgo that referenced this pull request Jul 25, 2023

modified trait implementation according to changes in CosmWasm/cosmwa…

29decd6

…sm#331

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Out of gas handling #331

Out of gas handling #331

reuvenpo commented May 13, 2020 •

edited

Loading

reuvenpo left a comment

webmaster128 left a comment

reuvenpo commented May 15, 2020

webmaster128 commented May 15, 2020 •

edited

Loading

reuvenpo commented May 15, 2020

webmaster128 commented May 15, 2020 •

edited

Loading

reuvenpo commented May 17, 2020 •

edited

Loading

webmaster128 commented May 18, 2020 •

edited

Loading

reuvenpo commented May 18, 2020

ethanfrey left a comment

ethanfrey May 18, 2020

reuvenpo May 18, 2020

webmaster128 May 18, 2020

reuvenpo May 18, 2020

webmaster128 May 18, 2020

ethanfrey May 18, 2020

ethanfrey May 18, 2020

reuvenpo May 18, 2020

ethanfrey May 18, 2020

webmaster128 left a comment

Out of gas handling #331

Out of gas handling #331

Conversation

reuvenpo commented May 13, 2020 • edited Loading

reuvenpo left a comment

Choose a reason for hiding this comment

webmaster128 left a comment

Choose a reason for hiding this comment

reuvenpo commented May 15, 2020

webmaster128 commented May 15, 2020 • edited Loading

reuvenpo commented May 15, 2020

webmaster128 commented May 15, 2020 • edited Loading

reuvenpo commented May 17, 2020 • edited Loading

webmaster128 commented May 18, 2020 • edited Loading

reuvenpo commented May 18, 2020

ethanfrey left a 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

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

webmaster128 left a comment

Choose a reason for hiding this comment

reuvenpo commented May 13, 2020 •

edited

Loading

webmaster128 commented May 15, 2020 •

edited

Loading

webmaster128 commented May 15, 2020 •

edited

Loading

reuvenpo commented May 17, 2020 •

edited

Loading

webmaster128 commented May 18, 2020 •

edited

Loading