Skip to content

fix(evmrpc): return null for out-of-range tx index and fix deferred error capture#3344

Merged
amir-deris merged 7 commits into
mainfrom
amir/plt-294-evmrpc-tx-index-overflow-uint32-error
May 1, 2026
Merged

fix(evmrpc): return null for out-of-range tx index and fix deferred error capture#3344
amir-deris merged 7 commits into
mainfrom
amir/plt-294-evmrpc-tx-index-overflow-uint32-error

Conversation

@amir-deris

@amir-deris amir-deris commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Problem

Two bugs in evmrpc/tx.go:

  1. Deferred error capture was always nildefer recordMetricsWithError(...) was called directly rather than inside a closure, so returnErr was captured at defer-registration time (always nil) instead of at function-return time. This caused metrics to never record actual errors.

  2. uint32 overflow returned an error instead of nulleth_getTransactionByBlockNumberAndIndex and eth_getTransactionByBlockHashAndIndex returned an RPC error when the transaction index exceeded math.MaxUint32. The Ethereum JSON-RPC spec requires these methods to return null for an out-of-range index, not an error.

Fix

  • Wrapped all defer recordMetricsWithError(...) calls in closures so returnErr is evaluated at return time.
  • Introduced a typed txUint32OverflowError so the overflow case can be distinguished from other errors.
  • After recording metrics, suppresses the overflow error (sets returnErr = nil) in GetTransactionByBlockNumberAndIndex and GetTransactionByBlockHashAndIndex to comply with the Ethereum JSON-RPC spec.

Test plan

  • eth_getTransactionByBlockNumberAndIndex with a tx index > 0xFFFFFFFF returns null, not an error
  • eth_getTransactionByBlockHashAndIndex with a tx index > 0xFFFFFFFF returns null, not an error

@amir-deris amir-deris self-assigned this Apr 29, 2026
@amir-deris amir-deris changed the title Amir/plt 294 evmrpc tx index overflow uint32 error fix(evmrpc): return null for out-of-range tx index and fix deferred error capture Apr 29, 2026
@github-actions

github-actions Bot commented Apr 29, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedMay 1, 2026, 5:40 PM

@amir-deris amir-deris requested review from bdchatham and masih April 29, 2026 23:27
Comment thread evmrpc/tx.go Outdated
startTime := time.Now()
defer recordMetricsWithError("eth_getTransactionReceipt", t.connectionType, startTime, returnErr)
defer func() {
recordMetricsWithError("eth_getTransactionReceipt", t.connectionType, startTime, returnErr)

@amir-deris amir-deris Apr 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The previous format defer functionCall(...) captures arguments at the call site which results in returnErr be nil for recordMetricsWithError.

@masih masih May 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A go convention that's worth following is to use _ prefix for named return variables to indicate that "this variable must never be directly set". I would stick with simple names too. e.g. _err.

@codecov

codecov Bot commented Apr 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.65217% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.17%. Comparing base (e68b82f) to head (c1e155f).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
evmrpc/tx.go 95.65% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             main    #3344    +/-   ##
========================================
  Coverage   59.16%   59.17%            
========================================
  Files        2097     2097            
  Lines      172613   172377   -236     
========================================
- Hits       102129   102002   -127     
+ Misses      61617    61533    -84     
+ Partials     8867     8842    -25     
Flag Coverage Δ
sei-chain-pr 64.77% <95.65%> (?)
sei-db 70.41% <ø> (ø)

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

Files with missing lines Coverage Δ
evmrpc/tx.go 85.48% <95.65%> (+0.46%) ⬆️

... and 52 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@amir-deris amir-deris enabled auto-merge May 1, 2026 17:42
@amir-deris amir-deris added this pull request to the merge queue May 1, 2026
Merged via the queue into main with commit 3cb756f May 1, 2026
38 checks passed
@amir-deris amir-deris deleted the amir/plt-294-evmrpc-tx-index-overflow-uint32-error branch May 1, 2026 18:05
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.

3 participants