add debug log for dropped pending txs notifications #300
Conversation
joshua-kim
requested review from
aaronbuchwald,
darioush and
ceyonur
as code owners
| @@ -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 { | |||
There was a problem hiding this comment.
Do we want to bound this somehow as a failsafe?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Corresponding PR in subnet-evm: ava-labs/subnet-evm#783 |
|
Closing since we added logs to cover this edge case in AvalancheGo instead. |
Why this should be merged
Add a debug log for blocks that we're not building due to the previous block still pending
How this works
Adds a debug log
How this was tested
Did not test