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

Validators at proposed #555

Merged
merged 17 commits into from
Nov 21, 2024
Merged

Validators at proposed #555

merged 17 commits into from
Nov 21, 2024

Conversation

iansuvak
Copy link
Contributor

@iansuvak iansuvak commented Nov 19, 2024

Why this should be merged

Supersedes #506

How this works

  • Updates avalanchego to v1.11.13 and subnet-evm to v0.6.12
  • Removes fallback calls to getCurrentValidators and replaces height passed into getValidatorsAt from the result of GetCurrentHeight to "proposed" flag.

I'm happy to add the getCurrentValidators fallback as well if it's preferred but assuming that getValidatorsAt(height: "proposed" is available on public API this would be the safest way of fetching validator sets.

How this was tested

Existing tests should pass. These tests fail against previous version of avalanchego with the code modifications here.

How is this documented

@iansuvak iansuvak marked this pull request as ready for review November 19, 2024 15:08
@iansuvak iansuvak requested a review from a team as a code owner November 19, 2024 15:08
Copy link
Contributor

@bernard-avalabs bernard-avalabs left a comment

Choose a reason for hiding this comment

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

Makes sense to me assuming that getValidatorsAt(height: "proposed") is available.

@@ -98,77 +91,14 @@ func (v *CanonicalValidatorClient) GetValidatorSet(
) (map[ids.NodeID]*validators.GetValidatorOutput, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the comment for this function be updated to remove the sentence about falling back to getCurrentValidators?

peers/validators/canonical_validator_client.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@geoff-vball geoff-vball left a comment

Choose a reason for hiding this comment

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

LGTM other than what others have commented

iansuvak and others added 4 commits November 20, 2024 09:13
Copy link
Contributor

@geoff-vball geoff-vball left a comment

Choose a reason for hiding this comment

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

Don't know if we have a testing plan yet? But the code as-is LGTM

@iansuvak
Copy link
Contributor Author

Don't know if we have a testing plan yet? But the code as-is LGTM

This PR would fail E2E if it wasn't working at all. I'll add an explicit E2E test that relies on proposed height != current p-chain height in a separate PR once a PR resolving #534 is up since that one will bring over latest tmpnet changes over from teleporter

@iansuvak iansuvak mentioned this pull request Nov 21, 2024
Copy link

@richardpringle richardpringle left a comment

Choose a reason for hiding this comment

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

LGTM, relying of @geoff-vball's review for the deleted code as I don't have the context there.

@iansuvak iansuvak merged commit 90ad76f into main Nov 21, 2024
8 checks passed
@iansuvak iansuvak deleted the validators_at_proposed branch November 21, 2024 21:28
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