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

EVM: fix EXTCODE_ ops #870

Merged
merged 16 commits into from
Dec 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 23 additions & 15 deletions actors/evm/src/interpreter/instructions/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ use fvm_shared::{address::Address, METHOD_SEND};

use crate::interpreter::precompiles::PrecompileContext;

use super::ext::{get_cid_type, get_evm_bytecode_cid, ContractType};

use {
super::memory::{copy_to_memory, get_memory_region},
crate::interpreter::address::EthAddress,
Expand Down Expand Up @@ -255,22 +257,28 @@ pub fn call_generic<RT: Runtime>(
system.send_with_gas(&dst_addr, method, params, value, gas_limit, read_only)
}
}
CallKind::DelegateCall => {
// first invoke GetBytecode to get the code CID from the target
let code = crate::interpreter::instructions::ext::get_evm_bytecode_cid(
system.rt, dst,
)?;

// and then invoke self with delegate; readonly context is sticky
let params = DelegateCallParams { code, input: input_data.to_vec() };
system.send(
&system.rt.message().receiver(),
Method::InvokeContractDelegate as u64,
RawBytes::serialize(&params)?,
TokenAmount::from(&value),
)
}
CallKind::DelegateCall => match get_cid_type(system.rt, dst) {
Copy link
Member

Choose a reason for hiding this comment

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

I removed the closure/return. We didn't need them.

Generally speaking, jumping like that is a "last resort" kind of thing as it can make code really hard to follow. It also increases diffs by changing indent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah diff was gross for sure. In all honesty this method ended up a lot cleaner than i was thinking (idk what i was thinking tbh). I probably should've given it at least a few minutes of poking before dismissing it as unfeasible.

ContractType::EVM(dst_addr) => {
// If we're calling an actual EVM actor, get it's code.
let code = get_evm_bytecode_cid(system.rt, &dst_addr)?;

// and then invoke self with delegate; readonly context is sticky
let params = DelegateCallParams { code, input: input_data.into() };
system.send(
&system.rt.message().receiver(),
Method::InvokeContractDelegate as u64,
RawBytes::serialize(&params)?,
TokenAmount::from(&value),
)
}
// If we're calling an account or a non-existent actor, return nothing because
// this is how the EVM behaves.
Copy link
Member

Choose a reason for hiding this comment

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

I changed this to return "ok" in these cases.

ContractType::Account | ContractType::NotFound => Ok(RawBytes::default()),
// If we're calling a "native" actor, always revert.
ContractType::Native(_) => {
Err(ActorError::forbidden("cannot delegate-call to native actors".into()))
}
},
CallKind::CallCode => Err(ActorError::unchecked(
EVM_CONTRACT_EXECUTION_ERROR,
"unsupported opcode".to_string(),
Expand Down
87 changes: 57 additions & 30 deletions actors/evm/src/interpreter/instructions/ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,22 @@ use {
fil_actors_runtime::runtime::Runtime,
};

#[inline]
pub fn extcodesize(
_state: &mut ExecutionState,
system: &System<impl Runtime>,
addr: U256,
) -> Result<U256, StatusCode> {
// TODO we're fetching the entire block here just to get its size. We should instead use
// TODO (M2.2) we're fetching the entire block here just to get its size. We should instead use
// the ipld::block_stat syscall, but the Runtime nor the Blockstore expose it.
// Tracked in https://github.com/filecoin-project/ref-fvm/issues/867
let len = get_evm_bytecode_cid(system.rt, addr)
.and_then(|cid| get_evm_bytecode(system.rt, &cid))
.map(|bytecode| bytecode.len())?;
let len = match get_cid_type(system.rt, addr) {
ContractType::EVM(addr) => {
get_evm_bytecode(system.rt, &addr).map(|bytecode| bytecode.len())?
}
ContractType::Native(_) => 1,
// account and not found are flattened to 0 size
_ => 0,
};

Ok(len.into())
}
Expand All @@ -33,8 +37,16 @@ pub fn extcodehash(
system: &System<impl Runtime>,
addr: U256,
) -> Result<U256, StatusCode> {
let cid = get_evm_bytecode_cid(system.rt, addr)?;
let digest = cid.hash().digest();
let addr = match get_cid_type(system.rt, addr) {
ContractType::EVM(a) => Ok(a),
_ => Err(StatusCode::InvalidArgument(
"Cannot invoke EXTCODEHASH for non-EVM actor.".to_string(),
)),
}?;
let bytecode_cid = get_evm_bytecode_cid(system.rt, &addr)?;

let digest = bytecode_cid.hash().digest();

// Take the first 32 bytes of the Multihash
let digest_len = digest.len().min(32);
Ok(digest[..digest_len].into())
Expand All @@ -48,40 +60,55 @@ pub fn extcodecopy(
data_offset: U256,
size: U256,
) -> Result<(), StatusCode> {
let bytecode =
get_evm_bytecode_cid(system.rt, addr).and_then(|cid| get_evm_bytecode(system.rt, &cid))?;
let bytecode = match get_cid_type(system.rt, addr) {
ContractType::EVM(addr) => get_evm_bytecode(system.rt, &addr)?,
ContractType::NotFound | ContractType::Account => Vec::new(),
// calling EXTCODECOPY on native actors results with a single byte 0xFE which solidtiy uses for its `assert`/`throw` methods
// and in general invalid EVM bytecode
_ => vec![0xFE],
};

copy_to_memory(&mut state.memory, dest_offset, size, data_offset, bytecode.as_slice(), true)
}

pub fn get_evm_bytecode_cid(rt: &impl Runtime, addr: U256) -> Result<Cid, StatusCode> {
let addr: EthAddress = addr.into();
let addr: Address = addr.try_into()?;
// TODO: just return none in most of these cases?
let actor_id = rt.resolve_address(&addr).ok_or_else(|| {
StatusCode::InvalidArgument("failed to resolve address".to_string())
// TODO better error code
})?;
#[derive(Debug)]
pub enum ContractType {
/// EVM Address and the CID of the actor (not the bytecode)
EVM(Address),
Native(Cid),
Account,
NotFound,
}

let evm_cid = rt.get_code_cid_for_type(Type::EVM);
let target_cid = rt.get_actor_code_cid(&actor_id);
/// Resolves an address to the address type
pub fn get_cid_type(rt: &impl Runtime, addr: U256) -> ContractType {
let addr: EthAddress = addr.into();

if Some(evm_cid) != target_cid {
return Err(StatusCode::InvalidArgument(
"cannot invoke EXTCODESIZE for non-EVM actor".to_string(),
)); // TODO better error code
}
addr.try_into()
Copy link
Member

Choose a reason for hiding this comment

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

I flattened this to make it easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ty

.ok() // into filecoin address
.and_then(|addr| rt.resolve_address(&addr)) // resolve actor id
.and_then(|id| rt.get_actor_code_cid(&id).map(|cid| (id, cid))) // resolve code cid
.map(|(id, cid)| match rt.resolve_builtin_actor_type(&cid) {
// TODO part of current account abstraction hack where embryos are accounts
Some(Type::Account | Type::Embryo) => ContractType::Account,
Some(Type::EVM) => ContractType::EVM(Address::new_id(id)),
// remaining builtin actors are native
_ => ContractType::Native(cid),
})
.unwrap_or(ContractType::NotFound)
}

let cid = rt
.send(&addr, crate::Method::GetBytecode as u64, Default::default(), TokenAmount::zero())?
.deserialize()?;
Ok(cid)
pub fn get_evm_bytecode_cid(rt: &impl Runtime, addr: &Address) -> Result<Cid, ActorError> {
Ok(rt
.send(addr, crate::Method::GetBytecode as u64, Default::default(), TokenAmount::zero())?
.deserialize()?)
}

pub fn get_evm_bytecode(rt: &impl Runtime, cid: &Cid) -> Result<Vec<u8>, StatusCode> {
pub fn get_evm_bytecode(rt: &impl Runtime, addr: &Address) -> Result<Vec<u8>, StatusCode> {
let cid = get_evm_bytecode_cid(rt, addr)?;
let raw_bytecode = rt
.store()
.get(cid) // TODO this is inefficient; should call stat here.
.get(&cid) // TODO this is inefficient; should call stat here.
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 fine. Note: we don't actually ever expect this error as it should be impossible.

.map_err(|e| StatusCode::InternalError(format!("failed to get bytecode block: {}", &e)))?
.ok_or_else(|| ActorError::not_found("bytecode block not found".to_string()))?;
Ok(raw_bytecode)
Expand Down
9 changes: 9 additions & 0 deletions actors/evm/tests/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,15 @@ const PRELUDE: &str = r#"
dup1
revert
%end

%macro return_stack_word()
# store at 0x00
push1 0x00
mstore
push1 0x20 # always return a full word
push1 0x00
return
%end
"#;

#[allow(dead_code)]
Expand Down
10 changes: 0 additions & 10 deletions actors/evm/tests/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,6 @@ mod util;
pub fn magic_precompile_contract() -> Vec<u8> {
let init = "";
let body = r#"


%macro return_stack_word()
push1 0x00
mstore
push1 0x20 # always return a full word
push1 0x00
return
%end

# magic value, used as initcode
push16 0x666F6F206261722062617A20626F7879 # foo bar baz boxy
push2 0x0100 # offset of input data
Expand Down
Loading