Skip to content

Commit

Permalink
fix: allow zero proof height, solo machine discards provided proof he…
Browse files Browse the repository at this point in the history 
…ight in favor of sequence (#2746)

imp: allow proof height to be zero for all core IBC `sdk.Msg` types that contain proofs.
imp: discard proofHeight for solo machines and use the solo machine sequence instead.
  • Loading branch information
@colin-axner
colin-axner committed Nov 22, 2022
1 parent fe5fb15 commit ac561b4
Show file tree
Hide file tree
Showing 12 changed files with 501 additions and 140 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

* (core) [\#2746](https://github.com/cosmos/ibc-go/pull/2746) Allow proof height to be zero for all core IBC `sdk.Msg` types that contain proofs.
* (light-clients/06-solomachine) [\#2746](https://github.com/cosmos/ibc-go/pull/2746) Discard proofHeight for solo machines and use the solo machine sequence instead.
* (modules/light-clients/07-tendermint) [\#1713](https://github.com/cosmos/ibc-go/pull/1713) Allow client upgrade proposals to update `TrustingPeriod`. See ADR-026 for context.
* (modules/core/02-client) [\#1188](https://github.com/cosmos/ibc-go/pull/1188/files) Routing `MsgSubmitMisbehaviour` to `UpdateClient` keeper function. Deprecating `SubmitMisbehaviour` endpoint.
* (modules/core/02-client) [\#1208](https://github.com/cosmos/ibc-go/pull/1208) Replace `CheckHeaderAndUpdateState` usage in 02-client with calls to `VerifyClientMessage`, `CheckForMisbehaviour`, `UpdateStateOnMisbehaviour` and `UpdateState`.
Expand Down
2 changes: 2 additions & 0 deletions docs/migrations/v6-to-v7.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ The `CheckMisbehaviourAndUpdateState` function has been removed from `ClientStat

The function `GetTimestampAtHeight` has been added to the `ClientState` interface. It should return the timestamp for a consensus state associated with the provided height.

A zero proof height is now allowed by core IBC and may be passed into `VerifyMembership` and `VerifyNonMembership`. Light clients are responsible for returning an error if a zero proof height is invalid behaviour.

### `Header` and `Misbehaviour`

`exported.Header` and `exported.Misbehaviour` interface types have been merged and renamed to `ClientMessage` interface.
Expand Down
9 changes: 0 additions & 9 deletions modules/core/03-connection/types/msgs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,6 @@ func (msg MsgConnectionOpenTry) ValidateBasic() error {
if len(msg.ProofConsensus) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof of consensus state")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
if msg.ConsensusHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "consensus height must be non-zero")
}
Expand Down Expand Up @@ -226,9 +223,6 @@ func (msg MsgConnectionOpenAck) ValidateBasic() error {
if len(msg.ProofConsensus) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof of consensus state")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
if msg.ConsensusHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "consensus height must be non-zero")
}
Expand Down Expand Up @@ -271,9 +265,6 @@ func (msg MsgConnectionOpenConfirm) ValidateBasic() error {
if len(msg.ProofAck) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof ack")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
_, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
Expand Down
8 changes: 2 additions & 6 deletions modules/core/03-connection/types/msgs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenTry() {
{"empty proofInit", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, emptyProof, suite.proof, suite.proof, clientHeight, clientHeight, signer), false},
{"empty proofClient", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, emptyProof, suite.proof, clientHeight, clientHeight, signer), false},
{"empty proofConsensus", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, emptyProof, clientHeight, clientHeight, signer), false},
{"invalid proofHeight", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clienttypes.ZeroHeight(), clientHeight, signer), false},
{"invalid consensusHeight", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clienttypes.ZeroHeight(), signer), false},
{"empty singer", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, ""), false},
{"success", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), true},
Expand Down Expand Up @@ -191,7 +190,6 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenAck() {
{"empty proofTry", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, emptyProof, suite.proof, suite.proof, clientHeight, clientHeight, ibctesting.ConnectionVersion, signer), false},
{"empty proofClient", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, suite.proof, emptyProof, suite.proof, clientHeight, clientHeight, ibctesting.ConnectionVersion, signer), false},
{"empty proofConsensus", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, suite.proof, suite.proof, emptyProof, clientHeight, clientHeight, ibctesting.ConnectionVersion, signer), false},
{"invalid proofHeight", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, suite.proof, suite.proof, suite.proof, clienttypes.ZeroHeight(), clientHeight, ibctesting.ConnectionVersion, signer), false},
{"invalid consensusHeight", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, suite.proof, suite.proof, suite.proof, clientHeight, clienttypes.ZeroHeight(), ibctesting.ConnectionVersion, signer), false},
{"invalid version", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, &types.Version{}, signer), false},
{"empty signer", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, ibctesting.ConnectionVersion, ""), false},
Expand All @@ -212,7 +210,6 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenConfirm() {
testMsgs := []*types.MsgConnectionOpenConfirm{
types.NewMsgConnectionOpenConfirm("test/conn1", suite.proof, clientHeight, signer),
types.NewMsgConnectionOpenConfirm(connectionID, emptyProof, clientHeight, signer),
types.NewMsgConnectionOpenConfirm(connectionID, suite.proof, clienttypes.ZeroHeight(), signer),
types.NewMsgConnectionOpenConfirm(connectionID, suite.proof, clientHeight, ""),
types.NewMsgConnectionOpenConfirm(connectionID, suite.proof, clientHeight, signer),
}
Expand All @@ -224,9 +221,8 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenConfirm() {
}{
{testMsgs[0], false, "invalid connection ID"},
{testMsgs[1], false, "empty proofTry"},
{testMsgs[2], false, "invalid proofHeight"},
{testMsgs[3], false, "empty signer"},
{testMsgs[4], true, "success"},
{testMsgs[2], false, "empty signer"},
{testMsgs[3], true, "success"},
}

for i, tc := range testCases {
Expand Down
2 changes: 1 addition & 1 deletion modules/core/04-channel/keeper/handshake_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ func (suite *KeeperTestSuite) TestChanCloseInit() {
path.SetChannelOrdered()
err = path.EndpointA.ChanOpenInit()
suite.Require().NoError(err)

// ensure channel capability check passes
suite.chainA.CreateChannelCapability(suite.chainA.GetSimApp().ScopedIBCMockKeeper, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
Expand Down
24 changes: 0 additions & 24 deletions modules/core/04-channel/types/msgs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,6 @@ func (msg MsgChannelOpenTry) ValidateBasic() error {
if len(msg.ProofInit) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof init")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
if msg.Channel.State != TRYOPEN {
return sdkerrors.Wrapf(ErrInvalidChannelState,
"channel state must be TRYOPEN in MsgChannelOpenTry. expected: %s, got: %s",
Expand Down Expand Up @@ -159,9 +156,6 @@ func (msg MsgChannelOpenAck) ValidateBasic() error {
if len(msg.ProofTry) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof try")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
_, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
Expand Down Expand Up @@ -207,9 +201,6 @@ func (msg MsgChannelOpenConfirm) ValidateBasic() error {
if len(msg.ProofAck) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof ack")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
_, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
Expand Down Expand Up @@ -294,9 +285,6 @@ func (msg MsgChannelCloseConfirm) ValidateBasic() error {
if len(msg.ProofInit) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof init")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
_, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
Expand Down Expand Up @@ -335,9 +323,6 @@ func (msg MsgRecvPacket) ValidateBasic() error {
if len(msg.ProofCommitment) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
_, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
Expand Down Expand Up @@ -384,9 +369,6 @@ func (msg MsgTimeout) ValidateBasic() error {
if len(msg.ProofUnreceived) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty unreceived proof")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
if msg.NextSequenceRecv == 0 {
return sdkerrors.Wrap(sdkerrors.ErrInvalidSequence, "next sequence receive cannot be 0")
}
Expand Down Expand Up @@ -435,9 +417,6 @@ func (msg MsgTimeoutOnClose) ValidateBasic() error {
if len(msg.ProofClose) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof of closed counterparty channel end")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
_, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
Expand Down Expand Up @@ -479,9 +458,6 @@ func (msg MsgAcknowledgement) ValidateBasic() error {
if len(msg.ProofAcked) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
if len(msg.Acknowledgement) == 0 {
return sdkerrors.Wrap(ErrInvalidAcknowledgement, "ack bytes cannot be empty")
}
Expand Down
13 changes: 1 addition & 12 deletions modules/core/04-channel/types/msgs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
"github.com/cosmos/ibc-go/v6/modules/core/04-channel/types"
commitmenttypes "github.com/cosmos/ibc-go/v6/modules/core/23-commitment/types"
"github.com/cosmos/ibc-go/v6/modules/core/exported"
"github.com/cosmos/ibc-go/v6/testing/simapp"
)

Expand Down Expand Up @@ -56,9 +55,7 @@ var (
packet = types.NewPacket(validPacketData, 1, portid, chanid, cpportid, cpchanid, timeoutHeight, timeoutTimestamp)
invalidPacket = types.NewPacket(unknownPacketData, 0, portid, chanid, cpportid, cpchanid, timeoutHeight, timeoutTimestamp)

emptyProof = []byte{}
invalidProofs1 = exported.Proof(nil)
invalidProofs2 = emptyProof
emptyProof = []byte{}

addr = sdk.AccAddress("testaddr111111111111").String()
emptyAddr string
Expand Down Expand Up @@ -158,7 +155,6 @@ func (suite *TypesTestSuite) TestMsgChannelOpenTryValidateBasic() {
{"too long port id", types.NewMsgChannelOpenTry(invalidLongPort, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false},
{"port id contains non-alpha", types.NewMsgChannelOpenTry(invalidPort, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false},
{"", types.NewMsgChannelOpenTry(portid, version, types.ORDERED, connHops, cpportid, cpchanid, "", suite.proof, height, addr), true},
{"proof height is zero", types.NewMsgChannelOpenTry(portid, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, clienttypes.ZeroHeight(), addr), false},
{"invalid channel order", types.NewMsgChannelOpenTry(portid, version, types.Order(4), connHops, cpportid, cpchanid, version, suite.proof, height, addr), false},
{"connection hops more than 1 ", types.NewMsgChannelOpenTry(portid, version, types.UNORDERED, invalidConnHops, cpportid, cpchanid, version, suite.proof, height, addr), false},
{"too short connection id", types.NewMsgChannelOpenTry(portid, version, types.UNORDERED, invalidShortConnHops, cpportid, cpchanid, version, suite.proof, height, addr), false},
Expand Down Expand Up @@ -202,7 +198,6 @@ func (suite *TypesTestSuite) TestMsgChannelOpenAckValidateBasic() {
{"channel id contains non-alpha", types.NewMsgChannelOpenAck(portid, invalidChannel, chanid, version, suite.proof, height, addr), false},
{"", types.NewMsgChannelOpenAck(portid, chanid, chanid, "", suite.proof, height, addr), true},
{"empty proof", types.NewMsgChannelOpenAck(portid, chanid, chanid, version, emptyProof, height, addr), false},
{"proof height is zero", types.NewMsgChannelOpenAck(portid, chanid, chanid, version, suite.proof, clienttypes.ZeroHeight(), addr), false},
{"invalid counterparty channel id", types.NewMsgChannelOpenAck(portid, chanid, invalidShortChannel, version, suite.proof, height, addr), false},
}

Expand Down Expand Up @@ -235,7 +230,6 @@ func (suite *TypesTestSuite) TestMsgChannelOpenConfirmValidateBasic() {
{"too long channel id", types.NewMsgChannelOpenConfirm(portid, invalidLongChannel, suite.proof, height, addr), false},
{"channel id contains non-alpha", types.NewMsgChannelOpenConfirm(portid, invalidChannel, suite.proof, height, addr), false},
{"empty proof", types.NewMsgChannelOpenConfirm(portid, chanid, emptyProof, height, addr), false},
{"proof height is zero", types.NewMsgChannelOpenConfirm(portid, chanid, suite.proof, clienttypes.ZeroHeight(), addr), false},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -297,7 +291,6 @@ func (suite *TypesTestSuite) TestMsgChannelCloseConfirmValidateBasic() {
{"too long channel id", types.NewMsgChannelCloseConfirm(portid, invalidLongChannel, suite.proof, height, addr), false},
{"channel id contains non-alpha", types.NewMsgChannelCloseConfirm(portid, invalidChannel, suite.proof, height, addr), false},
{"empty proof", types.NewMsgChannelCloseConfirm(portid, chanid, emptyProof, height, addr), false},
{"proof height is zero", types.NewMsgChannelCloseConfirm(portid, chanid, suite.proof, clienttypes.ZeroHeight(), addr), false},
}

for _, tc := range testCases {
Expand All @@ -322,7 +315,6 @@ func (suite *TypesTestSuite) TestMsgRecvPacketValidateBasic() {
expPass bool
}{
{"success", types.NewMsgRecvPacket(packet, suite.proof, height, addr), true},
{"proof height is zero", types.NewMsgRecvPacket(packet, suite.proof, clienttypes.ZeroHeight(), addr), false},
{"proof contain empty proof", types.NewMsgRecvPacket(packet, emptyProof, height, addr), false},
{"missing signer address", types.NewMsgRecvPacket(packet, suite.proof, height, emptyAddr), false},
{"invalid packet", types.NewMsgRecvPacket(invalidPacket, suite.proof, height, addr), false},
Expand Down Expand Up @@ -358,7 +350,6 @@ func (suite *TypesTestSuite) TestMsgTimeoutValidateBasic() {
expPass bool
}{
{"success", types.NewMsgTimeout(packet, 1, suite.proof, height, addr), true},
{"proof height must be > 0", types.NewMsgTimeout(packet, 1, suite.proof, clienttypes.ZeroHeight(), addr), false},
{"seq 0", types.NewMsgTimeout(packet, 0, suite.proof, height, addr), false},
{"missing signer address", types.NewMsgTimeout(packet, 1, suite.proof, height, emptyAddr), false},
{"cannot submit an empty proof", types.NewMsgTimeout(packet, 1, emptyProof, height, addr), false},
Expand Down Expand Up @@ -390,7 +381,6 @@ func (suite *TypesTestSuite) TestMsgTimeoutOnCloseValidateBasic() {
{"seq 0", types.NewMsgTimeoutOnClose(packet, 0, suite.proof, suite.proof, height, addr), false},
{"empty proof", types.NewMsgTimeoutOnClose(packet, 1, emptyProof, suite.proof, height, addr), false},
{"empty proof close", types.NewMsgTimeoutOnClose(packet, 1, suite.proof, emptyProof, height, addr), false},
{"proof height is zero", types.NewMsgTimeoutOnClose(packet, 1, suite.proof, suite.proof, clienttypes.ZeroHeight(), addr), false},
{"signer address is empty", types.NewMsgTimeoutOnClose(packet, 1, suite.proof, suite.proof, height, emptyAddr), false},
{"invalid packet", types.NewMsgTimeoutOnClose(invalidPacket, 1, suite.proof, suite.proof, height, addr), false},
}
Expand All @@ -417,7 +407,6 @@ func (suite *TypesTestSuite) TestMsgAcknowledgementValidateBasic() {
expPass bool
}{
{"success", types.NewMsgAcknowledgement(packet, packet.GetData(), suite.proof, height, addr), true},
{"proof height must be > 0", types.NewMsgAcknowledgement(packet, packet.GetData(), suite.proof, clienttypes.ZeroHeight(), addr), false},
{"empty ack", types.NewMsgAcknowledgement(packet, nil, suite.proof, height, addr), false},
{"missing signer address", types.NewMsgAcknowledgement(packet, packet.GetData(), suite.proof, height, emptyAddr), false},
{"cannot submit an empty proof", types.NewMsgAcknowledgement(packet, packet.GetData(), emptyProof, height, addr), false},
Expand Down
Loading

0 comments on commit ac561b4

Please sign in to comment.