fix: HandleRewarding gaps of unfinalized blocks
#378
Conversation
RafilxTenfen
changed the title
HandleRewarding gaps of unfinalized blocks
| @@ -30,7 +31,7 @@ func (k Keeper) HandleRewarding(ctx context.Context, targetHeight int64) { | |||
| panic(err) | |||
| } | |||
| if !block.Finalized { | |||
| break | |||
| continue | |||
There was a problem hiding this comment.
hmm the only concern I would have is that in case finality lags behind many blocks, this operation would be quite expensive i.e if we have 100 unfinalized blocks this will iterate over all of them.
This can be a bit expensive. Of course if we are lagging 100 blocks it means something is happening already so maybe this non issue?
In our case of all jailed fps, where there was no finalization for 5k blocks , this would iterate over all of them.
Wdyt @RafilxTenfen @gitferry ?
There was a problem hiding this comment.
yeap, that is true
but it would be updated to the closest finalized block height once we have it
There was a problem hiding this comment.
Good point. It would mitigate the issue if we increase nextHeightToReward regardless block finalization (see commit 10b90f8). If we do so, it means that we will never reward blocks back if they are not finalized. I think it also make sense to not reward those late votes rewardless block finalization
There was a problem hiding this comment.
Good point. It would mitigate the issue if we increase nextHeightToReward regardless block finalization (see commit 10b90f8). If we do so, it means that we will never reward blocks back if they are not finalized. I think it also make sense to not reward those late votes rewardless block finalization
Hmm this seems like protocol change (not rewarding those late votes) it definitely should not be done in this bug fix pr.
There was a problem hiding this comment.
Agreed. Let's merge this pr first and record this as an issue. Then bring it to the team for better visibility. Imo, we should make decision/implementation quick as it would cause breaking change
Prior to this fix, if we had an unfinalized block it would stuck to it as a next block to be rewarded By using `continue` instead of `break`, if there is any finalized block in the interval (`nextBlockToBeRewarded` and `targetHeight`) it will give out rewards and update the next block to be rewarded
Prior to this fix, if we had an unfinalized block it would stuck to it as a next block to be rewarded
By using
continueinstead ofbreak, if there is any finalized block in the interval (nextBlockToBeRewardedandtargetHeight) it will give out rewards and update the next block to be rewarded