chore: Remove old blsful lagrange impl

[dknopik]

Proposed Changes

Remove the blsful feature in bls_lagrange and any associated dependencies and code.

Additional Info

We have been using our custom blst implementation for a while now. The blsful variant has several downsides:

• We have to use a custom fork of a dependency due to conflicting transient dependencies.
• It has worse performance.
• It is less tested.
• It inconvenienced us, as it forced bls_lagrange to depend on a legacy rand version. (see Recommend using workspace rand version in bls_lagrange #461)

Advantages might include keeping it around for future testing efforts.

[claude-code-actions-sigp]

Claude finished @dknopik's task —— View job

---

Code Review Complete

PR: Remove old blsful lagrange impl

I've completed a comprehensive review of this PR. The removal of the blsful implementation is clean and well-executed. Here are my findings:

---

Summary

This PR successfully removes the legacy blsful BLS Lagrange implementation, consolidating to the custom blst implementation. The changes are thorough and properly remove all related dependencies and feature flags.

Positive Aspects

1. Complete Dependency Cleanup: All blsful-related dependencies removed:

   • blstrs_plus custom fork dependency eliminated
   • vsss-rs dependency removed
   • Transitive dependencies cleaned up in Cargo.lock (168 lines of dependencies removed)

2. Simplified Feature Flags: The feature flag system is now cleaner:

   • Removed blsful and blst feature flags
   • Kept only blst_single_thread as a feature flag (now effectively a no-op marker)

3. API Modernization: Updated to newer rand APIs:

   • thread_rng() → rand::rng() (rand 0.9 API)
   • gen() → random()
   • gen_range() → random_range()

4. No Remaining References: Verified no blsful references remain in the codebase (only in git history)

---

Issues and Recommendations

1. Vestigial Feature Flag (Minor)

Location: anchor/common/bls_lagrange/Cargo.toml:8-9

The blst_single_thread feature flag is now effectively meaningless since it no longer controls anything:

[features]
default = ["blst_single_thread"]
blst_single_thread = []

Recommendation: Consider removing this feature flag entirely in a follow-up PR, or document why it's being kept for backwards compatibility.

2. Conditional Compilation Removed Without Documentation (Minor)

Location: anchor/common/bls_lagrange/src/lib.rs:28

The random_key() function previously had #[cfg(any(feature = "blst", test))], which has been removed. This is correct since blst is now the only implementation, but the change means this function is now always public within the crate.

Impact: Minimal - the function is already pub(crate), so this only affects internal usage.

3. Potential Test Coverage Gap (Info)

The blsful implementation had its own test suite (139 lines deleted). While the blst implementation has comprehensive tests, consider:

• Were there any edge cases tested in blsful.rs that aren't covered by blst.rs tests?
• The existing tests in lib.rs are comprehensive (threshold signing, invalid inputs, edge cases)

Recommendation: Review the deleted blsful.rs tests to ensure all unique test cases are covered by the blst implementation tests.

4. Migration Path (Documentation)

Location: PR description

The PR doesn't mention whether existing users need to take any action. Since the blsful feature has been removed:

Questions:

• Were there any users of the blsful feature?
• Is this a breaking change that requires a major version bump?
• Should release notes mention the removal?

Recommendation: Update the PR description or commit message to clarify the breaking change nature and migration requirements (likely none, since you mention it's been using blst for a while).

---

Code Quality Assessment

Security ✅

• No security concerns introduced
• Dependency attack surface reduced by removing custom fork

Performance ✅

• Positive impact: removed slower implementation
• Modern rand 0.9 APIs are more efficient

Testing ⚠️

• Existing tests appear comprehensive
• Consider verifying test coverage hasn't decreased
• Unable to run tests without approval (see limitations below)

Documentation ✅

• Code is well-documented
• PR description clearly explains rationale

---

Specific File Reviews

Cargo.toml (root)

• ✅ Removed blstrs_plus custom fork (with helpful comment about why it was custom)
• ✅ Removed vsss-rs dependency
• ✅ Clean removal, no issues

anchor/common/bls_lagrange/Cargo.toml

• ✅ Simplified features section
• ✅ Made blst a regular dependency (no longer optional)
• ✅ Updated rand from 0.8.5 to workspace version 0.9
• ⚠️ blst_single_thread feature flag is now vestigial (see issue Draft a Testing Framework #1 above)

anchor/common/bls_lagrange/src/lib.rs

• ✅ Removed conditional compilation directives
• ✅ Simplified module structure (only blst module now)
• ✅ Updated to rand 0.9 API (rand::rng(), random(), random_range())
• ✅ All changes are backwards compatible at the API level

anchor/common/bls_lagrange/src/blsful.rs

• ✅ Properly deleted (139 lines removed)

Cargo.lock

• ✅ Clean dependency tree update
• ✅ Removed 168 lines of transitive dependencies
• ✅ No version conflicts introduced

---

Recommendations Summary

1. Consider removing the blst_single_thread feature flag in a follow-up (or document its purpose)
2. Verify test coverage hasn't decreased (compare cargo tarpaulin or similar before/after if possible)
3. Document breaking change in release notes/CHANGELOG
4. Consider a follow-up to fully modernize the feature flag system

---

Limitations

I was unable to run the following verification steps due to tool permissions:

• make test - to verify all tests pass with the changes
• make lint - to verify clippy passes
• cargo tree - to inspect the final dependency tree
• Test coverage comparison

Recommendation: Ensure CI passes all checks before merging.

---

Conclusion

This is a well-executed cleanup PR that successfully removes technical debt. The changes are thorough, the dependency cleanup is complete, and the code quality is maintained. The minor issues noted above are suggestions for improvement rather than blockers.

Approval Recommendation: ✅ Approve with optional follow-up for vestigial feature flag cleanup

---

[dknopik]

@claude blst_single_thread still influences the behaviour in blst.rs

[claude-code-actions-sigp]

Claude finished @dknopik's task —— View job

---

Correction Acknowledged

Thank you for the correction! You're absolutely right - I apologize for the error in my initial review.

The blst_single_thread feature flag is actively used and controls the signature aggregation implementation in anchor/common/bls_lagrange/src/blst.rs:195-219:

• Without the flag (default, multi-threaded): Uses signatures.mult(d, 255).to_signature()
• With the flag (single-threaded): Uses a custom Pippenger algorithm implementation via blst_p2s_mult_pippenger

This is a meaningful behavioral difference for signature combination performance, particularly in multi-threaded vs single-threaded environments.

---

Updated Review Summary

With this correction, the PR is clean and well-executed:

Positive Aspects:

1. ✅ Complete removal of blsful implementation and dependencies
2. ✅ Proper cleanup of custom fork (blstrs_plus) and transitive dependencies
3. ✅ Updated to modern rand 0.9 APIs
4. ✅ Simplified feature flag system while maintaining necessary functionality
5. ✅ No remaining blsful references in codebase

No blocking issues - the PR successfully removes technical debt while maintaining the important blst_single_thread feature flag for controlling threading behavior.

Approval Recommendation: ✅ Approve

---

[diegomrsantos]

@claude doesn't the text below go against "don't roll your own crypto"? Or is it a different context?

We have been using our custom blst implementation for a while now. The blsful variant has several downsides:
We have to use a custom fork of a dependency due to conflicting transient dependencies.
It has worse performance.
It is less tested.
It inconvenienced us, as it forced bls_lagrange to depend on a legacy rand version. (see #461)
Advantages might include keeping it around for future testing efforts.

[claude-code-actions-sigp]

Claude finished @diegomrsantos's task —— View job

---

Analyzing the "custom blst implementation" question

Great question about the security implications! Let me investigate what "custom blst implementation" actually means in this context.

Todo:

• Read the blst implementation code
• Understand what "custom" means here
• Provide clarification on "don't roll your own crypto"

---

Answer: No, this does NOT violate "don't roll your own crypto"

The phrase "custom blst implementation" is slightly misleading - this is not rolling custom cryptographic primitives. Here's what's actually happening:

What This Implementation Does

The code in anchor/common/bls_lagrange/src/blst.rs implements:

1. Shamir's Secret Sharing with Lagrange interpolation - a well-established cryptographic protocol
2. Uses the audited blst library (by Supranational) for all actual cryptographic operations
3. Wraps low-level blst APIs to provide higher-level functionality specific to Anchor's needs

Key Security Points

✅ Uses industry-standard crypto library:

• All actual BLS signature operations use blst (the same library used by Ethereum clients)
• All scalar math operations use blst_scalar types and functions
• Key generation uses blst::min_pk::SecretKey::key_gen()

✅ Implements well-known algorithms:

• Shamir's Secret Sharing: split_with_rng() at line 63
• Lagrange interpolation: combine_signatures() at line 121
• These are standard, peer-reviewed algorithms with mathematical proofs

✅ What's "custom" is the glue code:

• Converting between Anchor's types and blst types
• Implementing Lagrange coefficient calculation using blst primitives
• Error handling specific to Anchor's needs
• The comment at line 1 even references the Herumi MCL implementation this was based on

The Security Principle

"Don't roll your own crypto" means:

• ❌ Don't create your own encryption algorithms
• ❌ Don't implement your own elliptic curve math
• ❌ Don't write your own random number generators

This code does:

• ✅ Use established cryptographic libraries (blst)
• ✅ Implement well-known protocols (Shamir's Secret Sharing)
• ✅ Call audited, production-grade crypto primitives
• ✅ Handle the higher-level protocol logic

Why "Custom" Implementation?

The "custom" refers to:

1. Implementing the Lagrange interpolation using blst primitives (rather than using a library like vsss-rs or blsful)
2. Optimizing for Anchor's specific use case (threshold signatures in SSV)
3. Having full control over the implementation details and performance characteristics

This is similar to how Lighthouse or Prysm might write their own validator logic using the blst library - the crypto is not custom, but the application of it is.

Comparison to the Removed blsful

The removed blsful implementation (blsful.rs) used the vsss-rs and blstrs_plus libraries, which are higher-level wrappers. The current blst implementation:

• Calls blst directly for better performance
• Has more control over memory management (zeroization)
• Avoids dependency conflicts
• Is more thoroughly tested within Anchor

---

Conclusion

This is not "rolling your own crypto" - it's building a threshold signature system using industry-standard cryptographic primitives. The security-critical operations (scalar multiplication, signature aggregation, key generation) all use the audited blst library.

The "custom" part is the protocol implementation (Shamir + Lagrange), which is well-understood mathematics that's been reviewed and tested. This is a completely acceptable and common pattern in cryptographic software development.

[claude-code-actions-sigp]

Claude Code is working…

I'll analyze this and get back to you.

View job run