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.

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

Jump to bottom

backport-v8.5.x: self client/consensus state removal from connection handshake #7129

Merged
merged 4 commits into from
Aug 13, 2024
Merged

backport-v8.5.x: self client/consensus state removal from connection handshake #7129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d336d2
refactor: remove client/consensus state validation from connection ha…
colin-axner Aug 12, 2024
45751f9
chore: generate proto deprecation notice
colin-axner Aug 12, 2024
ab413b0
add changelog
colin-axner Aug 12, 2024
354d397
refactor: remove unpacking from msg server
colin-axner Aug 12, 2024
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ Ref: https://keepachangelog.com/en/1.0.0/

# Changelog

## [v8.5.0]()

### State Machine Breaking

* (core/03-connection)[\#7129](https://github.com/cosmos/ibc-go/pull/7129) Remove verification of self client and consensus state from connection handshake.

## [v8.4.0](https://github.com/cosmos/ibc-go/releases/tag/v8.4.0) - 2024-07-29

### Improvements
Expand Down
12 changes: 6 additions & 6 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ go 1.21
module github.com/cosmos/ibc-go/v8

retract (
// contain bug in channel upgradability that can result
// in a channel upgrade succeeding but with channel ends
// in incompatible state
[v8.2.0, v8.3.2]
// contain ASA-2024-007 vulnerability
[v8.0.0, v8.1.1]
// contain bug in channel upgradability that can result
// in a channel upgrade succeeding but with channel ends
// in incompatible state
[v8.2.0, v8.3.2]
// contain ASA-2024-007 vulnerability
[v8.0.0, v8.1.1]
)

require (
Expand Down
82 changes: 8 additions & 74 deletions modules/core/03-connection/keeper/handshake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
"github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"
commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types"
ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
"github.com/cosmos/ibc-go/v8/modules/core/exported"
)

Expand Down Expand Up @@ -72,37 +71,17 @@ func (k Keeper) ConnOpenTry(
counterparty types.Counterparty, // counterpartyConnectionIdentifier, counterpartyPrefix and counterpartyClientIdentifier
delayPeriod uint64,
clientID string, // clientID of chainA
clientState exported.ClientState, // clientState that chainA has for chainB
_ exported.ClientState, // clientState that chainA has for chainB
counterpartyVersions []*types.Version, // supported versions of chain A
initProof []byte, // proof that chainA stored connectionEnd in state (on ConnOpenInit)
clientProof []byte, // proof that chainA stored a light client of chainB
consensusProof []byte, // proof that chainA stored chainB's consensus state at consensus height
_ []byte, // proof that chainA stored a light client of chainB
_ []byte, // proof that chainA stored chainB's consensus state at consensus height
proofHeight exported.Height, // height at which relayer constructs proof of A storing connectionEnd in state
consensusHeight exported.Height, // latest height of chain B which chain A has stored in its chain B client
_ exported.Height, // latest height of chain B which chain A has stored in its chain B client
) (string, error) {
// generate a new connection
connectionID := k.GenerateConnectionIdentifier(ctx)

// check that the consensus height the counterparty chain is using to store a representation
// of this chain's consensus state is at a height in the past
selfHeight := clienttypes.GetSelfHeight(ctx)
if consensusHeight.GTE(selfHeight) {
return "", errorsmod.Wrapf(
ibcerrors.ErrInvalidHeight,
"consensus height is greater than or equal to the current block height (%s >= %s)", consensusHeight, selfHeight,
)
}

// validate client parameters of a chainB client stored on chainA
if err := k.consensusHost.ValidateSelfClient(ctx, clientState); err != nil {
return "", err
}

expectedConsensusState, err := k.consensusHost.GetSelfConsensusState(ctx, consensusHeight)
if err != nil {
return "", errorsmod.Wrapf(err, "self consensus state not found for height %s", consensusHeight.String())
}

// expectedConnection defines Chain A's ConnectionEnd
// NOTE: chain A's counterparty is chain B (i.e where this code is executed)
// NOTE: chainA and chainB must have the same delay period
Expand All @@ -129,18 +108,6 @@ func (k Keeper) ConnOpenTry(
return "", err
}

// Check that ChainA stored the clientState provided in the msg
if err := k.VerifyClientState(ctx, connection, proofHeight, clientProof, clientState); err != nil {
return "", err
}

// Check that ChainA stored the correct ConsensusState of chainB at the given consensusHeight
if err := k.VerifyClientConsensusState(
ctx, connection, proofHeight, consensusHeight, consensusProof, expectedConsensusState,
); err != nil {
return "", err
}

// store connection in chainB state
if err := k.addConnectionToClient(ctx, clientID, connectionID); err != nil {
return "", errorsmod.Wrapf(err, "failed to add connection with ID %s to client with ID %s", connectionID, clientID)
Expand All @@ -163,25 +130,15 @@ func (k Keeper) ConnOpenTry(
func (k Keeper) ConnOpenAck(
ctx sdk.Context,
connectionID string,
clientState exported.ClientState, // client state for chainA on chainB
_ exported.ClientState, // client state for chainA on chainB
version *types.Version, // version that ChainB chose in ConnOpenTry
counterpartyConnectionID string,
tryProof []byte, // proof that connectionEnd was added to ChainB state in ConnOpenTry
clientProof []byte, // proof of client state on chainB for chainA
consensusProof []byte, // proof that chainB has stored ConsensusState of chainA on its client
_ []byte, // proof of client state on chainB for chainA
_ []byte, // proof that chainB has stored ConsensusState of chainA on its client
proofHeight exported.Height, // height that relayer constructed proofTry
consensusHeight exported.Height, // latest height of chainA that chainB has stored on its chainA client
_ exported.Height, // latest height of chainA that chainB has stored on its chainA client
) error {
// check that the consensus height the counterparty chain is using to store a representation
// of this chain's consensus state is at a height in the past
selfHeight := clienttypes.GetSelfHeight(ctx)
if consensusHeight.GTE(selfHeight) {
return errorsmod.Wrapf(
ibcerrors.ErrInvalidHeight,
"consensus height is greater than or equal to the current block height (%s >= %s)", consensusHeight, selfHeight,
)
}

// Retrieve connection
connection, found := k.GetConnection(ctx, connectionID)
if !found {
Expand All @@ -204,17 +161,6 @@ func (k Keeper) ConnOpenAck(
)
}

// validate client parameters of a chainA client stored on chainB
if err := k.consensusHost.ValidateSelfClient(ctx, clientState); err != nil {
return err
}

// Retrieve chainA's consensus state at consensusheight
expectedConsensusState, err := k.consensusHost.GetSelfConsensusState(ctx, consensusHeight)
if err != nil {
return errorsmod.Wrapf(err, "self consensus state not found for height %s", consensusHeight.String())
}

prefix := k.GetCommitmentPrefix()
expectedCounterparty := types.NewCounterparty(connection.ClientId, connectionID, commitmenttypes.NewMerklePrefix(prefix.Bytes()))
expectedConnection := types.NewConnectionEnd(types.TRYOPEN, connection.Counterparty.ClientId, expectedCounterparty, []*types.Version{version}, connection.DelayPeriod)
Expand All @@ -227,18 +173,6 @@ func (k Keeper) ConnOpenAck(
return err
}

// Check that ChainB stored the clientState provided in the msg
if err := k.VerifyClientState(ctx, connection, proofHeight, clientProof, clientState); err != nil {
return err
}

// Ensure that ChainB has stored the correct ConsensusState for chainA at the consensusHeight
if err := k.VerifyClientConsensusState(
ctx, connection, proofHeight, consensusHeight, consensusProof, expectedConsensusState,
); err != nil {
return err
}

k.Logger(ctx).Info("connection state updated", "connection-id", connectionID, "previous-state", types.INIT.String(), "new-state", types.OPEN.String())

defer telemetry.IncrCounter(1, "ibc", "connection", "open-ack")
Expand Down
157 changes: 1 addition & 156 deletions modules/core/03-connection/keeper/handshake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,11 @@ package keeper_test
import (
"time"

sdk "github.com/cosmos/cosmos-sdk/types"

clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
"github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"
host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
"github.com/cosmos/ibc-go/v8/modules/core/exported"
ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint"
ibctesting "github.com/cosmos/ibc-go/v8/testing"
"github.com/cosmos/ibc-go/v8/testing/mock"
)

// TestConnOpenInit - chainA initializes (INIT state) a connection with
Expand Down Expand Up @@ -135,38 +131,6 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
// retrieve client state of chainA to pass as counterpartyClient
counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
}, true},
{"invalid counterparty client", func() {
err := path.EndpointA.ConnOpenInit()
suite.Require().NoError(err)

// retrieve client state of chainB to pass as counterpartyClient
counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)

// Set an invalid client of chainA on chainB
tmClient, ok := counterpartyClient.(*ibctm.ClientState)
suite.Require().True(ok)
tmClient.ChainId = "wrongchainid"

suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID, tmClient)
}, false},
{"consensus height >= latest height", func() {
err := path.EndpointA.ConnOpenInit()
suite.Require().NoError(err)

// retrieve client state of chainA to pass as counterpartyClient
counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)

consensusHeight = clienttypes.GetSelfHeight(suite.chainB.GetContext())
}, false},
{"self consensus state not found", func() {
err := path.EndpointA.ConnOpenInit()
suite.Require().NoError(err)

// retrieve client state of chainA to pass as counterpartyClient
counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)

consensusHeight = clienttypes.NewHeight(0, 1)
}, false},
{"counterparty versions is empty", func() {
err := path.EndpointA.ConnOpenInit()
suite.Require().NoError(err)
Expand All @@ -192,50 +156,6 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
// retrieve client state of chainA to pass as counterpartyClient
counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
}, false},
{"client state verification failed", func() {
err := path.EndpointA.ConnOpenInit()
suite.Require().NoError(err)

// retrieve client state of chainA to pass as counterpartyClient
counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)

// modify counterparty client without setting in store so it still passes validate but fails proof verification
tmClient, ok := counterpartyClient.(*ibctm.ClientState)
suite.Require().True(ok)
tmClient.LatestHeight = tmClient.LatestHeight.Increment().(clienttypes.Height)
}, false},
{"consensus state verification failed", func() {
// retrieve client state of chainA to pass as counterpartyClient
counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)

// give chainA wrong consensus state for chainB
consState, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetLatestClientConsensusState(suite.chainA.GetContext(), path.EndpointA.ClientID)
suite.Require().True(found)

tmConsState, ok := consState.(*ibctm.ConsensusState)
suite.Require().True(ok)

tmConsState.Timestamp = time.Now()
suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), path.EndpointA.ClientID, counterpartyClient.GetLatestHeight(), tmConsState)

err := path.EndpointA.ConnOpenInit()
suite.Require().NoError(err)
}, false},
{"override self consensus host", func() {
err := path.EndpointA.ConnOpenInit()
suite.Require().NoError(err)

// retrieve client state of chainA to pass as counterpartyClient
counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)

mockValidator := mock.ConsensusHost{
ValidateSelfClientFn: func(ctx sdk.Context, clientState exported.ClientState) error {
return mock.MockApplicationCallbackError
},
}

suite.chainB.App.GetIBCKeeper().SetConsensusHost(&mockValidator)
}, false},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -313,35 +233,6 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
// retrieve client state of chainB to pass as counterpartyClient
counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)
}, true},
{"invalid counterparty client", func() {
err := path.EndpointA.ConnOpenInit()
suite.Require().NoError(err)

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

// retrieve client state of chainB to pass as counterpartyClient
counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)

// Set an invalid client of chainA on chainB
tmClient, ok := counterpartyClient.(*ibctm.ClientState)
suite.Require().True(ok)
tmClient.ChainId = "wrongchainid"

suite.chainB.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainB.GetContext(), path.EndpointB.ClientID, tmClient)
}, false},
{"consensus height >= latest height", func() {
err := path.EndpointA.ConnOpenInit()
suite.Require().NoError(err)

// retrieve client state of chainB to pass as counterpartyClient
counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)

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

consensusHeight = clienttypes.GetSelfHeight(suite.chainA.GetContext())
}, false},
{"connection not found", func() {
// connections are never created

Expand All @@ -363,6 +254,7 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
suite.Require().True(found)

connection.Counterparty.ConnectionId = "badconnectionid"
path.EndpointB.ConnectionID = "badconnectionid"

suite.chainA.App.GetIBCKeeper().ConnectionKeeper.SetConnection(suite.chainA.GetContext(), path.EndpointA.ConnectionID, connection)

Expand Down Expand Up @@ -436,18 +328,6 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {

version = types.NewVersion(types.DefaultIBCVersionIdentifier, []string{"ORDER_ORDERED", "ORDER_UNORDERED", "ORDER_DAG"})
}, false},
{"self consensus state not found", func() {
err := path.EndpointA.ConnOpenInit()
suite.Require().NoError(err)

// retrieve client state of chainB to pass as counterpartyClient
counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)

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

consensusHeight = clienttypes.NewHeight(0, 1)
}, false},
{"connection state verification failed", func() {
// chainB connection is not in INIT
err := path.EndpointA.ConnOpenInit()
Expand All @@ -456,41 +336,6 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
// retrieve client state of chainB to pass as counterpartyClient
counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)
}, false},
{"client state verification failed", func() {
err := path.EndpointA.ConnOpenInit()
suite.Require().NoError(err)

// retrieve client state of chainB to pass as counterpartyClient
counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)

// modify counterparty client without setting in store so it still passes validate but fails proof verification
tmClient, ok := counterpartyClient.(*ibctm.ClientState)
suite.Require().True(ok)
tmClient.LatestHeight = tmClient.LatestHeight.Increment().(clienttypes.Height)

err = path.EndpointB.ConnOpenTry()
suite.Require().NoError(err)
}, false},
{"consensus state verification failed", func() {
err := path.EndpointA.ConnOpenInit()
suite.Require().NoError(err)

// retrieve client state of chainB to pass as counterpartyClient
counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)

// give chainB wrong consensus state for chainA
consState, found := suite.chainB.App.GetIBCKeeper().ClientKeeper.GetLatestClientConsensusState(suite.chainB.GetContext(), path.EndpointB.ClientID)
suite.Require().True(found)

tmConsState, ok := consState.(*ibctm.ConsensusState)
suite.Require().True(ok)

tmConsState.Timestamp = tmConsState.Timestamp.Add(time.Second)
suite.chainB.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainB.GetContext(), path.EndpointB.ClientID, counterpartyClient.GetLatestHeight(), tmConsState)

err = path.EndpointB.ConnOpenTry()
suite.Require().NoError(err)
}, false},
}

for _, tc := range testCases {
Expand Down
Loading
Loading