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

Add poll_latest and sample_range scripts #1029

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

njgheorghita
Copy link
Collaborator

@njgheorghita njgheorghita commented Nov 9, 2023

What was wrong?

Added two scripts to help with quick & easy network testing. These are meant to be "backups" to glados, but also will be very useful for running tests on kurtosis.

Later, I'd like to implement support for providers other than infura, but given that audit_latest requires a ws connection, which is really only "easily" available via infura (eg. pandaops doesn't support this), then I figured pandaops / generic provider support can wait

How was it fixed?

  • audit_latest script, which will basically poll how soon "latest" data appears on the network
  • sample_range script, which samples X number of randomly selected blocks from any given range

To-Do

@njgheorghita njgheorghita force-pushed the audit-latest branch 2 times, most recently from a89739d to b7a8633 Compare November 9, 2023 13:53
@njgheorghita njgheorghita changed the title Add latest and sample scripts Add poll_latest and sample_range scripts Nov 9, 2023
@njgheorghita njgheorghita self-assigned this Dec 4, 2023
@njgheorghita njgheorghita force-pushed the audit-latest branch 9 times, most recently from 71c166d to 30a579b Compare January 16, 2024 22:29
@njgheorghita njgheorghita marked this pull request as ready for review January 16, 2024 22:31
Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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

I don't like coupling script dependencies with trin dependencies in main Cargo.toml.
It is also expected to add more scripts in the future (purging db and etc).

Having a bin directory inside src is also confusing and it is not very clear which file is the trin binary and which one is script without looking into Cargo.toml and the files.

I think a better strategy is to rename the src folder to bin and then add trin crate inside bin, for example see reth: https://github.com/paradigmxyz/reth/tree/main/bin/reth

This will require updating Cargo.toml and adding the trin crate as a workspace member.

Тhen we can add any rust script as a separate crate inside bin/. This way, Trin and any script will have separate Cargo.toml and dependencies.

@@ -39,6 +39,8 @@ FROM ubuntu:22.04

# copy build artifacts from build stage
COPY --from=builder /trin/target/release/trin /usr/bin/
COPY --from=builder /trin/target/release/purge_db /usr/bin/
Copy link
Member

@ogenev ogenev Jan 17, 2024

Choose a reason for hiding this comment

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

purge_db was removed:

Suggested change
COPY --from=builder /trin/target/release/purge_db /usr/bin/
COPY --from=builder /trin/target/release/poll_latest /usr/bin/

@njgheorghita
Copy link
Collaborator Author

@ogenev

I don't like coupling script dependencies with trin dependencies in main Cargo.toml.
It is also expected to add more scripts in the future (purging db and etc).

Agreed & agreed

Having a bin directory inside src is also confusing

That is the structure that is officially recommended. It's also the structure we were using when we had the purge_db.rs script.

It is not very clear which file is the trin binary and which one is script

I don't really agree. main.rs is pretty obviously the trin binary, as defined by the cargo standards.

Looking at reth, reth/bin/reth is a fairly large project, a separate workspace, with many contents. If our main trin process was a large project, then I would be inclined to follow the Reth convention. In our case src/trin.rs is basically just a simple script. Based on this, I'm not convinced that it's a convention worth adopting, when our use case seems to follow the standard use case defined in the cargo standards more closely.

Basically, I do agree it's not great coupling script dependencies with trin dependencies, but it appears to me like we're following cargo best practices. Seems like this is still a bit of an unsolved issue with cargo?

@njgheorghita
Copy link
Collaborator Author

njgheorghita commented Jan 17, 2024

@ogenev looks like there's a newly failing test in light-client ... not sure what the cause might be, but i'm getting the same failure locally problem was fixed in #1099

Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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


fn update_average_time(&mut self, new_time: Duration) {
self.average_time =
(self.average_time * (self.success_count - 1) + new_time) / self.success_count
Copy link
Member

Choose a reason for hiding this comment

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

nitpick:

Suggested change
(self.average_time * (self.success_count - 1) + new_time) / self.success_count
(self.average_time * self.success_count + new_time) / (self.success_count + 1)

@njgheorghita njgheorghita merged commit 27f6677 into ethereum:master Jan 18, 2024
8 checks passed
@njgheorghita njgheorghita deleted the audit-latest branch January 18, 2024 15:34
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