Skip to content

evmrpc: return null on above-watermark for spec-compliant endpoints#3530

Merged
wen-coding merged 7 commits into
mainfrom
wen/watermark_null_for_jsonrpc
Jun 2, 2026
Merged

evmrpc: return null on above-watermark for spec-compliant endpoints#3530
wen-coding merged 7 commits into
mainfrom
wen/watermark_null_for_jsonrpc

Conversation

@wen-coding
Copy link
Copy Markdown
Contributor

@wen-coding wen-coding commented Jun 1, 2026

Summary

Several Ethereum JSON-RPC endpoints that take a block identifier currently surface the watermark race ("requested height N is not yet available; safe latest is N-1") as an error, where the Ethereum JSON-RPC spec says they should return JSON null (block doesn't exist yet from the caller's perspective). Wallets, indexers, and similar tools see this as a transient failure exactly when a tx has just been mined and the safe-latest watermark hasn't caught up.

#3119 fixed eth_getBlockByNumber and #3501 fixed eth_getTransactionReceipt with the same if errors.Is(err, ErrBlockHeightNotYetAvailable) { return nil, nil } pattern. This PR generalizes the pattern into two helpers and applies it to the remaining user‑facing endpoints.

Endpoints converted

RPC method File:line
eth_getBlockReceipts block.go
eth_getBlockTransactionCountByNumber block.go
eth_getBlockTransactionCountByHash block.go
eth_getBlockByHash + sei_getBlockByHashExcludeTraceFail (shared getBlockByHash) block.go
eth_getTransactionByBlockNumberAndIndex tx.go
eth_getTransactionByBlockHashAndIndex tx.go
eth_getTransactionByHash tx.go

Historic context (why propagation was the previous default)

The watermark integration (#2465) deliberately propagated the error so clients would know "data not yet ready — retry later" rather than getting a misleading null. That was the original conservative default across all evmrpc endpoints that touched a height.

Since then, subsequent PRs have progressively chosen JSON null per Ethereum spec on a per‑endpoint basis:

PR Endpoint Rationale
#3067 (Moji) eth_getBlockByHash (unknown hash) PLT‑162: "return result null for non‑existent hashes"
#3119 (Moji) eth_getBlockByNumber (above watermark) "Align with expectations" (Ethereum spec)
#3320 (Wen) eth_getTransactionByBlock*AndIndex (out‑of‑range index) "Per Ethereum JSON‑RPC spec, these methods must return null"
#3501 eth_getTransactionReceipt (above watermark) spec
#3530 (this) the remaining 7 user‑facing endpoints continues the same pattern

So the propagation on these 7 endpoints wasn't a deliberate "this endpoint should error" choice — it's historical inertia from #2465 that subsequent PRs have been chipping away one endpoint at a time. #3530 finishes the same job; nothing about the original watermark intent is being reversed where it still serves users (see out of scope below).

Out of scope

State queries (eth_getBalance/Code/StorageAt/Proof), simulation paths, filter internals, and info.go/association.go helpers keep using blockByNumberRespectingWatermarks directly. These are where the original #2465 design still applies:

  • The spec is fuzzier (some clients expect error for invalid blocks).
  • The "data not ready" semantic is more meaningful — a wallet showing balance: 0 for a not‑yet‑ready block would mislead users.
  • Internal call sites validate heights and rely on the error path.

Tests covering those error paths (TestStateAPIGetProofUnavailableHeight, the filter‑cache test) are intentionally unchanged.

Pruned-block path also unchanged. Explicit numeric requests for heights below the earliest watermark (height < earliest) still return "requested height N has been pruned". The helpers only convert ErrBlockHeightNotYetAvailable (and ErrBlockNotFoundByHash for the by-hash variant). Pre-PR behavior is preserved here — pruned is a different (permanent, not transient) failure class from the watermark race this PR addresses, and changing it would touch a different set of test assertions (watermark_manager_test.go:106,135). Hash-based requests for pruned blocks already return null in production because Tendermint returns Block: nil for unknown hashes, which blockByHashWithRetry wraps as ErrBlockNotFoundByHash.

Helpers

// blockByNumberOrNullForJSONRPC: wraps blockByNumberRespectingWatermarks
// and converts ErrBlockHeightNotYetAvailable to (nil, nil) so callers can
// return JSON null per the Ethereum spec.

// blockByHashOrNullForJSONRPC: same, by-hash variant. Also catches
// ErrBlockNotFoundByHash — both are "block doesn't exist" per spec.

Internal call sites that want the error keep using the originals.

Operational impact (broader than tests)

This is a production-correctness fix. Currently a wallet polling for a freshly-mined tx, or an indexer doing eth_getBlockReceipts near the chain head, can intermittently get a "block height not yet available" error response instead of null — they then surface that as a transient failure to users. With this change, those endpoints behave per Ethereum JSON-RPC spec.

Test plan

  • go test ./evmrpc/ -count=1 passes (21s, includes the updated above-watermark tests).
  • golangci-lint run ./evmrpc/: 0 issues.
  • gofmt -s -l clean.
  • CI integration matrix passes.

🤖 Generated with Claude Code

Endpoints that take a block identifier and return JSON null for missing
blocks per Ethereum JSON-RPC spec currently surface the watermark race
("requested height N is not yet available; safe latest is N-1") as an
error to clients. Wallets, indexers, and similar tools see a transient
failure where they expected null, exactly when a tx has just been mined
and the safe-latest watermark hasn't caught up yet.

#3119 fixed eth_getBlockByNumber and #3501 fixed eth_getTransactionReceipt
the same way. This generalizes the pattern via two helpers
(blockByNumberOrNullForJSONRPC, blockByHashOrNullForJSONRPC) and applies
the spec-aligned conversion to the remaining user-facing endpoints:

  eth_getBlockReceipts
  eth_getBlockTransactionCountByNumber
  eth_getBlockTransactionCountByHash
  eth_getBlockByHash (and sei_getBlockByHashExcludeTraceFail)
  eth_getTransactionByBlockNumberAndIndex
  eth_getTransactionByBlockHashAndIndex
  eth_getTransactionByHash

State queries (GetBalance/Code/StorageAt) and simulation/filter/internal
paths keep using blockByNumberRespectingWatermarks directly — they have
different semantics (reject invalid heights, internal validation).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cursor
Copy link
Copy Markdown

cursor Bot commented Jun 1, 2026

PR Summary

Medium Risk
Changes production JSON-RPC behavior for many high-traffic endpoints near chain head; spec-aligned but clients that retried on the old error must handle null instead.

Overview
This PR generalizes the pattern from earlier fixes (eth_getBlockByNumber, eth_getTransactionReceipt) so user-facing block/tx RPC methods return JSON null instead of a watermark error when a block is above safe-latest or unknown—matching Ethereum JSON-RPC semantics for “block doesn’t exist (yet).”

It adds blockByNumberOrNullForJSONRPC and blockByHashOrNullForJSONRPC in watermark_manager.go, mapping ErrBlockHeightNotYetAvailable (and ErrBlockNotFoundByHash for by-hash) to (nil, nil) while leaving real errors (e.g. pruned height, transport failures) unchanged. Block handlers (getBlockByHash, getBlockByNumber, transaction counts, GetBlockReceipts) and transaction handlers (getTransactionByHash, by-block index lookups, receipt fetch) now use these helpers and treat a nil block as null. GetBlockReceipts is refactored to resolve hash vs number separately so latest/safe/finalized/pending still resolve via safe-latest instead of being misread as missing.

State/simulation paths still use blockByNumberRespectingWatermarks and can return “not yet available” errors. Tests and the mock Tendermint client are updated to expect null for unknown/future blocks and to cover tag resolution and helper edge cases.

Reviewed by Cursor Bugbot for commit 5d0bccb. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 1, 2026

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedJun 2, 2026, 3:28 AM

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 1, 2026

Codecov Report

❌ Patch coverage is 82.60870% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.28%. Comparing base (f8f1af1) to head (5d0bccb).

Files with missing lines Patch % Lines
evmrpc/block.go 75.00% 3 Missing and 3 partials ⚠️
evmrpc/tx.go 83.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3530      +/-   ##
==========================================
- Coverage   59.12%   58.28%   -0.84%     
==========================================
  Files        2213     2140      -73     
  Lines      182774   174431    -8343     
==========================================
- Hits       108072   101675    -6397     
+ Misses      64985    63717    -1268     
+ Partials     9717     9039     -678     
Flag Coverage Δ
sei-chain-pr 67.98% <82.60%> (?)
sei-db 70.62% <ø> (+0.21%) ⬆️
sei-db-state-db ?

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

Files with missing lines Coverage Δ
evmrpc/watermark_manager.go 77.48% <100.00%> (+3.01%) ⬆️
evmrpc/tx.go 84.45% <83.33%> (-0.34%) ⬇️
evmrpc/block.go 83.43% <75.00%> (-0.11%) ⬇️

... and 76 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.

Comment thread evmrpc/block.go
Per cursor bugbot: GetBlockReceipts(blockHash) resolves the hash via
GetBlockNumberByNrOrHash, which internally calls the original
blockByHashRespectingWatermarks (not the new null-converting helper).
An above-watermark hash returns ErrBlockHeightNotYetAvailable from
that lookup, which propagates as an RPC error before the second
blockByNumberOrNullForJSONRPC call is reached.

Treat ErrBlockHeightNotYetAvailable from GetBlockNumberByNrOrHash the
same as ErrBlockNotFoundByHash (return null per Ethereum JSON-RPC spec).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread evmrpc/block.go
wen-coding and others added 2 commits June 1, 2026 19:11
Per cursor bugbot: the by-hash helper only converted
ErrBlockHeightNotYetAvailable; ErrBlockNotFoundByHash (genuinely
unknown hash) still propagated as an RPC error from
GetBlockTransactionCountByHash and GetTransactionByBlockHashAndIndex.
Both forms are "block doesn't exist from the caller's perspective"
and the Ethereum JSON-RPC spec maps both to null.

Extend the helper to convert both, then drop the now-redundant
explicit ErrBlockNotFoundByHash check from getBlockByHash so all
by-hash endpoints share uniform spec-aligned semantics through the
helper.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per review: GetBlockReceipts had duplicated watermark handling — once via
GetBlockNumberByNrOrHash and once via blockByNumberOrNullForJSONRPC.
Push the "block doesn't exist" mapping (both unknown-hash and above-
watermark) into GetBlockNumberByNrOrHash so callers serving Ethereum
JSON-RPC endpoints can detect "null result" via a single heightPtr==nil
check instead of two errors.Is branches. Single source of truth for the
conversion; behavior is equivalent across all scenarios (verified by
the existing height-availability + receipts tests).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 06389ea. Configure here.

Comment thread evmrpc/block.go Outdated
wen-coding and others added 3 commits June 1, 2026 22:51
…tests

Per review:
- getBlockByNumber (was #3119) and getTransactionReceipt (was #3501) had
  inline `if errors.Is(err, ErrBlockHeightNotYetAvailable) { return nil, nil }`
  predating this PR's helpers. Route both through blockByNumberOrNullForJSONRPC
  so all spec-compliant conversion lives in one place. Behavior verified
  equivalent by the existing endpoint tests
  (TestGetBlockByNumber*, TestGetTransactionReceiptReturnsNullAboveWatermark).
- Add direct unit tests covering each helper's branches (above-watermark,
  in-range, sentinel-not-found-by-hash, non-watermark error propagation).
  The Block:nil → ErrBlockNotFoundByHash path was previously not exercised
  by any test (MockClient returns a plain string error instead of the
  sentinel), so this closes that gap.

`errors` import removed from block.go (no remaining callers after this
consolidation; getBlockByHash still uses errors elsewhere... actually no
it doesn't either after the prior commit, hence the removal).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The centralization in 06389ea made GetBlockNumberByNrOrHash return
(nil, nil) for unknown hashes, but the function already returned (nil,
nil) for the "latest"/"safe"/"finalized"/"pending" named tags (via
getBlockNumber) — meaning "caller should resolve to latest height".
Overloading nil broke eth_getBlockReceipts("latest"): the new
"if heightPtr == nil { return nil, nil }" guard intercepted the
tag-means-latest case and returned JSON null instead of fetching the
latest block's receipts.

Disentangle:
- Revert GetBlockNumberByNrOrHash to its pre-PR form (errors for
  unknown-hash / above-watermark).
- GetBlockReceipts inlines hash-vs-number dispatch via the helpers
  directly; numberPtr=nil flows into blockByNumberOrNullForJSONRPC
  which routes it through wm.LatestHeight as intended.

Regression test TestBlockAPILatestTagResolves exercises all four
named tags against GetBlockReceipts and GetBlockTransactionCountByNumber.
Other tag-accepting endpoints (getBlockByNumber,
getTransactionByBlockNumberAndIndex) use the identical
(getBlockNumber → helper → if block == nil) pattern and are safe by
inspection — they don't short-circuit on numberPtr==nil between the
two calls.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three review-cleanup items in this commit:

1. MockClient.BlockByHash returned (nil, errors.New("not found")) for
   the test sentinel hash 0xbbbb..., whereas real Tendermint returns
   (ResultBlock{Block: nil}, nil) which blockByHashWithRetry wraps as
   ErrBlockNotFoundByHash. Tests against this mock exercised a different
   code path from production. Mock now matches production, and
   TestGetTransactionByBlockHashAndIndexErrors flips the unknown-hash
   assertion from "expect error" to "expect null result" — same Ethereum
   JSON-RPC spec contract as the surrounding tests.

2. GetBlockReceipts dispatch tightened: single err declaration, no
   shadow-and-assign through a temporary b variable.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@wen-coding wen-coding added this pull request to the merge queue Jun 2, 2026
Merged via the queue into main with commit 1da177f Jun 2, 2026
54 checks passed
@wen-coding wen-coding deleted the wen/watermark_null_for_jsonrpc branch June 2, 2026 14:36
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