Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix trace call for inner reverts #25971

Merged
merged 3 commits into from
Oct 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,6 @@
"gas": "0x20ee1",
"gasUsed": "0x5374",
"input": "0x581d5d60000000000000000000000000c212e03b9e060e36facad5fd8f4435412ca22e6b0000000000000000000000000000000000000000000000280faf689c35ac0000",
"output": "0x",
"to": "0xcf00ffd997ad14939736f026006498e3f099baaf",
"type": "CALL",
"value": "0x0"
Expand Down Expand Up @@ -305,7 +304,6 @@
"gas": "0x1a91d",
"gasUsed": "0x12fa",
"input": "0x0accce0600000000000000000000000000000000000000000000000000000000000000025842545553440000000000000000000000000000000000000000000000000000000000000000000000000000c212e03b9e060e36facad5fd8f4435412ca22e6b00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
"output": "0x",
"to": "0x2a98c5f40bfa3dee83431103c535f6fae9a8ad38",
"type": "CALL",
"value": "0x0"
Expand Down Expand Up @@ -377,7 +375,6 @@
"gas": "0x16e62",
"gasUsed": "0xebb",
"input": "0x645a3b72584254555344000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002816d180e30c390000",
"output": "0x",
"to": "0x2a98c5f40bfa3dee83431103c535f6fae9a8ad38",
"type": "CALL",
"value": "0x0"
Expand All @@ -387,7 +384,6 @@
"gas": "0x283b9",
"gasUsed": "0xc51c",
"input": "0x949ae479000000000000000000000000c212e03b9e060e36facad5fd8f4435412ca22e6b0000000000000000000000000000000000000000000000280faf689c35ac0000",
"output": "0x",
"to": "0x3e9286eafa2db8101246c2131c09b49080d00690",
"type": "CALL",
"value": "0x0"
Expand All @@ -397,7 +393,6 @@
"gas": "0x30b4a",
"gasUsed": "0xedb7",
"input": "0x51a34eb80000000000000000000000000000000000000000000000280faf689c35ac0000",
"output": "0x",
"to": "0xb4fe7aa695b326c9d219158d2ca50db77b39f99f",
"type": "CALL",
"value": "0x0"
Expand All @@ -407,7 +402,6 @@
"gas": "0x37b38",
"gasUsed": "0x1810b",
"input": "0x51a34eb80000000000000000000000000000000000000000000000280faf689c35ac0000",
"output": "0x",
"to": "0xc212e03b9e060e36facad5fd8f4435412ca22e6b",
"type": "CALL",
"value": "0x0"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@
"gas": "0x2d6e28",
"gasUsed": "0xbd55",
"input": "0x7065cb480000000000000000000000001523e55a1ca4efbae03355775ae89f8d7699ad9e",
"output": "0x",
"to": "0x269296dddce321a6bcbaa2f0181127593d732cba",
"type": "CALL",
"value": "0x0"
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,6 @@
"value": "0x0",
"gas": "0x1a466",
"gasUsed": "0x72de",
"input": "0x2e1a7d4d00000000000000000000000000000000000000000000000014d1120d7b160000",
"output": "0x",
"calls": []
"input": "0x2e1a7d4d00000000000000000000000000000000000000000000000014d1120d7b160000"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
{
"genesis": {
"baseFeePerGas": "1000000000",
"difficulty": "1",
"extraData": "0x00000000000000000000000000000000000000000000000000000000000000003623191d4ccfbbdf09e8ebf6382a1f8257417bc10000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
"gasLimit": "11500000",
"hash": "0x2af138b8a06e65b8dd0999df70b9e87609e9fc91ea201f08b1cc4f25ef01fcf6",
"miner": "0x0000000000000000000000000000000000000000",
"mixHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
"nonce": "0x0000000000000000",
"number": "0",
"stateRoot": "0xa775801d572e9b79585eb131d18d79f8a0f71895455ab9a5b656911428e11708",
"timestamp": "0",
"totalDifficulty": "1",
"alloc": {
"0x3623191d4ccfbbdf09e8ebf6382a1f8257417bc1": {
"balance": "0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7"
},
"0xd15abca351f79181dedfb6d019e382db90f3628a": {
"balance": "0x0"
}
},
"config": {
"chainId": 1337,
"homesteadBlock": 0,
"eip150Block": 0,
"eip150Hash": "0x0000000000000000000000000000000000000000000000000000000000000000",
"eip155Block": 0,
"eip158Block": 0,
"byzantiumBlock": 0,
"constantinopleBlock": 0,
"petersburgBlock": 0,
"istanbulBlock": 0,
"muirGlacierBlock": 0,
"berlinBlock": 0,
"londonBlock": 0,
"clique": {
"period": 0,
"epoch": 30000
}
}
},
"context": {
"number": "1",
"difficulty": "2",
"timestamp": "1665537018",
"gasLimit": "11511229",
"miner": "0x0000000000000000000000000000000000000000"
},
"input": "0x02f9029d82053980849502f90085010c388d00832dc6c08080b90241608060405234801561001057600080fd5b50600060405161001f906100a2565b604051809103906000f08015801561003b573d6000803e3d6000fd5b5090508073ffffffffffffffffffffffffffffffffffffffff1663c04062266040518163ffffffff1660e01b815260040160006040518083038186803b15801561008457600080fd5b505afa158015610098573d6000803e3d6000fd5b50505050506100af565b610145806100fc83390190565b603f806100bd6000396000f3fe6080604052600080fdfea264697066735822122077f7dbd3450d6e817079cf3fe27107de5768bb3163a402b94e2206b468eb025664736f6c63430008070033608060405234801561001057600080fd5b50610125806100206000396000f3fe6080604052348015600f57600080fd5b506004361060285760003560e01c8063c040622614602d575b600080fd5b60336035565b005b60036002116076576040517f08c379a0000000000000000000000000000000000000000000000000000000008152600401606d906097565b60405180910390fd5b565b6000608360128360b5565b9150608c8260c6565b602082019050919050565b6000602082019050818103600083015260ae816078565b9050919050565b600082825260208201905092915050565b7f546869732063616c6c6564206661696c6564000000000000000000000000000060008201525056fea264697066735822122033f8d92e29d467e5ea08d0024eab0b36b86b8cdb3542c6e89dbaabeb8ffaa42064736f6c63430008070033c001a07566181071cabaf58b70fc41557eb813bfc7a24f5c58554e7fed0bf7c031f169a0420af50b5fe791a4d839e181a676db5250b415dfb35cb85d544db7a1475ae2cc",
"result": {
"from": "0x3623191d4ccfbbdf09e8ebf6382a1f8257417bc1",
"to": "0x0000000000000000000000000000000000000000",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks strange, to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"gas": "0x2cd774",
"gasUsed": "0x25590",
"input": "0x608060405234801561001057600080fd5b50600060405161001f906100a2565b604051809103906000f08015801561003b573d6000803e3d6000fd5b5090508073ffffffffffffffffffffffffffffffffffffffff1663c04062266040518163ffffffff1660e01b815260040160006040518083038186803b15801561008457600080fd5b505afa158015610098573d6000803e3d6000fd5b50505050506100af565b610145806100fc83390190565b603f806100bd6000396000f3fe6080604052600080fdfea264697066735822122077f7dbd3450d6e817079cf3fe27107de5768bb3163a402b94e2206b468eb025664736f6c63430008070033608060405234801561001057600080fd5b50610125806100206000396000f3fe6080604052348015600f57600080fd5b506004361060285760003560e01c8063c040622614602d575b600080fd5b60336035565b005b60036002116076576040517f08c379a0000000000000000000000000000000000000000000000000000000008152600401606d906097565b60405180910390fd5b565b6000608360128360b5565b9150608c8260c6565b602082019050919050565b6000602082019050818103600083015260ae816078565b9050919050565b600082825260208201905092915050565b7f546869732063616c6c6564206661696c6564000000000000000000000000000060008201525056fea264697066735822122033f8d92e29d467e5ea08d0024eab0b36b86b8cdb3542c6e89dbaabeb8ffaa42064736f6c63430008070033",
"output": "0x08c379a000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000012546869732063616c6c6564206661696c65640000000000000000000000000000",
"error": "execution reverted",
"revertReason": "This called failed",
"calls": [
{
"from": "0xdebfb4b387033eac57af7b3de5116dd60056803b",
"gas": "0x2ba851",
"gasUsed": "0xe557",
"to": "0xd15abca351f79181dedfb6d019e382db90f3628a",
"input": "0x608060405234801561001057600080fd5b50610125806100206000396000f3fe6080604052348015600f57600080fd5b506004361060285760003560e01c8063c040622614602d575b600080fd5b60336035565b005b60036002116076576040517f08c379a0000000000000000000000000000000000000000000000000000000008152600401606d906097565b60405180910390fd5b565b6000608360128360b5565b9150608c8260c6565b602082019050919050565b6000602082019050818103600083015260ae816078565b9050919050565b600082825260208201905092915050565b7f546869732063616c6c6564206661696c6564000000000000000000000000000060008201525056fea264697066735822122033f8d92e29d467e5ea08d0024eab0b36b86b8cdb3542c6e89dbaabeb8ffaa42064736f6c63430008070033",
"output": "0x6080604052348015600f57600080fd5b506004361060285760003560e01c8063c040622614602d575b600080fd5b60336035565b005b60036002116076576040517f08c379a0000000000000000000000000000000000000000000000000000000008152600401606d906097565b60405180910390fd5b565b6000608360128360b5565b9150608c8260c6565b602082019050919050565b6000602082019050818103600083015260ae816078565b9050919050565b600082825260208201905092915050565b7f546869732063616c6c6564206661696c6564000000000000000000000000000060008201525056fea264697066735822122033f8d92e29d467e5ea08d0024eab0b36b86b8cdb3542c6e89dbaabeb8ffaa42064736f6c63430008070033",
"value": "0x0",
"type": "CREATE"
},
{
"from": "0xdebfb4b387033eac57af7b3de5116dd60056803b",
"gas": "0x2ac548",
"gasUsed": "0x1b2",
"to": "0xd15abca351f79181dedfb6d019e382db90f3628a",
"input": "0xc0406226",
"output": "0x08c379a000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000012546869732063616c6c6564206661696c65640000000000000000000000000000",
"error": "execution reverted",
"revertReason": "This called failed",
"type": "STATICCALL"
}
],
"value": "0x0",
"type": "CREATE"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
This test tests out the trace generated by the deployment of this contract:

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

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

The trace should show a revert, with the revert reason for both the top-call as well
as the inner call.
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
"gas": "0x0",
"gasUsed": "0x0",
"input": "0x",
"to": "0x000000000000000000000000000000000000dEaD",
"to": "0x000000000000000000000000000000000000dead",
"type": "SELFDESTRUCT",
"value": "0x4d87094125a369d9bd5"
}
Expand All @@ -67,7 +67,6 @@
"gas": "0x10738",
"gasUsed": "0x6fcb",
"input": "0x63e4bff40000000000000000000000000024f658a46fbb89d8ac105e98d7ac7cbbaf27c5",
"output": "0x",
"to": "0x3b873a919aa0512d5a0f09e6dcceaa4a6727fafe",
"type": "CALL",
"value": "0x0"
Expand Down
48 changes: 24 additions & 24 deletions eth/tracers/native/call.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,28 @@ func (f callFrame) TypeString() string {
return f.Type.String()
}

func (f *callFrame) capture(output []byte, err error) {
output = common.CopyBytes(output)
if err == nil {
f.Output = output
return
}
f.Error = err.Error()
if f.Type == vm.CREATE || f.Type == vm.CREATE2 {
f.To = common.Address{}
}
Comment on lines +66 to +68
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@holiman holiman Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if !errors.Is(err, vm.ErrExecutionReverted) || len(output) == 0 {
return
}
f.Output = output
if len(output) < 4 {
return
}
if unpacked, err := abi.UnpackRevert(output); err == nil {
f.Revertal = unpacked
}
}

type callFrameMarshaling struct {
TypeString string `json:"type"`
Gas hexutil.Uint64
Expand Down Expand Up @@ -110,22 +132,7 @@ func (t *callTracer) CaptureStart(env *vm.EVM, from common.Address, to common.Ad

// CaptureEnd is called after the call finishes to finalize the tracing.
func (t *callTracer) CaptureEnd(output []byte, gasUsed uint64, _ time.Duration, err error) {
output = common.CopyBytes(output)
if err == nil {
t.callstack[0].Output = output
return
}
t.callstack[0].Error = err.Error()
if !errors.Is(err, vm.ErrExecutionReverted) || len(output) == 0 {
return
}
t.callstack[0].Output = output
if len(output) < 4 {
return
}
if unpacked, err := abi.UnpackRevert(output); err == nil {
t.callstack[0].Revertal = unpacked
}
t.callstack[0].capture(output, err)
}

// CaptureState implements the EVMLogger interface to trace a single step of VM execution.
Expand Down Expand Up @@ -174,14 +181,7 @@ func (t *callTracer) CaptureExit(output []byte, gasUsed uint64, err error) {
size -= 1

call.GasUsed = gasUsed
if err == nil {
call.Output = common.CopyBytes(output)
} else {
call.Error = err.Error()
if call.Type == vm.CREATE || call.Type == vm.CREATE2 {
call.To = common.Address{}
}
}
call.capture(output, err)
t.callstack[size-1].Calls = append(t.callstack[size-1].Calls, call)
}

Expand Down