Skip to content

Conversation

@mattfoley8
Copy link
Contributor

No description provided.

@mattfoley8 mattfoley8 requested a review from a team as a code owner May 22, 2023 19:43
@mattfoley8 mattfoley8 self-assigned this May 22, 2023
}

// Retrieve top, active validators ordered by stake.
validatorEntries, err := bav.GetTopActiveValidatorsByStake(int(bav.Params.LeaderScheduleMaxNumValidators))
Copy link
Member

Choose a reason for hiding this comment

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

Never use int. Always be clear about the size of the int by using int32 or int64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in: #540.

}

// Retrieve top, active validators ordered by stake.
validatorEntries, err := bav.GetTopActiveValidatorsByStake(int(bav.Params.LeaderScheduleMaxNumValidators))
Copy link
Member

Choose a reason for hiding this comment

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

TODO: Need to make sure that this function returns the top validators in a deterministic order. If it has any non-determinism in it, then nodes will have different leader schedules.

Copy link
Contributor Author

@mattfoley8 mattfoley8 May 30, 2023

Choose a reason for hiding this comment

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

I updated the tests so that they run GenerateLeaderSchedule() 10x times for each CurrentRandomSeedHash. It generates the same LeaderSchedule each time. I then repeat that process with 7x different CurrentRandomSeedHashes, generating different LeaderSchedules each time. With those tests, I'm fairly confident in this algorithm's determinism.

// Take RandomUint256 modulo TotalStakeAmountNanos.
// For each ValidatorEntry:
// Skip if ValidatorPKID has already been added to the leader schedule.
// If ValidatorEntry.TotalStakeAmountNanos >= RandomUint256:
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment is incorrect. You're tracking the SUM of the TotalStakeAmountNanos.

}

// Take RandomUint256 % TotalStakeAmountNanos.
randomUint256 := uint256.NewInt().Mod(currentRandomSeedHash.ToUint256(), totalStakeAmountNanos)
Copy link
Member

Choose a reason for hiding this comment

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

TODO: Make sure Mod is efficient. I'm almos certain it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uint256.Mod() uses uint256.QuoRem() under the hood which looks to me like it's O(1). Link to Mod code. Link to QuoRem code.

}
}

return leaderSchedule, nil
Copy link
Member

Choose a reason for hiding this comment

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

Seems pretty clean. Good work!

Base automatically changed from mf/add-solorand-random-seed to feature/proof-of-stake May 30, 2023 14:50
@mattfoley8 mattfoley8 merged commit bd3b99c into feature/proof-of-stake May 30, 2023
@mattfoley8 mattfoley8 deleted the mf/generate-pos-leader-schedules branch May 30, 2023 16:04
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