Repository navigation
pkc%feat(bls): decode BLS fields under a scheme picked at run time - #123
PastaPastaPasta wants to merge 6 commits into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 5 minutes. View limit details
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. |
|
Warning This pull request may have conflicts, please coordinate with the authors of these pull requests. Potential conflicts |
github-actions
Bot
added
the
status:behind
|
✅ Final review complete — no blockers (commit 553b6a9) |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Updated after discussion: 1 finding(s) of this review were settled in the inline threads.
- ✅ Withdrawn: 🟡 Reuse the point decoded during canonicality checking
Verified the review against head 553b6a9 and confirmed one non-blocking performance suggestion: the new wrapper decoder repeats curve decompression after canonicality checking. No blocking issue was identified, and current CI reports passing corpus, lint, MSRV, stable-platform and cross-build checks. Validation was static; no local builds or tests were run.
🟡 1 0 suggestion(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); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The change adds intricate cryptographic decoding and stateful encoding logic in wrap_codec.rs, wrap_lazy.rs and wrap_secret.rs, directly changing BLS signature, public-key and secret-key handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-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; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/wrap_codec.rs`:
- [SUGGESTION → WITHDRAWN in discussion; do not fix] pkgs/pkc/src/bls/wrap_codec.rs:196-202: Reuse the point decoded during canonicality checking
`Self::is_canonical` already decompresses the input: the basic path calls `BlsScIetf::is_canonical_sig`, which invokes `sig_from_bytes`, and the legacy path calls its corresponding decoder. The branches below then decompress the same input again to obtain the point. G1 repeats this pattern at lines 157–166. Successful on-curve reads therefore perform two decompressions during eager decoding, lazy first `get`, and each valid global `get`, adding avoidable cryptographic work to field parsing. Factor an internal canonical-read helper that returns the decoded point or an explicit exotic outcome and reuse that result here. Preserve the basic subgroup and identity checks, the legacy off-curve acceptance rules, and the boolean-only canonicality path for callers that do not need point classification.
| if !Self::is_canonical(bytes, scheme) { | ||
| return None; | ||
| } | ||
| Some(match scheme { | ||
| BlsSchemeKind::Basic => (*bytes, BlsPoint::Usable(G2Affine::uncompress(bytes).ok()?)), | ||
| BlsSchemeKind::Legacy => { | ||
| let point = BlsScChia::sig_from_bytes(bytes).ok(); |
There was a problem hiding this comment.
✅ Withdrawn at
553b6a9b; see the replies below.
🟡 Suggestion: Reuse the point decoded during canonicality checking
Self::is_canonical already decompresses the input: the basic path calls BlsScIetf::is_canonical_sig, which invokes sig_from_bytes, and the legacy path calls its corresponding decoder. The branches below then decompress the same input again to obtain the point. G1 repeats this pattern at lines 157–166. Successful on-curve reads therefore perform two decompressions during eager decoding, lazy first get, and each valid global get, adding avoidable cryptographic work to field parsing. Factor an internal canonical-read helper that returns the decoded point or an explicit exotic outcome and reuse that result here. Preserve the basic subgroup and identity checks, the legacy off-curve acceptance rules, and the boolean-only canonicality path for callers that do not need point classification.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
There was a problem hiding this comment.
Leaving this as it is in this pull request. Each scheme keeps its canonical rule in one place, is_canonical_pk and is_canonical_sig, and the wrapper only asks that rule for a yes or no. To reuse the point it decodes, the wrapper would need either a second copy of the rule or a new BlsScheme method that returns the point along with the answer. Either one is a larger change than the decompression it saves, and the second changes the scheme API this pull request otherwise only builds on.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
I agree with leaving this as-is. Rechecking the scheme implementations, the canonicality methods own both the round-trip checks and the legacy off-curve exceptions, so safely reusing their decoded points needs a shared scheme-level refactor, not just a local wrapper change. The repeated decompression is real, but I haven't established a performance cost that justifies that refactor in this PR; I'm withdrawing the suggestion.
Withdrawing this finding.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review after discussion — commit 553b6a9
Standing review was COMMENTED; this re-review is APPROVE: no blocking findings remain.
Approved: the discussion withdrew or deferred every finding the full review of 553b6a9b (run 8980) raised, and the approval rests on that review.
Same commit as the standing review; the inline threads above carry the per-finding outcome. This follow-up exists only to correct the verdict.
Review provenance
Source: conversation lane gpt-6.1-sol (agent: conversation); no reviewer or verifier lanes ran for this follow-up
- Verdict moved because the inline discussion withdrew or deferred findings of the standing review of this commit; the code was not re-reviewed
There was a problem hiding this comment.
This PR effectively undoes the whole thesis of dash-pkc, it introduces state (runtime-dependent behaviour), gets rid of the boundary between the two schemes (byte bags versus operative types) by essentially allowing "casting" between schemes at the operative level.
Absolutely not.
|
Understood, thanks. Picking the scheme at run time, and the wrapper's lazy and cross-scheme behaviour, don't belong in 🤖 Posted autonomously by Claude on behalf of pasta. |
Motivation
The BLS types in
dash-pkcread keys and signatures under a scheme fixed by a type parameter,BlsScChiaorBlsScIetf. Matching the reference implementation needs the scheme picked at run time, and a field read differently from a plain key read. All-zero bytes give an invalid object that encodes as zeros. Bytes canonical only under the other scheme give the same invalid object rather than an error. Only bytes canonical under neither scheme fail. Its lazy fields keep their bytes until first use and then change in ways a re-encoding exposes: a failed decode drops the bytes, and encoding under the other scheme before a decode writes zeros. Matching the reference implementation byte for byte needs types that follow these rules.This pull request adds those types on top of the legacy reads in #101, with a corpus written from the reference implementation's own wrapper and a test that replays it.
Additional Information
BlsCtxBlsSchemeKindis the scheme as a value, andBlsCtxcarries the scheme a caller decodes under. They need no feature.BlsScheme::KINDgives each scheme type'sBlsSchemeKind.BlsEagerdecodes under aBlsSchemeKind: zeros or bytes canonical only under the other scheme give the invalid object, and bytes canonical under neither scheme fail withInvalidPublicKeyorInvalidSignature. It holds only the decoded object and encodes under the scheme it is given.BlsLazykeeps the bytes untilgetand changes ongetand on encoding as the reference wrapper does; the methods that change it take&mut self, so it needs no lock, andencodedandencoded_asgive the same bytes through&selfwithout the change.BlsValueholds a valid point or the invalid object, encodes under either scheme, and converts to and fromBlsPublicKey<S>andBlsSignature<S>through the encoding underS. A valid legacy object may hold a point off the curve or outside the subgroup, reported asBlsPoint::Exotic, which converts in neither direction. The sealed traitBlsWirecovers points only.BlsEagerSecretKeydecodes secret keys under each scheme's range (legacy0 < k < r, basic0 < k <= r); it holds the key asBlsSkBytes, which is wiped on drop, compares in constant time, redacts itsDebugoutput and has noHash, andto_bytesreturnsZeroizing<[u8; 32]>.BlsGlobalBlsGlobal::decodeapplies that check alone, and the field decodes before its context is known.get(ctx)gives the field under the context, which is what it encodes as.gen_read.cppmove tocontrib/util/inputs.hpp, so the wrapper program draws from the same classes.bls_read.json5comes out byte-identical.gen_wrapper.cpplinks the pinned release's wrapper and writesbls_wrapper.json5: eager reads of keys, signatures and secret keys under each scheme, runs of a construction and up to six operations on lazy objects, and comparisons of lazy objects in either order. Inputs include the first input of a class with the legacy sign bit flipped. The signature comparisons take legacy points outside the subgroup and their flipped twins in place of basic-only points with the sign bit set, which the eager reads still cover. Every outcome comes from the wrapper itself; the program cross-checks each one against the library's reads and stops, writing nothing, when they disagree.wrap_eager,wrap_secret,wrap_globalandwrap_lazyreplay the corpus: 226 eager inputs, 3960 lazy runs and 7200 comparisons, each comparison in both orders.BlsLazy's andBlsGlobal's==compare their state, so they can implementEqandHash.BlsLazy::eq_decodecompares as the reference wrapper does, decoding either side when needed; forBlsGlobal, the fieldsgetgives compare that way.Breaking Changes
None. Only additions to the public API.
How Has This Been Tested?
regen.py --program wrapperand--program readwritebls_wrapper.json5andbls_read.json5byte-identical to the committed ones. A planted wrong outcome ingen_wrapper.cppstops the run with exit status 1, the committed corpus untouched and an empty output directory left empty.cargo clippy -p dash-pkc --all-features --all-targets -- -D warnings, andcargo nextest run -p dash-pkcwith all features and with default features.cargo nextest run --workspace --all-features,cargo test -p dash-pkc --doc --all-featuresandRUSTDOCFLAGS='-D warnings' cargo doc -p dash-pkc --all-features --no-deps, and the lint suite apart from CodeQL is clean.getthat keeps its bytes, encoding under the other scheme without taking it), in the basic secret key range, in the comparison of valid objects (ignoring the last byte) and in the conversion to keys (keeping exotic points) each fail the tests.Checklist
comments and concerns without said assistance. I assume authorship of the code submitted.
Agent disclosure
Code, commit messages and this description were generated with Claude Code using Claude Opus 5.5, running autonomously as an agent on behalf of the author. The agent also built the pinned release's BLS library and wrote the corpus with it. The change was reviewed by a Claude Code agent using Opus 5.5.
🤖 Generated with Claude Code