Refactoring and improve update client semantics #1011

soareschen · 2021-05-28T11:12:03Z

Closes: #997

Description

For contributor use:

Updated the Unreleased section of CHANGELOG.md with the issue.
If applicable: Unit tests written, added test to CI.
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Updated relevant documentation (docs/) and code comments.
Re-reviewed Files changed in the Github PR explorer.

adizere

I like your proposal for the NonZeroHeight type, which could help us remove the clumsy if h == Height::zero() checks around the codebase. It would be good if you can briefly argue for the rationale behind this new type in the PR description.

I know it's still a draft, so you're probably fixing these problems already, but please mind:

most important: fix for issue Hermes client update fails if client was recently created #997 which is driving this refactor (I updated the acceptance criteria in the issue)
clippy
failing tests, probably because of the removed code in particular in ForeignClient

modules/src/ics02_client/height.rs

relayer/src/chain/cosmos.rs

adizere · 2021-05-28T14:39:40Z

relayer-cli/src/commands/tx/client.rs

+            .trusted_height
+            .and_then(|h| NonZeroHeight::new(ibc::Height::new(src_chain_id, h)));
+
+        let target_height = match m_target_height {


These checks make the command quite intelligent, which is nice. But it might be better if the user is allowed to exercise the corner-cases of update client CLIENT-ID --target-height 0 for example, especially for tx raw. To do that, it could be a good idea to leave TxUpdateClientCmd untouched (modulo the nice refactor with execute) and create a new command e.g., UpdateClientCmd that is similar to TxUpdateClientCmd but which has more checks in place to cover corner-cases.

Having a new UpdateClientCmd command will also address (part of) #773 .

I have added a NonZeroHeight::unsafe_new() function for creation of heights that are really meant to be zero. Btw note that previously even if zero height is allowed in the command line, it still really means the absence of height and the relayer will still fetch the current height.

I have now added the UpdateClientCmd and have the two behaviors slightly different. In UpdateClientCmd, only the target height can optionally be provided, and the command will wait for the target height to reach before updating. For TxUpdateClientCmd, both trusted height and target height must be provided, and there is no sanity checking in the command. So if invalid heights are provided, they will go into the implementation code and return error from there.

For TxUpdateClientCmd, both trusted height and target height must be provided, and there is no sanity checking in the command. So if invalid heights are provided, they will go into the implementation code and return error from there.

What is the rationale for this modification? It's probably better to keep this API unchanged and leave it simple so that we can invoke these commands with minimal effort.

This is just my suggestion, because otherwise I am not really sure what is the point of having two commands with separate implementations if their behavior are the same. Or do you have any recommendation of how the two commands should be differentiated?

relayer/src/foreign_client.rs

soareschen · 2021-06-01T12:29:18Z

The PR is now ready for review. Here is a more reliable way to reproduce #997:

Modify the timeout_commit and timeout_propose fields in the one-chain script to a longer duration, such as 10s.
Recreate the chains using ./scripts/dev-env.
Run commands such as./target/debug/hermes create client ibc-0 ibc-1 ; ./target/debug/hermes update client ibc-0 07-tendermint-{N}, with {N} replaced with expected new client ID.
The command should fail on master, but pass on this PR.

Draft refactoring of client update Fix lint More refactoring Fixing errors

adizere

Confirm that the fix for #997 works, thanks Soares!

> ./target/debug/hermes create client ibc-0 ibc-1 ; ./target/debug/hermes update client ibc-0 07-tendermint-6
Success: CreateClient(
    CreateClient(
        Attributes {
            height: Height {
                revision: 0,
                height: 522,
            },
            client_id: ClientId(
                "07-tendermint-6",
            ),
            client_type: Tendermint,
            consensus_height: Height {
                revision: 1,
                height: 521,
            },
        },
    ),
)
Jun 02 12:46:29.257  INFO ibc_relayer_cli::commands: using default configuration from '/Users/adi/.hermes/config.toml'
Jun 02 12:46:29.291  WARN ibc_relayer::foreign_client: [ibc-1 -> ibc-0:07-tendermint-6] skipping update: trusted height (1-521) >= chain target height (1-521)
Jun 02 12:46:29.291  INFO ibc_relayer::foreign_client: Client 07-tendermint-6 is already up-to-date with chain ibc-1@1-521
Success: None

The PR is difficult to review, however. There's multiple changes (in particular around the ForeignClient::build* methods) and could not understand their impact fully. The difficulty is mostly due to the complex ways in which these methods can be called and used (not necessarily because of this PR). So I'm wondering if it is possible to break the PR in two parts: one that strictly (and minimally) fixes 997, and another part with your refactoring.

adizere · 2021-06-02T09:58:47Z

relayer-cli/src/commands/tx/client.rs

-    #[options(help = "the trusted height of the client update", short = "t")]
-    trusted_height: Option<u64>,
+    #[options(
+        required,


I think this should stay unchanged, not clear why we need it to be required.

adizere · 2021-06-02T10:00:52Z

relayer-cli/src/commands/update.rs

+    dst_client_id: ClientId,
+
+    #[options(help = "the target height of the client update", short = "h")]
+    target_height: Option<u64>,


Just FYI: we may need to update the guide (this section) with this change.

definitely needs guide update. Also, if we want to change this as compared to tx raw we should discuss to understand why and how. At first look I would propose hermes client update <chain-id> <cliend-id> (i.e. drop the target_height)

adizere · 2021-06-02T10:16:35Z

relayer/src/foreign_client.rs

-        // If not specified, set trusted state to the highest height smaller than target height.
-        // Otherwise ensure that a consensus state at trusted height exists on-chain.
-        let cs_heights = self.consensus_state_heights()?;
-        let trusted_height = if trusted_height == Height::zero() {


Since we don't have this check in place anymore, this means that any caller of build_update_client_with_trusted has to make sure that the client has a consensus state at trusted_height installed. Do I understand correctly?

Correct. In pretty much all places, the trusted height is already obtained from the consensus state height, so the check is unnecessary. With the new update client command, I make it such that the trusted height is also automatically fetchd, so there is no need for checking. The only place left unchecked is in the tx raw update-client command, but since it is a raw command I think it is good to bypass checking so that we can test the behavior on the actual chain.

adizere · 2021-06-02T10:18:32Z

relayer/src/foreign_client.rs

@@ -540,9 +546,7 @@ impl ForeignClient {
            ))
        })?;

-        assert!(!events.is_empty());
-
-        Ok(events.pop().unwrap())


Nice refactor around this method.

ancazamfir · 2021-06-02T10:40:18Z

relayer-cli/src/commands/update.rs

+    dst_client_id: ClientId,
+
+    #[options(help = "the target height of the client update", short = "h")]
+    target_height: Option<u64>,


definitely needs guide update. Also, if we want to change this as compared to tx raw we should discuss to understand why and how. At first look I would propose hermes client update <chain-id> <cliend-id> (i.e. drop the target_height)

ancazamfir · 2021-06-02T10:46:32Z

Running locally e2e/run.py i get:

2021-06-02 12:37:20 [   DEBUG] {"result":null,"status":"success"}
Traceback (most recent call last):
  File "/Users/ancaz/rust/ibc-rs/./e2e/run.py", line 229, in <module>
    main()
  File "/Users/ancaz/rust/ibc-rs/./e2e/run.py", line 213, in main
    ibc0_client_id, ibc0_conn_id, ibc0_chan_id, ibc1_client_id, ibc1_conn_id, ibc1_chan_id = raw(config, ibc0 , ibc1, port_id)
  File "/Users/ancaz/rust/ibc-rs/./e2e/run.py", line 130, in raw
    ibc0_client_id = client.create_update_query_client(c, ibc0, ibc1)
  File "/Users/ancaz/rust/ibc-rs/e2e/e2e/client.py", line 128, in create_update_query_client
    update_client(c, dst, client.client_id)
  File "/Users/ancaz/rust/ibc-rs/e2e/e2e/client.py", line 111, in update_client
    res = cmd.run(c).success()
  File "/Users/ancaz/rust/ibc-rs/e2e/e2e/cmd.py", line 34, in success
    data = self.cmd.process(result)
  File "/Users/ancaz/rust/ibc-rs/e2e/e2e/client.py", line 49, in process
    return from_dict(ClientUpdated, result['UpdateClient']['common'])
KeyError: 'UpdateClient'

probably because the return value is None and update client event cannot be decoded.

(one-chain script changed with 10 seconds timeouts)

soareschen · 2021-06-02T10:58:44Z

probably because the return value is None and update client event cannot be decoded.

Hmm makes sense. If the trusted height is already at target height, then None is returned, so the e2e script will need to handle that. I will fix the e2e script then.

soareschen · 2021-06-02T11:08:01Z

So I'm wondering if it is possible to break the PR in two parts: one that strictly (and minimally) fixes 997, and another part with your refactoring.

I suppose if we want to fix #997 naively, we can just replace the comparator operator in the original build_update_client_and_send from < to <=:

diff --git a/relayer/src/foreign_client.rs b/relayer/src/foreign_client.rs
index dad8993d..1385fe4b 100644
--- a/relayer/src/foreign_client.rs
+++ b/relayer/src/foreign_client.rs
@@ -439,7 +439,7 @@ impl ForeignClient {
             // Get highest height smaller than target height
             cs_heights
                 .into_iter()
-                .find(|h| h < &target_height)
+                .find(|h| h <= &target_height)
                 .ok_or_else(|| {
                     ForeignClientError::ClientUpdate(format!(
                         "chain {} is missing trusted state smaller than target height {}",

I agree that the original code is pretty difficult to understand. And at least without familiarizing myself with what the code actually does, I wouldn't be able to be certain that the fix really solves the issue. Right now with the correct understanding after the refactoring, I think the simple fix could work.

The refactored code should preserve the original semantics while keeping it simpler. But since the original code is so complex, it certainly requires some reading to make sure that it really doesn't break anything.

soareschen · 2021-06-02T11:22:23Z

I suppose if we want to fix #997 naively, we can just replace the comparator operator in the original build_update_client_and_send from < to <=:

Hmm maybe it's not that straightforward. Somewhere upstream the original code would still convert empty result to error.

Jun 02 11:19:29.047  WARN ibc_relayer::foreign_client: [ibc-1 -> ibc-0:07-tendermint-3] skipping update: trusted height (1-14) >= chain target height (1-14)
Error: tx error: error raised while updating client: Client 07-tendermint-3 is already up-to-date with chain ibc-1@1-14

romac · 2021-06-02T11:45:48Z

So I'm wondering if it is possible to break the PR in two parts: one that strictly (and minimally) fixes 997, and another part with your refactoring.

That would be great, and would perhaps allow us to merge the fix for 997 before the release and take some time to properly review the refactoring afterwards.

soareschen · 2021-06-02T12:41:53Z

I have opened #1036 for the quick fix.

adizere · 2021-06-09T09:46:41Z

This work will be subsided by later work that will replace Height::zero with Option<Height>.

Heights can have different meanings:

some specific height
none specified, which can mean an error (because a specific height is necessary)
none specified, which can mean the latest one

@soareschen:

Create a discussion to capture this refactor, describe the rationale.

adizere requested changes May 28, 2021

View reviewed changes

soareschen force-pushed the soares/update-client-refactoring branch from 86df2fa to 02c3500 Compare June 1, 2021 08:33

soareschen marked this pull request as ready for review June 1, 2021 12:26

soareschen requested review from ancazamfir and romac as code owners June 1, 2021 12:26

soareschen added 4 commits June 1, 2021 15:19

Fixing update client

83ebf12

Draft refactoring of client update Fix lint More refactoring Fixing errors

Tests are now passing

0daaa50

Separate out update client and raw tx update commands

8ad4a76

Fix e2e tests

4d4cb52

soareschen force-pushed the soares/update-client-refactoring branch from d15c806 to 4d4cb52 Compare June 1, 2021 13:19

adizere reviewed Jun 2, 2021

View reviewed changes

ancazamfir requested changes Jun 2, 2021

View reviewed changes

soareschen mentioned this pull request Jun 2, 2021

Quick fix for update client when trusted_height == target_height #1036

Closed

5 tasks

adizere closed this Jun 9, 2021

romac deleted the soares/update-client-refactoring branch January 26, 2022 14:19

soareschen mentioned this pull request Mar 30, 2022

Option type should be used with non-zero Height #1009

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring and improve update client semantics #1011

Refactoring and improve update client semantics #1011

soareschen commented May 28, 2021 •

edited

Loading

adizere left a comment •

edited

Loading

adizere May 28, 2021

soareschen Jun 1, 2021

adizere Jun 2, 2021

soareschen Jun 2, 2021

soareschen commented Jun 1, 2021

adizere left a comment

adizere Jun 2, 2021

adizere Jun 2, 2021

ancazamfir Jun 2, 2021

adizere Jun 2, 2021

soareschen Jun 2, 2021

adizere Jun 2, 2021

ancazamfir Jun 2, 2021

ancazamfir commented Jun 2, 2021

soareschen commented Jun 2, 2021

soareschen commented Jun 2, 2021

soareschen commented Jun 2, 2021

romac commented Jun 2, 2021

soareschen commented Jun 2, 2021

adizere commented Jun 9, 2021

Refactoring and improve update client semantics #1011

Refactoring and improve update client semantics #1011

Conversation

soareschen commented May 28, 2021 • edited Loading

Description

adizere left a comment • 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

soareschen commented Jun 1, 2021

adizere 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

ancazamfir commented Jun 2, 2021

soareschen commented Jun 2, 2021

soareschen commented Jun 2, 2021

soareschen commented Jun 2, 2021

romac commented Jun 2, 2021

soareschen commented Jun 2, 2021

adizere commented Jun 9, 2021

soareschen commented May 28, 2021 •

edited

Loading

adizere left a comment •

edited

Loading