[CORE-538] Upgrade to Cosmos 0.50.1 and CometBFT 0.38 #867

lcwik · 2023-12-08T19:31:37Z

Changelist

Update to Cosmos 0.50.1 and CometBFT 0.38

Test Plan

Updated existing tests, some tests have been skipped or partially disabled with TODO tags added.

Author/Reviewer Checklist

If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
Manually add any of the following labels: refactor, chore, bug.

coderabbitai · 2023-12-08T19:31:42Z

Important

Auto Review Skipped

More than 25% of the files skipped due to max files limit. Skipping review to prevent low quality review.

214 files out of 296 files are above the max files limit of 75.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit's AI:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit-tests for this file.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository from git and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid.
- @coderabbitai read the files in the src/scheduler package and generate README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

linear · 2023-12-08T19:31:55Z

CORE-538 Upgrade to Cosmos 0.50.0 (when available) and CometBFT 0.38.0

lcwik · 2023-12-15T21:28:53Z

protocol/app/ante/sigverify.go

 )

-// Verify all signatures for a tx and return an error if any are invalid. Note,
+// SigVerificationDecorator verifies all signatures for a tx and return an error if any are invalid. Note,


This was updated with the copy from Cosmo 0.50 with the SkipSequenceValidation changes ported over.

lcwik · 2023-12-15T21:30:12Z

protocol/app/ante/sigverify_test.go

@@ -21,6 +29,28 @@ import (

 func TestSigVerification(t *testing.T) {


This was also copied over from Cosmos 0.50 with skip sequence validation tests ported over.

lcwik · 2023-12-15T21:43:12Z

protocol/app/msgs/all_msgs.go

@@ -48,6 +48,8 @@ var (
 		"/cosmos.distribution.v1beta1.CommunityPoolSpendProposal":             {},


Please double check that I grouped the new messages into the appropriate categories.

lcwik · 2023-12-15T21:48:08Z

protocol/app/testdata/default_genesis_state.json

@@ -262,10 +261,22 @@
      "threshold": "0.500000000000000000",
      "veto_threshold": "0.334000000000000000",
      "min_initial_deposit_ratio": "0.000000000000000000",
+      "proposal_cancel_ratio": "0.500000000000000000",


Check and decide on new parameters, specifically governance.

I think we should get product (Corey) and potentially research team involved if we are writing migration logic for this

To match the suggested staging/prod value, can you use the following?

Suggested change

"proposal_cancel_ratio": "0.500000000000000000",

"proposal_cancel_ratio": "1.0"

"proposal_cancel_dest": "",

"expedited_voting_period": "86400s",

"expedited_threshold": "0.75",

"expedited_min_deposit": [

{

"denom": "stake",

"amount": "50000000"

}

],

"burn_vote_quorum": false,

"burn_proposal_deposit_prevote": false,

"burn_vote_veto": true,

"min_deposit_ratio": "0.20"

},

"constitution": ""

},

lcwik · 2023-12-15T21:51:04Z

protocol/cmd/dydxprotocold/cmd/config.go

@@ -64,7 +64,6 @@ func initAppConfig() (string, *DydxAppConfig) {

 	// GRPC.
 	appConfig.GRPC.Address = "0.0.0.0:9090"
-	appConfig.GRPCWeb.Address = "0.0.0.0:9091"


This now runs under GRPC address with Cosmos 0.50

lcwik · 2023-12-15T21:51:22Z

protocol/cmd/dydxprotocold/cmd/config.go

@@ -84,7 +83,6 @@ func initTendermintConfig() *tmcfg.Config {
 	cfg.RPC.CORSAllowedOrigins = []string{"*"}

 	// Mempool config.
-	cfg.Mempool.Version = "v1"


Mempool v1 is now gone, TTL changes were backported to v0

lcwik · 2023-12-15T21:59:05Z

protocol/testutil/ante/testutil.go

 	FeeGrantKeeper *antetestutil.MockFeegrantKeeper
 	EncCfg         moduletestutil.TestEncodingConfig
 }

-// SetupTest setups a new test, with new app, context, and AnteHandler.


Updated from Cosmos 0.50 with v4 specific changes.

lcwik · 2023-12-15T22:17:05Z

protocol/testutil/constants/messages.go

@@ -68,6 +68,16 @@ var (
 			Amount:    500_000_000, // $500
 		},
 	}
+	Msg_Transfer_Invalid_SameSenderAndRecipient = &sendingtypes.MsgCreateTransfer{


ValidateBasic is now expected to not be implemented and users should be simulating their transactions before submitting them which is why I swapped to a message where we control the message type explicitly.

See cosmos/cosmos-sdk#18750

protocol/x/assets/keeper/asset.go

jayy04 · 2023-12-18T21:23:00Z

protocol/x/clob/abci.go

+	// TODO(CORE-538): Is there a cleaner way to check to see if this is genesis?
+	if ctx.BlockHeight() == 0 || ctx.BlockHeight() == 1 {
+		return
+	}


for my understanding - why do we need to do this?

The issue that I ran into is for the first block is that there is no ProcessProposerMatches events during PrepareCheckState. Cosmos 0.47 didn't run PrepareCheckState for the genesis block while Cosmos 0.50 does.

So I keep the original behavior by skipping PrepareCheckState for the genesis block.

protocol/app/ante/sigverify_test.go

protocol/app/app.go

ttl33 · 2023-12-19T11:35:14Z

protocol/app/process/other_msgs.go

@@ -59,8 +59,10 @@ func DecodeOtherMsgsTx(decoder sdk.TxDecoder, txBytes []byte) (*OtherMsgsTx, err
 // Validate returns an error if one of the underlying msgs fails `ValidateBasic`.
 func (omt *OtherMsgsTx) Validate() error {
 	for _, msg := range omt.msgs {
-		if err := msg.ValidateBasic(); err != nil {
-			return getValidateBasicError(msg, err)
+		if m, ok := msg.(sdk.HasValidateBasic); ok {


When do we NOT expect HasValidateBasic to be the case?

Should we handle else case?

Many of the Cosmos SDK messages have migrated away from using ValidateBasic which I filed cosmos/cosmos-sdk#18750 about. Cosmos SDK wants users to simulate their transactions to validate them instead of relying on ValidateBasic during CheckTx which is a change in expected user behavior going forward.

This is one of the points I brought up that we want to discuss in slack since this has ramifications to user experience.

Blocking comment: what's the conclusion of this discussion?

The conclusion was to simulate the transaction.

Gotcha, nit: would you be able to add a comment explaining this?

protocol/app/process/process_proposal_test.go

ttl33 · 2023-12-19T11:38:20Z

protocol/app/testdata/default_genesis_state.json

@@ -262,10 +261,22 @@
      "threshold": "0.500000000000000000",
      "veto_threshold": "0.334000000000000000",
      "min_initial_deposit_ratio": "0.000000000000000000",
+      "proposal_cancel_ratio": "0.500000000000000000",


I think we should get product (Corey) and potentially research team involved if we are writing migration logic for this

protocol/app/testdata/default_genesis_state.json

ttl33 · 2023-12-19T11:40:31Z

protocol/app/upgrades/v3.0.0/upgrade.go

@@ -98,9 +99,10 @@ func CreateUpgradeHandler(
 	configurator module.Configurator,
 	ak authkeeper.AccountKeeper,
 ) upgradetypes.UpgradeHandler {


as mentioned above, should we add the logic to init the new gov states? (i.e. expedited_voting_period)

Can also add a ticket in appropriate P0 project. I'm following up on the default values here

teddyding · 2023-12-19T14:33:14Z

proto/dydxprotocol/sending/transfer.proto

@@ -48,6 +48,8 @@ message MsgDepositToSubaccount {
 // MsgWithdrawFromSubaccount represents a single transfer from an
 // `x/subaccounts` subaccount to an `x/bank` account.
 message MsgWithdrawFromSubaccount {
+  option (cosmos.msg.v1.signer) = "sender";


I understand that we are specifying the sender should be sender.owner in CustomGetSigner here.

Should we remove this option, given that sender is not a valid signer (i.e. it's not an address string)?

The cosmos SDK was meant to support these recursively but didn't do it quite right since sender is supposed to go into that message and then use owner to find the appropriate address string. I filed cosmos/cosmos-sdk#18722 and they have fixed this but it won't be available till a future Cosmos SDK release.

I'd like to leave this for now and once able we can remove the CustomGetSigners implementation.

…osmos.network/v0.50/build/migrations/upgrading#xgenutil

…t doesn't impact consensus parameters as per cosmos upgrade guide https://docs.cosmos.network/v0.50/build/migrations/upgrading#set-preblocker

…mismatches.

…ller changes.

teddyding · 2024-01-03T16:49:41Z

protocol/app/app.go

@@ -245,6 +248,7 @@ type App struct {
 	ParamsKeeper     paramskeeper.Keeper
 	// IBC Keeper must be a pointer in the app, so we can SetRouter on it correctly
 	IBCKeeper             *ibckeeper.Keeper
+	ICAControllerKeeper   icacontrollerkeeper.Keeper


Why are we adding ICAControllerKeeper, for my understanding? For supporting interchain accounts we specifically wanted to avoid the controllerkeeper (context).

The ICAControllerKeeper is still disabled since Enabled is false in the upgrade (note the Enabled field is taking on the default value).

So instead of it being hard-coded disabled it is being disabled via parameters.

The simulation in IBC v8 expects that the controller module to exist otherwise the update params calls it generates fail with them being unroutable causing the sim tests to fail.

Instead of going with this approach, is it possible to simply exclude ICA module from simulation testing? Empirically, simulation testing is not very high signal for the dYdX chain, let alone a third-party module that is tested elsewhere.

We specifically preferred to explicitly disable (in code, instead of through gov-updatable params) the controller sub-module altogether to reduce surface of risk. This also eliminates the potential bugs in upgrade handler.

Simulation testing alone does not justify altering the original approach, IMO. Lmk if the above is possible and happy to huddle!

I swapped to exclude the interchainaccounts module from simulation as suggested.

…e default 10m.

protocol/app/app.go

protocol/app/msgs/internal_msgs.go

protocol/app/msgs/unsupported_msgs.go

protocol/app/msgs/normal_msgs.go

ttl33 · 2024-01-04T14:50:35Z

protocol/app/process/other_msgs.go

@@ -59,8 +59,10 @@ func DecodeOtherMsgsTx(decoder sdk.TxDecoder, txBytes []byte) (*OtherMsgsTx, err
 // Validate returns an error if one of the underlying msgs fails `ValidateBasic`.
 func (omt *OtherMsgsTx) Validate() error {
 	for _, msg := range omt.msgs {
-		if err := msg.ValidateBasic(); err != nil {
-			return getValidateBasicError(msg, err)
+		if m, ok := msg.(sdk.HasValidateBasic); ok {


Blocking comment: what's the conclusion of this discussion?

protocol/app/module/interface_registry.go

Co-authored-by: ttl33 <[email protected]>

lcwik · 2024-01-04T16:40:50Z

@ttl33 I couldn't comment on some of your comments for some reason so I commented here.

Would merging this PR make the main branch to be in a broken state? For example, if the route for the upgrade is not > registered, then upgrade via gov wouldn't work.

If there are no easy ways to resolve this in this PR, then I am okay. Just wanted to call out that we wanted to keep main always deployable and releasable.

We should be upgrading from the release branch with patches and backporting any fixes to it that need to be released immediately. There is no easy way to resolve this without implementing even more and this PR has been delayed due to RC0 and peoples vacation schedules so I want to get it in finally and not waste more time rebasing constantly.

Blocking comment: what's the conclusion of this discussion?

To rely on simulating transactions since CheckTx only does some of the work to validate things.

For MsgRecoverClient: yes, I think we should keep this in normal. This is because anyone should be able to submit the gov proposal to recover a client. See doc/comments here

For MsgIBCSoftwareUpgrade, yes, I also think we should keep this in normal. This is because it's replacing the MsgUpgradeClient which was the proposal submitted by users to upgrade.

Should we then move MsgUpdateClient and MsgUpgradeClient (note update vs upgrade here) to unsupported ones?

I added a TODO to handle this once I figure out how to get the upgrades done.

ttl33 · 2024-01-04T18:10:09Z

protocol/app/process/other_msgs.go

@@ -59,8 +59,10 @@ func DecodeOtherMsgsTx(decoder sdk.TxDecoder, txBytes []byte) (*OtherMsgsTx, err
 // Validate returns an error if one of the underlying msgs fails `ValidateBasic`.
 func (omt *OtherMsgsTx) Validate() error {
 	for _, msg := range omt.msgs {
-		if err := msg.ValidateBasic(); err != nil {
-			return getValidateBasicError(msg, err)
+		if m, ok := msg.(sdk.HasValidateBasic); ok {


Gotcha, nit: would you be able to add a comment explaining this?

ttl33 · 2024-01-04T18:15:07Z

protocol/app/testdata/default_genesis_state.json

@@ -262,10 +261,22 @@
      "threshold": "0.500000000000000000",
      "veto_threshold": "0.334000000000000000",
      "min_initial_deposit_ratio": "0.000000000000000000",
+      "proposal_cancel_ratio": "0.500000000000000000",


To match the suggested staging/prod value, can you use the following?

Suggested change

"proposal_cancel_ratio": "0.500000000000000000",

"proposal_cancel_ratio": "1.0"

"proposal_cancel_dest": "",

"expedited_voting_period": "86400s",

"expedited_threshold": "0.75",

"expedited_min_deposit": [

{

"denom": "stake",

"amount": "50000000"

}

],

"burn_vote_quorum": false,

"burn_proposal_deposit_prevote": false,

"burn_vote_veto": true,

"min_deposit_ratio": "0.20"

},

"constitution": ""

},

github-actions bot added the protocol label Dec 8, 2023

lcwik changed the title ~~Lukecore538~~ [CORE-538] Upgrade to Cosmos 0.50 and CometBFT 0.38 Dec 8, 2023

lcwik force-pushed the lukecore538 branch from f44455e to 417ebca Compare December 8, 2023 21:30

github-actions bot added the proto label Dec 13, 2023

lcwik force-pushed the lukecore538 branch 11 times, most recently from 657e07a to 9c1980a Compare December 15, 2023 22:41

lcwik commented Dec 15, 2023

View reviewed changes

lcwik marked this pull request as ready for review December 15, 2023 22:50

lcwik changed the title ~~[CORE-538] Upgrade to Cosmos 0.50 and CometBFT 0.38~~ [CORE-538] Upgrade to Cosmos 0.50.1 and CometBFT 0.38 Dec 15, 2023

lcwik force-pushed the lukecore538 branch 3 times, most recently from d0c76b8 to 89ebd37 Compare December 18, 2023 19:44

jayy04 reviewed Dec 18, 2023

View reviewed changes

protocol/x/assets/keeper/asset.go Outdated Show resolved Hide resolved

jayy04 reviewed Dec 18, 2023

View reviewed changes

lcwik force-pushed the lukecore538 branch from 89ebd37 to 8c1a076 Compare December 18, 2023 21:39

jayy04 approved these changes Dec 18, 2023

View reviewed changes

lcwik force-pushed the lukecore538 branch from 8c1a076 to 7aafae6 Compare December 18, 2023 22:19

ttl33 reviewed Dec 19, 2023

View reviewed changes

teddyding reviewed Dec 19, 2023

View reviewed changes

lcwik added 8 commits January 2, 2024 11:42

Swap to AppGenesisFromFile as per Cosmos upgrade guide https://docs.c…

f23cd65

…osmos.network/v0.50/build/migrations/upgrading#xgenutil

Remove TODO since blocktime makes sense to do in BeginBlocker since i…

76ec8d1

…t doesn't impact consensus parameters as per cosmos upgrade guide https://docs.cosmos.network/v0.50/build/migrations/upgrading#set-preblocker

Re-order imports

b4dd0dd

Fix typo

68d242c

Address Taehoon's PR comments.

fff691f

Resolve/update minor TODOs.

8c1a3f3

Address Roy's comments.

9656edc

Update message types, improve testapp logging/debugging for app hash …

bdd0719

…mismatches.

lcwik force-pushed the lukecore538 branch 2 times, most recently from e6c0d19 to ab6e05a Compare January 2, 2024 19:57

lcwik added 3 commits January 2, 2024 13:04

Rebase updates that required changes including ICA message and contro…

4078041

…ller changes.

Address Taehoon's comments wrt to message types.

208f41b

Fix lint

0328e71

lcwik force-pushed the lukecore538 branch from ab6e05a to 0328e71 Compare January 2, 2024 21:04

lcwik added 2 commits January 3, 2024 08:45

Skip upgrade test till upgrade handler has been updated for 4.0.0

81d7da8

Fix data race in test code

069ee8f

lcwik force-pushed the lukecore538 branch from d32e97e to 069ee8f Compare January 3, 2024 16:45

teddyding reviewed Jan 3, 2024

View reviewed changes

lcwik added 2 commits January 3, 2024 09:56

Increase timeout since some suites of tests are taking longer then th…

61fbda5

…e default 10m.

Remove the ICA controller and drop ICA from simulation

47ecfd6

teddyding approved these changes Jan 4, 2024

View reviewed changes

ttl33 reviewed Jan 4, 2024

View reviewed changes

Update protocol/app/module/interface_registry.go

857a768

Co-authored-by: ttl33 <[email protected]>

Address Taehoon's comments.

131a772

ttl33 approved these changes Jan 4, 2024

View reviewed changes

lcwik merged commit f08e61a into main Jan 4, 2024
23 of 24 checks passed

lcwik deleted the lukecore538 branch January 4, 2024 18:22

lcwik mentioned this pull request Jan 4, 2024

[CORE-538] Address nit and fix Cosmos 0.50 post merge conflict #922

Merged

6 tasks

czarcas7ic mentioned this pull request May 20, 2024

feat: sdk v0.50.x upgrade osmosis-labs/osmosis#8274

Merged

8 tasks

		@@ -21,6 +29,28 @@ import (

		func TestSigVerification(t *testing.T) {

		@@ -48,6 +48,8 @@ var (
		"/cosmos.distribution.v1beta1.CommunityPoolSpendProposal": {},

-      "proposal_cancel_ratio": "0.500000000000000000",
+      "proposal_cancel_ratio": "1.0"
+      "proposal_cancel_dest": "",
+      "expedited_voting_period": "86400s",
+      "expedited_threshold": "0.75",
+      "expedited_min_deposit": [
+        {
+          "denom": "stake",
+          "amount": "50000000"
+        }
+      ],
+      "burn_vote_quorum": false,
+      "burn_proposal_deposit_prevote": false,
+      "burn_vote_veto": true,
+      "min_deposit_ratio": "0.20"
+    },
+    "constitution": ""
+  },

[CORE-538] Upgrade to Cosmos 0.50.1 and CometBFT 0.38 #867

[CORE-538] Upgrade to Cosmos 0.50.1 and CometBFT 0.38 #867

Conversation

lcwik commented Dec 8, 2023 • edited Loading

Changelist

Test Plan

Author/Reviewer Checklist

coderabbitai bot commented Dec 8, 2023 • edited Loading

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

linear bot commented Dec 8, 2023

lcwik Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

lcwik Dec 15, 2023 • edited Loading

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

Choose a reason for hiding this comment

lcwik Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

lcwik Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

lcwik Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcwik Dec 18, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcwik Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

teddyding Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcwik commented Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcwik commented Dec 8, 2023 •

edited

Loading

coderabbitai bot commented Dec 8, 2023 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

lcwik Dec 15, 2023 •

edited

Loading

lcwik Dec 15, 2023 •

edited

Loading

lcwik Dec 15, 2023 •

edited

Loading

lcwik Dec 15, 2023 •

edited

Loading

lcwik Dec 15, 2023 •

edited

Loading

lcwik Dec 18, 2023 •

edited

Loading

lcwik Jan 3, 2024 •

edited

Loading

teddyding Jan 3, 2024 •

edited

Loading

lcwik commented Jan 4, 2024 •

edited

Loading