From 5ff5e3ee40f347076a3f4ceb93fe75bff1e6318d Mon Sep 17 00:00:00 2001 From: Fangyu Gai Date: Tue, 19 Nov 2024 19:30:43 +0800 Subject: [PATCH 1/6] allow min unbonding value --- testutil/btcstaking-helper/keeper.go | 5 +-- x/btccheckpoint/keeper/msg_server.go | 7 +++- x/btcstaking/keeper/btc_delegations.go | 2 +- x/btcstaking/keeper/genesis_test.go | 8 ++--- x/btcstaking/keeper/msg_server.go | 18 +++++++--- x/btcstaking/keeper/msg_server_test.go | 22 +++--------- x/btcstaking/keeper/params.go | 3 +- x/btcstaking/types/btcstaking.go | 15 -------- x/btcstaking/types/params.go | 5 ++- x/btcstaking/types/validate_parsed_message.go | 15 +++----- .../types/validate_parsed_message_test.go | 13 +++---- x/finality/keeper/power_dist_change_test.go | 34 ++++++------------- 12 files changed, 57 insertions(+), 90 deletions(-) diff --git a/testutil/btcstaking-helper/keeper.go b/testutil/btcstaking-helper/keeper.go index 88b56ec21..eb80fb3e0 100644 --- a/testutil/btcstaking-helper/keeper.go +++ b/testutil/btcstaking-helper/keeper.go @@ -124,7 +124,8 @@ func (h *Helper) BeginBlocker() { } func (h *Helper) GenAndApplyParams(r *rand.Rand) ([]*btcec.PrivateKey, []*btcec.PublicKey) { - return h.GenAndApplyCustomParams(r, 100, 0, 0) + // ensure that minUnbondingTime is larger than finalizationTimeout + return h.GenAndApplyCustomParams(r, 100, 200, 0) } func (h *Helper) SetCtxHeight(height uint64) { @@ -228,7 +229,7 @@ func (h *Helper) CreateDelegation( if unbondingValue == 0 { unbondingValue = defaultUnbondingValue } - defaultUnbondingTime := types.MinimumUnbondingTime(&bsParams, &bcParams) + 1 + defaultUnbondingTime := bsParams.MinUnbondingTimeBlocks if unbondingTime == 0 { unbondingTime = uint16(defaultUnbondingTime) } diff --git a/x/btccheckpoint/keeper/msg_server.go b/x/btccheckpoint/keeper/msg_server.go index 22c5421c9..81bff0830 100644 --- a/x/btccheckpoint/keeper/msg_server.go +++ b/x/btccheckpoint/keeper/msg_server.go @@ -4,9 +4,10 @@ import ( "context" errorsmod "cosmossdk.io/errors" - "github.com/babylonlabs-io/babylon/x/btccheckpoint/types" sdk "github.com/cosmos/cosmos-sdk/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + + "github.com/babylonlabs-io/babylon/x/btccheckpoint/types" ) var _ types.MsgServer = msgServer{} @@ -110,6 +111,10 @@ func (ms msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdatePara } ctx := sdk.UnwrapSDKContext(goCtx) + if req.Params.CheckpointFinalizationTimeout != ms.k.GetParams(ctx).CheckpointFinalizationTimeout { + return nil, govtypes.ErrInvalidProposalMsg.Wrapf("the checkpoint finalization timeout cannot be changed") + } + if err := ms.k.SetParams(ctx, req.Params); err != nil { return nil, err } diff --git a/x/btcstaking/keeper/btc_delegations.go b/x/btcstaking/keeper/btc_delegations.go index d1496d187..52393abdc 100644 --- a/x/btcstaking/keeper/btc_delegations.go +++ b/x/btcstaking/keeper/btc_delegations.go @@ -79,7 +79,7 @@ func (k Keeper) AddBTCDelegation( NewState: types.BTCDelegationStatus_UNBONDED, }) - // NOTE: we should have verified that EndHeight > btcTip.Height + max(w, min_unbonding_time) + // NOTE: we should have verified that EndHeight > btcTip.Height + min_unbonding_time k.addPowerDistUpdateEvent(ctx, btcDel.EndHeight-minUnbondingTime, unbondedEvent) } diff --git a/x/btcstaking/keeper/genesis_test.go b/x/btcstaking/keeper/genesis_test.go index 7300e7f31..c0e0bc4b7 100644 --- a/x/btcstaking/keeper/genesis_test.go +++ b/x/btcstaking/keeper/genesis_test.go @@ -6,23 +6,23 @@ import ( "strings" "testing" + "github.com/stretchr/testify/require" + "github.com/babylonlabs-io/babylon/testutil/datagen" "github.com/babylonlabs-io/babylon/testutil/helper" btclightclientt "github.com/babylonlabs-io/babylon/x/btclightclient/types" "github.com/babylonlabs-io/babylon/x/btcstaking/types" - "github.com/stretchr/testify/require" ) func TestExportGenesis(t *testing.T) { r, h := rand.New(rand.NewSource(11)), helper.NewHelper(t) - k, btclcK, btcCheckK, ctx := h.App.BTCStakingKeeper, h.App.BTCLightClientKeeper, h.App.BtcCheckpointKeeper, h.Ctx + k, btclcK, ctx := h.App.BTCStakingKeeper, h.App.BTCLightClientKeeper, h.Ctx numFps := 3 fps := datagen.CreateNFinalityProviders(r, t, numFps) params := k.GetParams(ctx) - btcckptParams := btcCheckK.GetParams(ctx) - minUnbondingTime := types.MinimumUnbondingTime(¶ms, &btcckptParams) + minUnbondingTime := params.MinUnbondingTimeBlocks chainsHeight := make([]*types.BlockHeightBbnToBtc, 0) // creates the first as it starts already with an chain height from the helper. diff --git a/x/btcstaking/keeper/msg_server.go b/x/btcstaking/keeper/msg_server.go index 1159835d5..aee41657d 100644 --- a/x/btcstaking/keeper/msg_server.go +++ b/x/btcstaking/keeper/msg_server.go @@ -46,7 +46,16 @@ func (ms msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdatePara return nil, govtypes.ErrInvalidProposalMsg.Wrapf("invalid parameter: %v", err) } + // ensure the min unbonding time is always larger than the checkpoint finalization timeout ctx := sdk.UnwrapSDKContext(goCtx) + ckptFinalizationTime := ms.btccKeeper.GetParams(ctx).CheckpointFinalizationTimeout + minUnbondingTime := req.Params.MinUnbondingTimeBlocks + if minUnbondingTime <= ckptFinalizationTime { + return nil, govtypes.ErrInvalidProposalMsg. + Wrapf("the min unbonding time %d must be larger than the checkpoint finalization timeout %d", + minUnbondingTime, ckptFinalizationTime) + } + if err := ms.SetParams(ctx, req.Params); err != nil { return nil, err } @@ -190,9 +199,7 @@ func (ms msgServer) CreateBTCDelegation(goCtx context.Context, req *types.MsgCre } // 6. Validate parsed message against parameters - btccParams := ms.btccKeeper.GetParams(ctx) - - paramsValidationResult, err := types.ValidateParsedMessageAgainstTheParams(parsedMsg, &vp.Params, &btccParams, ms.btcNet) + paramsValidationResult, err := types.ValidateParsedMessageAgainstTheParams(parsedMsg, &vp.Params, ms.btcNet) if err != nil { return nil, err @@ -200,6 +207,7 @@ func (ms msgServer) CreateBTCDelegation(goCtx context.Context, req *types.MsgCre // 7. If the delegation contains the inclusion proof, we need to verify the proof // and set start height and end height + btccParams := ms.btccKeeper.GetParams(ctx) var startHeight, endHeight uint32 if parsedMsg.StakingTxProofOfInclusion != nil { timeInfo, err := ms.VerifyInclusionProofAndGetHeight( @@ -300,7 +308,7 @@ func (ms msgServer) AddBTCDelegationInclusionProof( btccParams := ms.btccKeeper.GetParams(ctx) - minUnbondingTime := types.MinimumUnbondingTime(params, &btccParams) + minUnbondingTime := params.MinUnbondingTimeBlocks timeInfo, err := ms.VerifyInclusionProofAndGetHeight( ctx, @@ -349,7 +357,7 @@ func (ms msgServer) AddBTCDelegationInclusionProof( NewState: types.BTCDelegationStatus_UNBONDED, }) - // NOTE: we should have verified that EndHeight > btcTip.Height + max(w, min_unbonding_time) + // NOTE: we should have verified that EndHeight > btcTip.Height + min_unbonding_time ms.addPowerDistUpdateEvent(ctx, btcDel.EndHeight-minUnbondingTime, unbondedEvent) // at this point, the BTC delegation inclusion proof is verified and is not duplicated diff --git a/x/btcstaking/keeper/msg_server_test.go b/x/btcstaking/keeper/msg_server_test.go index eba55bffe..3c0da809d 100644 --- a/x/btcstaking/keeper/msg_server_test.go +++ b/x/btcstaking/keeper/msg_server_test.go @@ -690,14 +690,10 @@ func TestDoNotAllowDelegationWithoutFinalityProvider(t *testing.T) { // set covenant PK to params _, covenantPKs := h.GenAndApplyParams(r) bsParams := h.BTCStakingKeeper.GetParams(h.Ctx) - bcParams := h.BTCCheckpointKeeper.GetParams(h.Ctx) - minUnbondingTime := types.MinimumUnbondingTime( - &bsParams, - &bcParams, - ) + minUnbondingTime := bsParams.MinUnbondingTimeBlocks - slashingChangeLockTime := uint16(minUnbondingTime) + 1 + slashingChangeLockTime := uint16(minUnbondingTime) // We only generate a finality provider, but not insert it into KVStore. So later // insertion of delegation should fail. @@ -820,13 +816,6 @@ func TestCorrectUnbondingTimeInDelegation(t *testing.T) { finalizationTimeout: 100, err: nil, }, - { - name: "failed delegation when ubonding time in delegation is not larger than finalization time when min unbonding time is lower than finalization timeout", - unbondingTimeInDelegation: 100, - minUnbondingTime: 99, - finalizationTimeout: 100, - err: types.ErrInvalidUnbondingTx, - }, { name: "successful delegation when ubonding time ubonding time in delegation is larger than min unbonding time when min unbonding time is larger than finalization timeout", unbondingTimeInDelegation: 151, @@ -835,11 +824,11 @@ func TestCorrectUnbondingTimeInDelegation(t *testing.T) { err: nil, }, { - name: "failed delegation when ubonding time in delegation is not larger than minUnbondingTime when min unbonding time is larger than finalization timeout", + name: "successful delegation when ubonding time in delegation is equal to minUnbondingTime when min unbonding time is larger than finalization timeout", unbondingTimeInDelegation: 150, minUnbondingTime: 150, finalizationTimeout: 100, - err: types.ErrInvalidUnbondingTx, + err: nil, }, } @@ -1063,9 +1052,8 @@ func FuzzDeterminismBtcstakingBeginBlocker(f *testing.F) { stakingParams := h.App.BTCStakingKeeper.GetParams(h.Ctx) covQuorum := stakingParams.CovenantQuorum maxFinalityProviders := int32(h.App.FinalityKeeper.GetParams(h.Ctx).MaxActiveFinalityProviders) - btcckptParams := h.App.BtcCheckpointKeeper.GetParams(h.Ctx) - minUnbondingTime := types.MinimumUnbondingTime(&stakingParams, &btcckptParams) + minUnbondingTime := stakingParams.MinUnbondingTimeBlocks // Number of finality providers from 10 to maxFinalityProviders + 10 numFinalityProviders := int(r.Int31n(maxFinalityProviders) + 10) diff --git a/x/btcstaking/keeper/params.go b/x/btcstaking/keeper/params.go index be11a06e1..044b8bc9d 100644 --- a/x/btcstaking/keeper/params.go +++ b/x/btcstaking/keeper/params.go @@ -7,8 +7,9 @@ import ( "cosmossdk.io/math" "cosmossdk.io/store/prefix" - "github.com/babylonlabs-io/babylon/x/btcstaking/types" "github.com/cosmos/cosmos-sdk/runtime" + + "github.com/babylonlabs-io/babylon/x/btcstaking/types" ) // cosmos-sdk does not have utils for uint32 diff --git a/x/btcstaking/types/btcstaking.go b/x/btcstaking/types/btcstaking.go index 5a0fe1b6c..b7cdfa49d 100644 --- a/x/btcstaking/types/btcstaking.go +++ b/x/btcstaking/types/btcstaking.go @@ -3,13 +3,10 @@ package types import ( "fmt" - "cosmossdk.io/math" - sdk "github.com/cosmos/cosmos-sdk/types" asig "github.com/babylonlabs-io/babylon/crypto/schnorr-adaptor-signature" bbn "github.com/babylonlabs-io/babylon/types" - btcctypes "github.com/babylonlabs-io/babylon/x/btccheckpoint/types" ) func (fp *FinalityProvider) IsSlashed() bool { @@ -105,15 +102,3 @@ func GetOrderedCovenantSignatures(fpIdx int, covSigsList []*CovenantAdaptorSigna return orderedCovSigs, nil } - -// MinimumUnbondingTime returns the minimum unbonding time. It is the bigger value from: -// - MinUnbondingTime -// - CheckpointFinalizationTimeout -func MinimumUnbondingTime( - stakingParams *Params, - checkpointingParams *btcctypes.Params) uint32 { - return math.Max[uint32]( - stakingParams.MinUnbondingTimeBlocks, - checkpointingParams.CheckpointFinalizationTimeout, - ) -} diff --git a/x/btcstaking/types/params.go b/x/btcstaking/types/params.go index 43528030c..b565920d7 100644 --- a/x/btcstaking/types/params.go +++ b/x/btcstaking/types/params.go @@ -72,9 +72,8 @@ func DefaultParams() Params { MinCommissionRate: sdkmath.LegacyZeroDec(), // The Default slashing rate is 0.1 i.e., 10% of the total staked BTC will be burned. SlashingRate: sdkmath.LegacyNewDecWithPrec(1, 1), // 1 * 10^{-1} = 0.1 - // The default minimum unbonding time is 0, which effectively defaults to checkpoint - // finalization timeout. - MinUnbondingTimeBlocks: 0, + // min unbonding time should be always larger than the checkpoint finalization timeout + MinUnbondingTimeBlocks: 200, UnbondingFeeSat: 1000, DelegationCreationBaseGasFee: defaultDelegationCreationBaseGasFee, // The default allow list expiration height is 0, which effectively disables the allow list. diff --git a/x/btcstaking/types/validate_parsed_message.go b/x/btcstaking/types/validate_parsed_message.go index 33d8897ab..5a762c717 100644 --- a/x/btcstaking/types/validate_parsed_message.go +++ b/x/btcstaking/types/validate_parsed_message.go @@ -8,7 +8,6 @@ import ( "github.com/babylonlabs-io/babylon/btcstaking" bbn "github.com/babylonlabs-io/babylon/types" - btcckpttypes "github.com/babylonlabs-io/babylon/x/btccheckpoint/types" ) type ParamsValidationResult struct { @@ -21,17 +20,13 @@ type ParamsValidationResult struct { func ValidateParsedMessageAgainstTheParams( pm *ParsedCreateDelegationMessage, parameters *Params, - btcheckpointParameters *btcckpttypes.Params, net *chaincfg.Params, ) (*ParamsValidationResult, error) { // 1. Validate unbonding time first as it will be used in other checks - minUnbondingTime := MinimumUnbondingTime(parameters, btcheckpointParameters) - // Check unbonding time (staking time from unbonding tx) is larger than min unbonding time - // which is larger value from: - // - MinUnbondingTime - // - CheckpointFinalizationTimeout - if uint32(pm.UnbondingTime) <= minUnbondingTime { - return nil, ErrInvalidUnbondingTx.Wrapf("unbonding time %d must be larger than %d", pm.UnbondingTime, minUnbondingTime) + // Check unbonding time (staking time from unbonding tx) is not less than min unbonding time + if uint32(pm.UnbondingTime) < parameters.MinUnbondingTimeBlocks { + return nil, ErrInvalidUnbondingTx.Wrapf("unbonding time %d must not be less than %d", + pm.UnbondingTime, parameters.MinUnbondingTimeBlocks) } stakingTxHash := pm.StakingTx.Transaction.TxHash() @@ -206,6 +201,6 @@ func ValidateParsedMessageAgainstTheParams( return &ParamsValidationResult{ StakingOutputIdx: stakingOutputIdx, UnbondingOutputIdx: 0, // unbonding output always has only 1 output - MinUnbondingTime: minUnbondingTime, + MinUnbondingTime: parameters.MinUnbondingTimeBlocks, }, nil } diff --git a/x/btcstaking/types/validate_parsed_message_test.go b/x/btcstaking/types/validate_parsed_message_test.go index 5c65e2b1c..eeb1d1f5f 100644 --- a/x/btcstaking/types/validate_parsed_message_test.go +++ b/x/btcstaking/types/validate_parsed_message_test.go @@ -153,7 +153,7 @@ func createMsgDelegationForParams( stakingValue := int64(randRange(r, int(p.MinStakingValueSat), int(p.MaxStakingValueSat))) // always chose minimum unbonding time possible - unbondingTime := uint16(types.MinimumUnbondingTime(p, cp)) + 1 + unbondingTime := p.MinUnbondingTimeBlocks testStakingInfo := datagen.GenBTCStakingSlashingInfo( r, @@ -167,7 +167,7 @@ func createMsgDelegationForParams( stakingValue, p.SlashingPkScript, p.SlashingRate, - unbondingTime, + uint16(unbondingTime), ) slashingSpendInfo, err := testStakingInfo.StakingInfo.SlashingPathSpendInfo() @@ -205,7 +205,7 @@ func createMsgDelegationForParams( fpPk, stkTxHash, stkOutputIdx, - unbondingTime, + uint16(unbondingTime), unbondingValue, p, ) @@ -871,7 +871,7 @@ func TestValidateParsedMessageAgainstTheParams(t *testing.T) { t.Run(tt.name, func(t *testing.T) { r := rand.New(rand.NewSource(time.Now().Unix())) - msg, params, checkpointParams := tt.fn(r, t) + msg, params, _ := tt.fn(r, t) parsed, err := types.ParseCreateDelegationMessage(msg) @@ -885,7 +885,6 @@ func TestValidateParsedMessageAgainstTheParams(t *testing.T) { got, err := types.ValidateParsedMessageAgainstTheParams( parsed, params, - checkpointParams, &chaincfg.MainNetParams, ) @@ -895,9 +894,7 @@ func TestValidateParsedMessageAgainstTheParams(t *testing.T) { } else { require.NoError(t, err) require.NotNil(t, got) - - minUnbondingTime := types.MinimumUnbondingTime(params, checkpointParams) - require.Equal(t, minUnbondingTime, got.MinUnbondingTime) + require.Equal(t, params.MinUnbondingTimeBlocks, got.MinUnbondingTime) } }) diff --git a/x/finality/keeper/power_dist_change_test.go b/x/finality/keeper/power_dist_change_test.go index 86bee07d0..6da33ee5a 100644 --- a/x/finality/keeper/power_dist_change_test.go +++ b/x/finality/keeper/power_dist_change_test.go @@ -431,6 +431,8 @@ func FuzzBTCDelegationEvents_NoPreApproval(f *testing.F) { stakingValue := int64(2 * 10e8) delSK, _, err := datagen.GenRandomBTCKeyPair(r) h.NoError(err) + + stakingParams := h.BTCStakingKeeper.GetParamsWithVersion(h.Ctx).Params stakingTxHash, msgCreateBTCDel, actualDel, _, _, _, err := h.CreateDelegation( r, delSK, @@ -439,7 +441,7 @@ func FuzzBTCDelegationEvents_NoPreApproval(f *testing.F) { stakingValue, 1000, 0, - 0, + uint16(stakingParams.MinUnbondingTimeBlocks), false, false, ) @@ -454,13 +456,8 @@ func FuzzBTCDelegationEvents_NoPreApproval(f *testing.F) { events := h.BTCStakingKeeper.GetAllPowerDistUpdateEvents(h.Ctx, btcTip.Height, btcTip.Height) require.Len(t, events, 0) - btckptParams := btccKeeper.GetParams(h.Ctx) - stakingParams := h.BTCStakingKeeper.GetParamsWithVersion(h.Ctx).Params - - minUnbondingTime := types.MinimumUnbondingTime(&stakingParams, &btckptParams) - - // the BTC delegation will be unbonded at end height - max(w, min_unbonding_time) - unbondedHeight := actualDel.EndHeight - minUnbondingTime + // the BTC delegation will be unbonded at end height - min_unbonding_time + unbondedHeight := actualDel.EndHeight - stakingParams.MinUnbondingTimeBlocks events = h.BTCStakingKeeper.GetAllPowerDistUpdateEvents(h.Ctx, unbondedHeight, unbondedHeight) require.Len(t, events, 1) btcDelStateUpdate := events[0].GetBtcDelStateUpdate() @@ -606,13 +603,8 @@ func FuzzBTCDelegationEvents_WithPreApproval(f *testing.F) { require.Equal(t, stakingTxHash, btcDelStateUpdate.StakingTxHash) require.Equal(t, types.BTCDelegationStatus_ACTIVE, btcDelStateUpdate.NewState) - btckptParams := btccKeeper.GetParams(h.Ctx) - stakingParams := h.BTCStakingKeeper.GetParamsWithVersion(h.Ctx).Params - - minUnbondingTime := types.MinimumUnbondingTime(&stakingParams, &btckptParams) - - // the BTC delegation will be unbonded at end height - max(w, min_unbonding_time) - unbondedHeight := activatedDel.EndHeight - minUnbondingTime + // the BTC delegation will be unbonded at end height - minUnbondingTime + unbondedHeight := activatedDel.EndHeight - h.BTCStakingKeeper.GetParams(h.Ctx).MinUnbondingTimeBlocks events = h.BTCStakingKeeper.GetAllPowerDistUpdateEvents(h.Ctx, unbondedHeight, unbondedHeight) require.Len(t, events, 1) btcDelStateUpdate = events[0].GetBtcDelStateUpdate() @@ -678,6 +670,7 @@ func TestDoNotGenerateDuplicateEventsAfterHavingCovenantQuorum(t *testing.T) { stakingValue := int64(2 * 10e8) delSK, _, err := datagen.GenRandomBTCKeyPair(r) h.NoError(err) + stakingParams := h.BTCStakingKeeper.GetParamsWithVersion(h.Ctx).Params expectedStakingTxHash, msgCreateBTCDel, actualDel, _, _, _, err := h.CreateDelegation( r, delSK, @@ -686,7 +679,7 @@ func TestDoNotGenerateDuplicateEventsAfterHavingCovenantQuorum(t *testing.T) { stakingValue, 1000, 0, - 0, + uint16(stakingParams.MinUnbondingTimeBlocks), false, false, ) @@ -700,13 +693,8 @@ func TestDoNotGenerateDuplicateEventsAfterHavingCovenantQuorum(t *testing.T) { events := h.BTCStakingKeeper.GetAllPowerDistUpdateEvents(h.Ctx, btcTip.Height, btcTip.Height) require.Len(t, events, 0) - btckptParams := btccKeeper.GetParams(h.Ctx) - stakingParams := h.BTCStakingKeeper.GetParamsWithVersion(h.Ctx).Params - - minUnbondingTime := types.MinimumUnbondingTime(&stakingParams, &btckptParams) - - // the BTC delegation will be unbonded at end height - max(w, min_unbonding_time) - unbondedHeight := actualDel.EndHeight - minUnbondingTime + // the BTC delegation will be unbonded at end height - min_unbonding_time + unbondedHeight := actualDel.EndHeight - stakingParams.MinUnbondingTimeBlocks events = h.BTCStakingKeeper.GetAllPowerDistUpdateEvents(h.Ctx, unbondedHeight, unbondedHeight) require.Len(t, events, 1) btcDelStateUpdate := events[0].GetBtcDelStateUpdate() From 45639bb431737a598ba09e82a306f659e5eb668e Mon Sep 17 00:00:00 2001 From: Fangyu Gai Date: Tue, 19 Nov 2024 19:33:25 +0800 Subject: [PATCH 2/6] changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bcf52ef8f..2080b7fec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ## Unreleased +### State Breaking + +- [278](https://github.com/babylonlabs-io/babylon/pull/278) Allow unbonding time to be min unbonding value ### Bug fixes From b152dea8648ee4c3e761aecf293917aefc60a55c Mon Sep 17 00:00:00 2001 From: Fangyu Gai Date: Tue, 19 Nov 2024 20:17:42 +0800 Subject: [PATCH 3/6] add tests --- x/btccheckpoint/keeper/msg_server_test.go | 30 ++++++++++++++-- x/btcstaking/keeper/msg_server_test.go | 39 +++++++++++++++++++++ x/finality/keeper/power_dist_change_test.go | 4 +-- 3 files changed, 68 insertions(+), 5 deletions(-) diff --git a/x/btccheckpoint/keeper/msg_server_test.go b/x/btccheckpoint/keeper/msg_server_test.go index 8b234d59d..3d4c7266c 100644 --- a/x/btccheckpoint/keeper/msg_server_test.go +++ b/x/btccheckpoint/keeper/msg_server_test.go @@ -8,14 +8,17 @@ import ( "testing" "time" + "github.com/btcsuite/btcd/chaincfg" + sdk "github.com/cosmos/cosmos-sdk/types" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + "github.com/stretchr/testify/require" + dg "github.com/babylonlabs-io/babylon/testutil/datagen" keepertest "github.com/babylonlabs-io/babylon/testutil/keeper" bbn "github.com/babylonlabs-io/babylon/types" bkeeper "github.com/babylonlabs-io/babylon/x/btccheckpoint/keeper" btcctypes "github.com/babylonlabs-io/babylon/x/btccheckpoint/types" - "github.com/btcsuite/btcd/chaincfg" - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/stretchr/testify/require" ) type TestKeepers struct { @@ -78,6 +81,27 @@ func (k *TestKeepers) onTipChange() { k.BTCCheckpoint.OnTipChange(k.SdkCtx) } +func TestUpdateParams(t *testing.T) { + tk := InitTestKeepers(t) + + // Try to update params with a different checkpoint finalization timeout + msg := &btcctypes.MsgUpdateParams{ + Authority: authtypes.NewModuleAddress(govtypes.ModuleName).String(), + Params: btcctypes.Params{ + CheckpointFinalizationTimeout: btcctypes.DefaultParams().CheckpointFinalizationTimeout + 1, + }, + } + + _, err := tk.MsgSrv.UpdateParams(tk.Ctx, msg) + require.ErrorIs(t, err, govtypes.ErrInvalidProposalMsg, + "should not be able to change CheckpointFinalizationTimeout parameter") + + // Verify params were not changed + params := tk.BTCCheckpoint.GetParams(tk.SdkCtx) + require.Equal(t, btcctypes.DefaultParams().CheckpointFinalizationTimeout, params.CheckpointFinalizationTimeout, + "minUnbondingTime should remain unchanged") +} + func TestRejectDuplicatedSubmission(t *testing.T) { r := rand.New(rand.NewSource(time.Now().Unix())) epoch := uint64(1) diff --git a/x/btcstaking/keeper/msg_server_test.go b/x/btcstaking/keeper/msg_server_test.go index 3c0da809d..91a03db2f 100644 --- a/x/btcstaking/keeper/msg_server_test.go +++ b/x/btcstaking/keeper/msg_server_test.go @@ -13,6 +13,8 @@ import ( "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcd/wire" sdk "github.com/cosmos/cosmos-sdk/types" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" @@ -27,6 +29,43 @@ import ( "github.com/babylonlabs-io/babylon/x/btcstaking/types" ) +func FuzzMsgServer_UpdateParams(f *testing.F) { + datagen.AddRandomSeedsToFuzzer(f, 500) + + f.Fuzz(func(t *testing.T, seed int64) { + r := rand.New(rand.NewSource(seed)) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + // mock BTC light client and BTC checkpoint modules + btclcKeeper := types.NewMockBTCLightClientKeeper(ctrl) + btccKeeper := types.NewMockBtcCheckpointKeeper(ctrl) + h := testutil.NewHelper(t, btclcKeeper, btccKeeper) + + // set all parameters + h.GenAndApplyParams(r) + + params := h.BTCStakingKeeper.GetParams(h.Ctx) + ckptFinalizationTimeout := btccKeeper.GetParams(h.Ctx).CheckpointFinalizationTimeout + params.MinUnbondingTimeBlocks = uint32(r.Intn(int(ckptFinalizationTimeout))) + 1 + + // Try to update params with minUnbondingTime less than or equal to checkpointFinalizationTimeout + msg := &types.MsgUpdateParams{ + Authority: authtypes.NewModuleAddress(govtypes.ModuleName).String(), + Params: params, + } + + _, err := h.MsgServer.UpdateParams(h.Ctx, msg) + require.ErrorIs(t, err, govtypes.ErrInvalidProposalMsg, + "should not set minUnbondingTime to be less than checkpointFinalizationTimeout") + + // Try to update params with minUnbondingTime larger than checkpointFinalizationTimeout + msg.Params.MinUnbondingTimeBlocks = uint32(r.Intn(1000)) + ckptFinalizationTimeout + 1 + _, err = h.MsgServer.UpdateParams(h.Ctx, msg) + require.NoError(t, err) + }) +} + func FuzzMsgCreateFinalityProvider(f *testing.F) { datagen.AddRandomSeedsToFuzzer(f, 10) diff --git a/x/finality/keeper/power_dist_change_test.go b/x/finality/keeper/power_dist_change_test.go index 6da33ee5a..9f6d7e606 100644 --- a/x/finality/keeper/power_dist_change_test.go +++ b/x/finality/keeper/power_dist_change_test.go @@ -441,7 +441,7 @@ func FuzzBTCDelegationEvents_NoPreApproval(f *testing.F) { stakingValue, 1000, 0, - uint16(stakingParams.MinUnbondingTimeBlocks), + 0, false, false, ) @@ -679,7 +679,7 @@ func TestDoNotGenerateDuplicateEventsAfterHavingCovenantQuorum(t *testing.T) { stakingValue, 1000, 0, - uint16(stakingParams.MinUnbondingTimeBlocks), + 0, false, false, ) From afdf9580f9fc5c37b6ceb03ea0861c97b9612fb2 Mon Sep 17 00:00:00 2001 From: Fangyu Gai Date: Tue, 19 Nov 2024 22:04:52 +0800 Subject: [PATCH 4/6] fix e2e --- test/e2e/btc_staking_e2e_test.go | 17 +++++++++-------- test/e2e/btc_staking_pre_approval_e2e_test.go | 8 ++++---- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/test/e2e/btc_staking_e2e_test.go b/test/e2e/btc_staking_e2e_test.go index 0aeb4ebc2..bb020411b 100644 --- a/test/e2e/btc_staking_e2e_test.go +++ b/test/e2e/btc_staking_e2e_test.go @@ -17,9 +17,10 @@ import ( sdkmath "cosmossdk.io/math" feegrantcli "cosmossdk.io/x/feegrant/client/cli" - appparams "github.com/babylonlabs-io/babylon/app/params" sdk "github.com/cosmos/cosmos-sdk/types" + appparams "github.com/babylonlabs-io/babylon/app/params" + "github.com/babylonlabs-io/babylon/crypto/eots" "github.com/babylonlabs-io/babylon/test/e2e/configurer" "github.com/babylonlabs-io/babylon/test/e2e/configurer/chain" @@ -93,7 +94,7 @@ func (s *BTCStakingTestSuite) Test1CreateFinalityProviderAndDelegation() { params := nonValidatorNode.QueryBTCStakingParams() // minimal required unbonding time - unbondingTime := uint16(initialization.BabylonBtcFinalizationPeriod) + 1 + unbondingTime := params.MinUnbondingTimeBlocks // NOTE: we use the node's address for the BTC delegation stakerAddr := sdk.MustAccAddressFromBech32(nonValidatorNode.PublicAddress) @@ -508,7 +509,7 @@ func (s *BTCStakingTestSuite) Test6MultisigBTCDelegation() { params := nonValidatorNode.QueryBTCStakingParams() // minimal required unbonding time - unbondingTime := uint16(initialization.BabylonBtcFinalizationPeriod) + 1 + unbondingTime := params.MinUnbondingTimeBlocks // NOTE: we use the multisig address for the BTC delegation multisigStakerAddr := sdk.MustAccAddressFromBech32(multisigAddr) @@ -579,7 +580,7 @@ func (s *BTCStakingTestSuite) Test7BTCDelegationFeeGrant() { btcStkParams := nonValidatorNode.QueryBTCStakingParams() // minimal required unbonding time - unbondingTime := uint16(initialization.BabylonBtcFinalizationPeriod) + 1 + unbondingTime := btcStkParams.MinUnbondingTimeBlocks // NOTE: we use the grantee staker address for the BTC delegation PoP pop, err := bstypes.NewPoPBTC(granteeStakerAddr, s.delBTCSK) @@ -670,7 +671,7 @@ func (s *BTCStakingTestSuite) Test8BTCDelegationFeeGrantTyped() { btcStkParams := node.QueryBTCStakingParams() // minimal required unbonding time - unbondingTime := uint16(initialization.BabylonBtcFinalizationPeriod) + 1 + unbondingTime := btcStkParams.MinUnbondingTimeBlocks // NOTE: we use the grantee staker address for the BTC delegation PoP pop, err := bstypes.NewPoPBTC(granteeStakerAddr, s.delBTCSK) @@ -936,7 +937,7 @@ func (s *BTCStakingTestSuite) BTCStakingUnbondSlashInfo( ) { covenantBTCPKs := CovenantBTCPKs(params) // minimal required unbonding time - unbondingTime := uint16(initialization.BabylonBtcFinalizationPeriod) + 1 + unbondingTime := params.MinUnbondingTimeBlocks testStakingInfo = datagen.GenBTCStakingSlashingInfo( s.r, @@ -950,7 +951,7 @@ func (s *BTCStakingTestSuite) BTCStakingUnbondSlashInfo( s.stakingValue, params.SlashingPkScript, params.SlashingRate, - unbondingTime, + uint16(unbondingTime), ) // submit staking tx to Bitcoin and get inclusion proof @@ -985,7 +986,7 @@ func (s *BTCStakingTestSuite) BTCStakingUnbondSlashInfo( unbondingValue, params.SlashingPkScript, params.SlashingRate, - unbondingTime, + uint16(unbondingTime), ) stakingSlashingPathInfo, err := testStakingInfo.StakingInfo.SlashingPathSpendInfo() diff --git a/test/e2e/btc_staking_pre_approval_e2e_test.go b/test/e2e/btc_staking_pre_approval_e2e_test.go index 996ce1c8a..f4030f22b 100644 --- a/test/e2e/btc_staking_pre_approval_e2e_test.go +++ b/test/e2e/btc_staking_pre_approval_e2e_test.go @@ -92,7 +92,7 @@ func (s *BTCStakingPreApprovalTestSuite) Test1CreateFinalityProviderAndDelegatio params := nonValidatorNode.QueryBTCStakingParams() // minimal required unbonding time - unbondingTime := uint16(initialization.BabylonBtcFinalizationPeriod) + 1 + unbondingTime := params.MinUnbondingTimeBlocks // NOTE: we use the node's address for the BTC delegation stakerAddr := sdk.MustAccAddressFromBech32(nonValidatorNode.PublicAddress) @@ -535,7 +535,7 @@ func (s *BTCStakingPreApprovalTestSuite) BTCStakingUnbondSlashInfo( ) { covenantBTCPKs := CovenantBTCPKs(params) // minimal required unbonding time - unbondingTime := uint16(initialization.BabylonBtcFinalizationPeriod) + 1 + unbondingTime := params.MinUnbondingTimeBlocks testStakingInfo = datagen.GenBTCStakingSlashingInfo( s.r, @@ -549,7 +549,7 @@ func (s *BTCStakingPreApprovalTestSuite) BTCStakingUnbondSlashInfo( s.stakingValue, params.SlashingPkScript, params.SlashingRate, - unbondingTime, + uint16(unbondingTime), ) // submit staking tx to Bitcoin and get inclusion proof @@ -584,7 +584,7 @@ func (s *BTCStakingPreApprovalTestSuite) BTCStakingUnbondSlashInfo( unbondingValue, params.SlashingPkScript, params.SlashingRate, - unbondingTime, + uint16(unbondingTime), ) stakingSlashingPathInfo, err := testStakingInfo.StakingInfo.SlashingPathSpendInfo() From 43ddd793054cdea50706c2f017bc60d5ceee34e5 Mon Sep 17 00:00:00 2001 From: Fangyu Gai Date: Wed, 20 Nov 2024 11:48:27 +0800 Subject: [PATCH 5/6] pr comments --- x/btccheckpoint/keeper/msg_server.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x/btccheckpoint/keeper/msg_server.go b/x/btccheckpoint/keeper/msg_server.go index 81bff0830..c46fd6fdf 100644 --- a/x/btccheckpoint/keeper/msg_server.go +++ b/x/btccheckpoint/keeper/msg_server.go @@ -110,6 +110,8 @@ func (ms msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdatePara return nil, govtypes.ErrInvalidProposalMsg.Wrapf("invalid parameter: %v", err) } + // CheckpointFinalizationTimeout must remain immutable as changing it + // breaks a lot of system assumption ctx := sdk.UnwrapSDKContext(goCtx) if req.Params.CheckpointFinalizationTimeout != ms.k.GetParams(ctx).CheckpointFinalizationTimeout { return nil, govtypes.ErrInvalidProposalMsg.Wrapf("the checkpoint finalization timeout cannot be changed") From 6b4abc761e3966366f68f87982ab0c678132003d Mon Sep 17 00:00:00 2001 From: Fangyu Gai Date: Wed, 20 Nov 2024 15:13:03 +0800 Subject: [PATCH 6/6] remove parsed min-unbonding-time --- x/btcstaking/keeper/msg_server.go | 4 ++-- x/btcstaking/types/validate_parsed_message.go | 2 -- x/btcstaking/types/validate_parsed_message_test.go | 2 -- x/finality/keeper/power_dist_change_test.go | 2 +- 4 files changed, 3 insertions(+), 7 deletions(-) diff --git a/x/btcstaking/keeper/msg_server.go b/x/btcstaking/keeper/msg_server.go index aee41657d..4f74446d7 100644 --- a/x/btcstaking/keeper/msg_server.go +++ b/x/btcstaking/keeper/msg_server.go @@ -215,7 +215,7 @@ func (ms msgServer) CreateBTCDelegation(goCtx context.Context, req *types.MsgCre btcutil.NewTx(parsedMsg.StakingTx.Transaction), btccParams.BtcConfirmationDepth, uint32(parsedMsg.StakingTime), - paramsValidationResult.MinUnbondingTime, + vp.Params.MinStakingTimeBlocks, parsedMsg.StakingTxProofOfInclusion) if err != nil { return nil, fmt.Errorf("invalid inclusion proof: %w", err) @@ -259,7 +259,7 @@ func (ms msgServer) CreateBTCDelegation(goCtx context.Context, req *types.MsgCre } // add this BTC delegation, and emit corresponding events - if err := ms.AddBTCDelegation(ctx, newBTCDel, paramsValidationResult.MinUnbondingTime); err != nil { + if err := ms.AddBTCDelegation(ctx, newBTCDel, vp.Params.MinUnbondingTimeBlocks); err != nil { panic(fmt.Errorf("failed to add BTC delegation that has passed verification: %w", err)) } diff --git a/x/btcstaking/types/validate_parsed_message.go b/x/btcstaking/types/validate_parsed_message.go index 5a762c717..c7f9e577d 100644 --- a/x/btcstaking/types/validate_parsed_message.go +++ b/x/btcstaking/types/validate_parsed_message.go @@ -13,7 +13,6 @@ import ( type ParamsValidationResult struct { StakingOutputIdx uint32 UnbondingOutputIdx uint32 - MinUnbondingTime uint32 } // ValidateParsedMessageAgainstTheParams validates parsed message against parameters @@ -201,6 +200,5 @@ func ValidateParsedMessageAgainstTheParams( return &ParamsValidationResult{ StakingOutputIdx: stakingOutputIdx, UnbondingOutputIdx: 0, // unbonding output always has only 1 output - MinUnbondingTime: parameters.MinUnbondingTimeBlocks, }, nil } diff --git a/x/btcstaking/types/validate_parsed_message_test.go b/x/btcstaking/types/validate_parsed_message_test.go index eeb1d1f5f..93515e826 100644 --- a/x/btcstaking/types/validate_parsed_message_test.go +++ b/x/btcstaking/types/validate_parsed_message_test.go @@ -894,9 +894,7 @@ func TestValidateParsedMessageAgainstTheParams(t *testing.T) { } else { require.NoError(t, err) require.NotNil(t, got) - require.Equal(t, params.MinUnbondingTimeBlocks, got.MinUnbondingTime) } - }) } } diff --git a/x/finality/keeper/power_dist_change_test.go b/x/finality/keeper/power_dist_change_test.go index 9f6d7e606..f23c02fc7 100644 --- a/x/finality/keeper/power_dist_change_test.go +++ b/x/finality/keeper/power_dist_change_test.go @@ -686,7 +686,7 @@ func TestDoNotGenerateDuplicateEventsAfterHavingCovenantQuorum(t *testing.T) { h.NoError(err) /* at this point, there should be 1 event that BTC delegation - will become expired at end height - w + will become expired at end height - min_unbonding_time */ // there exists no event at the current BTC tip btcTip := btclcKeeper.GetTipInfo(h.Ctx)