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.

Sign up for GitHub

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

Jump to bottom

fixing multinode state transition logic by register polling subscription in rpc client #14534

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

fixing multinode state transition logic by register polling subscription in rpc client #14534

wants to merge 9 commits into from
+157 −5

Conversation

huangzhen1997
Copy link
Contributor

@huangzhen1997 huangzhen1997 commented Sep 23, 2024 •
edited
Loading

Description

SubscribeToFinalizedHeads and SubscribeToHeads won’t abort polling subscription after Close
It could cause clients like HeadTracker to keep using subscriptions to unhealthy RPC despite MultiNode's transitioning to an unhealthy state.
DoD: polling subscription must be registered

Also added a patch for subscribeNewHead when polling is enabled.

Tickets:

BCFR-946(https://smartcontract-it.atlassian.net/browse/BCFR-946

@huangzhen1997 huangzhen1997 marked this pull request as ready for review September 24, 2024 13:08
@huangzhen1997 huangzhen1997 requested review from a team as code owners September 24, 2024 13:08
@huangzhen1997 huangzhen1997 requested review from reductionista and removed request for a team September 24, 2024 13:08
@huangzhen1997 huangzhen1997 changed the title Register polling subscription in rpc client fixing multinode state transition logic by register polling subscription in rpc client Sep 24, 2024
dhaidashenko
dhaidashenko reviewed Sep 25, 2024
View reviewed changes
.changeset/moody-rules-agree.md Outdated
"chainlink": patch
---

- register polling subscription logic in rpc client so when node unhealthy new susbcription will be used
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- register polling subscription logic in rpc client so when node unhealthy new susbcription will be used
- register polling subscription to avoid subscription leaking when rpc client gets closed.

core/chains/evm/client/rpc_client_test.go Outdated
require.NoError(t, rpc.Dial(ctx))

ch := make(chan *evmtypes.Head)
_, err := rpc.SubscribeNewHead(tests.Context(t), ch)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you check that the returned subscription is closed?

dhaidashenko
dhaidashenko reviewed Sep 25, 2024
View reviewed changes
core/chains/evm/client/rpc_client_test.go Outdated
@@ -6,6 +6,7 @@ import (
"errors"
"fmt"
"math/big"
_ "net/http/pprof"
Copy link
Collaborator

Choose a reason for hiding this comment

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

debug artifact

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhaidashenko dhaidashenko dhaidashenko left review comments

@reductionista reductionista Awaiting requested review from reductionista reductionista is a code owner automatically assigned from smartcontractkit/foundations

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@huangzhen1997 @dhaidashenko