EIP-6780 selfdestruct tracing behaviour

[s1na]

As per EIP-6780 a contract is fully selfdestructed if it was created within the same tx-scope. Otherwise it will act as send all, transfering all ether to recipient. The tracing behaviour here needs discussion as pointed out by @nebojsa94. Current behaviour is:

• OnEnter(SELFDESTRUCT, ...) is emitted after trying to selfdestruct, even if contract is "old" and cannot be deleted.
• OnBalanceChange is emitted only if an actual transfer took place (prev balance > 0)
• OnCodeChange is emitted if contract was destroyed

The question is if OnEnter(SELFDESTRUCT) should be emitted even if contract wasn't destroyed due to being "old".

Note: I realized while reviewing the code we have a bug in the OnBalanceChange emitting logic:

go-ethereum/core/state/statedb_hooked.go

Line 245 in c3ef6c7

if s.hooks.OnBalanceChange != nil && changed && !prev.IsZero() {

This condition seems wrong. Even if changed == false the transfer can take place.

[s1na]

My first intuition was that only listening to OnBalanceChange and OnCodeChange should be enough because they are the relevant changes to the state. However I was told that this is not enough because some txes create empty bytecode.

It means the tracer will receive OnCodeChange(prev, new) where new is empty. This is very similar to the selfdestruct pattern and it's not possible to distinguish the two.

I think we should add a reason to OnCodeChange. Also IMO the OnEnter hook for selfdestruct served a purpose before we had state change hooks but is useless now and can be removed.

[maoueh]

The question is if OnEnter(SELFDESTRUCT) should be emitted even if contract wasn't destroyed due to being "old".

Good question, it's true indeed that there is other means to check that, for example we use OnOpCode to determine if a self destruct is initiated. We actually skip OnEnter/OnExit when the opcode is SELFDESTRUCT.

That being said, the self destruct actually happened, so it seems it would make sense to have some hooks for it. The added benefit of a OnEnter/OnExit pattern vs a single hook like OnOpCode is that you can "trap" traces that were done due to the self destruct happening, since you have known "time window" to gather traces that happened due to the self destruct.

Also, I have the feeling OnOpCode is an heavy "hook" just for an opcode that happen once in a while in a block.

Overall, I have the feeling that self destruct is important enough that it should have it's own tracing, but that's a personal feeling, not backed by some facts.

Note: I realized while reviewing the code we have a bug in the OnBalanceChange emitting logic:

Interesting, I can say that removing changed from the condition make our integration test suite fails for the test case selfdestruct: contract address is same as beneficary, this tests happen in two transactions in two different blocks and test the case where a contract is suicided and the beneficiary of the self destruct is the contract itself.

In that case, here the diff:

$ cat ./test/snapshots/suicide/fh3.0/contract_and_suicide_beneficiary_are_the_same.expected.resolved.json | jq -c '.calls[1].balanceChanges[]'
{"address":"a8af72e694b34788f5078cb91fe4bc928cefef5d","reason":"REASON_SUICIDE_WITHDRAW","ordinal":"11","oldValue":"01","newValue":""}
{"address":"a8af72e694b34788f5078cb91fe4bc928cefef5d","reason":"REASON_SUICIDE_REFUND","ordinal":"12","newValue":"01","oldValue":""}

$ cat ./test/snapshots/suicide/fh3.0/contract_and_suicide_beneficiary_are_the_same.actual.normalized.json | jq -c '.calls[1].balanceChanges[]'
{"address":"a8af72e694b34788f5078cb91fe4bc928cefef5d","reason":"REASON_SUICIDE_WITHDRAW","ordinal":"11","oldValue":"01","newValue":""}
{"address":"a8af72e694b34788f5078cb91fe4bc928cefef5d","reason":"REASON_SUICIDE_REFUND","ordinal":"12","newValue":"01","oldValue":""}
{"address":"a8af72e694b34788f5078cb91fe4bc928cefef5d","reason":"REASON_SUICIDE_WITHDRAW","ordinal":"13","oldValue":"01","newValue":""}

Where expected is the value we though was correctly traced and the actual is that new tracing we get with changed removed. So I think you are right, I would need to double-check that indeed a transfer happened, but should be easy to double-check that.

I can properly also create a unit test from my own integration test directly.