-
Notifications
You must be signed in to change notification settings - Fork 20.2k
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
core/tracing: extends tracing.Hooks with OnSystemCallStartV2 #30786
base: master
Are you sure you want to change the base?
core/tracing: extends tracing.Hooks with OnSystemCallStartV2 #30786
Conversation
The context won't be well defined here, missing coinbase and gasprice.
As for motivation, could you please instead explain the practical reason why you want/need this? The provided motivation is a bit theoretical, imo:
|
Sure, I've responded in a second PR with a practical usecase |
On It's of course a personal opinion, I'm quite fine if there is a preference to keep backward compatibility and introduce a |
This is a good catch actually which for me confirms we should remove gas price from vm context and replace it with base fee as per #30809. Although I think coinbase will be available I don't see why it shouldn't be. So IMO it's ok.
I think it's ok and we can add a new method for this. We have to eventually set up a process for making backwards incompatible changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR extends the Hooks interface with a new method,
OnSystemCallStartV2
, which takesVMContext
as its parameter.Motivation
By including
VMContext
as a parameter, theOnSystemCallStartV2
hook achieves parity with theOnTxStart
hook in terms of provided insights. This alignment simplifies the inner tracer logic, enabling consistent handling of state changes and internal calls within the same framework.