fix(ADR-024): bugs in voting power assignment (#38)

This PR fixed some issues in assigning the voting power, in particular
* even if no new delegations are included, we should also update the
cache about the timestamping status
* fixed return value of `HasTimestampedPubRand`
* e2e tests are fixed
* added table-driven tests for the voting table

test/e2e/btc_staking_e2e_test.go

@@ -244,21 +244,6 @@ func (s *BTCStakingTestSuite) Test2SubmitCovenantSignature() {
 
 	activeDel := activeDels.Dels[0]
 	s.True(activeDel.HasCovenantQuorums(covenantQuorum))
-
-	// wait for a block so that above txs take effect and the voting power table
-	// is updated in the next block's BeginBlock
-	nonValidatorNode.WaitForNextBlock()
-
-	// ensure BTC staking is activated
-	activatedHeight := nonValidatorNode.QueryActivatedHeight()
-	s.Positive(activatedHeight)
-	// ensure finality provider has voting power at activated height
-	currentBtcTip, err := nonValidatorNode.QueryTip()
-	s.NoError(err)
-	activeFps := nonValidatorNode.QueryActiveFinalityProvidersAtHeight(activatedHeight)
-	s.Len(activeFps, 1)
-	s.Equal(activeFps[0].VotingPower, activeDels.VotingPower(currentBtcTip.Height, initialization.BabylonBtcFinalizationPeriod, params.CovenantQuorum))
-	s.Equal(activeFps[0].VotingPower, activeDel.VotingPower(currentBtcTip.Height, initialization.BabylonBtcFinalizationPeriod, params.CovenantQuorum))
 }
 
 // Test2CommitPublicRandomnessAndSubmitFinalitySignature is an end-to-end
@@ -271,17 +256,19 @@ func (s *BTCStakingTestSuite) Test3CommitPublicRandomnessAndSubmitFinalitySignat
 	s.NoError(err)
 
 	// get activated height
-	activatedHeight := nonValidatorNode.QueryActivatedHeight()
-	s.Positive(activatedHeight)
-	_, err = nonValidatorNode.QueryCurrentHeight()
-	s.NoError(err)
+	_, err = nonValidatorNode.QueryActivatedHeight()
+	s.ErrorContains(err, bstypes.ErrBTCStakingNotActivated.Error())
+	fps := nonValidatorNode.QueryFinalityProviders()
+	s.Len(fps, 1)
+	s.Zero(fps[0].VotingPower)
 
 	/*
 		commit a number of public randomness since activatedHeight
 	*/
 	// commit public randomness list
 	numPubRand := uint64(100)
-	randListInfo, msgCommitPubRandList, err := datagen.GenRandomMsgCommitPubRandList(r, fpBTCSK, activatedHeight, numPubRand)
+	commitStartHeight := uint64(1)
+	randListInfo, msgCommitPubRandList, err := datagen.GenRandomMsgCommitPubRandList(r, fpBTCSK, commitStartHeight, numPubRand)
 	s.NoError(err)
 	nonValidatorNode.CommitPubRandList(
 		msgCommitPubRandList.FpBtcPk,
@@ -310,28 +297,29 @@ func (s *BTCStakingTestSuite) Test3CommitPublicRandomnessAndSubmitFinalitySignat
 			return false
 		}
 		return resp.Status == ckpttypes.Sealed
-	}, time.Minute, time.Second*5)
+	}, time.Minute, time.Millisecond*50)
 	nonValidatorNode.FinalizeSealedEpochs(1, currentEpoch)
-	lastFinalizedEpoch := uint64(0)
 
 	// ensure the committed epoch is finalized
+	lastFinalizedEpoch := uint64(0)
 	s.Eventually(func() bool {
 		lastFinalizedEpoch, err = nonValidatorNode.QueryLastFinalizedEpoch()
 		if err != nil {
 			return false
 		}
 		return lastFinalizedEpoch >= currentEpoch
-	}, time.Minute, time.Second)
+	}, time.Minute, time.Millisecond*50)
 
-	// ensure public randomness list is eventually committed
-	var prCommitMap map[uint64]*ftypes.PubRandCommitResponse
+	// ensure btc staking is activated
+	var activatedHeight uint64
 	s.Eventually(func() bool {
-		prCommitMap = nonValidatorNode.QueryListPubRandCommit(cacheFP.BtcPk)
-		return len(prCommitMap) > 0
-	}, time.Minute, time.Second*5)
-	s.Equal(prCommitMap[activatedHeight].NumPubRand, msgCommitPubRandList.NumPubRand)
-	s.Equal(prCommitMap[activatedHeight].Commitment, msgCommitPubRandList.Commitment)
-	s.LessOrEqual(prCommitMap[activatedHeight].EpochNum, lastFinalizedEpoch)
+		activatedHeight, err = nonValidatorNode.QueryActivatedHeight()
+		if err != nil {
+			return false
+		}
+		return activatedHeight > 0
+	}, time.Minute, time.Millisecond*50)
+	s.T().Logf("the activated height is %d", activatedHeight)
 
 	/*
 		submit finality signature
@@ -341,7 +329,7 @@ func (s *BTCStakingTestSuite) Test3CommitPublicRandomnessAndSubmitFinalitySignat
 	s.NoError(err)
 	appHash := blockToVote.AppHash
 
-	idx := 0
+	idx := activatedHeight - commitStartHeight
 	msgToSign := append(sdk.Uint64ToBigEndian(activatedHeight), appHash...)
 	// generate EOTS signature
 	sig, err := eots.Sign(fpBTCSK, randListInfo.SRList[idx], msgToSign)
@@ -355,9 +343,10 @@ func (s *BTCStakingTestSuite) Test3CommitPublicRandomnessAndSubmitFinalitySignat
 	s.Eventually(func() bool {
 		finalizedBlocks = nonValidatorNode.QueryListBlocks(ftypes.QueriedBlockStatus_FINALIZED)
 		return len(finalizedBlocks) > 0
-	}, time.Minute, time.Second)
+	}, time.Minute, time.Millisecond*50)
 	s.Equal(activatedHeight, finalizedBlocks[0].Height)
 	s.Equal(appHash.Bytes(), finalizedBlocks[0].AppHash)
+	s.T().Logf("the block %d is finalized", activatedHeight)
 
 	// ensure finality provider has received rewards after the block is finalised
 	fpRewardGauges, err := nonValidatorNode.QueryRewardGauge(fpBabylonAddr)
@@ -371,6 +360,7 @@ func (s *BTCStakingTestSuite) Test3CommitPublicRandomnessAndSubmitFinalitySignat
 	btcDelRewardGauge, ok := btcDelRewardGauges[itypes.BTCDelegationType.String()]
 	s.True(ok)
 	s.True(btcDelRewardGauge.Coins.IsAllPositive())
+	s.T().Logf("the finality provider received rewards for providing finality")
 }
 
 func (s *BTCStakingTestSuite) Test4WithdrawReward() {

test/e2e/configurer/chain/queries_btcstaking.go

@@ -94,15 +94,19 @@ func (n *NodeConfig) QueryUnbondedDelegations() []*bstypes.BTCDelegationResponse
 	return resp.BtcDelegations
 }
 
-func (n *NodeConfig) QueryActivatedHeight() uint64 {
+func (n *NodeConfig) QueryActivatedHeight() (uint64, error) {
 	bz, err := n.QueryGRPCGateway("/babylon/btcstaking/v1/activated_height", url.Values{})
-	require.NoError(n.t, err)
+	if err != nil {
+		return 0, err
+	}
 
 	var resp bstypes.QueryActivatedHeightResponse
 	err = util.Cdc.UnmarshalJSON(bz, &resp)
-	require.NoError(n.t, err)
+	if err != nil {
+		return 0, err
+	}
 
-	return resp.Height
+	return resp.Height, nil
 }
 
 // TODO: pagination support

test/e2e/upgrades/signet-launch.json

@@ -17,4 +17,4 @@
   "title": "any title",
   "summary": "any summary",
   "expedited": false
-}
+}

x/btcstaking/keeper/msg_server.go

@@ -10,16 +10,17 @@ import (
 
 	errorsmod "cosmossdk.io/errors"
 	sdkmath "cosmossdk.io/math"
-	"github.com/babylonlabs-io/babylon/btcstaking"
-	bbn "github.com/babylonlabs-io/babylon/types"
-	"github.com/babylonlabs-io/babylon/x/btcstaking/types"
 	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/btcutil"
 	"github.com/cosmos/cosmos-sdk/telemetry"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
+
+	"github.com/babylonlabs-io/babylon/btcstaking"
+	bbn "github.com/babylonlabs-io/babylon/types"
+	"github.com/babylonlabs-io/babylon/x/btcstaking/types"
 )
 
 type msgServer struct {

x/btcstaking/keeper/power_dist_change.go

@@ -35,8 +35,23 @@ func (k Keeper) UpdatePowerDist(ctx context.Context) {
 	if len(events) == 0 {
 		if dc != nil {
 			// map everything in prev height to this height
-			k.recordVotingPowerAndCache(ctx, dc)
+			// NOTE: deep copy the previous dist cache because the
+			// cache for the new height shares the same distribution
+			// info due to no new events but timestamping status
+			// might be changed in the new dist cache after calling
+			// k.recordVotingPowerAndCache()
+			newDc := types.NewVotingPowerDistCache()
+			newDc.TotalVotingPower = dc.TotalVotingPower
+			newDc.NumActiveFps = dc.NumActiveFps
+			newFps := make([]*types.FinalityProviderDistInfo, len(dc.FinalityProviders))
+			for i, prevFp := range dc.FinalityProviders {
+				newFp := *prevFp
+				newFps[i] = &newFp
+			}
+			newDc.FinalityProviders = newFps
+			k.recordVotingPowerAndCache(ctx, dc, newDc, maxActiveFps)
 		}
+
 		return
 	}
 
@@ -56,31 +71,56 @@ func (k Keeper) UpdatePowerDist(ctx context.Context) {
 	// to construct the new distribution
 	newDc := k.ProcessAllPowerDistUpdateEvents(ctx, dc, events, maxActiveFps)
 
-	// find newly bonded finality providers and execute the hooks
-	newBondedFinalityProviders := newDc.FindNewActiveFinalityProviders(dc)
-	for _, fp := range newBondedFinalityProviders {
-		if err := k.hooks.AfterFinalityProviderActivated(ctx, fp.BtcPk); err != nil {
-			panic(fmt.Errorf("failed to execute after finality provider %s bonded", fp.BtcPk.MarshalHex()))
-		}
-	}
-
 	// record voting power and cache for this height
-	k.recordVotingPowerAndCache(ctx, newDc)
+	k.recordVotingPowerAndCache(ctx, dc, newDc, maxActiveFps)
 	// record metrics
 	k.recordMetrics(newDc)
 }
 
-func (k Keeper) recordVotingPowerAndCache(ctx context.Context, dc *types.VotingPowerDistCache) {
+// recordVotingPowerAndCache assigns voting power to each active finality provider
+// with the following consideration:
+// 1. the fp must have timestamped pub rand
+// 2. the fp must in the top x ranked by the voting power (x is given by maxActiveFps)
+// NOTE: the previous and the new dist cache cannot be nil
+func (k Keeper) recordVotingPowerAndCache(ctx context.Context, prevDc, newDc *types.VotingPowerDistCache, maxActiveFps uint32) {
+	if prevDc == nil || newDc == nil {
+		panic("the voting power distribution cache cannot be nil")
+	}
+
 	babylonTipHeight := uint64(sdk.UnwrapSDKContext(ctx).HeaderInfo().Height)
 
-	// set voting power table for this height
-	for i := uint32(0); i < dc.NumActiveFps; i++ {
-		fp := dc.FinalityProviders[i]
+	// label fps with whether it has timestamped pub rand so that these fps
+	// will not be assigned voting power
+	for _, fp := range newDc.FinalityProviders {
+		// TODO calling HasTimestampedPubRand potentially iterates
+		// all the pub rand committed by the fp, which might slow down
+		// the process, need optimization
+		fp.IsTimestamped = k.FinalityKeeper.HasTimestampedPubRand(ctx, fp.BtcPk, babylonTipHeight)
+	}
+
+	// apply the finality provider voting power dist info to the new cache
+	// after which the cache would have active fps that are top N fps ranked
+	// by voting power with timestamped pub rand
+	newDc.ApplyActiveFinalityProviders(maxActiveFps)
+
+	// set voting power table for each active finality providers at this height
+	for i := uint32(0); i < newDc.NumActiveFps; i++ {
+		fp := newDc.FinalityProviders[i]
 		k.SetVotingPower(ctx, fp.BtcPk.MustMarshal(), babylonTipHeight, fp.TotalVotingPower)
 	}
 
+	// find newly activated finality providers and execute the hooks by comparing
+	// the previous dist cache
+	newActivatedFinalityProviders := newDc.FindNewActiveFinalityProviders(prevDc)
+	for _, fp := range newActivatedFinalityProviders {
+		if err := k.hooks.AfterFinalityProviderActivated(ctx, fp.BtcPk); err != nil {
+			panic(fmt.Errorf("failed to execute after finality provider %s activated", fp.BtcPk.MarshalHex()))
+		}
+		k.Logger(sdk.UnwrapSDKContext(ctx)).Info("a new finality provider is activated", "pk", fp.BtcPk.MarshalHex())
+	}
+
 	// set the voting power distribution cache of the current height
-	k.setVotingPowerDistCache(ctx, babylonTipHeight, dc)
+	k.setVotingPowerDistCache(ctx, babylonTipHeight, newDc)
 }
 
 func (k Keeper) recordMetrics(dc *types.VotingPowerDistCache) {
@@ -112,7 +152,6 @@ func (k Keeper) ProcessAllPowerDistUpdateEvents(
 	events []*types.EventPowerDistUpdate,
 	maxActiveFps uint32,
 ) *types.VotingPowerDistCache {
-	height := uint64(sdk.UnwrapSDKContext(ctx).HeaderInfo().Height)
 	// a map where key is finality provider's BTC PK hex and value is a list
 	// of BTC delegations that newly become active under this provider
 	activeBTCDels := map[string][]*types.BTCDelegation{}
@@ -236,18 +275,6 @@ func (k Keeper) ProcessAllPowerDistUpdateEvents(
 		}
 	}
 
-	// label fps that does not have timestamped pub rand
-	for _, fp := range newDc.FinalityProviders {
-		// TODO calling HasTimestampedPubRand potentially iterates
-		// all the pub rand committed by the fp, which might slow down
-		// the process, need optimization
-		fp.IsTimestamped = k.FinalityKeeper.HasTimestampedPubRand(ctx, fp.BtcPk, height)
-	}
-
-	// filter out the top N finality providers and their total voting power, and
-	// record them in the new cache
-	newDc.ApplyActiveFinalityProviders(maxActiveFps)
-
 	return newDc
 }
 

x/btcstaking/keeper/power_dist_change_test.go

@@ -157,7 +157,6 @@ func FuzzBTCDelegationEvents(f *testing.F) {
 		btclcKeeper := types.NewMockBTCLightClientKeeper(ctrl)
 		btccKeeper := types.NewMockBtcCheckpointKeeper(ctrl)
 		finalityKeeper := types.NewMockFinalityKeeper(ctrl)
-		finalityKeeper.EXPECT().HasTimestampedPubRand(gomock.Any(), gomock.Any(), gomock.Any()).Return(true).AnyTimes()
 		h := NewHelper(t, btclcKeeper, btccKeeper, finalityKeeper)
 
 		// set all parameters
@@ -221,10 +220,21 @@ func FuzzBTCDelegationEvents(f *testing.F) {
 		require.Equal(t, expectedStakingTxHash, btcDelStateUpdate.StakingTxHash)
 		require.Equal(t, types.BTCDelegationStatus_ACTIVE, btcDelStateUpdate.NewState)
 
-		// ensure this finality provider has voting power at the current height
+		// ensure this finality provider does not have voting power at the current height
+		// due to no timestamped randomness
+		babylonHeight += 1
+		h.SetCtxHeight(babylonHeight)
+		h.BTCLightClientKeeper.EXPECT().GetTipInfo(gomock.Eq(h.Ctx)).Return(btcTip).AnyTimes()
+		finalityKeeper.EXPECT().HasTimestampedPubRand(gomock.Any(), gomock.Any(), gomock.Eq(babylonHeight)).Return(false).AnyTimes()
+		err = h.BTCStakingKeeper.BeginBlocker(h.Ctx)
+		h.NoError(err)
+		require.Zero(t, h.BTCStakingKeeper.GetVotingPower(h.Ctx, *fp.BtcPk, babylonHeight))
+
+		// ensure this finality provider has voting power at the current height after having timestamped pub rand
 		babylonHeight += 1
 		h.SetCtxHeight(babylonHeight)
 		h.BTCLightClientKeeper.EXPECT().GetTipInfo(gomock.Eq(h.Ctx)).Return(btcTip).AnyTimes()
+		finalityKeeper.EXPECT().HasTimestampedPubRand(gomock.Any(), gomock.Any(), gomock.Eq(babylonHeight)).Return(true).AnyTimes()
 		err = h.BTCStakingKeeper.BeginBlocker(h.Ctx)
 		h.NoError(err)
 		require.Equal(t, uint64(stakingValue), h.BTCStakingKeeper.GetVotingPower(h.Ctx, *fp.BtcPk, babylonHeight))