Build block immediately when transitioning epoch - #514
Conversation
This commit adds WaitForPendingBlock() for the MSM that intercepts the VM's WaitForPendingBlock call and returns early in case the VM has no pending transaction but an epoch change needs to occur or is taking place. Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
83f6cd3 to
7d3b99a
Compare
| sealingBlock, finalization, err = sm.GetBlock(sealingBlockSeq, common.Digest{}) | ||
| // The only two reasons why we would not be able to retrieve the sealing block are: | ||
| // (1) The sealing block is finalized and there is a storage failure, or | ||
| // (2) The sealing block is not finalized yet, in which case we are still transitioning to a new epoch. |
There was a problem hiding this comment.
this comment is not true, if we have advanced to a round caused by a telock then we have verified the sealing block. This means it's in the cached storage and GetBlock should return it regardless of finalization status.
There was a problem hiding this comment.
The block is in the cache, correct. However, notice that we fetch it with a wildcard digest (any digest) becauseit might be buried several Telocks under.
When we fetch by a wildcard digest, we iterate over the cache and we might return a block that is on a different chain than we intended to.
That's why I said that only if it's finalized this does not happen, because then we will delete the block from the cache.
There was a problem hiding this comment.
Slightly changed the code, please check, I think it's more clear now: 8d8d5e3
| // The only two reasons why we would not be able to retrieve the sealing block are: | ||
| // (1) The sealing block is finalized and there is a storage failure, or | ||
| // (2) The sealing block is not finalized yet, in which case we are still transitioning to a new epoch. | ||
| // Either way and in case the sealing block isn't finalized, we shouldn't wait indefinitely for the VM to build a block. |
There was a problem hiding this comment.
why shouldn't we wait indefinetly? if the sealing block is not finalized, the epoch will either keep broadcasting empty votes to advance the round or it will eventually notarize/finalize it.
If we empty notarize the round, then the next block that will be proposed may be another sealing block. So I think waiting like normal is fine here, the BlockBuilder & epoch should properly cancel the WaitForPendingBlock context?
There was a problem hiding this comment.
why shouldn't we wait indefinitely? if the sealing block is not finalized, the epoch will either keep broadcasting empty votes to advance the round or it will eventually notarize/finalize it.
If we wait indefinitely and the leader is stuck, we will not send an empty vote, right?
| // (2) The sealing block is not finalized yet, in which case we are still transitioning to a new epoch. | ||
| // Either way and in case the sealing block isn't finalized, we shouldn't wait indefinitely for the VM to build a block. | ||
| if err != nil || sealingBlock.Type() != BlockTypeSealing || finalization == nil { | ||
| if err != nil { |
There was a problem hiding this comment.
can we move this error outside of the if?
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
| // If the sealing block isn't finalized, we need to build a Telock immediately to extend the epoch, | ||
| // so we wait up to MaxBlockBuildingWaitTime and then return. |
There was a problem hiding this comment.
this comment doesn't make sense. It says we need to build a telock immediately, but then we wait up to MaxBlockBuildingWaitTime.
Also, why do we need to wait? Telocks don't have any inner blocks
There was a problem hiding this comment.
yeah you're right we don't need to wait in such a case.
samliok
left a comment
There was a problem hiding this comment.
looks good to me, the function seems a bit complex & can stray away if we ever change things about BuildBlock, but i'm not sure how to make it cleaner
| currentState := parentBlock.Metadata.SimplexEpochInfo.NextState() | ||
|
|
||
| // We first check if we have obvious signs that we need to build a block immediately: | ||
| var shouldObviouslyBuildBlockImmediately bool |
There was a problem hiding this comment.
no action but seems weird naming a variable with shouldObviously..... It should be obvious via the code, not the variable name 🤷
There was a problem hiding this comment.
so, anything that isn't shouldObviouslyBuildBlockImmediately == true means we might need to epoch change but we might not.
|
|
||
| // blockingBlockBuilder waits in WaitForPendingBlock until it is handed a pending block, the way a | ||
| // real VM waits on its mempool. The notification is consumed by a single waiter. | ||
| type blockingBlockBuilder struct { |
There was a problem hiding this comment.
i think we have this in testutil.TestBlockBuilder
There was a problem hiding this comment.
they have a different interface, we can't re-use it.
There was a problem hiding this comment.
BuildBlock(context.Context, uint64) (avalanchego.VMBlock, error)
vs
BuildBlock(_ context.Context, metadata common.ProtocolMetadata, blacklist common.Blacklist) (common.VerifiedBlock, bool)
| pChainReferenceHeight := parentBlock.Metadata.SimplexEpochInfo.PChainReferenceHeight | ||
| if parentBlock.Type() == BlockTypeSealing { | ||
| // We've moved to a new epoch, so we need to use the next P-chain reference height of the sealing block, | ||
| // because the P-chain reference height is of the next epoch is inherited from the P-chain reference height of the sealing block. |
There was a problem hiding this comment.
| // because the P-chain reference height is of the next epoch is inherited from the P-chain reference height of the sealing block. | |
| // because the P-chain reference height of the next epoch is inherited from the P-chain reference height of the sealing block. |
| // TestMSMWaitForPendingBlock checks when WaitForPendingBlock stops waiting for the VM. It must | ||
| // return on its if BuildBlock would have produced a block. |
There was a problem hiding this comment.
| // TestMSMWaitForPendingBlock checks when WaitForPendingBlock stops waiting for the VM. It must | |
| // return on its if BuildBlock would have produced a block. | |
| // TestMSMWaitForPendingBlock checks when WaitForPendingBlock stops waiting for the VM. It must | |
| // return on its own if BuildBlock would have produced a block. |
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
* Build block immediately when transitioning epoch This commit adds WaitForPendingBlock() for the MSM that intercepts the VM's WaitForPendingBlock call and returns early in case the VM has no pending transaction but an epoch change needs to occur or is taking place. Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org> * Address code review comments Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org> * Address code review comments II Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org> --------- Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
No description provided.