-
Notifications
You must be signed in to change notification settings - Fork 380
feat(consenus): [CON-1565] splitting cup share making/validating/aggregating #8623
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
base: master
Are you sure you want to change the base?
Conversation
* validator * share aggregator
| height, | ||
| initial_dkg_attempts, | ||
| } = self; | ||
|
|
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.
Don't we need to do something like this?
| if let Some(status) = subnet_splitting_status { | |
| status.hash(state); | |
| } |
| assert!( | ||
| !self.records.is_empty(), | ||
| "Cannot setup a registry without records." | ||
| ); |
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 think self.records[0] two lines above would have panicked before reaching this assert.
| use std::sync::Arc; | ||
|
|
||
| /// CatchUpPackage maker is responsible for creating beacon shares | ||
| /// [`CatchUpPackage`] maker is responsible for creating beacon shares |
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.
| /// [`CatchUpPackage`] maker is responsible for creating beacon shares | |
| /// [`CatchUpPackage`] maker is responsible for creating catch up package shares |
| SubnetSplittingError(String), | ||
| } | ||
|
|
||
| /// Reasons for why a dkg payload might be invalid. | ||
| #[derive(PartialEq, Debug)] | ||
| #[allow(clippy::large_enum_variant)] | ||
| pub enum InvalidDkgPayloadReason { | ||
| CryptoError(CryptoError), | ||
| DkgVerifyDealingError(DkgVerifyDealingError), | ||
| MismatchedDkgSummary(DkgSummary, DkgSummary), | ||
| MissingDkgConfigForDealing, | ||
| DkgStartHeightDoesNotMatchParentBlock, | ||
| DkgSummaryAtNonStartHeight(Height), | ||
| SubnetSplittingNotEnabled, |
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.
What does "not enabled" mean? I don't think these variants are used anywhere (yet?).
| subnet_id: SubnetId, | ||
| registry: &dyn RegistryClient, | ||
| registry_version: RegistryVersion, | ||
| subnet_splitting_status: Option<SubnetSplittingStatus>, |
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 assume the Option part will disappear eventually? Do I understand correctly that calling this function with SubnetSplittingStatus::Scheduled { .. } doesn't make sense? Maybe we could change the argument to Option<SubnetId> where None corresponds to SubnetSplittingStatus::NotScheduled and Some(id) corresponds to SubnetSplittingStatus::Done { id }?
Or we could use the new CatchUpPackageType enum from below
|
|
||
| pub struct DependenciesBuilder { | ||
| pool_config: ArtifactPoolConfig, | ||
| records: Vec<(u64, SubnetId, SubnetRecord)>, |
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.
| records: Vec<(u64, SubnetId, SubnetRecord)>, | |
| subnet_records: Vec<(u64, SubnetId, SubnetRecord)>, |
| if let Some(last_version) = last_version { | ||
| add_subnet_list_record( | ||
| ®istry_data_provider, | ||
| last_version, | ||
| Vec::from_iter(subnet_ids), | ||
| ); | ||
| } |
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 do we need to do this twice if last_version != version (see line 174)?
| cup_block: &Block, | ||
| cup_type: CatchUpPackageType, | ||
| ) -> Result<NiDkgId, String> { | ||
| // TODO: can we always take the transcript from the block? |
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.
Intuitively I would say yes, because the summary block is the first block of the current interval
| } | ||
|
|
||
| let cup_content = | ||
| CatchUpContent::from_share_content(shares[0].content.clone(), block.clone()); |
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.
Is this clone necessary?
| CatchUpContent::from_share_content(shares[0].content.clone(), block.clone()); | |
| CatchUpContent::from_share_content(shares[0].content.clone(), block); |
| fn aggregate_catch_up_package_shares_for_summary_block( | ||
| &self, | ||
| pool: &PoolReader<'_>, | ||
| summary_block: &Block, |
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.
Does this work?
| summary_block: &Block, | |
| summary_block: Block, |
| .get_catch_up_package_shares(block.height()) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| if shares.len() < threshold { |
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 guess despite this check we might still not have enough shares below, because get_catch_up_package_shares above might return shares created by the other half of the subnet?
Or would the signature verification fail when validating those shares?
| InvalidArtifactReason::InvalidHeightInSplittingCatchUpPackageShare.into(), | ||
| ); | ||
| } | ||
| CatchUpPackageType::PostSplit { .. } | CatchUpPackageType::Normal => { |
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.
When do we ever expect to see the CatchUpPackageType::PostSplit { .. } case here?
Background
During subnet splitting, when replicas detect that a subnet should be split at a summary height
H, they will immediately start producing CUP shares at heightH + dkg_interval_length, according to the post-split subnet memberships, i.e.half of the node will produce the shares as members of the original subnet, and the other half as the member of the new subnet.
Implement the logic of subnet splitting CUP share creation, validation, and aggregation.
CUP shares creation
When a replica detects that a "subnet splitting" block (denoted by a new field,
subnet-splitting-statusin theDkgSummarystruct) has been finalized and the state at the block's height has been committed, it will create a CUP in the following way:CUP shares validation
Validation works in the following way:
Note
As the field
subnet_splitting_statusis not yet population, the new code branches are never executed.