Allow Checkpoint Sync on State Beyond Finalization

[ethDreamer]

Issue Addressed

• Checkpoint sync from states newer than finalization #5082

Proposed Changes

• Add --checkpoint-sync-state-id to enable user to specify a checkpoint StateId on CLI
• Prevent checkpoint syncing from a state outside the DA boundary if the chain is past the deneb fork
• Add AnchorState enum to ForkChoice to specify if the finalized state is truly finalized or just a non-revertible checkpiont state
• Detect if checkpoint state is newer than finalization automatically when using --checkpoint-sync-url
• Verify above automatic detection works with checkpointz
• Don't filter out sync peers with younger finalization if our finalized state is a non-revertible checkpoint state
• Properly persist AnchorState during shutdown

Additional Info

This is my first time messing around with fork-choice (been avoiding it..). I could use some extra scrutiny on the fork-choice modifications specifically.

[ethDreamer]

So this PR doesn't work with checkpointz but I have made @samcm aware of the need to implement this. That's the last thing on the checklist. Not sure what else should be added to this PR.

[realbigsean]

So this PR doesn't work with checkpointz but I have made @samcm aware of the need to implement this. That's the last thing on the checklist. Not sure what else should be added to this PR.

default behavior still works right? is it just that checkpointz won't serve non-finalized?

[realbigsean]

So this is used every time we add a peer who's finalized is lower than ours. Might not be a big deal in practice. But if it is, we could track "has our anchor state ever been set to AnchorState::Finalized" because it will never go from AnchorState::NonRevertible => AnchorState::Finalized. If we track that, we won't need to bother fork choice as soon as finalization advances at any point after startup

[realbigsean]

According to the versioning scheme we've got outlined here, the struct should be named based on the version it was introduced, so I think we want variants(V1, V20) here since PersistedForkChoice has always existed: https://github.com/sigp/lighthouse/blob/stable/beacon_node/beacon_chain/src/schema_change/README.md#naming-conventions

[ethDreamer]

I asked @michaelsproul earlier about versioning this inner PersistedForkChoice because I noticed we had a separate outer PersistedForkChoice and I was worried that the outer one was created to shield the inner one from versioning or something. He told me we've already had different versions of the inner PersistedForkChoice. They just get removed eventually as we stop supporting super old DB versions.

So it wouldn't make sense to name it V1 since it hasn't actually been the same since V1. I suppose we could look at old code to try and figure out the last time this struct was modified. But before we go there let's back up & consider this comment from the guide you linked (which I'm going to slightly edit to adhere to the discussion at hand):

Previously the schema migration code would name types by the last version at which they were valid. For example if PersistentForkChoice changed in V20 then we would name the two variants PersistentForkChoiceV19 and PersistentForkChoiceV20. The problem with this scheme is that if PersistentForkChoice changes again in the future at say v24 then PersistentForkChoiceV20 would need to be renamed to PersistentForkChoiceV23, which is annoying. Using the first valid version as described above does not have this issue.

I agree that being forced to rename is very annoying. But there's actually no reason we have to rename PersistentForkChoiceV20 to PersistentForkChoiceV23. We could just as easily carry on with PersistentForkChoiceV19,PersistentForkChoiceV20, and PersistentForkChoiceV24. After all, these structs are still named after the versions in which they are used.

The only issue is communicating this easily in naming conventions guide.

Perhaps instead of:

Prefer to name versions of structs by the version at which the change was introduced.

we might write something along the lines of:

When introducing new versions of a struct, adhere to the following convention:

If the struct has no previous versions in the codebase, the original version should be named by the last version in which it was valid. All subsequent versions should be named after the version in which they were introduced. For example, if the struct Foo has no previous migrations, and you added a field to Foo in v9, then you would call the original version FooV8 and write a schema change that migrates from FooV8 to FooV9. If at some later date, another field is added to Foo in v12, you would add a schema change that migrates from FooV9 to FooV12.

[ethDreamer]

While we're on the subject, I had another thought for the schema migrations guide. We should encourage the developer to add impl Into<FooV9> for FooV8 and impl Into<FooV8> for FooV9 just below the definition of the struct. Note that this encourages them to be explicit about which versions are being transformed.

Implementing this adds a significant amount of clarity about how versions are migrated and which versions are transformed into which. I previously got tripped up about this when I didn't implement these and introduced bugs in the migration code. You can leverage these impls in the schema migration code to make that simple as well. Also by being explicit about the versions you avoid having to clutter your diff with these lines when adding a new version. I already started to adopt this convention which is the only reason I made changes to beacon_fork_choice_store.rs

[realbigsean]

He told me we've already had different versions of the inner PersistedForkChoice. They just get removed eventually as we stop supporting super old DB versions.

Right that would make it tough to track introduced version. I like the suggestion to use the previous version if there are no versions defined in the super struct.

I'm on board with what you're suggesting re migration guide. And explicit intos as well

[realbigsean]

labeling this one as backwards incompat because it has a db migration

[pawanjay176]

Pushing this back to v5.2 after talking to @ethDreamer . Since this PR is almost ready, we can always make a patch release with this if really required.
Feel free to change it back if you feel this should go into 5.1.0

[michaelsproul]

This needs some updates to address the merge conflict caused by downloading checkpoint blobs

[michaelsproul]

Trying this on Goerli, I can't get any peers:

Mar 22 02:09:24.756 DEBG Handshake Failure, reason: Different finalized chain, peer: 16Uiu2HAmTyzbanJu6xJRi7efeUGMdDu1ae4qNm4aBfViNjfeZmLv, module: network::network_beacon_processor::rpc_methods:112

My node gets insta-banned after syncing off an unfinalized state. We had some fix for this I thought? Or does it require the peer to be running this PR as well?

[ethDreamer]

Trying this on Goerli, I can't get any peers:

Mar 22 02:09:24.756 DEBG Handshake Failure, reason: Different finalized chain, peer: 16Uiu2HAmTyzbanJu6xJRi7efeUGMdDu1ae4qNm4aBfViNjfeZmLv, module: network::network_beacon_processor::rpc_methods:112

My node gets insta-banned after syncing off an unfinalized state. We had some fix for this I thought? Or does it require the peer to be running this PR as well?

Concerning.. I had only tested it locally with kurtosis. I guess I'll have to dig into this :/

[michaelsproul]

Dropping this from v5.2.0

[michaelsproul]

Closing in favour of a new from-scratch impl based on recent learnings:

• Checkpoint sync from non-finalized checkpoint #7089