[EPIC] [Dynamic Protocol State | M2] ToDos, suggested revisions and tech debt

[AlexHentschel]

This issue if for collecting various suggestions, TODOs and tech debt related to the KVStore

High Prio

• I find it super confusing / misleading that Snapshot.EpochProtocolState().Entry() is defined through the InitialProtocolState interface. The InitialProtocolState is very clear in its documentation that it (quote) "returns constant data for given epoch." Therefore, the only possible conclusion is also that Entry returns the DynamicProtocolState as specified initially by the Epoch Setup and Commit events. However, the implementations always return the DynamicProtocolState as of the specified block. (for further details and discussion, see this comment)

  Suggestions: (discussion April 15th, Yurii, Jordan, Alex)

  • merge InitialProtocolState interface into DynamicProtocolState EpochProtocolState
  • whenever we are referring to Epoch-sub-state, we should explicitly refer to it using "Epoch..."

• Currently there is nothing guaranteeing that two models with a different version (but otherwise identical contents) will have a different ID. See discussion
  Suggestion: Let modelVersion and modelEncoded be the output of model.VersionedEncode(). Then change ID calculation to use hash(modelVersion || modelEncoded) instead of the default MakeID. Addressed by Include version in kvstore model encoding explicitly #5854

Medium Prio

• InvalidEpochTransitionAttempted (this is currently a dummy flag to demonstrate is currently duplicated (field in the EpochStateContainer (where is was originally) and in the data model for the KV-Store). See Jordan's PR comment for details.

• Remove additional redundant fields from EncodableSnapshot (in particular LatestSeal, Result, Head). See [KVStore] Reducing duplicated data in the Snapshot data structure (remove EncodableEpoch) #5682 (comment)

• We still directly operate on service events to emit epoch-related metrics events in the FollowerState (code). We should instead emit metrics events using the Protocol State field, and only manipulate service events with the StateMutator and protocol state machines (lower level logic).

• [Protocol KVStore] Move EpochProtocolState data into KVStore #5974

  • historically grown, the Epoch portion of the Protocol state is exposed in three different places in our APIs (see PR comment). We would like to consolidate this.

  Suggestion:

  • keep original Snapshot.Epoch API, but back it by the KV-store (this prevents needing to update/refactor all existing usages)
  • no (remove) top-level "getter" Snapshot.EpochProtocolState
  • top-level access to Epoch-data is still given through KV-Store's canonical representation (interface according to the newest protocol version 👉 KVStoreReader)

• [Protocol KV Store] Update bootstrapping to support transferring KVStore from old network to new #5973

Low Prio

• Clean up the test setup for epoch-related consensus integration tests (in particular withNextEpoch). See [KVStore] Reducing duplicated data in the Snapshot data structure (remove EncodableEpoch) #5682 (comment) for discussion.