-
Notifications
You must be signed in to change notification settings - Fork 662
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(optimistic_block): produce #12761
base: master
Are you sure you want to change the base?
Conversation
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.
Very nice! I think it can be merged after merging with master. Leaving mostly nits.
We can start thinking if something can already be unit-tested here. Maybe just run produce_optimistic, then produce and check that timestamps match. But it can be done later, and if setup is too hard, we'd have to make it simple first.
chain/client/src/client_actor.rs
Outdated
for height in | ||
latest_known.height + 1..=self.client.doomslug.get_largest_height_crossing_threshold() | ||
{ | ||
let next_block_producer_account = | ||
self.client.epoch_manager.get_block_producer(&epoch_id, height)?; | ||
|
||
if me == next_block_producer_account { | ||
if let Err(err) = self.produce_optimistic_block(height) { |
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.
I think we can call this only if
self.client.doomslug.ready_to_produce_block(
height,
true,
log_block_production_info,
)
Could you double check?
Also the order should be the opposite - if we can produce normal block, we can skip producing optimistic block in order to optimise processing flow.
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.
I agree that we should only produce the optimistic block if we cannot produce the actual block. I am not yet sure why do we need to wait for approvals for the previous block. Will look deeper into doomslug
492e6e3
to
66b73c5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12761 +/- ##
==========================================
+ Coverage 70.73% 70.76% +0.03%
==========================================
Files 850 850
Lines 174864 175009 +145
Branches 174864 175009 +145
==========================================
+ Hits 123683 123851 +168
+ Misses 46000 45970 -30
- Partials 5181 5188 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
We continue the implementation of Optimistic block #10584, by adding the logic to produce the block as soon as the previous block is done.
If available, the optimistic block will be used in the production of the block to use the same timestamp.