Skip to content

Move proposal/VID-share pairing into Consensus.#4698

Merged
twittner merged 3 commits into
mainfrom
tw/proposal-vid-pairing-in-consensus
Jul 18, 2026
Merged

Move proposal/VID-share pairing into Consensus.#4698
twittner merged 3 commits into
mainfrom
tw/proposal-vid-pairing-in-consensus

Conversation

@twittner

Copy link
Copy Markdown
Contributor

The coordinator used to join validated proposals with this node's VID share in ad-hoc caches and only hand the completed pair to consensus.

Consensus now receives the halves as separate inputs (Proposal, VidShare), parks the first arrival keyed by (view, payload commitment), and processes the pair when it completes. The coordinator effects formerly performed at pairing time, i.e. persisting the proposal,
VID share and state cert, and seeding the VID reconstructor, are driven by a new ProposalPaired output.

Behaviour is unchanged: ProposalPaired is emitted before the safety and DRB checks, matching the previous persist-before-checks order, and the parked halves are dropped on pairing and on local view GC exactly as the old caches were.

twittner added 2 commits July 16, 2026 14:09
The coordinator used to join validated proposals with this node's VID
share in ad-hoc caches and only hand the completed pair to consensus.

Consensus now receives the halves as separate inputs (`Proposal`,
`VidShare`), parks the first arrival keyed by (view, payload
commitment), and processes the pair when it completes. The coordinator
effects formerly performed at pairing time, i.e. persisting the proposal,
VID share and state cert, and seeding the VID reconstructor, are
driven by a new `ProposalPaired` output.

Behavior is unchanged: `ProposalPaired` is emitted before the safety
and DRB checks, matching the previous persist-before-checks order, and
the parked halves are dropped on pairing and on local view GC exactly
as the old caches were.
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@gemini-code-assist

Copy link
Copy Markdown
Contributor
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @twittner's task in 3m 12s —— View job


Code Review: Move proposal/VID-share pairing into Consensus

  • Read the diff and understand the changes
  • Review consensus.rs (pairing logic)
  • Review coordinator.rs (ProposalPaired handling)
  • Review test changes
  • Post review feedback

Nice refactor. The pairing logic that lived in ad-hoc coordinator caches now sits inside Consensus, and the coordinator drives its side effects off a new ProposalPaired output. I traced the behavior end-to-end and it faithfully preserves the previous semantics. Verified specifically:

  • Persist-before-checks order preserved. on_proposal_paired pushes ProposalPaired to the outbox before calling handle_proposal_with_vid_share (consensus.rs:1087-1091). Since the outbox is FIFO and the coordinator drains it after apply() returns, the persist + reconstructor-seed effects run before any safety/DRB-gated outputs — matching the old inline ordering in on_proposal_and_vid_share.
  • Pairing always emits regardless of view age. As before, pairing (and thus persistence) happens unconditionally; the "proposal too old" gate in handle_proposal_with_vid_share still runs after the effects. Consistent with the old flow.
  • GC semantics match. The pairing-time split_off(&(view + 1, vc)) (consensus.rs:1085-1086) and the local-GC split_off(&(view, vc)) (consensus.rs:970-971) mirror the coordinator's old cache GC exactly, including the VidCommitment2::default() minimal-key trick.
  • Keying is consistent between the two entry points: proposals key on the VidCommitment::V2(_) extracted from the header, VID shares on their payload_commitment (already a VidCommitment2).
  • No dangling references — old ProposalWithVidShare variant, cached_* fields, and on_proposal_and_vid_share are fully removed; unused imports (ValidatedProposal, VidDisperseShare2) dropped from the coordinator.

Minor observations (non-blocking)

  1. consensus.rs doc comment altitude. The ConsensusOutput::ProposalPaired variant (consensus.rs:160) has no doc comment describing that the coordinator is expected to persist the proposal/VID share and seed the reconstructor in response. Given the tight coupling between this output and the coordinator handler at coordinator.rs:729, a one-line note ("consumer must persist and seed the VID reconstructor") would help future readers who only look at the enum.

  2. Equivocation edge case (pre-existing, just noting). If a view has two parked halves with different payload commitments (e.g. an equivocating leader) and one pairs, on_proposal_paired's split_off(&(view + 1, _)) drops the other parked half for that view, so it can never pair. This matches the old coordinator behavior exactly, so it's not introduced here — flagging only so it's a conscious carry-over.

  3. Test coverage of the split itself. The tests now exercise pairing via apply_pair (proposal then VID share). Since the pairing order is now nondeterministic in production (two independent async streams), it might be worth one focused test that applies the halves in VID-share-first order to lock in that path — most existing tests go proposal-first through apply_pair. Not required, but cheap insurance for the newly-relocated logic. Fix this →

Testing / build

I reviewed statically and did not run cargo build/test for this crate (relying on CI). The test harness changes (apply_pair, the ProposalPaired storage-confirmation simulation in utils.rs:1124) correctly model the new two-input flow, and the direct-consensus.apply tests were updated to feed both halves.

Overall: LGTM. The change is well-scoped, behavior-preserving, and the internal parking model reads more clearly than the previous coordinator-side caches.

@twittner
twittner merged commit 75fd633 into main Jul 18, 2026
144 checks passed
@twittner
twittner deleted the tw/proposal-vid-pairing-in-consensus branch July 18, 2026 10:56
@claude claude Bot mentioned this pull request Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants