Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: switch EVM tx context in ApplyMessage #30809
core: switch EVM tx context in ApplyMessage #30809
Changes from 3 commits
1b79f97
cafdfc6
0e69f7a
beb45ec
57d0f64
96f76a5
f335a50
de82fd5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@s1na it might be an incompatible change, but I feel like it's the right direction.
The effectiveGasPrice could be derived with BaseFee (block context) and tx gas price.
Originally, we must to invoke evm.SetTxContext before the TxStartHook, with this change, the requirement is gone
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.
Hmm.. Why not just move the tracer stuff a bit? We ought to be able to get the tx-specific stuff into
OnTxStart
?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.
Wait a minute. The TxStart hook:
It has the
tx
. The tx has aGasPrice
method. Why would we have it also in theVMContext
? Gary's change makes total senseThere 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.
Sounds good to me
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.
For info, in our case, we indeed take the
gasPrice
from the transaction and not from the*VMContext
struct in our tracer.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.
@maoueh Just want to mention that the GasPrice taken from the transaction object is the different with one in the
*tracing.VMContext
, the first one refers to the full gas price while the latter one refers to the effectiveGasPriceThere 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.
@rjl493456442 Yes thanks for the info. We are handling the different tx types to pick the correct gas values.
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.
I think we should bring it back in another tracing API version. The tracer implementation should not have to compute the effective gas price by itself. It should be handled by the tracing infrastructure.