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

Pull apart writeCurrentStakers method - Part 1 #2074

Closed
wants to merge 75 commits into from

Conversation

abi87
Copy link
Contributor

@abi87 abi87 commented Sep 22, 2023

Why this should be merged

writeCurrentStaker is a bulky method in platformvm state package that process stakers diffs and store them.
This PR begins to break writeCurrentStaker into smaller, easier to grok methods.

Cleanup list:

How this works

writeCurrentStaker is broken into two main parts:

  1. a processCurrentStaker method that loop over stakers diffs and read relevant data
  2. smaller methods that only write blsKeys and weightKeys

How this was tested

CI

@abi87 abi87 marked this pull request as ready for review September 22, 2023 16:33
@abi87 abi87 requested review from joshua-kim and marun September 22, 2023 16:33
@abi87 abi87 self-assigned this Sep 22, 2023
@abi87 abi87 added cleanup Code quality improvement vm This involves virtual machines labels Sep 22, 2023
Comment on lines 1675 to 1678
weightDiffs, blsKeyDiffs, err := s.processCurrentStakers()
if err != nil {
return err
}
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 is the part where stakers-related content is scanned to extract specific information to be stored in different parts of the pchain state

Comment on lines 1684 to 1685
s.writeWeightDiffs(height, weightDiffs),
s.writeBlsKeyDiffs(height, blsKeyDiffs),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are some of methods where we only write the information collected in processCurrentStakers

@abi87 abi87 changed the title Pull apart writeCurrentStakers method Pull apart writeCurrentStakers method - Part 1 Sep 22, 2023
@abi87 abi87 force-pushed the make_write_current_stakers_testable branch from e08c437 to 9b77852 Compare September 23, 2023 12:51
Comment on lines 1767 to 1777
type subnetNodePair struct {
subnetID ids.ID
nodeID ids.NodeID
}

type results struct {
weightDiffs map[subnetNodePair]*ValidatorWeightDiff
blsKeyDiffs map[ids.NodeID]*bls.PublicKey
}

func (s *state) processCurrentStakers() (results, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we improve the naming here? Maybe calculateDiffs or something? processCurrentStakers and results is pretty generic... fwiw I'm not really sure results is needed at all

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 change looks forward to next PRs and tries and mininize diffs.

  1. Process is indeed generic, but in next PR we'll pull out data to update validator set and to store stakers data in the very same method. Hence the generic name. Not sure I can do better
  2. results will be expanded too to include data to update validators set and stakers data to be store in the P-chain

I guess the three parts should be merge altogether. I split them in three in order to ease up diff

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on rename, i would name this something like applyDiffs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Out of popular request, I renamed processCurrentStakers to calculateDiffs and results to `diffs.
We'll generalize naming as we review and merge next steps

@StephenButtolph StephenButtolph added this to the v1.10.12 milestone Sep 26, 2023
@StephenButtolph StephenButtolph modified the milestones: v1.10.17, v1.10.18 Dec 1, 2023
@StephenButtolph StephenButtolph removed this from the v1.10.18 milestone Dec 13, 2023
Copy link

@danlaine danlaine 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 the one comment

@abi87
Copy link
Contributor Author

abi87 commented Jan 3, 2024

Closing down since there is no agreement that this version is simpler to reason about

@abi87 abi87 closed this Jan 3, 2024
@dhrubabasu dhrubabasu deleted the make_write_current_stakers_testable branch January 9, 2024 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code quality improvement vm This involves virtual machines warp
Projects
Archived in project
Status: In Review 👀
Development

Successfully merging this pull request may close these issues.

Simplify Stakers handling in P-chain state
5 participants