Merge branch 'master' into robert/addr-memory-leak

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
 
 
 

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 {

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)
 

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
 }
 

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
+}