-
Notifications
You must be signed in to change notification settings - Fork 138
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
Harden actor deletion (and some other errors) #273
Conversation
act.sequence += 1; | ||
Ok(()) | ||
})?; | ||
self.state_tree_mut().mutate_actor_id(sender_id, |act| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small optimization.
.or_error(ExitCode::ErrIllegalArgument)?; | ||
.or_illegal_argument()?; | ||
|
||
if !is_account_actor(&act.code) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not directly related to deletion bug. But we would have previously returned a fatal error here if we attempted to "resolve" a non-account ID address.
@@ -876,6 +900,9 @@ fn verify_seal(vi: &SealVerifyInfo) -> Result<bool> { | |||
bytes_32(&vi.interactive_randomness.0), | |||
&vi.proof, | |||
) | |||
.or_fatal() | |||
.context("failed to verify seal proof") // TODO: Verify that this is actually a fatal error. | |||
.or_illegal_argument() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another drive-by fix.
@@ -244,18 +245,17 @@ where | |||
.into()); | |||
} | |||
|
|||
// TODO: make sure these are actually fatal. | |||
let mut from_actor = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can hit this if the actor has been deleted.
fvm/src/machine/default.rs
Outdated
.ok_or_else(|| anyhow!("receiver actor does not exist in state during transfer")) | ||
.or_fatal()?; | ||
.context("cannot transfer to non-existent receiver") | ||
.ok_error(ExitCode::SysErrInvalidReceiver)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably shouldn't hit this, but it can't hurt.
@@ -121,7 +121,7 @@ pub trait BlockOps { | |||
/// Depends on BlockOps to read and write blocks in the state tree. | |||
pub trait SelfOps: BlockOps { | |||
/// Get the state root. | |||
fn root(&self) -> Cid; | |||
fn root(&self) -> Result<Cid>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to return an error if the actor no longer exists in the state-tree.
a8c9e5e
to
1f98971
Compare
These cases should be unreachable in our current actors, but would not have been unreachable in user programmable actors. Fixes #185 by _not_ returning fatal errors if the current actor doesn't exist in state. After deletion: 1. Actors have "zero" balance. 2. Getting & setting the root fails with "illegal actor". 3. Sending still works, but sending with a value will fail because the balance is zero. 4. Sending _to_ this actor will fail. Effectively, I'm treating "deletion" as "unlinking" the actor in the state-tree. It still _exists_ until it returns, it just can't be looked-up or linked back into the state.
1f98971
to
a7319ab
Compare
These cases should be unreachable in our current actors, but would not have been unreachable in user programmable actors.
Fixes #185 by not returning fatal errors if the current actor doesn't exist in state.
After deletion:
Effectively, I'm treating "deletion" as "unlinking" the actor in the state-tree. It still exists until it returns, it just can't be looked-up or linked back into the state.
NOTE: the exact error codes will change with filecoin-project/fvm-specs#53.