refactor: unify validation with a Validator trait
#355
Conversation
📝 WalkthroughWalkthroughRefactors validation to a trait-based approach: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant InstantLockValidator
participant MasternodeListEngine
participant Logger
Client->>MasternodeListEngine: obtain masternode_engine
Client->>InstantLockValidator: InstantLockValidator::new(masternode_engine)
Client->>InstantLockValidator: validator.validate(&instant_lock)
InstantLockValidator->>MasternodeListEngine: verify signature / quorum info
MasternodeListEngine-->>InstantLockValidator: verification result
InstantLockValidator-->>Client: ValidationResult (Ok / Err)
InstantLockValidator->>Logger: log validation steps/errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
|
@xdustinface I would appreciate your opinion on this idea |
![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
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
ValidationModedefines three distinct modes:None(no validation),Basic(structure and timestamp validation), andFull(complete PoW and chain validation). TheBlockHeaderValidatormust be updated to accept and respect aValidationModeparameter to support this distinction, applying full PoW and chain validation only inFullmode while limiting to structure/timestamp checks inBasicmode.As a secondary optimization, consider caching
BlockHeaderValidatoras a field rather than instantiating it on everyhandle_headers_messagecall, 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_structuredirectly. Consider either:
- Making these methods
pub(crate)for test access only- Using
#[cfg(test)]to expose them- Updating tests to use
validate()where full validation isn't neededThis is a minor inconsistency that doesn't affect correctness.
Also applies to: 81-102
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
dash-spv/src/client/chainlock.rsdash-spv/src/sync/headers/manager.rsdash-spv/src/sync/headers/mod.rsdash-spv/src/validation/header.rsdash-spv/src/validation/instantlock.rsdash-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.rsdash-spv/src/validation/header.rsdash-spv/src/validation/mod.rsdash-spv/src/client/chainlock.rsdash-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 thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor 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.rsdash-spv/src/validation/header.rsdash-spv/src/validation/mod.rsdash-spv/src/client/chainlock.rsdash-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.rsdash-spv/src/validation/mod.rsdash-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.rsdash-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.rsdash-spv/src/validation/header.rsdash-spv/src/validation/mod.rsdash-spv/src/client/chainlock.rsdash-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.rsdash-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.rsdash-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.rsdash-spv/src/client/chainlock.rsdash-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.rsdash-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:
- Passed at construction time (as currently done with
InstantLockValidatorand itsMasternodeListEngine)- Added as an optional parameter to
validate()- 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
InstantLockValidatoris correctly constructed with themasternode_enginereference and validation is invoked through theValidatortrait. 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)]withnew()constructor follows Rust conventions. The stateless design is appropriate for header validation.
17-46: Solid parallel validation implementation.The use of
par_iter()withtry_for_eachefficiently validates headers in parallel. The chain continuity check correctly skipsi == 0to avoid out-of-bounds access, and PoW verification uses the cached block hash.One observation: per coding guidelines,
ValidationMode::Basicshould perform "structure and timestamp validation" whileValidationMode::Fullperforms "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 callingvalidate().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
MasternodeListEnginereference 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
Validatortrait implementation correctly sequences structure validation before signature verification. Thenew()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 instantiateInstantLockValidator::new(&masternode_engine). The pattern is consistent across all test cases.Note: Tests call
validate_structuredirectly 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 ifvalidate_structurebehavior changes independently ofvalidate.
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
added
the
merge-conflict
ZocoLini
force-pushed
the
refactor/validators
branch
from
801ce65 to
c9f5bd1
Compare
github-actions
bot
removed
the
merge-conflict
Yeah i like it! Definitely worth exploring further if we can map all validation to it. |
ZocoLini
force-pushed
the
refactor/validators
branch
from
c9f5bd1 to
dc2d22c
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
|
@ZocoLini This has conflicts. |
xdustinface
changed the title
Validator trait
|
I am aware of all the conflicts, de. Will fix them tomorrow |
ZocoLini
force-pushed
the
refactor/validators
branch
from
dc2d22c to
5e505f9
Compare
github-actions
bot
removed
the
merge-conflict
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@dash-spv/src/validation/mod.rs`:
- Around line 3-13: The Validator trait currently forces a single full
validation path by signature fn validate(&self, data: T) ->
ValidationResult<()>; update it to accept or carry a ValidationMode so callers
can choose None/Basic/Full: either add a parameter fn validate(&self, data: T,
mode: ValidationMode) -> ValidationResult<()> or make Validator a struct/trait
with a get_mode()/mode() accessor and store ValidationMode in concrete
validators (e.g., in header::BlockHeaderValidator and
instantlock::InstantLockValidator); then update all call sites to pass or
respect the chosen ValidationMode so validation behavior is configurable across
the validation modules.
| mod header; | ||
| mod instantlock; | ||
|
|
||
| pub use header::BlockHeaderValidator; | ||
| pub use instantlock::InstantLockValidator; | ||
|
|
||
| use crate::error::ValidationResult; | ||
|
|
||
| pub trait Validator<T> { | ||
| fn validate(&self, data: T) -> ValidationResult<()>; | ||
| } |
There was a problem hiding this comment.
ValidationMode not represented in the new Validator trait.
dash-spv/src/validation/** is expected to support ValidationMode::{None,Basic,Full}, but Validator::validate(&self, data) hard-codes a single (full) path. Please thread ValidationMode through the trait or store it in validators and update call sites to preserve configurability. Based on learnings, validation modules should implement configurable validation modes.
🤖 Prompt for AI Agents
In `@dash-spv/src/validation/mod.rs` around lines 3 - 13, The Validator trait
currently forces a single full validation path by signature fn validate(&self,
data: T) -> ValidationResult<()>; update it to accept or carry a ValidationMode
so callers can choose None/Basic/Full: either add a parameter fn validate(&self,
data: T, mode: ValidationMode) -> ValidationResult<()> or make Validator a
struct/trait with a get_mode()/mode() accessor and store ValidationMode in
concrete validators (e.g., in header::BlockHeaderValidator and
instantlock::InstantLockValidator); then update all call sites to pass or
respect the chosen ValidationMode so validation behavior is configurable across
the validation modules.
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
✏️ Tip: You can customize this high-level summary in your review settings.