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

Eip 6780 selfdestruct #5430

Merged
merged 30 commits into from
Jul 24, 2023
Merged

Eip 6780 selfdestruct #5430

merged 30 commits into from
Jul 24, 2023

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented May 3, 2023

  • Wire in a check to see if an account is new in the TX scope
  • Create a new SelfDestruct Opcode that only deletes accounts
    that register as new within tx scope.

PR description

Fixed Issue(s)

@github-actions
Copy link

github-actions bot commented May 3, 2023

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I have considered running ./gradlew acceptanceTestNonMainnet locally if my PR affects non-mainnet modules.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@shemnon shemnon marked this pull request as draft May 4, 2023 05:07
* Wire in a check to see if an account is new in the TX scope
* Create a new SelfDestruct Opcode that only deletes accounts
  that register as new within tx scope.

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
@shemnon shemnon marked this pull request as ready for review May 10, 2023 18:46
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

Looks pretty straight forward overall 👍

Some questions and name change suggestions

@@ -27,13 +27,28 @@
/** The Self destruct operation. */
public class SelfDestructOperation extends AbstractOperation {

final boolean eip6780Semantics;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
final boolean eip6780Semantics;
final boolean eip6780Enabled;

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this wasn't changed or the old name crept back in

Copy link
Contributor

@jflo jflo left a comment

Choose a reason for hiding this comment

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

Functionally this looks good, but I've left comments with regard to readability.

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
…destruct

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
…destruct

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

Do we have pre-existing self destruct reference tests that validate the behavior of self destruct recipient address the same as the self destruct address?

@@ -60,7 +75,9 @@ public OperationResult execute(final MessageFrame frame, final EVM evm) {
final Address address = frame.getRecipientAddress();
final MutableAccount account = frame.getWorldUpdater().getAccount(address).getMutable();

frame.addSelfDestruct(address);
if (!eip6780Semantics || account.isNewAccount()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we were processing a (malformed) block that included the account creation in a prior transaction, would the account still be marked as newAccount and allow a self destruct in a subsequent transaction?

At least in the case of bonsai, I would expect the accumulator would have the account cached with isNewAccount == true, since I don't see anywhere where we are resetting that flag at transaction boundary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add an EVMTool Test based off of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change of plans, I re-wrote the new account detection to not depend on changes to the Account interface.

@shemnon
Copy link
Contributor Author

shemnon commented Jul 7, 2023

./ethereum/referencetests/src/reference-test/external-resources/GeneralStateTests/stSystemOperationsTest/suicideAddress.json handles the self-destruct to self question. RefTests haven't been filled for Cancun. I'll add a evmtool test based off of this.

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
…destruct

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

👍

@shemnon
Copy link
Contributor Author

shemnon commented Jul 11, 2023

Waiting on ethereum/EIPs#7308 to settle before check in. We pass execution-spec-tests as the exist right now, without alteration.

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
…destruct

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
…destruct

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
…-if-tx-is-playback-protected

Use v to determine if the transaction is protected

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
…destruct

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

Another pass based on the new changes. I prefer this messageFrame approach 👍

}

/** Removes all entries in the create set. */
public void clearCreates() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't used, that's fine, just checking this wasn't left in by mistake.

@@ -27,13 +27,28 @@
/** The Self destruct operation. */
public class SelfDestructOperation extends AbstractOperation {

final boolean eip6780Semantics;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this wasn't changed or the old name crept back in

frame.addSelfDestruct(address);
if (!eip6780Semantics || frame.wasCreatedInTransaction(address)) {
frame.addSelfDestruct(address);
}

final MutableAccount recipient =
Copy link
Contributor

Choose a reason for hiding this comment

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

Think it would help the understanding of this PR to rename this to beneficiary (or evenselfDestructBeneficiary) to match the unit test and the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Settled on beneficiary and originator

@@ -60,19 +75,22 @@ public OperationResult execute(final MessageFrame frame, final EVM evm) {
final Address address = frame.getRecipientAddress();
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a little verbose, but while reviewing, I renamed this to addressOfAccountToMaybeDestruct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spec has originator, so I went with that.

recipient.incrementBalance(account.getBalance());
Wei contractBalance = account.getBalance();
if (eip6780Semantics || !account.getAddress().equals(recipient.getAddress())) {
recipient.incrementBalance(contractBalance);
Copy link
Contributor

@siladu siladu Jul 19, 2023

Choose a reason for hiding this comment

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

With the renames, this statement becomes a lot clearer IMO...

!addressOfAccountToMaybeDestruct.equals(selfDestructBeneficiary.getAddress())) {
      selfDestructBeneficiary.incrementBalance(balanceOfDestructedAccount);`
      ...


account.setBalance(Wei.ZERO);
account.decrementBalance(contractBalance);
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth a comment to explain the case where this balance should not be set to zero, as before.

Seems like for most cases this is equivalent to setting to zero, except the case where the contract account being "hollowed" (because it was created in a prior transaction) had an existing balance in it...IIUC, this is a rare and specific usage of CREATE2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worse than that, there was a bug relating to repeated self-destructs of the same address. I rewrote the logic into something much simpler.

shemnon and others added 2 commits July 22, 2023 07:54
Also, fixes a bug found in the new execution spec tests relating to
repeated calls to self-destruct on the same account.

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

Readability changes look good.

Spec has originator, so I went with that.

I actually wonder if the execution-spec has the wrong name in this case. Yellowpaper defines "sender" and "original transactor", saying that the original transactor (originator) can change when using CREATE.
But I think even the YP has a mistake in its definition of it, when compared to the ORIGIN opcode.

I think it's the sender that should change during CREATE. Indeed, this is how Besu appears to implements it.
To me, it makes sense that the transaction originator never changes and the sender can change if CREATE is involved.

I think that means for self-destruct we should be dealing with the sender rather than the originator.

}

@Override
public OperationResult execute(final MessageFrame frame, final EVM evm) {
final Address recipientAddress = Words.toAddress(frame.popStackItem());

// fist calculate cost. There's a bit of yak shaving getting values to calculate the cost
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// fist calculate cost. There's a bit of yak shaving getting values to calculate the cost
// first calculate cost. There's a bit of yak shaving getting values to calculate the cost


// with the cost we can test for two early exits: static or not enough gas
Copy link
Contributor

Choose a reason for hiding this comment

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

good comment 👍

if (!account.getAddress().equals(recipient.getAddress())) {
recipient.incrementBalance(account.getBalance());
// If we are actually destroying the contract (pre-Cancun or same-tx-create) we need to
// explicitly zero out the account balance (destroying ether if the originator is the
Copy link
Contributor

Choose a reason for hiding this comment

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

typo?

Suggested change
// explicitly zero out the account balance (destroying ether if the originator is the
// explicitly zero out the account balance (destroying even if the originator is the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I literally mean ether, not either or even. Clarified with ether/value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I read it as "either" rather than "ether" :)


if (!account.getAddress().equals(recipient.getAddress())) {
recipient.incrementBalance(account.getBalance());
// If we are actually destroying the contract (pre-Cancun or same-tx-create) we need to
Copy link
Contributor

Choose a reason for hiding this comment

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

for consistency with the rest of the comment/variable names

Suggested change
// If we are actually destroying the contract (pre-Cancun or same-tx-create) we need to
// If we are actually destroying the originator account (pre-Cancun or same-tx-create) we need to

Comment on lines +83 to +85
// Do the "sweep," all modes send all originator balance to the beneficiary account.
originatorAccount.decrementBalance(originatorBalance);
beneficiaryAccount.incrementBalance(originatorBalance);
Copy link
Contributor

Choose a reason for hiding this comment

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

Like that this is closer to the execution-spec 👍

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
@shemnon
Copy link
Contributor Author

shemnon commented Jul 24, 2023

Both Geth's dev-branch and ours agree on the testing results (not all execution-spec-tests appear to be correct, and geth/besu disagree with the spec tests in the same way).

So I'm considering it up to standard finally.

@shemnon shemnon enabled auto-merge (squash) July 24, 2023 19:27
@shemnon shemnon merged commit dd129c8 into hyperledger:main Jul 24, 2023
8 checks passed
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
* track CREATE/CREATE2/create-tx in a new "creates" field in the MessageFrame
* re-wrote Self-Destruct logic for clarity and optional EIP-6780 semantics.

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
* track CREATE/CREATE2/create-tx in a new "creates" field in the MessageFrame
* re-wrote Self-Destruct logic for clarity and optional EIP-6780 semantics.

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants