[EPIC] [Dynamic Protocol State | M2 & M3] ToDos, suggested revisions and tech debt
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 theInitialProtocolStateinterface. TheInitialProtocolStateis very clear in its documentation that it (quote) "returns constant data for given epoch." Therefore, the only possible conclusion is also thatEntryreturns theDynamicProtocolStateas specified initially by the Epoch Setup and Commit events. However, the implementations always return theDynamicProtocolStateas of the specified block. (for further details and discussion, see this comment)Suggestions: (discussion April 15th, Yurii, Jordan, Alex)
- merge
InitialProtocolStateinterface into ~DynamicProtocolState~EpochProtocolState - whenever we are referring to Epoch-sub-state, we should explicitly refer to it using "Epoch..."
- merge
-
[x] 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
modelVersionandmodelEncodedbe the output ofmodel.VersionedEncode(). Then changeIDcalculation to usehash(modelVersion || modelEncoded)instead of the defaultMakeID. Addressed by https://github.com/onflow/flow-go/pull/5854
Medium Prio
-
[x]
InvalidEpochTransitionAttempted(this is currently a dummy flag to demonstrate is currently duplicated (field in theEpochStateContainer(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 particularLatestSeal,Result,Head). See https://github.com/onflow/flow-go/pull/5682#issuecomment-2080002868 -
[ ] 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 theStateMutatorand protocol state machines (lower level logic). -
[ ] At the moment, we only commit the EpochProtocolState's Hash into the KV-Store -- we want to move the actual data into the KV-store
- [ ] 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.EpochAPI, 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)
-
[ ] At the moment, the bootstrapping code always generates a KV-Store filled with default data. This might need to change (carrying over values from the pre-Spork network or adding more data via the bootstrapping configuration)
Low Prio
- [ ] Clean up the test setup for epoch-related consensus integration tests (in particular
withNextEpoch). See https://github.com/onflow/flow-go/pull/5682#discussion_r1586369371 for discussion.
### Tasks
- [ ] https://github.com/onflow/flow-go/issues/5320