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

Avoid panic tx error message in debug trace #2057

Merged
merged 4 commits into from
Jan 27, 2025
Merged

Avoid panic tx error message in debug trace #2057

merged 4 commits into from
Jan 27, 2025

Conversation

jewei1997
Copy link
Contributor

@jewei1997 jewei1997 commented Jan 27, 2025

Describe your changes and provide context

Previously when we used a dynamic gas precompile with an insufficient gas provided the transaction trace would look like this:

{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"panic occurred: runtime error: invalid memory address or nil pointer dereference, could not trace tx: 0xa33838c0ec1f74b360db030440fd4791f960917bdf4074315b6ce3006c3f4f0d"}}

This is because the precompile's gas meter panics. This PR catches that panic so that trace can look something like this. Notably, it fails with the canonical "execution reverted" instead of "panic occurred":

{"jsonrpc":"2.0","id":1,"result":{"from":"0xf87a299e6bc7beba58dbbe5a5aa21d49bcd16d52","gas":"0x9c40","gasUsed":"0x9c40","to":"0x0000000000000000000000000000000000001001","input":"0x6ff45dad0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002a73656931636a7a706872363764756732387277397565657772716c6c6d786c71653566306177756c767900000000000000000000000000000000000000000000","output":"0x657865637574696f6e2072657665727465643a207b5772697465506572427974657d","error":"execution reverted","value":"0x1","type":"CALL"}}

Testing performed to validate your change

added integration tests

@jewei1997 jewei1997 changed the title [wip] Avoid panic tx error message Avoid panic tx error message Jan 27, 2025
@jewei1997 jewei1997 changed the title Avoid panic tx error message Avoid panic tx error message in debug trace Jan 27, 2025
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

Attention: Patch coverage is 60.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 61.33%. Comparing base (eeb164d) to head (04711cc).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
precompiles/bank/bank.go 40.00% 2 Missing and 1 partial ⚠️
precompiles/oracle/oracle.go 40.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2057      +/-   ##
==========================================
+ Coverage   61.31%   61.33%   +0.01%     
==========================================
  Files         264      264              
  Lines       24589    24604      +15     
==========================================
+ Hits        15076    15090      +14     
  Misses       8385     8385              
- Partials     1128     1129       +1     
Files with missing lines Coverage Δ
precompiles/addr/addr.go 64.88% <100.00%> (+1.07%) ⬆️
precompiles/bank/bank.go 60.31% <40.00%> (-0.42%) ⬇️
precompiles/oracle/oracle.go 64.61% <40.00%> (-2.06%) ⬇️

... and 3 files with indirect coverage changes

@jewei1997 jewei1997 merged commit bc47cc9 into main Jan 27, 2025
50 of 54 checks passed
@jewei1997 jewei1997 deleted the panic-tx-fix branch January 27, 2025 21:49
philipsu522 pushed a commit that referenced this pull request Jan 27, 2025
philipsu522 added a commit that referenced this pull request Jan 30, 2025
* Add v6.0.3 upgrade

* Changelog

* Properly encode ERC1155 translated batch event data

* update changelog

* Avoid panic tx error message in debug trace (#2057)

* checkpoint

* fix

* fix

---------

Co-authored-by: Tony Chen <[email protected]>
Co-authored-by: Jeremy Wei <[email protected]>
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