feat: add peer reputation penalization for invalid ChainLocks#163
feat: add peer reputation penalization for invalid ChainLocks#163PastaPastaPasta merged 13 commits intov0.41-devfrom
Conversation
Add network-level penalization for peers that relay invalid ChainLocks. When an invalid ChainLock is detected, the peer that sent it receives: - Reputation score penalty (INVALID_CHAINLOCK misbehavior score) - 10-minute temporary ban Changes: - Add penalize_last_message_peer() methods to NetworkManager trait - Implement temporary_ban_peer() in PeerReputationManager - Update chainlock.rs to penalize peers on validation errors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughEnhanced ChainLock and InstantLock validation with peer reputation penalties, implemented BLS signature verification for InstantLocks, simplified mempool strategy to FetchAll and BloomFilter, removed InstantLock storage persistence, added InstantLockReceived events, and improved CFHeaders overlap resolution logic. Changes
Sequence Diagram(s)sequenceDiagram
actor Peer
participant Client as DashSpvClient
participant Handler as MessageHandler
participant Validator as InstantLockValidator
participant Engine as MasternodeEngine
participant Network as NetworkManager
participant Events as EventEmitter
Peer->>Client: Send InstantLock
Client->>Handler: handle_inventory (ISLock)
rect rgba(200, 220, 240, 0.3)
Note over Handler: Check Sync & Engine Available
Handler->>Client: is_fully_synced()?
Handler->>Client: get_masternode_engine()?
end
alt Conditions Met
Handler->>Validator: validate(instantlock, engine)
rect rgba(240, 220, 200, 0.3)
Note over Validator: Perform Validation
Validator->>Validator: validate_structure()
Validator->>Engine: verify_is_lock()
end
alt Validation Success
Validator-->>Handler: Ok(())
Handler->>Events: emit(InstantLockReceived)
Events-->>Client: Event(txid, inputs)
else Validation Failed
Validator-->>Handler: Err(ValidationError)
Handler->>Network: penalize_last_message_peer_invalid_instantlock()
Network->>Network: apply_reputation_score()
Network->>Network: temporary_ban_peer(10 min)
Handler-->>Client: SpvError::Validation
end
else Sync Not Ready or Engine Unavailable
rect rgba(240, 200, 200, 0.3)
Note over Handler: Skip Processing
Handler->>Handler: log debug (conditions not met)
end
end
sequenceDiagram
actor Peer
participant Client as DashSpvClient
participant Handler as MessageHandler
participant Validator as ChainLockValidator
participant Network as NetworkManager
Peer->>Client: Send ChainLock
Client->>Handler: handle_chainlock()
Handler->>Validator: validate(chainlock)
alt Validation Success
Validator-->>Handler: Ok(())
Handler->>Client: Process ChainLock
else Validation Failed
rect rgba(240, 200, 200, 0.3)
Note over Handler: Handle Invalid ChainLock
Validator-->>Handler: Err(ValidationError)
Handler->>Network: penalize_last_message_peer_invalid_chainlock()
Network->>Network: apply_reputation_score(INVALID_CHAINLOCK)
Network->>Network: temporary_ban_peer(10 min)
end
Handler-->>Client: SpvError::Validation (early return)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)dash-spv/**/*.rs📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Files:
dash-spv/src/network/**/*.rs📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Files:
**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/src/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-08-25T17:15:59.361ZApplied to files:
📚 Learning: 2025-06-26T16:02:42.390ZApplied to files:
🧬 Code graph analysis (1)dash-spv/src/network/multi_peer.rs (1)
⏰ 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). (19)
🔇 Additional comments (3)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
dash-spv/src/network/mod.rs (1)
158-178: Consider enhancing documentation for the new penalization methods.The method documentation is minimal. Consider adding:
- When these methods are called (e.g., "Called when a peer sends invalid protocol data")
- What happens to the peer (e.g., "Updates reputation score and may result in temporary or permanent ban")
- Thread safety guarantees
- Return value semantics (e.g., "Returns Ok(()) even if no peer is available to penalize")
Example enhancement:
+ /// Penalize the last peer that sent us a message by adjusting reputation. + /// + /// This is typically called when invalid or malicious data is received from a peer. + /// The reputation system will automatically ban the peer if their score exceeds + /// the misbehavior threshold. + /// + /// # Arguments + /// * `score_change` - Positive value to penalize (increases misbehavior score) + /// * `reason` - Human-readable reason for the penalty (used in logs) + /// + /// # Returns + /// Returns `Ok(())` even if no peer is available to penalize. + /// /// Default implementation is a no-op for managers without reputation. async fn penalize_last_message_peer(dash-spv/src/network/multi_peer.rs (1)
1056-1066: Silent no-op when no peer is available to penalize.The method returns
Ok(())whenget_last_message_peer()returnsNone, silently doing nothing. This could mask issues in scenarios where:
- The caller expects a peer to be penalized
- The
last_message_peertracking has a bug- There's a race condition clearing the last message peer
Consider logging at
warnordebuglevel when penalization is requested but no peer is available:async fn penalize_last_message_peer( &self, score_change: i32, reason: &str, ) -> NetworkResult<()> { // Get the last peer that sent us a message if let Some(addr) = self.get_last_message_peer().await { self.reputation_manager.update_reputation(addr, score_change, reason).await; + } else { + log::debug!("Cannot penalize peer: no recent message peer available (reason: {})", reason); } Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dash-spv/src/client/chainlock.rs(1 hunks)dash-spv/src/network/mod.rs(1 hunks)dash-spv/src/network/multi_peer.rs(1 hunks)dash-spv/src/network/reputation.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/src/network/reputation.rsdash-spv/src/network/multi_peer.rsdash-spv/src/client/chainlock.rsdash-spv/src/network/mod.rs
dash-spv/src/network/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Implement DNS-first peer discovery with the specified seeds, immediate startup (no initial discovery delay), and exclusive peer mode when explicit peers are provided
Files:
dash-spv/src/network/reputation.rsdash-spv/src/network/multi_peer.rsdash-spv/src/network/mod.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv/src/network/reputation.rsdash-spv/src/network/multi_peer.rsdash-spv/src/client/chainlock.rsdash-spv/src/network/mod.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv/src/network/reputation.rsdash-spv/src/network/multi_peer.rsdash-spv/src/client/chainlock.rsdash-spv/src/network/mod.rs
🧬 Code graph analysis (3)
dash-spv/src/network/multi_peer.rs (1)
dash-spv/src/network/mod.rs (2)
penalize_last_message_peer(160-166)penalize_last_message_peer_invalid_chainlock(169-178)
dash-spv/src/client/chainlock.rs (1)
dash-spv/src/client/core.rs (2)
chain_state(206-209)storage(171-173)
dash-spv/src/network/mod.rs (1)
dash-spv/src/network/multi_peer.rs (2)
penalize_last_message_peer(1056-1066)penalize_last_message_peer_invalid_chainlock(1068-1084)
⏰ 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). (19)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Key Wallet Components Tests
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_outpoint_string)
🔇 Additional comments (1)
dash-spv/src/network/multi_peer.rs (1)
1068-1084: Approve the invalid ChainLock penalization logic with a note.The implementation correctly applies both a reputation score penalty and a 10-minute temporary ban for invalid ChainLocks, which is appropriate for this type of protocol violation.
However, be aware of the double
ban_countincrement issue flagged indash-spv/src/network/reputation.rslines 316-332. Theupdate_reputationcall at line 1075 may incrementban_countif the score threshold is exceeded, and thentemporary_ban_peerat line 1080 always increments it again. This should be addressed per the recommendation in the reputation.rs review.
| /// Temporarily ban a peer for a specified duration, regardless of score. | ||
| /// This can be used for critical protocol violations (e.g., invalid ChainLocks). | ||
| pub async fn temporary_ban_peer(&self, peer: SocketAddr, duration: Duration, reason: &str) { | ||
| let mut reputations = self.reputations.write().await; | ||
| let reputation = reputations.entry(peer).or_default(); | ||
|
|
||
| reputation.banned_until = Some(Instant::now() + duration); | ||
| reputation.ban_count += 1; | ||
|
|
||
| log::warn!( | ||
| "Peer {} temporarily banned for {:?} (ban #{}, reason: {})", | ||
| peer, | ||
| duration, | ||
| reputation.ban_count, | ||
| reason | ||
| ); | ||
| } |
There was a problem hiding this comment.
Critical: Double ban_count increment for invalid ChainLocks.
The temporary_ban_peer method increments ban_count at line 323. However, in dash-spv/src/network/multi_peer.rs at lines 1074-1081, the invalid ChainLock handling calls both update_reputation (which may increment ban_count at line 246 if score exceeds threshold) AND temporary_ban_peer (which always increments ban_count).
This results in ban_count being incremented twice for a single violation, corrupting the ban statistics.
Solution 1 (recommended): Add a parameter to control whether to increment ban_count:
- pub async fn temporary_ban_peer(&self, peer: SocketAddr, duration: Duration, reason: &str) {
+ pub async fn temporary_ban_peer(&self, peer: SocketAddr, duration: Duration, reason: &str, increment_ban_count: bool) {
let mut reputations = self.reputations.write().await;
let reputation = reputations.entry(peer).or_default();
reputation.banned_until = Some(Instant::now() + duration);
- reputation.ban_count += 1;
+ if increment_ban_count {
+ reputation.ban_count += 1;
+ }
log::warn!(
"Peer {} temporarily banned for {:?} (ban #{}, reason: {})",Then in multi_peer.rs line 1080:
- self.reputation_manager
- .temporary_ban_peer(addr, Duration::from_secs(10 * 60), reason)
- .await;
+ self.reputation_manager
+ .temporary_ban_peer(addr, Duration::from_secs(10 * 60), reason, false)
+ .await;Solution 2: Only call temporary_ban_peer without calling update_reputation first, and have temporary_ban_peer handle both the ban and score update.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Temporarily ban a peer for a specified duration, regardless of score. | |
| /// This can be used for critical protocol violations (e.g., invalid ChainLocks). | |
| pub async fn temporary_ban_peer(&self, peer: SocketAddr, duration: Duration, reason: &str) { | |
| let mut reputations = self.reputations.write().await; | |
| let reputation = reputations.entry(peer).or_default(); | |
| reputation.banned_until = Some(Instant::now() + duration); | |
| reputation.ban_count += 1; | |
| log::warn!( | |
| "Peer {} temporarily banned for {:?} (ban #{}, reason: {})", | |
| peer, | |
| duration, | |
| reputation.ban_count, | |
| reason | |
| ); | |
| } | |
| /// Temporarily ban a peer for a specified duration, regardless of score. | |
| /// This can be used for critical protocol violations (e.g., invalid ChainLocks). | |
| pub async fn temporary_ban_peer(&self, peer: SocketAddr, duration: Duration, reason: &str, increment_ban_count: bool) { | |
| let mut reputations = self.reputations.write().await; | |
| let reputation = reputations.entry(peer).or_default(); | |
| reputation.banned_until = Some(Instant::now() + duration); | |
| if increment_ban_count { | |
| reputation.ban_count += 1; | |
| } | |
| log::warn!( | |
| "Peer {} temporarily banned for {:?} (ban #{}, reason: {})", | |
| peer, | |
| duration, | |
| reputation.ban_count, | |
| reason | |
| ); | |
| } |
🤖 Prompt for AI Agents
In dash-spv/src/network/reputation.rs around lines 316 to 332,
temporary_ban_peer always increments reputation.ban_count causing duplicate
increments when callers also call update_reputation (e.g., multi_peer.rs lines
1074-1081); modify temporary_ban_peer to accept a boolean flag like
increment_ban_count: bool (default true) and only increment ban_count when that
flag is true, then update the multi_peer.rs call handling invalid ChainLocks to
call temporary_ban_peer(..., false) (or remove the extra update_reputation call)
so a single violation increments ban_count exactly once.
The is_multiple_of method does not exist in Rust's standard library. Use the modulo operator (%) instead for checking divisibility by 1000. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Refactor arithmetic to avoid potential underflow when filter_hashes is empty. Use saturating_sub for the offset calculation before applying it to stop_height. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ation Implements comprehensive InstantLock validation with cryptographic verification and peer reputation penalization for invalid messages. InstantLock Validation: - Add full BLS signature verification using MasternodeListEngine - Implement DIP 24 cyclehash-based quorum selection for signature validation - Add structural validation: null checks for txid, signature, and inputs - Split validation into validate_structure() and full validate() methods - Require masternode engine for signature verification (security critical) Peer Reputation System: - Add INVALID_INSTANTLOCK misbehavior score (35 points) - Penalize peers relaying invalid InstantLocks with 10-minute temporary ban - Add convenience method penalize_last_message_peer_invalid_instantlock() - Integrate peer penalization into InstantLock processing flow Quorum Manager: - Implement BLS signature verification using blsful library - Verify signatures against quorum public keys with proper error handling - Add detailed logging for signature verification success/failure Event System: - Add SpvEvent::InstantLockReceived event for validated InstantLocks - Emit events after successful validation in chainlock handler - Update FFI client to handle InstantLock events (with TODO for callbacks) Storage Cleanup: - Remove store_instant_lock() and load_instant_lock() methods - InstantLocks are ephemeral and validated on receipt, not persisted - Simplify storage trait by removing unused InstantLock persistence Error Handling: - Add InvalidSignature error variant for BLS verification failures - Improve error messages with context (quorum type, height, reason) Testing: - Add comprehensive unit tests for null checks and validation paths - Remove outdated instantsend_integration_test.rs (WalletManager API changed) - Tests verify structural validation and request ID computation Security: Never accept InstantLocks from network without full BLS signature verification. This implementation ensures cryptographic validation using the proper quorum selected via DIP 24 cyclehash mechanism. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…non-overlapping suffix Compute headers from CFHeaders message and verify continuity at expected overlap boundary; store only the non-overlapping tail. Remove invalid variable usage.
Remove the Selective mempool strategy which was not fully implemented and relied on wallet integration that doesn't exist yet. The Selective strategy only tracked transactions that the user recently sent, making it ineffective for monitoring mempool transactions and InstantLocks. Changes: - Remove MempoolStrategy::Selective enum variant - Remove recent_send_window_secs configuration field - Remove with_recent_send_window() configuration method - Remove record_send() and related recent send tracking logic - Remove record_transaction_send() from client API - Remove Selective validation from config validation - Update MempoolFilter::new() to remove Duration parameter - Update all MempoolFilter instantiations throughout codebase - Remove test_selective_strategy() test - Update all remaining tests to use FetchAll instead of Selective - Set MempoolStrategy::FetchAll as the new default FFI Changes: - Remove FFIMempoolStrategy::Selective enum variant - Update dash_spv_ffi_config_get_mempool_strategy() default to FetchAll - Remove record_transaction_send() call from FFI client With FetchAll strategy: - Client fetches all announced mempool transactions (up to capacity limit) - InstantLocks will be received and processed for all transactions - Higher bandwidth usage but complete mempool visibility - Suitable for monitoring network activity and testing The BloomFilter strategy remains available for future privacy-focused implementations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…nced Prevents ISLOCK validation errors and peer bans during header sync when masternode engine/quorums are not yet available.
…uplicate requests and off-by-one
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
dash-spv/src/mempool_filter.rs (1)
421-426: Incomplete removal: Selective strategy and recent_send_window still present throughout codebase.Verification found active references to the supposedly removed
Selectivestrategy andrecent_send_windowin multiple locations:
MempoolStrategy::Selectivestill used in test files (test_handshake_logic.rs, network/tests.rs, client/mod.rs, message_handler_test.rs, config_test.rs)recent_send_window_secsconfig field still present in network/tests.rs and validated in config_test.rsrecent_sendsHashMap still defined in types.rsEither the removal is incomplete or these items were not intended to be removed. Update all test files and configuration validation to use
FetchAllstrategy, or clarify the intended scope of removal.dash-spv/src/network/multi_peer.rs (1)
1326-1329: Infinite recursion bug in trait method implementation.async fn get_last_message_peer_id(&self) calls itself, causing recursion. Call the inherent method explicitly.
- async fn get_last_message_peer_id(&self) -> crate::types::PeerId { - // Call the instance method to avoid code duplication - self.get_last_message_peer_id().await - } + async fn get_last_message_peer_id(&self) -> crate::types::PeerId { + // Call the inherent method explicitly to avoid recursion + MultiPeerNetworkManager::get_last_message_peer_id(self).await + }This is unrelated to the penalization change but blocks correctness.
dash-spv/src/sync/filters/headers.rs (2)
162-167: Do not treat empty CFHeaders as “sync complete” in the sequential path.Peers can prematurely terminate sync by replying empty to a non-tip request. Treat as invalid/ignore, not completion.
Suggested minimal change:
- if cf_headers.filter_hashes.is_empty() { - // Empty response indicates end of sync - self.syncing_filter_headers = false; - return Ok(false); - } + if cf_headers.filter_hashes.is_empty() { + tracing::warn!("Empty CFHeaders response for sequential request; treating as invalid and continuing"); + return Err(SyncError::Validation("Empty CFHeaders response".into())); + }
915-920: Flow-control: empty CFHeaders must not end the whole sync.Same issue here; empty response for one batch should neither clear state nor flip syncing off.
Apply:
- if cf_headers.filter_hashes.is_empty() { - tracing::info!("Received empty CFHeaders response - sync complete"); - self.syncing_filter_headers = false; - self.clear_cfheader_flow_control_state(); - return Ok(false); - } + if cf_headers.filter_hashes.is_empty() { + tracing::warn!("Empty CFHeaders response under flow control; ignoring and continuing"); + return Ok(true); + }
♻️ Duplicate comments (3)
dash-spv/src/client/chainlock.rs (2)
40-49: Don’t discard penalization errors; avoid race on last_message_peer.
- let _ = drops failures; log at least a warning.
- Reading last_message_peer after awaits can penalize the wrong peer. Capture sender at receipt and pass it down instead of relying on mutable shared state. Same issue previously noted.
Apply minimal logging now:
- let _ = self.network.penalize_last_message_peer_invalid_chainlock(&reason).await; + if let Err(pe) = self.network.penalize_last_message_peer_invalid_chainlock(&reason).await { + tracing::warn!("Failed to penalize peer for invalid ChainLock: {}", pe); + }Follow-up (separate PR likely): change signature to thread sender explicitly.
- pub async fn process_chainlock(&mut self, chainlock: dashcore::ephemerealdata::chain_lock::ChainLock) + pub async fn process_chainlock(&mut self, chainlock: dashcore::ephemerealdata::chain_lock::ChainLock, sender: std::net::SocketAddr)Then call an explicit API:
- self.network.penalize_last_message_peer_invalid_chainlock(&reason).await + self.network.penalize_peer_invalid_chainlock(sender, &reason).await#!/bin/bash # Identify all last_message_peer reads and potential races across awaits rg -nP --type=rust -C2 'last_message_peer' ; rg -nP --type=rust '\.process_chainlock\(|process_instantsendlock\(' -C3
106-117: InstantLock penalize: same two issues (error drop + last_message_peer race).Mirror the ChainLock fix: log penalize errors and move toward explicit sender threading.
- let _ = self.network.penalize_last_message_peer_invalid_instantlock(&reason).await; + if let Err(pe) = self.network.penalize_last_message_peer_invalid_instantlock(&reason).await { + tracing::warn!("Failed to penalize peer for invalid InstantLock: {}", pe); + }dash-spv/src/network/reputation.rs (1)
319-335: Avoid double-incrementing ban_count when combining score-based bans and temporary bans.Currently increments ban_count here and also during update_reputation-triggered bans; results in +2 for one violation (previously reported).
Option A (minimal):
-pub async fn temporary_ban_peer(&self, peer: SocketAddr, duration: Duration, reason: &str) { +pub async fn temporary_ban_peer(&self, peer: SocketAddr, duration: Duration, reason: &str, increment_ban_count: bool) { let mut reputations = self.reputations.write().await; let reputation = reputations.entry(peer).or_default(); reputation.banned_until = Some(Instant::now() + duration); - reputation.ban_count += 1; + if increment_ban_count { + reputation.ban_count += 1; + }Then call sites that already increment via update_reputation should pass false.
Option B: centralize — temporary_ban_peer applies ban only; ban_count changes occur in one place.
🧹 Nitpick comments (13)
dash-spv/src/mempool_filter.rs (1)
46-46: _txid intentionally unused — OK; consider removing param if no future use planned.If txid isn’t needed long-term, simplify signature to avoid misleading API.
dash-spv/src/types.rs (1)
902-909: Unify txid types across SpvEvent for consistency.TransactionDetected.txid is String (dash-spv/src/types.rs:861) while new InstantLockReceived.txid is Txid (dash-spv/src/types.rs:905). Prefer Txid everywhere to avoid conversions and reduce mistakes across FFI/event consumers.
dash-spv-ffi/src/client.rs (1)
1439-1489: Clarify the purpose of the now-empty record_send function.The function
dash_spv_ffi_client_record_sendparses and validates atxidparameter but no longer performs any recording operation (the call tospv_client.record_transaction_send(txid).awaithas been removed). This leaves the function as a no-op that accepts an unused parameter.Consider one of the following approaches:
- Add a doc comment explaining that this function is deprecated/no-op (if maintaining FFI compatibility)
- Remove the function entirely if it's no longer needed
- Return an appropriate error code indicating the operation is not supported
Example documentation if keeping for compatibility:
+/// Records that a transaction was sent (deprecated - no longer performs any operation). +/// +/// This function is maintained for FFI compatibility but does not record transaction sends. +/// It validates the txid parameter and returns success, but performs no actual recording. +/// /// # Safety /// - `client` and `txid` must be valid, non-null pointers. #[no_mangle] pub unsafe extern "C" fn dash_spv_ffi_client_record_send(dash-spv/src/validation/quorum.rs (3)
101-138: Consider honoring quorum.is_active in selection.get_quorum_for_validation ignores the QuorumInfo.is_active flag; add a simple check to avoid using inactive quorums.
- for ((q_type, height), quorum) in &self.quorums { + for ((q_type, height), quorum) in &self.quorums { if *q_type != quorum_type { continue; } + if !quorum.is_active { + continue; + }
157-161: Formatting: avoid relying on LowerHex for BlockHash.Using {:x} assumes LowerHex; prefer Display or explicit hex encoding to avoid trait surprises across versions.
- debug!( - "Verifying {:?} signature with quorum from height {} (quorum hash: {:x})", - quorum_type, quorum.height, quorum.quorum_hash - ); + debug!( + "Verifying {:?} signature with quorum from height {} (quorum hash: {})", + quorum_type, + quorum.height, + quorum.quorum_hash + );
162-200: Good hardening on key length + conversions; add explicit scheme/DST check.The conversion and verify path looks correct. To avoid subtle mismatches, ensure the verification uses the same scheme/DST as Dash’s consensus (e.g., Basic scheme for BLS12-381). If blsful defaults differ, pass explicit params or use the dashcore-provided verify helper.
- If dashcore exposes a consensus verify helper, prefer it to avoid DST drift.
- Otherwise, add a unit test vector using a known-good quorum pubkey/signature pair to pin behavior.
dash-spv/src/validation/instantlock.rs (2)
60-90: Verify BLSSignature::is_zeroed method availability across MSRV and features.Some versions expose is_zero or require manual zero-check. If is_zeroed isn’t available in your locked dashcore, switch to a byte-equality check to keep clippy clean.
- if instant_lock.signature.is_zeroed() { + if instant_lock.signature.as_bytes().iter().all(|&b| b == 0) {As per coding guidelines.
94-122: Signature verify via MasternodeListEngine: good; preserve engine error context.You wrap errors into InvalidSignature, which is fine, but consider preserving the engine’s error type string fully to aid ops debugging.
No code change required; just ensure error strings aren’t truncated upstream.
dash-spv/src/sync/filters/headers.rs (5)
73-77: Batch start height calc looks correct; add a small guard.Computation is right (stop_height − (count−1)). Add a debug assertion that count > 0 to catch malformed CFHeaders early.
Apply:
fn calculate_batch_start_height(cf_headers: &CFHeaders, stop_height: u32) -> u32 { - let count = cf_headers.filter_hashes.len() as u32; + let count = cf_headers.filter_hashes.len() as u32; + debug_assert!(count > 0, "CFHeaders.filter_hashes must be non-empty"); let offset = count.saturating_sub(1); stop_height.saturating_sub(offset) }
1193-1201: Continuity mismatch should penalize the sending peer.When overlap continuity fails, we only log/ignore. Recommend penalizing the last-message peer (light score) to discourage bogus CFHeaders. You can bubble up a reason from this function or handle in the caller upon zero-stored with a mismatch indicator.
If you prefer, I can draft an OverlapResult enum (Stored(n), AlreadyProcessed, MissingLocal, Mismatch) and wire peer penalization in the caller.
1223-1225: Use saturating add for safety on height arithmetic.Low risk today, but this is a hot path and saturating math avoids accidental wrap.
Apply:
- let new_current_height = expected_start_height + new_filter_headers.len() as u32; + let new_current_height = + expected_start_height.saturating_add(new_filter_headers.len() as u32);
803-854: Queue build is strict; consider fallback like sequential path.Here we hard-fail if batch_end header is missing. The sequential path scans backward to the nearest available header. Mirroring that here would improve resilience on sparse storage or fast-moving tips.
I can adapt find_available_header_at_or_before() usage here with minimal code if you want.
1148-1155: Endianness verified correct per BIP158; optional improvements: test vector and Hash engine.Web search confirms BIP158 specifies double-SHA256 over raw 32-byte binary concatenation (filter_hash || prev_header). The code correctly implements this:
sha256d::Hash::hash()operates on canonical bytes fromas_byte_array()and produces FilterHeader viato_byte_array(). Byte order is handled correctly by the dashcore types.Optional enhancements:
- Add a unit test with a known BIP158 vector to validate the implementation.
- Consider using a Hash engine (from dashcore_hashes) instead of the 64-byte temporary buffer for efficiency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
dash-spv-ffi/include/dash_spv_ffi.h(1 hunks)dash-spv-ffi/src/client.rs(1 hunks)dash-spv-ffi/src/config.rs(1 hunks)dash-spv-ffi/src/types.rs(0 hunks)dash-spv/src/client/chainlock.rs(2 hunks)dash-spv/src/client/config.rs(2 hunks)dash-spv/src/client/lifecycle.rs(0 hunks)dash-spv/src/client/mempool.rs(0 hunks)dash-spv/src/client/message_handler.rs(2 hunks)dash-spv/src/client/sync_coordinator.rs(2 hunks)dash-spv/src/error.rs(1 hunks)dash-spv/src/main.rs(2 hunks)dash-spv/src/mempool_filter.rs(7 hunks)dash-spv/src/network/mod.rs(1 hunks)dash-spv/src/network/multi_peer.rs(1 hunks)dash-spv/src/network/reputation.rs(2 hunks)dash-spv/src/storage/disk/headers.rs(1 hunks)dash-spv/src/storage/disk/state.rs(0 hunks)dash-spv/src/storage/memory.rs(0 hunks)dash-spv/src/storage/mod.rs(0 hunks)dash-spv/src/sync/filters/headers.rs(3 hunks)dash-spv/src/sync/sequential/manager.rs(1 hunks)dash-spv/src/sync/sequential/post_sync.rs(1 hunks)dash-spv/src/types.rs(1 hunks)dash-spv/src/validation/instantlock.rs(9 hunks)dash-spv/src/validation/mod.rs(1 hunks)dash-spv/src/validation/quorum.rs(3 hunks)dash-spv/tests/instantsend_integration_test.rs(0 hunks)
💤 Files with no reviewable changes (7)
- dash-spv/src/storage/memory.rs
- dash-spv/src/storage/mod.rs
- dash-spv/src/client/lifecycle.rs
- dash-spv/tests/instantsend_integration_test.rs
- dash-spv/src/client/mempool.rs
- dash-spv/src/storage/disk/state.rs
- dash-spv-ffi/src/types.rs
🧰 Additional context used
📓 Path-based instructions (10)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/src/error.rsdash-spv/src/validation/mod.rsdash-spv/src/storage/disk/headers.rsdash-spv/src/types.rsdash-spv/src/client/sync_coordinator.rsdash-spv/src/sync/sequential/manager.rsdash-spv/src/validation/instantlock.rsdash-spv/src/network/reputation.rsdash-spv/src/sync/filters/headers.rsdash-spv/src/network/mod.rsdash-spv/src/sync/sequential/post_sync.rsdash-spv/src/mempool_filter.rsdash-spv/src/client/config.rsdash-spv/src/client/chainlock.rsdash-spv/src/validation/quorum.rsdash-spv/src/main.rsdash-spv/src/network/multi_peer.rsdash-spv/src/client/message_handler.rs
dash-spv/src/error.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Define and maintain domain-specific error types in
src/error.rs(NetworkError, StorageError, SyncError, ValidationError, SpvError) and update them when adding new failure modes
Files:
dash-spv/src/error.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv/src/error.rsdash-spv-ffi/src/config.rsdash-spv/src/validation/mod.rsdash-spv/src/storage/disk/headers.rsdash-spv/src/types.rsdash-spv/src/client/sync_coordinator.rsdash-spv-ffi/src/client.rsdash-spv/src/sync/sequential/manager.rsdash-spv/src/validation/instantlock.rsdash-spv/src/network/reputation.rsdash-spv/src/sync/filters/headers.rsdash-spv/src/network/mod.rsdash-spv/src/sync/sequential/post_sync.rsdash-spv/src/mempool_filter.rsdash-spv/src/client/config.rsdash-spv/src/client/chainlock.rsdash-spv/src/validation/quorum.rsdash-spv/src/main.rsdash-spv/src/network/multi_peer.rsdash-spv/src/client/message_handler.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv/src/error.rsdash-spv-ffi/src/config.rsdash-spv/src/validation/mod.rsdash-spv/src/storage/disk/headers.rsdash-spv/src/types.rsdash-spv/src/client/sync_coordinator.rsdash-spv-ffi/src/client.rsdash-spv/src/sync/sequential/manager.rsdash-spv/src/validation/instantlock.rsdash-spv/src/network/reputation.rsdash-spv/src/sync/filters/headers.rsdash-spv/src/network/mod.rsdash-spv/src/sync/sequential/post_sync.rsdash-spv/src/mempool_filter.rsdash-spv/src/client/config.rsdash-spv/src/client/chainlock.rsdash-spv/src/validation/quorum.rsdash-spv/src/main.rsdash-spv/src/network/multi_peer.rsdash-spv/src/client/message_handler.rs
dash-spv-ffi/src/**/*.rs
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
dash-spv-ffi/src/**/*.rs: FFI functions must be declared with #[no_mangle] extern "C"
Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Provide corresponding _destroy() functions for all FFI-owned types to enforce explicit memory management
Return Rust strings to C as *const c_char; allocate so the caller can free with dash_string_free
Accept input strings from C as *const c_char (borrowed; do not free inputs)
Functions returning pointers transfer ownership to the caller; functions taking pointers only borrow
Add cbindgen annotations for complex types so headers are generated correctly
Files:
dash-spv-ffi/src/config.rsdash-spv-ffi/src/client.rs
{dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries (C/Swift interop)
Files:
dash-spv-ffi/src/config.rsdash-spv-ffi/src/client.rsdash-spv-ffi/include/dash_spv_ffi.h
*-ffi/**
📄 CodeRabbit inference engine (AGENTS.md)
FFI bindings live in *-ffi crates
Files:
dash-spv-ffi/src/config.rsdash-spv-ffi/src/client.rsdash-spv-ffi/include/dash_spv_ffi.h
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Provide both
MemoryStorageManagerandDiskStorageManagerbehind theStorageManagertrait
Files:
dash-spv/src/storage/disk/headers.rs
dash-spv/src/network/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Implement DNS-first peer discovery with the specified seeds, immediate startup (no initial discovery delay), and exclusive peer mode when explicit peers are provided
Files:
dash-spv/src/network/reputation.rsdash-spv/src/network/mod.rsdash-spv/src/network/multi_peer.rs
dash-spv-ffi/include/dash_spv_ffi.h
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
The public C header is generated; ensure it reflects FFI changes after builds
Files:
dash-spv-ffi/include/dash_spv_ffi.h
🧠 Learnings (7)
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/error.rs : Define and maintain domain-specific error types in `src/error.rs` (NetworkError, StorageError, SyncError, ValidationError, SpvError) and update them when adding new failure modes
Applied to files:
dash-spv/src/error.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-spv,rpc-integration-test}/tests/**/*.rs : Provide integration tests for network operations
Applied to files:
dash-spv/src/validation/instantlock.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/psbt.rs : Place PSBT serialization/deserialization tests in tests/psbt.rs
Applied to files:
dash-spv/src/validation/instantlock.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/src/mempool_filter.rsdash-spv/src/client/config.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Applied to files:
dash-spv/src/main.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/include/dash_spv_ffi.h : The public C header is generated; ensure it reflects FFI changes after builds
Applied to files:
dash-spv-ffi/include/dash_spv_ffi.h
🧬 Code graph analysis (7)
dash-spv/src/error.rs (1)
dash-spv-ffi/src/platform_integration.rs (1)
error(22-28)
dash-spv/src/sync/sequential/manager.rs (2)
dash-spv/src/validation/quorum.rs (1)
new(80-85)dash-spv/src/sync/sequential/lifecycle.rs (1)
new(31-62)
dash-spv/src/validation/instantlock.rs (3)
dash-spv/src/sync/sequential/manager.rs (1)
masternode_list_engine(202-206)dash-spv/src/validation/mod.rs (1)
new(25-31)dash/src/ephemerealdata/instant_lock.rs (1)
request_id(58-77)
dash-spv/src/sync/filters/headers.rs (2)
dash-spv/src/storage/disk/manager.rs (1)
new(79-129)dash-spv/src/sync/filters/manager.rs (1)
new(111-154)
dash-spv/src/network/mod.rs (1)
dash-spv/src/network/multi_peer.rs (3)
penalize_last_message_peer(1056-1066)penalize_last_message_peer_invalid_chainlock(1068-1084)penalize_last_message_peer_invalid_instantlock(1086-1102)
dash-spv/src/client/chainlock.rs (4)
dash-spv/src/network/multi_peer.rs (1)
new(79-130)dash-spv/src/validation/instantlock.rs (1)
new(22-24)dash-spv/src/validation/mod.rs (1)
new(25-31)dash-spv/src/client/lifecycle.rs (1)
new(36-105)
dash-spv/src/network/multi_peer.rs (1)
dash-spv/src/network/mod.rs (3)
penalize_last_message_peer(160-166)penalize_last_message_peer_invalid_chainlock(169-178)penalize_last_message_peer_invalid_instantlock(181-190)
⏰ 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). (18)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: Core Components Tests
🔇 Additional comments (20)
dash-spv/src/sync/sequential/post_sync.rs (1)
356-358: LGTM! CFHeaders start height calculation is correct.The simplified logic correctly requests filter headers starting from the first missing height (filter_tip + 1). The guard condition at line 349 ensures filter_tip < stop_height, making this calculation safe and valid.
dash-spv/src/client/chainlock.rs (2)
98-104: Masternode engine requirement propagated as Validation error — LGTM.
125-130: InstantLockReceived event emission verified — all match arms properly updated.Verification confirms the
InstantLockReceivedevent emission inchainlock.rsis correct, and all 10SpvEventvariants (includingInstantLockReceived) are exhaustively matched in the FFI layer (dash-spv-ffi/src/client.rs:299-304). The match statement is complete with no gaps.dash-spv/src/network/reputation.rs (1)
44-46: New misbehavior score INVALID_INSTANTLOCK — LGTM.dash-spv/src/client/config.rs (1)
230-234: Critical: FFI mempool strategy implementation is incomplete and inconsistent.The PR default of
FetchAllis fine, but verification revealed a broader issue:
- FFI type mismatch:
dash-spv-ffi/src/types.rsdefinesFFIMempoolStrategywith onlyFetchAllandBloomFilter, but FFI headers (dash-spv-ffi/include/dash_spv_ffi.h) define it withFetchAll,BloomFilter, andSelective(lines 19-22 in both files).- Incomplete conversion:
From<MempoolStrategy> for FFIMempoolStrategyatdash-spv-ffi/src/types.rs:400-407only handlesFetchAllandBloomFilter—no match arm forSelective, which will cause a compilation error ifMempoolStrategy::Selectiveis ever converted.- Documentation mismatch:
dash-spv-ffi/include/dash_spv_ffi.h:820documents thatdash_spv_ffi_config_get_mempool_strategyreturnsSelectiveas default when null, but the actual implementation (dash-spv-ffi/src/config.rs:512) returnsFetchAll.- Test inconsistency:
dash-spv/src/client/config_test.rs:207usesMempoolStrategy::Selective, but the FFI layer cannot currently handle it.Update
dash-spv-ffi/src/types.rsto includeSelectivein theFFIMempoolStrategyenum and add the corresponding match arm in the conversion logic. Align FFI header documentation with the actual implementation.⛔ Skipped due to learnings
Learnt from: DCG-Claude PR: dashpay/rust-dashcore#0 File: :0-0 Timestamp: 2025-06-26T16:01:37.609Z Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.dash-spv/src/main.rs (2)
88-93: LGTM - Clean CLI integration for mempool control.The new
--no-mempoolflag provides a clear runtime override for disabling mempool tracking, complementing the removal of the Selective mempool strategy.
192-194: LGTM - Correct flag implementation.The flag is correctly wired to the config field, providing the expected behavior.
dash-spv-ffi/src/config.rs (1)
506-512: LGTM - Default strategy update aligns with codebase changes.The shift from
SelectivetoFetchAllas the default mempool strategy when config is null is consistent with the broader removal of the Selective strategy variant across the codebase.dash-spv-ffi/src/client.rs (1)
299-304: LGTM - Placeholder for future InstantLock callback.The TODO comment clearly indicates that FFI callback support for InstantLock notifications will be added later. The placeholder structure is appropriate.
dash-spv/src/error.rs (1)
145-147: LGTM - Clean error variant addition.The new
InvalidSignaturevariant follows the established pattern and provides necessary support for BLS signature validation failures in InstantLock processing.dash-spv/src/client/message_handler.rs (1)
406-417: LGTM - Appropriate gating for InstantLock fetching.The guard conditions ensure InstantSendLocks are only fetched when the client is fully synced and has the masternode engine required for validation. This prevents wasted bandwidth and potential validation failures.
dash-spv/src/client/sync_coordinator.rs (2)
56-60: LGTM - Clearer log messaging.The updated log messages more accurately describe the sync preparation phase and clarify that actual sync requests will be triggered by the monitoring loop.
615-624: LGTM - Consistent InstantLock gating logic.The guard conditions match the gating implemented in
message_handler.rs(lines 407-416), providing defense-in-depth by ensuring InstantLocks are only processed when the client is fully synced and has masternode data available for validation.dash-spv/src/validation/mod.rs (1)
62-76: Architectural split correctly implemented; callers properly validate with signature verification.The
validate_instantlock()method now performs structural validation only (viavalidate_structure()), as documented. Network message processing correctly handles full validation:process_instantsendlock()indash-spv/src/client/chainlock.rs:108callsInstantLockValidator::validate()with the masternode engine, performing BLS signature verification. Orchestration insync_coordinator.rs:614-625ensures the masternode engine is available before processing.dash-spv/src/sync/sequential/manager.rs (1)
230-237: The review comment is based on incorrect assumptions about current usage.The
get_quorum_manager()method has zero callers in the codebase. It's a new public API being added as a documented placeholder with clear TODO comments explaining its current limitations. Since no callers exist, the concern about callers misusing an empty manager is not actionable—the method cannot be misused if nothing calls it yet. The implementation is appropriate for staged feature infrastructure.Likely an incorrect or invalid review comment.
dash-spv/src/network/mod.rs (1)
158-190: Trait-level penalization helpers look good; default no-op is appropriate.Forwarders to penalize_last_message_peer keep the trait surface minimal for non-reputation managers. No issues.
Please ensure all concrete NetworkManager impls that have reputation override penalize_last_message_peer to avoid silent no-ops.
dash-spv/src/validation/instantlock.rs (1)
26-52: Full validation path is correct and clearly documented.Structure-first then signature via MasternodeListEngine is the right contract. No issues.
dash-spv/src/network/multi_peer.rs (2)
1053-1055: No functional change here.Closing braces/comments only.
1056-1066: Core penalize hook LGTM.Updates reputation of the last sender; simple and safe.
dash-spv-ffi/include/dash_spv_ffi.h (1)
819-823: ****The
Selectivestrategy was not removed—it remains in theFFIMempoolStrategyenum with value 2, continues to be used throughout the Rust implementation, tests, and Swift bindings (seeMempoolTypes.swift:68withpublic static func selective()). No enum value shifts occurred. Additionally, the header file documentation at line 820 still states the default isFFIMempoolStrategy::Selective, notFetchAllas shown in your snippet. The codebase shows no evidence of the changes described in this review comment. Verify the PR changeset is correctly applied to the files under review.Likely an incorrect or invalid review comment.
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dash-spv/src/sync/filters/headers.rs (1)
1166-1210: Consider adding inline comments for complex boundary validation logic.The boundary validation and suffix extraction logic is correct but involves multiple computed indices (
steps_to_expected_prev,start_index) and conditional paths. While the implementation is sound, adding a few inline comments explaining the index calculations and why they're safe would help future maintainers understand this critical verification path more quickly.Example:
// Determine the computed header at expected_prev_height using the batch data let steps_to_expected_prev = expected_start_height.saturating_sub(original_start_height); let computed_prev_at_expected = if steps_to_expected_prev == 0 { // Expected prev is before this batch; use the message's previous_filter_header cf_headers.previous_filter_header } else { // Expected prev is within this batch; index is valid because: // 1. steps_to_expected_prev > 0 (checked by if condition) // 2. steps_to_expected_prev <= headers_to_skip < filter_hashes.len() (checked at line 1135) computed_headers[(steps_to_expected_prev - 1) as usize] };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dash-spv-ffi/src/client.rs(2 hunks)dash-spv/src/client/lifecycle.rs(0 hunks)dash-spv/src/client/mempool.rs(0 hunks)dash-spv/src/sync/filters/headers.rs(3 hunks)dash-spv/src/validation/instantlock.rs(8 hunks)
💤 Files with no reviewable changes (2)
- dash-spv/src/client/mempool.rs
- dash-spv/src/client/lifecycle.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv-ffi/src/client.rs
🧰 Additional context used
📓 Path-based instructions (3)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/src/sync/filters/headers.rsdash-spv/src/validation/instantlock.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv/src/sync/filters/headers.rsdash-spv/src/validation/instantlock.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv/src/sync/filters/headers.rsdash-spv/src/validation/instantlock.rs
🧠 Learnings (1)
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Applied to files:
dash-spv/src/validation/instantlock.rs
🧬 Code graph analysis (2)
dash-spv/src/sync/filters/headers.rs (2)
dash-spv/src/storage/disk/manager.rs (1)
new(79-129)dash-spv/src/sync/filters/manager.rs (1)
new(111-154)
dash-spv/src/validation/instantlock.rs (4)
dash-spv/src/sync/sequential/manager.rs (1)
masternode_list_engine(202-206)dash-spv/src/validation/mod.rs (1)
new(25-31)dash-spv/src/sync/masternodes.rs (1)
new(48-111)dash/src/ephemerealdata/instant_lock.rs (1)
request_id(58-77)
⏰ 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). (19)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Key Wallet Components Tests
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deser_net_msg)
🔇 Additional comments (6)
dash-spv/src/sync/filters/headers.rs (2)
73-77: LGTM: Safe arithmetic for batch height calculation.The use of
saturating_subproperly prevents underflow when calculating the batch start height from the stop height and filter count.
1142-1155: LGTM: Correct filter header chain computation.The on-the-fly computation correctly implements the BIP 157/158 filter header chaining:
hash(filter_hash || prev_filter_header). The pre-allocated vector and proper prev_header chaining are well implemented.dash-spv/src/validation/instantlock.rs (4)
61-65: LGTM: Good security check for null txid.Rejecting InstantLocks with zero txids prevents processing of malformed messages and aligns with the broader validation hardening visible throughout this PR.
82-89: LGTM: Thorough input validation.The per-input null txid check with index-specific error messages is excellent defensive programming and helps diagnose malformed network data.
94-122: LGTM: Correct BLS signature verification integration.The signature validation properly delegates to the masternode engine's
verify_is_lock(), which implements DIP 24 quorum selection using cyclehash. The error mapping and debug logging are appropriate for this security-critical path.
26-52: LGTM: Well-documented full validation flow.The
validate()method properly combines structural and cryptographic validation with clear security warnings. The documentation correctly emphasizes that signature verification is required for accepting network messages, which aligns with the PR's goal of adding peer penalization for invalid data.
| //! InstantLock validation functionality. | ||
|
|
||
| use dashcore::InstantLock; | ||
| use dashcore_hashes::Hash; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the Hash import is needed outside tests.
The dashcore_hashes::Hash import appears to be used only in the test module (line 156 for sha256d::Hash). If it's not required for the main code (e.g., if Txid::all_zeros() doesn't need the trait in scope), this will trigger a clippy warning about unused imports in non-test builds.
Run clippy to verify:
If the import is test-only, move it into the test module:
-use dashcore_hashes::Hash;
use crate::error::{ValidationError, ValidationResult};
// ... rest of code ...
#[cfg(test)]
mod tests {
use super::*;
+ use dashcore_hashes::Hash;
use dashcore::blockdata::constants::COIN_VALUE;As per coding guidelines (clippy -D warnings).
🏁 Script executed:
#!/bin/bash
# Check for unused imports in this file
cargo clippy --all-targets --all-features -- -D warnings 2>&1 | grep -A 5 "instantlock.rs"Length of output: 0
🏁 Script executed:
#!/bin/bash
# View the file to see imports and Hash usage
cat dash-spv/src/validation/instantlock.rs | head -60Length of output: 2094
🏁 Script executed:
#!/bin/bash
# Search for all Hash usages in the file with context
rg -n "Hash" dash-spv/src/validation/instantlock.rs -C 3Length of output: 6905
🏁 Script executed:
#!/bin/bash
# Look for test module and Hash usage there
rg -n "#\[cfg\(test\)\]" dash-spv/src/validation/instantlock.rs -A 50 | head -70Length of output: 1825
Move dashcore_hashes::Hash import into the test module.
The Hash trait import at line 4 is not used in the main code. It only appears inside the test module (line 156, already re-imported). Moving the import will eliminate the unused import warning and follow Rust best practices for test-only dependencies.
-use dashcore_hashes::Hash;
use crate::error::{ValidationError, ValidationResult};Then in the test module at line 156, the import remains:
#[cfg(test)]
mod tests {
use super::*;
use dashcore::blockdata::constants::COIN_VALUE;
use dashcore::{OutPoint, ScriptBuf, Transaction, TxIn, TxOut};
use dashcore_hashes::{sha256d, Hash};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In dash-spv/src/validation/instantlock.rs around line 4, the top-level import
use dashcore_hashes::Hash is only needed by tests; remove this unused import
from the main scope and ensure the test module (around line 156) contains use
dashcore_hashes::Hash (or keep the existing re-import there) so the Hash trait
is only imported under #[cfg(test)] and the unused-import warning is eliminated.
Add network-level penalization for peers that relay invalid ChainLocks. When an invalid ChainLock is detected, the peer that sent it receives:
Changes:
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
--no-mempoolcommand-line flag to disable mempool transaction tracking.Bug Fixes
Changes