Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

R4R: Refactor Iterator Gas Consumption #2357

Merged
merged 9 commits into from
Sep 26, 2018
Merged

R4R: Refactor Iterator Gas Consumption #2357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
aec978b
Implement initial iterator gas updates + cleanup
Sep 19, 2018
8892bb6
Update gas iterator costs and godocs
Sep 19, 2018
915d3c7
Update Next godoc
Sep 19, 2018
0522a53
Reintroduce nolint to NewGasKVStore (returns unexported type)
Sep 19, 2018
6d56017
Fix gas store tests
Sep 19, 2018
be863a0
Update pending log
Sep 19, 2018
4e50ecb
Update gas consumption for gasIterator
Sep 20, 2018
ea8f457
Charge gas for store delete calls
Sep 26, 2018
570c500
Merge branch 'develop' into bez/2017-gas-iter-refactor
Sep 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions PENDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,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

* Tendermint
Expand Down
135 changes: 82 additions & 53 deletions store/gaskvstore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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)
Copy link
Contributor

@ValarDragon ValarDragon Sep 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't change this, but why is deleting 0 cost? Shouldn't it be at least as complex as Has?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good observation. We're freeing up space, but I agree it should charge the same price as Has. Thoughts @cwgoes ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't charge because space is freed - but actually this might be a DoS vector though, good catch. Let's charge the same as Has.

}

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

// Implements KVStore.
func (gi *gasKVStore) Iterator(start, end []byte) sdk.Iterator {
return gi.iterator(start, end, true)
// 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)
}

// Implements KVStore.
func (gi *gasKVStore) ReverseIterator(start, end []byte) sdk.Iterator {
return gi.iterator(start, end, false)
// 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) 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 {
Expand All @@ -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)
}
4 changes: 2 additions & 2 deletions store/gaskvstore_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
38 changes: 26 additions & 12 deletions types/gas.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
}
Expand All @@ -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,
Expand All @@ -41,6 +59,7 @@ type infiniteGasMeter struct {
consumed Gas
}

// NewInfiniteGasMeter returns a reference to a new infiniteGasMeter.
func NewInfiniteGasMeter() GasMeter {
return &infiniteGasMeter{
consumed: 0,
Expand All @@ -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()
Expand Down