-
Notifications
You must be signed in to change notification settings - Fork 176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[EFM] Updated type conversion of EpochRecover
#6506
base: feature/efm-recovery
Are you sure you want to change the base?
Conversation
…s of EpochRecover to perform correct serialization
…low/flow-go into yurii/6495-convert-epoch-recover
@jordanschalm I've made some changes after your approval: |
…over # Conflicts: # model/convert/service_event.go
took the liberty to merge changes introduced by the prior PR #6494 (mainly these changes, which are now in |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/efm-recovery #6506 +/- ##
========================================================
+ Coverage 42.10% 42.72% +0.62%
========================================================
Files 2012 1705 -307
Lines 178121 158216 -19905
========================================================
- Hits 74994 67602 -7392
+ Misses 96944 85064 -11880
+ Partials 6183 5550 -633
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice clean code. Only have a few suggestions regarding error messages (specifically that error messages contain enough context to figure out where they are coming from)
return nil, fmt.Errorf("failed to decode EpochCommit event: %w", err) | ||
} | ||
|
||
cdcDKGGroupKey, err := getField[cadence.String](fields, "dkgGroupKey") | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to decode EpochCommit event: %w", err) | ||
} | ||
|
||
cdcDKGIndexMap, err := getField[cadence.Dictionary](fields, "dkgIdMapping") | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to decode EpochCommit event: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In those three error messages, we are talking about the EpochCommit
event, while most other error messages are mentioning the EpochRecover
event. I think we should make it consistent:
- In my opinion, it would be fine to just talk about the
EpochRecover
in all error messages. This is because some fields (like the epoch counter) are shared between theEpochSetup
andEpochCommit
events. - Furthermore, I think there is little benefit in distinguishing the fields in the
EpochRecover
by whether they belong into theEpochSetup
orEpochCommit
event. In the end what matters is that we couldn't decode some specific field (everything beyond is an implementation details that is not very useful to expose in the error message)
@@ -398,7 +401,17 @@ func convertServiceEventEpochRecover(event flow.Event) (*flow.ServiceEvent, erro | |||
|
|||
cdcDKGKeys, err := getField[cadence.Array](fields, "dkgPubKeys") | |||
if err != nil { | |||
return nil, fmt.Errorf("failed to decode EpochRecover event: %w", err) | |||
return nil, fmt.Errorf("failed to decode EpochCommit event: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("failed to decode EpochCommit event: %w", err) | |
return nil, fmt.Errorf("failed to decode EpochRecover event: %w", err) |
|
||
cdcDKGGroupKey, err := getField[cadence.String](fields, "dkgGroupKey") | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to decode EpochCommit event: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("failed to decode EpochCommit event: %w", err) | |
return nil, fmt.Errorf("failed to decode EpochRecover event: %w", err) |
|
||
cdcDKGIndexMap, err := getField[cadence.Dictionary](fields, "dkgIdMapping") | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to decode EpochCommit event: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("failed to decode EpochCommit event: %w", err) | |
return nil, fmt.Errorf("failed to decode EpochRecover event: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you go over the error messages in lines 428 - 486 and make sure that they all mention the EpochRecover
event. For example, we should extend
flow-go/model/convert/service_event.go
Line 433 in b9c1beb
"could not decode random source hex (%v): %w", |
to
return nil, fmt.Errorf("could not decode random source (hex %v) from EpochRecover event: %w", randomSrcHex, err)
@@ -453,19 +466,28 @@ func convertServiceEventEpochRecover(event flow.Event) (*flow.ServiceEvent, erro | |||
return nil, fmt.Errorf("could not convert cluster qc vote data: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("could not convert cluster qc vote data: %w", err) | |
return nil, fmt.Errorf("failed to decode clusterQCVoteData from EpochRecover event: %w", err) |
// https://github.com/onflow/flow-go/blob/feature/dkg/module/dkg/client.go#L182-L183 | ||
parsedKeys, err := convertDKGKeys(cdcDKGKeys.Values) | ||
// parse DKG participants | ||
commit.DKGParticipantKeys, err = convertDKGKeys(cdcDKGKeys.Values) | ||
if err != nil { | ||
return nil, fmt.Errorf("could not convert DKG keys: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("could not convert DKG keys: %w", err) | |
return nil, fmt.Errorf("failed to decode DKG key shares from EpochRecover event: %w", err) |
// parse DKG group key | ||
commit.DKGGroupKey, err = convertDKGKey(cdcDKGGroupKey) | ||
if err != nil { | ||
return nil, fmt.Errorf("could not convert DKG group key: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("could not convert DKG group key: %w", err) | |
return nil, fmt.Errorf("failed to decode DKG group key from EpochRecover event: %w", err) |
for _, pair := range cdcDKGIndexMap.Pairs { | ||
nodeID, err := flow.HexStringToIdentifier(string(pair.Key.(cadence.String))) | ||
if err != nil { | ||
return nil, fmt.Errorf("could not convert hex string to flow.Identifer: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("could not convert hex string to flow.Identifer: %w", err) | |
return nil, fmt.Errorf("failed to decode flow.Identifer in DKGIndexMap entry from EpochRecover event: %w", err) |
#6495
Context
This PR is based on #6494 and introduces similar changes that were made to the
EpochCommit
but for theEpochRecover
service event.