fix(statemachine)!: use path within IBC store without escaping charac…

…ters in solomachine (backport #4429) (#4453) * fix(statemachine)!: use key within IBC store without escaping characters in solomachine (#4429) (cherry picked from commit 98b0c99) # Conflicts: # modules/light-clients/06-solomachine/client_state.go # modules/light-clients/07-tendermint/upgrade.go * fix conflicts * fix package usage * use sdkerrors * add changelog * Update CHANGELOG.md * Update CHANGELOG.md * Update CHANGELOG.md --------- Co-authored-by: Carlos Rodriguez <[email protected]>
cosmos · Aug 25, 2023 · b4766b0 · b4766b0
1 parent 812a364
commit b4766b0
Show file tree

Hide file tree

Showing 7 changed files with 131 additions and 36 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -47,10 +47,11 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### Improvements
 
 * [\#4187](https://github.com/cosmos/ibc-go/pull/4187) Adds function 'WithICS4Wrapper' to keepers to allow to set the middleware after the keeper's creation.
+* (light-clients/06-solomachine) [\#4429](https://github.com/cosmos/ibc-go/pull/4429) Remove IBC key from path of bytes signed by solomachine and not escape the path.
 
 ### Features
 
-* [\#3796](https://github.com/cosmos/ibc-go/pull/3796) Adds support for json tx encoding for interchain accounts.
+* (apps/27-interchain-accounts) [\#3796](https://github.com/cosmos/ibc-go/pull/3796) Adds support for json tx encoding for interchain accounts.
 * [\#4188](https://github.com/cosmos/ibc-go/pull/4188) Adds optional `PacketDataUnmarshaler` interface that allows a middleware to request the packet data to be unmarshaled by the base application.
 * [\#4199](https://github.com/cosmos/ibc-go/pull/4199) Adds optional `PacketDataProvider` interface for retrieving custom packet data stored on behalf of another application.
 * [\#4200](https://github.com/cosmos/ibc-go/pull/4200) Adds optional `PacketData` interface which application's packet data may implement.

diff --git a/modules/core/23-commitment/types/merkle.go b/modules/core/23-commitment/types/merkle.go
@@ -79,6 +79,12 @@ func NewMerklePath(keyPath ...string) MerklePath {
 // This represents the path in the same way the tendermint KeyPath will
 // represent a key path. The backslashes partition the key path into
 // the respective stores they belong to.
+//
+// Deprecated: This function makes assumptions about the way a merkle path
+// in a multistore should be represented as a string that are not standardized.
+// The decision on how to represent the merkle path as a string will be deferred
+// to upstream users of the type.
+// This function will be removed in a next release.
 func (mp MerklePath) String() string {
 	pathStr := ""
 	for _, k := range mp.KeyPath {
@@ -91,6 +97,12 @@ func (mp MerklePath) String() string {
 // This function will unescape any backslash within a particular store key.
 // This makes the keypath more human-readable while removing information
 // about the exact partitions in the key path.
+//
+// Deprecated: This function makes assumptions about the way a merkle path
+// in a multistore should be represented as a string that are not standardized.
+// The decision on how to represent the merkle path as a string will be deferred
+// to upstream users of the type.
+// This function will be removed in a next release.
 func (mp MerklePath) Pretty() string {
 	path, err := url.PathUnescape(mp.String())
 	if err != nil {
@@ -100,16 +112,11 @@ func (mp MerklePath) Pretty() string {
 }
 
 // GetKey will return a byte representation of the key
-// after URL escaping the key element
 func (mp MerklePath) GetKey(i uint64) ([]byte, error) {
 	if i >= uint64(len(mp.KeyPath)) {
 		return nil, fmt.Errorf("index out of range. %d (index) >= %d (len)", i, len(mp.KeyPath))
 	}
-	key, err := url.PathUnescape(mp.KeyPath[i])
-	if err != nil {
-		return nil, err
-	}
-	return []byte(key), nil
+	return []byte(mp.KeyPath[i]), nil
 }
 
 // Empty returns true if the path is empty

diff --git a/modules/core/exported/commitment.go b/modules/core/exported/commitment.go
@@ -28,7 +28,7 @@ type Prefix interface {
 // Path implements spec:CommitmentPath.
 // A path is the additional information provided to the verification function.
 type Path interface {
-	String() string
+	String() string // deprecated
 	Empty() bool
 }
 

diff --git a/modules/light-clients/06-solomachine/client_state.go b/modules/light-clients/06-solomachine/client_state.go
@@ -125,15 +125,21 @@ func (cs *ClientState) VerifyMembership(
 		return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", commitmenttypes.MerklePath{}, path)
 	}
 
-	if merklePath.Empty() {
-		return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "path is empty")
+	if len(merklePath.GetKeyPath()) != 2 {
+		return sdkerrors.Wrapf(host.ErrInvalidPath, "path must be of length 2: %s", merklePath.GetKeyPath())
+	}
+
+	// in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+	key, err := merklePath.GetKey(1)
+	if err != nil {
+		return sdkerrors.Wrapf(host.ErrInvalidPath, "key not found at index 1: %v", err)
 	}
 
 	signBytes := &SignBytes{
 		Sequence:    sequence,
 		Timestamp:   timestamp,
 		Diversifier: cs.ConsensusState.Diversifier,
-		Path:        []byte(merklePath.String()),
+		Path:        key,
 		Data:        value,
 	}
 
@@ -175,11 +181,21 @@ func (cs *ClientState) VerifyNonMembership(
 		return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", commitmenttypes.MerklePath{}, path)
 	}
 
+	if len(merklePath.GetKeyPath()) != 2 {
+		return sdkerrors.Wrapf(host.ErrInvalidPath, "path must be of length 2: %s", merklePath.GetKeyPath())
+	}
+
+	// in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+	key, err := merklePath.GetKey(1)
+	if err != nil {
+		return sdkerrors.Wrapf(host.ErrInvalidPath, "key not found at index 1: %v", err)
+	}
+
 	signBytes := &SignBytes{
 		Sequence:    sequence,
 		Timestamp:   timestamp,
 		Diversifier: cs.ConsensusState.Diversifier,
-		Path:        []byte(merklePath.String()),
+		Path:        key,
 		Data:        nil,
 	}
 

diff --git a/modules/light-clients/06-solomachine/client_state_test.go b/modules/light-clients/06-solomachine/client_state_test.go
@@ -176,11 +176,15 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 					suite.Require().NoError(err)
 
 					path = suite.solomachine.GetClientStatePath(counterpartyClientIdentifier)
+					merklePath, ok := path.(commitmenttypes.MerklePath)
+					suite.Require().True(ok)
+					key, err := merklePath.GetKey(1) // in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+					suite.Require().NoError(err)
 					signBytes = solomachine.SignBytes{
 						Sequence:    sm.GetHeight().GetRevisionHeight(),
 						Timestamp:   sm.Time,
 						Diversifier: sm.Diversifier,
-						Path:        []byte(path.String()),
+						Path:        key,
 						Data:        clientStateBz,
 					}
 
@@ -208,11 +212,15 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 					suite.Require().NoError(err)
 
 					path = sm.GetConsensusStatePath(counterpartyClientIdentifier, clienttypes.NewHeight(0, 1))
+					merklePath, ok := path.(commitmenttypes.MerklePath)
+					suite.Require().True(ok)
+					key, err := merklePath.GetKey(1) // in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+					suite.Require().NoError(err)
 					signBytes = solomachine.SignBytes{
 						Sequence:    sm.Sequence,
 						Timestamp:   sm.Time,
 						Diversifier: sm.Diversifier,
-						Path:        []byte(path.String()),
+						Path:        key,
 						Data:        consensusStateBz,
 					}
 
@@ -243,11 +251,15 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 					suite.Require().NoError(err)
 
 					path = sm.GetConnectionStatePath(ibctesting.FirstConnectionID)
+					merklePath, ok := path.(commitmenttypes.MerklePath)
+					suite.Require().True(ok)
+					key, err := merklePath.GetKey(1) // in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+					suite.Require().NoError(err)
 					signBytes = solomachine.SignBytes{
 						Sequence:    sm.Sequence,
 						Timestamp:   sm.Time,
 						Diversifier: sm.Diversifier,
-						Path:        []byte(path.String()),
+						Path:        key,
 						Data:        connectionEndBz,
 					}
 
@@ -279,11 +291,15 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 					suite.Require().NoError(err)
 
 					path = sm.GetChannelStatePath(ibctesting.MockPort, ibctesting.FirstChannelID)
+					merklePath, ok := path.(commitmenttypes.MerklePath)
+					suite.Require().True(ok)
+					key, err := merklePath.GetKey(1) // in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+					suite.Require().NoError(err)
 					signBytes = solomachine.SignBytes{
 						Sequence:    sm.Sequence,
 						Timestamp:   sm.Time,
 						Diversifier: sm.Diversifier,
-						Path:        []byte(path.String()),
+						Path:        key,
 						Data:        channelEndBz,
 					}
 
@@ -312,11 +328,15 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 					suite.Require().True(found)
 
 					path = sm.GetNextSequenceRecvPath(ibctesting.MockPort, ibctesting.FirstChannelID)
+					merklePath, ok := path.(commitmenttypes.MerklePath)
+					suite.Require().True(ok)
+					key, err := merklePath.GetKey(1) // in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+					suite.Require().NoError(err)
 					signBytes = solomachine.SignBytes{
 						Sequence:    sm.Sequence,
 						Timestamp:   sm.Time,
 						Diversifier: sm.Diversifier,
-						Path:        []byte(path.String()),
+						Path:        key,
 						Data:        sdk.Uint64ToBigEndian(nextSeqRecv),
 					}
 
@@ -351,11 +371,15 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 
 					commitmentBz := channeltypes.CommitPacket(suite.chainA.Codec, packet)
 					path = sm.GetPacketCommitmentPath(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
+					merklePath, ok := path.(commitmenttypes.MerklePath)
+					suite.Require().True(ok)
+					key, err := merklePath.GetKey(1) // in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+					suite.Require().NoError(err)
 					signBytes = solomachine.SignBytes{
 						Sequence:    sm.Sequence,
 						Timestamp:   sm.Time,
 						Diversifier: sm.Diversifier,
-						Path:        []byte(path.String()),
+						Path:        key,
 						Data:        commitmentBz,
 					}
 
@@ -378,11 +402,15 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 				"success: packet acknowledgement verification",
 				func() {
 					path = sm.GetPacketAcknowledgementPath(ibctesting.MockPort, ibctesting.FirstChannelID, 1)
+					merklePath, ok := path.(commitmenttypes.MerklePath)
+					suite.Require().True(ok)
+					key, err := merklePath.GetKey(1) // index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+					suite.Require().NoError(err)
 					signBytes = solomachine.SignBytes{
 						Sequence:    sm.Sequence,
 						Timestamp:   sm.Time,
 						Diversifier: sm.Diversifier,
-						Path:        []byte(path.String()),
+						Path:        key,
 						Data:        ibctesting.MockAcknowledgement,
 					}
 
@@ -405,11 +433,15 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 				"success: packet receipt verification",
 				func() {
 					path = sm.GetPacketReceiptPath(ibctesting.MockPort, ibctesting.FirstChannelID, 1)
+					merklePath, ok := path.(commitmenttypes.MerklePath)
+					suite.Require().True(ok)
+					key, err := merklePath.GetKey(1) // in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+					suite.Require().NoError(err)
 					signBytes = solomachine.SignBytes{
 						Sequence:    sm.Sequence,
 						Timestamp:   sm.Time,
 						Diversifier: sm.Diversifier,
-						Path:        []byte(path.String()),
+						Path:        key,
 						Data:        []byte{byte(1)}, // packet receipt is stored as a single byte
 					}
 
@@ -429,7 +461,7 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 				true,
 			},
 			{
-				"invalid path type",
+				"invalid path type - empty",
 				func() {
 					path = ibcmock.KeyPath{}
 				},
@@ -521,11 +553,15 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 				clientState = sm.ClientState()
 
 				path = commitmenttypes.NewMerklePath("ibc", "solomachine")
+				merklePath, ok := path.(commitmenttypes.MerklePath)
+				suite.Require().True(ok)
+				key, err := merklePath.GetKey(1) // in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+				suite.Require().NoError(err)
 				signBytes = solomachine.SignBytes{
 					Sequence:    sm.GetHeight().GetRevisionHeight(),
 					Timestamp:   sm.Time,
 					Diversifier: sm.Diversifier,
-					Path:        []byte(path.String()),
+					Path:        key,
 					Data:        []byte("solomachine"),
 				}
 
@@ -570,19 +606,21 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
 func (suite *SoloMachineTestSuite) TestSignBytesMarshalling() {
 	sm := suite.solomachine
 	merklePath := commitmenttypes.NewMerklePath("ibc", "solomachine")
+	key, err := merklePath.GetKey(1) // in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+	suite.Require().NoError(err)
 	signBytesNilData := solomachine.SignBytes{
 		Sequence:    sm.GetHeight().GetRevisionHeight(),
 		Timestamp:   sm.Time,
 		Diversifier: sm.Diversifier,
-		Path:        []byte(merklePath.String()),
+		Path:        key,
 		Data:        nil,
 	}
 
 	signBytesEmptyArray := solomachine.SignBytes{
 		Sequence:    sm.GetHeight().GetRevisionHeight(),
 		Timestamp:   sm.Time,
 		Diversifier: sm.Diversifier,
-		Path:        []byte(merklePath.String()),
+		Path:        key,
 		Data:        []byte{},
 	}
 
@@ -621,11 +659,15 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {
 				"success: packet receipt absence verification",
 				func() {
 					path = suite.solomachine.GetPacketReceiptPath(ibctesting.MockPort, ibctesting.FirstChannelID, 1)
+					merklePath, ok := path.(commitmenttypes.MerklePath)
+					suite.Require().True(ok)
+					key, err := merklePath.GetKey(1) // in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+					suite.Require().NoError(err)
 					signBytes = solomachine.SignBytes{
 						Sequence:    sm.GetHeight().GetRevisionHeight(),
 						Timestamp:   sm.Time,
 						Diversifier: sm.Diversifier,
-						Path:        []byte(path.String()),
+						Path:        key,
 						Data:        nil,
 					}
 
@@ -740,11 +782,15 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {
 				clientState = sm.ClientState()
 
 				path = commitmenttypes.NewMerklePath("ibc", "solomachine")
+				merklePath, ok := path.(commitmenttypes.MerklePath)
+				suite.Require().True(ok)
+				key, err := merklePath.GetKey(1) // in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+				suite.Require().NoError(err)
 				signBytes = solomachine.SignBytes{
 					Sequence:    sm.GetHeight().GetRevisionHeight(),
 					Timestamp:   sm.Time,
 					Diversifier: sm.Diversifier,
-					Path:        []byte(path.String()),
+					Path:        key,
 					Data:        nil,
 				}
 

diff --git a/modules/light-clients/07-tendermint/upgrade.go b/modules/light-clients/07-tendermint/upgrade.go
@@ -80,7 +80,7 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
 	// construct clientState Merkle path
 	upgradeClientPath := constructUpgradeClientMerklePath(cs.UpgradePath, lastHeight)
 	if err := merkleProofClient.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), upgradeClientPath, bz); err != nil {
-		return sdkerrors.Wrapf(err, "client state proof failed. Path: %s", upgradeClientPath.Pretty())
+		return sdkerrors.Wrapf(err, "client state proof failed. Path: %s", upgradeClientPath.GetKeyPath())
 	}
 
 	// Verify consensus state proof
@@ -91,7 +91,7 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
 	// construct consensus state Merkle path
 	upgradeConsStatePath := constructUpgradeConsStateMerklePath(cs.UpgradePath, lastHeight)
 	if err := merkleProofConsState.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), upgradeConsStatePath, bz); err != nil {
-		return sdkerrors.Wrapf(err, "consensus state proof failed. Path: %s", upgradeConsStatePath.Pretty())
+		return sdkerrors.Wrapf(err, "consensus state proof failed. Path: %s", upgradeConsStatePath.GetKeyPath())
 	}
 
 	// Construct new client state and consensus state