[RelayMiner] Implement relayminer query caching #1050

red-0ne · 2025-01-31T04:38:19Z

Summary

Implements caching layer for query clients to reduce network calls and improve performance

Primary Changes:

Added generic KeyValueCache and ParamsCache interfaces with thread-safe implementations
Integrated caching across all query clients (Account, Application, Bank, Service, etc.)
Added cache clearing on new blocks via WithNewBlockCacheClearing option

Secondary Changes:

Replaced manual sync.Mutex implementation in accQuerier with new cache interface
Added cache configuration to integration tests
Updated relayer dependencies to include cache initialization

Issue

The RelayMiner RPC queries are not cached, which puts excessive load on the configured full node, degrading the performance of both off-chain and on-chain components.

Type of change

Select one or more from the following:

New feature, functionality or library
Consensus breaking; add the consensus-breaking label if so. See [Infra] Automatically add the consensus-breaking label #791 for details
Bug fix
Code health or cleanup
Documentation
Other (specify)

Sanity Checklist

I have updated the GitHub Issue assignees, reviewers, labels, project, iteration and milestone
For docs, I have run make docusaurus_start
For code, I have run make go_develop_and_test and make test_e2e
For code, I have added the devnet-test-e2e label to run E2E tests in CI
For configurations, I have update the documentation
I added TODOs where applicable

github-actions · 2025-01-31T05:07:10Z

The image is going to be pushed after the next commit.

You can use make trigger_ci to push an empty commit.

If you also want to run E2E tests, please add devnet-test-e2e label.

Olshansk

@red-0ne I did a first partial review but have a lot of comments & questions.

Here's a high-level summary but PTAL at the actual comments as well:

Need to understand if/how this can build on top of [Off-chain] feat: in-memory query cache(s) #994 w/ @bryanchriswhite
See a few comments (logs + comments) that need to be addressed in multiple places
I’m a bit concerned (and don’t understand) how we’re not using “height” to retrieve things from the cache, especially when values are always changing
When does the cache ever get cleared?
Light on tests
I’d be interested to see numbers of performance improvement

pkg/client/query/accquerier.go

pkg/client/query/appquerier.go

pkg/client/query/sessionquerier.go

Olshansk · 2025-02-04T23:38:41Z

pkg/client/query/sharedquerier.go

@@ -49,11 +53,19 @@ func NewSharedQuerier(deps depinject.Config) (client.SharedQueryClient, error) {
 // Once `ModuleParamsClient` is implemented, use its replay observable's `#Last()` method
 // to get the most recently (asynchronously) observed (and cached) value.
 func (sq *sharedQuerier) GetParams(ctx context.Context) (*sharedtypes.Params, error) {
+	// Get the params from the cache if they exist.
+	if params, found := sq.paramsCache.Get(); found {


I'm concerned (and don't fully understand) the lack of a "height" param when retrieving things from the cache.

This cache implementation does not add any new functionality besides caching whatever has been queried.

It does not alter the RelayMiners current behavior

RelayMiner cold start

React to Params change

For those reasons, it does not leverage historical data that justifies the usage of height for cache querying.

My understanding is that the cache implementations here are NOT historical; i.e. ONLY the most recently observed value is cached for each ParamsCache instance (or key, in the case of KeyValueCache).

While #994 does include historical caching as well (via the HistoricalQueryCache interface, that's an additional and distinct feature.

This shouldn't be necessary here because we're clearing the cache on every new block. The end result being, somewhat sub-optimal, but significant caching. This reformulates the number off-chain queries from being a function of API usage, to no more than one per block, per cache.

pkg/client/query/sharedquerier.go

bryanchriswhite

Nice one @red-0ne! 🙌

Thanks for doing this! ❤️

pkg/client/query/cache/options.go

bryanchriswhite · 2025-02-05T11:01:53Z

pkg/client/query/interface.go

+}
+
+// KeyValueCache is an interface for a simple in-memory key-value cache implementation.
+type KeyValueCache[V any] interface {


This interface is consistent with what I was calling QueryCache[T any] in #994. It should be quite straightforward to refactor #994 to use this instead. I see KeyValueCache[V any] as a subsequent iteration of QueryCache[T any] which includes generalizing the name.

pkg/client/query/interface.go

pkg/client/query/cache/paramscache.go

bryanchriswhite · 2025-02-05T11:10:19Z

pkg/client/query/sessionquerier.go

+	clientConn        grpc.ClientConn
+	sessionQuerier    sessiontypes.QueryClient
+	sharedQueryClient client.SharedQueryClient
+	sessionsCache     KeyValueCache[*sessiontypes.Session]


bryanchriswhite · 2025-02-05T11:17:01Z

pkg/client/query/sharedquerier.go

@@ -49,11 +53,19 @@ func NewSharedQuerier(deps depinject.Config) (client.SharedQueryClient, error) {
 // Once `ModuleParamsClient` is implemented, use its replay observable's `#Last()` method
 // to get the most recently (asynchronously) observed (and cached) value.
 func (sq *sharedQuerier) GetParams(ctx context.Context) (*sharedtypes.Params, error) {
+	// Get the params from the cache if they exist.
+	if params, found := sq.paramsCache.Get(); found {


My understanding is that the cache implementations here are NOT historical; i.e. ONLY the most recently observed value is cached for each ParamsCache instance (or key, in the case of KeyValueCache).

While #994 does include historical caching as well (via the HistoricalQueryCache interface, that's an additional and distinct feature.

This shouldn't be necessary here because we're clearing the cache on every new block. The end result being, somewhat sub-optimal, but significant caching. This reformulates the number off-chain queries from being a function of API usage, to no more than one per block, per cache.

pkg/deps/config/suppliers.go

bryanchriswhite · 2025-02-07T08:35:28Z

pkg/client/query/appquerier.go

@@ -18,6 +19,12 @@ var _ client.ApplicationQueryClient = (*appQuerier)(nil)
 type appQuerier struct {
 	clientConn         grpc.ClientConn
 	applicationQuerier apptypes.QueryClient
+	logger             polylog.Logger
+
+	// applicationsCache caches applicationQueryClient.Application requests


Suggested change

// applicationsCache caches applicationQueryClient.Application requests

// applicationsCache caches application.Applications returned from applicationQueryClient.Application requests

bryanchriswhite · 2025-02-07T08:35:51Z

pkg/client/query/appquerier.go

+
+	// applicationsCache caches applicationQueryClient.Application requests
+	applicationsCache KeyValueCache[apptypes.Application]
+	// paramsCache caches applicationQueryClient.Params requests


Same as 👆

(seems like other places as well)

Olshansk

Leaving a partial review.

@bryanchriswhite Can you please prioritize getting [Off-chain] feat: in-memory query cache(s) #994 in? It's the most mature/versatile cache, and I'd like us to just build on top of 1 thing.
@red-0ne See some of my nits/edits, but in particular around using gomock for proper mocks.

Will do a full review after (1) & (2) are done.

Few notes:

If you think we should take a different direction, let's jump on a call.
We have other (large) parallel efforts going on, so there shouldn't be any blockers
I strongly believe we should benchmark in this PR. Seems like something an LLM can help get done in a couple of hours.

Olshansk · 2025-02-12T00:17:07Z

testutil/testclient/testqueryclients/grpcserver.go

+	c.callCount++
+}
+
+// MockServiceQueryServer is a mock implementation of the servicetypes.QueryServer interface


Up until now we've always been using gomock to generate this sort of thing, which has support for call counters.

I feel strongly that we should not be changing patterns now.

Olshansk · 2025-02-12T00:18:41Z

pkg/relayer/cmd/cmd.go

+		config.NewSupplyKeyValueCacheFn[*sessiontypes.Session](cache.WithNewBlockCacheClearing),
+		config.NewSupplyKeyValueCacheFn[*cosmostypes.Coin](cache.WithNewBlockCacheClearing),
+
+		config.NewSupplySharedQueryClientFn(), // leaf


We have some //leaf comments before the new code and some (this one) after the new code.

What's the idea behind this code organization?

Olshansk · 2025-02-12T00:19:01Z

pkg/relayer/cmd/cmd.go

+		config.NewSupplyKeyValueCacheFn[apptypes.Application](cache.WithNewBlockCacheClearing),
+		config.NewSupplyKeyValueCacheFn[cosmostypes.AccountI](cache.WithNewBlockCacheClearing),
+		config.NewSupplyKeyValueCacheFn[sharedtypes.Supplier](cache.WithNewBlockCacheClearing),
+		config.NewSupplyKeyValueCacheFn[*sessiontypes.Session](cache.WithNewBlockCacheClearing),


Olshansk · 2025-02-12T00:19:23Z

pkg/relayer/cmd/cmd.go

-		config.NewSupplySharedQueryClientFn(),                             // leaf
+
+		// Setup the params caches and configure them to clear on new blocks.
+		config.NewSupplyParamsCacheFn[sharedtypes.Params](cache.WithNewBlockCacheClearing),


Olshansk · 2025-02-12T00:21:29Z

pkg/relayer/cmd/cmd.go

-		config.NewSupplySharedQueryClientFn(),                             // leaf
+
+		// Setup the params caches and configure them to clear on new blocks.
+		// TODO_TECHDEBT: Consider a flag to change client queriers caching behavior.


I'm going to push on the fact that this is a TODO_IN_THIS_PR.

It's not that hard and I want to understand the benefit (if any) of this cache.

It feels like we've built a car but not actually checking if it works.

Olshansk · 2025-02-12T00:31:28Z

pkg/client/query/interface.go

+}
+
+// KeyValueCache is an interface for a simple in-memory key-value cache implementation.
+type KeyValueCache[V any] interface {


I left a comment on discord, but I feel strongly that we should finish #994 and rebase on top of it.

Now is the time to do this right.

Olshansk · 2025-02-12T00:37:33Z

pkg/client/query/types/balance.go

+// Balance represents a pointer to a Cosmos SDK Coin, specifically used for bank balance queries.
+// It is deliberately defined as a distinct type (not a type alias) to ensure clear dependency
+// injection and to differentiate it from other coin caches in the system. This type helps
+// maintain separation of concerns between different types of coin-related data in the caching
+// layer.


Suggested change

// Balance represents a pointer to a Cosmos SDK Coin, specifically used for bank balance queries.

// It is deliberately defined as a distinct type (not a type alias) to ensure clear dependency

// injection and to differentiate it from other coin caches in the system. This type helps

// maintain separation of concerns between different types of coin-related data in the caching

// layer.

// Balance represents a pointer to a Cosmos SDK Coin used for bank balance queries.

// It is defined as a distinct type (not an alias) to:

// - Ensure clear dependency injection

// - Differentiate from other coin caches in the system

// - Maintain separation of concerns between coin-related data in the caching layer

type Balance *sdk.Coin

@red-0ne Have you used the code-cleaner Claude project yet?

Olshansk · 2025-02-12T00:38:13Z

pkg/client/query/types/blockhash.go

+// BlockHash represents a byte slice, specifically used for bank balance query caches.
+// It is deliberately defined as a distinct type (not a type alias) to ensure clear
+// dependency injection and to differentiate it from other byte slice caches in the system.
+// This type helps maintain separation of concerns between different types of
+// byte slice data in the caching layer.


Suggested change

// BlockHash represents a byte slice, specifically used for bank balance query caches.

// It is deliberately defined as a distinct type (not a type alias) to ensure clear

// dependency injection and to differentiate it from other byte slice caches in the system.

// This type helps maintain separation of concerns between different types of

// byte slice data in the caching layer.

// BlockHash represents a byte slice used for bank balance query caches.

// It is defined as a distinct type (not an alias) to:

// - Ensure clear dependency injection

// - Differentiate from other byte slice caches

// - Maintain separation of concerns between byte slice data in caching layer

Olshansk · 2025-02-12T00:38:28Z

pkg/client/query/types/blockhash.go

@@ -0,0 +1,8 @@
+package types


Having separate files for this feels like overkill.

Can we just have a types.go?

Olshansk · 2025-02-12T00:40:53Z

pkg/client/query/supplierquerier.go

+		return supplier, nil
+	}
+
+	logger.Debug().Msgf("cache miss for key: %s", operatorAddress)


Leaving one comment but please update everywhere. If these logs ever becomes the source for debugging, you want it to be ULTRA obvious.

Suggested change

logger.Debug().Msgf("cache miss for key: %s", operatorAddress)

logger.Debug().Msgf("cache miss for operator address key: %s", operatorAddress)

red-0ne added 2 commits January 31, 2025 04:55

feat: Implement relayminer query caching

c12abfd

Merge remote-tracking branch 'origin/main' into feat/relayminer-caching

4e4d9de

red-0ne added the relayminer Changes related to the Relayminer label Jan 31, 2025

red-0ne added this to the Beta TestNet Iteration milestone Jan 31, 2025

red-0ne requested review from Olshansk and adshmh January 31, 2025 04:38

red-0ne self-assigned this Jan 31, 2025

red-0ne requested a review from bryanchriswhite January 31, 2025 04:41

red-0ne added the push-image CI related - pushes images to ghcr.io label Jan 31, 2025

fix: Failing test due to wrong balance

ccc99b4

red-0ne mentioned this pull request Feb 3, 2025

[DO NOT MERGE] RelayMiner querier caching #1038

Closed

14 tasks

Merge remote-tracking branch 'origin/main' into feat/relayminer-caching

10e0a50

Olshansk requested changes Feb 4, 2025

View reviewed changes

bryanchriswhite reviewed Feb 5, 2025

View reviewed changes

red-0ne added 5 commits February 5, 2025 23:31

chore: Address reivew change requests

8485713

Merge remote-tracking branch 'origin/main' into feat/relayminer-caching

8e2138b

fix: Test cache missing parameter

0cc0a32

test: Test cache and query client caching

410b1e4

chore: Add techdebt to enable caching flags

a10718b

red-0ne requested review from Olshansk and bryanchriswhite February 6, 2025 21:12

red-0ne and others added 3 commits February 7, 2025 02:39

fix: unchecked error

6afc4a2

fix: missing logger dpendency

fda3c8c

Merge branch 'main' into feat/relayminer-caching

8bd2fff

bryanchriswhite reviewed Feb 10, 2025

View reviewed changes

Olshansk mentioned this pull request Feb 12, 2025

[Off-chain] feat: in-memory query cache(s) #994

Open

15 tasks

Olshansk requested changes Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RelayMiner] Implement relayminer query caching #1050

[RelayMiner] Implement relayminer query caching #1050

red-0ne commented Jan 31, 2025

github-actions bot commented Jan 31, 2025

Olshansk left a comment

Olshansk Feb 4, 2025

red-0ne Feb 5, 2025

bryanchriswhite Feb 5, 2025

bryanchriswhite left a comment

bryanchriswhite Feb 5, 2025

bryanchriswhite Feb 5, 2025

bryanchriswhite Feb 5, 2025

bryanchriswhite Feb 7, 2025

bryanchriswhite Feb 7, 2025

Olshansk left a comment

Olshansk Feb 12, 2025

Olshansk Feb 12, 2025

Olshansk Feb 12, 2025

Olshansk Feb 12, 2025

Olshansk Feb 12, 2025

Olshansk Feb 12, 2025

Olshansk Feb 12, 2025

Olshansk Feb 12, 2025

Olshansk Feb 12, 2025

Olshansk Feb 12, 2025

	// applicationsCache caches applicationQueryClient.Application requests
	// applicationsCache caches application.Applications returned from applicationQueryClient.Application requests

-// Balance represents a pointer to a Cosmos SDK Coin, specifically used for bank balance queries.
-// It is deliberately defined as a distinct type (not a type alias) to ensure clear dependency
-// injection and to differentiate it from other coin caches in the system. This type helps
-// maintain separation of concerns between different types of coin-related data in the caching
-// layer.
+// Balance represents a pointer to a Cosmos SDK Coin used for bank balance queries.
+// It is defined as a distinct type (not an alias) to:
+// - Ensure clear dependency injection
+// - Differentiate from other coin caches in the system
+// - Maintain separation of concerns between coin-related data in the caching layer
+type Balance *sdk.Coin

	logger.Debug().Msgf("cache miss for key: %s", operatorAddress)
	logger.Debug().Msgf("cache miss for operator address key: %s", operatorAddress)

[RelayMiner] Implement relayminer query caching #1050

Are you sure you want to change the base?

[RelayMiner] Implement relayminer query caching #1050

Conversation

red-0ne commented Jan 31, 2025

Summary

Primary Changes:

Secondary Changes:

Issue

Type of change

Sanity Checklist

github-actions bot commented Jan 31, 2025

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanchriswhite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment