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.

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

Str-711: bump RETH (and deps) to the latest version. #542

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

Conversation

evgenyzdanovich
Copy link
Contributor

@evgenyzdanovich evgenyzdanovich commented Dec 14, 2024

Description

Str-711: bump RETH (and deps) to the latest version. Also, fix lints.

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

This is a preparation work needed to remove boa from the dependency graph. And turn cargo-audit back on.

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 tests that prove my changes are effective or that my feature works.
  • New and existing tests pass with my changes.

Related Issues

EDIT(@storopoli): Closes https://github.com/alpenlabs/strata/security/dependabot/12

@evgenyzdanovich evgenyzdanovich requested review from a team as code owners December 14, 2024 21:59
Copy link

codecov bot commented Dec 14, 2024

Codecov Report

Attention: Patch coverage is 2.21893% with 661 lines in your changes missing coverage. Please review.

Project coverage is 55.84%. Comparing base (807d3ca) to head (9f3c0fc).

Files with missing lines Patch % Lines
crates/reth/node/src/payload_builder.rs 0.00% 142 Missing ⚠️
crates/reth/node/src/node.rs 0.00% 116 Missing ⚠️
crates/reth/rpc/src/eth/pending_block.rs 0.00% 101 Missing ⚠️
crates/reth/rpc/src/eth/mod.rs 0.00% 82 Missing ⚠️
crates/reth/rpc/src/eth/transaction.rs 0.00% 68 Missing ⚠️
crates/reth/rpc/src/eth/call.rs 0.00% 66 Missing ⚠️
crates/reth/node/src/engine.rs 0.00% 34 Missing ⚠️
crates/reth/exex/src/prover_exex.rs 0.00% 12 Missing ⚠️
crates/reth/node/src/payload.rs 0.00% 11 Missing ⚠️
crates/reth/rpc/src/eth/block.rs 0.00% 8 Missing ⚠️
... and 6 more
@@            Coverage Diff             @@
##             main     #542      +/-   ##
==========================================
- Coverage   56.40%   55.84%   -0.56%     
==========================================
  Files         303      301       -2     
  Lines       30982    31285     +303     
==========================================
- Hits        17474    17471       -3     
- Misses      13508    13814     +306     
Files with missing lines Coverage Δ
bin/strata-cli/src/signet/persist.rs 0.00% <ø> (ø)
crates/chaintsn/src/transition.rs 82.86% <ø> (ø)
crates/consensus-logic/src/csm/state_tracker.rs 45.60% <ø> (ø)
crates/consensus-logic/src/duty/types.rs 0.00% <ø> (ø)
crates/db/src/database.rs 74.28% <ø> (ø)
crates/db/src/traits.rs 0.00% <ø> (ø)
crates/evmexec/src/block.rs 100.00% <ø> (ø)
crates/evmexec/src/el_payload.rs 98.63% <ø> (ø)
crates/evmexec/src/engine.rs 66.31% <100.00%> (+0.14%) ⬆️
crates/evmexec/src/fork_choice_state.rs 0.00% <ø> (ø)
... and 39 more

... and 3 files with indirect coverage changes

delbonis
delbonis previously approved these changes Dec 16, 2024
storopoli
storopoli previously approved these changes Dec 16, 2024
Copy link
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 15a7bd3

Despite this not removing boa it is a proper step in the right direction. I've opened an issue at reth to see if we can remove boa somehow.

Thanks @evgenyzdanovich.

Oh, we need to fix the prover CI job that is failing. You might need to bump the nightly version.

@storopoli storopoli mentioned this pull request Dec 16, 2024
14 tasks
@evgenyzdanovich
Copy link
Contributor Author

evgenyzdanovich commented Dec 16, 2024

@storopoli

Oh, we need to fix the prover CI job that is failing. You might need to bump the nightly version.

Yes, I noticed it. The problem is not in the nightly version.
The problem is some reth crates of version 1.1.3 require 1.82 rust, while the rust version in the sp1 toolchain is 1.81-dev... Those crates are used in the prover and it makes the prover build (guest code, which is built with the rust from the toolchain) process to fail.

I contacted succinctlabs on the telegram, asking to bump the rust version in the sp1 toolchain.

Copy link
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 9f3c0fc

@storopoli
Copy link
Member

I contacted succinctlabs on the telegram, asking to bump the rust version in the sp1 toolchain.

Any update on this? Maybe move the PR to a draft before we figuring this out.

@evgenyzdanovich
Copy link
Contributor Author

Any update on this? Maybe move the PR to a draft before we figuring this out.

@storopoli yes, there are updates.

They provided me with instructions on forking their toolchain, so I can do it myself while they are busy and it's not the priority. I tried doing everything according to the instructions, but turns out the default github runners are running out of disk space in the process of building the toolchain.

I just rented a vm in cloud and am trying to do it manually. If I fail, I'll convert this PR to draft and poke them from time to time.

Quite sad, because it means I have to rebase this big PR from time to time to maintain it. It is what it is...

@evgenyzdanovich
Copy link
Contributor Author

@storopoli convering to draft. I tried building everything from scratch on my cloud vm, but failed somewhere in the middle. I give up on efforts to fork their toolchain, not worth the time.

I'll periodically update this branch to keep up with the updates from main.

@evgenyzdanovich evgenyzdanovich marked this pull request as draft December 19, 2024 14:29
@storopoli
Copy link
Member

Any update on this? Maybe move the PR to a draft before we figuring this out.

@storopoli yes, there are updates.

They provided me with instructions on forking their toolchain, so I can do it myself while they are busy and it's not the priority. I tried doing everything according to the instructions, but turns out the default github runners are running out of disk space in the process of building the toolchain.

I just rented a vm in cloud and am trying to do it manually. If I fail, I'll convert this PR to draft and poke them from time to time.

Quite sad, because it means I have to rebase this big PR from time to time to maintain it. It is what it is...

Oooof babysitting PRs is annoying...
Let me know if you need to share the load once the SP1 saga is solved.

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.

3 participants