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

revert: Mark upgrade query field as deprecated instead of reserved #9847

Closed
wants to merge 1 commit into from

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Aug 4, 2021

Description

In #8673, we marked the upgraded_consensus_state field inside the x/upgrade QueryUpgradedConsensusStateResponse as reserved, thinking that it was alright.

After #9613, we should not mark fields as reserved. This PR reverts that change, and instead marks the field as deprecated following ADR-044.

ref: https://github.com/cosmos/cosmos-sdk/pull/8673/files#r682168269
ref: confio/cosmjs-types#4 (comment)


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)


bytes upgraded_consensus_state = 2;
bytes upgraded_consensus_state_bytes = 2;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot have two fields with same name. I propose to:

  • keep upgraded_consensus_state for the Any field, same as in 0.42
  • rename to new bytes field to upgraded_consensus_state_bytes.

This would be a breaking change in the RCs, and ibc-go might need to change some code. cc @colin-axner

Copy link
Collaborator

@robert-zaremba robert-zaremba Aug 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change in 0.43.0-RC -> 0.43.0-RC3.

I know it sounds "hacky" - but this thing was only added for IBC fix and only IBC uses it. So maybe we can keep the original breaking change (from https://github.com/cosmos/cosmos-sdk/pull/8673/files#r682168269)?

Copy link
Contributor

@colin-axner colin-axner Aug 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fairly certain ibc-go doesn't rely on this gRPC and relayers don't either. Relayers use ABCI queries since they need proof of the consensus state. To the best of my knowledge, it is unused

@webmaster128
Copy link
Member

I don't mind any changes in this type, neither the incompatible break, nor the still incompatible revert. We never used it in the client and I don't see that changing any time soon.

@robert-zaremba
Copy link
Collaborator

Worth to link the #8673 (comment) response
it seams it's only for CLI.

@amaury1093
Copy link
Contributor Author

amaury1093 commented Aug 4, 2021

OK. I created this PR so the overall proto upgrade between 0.42 -> 0.43 is more kosher (i.e. no breaking change). But if nobody uses this field and it breaks RCs, let's just:

  • either add a proto comment that this field shouldn't be used by gRPC clients
  • or just close this PR and release 0.43.0-rc3?

@robert-zaremba
Copy link
Collaborator

I'm for keeping the "current" version. I'm not sure if a comment will add anything (maybe some historical info)?

@amaury1093 amaury1093 closed this Aug 5, 2021
@amaury1093 amaury1093 deleted the am/fix-proto-upgrade branch August 5, 2021 08:25
@colin-axner
Copy link
Contributor

Current version sounds good to me

Sorry I was a little distracted yesterday, but to add more color on the context here. The upgraded consensus state only exists within state at the block height committed before the upgrade is scheduled. It will be removed immediately when the upgrade is performed. Furthermore it only contains the block time and next validator hash, which could be determined via different queries. On top of that, I just noticed we have added our own gRPC to fulfill this functionality. The upgraded consensus state information would only be useful information to a developer debugging an issue, I see no reason any external client would use this gRPC (unless gRPCs could return proofs)

We should just remove this functionality entirely in v0.44 before someone starts trying to use it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants