From a7a94963f191045e1c2b33941dbd41e7a7d0e2fc Mon Sep 17 00:00:00 2001 From: Takeshi Arabiki Date: Fri, 1 Aug 2025 14:56:50 +0900 Subject: [PATCH 1/6] Make channel-upgrade execute command fully idempotent The channel-upgrade execute command is basically idempotent. However, since proposed upgrades are deleted after the channel upgrade completes, re-running the command previously failed with the error: "Error: channel upgrade is not initialized." This behavior caused instability in tests such as: https://github.com/datachainlab/cosmos-ethereum-ibc-lcp/blob/v0.2.24/tests/e2e/cases/tm2eth/scripts/test-channel-upgrade#L122 To address this, the command now exits with exit code 0 even when no proposed upgrades exist. Although some users might forget to run the channel-upgrade init command, they will be notified through the newly added log message. Signed-off-by: Takeshi Arabiki --- core/channel-upgrade.go | 11 +++++++---- tests/cases/tm2tm/scripts/test-channel-upgrade | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/core/channel-upgrade.go b/core/channel-upgrade.go index 18127c79..83e9722c 100644 --- a/core/channel-upgrade.go +++ b/core/channel-upgrade.go @@ -382,9 +382,12 @@ func upgradeChannelStep(ctx context.Context, src, dst *ProvableChain, targetSrcS )} // check if both chains have reached the target states or UNINIT states - if !firstCall && srcState == UPGRADE_STATE_UNINIT && dstState == UPGRADE_STATE_UNINIT || - srcState != UPGRADE_STATE_UNINIT && dstState != UPGRADE_STATE_UNINIT && srcState == targetSrcState && dstState == targetDstState { - logger.InfoContext(ctx, "both chains have reached the target states") + if srcState == targetSrcState && dstState == targetDstState { + if firstCall && srcState == UPGRADE_STATE_UNINIT && dstState == UPGRADE_STATE_UNINIT { + logger.InfoContext(ctx, "both chains have already reached the target states, or the channel upgrade has not been initialized") + } else { + logger.InfoContext(ctx, "both chains have reached the target states") + } out.Last = true return out, nil } @@ -394,7 +397,7 @@ func upgradeChannelStep(ctx context.Context, src, dst *ProvableChain, targetSrcS dstAction := UPGRADE_ACTION_NONE switch { case srcState == UPGRADE_STATE_UNINIT && dstState == UPGRADE_STATE_UNINIT: - return nil, errors.New("channel upgrade is not initialized") + return nil, errors.New("channel upgrade has not been initialized; it will never reach the target states") case srcState == UPGRADE_STATE_INIT && dstState == UPGRADE_STATE_UNINIT: if dstChan.Channel.UpgradeSequence >= srcChan.Channel.UpgradeSequence { srcAction = UPGRADE_ACTION_CANCEL diff --git a/tests/cases/tm2tm/scripts/test-channel-upgrade b/tests/cases/tm2tm/scripts/test-channel-upgrade index a1e0cd1a..b8645d48 100755 --- a/tests/cases/tm2tm/scripts/test-channel-upgrade +++ b/tests/cases/tm2tm/scripts/test-channel-upgrade @@ -140,3 +140,17 @@ $RLY tx channel-upgrade execute ibc01 --target-src-state FLUSHING --target-dst-s sleep 20 # Both chains exceed upgrade.timeout.timestamp $RLY tx channel-upgrade execute ibc01 # ibc0,ibc1 <= chanUpgradeTimeout checkResult orig + +echo '##### case 10 #####' +# Both chains have already reached the target states, or the channel upgrade has not been initialized +$RLY tx channel-upgrade execute ibc01 +checkResult orig + +echo '##### case 11 #####' +# The target states will never be reached +if $RLY tx channel-upgrade execute ibc01 --target-src-state INIT --target-dst-state FLUSHING +then + echo 'channel-upgrade completed with exit code 0, but non-zero code was expected' >&2 + exit 1 +fi +checkResult orig From f9b6353f8ca3031c7c13f85dce7a72eabd85ee3a Mon Sep 17 00:00:00 2001 From: Takeshi Arabiki Date: Fri, 1 Aug 2025 19:28:26 +0900 Subject: [PATCH 2/6] Replace tab with spaces in shell script Signed-off-by: Takeshi Arabiki --- .../cases/tm2tm/scripts/test-channel-upgrade | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/cases/tm2tm/scripts/test-channel-upgrade b/tests/cases/tm2tm/scripts/test-channel-upgrade index b8645d48..85a96cff 100755 --- a/tests/cases/tm2tm/scripts/test-channel-upgrade +++ b/tests/cases/tm2tm/scripts/test-channel-upgrade @@ -53,21 +53,21 @@ checkResult() { if [ "$expectedSide" = orig ] then - if [ "$srcConnectionId" != "$srcOrigConnectionId" -o "$dstConnectionId" != "$dstOrigConnectionId" -o "$srcVersion" != "$srcOrigVersion" -o "$dstVersion" != "$dstOrigVersion" -o "$srcOrder" != "$srcOrigOrder" -o "$dstOrder" != "$dstOrigOrder" ] - then - echo "path config is not equal to the original one: $srcConnectionId, $dstConnectionId, $srcVersion, $dstVersion, $srcOrder, $dstOrder" - exit 1 - fi + if [ "$srcConnectionId" != "$srcOrigConnectionId" -o "$dstConnectionId" != "$dstOrigConnectionId" -o "$srcVersion" != "$srcOrigVersion" -o "$dstVersion" != "$dstOrigVersion" -o "$srcOrder" != "$srcOrigOrder" -o "$dstOrder" != "$dstOrigOrder" ] + then + echo "path config is not equal to the original one: $srcConnectionId, $dstConnectionId, $srcVersion, $dstVersion, $srcOrder, $dstOrder" + exit 1 + fi elif [ "$expectedSide" = alt ] then - if [ "$srcConnectionId" != "$srcAltConnectionId" -o "$dstConnectionId" != "$dstAltConnectionId" -o "$srcVersion" != "$srcAltVersion" -o "$dstVersion" != "$dstAltVersion" -o "$srcOrder" != "$srcAltOrder" -o "$dstOrder" != "$dstAltOrder" ] - then - echo "path config is not equal to the alternative one: $srcConnectionId, $dstConnectionId, $srcVersion, $dstVersion, $srcOrder, $dstOrder" - exit 1 - fi + if [ "$srcConnectionId" != "$srcAltConnectionId" -o "$dstConnectionId" != "$dstAltConnectionId" -o "$srcVersion" != "$srcAltVersion" -o "$dstVersion" != "$dstAltVersion" -o "$srcOrder" != "$srcAltOrder" -o "$dstOrder" != "$dstAltOrder" ] + then + echo "path config is not equal to the alternative one: $srcConnectionId, $dstConnectionId, $srcVersion, $dstVersion, $srcOrder, $dstOrder" + exit 1 + fi else - echo "expectedSide is invalid value: $expectedSide" - exit 1 + echo "expectedSide is invalid value: $expectedSide" + exit 1 fi } From 5dd0d0d1942b797e032ee52c4a188b4ae48bf462 Mon Sep 17 00:00:00 2001 From: Takeshi Arabiki Date: Wed, 27 Aug 2025 10:57:09 +0900 Subject: [PATCH 3/6] Handle case where current state can never reach target state According to the IBC spec https://github.com/cosmos/ibc/blob/main/spec/core/ics-004-channel-and-packet-semantics/UPGRADES.md#upgrade-handshake, some states may be skipped during the channel upgrade handshake. For example, the stat pair (INIT, FLUSHING) may transition directly to (FLUSHCOMPLETE, FLUSHING) without passing through FLUSHING from INIT. Signed-off-by: Takeshi Arabiki --- core/channel-upgrade.go | 49 ++++++++++++++++--- .../cases/tm2tm/scripts/test-channel-upgrade | 16 ++++-- 2 files changed, 55 insertions(+), 10 deletions(-) diff --git a/core/channel-upgrade.go b/core/channel-upgrade.go index 83e9722c..3c2d3d23 100644 --- a/core/channel-upgrade.go +++ b/core/channel-upgrade.go @@ -144,6 +144,11 @@ func ExecuteChannelUpgrade(ctx context.Context, pathName string, src, dst *Prova logger := GetChannelPairLogger(src, dst) defer logger.TimeTrackContext(ctx, time.Now(), "ExecuteChannelUpgrade") + if (targetSrcState == UPGRADE_STATE_UNINIT && targetDstState == UPGRADE_STATE_INIT) || + (targetSrcState == UPGRADE_STATE_INIT && targetDstState == UPGRADE_STATE_UNINIT) { + return fmt.Errorf("unreachable target state pair: (%s, %s)", targetSrcState, targetDstState) + } + failures := 0 firstCall := true err := runUntilComplete(ctx, interval, func() (bool, error) { @@ -381,12 +386,15 @@ func upgradeChannelStep(ctx context.Context, src, dst *ProvableChain, targetSrcS ), )} - // check if both chains have reached the target states or UNINIT states - if srcState == targetSrcState && dstState == targetDstState { - if firstCall && srcState == UPGRADE_STATE_UNINIT && dstState == UPGRADE_STATE_UNINIT { - logger.InfoContext(ctx, "both chains have already reached the target states, or the channel upgrade has not been initialized") + if hasReachedOrPassedTargetState(srcState, targetSrcState, dstState) && hasReachedOrPassedTargetState(dstState, targetDstState, srcState) { + if firstCall { + if srcState == UPGRADE_STATE_UNINIT && targetSrcState == UPGRADE_STATE_UNINIT { + logger.InfoContext(ctx, "both chains have already reached or passed the target states, or the channel upgrade has not been initialized") + } else { + logger.InfoContext(ctx, "both chains have already reached or passed the target states") + } } else { - logger.InfoContext(ctx, "both chains have reached the target states") + logger.InfoContext(ctx, "both chains have reached or passed the target states") } out.Last = true return out, nil @@ -397,7 +405,8 @@ func upgradeChannelStep(ctx context.Context, src, dst *ProvableChain, targetSrcS dstAction := UPGRADE_ACTION_NONE switch { case srcState == UPGRADE_STATE_UNINIT && dstState == UPGRADE_STATE_UNINIT: - return nil, errors.New("channel upgrade has not been initialized; it will never reach the target states") + // This line should never be reached because the channel upgrade is considered completed + return nil, errors.New("unexpected transition") case srcState == UPGRADE_STATE_INIT && dstState == UPGRADE_STATE_UNINIT: if dstChan.Channel.UpgradeSequence >= srcChan.Channel.UpgradeSequence { srcAction = UPGRADE_ACTION_CANCEL @@ -793,3 +802,31 @@ func buildActionMsg( panic(fmt.Errorf("unexpected action: %s", action)) } } + +// hasReachedOrPassedTargetState checks if the current state has reached or passed the target state, +// including cases where the target state is skipped. +func hasReachedOrPassedTargetState(currentState, targetState, counterpartyCurrentState UpgradeState) bool { + return currentState == targetState || hasPassedTargetState(currentState, targetState, counterpartyCurrentState) +} + +// hasPassedTargetState checks if the current state has passed the target state, +// including cases where the target state is skipped. +func hasPassedTargetState(currentState, targetState, counterpartyCurrentState UpgradeState) bool { + // Check if the current state has passed the target state. + // For simplicity, we consider that any state that can be reached after the target state has passed the target state. + // NOTE: Each chain can cancel the upgrade and initialize another upgrade at any time. This means that the state + // can transition to UNINIT from any state except the case where the counterparty is in INIT state. + + isUninitReachable := counterpartyCurrentState != UPGRADE_STATE_INIT + switch targetState { + case UPGRADE_STATE_INIT: + return currentState == UPGRADE_STATE_FLUSHING || currentState == UPGRADE_STATE_FLUSHCOMPLETE || + (isUninitReachable && currentState == UPGRADE_STATE_UNINIT) + case UPGRADE_STATE_FLUSHING: + return currentState == UPGRADE_STATE_FLUSHCOMPLETE || (isUninitReachable && currentState == UPGRADE_STATE_UNINIT) + case UPGRADE_STATE_FLUSHCOMPLETE: + return isUninitReachable && currentState == UPGRADE_STATE_UNINIT + default: + return false + } +} diff --git a/tests/cases/tm2tm/scripts/test-channel-upgrade b/tests/cases/tm2tm/scripts/test-channel-upgrade index 85a96cff..1002a109 100755 --- a/tests/cases/tm2tm/scripts/test-channel-upgrade +++ b/tests/cases/tm2tm/scripts/test-channel-upgrade @@ -147,10 +147,18 @@ $RLY tx channel-upgrade execute ibc01 checkResult orig echo '##### case 11 #####' -# The target states will never be reached -if $RLY tx channel-upgrade execute ibc01 --target-src-state INIT --target-dst-state FLUSHING +$RLY tx channel-upgrade init ibc01 ibc0 $altSrcOpts +$RLY tx channel-upgrade execute ibc01 --target-src-state FLUSHING --target-dst-state FLUSHING +# Both chains have already passed the target states +$RLY tx channel-upgrade execute ibc01 --target-src-state INIT --target-dst-state INIT +$RLY tx channel-upgrade execute ibc01 +checkResult alt + +echo '##### case 12 #####' +# Unreachable target pair (INIT, UNINIT) +if $RLY tx channel-upgrade execute ibc01 --target-src-state INIT then - echo 'channel-upgrade completed with exit code 0, but non-zero code was expected' >&2 + echo 'channel-upgrade finished with exit code 0, but non-zero code was expected' >&2 exit 1 fi -checkResult orig +checkResult alt From 71754df880626df9d90ed31d55a224dca8dd2bef Mon Sep 17 00:00:00 2001 From: Takeshi Arabiki Date: Wed, 29 Oct 2025 12:39:34 +0900 Subject: [PATCH 4/6] Improve comments related to channel upgrade Signed-off-by: Takeshi Arabiki --- core/channel-upgrade.go | 8 ++++---- tests/cases/tm2tm/scripts/test-channel-upgrade | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/core/channel-upgrade.go b/core/channel-upgrade.go index 3c2d3d23..e78e3799 100644 --- a/core/channel-upgrade.go +++ b/core/channel-upgrade.go @@ -812,12 +812,12 @@ func hasReachedOrPassedTargetState(currentState, targetState, counterpartyCurren // hasPassedTargetState checks if the current state has passed the target state, // including cases where the target state is skipped. func hasPassedTargetState(currentState, targetState, counterpartyCurrentState UpgradeState) bool { - // Check if the current state has passed the target state. - // For simplicity, we consider that any state that can be reached after the target state has passed the target state. - // NOTE: Each chain can cancel the upgrade and initialize another upgrade at any time. This means that the state + // Each chain can cancel the upgrade and initialize another upgrade at any time. This means that the state // can transition to UNINIT from any state except the case where the counterparty is in INIT state. - isUninitReachable := counterpartyCurrentState != UPGRADE_STATE_INIT + + // Check if the current state has passed the target state. + // For simplicity, we consider that any state that can be reached after the target state has passed the target state. switch targetState { case UPGRADE_STATE_INIT: return currentState == UPGRADE_STATE_FLUSHING || currentState == UPGRADE_STATE_FLUSHCOMPLETE || diff --git a/tests/cases/tm2tm/scripts/test-channel-upgrade b/tests/cases/tm2tm/scripts/test-channel-upgrade index 1002a109..fcc59ecc 100755 --- a/tests/cases/tm2tm/scripts/test-channel-upgrade +++ b/tests/cases/tm2tm/scripts/test-channel-upgrade @@ -149,8 +149,9 @@ checkResult orig echo '##### case 11 #####' $RLY tx channel-upgrade init ibc01 ibc0 $altSrcOpts $RLY tx channel-upgrade execute ibc01 --target-src-state FLUSHING --target-dst-state FLUSHING -# Both chains have already passed the target states +# No action is taken will happen because both chains have already passed the target states (INIT) $RLY tx channel-upgrade execute ibc01 --target-src-state INIT --target-dst-state INIT +checkResult orig $RLY tx channel-upgrade execute ibc01 checkResult alt From d883820b157542178fb04ea1b06bb764752cdead Mon Sep 17 00:00:00 2001 From: Takeshi Arabiki Date: Wed, 29 Oct 2025 19:52:43 +0900 Subject: [PATCH 5/6] Add comment on target state pair validation and improve test Signed-off-by: Takeshi Arabiki --- core/channel-upgrade.go | 6 ++++++ tests/cases/tm2tm/scripts/test-channel-upgrade | 3 +++ 2 files changed, 9 insertions(+) diff --git a/core/channel-upgrade.go b/core/channel-upgrade.go index e78e3799..d46d3be5 100644 --- a/core/channel-upgrade.go +++ b/core/channel-upgrade.go @@ -144,6 +144,12 @@ func ExecuteChannelUpgrade(ctx context.Context, pathName string, src, dst *Prova logger := GetChannelPairLogger(src, dst) defer logger.TimeTrackContext(ctx, time.Now(), "ExecuteChannelUpgrade") + // Assume the current state pair is (INIT, UNINIT). + // The target state pair (INIT, UNINIT) and (UNINIT, INIT) are invalid: + // - (INIT, UNINIT) is meaningless as a target since this function won't cause any state change + // - (UNINIT, INIT) is unreachable + // Therefore, it is unlikely that a user would intentionally set either pair after + // initializing an upgrade on only one side, so return an error. if (targetSrcState == UPGRADE_STATE_UNINIT && targetDstState == UPGRADE_STATE_INIT) || (targetSrcState == UPGRADE_STATE_INIT && targetDstState == UPGRADE_STATE_UNINIT) { return fmt.Errorf("unreachable target state pair: (%s, %s)", targetSrcState, targetDstState) diff --git a/tests/cases/tm2tm/scripts/test-channel-upgrade b/tests/cases/tm2tm/scripts/test-channel-upgrade index fcc59ecc..b7cb56cf 100755 --- a/tests/cases/tm2tm/scripts/test-channel-upgrade +++ b/tests/cases/tm2tm/scripts/test-channel-upgrade @@ -156,6 +156,7 @@ $RLY tx channel-upgrade execute ibc01 checkResult alt echo '##### case 12 #####' +$RLY tx channel-upgrade init ibc01 ibc0 $origSrcOpts # (UNINIT, INIT) # Unreachable target pair (INIT, UNINIT) if $RLY tx channel-upgrade execute ibc01 --target-src-state INIT then @@ -163,3 +164,5 @@ then exit 1 fi checkResult alt +$RLY tx channel-upgrade execute ibc01 +checkResult orig From 01eed49fa0970544e7d7abace816c41bfa9b55cc Mon Sep 17 00:00:00 2001 From: abicky Date: Sat, 7 Mar 2026 22:23:28 +0900 Subject: [PATCH 6/6] Refactor ExecuteChannelUpgrade Signed-off-by: abicky --- core/channel-upgrade.go | 57 ++++++++++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 15 deletions(-) diff --git a/core/channel-upgrade.go b/core/channel-upgrade.go index d46d3be5..7cec339c 100644 --- a/core/channel-upgrade.go +++ b/core/channel-upgrade.go @@ -144,15 +144,8 @@ func ExecuteChannelUpgrade(ctx context.Context, pathName string, src, dst *Prova logger := GetChannelPairLogger(src, dst) defer logger.TimeTrackContext(ctx, time.Now(), "ExecuteChannelUpgrade") - // Assume the current state pair is (INIT, UNINIT). - // The target state pair (INIT, UNINIT) and (UNINIT, INIT) are invalid: - // - (INIT, UNINIT) is meaningless as a target since this function won't cause any state change - // - (UNINIT, INIT) is unreachable - // Therefore, it is unlikely that a user would intentionally set either pair after - // initializing an upgrade on only one side, so return an error. - if (targetSrcState == UPGRADE_STATE_UNINIT && targetDstState == UPGRADE_STATE_INIT) || - (targetSrcState == UPGRADE_STATE_INIT && targetDstState == UPGRADE_STATE_UNINIT) { - return fmt.Errorf("unreachable target state pair: (%s, %s)", targetSrcState, targetDstState) + if err := validateTargetStates(targetSrcState, targetDstState); err != nil { + return err } failures := 0 @@ -167,7 +160,7 @@ func ExecuteChannelUpgrade(ctx context.Context, pathName string, src, dst *Prova firstCall = false if steps.Last { - logger.InfoContext(ctx, "Channel upgrade completed") + logger.InfoContext(ctx, "Channel upgrade steps completed up to the target states. Check the resulting states, as cancellations or timeouts can also lead to this result.") return true, nil } @@ -809,21 +802,55 @@ func buildActionMsg( } } -// hasReachedOrPassedTargetState checks if the current state has reached or passed the target state, -// including cases where the target state is skipped. +// validateTargetStates returns an error if the target state pair is meaningless or unreachable. +// Assume the current state pair is (INIT, UNINIT). +// +// - The target state pair (INIT, UNINIT) is meaningless as a target +// because this function would not cause any state change. +// - The target state pair (UNINIT, INIT) is unreachable. +// +// Therefore, it is unlikely that a user would intentionally specify either pair +// after initializing an upgrade on only one side, so we treat them as invalid +// and return an error. +// +// This validation also simplifies the logic of hasPassedTargetState. +// For example, +// +// - target state pair: (UNINIT, INIT) +// - current state pair: (INIT, UNINIT) +// +// In this case, hasPassedTargetState(UNINIT, INIT, INIT) returns false +// and the upgrade proceeds to the next step, which is unexpected. +// By rejecting this target state pair, the function does not need to handle this case. +func validateTargetStates(targetSrcState, targetDstState UpgradeState) error { + if (targetSrcState == UPGRADE_STATE_UNINIT && targetDstState == UPGRADE_STATE_INIT) || + (targetSrcState == UPGRADE_STATE_INIT && targetDstState == UPGRADE_STATE_UNINIT) { + return fmt.Errorf("unreachable target state pair: (%s, %s)", targetSrcState, targetDstState) + } + + return nil +} + +// hasReachedOrPassedTargetState checks if the current state has reached or passed the target state. +// For more details, see hasPassedTargetState. func hasReachedOrPassedTargetState(currentState, targetState, counterpartyCurrentState UpgradeState) bool { return currentState == targetState || hasPassedTargetState(currentState, targetState, counterpartyCurrentState) } // hasPassedTargetState checks if the current state has passed the target state, -// including cases where the target state is skipped. +// including cases where the target state is skipped. For example, UPGRADE_STATE_INIT can transition +// directly to UPGRADE_STATE_FLUSHCOMPLETE, skipping UPGRADE_STATE_FLUSHING. func hasPassedTargetState(currentState, targetState, counterpartyCurrentState UpgradeState) bool { // Each chain can cancel the upgrade and initialize another upgrade at any time. This means that the state - // can transition to UNINIT from any state except the case where the counterparty is in INIT state. + // can transition to UNINIT from any state. + // However, in the case where the counterparty state is INIT, the current state can be UNINIT when the upgrade + // has not been initialized. In this case this function should return false, + // so we treat UNINIT as unreachable. isUninitReachable := counterpartyCurrentState != UPGRADE_STATE_INIT // Check if the current state has passed the target state. - // For simplicity, we consider that any state that can be reached after the target state has passed the target state. + // For simplicity, any state reachable after the target state is considered to have passed the target state, + // including cases where the upgrade has been cancelled or timed out. switch targetState { case UPGRADE_STATE_INIT: return currentState == UPGRADE_STATE_FLUSHING || currentState == UPGRADE_STATE_FLUSHCOMPLETE ||