filecoin-project / ref-fvm

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

Remove or neuter DeleteActor syscall #726

Closed anorth closed 1 year ago

anorth commented 2 years ago

I think we should remove the DeleteActor syscall. Or, if something actively prevents that, reduce its functionality to leave the top-level actor object in the state tree. The critical state to preserve is the nonce.

Why do we have DeleteActor?

Chain state is expensive, and so removing it sounds like a good idea. Filecoin was inspired by Ethereum here, which has gone through a similar journey with SELFDESTRUCT.

DeleteActor has one uses in the built-in actors.

As a built-in actor, the forced "altruism" of deletion makes some sense. But we should expect in the future that new and better user-programmed payment channels will dominate usage, and they will not pay the extra cost.

(A previous use case where a defunct miner actor was deleted has since been removed.)

Why should we remove it?

At this point, pre-FVM launch, I think the appropriate question should be "why should we retain it?", with a default expectation of removing any complexity that's not well motivated. I don't know of a good reason: it's very rarely used in Ethereum, and the patterns it's used for (e.g. a faux multicall) usually have a more natural expression that we could support instead.

DeleteActor adds complexity because it changes the possible states for an address to go from no actor -> actor then to also include actor -> no actor, and then to cycle. With predictable actor address generation, an address could be re-used. While we might (with careful thought) be able to ensure that only the same code is deployed to that address, the actor nonce would recycle, opening up replay attacks for smart-contract wallets with account abstraction. One could not generally rely on any monotonicity of state implied by the code. This would also break any assumption that a message CID is can be included only once in the chain.

If we later build in a contract upgrade mechanism (to avoid proxy contracts), self-deleting actors will also complicate reasoning about the address/s authorized to deploy to the actor in question.

DeleteActor can unlink an arbitrarily large amount of state. While a net benefit in the long term, it's not clear that this is a "free" operation for nodes to perform. It would certainly complicate caching approaches. (We can unlink arbitrarily large amounts of state anyway, though).

There is strong support in Ethereum-land for removing or neutering their version of SELFDESTRUCT for similar reasons. EIP-4758 renames it to SENDALL which transfers balance but does not delete any state. This is not the first proposal, but the proximate motivation this time is that deleting arbitrary storage is incompatible with Verkle trees as a state representation.

Who can tell what other future innovation would conflict with actor deletion? The feature doesn't bring any benefits to justify the complexity.

Alternatives

I think just removing the syscall, and it's one use in payment channels, is the simplest and easiest to think about approach.

If we don't do that, I think we must neuter it to leave the top-level actor object (including nonce) in place, which would prevent re-use of the address. We could clear the Code CID and State CID.

Stebalien commented 2 years ago

Why do we have DeleteActor?

An additional motivation is security. For example, if an actor is an "owner" of another actor, it would be nice to have a clear way to "neuter" the actor.

However, this can also be done by just setting the actor into some "dead" state.

an address could be re-used

I don't think this is the case for us:

  1. The address in the state-tree is an ID address, and we assign ID addresses in ascending order.
  2. We don't remove the actor from the init actor, so we can't reuse those addresses.

So yes, I think we should have a strong "no address may be reused" guarantee, but I'd like the ability to at least "tombstone" an actor in such a way that it's easy to verify that the actor can never be revived.

anorth commented 2 years ago

I agree that IDs won't be re-used. But if we allow full deletion and predictable addresses, then a predictable address could be re-used and be assigned a new, different ID than it had last time!

Stebalien commented 2 years ago

I agree that IDs won't be re-used. But if we allow full deletion and predictable addresses, then a predictable address could be re-used and be assigned a new, different ID than it had last time!

Ah, my point is that we currently don't remove addresses, even if we delete the actor. It's perfectly valid to have an address pointing to a non-existent actor.

(steb now goes to check that this isn't a fatal error in the FVM...)

anorth commented 2 years ago

@Kubuxu has realised a much simpler solution. We can continue to delete the actor record, but leave the address in the Init actor's lookup table. The Init actor should refuse to overwrite an entry.

https://github.com/filecoin-project/FIPs/discussions/436

Stebalien commented 2 years ago

Yep, that's what we already do:

  1. We never use IDs.
  2. We never "unregister" addresses.
Kubuxu commented 2 years ago

Yeah, then we don't need to neuter DeleteActor if we call "Robust Address Exists and points to non-existent ID address" a tombstone and disallow the creation of a new actor under that robust address.

The behaviour today is to override the entry in the Robust -> ID map, but that can't happen (due to how the robust addresses are generated).

Stebalien commented 2 years ago

Oh, I see. Yeah, we shouldn't do that.

raulk commented 2 years ago

There is strong support in Ethereum-land for removing or neutering their version of SELFDESTRUCT for similar reasons. EIP-4758 renames it to SENDALL which transfers balance but does not delete any state. This is https://github.com/ethereum/EIPs/pull/2751 proposal, but the proximate motivation this time is that deleting arbitrary storage is incompatible with Verkle trees as a state representation.

Hm, the three motivations in Ethereum don't translate to Filecoin.

  • SELFDESTRUCT is the only opcode which causes an unbounded number of state objects to be altered in a single block

This would be a single IO operation in Filecoin setting the root of the actor state to an empty struct CID, or similar.

  • SELFDESTRUCT is the only opcode which can cause the code of a contract to change

We are planning to introduce code updates as a first-class citizen in the protocol anyway.

  • SELFDESTRUCT is the only opcode which can change other accounts’ balances without their consent

This alludes to the fact that Ethereum executes smart contract logic even on simple value transfers (transactions with no input parameters); we don't.

raulk commented 2 years ago

One aspect to consider here is at the intersection of account abstraction and extensible addressing in the future. We'll need to reconcile the tombstoning behaviour in that case too. What happens if an abstract account (i.e. a user-deployed actor that can act as a sender) with an f4 binding deletes itself, and it is recreated under the same f4 binding? cc @Stebalien (for the extensible addressing proposal)

Stebalien commented 2 years ago

This alludes to the fact that Ethereum executes smart contract logic even on simple value transfers (transactions with no input parameters); we don't.

Which is, unfortunately, something we should reconsider in light of some recent events.

What happens if an abstract account (i.e. a user-deployed actor that can act as a sender) with an f4 binding deletes itself, and it is recreated under the same f4 binding? cc @Stebalien (for the extensible addressing proposal)

I think the policy here is:

  1. We never delete an address binding (only the backing actor).
  2. Code can only be deployed at unassigned addresses, or addresses assigned to "eggs".

Basically, once an address has been assigned to a specific actor, that address may never be reassigned.

Kubuxu commented 2 years ago

recreated under the same f4 binding

As Stebalien has pointed out, we never remove the binding to the underlying ID address and only delete the underlying actor.

This was codified in builtin-actors https://github.com/filecoin-project/builtin-actors/pull/568/files

Stebalien commented 1 year ago

This has been FIPed and implemented.