-
Notifications
You must be signed in to change notification settings - Fork 180
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
[Consensus] SignerIndices Optimization (2/4) #2101
Changes from all commits
acc75b3
e54c08e
99c1858
54be691
11947f1
a5b6ae8
d612738
709fc96
e084823
52e2bc9
64f8324
7b6a1cf
32d162c
6c0586d
c7be2ad
0dc3c71
84cb177
715743e
3675126
929bcba
a1aca6a
f47f27c
2086754
dd85d5e
cf648c5
af6f97d
b09691f
f7c8290
3ab8b59
b886dea
619a82e
c94e689
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,8 +61,12 @@ func NewClusterCommittee( | |
return com, nil | ||
} | ||
|
||
func (c *Cluster) Identities(blockID flow.Identifier, selector flow.IdentityFilter) (flow.IdentityList, error) { | ||
|
||
// Identities returns the identities of all cluster members that are authorized to | ||
// participate at the given block. The order of the identities is the canonical order. | ||
func (c *Cluster) Identities(blockID flow.Identifier) (flow.IdentityList, error) { | ||
// blockID is a collection block not a block produced by consensus, | ||
// to query the identities from protocol state, we need to use the reference block id from the payload | ||
// | ||
// first retrieve the cluster block payload | ||
payload, err := c.payloads.ByBlockID(blockID) | ||
if err != nil { | ||
|
@@ -74,14 +78,12 @@ func (c *Cluster) Identities(blockID flow.Identifier, selector flow.IdentityFilt | |
|
||
// use the initial cluster members for root block | ||
if isRootBlock { | ||
return c.initialClusterMembers.Filter(selector), nil | ||
return c.initialClusterMembers, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Queries for the cluster's root block return the initial cluster members, without excluding any nodes ejected in the future. The reason is that the root block is defined at the same time as the initial cluster members are defined -- any ejections after that point shouldn't apply. Any ejections before that point should be reflected in the initial cluster members. |
||
} | ||
|
||
// otherwise use the snapshot given by the reference block | ||
identities, err := c.state.AtBlockID(payload.ReferenceBlockID).Identities(filter.And( | ||
selector, | ||
c.clusterMemberFilter, | ||
)) | ||
identities, err := c.state.AtBlockID(payload.ReferenceBlockID).Identities(c.clusterMemberFilter) // remove ejected nodes | ||
|
||
return identities, 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.
Why adding
QuorumCertificateWithSignerIDs
type?The smart contract emits a EpochCommit Event, in which the ClusterQC contains
VoterIDs
, and we need to convert the cadence value into golang struct to be used in protocol. It's better to keep the data type ofClusterQC
similar to the same struct in smart contract, therefore, I had to revert the change toClusterQC
in order to keep usingVoterIDs
field,, which makes the conversion side-effect free.However the QC now uses SignerIndices, which means
flow.ClusterQCVoteDatasFromQCs
can't convert directly QC into ClusterQC.I could change
flow.ClusterQCVoteDatasFromQCs
to let it take(qc, signerIDs)
, but it looks weird that the two inputs seems have no relation, but in fact the signerIDs must be converted from qc.SignerIndices.In order to make it a bit type-safe, I decided to define the
QuorumCertificateWithSignerIDs
. Now the caller ofClusterQCVoteDatasFromQCs
is responsible to convert QC intoQuorumCertificateWithSignerIDs
, such that clusterQC can be converted from it.The drawback of this approach is that now we have two QC definition
QuorumCertificate
andQuorumCertificateWithSignerIDs
. Also weird.Thoughts?
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.
👍
I like this option, because we anyway package the bootstrapping information on its own. By allowing the bootstrapping information to be in its own custom format, we gain flexibility.