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

feat: Add cli arg for pool lifetime #14273

Merged
merged 8 commits into from
Feb 10, 2025

Conversation

varun-doshi
Copy link
Contributor

Ref #14262


/// Duration after which stale external transactions are removed.
#[arg(long = "builder.interval", value_parser = parse_duration_from_secs_or_ms, default_value = "300", value_name = "DURATION")]
pub stale_tx_timeout: Duration,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be called txpool.lifetime:

https://github.com/ethereum/go-ethereum/blob/3fcbb6735e8cd7ca6508dcfeed130a0a86980ab5/cmd/utils/flags.go#L387-L388

and we can use default_value_t here I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

default_value_t requires that Duration implements the trait ToString which it doesn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to

#[arg(long = "builder.interval", value_parser = parse_duration_from_secs_or_ms, default_value = "1", value_name = "DURATION")]

Copy link
Collaborator

Choose a reason for hiding this comment

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

right, hence the value_parse fn

@emhane emhane added the A-cli Related to the reth CLI label Feb 8, 2025
@mattsse mattsse requested a review from gakonst as a code owner February 10, 2025 20:39
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.

lgtm

@mattsse
Copy link
Collaborator

mattsse commented Feb 10, 2025

fyi @Cyber-Mitch I integrated some of your changes here so you get listed as author

@mattsse mattsse added this pull request to the merge queue Feb 10, 2025
Merged via the queue into paradigmxyz:main with commit 85f66ff Feb 10, 2025
44 checks passed
@Cyber-Mitch
Copy link
Contributor

fyi @Cyber-Mitch I integrated some of your changes here so you get listed as author

oh okay cool

18aaddy pushed a commit to 18aaddy/reth that referenced this pull request Feb 12, 2025
Co-authored-by: Reentrancy <[email protected]>
Co-authored-by: Reentrancy <[email protected]>
Co-authored-by: Matthias Seitz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Related to the reth CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants