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 height #506

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Validators at height #506

wants to merge 17 commits into from

Conversation

iansuvak
Copy link
Contributor

@iansuvak iansuvak commented Oct 2, 2024

Why this should be merged

Closes #501 as well as adds the proposerHeightCache to get a best guess height for which to request validators.

How this works

  • Accepts pchain height as an argument to createSignedMessage.
  • Adds proposerHeightCache that updates itself on a set interval

How this was tested

  • Adds new unit tests for proposerHeightCache
  • existing tests should pass

How is this documented

Comments

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.

A few minor questions mostly to improve my own understanding.

peers/app_request_network.go Outdated Show resolved Hide resolved
peers/validators/canonical_validator_client.go 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.

I didn't look over the cache implementation too hard, but looks good. The rest LGTM aside from existing comments

@iansuvak iansuvak self-assigned this Oct 11, 2024
logger logging.Logger
pChainClient *validators.CanonicalValidatorClient
// protected by timeToHeightLock
timeToHeight *linked.Hashmap[time.Time, uint64]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't need to be a hashmap. Should look into a dequeue.

height uint64,
subnetID ids.ID,
) (*ConnectedCanonicalValidators, error) {
var err error

Choose a reason for hiding this comment

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

no need to set a new variable here instead of usual err := assignment, will only pass line 231 if err = nil.

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'm overriding the passed in height if it's 0 so I do need the err defined prior to that so that line 232 is pure assignment to already declared vars.

I could declare a new variable to replace height instead but I think this is cleaner

Also hi 😄

@iansuvak iansuvak marked this pull request as ready for review October 15, 2024 18:40
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.

Construct signatures for a specific p-chain height
5 participants