Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add debug log for dropped pending txs notifications #300

Closed
wants to merge 1 commit into from
Closed

add debug log for dropped pending txs notifications #300

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions plugin/evm/block_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ func (b *blockBuilder) needToBuild() bool {
func (b *blockBuilder) markBuilding() {
// If the engine has not called BuildBlock, no need to send another message.
if b.buildSent {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to bound this somehow as a failsafe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that's an interesting idea. I feel like the invariant should be that if we send PendingTxs to consensus, we are guaranteed that BuildBlock should be called on the VM.

If we do want to bound this by making this something like an atomic int we can as part of a separate PR but it's not something I'm personally inclined towards since I don't feel like the VM should have to worry about this from its perspective. I feel like I'd rather have either extreme of us only sending the notification once until the BuildBlock is called, or just not even track this builtSent flag at all.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the engine should signal this to the VM (probably with BuildBlock taking an extra err or in the block.context) so that VMs can know that proposerVM has failed creating the block. and they can reset buildSent.

Or we can set a safe timeout for this (>30 sec) and reset the buildSent once we timeout on BuildBlock.

cc @darioush @aaronbuchwald

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will currently emit a debug log every time we signal there are new transactions, so each time a new transactions event is issued, which seems more often than we would want imo.

log.Debug("previous PendingTxs notification still pending in consensus")
return
}
b.buildBlockTimer.Cancel() // Cancel any future attempt from the timer to send a PendingTxs message
Expand Down
Loading