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

chore: expose PruneConfig in CLI args #10639

Merged
merged 13 commits into from
Sep 6, 2024

Conversation

garwahl
Copy link
Contributor

@garwahl garwahl commented Aug 31, 2024

This PR closes #9816 and addresses #9808

This PR exposes PruneConfig via the CLI so that users can adjust any pruning configuration, similar to how it is adjusted in reth.toml currently.

cc @emhane

@emhane
Copy link
Member

emhane commented Aug 31, 2024

This PR attempts to close #9816

This draft currently has two outstanding questions.

I'd say, any more specific flags than --full, take precedence over --full. so --full currently sets default prune modes (as opposed to archive node which doesn't prune). setting --full --prune.receipts.distance <u64> would set all prune modes with defaults, except for receipts, which would be set to the the value passed to --prune.receipts.distance.

  • For configuring ReceiptsLogFilter this looks like it takes an Address:PruneMode pair, how do we expect this to look like as a CLI arg, ie --prune.receiptslogfilter address:distance:64,address2:before:64,address3:full or something else?

lgtm

cc @emhane

@garwahl garwahl marked this pull request as ready for review September 1, 2024 07:47
@garwahl
Copy link
Contributor Author

garwahl commented Sep 1, 2024

cc @emhane This is ready for review now PTAL

Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

looking good!

crates/node/core/src/args/pruning.rs Outdated Show resolved Hide resolved
crates/node/core/src/args/pruning.rs Outdated Show resolved Hide resolved
crates/node/core/src/args/pruning.rs Show resolved Hide resolved
crates/node/core/src/args/pruning.rs Outdated Show resolved Hide resolved
crates/node/core/src/args/pruning.rs Outdated Show resolved Hide resolved
@garwahl garwahl requested a review from emhane September 3, 2024 11:07
@garwahl
Copy link
Contributor Author

garwahl commented Sep 3, 2024

@emhane do you mind kicking off another run of the pipeline, lint should be fixed now. Thanks!

@joshieDo joshieDo added A-cli Related to the reth CLI A-pruning Related to pruning or full node labels Sep 3, 2024
@joshieDo joshieDo changed the title Expose PruneConfig in CLI args chore: expose PruneConfig in CLI args Sep 3, 2024
@garwahl
Copy link
Contributor Author

garwahl commented Sep 5, 2024

@mattsse @Rjected @onbjerg @gakonst PTAL

@emhane
Copy link
Member

emhane commented Sep 5, 2024

remaining are lint fixes

@garwahl
Copy link
Contributor Author

garwahl commented Sep 5, 2024

Hey @emhane could you kick off the lint again? I've run both ./book/cli/update.sh target/debug/reth and make pr after rebasing off main and not getting any diff

@garwahl
Copy link
Contributor Author

garwahl commented Sep 6, 2024

@emhane do you mind running the lint on this PR and pushing to the branch, I've run ./book/cli/update.sh target/debug/reth and make pr after pulling in upstream main and not seeing any diffs. Perhaps my local configuration is broken

@emhane
Copy link
Member

emhane commented Sep 6, 2024

@emhane do you mind running the lint on this PR and pushing to the branch, I've run ./book/cli/update.sh target/debug/reth and make pr after pulling in upstream main and not seeing any diffs. Perhaps my local configuration is broken

I was struggling with the book too, removing this extension in VS code solved it for me. this extension was recommended by vs code, hence why I installed it in the first place.
Screenshot 2024-09-06 at 12 36 46

@garwahl
Copy link
Contributor Author

garwahl commented Sep 6, 2024

@emhane do you mind running the lint on this PR and pushing to the branch, I've run ./book/cli/update.sh target/debug/reth and make pr after pulling in upstream main and not seeing any diffs. Perhaps my local configuration is broken

I was struggling with the book too, removing this extension in VS code solved it for me. this extension was recommended by vs code, hence why I installed it in the first place. Screenshot 2024-09-06 at 12 36 46

🤔 I'm running RustRover as an IDE.

I'd be very surprised if the dev env impacts ./book/cli/update.sh which looks like it runs ./book/cli/help.py under the hood but 🤷

@garwahl
Copy link
Contributor Author

garwahl commented Sep 6, 2024

Could you sanity check and rebuild the book from this branch and see if you get the diff? I can't see why the IDE would impact it

@emhane emhane added this pull request to the merge queue Sep 6, 2024
Merged via the queue into paradigmxyz:main with commit b13e982 Sep 6, 2024
35 checks passed
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 A-pruning Related to pruning or full node
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose entire prune config in CLI args
3 participants