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

blockchain: clearly distinguish vm error from other errors #1763

Merged
merged 3 commits into from Apr 17, 2023

Conversation

yoomee1313
Copy link
Contributor

@yoomee1313 yoomee1313 commented Jan 17, 2023

Proposed changes

  • klaytn code distinguishes the vm error and other errors, but the code was not clear enough. By bringing the ethereum PR, it clearly distinguish vm error from other errors.

  • vm error is an internal evm errror and we record this kind of error at the transaction receipt. If not, we don't record it, instead the error is returned immediately(call) or causes a bad block error(sendTransaction).

  • This PR is a refactoring, so the output of the rpc does not changes compared to the dev branch except revert reason.

    • dev: There's no 'data' field when there's revert reason.
    • PR: There's 'data' field when there's revert reason. It is a hexutil encoded value of the revert reason.
➜  klaytn_private curl -H "Content-Type: application/json" --data '{...}, "latest"], "id": 1}' http://localhost:8559 
{"jsonrpc":"2.0","id":1,"error":{"code":3,"message":"evm: execution reverted: new value should not be same as old value","data":"0x08c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000296e65772076616c75652073686f756c64206e6f742062652073616d65206173206f6c642076616c75650000000000000000000000000000000000000000000000"}}
  • This PR reorganizes the return values of TransitionDb() and ApplyMessage()
Before PR After PR Meaning
ret []byte ExecutionResult.ReturnData []byte
~ result.Return(), result.Revert()
Execution result (solidity return values)
gas uint64 ExecutionResult.UsedGas uint64 Gas used
kerror.Status uint ExecutionResult.VmExecutionStatus uint
~ result.Unwrap(), result.Failed()
ex: Success(1), OutOfGas(7), Reverted(9)
kerror.ErrTxInvalid error err error ex: "insufficient balance of the sender"

In turn, some internal functions' return values are reorganized as well.

Before PR After PR
EthDoCall() ->
returnData, gas, status, err
EthDoCall() ->
ExecutionResult, err
DoCall() ->
returnData, gas, compCost, status, error
DoCall() ->
ExecutionResult, compCost, err

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

  • Please leave the issue numbers or links related to this PR here.

Further comments

@kjeom
Copy link
Contributor

kjeom commented Jan 26, 2023

I left a few minor reviews and this PR looks more clear than before. Thank you for hard work.

I think we can remove the duplicated code(DoEstimateGas) in the eth/klay namespace,
but I think it would be nice to do this later while refactoring the API.

@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/CodeQL.yml:CodeQL-Build. As part of the setup process, we have scanned this repository and found 6 existing alerts. Please check the repository Security tab to see all alerts.

@yoomee1313
Copy link
Contributor Author

@kjeom I'm sorry for the late. Thank you for your patience. I agree with you about the refactoring of DoEstimate. I'll reflect it in another PR cause this PR is already too big.

@yoomee1313 yoomee1313 requested a review from kjeom April 3, 2023 01:43
@yoomee1313 yoomee1313 merged commit b78a2c3 into klaytn:dev Apr 17, 2023
11 checks passed
@yoomee1313 yoomee1313 deleted the seperate-error-code branch April 17, 2023 09:24
@JayChoi1736 JayChoi1736 mentioned this pull request Jul 24, 2023
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants