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.

Sign up for GitHub

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

Jump to bottom

eth/tracers: remove revertReasonTracer, add revert reason to callTracer #25508

Merged
merged 3 commits into from
Sep 29, 2022
Merged

eth/tracers: remove revertReasonTracer, add revert reason to callTracer #25508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
328c212
eth/tracers: add revertReason to callTracer
s1na Jul 28, 2022
bd713b8
update callframe gen json
s1na Sep 27, 2022
73d4755
add revertal to calltrace test
s1na Sep 27, 2022
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
21 changes: 11 additions & 10 deletions eth/tracers/internal/tracetest/calltrace_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,16 +104,17 @@ type callContext struct {

// callTrace is the result of a callTracer run.
type callTrace struct {
Type string `json:"type"`
From common.Address `json:"from"`
To common.Address `json:"to"`
Input hexutil.Bytes `json:"input"`
Output hexutil.Bytes `json:"output"`
Gas *hexutil.Uint64 `json:"gas,omitempty"`
GasUsed *hexutil.Uint64 `json:"gasUsed,omitempty"`
Value *hexutil.Big `json:"value,omitempty"`
Error string `json:"error,omitempty"`
Calls []callTrace `json:"calls,omitempty"`
Type string `json:"type"`
From common.Address `json:"from"`
To common.Address `json:"to"`
Input hexutil.Bytes `json:"input"`
Output hexutil.Bytes `json:"output"`
Gas *hexutil.Uint64 `json:"gas,omitempty"`
GasUsed *hexutil.Uint64 `json:"gasUsed,omitempty"`
Value *hexutil.Big `json:"value,omitempty"`
Error string `json:"error,omitempty"`
Revertal string `json:"revertReason,omitempty"`
Calls []callTrace `json:"calls,omitempty"`
}

// callTracerTest defines a single test to check the call tracer against.
Expand Down
3 changes: 2 additions & 1 deletion eth/tracers/internal/tracetest/testdata/call_tracer/revert_reason.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
"to": "0xf58833cf0c791881b494eb79d461e08a1f043f52",
"type": "CALL",
"value": "0x0",
"output": "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000001e53656c662d64656c65676174696f6e20697320646973616c6c6f7765642e0000"
"output": "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000001e53656c662d64656c65676174696f6e20697320646973616c6c6f7765642e0000",
"revertReason": "Self-delegation is disallowed."
}
}
39 changes: 24 additions & 15 deletions eth/tracers/native/call.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"sync/atomic"
"time"

"github.com/ethereum/go-ethereum/accounts/abi"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/ethereum/go-ethereum/core/vm"
Expand All @@ -36,15 +37,16 @@ func init() {
}

type callFrame struct {
Type vm.OpCode `json:"-"`
From common.Address `json:"from"`
Gas uint64 `json:"gas"`
GasUsed uint64 `json:"gasUsed"`
To common.Address `json:"to,omitempty" rlp:"optional"`
Input []byte `json:"input" rlp:"optional"`
Output []byte `json:"output,omitempty" rlp:"optional"`
Error string `json:"error,omitempty" rlp:"optional"`
Calls []callFrame `json:"calls,omitempty" rlp:"optional"`
Type vm.OpCode `json:"-"`
From common.Address `json:"from"`
Gas uint64 `json:"gas"`
GasUsed uint64 `json:"gasUsed"`
To common.Address `json:"to,omitempty" rlp:"optional"`
Input []byte `json:"input" rlp:"optional"`
Output []byte `json:"output,omitempty" rlp:"optional"`
Error string `json:"error,omitempty" rlp:"optional"`
Revertal string `json:"revertReason,omitempty"`
Calls []callFrame `json:"calls,omitempty" rlp:"optional"`
// Placed at end on purpose. The RLP will be decoded to 0 instead of
// nil if there are non-empty elements after in the struct.
Value *big.Int `json:"value,omitempty" rlp:"optional"`
Expand Down Expand Up @@ -109,13 +111,20 @@ func (t *callTracer) CaptureStart(env *vm.EVM, from common.Address, to common.Ad
func (t *callTracer) CaptureEnd(output []byte, gasUsed uint64, _ time.Duration, err error) {
t.callstack[0].GasUsed = gasUsed
output = common.CopyBytes(output)
if err != nil {
t.callstack[0].Error = err.Error()
if err.Error() == "execution reverted" && len(output) > 0 {
t.callstack[0].Output = output
}
} else {
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 {
Copy link
Member

Choose a reason for hiding this comment

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

Feels strange to just ignore the error if it happens. Also to just silently not do anything if the len(output) is too short. IMHO we should at least log a warning to the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave this a try, and it seems it's not so uncommon for this unpack to fail. Because some contracts have custom revert data instead of the string messages that solidity returns by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, there's nothing that mandates that a contract's revert message is ABI-formatted. If it's not, then I think it's fine to not put anything into the "revert-message according to abi specificaton" (and yes, ignore the error)

t.callstack[0].Revertal = unpacked
}
}

Expand Down
36 changes: 21 additions & 15 deletions eth/tracers/native/gen_callframe_json.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

108 changes: 0 additions & 108 deletions eth/tracers/native/revertreason.go
View file
Open in desktop

This file was deleted.