From 61ca6b537f685570dd06a102c2d62f42dfe394e4 Mon Sep 17 00:00:00 2001 From: Fabrice Bascoulergue Date: Thu, 22 Feb 2024 15:44:48 +0100 Subject: [PATCH 1/2] Fix ibc transfer to native --- .../keeper/callbacks_transfer_to_native.go | 4 ++-- x/millions/keeper/msg_server_deposit.go | 17 ++++------------- x/millions/types/keys.go | 3 ++- 3 files changed, 8 insertions(+), 16 deletions(-) diff --git a/x/millions/keeper/callbacks_transfer_to_native.go b/x/millions/keeper/callbacks_transfer_to_native.go index 2e4421c2..8001b436 100644 --- a/x/millions/keeper/callbacks_transfer_to_native.go +++ b/x/millions/keeper/callbacks_transfer_to_native.go @@ -39,10 +39,10 @@ func TransferToNativeCallback(k Keeper, ctx sdk.Context, packet channeltypes.Pac return errorsmod.Wrapf(types.ErrUnmarshalFailure, fmt.Sprintf("Unable to unmarshal transfer to native callback args: %s", err.Error())) } - // If the response status is a timeout, that's not an "error" since the relayer will retry then fail or succeed. - // We just log it out and return no error if ackResponse.Status == icacallbackstypes.AckResponseStatus_TIMEOUT { + // Timeout is considered a failure on IBC transfers k.Logger(ctx).Debug("Received timeout for a transfer to native packet") + return k.OnTransferDepositToRemoteZoneCompleted(ctx, transferCallback.GetPoolId(), transferCallback.GetDepositId(), true) } else if ackResponse.Status == icacallbackstypes.AckResponseStatus_FAILURE { k.Logger(ctx).Debug("Received failure for a transfer to native packet") return k.OnTransferDepositToRemoteZoneCompleted(ctx, transferCallback.GetPoolId(), transferCallback.GetDepositId(), true) diff --git a/x/millions/keeper/msg_server_deposit.go b/x/millions/keeper/msg_server_deposit.go index 52fbd284..3ac29b96 100644 --- a/x/millions/keeper/msg_server_deposit.go +++ b/x/millions/keeper/msg_server_deposit.go @@ -126,18 +126,9 @@ func (k msgServer) DepositRetry(goCtx context.Context, msg *types.MsgDepositRetr return nil, types.ErrInvalidDepositorAddress } - // State should be set to failure in order to retry something - if deposit.State != types.DepositState_Failure { - return nil, errorsmod.Wrapf( - types.ErrInvalidDepositState, - "state is %s instead of %s", - deposit.State.String(), types.DepositState_Failure.String(), - ) - } - newState := types.DepositState_Unspecified - if deposit.State == types.DepositState_IbcTransfer && ctx.BlockTime().After(deposit.UpdatedAt.Add(2*types.IBCTimeoutNanos*time.Nanosecond)) { - // Handle IBC stucked operation for more than twice the specified IBC timeout + if deposit.State == types.DepositState_IbcTransfer && ctx.BlockTime().After(deposit.UpdatedAt.Add(types.IBCForceRetryNanos*time.Nanosecond)) { + // Handle IBC stucked operation for more than 3 days (no timeout received) newState = types.DepositState_IbcTransfer if err := k.TransferDepositToRemoteZone(ctx, deposit.PoolId, deposit.DepositId); err != nil { return nil, err @@ -157,8 +148,8 @@ func (k msgServer) DepositRetry(goCtx context.Context, msg *types.MsgDepositRetr } else { return nil, errorsmod.Wrapf( types.ErrInvalidDepositState, - "error_state is %s instead of %s or %s", - deposit.ErrorState.String(), types.DepositState_IbcTransfer.String(), types.DepositState_IcaDelegate.String(), + "state is %s instead of %s", + deposit.State.String(), types.DepositState_Failure.String(), ) } diff --git a/x/millions/types/keys.go b/x/millions/types/keys.go index 8e7bab32..f3578bdc 100644 --- a/x/millions/types/keys.go +++ b/x/millions/types/keys.go @@ -24,7 +24,8 @@ const ( ) const ( - IBCTimeoutNanos = 1_800_000_000_000 // 30 minutes + IBCTimeoutNanos = 1_800_000_000_000 // 30 minutes + IBCForceRetryNanos = 259_200_000_000_000 // 3 days ) const ( From 1cde5989cc8f28f67a0bea9d85f2de51b58f9769 Mon Sep 17 00:00:00 2001 From: Fabrice Bascoulergue Date: Thu, 22 Feb 2024 16:56:38 +0100 Subject: [PATCH 2/2] Improve ICA error management and code comments --- x/millions/keeper/callbacks_bank_send.go | 3 +++ x/millions/keeper/callbacks_claim.go | 5 +++-- x/millions/keeper/callbacks_delegate.go | 5 +++-- x/millions/keeper/callbacks_redelegate.go | 6 ++++-- x/millions/keeper/callbacks_set_withdraw_address.go | 6 ++++-- x/millions/keeper/callbacks_transfer_from_native.go | 5 +++-- x/millions/keeper/callbacks_transfer_to_native.go | 4 +++- x/millions/keeper/callbacks_undelegate.go | 6 +++--- x/millions/keeper/msg_server_pool.go | 4 ++-- x/millions/keeper/queries_balance.go | 4 +++- 10 files changed, 31 insertions(+), 17 deletions(-) diff --git a/x/millions/keeper/callbacks_bank_send.go b/x/millions/keeper/callbacks_bank_send.go index 9a2903e2..6ddc6088 100644 --- a/x/millions/keeper/callbacks_bank_send.go +++ b/x/millions/keeper/callbacks_bank_send.go @@ -44,6 +44,9 @@ func BankSendCallback(k Keeper, ctx sdk.Context, packet channeltypes.Packet, ack return err } + // Scenarios: + // - Timeout: Does nothing, handled when restoring the ICA channel + // - Error: Put entity in error state to allow users to retry if ackResponse.Status == icacallbackstypes.AckResponseStatus_TIMEOUT { k.Logger(ctx).Debug("Received timeout for a bank send to native packet") } else if ackResponse.Status == icacallbackstypes.AckResponseStatus_FAILURE { diff --git a/x/millions/keeper/callbacks_claim.go b/x/millions/keeper/callbacks_claim.go index 8b47dd0a..64f96371 100644 --- a/x/millions/keeper/callbacks_claim.go +++ b/x/millions/keeper/callbacks_claim.go @@ -39,8 +39,9 @@ func ClaimCallback(k Keeper, ctx sdk.Context, packet channeltypes.Packet, ackRes return errorsmod.Wrapf(types.ErrUnmarshalFailure, fmt.Sprintf("Unable to unmarshal claim callback args: %s", err.Error())) } - // If the response status is a timeout, that's not an "error" since the relayer will retry then fail or succeed. - // We just log it out and return no error + // Scenarios: + // - Timeout: Does nothing, handled when restoring the ICA channel + // - Error: Put entity in error state to allow users to retry if ackResponse.Status == icacallbackstypes.AckResponseStatus_TIMEOUT { k.Logger(ctx).Debug("Received timeout for a claim packet") } else if ackResponse.Status == icacallbackstypes.AckResponseStatus_FAILURE { diff --git a/x/millions/keeper/callbacks_delegate.go b/x/millions/keeper/callbacks_delegate.go index 3841c21b..41edfceb 100644 --- a/x/millions/keeper/callbacks_delegate.go +++ b/x/millions/keeper/callbacks_delegate.go @@ -39,8 +39,9 @@ func DelegateCallback(k Keeper, ctx sdk.Context, packet channeltypes.Packet, ack return errorsmod.Wrapf(types.ErrUnmarshalFailure, fmt.Sprintf("Unable to unmarshal delegate callback args: %s", err.Error())) } - // If the response status is a timeout, that's not an "error" since the relayer will retry then fail or succeed. - // We just log it out and return no error + // Scenarios: + // - Timeout: Does nothing, handled when restoring the ICA channel + // - Error: Put entity in error state to allow users to retry if ackResponse.Status == icacallbackstypes.AckResponseStatus_TIMEOUT { k.Logger(ctx).Debug("Received timeout for a delegate packet") } else if ackResponse.Status == icacallbackstypes.AckResponseStatus_FAILURE { diff --git a/x/millions/keeper/callbacks_redelegate.go b/x/millions/keeper/callbacks_redelegate.go index e17f1142..3a7794fc 100644 --- a/x/millions/keeper/callbacks_redelegate.go +++ b/x/millions/keeper/callbacks_redelegate.go @@ -45,10 +45,12 @@ func RedelegateCallback(k Keeper, ctx sdk.Context, packet channeltypes.Packet, a return err } - // If the response status is a timeout, that's not an "error" since the relayer will retry then fail or succeed. - // We just log it out and return no error + // Scenarios: + // - Timeout: Treated as an error (see below) + // - Error: Revert Pool validator set known split if ackResponse.Status == icacallbackstypes.AckResponseStatus_TIMEOUT { k.Logger(ctx).Debug("Received timeout for a redelegate packet") + return k.OnRedelegateToActiveValidatorsOnRemoteZoneCompleted(ctx, redelegateCallback.GetPoolId(), redelegateCallback.GetOperatorAddress(), redelegateCallback.GetSplitDelegations(), true) } else if ackResponse.Status == icacallbackstypes.AckResponseStatus_FAILURE { k.Logger(ctx).Debug("Received failure for a redelegate packet") return k.OnRedelegateToActiveValidatorsOnRemoteZoneCompleted(ctx, redelegateCallback.GetPoolId(), redelegateCallback.GetOperatorAddress(), redelegateCallback.GetSplitDelegations(), true) diff --git a/x/millions/keeper/callbacks_set_withdraw_address.go b/x/millions/keeper/callbacks_set_withdraw_address.go index ab319718..ec08ab1d 100644 --- a/x/millions/keeper/callbacks_set_withdraw_address.go +++ b/x/millions/keeper/callbacks_set_withdraw_address.go @@ -45,8 +45,10 @@ func SetWithdrawAddressCallback(k Keeper, ctx sdk.Context, packet channeltypes.P return err } - // If the response status is a timeout, that's not an "error" since the relayer will retry then fail or succeed. - // We just log it out and return no error + // Scenarios: + // This operation is done right after the ICA channel open procedure + // - Timeout: Does nothing - fix may be needed manually + // - Error: Does nothing - fix may be needed manually if ackResponse.Status == icacallbackstypes.AckResponseStatus_TIMEOUT { k.Logger(ctx).Debug("Received timeout for a set withdraw address packet") } else if ackResponse.Status == icacallbackstypes.AckResponseStatus_FAILURE { diff --git a/x/millions/keeper/callbacks_transfer_from_native.go b/x/millions/keeper/callbacks_transfer_from_native.go index 928747ca..889ebafb 100644 --- a/x/millions/keeper/callbacks_transfer_from_native.go +++ b/x/millions/keeper/callbacks_transfer_from_native.go @@ -45,8 +45,9 @@ func TransferFromNativeCallback(k Keeper, ctx sdk.Context, packet channeltypes.P return err } - // If the response status is a timeout, that's not an "error" since the relayer will retry then fail or succeed. - // We just log it out and return no error + // Scenarios: + // - Timeout: Does nothing, handled when restoring the ICA channel + // - Error: Put entity in error state to allow users to retry if ackResponse.Status == icacallbackstypes.AckResponseStatus_TIMEOUT { k.Logger(ctx).Debug("Received timeout for a transfer from native packet") } else if ackResponse.Status == icacallbackstypes.AckResponseStatus_FAILURE { diff --git a/x/millions/keeper/callbacks_transfer_to_native.go b/x/millions/keeper/callbacks_transfer_to_native.go index 8001b436..ed602a92 100644 --- a/x/millions/keeper/callbacks_transfer_to_native.go +++ b/x/millions/keeper/callbacks_transfer_to_native.go @@ -39,8 +39,10 @@ func TransferToNativeCallback(k Keeper, ctx sdk.Context, packet channeltypes.Pac return errorsmod.Wrapf(types.ErrUnmarshalFailure, fmt.Sprintf("Unable to unmarshal transfer to native callback args: %s", err.Error())) } + // Scenarios: + // - Timeout: Treated as an error (see below) + // - Error: Put entity in error state to allow users to retry if ackResponse.Status == icacallbackstypes.AckResponseStatus_TIMEOUT { - // Timeout is considered a failure on IBC transfers k.Logger(ctx).Debug("Received timeout for a transfer to native packet") return k.OnTransferDepositToRemoteZoneCompleted(ctx, transferCallback.GetPoolId(), transferCallback.GetDepositId(), true) } else if ackResponse.Status == icacallbackstypes.AckResponseStatus_FAILURE { diff --git a/x/millions/keeper/callbacks_undelegate.go b/x/millions/keeper/callbacks_undelegate.go index fe1d4141..e90b1a75 100644 --- a/x/millions/keeper/callbacks_undelegate.go +++ b/x/millions/keeper/callbacks_undelegate.go @@ -69,13 +69,13 @@ func UndelegateCallback(k Keeper, ctx sdk.Context, packet channeltypes.Packet, a return err } - // If the response status is a timeout, that's not an "error" since the relayer will retry then fail or succeed. - // We just log it out and return no error + // Scenarios: + // - Timeout: Does nothing, handled when restoring the ICA channel + // - Error: Put back entities in the queue automatically if ackResponse.Status == icacallbackstypes.AckResponseStatus_TIMEOUT { k.Logger(ctx).Debug("Received timeout for an undelegate packet") } else if ackResponse.Status == icacallbackstypes.AckResponseStatus_FAILURE { k.Logger(ctx).Debug("Received failure for an undelegate packet") - // Failed OnUndelegateEpochUnbondingOnRemoteZoneCompleted return k.OnUndelegateWithdrawalsOnRemoteZoneCompleted( ctx, undelegateCallback.PoolId, diff --git a/x/millions/keeper/msg_server_pool.go b/x/millions/keeper/msg_server_pool.go index 45bbdc8a..03ddec4e 100644 --- a/x/millions/keeper/msg_server_pool.go +++ b/x/millions/keeper/msg_server_pool.go @@ -88,7 +88,7 @@ func (k msgServer) restoreICADepositEntities(ctx sdk.Context, poolID uint64) { // Restore withdrawals ICA locked operations on ICADeposit account withdrawals := k.Keeper.ListPoolWithdrawals(ctx, poolID) for _, w := range withdrawals { - if w.State == types.WithdrawalState_IcaUndelegate { + if w.State == types.WithdrawalState_IcaUndelegate || w.State == types.WithdrawalState_IbcTransfer { w.ErrorState = w.State w.State = types.WithdrawalState_Failure k.Keeper.setPoolWithdrawal(ctx, w) @@ -98,7 +98,7 @@ func (k msgServer) restoreICADepositEntities(ctx sdk.Context, poolID uint64) { // Restore draws ICA locked operations on ICADeposit account draws := k.Keeper.ListPoolDraws(ctx, poolID) for _, d := range draws { - if d.State == types.DrawState_IcaWithdrawRewards { + if d.State == types.DrawState_IcaWithdrawRewards || d.State == types.DrawState_IbcTransfer { d.ErrorState = d.State d.State = types.DrawState_Failure k.Keeper.SetPoolDraw(ctx, d) diff --git a/x/millions/keeper/queries_balance.go b/x/millions/keeper/queries_balance.go index 1676fc8c..3bdcad5d 100644 --- a/x/millions/keeper/queries_balance.go +++ b/x/millions/keeper/queries_balance.go @@ -39,7 +39,9 @@ func BalanceCallback(k Keeper, ctx sdk.Context, args []byte, query icqueriestype return errorsmod.Wrapf(types.ErrPoolNotFound, "no registered draw %d for pool %d", drawID, poolID) } - // Based on type, we want different processing + // Scenarios: + // - Timeout: Treated as an error (see below) + // - Error: Put entity in error state to allow users to retry if status == icqueriestypes.QueryResponseStatus_TIMEOUT { k.Logger(ctx).Error(fmt.Sprintf("QUERY TIMEOUT - QueryId: %s, TTL: %d, BlockTime: %d", query.Id, query.TimeoutTimestamp, ctx.BlockHeader().Time.UnixNano())) _, err := k.OnQueryFreshPrizePoolCoinsOnRemoteZoneCompleted(ctx, pool.GetPoolId(), draw.GetDrawId(), sdk.NewCoins(), true)