flow-go icon indicating copy to clipboard operation
flow-go copied to clipboard

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

Open AlexHentschel opened this issue 2 years ago • 0 comments

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..."
  • [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 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 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 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 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 the StateMutator and 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.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)
  • [ ] 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

AlexHentschel avatar Apr 12 '24 21:04 AlexHentschel