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

Improve eth_getLogs performance for latest block #6305

Merged
merged 17 commits into from
Feb 25, 2024

Conversation

leontiad
Copy link
Contributor

@leontiad leontiad commented Jan 31, 2024

Solves #5680

@leontiad leontiad changed the title init Solves #5680 Jan 31, 2024
@leontiad leontiad changed the title Solves #5680 Improve eth_getLogs performance for latest block #5680 Jan 31, 2024
@leontiad leontiad changed the title Improve eth_getLogs performance for latest block #5680 Improve eth_getLogs performance for latest block Jan 31, 2024
false,
);
}
return Ok(all_logs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey!
small nit: I think that the idiomatic way in Rust and across the repo is not to add ";" in return statements
maybe you have some auto-fmt tooling that put ";" after returns
I got such problems a few times in my rust setup
Try to use "cargo +nightly fmt" , it helps!
ps
cargo clippy --bin "reth" --workspace --features "ethereum" --lib --tests --benches --examples
and this one to check lints

@gakonst
Copy link
Member

gakonst commented Feb 16, 2024

@leontiad thanks for taking the time here! what's missing for us to take a look?

@mattsse
Copy link
Collaborator

mattsse commented Feb 16, 2024

ah my bad, this one slipped through

going over this asap

@leontiad leontiad marked this pull request as ready for review February 16, 2024 16:41
@leontiad
Copy link
Contributor Author

@leontiad thanks for taking the time here! what's missing for us to take a look?

I am having an issue with http::test_call_filter_functions_http test

@emhane emhane added C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started A-rpc Related to the RPC implementation labels Feb 17, 2024
@leontiad leontiad requested a review from allnil February 18, 2024 17:57
@leontiad
Copy link
Contributor Author

Overall thank you for the support ! As a generic comment as a first contributor I was confused on what local tests/fmt/clippy I need to run before pushing and open the PR. Unless I miss it and is already there a one command line unifying all clippy/formatter/tests would really help.

@Rjected
Copy link
Member

Rjected commented Feb 19, 2024

Overall thank you for the support ! As a generic comment as a first contributor I was confused on what local tests/fmt/clippy I need to run before pushing and open the PR. Unless I miss it and is already there a one command line unifying all clippy/formatter/tests would really help.

you can actually run make lint to run the clippy / formatter. For tests I usually just test things close to what I'm editing, but if you want something universal (although it may take longer), you can use make test-unit or cargo nextest run --workspace

@leontiad
Copy link
Contributor Author

Overall thank you for the support ! As a generic comment as a first contributor I was confused on what local tests/fmt/clippy I need to run before pushing and open the PR. Unless I miss it and is already there a one command line unifying all clippy/formatter/tests would really help.

you can actually run make lint to run the clippy / formatter. For tests I usually just test things close to what I'm editing, but if you want something universal (although it may take longer), you can use make test-unit or cargo nextest run --workspace

thanks, I need some help with a failing test. Can someone have a look @mattsse

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

very sorry for the delayed review here -.-

this is going in the right direction and is pretty close.

I only have one suggestion to get rid of the block_hash and latest lookup, by reusing the previously fetched ChainInfo object

crates/rpc/rpc/src/eth/filter.rs Show resolved Hide resolved
crates/rpc/rpc/src/eth/filter.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/filter.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nice!

tysm!

@mattsse mattsse added this pull request to the merge queue Feb 25, 2024
Merged via the queue into paradigmxyz:main with commit d3d994c Feb 25, 2024
29 checks passed
@gakonst
Copy link
Member

gakonst commented Feb 26, 2024

🚀 🚀 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Related to the RPC implementation C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants