fix(forge): determine if broadcasted tx is fixed gas limit using opcodes

[grandizzy]

Motivation

• closes bug: broadcast artifact includes isFixedGasLimit where not expected and causes transactions to have a too high gas limit #11579
• closes bug: derive block gas limit from RPC to prevent erroneous filling of block during simulation #9752
• confirmed closes Transaction failure at first time, but succeed in second time with forge script. #10968 too

Solution

• when broadcast, determine if it is not fixed gas limit by checking that the opcode before CALL is GAS;
• on cheatcode inspector step record if we see GAS opcode; on step_end (where we have access to next opcode) set fixed gas limit to false if CALL

PR Checklist

• Added Tests
• Added Documentation
• Breaking changes

[grandizzy]

@klkvr @zerosnacks pls check and lmk if it makes sense, thank you!

[grandizzy]

can also change and set this to Some(false) and in set_gas_limit_type if none and next opcode is CALL then set to Some(true)

        if interpreter.bytecode.opcode() == op::CALL {
            if self.is_fixed_gas_limit.is_none() {
                // If GAS opcode wasn't seen then mark as fixed gas limit.
                self.is_fixed_gas_limit = Some(true);
            }
        }

[klkvr]

I believe right now if opcode sequence would be GAS ...any sequence of opcodes other than CALL and GAS.. CALL we will still record this as non-fixed gas limit

what we could do is instead call record_gas_limit_opcode in step_end hook, and then in step either unset it or keep if the opcode is CALL

[grandizzy]

implemented in ba1215c but kept GAS followed by CALL check as it seems to me better logic. I updated to track Option<gas_seen, call_seen> and reset on step_end if GAS is not followed by CALL, pls check

[grandizzy]

confirmed it also solves #10968

[klkvr]

I stillf find it a bit hard to understand. isn't step_end only called after call has completed meaning that this check will likely happen too late?

[grandizzy]

I could be wrong but afaik instep_end we already have access to the next opcode but before call, @DaniPopes @rakita could you pls chime in?

[grandizzy]

@klkvr I think I got your comment wrong, so want to go through logic again

• in step we record if gas opcode. If it is a CALL then gas seen is false and will be interpreted as gas limit in call.
• call happens, we check if gas was seen before and call preceded (that is both set to true). We reset the seen recordings if broadcast.
• in step_end we check if next opcode is CALL and set no gas limit (Here we have access to next opcode )

[klkvr]

ah so in step_end interpreter.bytecode.opcode() is already the next opcode?

[grandizzy]

indeed, learnt this when added the arbitrary storage cheatcode which changes target address storage if next opcode is SLOAD

foundry/crates/cheatcodes/src/inspector.rs

Lines 1891 to 1899 in b3ee87d

	/// Generates or copies arbitrary values for storage slots.
	/// Invoked in inspector `step_end` (when the current opcode is not executed), if current opcode
	/// to execute is `SLOAD` and storage slot is cold.
	/// Ensures that in next step (when `SLOAD` opcode is executed) an arbitrary value is returned:
	/// - copies the existing arbitrary storage value (or the new generated one if no value in
	/// cache) from mapped source address to the target address.
	/// - generates arbitrary value and saves it in target address storage.
	#[cold]
	fn arbitrary_storage_end(&mut self, interpreter: &mut Interpreter, ecx: Ecx) {

[DaniPopes]

yeah that makes sense, the instruction pointer is advanced right before executing the instruction itself https://github.com/bluealloy/revm/blob/66e45930905b5c4b505dd328a716384d18eaa689/crates/interpreter/src/interpreter.rs#L286

[klkvr]

lgtm, I believe the logic here is correct

left a nitpick re flag structure but can skip as the change is tiny

[klkvr]

looks like this only really has 3 state None, Some((true, true)) and Some((true, false)). worth checking if we can get rid of e.g one of the bools and/or remove the option entirely

also iiuc is_fixed_gas_limit: Some((true, true)) corresponds to actually a non-fixed gas limit which is a bit tricky to read, so maybe worth changing name/semantics of the flag

[grandizzy]

yeah, I renamed to dynamic_gas_limit_sequence and updated description, hope that clarifies it a little bit. Re option, I still think we need it as the state can be reset to None on both when consuming in call, and also if next opcode is not a CALL... pending one more review to see others opinion