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.

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

Wrong URL encoded path used in solomachine client. #4128

Closed
3 tasks
en opened this issue Jul 19, 2023 · 7 comments · Fixed by #4429 or #4459
Closed
3 tasks

Wrong URL encoded path used in solomachine client. #4128

en opened this issue Jul 19, 2023 · 7 comments · Fixed by #4429 or #4459
Assignees
Milestone

Comments

@en
Copy link

en commented Jul 19, 2023

Summary of Bug

Path: []byte(merklePath.String()),

The String() method returns a URL encoded path (like this: "/path%2Fto%2Fleaf"), which is not consistent with spec and other implementations (e.g. ibc-rs)

Expected Behaviour

Should use path.Pretty() here.

Version

Steps to Reproduce


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@en en changed the title Wrong URL encoded path uses in solomachine client. Wrong URL encoded path used in solomachine client. Jul 20, 2023
@crodriguezvega
Copy link
Contributor

Hi @en. Thanks for opening the issue. I just have a couple of questions:

  • which is not consistent with spec: do you mean with ICS 06? I briefly checked ICS 06 and also ICS 23 and I cannot find mention of the expected string formatting of a CommimentPath.
  • and other implementations (e.g. ibc-rs): As far as I know ibc-rs doesn't have an implementation of ICS 06? Or you mean the solo machine implemented here? The paths used in stag are the standard paths specified in ICS 04, so there shouldn't be any spaces in the them. Those are the paths that ibc-go construct when verifying that the solo machine has commitment the data.
  • To understand better your request: why would you need to have a path with spaces or other "unsafe" characters?

@en
Copy link
Author

en commented Jul 20, 2023

Hi @crodriguezvega,

I mean the paths specified in ICS 04.
These paths are used as storage keys in ICS 07, and as part of the signed data in ICS 06. (

signBytes := &SignBytes{
Sequence: sequence,
Timestamp: timestamp,
Diversifier: cs.ConsensusState.Diversifier,
Path: []byte(merklePath.String()),
Data: value,
)

We(Octopus Network) are implementing ICS 06 based on ibc-rs. These paths have to be consistent on both sides so they can be validated by ibc-go. Although there are some definitions of paths in ibc-rs( https://github.com/cosmos/ibc-rs/blob/v0.28.0/crates/ibc/src/core/ics24_host/path.rs#L49-L54 ), we construct them manually into the escaped form(/path%2Fto%2Fleaf) to pass the validation.

So my question is:
Is ibc-go intentionally using the escaped form paths? Or ibc-go used the wrong path.

@DimitrisJim
Copy link
Contributor

DimitrisJim commented Jul 20, 2023

I don't think spec specifies this? Either way, I wasn't around for solomachine to hopefully remember something. It might be worth noting that paths appear to be unescaped in the tendermint light client tho.

@colin-axner
Copy link
Contributor

Hi @en, thanks for raising the issue! This is certainly behaviour that slipped our attention.

The side-effect of merklePath.String() is quite confusing. It turns out that the context of this behaviour is that the MerklePath type is meant to hold keys for a multitree structure (multistore). Each key is a key in a different tree and thus needs to be separated in some way in the String() print out (as each key will likely have /). The cometbft implementation separates the keys in this multi merkle path list by using the escaped paths.

The switch to including the merkle path in the sign bytes occurred in v7. It appears at the moment no solo machine users are on v7, so we could retroactively fix the issue.

My recommendation to the team would be to:

  • fix solo machine to use the unescaped ics24 path (this would not include the connection prefix)
  • remove merklePath.String() function
  • remove merklePath.Pretty() function
  • update documentation on the merklePath merkle keys documentation to indicate this expected structure

The solo machine does not have a multi tree structure. It should only signs over the ics24 paths expected. The connection prefix is unnecessary as solo machine clients also have a diversifer they sign over (which should logically separate a solo machines private key usage between chains/connections)

Would this match your implementation @en?

@crodriguezvega
Copy link
Contributor

crodriguezvega commented Aug 24, 2023

@en we have opened this PR with this change from @colin-axner's proposal above:

  • fix solo machine to use the unescaped ics24 path (this would not include the connection prefix)

We are planning to release this change in v7.3.0 (target release date: 1st September).

The other items will be tackled in a follow-up PR.

@en
Copy link
Author

en commented Sep 15, 2023

Sorry for the late reply.
I just checked it. With the multistore key removed, our program is also simplified a lot.
Thanks!

@crodriguezvega
Copy link
Contributor

Sorry for the late reply.
I just checked it. With the multistore key removed, our program is also simplified a lot.
Thanks!

Great to hear. Thanks for coming back to us!

mergify bot pushed a commit that referenced this issue Sep 26, 2023
…rklePath` (#4459)

## Description



Second part of #4128. First part was #4429.

closes: #4128 

### Commit Message / Changelog Entry

```bash
fix(api)!: remove `Pretty` and `String` custom implementations of `MerklePath`
```

see the [guidelines](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) for commit messages. (view raw markdown for examples)




---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#pull-request-targeting)).
- [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/11-structure.md) and [Go style guide](../docs/dev/go-style-guide.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/main/testing/README.md#ibc-testing-package).
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`).
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Provide a [commit message](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) to be used for the changelog entry in the PR description for review.
- [x] Re-reviewed `Files changed` in the Github PR explorer.
- [ ] Review `Codecov Report` in the comment section below once CI passes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment