vms/platformvm: Remove double block building logic
#2380
Conversation
dhrubabasu
requested review from
abi87,
danlaine and
StephenButtolph
as code owners
| // We can build a block now | ||
| now := b.txExecutorBackend.Clk.Time() | ||
| if !b.nextStakerChangeTime.After(now) { | ||
| // Block needs to be issued to advance time. | ||
| b.Mempool.RequestBuildBlock(true /*=emptyBlockPermitted*/) |
There was a problem hiding this comment.
The PR description says: setNextBuildBlockTime now no longer builds a block. It only is responsible for updating the nextStakerChangeTime field. We're not calling buildBlock in here anymore, but we're sort of building a block by calling b.Mempool.RequestBuildBlock.
There was a problem hiding this comment.
Should we return here? The rest of the work we do in this method seems unnecessary because BuildBlock is about to be invoked by the engine, and therefore the nextStakerChangeTime doesn't matter.
There was a problem hiding this comment.
Re-worked this function a bit and now return early
| @@ -92,28 +91,14 @@ func New( | |||
| func (b *builder) BuildBlock(context.Context) (snowman.Block, error) { | |||
| b.Mempool.DisableAdding() | |||
| defer func() { | |||
| b.Mempool.RequestBuildBlock(false /*=emptyBlockPermitted*/) | |||
There was a problem hiding this comment.
We may want to build another block immediately after building a block. For example, the mempool is so full that there are more txs in it that can fit in a single block
dhrubabasu
force-pushed
the
stop-building-block-twice
branch
from
e10a052 to
5b70127
Compare
| @@ -93,27 +94,12 @@ func (b *builder) BuildBlock(context.Context) (snowman.Block, error) { | |||
| b.Mempool.DisableAdding() | |||
| defer func() { | |||
| b.Mempool.EnableAdding() | |||
| b.ResetBlockTimer() | |||
| b.Mempool.RequestBuildBlock(false /*=emptyBlockPermitted*/) | |||
There was a problem hiding this comment.
- The timer previously fired to advance the chain time
- The next staker change time is to move a staker from pending to current
- There is an invalid tx in the mempool
I think we can end up building an invalid block and not scheduling to build another block to advance the timestamp.
| b.txExecutorBackend.Ctx.Log.Error("block builder encountered a fatal error", | ||
| zap.Error(err), | ||
| ) | ||
| return |
There was a problem hiding this comment.
Rather than killing the block builder loop we could do something like specify duration = time.Second or something... But really this should never happen.
Co-authored-by: aaronbuchwald <[email protected]> Signed-off-by: Stephen Buttolph <[email protected]>
| BuildBlock(context.Context) (snowman.Block, error) | ||
| // ShutdownBlockTimer stops block creation requests to advance the chain | ||
| // timestamp. | ||
| ShutdownBlockTimer() |
There was a problem hiding this comment.
nit: Would it make sense to just call this Shutdown?
There was a problem hiding this comment.
I think I like ShutdownBlockTimer more tbh- we're not really shutting down the block builder, just the timer
Why this should be merged
We currently build blocks twice by calling
buildBlocktwice. This PR removes this double-building logic and attempts to make the consensus engine messaging more obviously correct. This PR also removes the usage oftimer.Timer.How this works
TLDR we should only pass the
PendingTxsmessage to the engine once we either:NextStakerChangeTimeis in the pastThe block timer is now only responsible for checking if the
NextStakerChangeTimeis in the past. If it is, we callmempool.RequestBuildBlock()to passPendingTxsmessage to the engine with theemptyBlocksPermittedoverride set toTrue.ResetBlockTimeris called whenever the preferred block is updated: https://github.com/ava-labs/avalanchego/blob/dev/vms/platformvm/vm.go#L400-L405. This is the only time thatNextStakerChangeTimecan change so we force a recalculation of the block timer inResetBlockTimer.How this was tested
CI