refactor: validator trait

[ZocoLini]

Currently, validation logic for different structs is spread in the codebase without following a consistent pattern. Sometimes it is a ValidatorStruct, logic inside a method, an independant method, etc, making them hard to locate. The idea of this branch is to move all the validation logic into the same module and implementing the same trait as I made in the first commit with the headers and the instantlock

Summary by CodeRabbit

• Refactor
  • Restructured validation system to use trait-based design for block headers and instant locks, improving code maintainability and reducing internal API surface.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

This PR refactors the validation system from standalone functions to a trait-based validator pattern. A new Validator<T> trait is introduced in the validation module, with BlockHeaderValidator and InstantLockValidator types now implementing this trait. InstantLockValidator is refactored to accept MasternodeListEngine at construction time, and the header validation module is made private while validators are centralized and exposed through the validation module.

Changes

Cohort / File(s)	Summary
Validator Trait & Central API dash-spv/src/validation/mod.rs	Introduces new generic Validator<T> trait with validate method. Exposes BlockHeaderValidator publicly. Adds internal import for ValidationResult.
Header Validation Refactoring dash-spv/src/validation/header.rs, dash-spv/src/sync/headers/mod.rs	Replaces standalone validate_headers function with BlockHeaderValidator struct implementing Validator<&[CachedHeader]>. Removes public module and function exports from headers module to make validation private.
Header Validation Integration dash-spv/src/sync/headers/manager.rs	Updates handle_headers_message to use BlockHeaderValidator::new().validate(...) instead of standalone function call. Adds imports for BlockHeaderValidator and Validator.
InstantLock Validation Refactoring dash-spv/src/validation/instantlock.rs	Introduces InstantLockValidator<'a> struct holding MasternodeListEngine reference. Implements Validator<&InstantLock> trait. Replaces previous no-arg constructor with new(masternode_engine). Converts validation to trait-based method invocation.
Client Chainlock Integration dash-spv/src/client/chainlock.rs	Updates to use refactored InstantLockValidator API: passes masternode_engine to constructor instead of validation method. Changes call from validator.validate(&islock, masternode_engine) to validator.validate(&islock).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 Validators align in a trait so fine,
Chainlocks and headers in patterns divine,
Engines passed once, at construction's gate,
The rabbit refactors to keep code first-rate! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main refactoring objective: introducing a validator trait pattern and refactoring validation code to use it.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[ZocoLini]

@xdustinface I would appreciate your opinion on this idea

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

dash-spv/src/sync/headers/manager.rs (1)

190-196: Implement distinct validation modes for Basic and Full.

The code currently treats all non-None validation modes identically, but ValidationMode defines three distinct modes: None (no validation), Basic (structure and timestamp validation), and Full (complete PoW and chain validation). The BlockHeaderValidator must be updated to accept and respect a ValidationMode parameter to support this distinction, applying full PoW and chain validation only in Full mode while limiting to structure/timestamp checks in Basic mode.

As a secondary optimization, consider caching BlockHeaderValidator as a field rather than instantiating it on every handle_headers_message call, though this is minor given its stateless nature.

🧹 Nitpick comments (1)

dash-spv/src/validation/instantlock.rs (1)

44-79: Thorough structural and signature validation.

Structure validation covers all critical checks:

• Non-zero txid
• Non-zero signature
• At least one input
• No null input txids

Signature validation correctly delegates to masternode_engine.verify_is_lock() for DIP 24-compliant quorum-based BLS verification.

Minor: The doc comments on lines 44-45 and 81-82 say "Don't call or expose this method directly", yet tests call validate_structure directly. Consider either:

1. Making these methods pub(crate) for test access only
2. Using #[cfg(test)] to expose them
3. Updating tests to use validate() where full validation isn't needed

This is a minor inconsistency that doesn't affect correctness.

Also applies to: 81-102

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 230fbdf and 801ce65.

📒 Files selected for processing (6)

• dash-spv/src/client/chainlock.rs
• dash-spv/src/sync/headers/manager.rs
• dash-spv/src/sync/headers/mod.rs
• dash-spv/src/validation/header.rs
• dash-spv/src/validation/instantlock.rs
• dash-spv/src/validation/mod.rs

💤 Files with no reviewable changes (1)

• dash-spv/src/sync/headers/mod.rs

🧰 Additional context used

📓 Path-based instructions (5)

dash-spv/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features

Files:

• dash-spv/src/sync/headers/manager.rs
• dash-spv/src/validation/header.rs
• dash-spv/src/validation/mod.rs
• dash-spv/src/client/chainlock.rs
• dash-spv/src/validation/instantlock.rs

dash-spv/src/sync/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks

Files:

• dash-spv/src/sync/headers/manager.rs

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with #[cfg(test)] annotation; integration tests use the tests/ directory
Use snake_case for function and variable names
Use UpperCamelCase for types and traits
Use SCREAMING_SNAKE_CASE for constants
Format code with rustfmt before commits; ensure cargo fmt --all is run
Run cargo clippy --workspace --all-targets -- -D warnings for linting; avoid warnings in CI
Prefer async/await via tokio for asynchronous operations

**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys

Files:

• dash-spv/src/sync/headers/manager.rs
• dash-spv/src/validation/header.rs
• dash-spv/src/validation/mod.rs
• dash-spv/src/client/chainlock.rs
• dash-spv/src/validation/instantlock.rs

dash-spv/src/validation/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

Implement three validation modes: ValidationMode::None (no validation), ValidationMode::Basic (structure and timestamp validation), and ValidationMode::Full (complete PoW and chain validation)

Files:

• dash-spv/src/validation/header.rs
• dash-spv/src/validation/mod.rs
• dash-spv/src/validation/instantlock.rs

dash-spv/src/client/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization

Files:

• dash-spv/src/client/chainlock.rs

🧠 Learnings (16)

📓 Common learnings

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Implement three validation modes: ValidationMode::None (no validation), ValidationMode::Basic (structure and timestamp validation), and ValidationMode::Full (complete PoW and chain validation)

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks

Applied to files:

• dash-spv/src/sync/headers/manager.rs
• dash-spv/src/client/chainlock.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Implement three validation modes: ValidationMode::None (no validation), ValidationMode::Basic (structure and timestamp validation), and ValidationMode::Full (complete PoW and chain validation)

Applied to files:

• dash-spv/src/sync/headers/manager.rs
• dash-spv/src/validation/header.rs
• dash-spv/src/validation/mod.rs
• dash-spv/src/client/chainlock.rs
• dash-spv/src/validation/instantlock.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory

Applied to files:

• dash-spv/src/sync/headers/manager.rs
• dash-spv/src/validation/header.rs

📚 Learning: 2025-06-26T16:02:42.390Z

Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.

Applied to files:

• dash-spv/src/sync/headers/manager.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/error.rs : Use domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) rather than generic error handling

Applied to files:

• dash-spv/src/sync/headers/manager.rs
• dash-spv/src/client/chainlock.rs

📚 Learning: 2025-12-19T00:07:22.904Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Always validate network consistency when deriving or validating addresses; never mix mainnet and testnet operations

Applied to files:

• dash-spv/src/validation/header.rs
• dash-spv/src/client/chainlock.rs
• dash-spv/src/validation/instantlock.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features

Applied to files:

• dash-spv/src/validation/mod.rs
• dash-spv/src/client/chainlock.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager

Applied to files:

• dash-spv/src/client/chainlock.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/client/**/*.rs : Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization

Applied to files:

• dash-spv/src/client/chainlock.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks

Applied to files:

• dash-spv/src/validation/instantlock.rs

📚 Learning: 2025-12-30T22:00:57.000Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*integration*.rs : Write integration tests for network operations

Applied to files:

• dash-spv/src/validation/instantlock.rs

📚 Learning: 2025-12-22T17:59:51.097Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/tests/**/*.rs : Use descriptive test names (e.g., `test_parse_address_mainnet`)

Applied to files:

• dash-spv/src/validation/instantlock.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite

Applied to files:

• dash-spv/src/validation/instantlock.rs

📚 Learning: 2025-12-30T22:00:57.000Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*test*.rs : Test both mainnet and testnet configurations

Applied to files:

• dash-spv/src/validation/instantlock.rs

📚 Learning: 2025-12-22T17:59:51.097Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Cover critical parsing, networking, SPV, and wallet flows in tests; add regression tests for fixes; consider property tests with `proptest`

Applied to files:

• dash-spv/src/validation/instantlock.rs

🧬 Code graph analysis (3)

dash-spv/src/validation/header.rs (2)

dash-spv/src/validation/mod.rs (1)

• validate (12-12)

dash-spv/src/types.rs (1)

• header (67-69)

dash-spv/src/client/chainlock.rs (1)

dash-spv/src/validation/instantlock.rs (1)

• new (38-42)

dash-spv/src/validation/instantlock.rs (2)

dash-spv/src/validation/mod.rs (1)

• validate (12-12)

dash/src/sml/masternode_list_engine/mod.rs (1)

• default_for_network (206-211)

⏰ 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). (20)

• GitHub Check: fuzz (hashes_json)
• GitHub Check: fuzz (hashes_sha512)
• GitHub Check: fuzz (hashes_sha256)
• GitHub Check: fuzz (hashes_ripemd160)
• GitHub Check: fuzz (hashes_sha512_256)
• GitHub Check: fuzz (hashes_cbor)
• GitHub Check: fuzz (dash_deserialize_witness)
• GitHub Check: fuzz (dash_deserialize_block)
• GitHub Check: fuzz (dash_outpoint_string)
• GitHub Check: fuzz (hashes_sha1)
• GitHub Check: fuzz (dash_deser_net_msg)
• GitHub Check: fuzz (dash_deserialize_script)
• GitHub Check: fuzz (dash_deserialize_address)
• GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
• GitHub Check: fuzz (dash_deserialize_amount)
• GitHub Check: Address Sanitizer
• GitHub Check: Thread Sanitizer
• GitHub Check: Pre-commit (ubuntu-latest)
• GitHub Check: Pre-commit (windows-latest)
• GitHub Check: Pre-commit (macos-latest)

🔇 Additional comments (10)

dash-spv/src/sync/headers/manager.rs (1)

17-17: LGTM!

Import correctly brings in the new trait-based validation types.

dash-spv/src/validation/mod.rs (1)

11-13: Well-designed trait with minimal surface area.

The Validator<T> trait is clean and generic. However, per coding guidelines, validators should support three validation modes (None, Basic, Full). Consider whether the validation mode should be:

1. Passed at construction time (as currently done with InstantLockValidator and its MasternodeListEngine)
2. Added as an optional parameter to validate()
3. Configured per-validator instance

The current design allows mode configuration at construction time, which is reasonable.

dash-spv/src/client/chainlock.rs (2)

15-15: LGTM!

Import correctly updated to use the new trait-based validation types.

---

103-104: Clean migration to trait-based validation.

The InstantLockValidator is correctly constructed with the masternode_engine reference and validation is invoked through the Validator trait. The error handling and peer penalization logic remains intact.

dash-spv/src/validation/header.rs (3)

8-15: LGTM! Clean validator struct definition.

The #[derive(Default)] with new() constructor follows Rust conventions. The stateless design is appropriate for header validation.

---

17-46: Solid parallel validation implementation.

The use of par_iter() with try_for_each efficiently validates headers in parallel. The chain continuity check correctly skips i == 0 to avoid out-of-bounds access, and PoW verification uses the cached block hash.

One observation: per coding guidelines, ValidationMode::Basic should perform "structure and timestamp validation" while ValidationMode::Full performs "complete PoW and chain validation". The current implementation always performs Full validation (PoW check). Consider accepting a validation mode parameter if Basic validation (skipping PoW) is needed.

---

71-76: Tests are comprehensive and correctly updated.

Test coverage includes:

• Empty headers (edge case)
• Single header
• Valid chain construction
• Broken chain detection
• Invalid PoW detection (single and mid-chain)
• Genesis block validation for multiple networks

All tests properly instantiate BlockHeaderValidator::new() before calling validate().

Also applies to: 78-84, 86-100, 102-113, 115-130, 132-144, 146-167

dash-spv/src/validation/instantlock.rs (3)

11-16: Well-structured validator with lifetime-bound dependency.

The design correctly captures the MasternodeListEngine reference with a lifetime parameter, ensuring the validator cannot outlive the engine. The doc comment appropriately warns about the security requirement for signature verification.

---

18-35: Clean trait implementation and constructor.

The Validator trait implementation correctly sequences structure validation before signature verification. The new() constructor provides a clear entry point for creating validators with required dependencies.

Also applies to: 37-42

---

176-186: Tests correctly updated for new validator pattern.

Tests properly construct MasternodeListEngine::default_for_network(Network::Testnet) and instantiate InstantLockValidator::new(&masternode_engine). The pattern is consistent across all test cases.

Note: Tests call validate_structure directly despite the doc comment advising against it. This is acceptable for unit testing the structure validation in isolation, but be aware that these tests won't catch regressions if validate_structure behavior changes independently of validate.

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[xdustinface]

@xdustinface I would appreciate your opinion on this idea

Yeah i like it! Definitely worth exploring further if we can map all validation to it.