test: Add VerifySecure cross-compatibility test vectors for Rust implementation#111
test: Add VerifySecure cross-compatibility test vectors for Rust implementation#111PastaPastaPasta wants to merge 7 commits into
Conversation
WalkthroughUpdates CI Ubuntu runner to ubuntu-latest. Adds Rust test vectors for C++/Rust BLS cross-compatibility and secure aggregation verification. Introduces three C++ test/vector programs and wires two new test executables in CMake under BUILD_BLS_TESTS. One C++ file exposes CoreMPL’s AggregateSecure/VerifySecure via a public TestCoreMPL helper. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CPP as C++ Generators
participant RS as Rust Tests
participant Signers as Signers (SK→SIG)
participant Agg as Secure Aggregator
participant Ver as Verifier
CPP->>RS: Emit byte vectors (SK/PK/SIG/message,<br/>normal agg, secure agg)
RS->>Signers: Reconstruct SK/PK/SIG from bytes
Signers-->>RS: Individual verify results
RS->>Agg: Aggregate signatures securely with PKs
Agg-->>RS: Secure aggregated signature
RS->>Ver: verify_secure(agg_sig, PKs, message)
Ver-->>RS: Boolean result
note over RS,Ver: RS also checks that normal aggregation fails verify_secure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
src/test_full_compatibility.cpp (1)
41-60: Consider using deterministic key generation instead of hardcoded values.Using hardcoded private key bytes makes the test vectors less maintainable and potentially less secure. Consider generating keys deterministically from seeds like the other test programs.
Replace the hardcoded bytes with deterministic generation:
- // Create deterministic private keys - vector<uint8_t> sk1_bytes = { - 0x12, 0x34, 0x56, 0x78, 0x9a, 0xbc, 0xde, 0xf0, - // ... rest of hardcoded bytes - }; + // Create deterministic private keys from seeds + vector<uint8_t> seed1(32, 1); + vector<uint8_t> seed2(32, 2); + vector<uint8_t> seed3(32, 3); + + PrivateKey sk1 = BasicSchemeMPL().KeyGen(seed1); + PrivateKey sk2 = BasicSchemeMPL().KeyGen(seed2); + PrivateKey sk3 = BasicSchemeMPL().KeyGen(seed3); + + auto sk1_bytes = sk1.Serialize(); + auto sk2_bytes = sk2.Serialize(); + auto sk3_bytes = sk3.Serialize();src/test_verifysecure_vectors.cpp (1)
16-32: Consider consolidating output formatting functions.Two similar functions
printHexandprintRustArraycould be unified with a parameter to control the output format, improving maintainability.-void printHex(const string& label, const vector<uint8_t>& data) { - cout << label << " = ["; - for (size_t i = 0; i < data.size(); i++) { - if (i > 0) cout << ", "; - cout << "0x" << setfill('0') << setw(2) << hex << (int)data[i]; - } - cout << "];" << endl; -} - -void printRustArray(const string& label, const vector<uint8_t>& data) { - cout << label << " = ["; - for (size_t i = 0; i < data.size(); i++) { - if (i > 0) cout << ", "; - cout << (int)data[i] << "u8"; - } - cout << "];" << endl; -} +void printArray(const string& label, const vector<uint8_t>& data, bool hexFormat = true) { + cout << label << " = ["; + for (size_t i = 0; i < data.size(); i++) { + if (i > 0) cout << ", "; + if (hexFormat) { + cout << "0x" << setfill('0') << setw(2) << hex << (int)data[i]; + } else { + cout << (int)data[i] << "u8"; + } + } + cout << "];" << endl; +}src/test_cross_compatibility.cpp (1)
25-97: Consider generating more maintainable Rust test code.The current approach generates very verbose Rust code with repeated patterns. Consider using a more structured approach with helper functions or macros in the generated Rust code.
You could generate helper functions at the top of the Rust file to reduce repetition:
// Helper function for importing C++ keys fn import_cpp_keys(sk_bytes: &[&[u8; 32]], pk_bytes: &[&[u8; 48]], sig_bytes: &[&[u8; 96]]) -> (Vec<SecretKey<Bls12381G1Impl>>, Vec<PublicKey<Bls12381G1Impl>>, Vec<Signature<Bls12381G1Impl>>) { // Implementation }fresh_test_vectors.rs (3)
13-23: Consider extracting test vectors to separate files.The hardcoded byte arrays make the test functions very long and hard to read. Consider moving these vectors to separate constant files or using a macro to generate them.
Create a separate module for test vectors:
// test_vectors.rs pub const CPP_SK1_BYTES: [u8; 32] = [0x2a, 0x06, 0x16, /* ... */]; pub const CPP_PK1_BYTES: [u8; 48] = [0xb1, 0x45, 0xdf, /* ... */]; // ... other vectors // In test file: use crate::test_vectors::*;
61-63: Consolidate duplicate key imports.The same private key bytes are reused between test cases. Consider extracting common test vectors to reduce duplication.
// Extract common test vectors const COMMON_SK1_BYTES: [u8; 32] = [0x2a, 0x06, 0x16, /* ... */]; const COMMON_SK2_BYTES: [u8; 32] = [0x13, 0xc6, 0xe9, /* ... */]; // Use in both test functions let sk1 = SecretKey::<Bls12381G1Impl>::try_from(COMMON_SK1_BYTES.as_slice()).unwrap();
116-116: Improve error handling in signature creation.The
try_fromcall can fail and should provide more descriptive error messages for debugging.- let normal_sig = Signature::<Bls12381G1Impl>::try_from(normal_agg_bytes.as_slice()).unwrap(); + let normal_sig = Signature::<Bls12381G1Impl>::try_from(normal_agg_bytes.as_slice()) + .expect("Failed to deserialize normal aggregated signature from C++ bytes");cross_compatibility_test_vectors.rs (2)
44-48: Simplify aggregation result handling.The pattern matching with panic could be simplified and made more robust.
Consider using a more direct approach:
- let secure_agg = AggregateSignature::from_signatures_secure(&signatures, &public_keys).unwrap(); - let final_sig = match secure_agg { - AggregateSignature::Basic(sig) => Signature::Basic(sig), - _ => panic!("Expected Basic scheme"), - }; + let secure_agg = AggregateSignature::from_signatures_secure(&signatures, &public_keys) + .expect("Failed to create secure aggregation"); + let final_sig = match secure_agg { + AggregateSignature::Basic(sig) => Signature::Basic(sig), + _ => panic!("Expected Basic scheme from secure aggregation"), + };
25-25: Document the test message format.The message
[0x68, 0x65, 0x6c, 0x6c, 0x6f]is used across all tests but its meaning isn't immediately clear.Consider adding a comment to clarify:
- let message = [0x68, 0x65, 0x6c, 0x6c, 0x6f]; + let message = [0x68, 0x65, 0x6c, 0x6c, 0x6f]; // "hello" in ASCIIOr extract it as a constant:
const TEST_MESSAGE: &[u8] = b"hello";Also applies to: 76-76, 124-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cross_compatibility_test_vectors.rs(1 hunks)fresh_test_vectors.rs(1 hunks)src/CMakeLists.txt(1 hunks)src/test_cross_compatibility.cpp(1 hunks)src/test_full_compatibility.cpp(1 hunks)src/test_verifysecure_vectors.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: macos-latest, cmake, clang, easy backend
- GitHub Check: macos-latest, autotools, gcc, easy backend
- GitHub Check: ubuntu-latest, Python 3.11, Go 1.22
- GitHub Check: ubuntu-latest, Python 3.10, Go 1.22
- GitHub Check: ubuntu-latest, Python 3.12, Go 1.22
- GitHub Check: macos-latest, Python 3.11, Go 1.22
- GitHub Check: ubuntu-latest, Python 3.9, Go 1.22
- GitHub Check: macos-latest, Python 3.9, Go 1.22
- GitHub Check: macos-latest, Python 3.10, Go 1.22
- GitHub Check: js_bindings
🔇 Additional comments (2)
src/test_cross_compatibility.cpp (1)
116-129: Add error handling for cryptographic operations.Key generation and signing operations can fail and should be handled gracefully.
- PrivateKey sk1 = BasicSchemeMPL().KeyGen(seed1); - PrivateKey sk2 = BasicSchemeMPL().KeyGen(seed2); + PrivateKey sk1, sk2; + try { + sk1 = BasicSchemeMPL().KeyGen(seed1); + sk2 = BasicSchemeMPL().KeyGen(seed2); + } catch (const std::exception& e) { + cerr << "Failed to generate keys: " << e.what() << endl; + return 1; + } sks2.push_back(sk1); sks2.push_back(sk2); pks2.push_back(sk1.GetG1Element()); pks2.push_back(sk2.GetG1Element()); vector<uint8_t> message = {0x68, 0x65, 0x6c, 0x6c, 0x6f}; // "hello" // Sign with each key - sigs2.push_back(BasicSchemeMPL().Sign(sk1, message)); - sigs2.push_back(BasicSchemeMPL().Sign(sk2, message)); + try { + sigs2.push_back(BasicSchemeMPL().Sign(sk1, message)); + sigs2.push_back(BasicSchemeMPL().Sign(sk2, message)); + } catch (const std::exception& e) { + cerr << "Failed to sign message: " << e.what() << endl; + return 1; + }Likely an incorrect or invalid review comment.
fresh_test_vectors.rs (1)
28-33: Confirm BLS group assignments for key and signature typesI wasn’t able to locate the definitions of
Bls12381G1ImplorBls12381G2Implin the codebase, so please verify that:
Bls12381G1Implindeed represents G1 elements for public keys and that signatures under this type are handled in G2 (per the standard scheme)- If signatures are actually in G2, consider switching to a dedicated
Bls12381G2Implwhere appropriate to avoid confusionPoints to review:
- fresh_test_vectors.rs, lines 28–33 (all uses of
Signature::<Bls12381G1Impl>)- cross_compatibility_test_vectors.rs, similar locations
| // Helper class to access protected methods | ||
| class TestCoreMPL : public CoreMPL { | ||
| public: | ||
| TestCoreMPL() : CoreMPL("BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_NUL_") {} | ||
|
|
||
| using CoreMPL::AggregateSecure; | ||
| using CoreMPL::VerifySecure; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid exposing protected methods through inheritance.
The helper class breaks encapsulation by exposing protected methods. Consider using a friend class or refactoring the API to provide public access to these methods when needed for testing.
-// Helper class to access protected methods
-class TestCoreMPL : public CoreMPL {
-public:
- TestCoreMPL() : CoreMPL("BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_NUL_") {}
-
- using CoreMPL::AggregateSecure;
- using CoreMPL::VerifySecure;
-};
+// Consider adding public test methods to CoreMPL or use a proper test API🤖 Prompt for AI Agents
In src/test_full_compatibility.cpp around lines 25 to 32, the TestCoreMPL class
exposes protected methods AggregateSecure and VerifySecure by making them
public, which breaks encapsulation. To fix this, remove the 'using' declarations
that expose these methods and instead declare the test class as a friend inside
CoreMPL or refactor CoreMPL to provide public accessors or test hooks for these
methods, ensuring encapsulation is maintained while allowing necessary testing
access.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
cross_compatibility_test_vectors.rs (4)
13-22: Code duplication still present - extract test vectors into constants.The hardcoded byte arrays for keys and signatures are duplicated across multiple test functions, as noted in previous reviews. This creates maintenance overhead and potential for inconsistencies.
28-33: Error handling still uses.unwrap()- improve debugging capability.The conversion operations still use
.unwrap()which will cause panics on failure and make debugging difficult, as previously identified.
61-73: Common test setup duplication persists.The test setup code is still duplicated across test functions, as noted in previous reviews.
119-120: Duplicate public key definitions remain.The same public key byte arrays are redefined here, as identified in previous reviews.
🧹 Nitpick comments (2)
cross_compatibility_test_vectors.rs (2)
45-48: Replace panic with proper error handling.The pattern matching uses
panic!("Expected Basic scheme")which could be improved with more graceful error handling.Consider using
expect()with a more descriptive message or proper error handling:- let final_sig = match secure_agg { - AggregateSignature::Basic(sig) => Signature::Basic(sig), - _ => panic!("Expected Basic scheme"), - }; + let final_sig = match secure_agg { + AggregateSignature::Basic(sig) => Signature::Basic(sig), + _ => panic!("Secure aggregation produced unexpected scheme type"), + };
100-103: Consistent pattern matching issue.The same panic pattern is repeated here as in the two-signer test.
Apply the same error handling improvement as suggested for the first test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cross_compatibility_test_vectors.rs(1 hunks)src/test_cross_compatibility.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test_cross_compatibility.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: ubuntu-latest, Python 3.9, Go 1.22
- GitHub Check: ubuntu-latest, Python 3.11, Go 1.22
- GitHub Check: ubuntu-latest, Python 3.12, Go 1.22
- GitHub Check: ubuntu-latest, Python 3.10, Go 1.22
- GitHub Check: macos-latest, autotools, gcc, easy backend
- GitHub Check: macos-latest, autotools, clang, gmp backend
- GitHub Check: macos-latest, autotools, gcc, gmp backend
- GitHub Check: macos-latest, cmake, clang, gmp backend
- GitHub Check: js_bindings
🔇 Additional comments (1)
cross_compatibility_test_vectors.rs (1)
112-128: Test logic correctly validates security boundary.The test properly verifies that normal aggregation fails secure verification, which is the expected behavior for ensuring security guarantees.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
PastaPastaPasta
changed the title
|
pls change base branch and rebase on develop |
Provides comprehensive test vectors for validating VerifySecure implementation compatibility between C++ and Rust BLS signature libraries. - Add cross-compatibility test programs - Generate test vectors for Rust implementation validation - Include 2-signer and 3-signer test scenarios - Validate secure aggregation prevents rogue key attacks 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove duplicate fresh_test_vectors.rs file - Replace wildcard imports with explicit imports in Rust test files - Add named constants for seed values in C++ generator - Fix scheme mismatch: ensure consistent Bls12381G1Impl usage between C++ and Rust - Update signature parsing to use proper Signature::try_from method These changes address code review feedback and ensure proper cross-platform compatibility between C++ and Rust BLS signature implementations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Move test_verifysecure_vectors and test_cross_compatibility executables inside if(BUILD_BLS_TESTS) block - Ensures test executables are only built when tests are enabled - Follows same pattern as other test targets in the project 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Wrap PrivateKey::FromBytes calls in try-catch block - Handle potential exceptions during key creation from raw bytes - Return error code 1 with descriptive error message on failure - Ensures graceful failure handling for invalid key material 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Wrap BasicSchemeMPL().KeyGen(seed) calls in try-catch block - Handle potential exceptions during key generation from seeds - Return error code 1 with descriptive error message on failure - Includes seed number in error message for better debugging 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Create helper function create_test_vectors() to centralize test data - Move all hardcoded byte arrays for keys/signatures to helper function - Add count parameter to return appropriate number of test vectors - Update all test functions to use helper instead of duplicating setup code - Significantly reduce code duplication and improve maintainability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace .unwrap() calls with .expect() and descriptive error messages - Add signer number context to each conversion failure message - Improves debugging by clearly indicating which conversion and signer failed - Maintains panic behavior but with much better error information 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
PastaPastaPasta
force-pushed
the
feat/verify-secure-test-vectors
branch
from
b62ec60 to
fb4741c
Compare
|
@UdjinM6 done |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-test.yaml (1)
41-41: Ubuntu autotools deps missing; autotools build likely to fail.autogen.sh typically needs autoconf, automake, libtool, pkg-config. Install them on Ubuntu.
- sudo apt-get install -qq --yes valgrind libgmp-dev cmake + sudo apt-get install -qq --yes valgrind libgmp-dev cmake autoconf automake libtool pkg-config
♻️ Duplicate comments (1)
src/test_full_compatibility.cpp (1)
25-32: Avoid exposing protected CoreMPL methods via public inheritance.Use a friend declaration or a dedicated test-only wrapper in the library gated by BUILD_BLS_TESTS to keep encapsulation intact.
🧹 Nitpick comments (18)
cross_compatibility_test_vectors.rs (7)
32-38: Avoid eager String allocation in expect; use lazy panic for clearer errors.format! allocates even on success. Prefer unwrap_or_else with panic! so the message is built only on error.
- let sk = SecretKey::<Bls12381G1Impl>::try_from(sk_bytes[i].as_slice()) - .expect(&format!("Failed to create SecretKey from bytes for signer {}", i + 1)); - let pk = PublicKey::<Bls12381G1Impl>::try_from(pk_bytes[i].as_slice()) - .expect(&format!("Failed to create PublicKey from bytes for signer {}", i + 1)); - let sig = Signature::<Bls12381G1Impl>::try_from(sig_bytes[i].as_slice()) - .expect(&format!("Failed to create Signature from bytes for signer {}", i + 1)); + let sk = SecretKey::<Bls12381G1Impl>::try_from(sk_bytes[i].as_slice()) + .unwrap_or_else(|e| panic!("Failed to create SecretKey from bytes for signer {}: {e}", i + 1)); + let pk = PublicKey::<Bls12381G1Impl>::try_from(pk_bytes[i].as_slice()) + .unwrap_or_else(|e| panic!("Failed to create PublicKey from bytes for signer {}: {e}", i + 1)); + let sig = Signature::<Bls12381G1Impl>::try_from(sig_bytes[i].as_slice()) + .unwrap_or_else(|e| panic!("Failed to create Signature from bytes for signer {}: {e}", i + 1));
53-53: Silence unused variable warnings for secret_keys.Prefix with underscore to keep intent while avoiding warnings in strict CI.
- let (secret_keys, public_keys, signatures) = create_test_vectors(2); + let (_secret_keys, public_keys, signatures) = create_test_vectors(2);- let (secret_keys, public_keys, signatures) = create_test_vectors(3); + let (_secret_keys, public_keys, signatures) = create_test_vectors(3);Also applies to: 78-78
56-56: Use byte string literal for message.b"hello" is clearer and avoids hex noise.
- let message = [0x68, 0x65, 0x6c, 0x6c, 0x6f]; + let message = b"hello";Also applies to: 81-81, 111-111
63-63: Replace unwrap with expect for aggregation.Gives actionable failure context.
- let secure_agg = AggregateSignature::from_signatures_secure(&signatures, &public_keys).unwrap(); + let secure_agg = AggregateSignature::from_signatures_secure(&signatures, &public_keys) + .expect("secure aggregation failed");Also applies to: 89-89
64-67: Simplify variant handling with if-let.More concise without changing behavior.
- let final_sig = match secure_agg { - AggregateSignature::Basic(sig) => Signature::Basic(sig), - _ => panic!("Expected Basic scheme"), - }; + let final_sig = if let AggregateSignature::Basic(sig) = secure_agg { + Signature::Basic(sig) + } else { + panic!("Expected Basic scheme"); + };Also applies to: 90-93
101-107: Use expect when deserializing the normal aggregate.Improves diagnosability if bytes are malformed.
- let normal_sig = Signature::<Bls12381G1Impl>::try_from(normal_agg_bytes.as_slice()).unwrap(); + let normal_sig = Signature::<Bls12381G1Impl>::try_from(normal_agg_bytes.as_slice()) + .expect("failed to deserialize normal aggregated signature");
6-24: Hoist static vectors to module-level consts to avoid per-call allocation.Minor perf/readability win; the data is immutable.
Example:
const SK_BYTES: [[u8; 32]; 3] = [ /* ... */ ]; const PK_BYTES: [[u8; 48]; 3] = [ /* ... */ ]; const SIG_BYTES: [[u8; 96]; 3] = [ /* ... */ ]; fn create_test_vectors(count: usize) -> (...) { assert!(count <= 3, "..."); for i in 0..count { let sk = SecretKey::<Bls12381G1Impl>::try_from(&SK_BYTES[i][..])?; // ... } // ... }.github/workflows/build-test.yaml (3)
35-35: Bump checkout action to v4.Keeps up with GH Actions deprecations and perf improvements.
- uses: actions/checkout@v3 + uses: actions/checkout@v4
48-48: Remove stray ls -l.Debug leftover; speeds up logs a bit.
- ls -l
67-72: Optionally run the new C++ test-vector executables in CI.Lightweight smoke checks ensure generators stay working (e.g., only on Ubuntu+CMake).
Example step:
- name: Run vector generators (Ubuntu+CMake only) if: startsWith(matrix.os, 'ubuntu') && startsWith(matrix.builder, 'cmake') run: | ./test_verifysecure_vectors --help >/dev/null 2>&1 || true ./test_cross_compatibility --help >/dev/null 2>&1 || truesrc/CMakeLists.txt (1)
56-64: Optional: integrate with CTest so they can be invoked via ctest when desired.Helps local/CI runners that use ctest.
if(BUILD_BLS_TESTS) add_executable(test_verifysecure_vectors test_verifysecure_vectors.cpp) target_link_libraries(test_verifysecure_vectors PRIVATE dashbls) add_test(NAME verifysecure_vectors COMMAND test_verifysecure_vectors) add_executable(test_cross_compatibility test_cross_compatibility.cpp) target_link_libraries(test_cross_compatibility PRIVATE dashbls) add_test(NAME cross_compatibility COMMAND test_cross_compatibility) endif()src/test_cross_compatibility.cpp (3)
30-35: Make emitting private keys optional (default on to preserve current output).Helps avoid accidentally exporting SKs in CI artifacts or logs.
-void printRustTest(const string& test_name, - const vector<PrivateKey>& sks, - const vector<G1Element>& pks, - const vector<G2Element>& sigs, - const vector<uint8_t>& message) { +void printRustTest(const string& test_name, + const vector<PrivateKey>& sks, + const vector<G1Element>& pks, + const vector<G2Element>& sigs, + const vector<uint8_t>& message, + bool emit_private_keys = true) { @@ - // Export private keys - cout << " // Private keys from C++" << endl; - for (size_t i = 0; i < sks.size(); i++) { - auto skBytes = sks[i].Serialize(); - printHexBytes(" let cpp_sk" + to_string(i+1) + "_bytes", skBytes); - } + // Export private keys + if (emit_private_keys) { + cout << " // Private keys from C++" << endl; + for (size_t i = 0; i < sks.size(); i++) { + auto skBytes = sks[i].Serialize(); + printHexBytes(" let cpp_sk" + to_string(i+1) + "_bytes", skBytes); + } + }If you prefer to keep SK emission disabled by default, flip the default to false and pass true at both call sites.
Also applies to: 41-45
117-135: Avoid repeated temporary scheme construction.Create one BasicSchemeMPL and reuse it for KeyGen/Sign.
@@ - vector<uint8_t> seed1(32, SEED1_VALUE); - vector<uint8_t> seed2(32, SEED2_VALUE); + vector<uint8_t> seed1(32, SEED1_VALUE); + vector<uint8_t> seed2(32, SEED2_VALUE); + BasicSchemeMPL scheme; @@ - PrivateKey sk1 = BasicSchemeMPL().KeyGen(seed1); - PrivateKey sk2 = BasicSchemeMPL().KeyGen(seed2); + PrivateKey sk1 = scheme.KeyGen(seed1); + PrivateKey sk2 = scheme.KeyGen(seed2); @@ - sigs2.push_back(BasicSchemeMPL().Sign(sk1, message)); - sigs2.push_back(BasicSchemeMPL().Sign(sk2, message)); + sigs2.push_back(scheme.Sign(sk1, message)); + sigs2.push_back(scheme.Sign(sk2, message));
145-158: Apply the same scheme reuse in the three-signer block.@@ - PrivateKey sk3 = BasicSchemeMPL().KeyGen(seed3); + PrivateKey sk3 = scheme.KeyGen(seed3); @@ - sigs3.push_back(BasicSchemeMPL().Sign(sk1, message)); - sigs3.push_back(BasicSchemeMPL().Sign(sk2, message)); - sigs3.push_back(BasicSchemeMPL().Sign(sk3, message)); + sigs3.push_back(scheme.Sign(sk1, message)); + sigs3.push_back(scheme.Sign(sk2, message)); + sigs3.push_back(scheme.Sign(sk3, message));src/test_verifysecure_vectors.cpp (1)
96-101: Also emit reversed public keys as arrays for completeness.Current output only comments the mapping; emit actual bytes to simplify Rust-side reconstruction.
- cout << "// Public keys in reversed order:" << endl; - for (size_t i = 0; i < pksReversed.size(); i++) { - auto pkBytes = pksReversed[i].Serialize(); - cout << "// pk_reversed[" << i << "] = pk" << (3-i) << endl; - } + cout << "// Public keys in reversed order:" << endl; + for (size_t i = 0; i < pksReversed.size(); i++) { + auto pkBytes = pksReversed[i].Serialize(); + cout << "// pk_reversed[" << i << "] = pk" << (3 - i) << endl; + printHex("let pk_reversed" + to_string(i + 1) + "_bytes", pkBytes); + }src/test_full_compatibility.cpp (3)
16-23: Preferletin generated Rust function scope to avoid unused-const warnings.Switching to
letkeeps types explicit and reduces noise in Rust tests.-void printHexArray(const string& name, const vector<uint8_t>& data) { - cout << "const " << name << ": [u8; " << data.size() << "] = ["; +void printHexArray(const string& name, const vector<uint8_t>& data) { + cout << "let " << name << ": [u8; " << data.size() << "] = [";
89-91: Remove unused Rust import.
use subtle::CtOption;isn’t used in the generated snippet.- cout << " use subtle::CtOption;" << endl; - cout << endl; + cout << endl;
128-131: Emit assertions instead of constants in generated Rust.Make the snippet self-validating.
- cout << endl << " // Verification results" << endl; - cout << " const NORMAL_VERIFY_RESULT: bool = " << (normalVerify ? "true" : "false") << ";" << endl; - cout << " const SECURE_VERIFY_RESULT: bool = " << (secureVerify ? "true" : "false") << ";" << endl; + cout << endl << " // Verification results" << endl; + cout << " assert!(" << (normalVerify ? "true" : "false") << ", \"normal aggregate verify failed\");" << endl; + cout << " assert!(" << (secureVerify ? "true" : "false") << ", \"secure aggregate verify failed\");" << endl;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/build-test.yaml(1 hunks)cross_compatibility_test_vectors.rs(1 hunks)src/CMakeLists.txt(1 hunks)src/test_cross_compatibility.cpp(1 hunks)src/test_full_compatibility.cpp(1 hunks)src/test_verifysecure_vectors.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/test_full_compatibility.cpp (1)
rust-bindings/bls-signatures/src/schemes.rs (1)
sigs(64-64)
🔇 Additional comments (5)
.github/workflows/build-test.yaml (1)
25-25: Confirm ubuntu-latest compatibility (toolchain bump to 24.04).GCC/Clang and glibc versions change on 24.04; verify no ABI/tooling regressions vs 20.04/22.04. Pin to ubuntu-22.04 if reproducibility is required.
src/CMakeLists.txt (1)
56-64: LGTM: tests gated behind BUILD_BLS_TESTS and linked properly.Guarding these executables avoids overhead in non-test builds.
src/test_cross_compatibility.cpp (1)
92-101: Confirm Rust crate exposes required secure aggregation APIs
I couldn’t locate definitions forverify_secure,from_signatures_secure, or anyTryFrom<…>impls forSecretKey<Bls12381G1Impl>,PublicKey<…>, orSignature<…>—please manually verify these functions and impls exist in your Rust crate and match the C++ usage.src/test_verifysecure_vectors.cpp (1)
47-58: KeyGen error handling: looks good.src/test_full_compatibility.cpp (1)
119-127: AggregateSecure/VerifySecure flags and DST are correct
TestCoreMPL invokes CoreMPL with DST “BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_NUL_” (test_full_compatibility.cpp:28) and calls AggregateSecure/VerifySecure with fLegacy=false, matching BasicSchemeMPL::CIPHERSUITE_ID (“BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_NUL_”) in schemes.cpp:78. Rust’s BasicScheme wrapper likewise uses the same default scheme and flag. No changes needed.
| void printHexBytes(const string& label, const vector<uint8_t>& data) { | ||
| cout << label << " = ["; | ||
| for (size_t i = 0; i < data.size(); i++) { | ||
| if (i > 0) cout << ", "; | ||
| cout << "0x" << setfill('0') << setw(2) << hex << (int)data[i]; | ||
| } | ||
| cout << "];" << endl; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent iostream state leakage from hex formatting.
Without restoring flags/fill, later numeric inserts may print in hex unexpectedly. Localize formatting.
void printHexBytes(const string& label, const vector<uint8_t>& data) {
- cout << label << " = [";
+ auto f = cout.flags();
+ auto old_fill = cout.fill();
+ cout << label << " = [";
for (size_t i = 0; i < data.size(); i++) {
if (i > 0) cout << ", ";
cout << "0x" << setfill('0') << setw(2) << hex << (int)data[i];
}
- cout << "];" << endl;
+ cout << "];" << endl;
+ cout.flags(f);
+ cout.fill(old_fill);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void printHexBytes(const string& label, const vector<uint8_t>& data) { | |
| cout << label << " = ["; | |
| for (size_t i = 0; i < data.size(); i++) { | |
| if (i > 0) cout << ", "; | |
| cout << "0x" << setfill('0') << setw(2) << hex << (int)data[i]; | |
| } | |
| cout << "];" << endl; | |
| } | |
| void printHexBytes(const string& label, const vector<uint8_t>& data) { | |
| // Save and later restore cout’s format flags and fill character | |
| auto f = cout.flags(); | |
| auto old_fill = cout.fill(); | |
| cout << label << " = ["; | |
| for (size_t i = 0; i < data.size(); i++) { | |
| if (i > 0) cout << ", "; | |
| cout << "0x" | |
| << setfill('0') << setw(2) << hex | |
| << (int)data[i]; | |
| } | |
| cout << "];" << endl; | |
| // Restore the original formatting state | |
| cout.flags(f); | |
| cout.fill(old_fill); | |
| } |
🤖 Prompt for AI Agents
In src/test_cross_compatibility.cpp around lines 21 to 28, the function
printHexBytes modifies iostream formatting (hex, fill, width) and doesn't
restore the stream state, which can cause later numeric output to appear in hex;
save the stream's fmtflags and the fill character (e.g. auto oldFlags =
cout.flags(); char oldFill = cout.fill();), perform the hex/width/fill
operations, then restore cout.flags(oldFlags) and cout.fill(oldFill) after
printing (or alternatively use a local ostringstream to format the bytes and
output its string), ensuring no iostream state leaks.
| void printHex(const string& label, const vector<uint8_t>& data) { | ||
| cout << label << " = ["; | ||
| for (size_t i = 0; i < data.size(); i++) { | ||
| if (i > 0) cout << ", "; | ||
| cout << "0x" << setfill('0') << setw(2) << hex << (int)data[i]; | ||
| } | ||
| cout << "];" << endl; | ||
| } |
There was a problem hiding this comment.
Hex manipulator leaks into subsequent prints, corrupting Rust byte arrays.
After printHex sets hex, printRustArray may emit hex digits as decimal text (e.g., 0x74 -> "74u8"). Restore flags/fill to keep message and later arrays correct.
void printHex(const string& label, const vector<uint8_t>& data) {
- cout << label << " = [";
+ auto f = cout.flags();
+ auto old_fill = cout.fill();
+ cout << label << " = [";
for (size_t i = 0; i < data.size(); i++) {
if (i > 0) cout << ", ";
cout << "0x" << setfill('0') << setw(2) << hex << (int)data[i];
}
- cout << "];" << endl;
+ cout << "];" << endl;
+ cout.flags(f);
+ cout.fill(old_fill);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void printHex(const string& label, const vector<uint8_t>& data) { | |
| cout << label << " = ["; | |
| for (size_t i = 0; i < data.size(); i++) { | |
| if (i > 0) cout << ", "; | |
| cout << "0x" << setfill('0') << setw(2) << hex << (int)data[i]; | |
| } | |
| cout << "];" << endl; | |
| } | |
| void printHex(const string& label, const vector<uint8_t>& data) { | |
| auto f = cout.flags(); | |
| auto old_fill = cout.fill(); | |
| cout << label << " = ["; | |
| for (size_t i = 0; i < data.size(); i++) { | |
| if (i > 0) cout << ", "; | |
| cout << "0x" << setfill('0') << setw(2) << hex << (int)data[i]; | |
| } | |
| cout << "];" << endl; | |
| cout.flags(f); | |
| cout.fill(old_fill); | |
| } |
🤖 Prompt for AI Agents
In src/test_verifysecure_vectors.cpp around lines 16 to 23, printHex sets stream
manipulators (hex, setfill) but does not restore cout's flags or fill, causing
subsequent prints (like printRustArray) to output in hex or with wrong fill;
capture cout's current fmtflags and fill char at start, apply manipulators, then
restore the saved flags and fill before returning so later prints are
unaffected.
| @@ -0,0 +1,115 @@ | |||
| // Cross-Compatibility Test Vectors | |||
There was a problem hiding this comment.
shouldn't this file be somewhere in rust-bindings? kind of weird to have it in the root
| cout << "}" << endl; | ||
|
|
||
| return 0; | ||
| } No newline at end of file |
There was a problem hiding this comment.
nit: missing nl at eof (same for other cpp files)
| # Cross-compatibility test generator for true C++/Rust interoperability | ||
| add_executable(test_cross_compatibility test_cross_compatibility.cpp) | ||
| target_link_libraries(test_cross_compatibility PRIVATE dashbls) | ||
| endif() |
There was a problem hiding this comment.
what about test_full_compatibility.cpp? also, would probably make sense to sort them alphabetically here.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR claims to add VerifySecure cross-compatibility test vectors, but the implementation has critical integration gaps. The only file that actually calls VerifySecure (test_full_compatibility.cpp) is dead code — not registered in CMakeLists.txt. The Rust test file uses a third-party crate (blsful) unrelated to the repo's own rust-bindings and has no Cargo.toml, making it uncompilable. The file named test_verifysecure_vectors.cpp never calls VerifySecure at all.
Reviewed commit: fb4741c
🔴 3 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(s)
🤖 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 `src/CMakeLists.txt`:
- [BLOCKING] lines 56-64: test_full_compatibility.cpp is not registered as a build target — dead code
test_full_compatibility.cpp is the only file that actually exercises `CoreMPL::AggregateSecure` and `CoreMPL::VerifySecure`, making it the most valuable test in this PR. But it has no `add_executable()` entry in CMakeLists.txt, so it will never be compiled, and any errors in it will go undetected. This completely undermines the stated goal of verifying VerifySecure compatibility.
- [SUGGESTION] lines 56-64: New executables are not part of any CI test execution path
The new executables are standalone generators that print to stdout. They are not Catch2 tests, not registered with `add_test()`, and CI does not invoke them. If the generated vectors drift from what the Rust side expects, no CI check will catch it. Consider either: (a) adding `add_test()` commands so CTest runs them, or (b) incorporating the vector generation and verification into the existing `test.cpp` Catch2 suite.
In `cross_compatibility_test_vectors.rs`:
- [BLOCKING] lines 1-115: Rust test file cannot be compiled — no Cargo.toml, wrong crate dependency
This file sits at the repo root with no Cargo.toml, no module declaration, and no test harness. It imports `blsful` (a third-party pure-Rust BLS12-381 crate), but the repo's actual Rust bindings live under `rust-bindings/` and use `bls-dash-sys` (FFI to this C++ library). The cross-compatibility claim is unverifiable because this file can never be compiled or executed. Either integrate it into rust-bindings/ using the repo's own crate, or add a dedicated Cargo.toml with blsful as a dependency.
- [SUGGESTION] lines 62-70: Rust tests don't verify C++ secure aggregation output — only Rust-side aggregation
The Rust tests import C++ individual signatures and public keys, then perform secure aggregation entirely in Rust via `AggregateSignature::from_signatures_secure()` and verify with `verify_secure()`. This proves Rust can securely aggregate C++ individual signatures, but does NOT prove that C++ `AggregateSecure` produces byte-identical output to Rust's secure aggregation. A true cross-compatibility test should import the C++ secure aggregation result (from test_full_compatibility.cpp) and compare it to the Rust-computed result.
- [SUGGESTION] line 53: Unused secret_keys variable — imported private key material is never validated
`create_test_vectors()` returns secret keys, but both `test_cpp_rust_two_signers` (line 53) and `test_cpp_rust_three_signers` (line 78) bind `secret_keys` without using it. This means the test never verifies that Rust parses the C++ secret key bytes into the same scalar or derives the same public key. For consensus-critical BLS code, add assertions like `assert_eq!(sk.public_key(), pk)` to validate the secret-key-to-public-key derivation is consistent across implementations. At minimum, prefix with `_` to suppress warnings.
In `src/test_verifysecure_vectors.cpp`:
- [BLOCKING] lines 34-116: test_verifysecure_vectors.cpp never calls VerifySecure or AggregateSecure
Despite the filename and PR description, this program only uses `BasicSchemeMPL().Sign()` (line 76), `BasicSchemeMPL().Aggregate()` (line 84), and `BasicSchemeMPL().AggregateVerify()` (line 88). It never calls `CoreMPL::AggregateSecure()` or `CoreMPL::VerifySecure()`. The header comment on line 37 even acknowledges this: "coefficients must be computed separately." The compatibility-sensitive part of VerifySecure — key sorting and coefficient hashing via `HashPubKeys` — is completely untested by this file.
In `src/test_cross_compatibility.cpp`:
- [SUGGESTION] lines 121-122: Missing error handling for KeyGen calls in test_cross_compatibility.cpp
test_verifysecure_vectors.cpp wraps its `KeyGen` calls in try/catch (lines 51-57), but test_cross_compatibility.cpp calls `BasicSchemeMPL().KeyGen()` on lines 121-122 and 146 without any error handling. While KeyGen is unlikely to fail with valid 32-byte seeds, this is inconsistent with the error handling added in recent commits (878b67e, 7cd3296).
| if(BUILD_BLS_TESTS) | ||
| # Test vector generator for VerifySecure compatibility testing | ||
| add_executable(test_verifysecure_vectors test_verifysecure_vectors.cpp) | ||
| target_link_libraries(test_verifysecure_vectors PRIVATE dashbls) | ||
|
|
||
| # Cross-compatibility test generator for true C++/Rust interoperability | ||
| add_executable(test_cross_compatibility test_cross_compatibility.cpp) | ||
| target_link_libraries(test_cross_compatibility PRIVATE dashbls) | ||
| endif() |
There was a problem hiding this comment.
🔴 Blocking: test_full_compatibility.cpp is not registered as a build target — dead code
test_full_compatibility.cpp is the only file that actually exercises CoreMPL::AggregateSecure and CoreMPL::VerifySecure, making it the most valuable test in this PR. But it has no add_executable() entry in CMakeLists.txt, so it will never be compiled, and any errors in it will go undetected. This completely undermines the stated goal of verifying VerifySecure compatibility.
| if(BUILD_BLS_TESTS) | |
| # Test vector generator for VerifySecure compatibility testing | |
| add_executable(test_verifysecure_vectors test_verifysecure_vectors.cpp) | |
| target_link_libraries(test_verifysecure_vectors PRIVATE dashbls) | |
| # Cross-compatibility test generator for true C++/Rust interoperability | |
| add_executable(test_cross_compatibility test_cross_compatibility.cpp) | |
| target_link_libraries(test_cross_compatibility PRIVATE dashbls) | |
| endif() | |
| if(BUILD_BLS_TESTS) | |
| include(${CMAKE_CURRENT_SOURCE_DIR}/../depends/catch2/CMakeLists.txt) | |
| add_executable(runtest test.cpp) | |
| target_link_libraries(runtest | |
| PRIVATE dashbls | |
| PRIVATE catch2) | |
| # Test vector generators for VerifySecure compatibility testing | |
| add_executable(test_verifysecure_vectors test_verifysecure_vectors.cpp) | |
| target_link_libraries(test_verifysecure_vectors PRIVATE dashbls) | |
| add_executable(test_cross_compatibility test_cross_compatibility.cpp) | |
| target_link_libraries(test_cross_compatibility PRIVATE dashbls) | |
| add_executable(test_full_compatibility test_full_compatibility.cpp) | |
| target_link_libraries(test_full_compatibility PRIVATE dashbls) | |
| endif() |
source: ['claude-general', 'claude-security-auditor']
🤖 Fix this 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 `src/CMakeLists.txt`:
- [BLOCKING] lines 56-64: test_full_compatibility.cpp is not registered as a build target — dead code
test_full_compatibility.cpp is the only file that actually exercises `CoreMPL::AggregateSecure` and `CoreMPL::VerifySecure`, making it the most valuable test in this PR. But it has no `add_executable()` entry in CMakeLists.txt, so it will never be compiled, and any errors in it will go undetected. This completely undermines the stated goal of verifying VerifySecure compatibility.
| // Cross-Compatibility Test Vectors | ||
| // Generated by C++ bls-signatures for import into Rust | ||
| use blsful::{AggregateSignature, PublicKey, SecretKey, Signature, Bls12381G1Impl}; | ||
|
|
||
| // Helper function to create test keys and signatures | ||
| fn create_test_vectors(count: usize) -> (Vec<SecretKey<Bls12381G1Impl>>, Vec<PublicKey<Bls12381G1Impl>>, Vec<Signature<Bls12381G1Impl>>) { | ||
| // Pre-defined key and signature bytes from C++ bls-signatures | ||
| let sk_bytes = vec![ | ||
| [0x2a, 0x06, 0x16, 0x4d, 0xae, 0x47, 0x51, 0xe5, 0x66, 0xee, 0x28, 0x54, 0xf2, 0x86, 0x5f, 0x78, 0x2f, 0x28, 0xe2, 0x42, 0x0e, 0xc2, 0xad, 0xe0, 0x59, 0xed, 0x43, 0x49, 0x19, 0xb6, 0x7b, 0x5d], | ||
| [0x13, 0xc6, 0xe9, 0x4c, 0xf9, 0xd3, 0xaf, 0x2e, 0x5c, 0x35, 0xd5, 0xae, 0x8f, 0x68, 0x74, 0xc5, 0xbd, 0x81, 0xe5, 0x2a, 0xe6, 0xfa, 0xee, 0x28, 0x1d, 0x0a, 0x8f, 0x27, 0xc0, 0x6c, 0xeb, 0xa9], | ||
| [0x0c, 0x3f, 0xad, 0x5c, 0xd1, 0x1b, 0xd5, 0xbc, 0x8a, 0xec, 0xb1, 0xa3, 0x6d, 0x26, 0x3f, 0xd9, 0xa9, 0xaf, 0x86, 0x23, 0xed, 0x3f, 0x44, 0xfb, 0xb2, 0xcc, 0xbd, 0xa1, 0x7f, 0x08, 0x8c, 0x23], | ||
| ]; | ||
|
|
||
| let pk_bytes = vec![ | ||
| [0xb1, 0x45, 0xdf, 0xcb, 0x3c, 0xbd, 0xef, 0x21, 0x50, 0x2f, 0x30, 0x5d, 0x1b, 0xa1, 0xcb, 0xa5, 0x84, 0x79, 0x69, 0x18, 0x57, 0x1e, 0x8b, 0x5d, 0x85, 0xbe, 0x17, 0x6f, 0x34, 0x45, 0xac, 0x7a, 0xd9, 0x9a, 0xec, 0x19, 0xe1, 0x93, 0x1b, 0x69, 0x34, 0xd7, 0x29, 0x0b, 0x97, 0xec, 0x2d, 0x75], | ||
| [0x94, 0xa7, 0xd9, 0x42, 0x02, 0x7d, 0xf0, 0x42, 0xb1, 0x4b, 0x00, 0xd5, 0x65, 0x6c, 0xb6, 0xa3, 0xd7, 0x75, 0x51, 0xaf, 0x91, 0x7a, 0x18, 0x83, 0x60, 0x5c, 0x96, 0x4c, 0x4f, 0xfa, 0xaf, 0xb4, 0x4d, 0x72, 0x9d, 0xc5, 0xf5, 0x11, 0xc6, 0x04, 0x5b, 0x29, 0x08, 0x04, 0x40, 0xab, 0x85, 0xc8], | ||
| [0x81, 0x98, 0x50, 0xab, 0xe8, 0x4b, 0x78, 0x49, 0x4f, 0xa1, 0x81, 0xc9, 0x82, 0xe7, 0x97, 0x22, 0xd9, 0x78, 0xfa, 0x93, 0x90, 0xdd, 0x88, 0xc6, 0x7c, 0x95, 0x56, 0xba, 0x92, 0x07, 0xd4, 0x12, 0xa3, 0xa4, 0xc9, 0xf4, 0x70, 0x5b, 0xf0, 0x99, 0x91, 0x89, 0xb4, 0x9b, 0x25, 0x5d, 0x98, 0x83], | ||
| ]; | ||
|
|
||
| let sig_bytes = vec![ | ||
| [0x82, 0xc8, 0x08, 0x03, 0xa3, 0x24, 0x6f, 0x5d, 0x10, 0xb5, 0x19, 0x23, 0xd4, 0x96, 0x7b, 0xef, 0x55, 0x7c, 0xf0, 0x41, 0x6f, 0xa3, 0x06, 0x05, 0x94, 0x9c, 0xaa, 0xc6, 0x27, 0x3b, 0x59, 0x93, 0xd2, 0x6f, 0xef, 0x27, 0x8e, 0xb4, 0x86, 0x75, 0xc6, 0x2b, 0x42, 0x26, 0x6f, 0x9b, 0x01, 0x48, 0x0a, 0x8d, 0xc0, 0x7e, 0xf1, 0x68, 0xed, 0xd3, 0xf1, 0xa9, 0xca, 0xd3, 0x63, 0x30, 0x83, 0xdc, 0xb3, 0xc2, 0xdf, 0x37, 0x27, 0x89, 0xb8, 0x78, 0x71, 0x24, 0x9d, 0xab, 0x38, 0x0b, 0x07, 0x1d, 0xd4, 0x80, 0xd7, 0x42, 0xd0, 0x27, 0xf3, 0x7c, 0x36, 0xf8, 0x9e, 0x63, 0xb5, 0xf1, 0xe0, 0x93], | ||
| [0xb2, 0xca, 0x57, 0x80, 0x16, 0xe8, 0x96, 0x20, 0xb4, 0x94, 0x03, 0xa0, 0x29, 0xad, 0x96, 0x40, 0x27, 0x2c, 0x18, 0x45, 0x5a, 0x5f, 0xdd, 0xc4, 0x0f, 0x99, 0x9b, 0xd7, 0x5c, 0xb9, 0xda, 0xc0, 0x71, 0x61, 0x3c, 0xb5, 0xa6, 0x64, 0xe0, 0xa7, 0x19, 0xa2, 0x29, 0xff, 0x65, 0x51, 0xfd, 0x52, 0x0b, 0x6d, 0x92, 0x96, 0x75, 0x08, 0xd5, 0xf1, 0xf5, 0x8c, 0xd4, 0x14, 0xcb, 0xde, 0x44, 0xc9, 0xe3, 0x91, 0x18, 0x73, 0xb4, 0xa2, 0xe9, 0x54, 0x44, 0x29, 0xb8, 0x67, 0x6f, 0xca, 0x69, 0xcb, 0x97, 0xab, 0xc6, 0xf4, 0x99, 0x16, 0xd0, 0x8c, 0xee, 0xda, 0x42, 0xb0, 0x6b, 0xe3, 0x6f, 0xf4], | ||
| [0xa5, 0x05, 0x73, 0xad, 0x15, 0xdd, 0x23, 0x3d, 0xf0, 0x85, 0x69, 0x33, 0x72, 0x48, 0xba, 0xd3, 0x2c, 0xc2, 0x08, 0xd5, 0xda, 0x64, 0x5a, 0xf8, 0x16, 0x69, 0x43, 0x8d, 0x1a, 0x16, 0x54, 0xd5, 0xba, 0xe7, 0x6f, 0xc8, 0x20, 0x3b, 0x10, 0x8b, 0x91, 0x20, 0x73, 0xbd, 0x89, 0x8b, 0x56, 0xba, 0x0c, 0xee, 0x39, 0x71, 0xcd, 0x68, 0xbe, 0xc0, 0x01, 0x4a, 0x0d, 0x1a, 0x81, 0x4d, 0xbc, 0xe2, 0xc1, 0x52, 0x8e, 0x1d, 0x97, 0xd7, 0x5e, 0xd2, 0x69, 0xdf, 0x05, 0x5a, 0xc0, 0x02, 0xf9, 0x6e, 0x53, 0xfe, 0x3a, 0x58, 0xb1, 0x99, 0x90, 0x9b, 0xac, 0x0c, 0x62, 0xa7, 0x99, 0x92, 0xa5, 0x53], | ||
| ]; | ||
|
|
||
| assert!(count <= 3, "Helper function only supports up to 3 signers"); | ||
|
|
||
| let mut secret_keys = Vec::new(); | ||
| let mut public_keys = Vec::new(); | ||
| let mut signatures = Vec::new(); | ||
|
|
||
| for i in 0..count { | ||
| let sk = SecretKey::<Bls12381G1Impl>::try_from(sk_bytes[i].as_slice()) | ||
| .expect(&format!("Failed to create SecretKey from bytes for signer {}", i + 1)); | ||
| let pk = PublicKey::<Bls12381G1Impl>::try_from(pk_bytes[i].as_slice()) | ||
| .expect(&format!("Failed to create PublicKey from bytes for signer {}", i + 1)); | ||
| let sig = Signature::<Bls12381G1Impl>::try_from(sig_bytes[i].as_slice()) | ||
| .expect(&format!("Failed to create Signature from bytes for signer {}", i + 1)); | ||
|
|
||
| secret_keys.push(sk); | ||
| public_keys.push(pk); | ||
| signatures.push(sig); | ||
| } | ||
|
|
||
| (secret_keys, public_keys, signatures) | ||
| } | ||
|
|
||
| // === Test Case 1: Two Signers === | ||
|
|
||
| #[test] | ||
| fn test_cpp_rust_two_signers() { | ||
| // Use helper function to create test vectors | ||
| let (secret_keys, public_keys, signatures) = create_test_vectors(2); | ||
|
|
||
| // Message | ||
| let message = [0x68, 0x65, 0x6c, 0x6c, 0x6f]; | ||
|
|
||
| // Verify C++ signatures in Rust | ||
| assert!(signatures[0].verify(&public_keys[0], &message).is_ok()); | ||
| assert!(signatures[1].verify(&public_keys[1], &message).is_ok()); | ||
|
|
||
| // Create secure aggregation in Rust using C++ keys | ||
| let secure_agg = AggregateSignature::from_signatures_secure(&signatures, &public_keys).unwrap(); | ||
| let final_sig = match secure_agg { | ||
| AggregateSignature::Basic(sig) => Signature::Basic(sig), | ||
| _ => panic!("Expected Basic scheme"), | ||
| }; | ||
|
|
||
| // Verify secure aggregation | ||
| assert!(final_sig.verify_secure(&public_keys, &message).is_ok()); | ||
| } | ||
|
|
||
| // === Test Case 2: Three Signers === | ||
|
|
||
| #[test] | ||
| fn test_cpp_rust_three_signers() { | ||
| // Use helper function to create test vectors | ||
| let (secret_keys, public_keys, signatures) = create_test_vectors(3); | ||
|
|
||
| // Message | ||
| let message = [0x68, 0x65, 0x6c, 0x6c, 0x6f]; | ||
|
|
||
| // Verify C++ signatures in Rust | ||
| assert!(signatures[0].verify(&public_keys[0], &message).is_ok()); | ||
| assert!(signatures[1].verify(&public_keys[1], &message).is_ok()); | ||
| assert!(signatures[2].verify(&public_keys[2], &message).is_ok()); | ||
|
|
||
| // Create secure aggregation in Rust using C++ keys | ||
| let secure_agg = AggregateSignature::from_signatures_secure(&signatures, &public_keys).unwrap(); | ||
| let final_sig = match secure_agg { | ||
| AggregateSignature::Basic(sig) => Signature::Basic(sig), | ||
| _ => panic!("Expected Basic scheme"), | ||
| }; | ||
|
|
||
| // Verify secure aggregation | ||
| assert!(final_sig.verify_secure(&public_keys, &message).is_ok()); | ||
| } | ||
|
|
||
| // === Test Normal Aggregation (should fail verify_secure) === | ||
|
|
||
| #[test] | ||
| fn test_normal_aggregation_fails_secure_verify() { | ||
| let normal_agg_bytes = [0x8e, 0x40, 0x44, 0x44, 0x23, 0x14, 0xab, 0xb8, 0xbb, 0xdb, 0xd5, 0x06, 0xc6, 0xf7, 0xbe, 0xd1, 0xb7, 0xd0, 0x00, 0x40, 0x77, 0x52, 0x38, 0x4e, 0xca, 0x9e, 0xbf, 0xda, 0x34, 0xd0, 0x61, 0x21, 0x8e, 0x07, 0xf2, 0x5a, 0x0c, 0xbe, 0x42, 0xdc, 0xaf, 0x68, 0xac, 0x9e, 0xa9, 0xe5, 0xa0, 0x80, 0x0a, 0x56, 0x44, 0xa1, 0xf0, 0x20, 0xea, 0xd6, 0xe6, 0x27, 0xb3, 0xe0, 0x34, 0x93, 0x00, 0x02, 0x1c, 0xce, 0x82, 0xc4, 0xc9, 0xa3, 0x3a, 0x0a, 0x88, 0x20, 0x85, 0xc7, 0xc1, 0xeb, 0x93, 0x46, 0xa9, 0x3f, 0x0f, 0xd8, 0x0a, 0xd4, 0xee, 0x6e, 0x7a, 0x6c, 0x2a, 0xd7, 0x7e, 0xd2, 0xc0, 0x2a]; | ||
|
|
||
| // This normal aggregation should fail verify_secure | ||
| let normal_sig = Signature::<Bls12381G1Impl>::try_from(normal_agg_bytes.as_slice()).unwrap(); | ||
|
|
||
| // Get public keys from helper function | ||
| let (_, public_keys, _) = create_test_vectors(2); | ||
|
|
||
| let message = [0x68, 0x65, 0x6c, 0x6c, 0x6f]; | ||
|
|
||
| // Normal aggregation should fail verify_secure | ||
| assert!(normal_sig.verify_secure(&public_keys, &message).is_err()); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Rust test file cannot be compiled — no Cargo.toml, wrong crate dependency
This file sits at the repo root with no Cargo.toml, no module declaration, and no test harness. It imports blsful (a third-party pure-Rust BLS12-381 crate), but the repo's actual Rust bindings live under rust-bindings/ and use bls-dash-sys (FFI to this C++ library). The cross-compatibility claim is unverifiable because this file can never be compiled or executed. Either integrate it into rust-bindings/ using the repo's own crate, or add a dedicated Cargo.toml with blsful as a dependency.
source: ['claude-general']
🤖 Fix this 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 `cross_compatibility_test_vectors.rs`:
- [BLOCKING] lines 1-115: Rust test file cannot be compiled — no Cargo.toml, wrong crate dependency
This file sits at the repo root with no Cargo.toml, no module declaration, and no test harness. It imports `blsful` (a third-party pure-Rust BLS12-381 crate), but the repo's actual Rust bindings live under `rust-bindings/` and use `bls-dash-sys` (FFI to this C++ library). The cross-compatibility claim is unverifiable because this file can never be compiled or executed. Either integrate it into rust-bindings/ using the repo's own crate, or add a dedicated Cargo.toml with blsful as a dependency.
| int main() { | ||
| cout << "// Test Vectors for C++/Rust Compatibility" << endl; | ||
| cout << "// Generated by bls-signatures C++ implementation" << endl; | ||
| cout << "// These are the raw values - coefficients must be computed separately" << endl; | ||
|
|
||
| // Test Case 1: Three deterministic keys | ||
| cout << "\n// Test Case 1: Three signers with deterministic keys" << endl; | ||
|
|
||
| vector<PrivateKey> sks; | ||
| vector<G1Element> pks; | ||
|
|
||
| // Generate keys using seed [1,1,1...], [2,2,2...], [3,3,3...] | ||
| cout << "\n// Private keys generated from seeds:" << endl; | ||
| for (uint8_t i = 1; i <= 3; i++) { | ||
| vector<uint8_t> seed(32, i); | ||
| cout << "// Seed " << (int)i << ": [" << (int)i << ", " << (int)i << ", ...] (32 bytes)" << endl; | ||
| try { | ||
| PrivateKey sk = BasicSchemeMPL().KeyGen(seed); | ||
| sks.push_back(sk); | ||
| pks.push_back(sk.GetG1Element()); | ||
| } catch (const std::exception& e) { | ||
| cerr << "Error generating private key from seed " << (int)i << ": " << e.what() << endl; | ||
| return 1; | ||
| } | ||
| } | ||
|
|
||
| // Print public keys | ||
| cout << "\n// Public keys (48 bytes each, G1 compressed):" << endl; | ||
| for (size_t i = 0; i < pks.size(); i++) { | ||
| auto pkBytes = pks[i].Serialize(); | ||
| printHex("let pk" + to_string(i+1) + "_bytes", pkBytes); | ||
| } | ||
|
|
||
| // Message | ||
| vector<uint8_t> message = {0x74, 0x65, 0x73, 0x74, 0x20, 0x6d, 0x65, 0x73, 0x73, 0x61, 0x67, 0x65}; // "test message" | ||
| cout << "\n// Message: \"test message\"" << endl; | ||
| printRustArray("let message", message); | ||
|
|
||
| // Generate individual signatures | ||
| cout << "\n// Individual signatures (96 bytes each, G2 compressed):" << endl; | ||
| vector<G2Element> sigs; | ||
| for (size_t i = 0; i < sks.size(); i++) { | ||
| G2Element sig = BasicSchemeMPL().Sign(sks[i], message); | ||
| sigs.push_back(sig); | ||
| auto sigBytes = sig.Serialize(); | ||
| printHex("let sig" + to_string(i+1) + "_bytes", sigBytes); | ||
| } | ||
|
|
||
| // Aggregate signatures (without coefficients - just for reference) | ||
| cout << "\n// Basic aggregated signature (no coefficients):" << endl; | ||
| G2Element aggSig = BasicSchemeMPL().Aggregate(sigs); | ||
| printHex("let agg_sig_basic", aggSig.Serialize()); | ||
|
|
||
| // Verify basic aggregation works | ||
| bool basicVerify = BasicSchemeMPL().AggregateVerify(pks, vector<vector<uint8_t>>(pks.size(), message), aggSig); | ||
| cout << "// Basic aggregate verify result: " << (basicVerify ? "true" : "false") << endl; | ||
|
|
||
| // Test with different key orders | ||
| cout << "\n// Test Case 2: Keys in reversed order" << endl; | ||
| vector<G1Element> pksReversed = {pks[2], pks[1], pks[0]}; | ||
| vector<G2Element> sigsReversed = {sigs[2], sigs[1], sigs[0]}; | ||
|
|
||
| cout << "// Public keys in reversed order:" << endl; | ||
| for (size_t i = 0; i < pksReversed.size(); i++) { | ||
| auto pkBytes = pksReversed[i].Serialize(); | ||
| cout << "// pk_reversed[" << i << "] = pk" << (3-i) << endl; | ||
| } | ||
|
|
||
| G2Element aggSigReversed = BasicSchemeMPL().Aggregate(sigsReversed); | ||
| printHex("let agg_sig_reversed", aggSigReversed.Serialize()); | ||
|
|
||
| // Also print some debug info | ||
| cout << "\n// Debug: First 8 bytes of each public key for easy comparison:" << endl; | ||
| for (size_t i = 0; i < pks.size(); i++) { | ||
| auto pkBytes = pks[i].Serialize(); | ||
| cout << "// pk" << (i+1) << " first 8 bytes: "; | ||
| for (size_t j = 0; j < 8 && j < pkBytes.size(); j++) { | ||
| cout << setfill('0') << setw(2) << hex << (int)pkBytes[j] << " "; | ||
| } | ||
| cout << endl; | ||
| } | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
🔴 Blocking: test_verifysecure_vectors.cpp never calls VerifySecure or AggregateSecure
Despite the filename and PR description, this program only uses BasicSchemeMPL().Sign() (line 76), BasicSchemeMPL().Aggregate() (line 84), and BasicSchemeMPL().AggregateVerify() (line 88). It never calls CoreMPL::AggregateSecure() or CoreMPL::VerifySecure(). The header comment on line 37 even acknowledges this: "coefficients must be computed separately." The compatibility-sensitive part of VerifySecure — key sorting and coefficient hashing via HashPubKeys — is completely untested by this file.
source: ['codex-general', 'claude-general']
🤖 Fix this 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 `src/test_verifysecure_vectors.cpp`:
- [BLOCKING] lines 34-116: test_verifysecure_vectors.cpp never calls VerifySecure or AggregateSecure
Despite the filename and PR description, this program only uses `BasicSchemeMPL().Sign()` (line 76), `BasicSchemeMPL().Aggregate()` (line 84), and `BasicSchemeMPL().AggregateVerify()` (line 88). It never calls `CoreMPL::AggregateSecure()` or `CoreMPL::VerifySecure()`. The header comment on line 37 even acknowledges this: "coefficients must be computed separately." The compatibility-sensitive part of VerifySecure — key sorting and coefficient hashing via `HashPubKeys` — is completely untested by this file.
| if(BUILD_BLS_TESTS) | ||
| # Test vector generator for VerifySecure compatibility testing | ||
| add_executable(test_verifysecure_vectors test_verifysecure_vectors.cpp) | ||
| target_link_libraries(test_verifysecure_vectors PRIVATE dashbls) | ||
|
|
||
| # Cross-compatibility test generator for true C++/Rust interoperability | ||
| add_executable(test_cross_compatibility test_cross_compatibility.cpp) | ||
| target_link_libraries(test_cross_compatibility PRIVATE dashbls) | ||
| endif() |
There was a problem hiding this comment.
🟡 Suggestion: New executables are not part of any CI test execution path
The new executables are standalone generators that print to stdout. They are not Catch2 tests, not registered with add_test(), and CI does not invoke them. If the generated vectors drift from what the Rust side expects, no CI check will catch it. Consider either: (a) adding add_test() commands so CTest runs them, or (b) incorporating the vector generation and verification into the existing test.cpp Catch2 suite.
source: ['codex-general']
🤖 Fix this 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 `src/CMakeLists.txt`:
- [SUGGESTION] lines 56-64: New executables are not part of any CI test execution path
The new executables are standalone generators that print to stdout. They are not Catch2 tests, not registered with `add_test()`, and CI does not invoke them. If the generated vectors drift from what the Rust side expects, no CI check will catch it. Consider either: (a) adding `add_test()` commands so CTest runs them, or (b) incorporating the vector generation and verification into the existing `test.cpp` Catch2 suite.
| // Create secure aggregation in Rust using C++ keys | ||
| let secure_agg = AggregateSignature::from_signatures_secure(&signatures, &public_keys).unwrap(); | ||
| let final_sig = match secure_agg { | ||
| AggregateSignature::Basic(sig) => Signature::Basic(sig), | ||
| _ => panic!("Expected Basic scheme"), | ||
| }; | ||
|
|
||
| // Verify secure aggregation | ||
| assert!(final_sig.verify_secure(&public_keys, &message).is_ok()); |
There was a problem hiding this comment.
🟡 Suggestion: Rust tests don't verify C++ secure aggregation output — only Rust-side aggregation
The Rust tests import C++ individual signatures and public keys, then perform secure aggregation entirely in Rust via AggregateSignature::from_signatures_secure() and verify with verify_secure(). This proves Rust can securely aggregate C++ individual signatures, but does NOT prove that C++ AggregateSecure produces byte-identical output to Rust's secure aggregation. A true cross-compatibility test should import the C++ secure aggregation result (from test_full_compatibility.cpp) and compare it to the Rust-computed result.
source: ['claude-general', 'codex-security-auditor']
🤖 Fix this 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 `cross_compatibility_test_vectors.rs`:
- [SUGGESTION] lines 62-70: Rust tests don't verify C++ secure aggregation output — only Rust-side aggregation
The Rust tests import C++ individual signatures and public keys, then perform secure aggregation entirely in Rust via `AggregateSignature::from_signatures_secure()` and verify with `verify_secure()`. This proves Rust can securely aggregate C++ individual signatures, but does NOT prove that C++ `AggregateSecure` produces byte-identical output to Rust's secure aggregation. A true cross-compatibility test should import the C++ secure aggregation result (from test_full_compatibility.cpp) and compare it to the Rust-computed result.
| #[test] | ||
| fn test_cpp_rust_two_signers() { | ||
| // Use helper function to create test vectors | ||
| let (secret_keys, public_keys, signatures) = create_test_vectors(2); |
There was a problem hiding this comment.
🟡 Suggestion: Unused secret_keys variable — imported private key material is never validated
create_test_vectors() returns secret keys, but both test_cpp_rust_two_signers (line 53) and test_cpp_rust_three_signers (line 78) bind secret_keys without using it. This means the test never verifies that Rust parses the C++ secret key bytes into the same scalar or derives the same public key. For consensus-critical BLS code, add assertions like assert_eq!(sk.public_key(), pk) to validate the secret-key-to-public-key derivation is consistent across implementations. At minimum, prefix with _ to suppress warnings.
source: ['claude-general', 'codex-security-auditor', 'claude-security-auditor']
🤖 Fix this 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 `cross_compatibility_test_vectors.rs`:
- [SUGGESTION] line 53: Unused secret_keys variable — imported private key material is never validated
`create_test_vectors()` returns secret keys, but both `test_cpp_rust_two_signers` (line 53) and `test_cpp_rust_three_signers` (line 78) bind `secret_keys` without using it. This means the test never verifies that Rust parses the C++ secret key bytes into the same scalar or derives the same public key. For consensus-critical BLS code, add assertions like `assert_eq!(sk.public_key(), pk)` to validate the secret-key-to-public-key derivation is consistent across implementations. At minimum, prefix with `_` to suppress warnings.
| PrivateKey sk1 = BasicSchemeMPL().KeyGen(seed1); | ||
| PrivateKey sk2 = BasicSchemeMPL().KeyGen(seed2); |
There was a problem hiding this comment.
🟡 Suggestion: Missing error handling for KeyGen calls in test_cross_compatibility.cpp
test_verifysecure_vectors.cpp wraps its KeyGen calls in try/catch (lines 51-57), but test_cross_compatibility.cpp calls BasicSchemeMPL().KeyGen() on lines 121-122 and 146 without any error handling. While KeyGen is unlikely to fail with valid 32-byte seeds, this is inconsistent with the error handling added in recent commits (878b67e, 7cd3296).
source: ['claude-general']
🤖 Fix this 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 `src/test_cross_compatibility.cpp`:
- [SUGGESTION] lines 121-122: Missing error handling for KeyGen calls in test_cross_compatibility.cpp
test_verifysecure_vectors.cpp wraps its `KeyGen` calls in try/catch (lines 51-57), but test_cross_compatibility.cpp calls `BasicSchemeMPL().KeyGen()` on lines 121-122 and 146 without any error handling. While KeyGen is unlikely to fail with valid 32-byte seeds, this is inconsistent with the error handling added in recent commits (878b67e, 7cd3296).
| add_executable(runbench test-bench.cpp) | ||
| target_link_libraries(runbench PRIVATE dashbls) | ||
| endif() | ||
|
|
||
| if(BUILD_BLS_TESTS) | ||
| # Test vector generator for VerifySecure compatibility testing | ||
| add_executable(test_verifysecure_vectors test_verifysecure_vectors.cpp) | ||
| target_link_libraries(test_verifysecure_vectors PRIVATE dashbls) | ||
|
|
||
| # Cross-compatibility test generator for true C++/Rust interoperability | ||
| add_executable(test_cross_compatibility test_cross_compatibility.cpp) | ||
| target_link_libraries(test_cross_compatibility PRIVATE dashbls) | ||
| endif() |
There was a problem hiding this comment.
💬 Nitpick: Duplicate BUILD_BLS_TESTS conditional blocks should be merged
There are two separate if(BUILD_BLS_TESTS) blocks (lines 42-49 and 56-64). These should be a single block for maintainability.
source: ['claude-general']
|
✅ Review complete (commit fb4741c) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR adds test/vector-generation scaffolding for VerifySecure C++/Rust interop, but the wiring is broken in three converging ways: a generator file is never built, the Rust vectors aren't part of any cargo target, and the only generator that is built doesn't actually exercise VerifySecure. A separate concern: the orphan generator subclasses CoreMPL purely to promote protected secure-aggregation methods that take a raw fLegacy flag.
Reviewed commit: fb4741c
🔴 3 blocking | 🟡 1 suggestion(s)
🤖 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 `src/test_verifysecure_vectors.cpp`:
- [BLOCKING] lines 35-116: VerifySecure vector generator never calls VerifySecure or AggregateSecure
Despite its name and stated purpose, this generator only emits individual signatures and a plain `BasicSchemeMPL().Aggregate(...)` result. The header comment even concedes that 'coefficients must be computed separately', and the only verification performed is `BasicSchemeMPL().AggregateVerify` on the basic aggregate (line 88). There is no call to `CoreMPL::AggregateSecure` or `CoreMPL::VerifySecure`, so the coefficient-hashing/sorting logic that distinguishes VerifySecure is entirely absent from the generated vectors. A downstream Rust implementation can diverge in the secure path while still matching every artifact this tool produces, defeating the stated interop goal. Either drive the secure path through the public `BasicSchemeMPL`/`LegacySchemeMPL` API and emit the resulting aggregate plus a `VerifySecure` boolean, or remove the file as misleading.
In `src/CMakeLists.txt`:
- [BLOCKING] lines 56-64: src/test_full_compatibility.cpp is shipped but never compiled
`src/test_full_compatibility.cpp` exists in the tree but the `BUILD_BLS_TESTS` block only registers `test_verifysecure_vectors` and `test_cross_compatibility`. The full-compatibility generator is therefore never compiled by CI or any local build, even though the PR description calls it out as a key deliverable. Orphan sources rot silently against API changes and become a vector for stale or subtly-wrong cryptographic vectors that someone later 'rescues' and trusts. It is also the file containing the most security-sensitive pattern in this patch (re-exposing protected `AggregateSecure`/`VerifySecure`, see related finding). Either wire it into the `if(BUILD_BLS_TESTS)` block or delete it from the tree.
In `cross_compatibility_test_vectors.rs`:
- [BLOCKING] lines 1-115: Rust cross-compatibility tests are not wired into any Cargo target
This file lives at the repository root and contains `#[test]` functions, but it is not picked up by any Cargo target. `rust-bindings/bls-signatures` and `rust-bindings/bls-dash-sys` have no `tests/` directory containing it, no `[[test]]` entry referencing it, and crucially neither crate depends on `blsful` (which the file imports on line 3). `cargo test` therefore never compiles or runs these cases, so the PR's central claim of validated C++/Rust VerifySecure interop is not enforced anywhere — the vectors can rot, the external API can change, and CI stays green. Move the file into `rust-bindings/bls-signatures/tests/` (or a new dedicated interop crate), add the `blsful`/`subtle` dev-dependencies that it requires, and confirm `cargo test` actually runs it.
In `src/test_full_compatibility.cpp`:
- [SUGGESTION] lines 25-32: Test helper re-exposes protected AggregateSecure/VerifySecure with a raw fLegacy flag
`TestCoreMPL` derives from `CoreMPL` solely to promote the protected overloads via `using CoreMPL::AggregateSecure; using CoreMPL::VerifySecure;`. Those overloads are deliberately kept protected at `include/dashbls/schemes.hpp:106-113` so that scheme-specific public wrappers (`BasicSchemeMPL`/`LegacySchemeMPL`) can fix the legacy domain-separation flag internally. Bypassing that encapsulation in test code is a footgun: future contributors can copy this pattern and silently flip `fLegacy`, producing cross-domain-separation 'reference' vectors that other implementations would then trust. The current call sites pass `false` and the file isn't even built today, but if this generator is wired into CMake (per the related finding) it should drive the secure path through the public scheme API rather than re-exposing protected internals.
| cout << "// Test Vectors for C++/Rust Compatibility" << endl; | ||
| cout << "// Generated by bls-signatures C++ implementation" << endl; | ||
| cout << "// These are the raw values - coefficients must be computed separately" << endl; | ||
|
|
||
| // Test Case 1: Three deterministic keys | ||
| cout << "\n// Test Case 1: Three signers with deterministic keys" << endl; | ||
|
|
||
| vector<PrivateKey> sks; | ||
| vector<G1Element> pks; | ||
|
|
||
| // Generate keys using seed [1,1,1...], [2,2,2...], [3,3,3...] | ||
| cout << "\n// Private keys generated from seeds:" << endl; | ||
| for (uint8_t i = 1; i <= 3; i++) { | ||
| vector<uint8_t> seed(32, i); | ||
| cout << "// Seed " << (int)i << ": [" << (int)i << ", " << (int)i << ", ...] (32 bytes)" << endl; | ||
| try { | ||
| PrivateKey sk = BasicSchemeMPL().KeyGen(seed); | ||
| sks.push_back(sk); | ||
| pks.push_back(sk.GetG1Element()); | ||
| } catch (const std::exception& e) { | ||
| cerr << "Error generating private key from seed " << (int)i << ": " << e.what() << endl; | ||
| return 1; | ||
| } | ||
| } | ||
|
|
||
| // Print public keys | ||
| cout << "\n// Public keys (48 bytes each, G1 compressed):" << endl; | ||
| for (size_t i = 0; i < pks.size(); i++) { | ||
| auto pkBytes = pks[i].Serialize(); | ||
| printHex("let pk" + to_string(i+1) + "_bytes", pkBytes); | ||
| } | ||
|
|
||
| // Message | ||
| vector<uint8_t> message = {0x74, 0x65, 0x73, 0x74, 0x20, 0x6d, 0x65, 0x73, 0x73, 0x61, 0x67, 0x65}; // "test message" | ||
| cout << "\n// Message: \"test message\"" << endl; | ||
| printRustArray("let message", message); | ||
|
|
||
| // Generate individual signatures | ||
| cout << "\n// Individual signatures (96 bytes each, G2 compressed):" << endl; | ||
| vector<G2Element> sigs; | ||
| for (size_t i = 0; i < sks.size(); i++) { | ||
| G2Element sig = BasicSchemeMPL().Sign(sks[i], message); | ||
| sigs.push_back(sig); | ||
| auto sigBytes = sig.Serialize(); | ||
| printHex("let sig" + to_string(i+1) + "_bytes", sigBytes); | ||
| } | ||
|
|
||
| // Aggregate signatures (without coefficients - just for reference) | ||
| cout << "\n// Basic aggregated signature (no coefficients):" << endl; | ||
| G2Element aggSig = BasicSchemeMPL().Aggregate(sigs); | ||
| printHex("let agg_sig_basic", aggSig.Serialize()); | ||
|
|
||
| // Verify basic aggregation works | ||
| bool basicVerify = BasicSchemeMPL().AggregateVerify(pks, vector<vector<uint8_t>>(pks.size(), message), aggSig); | ||
| cout << "// Basic aggregate verify result: " << (basicVerify ? "true" : "false") << endl; | ||
|
|
||
| // Test with different key orders | ||
| cout << "\n// Test Case 2: Keys in reversed order" << endl; | ||
| vector<G1Element> pksReversed = {pks[2], pks[1], pks[0]}; | ||
| vector<G2Element> sigsReversed = {sigs[2], sigs[1], sigs[0]}; | ||
|
|
||
| cout << "// Public keys in reversed order:" << endl; | ||
| for (size_t i = 0; i < pksReversed.size(); i++) { | ||
| auto pkBytes = pksReversed[i].Serialize(); | ||
| cout << "// pk_reversed[" << i << "] = pk" << (3-i) << endl; | ||
| } | ||
|
|
||
| G2Element aggSigReversed = BasicSchemeMPL().Aggregate(sigsReversed); | ||
| printHex("let agg_sig_reversed", aggSigReversed.Serialize()); | ||
|
|
||
| // Also print some debug info | ||
| cout << "\n// Debug: First 8 bytes of each public key for easy comparison:" << endl; | ||
| for (size_t i = 0; i < pks.size(); i++) { | ||
| auto pkBytes = pks[i].Serialize(); | ||
| cout << "// pk" << (i+1) << " first 8 bytes: "; | ||
| for (size_t j = 0; j < 8 && j < pkBytes.size(); j++) { | ||
| cout << setfill('0') << setw(2) << hex << (int)pkBytes[j] << " "; | ||
| } | ||
| cout << endl; | ||
| } | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
🔴 Blocking: VerifySecure vector generator never calls VerifySecure or AggregateSecure
Despite its name and stated purpose, this generator only emits individual signatures and a plain BasicSchemeMPL().Aggregate(...) result. The header comment even concedes that 'coefficients must be computed separately', and the only verification performed is BasicSchemeMPL().AggregateVerify on the basic aggregate (line 88). There is no call to CoreMPL::AggregateSecure or CoreMPL::VerifySecure, so the coefficient-hashing/sorting logic that distinguishes VerifySecure is entirely absent from the generated vectors. A downstream Rust implementation can diverge in the secure path while still matching every artifact this tool produces, defeating the stated interop goal. Either drive the secure path through the public BasicSchemeMPL/LegacySchemeMPL API and emit the resulting aggregate plus a VerifySecure boolean, or remove the file as misleading.
source: ['codex']
🤖 Fix this 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 `src/test_verifysecure_vectors.cpp`:
- [BLOCKING] lines 35-116: VerifySecure vector generator never calls VerifySecure or AggregateSecure
Despite its name and stated purpose, this generator only emits individual signatures and a plain `BasicSchemeMPL().Aggregate(...)` result. The header comment even concedes that 'coefficients must be computed separately', and the only verification performed is `BasicSchemeMPL().AggregateVerify` on the basic aggregate (line 88). There is no call to `CoreMPL::AggregateSecure` or `CoreMPL::VerifySecure`, so the coefficient-hashing/sorting logic that distinguishes VerifySecure is entirely absent from the generated vectors. A downstream Rust implementation can diverge in the secure path while still matching every artifact this tool produces, defeating the stated interop goal. Either drive the secure path through the public `BasicSchemeMPL`/`LegacySchemeMPL` API and emit the resulting aggregate plus a `VerifySecure` boolean, or remove the file as misleading.
| if(BUILD_BLS_TESTS) | ||
| # Test vector generator for VerifySecure compatibility testing | ||
| add_executable(test_verifysecure_vectors test_verifysecure_vectors.cpp) | ||
| target_link_libraries(test_verifysecure_vectors PRIVATE dashbls) | ||
|
|
||
| # Cross-compatibility test generator for true C++/Rust interoperability | ||
| add_executable(test_cross_compatibility test_cross_compatibility.cpp) | ||
| target_link_libraries(test_cross_compatibility PRIVATE dashbls) | ||
| endif() |
There was a problem hiding this comment.
🔴 Blocking: src/test_full_compatibility.cpp is shipped but never compiled
src/test_full_compatibility.cpp exists in the tree but the BUILD_BLS_TESTS block only registers test_verifysecure_vectors and test_cross_compatibility. The full-compatibility generator is therefore never compiled by CI or any local build, even though the PR description calls it out as a key deliverable. Orphan sources rot silently against API changes and become a vector for stale or subtly-wrong cryptographic vectors that someone later 'rescues' and trusts. It is also the file containing the most security-sensitive pattern in this patch (re-exposing protected AggregateSecure/VerifySecure, see related finding). Either wire it into the if(BUILD_BLS_TESTS) block or delete it from the tree.
💡 Suggested change
| if(BUILD_BLS_TESTS) | |
| # Test vector generator for VerifySecure compatibility testing | |
| add_executable(test_verifysecure_vectors test_verifysecure_vectors.cpp) | |
| target_link_libraries(test_verifysecure_vectors PRIVATE dashbls) | |
| # Cross-compatibility test generator for true C++/Rust interoperability | |
| add_executable(test_cross_compatibility test_cross_compatibility.cpp) | |
| target_link_libraries(test_cross_compatibility PRIVATE dashbls) | |
| endif() | |
| if(BUILD_BLS_TESTS) | |
| # Test vector generator for VerifySecure compatibility testing | |
| add_executable(test_verifysecure_vectors test_verifysecure_vectors.cpp) | |
| target_link_libraries(test_verifysecure_vectors PRIVATE dashbls) | |
| # Cross-compatibility test generator for true C++/Rust interoperability | |
| add_executable(test_cross_compatibility test_cross_compatibility.cpp) | |
| target_link_libraries(test_cross_compatibility PRIVATE dashbls) | |
| # Full compatibility vector generator | |
| add_executable(test_full_compatibility test_full_compatibility.cpp) | |
| target_link_libraries(test_full_compatibility PRIVATE dashbls) | |
| endif() |
source: ['claude', 'codex']
🤖 Fix this 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 `src/CMakeLists.txt`:
- [BLOCKING] lines 56-64: src/test_full_compatibility.cpp is shipped but never compiled
`src/test_full_compatibility.cpp` exists in the tree but the `BUILD_BLS_TESTS` block only registers `test_verifysecure_vectors` and `test_cross_compatibility`. The full-compatibility generator is therefore never compiled by CI or any local build, even though the PR description calls it out as a key deliverable. Orphan sources rot silently against API changes and become a vector for stale or subtly-wrong cryptographic vectors that someone later 'rescues' and trusts. It is also the file containing the most security-sensitive pattern in this patch (re-exposing protected `AggregateSecure`/`VerifySecure`, see related finding). Either wire it into the `if(BUILD_BLS_TESTS)` block or delete it from the tree.
| // Cross-Compatibility Test Vectors | ||
| // Generated by C++ bls-signatures for import into Rust | ||
| use blsful::{AggregateSignature, PublicKey, SecretKey, Signature, Bls12381G1Impl}; | ||
|
|
||
| // Helper function to create test keys and signatures | ||
| fn create_test_vectors(count: usize) -> (Vec<SecretKey<Bls12381G1Impl>>, Vec<PublicKey<Bls12381G1Impl>>, Vec<Signature<Bls12381G1Impl>>) { | ||
| // Pre-defined key and signature bytes from C++ bls-signatures | ||
| let sk_bytes = vec![ | ||
| [0x2a, 0x06, 0x16, 0x4d, 0xae, 0x47, 0x51, 0xe5, 0x66, 0xee, 0x28, 0x54, 0xf2, 0x86, 0x5f, 0x78, 0x2f, 0x28, 0xe2, 0x42, 0x0e, 0xc2, 0xad, 0xe0, 0x59, 0xed, 0x43, 0x49, 0x19, 0xb6, 0x7b, 0x5d], | ||
| [0x13, 0xc6, 0xe9, 0x4c, 0xf9, 0xd3, 0xaf, 0x2e, 0x5c, 0x35, 0xd5, 0xae, 0x8f, 0x68, 0x74, 0xc5, 0xbd, 0x81, 0xe5, 0x2a, 0xe6, 0xfa, 0xee, 0x28, 0x1d, 0x0a, 0x8f, 0x27, 0xc0, 0x6c, 0xeb, 0xa9], | ||
| [0x0c, 0x3f, 0xad, 0x5c, 0xd1, 0x1b, 0xd5, 0xbc, 0x8a, 0xec, 0xb1, 0xa3, 0x6d, 0x26, 0x3f, 0xd9, 0xa9, 0xaf, 0x86, 0x23, 0xed, 0x3f, 0x44, 0xfb, 0xb2, 0xcc, 0xbd, 0xa1, 0x7f, 0x08, 0x8c, 0x23], | ||
| ]; | ||
|
|
||
| let pk_bytes = vec![ | ||
| [0xb1, 0x45, 0xdf, 0xcb, 0x3c, 0xbd, 0xef, 0x21, 0x50, 0x2f, 0x30, 0x5d, 0x1b, 0xa1, 0xcb, 0xa5, 0x84, 0x79, 0x69, 0x18, 0x57, 0x1e, 0x8b, 0x5d, 0x85, 0xbe, 0x17, 0x6f, 0x34, 0x45, 0xac, 0x7a, 0xd9, 0x9a, 0xec, 0x19, 0xe1, 0x93, 0x1b, 0x69, 0x34, 0xd7, 0x29, 0x0b, 0x97, 0xec, 0x2d, 0x75], | ||
| [0x94, 0xa7, 0xd9, 0x42, 0x02, 0x7d, 0xf0, 0x42, 0xb1, 0x4b, 0x00, 0xd5, 0x65, 0x6c, 0xb6, 0xa3, 0xd7, 0x75, 0x51, 0xaf, 0x91, 0x7a, 0x18, 0x83, 0x60, 0x5c, 0x96, 0x4c, 0x4f, 0xfa, 0xaf, 0xb4, 0x4d, 0x72, 0x9d, 0xc5, 0xf5, 0x11, 0xc6, 0x04, 0x5b, 0x29, 0x08, 0x04, 0x40, 0xab, 0x85, 0xc8], | ||
| [0x81, 0x98, 0x50, 0xab, 0xe8, 0x4b, 0x78, 0x49, 0x4f, 0xa1, 0x81, 0xc9, 0x82, 0xe7, 0x97, 0x22, 0xd9, 0x78, 0xfa, 0x93, 0x90, 0xdd, 0x88, 0xc6, 0x7c, 0x95, 0x56, 0xba, 0x92, 0x07, 0xd4, 0x12, 0xa3, 0xa4, 0xc9, 0xf4, 0x70, 0x5b, 0xf0, 0x99, 0x91, 0x89, 0xb4, 0x9b, 0x25, 0x5d, 0x98, 0x83], | ||
| ]; | ||
|
|
||
| let sig_bytes = vec![ | ||
| [0x82, 0xc8, 0x08, 0x03, 0xa3, 0x24, 0x6f, 0x5d, 0x10, 0xb5, 0x19, 0x23, 0xd4, 0x96, 0x7b, 0xef, 0x55, 0x7c, 0xf0, 0x41, 0x6f, 0xa3, 0x06, 0x05, 0x94, 0x9c, 0xaa, 0xc6, 0x27, 0x3b, 0x59, 0x93, 0xd2, 0x6f, 0xef, 0x27, 0x8e, 0xb4, 0x86, 0x75, 0xc6, 0x2b, 0x42, 0x26, 0x6f, 0x9b, 0x01, 0x48, 0x0a, 0x8d, 0xc0, 0x7e, 0xf1, 0x68, 0xed, 0xd3, 0xf1, 0xa9, 0xca, 0xd3, 0x63, 0x30, 0x83, 0xdc, 0xb3, 0xc2, 0xdf, 0x37, 0x27, 0x89, 0xb8, 0x78, 0x71, 0x24, 0x9d, 0xab, 0x38, 0x0b, 0x07, 0x1d, 0xd4, 0x80, 0xd7, 0x42, 0xd0, 0x27, 0xf3, 0x7c, 0x36, 0xf8, 0x9e, 0x63, 0xb5, 0xf1, 0xe0, 0x93], | ||
| [0xb2, 0xca, 0x57, 0x80, 0x16, 0xe8, 0x96, 0x20, 0xb4, 0x94, 0x03, 0xa0, 0x29, 0xad, 0x96, 0x40, 0x27, 0x2c, 0x18, 0x45, 0x5a, 0x5f, 0xdd, 0xc4, 0x0f, 0x99, 0x9b, 0xd7, 0x5c, 0xb9, 0xda, 0xc0, 0x71, 0x61, 0x3c, 0xb5, 0xa6, 0x64, 0xe0, 0xa7, 0x19, 0xa2, 0x29, 0xff, 0x65, 0x51, 0xfd, 0x52, 0x0b, 0x6d, 0x92, 0x96, 0x75, 0x08, 0xd5, 0xf1, 0xf5, 0x8c, 0xd4, 0x14, 0xcb, 0xde, 0x44, 0xc9, 0xe3, 0x91, 0x18, 0x73, 0xb4, 0xa2, 0xe9, 0x54, 0x44, 0x29, 0xb8, 0x67, 0x6f, 0xca, 0x69, 0xcb, 0x97, 0xab, 0xc6, 0xf4, 0x99, 0x16, 0xd0, 0x8c, 0xee, 0xda, 0x42, 0xb0, 0x6b, 0xe3, 0x6f, 0xf4], | ||
| [0xa5, 0x05, 0x73, 0xad, 0x15, 0xdd, 0x23, 0x3d, 0xf0, 0x85, 0x69, 0x33, 0x72, 0x48, 0xba, 0xd3, 0x2c, 0xc2, 0x08, 0xd5, 0xda, 0x64, 0x5a, 0xf8, 0x16, 0x69, 0x43, 0x8d, 0x1a, 0x16, 0x54, 0xd5, 0xba, 0xe7, 0x6f, 0xc8, 0x20, 0x3b, 0x10, 0x8b, 0x91, 0x20, 0x73, 0xbd, 0x89, 0x8b, 0x56, 0xba, 0x0c, 0xee, 0x39, 0x71, 0xcd, 0x68, 0xbe, 0xc0, 0x01, 0x4a, 0x0d, 0x1a, 0x81, 0x4d, 0xbc, 0xe2, 0xc1, 0x52, 0x8e, 0x1d, 0x97, 0xd7, 0x5e, 0xd2, 0x69, 0xdf, 0x05, 0x5a, 0xc0, 0x02, 0xf9, 0x6e, 0x53, 0xfe, 0x3a, 0x58, 0xb1, 0x99, 0x90, 0x9b, 0xac, 0x0c, 0x62, 0xa7, 0x99, 0x92, 0xa5, 0x53], | ||
| ]; | ||
|
|
||
| assert!(count <= 3, "Helper function only supports up to 3 signers"); | ||
|
|
||
| let mut secret_keys = Vec::new(); | ||
| let mut public_keys = Vec::new(); | ||
| let mut signatures = Vec::new(); | ||
|
|
||
| for i in 0..count { | ||
| let sk = SecretKey::<Bls12381G1Impl>::try_from(sk_bytes[i].as_slice()) | ||
| .expect(&format!("Failed to create SecretKey from bytes for signer {}", i + 1)); | ||
| let pk = PublicKey::<Bls12381G1Impl>::try_from(pk_bytes[i].as_slice()) | ||
| .expect(&format!("Failed to create PublicKey from bytes for signer {}", i + 1)); | ||
| let sig = Signature::<Bls12381G1Impl>::try_from(sig_bytes[i].as_slice()) | ||
| .expect(&format!("Failed to create Signature from bytes for signer {}", i + 1)); | ||
|
|
||
| secret_keys.push(sk); | ||
| public_keys.push(pk); | ||
| signatures.push(sig); | ||
| } | ||
|
|
||
| (secret_keys, public_keys, signatures) | ||
| } | ||
|
|
||
| // === Test Case 1: Two Signers === | ||
|
|
||
| #[test] | ||
| fn test_cpp_rust_two_signers() { | ||
| // Use helper function to create test vectors | ||
| let (secret_keys, public_keys, signatures) = create_test_vectors(2); | ||
|
|
||
| // Message | ||
| let message = [0x68, 0x65, 0x6c, 0x6c, 0x6f]; | ||
|
|
||
| // Verify C++ signatures in Rust | ||
| assert!(signatures[0].verify(&public_keys[0], &message).is_ok()); | ||
| assert!(signatures[1].verify(&public_keys[1], &message).is_ok()); | ||
|
|
||
| // Create secure aggregation in Rust using C++ keys | ||
| let secure_agg = AggregateSignature::from_signatures_secure(&signatures, &public_keys).unwrap(); | ||
| let final_sig = match secure_agg { | ||
| AggregateSignature::Basic(sig) => Signature::Basic(sig), | ||
| _ => panic!("Expected Basic scheme"), | ||
| }; | ||
|
|
||
| // Verify secure aggregation | ||
| assert!(final_sig.verify_secure(&public_keys, &message).is_ok()); | ||
| } | ||
|
|
||
| // === Test Case 2: Three Signers === | ||
|
|
||
| #[test] | ||
| fn test_cpp_rust_three_signers() { | ||
| // Use helper function to create test vectors | ||
| let (secret_keys, public_keys, signatures) = create_test_vectors(3); | ||
|
|
||
| // Message | ||
| let message = [0x68, 0x65, 0x6c, 0x6c, 0x6f]; | ||
|
|
||
| // Verify C++ signatures in Rust | ||
| assert!(signatures[0].verify(&public_keys[0], &message).is_ok()); | ||
| assert!(signatures[1].verify(&public_keys[1], &message).is_ok()); | ||
| assert!(signatures[2].verify(&public_keys[2], &message).is_ok()); | ||
|
|
||
| // Create secure aggregation in Rust using C++ keys | ||
| let secure_agg = AggregateSignature::from_signatures_secure(&signatures, &public_keys).unwrap(); | ||
| let final_sig = match secure_agg { | ||
| AggregateSignature::Basic(sig) => Signature::Basic(sig), | ||
| _ => panic!("Expected Basic scheme"), | ||
| }; | ||
|
|
||
| // Verify secure aggregation | ||
| assert!(final_sig.verify_secure(&public_keys, &message).is_ok()); | ||
| } | ||
|
|
||
| // === Test Normal Aggregation (should fail verify_secure) === | ||
|
|
||
| #[test] | ||
| fn test_normal_aggregation_fails_secure_verify() { | ||
| let normal_agg_bytes = [0x8e, 0x40, 0x44, 0x44, 0x23, 0x14, 0xab, 0xb8, 0xbb, 0xdb, 0xd5, 0x06, 0xc6, 0xf7, 0xbe, 0xd1, 0xb7, 0xd0, 0x00, 0x40, 0x77, 0x52, 0x38, 0x4e, 0xca, 0x9e, 0xbf, 0xda, 0x34, 0xd0, 0x61, 0x21, 0x8e, 0x07, 0xf2, 0x5a, 0x0c, 0xbe, 0x42, 0xdc, 0xaf, 0x68, 0xac, 0x9e, 0xa9, 0xe5, 0xa0, 0x80, 0x0a, 0x56, 0x44, 0xa1, 0xf0, 0x20, 0xea, 0xd6, 0xe6, 0x27, 0xb3, 0xe0, 0x34, 0x93, 0x00, 0x02, 0x1c, 0xce, 0x82, 0xc4, 0xc9, 0xa3, 0x3a, 0x0a, 0x88, 0x20, 0x85, 0xc7, 0xc1, 0xeb, 0x93, 0x46, 0xa9, 0x3f, 0x0f, 0xd8, 0x0a, 0xd4, 0xee, 0x6e, 0x7a, 0x6c, 0x2a, 0xd7, 0x7e, 0xd2, 0xc0, 0x2a]; | ||
|
|
||
| // This normal aggregation should fail verify_secure | ||
| let normal_sig = Signature::<Bls12381G1Impl>::try_from(normal_agg_bytes.as_slice()).unwrap(); | ||
|
|
||
| // Get public keys from helper function | ||
| let (_, public_keys, _) = create_test_vectors(2); | ||
|
|
||
| let message = [0x68, 0x65, 0x6c, 0x6c, 0x6f]; | ||
|
|
||
| // Normal aggregation should fail verify_secure | ||
| assert!(normal_sig.verify_secure(&public_keys, &message).is_err()); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Rust cross-compatibility tests are not wired into any Cargo target
This file lives at the repository root and contains #[test] functions, but it is not picked up by any Cargo target. rust-bindings/bls-signatures and rust-bindings/bls-dash-sys have no tests/ directory containing it, no [[test]] entry referencing it, and crucially neither crate depends on blsful (which the file imports on line 3). cargo test therefore never compiles or runs these cases, so the PR's central claim of validated C++/Rust VerifySecure interop is not enforced anywhere — the vectors can rot, the external API can change, and CI stays green. Move the file into rust-bindings/bls-signatures/tests/ (or a new dedicated interop crate), add the blsful/subtle dev-dependencies that it requires, and confirm cargo test actually runs it.
source: ['codex']
🤖 Fix this 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 `cross_compatibility_test_vectors.rs`:
- [BLOCKING] lines 1-115: Rust cross-compatibility tests are not wired into any Cargo target
This file lives at the repository root and contains `#[test]` functions, but it is not picked up by any Cargo target. `rust-bindings/bls-signatures` and `rust-bindings/bls-dash-sys` have no `tests/` directory containing it, no `[[test]]` entry referencing it, and crucially neither crate depends on `blsful` (which the file imports on line 3). `cargo test` therefore never compiles or runs these cases, so the PR's central claim of validated C++/Rust VerifySecure interop is not enforced anywhere — the vectors can rot, the external API can change, and CI stays green. Move the file into `rust-bindings/bls-signatures/tests/` (or a new dedicated interop crate), add the `blsful`/`subtle` dev-dependencies that it requires, and confirm `cargo test` actually runs it.
| // Helper class to access protected methods | ||
| class TestCoreMPL : public CoreMPL { | ||
| public: | ||
| TestCoreMPL() : CoreMPL("BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_NUL_") {} | ||
|
|
||
| using CoreMPL::AggregateSecure; | ||
| using CoreMPL::VerifySecure; | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: Test helper re-exposes protected AggregateSecure/VerifySecure with a raw fLegacy flag
TestCoreMPL derives from CoreMPL solely to promote the protected overloads via using CoreMPL::AggregateSecure; using CoreMPL::VerifySecure;. Those overloads are deliberately kept protected at include/dashbls/schemes.hpp:106-113 so that scheme-specific public wrappers (BasicSchemeMPL/LegacySchemeMPL) can fix the legacy domain-separation flag internally. Bypassing that encapsulation in test code is a footgun: future contributors can copy this pattern and silently flip fLegacy, producing cross-domain-separation 'reference' vectors that other implementations would then trust. The current call sites pass false and the file isn't even built today, but if this generator is wired into CMake (per the related finding) it should drive the secure path through the public scheme API rather than re-exposing protected internals.
source: ['claude']
🤖 Fix this 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 `src/test_full_compatibility.cpp`:
- [SUGGESTION] lines 25-32: Test helper re-exposes protected AggregateSecure/VerifySecure with a raw fLegacy flag
`TestCoreMPL` derives from `CoreMPL` solely to promote the protected overloads via `using CoreMPL::AggregateSecure; using CoreMPL::VerifySecure;`. Those overloads are deliberately kept protected at `include/dashbls/schemes.hpp:106-113` so that scheme-specific public wrappers (`BasicSchemeMPL`/`LegacySchemeMPL`) can fix the legacy domain-separation flag internally. Bypassing that encapsulation in test code is a footgun: future contributors can copy this pattern and silently flip `fLegacy`, producing cross-domain-separation 'reference' vectors that other implementations would then trust. The current call sites pass `false` and the file isn't even built today, but if this generator is wired into CMake (per the related finding) it should drive the secure path through the public scheme API rather than re-exposing protected internals.
3 participants
Summary
Key Components
Files Added
src/test_verifysecure_vectors.cpp- Core VerifySecure test vector generationsrc/test_cross_compatibility.cpp- Cross-platform compatibility validationsrc/test_full_compatibility.cpp- Complete compatibility test suitecross_compatibility_test_vectors.rs- Generated Rust test vectorsfresh_test_vectors.rs- Fresh test vectors for validationsrc/CMakeLists.txt- Build configuration for new test filesPurpose
This PR provides the C++ side test vectors and validation tools that complement the VerifySecure implementation in the agora-blsful repository. Together, they ensure full cross-platform compatibility between the C++ and Rust BLS signature implementations.
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores
Documentation