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

Remove error string from ics27 ack #744

Closed
3 tasks
colin-axner opened this issue Jan 17, 2022 · 2 comments · Fixed by #751
Closed
3 tasks

Remove error string from ics27 ack #744

colin-axner opened this issue Jan 17, 2022 · 2 comments · Fixed by #751
Assignees
Labels
27-interchain-accounts type: bug Something isn't working as expected

Comments

@colin-axner
Copy link
Contributor

colin-axner commented Jan 17, 2022

Summary of Bug

The ics27 host submodule currently returns the error string from message execution in the channel acknowledgement. If this value changes between patch releases, the marshalled acknowledgement will also changes. Nodes running different patch versions will set in state different acknowledgements and thus reach an app hash mismatch. Making error strings changes state machine breaking is too strict of a requirement for ics27 to impose on an entire chain.

Fix

Remove the error string from the channel acknowledgement. Emit the message execution error in the events.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added type: bug Something isn't working as expected 27-interchain-accounts labels Jan 17, 2022
@colin-axner colin-axner added this to the Interchain Accounts milestone Jan 17, 2022
@damiannolan damiannolan moved this to Backlog in ibc-go Jan 17, 2022
@damiannolan damiannolan self-assigned this Jan 18, 2022
@damiannolan
Copy link
Member

I've opened #751 with a fix for this. Should ICS27 be emitting more general events as well? Off the top of my head:

  • successful ack
  • account registration
  • successful msg execution

I've also noticed this issue is true for transfer? Would you like me to do the same there?

@damiannolan damiannolan moved this from Backlog to Todo in ibc-go Jan 18, 2022
@damiannolan damiannolan moved this from Todo to In progress in ibc-go Jan 18, 2022
@damiannolan damiannolan moved this from In progress to In review in ibc-go Jan 18, 2022
@colin-axner
Copy link
Contributor Author

colin-axner commented Jan 18, 2022

I've also noticed this issue is true for transfer? Would you like me to do the same there?

This is a really good point. We have two options:

  • ensure no calls to external modules outside ibc-go include error messages in our ack
  • remove the error string

I think we should probably remove the error string and document this issue. I think we could handle ensuring external calls are not included in our ack, but I don't think we can expect other developers developing IBC app modules to be aware of this issue, and since they will copy our code, it is probably best to keep it simple

Repository owner moved this from In review to Done in ibc-go Jan 20, 2022
faddat pushed a commit to notional-labs/ibc-go that referenced this issue Feb 23, 2022
Add static checking for common bug patterns
CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this issue Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
27-interchain-accounts type: bug Something isn't working as expected
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants