Fix SELFDESTRUCT incorrect account zeroing & missing beneficiary balance in witness #378

jsign · 2024-02-13T13:33:54Z

This PR fixes SELFDESTRUCT behavior aligned with EIP-6780:

It fixes a bug where we were doing "zero writes" to the tree in the case the contract calling SELFDESTRUCT was created in the same tx.
If fixes a bug where we weren't doing proper touching in the witness for the SELFDESTRUCT beneficiary.

It also adds four tests to cover different cases.

jsign · 2024-02-13T13:35:07Z

core/state_processor_test.go

+		0xFF, // SELFDESTRUCT
+	}
+	selfDestructContractAddr := common.HexToAddress("3a220f351252089d385b29beca14e27f204c296a")
+	_, _, _, statediff := GenerateVerkleChain(gspec.Config, genesis, beacon.New(ethash.NewFaker()), gendb, 2, func(i int, gen *BlockGen) {


I prefered to run two blocks:

First block creates the contract.

Second block runs the contract.

Technically, we could do this with two tx in the same block -- but separating in blocks is better to avoid the indirect "merge" of witnesses of the two tx in a same block.

jsign · 2024-02-13T13:35:38Z

core/state_processor_test.go

+		// The original balance was 42.
+		var fourtyTwo [32]byte
+		fourtyTwo[0] = 42
+		if *balanceStateDiff.CurrentValue != fourtyTwo {
+			t.Fatalf("the pre-state balance before self-destruct must be 42")
+		}


The pre-state of the second block for the balance should've 42, which is the balance sent when the contract was created in the first block.

jsign · 2024-02-13T13:35:51Z

core/state_processor_test.go

+		// The new balance must be 0.
+		if *balanceStateDiff.NewValue != zero {
+			t.Fatalf("the post-state balance after self-destruct must be 0")
+		}


Post-state should be zero, since it was sent to the beneficiary.

jsign · 2024-02-13T13:40:12Z

core/state_processor_test.go

+		if balanceStateDiff.CurrentValue == nil {
+			t.Fatalf("codeHash.CurrentValue must not be empty")
+		}
+		if balanceStateDiff.NewValue == nil {
+			t.Fatalf("codeHash.NewValue must not be empty")
+		}
+		preStateBalance := binary.LittleEndian.Uint64(balanceStateDiff.CurrentValue[:])
+		postStateBalance := binary.LittleEndian.Uint64(balanceStateDiff.NewValue[:])
+		if postStateBalance-preStateBalance != 42 {
+			t.Fatalf("the post-state balance after self-destruct must be 42")
+		}


The beneficiary pre and post state balance should have increased by 42.

jsign · 2024-02-13T13:40:36Z

core/state_processor_test.go

+	selfDestructContract := []byte{
+		0x73,                                                                                                                   // PUSH20
+		0x61, 0x77, 0x84, 0x3d, 0xb3, 0x13, 0x8a, 0xe6, 0x96, 0x79, 0xA5, 0x4b, 0x95, 0xcf, 0x34, 0x5E, 0xD7, 0x59, 0x45, 0x0d, // 0x6177843db3138ae69679A54b95cf345ED759450d
+		0xFF, // SELFDESTRUCT
+	}
+	selfDestructContractAddr := common.HexToAddress("3a220f351252089d385b29beca14e27f204c296a")
+	_, _, _, statediff := GenerateVerkleChain(gspec.Config, genesis, beacon.New(ethash.NewFaker()), gendb, 1, func(i int, gen *BlockGen) {
+		gen.SetPoS()
+		tx, _ := types.SignTx(types.NewContractCreation(0, big.NewInt(42), 100_000, big.NewInt(875000000), selfDestructContract), signer, testKey)
+		gen.AddTx(tx)
+	})


Here the strategy is to directly SELFDESTRUCT in the contract deployment.

jsign · 2024-02-13T13:41:02Z

core/state_processor_test.go

+		if balanceStateDiff.CurrentValue != nil {
+			t.Fatalf("the pre-state balance before must be nil, since the contract didn't exist")
+		}
+
+		if balanceStateDiff.NewValue != nil {
+			t.Fatalf("the post-state balance after self-destruct must be nil since the contract shouldn't be created at all")
+		}


For the self-destructed contract, both the pre and post state should be nil. This checks the bug that was found in Kaustinen.

jsign · 2024-02-13T13:41:21Z

core/state_processor_test.go

+			t.Fatalf("the post-state balance after self-destruct must be nil since the contract shouldn't be created at all")
+		}
+	}
+	{ // Check self-destructed target in the witness.


The beneficiary checks are the same as the previous test.

core/state_processor_test.go

jsign · 2024-02-13T13:42:03Z

core/state_processor_test.go

+		if balanceStateDiff.NewValue != nil {
+			t.Fatalf("the post-state balance after self-destruct must be nil since the contract shouldn't be created at all")
+		}


Double check that despite the beneficiary is the contract itself, it doesn't appear in the post state. (i.e: the ETH was "burned").

gballet

I think there's an issue in the case when the selfdestructed contract is also the beneficiary, since the balance will be read anyway. The rest LGTM.

gballet · 2024-02-14T16:05:39Z

trie/verkle.go

-	for i := 0; i < verkle.NodeWidth; i++ {
-		values[i] = zero[:]
-	}


oh I see, this code should never have made it to this branch ! 🤯 This is meant for the replay to simulate an account destruction. It shouldn't be there. Well, that clearly explains this bug then.

core/vm/instructions.go

jsign · 2024-02-14T18:00:53Z

core/state/statedb.go

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


Quite unfortunate that we need to add an extra method to StateDB, but it's a simple one.
Open to renaming the method if sounds too verbose.

jsign · 2024-02-14T18:01:13Z

core/state_processor_test.go

+		// The way 6780 is implemented today, it always SubBalance from the self-destructed contract, and AddBalance
+		// to the beneficiary. In this case both addresses are the same, thus this might be optimizable from a gas
+		// perspective. But until that happens, we need to honor this "balance reading" adding it to the witness.


Explaining a bit why we need this test since might not be obvious for future readers.

jsign · 2024-02-14T18:01:25Z

core/state_processor_test.go

+		// Note that the SubBalance+AddBalance net effect is a 0 change, so NewValue
+		// must be nil.
+		if balanceStateDiff.NewValue != nil {
+			t.Fatalf("the post-state balance after self-destruct must be empty")
+		}


This is an important check.

jsign · 2024-02-14T18:01:39Z

core/vm/instructions.go

+		// 1. The contract was created in the same transaction: the balance is already touched (no need to touch again)
+		// 2. The contract wasn't created in the same transaction: there's no net change in balance,
+		//    and SELFDESTRUCT will perform no action on the account header. (we touch since we did SubBalance+AddBalance above)
+		if contractAddr != beneficiaryAddr || interpreter.evm.StateDB.WasCreatedInCurrentTx(contractAddr) {


The added || ... condition.

Signed-off-by: Ignacio Hagopian <[email protected]>

gballet

LGTM

…nce in witness (#378) * fix SELFDESTRUCT witness recording Signed-off-by: Ignacio Hagopian <[email protected]> * add selfdestruct tests Signed-off-by: Ignacio Hagopian <[email protected]> * solve lint nit Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]>

jsign commented Feb 13, 2024

View reviewed changes

jsign changed the title ~~Fix SELFDESTRUCT behavior~~ Fix SELFDESTRUCT incorrect account zeroing & missing beneficiary balance in witness Feb 13, 2024

gballet reviewed Feb 14, 2024

View reviewed changes

jsign requested a review from gballet February 14, 2024 18:00

jsign commented Feb 14, 2024

View reviewed changes

jsign marked this pull request as ready for review February 14, 2024 18:01

jsign added 2 commits February 15, 2024 13:38

fix SELFDESTRUCT witness recording

3f562b4

Signed-off-by: Ignacio Hagopian <[email protected]>

add selfdestruct tests

434aa40

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign force-pushed the jsign-self-destruct-bug branch from faeef6d to 434aa40 Compare February 15, 2024 16:41

solve lint nit

8a93810

Signed-off-by: Ignacio Hagopian <[email protected]>

gballet approved these changes Feb 16, 2024

View reviewed changes

gballet added this to the verkle-gen-devnet-5 milestone Feb 16, 2024

gballet merged commit 5f05456 into kaustinen-with-shapella Feb 16, 2024
2 of 3 checks passed

jsign mentioned this pull request Mar 5, 2024

Fix TestProcessVerkle tests #400

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SELFDESTRUCT incorrect account zeroing & missing beneficiary balance in witness #378

Fix SELFDESTRUCT incorrect account zeroing & missing beneficiary balance in witness #378

jsign commented Feb 13, 2024 •

edited

Loading

jsign Feb 13, 2024

jsign Feb 13, 2024

jsign Feb 13, 2024

jsign Feb 13, 2024

jsign Feb 13, 2024

jsign Feb 13, 2024

jsign Feb 13, 2024

jsign Feb 13, 2024

gballet left a comment

gballet Feb 14, 2024

jsign Feb 14, 2024

jsign Feb 14, 2024

jsign Feb 14, 2024

jsign Feb 14, 2024

gballet left a comment

Fix SELFDESTRUCT incorrect account zeroing & missing beneficiary balance in witness #378

Fix SELFDESTRUCT incorrect account zeroing & missing beneficiary balance in witness #378

Conversation

jsign commented Feb 13, 2024 • 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

gballet 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

gballet left a comment

Choose a reason for hiding this comment

jsign commented Feb 13, 2024 •

edited

Loading