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: various fixes #30540

Merged
merged 7 commits into from
Oct 17, 2024
Merged

eth/tracers: various fixes #30540

merged 7 commits into from
Oct 17, 2024

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Oct 3, 2024

Changes in this PR

Breaking

  • The ChainConfig was exposed to tracers via VMContext passed in OnTxStart. This is unnecessary specially looking through the lens of live tracers as chain config remains the same throughout the lifetime of the program. It was there so that native API-invoked tracers could access it. So instead we moved it to the constructor of API tracers.

Non-breaking

  • Change the default config of the tracers to be {} instead of nil. This way an extra nil check can be avoided.

Refactoring

  • Rename supply struct to supplyTracer.
  • Un-export some hook definitions.

@s1na s1na changed the title core/tracing: various fixes eth/tracers: various fixes Oct 3, 2024
@s1na
Copy link
Contributor Author

s1na commented Oct 4, 2024

I think some other fields in VMContext which are block-related can also be moved to the constructor. But keeping that for future work.

eth/tracers/native/call.go Outdated Show resolved Hide resolved
eth/tracers/native/noop.go Outdated Show resolved Hide resolved
@karalabe
Copy link
Member

karalabe commented Oct 7, 2024

Apart of the nits, SGTM

@s1na
Copy link
Contributor Author

s1na commented Oct 9, 2024

I have addressed the comments. PTAL.

@holiman
Copy link
Contributor

holiman commented Oct 10, 2024

Please add a description in the description-field. Mostly so that

  1. Whoever makes the next release can get a quick overview about what this PR is all about, whether it's relevant to mention in release notes, and, if so, what to mention
  2. So that if we ever need to go back and investigate, after finding a bug, why something was changed, we can more easily understand why the change was performed.

Doesn't have to be a novel. It can be sufficient with "This PR addresses minor internal rough edges and should have no observable difference". (Note, haven't checked the code yet, have no idea if that really is the case here, just an example)

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Some tiny nits, otherwise lgtm

cmd/utils/flags.go Outdated Show resolved Hide resolved
eth/backend.go Show resolved Hide resolved
@karalabe
Copy link
Member

We should have the chain config passed for the live tracer constructors and drop the blockchain init hook altogether - unless there's a good reason to have it.

It might also be interesting to think about how we could access the state / chain on init.

@s1na
Copy link
Contributor Author

s1na commented Oct 11, 2024

We should have the chain config passed for the live tracer constructors and drop the blockchain init hook altogether - unless there's a good reason to have it.

That's a breaking change for live tracers. I would rather not do it in this PR.

Ah come to think of it removing ChainConfig from the VMContext is also a breaking change for live tracers.

@s1na s1na merged commit 978ca5f into ethereum:master Oct 17, 2024
3 checks passed
@karalabe
Copy link
Member

Then all current APIs should be moved into a v1 package and a v2 created that's designed better. It is unacceptable like it is now.

@fjl fjl added this to the 1.14.12 milestone Oct 17, 2024
holiman pushed a commit that referenced this pull request Nov 19, 2024
Breaking changes:

- The ChainConfig was exposed to tracers via VMContext passed in
`OnTxStart`. This is unnecessary specially looking through the lens of
live tracers as chain config remains the same throughout the lifetime of
the program. It was there so that native API-invoked tracers could
access it. So instead we moved it to the constructor of API tracers.

Non-breaking:

- Change the default config of the tracers to be `{}` instead of nil.
This way an extra nil check can be avoided.

Refactoring:

- Rename `supply` struct to `supplyTracer`.
- Un-export some hook definitions.
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