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

[SocketRegistry] get operator socket #863

Merged
merged 7 commits into from
Nov 26, 2024
Merged

Conversation

hopeyen
Copy link
Contributor

@hopeyen hopeyen commented Nov 5, 2024

Why are these changes needed?

Check on-chain socket registry state with indexed operator info

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@hopeyen hopeyen force-pushed the hope/get-operator-socket branch from 3bc6e95 to baa2bbd Compare November 5, 2024 09:12
@@ -223,6 +222,15 @@ func (n *Node) Start(ctx context.Context) error {
return fmt.Errorf("failed to register the operator: %w", err)
}
} else {
registeredSocket, err := n.Transactor.GetOperatorSocket(ctx, n.Config.ID)
// Error out if registration on-chain is a requirement
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that we can replace indexed operator info after testing here?

@hopeyen hopeyen force-pushed the hope/get-operator-socket branch from 1360bc8 to f7c7d8d Compare November 5, 2024 09:52
Copy link
Collaborator

@mooselumph mooselumph left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

Looks like we're adding these checks to ensure there's no discrepancy.
Are these check some temporary measure or going to stay here forever?


// check operator socket registration against the indexed state
for operatorID, operatorInfo := range operators {
socket, err := s.chainState.GetOperatorSocket(context.Background(), currentBlock, operatorID)
Copy link
Contributor

Choose a reason for hiding this comment

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

will this make extra N RPC calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there's no multicast call available from the contract unfortunately

Copy link
Contributor

Choose a reason for hiding this comment

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

@pschork how often is scanOperatorsHostInfo called? I believe we have some cronjob?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's called every 12hrs, but the dataAPI is public, so anyone can call it on demand

Copy link
Contributor Author

@hopeyen hopeyen Nov 26, 2024

Choose a reason for hiding this comment

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

🤔 should we add authentication to this method so not everyone can call it? (for future improvement)

@@ -619,3 +640,9 @@ func (t *Reader) GetReservationWindow(ctx context.Context) (uint32, error) {
// contract is not implemented yet
return 0, nil
}

func (t *Reader) GetOperatorSocket(ctx context.Context, operatorId core.OperatorID) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

whenever we call this, should we check if the string is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 I don't think the check is necessary, we will return whatever the contract returns with an empty string. Although this is a local optimization we can make, if we expect the input operator id is ever empty?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm should we just place that check in this method? i.e. return error if result is empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the result is empty, then the check against the expected operator socket would fail. Is there a reason we want to handle it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to check for empty string here

node/node.go Outdated
registeredSocket, err := n.Transactor.GetOperatorSocket(ctx, n.Config.ID)
// Error out if registration on-chain is a requirement
if err != nil {
n.Logger.Warn("failed to get operator socket: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Warnf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

node/node.go Outdated
n.Logger.Warn("failed to get operator socket: %w", err)
}
if registeredSocket != socket {
n.Logger.Warn("registered socket %s does not match expected socket %s", registeredSocket, socket)
Copy link
Contributor

Choose a reason for hiding this comment

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

Warnf

@hopeyen
Copy link
Contributor Author

hopeyen commented Nov 12, 2024

Are these check some temporary measure or going to stay here forever?

I think the checks are temporary and should be removed once we are confident we don't need the indexed operator state for the socket

@hopeyen hopeyen force-pushed the hope/get-operator-socket branch from dcca6c2 to 7714367 Compare November 25, 2024 17:37
@hopeyen hopeyen merged commit f6732a5 into master Nov 26, 2024
9 checks passed
@hopeyen hopeyen deleted the hope/get-operator-socket branch November 26, 2024 20:34
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.

4 participants