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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
- [#1232](https://github.com/crypto-org-chain/cronos/pull/1232) Adjust require gas in relayer precompile to be closed with actual consumed.


### Improvements

- [#1223](https://github.com/crypto-org-chain/cronos/pull/1223) Add required gas log in precompiles.

*October 17, 2023*

## v1.1.0-rc1
Expand Down
2 changes: 1 addition & 1 deletion app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ func New(
evmS,
[]vm.PrecompiledContract{
cronosprecompiles.NewRelayerContract(app.IBCKeeper, appCodec, app.Logger()),
cronosprecompiles.NewIcaContract(&app.ICAAuthKeeper, &app.CronosKeeper, appCodec, gasConfig),
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
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.WriteCostPerByte,
bankMethodMap,
bankGasRequiredByMethod,
false,
logger.With("precompiles", "bank"),
),
cdc: cdc,
bankKeeper: bankKeeper,
}
}

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
45 changes: 42 additions & 3 deletions x/cronos/keeper/precompiles/base_contract.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package precompiles

import (
"github.com/cometbft/cometbft/libs/log"
"github.com/ethereum/go-ethereum/common"
)

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

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

type baseContract struct {
address common.Address
address common.Address
writeCostPerByte uint64
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,
writeCostPerByte uint64,
nameByMethod map[[4]byte]string,
gasByMethod map[[4]byte]uint64,
emptyGasIfInputLessThanPrefix bool,
logger log.Logger,
) BaseContract {
return &baseContract{
address: address,
address,
writeCostPerByte,
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.Debug("required", "gas", gas, "method", method, "len", inputLen)
}()
if c.emptyGasIfInputLessThanPrefix && inputLen < 4 {
return
}
// base cost to prevent large input size
gas = uint64(inputLen) * c.writeCostPerByte

Check failure

Code scanning / gosec

Potential integer overflow by integer type conversion Error

Potential integer overflow by integer type conversion
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential integer overflow by integer type conversion. Consider adding a check to prevent overflow.

+ if inputLen > math.MaxUint64/c.writeCostPerByte {
+     // handle the error, possibly return an error
+ }
  gas = uint64(inputLen) * c.writeCostPerByte

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.

Suggested change
gas = uint64(inputLen) * c.writeCostPerByte
if inputLen > math.MaxUint64/c.writeCostPerByte:
# handle the error, possibly return an error
gas = uint64(inputLen) * c.writeCostPerByte

if requiredGas, ok := c.gasByMethod[methodID]; ok {
gas += requiredGas
}
return
}
35 changes: 18 additions & 17 deletions x/cronos/keeper/precompiles/ica.go
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.WriteCostPerByte,
icaMethodMap,
icaGasRequiredByMethod,
false,
logger.With("precompiles", "ica"),
),
cdc: cdc,
icaauthKeeper: icaauthKeeper,
cronosKeeper: cronosKeeper,
kvGasConfig: kvGasConfig,
}
}

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
88 changes: 52 additions & 36 deletions x/cronos/keeper/precompiles/relayer.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import (
"encoding/binary"
"errors"
"fmt"

"github.com/cometbft/cometbft/libs/log"

Expand All @@ -12,34 +13,45 @@
"github.com/ethereum/go-ethereum/core/vm"

authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
ibckeeper "github.com/cosmos/ibc-go/v7/modules/core/keeper"
cronosevents "github.com/crypto-org-chain/cronos/v2/x/cronos/events"
"github.com/crypto-org-chain/cronos/v2/x/cronos/types"
)

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] = 117462
relayerGasRequiredByMethod[prefixUpdateClient] = 111894
relayerGasRequiredByMethod[prefixUpgradeClient] = 400000
relayerGasRequiredByMethod[prefixSubmitMisbehaviour] = 100000
relayerGasRequiredByMethod[prefixConnectionOpenInit] = 19755
relayerGasRequiredByMethod[prefixConnectionOpenTry] = 38468
relayerGasRequiredByMethod[prefixConnectionOpenAck] = 29603
relayerGasRequiredByMethod[prefixConnectionOpenConfirm] = 12865
relayerGasRequiredByMethod[prefixChannelOpenInit] = 68701
relayerGasRequiredByMethod[prefixChannelOpenTry] = 70562
relayerGasRequiredByMethod[prefixChannelOpenAck] = 22127
relayerGasRequiredByMethod[prefixChannelOpenConfirm] = 21190
relayerGasRequiredByMethod[prefixChannelCloseInit] = 100000
relayerGasRequiredByMethod[prefixChannelCloseConfirm] = 31199
relayerGasRequiredByMethod[prefixRecvPacket] = 144025
relayerGasRequiredByMethod[prefixAcknowledgement] = 61781
relayerGasRequiredByMethod[prefixTimeout] = 104283
relayerGasRequiredByMethod[prefixTimeoutOnClose] = 100000
assignMethodGas(prefixCreateClient, 117462)
assignMethodGas(prefixUpdateClient, 111894)
assignMethodGas(prefixUpgradeClient, 400000)
assignMethodGas(prefixSubmitMisbehaviour, 100000)
assignMethodGas(prefixConnectionOpenInit, 19755)
assignMethodGas(prefixConnectionOpenTry, 38468)
assignMethodGas(prefixConnectionOpenAck, 29603)
assignMethodGas(prefixConnectionOpenConfirm, 12865)
assignMethodGas(prefixChannelOpenInit, 68701)
assignMethodGas(prefixChannelOpenTry, 70562)
assignMethodGas(prefixChannelOpenAck, 22127)
assignMethodGas(prefixChannelOpenConfirm, 21190)
assignMethodGas(prefixChannelCloseInit, 100000)
assignMethodGas(prefixChannelCloseConfirm, 31199)
assignMethodGas(prefixRecvPacket, 144025)
assignMethodGas(prefixAcknowledgement, 61781)
assignMethodGas(prefixTimeout, 104283)
assignMethodGas(prefixTimeoutOnClose, 100000)
}

type RelayerContract struct {
Expand All @@ -50,12 +62,24 @@
logger log.Logger
}

func NewRelayerContract(ibcKeeper types.IbcKeeper, cdc codec.Codec, logger log.Logger) vm.PrecompiledContract {
func NewRelayerContract(
ibcKeeper *ibckeeper.Keeper,
cdc codec.Codec,
logger log.Logger,
) vm.PrecompiledContract {
bcLogger := logger.With("precompiles", "relayer")
return &RelayerContract{
BaseContract: NewBaseContract(relayerContractAddress),
ibcKeeper: ibcKeeper,
cdc: cdc,
logger: logger.With("precompiles", "relayer"),
BaseContract: NewBaseContract(
relayerContractAddress,
authtypes.DefaultTxSizeCostPerByte,
relayerMethodMap,
relayerGasRequiredByMethod,
true,
bcLogger,
),
ibcKeeper: ibcKeeper,
cdc: cdc,
logger: bcLogger,
}
}

Expand All @@ -66,24 +90,16 @@
// RequiredGas calculates the contract gas use
// `max(0, len(input) * DefaultTxSizeCostPerByte + requiredGasTable[methodPrefix] - intrinsicGas)`
func (bc *RelayerContract) RequiredGas(input []byte) (gas uint64) {
if len(input) < prefixSize4Bytes {
return 0
}
intrinsicGas, err := core.IntrinsicGas(input, nil, false, true, true)
if err != nil {
return 0
}
prefix := int(binary.LittleEndian.Uint32(input[:prefixSize4Bytes]))
requiredGas, ok := relayerGasRequiredByMethod[prefix]
if !ok {
requiredGas = 0
}
// base cost to prevent large input size
baseCost := uint64(len(input)) * authtypes.DefaultTxSizeCostPerByte

total := bc.BaseContract.RequiredGas(input)
defer func() {
bc.logger.Debug("required", "gas", gas, "method", prefix, "len", len(input), "intrinsic", intrinsicGas)
bc.logger.Debug("required", "gas", gas, "intrinsic", intrinsicGas, "total", total)
}()
total := requiredGas + baseCost

if total < intrinsicGas {
return 0
}
Expand Down
Loading