From 07dc0d13f3b8a33e240d68827dc5220db8d4522c Mon Sep 17 00:00:00 2001 From: Cirrus Gai Date: Fri, 22 Nov 2024 13:18:33 +0800 Subject: [PATCH] hotfix: Invalid minUnbondingTime for verifying inclusion proof (#289) The bug is caused due to setting `params.MinStakingTime` to the btcstaking module's keeper method `VerifyInclusionProofAndGetHeight` other than `params.MinUnbondingTime`. In the fix, we remove `minUnbondingTime` from the parameter list of `VerifyInclusionProofAndGetHeight` as it should retrieve the parameter within the method. This PR also added comprehensive tests for `VerifyInclusionProofAndGetHeight` --- CHANGELOG.md | 4 + testutil/btcstaking-helper/keeper.go | 2 +- testutil/datagen/btc_transaction.go | 29 ++- x/btcstaking/keeper/btc_delegations.go | 2 +- x/btcstaking/keeper/genesis_test.go | 2 +- x/btcstaking/keeper/inclusion_proof.go | 20 ++- x/btcstaking/keeper/inclusion_proof_test.go | 187 ++++++++++++++++++++ x/btcstaking/keeper/msg_server.go | 12 +- x/btcstaking/keeper/msg_server_test.go | 30 +++- x/btcstaking/types/params.go | 2 +- 10 files changed, 263 insertions(+), 27 deletions(-) create mode 100644 x/btcstaking/keeper/inclusion_proof_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 982f2ce61..a3f04bc97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ## Unreleased +### Bug fixes + +- [#289](https://github.com/babylonlabs-io/babylon/pull/289) hotfix: Invalid minUnbondingTime for verifying inclusion proof + ## v0.17.0 ### State Breaking diff --git a/testutil/btcstaking-helper/keeper.go b/testutil/btcstaking-helper/keeper.go index eb80fb3e0..c5313cc43 100644 --- a/testutil/btcstaking-helper/keeper.go +++ b/testutil/btcstaking-helper/keeper.go @@ -159,7 +159,7 @@ func (h *Helper) GenAndApplyCustomParams( CovenantQuorum: 3, MinStakingValueSat: 1000, MaxStakingValueSat: int64(4 * 10e8), - MinStakingTimeBlocks: 10, + MinStakingTimeBlocks: 400, MaxStakingTimeBlocks: 10000, SlashingPkScript: slashingPkScript, MinSlashingTxFeeSat: 10, diff --git a/testutil/datagen/btc_transaction.go b/testutil/datagen/btc_transaction.go index dae95d047..ea5277e5a 100644 --- a/testutil/datagen/btc_transaction.go +++ b/testutil/datagen/btc_transaction.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/binary" "encoding/hex" + "fmt" "math" "math/rand" "runtime" @@ -331,7 +332,7 @@ func CreateBlockWithTransaction( proof, err := btcctypes.SpvProofFromHeaderAndTransactions(&headerBytes, txBytes, 1) if err != nil { - panic("could not calculate proof") + panic(fmt.Errorf("could not calculate proof: %w", err)) } return &BtcHeaderWithProof{ @@ -340,6 +341,32 @@ func CreateBlockWithTransaction( } } +func CreateDummyTx() *wire.MsgTx { + // Create a witness transaction instead of empty transaction + msgTx := wire.NewMsgTx(wire.TxVersion) + + // Add a dummy input + prevOut := wire.NewOutPoint(&chainhash.Hash{}, 0) + txIn := wire.NewTxIn(prevOut, nil, [][]byte{ + {0x01}, // Simple witness script + }) + msgTx.AddTxIn(txIn) + + // Add a dummy output + pkScript := []byte{ + 0x00, // Version 0 witness program + 0x14, // Push 20 bytes + 0x01, 0x02, 0x03, 0x04, 0x05, // Dummy public key hash (20 bytes) + 0x06, 0x07, 0x08, 0x09, 0x0a, + 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, + 0x10, 0x11, 0x12, 0x13, 0x14, + } + txOut := wire.NewTxOut(100000, pkScript) + msgTx.AddTxOut(txOut) + + return msgTx +} + func GenRandomTx(r *rand.Rand) *wire.MsgTx { // structure of the below tx is from https://github.com/btcsuite/btcd/blob/master/wire/msgtx_test.go tx := &wire.MsgTx{ diff --git a/x/btcstaking/keeper/btc_delegations.go b/x/btcstaking/keeper/btc_delegations.go index 52393abdc..83cf1b571 100644 --- a/x/btcstaking/keeper/btc_delegations.go +++ b/x/btcstaking/keeper/btc_delegations.go @@ -71,7 +71,7 @@ func (k Keeper) AddBTCDelegation( panic(fmt.Errorf("failed to emit EventBTCDelegationInclusionProofReceived for the new pending BTC delegation: %w", err)) } - // record event that the BTC delegation will become unbonded at endHeight-w + // record event that the BTC delegation will become unbonded at EndHeight-w // This event will be generated to subscribers as block event, when the // btc light client block height will reach btcDel.EndHeight-wValue unbondedEvent := types.NewEventPowerDistUpdateWithBTCDel(&types.EventBTCDelegationStateUpdate{ diff --git a/x/btcstaking/keeper/genesis_test.go b/x/btcstaking/keeper/genesis_test.go index c0e0bc4b7..72f1cd36b 100644 --- a/x/btcstaking/keeper/genesis_test.go +++ b/x/btcstaking/keeper/genesis_test.go @@ -81,7 +81,7 @@ func TestExportGenesis(t *testing.T) { DelBtcPk: del.BtcPk, } - // record event that the BTC delegation will become unbonded at endHeight-w + // record event that the BTC delegation will become unbonded at EndHeight-w unbondedEvent := types.NewEventPowerDistUpdateWithBTCDel(&types.EventBTCDelegationStateUpdate{ StakingTxHash: stakingTxHash.String(), NewState: types.BTCDelegationStatus_UNBONDED, diff --git a/x/btcstaking/keeper/inclusion_proof.go b/x/btcstaking/keeper/inclusion_proof.go index acedcba29..d3f7163d2 100644 --- a/x/btcstaking/keeper/inclusion_proof.go +++ b/x/btcstaking/keeper/inclusion_proof.go @@ -10,13 +10,15 @@ import ( "github.com/babylonlabs-io/babylon/x/btcstaking/types" ) -type delegationTimeRangeInfo struct { - startHeight uint32 - endHeight uint32 +type DelegationTimeRangeInfo struct { + StartHeight uint32 + EndHeight uint32 } // VerifyInclusionProofAndGetHeight verifies the inclusion proof of the given staking tx // and returns the start height and end height +// Note: the `minUnbondingTime` passed here should be from the corresponding params +// of the staking tx func (k Keeper) VerifyInclusionProofAndGetHeight( ctx sdk.Context, stakingTx *btcutil.Tx, @@ -24,7 +26,7 @@ func (k Keeper) VerifyInclusionProofAndGetHeight( stakingTime uint32, minUnbondingTime uint32, inclusionProof *types.ParsedProofOfInclusion, -) (*delegationTimeRangeInfo, error) { +) (*DelegationTimeRangeInfo, error) { // Check: // - timelock of staking tx // - staking tx is k-deep @@ -58,13 +60,15 @@ func (k Keeper) VerifyInclusionProofAndGetHeight( if stakingTxDepth < confirmationDepth { return nil, types.ErrInvalidStakingTx.Wrapf("not k-deep: k=%d; depth=%d", confirmationDepth, stakingTxDepth) } + // ensure staking tx's timelock has more than unbonding BTC blocks left if btcTip.Height+minUnbondingTime >= endHeight { - return nil, types.ErrInvalidStakingTx.Wrapf("staking tx's timelock has no more than unbonding(=%d) blocks left", minUnbondingTime) + return nil, types.ErrInvalidStakingTx. + Wrapf("staking tx's timelock has no more than unbonding(=%d) blocks left", minUnbondingTime) } - return &delegationTimeRangeInfo{ - startHeight: startHeight, - endHeight: endHeight, + return &DelegationTimeRangeInfo{ + StartHeight: startHeight, + EndHeight: endHeight, }, nil } diff --git a/x/btcstaking/keeper/inclusion_proof_test.go b/x/btcstaking/keeper/inclusion_proof_test.go new file mode 100644 index 000000000..05066d179 --- /dev/null +++ b/x/btcstaking/keeper/inclusion_proof_test.go @@ -0,0 +1,187 @@ +package keeper_test + +import ( + "math/rand" + "testing" + + "github.com/btcsuite/btcd/btcutil" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" + + testutil "github.com/babylonlabs-io/babylon/testutil/btcstaking-helper" + "github.com/babylonlabs-io/babylon/testutil/datagen" + bbntypes "github.com/babylonlabs-io/babylon/types" + btclctypes "github.com/babylonlabs-io/babylon/x/btclightclient/types" + "github.com/babylonlabs-io/babylon/x/btcstaking/types" +) + +func FuzzVerifyInclusionProofAndGetHeight(f *testing.F) { + datagen.AddRandomSeedsToFuzzer(f, 100) + + 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) + + // generate dummy staking tx data + msgTx := datagen.CreateDummyTx() + stakingTx := btcutil.NewTx(msgTx) + confirmationDepth := uint32(6) + stakingTime := uint32(1000) + + params := h.BTCStakingKeeper.GetParams(h.Ctx) + + // generate common merkle proof and inclusion header + prevBlock, _ := datagen.GenRandomBtcdBlock(r, 0, nil) + btcHeaderWithProof := datagen.CreateBlockWithTransaction(r, &prevBlock.Header, msgTx) + headerHash := btcHeaderWithProof.HeaderBytes.Hash() + headerBytes, err := bbntypes.NewBTCHeaderBytesFromBytes(btcHeaderWithProof.HeaderBytes) + require.NoError(t, err) + inclusionHeight := uint32(datagen.RandomInt(r, 1000) + 1) + inclusionHeader := &btclctypes.BTCHeaderInfo{ + Header: &headerBytes, + Height: inclusionHeight, + } + + // create inclusion proof + proof := &types.ParsedProofOfInclusion{ + HeaderHash: headerHash, + Proof: btcHeaderWithProof.SpvProof.MerkleNodes, + Index: btcHeaderWithProof.SpvProof.BtcTransactionIndex, + } + + // indicates the staking tx has room for unbonding + maxValidTipHeight := inclusionHeight + stakingTime - params.MinUnbondingTimeBlocks - 1 + // indicates the staking tx is k-deep + minValidTipHeight := inclusionHeight + confirmationDepth + + t.Run("successful verification", func(t *testing.T) { + // Set the tip height to be in the range of valid min and max tip height + tipHeight := datagen.RandomInt(r, int(maxValidTipHeight)-int(minValidTipHeight)+1) + uint64(minValidTipHeight) + mockTipHeaderInfo := &btclctypes.BTCHeaderInfo{Height: uint32(tipHeight)} + + btclcKeeper.EXPECT().GetHeaderByHash(gomock.Any(), headerHash).Return(inclusionHeader).Times(1) + btclcKeeper.EXPECT().GetTipInfo(gomock.Any()).Return(mockTipHeaderInfo).Times(1) + + // Verify inclusion proof + timeRange, err := h.BTCStakingKeeper.VerifyInclusionProofAndGetHeight( + h.Ctx, + stakingTx, + confirmationDepth, + stakingTime, + params.MinUnbondingTimeBlocks, + proof, + ) + + require.NoError(t, err) + require.Equal(t, inclusionHeader.Height, timeRange.StartHeight) + require.Equal(t, inclusionHeader.Height+stakingTime, timeRange.EndHeight) + }) + + t.Run("nil inclusion header", func(t *testing.T) { + // set the returned inclusion header as nil + btclcKeeper.EXPECT().GetHeaderByHash(gomock.Any(), headerHash).Return(nil).Times(1) + + // Verify inclusion proof + _, err := h.BTCStakingKeeper.VerifyInclusionProofAndGetHeight( + h.Ctx, + stakingTx, + confirmationDepth, + stakingTime, + params.MinUnbondingTimeBlocks, + proof, + ) + + require.ErrorContains(t, err, "header that includes the staking tx is not found") + }) + + t.Run("invalid proof", func(t *testing.T) { + btclcKeeper.EXPECT().GetHeaderByHash(gomock.Any(), headerHash).Return(inclusionHeader).Times(1) + + copyProof := *proof + // make the proof invalid by setting the index to a different value + copyProof.Index = proof.Index + 1 + _, err = h.BTCStakingKeeper.VerifyInclusionProofAndGetHeight( + h.Ctx, + stakingTx, + confirmationDepth, + stakingTime, + params.MinUnbondingTimeBlocks, + ©Proof, + ) + + require.ErrorContains(t, err, "not included in the Bitcoin chain") + }) + + t.Run("insufficient confirmation depth", func(t *testing.T) { + tipHeight := inclusionHeight + uint32(datagen.RandomInt(r, int(confirmationDepth))) + mockTipHeaderInfo := &btclctypes.BTCHeaderInfo{Height: tipHeight} + + btclcKeeper.EXPECT().GetHeaderByHash(gomock.Any(), headerHash).Return(inclusionHeader).Times(1) + btclcKeeper.EXPECT().GetTipInfo(gomock.Any()).Return(mockTipHeaderInfo).Times(1) + + // Verify inclusion proof + _, err = h.BTCStakingKeeper.VerifyInclusionProofAndGetHeight( + h.Ctx, + stakingTx, + confirmationDepth, + stakingTime, + params.MinUnbondingTimeBlocks, + proof, + ) + + require.ErrorContains(t, err, "not k-deep") + }) + + t.Run("insufficient unbonding time", func(t *testing.T) { + tipHeight := datagen.RandomInt(r, 1000) + uint64(maxValidTipHeight) + 1 + mockTipHeaderInfo := &btclctypes.BTCHeaderInfo{Height: uint32(tipHeight)} + + btclcKeeper.EXPECT().GetHeaderByHash(gomock.Any(), headerHash).Return(inclusionHeader).Times(1) + btclcKeeper.EXPECT().GetTipInfo(gomock.Any()).Return(mockTipHeaderInfo).Times(1) + + // Verify inclusion proof + _, err = h.BTCStakingKeeper.VerifyInclusionProofAndGetHeight( + h.Ctx, + stakingTx, + confirmationDepth, + stakingTime, + params.MinUnbondingTimeBlocks, + proof, + ) + + require.ErrorContains(t, err, "staking tx's timelock has no more than unbonding") + }) + + t.Run("invalid min unbonding time", func(t *testing.T) { + // Set the tip height to be in the range of valid min and max tip height + tipHeight := datagen.RandomInt(r, int(maxValidTipHeight)-int(minValidTipHeight)+1) + uint64(minValidTipHeight) + mockTipHeaderInfo := &btclctypes.BTCHeaderInfo{Height: uint32(tipHeight)} + + btclcKeeper.EXPECT().GetHeaderByHash(gomock.Any(), headerHash).Return(inclusionHeader).Times(1) + btclcKeeper.EXPECT().GetTipInfo(gomock.Any()).Return(mockTipHeaderInfo).Times(1) + + // an invalid min_unbonding_time should be >= end_height - tip_height + invalidMinUnbondingTime := uint32(datagen.RandomInt(r, 1000)) + inclusionHeight + stakingTime - uint32(tipHeight) + + _, err = h.BTCStakingKeeper.VerifyInclusionProofAndGetHeight( + h.Ctx, + stakingTx, + confirmationDepth, + stakingTime, + invalidMinUnbondingTime, + proof, + ) + + require.ErrorContains(t, err, "staking tx's timelock has no more than unbonding") + }) + }) +} diff --git a/x/btcstaking/keeper/msg_server.go b/x/btcstaking/keeper/msg_server.go index 4f74446d7..61b2e8392 100644 --- a/x/btcstaking/keeper/msg_server.go +++ b/x/btcstaking/keeper/msg_server.go @@ -215,14 +215,14 @@ func (ms msgServer) CreateBTCDelegation(goCtx context.Context, req *types.MsgCre btcutil.NewTx(parsedMsg.StakingTx.Transaction), btccParams.BtcConfirmationDepth, uint32(parsedMsg.StakingTime), - vp.Params.MinStakingTimeBlocks, + vp.Params.MinUnbondingTimeBlocks, parsedMsg.StakingTxProofOfInclusion) if err != nil { return nil, fmt.Errorf("invalid inclusion proof: %w", err) } - startHeight = timeInfo.startHeight - endHeight = timeInfo.endHeight + startHeight = timeInfo.StartHeight + endHeight = timeInfo.EndHeight } else { // NOTE: here we consume more gas to protect Babylon chain and covenant members against spamming // i.e creating delegation that will never reach BTC @@ -324,8 +324,8 @@ func (ms msgServer) AddBTCDelegationInclusionProof( } // 6. set start height and end height and save it to db - btcDel.StartHeight = timeInfo.startHeight - btcDel.EndHeight = timeInfo.endHeight + btcDel.StartHeight = timeInfo.StartHeight + btcDel.EndHeight = timeInfo.EndHeight ms.setBTCDelegation(ctx, btcDel) // 7. emit events @@ -351,7 +351,7 @@ func (ms msgServer) AddBTCDelegationInclusionProof( btcTip := ms.btclcKeeper.GetTipInfo(ctx) ms.addPowerDistUpdateEvent(ctx, btcTip.Height, activeEvent) - // record event that the BTC delegation will become unbonded at endHeight-w + // record event that the BTC delegation will become unbonded at EndHeight-w unbondedEvent := types.NewEventPowerDistUpdateWithBTCDel(&types.EventBTCDelegationStateUpdate{ StakingTxHash: req.StakingTxHash, NewState: types.BTCDelegationStatus_UNBONDED, diff --git a/x/btcstaking/keeper/msg_server_test.go b/x/btcstaking/keeper/msg_server_test.go index 91a03db2f..0f04dfb74 100644 --- a/x/btcstaking/keeper/msg_server_test.go +++ b/x/btcstaking/keeper/msg_server_test.go @@ -26,6 +26,7 @@ import ( testhelper "github.com/babylonlabs-io/babylon/testutil/helper" bbn "github.com/babylonlabs-io/babylon/types" btcctypes "github.com/babylonlabs-io/babylon/x/btccheckpoint/types" + btclctypes "github.com/babylonlabs-io/babylon/x/btclightclient/types" "github.com/babylonlabs-io/babylon/x/btcstaking/types" ) @@ -736,7 +737,8 @@ func TestDoNotAllowDelegationWithoutFinalityProvider(t *testing.T) { // We only generate a finality provider, but not insert it into KVStore. So later // insertion of delegation should fail. - _, fpPK, err := datagen.GenRandomBTCKeyPair(r) + + fp, err := datagen.GenRandomFinalityProvider(r) require.NoError(t, err) /* @@ -744,17 +746,17 @@ func TestDoNotAllowDelegationWithoutFinalityProvider(t *testing.T) { */ delSK, _, err := datagen.GenRandomBTCKeyPair(r) require.NoError(t, err) - stakingTimeBlocks := uint16(5) + stakingTimeBlocks := bsParams.MinStakingTimeBlocks stakingValue := int64(2 * 10e8) testStakingInfo := datagen.GenBTCStakingSlashingInfo( r, t, h.Net, delSK, - []*btcec.PublicKey{fpPK}, + []*btcec.PublicKey{fp.BtcPk.MustToBTCPK()}, covenantPKs, bsParams.CovenantQuorum, - stakingTimeBlocks, + uint16(stakingTimeBlocks), stakingValue, bsParams.SlashingPkScript, bsParams.SlashingRate, @@ -793,14 +795,14 @@ func TestDoNotAllowDelegationWithoutFinalityProvider(t *testing.T) { require.NoError(t, err) stkTxHash := testStakingInfo.StakingTx.TxHash() - unbondingTime := 100 + 1 + unbondingTime := bsParams.MinUnbondingTimeBlocks unbondingValue := stakingValue - datagen.UnbondingTxFee // TODO: parameterise fee testUnbondingInfo := datagen.GenBTCUnbondingSlashingInfo( r, t, h.Net, delSK, - []*btcec.PublicKey{fpPK}, + []*btcec.PublicKey{fp.BtcPk.MustToBTCPK()}, covenantPKs, bsParams.CovenantQuorum, wire.NewOutPoint(&stkTxHash, datagen.StakingOutIdx), @@ -816,10 +818,9 @@ func TestDoNotAllowDelegationWithoutFinalityProvider(t *testing.T) { h.NoError(err) // all good, construct and send MsgCreateBTCDelegation message - fpBTCPK := bbn.NewBIP340PubKeyFromBTCPK(fpPK) msgCreateBTCDel := &types.MsgCreateBTCDelegation{ StakerAddr: stakerAddr.String(), - FpBtcPkList: []bbn.BIP340PubKey{*fpBTCPK}, + FpBtcPkList: []bbn.BIP340PubKey{*fp.BtcPk}, BtcPk: bbn.NewBIP340PubKeyFromBTCPK(delSK.PubKey()), Pop: pop, StakingTime: uint32(stakingTimeBlocks), @@ -838,6 +839,19 @@ func TestDoNotAllowDelegationWithoutFinalityProvider(t *testing.T) { _, err = h.MsgServer.CreateBTCDelegation(h.Ctx, msgCreateBTCDel) require.Error(t, err) require.True(t, errors.Is(err, types.ErrFpNotFound)) + + AddFinalityProvider(t, h.Ctx, *h.BTCStakingKeeper, fp) + inclusionHeight := uint32(100) + inclusionHeader := &btclctypes.BTCHeaderInfo{ + Header: &btcHeader, + Height: inclusionHeight, + } + tipHeight := 150 + mockTipHeaderInfo := &btclctypes.BTCHeaderInfo{Height: uint32(tipHeight)} + btclcKeeper.EXPECT().GetHeaderByHash(gomock.Any(), btcHeader.Hash()).Return(inclusionHeader).Times(1) + btclcKeeper.EXPECT().GetTipInfo(gomock.Any()).Return(mockTipHeaderInfo).Times(1) + _, err = h.MsgServer.CreateBTCDelegation(h.Ctx, msgCreateBTCDel) + require.NoError(t, err) } func TestCorrectUnbondingTimeInDelegation(t *testing.T) { diff --git a/x/btcstaking/types/params.go b/x/btcstaking/types/params.go index b565920d7..fa982524e 100644 --- a/x/btcstaking/types/params.go +++ b/x/btcstaking/types/params.go @@ -65,7 +65,7 @@ func DefaultParams() Params { CovenantQuorum: quorum, MinStakingValueSat: 1000, MaxStakingValueSat: 10 * 10e8, - MinStakingTimeBlocks: 10, + MinStakingTimeBlocks: 400, // this should be larger than minUnbonding MaxStakingTimeBlocks: math.MaxUint16, SlashingPkScript: defaultSlashingPkScript(), MinSlashingTxFeeSat: 1000,