feat: cpu and store gas updates #3054
Conversation
There was a problem hiding this comment.
can we just make the txtars all just use a -gas-wanted of 10M?
just because they'll have to be updated again. I'd want for us to keep track of "golden" gas values just in one place.
| @@ -1154,127 +1154,130 @@ func (m *Machine) incrCPU(cycles int64) { | |||
| } | |||
|
|
|||
| const ( | |||
| // CPU cycles | |||
There was a problem hiding this comment.
Can we add a reference to the benchmarks and methodology?
There was a problem hiding this comment.
Yes please 🙏
I'm a bit worried about the new gas values, and a comparison would help us make a fair assessment of the gas bumps
| @@ -205,6 +205,7 @@ var gnoStoreContextKey gnoStoreContextKeyType | |||
| func (vm *VMKeeper) newGnoTransactionStore(ctx sdk.Context) gno.TransactionStore { | |||
| base := ctx.Store(vm.baseKey) | |||
| iavl := ctx.Store(vm.iavlKey) | |||
| vm.gnoStore.SetGasMeter(ctx.GasMeter()) | |||
There was a problem hiding this comment.
This is unsafe, because vm.gnoStore is shared, and this would set the gas meter globally; ie. not scoped to the transaction.
| func (ds *defaultStore) SetGasMeter(gm store.GasMeter) { | ||
| ds.gasMeter = gm | ||
| } |
There was a problem hiding this comment.
I think this ought to go in BeginTransaction, and the store shouldn't use a gas store otherwise.
| func DefaultGasConfig() GasConfig { | ||
| return GasConfig{ | ||
| GasGetObject: 16, // per byte cost | ||
| GasSetObject: 16, // per byte cost | ||
| GasGetType: 52, // per byte cost | ||
| GasSetType: 52, // per byte cost | ||
| GasGetPackageRealm: 524, // per byte cost | ||
| GasSetPackageRealm: 524, // per byte cost | ||
| GasAddMemPackage: 8, // per byte cost | ||
| GasGetMemPackage: 8, // per byte cost | ||
| GasDeleteObject: 3715, // flat cost | ||
| } | ||
| } |
There was a problem hiding this comment.
These are constants for the VM. Can these also not be constants?
| func (c Context) Store(key store.StoreKey) store.Store { | ||
| return c.MultiStore().GetStore(key) | ||
| } | ||
|
|
There was a problem hiding this comment.
I can understand the rename to GasStore, but I don't think we should expose a gas-less Store method, especially when getting the underlying store is still trivial and exported.
There was a problem hiding this comment.
I think the PR looks fine, apart from the comments left by @thehowl, which should be addressed about the global gas meter setting in the VM keeper 🙏
I'm temporarily just putting a pause on the PR, to allow for us to discuss whether these new gas values make sense, since they are a huge change. I also want to see some more reasoning and gas comparisons, apart from #2241
If we're fine with the bumps, I am also cool with the PR moving forward as is ✅
| stdout OK! | ||
|
|
||
| # send 10000ugnot to `proxywugnot` to wrap it | ||
| gnokey maketx call -pkgpath gno.land/r/demo/proxywugnot --send "10000ugnot" -func ProxyWrap -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1 |
There was a problem hiding this comment.
Is this real? This is a huge increase in gas usage.
Do we have some kind of comparison data?
| @@ -1154,127 +1154,130 @@ func (m *Machine) incrCPU(cycles int64) { | |||
| } | |||
|
|
|||
| const ( | |||
| // CPU cycles | |||
There was a problem hiding this comment.
Yes please 🙏
I'm a bit worried about the new gas values, and a comparison would help us make a fair assessment of the gas bumps
Summary:
This PR updates the CPU and Store gas based on results from the benchmarking tool:#2241
For CPU gas, the measurement is in nanoseconds per opcode execution.
For storage gas, the measurement is in nanoseconds per byte for each type of Gno store access.
Changes:
We moved the gas meter from the underlying store to the upper Gno store to capture accurate resource consumption for VM transactions. At the same time, we retain the original gas store and gas meter for the Auth Keeper to handle regular blockchain transactions that do not necessarily involve the VM.
We also updated the gas-wanted in the integration test to reflect actual gas usage. This can serve as a flag to alert us to future changes that might increase gas assumptions.
Additional reasons for these changes include:
Here are the diagrams to explain the store access gas before and after changes
Before:
After:
Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description