refactor!: x/slashing missed block window #15580
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // determine if the validator signed the previous block | ||
| previous, err := k.GetMissedBlockBitmapValue(ctx, consAddr, index) | ||
| if err != nil { | ||
| panic(errors.Wrap(err, "failed to get the validator's bitmap value")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
alexanderbez
changed the title
…/cosmos-sdk into bez/2562-refactor-liveness-window
x/slashing/keeper/signing_info.go
Outdated
| // GetMissedBlockBitmapChunk gets the bitmap chunk at the given chunk index for | ||
| // a validator's missed block signing window. | ||
| func (k Keeper) GetMissedBlockBitmapChunk(ctx sdk.Context, addr sdk.ConsAddress, chunkIndex int64) []byte { | ||
| store := ctx.KVStore(k.storeKey) | ||
| bz := k.cdc.MustMarshal(&gogotypes.BoolValue{Value: missed}) | ||
| store.Set(types.ValidatorMissedBlockBitArrayKey(address, index), bz) | ||
| chunk := store.Get(types.ValidatorMissedBlockBitmapKey(addr, chunkIndex)) | ||
| return chunk | ||
| } | ||
|
|
||
| // clearValidatorMissedBlockBitArray deletes every instance of ValidatorMissedBlockBitArray in the store | ||
| func (k Keeper) clearValidatorMissedBlockBitArray(ctx sdk.Context, address sdk.ConsAddress) { | ||
| // SetMissedBlockBitmapChunk sets the bitmap chunk at the given chunk index for | ||
| // a validator's missed block signing window. | ||
| func (k Keeper) SetMissedBlockBitmapChunk(ctx sdk.Context, addr sdk.ConsAddress, chunkIndex int64, chunk []byte) { |
There was a problem hiding this comment.
It looks like these two methods should be unexported. They are only called from within this package. The chunking looks like an implementation detail that should not be part of the keeper's exported API. External callers should be using {Get,Set}MissedBlockBitmapValue.
I also wonder whether these two methods should operate on [types.MissedBlockBitmapChunkSize]byte instead of a plain slice, for a little more compile-time safety. I'm not sure what would go wrong if an incorrect-sized slice was passed.
There was a problem hiding this comment.
Yeah there's no need for them to be public. Updated.
Re; array vs slice, the size of the bytes is not actually MissedBlockBitmapChunkSize, it could be a bit more...depending on the value of the chunk size, the bitmap might need to be padded slightly to accommodate the correct number of int64 words.
If we stick with the current value MissedBlockBitmapChunkSize, I'm pretty sure the bytes will always be the same fixed size, but if we ever change MissedBlockBitmapChunkSize then we'd have to change the array values.
Do you really see a strong motivation for this?
There was a problem hiding this comment.
It seems feasible that the constant chunk size could change.
If there is a programmer mistake where they end up passing an incorrect byte slice, what happens? Would it make sense to add a runtime check that the slice has the correct length?
…/cosmos-sdk into bez/2562-refactor-liveness-window
| // Bitmap value has changed from not missed to missed, so we flip the bit | ||
| // and increment the counter. | ||
| if err := k.SetMissedBlockBitmapValue(ctx, consAddr, index, true); err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| // Bitmap value has changed from missed to not missed, so we flip the bit | ||
| // and decrement the counter. | ||
| if err := k.SetMissedBlockBitmapValue(ctx, consAddr, index, false); err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
Description
Closes: #2562
This PR introduces a refactor into the way we store, represent, and use a validator's missed block bitmap, i.e the rolling window.
Prior, we were storing a single boolean entry for each possible index, where index ranges
[0, SignedBlocksWindow). E.g. if a chain has a window of 32,000, then this means we'd store 32,000 boolean entries per validator! So we treated this like a bitmap, although it wasn't a real bitmap.With this refactor, we use a real bitmap. So each validator has a bitmap where each bit corresponds to a missed or signed block in the rolling window. However, we don't store the entire bitmap as a single entry in state. We instead break up the bitmap into chunks favoring IAVL's storage semantics, offering better write and read performance. These chunks compose the validator's bitmap. So taking our 32,000 block window and a chunk size of 1024 bits, we'd have roughly ~31 chunks/state entries per validator.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change