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

ChanUpgradeTimeout handler for 04-channel #3829

Merged
merged 30 commits into from
Jun 17, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
50b1163
writeupgradetimeout method, pull in util methods
charleenfei Jun 8, 2023
ba1fde4
update method to use abort method instead of restoreChannel
charleenfei Jun 9, 2023
8cd8c0b
Merge branch '04-channel-upgrades' into charly/#37500-write-upgrade-t…
charleenfei Jun 9, 2023
392a5b5
add expected keepers
charleenfei Jun 12, 2023
0a363de
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 12, 2023
a227949
finish upgrade timeout handler
charleenfei Jun 12, 2023
8b18b02
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 12, 2023
7b8b67b
add tests
charleenfei Jun 13, 2023
760940b
lint
charleenfei Jun 13, 2023
106ac13
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 13, 2023
7f12c0b
update test
charleenfei Jun 13, 2023
29f84ce
rm events file from pr
charleenfei Jun 13, 2023
16f196a
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 13, 2023
17748af
rm unnecessary param in test
charleenfei Jun 13, 2023
7341c5a
Merge branch 'charly/#3747-upgrade-timeout-hand' of github.com:cosmos…
charleenfei Jun 13, 2023
e9a2c20
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 13, 2023
900dcb6
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 13, 2023
8cf4a62
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 13, 2023
661f006
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 14, 2023
ba5e566
update method order in upgrade.go
charleenfei Jun 14, 2023
e3315ea
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 14, 2023
e1e7da7
pr comments
charleenfei Jun 14, 2023
de2607d
add connection OPEN check + test
charleenfei Jun 15, 2023
634b53e
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 15, 2023
6070c07
use default timeout height, small nit
charleenfei Jun 15, 2023
8fc3ee6
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 15, 2023
2df489c
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 16, 2023
065b003
set ErrReceipt to pointer
charleenfei Jun 16, 2023
5b9fa3e
lint
charleenfei Jun 17, 2023
470ee69
Merge branch '04-channel-upgrades' into charly/#3747-upgrade-timeout-…
charleenfei Jun 17, 2023
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
151 changes: 125 additions & 26 deletions modules/core/04-channel/keeper/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types"
"github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
"github.com/cosmos/ibc-go/v7/modules/core/exported"
)

// ChanUpgradeInit is called by a module to initiate a channel upgrade handshake with
Expand Down Expand Up @@ -204,32 +205,6 @@ func (k Keeper) WriteUpgradeTryChannel(ctx sdk.Context, portID, channelID string
return channel, upgrade
}

// WriteUpgradeAckChannel writes a channel which has successfully passed the UpgradeAck handshake step as well as
// setting the upgrade for that channel.
// An event is emitted for the handshake step.
func (k Keeper) WriteUpgradeAckChannel(
ctx sdk.Context,
portID, channelID string,
proposedUpgrade types.Upgrade,
) {
defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-ack")

channel, found := k.GetChannel(ctx, portID, channelID)
if !found {
panic(fmt.Sprintf("could not find existing channel when updating channel state in successful ChanUpgradeAck step, channelID: %s, portID: %s", channelID, portID))
}

previousState := channel.State
channel.State = types.ACKUPGRADE
channel.FlushStatus = types.FLUSHING

k.SetChannel(ctx, portID, channelID, channel)
k.SetUpgrade(ctx, portID, channelID, proposedUpgrade)

k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState, "new-state", types.ACKUPGRADE.String())
emitChannelUpgradeAckEvent(ctx, portID, channelID, channel, proposedUpgrade)
}

// ChanUpgradeAck is called by a module to accept the ACKUPGRADE handshake step of the channel upgrade protocol.
// This method should only be called by the IBC core msg server.
// This method will verify that the counterparty has entered TRYUPGRADE
Expand Down Expand Up @@ -293,6 +268,130 @@ func (k Keeper) ChanUpgradeAck(
return nil
}

// ChanUpgradeTimeout times out an outstanding upgrade.
// This should be used by the initialising chain when the counterparty chain has not responded to an upgrade proposal within the specified timeout period.
func (k Keeper) ChanUpgradeTimeout(
ctx sdk.Context,
portID, channelID string,
counterpartyChannel types.Channel,
prevErrorReceipt types.ErrorReceipt,
proofCounterpartyChannel,
proofErrorReceipt []byte,
proofHeight exported.Height,
) error {
channel, found := k.GetChannel(ctx, portID, channelID)
if !found {
return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
}

if channel.State != types.INITUPGRADE {
return errorsmod.Wrapf(types.ErrInvalidChannelState, "channel state is not INITUPGRADE (got %s)", channel.State)
}

upgrade, found := k.GetUpgrade(ctx, portID, channelID)
if !found {
return errorsmod.Wrapf(types.ErrUpgradeNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
}

connection, found := k.connectionKeeper.GetConnection(ctx, channel.ConnectionHops[0])
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
if !found {
return errorsmod.Wrap(
connectiontypes.ErrConnectionNotFound,
channel.ConnectionHops[0],
)
}

// proof must be from a height after timeout has elapsed. Either timeoutHeight or timeoutTimestamp must be defined.
// if timeoutHeight is defined and proof is from before timeout height, abort transaction
proofTimestamp, err := k.connectionKeeper.GetTimestampAtHeight(ctx, connection, proofHeight)
if err != nil {
return err
}

if (upgrade.Timeout.Height.IsZero() || proofHeight.LT(upgrade.Timeout.Height)) &&
(upgrade.Timeout.Timestamp == 0 || proofTimestamp < upgrade.Timeout.Timestamp) {
return errorsmod.Wrap(types.ErrInvalidUpgradeTimeout, "timeout has not yet passed on counterparty chain")
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
}

// verify the counterparty channel state
if err := k.connectionKeeper.VerifyChannelState(
ctx,
connection,
proofHeight, proofCounterpartyChannel,
channel.Counterparty.PortId,
channel.Counterparty.ChannelId,
counterpartyChannel,
); err != nil {
return errorsmod.Wrap(err, "failed to verify counterparty channel state")
}

// counterparty channel must be proved to still be in OPEN state or INITUPGRADE state (crossing hellos)
if !collections.Contains(counterpartyChannel.State, []types.State{types.OPEN, types.INITUPGRADE}) {
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s], got %s", types.OPEN, types.INITUPGRADE, counterpartyChannel.State)
}

// Error receipt passed in is either nil or it is a stale error receipt from a previous upgrade
if (prevErrorReceipt == types.ErrorReceipt{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to change the prevErrorReceipt to be of type *types.ErrorReceipt. This would be more in line with the spec. There is no functional difference anyway.

err := k.connectionKeeper.VerifyChannelUpgradeErrorAbsence(
ctx,
channel.Counterparty.PortId, channel.Counterparty.ChannelId,
connection,
proofErrorReceipt,
proofHeight,
)
if err != nil {
return errorsmod.Wrap(err, "failed to verify absence of counterparty channel upgrade error receipt")
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] to reduce indentation we can remove the else, and just add a return nil the line above this.

// timeout for this sequence can only succeed if the error receipt written into the error path on the counterparty
// was for a previous sequence by the timeout deadline.
upgradeSequence := channel.UpgradeSequence
if upgradeSequence < prevErrorReceipt.Sequence {
return errorsmod.Wrapf(types.ErrInvalidUpgradeSequence, "previous counterparty error receipt sequence is greater than our current upgrade sequence: %d > %d", prevErrorReceipt.Sequence, upgradeSequence)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

idea: maybe we could also just call ChanUpgradeCancel here in this case, instead of forcing the user to call.

}

err := k.connectionKeeper.VerifyChannelUpgradeError(
ctx,
channel.Counterparty.PortId, channel.Counterparty.ChannelId,
connection,
prevErrorReceipt,
proofErrorReceipt,
proofHeight,
)
if err != nil {
return errorsmod.Wrap(err, "failed to verify counterparty channel upgrade error receipt")
}
}

return nil
}

// WriteUpgradeAckChannel writes a channel which has successfully passed the UpgradeAck handshake step as well as
// setting the upgrade for that channel.
// An event is emitted for the handshake step.
func (k Keeper) WriteUpgradeAckChannel(
ctx sdk.Context,
portID, channelID string,
proposedUpgrade types.Upgrade,
) {
defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-ack")

channel, found := k.GetChannel(ctx, portID, channelID)
if !found {
panic(fmt.Sprintf("could not find existing channel when updating channel state in successful ChanUpgradeAck step, channelID: %s, portID: %s", channelID, portID))
}

previousState := channel.State
channel.State = types.ACKUPGRADE
channel.FlushStatus = types.FLUSHING

k.SetChannel(ctx, portID, channelID, channel)
k.SetUpgrade(ctx, portID, channelID, proposedUpgrade)

k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState, "new-state", types.ACKUPGRADE.String())
emitChannelUpgradeAckEvent(ctx, portID, channelID, channel, proposedUpgrade)
}

// startFlushUpgradeHandshake will verify the counterparty proposed upgrade and the current channel state.
// Once the counterparty information has been verified, it will be validated against the self proposed upgrade.
// If any of the proposed upgrade fields are incompatible, an upgrade error will be returned resulting in an
Expand Down
168 changes: 168 additions & 0 deletions modules/core/04-channel/keeper/upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,174 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() {
}
}

func (suite *KeeperTestSuite) TestChanUpgradeTimeout() {
var (
path *ibctesting.Path
errReceipt types.ErrorReceipt
proofHeight exported.Height
proofCounterpartyChannel []byte
proofErrorReceipt []byte
)

testCases := []struct {
name string
malleate func()
expError error
}{
{
"success",
func() {
},
nil,
},
{
"success: non-nil error receipt",
func() {
errReceipt = types.ErrorReceipt{
Sequence: 1,
Message: types.ErrInvalidUpgrade.Error(),
}

suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errReceipt)

suite.Require().NoError(path.EndpointB.UpdateClient())
suite.Require().NoError(path.EndpointA.UpdateClient())

proofCounterpartyChannel, _, proofHeight = path.EndpointA.QueryChannelUpgradeProof()
upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
proofErrorReceipt, _ = suite.chainB.QueryProof(upgradeErrorReceiptKey)
},
nil,
},
{
"channel not found",
func() {
path.EndpointA.ChannelID = ibctesting.InvalidID
},
types.ErrChannelNotFound,
},
{
"channel state is not in INITUPGRADE state",
func() {
suite.Require().NoError(path.EndpointA.SetChannelState(types.ACKUPGRADE))
},
types.ErrInvalidChannelState,
},
{
"current upgrade not found",
func() {
suite.chainA.DeleteKey(host.ChannelUpgradeKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID))
},
types.ErrUpgradeNotFound,
},
{
"connection not found",
func() {
channel := path.EndpointA.GetChannel()
channel.ConnectionHops[0] = ibctesting.InvalidID
path.EndpointA.SetChannel(channel)
},
connectiontypes.ErrConnectionNotFound,
},
{
"unable to retrieve timestamp at proof height",
func() {
proofHeight = clienttypes.NewHeight(0, uint64(path.EndpointA.Chain.GetContext().BlockHeight()+100))
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
},
clienttypes.ErrConsensusStateNotFound,
},
{
"timeout has not passed",
func() {
upgrade := path.EndpointA.GetProposedUpgrade()
upgrade.Timeout.Height = clienttypes.NewHeight(1, uint64(path.EndpointA.Chain.GetContext().BlockHeight()+100))
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
upgrade.Timeout.Height = clienttypes.NewHeight(1, uint64(path.EndpointA.Chain.GetContext().BlockHeight()+100))
upgrade.Timeout.Height = suite.chainA.GetTimeoutHeight()

suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgrade)

suite.Require().NoError(path.EndpointA.UpdateClient())

proofCounterpartyChannel, _, proofHeight = path.EndpointA.QueryChannelUpgradeProof()
upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
proofErrorReceipt, _ = suite.chainB.QueryProof(upgradeErrorReceiptKey)
},
types.ErrInvalidUpgradeTimeout,
},
{
"counterparty channel state is not OPEN or INITUPGRADE (crossing hellos)",
func() {
channel := path.EndpointB.GetChannel()
channel.State = types.TRYUPGRADE
path.EndpointB.SetChannel(channel)

suite.Require().NoError(path.EndpointB.UpdateClient())
suite.Require().NoError(path.EndpointA.UpdateClient())

proofCounterpartyChannel, _, proofHeight = path.EndpointA.QueryChannelUpgradeProof()
upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
proofErrorReceipt, _ = suite.chainB.QueryProof(upgradeErrorReceiptKey)
},
types.ErrInvalidChannelState,
},
{
"non-nil error receipt: error receipt seq greater than current upgrade seq",
func() {
errReceipt = types.ErrorReceipt{
Sequence: 3,
Message: types.ErrInvalidUpgrade.Error(),
}
},
types.ErrInvalidUpgradeSequence,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()
expPass := tc.expError == nil

path = ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.Setup(path)

path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion

// set timeout height to 1 to ensure timeout
path.EndpointA.ChannelConfig.ProposedUpgrade.Timeout.Height = clienttypes.NewHeight(1, 1)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
err := path.EndpointA.ChanUpgradeInit()
suite.Require().NoError(err)

errReceipt = types.ErrorReceipt{}

// ensure clients are up to date to receive valid proofs
suite.Require().NoError(path.EndpointB.UpdateClient())
suite.Require().NoError(path.EndpointA.UpdateClient())

proofCounterpartyChannel, _, proofHeight = path.EndpointA.QueryChannelUpgradeProof()
upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
proofErrorReceipt, _ = suite.chainB.QueryProof(upgradeErrorReceiptKey)

tc.malleate()

err = suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeTimeout(
suite.chainA.GetContext(),
path.EndpointA.ChannelConfig.PortID,
path.EndpointA.ChannelID,
path.EndpointB.GetChannel(),
errReceipt,
proofCounterpartyChannel,
proofErrorReceipt,
proofHeight,
)

if expPass {
suite.Require().NoError(err)
} else {
suite.assertUpgradeError(err, tc.expError)
}
})
}
}

// TestStartFlushUpgradeHandshake tests the startFlushUpgradeHandshake.
// UpgradeInit will be run on chainA and startFlushUpgradeHandshake
// will be called on chainB
Expand Down
3 changes: 3 additions & 0 deletions modules/core/04-channel/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,7 @@ var (
ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade")
ErrInvalidUpgradeError = errorsmod.Register(SubModuleName, 32, "invalid upgrade error")
ErrInvalidFlushStatus = errorsmod.Register(SubModuleName, 33, "invalid flush status")
ErrUpgradeRestoreFailed = errorsmod.Register(SubModuleName, 34, "restore failed")
ErrUpgradeTimeout = errorsmod.Register(SubModuleName, 35, "upgrade timed-out")
ErrInvalidUpgradeTimeout = errorsmod.Register(SubModuleName, 36, "upgrade timeout is invalid")
)