fix: mnlist engine test failures #96
Conversation
WalkthroughUpdated blsful dependency revision in Cargo.toml and refactored quorum aggregated signature verification to use a unified blsful Signature conversion (try_into) and verify_secure over collected public keys, removing legacy aggregated-signature branching. Error mapping updated; public APIs unchanged. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant QQE as QualifiedQuorumEntry
participant Keys as Operator Public Keys
participant Sig as blsful::Signature
participant BLS as verify_secure
Caller->>QQE: verify_aggregated_commitment_signature(operator_keys)
QQE->>Keys: Deserialize per-key (Legacy/Modern)
QQE->>Sig: Convert aggregated bytes via try_into()
QQE->>BLS: signature.verify_secure(Keys, message)
BLS-->>QQE: Ok or Error
QQE-->>Caller: Result<(), QuorumValidationError>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
dash/src/sml/quorum_entry/validation.rs (3)
4-6: Unused import:verify_secure_basic_with_modeLine 5 imports
verify_secure_basic_with_modebut it's no longer used in the main code after the refactoring to useverify_secure. It's only used in test code.Move this import to the test module where it's actually used:
-use blsful::verify_secure_basic_with_mode; use blsful::{Bls12381G2Impl, PublicKey, SerializationFormat, Signature, SignatureSchemes};And add it to the benchmarks module:
mod benchmarks { use super::super::*; use blsful::{ - Bls12381G2Impl, PublicKey, Signature, SignatureSchemes, verify_secure_basic_with_mode, + Bls12381G2Impl, PublicKey, Signature, SignatureSchemes, + verify_secure_basic_with_mode, };
393-396: Potential panic in test codeThe benchmark test assumes the signature will always be
Basicvariant and will panic otherwise. While this is test code, it would be more robust to handle this gracefully.Consider using a more graceful error handling:
- let inner_sig = match sig { - Signature::Basic(s) => s, - _ => panic!("Expected Basic signature"), - }; + let inner_sig = match sig { + Signature::Basic(s) => s, + _ => { + eprintln!("Skipping benchmark: Expected Basic signature"); + return; + } + };
410-421: Consider using the newverify_secureAPI in benchmarksThe benchmark is still using the old
verify_secure_basic_with_modeAPI while the main code has been refactored to use the simplerverify_secureAPI. Consider updating the benchmark to test the actual code path being used.Update the benchmark to use the same API as the production code:
- for _ in 0..iterations { - let _ = verify_secure_basic_with_mode::<Bls12381G2Impl, _>( - &operator_keys, - inner_sig.clone(), - msg, - SerializationFormat::Modern, - ); - } + // Create a proper Signature for benchmarking + let sig = Signature::<Bls12381G2Impl>::from_bytes_with_mode( + &sig_bytes, + SignatureSchemes::Basic, + SerializationFormat::Modern, + ).unwrap(); + + for _ in 0..iterations { + let _ = sig.verify_secure(&operator_keys, msg); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dash/Cargo.toml(1 hunks)dash/src/sml/quorum_entry/validation.rs(2 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). (5)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (hashes_sha512)
🔇 Additional comments (2)
dash/Cargo.toml (1)
69-69: Dependency update looks good.The blsful dependency revision has been updated to incorporate the fix for test failures as mentioned in the PR description. This aligns with the PR objective of fixing mnlist engine test failures.
dash/src/sml/quorum_entry/validation.rs (1)
65-71: Simplified signature verification implementation.Good refactoring! The new implementation is cleaner and more straightforward:
- Uses
try_into()for direct conversion toSignature<Bls12381G2Impl>- Calls
verify_securedirectly without manual format detection- Properly maps errors to domain-specific error types
This change aligns well with the PR's goal of fixing test failures through the updated blsful dependency.
On this branch, previously all these tests were failing. This was overall fixed via dashpay/agora-blsful#3
Summary by CodeRabbit
Bug Fixes
Refactor
Chores