-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
miner/worker: broadcast block immediately once sealed #2576
miner/worker: broadcast block immediately once sealed #2576
Conversation
@@ -946,8 +946,9 @@ func (h *handler) minedBroadcastLoop() { | |||
if obj == nil { | |||
continue | |||
} | |||
if ev, ok := obj.Data.(core.NewMinedBlockEvent); ok { | |||
h.BroadcastBlock(ev.Block, true) // First propagate block to peers | |||
if ev, ok := obj.Data.(core.NewSealedBlockEvent); ok { |
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 am not sure how exactly this improves things? Could you clarify bit more?
Now we are waiting for a sealing to happen before broadcasting rather than mining to happen before broadcasting it.
But in (w *worker) resultLoop()
function, there are several checks made before a mining event is emitted which could fail e.g. double sign. Wouldn't that be a problem?
@@ -665,7 +665,7 @@ func (w *worker) resultLoop() { | |||
// Commit block and state to database. | |||
task.state.SetExpectedStateRoot(block.Root()) | |||
start := time.Now() | |||
status, err := w.chain.WriteBlockAndSetHead(block, receipts, logs, task.state, true) | |||
status, err := w.chain.WriteBlockAndSetHead(block, receipts, logs, task.state, true, w.mux) |
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.
@emailtovamos this happens after double sign
maybe NewSealedBlockEvent is not exactly accurate, but I have not got a better word
mux.Post(NewSealedBlockEvent{Block: block}) | ||
} | ||
|
||
if err := bc.writeBlockWithState(block, receipts, state); err != nil { |
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.
writeBlockWithState cost much, may be hundreds of milliseconds
we can post a NewSealedBlockEvent before it, so broadcast it early @emailtovamos
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.
Ok, understood.
But if writeBlockWithState()
fails for some reason then it is still okay to post a NewSealedBlockEvent
? This is my only doubt.
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.
Ok, understood. But if
writeBlockWithState()
fails for some reason then it is still okay to post aNewSealedBlockEvent
? This is my only doubt.
every verification has been passed when do writeBlockWithState
.
if a miner has a bad db, cause failed to writeBlockWithState. It's ok, others will write the mined block into db.
if the block is bad, cause failed to writeBlockWithState. It's ok, others will reject the mined block into db.
Description
miner/worker: broadcast block immediately once sealed
Rationale
state.commit and write down some block may cost more than 700+ms
this PR is to reduce the unnecessary delay
Example
add an example CLI or API response...
Changes
Notable changes: