EVM: Implement reverts correctly #864
Conversation
vyzo
force-pushed
the
next-revert-proper
branch
from
7a1228b to
da1cc9f
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## next #864 +/- ##
==========================================
- Coverage 87.32% 87.31% -0.02%
==========================================
Files 125 125
Lines 22784 22896 +112
==========================================
+ Hits 19897 19991 +94
- Misses 2887 2905 +18
|
| A::invoke_method(self, method_num, params) | ||
| })) | ||
| .unwrap_or_else(|panic| { | ||
| if self.actor_exit.borrow().is_some() { |
There was a problem hiding this comment.
Let's instead embed the exit inside the error, and cast with https://doc.rust-lang.org/std/any/trait.Any.html#method.downcast. That way, we can't accidentally swallow up a real panic.
There was a problem hiding this comment.
Basically, we can use panic to "throw" the exit value.
There was a problem hiding this comment.
Does this even make sense here?
We set the exit and immediately panic, which lands here; any other panic will be resumed. So the mentioned condition is not really possible, and it is nicer to the eye in tests since cargo will still report these panics at runtime in the console.
Now, if this was prod code, i would definitely consider the fancy way.
Here, i dont think it adds anything but complexity, really.
Having said all that, if you feel overly strongly about that and insist, i will abide.
There was a problem hiding this comment.
Eh, you're right. There's little benefit.
| precompiles::Precompiles::call_precompile(system.rt, dst, input_data) | ||
| .map_err(|_| StatusCode::PrecompileFailure)? | ||
| match precompiles::Precompiles::call_precompile(system.rt, dst, input_data) | ||
| .map_err(|_| StatusCode::PrecompileFailure) |
There was a problem hiding this comment.
This will need to be re-done when merging with #861, since there are cases where we want to return to a status code error and others where we have a precompile error
There was a problem hiding this comment.
Sure, i'll leave that up to you.
There was a problem hiding this comment.
Well, it has already been merged.
There was a problem hiding this comment.
Yeah i noticed. I will rebase and we can discuss what do here when we are happy with the rest.
There was a problem hiding this comment.
ok, merged, very little to do here other than resolve the conflict.
| @@ -634,7 +639,7 @@ pub fn trampoline<C: ActorCode>(params: u32) -> u32 { | |||
| let mut rt = FvmRuntime::default(); | |||
| // Invoke the method, aborting if the actor returns an errored exit code. | |||
| let ret = C::invoke_method(&mut rt, method, ¶ms) | |||
| .unwrap_or_else(|err| fvm::vm::abort(err.exit_code().value(), Some(err.msg()))); | |||
| .unwrap_or_else(|err| fvm::vm::exit(err.exit_code().value(), err.data(), Some(err.msg()))); | |||
nested call revert should go in integration test, really
vyzo
force-pushed
the
next-revert-proper
branch
from
9c4e443 to
3f91e75
Compare
|
rebased on next to merge the precompile/callactor work and did a very minor dedup simplification in 3f91e75. |
Adjust implementation so that
TBD:
Notes:
reverts for direct invocations are tested with a unit test; reverts in nested invocations are tested with the callvariants test in the integration test suite.