Pre EIP-2 support

[mothran]

I'm working on adding support to revm for pre-BYZANTIUM spec's and encountered a issue with the library design that I am unsure the right way to solve.

For context, pre EIP-2 / HOMESTEAD (https://eips.ethereum.org/EIPS/eip-2) a transaction that creates a contract and runs out of gas succeeds but creates a zero length / empty contract. Example transaction: https://etherscan.io/tx/0x6e5c7be761ce29049f8d97d5ecde95758c546e9b8121fe421c99730d32e77a47

I initially tried the following (its a little ugly workaround):

diff --git a/crates/revm/src/evm_impl.rs b/crates/revm/src/evm_impl.rs
index bdea4ae..401ed01 100644
--- a/crates/revm/src/evm_impl.rs
+++ b/crates/revm/src/evm_impl.rs
@@ -415,8 +415,22 @@ impl<'a, GSPEC: Spec, DB: Database, const INSPECT: bool> EVMImpl<'a, GSPEC
, DB,
                     let gas_for_code = code.len() as u64 * crate::gas::CODEDEPOSIT;
                     // record code deposit gas cost and check if we are out of gas.
                     if !interp.gas.record_cost(gas_for_code) {
-                        self.data.subroutine.checkpoint_revert(checkpoint);
-                        return (Return::OutOfGas, ret, interp.gas, b);
+                        // EIP-2
+                        if SPEC::enabled(HOMESTEAD) {
+                            self.data.subroutine.checkpoint_revert(checkpoint);
+                            return (Return::OutOfGas, ret, interp.gas, b);
+                        } else {
+                            self.data.subroutine.checkpoint_commit();
+                            let empty_contract = Bytes::new();
+                            let code_hash =
+                                H256::from_slice(Keccak256::digest(&empty_contract).as_slice());
+                            self.data.subroutine.set_code(
+                                created_address,
+                                empty_contract,
+                                code_hash,
+                            );
+                            return (Return::Continue, ret, interp.gas, b);
+                        }
                     }
                 }
                 // if we have enought gas

But during the db.commit() phase it still triggers the Account::is_empty() to be true and the account to be destroyed. Unfortunately both the Account and DatabaseCommit traits don't currently have access to the SPEC. Is there a better place I could try to prevent the contract from being deleted if emulating pre-HOMESTEAD?

Additional Question:

I am interested in full Ethereum chain historic emulation so I am happy to create a PR for pre-BYZANTIUM spec changes I am making, but I wanted to check: is this something you're interested in supporting?

Thanks!

[rakita]

I'm working on adding support to revm for pre-BYZANTIUM spec's and encountered a issue with the library design that I am unsure the right way to solve.

For context, pre EIP-2 / HOMESTEAD (https://eips.ethereum.org/EIPS/eip-2) a transaction that creates a contract and runs out of gas succeeds but creates a zero length / empty contract. Example transaction: https://etherscan.io/tx/0x6e5c7be761ce29049f8d97d5ecde95758c546e9b8121fe421c99730d32e77a47

Thank you for doing this!

I initially tried the following (its a little ugly workaround):

diff --git a/crates/revm/src/evm_impl.rs b/crates/revm/src/evm_impl.rs
index bdea4ae..401ed01 100644
--- a/crates/revm/src/evm_impl.rs
+++ b/crates/revm/src/evm_impl.rs
@@ -415,8 +415,22 @@ impl<'a, GSPEC: Spec, DB: Database, const INSPECT: bool> EVMImpl<'a, GSPEC
, DB,
                     let gas_for_code = code.len() as u64 * crate::gas::CODEDEPOSIT;
                     // record code deposit gas cost and check if we are out of gas.
                     if !interp.gas.record_cost(gas_for_code) {
-                        self.data.subroutine.checkpoint_revert(checkpoint);
-                        return (Return::OutOfGas, ret, interp.gas, b);
+                        // EIP-2
+                        if SPEC::enabled(HOMESTEAD) {
+                            self.data.subroutine.checkpoint_revert(checkpoint);
+                            return (Return::OutOfGas, ret, interp.gas, b);
+                        } else {
+                            self.data.subroutine.checkpoint_commit();
+                            let empty_contract = Bytes::new();
+                            let code_hash =
+                                H256::from_slice(Keccak256::digest(&empty_contract).as_slice());
+                            self.data.subroutine.set_code(
+                                created_address,
+                                empty_contract,
+                                code_hash,
+                            );
+                            return (Return::Continue, ret, interp.gas, b);
+                        }
                     }
                 }
                 // if we have enought gas

This looks okay.

But during the db.commit() phase it still triggers the Account::is_empty() to be true and the account to be destroyed. Unfortunately both the Account and DatabaseCommit traits don't currently have access to the SPEC. Is there a better place I could try to prevent the contract from being deleted if emulating pre-HOMESTEAD?

As this is not something that is going to be valid in newer hardforks, I wouldn't want to touch DatabaseCommit trait for this. What we can do is introduce flag inside CacheDb that we can set in constructor of CacheDb (something like `new_prehomestead) , this will be a little bit hidden and it will do the job.

Additional Question:

I am interested in full Ethereum chain historic emulation so I am happy to create a PR for pre-BYZANTIUM spec changes I am making, but I wanted to check: is this something you're interested in supporting?

Thanks!

Yep, I am interested to have all hardforks supported. pre EIP-2 harfork is probably going to be the biggest one.

[mothran]

Great I will incorporate that change to CacheDb. I have a lot of old transactions to validate before I make a larger PR. But I will keep track of my changes in my fork for the time being: main...mothran:revm:old-forks

One commit that might be worth pulling out directly is the change to the gas calculation of the CALL opcode for calling precompiles, I found it was over charging on gas because it thought a precompile was a "new" address.

[mothran]

Follow up question, I am working on a bug related to calling precompiles and gas fees.

Currently revm appears to treat each CALL opcode to a precompile as loading a new account because the is_empty() check returns true and in host.rs is_new account fee is added. But while emulating through the older chain state, I found that historically it appears the only the first time a precompile is called, the extra 25k gas fee is added. Example transactions:

First usage of the 0x4 precompile:
https://etherscan.io/tx/0x1ae5f96a8b446ddd51e96019472f7a31fcad3d2e774373ecfbb2ba94ff72fc04 (note: the new_address gas fee IS added here)

and the next (non failing) usage of 0x4 precompile:
https://etherscan.io/tx/0x9ce0b79a8b0c6b8297988ac0d0605b184424e6761b7451936ebace6c48e8384b (note: the new_address fee is not added)

I initially tried adding a boolean into the Account struct to mark when a precompile had been called the first time then checking / assigning that boolean in the subroutines.rs load_account_exist() but I was unable to get the account to properly show up as dirty at finalization. Do you happen to have a better way I could go about addressing this gas issue?

[rakita]

I apologise for not responding I wanted to check that precompile "new" address, but it slipped my mind.

That is fun, in essence we dont know if account is called first time (we should add create account fee of 25k) or it is called second time as in both cases account is_empty() :)

And we dont apply 25k new account fee:
https://github.com/wolflo/evm-opcodes/blob/main/gas.md#aa-1-call

Let me take a look tomorrow, to see where is best to add that flag, or how to address it.

edit: btw what do you use to run those transactions?

[mothran]

Awesome! thank you for looking into it, really appreciated.

As for how I am running them, I am effectively replaying blocks/transactions from genesis forward. Using the geth freezer files as a source for the block data. Unfortunately it's inside a larger custom harness that is not public. I assume it would be possible to replicate it using the web3 DB + some iteration tooling to walk the blocks and perform the mining rewards. Or just replaying single transactions with the web3 based DB + pulling TX/block data from the RPC.

If it helps with debugging I could look at making a smaller stand alone tool to run arbitrary TX's from chain history using the web3 DB.

[rakita]

Here is where the gas calculation in the CALL is done on new account:

revm/crates/revm/src/instructions/host.rs

Lines 342 to 354 in 7748718

	let (is_cold, exist) = host.load_account(to);
	let is_new = !exist;
	//let is_cold = false;
	gas!(
	interp,
	gas::call_cost::<SPEC>(
	value,
	is_new,
	is_cold,
	matches!(scheme, CallScheme::Call | CallScheme::CallCode),
	matches!(scheme, CallScheme::Call | CallScheme::StaticCall),
	)
	);

This is probably not relayed only to precompiles but with every account before EIP-2 as I am assuming that they should have the same logic.

We probably need to change Database to address new state, we now have NotExisting, Empty, NotEmpty. This function to return Option:

revm/crates/revm/src/db.rs

Line 20 in 7748718

fn basic(&mut self, address: H160) -> AccountInfo;

and make changes to a subroutine so that we can make difference between NoExisting and Empty:

revm/crates/revm/src/subroutine.rs

Line 489 in 7748718

let acc: Account = db.basic(address).into();

[mothran]

So based on my testing, I have only found precompiles have had this gas difference, I am able to linearly emulate into block 80,000 with my current patches. So far only hitting the precompile's gas costs. I will look at this modification to load account to see if I can make it work. Thanks!

[rakita]

How many blocks did you cover? I would be amazed that these are the only changes needed :D

This change for empty existing accounts is effective until EIP-161 (check c. and d.) where cleanup happened.
And from here it says EIP-161 is included in: Spurious Dragon (block 2675000)

[mothran]

Using the most recent commit to by fork, I was able to emulate though block 76431, checking assert_eq!(revm_gas == receipt.gas) as a spot check. I still need to do deeper validation using a RPC interface + web3 db.

I was able to take a first stab at solving it, but I don't really like my solution so far:
mothran@9f9a7a2

I had to do the additional enum on a account to correctly indicate when a precompile changes and is committed. Because each call_inner performs a transfer operation that uses log_dirty() when calling a precompile, every call to a precompile shows up in the change set. I had to modify is_empty() so the in_memory db would commit changes to the precompile state, unfortunately I think currently the in_memory DB does not correctly commit balance changes to the precompile.

I feel like the current way of tracking if an account is a precompile using the Filth method feels like it might not be the correct way to handle this because I feel like a lot of internal logic is making it into the commit() method of the DB trait.

I did initially try and implement the Option return on the basic() method but I was unable to make it work the way I felt you were trying to describe. So any pointers on my approach or an alternative approach would be helpful!

[rakita]

Yeah, adding precomp_state is not great.

It is hard to tell exactly how to support it, it is a tricky thing to add, I will try to find the time in next few days so that I can focus on code and try to see what is the best way to support it.

[rakita]

@mothran i started enabling past forks in tests, this will cleanup some things. I am currently at Constantinople/Petersburg.

[mothran]

Awesome, thank you so much for the help! I just looked through that branch, let me know if I can provide anything that helps out. I am happy to run my own stress tests but I just have to start at block 0.

[rakita]

It was annoying and a little bit time consuming to add support for all past roks but It was a fruitfully effort and revm is passing all legacy (found here) tests

changes are still in branch: https://github.com/bluealloy/revm/tree/forks
I still need to merge branch with main and do sanity review, should be soonish added to main branch.

[gakonst]

sweet!

[mothran]

Thats great news! I will start testing that branch on my emulation engine to see if I can shake any bugs out.

[mothran]

So far this looks great! I did trigger what looks like a gas bug in transaction: https://etherscan.io/tx/0x251b962f6f782498e33614f0dc89a61848620fbf12b58d1e4735c7cc2d8e164a

REVM gas used: 121,622
but etherscan / chain data receipts have: 121,408

I am working on a tool to use the web3db + rpc node to replay specific transactions in revm but still working out a few bugs to get the gas calculations to match what I am seeing in other tooling.

[rakita]

Yeah, gas calculation for tracing got few edge cases with gas block introduced, here is example of inspector that i used for debugging: https://github.com/bluealloy/revm/blob/main/bins/revme/src/statetest/trace.rs

Most efficient way for debugging is to compare traces to get a clue where the bug is. If you have archive node to get those traces for comparison that is great, if not, we will need to find it somewhere.

[rakita]

Foundry cast run (i think that is how cmd is called) runs trabsaction from mainnet, this could probably be used for debugging.

@gakonst @onbjerg do we have some docs how to call it?

[gakonst]

cast run -v <txhash> [--rpc-url <url> --etherscan-api-key <...>]

[rakita]

as foundry integrated newest revm, we can now run cast. I needed to hardcode FRONTIER but it worked :) rakita/foundry@e26003a

It seems that revm with cast gives correct gas:

~/workspace/github/foundry/cli$ cargo run --release --bin cast run -v 0x251b962f6f782498e33614f0dc89a61848620fbf12b58d1e4735c7cc2d8e164a --rpc-url https://mainnet.infura.io/v3/________
    Finished release [optimized] target(s) in 0.26s
     Running `/home/draganrakita/workspace/github/foundry/target/release/cast run -v 0x251b962f6f782498e33614f0dc89a61848620fbf12b58d1e4735c7cc2d8e164a --rpc-url 'https://mainnet.infura.io/v3/_______'`
Executing previous transactions from the block.
Traces:
  [110772] 0x35e57f2b08596f1946ecd9d31975ec3c2d7e1c1d::setStatus() 
    ├─ [0] 0x0e320219838e859b2f9f18b72e3d4073ca50b37d::fallback{value: 297000000000000000}() 
    │   └─ ← ()
    ├─ [0] 0x0e320219838e859b2f9f18b72e3d4073ca50b37d::fallback{value: 2000000000000000}() 
    │   └─ ← ()
    └─ ← ()


Transaction successfully executed.
Gas used: 121408

This means that problem is probably related to saved accounts, storage. Or maybe some previous accounts didn't get saved correctly after tx execution.

Here is what is cached inside the foundry (at ~/.foundry/cache/rpc/mainnet) when the cast is called, we can probably compare the inputs:
block76432.json.txt

[rakita]

@mothran hey, just checking, do you have any new info on this?

[mothran]

@rakita sorry for the delay, great point about testing in cast! Its mostly a bug on my end then. I am happy to close this issue as resolved and then open more narrow focused tickets if I find them.

[rakita]

@mothran maybe it is the problem in some previous transactions that sets value in wrong way, or maybe it is in your side it is hard to tell, but either way if you find something unusual ping me/open issue. And thank you for getting involved! :)