From 1f06f5bec67e76843ceb00502a814b2a87e81d6b Mon Sep 17 00:00:00 2001 From: yihuang Date: Mon, 20 May 2024 17:13:42 +0800 Subject: [PATCH 1/5] fix: nil pointer panic when store don't exists in historical version (#20425) --- store/CHANGELOG.md | 6 ++++++ store/rootmulti/store.go | 4 ++++ store/rootmulti/store_test.go | 9 ++++++--- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/store/CHANGELOG.md b/store/CHANGELOG.md index cdd7bb1c59fa..84b2a0d35eb3 100644 --- a/store/CHANGELOG.md +++ b/store/CHANGELOG.md @@ -23,6 +23,12 @@ Ref: https://keepachangelog.com/en/1.0.0/ # Changelog +## [Unreleased] + +### Bug Fixes + +* (store) [#20425](https://github.com/cosmos/cosmos-sdk/pull/20425) Fix nil pointer panic when query historical state where a new store don't exist. + ## v1.1.0 (March 20, 2024) ### Improvements diff --git a/store/rootmulti/store.go b/store/rootmulti/store.go index cfcc5f47b8b2..dcd418ccb09c 100644 --- a/store/rootmulti/store.go +++ b/store/rootmulti/store.go @@ -607,6 +607,10 @@ func (rs *Store) CacheMultiStoreWithVersion(version int64) (types.CacheMultiStor if storeInfos[key.Name()] { return nil, err } + + // If the store donesn't exist at this version, create a dummy one to prevent + // nil pointer panic in newer query APIs. + cacheStore = dbadapter.Store{DB: dbm.NewMemDB()} } default: diff --git a/store/rootmulti/store_test.go b/store/rootmulti/store_test.go index 2702f3e08623..ebdd657de41f 100644 --- a/store/rootmulti/store_test.go +++ b/store/rootmulti/store_test.go @@ -100,16 +100,19 @@ func TestCacheMultiStoreWithVersion(t *testing.T) { require.Equal(t, kvStore.Get(k), v) // add new module stores (store4 and store5) to multi stores and commit - ms.MountStoreWithDB(types.NewKVStoreKey("store4"), types.StoreTypeIAVL, nil) - ms.MountStoreWithDB(types.NewKVStoreKey("store5"), types.StoreTypeIAVL, nil) + key4, key5 := types.NewKVStoreKey("store4"), types.NewKVStoreKey("store5") + ms.MountStoreWithDB(key4, types.StoreTypeIAVL, nil) + ms.MountStoreWithDB(key5, types.StoreTypeIAVL, nil) err = ms.LoadLatestVersionAndUpgrade(&types.StoreUpgrades{Added: []string{"store4", "store5"}}) require.NoError(t, err) ms.Commit() // cache multistore of version before adding store4 should works - _, err = ms.CacheMultiStoreWithVersion(1) + cms2, err := ms.CacheMultiStoreWithVersion(1) require.NoError(t, err) + require.Empty(t, cms2.GetKVStore(key4).Get([]byte("key"))) + // require we cannot commit (write) to a cache-versioned multi-store require.Panics(t, func() { kvStore.Set(k, []byte("newValue")) From c2f6c19aaf3e8caaea3a19968c3e9fdc0f0721c6 Mon Sep 17 00:00:00 2001 From: Marko Date: Tue, 21 May 2024 10:02:11 +0200 Subject: [PATCH 2/5] revert: bank change module to account change (#20427) --- store/cache/cache.go | 2 +- store/rootmulti/store.go | 6 +++--- tests/e2e/accounts/base_account_test.go | 1 - testutil/testdata/grpc_query.go | 3 ++- x/bank/CHANGELOG.md | 1 - x/bank/keeper/keeper.go | 7 ------- x/bank/keeper/keeper_test.go | 18 ------------------ 7 files changed, 6 insertions(+), 32 deletions(-) diff --git a/store/cache/cache.go b/store/cache/cache.go index 98d17d0341ac..748eae8c422b 100644 --- a/store/cache/cache.go +++ b/store/cache/cache.go @@ -43,7 +43,7 @@ type ( func NewCommitKVStoreCache(store types.CommitKVStore, size uint) *CommitKVStoreCache { cache, err := lru.NewARC(int(size)) if err != nil { - panic(fmt.Errorf("failed to create KVStore cache: %s", err)) + panic(fmt.Errorf("failed to create KVStore cache: %w", err)) } return &CommitKVStoreCache{ diff --git a/store/rootmulti/store.go b/store/rootmulti/store.go index dcd418ccb09c..33589123bcd0 100644 --- a/store/rootmulti/store.go +++ b/store/rootmulti/store.go @@ -5,7 +5,7 @@ import ( "errors" "fmt" "io" - "math" + "math" "sort" "strings" "sync" @@ -834,7 +834,7 @@ func (rs *Store) Snapshot(height uint64, protoWriter protoio.Writer) error { keys := keysFromStoreKeyMap(rs.stores) for _, key := range keys { switch store := rs.GetCommitKVStore(key).(type) { - case *iavl.Store: + case *iavl.Store: stores = append(stores, namedStore{name: key.Name(), Store: store}) case *transient.Store, *mem.Store: // Non-persisted stores shouldn't be snapshotted @@ -854,7 +854,7 @@ func (rs *Store) Snapshot(height uint64, protoWriter protoio.Writer) error { // are demarcated by new SnapshotStore items. for _, store := range stores { rs.logger.Debug("starting snapshot", "store", store.name, "height", height) - exporter, err := store.Export(int64(height)) + exporter, err := store.Export(int64(height)) if err != nil { rs.logger.Error("snapshot failed; exporter error", "store", store.name, "err", err) return err diff --git a/tests/e2e/accounts/base_account_test.go b/tests/e2e/accounts/base_account_test.go index 60292a3c52dd..64a6dff9a50a 100644 --- a/tests/e2e/accounts/base_account_test.go +++ b/tests/e2e/accounts/base_account_test.go @@ -80,5 +80,4 @@ func bechify(t *testing.T, app *simapp.SimApp, addr []byte) string { func fundAccount(t *testing.T, app *simapp.SimApp, ctx sdk.Context, addr sdk.AccAddress, amt string) { require.NoError(t, testutil.FundAccount(ctx, app.BankKeeper, addr, coins(t, amt))) - } diff --git a/testutil/testdata/grpc_query.go b/testutil/testdata/grpc_query.go index 1e5ae1830de9..1078f60b8b81 100644 --- a/testutil/testdata/grpc_query.go +++ b/testutil/testdata/grpc_query.go @@ -3,9 +3,10 @@ package testdata import ( "context" "fmt" - "github.com/cosmos/gogoproto/types/any/test" "testing" + "github.com/cosmos/gogoproto/types/any/test" + "github.com/cosmos/gogoproto/proto" "google.golang.org/grpc" "gotest.tools/v3/assert" diff --git a/x/bank/CHANGELOG.md b/x/bank/CHANGELOG.md index 6f298085ee62..9d5a91718557 100644 --- a/x/bank/CHANGELOG.md +++ b/x/bank/CHANGELOG.md @@ -49,4 +49,3 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Consensus Breaking Changes * [#19188](https://github.com/cosmos/cosmos-sdk/pull/19188) Remove creation of `BaseAccount` when sending a message to an account that does not exist -* [#20343](https://github.com/cosmos/cosmos-sdk/pull/20343) Add a check in send moduleaccount to account to prevent module accounts from sending disabled tokens to accounts diff --git a/x/bank/keeper/keeper.go b/x/bank/keeper/keeper.go index 3c6ade1c235d..c8f9e5ba7bca 100644 --- a/x/bank/keeper/keeper.go +++ b/x/bank/keeper/keeper.go @@ -269,13 +269,6 @@ func (k BaseKeeper) SendCoinsFromModuleToAccount( return errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule) } - for _, coin := range amt { - sendEnabled, found := k.getSendEnabled(ctx, coin.Denom) - if found && !sendEnabled { - return fmt.Errorf("denom: %s, is prohibited from being sent at this time", coin.Denom) - } - } - if k.BlockedAddr(recipientAddr) { return errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", recipientAddr) } diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go index 45fdb7031dc8..12db16b24c2b 100644 --- a/x/bank/keeper/keeper_test.go +++ b/x/bank/keeper/keeper_test.go @@ -389,24 +389,6 @@ func (suite *KeeperTestSuite) TestSendCoinsFromModuleToAccount_Blocklist() { )) } -func (suite *KeeperTestSuite) TestSendCoinsFromModuleToAccount_CoinSendDisabled() { - ctx := suite.ctx - require := suite.Require() - keeper := suite.bankKeeper - - suite.mockMintCoins(mintAcc) - require.NoError(keeper.MintCoins(ctx, banktypes.MintModuleName, initCoins)) - - keeper.SetSendEnabled(ctx, sdk.DefaultBondDenom, false) - - suite.authKeeper.EXPECT().GetModuleAddress(mintAcc.Name).Return(mintAcc.GetAddress()) - err := keeper.SendCoinsFromModuleToAccount( - ctx, banktypes.MintModuleName, accAddrs[2], initCoins, - ) - require.Contains(err.Error(), "stake, is prohibited from being sent at this time") - keeper.SetSendEnabled(ctx, sdk.DefaultBondDenom, true) -} - func (suite *KeeperTestSuite) TestSupply_DelegateUndelegateCoins() { ctx := suite.ctx require := suite.Require() From 7eef3b3705877da222271caa9562cb0b329c50ad Mon Sep 17 00:00:00 2001 From: cocoyeal <150209682+cocoyeal@users.noreply.github.com> Date: Tue, 21 May 2024 16:17:52 +0800 Subject: [PATCH 3/5] docs: fix some markdown syntax (#20432) --- docs/learn/advanced/00-baseapp.md | 4 ++-- docs/learn/advanced/08-events.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/learn/advanced/00-baseapp.md b/docs/learn/advanced/00-baseapp.md index 15a9feb31cbb..f19c132994ae 100644 --- a/docs/learn/advanced/00-baseapp.md +++ b/docs/learn/advanced/00-baseapp.md @@ -354,7 +354,7 @@ The response contains: https://github.com/cosmos/cosmos-sdk/blob/v0.50.0-alpha.0/x/auth/ante/basic.go#L102 ``` -* `Events ([]cmn.KVPair)`: Key-Value tags for filtering and indexing transactions (eg. by account). See [`event`s](./08-events.md) for more. +* `Events ([]cmn.KVPair)`: Key-Value tags for filtering and indexing transactions (eg. by account). See [`events`](./08-events.md) for more. * `Codespace (string)`: Namespace for the Code. #### RecheckTx @@ -495,7 +495,7 @@ Each transaction returns a response to the underlying consensus engine of type [ * `Info (string):` Additional information. May be non-deterministic. * `GasWanted (int64)`: Amount of gas requested for transaction. It is provided by users when they generate the transaction. * `GasUsed (int64)`: Amount of gas consumed by transaction. During transaction execution, this value is computed by multiplying the standard cost of a transaction byte by the size of the raw transaction, and by adding gas each time a read/write to the store occurs. -* `Events ([]cmn.KVPair)`: Key-Value tags for filtering and indexing transactions (eg. by account). See [`event`s](./08-events.md) for more. +* `Events ([]cmn.KVPair)`: Key-Value tags for filtering and indexing transactions (eg. by account). See [`events`](./08-events.md) for more. * `Codespace (string)`: Namespace for the Code. #### EndBlock diff --git a/docs/learn/advanced/08-events.md b/docs/learn/advanced/08-events.md index 2b8d754ac6a8..0c5772e325a6 100644 --- a/docs/learn/advanced/08-events.md +++ b/docs/learn/advanced/08-events.md @@ -4,7 +4,7 @@ sidebar_position: 1 # Events :::note Synopsis -`Event`s are objects that contain information about the execution of the application. They are mainly used by service providers like block explorers and wallet to track the execution of various messages and index transactions. +`Events` are objects that contain information about the execution of the application. They are mainly used by service providers like block explorers and wallet to track the execution of various messages and index transactions. ::: :::note Pre-requisite Readings From a2aaed66e872e35164eee30073dadcde8e5ad158 Mon Sep 17 00:00:00 2001 From: Hwangjae Lee Date: Tue, 21 May 2024 18:00:13 +0900 Subject: [PATCH 4/5] docs(x/account/auth): Improve error handling and comments in fee.go (#20426) Signed-off-by: Hwangjae Lee Co-authored-by: son trinh --- x/auth/ante/fee.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/x/auth/ante/fee.go b/x/auth/ante/fee.go index c057d5a7fa73..21b5300fd53c 100644 --- a/x/auth/ante/fee.go +++ b/x/auth/ante/fee.go @@ -12,14 +12,14 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) -// TxFeeChecker check if the provided fee is enough and returns the effective fee and tx priority, -// the effective fee should be deducted later, and the priority should be returned in abci response. +// TxFeeChecker checks if the provided fee is enough and returns the effective fee and tx priority. +// The effective fee should be deducted later, and the priority should be returned in the ABCI response. type TxFeeChecker func(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, int64, error) // DeductFeeDecorator deducts fees from the fee payer. The fee payer is the fee granter (if specified) or first signer of the tx. // If the fee payer does not have the funds to pay for the fees, return an InsufficientFunds error. -// Call next AnteHandler if fees successfully deducted. -// CONTRACT: Tx must implement FeeTx interface to use DeductFeeDecorator +// Call next AnteHandler if fees are successfully deducted. +// CONTRACT: The Tx must implement the FeeTx interface to use DeductFeeDecorator. type DeductFeeDecorator struct { accountKeeper AccountKeeper bankKeeper types.BankKeeper @@ -43,7 +43,7 @@ func NewDeductFeeDecorator(ak AccountKeeper, bk types.BankKeeper, fk FeegrantKee func (dfd DeductFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, _ bool, next sdk.AnteHandler) (sdk.Context, error) { feeTx, ok := tx.(sdk.FeeTx) if !ok { - return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") + return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "Tx must implement the FeeTx interface") } txService := dfd.accountKeeper.GetEnvironment().TransactionService @@ -76,10 +76,11 @@ func (dfd DeductFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, _ bool, nex func (dfd DeductFeeDecorator) checkDeductFee(ctx sdk.Context, sdkTx sdk.Tx, fee sdk.Coins) error { feeTx, ok := sdkTx.(sdk.FeeTx) if !ok { - return errorsmod.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") + return errorsmod.Wrap(sdkerrors.ErrTxDecode, "Tx must implement the FeeTx interface") } - if addr := dfd.accountKeeper.GetModuleAddress(types.FeeCollectorName); addr == nil { + addr := dfd.accountKeeper.GetModuleAddress(types.FeeCollectorName) + if len(addr) == 0 { return fmt.Errorf("fee collector module account (%s) has not been set", types.FeeCollectorName) } @@ -87,8 +88,8 @@ func (dfd DeductFeeDecorator) checkDeductFee(ctx sdk.Context, sdkTx sdk.Tx, fee feeGranter := feeTx.FeeGranter() deductFeesFrom := feePayer - // if feegranter set deduct fee from feegranter account. - // this works with only when feegrant enabled. + // if feegranter set, deduct fee from feegranter account. + // this works only when feegrant is enabled. if feeGranter != nil { feeGranterAddr := sdk.AccAddress(feeGranter) @@ -132,7 +133,7 @@ func DeductFees(bankKeeper types.BankKeeper, ctx sdk.Context, acc []byte, fees s err := bankKeeper.SendCoinsFromAccountToModule(ctx, sdk.AccAddress(acc), types.FeeCollectorName, fees) if err != nil { - return errorsmod.Wrapf(sdkerrors.ErrInsufficientFunds, err.Error()) + return fmt.Errorf("failed to deduct fees: %w", err) } return nil From a2dd2a0923bf4f1ab0ca04178a3815d2d1dc8d52 Mon Sep 17 00:00:00 2001 From: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Date: Tue, 21 May 2024 11:40:01 +0200 Subject: [PATCH 5/5] fix(x/accounts): check for overflows in multisig weights and votes (#20384) --- x/accounts/defaults/multisig/account.go | 17 +++++- x/accounts/defaults/multisig/account_test.go | 54 +++++++++++++++++++ x/accounts/defaults/multisig/update_config.go | 6 ++- 3 files changed, 75 insertions(+), 2 deletions(-) diff --git a/x/accounts/defaults/multisig/account.go b/x/accounts/defaults/multisig/account.go index 6ab25a8440d9..d6fadbfabff9 100644 --- a/x/accounts/defaults/multisig/account.go +++ b/x/accounts/defaults/multisig/account.go @@ -91,7 +91,10 @@ func (a *Account) Init(ctx context.Context, msg *v1.MsgInit) (*v1.MsgInitRespons return nil, err } - totalWeight += msg.Members[i].Weight + totalWeight, err = safeAdd(totalWeight, msg.Members[i].Weight) + if err != nil { + return nil, err + } } if err := validateConfig(*msg.Config, totalWeight); err != nil { @@ -279,6 +282,7 @@ func (a Account) ExecuteProposal(ctx context.Context, msg *v1.MsgExecuteProposal } totalWeight := yesVotes + noVotes + abstainVotes + var ( rejectErr error execErr error @@ -387,3 +391,14 @@ func (a *Account) RegisterQueryHandlers(builder *accountstd.QueryBuilder) { accountstd.RegisterQueryHandler(builder, a.QueryProposal) accountstd.RegisterQueryHandler(builder, a.QueryConfig) } + +func safeAdd(nums ...uint64) (uint64, error) { + var sum uint64 + for _, num := range nums { + if sum+num < sum { + return 0, errors.New("overflow") + } + sum += num + } + return sum, nil +} diff --git a/x/accounts/defaults/multisig/account_test.go b/x/accounts/defaults/multisig/account_test.go index ab95b5be4803..58d3e11a4457 100644 --- a/x/accounts/defaults/multisig/account_test.go +++ b/x/accounts/defaults/multisig/account_test.go @@ -2,6 +2,7 @@ package multisig import ( "context" + "math" "testing" "time" @@ -312,6 +313,29 @@ func TestUpdateConfig(t *testing.T) { }, }, }, + { + "change members, invalid weights", + &v1.MsgUpdateConfig{ + UpdateMembers: []*v1.Member{ + { + Address: "addr1", + Weight: math.MaxUint64, + }, + { + Address: "addr2", + Weight: 1, + }, + }, + Config: &v1.Config{ + Threshold: 666, + Quorum: 400, + VotingPeriod: 60, + }, + }, + "overflow", + nil, + nil, + }, } for _, tc := range testcases { @@ -658,3 +682,33 @@ func TestProposalPassing(t *testing.T) { require.Equal(t, expectedMembers, cfg.Members) } + +func TestWeightOverflow(t *testing.T) { + ctx, ss := newMockContext(t) + acc := setup(t, ctx, ss, nil) + + startAcc := &v1.MsgInit{ + Config: &v1.Config{ + Threshold: 2640, + Quorum: 2000, + VotingPeriod: 60, + }, + Members: []*v1.Member{ + { + Address: "addr1", + Weight: math.MaxUint64, + }, + }, + } + + _, err := acc.Init(ctx, startAcc) + require.NoError(t, err) + + // add another member with weight 1 to trigger an overflow + startAcc.Members = append(startAcc.Members, &v1.Member{ + Address: "addr2", + Weight: 1, + }) + _, err = acc.Init(ctx, startAcc) + require.ErrorContains(t, err, "overflow") +} diff --git a/x/accounts/defaults/multisig/update_config.go b/x/accounts/defaults/multisig/update_config.go index a1966c787b0b..d352d49926fa 100644 --- a/x/accounts/defaults/multisig/update_config.go +++ b/x/accounts/defaults/multisig/update_config.go @@ -44,7 +44,11 @@ func (a Account) UpdateConfig(ctx context.Context, msg *v1.MsgUpdateConfig) (*v1 // get the weight from the stored members totalWeight := uint64(0) err := a.Members.Walk(ctx, nil, func(_ []byte, value uint64) (stop bool, err error) { - totalWeight += value + var adderr error + totalWeight, adderr = safeAdd(totalWeight, value) + if adderr != nil { + return true, adderr + } return false, nil }) if err != nil {