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

evm: Fix the gasCost logs in op code trace #2686

Merged
merged 1 commit into from
May 10, 2023
Merged

Conversation

g11tech
Copy link
Contributor

@g11tech g11tech commented May 10, 2023

While matching op code trace of ethereumjs with the geth it was observed that ethereumjs's gasCost logs were incorrect w.r.t the actual gas used by the opcode.

This was happening because the dynamicFee should be the one to be logged which is the actual gas consumed by the opcode

Post this the gasCost mismatch diff with geth greatly reduced and the 0 gasCost entries disappeared from the trace

@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

❗ No coverage uploaded for pull request base (develop-v7@dd19ccf). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 90.30% <0.00%> (?)
blockchain 90.48% <0.00%> (?)
client 86.86% <0.00%> (?)
common 95.76% <0.00%> (?)
devp2p 91.84% <0.00%> (?)
ethash ∅ <0.00%> (?)
evm 79.33% <0.00%> (?)
rlp ∅ <0.00%> (?)
statemanager 88.53% <0.00%> (?)
trie 90.02% <0.00%> (?)
util 81.34% <0.00%> (?)
vm 84.39% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM

@g11tech g11tech merged commit 8b84f5d into develop-v7 May 10, 2023
@holgerd77 holgerd77 deleted the fix-evm-gas-logs branch June 6, 2023 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants