Add LOOP-compatible Head Reporter by reductionista · Pull Request #17580 · smartcontractkit/chainlink

reductionista · 2025-05-05T18:47:57Z

Description

Presently, we only have an evm implementation (headreporter.telemetryReporter) of HeadReporter based on the legacy EVM chains.

This re-names headreporter.telemetryReporter to headreporter.evmTelemetryReporter and adds headreporter.loopTelemetryReporter, a chain-agnostic implementation of HeadReporter.

This should work for any chains that fully implement the Relayer interface (specifically LatestHead) since they are all required to satisfy the same interface. We don't want to replace the legacy EVM implementation yet with it for EVM, because that would remove reporting on finalized heads.

Adding support for finalized heads later will require extending the relayer interface. To avoid removing any functionality from EVM, this will run side-by-side the legacy EVM implementation, which can be removed once we add LatestFinalizedHead (or similar) to the relayer interface. There will be some extra reporting on latest heads for evm now, but these are cached in memory so it shouldn't add any overhead.

Copilot

Pull Request Overview

This PR extends the telemetry reporting functionality by renaming the legacy EVM telemetry reporter and adding a new Solana telemetry reporter implementation. Key changes include:

Renaming the EVM telemetry reporter to EVMTelemetryReporter and updating related tests.
Introducing the SolanaTelemetryReporter, along with associated report functions and tests.
Updating wiring in the application to accommodate multiple telemetry reporters for different chain types.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
core/utils/safe_math.go & safe_math_test.go	Added helper function and tests for safely converting int64 to uint64.
core/services/headreporter/telemetry_reporter.go	Renamed telemetry reporter for EVM, and added Solana telemetry reporter implementation.
core/services/headreporter/telemetry_reporter_test.go	Updated tests to reflect the renaming and added tests for Solana telemetry reporting.
core/services/chainlink/relayer_chain_interoperators.go	Updated inline documentation to reflect the correct filtering function.
core/services/chainlink/application.go	Integrated both EVM and Solana telemetry reporters into the application wiring.

Comments suppressed due to low confidence (1)

core/services/chainlink/application.go:480

[nitpick] Consider renaming 'legacyEVMTelemReporter' to 'legacyEVMTelemetryReporter' to improve clarity and maintain consistent naming conventions.

legacyEVMTelemReporter := headreporter.NewEVMTelemetryReporter(telemetryManager, globalLogger, evmChainIDs...)

Copilot · 2025-05-06T10:32:45Z

 	}
-	telemReporter := headreporter.NewTelemetryReporter(telemetryManager, globalLogger, chainIDs...)
-	headReporter := headreporter.NewHeadReporterService(opts.DS, globalLogger, promReporter, telemReporter)
+	solanaRelayers, _ := relayChainInterops.List(FilterRelayersByType("Solana")).GetIDToRelayerMap()


Ignoring the error returned from GetIDToRelayerMap may lead to unhandled failure cases; consider checking and handling the error appropriately.

Suggested change

solanaRelayers, _ := relayChainInterops.List(FilterRelayersByType("Solana")).GetIDToRelayerMap()

solanaRelayers, err := relayChainInterops.List(FilterRelayersByType("Solana")).GetIDToRelayerMap()

if err != nil {

return nil, errors.Wrap(err, "failed to get Solana relayers map")

}

Yeah we should probably handle this error here

It seemed a bit silly to handle it when the function always returns nil. I guess I'll just update the signature so that it doesn't look like it can return an error.

prashantkumar1982 · 2025-05-06T16:26:50Z

+}
+
+// ReportNewHead is unimplemented on Solana because there is no Headtracker to subscribe to
+func (t *solanaTelemetryReporter) ReportNewHead(_ context.Context, _ *evmtypes.Head) error {


Don't like the fact that this method still needs an evmtypes.Head, even though this is part of the interface for Solana.
Seems easy to change to a generic head, but perhaps lets do it in a separate PR later.

Yeah, I considered making that change, but I think this ReportNewHead method will need to be removed soon anyway. The only reason EVM is able to use ReportNewHead (registered callback) rather than PeriodicReport (polling) is because it's not running in LOOP. I don'tt hink non-evm chains would ever be able to use it unless we changed the way our LOOP arch is set up so that relayers could make callbacks to the core node.

prashantkumar1982 · 2025-05-06T16:37:40Z

+	return nil
+}
+
+type solanaTelemetryReporter struct {


nice work!
I see there's literally nothing solana specific here.
What do you think about renaming it to genericTelemReporter, and enable it for all chain families, perhaps except EVM, or maybe don't even need to filter out EVM.
This will just add more telem reporting of EVM Heads at periodic intervals too. I already checked, in EVM relayer's LatestHead(), we return a cached version of head, so no extra RPCs are made.

No other non-evm is launched on CCIP yet, so this change would be no-op in production till they launch.
I only see Aptos and Tron DF being live that would start reporting these Heads, but not a bad thing.
What do you think?

Yeah, I guess we could do that.

I was mostly thinking that the amount of QA testing needed might be a lot less if we know the change only affects Solana. But you have a good point, I don't think LatestHead is implemented for any families but EVM & Solana so the other chains shouldn't matter. And if it's cached on EVM it shouldn't make much of a difference there and can be tested simultaneously with Solana on staging.

prashantkumar1982 · 2025-05-06T16:46:05Z

+	if i < 0 {
+		i = 0
+	}
+	return uint64(i) //nolint:gosec


Did you get any benefit by creating this, then, if you still get this lint warning?
I would say, since we know we are converting from a positive number, lets just ignore this warning

The benefit was that, there were at least 6 different lines where I had to add //nolint:gosec to get CI to pass... now it's contained to just a single location. Several times I thought I'd gotten all of them and then the CI would fail again.

Next time I'm making an update to chainlink-common I'd like to move it there, and re-use it also in LogPoller since that is also filled with a lot of the same //nolint:gosec's

Also, it seems slightly safer in that... if somehow there were a situation where a negative block number were passed it wouldn't accidentally get converted into a positive number. And if there were a clock that was set prior to 1970 it would cause less unpredictable behavior.

Renames: - NewEVMTelemetry -> NewLegacyEVMTelemetry - NewPrometheusReporter -> NewLegacyEVMPrometheusReporter - evmTelemetryReporter -> legacyEVMTelemetryReporter - NewSolanaTelemetryReporter -> NewTelemetryReporter - solanaTelemetryReporter -> loopTelemetryReporter Now, NewTelemetryReporter handles all relayers in the same way (including EVM). The legacy EVM Telemetry Reporter is left operational so we still receive metrics about finalized heads. This can be removed once we extend the relayer interface to be able to fetch finalized heads from there.

cl-sonarqube-production · 2025-05-06T22:24:08Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

* Add Solana Head Reporter * Add explanation to //golint:nosec * Address PR comments Renames: - NewEVMTelemetry -> NewLegacyEVMTelemetry - NewPrometheusReporter -> NewLegacyEVMPrometheusReporter - evmTelemetryReporter -> legacyEVMTelemetryReporter - NewSolanaTelemetryReporter -> NewTelemetryReporter - solanaTelemetryReporter -> loopTelemetryReporter Now, NewTelemetryReporter handles all relayers in the same way (including EVM). The legacy EVM Telemetry Reporter is left operational so we still receive metrics about finalized heads. This can be removed once we extend the relayer interface to be able to fetch finalized heads from there. * Remove unused error output from GetIDToRelayerMap()

reductionista force-pushed the feature/NONEVM-1642-solana-headreport branch 3 times, most recently from c7cba34 to 0a6f1f5 Compare May 6, 2025 02:11

reductionista marked this pull request as ready for review May 6, 2025 03:41

reductionista requested review from a team as code owners May 6, 2025 03:41

reductionista requested a review from ilija42 May 6, 2025 03:41

reductionista force-pushed the feature/NONEVM-1642-solana-headreport branch 3 times, most recently from 4f9eb45 to 0959d71 Compare May 6, 2025 04:25

Add Solana Head Reporter

127520e

reductionista force-pushed the feature/NONEVM-1642-solana-headreport branch from 0959d71 to 127520e Compare May 6, 2025 04:33

sebawo requested a review from Copilot May 6, 2025 10:32

Copilot AI reviewed May 6, 2025

View reviewed changes

prashantkumar1982 reviewed May 6, 2025

View reviewed changes

reductionista added 2 commits May 6, 2025 11:20

Add explanation to //golint:nosec

d57c181

reductionista requested review from a team as code owners May 6, 2025 19:34

reductionista requested review from 0xAustinWang, asoliman92, dimkouv, makramkd, matYang, mateusz-sekara and winder May 6, 2025 19:34

reductionista force-pushed the feature/NONEVM-1642-solana-headreport branch 2 times, most recently from 08872b1 to 1200f29 Compare May 6, 2025 21:19

reductionista force-pushed the feature/NONEVM-1642-solana-headreport branch from 1200f29 to 6040248 Compare May 6, 2025 22:09

Remove unused error output from GetIDToRelayerMap()

04b7ccb

reductionista force-pushed the feature/NONEVM-1642-solana-headreport branch from 6040248 to 04b7ccb Compare May 6, 2025 22:14

prashantkumar1982 approved these changes May 6, 2025

View reviewed changes

jmank88 approved these changes May 6, 2025

View reviewed changes

DylanTinianov approved these changes May 7, 2025

View reviewed changes

reductionista enabled auto-merge May 7, 2025 15:36

winder approved these changes May 7, 2025

View reviewed changes

krehermann approved these changes May 7, 2025

View reviewed changes

reductionista added this pull request to the merge queue May 7, 2025

Merged via the queue into develop with commit 63f9194 May 7, 2025
137 of 138 checks passed

reductionista deleted the feature/NONEVM-1642-solana-headreport branch May 7, 2025 17:48

reductionista changed the title ~~Add Solana Head Reporter~~ Add LOOP-compatible Head Reporter May 7, 2025

reductionista mentioned this pull request May 7, 2025

Add Loop-compatible Head Reporter for Solana (#17580) #17627

Closed

-	solanaRelayers, _ := relayChainInterops.List(FilterRelayersByType("Solana")).GetIDToRelayerMap()
+	solanaRelayers, err := relayChainInterops.List(FilterRelayersByType("Solana")).GetIDToRelayerMap()
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to get Solana relayers map")
+	}

Conversation

reductionista commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cl-sonarqube-production Bot commented May 6, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

reductionista commented May 5, 2025 •

edited

Loading