finality: improve tallying algorithm when many blocks are not finalis…

…ed (#97)

x/finality/keeper/tallying.go

@@ -16,68 +16,60 @@ import (
 // - non-finalisable blocks (i.e., block with no active validator)
 // but without block that has validator set AND does not receive QC
 func (k Keeper) TallyBlocks(ctx sdk.Context) {
- // blocksToFinalize is the set of blocks to finalise within this tallying attempt
- blocksToFinalize := []*types.IndexedBlock{}
- // valSets is the BTC validator set at each height with a non-finalised block
- valSets := map[uint64]map[string]uint64{}
-
  activatedHeight, err := k.BTCStakingKeeper.GetBTCStakingActivatedHeight(ctx)
  if err != nil {
  // invoking TallyBlocks when BTC staking protocol is not activated is a programming error
  panic(fmt.Errorf("cannot tally a block when the BTC staking protocol hasn't been activated yet, current height: %v, activated height: %v",
  ctx.BlockHeight(), activatedHeight))
  }
 
- // find all blocks that are non-finalised AND have validator set, from latest to the earliest activated height
+ // start finalising blocks since max(activatedHeight, nextHeightToFinalize)
+ startHeight := k.getNextHeightToFinalize(ctx)
+ if startHeight < activatedHeight {
+ startHeight = activatedHeight
+ }
+
+ // find all blocks that are non-finalised AND have validator set since max(activatedHeight, lastFinalizedHeight+1)
  // There are 4 different scenarios as follows
- // - has validators, non-finalised: can finalise, add to blocksToFinalize
- // - does not have validators, non-finalised: non-finalisable, skip
- // - has validators, finalised, break here
+ // - has validators, non-finalised: tally and try to finalise
+ // - does not have validators, non-finalised: non-finalisable, continue
+ // - has validators, finalised: impossible to happen, panic
  // - does not have validators, finalised: impossible to happen, panic
  // After this for loop, the blocks since earliest activated height are either finalised or non-finalisable
- blockRevIter := k.blockStore(ctx).ReverseIterator(sdk.Uint64ToBigEndian(uint64(activatedHeight)), nil)
- for ; blockRevIter.Valid(); blockRevIter.Next() {
- // get the indexed block
- ibBytes := blockRevIter.Value()
- var ib types.IndexedBlock
- k.cdc.MustUnmarshal(ibBytes, &ib)
+ for i := startHeight; i <= uint64(ctx.BlockHeight()); i++ {
+  ib, err := k.GetBlock(ctx, i)
+ if err != nil {
+  panic(err) // failing to get an existing block is a programming error
+ }
+
  // get the validator set of this block
  valSet := k.BTCStakingKeeper.GetVotingPowerTable(ctx, ib.Height)
 
  if valSet != nil && !ib.Finalized {
- // has validators, non-finalised: can finalise, add block and valset
- blocksToFinalize = append(blocksToFinalize, &ib)
- valSets[ib.Height] = valSet
+ // has validators, non-finalised: tally and try to finalise the block
+ voterBTCPKs := k.GetVoters(ctx, ib.Height)
+ if tally(valSet, voterBTCPKs) {
+ // if this block gets >2/3 votes, finalise it
+ k.finalizeBlock(ctx, ib, voterBTCPKs)
+ } else {
+ // if not, then this block and all subsequent blocks should not be finalised
+ // thus, we need to break here
+ break
+ }
  } else if valSet == nil && !ib.Finalized {
- // does not have validators, non-finalised: not finalisable, skip
+ // does not have validators, non-finalised: not finalisable,
+ // increment the next height to finalise and continue
+ k.setNextHeightToFinalize(ctx, ib.Height+1)
  continue
  } else if valSet != nil && ib.Finalized {
  // has validators and the block has finalised
- // this means that the entire prefix has been finalised, break here
- break
+ // this can only be a programming error
+ panic(fmt.Errorf("block %d is finalized, but last finalized height in DB does not reach here", ib.Height))
  } else if valSet == nil && ib.Finalized {
  // does not have validators, finalised: impossible to happen, panic
  panic(fmt.Errorf("block %d is finalized, but does not have a validator set", ib.Height))
  }
  }
- // closing the iterator right now before finalising the finalisable blocks
- // this is to follow the contract at https://github.com/cosmos/cosmos-sdk/blob/v0.47.4/store/types/store.go#L239-L240
- blockRevIter.Close()
-
- // for each of these blocks from earliest to latest, tally the block w.r.t. existing votes
- for i := len(blocksToFinalize) - 1; i >= 0; i-- {
- blockToFinalize := blocksToFinalize[i]
- voterBTCPKs := k.GetVoters(ctx, blockToFinalize.Height)
- valSet := valSets[blockToFinalize.Height]
- if tally(valSet, voterBTCPKs) {
- // if this block gets >2/3 votes, finalise it
- k.finalizeBlock(ctx, blockToFinalize, voterBTCPKs)
- } else {
- // if not, then this block and all subsequent blocks should not be finalised
- // thus, we need to break here
- break
- }
- }
 }
 
 // finalizeBlock sets a block to be finalised in KVStore and distributes rewards to
@@ -86,6 +78,8 @@ func (k Keeper) finalizeBlock(ctx sdk.Context, block *types.IndexedBlock, voterB
  // set block to be finalised in KVStore
  block.Finalized = true
  k.SetBlock(ctx, block)
+ // set next height to finalise as height+1
+ k.setNextHeightToFinalize(ctx, block.Height+1)
  // distribute rewards to BTC staking stakeholders w.r.t. the reward distribution cache
  rdc, err := k.BTCStakingKeeper.GetRewardDistCache(ctx, block.Height)
  if err != nil {
@@ -110,5 +104,23 @@ func tally(valSet map[string]uint64, voterBTCPKs map[string]struct{}) bool {
  votedPower += power
  }
  }
- return votedPower > totalPower*2/3
+ return votedPower*3 > totalPower*2
+}
+
+// setNextHeightToFinalize sets the next height to finalise as the given height
+func (k Keeper) setNextHeightToFinalize(ctx sdk.Context, height uint64) {
+ store := ctx.KVStore(k.storeKey)
+ heightBytes := sdk.Uint64ToBigEndian(height)
+ store.Set(types.NextHeightToFinalizeKey, heightBytes)
+}
+
+// getNextHeightToFinalize gets the next height to finalise
+func (k Keeper) getNextHeightToFinalize(ctx sdk.Context) uint64 {
+ store := ctx.KVStore(k.storeKey)
+ bz := store.Get(types.NextHeightToFinalizeKey)
+ if bz == nil {
+ return 0
+ }
+ height := sdk.BigEndianToUint64(bz)
+ return height
 }

x/finality/keeper/tallying_test.go

@@ -16,7 +16,7 @@ import (
  "github.com/stretchr/testify/require"
 )
 
-func FuzzTallying(f *testing.F) {
+func FuzzTallying_PanicCases(f *testing.F) {
  datagen.AddRandomSeedsToFuzzer(f, 10)
 
  f.Fuzz(func(t *testing.T, seed int64) {
@@ -39,14 +39,29 @@ func FuzzTallying(f *testing.F) {
  Finalized: true,
  })
  // activate BTC staking protocol at height 1
+ ctx = ctx.WithBlockHeight(1)
  bsKeeper.EXPECT().GetBTCStakingActivatedHeight(gomock.Any()).Return(uint64(1), nil).Times(1)
  bsKeeper.EXPECT().GetVotingPowerTable(gomock.Any(), gomock.Eq(uint64(1))).Return(nil).Times(1)
  require.Panics(t, func() { fKeeper.TallyBlocks(ctx) })
+ })
+}
+
+func FuzzTallying_FinalizingNoBlock(f *testing.F) {
+ datagen.AddRandomSeedsToFuzzer(f, 10)
+
+ f.Fuzz(func(t *testing.T, seed int64) {
+ r := rand.New(rand.NewSource(seed))
+ ctrl := gomock.NewController(t)
+ defer ctrl.Finish()
+
+ bsKeeper := types.NewMockBTCStakingKeeper(ctrl)
+ iKeeper := types.NewMockIncentiveKeeper(ctrl)
+ fKeeper, ctx := keepertest.FinalityKeeper(t, bsKeeper, iKeeper)
 
  // activate BTC staking protocol at a random height
  activatedHeight := datagen.RandomInt(r, 10) + 1
 
- // Case 3: index a list of blocks, don't give them QCs, and tally them
+ // index a list of blocks, don't give them QCs, and tally them
  // Expect they are not finalised
  for i := activatedHeight; i < activatedHeight+10; i++ {
  // index blocks
@@ -55,22 +70,12 @@ func FuzzTallying(f *testing.F) {
  LastCommitHash: datagen.GenRandomByteArray(r, 32),
  Finalized: false,
  })
- // 1 vote
- votedValPK, err := datagen.GenRandomBIP340PubKey(r)
+ // this block does not have QC
+ err := giveNoQCToHeight(r, ctx, bsKeeper, fKeeper, i)
  require.NoError(t, err)
- votedSig, err := bbn.NewSchnorrEOTSSig(datagen.GenRandomByteArray(r, 32))
- require.NoError(t, err)
- fKeeper.SetSig(ctx, i, votedValPK, votedSig)
- // 4 BTC vals
- valSet := map[string]uint64{
- hex.EncodeToString(votedValPK.MustMarshal()): 1,
- hex.EncodeToString(datagen.GenRandomByteArray(r, 32)): 1,
- hex.EncodeToString(datagen.GenRandomByteArray(r, 32)): 1,
- hex.EncodeToString(datagen.GenRandomByteArray(r, 32)): 1,
- }
- bsKeeper.EXPECT().GetVotingPowerTable(gomock.Any(), gomock.Eq(i)).Return(valSet).Times(1)
  }
  // add mock queries to GetBTCStakingActivatedHeight
+ ctx = ctx.WithBlockHeight(int64(activatedHeight) + 10 - 1)
  bsKeeper.EXPECT().GetBTCStakingActivatedHeight(gomock.Any()).Return(activatedHeight, nil).Times(1)
  // tally blocks and none of them should be finalised
  fKeeper.TallyBlocks(ctx)
@@ -79,8 +84,26 @@ func FuzzTallying(f *testing.F) {
  require.NoError(t, err)
  require.False(t, ib.Finalized)
  }
+ })
+
+}
+
+func FuzzTallying_FinalizingSomeBlocks(f *testing.F) {
+ datagen.AddRandomSeedsToFuzzer(f, 10)
 
- // Case 4: index a list of blocks, give some of them QCs, and tally them.
+ f.Fuzz(func(t *testing.T, seed int64) {
+ r := rand.New(rand.NewSource(seed))
+ ctrl := gomock.NewController(t)
+ defer ctrl.Finish()
+
+ bsKeeper := types.NewMockBTCStakingKeeper(ctrl)
+ iKeeper := types.NewMockIncentiveKeeper(ctrl)
+ fKeeper, ctx := keepertest.FinalityKeeper(t, bsKeeper, iKeeper)
+
+ // activate BTC staking protocol at a random height
+ activatedHeight := datagen.RandomInt(r, 10) + 1
+
+ // index a list of blocks, give some of them QCs, and tally them.
  // Expect they are all finalised
  numWithQCs := datagen.RandomInt(r, 5) + 1
  for i := activatedHeight; i < activatedHeight+10; i++ {
@@ -105,6 +128,7 @@ func FuzzTallying(f *testing.F) {
  iKeeper.EXPECT().RewardBTCStaking(gomock.Any(), gomock.Any(), gomock.Any()).Return().Times(int(numWithQCs))
  bsKeeper.EXPECT().RemoveRewardDistCache(gomock.Any(), gomock.Any()).Return().Times(int(numWithQCs))
  // add mock queries to GetBTCStakingActivatedHeight
+ ctx = ctx.WithBlockHeight(int64(activatedHeight) + 10 - 1)
  bsKeeper.EXPECT().GetBTCStakingActivatedHeight(gomock.Any()).Return(activatedHeight, nil).Times(1)
  // tally blocks and none of them should be finalised
  fKeeper.TallyBlocks(ctx)
@@ -118,6 +142,7 @@ func FuzzTallying(f *testing.F) {
  }
  }
  })
+
 }
 
 func giveQCToHeight(r *rand.Rand, ctx sdk.Context, bsKeeper *types.MockBTCStakingKeeper, fKeeper *keeper.Keeper, height uint64) error {
@@ -162,7 +187,7 @@ func giveNoQCToHeight(r *rand.Rand, ctx sdk.Context, bsKeeper *types.MockBTCStak
  hex.EncodeToString(datagen.GenRandomByteArray(r, 32)): 1,
  hex.EncodeToString(datagen.GenRandomByteArray(r, 32)): 1,
  }
- bsKeeper.EXPECT().GetVotingPowerTable(gomock.Any(), gomock.Eq(height)).Return(valSet).Times(1)
+ bsKeeper.EXPECT().GetVotingPowerTable(gomock.Any(), gomock.Eq(height)).Return(valSet).MaxTimes(1)
 
  return nil
 }

x/finality/types/keys.go

@@ -15,11 +15,12 @@ const (
 )
 
 var (
- BlockKey = []byte{0x01} // key prefix for blocks
- VoteKey = []byte{0x02} // key prefix for votes
- PubRandKey = []byte{0x03} // key prefix for public randomness
- ParamsKey = []byte{0x04} // key prefix for the parameters
- EvidenceKey = []byte{0x05} // key prefix for evidences
+ BlockKey = []byte{0x01} // key prefix for blocks
+ VoteKey = []byte{0x02} // key prefix for votes
+ PubRandKey = []byte{0x03} // key prefix for public randomness
+ ParamsKey = []byte{0x04} // key prefix for the parameters
+ EvidenceKey = []byte{0x05} // key prefix for evidences
+ NextHeightToFinalizeKey = []byte{0x06} // key prefix for next height to finalise
 )
 
 func KeyPrefix(p string) []byte {