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

Flat call tracer #14

Open
wants to merge 54 commits into
base: master
Choose a base branch
from
Open

Flat call tracer #14

wants to merge 54 commits into from

Conversation

ziogaschr
Copy link
Member

No description provided.

@ziogaschr ziogaschr force-pushed the feat/flat-call-tracer branch from 28d97d1 to f41caea Compare November 21, 2022 19:07
}

func (t *flatCallTracer) fillCallFrameFromContext(callFrame *flatCallFrame) {
if t.ctx != nil {
Copy link

Choose a reason for hiding this comment

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

Hm I notice the context doesn't have the block number which is usually part of the parity trace. I'll have to look into why

Copy link
Member Author

@ziogaschr ziogaschr Dec 21, 2022

Choose a reason for hiding this comment

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

BlockNumber has to be added, as said the PR is not finished.
I would like to know your thoughts on using a proxy tracers to call_tracer or a new tracer for flat_tracer.

The one thing I liked with the proxy one, is that we will have a single point in codebase to maintain the tracer logic. Although this approach, has some nitpicks in order to work.

Example nitpick which is not pushed here follows. Here and because we don't have access to callTracer.callstack we have to keep track of some stuff in this tracer ourselves. Making the callTracer.**C**allstack public will help, but then it will make more trivial to maintain the callTracer if we want to apply any changes.

// CaptureStart implements the EVMLogger interface to initialize the tracing operation.
func (t *flatCallTracer) CaptureStart(env *vm.EVM, from common.Address, to common.Address, create bool, input []byte, gas uint64, value *big.Int) {
	// Update list of precompiles based on current block
	rules := env.ChainConfig().Rules(env.Context.BlockNumber, env.Context.Random != nil)
	t.activePrecompiles = vm.ActivePrecompiles(rules)

	// this is a workaround in order to exclude precompile calls in CaptureExit where we don't have the proxied tracer context
	t.currentCall = flatCallFrame{
		Type: strings.ToLower(vm.CALL.String()),
		Action: FlatCallTraceAction{
			From: &from,
			To:   &to,
			// Gas:   gas,
			// Value: value,
		},
	}

	if create {
		t.currentCall.Type = strings.ToLower(vm.CREATE.String())
	}

	t.tracer.CaptureStart(env, from, to, create, input, gas, value)
}

Copy link

Choose a reason for hiding this comment

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

is that we will have a single point in codebase to maintain the tracer logic

I agree with that. As long as there aren't many work-arounds needed because of differences between parity-vs-geth styles like the one mentioned. That depends on whether we want a flatTracer or we want the parityTracer. Because as you mentioned parity tracer has some non-cosmetic differences like precompiles being excluded. Do you know what are the other differences?

You can cast the t.tracer.(*callTracer) and then you'll have access to callstack since they're in the same package. That should make things easier.

Comparing this version and the one in core-geth I must admit I like this one more. Even if we don't go with the proxy approach I think we should track the calls in the same shape that they will be returned (i.e. flat) so we don't need post-processing apart from toHex and toJSON conversions. I.e. avoid the Finalize method in your code. I realize a challenge is that we need access to the call in CaptureExit. But we can find the index of the call in the flat list by storing an additional stack that only tracks indices.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because as you mentioned parity tracer has some non-cosmetic differences like precompiles being excluded.

Do you find a bad idea to add an option in CallTracer that allows user to decide? excludePrecompiles: boolean

You can cast the t.tracer.(*callTracer) and then you'll have access to callstack since they're in the same package. That should make things easier.

Nice one. Will check it and think if it adds more complexity than needed for maintenance later on.

}

// GetResult returns an empty json object.
func (t *flatCallTracer) GetResult() (json.RawMessage, error) {
Copy link

Choose a reason for hiding this comment

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

Here we're doing an extra marshal and unmarshal. A call to t.tracer.GetResult is not necessary. We can directly access its callstack and process the output.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this make things a lot easier. Will try to work on this direction using this proxy tracer and see how it goes.
Will be nice if you can reply here and with a go/no-go.

Value *hexutil.Big
Input hexutil.Bytes
Output hexutil.Bytes
Type stringOpCode `json:"type"`
Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think of these changes here @s1na? Was thinking that even this PR doesn't need this change, it is good to have it upstream for gen_callframe_json.go, so as if/when needed to be used, it will work as expected.

s1na and others added 30 commits December 28, 2022 15:52
…om JSON Marshaller

eth/tracers/native: change struct fields to pointers to be ommited from JSON Marshaller
…/flat_calltrace_test.go

Co-authored-by: Sina Mahmoodi <1591639+s1na@users.noreply.github.com>
For compatibility with Parity tracer we had removed the internal calls to precompiles for CALL/STATICCALL.

Geth decides to present them so as to present the full information in the tracers
`convertParityErrors` tracer config option  has been used in order to keep errors same as in parity for compatibility.

Though, being compatibility in this level is not needed and is being removed for sanity
…piles

For compatibility with Parity tracer we had removed the internal calls to precompiles for CALL/STATICCALL.

Geth decides to present them so as to present the full information in the tracers

For compatibility with Parity tracer we added the option `IncludePrecompiles` in order to include/exclude the internal calls to precompiles for CALL/STATICCALL.
…r “convertParityErrors” flatCall tracer option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants