fix: Non-determinism while jailing #342
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gitferry
changed the title
gitferry
requested review from
KonradStaniec,
samricotta and
SebastianElvis
and removed request for
a team
x/finality/keeper/liveness.go
Outdated
| @@ -21,7 +22,15 @@ func (k Keeper) HandleLiveness(ctx context.Context, height int64) { | |||
| // Iterate over all the finality providers which *should* have signed this block | |||
| // store whether or not they have actually signed it, identify sluggish | |||
| // ones, and apply punishment (TBD) | |||
| // Create a sorted slice of keys for deterministic iteration | |||
| fpPkHexes := make([]string, 0, len(fpSet)) | |||
| for fpPkHex := range fpSet { | |||
There was a problem hiding this comment.
From what I remember the elements in votingPowerBbnBlockHeightStore are ordered by voting power (sorting and saving happens during block tallying)
What do you think about introducing new function to finality keeper like GetVotingPowerTableOrdered that will return something like []*FpWithVotingPower where:
type FpWithVotingPower struct {
fpKeyHex string
votingPower uint64
}
that will just collect values from votingPowerBbnBlockHeightStore into list.
This way:
- we will avoid this intermediate
map[string]uint64steap - we will avoid this additional sorting and use the same order we have in
votingPowerBbnBlockHeightStore
There was a problem hiding this comment.
Good point. I thought the ordering of the iterator is implicit but the benefits are clear if we can assume it is already ordered. Fixed in 68d5397
KonradStaniec
approved these changes
Dec 13, 2024
Lazar955
approved these changes
Dec 13, 2024
|
@KonradStaniec Local tested with #344 and worked. Merging it now |
gitferry
added a commit
that referenced
this pull request
Dec 13, 2024
The voting power table is a map which causes random order of processing jailing events. If more than one fps jailed at one block, non-determinism could happen. The PR fixed this by introducing ordered voting power table which retains the order from interating the `votingPowerBbnBlockHeightStore`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The voting power table is a map which causes random order of processing jailing events. If more than one fps jailed at one block, non-determinism could happen. The PR fixed this by introducing ordered voting power table which retains the order from interating the
votingPowerBbnBlockHeightStore