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 5 commits into
base: main
Choose a base branch
from
+89 −17
Open

Make channel-upgrade execute command fully idempotent #171

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 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
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
50 changes: 45 additions & 5 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,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)
Copy link
Contributor

@siburu siburu Oct 29, 2025

Choose a reason for hiding this comment

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

@abicky Do you mean "the target state pair is unreachable, unless the current state pair already is equal to it"?

Copy link
Contributor Author

@abicky abicky Oct 29, 2025

Choose a reason for hiding this comment

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

Yes. To be precise, this target pair is meaningless or unexpected. As you mentioned, the state pair becomes (INIT, UNINIT) when a user initializes an upgrade on only one side. In this case:

  • yrly tx channel-upgrade execute ibc01 --target-src-state INIT won't cause any state change
    • It is unlikely that any user would intentionally run this command
  • yrly tx channel-upgrade execute ibc01 --target-dst-state INIT shouldn't succeed (unreachable)
    • The user might have run this command unexpectedly

Therefore, I thought it would be better to throw an error to make the hasReachedOrPassedTargetState function simple.

Without this validation, hasReachedOrPassedTargetState will become more complicated. Here's why.

Assume:

  • target state pair: (UNINIT, INIT)
  • current state pair: (INIT, UNINIT)
  • and we remove this validation.

With the current implementation, hasPassedTargetState(UNINIT, INIT, INIT) returns false, so the upgrade proceeds to the next step, which I believe is unexpected.

Copy link
Contributor Author

@abicky abicky Oct 29, 2025

Choose a reason for hiding this comment

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

I've added a comment and improved the test in d883820.

}

failures := 0
firstCall := true
err := runUntilComplete(ctx, interval, func() (bool, error) {
Expand Down Expand Up @@ -381,10 +386,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 +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 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 +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 {
// 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 ||
(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
}
}
47 changes: 35 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,26 @@ $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 #####'
# 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