pkc%feat(bls): migrate operational types to Bls{PublicKey,SecretKey,BlsSignature}<S>, reap bls_{chia,ietf} modules for unified bls module - #25
Merged
Conversation
|
Note This pull request has no conflicts! 🎊 🎉 🎊 |
📝 WalkthroughWalkthroughThe PR replaces separate Chia and IETF BLS APIs with scheme-generic key, signature, aggregation, threshold, and proof-of-possession APIs. It removes legacy modules, updates benchmarks and dependencies, and adjusts CodeQL secret classification. ChangesGeneric BLS API
CodeQL secret classification
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BlsSecretKey
participant BlsSignature
participant BlsScheme
Caller->>BlsSecretKey: sign(message)
BlsSecretKey->>BlsScheme: sign_with(secret key, message)
BlsScheme-->>BlsSecretKey: inner signature
BlsSecretKey-->>Caller: BlsSignature
Caller->>BlsSignature: verify(message, public key)
BlsSignature->>BlsScheme: verify_with(signature, message, public key)
BlsScheme-->>BlsSignature: verify_ok(BLST_ERROR)
BlsSignature-->>Caller: Result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkgs/pkc/src/bls/scheme_ietf.rs (1)
158-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the DST selection into one helper.
sign_withandverify_withrepeat the samematchoverBlsSigId. If one copy changes, signing and verification can select different domain separation tags, and the mismatch is silent until a verification fails.♻️ Proposed refactor
impl BlsScIetf { + /// Select the DST for the given signature scheme id. + fn dst_for(id: BlsSigId) -> &'static [u8] { + match id { + BlsSigId::Basic => DST_BASIC, + BlsSigId::ProofOfPossession => DST_POP, + } + } + /// Sign under the DST selected by `id`. pub(crate) fn sign_with(sk: &SecretKey, msg: &[u8], id: BlsSigId) -> Signature { - let dst = match id { - BlsSigId::Basic => DST_BASIC, - BlsSigId::ProofOfPossession => DST_POP, - }; - sk.sign(msg, dst, &[]) + sk.sign(msg, Self::dst_for(id), &[]) } /// Verify under the DST selected by `id`. /// /// # Errors /// /// Returns `VerifyFailed` when the pairing check does not hold. pub(crate) fn verify_with(sig: &Signature, msg: &[u8], pk: &PublicKey, id: BlsSigId) -> Result<(), BlsError> { - let dst = match id { - BlsSigId::Basic => DST_BASIC, - BlsSigId::ProofOfPossession => DST_POP, - }; - verify_ok(sig.verify(true, msg, dst, &[], pk, true)) + verify_ok(sig.verify(true, msg, Self::dst_for(id), &[], pk, true)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkgs/pkc/src/bls/scheme_ietf.rs` around lines 158 - 179, Extract the shared BlsSigId-to-DST match into a helper associated with BlsScIetf, then update sign_with and verify_with to reuse it instead of selecting the DST independently. Preserve the existing DST_BASIC and DST_POP mappings.pkgs/pkc/src/bls/sig_aggregate.rs (1)
150-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
verify_aggregateserror contract.
BlsScIetf::verify_aggregatesdocumentsCountMismatchfor unequal counts andEmptyAggregationfor no keys.ietf_verify_distinct_messagescovers only the success path and the swapped-message path. Add cases for a message/key count mismatch and for empty inputs, so the documented error contract stays pinned.💚 Proposed additional test
+ #[rstest] + fn ietf_verify_aggregates_rejects_bad_inputs() { + let sk = BlsSecretKey::<BlsScIetf>::generate(&SEED_0).unwrap(); + let msg: &[u8] = b"first message"; + let sig = sk.sign(msg); + let pk = sk.public_key(); + + assert!(matches!( + sig.verify_aggregates(&[msg], &[&pk, &pk]), + Err(BlsError::CountMismatch) + )); + assert!(matches!( + sig.verify_aggregates(&[], &[]), + Err(BlsError::EmptyAggregation) + )); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkgs/pkc/src/bls/sig_aggregate.rs` around lines 150 - 165, Extend ietf_verify_distinct_messages to assert the verify_aggregates error contract: add a case with unequal message and public-key counts that returns CountMismatch, and a case with empty inputs that returns EmptyAggregation. Keep the existing successful and swapped-message assertions unchanged.pkgs/pkc/src/bls/sig_threshold.rs (1)
17-31: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocument that
Okdoes not prove a sufficient quorum.
recoverinterpolates any set of two or more valid shares. The testassert_sub_threshold_does_not_verifyat lines 79-91 confirms that a below-threshold set still returnsOk, and only verification rejects the result. The current Rustdoc does not state this. A caller can readOkas proof of a valid quorum. Add a note that the caller must verify the recovered signature against the group public key.📝 Proposed doc addition
/// Recover a full signature from threshold signature shares via Lagrange /// interpolation in G2. /// + /// Recovery does not check that the share count reaches the threshold used + /// at split time. A below-threshold set still interpolates to a point and + /// returns `Ok`. Verify the recovered signature against the group public + /// key before you trust it. + /// /// # Errors🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkgs/pkc/src/bls/sig_threshold.rs` around lines 17 - 31, Update the Rustdoc for `BlsSignature::recover` to state that successful interpolation does not establish a sufficient quorum, since two or more valid shares may return `Ok`. Explicitly instruct callers to verify the recovered signature against the group public key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkgs/pkc/bench/bls.rs`:
- Around line 171-184: Update the 1,000-item benchmark setup around BlsSecretKey
generation and test_msg calls to use inputs that remain unique across all n
entries, avoiding the u8 truncation at 256. Prefer usize-compatible helper
inputs while preserving the existing n-sized keys, messages, signatures, and
aggregate flow.
In `@pkgs/pkc/src/bls/scheme_ietf.rs`:
- Around line 203-211: Update verify_aggregates to reject duplicate entries in
msgs before invoking sig.aggregate_verify, while preserving the existing count
and empty-aggregation checks. Detect equality across the full message byte
slices, return the appropriate BlsError for invalid aggregation, and only call
aggregate_verify when all messages are unique.
In `@pkgs/pkc/src/bls/secret_ops.rs`:
- Around line 63-75: Change dh_exchange to return a dedicated shared-secret
newtype rather than BlsPublicKey, with redacted Debug, constant-time equality,
and zeroization, and update callers accordingly. Add explicit prime-order
subgroup validation in the BlsScChia DH implementation before scalar
multiplication, including validation of the peer key and resulting point as
required by the existing error contract. Add a regression test covering
rejection of an on-curve, non-subgroup peer key.
---
Nitpick comments:
In `@pkgs/pkc/src/bls/scheme_ietf.rs`:
- Around line 158-179: Extract the shared BlsSigId-to-DST match into a helper
associated with BlsScIetf, then update sign_with and verify_with to reuse it
instead of selecting the DST independently. Preserve the existing DST_BASIC and
DST_POP mappings.
In `@pkgs/pkc/src/bls/sig_aggregate.rs`:
- Around line 150-165: Extend ietf_verify_distinct_messages to assert the
verify_aggregates error contract: add a case with unequal message and public-key
counts that returns CountMismatch, and a case with empty inputs that returns
EmptyAggregation. Keep the existing successful and swapped-message assertions
unchanged.
In `@pkgs/pkc/src/bls/sig_threshold.rs`:
- Around line 17-31: Update the Rustdoc for `BlsSignature::recover` to state
that successful interpolation does not establish a sufficient quorum, since two
or more valid shares may return `Ok`. Explicitly instruct callers to verify the
recovered signature against the group public key.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc0d1af9-30c9-4d4f-a786-35c53aa16017
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (53)
contrib/codeql/lib/policy.qllcontrib/codeql/zeroize.qlpkgs/pkc/Cargo.tomlpkgs/pkc/bench/bls.rspkgs/pkc/bench/bls_chia.rspkgs/pkc/bench/bls_ietf.rspkgs/pkc/bench/main.rspkgs/pkc/src/bls/blst_ffi.rspkgs/pkc/src/bls/chia_h2c.rspkgs/pkc/src/bls/mod.rspkgs/pkc/src/bls/public_ops.rspkgs/pkc/src/bls/scheme_chia.rspkgs/pkc/src/bls/scheme_ietf.rspkgs/pkc/src/bls/scheme_ops.rspkgs/pkc/src/bls/secret_ops.rspkgs/pkc/src/bls/share_ops.rspkgs/pkc/src/bls/sig_aggregate.rspkgs/pkc/src/bls/sig_basic.rspkgs/pkc/src/bls/sig_pop.rspkgs/pkc/src/bls/sig_threshold.rspkgs/pkc/src/bls/tests.rspkgs/pkc/src/bls_chia/agg.rspkgs/pkc/src/bls_chia/mod.rspkgs/pkc/src/bls_chia/pk.rspkgs/pkc/src/bls_chia/sig.rspkgs/pkc/src/bls_chia/sk.rspkgs/pkc/src/bls_chia/threshold.rspkgs/pkc/src/bls_ietf/agg.rspkgs/pkc/src/bls_ietf/mod.rspkgs/pkc/src/bls_ietf/pk.rspkgs/pkc/src/bls_ietf/sig.rspkgs/pkc/src/bls_ietf/sk.rspkgs/pkc/src/bls_ietf/threshold.rspkgs/pkc/src/common/bls/contract.rspkgs/pkc/src/common/bls/mod.rspkgs/pkc/src/common/mod.rspkgs/pkc/src/lib.rspkgs/pkc/src/prelude.rspkgs/pkc/tests/bls_chia_aggregate.rspkgs/pkc/tests/bls_chia_dh.rspkgs/pkc/tests/bls_chia_keygen.rspkgs/pkc/tests/bls_chia_llmq.rspkgs/pkc/tests/bls_chia_ser.rspkgs/pkc/tests/bls_chia_sign.rspkgs/pkc/tests/bls_chia_threshold.rspkgs/pkc/tests/bls_ietf_aggregate.rspkgs/pkc/tests/bls_ietf_dh.rspkgs/pkc/tests/bls_ietf_keygen.rspkgs/pkc/tests/bls_ietf_llmq.rspkgs/pkc/tests/bls_ietf_pop.rspkgs/pkc/tests/bls_ietf_sign.rspkgs/pkc/tests/bls_ietf_threshold.rspkgs/pkc/tests/common/mod.rs
💤 Files with no reviewable changes (33)
- pkgs/pkc/tests/common/mod.rs
- pkgs/pkc/src/bls_chia/sk.rs
- pkgs/pkc/src/bls_chia/pk.rs
- pkgs/pkc/bench/bls_ietf.rs
- pkgs/pkc/src/bls_chia/sig.rs
- pkgs/pkc/src/lib.rs
- pkgs/pkc/src/bls_ietf/sig.rs
- pkgs/pkc/tests/bls_ietf_dh.rs
- pkgs/pkc/tests/bls_ietf_pop.rs
- pkgs/pkc/tests/bls_ietf_keygen.rs
- pkgs/pkc/tests/bls_chia_dh.rs
- pkgs/pkc/src/bls_chia/agg.rs
- pkgs/pkc/src/bls_ietf/mod.rs
- pkgs/pkc/src/bls_chia/mod.rs
- pkgs/pkc/src/common/bls/contract.rs
- pkgs/pkc/bench/bls_chia.rs
- pkgs/pkc/src/bls_ietf/sk.rs
- pkgs/pkc/tests/bls_ietf_llmq.rs
- pkgs/pkc/tests/bls_chia_aggregate.rs
- pkgs/pkc/src/common/mod.rs
- pkgs/pkc/tests/bls_chia_llmq.rs
- pkgs/pkc/tests/bls_chia_threshold.rs
- pkgs/pkc/tests/bls_chia_ser.rs
- pkgs/pkc/tests/bls_chia_keygen.rs
- pkgs/pkc/src/bls_ietf/pk.rs
- pkgs/pkc/tests/bls_ietf_aggregate.rs
- pkgs/pkc/src/bls_chia/threshold.rs
- pkgs/pkc/tests/bls_chia_sign.rs
- pkgs/pkc/src/bls_ietf/agg.rs
- pkgs/pkc/src/common/bls/mod.rs
- pkgs/pkc/tests/bls_ietf_sign.rs
- pkgs/pkc/tests/bls_ietf_threshold.rs
- pkgs/pkc/src/bls_ietf/threshold.rs
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
1 participant
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.
Additional Information
Depends on pkc%fix(bls): implement
Fr,Fp{,2},G{1,2}{,Affine}wrappers, usedraft-03keygen, improve conformance withbls-signatures#20Depends on sdk%refac: consolidate corpus logic, JSON and hex helpers to
dash-dev, defineBlsSchemetrait to supersede old layout #21Depends on pkc%feat: add fixed-length buffers for secret-holding types, enforce
subtleandzeroizeuse with CodeQL, implement ungated byte bags for BLS and ECDSA types, addenum_maphelper macro #22Depends on types%feat(secret): extract array-bound codec paths to dedicated module, add new macros
dlgt_scodec!andderive_sbytes!, expandderive_{,s}bytes!scope, extractCompactSize#24All
Clone,Debug,PartialEq,Eq, andHashimpls on the generic types are hand-written rather than derived, because a derive would place the bound onSinstead of on the wrapped point. For the same reason the serde attributes carry#[serde(bound(serialize = "", deserialize = ""))].CodeQL's
secretTypeincontrib/codeql/zeroize.qlnow matches on shape (wipesSelf) as well as on name, so a type that behaves like a secret is subject to the redaction, constant-time, buffer, and encoder rules even if its name misses the exclusions list.Breaking Changes
SecretKey::to_bytes() -> [u8; 32]BlsSecretKey::<S>::to_bytes() -> Zeroizing<[u8; 32]>bls_chia::{PublicKey, SecretKey, Signature}bls::{BlsPublicKey, BlsSecretKey, BlsSignature}<BlsScChia>bls_ietf::{PublicKey, SecretKey, Signature}bls::{BlsPublicKey, BlsSecretKey, BlsSignature}<BlsScIetf>bls_{chia,ietf}::threshold::SecretKeySharebls::BlsSkShare<S>bls_{chia,ietf}::threshold::SignatureSharebls::BlsSigShare<S>bls_ietf::Schemebls::BlsSigIdPublicKey::dh_exchange(sk, peer_pk)BlsSecretKey::<S>::dh_exchange(&self, peer_pk)bls_ietf::SecretKey::prove_possession,bls_ietf::PublicKey::verify_possessionbls::BlsSecretKey::<BlsScIetf>::prove_possession,bls::BlsPublicKey::<BlsScIetf>::verify_possessionbls_{chia,ietf}::aggregate_pkBlsPublicKey::<S>::aggregatebls_{chia,ietf}::aggregate_skBlsSecretKey::<S>::aggregatebls_{chia,ietf}::aggregate_sigBlsSignature::<S>::aggregatebls_{chia,ietf}::{fast,secure}_verify_aggregatesBlsSignature<S>bls_ietf::verify_aggregatesBlsSignature::<BlsScIetf>::verify_aggregatesbls_{chia,ietf}::threshold::split_sk(sk, threshold, ids, rng)BlsSecretKey::<S>::split(&self, threshold, ids, rng)bls_{chia,ietf}::threshold::recover_sig(shares)BlsSignature::<S>::recover(shares)bls_{chia,ietf}::threshold::derive_pk_share(master_pks, id)BlsPublicKey::<S>::derive_share(master_pks, id)How Has This Been Tested?
Checklist