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.

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

feat(hard_fork): part2 (Delyayed ack callback) #1355

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
1 change: 0 additions & 1 deletion proto/dymensionxyz/dymension/common/status.proto
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,4 @@ option go_package = "github.com/dymensionxyz/dymension/v3/x/common/types";
enum Status {
PENDING = 0;
FINALIZED = 1;
REVERTED = 3;
}
5 changes: 5 additions & 0 deletions testutil/keeper/delayedack.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@

type ChannelKeeperStub struct{}

// SetPacketReceipt implements types.ChannelKeeper.
func (c ChannelKeeperStub) SetPacketReceipt(ctx sdk.Context, portID string, channelID string, sequence uint64) {
return

Check failure on line 33 in testutil/keeper/delayedack.go

View workflow job for this annotation

GitHub Actions / golangci-lint

S1023: redundant `return` statement (gosimple)
}

func (ChannelKeeperStub) LookupModuleByChannel(ctx sdk.Context, portID, channelID string) (string, *capabilitytypes.Capability, error) {
return "", nil, nil
}
Expand Down
4 changes: 0 additions & 4 deletions x/common/types/key_rollapp_packet.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ var (
PendingRollappPacketKeyPrefix = []byte{0x00, 0x01}
// FinalizedRollappPacketKeyPrefix is the prefix for finalized rollapp packets
FinalizedRollappPacketKeyPrefix = []byte{0x00, 0x02}
// RevertedRollappPacketKeyPrefix is the prefix for reverted rollapp packets
RevertedRollappPacketKeyPrefix = []byte{0x00, 0x03}
// keySeparatorBytes is used to separate the rollapp packet key parts
keySeparatorBytes = []byte("/")
)
Expand Down Expand Up @@ -100,8 +98,6 @@ func MustGetStatusBytes(status Status) []byte {
return PendingRollappPacketKeyPrefix
case Status_FINALIZED:
return FinalizedRollappPacketKeyPrefix
case Status_REVERTED:
return RevertedRollappPacketKeyPrefix
default:
panic(fmt.Sprintf("invalid packet status: %s", status))
}
Expand Down
18 changes: 7 additions & 11 deletions x/common/types/status.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions x/delayedack/client/cli/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func CmdGetPacketsByRollapp() *cobra.Command {
cmd := &cobra.Command{
Use: "packets-by-rollapp rollapp-id [status] [type]",
Short: "Get packets by rollapp-id",
Long: `Get packets by rollapp-id. Can filter by status (pending/finalized/reverted) and by type (recv/ack/timeout)
Long: `Get packets by rollapp-id. Can filter by status (pending/finalized) and by type (recv/ack/timeout)
Example:
packets rollapp1
packets rollapp1 PENDING
Expand Down Expand Up @@ -124,7 +124,7 @@ func CmdGetPacketsByStatus() *cobra.Command {
cmd := &cobra.Command{
Use: "packets-by-status status [type]",
Short: "Get packets by status",
Long: `Get packets by status (pending/finalized/reverted). Can filter by type (recv/ack/timeout)
Long: `Get packets by status (pending/finalized). Can filter by type (recv/ack/timeout)
Example:
packets-by-status pending
packets-by-status finalized recv`,
Expand Down
42 changes: 7 additions & 35 deletions x/delayedack/keeper/finalize.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"github.com/osmosis-labs/osmosis/v15/osmoutils"

commontypes "github.com/dymensionxyz/dymension/v3/x/common/types"
"github.com/dymensionxyz/dymension/v3/x/delayedack/types"
)

// FinalizeRollappPacket finalizes a singe packet by its rollapp packet key.
Expand All @@ -25,7 +24,7 @@ func (k Keeper) FinalizeRollappPacket(ctx sdk.Context, ibc porttypes.IBCModule,
// Verify the height is finalized
err = k.VerifyHeightFinalized(ctx, packet.RollappId, packet.ProofHeight)
if err != nil {
return packet, fmt.Errorf("verify height is finalized: rollapp '%s': %w", packet.RollappId, err)
return packet, fmt.Errorf("verify height: rollapp '%s': %w", packet.RollappId, err)
}

// Finalize the packet
Expand Down Expand Up @@ -137,41 +136,14 @@ func (k Keeper) onTimeoutPacket(rollappPacket commontypes.RollappPacket, ibc por
}

func (k Keeper) VerifyHeightFinalized(ctx sdk.Context, rollappID string, height uint64) error {
// Get the latest state info of the rollapp
latestIndex, found := k.rollappKeeper.GetLatestFinalizedStateIndex(ctx, rollappID)
if !found {
return gerrc.ErrNotFound.Wrapf("latest finalized state index is not found")
}
stateInfo, found := k.rollappKeeper.GetStateInfo(ctx, rollappID, latestIndex.Index)
if !found {
return gerrc.ErrNotFound.Wrapf("state info is not found")
}
// Check the latest finalized height of the rollapp is higher than the height specified
if height > stateInfo.GetLatestHeight() {
return gerrc.ErrInvalidArgument.Wrapf("packet height is not finalized yet: height '%d', latest height '%d'", height, stateInfo.GetLatestHeight())
}
return nil
}

func (k Keeper) GetRollappLatestFinalizedHeight(ctx sdk.Context, rollappID string) (uint64, error) {
latestIndex, found := k.rollappKeeper.GetLatestFinalizedStateIndex(ctx, rollappID)
if !found {
return 0, gerrc.ErrNotFound.Wrapf("latest finalized state index is not found")
}
stateInfo, found := k.rollappKeeper.GetStateInfo(ctx, rollappID, latestIndex.Index)
if !found {
return 0, gerrc.ErrNotFound.Wrapf("state info is not found")
}
return stateInfo.GetLatestHeight(), nil
}

func (k Keeper) GetPendingPacketsUntilLatestHeight(ctx sdk.Context, rollappID string) ([]commontypes.RollappPacket, uint64, error) {
// Get rollapp's latest finalized height
latestFinalizedHeight, err := k.GetRollappLatestFinalizedHeight(ctx, rollappID)
if err != nil {
return nil, 0, fmt.Errorf("get latest finalized height: rollapp '%s': %w", rollappID, err)
return err
}

// Get all pending rollapp packets until the latest finalized height
return k.ListRollappPackets(ctx, types.PendingByRollappIDByMaxHeight(rollappID, latestFinalizedHeight)), latestFinalizedHeight, nil
// Check the latest finalized height of the rollapp is higher than the height specified
if height > latestFinalizedHeight {
return gerrc.ErrInvalidArgument.Wrapf("packet height is not finalized yet: height '%d', latest finalized height '%d'", height, latestFinalizedHeight)
}
return nil
}
2 changes: 1 addition & 1 deletion x/delayedack/keeper/finalize_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (s *DelayedAckTestSuite) TestFinalizePacket() {
},
rollappHeight: 10,
expectErr: true,
errContains: "packet height is not finalized yet: height '15', latest height '10'",
errContains: "verify height",
},
}

Expand Down
28 changes: 14 additions & 14 deletions x/delayedack/keeper/fraud.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,11 @@
commontypes "github.com/dymensionxyz/dymension/v3/x/common/types"
)

func (k Keeper) HandleFraud(ctx sdk.Context, rollappID string, ibc porttypes.IBCModule) error {
// Get all the pending packets
rollappPendingPackets := k.ListRollappPackets(ctx, types.ByRollappIDByStatus(rollappID, commontypes.Status_PENDING))
if len(rollappPendingPackets) == 0 {
return nil
}
func (k Keeper) HandleHardFork(ctx sdk.Context, rollappID string, height uint64, ibc porttypes.IBCModule) error {
logger := ctx.Logger().With("module", "DelayedAckMiddleware")
logger.Info("reverting IBC rollapp packets", "rollappID", rollappID)

// Get all the pending packets
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Get all the pending packets
// Get all the pending packets from fork height inclusive

rollappPendingPackets := k.ListRollappPackets(ctx, types.PendingByRollappIDFromHeight(rollappID, height))
Copy link
Contributor

Choose a reason for hiding this comment

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

probably wont fix for now but I think possible DOS if a lot of packets and fraud is e.g few days ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pls open research topic


// Iterate over all the pending packets and revert them
for _, rollappPacket := range rollappPendingPackets {
Expand All @@ -28,22 +25,25 @@
"sequence", rollappPacket.Packet.Sequence,
}

// refund all pending outgoing packets
if rollappPacket.Type == commontypes.RollappPacket_ON_ACK || rollappPacket.Type == commontypes.RollappPacket_ON_TIMEOUT {
// refund all pending outgoing packets
// we don't have access directly to `refundPacketToken` function, so we'll use the `OnTimeoutPacket` function
err := ibc.OnTimeoutPacket(ctx, *rollappPacket.Packet, rollappPacket.Relayer)
if err != nil {
logger.Error("failed to refund reverted packet", append(logContext, "error", err.Error())...)
}
Comment on lines +29 to 35
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to follow the ADR:

  1. timeout - only deleting the rollapp packet
  2. on_ack - to delete packet and re-create the commitment

}
// Update status to reverted
_, err := k.UpdateRollappPacketWithStatus(ctx, rollappPacket, commontypes.Status_REVERTED)
if err != nil {
logger.Error("error reverting IBC rollapp packet", append(logContext, "error", err.Error())...)
return err
} else {
// for incoming packets, we need to reset the packet receipt
ibcPacket := rollappPacket.Packet
k.channelKeeper.SetPacketReceipt(ctx, ibcPacket.GetDestPort(), ibcPacket.GetDestChannel(), ibcPacket.GetSequence())
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how this is deleting the packet reciept?

from the ADR

	// remove receipt
    ibcPacket := rollappPacket.Packet
    receiptKey := host.PacketReceiptKey(ibcPacket.GetDestPort(), ibcPacket.GetDestChannel(), ibcPacket.GetSequence())
    channelKeeperStore.Delete(receiptKey)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

u right!
will fix

}

// delete the packet
k.DeleteRollappPacket(ctx, &rollappPacket)

Check failure on line 42 in x/delayedack/keeper/fraud.go

View workflow job for this annotation

GitHub Actions / golangci-lint

Error return value of `k.DeleteRollappPacket` is not checked (errcheck)
logger.Debug("reverted IBC rollapp packet", logContext...)
}

logger.Info("reverting IBC rollapp packets", "rollappID", rollappID, "numPackets", len(rollappPendingPackets))

return nil
}
63 changes: 21 additions & 42 deletions x/delayedack/keeper/fraud_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,66 +18,45 @@ func (suite *DelayedAckTestSuite) TestHandleFraud() {
)

rollappId := "testRollappId"
pkts := generatePackets(rollappId, 5)
pkts := generatePackets(rollappId, 10)
rollappId2 := "testRollappId2"
pkts2 := generatePackets(rollappId2, 5)
pkts2 := generatePackets(rollappId2, 10)
prefixPending1 := types.ByRollappIDByStatus(rollappId, commontypes.Status_PENDING)
prefixPending2 := types.ByRollappIDByStatus(rollappId2, commontypes.Status_PENDING)
prefixReverted := types.ByRollappIDByStatus(rollappId, commontypes.Status_REVERTED)
prefixFinalized := types.ByRollappIDByStatus(rollappId, commontypes.Status_FINALIZED)
prefixFinalized1 := types.ByRollappIDByStatus(rollappId, commontypes.Status_FINALIZED)
prefixFinalized2 := types.ByRollappIDByStatus(rollappId, commontypes.Status_FINALIZED)

for _, pkt := range append(pkts, pkts2...) {
keeper.SetRollappPacket(ctx, pkt)
}

suite.Require().Equal(5, len(keeper.ListRollappPackets(ctx, prefixPending1)))
suite.Require().Equal(5, len(keeper.ListRollappPackets(ctx, prefixPending2)))
suite.Require().Equal(10, len(keeper.ListRollappPackets(ctx, prefixPending1)))
suite.Require().Equal(10, len(keeper.ListRollappPackets(ctx, prefixPending2)))

// finalize some packets
// finalize one packet
_, err := keeper.UpdateRollappPacketWithStatus(ctx, pkts[0], commontypes.Status_FINALIZED)
suite.Require().Nil(err)
_, err = keeper.UpdateRollappPacketWithStatus(ctx, pkts2[0], commontypes.Status_FINALIZED)
suite.Require().Nil(err)

err = keeper.HandleFraud(ctx, rollappId, transferStack)
// call fraud on the 4 packet
err = keeper.HandleHardFork(ctx, rollappId, 4, transferStack)
suite.Require().Nil(err)

suite.Require().Equal(0, len(keeper.ListRollappPackets(ctx, prefixPending1)))
suite.Require().Equal(4, len(keeper.ListRollappPackets(ctx, prefixPending2)))
suite.Require().Equal(4, len(keeper.ListRollappPackets(ctx, prefixReverted)))
suite.Require().Equal(1, len(keeper.ListRollappPackets(ctx, prefixFinalized)))
suite.Require().Equal(1, len(keeper.ListRollappPackets(ctx, prefixFinalized2)))
}

func (suite *DelayedAckTestSuite) TestDeletionOfRevertedPackets() {
keeper, ctx := suite.App.DelayedAckKeeper, suite.Ctx
transferStack := damodule.NewIBCMiddleware(
damodule.WithIBCModule(ibctransfer.NewIBCModule(suite.App.TransferKeeper)),
damodule.WithKeeper(keeper),
damodule.WithRollappKeeper(suite.App.RollappKeeper),
)

rollappId := "testRollappId"
pkts := generatePackets(rollappId, 5)
rollappId2 := "testRollappId2"
pkts2 := generatePackets(rollappId2, 5)

for _, pkt := range append(pkts, pkts2...) {
keeper.SetRollappPacket(ctx, pkt)
}

err := keeper.HandleFraud(ctx, rollappId, transferStack)
suite.Require().Nil(err)
// expected result:
// rollappId:
// - packet 1 are finalized
// - packet 2-3 are still pending
// - packets 4-10 are deleted
// rollappId2:
// - packet 1 are finalized
// - packets 2-10 are still pending

suite.Require().Equal(10, len(keeper.GetAllRollappPackets(ctx)))
suite.Require().Equal(1, len(keeper.ListRollappPackets(ctx, prefixFinalized1)))
suite.Require().Equal(2, len(keeper.ListRollappPackets(ctx, prefixPending1)))

keeper.SetParams(ctx, types.Params{EpochIdentifier: "minute", BridgingFee: keeper.BridgingFee(ctx)})
epochHooks := keeper.GetEpochHooks()
err = epochHooks.AfterEpochEnd(ctx, "minute", 1)
suite.Require().NoError(err)

suite.Require().Equal(5, len(keeper.GetAllRollappPackets(ctx)))
suite.Require().Equal(1, len(keeper.ListRollappPackets(ctx, prefixFinalized2)))
suite.Require().Equal(9, len(keeper.ListRollappPackets(ctx, prefixPending2)))
}

// TODO: test refunds of pending packets
Expand All @@ -86,7 +65,7 @@ func (suite *DelayedAckTestSuite) TestDeletionOfRevertedPackets() {

func generatePackets(rollappId string, num uint64) []commontypes.RollappPacket {
var packets []commontypes.RollappPacket
for i := uint64(0); i < num; i++ {
for i := uint64(1); i <= num; i++ {
packets = append(packets, commontypes.RollappPacket{
RollappId: rollappId,
Packet: &channeltypes.Packet{
Expand Down
4 changes: 2 additions & 2 deletions x/delayedack/keeper/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func (e epochHooks) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epoch
return nil
}

listFilter := types.ByStatus(commontypes.Status_FINALIZED, commontypes.Status_REVERTED).Take(int(deletePacketsBatchSize))
listFilter := types.ByStatus(commontypes.Status_FINALIZED).Take(int(deletePacketsBatchSize))
count := 0

// Get batch of rollapp packets with status != PENDING and delete them
Expand All @@ -83,7 +83,7 @@ func (e epochHooks) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epoch

for _, packet := range toDeletePackets {
err := osmoutils.ApplyFuncIfNoError(ctx, func(ctx sdk.Context) error {
return e.deleteRollappPacket(ctx, &packet)
return e.DeleteRollappPacket(ctx, &packet)
})
if err != nil {
e.Logger(ctx).Error("Failed to delete rollapp packet",
Expand Down
Loading
Loading