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

Jump to bottom

feat: allow ability to lock fee module in presence of severe bug #1239

Merged
merged 6 commits into from
Apr 11, 2022
Merged

feat: allow ability to lock fee module in presence of severe bug #1239

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b5c3cbe
apply code changes from pr #1031
colin-axner Apr 11, 2022
49588fa
update comments
colin-axner Apr 11, 2022
9448588
merge main
colin-axner Apr 11, 2022
d0bb241
chore: update fee module is locked error string
colin-axner Apr 11, 2022
f6e7a8e
update godoc to reference ADR 004
colin-axner Apr 11, 2022
b85f814
add more references to ADR 004
colin-axner Apr 11, 2022
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
10 changes: 9 additions & 1 deletion modules/apps/29-fee/fee_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ type FeeTestSuite struct {
chainB *ibctesting.TestChain
chainC *ibctesting.TestChain

path *ibctesting.Path
path *ibctesting.Path
pathAToC *ibctesting.Path
}

Expand Down Expand Up @@ -63,3 +63,11 @@ func (suite *FeeTestSuite) CreateMockPacket() channeltypes.Packet {
0,
)
}

// helper function
func lockFeeModule(chain *ibctesting.TestChain) {
ctx := chain.GetContext()
storeKey := chain.GetSimApp().GetKey(types.ModuleName)
store := ctx.KVStore(storeKey)
store.Set(types.KeyLocked(), []byte{1})
}
15 changes: 14 additions & 1 deletion modules/apps/29-fee/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,16 @@ func (im IBCModule) OnAcknowledgementPacket(
return sdkerrors.Wrapf(err, "cannot unmarshal ICS-29 incentivized packet acknowledgement: %v", ack)
}

if im.keeper.IsLocked(ctx) {
// if the fee keeper is locked then fee logic should be skipped
// this may occur in the presence of a severe bug which leads to invalid state
// the fee keeper will be unlocked after manual intervention
// the acknowledgement has been unmarshalled into an ics29 acknowledgement
// since the counterparty is still sending incentivized acknowledgements
// for fee enabled channels
return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer)
}

packetID := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence)
feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID)
if found {
Expand All @@ -251,7 +261,10 @@ func (im IBCModule) OnTimeoutPacket(
packet channeltypes.Packet,
relayer sdk.AccAddress,
) error {
if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) {
// if the fee keeper is locked then fee logic should be skipped
// this may occur in the presence of a severe bug which leads to invalid state
// the fee keeper will be unlocked after manual intervention
if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) || im.keeper.IsLocked(ctx) {
return im.app.OnTimeoutPacket(ctx, packet, relayer)
}

Expand Down
18 changes: 18 additions & 0 deletions modules/apps/29-fee/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,15 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
},
true,
},
{
"success: fee module is disabled, skip fee logic",
func() {
lockFeeModule(suite.chainA)

expectedBalance = originalBalance
},
true,
},
{
"fail on distribute receive fee (blocked address)",
func() {
Expand Down Expand Up @@ -724,6 +733,15 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
},
false,
},
{
"fee module is disabled, skip fee logic",
func() {
lockFeeModule(suite.chainA)

expectedBalance = originalBalance
},
false,
},
{
"no op if identified packet fee doesn't exist",
func() {
Expand Down
79 changes: 45 additions & 34 deletions modules/apps/29-fee/keeper/escrow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,27 +125,62 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
reverseRelayer sdk.AccAddress
forwardRelayer string
refundAcc sdk.AccAddress
refundAccBal sdk.Coin
fee types.Fee
packetID channeltypes.PacketId
)

validSeq := uint64(1)

testCases := []struct {
name string
malleate func()
expPass bool
name string
malleate func()
expResult func()
}{
{
"success", func() {}, true,
"success", func() {}, func() {
// check if the reverse relayer is paid
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, fee.AckFee[0].Add(fee.AckFee[0]))
suite.Require().True(hasBalance)

// check if the forward relayer is paid
forward, err := sdk.AccAddressFromBech32(forwardRelayer)
suite.Require().NoError(err)
hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), forward, fee.RecvFee[0].Add(fee.RecvFee[0]))
suite.Require().True(hasBalance)

// check if the refund acc has been refunded the timeoutFee
expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0].Add(fee.TimeoutFee[0]))
hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal)
suite.Require().True(hasBalance)

// check the module acc wallet is now empty
hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(0)})
suite.Require().True(hasBalance)
},
},
{
"invalid forward address", func() {
forwardRelayer = "invalid address"
}, false,
},
func() {
// check if the refund acc has been refunded the timeoutFee & onRecvFee
expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]).Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0])
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal)
suite.Require().True(hasBalance)

},
},
{
"invalid forward address: blocked address", func() {
forwardRelayer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()
}, false,
},
func() {
// check if the refund acc has been refunded the timeoutFee & onRecvFee
expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]).Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0])
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal)
suite.Require().True(hasBalance)
},
},
}

Expand All @@ -161,8 +196,8 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
reverseRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address())
forwardRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String()

packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, validSeq)
fee := types.Fee{
packetID = channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, validSeq)
fee = types.Fee{
RecvFee: defaultReceiveFee,
AckFee: defaultAckFee,
TimeoutFee: defaultTimeoutFee,
Expand All @@ -180,35 +215,11 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
tc.malleate()

// refundAcc balance after escrow
refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
refundAccBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)

suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, []types.PacketFee{packetFee, packetFee})

if tc.expPass {
// check if the reverse relayer is paid
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, fee.AckFee[0].Add(fee.AckFee[0]))
suite.Require().True(hasBalance)

// check if the forward relayer is paid
forward, err := sdk.AccAddressFromBech32(forwardRelayer)
suite.Require().NoError(err)
hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), forward, fee.RecvFee[0].Add(fee.RecvFee[0]))
suite.Require().True(hasBalance)

// check if the refund acc has been refunded the timeoutFee
expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0].Add(fee.TimeoutFee[0]))
hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal)
suite.Require().True(hasBalance)

// check the module acc wallet is now empty
hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(0)})
suite.Require().True(hasBalance)
} else {
// check if the refund acc has been refunded the timeoutFee & onRecvFee
expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]).Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0])
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal)
suite.Require().True(hasBalance)
}
tc.expResult()
})
}
}
Expand Down
13 changes: 13 additions & 0 deletions modules/apps/29-fee/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,19 @@ func (k Keeper) EscrowAccountHasBalance(ctx sdk.Context, coins sdk.Coins) bool {
return true
}

// lockFeeModule sets a flag to determine if fee handling logic should run for the given channel
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add an unlockFeeModule helper fn also or is the idea to do that in a follow-up PR?

I'm also unclear as to what the actual process looks like for unlocking. What is your idea for that (sorry if this is written down somewhere I couldn't find mention in the ADR/issues) :)

Once we have the unlock flow I guess it would make sense to also have a test case for:

  1. Lock fee module
  2. Try to incentivize packet (doesn't work)
  3. Unlock fee module
  4. Try to incentivize packet (works)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unlockFeeModule is unnecessary. Unlocking the fee module requires a coordinated upgrade to manually fix invalid state. Given that we can't know what the bug is now, a migration command to fix the invalid state and unlock the fee module cannot be written now. Likely if this ever occurred, the unlocking would be done by explicitly removing the lock flag in code, not by calling a function

// identified by channel and port identifiers.
func (k Keeper) lockFeeModule(ctx sdk.Context) {
store := ctx.KVStore(k.storeKey)
store.Set(types.KeyLocked(), []byte{1})
}

// IsLocked indicates if the fee module is locked
func (k Keeper) IsLocked(ctx sdk.Context) bool {
store := ctx.KVStore(k.storeKey)
return store.Has(types.KeyLocked())
}

// SetFeeEnabled sets a flag to determine if fee handling logic should run for the given channel
// identified by channel and port identifiers.
func (k Keeper) SetFeeEnabled(ctx sdk.Context, portID, channelID string) {
Expand Down
18 changes: 17 additions & 1 deletion modules/apps/29-fee/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ func TestKeeperTestSuite(t *testing.T) {
suite.Run(t, new(KeeperTestSuite))
}

// helper function
func lockFeeModule(chain *ibctesting.TestChain) {
ctx := chain.GetContext()
storeKey := chain.GetSimApp().GetKey(types.ModuleName)
store := ctx.KVStore(storeKey)
store.Set(types.KeyLocked(), []byte{1})
}

func (suite *KeeperTestSuite) TestEscrowAccountHasBalance() {
fee := types.Fee{
AckFee: defaultAckFee,
Expand All @@ -85,7 +93,6 @@ func (suite *KeeperTestSuite) TestEscrowAccountHasBalance() {
// increase ack fee
fee.AckFee = fee.AckFee.Add(defaultAckFee...)
suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.EscrowAccountHasBalance(suite.chainA.GetContext(), fee.Total()))

}

func (suite *KeeperTestSuite) TestFeesInEscrow() {
Expand All @@ -111,6 +118,15 @@ func (suite *KeeperTestSuite) TestFeesInEscrow() {
suite.Require().False(hasFeesInEscrow)
}

func (suite *KeeperTestSuite) TestIsLocked() {
ctx := suite.chainA.GetContext()
suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(ctx))

lockFeeModule(suite.chainA)

suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(ctx))
}

func (suite *KeeperTestSuite) TestDisableAllChannels() {
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "port1", "channel1")
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "port2", "channel2")
Expand Down
8 changes: 8 additions & 0 deletions modules/apps/29-fee/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ func (k Keeper) RegisterCounterpartyAddress(goCtx context.Context, msg *types.Ms
func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) (*types.MsgPayPacketFeeResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

if k.IsLocked(ctx) {
return nil, types.ErrFeeModuleLocked
}

// get the next sequence
sequence, found := k.GetNextSequenceSend(ctx, msg.SourcePortId, msg.SourceChannelId)
if !found {
Expand All @@ -57,6 +61,10 @@ func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee)
func (k Keeper) PayPacketFeeAsync(goCtx context.Context, msg *types.MsgPayPacketFeeAsync) (*types.MsgPayPacketFeeAsyncResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

if k.IsLocked(ctx) {
return nil, types.ErrFeeModuleLocked
}

if err := k.EscrowPacketFee(ctx, msg.PacketId, msg.PacketFee); err != nil {
return nil, err
}
Expand Down
21 changes: 19 additions & 2 deletions modules/apps/29-fee/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package keeper_test

import (
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types"
channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
ibctesting "github.com/cosmos/ibc-go/v3/testing"
Expand Down Expand Up @@ -62,6 +64,13 @@ func (suite *KeeperTestSuite) TestPayPacketFee() {
true,
func() {},
},
{
"fee module is locked",
false,
func() {
lockFeeModule(suite.chainA)
},
},
}

for _, tc := range testCases {
Expand All @@ -78,7 +87,8 @@ func (suite *KeeperTestSuite) TestPayPacketFee() {
msg := types.NewMsgPayPacketFee(fee, suite.path.EndpointA.ChannelConfig.PortID, channelID, refundAcc.String(), []string{})

tc.malleate()
_, err := suite.chainA.SendMsgs(msg)

_, err := suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFee(sdk.WrapSDKContext(suite.chainA.GetContext()), msg)

if tc.expPass {
suite.Require().NoError(err) // message committed
Expand All @@ -99,6 +109,13 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() {
true,
func() {},
},
{
"fee module is locked",
false,
func() {
lockFeeModule(suite.chainA)
},
},
}

for _, tc := range testCases {
Expand All @@ -125,7 +142,7 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() {
tc.malleate()

msg := types.NewMsgPayPacketFeeAsync(packetID, packetFee)
_, err := suite.chainA.SendMsgs(msg)
_, err := suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFeeAsync(sdk.WrapSDKContext(suite.chainA.GetContext()), msg)

if tc.expPass {
suite.Require().NoError(err) // message committed
Expand Down
1 change: 1 addition & 0 deletions modules/apps/29-fee/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ var (
ErrForwardRelayerAddressNotFound = sdkerrors.Register(ModuleName, 8, "forward relayer address not found")
ErrFeeNotEnabled = sdkerrors.Register(ModuleName, 9, "fee module is not enabled for this channel. If this error occurs after channel setup, fee module may not be enabled")
ErrRelayerNotFoundForAsyncAck = sdkerrors.Register(ModuleName, 10, "relayer address must be stored for async WriteAcknowledgement")
ErrFeeModuleLocked = sdkerrors.Register(ModuleName, 11, "the fee module is currently locked")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

It might be worth making this error message more descriptive so that people know what/why the module is "locked". wdyt?

Maybe something added to the end like

The fee module is locked if a severe bug is present

)
6 changes: 6 additions & 0 deletions modules/apps/29-fee/types/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ const (
ForwardRelayerPrefix = "forwardRelayer"
)

// KeyLocked returns the key used to lock and unlock the fee module. This key is used
// in the presence of a severe bug.
func KeyLocked() []byte {
return []byte("locked")
}

// KeyFeeEnabled returns the key that stores a flag to determine if fee logic should
// be enabled for the given port and channel identifiers.
func KeyFeeEnabled(portID, channelID string) []byte {
Expand Down