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

self_destruct should auto-create beneficiary #736

Closed
Stebalien opened this issue May 19, 2022 · 11 comments
Closed

self_destruct should auto-create beneficiary #736

Stebalien opened this issue May 19, 2022 · 11 comments
Labels
Kind: Improvement An improvement of something that exists. MIGRATED Topic: Syscalls
Milestone

Comments

@Stebalien
Copy link
Member

Currently, if the beneficiary doesn't exist and is, e.g., a key address, we don't automatically create it. This differs from send where we do.

@anorth
Copy link
Member

anorth commented May 19, 2022

I concur

@anorth
Copy link
Member

anorth commented Jun 14, 2022

See also #726

@raulk raulk added the MIGRATED label Aug 18, 2022
@raulk raulk transferred this issue from filecoin-project/fvm-specs Aug 18, 2022
@raulk raulk added Topic: Syscalls Kind: Improvement An improvement of something that exists. labels Aug 18, 2022
@maciejwitowski maciejwitowski added this to the M2.1 milestone Oct 7, 2022
@Stebalien
Copy link
Member Author

Proposal: Just treat this as a method-0 send with no parameters. I.e.:

  1. Lookup the current balance.
  2. Call send (charging all the normal send gas).
  3. Delete self (refunding nothing).

@Stebalien
Copy link
Member Author

Ok, so, the issue with that approach is that it conflicts with the approach taken in #273 assuming we ever allow code on method 0 sends (#835). Basically, I'm concerned about re-entering a half-dead actor.


New proposal: remove the "beneficiary" from self-destruct, and burn any remaining balance. This puts the user in control of how the distribute funds and simplifies the implementation.

@Stebalien
Copy link
Member Author

In other words, the user should now:

  1. Call send (as many times as necessary) to drain the account.
  2. Call self_destruct with no arguments.

@Stebalien
Copy link
Member Author

Er, maybe better to just reject the call to self_destruct rather than burning money.

@Stebalien
Copy link
Member Author

@Stebalien
Copy link
Member Author

E.g. an entry point actor that needs to pay its own gas? It'll probably have some refund amount that needs to be burnt, at least.

I'd moderately prefer to burn just the refund but still reject self_destruct otherwise. It's a nice safe-guard to some otherwise nasty re-entrency bugs. I.e.:

  1. Send away funds...
  2. Send causes a chain reaction, granting us funds.
  3. Self destruct burns them.

@anorth
Copy link
Member

anorth commented Nov 14, 2022

Ok so that would require a method inside the actor itself to (1) send its entire balance somewhere (the gas limit having already been reserved), and then (2) call self_destruct with no remaining balance.

But that is basically what self-destruct does! (When modified as you proposed to use a basic send). I don't really get the "re-entering a half-dead actor" concern if self_destruct works in this straightforward way.

However, I'll be convinced by an argument that there's some complexity here, and we should kick that complexity out of the VM and make the actors handle it.

@Stebalien
Copy link
Member Author

Yep, the idea is to punt all the complexity to the user.

My primary concern is that, if we ever allow users to run code on method 0, the "send" from self_destruct could:

  1. Fail for some reason. We have no good way to return the error to the user.
  2. Re-enter the actor that's self-destructing, possibly sending it funds in the process.

We can handle these cases, but it's simpler to just ask the user to drain their actor before calling self_destruct.

Stebalien added a commit to filecoin-project/builtin-actors that referenced this issue Dec 2, 2022
In the EVM:

1. Self destruct continues even if it sends funds into oblivion.
2. Self destruct will auto-create a beneficiary.

This lets us punt filecoin-project/ref-fvm#736
to M2.2.
Stebalien added a commit to filecoin-project/builtin-actors that referenced this issue Dec 2, 2022
In the EVM:

1. Self destruct continues even if it sends funds into oblivion.
2. Self destruct will auto-create a beneficiary.

This lets us punt filecoin-project/ref-fvm#736
to M2.2.
Stebalien added a commit to filecoin-project/builtin-actors that referenced this issue Dec 2, 2022
In the EVM:

1. Self destruct continues even if it sends funds into oblivion.
2. Self destruct will auto-create a beneficiary.

This lets us punt filecoin-project/ref-fvm#736
to M2.2.
Stebalien added a commit to filecoin-project/builtin-actors that referenced this issue Dec 2, 2022
In the EVM:

1. Self destruct continues even if it sends funds into oblivion.
2. Self destruct will auto-create a beneficiary.

This lets us punt filecoin-project/ref-fvm#736
to M2.2.
Stebalien added a commit to filecoin-project/builtin-actors that referenced this issue Dec 2, 2022
In the EVM:

1. Self destruct continues even if it sends funds into oblivion.
2. Self destruct will auto-create a beneficiary.

This lets us punt filecoin-project/ref-fvm#736
to M2.2.
Stebalien added a commit to filecoin-project/builtin-actors that referenced this issue Dec 2, 2022
In the EVM:

1. Self destruct continues even if it sends funds into oblivion.
2. Self destruct will auto-create a beneficiary.

This lets us punt filecoin-project/ref-fvm#736
to M2.2.
Stebalien added a commit to filecoin-project/builtin-actors that referenced this issue Dec 2, 2022
In the EVM:

1. Self destruct continues even if it sends funds into oblivion.
2. Self destruct will auto-create a beneficiary.

This lets us punt filecoin-project/ref-fvm#736
to M2.2.
@Stebalien Stebalien removed this from the M2.1 (rr10) Carbonado milestone Dec 3, 2022
@Stebalien Stebalien added this to the M2.2 milestone Dec 3, 2022
@Stebalien
Copy link
Member Author

We've worked around this in the EVM by sending, then self destructing. Punted to M2.2 to avoid yet another FIP.

@Stebalien Stebalien modified the milestones: M2.2, NV20 Mar 2, 2023
@Stebalien Stebalien closed this as not planned Won't fix, can't repro, duplicate, stale Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Kind: Improvement An improvement of something that exists. MIGRATED Topic: Syscalls
Projects
None yet
Development

No branches or pull requests

4 participants