Better web3 rpc handling & add caching for getBlockBeforeTime#4099
Better web3 rpc handling & add caching for getBlockBeforeTime#4099kajoseph wants to merge 10 commits intobitpay:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves Web3 RPC handling by implementing round-robin load balancing and reducing memory leaks through proper connection lifecycle management. It also adds caching for getBlockBeforeTime() calls and fixes various logging issues.
Changes:
- Refactored RPC initialization to use round-robin load balancing across multiple providers
- Added proper RPC teardown methods to prevent memory leaks
- Implemented LRU caching for
getBlockBeforeTime()to reduce redundant database queries - Fixed logging typos and improved error message formatting
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/bitcore-node/src/providers/chain-state/evm/api/csp.ts | Implements round-robin RPC selection and connection management |
| packages/bitcore-node/src/providers/chain-state/svm/api/csp.ts | Implements similar RPC management for Solana chains |
| packages/bitcore-node/src/providers/chain-state/internal/internal.ts | Adds LRU cache for block-at-time queries |
| packages/bitcore-node/src/modules/moralis/api/csp.ts | Adds caching to Moralis provider's getBlockBeforeTime |
| packages/bitcore-node/src/routes/api/fee.ts | Fixes logging typo (stackk → stack) |
| packages/bitcore-node/src/modules/moralis/p2p/p2p.ts | Fixes missing parameters in error log |
| packages/bitcore-node/test/unit/modules/base/csp.test.ts | Adds comprehensive tests for new RPC management |
| packages/bitcore-node/test/unit/modules/ethereum/csp.test.ts | Updates tests to match new RPC structure |
| packages/bitcore-node/test/unit/modules/matic/csp.test.ts | Updates tests to match new RPC structure |
| packages/bitcore-node/test/integration/ethereum/memory-leaks.test.ts | Adds eslint disable comment for legitimate this alias |
| packages/bitcore-node/package.json | Removes tsx dependency, adds lru-cache |
| packages/bitcore-node/src/types/Config.ts | Adds ProviderDataType type alias |
| packages/bitcore-node/src/providers/chain-state/external/providers/provider.ts | File deleted (functionality moved inline) |
Files not reviewed (1)
- packages/bitcore-node/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/bitcore-node/src/providers/chain-state/internal/internal.ts
Outdated
Show resolved
Hide resolved
| } | ||
| this.blockAtTimeCache[chainNetwork].set(date.toISOString(), block); | ||
| } else { | ||
| this.blockAtTimeCache[chainNetwork].set(date.toISOString(), block, { ttl: 10000 }); // cache nulls for 10 sec |
There was a problem hiding this comment.
Would reduce cognitive load to either directly set the cache to null here - or at the least a comment above - "block is null"
| static rpcs = {} as { [chainNetwork: string]: { historical: GetWeb3Response[]; realtime: GetWeb3Response[] } }; | ||
| static rpcIndicies = {} as { [chainNetwork: string]: { historical: number; realtime: number } }; |
There was a problem hiding this comment.
Be declarative - use explicit type annotations instead of type assertions
| if (BaseEVMStateProvider.rpcInitialized[chain]) { | ||
| return; | ||
| } | ||
| BaseEVMStateProvider.rpcInitialized[chain] = true; |
There was a problem hiding this comment.
Setting flag after initialization seems right semantically, and provides (hopefully) redundant support if the initialization fails.
There was a problem hiding this comment.
This is meant to reduce race conditions. Though I suppose the "danger" of duplicate rpc's isn't very dangerous.
| } | ||
| } | ||
| index = (index + 1) % BaseEVMStateProvider.rpcs[chainNetwork][type].length; | ||
| } while (index !== lastIndex + 1); |
There was a problem hiding this comment.
Check when lastIndex is length - 1.
There was a problem hiding this comment.
Not sure I understand this comment. lastIndex is a const so while (lastIndex != length - 1) would be a forever loop.
| if (!this.blockAtTimeCache[chainNetwork]) { | ||
| this.blockAtTimeCache[chainNetwork] = new LRUCache<string, IBlock>({ max: 1000 }); | ||
| } | ||
| this.blockAtTimeCache[chainNetwork].set(date.toISOString(), block); |
There was a problem hiding this comment.
How often will this be called w/o params.time? This is going to make a lot of churn. I know it's capped at 1k, but may be worth thinking about a bucket implementation depending on how it's usually called.
2 participants
Description
This PR improves the getWeb() by round-robin load-balancing RPCs as well as reduces memory leaks. Also adds caching to getBlockBeforeTime() to reduce redundant calls. Also a few various/random log typo fixes & testing improvements.
Changelog
tsxdependency to run tests as js files.Checklist
BWCif modifying the bitcore-wallet-client package,CLIif modifying the bitcore-cli package, etc.)