Conversation
…c into eichhorl/rm-summary-remote-dkg
This reverts commit 7074dd4.
…to eichhorl/early-dkg-configs
There was a problem hiding this comment.
Pull request overview
This draft refactors consensus DKG so remote/initial DKG configs are derived directly from replicated state during data-block production and DKG pool processing, instead of being carried in DkgSummary. It touches core consensus/DKG paths, replica wiring, and a broad set of tests/mocks.
Changes:
- Moves remote DKG config/transcript handling out of summary construction and into state-driven data-payload / DKG-pool logic.
- Extends
DkgImplwith subnet, registry, and state-reader dependencies to build remote configs on demand. - Updates tests/utilities to match the new constructor signatures and early-remote-DKG behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
rs/types/types/src/consensus/dkg.rs |
Simplifies DkgSummary::new and stops passing remote transcripts into the constructor. |
rs/tests/consensus/subnet_splitting_test.rs |
Adjusts the test DKG interval constant. |
rs/test_utilities/consensus/src/fake.rs |
Updates fake DkgSummary construction for the new signature. |
rs/replica/setup_ic_network/src/lib.rs |
Wires new DkgImpl dependencies in replica startup. |
rs/consensus/tests/payload.rs |
Updates consensus test setup/mocks for the new DKG constructor and state access. |
rs/consensus/tests/framework/runner.rs |
Passes subnet/registry/state into the DKG test runner. |
rs/consensus/idkg/src/payload_builder.rs |
Adapts IDKG test summary creation to the new DkgSummary::new signature. |
rs/consensus/dkg/src/test_utils.rs |
Updates DKG state-manager helpers and removes obsolete remote-request helpers. |
rs/consensus/dkg/src/remote.rs |
Keeps remote-config test helpers aligned with the new summary shape. |
rs/consensus/dkg/src/payload_validator.rs |
Validates dealings/transcripts against remote configs rebuilt from state. |
rs/consensus/dkg/src/payload_builder.rs |
Refactors summary/data payload building around early remote configs and state-derived callbacks. |
rs/consensus/dkg/src/lib.rs |
Extends DkgImpl and changes on-state-change logic to merge summary configs with remote configs from state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (dkg_id, config) in last_summary.configs.iter() { | ||
| if completed_dkgs.contains(dkg_id) { | ||
| // Skip DKGs that have already been completed as part of data blocks | ||
| if dkg_id.target_subnet != NiDkgTargetSubnet::Local { | ||
| // Skip remote DKGs | ||
| continue; |
There was a problem hiding this comment.
Yes, technically including dealings in the last data block is pointless now. More generally, it is pointless to include dealings for transcripts that cannot be completed anymore in the current interval, given a limited amount of remaining data blocks and dealings per block.
These transcripts will instead be retried during the next interval. Fixing this is only an optimization and requires enough work to be deferred to a follow up PR: Ideally we not only stop including dealings into blocks once we know there aren't enough data blocks left, but also stop creating those dealings in the first place. To do this, the DKG client needs to know at which state height a remote DKG request was inserted, and how far this height is from the next summary. Currently this is not possible, as the remote DKG context does not contain the insertion height. This could be added in the future.
| let remote_dkg_attempts = get_updated_remote_dkg_attempts( | ||
| last_summary, | ||
| state.get_ref(), | ||
| get_completed_target_ids(&completed_dkgs), |
There was a problem hiding this comment.
context.registry_version should never be ahead of the subnet for more than a couple seconds. If it is, then that indicates a bug and removing such contexts seems like a good idea. Otherwise malformed contexts might never time out, if their registry version is too far in the future.
| validation_context.registry_version, | ||
| last_dkg_summary, | ||
| &logger, | ||
| )?; |
There was a problem hiding this comment.
I'm proposing to instead limit the number of remote contexts in this PR: #9821
No description provided.