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

Revert "Gaslimit + gascap flags" #1717

Closed
wants to merge 1 commit 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
3 changes: 0 additions & 3 deletions .github/workflows/build-pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@ jobs:
integration/.build/noderunner/noderunner-*.txt
integration/.build/wallet_extension/wal-ext-*.txt
integration/.build/eth2/*
integration/.build/faucet/*
integration/.build/obscuroscan/*
integration/.build/tengateway/*
retention-days: 1


Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {
return l2Network.deployments.execute("CrossChainMessenger", {
from: l2Accounts.deployer,
log: true,
gasLimit: 2_500_000
gasLimit: 1024_000_000
}, "relayMessage", msg);
};

Expand Down
6 changes: 2 additions & 4 deletions go/config/enclave_cli_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ const (
MaxRollupSizeFlag = "maxRollupSize"
L2BaseFeeFlag = "l2BaseFee"
L2CoinbaseFlag = "l2Coinbase"
GasBatchExecutionLimit = "gasBatchExecutionLimit"
GasLocalExecutionCapFlag = "gasLocalExecutionCap"
L2GasLimitFlag = "l2GasLimit"
)

// EnclaveFlags are the flags that the enclave can receive
Expand All @@ -55,14 +54,13 @@ var EnclaveFlags = map[string]*flag.TenFlag{
MaxRollupSizeFlag: flag.NewUint64Flag(MaxRollupSizeFlag, 1024*64, "The maximum size a rollup is allowed to reach"),
L2BaseFeeFlag: flag.NewUint64Flag(L2BaseFeeFlag, 1, ""),
L2CoinbaseFlag: flag.NewStringFlag(L2CoinbaseFlag, "0xd6C9230053f45F873Cb66D8A02439380a37A4fbF", ""),
GasBatchExecutionLimit: flag.NewUint64Flag(GasBatchExecutionLimit, 3_000_000, "Max gas that can be executed in a single batch"),
L2GasLimitFlag: flag.NewUint64Flag(L2GasLimitFlag, 9e18, ""),
ObscuroGenesisFlag: flag.NewStringFlag(ObscuroGenesisFlag, "", "The json string with the obscuro genesis"),
L1ChainIDFlag: flag.NewInt64Flag(L1ChainIDFlag, 1337, "An integer representing the unique chain id of the Ethereum chain used as an L1 (default 1337)"),
ObscuroChainIDFlag: flag.NewInt64Flag(ObscuroChainIDFlag, 443, "An integer representing the unique chain id of the Obscuro chain (default 443)"),
UseInMemoryDBFlag: flag.NewBoolFlag(UseInMemoryDBFlag, true, "Whether the enclave will use an in-memory DB rather than persist data"),
ProfilerEnabledFlag: flag.NewBoolFlag(ProfilerEnabledFlag, false, "Runs a profiler instance (Defaults to false)"),
DebugNamespaceEnabledFlag: flag.NewBoolFlag(DebugNamespaceEnabledFlag, false, "Whether the debug namespace is enabled"),
GasLocalExecutionCapFlag: flag.NewUint64Flag(GasLocalExecutionCapFlag, 3_000_000, "Max gas usage when executing local transactions"),
}

// enclaveRestrictedFlags are the flags that the enclave can receive ONLY over the Ego signed enclave.json
Expand Down
10 changes: 4 additions & 6 deletions go/config/enclave_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,9 @@ type EnclaveConfig struct {
// to include a transaction if it goes above it
MaxRollupSize uint64

GasPaymentAddress gethcommon.Address
BaseFee *big.Int
GasBatchExecutionLimit uint64
GasLocalExecutionCapFlag uint64
GasPaymentAddress gethcommon.Address
BaseFee *big.Int
GasLimit *big.Int
Comment on lines +68 to +70
Copy link

Choose a reason for hiding this comment

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

The consolidation of GasBatchExecutionLimit and GasLocalExecutionCapFlag into a single GasLimit field simplifies the configuration. Ensure that all usages of the old fields are updated to use GasLimit.

}

func NewConfigFromFlags(cliFlags map[string]*flag.TenFlag) (*EnclaveConfig, error) {
Expand Down Expand Up @@ -185,8 +184,7 @@ func newConfig(flags map[string]*flag.TenFlag) (*EnclaveConfig, error) {
cfg.MaxRollupSize = flags[MaxRollupSizeFlag].Uint64()
cfg.BaseFee = big.NewInt(0).SetUint64(flags[L2BaseFeeFlag].Uint64())
cfg.GasPaymentAddress = gethcommon.HexToAddress(flags[L2CoinbaseFlag].String())
cfg.GasBatchExecutionLimit = flags[GasBatchExecutionLimit].Uint64()
cfg.GasLocalExecutionCapFlag = flags[GasLocalExecutionCapFlag].Uint64()
cfg.GasLimit = big.NewInt(0).SetUint64(flags[L2GasLimitFlag].Uint64())

return cfg, nil
}
6 changes: 3 additions & 3 deletions go/config/enclave_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func TestCLIFlagTypes(t *testing.T) {
err = flag.CommandLine.Set(MinGasPriceFlag, "3333")
require.NoError(t, err)

err = flag.CommandLine.Set(GasBatchExecutionLimit, "222222")
err = flag.CommandLine.Set(L2GasLimitFlag, "222222")
require.NoError(t, err)

flag.Parse()
Expand All @@ -50,7 +50,7 @@ func TestCLIFlagTypes(t *testing.T) {
require.Equal(t, true, flags[WillAttestFlag].Bool())
require.Equal(t, 123, flags[LogLevelFlag].Int())
require.Equal(t, int64(3333), flags[MinGasPriceFlag].Int64())
require.Equal(t, uint64(222222), flags[GasBatchExecutionLimit].Uint64())
require.Equal(t, uint64(222222), flags[L2GasLimitFlag].Uint64())

enclaveConfig, err := newConfig(flags)
require.NoError(t, err)
Expand All @@ -59,7 +59,7 @@ func TestCLIFlagTypes(t *testing.T) {
require.Equal(t, true, enclaveConfig.WillAttest)
require.Equal(t, 123, enclaveConfig.LogLevel)
require.Equal(t, big.NewInt(3333), enclaveConfig.MinGasPrice)
require.Equal(t, uint64(222222), enclaveConfig.GasBatchExecutionLimit)
require.Equal(t, big.NewInt(222222), enclaveConfig.GasLimit)
}

func TestRestrictedMode(t *testing.T) {
Expand Down
25 changes: 9 additions & 16 deletions go/enclave/components/batch_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ type batchExecutor struct {

// stateDBMutex - used to protect calls to stateDB.Commit as it is not safe for async access.
stateDBMutex sync.Mutex

batchGasLimit uint64 // max execution gas allowed in a batch
}

func NewBatchExecutor(
Expand All @@ -52,7 +50,6 @@ func NewBatchExecutor(
genesis *genesis.Genesis,
gasOracle gas.Oracle,
chainConfig *params.ChainConfig,
batchGasLimit uint64,
logger gethlog.Logger,
) BatchExecutor {
return &batchExecutor{
Expand All @@ -63,7 +60,6 @@ func NewBatchExecutor(
logger: logger,
gasOracle: gasOracle,
stateDBMutex: sync.Mutex{},
batchGasLimit: batchGasLimit,
}
}

Expand Down Expand Up @@ -305,12 +301,18 @@ func (executor *batchExecutor) CreateGenesisState(
timeNow uint64,
coinbase gethcommon.Address,
baseFee *big.Int,
gasLimit *big.Int,
) (*core.Batch, *types.Transaction, error) {
preFundGenesisState, err := executor.genesis.GetGenesisRoot(executor.storage)
if err != nil {
return nil, nil, err
}

limit := params.MaxGasLimit / 6
if gasLimit != nil {
limit = gasLimit.Uint64()
}

genesisBatch := &core.Batch{
Header: &common.BatchHeader{
ParentHash: common.L2BatchHash{},
Expand All @@ -324,7 +326,7 @@ func (executor *batchExecutor) CreateGenesisState(
Time: timeNow,
Coinbase: coinbase,
BaseFee: baseFee,
GasLimit: executor.batchGasLimit,
GasLimit: limit, // todo (@siliev) - does the batch header need uint64?
},
Transactions: []*common.L2Tx{},
}
Expand Down Expand Up @@ -409,17 +411,8 @@ func (executor *batchExecutor) processTransactions(
var executedTransactions []*common.L2Tx
var excludedTransactions []*common.L2Tx
var txReceipts []*types.Receipt
txResults := evm.ExecuteTransactions(
txs,
stateDB,
batch.Header,
executor.storage,
cc,
tCount,
noBaseFee,
executor.batchGasLimit,
executor.logger,
)

txResults := evm.ExecuteTransactions(txs, stateDB, batch.Header, executor.storage, cc, tCount, noBaseFee, executor.logger)
for _, tx := range txs {
result, f := txResults[tx.Hash()]
if !f {
Expand Down
2 changes: 1 addition & 1 deletion go/enclave/components/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ type BatchExecutor interface {
// CreateGenesisState - will create and commit the genesis state in the stateDB for the given block hash,
// and uint64 timestamp representing the time now. In this genesis state is where one can
// find preallocated funds for faucet. TODO - make this an option
CreateGenesisState(common.L1BlockHash, uint64, gethcommon.Address, *big.Int) (*core.Batch, *types.Transaction, error)
CreateGenesisState(common.L1BlockHash, uint64, gethcommon.Address, *big.Int, *big.Int) (*core.Batch, *types.Transaction, error)
}

type BatchRegistry interface {
Expand Down
1 change: 1 addition & 0 deletions go/enclave/components/rollup_compression.go
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ func (rc *RollupCompression) executeAndSaveIncompleteBatches(calldataRollupHeade
incompleteBatch.time,
calldataRollupHeader.Coinbase,
calldataRollupHeader.BaseFee,
big.NewInt(0).SetUint64(calldataRollupHeader.GasLimit),
)
if err != nil {
return err
Expand Down
4 changes: 2 additions & 2 deletions go/enclave/crosschain/message_bus_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func (m *MessageBusManager) GenerateMessageBusDeployTx() (*common.L2Tx, error) {
tx := &types.LegacyTx{
Nonce: 0, // The first transaction of the owner identity should always be deploying the contract
Value: gethcommon.Big0,
Gas: 2_000_000, // It's quite the expensive contract.
Gas: 5_000_000, // It's quite the expensive contract.
GasPrice: gethcommon.Big0, // Synthetic transactions are on the house. Or the house.
Data: gethcommon.FromHex(MessageBus.MessageBusMetaData.Bin),
To: nil, // Geth requires nil instead of gethcommon.Address{} which equates to zero address in order to return receipt.
Expand Down Expand Up @@ -219,7 +219,7 @@ func (m *MessageBusManager) CreateSyntheticTransactions(messages common.CrossCha
tx := &types.LegacyTx{
Nonce: startingNonce + uint64(idx),
Value: gethcommon.Big0,
Gas: 2_000_000,
Gas: 5_000_000,
GasPrice: gethcommon.Big0, // Synthetic transactions are on the house. Or the house.
Data: data,
To: m.messageBusAddress,
Expand Down
16 changes: 11 additions & 5 deletions go/enclave/enclave.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ type enclaveImpl struct {
service nodetype.NodeType
registry components.BatchRegistry

// todo (#627) - use the ethconfig.Config instead
GlobalGasCap uint64 // 5_000_000_000, // todo (#627) - make config
BaseFee *big.Int // gethcommon.Big0,

mgmtContractLib mgmtcontractlib.MgmtContractLib
attestationProvider components.AttestationProvider // interface for producing attestation reports and verifying them

Expand Down Expand Up @@ -180,7 +184,7 @@ func NewEnclave(

gasOracle := gas.NewGasOracle()
blockProcessor := components.NewBlockProcessor(storage, crossChainProcessors, gasOracle, logger)
batchExecutor := components.NewBatchExecutor(storage, crossChainProcessors, genesis, gasOracle, chainConfig, config.GasBatchExecutionLimit, logger)
batchExecutor := components.NewBatchExecutor(storage, crossChainProcessors, genesis, gasOracle, chainConfig, logger)
sigVerifier, err := components.NewSignatureValidator(config.SequencerID, storage)
registry := components.NewBatchRegistry(storage, logger)
rProducer := components.NewRollupProducer(config.SequencerID, storage, registry, logger)
Expand Down Expand Up @@ -218,7 +222,7 @@ func NewEnclave(
MaxBatchSize: config.MaxBatchSize,
MaxRollupSize: config.MaxRollupSize,
GasPaymentAddress: config.GasPaymentAddress,
BatchGasLimit: config.GasBatchExecutionLimit,
BatchGasLimit: config.GasLimit,
BaseFee: config.BaseFee,
},
blockchain,
Expand All @@ -233,7 +237,6 @@ func NewEnclave(
genesis,
logger,
registry,
config.GasLocalExecutionCapFlag,
)

// ensure cached chain state data is up-to-date using the persisted batch data
Expand Down Expand Up @@ -272,6 +275,9 @@ func NewEnclave(
registry: registry,
service: service,

GlobalGasCap: 5_000_000_000, // todo (#627) - make config
BaseFee: gethcommon.Big0,
Comment on lines +278 to +279
Copy link

Choose a reason for hiding this comment

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

The GlobalGasCap and BaseFee fields are initialized with hardcoded values. Consider making these configurable through the config object or environment variables to avoid recompilation for changes.


mainMutex: sync.Mutex{},
}
}
Expand Down Expand Up @@ -1022,7 +1028,7 @@ func (e *enclaveImpl) EstimateGas(encryptedParams common.EncryptedParamsEstimate
return responses.AsEncryptedError(err, vkHandler), nil
}

executionGasEstimate, err := e.DoEstimateGas(callMsg, blockNumber, e.config.GasLocalExecutionCapFlag)
executionGasEstimate, err := e.DoEstimateGas(callMsg, blockNumber, e.GlobalGasCap)
if err != nil {
err = fmt.Errorf("unable to estimate transaction - %w", err)

Expand Down Expand Up @@ -1149,7 +1155,7 @@ func (e *enclaveImpl) DoEstimateGas(args *gethapi.TransactionArgs, blkNumber *ge
}
hi = block.GasLimit()
*/
hi = e.config.GasLocalExecutionCapFlag
hi = e.GlobalGasCap
}
// Normalize the max fee per gas the call is willing to spend.
var feeCap *big.Int
Comment on lines +1158 to 1161
Copy link

Choose a reason for hiding this comment

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

The DoEstimateGas method has a high cognitive complexity. Consider refactoring to simplify the logic and improve readability. Additionally, address the TODO comment regarding the review of gas mechanics/tokenomics.

Expand Down
19 changes: 8 additions & 11 deletions go/enclave/evm/evm_facade.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package evm
import (
"errors"
"fmt"
"math"
"math/big"

"github.com/ethereum/go-ethereum/accounts/abi"
Expand Down Expand Up @@ -37,11 +38,9 @@ func ExecuteTransactions(
chainConfig *params.ChainConfig,
fromTxIndex int,
noBaseFee bool,
batchGasLimit uint64,
logger gethlog.Logger,
) map[common.TxHash]interface{} {
chain, vmCfg := initParams(storage, noBaseFee, logger)
gp := gethcore.GasPool(batchGasLimit)
chain, vmCfg, gp := initParams(storage, noBaseFee, logger)
zero := uint64(0)
usedGas := &zero
result := map[common.TxHash]interface{}{}
Expand All @@ -58,7 +57,7 @@ func ExecuteTransactions(
s,
chainConfig,
chain,
&gp,
gp,
ethHeader,
t,
usedGas,
Expand Down Expand Up @@ -158,7 +157,6 @@ func ExecuteObsCall(
header *common.BatchHeader,
storage storage.Storage,
chainConfig *params.ChainConfig,
gasEstimationCap uint64,
logger gethlog.Logger,
) (*gethcore.ExecutionResult, error) {
noBaseFee := true
Expand All @@ -168,9 +166,7 @@ func ExecuteObsCall(

defer core.LogMethodDuration(logger, measure.NewStopwatch(), "evm_facade.go:ObsCall()")

gp := gethcore.GasPool(gasEstimationCap)
gp.SetGas(gasEstimationCap)
chain, vmCfg := initParams(storage, noBaseFee, nil)
chain, vmCfg, gp := initParams(storage, noBaseFee, nil)
ethHeader, err := gethencoding.CreateEthHeaderForBatch(header, secret(storage))
if err != nil {
return nil, err
Expand All @@ -181,7 +177,7 @@ func ExecuteObsCall(
txContext := gethcore.NewEVMTxContext(msg)
vmenv := vm.NewEVM(blockContext, txContext, s, chainConfig, vmCfg)

result, err := gethcore.ApplyMessage(vmenv, msg, &gp)
result, err := gethcore.ApplyMessage(vmenv, msg, gp)
// Follow the same error check structure as in geth
// 1 - vmError / stateDB err check
// 2 - evm.Cancelled() todo (#1576) - support the ability to cancel function call if it takes too long
Expand All @@ -206,11 +202,12 @@ func ExecuteObsCall(
return result, nil
}

func initParams(storage storage.Storage, noBaseFee bool, l gethlog.Logger) (*ObscuroChainContext, vm.Config) {
func initParams(storage storage.Storage, noBaseFee bool, l gethlog.Logger) (*ObscuroChainContext, vm.Config, *gethcore.GasPool) {
vmCfg := vm.Config{
NoBaseFee: noBaseFee,
}
return NewObscuroChainContext(storage, l), vmCfg
gp := gethcore.GasPool(math.MaxUint64)
return NewObscuroChainContext(storage, l), vmCfg, &gp
}

// todo (#1053) - this is currently just returning the shared secret
Expand Down
17 changes: 7 additions & 10 deletions go/enclave/l2chain/l2_chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ type obscuroChain struct {

logger gethlog.Logger

Registry components.BatchRegistry
gasEstimationCap uint64
Registry components.BatchRegistry
}

func NewChain(
Expand All @@ -44,15 +43,13 @@ func NewChain(
genesis *genesis.Genesis,
logger gethlog.Logger,
registry components.BatchRegistry,
gasEstimationCap uint64,
) ObscuroChain {
return &obscuroChain{
storage: storage,
chainConfig: chainConfig,
logger: logger,
genesis: genesis,
Registry: registry,
gasEstimationCap: gasEstimationCap,
storage: storage,
chainConfig: chainConfig,
logger: logger,
genesis: genesis,
Registry: registry,
}
}

Expand Down Expand Up @@ -147,7 +144,7 @@ func (oc *obscuroChain) ObsCallAtBlock(apiArgs *gethapi.TransactionArgs, blockNu
batch.Header.Root.Hex())
}})

result, err := evm.ExecuteObsCall(callMsg, blockState, batch.Header, oc.storage, oc.chainConfig, oc.gasEstimationCap, oc.logger)
result, err := evm.ExecuteObsCall(callMsg, blockState, batch.Header, oc.storage, oc.chainConfig, oc.logger)
if err != nil {
// also return the result as the result can be evaluated on some errors like ErrIntrinsicGas
return result, err
Expand Down
3 changes: 2 additions & 1 deletion go/enclave/nodetype/sequencer.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type SequencerSettings struct {
MaxBatchSize uint64
MaxRollupSize uint64
GasPaymentAddress gethcommon.Address
BatchGasLimit uint64
BatchGasLimit *big.Int
Copy link

Choose a reason for hiding this comment

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

The BatchGasLimit field type change from uint64 to *big.Int requires careful handling of the new pointer type to avoid nil pointer dereferences. Ensure that all usages of BatchGasLimit check for nil before dereferencing.

BaseFee *big.Int
}

Expand Down Expand Up @@ -138,6 +138,7 @@ func (s *sequencer) createGenesisBatch(block *common.L1Block) error {
uint64(time.Now().Unix()),
s.settings.GasPaymentAddress,
s.settings.BaseFee,
s.settings.BatchGasLimit,
)
if err != nil {
return err
Expand Down
Loading
Loading