Clean lastSnapshots when finalizing (#3161)

When cleaning snapshots, it may happen that frames have already been
written (via base updates) leading to a crash when trying to clean up
their snapshot - `deques` doesn't readily support removing items in the
middle so use a circular buffer instead.

Also fixes an issue where a partial database write may lead to a
"future" block header may turn up in the database - when this happens,
we don't have an in-memory state that can be advanced and the next block
will fail to apply. Most likely, this happens because of how the syncer
shortcuts txframe updates when writing headers, which in and of itself
is a problem that needs a proper solution - in the meantime however,
checking that the state exists allows recovering from such an
inconsistent database.

Author: arnetheduck

execution_chain/beacon/api_handler/api_newpayload.nim

@@ -161,7 +161,7 @@ proc newPayload*(ben: BeaconEngineRef,
 
   # If we already have the block locally, ignore the entire execution and just
   # return a fake success.
-  if ben.chain.haveBlockLocally(blockHash):
+  if ben.chain.haveBlockAndState(blockHash):
     warn "Ignoring already known beacon payload",
       number = header.number, hash = blockHash.short
     return validStatus(blockHash)

execution_chain/core/chain/forked_chain.nim

@@ -142,20 +142,22 @@ proc validateBlock(c: ForkedChainRef,
 
   c.writeBaggage(blk, blkHash, txFrame, receipts)
 
-  while c.lastSnapshots.len() >= 10:
+  let pos = c.lastSnapshotPos
+  c.lastSnapshotPos = (c.lastSnapshotPos + 1) mod c.lastSnapshots.len
+  if not isNil(c.lastSnapshots[pos]):
     # Put a cap on frame memory usage by clearing out the oldest snapshots -
     # this works at the expense of making building on said branches slower.
     # 10 is quite arbitrary.
-    let oldFrame = c.lastSnapshots.popFirst()
-    oldFrame.clearSnapshot()
+    c.lastSnapshots[pos].clearSnapshot()
+    c.lastSnapshots[pos] = nil
 
   # Block fully written to txFrame, mark it as such
   # Checkpoint creates a snapshot of ancestor changes in txFrame - it is an
   # expensive operation, specially when creating a new branch (ie when blk
   # is being applied to a block that is currently not a head)
   txFrame.checkpoint(blk.header.number)
 
-  c.lastSnapshots.addLast(txFrame)
+  c.lastSnapshots[pos] = txFrame
 
   c.updateBranch(parent, blk, blkHash, txFrame, move(receipts))
 
@@ -281,6 +283,11 @@ proc removeBlockFromCache(c: ForkedChainRef, bd: BlockDesc) =
   c.hashToBlock.del(bd.hash)
   for tx in bd.blk.transactions:
     c.txRecords.del(rlpHash(tx))
+
+  for v in c.lastSnapshots.mitems():
+    if v == bd.txFrame:
+      v = nil
+
   bd.txFrame.dispose()
 
 proc updateHead(c: ForkedChainRef, head: BlockPos) =

execution_chain/core/chain/forked_chain/chain_desc.nim

@@ -34,7 +34,8 @@ type
     extraValidation*: bool
     baseDistance*: uint64
 
-    lastSnapshots*: Deque[CoreDbTxRef]
+    lastSnapshots*: array[10, CoreDbTxRef]
+    lastSnapshotPos*: int
 # ----------------
 
 func txRecords*(c: ForkedChainRef): var Table[Hash32, (Hash32, uint64)] =