l2geth: allow for the configured max tx gaslimit to be set in the cfg#840
l2geth: allow for the configured max tx gaslimit to be set in the cfg#840
Conversation
🦋 Changeset detectedLatest commit: a6c8bfc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #840 +/- ##
=======================================
Coverage 82.2% 82.2%
=======================================
Files 48 48
Lines 1895 1895
Branches 303 303
=======================================
Hits 1558 1558
Misses 337 337 🚀 New features to boost your workflow:
|
smartcontracts
left a comment
There was a problem hiding this comment.
Were we making use of TARGET_GAS_LIMIT previously? I'm concerned that we might accidentally set this value to something different than ExecutionManager.maxTransactionGasLimit. Is there something we can do to prevent shooting ourselves in the foot here?
|
Also; does this need a changeset? |
This PR overwrites the value from the state dump, we currently have nothing stopping us from using the configured value from differing from the value in the state dump. This is technically safer than before because the values could differ. The remaining problem is that there is a transaction gas limit in the CTC as well that is used to meter L1 to L2 transactions that may be different than the gas limit in the execution manager. There really should be only 1 gas limit config option and setting it in geth is the most flexible.
This does need a changeset, I was on the fence of whether or not this PR should actually get merged as it was put together to help debug other issues. Now I'd like it to be merged so I will add a changeset. The remaining question for me is should the value also be used to set the value in the CTC? |
|
I've added a changeset, the last remaining question is if the same env var should set the storage slot in the CTC as well |
|
It sounds like we will not be setting the CTC storage slot - this must be fixed at the contract level |
…ethereum-optimism#840) * l2geth: allow for the configured max tx gaslimit to be set in the contracts * chore: add changeset
Description
This PR allows the configured gasLimit to be passed through to the contracts state. It was previously depending on the state dump to set the gasLimit in the contracts while the configured value must match the value set in the state dump
Additional context
If this value is misconfigured, it will result in a mismatched state root instantly. I suppose that is better than it eventually resulting in a mismatched state root if it was configured improperly.
I found the storage slot with the command:
After bringing up
l2gethwith the sync service turned offYou can see that the value is set here in the state dump:
optimism/packages/contracts/src/contract-dumps.ts
Line 118 in 52d9e6b
The value that is being set in the state dynamically is here:
optimism/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Line 49 in 52d9e6b
The gaslimit can be configured with:
TARGET_GAS_LIMITor--miner.gastargetand--miner.gaslimitSee:
optimism/l2geth/cmd/utils/flags.go
Line 455 in 52d9e6b
optimism/l2geth/cmd/utils/flags.go
Line 441 in 52d9e6b