Upgrade cosmos sdk to latest release. #1343

arijitAD · 2021-07-29T15:26:28Z

Description

Motivation and Context

I have raised an issue to propose this change (required)
My issue has received approval from the maintainers or lead with the design/approved label

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

hydrogen18 · 2021-07-29T15:40:52Z

I think we may have to backport all the bug fixes I made into the "latest" version of cosmos SDK again

arijitAD · 2021-08-02T15:46:03Z

hydrogen18 · 2021-08-02T16:55:19Z

For tendermint, the only changes I made are the following

diff --git a/rpc/client/http/http.go b/rpc/client/http/http.go
index 10fada8a6b..a825d9adec 100644
--- a/rpc/client/http/http.go
+++ b/rpc/client/http/http.go
@@ -737,15 +737,7 @@ func (w *WSEvents) eventListener() {
 
                        w.mtx.RLock()
                        if out, ok := w.subscriptions[result.Query]; ok {
-                               if cap(out) == 0 {
-                                       out <- *result
-                               } else {
-                                       select {
-                                       case out <- *result:
-                                       default:
-                                               w.Logger.Error("wanted to publish ResultEvent, but out channel is full", "result", result, "query", result.Query)
-                                       }
-                               }
+                               out <- *result
                        }
                        w.mtx.RUnlock()
                case <-w.Quit():
diff --git a/rpc/jsonrpc/client/ws_client.go b/rpc/jsonrpc/client/ws_client.go
index 1c7ade6578..d0cfd33a36 100644
--- a/rpc/jsonrpc/client/ws_client.go
+++ b/rpc/jsonrpc/client/ws_client.go
@@ -20,9 +20,9 @@ import (
 
 const (
        defaultMaxReconnectAttempts = 25
-       defaultWriteWait            = 0
-       defaultReadWait             = 0
-       defaultPingPeriod           = 0
+       defaultWriteWait            = 15 * time.Second
+       defaultReadWait             = 31 * time.Second
+       defaultPingPeriod           = 15 * time.Second
 )
 
 // WSClient is a JSON-RPC client, which uses WebSocket for communication with

The changes have not been made in the tendermint version you are targeting and needs to be backported by forking that tag and applying them in our repo.

hydrogen18 · 2021-08-02T17:04:18Z

For cosmos SDK, we made the following changes in our tag v0.41.4-akash-4

diff --git a/x/auth/legacy/v040/migrate.go b/x/auth/legacy/v040/migrate.go
index 363ec7ba82..f819a1a33d 100644
--- a/x/auth/legacy/v040/migrate.go
+++ b/x/auth/legacy/v040/migrate.go
@@ -2,6 +2,7 @@ package v040
 
 import (
        codectypes "github.com/cosmos/cosmos-sdk/codec/types"
+       multisigtypes "github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
        sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
        v039auth "github.com/cosmos/cosmos-sdk/x/auth/legacy/v039"
        v040auth "github.com/cosmos/cosmos-sdk/x/auth/types"
@@ -11,9 +12,16 @@ import (
 // convertBaseAccount converts a 0.39 BaseAccount to a 0.40 BaseAccount.
 func convertBaseAccount(old *v039auth.BaseAccount) *v040auth.BaseAccount {
        var any *codectypes.Any
-       // If the old genesis had a pubkey, we pack it inside an Any. Or else, we
-       // just leave it nil.
-       if old.PubKey != nil {
+
+       _, ok := old.PubKey.(*multisigtypes.LegacyAminoPubKey)
+
+       // If pubkey is multisig type, then leave it as nil for now
+       // Ref: https://github.com/cosmos/cosmos-sdk/issues/8776#issuecomment-790552126
+       // Else if the old genesis had a pubkey, we pack it inside an Any.
+       // Or else, we just leave it nil.
+       if ok {
+               // TODO migrate multisig public_keys
+       } else if old.PubKey != nil {
                var err error
                any, err = codectypes.NewAnyWithValue(old.PubKey)
                if err != nil {

This change needs to be backported

diff --git a/x/bank/keeper/keeper.go b/x/bank/keeper/keeper.go
index 1a7d435841..6a1b7fc8e7 100644
--- a/x/bank/keeper/keeper.go
+++ b/x/bank/keeper/keeper.go
@@ -236,7 +236,8 @@ func (k BaseKeeper) SetDenomMetaData(ctx sdk.Context, denomMetaData types.Metada
 }
 
 // SendCoinsFromModuleToAccount transfers coins from a ModuleAccount to an AccAddress.
-// It will panic if the module account does not exist.
+// It will panic if the module account does not exist. An error is returned if
+// the recipient address is black-listed or if sending the tokens fails.
 func (k BaseKeeper) SendCoinsFromModuleToAccount(
        ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins,
 ) error {
@@ -246,6 +247,10 @@ func (k BaseKeeper) SendCoinsFromModuleToAccount(
                panic(sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule))
        }
 
+       if k.BlockedAddr(recipientAddr) {
+               return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", recipientAddr)
+       }
+
        return k.SendCoins(ctx, senderAddr, recipientAddr, amt)
 }
 
@@ -387,6 +392,7 @@ func (k BaseKeeper) trackDelegation(ctx sdk.Context, addr sdk.AccAddress, blockT
        if ok {
                // TODO: return error on account.TrackDelegation
                vacc.TrackDelegation(blockTime, balance, amt)
+               k.ak.SetAccount(ctx, acc)
        }
 
        return nil
@@ -402,6 +408,7 @@ func (k BaseKeeper) trackUndelegation(ctx sdk.Context, addr sdk.AccAddress, amt
        if ok {
                // TODO: return error on account.TrackUndelegation
                vacc.TrackUndelegation(amt)
+               k.ak.SetAccount(ctx, acc)
        }
 
        return nil

This change has been partially applied in the new version. The changes to the functions trackDelegation & trackUndelegation need to be applied.

diff --git a/x/evidence/types/evidence.go b/x/evidence/types/evidence.go
index 95705cc5b9..fca6126ec3 100644
--- a/x/evidence/types/evidence.go
+++ b/x/evidence/types/evidence.go
@@ -88,7 +88,8 @@ func (e Equivocation) GetTotalPower() int64 { return 0 }
 // FromABCIEvidence converts a Tendermint concrete Evidence type to
 // SDK Evidence using Equivocation as the concrete type.
 func FromABCIEvidence(e abci.Evidence) exported.Evidence {
-       consAddr, err := sdk.Bech32ifyAddressBytes(sdk.Bech32PrefixConsAddr, e.Validator.Address)
+       bech32PrefixConsAddr := sdk.GetConfig().GetBech32ConsensusAddrPrefix()
+       consAddr, err := sdk.Bech32ifyAddressBytes(bech32PrefixConsAddr, e.Validator.Address)
        if err != nil {
                panic(err)
        }

This change has been applied and does not need to be backported

boz · 2021-08-02T17:08:30Z

x/auth/legacy/v040/migrate.go: we might as well migrate it, but it's not used anymore (we're past that migration)
x/bank/keeper/keeper.go: IIRC they've done some further refactoring, and this was fixed in another way (cosmos hub was upgraded for this security fix).

The tendermint fix we definitely want to carry forward if upstream hasn't merged it yet.

codecov · 2021-08-03T15:09:21Z

Codecov Report

Merging #1343 (5279545) into master (7965cb6) will decrease coverage by 0.28%.
The diff coverage is 43.49%.

@@            Coverage Diff             @@
##           master    #1343      +/-   ##
==========================================
- Coverage   42.77%   42.48%   -0.29%     
==========================================
  Files         257      272      +15     
  Lines       15192    15327     +135     
==========================================
+ Hits         6498     6512      +14     
- Misses       8001     8112     +111     
- Partials      693      703      +10

arijitAD · 2021-08-04T15:20:29Z

const (
	keyAddrPrefixLen = 1 + 20 // TODO: discuss and find a proper fix for this
)

https://github.com/ovrclk/akash/pull/1343/files#diff-d2cc399939acd1af527a3fed7af83beab3993932f4f6e12cd44c307e6cba014eR13

We need to correctly set x/cert/keeper/key.go:keyAddrPrefixLen which depended on the sdk.AddrLen which was fixed to 20 bytes previously, but can be variable length now (20 or 32).

We need to write migration for address if we are storing the address in a module similar to this code. https://github.com/cosmos/cosmos-sdk/blob/0ab5726a7aa5c2d66916b14becb696e6e19a1efb/x/bank/legacy/v043/store.go#L51

I suspect cert module might be storing the address. https://github.com/ovrclk/akash/blob/04e7a7667dd94b5a15fa039e4f7df5c9ad93be4f/x/cert/keeper/key.go#L42
Can someone verify? Also, if there are other modules using address?

Ref:

troian · 2021-08-04T15:50:03Z

@arijitAD yes, cert modules uses address and we need migration there.
it seems we need to add extra byte after prefix to identify address length (unless there is a univocal way identifying address length by first 20bytes) and migration should set it as 20bytes key for all existing certificates.

troian · 2021-08-04T15:54:26Z

@arijitAD does new address format have prefix defining address length in it?

arijitAD · 2021-08-04T15:56:26Z

@arijitAD does new address format have prefix defining address length in it?

From this migration, it seems that 1-byte prefix is allocated for length. https://github.com/cosmos/cosmos-sdk/blob/0ab5726a7aa5c2d66916b14becb696e6e19a1efb/x/bank/legacy/v043/store.go#L55

troian · 2021-08-04T16:00:35Z

i see, then we have to set it same way by adding extra byte for address type

boz · 2021-08-04T22:47:37Z

@arijitAD yes, cert modules uses address and we need migration there.
it seems we need to add extra byte after prefix to identify address length (unless there is a univocal way identifying address length by first 20bytes) and migration should set it as 20bytes key for all existing certificates.

Let's use type instead of length for the prefix if possible.

Also, if we're going to go and rewrite our indexes, we should use the binary/packed address instead of the base 16 ascii representation representation.

troian · 2021-08-05T00:13:54Z

@arijitAD yes, cert modules uses address and we need migration there.
it seems we need to add extra byte after prefix to identify address length (unless there is a univocal way identifying address length by first 20bytes) and migration should set it as 20bytes key for all existing certificates.

Let's use type instead of length for the prefix if possible.

Also, if we're going to go and rewrite our indexes, we should use the binary/packed address instead of the base 16 ascii representation representation.

yah, sorry, i meant to say extra byte as address type but was also curious if new address has any sort of meta information in it to determine it's length. it's not a case anyways and extra byte for type is only option

arijitAD · 2021-08-06T11:09:51Z

x/cert/keeper/keeper.go

-	iter := sdk.KVStorePrefixIterator(store, certificatePrefix(owner))
-	defer func() {
-		_ = iter.Close()
-	}()


The iter was declared but wasn't used. Seemed unnecessary, so removed.

troian · 2021-08-06T16:18:47Z

sdkutil/address.go

+// GetAccAddressFromBech32 creates an AccAddress from a Bech32 string.
+// It internally calls `sdk.AccAddressFromBech32` and ignores the error.
+func GetAccAddressFromBech32(address string) sdk.AccAddress {
+	addr, _ := sdk.AccAddressFromBech32(address)


should we set here panic if address is invalid?

I had done that initially, but, that causes TestCreateBidNonExistingOrder to fail here: https://github.com/ovrclk/akash/blob/cosmos-upgrade/x/market/handler/handler_test.go#L134 because it passes an empty string for order owner and provider address.

I guess, we can set those strings to a valid address that doesn't exist in the store to make the test pass.

troian · 2021-08-06T16:19:33Z

script/protoc-swagger-gen.sh

@@ -21,7 +21,7 @@ done
 # combine swagger files
 # uses nodejs package `swagger-combine`.
 # all the individual swagger files need to be configured in `config.json` for merging
-swagger-combine ./client/docs/config.json \
+.cache/node_modules/.bin/swagger-combine ./client/docs/config.json \


it should pick swagger-combine from PATH as we manually set it. doesn't it work?

simply running make codegen wasn't working locally for me without this change.

troian · 2021-08-06T16:22:45Z

x/cert/keeper/key.go

-
-var (
-	prefixCertificateID = []byte{0x01}
+	keyAddrPrefixLen = 1 /*len(PrefixCertificateID)*/ + 1 /*owner_address_len (1 byte)*/


something like that to make it more readable?

keyAddrPrefixLen = 2 // 1 byte for PrefixCertificateID) followed by owner_address_len byte

It is const. The diff shows var on the LHS, it is already const on the RHS.

boz

Looks great @arijitAD.

For the prefixes, I would really like to use functions that return new objects than to share mutable state if it is possible.

I believe that this will affect querying - possibly searching or desearializing indexes and data. Take a look at these queriers in the modules to be sure.

boz · 2021-08-08T00:48:17Z

util/legacy/v013/helpers.go

+		}
+
+		endBz := oldStoreIter.Key()[V012Bech32AddrLen:]
+		newStoreKey := append(append(prefixBz, address.MustLengthPrefix(addr)...), endBz...)


Interesting that they use just the length instead of a type (which could in also encode the length).

Yeah! they call these variable-length addresses. So, using length is a generic way to encode and be future proof if some new address of length say 40 gets introduced. The max length they have set for an address is 255, so well within a byte.

x/audit/types/key.go

boz · 2021-08-08T00:56:03Z

x/cert/keeper/key.go

-
-var (
-	prefixCertificateID = []byte{0x01}
+	keyAddrPrefixLen = 1 /*len(PrefixCertificateID)*/ + 1 /*owner_address_len (1 byte)*/


arijitAD · 2021-08-11T11:02:46Z

app/app.go

@@ -454,10 +452,46 @@ func NewApp(
 	return app
 }

+func (app *AkashApp) registerUpgradeHandlers() {
+	app.keeper.upgrade.SetUpgradeHandler("v0.43", func(ctx sdk.Context, plan upgradetypes.Plan,


Please suggest a suitable name for the upgrade handler. Currently, it is named v0.43 which is based on the cosmos version we are upgrading to. Should it be something like: v0.13 as that will be the version of the next akash release?
Or should it be something like: akashnet-2-upgrade-1 as it was previously?

what about v0.13.0_v0.43 where first part is akash version and 2nd is cosmos

Nice idea! Named it: akash_v0.13.0-cosmos_v0.43.0.

can we do it using this pattern akash_v0.13.0_cosmos_v0.43.0 as - is valid symbol in semantic versioning indicating it is prerelease

hydrogen18 · 2021-08-13T14:35:48Z

app/app.go

+	// there is nothing left over in the validator fee pool, so as to keep the
+	// CanWithdrawInvariant invariant.
+	// NOTE: staking module is required if HistoricalEntries param > 0
+	// NOTE: capability module's beginblocker must come before any modules using capabilities (e.g. IBC)


Did you find this information somewhere in the Cosmos docs or just figure it out from reading the code?

Took it from cosmos sdk code

Can you put a link to that in the source code? Someone else may want to look at that as well.

hydrogen18 · 2021-08-13T15:54:35Z

sdkutil/address.go

+import sdk "github.com/cosmos/cosmos-sdk/types"
+
+// MustAccAddressFromBech32 creates an AccAddress from a Bech32 string.
+// It internally calls `sdk.AccAddressFromBech32` and ignores the error.


I think this comment is wrong? It panics if there is an error.

Thanks, corrected.

sdkutil/broadcast.go

x/cert/keeper/key.go

boz

Looks great! I might have missed it, but I didn't see address migration for x/deployment?

boz · 2021-08-17T18:30:32Z

x/deployment/legacy/v013/store.go

+func MigrateStore(ctx sdk.Context, storeKey sdk.StoreKey) error {
+	store := ctx.KVStore(storeKey)
+	v013.MigratePrefixBech32AddrBytes(store, types.DeploymentPrefix())
+	v013.MigratePrefixBech32AddrBytes(store, types.GroupPrefix())


Do the addresses need to be migrated from ascii/hex to binary here?

That is what is being done by v013.MigratePrefixBech32AddrBytes(). It migrating addresses from the hex encoded bech32 string format to length prefixed binary format.

boz

🚀

arijitAD force-pushed the cosmos-upgrade branch from ad25044 to 6f918fb Compare August 3, 2021 15:02

arijitAD marked this pull request as ready for review August 3, 2021 15:38

arijitAD requested review from boz, hydrogen18 and troian as code owners August 3, 2021 15:38

arijitAD commented Aug 6, 2021

View reviewed changes

troian reviewed Aug 6, 2021

View reviewed changes

boz reviewed Aug 8, 2021

View reviewed changes

arijitAD requested review from boz and troian August 10, 2021 10:12

arijitAD commented Aug 11, 2021

View reviewed changes

hydrogen18 reviewed Aug 13, 2021

View reviewed changes

sdkutil/broadcast.go Show resolved Hide resolved

hydrogen18 reviewed Aug 13, 2021

View reviewed changes

x/cert/keeper/key.go Show resolved Hide resolved

boz reviewed Aug 17, 2021

View reviewed changes

boz approved these changes Aug 18, 2021

View reviewed changes

arijitAD added 9 commits August 19, 2021 16:05

Upgrade cosmos sdk to v0.43.0-rc2.

f65bec7

make codegen

ecc58af

upgrade modules

a0b72f7

fix test

5af4b23

review comments

a7a16db

upgrade to cosmos-sdk@v0.43.0

c3636f5

change upgrade handler name

d1d0094

update ibc-go and modvendor

bb08d5d

address review comments

5279545

arijitAD force-pushed the cosmos-upgrade branch from bcbac60 to 5279545 Compare August 19, 2021 10:35

arijitAD merged commit a7163d0 into master Aug 19, 2021

arijitAD deleted the cosmos-upgrade branch August 19, 2021 12:45

akhilkumarpilli mentioned this pull request Aug 21, 2021

feat: update SDK to v0.43.0 and add upgrade handler #1262

Closed

Upgrade cosmos sdk to latest release. #1343

Upgrade cosmos sdk to latest release. #1343

Conversation

arijitAD commented Jul 29, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

hydrogen18 commented Jul 29, 2021

arijitAD commented Aug 2, 2021

Akash: cosmos lib 0.43.0-rc2 upgrade

hydrogen18 commented Aug 2, 2021

hydrogen18 commented Aug 2, 2021

boz commented Aug 2, 2021 • edited Loading

codecov bot commented Aug 3, 2021 • edited Loading

Codecov Report

arijitAD commented Aug 4, 2021

troian commented Aug 4, 2021

troian commented Aug 4, 2021

arijitAD commented Aug 4, 2021

troian commented Aug 4, 2021

boz commented Aug 4, 2021

troian commented Aug 5, 2021

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

troian Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boz 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

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

boz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arijitAD Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

boz left a comment

Choose a reason for hiding this comment

arijitAD commented Jul 29, 2021 •

edited

Loading

boz commented Aug 2, 2021 •

edited

Loading

codecov bot commented Aug 3, 2021 •

edited

Loading

troian Aug 6, 2021 •

edited

Loading

arijitAD Aug 18, 2021 •

edited

Loading