eth/tracers: check the parent callframe's length before removing precompile call

[minh-bq]

When using flat call tracer, by default, we remove the call to precompile. However, we should check the length of parent's callframe before doing that because when OnlyTopCall option is provided the call tracer's CaptureExit does not push the child's callframe to parent's callframe. As a result, the parent's callframe is empty, trying to remove the precompile call leads to index out of range. This commit checks the length of parent's callframe before removing precompile call.

[s1na]

Good find!

I'm a bit torn on this. I feel like we should clear the config for the flat call tracer. For example WithLogs is also not implemented yet. For that we'll have to change the format and add in logs.

Do you need OnlyTopCall for the flatCallTracer? or discovered this by accident?

[minh-bq]

Yes, I also feel that call tracer configs are not intended to be used when flat call tracer is used. Our team does not use the OnlyTopCall for flatCallTracer, just find it during testing.

So, do you prefer this solution, not pass the config from flat call tracer down to inner call tracer?

diff --git a/eth/tracers/native/call_flat.go b/eth/tracers/native/call_flat.go
index a9c056f27..1334b8baa 100644
--- a/eth/tracers/native/call_flat.go
+++ b/eth/tracers/native/call_flat.go
@@ -129,6 +129,7 @@ func newFlatCallTracer(ctx *tracers.Context, cfg json.RawMessage) (tracers.Trace
                }
        }
 
+       cfg, _ = json.Marshal(callTracerConfig{})
        tracer, err := tracers.DefaultDirectory.New("callTracer", ctx, cfg)
        if err != nil {
                return nil, err

[ziogaschr]

Nice find!
There is a possibility that the OnlyTopCall is not useful for flatCallTracer. Though it might be preferably to keep it in, as flatCallTracer is a proxy tracer that mostly formats the output of callTracer.

For some stuff, I wouldn't suggest touching the base formatting as this might be useful for endusers consuming the Parity tracer in past. On the other hand, extending on top of this tracer feels welcoming, and OnlyTopCall can be easily kept.

About the WithLogs, we can ignore it in the config by passing a false value in the CallTracer, and why not later add support for it.

I think it makes sense to have all the logic and base functionality coming from the CallTracer and having flatCallTracer only doing the formatting and needed changes.

[s1na]

Honestly I'd prefer to clear the config for now and only allow those options if someone asks for it. The reason we added the flatCallTracer is to have compatibility with other clients. I'd say if we want to e.g. add logs we should consult with them and do it together.

@minh-bq that patch looks good!

[ziogaschr]

@s1na yes that a valid point. It's wiser to do this in discussion with other clients when needed.

[ziogaschr]

Do you think the suggestion below will miss any edge cases in case the CallTracer.CaptureExit gets modified? Probably the way you coded it is more safe.

Suggested change

	if len(t.tracer.callstack[len(t.tracer.callstack)-1].Calls) == 0 {
	if t.tracer.config.OnlyTopCall {

[minh-bq]

This suggestion looks good to me

[minh-bq]

Look like we prefer to clear the config passed to inner call tracer, don't want to edit this PR description and title for reference, I create another PR: #27304 for that implementation.