Non-checkpoint states are added into checkpoint cache

[nflaig]

Currently it looks like we are adding non-checkpoint states into the checkpoint cache

lodestar/packages/beacon-node/src/chain/stateCache/stateContextCheckpointsCache.ts

Line 55 in 5f7f995

add(cp: phase0.Checkpoint, item: CachedBeaconStateAllForks): void {

This happens when importing a block once every epoch

lodestar/packages/beacon-node/src/chain/blocks/importBlock.ts

Lines 331 to 335 in 9034831

	if (block.message.slot % SLOTS_PER_EPOCH === 0) {
	// Cache state to preserve epoch transition work
	const checkpointState = postState;
	const cp = getCheckpointFromState(checkpointState);
	this.regen.addCheckpointState(cp, checkpointState);

Non-checkpoint states can be determined by checking the state root in block header is a zero hash

// ...
add(cp: phase0.Checkpoint, item: CachedBeaconStateAllForks): void {
    console.log("added checkpoint state");
    if (byteArrayEquals(item.latestBlockHeader.stateRoot, ZERO_HASH)) {
      console.log("not a checkpoint state");
    }
// ...

We add 2 different states to checkpoint cache each epoch and one of them is not a checkpoint state

Aug-04 16:01:52.999[]                 info: Synced - slot: 7027807 - head: 0x0300…1e75 - exec-block: valid(17842082 0xdacd…) - finalized: 0xdfe0…1817:219616 - peers: 32
added checkpoint state
Aug-04 16:02:05.051[]                 info: Synced - slot: 7027808 - head: (slot -1) 0x0300…1e75 - exec-block: valid(17842082 0xdacd…) - finalized: 0x9f2e…87f6:219617 - peers: 31
added checkpoint state
not a checkpoint state <-- this should never happen
Aug-04 16:02:17.000[]                 info: Synced - slot: 7027809 - head: 0xc202…3c70 - exec-block: valid(17842084 0x1b1f…) - finalized: 0x9f2e…87f6:219617 - peers: 33
....
Aug-04 16:08:17.001[]                 info: Synced - slot: 7027839 - head: 0xe276…f61c - exec-block: valid(17842114 0x445d…) - finalized: 0x9f2e…87f6:219617 - peers: 41
added checkpoint state
Aug-04 16:08:29.024[]                 info: Synced - slot: 7027840 - head: (slot -1) 0xe276…f61c - exec-block: valid(17842114 0x445d…) - finalized: 0xadc1…d8a6:219618 - peers: 42
added checkpoint state
not a checkpoint state <-- this should never happen
Aug-04 16:08:41.246[]                 info: Synced - slot: 7027841 - head: 0xe6ee…4149 - exec-block: valid(17842116 0x8354…) - finalized: 0xadc1…d8a6:219618 - peers: 41

cc @dapplion @tuyennhv @wemeetagain

[twoeths]

yes this happens after we process the 1st block of an epoch where stateRoot is always ZERO_HASH. I don't remember in which cases we use this state, if yes we should add more comments there

to add more context, this {root: blockRoots[n], epoch: e} and the regular/most used checkpoint state {root: blockRoots[n-1], epoch: e} (where n is the first slot of epoch e) share a lot of common data so this is not a big issue

[dapplion]

to add more context, this {root: blockRoots[n], epoch: e} and the regular/most used checkpoint state {root: blockRoots[n-1], epoch: e} (where n is the first slot of epoch e) share a lot of common data so this is not a big issue

It's an issue yes because if there's a re-org on the begning of the epoch you can no longer use that state because it has already processed the first block. If we want to store a regular state in the cache, that's why the state cache exist. Why duplicate it and store also on checkpoint state where it may be miss-used?

[twoeths]

Right now we store 2 checkpoint states per epoch:

• One is with ${last block root of previous epoch : current epoch}, this is non-spec and to save us time for possible epoch transition
• One is with ${block root of slot 0 of current epoch : current epoch}, this is per-spec checkpoint state

  lodestar/packages/beacon-node/src/chain/blocks/importBlock.ts

  Lines 331 to 335 in 9034831

  	if (block.message.slot % SLOTS_PER_EPOCH === 0) {
  	// Cache state to preserve epoch transition work
  	const checkpointState = postState;
  	const cp = getCheckpointFromState(checkpointState);
  	this.regen.addCheckpointState(cp, checkpointState);

So this works as designed, we need to add comments to different places for later reference

[twoeths]

also probably we need to change the condition for the spec checkpoint above to if (blockEpoch > parentEpoch), will address in #5968