Serve verified uncommitted blocks to seq-only lookups in CachedStorage

[samliok]

CachedStorage keeps verified-but-not-yet-finalized blocks in a cache keyed by digest. The msm sealing-block checks (buildBlockOrTransitionEpoch, areWeReadyToTransitionEpoch) look blocks up by sequence only and pass a zero digest, so they can never hit the cache. The lookup falls through to committed storage, and while the sealing block is notarized but not yet indexed, block building fails with failed to retrieve sealing block for previous epoch. The failed build is not retried, so the leader misses its round and the round empty-notarizes.

[yacovm]

why the locking change?

[yacovm]

shouldn't this be: f err == common.ErrBlockNotFound || digest == (common.Digest{}) ?

[samliok]

no in this case it should be &&.

because || would mean we grab and return from the cache even if we returned a block & finalization above. this shouldn't happen because if we finalize a block, it shouldn't be in the cache but still ranging through the cache seems unnecessary.

[yacovm]

After reading the code again - I just noticed that we do the check after we check the storage with cs.GetBlock(seq) in line 126.

Why do we do that? Shouldn't we iterate the cache before we ask the storage?

What's the point of asking the storage first?

[yacovm]

Can't we do this?

func (cs *CachedStorage) Retrieve(seq uint64, digest common.Digest) (common.VerifiedBlock, *common.Finalization, error) {
	cs.lock.RLock()
	defer cs.lock.RUnlock()

	// A cached block is not finalized yet, since indexing removes it from the cache.
	if item, exists := cs.cache[digest]; exists {
		return item.ParsedBlock, nil, nil
	}

	for _, cb := range cs.cache {
		if cb.BlockHeader().Seq == seq && digest == (common.Digest{}) {
			return cb.ParsedBlock, nil, nil
		}
	}

	// We don't populate the cache here because we populate it externally.
	block, finalization, err := cs.GetBlock(seq)
	if err == nil {
		return &ParsedBlock{
			StateMachineBlock: block,
			msm:               cs.msm,
		}, finalization, nil
	}

	return nil, nil, err
}

[samliok]

yep, i updated the logic + re-added the locking stuff from before