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

eth/tracers: move tracing APIs into eth/tracers #22161

Merged
merged 4 commits into from
Jan 25, 2021

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Jan 12, 2021

This PR moves the tracing RPC API implementation to package eth/tracers.
By doing so, package eth no longer depends on tracing and the go-duktape JS engine.

The change also enables tracing using the light client. All tracing methods work with the
light client, but it's a lot slower compared to using a full node.

Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

I think we should use the existing EthAPIBackend as the backend object for TracerAPI.
It's almost compatible, just need to add the StateAtBlock and StateAtTransaction methods to it.

The TracerAPI should be renamed to API because it's now in the package "tracers".

cmd, eth: rename

eth: remove blank lines

eth: fix empty block

cmd, eth, internal, les: update for tracers

eth/tracers: polish

eth, les: update tests

eth/tracers: fix rpc namespace and visibility

eth/tracers: retrieve the txlookup from the backend

eth: updates

eth/tracers: updates

eth: remove state.Reset
@rjl493456442
Copy link
Member Author

@fjl @karalabe The tracer logic has been changed a lot. Please take a look if it's on the right track.

// Append all the local APIs and return
return []rpc.API{
{
Namespace: "debug",
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it make sense to rename the namespace to trace?
Though we can't drop existing namespace support for debug_trace* methods easily as some users might use it in their scripts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, trace_traceTransaction sounds weird to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can rename to trace_transaction as we have their own namespace now.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't rename these methods without keeping the original ones alive, since they have been around for quite a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of having proxy methods of the old ones to the new ones for a while (until next major release?) with a deprecated warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm generally in favor of renaming because the debug namespace is quite messy and also includes some dangerous functionality that should never be exposed, such as debug_traceBlockToFile. If we can make a 'safe' subset of tracing APIs, it would be great to have them available in a dedicated namespace.

@fjl fjl changed the title cmd, eth: move tracer APIs to its own package eth/tracers: move tracing APIs into eth/tracers Jan 25, 2021
eth/state_accessor.go Outdated Show resolved Hide resolved
internal/ethapi/backend.go Outdated Show resolved Hide resolved
eth/tracers/api.go Outdated Show resolved Hide resolved
les/state_accessor.go Outdated Show resolved Hide resolved
@rjl493456442
Copy link
Member Author

@fjl It's updated.

Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great. I'm especially happy to see the added test for tracing.

@fjl fjl added this to the 1.10.0 milestone Jan 25, 2021
@fjl fjl merged commit adf130d into ethereum:master Jan 25, 2021
bulgakovk pushed a commit to bulgakovk/go-ethereum that referenced this pull request Jan 26, 2021
This moves the tracing RPC API implementation to package eth/tracers.
By doing so, package eth no longer depends on tracing and the duktape JS engine.

The change also enables tracing using the light client. All tracing methods work with the
light client, but it's a lot slower compared to using a full node.
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.

4 participants