diff --git a/CHANGELOG.md b/CHANGELOG.md index b3aa1dc82..a46de7009 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/bank) [\#8614](https://github.com/cosmos/cosmos-sdk/issues/8614) Add `Name` and `Symbol` fields to denom metadata * (x/auth) [\#8522](https://github.com/cosmos/cosmos-sdk/pull/8522) Allow to query all stored accounts * (crypto/types) [\#8600](https://github.com/cosmos/cosmos-sdk/pull/8600) `CompactBitArray`: optimize the `NumTrueBitsBefore` method and add an `Equal` method. +* (store/cachekv), (x/bank/types) [\#8719](https://github.com/cosmos/cosmos-sdk/pull/8719) algorithmically fix pathologically slow code diff --git a/store/cachekv/store.go b/store/cachekv/store.go index b2e394e95..42c94370b 100644 --- a/store/cachekv/store.go +++ b/store/cachekv/store.go @@ -4,9 +4,11 @@ import ( "bytes" "container/list" "io" + "reflect" "sort" "sync" "time" + "unsafe" dbm "github.com/tendermint/tm-db" @@ -177,18 +179,48 @@ func (store *Store) iterator(start, end []byte, ascending bool) types.Iterator { return newCacheMergeIterator(parent, cache, ascending) } +// strToByte is meant to make a zero allocation conversion +// from string -> []byte to speed up operations, it is not meant +// to be used generally, but for a specific pattern to check for available +// keys within a domain. +func strToByte(s string) []byte { + var b []byte + hdr := (*reflect.SliceHeader)(unsafe.Pointer(&b)) + hdr.Cap = len(s) + hdr.Len = len(s) + hdr.Data = (*reflect.StringHeader)(unsafe.Pointer(&s)).Data + return b +} + +// byteSliceToStr is meant to make a zero allocation conversion +// from []byte -> string to speed up operations, it is not meant +// to be used generally, but for a specific pattern to delete keys +// from a map. +func byteSliceToStr(b []byte) string { + hdr := (*reflect.StringHeader)(unsafe.Pointer(&b)) + return *(*string)(unsafe.Pointer(hdr)) +} + // Constructs a slice of dirty items, to use w/ memIterator. func (store *Store) dirtyItems(start, end []byte) { unsorted := make([]*kv.Pair, 0) + n := len(store.unsortedCache) for key := range store.unsortedCache { - cacheValue := store.cache[key] - - if dbm.IsKeyInDomain([]byte(key), start, end) { + if dbm.IsKeyInDomain(strToByte(key), start, end) { + cacheValue := store.cache[key] unsorted = append(unsorted, &kv.Pair{Key: []byte(key), Value: cacheValue.value}) + } + } + if len(unsorted) == n { // This pattern allows the Go compiler to emit the map clearing idiom for the entire map. + for key := range store.unsortedCache { delete(store.unsortedCache, key) } + } else { // Otherwise, normally delete the unsorted keys from the map. + for _, kv := range unsorted { + delete(store.unsortedCache, byteSliceToStr(kv.Key)) + } } sort.Slice(unsorted, func(i, j int) bool { diff --git a/x/auth/client/cli/tx_multisign.go b/x/auth/client/cli/tx_multisign.go index 61ec84bdc..c04d37123 100644 --- a/x/auth/client/cli/tx_multisign.go +++ b/x/auth/client/cli/tx_multisign.go @@ -32,21 +32,21 @@ func GetMultiSignCommand() *cobra.Command { Long: strings.TrimSpace( fmt.Sprintf(`Sign transactions created with the --generate-only flag that require multisig signatures. -Read signature(s) from [signature] file(s), generate a multisig signature compliant to the -multisig key [name], and attach it to the transaction read from [file]. +Read one or more signatures from one or more [signature] file, generate a multisig signature compliant to the +multisig key [name], and attach the key name to the transaction read from [file]. Example: $ %s tx multisign transaction.json k1k2k3 k1sig.json k2sig.json k3sig.json -If the flag --signature-only flag is on, it outputs a JSON representation -of the generated signature only. +If --signature-only flag is on, output a JSON representation +of only the generated signature. -The --offline flag makes sure that the client will not reach out to an external node. -Thus account number or sequence number lookups will not be performed and it is -recommended to set such parameters manually. +If the --offline flag is on, the client will not reach out to an external node. +Account number or sequence number lookups are not performed so you must +set these parameters manually. -The current multisig implementation doesn't support SIGN_MORE_DIRECT and defaults -to amino-json sign mode.' +The current multisig implementation defaults to amino-json sign mode. +The SIGN_MODE_DIRECT sign mode is not supported.' `, version.AppName, ), @@ -56,8 +56,8 @@ to amino-json sign mode.' } cmd.Flags().Bool(flagSigOnly, false, "Print only the generated signature, then exit") - cmd.Flags().String(flags.FlagOutputDocument, "", "The document will be written to the given file instead of STDOUT") - cmd.Flags().Bool(flagAmino, false, "Generate Amino encoded JSON suitable for submiting to the txs REST endpoint") + cmd.Flags().String(flags.FlagOutputDocument, "", "The document is written to the given file instead of STDOUT") + cmd.Flags().Bool(flagAmino, false, "Generate Amino-encoded JSON suitable for submitting to the txs REST endpoint") flags.AddTxFlagsToCmd(cmd) cmd.Flags().String(flags.FlagChainID, "", "network chain ID") @@ -196,14 +196,14 @@ func GetMultiSignBatchCmd() *cobra.Command { Long: strings.TrimSpace( fmt.Sprintf(`Assemble a batch of multisig transactions generated by batch sign command. -Read signature(s) from [signature] file(s), generates multisig signatures compliant to the -multisig key [name], and attach it to the transactions read from [file]. +Read one or more signatures from one or more [signature] file, generate a multisig signature compliant to the +multisig key [name], and attach the key name to the transaction read from [file]. Example: $ %s tx multisign-batch transactions.json multisigk1k2k3 k1sigs.json k2sigs.json k3sig.json -The current multisig implementation doesn't support sign_mode_direct and defaults -to amino-json sign mode.' +The current multisig implementation defaults to amino-json sign mode. +The SIGN_MODE_DIRECT sign mode is not supported.' `, version.AppName, ), ), @@ -215,7 +215,7 @@ to amino-json sign mode.' cmd.Flags().Bool(flagNoAutoIncrement, false, "disable sequence auto increment") cmd.Flags().String( flagMultisig, "", - "Address of the multisig account on behalf of which the transaction shall be signed", + "Address of the multisig account that the transaction signs on behalf of", ) flags.AddTxFlagsToCmd(cmd) diff --git a/x/bank/types/balance.go b/x/bank/types/balance.go index 615067902..7c017ee90 100644 --- a/x/bank/types/balance.go +++ b/x/bank/types/balance.go @@ -62,16 +62,47 @@ func (b Balance) Validate() error { // SanitizeGenesisBalances sorts addresses and coin sets. func SanitizeGenesisBalances(balances []Balance) []Balance { - sort.Slice(balances, func(i, j int) bool { - addr1, _ := sdk.AccAddressFromBech32(balances[i].Address) - addr2, _ := sdk.AccAddressFromBech32(balances[j].Address) - return bytes.Compare(addr1.Bytes(), addr2.Bytes()) < 0 - }) + // Given that this function sorts balances, using the standard library's + // Quicksort based algorithms, we have algorithmic complexities of: + // * Best case: O(nlogn) + // * Worst case: O(n^2) + // The comparator used MUST be cheap to use lest we incur expenses like we had + // before whereby sdk.AccAddressFromBech32, which is a very expensive operation + // compared n * n elements yet discarded computations each time, as per: + // https://github.com/cosmos/cosmos-sdk/issues/7766#issuecomment-786671734 + // with this change the first step is to extract out and singly produce the values + // that'll be used for comparisons and keep them cheap and fast. + // 1. Retrieve the byte equivalents for each Balance's address and maintain a mapping of + // its Balance, as the mapper will be used in sorting. + type addrToBalance struct { + // We use a pointer here to avoid averse effects of value copying + // wasting RAM all around. + balance *Balance + accAddr []byte + } + adL := make([]*addrToBalance, 0, len(balances)) for _, balance := range balances { - balance.Coins = balance.Coins.Sort() + balance := balance + addr, _ := sdk.AccAddressFromBech32(balance.Address) + adL = append(adL, &addrToBalance{ + balance: &balance, + accAddr: []byte(addr), + }) } + // 2. Sort with the cheap mapping, using the mapper's + // already accAddr bytes values which is a cheaper operation. + sort.Slice(adL, func(i, j int) bool { + ai, aj := adL[i], adL[j] + return bytes.Compare(ai.accAddr, aj.accAddr) < 0 + }) + + // 3. To complete the sorting, we have to now just insert + // back the balances in the order returned by the sort. + for i, ad := range adL { + balances[i] = *ad.balance + } return balances } diff --git a/x/bank/types/balance_test.go b/x/bank/types/balance_test.go index 3987f5189..9736c847b 100644 --- a/x/bank/types/balance_test.go +++ b/x/bank/types/balance_test.go @@ -1,10 +1,12 @@ package types_test import ( + "bytes" "testing" "github.com/stretchr/testify/require" + "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" sdk "github.com/cosmos/cosmos-sdk/types" bank "github.com/cosmos/cosmos-sdk/x/bank/types" ) @@ -101,3 +103,79 @@ func TestBalance_GetAddress(t *testing.T) { }) } } + +func TestSanitizeBalances(t *testing.T) { + // 1. Generate balances + tokens := sdk.TokensFromConsensusPower(81) + coin := sdk.NewCoin("benchcoin", tokens) + coins := sdk.Coins{coin} + addrs, _ := makeRandomAddressesAndPublicKeys(20) + + var balances []bank.Balance + for _, addr := range addrs { + balances = append(balances, bank.Balance{ + Address: addr.String(), + Coins: coins, + }) + } + // 2. Sort the values. + sorted := bank.SanitizeGenesisBalances(balances) + + // 3. Compare and ensure that all the values are sorted in ascending order. + // Invariant after sorting: + // a[i] <= a[i+1...n] + for i := 0; i < len(sorted); i++ { + ai := sorted[i] + // Ensure that every single value that comes after i is less than it. + for j := i + 1; j < len(sorted); j++ { + aj := sorted[j] + + if got := bytes.Compare(ai.GetAddress(), aj.GetAddress()); got > 0 { + t.Errorf("Balance(%d) > Balance(%d)", i, j) + } + } + } +} + +func makeRandomAddressesAndPublicKeys(n int) (accL []sdk.AccAddress, pkL []*ed25519.PubKey) { + for i := 0; i < n; i++ { + pk := ed25519.GenPrivKey().PubKey().(*ed25519.PubKey) + pkL = append(pkL, pk) + accL = append(accL, sdk.AccAddress(pk.Address())) + } + return accL, pkL +} + +var sink, revert interface{} + +func BenchmarkSanitizeBalances500(b *testing.B) { + benchmarkSanitizeBalances(b, 500) +} + +func BenchmarkSanitizeBalances1000(b *testing.B) { + benchmarkSanitizeBalances(b, 1000) +} + +func benchmarkSanitizeBalances(b *testing.B, nAddresses int) { + b.ReportAllocs() + tokens := sdk.TokensFromConsensusPower(81) + coin := sdk.NewCoin("benchcoin", tokens) + coins := sdk.Coins{coin} + addrs, _ := makeRandomAddressesAndPublicKeys(nAddresses) + + b.ResetTimer() + for i := 0; i < b.N; i++ { + var balances []bank.Balance + for _, addr := range addrs { + balances = append(balances, bank.Balance{ + Address: addr.String(), + Coins: coins, + }) + } + sink = bank.SanitizeGenesisBalances(balances) + } + if sink == nil { + b.Fatal("Benchmark did not run") + } + sink = revert +}