-
Notifications
You must be signed in to change notification settings - Fork 8
feat: skip empty blocks #287
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: skip empty blocks #287
Conversation
jonastheis
left a comment
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.
Looks good but would be better to have the explicit option via command line flag to enable empty block production: #282 (comment)
Understood, will do |
frisitano
left a comment
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.
Looks good, thanks for this! I've left one comment inline which I would like clarifcation from @jonastheis on.
As a follow up I think it would be good to introduce an RPC extension that allows us to toggle this setting following the pattern in #289.
jonastheis
left a comment
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.
This CI failure might be a real one: for the docker integration tests we probably need to run the node now with the flag to enable empty blocks specifically.
jonastheis
left a comment
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!
|
@varun-doshi best to always run |
frisitano
left a comment
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.
fix lints and then lgtm
Fixes #282