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 AllowUpdateAfter usage from 07-tendermint clients #607

Closed
3 tasks
colin-axner opened this issue Dec 8, 2021 · 5 comments
Closed
3 tasks

Remove AllowUpdateAfter usage from 07-tendermint clients #607

colin-axner opened this issue Dec 8, 2021 · 5 comments
Assignees

Comments

@colin-axner
Copy link
Contributor

colin-axner commented Dec 8, 2021

Summary

Disable recognition of AllowUpdateAfterExpiry and AllowUpdateAfterMisbheaviour

Problem Definition

Currently these parameters are intended to signal the recovery options for a client if they become expired or frozen. A governance proposal cannot overwrite a client if these parameters if they are set to false. However, a code migration can overwrite the client and consensus states regardless of the value of these parameters. Thus, I don't find value in enforcing these checks. If governance votes to overwrite a client or consensus state, it is likely governance is also willing to perform a code migration to do the same.

Proposal

Do not modify the client state. Leave the booleans there for signalling intent, but remove the enforcement checks. This will allow for easier means of client recovery

@cwgoes thoughts?


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added the needs discussion Issues that need discussion before they can be worked on label Dec 23, 2021
@cwgoes
Copy link
Contributor

cwgoes commented Jan 19, 2022

You're completely right, these don't make sense given the possibility of code migration. I'm fine with leaving them there for signalling intent, but I'm also fine with removing them altogether, arguably signalling should be done elsewhere (e.g. in the text of a governance proposal).

@charleenfei
Copy link
Contributor

@colin-axner I opted to remove the booleans entirely from the ClientState to avoid any kind of confusion regarding if they have effect or not. let me know if this should be revised to leave the booleans in --

@colin-axner
Copy link
Contributor Author

The booleans cannot be removed as this changes the encoding of the ClientState. Connection handshakes to chains without this change will fail as they will be trying to verify the old client state, thus breaking IBC backwards compatibility

@colin-axner
Copy link
Contributor Author

We can mark them as deprecated and if we ever implement a way to modify the client state without breaking new connection handshakes, then we can remove them

@crodriguezvega crodriguezvega removed the needs discussion Issues that need discussion before they can be worked on label Mar 21, 2022
@crodriguezvega crodriguezvega added this to the Q2-2022-backlog milestone Apr 19, 2022
@colin-axner
Copy link
Contributor Author

closed by #1118

CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this issue Nov 6, 2023
<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.
-->

## Overview

closes: cosmos#607 

<!-- 
Please provide an explanation of the PR, including the appropriate
context,
background, goal, and rationale. If there is an issue with this
information,
please provide a tl;dr and link the issue. 
-->

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [ ] New and updated code has new and/or updated testing
- [ ] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords

---------

Co-authored-by: Manav Aggarwal <manavaggarwal1234@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

4 participants