Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

latest finalized block metrics #12339

Merged

Conversation

dhaidashenko
Copy link
Collaborator

@dhaidashenko dhaidashenko commented Mar 7, 2024

Added metric to track latest finalized block observed by an RPC.
TIcket has more details.

This PR only introduces metric and does not include latest finalised block into health assessment of an RPC.
We plan to introduce it separately. Here is ticket for tracking

dhaidashenko and others added 30 commits February 19, 2024 17:37
# Conflicts:
#	common/client/mock_rpc_test.go
…smartcontractkit/chainlink into feature/BCI-2649-latest-finalized-block
…smartcontractkit/chainlink into feature/BCI-2649-latest-finalized-block
handle corner case for multiple uncle blocks at the end of the slice
FinalizedBlockPollInterval() time.Duration
}

type ChainConfig interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help me reason through why this configuration belongs to ChainConfig vs NodeConfig or if ChainConfig is living in the correct place (vs in MultiNode)?

I'd expect configuration here to be node specific and for chain details to live at a higher level in the abstraction hierarchy, or for node to store a map[string]*ChainConfig to support multiple chain configurations

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FinalizedBlockPollInterval belongs to NodeConfig and defines how often a Node instance should poll for a new finalized block. It's not a property of a chain; it's a property of the component that performs the health assessment of an RPC.

Node is responsible for the health assessment of a single RPC that works only with one chain. Node does not store map[string]*ChainConfig as there is no reason for it to be aware of other chain configurations.

@@ -43,6 +44,14 @@ type NodeConfig interface {
SelectionMode() string
SyncThreshold() uint32
NodeIsSyncingEnabled() bool
FinalizedBlockPollInterval() time.Duration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering, do we need different configs for each type of poll?
What if we just reuse the PollInterval for all polling?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some chains, we may also look for new heads via polling, not via subscribe.
In that case too, we wouldn't like to poll separately for new heads and new finalized heads. Mostly just make a single batch call to get both.
So that's why I am thinking, could we club all things to be polled under a same config, and fetch them together in a batch call?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a similar impression, it would be more efficient to batch whenever we can instead of introducing a new ticker that will increase pressure on the RPC. AFAIK we already poll RPC to verify if it's healthy so probably we could use that logic for adding latest finalized block. If we manage to bundle that together into a single batch then we will get finality tracking for free

Maybe reusing existing <-pollCh?

	for {
		select {
		case <-n.nodeCtx.Done():
			return
		case <-pollCh:

Copy link
Collaborator Author

@dhaidashenko dhaidashenko Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not in favour of merging all of the polls into a single ticker, as they check different properties of the RPC.
Poll checks that RPC is reachable, this is super basic check and we want to do it often and be aggressive with the timeouts, if an RPC needs > 1s to return it's version, it not healthy, while for finalized block it seems ok.

Regarding the new heads polling, it makes sense to batch poll in this case.

var pollFinalizedHeadCh <-chan time.Time
if n.nodePoolCfg.FinalizedBlockPollInterval() > 0 {
lggr.Debugw("Finalized block polling enabled")
pollT := time.NewTicker(n.nodePoolCfg.FinalizedBlockPollInterval())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't the NewTicker() panic if the parameter is 0?
I think in the fallback file, you should choose a default same as Eth mainnet, that's maybe 5 seconds?
Also, in config validation, ensure that this value is more than 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't the NewTicker() panic if the parameter is 0?

Yes, the NewTicker panics if the parameter is 0, that's why we do not initialize it unless provided value is > 0

I think in the fallback file, you should choose a default same as Eth mainnet, that's maybe 5 seconds?

Sounds good.

Also, in config validation, ensure that this value is more than 0.

IMHO, it should be possible to disable the check. Other health checks are optional, I do not see why this one should be an exception

Copy link
Contributor

@amit-momin amit-momin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I haven't been too involved in multinode changes but the config builder and chain client changes look good.

@prashantkumar1982 prashantkumar1982 added this pull request to the merge queue Mar 26, 2024
Merged via the queue into develop with commit 96d2fe1 Mar 26, 2024
104 checks passed
@prashantkumar1982 prashantkumar1982 deleted the feature/BCI-2663-rpc-metrics-for-finalized-block branch March 26, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants