fix(crypto/frost): BLS/FROST primitive parity and edge-case correctness#516
Open
varex83agent wants to merge 4 commits into
Open
fix(crypto/frost): BLS/FROST primitive parity and edge-case correctness#516varex83agent wants to merge 4 commits into
varex83agent wants to merge 4 commits into
Conversation
Charon's Recover rejects any recovery byte outside {0, 1, 27, 28} before
attempting recovery (app/k1util/k1util.go @ v1.7.1). The Rust port only
normalized 27/28 -> 0/1 and then deferred to RecoveryId::from_byte, which
accepts the x-reduced ids 2 and 3 (recovering a different key). Reject
everything outside the Charon domain up front; the error now carries the
original (pre-normalization) byte, matching Charon's z.Any("id", ...).
verify_65 inherits the fix.
Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
…h kryptology
Bring round2 to functional parity with ObolNetwork/kryptology@v0.1.0
(pkg/dkg/frost/dkg_round2.go), the fork Charon v1.7.1 uses:
- bcast upper bound is now max_signers (feldman.Limit), not max_signers-1,
so a Charon-shaped broadcast map that includes this node's own Round1Bcast
is accepted; the p2psend/shares upper bound stays max_signers-1.
- A self entry (sender_id == secret.id) in the broadcast map is skipped
(`continue`) rather than rejected with InvalidParticipantId, matching Go's
`if id == dp.Id { continue }`. Self is thereby excluded from share lookup
and share_sum; own_share_scalar and the own commitment are still added once.
- Add an explicit zero-valued Shamir share guard mirroring kryptology's
ShamirShare.Validate (pkg/sharing/shamir.go:27-39), which rejects id == 0
and a zero scalar before the Feldman VSS equation.
Replaces round2_rejects_self_broadcast with round2_skips_self_broadcast and
adds bound/zero-share regression tests.
Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
…plit errors Align BLS primitives in blst_impl with Charon's Herumi backend (tbls/herumi.go @ v1.7.1): - aggregate: an empty input is no longer an error; return the serialized G2 point at infinity (identity signature), matching Herumi's Aggregate. A single input is deserialized + re-serialized (canonicalized) instead of returned verbatim, so a malformed single input is now rejected. - threshold_split_insecure: map the usize::try_from(threshold) failure to a new dedicated Error::ThresholdOverflow instead of the misleading InvalidThreshold; document the (retained) threshold > total hardening that diverges from Charon's threshold <= 1-only check; rename the misspelled `threashlod` local. - Unify BlstSecretKey::from_bytes error mapping across secret_to_public_key, threshold_split_insecure, and sign to Error::InvalidSecretKey, matching recover_secret (previously three surfaced the generic BlsError). Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
iamquang95
reviewed
Jul 3, 2026
| invalid_recovery_byte: original_recovery_byte, | ||
| })?; | ||
|
|
||
| let pubkey = ecdsa::VerifyingKey::recover_from_prehash(hash, &signature, recovery_id) |
Collaborator
There was a problem hiding this comment.
recover() still rejects high-S signatures that charon accepts.
Go side — charon Recover (app/k1util/k1util.go:120) delegates to decred's ecdsa.RecoverCompact (dcrd/dcrec/secp256k1/v4@v4.4.0 ecdsa/signature.go:796). Its only S validation is signature.go:898-905: reject S >= group order and S == 0. No low-S rule — a high-S signature recovers fine.
Rust side — this call, ecdsa::VerifyingKey::recover_from_prehash, ends with a self-verification ([ecdsa-0.16.9 recovery.rs:313 (https://github.com/RustCrypto/signatures/blob/ecdsa/v0.16.9/ecdsa/src/recovery.rs#L313)) that hits k256's verify_prehashed, which rejects high-S outright (k256-0.13.4 src/ecdsa.rs:203-205: if sig.s().is_high() { return Err })
emlautarom1
approved these changes
Jul 6, 2026
Comment on lines
+344
to
+347
| struct Case { | ||
| v: u8, | ||
| accepted: bool, | ||
| } |
Collaborator
There was a problem hiding this comment.
Too noisy in code, not explicit enough in actual test runs. Prefer to use test_case to parametrize the test.
Comment on lines
+416
to
+420
| // Bounds mirror ObolNetwork/kryptology@v0.1.0 dkg_round2.go. | ||
| // feldman.Limit == max_signers. | ||
| // bcast: threshold-1 <= len <= max_signers (may include this node's | ||
| // own bcast) p2psend: threshold-1 <= len <= max_signers - 1 (never | ||
| // includes self) |
Collaborator
There was a problem hiding this comment.
The formatting here is broken
Comment on lines
193
to
196
| let recovery_id = | ||
| RecoveryId::from_byte(recovery_byte).ok_or(K1UtilError::InvalidSignatureRecoveryId { | ||
| invalid_recovery_byte: recovery_byte, | ||
| invalid_recovery_byte: original_recovery_byte, | ||
| })?; |
Collaborator
There was a problem hiding this comment.
This infallible, so use .expect instead.
- recover(): accept high-S signatures for Charon parity. k256's recover_from_prehash self-verifies and rejects high-S, which Charon's decred-backed Recover accepts. Canonicalize to low-S (negate S, flip recovery-id parity) — recovers the same key without narrowing the acceptance domain. - recover(): use .expect for the infallible RecoveryId::from_byte now that recovery_byte is guaranteed to be 0 or 1. - Parametrize recover_recovery_id_domain with test_case and add a high-S recovery regression test. - frost round2: fix mangled bounds comment formatting. Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
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
3 participants
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.
Summary
Brings the BLS and FROST primitives into functional parity with Charon
(Go, tag
v1.7.1) and the ObolNetwork kryptology fork on a set ofedge/soundness cases. Implements the five tasks from the
03-crypto-bls-frost-parity-and-correctnessplan. All findings werere-verified against the Go source before implementing.
Changes
T01 —
k1util::recoverrecovery-id domain (crates/k1util)Charon's
Recoveronly accepts recovery bytes{0, 1, 27, 28}; the Rustport deferred to
RecoveryId::from_byte, which also accepts the x-reducedids
2and3(recovering a different key). Now rejects everythingoutside the Charon domain up front, and the error carries the original
byte.
verify_65inherits the fix.T02 — FROST round2 bcast bounds + self-broadcast (
crates/frost)Matches
ObolNetwork/kryptology@v0.1.0dkg_round2.go: the bcast upperbound is
max_signers(so a Charon-shaped broadcast map that includes thenode's own Round1Bcast is accepted), the shares upper bound stays
max_signers-1, and a self entry in the bcast map is skipped rather thanrejected with
InvalidParticipantId.T03 — FROST round2 zero-share guard (
crates/frost)Adds an explicit zero-valued Shamir share guard mirroring kryptology's
ShamirShare.Validate(id == 0/ zero scalar) before the Feldman VSSequation.
T04 — Go Herumi empty/single aggregate parity (
crates/crypto)aggregate(&[])now returns the serialized G2 identity signature (never anerror), and a single input is deserialized + re-serialized (canonicalized)
instead of returned verbatim, matching
tbls/herumi.go.T05 —
threshold_splitcontract, error mapping, misspelling (crates/crypto)Maps the
usize::try_fromfailure to a new dedicatedError::ThresholdOverflow,documents the retained
threshold > totalhardening (a divergence fromCharon's
threshold <= 1-only check), fixes the misspelledthreashlodlocal, and unifies
BlstSecretKey::from_byteserror mapping toError::InvalidSecretKeyacrosssecret_to_public_key,threshold_split_insecure, andsign.Testing
cargo test --workspace --all-features— all passcargo clippy --workspace --all-targets --all-features -- -D warnings— cleancargo +nightly fmt --all --check— cleanNew/updated tests: recovery-id domain table test; frost round2
self-broadcast acceptance, bcast/shares bound rejection, and zero-share /
zero-id rejection tests; empty-aggregate identity + Go-fixture parity,
single-input canonicalization, malformed-single-input rejection; threshold
variant assertions,
threshold == totalvalidity, and consistentinvalid-secret-key error across all four entry points.
🤖 Generated with Claude Code