Fix trace call for inner reverts

[ngotchac]

The native TraceCall tracer wasn't capturing the output/revert reason if last inner calls were reverting. This fixes this, and adds a new test based on this dummy contract:

contract Revertor {
    function run() public pure {
        require(2 > 3, "This called failed");
    }
}

contract Contract {
    constructor() {
        Revertor r = new Revertor();
        r.run();
    }
}

I had to tweak a bit the tests setup because it couldn't detect the transaction I sent using geth --dev

Backwards-compatibility note

In a call frame of type CREATE, the field to will contain normally the address of the created address. In case of call failure the to field contained inconsistent values depending on context. If call frame was the top-level call, then the to field still contained the address, but for nested calls the to field was cleared to the 0x000...00 address. This inconsistency has been fixed. I.e. a failing top-level call frame of type CREATE will have its to field set to 0x000...00.

[holiman]

Nice catch!
It would be good if we can also commit the solidity contracts somewhere, if not in the json, then maybe in the go-code somewhere, or an accompanying readme. Not sure, up for suggestions.

[holiman]

This looks strange, to me

[ngotchac]

So that's because it's a contract creation, but I guess there shouldn't be any to field then?

[holiman]

If we omit this clause, then one of the existing tests, innerCreateOogOuterThrow fails, beacuse it expects one of the inner creates to have 000... instead of an actual address.
The current behaviour is a bit strange to me.

1. 0x000.. is an actual valid address. We should not really use this as a "special denominator meaning failed". We could use null if we actually means "does not exist".
2. As it is, the outermost scope shows the address on a failed create. But inner creates for some reason "clean up" the create-address on failed creates.

@s1na do you have any idea where this quirk comes from?

[holiman]

Hm, with omitempty set, from https://github.com/ethereum/go-ethereum/pull/25637/files, the idea is that 0x000.. should not be present in the output json. So it's basically null. Which is a bit wonky too, but.. Still I wonder why we have this clause there.

[holiman]

I think the original difference between outer scope vs inner scope, which was introduced here: 8d7e606#diff-c7b3691786d65b22103e27eca0ca1e6650ddf78874b7326bf7afcc30bf6d2b2fR118 -- is because it originally needed to look up the call in a t.callstack. And the outermost scope didn't have preceding entry in the callstack.

As for why the to was set to empty, 8d7e606#diff-c7b3691786d65b22103e27eca0ca1e6650ddf78874b7326bf7afcc30bf6d2b2fR131 , I'm not completely sure.

[ngotchac]

So here I just kept what was there for the CaptureExit case. Regarding a "null" to field, maybe we can have a To *common.Address (so a pointer) with omitempty, so that we can have trully null values, different from just the 0x00..00 address. WDYT?

[ngotchac]

It would be good if we can also commit the solidity contracts somewhere, if not in the json, then maybe in the go-code somewhere, or an accompanying readme.

Yeah I wanted to put the contract code somewhere but didn't really find where. I can add a inner_revert_reason.md next to the inner_revert_reason.json file for this maybe?

[holiman]

I think there are two things that needs to be decided.

1. For failed CREATE, should we clear the address-which-wasn't-created ?
   1.a Clear it for non-toplevel scope (current master-behaviour)
   1.b. Clear it for all scopes (this PR)
   1.c Don't clear it

I prefer 1.b over 1.a, for consistency. And I suspect the reason we clear it is to make it easy for a caller to tell whether a contract was created or not. So I'm leaning towards 1.b -- mostly keeping the current behaviour, but making it more consistent. @s1na WDYT?

Then there's a second decision to make, which is how to handle omitempty. If someone sends an ether to the 0x00...00 address, then the address should be shown, as a regular address. Not hidden due to omitempty rules. Therefore, we may need to redefine something as a *common.Address. I don't think we need to solve this second problem in this PR.

[holiman]

I can add a inner_revert_reason.md next to the inner_revert_reason.json file for this maybe?

Sounds good to me!

[holiman]

Ah, actually..

func TestFooBar(t *testing.T) {
	type test struct {
		Address common.Address `json:"to,omitempty"`
		BytesA  hexutil.Bytes  `json:"bytesA,omitempty"`
		BytesB  hexutil.Bytes  `json:"bytesB,omitempty"`
		String  string         `json:"string,omitempty"`
	}

	var enc test
	enc.Address = common.Address{}
	enc.BytesA = make([]byte, 0, 0)
	enc.BytesB = nil
	enc.String = ""
	outp, err := json.MarshalIndent(&enc, "", "  ")
	if err != nil {
		t.Fatal(err)
	}
	t.Logf("enc: %v\n", string(outp))
}

Output:

=== RUN   TestFooBar
    calltrace_test.go:403: enc: {
          "to": "0x0000000000000000000000000000000000000000"
        }
--- PASS: TestFooBar (0.00s)

So, the omitempty is useless on common.Address. So the problem is not calls to zero looks like contract-creation, but rather contract-creation looks like calls to zero.

[ngotchac]

@holiman I added a description in a Markdown for the added test (with the contract code), I updated the callFrame definition for To to have a pointer, so it's nil when no contract was created, and I updated the tests' results accordingly. Let me know if this works for you!

[holiman]

Ping @s1na re #25971 (comment)

[s1na]

So when I ported the tracer to Go I merely adopted this behavior which I also suspect is to make it easier to detect when creation failed. But you're right that it's inconsistent now. My preference would be 1c (not clearing it for any scope). The reasoning is that the error field can be checked to see whether creation failed or not. And users will still be able to see the potential address. E.g. in case of collision they can check what exists at that address.

[ngotchac]

I think you're right @s1na ; I updated the PR so it's doesn't change the To field in case of errors, and updated the test data accordingly.

[s1na]

I'm sorry for the extra hassle but can you please revert the To clearing logic? we decided to do that for logs in #25991 (comment), and it would be good to stay consistent on this behavior. After all created contract and logs are both artefacts that are discarded when a call fails.

Also in the meanwhile #26048 was merged which has some merge conflicts with this PR. The gist of the change there is gasUsed field for the top-level call is now computed in TxEnd instead of End hook.

[ngotchac]

No worries, all good! I'll change it back

please revert the To clearing logic

Would it be better to have To common.Address, thus set to 0x00..00 or To *common.Address set to nil/inexistant in the marhsalled JSON?

[ngotchac]

@s1na Done with cleared To set to 0x00..00

[holiman]

and it would be good to stay consistent on this behavior.

@s1na good call -- also because it's closer to the current behaviour, so less chance of breaking things.

[s1na]

LGTM thanks!