core/state, core/vm: implement EIP 6780 #27189

jwasinger · 2023-04-28T14:56:29Z

EIP-6780: SELFDESTRUCT only in same transaction

SELFDESTRUCT will recover all funds to the caller but not delete the account, except when called in the same transaction as creation

I'm taking a stab at this (probably broken atm. needs to be verified with tests)

holiman · 2023-05-10T14:30:19Z

core/state/statedb.go

+func (s *StateDB) SendAll(addr common.Address) bool {
+	stateObject := s.getStateObject(addr)
+	if stateObject == nil {
+		return false
+	}
+
+	stateObject.sendalled = true
+	return true


This needs journalling, just like the Suicide above does it

The operations that need to be journaled are also done when this method is called: https://github.com/ethereum/go-ethereum/pull/27189/files#diff-7da47ad076053fe0f266b3088eb9ee206c7f3f8fe5a93116a3f5e25ee5074213R837-R839 .

Can move them into this method to make it clearer.

So, IIUC, you have implicitly journalled the balance movement. But you have not journalled the stateObject.sendalled=true. So if

a -> b create c (c.create = true, c.sendall = false) ... a -> b -> c sendall to d. (c.sendall = true) a b revert

c now has create and sendall, and will be deleted, but it shouldn't be since it was supposedly reverted.

Ah yes. good point.

holiman · 2023-05-10T14:33:45Z

core/vm/instructions.go

+	interpreter.evm.StateDB.SubBalance(scope.Contract.Address(), balance)
+	interpreter.evm.StateDB.AddBalance(beneficiary.Bytes20(), balance)
+	interpreter.evm.StateDB.SendAll(scope.Contract.Address())


The way this is implemented, if sendall is executed many times, it will re-sendall. Just noting it, not sure if it's explicitly stated in the spec.

A sends 100 wei to b

A -> b.Sendall( c) // sends 100 to c

A sends 100 wei to b

A -> b.Sendall(c) // sends 100 to c again

Good observation. This behavior seems in-line with the spec IMO.

holiman · 2023-05-10T14:36:31Z

core/state/statedb.go

+	newobj.created = true
+	newobj.sendalled = false


Is it 100% certain that the newobj is flushed, so that when the next transaction is processed, we no longer have any objects with created set to true? I am not 100% certain, off the top of my head.

(also, settings sendalled to false is not necessary, but it might be nice to do so anyway, as you have done)

Accounts created in a transaction will be in journal.dirties, so stateObject.created will be set to false in StateDB.Finalise. Upon revert, a created object will be removed from StateDB.stateObjects. So it doesn't appear that there can be
a situation where a state object created in a previous transaction can have created set to true.

adriandev111 · 2023-06-11T01:44:33Z

PLEASE DO NOT DO THIS PLEASE

Our project utilizes our own version of GasToken (similar to 1inch CHI Token) to save gas fees for our users. Although GasToken no longer saves gas, it still performs CREATE2 and SELFDESTRUCT operations in our contract logic. Our GasToken maintains an offset, consistently executing CREATE2 or SELFDESTRUCT at this offset. After this update, CREATE2 will fail if the current offset has not been properly SELFDESTRUCTed.

Our contracts are non-upgradable. After this update, most of our contract functions will revert, and user funds in our contracts, as well as liquidity in other DeFi projects, will be locked indefinitely.

We believe that making such breaking changes is highly unacceptable. We opted for non-upgradable contracts to maintain decentralization. Introducing breaking changes like this introduces significant uncertainty for developers and promotes the use of centralized, upgradable contracts in the Ethereum ecosystem.

holiman · 2023-06-11T07:47:03Z

This is the wrong forum for discussion -- this is a pr implementing a spec-change, so this is where we discuss the implementation, not the spec change.

You should raise this instead on e.g. forum of ethereum magicians, or somewhere listed in the discussions-to on EIP 6780. You should also be specific: what contract (what addresses), how much ether is at risk, and what is preventing you from draning these funds now, in advance. What is the usecase, and exactly how is it implemented?

core/state/journal.go

core/vm/instructions.go

holiman · 2023-07-07T08:49:01Z

Random observation: EIP 6780 removes an ancient quirk. Currently, doing selfdestruct(self) makes ether vanish from the state (as opposed to being just inaccessible, it actually removes it from existence).

With 6780, that no longer happens, at least not in this implementation.

core/state/statedb.go

core/state/state_object.go

jwasinger · 2023-07-11T17:06:35Z

Random observation: EIP 6780 removes an ancient quirk. Currently, doing selfdestruct(self) makes ether vanish from the state (as opposed to being just inaccessible, it actually removes it from existence).

With 6780, that no longer happens, at least not in this implementation.

So the wording of the EIP could be interpreted to mean that it is possible to burn Eth when calling selfdestruct to self within a contract that was created in the same transaction:

When SELFDESTRUCT is executed in the same transaction as the contract was created:
...
Transfer the account balance to the target and set account balance to 0

holiman · 2023-07-13T09:16:27Z

So the wording of the EIP could be interpreted to mean that it is possible to burn Eth when calling selfdestruct to self within a contract that was created in the same transaction:

As you remark: burn is still possible with selfdestruct-to-self in same transaction. I don't think that needs clarification, because the eip already says basically that "selfdetruct in same transaction is not modified".

However, any contracts that rely on the ability to burn via this feature will cease working. This may totally change the incentive mechanism behind the schema.

Anyway, it's not directly related to this PR.

cc @Dedaub any thoughts?

holiman · 2023-07-13T09:25:46Z

core/state/statedb.go

@@ -474,6 +474,18 @@ func (s *StateDB) Suicide(addr common.Address) bool {
 	return true
 }

+func (s *StateDB) Selfdestruct6780(addr common.Address) bool {


What is the meaning of the returnvalue here? I guess you just mimicked the previous Suicide... but seems like it's very unused (and totally undocumented)

Yeah, that appears to be the case. neither Suicide nor Selfdestruct6780 use the return value.

holiman

This PR became pretty straight-forward in the end, looks good to me. But before we merge it, I'd like some more tests covering it

jwasinger · 2023-07-13T09:50:09Z

core/state/statedb.go

+func (s *StateDB) Selfdestruct6780(addr common.Address) {
+	stateObject := s.getStateObject(addr)
+	if stateObject == nil {
+		return


It also looks like getStateObject will never return nil here because the currently executing address cannot be a deleted state object.

holiman · 2023-07-15T17:10:23Z

Needs a rebase now

holiman · 2023-07-15T18:11:41Z

>>> build/cache/golangci-lint-1.51.1-linux-amd64/golangci-lint run --config .golangci.yml ./...
core/state/state_object.go:91: File is not `goimports`-ed (goimports)
	created   bool 
util.go:48: exit status 1
exit status 1

core/vm/eips.go

core/vm/jump_table.go

holiman

LGTM!

Cherry pick ethereum/go-ethereum#27189 --------- Co-authored-by: jwasinger <j-wasinger@hotmail.com>

EIP-6780: SELFDESTRUCT only in same transaction > SELFDESTRUCT will recover all funds to the caller but not delete the account, except when called in the same transaction as creation --------- Co-authored-by: Martin Holst Swende <martin@swende.se>

Cherry pick ethereum/go-ethereum#27189 --------- Co-authored-by: jwasinger <j-wasinger@hotmail.com>

EIP-6780: SELFDESTRUCT only in same transaction > SELFDESTRUCT will recover all funds to the caller but not delete the account, except when called in the same transaction as creation --------- Co-authored-by: Martin Holst Swende <martin@swende.se>

This reverts commit 876a66c.

holiman reviewed May 10, 2023

View reviewed changes

jwasinger marked this pull request as ready for review June 8, 2023 15:18

jwasinger requested review from karalabe and rjl493456442 as code owners June 8, 2023 15:18

jwasinger force-pushed the eip-6780 branch from 7d3fb6c to c97c4e7 Compare June 8, 2023 16:53

holiman added the cancun label Jul 6, 2023

holiman reviewed Jul 7, 2023

View reviewed changes

core/state/journal.go Outdated Show resolved Hide resolved

holiman reviewed Jul 7, 2023

View reviewed changes

core/vm/instructions.go Outdated Show resolved Hide resolved

holiman reviewed Jul 7, 2023

View reviewed changes

core/state/statedb.go Outdated Show resolved Hide resolved

holiman reviewed Jul 7, 2023

View reviewed changes

core/state/state_object.go Outdated Show resolved Hide resolved

holiman reviewed Jul 13, 2023

View reviewed changes

jwasinger commented Jul 13, 2023

View reviewed changes

jwasinger force-pushed the eip-6780 branch from 35fcb12 to 842f76e Compare July 15, 2023 17:33

core/vm, core/state: implement eip-6780

d6e3e56

jwasinger force-pushed the eip-6780 branch from 842f76e to d6e3e56 Compare July 15, 2023 17:41

goimports

f419b84

holiman reviewed Jul 15, 2023

View reviewed changes

core/vm/eips.go Outdated Show resolved Hide resolved

core/vm/jump_table.go Outdated Show resolved Hide resolved

holiman added 2 commits July 15, 2023 14:41

Update core/vm/eips.go

95d7fc2

Update core/vm/jump_table.go

d5bc69e

holiman approved these changes Jul 15, 2023

View reviewed changes

holiman added this to the 1.12.1 milestone Jul 15, 2023

holiman merged commit 988d84a into ethereum:master Jul 17, 2023
1 of 2 checks passed

jwasinger deleted the eip-6780 branch July 17, 2023 17:03

holiman mentioned this pull request Aug 1, 2023

Execution Layer Meeting 167 ethereum/pm#836

Closed

yperbasis mentioned this pull request Aug 6, 2023

Implement EIP-6780: SELFDESTRUCT only in same transaction ledgerwatch/erigon#7976

Merged

yperbasis added a commit to ledgerwatch/erigon that referenced this pull request Aug 6, 2023

Implement EIP-6780: SELFDESTRUCT only in same transaction (#7976)

a25f22a

Cherry pick ethereum/go-ethereum#27189 --------- Co-authored-by: jwasinger <j-wasinger@hotmail.com>

jwasinger mentioned this pull request Aug 28, 2023

tests/cancun/eip6780: add test cases which use revert ethereum/execution-spec-tests#270

Merged

AskAlexSharov pushed a commit to ledgerwatch/erigon that referenced this pull request Sep 6, 2023

Implement EIP-6780: SELFDESTRUCT only in same transaction (#7976)

48d805a

Cherry pick ethereum/go-ethereum#27189 --------- Co-authored-by: jwasinger <j-wasinger@hotmail.com>

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "core/state, core/vm: implement EIP 6780 (ethereum#27189)"

a7ec785

This reverts commit 876a66c.

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "core/state, core/vm: implement EIP 6780 (ethereum#27189)"

47377e7

This reverts commit 876a66c.

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "core/state, core/vm: implement EIP 6780 (ethereum#27189)"

63dfd57

This reverts commit 876a66c.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/state, core/vm: implement EIP 6780 #27189

core/state, core/vm: implement EIP 6780 #27189

jwasinger commented Apr 28, 2023 •

edited by holiman

holiman May 10, 2023

jwasinger May 10, 2023

holiman May 10, 2023

jwasinger May 10, 2023

holiman May 10, 2023

jwasinger May 10, 2023

holiman May 10, 2023

jwasinger May 25, 2023

adriandev111 commented Jun 11, 2023

holiman commented Jun 11, 2023

holiman commented Jul 7, 2023

jwasinger commented Jul 11, 2023

holiman commented Jul 13, 2023 •

edited

holiman Jul 13, 2023

jwasinger Jul 13, 2023

holiman left a comment

jwasinger Jul 13, 2023

holiman commented Jul 15, 2023

holiman commented Jul 15, 2023

holiman left a comment

core/state, core/vm: implement EIP 6780 #27189

core/state, core/vm: implement EIP 6780 #27189

Conversation

jwasinger commented Apr 28, 2023 • edited by holiman

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

adriandev111 commented Jun 11, 2023

holiman commented Jun 11, 2023

holiman commented Jul 7, 2023

jwasinger commented Jul 11, 2023

holiman commented Jul 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman commented Jul 15, 2023

holiman commented Jul 15, 2023

holiman left a comment

Choose a reason for hiding this comment

jwasinger commented Apr 28, 2023 •

edited by holiman

holiman commented Jul 13, 2023 •

edited