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

core/tracing: document OnCodeChange now being called from SelfDestruct #31007

Merged
merged 3 commits into from
Jan 16, 2025
Merged

core/tracing: document OnCodeChange now being called from SelfDestruct #31007

merged 3 commits into from
Jan 16, 2025

Conversation

maoueh
Copy link
Contributor

@maoueh maoueh commented Jan 8, 2025

No description provided.

@maoueh maoueh requested a review from s1na as a code owner January 8, 2025 18:53
rjl493456442
rjl493456442 reviewed Jan 9, 2025
View reviewed changes
core/tracing/CHANGELOG.md Outdated

### `OnCodeChange` change

The `OnCodeChange` is now called when `StateDB#SelfDestruct` or `StateDB#SelfDestruct6780` is invoked, which happens when a contract self destruct itself. Previously, no code change was emitted on such occasions.
Copy link
Member

Choose a reason for hiding this comment

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

It's not accurate.

OnCodeChange is invoked only when StateDB#SelfDestruct is called, or when StateDB#SelfDestruct6780 is called and the contract is actually destroyed.

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've re-worded to be clearer on SelfDestruct6780.

Do you think we should also document when those situations happens and on which fork 6780 is actually used?

@holiman holiman changed the title Document OnCodeChange now being called in SelfDestruct core/tracing: document OnCodeChange now being called from SelfDestruct Jan 13, 2025
s1na
s1na reviewed Jan 15, 2025
View reviewed changes
core/tracing/CHANGELOG.md Outdated Show resolved Hide resolved
s1na
s1na approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

LGTM thanks. That reminds me there are a few other changes that should go into the changelog

@s1na
Copy link
Contributor

s1na commented Jan 16, 2025 •
edited
Loading

Is this the clarification for #31011 btw? Can we close that or that's a separate issue?

@s1na s1na merged commit 4d94bd8 into ethereum:master Jan 16, 2025
2 checks passed
@maoueh maoueh deleted the patch-1 branch January 20, 2025 18:59
@maoueh
Copy link
Contributor Author

maoueh commented Jan 20, 2025

@s1na No not at all. I'm not sure why the issue was created on go-ethereum since it happened on Optimism which uses a fork of Geth.

The panicked occurred due to some speculative execution of the block, so the live tracer was active from two block tracing code paths which cannot work with our Firehose tracer.

All in all, it's unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjl493456442 rjl493456442 rjl493456442 left review comments

@s1na s1na s1na approved these changes

Assignees

@s1na s1na

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maoueh @s1na @rjl493456442