Skip to content

feat: basic ferment integration #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 81 commits into
base: master
Choose a base branch
from
Open

feat: basic ferment integration #60

wants to merge 81 commits into from

Conversation

pankcuf
Copy link
Contributor

@pankcuf pankcuf commented Feb 28, 2025

Summary by CodeRabbit

  • New Features

    • Added conditional export attributes to many public types, enhancing compatibility with external tools when a specific feature is enabled.
    • Introduced new methods for masternode and quorum public key lookup.
    • Made several modules and types publicly accessible, improving API usability.
    • Added support for vector encoding/decoding of signed 32-bit integers.
    • Added a new feature named apple with associated dependencies to extend functionality.
  • Improvements

    • Updated visibility of fields and modules to allow broader access where needed.
    • Enhanced modularity and dependency management through new and updated dependencies.
    • Reorganized import paths for improved clarity and maintainability.
  • Bug Fixes

    • Updated field and method types for consistency and improved type safety.
  • Chores

    • Refined and reorganized import paths for better maintainability.

Copy link

coderabbitai bot commented Feb 28, 2025

Walkthrough

This update introduces a new "apple" feature and several optional dependencies, restructures module paths for clarity, and conditionally annotates many public types and enums with the ferment_macro::export attribute for feature-based export. Numerous structs and fields have their visibility increased, and several new utility methods are added. Import paths are reorganized to reflect a more consistent module hierarchy.

Changes

File(s) / Path(s) Change Summary
dash/Cargo.toml, hashes/Cargo.toml, rpc-json/Cargo.toml Added new optional dependencies (bincode_derive, ferment, ferment-macro), updated bls-signatures revision, and introduced the apple feature with associated dependencies.
dash/src/blockdata/script/mod.rs, dash/src/lib.rs, dash/src/sml/mod.rs, dash/src/sml/masternode_list_engine/mod.rs Changed module visibility to public for several modules.
dash/src/blockdata/script/owned.rs, dash/src/blockdata/transaction/mod.rs, dash/src/blockdata/transaction/outpoint.rs, dash/src/blockdata/transaction/txin.rs, dash/src/blockdata/transaction/txout.rs, dash/src/blockdata/transaction/special_transaction/asset_lock.rs, dash/src/blockdata/transaction/special_transaction/asset_unlock/qualified_asset_unlock.rs, dash/src/blockdata/transaction/special_transaction/asset_unlock/request_info.rs, dash/src/blockdata/transaction/special_transaction/asset_unlock/unqualified_asset_unlock.rs, dash/src/blockdata/transaction/special_transaction/coinbase.rs, dash/src/blockdata/transaction/special_transaction/mod.rs, dash/src/blockdata/transaction/special_transaction/provider_registration.rs, dash/src/blockdata/transaction/special_transaction/provider_update_registrar.rs, dash/src/blockdata/transaction/special_transaction/provider_update_revocation.rs, dash/src/blockdata/transaction/special_transaction/provider_update_service.rs, dash/src/blockdata/transaction/special_transaction/quorum_commitment.rs, dash/src/blockdata/witness.rs, dash/src/bls_sig_utils.rs, dash/src/ephemerealdata/instant_lock.rs, dash/src/ephemerealdata/chain_lock.rs, dash/src/sml/llmq_entry_verification.rs, dash/src/sml/llmq_type/mod.rs, dash/src/sml/masternode_list/mod.rs, dash/src/sml/masternode_list_entry/mod.rs, dash/src/sml/masternode_list_entry/qualified_masternode_list_entry.rs, dash/src/sml/quorum_entry/qualified_quorum_entry.rs, dash/src/sml/quorum_validation_error.rs, dash/src/sml/error.rs, dash/src/lib.rs Added #[cfg_attr(feature = "apple", ferment_macro::export)] to many structs, enums, and types for conditional export. Increased visibility of several fields and types.
dash/src/sml/masternode_list/mod.rs, dash/src/sml/masternode_list_engine/mod.rs, dash/src/sml/masternode_list_entry/mod.rs Added new methods for finding quorum public keys and classifying operator key types.
dash/src/sml/masternode_list_entry/hash.rs, dash/src/sml/masternode_list_entry/qualified_masternode_list_entry.rs Changed hash field and method types from sha256d::Hash to Sha256dHash.
dash/src/blockdata/transaction/special_transaction/quorum_commitment.rs, dash/src/blockdata/witness.rs Made selected struct fields public.
dash/src/consensus/encode.rs, dash/src/crypto/sighash.rs, dash/src/taproot.rs, dash/src/bip158.rs, dash/src/network/message_qrinfo.rs, dash/src/network/message_sml.rs, dash/src/sml/quorum_entry/hash.rs, dash/src/hash_types.rs Updated import paths to use new or more explicit module hierarchies.
dash/src/consensus/encode.rs Added consensus encoding/decoding for Vec<i32>.
dash/src/sml/masternode_list/merkle_roots.rs Changed hash collection to use to_raw_hash() method.
dash/src/sml/masternode_list_engine/mod.rs Added clear and find_quorum_public_key methods; changed return type of apply_diff.
hashes/src/internal_macros.rs Minor formatting change (added blank line).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant FeatureFlag
    participant StructsAndEnums
    participant FermentMacro

    User->>FeatureFlag: Enable "apple" feature
    FeatureFlag->>StructsAndEnums: Apply #[cfg_attr(feature = "apple", ferment_macro::export)]
    StructsAndEnums->>FermentMacro: Export selected types and enums
Loading
sequenceDiagram
    participant MasternodeListEngine
    participant MasternodeList
    participant QuorumType
    participant QuorumHash

    MasternodeListEngine->>MasternodeList: find_quorum_public_key(quorum_type, quorum_hash)
    MasternodeList->>QuorumType: Match quorum type
    MasternodeList->>QuorumHash: Match quorum hash
    MasternodeList-->>MasternodeListEngine: Return Option<BLSPublicKey>
Loading

Possibly related PRs

  • dashpay/rust-dashcore#67: Adds public methods to MasternodeListEngine related to quorum entries and signatures, sharing enhancements in quorum-related functionality.

Poem

A hop, a skip, a feature called "apple"—
Now structs and enums are ready to travel!
With Ferment's export, new fields in the sun,
Public and proud, their journey's begun.
Optional crates and imports align,
The codebase grows stronger, line by line.
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 90f28c4 and 122c0ca.

📒 Files selected for processing (1)
  • dash/src/blockdata/transaction/outpoint.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • dash/src/blockdata/transaction/outpoint.rs
⏰ Context from checks skipped due to timeout of 90000ms (15)
  • GitHub Check: fuzz (hashes_sha512)
  • GitHub Check: fuzz (hashes_sha1)
  • GitHub Check: fuzz (hashes_sha256)
  • GitHub Check: fuzz (hashes_sha512_256)
  • GitHub Check: fuzz (hashes_json)
  • GitHub Check: fuzz (hashes_ripemd160)
  • GitHub Check: fuzz (dash_deserialize_block)
  • GitHub Check: fuzz (dash_deserialize_script)
  • GitHub Check: fuzz (dash_deserialize_witness)
  • GitHub Check: fuzz (dash_deserialize_address)
  • GitHub Check: fuzz (hashes_cbor)
  • GitHub Check: fuzz (dash_deser_net_msg)
  • GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
  • GitHub Check: fuzz (dash_outpoint_string)
  • GitHub Check: fuzz (dash_deserialize_amount)
✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@pankcuf pankcuf changed the title Feat/ferment feat: basic ferment integration Feb 28, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
dash/Cargo.toml (1)

75-76: Verify the ferment dependencies are pinned to a specific version or commit.

The newly added dependencies are fetched directly from a git repository without specifying a commit or tag, which could lead to build issues if the upstream code changes.

Consider pinning these dependencies to a specific tag, commit, or version:

-ferment = { git = "https://github.com/dashpay/ferment", package = "ferment" }
-ferment-macro = { git = "https://github.com/dashpay/ferment", package = "ferment-macro" }
+ferment = { git = "https://github.com/dashpay/ferment", package = "ferment", tag = "v0.1.0" }
+ferment-macro = { git = "https://github.com/dashpay/ferment", package = "ferment-macro", tag = "v0.1.0" }

Replace v0.1.0 with the appropriate version.

dash/src/blockdata/witness.rs (1)

35-35: Consider documenting the implications of changing field visibility.

You're making internal fields public and marking the struct for export with ferment, which changes the API contract. This could have implications for backward compatibility and encapsulation.

Consider adding documentation that explains:

  1. Why these fields are now public
  2. The intended use cases for direct field access
  3. Any constraints or invariants that users must maintain when modifying these fields

For example:

/// Fields are made public to support integration with the ferment serialization system.
/// When modifying these fields directly, ensure that the internal invariants of the Witness
/// structure are maintained to avoid breaking the witness validation logic.
#[ferment_macro::export]
pub struct Witness {
    // ...
}

Also applies to: 39-39, 45-45, 49-49

dash/src/blockdata/transaction/special_transaction/quorum_commitment.rs (1)

184-189: Ensure consistent access control across the codebase.

Making fields public in QuorumCommitmentPayload is a significant change to the API contract. While this supports the ferment integration, it represents a shift from encapsulation to direct field access.

Consider adding validation or update methods to ensure that when fields are modified directly, all invariants are maintained. For example:

impl QuorumCommitmentPayload {
    // Existing methods...
    
    /// Validates that the current state of the payload is consistent
    pub fn validate(&self) -> Result<(), SomeErrorType> {
        // Check version is valid
        // Check height is valid
        // Validate finalization_commitment
        Ok(())
    }
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 051501c and 73b5df6.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (28)
  • dash/Cargo.toml (1 hunks)
  • dash/src/blockdata/script/mod.rs (1 hunks)
  • dash/src/blockdata/script/owned.rs (1 hunks)
  • dash/src/blockdata/transaction/mod.rs (1 hunks)
  • dash/src/blockdata/transaction/outpoint.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/asset_lock.rs (2 hunks)
  • dash/src/blockdata/transaction/special_transaction/asset_unlock/qualified_asset_unlock.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/asset_unlock/request_info.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/asset_unlock/unqualified_asset_unlock.rs (2 hunks)
  • dash/src/blockdata/transaction/special_transaction/coinbase.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/mod.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/provider_registration.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/provider_update_registrar.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/provider_update_revocation.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/provider_update_service.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/quorum_commitment.rs (2 hunks)
  • dash/src/blockdata/transaction/txin.rs (1 hunks)
  • dash/src/blockdata/transaction/txout.rs (1 hunks)
  • dash/src/blockdata/witness.rs (1 hunks)
  • dash/src/bls_sig_utils.rs (2 hunks)
  • dash/src/ephemerealdata/instant_lock.rs (1 hunks)
  • dash/src/sml/llmq_entry_verification.rs (1 hunks)
  • dash/src/sml/llmq_type/mod.rs (1 hunks)
  • dash/src/sml/masternode_list/mod.rs (1 hunks)
  • dash/src/sml/masternode_list_entry/mod.rs (3 hunks)
  • dash/src/sml/masternode_list_entry/qualified_masternode_list_entry.rs (1 hunks)
  • dash/src/sml/quorum_entry/qualified_quorum_entry.rs (1 hunks)
  • hashes/src/internal_macros.rs (1 hunks)
✅ Files skipped from review due to trivial changes (15)
  • dash/src/blockdata/transaction/txout.rs
  • dash/src/blockdata/transaction/special_transaction/provider_update_revocation.rs
  • dash/src/blockdata/transaction/special_transaction/asset_unlock/request_info.rs
  • dash/src/sml/quorum_entry/qualified_quorum_entry.rs
  • dash/src/blockdata/transaction/special_transaction/coinbase.rs
  • dash/src/sml/masternode_list_entry/qualified_masternode_list_entry.rs
  • dash/src/blockdata/transaction/txin.rs
  • dash/src/blockdata/transaction/special_transaction/provider_update_registrar.rs
  • dash/src/blockdata/transaction/special_transaction/mod.rs
  • dash/src/blockdata/transaction/mod.rs
  • hashes/src/internal_macros.rs
  • dash/src/blockdata/transaction/special_transaction/provider_registration.rs
  • dash/src/sml/masternode_list_entry/mod.rs
  • dash/src/sml/llmq_entry_verification.rs
  • dash/src/sml/llmq_type/mod.rs
🧰 Additional context used
🪛 GitHub Actions: Continuous integration
dash/Cargo.toml

[error] 1-1: Failed to parse manifest: feature quorum_validation includes bls-signatures, but bls-signatures is not an optional dependency. A non-optional dependency of the same name is defined; consider adding optional = true to its definition.

🔇 Additional comments (16)
dash/src/blockdata/transaction/special_transaction/provider_update_service.rs (1)

55-55: Attribute added consistently with the rest of the codebase.

The addition of #[ferment_macro::export] to the ProviderUpdateServicePayload struct is consistent with similar changes across the codebase as mentioned in the PR summary. This enables the struct to be properly exported through the ferment framework.

dash/src/blockdata/script/owned.rs (1)

32-32: Adds ferment export to ScriptBuf struct.

Adding the #[ferment_macro::export] attribute to the ScriptBuf struct enables its export for use with the ferment macro system. This aligns with the PR objective of adding basic ferment integration.

dash/src/blockdata/script/mod.rs (1)

72-72: Makes the owned module public to support ferment exports.

Changing the visibility of the owned module from private to public is necessary to allow external access to the now-exported ScriptBuf struct. This change logically complements the export attribute added to ScriptBuf in the owned.rs file.

dash/src/blockdata/transaction/outpoint.rs (1)

38-38: Adds ferment export to OutPoint struct.

Adding the #[ferment_macro::export] attribute to the OutPoint struct makes it available for the ferment macro system. This is consistent with the pattern of adding ferment exports across the codebase.

dash/src/blockdata/transaction/special_transaction/asset_unlock/qualified_asset_unlock.rs (1)

54-54: Adds ferment export to AssetUnlockPayload struct.

Adding the #[ferment_macro::export] attribute to the AssetUnlockPayload struct enables its export for the ferment macro system. This continues the consistent pattern of adding ferment exports to key data structures throughout the codebase.

dash/src/blockdata/transaction/special_transaction/asset_unlock/unqualified_asset_unlock.rs (2)

33-33: Import path updated correctly.

The import path for TxOut has been updated to use the more specific hierarchical path, which is consistent with the module reorganization in this PR.


42-42: Added ferment export attribute.

The #[ferment_macro::export] attribute has been properly added to the AssetUnlockBasePayload struct, which aligns with the PR's goal of adding basic ferment integration.

dash/src/sml/masternode_list/mod.rs (2)

18-18: Import statement properly expanded.

The hash types import has been expanded to include additional required types (BlockHash, ProTxHash, and QuorumHash), which provides more explicit dependencies.


27-27: Added ferment export attribute.

The #[ferment_macro::export] attribute has been properly added to the MasternodeList struct, consistent with other structs in this PR.

dash/src/ephemerealdata/instant_lock.rs (3)

5-9: Simplified Vec imports.

The conditional imports for Vec from both alloc and std libraries have been commented out, which suggests the code now relies on the standard prelude for Vec. This simplifies the import structure.


13-20: Reorganized imports for better structure.

The imports have been reorganized to be more specific and follow a consistent pattern throughout the codebase. The OutPoint import now correctly comes from the blockdata::transaction::outpoint module, and QuorumHash has been added to the hash types import.


27-27: Added ferment export attribute.

The #[ferment_macro::export] attribute has been properly added to the InstantLock struct, aligning with the PR's goal of ferment integration.

dash/src/blockdata/transaction/special_transaction/asset_lock.rs (2)

30-30: Import path updated correctly.

The import path for TxOut has been updated to use the more specific hierarchical path crate::blockdata::transaction::txout::TxOut, which is consistent with the module reorganization in this PR.


44-44: Added ferment export attribute.

The #[ferment_macro::export] attribute has been properly added to the AssetLockPayload struct, which aligns with the PR's goal of adding basic ferment integration.

dash/src/bls_sig_utils.rs (1)

37-38:

❓ Verification inconclusive

Consider implications of exposing internal byte arrays.

By making the tuple struct fields public, you're exposing direct access to the raw byte arrays. This could potentially allow manipulations that bypass validation.

Ensure that any code that modifies these public byte arrays directly doesn't violate invariants expected by the BLS signature operations. Consider whether accessor methods or validator functions would be safer alternatives.

Also applies to: 96-97


🏁 Script executed:

#!/bin/bash
# Check if there are any direct modifications to these byte arrays in the codebase
rg -A 2 -B 2 "\.0\s*=\s*\[" --type rust

Length of output: 41


Action Required: Verify Direct Access and Mutability of BLSPublicKey

No instances of direct modifications to the internal byte array were detected in our automated search. However, because the tuple struct exposes its &[u8; 48] field publicly, it's important to manually confirm that all usages of BLSPublicKey preserve the invariants expected by BLS signature operations. Consider whether helper methods—such as accessors or validators—could be introduced to restrict unsafe modifications.

  • Verify that no code elsewhere bypasses validation by directly modifying .0.
  • Reassess the design for both instances (lines 37–38 and 96–97) to ensure future changes won’t inadvertently introduce invariant violations.
dash/src/blockdata/transaction/special_transaction/quorum_commitment.rs (1)

42-43: LGTM: QuorumEntry export annotation is consistent with project direction.

The #[ferment_macro::export] annotation for QuorumEntry aligns with the pattern of exposing key structures for external use through the ferment system.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
dash/Cargo.toml (3)

36-36: New "apple" Feature Integration:
The new "apple" feature correctly aggregates the dependencies on ferment, ferment-macro, and the specified sub-features from bls-signatures. To improve maintainability, consider adding an explanatory comment in this section or updating your documentation to clarify the purpose and usage of the "apple" feature.


76-76: New "ferment" Dependency Added:
The new ferment dependency is added correctly with the optional = true flag. Ensure that any integrations or tests involving this dependency are updated accordingly, and consider documenting its usage if it isn’t already clearly explained.


77-77: New "ferment-macro" Dependency Added:
The ferment-macro dependency has been added as expected with the optional flag. Please verify that any modules using this macro are properly covered by tests and that the integration is documented for clarity.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 73b5df6 and c8b35fd.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • dash/Cargo.toml (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Tests (stable, true, true, false)
  • GitHub Check: Tests (beta, true, false)
🔇 Additional comments (1)
dash/Cargo.toml (1)

75-75: Updated bls-signatures Dependency:
The bls-signatures dependency has been updated to use the tagged version "1.3.5" and now includes the optional = true flag, which aligns with previous review recommendations.

shumkov pushed a commit that referenced this pull request Feb 28, 2025
* updated dashcore version

* updated dashcore version
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
dash/Cargo.toml (1)

75-75: Confirm Updated "bls-signatures" Revision
The bls-signatures dependency has been updated to use the revision a0fec2f4da2bdbb753e6287b0546ae0b08e74e02 and is correctly marked as optional. This update aligns with previous feedback requiring the optional attribute. Please ensure this revision meets the expected stability and feature support for the apple feature.

🧹 Nitpick comments (1)
dash/Cargo.toml (1)

66-66: Validate New Dependency "bincode_derive"
The addition of bincode_derive with the version pinned to =2.0.0-rc.3 and marked as optional is clear and follows the intended dependency versioning strategy. Consider if pinning to an exact pre-release version is required or if a compatible version range might be more maintainable in the long run.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f5213fe and 1aeeb4b.

📒 Files selected for processing (1)
  • dash/Cargo.toml (3 hunks)
🔇 Additional comments (3)
dash/Cargo.toml (3)

36-36: Review New Feature "apple"
The new feature flag apple has been correctly added with its dependency list. Please verify that all the dependencies referenced here (such as "ferment", "ferment-macro", and the various bls-signatures/* features) are correctly declared in the [dependencies] section and are available as optional dependencies.


76-76: Check New Dependency "ferment"
The new dependency ferment is added with version "0.2", correctly referenced via its package name and marked as optional. Confirm that this dependency integrates well with the apple feature and does not conflict with existing versions used elsewhere.


77-77: Review Addition of "ferment-macro" Dependency
Similarly, the ferment-macro dependency is added with the version "0.2", properly set as optional. Ensure that enabling the apple feature triggers the intended macro exports and that this addition is consistent with the project's dependency strategy.

pankcuf added 2 commits April 10, 2025 17:43
# Conflicts:
#	dash/Cargo.toml
#	dash/src/bip158.rs
#	dash/src/blockdata/transaction/mod.rs
#	dash/src/blockdata/transaction/special_transaction/asset_lock.rs
#	dash/src/blockdata/transaction/special_transaction/asset_unlock/unqualified_asset_unlock.rs
#	dash/src/blockdata/transaction/special_transaction/provider_registration.rs
#	dash/src/blockdata/transaction/special_transaction/provider_update_registrar.rs
#	dash/src/blockdata/transaction/special_transaction/provider_update_service.rs
#	dash/src/blockdata/transaction/txin.rs
#	dash/src/consensus/encode.rs
#	dash/src/crypto/sighash.rs
#	dash/src/ephemerealdata/instant_lock.rs
#	dash/src/network/message_qrinfo.rs
#	dash/src/sml/masternode_list_entry/qualified_masternode_list_entry.rs
#	dash/src/taproot.rs
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
dash/src/blockdata/witness.rs (1)

39-39: Field visibility changes increase exposure of internal implementation details.

Changing these fields from private to public exposes implementation details that were previously encapsulated. The Witness struct uses a complex internal representation for performance reasons, with specific invariants between these fields that must be maintained.

Consider adding more robust documentation to these newly public fields explaining:

  1. The invariants that must be maintained between them
  2. How they should be manipulated safely (or preferably not at all directly)
  3. That direct manipulation could break the correctness of the Witness struct

While making these fields public may be necessary for ferment serialization, it increases the risk of improper usage. Ideally, there should be comprehensive tests that verify the fields can be safely accessed and modified through ferment without breaking the struct's behavior.

Also applies to: 45-45, 49-49

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d7ef308 and ff4a39c.

📒 Files selected for processing (33)
  • dash/Cargo.toml (2 hunks)
  • dash/src/bip158.rs (1 hunks)
  • dash/src/blockdata/script/mod.rs (1 hunks)
  • dash/src/blockdata/script/owned.rs (1 hunks)
  • dash/src/blockdata/transaction/mod.rs (3 hunks)
  • dash/src/blockdata/transaction/outpoint.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/asset_lock.rs (2 hunks)
  • dash/src/blockdata/transaction/special_transaction/asset_unlock/qualified_asset_unlock.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/asset_unlock/request_info.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/asset_unlock/unqualified_asset_unlock.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/coinbase.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/mod.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/provider_registration.rs (2 hunks)
  • dash/src/blockdata/transaction/special_transaction/provider_update_registrar.rs (2 hunks)
  • dash/src/blockdata/transaction/special_transaction/provider_update_revocation.rs (1 hunks)
  • dash/src/blockdata/transaction/special_transaction/provider_update_service.rs (2 hunks)
  • dash/src/blockdata/transaction/special_transaction/quorum_commitment.rs (2 hunks)
  • dash/src/blockdata/witness.rs (1 hunks)
  • dash/src/consensus/encode.rs (2 hunks)
  • dash/src/crypto/sighash.rs (1 hunks)
  • dash/src/ephemerealdata/chain_lock.rs (1 hunks)
  • dash/src/hash_types.rs (1 hunks)
  • dash/src/network/message_qrinfo.rs (3 hunks)
  • dash/src/sml/llmq_entry_verification.rs (1 hunks)
  • dash/src/sml/llmq_type/mod.rs (2 hunks)
  • dash/src/sml/masternode_list/merkle_roots.rs (1 hunks)
  • dash/src/sml/masternode_list_engine/mod.rs (7 hunks)
  • dash/src/sml/masternode_list_entry/hash.rs (1 hunks)
  • dash/src/sml/quorum_entry/hash.rs (1 hunks)
  • dash/src/sml/quorum_validation_error.rs (2 hunks)
  • dash/src/taproot.rs (1 hunks)
  • hashes/Cargo.toml (1 hunks)
  • rpc-json/Cargo.toml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (32)
  • dash/src/blockdata/transaction/special_transaction/provider_update_revocation.rs
  • dash/src/blockdata/transaction/special_transaction/mod.rs
  • dash/src/sml/quorum_entry/hash.rs
  • dash/src/sml/masternode_list/merkle_roots.rs
  • dash/src/blockdata/script/owned.rs
  • dash/src/blockdata/transaction/special_transaction/coinbase.rs
  • dash/src/hash_types.rs
  • dash/src/blockdata/transaction/outpoint.rs
  • dash/src/sml/quorum_validation_error.rs
  • dash/src/bip158.rs
  • dash/src/blockdata/transaction/special_transaction/provider_update_registrar.rs
  • dash/src/crypto/sighash.rs
  • dash/src/blockdata/script/mod.rs
  • dash/src/blockdata/transaction/special_transaction/asset_lock.rs
  • dash/src/blockdata/transaction/special_transaction/provider_update_service.rs
  • dash/src/sml/masternode_list_entry/hash.rs
  • dash/src/blockdata/transaction/special_transaction/asset_unlock/qualified_asset_unlock.rs
  • hashes/Cargo.toml
  • rpc-json/Cargo.toml
  • dash/src/blockdata/transaction/special_transaction/asset_unlock/request_info.rs
  • dash/src/sml/llmq_type/mod.rs
  • dash/src/ephemerealdata/chain_lock.rs
  • dash/src/blockdata/transaction/mod.rs
  • dash/src/sml/llmq_entry_verification.rs
  • dash/src/taproot.rs
  • dash/src/blockdata/transaction/special_transaction/provider_registration.rs
  • dash/src/blockdata/transaction/special_transaction/asset_unlock/unqualified_asset_unlock.rs
  • dash/src/network/message_qrinfo.rs
  • dash/src/blockdata/transaction/special_transaction/quorum_commitment.rs
  • dash/src/consensus/encode.rs
  • dash/Cargo.toml
  • dash/src/sml/masternode_list_engine/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (15)
  • GitHub Check: fuzz (hashes_cbor)
  • GitHub Check: fuzz (hashes_sha256)
  • GitHub Check: fuzz (hashes_sha512_256)
  • GitHub Check: fuzz (dash_deserialize_amount)
  • GitHub Check: fuzz (hashes_ripemd160)
  • GitHub Check: fuzz (hashes_sha512)
  • GitHub Check: fuzz (dash_deser_net_msg)
  • GitHub Check: fuzz (dash_deserialize_address)
  • GitHub Check: fuzz (hashes_sha1)
  • GitHub Check: fuzz (hashes_json)
  • GitHub Check: fuzz (dash_deserialize_block)
  • GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
  • GitHub Check: fuzz (dash_outpoint_string)
  • GitHub Check: fuzz (dash_deserialize_script)
  • GitHub Check: fuzz (dash_deserialize_witness)

@@ -32,20 +32,21 @@ use crate::{Script, VarInt};
/// [segwit upgrade]: <https://github.com/bitcoin/bips/blob/master/bip-0143.mediawiki>
#[derive(Clone, Default, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
#[cfg_attr(feature = "bincode", derive(Encode, Decode))]
#[cfg_attr(feature = "apple", ferment_macro::export)]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Attribution addition for ferment integration matches other similar changes.

The addition of the #[cfg_attr(feature = "apple", ferment_macro::export)] attribute aligns with the overall PR objective of integrating with ferment when the "apple" feature flag is enabled.

Looking at the previous review comment, there's a concern about testing the ferment serialization of the Witness struct. Let's verify if any tests have been added since that comment:


🏁 Script executed:

#!/bin/bash
# Search for tests that verify ferment serialization of Witness

echo "Looking for existing tests for ferment serialization of Witness"
rg -l "ferment.*Witness" --type rust

echo "Checking for test files that might contain ferment serialization tests"
rg -l "test.*ferment.*witness" --type rust -i

echo "Looking for ferment-related serialization tests"
rg -l "test.*serialize.*ferment" --type rust -i

Length of output: 529


Action Required: Add Tests for Ferment Serialization of the Witness Struct

The new attribute #[cfg_attr(feature = "apple", ferment_macro::export)] is in line with similar changes across the codebase, which is great. However, our current search did not uncover any tests verifying the ferment serialization of the Witness struct. To ensure that this integration behaves as expected when the "apple" feature is enabled, please add tests covering this functionality or point to where such tests exist if they’ve been added elsewhere.

  • File: dash/src/blockdata/witness.rs (line 35)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants