Backport genesis speed ups to 0.41.x (#8722)

From: #8719
Fixes: #7766

Co-authored-by: Emmanuel T Odeke <[email protected]>
Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Federico Kunze <[email protected]>

CHANGELOG.md

@@ -36,7 +36,13 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
-**IMPORTANT:** Due to a bug in the `v0.41.x` series with how evidence handles validator consensus addresses [\#8461](https://github.com/cosmos/cosmos-sdk/issues/8461), SDK based chains that are not using the de$
+**IMPORTANT**: Due to a bug in the v0.41.x series with how evidence handles validator consensus addresses #8461, SDK based chains that are not using the default bech32 prefix (cosmos, aka all chains except for the Cosmos Hub) should not use this release or any release in the v0.41.x series. Please see #8668 for tracking & timeline for the v0.42.0 release, which will include a fix for this issue.
+
+
+###Improvements
+
+* [\#8719](https://github.com/cosmos/cosmos-sdk/pull/8719) (store/cachekv), (x/bank/types) algorithmically fix pathologically slow code
+
 
 ## [v0.41.3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.41.3) - 2021-02-18
 

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