-
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
use std::collections::BTreeMap; | ||
use std::convert::{TryFrom, TryInto}; | ||
|
||
use anyhow::anyhow; | ||
use anyhow::{anyhow, Context as _}; | ||
use byteorder::{BigEndian, WriteBytesExt}; | ||
use cid::Cid; | ||
use filecoin_proofs_api::seal::{ | ||
|
@@ -26,6 +26,7 @@ use rayon::iter::{IndexedParallelIterator, IntoParallelRefIterator, ParallelIter | |
use super::blocks::{Block, BlockRegistry}; | ||
use super::error::Result; | ||
use super::*; | ||
use crate::account_actor::is_account_actor; | ||
use crate::builtin::{is_builtin_actor, is_singleton_actor, EMPTY_ARR_CID}; | ||
use crate::call_manager::{CallManager, InvocationResult}; | ||
use crate::externs::{Consensus, Rand}; | ||
|
@@ -110,14 +111,21 @@ where | |
let act = state_tree | ||
.get_actor(addr)? | ||
.ok_or(anyhow!("state tree doesn't contain actor")) | ||
.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 commentThe 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. |
||
return Err( | ||
syscall_error!(SysErrIllegalArgument; "target actor is not an account").into(), | ||
Stebalien marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
} | ||
|
||
let state: crate::account_actor::State = state_tree | ||
.store() | ||
.get_cbor(&act.state) | ||
.or_fatal()? | ||
.context("failed to decode actor state as an account") | ||
.or_fatal()? // because we've checked and this should be an account. | ||
.ok_or(anyhow!("account actor state not found")) | ||
.or_fatal()?; | ||
.or_fatal()?; // because the state should exist. | ||
|
||
Ok(state.address) | ||
} | ||
|
@@ -157,51 +165,66 @@ where | |
let (market_state, _) = MarketActorState::load(self.call_manager.state_tree())?; | ||
Ok(market_state.total_locked()) | ||
} | ||
|
||
/// Returns `Some(actor_state)` or `None` if this actor has been deleted. | ||
fn get_self(&self) -> Result<Option<ActorState>> { | ||
self.call_manager | ||
.state_tree() | ||
.get_actor_id(self.actor_id) | ||
.or_fatal() | ||
.context("error when finding current actor") | ||
} | ||
|
||
/// Mutates this actor's state, returning a syscall error if this actor has been deleted. | ||
fn mutate_self<F>(&mut self, mutate: F) -> Result<()> | ||
where | ||
F: FnOnce(&mut ActorState) -> Result<()>, | ||
{ | ||
self.call_manager | ||
.state_tree_mut() | ||
.maybe_mutate_actor_id(self.actor_id, mutate) | ||
.context("failed to mutate self") | ||
.and_then(|found| { | ||
if found { | ||
Ok(()) | ||
} else { | ||
Err(syscall_error!(SysErrIllegalActor; "actor deleted").into()) | ||
} | ||
}) | ||
} | ||
} | ||
|
||
impl<C> SelfOps for DefaultKernel<C> | ||
where | ||
C: CallManager, | ||
{ | ||
fn root(&self) -> Cid { | ||
let addr = Address::new_id(self.actor_id); | ||
let state_tree = self.call_manager.state_tree(); | ||
|
||
state_tree | ||
.get_actor(&addr) | ||
.unwrap() | ||
.expect("expected actor to exist") | ||
.state | ||
fn root(&self) -> Result<Cid> { | ||
// This can fail during normal operations if the actor has been deleted. | ||
Ok(self | ||
.get_self()? | ||
.context("state root requested after actor deletion") | ||
.or_error(ExitCode::SysErrIllegalActor)? | ||
.state) | ||
} | ||
|
||
fn set_root(&mut self, new: Cid) -> Result<()> { | ||
let addr = Address::new_id(self.actor_id); | ||
let state_tree = self.call_manager.state_tree_mut(); | ||
|
||
state_tree.mutate_actor(&addr, |actor_state| { | ||
self.mutate_self(|actor_state| { | ||
actor_state.state = new; | ||
Ok(()) | ||
})?; | ||
|
||
Ok(()) | ||
}) | ||
} | ||
|
||
fn current_balance(&self) -> Result<TokenAmount> { | ||
let addr = Address::new_id(self.actor_id); | ||
let balance = self | ||
.call_manager | ||
.state_tree() | ||
.get_actor(&addr) | ||
.or_fatal() | ||
.context("error when finding current actor")? | ||
.ok_or(anyhow!("state tree doesn't contain current actor")) | ||
.or_fatal()? | ||
.balance; | ||
Ok(balance) | ||
// If the actor doesn't exist, it has zero balance. | ||
Ok(self | ||
.get_self()? | ||
.map(|a| a.balance) | ||
.unwrap_or_else(|| TokenAmount::zero())) | ||
} | ||
|
||
fn self_destruct(&mut self, beneficiary: &Address) -> Result<()> { | ||
// TODO abort with internal error instead of returning. | ||
// Idempotentcy: If the actor doesn't exist, this won't actually do anything. The current | ||
// balance will be zero, and `delete_actor_id` will be a no-op. | ||
self.call_manager | ||
.charge_gas(self.call_manager.price_list().on_delete_actor())?; | ||
|
||
|
@@ -211,9 +234,10 @@ where | |
// exists; if missing, it fails the self destruct. | ||
// | ||
// In FVM we check unconditionally, since we only support nv13+. | ||
let beneficiary_id = self.resolve_address(beneficiary)?.ok_or_else(|| | ||
// TODO this should not be an actor error, but a system error with an exit code. | ||
syscall_error!(SysErrIllegalArgument, "beneficiary doesn't exist"))?; | ||
let beneficiary_id = self | ||
.resolve_address(beneficiary)? | ||
.context("beneficiary doesn't exist") | ||
.or_error(ExitCode::SysErrIllegalArgument)?; | ||
|
||
if beneficiary_id == self.actor_id { | ||
return Err(syscall_error!( | ||
|
@@ -230,7 +254,6 @@ where | |
} | ||
|
||
// Delete the executing actor | ||
// TODO errors here are FATAL errors | ||
self.call_manager | ||
.state_tree_mut() | ||
.delete_actor_id(self.actor_id) | ||
|
@@ -876,6 +899,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 commentThe reason will be displayed to describe this comment to others. Learn more. Another drive-by fix. |
||
// TODO: There are probably errors here that should be fatal, but it's hard to tell so I'm | ||
// sticking with illegal argument for now. | ||
// Worst case, _some_ node falls out of sync. Better than the network halting. | ||
.context("failed to verify seal proof") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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. |
||
|
||
/// Update the state-root. | ||
/// | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ use fvm_shared::address::Address; | |
use fvm_shared::blockstore::{Blockstore, Buffered}; | ||
use fvm_shared::clock::ChainEpoch; | ||
use fvm_shared::econ::TokenAmount; | ||
use fvm_shared::error::ExitCode; | ||
use fvm_shared::version::NetworkVersion; | ||
use fvm_shared::ActorID; | ||
use log::Level::Trace; | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. We can hit this if the actor has been deleted. |
||
.state_tree | ||
.get_actor_id(from)? | ||
.ok_or_else(|| anyhow!("sender actor does not exist in state during transfer")) | ||
.or_fatal()?; | ||
.context("cannot transfer from non-existent sender") | ||
.or_error(ExitCode::SysErrSenderInvalid)?; | ||
|
||
let mut to_actor = self | ||
.state_tree | ||
.get_actor_id(to)? | ||
.ok_or_else(|| anyhow!("receiver actor does not exist in state during transfer")) | ||
.or_fatal()?; | ||
.context("cannot transfer to non-existent receiver") | ||
.or_error(ExitCode::SysErrInvalidReceiver)?; | ||
|
||
from_actor.deduct_funds(value).map_err(|e| { | ||
syscall_error!(SysErrInsufficientFunds; | ||
|
@@ -264,12 +264,8 @@ where | |
})?; | ||
to_actor.deposit_funds(value); | ||
|
||
// TODO turn failures into fatal errors | ||
self.state_tree.set_actor_id(from, from_actor)?; | ||
// .map_err(|e| e.downcast_fatal("failed to set from actor"))?; | ||
// TODO turn failures into fatal errors | ||
self.state_tree.set_actor_id(to, to_actor)?; | ||
//.map_err(|e| e.downcast_fatal("failed to set to actor"))?; | ||
|
||
log::trace!("transfered {} from {} to {}", value, from, to); | ||
|
||
|
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.