From d14dbe0dc4a4bf9b9654099377c8af2997ea0309 Mon Sep 17 00:00:00 2001 From: Cirrus Gai Date: Fri, 30 Aug 2024 17:52:15 +0800 Subject: [PATCH] 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 | 56 ++++----- .../configurer/chain/queries_btcstaking.go | 12 +- test/e2e/upgrades/signet-launch.json | 2 +- x/btcstaking/keeper/msg_server.go | 7 +- x/btcstaking/keeper/power_dist_change.go | 83 +++++++----- x/btcstaking/keeper/power_dist_change_test.go | 14 ++- x/btcstaking/types/incentive_test.go | 119 ++++++++++++++++++ x/finality/keeper/liveness.go | 2 +- x/finality/keeper/public_randomness.go | 5 +- 9 files changed, 227 insertions(+), 73 deletions(-) create mode 100644 x/btcstaking/types/incentive_test.go diff --git a/test/e2e/btc_staking_e2e_test.go b/test/e2e/btc_staking_e2e_test.go index d8b733515..58c28f562 100644 --- a/test/e2e/btc_staking_e2e_test.go +++ b/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() { diff --git a/test/e2e/configurer/chain/queries_btcstaking.go b/test/e2e/configurer/chain/queries_btcstaking.go index f7a735c8f..861ac6a69 100644 --- a/test/e2e/configurer/chain/queries_btcstaking.go +++ b/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 diff --git a/test/e2e/upgrades/signet-launch.json b/test/e2e/upgrades/signet-launch.json index 820686a2f..2bd5a9f31 100644 --- a/test/e2e/upgrades/signet-launch.json +++ b/test/e2e/upgrades/signet-launch.json @@ -17,4 +17,4 @@ "title": "any title", "summary": "any summary", "expedited": false -} \ No newline at end of file +} diff --git a/x/btcstaking/keeper/msg_server.go b/x/btcstaking/keeper/msg_server.go index f797f922f..b94ff8bd9 100644 --- a/x/btcstaking/keeper/msg_server.go +++ b/x/btcstaking/keeper/msg_server.go @@ -10,9 +10,6 @@ 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" @@ -20,6 +17,10 @@ import ( 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 { diff --git a/x/btcstaking/keeper/power_dist_change.go b/x/btcstaking/keeper/power_dist_change.go index 2b42b1742..d924dd5e4 100644 --- a/x/btcstaking/keeper/power_dist_change.go +++ b/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 } diff --git a/x/btcstaking/keeper/power_dist_change_test.go b/x/btcstaking/keeper/power_dist_change_test.go index 0016ca9c6..0277ad28c 100644 --- a/x/btcstaking/keeper/power_dist_change_test.go +++ b/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)) diff --git a/x/btcstaking/types/incentive_test.go b/x/btcstaking/types/incentive_test.go new file mode 100644 index 000000000..fe844fe21 --- /dev/null +++ b/x/btcstaking/types/incentive_test.go @@ -0,0 +1,119 @@ +package types + +import ( + "testing" + + "github.com/btcsuite/btcd/btcec/v2" + "github.com/stretchr/testify/require" + + "github.com/babylonlabs-io/babylon/types" +) + +var ( + fpPrivKey1, _ = btcec.NewPrivateKey() + fpPrivKey2, _ = btcec.NewPrivateKey() + fpPubKey1 = types.NewBIP340PubKeyFromBTCPK(fpPrivKey1.PubKey()) + fpPubKey2 = types.NewBIP340PubKeyFromBTCPK(fpPrivKey2.PubKey()) +) + +func TestVotingPowerDistCache(t *testing.T) { + tests := []struct { + desc string + maxActiveFPs uint32 + numActiveFps uint32 + totalVotingPower uint64 + prevDistCache *VotingPowerDistCache + fps []*FinalityProviderDistInfo + }{ + { + desc: "all not timestamped", + maxActiveFPs: 80, + numActiveFps: 0, + totalVotingPower: 0, + prevDistCache: NewVotingPowerDistCache(), + fps: []*FinalityProviderDistInfo{ + { + BtcPk: fpPubKey1, + TotalVotingPower: 1000, + IsTimestamped: false, + }, + { + BtcPk: fpPubKey2, + TotalVotingPower: 2000, + IsTimestamped: false, + }, + }, + }, + { + desc: "all timestamped", + maxActiveFPs: 80, + numActiveFps: 2, + totalVotingPower: 3000, + prevDistCache: NewVotingPowerDistCache(), + fps: []*FinalityProviderDistInfo{ + { + BtcPk: fpPubKey1, + TotalVotingPower: 1000, + IsTimestamped: true, + }, + { + BtcPk: fpPubKey2, + TotalVotingPower: 2000, + IsTimestamped: true, + }, + }, + }, + { + desc: "partly timestamped", + maxActiveFPs: 80, + numActiveFps: 1, + totalVotingPower: 1000, + prevDistCache: NewVotingPowerDistCache(), + fps: []*FinalityProviderDistInfo{ + { + BtcPk: fpPubKey1, + TotalVotingPower: 1000, + IsTimestamped: true, + }, + { + BtcPk: fpPubKey2, + TotalVotingPower: 2000, + IsTimestamped: false, + }, + }, + }, + { + desc: "small max active fps", + maxActiveFPs: 1, + numActiveFps: 1, + totalVotingPower: 2000, + prevDistCache: NewVotingPowerDistCache(), + fps: []*FinalityProviderDistInfo{ + { + BtcPk: fpPubKey1, + TotalVotingPower: 1000, + IsTimestamped: true, + }, + { + BtcPk: fpPubKey2, + TotalVotingPower: 2000, + IsTimestamped: true, + }, + }, + }, + } + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + dc := NewVotingPowerDistCache() + for _, fp := range tc.fps { + dc.AddFinalityProviderDistInfo(fp) + } + dc.ApplyActiveFinalityProviders(tc.maxActiveFPs) + require.Equal(t, tc.totalVotingPower, dc.TotalVotingPower) + require.Equal(t, tc.numActiveFps, dc.NumActiveFps) + + newBondedFps := dc.FindNewActiveFinalityProviders(tc.prevDistCache) + require.Equal(t, tc.numActiveFps, uint32(len(newBondedFps))) + }) + } +} diff --git a/x/finality/keeper/liveness.go b/x/finality/keeper/liveness.go index 59cf453cc..f46e52b37 100644 --- a/x/finality/keeper/liveness.go +++ b/x/finality/keeper/liveness.go @@ -142,7 +142,7 @@ func (k Keeper) updateSigningInfo( // fetch signing info signInfo, err := k.FinalityProviderSigningTracker.Get(ctx, fpPk.MustMarshal()) if err != nil { - return false, nil, err + return false, nil, fmt.Errorf("the signing info is not created") } signedBlocksWindow := params.SignedBlocksWindow diff --git a/x/finality/keeper/public_randomness.go b/x/finality/keeper/public_randomness.go index 1fef0066e..d68dc6f48 100644 --- a/x/finality/keeper/public_randomness.go +++ b/x/finality/keeper/public_randomness.go @@ -44,6 +44,9 @@ func (k Keeper) GetTimestampedPubRandCommitForHeight(ctx context.Context, fpBtcP // ensure the finality provider's last randomness commit is already finalised by BTC timestamping finalizedEpoch := k.GetLastFinalizedEpoch(ctx) + if finalizedEpoch == 0 { + return nil, types.ErrPubRandCommitNotBTCTimestamped.Wrapf("no finalized epoch yet") + } if finalizedEpoch < prCommit.EpochNum { return nil, types.ErrPubRandCommitNotBTCTimestamped. Wrapf("the finality provider %s last committed epoch number: %d, last finalized epoch number: %d", @@ -55,7 +58,7 @@ func (k Keeper) GetTimestampedPubRandCommitForHeight(ctx context.Context, fpBtcP func (k Keeper) HasTimestampedPubRand(ctx context.Context, fpBtcPK *bbn.BIP340PubKey, height uint64) bool { _, err := k.GetTimestampedPubRandCommitForHeight(ctx, fpBtcPK, height) - return err != nil + return err == nil } // SetPubRandCommit adds the given public randomness commitment for the given public key