Adding gRPC dial capability for mercury #12560

patrickhuie19 · 2024-03-25T06:44:29Z

Summary
This adds a grpc (over HTTP/2) mode to the mercury client, in addition to the currently in production wsrpc mode.
We will need to support both modes for ~4-8 weeks while we learn to operate the service with grpc.

Discussion
After lots of iteration, I chose to implement this change by manually changing the autogenerated proto client so that I could add grpc handlers. I found this to be by far the most efficient way to accomplish this - both generics and introducing a new higher level grpc client result in loads of duplicated and unnecessarily complicated code.

What do you need to review?

Proper test coverage to ensure no regressions to wsrpc behavior
Review the related WSRPC changes: Support wsrpc --> grpc for Mercury wsrpc#59. Includes the update to the autogeneration script
Metadata key naming. The key naming of the KV pairs end up in HTTP headers.

Key material in this PR is for testing only and is fine to be leaked.

github-actions · 2024-03-25T06:44:49Z

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

#added For any new functionality added.
#breaking_change For any functionality that requires manual action for the node to boot.
#bugfix For bug fixes.
#changed For any change to the existing functionality.
#db_update For any feature that introduces updates to database schema.
#deprecation_notice For any upcoming deprecation functionality.
#internal For changesets that need to be excluded from the final changelog.
#nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
#removed For any functionality/config that is removed.
#updated For any functionality that is updated.
#wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

cl-sonarqube-production · 2024-03-25T07:03:17Z

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

patrickhuie19 · 2024-04-02T03:49:53Z

core/services/relay/evm/mercury/wsrpc/fixtures/domain.key

@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----


These are fixtures that are for testing only and are fine to be made public

Same comment than for core/services/ocr2/plugins/mercury/fixtures/domain.key, could we change the name to reflect it is intended for tests?

core/services/ocr2/plugins/mercury/helpers_test.go

patrickhuie19 · 2024-04-22T06:32:44Z

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

 	return &client{
 		csaKey:                     clientPrivKey,
 		serverPubKey:               serverPubKey,
 		serverURL:                  serverURL,
-		logger:                     lggr.Named("WSRPC").Named(serverURL).With("serverURL", serverURL),
+		logger:                     lggr.Named("Client").Named(serverURL),


If there are alerts defined on this logger name its likely best to leave alone

cl-sonarqube-production · 2024-04-24T17:34:29Z

Quality Gate failed

Failed conditions
8 New Major Issues (required ≤ 5)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

samsondav · 2024-04-26T14:03:33Z

core/cmd/shell.go

@@ -159,11 +159,17 @@ func (n ChainlinkAppFactory) NewApplication(ctx context.Context, cfg chainlink.G

 	loopRegistry := plugins.NewLoopRegistry(appLggr, cfg.Tracing())

+	// TOML config + env var protected usage of grpc mercury pool
+	var mercuryTlsCertFile *string
+	if os.Getenv("CHAINLINK_MERCURY_USE_GRPC") == "true" {


Probably better as a config variable

I think this is true now that many of our smoke tests are moving to using TOML.

samsondav · 2024-04-26T14:04:04Z

core/internal/cltest/cltest.go

@@ -533,7 +543,7 @@ func NewEthMocksWithTransactionsOnBlocksAssertions(t testing.TB) *evmclimocks.Cl
 }

 // Start starts the chainlink app and registers Stop to clean up at end of test.
-func (ta *TestApplication) Start(ctx context.Context) error {
+func (ta TestApplication) Start(ctx context.Context) error {


This change seems like a mistake

given how the application interface works in the testing this was the simplest change to not require a lot of code changes. Potentially an opportunity for cleanup?

@jmank88 are there any long term code organization/styling difference son using a pointer receiver vs non-pointer receiver for methods?

This is functionally different and likely breaks tests, no? The linter sees the problem on 548.

samsondav · 2024-04-26T14:04:40Z

core/services/ocr2/plugins/mercury/fixtures/domain.key

+-----BEGIN PRIVATE KEY-----
+MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCswr7N//BPhAkF
+mtvwOge2j536x8IJygZZNccLmOGXG2aj0FI9xD2kBBlzpk+/Ma5WIGbcVgcum49m
+b5+upEY8nmZK74yGluNKUi2Zoiw3f7uBA2EwFI4irfDT3WUGr7N54hdIWi0GgegP
+KjAqkPfNrd3ZP3mJcTfDR0KbgT/Gv65MLHEPkdkh7dHlsdSIRVKkfrAZ+Vr8uSZq
+TZlapwZHn1RaeYdta11zvFwEyRxmHFDNZUpwpr8pk8ytjQwgkIe9lzIWN8esO5An
+7yT9AICEXcumVjvU6DTXOwtexTPfegKN7S4eFU4b5ctY/TLaxws0xbxL+/tAytNf
+qcFou22fAgMBAAECggEAD9723GyOInVExukKb4VFPsuZfc0VfFoSPzvQ2gs6HkGm
+1OxxwbmM2CEwSA7tSz6V1NUL3g4L1tz0+Dok90xMJOGAZZNK25/HqFo9qjGF0mC/
+qa3VIy+gPHtc9nm5ADuVR2CH4bO3EzP396dZNBqL9ASXBDrv6rp7SaClnoI/M6jd
+lreoedH5TxDQ+tS+smmOhOVXYr8H/y3I2Kt4GrcuvZVp7ftL0zcN7q2l4TAvPE/B
+a/t8Vp4kdMAOHMCAGXs4YeQ+kDpgKzUrDrRhpxoC4EBkHZNDcd4ww9Bb9hSFNzFk
+oNZYaB6Lxm7Gw7cVtz6IYCJhrIQEj7J5ddCR3/CesQKBgQDicvOhUaERiyjKgWib
+mzinGkAgPFTupKH+VEFcQww7VuUS7lyRv2LFaUB2iJTKt39zipht1p3ZZDIGdoEL
+J3lMqw3wPMhkhrnu509ma+0XWJG/4mW/tVPfeGLhWQb8FwR41BT2mVCw6jEUpApT
+rjFq5AFPmxR5fx39YRDWkToRmwKBgQDDTjZ+NdtUbalVBTG271z3Hyf1JSwAs806
+H6wcLHhKGZGZPPLYNKv+KFKZiXCJuyDUfAzwsfelNzq0PLfymhnmsYG52YIxVJ5c
+vl2uX9LH853TNwaGDabCpbEj4X7OxTOjp0rwsESpOnEOP8w7HgDvX+3Rrk89HNni
+78yM0neGTQKBgQCYiz87iLWsD8ZmxE4npKTBTJmuHXZJXHYT0cZF1JTE1IB0BEYv
+mHF88q4OK2uwM3ST7OVJ+K1U6H4ahHHhhbHcx84X65pCvc869G80W7fXZj6ouGY6
+gngBriNOEp716ruEebm1dQo6Y0i00txyCoIXs4h+i8V7Ivqc2WpyYFpxyQKBgQCx
+lsTmLoKpWq3GqVpzRWP7MVFcD9jbKqNKXtJZK/aVKnrCJDGNdxeNm4zAH8E8+/L2
+Q3ZgxvKwQHAYw71f09AJfQ7At48y3AbDvjXhATDgsByqmjZMXs1r4M4aGkj8K0Sx
+YsU55o4IcyOqGUjT2IxxHYFaUG2s3Dcjq3kL87TLYQKBgGcBQz2mhwaTAMSpo6n5
+QWtYYmLmE3DCsAH1l0GcgSNYY4ZsZ1066xw7t1BABWScInlkA3BbeQJEYclhVli/
+9cRv+xUORGHQ08btRxLdDKX0qw50LmRJONYjHDPQnUJSxY2pWGi9JBIaE5uMAqBl
+FxIhwpSwH/dcU8gRXU4yllLA
+-----END PRIVATE KEY-----


Did you mean to commit a private key?

Yep, these are just test fixtures.

Can the filename be changed to something like domain_test.key to emphasize this is for tests?

samsondav · 2024-04-26T14:45:28Z

core/services/ocr2/plugins/mercury/helpers_test.go

+	if s.tlsCertFile != nil {
+		md, ok := metadata.FromIncomingContext(ctx)
+		if !ok {
+			return nil, errors.New("could not extract public key from grpc outgoing context")
+		}
+		peerID = md.Get("csa-key")[0]
+		signature := md.Get("signature")[0]
+		require.NoError(s.t, VerifySignature(peerID, req, signature), "signature verification failed")
+	} else {
+		p, ok := peer.FromContext(ctx)
+		if !ok {
+			return nil, errors.New("could not extract public key")
+		}
+
+		peerID = p.PublicKey.String()


This will need audit I think

Yes definitely

… a data race on conn.WaitForReady and dialWsrpc

…ally

timweri · 2024-07-16T22:17:17Z

core/cmd/shell.go

+	var mercuryTlsCertFile *string
+	if os.Getenv("CHAINLINK_MERCURY_USE_GRPC") == "true" {
+		tlsCertFile := cfg.Mercury().TLS().CertFile()
+		mercuryTlsCertFile = &tlsCertFile


Do we need to null check here similar to https://github.com/smartcontractkit/chainlink/pull/12560/files#diff-ca252fec8b919ed465bc5f042afca2b4f6be3f20640d327478983967563fb886R357-R359?

timweri · 2024-07-17T01:35:18Z

core/services/ocr2/plugins/mercury/helpers_test.go

+	ocr2types "github.com/smartcontractkit/libocr/offchainreporting2plus/types"
 	ocrtypes "github.com/smartcontractkit/libocr/offchainreporting2plus/types"


What's the purpose of the duplication here?

timweri · 2024-07-17T02:39:24Z

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

+}
+
+// Adapting Grpc Client Conn to Conn interface
+type AdapatedGrpcClientConn struct {


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

timweri · 2024-07-19T17:35:25Z

We should separate the logic for gRPC and WS more clearly, especially when authentication is involved. Also, we shouldn't use the presence of a config tlsCertFile to decide on the control flow between gRPC and WS. We need something more explicit.

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

patrickhuie19 · 2024-09-06T17:40:35Z

We should separate the logic for gRPC and WS more clearly, especially when authentication is involved.

Do you have ideas?
If its by separating the higher level abstractions, like Pool, I'm very hesitant to revisit that - I've already tried that and the code is an absolute mess. Most maintainers should not even be aware of this low level detail, so the code is isolated about as cleanly as I could make it.

Also, we shouldn't use the presence of a config tlsCertFile to decide on the control flow between gRPC and WS. We need something more explicit.

It also requires the env var CHAINLINK_MERCURY_USE_GRPC

github-actions · 2024-11-12T00:15:31Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

patrickhuie19 temporarily deployed to sdlc March 25, 2024 06:44 — with GitHub Actions Inactive

patrickhuie19 added do not merge and removed do not merge labels Mar 25, 2024

patrickhuie19 commented Apr 2, 2024

View reviewed changes

core/services/ocr2/plugins/mercury/helpers_test.go Show resolved Hide resolved

patrickhuie19 commented Apr 22, 2024

View reviewed changes

patrickhuie19 temporarily deployed to sdlc April 24, 2024 15:36 — with GitHub Actions Inactive

patrickhuie19 force-pushed the feature/BCF-2872-mercury-grpc-dial-2 branch 2 times, most recently from 1b76e7e to 7179712 Compare April 24, 2024 16:17

patrickhuie19 temporarily deployed to sdlc April 24, 2024 16:17 — with GitHub Actions Inactive

patrickhuie19 temporarily deployed to sdlc April 24, 2024 16:53 — with GitHub Actions Inactive

patrickhuie19 temporarily deployed to sdlc April 24, 2024 17:18 — with GitHub Actions Inactive

samsondav reviewed Apr 26, 2024

View reviewed changes

patrickhuie19 marked this pull request as ready for review May 14, 2024 15:11

patrickhuie19 requested a review from a team as a code owner May 14, 2024 15:11

patrickhuie19 requested a review from a team May 14, 2024 15:11

patrickhuie19 requested a review from a team as a code owner May 14, 2024 15:11

patrickhuie19 requested review from jmank88 and obelisk May 14, 2024 15:11

patrickhuie19 added 8 commits June 11, 2024 11:05

Adding grpc dial functionality with manually edited mercury_wsrpc.pb.go

ec8d5e8

Wiring Mercury.TLS.CertFile through to NewPool

daa40ea

WIP: contains non-priveleged key material

302071f

functioning unit test for choosing dial

f554fd1

WIP: working TestIntegration_MercuryV3

4cfc450

moved grpc metadata creation from consumer to within client and fixed…

38da3ac

… a data race on conn.WaitForReady and dialWsrpc

cleaning up mercury integration test file

5b35807

cleanup

cfab7c3

patrickhuie19 added 5 commits June 11, 2024 15:46

Implemented signature scheme for grpc requests

cf7fedc

TestIntegration_MercuryV3 verifies grpc client signatures

cc438a9

Fixing rebase errors. Tested TestIntegration_MercuryV3 still runs loc…

8610e22

…ally

minor cleanup and adding signature to TestIntegration_MercuryGRPC

e6773d3

Adding kv pairs to metadata now only happens if client is in grpc mode

bd6865b

patrickhuie19 force-pushed the feature/BCF-2872-mercury-grpc-dial-2 branch from 55bee64 to bd6865b Compare June 11, 2024 19:52

patrickhuie19 had a problem deploying to sdlc June 11, 2024 19:52 — with GitHub Actions Failure

fixing rebase

53a4ca0

patrickhuie19 temporarily deployed to sdlc June 11, 2024 20:01 — with GitHub Actions Inactive

timweri reviewed Jul 16, 2024

View reviewed changes

timweri reviewed Jul 17, 2024

View reviewed changes

timweri reviewed Jul 19, 2024

View reviewed changes

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

timweri reviewed Jul 19, 2024

View reviewed changes

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

pkcll reviewed Sep 4, 2024

View reviewed changes

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

Merge branch 'develop' into feature/BCF-2872-mercury-grpc-dial-2

e9ba8a1

patrickhuie19 requested review from a team as code owners September 8, 2024 18:08

pkcll self-requested a review September 12, 2024 15:05

github-actions bot added the Stale label Nov 12, 2024

github-actions bot closed this Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding gRPC dial capability for mercury #12560

Adding gRPC dial capability for mercury #12560

patrickhuie19 commented Mar 25, 2024 •

edited

Loading

github-actions bot commented Mar 25, 2024 •

edited

Loading

cl-sonarqube-production bot commented Mar 25, 2024

patrickhuie19 Apr 2, 2024

javuto Jul 11, 2024

patrickhuie19 Apr 22, 2024

cl-sonarqube-production bot commented Apr 24, 2024

samsondav Apr 26, 2024

patrickhuie19 May 14, 2024

samsondav Apr 26, 2024

patrickhuie19 May 14, 2024

jmank88 May 14, 2024

samsondav Apr 26, 2024

patrickhuie19 Apr 26, 2024

javuto Jul 11, 2024

samsondav Apr 26, 2024

timweri Jul 19, 2024

timweri Jul 16, 2024

timweri Jul 17, 2024

timweri Jul 17, 2024

timweri commented Jul 19, 2024

patrickhuie19 commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Nov 12, 2024

		ocr2types "github.com/smartcontractkit/libocr/offchainreporting2plus/types"
		ocrtypes "github.com/smartcontractkit/libocr/offchainreporting2plus/types"

Adding gRPC dial capability for mercury #12560

Adding gRPC dial capability for mercury #12560

Conversation

patrickhuie19 commented Mar 25, 2024 • edited Loading

github-actions bot commented Mar 25, 2024 • edited Loading

cl-sonarqube-production bot commented Mar 25, 2024

Quality Gate failed

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 Apr 24, 2024

Quality Gate failed

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timweri commented Jul 19, 2024

patrickhuie19 commented Sep 6, 2024 • edited Loading

github-actions bot commented Nov 12, 2024

patrickhuie19 commented Mar 25, 2024 •

edited

Loading

github-actions bot commented Mar 25, 2024 •

edited

Loading

patrickhuie19 commented Sep 6, 2024 •

edited

Loading