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

basic EVM smart contract actor #517

Merged
merged 31 commits into from
Aug 18, 2022
Merged

basic EVM smart contract actor #517

merged 31 commits into from
Aug 18, 2022

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented Aug 11, 2022

Description by @raulk

Closes filecoin-project/ref-fvm#693.

This contains the implementation of a basic EVM smart contract actor, based on what currently lives https://github.com/filecoin-project/fvm-evm/, but simplified to account for new designs around account abstraction and extensible addressing at the protocol level.

Supported features

  • Take init EVM bytecode in the constructor
  • Initialize a smart contract from that bytecode
  • Create the storage data structures in state (EVM storage is mapped to an IPLD HAMT, this will be optimised in the near future)
  • Run self-contained EVM smart contracts that do not call any other contracts/actors

Not supported yet

The memory management needs to be reviewed entirely.


In other words, there is still a ton of work to do, but this introduces an initial landmark to set as a departure point for all upcoming work.

The EVM actor has been included in the bundle produced by this repo.

Once this PR is merged, the https://github.com/filecoin-project/fvm-evm/ repo can be archived.

@raulk
Copy link
Member

raulk commented Aug 11, 2022

Our EVM implementation is a fork of https://github.com/vorot93/evmodin with several changes, amongst which are:

  • Removed support for continuations. We don't expect to use this feature in FVM.
  • Removed support for tracing. We may want to re-introduce this at some point proxying over to the debug::log syscall.
  • All instructions under instructions/ have been #[inlined].
  • The Host trait has been removed and substituted by a System concrete type that uses the FVM SDK (and thus depends on the actor Wasm sandbox). I think we will need to restore this trait for testing purposes.
  • The Memory is now backed by BytesVec instead of a Vec; it exposes a method to grow it, but we need to check that it's being called from every possible point.
  • Message#code_address has been removed; we may need to reintroduce when we start handling delegate call.
  • Opcode/EVM evolution throughout Ethereum's history has been removed. This version supports the Berlin hardfork.
  • Bytecode processing has lost features, e.g. code padding (not sure if this was an implementation detail), once-only jumpdest table population, etc. I think this is connected with the fact that we removed continuations and other unnecessary features, but gotta check.

@karim-agha
Copy link

I would recommend using the “toronto-demo” branch. It has more progress than master

@vyzo vyzo changed the title [WIP] EVM actor EVM actor Aug 16, 2022
raulk added 2 commits August 17, 2022 12:38
These mappings will not be managed by the EVM actor, but rather by
the environment through address classes.
@raulk raulk changed the title EVM actor initial EVM smart contract actor Aug 17, 2022
@raulk raulk changed the title initial EVM smart contract actor basic EVM smart contract actor Aug 17, 2022
@raulk raulk marked this pull request as ready for review August 17, 2022 13:29
@raulk raulk added the FEVM label Aug 17, 2022
Comment on lines +62 to +65
// create an instance of the platform abstraction layer -- note: do we even need this?
let system = System::new(rt, init_contract_state_cid).map_err(|e| {
ActorError::unspecified(format!("failed to create execution abstraction layer: {e:?}"))
})?;
Copy link
Member

Choose a reason for hiding this comment

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

We will need to reintroduce a trait here (like evmodin had Host) to make this abstraction layer useful for testing.


let state = State::new(
rt.store(),
RawBytes::new(contract_bytecode.to_vec()),
Copy link
Member

Choose a reason for hiding this comment

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

nit: This is a mouthful to read as a parameter, would probably extract it to a local var and name it for what it is (evm_bytecode_bytes, or something).

Comment on lines +111 to +113
// TODO this is fine in a transaction for now, as we don't have yet cross-contact calls
// some refactoring will be needed when we start making cross contract calls.
rt.transaction(|state: &mut State, rt| {
Copy link
Member

Choose a reason for hiding this comment

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

Yes, all of this will evolve as we implement https://github.com/filecoin-project/fvm-specs/issues/107.

{
rt.validate_immediate_caller_accept_any()?;

if params.bytecode.len() > MAX_CODE_SIZE {
Copy link
Member

Choose a reason for hiding this comment

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

I'd actually call this field init_code because that's what the Ethereum protocol calls it (it's the constructor code).

#[inline]
pub fn sload<'r, BS: Blockstore, RT: Runtime<BS>>(
state: &mut ExecutionState,
platform: &'r System<'r, BS, RT>,
Copy link
Member

Choose a reason for hiding this comment

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

There's some naming incongruence left over from past refactors. Platform/System/Host... Need to fix this eventually.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

There are obviously many TODOs, many bugs lurking, and many things that will get rewritten, but it serves as a reasonable starting point. I'm happy to merge this into next and continue iterating there.

@raulk
Copy link
Member

raulk commented Aug 17, 2022

Ah, I also moved my notes on evmodin divergence to a README.md file under the interpreter module: https://github.com/filecoin-project/builtin-actors/pull/517/files#diff-4ff2e70b4b59321d541e648b02188a6139aefb30079fea0c467e3b6c23415009R1

@raulk raulk requested review from Stebalien and mriise August 17, 2022 22:24
@raulk raulk merged commit 36ec85a into next Aug 18, 2022
@raulk raulk deleted the next-evm branch August 18, 2022 10:20
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Oct 3, 2022
Co-authored-by: Raúl Kripalani <raul@protocol.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants