From 6a74ebcdfa2d2d457b2e42394703ee86716ca752 Mon Sep 17 00:00:00 2001 From: Runchao Han Date: Mon, 7 Oct 2024 15:51:57 +1100 Subject: [PATCH] chore: tx fee refunding follow-up (#130) This follow-up PR fixes the comments in https://github.com/babylonlabs-io/babylon/pull/125, including - changelog and typo comments in BTCCheckpoint module - allow covenant signatures after covenant quorum are reached, and return error if the covenant signature is duplicated. This is to ensure covenants won't have operational cost when submitting covenant sig late, and avoid refunding to duplicated covenant signatures - reject duplicated finality signature. This is to avoid refunding to duplicated covenant signatures - add a dedup check in the PostHandler, to ensure one won't exploit the refunding by having many duplicated messages in a single tx. Also added a fuzz test for this. --- CHANGELOG.md | 9 +- x/btccheckpoint/keeper/msg_server.go | 4 +- x/btcstaking/keeper/keeper_test.go | 8 +- x/btcstaking/keeper/msg_server.go | 17 ++- x/btcstaking/keeper/msg_server_test.go | 6 +- x/btcstaking/types/errors.go | 1 + x/finality/keeper/msg_server.go | 7 +- x/finality/keeper/msg_server_test.go | 7 +- x/finality/types/errors.go | 7 +- x/incentive/keeper/refund_tx_decorator.go | 44 +++++--- .../keeper/refund_tx_decorator_test.go | 104 ++++++++++++++++++ 11 files changed, 168 insertions(+), 46 deletions(-) create mode 100644 x/incentive/keeper/refund_tx_decorator_test.go 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))) + } + }) + } +}