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

fix: guarentee proof availiability for channel upgrades #5638

Merged
merged 10 commits into from
Jan 23, 2024
4 changes: 4 additions & 0 deletions docs/docs/01-ibc/06-channel-upgrades.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ As part of the handling of the `MsgChannelUpgradeInit` message, the application'

After this message is handled successfully, the channel's upgrade sequence will be incremented. This upgrade sequence will serve as a nonce for the upgrade process to provide replay protection.

::: warning
Initiating an upgrade in the same block as opening a channel may potentially prevent the counterparty channel from also opening.
:::

### Governance gating on `ChanUpgradeInit`

The message signer for `MsgChannelUpgradeInit` must be the address which has been designated as the `authority` of the `IBCKeeper`. If this proposal passes, the counterparty's channel will upgrade by default.
Expand Down
1 change: 1 addition & 0 deletions e2e/tests/core/04-channel/upgrades_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
testifysuite "github.com/stretchr/testify/suite"

sdkmath "cosmossdk.io/math"

govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"

"github.com/cosmos/ibc-go/e2e/testsuite"
Expand Down
2 changes: 2 additions & 0 deletions modules/core/04-channel/keeper/handshake.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,8 @@ func (k Keeper) ChanOpenConfirm(
counterpartyHops, channel.Version,
)

// NOTE: If the counterparty has initialized an upgrade in the same block as performing the
// ACK handshake step, this channel end will be incapable of opening.
return k.connectionKeeper.VerifyChannelState(
ctx, connectionEnd, proofHeight, ackProof,
channel.Counterparty.PortId, channel.Counterparty.ChannelId,
Expand Down
11 changes: 10 additions & 1 deletion modules/core/04-channel/keeper/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,7 @@ func (k Keeper) ChanUpgradeOpen(
portID,
channelID string,
counterpartyChannelState types.State,
counterpartyUpgradeSequence uint64,
channelProof []byte,
proofHeight clienttypes.Height,
) error {
Expand Down Expand Up @@ -510,13 +511,21 @@ func (k Keeper) ChanUpgradeOpen(
return errorsmod.Wrapf(connectiontypes.ErrInvalidConnectionState, "connection state is not OPEN (got %s)", connectiontypes.State(upgradeConnection.GetState()).String())
}

// The counterparty upgrade sequence must be greater than or equal to
// the channel upgrade sequence. It should normally be equivalent, but
// in the unlikely case a new upgrade is initiated after it reopens,
// then the upgrade sequence will be greater than our upgrade sequence.
if counterpartyUpgradeSequence < channel.UpgradeSequence {
return errorsmod.Wrapf(types.ErrInvalidUpgradeSequence, "counterparty upgrade sequence must be greater than or equal to our upgrade sequence, expected: %d, got %d", channel.UpgradeSequence, counterpartyUpgradeSequence)
}

counterpartyChannel = types.Channel{
State: types.OPEN,
Ordering: upgrade.Fields.Ordering,
ConnectionHops: []string{upgradeConnection.GetCounterparty().GetConnectionID()},
Counterparty: types.NewCounterparty(portID, channelID),
Version: upgrade.Fields.Version,
UpgradeSequence: channel.UpgradeSequence,
UpgradeSequence: counterpartyUpgradeSequence,
}

case types.FLUSHCOMPLETE:
Expand Down
25 changes: 24 additions & 1 deletion modules/core/04-channel/keeper/upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1187,6 +1187,29 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() {
},
nil,
},
{
"success: counterparty initiated new upgrade after opening",
func() {
// create reason to upgrade
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + "additional upgrade"

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

err = path.EndpointA.UpdateClient()
suite.Require().NoError(err)
},
nil,
},
{
"success: counterparty upgrade sequence is incorrect",
func() {
counterpartyCh := path.EndpointB.GetChannel()
counterpartyCh.UpgradeSequence--
path.EndpointB.SetChannel(counterpartyCh)
},
types.ErrInvalidUpgradeSequence,
},
{
"channel not found",
func() {
Expand Down Expand Up @@ -1273,7 +1296,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() {

err = suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeOpen(
suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID,
path.EndpointB.GetChannel().State, channelProof, proofHeight,
path.EndpointB.GetChannel().State, path.EndpointB.GetChannel().UpgradeSequence, channelProof, proofHeight,
)

if tc.expError == nil {
Expand Down
18 changes: 12 additions & 6 deletions modules/core/04-channel/types/msgs.go
Original file line number Diff line number Diff line change
Expand Up @@ -593,17 +593,19 @@ func NewMsgChannelUpgradeOpen(
portID,
channelID string,
counterpartyChannelState State,
counterpartyUpgradeSequence uint64,
channelProof []byte,
proofHeight clienttypes.Height,
signer string,
) *MsgChannelUpgradeOpen {
return &MsgChannelUpgradeOpen{
PortId: portID,
ChannelId: channelID,
CounterpartyChannelState: counterpartyChannelState,
ProofChannel: channelProof,
ProofHeight: proofHeight,
Signer: signer,
PortId: portID,
ChannelId: channelID,
CounterpartyChannelState: counterpartyChannelState,
CounterpartyUpgradeSequence: counterpartyUpgradeSequence,
ProofChannel: channelProof,
ProofHeight: proofHeight,
Signer: signer,
}
}

Expand All @@ -625,6 +627,10 @@ func (msg MsgChannelUpgradeOpen) ValidateBasic() error {
return errorsmod.Wrapf(ErrInvalidChannelState, "expected channel state to be one of: [%s, %s], got: %s", FLUSHCOMPLETE, OPEN, msg.CounterpartyChannelState)
}

if msg.CounterpartyUpgradeSequence == 0 {
return errorsmod.Wrap(ErrInvalidUpgradeSequence, "counterparty upgrade sequence must be non-zero")
}

_, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
Expand Down
10 changes: 9 additions & 1 deletion modules/core/04-channel/types/msgs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1575,6 +1575,14 @@ func (suite *TypesTestSuite) TestMsgChannelUpgradeOpenValidateBasic() {
},
errorsmod.Wrapf(types.ErrInvalidChannelState, "expected channel state to be one of: [%s, %s], got: %s", types.FLUSHCOMPLETE, types.OPEN, types.CLOSED),
},
{
"invalid counterparty upgrade seqeuence",
func() {
msg.CounterpartyUpgradeSequence = 0
},
errorsmod.Wrapf(types.ErrInvalidUpgradeSequence, "counterparty upgrade sequence must be non-zero"),
},

{
"cannot submit an empty channel proof",
func() {
Expand All @@ -1596,7 +1604,7 @@ func (suite *TypesTestSuite) TestMsgChannelUpgradeOpenValidateBasic() {
suite.Run(tc.name, func() {
msg = types.NewMsgChannelUpgradeOpen(
ibctesting.MockPort, ibctesting.FirstChannelID,
types.FLUSHCOMPLETE, suite.proof,
types.FLUSHCOMPLETE, 1, suite.proof,
height, addr,
)

Expand Down
Loading
Loading