Non-deposit transactions have depositReceiptVersion=1

[0xZerohero]

Describe the bug

All non-deposit transactions are stored in the db with depositReceiptVersion=1, but it should be null.

Steps to reproduce

Find a non-deposit and print it.

> op-reth get TxHashNumber 0xc76c50bee5cffb7e6a4fea31d0fd360369037d26ff010bec2270916d086aa365
96808239
> op-reth db get Receipts 96808239

Node logs

No response

Platform(s)

Linux (x86)

What version/commit are you on?

0.1.0-alpha.17

What database version are you on?

Current database version: 1
Local database version: 1

What type of node are you running?

Archive (default)

What prune config do you use, if any?

No response

If you've built Reth from source, provide the full command you used

No response

Code of Conduct

• I agree to follow the Code of Conduct

[0xZerohero]

It looks like the issue is here:
https://github.com/paradigmxyz/reth/blob/7ef570fc17b3565dbc4668f72288b87e91afdc98/crates/revm/src/optimism/processor.rs#L180C17-L183C35

In op-geth there is an extra check if msg.IsDepositTx:
https://github.com/ethereum-optimism/op-geth/blob/f33198a04ad51e89570d646cf3802a493c93e0b2/core/state_processor.go#L144

[mattsse]

this can be closed @0xZerohero ?

[0xZerohero]

I'll check if it was fixed in the latest version. If not I can make a PR

[0xZerohero]

@mattsse
I'm trying to add unit tests, but I have issues and I'm wondering if I'm adding them in the right place

For example cargo test fails with:

> cargo test -p reth-revm --features "optimism" -- --nocapture --test optimism::processor::tests
   Compiling reth-ethereum-payload-builder v0.1.0-alpha.19 (/home/zerohero/Work/oob/reth-zh/crates/payload/ethereum)
error[E0063]: missing fields `deposit_nonce` and `deposit_receipt_version` in initializer of `Receipt`
   --> crates/payload/ethereum/src/lib.rs:320:32
    |
320 |             receipts.push(Some(Receipt {
    |                                ^^^^^^^ missing `deposit_nonce` and `deposit_receipt_version`

error[E0061]: this method takes 3 arguments but 1 argument was supplied
   --> crates/payload/ethereum/src/lib.rs:355:36
    |
355 |         let receipts_root = bundle.receipts_root_slow(block_number).expect("Number is in range");
    |                                    ^^^^^^^^^^^^^^^^^^-------------- two arguments of type `&ChainSpec` and `u64` are missing
    |
note: method defined here
   --> /home/zerohero/Work/oob/reth-zh/crates/storage/provider/src/bundle_state/bundle_state_with_receipts.rs:175:12
    |
175 |     pub fn receipts_root_slow(
    |            ^^^^^^^^^^^^^^^^^^
help: provide the arguments
    |
355 |         let receipts_root = bundle.receipts_root_slow(block_number, /* &ChainSpec */, /* u64 */).expect("Number is in range");
    |                                                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I can fix that by adding:

--- a/crates/node-ethereum/Cargo.toml
+++ b/crates/node-ethereum/Cargo.toml
@@ -34,4 +34,4 @@ reth-db.workspace = true
 [features]
 # This is a workaround for reth-cli crate to allow this as mandatory dependency without breaking the build even if unused.
 # This makes managing features and testing workspace easier because clippy always builds all members if --workspace is provided
-optimism = []
\ No newline at end of file
+optimism = ["reth-ethereum-payload-builder/optimism"]

cargo check also fails for reth-revm with enabled feature "optimism" and if we want to fix it, there are more Cargo.toml files that have to be fixed.

How to proceed?

1. Fixing them all
2. only for cargo test
3. or I'm doing something completely wrong :)

[mattsse]

yeah, this is kinda fucked rn

I addressed most of this in #6611

there's no good solution atm in this crate,
I guess we want to have them in the node-optimism crate instead because we're already preparing custom evm processors #6461

[0xZerohero]

I'll check it tomorrow(I hope). In node-optimism, it should be an integration test, right? I was hoping to find some similar tests there :)

Btw I managed to put together something working, but it's in the revm::optimism::processor module, and it's a rough draft.
main...0xZerohero:reth:fix/deposit_receipt_version
The fix is the first commit. As u can see, it's just one line, so if you prefer, I can PR only it...

[0xZerohero]

I added TestEvmConfig struct to remove the reth-node-* dependencies. This way we don't have to move the unit tests in another crate. WDYT @mattsse ?

One of the main problems in the current case is that optional dev-dependencies are not allowed, but I found another solution used by tokio. They are passing --cfg option to rustc by using RUSTFLASGS.

For example:

[target.'cfg(optimism)'.dev-dependencies]
reth-node-optimism = { workspace = true, features = ["optimism"] }

[target.'cfg(not(optimism))'.dev-dependencies]
reth-node-ethereum.workspace = true

But this is more general design decision. I still don't fully understand how the code separation between ethereum and optimist is intended. There are ethereum and optimism specific crates and other that are using feature "optimism" for conditional compilation and this leads to problems.