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

Decide if IBC err acks are important #568

Open
Tracked by #1086
shaspitz opened this issue Dec 7, 2022 · 6 comments
Open
Tracked by #1086

Decide if IBC err acks are important #568

shaspitz opened this issue Dec 7, 2022 · 6 comments
Assignees
Labels
more-info-needed Further information is requested S: ImprovingThings Improving things: Customer requests, performance improvements, reliability and usability

Comments

@shaspitz
Copy link
Contributor

shaspitz commented Dec 7, 2022

Problem

#542 makes it more explicit for which cases we return an IBC err ack when a slash packet is recv. However, we could simplify a lot of code if we do not return IBC err acks in any case, and always drop slash packets when there is something wrong with the data. This warrants a larger question, how much does it matter whether or not we return IBC err acks? Does this affect anything?

Should relayers be aware of the fact that they are relaying invalid packets? There are some cases that we currently silently fail slash packets for example, without a relayer being aware of such silent failures

CC @mpoke

Closing criteria

Decide if IBC err acks are important, act accordingly

@shaspitz
Copy link
Contributor Author

shaspitz commented Dec 7, 2022

Related to this, if IBC err acks are deemed to be neccessary. There are additional places we could return them when something goes wrong. Namely, VSCMatured packets could be validated upon being recv, and return an IBC err ack if there is something invalid with the vscID

Note: this would require logic and/or additional state to determine what makes a vscID invalid. There's not any sensible stateless validation we can make AFAIK

@danwt
Copy link
Contributor

danwt commented Dec 13, 2022

Doesn't an error ack close the channel?

@mpoke
Copy link
Contributor

mpoke commented Dec 13, 2022

Message from @colin-axner

In ibc-go the main difference is that core IBC will revert all application state changes if an error acknowledgement is returned. Other than that, the acknowledgement should be seen as opaque. See here

@mpoke
Copy link
Contributor

mpoke commented Dec 13, 2022

Doesn't an error ack close the channel?

AFAIK, not in the IBC module. Only if we close it on the application side (i.e., in the OnAck code).

@danwt
Copy link
Contributor

danwt commented Jan 3, 2023

AFAIK, not in the IBC module. Only if we close it on the application side (i.e., in the OnAck cod

Let's check. I think a while ago in a WIP PR I accidentally returned an error and it automatically closed the channel.

@mpoke
Copy link
Contributor

mpoke commented Jan 4, 2023

I think a while ago in a WIP PR I accidentally returned an error and it automatically closed the channel.

This is due to the code here or here. So, this is due to the application code (i.e., the ICS code).

@mpoke mpoke added this to the ICS v1.1.0 milestone Jan 27, 2023
@mpoke mpoke modified the milestones: ICS v1.1.0, ICS next release Mar 5, 2023
@mpoke mpoke added the S: ImprovingThings Improving things: Customer requests, performance improvements, reliability and usability label Sep 12, 2023
@mpoke mpoke added more-info-needed Further information is requested and removed question labels Mar 22, 2024
@mpoke mpoke self-assigned this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-info-needed Further information is requested S: ImprovingThings Improving things: Customer requests, performance improvements, reliability and usability
Projects
Status: 📥 F2: Todo
Development

No branches or pull requests

3 participants