Skip to content

Str-1184: bump reth to 1.2.0 #773

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.

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

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Conversation

evgenyzdanovich
Copy link
Contributor

@evgenyzdanovich evgenyzdanovich commented Apr 15, 2025

Description

Bump reth to 1.2.0.
Currently in the draft: some fntests are failing.

the issues likely with exex for witness should use serde_bincode_compat which was erroneosly removed by reth:
paradigmxyz/reth#15751

we either have to craft TransactionSigned ourselves (by reusing different bincode compat transactions) or wait the new version that fixes it in reth.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactor
  • New or updated tests
  • Dependency Update

Notes to Reviewers

Checklist

  • I have performed a self-review of my code.
  • I have commented my code where necessary.
  • I have updated the documentation if needed.
  • My changes do not introduce new warnings.
  • I have added (where necessary) tests that prove my changes are effective or that my feature works.
  • New and existing tests pass with my changes.

Related Issues

@storopoli storopoli mentioned this pull request Apr 15, 2025
13 tasks
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

Attention: Patch coverage is 2.51256% with 194 lines in your changes missing coverage. Please review.

Project coverage is 52.84%. Comparing base (2f02feb) to head (9b4a9bc).

Files with missing lines Patch % Lines
crates/reth/node/src/payload_builder.rs 0.00% 113 Missing ⚠️
crates/reth/node/src/evm.rs 0.00% 34 Missing ⚠️
crates/reth/rpc/src/eth/pending_block.rs 0.00% 9 Missing ⚠️
crates/reth/node/src/engine.rs 0.00% 8 Missing ⚠️
crates/reth/rpc/src/eth/block.rs 0.00% 7 Missing ⚠️
crates/reth/rpc/src/eth/transaction.rs 0.00% 6 Missing ⚠️
crates/evmexec/src/http_client.rs 0.00% 5 Missing ⚠️
crates/reth/node/src/payload.rs 0.00% 5 Missing ⚠️
crates/reth/exex/src/prover_exex.rs 0.00% 3 Missing ⚠️
bin/strata-cli/src/cmd/drain.rs 0.00% 1 Missing ⚠️
... and 3 more
@@            Coverage Diff             @@
##             main     #773      +/-   ##
==========================================
+ Coverage   52.74%   52.84%   +0.10%     
==========================================
  Files         300      300              
  Lines       32941    32877      -64     
==========================================
  Hits        17374    17374              
+ Misses      15567    15503      -64     
Files with missing lines Coverage Δ
bin/strata-cli/src/strata.rs 0.00% <ø> (ø)
crates/proof-impl/evm-ee-stf/src/primitives.rs 100.00% <ø> (ø)
crates/proof-impl/evm-ee-stf/src/processor.rs 63.03% <100.00%> (ø)
crates/reth/rpc/src/eth/call.rs 0.00% <ø> (ø)
bin/strata-cli/src/cmd/drain.rs 0.00% <0.00%> (ø)
crates/reth/node/src/node.rs 0.00% <0.00%> (ø)
crates/reth/rpc/src/eth/mod.rs 0.00% <0.00%> (ø)
crates/reth/rpc/src/eth/receipt.rs 0.00% <0.00%> (ø)
crates/reth/exex/src/prover_exex.rs 0.00% <0.00%> (ø)
crates/evmexec/src/http_client.rs 1.92% <0.00%> (+0.10%) ⬆️
... and 7 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@storopoli
Copy link
Member

storopoli commented Apr 15, 2025

the issues likely with exex for witness should use serde_bincode_compat which was erroneosly removed by reth: paradigmxyz/reth#15751

we either have to craft TransactionSigned ourselves (by reusing different bincode compat transactions) or wait the new version that fixes it in reth.

This was fixed in paradigmxyz/reth#15755 and already included in the 1.3.10 release.

Thanks for reporting the issue upstream to reth :)

@evgenyzdanovich
Copy link
Contributor Author

evgenyzdanovich commented Apr 16, 2025

@storopoli tried to bump to 1.3.10 to see the amount of errors. The very first one is:

error: failed to select a version for `c-kzg`.
    ... required by package `alloy-consensus v0.11.0`
    ... which satisfies dependency `alloy-consensus = "^0.11"` of package `alloy-signer-local v0.11.0`
    ... which satisfies dependency `alloy-signer-local = "^0.11"` of package `sp1-sdk v4.1.7`
    ... which satisfies dependency `sp1-sdk = "^4.1.7"` of package `strata-sp1-guest-builder v0.1.0 (/home/jk-qq/dev/alpenlabs/strata/provers/sp1)`
    ... which satisfies path dependency `strata-sp1-guest-builder` (locked to 0.1.0) of package `strata-datatool v0.1.0 (/home/jk-qq/dev/alpenlabs/strata/bin/datatool)`
versions that meet the requirements `^1.0` are: 1.0.3, 1.0.2, 1.0.0

the package `c-kzg` links to the native library `ckzg`, but it conflicts with a previous package which links to `ckzg` as well:
package `c-kzg v2.1.0`
    ... which satisfies dependency `c-kzg = "^2.1"` of package `alloy-consensus v0.14.0`
    ... which satisfies dependency `alloy-consensus = "^0.14"` of package `alloy v0.14.0`
    ... which satisfies dependency `alloy = "^0.14.0"` of package `strata-cli v0.3.0 (/home/jk-qq/dev/alpenlabs/strata/bin/strata-cli)`
Only one package in the dependency graph may specify the same links value. This helps ensure that only one copy of a native library is linked in the final binary. Try to adjust your dependencies so that only one package uses the `links = "ckzg"` value. For more information, see https://doc.rust-lang.org/cargo/reference/resolver.html#links.

Basically, sp1 uses alloy of an older version, which links rust binding ckzg of version 1. reth uses alloy of a newer version that links ckzg of version 2.

Reported: succinctlabs/sp1#2244

@MdTeach MdTeach self-requested a review May 9, 2025 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants