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.

Sign up for GitHub

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

Check that client state is zeroed out for ibc client upgrade proposals #1529

Closed
wants to merge 10 commits into from
Closed

Check that client state is zeroed out for ibc client upgrade proposals #1529

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d51c228
wip: Added check for zeroed out custom fields in ValidateBasic
chatton Jun 13, 2022
2e614a9
test: Added test for validate basic with non-zeroed fields.
chatton Jun 13, 2022
fd0f457
chore: Adding CHANGELOG.md entry.
chatton Jun 13, 2022
0ba2ab9
merge main
chatton Jun 20, 2022
6d8f3d5
chore: addressed PR feedback, updated CHANGELOG.md
chatton Jun 20, 2022
5b3028d
Merge branch 'main' into cian/issue#291-check-that-client-state-is-ze…
chatton Jun 22, 2022
64893c3
Merge branch 'main' into cian/issue#291-check-that-client-state-is-ze…
chatton Jul 5, 2022
595731a
Merge branch 'main' into cian/issue#291-check-that-client-state-is-ze…
chatton Jul 7, 2022
b1a3203
chore: addressing PR feedback
chatton Jul 7, 2022
98a179b
Merge branch 'cian/issue#291-check-that-client-state-is-zeroed-out-fo…
chatton Jul 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Bug Fixes

* (apps/29-fee) [\#1278](https://github.com/cosmos/ibc-go/pull/1278) The URI path for the query to get all incentivized packets for a specific channel did not follow the same format as the rest of queries.
* (apps/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1523) Fixed an issue where a bad refund address would prevent channel closure.
* (modules/core/02-client)[\#1529](https://github.com/cosmos/ibc-go/pull/1529) ClientState must be zeroed out for `UpgradeProposals` to pass validation.

## [v3.1.0](https://github.com/cosmos/ibc-go/releases/tag/v3.1.0) - 2022-04-16

Expand Down
8 changes: 6 additions & 2 deletions modules/core/02-client/types/proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ package types

import (
"fmt"

codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
"reflect"
Copy link
Member

Choose a reason for hiding this comment

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

nit: group import statements like so:

import (
        // standard library imports
	"fmt"
	"testing"
        
        // external library imports
	"github.com/stretchr/testify/require"
	abci "github.com/tendermint/tendermint/abci/types"
        
         // ibc-go library imports
	"github.com/cosmos/ibc-go/modules/core/23-commitment/types"
)

Fixed Show fixed Hide fixed

"github.com/cosmos/ibc-go/v4/modules/core/exported"
)
Expand Down Expand Up @@ -111,11 +111,15 @@ func (up *UpgradeProposal) ValidateBasic() error {
return sdkerrors.Wrap(ErrInvalidUpgradeProposal, "upgraded client state cannot be nil")
}

_, err := UnpackClientState(up.UpgradedClientState)
clientState, err := UnpackClientState(up.UpgradedClientState)
if err != nil {
return sdkerrors.Wrap(err, "failed to unpack upgraded client state")
}

if !reflect.DeepEqual(clientState, clientState.ZeroCustomFields()) {
return sdkerrors.Wrap(ErrInvalidUpgradeProposal, "upgraded client state is not zeroed out")
}

return nil
}

Expand Down
17 changes: 14 additions & 3 deletions modules/core/02-client/types/proposal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,20 +109,31 @@ func (suite *TypesTestSuite) TestUpgradeProposalValidateBasic() {
}{
{
"success", func() {
proposal, err = types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, plan, cs)
proposal, err = types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, plan, cs.ZeroCustomFields())
suite.Require().NoError(err)
}, true,
},
{
"fails validate abstract - empty title", func() {
proposal, err = types.NewUpgradeProposal("", ibctesting.Description, plan, cs)
proposal, err = types.NewUpgradeProposal("", ibctesting.Description, plan, cs.ZeroCustomFields())
Copy link
Contributor Author

@chatton chatton Jun 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

previous tests did not have zeroed out state.

suite.Require().NoError(err)
}, false,
},
{
"non zeroed fields", func() {
proposal, err = types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, plan, &ibctmtypes.ClientState{
FrozenHeight: types.Height{
RevisionHeight: 10,
},
})
suite.Require().NoError(err)

}, false,
},
{
"plan height is zero", func() {
invalidPlan := upgradetypes.Plan{Name: "ibc upgrade", Height: 0}
proposal, err = types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, invalidPlan, cs)
proposal, err = types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, invalidPlan, cs.ZeroCustomFields())
suite.Require().NoError(err)
}, false,
},
Expand Down