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

fix(statemachine)!: use path within IBC store without escaping characters in solomachine #4429

Merged
merged 5 commits into from
Aug 24, 2023
Merged

fix(statemachine)!: use path within IBC store without escaping characters in solomachine #4429

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8a59dbb
use path in IBC store without escaping characters for solo machine
Aug 22, 2023
f946251
comment
Aug 22, 2023
de738a8
Merge branch 'main' into carlos/4128-wrong-url-encoded-path-used-in-s…
Aug 24, 2023
7424f5c
remove character unescaping in GetKey
Aug 24, 2023
6a74acb
Merge branch 'main' into carlos/4128-wrong-url-encoded-path-used-in-s…
Aug 24, 2023
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
12 changes: 12 additions & 0 deletions modules/core/23-commitment/types/merkle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a deprecated notice to these functions instead of removing them completely because I think removing them is API breaking and if I remove them in this PR the backport to v7.3.x will be more difficult. I will remove them in a follow up PR once this one gets merged.

// 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 {
Expand All @@ -93,6 +99,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 {
Expand Down
2 changes: 1 addition & 1 deletion modules/core/exported/commitment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove this function, I am not sure if this interface makes much sense anymore...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

Empty() bool
}

Expand Down
24 changes: 20 additions & 4 deletions modules/light-clients/06-solomachine/client_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,15 +128,21 @@ func (cs *ClientState) VerifyMembership(
return errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected %T, got %T", commitmenttypes.MerklePath{}, path)
}

if merklePath.Empty() {
return errorsmod.Wrap(commitmenttypes.ErrInvalidProof, "path is empty")
if len(merklePath.GetKeyPath()) != 2 {
return errorsmod.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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetKey seems to do some unescaping on the string also, is this okay here?

from GetKey:

key, err := url.PathUnescape(mp.KeyPath[i])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should just use merklePath.KeyPath[1]?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this be fine considering escaping calls have been removed? It seems like it would just be best to tweak GetKey.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good observation @damiannolan!

I think it is still fine, since it is doing unescaping. And also according to ICS 24 % is not a valid character in identifiers and paths, so it should not happen that a path contains an escaped character and then GetKey would return it unescaped.

Copy link
Contributor Author

@crodriguezvega crodriguezvega Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you guys prefer to do?

  • Leave as is.
  • Use merklePath.KeyPath[1]
  • Tweak GetKey. I would prefer to do this in a separate PR where I remove the String and Pretty functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with opt 2 or 3 I think!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're having a vote I'd go towards 3.

Copy link
Contributor Author

@crodriguezvega crodriguezvega Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will go for 2 and implement 3 in the next PR. Nah, I changed my mind: I will go for 3, will make working on the follow-up PR easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I will merge when tests pass.

if err != nil {
return errorsmod.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,
}

Expand Down Expand Up @@ -178,11 +184,21 @@ func (cs *ClientState) VerifyNonMembership(
return errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected %T, got %T", commitmenttypes.MerklePath{}, path)
}

if len(merklePath.GetKeyPath()) != 2 {
return errorsmod.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 errorsmod.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,
}

Expand Down
74 changes: 60 additions & 14 deletions modules/light-clients/06-solomachine/client_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}

Expand Down Expand Up @@ -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,
}

Expand Down Expand Up @@ -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,
}

Expand Down Expand Up @@ -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,
}

Expand Down Expand Up @@ -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),
}

Expand Down Expand Up @@ -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,
}

Expand All @@ -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,
}

Expand All @@ -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
}

Expand All @@ -429,7 +461,7 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
true,
},
{
"invalid path type",
"invalid path type - empty",
func() {
path = ibcmock.KeyPath{}
},
Expand Down Expand Up @@ -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"),
}

Expand Down Expand Up @@ -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{},
}

Expand Down Expand Up @@ -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,
}

Expand Down Expand Up @@ -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,
}

Expand Down
4 changes: 2 additions & 2 deletions modules/light-clients/07-tendermint/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,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 errorsmod.Wrapf(err, "client state proof failed. Path: %s", upgradeClientPath.Pretty())
return errorsmod.Wrapf(err, "client state proof failed. Path: %s", upgradeClientPath.GetKeyPath())
}

// Verify consensus state proof
Expand All @@ -94,7 +94,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 errorsmod.Wrapf(err, "consensus state proof failed. Path: %s", upgradeConsStatePath.Pretty())
return errorsmod.Wrapf(err, "consensus state proof failed. Path: %s", upgradeConsStatePath.GetKeyPath())
}

// Construct new client state and consensus state
Expand Down
Loading