Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a time-based caching mechanism for block and hyperblock queries to improve performance by reducing redundant requests to observer nodes. The implementation introduces a new TimedCache interface with a sweep-based expiration strategy, and integrates it into the block retrieval workflow.
Key Changes:
- Added
TimedCacheinterface andtimeCacherimplementation with automatic TTL-based eviction - Implemented dual-indexing cache strategy (by hash and by nonce) for blocks and hyperblocks
- Added configurable
BlockCacheDurationSecsetting (default: 30 seconds)
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
process/interface.go |
Added TimedCache interface definition for time-based caching |
process/errors.go |
Added error constant for nil timed cache validation |
process/cache/timeCacher.go |
New implementation of time-based cache with periodic sweep goroutine |
process/cache/errors.go |
Added error constants for cache validation |
process/blockProcessor.go |
Integrated cache lookups before fetching blocks/hyperblocks from observers |
process/blockProcessorCache.go |
Core caching utilities with dual-indexing (hash and nonce) and generic retrieval |
data/block.go |
Added Hash() and Nonce() methods to response types for cache interface |
process/blockProcessor_test.go |
Updated all tests to include TimedCache parameter in constructor calls |
process/blockProcessorCache_test.go |
Comprehensive tests for cache key generation and retrieval logic |
facade/mock/timedCacheStub.go |
Mock implementation for testing (no-op cache) |
facade/mock/timedCacheMock.go |
Mock implementation with actual in-memory storage for tests |
config/config.go |
Added BlockCacheDurationSec configuration field |
cmd/proxy/config/config.toml |
Configuration documentation for cache duration setting |
cmd/proxy/main.go |
Initialization of timeCacher and injection into BlockProcessor |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log.Info("block request", "shard id", observer.ShardId, "hash", hash, "observer", observer.Address) | ||
| return &response, nil | ||
|
|
||
| bp.cacheObject(&response, scope, options) | ||
| return &response, nil |
There was a problem hiding this comment.
Caching responses with errors: The code caches the response before checking if it contains an error (the response.Error field). If an observer returns a response with an error field populated but doesn't return a connection error, that erroneous response will be cached. This could lead to caching and serving invalid data.
Consider checking the response's Error field before caching:
log.Info("block request", "shard id", observer.ShardId, "hash", hash, "observer", observer.Address)
if response.Error != "" {
log.Error("block response contains error", "error", response.Error)
continue
}
bp.cacheObject(&response, scope, options)
return &response, nilThere was a problem hiding this comment.
I think it would be expected
| log.Info("block request", "shard id", observer.ShardId, "nonce", nonce, "observer", observer.Address) | ||
| bp.cacheObject(&response, scope, options) | ||
| return &response, nil |
There was a problem hiding this comment.
Caching responses with errors: The code caches the response before checking if it contains an error (the response.Error field). If an observer returns a response with an error field populated but doesn't return a connection error, that erroneous response will be cached. This could lead to caching and serving invalid data.
Consider checking the response's Error field before caching:
log.Info("block request", "shard id", observer.ShardId, "nonce", nonce, "observer", observer.Address)
if response.Error != "" {
log.Error("block response contains error", "error", response.Error)
continue
}
bp.cacheObject(&response, scope, options)
return &response, nilThere was a problem hiding this comment.
I think it would be expected
Overview
Cache block/hyperblock results by hash/nonce. Before fetching from observer nodes,
GetBlockByHash,GetBlockByNonce,GetHyperBlockByHash, andGetHyperBlockByNoncenow:Configurable Cache Duration
BlockCacheDurationSec: Defines how long block/hyperblock responses are kept in cache before expiring.Timed cache
Took core logic from
timeCoreCacherfrom storage repo. If we want to use and adapt original code, there are a lot of needed changes and refactor, therefore so far that file does not have unit tests.Caching Logic
Added generic caching utilities for both blocks and hyperblocks.
Responses are cached using a composite key based on:
-- scope (block or hyperblock) + other specific input params such as shard,
-- hash,
-- query options (opts).
For each response we index:
-- the request object under its object key (hash + opts)
-- a hash lookup key for object key
-- a nonce lookup key for object key
Therefore, when a block/hyperblock is cached for the first time (either by a
GetByHashorGetByNoncerequest), it will be available afterwards for both request types