sdk%refac: consolidate corpus logic, JSON and hex helpers to dash-dev, define BlsScheme trait to supersede old layout - #21
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (27)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (24)
📝 WalkthroughWalkthroughThis change centralizes Chia and IETF BLS operations behind a shared scheme trait, adds reusable corpus, JSON, and hex test helpers, migrates dependent tests and benchmarks, updates feature wiring, and introduces CodeQL and Semgrep policy checks. ChangesStatic analysis policy checks
Shared development test infrastructure
Centralized BLS implementation
Sequence Diagram(s)sequenceDiagram
participant BLSAPI
participant BlsScheme
participant ChiaOrIETF
participant CurveMath
participant CorpusTests
BLSAPI->>BlsScheme: invoke key, signature, DH, aggregation, or threshold operation
BlsScheme->>ChiaOrIETF: select scheme implementation
ChiaOrIETF->>CurveMath: encode, decode, hash, multiply, or interpolate
CurveMath-->>ChiaOrIETF: cryptographic result
ChiaOrIETF-->>BlsScheme: scheme result
CorpusTests->>BLSAPI: compare result with corpus vector
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
|
Warning This pull request may have conflicts, please coordinate with the authors of these pull requests. Potential conflicts |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkgs/pkc/src/bls_chia/agg.rs (1)
16-63: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument errors consistently across the legacy Chia BLS API. These delegated public methods return
Resultwithout# Errorsdocumentation.
pkgs/pkc/src/bls_chia/agg.rs#L16-L63: document delegated aggregation and verification failure conditions.pkgs/pkc/src/bls_chia/sig.rs#L28-L41: document decoding and verification failure conditions.🤖 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_chia/agg.rs` around lines 16 - 63, Add Rustdoc # Errors sections to every Result-returning public method in pkgs/pkc/src/bls_chia/agg.rs lines 16-63—aggregate_pk, aggregate_sig, verify_aggregates, fast_verify_aggregates, secure_verify_aggregates, and aggregate_sk—describing delegated aggregation or verification failures. Also document decoding and verification failure conditions for the Result-returning methods in pkgs/pkc/src/bls_chia/sig.rs lines 28-41, keeping the descriptions consistent across the legacy Chia BLS API.Source: Coding guidelines
🧹 Nitpick comments (5)
pkgs/pkc/src/bls_chia/sk.rs (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a
pub(super) fn from_innerover apub(super)secret field.
PublicKeyandSignaturein this crate already exposefrom_inner(used at Lines 50 and 56 here, and inpkgs/pkc/src/bls_ietf/pk.rs:28), whileSecretKeyinstead widens the raw scalar field sopkgs/pkc/src/bls_chia/threshold.rs:103can buildSecretKey(inner). A constructor keeps the secret scalar encapsulated and matches the surrounding pattern.♻️ Proposed change
-pub struct SecretKey(pub(super) blst::blst_scalar); +pub struct SecretKey(blst::blst_scalar);Add alongside the other constructors:
pub(super) fn from_inner(inner: blst::blst_scalar) -> Self { Self(inner) }Then in
pkgs/pkc/src/bls_chia/threshold.rsandpkgs/pkc/src/bls_ietf/threshold.rs, replaceSecretKey(inner)withSecretKey::from_inner(inner).🤖 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_chia/sk.rs` around lines 16 - 18, Encapsulate SecretKey’s raw scalar by making its tuple field private and adding a pub(super) SecretKey::from_inner constructor, matching the existing PublicKey and Signature patterns. Update threshold construction in the bls_chia and bls_ietf modules to call SecretKey::from_inner instead of SecretKey(inner).pkgs/pkc/src/bls/scheme_ops.rs (3)
342-358: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a typed error over
Result<_, ()>forgenerate_shares.The only failure path is
key_gen_v3, and the caller blindly maps()toInvalidSecretKey. ReturningBlsErrordirectly removes themap_err(|()| ...)dance and keeps the failure reason intact.♻️ Proposed signature change
-) -> Result<Vec<RawShare>, ()> { +) -> Result<Vec<RawShare>, BlsError> {- let rand_sk = blst::min_pk::SecretKey::key_gen_v3(ikm.as_ref(), &[]).map_err(|_| ())?; + let rand_sk = blst::min_pk::SecretKey::key_gen_v3(ikm.as_ref(), &[]).map_err(|_| BlsError::InvalidSecretKey)?;Then at the call site (Line 242):
- let raw = generate_shares(&sk_bytes, threshold, ids, rng).map_err(|()| BlsError::InvalidSecretKey)?; + let raw = generate_shares(&sk_bytes, threshold, ids, rng)?;🤖 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_ops.rs` around lines 342 - 358, Update generate_shares to return Result<Vec<RawShare>, BlsError> instead of Result<Vec<RawShare>, ()>, propagate the BlsError from SecretKey::key_gen_v3 without mapping it to (), and adjust its caller to propagate or handle the typed error directly rather than converting it to InvalidSecretKey.
220-234: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
ThresholdTooLargeconflates three distinct input errors.
threshold < 2, emptyids, andthreshold > ids.len()all surface asThresholdTooLarge, so a caller passingthreshold == 1or an empty id list gets a misleading diagnostic. Consider distinct variants (e.g.ThresholdTooSmall,NoShareIds) so the error text matches the actual fault.🤖 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_ops.rs` around lines 220 - 234, Update split_sk to distinguish invalid threshold and share-ID inputs: return a dedicated small-threshold error for threshold < 2, a no-share-IDs error for empty ids, and retain ThresholdTooLarge only when threshold exceeds ids.len(). Add the corresponding BlsError variants and ensure their displayed diagnostics identify the actual fault.
169-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify the 256-bit width on the weight multiply is intentional.
Line 194 passes a literal
256tomul_scalarwhile every other call in this file usesblst_ffi::FR_BITS. The SHA-256 weight is unreduced, so a full 256-bit width is presumably required for reference compatibility, but the literal is easy to misread as a mistake. Consider a named constant (e.g.WEIGHT_BITS) with a one-line rationale.#!/bin/bash # Confirm FR_BITS value and other mul_scalar widths used in the crate. rg -nP --type=rust 'FR_BITS' pkgs/pkc/src rg -nP --type=rust 'mul_scalar\s*\(' pkgs/pkc/src🤖 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_ops.rs` around lines 169 - 199, Clarify the intentional 256-bit width in secure_verify_aggregates by replacing the literal passed to mul_scalar with a named constant such as WEIGHT_BITS, set to 256, and add a brief rationale that the unreduced SHA-256 weight requires the full width for compatibility. Leave the surrounding aggregation and verification logic unchanged.pkgs/pkc/src/bls/scheme_chia.rs (1)
307-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
sig_ietfis never exercised against the implementation.The test decodes and re-encodes only
sig_legacy; the sole use ofsig_ietfis anassert_ne!against the legacy hex, which would pass even if the vector held arbitrary garbage. Consider asserting the decoded signature's IETF compression equalsv.sig_ietfso the cross-format mapping is actually covered.🤖 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_chia.rs` around lines 307 - 318, Update signature_serialization_matches_vectors to also assert that the decoded signature’s IETF serialization matches v.sig_ietf, using the appropriate BlsScChia serialization method. Keep the existing legacy round-trip assertion, and replace the insufficient inequality-only check with validation of the expected IETF encoding.
🤖 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/dev/src/corpus.rs`:
- Around line 111-117: Wrap the overlong expressions in the corpus-loading code
to keep every Rust line within 120 characters, including the serde_json
deserialization panic expression and the hex decoding expressions near the
result-building logic. Preserve the existing behavior and messages while
formatting the chained calls and closures across multiple lines.
In `@pkgs/dev/src/lib.rs`:
- Around line 18-19: Replace the #[allow] attribute on the prelude module with
#[expect], preserving the existing unused-imports lint and reason so the
suppression is reported when no longer needed.
In `@pkgs/pkc/Cargo.toml`:
- Around line 38-45: Replace the `tests` feature with the reserved `_internal`
feature in the `pkc` feature configuration, keeping `rstest` and `std`
dependencies under `_internal` as appropriate. Remove the test feature from
`full`, and update the `cfg` gates in `chia_h2c.rs` and `scheme_chia.rs` plus
all related `required-features` entries to reference `_internal`.
In `@pkgs/pkc/src/bls/scheme_chia.rs`:
- Around line 75-92: Update pk_from_bytes to reject non-canonical legacy
encodings with stray bits 5-6, matching the sig_from_bytes/G2 validation
behavior and preventing non-round-trippable keys. Validate the original first
byte before normalizing it, while preserving the intended handling of the
compression and sign bits; use the existing BlsError::InvalidPublicKey path.
In `@pkgs/pkc/tests/bls_ietf_llmq.rs`:
- Around line 36-43: Restore the per-contributor assertion in the test around
vvec so vvec[0] is verified to match that contributor’s public key, not merely a
valid G1 point. Keep the existing length and parseability checks, and use the
contributor key already available in the surrounding test context.
---
Outside diff comments:
In `@pkgs/pkc/src/bls_chia/agg.rs`:
- Around line 16-63: Add Rustdoc # Errors sections to every Result-returning
public method in pkgs/pkc/src/bls_chia/agg.rs lines 16-63—aggregate_pk,
aggregate_sig, verify_aggregates, fast_verify_aggregates,
secure_verify_aggregates, and aggregate_sk—describing delegated aggregation or
verification failures. Also document decoding and verification failure
conditions for the Result-returning methods in pkgs/pkc/src/bls_chia/sig.rs
lines 28-41, keeping the descriptions consistent across the legacy Chia BLS API.
---
Nitpick comments:
In `@pkgs/pkc/src/bls_chia/sk.rs`:
- Around line 16-18: Encapsulate SecretKey’s raw scalar by making its tuple
field private and adding a pub(super) SecretKey::from_inner constructor,
matching the existing PublicKey and Signature patterns. Update threshold
construction in the bls_chia and bls_ietf modules to call SecretKey::from_inner
instead of SecretKey(inner).
In `@pkgs/pkc/src/bls/scheme_chia.rs`:
- Around line 307-318: Update signature_serialization_matches_vectors to also
assert that the decoded signature’s IETF serialization matches v.sig_ietf, using
the appropriate BlsScChia serialization method. Keep the existing legacy
round-trip assertion, and replace the insufficient inequality-only check with
validation of the expected IETF encoding.
In `@pkgs/pkc/src/bls/scheme_ops.rs`:
- Around line 342-358: Update generate_shares to return Result<Vec<RawShare>,
BlsError> instead of Result<Vec<RawShare>, ()>, propagate the BlsError from
SecretKey::key_gen_v3 without mapping it to (), and adjust its caller to
propagate or handle the typed error directly rather than converting it to
InvalidSecretKey.
- Around line 220-234: Update split_sk to distinguish invalid threshold and
share-ID inputs: return a dedicated small-threshold error for threshold < 2, a
no-share-IDs error for empty ids, and retain ThresholdTooLarge only when
threshold exceeds ids.len(). Add the corresponding BlsError variants and ensure
their displayed diagnostics identify the actual fault.
- Around line 169-199: Clarify the intentional 256-bit width in
secure_verify_aggregates by replacing the literal passed to mul_scalar with a
named constant such as WEIGHT_BITS, set to 256, and add a brief rationale that
the unreduced SHA-256 weight requires the full width for compatibility. Leave
the surrounding aggregation and verification logic unchanged.
🪄 Autofix (Beta)
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: e9e12aa0-2be8-4c73-aabf-12f90cc5167f
⛔ Files ignored due to path filters (42)
Cargo.lockis excluded by!**/*.lock,!**/*.lockpkgs/pkc/corpus/bls_aggregate.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_chia_aggregate.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_chia_dh.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_chia_hash_internals.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_chia_keygen.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_chia_llmq_100.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_chia_secure_aggregate.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_chia_ser_internals.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_chia_sign.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_chia_threshold.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_ietf_aggregate.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_ietf_dh.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_ietf_keygen.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_ietf_llmq_100.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_ietf_secure_aggregate.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_ietf_sign.json5is excluded by!**/*.json5pkgs/pkc/corpus/bls_ietf_threshold.json5is excluded by!**/*.json5pkgs/pkc/corpus/k256_keygen.json5is excluded by!**/*.json5pkgs/pkc/corpus/k256_sign.json5is excluded by!**/*.json5pkgs/pow/corpus/blake.jsonis excluded by!**/*.jsonpkgs/pow/corpus/blake.json5is excluded by!**/*.json5pkgs/pow/corpus/bmw.jsonis excluded by!**/*.jsonpkgs/pow/corpus/bmw.json5is excluded by!**/*.json5pkgs/pow/corpus/cubehash.jsonis excluded by!**/*.jsonpkgs/pow/corpus/cubehash.json5is excluded by!**/*.json5pkgs/pow/corpus/echo.jsonis excluded by!**/*.jsonpkgs/pow/corpus/echo.json5is excluded by!**/*.json5pkgs/pow/corpus/groestl.jsonis excluded by!**/*.jsonpkgs/pow/corpus/groestl.json5is excluded by!**/*.json5pkgs/pow/corpus/jh.jsonis excluded by!**/*.jsonpkgs/pow/corpus/jh.json5is excluded by!**/*.json5pkgs/pow/corpus/keccak.jsonis excluded by!**/*.jsonpkgs/pow/corpus/keccak.json5is excluded by!**/*.json5pkgs/pow/corpus/luffa.jsonis excluded by!**/*.jsonpkgs/pow/corpus/luffa.json5is excluded by!**/*.json5pkgs/pow/corpus/shavite.jsonis excluded by!**/*.jsonpkgs/pow/corpus/shavite.json5is excluded by!**/*.json5pkgs/pow/corpus/simd.jsonis excluded by!**/*.jsonpkgs/pow/corpus/simd.json5is excluded by!**/*.json5pkgs/pow/corpus/skein.jsonis excluded by!**/*.jsonpkgs/pow/corpus/skein.json5is excluded by!**/*.json5
📒 Files selected for processing (88)
contrib/codeql/lib/policy.qllcontrib/codeql/lib/traits.qllcontrib/codeql/trait.qlcontrib/semgrep/cargo.ymlpkgs/dev/Cargo.tomlpkgs/dev/src/corpus.rspkgs/dev/src/encode.rspkgs/dev/src/json.rspkgs/dev/src/lib.rspkgs/num/Cargo.tomlpkgs/num/tests/serde.rspkgs/p2p_core/src/msg/headers2.rspkgs/p2p_core/src/msg/inv.rspkgs/p2p_core/src/msg/ping.rspkgs/p2p_core/src/msg/version.rspkgs/p2p_core/src/primitives/mn_list.rspkgs/pkc/Cargo.tomlpkgs/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/scheme_chia.rspkgs/pkc/src/bls/scheme_ietf.rspkgs/pkc/src/bls/scheme_ops.rspkgs/pkc/src/bls/schemes.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/ser.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/mod.rspkgs/pkc/src/common/bls/threshold.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.rspkgs/pkc/tests/k256_keygen.rspkgs/pkc/tests/k256_sign.rspkgs/pow/Cargo.tomlpkgs/pow/tests/blake.rspkgs/pow/tests/bmw.rspkgs/pow/tests/common/mod.rspkgs/pow/tests/cubehash.rspkgs/pow/tests/echo.rspkgs/pow/tests/groestl.rspkgs/pow/tests/jh.rspkgs/pow/tests/keccak.rspkgs/pow/tests/luffa.rspkgs/pow/tests/shavite.rspkgs/pow/tests/simd_hash.rspkgs/pow/tests/skein.rspkgs/primitives/Cargo.tomlpkgs/primitives/src/block.rspkgs/primitives/src/gov.rspkgs/primitives/src/payload/assetlock.rspkgs/primitives/src/payload/assetunlock.rspkgs/primitives/src/payload/cbtx.rspkgs/primitives/src/payload/mnhftx.rspkgs/primitives/src/payload/proregtx.rspkgs/primitives/src/payload/proupregtx.rspkgs/primitives/src/payload/prouprevtx.rspkgs/primitives/src/payload/proupservtx.rspkgs/primitives/src/payload/quorum.rspkgs/primitives/src/transaction.rspkgs/types/Cargo.toml
💤 Files with no reviewable changes (19)
- pkgs/pow/tests/bmw.rs
- pkgs/pow/tests/groestl.rs
- pkgs/pow/tests/simd_hash.rs
- pkgs/pkc/src/bls_chia/ser.rs
- pkgs/pkc/bench/main.rs
- pkgs/pow/tests/echo.rs
- pkgs/pow/tests/cubehash.rs
- pkgs/pkc/src/common/bls/threshold.rs
- pkgs/pow/tests/skein.rs
- pkgs/pow/tests/keccak.rs
- pkgs/pow/tests/blake.rs
- pkgs/pow/tests/jh.rs
- pkgs/pkc/src/bls_chia/mod.rs
- pkgs/pow/tests/shavite.rs
- pkgs/pkc/src/bls/blst_ffi.rs
- pkgs/pow/tests/luffa.rs
- pkgs/types/Cargo.toml
- pkgs/primitives/Cargo.toml
- pkgs/pkc/src/common/bls/mod.rs
Additional Information
{pk_to_g1,g1_to_pk}and{sig_to_g2,g2_to_sig}exist so the group arithmetic can be lifted without dragging serialization along with itcompress/uncompressbecause blst's key types are opaque. To sidestep this,secure_agg_pointis a separate hook purely so IETF can skip re-validating keys it has already validated.split_sktakes aninto_sharebuilder closure rather than returning a shared share type, because each facade owns its own and unifying them would leak scheme-specific types across the boundary.base-sdk/trait-rules) now enforces mutual exclusion forBlsSchemeto prevent a per-scheme override would silently shadow the shared implementation, forcing either code to be commonly defined or defined per-implbut not both simultaneously.Breaking Changes
BlsScheme::recover_sig_sharesnow returnsInsufficientShareswhenids.len() != sigs.len(), where previously onlysigs.len() < 2was checked. The two slices are paired, so a mismatch would desync interpolation and could index out of bounds ininterpolate_g2.Test-support hex decoding now enforces length during the decode rather than after the fact, so odd-length input yields a hex error instead of a slice-index panic.
Superseded
dash-pkc'sfullfeature now impliestests. Consumers building tests againstfullneed no change; feature enumeration may need to be updated.How Has This Been Tested?
Checklist