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

Allow Unordered ICA Channels #3996

Closed
10 tasks done
srdtrk opened this issue Jul 3, 2023 · 6 comments · Fixed by #5633
Closed
10 tasks done

Allow Unordered ICA Channels #3996

srdtrk opened this issue Jul 3, 2023 · 6 comments · Fixed by #5633
Labels
27-interchain-accounts epic type: feature New features, sub-features or integrations
Milestone

Comments

@srdtrk
Copy link
Member

srdtrk commented Jul 3, 2023

Summary

Currently, ICA only allows ordered channels. This causes the channel to close if a timeout occurs, forcing the user to reopen it.

Problem Definition

ICA only allows ordered channels because if ordering is not enforced, then the relayer could try malicious attacks by reordering/censoring some of the ica-controller's transactions. However, this requirement can be dropped because the ica-controller can choose not to send any new transactions until it receives an acknowledgement or a timeout packet.

Proposal

Tasks

  1. 27-interchain-accounts
    charleenfei
  2. 27-interchain-accounts
    chatton
  3. 27-interchain-accounts
    chatton
  4. 27-interchain-accounts
    DimitrisJim
  5. 27-interchain-accounts
    damiannolan
  6. 27-interchain-accounts
    DimitrisJim
  7. 27-interchain-accounts state machine breaking
    srdtrk
  8. 27-interchain-accounts e2e
    chatton
  9. 27-interchain-accounts e2e
    charleenfei
  10. 27-interchain-accounts docs
    chatton

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@srdtrk srdtrk added needs discussion Issues that need discussion before they can be worked on nice-to-have 27-interchain-accounts labels Jul 3, 2023
@migueldingli1997
Copy link

migueldingli1997 commented Sep 28, 2023

This would be very helpful for our use-case as well. We are using ICA round the clock with low timeout values for the transactions (because we want them to pass in a narrow time range, or not pass). If there is any event that causes this timeout then we're suddenly stuck with a closed channel that has to be re-opened.

We're already keeping track of tx sequences anyways so switching to an unordered channel would be a breeze.

@womensrights
Copy link
Contributor

Great to hear you are using ICA! Could you tell us a bit more about your precise use case if you are happy for it to be public? We had heard that ordered channels for ICA was a pain point for a few users but when we looked in more detail at the number of channel closures happening is actually really small. For example on Stride, the number of ICA timeouts has been 40/ 35,261 which is 0.11% of transactions since genesis and for Quicksilver it was 68 of 312,565 ICA txs since Jan. so approx 0.02% therefore the change would be a relatively small improvement it seems but would like to hear more about your own experience

@hxrts
Copy link

hxrts commented Oct 14, 2023

approx 0.02% therefore the change would be a relatively small improvement it seems

I think this may be due to survivorship bias. I know most relayers don't want to run ICAs because it's too error prone and complex. Devs have a much harder time building applications because ICA relay channels are harder to come by. You should speak to some of the devs from the recent Dora hackathon within the Neutron track at Cosmoverse. The challenge required devs to use ICA and most people gave up because it was too difficult.

@joe-bowman
Copy link
Contributor

Quicksilver it was 68 of 312,565 ICA txs since Jan. so approx 0.02% therefore the change would be a relatively small improvement

'Small improvement' in percentage of overall terms, but the wasted hours of finding, fixing and working around channel closures has been an expensive past time :)

'Big improvement' in DevX.

@womensrights
Copy link
Contributor

I'm not trying to disregard the pain of debugging the problem at all and certainly it is a DevX improvement, I just put the numbers there for context and I think its interesting to see. The changes to the spec required have already been merged, and the implementation work will be done sometime next year

@joe-bowman
Copy link
Contributor

Apologies; it was not my intention to suggest so :) great to see the spec is merged, I’m looking forward to it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
27-interchain-accounts epic type: feature New features, sub-features or integrations
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants