feat(tests): Add flag to configure max_gas for fill command

[reedsa]

🗒️ Description

Add flag to configure max_gas for fill command with default of of 30_000_000.
Add fixture for max_gas to use in gas_limit of a Transaction.

🔗 Related Issues

#1434

✅ Checklist

• All: Set appropriate labels for the changes.
• All: Considered squashing commits to improve commit history.
• All: Added an entry to CHANGELOG.md.
• All: Considered updating the online docs in the ./docs/ directory.
• Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
• Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
• Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
• Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

[reedsa]

Are tests that use more than 30_000_000 gas expected to be set to the enormous amounts like in this example. Some values of 50_000_000, etc. in various osaka tests as well.

Should these values remain or should they be set to the max_gas value from the configurable fixture?

Another question: There are gas limits used in the execution command. Are those to remain as is for now?

[reedsa]

@marioevz I like the idea of setting the gas_limit as part of the Environment class. I assume the initial value should just use the flag value via the test fixture somehow? Perhaps a new method on the class similar to set_fork_requirements that is called from make_state_test_fixture?

I noticed there is a default value of 100_000_000_000_000_000 for the gas_limit. Should that value be lowered to the default configurable value of 30_000_000 and allow overrides with the --gas_limit flag?

Once it is in the environment, all tests will change and I'm not certain what effect lowering the value will have. Is this the right approach?

[marioevz]

@marioevz I like the idea of setting the gas_limit as part of the Environment class. I assume the initial value should just use the flag value via the test fixture somehow? Perhaps a new method on the class similar to set_fork_requirements that is called from make_state_test_fixture?

I noticed there is a default value of 100_000_000_000_000_000 for the gas_limit. Should that value be lowered to the default configurable value of 30_000_000 and allow overrides with the --gas_limit flag?

Once it is in the environment, all tests will change and I'm not certain what effect lowering the value will have. Is this the right approach?

I think the default of 100_000_000_000_000_000 is way overblown, and we should be able to lower it and match it to the value we specify as the block limit, and not that many tests should fail.

Doing the environment-as-a-fixture would require some big changes, like the ones we did for the pre-as-a-fixture.
I.e. The environment changes from being something that gets instantiated by the test function (most tests just do Environment()) to something that is passed as a parameter and then read and potentially modified if necessary.

It might not even be necessary that the test function uses the parameter and we could simply pass it from the filler to the spec like we do for the pre here:

execution-spec-tests/src/pytest_plugins/filler/filler.py

Lines 717 to 718 in b48d1dc

	if "pre" not in kwargs:
	kwargs["pre"] = pre

If the test function requires to know about its environment, it will be included in the test function as a parameter:

def test_something(pre: Alloc, env: Environment ...):

And the test could then:

• Read values from its environment, like the gas_limit and adapt the test to only use that at most. If the test uses the environment as read-only, the test can be easily filled (uv run fill) and executed (uv run execute).
• Write values to its environment, like increase the gas_limit to a minimum value because the test requires it as a minimum to execute its test logic, and in this case the test could be filled but not executed (because we cannot modify the environment we are running in when it's a live network).

One change I would do to the current implementation of this PR is rename max_gas to block_gas_limit.

[reedsa]

@marioevz Pushed up my latest changes and had some follow up questions for what we've discussed.

1. I was wondering if it would be useful to have the existing env.gas_limit only used for transactions and add a new env.block_gas_limit variable that is configurable?
2. I am looking at the pytantic models, not totally certain about where validation happens to check that the env has the block_gas_limit value set? So far I have: block_gas_limit: NumberBoundTypeVar = Field(DEFAULT_BLOCK_GAS_LIMIT, alias="currentBlockGasLimit") as an attribute of EnvironmentGeneric with the default set.

[marioevz]

1. I was wondering if it would be useful to have the existing `env.gas_limit` only used for transactions and add a new `env.block_gas_limit` variable that is configurable?

I normally see tx.gas_limit as the limit applied to the transaction and env.gas_limit as the total gas limit in the environment (block) where the transaction will execute. If you set tx.gas_limit=env.gas_limit it should (IMO) be seen as "the tx should consume all the gas that it can from the environment where it runs".

2. I am looking at the pytantic models, not totally certain about where validation happens to check that the env has the `block_gas_limit` value set? So far I have: `block_gas_limit: NumberBoundTypeVar = Field(DEFAULT_BLOCK_GAS_LIMIT, alias="currentBlockGasLimit")` as an attribute of `EnvironmentGeneric` with the default set.

I don't think this validation happens anywhere at the moment, it's simply set to a default max.

As an example, we do a hacky trick to alter the defaults of a transaction by using a TransactionDefaults class:

execution-spec-tests/src/ethereum_test_types/types.py

Lines 578 to 585 in c35b42f

	@dataclass
	class TransactionDefaults:
	"""Default values for transactions."""
	chain_id: int = 1
	gas_price = 10
	max_fee_per_gas = 7
	max_priority_fee_per_gas: int = 0

And then in the fill/execute plugins we import and modify the value to the new defaults for the given execution context:

execution-spec-tests/src/pytest_plugins/execute/execute.py

Lines 192 to 200 in c35b42f

	@pytest.fixture(autouse=True, scope="session")
	def modify_transaction_defaults(
	default_gas_price: int, default_max_fee_per_gas: int, default_max_priority_fee_per_gas: int
	):
	"""Modify transaction defaults to values better suited for live networks."""
	TransactionDefaults.gas_price = default_gas_price
	TransactionDefaults.max_fee_per_gas = default_max_fee_per_gas
	TransactionDefaults.max_priority_fee_per_gas = default_max_priority_fee_per_gas

Maybe this helps for environment too?

[marioevz]

I think it looks very good!

After going through the code, I'm more convinced about the separation between gas_limit and block_gas_limit.

I'll try the changes today locally and potentially merge if it goes well :)

[reedsa]

@marioevz Is it worth removing the env = Environment() setup that is in many of the tests?

I also haven't updated the execute command with a default value. Is that necessary?

[marioevz]

@marioevz Is it worth removing the env = Environment() setup that is in many of the tests?

I was just thinking about that, it's maybe way too many tests using this pattern.

Maybe since it's simpler to just use the EnvironmentDefaults trick in this PR and focus on the ones that directly set Environment(gas_limit=...)?

I also haven't updated the execute command with a default value. Is that necessary?

If we go ahead and skip using a fixture then nothing needs to be changed for execute, but if we end up using the fixture route then we need to define it also in the execute plugin.

[marioevz]

Just to summarize, the two options are:

env fixture

• Tests would have to define the env parameter, and then pass it to state_test.
• Basically all state tests are affected

EnvironmentDefaults

• Only tests that modify gas limit would be affected, and they would need to do:

env = Environment()
...
tx.gas_limit = env.gas_limit  # <- env.gas_limit here automatically contains the --block-max-gas value

• We could detect tests that go over this limit by checking tx.gas_limit > Environment().gas_limit, but for fill it's irrelevant, and we might only need to check this for execute.

[reedsa]

I'm fine with using the EnvironmentFixture instead of the fixture. I'll update that shortly!

[reedsa]

@marioevz I'm not seeing the flag in the help section when I put in the correct description for the flag group parser.getgroup("env", "Arguments defining the environment"). The flag had worked prior when I had it using the same description as the debug group. 😕

With the EnvironmentDefaults approach I am not sure if that's preventing the limit flag from propogating into the fill tests. Maybe I'm missing something or confused about how the flag should work without using a fixture.

[marioevz]

@marioevz I'm not seeing the flag in the help section when I put in the correct description for the flag group parser.getgroup("env", "Arguments defining the environment"). The flag had worked prior when I had it using the same description as the debug group. 😕

With the EnvironmentDefaults approach I am not sure if that's preventing the limit flag from propogating into the fill tests. Maybe I'm missing something or confused about how the flag should work without using a fixture.

Have an idea on how to fix it: I think we should move the flag to the shared plugin.

I'll push a commit in a bit 👍

[marioevz]

@reedsa Added the changes and it seems to be working now.

The nice thing is that with this change, since the default for Environment.gas_limit was drastically reduced, tests that were using excessive gas started failing, and I took the opportunity and reduced the gas limit of many tests where it was unnecessary.

Let me know what you think of the changes.

[reedsa]

Great work on these updates! I think the only update I would make is to the help text and I'll go ahead and push up that change.

Any test that currently just uses the default environment could also stop using env = Environment() to pass into state_test but isn't necessarily hurting anything as is. Looks good to go!

[marioevz]

LGTM, thanks for this!

I'll apply my comments and merge :)