[Dynamic Protocol State] Split flow.Identity in flow.IdentitySkeleton and flow.DynamicIdentity
#4545
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d fixed usages. Updated signer indices
…nding on context. Updated usages. Fixed tests
…/6232-static-identity-model
durkmurder
requested review from
yhassanzadeh13,
peterargue,
AlexHentschel and
jordanschalm
as code owners
Codecov Report
@@ Coverage Diff @@
## feature/dynamic-protocol-state #4545 +/- ##
==================================================================
- Coverage 54.44% 54.39% -0.06%
==================================================================
Files 912 912
Lines 85165 85268 +103
==================================================================
+ Hits 46372 46385 +13
- Misses 35215 35309 +94
+ Partials 3578 3574 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…/6232-static-identity-model
durkmurder
requested review from
gomisha,
tarakby,
ramtinms and
janezpodhostnik
as code owners
…nflow/flow-go into yurii/6232-static-identity-model
| @@ -401,11 +403,11 @@ func TestZeroWeightNodeWillNotBeSelected(t *testing.T) { | |||
| t.Run("if there is only 1 node has weight, then it will be always be the leader and the only leader", func(t *testing.T) { | |||
| toolRng := getPRG(t, someSeed) | |||
|
|
|||
| identities := unittest.IdentityListFixture(1000, unittest.WithWeight(0)) | |||
| identities := unittest.IdentityListFixture(1000, unittest.WithInitialWeight(0)).ToSkeleton() | |||
There was a problem hiding this comment.
Noting a point to clarify here, eventually. This shouldn't be addressed in this PR.
- Leader selection is determined based on initial weight. Let some node
$N$ 's initial weight when it first registers be$W_0$ - Suppose we process a weight-changing event
$\delta$ which results in node$N$ 's dynamic weight becoming$W_1$ , and$W_1 \ll W_0$ - In the next epoch,
$N$ 's leader assignment intuitively should be based on its new weight, at the beginning of that epoch.
Question: Does initial weight persist across epochs? Does the initial weight field mean (1) "initial weight when the node first registered" or (2) "initial weight as of the beginning of this epoch".
I think (2) is most intuitive and useful, but we should align and clarify documentation here, when we get to it.
This was referenced Aug 18, 2023
| @@ -37,9 +37,18 @@ type Identity struct { | |||
| // Role is the node's role in the network and defines its abilities and | |||
| // responsibilities. | |||
| Role Role | |||
| // InitialWeight is a 'trust score' initially assigned by EpochSetup event after | |||
| // the staking phase. The initial weights define the supermajority thresholds for | |||
| // the cluster and security node consensus throughout the Epoch. | |||
There was a problem hiding this comment.
Suggested change
| // the cluster and security node consensus throughout the Epoch. | |
| // the cluster and security nodes' consensus throughout the Epoch. |
• minor optimization avoiding repetitive memory allocation during slice append • minor code simplifications
| // Identity is combined from static and dynamic part and represents the full public identity of one network participant (node). | ||
| type Identity struct { | ||
| IdentitySkeleton | ||
| DynamicIdentity | ||
| } | ||
|
|
||
| // ParseIdentity parses a string representation of an identity. |
There was a problem hiding this comment.
Not sure if method ParseIdentity is used outside of flow-go, but my IDE says it is not used within the package. Maybe it is legacy code (?) We could remove it, or depreciate it?
added 3 commits
August 21, 2023 18:11
• adjusted test to fail if ordering convention is violated
AlexHentschel
approved these changes
Aug 22, 2023
| @@ -69,8 +69,8 @@ type EpochSetup struct { | |||
| DKGPhase2FinalView uint64 // the final view of DKG phase 2 | |||
| DKGPhase3FinalView uint64 // the final view of DKG phase 3 | |||
| FinalView uint64 // the final view of the epoch | |||
| Participants IdentityList // all participants of the epoch | |||
| Assignments AssignmentList // cluster assignment for the epoch | |||
| Participants IdentityList // all participants of the epoch in canonical order | |||
There was a problem hiding this comment.
- I feel this should ideally be of type
IdentitySkeleton, same as the return value offlow-go/state/protocol/epoch.go
Lines 109 to 115 in c9087fc
- Would be nice if we could establish a convention on canonical ordering as well and document it.
Probably, both points are beyond the scope of this PR.
| @@ -401,11 +403,11 @@ func TestZeroWeightNodeWillNotBeSelected(t *testing.T) { | |||
| t.Run("if there is only 1 node has weight, then it will be always be the leader and the only leader", func(t *testing.T) { | |||
| toolRng := getPRG(t, someSeed) | |||
|
|
|||
| identities := unittest.IdentityListFixture(1000, unittest.WithWeight(0)) | |||
| identities := unittest.IdentityListFixture(1000, unittest.WithInitialWeight(0)).ToSkeleton() | |||
Comment on lines
363
to
365
| slices.SortFunc(decodedSigners, func(lhs, rhs *flow.IdentitySkeleton) bool { | ||
| return order.IdentifierCanonical(lhs.NodeID, rhs.NodeID) | ||
| }) |
There was a problem hiding this comment.
By construction, the decoded signer set should be canonically ordered. I think it would be beneficial to test this here, i.e. remove the output ordering so the test would fail, if DecodeSignerIndicesToIdentities violates the ordering convention
19 tasks
jordanschalm
approved these changes
Aug 22, 2023
There was a problem hiding this comment.
Looks good!
- I applied minor comments in the last few commits, directly to the branch
- Added a discussion point in [Dynamic Protocol State] Remaining work and ToDos #4649
|
bors merge |
bors bot
added a commit
that referenced
this pull request
Aug 23, 2023
4545: [Dynamic Protocol State] Split `flow.Identity` in `flow.IdentitySkeleton` and `flow.DynamicIdentity` r=jordanschalm a=durkmurder https://github.com/dapperlabs/flow-go/issues/6232 ### Context This PR updates `flow.Identity` to split it in `flow.IdentitySkeleton` and `flow.DynamicIdentity`. As part of this PR: - Refactored `flow.Identity` - Now `flow.Identity` has `InitialWeight`(part of `IdentitySkeleton`) and `Weight`(part of `flow.DynamicIdentity`) - Updated usages of `flow.Identity` - Updated consensus committee to return `flow.IdentitySkeleton` for epoch-level calls(`Replicas` interface) and `flow.Identity` for block-level calls(`DynamicCommittee` interface) - Updated all usages in consensus code to work with `IdentitySkeleton` where applicable. :warning: Due to introducing `InitialWeight` we need to be careful when we filter identities by weight. `InitialWeight` will be always positive even if identity is not part of the current epoch(for instance it's participant of next epoch). The only reliable weight for filtering is `flow.DynamicIdentity.Weight` which represents current weight. As part of this PR I didn't update epoch structures to return `InitialIdentites` or `EpochSetup.Participants` to be IdentitySkeletons since I am not sure we can and want to omit `Weight` for epoch setup. This needs an extra round of discussions. Co-authored-by: Yurii Oleksyshyn <yuraolex@gmail.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
|
Build failed: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://github.com/dapperlabs/flow-go/issues/6232
Context
This PR updates
flow.Identityto split it inflow.IdentitySkeletonandflow.DynamicIdentity.As part of this PR:
flow.Identityflow.IdentityhasInitialWeight(part ofIdentitySkeleton) andWeight(part offlow.DynamicIdentity)flow.Identityflow.IdentitySkeletonfor epoch-level calls(Replicasinterface) andflow.Identityfor block-level calls(DynamicCommitteeinterface)IdentitySkeletonwhere applicable.InitialWeightwe need to be careful when we filter identities by weight.InitialWeightwill be always positive even if identity is not part of the current epoch(for instance it's participant of next epoch). The only reliable weight for filtering isflow.DynamicIdentity.Weightwhich represents current weight.As part of this PR I didn't update epoch structures to return
InitialIdentitesorEpochSetup.Participantsto be IdentitySkeletons since I am not sure we can and want to omitWeightfor epoch setup. This needs an extra round of discussions.