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

Problem: no required gas log in precompiles #1223

Closed
wants to merge 6 commits into from
Closed

Problem: no required gas log in precompiles #1223

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5e07a2b
Problem: no require gas log in precompiles
mmsqe Oct 25, 2023
df59ecf
add log in ica
mmsqe Oct 25, 2023
707a600
add log in bank
mmsqe Oct 25, 2023
a9d070e
mv log gas to base
mmsqe Oct 25, 2023
41f3ebc
Update CHANGELOG.md
mmsqe Oct 25, 2023
74860a9
Merge remote-tracking branch 'origin/main' into log_gas
mmsqe Nov 7, 2023
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
- [#1215](https://github.com/crypto-org-chain/cronos/pull/1215) Update ethermint to fix of concurrent write in fee history.
- [#1217](https://github.com/crypto-org-chain/cronos/pull/1217) Use the default chain-id behavour in sdk.

### Improvements

- [#](https://github.com/crypto-org-chain/cronos/pull/) Add required gas log in precompiles.
mmsqe marked this conversation as resolved.
Show resolved Hide resolved

*October 17, 2023*

## v1.1.0-rc1
Expand Down
4 changes: 2 additions & 2 deletions app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,8 +553,8 @@ func New(
tracer,
evmS,
[]vm.PrecompiledContract{
cronosprecompiles.NewRelayerContract(app.IBCKeeper, appCodec, gasConfig),
cronosprecompiles.NewIcaContract(&app.ICAAuthKeeper, &app.CronosKeeper, appCodec, gasConfig),
cronosprecompiles.NewRelayerContract(app.IBCKeeper, appCodec, gasConfig, app.Logger()),
cronosprecompiles.NewIcaContract(&app.ICAAuthKeeper, &app.CronosKeeper, appCodec, gasConfig, app.Logger()),
},
allKeys,
)
Expand Down
38 changes: 20 additions & 18 deletions x/cronos/keeper/precompiles/bank.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"errors"
"math/big"

"github.com/cometbft/cometbft/libs/log"
storetypes "github.com/cosmos/cosmos-sdk/store/types"

sdkmath "cosmossdk.io/math"
Expand Down Expand Up @@ -31,6 +32,7 @@ var (
bankABI abi.ABI
bankContractAddress = common.BytesToAddress([]byte{100})
bankGasRequiredByMethod = map[[4]byte]uint64{}
bankMethodMap = map[[4]byte]string{}
)

func init() {
Expand All @@ -50,6 +52,7 @@ func init() {
default:
bankGasRequiredByMethod[methodID] = 0
}
bankMethodMap[methodID] = methodName
}
}

Expand All @@ -58,33 +61,32 @@ func EVMDenom(token common.Address) string {
}

type BankContract struct {
bankKeeper types.BankKeeper
cdc codec.Codec
kvGasConfig storetypes.GasConfig
BaseContract

bankKeeper types.BankKeeper
cdc codec.Codec
}

// NewBankContract creates the precompiled contract to manage native tokens
func NewBankContract(bankKeeper types.BankKeeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig) vm.PrecompiledContract {
return &BankContract{bankKeeper, cdc, kvGasConfig}
func NewBankContract(bankKeeper types.BankKeeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig, logger log.Logger) vm.PrecompiledContract {
return &BankContract{
BaseContract: NewBaseContract(
bankContractAddress,
kvGasConfig,
bankMethodMap,
bankGasRequiredByMethod,
false,
logger.With("precompiles", "bank"),
),
cdc: cdc,
bankKeeper: bankKeeper,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The BankContract struct is updated to embed the BaseContract struct. The kvGasConfig field is removed from BankContract as it is now part of BaseContract. The NewBankContract function is updated to initialize the BaseContract with additional parameters including the contract address, gas configuration, method map, gas required by method, a boolean indicating if the contract is stateless, and a logger with a specific context. This change enhances the modularity and maintainability of the code by moving common functionality to the BaseContract.

}

func (bc *BankContract) Address() common.Address {
return bankContractAddress
}

Comment on lines 86 to 89
Copy link
Contributor

Choose a reason for hiding this comment

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

The RequiredGas method is removed from the BankContract struct. This is likely because the method is now part of the BaseContract and can be accessed through it. This change reduces code duplication and enhances maintainability.

// RequiredGas calculates the contract gas use
func (bc *BankContract) RequiredGas(input []byte) uint64 {
// base cost to prevent large input size
baseCost := uint64(len(input)) * bc.kvGasConfig.WriteCostPerByte
var methodID [4]byte
copy(methodID[:], input[:4])
requiredGas, ok := bankGasRequiredByMethod[methodID]
if ok {
return requiredGas + baseCost
}
return baseCost
}

func (bc *BankContract) IsStateful() bool {
return true
}
Expand Down
46 changes: 43 additions & 3 deletions x/cronos/keeper/precompiles/base_contract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package precompiles

import (
"github.com/cometbft/cometbft/libs/log"
storetypes "github.com/cosmos/cosmos-sdk/store/types"
"github.com/ethereum/go-ethereum/common"
)

Expand All @@ -10,18 +12,56 @@

type BaseContract interface {
Registrable
RequiredGas(input []byte) uint64
}

type baseContract struct {
address common.Address
address common.Address
kvGasConfig storetypes.GasConfig
nameByMethod map[[4]byte]string
gasByMethod map[[4]byte]uint64
emptyGasIfInputLessThanPrefix bool
logger log.Logger
}

func NewBaseContract(address common.Address) BaseContract {
func NewBaseContract(
address common.Address,
kvGasConfig storetypes.GasConfig,
nameByMethod map[[4]byte]string,
gasByMethod map[[4]byte]uint64,
emptyGasIfInputLessThanPrefix bool,
logger log.Logger,
) BaseContract {
return &baseContract{
address: address,
address,
kvGasConfig,
nameByMethod,
gasByMethod,
emptyGasIfInputLessThanPrefix,
logger,
}
}

func (c *baseContract) RegistryKey() common.Address {
return c.address
}

// RequiredGas calculates the contract gas use
func (c *baseContract) RequiredGas(input []byte) (gas uint64) {
var methodID [4]byte
copy(methodID[:], input[:4])
inputLen := len(input)
defer func() {
method := c.nameByMethod[methodID]
c.logger.Info("required", "gas", gas, "method", method, "len", inputLen)
thomas-nguy marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's this for, temporary debugging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought we want to record required gas for each method

Copy link
Collaborator

Choose a reason for hiding this comment

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

for debugging or what reason?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes debugging, we could collect the data on some nodes to tune the requiredgas map later on

but the data has to be the gas consumed at cosmos level during execution, not the value returned from the map

Copy link
Collaborator

@yihuang yihuang Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see, so this could be a draft PR for building a local binary, no need to merge, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we could use those data on mainnet
https://cronos.yummy.capital/txs/E2B835B672BC046028E3FEBEB1770CD8A506DB505380982AD16C39F13E5DD861

Somehow hermes in mainnet is spending more gas than that. But may be also because of hermez config?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we should have bigger client header like more validators, and I'm not sure why we use TransientGasConfig

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the TransientGasConfig, it inspired from evmos implementation of the gasRequired
https://github.com/evmos/evmos/blob/main/precompiles/common/precompile.go#L34

They also use the TransientGasConfig, I think that make sense since the goal for us is just to add an "extra" fee based on the length of the input, to avoid DDOS attack for very large input

But in practice, the additional fee does not make much sense since input length is not related to the cost of operation in memory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think evmos use KvGasConfig instead and input length does related to TxSizeCost which is the major cost in cosmos tx

Copy link
Collaborator

@thomas-nguy thomas-nguy Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

mmm in the original context, the base cost is just an extra cost to avoid DDOS attack, since the input it never written in store, it would make sense to use TransientGasConfig for the computation.

But I think what you are trying to do is to replicate the cosmos intrinsic cost? In that case KvGasConfig is used

}()
if c.emptyGasIfInputLessThanPrefix && inputLen < 4 {
return
}
// base cost to prevent large input size
gas = uint64(inputLen) * c.kvGasConfig.WriteCostPerByte
Fixed Show fixed Hide fixed
if requiredGas, ok := c.gasByMethod[methodID]; ok {
gas += requiredGas
}
return
}
35 changes: 18 additions & 17 deletions x/cronos/keeper/precompiles/ica.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"math/big"
"time"

"github.com/cometbft/cometbft/libs/log"
storetypes "github.com/cosmos/cosmos-sdk/store/types"

"github.com/cosmos/cosmos-sdk/codec"
Expand Down Expand Up @@ -34,6 +35,7 @@ var (
icaCallbackABI abi.ABI
icaContractAddress = common.BytesToAddress([]byte{102})
icaGasRequiredByMethod = map[[4]byte]uint64{}
icaMethodMap = map[[4]byte]string{}
)

func init() {
Expand All @@ -57,6 +59,7 @@ func init() {
default:
icaGasRequiredByMethod[methodID] = 0
}
icaMethodMap[methodID] = methodName
}
}

Expand All @@ -70,36 +73,34 @@ type IcaContract struct {
cdc codec.Codec
icaauthKeeper types.Icaauthkeeper
cronosKeeper types.CronosKeeper
kvGasConfig storetypes.GasConfig
}

func NewIcaContract(icaauthKeeper types.Icaauthkeeper, cronosKeeper types.CronosKeeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig) vm.PrecompiledContract {
func NewIcaContract(
icaauthKeeper types.Icaauthkeeper,
cronosKeeper types.CronosKeeper,
cdc codec.Codec,
kvGasConfig storetypes.GasConfig,
logger log.Logger,
) vm.PrecompiledContract {
return &IcaContract{
BaseContract: NewBaseContract(icaContractAddress),
BaseContract: NewBaseContract(
icaContractAddress,
kvGasConfig,
icaMethodMap,
icaGasRequiredByMethod,
false,
logger.With("precompiles", "ica"),
),
cdc: cdc,
icaauthKeeper: icaauthKeeper,
cronosKeeper: cronosKeeper,
kvGasConfig: kvGasConfig,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The NewIcaContract function signature has been updated to include two new parameters: kvGasConfig of type storetypes.GasConfig and logger of type log.Logger. The kvGasConfig parameter is used to configure the gas usage for key-value store operations, while the logger parameter is used for logging. The kvGasConfig field has been removed from the IcaContract struct, and the logger field has been added to the BaseContract struct. The RequiredGas function has been removed from the IcaContract and moved to the BaseContract. These changes are part of the effort to enhance the functionality and configurability of the contracts.

- func NewIcaContract(icaauthKeeper types.Icaauthkeeper, cronosKeeper types.CronosKeeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig) vm.PrecompiledContract {
+ func NewIcaContract(icaauthKeeper types.Icaauthkeeper, cronosKeeper types.CronosKeeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig, logger log.Logger) vm.PrecompiledContract {
  	return &IcaContract{
- 		BaseContract:  NewBaseContract(icaContractAddress),
+ 		BaseContract: NewBaseContract(
+ 			icaContractAddress,
+ 			kvGasConfig,
+ 			icaMethodMap,
+ 			icaGasRequiredByMethod,
+ 			false,
+ 			logger.With("precompiles", "ica"),
+ 		),
  		cdc:           cdc,
  		icaauthKeeper: icaauthKeeper,
  		cronosKeeper:  cronosKeeper,
- 		kvGasConfig:   kvGasConfig,
  	}
  }
Committable suggestion (Beta)
Suggested change
func NewIcaContract(
icaauthKeeper types.Icaauthkeeper,
cronosKeeper types.CronosKeeper,
cdc codec.Codec,
kvGasConfig storetypes.GasConfig,
logger log.Logger,
) vm.PrecompiledContract {
return &IcaContract{
BaseContract: NewBaseContract(icaContractAddress),
BaseContract: NewBaseContract(
icaContractAddress,
kvGasConfig,
icaMethodMap,
icaGasRequiredByMethod,
false,
logger.With("precompiles", "ica"),
),
cdc: cdc,
icaauthKeeper: icaauthKeeper,
cronosKeeper: cronosKeeper,
kvGasConfig: kvGasConfig,
}
}
func NewIcaContract(
icaauthKeeper types.Icaauthkeeper,
cronosKeeper types.CronosKeeper,
cdc codec.Codec,
kvGasConfig storetypes.GasConfig,
logger log.Logger,
) vm.PrecompiledContract {
return &IcaContract{
BaseContract: NewBaseContract(
icaContractAddress,
kvGasConfig,
icaMethodMap,
icaGasRequiredByMethod,
false,
logger.With("precompiles", "ica"),
),
cdc: cdc,
icaauthKeeper: icaauthKeeper,
cronosKeeper: cronosKeeper,
}
}


func (ic *IcaContract) Address() common.Address {
return icaContractAddress
}

// RequiredGas calculates the contract gas use
func (ic *IcaContract) RequiredGas(input []byte) uint64 {
// base cost to prevent large input size
baseCost := uint64(len(input)) * ic.kvGasConfig.WriteCostPerByte
var methodID [4]byte
copy(methodID[:], input[:4])
requiredGas, ok := icaGasRequiredByMethod[methodID]
if ok {
return requiredGas + baseCost
}
return baseCost
}

func (ic *IcaContract) IsStateful() bool {
return true
}
Expand Down
87 changes: 47 additions & 40 deletions x/cronos/keeper/precompiles/relayer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,77 +3,84 @@
import (
"encoding/binary"
"errors"
"fmt"

storetypes "github.com/cosmos/cosmos-sdk/store/types"

"github.com/cosmos/cosmos-sdk/codec"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/vm"

"github.com/cometbft/cometbft/libs/log"
ibckeeper "github.com/cosmos/ibc-go/v7/modules/core/keeper"
cronosevents "github.com/crypto-org-chain/cronos/v2/x/cronos/events"
)

var (
relayerContractAddress = common.BytesToAddress([]byte{101})
relayerGasRequiredByMethod = map[int]uint64{}
relayerGasRequiredByMethod = map[[4]byte]uint64{}
relayerMethodMap = map[[4]byte]string{}
)

func assignMethodGas(prefix int, gas uint64) {
data := make([]byte, 4)
binary.LittleEndian.PutUint32(data, uint32(prefix))

Check failure

Code scanning / gosec

Potential integer overflow by integer type conversion Error

Potential integer overflow by integer type conversion
var id [4]byte
copy(id[:], data[:4])
relayerMethodMap[id] = fmt.Sprintf("%d", prefix)
relayerGasRequiredByMethod[id] = gas
}
Comment on lines +27 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

The assignMethodGas function is well implemented. It populates the relayerMethodMap and relayerGasRequiredByMethod maps with the correct values. However, there is a potential integer overflow issue on line 29 as pointed out by the bot in the previous comments. You should handle this potential overflow.

binary.LittleEndian.PutUint32(data, uint32(prefix))


func init() {
relayerGasRequiredByMethod[prefixCreateClient] = 200000
relayerGasRequiredByMethod[prefixUpdateClient] = 400000
relayerGasRequiredByMethod[prefixUpgradeClient] = 400000
relayerGasRequiredByMethod[prefixSubmitMisbehaviour] = 100000
relayerGasRequiredByMethod[prefixConnectionOpenInit] = 100000
relayerGasRequiredByMethod[prefixConnectionOpenTry] = 100000
relayerGasRequiredByMethod[prefixConnectionOpenAck] = 100000
relayerGasRequiredByMethod[prefixConnectionOpenConfirm] = 100000
relayerGasRequiredByMethod[prefixChannelOpenInit] = 100000
relayerGasRequiredByMethod[prefixChannelOpenTry] = 100000
relayerGasRequiredByMethod[prefixChannelOpenAck] = 100000
relayerGasRequiredByMethod[prefixChannelOpenConfirm] = 100000
relayerGasRequiredByMethod[prefixRecvPacket] = 250000
relayerGasRequiredByMethod[prefixAcknowledgement] = 250000
relayerGasRequiredByMethod[prefixTimeout] = 100000
relayerGasRequiredByMethod[prefixTimeoutOnClose] = 100000
assignMethodGas(prefixCreateClient, 200000)
assignMethodGas(prefixUpdateClient, 400000)
assignMethodGas(prefixUpgradeClient, 400000)
assignMethodGas(prefixSubmitMisbehaviour, 100000)
assignMethodGas(prefixConnectionOpenInit, 100000)
assignMethodGas(prefixConnectionOpenTry, 100000)
assignMethodGas(prefixConnectionOpenAck, 100000)
assignMethodGas(prefixConnectionOpenConfirm, 100000)
assignMethodGas(prefixChannelOpenInit, 100000)
assignMethodGas(prefixChannelOpenTry, 100000)
assignMethodGas(prefixChannelOpenAck, 100000)
assignMethodGas(prefixChannelOpenConfirm, 100000)
assignMethodGas(prefixRecvPacket, 250000)
assignMethodGas(prefixAcknowledgement, 250000)
assignMethodGas(prefixTimeout, 100000)
assignMethodGas(prefixTimeoutOnClose, 100000)
}

type RelayerContract struct {
BaseContract

cdc codec.Codec
ibcKeeper *ibckeeper.Keeper
kvGasConfig storetypes.GasConfig
cdc codec.Codec
ibcKeeper *ibckeeper.Keeper
}

func NewRelayerContract(ibcKeeper *ibckeeper.Keeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig) vm.PrecompiledContract {
func NewRelayerContract(
ibcKeeper *ibckeeper.Keeper,
cdc codec.Codec,
kvGasConfig storetypes.GasConfig,
logger log.Logger,
) vm.PrecompiledContract {
return &RelayerContract{
BaseContract: NewBaseContract(relayerContractAddress),
ibcKeeper: ibcKeeper,
cdc: cdc,
kvGasConfig: kvGasConfig,
BaseContract: NewBaseContract(
relayerContractAddress,
kvGasConfig,
relayerMethodMap,
relayerGasRequiredByMethod,
true,
logger.With("precompiles", "relayer"),
),
ibcKeeper: ibcKeeper,
cdc: cdc,
}
}

func (bc *RelayerContract) Address() common.Address {
return relayerContractAddress
}

// RequiredGas calculates the contract gas use
func (bc *RelayerContract) RequiredGas(input []byte) uint64 {
// base cost to prevent large input size
baseCost := uint64(len(input)) * bc.kvGasConfig.WriteCostPerByte
if len(input) < prefixSize4Bytes {
return 0
}
prefix := int(binary.LittleEndian.Uint32(input[:prefixSize4Bytes]))
requiredGas, ok := relayerGasRequiredByMethod[prefix]
if ok {
return requiredGas + baseCost
}
return baseCost
}

func (bc *RelayerContract) IsStateful() bool {
return true
}
Expand Down
Loading