Introduce generalized multi node client #10907

dimriou · 2023-10-11T13:53:40Z

Duplicate #9900, originally drafted by Silas.

common/chains/client should also be moved under common/client in a new PR.

github-actions · 2023-10-11T13:54:01Z

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

common/client/node_fsm.go

dhaidashenko · 2023-10-12T15:49:16Z

common/chains/client/models.go

+	Secondary
+)
+
+func (n NodeTier) String() string {


Is it ok that fields of type NodeTier are only used in logging and no other logic depends on it?

common/client/multi_node.go

common/client/node_fsm.go

dhaidashenko · 2023-10-12T16:57:36Z

common/client/node.go

+}
+
+func (n *node[CHAIN_ID, HEAD, RPC]) String() string {
+	s := fmt.Sprintf("(primary)%s:%s", n.name, n.ws.String())


Should we use client.NodeTier?

common/chains/client/models.go

common/client/node_lifecycle.go

dhaidashenko · 2023-10-12T17:40:52Z

common/client/send_only_node_lifecycle.go

+// verifyLoop may only be triggered once, on Start, if initial chain ID check
+// fails.
+//
+// It will continue checking until success and then exit permanently.


Could you elaborate on why it's ok?

core/chains/evm/client/errors.go

common/client/multi_node.go

dhaidashenko · 2023-10-16T15:24:46Z

common/client/send_only_node.go

+	"github.com/smartcontractkit/chainlink/v2/core/utils"
+)
+
+type SendOnlyClient[


This type is quite confusing:

It does not require implementation to have any kind of SendTransaction

It's not used, as in NewMultiNode we require sendOnlyNodes to use same RPC as nodes

I understand that it just defines methods used by sendOnlyNode, but high number of public types might confuse users of the package. Can we use RPC as single dependency across all implementations? As alternative we can create package documentation explaining which types are essential for the users and which are internals.

I think for now we can unexport this interface so it doesn't pollute the API of the package. In the future though, once we decide on a testing strategy for this package, it will make more sense to have a targeted client API that can be easily mocked instead of having an entire RPC interface.

dhaidashenko · 2023-10-16T17:57:03Z

common/client/send_only_node.go

+
+	String() string
+	// State returns nodeState
+	State() nodeState


It's not possible to inject custom implementation of SendOnlyNode|Node as nodeState is private.
Even if it's not needed, it feels weird

Not sure I understand this. Even if nodeState is changed internally, State() is used by the MultiNode to access the state. Let me know if you have something else on your mind.

During integration of new chain someone might benefit from possibility to create their own implementation of SendOnlyNode or Node and still use it with MultiNode. But it's not possible to create custom implementation in different package as there is no way to implement State() nodeState method as nodeState is private for common/client package.

One way to resolve it is to make SendOnlyNode and Node private types and create instances of them in NewMultiNode using passed RPCs.

Another is to keep nodeState public

Personally, I would go in the exact opposite direction and totally abstract Node from the user. Then they would only have to instantiate the MultiNode and the RPCs. The aim of this package is for the user to use the exact same API as Geth or other clients, without worrying about the complexity of internal node states. The only issue I see with that is that currently Node instantiation is coupled with toml.Node config and it might need some additional changes.

I'm having trouble following this thread, but there are two different concerns here:

Whether or not the method is exported affects who can call it.

Whether or the returned type is exported affects who can implement it.

Isn't it critical for this to be implemented by types outside of this package?

The aim of this package is for the user to use the exact same API as Geth or other clients, without worrying about the complexity of internal node states.

Can you elaborate on this? That doesn't fit my mental model. The Geth API is the thing that we want to abstract away, in order to reuse all the stuff that is not specific to geth's API, with other chain clients. Aren't the 'user's of this package the developers integrating those other chain clients? Do they need to implement this method? Call it?

Maybe there are two separate uses here? Unexported & strongly-typed, and exported string?

Suggested change

State() nodeState

state() nodeState

func (s *sendOnlyNode[CHAIN_ID, RPC]) State() string { return string(s.state()) }

Yes, we do abstract the geth-specific calls into more generic ones, but at the same time, we should aim to abstract the Node as much as possible as well. I see two use cases here:

If a user wants to interact with a chain, instead of utilizing a Geth library or some other chain-specific one, we give them a slightly more generalized version of it that can apply to most chains. We should still aim to provide familiar methods like BlockByHash because the average user just wants to have methods as close as possible to the chain client.

To me the user shouldn't be worried about the internal node lifecycle and ideally about the creation and implementation of the node. The transition should be as simple as: "Instead of using a generalized API that makes direct calls to an RPC, you can pass multiple URLs and still use the same calls and we will take care of how the multi-node spins up nodes and handles them. Ideally, the nodes should be totally abstracted.

One example I can think of is the RTSP team. They want to have a client that interacts with a chain without worrying about a node's lifecycle and state.

By 'abstract away' I mean exclude. Meaning that the multi-node code can be used by any kind of client. This is a separate concern from providing a chain-agnostic API layer for clients to adapt. Each has distinctly different ideas of what a user is. Entangling them comes at a cost.

Ok, I think the problem starts from the fact that the original code exports all these fields and methods we are trying to generalize because they are used in different packages for mocking and testing (perhaps it's also on purpose to allow users to extend?).
So when we transferred everything we kept the same convention, but common/client doesn't have tests on the same package as it requires concrete types, and that means we could unexport everything and let the MultiNode act as the interface with the user.
But the question here is do we want the nodes to be extended by the user or totally abstracted? Also, something to consider is that once we agree on a strategy to test generalized packages we might end up going back to exporting those methods and fields.

core/chains/evm/client/errors.go

common/chains/client/models.go

jmank88 · 2023-10-30T11:01:04Z

common/client/multi_node.go

+	NodeSelectionMode_HighestHead     = "HighestHead"
+	NodeSelectionMode_RoundRobin      = "RoundRobin"
+	NodeSelectionMode_TotalDifficulty = "TotalDifficulty"
+	NodeSelectionMode_PriorityLevel   = "PriorityLevel"


I don't know what the best way to name these is, but let's drop the un-idiomatic underscore?

Suggested change

NodeSelectionMode_HighestHead = "HighestHead"

NodeSelectionMode_RoundRobin = "RoundRobin"

NodeSelectionMode_TotalDifficulty = "TotalDifficulty"

NodeSelectionMode_PriorityLevel = "PriorityLevel"

NodeSelectionModeHighestHead = "HighestHead"

NodeSelectionModeRoundRobin = "RoundRobin"

NodeSelectionModeTotalDifficulty = "TotalDifficulty"

NodeSelectionModePriorityLevel = "PriorityLevel"

Suggested change

NodeSelectionMode_HighestHead = "HighestHead"

NodeSelectionMode_RoundRobin = "RoundRobin"

NodeSelectionMode_TotalDifficulty = "TotalDifficulty"

NodeSelectionMode_PriorityLevel = "PriorityLevel"

HighestHeadNodeSelectionMode = "HighestHead"

RoundRobinNodeSelectionMode = "RoundRobin"

TotalDifficultyNodeSelectionMode = "TotalDifficulty"

PriorityLevelNodeSelectionMode = "PriorityLevel"

Suggested change

NodeSelectionMode_HighestHead = "HighestHead"

NodeSelectionMode_RoundRobin = "RoundRobin"

NodeSelectionMode_TotalDifficulty = "TotalDifficulty"

NodeSelectionMode_PriorityLevel = "PriorityLevel"

HighestHeadSelectionMode = "HighestHead"

RoundRobinSelectionMode = "RoundRobin"

TotalDifficultySelectionMode = "TotalDifficulty"

PriorityLevelSelectionMode = "PriorityLevel"

cl-sonarqube-production · 2023-11-01T11:12:08Z

SonarQube Quality Gate

45.2% Coverage on New Code (is less than 75%)

See analysis details on SonarQube

jmank88 · 2023-11-01T16:02:40Z

core/chains/evm/client/chain_client.go

+
+// TODO-1663: change this to actual ChainID() call once client.go is deprecated.
+func (c *chainClient) ChainID() (*big.Int, error) {
+	//return c.multiNode.ChainID(ctx), nil


Suggested change

//return c.multiNode.ChainID(ctx), nil

dimriou added 3 commits October 11, 2023 16:32

Introduce generalized multi node client

93f79d1

Export EVM RPC client

1d1f647

Unexport clientAPI interface

9aeea74

dimriou requested review from samsondav, prashantkumar1982 and a team as code owners October 11, 2023 13:53

Add BlockDifficulty to mocks

1971d82

dimriou requested a review from dhaidashenko October 11, 2023 15:38

jmank88 reviewed Oct 11, 2023

View reviewed changes

common/client/node_fsm.go Outdated Show resolved Hide resolved

Unexport node state

2234144

dhaidashenko reviewed Oct 12, 2023

View reviewed changes

jmank88 reviewed Oct 14, 2023

View reviewed changes

core/chains/evm/client/errors.go Outdated Show resolved Hide resolved

Rename error parsing

2b800b2

dhaidashenko reviewed Oct 16, 2023

View reviewed changes

dhaidashenko previously approved these changes Oct 16, 2023

View reviewed changes

Nit fixes

d2f9a4e

dimriou dismissed dhaidashenko’s stale review via d2f9a4e October 17, 2023 10:06

Merge branch 'develop' into generalize_multinode

316c838

jmank88 reviewed Oct 17, 2023

View reviewed changes

core/chains/evm/client/errors.go Outdated Show resolved Hide resolved

Rename error classification functions

47a3dbb

jmank88 reviewed Oct 30, 2023

View reviewed changes

common/chains/client/models.go Show resolved Hide resolved

jmank88 reviewed Oct 30, 2023

View reviewed changes

dimriou added 3 commits October 30, 2023 17:08

Update NodeSelection names

f52429e

Merge branch 'develop' into generalize_multinode

679799e

Deprecate StartStopOnce

25f09f7

jmank88 reviewed Nov 1, 2023

View reviewed changes

jmank88 approved these changes Nov 1, 2023

View reviewed changes

dhaidashenko approved these changes Nov 1, 2023

View reviewed changes

prashantkumar1982 approved these changes Nov 1, 2023

View reviewed changes

prashantkumar1982 added this pull request to the merge queue Nov 1, 2023

Merged via the queue into develop with commit 08c9f89 Nov 1, 2023
83 of 84 checks passed

prashantkumar1982 deleted the generalize_multinode branch November 1, 2023 20:56

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

Introduce generalized multi node client #10907

Introduce generalized multi node client #10907

dimriou commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 11, 2023

dhaidashenko Oct 12, 2023

dhaidashenko Oct 12, 2023

dhaidashenko Oct 12, 2023

dhaidashenko Oct 16, 2023

dimriou Oct 17, 2023

dhaidashenko Oct 16, 2023

dimriou Oct 17, 2023

dhaidashenko Oct 17, 2023

dimriou Oct 17, 2023

jmank88 Oct 17, 2023

jmank88 Oct 17, 2023

jmank88 Oct 17, 2023

dimriou Oct 17, 2023 •

edited

Loading

jmank88 Oct 17, 2023

dimriou Oct 17, 2023

jmank88 Oct 30, 2023

cl-sonarqube-production bot commented Nov 1, 2023

jmank88 Nov 1, 2023

Introduce generalized multi node client #10907

Introduce generalized multi node client #10907

Conversation

dimriou commented Oct 11, 2023 • edited Loading

github-actions bot commented Oct 11, 2023

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

dimriou Oct 17, 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

cl-sonarqube-production bot commented Nov 1, 2023

Choose a reason for hiding this comment

dimriou commented Oct 11, 2023 •

edited

Loading

dimriou Oct 17, 2023 •

edited

Loading