Add quick balance cache to test if it helps with load on mainnet #607
Conversation
peterargue
requested review from
janezpodhostnik,
m-Peter,
zhangchiqing and
ramtinms
as code owners
WalkthroughThe changes introduce modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BlockChainAPI
participant EVM
Client->>BlockChainAPI: Request Balance
BlockChainAPI->>BlockChainAPI: Check balanceCache
alt Balance Found
BlockChainAPI-->>Client: Return Cached Balance
else Balance Not Found
BlockChainAPI->>EVM: Get Balance
EVM-->>BlockChainAPI: Return Balance
BlockChainAPI->>BlockChainAPI: Cache the Balance
BlockChainAPI-->>Client: Return Balance
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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}
| @@ -91,6 +93,8 @@ type BlockChainAPI struct { | |||
| indexingResumedHeight uint64 | |||
| limiter limiter.Store | |||
| collector metrics.Collector | |||
|
|
|||
| balanceCache *lru.TwoQueueCache[common.Hash, *hexutil.Big] | |||
There was a problem hiding this comment.
Ensure thread safety of balanceCache by using a concurrent cache or synchronization
The balanceCache is accessed in the GetBalance method, which may be called concurrently. The lru.TwoQueueCache from github.com/hashicorp/golang-lru/v2 is not safe for concurrent use without additional synchronization.
To prevent data races and ensure thread safety:
- Use a thread-safe cache implementation, such as
lru.ARCCache, which is safe for concurrent access.
Apply this diff to switch to a thread-safe cache:
- balanceCache, err := lru.New2Q[common.Hash, *hexutil.Big](10_000)
+ balanceCache, err := lru.NewARC[common.Hash, *hexutil.Big](10_000)
if err != nil {
return nil, fmt.Errorf("failed to create balance cache: %w", err)
}Alternatively, protect access to balanceCache with a sync.RWMutex:
import (
// ...
+ "sync"
)
// ...
type BlockChainAPI struct {
// ...
balanceCache *lru.TwoQueueCache[common.Hash, *hexutil.Big]
+ balanceCacheMutex sync.RWMutex
}
func (b *BlockChainAPI) GetBalance(
ctx context.Context,
address common.Address,
blockNumberOrHash rpc.BlockNumberOrHash,
) (*hexutil.Big, error) {
// ...
+ b.balanceCacheMutex.RLock()
if balance, ok := b.balanceCache.Get(cacheKey); ok {
+ b.balanceCacheMutex.RUnlock()
return balance, nil
}
+ b.balanceCacheMutex.RUnlock()
// ...
balance, err := b.evm.GetBalance(ctx, address, evmHeight)
// ...
+ b.balanceCacheMutex.Lock()
b.balanceCache.Add(cacheKey, response)
+ b.balanceCacheMutex.Unlock()
// ...
}Also applies to: 232-236, 244-245
There was a problem hiding this comment.
this is not true according to the module's docs
https://github.com/hashicorp/golang-lru/blob/d582c453703398d4d70e3fa70d3f812ecc85a1c6/2q.go#L23-L24
There was a problem hiding this comment.
@peterargue, thank you for pointing that out. You're correct, according to the module's documentation, lru.TwoQueueCache is safe for concurrent use without additional synchronization. I'll keep this in mind for future reviews.
(꒦_꒦)
✏️ Learnings added
Learnt from: peterargue
PR: onflow/flow-evm-gateway#607
File: api/api.go:97-97
Timestamp: 2024-10-03T00:55:53.271Z
Learning: The `lru.TwoQueueCache` from `github.com/hashicorp/golang-lru/v2` is safe for concurrent use without additional synchronization.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
api/api.go
Outdated
| func getCacheKey(address common.Address, evmHeight int64) common.Hash { | ||
| // hash is 32 bytes | ||
| // address (20 bytes) + int64 (8 bytes) = 28 bytes total | ||
| keyData := address.Bytes() | ||
| binary.BigEndian.AppendUint64(keyData, uint64(evmHeight)) | ||
| return common.BytesToHash(keyData) |
There was a problem hiding this comment.
Fix 'AppendUint64' usage and prevent cache key collisions in 'getCacheKey' function
In the getCacheKey function, there are two issues:
-
Incorrect use of
AppendUint64: Thebinary.BigEndian.AppendUint64function returns the updated slice, but the current code does not assign this return value back tokeyData. As a result, the block height is not appended tokeyData, and the cache key does not include the block height. -
Potential cache key collisions: The
keyDataslice after appending is 28 bytes long (20 bytes for the address and 8 bytes for the block height). However,common.BytesToHashexpects a 32-byte input and pads it if it's shorter. This could lead to cache key collisions, as differentkeyDatavalues might result in the same hash.
To fix these issues:
- Assign the result of
AppendUint64back tokeyData. - Ensure that
keyDatais exactly 32 bytes before converting it to a hash.
Apply this diff to address the issues:
func getCacheKey(address common.Address, evmHeight int64) common.Hash {
keyData := address.Bytes()
- binary.BigEndian.AppendUint64(keyData, uint64(evmHeight))
+ keyData = binary.BigEndian.AppendUint64(keyData, uint64(evmHeight))
+ // Ensure keyData is 32 bytes
+ var fullKeyData [32]byte
+ copy(fullKeyData[:28], keyData)
+ // Optionally, zero-fill the remaining bytes if needed
return common.BytesToHash(fullKeyData[:])
}Alternatively, use a cryptographic hash function to generate a consistent 32-byte key:
+import (
+ "crypto/sha256"
+)
// ...
func getCacheKey(address common.Address, evmHeight int64) common.Hash {
keyData := address.Bytes()
keyData = binary.BigEndian.AppendUint64(keyData, uint64(evmHeight))
+ hash := sha256.Sum256(keyData)
+ return common.BytesToHash(hash[:])
}📝 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 getCacheKey(address common.Address, evmHeight int64) common.Hash { | |
| // hash is 32 bytes | |
| // address (20 bytes) + int64 (8 bytes) = 28 bytes total | |
| keyData := address.Bytes() | |
| binary.BigEndian.AppendUint64(keyData, uint64(evmHeight)) | |
| return common.BytesToHash(keyData) | |
| func getCacheKey(address common.Address, evmHeight int64) common.Hash { | |
| // hash is 32 bytes | |
| // address (20 bytes) + int64 (8 bytes) = 28 bytes total | |
| keyData := address.Bytes() | |
| keyData = binary.BigEndian.AppendUint64(keyData, uint64(evmHeight)) | |
| // Ensure keyData is 32 bytes | |
| var fullKeyData [32]byte | |
| copy(fullKeyData[:28], keyData) | |
| // Optionally, zero-fill the remaining bytes if needed | |
| return common.BytesToHash(fullKeyData[:]) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
api/api.go (1)
Line range hint
1-1354: Summary of balance cache implementation reviewThe addition of a balance cache to the
BlockChainAPIis a positive step towards improving performance and reducing load on the mainnet, which aligns well with the PR objectives. However, there are several important points that need to be addressed:
- The cache size is currently hardcoded and should be made configurable for better flexibility.
- There's a potential race condition in accessing the cache that needs to be resolved, either by using a thread-safe cache implementation or by adding proper synchronization.
- The cache key generation method could lead to key collisions and should be improved to ensure uniqueness.
Addressing these issues will greatly improve the robustness and correctness of the balance cache implementation. Once these changes are made, the performance benefits of the cache can be realized without introducing new bugs or race conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- api/api.go (5 hunks)
🔇 Additional comments (1)
api/api.go (1)
6-6: LGTM: Addition of balance cache aligns with PR objectiveThe new import of the
lrupackage and the addition of thebalanceCachefield to theBlockChainAPIstruct are in line with the PR objective of adding a quick balance cache to help with load on mainnet. This is a good step towards improving performance.Also applies to: 13-13, 96-97
| balanceCache, err := lru.New2Q[common.Hash, *hexutil.Big](10_000) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create balance cache: %w", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the balance cache size configurable
The initialization of the balanceCache looks good, but the cache size is currently hardcoded to 10,000. It might be beneficial to make this value configurable, either through the config struct or as a parameter to the NewBlockChainAPI function. This would allow for easier tuning of the cache size based on different deployment environments or requirements.
Example:
balanceCache, err := lru.New2Q[common.Hash, *hexutil.Big](config.BalanceCacheSize)Also applies to: 133-133
| cacheKey := getCacheKey(address, evmHeight) | ||
| if balance, ok := b.balanceCache.Get(cacheKey); ok { | ||
| return balance, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure thread-safety when accessing the balance cache
The implementation of the cache lookup and update is correct, but there's a potential race condition when accessing the balanceCache. The lru.TwoQueueCache is not safe for concurrent use without additional synchronization.
To ensure thread-safety, consider one of the following options:
- Use a thread-safe cache implementation, such as
lru.ARCCache. - Protect access to
balanceCachewith async.RWMutex.
Here's an example of how you could implement option 2:
type BlockChainAPI struct {
// ...
balanceCache *lru.TwoQueueCache[common.Hash, *hexutil.Big]
balanceCacheMutex sync.RWMutex
}
func (b *BlockChainAPI) GetBalance(ctx context.Context, address common.Address, blockNumberOrHash rpc.BlockNumberOrHash) (*hexutil.Big, error) {
// ...
b.balanceCacheMutex.RLock()
if balance, ok := b.balanceCache.Get(cacheKey); ok {
b.balanceCacheMutex.RUnlock()
return balance, nil
}
b.balanceCacheMutex.RUnlock()
// ... (existing code to fetch balance)
b.balanceCacheMutex.Lock()
b.balanceCache.Add(cacheKey, response)
b.balanceCacheMutex.Unlock()
return response, nil
}This ensures that concurrent access to the cache is properly synchronized.
Also applies to: 244-245
| func getCacheKey(address common.Address, evmHeight int64) common.Hash { | ||
| // hash is 32 bytes | ||
| // address (20 bytes) + int64 (8 bytes) = 28 bytes total | ||
| keyData := address.Bytes() | ||
| keyData = binary.BigEndian.AppendUint64(keyData, uint64(evmHeight)) | ||
| return common.BytesToHash(keyData) |
There was a problem hiding this comment.
Improve cache key generation to prevent potential collisions
The getCacheKey function's logic is correct, but the current implementation might lead to key collisions. The common.BytesToHash function used at the end expects a 32-byte input, but keyData is only 28 bytes (20 for address + 8 for uint64). This means that different inputs could potentially produce the same hash.
To prevent this, consider one of the following approaches:
- Use a cryptographic hash function to generate a consistent 32-byte key:
import "crypto/sha256"
func getCacheKey(address common.Address, evmHeight int64) common.Hash {
keyData := address.Bytes()
keyData = binary.BigEndian.AppendUint64(keyData, uint64(evmHeight))
hash := sha256.Sum256(keyData)
return common.BytesToHash(hash[:])
}- Ensure the input to
BytesToHashis always 32 bytes:
func getCacheKey(address common.Address, evmHeight int64) common.Hash {
var keyData [32]byte
copy(keyData[:20], address.Bytes())
binary.BigEndian.PutUint64(keyData[24:], uint64(evmHeight))
return common.BytesToHash(keyData[:])
}Both of these approaches will ensure that the cache key is unique for each combination of address and height, preventing potential collisions.
| @@ -219,12 +229,29 @@ func (b *BlockChainAPI) GetBalance( | |||
| return handleError[*hexutil.Big](err, l, b.collector) | |||
| } | |||
|
|
|||
| cacheKey := getCacheKey(address, evmHeight) | |||
There was a problem hiding this comment.
The CI is failing because of the special values for evmHeight. For example:
{
"jsonrpc": "2.0",
"method": "eth_getBalance",
"params": [
"0xa94f5374Fce5edBC8E2a8697C15331677e6EbF0B",
"latest"
],
"id": 10
}This is a very common JSON-RPC request for retrieving the balance of an address. The "latest" special value is used to indicate the latest EVM block height. In the code, this means that evmHeight has the special value -2. Internally, we use this special value to call ExecuteScriptAtLatestBlock instead of ExecuteScriptAtBlockHeight.
We might want to do something like this:
var height int64
if evmHeight < 0 {
latestEVMHeight, err := b.blocks.LatestEVMHeight()
if err != nil {
return handleError[*hexutil.Big](err, l, b.collector)
}
height = int64(latestEVMHeight)
} else {
height = evmHeight
}
cacheKey := getCacheKey(address, height)to translate the special value -2 to an actual EVM block height.
|
this ultimately wouldn't have solved the issue we encountered. closing this and we can circle back with a better caching solution later |
Closes: #???
Description
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit