feat: instant send and chain lock verification with improved quorum in lists verification #56
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes refine various parts of the system by reorganizing and updating import statements and formatting, while introducing new public methods that compute signing IDs for both chain and instant locks. New hash types and associated methods for quorum ordering and signing have been added, and several methods for quorum selection and message verification within the masternode list engine have been implemented or updated. Additionally, some legacy modules and methods related to locking have been removed, while error handling via a new message verification error type has been introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Module as ChainLock/InstantLock
participant Engine as Signing Engine
Client->>Module: Call sign_id(quorum_type, quorum_hash, precomputed_request_id)
Module->>Engine: Process input, generate request ID if needed
Engine-->>Module: Return QuorumSigningSignId
Module-->>Client: Return Result(QuorumSigningSignId or io::Error)
sequenceDiagram
participant Lock as InstantLock/ChainLock
participant MLE as MasternodeListEngine
participant ML as MasternodeList
participant QEntry as QualifiedQuorumEntry
participant Err as MessageVerificationError
Lock->>MLE: Initiate lock verification (verify_is_lock/verify_chain_lock)
MLE->>ML: Retrieve quorum entries (using quorum_for_request)
ML-->>MLE: Return quorum candidate
MLE->>QEntry: Request signature verification with public key
QEntry-->>MLE: Return verification result
alt Verification fails
MLE->>Err: Return detailed MessageVerificationError
else Verification succeeds
MLE-->>Lock: Verification successful
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
QuantumExplorer
changed the title
![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 (9)
dash/src/sml/masternode_list/quorum_helpers.rs (1)
12-25: Added quorum_for_request method for deterministic quorum selection.This method selects a quorum for a given request ID using
QuorumOrderingHashfor deterministic ordering. The implementation is clean and handles error cases appropriately.The code correctly:
- Fetches quorums of the specified type
- Selects the "minimum" quorum based on the ordering hash
- Returns appropriate errors when quorums are not found
There is a slight duplication in error handling - both the initial check and the min_by_key fallback use the same error type. Consider refactoring to improve clarity:
- let quorums_of_type = self - .quorums - .get(&quorum_type) - .ok_or(MessageVerificationError::MasternodeListHasNoQuorums(self.known_height))?; - quorums_of_type - .values() - .min_by_key(|quorum| QuorumOrderingHash::create(&quorum.quorum_entry, request_id)) - .ok_or(MessageVerificationError::MasternodeListHasNoQuorums(self.known_height)) + let quorums_of_type = self + .quorums + .get(&quorum_type) + .ok_or_else(|| MessageVerificationError::MasternodeListHasNoQuorums(self.known_height))?; + + if quorums_of_type.is_empty() { + return Err(MessageVerificationError::MasternodeListHasNoQuorums(self.known_height)); + } + + quorums_of_type + .values() + .min_by_key(|quorum| QuorumOrderingHash::create(&quorum.quorum_entry, request_id)) + .ok_or_else(|| MessageVerificationError::MasternodeListHasNoQuorums(self.known_height))dash/src/sml/masternode_list_engine/rotated_quorum_construction.rs (2)
58-58: Consider removing double-reference for quorums.
Using&'a [&'a QualifiedQuorumEntry]may be more complex than necessary.Example diff:
- quorums: &'a [&'a QualifiedQuorumEntry], + quorums: &'a [QualifiedQuorumEntry],
133-152: Reduce duplicated logic for previous rotation checks.
Lines 133–152 re-implement similar logic used above. Consider abstracting it into a helper to avoid duplication.dash/src/sml/message_verification_error.rs (1)
55-65: Conversion fromQuorumValidationErroris broad.Most variants are mapped cleanly, but be cautious that other
QuorumValidationErrorvariants collapse toGeneric, which may reduce granularity of error handling.dash/src/sml/masternode_list_engine/message_request_verification.rs (1)
99-103: Potential out-of-range panic on quorum index.Line 101 uses
expect(...). Although DIP 24 logic should guarantee a valid index, consider gracefully returning an error instead of panicking if corrupted data is ever encountered.dash/src/sml/masternode_list_engine/mod.rs (4)
39-46: Consider replacing tuple with a dedicated struct or type alias.
Defining a tuple of(BTreeSet<CoreBlockHeight>, BLSPublicKey, LLMQEntryVerificationStatus)might reduce readability in the long run. A named struct or type alias could enhance clarity and maintainability.-pub quorum_statuses: BTreeMap< - LLMQType, - BTreeMap< - QuorumHash, - (BTreeSet<CoreBlockHeight>, BLSPublicKey, LLMQEntryVerificationStatus), - >, ->, +pub struct QuorumTrackingInfo { + pub heights: BTreeSet<CoreBlockHeight>, + pub public_key: BLSPublicKey, + pub status: LLMQEntryVerificationStatus, +} + +pub quorum_statuses: BTreeMap<LLMQType, BTreeMap<QuorumHash, QuorumTrackingInfo>>,
104-123: Readable logic for optional block hash filtering.
Usingonly_return_block_hashes_with_missing_masternode_lists_from_engineis verbose but clear. You might consider a more concise naming.
125-149: Opportunity to reduce duplication.
This function’s filtering logic is very similar tolatest_masternode_list_non_rotating_quorum_hashes. A helper might DRY these methods.- // Repeated logic for filtering based on missing masternode lists + // Consider refactoring to a shared helper or generic method
770-770: Consider more explicit error handling instead of double unwrap.
Although logically guaranteed here, changing.unwrap()calls to.expect("message")makes debugging easier if data is ever missing unexpectedly.-results.get_mut(quorum_type).unwrap().remove(quorum_hash).unwrap() +results + .get_mut(quorum_type) + .expect("missing quorum_type in results") + .remove(quorum_hash) + .expect("missing quorum_hash in results map")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
dash/src/blockdata/transaction/special_transaction/provider_registration.rs(3 hunks)dash/src/ephemerealdata/chain_lock.rs(2 hunks)dash/src/ephemerealdata/instant_lock.rs(2 hunks)dash/src/hash_types.rs(6 hunks)dash/src/network/message_sml.rs(1 hunks)dash/src/sml/address.rs(1 hunks)dash/src/sml/llmq_entry_verification.rs(1 hunks)dash/src/sml/masternode_list/builder.rs(1 hunks)dash/src/sml/masternode_list/is_lock_methods.rs(0 hunks)dash/src/sml/masternode_list/masternode_helpers.rs(1 hunks)dash/src/sml/masternode_list/mod.rs(1 hunks)dash/src/sml/masternode_list/quorum_helpers.rs(1 hunks)dash/src/sml/masternode_list_engine/helpers.rs(1 hunks)dash/src/sml/masternode_list_engine/message_request_verification.rs(1 hunks)dash/src/sml/masternode_list_engine/mod.rs(16 hunks)dash/src/sml/masternode_list_engine/rotated_quorum_construction.rs(2 hunks)dash/src/sml/masternode_list_engine/validation.rs(2 hunks)dash/src/sml/masternode_list_entry/hash.rs(1 hunks)dash/src/sml/masternode_list_entry/mod.rs(1 hunks)dash/src/sml/masternode_list_entry/qualified_masternode_list_entry.rs(1 hunks)dash/src/sml/message_verification_error.rs(1 hunks)dash/src/sml/mod.rs(1 hunks)dash/src/sml/quorum_entry/helpers.rs(0 hunks)dash/src/sml/quorum_entry/mod.rs(0 hunks)dash/src/sml/quorum_entry/validation.rs(2 hunks)hashes/src/util.rs(2 hunks)
💤 Files with no reviewable changes (3)
- dash/src/sml/quorum_entry/mod.rs
- dash/src/sml/quorum_entry/helpers.rs
- dash/src/sml/masternode_list/is_lock_methods.rs
✅ Files skipped from review due to trivial changes (10)
- dash/src/sml/masternode_list/masternode_helpers.rs
- dash/src/network/message_sml.rs
- dash/src/sml/masternode_list_entry/hash.rs
- dash/src/sml/masternode_list/builder.rs
- dash/src/sml/masternode_list_entry/qualified_masternode_list_entry.rs
- dash/src/sml/llmq_entry_verification.rs
- dash/src/sml/mod.rs
- dash/src/sml/masternode_list/mod.rs
- dash/src/sml/masternode_list_entry/mod.rs
- dash/src/blockdata/transaction/special_transaction/provider_registration.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: Tests (nightly, true, false, true)
- GitHub Check: Tests (beta, true, false)
- GitHub Check: Tests (stable, true, true, false)
- GitHub Check: Tests (nightly, true, false, true)
- GitHub Check: Tests (beta, true, false)
- GitHub Check: Tests (stable, true, true, false)
🔇 Additional comments (46)
dash/src/sml/address.rs (1)
51-52: Good addition - restores test functionality.Adding
use super::*;is necessary to give the test module access to theEncodableandDecodabletrait implementations forSocketAddrdefined in the parent module. Without this import, the tests would fail to compile since they rely on methods likeconsensus_encodeandconsensus_decode.hashes/src/util.rs (2)
323-327: Added From trait implementation for byte array conversion.This is a helpful addition, providing a cleaner and more idiomatic way to construct hash newtypes directly from byte arrays. The implementation correctly delegates to the underlying hash type's
from_byte_arraymethod.
458-462: Same implementation pattern for non-orderable hash newtypes.The same
From<[u8; LEN]>trait implementation is correctly added to thehash_newtype_no_ordmacro, maintaining consistency between both macro variants.dash/src/ephemerealdata/chain_lock.rs (2)
16-19: Updated imports for quorum-related functionality.Good organization of imports, adding the necessary types (
QuorumSigningSignId,LLMQType) for the new functionality.
56-70: New sign_id method for chain lock verification.This method creates a
QuorumSigningSignIdby combining the quorum type, quorum hash, request ID, and block hash. The implementation correctly handles the case where a precomputed request ID is not provided.One potential issue: The code unwraps the precomputed_request_id without checking if it's None first, which could panic if None is passed but self.request_id() fails.
- engine.input(precomputed_request_id.unwrap_or(self.request_id()?).as_byte_array()); + let request_id = match precomputed_request_id { + Some(id) => id, + None => self.request_id()?, + }; + engine.input(request_id.as_byte_array());dash/src/sml/masternode_list_engine/validation.rs (2)
45-48: Changed parameter type to accept references instead of values.This change improves efficiency by accepting references to quorum entries (
&[&QualifiedQuorumEntry]) rather than requiring owned or copied values. This is a good optimization that reduces unnecessary copying, especially when working with quorum data that might be large.
82-85: Consistent parameter type change for validation statuses method.The parameter type is updated consistently with the previous method, maintaining API coherence across related functions.
dash/src/sml/quorum_entry/validation.rs (4)
3-3: Great addition for double-hash usage.
The import ofsha256daligns well with the need for double-hash operations. No issues found.
5-5: BLSSignature import is appropriate.
ImportingBLSSignaturefrombls_sig_utilsis consistent with your BLS usage.
7-7: MessageVerificationError import fits the new error handling.
IncorporatingMessageVerificationErrorhere provides a clean way to unify signature verification errors.
115-164:❓ Verification inconclusive
Confirm the 32-byte digest usage in
verify_message_digest.
The function properly converts the stored public key and supplied signature intoblsfultypes before verification. Ensure all callers always provide a 32-byte digest for consistency and correctness.Use this script to check references to
verify_message_digestand review their arguments:
🏁 Script executed:
#!/bin/bash # Attempt to locate function calls to verify_message_digest ast-grep --pattern $'verify_message_digest($_, $_)'Length of output: 52
Action: Verify Consistent 32-Byte Digest Usage Across Call Sites
Theverify_message_digestfunction explicitly requires a 32-byte digest (i.e.,[u8; 32]). Automated searches for its call sites (usingast-grep) produced no output, so we couldn’t automatically confirm that every caller adheres to this expectation. Please manually verify that all invocations of this function in the codebase indeed supply a 32-byte digest to avoid potential signature verification issues. Note that the conversion of the quorum public key and signature to their respectiveblsfultypes is correct.dash/src/sml/masternode_list_engine/helpers.rs (1)
1-49: Methodmasternode_lists_around_heightis well-designed.
The search logic usingrange(..=core_block_height).rev()andrange(core_block_height + 1..)is clean, with clear doc comments and efficient lookups in theBTreeMap. No changes needed.dash/src/sml/message_verification_error.rs (2)
16-49: Well-structured error enum for message verification.All variants are clearly named and convey specific error scenarios, improving maintainability and debugging.
51-53: Smooth conversion fromStringto error.This facilitates broad error handling without complicated unwraps.
dash/src/ephemerealdata/instant_lock.rs (3)
15-16: Imports are coherent and well-organized.They gather relevant types from the crate for hashing, BLS signatures, and the new sign ID logic, matching the file’s functionality.
Also applies to: 17-18
55-74: Request ID computation seems solid.The prefix-based VarInt encoding logic for “islock” plus each outpoint is robust. Confirm that downstream code consistently accounts for this encoding approach.
76-91: Hash-based sign ID calculation looks correct.The function consolidates the quorum type, quorum hash, request ID, and txid into a single digest. This is consistent and should ensure unique sign IDs.
dash/src/hash_types.rs (5)
140-140: New typeQuorumSigningSignIdis well-defined.It follows the established pattern for custom hash types, and ensures consistent usage throughout the codebase.
156-157:QuorumOrderingHashnewtype introduced.This allows easy sorting of quorums by reversing bytes. Ensure reversing logic consistently matches other parts of the code where reversed block hashes are expected.
171-181: Ordering implementations mirrorScoreHashapproach.Reversing byte arrays for comparison is a standard technique in this codebase, so it remains consistent. Just keep in mind that reversing is an in-place operation in standard Rust if used directly.
Also applies to: 189-193
199-201:CycleHashre-aliased toBlockHash.Reintroducing it maintains semantic clarity while reusing existing tooling for block hashes.
287-312: ConstructingQuorumOrderingHashwith reversed quorum hash.Be sure it aligns with the rest of the system’s endianness assumptions. The approach is consistent with other reversed-hash logic, but thorough test coverage is recommended.
dash/src/sml/masternode_list_engine/message_request_verification.rs (4)
11-30: Quorum retrieval for Instant Locks is clear.Errors are thrown if the cycle hash is missing or empty, providing an explicit failure path. This is good for maintainability.
107-157: Instant Lock verification is thorough.It computes both request and sign IDs, then defers to
verify_message_digest. This consistent approach prevents mixing up the hashed inputs.
239-313: Chain Lock verification uses fallback logic.First it tries the “before” list, then, if needed, the “after” list. This is aligned with DIP 24. Confirm that the fallback scenario is properly tested to catch any edge cases where data from both lists might overlap.
315-346: Helper function for chain lock verification is modular.Separating
verify_chain_lock_with_masternode_listclarifies the logic and fosters reuse, which is a good design choice.dash/src/sml/masternode_list_engine/mod.rs (20)
1-2: No issues with new modules.
These new module declarations are straightforward and appear consistent with the existing file structure.
15-15: Imports look fine.
The addedBLSPublicKeyandBLSSignatureimports are appropriate.
20-20: Good addition of the LLMQ entry verification status import.
No concerns regarding this new import.
50-63: Default implementation is consistent.
Ensuring all fields inMasternodeListEngineare correctly defaulted is good practice. No issues here.
66-66: Clear constructor for specifying network.
This helper neatly allows different network defaults while leveraging the existingDefaultimpl.
82-83: No issues with initializing new fields.
Your updated initializer properly sets the new fields to defaults.
189-190: Boolean flag usage looks fine.
Theshould_request_for_previous_validationvariable is clear and matches usage below.
306-306: Chain lock fetching logic is straightforward.
Fetching chain lock signatures conditionally aligns with the gating callback.
603-642: Automatic quorum verification logic is robust.
This block updates previously unverified quorums and synchronizes status across relevant heights. Nesting can get complex, but the approach is valid.
643-660: Deferred verification path.
This else branch gracefully defers verifying quorums by consistently syncing statuses.
674-687: Maintainingquorum_statusesin no-validation builds.
This code helps track height usage of each quorum for scenarios without full validation. No concerns.
698-702: Definition of a dedicated non-rotating verification method is clear.
Setting up a separate entry point for verifying non-rotating quorums is logical.
703-705: Error handling for absent masternode list is clean.
Using the newerelseblock syntax is concise and understandable.
708-708: Loop structure is straightforward.
Beginning iteration over quorums is standard.
714-715: Intermediate map usage looks good.
Collecting results in a temporary map is an effective pattern for verification.
721-722: Collecting updates in a vector is a good approach.
This helps avoid multiple mutable borrows on the same data.
724-726: Same style of error check.
Matches the approach used elsewhere.
728-728: No issue in iterating mutable quorums.
The usage is consistent with prior patterns.
733-734: Quorum status retrieval is consistent.
Looks correct for lookup or insertion inquorum_statuses.
736-761: Rotation logic overall is coherent.
Identifying the cycle hash from index=0 quorums and updating statuses accordingly is clearly delineated.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
dash/src/hash_types.rs (3)
139-140: Ensure test coverage for new hash type.
QuorumSigningSignIdis a new hash type. Although it appears structurally consistent, consider adding tests to verify creation, usage, and serialization/deserialization.
171-182: Commented-outOrdtrait.The
Ordimplementation forQuorumOrderingHashis fully commented out. If it’s no longer needed, consider removing for clarity, or restore it if ordering functionality is required.
189-194: Commented-outPartialOrdtrait.Similar to the
Ordtrait, this partial ordering code is commented out. Remove if obsolete or reinstate if sorting logic is intended.dash/src/sml/masternode_list_engine/message_request_verification.rs (3)
146-152: Improve error handling for sign_id()Converting errors to strings with
map_err(|e| e.to_string())?loses error context and type information. Consider defining and using a more specific error variant inMessageVerificationError.let sign_id = instant_lock .sign_id( quorum.quorum_entry.llmq_type, quorum.quorum_entry.quorum_hash, Some(request_id), ) - .map_err(|e| e.to_string())?; + .map_err(|e| MessageVerificationError::SignIdCreationFailed(e.to_string()))?;
467-467: Uncomment or remove the commented-out assertionThere's a commented-out assertion in the test. Either uncomment it if it's a valid test case, or remove it if it's no longer needed. Commented-out code can lead to confusion about the intended behavior.
1-471: Overall: Implement consistent error handling approachThroughout the file, various methods handle errors differently:
- Some convert errors to strings with
map_err(|e| e.to_string())?- Some use
expect()orunwrap()- Some properly use the
MessageVerificationErrorenumConsider refactoring to use a consistent approach, preferably using appropriate variants in the
MessageVerificationErrorenum to provide more context and better error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dash/src/hash_types.rs(6 hunks)dash/src/sml/masternode_list_engine/message_request_verification.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Tests (nightly, true, false, true)
- GitHub Check: Tests (beta, true, false)
- GitHub Check: Tests (stable, true, true, false)
- GitHub Check: Tests (nightly, true, false, true)
- GitHub Check: Tests (beta, true, false)
- GitHub Check: Tests (stable, true, true, false)
🔇 Additional comments (6)
dash/src/hash_types.rs (6)
80-80: Import looks good.The import of
QuorumEntryis consistent with its usage in the newly introducedQuorumOrderingHash::createmethod. No further concerns.
151-151: Confirm naming consistency for new hash type.
QuorumOrderingHashfollows the library’s naming pattern. Ensure all references to this type reflect intended usage and do not conflict with the commented-out ordering logic below.
156-156: No functional changes detected.This line change seems to be whitespace or formatting only and does not affect functionality.
200-201: CycleHash presence conflicts with summary.The AI-generated summary stated the
CycleHashtype was removed. However, you’re introducingpub type CycleHash = BlockHash;. Please verify which approach is correct and remove or rename this as needed.Likely an incorrect or invalid review comment.
228-228: Extendingimpl_hashencode!is consistent.Adding
QuorumOrderingHashto the hashing encode/decode macro aligns with the other hash types and ensures consistent serialization.
287-312: Check range and usage for LLMQ type.The
createmethod forQuorumOrderingHashdirectly casts thequorum.llmq_typetou8. Ifllmq_typecan exceed that range, consider validation or an explicit enum. Also ensure test coverage for these methods.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
dash/src/hash_types.rs (4)
139-140: Consider adding hex utility methods forQuorumSigningSignId.Other newtypes (e.g.,
ScoreHash,Txid,ProTxHash) providefrom_hexandto_hexmethods. Consistency might be improved by offering the same utilities forQuorumSigningSignId, unless they are intentionally unnecessary.
170-190: Uncomment or remove theOrdandPartialOrdimplementations forQuorumOrderingHash.These implementations are currently commented out, but the AI-generated summary mentions that ordering support was introduced. If you intend to use sorted
QuorumOrderingHashstructures, you may want to uncomment and finalize these traits.
195-196: Fix minor documentation typo.Change "A hash used to identity a cycle" to "A hash used to identify a cycle" to maintain clarity in the comment.
- /// A hash used to identity a cycle + /// A hash used to identify a cycle
281-282: Correct the doc comments referencingScoreHashinstead ofQuorumOrderingHash.Lines 281–282 and 286–287 describe creating and converting a “ScoreHash” but should refer to “QuorumOrderingHash” to match the actual type. Update these doc comments to avoid confusion.
- /// Create a ScoreHash from a string + /// Create a QuorumOrderingHash from a string - /// Convert a ScoreHash to a string + /// Convert a QuorumOrderingHash to a stringAlso applies to: 286-287
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dash/src/hash_types.rs(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Tests (nightly, true, false, true)
- GitHub Check: Tests (beta, true, false)
- GitHub Check: Tests (stable, true, true, false)
- GitHub Check: Tests (nightly, true, false, true)
- GitHub Check: Tests (beta, true, false)
- GitHub Check: Tests (stable, true, true, false)
| quorums_of_type | ||
| .values() | ||
| .min_by_key(|quorum| QuorumOrderingHash::create(&quorum.quorum_entry, request_id)) | ||
| .ok_or(MessageVerificationError::MasternodeListHasNoQuorums(self.known_height)) |
dash/src/sml/masternode_list_engine/message_request_verification.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Ivan Shumkov <[email protected]>
Summary by CodeRabbit
New Features
Refactor
Chores