Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove downcasting and use explicit errors #214

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Apr 12, 2022

  • This removes all downcast_ functionality, replacing it with explicit errors wherever possible. (*)
  • For adding error context, there is now .context and .with_context on Result<T, ActorError>, such that this can be preserved
  • anyhow is not used anymore, except for testing (*)
  • The matching of error to exit code is now mostly done in the From implementations for ActorError, or at the very source inside actors where an error is first encountered that is not an ActorError.

(*) Except in the Runtime which will go away once proper Runtime errors are introduced.

@jennijuju jennijuju requested review from raulk and anorth April 12, 2022 16:27
@dignifiedquire dignifiedquire changed the title [WIP] remove downcasting and use explicit errors Remove downcasting and use explicit errors Apr 12, 2022
@dignifiedquire dignifiedquire marked this pull request as ready for review April 12, 2022 16:49
@@ -63,14 +63,14 @@ members = [
## Uncomment entries below when working locally on ref-fvm and this repo simultaneously.
## Assumes the ref-fvm checkout is in a sibling directory with the same name.
## (Valid once FVM modules are published to crates.io)
# [patch.crates-io]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: undo once a new release with the changes are cut for ref-fvm

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a much better way of doing things, but it changes some behaviour. We need to trade-off consistency with the old/Go code with this better way of doing things. The unit tests are going to catch some of the inconsistencies as the tests are ported from Go. But they won't cover all cases. I think resolving this is the big question here. It will be much simpler to ensure alignment if the adapting to a concrete exit code is generally done higher up in the call stack, in line with the Go code (and the old downcast calls).

I'm not so sure about pushing ActorError down into structures like the bitfield queue. This queue is a pure data structure, an interpretation of an AMT. Most of the possible exit codes don't make sense there – they're addressed at application level. The Go code makes no use of xc to set exit codes. I would probably treat these structures similar to the H/AMT and return concrete error types.

For structures like deadline state it's a bit more borderline, I could see either way working and this change is probably for the better. In Go there are some cases where the exit code is determined at that level (usually ErrIllegalArgument), but most just return an opaque error that turns into ErrIllegalState.

Err(format!("New balance in table cannot be negative: {}", sum).into())
} else if sum.is_zero() && !prev.is_zero() {
return Err(actor_error!(
illegal_argument,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The challenge with moving the exit codes a bit lower will be ensuring consistency with the Go code. I traced this one as an example, and on all the paths I could find this results in ErrIllegalState in the Go implementation. Indeed, I'd say ErrIllegalState is your go-to for anything lower than the top-level actor calls, but those top-level call sites might sometimes be able to adapt into a better exit code.

ErrIllegalArgument is mostly only used at the top level when checking parameters directly provided by the user.

On this pass through I'm not going to check them all, but I'd say the default should be illegal_state to get maximal matching with current code. And then we need to decide how much we care about exactly matching the exit codes. We'll need to do so for all practical abort paths in order to maintain sync with mainnet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wouldn't push actor errors down the stack like this as it can be difficult to pick a sane default. Really, the correct error here is likely "assertion failed".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may be able to get away with more implicit conversions and custom error types?

e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to construct init actor state")
})?;
let state = State::new(rt.store(), params.network_name)
.context("failed to construct init actor state")?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice

if deadline_idx >= policy.wpost_period_deadlines {
return Err(anyhow!("invalid deadline {}", deadline_idx));
return Err(actor_error!(illegal_argument, "invalid deadline {}", deadline_idx));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting case that's illegal state in Go (by way of no exit code being specified at this level), but using illegal argument would be better.

// Check for sector intersection. This could be cheaper with a combined intersection/difference method used below.
if !self.on_time_sectors.contains_all(on_time_sectors) {
return Err(anyhow!(
return Err(actor_error!(
illegal_argument,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another interesting example case. This is illegal state in Go, and that's more correct because the cause of this is some inconsistency or broken invariants in the actor's state. This doesn't result from a bad parameter provided by top-level caller.

@dignifiedquire
Copy link
Contributor Author

Added a custom error for bitfield_queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants