Skip to content

Add two-group signing with group-tagged binding factors - #8

Open
keenan-lightspark wants to merge 1 commit into
mainfrom
sp2884-two-group-signing
Open

Add two-group signing with group-tagged binding factors#8
keenan-lightspark wants to merge 1 commit into
mainfrom
sp2884-two-group-signing

Conversation

@keenan-lightspark

Copy link
Copy Markdown

What

Adds frost_core::two_group: FROST signing where the second participant group (Spark: the user side) is itself a t-of-n FROST group with its own identifier space, per the settled F2 design (SP-2884).

  • Collections are keyed by (group, identifier); sub-user positions may numerically collide with operator identifiers and never alias.
  • Binding-factor preimage is domain-separated by a one-byte group tag (0x00 primary / 0x01 secondary), present both in each signer's preimage and in every commitment-list entry hashed by H5:
    rho_i = H1(tag(group(i)) || vk || H4(msg) || H5(commitment_list) || id(i))
    commitment_list = concat over signers sorted by (tag, id) of tag || id || hiding || binding
    
  • Lagrange coefficients stay within the signer's own group (same rule as the deployed participant-groups scheme); aggregate only sums shares.
  • Group commitment R, challenge, share computation/verification, and BIP-340 even-Y handling reuse the Ciphersuite hooks unchanged — even-Y parity is decided by the single R over both groups' commitments.
  • Aggregation takes verifying shares for the primary group only: primary shares get share-level blame (InvalidSignatureShare); a failure attributable to the secondary group (no verifying material on the wire by design) is reported as InvalidSignature without blame.
  • No changes to existing entry points — deployed single-party byte behavior is untouched; callers select this scheme explicitly by calling the new module.

Tests

frost-secp256k1-tr/tests/two_group_tests.rs:

  • (3-of-5)×(2-of-3) sign+aggregate verifies, iterated for both R parities; default identifier lists make cross-group identifier collisions the default case
  • colliding identifiers get distinct tagged binding factors (tag is the only preimage difference)
  • tampered primary share → blamed by identifier; tampered secondary share → InvalidSignature without blame
  • a secondary signer binding a divergent primary commitment set fails loudly at the aggregate check
  • taproot-tweaked flow under the deployed key convention (primary pre-normalized to the untweaked combined key's parity carrying the tweaked VK; secondary applies tweak() at signing so the taptweak scalar enters the sum exactly once)
  • the tagged scheme produces a different (still valid) signature than the flat participant-groups scheme over identical commitments and nonces

Note: serialization_tests::check_signing_package_postcard_serialization fails on main before this PR (stale upstream snapshot vs the fork's added SigningPackage fields); not addressed here.

Generalizes nested signing to a second participant group that is itself
a t-of-n FROST group with its own identifier space. Collections are
keyed by (group, identifier) and the binding-factor preimage is
domain-separated by a one-byte group tag (0x00/0x01), in both the
per-signer preimage and each commitment-list entry, so identifiers that
collide numerically across groups never alias. Group commitment,
challenge, share computation/verification, and even-Y handling reuse
the Ciphersuite hooks unchanged; existing entry points are untouched.

Aggregation carries verifying shares for the primary group only:
share-level blame is assigned for primary signers, while a failure that
lies with the secondary group (whose verifying shares are not
available) is reported as InvalidSignature without blame.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Jul 27, 2026

Copy link
Copy Markdown

Greptile Summary

Adds an explicit two-group FROST signing scheme with group-tagged binding factors.

  • Introduces independent primary and secondary identifier spaces and canonical tagged commitment encoding.
  • Adds two-group share generation, aggregation, primary-share blame, and secp256k1-tr parity/tweak handling.
  • Adds integration tests for identifier collisions, tampering, divergent commitment views, Taproot tweaking, and compatibility boundaries.

Confidence Score: 4/5

The PR appears safe to merge, with a non-blocking test-coverage gap around deterministic exercise of both aggregate-nonce parities.

The implementation consistently hashes and aggregates both tagged groups, while the only accepted concern is that one randomized test can pass without covering both parity branches it claims to exercise.

Files Needing Attention: frost-secp256k1-tr/tests/two_group_tests.rs

Important Files Changed

Filename Overview
frost-core/src/two_group.rs Implements tagged two-group packages, binding factors, per-group interpolation, signing, aggregation, and primary-share verification.
frost-secp256k1-tr/tests/two_group_tests.rs Adds broad two-group coverage, but its randomized loop does not guarantee the claimed coverage of both aggregate-nonce parities.
frost-core/src/lib.rs Exposes the new two_group module through the public frost-core API.

Sequence Diagram

sequenceDiagram
    participant P as Primary signers
    participant S as Secondary signers
    participant A as Aggregator
    P->>A: Tagged commitments (0x00, id)
    S->>A: Tagged commitments (0x01, id)
    A-->>P: TwoGroupSigningPackage
    A-->>S: TwoGroupSigningPackage
    P->>P: Compute tagged binding factor and primary-group Lagrange coefficient
    S->>S: Compute tagged binding factor and secondary-group Lagrange coefficient
    P->>A: Primary signature shares
    S->>A: Secondary signature shares
    A->>A: Sum all shares and verify combined signature
    alt Aggregate invalid
        A->>A: Verify primary shares for blame
    end
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
frost-secp256k1-tr/tests/two_group_tests.rs:121
**Parity coverage is nondeterministic**

The fixed random loop never records the aggregate commitment parity, so all eight iterations can exercise the same branch while the test still passes, leaving a parity-specific regression undetected.

Reviews (1): Last reviewed commit: "Add two-group signing with group-tagged ..." | Re-trigger Greptile

for _ in 0..8 {
let groups = make_groups((5, 3), (3, 2), None, None)?;
let message = b"two-group message";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Parity coverage is nondeterministic

The fixed random loop never records the aggregate commitment parity, so all eight iterations can exercise the same branch while the test still passes, leaving a parity-specific regression undetected.

Prompt To Fix With AI
This is a comment left during a code review.
Path: frost-secp256k1-tr/tests/two_group_tests.rs
Line: 121

Comment:
**Parity coverage is nondeterministic**

The fixed random loop never records the aggregate commitment parity, so all eight iterations can exercise the same branch while the test still passes, leaving a parity-specific regression undetected.

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant