filecoin-project / ref-fvm

Reference implementation of the Filecoin Virtual Machine
https://fvm.filecoin.io/
Other
384 stars 139 forks source link

EVM Proposal: userspace selfdestruct #1221

Closed Stebalien closed 1 year ago

Stebalien commented 1 year ago

Our current SELFDESTRUCT operation is incorrect because it deletes the contract immediately. Also, see #1174.

Proposal: Implement SELFDESTRUCT in userspace by:

  1. Sending all funds to the beneficiary.
  2. Marking the contract as "selfdestructed" in the contract's actor state by recording the current origin and nonce.
  3. Changing the EVM actor to "pretend like it has no code" if it (a) sees this field set and (b) the nonce/origin don't match (i.e., we've moved on to a new transaction).
  4. Adding a new Resurrect function that behaves like the Constructor but:
    1. Must be called directly by the EAM.
    2. Can only be called if the selfdestruct field is set.
    3. Blows away state and re-constructs the EVM contract.
  5. Change the EAM to call Resurrect when a user attempts to call CREATE2 and the target actor already exists.

Changes WRT Ethereum:

  1. This won't ever delete state and/or refunds. But, whatever.
  2. Unlike Ethereum, funds sent to a contract after SELFDESTRUCT but before the end of the transaction won't poof into the void and will isntead remain with the contract. But I consider that a feature.
vyzo commented 1 year ago

SGTM. We want it when?

Stebalien commented 1 year ago

Now. Want to handle this tomorrow?

raulk commented 1 year ago

This sounds reasonable, especially given that the Ethereum community has noted the many quirks of the SELFDESTRUCT opcode and is potentially looking to eliminate it. So yet another reason to keep those quirks contained in the EVM runtime actor.

Just one question: could you elaborate on the reason why we don't delink the storage HAMT immediately? Can we re-enter within the same transaction with the expectation that storage will still be alive?

Stebalien commented 1 year ago

Just one question: could you elaborate on the reason why we don't delink the storage HAMT immediately? Can we re-enter within the same transaction with the expectation that storage will still be alive?

Exactly this. Deletion is deferred until the end of the transaction.

Stebalien commented 1 year ago

Current status: the builtin actors use the latest SDK so this is unblocked, but I haven't made any progress towards actually implementing it. You're good to go vyzo!

mriise commented 1 year ago

Please be aware of behavior of EIP-161 (and if we choose to adopt it, EIP-4747), since there are some cases where there is an auto deletion of "empty" accounts.

We will still be doing userspace deletes for these as well though.

cc @vyzo

edit: we may wish to disregard this since I'm having trouble thinking of an application that would depend on this behavior.

raulk commented 1 year ago

@Stebalien in your refactor of SELFDESTRUCT, can you perform the audit specified here: https://filecoinproject.slack.com/archives/C029MT4PQB1/p1671723226685449.

raulk commented 1 year ago

@Stebalien did we end up committing to a concrete solution here? Is there a sketch PR somewhere that I can take a look to specify in FIP-0054?

Stebalien commented 1 year ago

First, on SELFDESTRUCT, we record the current origin/nonce in the actor state:

pub struct EvmContractState {
    ...
    tombstone: Option<(ActorId, u64)>,
}

We consider the contact "dead" if, at any point, tombstone is not none (not null) and tombstone is not equal to the current origin/nonce.

If the contract is dead, on:

  1. InvokeContract, we simply return with success (do nothing).
  2. On GetBytecode, we return the CID of empty code (ignoring the actual bytecode).
  3. On GetBytecodeHash:
    1. ~If the balance is zero, we return all zeros (32 bytes of zeros).~ edit: I'm nixing this. Technically, this is correct, but it complicates the code and it just doesn't matter.
    2. Otherwise, return the empty contract hash.

On CREATE2/CREATE:

  1. The EAM will compute the target address and check to see if an actor already exists (both in create2 and create).
  2. If the target actor exists, it'll call the constructor directly.
  3. Otherwise, it'll use the normal Exec4 method.

Finally, the EVM's constructor will allow itself to be called by either the EAM (in which case, it'll delete its own state object and replace it with an entirely new one) or the init actor.

Stebalien commented 1 year ago

We discussed bypassing the EAM on resurrect, but after writing this all out, I'm nixing that approach as it would require computing the expected EVM address in multiple places.

Stebalien commented 1 year ago

This also requires changing the EAM's Create2 and Create signatures to return an Option<Address> for the "predictable" address, because they may be "resurrecting" an actor.