fix: allow zero proof height, solo machine discards provided proof height in favor of sequence

CHANGELOG.md

@@ -73,6 +73,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Improvements
 
+* (light-clients/06-solomachine) [\#2746](https://github.com/cosmos/ibc-go/pull/2746) Discard proofHeight for solo machines and use sequence. Allow proof height to be zero for connection handshake.
 * (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`. 

modules/core/03-connection/types/msgs.go

@@ -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")
 	}
@@ -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")
 	}
@@ -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)

modules/core/03-connection/types/msgs_test.go

@@ -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},
@@ -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},
@@ -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),
 	}
@@ -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 {

modules/core/04-channel/keeper/handshake_test.go

@@ -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)

modules/light-clients/06-solomachine/client_state.go

@@ -102,20 +102,20 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
 	return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade solomachine client")
 }
 
-// VerifyMembership is a generic proof verification method which verifies a proof of the existence of a value at a given CommitmentPath at the specified height.
+// VerifyMembership is a generic proof verification method which verifies a proof of the existence of a value at a given CommitmentPath at the latest sequence.
 // The caller is expected to construct the full CommitmentPath from a CommitmentPrefix and a standardized path (as defined in ICS 24).
 func (cs *ClientState) VerifyMembership(
 	ctx sdk.Context,
 	clientStore sdk.KVStore,
 	cdc codec.BinaryCodec,
-	height exported.Height,
+	_ exported.Height,
 	delayTimePeriod uint64,
 	delayBlockPeriod uint64,
 	proof []byte,
 	path []byte,
 	value []byte,
 ) error {
-	publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, proof)
+	publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, proof)
 	if err != nil {
 		return err
 	}
@@ -149,19 +149,19 @@ func (cs *ClientState) VerifyMembership(
 	return nil
 }
 
-// VerifyNonMembership is a generic proof verification method which verifies the absence of a given CommitmentPath at a specified height.
+// VerifyNonMembership is a generic proof verification method which verifies the absence of a given CommitmentPath at the latest sequence.
 // The caller is expected to construct the full CommitmentPath from a CommitmentPrefix and a standardized path (as defined in ICS 24).
 func (cs *ClientState) VerifyNonMembership(
 	ctx sdk.Context,
 	clientStore sdk.KVStore,
 	cdc codec.BinaryCodec,
-	height exported.Height,
+	_ exported.Height,
 	delayTimePeriod uint64,
 	delayBlockPeriod uint64,
 	proof []byte,
 	path []byte,
 ) error {
-	publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, proof)
+	publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, proof)
 	if err != nil {
 		return err
 	}
@@ -197,18 +197,12 @@ func (cs *ClientState) VerifyNonMembership(
 
 // produceVerificationArgs perfoms the basic checks on the arguments that are
 // shared between the verification functions and returns the public key of the
-// consensus state, the unmarshalled proof representing the signature and timestamp
-// along with the solo-machine sequence encoded in the proofHeight.
+// consensus state, the unmarshalled proof representing the signature and timestamp.
 func produceVerificationArgs(
 	cdc codec.BinaryCodec,
 	cs *ClientState,
-	height exported.Height,
 	proof []byte,
 ) (cryptotypes.PubKey, signing.SignatureData, uint64, uint64, error) {
-	if revision := height.GetRevisionNumber(); revision != 0 {
-		return nil, nil, 0, 0, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "revision must be 0 for solomachine, got revision-number: %d", revision)
-	}
-
 	if proof == nil {
 		return nil, nil, 0, 0, sdkerrors.Wrap(ErrInvalidProof, "proof cannot be empty")
 	}
@@ -228,20 +222,11 @@ func produceVerificationArgs(
 		return nil, nil, 0, 0, err
 	}
 
-	// sequence is encoded in the revision height of height struct
-	sequence := height.GetRevisionHeight()
-	latestSequence := cs.GetLatestHeight().GetRevisionHeight()
-	if latestSequence != sequence {
-		return nil, nil, 0, 0, sdkerrors.Wrapf(
-			sdkerrors.ErrInvalidHeight,
-			"client state sequence != proof sequence (%d != %d)", latestSequence, sequence,
-		)
-	}
-
 	if cs.ConsensusState.GetTimestamp() > timestamp {
 		return nil, nil, 0, 0, sdkerrors.Wrapf(ErrInvalidProof, "the consensus state timestamp is greater than the signature timestamp (%d >= %d)", cs.ConsensusState.GetTimestamp(), timestamp)
 	}
 
+	sequence := cs.GetLatestHeight().GetRevisionHeight()
 	publicKey, err := cs.ConsensusState.GetPubKey()
 	if err != nil {
 		return nil, nil, 0, 0, err

modules/light-clients/06-solomachine/client_state_test.go

@@ -143,12 +143,11 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 
 		var (
 			clientState *solomachine.ClientState
-			err         error
-			height      clienttypes.Height
 			path        []byte
 			proof       []byte
 			testingPath *ibctesting.Path
 			signBytes   solomachine.SignBytes
+			err         error
 		)
 
 		testCases := []struct {
@@ -203,7 +202,7 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 					consensusStateBz, err := suite.chainA.Codec.Marshal(consensusState)
 					suite.Require().NoError(err)
 
-					merklePath := sm.GetConsensusStatePath(counterpartyClientIdentifier, height)
+					merklePath := sm.GetConsensusStatePath(counterpartyClientIdentifier, clienttypes.NewHeight(0, 1))
 					signBytes = solomachine.SignBytes{
 						Sequence:    sm.Sequence,
 						Timestamp:   sm.Time,
@@ -445,25 +444,6 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 				},
 				true,
 			},
-			{
-				"client state latest height is less than sequence",
-				func() {
-					consensusState := &solomachine.ConsensusState{
-						Timestamp: sm.Time,
-						PublicKey: sm.ConsensusState().PublicKey,
-					}
-
-					clientState = solomachine.NewClientState(sm.Sequence-1, consensusState)
-				},
-				false,
-			},
-			{
-				"height revision number is not zero",
-				func() {
-					height = clienttypes.NewHeight(1, sm.GetHeight().GetRevisionHeight())
-				},
-				false,
-			},
 			{
 				"malformed merkle path fails to unmarshal",
 				func() {
@@ -551,7 +531,6 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 				testingPath = ibctesting.NewPath(suite.chainA, suite.chainB)
 
 				clientState = sm.ClientState()
-				height = clienttypes.NewHeight(sm.GetHeight().GetRevisionNumber(), sm.GetHeight().GetRevisionHeight())
 
 				merklePath := commitmenttypes.NewMerklePath("ibc", "solomachine")
 				signBytes = solomachine.SignBytes{
@@ -587,7 +566,7 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 
 				err = clientState.VerifyMembership(
 					suite.chainA.GetContext(), suite.store, suite.chainA.Codec,
-					height, 0, 0, // solomachine does not check delay periods
+					clienttypes.ZeroHeight(), 0, 0, // solomachine does not check delay periods
 					proof, path, signBytes.Data,
 				)
 
@@ -609,11 +588,10 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {
 
 		var (
 			clientState *solomachine.ClientState
-			err         error
-			height      clienttypes.Height
 			path        []byte
 			proof       []byte
 			signBytes   solomachine.SignBytes
+			err         error
 		)
 
 		testCases := []struct {
@@ -656,25 +634,6 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {
 				},
 				true,
 			},
-			{
-				"client state latest height is less than sequence",
-				func() {
-					consensusState := &solomachine.ConsensusState{
-						Timestamp: sm.Time,
-						PublicKey: sm.ConsensusState().PublicKey,
-					}
-
-					clientState = solomachine.NewClientState(sm.Sequence-1, consensusState)
-				},
-				false,
-			},
-			{
-				"height revision number is not zero",
-				func() {
-					height = clienttypes.NewHeight(1, sm.GetHeight().GetRevisionHeight())
-				},
-				false,
-			},
 			{
 				"malformed merkle path fails to unmarshal",
 				func() {
@@ -772,7 +731,6 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {
 
 			suite.Run(tc.name, func() {
 				clientState = sm.ClientState()
-				height = clienttypes.NewHeight(sm.GetHeight().GetRevisionNumber(), sm.GetHeight().GetRevisionHeight())
 
 				merklePath := commitmenttypes.NewMerklePath("ibc", "solomachine")
 				signBytes = solomachine.SignBytes{
@@ -808,7 +766,7 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {
 
 				err = clientState.VerifyNonMembership(
 					suite.chainA.GetContext(), suite.store, suite.chainA.Codec,
-					height, 0, 0, // solomachine does not check delay periods
+					clienttypes.ZeroHeight(), 0, 0, // solomachine does not check delay periods
 					proof, path,
 				)
 

modules/light-clients/06-solomachine/solomachine_test.go

@@ -46,6 +46,18 @@ func TestSoloMachineTestSuite(t *testing.T) {
 	suite.Run(t, new(SoloMachineTestSuite))
 }
 
+func (suite *SoloMachineTestSuite) TestConnectionHandshake() {
+	clientID := suite.solomachine.CreateClient(suite.chainA)
+
+	connectionID := suite.solomachine.ConnOpenInit(suite.chainA, clientID)
+
+	// open try is not necessary as the solo machine implementation is mock'd
+
+	suite.solomachine.ConnOpenAck(suite.chainA, clientID, connectionID)
+
+	// open confirm is not necessary as the solo machine implementation is mock'd
+}
+
 func (suite *SoloMachineTestSuite) GetSequenceFromStore() uint64 {
 	bz := suite.store.Get(host.ClientStateKey())
 	suite.Require().NotNil(bz)