-
Notifications
You must be signed in to change notification settings - Fork 860
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
Disable bonsai-limit-trie-logs-enabled if sync-mode=FULL #7357
Disable bonsai-limit-trie-logs-enabled if sync-mode=FULL #7357
Conversation
Instead of preventing startup Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
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.
LGTM.
One ask though, it would be helpful to see these configs (--bonsai-limit-trie-logs-enabled
, and --bonsai-trie-logs-pruning-window-size
) in the startup banner logs since they have a significant impact to linea and fleet mode operations
@garyschulte its been in the startup banner from the get go 🙂 |
I thought that was the case but I didn't see it when I didn't specify it on the command line
|
@garyschulte ahh it's only present when it's enabled. In this case, FULL sync has auto-disabled it and printed a warning log. Are you suggesting explicitly stating it's disabled in the banner? I'm not sure there's precedent for that with other disabled features but it does seem reasonable since it's no longer experimental and is enabled by default for bonsai. |
I think it was an expectation issue on my part. I was thinking of adding it as a static element, but of course it only makes sense for bonsai storage format. It seems reasonable to just rely on its absence to indicate it is disabled 👍 |
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…#7357) There is still a startup error when bonsai-limit-trie-logs-enabled is explicitly set to true --------- Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: gconnect <agatevureglory@gmail.com>
Since bonsai-limit-trie-logs-enabled has been defaulted to true, instead of preventing startup, implicitly set
bonsai-limit-trie-logs-enabled=false
and warn...However, if you explicitly try to set it to true, you get a startup error...
This also avoids a scenario where private networks can end up starting with sync-mode=FULL and bonsai-limit-trie-logs-enabled=true.
Thanks for sending a pull request! Have you done the following?
doc-change-required
label to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build
./gradlew acceptanceTest
./gradlew integrationTest
./gradlew ethereum:referenceTests:referenceTests