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

Provider and Consumer's OnAcknowledgementPacket should only log an error #547

Closed
Tracked by #1086
shaspitz opened this issue Dec 2, 2022 · 4 comments
Closed
Tracked by #1086
Labels
invalid This doesn't seem right S: NewThings Work towards your business objectives with new products, features, or integrations

Comments

@shaspitz
Copy link
Contributor

shaspitz commented Dec 2, 2022

Problem

OnAcknowledgementPacket currently closes the CCV channel in the event that an IBC err ack is returned from the provider. If CCV is working as intended, these paths should never be executed. Therefore a returned err ack is probably a symptom of a deeper bug that needs to be addressed manually.

Closing criteria

Either only log an error in OnAcknowledgementPacket, or halt the consumer. Let users know that there's a critical bug to be addressed, without polluting state by executing automatic recovery logic.

@shaspitz shaspitz added the invalid This doesn't seem right label Dec 2, 2022
@shaspitz shaspitz moved this to Todo in Replicated Security Dec 2, 2022
@shaspitz shaspitz changed the title Consumer's OnAcknowledgementPacket should only log an error Provider and Consumer's OnAcknowledgementPacket should only log an error Dec 2, 2022
@shaspitz
Copy link
Contributor Author

shaspitz commented Dec 2, 2022

Edit: this issue also applies to the provider's OnAcknowledgementPacket method, and should be changed for the same reasons

@mpoke
Copy link
Contributor

mpoke commented Dec 6, 2022

@jtremback This is a product discussion. Should a critical bug (e.g., marshaling errors) result in the consumer shutting down?

@shaspitz shaspitz mentioned this issue Dec 19, 2022
3 tasks
@mpoke mpoke self-assigned this Dec 20, 2022
@mpoke mpoke moved this from Todo to Next in Replicated Security Dec 21, 2022
@mpoke mpoke added this to Cosmos Hub Jan 27, 2023
@github-project-automation github-project-automation bot moved this to 🩹 Triage in Cosmos Hub Jan 27, 2023
@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 moved this from 🩹 Triage to 📥 Todo in Cosmos Hub Mar 5, 2023
@mpoke mpoke added the good first issue Good for newcomers label Mar 13, 2023
@mpoke mpoke modified the milestones: ICS next release, Gaia v10.0.0 Mar 13, 2023
@mpoke mpoke removed their assignment Jun 20, 2023
@mpoke mpoke removed this from the Gaia v10.0.0 milestone Jun 20, 2023
@shaspitz
Copy link
Contributor Author

Note if we're able to implement epochs, we could elegantly halt the consumer upon receiving an err ack, be able to patch the consumer binary with no state migrations, and resume the consumer once fixed. Any epoch packets constructed before/during the halt would be queued up, and ready to send once the consumer is back up

@mpoke mpoke moved this from 📥 Todo to 🏗 In progress in Cosmos Hub Jun 29, 2023
@mpoke mpoke removed the good first issue Good for newcomers label Jun 29, 2023
@mpoke mpoke moved this from 🏗 In progress to 📥 Todo in Cosmos Hub Aug 14, 2023
@mpoke mpoke added the S: NewThings Work towards your business objectives with new products, features, or integrations label Sep 12, 2023
@mpoke mpoke moved this from 📥 F2: Todo to 🩹 F1: Triage in Cosmos Hub Sep 12, 2023
@mpoke
Copy link
Contributor

mpoke commented Nov 16, 2023

Closing as non-issue. The behavior is as intended.

@mpoke mpoke closed this as completed Nov 16, 2023
@github-project-automation github-project-automation bot moved this from 🩹 F1: Triage to 👍 F4: Assessment in Cosmos Hub Nov 16, 2023
@mpoke mpoke moved this from 👍 F4: Assessment to ✅ Done in Cosmos Hub Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right S: NewThings Work towards your business objectives with new products, features, or integrations
Projects
Status: ✅ Done
Development

No branches or pull requests

2 participants