Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves RPC error handling in the execution client by properly distinguishing between transient and fatal JSON-RPC errors. The changes ensure that transient errors like server errors (-32000) and internal errors (-32603) trigger infinite retries, while fatal errors like parse errors (-32700) cause immediate failure without retrying.
Changes:
- Added comprehensive test coverage for both transient and fatal RPC error scenarios using an HTTP proxy pattern
- Introduced
isTransientErrorhelper function to classify transient errors (ErrServer, ErrInternal) that should be retried - Updated
IsFatalErrorandIsNonFatalErrorlogic to properly exclude transient errors from fatal classification - Added
ErrorCode()method to RPC Error type to support error code inspection
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| testing/simulated/rpc_errors_test.go | New test suite with HTTP proxy to inject JSON-RPC errors and verify recovery behavior for transient errors and immediate failure for fatal errors |
| execution/client/errors.go | Added isTransientError helper and updated error classification logic to properly handle transient vs fatal errors |
| execution/client/ethclient/rpc/types.go | Added ErrorCode() method to satisfy jsonrpc.Error interface |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3029 +/- ##
==========================================
+ Coverage 62.63% 62.69% +0.05%
==========================================
Files 357 357
Lines 17224 17226 +2
==========================================
+ Hits 10788 10799 +11
+ Misses 5580 5572 -8
+ Partials 856 855 -1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I'm not sure we need to update behavior for the jsonrpc.ErrServer, jsonrpc.ErrInternal errors. Can you confirm from the EL (or maybe even CL spec) in which case these are returned and how they should be handled. I'm more inclined to file a ticket for this (to investigate further) and leave this change out of this bug fix PR @bar-bera
There was a problem hiding this comment.
Agree, lets keep the IsFatalError/IsNonFatalError logic the same in this PR and look at them separately
There was a problem hiding this comment.
ok agree to not touch the logic, just keep the test structure in case we want to test different behavior for different errors
Not sure here on what we should consider transient as an error, beside
-32000. Also added-32603for now but maybe should be considered fatal.