Provider panic cleanup #647

mpoke · 2023-01-06T18:28:01Z

Description

Audit of panic calls in x/provider

Linked issues

Works toward #621

Type of change

If you've checked more than one of the first three boxes, consider splitting this PR into multiple PRs!

Fix: Changes and/or adds code behavior, specifically to fix a bug
Refactor: Changes existing code style, naming, structure, etc.
Testing: Adds testing

Regression tests

Updated existing tests.

New behavior tests

TestRemoveConsumerFromUnbondingOp
new test cases for TestValidateGenesisState

…alUpdates

mpoke · 2023-01-06T18:51:48Z

x/ccv/provider/keeper/proposal.go still needs to be audit

mpoke · 2023-01-09T13:27:32Z

x/ccv/provider/types/genesis.go

+		return sdkerrors.Wrap(ccv.ErrInvalidGenesis, "unbonding operations cannot have an empty consumer chain list")
+	}
+
+	// Check that the ID is set correctly in the UnbondingOpsIndex


Checks that every unbonding op has the corresponding UnbondingOpsIndex set correctly.

mpoke · 2023-01-09T13:27:58Z

x/ccv/provider/keeper/relay.go

 					panic(fmt.Errorf("consumer chain failed to stop: %w", err))
 				}
 			}
 		}
 	}
 }

-func removeStringFromSlice(slice []string, x string) (newSlice []string, numRemoved int) {


Move to keeper.go

x/ccv/provider/keeper/relay.go

mpoke · 2023-01-09T13:30:28Z

x/ccv/provider/keeper/relay.go

@@ -210,6 +199,8 @@ func (k Keeper) SendVSCPacketsToChain(ctx sdk.Context, chainID, channelID string
 				k.Logger(ctx).Debug("IBC client is expired, cannot send VSC, leaving packet data stored:", "chainID", chainID, "vscid", data.ValsetUpdateId)
 				return
 			}
+			// TODO do not panic if the send fails


Opened issue to keep track of this.

mpoke · 2023-01-09T13:31:25Z

x/ccv/provider/keeper/proposal.go

 	k.DeleteKeyAssignments(ctx, chainID)

 	// close channel and delete the mappings between chain ID and channel ID
 	if channelID, found := k.GetChainToChannel(ctx, chainID); found {
 		if closeChan {
-			k.CloseChannel(ctx, channelID)


CloseChannel was only used here.

mpoke · 2023-01-09T13:32:06Z

x/ccv/provider/keeper/proposal_test.go

-		{
-			description: "fail due to an invalid unbonding index",
-			setup: func(ctx sdk.Context, providerKeeper *providerkeeper.Keeper, mocks testkeeper.MockedKeepers) {
-				// set invalid unbonding op index
-				providerKeeper.SetUnbondingOpIndex(ctx, "chainID", 0, []uint64{0})
-
-				// StopConsumerChain should return error, but state is still cleaned (asserted with mocks).
-				testkeeper.SetupForStoppingConsumerChain(t, ctx, providerKeeper, mocks)
-			},
-			expErr: true,
-		},


No longer needed.

…security into marius/panic-provider

danwt

Good!

Only nit is that the An error here would indicate something is very wrong lines above panics are redundant

x/ccv/provider/keeper/keeper.go

danwt · 2023-01-09T17:17:56Z

x/ccv/provider/keeper/relay.go

@@ -473,6 +463,10 @@ func (k Keeper) EndBlockCCR(ctx sdk.Context) {
 				"chainID", initTimeoutTimestamp.ChainId)
 			err := k.StopConsumerChain(ctx, initTimeoutTimestamp.ChainId, false)
 			if err != nil {
+				if ccv.ErrConsumerChainNotFound.Is(err) {
+					// consumer chain not found


It would be great to add here why this can happen

Only if the state is corrupted. The thing is that we shouldn't halt the chain in case this happens.

danwt · 2023-01-09T17:17:58Z

x/ccv/provider/keeper/relay.go

@@ -495,23 +490,17 @@ func (k Keeper) EndBlockCCR(ctx sdk.Context) {
 				)
 				err := k.StopConsumerChain(ctx, channelToChain.ChainId, true)
 				if err != nil {
+					if ccv.ErrConsumerChainNotFound.Is(err) {
+						// consumer chain not found


It would be great to add here why this can happen

x/ccv/provider/keeper/hooks.go

shaspitz

Great work! Just a few comments

x/ccv/provider/keeper/keeper.go

x/ccv/provider/keeper/relay.go

shaspitz

LGTM

shaspitz · 2023-01-10T16:25:50Z

x/ccv/provider/keeper/relay_test.go

+		require.True(t, found)
+		require.Equal(t, id, unbondingOp.Id)
+		require.Equal(t, expectedChains, unbondingOp.UnbondingConsumerChains)
+	}


All looks great

* Backport commits from main to v3 release branch (cosmos#682) * reorganize channel handshake handler (cosmos#647) * reorganize channel handshake handler split out channel state changes into its own function. * readjust 27-interchain-accounts to not rely on state being set before the application callback * add changelog and migration doc entry * Update modules/core/04-channel/keeper/handshake.go * docs: ICA Overview (cosmos#626) * docs: ica overview * fix: ordering * add spacing * fix: spacing * fix: remove bulletpoints * fix: wording * update go mod for security vulnerabilities (cosmos#655) * update go mod for security vulnerabilities * update package-lock.json * update vue dependency (cosmos#662) * bump glob-parent version in json package (cosmos#663) * build(deps): bump actions/setup-go from 2.1.4 to 2.1.5 (cosmos#656) Bumps [actions/setup-go](https://github.com/actions/setup-go) from 2.1.4 to 2.1.5. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@v2.1.4...v2.1.5) --- updated-dependencies: - dependency-name: actions/setup-go dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: colin axnér <[email protected]> * docs: begin removal of internal "spec" directories (cosmos#634) * begin removal of spec docs within core ibc * remove broken link * Apply suggestions from code review Co-authored-by: Damian Nolan <[email protected]> * remove broken link * remove broken links * Apply suggestions from code review Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> * Modify `OnChanOpenTry` application callback to perform app version negotitation (cosmos#646) * remove NegotiateAppVersion and AppVersion gRPC (cosmos#643) The NegotiateAppVersion callback has been removed from the IBC Application interface. The gRPC AppVersion has been removed. The app version negoitation will be handled by applications by returning the version in OnChanOpenTry. * Modify `OnChanOpenTry` to return application version (cosmos#650) * modify OnChanOpenTry to return negotiated version modify IBCModule interface function OnChanOpenTry to return the negotiated app version. Tests have not been updated * fix ibc_module_test.go tests * fix tests * Apply suggestions from code review * add handshake test case * add CHANGELOG and migration docs * update documentation * fix broken link * fix broken link (cosmos#664) * chore: update make build-docs, add docs build checker (cosmos#667) * update Makefile, add docs build checker * Update .github/workflows/check-docs.yml Co-authored-by: Marko <[email protected]> Co-authored-by: Marko <[email protected]> * register ICA query server, fix panics in params query cli (cosmos#666) Register the controller and host query servers to a chain. Returns an error upon cli params query failure instead of panicing. * update of roadmap with latest release (cosmos#653) * update of roadmap with latest release and changed the way release versions are encoded * fixed typo Co-authored-by: Carlos Rodriguez <[email protected]> * build(deps): bump actions/checkout from 2.3.1 to 2.4.0 (cosmos#672) Bumps [actions/checkout](https://github.com/actions/checkout) from 2.3.1 to 2.4.0. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v2.3.1...v2.4.0) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * allow ics20 to connect to middleware (cosmos#675) * allow ics20 to connect to middleware Creates ics4Wrapper which allows middleware applications to only implement SendPacket to connect ics20 as an application in its middleware stack * add changelog and migration doc * fix migration doc spelling * fix: register InterchainAccount as x/auth GenesisAccount (cosmos#676) * adding GenesisAccount interface registration for InterchainAccount impl * updating RegisterInterfaces ica godoc * enable mergify for backports (cosmos#678) Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: Sean King <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> * fixes evmos tests * cleanup * changelog and cleanup * pr suggestions. bump chainid Co-authored-by: colin axnér <[email protected]> Co-authored-by: Sean King <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: Federico Kunze Küllmer <[email protected]>

mpoke added 12 commits January 5, 2023 19:01

EndBlockCCR and StopConsumerChain

1396e56

audit genesis.go panics

80672f2

GetMaturedUnbondingOps panics instead of returning error

b91537e

completeMaturedUnbondingOps done

5ac5059

replace ApplyKeyAssignmentToValUpdates with MustApplyKeyAssignmentToV…

1f8a273

…alUpdates

panics in keeper - wip

1111cfe

add RemoveConsumerFromUnbondingOp

095d91d

add TODO for GenesisState validation

b724fa0

add ValidateUnbondingOp

86e038b

hooks panics

5a769b7

make sure ConsumerGenesis is serializable

07efa97

Merge branch 'main' into marius/panic-provider

0e7b2e6

mpoke added 3 commits January 9, 2023 14:06

panics in proposal.go

bfae75a

add TODO for failed VSCPacket sends

fea9f55

Merge branch 'main' into marius/panic-provider

bc54ae8

mpoke commented Jan 9, 2023

View reviewed changes

x/ccv/provider/keeper/relay.go Show resolved Hide resolved

mpoke commented Jan 9, 2023

View reviewed changes

mpoke marked this pull request as ready for review January 9, 2023 13:32

mpoke requested review from jtremback, sainoe, danwt and shaspitz as code owners January 9, 2023 13:32

mpoke added 2 commits January 9, 2023 14:34

fix linter

f8dda43

Merge branch 'marius/panic-provider' of github.com:cosmos/interchain-…

8faf05d

…security into marius/panic-provider

mpoke requested a review from MSalopek January 9, 2023 13:34

danwt approved these changes Jan 9, 2023

View reviewed changes

shaspitz reviewed Jan 9, 2023

View reviewed changes

x/ccv/provider/keeper/hooks.go Outdated Show resolved Hide resolved

remove redundant code

6bbc5b0

shaspitz reviewed Jan 9, 2023

View reviewed changes

x/ccv/provider/keeper/keeper.go Outdated Show resolved Hide resolved

x/ccv/provider/keeper/keeper.go Show resolved Hide resolved

x/ccv/provider/keeper/relay.go Show resolved Hide resolved

mpoke added 5 commits January 10, 2023 11:22

apply review suggestions

3b07af7

add TestHandleVSCMaturedPacket

9a97297

Merge branch 'main' into marius/panic-provider

af3f8db

fix TestRemoveConsumerFromUnbondingOp

5d2d936

fix linter

c74bc01

mpoke requested a review from shaspitz January 10, 2023 12:23

mpoke added 2 commits January 10, 2023 16:34

Merge branch 'main' into marius/panic-provider

b9fce3c

Merge branch 'main' into marius/panic-provider

1b1c8fa

mpoke mentioned this pull request Jan 10, 2023

Security feedback: Consider employing structured error handling #621

Closed

4 tasks

shaspitz approved these changes Jan 10, 2023

View reviewed changes

shaspitz merged commit c81d011 into main Jan 10, 2023

shaspitz deleted the marius/panic-provider branch January 10, 2023 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provider panic cleanup #647

Provider panic cleanup #647

mpoke commented Jan 6, 2023

mpoke commented Jan 6, 2023

mpoke Jan 9, 2023

mpoke Jan 9, 2023

mpoke Jan 9, 2023

mpoke Jan 9, 2023

mpoke Jan 9, 2023

danwt left a comment

danwt Jan 9, 2023

mpoke Jan 9, 2023

danwt Jan 9, 2023

mpoke Jan 9, 2023

shaspitz left a comment

shaspitz left a comment

shaspitz Jan 10, 2023

Provider panic cleanup #647

Provider panic cleanup #647

Conversation

mpoke commented Jan 6, 2023

Description

Linked issues

Type of change

Regression tests

New behavior tests

mpoke commented Jan 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaspitz left a comment

Choose a reason for hiding this comment

shaspitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment