[kwvg]

Motivation

Unblocking the mock quorum builder in platform#5323.

Breaking Changes

See changelog.

How Has This Been Tested?

just comb

Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional tests
• I have made corresponding changes to the documentation
• I did not use agentic assistance (i.e. LLMs, AI) except as disclosed and can answer questions, comments and concerns without said assistance. I assume authorship of the code submitted.
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

Agent disclosure

Code generated with Claude Code using Opus 5.5, medium effort. Reviewed with Claude Code using Opus 5.5, medium effort.

[coderabbitai]

No actionable comments were generated in the recent review.

Summary

Summary by CodeRabbit

• New Features
  • Recover a BLS master secret key from secret-key shares using their associated identifiers.
  • Recovery supports threshold-sized subsets of shares, including subsets taken from different positions in the share set.
  • Recovery rejects invalid inputs, including mismatched share and identifier counts, duplicate identifiers, fewer than two shares, and inputs that produce an invalid secret key.

The BLS API now recovers a master secret key from secret-key shares using scalar interpolation. The change adds recovery corpus generation for multiple threshold splits and tests recovery behavior for both schemes, including LLMQ quorum keys.

Sequence Diagram(s)

sequenceDiagram
  participant Recover as BlsSecretKey recover_shares
  participant Scheme as BlsScheme recover_sk_shares
  participant Interpolate as interpolate_fr
  Recover->>Scheme: share IDs and secret keys
  Scheme->>Interpolate: scalar shares
  Interpolate-->>Scheme: scalar interpolated at zero
  Scheme-->>Recover: parsed secret key

Loading

Suggested reviewers: pastapastapasta, thepastaclaw

Merge Risk: Minimal · up to 0750b

The secret-key recovery change has no identified issue requiring resolution before merge; normal checks still apply.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title names the main change: a new capability to recover a secret key from threshold shares in the BLS module of the pkc package. The raw summary confirms this is the central addition in scheme_op…
Description check		The description states the motivation (unblocking the mock quorum builder in platform#5323), points readers to the changelog for breaking changes, and gives the test command. The changeset adds BlsSec…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the main change: a new capability to recover a secret key from threshold shares in the BLS module of the pkc package. The raw summary confirms this is the central addition in scheme_ops.rs and share_ops.rs. The title is short and specific, with a conventional scope prefix.

Full details: Description check

Explanation

The description states the motivation (unblocking the mock quorum builder in platform#5323), points readers to the changelog for breaking changes, and gives the test command. The changeset adds BlsSecretKey::recover_shares and BlsScheme::recover_sk_shares, and the changelog and tests match that. The description is related to the changeset, though it does not describe the code in detail.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Run configuration

• Source mode: Auto
• Repository: dashpay/base-sdk
• Branch: develop
• File: .coderabbit.yml
• Commit: f588bc7
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: c8e592d6-dbbf-4a66-8a07-c9f8b2acf47f

Commits

Reviewing files that changed from the base of the PR and between 4482463 and 0750b7b.

Files ignored due to path filters: 1

• pkgs/pkc/corpus/bls_threshold.json5 is excluded by !**/*.json5

Files selected for processing: 2

• pkgs/pkc/src/bls/scheme_ops.rs
• pkgs/pkc/src/bls/share_ops.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

[github-actions]

Warning

This pull request may have conflicts, please coordinate with the authors of these pull requests.

Potential conflicts

• pkc%feat(bls)!: read the BLS-IES key from a chosen encoding #120

[thepastaclaw]

✅ Final review complete — no blockers (commit 0750b7b) · triage: critical

[thepastaclaw]

Final validation — Phase 1 + Phase 2

Static verification at head 4482463 found no blocking defects: scalar recovery matches the pinned Dash Core interpolation algorithm and reuses existing identifier validation and secret-handling patterns. Three non-blocking improvements remain in API documentation, direct error-path coverage, and an inaccurate guard comment. Current CI shows all three stable-platform checks and one MSRV build/test passing; lint, corpus verification, cross-builds, and additional checks remain pending.

🟡 2 suggestion(s) | 💬 1 nitpick(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: critical by gpt-6.1-sol (effort low) — The new recover_sk_shares and interpolate_fr functions in pkgs/pkc/src/bls/scheme_ops.rs implement intricate cryptographic secret-key reconstruction, including scalar conversions, Lagrange interpolation, validation, and sensitive-memory cleanup.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer
• Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `pkgs/pkc/src/bls/scheme_ops.rs`:
- [SUGGESTION] pkgs/pkc/src/bls/scheme_ops.rs:631-633: Directly test the secret-recovery count-mismatch guard
  The new recovery tests all call `BlsSecretKey::recover_shares`, which derives equal-length ID and key slices from the same input, so they cannot exercise this `CountMismatch` branch. The separately callable `BlsScheme::recover_sk_shares` needs the guard: without it, `interpolate_fr` would silently truncate its coefficient/scalar `zip` and could return an incorrect key. Add direct scheme-level tests for both mismatch directions and both schemes, keeping at least two secret shares so `InsufficientShares` does not mask the mismatch. The existing `assert_mismatched_id_count_rejected` signature-recovery test provides the pattern.
- [NITPICK] pkgs/pkc/src/bls/scheme_ops.rs:629-630: Describe truncation rather than indexing in the mismatch comment
  `interpolate_fr` iterates with `coeffs.iter_mut().zip(scalars)` rather than indexing the scalar slice. Unlike the sibling G1/G2 helpers, unequal lengths would silently truncate the interpolation, not cause an out-of-bounds access. The guard is correct; update its comment to describe the failure it actually prevents.

In `pkgs/pkc/src/bls/share_ops.rs`:
- [SUGGESTION] pkgs/pkc/src/bls/share_ops.rs:229-236: Document that successful recovery does not authenticate the master key
  This operation receives neither the original threshold nor an expected master public key. Consequently, two or more structurally valid shares below the split's threshold can interpolate to a nonzero scalar and return `Ok` with a key other than the master. The current error documentation explains the minimum of two but does not make this distinction explicit. Document that callers must supply enough matching shares and that `Ok` alone does not establish correct reconstruction; comparing `recovered.public_key()` with a trusted expected public key can validate the result. This is a documentation improvement, not a request to change the Core-compatible interpolation behavior.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Independently reviewed the complete diff at 0750b7b and found no actionable issues; secret-key recovery matches the pinned Dash Core interpolation algorithm and preserves existing input-validation and zeroization conventions. All three prior findings are fixed. This was a static review only: current CI still shows lint, stable-platform builds, and MSRV-related jobs pending, with no failed code checks reported.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: critical by gpt-6.1-sol (effort low) — The new recover_sk_shares and interpolate_fr functions in pkgs/pkc/src/bls/scheme_ops.rs implement intricate cryptographic secret-key reconstruction, including scalar conversion, Lagrange interpolation, validation, and sensitive-memory zeroization.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.