Merge light clients config in relayer config and add commands to add/remove light clients #348

romac · 2020-10-29T15:42:05Z

Description

This PR merge the light clients config (previously in light_clients.toml) in the main relayer config and add commands to add/remove light clients.

Commands

`relayer light`

USAGE:
    relayer light <SUBCOMMAND>

DESCRIPTION:
    basic functionality for managing the lite clients

SUBCOMMANDS:
    add        add a light client peer for a given chain
    rm         remove a light client peer for a given chain

`relayer light add`

USAGE:
    relayer light add <OPTIONS>

DESCRIPTION:
    add a light client peer for a given chain

POSITIONAL ARGUMENTS:
    chain_id                  identifier of the chain

FLAGS:
    -i, --peer-id PEER-ID     peer id for this client
    -p, --primary             whether this is the primary peer
    -x, --hash HASH           trusted header hash
    -a, --address ADDRESS     RPC network address
    -h, --height HEIGHT       trusted header height

`relayer light rm`

USAGE:
    relayer light rm <OPTIONS>

DESCRIPTION:
    remove a light client peer for a given chain

POSITIONAL ARGUMENTS:
    chain_id                  identifier of the chain

FLAGS:
    -i, --peer-id PEER-ID     peer id for this client
    -f, --force               force removal of primary peer

Caveat

The main downside of this PR is that running either of the commands above will remove any comments or formatting in the config file. This could be improved by using format-preserving TOML parser/serializer (see toml_edit), but it would be a substantial amount of work to manually apply the config changes to the parsed TOML value with toml_edit. Definitely something to revisit in the future.

The main downside of this PR is that running either of the commands above will "scramble" the config file when writing the new configuration to it.
This stems from the fact I have not figured out a way to express our config structs so that they can be serialized to a TOML string directly via serde.

As a workaround, I am serializing the config struct to a TOML Value first and then serializing that to a string. Unfortunately, this does not respect the field order of the Rust structs and thus scrambles the original file (but preserves its semantics, thankfully).

I see a few potential ways out of that:

a) figure out how to express the config so that it can be directly serialized to a TOML string via Serde (no luck so far)
b) use a format-preserving TOML parser/serializer (see toml_edit)
c) move to another config format (Dhall, RON, ?)

Pros

a) quick fix if we can figure it out
b) would preserve formatting and comments
c) no need to massage our config struct to fit into what's accepted by TOML

Cons

a) comments and formatting would still be lost
b) it might take a substantial amount of work to manually apply the config changes to the parsed TOML value with toml_edit.
c) comments and formatting would still be lost

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.

ancazamfir · 2020-10-30T12:25:58Z

My concern with these commands is UX, the user has to do some work to retrieve the peer-id, hash, height, etc.
This could be done by the command itself:

USAGE:
    relayer-cli light add <OPTIONS>

DESCRIPTION:
    add a light client peer for a given chain

POSITIONAL ARGUMENTS:
    address                 RPC network address

FLAGS:
    -p, --primary             whether this is the primary peer
    -f, --force               allow overriding an existing peer

And obtain all the info from the status() rpc to the address specified, i.e. from network, id, latest_block_hash, latest_block_height.
Maybe one way is to allow only the address to be specified and in this case retrieve what is needed from status().

ancazamfir · 2020-11-02T12:47:12Z

relayer/src/config.rs

    #[serde(default = "default::trusting_period", with = "humantime_serde")]
    pub trusting_period: Duration,
+    #[serde(default)]
+    pub trust_threshold: TrustThreshold,


Should this be in the client config? Same question for the trusting_period? Different apps may use different clients of same chain if they have different security requirements.

@ebuchman ^

Tracked in #356

romac · 2020-11-02T12:48:13Z

Good point! Couple questions:

Is there any value in letting the user specify the height they want to trust?
What do you think of having the command print out the details it fetched from the node and ask for confirmation? Is that overkill?

ancazamfir · 2020-11-02T12:55:47Z

Good point! Couple questions:

Is there any value in letting the user specify the height they want to trust?

I think the user should be able to specify any number of params, at the minimum the rpc address. If height is specified then it should be used. If hash is specified then height must also be. But to start with, if we can allow just the address it will go a long way, especially in the development phase, CI integration and initial release with lower focus on security.

What do you think of having the command print out the details it fetched from the node and ask for confirmation? Is that overkill?

That would be nice :)

romac · 2020-11-02T19:03:35Z

But to start with, if we can allow just the address it will go a long way, especially in the development phase, CI integration and initial release with lower focus on security.

Done.

Here's the new usage of the light add command:

USAGE:
    relayer light add <OPTIONS>

DESCRIPTION:
    add a light client peer for a given chain

POSITIONAL ARGUMENTS:
    address                   RPC network address

FLAGS:
    -c, --chain-id CHAIN-ID   identifier of the chain
    -p, --primary             whether this is the primary peer
    -f, --force               allow overriding an existing peer
    -y, --yes                 skip confirmation

I also adapted the options for the light rm command for consistency with the new light add options:

USAGE:
    relayer light rm <OPTIONS>

DESCRIPTION:
    remove a light client peer for a given chain

POSITIONAL ARGUMENTS:
    peer_id                   peer id for this client

FLAGS:
    -c, --chain-id CHAIN-ID   identifier of the chain
    -f, --force               force removal of primary peer

ancazamfir

Looks great! Thanks Romain!

…remove light clients (informalsystems#348) * Implement command to add light client peer * Implement command to remove light client peer * Refactor config * Formatting * Cleanup * Cleanup deps * Fix start command * Cleanup example config * Add test for serializing config * Fix TOML serialization issue * Comments, bugfix, and better names * Fetch peer id, block hash and height from given node via RPC * Adapt light rm options for consistency with light add * Update changelog

romac added 11 commits October 28, 2020 15:38

Implement command to add light client peer

ca87d5f

Implement command to remove light client peer

4bf9ae1

Refactor config

3ce1d3c

Formatting

50acd78

Cleanup

579e76b

Cleanup deps

4a2cb8f

Fix start command

1c67d58

Cleanup example config

2e53878

Add test for serializing config

9ad6076

Fix TOML serialization issue

c425284

Comments, bugfix, and better names

c18512b

romac requested review from adizere and ancazamfir October 29, 2020 15:42

Merge branch 'master' into romac/light-config

4de4d4d

ancazamfir reviewed Nov 2, 2020

View reviewed changes

romac added 2 commits November 2, 2020 19:59

Fetch peer id, block hash and height from given node via RPC

8a3581f

Adapt light rm options for consistency with light add

a01d6fd

ancazamfir approved these changes Nov 3, 2020

View reviewed changes

Update changelog

f796833

romac mentioned this pull request Nov 3, 2020

Where should thetrust_threshold and trusting_period configuration params live? #356

Closed

5 tasks

romac merged commit 5f314e5 into master Nov 3, 2020

romac deleted the romac/light-config branch November 3, 2020 15:40

romac mentioned this pull request Nov 5, 2020

Allow specifying primary and witness peers for the light clients #311

Closed

4 tasks

ancazamfir mentioned this pull request Dec 23, 2020

Align relayer config file with the light client config #99

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge light clients config in relayer config and add commands to add/remove light clients #348

Merge light clients config in relayer config and add commands to add/remove light clients #348

romac commented Oct 29, 2020 •

edited

Loading

ancazamfir commented Oct 30, 2020 •

edited

Loading

ancazamfir Nov 2, 2020

romac Nov 3, 2020

romac Nov 3, 2020

romac commented Nov 2, 2020

ancazamfir commented Nov 2, 2020 •

edited

Loading

romac commented Nov 2, 2020 •

edited

Loading

ancazamfir left a comment

Merge light clients config in relayer config and add commands to add/remove light clients #348

Merge light clients config in relayer config and add commands to add/remove light clients #348

Conversation

romac commented Oct 29, 2020 • edited Loading

Description

Commands

relayer light

relayer light add

relayer light rm

Caveat

Pros

Cons

ancazamfir commented Oct 30, 2020 • edited Loading

ancazamfir Nov 2, 2020

Choose a reason for hiding this comment

romac Nov 3, 2020

Choose a reason for hiding this comment

romac Nov 3, 2020

Choose a reason for hiding this comment

romac commented Nov 2, 2020

ancazamfir commented Nov 2, 2020 • edited Loading

romac commented Nov 2, 2020 • edited Loading

ancazamfir left a comment

Choose a reason for hiding this comment

romac commented Oct 29, 2020 •

edited

Loading

`relayer light`

`relayer light add`

`relayer light rm`

ancazamfir commented Oct 30, 2020 •

edited

Loading

ancazamfir commented Nov 2, 2020 •

edited

Loading

romac commented Nov 2, 2020 •

edited

Loading