Skip to content

Commit

Permalink
fix: consolidate vm gas consumption (gnolang#1430)
Browse files Browse the repository at this point in the history
<!-- please provide a detailed description of the changes made in this
pull request. -->

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [x] No breaking changes were made
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

Ref: gnolang#1070 gnolang#1067 gnolang#649 gnolang#1281 


## Summary

The current gno.land node, optimized for development purposes, has a
simplified verification process and gas meter implementation. To
transition the gno.land node to a production-ready state, it is
necessary to implement a comprehensive gas metering system that
accurately accounts for VM gas consumption. This includes refining the
gas fee structure to encompass all relevant costs, ensuring robust
transaction validation, and calculating gas consumption based on actual
computational load.

This PR aims to address these limitations by introducing a complete gas
meter and validation flow, laying the groundwork for further gas meter
profiling and configuration.


## Problem Definition

Current State and Limitations for Production:

- **VM Gas Consumption Not Accounted in Gas Meter:** The current gas
meter fails to calculate VM gas consumption, potentially allowing heavy
contract loads without corresponding gas meter deductions. A refined
system should measure and charge for VM gas usage accurately.

- **Gas Fee Structure:** Presently, the gas fee structure only includes
storage access, transaction size, and signature verification. VM gas
fees are levied as a separate, flat fee, which might lead to confusion
among users expecting the total fee to match the amount specified in the
'gas-fee' argument. For improved transparency and precision, the gas fee
structure should integrate all these aspects.

- **Transaction Validation:** The system currently validates basic
information for VM msg_addpkg and msg_call. However, gas consumption
cannot be determined before fully executing these messages against the
VM. Consequently, VM transactions are placed in the mempool and
propagated to other nodes, even if they may not meet the gas fee
requirements to execute these transactions. This undermines the purpose
of using gas fees to prevent VM spamming.


## Solution: ( Updated )
This is a high-level description of the implemented features:

~~Added an anteHandler in VM to monitor gas consumption~~
~~Implemented chained VM anteHandler in auth.anteHandler~~
- Consume gas to verify account, signature and tx size in CheckTx 
- Consume VM gas in DeliverTx 
- Accumulated VM CPU cycles, memory allocation, store access,
transaction size, and signature verification into a single gas meter.
- Enabled local node checks of VM resource usage. The VM message is only
aborted if it runs out of gas in basic CheckTx. However, the message is
still propagated to other nodes if execution fails to prevent censorship
- Introduced a structured format for logging gas consumption for
profiling and metrics.
- Introduced a gas factor linking gas to vm CPU cycles and memory
allocation to balance between vm gas consumption with the rest.



## Trade-offs and Future Optimization: ( Updated )
~~The current implementation processes messages against the VM to check
gas consumption in abci.CheckTx() before inclusion in the mempool and
propagation to other nodes.~~

~~Messages lacking sufficient gas-wanted will be dropped, preventing
abuse without adequate gas fees. However, the trade-off is that for each
message with enough gas, the VM executes the transaction twice: once in
CheckTx() and once in DeliverTx(). As these occur in separate execution
contexts and are not in synchronized sequence, the performance impact is
currently a secondary concern.~~

We moved the VM gas check from CheckTx to DeliverTx for the following
reasons:

- We only know the VM gas consumption after the messages have been
processed.
- Running VM execution for many CheckTx requests from the peers could
overload the mempool that is executing CheckTx.
- This could slow down the propagation of transactions across the entire
network.

By moving the VM gas check from CheckTx to DeliverTx, we are able to
reduce the load on the mempool of a node and allow transactions to
propagate through the network faster.

In the future, we may use a predicted median value instead of the exact
value from transaction execution for efficiency.

## What's Next:
- Add a minimum gas price flag and configuration for node operation.
- Provide a user-friendly fee input interface, offering 'gas-wanted' and
'gas price' as alternatives to the current 'gas-wanted' and 'gas-fee'
inputs.
- Tune the gas factor based on VM CPU and Memory Profiling. The current
factor is 1:1 between gas and VM CPU cycles and memory allocation.

---------

Co-authored-by: Thomas Bruyelle <thomas.bruyelle@gmail.com>
  • Loading branch information
piux2 and tbruyelle authored Apr 25, 2024
1 parent 0ba95bf commit 8cc5636
Show file tree
Hide file tree
Showing 9 changed files with 332 additions and 100 deletions.
40 changes: 27 additions & 13 deletions gno.land/cmd/gnoland/testdata/addpkg.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,35 @@
## start a new node
gnoland start

## add bar.gno package located in $WORK directory as gno.land/r/foobar/bar
gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/foobar/bar -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
## add hello.gno package located in $WORK directory as gno.land/r/hello
gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/hello -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1

## execute Render
gnokey maketx call -pkgpath gno.land/r/foobar/bar -func Render -gas-fee 1000000ugnot -gas-wanted 2000000 -args '' -broadcast -chainid=tendermint_test test1
## compare AddPkg
cmp stdout stdout.addpkg.success

## compare render
stdout '("hello from foo" string)'
stdout 'OK!'
stdout 'GAS WANTED: 2000000'
stdout 'GAS USED: [0-9]+'

-- bar.gno --
package bar
## execute SayHello
gnokey maketx call -pkgpath gno.land/r/hello -func SayHello -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1

func Render(path string) string {
return "hello from foo"

## compare SayHello
cmp stdout stdout.call.success

-- hello.gno --
package hello

func SayHello() string {
return "hello world!"
}


-- stdout.addpkg.success --

OK!
GAS WANTED: 2000000
GAS USED: 119829
-- stdout.call.success --
("hello world!" string)
OK!
GAS WANTED: 2000000
GAS USED: 52801
2 changes: 1 addition & 1 deletion gno.land/pkg/gnoland/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ func NewAppWithOptions(cfg *AppOptions) (abci.Application, error) {
baseKey := store.NewStoreKey("base")

// Create BaseApp.
// TODO: Add a consensus based min gas prices for the node, by default it does not check
baseApp := sdk.NewBaseApp("gnoland", cfg.Logger, cfg.DB, baseKey, mainKey)
baseApp.SetAppVersion("dev")

Expand Down Expand Up @@ -139,7 +140,6 @@ func NewApp(dataRootDir string, skipFailingGenesisTxs bool, logger *slog.Logger,
}

cfg.Logger = logger

return NewAppWithOptions(cfg)
}

Expand Down
177 changes: 177 additions & 0 deletions gno.land/pkg/sdk/vm/gas_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
package vm

import (
"testing"

bft "github.com/gnolang/gno/tm2/pkg/bft/types"
"github.com/gnolang/gno/tm2/pkg/crypto"
"github.com/gnolang/gno/tm2/pkg/sdk"
"github.com/gnolang/gno/tm2/pkg/sdk/auth"
"github.com/gnolang/gno/tm2/pkg/std"
"github.com/gnolang/gno/tm2/pkg/store"
"github.com/stretchr/testify/assert"
)

// Gas for entire tx is consumed in both CheckTx and DeliverTx.
// Gas for executing VM tx (VM CPU and Store Access in bytes) is consumed in DeliverTx.
// Gas for balance checking, message size checking, and signature verification is consumed (deducted) in checkTx.

// Insufficient gas for a successful message.

func TestAddPkgDeliverTxInsuffGas(t *testing.T) {
success := true
ctx, tx, vmHandler := setupAddPkg(success)

ctx = ctx.WithMode(sdk.RunTxModeDeliver)
simulate := false
tx.Fee.GasWanted = 3000
gctx := auth.SetGasMeter(simulate, ctx, tx.Fee.GasWanted)

var res sdk.Result
abort := false

defer func() {
if r := recover(); r != nil {
switch r.(type) {
case store.OutOfGasException:
res.Error = sdk.ABCIError(std.ErrOutOfGas(""))
abort = true
default:
t.Errorf("should panic on OutOfGasException only")
}
assert.True(t, abort)
assert.False(t, res.IsOK())
gasCheck := gctx.GasMeter().GasConsumed()
assert.Equal(t, int64(3231), gasCheck)
} else {
t.Errorf("should panic")
}
}()
msgs := tx.GetMsgs()
res = vmHandler.Process(gctx, msgs[0])
}

// Enough gas for a successful message.
func TestAddPkgDeliverTx(t *testing.T) {
success := true
ctx, tx, vmHandler := setupAddPkg(success)

var simulate bool

ctx = ctx.WithMode(sdk.RunTxModeDeliver)
simulate = false
tx.Fee.GasWanted = 500000
gctx := auth.SetGasMeter(simulate, ctx, tx.Fee.GasWanted)
msgs := tx.GetMsgs()
res := vmHandler.Process(gctx, msgs[0])
gasDeliver := gctx.GasMeter().GasConsumed()

assert.True(t, res.IsOK())
assert.Equal(t, int64(87809), gasDeliver)
}

// Enough gas for a failed transaction.
func TestAddPkgDeliverTxFailed(t *testing.T) {
success := false
ctx, tx, vmHandler := setupAddPkg(success)

var simulate bool

ctx = ctx.WithMode(sdk.RunTxModeDeliver)
simulate = false
tx.Fee.GasWanted = 500000
gctx := auth.SetGasMeter(simulate, ctx, tx.Fee.GasWanted)
msgs := tx.GetMsgs()
res := vmHandler.Process(gctx, msgs[0])
gasDeliver := gctx.GasMeter().GasConsumed()

assert.False(t, res.IsOK())
assert.Equal(t, int64(17989), gasDeliver)
}

// Not enough gas for a failed transaction.
func TestAddPkgDeliverTxFailedNoGas(t *testing.T) {
success := false
ctx, tx, vmHandler := setupAddPkg(success)

var simulate bool

ctx = ctx.WithMode(sdk.RunTxModeDeliver)
simulate = false
tx.Fee.GasWanted = 17988
gctx := auth.SetGasMeter(simulate, ctx, tx.Fee.GasWanted)

var res sdk.Result
abort := false

defer func() {
if r := recover(); r != nil {
switch r.(type) {
case store.OutOfGasException:
res.Error = sdk.ABCIError(std.ErrOutOfGas(""))
abort = true
default:
t.Errorf("should panic on OutOfGasException only")
}
assert.True(t, abort)
assert.False(t, res.IsOK())
gasCheck := gctx.GasMeter().GasConsumed()
assert.Equal(t, int64(17989), gasCheck)
} else {
t.Errorf("should panic")
}
}()

msgs := tx.GetMsgs()
res = vmHandler.Process(gctx, msgs[0])
}

// Set up a test env for both a successful and a failed tx
func setupAddPkg(success bool) (sdk.Context, sdk.Tx, vmHandler) {
// setup
env := setupTestEnv()
ctx := env.ctx
// conduct base gas meter tests from a non-genesis block since genesis block use infinite gas meter instead.
ctx = ctx.WithBlockHeader(&bft.Header{Height: int64(1)})
vmHandler := NewHandler(env.vmk)
// Create an account with 10M ugnot (10gnot)
addr := crypto.AddressFromPreimage([]byte("test1"))
acc := env.acck.NewAccountWithAddress(ctx, addr)
env.acck.SetAccount(ctx, acc)
env.bank.SetCoins(ctx, addr, std.MustParseCoins("10000000ugnot"))
// success message
var files []*std.MemFile
if success {
files = []*std.MemFile{
{
Name: "hello.gno",
Body: `package hello
func Echo() string {
return "hello world"
}`,
},
}
} else {
// failed message
files = []*std.MemFile{
{
Name: "hello.gno",
Body: `package hello
func Echo() UnknowType {
return "hello world"
}`,
},
}
}

pkgPath := "gno.land/r/hello"
// create messages and a transaction
msg := NewMsgAddPackage(addr, pkgPath, files)
msgs := []std.Msg{msg}
fee := std.NewFee(500000, std.MustParseCoin("1ugnot"))
tx := std.NewTx(msgs, fee, []std.Signature{}, "")

return ctx, tx, vmHandler
}
33 changes: 3 additions & 30 deletions gno.land/pkg/sdk/vm/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (

abci "github.com/gnolang/gno/tm2/pkg/bft/abci/types"
"github.com/gnolang/gno/tm2/pkg/sdk"
"github.com/gnolang/gno/tm2/pkg/sdk/auth"
"github.com/gnolang/gno/tm2/pkg/std"
)

Expand Down Expand Up @@ -37,15 +36,7 @@ func (vh vmHandler) Process(ctx sdk.Context, msg std.Msg) sdk.Result {

// Handle MsgAddPackage.
func (vh vmHandler) handleMsgAddPackage(ctx sdk.Context, msg MsgAddPackage) sdk.Result {
amount, err := std.ParseCoins("1000000ugnot") // XXX calculate
if err != nil {
return abciResult(err)
}
err = vh.vm.bank.SendCoins(ctx, msg.Creator, auth.FeeCollectorAddress(), amount)
if err != nil {
return abciResult(err)
}
err = vh.vm.AddPackage(ctx, msg)
err := vh.vm.AddPackage(ctx, msg)
if err != nil {
return abciResult(err)
}
Expand All @@ -54,16 +45,7 @@ func (vh vmHandler) handleMsgAddPackage(ctx sdk.Context, msg MsgAddPackage) sdk.

// Handle MsgCall.
func (vh vmHandler) handleMsgCall(ctx sdk.Context, msg MsgCall) (res sdk.Result) {
amount, err := std.ParseCoins("1000000ugnot") // XXX calculate
if err != nil {
return abciResult(err)
}
err = vh.vm.bank.SendCoins(ctx, msg.Caller, auth.FeeCollectorAddress(), amount)
if err != nil {
return abciResult(err)
}
resstr := ""
resstr, err = vh.vm.Call(ctx, msg)
resstr, err := vh.vm.Call(ctx, msg)
if err != nil {
return abciResult(err)
}
Expand All @@ -81,16 +63,7 @@ func (vh vmHandler) handleMsgCall(ctx sdk.Context, msg MsgCall) (res sdk.Result)

// Handle MsgRun.
func (vh vmHandler) handleMsgRun(ctx sdk.Context, msg MsgRun) (res sdk.Result) {
amount, err := std.ParseCoins("1000000ugnot") // XXX calculate
if err != nil {
return abciResult(err)
}
err = vh.vm.bank.SendCoins(ctx, msg.Caller, auth.FeeCollectorAddress(), amount)
if err != nil {
return abciResult(err)
}
resstr := ""
resstr, err = vh.vm.Run(ctx, msg)
resstr, err := vh.vm.Run(ctx, msg)
if err != nil {
return abciResult(err)
}
Expand Down
Loading

0 comments on commit 8cc5636

Please sign in to comment.