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

feat: Add kv for validator BLS key set #242

Merged
merged 2 commits into from
Dec 9, 2022
Merged

Conversation

gitferry
Copy link
Contributor

@gitferry gitferry commented Dec 9, 2022

This PR is to add a kv for the validator BLS key set, i.e., epoch -> repeated(addr, BLS PK, power). Adding this is to support ZoneConcierge module to generate a succinct poof of the sealed epoch.

The validator BLS key set is stored for every epoch upon epoch boundary.

Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

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

Thanks for helping out! No blocker from my side except for two minor bugs

}

// ClearValidatorSet removes the validator BLS set of a given epoch
// TODO: This is called upon the epoch is checkpointed
Copy link
Member

Choose a reason for hiding this comment

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

Will we prune validator sets of finalised epochs? I feel the answer is no since CZ might want to query CZ headers in finalised epochs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I had the same feelings. They should stay in storage for a fairly long period of time. But, a question maybe not for now, are we going to prune it, and how?

var (
valSet []*types.ValidatorWithBlsKey
valSet *types.ValidatorWithBlsKeySet
Copy link
Member

Choose a reason for hiding this comment

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

Is valSet.ValSet initialised here? If not then its value will be nil and L84 will panic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch!

@@ -22,6 +23,12 @@ func BeginBlocker(ctx sdk.Context, k keeper.Keeper, req abci.RequestBeginBlock)

// if this block is the second block of an epoch
epoch := k.GetEpoch(ctx)
if epoch.IsFirstBlock(ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you help me add a comment about the contract for IsFirstBlockOfNextEpoch as our offline discussion? Something like below. Thanks :)

// CONTRACT: IsFirstBlockOfNextEpoch can only be called by the epoching module once upon the first block of a new epoch. Other modules should use IsFirstBlock instead.

valSet *ValidatorWithBlsKeySet
)

for i := 0; i < bm.Len(); i++ {
Copy link
Member

@SebastianElvis SebastianElvis Dec 9, 2022

Choose a reason for hiding this comment

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

Suggested change
for i := 0; i < bm.Len(); i++ {
for i := 0; i < len(ks); i++ {

Here bm.Len() will be 104 while len(ks) will be 100. If iterating over bm.Len() then L30 will panic upon i reaches 100. This PR identifies and fixes this issue. Perhaps we can fix it here directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice one!

@gitferry gitferry merged commit 80c406a into dev Dec 9, 2022
@gitferry gitferry deleted the checkpointing/store-valset-kv branch December 9, 2022 07:11
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.

2 participants