Integrate watermark into evmrpc#2465
Merged
Merged
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2465 +/- ##
==========================================
- Coverage 43.62% 43.57% -0.05%
==========================================
Files 1679 1695 +16
Lines 143747 145352 +1605
==========================================
+ Hits 62708 63338 +630
- Misses 75414 76311 +897
- Partials 5625 5703 +78
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+117
to
+121
| for h := range c.mockedBlockResults { | ||
| if h > latest { | ||
| latest = h | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning test
jewei1997
changed the title
yzang2019
reviewed
Oct 15, 2025
jewei1997
force-pushed
the
watermark-evmrpc
branch
from
b05e87a to
2da9bbe
Compare
Contributor
|
We should be able to remove sync flush receipt in this PR since we have watermark |
yzang2019
reviewed
Oct 15, 2025
| if err := k.FlushTransientReceipts(ctx); err != nil { | ||
| return err | ||
| } | ||
| deadline := time.Now().Add(2 * time.Second) |
Check warning
Code scanning / CodeQL
Calling the system time Warning
| } else if err != nil && err.Error() != "not found" { | ||
| return err | ||
| } | ||
| if time.Now().After(deadline) { |
Check warning
Code scanning / CodeQL
Calling the system time Warning
yzang2019
approved these changes
Oct 16, 2025
sei-will
approved these changes
Oct 16, 2025
codchen
approved these changes
Oct 17, 2025
codchen
left a comment
codchen
left a comment
Collaborator
There was a problem hiding this comment.
question: for the earliest state, is it possible for it to become stale after the request to pass the check but before the state is read? (i.e. state pruned after the check but before the actual read)
Contributor
Author
|
@codchen I think the situation you mentioned is possible. However, I think we mostly care about latest height for the watermark guarantees. |
4 tasks
m3diumrare
pushed a commit
to m3diumrare/sei-chain
that referenced
this pull request
Jun 9, 2026
…ei-protocol#3530) ## 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. sei-protocol#3119 fixed `eth_getBlockByNumber` and sei-protocol#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 (sei-protocol#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 | |---|---|---| | sei-protocol#3067 (Moji) | `eth_getBlockByHash` (unknown hash) | PLT‑162: "return result null for non‑existent hashes" | | sei-protocol#3119 (Moji) | `eth_getBlockByNumber` (above watermark) | "Align with expectations" (Ethereum spec) | | sei-protocol#3320 (Wen) | `eth_getTransactionByBlock*AndIndex` (out‑of‑range index) | "Per Ethereum JSON‑RPC spec, these methods must return null" | | sei-protocol#3501 | `eth_getTransactionReceipt` (above watermark) | spec | | **sei-protocol#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 sei-protocol#2465 that subsequent PRs have been chipping away one endpoint at a time. sei-protocol#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 sei-protocol#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 ```go // 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 - [x] `go test ./evmrpc/ -count=1` passes (21s, includes the updated above-watermark tests). - [x] `golangci-lint run ./evmrpc/`: 0 issues. - [x] `gofmt -s -l` clean. - [ ] CI integration matrix passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes and provide context
A common workflow that users do is to query the latest height and then query information about that height such as events/logs. However, since we have an option to do async writes for receipts, the events/logs may not be available yet for that height, causing the query to fail.
In general, we want all of our heights outputted by our RPC endpoints to be aligned with each other. We have introduced the concept of a watermark (sei-protocol/sei-db#116) across Tendermint block store, SS Store, and receipt store. Pulling this watermark across all 3 dbs should allow the evmrpc to output the minimum height available across these dbs for a latest height. This will ensure that all data for a given height outputted by our RPCs is available across all our endpoints.
This PR introduces a WatermarkManager that manages the heights of these DBs, which the evmrpc endpoints can use to get the latest heights that are ready.
Testing performed to validate your change
unit tests + manual testing