diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f02c492c..f3068105c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ## Unreleased +### State Machine Breaking + +* [#130](https://github.com/babylonlabs-io/babylon/pull/130) Fix bugs in the +transaction fee refunding mechanism for covenant signatures and finality signatures +* [#125](https://github.com/babylonlabs-io/babylon/pull/125) Implement ADR-028 and +refund transaction fee for certain transactions from protocol stakeholders + ### Misc Improvements * [#113](https://github.com/babylonlabs-io/babylon/pull/113) Add multibuild binary @@ -46,8 +53,6 @@ for upgrade handler `testnet` and `mainnet`. ### State Machine Breaking -* [#125](https://github.com/babylonlabs-io/babylon/pull/125) Implement ADR-028 and -refund transaction fee for certain transactions from protocol stakeholders * [#107](https://github.com/babylonlabs-io/babylon/pull/107) Implement ADR-027 and enable in-protocol minimum gas price * [#103](https://github.com/babylonlabs-io/babylon/pull/103) Add token distribution diff --git a/x/btccheckpoint/keeper/msg_server.go b/x/btccheckpoint/keeper/msg_server.go index dc3cbfd93..22c5421c9 100644 --- a/x/btccheckpoint/keeper/msg_server.go +++ b/x/btccheckpoint/keeper/msg_server.go @@ -92,8 +92,8 @@ func (ms msgServer) InsertBTCSpvProof(ctx context.Context, req *types.MsgInsertB return nil, err } - // At this point, the BTC checkpoint is considered the first valid submission - // for the epoch. + // At this point, the BTC checkpoint is a valid submission and is + // not duplicated (first time seeing the pair of BTC txs) // Thus, we can safely consider this message as refundable ms.k.incentiveKeeper.IndexRefundableMsg(sdkCtx, req) diff --git a/x/btcstaking/keeper/keeper_test.go b/x/btcstaking/keeper/keeper_test.go index 78ccf6e9d..35b30f310 100644 --- a/x/btcstaking/keeper/keeper_test.go +++ b/x/btcstaking/keeper/keeper_test.go @@ -75,8 +75,8 @@ func (h *Helper) NoError(err error) { require.NoError(h.t, err) } -func (h *Helper) Error(err error) { - require.Error(h.t, err) +func (h *Helper) Error(err error, msgAndArgs ...any) { + require.Error(h.t, err, msgAndArgs...) } func (h *Helper) GenAndApplyParams(r *rand.Rand) ([]*btcec.PrivateKey, []*btcec.PublicKey) { @@ -407,8 +407,8 @@ func (h *Helper) CreateCovenantSigs( h.NoError(err) covenantMsgs := h.GenerateCovenantSignaturesMessages(r, covenantSKs, msgCreateBTCDel, del) - for _, msg := range covenantMsgs { - msgCopy := msg + for i := 0; i < int(bsParams.CovenantQuorum); i++ { + msgCopy := covenantMsgs[i] _, err := h.MsgServer.AddCovenantSigs(h.Ctx, msgCopy) h.NoError(err) } diff --git a/x/btcstaking/keeper/msg_server.go b/x/btcstaking/keeper/msg_server.go index 0fe2747dd..575f61aa1 100644 --- a/x/btcstaking/keeper/msg_server.go +++ b/x/btcstaking/keeper/msg_server.go @@ -353,21 +353,18 @@ func (ms msgServer) AddCovenantSigs(goCtx context.Context, req *types.MsgAddCove if btcDel.IsSignedByCovMember(req.Pk) && btcDel.BtcUndelegation.IsSignedByCovMember(req.Pk) { ms.Logger(ctx).Debug("Received duplicated covenant signature", "covenant pk", req.Pk.MarshalHex()) - return &types.MsgAddCovenantSigsResponse{}, nil + // return error if the covenant signature is already submitted + // this is to secure the tx refunding against duplicated messages + return nil, types.ErrDuplicatedCovenantSig } - if btcDel.HasCovenantQuorums(params.CovenantQuorum) { - ms.Logger(ctx).Debug("Received covenant signature after achieving quorum", "covenant pk", req.Pk.MarshalHex()) - return &types.MsgAddCovenantSigsResponse{}, nil - } - - // ensure BTC delegation is still pending, i.e., not expired + // ensure BTC delegation is still pending, i.e., not unbonded btcTipHeight := ms.btclcKeeper.GetTipInfo(ctx).Height wValue := ms.btccKeeper.GetParams(ctx).CheckpointFinalizationTimeout status := btcDel.GetStatus(btcTipHeight, wValue, params.CovenantQuorum) - if status != types.BTCDelegationStatus_PENDING { - ms.Logger(ctx).Debug("Received covenant signature after the BTC delegation is already expired", "covenant pk", req.Pk.MarshalHex()) - return &types.MsgAddCovenantSigsResponse{}, nil + if status == types.BTCDelegationStatus_UNBONDED { + ms.Logger(ctx).Debug("Received covenant signature after the BTC delegation is already unbonded", "covenant pk", req.Pk.MarshalHex()) + return nil, types.ErrInvalidCovenantSig.Wrap("the BTC delegation is already unbonded") } // Check that the number of covenant sigs and number of the diff --git a/x/btcstaking/keeper/msg_server_test.go b/x/btcstaking/keeper/msg_server_test.go index e1cbaafbc..5d126c9f1 100644 --- a/x/btcstaking/keeper/msg_server_test.go +++ b/x/btcstaking/keeper/msg_server_test.go @@ -294,12 +294,12 @@ func FuzzAddCovenantSigs(f *testing.F) { _, err = h.MsgServer.AddCovenantSigs(h.Ctx, &bogusMsg) h.Error(err) - for _, msg := range msgs { + for i, msg := range msgs { _, err = h.MsgServer.AddCovenantSigs(h.Ctx, msg) h.NoError(err) - // check that submitting the same covenant signature does not produce an error + // check that submitting the same covenant signature returns error _, err = h.MsgServer.AddCovenantSigs(h.Ctx, msg) - h.NoError(err) + h.Error(err, "i: %d", i) } // ensure the BTC delegation now has voting power diff --git a/x/btcstaking/types/errors.go b/x/btcstaking/types/errors.go index 1c7ff999d..aa9f81779 100644 --- a/x/btcstaking/types/errors.go +++ b/x/btcstaking/types/errors.go @@ -32,4 +32,5 @@ var ( ErrParamsNotFound = errorsmod.Register(ModuleName, 1124, "the parameters are not found") ErrFpAlreadyJailed = errorsmod.Register(ModuleName, 1125, "the finality provider has already been jailed") ErrFpNotJailed = errorsmod.Register(ModuleName, 1126, "the finality provider is not jailed") + ErrDuplicatedCovenantSig = errorsmod.Register(ModuleName, 1127, "the covenant signature is already submitted") ) diff --git a/x/finality/keeper/msg_server.go b/x/finality/keeper/msg_server.go index ac8606d2f..6fcf5edfe 100644 --- a/x/finality/keeper/msg_server.go +++ b/x/finality/keeper/msg_server.go @@ -97,9 +97,10 @@ func (ms msgServer) AddFinalitySig(goCtx context.Context, req *types.MsgAddFinal } existingSig, err := ms.GetSig(ctx, req.BlockHeight, fpPK) if err == nil && existingSig.Equals(req.FinalitySig) { - ms.Logger(ctx).Debug("Received duplicated finiality vote", "block height", req.BlockHeight, "finality provider", req.FpBtcPk) - // exactly same vote already exists, return success to the provider - return &types.MsgAddFinalitySigResponse{}, nil + ms.Logger(ctx).Debug("Received duplicated finality vote", "block height", req.BlockHeight, "finality provider", req.FpBtcPk) + // exactly same vote already exists, return error + // this is to secure the tx refunding against duplicated messages + return nil, types.ErrDuplicatedFinalitySig } // find the timestamped public randomness commitment for this height from this finality provider diff --git a/x/finality/keeper/msg_server_test.go b/x/finality/keeper/msg_server_test.go index 4466bc764..50fc04076 100644 --- a/x/finality/keeper/msg_server_test.go +++ b/x/finality/keeper/msg_server_test.go @@ -104,7 +104,7 @@ func FuzzCommitPubRandList(f *testing.F) { } func FuzzAddFinalitySig(f *testing.F) { - datagen.AddRandomSeedsToFuzzer(f, 100) + datagen.AddRandomSeedsToFuzzer(f, 10) f.Fuzz(func(t *testing.T, seed int64) { r := rand.New(rand.NewSource(seed)) @@ -193,9 +193,8 @@ func FuzzAddFinalitySig(f *testing.F) { // Case 4: In case of duplicate vote return success bsKeeper.EXPECT().GetFinalityProvider(gomock.Any(), gomock.Eq(fpBTCPKBytes)).Return(fp, nil).Times(1) - resp, err := ms.AddFinalitySig(ctx, msg) - require.NoError(t, err) - require.NotNil(t, resp) + _, err = ms.AddFinalitySig(ctx, msg) + require.Error(t, err) // Case 5: the finality provider is slashed if it votes for a fork blockAppHash2 := datagen.GenRandomByteArray(r, 32) diff --git a/x/finality/types/errors.go b/x/finality/types/errors.go index d496d2b92..e7ffc6877 100644 --- a/x/finality/types/errors.go +++ b/x/finality/types/errors.go @@ -15,7 +15,8 @@ var ( ErrInvalidPubRand = errorsmod.Register(ModuleName, 1106, "the public randomness list is invalid") ErrEvidenceNotFound = errorsmod.Register(ModuleName, 1107, "evidence is not found") ErrInvalidFinalitySig = errorsmod.Register(ModuleName, 1108, "finality signature is not valid") - ErrNoSlashableEvidence = errorsmod.Register(ModuleName, 1109, "there is no slashable evidence") - ErrPubRandCommitNotBTCTimestamped = errorsmod.Register(ModuleName, 1110, "the public randomness commit is not BTC timestamped yet") - ErrJailingPeriodNotPassed = errorsmod.Register(ModuleName, 1111, "the jailing period is not passed") + ErrDuplicatedFinalitySig = errorsmod.Register(ModuleName, 1109, "duplicated finality vote") + ErrNoSlashableEvidence = errorsmod.Register(ModuleName, 1110, "there is no slashable evidence") + ErrPubRandCommitNotBTCTimestamped = errorsmod.Register(ModuleName, 1111, "the public randomness commit is not BTC timestamped yet") + ErrJailingPeriodNotPassed = errorsmod.Register(ModuleName, 1112, "the jailing period is not passed") ) diff --git a/x/incentive/keeper/refund_tx_decorator.go b/x/incentive/keeper/refund_tx_decorator.go index bd2c36921..edfde087f 100644 --- a/x/incentive/keeper/refund_tx_decorator.go +++ b/x/incentive/keeper/refund_tx_decorator.go @@ -33,32 +33,46 @@ func (d *RefundTxDecorator) PostHandle(ctx sdk.Context, tx sdk.Tx, simulate, suc return next(ctx, tx, simulate, success) } - refundableMsgHashList := make([][]byte, 0) + refundable := d.CheckTxAndClearIndex(ctx, tx) + + // if the tx is refundable, refund it + if refundable { + err := d.k.RefundTx(ctx, feeTx) + if err != nil { + d.k.Logger(ctx).Error("failed to refund tx", "error", err) + return next(ctx, tx, simulate, success) + } + } + + // move to the next PostHandler + return next(ctx, tx, simulate, success) +} + +// CheckTxAndClearIndex parses the given tx and returns a boolean indicating +// whether the tx is refundable, and clears the refundable messages from the index +// NOTE: a tx is refundable if all of its messages are unique and refundable +func (d *RefundTxDecorator) CheckTxAndClearIndex(ctx sdk.Context, tx sdk.Tx) bool { + // NOTE: we use a map to avoid duplicated refundable messages, as + // otherwise one can fill a tx with duplicate messages to bloat the blockchain + refundableMsgHashSet := make(map[string]struct{}) // iterate over all messages in the tx, and record whether they are refundable for _, msg := range tx.GetMsgs() { msgHash := types.HashMsg(msg) if d.k.HasRefundableMsg(ctx, msgHash) { - refundableMsgHashList = append(refundableMsgHashList, msgHash) + refundableMsgHashSet[string(msgHash)] = struct{}{} } } - // if all messages in the tx are refundable, refund the tx - if len(refundableMsgHashList) == len(tx.GetMsgs()) { - err := d.k.RefundTx(ctx, feeTx) - if err != nil { - d.k.Logger(ctx).Error("failed to refund tx", "error", err) - return next(ctx, tx, simulate, success) - } - } + // if all messages in the tx are unique and refundable, then this tx is refundable + refundable := len(refundableMsgHashSet) == len(tx.GetMsgs()) // remove the refundable messages from the index, regardless whether the tx is refunded or not - // NOTE: If the message with same hash shows up again, the refunding rule will + // NOTE: If the message with same hash shows up in a later tx, the refunding rule will // consider it duplicated and the tx will not be refunded - for _, msgHash := range refundableMsgHashList { - d.k.RemoveRefundableMsg(ctx, msgHash) + for msgHash := range refundableMsgHashSet { + d.k.RemoveRefundableMsg(ctx, []byte(msgHash)) } - // move to the next PostHandler - return next(ctx, tx, simulate, success) + return refundable } diff --git a/x/incentive/keeper/refund_tx_decorator_test.go b/x/incentive/keeper/refund_tx_decorator_test.go new file mode 100644 index 000000000..a9eabd360 --- /dev/null +++ b/x/incentive/keeper/refund_tx_decorator_test.go @@ -0,0 +1,104 @@ +package keeper_test + +import ( + "testing" + + keepertest "github.com/babylonlabs-io/babylon/testutil/keeper" + "github.com/babylonlabs-io/babylon/x/incentive/keeper" + "github.com/babylonlabs-io/babylon/x/incentive/types" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/reflect/protoreflect" +) + +var _ sdk.Tx = &TestTx{} + +type TestTx struct { + Msgs []sdk.Msg +} + +func (tx *TestTx) GetMsgs() []sdk.Msg { + return tx.Msgs +} + +func (tx *TestTx) GetMsgsV2() ([]protoreflect.ProtoMessage, error) { + return nil, nil +} + +func TestCheckTxAndClearIndex(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + iKeeper, ctx := keepertest.IncentiveKeeper(t, nil, nil, nil) + decorator := keeper.NewRefundTxDecorator(iKeeper) + + testCases := []struct { + name string + setup func(ctx sdk.Context) sdk.Tx + expected bool + }{ + { + name: "if all messages are unique and refundable, the tx is refundable", + setup: func(ctx sdk.Context) sdk.Tx { + msg1 := types.MsgWithdrawReward{ + Address: "address", + } + msg2 := types.MsgWithdrawReward{ + Address: "address2", + } + iKeeper.IndexRefundableMsg(ctx, &msg1) + iKeeper.IndexRefundableMsg(ctx, &msg2) + return &TestTx{Msgs: []sdk.Msg{&msg1, &msg2}} + }, + expected: true, + }, + { + name: "if some messages are not refundable, the tx is not refundable", + setup: func(ctx sdk.Context) sdk.Tx { + msg1 := types.MsgWithdrawReward{ + Address: "address", + } + msg2 := types.MsgWithdrawReward{ + Address: "address2", + } + iKeeper.IndexRefundableMsg(ctx, &msg1) + return &TestTx{Msgs: []sdk.Msg{&msg1, &msg2}} + }, + expected: false, + }, + { + name: "if some messages are duplicated, the tx is not refundable", + setup: func(ctx sdk.Context) sdk.Tx { + msg := types.MsgWithdrawReward{ + Address: "address", + } + msg2 := types.MsgWithdrawReward{ + Address: "address", + } + msg3 := types.MsgWithdrawReward{ + Address: "address2", + } + iKeeper.IndexRefundableMsg(ctx, &msg) + iKeeper.IndexRefundableMsg(ctx, &msg2) + iKeeper.IndexRefundableMsg(ctx, &msg3) + return &TestTx{Msgs: []sdk.Msg{&msg, &msg2, &msg3}} + }, + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tx := tc.setup(ctx) + result := decorator.CheckTxAndClearIndex(ctx, tx) + + require.Equal(t, tc.expected, result) + + // Check that all messages have been cleared from the index + for _, msg := range tx.GetMsgs() { + require.False(t, iKeeper.HasRefundableMsg(ctx, types.HashMsg(msg))) + } + }) + } +}