Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: #8759, default (low) gas limit set even when disabled, use custom gas_limit on forks #8933

Merged
merged 8 commits into from
Sep 24, 2024

Conversation

nhtyy
Copy link
Contributor

@nhtyy nhtyy commented Sep 22, 2024

Motivation

#8759 and others I believe were mostly afflicted by the fact that most eth_call requests do not typically fill out the gas field on call requests (including alloy bindings), and if you weren't in forking mode, with disable_block_gas_limit, any incoming call without this field would be limited to 30M gas,

as you can see on line 1209 we try to get the requests gas field to setup the tx env, but its likely not there

max_priority_fee_per_gas,
max_fee_per_blob_gas,
} = fee_details;
let gas_limit = gas.unwrap_or(block_env.gas_limit.to());
let mut env = self.env.read().clone();
env.block = block_env;
// we want to disable this in eth_call, since this is common practice used by other node
// impls and providers <https://github.com/foundry-rs/foundry/issues/4388>
env.cfg.disable_block_gas_limit = true;
// The basefee should be ignored for calls against state for
// - eth_call
// - eth_estimateGas
// - eth_createAccessList
// - tracing
env.cfg.disable_base_fee = true;
let gas_price = gas_price.or(max_fee_per_gas).unwrap_or_else(|| {
self.fees().raw_gas_price().saturating_add(MIN_SUGGESTED_PRIORITY_FEE)
});
let caller = from.unwrap_or_default();
let to = to.as_ref().and_then(TxKind::to);
env.tx = TxEnv {
caller,
gas_limit: gas_limit as u64,
gas_price: U256::from(gas_price),

so on line 1230 we set the tx.env.gas_limit to the block gas limit, however even when we set thedisable_block_gas_limit flag, this block gas limit defaults to 30_000_000 as seen here and here, which can make it seem like the flag is not working if the call youre testing is over 30M gas

Lastly, the fork mode appeared to work (as mentioned in #8759) because we branch if we have a fork, and in that branch we explicitly set the env block gas limit high if the flag is set , which would transitively apply to incoming calls.

// `0x0` which would inevitably result in `OutOfGas` errors as soon as the evm is about to record gas, See also <https://github.com/foundry-rs/foundry/issues/3247>
let gas_limit = if self.disable_block_gas_limit || block.header.gas_limit == 0 {
u64::MAX as u128
} else {
block.header.gas_limit
};
env.block = BlockEnv {
number: U256::from(fork_block_number),
timestamp: U256::from(block.header.timestamp),
difficulty: block.header.difficulty,
// ensures prevrandao is set
prevrandao: Some(block.header.mix_hash.unwrap_or_default()),
gas_limit: U256::from(gas_limit),
// Keep previous `coinbase` and `basefee` value
coinbase: env.block.coinbase,

Solution

In this PR there is breaking change (gas_limit is now an option on NodeConfig) to allow for using custom gas limits in forks, if we dont care about that then it doesnt need to breaking.

If the disable_block_gas_limit flag is set, make sure the block gas limit is high to ensure we dont limit incoming calls (without explicit gas_limit) to the default (low) limit

@nhtyy nhtyy changed the title fix: #8759, default (low) gas limit set when disabled, use custom gas_limit on forks fix: #8759, default (low) gas limit set even when disabled, use custom gas_limit on forks Sep 22, 2024
@@ -1171,9 +1186,10 @@ latest block number: {latest_block}"
// the next block
let next_block_base_fee = fees.get_next_block_base_fee_per_gas(
block.header.gas_used,
block.header.gas_limit,
gas_limit,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure about this

Copy link
Member

Choose a reason for hiding this comment

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

this should be okay, but in case u128 the basefee would be more or less locked to the lowest possible value 7.
I assume this is okay if you disable the gas limit

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I think this makes sense.

could we two more test cases, one fork test for base and one where the gaslimit is disabled via config?

crates/anvil/src/config.rs Outdated Show resolved Hide resolved
@@ -1171,9 +1186,10 @@ latest block number: {latest_block}"
// the next block
let next_block_base_fee = fees.get_next_block_base_fee_per_gas(
block.header.gas_used,
block.header.gas_limit,
gas_limit,
Copy link
Member

Choose a reason for hiding this comment

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

this should be okay, but in case u128 the basefee would be more or less locked to the lowest possible value 7.
I assume this is okay if you disable the gas limit

@nhtyy nhtyy force-pushed the n/fix-8759 branch 2 times, most recently from 7e7d172 to c2c4da6 Compare September 24, 2024 03:07
@nhtyy
Copy link
Contributor Author

nhtyy commented Sep 24, 2024

@mattsse thanks, I added a test for base, and some potential tests for setting/disabling the gas limits on both forks and local

@nhtyy nhtyy force-pushed the n/fix-8759 branch 3 times, most recently from 1f209f9 to a27652c Compare September 24, 2024 03:38
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

last nits

crates/anvil/src/eth/backend/mem/mod.rs Outdated Show resolved Hide resolved
crates/anvil/src/config.rs Outdated Show resolved Hide resolved
crates/anvil/tests/it/fork.rs Outdated Show resolved Hide resolved
@nhtyy nhtyy force-pushed the n/fix-8759 branch 3 times, most recently from 5daa35c to 7662cbf Compare September 24, 2024 12:14
@mattsse mattsse merged commit 81fb0f6 into foundry-rs:master Sep 24, 2024
20 checks passed
rplusq pushed a commit to rplusq/foundry that referenced this pull request Sep 25, 2024
… use custom gas_limit on forks (foundry-rs#8933)

* fix: foundry-rs#8759, do not set low gas price on block if disabled, use custom gas price in forks

* test(fix): default block gas limit for large mine test

* fix fmt

* fix: optional gas_limit in as_json

* fix: use option not serde_json::Value::Null

* tests: base tests + config tests

* fix: nits

* fix: comment
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants