fix: InitGenesis changes account numbers (upstream fix from Provenanc…

…e) (#13877) * fix: InitGenesis changes account numbers (upstream fix from Provenance) * cl++ * rename GetNextAccountNumber to NextAccountNumber * suggestion from @julienrbrt * cl++ * fix readme * missing replace (cherry picked from commit 394f1b9)
cosmos · Nov 16, 2022 · 2c1f22c · 2c1f22c
1 parent a3170f0
commit 2c1f22c
Show file tree

Hide file tree

Showing 7 changed files with 180 additions and 9 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
 }