Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

make pr failed of lockfile #9381

Closed
1 task done
jsvisa opened this issue Jul 8, 2024 · 1 comment · Fixed by #9403
Closed
1 task done

make pr failed of lockfile #9381

jsvisa opened this issue Jul 8, 2024 · 1 comment · Fixed by #9403
Labels
C-bug An unexpected or incorrect behavior C-test A change that impacts how or what we test

Comments

@jsvisa
Copy link
Contributor

jsvisa commented Jul 8, 2024

Describe the bug

run make test failed of:

test lockfile::tests::test_drop_lock ... FAILED
test implementation::mdbx::tests::db_iterate_over_all_dup_values ... ok
test implementation::mdbx::tests::db_manual_put_get ... ok
test implementation::mdbx::tests::db_reverse_walker ... ok
test implementation::mdbx::tests::db_walker ... ok
test tables::codecs::fuzz::BlockNumberAddress::fuzz_fuzz::entry ... ok
test implementation::mdbx::tests::db_sharded_key ... ok
test implementation::mdbx::tests::dup_value_with_same_subkey ... ok
test tables::codecs::fuzz::IntegerList::fuzz_fuzz::entry ... ok
test tables::tests::parse_table_from_str ... ok
test implementation::mdbx::tests::db_walk_back ... ok
test utils::tests::is_database_empty_false_if_db_path_is_a_file ... ok
test version::tests::malformed_file ... ok
test version::tests::missing_file ... ok
test version::tests::version_mismatch ... ok
test tests::db_version ... ok
test tables::codecs::fuzz::IntegerList::fuzz_fuzz::auto_generate ... ok
test tables::codecs::fuzz::BlockNumberAddress::fuzz_fuzz::auto_generate ... ok
test lockfile::tests::test_lock ... FAILED
test tables::codecs::fuzz::IntegerList::test ... ok
test tables::codecs::fuzz::BlockNumberAddress::test ... ok
test implementation::mdbx::tx::tests::long_read_transaction_safety_enabled ... ok
test implementation::mdbx::tx::tests::long_read_transaction_safety_disabled ... ok
test tests::db_client_version ... ok

failures:

---- lockfile::tests::test_drop_lock stdout ----
thread 'lockfile::tests::test_drop_lock' panicked at crates/storage/db/src/lockfile.rs:186:9:
assertion failed: lock_file.exists()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- lockfile::tests::test_lock stdout ----
thread 'lockfile::tests::test_lock' panicked at crates/storage/db/src/lockfile.rs:171:9:
assertion `left == right` failed
  left: Err(Taken(1))
 right: Ok(StorageLock(StorageLockInner { file_path: "/var/folders/j2/hsd5xc515qq6x2chgx12s5ch0000gn/T/.tmpmcpYoq/lock" }))


failures:
    lockfile::tests::test_drop_lock
    lockfile::tests::test_lock

test result: FAILED. 39 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.55s

error: test failed, to rerun pass `-p reth-db --lib`

after some dig, it seems like the command cargo test --lib --workspace -p reth-db -- lockfile::tests failed to run, but cargo test --lib -p reth-db -- lockfile::tests is success

Steps to reproduce

make pr

Node logs

No response

Platform(s)

Linux (x86), Mac (Apple Silicon)

What version/commit are you on?

main

What database version are you on?

2

Which chain / network are you on?

mainnet

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
@jsvisa jsvisa added C-bug An unexpected or incorrect behavior S-needs-triage This issue needs to be labelled labels Jul 8, 2024
@mattsse
Copy link
Collaborator

mattsse commented Jul 8, 2024

@joshieDo I believe this happens because both tests are run in the same process in cargo t

we should run them in sequence with serial_test or some other sync mechanism

@mattsse mattsse added C-test A change that impacts how or what we test and removed S-needs-triage This issue needs to be labelled labels Jul 8, 2024
@mattsse mattsse mentioned this issue Jul 9, 2024
Merged
@github-project-automation github-project-automation bot moved this from Todo to Done in Reth Tracker Jul 9, 2024
This was referenced Jul 24, 2024
Merged
Merged
mattsse pushed a commit to paradigmxyz/revm-inspectors that referenced this issue Jul 25, 2024
@ZzPoLariszZ
fix: gas and gasUsed in trace root only for ParityTrace (#171)
9a54f21 
## Description

This pull request is Part 1/2 of fixing the bug where the `gas` and
`gasUsed` fields in Parity Trace root are incorrect.

Part 2/2 paradigmxyz/reth#9761

## Related Issues and Pull Requests

- Follow: ethereum/go-ethereum#27029
- Improve: paradigmxyz/reth#3678 and paradigmxyz/reth#3719
- Fix: paradigmxyz/reth#9142 with #170 
- Update: paradigmxyz/reth#3782

## Problem

The `gas` and `gasUsed` fields in Geth Debug Trace root should be the
gas limit and gas used for the entire transaction.

However, two fields in Parity Trace root should be the original ones.

### Reproducible Example

With the latest version Reth v1.0.3, using `trace_transaction()` to
trace
the transaction
`0x03128677ee3a9623d20f3c677f423ccc592d126374bf32e331343dd1bdf38b61`

```
curl http://localhost:8545 \
  -X POST \
  -H "Content-Type: application/json" \
  --data '{"method":"trace_transaction","params":["0x03128677ee3a9623d20f3c677f423ccc592d126374bf32e331343dd1bdf38b61"],"id":1,"jsonrpc":"2.0"}'  
```

**From Reth**

```
gas: 0x55493 (349331)
gasUsed: 0x32d16 (208150)
```

**From
[Etherscan](https://etherscan.io/vmtrace?txhash=0x03128677ee3a9623d20f3c677f423ccc592d126374bf32e331343dd1bdf38b61&type=parity#raw)
and QuickNode**

```
gas: 0x4f227 (324135)
gasUsed: 0x36622 (222754)
```

## Solution for `revm-inspectors`

1. Not modify `gas_limit` and `gas_used` in the trace root

    ```diff
    - gas_limit = context.env.tx.gas_limit;
    - trace.set_root_trace_gas_used(gas_used);
- trace.gas_used = gas_used(context.spec_id(), gas.spent(),
gas.refunded() as u64);
    ```

2. The modification in Step 1 will cause another problem

The `gas` field for Geth Debug Trace root will also be reset (not the
gas limitation for the entire transaction).

therefore, can define `set_transaction_gas_limit()` and
`with_transaction_gas_limit()` for Geth Debug,

which is similar to current `set_transaction_gas_used()` and
`with_transaction_gas_used()` for Parity.

3. Then, modify the Reth Part: 

`crates/rpc/rpc/src/trace.rs` and `crates/rpc/rpc/src/debug.rs` to
completely fix the bug.

## Miscellaneous

- Actually, I love the current design, but the results are inconsistent
with those of others.

- When I used `make pr` to test the Reth Part, the issue
paradigmxyz/reth#9381 still exists for me.

    I should only skip tests for `lockfile` and test them seperately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-bug An unexpected or incorrect behavior C-test A change that impacts how or what we test
Projects
Reth Tracker
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: add serial lock for lockfile tests paradigmxyz/reth
2 participants
@jsvisa @mattsse