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

CLI create channel can fail and spam with unused clients/connections #1421

Closed
4 of 7 tasks
Tracked by #1562
adizere opened this issue Oct 5, 2021 · 7 comments · Fixed by #2014
Closed
4 of 7 tasks
Tracked by #1562

CLI create channel can fail and spam with unused clients/connections #1421

adizere opened this issue Oct 5, 2021 · 7 comments · Fixed by #2014
Assignees
Labels
A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews I: CLI Internal: related to the relayer's CLI O: usability Objective: cause to improve the user experience (UX) and ease using the product
Milestone

Comments

@adizere
Copy link
Member

adizere commented Oct 5, 2021

Crate

ibc-relayer-cli

Summary of problem

This problem is from a conversation with a relayer operator. Ref:
https://twitter.com/gadikian/status/1445306915211251718

The issue documented here appears as a consequence of:

  • Hermes giving up very easily in CLIs, and at the same time
  • abusing the create channel command

More concretely, if a user runs create channel and notices a failure, then the first instinct is to call create channel again, and again, and again. This can lead to a "spam-attack" of creating many apparently useless clients (and sometimes also connections, depending when the failure happens). The problem appears in particular for production chains, where Hermes is unable to find confirmations within the preconfigured rpc_timeout = 10s time (or even longer)

https://github.com/informalsystems/ibc-rs/blob/61cd02d1de296e6ab09df30f1f88bb7c8f8cfa33/config.toml#L91

Diagnostic

This is not exactly a Hermes bug. Documenting it here so we can learn from how our users understand the CLIs and how we can improve them.

The problem is rather a combination of multiple circumstance: unintuitive UI, lack of clear logging in the error case for create channel, abusing this CLI, and Hermes giving up too easily with confirming tx-es for mainnets.

Proposed solution

I think the solution is to introduce an additional parameter to create channel e.g., the option [--fresh] which is required and users have to specify it when they want fresh new clients and connections underlying their new channel. It's very possible that the CLI will continue to be misused otherwise.

More importantly: In addition we should consider making Hermes more stubborn when executing CLIs as critical as create channel or create connection or create client. These CLIs can sometimes fail even on testnets (with a timeout of 5s) and we recently increased that timeout (#1385) to avoid this issue. It would make sense to make Hermes either:

a. retry for a couple of minutes (hardcoded is fine) to find tx confirmations before giving up,
b. explicitly log and inform the user that rpc_timeout may be too small or these was an unknown problem (gas prices or fees can also affect this)

Version

0.7.3

Steps to Reproduce

  • hermes create channel juno-1 osmosis-1 --port-a transfer --port-b transfer
  • repeat above

Acceptance Criteria

  • add explicit --fresh flag for create CLIs
  • improve the UX of create CLIs with respect to timing out confirmations
    • handled in Structured logs for relayer logic #1491
    • the PR above overrides the rpc_timeout parameter for non-start CLIs, so that Hermes is more stubborn when creating clients, channels, connections, etc.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@adizere adizere added I: CLI Internal: related to the relayer's CLI O: usability Objective: cause to improve the user experience (UX) and ease using the product labels Oct 5, 2021
@adizere adizere added this to the 11.2021 milestone Oct 5, 2021
@romac romac added A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews and removed P-medium labels Nov 9, 2021
@romac romac mentioned this issue Nov 11, 2021
7 tasks
@adizere adizere modified the milestones: v0.9.0, v0.9.3 Nov 16, 2021
@adizere adizere modified the milestones: v0.11.1, v0.11.2 Jan 26, 2022
@romac romac modified the milestones: v0.12.0, v0.12.1 Feb 8, 2022
@seanchen1991
Copy link
Contributor

If the --fresh flag is required in for new clients to be created, doesn't that mean that the first time a user legitimately wants to execute create channel, the --fresh flag will have to be included? Then if that fails, the user will simply re-enter the last create channel command they just executed, which will already include the --fresh flag. That's the flow that I'm thinking of in this case; is that correct? And if it is, then it doesn't seem like the --fresh flag is actually fixing the issue here.

@adizere
Copy link
Member Author

adizere commented Mar 10, 2022

Then if that fails, the user will simply re-enter the last create channel command they just executed, which will already include the --fresh flag. That's the flow that I'm thinking of in this case; is that correct?

Hmm you're right, I hadn't thought of that! What would you recommend as an alternative to the --fresh flag solution?

@seanchen1991
Copy link
Contributor

It sounds like we want to introduce something akin to rate limiting when it comes to these commands such that subsequent executions of the same command within the cooldown period are ignored.

@adizere
Copy link
Member Author

adizere commented Mar 22, 2022

One more data point I got today wrt our create channel CLI, from a relayer operator:

We understood that whenever a chain becomes IBC-enabled we should create a channel between that chain and the other main chains (eg Hub and Osmosis). Usually multiple operators do that, not knowing about one another because of miscommunication and lack of coordination, which results in ad-hoc channels, connections and clients being created, some of them with poorly configured client security parameter (eg very small trusting period). It's not clear when & how to use the create channel command.

The way I interpret this is as another signal that create channel is easily abused and poorly understood.

@adizere
Copy link
Member Author

adizere commented Mar 22, 2022

From today's meeting notes: we will try the following approach (instead of --fresh):

  • Interactive Y/N input when we pass a —-new-client-connection. Also a disclaimer at WARN level.
    - Consider also --yes to override interactivity.

@seanchen1991
Copy link
Contributor

Currently the command can be run in this way:

hermes create channel --port-a <PORT-ID> --port-b <PORT-ID> <CHANNEL-A-ID> <CHANNEL-B-ID>

If the --new-client-connection flag is not passed as part of this invocation, should this command simply fail?

@adizere adizere modified the milestones: v0.13.0, v0.14.0 Mar 24, 2022
@adizere
Copy link
Member Author

adizere commented Mar 24, 2022

If the --new-client-connection flag is not passed as part of this invocation, should this command simply fail?

Yes.

I think we want to change the default invocation from:

hermes create channel --port-a <PORT-ID> --port-b <PORT-ID> <chain-A-ID> <chain-B-ID>

to

hermes create channel --port-a <PORT-ID> --port-b <PORT-ID> <chain-A-ID> <connection-A-ID>

In other words, if the users wants fresh new clients and connection, they would need to invoke the command differently:

hermes create channel --port-a <PORT-ID> --port-b <PORT-ID> <chain-A-ID> <chain-B-ID>  --new-client-connection 

So the presence of --new-client-connection implies that:

  • The invocation should specify a <chain-B-ID> option

  • The invocation should omit specifying any connection identifier <connection-A-ID>

  • Hermes will interactively ask:

WARN: Are you sure you want new clients & connection created? Hermes will use default security parameters.
Y/N
Hint: consider using the default invocation hermes create channel --port-a <PORT-ID> --port-b <PORT-ID> <chain-A-ID> <connection-A-ID> to re-use an already existing connection.

WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews I: CLI Internal: related to the relayer's CLI O: usability Objective: cause to improve the user experience (UX) and ease using the product
Projects
No open projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

3 participants