Skip to content

miner/worker: broadcast block immediately once sealed#2576

Merged
zzzckck merged 1 commit intobnb-chain:developfrom
buddh0:blocks_broadcast_once_sealed
Jul 16, 2024
Merged

miner/worker: broadcast block immediately once sealed#2576
zzzckck merged 1 commit intobnb-chain:developfrom
buddh0:blocks_broadcast_once_sealed

Conversation

@buddh0
Copy link
Copy Markdown
Contributor

@buddh0 buddh0 commented Jul 12, 2024

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:

  • add each change in a bullet point here
  • ...

}
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 {
Copy link
Copy Markdown
Contributor

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?

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)
Copy link
Copy Markdown
Contributor Author

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 {
Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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.

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.

@buddh0 buddh0 requested review from brilliant-lx and galaio July 15, 2024 09:57
@zzzckck zzzckck merged commit 6d5b4ad into bnb-chain:develop Jul 16, 2024
jingjunLi added a commit to jingjunLi/bsc that referenced this pull request Jul 22, 2024
NathanBSC added a commit to NathanBSC/bsc that referenced this pull request Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants