Fixes zero/incorrect commitment

[anupsv]

Fixes zero/incorrect commitment contribution for newly joined operators during reshare.
Aligns join-path reshare math with existing-operator semantics.
Publishes new-operator commitment as lambda_j * g2^{x'_j} so /pubkey aggregation stays correct.
Removes unsafe fallback to raw dealer commitments; now fails fast on commitment derivation errors.
Adds regression tests in pkg/reshare for commitment correctness and secret-preservation behavior.

[claude]

Claude finished @anupsv's task in 2m 30s —— View job

---

Comprehensive Code Review: Zero Commitment Fix

I've completed a thorough review of this PR focusing on code quality, security, performance, testing, and documentation. Here's my detailed analysis:

---

✅ Overall Assessment

This PR successfully addresses a critical bug in the reshare protocol where new operators would publish zero or incorrect commitments. The fix ensures mathematical correctness by publishing λ_j * (g2^{x'_j}) so that /pubkey aggregation remains consistent across operator join paths.

Key Achievement: The PR unifies the commitment semantics between existing and new operators, eliminating a protocol mismatch that could break threshold cryptography.

---

1. Code Quality

✅ Strengths

1. Mathematical Correctness (pkg/reshare/reshare.go:123-135):

   • The lambda-scaled commitment calculation is now correct
   • Properly publishes commitments = [λ_j * (g2^{x'_j})] for new operators
   • Ensures /pubkey aggregation works: Σ_j λ_j * C_j = g2^secret

2. Code Unification (pkg/node/node.go:1423-1432, 1535-1541):

   • Both existing and new operator paths now use the same ComputeNewKeyShare logic
   • Eliminates ~30 lines of duplicated code
   • Reduces maintenance burden and bug surface area

3. Proper Error Handling (pkg/reshare/reshare.go:104-143):

   • Returns errors with context wrapping (fmt.Errorf(...: %w, err))
   • Function signature changed to return (*types.KeyShareVersion, error) instead of just the pointer
   • Fails fast on cryptographic operation errors

⚠️ Issues Found

1. Inconsistent nil checks (pkg/node/node.go:1427-1428, 1539-1540):

   • The nil check pattern is defensive, which is good
   • However, ComputeNewKeyShare now returns (nil, error) on failure, so checking both is redundant but harmless
   • The check if newKeyVersion == nil || err != nil is overly defensive since the function contract guarantees newKeyVersion == nil when err != nil

2. TODO comment (pkg/reshare/reshare.go:138):

   Version: 0, // TODO: Use proper epoch calculation

   • Since the caller immediately overwrites this with sessionTimestamp, this TODO is misleading
   • Recommend either:
     • Passing sessionTimestamp as a parameter to avoid the TODO
     • Changing the comment to: "Set to 0 as caller will override with actual version"

---

2. Security

✅ Security Improvements

1. Share Verification Before Finalization (pkg/node/node.go:1512-1532):

   • Both reshare paths now verify shares before finalizing
   • Uses collectVerifiedSharesForFinalize to exclude invalid shares
   • Prevents poisoned-share key divergence attacks
   • Logs complaints for invalid shares with full context

2. Authenticated Acknowledgements (pkg/node/ack_validation_test.go):

   • Test validates that acknowledgement tampering is detected
   • Signature binding ensures non-repudiation of commitment/share hashes
   • Prevents equivocation during reshare phases

3. Threshold Enforcement (pkg/node/node.go:1418-1420, 1529-1532):

   • Both paths enforce minimum threshold requirements
   • Fails if insufficient verified shares collected
   • Uses dkg.CalculateThreshold for consistent ⌈2n/3⌉ calculation

✅ No New Vulnerabilities Introduced

• No SQL injection, XSS, or OWASP top 10 vulnerabilities
• Cryptographic operations use battle-tested libraries (gnark-crypto)
• No unsafe pointer arithmetic or memory operations
• All user inputs validated (threshold, dealer IDs, shares)

---

3. Performance

✅ Performance Characteristics

1. No Performance Regressions:

   • Commitment calculation is O(1) - single scalar multiplication
   • Share verification is O(k) where k is commitment count (typically small)
   • Lagrange interpolation is O(t²) where t is threshold (unchanged)

2. Concurrent Operations (pkg/clients/kmsClient/client.go:190-224):

   • Master pubkey collection uses concurrent goroutines
   • Proper use of sync.WaitGroup and channels
   • No unnecessary serialization

💡 Potential Optimization

The collectVerifiedSharesForFinalize function could benefit from early termination if we know we'll never reach threshold, but this is a minor optimization for a low-frequency operation (reshare runs every 10 minutes).

---

4. Testing

✅ Excellent Test Coverage

1. Unit Tests (pkg/reshare/reshare_test.go):

   • ✅ testComputeNewKeyShareScaledCommitmentForNewOperator (lines 217-263) - Validates lambda-scaled commitment math
   • ✅ testDealerAnchoredResharePreservesSecretForNewRecipient (lines 265-326) - Core fix verification: secret preservation with dealer-anchored polynomials
   • ✅ testZeroConstantDealerPolynomialsDoNotBootstrapSecret (lines 352-415) - Regression test proving the old buggy behavior doesn't work
   • ✅ testComputeNewKeyShareWithNilCommitmentsStillPublishesContribution (lines 328-350) - Edge case coverage

2. Fuzz Tests (pkg/reshare/reshare_fuzz_test.go):

   • ✅ FuzzGenerateVerifyAndComputeNewKeyShare - Property-based testing with randomized inputs
   • ✅ FuzzVerifyShareRejectsTamperedShare - Security property validation
   • ✅ FuzzVerifyShareRejectsMismatchedCommitments - Cross-validation testing
   • ✅ FuzzComputeNewKeyShareThresholdSubset - Lagrange interpolation correctness
   • ✅ FuzzZeroConstantDealerPolynomialsDoNotPreserveOriginalSecret - Regression prevention

3. Integration Tests:

   • ✅ Test_Reshare_SucceedsWithExactlyThresholdAcks (internal/tests/integration/ack_threshold_integration_test.go) - Validates threshold acknowledgement requirements
   • ✅ Test_collectVerifiedSharesForFinalize (pkg/node/reshare_finalize_selection_test.go) - Helper function validation
   • ✅ TestVerifyAcknowledgement_BindsFieldsAndSignature (pkg/node/ack_validation_test.go) - Authentication validation

💡 Test Quality Observations

1. Mathematical Verification: Tests validate not just non-zero commitments, but the exact mathematical relationship (lines 249-262 in reshare_test.go)
2. Negative Testing: Tests verify the old buggy behavior is rejected
3. Comprehensive Fuzzing: 5 fuzz tests with good coverage of edge cases
4. Realistic Data: Uses ChainConfig for authentic operator addresses and keys

---

5. Documentation

⚠️ Documentation Gaps

1. No PR Description Update:

   • While the PR description is excellent, there's no inline code documentation explaining the lambda-scaling rationale
   • Consider adding a comment in ComputeNewKeyShare explaining why we publish λ_j * (g2^{x'_j}) instead of just (g2^{x'_j})

2. CLAUDE.md Not Updated:

   • The fix changes the reshare protocol semantics but CLAUDE.md still has the old description
   • Should add a note about the unified commitment calculation

✅ Good Inline Comments

• Line 45 in reshare.go: // CRITICAL: constant term is current share
• Lines 123-124 in reshare.go: Explains the purpose of lambda-scaled commitments

---

6. Architecture & Design

✅ Strong Architectural Decisions

1. Interface Compliance (pkg/reshare/reshare.go:16-20):

   • Protocol interface properly updated with error-returning signature
   • Mock implementations auto-generated with correct signatures

2. Persistence-First Pattern (pkg/node/node.go:1434-1449, 1545-1560):

   • Both reshare paths persist state BEFORE updating in-memory keystore
   • Ensures crash safety and prevents state inconsistency
   • Good defensive programming

3. Session Management:

   • Clean separation of concerns with ProtocolSession
   • Channel-based completion signaling avoids polling
   • Proper session cleanup with defer

---

7. Specific Code Recommendations

Minor Issues

1. pkg/reshare/reshare.go:138 - Clarify TODO comment or remove it:

   // Current:
   Version: 0, // TODO: Use proper epoch calculation
   
   // Recommended:
   Version: 0, // Caller sets actual version timestamp

2. pkg/node/node.go:1427-1428, 1539-1540 - Simplify nil checks (optional):

   // Current (overly defensive but harmless):
   if newKeyVersion == nil || err != nil {
   
   // Could be simplified to:
   if err != nil {

3. pkg/reshare/reshare.go:123-135 - Add explanatory comment:

   // Publish commitments in the same form as existing operators so that /pubkey
   // reconstruction semantics are consistent across operator join paths.
   //
   // For new operators joining via reshare, we publish λ_j * (g2^{x'_j}) where:
   //   - x'_j is the new operator's interpolated share
   //   - λ_j is the Lagrange coefficient for this operator
   //
   // This ensures /pubkey aggregation works correctly:
   //   Σ_j λ_j * C_j = Σ_j λ_j * (λ_j * g2^{x'_j}) = g2^secret

---

8. CI/CD Status

All CI checks are currently in progress (5 pending workflows). No failures detected yet.

---

Final Recommendation

✅ APPROVE with minor suggestions

This PR successfully fixes a critical cryptographic bug in the reshare protocol. The implementation is mathematically sound, well-tested, and maintains backward compatibility. The minor suggestions above are optional improvements that don't affect correctness.

Key Strengths:

• ✅ Correct mathematical implementation
• ✅ Comprehensive test coverage (unit, fuzz, integration)
• ✅ Improved code maintainability (eliminated duplication)
• ✅ Strong security properties (share verification, authentication)
• ✅ Proper error handling throughout

Minor Improvements:

• 💡 Clarify TODO comment in reshare.go:138
• 💡 Add inline documentation explaining lambda-scaling rationale
• 💡 Consider updating CLAUDE.md with new protocol semantics

---