WS URL can be optional when LogBroadcaster is disabled #14364

huangzhen1997 · 2024-09-06T15:34:49Z

Description

WS URL can be optional (empty string) when LogBroadcaster is disabled. If WS URL was not provided, SubscribeFilterLogs should fail with an explicit error

Tickets:

BCFR-451

…vided

core/chains/evm/config/toml/config.go

dhaidashenko · 2024-09-09T15:53:16Z

core/chains/evm/client/evm_client.go

@@ -20,7 +20,7 @@ func NewEvmClient(cfg evmconfig.NodePool, chainCfg commonclient.ChainConfig, cli
 	var sendonlys []commonclient.SendOnlyNode[*big.Int, RPCClient]
 	largePayloadRPCTimeout, defaultRPCTimeout := getRPCTimeouts(chainType)
 	for i, node := range nodes {
-		if node.SendOnly != nil && *node.SendOnly {
+		if node.WSURL == nil || (node.SendOnly != nil && *node.SendOnly) {


A secondary or SendOnly node has limited functionality. It's only used to broadcast transactions (fire and forget), and we are not running health checks for it.
We should not change the type of the node. Instead, introduce changes to the RPC client.

Moving the enforcement check to Dial() at rpc_client.go

dhaidashenko · 2024-09-09T16:05:54Z

core/chains/evm/client/config_builder.go

@@ -76,18 +85,37 @@ func NewClientConfigs(
 	return chainConfig, nodePoolCfg, nodes, nil
 }

+// verifyLogBroadcasterFlag checks node config and return error if LogBroadcaster enabled but some node missing WSURL
+func verifyLogBroadcasterFlag(nodes []*toml.Node, LogBroadcasterEnabled bool) error {


Config Builder is built for external applications like Atlas, where LogBroadcaster does not exist. The CL Node does not use it.
verifyLogBroadcasterFlag should be moved to CL Node config validation, which is located in toml pkg

Okay I moved the restriction (ws exist when logBroadcaster disabled + http exist when ws missing) to rpc_client.go.

Just making sure, we don't have to worry about the node config validation here (also here), because it's for external users right ?

Not sure why you've moved the config validation to the rpc_client.go, the link I've provided in my previous msg points to core/chains/evm/config/toml/config.go.
The validation must be part of the config validation step to ensure CL Node won't start with the invalid configs

…BCFR-451/make-ws-url-optional

dhaidashenko · 2024-09-10T12:51:09Z

core/chains/evm/client/rpc_client.go

@@ -174,6 +179,7 @@ func NewRPCClient(
 	r.tier = tier
 	r.ws.uri = wsuri
 	r.finalizedBlockPollInterval = finalizedBlockPollInterval
+	r.logBroadcasterEnabled = logBroadcasterEnabled
 	if httpuri != nil {


RPC Client does not need to know about LogBroadcaster, this breaks abstraction

dhaidashenko · 2024-09-10T13:11:13Z

core/chains/evm/client/config_builder.go

@@ -76,18 +85,37 @@ func NewClientConfigs(
 	return chainConfig, nodePoolCfg, nodes, nil
 }

+// verifyLogBroadcasterFlag checks node config and return error if LogBroadcaster enabled but some node missing WSURL
+func verifyLogBroadcasterFlag(nodes []*toml.Node, LogBroadcasterEnabled bool) error {


Not sure why you've moved the config validation to the rpc_client.go, the link I've provided in my previous msg points to core/chains/evm/config/toml/config.go.
The validation must be part of the config validation step to ensure CL Node won't start with the invalid configs

core/config/docs/chains-evm.toml

.changeset/silly-lies-boil.md

jmank88 · 2024-09-13T13:07:38Z

core/chains/evm/client/config_builder.go

-		httpUrl := commonconfig.MustParseURL(*nodeCfg.HTTPURL)
+
+		if nodeCfg.HTTPURL == nil {
+			return nil, fmt.Errorf("node config [%d]: HTTP URL", i)


Suggested change

return nil, fmt.Errorf("node config [%d]: HTTP URL", i)

return nil, fmt.Errorf("node config [%d]: missing HTTP URL", i)

jmank88 · 2024-09-13T13:09:28Z

core/chains/evm/client/rpc_client_test.go

+		httpURL := url.URL{}
+		observedLggr, _ := logger.TestObserved(t, zap.DebugLevel)
+		rpcClient := client.NewRPCClient(observedLggr, url.URL{}, &httpURL, "rpc", 1, chainId, commonclient.Primary, 0, commonclient.QueryTimeout, commonclient.QueryTimeout, "")


Could httpURL be omitted?

Suggested change

httpURL := url.URL{}

observedLggr, _ := logger.TestObserved(t, zap.DebugLevel)

rpcClient := client.NewRPCClient(observedLggr, url.URL{}, &httpURL, "rpc", 1, chainId, commonclient.Primary, 0, commonclient.QueryTimeout, commonclient.QueryTimeout, "")

observedLggr, _ := logger.TestObserved(t, zap.DebugLevel)

rpcClient := client.NewRPCClient(observedLggr, url.URL{}, nil, "rpc", 1, chainId, commonclient.Primary, 0, commonclient.QueryTimeout, commonclient.QueryTimeout, "")

For this specific test it can't, since Dial() will fail due to errWSAndHTTPBothMissing. I can add another test that covers when both WS and HTTPURL are empty

jmank88 · 2024-09-13T13:10:14Z

core/chains/evm/config/toml/config.go

@@ -310,6 +313,12 @@ func (c *EVMConfig) ValidateConfig() (err error) {
 	if len(c.Nodes) == 0 {
 		err = multierr.Append(err, commonconfig.ErrMissing{Name: "Nodes", Msg: "must have at least one node"})
 	} else {
+		if c.LogBroadcasterEnabled != nil {
+			if err = verifyLogBroadcasterFlag(c.Nodes, *c.LogBroadcasterEnabled); err != nil {
+				err = multierr.Append(err, commonconfig.ErrMissing{Name: "Nodes", Msg: "must have at least one ws uri when LogBroadcaster is enabled"})


Suggested change

err = multierr.Append(err, commonconfig.ErrMissing{Name: "Nodes", Msg: "must have at least one ws uri when LogBroadcaster is enabled"})

err = multierr.Append(err, commonconfig.ErrMissing{Name: "Nodes", Msg: "must have at least one node with a WSURL when LogBroadcaster is enabled"})

jmank88 · 2024-09-13T13:12:48Z

core/services/chainlink/config_test.go

+			- logbroadcaster cannot be enabled unless all nodes have WSURL provided
+			- Nodes: missing: must have at least one ws uri when LogBroadcaster is enabled


These seem contradictory. Must all nodes have WSURL, or just one?

Thanks for catching it. According to the requirement, I was intended to enforce Must all nodes have WSURL when LogBroadcaster is enabled, so the second error message is incorrect. But I will double check if the requirement is correct, as it might not backward compatible

Updated the error message. We going to require all primary nodes to provide valid WS URL when LogBroadcaster is enabled.

core/services/chainlink/testdata/config-invalid.toml

dhaidashenko

Could you try starting the CL node locally without WS URL? It seems like in the current version, it should fail with nil pointer deference. Due to this

dhaidashenko · 2024-09-19T12:19:56Z

.changeset/silly-lies-boil.md

+"chainlink": minor
+---
+
+Make websocket URL flag `WSURL` for `EVM.Nodes`, and apply logic so that:


Suggested change

Make websocket URL flag `WSURL` for `EVM.Nodes`, and apply logic so that:

Make websocket URL `WSURL` for `EVM.Nodes` optional, and apply logic so that:

dhaidashenko · 2024-09-19T12:29:16Z

core/chains/evm/client/config_builder.go

-		if nodeCfg.WSURL == nil || nodeCfg.HTTPURL == nil {
-			return nil, fmt.Errorf("node config [%d]: missing WS or HTTP URL", i)
+		var wsURL, httpURL *commonconfig.URL
+		// wsUrl is optional, if LogBroadcaster is enabled, at least one wsURL is required and this will be checked in EVMConfig validation


LogBroadcaster is not relevant to the context of evm/client. It's one of many users, we should not reference it here.

dhaidashenko · 2024-09-19T12:53:45Z

core/chains/evm/client/rpc_client.go

-	lggr := r.rpcLog.With("wsuri", r.ws.uri.Redacted())
-	if r.http != nil {
-		lggr = lggr.With("httpuri", r.http.uri.Redacted())
+	if r.ws.uri.String() == "" && r.http == nil {


WS is optional. Can't we make it a *url.URL instead of url.URL?

We can, but it requires a lot refactor and adding nil ptr check and not sure if we want to do in this PR

dhaidashenko · 2024-09-19T12:54:54Z

core/chains/evm/client/rpc_client.go

+		r.ws.rpc = wsrpc
+		r.ws.geth = ethclient.NewClient(wsrpc)
+	} else {
+		lggr = lggr.With("wsuri", "empty WS URI")


There is no need to include empty was URI into the log

dhaidashenko · 2024-09-19T13:01:48Z

core/chains/evm/client/rpc_client_test.go

@@ -45,6 +45,7 @@ func TestRPCClient_SubscribeNewHead(t *testing.T) {

 	chainId := big.NewInt(123456)
 	lggr := logger.Test(t)
+	var missingBothWSAndHTTPErr = errors.New("cannot dial rpc client when both ws and http info are missing")


We should try minimizing the lifespan of a variable. This one is only used in a single test case. There is no need to define it globally for the testing function.

dhaidashenko · 2024-09-19T13:02:40Z

core/chains/evm/client/rpc_client_test.go

+		require.Nil(t, rpcClient.Dial(ctx))
+
+		_, err := rpcClient.SubscribeFilterLogs(ctx, ethereum.FilterQuery{}, make(chan types.Log))
+		require.Error(t, err)


Could you ensure that we return proper error msg?

dhaidashenko · 2024-09-19T13:03:48Z

core/chains/evm/client/rpc_client.go

@@ -75,6 +75,8 @@ var (
 			float64(8 * time.Second),
 		},
 	}, []string{"evmChainID", "nodeName", "rpcHost", "isSendOnly", "success", "rpcCallName"})
+	errSubscribeFilterLogsNotAllowedWithoutWS = errors.New("SubscribeFilterLogs is not allowed without ws url")


Are we reusing these variables across multiple functions? If not, it might make sense to define them inline or make them public and use in tests

dhaidashenko · 2024-09-19T13:07:14Z

core/chains/evm/config/toml/config.go

-			hasPrimary = true
-			break
+
+			if !hasPrimary {


nit: if statement is redundant as it's fine to set hasPrimary = true if it's already true

dhaidashenko · 2024-09-19T13:07:54Z

core/chains/evm/config/toml/config.go

+
+			// if the node is a primary node, then the WS URL is required when LogBroadcaster is enabled
+			if logBroadcasterEnabled && (n.WSURL == nil || n.WSURL.IsZero()) {
+				err = multierr.Append(err, commonconfig.ErrMissing{Name: "Nodes", Msg: "all primary nodes must provide a valid WSURL when LogBroadcaster is enabled"})


It might be a good idea to specify failed node index

cl-sonarqube-production · 2024-09-19T23:19:40Z

Quality Gate failed

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

See analysis details on SonarQube

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

huangzhen1997 added 9 commits September 5, 2024 15:43

WS URL can be optional

b05a1a7

add changeset

71c84c6

change

e99aae0

make WSURL optional

6737f0c

fix test, and enforce SubscribeFilterLogs to fail when ws url not pro…

8df1659

…vided

add comments

e1fa795

update changeset

7f59b07

update dial logic and make ws optional not required

48c7f1a

fix test

515888c

huangzhen1997 marked this pull request as ready for review September 6, 2024 20:46

huangzhen1997 requested review from a team as code owners September 6, 2024 20:46

huangzhen1997 requested review from reductionista and dhaidashenko and removed request for a team September 6, 2024 20:46

huangzhen1997 added 2 commits September 9, 2024 09:47

fix

9ecc624

fix lint

01e9e08

jmank88 requested changes Sep 9, 2024

View reviewed changes

core/chains/evm/config/toml/config.go Outdated Show resolved Hide resolved

dhaidashenko requested changes Sep 9, 2024

View reviewed changes

dhaidashenko reviewed Sep 9, 2024

View reviewed changes

huangzhen1997 added 10 commits September 9, 2024 12:26

Merge branch 'develop' of github.com:smartcontractkit/chainlink into …

4b860a1

…BCFR-451/make-ws-url-optional

address comments

fdc5c8c

update comments

2660903

fix test

b3e270d

add check when both ws and http missing

ba73ef6

add test and add restrictions

5bec775

add comment

9a841ce

revert outdated change

670ebb8

remove extra line

799dc6c

fix test

427a8a0

dhaidashenko requested changes Sep 10, 2024

View reviewed changes

huangzhen1997 added 4 commits September 10, 2024 09:17

revert changes from rpc client

7738ff0

unintended change

795ee7b

remove unused

3b27171

update verification logic

d3f523c

huangzhen1997 requested a review from dhaidashenko September 10, 2024 16:54

huangzhen1997 added 2 commits September 10, 2024 12:34

add test fix

a71015d

modify unit test to cover logbroadcaster enabled false

7908e2a

huangzhen1997 mentioned this pull request Sep 11, 2024

support new heads polling over http rpc client #14373

Open

jmank88 reviewed Sep 12, 2024

View reviewed changes

core/config/docs/chains-evm.toml Outdated Show resolved Hide resolved

update doc

ae09d35

huangzhen1997 requested review from jmank88 and DylanTinianov September 12, 2024 18:31

jmank88 reviewed Sep 12, 2024

View reviewed changes

.changeset/silly-lies-boil.md Outdated Show resolved Hide resolved

udpate changeset

fe19568

jmank88 reviewed Sep 13, 2024

View reviewed changes

core/services/chainlink/testdata/config-invalid.toml Outdated Show resolved Hide resolved

huangzhen1997 added 6 commits September 13, 2024 11:03

address PR comments

cc05a3f

address pr comments

d7a7719

update invalid toml config

3e92077

fix test

d7af044

ws required for primary nodes when logbroadcaster enabled

4d83e3f

minor

d5dbade

dhaidashenko requested changes Sep 19, 2024

View reviewed changes

huangzhen1997 added 2 commits September 19, 2024 14:36

Dmytro's comments

e8901e7

fix nil ptr, more fix to come

2713252

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WS URL can be optional when LogBroadcaster is disabled #14364

WS URL can be optional when LogBroadcaster is disabled #14364

huangzhen1997 commented Sep 6, 2024 •

edited

Loading

dhaidashenko Sep 9, 2024

huangzhen1997 Sep 9, 2024 •

edited

Loading

dhaidashenko Sep 9, 2024

huangzhen1997 Sep 9, 2024 •

edited

Loading

dhaidashenko Sep 10, 2024

dhaidashenko Sep 10, 2024

dhaidashenko Sep 10, 2024

jmank88 Sep 13, 2024

jmank88 Sep 13, 2024

huangzhen1997 Sep 13, 2024 •

edited

Loading

jmank88 Sep 13, 2024

jmank88 Sep 13, 2024

huangzhen1997 Sep 13, 2024 •

edited

Loading

huangzhen1997 Sep 16, 2024

dhaidashenko left a comment

dhaidashenko Sep 19, 2024

dhaidashenko Sep 19, 2024

dhaidashenko Sep 19, 2024

huangzhen1997 Sep 19, 2024

dhaidashenko Sep 19, 2024

dhaidashenko Sep 19, 2024

dhaidashenko Sep 19, 2024

dhaidashenko Sep 19, 2024

dhaidashenko Sep 19, 2024

dhaidashenko Sep 19, 2024

cl-sonarqube-production bot commented Sep 19, 2024

	return nil, fmt.Errorf("node config [%d]: HTTP URL", i)
	return nil, fmt.Errorf("node config [%d]: missing HTTP URL", i)

	err = multierr.Append(err, commonconfig.ErrMissing{Name: "Nodes", Msg: "must have at least one ws uri when LogBroadcaster is enabled"})
	err = multierr.Append(err, commonconfig.ErrMissing{Name: "Nodes", Msg: "must have at least one node with a WSURL when LogBroadcaster is enabled"})

		- logbroadcaster cannot be enabled unless all nodes have WSURL provided
		- Nodes: missing: must have at least one ws uri when LogBroadcaster is enabled

	Make websocket URL flag `WSURL` for `EVM.Nodes`, and apply logic so that:
	Make websocket URL `WSURL` for `EVM.Nodes` optional, and apply logic so that:

WS URL can be optional when LogBroadcaster is disabled #14364

Are you sure you want to change the base?

WS URL can be optional when LogBroadcaster is disabled #14364

Conversation

huangzhen1997 commented Sep 6, 2024 • edited Loading

Description

Tickets:

Choose a reason for hiding this comment

huangzhen1997 Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huangzhen1997 Sep 9, 2024 • 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

huangzhen1997 Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huangzhen1997 Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaidashenko 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

cl-sonarqube-production bot commented Sep 19, 2024

Quality Gate failed

huangzhen1997 commented Sep 6, 2024 •

edited

Loading

huangzhen1997 Sep 9, 2024 •

edited

Loading

huangzhen1997 Sep 9, 2024 •

edited

Loading

huangzhen1997 Sep 13, 2024 •

edited

Loading

huangzhen1997 Sep 13, 2024 •

edited

Loading