feat(rpc): add rpc pool for endpoint fallback - #102
Conversation
WalkthroughAdds multi-endpoint RPC support with a scored RPCPool (health, retries, timeouts, failover/recovery), updates RPCClient to leverage the pool and new constructors, changes RPC config fields from single string to []string across code and docs, defers RPC client creation to Initialize, and wires RPC timeout through context/CLI and README. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant RPCClient
participant RPCPool
participant EndpointA
participant EndpointB
App->>RPCClient: Invoke RPC (ctx)
RPCClient->>RPCPool: ExecuteWithFallback(fn)
RPCPool->>EndpointA: Call with timeout
alt EndpointA success
EndpointA-->>RPCPool: Success (score↑)
RPCPool-->>RPCClient: Return result
RPCClient-->>App: Result
else EndpointA fails
EndpointA-->>RPCPool: Error (score↓)
RPCPool->>EndpointB: Try next endpoint
alt EndpointB success
EndpointB-->>RPCPool: Success (score↑)
RPCPool-->>RPCClient: Return result
RPCClient-->>App: Result
else All fail & retries exhausted
RPCPool-->>RPCClient: Aggregated error
RPCClient-->>App: Error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 4
🧹 Nitpick comments (2)
node/rpcclient/client.go (2)
44-69: Consider using CreateRPCClient to reduce code duplicationThis constructor duplicates the logic from
CreateRPCClientinrpcpool.go. Consider delegating to that function to maintain DRY principles.Replace the implementation with:
func NewRPCClient(cdc codec.Codec, rpcAddrs []string) (*RPCClient, error) { - if len(rpcAddrs) == 0 { - return nil, errors.New("no RPC addresses provided") - } - - // Create logger - logger, err := zap.NewProduction() - if err != nil { - return nil, errors.Wrap(err, "failed to create logger") - } - - // Create RPC pool - pool := NewRPCPool(rpcAddrs, logger) - - // Create HTTP client with the first endpoint - client, err := clienthttp.New(pool.GetCurrentEndpoint(), "/websocket") - if err != nil { - return nil, err - } - - return &RPCClient{ - HTTP: client, - cdc: cdc, - pool: pool, - }, nil + logger, err := zap.NewProduction() + if err != nil { + return nil, errors.Wrap(err, "failed to create logger") + } + + return CreateRPCClient(cdc, rpcAddrs, logger) }
71-90: Document the expected behavior for pre-created clientsWhen using a pre-created HTTP client, ensure it's documented that the client's endpoint should match one of the provided endpoints, or that it's a mock client (with empty remote).
Add a comment to clarify the expected usage:
func NewRPCClientWithClient(cdc codec.Codec, client *clienthttp.HTTP, endpoints []string) (*RPCClient, error) { + // This constructor is primarily used for testing with mock clients. + // If using a real client, ensure its endpoint matches one of the provided endpoints. if len(endpoints) == 0 { return nil, errors.New("no RPC endpoints provided") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
README.md(1 hunks)challenger/README.md(1 hunks)challenger/types/config.go(3 hunks)executor/README.md(1 hunks)executor/batchsubmitter/batch_test.go(2 hunks)executor/batchsubmitter/handler_test.go(1 hunks)executor/types/config.go(5 hunks)node/node.go(1 hunks)node/rpcclient/client.go(4 hunks)node/rpcclient/rpcpool.go(1 hunks)node/rpcclient/rpcpool_test.go(1 hunks)node/types/config.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
executor/batchsubmitter/batch_test.go (2)
node/rpcclient/client.go (1)
NewRPCClientWithClient(71-90)client/client.go (1)
NewWithCaller(152-156)
challenger/types/config.go (2)
executor/types/config.go (1)
NodeConfig(13-20)node/types/config.go (1)
NodeConfig(17-32)
executor/batchsubmitter/handler_test.go (2)
node/rpcclient/client.go (1)
NewRPCClientWithClient(71-90)client/client.go (1)
NewWithCaller(152-156)
node/rpcclient/rpcpool.go (2)
node/rpcclient/client.go (1)
RPCClient(37-42)client/client.go (2)
New(116-122)HTTP(64-70)
node/rpcclient/client.go (3)
node/rpcclient/rpcpool.go (2)
RPCPool(27-35)NewRPCPool(38-61)client/client.go (2)
New(116-122)HTTP(64-70)types/context.go (1)
Context(11-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: golangci-lint
- GitHub Check: Run test
- GitHub Check: opinitd
- GitHub Check: Analyze (go)
🔇 Additional comments (33)
node/node.go (1)
52-55: LGTM: Enhanced error context for RPC client creation.The addition of
errors.Wrap()provides valuable context when RPC client creation fails, which will improve debugging experience. This aligns well with the new RPC pool functionality that may have more complex failure scenarios.executor/README.md (1)
29-32: LGTM: Documentation accurately reflects RPC pool configuration.The configuration examples properly demonstrate the new array-based RPC address format, showing multiple endpoints for each node type. This will help users understand how to configure fallback RPC endpoints effectively.
Also applies to: 40-43, 51-54
node/types/config.go (2)
22-22: LGTM: Core RPC configuration change to support multiple endpoints.The field type change from
stringto[]stringproperly enables multiple RPC endpoint support, which is fundamental to the RPC pool functionality.
39-41: LGTM: Validation logic correctly updated for slice type.The validation properly checks for empty slice length instead of empty string, maintaining the same validation semantics while supporting the new array-based configuration.
executor/types/config.go (3)
16-16: LGTM: Consistent RPC address field type change.The
RPCAddressfield change to[]stringis consistent with similar changes across other configuration modules, enabling multiple RPC endpoint support.
29-31: LGTM: Proper validation for slice-based RPC addresses.The validation logic correctly checks for empty slice length, maintaining proper validation semantics for the new array-based configuration format.
112-112: LGTM: Default configurations maintain backward compatibility.The default values use single-element slices, which provides backward compatibility while enabling the new multiple endpoint functionality. This approach ensures existing configurations work seamlessly.
Also applies to: 121-121, 130-130
challenger/types/config.go (3)
13-13: LGTM: Consistent RPC address configuration pattern.The
RPCAddressfield change to[]stringfollows the same pattern established in other configuration modules, ensuring consistency across the codebase for RPC pool support.
23-25: LGTM: Validation logic properly updated.The validation correctly checks for empty slice length instead of empty string, maintaining the same validation behavior while supporting multiple RPC endpoints.
69-69: LGTM: Default configurations use backward-compatible approach.The single-element slice defaults ensure existing configurations continue to work while enabling the new multiple endpoint functionality.
Also applies to: 75-75
executor/batchsubmitter/batch_test.go (2)
275-276: LGTM: Test updated correctly for new RPC client signature.The test properly accommodates the new
NewRPCClientWithClientsignature that accepts multiple RPC endpoints and returns an error. The error handling is appropriate for test code.
757-758: LGTM: Consistent test update for new RPC client signature.This follows the same correct pattern as the previous test update, maintaining consistency across the test suite.
executor/batchsubmitter/handler_test.go (1)
49-50: LGTM: Test updated consistently with new RPC client interface.The change properly handles the new constructor signature requiring multiple RPC endpoints and returning an error, maintaining consistency with other test updates in the codebase.
challenger/README.md (2)
31-34: LGTM: Clear documentation of multi-endpoint RPC configuration.The example configuration clearly demonstrates how to specify multiple RPC endpoints as an array, which aligns with the new RPC pool functionality.
39-42: LGTM: Consistent documentation format for L2 node configuration.The L2 node configuration follows the same clear pattern as the L1 node, maintaining consistency in the documentation.
README.md (1)
104-151: Excellent documentation of the new RPC pool feature.The new section provides comprehensive coverage of the multi-endpoint RPC configuration:
- Clear explanation of the reliability benefits
- Well-structured JSON configuration examples for all node types
- Proper documentation of the
RPC_TIMEOUT_SECONDSenvironment variable- Practical usage examples
This will help users effectively configure and use the new fallback functionality.
node/rpcclient/rpcpool_test.go (9)
15-22: LGTM: Simple and effective test for basic functionality.The test correctly verifies that the initial endpoint is the first one in the list.
24-40: LGTM: Comprehensive test of endpoint cycling logic.The test properly verifies the round-robin behavior including wrap-around, which is crucial for the fallback mechanism.
42-57: LGTM: Clear test of successful RPC execution.The test verifies that successful calls don't trigger fallback and maintains the current endpoint correctly.
59-77: LGTM: Well-designed fallback success test.The test effectively simulates endpoint failure and verifies that the pool correctly falls back to the next endpoint while tracking call counts and endpoint state.
79-96: LGTM: Thorough test of failure scenarios with retry logic.The test correctly verifies the retry mechanism and error message formatting. The call count calculation (2 endpoints + 2 more for 1 retry = 4 calls) demonstrates good understanding of the retry logic.
98-116: LGTM: Proper timeout handling test.The test effectively uses context timeout to verify that RPC timeouts are properly handled and propagated.
118-137: LGTM: Effective retry success test.The test properly configures retry parameters and verifies that retry logic works correctly when calls eventually succeed.
139-165: LGTM: Comprehensive context cancellation test.The test properly simulates context cancellation during RPC execution and verifies that the cancellation is properly detected and handled.
167-195: LGTM: Important logging verification test.The test ensures that RPC failures are properly logged, which is crucial for debugging and monitoring in production environments. The use of observer pattern for log capture is appropriate.
node/rpcclient/rpcpool.go (5)
1-24: LGTM!The package imports and constants are well-organized with reasonable default values.
26-35: LGTM!The
RPCPoolstruct is well-designed with appropriate fields for managing multiple RPC endpoints with thread-safe access.
63-68: LGTM!The method correctly uses read lock for thread-safe access to the current endpoint.
70-78: LGTM!The round-robin endpoint switching logic is correctly implemented with proper locking and helpful logging.
160-183: LGTM!The
CreateRPCClientfunction properly validates input, initializes the RPC pool, and creates the client with appropriate error handling.node/rpcclient/client.go (3)
19-42: LGTM!The import additions and struct modification properly support the new RPC pool functionality.
182-205: LGTM!The QueryABCI method correctly implements the fallback pattern with proper error handling.
298-417: LGTM!All RPC methods consistently implement the fallback pattern with proper error handling and variable scoping.
|
We could also add a scoring system based on things like how many timeouts occur, |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
node/rpcclient/rpcpool.go (2)
38-40: Empty endpoints validation properly implemented.The validation for empty endpoints slice is correctly implemented and will prevent runtime panics in methods like
GetCurrentEndpoint().
22-22: Consider using existing retry constants.As mentioned in past comments, there are similar constants in
types/retry.go. TheMaxRetryCount = 4constant fromtypes/retry.gois already being used (line 61), but consider ifDefaultRPCTimeoutshould also be consolidated with existing timeout constants.node/rpcclient/client.go (1)
267-284: Thread safety concern in updateHTTPClient.The
updateHTTPClientmethod modifies theq.HTTPfield without synchronization, which could cause race conditions when multiple goroutines call RPC methods concurrently.Consider adding mutex protection as suggested in previous reviews:
type RPCClient struct { *clienthttp.HTTP cdc codec.Codec pool *RPCPool + mu sync.RWMutex } func (q *RPCClient) updateHTTPClient() error { + q.mu.Lock() + defer q.mu.Unlock() // If this is a mock client (created with NewWithCaller), don't replace it if q.HTTP.Remote() == "" { return nil } // ... rest of the method }
🧹 Nitpick comments (1)
node/rpcclient/rpcpool.go (1)
97-176: Consider consolidating duplicate logging logic.The
tryAllEndpointsmethod has duplicated logging logic for different retry attempts. This could be refactored to reduce code duplication:func (p *RPCPool) logAttempt(endpoint string, retryAttempt int, isError bool, err error) { if retryAttempt == 0 { if isError { p.logger.Warn("RPC request failed, trying next endpoint", zap.String("endpoint", endpoint), zap.String("error", err.Error())) } else { p.logger.Debug("Trying RPC endpoint", zap.String("endpoint", endpoint)) } } else { if isError { p.logger.Warn("RPC request failed during retry, trying next endpoint", zap.String("endpoint", endpoint), zap.String("error", err.Error()), zap.Int("retry", retryAttempt)) } else { p.logger.Debug("Retrying RPC endpoint", zap.String("endpoint", endpoint), zap.Int("retry", retryAttempt)) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
challenger/challenger.go(3 hunks)cmd/opinitd/db.go(1 hunks)cmd/opinitd/tx.go(3 hunks)executor/batchsubmitter/batch_test.go(2 hunks)executor/batchsubmitter/handler_test.go(1 hunks)executor/executor.go(2 hunks)node/node.go(2 hunks)node/rpcclient/client.go(4 hunks)node/rpcclient/rpcpool.go(1 hunks)provider/child/child.go(3 hunks)provider/host/host.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmd/opinitd/tx.go
🚧 Files skipped from review as they are similar to previous changes (2)
- executor/batchsubmitter/batch_test.go
- executor/batchsubmitter/handler_test.go
🧰 Additional context used
🧠 Learnings (1)
cmd/opinitd/db.go (1)
Learnt from: beer-1
PR: #41
File: cmd/opinitd/db.go:11-12
Timestamp: 2024-11-11T07:52:18.272Z
Learning: When adding migration commands, consider accepting an argument to specify the migration version, enhancing flexibility for future upgrades.
🧬 Code Graph Analysis (3)
cmd/opinitd/db.go (1)
node/rpcclient/client.go (1)
NewRPCClient(44-63)
node/rpcclient/rpcpool.go (3)
types/retry.go (1)
MaxRetryCount(11-11)node/rpcclient/client.go (1)
RPCClient(37-42)client/client.go (2)
New(116-122)HTTP(64-70)
node/rpcclient/client.go (3)
node/rpcclient/rpcpool.go (2)
RPCPool(26-34)NewRPCPool(37-64)client/client.go (2)
New(116-122)HTTP(64-70)types/context.go (1)
Context(11-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: opinitd
- GitHub Check: golangci-lint
- GitHub Check: Run test
- GitHub Check: Analyze (go)
🔇 Additional comments (24)
cmd/opinitd/db.go (1)
82-82: LGTM: Logger parameter added to RPC client constructorThe addition of the logger parameter aligns with the new RPC client architecture that supports multiple endpoints with fallback and retry logic. The naming "migration-rpcclient" is appropriate for this context.
node/node.go (3)
79-85: Excellent architectural improvement with deferred RPC client initializationMoving RPC client creation to the
Initializemethod enables proper context and logger injection, which is essential for the new multi-endpoint RPC pool functionality. The lazy initialization pattern prevents unnecessary resource allocation during construction.
87-99: Good conditional broadcaster initializationThe conditional check prevents recreating the broadcaster if it already exists, while proper error wrapping provides clear context for debugging initialization failures.
238-240: Appropriate enforcement of initialization orderThe panic with a clear message ensures that
Initialize()must be called before accessing the RPC client, which is essential for the new deferred initialization pattern.executor/executor.go (2)
73-77: Good explicit query client initializationExplicitly initializing the child's query client before querying bridge info ensures the underlying RPC client is ready. The error wrapping provides clear context for debugging initialization failures.
87-91: Consistent pattern for host query client initializationThe same initialization pattern applied to the host query client maintains consistency and ensures proper setup before querying bridge configuration.
challenger/challenger.go (3)
87-91: Consistent query client initialization patternThe explicit initialization of the child's query client follows the same pattern as in the executor, ensuring consistency across components.
101-105: Proper host query client initializationThe host query client initialization maintains the established pattern with appropriate error handling and context wrapping.
194-211: Improved concurrent component startupStarting host and child components in separate goroutines enables proper shutdown coordination through the error group. The defer functions with logging provide good observability for component lifecycle management.
provider/child/child.go (2)
104-121: Well-designed deferred query client initializationThe
InitializeQueryClientmethod enables early querying before full node setup, which is essential for the new architecture. The minimal initialization with height 0 and empty keyring configs is appropriate for query-only operations. The early return if already initialized prevents redundant work.
136-152: Sophisticated dual-path initialization logicThe
Initializemethod elegantly handles both scenarios:
- Fresh initialization (lines 139-145) when query client hasn't been pre-initialized
- Re-initialization (lines 147-151) when
InitializeQueryClientwas called firstThis flexibility supports both the new deferred initialization pattern and backward compatibility. The error handling is consistent throughout both paths.
provider/host/host.go (3)
56-56: Comment clarifies deferred initialization pattern.The comment appropriately explains that
ophostQueryClientwill be initialized later, supporting the new two-phase initialization approach.
76-93: Consider thread safety for ophostQueryClient access.The new
InitializeQueryClientmethod provides lightweight initialization, which aligns well with the deferred initialization pattern. However, there's a potential race condition sinceophostQueryClientis accessed without synchronization.Consider adding mutex protection if this method might be called concurrently:
type BaseHost struct { version uint8 node *node.Node bridgeInfo ophosttypes.QueryBridgeResponse cfg nodetypes.NodeConfig ophostQueryClient ophosttypes.QueryClient + mu sync.RWMutex processedMsgs []btypes.ProcessedMsgs msgQueue map[string][]sdk.Msg }Please verify if concurrent access to
InitializeQueryClientis possible in your usage patterns.
95-116: Approve the conditional initialization logic.The updated
Initializemethod correctly handles both initialization scenarios:
- If
ophostQueryClientis nil: full initialization with query client creation- If already initialized: reinitialize node with proper config without recreating query client
The error handling and method flow are appropriate.
node/rpcclient/rpcpool.go (4)
43-43: Environment variable timeout configuration is appropriate.The environment variable
RPC_TIMEOUT_SECONDSprovides flexible timeout configuration with proper validation and fallback to default values. The warning log for invalid values is helpful for debugging.
115-116: Context leak properly fixed.The context cancellation is now correctly placed immediately after the function call, preventing the context leak that was identified in past reviews.
150-151: Context leak properly fixed in loop.Similar to the first fix, the context cancellation in the loop is now properly handled, preventing resource leaks.
189-196: Exponential backoff implementation looks correct.The exponential backoff calculation using
math.Pow(2, float64(retry))provides appropriate increasing delays between retry attempts. The implementation respects context cancellation during the backoff period.node/rpcclient/client.go (6)
40-41: RPCClient integration with RPCPool is well-designed.The addition of the
pool *RPCPoolfield enables the fallback and retry functionality. The struct design is clean and follows the established pattern.
44-63: Constructor properly validates input and initializes pool.The
NewRPCClientconstructor correctly validates the RPC addresses, creates the pool, and initializes the HTTP client with the first endpoint. Error handling is appropriate.
65-78: NewRPCClientWithClient provides flexible initialization.This constructor variant allows injecting a custom HTTP client while still benefiting from the pool's fallback logic. The validation and initialization logic is consistent with the primary constructor.
176-186: QueryABCI properly integrated with fallback logic.The method correctly uses the
ExecuteWithFallbackpattern to leverage the pool's retry and endpoint switching capabilities. The error handling preserves the original error semantics.
255-264: ExecuteWithFallback method provides clean abstraction.The method properly delegates to the pool's fallback logic while ensuring the HTTP client is updated to the current endpoint before each attempt. This design separates concerns effectively.
223-253: All RPC methods consistently use fallback pattern.The updated methods (
QueryRawCommit,QueryBlockBulk, etc.) all follow the same pattern of usingExecuteWithFallbackwith proper variable scoping and error handling. This consistency is good for maintainability.
…ypes.SleepWithRetry and consolidating duplicate logging logic in rpcpool
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
node/rpcclient/rpcpool.go (1)
19-22: Consider reusing existing retry constantsAs mentioned in past comments, there are similar constants in
types/retry.go. Consider consolidating timeout-related constants to avoid duplication.#!/bin/bash # Description: Check for existing timeout/retry constants in the codebase # Expected: Find similar constants that could be reused rg -A 2 -B 2 "DefaultMaxRetries|DefaultRPCTimeout|MaxRetryCount" --type go
🧹 Nitpick comments (1)
node/rpcclient/rpcpool.go (1)
36-63: Constructor addresses past validation concernThe constructor now includes validation for empty endpoints slice (lines 37-39) as suggested in past comments, which prevents runtime panics.
However, consider the user's suggestion about managing timeout through context when the bot is initialized rather than environment variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)challenger/README.md(1 hunks)challenger/types/config.go(5 hunks)e2e/helper.go(3 hunks)executor/README.md(2 hunks)executor/types/config.go(8 hunks)node/rpcclient/rpcpool.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- challenger/README.md
- e2e/helper.go
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- executor/README.md
- executor/types/config.go
- challenger/types/config.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run test
- GitHub Check: opinitd
- GitHub Check: golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
node/rpcclient/rpcpool.go (9)
1-17: LGTM: Clean package structure and importsThe package declaration and imports are well-organized and include all necessary dependencies for the RPC pool functionality.
24-33: Well-designed thread-safe structureThe RPCPool struct is well-designed with appropriate fields for managing multiple endpoints, thread safety via mutex, and configurable timeouts/retries.
65-70: Thread-safe endpoint retrievalThe method correctly uses read lock for thread-safe access to the current endpoint.
72-80: Round-robin endpoint switching with proper loggingThe method implements correct round-robin logic with appropriate mutex locking and informative logging.
82-94: Proper encapsulation of index operationsThese helper methods provide thread-safe access to the current index with appropriate locking mechanisms.
96-119: Comprehensive logging for debuggingThe logging methods provide detailed information for both attempts and failures, which will be valuable for debugging RPC issues.
121-170: Core fallback logic is well-implementedThe
tryAllEndpointsmethod implements proper fallback logic by trying the current endpoint first, then cycling through others. The context timeout handling is correct with immediate cancellation after each attempt, addressing the context leak issue from past comments.The method properly resets to the original position if all endpoints fail during the initial attempt (lines 165-167), which maintains consistent behavior.
172-202: Effective retry strategy with exponential backoffThe method implements a solid retry strategy that first tries all endpoints once, then retries with exponential backoff using the existing
SleepWithRetryutility. This addresses the past comment about reusing similar logic.The context cancellation handling is correct (lines 189-191), and the final error wrapping provides useful debugging information.
204-227: Robust client factory functionThe
CreateRPCClientfunction provides proper validation, error handling, and integration with the existing HTTP client infrastructure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
node/rpcclient/rpcpool.go (1)
163-193: Consider implementing endpoint scoring system as suggested in PR comments.The current implementation uses simple round-robin fallback. As mentioned in the PR objectives, implementing a scoring system based on endpoint reliability metrics (timeouts, failures) could improve performance by prioritizing more reliable endpoints.
This could include:
- Tracking success/failure rates per endpoint
- Implementing weighted endpoint selection based on scores
- Periodic score reset for endpoint recovery
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/opinitd/db.go(3 hunks)cmd/opinitd/start.go(3 hunks)cmd/opinitd/tx.go(3 hunks)executor/batchsubmitter/batch_test.go(2 hunks)executor/batchsubmitter/handler_test.go(1 hunks)node/node.go(2 hunks)node/rpcclient/client.go(6 hunks)node/rpcclient/rpcpool.go(1 hunks)node/rpcclient/rpcpool_test.go(1 hunks)types/context.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmd/opinitd/tx.go
🚧 Files skipped from review as they are similar to previous changes (4)
- node/node.go
- executor/batchsubmitter/handler_test.go
- cmd/opinitd/db.go
- executor/batchsubmitter/batch_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/opinitd/start.go (1)
types/context.go (1)
NewContext(23-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: golangci-lint
- GitHub Check: Run test
- GitHub Check: opinitd
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
types/context.go (1)
19-19: LGTM! Clean implementation following established patterns.The addition of
rpcTimeoutfield and its accessor methods follows the same pattern as the existingtxTimeoutfield, maintaining consistency in the codebase.Also applies to: 75-78, 105-107
cmd/opinitd/start.go (1)
22-22: LGTM! Proper CLI flag implementation for RPC timeout.The RPC timeout flag implementation follows the established pattern used for
polling-interval, with a reasonable default of 5 seconds and proper integration into the context chain.Also applies to: 66-69, 74-74, 85-85
node/rpcclient/rpcpool_test.go (5)
17-21: LGTM! Good helper function for consistent test setup.The
createTestContexthelper function provides a consistent way to create test contexts with appropriate RPC timeout settings.
23-48: LGTM! Comprehensive endpoint management tests.The tests for
GetCurrentEndpointandMoveToNextEndpointproperly verify the round-robin behavior and wrap-around functionality.
50-104: LGTM! Excellent coverage of fallback scenarios.The tests properly cover immediate success, fallback success, and complete failure scenarios with appropriate call count verification.
106-145: LGTM! Good timeout and retry logic testing.The tests properly verify timeout behavior and retry functionality with appropriate timing controls for fast test execution.
147-203: LGTM! Comprehensive context cancellation and logging tests.The context cancellation test properly simulates real-world scenarios, and the logging test verifies that failures are properly logged for debugging purposes.
node/rpcclient/rpcpool.go (4)
34-54: LGTM! Robust constructor with proper validation.The
NewRPCPoolconstructor properly validates empty endpoints (addressing previous feedback) and correctly initializes all fields with sensible defaults. The context-based timeout configuration is well implemented.
56-86: LGTM! Thread-safe endpoint management.The endpoint management methods properly use RWMutex for thread safety, with read locks for queries and write locks for modifications. The round-robin logic is correct.
112-161: LGTM! Correct context handling without leaks.The
tryAllEndpointsmethod properly handles context timeouts by callingcancel()immediately after each attempt, avoiding the context leak issue identified in previous reviews.
195-218: LGTM! Clean factory function with proper error handling.The
CreateRPCClientfunction provides a convenient way to create RPC clients with proper validation and error handling.node/rpcclient/client.go (5)
42-45: LGTM! Proper thread safety implementation.The addition of the
poolfield andmumutex addresses the thread safety concerns raised in previous reviews.
47-66: LGTM! Robust constructors with proper validation.Both constructor methods properly validate input parameters and handle empty endpoint slices with appropriate error returns.
Also applies to: 68-81
173-196: LGTM! Consistent fallback pattern implementation.All RPC query methods (
QueryABCI,QueryRawCommit,QueryBlockBulk) follow the same consistent pattern usingExecuteWithFallbackfor automatic endpoint failover and retry logic.Also applies to: 219-236, 239-256
258-289: LGTM! Thread-safe HTTP client updates.The
ExecuteWithFallbackandupdateHTTPClientmethods properly handle thread safety with mutex locking and include smart mock client detection to avoid replacing test clients.
291-410: LGTM! Comprehensive RPC method coverage.All RPC methods (
Status,Block,BlockResults,QueryTx,Tx,BroadcastTxSync,BroadcastTxAsync) are consistently updated to use the fallback pattern, ensuring reliable RPC operations across all functionality.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
challenger/challenger.go(4 hunks)node/rpcclient/client.go(6 hunks)node/rpcclient/rpcpool.go(1 hunks)node/rpcclient/rpcpool_test.go(1 hunks)node/rpcclient/scoring_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- node/rpcclient/rpcpool_test.go
- challenger/challenger.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
node/rpcclient/client.go (3)
node/rpcclient/rpcpool.go (3)
RPCPool(47-56)CreateRPCClient(441-468)NewRPCPool(59-118)types/context.go (1)
Context(11-21)client/client.go (2)
HTTP(64-70)New(116-122)
node/rpcclient/scoring_test.go (2)
node/rpcclient/rpcpool.go (10)
RPCPool(47-56)NewRPCPool(59-118)DefaultInitialScore(23-23)ScoreIncreaseOnSuccess(26-26)ScoreDecayOnFailure(24-24)ScoreDecayOnTimeout(25-25)MaxScore(28-28)MinScore(27-27)RPCClientInfo(33-44)ScoreResetInterval(29-29)types/context.go (2)
NewContext(23-30)Context(11-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: golangci-lint
- GitHub Check: Run test
- GitHub Check: opinitd
- GitHub Check: Analyze (go)
🔇 Additional comments (19)
node/rpcclient/scoring_test.go (9)
17-32: LGTM!The test helper function correctly creates a pool with mock endpoints and marks them as healthy for testing purposes.
34-47: LGTM!The test correctly verifies that all endpoints are initialized with default scores and zero counters.
104-131: LGTM!The test comprehensively verifies that scores are properly bounded within MinScore and MaxScore limits.
133-155: LGTM!The test correctly verifies that clients are sorted by score in descending order.
157-181: LGTM!The test correctly verifies the selection of the highest-scoring healthy client.
183-210: LGTM!The test correctly verifies that unhealthy clients are skipped when selecting the best client.
212-234: LGTM!The test correctly verifies that nil is returned when no healthy clients are available.
236-268: LGTM!The test correctly verifies that scores and counters are reset to their initial values.
270-469: LGTM!The remaining tests provide comprehensive coverage for:
- Automatic score reset based on time intervals
- Endpoint selection prioritized by score
- Handling of all endpoints failing
- Score update behavior with multiple successes and failures
- Thread safety with concurrent score updates
All tests are well-structured and properly use synchronization primitives.
node/rpcclient/client.go (4)
38-71: LGTM!The RPCClient struct properly includes a mutex for thread safety, and the constructors are well-designed to support both production use with pool fallback and testing with mocked clients.
248-270: LGTM!The ExecuteWithFallback method properly addresses thread safety concerns by using mutex locks when updating the HTTP client. The nil pool check elegantly handles test clients.
163-186: LGTM!The QueryABCI method correctly uses ExecuteWithFallback for retry and fallback logic.
208-391: LGTM!All RPC methods are consistently refactored to use ExecuteWithFallback, providing uniform retry and fallback behavior across all RPC operations. The pattern of declaring result/err variables outside the closure is correct.
node/rpcclient/rpcpool.go (6)
18-56: LGTM!The constants and type definitions are well-structured with clear documentation. The scoring system parameters provide a good balance between rewarding success and penalizing failures.
58-118: LGTM!The constructor properly validates non-empty endpoints and handles client creation failures gracefully by marking failed endpoints as unhealthy while still including them in the pool for potential recovery.
120-197: LGTM!The client management methods are well-implemented with proper synchronization using read/write locks. The recovery mechanism allows failed endpoints to be retried after a cooldown period.
199-297: LGTM!The scoring system is well-implemented with:
- Proper bounds checking (MinScore to MaxScore)
- Thread-safe score updates
- Periodic score resets to allow recovered endpoints to regain priority
- Detailed logging for debugging
341-438: LGTM!The execution methods are well-implemented:
- Properly handles context cancellation without leaks (cancel() called immediately, not deferred)
- Tries endpoints in descending score order for optimal performance
- Includes exponential backoff with jitter for retries
- Attempts to recover unhealthy clients before retrying
440-468: LGTM!The CreateRPCClient function properly validates input and ensures at least one healthy client is available before creating the RPCClient instance.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
node/rpcclient/rpcpool.go (3)
32-44: Consider adding documentation for thread safetyThe struct fields are well-designed for tracking endpoint health and performance metrics. However, it would be helpful to document which fields require synchronization since this struct is used in a concurrent environment.
// RPCClientInfo holds information about an RPC client and its health status +// Note: Access to fields should be synchronized using the RPCPool's mutex type RPCClientInfo struct {
59-62: Good validation, but consider returning error instead of panicWhile the validation prevents runtime issues, panicking in a constructor can be harsh for library code. Consider returning an error instead to allow callers to handle the situation gracefully.
-func NewRPCPool(ctx types.Context, endpoints []string, logger *zap.Logger) *RPCPool { +func NewRPCPool(ctx types.Context, endpoints []string, logger *zap.Logger) (*RPCPool, error) { if len(endpoints) == 0 { - panic("endpoints slice cannot be empty") + return nil, errors.New("endpoints slice cannot be empty") }Note: This would require updating the caller in
CreateRPCClientfunction to handle the error.
100-102: Consistent error handling approach neededThis panic is inconsistent with the suggested error-returning approach for the empty endpoints check. If you modify the constructor to return errors, this should also return an error instead of panicking.
if len(clients) == 0 { - panic("no valid endpoints found - all endpoints failed to create HTTP clients") + return nil, errors.New("no valid endpoints found - all endpoints failed to create HTTP clients") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
node/rpcclient/rpcpool.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
node/rpcclient/rpcpool.go (4)
client/client.go (2)
HTTP(64-70)New(116-122)types/context.go (1)
Context(11-21)types/retry.go (2)
MaxRetryCount(11-11)SleepWithRetry(14-32)node/rpcclient/client.go (1)
RPCClient(39-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: golangci-lint
- GitHub Check: Run test
- GitHub Check: opinitd
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
node/rpcclient/rpcpool.go (8)
18-30: LGTM! Well-defined constants with appropriate valuesThe constants are well-structured and provide sensible defaults for the scoring system. The timeout default of 5 seconds is reasonable, and the scoring system parameters are balanced to provide meaningful differentiation between endpoints.
110-110: Good reuse of existing constantUsing
types.MaxRetryCountmaintains consistency with the existing retry logic throughout the codebase, which addresses the previous comment about code duplication.
128-141: Robust circular iteration with proper fallbackThe implementation correctly handles circular iteration through endpoints and provides proper fallback when no healthy clients are available. The reset to original position on failure is a good defensive practice.
158-181: Excellent recovery mechanism with detailed documentationThe client recovery implementation addresses the requirements mentioned in the PR comments effectively. The detailed comment explains the rationale for recreating HTTP clients, and the implementation properly handles both success and failure scenarios.
197-218: Well-implemented scoring systemThe scoring mechanism provides proper bounds checking and handles both success and failure cases appropriately. The differentiation between timeout and general failures (with higher penalty for timeouts) is a good design choice.
396-436: Well-structured execution flow with proper integrationThe main execution function properly implements the fallback strategy:
- Score-based prioritization for the first attempt
- Recovery attempts for unhealthy clients
- Exponential backoff retries using the existing
SleepWithRetryfunction- Proper error propagation
This addresses the scoring system suggestion from the PR comments effectively.
438-466: Robust client creation with proper error handlingThe function properly validates inputs, creates the pool, and ensures a healthy client is available before creating the RPC client. The error handling covers edge cases well.
339-394: Timeout detection logic is consistent across the codebaseI searched for all occurrences of
context.DeadlineExceededandcontext.Canceledand only found the check innode/rpcclient/rpcpool.go. The timeout‐handling pattern is applied correctly and no further changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
node/rpcclient/rpcpool.go (1)
350-357: Context cancellation timing is correct (leak fixed)cancel() is called immediately after each attempt, avoiding per-iteration context leaks previously reported.
🧹 Nitpick comments (2)
node/rpcclient/rpcpool.go (2)
326-329: Reduce contention: use a read lock when taking a scored snapshotThis section only reads pool state; using a full write lock is unnecessary and increases contention under load.
Apply this diff:
- p.mu.Lock() + p.mu.RLock() sortedClients := p.getSortedClientsByScore() - p.mu.Unlock() + p.mu.RUnlock()
340-349: Optional: avoid O(n) scan to find currentIndex by pointerThis linear scan runs once per endpoint attempt and is O(n^2) across all endpoints. Maintain a pointer->index map or persist the index in RPCClientInfo to make this O(1).
I can provide a minimal patch that adds a map[int]*RPCClientInfo at pool init and keeps it in sync if you'd like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
node/rpcclient/rpcpool.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
node/rpcclient/rpcpool.go (4)
client/client.go (2)
HTTP(64-70)New(116-122)types/context.go (1)
Context(11-21)types/retry.go (2)
MaxRetryCount(11-11)SleepWithRetry(14-32)node/rpcclient/client.go (1)
RPCClient(39-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: opinitd
- GitHub Check: Run test
- GitHub Check: golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
node/rpcclient/rpcpool.go (1)
58-62: Initialization behavior and invalid-endpoint handling look goodClear docstring and explicit error when all endpoints are invalid improve debuggability and prevent surprises at runtime.
Also applies to: 102-106
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
node/rpcclient/rpcpool_test.go (3)
97-115: Tests may be slow due to exponential backoff; consider injectable backoff for speedWith maxRetries = 1, types.SleepWithRetry will sleep ~4–6s (exp backoff + jitter). That’s fine occasionally, but across many tests it slows CI. Recommend making the backoff strategy injectable on RPCPool so tests can supply a no-sleep or millisecond backoff without altering production behavior.
Example approach (outside this hunk): add a field backoffFn func(ctx context.Context, retry int) bool defaulting to types.SleepWithRetry, and in tests set pool.backoffFn to a fast stub.
138-158: Retry-success test correctness; note test runtimeLogic is correct (first attempt fails, first retry succeeds). Be aware this will still wait for the first backoff window (~4s) before retrying unless you inject a faster backoff. See prior suggestion.
233-244: Potential fragility: depends on client validation semanticsThis test assumes clienthttp.New will error on malformed URLs (e.g., "://malformed-url"). If the constructor defers validation until first request, this assertion will fail. Please confirm with the verification script above and align test/constructor behavior accordingly.
node/rpcclient/rpcpool.go (4)
278-293: Reduce lock contention while sorting by score (snapshot scores under RLock)Currently tryAllEndpointsWithScoring acquires an exclusive Lock to call getSortedClientsByScore. You can lower contention by taking an RLock, snapshotting the scores, then sorting outside the lock using the snapshot to avoid racing on client.score.
Apply this diff to getSortedClientsByScore:
func (p *RPCPool) getSortedClientsByScore() []*RPCClientInfo { - // Create a copy of the clients slice - sortedClients := make([]*RPCClientInfo, len(p.clients)) - copy(sortedClients, p.clients) - - // Sort by score (highest first), then by endpoint name for consistency - sort.Slice(sortedClients, func(i, j int) bool { - if sortedClients[i].score == sortedClients[j].score { - return sortedClients[i].endpoint < sortedClients[j].endpoint - } - return sortedClients[i].score > sortedClients[j].score - }) - - return sortedClients + // Snapshot clients and their scores under read lock, then sort using the snapshot. + p.mu.RLock() + sortedClients := make([]*RPCClientInfo, len(p.clients)) + copy(sortedClients, p.clients) + scores := make(map[*RPCClientInfo]float64, len(sortedClients)) + for _, c := range sortedClients { + scores[c] = c.score + } + p.mu.RUnlock() + + sort.Slice(sortedClients, func(i, j int) bool { + si, sj := scores[sortedClients[i]], scores[sortedClients[j]] + if si == sj { + return sortedClients[i].endpoint < sortedClients[j].endpoint + } + return si > sj + }) + return sortedClients }
309-317: Prefer zap.Error for richer, structured error loggingUse zap.Error(err) instead of stringifying errors to preserve stack/context and enable better log querying.
Apply this diff:
- p.logger.Warn("RPC request failed, trying next endpoint", - zap.String("endpoint", endpoint), - zap.String("error", err.Error())) + p.logger.Warn("RPC request failed, trying next endpoint", + zap.String("endpoint", endpoint), + zap.Error(err)) } else { - p.logger.Warn("RPC request failed during retry, trying next endpoint", - zap.String("endpoint", endpoint), - zap.String("error", err.Error()), - zap.Int("retry", retryAttempt)) + p.logger.Warn("RPC request failed during retry, trying next endpoint", + zap.String("endpoint", endpoint), + zap.Error(err), + zap.Int("retry", retryAttempt)) }
320-381: Don’t penalize endpoints on caller cancellation; classify timeouts vs cancellations explicitlyWhen the parent ctx is canceled, current code marks the endpoint unhealthy and decays its score. That unfairly penalizes healthy endpoints and can degrade routing after user-initiated aborts. Detect cancellation and skip scoring/health penalties in that case.
Apply this minimal diff:
@@ - var lastErr error - var isTimeout bool + var lastErr error + var isTimeout bool + var isCanceled bool @@ - isTimeout = err != nil && (errors.Is(timeoutCtx.Err(), context.DeadlineExceeded)) + isTimeout = err != nil && errors.Is(timeoutCtx.Err(), context.DeadlineExceeded) + isCanceled = err != nil && errors.Is(timeoutCtx.Err(), context.Canceled) @@ - // Failure - update score negatively and mark as unhealthy - p.UpdateScoreOnFailure(client, err, isTimeout) - p.MarkClientUnhealthy(client, err) + // Failure - if caller canceled, don't penalize the endpoint + if !isCanceled { + p.UpdateScoreOnFailure(client, err, isTimeout) + p.MarkClientUnhealthy(client, err) + } else { + p.logger.Info("RPC request aborted by caller (no penalty)", + zap.String("endpoint", client.endpoint)) + }Rationale:
- Timeout => keep current penalty (endpoint likely at fault or network issue)
- Cancellation => user-driven abort; endpoint should not be marked unhealthy or down-scored
383-423: Shadowed err variable in retry loop; simplify to avoid confusionThe inner
err := p.tryAllEndpointsWithScoring(...)shadows the outer err. This is harmless but misleading. Reuse the outer variable.Apply this diff:
- err := p.tryAllEndpointsWithScoring(ctx, fn, 0) + err := p.tryAllEndpointsWithScoring(ctx, fn, 0) if err == nil { return nil } @@ - err := p.tryAllEndpointsWithScoring(ctx, fn, retry) + err = p.tryAllEndpointsWithScoring(ctx, fn, retry) if err == nil { return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
node/rpcclient/rpcpool.go(1 hunks)node/rpcclient/rpcpool_test.go(1 hunks)node/rpcclient/scoring_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- node/rpcclient/scoring_test.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
node/rpcclient/rpcpool_test.go (3)
types/context.go (2)
Context(11-21)NewContext(23-30)node/rpcclient/rpcpool.go (1)
NewRPCPool(62-117)client/client.go (1)
New(116-122)
node/rpcclient/rpcpool.go (5)
client/client.go (2)
HTTP(64-70)New(116-122)types/context.go (1)
Context(11-21)types/retry.go (2)
MaxRetryCount(11-11)SleepWithRetry(14-32)version/version.go (1)
Info(11-14)node/rpcclient/client.go (1)
RPCClient(39-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run test
- GitHub Check: opinitd
- GitHub Check: golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
node/rpcclient/rpcpool_test.go (7)
17-21: Helper context setup looks goodUsing WithRPCTimeout on a shared test context keeps tests consistent and avoids hard-coding timeout inside the pool. LGTM.
117-136: Timeout test is soundForcing rpcTimeout < work duration and asserting context deadline exceeded is correct and deterministic. LGTM.
160-187: Cancellation flow behaves as expectedCancelling the parent context mid-flight and asserting context canceled is the right contract. LGTM.
189-218: Good logging verificationUsing zaptest/observer to assert the fallback warning is emitted is solid. The message matches the pool’s warn path. LGTM.
220-231: Valid endpoints test is appropriateEnsures the pool retains all syntactically valid endpoints. LGTM.
246-270: Mixed endpoints behavior and warning logs look goodFiltering invalid endpoints and asserting a warning was logged is good. Same caveat as above: ensure the client constructor actually rejects malformed endpoints at creation time.
272-283: Empty endpoints guard is correctAsserting an error ("no RPC endpoints provided") on empty input aligns with constructor behavior. LGTM.
node/rpcclient/rpcpool.go (3)
58-66: Constructor docs and empty-input validation are solidNice: you document that invalid endpoints are dropped and return a typed error for empty input. This aligns with prior review feedback and avoids panics.
146-158: Great fix: target-specific health/score updates prevent misattribution under concurrencyRefactoring MarkClientUnhealthy/UpdateScoreOn{Success,Failure} to take an explicit target client eliminates races caused by relying on currentIndex. This was a critical correctness improvement.
Also applies to: 197-225
425-456: RPC client creation path is coherent
- Guard against empty rpcAddresses
- Build pool, pick a healthy client (fallback to next healthy), wire into RPCClient
LGTM. Consider documenting that RPCClient methods assume the underlying pool will rotate/recover on failures.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
node/rpcclient/rpcpool_test.go (1)
25-31: Resolved: tests now use fully qualified URLsGood catch addressing earlier feedback by using scheme-qualified endpoints. This avoids false negatives during client construction.
🧹 Nitpick comments (9)
node/rpcclient/rpcpool_test.go (9)
3-15: Add missing imports to support robust log matching and early-failing assertionsTo support substring matching in the logging test and to fail fast when pool creation fails, import strings and require.
import ( "context" "errors" "testing" "time" + "strings" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/zap" "go.uber.org/zap/zaptest" "go.uber.org/zap/zaptest/observer" "github.com/initia-labs/opinit-bots/types" )
23-31: Run tests in parallel where safeThese unit tests don’t share global state; running in parallel speeds up CI without side effects.
func TestRPCPool_GetCurrentEndpoint(t *testing.T) { - logger := zaptest.NewLogger(t) + t.Parallel() + logger := zaptest.NewLogger(t)
33-56: Consider also asserting skip-over of unhealthy clientsNice wrap-around coverage. For completeness, consider marking the middle client unhealthy (e.g., via a public helper or exported method if available) and asserting MoveToNextHealthyClient skips it. This guards against regressions in health filtering.
97-115: Make assertions less brittle; derive expected call countString-check is fragile (“1 retries”), and callCount should be derived from endpoints and retries to avoid magic numbers.
assert.Error(t, err) - assert.Contains(t, err.Error(), "all RPC endpoints failed after 1 retries") - // 2 endpoints + 2 more for 1 retry = 4 calls - assert.Equal(t, 4, callCount) + // Error text can change; assert the stable prefix instead. + assert.Contains(t, err.Error(), "all RPC endpoints failed") + // expectedCalls = N endpoints * (maxRetries + 1 passes) + expectedCalls := len(endpoints) * (pool.maxRetries + 1) + assert.Equal(t, expectedCalls, callCount)
160-187: Reduce flakiness in cancellation testAvoid the goroutine/sleep race; use a deadline on the parent context so derived attempt contexts cancel deterministically.
// Create a context that will be canceled - ctx, cancel := context.WithCancel(context.Background()) - - // Cancel the context after a short delay - go func() { - time.Sleep(50 * time.Millisecond) - cancel() - }() + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel()
189-218: Relax log assertion to substring match to avoid brittle testsExact log messages often evolve; checking a stable substring is more resilient.
foundFailureLog := false for _, log := range logs { - if log.Message == "RPC request failed, trying next endpoint" { + if strings.Contains(log.Message, "RPC request failed") { foundFailureLog = true break } } assert.True(t, foundFailureLog, "Should have logged endpoint failure")
246-270: Optionally assert structured details on invalid endpoint logYou already assert the warning message is emitted. Consider also validating that the “endpoint” field is present and contains the malformed value. This ensures structured logs retain critical context.
Example augmentation within this test:
endpointFieldLogged := false for _, log := range logs { if log.Message == "Removing invalid endpoint from pool" { for _, f := range log.Context { if f.Key == "endpoint" && strings.Contains(f.String, "malformed-url") { endpointFieldLogged = true break } } } } assert.True(t, endpointFieldLogged, "Invalid endpoint should be included in warning fields")
102-105: Prefer options over mutating unexported fields in testsTests directly tweak pool internals (maxRetries, retryInterval, rpcTimeout). Consider functional options on NewRPCPool, or exported setters, to avoid reaching into internals and to document intended variability.
Example:
pool, err := NewRPCPool( ctx, endpoints, logger, WithMaxRetries(1), WithRetryInterval(10*time.Millisecond), WithRPCTimeout(100*time.Millisecond), )I can draft the options and wire them if you’d like.
Also applies to: 122-123, 143-145
27-27: Use require.NoError for setup to fail fastIf NewRPCPool fails, continuing will cause misleading follow-up errors. require.NoError stops the test early.
-assert.NoError(t, err) +require.NoError(t, err)Also update the two tests with custom messages similarly:
-assert.NoError(t, err, "NewRPCPool should succeed with valid endpoints") +require.NoError(t, err, "NewRPCPool should succeed with valid endpoints") -assert.NoError(t, err, "NewRPCPool should succeed with mixed endpoints (invalid ones filtered out)") +require.NoError(t, err, "NewRPCPool should succeed with mixed endpoints (invalid ones filtered out)")Also applies to: 37-37, 62-62, 80-80, 101-101, 121-121, 142-142, 164-164, 196-196, 226-226, 255-255
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
node/rpcclient/rpcpool_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
node/rpcclient/rpcpool_test.go (3)
types/context.go (2)
Context(11-21)NewContext(23-30)node/rpcclient/rpcpool.go (1)
NewRPCPool(62-117)client/client.go (1)
New(116-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: opinitd
- GitHub Check: golangci-lint
- GitHub Check: Run test
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
node/rpcclient/rpcpool_test.go (7)
58-75: LGTM: happy-path fallback APICovers single-attempt success and preserves current endpoint when no failover is needed.
76-96: LGTM: failover to next endpointGood verification that current endpoint advances on failure and remains on the succeeding endpoint.
117-136: LGTM: per-call timeout respectedSolid coverage of request timeout behavior using a short rpcTimeout and a longer task.
138-158: LGTM: retry path succeedsValidates retryInterval/maxRetries without overconstraining timing.
220-231: LGTM: valid endpoints pathAsserts happy-path initialization with multiple valid endpoints and preserves count.
233-245: LGTM: invalid endpoints pathCovers failure when all endpoints are malformed and validates error text.
272-283: LGTM: empty endpoints pathCovers required validation for empty inputs.
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests
Refactor