Feed LatestPrice query cache #11326

samsondav · 2023-11-17T14:38:44Z

No description provided.

github-actions · 2023-11-17T16:07:06Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

github-actions · 2023-11-17T16:07:06Z

I see that you haven't updated any README files. Would it make sense to do so?

core/services/relay/evm/mercury/cache.go

cl-sonarqube-production · 2023-11-21T18:33:56Z

SonarQube Quality Gate

Reliability Rating on New Code (is worse than A)
8.8% Coverage on New Code (is less than 75%)

See analysis details on SonarQube

Fix issues before they fail your Quality Gate with SonarLint in your IDE.

core/services/relay/evm/mercury/wsrpc/cache/cache.go

core/services/relay/evm/evm.go

core/services/relay/evm/mercury/wsrpc/cache/cache_set.go

core/services/chainlink/application.go

jmank88 · 2023-11-27T18:16:02Z

core/services/relay/evm/evm.go

@@ -100,7 +101,7 @@ func NewRelayer(lggr logger.Logger, chain evm.Chain, opts RelayerOpts) (*Relayer
 		chain:            chain,
 		lggr:             lggr,
 		ks:               opts.CSAETHKeystore,
-		mercuryPool:      wsrpc.NewPool(lggr),
+		mercuryPool:      opts.MercuryPool,


Why is this a top level pool now? Is the intention to share it across chains? That means it will cross the relay API, and I'm not sure how that is meant to work 🤔

Yes it is shared across chains. I asked @cedric-cordenier and this is what he suggested.

So the relay API has to proxy the shared websocket connections? Why is that better than each plugin process having it's own local pool?

@jmank88 The reason why I suggested adding this here was because I didn't really consider this part of the relayer LOOPP API. For me this isn't part of it -- see for example how we're passing in the database. In a LOOP world, we wouldn't do that: we'd either pass in the config values and create the pool in the relayer proper, or pass in a proxy service.

I'm trying to understand your objection: I guess you're saying that this would break the isolation LOOPPs are supposed to provide?

I'm observing that this at least adds to the technical debt in the way of LOOPPifying mercury.

If we intend to share these pools between host and plugins, then I guess that is just something to figure out how to do (but it does not immediately make sense to me).

If we do not intend to share these pools between host and plugins, then there is no need to refactor things to be global like this in the first place since we'll just have to undo it in a few weeks. Instead, each relayer should simply own and manage a pool internally.

Yes, good point.

One interesting thought that occurred to me is that you could look at this as a symptom of there not being a single "Mercury relayer", i.e. it's a symptom of there being a mercury provider in all relayers.

Instead, maybe a more natural way to look at this is that Mercury is a "chain", and we should give it its own relayer. If we do that, this problem goes away as calls to Mercury would go via the same relayer and therefore would benefit from the wsrpc pool which in that case would be a totally internal, but shared resource. This Mercury relayer would only implement the write APIs (i.e. the transmitter, the codec etc), and would leave the read side unimplemented or stubbed out.

The benefit of this is that it cleanly maps the relayer as a 1-1 abstraction over the underlying resource, in the same way that relayers do with the chains they target.

It does mean that we'd need to refine the relayer abstraction somewhat, to allow products to use a different relayer for reading and for writing, but that doesn't feel wrong to me, and might even be a better underlying fit (CCIP might benefit from this for example).

Yeah I think that separate reading and writing relayers makes sense 👍

@samsondav Do you think that is a good long-term direction for Mercury?

I agree, separating the read/write relayers makes sense to me. However, note that in this case the "write" relayer would also have to support reading. Since we send reports to mercury but also query reports from it.

Treating mercury server as a "chain" is definitely the right way to go.

core/services/relay/evm/mercury/wsrpc/cache/cache.go

cedric-cordenier · 2023-11-28T09:58:28Z

core/services/relay/evm/mercury/wsrpc/cache/cache.go

+		// NOTE: must drop down to RawClient here otherwise we enter an
+		// infinite loop of calling a client that calls back to this same cache
+		// and on and on
+		val, err = m.client.RawClient().LatestReport(ctx, req)


@samsondav Did you consider inverting the dependency here? i.e. rather than having a cache which depends on a client, you have a client which depends on a cache. That would allow the cache to be totally generic and reusable and not have any notion of client, while your client is responsible for making requests and storing things in the cache as needed (even if this happens in a loop).

I thought this was how I designed it? The client does depend on the cache and calls to the cache if its there.

We discussed this offline: an aside cache won't work here since we're also trying to dedupe in flight requests. The main reason for this cache is to service ETH/LINK feeds, which are high volume, so this complexity is worth it.

cedric-cordenier · 2023-11-28T10:06:23Z

core/services/relay/evm/mercury/wsrpc/cache/cache.go

+// Context should be set carefully and timed to be the maximum time we are
+// willing to wait for a result, the background thread will keep re-querying
+// until it gets one even on networking errors etc.
+func (m *memCache) LatestReport(ctx context.Context, req *pb.LatestReportRequest) (resp *pb.LatestReportResponse, err error) {


Is this more complicated than it needs to be? I can see that it accounts for the cache wait case, but I'm wondering if you considered using a simpler strategy (cache aside, rather than read through). This might be a bit less efficient, but I think would significantly reduce all of the edge cases you need to handle, which feels like a win.

Can you outline how the simpler strategy would work?

Update: as above.

core/services/relay/evm/mercury/wsrpc/cache/cache_set.go

core/services/relay/evm/mercury/wsrpc/cache/cache.go

Problem: Core nodes are querying LatestReport a lot (~70rps). This is not only wasteful, it potentially increases latency if Mercury server falls behind and is also completely unnecessary. We do not need such up-to-the-millisecond accuracy for billing, Proposed solution: Introduce a price cache for mercury with a TTL e.g. of 1 minute or whatever tolerance we have for billing price and only fetch from the server when the cache becomes stale.

cl-sonarqube-production · 2023-11-28T15:26:14Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

77.1% Coverage
0.0% Duplication

* Price retrieval cache for Mercury Problem: Core nodes are querying LatestReport a lot (~70rps). This is not only wasteful, it potentially increases latency if Mercury server falls behind and is also completely unnecessary. We do not need such up-to-the-millisecond accuracy for billing, Proposed solution: Introduce a price cache for mercury with a TTL e.g. of 1 minute or whatever tolerance we have for billing price and only fetch from the server when the cache becomes stale. * fix * Some PR comments * Fix more PR comments * PR comments * lint

samsondav force-pushed the MERC-2486-cache-for-querying-link-eth-prices-in-mercury branch 5 times, most recently from 0cca18b to 9f7adda Compare November 17, 2023 16:43

jmank88 reviewed Nov 17, 2023

View reviewed changes

core/services/relay/evm/mercury/cache.go Outdated Show resolved Hide resolved

jmank88 reviewed Nov 17, 2023

View reviewed changes

core/services/relay/evm/mercury/cache.go Outdated Show resolved Hide resolved

jmank88 reviewed Nov 17, 2023

View reviewed changes

core/services/relay/evm/mercury/cache.go Outdated Show resolved Hide resolved

samsondav force-pushed the MERC-2486-cache-for-querying-link-eth-prices-in-mercury branch 2 times, most recently from b5b3a91 to 5f84251 Compare November 21, 2023 18:21

samsondav force-pushed the MERC-2486-cache-for-querying-link-eth-prices-in-mercury branch 6 times, most recently from 6507d18 to b97d8f2 Compare November 22, 2023 21:45

samsondav commented Nov 22, 2023

View reviewed changes

core/services/relay/evm/mercury/wsrpc/cache/cache.go Show resolved Hide resolved

samsondav force-pushed the MERC-2486-cache-for-querying-link-eth-prices-in-mercury branch from b97d8f2 to c8a74c5 Compare November 22, 2023 22:06

samsondav commented Nov 22, 2023

View reviewed changes

core/services/relay/evm/evm.go Show resolved Hide resolved

samsondav force-pushed the MERC-2486-cache-for-querying-link-eth-prices-in-mercury branch 2 times, most recently from 318cb58 to 81fda1b Compare November 24, 2023 16:52

samsondav marked this pull request as ready for review November 24, 2023 16:57

samsondav requested review from a team, jkongie, sdrug, martin-cll and krehermann as code owners November 24, 2023 16:57

jmank88 reviewed Nov 27, 2023

View reviewed changes

core/services/relay/evm/mercury/wsrpc/cache/cache_set.go Outdated Show resolved Hide resolved

samsondav force-pushed the MERC-2486-cache-for-querying-link-eth-prices-in-mercury branch from 510b488 to 41a617a Compare November 27, 2023 18:07

jmank88 reviewed Nov 27, 2023

View reviewed changes

core/services/chainlink/application.go Show resolved Hide resolved

jmank88 reviewed Nov 27, 2023

View reviewed changes

core/services/relay/evm/mercury/wsrpc/cache/cache.go Outdated Show resolved Hide resolved

jmank88 reviewed Nov 27, 2023

View reviewed changes

core/services/relay/evm/mercury/wsrpc/cache/cache.go Outdated Show resolved Hide resolved

samsondav requested review from brunotm and jmank88 November 27, 2023 19:01

cedric-cordenier reviewed Nov 28, 2023

View reviewed changes

jmank88 reviewed Nov 28, 2023

View reviewed changes

core/services/relay/evm/mercury/wsrpc/cache/cache_set.go Outdated Show resolved Hide resolved

jmank88 reviewed Nov 28, 2023

View reviewed changes

core/services/relay/evm/mercury/wsrpc/cache/cache.go Outdated Show resolved Hide resolved

samsondav added 6 commits November 28, 2023 10:12

fix

86bb681

Some PR comments

245ab1f

Fix more PR comments

acceab4

PR comments

eac240c

lint

e359f42

samsondav force-pushed the MERC-2486-cache-for-querying-link-eth-prices-in-mercury branch from 23d746a to e359f42 Compare November 28, 2023 15:12

samsondav enabled auto-merge November 28, 2023 15:32

samsondav requested review from cedric-cordenier and jmank88 November 28, 2023 16:44

cedric-cordenier approved these changes Nov 28, 2023

View reviewed changes

samsondav added this pull request to the merge queue Nov 28, 2023

Merged via the queue into develop with commit f47e290 Nov 28, 2023
88 checks passed

samsondav deleted the MERC-2486-cache-for-querying-link-eth-prices-in-mercury branch November 28, 2023 17:01

snehaagni mentioned this pull request Jan 24, 2024

release/2.8.0 -> master #11883

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feed LatestPrice query cache #11326

Feed LatestPrice query cache #11326

samsondav commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

cl-sonarqube-production bot commented Nov 21, 2023

jmank88 Nov 27, 2023 •

edited

Loading

samsondav Nov 27, 2023 •

edited

Loading

jmank88 Nov 27, 2023

cedric-cordenier Nov 27, 2023

jmank88 Nov 27, 2023 •

edited

Loading

cedric-cordenier Nov 27, 2023 •

edited

Loading

jmank88 Nov 27, 2023

cedric-cordenier Nov 27, 2023

samsondav Nov 28, 2023 •

edited

Loading

cedric-cordenier Nov 28, 2023

samsondav Nov 28, 2023

cedric-cordenier Nov 28, 2023

cedric-cordenier Nov 28, 2023

samsondav Nov 28, 2023

cedric-cordenier Nov 28, 2023

cl-sonarqube-production bot commented Nov 28, 2023

Feed LatestPrice query cache #11326

Feed LatestPrice query cache #11326

Conversation

samsondav commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

cl-sonarqube-production bot commented Nov 21, 2023

jmank88 Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

samsondav Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

cedric-cordenier Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsondav Nov 28, 2023 • edited Loading

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

cl-sonarqube-production bot commented Nov 28, 2023

jmank88 Nov 27, 2023 •

edited

Loading

samsondav Nov 27, 2023 •

edited

Loading

jmank88 Nov 27, 2023 •

edited

Loading

cedric-cordenier Nov 27, 2023 •

edited

Loading

samsondav Nov 28, 2023 •

edited

Loading