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

query connection given connection_id #130

Closed
ebuchman opened this issue Jul 7, 2020 · 13 comments · Fixed by #136
Closed

query connection given connection_id #130

ebuchman opened this issue Jul 7, 2020 · 13 comments · Fixed by #136
Assignees
Labels
I: CLI Internal: related to the relayer's CLI I: logic Internal: related to the relaying logic
Milestone

Comments

@ebuchman
Copy link
Member

ebuchman commented Jul 7, 2020

Given a connection id, query the connection.

Should be able to run this from the relayer CLI.

@ebuchman ebuchman added this to the v0.0.2 milestone Jul 7, 2020
@adizere adizere added I: CLI Internal: related to the relayer's CLI connection I: logic Internal: related to the relaying logic labels Jul 8, 2020
@ancazamfir
Copy link
Collaborator

Proto definitions for connection queries and responses (see ConnectionEnd, Counterparty, etc) are here:
https://github.com/cosmos/cosmos-sdk/tree/master/proto/ibc/connection

@andynog
Copy link
Contributor

andynog commented Jul 8, 2020

As per our discussion today, I'll create a proto folder and store the proto files in there following the same structure as the cosmos sdk proto folder. Also, per our discussion will be using prost crate to handle proto files/logic.

@liamsi
Copy link
Member

liamsi commented Jul 8, 2020

BTW, something I forgot to mention but you've probably seen already is that if you want to generate the rust types during build time, you also need a dependency on prost-build:

[build-dependencies]
prost-build = "0.6"

and a build.rs file. In tendermint-rs we never used that because we never generated the types (this might change in the near future too). Here is an example:
https://github.com/libp2p/rust-libp2p/blob/master/core/build.rs
https://github.com/libp2p/rust-libp2p/blob/master/core/src/keys.proto
https://github.com/libp2p/rust-libp2p/blob/6ad05b0ff1cb11ceec37cb191adb0d13feaceeb4/core/src/lib.rs#L38-L40

@andynog
Copy link
Contributor

andynog commented Jul 8, 2020

  • Added a new branch andy/proto
  • Implemented the initial proto files required for the connection.
  • The proto files are in the modules folder.
  • Copied proto files from cosmos-sdk and tendermint repos, but it seems they are not in sync, e.g. a cosmos proto file reference an import from tendermint that was not valid. Noticed the tendermint proto files just had a refactoring a few hours ago. Looks like this proto work on the crates are pretty much in flux for now.
  • There's a new build.rs in there that generate the files in the output folder (e.g. /target/debug/build/relayer-modules-[HASH]/ibc.connection.rs).
  • Then tried to replace the connections mod (modules/src/ics03_connection/mod.rs) with generated file as per prost_build instructions but got bunch of failures probably because there are dependencies that the original Rust connection had and it's not in the generated proto files.

Next steps are trying to make it compile again using the new proto generated file. We might need to chat about this. Seems to me that a lot of logic in the proto files are somewhat duplicate with things already implemented in the Rust code.

@ebuchman
Copy link
Member Author

ebuchman commented Jul 9, 2020

Just a note for now let's try to use the minimal amount of proto to get this working. Since this is just the ConnectionEnd we should probably copy over just the pieces we need to query just a ConnectionEnd - I don't think that should have any dependency on Tendermint. If we copy over the full dir we'll have lots of deps but let's try to simplify first to get it working.

@andynog
Copy link
Contributor

andynog commented Jul 9, 2020

Just a note for now let's try to use the minimal amount of proto to get this working. Since this is just the ConnectionEnd we should probably copy over just the pieces we need to query just a ConnectionEnd - I don't think that should have any dependency on Tendermint. If we copy over the full dir we'll have lots of deps but let's try to simplify first to get it working.

I’ve tried to use the minimal as far as I know. There might be more ways to trim possibly. The dependency on tendermint proto is from the cosmos proto, there’s an import for tendermint.crypto.Proof (which the cosmos proto had what it seems an old reference tendermint.crypto.merkle.Proof and I changed to match the new tendermint reorged protos). We should have some discussions on how we want to handle proto files on our side. Thought the idea of using proto in the end was that all implementations used the same interface to prevent mismatches and deserialization issues. But if we’re going to implement our own protos then we can structure for now in a more simplistic way.

@adizere
Copy link
Member

adizere commented Jul 9, 2020

Regarding the dependency on the Go proto definitions & compiling them:

It seems that the problem with using the Go-based .proto files is that there are some Go-specific declarations in there. The gogo extension is problematic:

message ConnectionEnd {
  option (gogoproto.goproto_getters) = false;
  // connection identifier.
  string id = 1 [(gogoproto.customname) = "ID", (gogoproto.moretags) = "yaml:\"id\""];
  // client associated with this connection.
  string client_id = 2 [(gogoproto.customname) = "ClientID", (gogoproto.moretags) = "yaml:\"client_id\""];
  // opaque string which can be utilised to determine encodings or protocols for
  // channels or packets utilising this connection
  repeated string versions = 3;
  // current state of the connection end.
  State state = 4;
  // counterparty chain associated with this connection.
  Counterparty counterparty = 5 [(gogoproto.nullable) = false];
}

Trying to trim out the gogo part and see where that leads.

@adizere
Copy link
Member

adizere commented Jul 9, 2020

While writing the above ^ comment I remembered that there are more "canonical" proto definitions here:

https://github.com/cosmos/ics/blob/master/spec/ics-003-connection-semantics/data-structures.proto

These are slightly different, however, than the Go proto. Which makes me wonder why are they different...

@liamsi
Copy link
Member

liamsi commented Jul 9, 2020

It seems that the problem with using the Go-based .proto files is that there are some Go-specific declarations in there. The gogo extension is problematic:

Out of curiosity do they err if build with prost? It would be good to give that as feedback to the tendermint/sdk team.

@andynog
Copy link
Contributor

andynog commented Jul 9, 2020

  • Implemented logic that the response can be decode using prost.
  • Error handling is not the best for now, just added something so it could compile need to understand how to do it properly
  • There are a few classes that have todo!() that we might need to implement later now that we are deserializing things (e.g. CommitmentPath)

If you want to run this

This is the output for the command above. Need someone (maybe @ancazamfir) to validate if it makes sense

Options QueryConnectionOptions { connection_id: ConnectionId("connectionidone"), height: 0, proof: true }
connection query result:  IdentifiedConnectionEnd { connection_end: ConnectionEnd { state: Uninitialized, client_id: ClientId("clientidone"), counterparty: Counterparty { client_id: ClientId("clientidone"), connection_id: ConnectionId("connectionidone"), prefix: CommitmentPrefix }, versions: ["(1,[ORDER_ORDERED,ORDER_UNORDERED])"] }, connection_id: ConnectionId("connectionidone") }

and here's the output from the curl command to simd data

curl 'localhost:26657/abci_query?path="store/ibc/key"&data="connections/connectionidone"'
{
  "jsonrpc": "2.0",
  "id": -1,
  "result": {
    "response": {
      "code": 0,
      "log": "",
      "info": "",
      "index": "0",
      "key": "Y29ubmVjdGlvbnMvY29ubmVjdGlvbmlkb25l",
      "value": "Cg9jb25uZWN0aW9uaWRvbmUSC2NsaWVudGlkb25lGiMoMSxbT1JERVJfT1JERVJFRCxPUkRFUl9VTk9SREVSRURdKSABKigKC2NsaWVudGlkdHdvEg9jb25uZWN0aW9uaWR0d28aCAoGcHJlZml4",
      "proof": null,
      "height": "6453",
      "codespace": ""
    }
  }

the key above Base64 decoded is:

connections/connectionidone

the value above Base64 decoded is:

�connectionidone��clientidone�#(1,[ORDER_ORDERED,ORDER_UNORDERED]) �*(
�clientidtwo��connectionidtwo��
�prefix

@adizere
Copy link
Member

adizere commented Jul 10, 2020

Out of curiosity do they err if build with prost? It would be good to give that as feedback to the tendermint/sdk team.

No, they don't error, surprisingly. But note we had to manually copy/paste the gogo.proto from github into our repo..

It seems that only the prefix needs fixing. And we need to also define pub struct CommitmentPrefix which is empty. I'm now overhauling the code and will push an update soon with a simplified .proto versions. @andynog please let me keep the token on your branch for a bit longer.

@ancazamfir
Copy link
Collaborator

This is the output for the command above. Need someone (maybe @ancazamfir) to validate if it makes sense

Looks good! I made some comments in the PR wrt error handling. The command takes some other parameters, -h <height> for example queries at the specified height. I had simd running for longer and because of pruning the query at low heights fails with pages of traces. Same with non-existing connection.

andynog added a commit that referenced this issue Jul 13, 2020
… connection. Doesn't throw exception anymore. Show error message. #130
@andynog
Copy link
Contributor

andynog commented Jul 13, 2020

Worked on the issue that an exception was thrown if height parameter was specified. Now it shows an error message (but not sure if the error makes sense, need @ancazamfir to validate message)

For example specifying height 4:

./target/debug/relayer -c ./relayer/relay/tests/config/fixtures/relayer_conf_example.toml query connection end chain_A connectionidone -h 4
     Options QueryConnectionOptions { connection_id: ConnectionId("connectionidone"), height: 4, proof: true }
connection query error RPC error: proof is unexpectedly empty; ensure height has not been pruned: invalid request

Also noticed something strange, if you specify proof false (-p false) and a height (e.g. -h 4), the error message returned is about a non-existing connection

 ./target/debug/relayer -c ./relayer/relay/tests/config/fixtures/relayer_conf_example.toml query connection end chain_A connectionidone -p false -h 4
     Options QueryConnectionOptions { connection_id: ConnectionId("connectionidone"), height: 4, proof: false }
connection query error Could not parse/unmarshall response: queried for a non-existing connection

But if you omit the height the query works:

./target/debug/relayer -c ./relayer/relay/tests/config/fixtures/relayer_conf_example.toml query connection end chain_A connectionidone -p false 
     Options QueryConnectionOptions { connection_id: ConnectionId("connectionidone"), height: 0, proof: false }
connection query result:  IdentifiedConnectionEnd { connection_end: ConnectionEnd { state: Init, client_id: ClientId("clientidone"), counterparty: Counterparty { client_id: ClientId("clientidtwo"), connection_id: ConnectionId("connectionidtwo"), prefix: CommitmentPrefix([112, 114, 101, 102, 105, 120]) }, versions: ["(1,[ORDER_ORDERED,ORDER_UNORDERED])"] }, connection_id: ConnectionId("connectionidone") }

So not sure what's going on, maybe some logic error?

greg-szabo pushed a commit that referenced this issue Jul 14, 2020
* Initial implementation of proto files required for connection. Proto files generated but replacing connection mod breaks the build

* Removed reference to MerklePrefix

* Implemented logic to proto_unmarshall the connection using prost #130

* Trimmed the proto file, fixed commitments. Ready for review. (#130)

* Fixed a couple of nits

* Fix for unused_qualifications h/t @gregszabo @andynog.

* Fixing unit testing since CommitPrefix was changed, failed to cargo build #136

* Replaced prost compilation with dependency on ibc-proto crate.

* Better error propagation in connection query.

* Fixed error handling when specifying height (-h) parameter in a query connection. Doesn't throw exception anymore. Show error message. #130

* Added validation for versions in ConnectionEnd unmarshalling

* Implemented fmt::Debug trait for CommitPrefix to properly display its value (string instead of vector of bytes). Assumes value is valid utf-8. #136

Co-authored-by: Andy Nogueira <me@andynogueira.dev>
@greg-szabo greg-szabo mentioned this issue Jul 30, 2020
6 tasks
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this issue Sep 13, 2022
* Initial implementation of proto files required for connection. Proto files generated but replacing connection mod breaks the build

* Removed reference to MerklePrefix

* Implemented logic to proto_unmarshall the connection using prost informalsystems#130

* Trimmed the proto file, fixed commitments. Ready for review. (informalsystems#130)

* Fixed a couple of nits

* Fix for unused_qualifications h/t @gregszabo @andynog.

* Fixing unit testing since CommitPrefix was changed, failed to cargo build informalsystems#136

* Replaced prost compilation with dependency on ibc-proto crate.

* Better error propagation in connection query.

* Fixed error handling when specifying height (-h) parameter in a query connection. Doesn't throw exception anymore. Show error message. informalsystems#130

* Added validation for versions in ConnectionEnd unmarshalling

* Implemented fmt::Debug trait for CommitPrefix to properly display its value (string instead of vector of bytes). Assumes value is valid utf-8. informalsystems#136

Co-authored-by: Andy Nogueira <me@andynogueira.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: CLI Internal: related to the relayer's CLI I: logic Internal: related to the relaying logic
Projects
None yet
5 participants