-
Notifications
You must be signed in to change notification settings - Fork 126
refactor(l1,l2,levm): remove dead code #5452
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
base: main
Are you sure you want to change the base?
Conversation
Lines of code reportTotal lines added: Detailed view |
Benchmark Results ComparisonBenchmark Results: Push
Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
ilitteri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to test these changes in our tooling https://github.com/lambdaclass/ethrex-replay and https://github.com/lambdaclass/rex
pablodeymo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to review external dependencies
|
| /// Fetches all receipts for the given block hashes via p2p and stores them | ||
| // TODO: remove allow when used again | ||
| #[allow(unused)] | ||
| async fn store_receipts( | ||
| mut block_hashes: Vec<BlockHash>, | ||
| mut peers: PeerHandler, | ||
| store: Store, | ||
| ) -> Result<(), SyncError> { | ||
| loop { | ||
| debug!("Requesting Receipts "); | ||
| if let Some(receipts) = peers.request_receipts(block_hashes.clone()).await? { | ||
| debug!(" Received {} Receipts", receipts.len()); | ||
| // Track which blocks we have already fetched receipts for | ||
| for (block_hash, receipts) in block_hashes.drain(0..receipts.len()).zip(receipts) { | ||
| store.add_receipts(block_hash, receipts).await?; | ||
| } | ||
| // Check if we need to ask for another batch | ||
| if block_hashes.is_empty() { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be used when we address #1766 , maybe we should leave it and link the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is #1766 going to be addressed soon? While that issue is not being addressed, I'd prefer to mention this thread in the issue and remove the code for now.
| pub fn read_block_file(block_file_path: &str) -> Block { | ||
| let encoded_block = std::fs::read(block_file_path) | ||
| .unwrap_or_else(|_| panic!("Failed to read block file with path {block_file_path}")); | ||
| Block::decode(&encoded_block) | ||
| .unwrap_or_else(|_| panic!("Failed to decode block file {block_file_path}")) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I though the import command used this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done by read_chain_file, which is almost identical.
| ### 2025-10-17 | ||
|
|
||
| - Replaces incremental iteration with a one-time precompute method that scans the entire bytecode, building a `BitVec<u8, Msb0>` where bits mark valid `JUMPDEST` positions, skipping `PUSH1..PUSH32` data bytes. | ||
| - Updates `is_blacklisted` to O(1) bit lookup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changelog shouldn't change. If anything, log that it was recently removed in a new entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_blacklisted's case is particular since it was already dead when that change happened, but I see why we might not want to pretend it never happened
Maybe adding a note or a strikethrough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's first send this one as a separate PR as this is surely unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a separate PR?
Motivation
Dead code in pub functions is not found by clippy, so they can easily accumulate.
Description
Using a tool, several functions that are never used are found.