Skip to content
This repository was archived by the owner on Aug 19, 2024. It is now read-only.

Call tracing result compatibility with Ethereum#1953

Merged
sjnam merged 6 commits intoklaytn:devfrom
sjnam:debug-trace
Sep 13, 2023
Merged

Call tracing result compatibility with Ethereum#1953
sjnam merged 6 commits intoklaytn:devfrom
sjnam:debug-trace

Conversation

@sjnam
Copy link

@sjnam sjnam commented Sep 4, 2023

Proposed changes

This PR

  • matches the call tracing result with that of Ethereum.
  • collects execution tracer even if an account is not a program account.
  • fixes the mismatching of gasUsed field of the test result.

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@sjnam sjnam self-assigned this Sep 4, 2023
@blukat29 blukat29 marked this pull request as draft September 4, 2023 03:16
@blukat29 blukat29 marked this pull request as ready for review September 4, 2023 04:15
@hyunsooda
Copy link
Contributor

The existing implementation did not consider the 'initial gas' for the usage in the following context. The passed argument to the context of EVM is the residual gas that subtracts the intrinsic gas. You may refer the geth patch for this problem.

blukat29
blukat29 previously approved these changes Sep 7, 2023
hyunsooda
hyunsooda previously approved these changes Sep 7, 2023
// virtual machine configuration options used to initialise the
// evm.
vmConfig *Config
VmConfig *Config
Copy link
Contributor

Choose a reason for hiding this comment

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

How about the name VMConfig instead?

yoomee1313
yoomee1313 previously approved these changes Sep 7, 2023
Copy link
Contributor

@yoomee1313 yoomee1313 left a comment

Choose a reason for hiding this comment

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

@sjnam Shouldn't the description be updated? Previously, gasUsed field was not matched with receipt when it's CA, but with PR, it became matched. Isn't it?

@sjnam sjnam dismissed stale reviews from yoomee1313, hyunsooda, and blukat29 via 4340f39 September 7, 2023 13:07
@sjnam sjnam closed this Sep 13, 2023
@sjnam sjnam reopened this Sep 13, 2023
@sjnam sjnam merged commit f50a9b6 into klaytn:dev Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants