Skip to content

Make channel-upgrade execute command fully idempotent #171

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

Open
abicky wants to merge 6 commits into
base: main
Choose a base branch
from
+117 −18
Open

Make channel-upgrade execute command fully idempotent #171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a7a9496
Make channel-upgrade execute command fully idempotent
abicky Aug 1, 2025
f9b6353
Replace tab with spaces in shell script
abicky Aug 1, 2025
5dd0d0d
Handle case where current state can never reach target state
abicky Aug 27, 2025
71754df
Improve comments related to channel upgrade
abicky Oct 29, 2025
d883820
Add comment on target state pair validation and improve test
abicky Oct 29, 2025
01eed49
Refactor ExecuteChannelUpgrade
abicky Mar 7, 2026
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
85 changes: 79 additions & 6 deletions core/channel-upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,10 @@ func ExecuteChannelUpgrade(ctx context.Context, pathName string, src, dst *Prova
logger := GetChannelPairLogger(src, dst)
defer logger.TimeTrackContext(ctx, time.Now(), "ExecuteChannelUpgrade")

if err := validateTargetStates(targetSrcState, targetDstState); err != nil {
return err
}

failures := 0
firstCall := true
err := runUntilComplete(ctx, interval, func() (bool, error) {
Expand All @@ -156,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
}

Expand Down Expand Up @@ -381,10 +385,16 @@ 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 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 or passed the target states")
}
out.Last = true
return out, nil
}
Expand All @@ -394,7 +404,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 is not initialized")
// 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
Expand Down Expand Up @@ -790,3 +801,65 @@ func buildActionMsg(
panic(fmt.Errorf("unexpected action: %s", action))
}
}

// 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. 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.
// 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, 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 ||
(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
}
}
50 changes: 38 additions & 12 deletions tests/cases/tm2tm/scripts/test-channel-upgrade
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -140,3 +140,29 @@ $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 #####'
$RLY tx channel-upgrade init ibc01 ibc0 $altSrcOpts
$RLY tx channel-upgrade execute ibc01 --target-src-state FLUSHING --target-dst-state FLUSHING
# 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

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
echo 'channel-upgrade finished with exit code 0, but non-zero code was expected' >&2
exit 1
fi
checkResult alt
$RLY tx channel-upgrade execute ibc01
checkResult orig