feat: Use chain lock sigs in Masternode list diffs instead of requesting them #64
Conversation
WalkthroughThis pull request updates several Cargo manifest files by bumping the package version and changing the version specification for the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant MLE as MasternodeListEngine
participant BC as BlockContainer
participant QP as QuorumProcessor
participant CLV as ChainLockVerifier
C->>MLE: Call apply_diff(diff, diff_end_height, previous_chain_lock_sigs)
MLE->>BC: feed_block_height(height, block_hash)
MLE->>QP: Process each quorum from diff
alt Rotating quorum detected
QP->>CLV: Validate using previous_chain_lock_sigs
CLV-->>QP: Return verified chain lock signature
end
QP-->>MLE: Return updated quorum entries
MLE-->>C: Return (Updated MasternodeList, Optional chain lock signature)
sequenceDiagram
participant Caller
participant LLMQ as LLMQModifierType
Caller->>LLMQ: new_quorum_modifier_type(llmq_type, work_block_hash, work_block_height, best_cl_signature, network)
LLMQ-->>Caller: Returns LLMQModifierType::CoreV20 variant using best_cl_signature
Poem
✨ Finishing Touches
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:
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 (
|
![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 (11)
dash/src/sml/quorum_entry/validation.rs (1)
49-53: Consider updating doc comment or reintroducing a log statement.Though line 27 states “Prints an error message if a public key fails to parse,” the actual log statement is now commented out. If you intend to keep logs removed, consider removing or adjusting the doc comment. Otherwise, a debug or warn-level log might help diagnose parsing issues.
dash/src/sml/quorum_entry/qualified_quorum_entry.rs (1)
11-18: Add optional documentation for the new enum.While introducing
VerifyingChainLockSignaturesType, consider adding a brief comment explaining when to useRotatingvsNonRotatingto help maintainers quickly grasp the variant logic.dash/src/sml/quorum_entry/quorum_modifier_type.rs (1)
110-110: Streamlined implementation removes potential error pathDirectly using the provided signature rather than performing a lookup simplifies the code and removes a potential failure point. The function's documentation (line 96-97) mentions a potential error that no longer exists, which should be updated.
Consider updating the function documentation to remove the mention of
Err(QuorumValidationError::RequiredChainLockNotPresent)since this error case is no longer possible with the new implementation.dash/src/sml/masternode_list/apply_diff.rs (3)
42-43: Document the new parameter and return type.The signature now takes
previous_chain_lock_sigsand returns a tuple. Please update the function's doc comment to describe these new elements.
81-90: Watch out for repeated indexes in index_set.If a single index appears multiple times in different signature objects, the last assignment will overwrite the slot. Consider validating or ignoring duplicates if that scenario is not intended.
97-97: Optional: Rename for clarity.
rotating_sigcould be renamed torotating_chain_lock_sigor something more descriptive, reflecting its usage.dash/src/sml/masternode_list_engine/rotated_quorum_construction.rs (3)
82-82: Block height check repeated.Same logic as lines 33-34. Consolidation might be possible, but it's consistent to keep it inline.
163-164: Repeated block height lookup.Same pattern. Possibly refactor if needed, but no immediate issues.
197-197: Parameter name clarity.
chain_lock_sigsimplies multiple signatures. Provide a short docstring or comment about its guaranteed length being 4 for rotating quorums.dash/src/sml/masternode_list_engine/mod.rs (2)
32-40: New BTreeMapBlockContainer structure.This struct is a neat approach to storing block heights and hashes. Just ensure concurrency is handled or not required.
653-654: New param for previous_chain_lock_sigs.Doc comment updated usage recommended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
dash/tests/data/test_DML_diffs/block_container_2240504.datis excluded by!**/*.datdash/tests/data/test_DML_diffs/mn_list_diff_0_2227096.binis excluded by!**/*.bindash/tests/data/test_DML_diffs/mn_list_diff_2227096_2241332.binis excluded by!**/*.bindash/tests/data/test_DML_diffs/mnlistdiffs_2240504.datis excluded by!**/*.datdash/tests/data/test_DML_diffs/qrinfo_2240504.datis excluded by!**/*.dat
📒 Files selected for processing (18)
Cargo.toml(1 hunks)dash/Cargo.toml(1 hunks)dash/src/network/message_qrinfo.rs(2 hunks)dash/src/sml/error.rs(1 hunks)dash/src/sml/masternode_list/apply_diff.rs(4 hunks)dash/src/sml/masternode_list/from_diff.rs(2 hunks)dash/src/sml/masternode_list/merkle_roots.rs(0 hunks)dash/src/sml/masternode_list_engine/message_request_verification.rs(6 hunks)dash/src/sml/masternode_list_engine/mod.rs(16 hunks)dash/src/sml/masternode_list_engine/non_rotated_quorum_construction.rs(3 hunks)dash/src/sml/masternode_list_engine/rotated_quorum_construction.rs(13 hunks)dash/src/sml/masternode_list_engine/validation.rs(0 hunks)dash/src/sml/quorum_entry/qualified_quorum_entry.rs(3 hunks)dash/src/sml/quorum_entry/quorum_modifier_type.rs(1 hunks)dash/src/sml/quorum_entry/validation.rs(1 hunks)dash/src/sml/quorum_validation_error.rs(1 hunks)hashes/Cargo.toml(1 hunks)rpc-json/Cargo.toml(1 hunks)
💤 Files with no reviewable changes (2)
- dash/src/sml/masternode_list_engine/validation.rs
- dash/src/sml/masternode_list/merkle_roots.rs
🧰 Additional context used
🧬 Code Definitions (5)
dash/src/sml/quorum_entry/qualified_quorum_entry.rs (2)
dash/src/sml/masternode_list_engine/rotated_quorum_construction.rs (2)
sml(29-70)sml(72-128)dash/src/sml/masternode_list_engine/non_rotated_quorum_construction.rs (1)
sml(30-56)
dash/src/network/message_qrinfo.rs (4)
dash/src/consensus/serde.rs (1)
deserialize(603-614)dash/src/consensus/encode.rs (3)
deserialize(236-245)deserialize(1427-1427)deserialize(1452-1452)dash/src/network/message.rs (1)
deserialize(829-829)dash/src/network/constants.rs (1)
magic(127-135)
dash/src/sml/masternode_list_engine/non_rotated_quorum_construction.rs (2)
dash/src/sml/masternode_list_engine/rotated_quorum_construction.rs (2)
sml(29-70)sml(72-128)dash/src/sml/quorum_entry/quorum_modifier_type.rs (1)
new_quorum_modifier_type(102-114)
dash/src/sml/masternode_list_engine/message_request_verification.rs (2)
dash/src/ephemerealdata/chain_lock.rs (2)
request_id(41-54)sign_id(56-70)dash/src/ephemerealdata/instant_lock.rs (2)
request_id(55-74)sign_id(77-91)
dash/src/sml/masternode_list/apply_diff.rs (2)
dash/src/sml/masternode_list_engine/rotated_quorum_construction.rs (2)
sml(29-70)sml(72-128)dash/src/sml/masternode_list_engine/non_rotated_quorum_construction.rs (1)
sml(30-56)
🔇 Additional comments (69)
Cargo.toml (1)
6-6: Version bump appropriately reflects the feature addition.The version increment from "0.38.0" to "0.39.0" follows semantic versioning practices correctly, as this PR introduces a new feature (using chain lock sigs in Masternode list diffs).
rpc-json/Cargo.toml (1)
30-30: Exact versioning for bincode dependency is appropriate.Using the exact version specifier (
=2.0.0-rc.3) rather than allowing compatible versions is a good practice, especially for release candidate dependencies where API stability isn't guaranteed. This prevents potential compatibility issues across different builds.dash/Cargo.toml (1)
64-64: Consistent dependency versioning maintained across the project.The bincode dependency is now pinned to the exact version "=2.0.0-rc.3", matching the changes in the other Cargo.toml files. This consistency helps ensure deterministic builds across all components.
hashes/Cargo.toml (1)
41-41: Consistent exact versioning for bincode across all packages.The pinned version requirement for bincode here completes the consistent approach across all packages in the workspace. This coordinated change helps maintain compatibility between interdependent components.
dash/src/sml/error.rs (1)
47-53: Appropriate error variants added for chain lock signature validation.These new error variants directly support the PR objective of using chain lock signatures from MNListDiff messages. They provide more granular error handling for signature validation scenarios:
InvalidIndexInSignatureSet- Provides specific information about invalid indices in quorum signature setsIncompleteSignatureSet- Clearly indicates when signature sets are missing required signaturesThis improves error reporting and debugging when working with chain lock signatures as described in DIP 29.
dash/src/sml/quorum_validation_error.rs (1)
42-47: Validate thatu8is sufficient for the rotated chain lock signature index.Storing a height offset in an 8-bit field might risk overflow if differences can exceed 255. Please confirm or consider broadening the type if larger offsets are possible.
dash/src/sml/quorum_entry/qualified_quorum_entry.rs (4)
1-1: No concerns with the new import.
8-9: Feature-gated bincode usage looks correct.
37-38: Field introduction is appropriate.This optional field aligns with the stated design of storing chain lock signatures for quorums in different modes.
52-52: Setting this toNoneby default is logical.No issues here; the default initialization prevents uninitialized usage.
dash/src/sml/masternode_list/from_diff.rs (6)
3-3: ImportingBLSSignatureis straightforward.
10-12: ImportingQualifiedQuorumEntryandVerifyingChainLockSignaturesType.
91-93: Creating the lookup vector is a solid approach.Initializing a
Vec<Option<&BLSSignature>>with the same length asnew_quorumsensures consistent indexing.
94-103: Populating signature slots is well-structured.Index validation and assigning the reference is straightforward. This helps prevent out-of-bounds errors.
105-108: Ensuring all slots are filled improves consistency.Returning an error early if any signature is missing fosters robust error handling.
110-134: Confirm whether rotating signatures are expected here.The code always sets
verifying_chain_lock_signatureto theNonRotatingvariant. If you intend to handleRotating([BLSSignature; 4]), consider extending or branching accordingly. Otherwise, it may be unclear how rotating quorums are supported.dash/src/sml/masternode_list_engine/non_rotated_quorum_construction.rs (4)
5-7: Appropriate addition of import forVerifyingChainLockSignaturesTypeThis import aligns with the PR objective to use chain lock signatures directly from the quorum structures rather than requesting them separately.
17-17: Correctly updated to use new block container architectureChanged from accessing
self.block_heightsdirectly to usingself.block_container.get_height(block_hash), which properly encapsulates block height lookups.
38-45: Well-implemented pattern matching for chain lock signaturesThis change properly extracts chain lock signatures directly from the quorum entry, with appropriate error handling for missing signatures. This is a key implementation of the DIP 29 functionality mentioned in the PR, which incorporates chain lock signatures in MNListDiff messages.
50-50: Properly utilizes extracted chain lock signatureThe function now uses the extracted signature directly instead of looking it up in a map, simplifying the code and reducing potential points of failure. This change aligns with the PR objective of eliminating the need to request chain lock signatures separately.
dash/src/network/message_qrinfo.rs (2)
36-38: Good addition of serialization/deserialization capabilitiesAdding the conditional derive attributes for bincode and serde enables proper serialization and deserialization of the
QRInfostruct, which could be important for efficiently handling messages containing chain lock signatures.
329-338: Improved test case robustnessThe updated test pattern matching explicitly verifies the deserialized message type and provides clearer error messages if unexpected types are encountered. This makes the tests more reliable and easier to debug.
dash/src/sml/masternode_list_engine/message_request_verification.rs (1)
368-468: Appropriately updated test data to match new implementationAll the updated hexadecimal data and expected values properly align with the revised implementation that uses chain lock signatures from MNListDiff messages rather than requesting them separately.
dash/src/sml/quorum_entry/quorum_modifier_type.rs (1)
106-107: Simplified parameter interface for better usabilityChanged from accepting a map of chain lock signatures to directly accepting a single signature, which eliminates the need for lookups and potential error cases. This change is central to implementing the PR objective of using embedded chain lock signatures.
dash/src/sml/masternode_list/apply_diff.rs (7)
78-80: Implementation looks fine.The approach of pre-allocating a vector of optional signatures is appropriate and ensures consistent indexing.
92-96: Good completeness check.Returning
SmlError::IncompleteSignatureSetis a clear approach to handle missing signatures.
100-114: Potential limitation if multiple rotating quorums exist.The code only sets
rotating_sigonce, ignoring subsequent rotating quorums. Verify if multiple rotating quorums can occur in a single diff. If so, consider storing or combining multiple signatures.
115-129: Logic for combining previous chain lock signatures.This logic appears correct, but please ensure that
previous_chain_lock_sigsis always the right length and used consistently across the codebase.
130-133: Good usage of NonRotating variant.No issues found here. The approach strictly differentiates rotating vs non-rotating, which is consistent with DIP 29.
134-143: Encapsulation is neat.The construction of
QualifiedQuorumEntryis well-organized. No issues noted.
155-155: Confirmed single rotating signature return.Returning a single
rotating_sigmight be limiting. Double-check that only one rotating quorum can appear in each diff, or handle multiple if needed.dash/src/sml/masternode_list_engine/rotated_quorum_construction.rs (5)
33-34: Ensure block heights are fed correctly.This check is crucial. If the block container hasn't been updated, it correctly returns an error. Good.
48-54: Graceful handling of missing rotating signatures.Returning
RequiredRotatedChainLockSigsNotPresentis appropriate to signal incomplete data.
57-61: Clear separation of rotating quorum retrieval.Invoking
masternode_list_entry_members_for_rotated_quorumis well-defined. Implementation is consistent with the new approach.
100-106: Validation for rotating signatures repeated.Again, properly returning an error if signatures are missing. Consistent with the approach in
find_rotated_masternodes_for_quorum.
146-147: Confirm block reference.Ensuring the block container has the block height is mandatory. This consistent approach is good.
dash/src/sml/masternode_list_engine/mod.rs (33)
25-27: Imports for QuorumEntry.Looks correct to import
QualifiedQuorumEntryandVerifyingChainLockSignaturesType.
41-46: Feeding block height method.Implementation is straightforward; no issues.
48-54: Enum wrapping the container.Clear approach. Potentially consider additional container types in the future.
56-60: Default trait for block container.Good that you provide a default, ensuring a consistent fallback.
62-66: From impl for the container.Smooth conversion from BTreeMap container to general block container type.
68-75: Implementing get_height.Accessing the stored height from the BTreeMap is valid.
77-83: Implementing get_hash.Symmetrical logic for retrieving block hash by height. LGTM.
85-91: Implementing contains_hash.Inline approach is consistent with the pattern used above.
93-99: Implementing contains_height.Mirror logic for checking if the container knows the height.
101-107: Forwarding feed_block_height.Ensures we remain container-agnostic in the engine.
109-113: known_block_count method.Convenient for debugging or analytics.
116-133: MasternodeListEngine updated struct.Now includes
block_containerfor better modularity. This aligns with DIP29 usage.
138-138: Use of Default for block_container.Approach is consistent with the type's own default implementation.
165-169: Initialize with diff to height.Populating
block_hashesandblock_heightsin the new container is correct.
256-259: masternode_list_for_block_hash method.Switch to using
block_containerfor block height retrieval is consistent. Good job removing duplication.
260-260: Exposing feed_block_height.Feeding block heights into the container ensures the engine is always aware of new blocks.
Also applies to: 262-262
330-330: Conditional feed of QuorumEntry block height.This block properly updates the container if the
quorum_hashis unknown.
339-356: request_mn_list_diff_heights now checks container.This ensures the container is updated with
base_block_hashandblock_hashbefore usage.
363-363: New param verify_tip_non_rotated_quorums.Expands functionality to let users specify if tip-based quorums should be verified.
395-395: apply_diff with None for previous_chain_lock_sigs.Ensures partial diffs are still applied without rotating chain lock usage.
409-409: Validating tip block hash presence.Appropriately returns error if block is not recognized.
419-419: Apply an older diff if found.The code gracefully merges the older diff into the engine as well.
424-430: Returning error if no rotating sig is found.Ensures we do not proceed with incomplete chain lock data for h - 3c.
433-449: Similar approach for h - 2c.Again, correct error if rotating sig is missing.
450-455: Same logic for h - c.
457-467: apply_diff with [sigm2, sigm1, sigm0].Correctly passing the chain lock sig array to handle rotating quorums.
471-478: Embedding verifying_chain_lock_signature.We store the rotating array in
QualifiedQuorumEntry. Clean approach.
655-683: Apply diff logic integrating rotation signature.Neatly merges the rotation signature if
quorum_validationis enabled.
687-691: Missing base block hash scenario.Properly returns
BlockHashLookupFailed.
708-713: Storing rotation_sig and updating masternode lists.The code merges the new data into the engine's internal state.
Also applies to: 775-777
779-804: No quorum_validation: fallback logic for rotation_sig.Graceful fallback if the feature is not turned on.
805-805: Feeding block height from diff_end_height.Consistent with the approach in other places.
807-807: Return the optional rotation signature.Completes the application of the diff.
The previous system worked by requesting chain lock sigs from the client. However SPV clients do not have easy access to these chain lock sigs.
In DIP 29 the chain lock sigs were added to the MNListDiff messages to fix this issue, and in this code we use these instead of requesting them.
Summary by CodeRabbit
Chores
Refactor
Tests