[EFM] EpochCommit is backward compatible
#6795
Conversation
…ort backward-compatibility for DKGIndexMap
…. Added test to ensure that we behave as expected
…ed tests to ensure that we indeed can parse those
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/efm-recovery #6795 +/- ##
========================================================
+ Coverage 41.68% 41.72% +0.03%
========================================================
Files 2033 2033
Lines 180749 181036 +287
========================================================
+ Hits 75341 75529 +188
- Misses 99204 99283 +79
- Partials 6204 6224 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Looks good!
Additional comments largely unrelated to this PR 👇
While reviewing this PR, I noticed another area where we are not currently backward-compatible. The submission of the final result now includes the DKG index map, but this depends on the FlowDKG smart contract being updated.
We can add backward-compatibility logic to the result submission step, based on the protocol version. However, doing this means that we must coordinate the protocol upgrade and the smart contract upgrade within the same epoch phase. Previously we discussed how implementing backward-compatibility would allow us to decouple the protocol upgrade and the smart contract upgrade; because of the result submission, I think they will need to be coupled.
| } | ||
| } | ||
|
|
||
| func (commit *epochCommitV0) MarshalMsgpack() ([]byte, error) { |
There was a problem hiding this comment.
Hmm, so we added the DKGIndexMap to Mainnet26 (current network) so that we would not have a data model change at the Protocol HCU (just nil vs non-nil values). But the encodableFromCommit wrapper made it so that the DKGIndexMap field was excluded when persisting data to storage anyway. 😬
I guess that's why you added this test with the EpochCommit type without a DKGIndexMap field.
In the end, adding the nil DKGIndexMap field wouldn't have made a difference anyway (even if it was included in the encoding). Reading an encoded value with a nil field X is equivalent to reading a value without any field X: in either case the decoded struct has a nil X field.
| v1 := unittest.EpochCommitFixture() | ||
| v0 := &epochCommitV0{ | ||
| Counter: v1.Counter, | ||
| ClusterQCs: v1.ClusterQCs, | ||
| DKGGroupKey: v1.DKGGroupKey, | ||
| DKGParticipantKeys: v1.DKGParticipantKeys, | ||
| } | ||
| require.Equal(t, v0.ID(), v1.ID()) |
There was a problem hiding this comment.
for a second, I was confused why these two would have the same ID ... until I realized that unittest.EpochCommitFixture() returns a v1 data structure, but sill leaves DKGIndexMap as nil.
I think it would be good to eventually have EpochCommitFixture() return a properly instantiated Epoch Commit. But we don't have to do it in this PR
There was a problem hiding this comment.
The problem is we can create only an non-empty DKGIndexMap but not a valid one since EpochCommitFixture doesn't know anything about participants unless we require that caller needs to be pass participants for each call.
Co-authored-by: Alexander Hentschel <alex.hentschel@flowfoundation.org> Co-authored-by: Jordan Schalm <jordan.schalm@flowfoundation.org>
#6785
Context
This PR fixes code paths that are relying on changes to the
EpochCommitwhich were introduced in EFM workstream. In order to support protocol upgrade without spork we need to ensure that our new software version supports both data models (v0 and v1).This PR modifies existing logic to use v0 behavior for v0 data models. Majority of logic still relies on new representation of
flow.EpochCommitwith theflow.DKGIndexMapfield. However ifflow.DKGIndexMap = nilwe will use pre-upgrade logic to follow the protocol, otherwise we use new rules.I have created multiple TODOs in next format:
TODO(EFM, #6794)that needs to be removed after performing the upgrade, when we decide that we don't want to support v0 anymore.