feat: reduce rpc complexity #69
Conversation
|
Warning Rate limit exceeded@randygrok has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughThis change removes the previous JSON-RPC provider, handler, and service architecture, replacing it with a modular, function-based RPC core. All RPC endpoints are now implemented as standalone functions in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPCServer
participant CoreFunc
participant EnvAdapter
Client->>RPCServer: HTTP JSON-RPC request (e.g., "block", "tx", "status")
RPCServer->>CoreFunc: Call corresponding standalone function (e.g., Block, Tx)
CoreFunc->>EnvAdapter: Use global env for state, indexer, adapter
EnvAdapter-->>CoreFunc: Return data/result
CoreFunc-->>RPCServer: Return result or error
RPCServer-->>Client: JSON-RPC response
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (3)
server/start.go (3)
119-123: 🛠️ Refactor suggestionDiscarding the returned
node.Nodeprevents graceful shutdown & observability
startNodenow returns anode.Nodeinstance as its first result, but it is immediately discarded (_).
Keeping a reference lets the process:
- expose node-level metrics/API if needed
- participate in a coordinated shutdown (
node.Stop()or similar) rather than relying only on context cancellation.- _, cleanupFn, err := startNode(ctx, svrCtx, cmtCfg, app) + rkNode, cleanupFn, err := startNode(ctx, svrCtx, cmtCfg, app) if err != nil { return err } defer cleanupFn() + + // ensure the node is stopped when the server terminates + defer func() { + if rkNode != nil { + _ = rkNode.Stop() // TODO: check returned error + } + }()
354-356: 🛠️ Refactor suggestionLocal variable shadows imported package name
mempool
Redeclaringmempool := mempool.NewCListMempool(...)hides the imported package for the rest of the block and can confuse readers or tools.- mempool := mempool.NewCListMempool(cfg.Mempool, proxyApp.Mempool(), int64(height)) - executor.SetMempool(mempool) + tmMempool := mempool.NewCListMempool(cfg.Mempool, proxyApp.Mempool(), int64(height)) + executor.SetMempool(tmMempool)
358-365:⚠️ Potential issue
cleanupFndoes not stop RPC server or Rollkit node
cleanupFnonly cancels the context. The RPC server and the running node may continue serving until their internal contexts notice the cancellation, which is not guaranteed to be immediate.
EnhancecleanupFnto callrpcServer.Stop()androllkitNode.Stop()to avoid port leakage in tests and to shut down quickly.
🧹 Nitpick comments (5)
pkg/rpc/server.go (1)
33-34: Update outdated comment about RPC providerThe comment states that
RPCServer"delegates the actual RPC method implementations to an rpcProvider", but this is no longer accurate after removing the legacy JSON-RPC provider. The RPC server now uses the global environment and routes map.- // RPCServer manages the HTTP server for RPC requests. - // It delegates the actual RPC method implementations to an rpcProvider. + // RPCServer manages the HTTP server for RPC requests. + // It serves registered RPC functions from the Routes map using the global environment.pkg/rpc/core/abci.go (1)
31-40: Remove commented-out codeThe commented-out
ABCIInfofunction adds no value since it's not used. It's better to add it back when needed rather than keeping dead code.-// ABCIInfo gets some info about the application. -// More: https://docs.cometbft.com/v0.37/rpc/#/ABCI/abci_info -// func ABCIInfo(ctx *rpctypes.Context) (*ctypes.ResultABCIInfo, error) { -// resInfo, err := env.ProxyAppQuery.InfoSync(proxy.RequestInfo) -// if err != nil { -// return nil, err -// } -// -// return &ctypes.ResultABCIInfo{Response: *resInfo}, nil -// }pkg/rpc/routes.go (1)
8-48: Clean up commented-out routesThe file contains many commented-out routes which clutter the code. Since the PR goal is to reduce complexity, consider removing them.
Options:
- Remove all commented code
- Move them to a separate reference file
- Add a clear comment at the top explaining why they're commented out
If these routes are intended to be re-added later, consider documenting them in a separate markdown file instead of keeping them as commented code.
server/start.go (2)
131-132: Remove or replace commented-out dead code
The commented statement//clientCtx = clientCtx.WithClient(rpcServer)is a left-over from the previous RPC stack and is now misleading.
Delete it or update the comment to explain the new approach so future readers are not puzzled.
256-261: Typo in named return value reduces readability
rolllkitNode(three “l”s) is easy to mis-read and grepping for it later is error-prone.-) (rolllkitNode node.Node, cleanupFn func(), err error) { +) (rollkitNode node.Node, cleanupFn func(), err error) {Remember to update all references inside the function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
pkg/rpc/client.go(0 hunks)pkg/rpc/core/abci.go(1 hunks)pkg/rpc/core/env.go(1 hunks)pkg/rpc/json/handler.go(0 hunks)pkg/rpc/json/service.go(0 hunks)pkg/rpc/json/types.go(0 hunks)pkg/rpc/json/ws.go(0 hunks)pkg/rpc/provider/abci.go(0 hunks)pkg/rpc/provider/blockchain.go(0 hunks)pkg/rpc/provider/events.go(0 hunks)pkg/rpc/provider/evidence.go(0 hunks)pkg/rpc/provider/info.go(0 hunks)pkg/rpc/provider/mempool.go(0 hunks)pkg/rpc/provider/provider.go(0 hunks)pkg/rpc/provider/tx.go(0 hunks)pkg/rpc/provider/utils.go(0 hunks)pkg/rpc/routes.go(1 hunks)pkg/rpc/server.go(3 hunks)server/start.go(9 hunks)
💤 Files with no reviewable changes (14)
- pkg/rpc/provider/evidence.go
- pkg/rpc/client.go
- pkg/rpc/json/handler.go
- pkg/rpc/provider/events.go
- pkg/rpc/provider/info.go
- pkg/rpc/json/ws.go
- pkg/rpc/provider/abci.go
- pkg/rpc/provider/tx.go
- pkg/rpc/json/types.go
- pkg/rpc/provider/provider.go
- pkg/rpc/provider/utils.go
- pkg/rpc/provider/mempool.go
- pkg/rpc/provider/blockchain.go
- pkg/rpc/json/service.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/rpc/core/env.go (1)
pkg/adapter/adapter.go (1)
Adapter(39-55)
pkg/rpc/core/abci.go (1)
pkg/adapter/adapter.go (1)
Adapter(39-55)
pkg/rpc/routes.go (1)
pkg/rpc/core/abci.go (1)
ABCIQuery(12-29)
🔇 Additional comments (2)
pkg/rpc/server.go (1)
63-65: Well-structured RPC handler initializationThe refactoring to create the HTTP handler internally is a good simplification. This approach centralizes HTTP handler management within the RPC server rather than requiring it to be passed in from outside.
pkg/rpc/routes.go (1)
44-44:❓ Verification inconclusive
Use of server.Cacheable for abci_query
The active abci_query route doesn't use the
server.Cacheable()option unlike many of the commented-out routes. Consider whether caching would be beneficial for ABCI queries.Would ABCI queries benefit from caching similar to other read operations? If so, update the route registration:
- "abci_query": server.NewRPCFunc(core.ABCIQuery, "path,data,height,prove"), + "abci_query": server.NewRPCFunc(core.ABCIQuery, "path,data,height,prove", server.Cacheable("height")),Run this to check if other similar query endpoints use caching:
🏁 Script executed:
#!/bin/bash # Look for any other similar query endpoints in the codebase # to see if they use caching for consistency # Search for NewRPCFunc usage with query-related functions rg -A 2 "NewRPCFunc\(.*query.*" --type goLength of output: 484
Reevaluate caching for abci_query in pkg/rpc/routes.go
Unlike the commented-out
validatorsroute (which usesserver.Cacheable("height")),abci_queryaccepts dynamicpathanddataparameters. Caching by height alone can return incorrect results when different queries share the same height.• Location:
pkg/rpc/routes.go:44
• Current registration:"abci_query": server.NewRPCFunc(core.ABCIQuery, "path,data,height,prove"),• If you decide caching is desirable, include all identifiers in the cache key:
- "abci_query": server.NewRPCFunc(core.ABCIQuery, "path,data,height,prove"), + "abci_query": server.NewRPCFunc( + core.ABCIQuery, + "path,data,height,prove", + server.Cacheable("path,data,height"), + ),Otherwise, it’s safer to leave caching disabled to guarantee fresh responses per unique query.
| var ( | ||
| // set by Node | ||
| env *Environment | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add synchronization for global environment access
The global env variable could cause race conditions or nil pointer dereferences if accessed before initialization or concurrently modified. While there's a comment warning about races, there are no actual synchronization mechanisms.
Consider using a sync.Once pattern to ensure initialization happens only once:
import (
cmtlog "github.com/cometbft/cometbft/libs/log"
"github.com/rollkit/go-execution-abci/pkg/adapter"
+ "sync"
)
var (
// set by Node
env *Environment
+ envOnce sync.Once
+ envMutex sync.RWMutex
)Then update the setter function:
// SetEnvironment sets up the given Environment.
// It will race if multiple Node call SetEnvironment.
func SetEnvironment(e *Environment) {
- env = e
+ envOnce.Do(func() {
+ envMutex.Lock()
+ defer envMutex.Unlock()
+ env = e
+ })
}And add a safe getter that returns an error if env is nil:
// GetEnvironment safely retrieves the environment, returning an error if not set
func GetEnvironment() (*Environment, error) {
envMutex.RLock()
defer envMutex.RUnlock()
if env == nil {
return nil, errors.New("environment not initialized")
}
return env, nil
}| // SetEnvironment sets up the given Environment. | ||
| // It will race if multiple Node call SetEnvironment. | ||
| func SetEnvironment(e *Environment) { | ||
| env = e | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safeguards against multiple environment initializations
The comment warns about races with multiple calls to SetEnvironment, but there's no enforcement mechanism to prevent this. This could lead to unpredictable behavior.
Following the pattern suggested above with sync.Once would help, or alternatively:
// SetEnvironment sets up the given Environment.
// It will race if multiple Node call SetEnvironment.
func SetEnvironment(e *Environment) {
+ if env != nil {
+ panic("Environment already set - cannot be initialized twice")
+ }
env = e
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SetEnvironment sets up the given Environment. | |
| // It will race if multiple Node call SetEnvironment. | |
| func SetEnvironment(e *Environment) { | |
| env = e | |
| } | |
| // SetEnvironment sets up the given Environment. | |
| // It will race if multiple Node call SetEnvironment. | |
| func SetEnvironment(e *Environment) { | |
| if env != nil { | |
| panic("Environment already set - cannot be initialized twice") | |
| } | |
| env = e | |
| } |
| resp, err := env.Adapter.App.Query(ctx.Context(), &abci.RequestQuery{ | ||
| Data: data, | ||
| Path: path, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Add nil check for environment before use
The function directly accesses the global env variable without checking if it's been initialized. This could lead to nil pointer panics if SetEnvironment hasn't been called.
func ABCIQuery(
ctx *rpctypes.Context,
path string,
data bytes.HexBytes,
height int64,
prove bool,
) (*ctypes.ResultABCIQuery, error) {
+ if env == nil {
+ return nil, errors.New("rpc environment not initialized")
+ }
+
resp, err := env.Adapter.App.Query(ctx.Context(), &abci.RequestQuery{
Data: data,
Path: path,
})
if err != nil {
return nil, err
}
return &ctypes.ResultABCIQuery{
Response: *resp,
}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resp, err := env.Adapter.App.Query(ctx.Context(), &abci.RequestQuery{ | |
| Data: data, | |
| Path: path, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| func ABCIQuery( | |
| ctx *rpctypes.Context, | |
| path string, | |
| data bytes.HexBytes, | |
| height int64, | |
| prove bool, | |
| ) (*ctypes.ResultABCIQuery, error) { | |
| if env == nil { | |
| return nil, errors.New("rpc environment not initialized") | |
| } | |
| resp, err := env.Adapter.App.Query(ctx.Context(), &abci.RequestQuery{ | |
| Data: data, | |
| Path: path, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return &ctypes.ResultABCIQuery{ | |
| Response: *resp, | |
| }, nil | |
| } |
| func ABCIQuery( | ||
| ctx *rpctypes.Context, | ||
| path string, | ||
| data bytes.HexBytes, | ||
| height int64, | ||
| prove bool, | ||
| ) (*ctypes.ResultABCIQuery, error) { | ||
| resp, err := env.Adapter.App.Query(ctx.Context(), &abci.RequestQuery{ | ||
| Data: data, | ||
| Path: path, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &ctypes.ResultABCIQuery{ | ||
| Response: *resp, | ||
| }, nil |
There was a problem hiding this comment.
Unused parameters and missing validation in ABCIQuery
The function declares height and prove parameters but doesn't use them in the actual query. There's also no input validation.
Two issues need to be addressed:
- Utilize the unused parameters or remove them to avoid confusion
- Add basic validation for inputs
func ABCIQuery(
ctx *rpctypes.Context,
path string,
data bytes.HexBytes,
height int64,
prove bool,
) (*ctypes.ResultABCIQuery, error) {
+ // Validate inputs
+ if height < 0 {
+ return nil, fmt.Errorf("height cannot be negative: %d", height)
+ }
+
resp, err := env.Adapter.App.Query(ctx.Context(), &abci.RequestQuery{
Data: data,
Path: path,
+ Height: height,
+ Prove: prove,
})
if err != nil {
return nil, err
}
return &ctypes.ResultABCIQuery{
Response: *resp,
}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func ABCIQuery( | |
| ctx *rpctypes.Context, | |
| path string, | |
| data bytes.HexBytes, | |
| height int64, | |
| prove bool, | |
| ) (*ctypes.ResultABCIQuery, error) { | |
| resp, err := env.Adapter.App.Query(ctx.Context(), &abci.RequestQuery{ | |
| Data: data, | |
| Path: path, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return &ctypes.ResultABCIQuery{ | |
| Response: *resp, | |
| }, nil | |
| func ABCIQuery( | |
| ctx *rpctypes.Context, | |
| path string, | |
| data bytes.HexBytes, | |
| height int64, | |
| prove bool, | |
| ) (*ctypes.ResultABCIQuery, error) { | |
| // Validate inputs | |
| if height < 0 { | |
| return nil, fmt.Errorf("height cannot be negative: %d", height) | |
| } | |
| resp, err := env.Adapter.App.Query(ctx.Context(), &abci.RequestQuery{ | |
| Data: data, | |
| Path: path, | |
| Height: height, | |
| Prove: prove, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return &ctypes.ResultABCIQuery{ | |
| Response: *resp, | |
| }, nil | |
| } |
server/start.go
Outdated
| core.SetEnvironment(&core.Environment{ | ||
| Logger: servercmtlog.CometLoggerWrapper{Logger: logger}, | ||
| Adapter: executor, | ||
| }) | ||
|
|
||
| // Pass the created handler to the RPC server constructor | ||
| rpcServer := rpc.NewRPCServer(rpcHandler, cfg.RPC, logger) | ||
| rpcServer := rpc.NewRPCServer(cfg.RPC, logger) | ||
| err = rpcServer.Start() | ||
| if err != nil { | ||
| return nil, nil, cleanupFn, fmt.Errorf("failed to start rpc server: %w", err) | ||
| return nil, cleanupFn, fmt.Errorf("failed to start rpc server: %w", err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential race: RPC server starts before executor is running
core.SetEnvironment(...) and rpcServer.Start() are invoked before executor.Start(ctx) at line 419.
If an abci_query comes in during that gap, the adapter hasn’t begun processing and may return inconsistent results or panic.
Consider either:
- Starting the executor first, or
- Having the RPC handler block until
executor.Startsignals readiness.
- rpcServer := rpc.NewRPCServer(cfg.RPC, logger)
- err = rpcServer.Start()
+ executorReady := make(chan struct{})
+ go func() {
+ <-executorReady
+ _ = rpcServer.Start()
+ }()
...
- if err != nil { ... }
...
- if err = executor.Start(ctx); err != nil {
+ close(executorReady)
+ if err = executor.Start(ctx); err != nil {Committable suggestion skipped: line range outside the PR's diff.
server/start.go
Outdated
| rpcServer := rpc.NewRPCServer(cfg.RPC, logger) | ||
| err = rpcServer.Start() | ||
| if err != nil { | ||
| return nil, nil, cleanupFn, fmt.Errorf("failed to start rpc server: %w", err) | ||
| return nil, cleanupFn, fmt.Errorf("failed to start rpc server: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure RPC server is stopped when startNode returns with error
If rpcServer.Start() succeeds but a later step fails (e.g. node run, executor start), the function returns an error yet the RPC listener keeps the port bound.
Capture rpcServer in the earlier cleanupFn to guarantee it is closed in all exit paths.
cleanupFn = func() {
cancelFn()
+ if rpcServer != nil {
+ _ = rpcServer.Stop() // ignore secondary error
+ }
}![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| func Genesis(ctx *rpctypes.Context) (*coretypes.ResultGenesis, error) { | ||
| // Returning unimplemented as per the original code. | ||
| // Consider implementing or returning a more specific error if needed. | ||
| panic("unimplemented") | ||
| } |
There was a problem hiding this comment.
The Genesis function currently uses panic("unimplemented") which would crash the application if called. This violates the principle of graceful error handling. Consider returning a proper error value instead:
func Genesis(ctx *rpctypes.Context) (*coretypes.ResultGenesis, error) {
return nil, errors.New("Genesis RPC method is not implemented")
}This approach allows callers to handle the error appropriately without crashing the application.
| func Genesis(ctx *rpctypes.Context) (*coretypes.ResultGenesis, error) { | |
| // Returning unimplemented as per the original code. | |
| // Consider implementing or returning a more specific error if needed. | |
| panic("unimplemented") | |
| } | |
| func Genesis(ctx *rpctypes.Context) (*coretypes.ResultGenesis, error) { | |
| return nil, errors.New("Genesis RPC method is not implemented") | |
| } | |
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (4)
pkg/rpc/core/events.go (1)
12-37: 💡 Verification agent🧩 Analysis chain
Subscription entry‐point is a stub: callers will get a permanent 501-like error
Subscribeimmediately returnsevent subscription functionality is not yet implemented, which means every consumer of the"subscribe"RPC route will always fail.
That is fine while the route is flagged experimental, but once the route is registered inRoutesthe node will expose an RPC that can never succeed – effectively breaking client libraries that expect CometBFT compatibility.Recommended minimum guard-rail:
-func Subscribe(ctx *rpctypes.Context, query string) (*ctypes.ResultSubscribe, error) { +// Subscribe is currently not implemented. We deliberately *do not* register +// it in Routes until the implementation is ready to avoid breaking clients. +func Subscribe(ctx *rpctypes.Context, query string) (*ctypes.ResultSubscribe, error) {and make sure
routes.goomits the registration (or feature-gate it behind a build tag / config flag).If you do intend to ship the route, please implement at least the happy-path (parse query, call
EventBus.Subscribe, return a validResultSubscribe) before merging.
🏁 Script executed:
#!/bin/bash set -e echo "1) Locate all routes.go files in pkg/rpc:" find pkg/rpc -type f -name "routes.go" echo echo "2) Search for 'subscribe' registration in pkg/rpc/core/routes.go:" rg -n -C2 "subscribe" pkg/rpc/core/routes.go || echo "No 'subscribe' entry found in routes.go"Length of output: 688
Prevent exposing unimplemented Subscribe RPC route
The
Subscribehandler inpkg/rpc/core/events.goalways returns an error and is nevertheless registered inpkg/rpc/core/routes.go– any client calling the"subscribe"RPC will permanently fail. You must either gate this route or implement the happy-path before exposing it.Locations to address:
- pkg/rpc/core/events.go: stubbed
Subscribereturnserrors.New("…not yet implemented")- pkg/rpc/core/routes.go: line 9 registers the stub via
server.NewWSRPCFunc(Subscribe, "query")Suggested fix (remove registration until implementation is ready):
--- a/pkg/rpc/core/routes.go @@ -6,7 +6,7 @@ var Routes = map[string]*server.RPCFunc{ - "subscribe": server.NewWSRPCFunc(Subscribe, "query"), + // "subscribe": server.NewWSRPCFunc(Subscribe, "query") // unimplemented "unsubscribe": server.NewWSRPCFunc(Unsubscribe, "query"), "unsubscribe_all": server.NewWSRPCFunc(UnsubscribeAll, ""),Alternatively, feature-gate the registration behind a config flag or build tag, or implement the subscription logic (parse query, call
EventBus.Subscribe, stream events).pkg/rpc/core/tx.go (1)
134-151: 🛠️ Refactor suggestionAPI result construction bypasses hash validation
You recompute the hash with
types.Tx(result.Tx).Hash()for search results, but in the simpleTxpath you trusthashfrom the caller. An invalid hash that collides with an existing tx length could leak inconsistent data.Recommendation: in
Tx, recompute the hash too and compare to the suppliedhash. If they mismatch, returnErrTxHashMismatch.pkg/rpc/core/mempool.go (1)
27-61: 🛠️ Refactor suggestion
BroadcastTxAsyncblocks untilCheckTxfinishes – not truly asyncThe CometBFT spec states that
broadcast_tx_asyncshould return immediately after the tx is enqueued, only providing the hash. Waiting for theCheckTxcallback turns it into “sync” behaviour and adds head-of-line blocking under load.Minimum change:
- select { - case res = <-responseCh: - … - case <-unwrappedCtx.Done(): - … - } - - return &coretypes.ResultBroadcastTx{ … }, nil + return &coretypes.ResultBroadcastTx{ + Code: 0, // unknown yet + Hash: tx.Hash(), + }, nilAlternatively keep the callback for logging but do not block the RPC response.
pkg/rpc/core/blocks.go (1)
116-125: 🛠️ Refactor suggestionGuard against nil header before hashing
header, data, err := env.Adapter.Store.GetBlockData(...)could legally return anilheader (e.g. pruned block).
header.Hash()on a nil pointer causes a panic.+ if header == nil { + return nil, fmt.Errorf("block at height %d not found", heightValue) + }Replicate the same guard in
BlockByHash.
♻️ Duplicate comments (4)
pkg/rpc/core/abci.go (2)
12-18:heightandproveparameters are silently ignored
The function signature advertises that callers can specify a block height and whether a proof is required, but these values are never forwarded toabci.RequestQuery. This is confusing for callers and may hide bugs.
18-25: Add nil-check for globalenvbefore use
Same concern as raised in earlier review: dereferencingenv.Adapterwithout verifying initialisation will panic ifSetEnvironmenthasn’t been invoked.pkg/rpc/core/events.go (1)
40-63: 🛠️ Refactor suggestionSame “TODO” pattern and nil checks apply to Unsubscribe/UnsubscribeAll
Both functions replicate the concerns above: stubbed implementation, missing env nil-guards, and unused ctx parameter. Please address them together with
Subscribeto keep API behaviour consistent.pkg/rpc/core/env.go (1)
12-21: 🛠️ Refactor suggestion
⚠️ Potential issueGlobal
envis still an unsynchronised singleton – races & nil-dereference remain possibleThe past review already pointed out that concurrent reads/writes to the package-level
envmay cause data races, and that multiple initialisations are not prevented. Nothing has changed in this revision – the variable is still a plain pointer andSetEnvironmentmerely assigns it.-var ( - // set by Node - env *Environment -) +var ( + // set by Node — protected by once + RW-lock + env *Environment + envOnce sync.Once + envMu sync.RWMutex +) + +// GetEnvironment safely returns the global env or an error if not initialised. +func GetEnvironment() (*Environment, error) { + envMu.RLock() + defer envMu.RUnlock() + if env == nil { + return nil, errors.New("rpc core environment not initialised") + } + return env, nil +}…and in
SetEnvironment:-func SetEnvironment(e *Environment) { - env = e -} +func SetEnvironment(e *Environment) { + envOnce.Do(func() { + envMu.Lock() + defer envMu.Unlock() + env = e + }) +}Besides eliminating the race, callers now get a safer accessor instead of dereferencing a global that might still be
nil.
🧹 Nitpick comments (12)
pkg/rpc/core/status.go (1)
25-40:NodeInfoscaffold still contains TODOs – callers receive mostly-empty fields
All critical metadata (protocol versions, node ID, network, moniker, etc.) is commented out. Until these are populated, clients of thestatusRPC receive an almost-blanknode_info, which can break dashboards and light clients. Consider wiring the necessary data in or, at minimum, documenting the limitation in RPC docs.pkg/rpc/core/tx.go (2)
35-54:provepath always returns an error – consider short-circuiting earlierBecause proofs are not supported yet, the extra comments and half-implemented code inside the
if prove { … }block add cognitive load without providing functionality.Simpler alternative:
- var proof types.TxProof - if prove { - return nil, errors.New("transaction proof generation is not supported") - } + if prove { + return nil, errors.New("transaction proof generation is not supported") + } + var proof types.TxProof // zero valueThis removes dead code while keeping the API.
70-123: Large in-memory sort on every TxSearch call can DOS the node
env.TxIndexer.Searchmay already return results sorted; sorting again (potentially tens of thousands of entries) is O(n log n) and allocates additional memory. On a public endpoint this becomes an attack vector.Action items:
- Confirm
TxIndexer.Searchordering contract; if deterministic, drop local sort.- If local sort is required, cap
totalCountto a sane upper bound (e.g., 10 000).- Stream results instead of building a full slice when possible.
pkg/rpc/core/consensus.go (3)
33-37: Int vs int64 mismatch risks overflow on large heights
normalizeHeightreturnsint, later compared toint64:if heightPtr != nil && int64(height) != s.LastBlockHeight { … }On 32-bit builds or extremely high chains, the cast may overflow. Prefer using
int64throughout height-handling helpers to avoid truncation.
41-53: Pagination helpers silently mutate slice – copy when trimming
validators = validators[start:end]re-slices the backing array. If the original slice is reused elsewhere this may create subtle bugs. Consider copying:paged := make([]*cmttypes.Validator, end-start) copy(paged, validators[start:end]) validators = paged
81-83:ConsensusState/DumpConsensusStatejust return “not available” – expose in routes?These stubs satisfy the interface but may surprise clients. If Rollkit will never support these endpoints consider omitting them from
Routesto return “method not found” instead of a runtime error.pkg/rpc/core/mempool.go (4)
70-80: Possible goroutine leak ifenv.Adapter.Mempool.CheckTximmediately errorsWhen
CheckTxreturns an error,resChis never closed and the anonymous callback might still attempt to send, blocking forever.
Consider:err := env.Adapter.Mempool.CheckTx(tx, func(res *abci.ResponseCheckTx) { select { case resCh <- res: default: // channel already closed on error path } }, mempool.TxInfo{})…and
close(resCh)right after the error is detected.
90-107: Gossip failure returns error even though CheckTx succeeded – may surprise clientsCurrent behaviour:
CheckTxok- Gossip fails ⇒ RPC returns error
Tendermint keeps tx in the mempool and still returns success (code 0) while logging the gossip failure. Consider aligning with that to preserve liveness under temporary P2P issues.
128-143: Hard-codedmaxSubs/maxClientsplaceholdersLeaving TODO values in production code creates hidden limits. Either:
- Read them from
env.Adapter.CometCfg.RPC(there are config fields for both), or- Make them constants with explanatory names.
Otherwise clients may unexpectedly hit an arbitrary ceiling.
301-327:CheckTxwaits indefinitely – add timeout guardIf the ABCI app never calls back (bug or panic) this RPC will block forever. Enforce an upper-bound using
time.After, similar to the commit path.select { case res = <-responseCh: … case <-time.After(env.Adapter.CometCfg.RPC.TimeoutBroadcastTxCommit): return nil, errors.New("check_tx timed out") case <-unwrappedCtx.Done(): … }pkg/rpc/core/blocks.go (2)
231-253: Update parameter type after fixing the duplicate importAfter removing the un-aliased import, change the function signature to keep working:
-func HeaderByHash(ctx *rpctypes.Context, hash bytes.HexBytes) (*ctypes.ResultHeader, error) { +func HeaderByHash(ctx *rpctypes.Context, hash cmtbytes.HexBytes) (*ctypes.ResultHeader, error) {Failing to do so will re-introduce the shadowing problem.
259-299: Large in-memory slice can be capped for safety
blocks := make([]*cmttypes.BlockMeta, 0, maxHeight-minHeight+1)may allocate up to 20 items (due to the limit), which is fine, but if the limit constant is ever changed the allocation could explode.Consider using
min(maxHeight-minHeight+1, limit)in the capacity expression to future-proof memory usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
go.mod(1 hunks)pkg/rpc/core/abci.go(1 hunks)pkg/rpc/core/blocks.go(9 hunks)pkg/rpc/core/consensus.go(1 hunks)pkg/rpc/core/const.go(1 hunks)pkg/rpc/core/env.go(1 hunks)pkg/rpc/core/errors.go(1 hunks)pkg/rpc/core/events.go(3 hunks)pkg/rpc/core/evidence.go(1 hunks)pkg/rpc/core/health.go(1 hunks)pkg/rpc/core/mempool.go(10 hunks)pkg/rpc/core/net.go(1 hunks)pkg/rpc/core/routes.go(1 hunks)pkg/rpc/core/status.go(1 hunks)pkg/rpc/core/tx.go(5 hunks)pkg/rpc/core/utils.go(2 hunks)pkg/rpc/server.go(3 hunks)
✅ Files skipped from review due to trivial changes (4)
- pkg/rpc/core/health.go
- pkg/rpc/core/errors.go
- pkg/rpc/core/const.go
- pkg/rpc/core/routes.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/rpc/server.go
🧰 Additional context used
🧬 Code Graph Analysis (6)
pkg/rpc/core/abci.go (1)
pkg/adapter/adapter.go (1)
Adapter(39-55)
pkg/rpc/core/utils.go (1)
pkg/adapter/adapter.go (1)
Adapter(39-55)
pkg/rpc/core/events.go (1)
pkg/adapter/adapter.go (1)
Adapter(39-55)
pkg/rpc/core/net.go (1)
pkg/adapter/adapter.go (1)
Adapter(39-55)
pkg/rpc/core/env.go (1)
pkg/adapter/adapter.go (1)
Adapter(39-55)
pkg/rpc/core/mempool.go (4)
pkg/rpc/core/tx.go (1)
Tx(20-64)pkg/adapter/adapter.go (1)
Adapter(39-55)pkg/p2p/errors.go (1)
ErrNotReady(5-5)pkg/rpc/core/events.go (2)
Subscribe(12-38)Unsubscribe(42-54)
🔇 Additional comments (4)
go.mod (1)
104-104:✅ Verification successful
Ensure
github.com/gorilla/websocketisn’t an orphaned dependencyOur new core RPC implementation uses
nhooyr.io/websocket, sogithub.com/gorilla/websocketmay no longer be needed. Please verify there are no remaining imports and clean up your module:If it’s unused, remove the line from
go.mod; otherwise, confirm its necessity.
🏁 Script executed:
#!/bin/bash # Check for any direct imports of gorilla/websocket rg -n "github.com/gorilla/websocket" --glob='*.go' # Tidy up go.mod and go.sum go mod tidyLength of output: 18695
🏁 Script executed:
#!/bin/bash # Search for any direct imports in all .go files rg -n "github.com/gorilla/websocket" # Identify which dependencies pull in gorilla/websocket transitively go mod graph | grep "github.com/gorilla/websocket"Length of output: 1555
No action needed: retain
github.com/gorilla/websocketas an indirect dependency
rgconfirms there are no direct imports ofgithub.com/gorilla/websocketin any.gofile.go mod graphshows it’s pulled in transitively by core dependencies (e.g.cometbft/cometbft,cosmos-sdk,rollkit,go-jsonrpc,ipfs/boxo,libp2p/*, etc.).- Removing it manually would simply cause
go mod tidyto re-add it for those modules.Leave the existing line in go.mod;
go mod tidywill keep it only as an indirect dependency.pkg/rpc/core/env.go (1)
62-74: Verify visibility ofdefaultPerPageandmaxPerPageconstantsThese identifiers are referenced but not declared in this file. Ensure they live in the same package and are exported or imported to avoid a compilation failure.
pkg/rpc/core/blocks.go (2)
31-33: Incorrect type assertion for the null indexer
*blockidxnull.BlockerIndexerdoes not exist; the null implementation is calledIndexer(orBlocker, depending on the CometBFT version).
A wrong assertion will always panic even when indexing is enabled.Please confirm the concrete type name and adjust:
- if _, ok := env.BlockIndexer.(*blockidxnull.BlockerIndexer); ok { + if _, ok := env.BlockIndexer.(*blockidxnull.Indexer); ok { // or the correct concrete type
71-88: 🛠️ Refactor suggestionMissing nil-checks may lead to panics on corrupted stores
If
env.Adapter.Store.GetBlockDatareturnsnil, nil, nilfor a height that the indexer knows about, the subsequent call toheader.Hash()insideToABCIBlockwill dereference a nil pointer and crash the node.Consider hard-stopping before conversion:
- if header == nil || data == nil { - return nil, fmt.Errorf("nil header or data for height %d from store", height) - } + if header == nil || data == nil { + // Decide whether to skip silently or to fail fast. + return nil, fmt.Errorf("corrupted store: missing header or data for height %d", height) + }Likely an incorrect or invalid review comment.
| func BroadcastEvidence(ctx *rpctypes.Context, ev cmttypes.Evidence) (*ctypes.ResultBroadcastEvidence, error) { | ||
| // Log that evidence broadcasting is not supported or is a no-op? | ||
| p.logger.Debug("BroadcastEvidence called, but evidence handling is not implemented in Rollkit RPC.") | ||
| return &coretypes.ResultBroadcastEvidence{ | ||
| Hash: evidence.Hash(), | ||
| env.Logger.Debug("BroadcastEvidence called, but evidence handling is not implemented in Rollkit RPC.") | ||
| return &ctypes.ResultBroadcastEvidence{ | ||
| Hash: ev.Hash(), | ||
| }, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against an un-initialised RPC environment
This function dereferences env.Logger without checking that env (and its Logger) has been set via SetEnvironment. A nil global will cause a panic at runtime.
+ if env == nil || env.Logger == nil {
+ return nil, errors.New("rpc environment not initialised")
+ }
+
env.Logger.Debug("BroadcastEvidence called, but evidence handling is not implemented in Rollkit RPC.")(Remember to import errors.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func BroadcastEvidence(ctx *rpctypes.Context, ev cmttypes.Evidence) (*ctypes.ResultBroadcastEvidence, error) { | |
| // Log that evidence broadcasting is not supported or is a no-op? | |
| p.logger.Debug("BroadcastEvidence called, but evidence handling is not implemented in Rollkit RPC.") | |
| return &coretypes.ResultBroadcastEvidence{ | |
| Hash: evidence.Hash(), | |
| env.Logger.Debug("BroadcastEvidence called, but evidence handling is not implemented in Rollkit RPC.") | |
| return &ctypes.ResultBroadcastEvidence{ | |
| Hash: ev.Hash(), | |
| }, nil | |
| func BroadcastEvidence(ctx *rpctypes.Context, ev cmttypes.Evidence) (*ctypes.ResultBroadcastEvidence, error) { | |
| if env == nil || env.Logger == nil { | |
| return nil, errors.New("rpc environment not initialised") | |
| } | |
| // Log that evidence broadcasting is not supported or is a no-op? | |
| env.Logger.Debug("BroadcastEvidence called, but evidence handling is not implemented in Rollkit RPC.") | |
| return &ctypes.ResultBroadcastEvidence{ | |
| Hash: ev.Hash(), | |
| }, nil | |
| } |
Unused ctx parameter will break compilation – either use it or mark it intentionally ignored
ctx is never referenced after being declared, so go vet / the compiler will error with “ctx declared but not used”. Rename the parameter to _ (or actually make use of it) to keep the public signature while satisfying the compiler.
-func BroadcastEvidence(ctx *rpctypes.Context, ev cmttypes.Evidence) (*ctypes.ResultBroadcastEvidence, error) {
+func BroadcastEvidence(_ *rpctypes.Context, ev cmttypes.Evidence) (*ctypes.ResultBroadcastEvidence, error) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func BroadcastEvidence(ctx *rpctypes.Context, ev cmttypes.Evidence) (*ctypes.ResultBroadcastEvidence, error) { | |
| // Log that evidence broadcasting is not supported or is a no-op? | |
| p.logger.Debug("BroadcastEvidence called, but evidence handling is not implemented in Rollkit RPC.") | |
| return &coretypes.ResultBroadcastEvidence{ | |
| Hash: evidence.Hash(), | |
| env.Logger.Debug("BroadcastEvidence called, but evidence handling is not implemented in Rollkit RPC.") | |
| return &ctypes.ResultBroadcastEvidence{ | |
| Hash: ev.Hash(), | |
| }, nil | |
| func BroadcastEvidence(_ *rpctypes.Context, ev cmttypes.Evidence) (*ctypes.ResultBroadcastEvidence, error) { | |
| // Log that evidence broadcasting is not supported or is a no-op? | |
| env.Logger.Debug("BroadcastEvidence called, but evidence handling is not implemented in Rollkit RPC.") | |
| return &ctypes.ResultBroadcastEvidence{ | |
| Hash: ev.Hash(), | |
| }, nil | |
| } |
pkg/rpc/core/status.go
Outdated
| info, err := env.Adapter.App.Info(&abci.RequestInfo{}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| s, err := env.Adapter.LoadState(ctx.Context()) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing nil-check for global env can panic
env.Adapter.App.Info and env.Adapter.LoadState are both called unconditionally. If env is nil, or Adapter is nil, the node will crash.
+ if env == nil || env.Adapter == nil {
+ return nil, errors.New("rpc environment not initialised")
+ }(Remember to add errors to the import list.)
| if env.Adapter.EventBus == nil { | ||
| return nil, errors.New("event bus is not configured, cannot subscribe to events") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential nil-pointer dereference on global env
env.Adapter is accessed without a nil check (env.Adapter.EventBus).
If SetEnvironment is accidentally skipped (unit-tests, CLI tools, etc.) the process will panic on every RPC call.
- if env.Adapter.EventBus == nil {
+ if env == nil || env.Adapter == nil || env.Adapter.EventBus == nil {
return nil, errors.New("event bus is not configured, cannot subscribe to events")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if env.Adapter.EventBus == nil { | |
| return nil, errors.New("event bus is not configured, cannot subscribe to events") | |
| } | |
| if env == nil || env.Adapter == nil || env.Adapter.EventBus == nil { | |
| return nil, errors.New("event bus is not configured, cannot subscribe to events") | |
| } |
| func Tx(ctx *rpctypes.Context, hash []byte, prove bool) (*ctypes.ResultTx, error) { | ||
| // if index is disabled, return error | ||
| if _, ok := env.TxIndexer.(*null.TxIndex); ok { | ||
| return nil, fmt.Errorf("transaction indexing is disabled") | ||
| } | ||
|
|
||
| txResult, err := p.txIndexer.Get(hash) | ||
| r, err := env.TxIndexer.Get(hash) | ||
| if err != nil { | ||
| // If the tx is not found, return nil result without error, maintaining behaviour. | ||
| // This differs from some Tendermint versions that might return an error. | ||
| // TODO: Consider aligning error handling with target Tendermint version if necessary. | ||
| // A more robust check might involve type asserting the error if txIndexer returns a specific "not found" error type. | ||
| if txResult == nil { // Heuristic check if error implies not found | ||
| return nil, nil | ||
| } | ||
| // Return other errors encountered during Get | ||
| return nil, fmt.Errorf("error getting tx from indexer: %w", err) | ||
| return nil, err | ||
| } | ||
|
|
||
| if txResult == nil { | ||
| // Tx not found | ||
| return nil, nil | ||
| if r == nil { | ||
| return nil, fmt.Errorf("tx (%X) not found", hash) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing nil checks & ambiguous error when TxIndexer is not wired
You guard the disabled case (null.TxIndex) but not the absence of an indexer:
if env.TxIndexer == nil {
return nil, errors.New("tx indexer is not configured")
}Without this, env.TxIndexer.Get will panic.
Also, a zero-length hash slice should be rejected early to avoid a linear scan returning “not found”.
| func NetInfo(ctx *rpctypes.Context) (*coretypes.ResultNetInfo, error) { | ||
| res := coretypes.ResultNetInfo{ | ||
| Listening: true, // Assuming node is always listening if RPC is up | ||
| Listeners: []string{}, // TODO: Populate with actual listener addresses if available | ||
| } | ||
|
|
||
| // Access P2P client from adapter to get peer info | ||
| if env.Adapter.P2PClient != nil { | ||
| for _, ma := range env.Adapter.P2PClient.Addrs() { | ||
| res.Listeners = append(res.Listeners, ma.String()) | ||
| } | ||
| peers := env.Adapter.P2PClient.Peers() | ||
| res.NPeers = len(peers) | ||
| for _, peer := range peers { | ||
| // Convert peer info to coretypes.Peer | ||
| // Ensure p2p.DefaultNodeInfo is correctly populated from peer.NodeInfo | ||
| res.Peers = append(res.Peers, coretypes.Peer{ | ||
| NodeInfo: p2p.DefaultNodeInfo{ // Adapt this based on actual available PeerInfo structure | ||
| // Access fields via peer.NodeInfo | ||
| DefaultNodeID: p2p.ID(peer.NodeInfo.NodeID), | ||
| ListenAddr: peer.NodeInfo.ListenAddr, | ||
| Network: peer.NodeInfo.Network, | ||
| // Other fields like Moniker, Version might be available too | ||
| }, | ||
| IsOutbound: peer.IsOutbound, | ||
| RemoteIP: peer.RemoteIP, | ||
| }) | ||
| } | ||
| } else { | ||
| // Handle case where P2P client is not available or initialized | ||
| res.NPeers = 0 | ||
| res.Peers = []coretypes.Peer{} | ||
| } | ||
|
|
||
| return &res, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
NetInfo exposes partially-filled peer objects and risks panics
Issues observed:
peer.NodeInfois dereferenced without checkingpeer.NodeInfo != nil.p2p.DefaultNodeInfois not the concrete type returned by many libp2p peers and may not compile depending on imports.Listeningis hard-codedtrue; if the node is in light-client mode it will be wrong.
Quick defensive patch:
- res := coretypes.ResultNetInfo{
- Listening: true,
- Listeners: []string{},
- }
+ res := coretypes.ResultNetInfo{
+ Listening: env.Adapter != nil && env.Adapter.P2PClient != nil,
+ Listeners: []string{},
+ }and inside the loop:
if peer.NodeInfo != nil {
res.Peers = append(res.Peers, coretypes.Peer{ /* … */ })
}Ideally create a helper to convert your concrete peer type into the CometBFT RPC shape.
| func validatePage(pagePtr *int, perPage, totalCount int) (int, error) { | ||
| if perPage < 1 { | ||
| panic(fmt.Sprintf("zero or negative perPage: %d", perPage)) | ||
| } | ||
|
|
||
| if pagePtr == nil { // no page parameter | ||
| return 1, nil | ||
| } | ||
|
|
||
| pages := ((totalCount - 1) / perPage) + 1 | ||
| if pages == 0 { | ||
| pages = 1 // one page (even if it's empty) | ||
| } | ||
| page := *pagePtr | ||
| if page <= 0 || page > pages { | ||
| return 1, fmt.Errorf("page should be within [1, %d] range, given %d", pages, page) | ||
| } | ||
|
|
||
| return page, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
validatePage panics on invalid perPage; prefer graceful error
A public RPC helper should not bring the process down with panic. Returning an error provides the caller the chance to surface a proper JSON-RPC error to the user.
- if perPage < 1 {
- panic(fmt.Sprintf("zero or negative perPage: %d", perPage))
- }
+ if perPage < 1 {
+ return 0, fmt.Errorf("per_page must be positive, got %d", perPage)
+ }Adjust the callers to handle the extra error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func validatePage(pagePtr *int, perPage, totalCount int) (int, error) { | |
| if perPage < 1 { | |
| panic(fmt.Sprintf("zero or negative perPage: %d", perPage)) | |
| } | |
| if pagePtr == nil { // no page parameter | |
| return 1, nil | |
| } | |
| pages := ((totalCount - 1) / perPage) + 1 | |
| if pages == 0 { | |
| pages = 1 // one page (even if it's empty) | |
| } | |
| page := *pagePtr | |
| if page <= 0 || page > pages { | |
| return 1, fmt.Errorf("page should be within [1, %d] range, given %d", pages, page) | |
| } | |
| return page, nil | |
| } | |
| func validatePage(pagePtr *int, perPage, totalCount int) (int, error) { | |
| if perPage < 1 { | |
| - panic(fmt.Sprintf("zero or negative perPage: %d", perPage)) | |
| + return 0, fmt.Errorf("per_page must be positive, got %d", perPage) | |
| } | |
| if pagePtr == nil { // no page parameter | |
| return 1, nil | |
| } | |
| pages := ((totalCount - 1) / perPage) + 1 | |
| if pages == 0 { | |
| pages = 1 // one page (even if it's empty) | |
| } | |
| page := *pagePtr | |
| if page <= 0 || page > pages { | |
| return 1, fmt.Errorf("page should be within [1, %d] range, given %d", pages, page) | |
| } | |
| return page, nil | |
| } |
pkg/rpc/core/consensus.go
Outdated
| func Validators(ctx *rpctypes.Context, heightPtr *int64, pagePtr, perPagePtr *int) (*coretypes.ResultValidators, error) { | ||
| // Determine the height to query validators for. | ||
| // If height is nil or latest, use the current block height. | ||
| // Otherwise, use the specified height (if state for that height is available). | ||
| // Note: Loading state for arbitrary past heights might not be supported | ||
| // depending on state pruning. The current implementation implicitly loads latest state. | ||
| height := normalizeHeight(heightPtr) | ||
|
|
||
| s, err := env.Adapter.LoadState(ctx.Context()) // Loads the *latest* state | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load current state: %w", err) | ||
| } | ||
|
|
||
| // Check if the requested height matches the loaded state height if a specific height was requested. | ||
| // If state history is not kept, this check might be necessary or always fail for past heights. | ||
| if heightPtr != nil && int64(height) != s.LastBlockHeight { | ||
| // This implies state for the requested height is not available with the current LoadState method. | ||
| // Adjust implementation if historical state access is possible and needed. | ||
| return nil, fmt.Errorf("validator set for height %d is not available, latest height is %d", height, s.LastBlockHeight) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validators currently always loads latest state – historical queries will fail
env.Adapter.LoadState fetches only the current state, so any height that differs from s.LastBlockHeight returns an error. This diverges from CometBFT’s RPC semantics where historical validator sets are available as long as they are retained.
Two options:
- Document the limitation explicitly in the RPC docs and return a canonical “height not available” error code.
- If the store keeps historical states, expose
LoadStateAt(height int64)in the adapter and use it here.
Failing silently may break existing clients expecting Tendermint-compat behaviour.
pkg/rpc/core/mempool.go
Outdated
| // UnconfirmedTxs gets unconfirmed transactions (maximum ?limit entries) | ||
| // including their number. | ||
| // More: https://docs.cometbft.com/v0.37/rpc/#/Info/unconfirmed_txs | ||
| func UnconfirmedTxs(ctx *rpctypes.Context, limitPtr *int) (*ctypes.ResultUnconfirmedTxs, error) { | ||
| txs := env.Adapter.Mempool.ReapMaxTxs(-1) // Reap all transactions | ||
|
|
||
| var res *abci.ResponseCheckTx | ||
| select { | ||
| case res = <-resCh: | ||
| // Got response | ||
| case <-ctx.Done(): | ||
| return nil, fmt.Errorf("context cancelled waiting for CheckTx: %w", ctx.Err()) | ||
| } | ||
|
|
||
| // Gossip the transaction if it passed CheckTx. | ||
| if res.Code == abci.CodeTypeOK { | ||
| if p.adapter.TxGossiper == nil { | ||
| return nil, execp2p.ErrNotReady // Cannot gossip if gossiper is not ready | ||
| } | ||
|
|
||
| err = p.adapter.TxGossiper.Publish(ctx, tx) | ||
| if err != nil { | ||
| // If gossiping fails, remove the tx from the mempool to allow resubmission. | ||
| // This matches the behaviour described in the original comments. | ||
| rmErr := p.adapter.Mempool.RemoveTxByKey(tx.Key()) | ||
| if rmErr != nil { | ||
| // Log if removal also failed, but return the gossip error primarily. | ||
| p.logger.Error("Failed to remove tx from mempool after gossip failure", "tx_key", tx.Key(), "removal_error", rmErr) | ||
| } | ||
| return nil, fmt.Errorf("failed to gossip tx: %w", err) | ||
| limit := len(txs) | ||
| if limitPtr != nil { | ||
| limit = cmtmath.MinInt(*limitPtr, limit) | ||
| if limit < 0 { | ||
| limit = 0 | ||
| } | ||
| } | ||
| paginatedTxs := txs[:limit] | ||
|
|
||
| return &coretypes.ResultBroadcastTx{ | ||
| Code: res.Code, | ||
| Data: res.Data, | ||
| Log: res.Log, | ||
| Codespace: res.Codespace, | ||
| Hash: tx.Hash(), | ||
| return &coretypes.ResultUnconfirmedTxs{ | ||
| Count: len(paginatedTxs), | ||
| Total: env.Adapter.Mempool.Size(), | ||
| TotalBytes: env.Adapter.Mempool.SizeBytes(), | ||
| Txs: paginatedTxs, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
UnconfirmedTxs slices without copying – risk of data race with mempool
ReapMaxTxs returns references into the mempool; concurrent mempool updates may mutate the slice while it’s being JSON-encoded. Make an explicit copy:
raw := env.Adapter.Mempool.ReapMaxTxs(-1)
copied := append([]types.Tx(nil), raw[:limit]...)| // Use the locally defined maxQueryLength from provider_utils.go | ||
| if len(query) > maxQueryLength { | ||
| return nil, errors.New("maximum query length exceeded") | ||
| } |
There was a problem hiding this comment.
maxQueryLength is referenced but never defined
maxQueryLength was previously located in provider_utils.go, which no longer exists.
Dereferencing an undefined identifier breaks compilation.
If you still want the guard, add a constant in a common file (e.g. env.go) and import it here:
// env.go
const MaxQueryLength = 512- if len(query) > maxQueryLength {
+ if len(query) > MaxQueryLength {
pkg/rpc/core/blocks.go
Outdated
| "github.com/cometbft/cometbft/libs/bytes" | ||
| cmtbytes "github.com/cometbft/cometbft/libs/bytes" | ||
| cmtmath "github.com/cometbft/cometbft/libs/math" | ||
| cmtquery "github.com/cometbft/cometbft/libs/pubsub/query" | ||
| coretypes "github.com/cometbft/cometbft/rpc/core/types" | ||
| ctypes "github.com/cometbft/cometbft/rpc/core/types" | ||
| rpctypes "github.com/cometbft/cometbft/rpc/jsonrpc/types" |
There was a problem hiding this comment.
Duplicate import of the same package causes a hard‐compile error
"github.com/cometbft/cometbft/libs/bytes" is imported twice – once unnamed (line 8) and once as the alias cmtbytes (line 9).
Go does not allow the same import path to appear twice, so the file will not compile.
In addition, the first import shadows the standard‐library bytes package everywhere in this file, which is highly confusing.
- "github.com/cometbft/cometbft/libs/bytes"
- cmtbytes "github.com/cometbft/cometbft/libs/bytes"
+ cmtbytes "github.com/cometbft/cometbft/libs/bytes"You will also need to:
- Update the
HeaderByHashsignature (line 231) frombytes.HexBytestocmtbytes.HexBytes. - Replace the single remaining occurrence of the unaliased
bytes.HexBytes(line 231) accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "github.com/cometbft/cometbft/libs/bytes" | |
| cmtbytes "github.com/cometbft/cometbft/libs/bytes" | |
| cmtmath "github.com/cometbft/cometbft/libs/math" | |
| cmtquery "github.com/cometbft/cometbft/libs/pubsub/query" | |
| coretypes "github.com/cometbft/cometbft/rpc/core/types" | |
| ctypes "github.com/cometbft/cometbft/rpc/core/types" | |
| rpctypes "github.com/cometbft/cometbft/rpc/jsonrpc/types" | |
| import ( | |
| cmtbytes "github.com/cometbft/cometbft/libs/bytes" | |
| cmtmath "github.com/cometbft/cometbft/libs/math" | |
| cmtquery "github.com/cometbft/cometbft/libs/pubsub/query" | |
| ctypes "github.com/cometbft/cometbft/rpc/core/types" | |
| rpctypes "github.com/cometbft/cometbft/rpc/jsonrpc/types" | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (5)
pkg/rpc/core/utils.go (2)
148-166: 🛠️ Refactor suggestion
getBlockMetashould return errors instead of logging and returning nilCurrently, the function logs errors and returns
nil, forcing callers to inspect the return value fornilevery time instead of handling specific errors.-func getBlockMeta(ctx context.Context, n uint64) *cmtypes.BlockMeta { +func getBlockMeta(ctx context.Context, n uint64) (*cmtypes.BlockMeta, error) { header, data, err := env.Adapter.RollkitStore.GetBlockData(ctx, n) if err != nil { - env.Logger.Error("Failed to get block data in getBlockMeta", "height", n, "err", err) - return nil + return nil, fmt.Errorf("get block %d: %w", n, err) } if header == nil || data == nil { - env.Logger.Error("Nil header or data returned from GetBlockData", "height", n) - return nil + return nil, fmt.Errorf("missing header/data for height %d", n) } // Assuming ToABCIBlockMeta is now in pkg/rpc/provider/provider_utils.go bmeta, err := ToABCIBlockMeta(header, data) // Removed rpc. prefix if err != nil { - env.Logger.Error("Failed to convert block to ABCI block meta", "height", n, "err", err) - return nil + return nil, fmt.Errorf("convert block meta: %w", err) } - return bmeta + return bmeta, nil }The comment on line 158 is also outdated as the function is now in this file, not in
provider_utils.go.
120-146: 🛠️ Refactor suggestionHandle errors more robustly in
normalizeHeightfunctionThe current implementation swallows errors and returns 0 when the store lookup fails, which can yield misleading results - callers might think the chain is at genesis height when there's actually an error.
-func normalizeHeight(height *int64) uint64 { +func normalizeHeight(height *int64) (uint64, error) { var heightValue uint64 if height == nil { var err error // TODO: Decide how to handle context here. Using background for now. heightValue, err = env.Adapter.RollkitStore.Height(context.Background()) if err != nil { - // TODO: Consider logging or returning error - env.Logger.Error("Failed to get current height in normalizeHeight", "err", err) - return 0 + return 0, fmt.Errorf("failed to get current height: %w", err) } } else if *height < 0 { // Handle negative heights if they have special meaning (e.g., -1 for latest) // Currently, just treat them as 0 or latest, adjust as needed. // For now, let's assume negative height means latest valid height. var err error heightValue, err = env.Adapter.RollkitStore.Height(context.Background()) if err != nil { - env.Logger.Error("Failed to get current height for negative height in normalizeHeight", "err", err) - return 0 + return 0, fmt.Errorf("failed to get height for negative input: %w", err) } } else { heightValue = uint64(*height) } - return heightValue + return heightValue, nil }All call sites will need to be updated to handle the error return value, preventing silent failures.
pkg/rpc/core/blocks.go (3)
35-38:⚠️ Potential issue
maxQueryLengthis referenced but not definedThe variable
maxQueryLengthis referenced in the comment and code but not defined in this file or imported. It was likely defined in the removedprovider_utils.gofile.Add a constant definition in a common file (like
env.goorconst.go):// In const.go or env.go const MaxQueryLength = 512 // Adjust value as appropriateThen update the reference here:
- // Use the locally defined maxQueryLength from provider_utils.go - if len(query) > maxQueryLength { + if len(query) > MaxQueryLength {
231-231:⚠️ Potential issueFix the parameter type to use the aliased HexBytes
Since you're removing the unnamed import of
github.com/cometbft/cometbft/libs/bytes, update the parameter type to use the aliasedcmtbytes.HexBytes.-func HeaderByHash(ctx *rpctypes.Context, hash bytes.HexBytes) (*ctypes.ResultHeader, error) { +func HeaderByHash(ctx *rpctypes.Context, hash cmtbytes.HexBytes) (*ctypes.ResultHeader, error) {
8-9:⚠️ Potential issueFix duplicate package import
The package
github.com/cometbft/cometbft/libs/bytesis imported twice - once unnamed (which shadows the standard librarybytespackage) and once ascmtbytes. This causes a compilation error.-import ( - "errors" - "fmt" - "sort" - - "github.com/cometbft/cometbft/libs/bytes" - cmtbytes "github.com/cometbft/cometbft/libs/bytes" +import ( + "errors" + "fmt" + "sort" + + cmtbytes "github.com/cometbft/cometbft/libs/bytes"You'll also need to update line 231 to use the alias:
-func HeaderByHash(ctx *rpctypes.Context, hash bytes.HexBytes) (*ctypes.ResultHeader, error) { +func HeaderByHash(ctx *rpctypes.Context, hash cmtbytes.HexBytes) (*ctypes.ResultHeader, error) {
🧹 Nitpick comments (3)
pkg/rpc/core/utils.go (1)
158-158: Remove outdated code commentThe comment refers to
ToABCIBlockMetabeing inpkg/rpc/provider/provider_utils.go, but this function is actually now defined in this file.- // Assuming ToABCIBlockMeta is now in pkg/rpc/provider/provider_utils.go bmeta, err := ToABCIBlockMeta(header, data) // Removed rpc. prefixpkg/rpc/core/consensus.go (1)
95-95: Remove outdated TODO commentThe comment suggests
normalizeHeightshould be moved torpcProvider, but this function is already in the core package.- // Use normalizeHeight which should be moved to rpcProvider as well normalizedHeight := normalizeHeight(heightPtr) // Changed r.normalizeHeight to p.normalizeHeightpkg/rpc/core/blocks.go (1)
246-250: Clarify behavior for block not found by hashThere's an ambiguous comment about whether to return an empty result or an error when a block is not found by hash. This should be explicitly decided.
For better API consistency, consider returning an error instead of an empty result:
if blockMeta == nil { - // Return empty result without error if block not found by hash, consistent with original behaviour? - // Or return an error? fmt.Errorf("block with hash %X not found", hash) - return &ctypes.ResultHeader{}, nil // Current behaviour matches original code + return nil, fmt.Errorf("block with hash %X not found", hash) }This would align with the behavior of the
Headerfunction which returns an error when a block is not found by height.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.mod(1 hunks)pkg/rpc/core/blocks.go(9 hunks)pkg/rpc/core/consensus.go(1 hunks)pkg/rpc/core/status.go(1 hunks)pkg/rpc/core/utils.go(2 hunks)server/start.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- pkg/rpc/core/status.go
- server/start.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/rpc/core/consensus.go (4)
pkg/adapter/adapter.go (1)
Adapter(40-57)pkg/adapter/store.go (1)
Store(22-24)pkg/rpc/core/errors.go (1)
ErrConsensusStateNotAvailable(7-7)pkg/rpc/core/blocks.go (1)
Block(104-136)
pkg/rpc/core/utils.go (1)
pkg/adapter/adapter.go (1)
Adapter(40-57)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint / golangci-lint
| height := normalizeHeight(heightPtr) | ||
|
|
||
| s, err := env.Adapter.Store.LoadState(ctx.Context()) // Loads the *latest* state | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load current state: %w", err) | ||
| } | ||
|
|
||
| // Check if the requested height matches the loaded state height if a specific height was requested. | ||
| // If state history is not kept, this check might be necessary or always fail for past heights. | ||
| if heightPtr != nil && int64(height) != s.LastBlockHeight { | ||
| // This implies state for the requested height is not available with the current LoadState method. | ||
| // Adjust implementation if historical state access is possible and needed. | ||
| return nil, fmt.Errorf("validator set for height %d is not available, latest height is %d", height, s.LastBlockHeight) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function to handle returning error from normalizeHeight
If you implement the suggested change to make normalizeHeight return an error, this function needs to be updated to handle it.
// If height is nil or latest, use the current block height.
// Otherwise, use the specified height (if state for that height is available).
// Note: Loading state for arbitrary past heights might not be supported
// depending on state pruning. The current implementation implicitly loads latest state.
- height := normalizeHeight(heightPtr)
+ height, err := normalizeHeight(heightPtr)
+ if err != nil {
+ return nil, fmt.Errorf("failed to normalize height: %w", err)
+ }
s, err := env.Adapter.Store.LoadState(ctx.Context()) // Loads the *latest* state
if err != nil {
return nil, fmt.Errorf("failed to load current state: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| height := normalizeHeight(heightPtr) | |
| s, err := env.Adapter.Store.LoadState(ctx.Context()) // Loads the *latest* state | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to load current state: %w", err) | |
| } | |
| // Check if the requested height matches the loaded state height if a specific height was requested. | |
| // If state history is not kept, this check might be necessary or always fail for past heights. | |
| if heightPtr != nil && int64(height) != s.LastBlockHeight { | |
| // This implies state for the requested height is not available with the current LoadState method. | |
| // Adjust implementation if historical state access is possible and needed. | |
| return nil, fmt.Errorf("validator set for height %d is not available, latest height is %d", height, s.LastBlockHeight) | |
| } | |
| // If height is nil or latest, use the current block height. | |
| // Otherwise, use the specified height (if state for that height is available). | |
| // Note: Loading state for arbitrary past heights might not be supported | |
| // depending on state pruning. The current implementation implicitly loads latest state. | |
| - height := normalizeHeight(heightPtr) | |
| + height, err := normalizeHeight(heightPtr) | |
| + if err != nil { | |
| + return nil, fmt.Errorf("failed to normalize height: %w", err) | |
| + } | |
| s, err := env.Adapter.Store.LoadState(ctx.Context()) // Loads the *latest* state | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to load current state: %w", err) | |
| } | |
| // Check if the requested height matches the loaded state height if a specific height was requested. | |
| // If state history is not kept, this check might be necessary or always fail for past heights. | |
| if heightPtr != nil && int64(height) != s.LastBlockHeight { | |
| // This implies state for the requested height is not available with the current LoadState method. | |
| // Adjust implementation if historical state access is possible and needed. | |
| return nil, fmt.Errorf("validator set for height %d is not available, latest height is %d", height, s.LastBlockHeight) | |
| } |
| normalizedHeight := normalizeHeight(heightPtr) // Changed r.normalizeHeight to p.normalizeHeight | ||
| return &coretypes.ResultConsensusParams{ | ||
| BlockHeight: int64(normalizedHeight), //nolint:gosec | ||
| ConsensusParams: cmttypes.ConsensusParams{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function to handle returning error from normalizeHeight
If you implement the suggested change to make normalizeHeight return an error, this function needs to be updated to handle it.
// Use normalizeHeight which should be moved to rpcProvider as well
- normalizedHeight := normalizeHeight(heightPtr) // Changed r.normalizeHeight to p.normalizeHeight
+ normalizedHeight, err := normalizeHeight(heightPtr)
+ if err != nil {
+ return nil, fmt.Errorf("failed to normalize height: %w", err)
+ }
return &coretypes.ResultConsensusParams{
BlockHeight: int64(normalizedHeight), //nolint:gosec📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| normalizedHeight := normalizeHeight(heightPtr) // Changed r.normalizeHeight to p.normalizeHeight | |
| return &coretypes.ResultConsensusParams{ | |
| BlockHeight: int64(normalizedHeight), //nolint:gosec | |
| ConsensusParams: cmttypes.ConsensusParams{ | |
| // Use normalizeHeight which should be moved to rpcProvider as well | |
| normalizedHeight, err := normalizeHeight(heightPtr) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to normalize height: %w", err) | |
| } | |
| return &coretypes.ResultConsensusParams{ | |
| BlockHeight: int64(normalizedHeight), //nolint:gosec | |
| ConsensusParams: cmttypes.ConsensusParams{ |
| func Validators(ctx *rpctypes.Context, heightPtr *int64, pagePtr, perPagePtr *int) (*coretypes.ResultValidators, error) { | ||
| // Determine the height to query validators for. | ||
| // If height is nil or latest, use the current block height. | ||
| // Otherwise, use the specified height (if state for that height is available). | ||
| // Note: Loading state for arbitrary past heights might not be supported | ||
| // depending on state pruning. The current implementation implicitly loads latest state. | ||
| height := normalizeHeight(heightPtr) | ||
|
|
||
| s, err := env.Adapter.Store.LoadState(ctx.Context()) // Loads the *latest* state | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load current state: %w", err) | ||
| } | ||
|
|
||
| // Check if the requested height matches the loaded state height if a specific height was requested. | ||
| // If state history is not kept, this check might be necessary or always fail for past heights. | ||
| if heightPtr != nil && int64(height) != s.LastBlockHeight { | ||
| // This implies state for the requested height is not available with the current LoadState method. | ||
| // Adjust implementation if historical state access is possible and needed. | ||
| return nil, fmt.Errorf("validator set for height %d is not available, latest height is %d", height, s.LastBlockHeight) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validators function only provides latest validator set
The function always loads the latest state, even when a specific historical height is requested. This diverges from CometBFT's RPC semantics where historical validator sets are available (if retained).
Consider one of these approaches:
- Document this limitation in the function comment for API clarity
- If the store keeps historical states, enhance the adapter with a
LoadStateAt(height uint64)method and use it here
🏁 Script executed:
#!/bin/bash
# Check if RollkitStore interface has support for historical state access
echo "Checking RollkitStore interface definition:"
rg -A 15 "type Store interface" "pkg/store/store.go"
echo "Checking for any existing implementations that might access historical states:"
rg -A 3 "LoadState.*height|LoadStateAt" --type goLength of output: 427
🏁 Script executed:
#!/bin/bash
set -e
echo "Searching for Store interface definitions across the repo:"
rg -R "type Store interface" -A15
echo
echo "Searching for any LoadState implementations or overloads:"
rg -R "func .*LoadState" -A3 --type go
echo
echo "Searching for Adapter definition to locate Store field:"
rg -R "type Adapter" -A10 --type goLength of output: 576
🏁 Script executed:
#!/bin/bash
set -e
echo "Searching for Store interface definitions across the repo:"
rg "type Store interface" -A15 --type go || echo "No Store interface found"
echo
echo "Searching for any LoadState implementations or overloads:"
rg "func .*LoadState" -A3 --type go || echo "No LoadState implementations found"
echo
echo "Searching for Adapter definition to locate Store field:"
rg "type Adapter" -A10 --type go || echo "No Adapter definition found"Length of output: 1360
Validators RPC only returns the latest validator set
The Validators handler in pkg/rpc/core/consensus.go always calls
s, err := env.Adapter.Store.LoadState(ctx.Context())which only loads the latest state—any non-nil heightPtr will trigger an error rather than fetching a historical validator set.
Please address this by one of the following:
-
Document the limitation if your chain does not retain past state (due to pruning). For example, update the function comment:
// Validators returns the validator set at the specified height. // Note: only the latest state is currently supported; requesting a past height // will return an error if historical state is unavailable. func Validators(…)
-
Implement historical‐state lookup if the underlying store can serve past heights:
- Add a method on your adapter, e.g.
func (s *Store) LoadStateAt(ctx context.Context, height int64) (*cmtstate.State, error) { … }
- In
Validators, branch onheightPtr != nilto callLoadStateAtwhen fetching a past height.
- Add a method on your adapter, e.g.
Locations needing attention:
- pkg/rpc/core/consensus.go (lines 18–37)
- pkg/adapter/store.go – no
LoadStateAtmethod exists today
| _ = normalizeHeight(heightPtr) // Use height to avoid unused variable error, logic depends on future implementation | ||
| return &ctypes.ResultBlockResults{}, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function to handle returning error from normalizeHeight
If you implement the suggested change to make normalizeHeight return an error, this function needs to be updated to handle it.
- _ = normalizeHeight(heightPtr) // Use height to avoid unused variable error, logic depends on future implementation
+ _, _ = normalizeHeight(heightPtr) // Use height to avoid unused variable error, logic depends on future implementationCommittable suggestion skipped: line range outside the PR's diff.
| heightValue := normalizeHeight(heightPtr) | ||
| header, data, err := env.Adapter.RollkitStore.GetBlockData(ctx.Context(), heightValue) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function to handle returning error from normalizeHeight
If you implement the suggested change to make normalizeHeight return an error, this function needs to be updated to handle it.
- heightValue := normalizeHeight(heightPtr)
+ heightValue, err := normalizeHeight(heightPtr)
+ if err != nil {
+ return nil, fmt.Errorf("failed to normalize height: %w", err)
+ }
header, data, err := env.Adapter.RollkitStore.GetBlockData(ctx.Context(), heightValue)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| heightValue := normalizeHeight(heightPtr) | |
| header, data, err := env.Adapter.RollkitStore.GetBlockData(ctx.Context(), heightValue) | |
| heightValue, err := normalizeHeight(heightPtr) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to normalize height: %w", err) | |
| } | |
| header, data, err := env.Adapter.RollkitStore.GetBlockData(ctx.Context(), heightValue) |
| height := normalizeHeight(heightPtr) | ||
| blockMeta := getBlockMeta(ctx.Context(), height) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function to handle returning error from normalizeHeight and getBlockMeta
If you implement the suggested changes to make normalizeHeight and getBlockMeta return errors, this function needs to be updated to handle them.
- height := normalizeHeight(heightPtr)
- blockMeta := getBlockMeta(ctx.Context(), height)
+ height, err := normalizeHeight(heightPtr)
+ if err != nil {
+ return nil, fmt.Errorf("failed to normalize height: %w", err)
+ }
+
+ blockMeta, err := getBlockMeta(ctx.Context(), height)
if blockMeta == nil {
return nil, fmt.Errorf("block at height %d not found", height)
}Committable suggestion skipped: line range outside the PR's diff.
| bMeta := getBlockMeta(ctx.Context(), uint64(h)) | ||
| if bMeta != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function to handle returning error from getBlockMeta
If you implement the suggested change to make getBlockMeta return an error, this function needs to be updated to handle it.
- bMeta := getBlockMeta(ctx.Context(), uint64(h))
+ bMeta, err := getBlockMeta(ctx.Context(), uint64(h))
+ if err != nil {
+ // Log the error but continue - maintain existing behavior of skipping blocks with retrieval errors
+ env.Logger.Debug("Failed to get block meta", "height", h, "err", err)
+ }
if bMeta != nil {
blocks = append(blocks, bMeta)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bMeta := getBlockMeta(ctx.Context(), uint64(h)) | |
| if bMeta != nil { | |
| bMeta, err := getBlockMeta(ctx.Context(), uint64(h)) | |
| if err != nil { | |
| // Log the error but continue - maintain existing behavior of skipping blocks with retrieval errors | |
| env.Logger.Debug("Failed to get block meta", "height", h, "err", err) | |
| } | |
| if bMeta != nil { | |
| blocks = append(blocks, bMeta) | |
| } |
| func Block(ctx *rpctypes.Context, heightPtr *int64) (*ctypes.ResultBlock, error) { | ||
| var heightValue uint64 | ||
|
|
||
| switch { | ||
| // block tag = included | ||
| case height != nil && *height == -1: | ||
| case heightPtr != nil && *heightPtr == -1: | ||
| // heightValue = p.adapter.store.GetDAIncludedHeight() | ||
| // TODO: implement | ||
| return nil, errors.New("DA included height not implemented") | ||
| default: | ||
| heightValue = p.normalizeHeight(height) | ||
| heightValue = normalizeHeight(heightPtr) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function to handle returning error from normalizeHeight
If you implement the suggested change to make normalizeHeight return an error, this function needs to be updated to handle it.
default:
- heightValue = normalizeHeight(heightPtr)
+ var err error
+ heightValue, err = normalizeHeight(heightPtr)
+ if err != nil {
+ return nil, fmt.Errorf("failed to normalize height: %w", err)
+ }
}
header, data, err := env.Adapter.RollkitStore.GetBlockData(ctx.Context(), heightValue)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func Block(ctx *rpctypes.Context, heightPtr *int64) (*ctypes.ResultBlock, error) { | |
| var heightValue uint64 | |
| switch { | |
| // block tag = included | |
| case height != nil && *height == -1: | |
| case heightPtr != nil && *heightPtr == -1: | |
| // heightValue = p.adapter.store.GetDAIncludedHeight() | |
| // TODO: implement | |
| return nil, errors.New("DA included height not implemented") | |
| default: | |
| heightValue = p.normalizeHeight(height) | |
| heightValue = normalizeHeight(heightPtr) | |
| } | |
| func Block(ctx *rpctypes.Context, heightPtr *int64) (*ctypes.ResultBlock, error) { | |
| var heightValue uint64 | |
| switch { | |
| // block tag = included | |
| case heightPtr != nil && *heightPtr == -1: | |
| // heightValue = p.adapter.store.GetDAIncludedHeight() | |
| // TODO: implement | |
| return nil, errors.New("DA included height not implemented") | |
| default: | |
| var err error | |
| heightValue, err = normalizeHeight(heightPtr) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to normalize height: %w", err) | |
| } | |
| } | |
| header, data, err := env.Adapter.RollkitStore.GetBlockData(ctx.Context(), heightValue) | |
| // ... | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
pkg/rpc/core/mempool.go (1)
52-59:⚠️ Potential issueReturn value uses undefined package name.
The function attempts to return a
coretypes.ResultBroadcastTxbut there's no import forcoretypes, only forctypes.- return &coretypes.ResultBroadcastTx{ + return &ctypes.ResultBroadcastTx{ Code: res.Code, Data: res.Data, Log: res.Log, Codespace: res.Codespace, Hash: tx.Hash(), }, nil🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 52-52:
undefined: coretypespkg/rpc/core/blocks.go (1)
187-215: 🛠️ Refactor suggestionConsider implementing full BlockResults functionality.
The
BlockResultsfunction is currently a stub that returns empty results. Consider implementing the full functionality using the original commented-out logic as a guide, as this would provide valuable block result data to clients.
♻️ Duplicate comments (5)
pkg/rpc/core/mempool.go (1)
269-286:UnconfirmedTxsslices without copying – risk of data race with mempool
ReapMaxTxsreturns references into the mempool; concurrent mempool updates may mutate the slice while it's being JSON-encoded. Make an explicit copy.- txs := env.Adapter.Mempool.ReapMaxTxs(-1) // Reap all transactions + rawTxs := env.Adapter.Mempool.ReapMaxTxs(-1) // Reap all transactions + txs := make([]types.Tx, len(rawTxs)) + copy(txs, rawTxs) limit := len(txs) if limitPtr != nil { limit = cmtmath.MinInt(*limitPtr, limit) if limit < 0 { limit = 0 } } paginatedTxs := txs[:limit]pkg/rpc/core/blocks.go (4)
165-168: Update function to handle returning error from normalizeHeight.Previously, it was suggested to make
normalizeHeightreturn an error that needs to be handled here.- heightValue := normalizeHeight(heightPtr) + heightValue, err := normalizeHeight(heightPtr) + if err != nil { + return nil, fmt.Errorf("failed to normalize height: %w", err) + } header, data, err := env.Adapter.RollkitStore.GetBlockData(ctx.Context(), heightValue)
220-225: Update function to handle returning errors from helper functions.The functions
normalizeHeightandgetBlockMetashould return errors that need to be handled here.- height := normalizeHeight(heightPtr) - blockMeta := getBlockMeta(ctx.Context(), height) + height, err := normalizeHeight(heightPtr) + if err != nil { + return nil, fmt.Errorf("failed to normalize height: %w", err) + } + + blockMeta, err := getBlockMeta(ctx.Context(), height) + if err != nil { + return nil, fmt.Errorf("failed to get block meta: %w", err) + } if blockMeta == nil { return nil, fmt.Errorf("block at height %d not found", height) }
283-286: Update function to handle returning error from getBlockMeta.The
getBlockMetafunction should return an error that needs to be handled here.- bMeta := getBlockMeta(ctx.Context(), uint64(h)) + bMeta, err := getBlockMeta(ctx.Context(), uint64(h)) + if err != nil { + // Log the error but continue - maintain existing behavior of skipping blocks with retrieval errors + env.Logger.Debug("Failed to get block meta", "height", h, "err", err) + } if bMeta != nil { blocks = append(blocks, bMeta) }
231-235:⚠️ Potential issueUndefined bytes package - fix import issue.
The HeaderByHash function signature uses
bytes.HexBytesbut there's no import for the bytes package. You're already importing the correct package ascmtbytes.- func HeaderByHash(ctx *rpctypes.Context, hash bytes.HexBytes) (*ctypes.ResultHeader, error) { + func HeaderByHash(ctx *rpctypes.Context, hash cmtbytes.HexBytes) (*ctypes.ResultHeader, error) {🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 231-231:
undefined: bytes🪛 GitHub Actions: CI and Release
[error] 231-231: undefined: bytes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/rpc/core/blocks.go(9 hunks)pkg/rpc/core/const.go(1 hunks)pkg/rpc/core/env.go(1 hunks)pkg/rpc/core/mempool.go(9 hunks)pkg/rpc/server.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/rpc/core/env.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/rpc/server.go (1)
pkg/rpc/core/routes.go (1)
Routes(7-48)
🪛 GitHub Check: lint / golangci-lint
pkg/rpc/core/blocks.go
[failure] 231-231:
undefined: bytes
pkg/rpc/core/mempool.go
[failure] 28-28:
undefined: types
[failure] 117-117:
undefined: types
[failure] 106-106:
undefined: coretypes
[failure] 64-64:
undefined: types
[failure] 302-302:
undefined: types
🪛 GitHub Actions: CI and Release
pkg/rpc/core/blocks.go
[error] 231-231: undefined: bytes
🔇 Additional comments (5)
pkg/rpc/core/const.go (1)
1-10: Well-structured constants with clear documentation.The constants define appropriate limits for query length and pagination, with good descriptive names and helpful comments.
pkg/rpc/server.go (3)
13-19: Clean import organization with appropriate dependencies.The imports are well-organized with the new core package import properly added.
27-31: Appropriate logger wrapping for CometBFT compatibility.The server now properly wraps the SDK logger with the CometBFT logger wrapper, improving compatibility with the CometBFT libraries.
65-67: Clean RPC route registration pattern.The server now creates and manages its own HTTP mux, registering the centralized RPC functions from the core package. This approach is more modular and self-contained than the previous external handler injection.
pkg/rpc/core/blocks.go (1)
34-38: Great use of the newly defined maxQueryLength constant.The code now correctly uses the maxQueryLength constant defined in
pkg/rpc/core/const.go, addressing a previous issue.
| package core | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "time" | ||
|
|
||
| abci "github.com/cometbft/cometbft/abci/types" // Check if logger is used here | ||
| abci "github.com/cometbft/cometbft/abci/types" | ||
| cmtmath "github.com/cometbft/cometbft/libs/math" | ||
| "github.com/cometbft/cometbft/mempool" | ||
| coretypes "github.com/cometbft/cometbft/rpc/core/types" | ||
| ctypes "github.com/cometbft/cometbft/rpc/core/types" | ||
| rpctypes "github.com/cometbft/cometbft/rpc/jsonrpc/types" | ||
| cmttypes "github.com/cometbft/cometbft/types" | ||
|
|
||
| execp2p "github.com/rollkit/go-execution-abci/pkg/p2p" | ||
| ) |
There was a problem hiding this comment.
Fix missing type imports.
The code references undefined types that need to be imported.
package core
import (
"context"
"errors"
"fmt"
"time"
abci "github.com/cometbft/cometbft/abci/types"
cmtmath "github.com/cometbft/cometbft/libs/math"
"github.com/cometbft/cometbft/mempool"
ctypes "github.com/cometbft/cometbft/rpc/core/types"
rpctypes "github.com/cometbft/cometbft/rpc/jsonrpc/types"
cmttypes "github.com/cometbft/cometbft/types"
execp2p "github.com/rollkit/go-execution-abci/pkg/p2p"
+ "github.com/rollkit/rollkit/types"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| package core | |
| import ( | |
| "context" | |
| "errors" | |
| "fmt" | |
| "time" | |
| abci "github.com/cometbft/cometbft/abci/types" // Check if logger is used here | |
| abci "github.com/cometbft/cometbft/abci/types" | |
| cmtmath "github.com/cometbft/cometbft/libs/math" | |
| "github.com/cometbft/cometbft/mempool" | |
| coretypes "github.com/cometbft/cometbft/rpc/core/types" | |
| ctypes "github.com/cometbft/cometbft/rpc/core/types" | |
| rpctypes "github.com/cometbft/cometbft/rpc/jsonrpc/types" | |
| cmttypes "github.com/cometbft/cometbft/types" | |
| execp2p "github.com/rollkit/go-execution-abci/pkg/p2p" | |
| ) | |
| package core | |
| import ( | |
| "context" | |
| "errors" | |
| "fmt" | |
| "time" | |
| abci "github.com/cometbft/cometbft/abci/types" | |
| cmtmath "github.com/cometbft/cometbft/libs/math" | |
| "github.com/cometbft/cometbft/mempool" | |
| ctypes "github.com/cometbft/cometbft/rpc/core/types" | |
| rpctypes "github.com/cometbft/cometbft/rpc/jsonrpc/types" | |
| cmttypes "github.com/cometbft/cometbft/types" | |
| execp2p "github.com/rollkit/go-execution-abci/pkg/p2p" | |
| "github.com/rollkit/rollkit/types" | |
| ) |
pkg/rpc/server.go
Outdated
| "cosmossdk.io/log" | ||
| cmtcfg "github.com/cometbft/cometbft/config" | ||
| "github.com/rs/cors" | ||
| cmtlog "github.com/cometbft/cometbft/libs/log" | ||
| rpcserver "github.com/cometbft/cometbft/rpc/jsonrpc/server" | ||
| servercmtlog "github.com/cosmos/cosmos-sdk/server/log" | ||
| "golang.org/x/net/netutil" | ||
| ) | ||
| "github.com/rs/cors" |
There was a problem hiding this comment.
Consider organizing imports into logical groups with blank lines between them:
- Standard library imports
- External dependencies (with CometBFT packages grouped together)
- Internal packages
This makes the import section more readable and follows common Go conventions.
| "cosmossdk.io/log" | |
| cmtcfg "github.com/cometbft/cometbft/config" | |
| "github.com/rs/cors" | |
| cmtlog "github.com/cometbft/cometbft/libs/log" | |
| rpcserver "github.com/cometbft/cometbft/rpc/jsonrpc/server" | |
| servercmtlog "github.com/cosmos/cosmos-sdk/server/log" | |
| "golang.org/x/net/netutil" | |
| ) | |
| "github.com/rs/cors" | |
| "cosmossdk.io/log" | |
| cmtcfg "github.com/cometbft/cometbft/config" | |
| cmtlog "github.com/cometbft/cometbft/libs/log" | |
| rpcserver "github.com/cometbft/cometbft/rpc/jsonrpc/server" | |
| servercmtlog "github.com/cosmos/cosmos-sdk/server/log" | |
| "github.com/rs/cors" | |
| "golang.org/x/net/netutil" |
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
Overview
Summary by CodeRabbit
New Features
Refactor
Chores