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

l2geth: allow for the configured max tx gaslimit to be set in the cfg #840

Merged
merged 5 commits into from
May 13, 2021

Conversation

tynes
Copy link
Contributor

@tynes tynes commented May 11, 2021

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:

$ seth storage 0xdeaddeaddeaddeaddeaddeaddeaddeaddead0005 4

After bringing up l2geth with the sync service turned off

You can see that the value is set here in the state dump:

maxTransactionGasLimit: 9_000_000,

The value that is being set in the state dynamically is here:

The gaslimit can be configured with: TARGET_GAS_LIMIT or --miner.gastarget and --miner.gaslimit
See:

@changeset-bot
Copy link

changeset-bot bot commented May 11, 2021

🦋 Changeset detected

Latest commit: a6c8bfc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@eth-optimism/l2geth Patch

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

@tynes tynes changed the title l2geth: allow for the configured max tx gaslimit to be set in the con… l2geth: allow for the configured max tx gaslimit to be set in the cfg May 11, 2021
@codecov-commenter
Copy link

codecov-commenter commented May 11, 2021

Codecov Report

Merging #840 (a6c8bfc) into develop (de5e3dc) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #840   +/-   ##
========================================
  Coverage    82.21%   82.21%           
========================================
  Files           48       48           
  Lines         1895     1895           
  Branches       303      303           
========================================
  Hits          1558     1558           
  Misses         337      337           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de5e3dc...a6c8bfc. Read the comment docs.

Copy link
Contributor

@smartcontracts smartcontracts left a comment

Choose a reason for hiding this comment

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

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?

@smartcontracts
Copy link
Contributor

Also; does this need a changeset?

@tynes
Copy link
Contributor Author

tynes commented May 12, 2021

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?

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.

Also; does this need a changeset?

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?

@tynes
Copy link
Contributor Author

tynes commented May 12, 2021

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

@tynes
Copy link
Contributor Author

tynes commented May 12, 2021

It sounds like we will not be setting the CTC storage slot - this must be fixed at the contract level

@tynes tynes merged commit cf3cfe4 into develop May 13, 2021
@tynes tynes deleted the fix/dynamic-gaslimit branch May 13, 2021 21:22
InoMurko pushed a commit to omgnetwork/optimism that referenced this pull request May 25, 2021
…ethereum-optimism#840)

* l2geth: allow for the configured max tx gaslimit to be set in the contracts

* chore: add changeset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants