test: Add VerifySecure cross-compatibility test vectors for Rust implementation #111
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.
2 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