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 IBC logic from x/upgrade #8673

Merged
merged 47 commits into from
Mar 1, 2021
Merged

Remove IBC logic from x/upgrade #8673

merged 47 commits into from
Mar 1, 2021

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Feb 23, 2021

Description

As part of the preparation for #8501, IBC needs to be removed from x/upgrade. We still need to set upgraded client states and consensus states in the upgrade store (otherwise chains would need to do an IBC breaking upgrade, which is not desirable).

To mitigate these issues, x/upgrade will now set bytes for the client state and consensus states, thereby removing notions of what IBC is setting in the upgrade store. The abci begin blocker setting of consensus states has been moved to 02-client. A new proposal type is added to IBC which wraps an upgrade plan. If the proposal is successful it will execute IBC upgrade logic and schedule an upgrade

closes: #XXXX


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@colin-axner colin-axner force-pushed the colin/rm-ibc-from-upgrade branch from 1101778 to 31ca56d Compare February 24, 2021 11:30
x/upgrade/abci_test.go Show resolved Hide resolved
x/upgrade/abci_test.go Outdated Show resolved Hide resolved
@colin-axner colin-axner marked this pull request as ready for review February 24, 2021 13:12
@colin-axner colin-axner requested a review from cwgoes February 24, 2021 13:12
@@ -64,5 +64,5 @@ message QueryUpgradedConsensusStateRequest {
// QueryUpgradedConsensusStateResponse is the response type for the Query/UpgradedConsensusState
// RPC method.
message QueryUpgradedConsensusStateResponse {
google.protobuf.Any upgraded_consensus_state = 1;
bytes upgraded_consensus_state = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is proto-breaking. Could we just add a new field instead? Or else we can also bump to v1beta2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do whichever you prefer. The old fields would just be unused if I added a new field. It would also be unusable since I cannot fulfill the UnpackInterfaces function since there won't be a interface to unpack into. I presume I'd have to change this for the Upgrade Plan as well or does only the Query definitions matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not bumping to v1beta2 would be ideal. If the old field is not used anymore, I would say let's go for reserved, like here

See https://developers.google.com/protocol-buffers/docs/proto#updating

Non-required fields can be removed, as long as the field number is not used again in your updated message type. You may want to rename the field instead, perhaps adding the prefix "OBSOLETE_", or make the field number reserved, so that future users of your .proto can't accidentally reuse the number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sweet, thanks for catching this. Updated it, let me know if anything is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proto-breakage check still says breaking, but I presume this is fine?

Do I need the reserved "option"; part?

@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

Merging #8673 (9dbcbe1) into master (a93edee) will decrease coverage by 0.05%.
The diff coverage is 59.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8673      +/-   ##
==========================================
- Coverage   61.45%   61.39%   -0.06%     
==========================================
  Files         673      671       -2     
  Lines       38306    38415     +109     
==========================================
+ Hits        23541    23586      +45     
- Misses      12285    12360      +75     
+ Partials     2480     2469      -11     
Impacted Files Coverage Δ
x/ibc/core/02-client/keeper/grpc_query.go 60.46% <0.00%> (-16.01%) ⬇️
x/ibc/core/02-client/proposal_handler.go 75.00% <0.00%> (-25.00%) ⬇️
x/ibc/core/02-client/types/keys.go 100.00% <ø> (ø)
x/ibc/core/keeper/grpc_query.go 0.00% <0.00%> (ø)
x/ibc/light-clients/07-tendermint/types/upgrade.go 73.33% <ø> (-3.34%) ⬇️
x/upgrade/abci.go 92.30% <ø> (-1.03%) ⬇️
x/upgrade/keeper/grpc_query.go 58.82% <0.00%> (+8.82%) ⬆️
x/upgrade/types/proposal.go 84.61% <ø> (-0.57%) ⬇️
x/upgrade/keeper/keeper.go 73.40% <60.00%> (+5.30%) ⬆️
x/ibc/core/02-client/types/proposal.go 82.53% <77.50%> (-8.77%) ⬇️
... and 19 more

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Mainly worried about illegible data in the proposal.

Maybe we should move the proposal to IBC now. The IBC proposal can contain the ibcexported ClientState, and then just marshal it before storing in x/upgrade

docs/core/proto-docs.md Outdated Show resolved Hide resolved
proto/cosmos/upgrade/v1beta1/query.proto Outdated Show resolved Hide resolved
x/ibc/core/02-client/abci_test.go Outdated Show resolved Hide resolved
x/ibc/light-clients/07-tendermint/types/upgrade.go Outdated Show resolved Hide resolved
x/ibc/light-clients/07-tendermint/types/upgrade.go Outdated Show resolved Hide resolved
@@ -64,5 +64,8 @@ message QueryUpgradedConsensusStateRequest {
// QueryUpgradedConsensusStateResponse is the response type for the Query/UpgradedConsensusState
// RPC method.
message QueryUpgradedConsensusStateResponse {
google.protobuf.Any upgraded_consensus_state = 1;
reserved 1;
reserved "option";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
reserved "option";

You can remove this. This is to reserve a field name. Since you're using the same field name in the new field, there's no need to reserve anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know, thanks. I'm waiting on a confirmation from @AdityaSripal, but I think the direction with this pr might be to remove IBC entirely:

  • create a duplicate proposal type in IBC that wraps an upgrade plan, then call ScheduleUpgrade from within IBC
  • handle IBC related logic in IBC

This would make this field obsolete. Any preference for how to deprecate QueryUpgradedConsensusStateResponse? And for Plan I guess reserving the field and name would be sufficient

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. Yes, reserving both field number and name should be enough.

Add a new Key to the IBC client store to track the IBC Upgrade Height. This allows IBC upgrades to correctly remove old IBC upgrade states
if up.Plan.Time.Unix() > 0 {
return sdkerrors.Wrap(ErrInvalidUpgradeProposal, "IBC chain upgrades must only set height")
}

if up.Plan.Height <= 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unreachable but safe to have. plan.ValidateBasic won't allow not setting time and height and it won't allow setting both time and height

@colin-axner
Copy link
Contributor Author

@cwgoes @AdityaSripal @fedekunze see the last bit of logic added. Will merge after I get approvals again

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

ACK

@colin-axner colin-axner mentioned this pull request Mar 1, 2021
9 tasks
Revert using a key to track IBC upgrades. By clearing any IBC state using an old plan in ScheduleUpgrade, IBC upgrades do not need to be tracked by IBC. This reduces code complexity and reduces potential for bugs.
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

ACK final approach, great work @colin-axner!

@colin-axner colin-axner added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 1, 2021
@mergify mergify bot merged commit 1c6e267 into master Mar 1, 2021
@mergify mergify bot deleted the colin/rm-ibc-from-upgrade branch March 1, 2021 16:28
google.protobuf.Any upgraded_consensus_state = 1;
reserved 1;

bytes upgraded_consensus_state = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this was changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for not documenting it, but I believe the reason why is because then the upgrade module would have a dependency on the IBC module which we wanted to completely avoid. In order to return the correct Any, the upgrade module would need to understand what an IBC consensus state is, thus creating a dependency on the IBC module

Is there an issue with the new behaviour?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It breaks the API. It seams that you were not correctly using this function - because Any was not used.

Copy link
Contributor Author

@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.

This is just a convenience function for the cli. Relayers who perform the client upgrade functionality do ABCI queries to obtain the proofs necessary

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* add zeroed custom fields check to tm client

* remove custom fields function from x/upgrade and fix tests

* use []byte in x/upgrade, move abci to 02-client

* remove x/ibc from types

* whoops, delete testing files

* fix upgrade tests

* fix tm tests

* fix tests

* update CHANGELOG

* revert proto breakage, use reserved field cc @AmauryM

* add IBC Upgrade Proposal type

* remove IBC from upgrade types

* add IBC upgrade logic to 02-client

* fix all tests for x/upgrade

* Add CLI for IBC Upgrade Proposal

* Update x/ibc/core/02-client/types/proposal_test.go

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

* add gRPC for upgraded client state

* test fixes

* add HandleUpgradeProposal tests

* update docs and remove unnecessary code

* self review bug and test fixes

* neatness

* construct empty rest handler

* fix tests

* fix stringer tests

* Update docs/core/proto-docs.md

Co-authored-by: Christopher Goes <cwgoes@pluranimity.org>

* add key in ibc store tracking ibc upgrade heights

Add a new Key to the IBC client store to track the IBC Upgrade Height. This allows IBC upgrades to correctly remove old IBC upgrade states

* update abci and tests

* revert key storage after discussion with @AdityaSripal

Revert using a key to track IBC upgrades. By clearing any IBC state using an old plan in ScheduleUpgrade, IBC upgrades do not need to be tracked by IBC. This reduces code complexity and reduces potential for bugs.

* clear IBC states on cancelled upgrades

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Christopher Goes <cwgoes@pluranimity.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants