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

Use counterparty connection hops when verifying channel state #4074

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
10 changes: 9 additions & 1 deletion modules/core/04-channel/keeper/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -355,11 +355,19 @@ func (k Keeper) ChanUpgradeOpen(
if !found {
return errorsmod.Wrapf(types.ErrUpgradeNotFound, "failed to retrieve channel upgrade: port ID (%s) channel ID (%s)", portID, channelID)
}
// If counterparty has reached OPEN, we must use the upgraded connection to verify the counterparty channel
Copy link
Contributor Author

Choose a reason for hiding this comment

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

linky-link more, opened issue on spec repo to keep track cosmos/ibc#1004

upgradedConnection, err := k.GetConnection(ctx, upgrade.Fields.ConnectionHops[0])
if err != nil {
return errorsmod.Wrap(err, "failed to retrieve connection using the upgrade connection hops")
}
if upgradedConnection.GetState() != int32(connectiontypes.OPEN) {
return errorsmod.Wrapf(connectiontypes.ErrInvalidConnectionState, "connection state is not OPEN (got %s)", connectiontypes.State(upgradedConnection.GetState()).String())
}
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved

counterpartyChannel = types.Channel{
State: types.OPEN,
Ordering: upgrade.Fields.Ordering,
ConnectionHops: upgrade.Fields.ConnectionHops,
ConnectionHops: []string{upgradedConnection.GetCounterparty().GetConnectionID()},
Counterparty: types.NewCounterparty(portID, channelID),
Version: upgrade.Fields.Version,
UpgradeSequence: channel.UpgradeSequence,
Expand Down
85 changes: 85 additions & 0 deletions modules/core/04-channel/keeper/upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,91 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() {
}
}

// TestChanUpgradeOpenCounterPartyStates tests the handshake in the cases where
// the counterparty is in a state other than OPEN.
func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterPartyStates() {
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
var path *ibctesting.Path
testCases := []struct {
name string
malleate func()
expError error
}{
{
"success, counterparty in OPEN",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe we could integrate these 2 tests in the TestChanUpgradeOpen function above instead of adding a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya, this is a tradeoff really, we can add these in the main test but require each test case call the ugprade handshake explicitly or we can separate these. Colin also mentioned how he preferred the separation (ref: #3844 (comment)).

I'm open to both options, I think this way it might be slightly cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

i would be fine with merging this pr as is :)

func() {
err := path.EndpointB.ChanUpgradeInit()
suite.Require().NoError(err)

err = path.EndpointA.ChanUpgradeTry()
suite.Require().NoError(err)

err = path.EndpointB.ChanUpgradeAck()
suite.Require().NoError(err)

// TODO: Remove when #4030 is closed. Channel will automatically
Copy link
Contributor Author

Choose a reason for hiding this comment

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

link #4030

// move to OPEN in that case.
err = path.EndpointB.ChanUpgradeOpen()
suite.Require().NoError(err)

suite.coordinator.CommitBlock(suite.chainA, suite.chainB)
suite.Require().NoError(path.EndpointA.UpdateClient())
},
nil,
},
{
"success, counterparty in TRYUPGRADE",
func() {
err := path.EndpointA.ChanUpgradeInit()
suite.Require().NoError(err)

err = path.EndpointB.ChanUpgradeTry()
suite.Require().NoError(err)

err = path.EndpointA.ChanUpgradeAck()
suite.Require().NoError(err)
},
nil,
},
}

// Create an initial path used only to invoke ConnOpenInit/ChanOpenInit handlers.
// This bumps the connection/channel identifiers generated for chain A on the
// next path used to run the upgrade handshake.
// See issue 4062.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

path = ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.SetupClients(path)
suite.Require().NoError(path.EndpointA.ConnOpenInit())
suite.coordinator.SetupConnections(path)
suite.Require().NoError(path.EndpointA.ChanOpenInit())

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

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

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

tc.malleate()

proofCounterpartyChannel, _, proofHeight := path.EndpointA.QueryChannelUpgradeProof()
err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeOpen(
suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID,
path.EndpointB.GetChannel().State, proofCounterpartyChannel, proofHeight,
)

if tc.expError == nil {
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
suite.Require().NoError(err)
} else {
suite.Require().ErrorIs(err, tc.expError)
}
})
}
}

func (suite *KeeperTestSuite) TestChanUpgradeTimeout() {
var (
path *ibctesting.Path
Expand Down