Use custom options of the `create client` command #2004

mzabaluev · 2022-03-23T14:17:16Z

Closes: #1921

Description

Rework settings passed to build_client_state to make the custom parameters of create client command work.

PR author checklist:

Added changelog entry, using unclog.
Added tests: integration (for Hermes) or unit/mock tests (for modules).
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

ClientSettings is now realized from CreateOptions and chain configurations.

relayer/src/foreign_client.rs

adizere

I did some tests and the CLI works as expected when customizing trusting period and trust threshold.

For customizing clock drift, I realize it's not clear to me what the expected behavior should be.

Example:

hermes create client ibc-0 ibc-1 -t 1/2 -p 1200s -d 2s

Do we want 2s to be either:

option A: the final clock drift used in this client state? Or
option B: do we want to use 2s to compute the client's clock drift (like we used to do here).

In this PR, we implement ~~option B~~ option A. I agree with this. So the CLI above gives

...
trust_level: TrustThreshold {
numerator: 1,
denominator: 2,
},
trusting_period: 1200s,
unbonding_period: 1814400s,
max_clock_drift: 2s,

Note that the max_clock_drift is set to 2s, as I specified in the create command CLI. This means we should update the CLI help command, which is not accurate at the moment, because it says the following:

-d, --clock-drift <CLOCK_DRIFT>
Override the default clock drift specified in the configuration.

With -d we're not exactly overriding the configuration value. Instead, we are providing the exact clock_drift to be used in the created client state.

relayer/src/chain/client.rs

Co-authored-by: Adi Seredinschi <adi@informal.systems>

Explain that the value goes directly into the client state parameter.

…stems/ibc-rs into mikhail/create-client-options

ancazamfir · 2022-03-24T09:06:38Z

Do we want 2s to be either:

option A: the final clock drift used in this client state? Or

option B: do we want to use 2s to compute the client's clock drift (like we used to do here).

In this PR, we implement option B.

I think we implement option A, right?

I also agree with A and we should update the help string.

ancazamfir · 2022-03-24T09:16:02Z

One other thought is to warn the user if the configured client max_clock_drift is smaller than the destination chain max_block_time. In this case there is a high chance of "header in the future" chain errors.

tools/test-framework/src/bootstrap/binary/chain.rs

Make the settings specific to the chain type, in anticipation of support for non-Cosmos chains.

Change the test framework API to bootstrap ForeignClient and ForeignClientPair to use the builder pattern. The client_settings methods are used to override the default client settings.

tools/integration-test/src/tests/client_settings.rs

Use the IBC TrustThreshold type throughout. Override a warning.

soareschen · 2022-03-30T16:10:35Z

tools/integration-test/src/tests/client_settings.rs

+    }
+}
+
+impl BinaryChainTest for ClientDefaultsTest {


I think it would be clearer to have a BinaryNodeTest for this case, with the foreign clients being created manually with the custom client settings you want. Basically the test should call ForeignClient::new() or ForeignClient::restore() directly, and then call ForeignClient::build_create_client_and_send() with the custom client settings, because this is the actual method we want to test here.

For this particular test, it is more of testing that the test framework is correctly initializing the foreign clients with the wanted settings. We could still leave the test here for testing the correctness of the test framework itself.

I think it is also worth writing some simple tests to demonstrate why we need a SettingsTestOverrides. For example, is having the ability to override the max_clock_drift setting helps us write new integration tests that we couldn't before? If the tests are too complicated to write in this PR, then we should file a new issues to follow up on writing these tests.

Good points, the tests could exercise the API directly indeed rather than deferring it to the framework.

As for ClientSettingsOverride, I think the ability to tweak the pre-created clients could be useful, and it does not get in the way of writing any tests that don't need this feature. The long trait bound lists in the framework code itself could become unwieldy, but they could be made more manageable with helper traits if needed. One thing this override trait cannot address is n-ary tests, but the same problem exists for the other override traits.

Ok, so do you think it is unnecessary to write another test using BinaryNodeTest to test ForeignClient::build_create_client_and_send() directly?

One thing this override trait cannot address is n-ary tests, but the same problem exists for the other override traits.

For the case of ports override, we use nested arrays to provide a matrix of overrides. But I think we can just omit that for now and only add them when it is needed.

@romac says custom client settings could be useful to test misbehavior detection.

Actually I meant writing an additional test to test the function directly. But anyway it's a nit and doesn't matter too much.

tools/integration-test/src/tests/client_settings.rs

adizere

I only did a cursory review of the code, looks good, I have no suggestions there, cool work Mikhail esp. with integration test and modular support for capturing client state setting!

The CLI behaves as expected.

Will let @ancazamfir and @soareschen weigh in on the details of the implementation.

adizere · 2022-03-31T14:42:31Z

relayer/src/chain/client.rs

+/// The parameters are specialized for each supported chain type.
+#[derive(Clone, Debug)]
+pub enum ClientSettings {
+    Cosmos(cosmos::client::Settings),


I'm not sure if Cosmos or Tendermint is the right name to use here.
AnyClientState uses Tendermint, maybe we should use it here too for consistency?

This is a very good point. Tendermint is more appropriate than Cosmos.

OTOH the module hierarchy under chain uses cosmos. This is a relayer API anyway, so we don't have to be super-diligent.

adizere · 2022-03-31T16:33:54Z

Added the "needs guide update" label as it would be worthwhile to document the extended support for -t -d and -p options in the create client guide entry: https://hermes.informal.systems/commands/path-setup/clients.html#create-client

relayer/src/chain/cosmos.rs

Restore the "phase transition" between user-supplied options in the relayer CLI and client settings in the chain interface. This gets rid of the ugly must-use method (fill_in_from_chain_configs) and makes the ForeignClient API easier to use.

* Rework of CreateParams -> ClientOptions * Split CreateOptions and ClientSettings ClientSettings is now realized from CreateOptions and chain configurations. * Update docs in relayer/src/chain/client.rs Co-authored-by: Adi Seredinschi <adi@informal.systems> * Improve help for --clock-drift Explain that the value goes directly into the client state parameter. * Rework ClientSettings Make the settings specific to the chain type, in anticipation of support for non-Cosmos chains. * Builder pattern for foreign client tests Change the test framework API to bootstrap ForeignClient and ForeignClientPair to use the builder pattern. The client_settings methods are used to override the default client settings. * Add client setting overrides to the test framework * Warn when client clock drift exceeds max_block_time * relayer: Sanitize imports in mod chain

mzabaluev added 2 commits March 23, 2022 13:47

Rework of CreateParams -> ClientOptions

3883068

Split CreateOptions and ClientSettings

db563e4

ClientSettings is now realized from CreateOptions and chain configurations.

mzabaluev changed the title ~~Mikhail/create client options~~ Use custom options of the create client command Mar 23, 2022

romac reviewed Mar 23, 2022

View reviewed changes

relayer/src/foreign_client.rs Outdated Show resolved Hide resolved

adizere requested changes Mar 23, 2022

View reviewed changes

relayer/src/chain/client.rs Outdated Show resolved Hide resolved

mzabaluev and others added 3 commits March 23, 2022 21:41

Update docs in relayer/src/chain/client.rs

abbd4dc

Co-authored-by: Adi Seredinschi <adi@informal.systems>

Improve help for --clock-drift

25665be

Explain that the value goes directly into the client state parameter.

Merge branch 'mikhail/create-client-options' of github.com:informalsy…

22a77c1

…stems/ibc-rs into mikhail/create-client-options

adizere reviewed Mar 24, 2022

View reviewed changes

tools/test-framework/src/bootstrap/binary/chain.rs Show resolved Hide resolved

mzabaluev added 4 commits March 25, 2022 18:59

Rework ClientSettings

b661a55

Make the settings specific to the chain type, in anticipation of support for non-Cosmos chains.

Merge branch 'master' into mikhail/create-client-options

2acb458

Builder pattern for foreign client tests

08748ee

Change the test framework API to bootstrap ForeignClient and ForeignClientPair to use the builder pattern. The client_settings methods are used to override the default client settings.

Add client setting overrides to the test framework

f18a9c1

mzabaluev requested a review from soareschen March 29, 2022 09:55

Draft test for custom client settings

e4ba706

mzabaluev commented Mar 29, 2022

View reviewed changes

tools/integration-test/src/tests/client_settings.rs Outdated Show resolved Hide resolved

mzabaluev added 12 commits March 29, 2022 16:35

Fix clippy lints

db8f60f

Merge branch 'master' into mikhail/create-client-options

5745837

Post-merge fixes

df321e6

client_settings test: query state of the client

a75d880

Make client_settings test work

f8f7d92

Merge branch 'master' into mikhail/create-client-options

fe2fe9f

Clean up client_settings test

9d4b8cf

Use the IBC TrustThreshold type throughout. Override a warning.

Add a test for default client settings

658ee0d

Use the IBC TrustThreshold type in client settings

a2d6688

Merge branch 'master' into mikhail/create-client-options

29e891c

Warn when client clock drift exceeds max_block_time

67ffff4

Changelog entry for #2004

7e45352

mzabaluev marked this pull request as ready for review March 30, 2022 12:00

mzabaluev requested a review from adizere March 30, 2022 12:01

soareschen requested changes Mar 30, 2022

View reviewed changes

soareschen reviewed Mar 31, 2022

View reviewed changes

tools/integration-test/src/tests/client_settings.rs Show resolved Hide resolved

soareschen reviewed Mar 31, 2022

View reviewed changes

tools/integration-test/src/tests/client_settings.rs Outdated Show resolved Hide resolved

Simplify overrides for ClientSettingsTest

4c76c3a

adizere approved these changes Mar 31, 2022

View reviewed changes

adizere added the needs guide update label Mar 31, 2022

mzabaluev added 2 commits April 1, 2022 00:38

integration-test: Remove an unused structure

f068c16

relayer: Sanitize imports in mod chain

9df0cba

soareschen approved these changes Apr 1, 2022

View reviewed changes

relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved

mzabaluev added 4 commits April 1, 2022 14:43

Rename ClientSettings::Cosmos -> Tendermint

d4ed916

Fix clippy lint

75d60a7

Merge branch 'master' into mikhail/create-client-options

5c3bef4

mzabaluev merged commit 1fda889 into master Apr 4, 2022

mzabaluev deleted the mikhail/create-client-options branch April 4, 2022 11:09

adizere mentioned this pull request Apr 25, 2022

Release v0.14 #2117

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use custom options of the `create client` command #2004

Use custom options of the `create client` command #2004

mzabaluev commented Mar 23, 2022 •

edited by adizere

Loading

adizere left a comment •

edited

Loading

ancazamfir commented Mar 24, 2022 •

edited

Loading

ancazamfir commented Mar 24, 2022

soareschen Mar 30, 2022

mzabaluev Mar 31, 2022

soareschen Mar 31, 2022

mzabaluev Mar 31, 2022

soareschen Apr 1, 2022

adizere left a comment

adizere Mar 31, 2022

mzabaluev Mar 31, 2022

adizere Apr 1, 2022

mzabaluev Apr 1, 2022

adizere commented Mar 31, 2022

Use custom options of the create client command #2004

Use custom options of the create client command #2004

Conversation

mzabaluev commented Mar 23, 2022 • edited by adizere Loading

Description

PR author checklist:

Reviewer checklist:

adizere left a comment • edited Loading

Choose a reason for hiding this comment

ancazamfir commented Mar 24, 2022 • edited Loading

ancazamfir commented Mar 24, 2022

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

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

adizere commented Mar 31, 2022

Use custom options of the `create client` command #2004

Use custom options of the `create client` command #2004

mzabaluev commented Mar 23, 2022 •

edited by adizere

Loading

adizere left a comment •

edited

Loading

ancazamfir commented Mar 24, 2022 •

edited

Loading