From 394f1b9478a8dc568d4bab079732932488b46704 Mon Sep 17 00:00:00 2001 From: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Date: Wed, 16 Nov 2022 14:13:20 -0300 Subject: [PATCH] fix: InitGenesis changes account numbers (upstream fix from Provenance) (#13877) * fix: InitGenesis changes account numbers (upstream fix from Provenance) * cl++ * rename GetNextAccountNumber to NextAccountNumber * suggestion from @julienrbrt * cl++ * fix readme * missing replace --- CHANGELOG.md | 2 + x/auth/README.md | 2 +- x/auth/keeper/account.go | 2 +- x/auth/keeper/genesis.go | 10 ++- x/auth/keeper/keeper.go | 6 +- x/auth/keeper/keeper_test.go | 127 ++++++++++++++++++++++++++++++++++- x/auth/types/genesis.go | 40 ++++++++++- 7 files changed, 180 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f0cbd06da5cb..bb3aaad87d4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -118,6 +118,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking Changes +* (x/auth) [#13877](https://github.com/cosmos/cosmos-sdk/pull/13877) Rename `AccountKeeper`'s `GetNextAccountNumber` to `NextAccountNumber`. * (x/evidence) [#13740](https://github.com/cosmos/cosmos-sdk/pull/13740) The `NewQueryEvidenceRequest` function now takes `hash` as a HEX encoded `string`. * (server) [#13485](https://github.com/cosmos/cosmos-sdk/pull/13485) The `Application` service now requires the `RegisterNodeService` method to be implemented. * (x/slashing, x/staking) [#13122](https://github.com/cosmos/cosmos-sdk/pull/13122) Add the infraction a validator commited type as an argument to the `Slash` keeper method. @@ -199,6 +200,7 @@ extension interfaces. `module.Manager.Modules` is now of type `map[string]interf * (x/gov) [#13728](https://github.com/cosmos/cosmos-sdk/pull/13728) Fix propagation of message events to the current context in `EndBlocker`. * (server) [#13778](https://github.com/cosmos/cosmos-sdk/pull/13778) Set Cosmos SDK default endpoints to localhost to avoid unknown exposure of endpoints. * [#13861](https://github.com/cosmos/cosmos-sdk/pull/13861) Allow `_` characters in tx event queries, i.e. `GetTxsEvent`. +* (x/auth) [#13877](https://github.com/cosmos/cosmos-sdk/pull/13877) Handle missing account numbers during `InitGenesis`. ### Deprecated diff --git a/x/auth/README.md b/x/auth/README.md index 3ede66b2f1ad..e7526f004082 100644 --- a/x/auth/README.md +++ b/x/auth/README.md @@ -217,7 +217,7 @@ type AccountKeeperI interface { GetSequence(sdk.Context, sdk.AccAddress) (uint64, error) // Fetch the next account number, and increment the internal counter. - GetNextAccountNumber(sdk.Context) uint64 + NextAccountNumber(sdk.Context) uint64 } ``` diff --git a/x/auth/keeper/account.go b/x/auth/keeper/account.go index bfa03ea1e608..446f0c97774a 100644 --- a/x/auth/keeper/account.go +++ b/x/auth/keeper/account.go @@ -18,7 +18,7 @@ func (ak AccountKeeper) NewAccountWithAddress(ctx sdk.Context, addr sdk.AccAddre // NewAccount sets the next account number to a given account interface func (ak AccountKeeper) NewAccount(ctx sdk.Context, acc types.AccountI) types.AccountI { - if err := acc.SetAccountNumber(ak.GetNextAccountNumber(ctx)); err != nil { + if err := acc.SetAccountNumber(ak.NextAccountNumber(ctx)); err != nil { panic(err) } diff --git a/x/auth/keeper/genesis.go b/x/auth/keeper/genesis.go index 44c0af2d3bb8..6e748ef5b44a 100644 --- a/x/auth/keeper/genesis.go +++ b/x/auth/keeper/genesis.go @@ -20,8 +20,14 @@ func (ak AccountKeeper) InitGenesis(ctx sdk.Context, data types.GenesisState) { } accounts = types.SanitizeGenesisAccounts(accounts) - for _, a := range accounts { - acc := ak.NewAccount(ctx, a) + // Set the accounts and make sure the global account number matches the largest account number (even if zero). + var lastAccNum *uint64 + for _, acc := range accounts { + accNum := acc.GetAccountNumber() + for lastAccNum == nil || *lastAccNum < accNum { + n := ak.NextAccountNumber(ctx) + lastAccNum = &n + } ak.SetAccount(ctx, acc) } diff --git a/x/auth/keeper/keeper.go b/x/auth/keeper/keeper.go index 4e8494092329..e42183ad0193 100644 --- a/x/auth/keeper/keeper.go +++ b/x/auth/keeper/keeper.go @@ -46,7 +46,7 @@ type AccountKeeperI interface { GetSequence(sdk.Context, sdk.AccAddress) (uint64, error) // Fetch the next account number, and increment the internal counter. - GetNextAccountNumber(sdk.Context) uint64 + NextAccountNumber(sdk.Context) uint64 // GetModulePermissions fetches per-module account permissions GetModulePermissions() map[string]types.PermissionsForAddress @@ -128,9 +128,9 @@ func (ak AccountKeeper) GetSequence(ctx sdk.Context, addr sdk.AccAddress) (uint6 return acc.GetSequence(), nil } -// GetNextAccountNumber returns and increments the global account number counter. +// NextAccountNumber returns and increments the global account number counter. // If the global account number is not set, it initializes it with value 0. -func (ak AccountKeeper) GetNextAccountNumber(ctx sdk.Context) uint64 { +func (ak AccountKeeper) NextAccountNumber(ctx sdk.Context) uint64 { var accNumber uint64 store := ctx.KVStore(ak.storeKey) diff --git a/x/auth/keeper/keeper_test.go b/x/auth/keeper/keeper_test.go index ff5afcb4d4fb..7448fa5e4df6 100644 --- a/x/auth/keeper/keeper_test.go +++ b/x/auth/keeper/keeper_test.go @@ -8,6 +8,8 @@ import ( tmproto "github.com/tendermint/tendermint/proto/tendermint/types" "github.com/cosmos/cosmos-sdk/baseapp" + codectypes "github.com/cosmos/cosmos-sdk/codec/types" + "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" "github.com/cosmos/cosmos-sdk/testutil" sdk "github.com/cosmos/cosmos-sdk/types" moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil" @@ -62,7 +64,6 @@ func (suite *KeeperTestSuite) SetupTest() { "cosmos", types.NewModuleAddress("gov").String(), ) - suite.msgServer = keeper.NewMsgServerImpl(suite.accountKeeper) queryHelper := baseapp.NewQueryServerTestHelper(suite.ctx, suite.encCfg.InterfaceRegistry) types.RegisterQueryServer(queryHelper, suite.accountKeeper) @@ -159,3 +160,127 @@ func (suite *KeeperTestSuite) TestSupply_ValidatePermissions() { err = suite.accountKeeper.ValidatePermissions(otherAcc) suite.Require().Error(err) } + +func (suite *KeeperTestSuite) TestInitGenesis() { + suite.SetupTest() // reset + + // Check if params are set + genState := types.GenesisState{ + Params: types.Params{ + MaxMemoCharacters: types.DefaultMaxMemoCharacters + 1, + TxSigLimit: types.DefaultTxSigLimit + 1, + TxSizeCostPerByte: types.DefaultTxSizeCostPerByte + 1, + SigVerifyCostED25519: types.DefaultSigVerifyCostED25519 + 1, + SigVerifyCostSecp256k1: types.DefaultSigVerifyCostSecp256k1 + 1, + }, + } + + ctx := suite.ctx + suite.accountKeeper.InitGenesis(ctx, genState) + + params := suite.accountKeeper.GetParams(ctx) + suite.Require().Equal(genState.Params.MaxMemoCharacters, params.MaxMemoCharacters, "MaxMemoCharacters") + suite.Require().Equal(genState.Params.TxSigLimit, params.TxSigLimit, "TxSigLimit") + suite.Require().Equal(genState.Params.TxSizeCostPerByte, params.TxSizeCostPerByte, "TxSizeCostPerByte") + suite.Require().Equal(genState.Params.SigVerifyCostED25519, params.SigVerifyCostED25519, "SigVerifyCostED25519") + suite.Require().Equal(genState.Params.SigVerifyCostSecp256k1, params.SigVerifyCostSecp256k1, "SigVerifyCostSecp256k1") + + suite.SetupTest() // reset + ctx = suite.ctx + // Fix duplicate account numbers + pubKey1 := ed25519.GenPrivKey().PubKey() + pubKey2 := ed25519.GenPrivKey().PubKey() + accts := []types.AccountI{ + &types.BaseAccount{ + Address: sdk.AccAddress(pubKey1.Address()).String(), + PubKey: codectypes.UnsafePackAny(pubKey1), + AccountNumber: 0, + Sequence: 5, + }, + &types.ModuleAccount{ + BaseAccount: &types.BaseAccount{ + Address: types.NewModuleAddress("testing").String(), + PubKey: nil, + AccountNumber: 0, + Sequence: 6, + }, + Name: "testing", + Permissions: nil, + }, + &types.BaseAccount{ + Address: sdk.AccAddress(pubKey2.Address()).String(), + PubKey: codectypes.UnsafePackAny(pubKey2), + AccountNumber: 5, + Sequence: 7, + }, + } + genState = types.GenesisState{ + Params: types.DefaultParams(), + Accounts: nil, + } + for _, acct := range accts { + genState.Accounts = append(genState.Accounts, codectypes.UnsafePackAny(acct)) + } + + suite.accountKeeper.InitGenesis(ctx, genState) + + keeperAccts := suite.accountKeeper.GetAllAccounts(ctx) + // len(accts)+1 because we initialize fee_collector account after the genState accounts + suite.Require().Equal(len(keeperAccts), len(accts)+1, "number of accounts in the keeper vs in genesis state") + for i, genAcct := range accts { + genAcctAddr := genAcct.GetAddress() + var keeperAcct types.AccountI + for _, kacct := range keeperAccts { + if genAcctAddr.Equals(kacct.GetAddress()) { + keeperAcct = kacct + break + } + } + suite.Require().NotNilf(keeperAcct, "genesis account %s not in keeper accounts", genAcctAddr) + suite.Require().Equal(genAcct.GetPubKey(), keeperAcct.GetPubKey()) + suite.Require().Equal(genAcct.GetSequence(), keeperAcct.GetSequence()) + if i == 1 { + suite.Require().Equalf(1, int(keeperAcct.GetAccountNumber()), genAcctAddr.String()) + } else { + suite.Require().Equal(genAcct.GetSequence(), keeperAcct.GetSequence()) + } + } + + // fee_collector's is the last account to be set, so it has +1 of the highest in the accounts list + feeCollector := suite.accountKeeper.GetModuleAccount(ctx, "fee_collector") + suite.Require().Equal(6, int(feeCollector.GetAccountNumber())) + + // The 3rd account has account number 5, but because the FeeCollector account gets initialized last, the next should be 7. + nextNum := suite.accountKeeper.NextAccountNumber(ctx) + suite.Require().Equal(7, int(nextNum)) + + suite.SetupTest() // reset + ctx = suite.ctx + // one zero account still sets global account number + genState = types.GenesisState{ + Params: types.DefaultParams(), + Accounts: []*codectypes.Any{ + codectypes.UnsafePackAny(&types.BaseAccount{ + Address: sdk.AccAddress(pubKey1.Address()).String(), + PubKey: codectypes.UnsafePackAny(pubKey1), + AccountNumber: 0, + Sequence: 5, + }), + }, + } + + suite.accountKeeper.InitGenesis(ctx, genState) + + keeperAccts = suite.accountKeeper.GetAllAccounts(ctx) + // len(genState.Accounts)+1 because we initialize fee_collector as account number 1 (last) + suite.Require().Equal(len(keeperAccts), len(genState.Accounts)+1, "number of accounts in the keeper vs in genesis state") + + // Check both accounts account numbers + suite.Require().Equal(0, int(suite.accountKeeper.GetAccount(ctx, sdk.AccAddress(pubKey1.Address())).GetAccountNumber())) + feeCollector = suite.accountKeeper.GetModuleAccount(ctx, "fee_collector") + suite.Require().Equal(1, int(feeCollector.GetAccountNumber())) + + nextNum = suite.accountKeeper.NextAccountNumber(ctx) + // we expect nextNum to be 2 because we initialize fee_collector as account number 1 + suite.Require().Equal(2, int(nextNum)) +} diff --git a/x/auth/types/genesis.go b/x/auth/types/genesis.go index d9f3274d9ba5..82e658d60748 100644 --- a/x/auth/types/genesis.go +++ b/x/auth/types/genesis.go @@ -75,10 +75,48 @@ func ValidateGenesis(data GenesisState) error { // SanitizeGenesisAccounts sorts accounts and coin sets. func SanitizeGenesisAccounts(genAccs GenesisAccounts) GenesisAccounts { + // Make sure there aren't any duplicated account numbers by fixing the duplicates with the lowest unused values. + // seenAccNum = easy lookup for used account numbers. + seenAccNum := map[uint64]bool{} + // dupAccNum = a map of account number to accounts with duplicate account numbers (excluding the 1st one seen). + dupAccNum := map[uint64]GenesisAccounts{} + for _, acc := range genAccs { + num := acc.GetAccountNumber() + if !seenAccNum[num] { + seenAccNum[num] = true + } else { + dupAccNum[num] = append(dupAccNum[num], acc) + } + } + + // dupAccNums a sorted list of the account numbers with duplicates. + var dupAccNums []uint64 + for num := range dupAccNum { + dupAccNums = append(dupAccNums, num) + } + sort.Slice(dupAccNums, func(i, j int) bool { + return dupAccNums[i] < dupAccNums[j] + }) + + // Change the account number of the duplicated ones to the first unused value. + globalNum := uint64(0) + for _, dupNum := range dupAccNums { + accs := dupAccNum[dupNum] + for _, acc := range accs { + for seenAccNum[globalNum] { + globalNum++ + } + if err := acc.SetAccountNumber(globalNum); err != nil { + panic(err) + } + seenAccNum[globalNum] = true + } + } + + // Then sort them all by account number. sort.Slice(genAccs, func(i, j int) bool { return genAccs[i].GetAccountNumber() < genAccs[j].GetAccountNumber() }) - return genAccs }