fix DiffKVStores(), store/types gets 100% coverage

DiffKVStores() used to return duplicated entries
in some cases.

Add test cases, aiming to reach 100% coverage for
store package.

Remove duplicate Cp function from the store package.
Same functionality is provided by types.CopyBytes().

CHANGELOG.md

@@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 balances or a single balance by denom when the `denom` query parameter is present.
 * (client) [\#5640](https://github.com/cosmos/cosmos-sdk/pull/5640) The rest server endpoint `/swagger-ui/` is replaced by ´/´.
 * (x/auth) [\#5702](https://github.com/cosmos/cosmos-sdk/pull/5702) The `x/auth` querier route has changed from `"acc"` to `"auth"`.
+* (store/types) [\#5730](https://github.com/cosmos/cosmos-sdk/pull/5730) store.types.Cp() is removed in favour of types.CopyBytes().
 
 ### API Breaking Changes
 

store/firstlast.go

@@ -5,7 +5,7 @@ import (
 
 	tmkv "github.com/tendermint/tendermint/libs/kv"
 
-	"github.com/cosmos/cosmos-sdk/store/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
 // Gets the first item.
@@ -24,7 +24,7 @@ func Last(st KVStore, start, end []byte) (kv tmkv.Pair, ok bool) {
 	iter := st.ReverseIterator(end, start)
 	if !iter.Valid() {
 		if v := st.Get(start); v != nil {
-			return tmkv.Pair{Key: types.Cp(start), Value: types.Cp(v)}, true
+			return tmkv.Pair{Key: sdk.CopyBytes(start), Value: sdk.CopyBytes(v)}, true
 		}
 		return kv, false
 	}

store/iavl/store.go

@@ -15,6 +15,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/store/cachekv"
 	"github.com/cosmos/cosmos-sdk/store/tracekv"
 	"github.com/cosmos/cosmos-sdk/store/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 )
 
@@ -357,8 +358,8 @@ var _ types.Iterator = (*iavlIterator)(nil)
 func newIAVLIterator(tree *iavl.ImmutableTree, start, end []byte, ascending bool) *iavlIterator {
 	iter := &iavlIterator{
 		tree:      tree,
-		start:     types.Cp(start),
-		end:       types.Cp(end),
+		start:     sdk.CopyBytes(start),
+		end:       sdk.CopyBytes(end),
 		ascending: ascending,
 		iterCh:    make(chan tmkv.Pair), // Set capacity > 0?
 		quitCh:    make(chan struct{}),

store/iavl/tree_test.go

@@ -0,0 +1,37 @@
+package iavl
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"github.com/tendermint/iavl"
+	dbm "github.com/tendermint/tm-db"
+)
+
+func TestImmutableTreePanics(t *testing.T) {
+	t.Parallel()
+	immTree := iavl.NewImmutableTree(dbm.NewMemDB(), 100)
+	it := &immutableTree{immTree}
+	require.Panics(t, func() { it.Set([]byte{}, []byte{}) })
+	require.Panics(t, func() { it.Remove([]byte{}) })
+	require.Panics(t, func() { it.SaveVersion() })
+	require.Panics(t, func() { it.DeleteVersion(int64(1)) })
+	v, _ := it.GetVersioned([]byte{0x01}, 1)
+	require.Equal(t, int64(-1), v)
+	v, _ = it.GetVersioned([]byte{0x01}, 0)
+	require.Equal(t, int64(0), v)
+
+	val, proof, err := it.GetVersionedWithProof(nil, 1)
+	require.Error(t, err)
+	require.Nil(t, val)
+	require.Nil(t, proof)
+
+	imm, err := it.GetImmutable(1)
+	require.Error(t, err)
+	require.Nil(t, imm)
+
+	imm, err = it.GetImmutable(0)
+	require.NoError(t, err)
+	require.NotNil(t, imm)
+	require.Equal(t, immTree, imm)
+}

store/types/gas_test.go

@@ -7,7 +7,23 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestInfiniteGasMeter(t *testing.T) {
+	t.Parallel()
+	meter := NewInfiniteGasMeter()
+	require.Equal(t, uint64(0), meter.Limit())
+	require.Equal(t, uint64(0), meter.GasConsumed())
+	require.Equal(t, uint64(0), meter.GasConsumedToLimit())
+	meter.ConsumeGas(10, "consume 10")
+	require.Equal(t, uint64(10), meter.GasConsumed())
+	require.Equal(t, uint64(10), meter.GasConsumedToLimit())
+	require.False(t, meter.IsPastLimit())
+	require.False(t, meter.IsOutOfGas())
+	meter.ConsumeGas(Gas(math.MaxUint64/2), "consume half max uint64")
+	require.Panics(t, func() { meter.ConsumeGas(Gas(math.MaxUint64/2)+2, "panic") })
+}
+
 func TestGasMeter(t *testing.T) {
+	t.Parallel()
 	cases := []struct {
 		limit Gas
 		usage []Gas
@@ -41,11 +57,14 @@ func TestGasMeter(t *testing.T) {
 		require.Panics(t, func() { meter.ConsumeGas(1, "") }, "Exceeded but not panicked. tc #%d", tcnum)
 		require.Equal(t, meter.GasConsumedToLimit(), meter.Limit(), "Gas consumption (to limit) not match limit")
 		require.Equal(t, meter.GasConsumed(), meter.Limit()+1, "Gas consumption not match limit+1")
-
+		meter2 := NewGasMeter(math.MaxUint64)
+		meter2.ConsumeGas(Gas(math.MaxUint64/2), "consume half max uint64")
+		require.Panics(t, func() { meter2.ConsumeGas(Gas(math.MaxUint64/2)+2, "panic") })
 	}
 }
 
 func TestAddUint64Overflow(t *testing.T) {
+	t.Parallel()
 	testCases := []struct {
 		a, b     uint64
 		result   uint64
@@ -69,3 +88,17 @@ func TestAddUint64Overflow(t *testing.T) {
 		)
 	}
 }
+
+func TestTransientGasConfig(t *testing.T) {
+	t.Parallel()
+	config := TransientGasConfig()
+	require.Equal(t, config, GasConfig{
+		HasCost:          1000,
+		DeleteCost:       1000,
+		ReadCostFlat:     1000,
+		ReadCostPerByte:  3,
+		WriteCostFlat:    2000,
+		WriteCostPerByte: 30,
+		IterNextCostFlat: 30,
+	})
+}

store/types/store_test.go

@@ -1,12 +1,15 @@
 package types
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestStoreUpgrades(t *testing.T) {
+	t.Parallel()
 	type toDelete struct {
 		key    string
 		delete bool
@@ -55,3 +58,27 @@ func TestStoreUpgrades(t *testing.T) {
 		})
 	}
 }
+
+func TestCommitID(t *testing.T) {
+	t.Parallel()
+	require.True(t, CommitID{}.IsZero())
+	require.False(t, CommitID{Version: int64(1)}.IsZero())
+	require.False(t, CommitID{Hash: []byte("x")}.IsZero())
+	require.Equal(t, "CommitID{[120 120 120 120]:64}", CommitID{Version: int64(100), Hash: []byte("xxxx")}.String())
+}
+
+func TestKVStoreKey(t *testing.T) {
+	t.Parallel()
+	key := NewKVStoreKey("test")
+	require.Equal(t, "test", key.name)
+	require.Equal(t, key.name, key.Name())
+	require.Equal(t, fmt.Sprintf("KVStoreKey{%p, test}", key), key.String())
+}
+
+func TestTransientStoreKey(t *testing.T) {
+	t.Parallel()
+	key := NewTransientStoreKey("test")
+	require.Equal(t, "test", key.name)
+	require.Equal(t, key.name, key.Name())
+	require.Equal(t, fmt.Sprintf("TransientStoreKey{%p, test}", key), key.String())
+}

store/types/utils.go

@@ -17,15 +17,18 @@ func KVStoreReversePrefixIterator(kvs KVStore, prefix []byte) Iterator {
 }
 
 // DiffKVStores compares two KVstores and returns all the key/value pairs
-// that differ from one another. It also skips value comparison for a set of provided prefixes
+// that differ from one another. It also skips value comparison for a set of provided prefixes.
 func DiffKVStores(a KVStore, b KVStore, prefixesToSkip [][]byte) (kvAs, kvBs []tmkv.Pair) {
 	iterA := a.Iterator(nil, nil)
+	defer iterA.Close()
 	iterB := b.Iterator(nil, nil)
+	defer iterB.Close()
 
 	for {
 		if !iterA.Valid() && !iterB.Valid() {
-			break
+			return kvAs, kvBs
 		}
+
 		var kvA, kvB tmkv.Pair
 		if iterA.Valid() {
 			kvA = tmkv.Pair{Key: iterA.Key(), Value: iterA.Value()}
@@ -38,7 +41,9 @@ func DiffKVStores(a KVStore, b KVStore, prefixesToSkip [][]byte) (kvAs, kvBs []t
 		if !bytes.Equal(kvA.Key, kvB.Key) {
 			kvAs = append(kvAs, kvA)
 			kvBs = append(kvBs, kvB)
+			continue // no need to compare the value
 		}
+
 		compareValue := true
 		for _, prefix := range prefixesToSkip {
 			// Skip value comparison if we matched a prefix
@@ -51,7 +56,6 @@ func DiffKVStores(a KVStore, b KVStore, prefixesToSkip [][]byte) (kvAs, kvBs []t
 			kvBs = append(kvBs, kvB)
 		}
 	}
-	return kvAs, kvBs
 }
 
 // PrefixEndBytes returns the []byte that would end a
@@ -69,13 +73,13 @@ func PrefixEndBytes(prefix []byte) []byte {
 		if end[len(end)-1] != byte(255) {
 			end[len(end)-1]++
 			break
-		} else {
-			end = end[:len(end)-1]
-			if len(end) == 0 {
-				end = nil
-				break
-			}
 		}
+		end = end[:len(end)-1]
+		if len(end) == 0 {
+			end = nil
+			break
+		}
+
 	}
 	return end
 }
@@ -85,13 +89,3 @@ func PrefixEndBytes(prefix []byte) []byte {
 func InclusiveEndBytes(inclusiveBytes []byte) []byte {
 	return append(inclusiveBytes, byte(0x00))
 }
-
-//----------------------------------------
-func Cp(bz []byte) (ret []byte) {
-	if bz == nil {
-		return nil
-	}
-	ret = make([]byte, len(bz))
-	copy(ret, bz)
-	return ret
-}

store/types/utils_test.go

@@ -0,0 +1,88 @@
+package types_test
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	dbm "github.com/tendermint/tm-db"
+
+	"github.com/cosmos/cosmos-sdk/store/rootmulti"
+	"github.com/cosmos/cosmos-sdk/store/types"
+)
+
+func initTestStores(t *testing.T) (types.KVStore, types.KVStore) {
+	db := dbm.NewMemDB()
+	ms := rootmulti.NewStore(db)
+
+	key1 := types.NewKVStoreKey("store1")
+	key2 := types.NewKVStoreKey("store2")
+	require.NotPanics(t, func() { ms.MountStoreWithDB(key1, types.StoreTypeIAVL, db) })
+	require.NotPanics(t, func() { ms.MountStoreWithDB(key2, types.StoreTypeIAVL, db) })
+	require.NoError(t, ms.LoadLatestVersion())
+	return ms.GetKVStore(key1), ms.GetKVStore(key2)
+}
+
+func TestDiffKVStores(t *testing.T) {
+	t.Parallel()
+	store1, store2 := initTestStores(t)
+	// Two equal stores
+	k1, v1 := []byte("k1"), []byte("v1")
+	store1.Set(k1, v1)
+	store2.Set(k1, v1)
+
+	kvAs, kvBs := types.DiffKVStores(store1, store2, nil)
+	require.Equal(t, 0, len(kvAs))
+	require.Equal(t, len(kvAs), len(kvBs))
+
+	// delete k1 from store2, which is now empty
+	store2.Delete(k1)
+	kvAs, kvBs = types.DiffKVStores(store1, store2, nil)
+	require.Equal(t, 1, len(kvAs))
+	require.Equal(t, len(kvAs), len(kvBs))
+
+	// set k1 in store2, different value than what store1 holds for k1
+	v2 := []byte("v2")
+	store2.Set(k1, v2)
+	kvAs, kvBs = types.DiffKVStores(store1, store2, nil)
+	require.Equal(t, 1, len(kvAs))
+	require.Equal(t, len(kvAs), len(kvBs))
+
+	// add k2 to store2
+	k2 := []byte("k2")
+	store2.Set(k2, v2)
+	kvAs, kvBs = types.DiffKVStores(store1, store2, nil)
+	require.Equal(t, 2, len(kvAs))
+	require.Equal(t, len(kvAs), len(kvBs))
+
+	// Reset stores
+	store1.Delete(k1)
+	store2.Delete(k1)
+	store2.Delete(k2)
+
+	// Same keys, different value. Comparisons will be nil as prefixes are skipped.
+	prefix := []byte("prefix:")
+	k1Prefixed := append(prefix, k1...)
+	store1.Set(k1Prefixed, v1)
+	store2.Set(k1Prefixed, v2)
+	kvAs, kvBs = types.DiffKVStores(store1, store2, [][]byte{prefix})
+	require.Equal(t, 0, len(kvAs))
+	require.Equal(t, len(kvAs), len(kvBs))
+}
+
+func TestPrefixEndBytes(t *testing.T) {
+	t.Parallel()
+	bs1 := []byte{0x23, 0xA5, 0x06}
+	require.True(t, bytes.Equal([]byte{0x23, 0xA5, 0x07}, types.PrefixEndBytes(bs1)))
+	bs2 := []byte{0x23, 0xA5, 0xFF}
+	require.True(t, bytes.Equal([]byte{0x23, 0xA6}, types.PrefixEndBytes(bs2)))
+	require.Nil(t, types.PrefixEndBytes([]byte{0xFF}))
+	require.Nil(t, types.PrefixEndBytes(nil))
+}
+
+func TestInclusiveEndBytes(t *testing.T) {
+	t.Parallel()
+	require.True(t, bytes.Equal([]byte{0x00}, types.InclusiveEndBytes(nil)))
+	bs := []byte("test")
+	require.True(t, bytes.Equal(append(bs, byte(0x00)), types.InclusiveEndBytes(bs)))
+}

store/types/validity_test.go

@@ -0,0 +1,23 @@
+package types_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/cosmos/cosmos-sdk/store/types"
+)
+
+func TestAssertValidKey(t *testing.T) {
+	t.Parallel()
+	require.NotPanics(t, func() { types.AssertValidKey([]byte{}) })
+	require.NotPanics(t, func() { types.AssertValidKey([]byte{0x01}) })
+	require.Panics(t, func() { types.AssertValidKey(nil) })
+}
+
+func TestAssertValidValue(t *testing.T) {
+	t.Parallel()
+	require.NotPanics(t, func() { types.AssertValidValue([]byte{}) })
+	require.NotPanics(t, func() { types.AssertValidValue([]byte{0x01}) })
+	require.Panics(t, func() { types.AssertValidValue(nil) })
+}

types/utils.go

@@ -80,3 +80,13 @@ func NewLevelDB(name, dir string) (db dbm.DB, err error) {
 	}()
 	return dbm.NewDB(name, backend, dir), err
 }
+
+// copy bytes
+func CopyBytes(bz []byte) (ret []byte) {
+	if bz == nil {
+		return nil
+	}
+	ret = make([]byte, len(bz))
+	copy(ret, bz)
+	return ret
+}