cache chain ID at startup to prevent nil panic in ChainID()#3
Open
cache chain ID at startup to prevent nil panic in ChainID()#3
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBackend.NewBackend now returns an error and RPC API creator closures panic on backend-init errors; Backend caches parsed chain ID and ChainID() returns it. A new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as "evm-gateway CLI"
participant Main as "cmd/main (runReindex)"
participant Config as "Config Loader"
participant Logger as "Logger"
participant DB as "Indexer DB"
participant Indexer as "internal/indexer"
CLI->>Main: invoke `reindex` with args (--from, --to,...)
Main->>Config: load config & apply overrides
Main->>Logger: init logger
Main->>DB: open indexer DB
Main->>Indexer: ClearBlockRange(db, logger, from, to)
Indexer->>DB: iterate heights in batches -> read `BlockMeta`
Indexer->>DB: batch delete `BlockMeta` and `BlockLogs`
Indexer->>DB: delete `BlockHash` entries for collected hashes
Indexer->>DB: iterate heights -> scan & delete `TxIndex`, `TxHash`, `Receipt`, RPC keys
Indexer->>Logger: log progress and completion
Main->>CLI: exit (success/failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/evm/rpc/apis.go (1)
68-71: Extract the repeated backend constructor block.The same
NewBackend+panic(err)sequence now lives in four namespace creators. A small helper keeps these sites aligned if the constructor arguments or startup failure policy change again.Also applies to: 120-123, 140-143, 160-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/evm/rpc/apis.go` around lines 68 - 71, Extract the repeated backend.NewBackend(...) + panic(err) sequence into a single helper (e.g., initBackend or mustNewBackend) that accepts the same arguments (logger, cfg, clientCtx, allowUnprotectedTxs, indexer, status) and returns the constructed backend or panics on error; replace the four occurrences that currently call backend.NewBackend and then panic(err) with calls to this helper (refer to backend.NewBackend and the existing panic(err) sites in the namespace creator functions) so startup behavior and constructor arguments remain centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/evm/rpc/backend/chain_info.go`:
- Around line 29-30: ChainID() currently returns a pointer to the shared
cachedChainID allowing callers to mutate the cached value; change ChainID() to
return a defensive copy by allocating a new big.Int (converted to *hexutil.Big)
copied from b.cachedChainID so callers get an independent instance; update any
callers (e.g., SetTxDefaults which assigns args.ChainID) to continue using the
returned value without risk of modifying the backend's cachedChainID.
---
Nitpick comments:
In `@internal/evm/rpc/apis.go`:
- Around line 68-71: Extract the repeated backend.NewBackend(...) + panic(err)
sequence into a single helper (e.g., initBackend or mustNewBackend) that accepts
the same arguments (logger, cfg, clientCtx, allowUnprotectedTxs, indexer,
status) and returns the constructed backend or panics on error; replace the four
occurrences that currently call backend.NewBackend and then panic(err) with
calls to this helper (refer to backend.NewBackend and the existing panic(err)
sites in the namespace creator functions) so startup behavior and constructor
arguments remain centralized and consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e4064c7-67bd-438f-9736-7b6eee2956e1
📒 Files selected for processing (3)
internal/evm/rpc/apis.gointernal/evm/rpc/backend/backend.gointernal/evm/rpc/backend/chain_info.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/evm-gateway/main.go`:
- Around line 202-205: The fmt.Fprintf call in the fs.Parse error branch is
missing the err argument for the "%v" format specifier; update the error
handling after fs.Parse(args) to pass err into fmt.Fprintf (i.e.,
fmt.Fprintf(os.Stderr, "reindex: %v\n\nUsage:\n", err)) so the actual parse
error is printed before calling fs.PrintDefaults() and exiting; locate this in
the main function where fs.Parse, fmt.Fprintf and fs.PrintDefaults are used.
In `@internal/indexer/reindex.go`:
- Around line 74-79: The code currently ignores errors from db.Get and
json.Unmarshal when loading CachedBlockMeta, which can leave stale BlockHash
mappings; update the logic in the reindex loop where db.Get(metaKey) and
json.Unmarshal are used (referencing db.Get, metaKey, CachedBlockMeta,
json.Unmarshal, and blockHashes) to explicitly handle failures: if db.Get
returns an error or empty value, log/return the error or continue with a
warning; if json.Unmarshal fails or meta.Hash is empty, remove the stale mapping
(e.g., call db.Delete(metaKey) or equivalent) and log the failure instead of
silently skipping, and only append to blockHashes when unmarshalling succeeds
and meta.Hash is valid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df4211a2-4b2d-45f9-8060-9bb8d4ec59c5
📒 Files selected for processing (3)
cmd/evm-gateway/main.gointernal/evm/rpc/types/parsed_tx.gointernal/indexer/reindex.go
…s or parseable log
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/indexer/kv_indexer.go (1)
133-150: Consider extracting duplicated fallback logic.Lines 136-138 and 148-150 perform identical operations:
txResult.GasUsed = ethMsg.GetGas() txResult.Failed = true txHash = ethMsg.Hash()This could be extracted into a small helper or consolidated, though the two branches handle different failure scenarios (non-EVM codespace vs. EVM codespace with missing parsed data). If the duplication is intentional for clarity, a brief inline comment linking the two cases would help future readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/indexer/kv_indexer.go` around lines 133 - 150, There is duplicated fallback logic setting txResult.GasUsed, txResult.Failed and txHash from ethMsg in two branches (the non-EVM codespace branch and the parsedTx==nil branch); extract that into a small helper function (e.g., setFallbackTxResult(txResult *TxResult, ethMsg *MsgEthereumTx) string or similar) and call it from both places (replace the three lines in the branch handling result.Codespace != evmtypes.ModuleName and the three lines in the parsedTx == nil branch), or consolidate by calling that helper and returning its txHash; keep existing comments about the different failure scenarios or add a short inline note linking the two cases for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/indexer/kv_indexer.go`:
- Around line 133-150: There is duplicated fallback logic setting
txResult.GasUsed, txResult.Failed and txHash from ethMsg in two branches (the
non-EVM codespace branch and the parsedTx==nil branch); extract that into a
small helper function (e.g., setFallbackTxResult(txResult *TxResult, ethMsg
*MsgEthereumTx) string or similar) and call it from both places (replace the
three lines in the branch handling result.Codespace != evmtypes.ModuleName and
the three lines in the parsedTx == nil branch), or consolidate by calling that
helper and returning its txHash; keep existing comments about the different
failure scenarios or add a short inline note linking the two cases for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d313caa-cd9a-480f-a08c-e4c0efb05596
📒 Files selected for processing (1)
internal/indexer/kv_indexer.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/indexer/kv_indexer.go`:
- Around line 142-154: The positional lookup via txs.GetTxByMsgIndex(msgIndex)
can return nil for sparse msgIndex values, and the current branch skips indexing
for successful txs; change the nil-handling so that when parsedTx == nil and
result.Code == abci.CodeTypeOK you do NOT continue/skip but instead construct
the necessary tx indexing entities from the decoded ethMsg (set txResult.GasUsed
= ethMsg.GetGas(), set txResult.Failed = false, set txHash = ethMsg.Hash(),
build a receipt/RPC tx entry as the code normally would when parsedTx exists)
and then call the existing saveTxResult / receipt save / RPC tx write paths (the
same functions used for parsedTx cases) so the successful EVM message is
indexed; keep the logger.Error for diagnostics but remove the continue in that
success path so saveTxResult and downstream writes still run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54b80e03-b2d9-4fe1-a111-a073182669fc
📒 Files selected for processing (1)
internal/indexer/kv_indexer.go
Comment on lines
142
to
+154
| parsedTx := txs.GetTxByMsgIndex(msgIndex) | ||
| if parsedTx == nil { | ||
| kv.logger.Error("msg index not found in results", "msgIndex", msgIndex) | ||
| continue | ||
| if result.Code != abci.CodeTypeOK { | ||
| // No ethereum_tx events and no parseable ABCI log — derive | ||
| // what we can directly from the decoded message so the tx | ||
| // is still recorded and findable by hash. | ||
| txResult.GasUsed = ethMsg.GetGas() | ||
| txResult.Failed = true | ||
| txHash = ethMsg.Hash() | ||
| } else { | ||
| kv.logger.Error("msg index not found in results for successful tx", "msgIndex", msgIndex) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Avoid dropping successful EVM messages on positional lookup miss.
At Line 142, GetTxByMsgIndex(msgIndex) is positional; if msgIndex is sparse relative to parsed entries, Line 153 skips indexing entirely for a successful tx (no saveTxResult, receipt, or RPC tx write).
💡 Suggested fix
- parsedTx := txs.GetTxByMsgIndex(msgIndex)
+ parsedTx := txs.GetTxByMsgIndex(msgIndex)
+ if parsedTx == nil {
+ // Fallback to semantic lookup by ParsedTx.MsgIndex when
+ // positional indexing is sparse/non-contiguous.
+ for i := range txs.Txs {
+ if txs.Txs[i].MsgIndex == msgIndex {
+ parsedTx = &txs.Txs[i]
+ break
+ }
+ }
+ }
if parsedTx == nil {
if result.Code != abci.CodeTypeOK {Based on learnings, internal/indexer/**/*.go must implement a KV indexer for EVM transaction results; silently skipping successful EVM messages breaks index completeness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/indexer/kv_indexer.go` around lines 142 - 154, The positional lookup
via txs.GetTxByMsgIndex(msgIndex) can return nil for sparse msgIndex values, and
the current branch skips indexing for successful txs; change the nil-handling so
that when parsedTx == nil and result.Code == abci.CodeTypeOK you do NOT
continue/skip but instead construct the necessary tx indexing entities from the
decoded ethMsg (set txResult.GasUsed = ethMsg.GetGas(), set txResult.Failed =
false, set txHash = ethMsg.Hash(), build a receipt/RPC tx entry as the code
normally would when parsedTx exists) and then call the existing saveTxResult /
receipt save / RPC tx write paths (the same functions used for parsedTx cases)
so the successful EVM message is indexed; keep the logger.Error for diagnostics
but remove the continue in that success path so saveTxResult and downstream
writes still run.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/indexer/kv_indexer.go (2)
313-313: Downgrade GetByTxHash diagnostic log to Debug level.This method is invoked for every
eth_getTransactionByHashRPC call. Logging at Info level will create significant noise in production. Additionally, consider logging only on error or non-empty results to reduce volume.♻️ Suggested change
func (kv *KVIndexer) GetByTxHash(hash common.Hash) (*chaintypes.TxResult, error) { bz, err := kv.db.Get(TxHashKey(hash)) - kv.logger.Info("[diag] GetByTxHash", "hash", hash.Hex(), "bytesLen", len(bz), "err", err) + kv.logger.Debug("[diag] GetByTxHash", "hash", hash.Hex(), "bytesLen", len(bz), "err", err) if err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/indexer/kv_indexer.go` at line 313, The diagnostic log in GetByTxHash currently uses kv.logger.Info and logs every call; change it to use kv.logger.Debug and only emit when there is an error or when the result is non-empty (e.g., len(bz) > 0) to reduce noise. Update the kv.logger.Info(...) call to kv.logger.Debug(...) and guard the call so it runs only if err != nil || len(bz) > 0, keeping the same fields ("hash", hash.Hex(), "bytesLen", len(bz), "err", err) for context.
139-139: Downgrade diagnostic logs to Debug level for production.The
[diag]prefix indicates these are debugging aids rather than operational logs. At Info level, these logs will fire for every EVM message in every block, creating significant noise and potential performance overhead in production.♻️ Suggested change to use Debug level
- kv.logger.Info("[diag] tx hash from ethMsg (block-gas-limit path)", "height", block.Height, "txIndex", txIndex, "hash", txHash.Hex()) + kv.logger.Debug("[diag] tx hash from ethMsg (block-gas-limit path)", "height", block.Height, "txIndex", txIndex, "hash", txHash.Hex())- kv.logger.Info("[diag] parsedTx lookup", "height", block.Height, "txIndex", txIndex, "msgIndex", msgIndex, + kv.logger.Debug("[diag] parsedTx lookup", "height", block.Height, "txIndex", txIndex, "msgIndex", msgIndex, "resultCode", result.Code, "codespace", result.Codespace, "parsedTxNil", parsedTx == nil, "parsedTxsCount", len(txs.Txs), )Apply similar changes to all other
[diag]logs (lines 157, 174, 176).If these logs are needed temporarily for debugging a specific issue, consider removing them before merging or gating them behind a debug flag.
Also applies to: 144-148, 157-157, 174-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/indexer/kv_indexer.go` at line 139, Change the diagnostic "[diag]" logs from Info to Debug: replace the kv.logger.Info(...) calls that log "[diag]" messages (e.g., the tx hash log using txHash.Hex()) with kv.logger.Debug(...) and do the same for the other "[diag]" occurrences in this file (the other kv.logger.Info calls that include "[diag]" context). Ensure you only adjust log level and keep the same message keys/arguments so diagnostics remain available at debug level.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/indexer/kv_indexer.go`:
- Line 313: The diagnostic log in GetByTxHash currently uses kv.logger.Info and
logs every call; change it to use kv.logger.Debug and only emit when there is an
error or when the result is non-empty (e.g., len(bz) > 0) to reduce noise.
Update the kv.logger.Info(...) call to kv.logger.Debug(...) and guard the call
so it runs only if err != nil || len(bz) > 0, keeping the same fields ("hash",
hash.Hex(), "bytesLen", len(bz), "err", err) for context.
- Line 139: Change the diagnostic "[diag]" logs from Info to Debug: replace the
kv.logger.Info(...) calls that log "[diag]" messages (e.g., the tx hash log
using txHash.Hex()) with kv.logger.Debug(...) and do the same for the other
"[diag]" occurrences in this file (the other kv.logger.Info calls that include
"[diag]" context). Ensure you only adjust log level and keep the same message
keys/arguments so diagnostics remain available at debug level.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 961bf409-6988-4fcc-8d24-7f4c2d8d6e02
📒 Files selected for processing (1)
internal/indexer/kv_indexer.go
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Logging