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 3 #2086

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.

Cleanup list:

How this works

writeCurrentStaker is broken into two main parts:

  1. a processCurrentStaker method that loops over stakers diffs and read relevant data
  2. a smaller method that only update in memory validator set

How this was tested

CI

@abi87 abi87 changed the title Pull apart writeCurrentStakers method - Part 2 Pull apart writeCurrentStakers method - Part 3 Sep 22, 2023
@abi87 abi87 marked this pull request as ready for review September 22, 2023 21:27
@abi87 abi87 requested review from joshua-kim and marun September 22, 2023 21:28
@abi87 abi87 self-assigned this Sep 22, 2023
// nothing to do
}

addedDelegatorIterator := NewTreeIterator(validatorDiff.addedDelegators)
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) Why do addedDelegators use an iterator but deletedDelegators do not?

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 has to do with the way we have built iteration over stakers in diffs. Delegators added must be iterated in order hence are stored in a tree; deleted delegators must only be queried and skipped.

status: added,
staker: validatorDiff.validator,
})

outputWeights[key].Amount = weight
if blkKey != nil {
// Record that the public key for the validator is being
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) Why is the variable named outputBlsKey if it storing the 'prior value for the public key'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

outputBlksKey will be used to store bls key diffs. Naming is in analogy with outputWeights

outputValSet[key] = validatorStatusPair{
validator: validatorDiff.validator,
status: validatorDiff.validatorStatus,
outputValSet[key] = stakerStatusPair{
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) Where possible, consider avoiding mixing non-functional (e.g. naming) changes with functional ones.

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 tried to be descriptive with the struct name. stakerStatusPair used hold validators data only, while in this PR it is extended to hold delegators data too. Seems fit to rename the struct

addedDelegatorIterator := NewTreeIterator(validatorDiff.addedDelegators)
defer addedDelegatorIterator.Release()
for addedDelegatorIterator.Next() {
output = append(output, stakerStatusPair{
Copy link
Contributor

Choose a reason for hiding this comment

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

Is addition/deletion of a given delegator dependent on the state of their validator?

Copy link
Contributor Author

@abi87 abi87 Sep 26, 2023

Choose a reason for hiding this comment

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

not at state level. When accepting transactions created delegators, validations enforces that delegators delegate to an existent validator.

@marun
Copy link
Contributor

marun commented Sep 25, 2023

Is this strictly a refactor for readability, or is testability also an eventual concern?

@abi87
Copy link
Contributor Author

abi87 commented Sep 27, 2023

Is this strictly a refactor for readability, or is testability also an eventual concern?

I'd say it's for readability. I am not sure we should unit test these low level changes. I see them as implementation and if we change it, we'd have to change the test too.
On the other hand, we could add specific tests to make sure that all of the relevant quantities are stored. Working on it

@abi87 abi87 force-pushed the make_write_current_stakers_testable_3 branch from 7ad8fd5 to 83c661c Compare September 27, 2023 07:54
abi87 added 23 commits November 9, 2023 01:28
@abi87 abi87 linked an issue Dec 19, 2023 that may be closed by this pull request
@abi87 abi87 requested a review from dhrubabasu as a code owner January 3, 2024 13:17
@abi87
Copy link
Contributor Author

abi87 commented Jan 3, 2024

Closing down since there is no agreement this is a simpler solution than existing one

@abi87 abi87 closed this Jan 3, 2024
@dhrubabasu dhrubabasu deleted the make_write_current_stakers_testable_3 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
Projects
Archived in project
Status: In Review 👀
Development

Successfully merging this pull request may close these issues.

Simplify Stakers handling in P-chain state
4 participants