Conversation
Using Gemini Code AssistThe 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
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 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 finished @bfish713's task in 3m 43s —— View job Code Review: Verify Certs Off Main Thread
SummaryThis PR cleanly moves certificate verification (threshold-signature checks that take >1ms) off the main coordinator thread using Architecture:
Things done well:
No blocking issues found. The inline comments are observational/informational only. The code is well-reasoned and the refactoring is clean — verification responsibility is properly lifted to the coordinator layer while consensus simply records already-verified certs. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// Yield the next certificate that passed verification. | ||
| pub async fn next(&mut self) -> Option<C> { | ||
| loop { | ||
| match self.tasks.join_next().await { | ||
| Some((view, Ok(Some(cert)))) => { | ||
| if view >= self.lower_bound { | ||
| self.completed.insert(view); | ||
| return Some(cert); | ||
| } | ||
| }, | ||
| // Invalid signature or stale view. | ||
| Some((_, Ok(None))) => {}, | ||
| Some((view, Err(err))) => { | ||
| if !err.is_cancelled() { | ||
| error!(%view, %err, cert = type_name::<C>(), "cert verification task panic"); | ||
| } | ||
| }, | ||
| None => return None, | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential concern: next() returns None when tasks is empty, which resolves the select! branch immediately.
When there are zero in-flight verification tasks, JoinMap::join_next() returns None immediately, so next() returns None, and the select! branch Some(cert1) = self.cert_verifiers.cert1.next() won't match (since None doesn't match Some(...)). This is correct behavior — select! will just skip that branch.
However, if all three verifier JoinMaps happen to be non-empty, there's a subtle fairness question: select! in tokio picks a random ready branch each poll, so the three verifier branches compete with each other and with the vote-collector branches. This seems fine for the expected workload but worth keeping in mind if verification throughput becomes a bottleneck.
No action needed — just flagging for awareness.
| self.pending.insert(view, cert); | ||
| return Some(epoch); | ||
| }; |
There was a problem hiding this comment.
Nit: a cert for a view that's already pending is silently replaced.
If a second certificate for the same view arrives while the first is still pending (epoch unavailable), self.pending.insert(view, cert) replaces the old one. Given certificates for the same view should be identical this is fine in practice, but worth noting that in a scenario where two different certificates for the same view somehow arrived (one valid, one malicious), the last-writer-wins behavior here could discard a valid one. This is unlikely given the duplicate-check at the top of verify(), but BTreeMap::insert does overwrite.
No action needed — just flagging.
| self.leaves.insert(view, proposal.clone().into()); | ||
| self.vid_shares.insert(view, vid_share); | ||
|
|
||
| // Register the proposal's justify_qc so a node that didn't vote in the | ||
| // parent view (e.g. one catching up) still has the parent's QC. | ||
| self.certs | ||
| .entry(proposal.justify_qc.view_number()) | ||
| .or_insert_with(|| proposal.justify_qc.clone()); | ||
|
|
||
| self.adopt_certified_drb(proposal.justify_qc.view_number()); |
There was a problem hiding this comment.
Good additions for catching-up nodes. Registering the proposal's justify_qc as a cert and calling adopt_certified_drb from both the proposal path and handle_certificate1 ensures that a catching-up node that receives the QC (now verified off-thread) and the proposal in either order can still adopt the DRB.
One minor observation: adopt_certified_drb does a proposal_commitment(proposal) != cert.data.leaf_commit comparison every time. For a catching-up node processing many views, this runs repeatedly for proposals in non-transition views, but the early-return checks (non-transition view, drb_results.contains_key) should short-circuit most calls quickly.
| if self.proposed_views.contains(&view) { | ||
| return; | ||
| } | ||
| // Never (re)propose a view at or below `timeout_view`. `proposed_views` | ||
| // resets on restart, so this guards a recovered leader from re-proposing | ||
| // a view it already acted in. Mirrors the guard in `maybe_vote_1`. |
There was a problem hiding this comment.
Good safety addition. The timeout_view guard here correctly mirrors the existing guard in maybe_vote_1 (line 1644 on main). Since proposed_views resets on restart, this prevents a recovered leader from re-proposing a view it already acted in — important now that off-thread cert verification can surface older QCs asynchronously after recovery.
| Some(cert1) = self.cert_verifiers.cert1.next() => { | ||
| finish_measurement(next_input); | ||
| return Ok(ConsensusInput::Certificate1(cert1)) | ||
| } | ||
| Some(cert2) = self.cert_verifiers.cert2.next() => { | ||
| finish_measurement(next_input); | ||
| return Ok(ConsensusInput::Certificate2(cert2)) | ||
| } | ||
| Some(tc) = self.cert_verifiers.timeout.next() => { | ||
| finish_measurement(next_input); | ||
| return Ok(ConsensusInput::TimeoutCertificate(tc)) | ||
| } |
There was a problem hiding this comment.
Observation: Locally-formed certs from vote collectors and network certs from verifiers both feed the same ConsensusInput variants.
Locally-formed certificates (from vote collectors, lines 427-434) bypass the verifier and go straight to consensus — this is correct since the node itself aggregated the threshold signature from individually-verified votes, so re-verifying would be redundant.
Network certificates go through the off-thread verifier first and then arrive here. The ordering between the two sources in select! is fair (random), which is fine.
One subtlety worth noting: if a vote-collected cert and a network cert for the same view both resolve, consensus.apply will be called twice for the same view. The handle_certificate1/handle_certificate2 methods now use .entry(view).or_insert(...) which handles this idempotently — good.
| GcScope::Decided(view) => { | ||
| self.epoch_manager.gc(epoch); | ||
| self.epoch_root_collector.gc(view, epoch); | ||
| // GC the verifiers at the decided view, not the local view: a | ||
| // catching-up node's local view races ahead of its decided view, | ||
| // and GC-ing at the local view would drop pending verifications | ||
| // for certs it still needs. | ||
| self.cert_verifiers.gc(view); |
There was a problem hiding this comment.
Good choice to GC at decided view. The comment explains the reasoning well — a catching-up node's local view races ahead of its decided view, and GC-ing verifiers at the local view would drop certs the node still needs to catch up. GC-ing at decided_view is the right granularity here.
Nextest failures (3) in this run
See the step summary for flaky tests and slowest tests. |
1 participant
Closes #<ISSUE_NUMBER>
This PR:
This PR does not:
Key places to review: