Fix Cached Storage Verification Race

adapters.go

@@ -181,13 +181,15 @@
 }
 
 func (cs *CachedStorage) Index(ctx context.Context, block common.VerifiedBlock, certificate common.Finalization) error {
+	// Holding the lock across indexing and pruning prevents Retrieve from serving a cached block at an indexed seq.
+	cs.lock.Lock()
+	defer cs.lock.Unlock()
+
 	err := cs.Storage.Index(ctx, block, certificate)
 
 	if err == nil {
 		// We delete the block from the cache after it has been indexed because now that it is persisted,
 		// we can just lookup by sequence number instead of digest.
-		cs.lock.Lock()
-		defer cs.lock.Unlock()
 		delete(cs.cache, block.BlockHeader().Digest)
 
 		// We also delete all blocks that are older than the indexed block, including the finalized block because they are now finalized and persisted.
@@ -205,6 +207,11 @@
 	cs.lock.Lock()
 	defer cs.lock.Unlock()
 
+	// A verification that completes after its seq was indexed must not shadow the finalized block.
+	if block.BlockHeader().Seq < cs.Storage.NumBlocks() {
+		return
+	}
+
 	cs.cache[block.Digest()] = cachedBlock{
 		ParsedBlock: block,
 	}

adapters_test.go

@@ -140,6 +140,28 @@ func TestCachedStorageIndexEvictsSameSeqFork(t *testing.T) {
 	require.NotNil(t, fin)
 }
 
+// TestCachedStorageLateVerify asserts that a fork whose
+// verification completes after its seq was indexed is not cached.
+func TestCachedStorageLateVerifyDoesNotShadowIndexed(t *testing.T) {
+	cs := NewCachedStorage(newTestStorage(), 0)
+	require.NoError(t, cs.Index(t.Context(), newTestParsedBlock(0, "genesis"), common.Finalization{}))
+
+	finalized := newTestParsedBlock(1, "finalized")
+	require.NoError(t, cs.Index(t.Context(), finalized, common.Finalization{}))
+
+	delayedVerificationBlock := &cachedBlock{
+		ParsedBlock: newTestParsedBlock(1, "fork"),
+		cache:       cs,
+	}
+	_, err := delayedVerificationBlock.Verify(t.Context(), common.OnlyVMVerifyOpt)
+	require.NoError(t, err)
+
+	retrievedBlock, fin, err := cs.Retrieve(1, common.Digest{})
+	require.NoError(t, err)
+	require.Equal(t, finalized.BlockHeader().Digest, retrievedBlock.BlockHeader().Digest)
+	require.NotNil(t, fin)
+}
+
 // TestCachedStoragePopulatedByWal asserts that a block restored from the WAL on
 // startup ends up in the instance's CachedStorage, retrievable by seq before it
 // is finalized and indexed.