Add client queries to the relayer #38

ancazamfir · 2020-03-17T18:08:31Z

Closes: #37

Description

Add consensus state and full state query clis to the relayer:

command to query full state of client
command to query consensus state of client
validate parameters against loaded configuration file
move to tm v0.33 branch that fixes the new merkle proof format in the abci response

Note: full deserialization does not yet work, waiting for migration of SDK/ TM to protobuf.

ancazamfir · 2020-03-17T18:10:35Z

More work needs to be done here but at this point trying to figure out why we get error when we ask for proof vs. with no proof. Will add more soon.

codecov-io · 2020-03-17T18:16:43Z

Codecov Report

❗ No coverage uploaded for pull request base (master@16ab364). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             master    #38   +/-   ##
=======================================
  Coverage          ?   7.9%           
=======================================
  Files             ?     42           
  Lines             ?   1299           
  Branches          ?    174           
=======================================
  Hits              ?    103           
  Misses            ?   1163           
  Partials          ?     33

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16ab364...b5bc847. Read the comment docs.

liamsi · 2020-04-03T12:06:21Z

modules/src/ics07_tendermint/client_state.rs

+#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
+pub struct ClientState {
+    id: String,
+    trusting_period: Duration,


Note (mostly to myself):

in the go code, time.Duration actually is this type alias:

// A Duration represents the elapsed time between two instants // as an int64 nanosecond count. The representation limits the // largest representable duration to approximately 290 years. type Duration int64

while rust's std::time::Duration actually is

/// A `Duration` type to represent a span of time, typically used for system /// timeouts. /// /// Each `Duration` is composed of a whole number of seconds and a fractional part /// represented in nanoseconds. If the underlying system does not support /// nanosecond-level precision, APIs binding a system timeout will typically round up /// the number of nanoseconds. /// [...] pub struct Duration { secs: u64, nanos: u32, // Always 0 <= nanos < NANOS_PER_SEC }

Same for the unbonding_period field below.

So for deserialization (independent from amino or not) we need to translate between the two.

ref #45

liamsi · 2020-04-06T11:30:34Z

modules/src/ics07_tendermint/client_state.rs

+    id: String,
+    trusting_period: Duration,
+    unbonding_period: Duration,
+    latest_header: Header,


Note that the Header here contains a SignedHeader which in turn contains a Commit. Looking at the Commit struct in the iqlusion/relayer (on master) shows that they are already using tendermint v0.33.2 which modified the Commit struct to:

type Commit struct { // NOTE: The signatures are in order of address to preserve the bonded // ValidatorSet order. // Any peer with a block can gossip signatures by index with a peer without // recalculating the active ValidatorSet. Height int64 `json:"height"` Round int `json:"round"` BlockID BlockID `json:"block_id"` Signatures []CommitSig `json:"signatures"` // ... }

while this branch uses the v0.32 version of the Commit:

pub struct Commit { /// Block ID of the last commit pub block_id: block::Id, /// Precommits pub precommits: Precommits, }

Or in golang (from v0.32.x): https://github.com/tendermint/tendermint/blob/64e61365c16f74d56ccaa9eb784e05df5b01764c/types/block.go#L489-L494

v0.33 tendermint-rs is currently worked on in informalsystems/tendermint-rs#196

Does it make sense to assume we want to decode v0.33 commits directly (for the sake of #45)? @ancazamfir

yes, let's look at 0.33! thx!

…ormalsystems/ibc-rs into anca/add_consensus_query_cmd

liamsi

Left a few comments. I did not review the cli in detail but everything else. LGTM

liamsi · 2020-04-09T12:26:43Z

modules/src/ics02_client/state.rs

+    fn verify_client_consensus_state(
+        &self,
+        root: &CommitmentRoot,
+    ) -> Result<(), Self::ValidationError>;


I guess the other state verification functions are not included on purpose?
https://github.com/cosmos/cosmos-sdk/blob/b5a658729170e577b0e6e809e73d5e0756b90e07/x/ibc/02-client/exported/exported.go#L26-L92

Yes, a bit more work to get there :)

liamsi · 2020-04-09T12:29:09Z

modules/src/ics07_tendermint/client_state.rs

+    trusting_period: Duration,
+    unbonding_period: Duration,
+    latest_header: Header,
+    frozen_height: crate::Height,


The order in the go-code is different (latest header should be last):

Suggested change

frozen_height: crate::Height,

frozen_height: crate::Height,

latest_header: Header,

Oy! good catch! thx, just changed. The problem is that we cannot test properly with SDK since we don't deserialize yet. Once we move to protobuf and we do the decoding we will catch more of these.

liamsi · 2020-04-09T12:29:53Z

modules/src/ics07_tendermint/client_state.rs

+    id: String,
+    trusting_period: Duration,
+    unbonding_period: Duration,
+    latest_header: Header,


Only commit batched together with the suggestion below (frozen_height):

Suggested change

latest_header: Header,

liamsi · 2020-04-09T12:32:16Z

modules/src/ics23_commitment/mod.rs

 impl CommitmentProof {
    pub fn from_bytes(_bytes: &[u8]) -> Self {
        todo!()
    }
 }
+*/


Shouldn't we delete this but but extend the abci::Proof type instead?

Yes, but will do in a separate issue/PR

liamsi · 2020-04-09T12:38:53Z

relayer/cli/src/commands/utils.rs

+        .build()
+        .unwrap()
+        .block_on(future)
+}


You can probably get rid of this block_on method (might be relevant: informalsystems/tendermint-rs#171, informalsystems/tendermint-rs#173, informalsystems/tendermint-rs#125 (comment)).

Unfortunately, I have been unable to get abscissa_tokio to work, abscissa_tokio::run just hangs (on my machine at least).

I haven't really put any time into debugging this, as I'd rather wait until we figure the new light client + relayer architecture, before spending time on that (block_on does the job just fine for now imho as it is called at the top level only).

I haven't really put any time into debugging this, as I'd rather wait until we figure the new light client + relayer architecture, before spending time on that (block_on does the job just fine for now imho as it is called at the top level only).

Makes sense!

@romac just a note that we use this now for the query also.

liamsi · 2020-04-09T12:41:23Z

relayer/relay/src/query/client_consensus_state.rs

@@ -67,17 +74,15 @@ impl<CS> ConsensusStateResponse<CS> {
    pub fn new(
        client_id: ClientId,
        consensus_state: CS,
-        abci_proof: abci::Proof,
+        abci_proof: Option<abci::Proof>,


Why isn't the type alias (CommitmentProof) used consistently?

my bad, changed.

liamsi · 2020-04-09T12:50:02Z

relayer/relay/src/query/client_state.rs

+use crate::chain::Chain;
+use crate::error;
+
+pub struct QueryClientFullState<CLS> {


Is that the corresponding request to ClientStateResponse?

yes, changed the name of response.

liamsi · 2020-04-09T12:50:48Z

relayer/relay/src/query/client_state.rs

+}
+
+fn amino_unmarshal_binary_length_prefixed<T>(_bytes: &[u8]) -> Result<T, error::Error> {
+    todo!()


liamsi

I think it is reasonable to collect outstanding comments in a followup issue and merge this as it is. LGTM 👍

liamsi

👍

* debugging the consensus query with proof * started on client state * fixed unused warning * add height param to client query * cargo fmt * make proof option for client state * fix client state prove flag * cleanup queries * Try integration with tm-rs v0.33 to pick the new merkle proof * add validation function for common params * add utils * some cleanup and err handling * addressing Ismail's comments

debugging the consensus query with proof

efbb695

ancazamfir requested a review from romac as a code owner March 17, 2020 18:08

ancazamfir added 7 commits March 24, 2020 16:20

merge

7de4896

started on client state

b528dc6

fixed unused warning

b783baa

add height param to client query

3f4afdb

cargo fmt

9b9917e

make proof option for client state

1558935

fix client state prove flag

b603ccc

romac mentioned this pull request Mar 30, 2020

ABCI Merkle Proof struct does not match Go counterpart informalsystems/tendermint-rs#198

Closed

Merge branch 'master' into anca/add_consensus_query_cmd

7da6712

liamsi reviewed Apr 3, 2020

View reviewed changes

liamsi reviewed Apr 6, 2020

View reviewed changes

ancazamfir added 5 commits April 7, 2020 16:14

cleanup queries

ab168d8

Merge branch 'anca/add_consensus_query_cmd' of https://github.com/inf…

5d5d988

…ormalsystems/ibc-rs into anca/add_consensus_query_cmd

Try integration with tm-rs v0.33 to pick the new merkle proof

46f70ce

add validation function for common params

a9b9b53

add utils

b1dd6ee

ancazamfir changed the title ~~[WIP] Add consensus state query cli to the relayer~~ [WIP] Add client queries to the relayer Apr 9, 2020

ancazamfir changed the title ~~[WIP] Add client queries to the relayer~~ Add client queries to the relayer Apr 9, 2020

ancazamfir added this to the 0.3-3mo milestone Apr 9, 2020

ancazamfir self-assigned this Apr 9, 2020

ancazamfir added I: CLI Internal: related to the relayer's CLI I: logic Internal: related to the relaying logic labels Apr 9, 2020

liamsi reviewed Apr 9, 2020

View reviewed changes

liamsi approved these changes Apr 9, 2020

View reviewed changes

ancazamfir added 2 commits April 9, 2020 19:33

some cleanup and err handling

b5bc847

addressing Ismail's comments

b68bff6

liamsi approved these changes Apr 9, 2020

View reviewed changes

ancazamfir merged commit 287bd64 into master Apr 10, 2020

ancazamfir deleted the anca/add_consensus_query_cmd branch April 16, 2020 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add client queries to the relayer #38

Add client queries to the relayer #38

ancazamfir commented Mar 17, 2020 •

edited

Loading

ancazamfir commented Mar 17, 2020

codecov-io commented Mar 17, 2020 •

edited

Loading

liamsi Apr 3, 2020 •

edited

Loading

liamsi Apr 6, 2020 •

edited

Loading

ancazamfir Apr 6, 2020

liamsi left a comment

liamsi Apr 9, 2020

ancazamfir Apr 9, 2020

liamsi Apr 9, 2020

ancazamfir Apr 9, 2020

liamsi Apr 9, 2020

ancazamfir Apr 9, 2020

liamsi Apr 9, 2020

ancazamfir Apr 9, 2020

liamsi Apr 9, 2020

romac Apr 9, 2020

liamsi Apr 9, 2020 •

edited

Loading

ancazamfir Apr 9, 2020

liamsi Apr 9, 2020

ancazamfir Apr 9, 2020

liamsi Apr 9, 2020

ancazamfir Apr 9, 2020

liamsi Apr 9, 2020

liamsi left a comment

liamsi left a comment

	frozen_height: crate::Height,
	frozen_height: crate::Height,
	latest_header: Header,

Add client queries to the relayer #38

Add client queries to the relayer #38

Conversation

ancazamfir commented Mar 17, 2020 • edited Loading

Description

ancazamfir commented Mar 17, 2020

codecov-io commented Mar 17, 2020 • edited Loading

Codecov Report

liamsi Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

liamsi Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

ancazamfir commented Mar 17, 2020 •

edited

Loading

codecov-io commented Mar 17, 2020 •

edited

Loading

liamsi Apr 3, 2020 •

edited

Loading

liamsi Apr 6, 2020 •

edited

Loading

liamsi Apr 9, 2020 •

edited

Loading