From 8ca8acf6387c753cc95a8670642e3cb4e20f0095 Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Wed, 26 Sep 2018 14:02:56 +0000 Subject: [PATCH] Merge PR #2357: Refactor Iterator Gas Consumption --- PENDING.md | 3 + store/gaskvstore.go | 143 +++++++++++++++++++++++---------------- store/gaskvstore_test.go | 4 +- types/gas.go | 38 +++++++---- 4 files changed, 117 insertions(+), 71 deletions(-) diff --git a/PENDING.md b/PENDING.md index 8e6c9e94e9..2fc72dd73e 100644 --- a/PENDING.md +++ b/PENDING.md @@ -137,6 +137,9 @@ IMPROVEMENTS * [simulation] Logs get written to file if large, and also get printed on panics \#2285 * [gaiad] \#1992 Add optional flag to `gaiad testnet` to make config directory of daemon (default `gaiad`) and cli (default `gaiacli`) configurable * [x/stake] Add stake `Queriers` for Gaia-lite endpoints. This increases the staking endpoints performance by reusing the staking `keeper` logic for queries. [#2249](https://github.com/cosmos/cosmos-sdk/pull/2149) + * [store] [\#2017](https://github.com/cosmos/cosmos-sdk/issues/2017) Refactor + gas iterator gas consumption to only consume gas for iterator creation and `Next` + calls which includes dynamic consumption of value length. * [types/decimal] \#2378 - Added truncate functionality to decimal * [client] [\#1184](https://github.com/cosmos/cosmos-sdk/issues/1184) Remove unused `client/tx/sign.go`. diff --git a/store/gaskvstore.go b/store/gaskvstore.go index 53ac340bd7..2ce6415e66 100644 --- a/store/gaskvstore.go +++ b/store/gaskvstore.go @@ -8,13 +8,15 @@ import ( var _ KVStore = &gasKVStore{} -// gasKVStore applies gas tracking to an underlying kvstore +// gasKVStore applies gas tracking to an underlying KVStore. It implements the +// KVStore interface. type gasKVStore struct { gasMeter sdk.GasMeter gasConfig sdk.GasConfig parent sdk.KVStore } +// NewGasKVStore returns a reference to a new GasKVStore. // nolint func NewGasKVStore(gasMeter sdk.GasMeter, gasConfig sdk.GasConfig, parent sdk.KVStore) *gasKVStore { kvs := &gasKVStore{ @@ -26,83 +28,96 @@ func NewGasKVStore(gasMeter sdk.GasMeter, gasConfig sdk.GasConfig, parent sdk.KV } // Implements Store. -func (gi *gasKVStore) GetStoreType() sdk.StoreType { - return gi.parent.GetStoreType() +func (gs *gasKVStore) GetStoreType() sdk.StoreType { + return gs.parent.GetStoreType() } // Implements KVStore. -func (gi *gasKVStore) Get(key []byte) (value []byte) { - gi.gasMeter.ConsumeGas(gi.gasConfig.ReadCostFlat, "ReadFlat") - value = gi.parent.Get(key) +func (gs *gasKVStore) Get(key []byte) (value []byte) { + gs.gasMeter.ConsumeGas(gs.gasConfig.ReadCostFlat, sdk.GasReadCostFlatDesc) + value = gs.parent.Get(key) + // TODO overflow-safe math? - gi.gasMeter.ConsumeGas(gi.gasConfig.ReadCostPerByte*sdk.Gas(len(value)), "ReadPerByte") + gs.gasMeter.ConsumeGas(gs.gasConfig.ReadCostPerByte*sdk.Gas(len(value)), sdk.GasReadPerByteDesc) return value } // Implements KVStore. -func (gi *gasKVStore) Set(key []byte, value []byte) { - gi.gasMeter.ConsumeGas(gi.gasConfig.WriteCostFlat, "WriteFlat") +func (gs *gasKVStore) Set(key []byte, value []byte) { + gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostFlat, sdk.GasWriteCostFlatDesc) // TODO overflow-safe math? - gi.gasMeter.ConsumeGas(gi.gasConfig.WriteCostPerByte*sdk.Gas(len(value)), "WritePerByte") - gi.parent.Set(key, value) + gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostPerByte*sdk.Gas(len(value)), sdk.GasWritePerByteDesc) + gs.parent.Set(key, value) } // Implements KVStore. -func (gi *gasKVStore) Has(key []byte) bool { - gi.gasMeter.ConsumeGas(gi.gasConfig.HasCost, "Has") - return gi.parent.Has(key) +func (gs *gasKVStore) Has(key []byte) bool { + gs.gasMeter.ConsumeGas(gs.gasConfig.HasCost, sdk.GasHasDesc) + return gs.parent.Has(key) } // Implements KVStore. -func (gi *gasKVStore) Delete(key []byte) { - // No gas costs for deletion - gi.parent.Delete(key) +func (gs *gasKVStore) Delete(key []byte) { + // charge gas to prevent certain attack vectors even though space is being freed + gs.gasMeter.ConsumeGas(gs.gasConfig.DeleteCost, sdk.GasDeleteDesc) + gs.parent.Delete(key) } // Implements KVStore -func (gi *gasKVStore) Prefix(prefix []byte) KVStore { +func (gs *gasKVStore) Prefix(prefix []byte) KVStore { // Keep gasstore layer at the top return &gasKVStore{ - gasMeter: gi.gasMeter, - gasConfig: gi.gasConfig, - parent: prefixStore{gi.parent, prefix}, + gasMeter: gs.gasMeter, + gasConfig: gs.gasConfig, + parent: prefixStore{gs.parent, prefix}, } } // Implements KVStore -func (gi *gasKVStore) Gas(meter GasMeter, config GasConfig) KVStore { - return NewGasKVStore(meter, config, gi) +func (gs *gasKVStore) Gas(meter GasMeter, config GasConfig) KVStore { + return NewGasKVStore(meter, config, gs) +} + +// Iterator implements the KVStore interface. It returns an iterator which +// incurs a flat gas cost for seeking to the first key/value pair and a variable +// gas cost based on the current value's length if the iterator is valid. +func (gs *gasKVStore) Iterator(start, end []byte) sdk.Iterator { + return gs.iterator(start, end, true) +} + +// ReverseIterator implements the KVStore interface. It returns a reverse +// iterator which incurs a flat gas cost for seeking to the first key/value pair +// and a variable gas cost based on the current value's length if the iterator +// is valid. +func (gs *gasKVStore) ReverseIterator(start, end []byte) sdk.Iterator { + return gs.iterator(start, end, false) } // Implements KVStore. -func (gi *gasKVStore) Iterator(start, end []byte) sdk.Iterator { - return gi.iterator(start, end, true) -} - -// Implements KVStore. -func (gi *gasKVStore) ReverseIterator(start, end []byte) sdk.Iterator { - return gi.iterator(start, end, false) -} - -// Implements KVStore. -func (gi *gasKVStore) CacheWrap() sdk.CacheWrap { +func (gs *gasKVStore) CacheWrap() sdk.CacheWrap { panic("cannot CacheWrap a GasKVStore") } // CacheWrapWithTrace implements the KVStore interface. -func (gi *gasKVStore) CacheWrapWithTrace(_ io.Writer, _ TraceContext) CacheWrap { +func (gs *gasKVStore) CacheWrapWithTrace(_ io.Writer, _ TraceContext) CacheWrap { panic("cannot CacheWrapWithTrace a GasKVStore") } -func (gi *gasKVStore) iterator(start, end []byte, ascending bool) sdk.Iterator { +func (gs *gasKVStore) iterator(start, end []byte, ascending bool) sdk.Iterator { var parent sdk.Iterator if ascending { - parent = gi.parent.Iterator(start, end) + parent = gs.parent.Iterator(start, end) } else { - parent = gi.parent.ReverseIterator(start, end) + parent = gs.parent.ReverseIterator(start, end) } - return newGasIterator(gi.gasMeter, gi.gasConfig, parent) + + gi := newGasIterator(gs.gasMeter, gs.gasConfig, parent) + if gi.Valid() { + gi.(*gasIterator).consumeSeekGas() + } + + return gi } type gasIterator struct { @@ -120,36 +135,50 @@ func newGasIterator(gasMeter sdk.GasMeter, gasConfig sdk.GasConfig, parent sdk.I } // Implements Iterator. -func (g *gasIterator) Domain() (start []byte, end []byte) { - return g.parent.Domain() +func (gi *gasIterator) Domain() (start []byte, end []byte) { + return gi.parent.Domain() } // Implements Iterator. -func (g *gasIterator) Valid() bool { - return g.parent.Valid() +func (gi *gasIterator) Valid() bool { + return gi.parent.Valid() } -// Implements Iterator. -func (g *gasIterator) Next() { - g.parent.Next() +// Next implements the Iterator interface. It seeks to the next key/value pair +// in the iterator. It incurs a flat gas cost for seeking and a variable gas +// cost based on the current value's length if the iterator is valid. +func (gi *gasIterator) Next() { + if gi.Valid() { + gi.consumeSeekGas() + } + + gi.parent.Next() } -// Implements Iterator. -func (g *gasIterator) Key() (key []byte) { - g.gasMeter.ConsumeGas(g.gasConfig.KeyCostFlat, "KeyFlat") - key = g.parent.Key() +// Key implements the Iterator interface. It returns the current key and it does +// not incur any gas cost. +func (gi *gasIterator) Key() (key []byte) { + key = gi.parent.Key() return key } -// Implements Iterator. -func (g *gasIterator) Value() (value []byte) { - value = g.parent.Value() - g.gasMeter.ConsumeGas(g.gasConfig.ValueCostFlat, "ValueFlat") - g.gasMeter.ConsumeGas(g.gasConfig.ValueCostPerByte*sdk.Gas(len(value)), "ValuePerByte") +// Value implements the Iterator interface. It returns the current value and it +// does not incur any gas cost. +func (gi *gasIterator) Value() (value []byte) { + value = gi.parent.Value() return value } // Implements Iterator. -func (g *gasIterator) Close() { - g.parent.Close() +func (gi *gasIterator) Close() { + gi.parent.Close() +} + +// consumeSeekGas consumes a flat gas cost for seeking and a variable gas cost +// based on the current value's length. +func (gi *gasIterator) consumeSeekGas() { + value := gi.Value() + + gi.gasMeter.ConsumeGas(gi.gasConfig.ValueCostPerByte*sdk.Gas(len(value)), sdk.GasValuePerByteDesc) + gi.gasMeter.ConsumeGas(gi.gasConfig.IterNextCostFlat, sdk.GasIterNextCostFlatDesc) } diff --git a/store/gaskvstore_test.go b/store/gaskvstore_test.go index b2c23c9553..eb3ae7dd4f 100644 --- a/store/gaskvstore_test.go +++ b/store/gaskvstore_test.go @@ -25,7 +25,7 @@ func TestGasKVStoreBasic(t *testing.T) { require.Equal(t, valFmt(1), st.Get(keyFmt(1))) st.Delete(keyFmt(1)) require.Empty(t, st.Get(keyFmt(1)), "Expected `key1` to be empty") - require.Equal(t, meter.GasConsumed(), sdk.Gas(183)) + require.Equal(t, meter.GasConsumed(), sdk.Gas(193)) } func TestGasKVStoreIterator(t *testing.T) { @@ -49,7 +49,7 @@ func TestGasKVStoreIterator(t *testing.T) { iterator.Next() require.False(t, iterator.Valid()) require.Panics(t, iterator.Next) - require.Equal(t, meter.GasConsumed(), sdk.Gas(356)) + require.Equal(t, meter.GasConsumed(), sdk.Gas(384)) } func TestGasKVStoreOutOfGasSet(t *testing.T) { diff --git a/types/gas.go b/types/gas.go index e8486c8136..cce4975c30 100644 --- a/types/gas.go +++ b/types/gas.go @@ -1,9 +1,26 @@ package types +// Gas consumption descriptors. +const ( + GasIterNextCostFlatDesc = "IterNextFlat" + GasValuePerByteDesc = "ValuePerByte" + GasWritePerByteDesc = "WritePerByte" + GasReadPerByteDesc = "ReadPerByte" + GasWriteCostFlatDesc = "WriteFlat" + GasReadCostFlatDesc = "ReadFlat" + GasHasDesc = "Has" + GasDeleteDesc = "Delete" +) + +var ( + cachedDefaultGasConfig = DefaultGasConfig() + cachedTransientGasConfig = TransientGasConfig() +) + // Gas measured by the SDK type Gas = int64 -// Error thrown when out of gas +// ErrorOutOfGas defines an error thrown when an action results in out of gas. type ErrorOutOfGas struct { Descriptor string } @@ -19,6 +36,7 @@ type basicGasMeter struct { consumed Gas } +// NewGasMeter returns a reference to a new basicGasMeter. func NewGasMeter(limit Gas) GasMeter { return &basicGasMeter{ limit: limit, @@ -41,6 +59,7 @@ type infiniteGasMeter struct { consumed Gas } +// NewInfiniteGasMeter returns a reference to a new infiniteGasMeter. func NewInfiniteGasMeter() GasMeter { return &infiniteGasMeter{ consumed: 0, @@ -58,35 +77,30 @@ func (g *infiniteGasMeter) ConsumeGas(amount Gas, descriptor string) { // GasConfig defines gas cost for each operation on KVStores type GasConfig struct { HasCost Gas + DeleteCost Gas ReadCostFlat Gas ReadCostPerByte Gas WriteCostFlat Gas WriteCostPerByte Gas - KeyCostFlat Gas - ValueCostFlat Gas ValueCostPerByte Gas + IterNextCostFlat Gas } -var ( - cachedDefaultGasConfig = DefaultGasConfig() - cachedTransientGasConfig = TransientGasConfig() -) - -// Default gas config for KVStores +// DefaultGasConfig returns a default gas config for KVStores. func DefaultGasConfig() GasConfig { return GasConfig{ HasCost: 10, + DeleteCost: 10, ReadCostFlat: 10, ReadCostPerByte: 1, WriteCostFlat: 10, WriteCostPerByte: 10, - KeyCostFlat: 5, - ValueCostFlat: 10, ValueCostPerByte: 1, + IterNextCostFlat: 15, } } -// Default gas config for TransientStores +// TransientGasConfig returns a default gas config for TransientStores. func TransientGasConfig() GasConfig { // TODO: define gasconfig for transient stores return DefaultGasConfig()