Eip 6780 selfdestruct #5430

shemnon · 2023-05-03T21:27:21Z

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 · 2023-05-03T21:27:30Z

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

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructEIP6780Operation.java

* 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>

siladu

Looks pretty straight forward overall 👍

Some questions and name change suggestions

siladu · 2023-05-11T07:03:04Z

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructOperation.java

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

+  final boolean eip6780Semantics;


Suggested change

final boolean eip6780Semantics;

final boolean eip6780Enabled;

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

evm/src/test/java/org/hyperledger/besu/evm/operations/SelfDestructOperationTest.java

evm/src/main/java/org/hyperledger/besu/evm/fluent/SimpleAccount.java

evm/src/main/java/org/hyperledger/besu/evm/worldstate/UpdateTrackingAccount.java

jflo

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

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiAccount.java

evm/src/test/java/org/hyperledger/besu/evm/operations/SelfDestructOperationTest.java

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>

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

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

garyschulte

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?

garyschulte · 2023-07-07T01:28:28Z

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructOperation.java

@@ -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()) {


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.

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

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

shemnon · 2023-07-07T14:46:41Z

./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>

garyschulte

👍

shemnon · 2023-07-11T19:57:02Z

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>

…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>

siladu

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

siladu · 2023-07-19T05:38:59Z

evm/src/main/java/org/hyperledger/besu/evm/frame/MessageFrame.java

+  }
+
+  /** Removes all entries in the create set. */
+  public void clearCreates() {


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

siladu · 2023-07-19T07:00:37Z

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructOperation.java

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

+  final boolean eip6780Semantics;


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

siladu · 2023-07-19T07:03:49Z

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructOperation.java

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

    final MutableAccount recipient =


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

Settled on beneficiary and originator

siladu · 2023-07-19T07:04:49Z

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructOperation.java

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


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

Spec has originator, so I went with that.

siladu · 2023-07-19T07:07:04Z

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructOperation.java

-      recipient.incrementBalance(account.getBalance());
+    Wei contractBalance = account.getBalance();
+    if (eip6780Semantics || !account.getAddress().equals(recipient.getAddress())) {
+      recipient.incrementBalance(contractBalance);


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

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

siladu · 2023-07-19T07:17:54Z

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructOperation.java


-    account.setBalance(Wei.ZERO);
+    account.decrementBalance(contractBalance);


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?

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

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>

siladu

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.

siladu · 2023-07-24T00:02:30Z

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructOperation.java

  }

  @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


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

siladu · 2023-07-24T00:04:47Z

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructOperation.java


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


good comment 👍

siladu · 2023-07-24T00:08:38Z

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructOperation.java

-    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


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

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

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

siladu · 2023-07-24T00:09:55Z

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructOperation.java


-    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


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

siladu · 2023-07-24T00:12:11Z

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructOperation.java

+    // Do the "sweep," all modes send all originator balance to the beneficiary account.
+    originatorAccount.decrementBalance(originatorBalance);
+    beneficiaryAccount.incrementBalance(originatorBalance);


Like that this is closer to the execution-spec 👍

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

shemnon · 2023-07-24T19:25:05Z

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.

* 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>

shemnon marked this pull request as draft May 4, 2023 05:07

jframe reviewed May 5, 2023

View reviewed changes

evm/src/main/java/org/hyperledger/besu/evm/operation/SelfDestructEIP6780Operation.java Outdated Show resolved Hide resolved

shemnon added 3 commits May 10, 2023 12:40

EIP-6780 SELFDESTRUCT Support

e4022e1

* 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>

unit test

4875b66

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

merge self destruct semantics into one operation.

9b6eb6a

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

shemnon force-pushed the eip-6780-selfdestruct branch from 64222a1 to 9b6eb6a Compare May 10, 2023 18:41

javadoc

5d3caab

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

shemnon marked this pull request as ready for review May 10, 2023 18:46

siladu reviewed May 11, 2023

View reviewed changes

jflo reviewed Jun 28, 2023

View reviewed changes

siladu reviewed Jun 29, 2023

View reviewed changes

evm/src/test/java/org/hyperledger/besu/evm/operations/SelfDestructOperationTest.java Outdated Show resolved Hide resolved

shemnon added 4 commits June 30, 2023 10:14

review changes

95ddfe1

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

Merge branch 'main' of github.com:hyperledger/besu into eip-6780-self…

6ac36fe

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

update verification data

f473755

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

update verification data

1ec79a8

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

jflo approved these changes Jun 30, 2023

View reviewed changes

shemnon added 2 commits July 6, 2023 11:01

Merge branch 'main' of github.com:hyperledger/besu into eip-6780-self…

9d554ad

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

updates from execution-spec-tests

3cc93b3

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

garyschulte reviewed Jul 7, 2023

View reviewed changes

shemnon added 4 commits July 7, 2023 15:33

Don't use getOrCreate in EVM

686b34f

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

Merge branch 'main' of github.com:hyperledger/besu into eip-6780-self…

d099a99

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

Don't use getOrCreate in EVM

b46d7dd

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

remove stacktrace

4db9a1a

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

shemnon mentioned this pull request Jul 8, 2023

EIP 6780 Tests ethereum/execution-spec-tests#122

Merged

shemnon added 4 commits July 10, 2023 12:12

hack plus notation for execution test specs

f7cea80

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

spotless

83aaec8

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

Align with Geth re contract creation/account creation.

fdf3d03

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

spotless and unneeded change removal

632e954

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

garyschulte approved these changes Jul 11, 2023

View reviewed changes

Merge branch 'main' into eip-6780-selfdestruct

60740c3

shemnon added 7 commits July 12, 2023 13:04

fix json libraries in t8n server and protected legacy TXes

8a1ddc0

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

add evm tool to the default distribution

9b37fb9

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

Merge branch 'main' of github.com:hyperledger/besu into eip-6780-self…

f8ec251

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

Merge branch 'main' of github.com:hyperledger/besu into eip-6780-self…

077b6a8

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

Merge pull request #14 from danceratopz/t8n-server-use-v-to-determine…

3e053c6

…-if-tx-is-playback-protected Use v to determine if the transaction is protected Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>

remove "protected" attribute

eb5a964

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

Merge branch 'main' of github.com:hyperledger/besu into eip-6780-self…

fa4caf5

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

siladu reviewed Jul 19, 2023

View reviewed changes

shemnon and others added 2 commits July 22, 2023 07:54

Refactor into more readable code

2807e29

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>

Merge branch 'main' into eip-6780-selfdestruct

dd060d1

siladu reviewed Jul 24, 2023

View reviewed changes

tpyos

c41801e

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

Merge branch 'main' into eip-6780-selfdestruct

22e33d7

shemnon enabled auto-merge (squash) July 24, 2023 19:27

shemnon merged commit dd129c8 into hyperledger:main Jul 24, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eip 6780 selfdestruct #5430

Eip 6780 selfdestruct #5430

shemnon commented May 3, 2023

github-actions bot commented May 3, 2023

siladu left a comment

siladu May 11, 2023

siladu Jul 19, 2023

jflo left a comment

garyschulte left a comment

garyschulte Jul 7, 2023

shemnon Jul 7, 2023

shemnon Jul 7, 2023

shemnon commented Jul 7, 2023

garyschulte left a comment

shemnon commented Jul 11, 2023

siladu left a comment

siladu Jul 19, 2023

siladu Jul 19, 2023

siladu Jul 19, 2023

shemnon Jul 22, 2023

siladu Jul 19, 2023

shemnon Jul 22, 2023

siladu Jul 19, 2023 •

edited

Loading

siladu Jul 19, 2023

shemnon Jul 22, 2023

siladu left a comment •

edited

Loading

siladu Jul 24, 2023

siladu Jul 24, 2023

siladu Jul 24, 2023

shemnon Jul 24, 2023

siladu Jul 24, 2023

siladu Jul 24, 2023

siladu Jul 24, 2023

shemnon commented Jul 24, 2023

	final boolean eip6780Semantics;
	final boolean eip6780Enabled;

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


		account.setBalance(Wei.ZERO);
		account.decrementBalance(contractBalance);

	// 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

	// 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

	// 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

Eip 6780 selfdestruct #5430

Eip 6780 selfdestruct #5430

Conversation

shemnon commented May 3, 2023

PR description

Fixed Issue(s)

github-actions bot commented May 3, 2023

siladu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jflo left a comment

Choose a reason for hiding this comment

garyschulte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon commented Jul 7, 2023

garyschulte left a comment

Choose a reason for hiding this comment

shemnon commented Jul 11, 2023

siladu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siladu Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siladu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon commented Jul 24, 2023

siladu Jul 19, 2023 •

edited

Loading

siladu left a comment •

edited

Loading