feat!: upgrade ics to ibc-go/v7 and cosmos-sdk/v0.47

[MSalopek]

Closes: #942, #943, #944, #945, #946, #947

@faddat thank you for your contributions around this upgrade, all commits were retained.

Details

SDK related

1. StakingKeeper interface implementation was incorrect (now fixed)

• sdk v47 contains changes required for ICS
• SlashWithInfractionReason must be used instead of Slash

2. Issues with staking hooks AfterUnbondInitiated were not addressed

• issue was uncoveded and mitigated after fixing MBT testing
• MBT testing caught the inconsistency immediately
• PR was submitted to SDK to mitigate the issue (fix: call AfterUnbondingInitiated for new unbonding entries only cosmos-sdk#16043)

3. CancelUnbonding operation on cosmos-sdk v47 was not addressed

• since this was not supported, the provider chain can panic because state seems incorrect from ICS provider perspective

SDK & ibc-go/testing related

1. consumer chain initialization for integration and difftests was broken (now fixed)

• incorrect migration precluded effective use of the integration testing framework

All tests would panic with mainline ibc-go in this manner

--- FAIL: TestInitTimeout (0.04s)
panic: validator set is empty after InitGenesis, please ensure at least one validator is initialized with a delegation greater than or equal to the DefaultPowerReduction ({1374393344128}) [recovered]

The issue was fixed mitigated by correctly upgrading the consumer chain initialization procedure in legacy_ibc_testing/testing/app.go and legacy_ibc_testing/testing/chain.go

At this point, most of the features in legacy_ibc_testing were included in mainline ibc-go. These will be refactored out at a later time as part of QA plans.

2. consumer chain initialization for MBT (difftests) was broken (now fixed)

• incorrect migration precluded effective use of the MBT testing framework

Panic is the same as in case 1, steps to solve are slightly different.

panic: validator set is empty after InitGenesis, please ensure at least one validator is initialized with a delegation greater than or equal to the DefaultPowerReduction ({1374393344128}) [recovered]

3. app.go wiring

ICS can function with mainline ibc-go. Most changes were already included in versions prior to v7 (e.g. in v5)

The app wiring does not effect the library code used and it's main purpose is to be used in testing.

There were changes in the StakingKeeper interface expected by IBCKeeper.
Changes in the app initialization were needed to support the new constraints on the StakingKeeper during initialization.

Running a chain

e2e tests were broken and the existing apps don't seem to run properly, even thought a chain binary can be compiled without issues. It is unclear if the binary is able to function properly.

One of the issues that arose is the inability to collect genesis transactions.

With the current work, the chain binary is unable to execute collect-gentxs correctly, resulting in a panic:

startChain: + interchain-security-pd collect-gentxs --home /provi/validatorbob
startChain: panic: runtime error: invalid memory address or nil pointer dereference
startChain: [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1305688]
startChain:
startChain: goroutine 1 [running]:
startChain: github.com/cosmos/cosmos-sdk/x/genutil/types.ValidateAndGetGenTx({0x40002eea00, 0x489, 0x48a}, 0x1a4afa0?, 0x0)
startChain: 	/go/pkg/mod/github.com/notional-labs/[email protected]/x/genutil/types/genesis_state.go:130 +0xd8
startChain: github.com/cosmos/cosmos-sdk/x/genutil.CollectTxs({0x2a00310, 0x400017f6e0}, 0x1500000?, {0x400036bb70, 0xc}, {0x4000b88b80, 0x20}, {{0x1efbb7e2, 0xedbddc8dd, 0x0}, ...}, ...)
startChain: 	/go/pkg/mod/github.com/notional-labs/[email protected]/x/genutil/collect.go:114 +0x360
startChain: github.com/cosmos/cosmos-sdk/x/genutil.GenAppStateFromConfig({0x2a00310, 0x400017f6e0}, {0xffff74b82bb8, 0x400053fa80}, 0x4000bd8580, {{0x4000b87fc9, 0x5}, {0x4000b88b80, 0x20}, {0x4000b8dd70, ...}, ...}, ...)
startChain: 	/go/pkg/mod/github.com/notional-labs/[email protected]/x/genutil/collect.go:32 +0x110
startChain: github.com/cosmos/cosmos-sdk/x/genutil/client/cli.CollectGenTxsCmd.func1(0x4000acc900?, {0x400037c9a0?, 0x0?, 0x2?})
startChain: 	/go/pkg/mod/github.com/notional-labs/[email protected]/x/genutil/client/cli/collect.go:53 +0x404
startChain: github.com/spf13/cobra.(*Command).execute(0x4000acc900, {0x400037c980, 0x2, 0x2})
startChain: 	/go/pkg/mod/github.com/spf13/[email protected]/command.go:916 +0x5e0
startChain: github.com/spf13/cobra.(*Command).ExecuteC(0x4000acc300)
startChain: 	/go/pkg/mod/github.com/spf13/[email protected]/command.go:1044 +0x368
startChain: github.com/spf13/cobra.(*Command).Execute(...)
startChain: 	/go/pkg/mod/github.com/spf13/[email protected]/command.go:968
startChain: github.com/spf13/cobra.(*Command).ExecuteContext(...)
startChain: 	/go/pkg/mod/github.com/spf13/[email protected]/command.go:961
startChain: github.com/cosmos/cosmos-sdk/server/cmd.Execute(0x60?, {0x0, 0x0}, {0x4000289da0, 0x1c})
startChain: 	/go/pkg/mod/github.com/notional-labs/[email protected]/server/cmd/execute.go:32 +0x144
startChain: main.main()
startChain: 	/interchain-security/cmd/interchain-security-pd/main.go:15 +0x40

The same issue appears on both the fork and mainline sdk

+ interchain-security-pd collect-gentxs --home <user_dir>/provider-coordinator --gentx-dir <user_dir>provider-coordinator/config/gentx/
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x105cee878]

goroutine 1 [running]:
github.com/cosmos/cosmos-sdk/x/genutil/types.ValidateAndGetGenTx({0x1400058c000, 0x48a, 0x48b}, 0x106f06a20?, 0x0)
	<user_dir>go/pkg/mod/github.com/cosmos/[email protected]/x/genutil/types/genesis_state.go:130 +0xd8
github.com/cosmos/cosmos-sdk/x/genutil.CollectTxs({0x10736eb70, 0x1400032ad20}, 0x14000d9f448?, {0x14000cba2d0, 0xb}, {0x16b377746, 0x32}, {{0x1a870bd0, 0xedbe6cbc7, 0x0}, ...}, ...)
	<user_dir>go/pkg/mod/github.com/cosmos/[email protected]/x/genutil/collect.go:114 +0x2b8
github.com/cosmos/cosmos-sdk/x/genutil.GenAppStateFromConfig({0x10736eb70, 0x1400032ad20}, {0x11088bfb8, 0x14000a06900}, 0x14000c566e0, {{0x14000da23b8, 0x8}, {0x16b377746, 0x32}, {0x1400043ba10, ...}, ...}, ...)
	<user_dir>go/pkg/mod/github.com/cosmos/[email protected]/x/genutil/collect.go:32 +0x10c
github.com/cosmos/cosmos-sdk/x/genutil/client/cli.CollectGenTxsCmd.func1(0x14000bb6000?, {0x14000c0b6c0?, 0x0?, 0x4?})
	<user_dir>go/pkg/mod/github.com/cosmos/[email protected]/x/genutil/client/cli/collect.go:53 +0x414
github.com/spf13/cobra.(*Command).execute(0x14000bb6000, {0x14000c0b680, 0x4, 0x4})
	<user_dir>go/pkg/mod/github.com/spf13/[email protected]/command.go:916 +0x5c4
github.com/spf13/cobra.(*Command).ExecuteC(0x14000b3b800)
	<user_dir>go/pkg/mod/github.com/spf13/[email protected]/command.go:1044 +0x340
github.com/spf13/cobra.(*Command).Execute(...)
	<user_dir>go/pkg/mod/github.com/spf13/[email protected]/command.go:968
github.com/spf13/cobra.(*Command).ExecuteContext(...)
	<user_dir>go/pkg/mod/github.com/spf13/[email protected]/command.go:961
github.com/cosmos/cosmos-sdk/server/cmd.Execute(0x106321904?, {0x0, 0x0}, {0x140005b3350, 0x26})
	<user_dir>go/pkg/mod/github.com/cosmos/[email protected]/server/cmd/execute.go:32 +0x13c
main.main()
	<user_dir>informal/interchain-security/cmd/interchain-security-pd/main.go:15 +0x38

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 SDK 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

[mpoke]

Great work @MSalopek. Here are my comments from a partial review.

[mpoke]

The docstring for this panic is not accurate. Currently, AfterValidatorBonded is called by the slashing module and it only returns nil.

[MSalopek]

Changed: // AfterValidatorBonded is called by the Slashing module and should not return an error.

[faddat]

quick question:

I thought we needed that latest PR to the SDK?

I see that this passes all of the e2e tests, and that's super. But can anyone please explain why that's no longer needed?

• fix: call AfterUnbondingInitiated for new unbonding entries only cosmos-sdk#16043

Thanks

[faddat]

@mpoke would be neat if we got even a single review from you in the month and a half we have been working on this.

We'd been asking for that.

More ideal stewardship might look like:

• Remove spm #812 <- don't do this, give attribution when you use others' work
• review pull requests from outside informal, offer a hand
• don't make threats: ci: run golang tests gaia#2443
• communicate clearly: so, it did not need changes to the SDK after all? Just one?
• refactor: errors module gaia#2471 <- check what you're discussing first
• train newcomers, so that the cosmos footprint can grow

No one at Notional is under the illusion that we built ICS. Informal did that. What we did, consistently, was attempt to help with a high-priority matter.

I'm glad that this is completed, and I'm sad that it's been more or less the most dispiriting process imaginable.

[mpoke]

quick question:

I thought we needed that latest PR to the SDK?

I see that this passes all of the e2e tests, and that's super. But can anyone please explain why that's no longer needed?

• fix: call AfterUnbondingInitiated for new unbonding entries only cosmos-sdk#16043

Thanks

@faddat Please see #945 and fa543a8 for more context.

[shaspitz]

I've reviewed all files that I have relevant knowledge over, and added comments as needed. I don't think any of my comments should be very challenging to address. Thanks to everyone involved!

[shaspitz]

@MSalopek see 5a51545, tests pass locally for me. I'll approve this PR as most of my comments seem addressed.

The only comment left that I do think we should address is #918 (comment)

[shaspitz]

Reminder to add entry to changelog where appropriate

[mpoke]

Approval.
Great work @MSalopek.