fix(mpc): fail chain reads loud on a missing active-member mpc_data record - #1850
Merged
Conversation
…ecord An active committee member's on-chain mpc_data record is written at candidate registration and never emptied, so a missing or undecodable record at read time is always a read defect (fullnode lag, table-walk race, decode failure) — never a legitimate state. Both chain-view committee builders tolerated it as a silent member skip, and since each validator reads through its own fullnode, the tolerated gap became an UNAGREED party-set exclusion: a locally-shrunken class-groups map feeding divergent MPC public inputs across honest validators (the manager's validator-key seed pre-v4, and the reconfiguration MPC input via the legacy next-committee fallback). This was the mechanism behind the issue-#1772 epoch-boundary flake. - get_epoch_start_system: an active member absent from the fetched mpc_data map now fails the whole read (InvalidCommittee) with a should_never_happen error log and a dedicated sui_rpc_errors label; must_get_epoch_start_system retries until the chain view is complete, and the degraded EpochStartSystem can no longer be persisted. - sui_syncer new_committee chain fallback: a missing/undecodable record errors the committee build (new MissingOnChainMpcData variant) so the sync loop retries next tick, instead of dropping the member from the reconfiguration MPC party set. - get_ika_committee: the silent filter_map None arm (reachable only from a pre-gate persisted EpochStartSystem) now logs at error; get_ika_committee_with_network_metadata no longer swallows decode errors with .ok(). - Exclusion decisions belong exclusively to the consensus-agreed freeze; the completeness check lives at the read boundary, NOT on Committee construction (post-freeze assembled committees legitimately omit excluded members). Spec invariant 3 updated to bind every builder of the map and document the read-boundary gates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or 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
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.
Problem
Both chain-view committee builders tolerated a missing/undecodable on-chain
mpc_datarecord for an active committee member as a silent member skip:EpochStartSystem::get_ika_committee(epoch_start_system.rs) — a silentfilter_mapskip buildingCommittee.class_groups_public_keys_and_proofs, which seeds the MPC manager's validator keys pre-v4 (class_groups_keys_by_party_id, live on every deployed network today sincenetwork_encryption_key_version = 3only arrives with protocol v4);new_committee) — the same shape building the next committee that feeds the reconfiguration MPC every epoch under deployed protocol versions, where the missing-record case was dropped with no log at all.A record cannot legitimately be absent: it is written at candidate registration (
validator_info.movefillsmpc_data_bytesat creation) and rotation never empties it — under v4 chain writes remain. So every gap is a read defect (fullnode lag, table-walk race, decode failure). Because each validator reads through its own fullnode, the tolerated gap became an unagreed party-set exclusion: a locally-shrunken class-groups map producing divergent MPC public inputs across honest validators (the crypto layer deliberately accepts partial key maps and deals only to present parties, so nothing fails locally). Exclusion decisions belong exclusively to the consensus-agreed freeze — this was the follow-up flagged in #1846's investigation (spec invariant 3), and the transient chain-read gap was the mechanism behind the issue-#1772 epoch-boundary flake.Fix
Fail the read, not just the log — retry infrastructure already existed:
get_epoch_start_system(ika-sui-client): an active member absent from the fetched mpc_data map fails the whole read withInvalidCommittee, ashould_never_happenerror log, and a dedicatedsui_rpc_errors{epoch_start_missing_mpc_data}label. All callers go throughmust_get_epoch_start_system, which retries until the chain view is complete — and sinceEpochStartSystemis persisted in the epoch-start configuration, a degraded snapshot can no longer be persisted and rebuilt on every restart. The gate sits above bothSuiClientInnertransports.new_committeechain fallback (sui_syncer): a missing or undecodable record errors the committee build (newDwalletMPCError::MissingOnChainMpcData, wired into thekind()metric-label match); the sync loop retries on the next tick instead of shipping a shrunken reconfiguration party set.Nonearm inget_ika_committee(reachable only from a snapshot persisted before the fetch gate) now logs aterror!;get_ika_committee_with_network_metadatano longer swallows decode errors with.ok().Committee::new: a post-freeze assembled committee legitimately omits excluded members, so a type-level "map covers all members" invariant would be wrong — the completeness check belongs at the chain-read boundary, where no exclusion concept exists.Spec
dev-docs/specs/validator-mpc-data-announcements.mdinvariant 3 updated in the same PR: it now binds every builder of the map, documents the read-boundary gates and the unagreed-exclusion rationale, and the code anchors point at both gates.Trade-off, considered and accepted: a persistently inconsistent fullnode now stalls epoch entry visibly (warn + metric per retry) instead of letting the validator enter with divergent MPC input. Stuck-loudly is diagnosable in minutes; divergent-input failures historically surfaced as false-malicious convictions and took days.
Validation
cargo check --release --workspace --all-targetsclean (caught the one exhaustive-match site needing the new error variant's arm); clippy clean on the three touched crates.test_joiner_lands_in_next_committee_class_groupsfrom test(cluster): assert eventual joiner capture instead of first-freeze timing (#1772) #1846, which exercises exactly this epoch-boundary read).🤖 Generated with Claude Code