From e0d029388df204fcb265e9704846d4937115e582 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Mon, 2 Mar 2020 00:27:03 +0000 Subject: [PATCH] fix DiffKVStores(), store/types gets 100% coverage (#5730) * 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(). * More test cases Co-authored-by: Alexander Bezobchuk --- CHANGELOG.md | 1 + store/firstlast.go | 4 +- store/gaskv/store_test.go | 27 ++++++++++- store/iavl/store.go | 5 +- store/iavl/tree_test.go | 37 +++++++++++++++ store/types/gas_test.go | 35 +++++++++++++- store/types/store_test.go | 27 +++++++++++ store/types/utils.go | 32 ++++++------- store/types/utils_test.go | 88 ++++++++++++++++++++++++++++++++++++ store/types/validity_test.go | 23 ++++++++++ types/bytes.go | 11 ----- types/utils.go | 10 ++++ types/utils_test.go | 10 ++++ 13 files changed, 274 insertions(+), 36 deletions(-) create mode 100644 store/iavl/tree_test.go create mode 100644 store/types/utils_test.go create mode 100644 store/types/validity_test.go delete mode 100644 types/bytes.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ca4f41b53..8499de7178 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/store/firstlast.go b/store/firstlast.go index 05a9a5fd85..0aab0a53ab 100644 --- a/store/firstlast.go +++ b/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 } diff --git a/store/gaskv/store_test.go b/store/gaskv/store_test.go index 6fab9ebf83..ec0d42be85 100644 --- a/store/gaskv/store_test.go +++ b/store/gaskv/store_test.go @@ -22,6 +22,11 @@ func TestGasKVStoreBasic(t *testing.T) { mem := dbadapter.Store{DB: dbm.NewMemDB()} meter := types.NewGasMeter(10000) st := gaskv.NewStore(mem, meter, types.KVGasConfig()) + + require.Equal(t, types.StoreTypeDB, st.GetStoreType()) + require.Panics(t, func() { st.CacheWrap() }) + require.Panics(t, func() { st.CacheWrapWithTrace(nil, nil) }) + require.Empty(t, st.Get(keyFmt(1)), "Expected `key1` to be empty") st.Set(keyFmt(1), valFmt(1)) require.Equal(t, valFmt(1), st.Get(keyFmt(1))) @@ -34,11 +39,20 @@ func TestGasKVStoreIterator(t *testing.T) { mem := dbadapter.Store{DB: dbm.NewMemDB()} meter := types.NewGasMeter(10000) st := gaskv.NewStore(mem, meter, types.KVGasConfig()) + require.False(t, st.Has(keyFmt(1))) require.Empty(t, st.Get(keyFmt(1)), "Expected `key1` to be empty") require.Empty(t, st.Get(keyFmt(2)), "Expected `key2` to be empty") st.Set(keyFmt(1), valFmt(1)) + require.True(t, st.Has(keyFmt(1))) st.Set(keyFmt(2), valFmt(2)) + iterator := st.Iterator(nil, nil) + start, end := iterator.Domain() + require.Nil(t, start) + require.Nil(t, end) + require.NoError(t, iterator.Error()) + + t.Cleanup(iterator.Close) ka := iterator.Key() require.Equal(t, ka, keyFmt(1)) va := iterator.Value() @@ -51,7 +65,18 @@ func TestGasKVStoreIterator(t *testing.T) { iterator.Next() require.False(t, iterator.Valid()) require.Panics(t, iterator.Next) - require.Equal(t, meter.GasConsumed(), types.Gas(6987)) + require.NoError(t, iterator.Error()) + + reverseIterator := st.ReverseIterator(nil, nil) + t.Cleanup(reverseIterator.Close) + require.Equal(t, reverseIterator.Key(), keyFmt(2)) + reverseIterator.Next() + require.Equal(t, reverseIterator.Key(), keyFmt(1)) + reverseIterator.Next() + require.False(t, reverseIterator.Valid()) + require.Panics(t, reverseIterator.Next) + + require.Equal(t, types.Gas(9194), meter.GasConsumed()) } func TestGasKVStoreOutOfGasSet(t *testing.T) { diff --git a/store/iavl/store.go b/store/iavl/store.go index d1e1d9c4ca..311d0f887a 100644 --- a/store/iavl/store.go +++ b/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{}), diff --git a/store/iavl/tree_test.go b/store/iavl/tree_test.go new file mode 100644 index 0000000000..7a1eef3023 --- /dev/null +++ b/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) +} diff --git a/store/types/gas_test.go b/store/types/gas_test.go index 00d3d834db..bd62d463cd 100644 --- a/store/types/gas_test.go +++ b/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, + }) +} diff --git a/store/types/store_test.go b/store/types/store_test.go index 7ef1d6e889..bf70af1bb6 100644 --- a/store/types/store_test.go +++ b/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()) +} diff --git a/store/types/utils.go b/store/types/utils.go index 3149c07fb6..8d4c837354 100644 --- a/store/types/utils.go +++ b/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 -} diff --git a/store/types/utils_test.go b/store/types/utils_test.go new file mode 100644 index 0000000000..32064d7e18 --- /dev/null +++ b/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))) +} diff --git a/store/types/validity_test.go b/store/types/validity_test.go new file mode 100644 index 0000000000..e2adcd19c4 --- /dev/null +++ b/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) }) +} diff --git a/types/bytes.go b/types/bytes.go deleted file mode 100644 index 600af9f04c..0000000000 --- a/types/bytes.go +++ /dev/null @@ -1,11 +0,0 @@ -package types - -// copy bytes -func CopyBytes(bz []byte) (ret []byte) { - if bz == nil { - return nil - } - ret = make([]byte, len(bz)) - copy(ret, bz) - return ret -} diff --git a/types/utils.go b/types/utils.go index 2a6add6b45..be33160261 100644 --- a/types/utils.go +++ b/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 +} diff --git a/types/utils_test.go b/types/utils_test.go index 352beccd42..6aa652220e 100644 --- a/types/utils_test.go +++ b/types/utils_test.go @@ -1,6 +1,7 @@ package types import ( + "bytes" "testing" "time" @@ -66,3 +67,12 @@ func TestTimeFormatAndParse(t *testing.T) { require.Equal(t, timeFromRFC.Format(SortableTimeFormat), tc.SDKSortableTimeStr) } } + +func TestCopyBytes(t *testing.T) { + t.Parallel() + require.Nil(t, CopyBytes(nil)) + require.Equal(t, 0, len(CopyBytes([]byte{}))) + bs := []byte("test") + bsCopy := CopyBytes(bs) + require.True(t, bytes.Equal(bs, bsCopy)) +}