Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Aug 30, 2024
+227 −73
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2b1a90a
fix voting power
gitferry Aug 26, 2024
b74b8b7
add tests and fix updating prev dist cache
gitferry Aug 27, 2024
0588060
fix e2e
gitferry Aug 27, 2024
0ce82b9
reduce e2e test polling time
gitferry Aug 28, 2024
6e7436f
fix dist cache copy issue
gitferry Aug 28, 2024
f3099ee
pr comment
gitferry Aug 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 23 additions & 33 deletions test/e2e/btc_staking_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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() {
Expand Down
12 changes: 8 additions & 4 deletions test/e2e/configurer/chain/queries_btcstaking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/upgrades/signet-launch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@
"title": "any title",
"summary": "any summary",
"expedited": false
}
}
71 changes: 44 additions & 27 deletions x/btcstaking/keeper/power_dist_change.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ 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)
k.recordVotingPowerAndCache(ctx, dc, nil, maxActiveFps)
}
return
}
Expand All @@ -56,31 +56,61 @@ 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) {
func (k Keeper) recordVotingPowerAndCache(ctx context.Context, prevDc, newDc *types.VotingPowerDistCache, maxActiveFps uint32) {
if prevDc == nil {
panic("the previous voting power distribution cache cannot be nil")
}

// deep copy the previous dist cache if the new dist cache is nil
if newDc == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be cleaner to move this copying process to above the call to recordVotingPowerAndCache in case when there is no new events i.e somewehre under // map everything in prev height to this height comment.

Otherwise this nil check just generate questions like:

  • oh why we can pass nil here ?
  • should we also check prevDc to be non-nil ?
    etc.

Where if this is handled on call site we can add comments along the line that becouse there is no new power changing events, the new dc is just copy of the old one and the deep copy is need due to recordVotingPowerAndCache modifying provided arguments. (or something like that)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed allowing nil parameter is hacky. Fixed in f3099ee

newDc = types.NewVotingPowerDistCache()
newDc.TotalVotingPower = prevDc.TotalVotingPower
newDc.NumActiveFps = prevDc.NumActiveFps
newFps := make([]*types.FinalityProviderDistInfo, len(prevDc.FinalityProviders))
for i, prevFp := range prevDc.FinalityProviders {
newFp := *prevFp
newFps[i] = &newFp
}
newDc.FinalityProviders = newFps
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a deep copy here? It seems that prevDc is only used in L104. Is
it possible to change L103-L110 to that, if newDc != nil then execute
L103-L110, otherwise avoid doing so? To make L85-L91 work we can simply do
newDc := prevDc

Copy link
Member Author

@gitferry gitferry Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't do deep copy, the change at Line 90 will change the IsTimestamped value in prevDc, which makes no newly activated finality providers at L104. This specifically affects the case where no new delegations are added but new finality providers have timestamped pub rand


babylonTipHeight := uint64(sdk.UnwrapSDKContext(ctx).HeaderInfo().Height)

// label fps with whether it has 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, babylonTipHeight)
}

// filter out the top N finality providers and their total voting power, and
// record them in the new cache
newDc.ApplyActiveFinalityProviders(maxActiveFps)

// set voting power table for this height
for i := uint32(0); i < dc.NumActiveFps; i++ {
fp := dc.FinalityProviders[i]
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
newActivatedFinalityProviders := newDc.FindNewActiveFinalityProviders(prevDc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the case where len(events) == 0 and dc != nil that we do the deep copy, it will never have a set of newActivatedFinalityProviders ?

Copy link
Member Author

@gitferry gitferry Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same but turned out there is an issue if we don't take care of this (took me a long time to realize this...). So, even if no new events, some fps could have newly timestamped pub rand and become a new activated fp. For example, we only have one fp and one delegation to it at height 100 but it does not have timestamped pub rand. When it comes to height 101, the fp's pub rand commit is timestamped, so it will be assigned voting power for 101, thus it becomes activated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, thanks for explaining!
In this case, we don't have a way to avoid the deep copy :/

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) {
Expand Down Expand Up @@ -112,7 +142,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{}
Expand Down Expand Up @@ -236,18 +265,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
}

Expand Down
14 changes: 12 additions & 2 deletions x/btcstaking/keeper/power_dist_change_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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))
Expand Down
Loading
Loading