Open Stebalien opened 2 years ago
from zen: make sense to do before the audit, but shouldn't be a blocker
I've spent some time investigating how to improve things here, but it turns out to be quite complex. In our current situation we have:
[Result].map_err(|e| e.downcast_default(...))
for adapting non-ActorErrors into ActorError. It's quite verbose.AsActorError
implemented for Result<T, E: Display>
with context_code()
and similar methods to adapt into an Result<T, ActorError>
with a specified exit code. These are concise to use but don't attempt any downcasting to discover wrapped exit codes.Some problems with this state:
fvm_ipld_kamt::Error
and the frc46 token StateError
(which can wrap an fvm_ipld_hamt::Error
). In Rust, we'd need to import all these error types into the module defining the trait in order to implement it, and could easily miss new ones. The approach doesn't scale to more different error types.AsActorError::context_code()
can fail to propagate an ActorError nested inside, e.g. hamt::Error::Dynamic
.ActorError
implements Display
, the context_code()
method can be inadvertently used to overwrite an exit code even when it's not hidden inside another error typeWhy do we have downcasting at all? There are some places where we can generate an ActorError with exit code but not propagate it directly, usually due to library code. E.g. the HAMT/AMT for_each()
method with return an Error::Dynamic
containing an anyhow::Error
that's actually an ActorError
raised from the callback.
In #1322 I tried replacing a bunch of map and downcast patterns with more concise trait methods. It's on hold at present because a few tests fail due to changes in exit codes, as a code generated inside for_each
was trashed (e.g. this USR_ILLEGAL_ARGUMENT in remove partitions).
So what shall we do? Some options:
?
. Something like leaning into #1338?
propagation, but usually only ILLEGAL_STATE. Other exit codes can be generated in top-level actor code. Something like #1322.abort()
syscall available in state methods (perhaps via a new very slim runtime trait passed to them) and call it directly in state code, with no opportunity for actor code to handle the error or adapt the exit code. Implement mock runtime and test VM etc to use panic or similar to support this.E.g. the HAMT/AMT for_each() method with return an Error::Dynamic containing an anyhow::Error that's actually an ActorError raised from the callback.
We should implement iteration with external iterators instead to fix this issue. I have a WIP patch locally that I can try to cleanup and ship if there's interest.
That should remove a large part of this issue.
Give up on preserving exit codes generated low down in the code.
In many cases, I believe this is the right thing to do. Most of these errors should just be "illegal state".
Implement specific error types for state objects that enumerate error conditions. Stop returning ActorError from state methods, instead only generate exit code in top-level actor business logic.
IMO, this is the "correct" solution. It's just really annoying which is why most rust users just use crates like anyhow
and yolo it.
But yeah, we can do this where it matters. I.e.:
From<CustomErrorType> for ActorError
wherever we care about conversions.Make the abort() syscall available in state methods (perhaps via a new very slim runtime trait passed to them) and call it directly in state code, with no opportunity for actor code to handle the error or adapt the exit code. Implement mock runtime and test VM etc to use panic or similar to support this.
I like the idea of aborting early but... I'd prefer not to do this in all cases:
abort
directly if some state invariant is violated. Specifically: state block doesn't exist, state fails to decode, etc.abort
directly if, e.g., the user passes an illegal argument, something isn't found, etc. (the actor may want to react to this in some way?).However, I'm concerned about using the runtime here as it may be hard to thread it through to the very bottom (and makes it harder to reason about what the code can do). Instead, I'd consider exposing it as a bare function that:
exit
syscall otherwise.External iterators would be great. And then we could "give up" on doing better than "illegal state" for remaining hard cases.
Your proposal re aborting sounds reasonable, and could clean up some boilerplate. In mechanism, why not just call the runtime's 'abort' anyway and have that panic (and catch it out the other side). Conditional compilation and static cling already causes headaches in this repo, I would rather avoid adding any more that we can.
Currently, we have a lot of "anyhow" errors and rely on error downcasting to figure out the right exit code. Unfortunately, this makes it very difficult to figure out what the exit code should be.
Instead, we'd ideally: